feat: support for downloading whole playlist at once #5525
Conversation
There was a problem hiding this comment.
@RafaelsRamos if you have time, I'd appreciate a quick review!
Sure, it is always a pleasure to help!
First of all, congratulations, great work 💪 🚀
That being said, with the introduction of this incredible feature, it may be worth revisiting the Downloads screen.
There are a few points I think we should address in future tickets:
- Adding a multiple downloads removal feature, since at the moment, it takes too much time to remove large amounts of downloads one by one;
- The downloads recycler view has flaky behaviour when removing items from it (video below - 1st video);
- When selecting to watch a download that has yet to start being downloaded (It frequently happens when we're trying to download many videos), we get into a state where the video will never play (sample below - 2nd video). I don't think this needs to be addressed ASAP, but I think at some point, we could prevent the user from advancing + show a toast with some message 🤔 ;
- Lastly, with this new feature, we need to reload the downloads page a few times until we can see all the items that will be downloaded. This issue can be mitigated by adding that "Repository" layer we discussed the other day. With this layer, the DownloadsFragment can observe changes on a LiveData object that resides in the repository and is updated when new entries are added to the local database (also, this issue is not crucial, but it might be worth looking at in the future);
Of course, I would be very happy to address some of the issues pointed out above if you agree 👍
flaky_rv_behaviour.mp4
watching_video_with_unstarted_download.mp4
app/src/main/java/com/github/libretube/services/PlaylistDownloadEnqueueService.kt
Show resolved
Hide resolved
| val availableLanguages = LocaleHelper.getAvailableLocales() | ||
|
|
||
| binding.videoSpinner.items = listOf(getString(R.string.no_video)) + possibleVideoQualities | ||
| binding.audioSpinner.items = listOf(getString(R.string.no_audio)) + possibleAudioQualities |
There was a problem hiding this comment.
Wanted to bring up the debate on having different default values. Additionally, do you think it may be worth saving on shared prefs the previously used settings in the future? 🤔
There was a problem hiding this comment.
Yes, in the future. We already do this for the normal download dialog, but I wanted to keep this PR simple and basic for now.
app/src/main/java/com/github/libretube/ui/dialogs/DownloadPlaylistDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/dialogs/DownloadPlaylistDialog.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| companion object { | ||
| private const val ENQUEUE_PROGRESS_NOTIFICATION_ID = 3 |
There was a problem hiding this comment.
Rather than having this here as a magic number, would it make sense to create an enum somewhere else along with other notification IDs? It's difficult to understand why 3 is used otherwise. (I'm guessing 1 and 2 are used elsewhere in the app)
This could be another PR perhaps.
There was a problem hiding this comment.
Yes, we should probably do thay in an other PR. 1 and 2 are used for the player and the normal download notification.
app/src/main/java/com/github/libretube/services/PlaylistDownloadEnqueueService.kt
Show resolved
Hide resolved
Bnyro
force-pushed
the
download-playlists
branch
from
3256e4e
to
16c7986
Compare
I had the same thought and opened #5526 yesterday. However, being able to select which ones to delete would be nice too. It's up to what you think works best and is doable to implement 👍
Yes, I noticed that too, but I guess it's rather low priority for now.
Yes, that's certainly a good idea (but as you said, not that important for now, but still reasonable).
I agree with you on that, in this specific case it would make sense to use a repository and live data to make everything more responsive. Feel free to look into that if you want to, I'd appreciate it! |
closes #3080
disclaimer: only tested for very small playlists so far
@RafaelsRamos if you have time, I'd appreciate a quick review!