[Redesign] Playlist removal from queue

[Komodo5197]

Allows removing queue items from their source playlist on player screen. Can be done from the song menus, or via a shortcut by long pressing the queue source in the appbar. Items associated with the queue's original source can still be removed after a queue restore, others cannot. Currently targeting desktop-beta due to build on top of the refactored song menu, I'll re-target if that gets merged.

[Chaphasilor]

That's awesome, and fixes #439 !

Regarding the long-press gesture, I'd prefer re-using the long-press on the menu button for this, and then showing a menu-chooser like the one Spotify uses for picking artists:

The two options would be "Remove track from playlist '\<playlist name>'" and "Add track to another playlist" (could also be the same options as the ones currently in the song menu, but showing the affected playlist's name would be nice).
Having this kind of confirmation is a good idea imo since removing from a playlist is a destructive action.

This would also avoid adding too many "invisible" interactions (e.g. long-pressing random elements). I'll try to tackle this in a few hours, should have time for it today :D

[Chaphasilor]

Just some thoughts I had while testing this out:

• The confirmation already showing the playlist name is nice, I also like that you made it a parameter
• After removing, I think we should close the menu automatically. Otherwise the "remove from playlist" option is still shown, which might be confusing (I'll handle that)
• All of this should also work when adding a playlist to Next Up (Already added that change)
• I'm guessing the changes in queue_list.dart are an unrelated fix?
• Is there are reason why you're supplying an empty function as onRemoveFromList in player_buttons_more.dart, instead of leaving it default to null?
• could you add a few more comments to your changes in queue_service.dart? The variable names could use some work/explaining. I'm guessing you're diffing the playlist IDs to get the unique IDs because you've already added those to the idMap? In that case, something like idsToRestore or remainingIds might be more telling
• When restoring in offline mode, wouldn't it make sense to fetch the downloaded playlist and check if the tracks are still included, instead of doing nothing? If I remove a song from a playlist in online mode, then toggle offline mode and re-start the app (would work e.g. if all playlists are always downloaded), the downloaded playlists would have the correct info, but the queue would still be "outdated". And since the "remove from playlist" button is shown in offline mode (just disabled), it might again be confusing, because it suggest the track is still part of the playlist, even though the app knows it isn't anymore.

Edit:

• We should also remove the ID from playlistRemovalsCache if the track was re-added to the playlist (e.g. if the removal was a mistake, or the user is undecided). However, I'm not sure how that will work, since we're dealing with playlistItemIds. Maybe that's too complicated to implement for now?
• Given that there can be multiple playlists inside the queue (e.g. adding multiple playlists to Next Up, the playlistRemovalsCache should probably be some kind of map, to keep track which item was removed from which playlist. But again, I'm not sure if this is actually feasible...

[Chaphasilor]

Okay, I've now implemented the menu I mentioned. Let me know what you think or if I missed anything. Could also be that I copied a bit too much unneeded code.

Do you think it would be possible to use the metadata provider for the inPlaylist check (actually fetching the playlist from the server instead of only fetching it on queue creation)? And I'd also like to use the metadata provider to power the song menu in general, but that's probably out-of-scope for this PR...

[Komodo5197]

Yeah, the queue_list stuff is fixing an issue where, after changing tracks on the queue screen, we hide the jump to current track button around the location of the old track instead of the new current one. Looking at the offline case, it does need more work, but I think I just need to lock it down more. The problem is that to remove an item from a playlist, I need the items's playlistItemId. The offline song item may not have a playlistItemId attached, and even if it does it may not be for the correct playlist. So I don't think we can actually allow removals from a queue created/restored offline at all.

The playlist removal cache uses playlistItemIds as part of the entries, so it already handles having multiple playlists in the queue properly. Dealing with the possibility of re-adding items to a playlist after queuing seemed too complex and niche to deal with.

Using the metadataProvider could possibly have some uses and improve the experiance with queues created offline, but it could be messy. Forcing people to wait on a network request to complete before opening the song menu doesn't seem great. Looking at the quick actions menu, my big problem is that I don't think it's really quick. It's actually slower than going through the song menu for adding to a playlist due to the long press. I think it's the same speed for remove from playlist due to baking in the confirmation, at least.

[Chaphasilor]

Re: offline removals, I guess that's fine. We could show the disabled list tile when long-pressing a track inside a list view, but completely hide it on the player screen / queue song menus?

You're right about the network request. Enhancing the menu once the request completes would be nice, but would also potentially cause layout shifts, so it's now really an option. We could use the metadata we already have for the player screen menu though, by passing it to the menu invocation?

My reasoning for the quick actions menu is that it's not used if there's only one option, like it worked before. Just in case where two different options would be possible, this is the same gesture for the same type of action (managing playlists).
Do you think it would be better to make this a setting instead, that lets users choose between showing the quick actions when in doubt, or always adding to a playlist? I don't see a good way to add some kind of long-press interaction for removing from the current playlist, so I don't think I would make that an option...

[Komodo5197]

I'm not sure switching between showing one menu or the other on the same action is a great idea. I feel like an option might make more sense? But really, I feel like the design where you long press on the song menu to add to playlist, or on the queue source to remove from playlist was decent enough. I feel like the idea of having everything easily discoverable in the song menu and then additional gestures that can speed up navigation for users that know about them makes sense, sort of like keyboard shortcuts. We should probably add some listing of these in the settings somewhere, I don't think I ever would have discovered the long press on the song menu button naturally.

Also, I tried my hand at factoring out all the theming code from the song/quick menus, because I imagine we're going to end up with even more of these in the long term. I'm not sure it went that well due to how tangled the song menu code is, tell me if the interface is sensible enough to use.

[Chaphasilor]

Okay, I'll think about this some more.

For the app bar gesture, I was worried about the hidden state (gesture not always available) and the no-op in that case, but I guess the state isn't hidden (queue source on the player screen shows if it's from a playlist), and instead of a no-op we could show a snackbar if the track isn't part of a playlist. So it would definitely be possible.

Another option might be dismissing the app bar in different directions for different actions, but I'm not sure if that would work yet.

I'll give the PR a test drive for today to see if there are any issues with the theme! Thanks for the improvements :)

[Komodo5197]

A snackbar on the no-op would probably make sense, even though you can see the queue source isn't a playlist. A swipe feels a bit less intuitive, and is also easier to trigger accidentally. And the player screen already has five swipe gestures, I'm not sure it needs more.

In a related interface question, do you think the current behavior of having to confirm playlist removal from the queue items is good? Or should it be instant with the playlist name shown like in the quick menu? Also, is it worth binding any of this to right click? I didn't bind these shortcuts, but I'm thinking I bind right click on the cover to bring up the song menu and you can probably beat the shortcuts anyway.

[Chaphasilor]

Yeah, there are probably enough gestures...

Confirming destructive actions is fine, but even better would be no confirmation but a way to Undo the action. That is more complicated though.
Feel free to bind to right click (can be anywhere imo, not just on the cover).

Do you think it's possible to use showThemedBottomSheet for the queue panel too, or is that too different?

[Komodo5197]

Yeah, I can't think of a clean way to add an undo. I was thinking about maybe binding clicking the controls to bringing up the queue or something, but that feels a bit weird, so right clicking anywhere does make sense.

I think the queue is too different for the change to be a positive. A lot of the showThemedBottomSheet code is related to quickly pulling a theme up, and that's useless for a widget only shown on the player screen. Plus it has scrollbar tweaks, and we don't do the stackHeight thing, and the queue hasn't been narrowed on desktop like the song menu.

[Chaphasilor]

While thinking about this, I remembered the discussion in https://github.com/jmshrv/finamp/issues/629

In principle I like Spotify's interaction, but there are two problems I see with it:

• The "save" button on the player screen cannot differentiate between a track being part of the currently playing playlist or any other playlist. It just differentiates between "saved" and "not saved"
• It isn't possible to add tracks to a playlist multiple times

I'm guessing Spotify put some user research into it, and can see how duplicates in playlists are very rarely wanted, but I'm not sure if the "saved" or "not saved" approach applies to Finamp.
But I'd definitely like to investigate this further.

How strong is your preference for not having the quick actions menu?

[Komodo5197]

I believe jellyfin doesn't allow duplicates in playlists without server-side finagling, but a model where the main focus is the distinction between in any playlist or not in a playlist feels like it makes way more sense in a less focused environment like Spotify then it does sure. But I'm not certain my usage is particularly representative here.

I'm reasonably convinced that the slightly faster add to playlist without the quick actions menu is important. But that's in a theoretical sense, because I don't really use that button in practice. I typically build playlists by adding whole albums, and then removing the songs I don't want later, so I'm not actually affected by this very much. If we're consolidating the actions, I'd like to mention an alternative design where the quick menu fully replaces the add to playlist screen, with a full list of individual playlists to add to but also the remove button at the top. I'm not sure if this is better, but it seems worth considering?

[Chaphasilor]

I'll take a look at how duplicatea are handled server-side, and what the future plans are in that regard.

And yes, I'm also considering the alternative design. I'd probably replace the favorite button, and use onTap to open the menu, with onLongPress toggling the favorite state. But that's still up for debate.
My main concern there was that it's probably very inefficient to fetch what playlists a track is part of. We need the playlistItemId of that track both for showing the indicator, and for actually removing it from the playlist. Any ideas?

But also, that would keep the flow for adding to a playlist at the same number of interactions, but make removing from playlists slightly slower (which seems to be your main action when creating playlists).

[Komodo5197]

The current removal flow is two presses - one to long press the appbar, and one to confirm. If we keep the no-confirmation removal the quick menu currently has, that's still only two. And yeah, I don't believe there's any efficient way to find out if something is part of a playlist, so a spotify style indicator would take a lot of large requests, as would allowing you to remove from any playlist other than the one in the queue item source.

[Chaphasilor]

Okay. But I guess if we have a separate section at the top, with "remove from playlists 'xyz'" and maybe "add to favorites", then a divider, and then the list of playlists, that should work fine.

I'll mock something up!

[Chaphasilor]

Okay, I'm super tired but this is what I came up with for now:

The section at the top is meant to be the same section as in the song menu, I just don't have an up-to-date mock of that.
What do you think about it? I think we could try to implement it (just not today for me, I'm going to bed).

Adding duplicates really isn't possible, but the server will respond with a 204 - Items added to playlist.. I guess we can ignore this for now. Maybe we can get some kind of batch endpoint in 10.10 that lets us look up which playlists a track is already part of...

[Chaphasilor]

Thanks for getting started with this. I've made further progress and have now completely replaced the "AddToPlaylistScreen" with the new menu. Would be nice if you could make sure everything is in order.

I've also reworked the favorite button on the player screen, queue list and now playing bar to be a more general "add to playlist" button, A single tap will now open the menu, while a double tap will like the track.
Only the favorite status is shown by the button, because a) there is no way for us to check if the track is part of any playlists, and b) if the track is part of the current playlists, that should be obvious by the source. The menu can also be used to confirm that.

Now only the localization strings should be missing as far as I'm concerned.

[Komodo5197]

It seems pretty good to me. My thoughts:

• Not being able to remove the track from the bottom section is a bit strange. We could probably fix this for the queue playlist, or just hide the item because it's redundant with the top. For others, I guess we can't do anything unless we fetch playlistItemId somehow.
• There isn't any way to create a new playlist using this screen, so that needs to be added somewhere. Probably just some sort of blank playlist entry at the end of the list?
• I feel like some/all of the header info should be hidden when accessed via the add to playlist button in the song menu, because the user already has a specific intention and it helps keep more playlists onscreen without scrolling. What do you think about this?
• The track count should probably update when adding/removing a track - I'll try to add this in.
• The use of the doubletap gesture actually adds a delay to opening the menu because it has to wait for a possible second tap. Is there a reason this specific gesture is used?
• The way themes are propogated around is pretty messy. I'll try to figure out a cleaner method.

[Chaphasilor]

Sorry for not responding earlier. I'll take a look at your changes in a bit.

We could fetch the playlist info when the user wants to remove a track, so just make two requests. But that won't be common anyway, since it can only happen after adding the item to a playlist. Still, might be worth it.

Playlist creation is a good point. I'll take a look at what you came up with!

I wanted to add the header to make it clear what track they're adding, to show the system state. Since we always have a certain track referenced when the playlist menu is open, we could also get rid of it completely, or make it optional. At least the track name should be shown somewhere in that case I think.

Double tap was used because it felt faster than the long press, and the double-tap-to-like gesture is pretty common for apps like Instagram, hence why I also added the gesture for the album cover. The menu button actually shouldn't have any gesture at all, I forgot to remove it.

I'm also not sure about the separate "Add to playlist" / "remove from playlist" buttons. Do you think we should consolidate them into having one option that opens the playlist menu?

[Chaphasilor]

Okay, some more thoughts:

• The "new playlist" item at the end is fine I think. Other apps have it at a more prominent position (Spotify at the top, YT Music as a FAB), but I think having after all playlists makes sense. I'll go ahead and change the design a bit, do you think it would be possible to show the newly created playlist in the list of playlists immediately? Right now the menu gets closed, which is different from what happens with the other playlists. It's fine if the new playlist is shown at the end of the list.
• What exactly does the favorite provider do? Is it just a wrapper for toggling the favorite status?
• We could probably decrease the overall size of the track info header. Maybe by adding a discrete condensed size that can be selected through an argument?
• Guess you already removed the gesture from the menu button ^^
• I'll change the double tap gesture to a long press gesture for the add_to_playlist_button. You're right, the delay for opening the menu is a bit irritating. I'll leave the double-tap-to-like gesture for the cover unchanged, I think that works fine
• What do you think about adding a filter to the list of playlists? Adding filters to various views (e.g. album, playlist, artist screen) is on my todo list anyway, so we could try this out here to see if there are any challenges...

[Komodo5197]

• To properly add the new playlist to the end, we'll need to contact the server to actually fetch the new item. This probably isn't too hard? And I think it probably makes sense. I might also add a fetch to allow removing from playlist. I'll note this same scenario happens if you remove, then add, then remove from the parent playlist, so that needs to be fixed as well.
• The favorite providers primary function is to keep all the favorite icons synced up. It also consolidates the update logic and makes sure everyone is consistently updating immediately, then rolling back if it fails.
• Having a condensed version of the song info would be nice.
• I'd rather not mess around with filters at this point. That should probably be nailed down somewhere with a better usecase first.
• I think I'd prefer to keep the add and remove entries separate on the song menu, although I'm not particularly certain about that. You can merge them if you feel it's best.
• I was looking at your tweaks to the buttons with an accent color, and I'd like to note that I don't really like the pattern of passing accent colors around. I'd really prefer to rely on actually setting the context theme correctly, and using that. Do you have any thoughts on this?

[Chaphasilor]

• I noticed the special download for caching library images prompts for a transcoding provide if set to ask, that probably doesn't make a whole lot of sense. Totally forgot about that option, but I'll make sure to put it in the release notes!
• It would be great if you could add the needed requests. Since they're triggered by a user action and we have a spinner in place, it should be fine. Do you thing you could show a placeholder item (with a running spinner) while the new playlist is being created, or does that add an unnecessary amount of complexity?
• Ahh that makes sense! I was surprised the syncing worked reasonably well to begin with ^^
• I'll try to create the condensed version tomorrow :)
• Alright, we'll get to filters when redesigning the playlist screen then, I guess
• I'll think about how to handle the two actions some more. Keeping the "remove from playlist" action does make sense for the menu inside a playlist, so you might be right...
• You're right, cascading themes would be better. Is the global theme using the correct colors? I'd like for the CTAs to keep the current color within the login flow, where we don't have an explicit theme set I believe

[Komodo5197]

The global themes are set based on color_schemes.g.dart. Dark mode has jellyfin blue as primary, light mode has a slightly darker variant. We should also probably be using all of these different scheme colors more instead of just messing with opacity on primary?

I'll see how hard it is to get the new playlist placeholder in place. Hopefully it isn't too bad.

[Chaphasilor]

The problem with setting these background colors through the theme is that it's somewhat unpredictable which material widget will make sure of which colors where, so it might have unintended effects. I'd leave it out of this PR for now, but we can investigate this in the future. With the redesigned elements we should be relying on the pre-styled material widgets less and less, so eventually this is definitely possible to do, yes.

I also encountered two errors after unlocking my phone this morning: something about a missing provider scope, and a duplicate global key.
Logs are attached, but the trace doesn't seem to be all that helpful. Last thing I did last night was set a sleep timer, so either the player song menu or the player screen were currently open.

finamp-logs_missing-provider-scope_duplicate-globalkey.txt

Aside from that, everything looks good! I think I'll simply change the working for the menu actions a bit, and shrink the header of course. But your changes for adding and removing, as well as the playlist creation, work beautifully! Really appreciate your efforts!

[Chaphasilor]

One more thing I just noticed: I use the dashed check icon to indicate that we don't know if the track is part of a playlist or not. But after removing a track from a playlist, we can be sure that it isn't part of the playlist anymore, so the non-dashed icon should be shown instead.

Edit: Another thing, because the updateFavorite method doesn't return a future, the success feedback of the ToggleableListTile will fire immediately when toggling the favorite state. And since the list tile already offers feedback, the feedback from the provider should probably be suppressed in that case. In the end, the feedback should be a selection onTap, and a success after the request to the server was successful

[Komodo5197]

The ProviderScope thing was caused by the system theme changing, which caused the full FinampApp to rebuild above the ProviderScope due to an incorrect context being used for the MediaQuery. I believe the duplicate key thing is just a side effect of the tree rebuild breaking.

I tweaked the icons like you mentioned, and while doing that I realized that using a checkmark for an item that isn't in a playlist is kind of confusing. I feel like it should be an empty circle or something?

[Chaphasilor]

My initial idea was using a question mark inside the circle for when we're not sure if it's part of a playlist, but that looked too much like a help button.

Maybe we could use the empty non-dashed circle for if it's definitely not part of the playlist, the dashed with check if we're not sure, and the filled check for when it definitely is?

[Chaphasilor]

Just added the condense header and updated the strings. Regarding the icon, I think I still prefer the dashed check over the dashed plus, but both really aren't ideal. If you feel like the plus icon works better, leave it for now. I'll try to get a proper endpoint for JF 10.10...

Edit: I'll go to bed now and look at any additional changes tomorrow. I'll probably merge it then, too!

[Komodo5197]

I tried using empty circles for the icons, but it looked a bit weird. And I feel like the checkmarks are a bit confusing on first glance.

I noticed that the playlist menu had a drag handle, but the song menu didn't. Is this something we want, or should they be consistent? Also, should drag handles be shown on desktop? The swipe down to close is still there, but on desktop there's no movement, it's either open or closed.

[Chaphasilor]

Design-wise I based the playlist actions menu on the queue panel, but the song menu probably also should have the drag handle. That would take up additional space though, not sure if you'd like that.
On desktop we should have plenty of space to show the handle, and since you can still use a drag gesture, swipe gesture (on a touchpad) or swipe gesture (on a touchscreen) to interact with the panels, I think we should keep the handles there for now.

And thanks for the metadataProvider improvements!

[Komodo5197]

The drag handle is pretty small, and half the song menu options are already off the screen for me, so I don’t think I mind that much. It does make sense to show it to hint at the gestures. I guess this is already good.

I think the only idea I have left is the idea of moving the playlist buttons down in the song menu now that the playlist actions shortcut is more discoverable. Although they’re still probably in high demand outside the player screen, and the quick action is easier to find but still not 100% obvious, so maybe they’re better where they are. We really should add some sort of gesture guide somewhere.

[Chaphasilor]

I'm not a fan of needing a gesture guide, I'd much rather have interactions that are easy to understand. But at the moment I have no better idea than the current solution...

Which menu items would you like to prioritize? I'm fine with moving the playlist action further down again, but I'm not sure to where. Initially it was placed below the queueing options, so where the remove from playlist option still is.

[Komodo5197]

Yeah, needing this sort of guide isn't ideal, but I feel like there's generally a limit to how intuitive these sort of quick-acess gestures can be, because fundamentally they're completely invisible. It's just that some are common across other apps so the user expects they might be here as well, and if they don't find them, that's why we still have all the buttons and song menu entries to do the same things, just slightly slower. But putting some sort of list of them in the settings for people willing to dig through all of those is better than the current situation where it's pretty easy to just never know some exist.

I was thinking about pushing them down below the queue addition buttons, and maybe even the download button, but I think I've talked myself out of it. I feel like adding/removing from a playlist in the album/artist menus is just too common to push them down a meaningful distance, and having a different order on the player screen vs elsewhere seems insane. It's probably best to leave them where they are.

[Chaphasilor]

You're right. For the "shortcuts" that are just some special gesture, some guide/tutorial would be good. This could also be made more "interactive" by actually embedding the relevant component to try it out. Otherwise we're stuck with text descriptions.
Or we create short clips that can be embedded instead?

Yeah, let's leave it as it is. The queue actions in the player screen menu don't make too much sense either, but it's consistent :)

I guess this is ready to merge then?

[Komodo5197]

Yeah, I think it's good.

[Chaphasilor]

Awesome! Let's hope I didn't break too much with the merge ^^