Fix system tray UI/UX & refactor
ClosedPublic

Authored by ratijas on Mar 14 2019, 4:20 AM.

Details

Summary

System tray widget had the following UX problem:
Icons are laid out in a Flow QML layout, using minimal amount of
space, thus not filling the height/width of the task bar. In other
words: user can only click directly on an icon, not over or under it.

Consider the following scenario:

Given icon size X and task bar of height 1.5 * X located at the bottom;
User moves pointer down to the limit and tries to click the icon.
Expected outcome: applet is activated.
Actual outcome: nothing happens, because icon (together with mouse
area) floats slightly above the bottom.

Which is inconvenient, especially when most other widgets tend to fill
up the space.

This patch fixes aforementioned problem by refactoring layouts using
modern GridLayout, RowLayout et al., so that icons are arranged in
rows and columns based on their number, and each one fills up its
cell. I also made a handful of minor internal refactorings and fixes.
Unfortunately, due to tight coupling, almost all files needed changes
anyway.

Special note on 'CompactApplet.location': it didn't seem to affect
anything at all, so removed it.

At the end of the day no visual changes should be noticeable. Layout
works in both vertical and horizontal form-factor an all four sides of
the desktop.

Test Plan

Please, check whether 'LayoutMirroring' works properly.

Diff Detail

Repository
R120 Plasma Workspace
Branch
fix-system-tray
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11853
Build 11871: arc lint + arc unit
ratijas created this revision.Mar 14 2019, 4:20 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 14 2019, 4:20 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ratijas requested review of this revision.Mar 14 2019, 4:20 AM
anthonyfieroni added a subscriber: anthonyfieroni.

Video or screenshot will be helpful.

Video or screenshot will be helpful.

Here they are, "debug" mode via colored rectangles with anchors.fill: parent.


ndavis added a subscriber: ndavis.Mar 14 2019, 6:40 AM

+1 to this. Fitts' law comes to mind.

Pretty nice stuff! While you're at it, how about implementing a UI change so that while the popup is open, its contents change as you slide the mouse along the icons (but only when there's a single row/column)?

ratijas added a comment.EditedMar 14 2019, 4:52 PM

Pretty nice stuff! While you're at it, how about implementing a UI change so that while the popup is open, its contents change as you slide the mouse along the icons (but only when there's a single row/column)?

Only as an optional feature. Will it be usable, though? Anyway, it'll be a separate pull request. Let me see if I can manage my time this week.

Hello,

Any progress on this one? Does it usually take long to review?

Sorry for impatience.

A few more days of waiting may be required since most of the Plasma developers are quite busy right now. But it will get reviewed, don't worry!

I'm afraid the more we wait, the more code base is diverging, which will require more patches for this pull request to be accepted. Btw, I'm still stuck with pinning old version in my package manager.

I will review this, sorry it's taken so much time.

I had to solve a minor merge conflict when rebasing this on master. Could you do that and update the patch? Thanks!

My parent commit was 169238848136; current master is f8d5e1a2. While on master, I am doing git diff --summary 169238848136. There is no intersection in changed files with my pull request. Still patch (as you aforementioned) can not be applied to master without tweaking.

Am I missing something? Is git diff hiding something?

What you need to do is rebase the branch for your patch on current master: git pull --rebase origin master

Then fix up the merge conflicts. Then run arc diff again.

Oh, nevermind. I forgot I should be using arc indeed.

I love use of layouts for everything, so in general ++

applets/systemtray/package/contents/applet/CompactApplet.qml
34

When you say it did nothing, You'd need to test every hidden applet whilst having the panel on the left.

See D1253

If it ain't broke...

applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
45

Huh? how did this work before?

applets/systemtray/package/contents/ui/main.qml
68

Not really.
Using objectNames is a bit of an anti pattern, especially when QML has so the built-in component scope hierachy.

We use it in the system tray already, and it's arguably no worse than the existing applet.parent.parent.

So fine here, but only because the system is mad.

applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
45

Applets do it by itself, when it not do, it does not work like Weather Widget embed in systray.

So, I've merged done git rebase on master — just one conflicting line, rewritten after me in a better way. But now I have no way to test it, because CMake Error Could not find a configuration file for package "ECM" that is compatible with requested version "5.58.0" and even all-up-to-date Arch Linux does not provide those pieces. I've installed the latest CMake from the testing repository, but other versioning issues showed up.

I'm not a big fan of submitting without checking, but I don't see any other way around.

applets/systemtray/package/contents/applet/CompactApplet.qml
34

As far as I remember, one of the branches never got executed in my environment. Could you point to specific use case which requires this "almost dead" code?

