[Task Manager] Do not crop album art in tooltip
ClosedPublic

Authored by trmdi on Nov 21 2018, 1:54 PM.

Details

Summary
  • The standard aspect ratio of albumImage is always 1:1. We should not crop the important part of albumArt in some cases.
  • Hide the window title text of media players to avoid displaying the title/artist repeatedly.

BUG: 401234
FIXED-IN: 5.16.0

Test Plan

Before vs After:

  • Breeze Dark

  • Breeze Light

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
trmdi created this revision.Nov 21 2018, 1:54 PM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 21 2018, 1:54 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
trmdi requested review of this revision.Nov 21 2018, 1:54 PM
anthonyfieroni added a subscriber: anthonyfieroni.EditedNov 21 2018, 2:45 PM

Whole point is to not change tooltip height, album art can be larger that entire screen or smaller than 16x16, so the solution is to scale image to current aspect ratio. Otherwise it can lead to inconsistency.

abetts added a subscriber: abetts.Nov 21 2018, 2:53 PM

Looks great. Thanks for the fix.

We should avoid resizing the pop-up but rather scale the image to fit while preserving its existing aspect ratio.

I know this patch doesn't deal with this, but I would suggest doing a background blur on the bottom bar that has the player buttons and the labels. It will improve readability

trmdi added a comment.Nov 22 2018, 8:26 AM

How about this ?

ndavis added a subscriber: ndavis.Nov 22 2018, 8:59 AM

How about this ?

I think this version works much better than the other 2. It shows the entire image and it fills up the space while maintaining the consistent thumbnail size.

If you want to try adding blur to the background, check out the code here: https://cgit.kde.org/elisa.git/tree/src/qml/HeaderBar.qml#n53

trmdi updated this revision to Diff 46007.Nov 22 2018, 2:05 PM
trmdi edited the test plan for this revision. (Show Details)

That blur looks nice, but I think the background should be more opaque like the Elisa header background.

trmdi added a comment.Nov 23 2018, 3:49 AM

That blur looks nice, but I think the background should be more opaque like the Elisa header background.

Do you want to suggest some numbers ?

ndavis added a comment.EditedNov 23 2018, 2:27 PM

That blur looks nice, but I think the background should be more opaque like the Elisa header background.

Do you want to suggest some numbers ?

100% opacity. I think having the same look as Elisa's header would look nice. If you think something else looks better, go with that.

Staring at these screenshots again, it occurs to me that the artist and title are unnecessarily repeated in the current UI: once right under the app name, and again towards the bottom, on the transparent bar. We can probably get rid of one of these to scrounge up some more vertical space for the album art.

Staring at these screenshots again, it occurs to me that the artist and title are unnecessarily repeated in the current UI: once right under the app name, and again towards the bottom, on the transparent bar. We can probably get rid of one of these to scrounge up some more vertical space for the album art.

Noticed the same thing but didn't think it was part of the patch. It would be nice if the information wasn't repeated. +1

filipf added a subscriber: filipf.Nov 27 2018, 12:41 AM

Great patch and good thinking concerning removing an extra label. I'd remove the upper one (and then we can move the "on" desktop 1" text up) because I think the bigger one is nicer.

trmdi added a comment.Dec 18 2018, 3:00 PM

How about this? I'd send a new patch if it's possible to change 2 things in one patch.

You can change two things in one patch if they're both related to the same overall goal, which is in the title of the patch. :)

How about this? I'd send a new patch if it's possible to change 2 things in one patch.

I was thinking here that you might have to visually prioritize the content and controls.

For example, instead of having the playback buttons on a small corner on the bottom right, have them centered and a little bigger. I would remove the duplicate song title. In the end, the user will probably want to interact with the popup more than trying to read the labels. In my mind, the user appreciates the content but his primary action is to work with the controls. Therefore, those controls should have more visual prominence.

trmdi updated this revision to Diff 47876.Dec 20 2018, 8:26 AM
trmdi edited the summary of this revision. (Show Details)
trmdi edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Jan 2 2019, 4:06 AM

This looks sensible to me. Plasma folks?

This revision is now accepted and ready to land.Jan 2 2019, 4:06 AM
rooty added a subscriber: rooty.EditedFeb 11 2019, 1:17 PM

+1,

the covers shouldn't get cropped.

Them blurry bars are a little too thick/wide for my taste though, is there any way to make the preview taller or to get rid of the application title to make more of the album covers fit?


