[Applet]Fix device label problems
ClosedPublic

Authored by gvgeo on Feb 3 2020, 10:14 AM.

Details

Summary

Limit label maximum width, to avoid breaking UI.
Change label to use plasma components 3, for theme color compatibility.
Use of implicit sizes, to avoid binding loops.

BUG: 417074
BUG: 417106

Test Plan

Use applet outside of systemtray and limit it's width.
If device description is not long enough, enable in the configuration
"Add virtual output device for simultaneous output on all local sound cars".
Before: Hamburger menu moves to the right, and there is no eliding.
After: Proper display.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Branch
labelWidth2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22042
Build 22060: arc lint + arc unit
gvgeo created this revision.Feb 3 2020, 10:14 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 3 2020, 10:14 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gvgeo requested review of this revision.Feb 3 2020, 10:14 AM
gvgeo added inline comments.Feb 3 2020, 10:18 AM
applet/contents/ui/ListItemBase.qml
109

With testing I found 5 pixel spacing. But couldn't trace where it comes from, maybe padding around items?. Used smallSpacing instead to avoid another magic number.

gvgeo added a comment.EditedFeb 3 2020, 10:18 AM

@ davidedmundson Is this monstrosity correct? I tried to avoid binding loops by setting minimum maximum sizes. And the origin of the values is contextMenuButton.implicitHeight.
Never mind, it was simplified.

ngraham added inline comments.Feb 3 2020, 2:49 PM
applet/contents/ui/ListItemBase.qml
116

This feels wrong. The radio button shouldn't need a maximum width set on it.

129

Instead of giving this label a maximum width, maybe you could just have it be the item in the layout that gets Layout.fillWidth: true and then remove the Item below it

gvgeo planned changes to this revision.Feb 3 2020, 2:58 PM
gvgeo added inline comments.
applet/contents/ui/ListItemBase.qml
129

This will also solve the complexity of this patch.

gvgeo updated this revision to Diff 74933.Feb 3 2020, 4:11 PM

Simplify.
Fix label color issues.

gvgeo retitled this revision from [Applet]Fix long device name to [Applet]Fix device label problems.Feb 3 2020, 4:22 PM
gvgeo edited the summary of this revision. (Show Details)
gvgeo edited the test plan for this revision. (Show Details)
gvgeo updated this revision to Diff 74936.Feb 3 2020, 4:28 PM
gvgeo edited the test plan for this revision. (Show Details)

Code style

gvgeo marked 3 inline comments as done.Feb 3 2020, 4:29 PM
gvgeo edited the test plan for this revision. (Show Details)
gvgeo added a subscriber: cfeck.Feb 3 2020, 7:22 PM

Tried couple themes, but could not reproduce the color problem. https://bugs.kde.org/show_bug.cgi?id=417106
Hopefully this change make sense.

applet/contents/ui/ListItemBase.qml
127

Is this all it needs to fix text color bug?

ngraham requested changes to this revision.Feb 3 2020, 9:54 PM
ngraham added inline comments.
applet/contents/ui/ListItemBase.qml
116

Replace this line with Layout.fillWidth: true. This works because soloLabel and defaultButton will never be visible at the same time; when soloLabel isn't visible, then the radio button's label will be the item that's filling the width.

This revision now requires changes to proceed.Feb 3 2020, 9:54 PM
gvgeo added a comment.Feb 4 2020, 2:54 AM

It works like you said. The problem is that will be too easy, accidentally to change the default device. It is not right for the empty space, without any indication, to change your device.

Ah I see why you did it then. That makes some sense. Can you add a comment explaining this?

gvgeo updated this revision to Diff 74971.Feb 4 2020, 3:36 AM
gvgeo edited the summary of this revision. (Show Details)

Add comment

ngraham accepted this revision.Feb 4 2020, 8:27 PM

Thanks!

This revision is now accepted and ready to land.Feb 4 2020, 8:27 PM
This revision was automatically updated to reflect the committed changes.