Make KCM look more like applet
ClosedPublic

Authored by sefaeyeoglu on Sep 23 2019, 3:28 PM.

Details

Summary

I was looking through changes in plasma-pa and noticed, that the KCM looks very different in contrast to the applet.

Test Plan

Checked for mute button function on all occasions (sinks, sources, applications playing, applications recording). Checked if volume slider works as expected.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Branch
feat-applet-ux
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17067
Build 17085: arc lint + arc unit
sefaeyeoglu created this revision.Sep 23 2019, 3:28 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 23 2019, 3:28 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sefaeyeoglu requested review of this revision.Sep 23 2019, 3:28 PM
GB_2 added reviewers: Plasma, VDG.EditedSep 23 2019, 3:31 PM
GB_2 added a project: VDG.
GB_2 added a subscriber: Plasma.
GB_2 added a subscriber: GB_2.

Please add reviewers and subscribers next time.

sefaeyeoglu edited the summary of this revision. (Show Details)Sep 23 2019, 3:32 PM
sefaeyeoglu edited the test plan for this revision. (Show Details)
In D24161#536408, @GB_2 wrote:

Please add reviewers and subscribers next time.

Sorry. I didn't know who I should add so I just added VDG as subscriber.

davidedmundson added inline comments.
src/kcm/package/contents/ui/Applications.qml
87

please don't remove the i18nd

you'll break translations if the kcm is loaded within the applet

Set domain for all i18n strings

sefaeyeoglu marked an inline comment as done.Sep 23 2019, 3:40 PM
sefaeyeoglu added inline comments.
src/kcm/package/contents/ui/MuteButton.qml
32

I actually think this is the wrong way of doing tooltips here. The applet does it with PlasmaComponents. How can I create a tooltip with QCC?

ngraham added inline comments.
src/kcm/package/contents/ui/Applications.qml
49

I don't think the addition of the word "Streams" adds useful information here.

90

ditto

src/kcm/package/contents/ui/MuteButton.qml
23–24

If you're going to change this (and +1 for it), make it say import QtQuick.Controls 2.5 as QQC2 and then add the QQC2.<whatever> namespace

32

Using the attached ToolTip property is just fine here for QQC2 items.

sefaeyeoglu added inline comments.Sep 24 2019, 6:18 PM
src/kcm/package/contents/ui/Applications.qml
49

The applet already says Streams here.

src/kcm/package/contents/ui/MuteButton.qml
32

I feel like QQC2 tooltips look out of place. They don't have the same shadow as normal tooltips and do not fade in

Import QtQuick.Controls as QQC2 and fix ToolTip delay

sefaeyeoglu marked 3 inline comments as done.Sep 24 2019, 6:51 PM
ngraham added inline comments.Sep 26 2019, 3:12 PM
src/kcm/package/contents/ui/MuteButton.qml
26

Now that this thing is moved next to the volume slider, I feel like it should be a ToolButton, like in the applet.

Use QQC2 ToolButton instead of Button for Mute. Add stepping to volume slider, similar to applet.

sefaeyeoglu marked 2 inline comments as done.Sep 26 2019, 5:52 PM
sefaeyeoglu edited the test plan for this revision. (Show Details)
sefaeyeoglu edited the test plan for this revision. (Show Details)
sefaeyeoglu edited the test plan for this revision. (Show Details)

Remove redundant whitespace from copyright header

ngraham added inline comments.Sep 26 2019, 6:42 PM
src/kcm/package/contents/ui/DeviceListItem.qml
54

While we're at it, let's also make this say "Port:"

src/kcm/package/contents/ui/VolumeSlider.qml
44

Hmm, I don't like how this makes tickmarks appear. If you need to round the value, you can do that in onMoved: { blabla; }, or just round the value of the label itself.

Remove stepping again, change label text of port selection

sefaeyeoglu marked 2 inline comments as done.Sep 26 2019, 7:08 PM
sefaeyeoglu added inline comments.
src/kcm/package/contents/ui/VolumeSlider.qml
44

I have removed this now as I can't replicate the same behavior as the applet. Added a TODO for the future

