Increase UI commonality between KCM and applet
ClosedPublic

Authored by ngraham on Oct 3 2019, 10:13 PM.

Details

Summary

This patch beings the user interface of the applet and the KCM closer together. To
accomplish this, the following changes are made:

  • Move the "Default Device" button onto the first row in the applet and make it a ToolButton to reduce its visual weight
  • In both the applet and the KCM, show the port chooser UI in combobox form only when multiple ports are available; otherwise show only a text label. This is allowed by the HIG because it is a case of hardware availability (https://hig.kde.org/plattform/settings.html?highlight=inapplicable#behavior)
  • Remove the port choosing functionality from the applet's hamburger menu since it's now available in the main UI when applicable

Depends on D24402

Test Plan

Simple case:


More complex case:

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.
ngraham created this revision.Oct 3 2019, 10:13 PM
Restricted Application added a project: Plasma. Β· View Herald TranscriptOct 3 2019, 10:13 PM
Restricted Application added a subscriber: plasma-devel. Β· View Herald Transcript
ngraham requested review of this revision.Oct 3 2019, 10:13 PM
ngraham updated this revision to Diff 67293.Oct 3 2019, 10:17 PM

Correct porting error in KCM combobox model generation

mmustac added a subscriber: mmustac.Oct 4 2019, 5:51 AM

Not related to this patch, but...
While looking at these screenshots I find it not self-explaining but more confusing that both buttons look the same, except one of them is in "active" state.
I would propose to change the icon for the non-default device to an empty star, with just an outline to be more clear which device is the current default.

Not related to this patch, but...
While looking at these screenshots I find it not self-explaining but more confusing that both buttons look the same, except one of them is in "active" state.
I would propose to change the icon for the non-default device to an empty star, with just an outline to be more clear which device is the current default.

I actually did that in the original diff after @GB_2 suggested it and quite liked the idea. However we ran into a potential issue after @ndavis rightly pointed out that we couldn't guarantee that for any given icon name pair in any icon theme, one would have a filled star and the other would have an empty star. This is the way it is in the Breeze icon theme, but it's not necessarily set up like that in other icon themes.

Maybe we can revisit that later or put appropriate icons in the plasma icon theme or something. I'd like to find a way to implement that, too.

ngraham edited the summary of this revision. (Show Details)Oct 6 2019, 10:44 PM
ngraham added a reviewer: sefaeyeoglu.

@nicolasfella do you have thoughts?

The code looks fine, but then I don't really do qml on a daily basis. Visually I find the applet crowded, but maybe that's just me.


Looks like replacing the label with a combobox breaks alignment in the KCM and exceeds the full width in the applet.

ngraham updated this revision to Diff 67610.Oct 10 2019, 2:10 PM

Fix overflow in the applet and poor alignment in the KCM when the combobox is visible

Thanks for testing that, @sefaeyeoglu. I don't actually have a combination of devices that makes the combobox appear so that's much appreciated.

Nice work! It looks very good now.

One odd thing I just noticed:

That should be fixed by D24402, in plasma-framework. Can you verify that it's fixed if you apply that or build plasma-framework from master?

sefaeyeoglu added a comment.EditedOct 10 2019, 2:33 PM

That should be fixed by D24402, in plasma-framework. Can you verify that it's fixed if you apply that or build plasma-framework from master?

Sorry that I didn't really make it obvious what I was referring to. For some reason I can check multiple PA sources as default. Sinks on the other hand behave as they should and change their checked state.

P.S. I am master of all repos right now (using kdesrc-build)

That should be fixed by D24402, in plasma-framework. Can you verify that it's fixed if you apply that or build plasma-framework from master?

Sorry that I didn't really make it obvious what I was referring to. For some reason I can check multiple PA sources as default. Sinks on the other hand behave as they should and change their checked state.

P.S. I am master of all repos right now (using kdesrc-build)

How strange. Is that a pre-existing problem, or was it introduced by this patch?

That should be fixed by D24402, in plasma-framework. Can you verify that it's fixed if you apply that or build plasma-framework from master?

Sorry that I didn't really make it obvious what I was referring to. For some reason I can check multiple PA sources as default. Sinks on the other hand behave as they should and change their checked state.

P.S. I am master of all repos right now (using kdesrc-build)

How strange. Is that a pre-existing problem, or was it introduced by this patch?

I just checked out master on plasma-pa again and it seems to happen there, too

I had a feeling. I say let's land this, then fix that. Do you agree?

sefaeyeoglu accepted this revision.EditedOct 10 2019, 2:38 PM

I had a feeling. I say let's land this, then fix that. Do you agree?

No objections πŸ˜ƒ

Edit: I just restarted my pulseaudio daemon and it didn't happen after that. Weird.

This revision is now accepted and ready to land.Oct 10 2019, 2:38 PM
This revision was automatically updated to reflect the committed changes.