search

quran / quran_android

a quran reading application for android
http://android.quran.com
GNU General Public License v3.0
2.02k stars 892 forks source link

ability to delete audio files for qaris #1011

Closed ahmedre closed 4 years ago

ahmedre commented 5 years ago

this should likely live in the download manager

ahmedre commented 5 years ago

@murtraja want to try this one?

ahmedre commented 5 years ago

this is when you go to settings -> download manager. today, we show a large list of the shuyookh and how many files you downloaded for each one. if you click any of them, it starts downloading the rest of the shuyookh.

this would be to change it so clicking goes into a screen with the list of suras. there'd be a download all button somewhere (toolbar probably). clicking a sura that is not downloaded would start downloading it. clicking one that is downloaded would prompt for deleting and delete it.

murtraja commented 5 years ago

@ahmedre, sure. I have no experience whatsoever with Dagger or RxJava (or even Android Apps, actually), so I will simply follow suit. I would love to have a thorough review of the PR.

Initially, I am planning to do this:

clicking a sura that is not downloaded would start downloading it.

But I would really love if the user could "create a transaction" and hit run to either download and/or delete, whatever the user selected. (Just like how gparted does it when creating and deleting partitions, or how a shopping cart works, or how the Android SDK manager does it) instead of blocking the UI with downloading progress for just one Surah

P.S. BTW, why is the back button on the toolbar not taking me back but the hardware back button does here?

ahmedre commented 5 years ago

sounds good in sha' Allah. the service does the downloads in the background and doesn't have to block the ui (even if it is blocking it right now).

for the bug, i think i must have forgotten the code to handle R.id.home click.

yaaminu commented 5 years ago

Salam Alaikum brothers, is this issue still active? I would love to give it a shot if that's the case inshallah

murtraja commented 5 years ago

@yaaminu, I have completed adding UI screens, can you add the onClick functionality? I will update the repo and will inform you

murtraja commented 5 years ago

Screenshot_1554990025 The UI isn't much really and it would be better if you started the implementation from scratch, using this repo, if needed

yaaminu commented 5 years ago

Alright @murtraja

murtraja commented 5 years ago

@yaaminu , I am stealing this back from you :P @ahmedre , I plan to finish this in three stages

  1. Complete the feature functionally - Individual deletion/download, blocking
  2. UI changes - changing icons, UX discussion, etc.
  3. Transactional download/delete - non-blocking
ahmedre commented 5 years ago

sounds good - jazakAllah khairan. one request - let's use the contextual action bar for the batch modes

murtraja commented 5 years ago

@ahmedre, in the current implementation of AudioManagerActivity, it is expected that, once the download is successful, we again fetch ShuyookhData so that the UI is updated. But, the onSuccess callback of Observer is not triggered again and consequently UI is not updated. On reading the docs here (Like all other consumers, DisposableObserver can be subscribed only once.) I realized that this is intended behaviour.

So therefore, instead of having the observer as a member variable (mOnDownloadInfo) shouldn't we recreate it whenever we fetch ShuyookhData?

I am sorry if this is a trivial problem, and if the question is silly, it's just that I have never worked with RxJava before and I do not understand the semantics properly at this point.

getShuyookhData: https://github.com/quran/quran_android/blob/9350171ec85ec8e7f0a5ef7169aa18831ae402df/app/src/main/java/com/quran/labs/androidquran/ui/AudioManagerActivity.java#L90-L97

mOnDownloadInfo (the onSuccess is not triggered again): https://github.com/quran/quran_android/blob/9350171ec85ec8e7f0a5ef7169aa18831ae402df/app/src/main/java/com/quran/labs/androidquran/ui/AudioManagerActivity.java#L123-L135

onSuccess Callback of Download service is triggered properly: https://github.com/quran/quran_android/blob/9350171ec85ec8e7f0a5ef7169aa18831ae402df/app/src/main/java/com/quran/labs/androidquran/ui/AudioManagerActivity.java#L175-L178

ahmedre commented 5 years ago

yes, you have to re-subscribe to the observable again because it only emits one time today (so call getShuyookhData a second time.

murtraja commented 5 years ago

@ahmedre, I am not sure if I have properly explained the problem. We do call getShuyookhData, it's just that we don't recreate mOnDownloadInfo

ahmedre commented 5 years ago

okay good point - yeah this is because it can only be subscribed to once. i think if we remove it altogether and inline it instead, it should solve the problem.

ahmedre commented 4 years ago

this is done now - may Allah reward @murtraja for his work on this!