Increase the size of system tray icon hitboxes on the System Tray Plasmoid
ClosedPublic

Authored by The-Feren-OS-Dev on Feb 17 2020, 9:44 PM.

Details

Summary

Depends on D27465

@ngraham recommended that I split up https://phabricator.kde.org/D27438 into three patches:

  1. The foundation code to get landed first so that the other two patches can be done as patches that change two separate lines instead of just one
  1. Tablet Mode increases tray icon spacing
  1. This patch - Increase tray icon spacing slightly on Non-Tablet Mode (half of the amount it increases by in Tablet Mode)

This patch is for the third patch of the three recommended patches to split the original patch into, and makes the click areas for the System Tray Plasmoid slightly larger in normal Plasma.

Test Plan
  1. Install this patch to the System Tray Plasmoid

BEFORE:

AFTER:

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23419
Build 23437: arc lint + arc unit
The-Feren-OS-Dev requested review of this revision.Feb 17 2020, 9:44 PM
The-Feren-OS-Dev created this revision.
The-Feren-OS-Dev edited the summary of this revision. (Show Details)Feb 17 2020, 9:46 PM
The-Feren-OS-Dev edited the test plan for this revision. (Show Details)
The-Feren-OS-Dev added a reviewer: VDG.
The-Feren-OS-Dev set the repository for this revision to R120 Plasma Workspace.
The-Feren-OS-Dev added a project: Plasma.
The-Feren-OS-Dev added a subscriber: plasma-devel.
The-Feren-OS-Dev edited the test plan for this revision. (Show Details)Feb 17 2020, 9:49 PM
The-Feren-OS-Dev edited the summary of this revision. (Show Details)Feb 17 2020, 9:54 PM
The-Feren-OS-Dev added a subscriber: ngraham.

Updated diff to be in line with parent patch

Hopefully fixed the diff again

Arcanist please

You gotta love arc/git sometimes...

niccolove accepted this revision as: niccolove.Feb 29 2020, 1:00 PM
This revision is now accepted and ready to land.Feb 29 2020, 1:00 PM
ngraham accepted this revision.Feb 29 2020, 2:28 PM

So at this point it's just one pixel, and I don't see the huge harm. :) However the last patch to adjust this (D27438) proved quite controversial, so we should wait for more opinions to avoid unduly ruffling feathers. :)

So at this point it's just one pixel, and I don't see the huge harm. :) However the last patch to adjust this (D27438) proved quite controversial, so we should wait for more opinions to avoid unduly ruffling feathers. :)

To be fair, part of the issue with the previous patch was that it didn't increase hitbox size while increasing spacing, while this one does, so hopefully there won't be too many issues with this.

IlyaBizyaev added a subscriber: IlyaBizyaev.EditedFeb 29 2020, 7:18 PM

Repeating my comment on the previous diff for this change: on my machine, the current spacing for systray elements already looks like your "after" screenshot. With normal usage, my systray area (icons + clock + a note widget) takes up 1/4 of the panel. Does it mean that after this change, it's going to be even larger?

Repeating my comment on the previous diff for this change: on my machine, the current spacing for systray elements already looks like your "after" screenshot. With normal usage, my systray area (icons + clock + a note widget) takes up 1/4 of the panel. Does it mean that after this change, it's going to be even larger?

This patch only affects the System Tray widget itself, not anything else on the panel. Only the spacing around the icons that are part of the System Tray applet itself will be changed by this patch.

This patch only affects the System Tray widget itself, not anything else on the panel. Only the spacing around the icons that are part of the System Tray applet itself will be changed by this patch.

I understand, but, as I mentioned, for me it already looks like what you expect to achieve with this.
Is it maybe DPI or font dependent, causing the spacing to be smaller in your local setup?

This patch only affects the System Tray widget itself, not anything else on the panel. Only the spacing around the icons that are part of the System Tray applet itself will be changed by this patch.

I understand, but, as I mentioned, for me it already looks like what you expect to achieve with this.
Is it maybe DPI or font dependent, causing the spacing to be smaller in your local setup?

I have 1.0 DPI scaling, though the system tray icon spacing is indeed controlled by DPI scaling due to the 'units.smallSpacing' size unit.

