Increase System Tray Plasmoid spacing value slightly
AbandonedPublic

Authored by The-Feren-OS-Dev on Feb 16 2020, 5:29 PM.

Details

Reviewers
ngraham
davidre
Group Reviewers
VDG
Summary

In my honest opinion Plasma's System Tray Plasmoid's current spacing value is currently set to 0, which leads to most of the tray icons looking rather cramped when put next to each other. This small patch increases the System Tray Plasmoid's spacing value from its current 0 value to Math.round(units.smallSpacing * 0.7).

Test Plan
  1. Change 'spacing: 0' to 'spacing: Math.round(units.smallSpacing * 0.7)' in /usr/share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/main.qml - what this patch is basically doing anyway
  2. Reload plasmashell and add a System Tray Plasmoid if one isn't already added

Breeze:
Before:


After:

Before:


After:

Feren OS's Plasma Theme:
Before:


After:

Before:


After:

Reactionary (3rd-party theme example):
Before:


After:

Before:


After:

(ignore the blur on the tray expand button, that's unrelated to this patch)

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22553
Build 22571: arc lint + arc unit
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 16 2020, 5:29 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
The-Feren-OS-Dev requested review of this revision.Feb 16 2020, 5:29 PM
ngraham requested changes to this revision.Feb 16 2020, 5:33 PM
ngraham added a subscriber: ngraham.
ngraham added inline comments.
applets/systemtray/package/contents/ui/main.qml
290

Don't use magic values. How about units.smallSpacing would be more semantically appropriate. That maps to 2, BTW. If 2 is still too cramped, you could do units.smallSpacing * 1.5 but using default spacing values is probably preferred.

This revision now requires changes to proceed.Feb 16 2020, 5:33 PM

Before:

After:

Patch update coming in a bit.

Changed spacing to units.smallSpacing

The-Feren-OS-Dev retitled this revision from Set System Tray Plasmoid spacing value from 0 to 3 to Set System Tray Plasmoid spacing value from 0 to units.smallSpacing.Feb 16 2020, 5:46 PM
The-Feren-OS-Dev edited the summary of this revision. (Show Details)
The-Feren-OS-Dev edited the test plan for this revision. (Show Details)

I think 0 doesn't look cramped at all. Increasing the spacing also increases the space usage . -1 from me

Maybe units.smallSpacing / 2, i'm pretty sure people that use show all entries in systray will be unhappy to see no free space in their panels.


How it looks now with 0 spacing, with 2 will have no space in panel at all.

Here's it with / 2 added to the spacing value:

The-Feren-OS-Dev added a comment.EditedFeb 16 2020, 5:56 PM

Would people prefer that above design to 2px? (actual Units.smallSpacing according to the HIG - https://hig.kde.org/layout/units.html) ^ (above message)

The-Feren-OS-Dev edited the summary of this revision. (Show Details)Feb 16 2020, 5:57 PM
The-Feren-OS-Dev edited the summary of this revision. (Show Details)Feb 16 2020, 5:59 PM

Which one would people prefer?
Units.smallSpacing:

Units.smallSpacing / 2:

ndavis added a subscriber: ndavis.Feb 16 2020, 6:43 PM

Which one would people prefer?
Units.smallSpacing:

Units.smallSpacing / 2:

It doesn't matter that much to me. If it results in more clickable area for each systray item, then smallSpacing might be better.

Please check how this looks with a vertical panel.

Please check how this looks with a vertical panel.

Looks a whole lot better on Vertical than without spacing IMHO (unitSpacing):

VS same but with / 2:

ngraham resigned from this revision.Feb 16 2020, 6:51 PM

I have a vertical panel and use large tray icons but intentionally minimize their number, and the result looks fine to me IMO:

However I'll admit that I thought the status quo was fine too. So I have no particular opinion on the aesthetics of this patch, but I think we should be careful to avoid adding too much spacing because the people who have and like lots of tray icons have a legitimate concern here. To me, units.smallSpacing (i.e. 2) is the absolute max we should do.

To make this patch easier to review without applying it, please add all the various sets of before-and-after images to the Test Plan section, not in comments.

The-Feren-OS-Dev edited the test plan for this revision. (Show Details)Feb 16 2020, 7:06 PM

To make this patch easier to review without applying it, please add all the various sets of before-and-after images to the Test Plan section, not in comments.

Done.

ngraham added a comment.EditedFeb 16 2020, 7:08 PM

Also, it's nice to have the "after" versions right after each corresponding "before" version so you can more easily compare them by clicking on one and navigating between them with the arrow keys. :)

The-Feren-OS-Dev edited the test plan for this revision. (Show Details)Feb 16 2020, 7:15 PM

Also, it's nice to have the "after" versions right after each corresponding "before" version so you can more easily compare them by clicking on one and navigating between them with the arrow keys. :)

Also done

After clicking through the before after I still think it's to much

After a bit more digging around the code, I found another alternative way to do this which also fixes a bug I just noticed with this patch (there's clicking deadzones in-between the tray icons), however the method for doing it causes this spacing:

I'll do a bit more digging on this alternative way to see if I can get it to be a spacing we can all agree on more than the current one.

The-Feren-OS-Dev planned changes to this revision.EditedFeb 16 2020, 7:41 PM
The-Feren-OS-Dev marked an inline comment as done.

Managed to come to a good compromise, however now I've hit a bit of a roadblock.

I've found that by editing these lines:

property int left: Math.round(units.smallSpacing / 2)
property int top: Math.round(units.smallSpacing / 2)
property int right: Math.round(units.smallSpacing / 2)
property int bottom: Math.round(units.smallSpacing / 2)

...to replace them with this:

property int left: Math.round(units.smallSpacing * 0.85)
property int top: Math.round(units.smallSpacing * 0.85)
property int right: Math.round(units.smallSpacing * 0.85)
property int bottom: Math.round(units.smallSpacing * 0.85)

...provides a better looking tray while not being that big spacing wise, while ALSO ever-so-slightly increasing hitboxes for each tray icon in the process:

However, there's one problem: Those lines aren't in the file currently being patched. They are there on the compiled version of the file, but for some reason they seem to be absent from the pre-compiled version of the file. I'll do some investigation, however for now I'll just mark as Changed Planned until I find a solution to the roadblock preventing me from replacing this patch with a better rendition of it.

Nevermind, then, I guess something changed between now and Plasma 5.18.1 master code meaning that no longer works, back to the old method...

Changed value to Math.round(units.smallSpacing * 0.7)

The-Feren-OS-Dev edited the summary of this revision. (Show Details)Feb 16 2020, 9:18 PM
The-Feren-OS-Dev edited the test plan for this revision. (Show Details)
The-Feren-OS-Dev retitled this revision from Set System Tray Plasmoid spacing value from 0 to units.smallSpacing to Increase System Tray Plasmoid spacing value slightly.
filipf added a subscriber: filipf.Feb 16 2020, 10:18 PM

copy paste from Telegram

fwiw I like units.smallSpacing / 2 more but it's not that big of a difference

2*0.7 rounded down is going to be 1. Just divide units.smallSpacing by two if you want 1.

But then I have to wonder... is it really worth it to add one pixel of spacing?

Went to 0.5, because 0.6 indeed looks just like 0.5 in execution

Fair point, just checked that now and they're indeed the same. Got confused with 0.65 as 0.65 looks different to 0.5.

ngraham accepted this revision as: ngraham.Feb 16 2020, 10:37 PM

Hopefully people won't murder us over one pixel. :) IMO time to finish the bikeshedding if VDG people are happy with it and everyone else can tolerate it.

There is already a margin added around icons, "units.smallSpacing / 2" if I remember correctly.
IMO it is OK without additional spacing (maybe because I'm used to it?). Anyway, VDG should decide what is the best and what is consistent with other elements of Plasma.

One more patch update to keep the change consistent with the rest of the code

davidre requested changes to this revision.Feb 17 2020, 9:08 AM

There is now not clickable space between items:


At least it should be clickable

This revision now requires changes to proceed.Feb 17 2020, 9:08 AM


The spacing I have here currently is already excessive imho.

There is now not clickable space between items:


At least it should be clickable

For future reference, how do you get that size examining mode in Plasma Desktop Workspace?

There is now not clickable space between items:


At least it should be clickable

For future reference, how do you get that size examining mode in Plasma Desktop Workspace?

That's inside gammaray

Changed method used for increasing spacing between tray icons to something that increases hitbox sizes as well

There is now not clickable space between items:


At least it should be clickable

For future reference, how do you get that size examining mode in Plasma Desktop Workspace?

That's inside gammaray

For some reason I couldn't get gammaray to do that over here, but I did come up with a new method for the patch that should increase hitboxes as well as icon spacing. Could you check if it does so, just to be sure?

Tweaked an added comment

This should probably be two patches at this point: one to increase the spacing when using tablet mode (which IMO is an uncontroversial no-brainer) and another to increase the spacing when in desktop mode.

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

This should probably be rewritten as an inline function as it's now very difficult to read. e.g.:

readonly property int itemSize {
    if (Kirigami.Settings.tabletMode) {
        return foo;
    } else {
        [put other logic here etc]
        return bar;
    }
}
The-Feren-OS-Dev added a comment.EditedFeb 17 2020, 6:40 PM

@ngraham How would I split it into two patches? Both patches would edit the exact same line.

https://community.kde.org/Infrastructure/Phabricator#Marking_patches_as_dependent_on_other_patches

It would probably be simplest do just do this though:

  1. Abandon this patch
  2. Submit a patch to refactor that logic to use a nested multi-line function for readability, as proposed in my comment
  3. After that patch lands, submit two more patches, one to increase the spacing when in tablet mode, and another one to increase the spacing when in desktop mode. At this point each patch will be changing a different line in the function

Clear as mud? :)

I agree with @broulik in that the spacing already feels quite excessive, or at least doesn't need to be increased (:

Does this depend on DPI/panel size maybe?

The-Feren-OS-Dev abandoned this revision.Feb 17 2020, 7:57 PM

Alright, I'll start splitting the patches up now. Marking this one as Abandoned.

I agree with @broulik in that the spacing already feels quite excessive, or at least doesn't need to be increased (:

Does this depend on DPI/panel size maybe?

Units.smallSpacing should change depending on the DPI, from what I've heard.

The-Feren-OS-Dev added a comment.EditedFeb 17 2020, 8:08 PM

...one to increase the spacing when in tablet mode, and another one to increase the spacing when in desktop mode. At this point each patch will be changing a different line in the function

Due to how the current patch handles it, I'll probably have to all of step 3 as one patch since they change the same line as each other, but for sure I can split steps 2 and 3(1)+3(2) of your post into individual patches.

Actually, on second thought, @ngraham I'm not even sure if step 2 would even be worth it on its own, if possible.

On your suggestion you have it adjusted to have step 3's tablet mode checker in step 2. If I don't have that check in place, for step 2, then it'd just be how it currently is anyway on master, effectively resulting in nothing to edit for a patch. I'll definitely do Step 3 as a patch, though.

The initial patch of the three, now I've thought up a suitable way of doing this, has been made:
https://phabricator.kde.org/D27465