sefaeyeoglu edited the test plan for this revision. (Show Details)Sep 26 2019, 7:10 PM

Nice.

Now that I compare the two, one remaining difference is that each item's icon differs in location in the applet vs the KCM. In the Applet, it's larger and to the left of everything else; whereas in the KCM the icon is small and in the same row as the title.

sefaeyeoglu marked 2 inline comments as done.Sep 26 2019, 7:19 PM

Right! I completely forgot about that. Will work on it

sefaeyeoglu edited the test plan for this revision. (Show Details)

Make icon full height for playback devices, too

sefaeyeoglu edited the test plan for this revision. (Show Details)Sep 26 2019, 7:31 PM
ngraham accepted this revision.Sep 26 2019, 8:02 PM
ngraham added a subscriber: drosca.

I love it, and the code changes look sane to me.

Plasma folks or @drosca, any objections?

This revision is now accepted and ready to land.Sep 26 2019, 8:02 PM

I love it, and the code changes look sane to me.

Plasma folks or @drosca, any objections?

Glad you like it 😄 Looks like this will be my premiere on "This week in KDE" 🎉

GB_2 added inline comments.Sep 27 2019, 4:35 AM
src/kcm/package/contents/ui/MuteButton.qml
32

That is something that needs to be fixed in the QQC2 desktop style, so it is unrelated to this.

GB_2 added a comment.Sep 27 2019, 12:36 PM

I'd also align the mute button with the slider, like in the applet.

src/kcm/package/contents/ui/DeviceListItem.qml
104

"Default Device"

Fix wrong display name of default-device button

sefaeyeoglu marked an inline comment as done.Sep 27 2019, 12:40 PM
sefaeyeoglu updated this revision to Diff 66945.EditedSep 27 2019, 12:54 PM

Align mute button centered with slider

Before:

After:

GB_2 accepted this revision as: GB_2.Sep 27 2019, 12:55 PM
sefaeyeoglu edited the test plan for this revision. (Show Details)Sep 27 2019, 12:55 PM
sefaeyeoglu marked an inline comment as done.Sep 27 2019, 12:59 PM
sefaeyeoglu edited the summary of this revision. (Show Details)Sep 27 2019, 6:14 PM
This revision was automatically updated to reflect the committed changes.

Nice work. Landed on master so we have plenty of time to polish it up in follow-up patches. Speaking of that, I have a suggestions for unifying the UI even more: for multi-port devices, show the port chooser combobox instead of the name of the current port, in both the KCM and the applet. There's guaranteed to be enough room (we'd be replacing a string with the same string in a combobox). Then remove the "Ports" items in the applet's hamburger menu.

Oh and I just noticed a regression I didn't see before landing the patch, sorry. :/ The icons no longer match between the KCM and the applet when I add an external bluetooth device:

Nice work. Landed on master so we have plenty of time to polish it up in follow-up patches. Speaking of that, I have a suggestions for unifying the UI even more: for multi-port devices, show the port chooser combobox instead of the name of the current port, in both the KCM and the applet. There's guaranteed to be enough room (we'd be replacing a string with the same string in a combobox). Then remove the "Ports" items in the applet's hamburger menu.

Oh and I just noticed a regression I didn't see before landing the patch, sorry. :/ The icons no longer match between the KCM and the applet when I add an external bluetooth device:

The applet uses a utility function in icon.js. It would be easy to fix that. But there would be one implementation detail I would need to know. As both the applet and the kcm have an icon.js can we somehow share this code between them, so we don't have duplicate code. Like some kind of "common" directory to put the icon.js into and load it from there? Or should we just symlink the file from one place to another?

Nice work. Landed on master so we have plenty of time to polish it up in follow-up patches. Speaking of that, I have a suggestions for unifying the UI even more: for multi-port devices, show the port chooser combobox instead of the name of the current port, in both the KCM and the applet. There's guaranteed to be enough room (we'd be replacing a string with the same string in a combobox). Then remove the "Ports" items in the applet's hamburger menu.

Oh and I just noticed a regression I didn't see before landing the patch, sorry. :/ The icons no longer match between the KCM and the applet when I add an external bluetooth device:

I created a new Diff to fix this: D24289