Went to check on different DPIs again to be sure it's fine and it definitely looks fine to me, scaling up the hitbox padding additions (and therefore the icon spacing) in sync with the DPI scaling very nicely, so that shouldn't be an issue.

It is (already) font-dependent, yes. A higher font size will result in larger spacing.

IMO this is not super sensible, but, there is it. :p

It is (already) font-dependent, yes. A higher font size will result in larger spacing.

IMO this is not super sensible, but, there is it. :p

Ok, so I guess for the group of users who currently have the right spacing it's going to become excessive then :/

I mean, it's only one more pixel. :) But yes, for some people with certain fonts maybe it will be too much. But then again, could a single pixel really be enough to make it too much?

It's hard to measure this in pixels, but on the screenshot, it's a 10% increase in size.
I use default font settings btw, with a global scaling factor of 1.25.

I'm not strongly against this, but I don't really see a point in this either. In, like, 4 years of Plasma on my machines, I haven't missed a systray item a single time; probably because they're quite big already, compared to other elements like checkboxes and dropdown list arrows.

The main point of this patch is the spacing increase between tray icons that this hitbox size increase gives, which makes the tray look less cramped as a result.

E.g.: dedoimedo complained about this in both the .18 and .17 review.

Well, if dedoimedo likes it this way, then it's probably the right thing to do ;)

Since no one else complains about this change, I'll assume I just was lucky to have proper scaling on my machine without "magic number" spacings.

gvgeo added a subscriber: gvgeo.EditedMar 2 2020, 5:06 AM

IMO this is the wrong way to do these changes.

  1. I don't see "itemSize" to be used anywhere else(didn't check) than "tasksRow" where we already add a smallSpacing. It would be best to increase the size in one place.
  2. For tablets already allow to use bigger size icons. There is no need to artificially increase the size this way, if they need bigger size can increase the panel height.
  3. Can expose plasmoid.configuration.iconSize to the user. Otherwise can be removed.

IMO this is the wrong way to do these changes.

  1. I don't see "itemSize" to be used anywhere else(didn't check) than "tasksRow" where we already add a smallSpacing. It would be best to increase the size in one place.
  2. For tablets already allow to use bigger size icons. There is no need to artificially increase the size this way, if they need bigger size can increase the panel height.
  3. Can expose plasmoid.configuration.iconSize to the user. Otherwise can be removed.

SystemTray is quite messy in a lot of places. How conveniently, there is a change with some refactoring and cleanup (D26992) waiting for a review :)

The-Feren-OS-Dev edited the summary of this revision. (Show Details)Mar 8 2020, 3:00 PM

Rebased on master

One more diff tweak to make it normal-er.

Redone, again.

This revision was automatically updated to reflect the committed changes.
broulik added a subscriber: broulik.EditedMar 9 2020, 7:56 AM

This causes layouting issues in narrow panels:


The icons are also too far apart

This causes layouting issues in narrow panels:


The icons are also too far apart

For small icons this does not look nice. BaseSize is increased by the same units.smallSpacing /2 regardless of the icon size, which is a great proportion for small icons.

gvgeo added a comment.Mar 9 2020, 11:37 AM

I believe that D26992, moves the icons down. And this patch, by adding an extra smallSpacing/2 makes it more noticeable.

Baking magic numbers is always questionable. Either the 1 smallspacing before, or the current 3/2, or 2 for tablet. IMO icons size should never be bigger than available space.

I believe that D26992, moves the icons down.

It looks like with that patch the icon sizes differ between plasmoids and SNIs

I believe that D26992, moves the icons down.

It looks like with that patch the icon sizes differ between plasmoids and SNIs

That was my work, definitely that should not be the outcome. :) I will check it.

The main problem here is that itemSize (regardless if it is plasmoid or SNI) is calculated in a very complicated and tricky way. If I remember correctly, firstly icon size is calculated, then margin is added but it is not a real margin - simply SNI icons are rounded to closes icon size and then by happy coincident we get margin. I guess it needs to be reworked... I can do that, just need few days for investigation.

I guess it needs to be reworked... I can do that, just need few days for investigation.

Thanks, much appreciated!

Fix for narrow panel in D27958