ratijas added inline comments.May 15 2019, 11:43 AM
applets/systemtray/package/contents/ui/main.qml
68

Pattern or not — Qt provides us with this objectName property so we can do stuff. But Qt itself is not engaged in providing further support for it. That's frustrating.

In D19745#465569, @ratijastk wrote:

So, I've merged done git rebase on master — just one conflicting line, rewritten after me in a better way. But now I have no way to test it, because CMake Error Could not find a configuration file for package "ECM" that is compatible with requested version "5.58.0" and even all-up-to-date Arch Linux does not provide those pieces. I've installed the latest CMake from the testing repository, but other versioning issues showed up.

I'm not a big fan of submitting without checking, but I don't see any other way around.

ECM is an KDE project https://api.kde.org/ecm/ and part of frameworks. Try installing extra-cmake-modules (don't know the exact name on arch).

Even with extra-cmake-modules 5.58, kcoreaddons needs to be updated as well... work in progress... please, be patient: i have autism^W slow hardware.

5.58.0 releases just rolled out, upgrading...

ratijas updated this revision to Diff 58123.May 15 2019, 1:16 PM

Rebase on most recent master

Double checked on my system. Nothing bad happened so far.

Bump (again)

davidedmundson accepted this revision.May 18 2019, 6:33 PM
davidedmundson added inline comments.
applets/systemtray/package/contents/applet/CompactApplet.qml
34

You will enter this branch if the system tray is on the left.

It doesn't make a difference now since Dialog gained support to not not overlap panels and to switch edges if it doesn't fit.

This puts the location in a "wrong" way we hit the conflict detection in Dialog and it fixes it.

This revision is now accepted and ready to land.May 18 2019, 6:33 PM

Please, check whether 'LayoutMirroring' works properly.

To test layout mirroring run "plasmashell --reverse"
Layouts will auto handle most things, so things should be fine.

Do you have commit access?

ngraham added a subscriber: aacid.May 19 2019, 4:36 AM

Please, check whether 'LayoutMirroring' works properly.

To test layout mirroring run "plasmashell --reverse"
Layouts will auto handle most things, so things should be fine.

Do you have commit access?

BTW @aacid recently taught me that it's possible to find out this information for oneself by searching for a username, real name, or email address on https://websvn.kde.org/trunk/kde-common/accounts?view=markup.

@davidedmundson

Thanks.

It is my first contribution to KDE/Plasma, so I do hope that I don't have a commit access.

applets/systemtray/package/contents/applet/CompactApplet.qml
34

Honestly, I tried putting plasma panel on each of four sides (naturally including left and right), but for some reason I still did not enter the branch. Or maybe it did, but no visual changes were observed. I don't remember.

It was not an easy decision to remove someone else's code which I do not understand. Whereas "(a) I do not understand; and (b) I has no visual effect" sounds a little bit more convincing.

As far as i get it from your reply, it's now Dialog's unified responsibility (which is fine, I guess) so this lines were redundant already.

This revision was automatically updated to reflect the committed changes.

Thanks again. Very nice patch. May it be the first of many! :)

Mouse buttons aren't propagated to the applet anymore, e.g. middle click on volume icon to mute no longer works.

applets/systemtray/package/contents/ui/items/AbstractItem.qml
22

You sure this high version number is neccessary?

30–33

Avoid doing negated disjunctions for clarity: !labelVisible && !vertical

74

For some reason the icons are now smaller than they were before, observed on two separate computers, they used to be the same as the panel icon here:

76

This margin stuff doesn't work with right to left layout.
Run plasmashell -reverse and observe that there's no left padding in the item anymore:

130

Can you just make the iconItem have a anchors.fill: parent?

@ratijastk, are you interested in sending a follow-up patch to address @broulik's comments?

ratijas added inline comments.May 27 2019, 8:22 AM
applets/systemtray/package/contents/ui/items/AbstractItem.qml
22

Not exactly. But old layouts used to have a load of restrictions, for sure.

30–33

Oh, give it a rest. These two lines looking good together, this way it is clear that they are an opposite of each other.

74

Not even sure, is it directly my mistake, or rebase on that later master branch. Could you provide more specific numbers, please?

130

maaayyybe...

Will you address the broken event delivery to the compact applet item?

applets/systemtray/package/contents/ui/items/AbstractItem.qml
22

"Old layouts"? You only use Layouts 1.2 features as far as I can tell

30–33

You first have to evaluate the inner statement in your head and then negate it vs. just seeing at a glance what it is supposed to do

74

When I revert this patch in master the icons return to normal, so this isn't a rebase issue.

@ratijastk please feel free to submit another patch that does the same thing, but addresses @broulik's concerns. Thanks!