recloudstream
Download rework
Changes the whole download system to:
- Add a user visible download queue
- Migrate away from WorkManager (since it does not work for everyone)
- Add instantaneous download button feedback
- Add batch downloading
Remaining issues to fix:
- [x] Batch download button (easy)
- [x] Fix the resume download button (easy)
- [x] Add link loading notification (easy)
- [x] Good download queue UI (hard)
- [ ] Quality assurance
I plan to complete this pull request this week.
The queue should now work regardless of how you try to interrupt it. Please test it @fire-light42 @Luna712
Thank you. I did a lot of testing but only in series. I forgot the movies.
Here is a bit more comprehensive review with more issues I've found. Though still not done testing completely.
-
Managed to hit a crash:
-
Cancel all queued downloads doesn't cancel in progress or paused downloads (not sure if intentional or not)
-
Not sure if this is intentional either, but 1) you can't drag one from below the separator to above the separated to start that download immediately and requeue one above it. Additionally if you pause the 3 parallel downloads others don't start while those are paused. Though this may be intentional behavior.
-
When I click the delete file on an in progress downloads while in the queue, it doesn't remove them from the adapter (or more likely view model because even if I close and reopen the fragment they still remain there).
-
A bit inconsistent/odd padding for the download all button:
Perhaps the UI could be simplified by condensing into a download all icon? Not 100% sure though.
- When I click download all, the icons immediately change, but clicking them doesn't give an option to cancel them until it finishes finding the links for them. Also maybe a cancel all button should be added if all currently queued to reverse that download all action?
- Some animations randomly stop spinning. Not sure if that is just due to recyclerviews though:
- Managed to hit a crash
It is such a weird issue because the mutex is surrounded by a try catch. I will try to fix it.
- Cancel all queued downloads doesn't cancel in progress or paused downloads (not sure if intentional or not)
This is intentional and I tried to clarify that using the wording in the popup. The queued downloads are instantaneous and easy to readd, making them of lower importance. While active downloads, which require link loading, are presumed to be more important, requiring explicit intent to cancel or pause.
- Not sure if this is intentional either, but 1) you can't drag one from below the separator to above the separated to start that download immediately and requeue one above it. Additionally if you pause the 3 parallel downloads others don't start while those are paused. Though this may be intentional behavior.
All of this behavior is intentional, although some of it may change in a future update. While it would be nice to move the separator or active downloads it would complicate the system, pull request and UX too much. Future pull requests may try to improve this, this pull request is big enough already.
- When I click the delete file on an in progress downloads while in the queue, it doesn't remove them from the adapter (or more likely view model because even if I close and reopen the fragment they still remain there).
I thought I fixed that issue, but I suppose not. Thank you for reporting it.
- A bit inconsistent/odd padding for the download all button. Perhaps the UI could be simplified by condensing into a download all icon? Not 100% sure though.
Will fix :+1: Netflix designs the button with an additional line in the download icon, maybe that would be nice to have.
- When I click download all, the icons immediately change, but clicking them doesn't give an option to cancel them until it finishes finding the links for them. Also maybe a cancel all button should be added if all currently queued to reverse that download all action?
I will add a click to cancel popup to each. However, I do not want to add more buttons to the result fragment. It already lacks space. A cancel all button in the queue menu is enough in my opinion.
Some animations randomly stop spinning. Not sure if that is just due to recyclerviews though.
I think the state is not properly set. I will try to fix.
Thank you for your review :heart:
This is intentional and I tried to clarify that using the wording in the popup. The queued downloads are instantaneous and easy to readd, making them of lower importance. While active downloads, which require link loading, are presumed to be more important, requiring explicit intent to cancel or pause.
That makes sense and yeah I wasn't sure.
All of this behavior is intentional, although some of it may change in a future update. While it would be nice to move the separator or active downloads it would complicate the system, pull request and UX too much. Future pull requests may try to improve this, this pull request is big enough already.
Same as above
I will add a click to cancel popup to each. However, I do not want to add more buttons to the result fragment. It already lacks space. A cancel all button in the queue menu is enough in my opinion.
I wasn't necessarily thinking another button but rather replace the download all with cancel all if all in progress or delete all if all downloaded in that available batch, but the other button works too. Was just an idea, but isn't actually necessary for this PR. I think the first part of this I mentioned where you can't cancel at all until it finishes finding links is probably a bug though that could be fixed but not certain if that was intentional as well or not.
Thank you for your review ❤️
Of course, this PR really works amazing. Downloads no longer lag the entire app when in progress and the UI especially queue and instant download feedback is amazing.
Just for the record to keep track of some issues this should at least in theory improve or fix:
- #372 (fixes)
- #399 (fixes)
- #621 (fixes)
- #1089 (I think this fixes this one, though kinda confused what that one is asking)
- #1118 (at least improves not sure if it resolves completely)
- #1166 (fixes)
- #1351 (at least improves not sure if it resolves completely)
- #2182 (at least improves not sure if it resolves completely)
- #2224 (at least improves not sure if it resolves completely)
- #2268 (fixes)
- #2269 (fixes)
Potentially more also as I didn't go through every one.
Changes:
- Replaced the download all button with an icon.
- Added a text to view the queue size at a glance from the download fragment
- Likely fixed the crash
- Likely fixed the issue of deleted downloads remaining in the queue
- Added redundant key removal (will make the logcat much cleaner)
I was not able to fix the animations by changing the button code, even when I changed setStatusInternal to always start the animation and never cancel the animation it still stopped. I suppose we must use ItemAnimator to manage the animations.
Rebased onto master and fixed various small issues, including the animations. Please test and review @fire-light42 @Luna712
One major issue I found with this is that removing keys should probably be done using batch like how search history and backups are done. When I did this and restored from backup of pre I have it logged in logcat about 18,000 times removing keys, which froze the app and it never finished leading to a crash (app not responding) loop.
One major issue I found with this is that removing keys should probably be done using batch like how search history and backups are done.
Excellent feedback! I pushed a fix which should resolve this issue, please test to see if it works.
The key deletion issue seems fixed. But maybe add the backup keys to nonTransferableKeys? Backup and restore and backup immediately again caused backup file to grow a bit.
The issues/suggestions I've found so far now is:
Major issue is that backup while there is in progress/queued downloads, then clear app data and restore tries to re-download. While it sometimes works, it won't always work due to storage permissions and should be excluded from backup. It seems to only happen with the 3 in progress downloads but I'm not certain if there is some case it could try all queued also.
While links are loading there is absolutely no way to cancel a download. Not from the queue and not from the series page. Long clicking the loading animation download icon just opens sources again (probably fine for that one) and in the queue clicking it does nothing. While this was probably the case before, the instant feedback makes it more obvious and not clear that you cant cancel it. Probably allowing to cancel a download in the queue before link loading is finished would be good.
When you delete an in progress download, it does now remove from the adapter but it takes about 15 seconds sometimes but other times it was very quick so may not be an issue and just my device.
Perhaps there should be some UI indicator for no downloads in queue other then just a blank page? Not sure.
I'm not certain if it does, but perhaps preload links for next item in queue when its close to being active (like when current active are almost done) just a suggestion perhaps a later follow up.
Maybe not for this PR not sure, but better handling for failed downloads would probably be good. I tested this by batch downloading something with 26 very short and small episodes, then put my phone in airplane mode. The UI flickers for a few seconds while all queued downloads are instantly failed and removed from the queue while the 3 that were actually running are also removed from the queue, but still appear as half downloaded in the actual child list. Also when the downloads failed all downloads in queue created 0 byte files that remained on the device.
Again, probably not for this PR, but downloads should probably honor the source priority system or something because that is made worse with batch downloads, whereas it ends up downloading like 4K first which takes like 30GB sometimes.
Maybe this (and the subtitle if it exists) should auto scroll to eventually show the full title? Also when there is no subtitle the title doesnt quite align with the icon.
The key deletion issue seems fixed. But maybe add the backup keys to nonTransferableKeys? Backup and restore and backup immediately again caused backup file to grow a bit.
Good idea. I will do that :+1:
Major issue is that backup while there is in progress/queued downloads, then clear app data and restore tries to re-download. While it sometimes works, it won't always work due to storage permissions and should be excluded from backup. It seems to only happen with the 3 in progress downloads but I'm not certain if there is some case it could try all queued also.
Downloads should not start from backups. I will also add relevant keys to nonTransferableKeys to ensure it does not happen.
While links are loading there is absolutely no way to cancel a download. Not from the queue and not from the series page. Long clicking the loading animation download icon just opens sources again (probably fine for that one) and in the queue clicking it does nothing. While this was probably the case before, the instant feedback makes it more obvious and not clear that you cant cancel it. Probably allowing to cancel a download in the queue before link loading is finished would be good.
I initially considered this out of scope for the pull request. However, it would be easy to fix. I will add it.
When you delete an in progress download, it does now remove from the adapter but it takes about 15 seconds sometimes but other times it was very quick so may not be an issue and just my device.
The queue directly reflects the internal state of the download manager. If the download stays for 15 seconds it may reflect a mistake in the download management. Debugging this may fix some hidden issue. However, I have not managed to recreate the issue. Are you able to recreate it?
Perhaps there should be some UI indicator for no downloads in queue other then just a blank page? Not sure.
Good idea. I will add it.
I'm not certain if it does, but perhaps preload links for next item in queue when its close to being active (like when current active are almost done) just a suggestion perhaps a later follow up.
An excellent idea, but better to have as a separate pull request after this one. Any additional features or changes to the internals may create new bugs which would be hard to catch in an already huge pull request. I also want to prevent scope creep to ensure this pull request gets merged at all.
Maybe not for this PR not sure, but better handling for failed downloads would probably be good. I tested this by batch downloading something with 26 very short and small episodes, then put my phone in airplane mode. The UI flickers for a few seconds while all queued downloads are instantly failed and removed from the queue while the 3 that were actually running are also removed from the queue, but still appear as half downloaded in the actual child list. Also when the downloads failed all downloads in queue created 0 byte files that remained on the device.
Same reasoning as above. Good to have as a separate pull request.
Again, probably not for this PR, but downloads should probably honor the source priority system or something because that is made worse with batch downloads, whereas it ends up downloading like 4K first which takes like 30GB sometimes.
This is something I want myself. However, I want to keep the core download logic as similar as possible to before. It would be one of my first pull requests after this one.
Maybe this (and the subtitle if it exists) should auto scroll to eventually show the full title?
I had that before but it was too laggy to keep. Scrolling with 20 animated texts was a terrible experience.
Also when there is no subtitle the title doesnt quite align with the icon.
Will fix.
Thank you a lot for another well thought out review :smile:
The queue directly reflects the internal state of the download manager. If the download stays for 15 seconds it may reflect a mistake in the download management. Debugging this may fix some hidden issue. However, I have not managed to recreate the issue. Are you able to recreate it?
I can only reproduce it if I immediately cancel almost as soon as it starts and when there are 3 in progress at once, and I try to cancel two or all three in quick succession. It is also much more obvious if I delete all 3 at once by deleting the entire series using multi-delete, while 3 are in progress then go back into the queue and they remain there for 15 or so seconds longer.
I had that before but it was too laggy to keep. Scrolling with 20 animated texts was a terrible experience.
Oh yeah that makes sense in that case totally not worth it.
I found another bug as well. I enabled 10 parallel downloads and the app eventually crashed with an OOM (expected to be honest) but the issue is when it restarted into safe mode and downloads resumed, some downloads that were previously active are now queued, the queue order is changed to some random order and only a few downloads start simultaneously while others in queue are paused and those above the parallel downloads won't download and does like one at time. It's honestly hard to explain the behavior but it acts quite odd. But this is also such an edge case, and I think it always happened it just wasn't visible before so isn't necessarily something to fix with this PR.
The final issue I've found is when downloads are completed they aren't always removed from the queue sometimes they stay in the queue while the icon is the completed icon. There is a potential this ties into the above issue as well, since I noticed this issue at the same time as well.
EDIT For that second issue its like its supposed to be downloading but isn't, it doesn't appear at all in main downloads, the queue says "8/8", and clicking the download completed icon does nothing. Clear queue does nothing either. They are still there after restarting the app also. The notification also still says "8 active downloads". There seems to be no way to remove them at all, making that potential issue a larger issue then I first thought.
EDIT 2 This also seems to be the actual reason the first issue happens. Because there was these 8, they take up the 8 threads somehow so only 2 other downloads were actually working in parallel.
EDIT 3 The episodes did fully download, the full files exist, but it's like they weren't moved from the queue to completed states when they should have.
On another note, amazing work with this PR downloads work so smoothly without lagging the entire app now also, before the OOM there was zero lag with 10 ongoing downloads, before it barely worked with 2 without lagging much.
Fixes:
- No automatic queue in safe mode
- More reliable queue management
- Now possible to cancel loading links
- Empty queue message
- Centered text for queue items when no subtitle
- No download data in backups
New bugs I've found now:
- Download something, then while links are still load go to queue and cancel it, then return to the results fragment for the download you canceled, now a download failed and clicking it still shows cancel option. Additionally, I tried this a second time and when canceling in queue for an episode, it showed the failed icon and remained in the queue. Queue remained showing 1/1, and exiting and reentering does not clear it.
- The safe mode bug is mostly fixed, however when I triggered it again, then exited safe mode by restarting apps, the downloads that were running already were automatically resumed but ever other queued download was removed from queue and only active ones resumed again. Additionally while in safe mode the downloads notifications just appear frozen, downloads appear paused but the notifications remain as if they are frozen rather than any indication of being paused or anything and the cancel button doesn't work.
- Some notifications seem to actually get stuck (might be related to the safe mode bug) I have multiple download episodes, that is still a notification but no longer in the queue at all. Most sitting at 99%, but a couple at like 40 or 50% even though rhey are complete, in some cases or perhaps canceled in other cases? A couple of them ended up in main downloads with just a download icon:
EDIT one more thing, subtitles are not deleted when download is canceled after it actually started, which while an existing issue also from what I can understand from the code this does have some logic to delete subtitles when canceled also, so thought I'd mention it.