D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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:

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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.

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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 ?

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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:

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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:

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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:

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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,

D17073: Do not crop albumArt

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

D17073: Do not crop albumArt

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