We could add some padding at the bottom of the other previews to make more room for the app album art so the blurry bars wouldn't be as wide.

trmdi abandoned this revision.Mar 9 2019, 3:27 AM
filipf added a comment.Mar 9 2019, 4:23 PM

I liked this patch, how come you abandoned it @trmdi ?

Probably someone else should commandeer it and finish it up so we can get it in for 5.16. Do you wanna do that, @filipf or @rooty?

filipf added a comment.EditedMar 10 2019, 8:29 PM

If there's no objections and because I'd really like for this to end up in 5.16, we could do that yeah.

I agree that we're still obfuscating a non-negligible portion of the album art:

Maybe the thumbnail should be bottom anchored to the player controls in the case of hasPlayer but I'm not sure if we can have popups taller when using a player.

I don't think anyone'll object if you wanna grab this!

trmdi updated this revision to Diff 53710.Mar 12 2019, 8:58 AM

Improve some stuff:

  • Use smaller fontsize, lineHeight for track/artist
  • "Anchor" albumArtImage's bottom to playerControlsLoader's top
  • When artist == "", let the song name fill the width/height when possible
  • 1x1 cover:

  • non 1x1 cover (e.g. open youtube urls in VLC):

  • when the song name is very long and the artist info is empty:

This revision is now accepted and ready to land.Mar 12 2019, 8:58 AM
filipf accepted this revision.Mar 12 2019, 9:12 AM

Looking real good. Big props for keeping the same tooltip height as in all other situations. For reference here's what it looks like with a light scheme:

The only thing I'd do now is add a just a little bit of a left margin for the artist & track info, and the same as a right margin for the player controls. But that could be done in another patch anyway.

ngraham added a comment.EditedMar 12 2019, 1:38 PM

The only thing I'd do now is add a just a little bit of a left margin for the artist & track info, and the same as a right margin for the player controls. But that could be done in another patch anyway.

Yep, just do this and I think the patch can land. It's fine to do that here IMO.

...Either that, or don't extend the blurred background so low that it's underneath the text and controls, in which case the existing built-in margins become acceptable.

trmdi updated this revision to Diff 53769.Mar 13 2019, 7:50 AM
  • Add 2px to leftMargin of track/artist. I don't set the margin of the Next button because it has its own padding.
  • Resize the player icon when "minimized, we don't have a preview"

Is it fine enough to commit now ?

trmdi updated this revision to Diff 53770.Mar 13 2019, 8:50 AM

Remove duplicated "ToolTipWindowMouseArea"

rooty requested changes to this revision.EditedMar 13 2019, 9:10 AM

How about adding a little bit of a margin above the song name (Layout.topMargin 2, same as the left margin)
And why not keep the level of the track name at 4? It's more important than the artist name (subheading)

Before:

After:

The difference in heading levels matches up really nicely with our notifications, for example:

This revision now requires changes to proceed.Mar 13 2019, 9:10 AM
trmdi added a comment.EditedMar 13 2019, 9:23 AM
  • Using a bigger font size means less characters could be displayed. I don't see what is more important between artist/song.
  • I don't think the topMargin is needed because there is already a space between the song name and the cover image.

My goal is to keep the cover image as big as possible.

How do others think ?

rooty added a comment.EditedMar 13 2019, 9:33 AM
  • Using a bigger font size means less characters could be display.

The song name is always going to be elided, so it doesn't matter.

I don't see what is more important between artist/song.

The song name is more important (more 'current' and calls for more attention, especially when skipping through an album).

  • I don't think the topMargin is needed because there is already a space between the song name and the cover image. My goal is to keep the cover image as big as possible.

It looks stuck on. And considering that this patch shrinks the album art a great deal already (and we seem to deem that to be an acceptable side effect), sacrificing the margins to get more album art size doesn't make sense.

I propose that you also add a right margin (2 px) to offset the one on the left.

rooty added a comment.EditedMar 13 2019, 12:14 PM

Level 5 for both labels would be acceptable to me if you made the song name bold.
I would actually prefer this. However, it's likely that mart would throw a hissy fit over this change :D

Nothing's a deal-breaker for me. I'm happy with the padding as it is.

@rooty is right though that usually there is always a hierarchy between the song and the artist. I'd also prefer a level 5 bold song title, but yeah it's a controversial topic these days.

rooty added a comment.EditedMar 13 2019, 12:49 PM

