Add button to add a remote playlist to a local one #7355
Conversation
There was a problem hiding this comment.
Code looks good (I didn't test). The only problem is the one I pointed out below. Maybe this could be merged anyway? The problem of not loading the whole playlist when we should is not only a problem of this PR but of some parts of the app in general, so I think merging this PR anyway is not really introducing a new problem.
| getPlayQueue() | ||
| .getStreams() |
There was a problem hiding this comment.
The issue with this is that if the playlist was not completely loaded, only the first few items are added to the other playlist, not all of them. E.g. my YouTube music playlist has ~200 videos, but YouTube loads 100 items at once, so in order to add the whole playlist to another playlist with this option I'd first have to scroll down to the bottom so that I'm sure it was loaded completely.
There was a problem hiding this comment.
I had tried having it load the entire playlist automatically when selecting the menu option. However, the lack of a loading indicator when loading large playlists may be a problem (see video). What are your thoughts?
video.mp4
There was a problem hiding this comment.
I'm not sure. Somebody will definitely find this useful, but I wouldn't introduce full playlist loading in this PR since it introduces some things that need to be resolved, and I would address them in a separate PR. You'd be welcome to create such a PR :-)
- a loading indicator should be shown somewhere, as you said
- what happens if the playlist length is infinite (and we could not know that it is infinite)? Infinite playlists are e.g. YouTube mixes. We have to discuss this.
This comment has been minimized.
This comment has been minimized.
|
@SameenAhnaf sorry but no, that's a strange solution. Bookmarking should be easily available through the toolbar button like it is now (not hidden under two clicks). And putting "Add to playlist" to the toolbar hidden items is a perfectly normal and good practice thing to do in Android. Also, this "Add to playlist" button does not need to be that fast-accessible, hiding it under the three dot menu suffices. |
This comment was marked as spam.
This comment was marked as spam.
|
I think this PR does a different thing, so I'd keep it. And I don't understand your sencond paragraph 😅 |
Yes... because it's not the same |
|
@opusforlife2 yes, the roundabout way is strange, this PR is the proper way (and it is also so simple). Also, the roundabout way existed also before #8008, as I already said when reviewing the blogpost ;-) @petlyh I would merge this, sorry for the delay. Could you rebase? Thanks :-) |
petlyh
force-pushed
the
append-remote-playlist
branch
from
ded991e
to
fcaa787
Compare
@Stypox Done :-) |
|
Kudos, SonarCloud Quality Gate passed! |
Stypox
changed the title
What is it?
Description of the changes in your PR
This PR adds an option to the menu for remote/non-local playlists
that appends the playlist items to a local playlist.
NOTE FOR BLOGPOST WRITERS: make sure to specify that only the part of the playlist that has already been loaded (in case of YT, the first 100 videos) will be actually added to the chosen local playlist; the other videos won't be fetched. See #7355 (comment).
Before/After Screenshots/Screen Record
video.mp4
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence