[Applet]Update layout based on T10470
ClosedPublic

Authored by gvgeo on Dec 29 2019, 9:43 AM.

Details

Summary

Change layout according to T10470 mockups.
Remove port combobox and put port selection back the hamburger menu.
Compact representation tooltip shows only port description, if available.
Display only port description, if there is no other device with same
description sub-string.

Visual Issues:
Unable to extent the bottom line to touch the edges of the window,
that are part of the system tray.
Line svg has spacing on the sides.

1st part of patches D26271, D26256, D26418, D26574.
Not included in these patches: 100% volume mark, Color style.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
gvgeo added a comment.Dec 29 2019, 8:13 PM

I was not able to change the checkbox size, nothing I tried worked.
Checked other places with smaller checkbox(like notifications). But could not found how the size is controlled.

Current view:

Awesome work, @gvego. I'm really liking all of these patches of yours. :)

The checkbox size issue is probably from the styling of the PlasmaComponents checkbox, as opposed to the basic QQC2 Checkbox. If I'm right, we'd need to fix the PC checkbox, or else just use the QQC2 one instead.

It might also be worth figuring out how to implement the different window/view background colors as depicted in the mockup, because it's a common pattern on those mockups. Might require tweaking the scrollable list appearance in the Plasma style, maybe?

gvgeo updated this revision to Diff 72373.EditedDec 30 2019, 12:06 PM

Brought back spacing, and corrected sizes.
Centered icons/boxes, and align text.
Reduced checkbox to 18 pixel.

QQC2 checkbox is 16 pixels, but does not scale. Instead used PlasmaComponents3 which is 18.

gvgeo edited the summary of this revision. (Show Details)Dec 30 2019, 12:17 PM

Without device icons, the text needs to be improved a bit, or else I see this when I have multiple devices:

applet/contents/ui/ListItemBase.qml
113

Give the radio button a text: property and remove the heading item; that way the text will be clickable

applet/contents/ui/main.qml
403–404

I think there isn't a line here in the mockup, right?

471–472

ditto

528

you can give it negative side margins to get it to touch the sides of the pop-up, as in the mockup

542

So this doesn't actually do anything yet? Obviously that needs to change before this can land. :) Also the checked state needs to be saved somewhere.

545

Don't use a separate Heading here; give the Checkbox a text: property

filipf added a subscriber: filipf.Dec 30 2019, 10:16 PM
filipf added inline comments.
applet/contents/ui/ListItemBase.qml
115

Margins should but don't get mirrored when using a RTL layout. I haven't tested anything, but I think you need to add a rightMargin when layout mirroring is enabled.

gvgeo updated this revision to Diff 72739.EditedJan 4 2020, 12:20 PM

Remove port combobox and put port selection back the hamburger menu.
-Now port selection appears only if there is at least 2 available ports.
Made devices/applications labels clickable.
Removed RTL margin problems.
Made playback/recording separation line smaller.(according to mockup)

gvgeo marked 7 inline comments as done.Jan 4 2020, 12:48 PM

About the devices labels, I feel it is fine as it is. It is not a problem, to have two devices with the same port name.
Only if could put a tooltip. But no, Radiobutton does not deserve a tooltip.
What else to do, without displaying the full device names?

I took a look at breeze, for the lines between list items and coloring and tab changes. I was lost and didn't know, what I was looking for exactly.
I will keep an eye, but... I feel this will be part of a more general breeze evolution.
By the time I will have decode how theme system works, it is possible that you will have already made the changes.

applet/contents/ui/main.qml
404–412

This is the line that separates the playback and recording applications.
Just notice that should not always be visible. Will fix.

gvgeo updated this revision to Diff 72750.EditedJan 4 2020, 3:34 PM

Add visibility to the line that splits playback and record.
On the application side, will show if there is 1 record and 1 playback
application. Because there is no port name to easily differentiate them.

Current Issue:
The lines added with svg, are not the same with mockup or current lines between playback devices.
There are 2 pixels, and have lower opacity (with 1 pixel spacing?).

gvgeo marked an inline comment as done.Jan 4 2020, 4:11 PM
ngraham requested changes to this revision.Jan 4 2020, 9:13 PM

Very nice work! I continue to be impressed with your contributions.

What else to do, without displaying the full device names?

Display full device names when there's more than one playback or input device, maybe. Or only when there are more than 2 and their names are identical.

applet/contents/ui/ListItemBase.qml
115

Don't multiply by 0.75; this could result in a fractional value and the spacing doesn't doesn't need to be literally pixel-identical with the mockup

116

Don't override this here. If it looks bad, let's fix it in the RadioButton control itself

127

This is the default wrap value; no need to explicitly set it

317

By choice -> Intentionally

applet/contents/ui/main.qml
317

Revert this change

This revision now requires changes to proceed.Jan 4 2020, 9:13 PM
gvgeo updated this revision to Diff 72854.EditedJan 6 2020, 9:37 AM

Changes:
Cleanup
Remove separation lines
Fix drag function
Full Name display when a port name sub-strings are same.
Requested changes
Fix bugs 393418 and 413448
Add RTL margins

Issues:

  1. Check comment at the bottom of main.qml in D26256

These two strange margins (including defaultButton in this patch), are required to vertical align the icons in their center, and the texts with the slider.

  1. Setting preferredHeight in fullRepresentation makes widget on desktop to load with size >= preferredHeight on boot.
gvgeo marked 5 inline comments as done.EditedJan 6 2020, 9:49 AM

...there's a 5px sized padding after the divider that cuts off part of the Internal Microphone...

Had to bring this back. To avoid fractional values and line getting fuzzy in two pixels.
Line has different opacity.

Current view:

gvgeo edited the summary of this revision. (Show Details)Jan 6 2020, 10:03 AM
ndavis added a subscriber: ndavis.Jan 6 2020, 10:08 AM

Why do the unchecked radiobuttons have a dark outline in your screenshot?

gvgeo added a comment.Jan 6 2020, 10:39 AM

It is a theme problem. Breeze light also has problem.
Widgets take color from Plasma Style. While in applications are fine.
I checked with this widget to compare.

If it isn't possible that's okay (it looks really nice right now, great job btw :D), although it'd be nice to make it consistent with the kirigami style divider

It is a theme problem. Breeze light also has problem.
Widgets take color from Plasma Style. While in applications are fine.
I checked with this widget to compare.

Interesting. I'll see if I can fix that.

gvgeo added inline comments.Jan 6 2020, 2:29 PM
applet/contents/ui/DeviceListItem.qml
36

if (description)

Lovely. Any chance we could make the bottom divider touch the edges of the window?

I suppose we'll need to adjust the Breeze Plasma theme for the changes to the tab bar appearance.

gvgeo added a comment.Jan 6 2020, 3:47 PM

Lovely. Any chance we could make the bottom divider touch the edges of the window?

Not really, not from here. This space is part of system tray. If you use the plasma-pa widget only, will see that it touches exactly the edge.
Bigger negative value does not show in the system tray, but would make the line appear outside the widget if used on desktop.

gvgeo updated this revision to Diff 72914.Jan 6 2020, 6:57 PM

Add description check

gvgeo marked an inline comment as done.Jan 6 2020, 6:59 PM
gvgeo planned changes to this revision.EditedJan 9 2020, 8:15 AM

Found a bug. There is no update when a port that is not in use gets unavailable.

Why is a problem in this patch:
When using port 1, if port 2 becomes unavailable, is possible to switch to port that is unavailable.
The real problem is you switch to port 2 then. Info gets updated, the UI see only one available port, thus removing the options. Locking you to the unavailable port. Only solution is to use KCM.

Don't know how to fix it. As workaround could:

  1. Make menu options always available.
  2. Or confirm port is available after the update, and switch back if need. But this would made impossible to select an unavailable port from kcm too.

Bug affects also fb5631928121

edit:
If I'm not wrong, this is a limitation from pulseaudio.
I don't see a case for availability change in context_state_callback function.

But this would made impossible to select an unavailable port from kcm too.

Is there an actual use case for this? If not, option 2 might make the most sense.

gvgeo added a comment.Jan 9 2020, 4:03 PM

It makes possible to change volume/mute before plug something. But have no idea, if there is any real reason.


About these 2 bugs https://bugs.kde.org/show_bug.cgi?id=393418, https://bugs.kde.org/show_bug.cgi?id=413448 .
Should I separate the fixes from this patch? It is only 8 lines, and afraid will get more reports if this patch gets delayed.

Layout.minimumHeight: units.gridUnit * 8
Layout.minimumWidth: units.gridUnit * 14
Layout.preferredHeight: units.gridUnit * 21
Layout.preferredWidth: units.gridUnit * 24
Plasmoid.switchHeight: Layout.minimumHeight
Plasmoid.switchWidth: Layout.minimumWidth
.
.
.
   Layout.preferredHeight: main.Layout.preferredHeight
   Layout.preferredWidth: main.Layout.preferredWidth

