Support for multi deleting downloads and other major improvements/fixes

[Luna712]

TODO:

• [x] Support deleting all episodes in a series
• [x] Support multi deleting in child fragment (multi-deleting episodes)
• [x] Update DownloadButtonSetup long click stuff to remove the unnecessary toasts and instead just handle multi delete
• [x] If currently in 'multi-delete mode', clicking on an item should not play the media but toggle its deletion state
• [x] Fix a UI bug with the AppBarLayout's always having one or the other never be visible until you start scrolling down
• [x] Fix a bug where if you scroll down about 25 items some of the items at the top start getting deselected
• [x] Fix a bug that only happens sometimes where if you long press to select an item, then click the cancel button, then long click the same item it will select and immediately deselect again
• [x] Add a 'select all' button
• [x] Make the back button close 'multi-delete mode' if it is currently active
• [x] A few more UI/UX improvements, maybe utilizing some snack bars or toasts
• [x] Improve formatting of confirmation dialog, to better differentiate series and others
• [x] Add total bytes selected for deletion to delete app bar
• [x] Make setOnLongClickListener toggle, so if it is already selected, another long click should deselect
• [x] Don't unexpectedly exit 'multi-delete mode' if you manually deselect all items, instead add a message in app bar to select items, and make the 'Select All' possible to become a 'Deselect All'
• [x] Move updating child lists to the view model
• [x] Fix a bug where the download storage app bar is hidden when there are no downloads
• [x] Fix a bug where the download storage app bar and delete app bar both overlap each other after actually deleting and it updates the storage data in view model which reshows that, after deleting 'multi-delete' mode should deactivate
• [x] Fix bug where if you select all (manually or with the button) then manually deselect any, the deselect all button still appears and doesn't go back to select all, unless you actually click the deselect all button
• [x] Improve multi-delete dialog for child downloads, mention season and episode, and make dialog mention episodes — we should use context.getNameFull() here
• [x] Unset downloadDeleteEvent if we are multi-deleting so we don't end up running update on the lists for every single download
• [x] Add better support for single deletions in dialog message
• [x] Use view model for single deletions
• [x] Faster update view model lists
• [x] Further performance improvements in view model
• [x] Fix updating child list on deletion
• [x] Fix layout crash on Emulator and TV
• [x] Fix focus issues on TV
• [x] Fix potential race condition
• [x] Fix paused download UI bug when adapter is refreshed
• [x] Fix completely random inconsistent sorting in downloads
• [x] Fix incorrect usedBytes
• [x] Fix bug where if you delete a child episode with multi-delete, and re long click, the checkbox does not get checked properly
• [x] Fix deprecations in Context.isNetworkAvailable()
• [x] Fix a pre-existing bug where if you delete an item, a random item somewhere below it has it's bytes disappear
• [x] Fix UI delay with download icons
• [x] Make sure matching subtitles are also deleted (closes #185)

Code-cleanup:

• [x] Rename selectedChangedCallback in adapter to the cleaner and more descriptive onItemSelectionChanged — also should rename other callbacks to this format as well
• [x] Remove unneeded multiDeleteStateCallback (we can just use selectedChangedCallback/onItemSelectionChanged)
• [x] Don't use notifyDataSetChanged()
• [x] Add a BackPressedCallbackHelper to unify back pressed logic
• [x] Add a SubtitleUtils for more shared code to reduce duplication
• [x] Remove usage of selectedIds in adapter in favor of using view model
• [x] Only use IDs for storing in view model
• [x] Massive cleanup to DownloadViewModel
• [x] Remove use of requireContext()
• [x] Rename updateList() to updateHeaderList() to match updateChildList() format
• [x] Use Set() for ids since they should be unique
• [x] Pass UiText in SnackbarHelper
• [x] Remove unneeded TODO in DownloadChildFragment
• [x] Fix comments for isMatchingSubtitle

I have tested this with up to 30 random videos from invidious and two different series with 4 episodes each, all at once, and it does appear to delete them all properly, along in their subtitles, and in about the same amount of time as just deleting one with the pre release version takes for me.

Follow-up patches

Collapsed for cleaner description here since these don't necessarily have anything to do with this patch.

• Make sure series sub-folders themselves are completely deleted if all child downloads are deleted
• Check network connection before trying to load online results
• Navigate to downloads by default if there is no network connection, and you have downloads
• Maybe remove the old delete method eventually when this method becomes more known, or some sort of edit button added to UI, similar to Netflix
• Add an animation when an item is selected, similar to Netflix — I would do this in this PR but I don't know how yet, I am working on it and this will likely be a follow-up patch instead
• Add support for the watch progress bar on header cards
• Should the UI for this be approved here, follow-ups will include multi select (using similar UI) for batch download, and batch mark as watched
  • Mark as watched will be first as it's easier than batch downloads due to how the download queuing system works and is in need of further improvement first
  • Batch downloads will mean a major update to the performance and functionality of downloads, improving how they work, and applying some of #1166 (which even before that issue went up was part of my plans)
  • Basically all those in queue will need to be made clear that they are and for failed downloads to work better
  • I would also probably apply #1185 for Downloads before batch download because it can cause a significant issue if you duplicate download due to naming conventions of Downloads. If you duplicate download and the name is the same, the old download is automatically deleted without confirmation. For an even large batch download this is a bigger issue
  • The queue needs fixed so 3 can simultaneously be started regardless in the order they were started in. As it stands after you start 3 downloads, when you queue the fourth, you have to wait for whatever one you started third to finish, then the fourth has to finish before the fifth starts, etc... this needs fixed before we go to true batch downloading as well

[fire-light42]

I believe this is best done in the viewmodel as otherwise the views will recycle when scrolling or switching the episode ranges ect.

[Luna712]

I believe this is best done in the viewmodel as otherwise the views will recycle when scrolling or switching the episode ranges ect.

@fire-light42

I thought about that, but I figured this would work to do through the adapter, and I wasn't sure how best to actually implement something through the view model. But I agree that would be best also. The whole reason I actually passed scope to the delete method was to pass in a view model scope, which would be best. But then I couldn't figure out how to actually implement it through the view model. But I can definitely give another shot at it. Thanks!

Also, one more note, the reason I even switched single deletes to use the new method passing one ID as a listOf() is because it seemed to actually have better performance. But maybe that was just in my mind. The old method, when monitoring some old logcat, seems to have some sort of OOM when used as a standalone, and this seemed to be less likely to OOM, but I'm also not certain about this 100%. But I did test that part that it works, which also confirms the actual function works, but I will attempt to move the multi delete activation code to the view model.

Also, just an FYI, I will do the actually multi-delete functionality in this PR and UI in another PR when I build it. I'd rather split it because that way, it's easier to review and less likely to be buggy, in my mind anyway. But if you'd rather me do it differently, I can try as well.

[Luna712]

Since I have a working UI that just needs more improvement I have decided to just do this all at once.

[Luna712]

@fire-light42 for the most part this works other than those TODOs however for the life of me I can not figure out that UI bug mentioned above and was wondering if you have any ideas/suggestions on that...

EDIT: I FINALLY figured the UI bug out, turns out it is some sort of Android bug, but there is a workaround the seemed to work...

[Luna712]

@fire-light42 other than deleting folders (which I am not sure what the best path for that is) I think this is ready for review. I do still plan to finish folders somehow, and you may want me to tweak UI which is fine if so as well, and I can do that.

EDIT: I found a few more things that still need to be worked on.

[Luna712]

Hmm maybe I should make it more like Netflix, remove the 'select all', add a 'delete all downloads' somewhere else, and move the delete button to an icon at the end. I personally like how clean that looks but maybe this is okay for now, that would be a but more complicated to add the 'delete all downloads' thing.

[fire-light42]

This is quite a big PR, and looks like to actively developed so I will leave a detailed review and comment next week. If you have not already I would suggest expanding multi-delete to multi-select for batch download and mark as watched in resultspage.

[Luna712]

This is quite a big PR, and looks like to actively developed so I will leave a detailed review and comment next week. If you have not already I would suggest expanding multi-delete to multi-select for batch download and mark as watched in resultspage.

This doesn't do that. I thought about it but because it's different view models, etc... it wouldn't be the same (I think?), and I thought because this is already a large PR that should be done separately...

[Luna712]

@fire-light42 other than whatever you find in your review and maybe a bit of code cleanup, functionality wise this should be 100% done.

I have tested this now with 30 random videos from invidious and two different series with 4 episodes each, all at once, and it does appear to delete them all properly.

UI/UX wise I think I am done as well, the UI seems to work without issue when testing. I tried my best to not make it seem crowded or out of place so-to-speak, but if you have any suggestions there I am open to it as well.

Another thing I had to do is migrating the child updateList method to view model to make items selection work properly. That part may need more extensive testing.

Like you already said, this is a pretty big PR (and even bigger now than when you said it) so I am almost certain I did mess up somewhere. I also don't want to complicate it by doing more changes unrelated to multi delete for downloads.

I also introduced SnackbarHelper with this PR, which also fixes another style bug with the snackbar used in MainActivity, as well as switches the toast in DownloadButtonSetup to a Snackbar because it looks nicer for a bit longer text, and the old simple one-word toasts actually made no sense. So this uses a snackbar as it makes it look a bit nicer.

[Luna712]

This now also deletes subtitles when deleting files

[Luna712]

I don't know why CodeFactor is failing

[Luna712]

Okay this is finally 100% done other than whatever is found in review.

[Luna712]

The UI is great! But I have some comments about the code.

1. Multi selecting episodes that are paused and then unselecting leaves them with zero progress. This is a UI bug that needs to be fixed.

Thanks for reminding me. This one I did notice once but forgot about.

2. Deleting a fully downloaded episode by multi selecting it and deleting it will not make it disappear. I have to delete it twice???

Hmm thats interesting, I didn't have that issue when testing

3. I dont like that you store the selectedItems as VisualDownloadCached, use the Ids instead and a set to make duplication impossible

I did have it as ids initially but found it to have better performance and reliability to store the full object and access that as the data when getting for the dialog, otherwise it would sometimes take longer and cause more memory to be use which actually led to some OOMs on lower-memory devices when testing. Which I wasn't 100% sure why but going with this seemed to solve that I can try to go back to the other way if you really want though?

4. I dont like that you store the state of the selectedIds inside the adapter, as that can be recreated at any time. Use the viewmodel as the source of truth.

I did this originally but it caused an issue where when you select an item, sometimes as you scroll down the adapter forgets the view model due to recycling and unchecks it while the view model still knows of the item and makes the displayed count different from what the adapter knows. The whole point of storing it in the adaprer though is so that it is maintained when it is recreated or the views are recycled, I did think of adding a selected state to VisualDownloadCached (and actually did at one point), but similarly to above that caused an even further delay and performance cost as it meant updating the entire VisualDownloadCached rather than just what we actually need which is why I chose to go with the adapter method. Although I can again try another way if you wish?

[fire-light42]

3. I dont like that you store the selectedItems as VisualDownloadCached, use the Ids instead and a set to make duplication impossible

I did have it as ids initially but found it to have better performance and reliability to store the full object and access that as the data when getting for the dialog, otherwise it would sometimes take longer and cause more memory to be use which actually led to some OOMs on lower-memory devices when testing. Which I wasn't 100% sure why but going with this seemed to solve that I can try to go back to the other way if you really want though?

4. I dont like that you store the state of the selectedIds inside the adapter, as that can be recreated at any time. Use the viewmodel as the source of truth.

I did this originally but it caused an issue where when you select an item, sometimes as you scroll down the adapter forgets the view model due to recycling and unchecks it while the view model still knows of the item and makes the displayed count different from what the adapter knows. The whole point of storing it in the adaprer though is so that it is maintained when it is recreated or the views are recycled, I did think of adding a selected state to VisualDownloadCached (and actually did at one point), but similarly to above that caused an even further delay and performance cost as it meant updating the entire VisualDownloadCached rather than just what we actually need which is why I chose to go with the adapter method. Although I can again try another way if you wish?

For 3, I would still like to make it impossible to have any sort of duplicates/illegal state. OOM just sounds like you did something else strange. For 4. No, the adapter can also be recreated if oncreateview is called due to background killing, switching tabs or if we choose to recreate them on rotation. The viewmodel should be the single source of truth as it is unbound of the android lifecycle.

[Luna712]

For 3, I would still like to make it impossible to have any sort of duplicates/illegal state. OOM just sounds like you did something else strange. For 4. No, the adapter can also be recreated if oncreateview is called due to background killing, switching tabs or if we choose to recreate them on rotation. The viewmodel should be the single source of truth as it is unbound of the android lifecycle.

I'm sorry maybe I just don't understand lol, why would it be any more possible for duplicates if we store the whole thing (which still includes the ID just also allowing direct access to other fields) wouldn't that even make it more unique ti also make it unique by every other field as well (even though the ID field is always unique, or should be) which would make it a unique object for each new ID anyway, just a cleaner method (in my opinion) to access other fields rather than rebuilding the object when we already have it built? That just seems like redundant operations and isn't the best method.

I apologize if I just misunderstand.

Re adapter, I can try something else but also maybe I explained wrong the reason we do use adapters is we don't want it to be retained when oncreateview is called again, as it leads to outdated data, which is why we also clear the view model at that point, this just seemed more efficient than also having to re-notify the adapter of changes as well. Nonetheless I do understand what you are saying and this is also the one part of this PR I didn't like myself. So I will see if I can make something else work here. Thanks!

[Luna712]

@fire-light42 so I again tried to let the view model solely handle the selected items. Here is the behavior that ends up happening:

1. You have a lot of items and scroll down like to the end
2. Views are recycled
3. You scroll back up
4. The checked states are lost
5. They end up getting rechecked, but there is a visible UI delay for about 2-3 whole seconds before they are, creating an inconsistent UI state.

I will keep trying to achieve this though, but do you have any ideas how I can do it better? To tell you the truth, I know I am probably doing this completely wrong.

Also to note, I forgot to mention in a comment in code (which I can rectify) but after struggling with issues like this for a long time, I did some research, and the method I used was based on https://medium.com/@wambuinjumbi/how-to-save-checkbox-state-in-a-recylerview-60e1e44bdcaf

The issue is once a user selects and scrolls up and down, the checkbox becomes unchecked.

Which is the exact problem I also kept having here... no matter what or how I did it, which is why I did it this way, even if I didn't like how I had to do it.

[Luna712]

Also I guess that download icon bug is some other unrelated bug I can not figure out, whenever the adapter is notified of any changes it is lost, meaning I think it is the same cause where (even before) if you delete a download then it disappears the status a few random places below it due to the rebinding of positions. I have no idea how to fix it...but I will keep trying

[Luna712]

I also still can't reproduce the bug that you had to delete twice? Can you provide exact steps that led to that circumstance? Like how many total downloads you have, is it is an episode, or header type, etc... because it has never happened for me.

[Luna712]

I FINALLY figured out how to make the selection states managed fully by the view model!

[Luna712]

@fire-light42 I have Luna712#4 which makes it only use ids then rebuilds the item from there to get other data we need, I can push that to the main branch here if you really want but it is my opinion it is nicer to do it like it is as it is more direct (and the other method also appears to have a bit worse performance?)

[fire-light42]

(and the other method also appears to have a bit worse performance?)

I very much doubt that you get more than a ms of difference in performance. But yes, I would like you to use the ids instead as I dont like duplication of data, because it makes it a lot easier to introduce bugs or memory leaks.

[fire-light42]

I also still can't reproduce the bug that you had to delete twice? Can you provide exact steps that led to that circumstance? Like how many total downloads you have, is it is an episode, or header type, etc... because it has never happened for me.

Quite literally just fully download something. I assume this is because of some race condition with keys or io.

[Luna712]

I also still can't reproduce the bug that you had to delete twice? Can you provide exact steps that led to that circumstance? Like how many total downloads you have, is it is an episode, or header type, etc... because it has never happened for me.

Quite literally just fully download something. I assume this is because of some race condition with keys or io.

@fire-light42

Yeah, I delete something and it only has to be deleted once so I don't understand lol

[Luna712]

@fire-light42 anything in logcat tagged with "FileDeletion"? And does it actually delete the file from the system just not remove it from adapter or what? I really don't understand this issue lol... I tested this probably literally close to 100 times and I have never encountered such an issue so it's super bizarre to me that it happens for you.

[Luna712]

@fire-light42 I finally figured out that UI bug as well which should be fixed

[Luna712]

@fire-light42 I tried a fix for a race condition but I still could not reproduce, do you mind testing and let me know if it works?

[fire-light42]

I will investigate tomorrow and do another review.

[fire-light42]

1. The issue with deleted episodes still exists: This is caused by safeFile reporting a failure to delete the file while the file is actually deleted (val isFileDeleted = info.toFile(context)?.delete()) due to outdated security permissions or something. While I can hardly blame you for this, nor need you to fix this, be aware that it exists. Updating download path fixes this.

2. You have also the cache invalidation bug where the progressview updates the progress, but it is reset back due to the viewmodel.

Before select and unselect: After:

(This is not related the the other issue with progress being reset when pausing I wrote last time, as this does not require you to restart the app)

3. The other issue I found was that the views take some time to load in, as in that they show the download button for like 0.1s instead of waiting. I consider this really bad, especially because this happens every time you deselect items.

https://github.com/user-attachments/assets/8fd586e9-0236-4c47-af6a-b31f355831ea

I require all of these to be fixed to merge this PR.

[Luna712]

You have also the cache invalidation bug where the progressview updates the progress, but it is reset back due to the viewmodel.

I am pretty sure it is the same underlying cause you mentioned before it is just that the views are updated here whereas they are updated after app is restarted in results. I can see what I can do here though.

3. The other issue I found was that the views take some time to load in, as in that they show the download button for like 0.1s instead of waiting. I consider this really bad, especially because this happens every time you deselect items.

That will only happen with paused downloads, it won't with done downloads. It is not related to this either, it does happen when you first load downloads (currently as well) before it updates from the delayed IO operations, if you have a paused download towards the top, and when you first navigate to downloads the same issue will be noticed. The issue is when views are loaded it gets the IO operations again to reset progress, however I may be able to do it so when unselected it delays UI updates, but that is also bad in my opinion as it causes it to lag before it exits multi delete state. I am not sure the best way to fix this, if you have any ideas it would be appreciated as well.

[fire-light42]

I am not sure the best way to fix this, if you have any ideas it would be appreciated as well.

I would consider the current prerelease to have a better download status/update. Even if it takes some time to load, it does not run on the main thread nor does it show invalid state. If this is a real problem then you can just add some loading animation.