Fixes of Zim file does not download / pending forever

[MohitMaliFtechiz]

Fixes #3441

We encountered an issue where the download remained pending indefinitely if we canceled it from the notification. This happened because when we canceled the download by clicking the notification cancel button, it kept a reference in the fetch system. Consequently, if we tried to download the same file again, fetch created a new download ID, resulting in a conflict with the existing instance of the file. As a result, the download did not start.

However, when we canceled the download within the app itself, it deletes the instance from fetch, which automatically deletes the temporary file from storage.

To resolve this issue, we have made a change. Instead of using the CANCEL action, we now use the DELETE action. So, if the user cancels the download from the notification, it will function the same way as if they have canceled it from within the application.

downloadcancelfromnotificationissue.mp4

[codecov]

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.01 ⚠️

Comparison is base (73fe388) 48.68% compared to head (842a7ee) 48.67%.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #3451      +/-   ##
=============================================
- Coverage      48.68%   48.67%   -0.01%     
  Complexity      1006     1006              
=============================================
  Files            285      285              
  Lines          10096    10095       -1     
  Branches        1348     1348              
=============================================
- Hits            4915     4914       -1     
  Misses          4398     4398              
  Partials         783      783              

Impacted Files	Coverage Δ
...ava/org/kiwix/kiwixmobile/core/compat/CompatV21.kt	60.00% <ø> (-6.67%)	⬇️
...kiwix/kiwixmobile/core/data/remote/KiwixService.kt	100.00% <ø> (ø)
...wnloader/fetch/FetchDownloadNotificationManager.kt	56.81% <50.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gouri-panda]

@MohitMaliDeveloper @kelson42 LGTM! @MohitMaliDeveloper Thanks! Would be great if we added tests.

[kelson42]

@MohitMaliDeveloper strongly disagree with this PR because what you have developed is a pause/resume feature without calling it appropriatly and hidding it behind the cancel feature. Basically a failed attempt to fix #2093.

Naming things properly is the highest challeng in software engineering, by not doing so you have created new bugs…. And actually the PR does not match the bug reported first. @gouri-panda This PR ahould not have been approved.

An example of bug: user download 1GB of data for a zim then want to cancel. He cancels… but now one temporary data of 1GB is on the disk and he has no way to recover that space. If he do that a few times, he can fill the disk with twmporary data!

Now, @MohitMaliDeveloper please:

• Implement a fix for Zim file does not download / pending forever #3441 and only that
• Move the pause resume code part in a PR (or keep that one) with purpose to implement Reintroduce Download pause/resume #2093, but first propose a UI change so the UX is proporr.

@MohitMaliDeveloper Does all of this make sense or do I’m missing aomething?

[kelson42]

PR does not only fix bug but introduce a feature without having discuss it properly.

[MohitMaliFtechiz]

strongly disagree with this PR because what you have developed is a pause/resume feature without calling it appropriatly and hidding it behind the cancel feature.
Naming things properly is the highest challeng in software engineering, by not doing so you have created new bugs…. And actually the PR does not match the bug reported first.

@kelson42, Yes, we have implemented the pause/resume feature. I properly updated the PR description, taking reference from Gouri's comment to reduce extra network costs. However, I apologize for not updating the PR title. The main issue we addressed was when canceling a download from the notification, we used the fetch cancel function, which cancels the download. If we then attempt to download the same item again, the file is already present in storage and we end up with two download IDs for the same file in fetch. On the other hand, when canceling the download from the application, it deletes the download ID from fetch, which automatically removes the corresponding file from storage.

user download 1GB of data for a zim then want to cancel. He cancels… but now one temporary data of 1GB is on the disk and he has no way to recover that space. If he do that a few times, he can fill the disk with temporary data!

I agree with you. The memory occupied by the canceled download is not recoverable from within the application itself. The user can only recover that memory by manually deleting the file using a file manager.

Move the pause resume code part in a PR (or keep that one) with purpose to implement #2093, but first propose a UI change so the UX is proporr.

In the current UI, when the user clicks on the cancel button, we show a dialog to confirm the cancellation of the download. We can enhance this dialog by adding a new button called "Pause." When the user clicks on the Pause button, the download will be paused, and we will update the UI to display a pause button instead of the stop button. If the user clicks on the pause button, we can show another dialog with two options:
a) Resume the download.
b) Cancel the download, which will delete the files from storage.
However, this approach has a drawback. For instance, if the user is at the bottom or in the middle of the download list and pauses a download, then closes the application. When they want to resume the download later, they would need to find that particular zim file in the list. This approach can be frustrating for the user. To address this issue, we have two additional approaches:

a) Show Paused Downloads at the Top:
We can display the paused downloads at the top of the list, making it easier for the user to locate and resume them.

b) Create a Separate Screen for Paused Downloads:
We can introduce a separate screen dedicated to displaying paused downloads. This way, the user can easily access and manage their paused downloads without having to search for them in the main download list.

[kelson42]

The main issue we addressed was when canceling a download from the notification, we used the fetch cancel function, which cancels the download. If we then attempt to download the same item again, the file is already present in storage and we end up with two download IDs for the same file in fetch. On the other hand, when canceling the download from the application, it deletes the download ID from fetch, which automatically removes the corresponding file from storage.

It's mandatory to always delete temporary file if download is canceled. If not always done, this is a bug and this is what has to be fixed here.

For the rest, this should be done on appropriate ticket, not here.

[MohitMaliFtechiz]

It's mandatory to always delete temporary file if download is canceled. If not always done, this is a bug and this is what has to be fixed here.

@kelson42, I have done the changes as you requested also updated the PR description.

[kelson42]

@gouri-panda can you please review again?

[gouri-panda]

An example of bug: user download 1GB of data for a zim then want to cancel. He cancels… but now one temporary data of 1GB is on the disk and he has no way to recover that space. If he do that a few times, he can fill the disk with twmporary data!

@kelson42 Sorry, I didn't see this edge case. You made a good valid point. Storage is also our priority too. I agree with @MohitMaliDeveloper too, we should have pause, resume and cancel button use cases too. pause will allow to stop downloading files for a while. resume will allow start downloading from the previous state and cancel will allow stop the downloading & delete the whole file from the storage.

We can display the paused downloads at the top of the list, making it easier for the user to locate and resume them.

@MohitMaliDeveloper I think we have this ticket before. Perhaps this is a good time to remove the dependency of the fetch library. Also, we'll benefit the duplicate id problem you previously discussed.

[kelson42]

Not in favour of replacing Fetch now.... not a good timing for such a big change.

[kelson42]

@gouri-panda Thank you for the review.
@MohitMaliFtechiz Before I merge, can you please open a new brqnch from here to fix the oause/resume issue. Otherwise you risk to loose all the code you have done.

[MohitMaliFtechiz]

@kelson42, I have created a PR #3459 with these pause/resume download changes.