New header style for playlist
ClosedPublic

Authored by januz on Mon, Jul 9, 10:25 PM.

Details

Summary

This patch moves the buttons in the playlist to the top and groups them. I also changed the clear playlist icon to the trash icon (I think it conveys the meaning better), and also tweaked the buttons text for more consistency.

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
januz requested review of this revision.Mon, Jul 9, 10:25 PM
januz created this revision.
januz edited the summary of this revision. (Show Details)Mon, Jul 9, 10:26 PM
januz added a reviewer: Elisa.
januz added a project: Elisa.

Based on the old mockups at: https://diegogangl.github.io/

My objection to using the term "Playlist" for this widget is that it conflicts with the same use of that term to mean "a saved list of songs you can access and play later" in most other contexts. Since the "Now Playing" string is also problematic because it conflicts with the use of that same term in the list on the left, how about instead "Playback Queue"? For the other strings, my recommendation would be:

  • "Clear all"
  • "Save as playlist file..."
  • "Load playlist file..."

The last two need an ellipsis since they open save or open dialogs.

The trash icon could frighten people into thinking that their music will be deleted. I recommend instead using the new "broom-style" edit-clear-all icon that we just added to Breeze for just this kind of application : https://cgit.kde.org/breeze-icons.git/commit/?id=a3588fca59b2b71940272983e295cf339be68dd8

(Assuming you're okay with Frameworks 5.48 as a dependency, of course)

+1 for removing the gray bar at the bottom.

Good point in the ellipsis. The broom icon is way better but I'm going to wait for confirmation from @mgallien or @astippich since we are on 5.45 now.

Not sure about the "Playback Queue" idea though. I've always seen this element called "a playlist" (ever since the winamp days). Also, it's saved when you quit and you can save/load to a file too, so it fits the description :)

mgallien requested changes to this revision.Wed, Jul 11, 5:44 AM

Thanks for your work.
I have two remarks.

In reversed mode, the menu is shown at the wrong place. It is shown at the other side of the playlist component.

I am not so sure about the "clear playlist" needing now two clicks. This is an action that may need to be still reachable with only one click. What do you think ?

This revision now requires changes to proceed.Wed, Jul 11, 5:44 AM

I like the idea of removing the bar at the bottom, but imho the actions should either be all accessible through the menu or still have dedicated buttons, for consistency. Isn't there enough space to add 4 actions instead?

Yeah, it looks like there's room for some buttons up on top without needing a drop-down menu.

januz added a comment.Fri, Jul 13, 2:00 PM

Hey guys, yeah there's room for the 4 buttons up there. The main pro for the dropdown was having text for the buttons that modified the playlist, but having the buttons could also simplify things. Sorry I keep forgetting about reversed mode

Might there even be room for buttons with text?

januz updated this revision to Diff 37700.EditedFri, Jul 13, 2:17 PM

Replace menu with buttons (also eliminates the reverse mode problem)

januz added a comment.Fri, Jul 13, 2:19 PM

There is some room, but it ends up looking more cramped.

(vs just the icons)

How about putting the icons on top and making them toolbuttons without a visible button-looking border? There's plenty of unused vertical space there, and square buttons might fit well in the available space.

januz added a comment.Fri, Jul 13, 2:31 PM

How about putting the icons on top and making them toolbuttons without a visible button-looking border? There's plenty of unused vertical space there, and square buttons might fit well in the available space.

Do you mean in the Playlist title row? They are already toolbuttons in the latest revision

I mean they could be buttons with:

  • no visible borders (the borders appear on hover)
  • the icon on top, rather than on the side

That would make them take up more of the vertical space and less of the horizontal space.

Looks like ToolButtons ignore text. I managed to hack this by shoving a Text object inside them but it really messes up the layout and only the icon is clickable.

I think having icons only would be fine, that's what tooltips are for anyways :)

@mgallien, @astippich What do you think about the clear icon? Should we revert back to the line (current one) or bump KF to 5.48 to use the new broom icon?

imho only toolbuttons is fine.
I would suggest to keep the old icon for now, since it is also somewhat unrelated to the rest. You can change it afterwards when Elisa requires KF 5.48. Since it was just released, it is probably a little bit too early.

Looks like ToolButtons ignore text. I managed to hack this by shoving a Text object inside them but it really messes up the layout and only the icon is clickable.

I think having icons only would be fine, that's what tooltips are for anyways :)

@mgallien, @astippich What do you think about the clear icon? Should we revert back to the line (current one) or bump KF to 5.48 to use the new broom icon?

I have no objection. The current icon could be better. I like your idea.

januz updated this revision to Diff 37769.Sat, Jul 14, 9:37 PM
  • Reverse clear list icon
  • Add ellipsis on load/save texts (for tooltips)
astippich accepted this revision.Sun, Jul 15, 7:17 AM

looking great!

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Jul 15, 11:35 PM
This revision was automatically updated to reflect the committed changes.