D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  Looks amazing.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: mvourlakos, rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Huge improvement. Let's do it.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: mvourlakos, rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  > HInt: Please dont discuss here the Latte case because it creates noise for 
all the reviewers interested in this
  
  Sure thing. Sorry :D

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: mvourlakos, rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17073: Do not crop albumArt

[Michail Vourlakos]

mvourlakos added a comment.


  In D17073#431047 , @rooty wrote:
  
  > 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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: mvourlakos, rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  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:
  
  F6691364: image.png 
  
  Might be worth another patch? I could post my changes or if you want you 
could write your own version :D

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty accepted this revision as: rooty.
rooty added a comment.


  I normally wouldn't ask this but seeing as my fonts have been acting up, can 
you please post a screenshot? 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 it's 
worth it
  
  Nice work

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> ngraham wrote in ToolTipInstance.qml:338
> No need to reduce the opacity here if the song name is bold.

I think that makes it look nicer.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> ToolTipInstance.qml:338
> +visible: text != ""
> +opacity: 0.75
>  }

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Tranter Madi]

trmdi updated this revision to Diff 53884.
trmdi added a comment.


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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17073?vs=53812=53884

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  In D17073#430519 , @ngraham wrote:
  
  > 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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added a comment.


  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%.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty requested changes to this revision.
rooty added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Awesome!

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  In D17073#430382 , @trmdi wrote:
  
  > Increase the contrast between the song name and the artist.
  >  F6689279: image.png 
  
  
  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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Tranter Madi]

trmdi updated this revision to Diff 53812.
trmdi added a comment.


  Increase the contrast between the song name and the artist.
  F6689279: image.png 

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17073?vs=53770=53812

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  
  
  > @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.
  
  F6688452: Screenshot_20190313-134318_Spotify.jpg 

  
  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Filip Fila]

filipf added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  > - Using a bigger font size means less characters could be display.
  
  The song name is always going to be elided, so that 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.
  
  F6688201: image.png 
  
  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.
  
  F6688214: image.png 
  
  I'd even go so far as to say you should use "Layout.margins: 2", but I'll 
leave that decision to you.
  I propose that you also add a right margin (2 px) to offset the one on the 
left.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Tranter Madi]

trmdi added a comment.


  - Using a bigger font size means less characters could be display. 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 ?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty requested changes to this revision.
rooty added a comment.
This revision now requires changes to proceed.


  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:
  
  F6688168: after.png 
  
  After:
  
  F6688175: after-2.png 

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf, rooty
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Tranter Madi]

trmdi updated this revision to Diff 53770.
trmdi added a comment.


  Remove duplicated "ToolTipWindowMouseArea"

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17073?vs=53769=53770

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg, filipf
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Tranter Madi]

trmdi updated this revision to Diff 53769.
trmdi added a comment.


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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17073?vs=53710=53769

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg, filipf
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added a comment.


  In D17073#429514 , @filipf wrote:
  
  > F6686129: Screenshot_20190312_100751.png 

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Filip Fila]

filipf accepted this revision.
filipf added a comment.


  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:
  
  F6686129: Screenshot_20190312_100751.png 

  
  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg, filipf
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Tranter Madi]

trmdi updated this revision to Diff 53710.
trmdi added a comment.
This revision is now accepted and ready to land.


  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:
  
  F6686096: image.png 
  
  - non 1x1 cover (e.g. open youtube urls in VLC):
  
  F6686102: image.png 
  
  - when the song name is very long and the artist info is empty:
  
  F6686109: image.png 

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17073?vs=47876=53710

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Filip Fila]

filipf added a comment.


  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:
  
  F6682163: Screenshot_20190310_205631.png 

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Filip Fila]

filipf added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Krešimir Čohar]

rooty added a comment.


  +1, F6607842: image.png  the covers 
shouldn't get cropped.
  Them blurry bars are a little too thick/wide for my taste, 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?
  
  F6607847: image.png 
  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: rooty, filipf, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  This looks sensible to me. #plasma  
folks?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: filipf, ndavis, abetts, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[trmdi]

trmdi updated this revision to Diff 47876.
trmdi edited the summary of this revision.
trmdi edited the test plan for this revision.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17073?vs=46007=47876

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg
Cc: filipf, ndavis, abetts, anthonyfieroni, plasma-devel, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Andres Betts]

abetts added a comment.


  In D17073#378898 , @trmdi wrote:
  
  > How about this? I'd send a new patch if it's possible to change 2 things in 
one patch.
  >  F6486761: test.png 
  
  
  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: filipf, ndavis, abetts, anthonyfieroni, plasma-devel, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added a comment.


  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. :)

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: filipf, ndavis, abetts, anthonyfieroni, plasma-devel, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[trmdi]

trmdi added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: filipf, ndavis, abetts, anthonyfieroni, plasma-devel, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Filip Fila]

filipf added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: filipf, ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Andres Betts]

abetts added a comment.


  In D17073#366588 , @ngraham wrote:
  
  > 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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Noah Davis]

ndavis added a comment.


  In D17073#364541 , @trmdi wrote:
  
  > In D17073#364393 , @ndavis wrote:
  >
  > > That blur looks nice, but I think the background should be more opaque 
like the Elisa header background.F6437066: Screenshot_20181122_005432.png 

  >
  >
  > Do you want to suggest some numbers ?
  
  
  100%. I think having the same look as Elisa's header would look nice. If you 
think something else looks better go with that.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[trmdi]

trmdi added a comment.


  In D17073#364393 , @ndavis wrote:
  
  > That blur looks nice, but I think the background should be more opaque like 
the Elisa header background.F6437066: Screenshot_20181122_005432.png 

  
  
  Do you want to suggest some numbers ?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Noah Davis]

ndavis added a comment.


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


REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[trmdi]

trmdi updated this revision to Diff 46007.
trmdi edited the test plan for this revision.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17073?vs=45950=46007

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Noah Davis]

ndavis added a comment.


  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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Noah Davis]

ndavis added a comment.


  In D17073#364003 , @trmdi wrote:
  
  > How about this ?
  >  F6436415: test.png 
  
  
  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: ndavis, abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Anthony Fieroni]

anthonyfieroni added a comment.


  +1

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[trmdi]

trmdi added a comment.


  How about this ?
  F6436415: test.png 

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Andres Betts]

abetts added a comment.


  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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Andres Betts]

abetts added a comment.


  Looks great. Thanks for the fix.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: abetts, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17073: Do not crop albumArt

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

To: trmdi, hein, broulik, ngraham, #vdg
Cc: anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17073: Do not crop albumArt

[trmdi]

trmdi created this revision.
trmdi added reviewers: hein, broulik, ngraham, VDG.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
trmdi requested review of this revision.

REVISION SUMMARY
  Because:
  
  - the standard aspect ratio of `albumImage` is always `1:1`
  - we should not crop the important part of `albumArt` in some cases.
  
  BUG: 401234

TEST PLAN
  Before:
  F6435279: Screenshot_20181121_200913.png 

  After:
  F6435280: Screenshot_20181121_201846.png 


REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17073

AFFECTED FILES
  applets/taskmanager/package/contents/ui/ToolTipInstance.qml

To: trmdi, hein, broulik, ngraham, #vdg
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart