[applets/mediacontroller] Visually refresh media controller plasmoid
Needs RevisionPublic

Authored by cblack on Wed, Feb 5, 1:13 AM.

Details

Summary

The media controller has been adjusted visually. Note that this is marked as WIP due to the fact that the system tray displays margins around the applet, which doesn't look too good.

Co-authored-by: Ismael Asensio <isma.af@gmail.com>

Test Plan

Before:


After:

Diff Detail

Repository
R120 Plasma Workspace
Branch
media-plasmoid-relayout (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22157
Build 22175: arc lint + arc unit
cblack created this revision.Wed, Feb 5, 1:13 AM
Restricted Application added a project: Plasma. · View Herald TranscriptWed, Feb 5, 1:13 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Wed, Feb 5, 1:13 AM
cblack planned changes to this revision.Wed, Feb 5, 1:14 AM
cblack edited the test plan for this revision. (Show Details)
cblack edited the test plan for this revision. (Show Details)
ngraham added subscribers: manueljlin, ngraham.

Overall very nice. I think it's a good visual direction. However it's not totally consistent with @manueljlin's mockups in T10470 so let's make sure he's happy too.

applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
195

whitespace

197

whitespace

247

Is a hardcoded white color always guaranteed to look good even when the album art is very light? Is the blurred background always going to be dark enough?

251

I think that's the default, no?

298

text.length !== 0 is a bit faster

323

If the user does something a bit silly like making the applet as big as the whole screen, they might prefer if the seek bar is really long

cblack marked 2 inline comments as done.Wed, Feb 5, 2:10 AM
cblack added inline comments.
applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
195

Whitespace is intentional to help with code legibility

cblack updated this revision to Diff 75025.Wed, Feb 5, 2:11 AM
cblack marked an inline comment as done.

Adjust layout of components

cblack edited the test plan for this revision. (Show Details)Wed, Feb 5, 2:11 AM
cblack marked an inline comment as done.Wed, Feb 5, 2:21 AM
cblack added inline comments.
applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
247

ngraham added inline comments.Wed, Feb 5, 3:10 AM
applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
247

All right, looks good!

davidedmundson added inline comments.
applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
452

Items in a layout shouldn't specify a width

manueljlin added a comment.EditedWed, Feb 5, 7:06 AM

I love how it looks :D
The only thing I would change is the progress bar padding by either extending the bar to the left border to the album art or to the ~11px border from before

edit2: nvm about the comment from line 216 oops

gvgeo added a subscriber: gvgeo.EditedWed, Feb 5, 7:16 AM

Maybe I'm wrong, don't give too much weight on these comments.

edit, clarification for 216: white theme and no background image.

applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
243–249

You are limiting Details Column's width, while Album Art is not always visible.

247

With a white theme?

broulik added a subscriber: broulik.Wed, Feb 5, 8:45 AM

When there is no player it completely falls apart. It also doesn't work when there is no album art. The fallback player icon is gone.

The line spacing also seems a little excessive. Since there's a massive right padding to match the one on the left, you often end up with awkward line breaks:


Note how the "seh" could have easily fitted in the same line. It now also looks a bit awkward when you can't seek where there's no slider handle.

It looks really lovely with short texts


but you know...

And obviously I would prefer a better look for the player selection combo :)

applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
139

Superfluous since you fill both width and height?

147–150

Do you have a source for this claim? From what I know blur merely blurs the actual item texture which is rendered, no the raw image source?

235

Does assigning undefined work? I don't see a RESET property for it. Probably want to assign color scope text color instead?

243–249

Make sure to Math.round random factors of a size

323

No magic numbers, please

340

The labels are shown outside the album art, so they are unreadable now

cblack updated this revision to Diff 75075.Wed, Feb 5, 11:47 PM
cblack marked an inline comment as done.

Address feedback

cblack edited the summary of this revision. (Show Details)Wed, Feb 5, 11:48 PM
ndavis added a subscriber: ndavis.Thu, Feb 6, 2:24 AM

I don't think putting the source selector on the bottom is better. It moves the buttons farther away from where the mouse would be when opening the widget on a default panel.

cblack edited the test plan for this revision. (Show Details)Thu, Feb 6, 2:29 AM
cblack updated this revision to Diff 75076.Thu, Feb 6, 2:30 AM

Fix plasmashell freeze

ndavis added a comment.Thu, Feb 6, 2:33 AM

Looks like my previous comment is obsolete. I think tabs are more convenient than a combo box in this case, but I'm not sure this is the best use of tabs. AFAIK, we never use tabs as an actual selection control anywhere, so this behavior is unexpected.

Or are these actually different views for controlling all the applications separately rather than just switching sources?

Yeah at least with text on the side, tabs won't work at all for smaller sizes, such as in the system tray:

That's with just two sources!

If the text were on the bottom and it was limited to wide sizes, I could maybe see that working.

cblack updated this revision to Diff 75077.Thu, Feb 6, 3:40 AM

Use toolbutton (for initial visual feedback; need to apply colour scheme as to prevent legibility issues)

cblack edited the test plan for this revision. (Show Details)Thu, Feb 6, 3:40 AM
cblack planned changes to this revision.Thu, Feb 6, 3:47 AM
cblack marked 9 inline comments as done.Thu, Feb 6, 8:42 PM
cblack updated this revision to Diff 75138.Thu, Feb 6, 9:00 PM

Cache data from the MPRIS sources

cblack marked an inline comment as done.Thu, Feb 6, 9:05 PM
cblack added inline comments.
cblack updated this revision to Diff 75140.Thu, Feb 6, 9:09 PM
cblack marked 7 inline comments as done.

Address feedback

cblack updated this revision to Diff 75141.Thu, Feb 6, 9:37 PM

Move source selector to bottom of column, hide by default

cblack updated this revision to Diff 75142.Thu, Feb 6, 9:38 PM

Add license headers to two new QML files

cblack edited the test plan for this revision. (Show Details)Thu, Feb 6, 9:40 PM
cblack updated this revision to Diff 75143.Thu, Feb 6, 9:42 PM

Remove extraneous print

cblack updated this revision to Diff 75144.Thu, Feb 6, 9:43 PM

Swap in gridUnit instead of largeSpacing

cblack retitled this revision from [mediacontroller] WIP: Visually refresh media controller plasmoid to [applets/mediacontroller] Visually refresh media controller plasmoid.Thu, Feb 6, 9:55 PM

Overall looks much better I think! However the minimum height seems to be a bit tall now:

Also I have a hard time putting my finger on why, but somehow the scrubber not filling the full width feels odd to me, especially when the applet is wider than it is tall:

When the scrubber is the same width as the player controls below it, it seems fine. But when the window is wider and they no longer align vertically, it doesn't feel quite right to me.

ngraham requested changes to this revision.Wed, Feb 12, 4:14 AM
ngraham added inline comments.
applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
261

there's a massive binding loop somewhere here:

file:///home/nate/kde/usr/share/plasma/plasmoids/org.kde.plasma.mediacontroller/contents/ui/ExpandedRepresentation.qml:261:21: QML Heading: Binding loop detected for property "height"
This revision now requires changes to proceed.Wed, Feb 12, 4:14 AM
davidedmundson added inline comments.Wed, Feb 12, 7:03 AM
applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
147–150

That says the opposite of what you want.

We want image's texture provider to be used directly. Only then will this sourcesize matter.

This is saying when layer effect is enabled it uses the base blitting path.

We don't want to use layer here, we want the image as a source to the blur directly