[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 #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]

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 ^^