@rooty is right though that usually there is always a hierarchy between the song and the artist. I'd also prefer a level 5 bold song title, but yeah it's a controversial topic these days.

It's almost universal nowadays - song names in boldface. The applications/services that use it include but aren't limited to: Lollypop, Spotify, Tidal, iTunes, Deezer, and Soundcloud.

The only potential hurdle might be the track name on the lock screen.

I think it looks gorgeous though.

trmdi updated this revision to Diff 53812.Mar 13 2019, 5:21 PM

Increase the contrast between the song name and the artist.

rooty added a comment.Mar 13 2019, 5:55 PM

Increase the contrast between the song name and the artist.

I would avoid using "Font.DemiBold" because

  • it doesn't always work - for example, on both Manjaro/Arch and neon, Font.DemiBold looks the same to me as Font.Normal (I think that's a bug though!)
  • many fonts don't have a semibold/medium variant (and it just uses Normal instead)
  • I don't think Noto Sans has a semibold variant, only medium (I could be mistaken)

Also I could swear that it looks bold in that screenshot, not semibold.
But it's a step in the right direction :D

ngraham accepted this revision.Mar 13 2019, 6:51 PM

Awesome!

rooty requested changes to this revision.EditedMar 13 2019, 7:38 PM

We still need to resolve the DemiBold issue.
DemiBold fonts either aren't present or don't work in neon. (Or both...)

This revision now requires changes to proceed.Mar 13 2019, 7:38 PM

Just use a real bold if you want it bold. :) In which case, the label below it should have normal 100% opacity instead of 75%.

rooty added a comment.Mar 13 2019, 8:39 PM

Just use a real bold if you want it bold. :) In which case, the label below it should have normal 100% opacity instead of 75%.

+1 for the Bold. In for a penny in for a pound :D

trmdi updated this revision to Diff 53884.EditedMar 14 2019, 2:30 PM

Use Font.Bold for the song name.
(I feel it's a bit too bold though when using Breeze Light)

ngraham added inline comments.Mar 14 2019, 2:57 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
348

No need to reduce the opacity here if the song name is bold.

trmdi added inline comments.Mar 14 2019, 3:00 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
348

I think that makes it look nicer.

rooty accepted this revision as: rooty.EditedMar 14 2019, 3:55 PM

I normally wouldn't ask this but seeing as my fonts have been acting up, can you please update the test plan screenshots? Sorry :D

I'm giving you my +1, if the others decide we shouldn't go ahead with the bold (in case it's too bold) then I'm not going to argue, but I do think the bold is really nice

Nice work

rooty added a comment.Mar 14 2019, 4:04 PM

P.S. We could also implement this in latte-dock. Not just that, but also give latte's ToolTipInstance a makeover (the fonts are really huge).
I've actually already used this diff to modify latte-dock's ToolTipInstance:

Might be worth another patch? I could post my changes or if you want you could write your own version :D

P.S. We could also implement this in latte-dock. Not just that, but also give latte's ToolTipInstance a makeover (the fonts are really huge).

No problem... Concerning Latte, just a side note, we discussed this with @trmdi when he initially submitted the first version of this patch for Latte.
My decision for this is that whatever the official plasma taskmanager supports as a design decision the same will be applied to Latte also. I want
the Latte Preview windows to look the same with plasma taskmanagers one. So this patch I think is an effort from @trmdi to update the plasma
taskmanager previews in order afterwards to apply the same thing to Latte....

Again no problem from me, as long as the Latte and plasma taskmanagers have same Preview Windows

Might be worth another patch? I could post my changes or if you want you could write your own version :D

no problem, I would propose first to have an acceptance from here and afterwards create a new PR for Latte

HInt: Please dont discuss here the Latte case because it creates noise for all the reviewers interested in this

rooty added a comment.Mar 14 2019, 4:17 PM

HInt: Please dont discuss here the Latte case because it creates noise for all the reviewers interested in this

Sure thing. Sorry :D

trmdi edited the test plan for this revision. (Show Details)Mar 14 2019, 5:32 PM
ngraham accepted this revision.Mar 14 2019, 5:38 PM

Huge improvement. Let's do it.

This revision is now accepted and ready to land.Mar 14 2019, 5:38 PM
rooty added a comment.Mar 14 2019, 5:38 PM

Looks amazing.

ngraham retitled this revision from Do not crop albumArt to [Task Manager] Do not crop album art in tooltip.Mar 14 2019, 5:43 PM
ngraham edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.