-
Notifications
You must be signed in to change notification settings - Fork 753
Download rework #2037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Download rework #2037
Conversation
app/src/main/java/com/lagradost/cloudstream3/services/DownloadQueueService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lagradost/cloudstream3/services/DownloadQueueService.kt
Outdated
Show resolved
Hide resolved
| .data(url) | ||
| .apply { | ||
| headers?.forEach { (key, value) -> | ||
| extras[Extras.Key<String>(key)] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headerBuilder["User-Agent"] = USER_AGENT
64fd296 to
66ea1b4
Compare
app/src/main/java/com/lagradost/cloudstream3/ui/download/DownloadFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lagradost/cloudstream3/ui/download/queue/DownloadQueueFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lagradost/cloudstream3/ui/download/queue/DownloadQueueAdapter.kt
Outdated
Show resolved
Hide resolved
|
The queue should now work regardless of how you try to interrupt it. Please test it @fire-light42 @Luna712 |
Luna712
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thank you. |
|
Fixed 👍 |
Luna712
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more issue, R.id.navigation_download_queue needs to be added to
cloudstream/app/src/main/java/com/lagradost/cloudstream3/MainActivity.kt
Lines 561 to 563 in 1a852f1
| when (destination.id) { | |
| in listOf(R.id.navigation_downloads, R.id.navigation_download_child) -> { | |
| navRailView.menu.findItem(R.id.navigation_downloads).isChecked = true |
It is such a weird issue because the mutex is surrounded by a try catch. I will try to fix it.
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.
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.
I thought I fixed that issue, but I suppose not. Thank you for reporting it.
Will fix 👍
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 think the state is not properly set. I will try to fix. Thank you for your review ❤️ |
That makes sense and yeah I wasn't sure.
Same as above
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.
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:
Potentially more also as I didn't go through every one. |
|
Changes:
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. |
fire-light42
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool pull request, the code looks very clean but I have not yet tested it.
app/src/main/java/com/lagradost/cloudstream3/ui/download/queue/DownloadQueueViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lagradost/cloudstream3/services/DownloadQueueService.kt
Show resolved
Hide resolved
b218f67 to
3bb8998
Compare
|
Rebased onto master and fixed various small issues, including the animations. |
|
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. |
Excellent feedback! I pushed a fix which should resolve this issue, please test to see if it works. |
|
Awesome thanks, I'll do another in depth test on this tomorrow. |
Good idea. I will do that 👍
Downloads should not start from backups. I will also add relevant keys to
I initially considered this out of scope for the pull request. However, it would be easy to fix. I will add it.
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?
Good idea. I will add it.
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.
Same reasoning as above. Good to have as a separate pull request.
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.
I had that before but it was too laggy to keep. Scrolling with 20 animated texts was a terrible experience.
Will fix. Thank you a lot for another well thought out review 😄 |
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.
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:
|










Changes the whole download system to:
Remaining issues to fix:
I plan to complete this pull request this week.