Yeah, feel free to fix those bugs separately so we can get them in more quickly.

Personally I don't think there's a major use case for manipulating the settings of a port when it's not actually being used.

gvgeo updated this revision to Diff 73238.EditedJan 10 2020, 10:21 PM

Separate bug fixes.
Change defaultButton visibility
Change defaultButton's RowLayout height

Workaround for the bug D26574

ngraham accepted this revision.Jan 10 2020, 11:20 PM

I'm good with this now as an initial implementation of the mockup. However we're a bit close to the Plasma 5.18 tagging date (January 16th). My gut instinct is to land these early in the 5.19 cycle rather than rushing to get everything into 5.18 and potentially shipping with un-noticed regressions.

On the other hand, the UI to switch the default device is much nicer with this patch series, and we've received a lot of user complaints about the big buttons I added to implement that in 5.17 (sorry).

Thoughts?

This revision is now accepted and ready to land.Jan 10 2020, 11:20 PM
gvgeo added a comment.Jan 11 2020, 6:18 AM

Can't include an untested patch because we are in a hurry.

To clarify my comment's D26574#591702 part "If these patches ever get accepted".
I wasn't aware of the release schedule, and my doubts are about code acceptance.

Found this request to display the name of the stream https://bugs.kde.org/show_bug.cgi?id=409453 .
Which was done, but remove here. Maybe should leave it too?
With a quick check, not many applications have this information.

Found this request to display the name of the stream https://bugs.kde.org/show_bug.cgi?id=409453 .
Which was done, but remove here. Maybe should leave it too?
With a quick check, not many applications have this information.

I'm skeptical of the need for this feature. It seems like something very few people will need. Out of all of those stream names, only one of them has information that could be considered possibly useful to most people. If there's a good way to not show the information except when it contains something useful, then go for it. Otherwise, I'd say leave it out.

ndavis added a subscriber: cfeck.Jan 13 2020, 1:51 PM

Since it's @cfeck's bug report, I'll add him to the subscribers.

cfeck added a comment.EditedJan 13 2020, 8:41 PM

The radio stream I use (via mpv) shows the interpret and title information for all titles it plays. Also, when mpv plays a local file, title information about the file is added there. I don't use any other media streams, so I cannot comment on the general availability of this additional stream information.

gvgeo updated this revision to Diff 74141.Wed, Jan 22, 5:12 PM

Update for latest scrollbar fixes (remove workaround).

gvgeo updated this revision to Diff 74145.Wed, Jan 22, 5:36 PM

Correct update. Last was wrong one.

manueljlin accepted this revision.Fri, Jan 24, 4:20 PM

Nice, looks great! About the stream name, it doesn't look like it's useful most of the time so it might be better to remove it

I applied the patches in this series and tested and went through the code again and everything looks good to me. Shall we start landing them for 5.19? We can always tweak things later; there's loads of time before the next release.

gvgeo edited the summary of this revision. (Show Details)Fri, Jan 31, 2:21 PM
This revision was automatically updated to reflect the committed changes.

@gvgeo though I tested this before and found it to be working, after landing the patch I'm now seeing that using the new radio buttons to change the default device does not automatically move all running streams to the new device, as it used to. Can you check this? Thanks!

gvgeo added a comment.Fri, Jan 31, 7:27 PM

@gvgeo though I tested this before and found it to be working, after landing the patch I'm now seeing that using the new radio buttons to change the default device does not automatically move all running streams to the new device, as it used to. Can you check this? Thanks!

This should not happen. The changes to the defaultButton were minimum, except the transformation to RadioButton, onClicked stayed the same. I suspect something else is to blame, but cannot replicate in two systems, and cannot see something in the code.

Can you check the console for any messages?
Next step is to confirm that was working before these patches. And try to pinpoint the responsible patch, by applying them 1 by 1 and testing.

davidedmundson added inline comments.
applet/contents/ui/ListItemBase.qml
111

That's asking for a binding loop.

An implicit size should never be based on a current size.

This could be:
Layout.minimumHeight: contextMenuButton.implicitHeight

Though I'm surprised you need this line at all.

gvgeo added a comment.Mon, Feb 3, 12:01 PM

Change to implicitHeight in D27117.

That line should be in D26574, messed up splitting the patches.
It keeps the height of the row fixed, so that it won't change when the button gets hidden.