Skip to content
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

Add swipe to delete downloads and minor performance improvements #1127

Closed
wants to merge 11 commits into from

Conversation

Luna712
Copy link
Contributor

@Luna712 Luna712 commented Jun 8, 2024

This could honestly be a horrible way to implement this (or maybe you don't want it at all) but figured I would give it a try as I often am used to you being able to on other apps so often out of habit try like this and of course does not work.

This also seems to fix an issue where downloaded icon becomes mixed with in progress and downloaded overlapping on scroll if you have in progress downloads as well as size becoming inaccurate while scrolling (it used the wrong file's size sometimes) which seems to be fixed with setItemViewCacheSize. setHasFixedSize also improves performance by reducing the need to continue re-requesting layout when it does not need to (testing did not show any issues with it anyway, but not sure if this is the best way to do either of these things.

Known issues:

  • Does not work for series/individual episodes (will no longer act like it should; may add support for deleting full series in a follow-up)
  • Delete icon UI needs improvements (improved quite a bit)
  • If you cancel on touch outside or with back button swipes for others will stop working until view is recreated (no longer an issue)
    • Same thing if you try to do on a series (no longer an issue)

TODO (for further download improvements; maybe in follow-ups):

  • Show more information (IE synopsis/descriptions) for downloaded content
  • Add select delete multiple (particularly useful for episodes)
  • Further performance improvements for UI threads etc...

If the entire concept of this idea is undesired I suppose we can close it and I will just do some of the other things to improve performance etc... (IE when having a moderate amount of downloads scroll is very laggy) which this also seems to at least improve quite a lot.

UPDATE: since 81ae8b6, you now have to click the delete icon. This was a UX choice so you don't inadvertently swipe and create unexpected popups, if prefered yhe previous way I can revert back to it. I'm also not sure if I did this functionality the best way in the first place anyway. But after trying both ways this way seemed less issue prone and less prone to accidental and unexpected actions.

This could honestly be a horrible way to impliment this (or maybe you don't want it at all) but figured I would give it a try as I often am used to you being able to on other apps so often out of habbit try like this and of course does not work.

This also seems to fix an issue where downloaded icon becomes mixed with in progress and downloaded overlapping on scroll if you have inprogress downloads as well as size becoming inaccurate while scrolling (it used the wrong file's size sometimes) which seems to be fixed with `setItemViewCacheSize`. `setHasFixedSize` also improves performance by reducing the need to continue re-requesting layout when it does not need to (testing did not show any issues with it anyway, but not sure if this is the best way to do either of these things.

Known issues:
* Does not work for series/individual episodes
* Delete icon UI needs improvements

TODO (for further download improvements; maybe in follow-ups):
* Show more information (IE synopsis/descriptions) for downloaded content
* Further performance improvements for UI threads etc...


If the entire concept of this idea is undesired I suppose we can close it and I will just do some of the other things to improve performance etc... (IE when having a moderate amount of downloads scroll is very laggy which this also seems to at least improve quite a lot.
@fire-light42
Copy link
Collaborator

  1. The code looks a lot like https://medium.com/@zackcosborn/step-by-step-recyclerview-swipe-to-delete-and-undo-7bbae1fce27e if so, then give credit
  2. While testing this it felt really inconstant and weird. To be honest, I dont think this fits into cs3, even if it is a great idea.
  3. Download icon stuff being invalid is a known issue
  4. This might solve the lag, but is not the core issue (I think) so while this perf improvement is nice, it would be better to fix the underlying issue

@Luna712
Copy link
Contributor Author

Luna712 commented Jun 8, 2024

  1. The code looks a lot like https://medium.com/@zackcosborn/step-by-step-recyclerview-swipe-to-delete-and-undo-7bbae1fce27e if so, then give credit
  2. While testing this it felt really inconstant and weird. To be honest, I dont think this fits into cs3, even if it is a great idea.
  3. Download icon stuff being invalid is a known issue
  4. This might solve the lag, but is not the core issue (I think) so while this perf improvement is nice, it would be better to fix the underlying issue

@fire-light42

  1. Hey I didn't even know that existed. I took basic examples from other sources on GH for a start point here but for the most part the code itself is my own.
  2. I agree I am looking into ways to improve it further but if can't really will probably just abandon this.
  3. Ah, got it thanks!
  4. I agree with that yes. My initial suspicion was it was just doing to many operations on one thread leading to thread overload causing the lag but I'm not 100% sure about that anymore.

@Luna712
Copy link
Contributor Author

Luna712 commented Jun 9, 2024

@fire-light42 if I perfect the functionality is this something that has a chance of getting merged? I just want to know so I don't put more work into it if it will waste my time. Which is fine I'd just like to know.

UPDATE: I decided to just update this PR even if you decide not because I managed to find a way to fairly easily fix all the underlying issues with this that I was aware of.

@Luna712
Copy link
Contributor Author

Luna712 commented Jun 11, 2024

@fire-light42

I completely understand if you decide not to proceed with this.

I have another partially done commit that I can finish and push instead, which focuses on drastically improving UI performance based on my testing.

Additionally, I'd like to propose expanding the download functionality to include at least a synopsis. When downloading content, I find it useful to have the synopsis available offline without me needing to save Wikipedia pages separately (which is my current method). The current download system also needs significant improvements, which I am eager to address as my next major project. Since my work on accounts, I started looking into how the downloads system worked, and I now feel at least semi-confident in my understanding to begin tackling these issues.

The current system is somewhat unreliable—for instance, thumbnails don't display without an internet connection (or at least most don't). Also, the issue of the download icon mentioned above and the issue of download size being very buggy. Other improvements I have in mind include automatically deleting subtitles when downloads are removed. I am also considering the option to download files in formats like MKV, which can contain multiple subtitle tracks, or burn one selectable subtitle track into an MP4 file since MP4s only support one AFAIK. These changes could greatly enhance the user experience. However, I wanted to get your feedback and the opinions of the main developers before diving in too deep and potentially going in the wrong direction.

Also, if you have no internet connection I was thinking we should default the entry point to downloads if that is feasible for just minor improvements of startup speed to not have to go to the no connection home page. Additionally, if you click on broken or unbroken thumbnails it takes you to the API source even with no internet that serves no purpose if you have no connection which I find to be a good reason to have downloads. But maybe there are reasons to not do this.

Another (semi-offtopic) idea I have is to allow separate path selection for backups. This way, users could choose a cloud storage path like Google Drive / OneDrive or even Network Storage if their device supports it. Additionally, auto-expiring backups after (for example) every three generated would help prevent clutter. During a recent cleanup of my device, I found over 400 randomly generated backup files from three different versions of CloudStream. This indicates room for improvement.

Finally, I am interested in exploring the ability to import downloaded files from outside CloudStream (or from a different version) and sync manually downloaded subtitles with CloudStream downloads. I haven't found a way to "download with online subtitles" yet, but maybe I'm missing something. I'm not entirely sure how to approach this and would appreciate any guidance.

I may create one or a couple of issues to track these ideas—perhaps one for downloads and another for backups. However, I wanted to share these thoughts here first and get some feedback before proceeding.

Thank you for everything you all have done with this amazing app.

P.S. I would indeed appreciate it if this PR could be reviewed and merged, as I believe it enhances the UI/UX functionality. Additionally, I have more ideas for further expansion on this later, but I do completely understand if you decide not to proceed with it.

P.P.S. I know I have a lot of very random thoughts and not all of them are good so do feel free to just completely ignore some of them if they sound complete nonsensical and I do apologize in advance if so. I also may be a bit out of place even putting them on this PR instead of an issue, but I decided to as otherwise I'll just forget them, but I can move to an issue later if desired. I would also like to note there is absolutely no way I will be able to tackle all of these myself and may just be unrealistic dreams of mine but it's a lot of my random thoughts I've had in recent days on how this could be improved.

@fire-light42
Copy link
Collaborator

To answer some questions:

  1. The download system is a bit wonky, but some of that can be attributed to android being wack with UI. Also metadata != download function, you can simply add the synopsis field if it is not already added in the class
  2. We choose to not reencode the media like MPV as that takes a lot of time and we would require smth like ffmpeg, and doing so would also make it impossible to stream the media while downloading it
  3. Thumbnails are not downloaded, simply because we are too lazy to care, glide caching is good enough
  4. Backup path ect is Remote sync #623 but this has not been merged due to it not working
  5. We cant import downloads into cs3, as they simply lack the metadata required and file fuckery is hell. We have also chosen to not store the metadata with the file itself, as that bloats the file directory.

Next time you write a wall of text, can you split it up in several issues or make it a checklist. It can be hard to answer when you have many different thought on different topics in a single post.

@Luna712
Copy link
Contributor Author

Luna712 commented Jun 11, 2024

Next time you write a wall of text, can you split it up in several issues or make it a checklist. It can be hard to answer when you have many different thought on different topics in a single post.

Yes I apologize about that. Sometimes I just have some random thoughts I decided to post and like I said not all were good. I do apologize and will try not to and to create issues next time.

The rest of your answers make perfect sense and totally understand. Thank you for that.

You now have to click the delete icon. This was a UX choice so you don't inadvertently swipe and create unexpected popups
… and other cleanup


We can use adapter.notifyItemRemoved and remove it from the cardList in adapater directly since we do understand the position in this case. It works better because it does not require reloading everything and causes less bugs with the view model.

I am not sure if this was the proper way to do this. If not I can revert the change to this method

It also removes the need for a callback in DownloadButtonSetup
@Luna712
Copy link
Contributor Author

Luna712 commented Jun 17, 2024

@fire-light42
Copy link
Collaborator

While it looks like you spent a lot of time and effort on this and it is polished both from a UI and code perspective, I feel like this does not improve the UX. We already have a delete button 2 clicks away. For this reason I will close the PR, however if you still want to add the performance improvements parts of this pr, then it will be merged as a separate pr or reopen this and remove/disable the swipe code.

@Luna712
Copy link
Contributor Author

Luna712 commented Jun 19, 2024

While it looks like you spent a lot of time and effort on this and it is polished both from a UI and code perspective, I feel like this does not improve the UX. We already have a delete button 2 clicks away. For this reason I will close the PR, however if you still want to add the performance improvements parts of this pr, then it will be merged as a separate pr or reopen this and remove/disable the swipe code.

@fire-light42

That's absolutely fine. I think I will instead redo this PR removing swipe code and instead do long click to delete multiple items similar to netflix if that one would work for you? The idea of this was to make it a lot quicker to delete multiple items and it was easier than doing multiple at once similar to Netflix but I suppose I will give the other way an attempt.

@fire-light42
Copy link
Collaborator

@Luna712 Yes, multiselect is something that cs3 needs, adding it would be very nice. While Netflix has great UI+UX multiselect, you can also checkout Tachiyomi if you want inspiration for how it should look like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants