Fix tray icon size scaling when changing the panel size (fix bug 360333)
Needs ReviewPublic

Authored by pgkos on Feb 13 2018, 6:26 PM.

Details

Reviewers
davidedmundson
Group Reviewers
Plasma: Workspaces
Summary

BUG: 360333

This fixes the bug 360333 by making the tray icons scale exactly like the application icons on the left.

Test Plan

Before:

After:

Diff Detail

Repository
R120 Plasma Workspace
Branch
tray-icon-size-fix
Lint
No Linters Available
Unit
No Unit Test Coverage
pgkos created this revision.Feb 13 2018, 6:26 PM
Restricted Application edited projects, added Plasma; removed Plasma: Workspaces. · View Herald TranscriptFeb 13 2018, 6:26 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pgkos requested review of this revision.Feb 13 2018, 6:26 PM

Which application icons, and where are they set to 0.8?

Thanks for the patch!

  1. Please follow the formatting guidelines in https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch
  2. Please update the Test Plan section with evidence of testing. A pair of before-and-after screenshots is always very nice (and may get you featured in next weekend's Usability & Productivity blog post)
pgkos edited the test plan for this revision. (Show Details)Feb 13 2018, 7:26 PM
pgkos updated this revision to Diff 27095.Feb 13 2018, 7:37 PM

Fix tray icon scaling

pgkos edited the summary of this revision. (Show Details)Feb 13 2018, 7:38 PM
pgkos added a comment.Feb 13 2018, 7:41 PM

The 0.8 is used to create a small padding, so the icons never touch the panel's borders.

pgkos added a comment.Feb 13 2018, 7:43 PM

This patch also enables the user to configure the tray icon's size limit using System Settings -> Icons -> Advanced -> Panel

I fear this will generate the exact same types of user complaints that the huge clock text did before we gave it more vertical padding: people will say that the icons are too tall and don't have enough padding. Let's try not changing the appearance and vertical padding for default-height panels and just focus on making sure that they scale well as the panel is increased in size.

davidedmundson requested changes to this revision.Feb 13 2018, 7:47 PM

The 0.8 is used to create a small padding, so the icons never touch the panel's borders.

If the goal of the patch is to make the icon sizes the same as the icon in some other bit of the panel, then surely the size code needs to be identical to that other bit of code. I can't see the 0.8 anywhere else.

This revision now requires changes to proceed.Feb 13 2018, 7:47 PM
pgkos updated this revision to Diff 27103.Feb 13 2018, 9:01 PM

Make the padding of all the panel icons the same

pgkos updated this revision to Diff 27105.Feb 13 2018, 9:08 PM

Squashed the commits

pgkos added a comment.Feb 13 2018, 9:15 PM

I have tested it with the panel in both horizontal and vertical orientations, and 0.85 looks optimal for me.

davidedmundson requested changes to this revision.Feb 13 2018, 9:16 PM

I'm not merging a patch that just adds random numbers that you happen to think look good.

It's not unification if you can't justify where the numbers come from in relation to other bits of code.

This revision now requires changes to proceed.Feb 13 2018, 9:16 PM
pgkos added a comment.Feb 13 2018, 9:37 PM

I could argue that the numbers come from Material Design guidelines - it recommends between 10% to 20% of padding around icons.

But with your attitude it will be difficult to justify anything.

ngraham added a comment.EditedFeb 13 2018, 9:43 PM

I could argue that the numbers come from Material Design guidelines - it recommends between 10% to 20% of padding around icons.

But with your attitude it will be difficult to justify anything.

And with that attitude, you won't get a lot of patches accepted. :) Material Design is for Microsoft's platform, not ours. Let's not try to change multiple things in one patch. You may be able to get support for making the icons scale as the Panel is increased in height, but if you want to change the default padding, do that in another patch.

Yeah, the clock code isn't something I'd accept either.

But with your attitude it will be difficult to justify anything.

Plasma has a predefined margin size smallSpacing. If you're using something else, it probably isnt' going to end up consistent.

You'll also find iconItem which the Icon plasmoid uses visually will pad icons to the nearest icon size internally to the QQuickItem.
Some Plasmoid's compact representation is a striaght IconItem, some aren't, which is why systray forces this to make them more uniform.

When testing make sure you have the notification applet visible, and some SNIs.


All I'm asking for is, if the intention is to make them the same as something else, we need to make them actually programatically match in a way that works across any settings any user might have.

pgkos added a comment.Feb 13 2018, 9:59 PM

I have tried and tested this diff without any padding, and trust me, it looks very bad - I would rather have this patch not accepted - than not to add any padding.

Particularly, the tray icons look extremely bad when there is no padding - I suppose this is because the SVG icons are sized a little differently than pixmap icons.

For example, the KDE Connect icon almost doesn't fit in the panel when it has no padding.

I will try with units.smallSpacing.

Padding is fine. We're just saying that:

  • You need to retain the default appearance for a default sized panel
  • You need to use programmatic padding values rather than magic numbers
pgkos updated this revision to Diff 27114.Feb 13 2018, 10:25 PM

Use units.smallSpacing for padding instead of magic numbers.

pgkos updated this revision to Diff 27138.Feb 14 2018, 9:11 AM

Return to previous version, which was better.

pgkos updated this revision to Diff 27139.Feb 14 2018, 9:25 AM

Fix the previous commit

pgkos added a comment.Feb 14 2018, 9:28 AM

I have returned to the previous version, as the version which used units.smallSpacing looked awful.

Yes, it uses magic constant 0.85, but it really looks much better. I have patched the digital clock height too, so it perfectly matches the height of the tray icons.

I think that this constant should be placed inside the Units class. More, I also think this constant should be configurable by the user (for example, in the panel settings -> more settings...).

This code has the benefit that it does not change the appearance at the default panel height at all compared to the version without this patch. I have tested this on both small HD screens and 4K HiDPI screens (with screen scaling factor == 2.0), using KDE Neon git unstable live USB stick.

davidedmundson requested changes to this revision.Feb 14 2018, 11:09 AM

no.

This revision now requires changes to proceed.Feb 14 2018, 11:09 AM

Going up from the 0.71 magic number to the 0.85 magic number would make the clock bigger than it currently is, no? Can you supply a screenshot of how this works now at standard size, increased size, and witha high-DPI display?

Since magic numbers are bad, and we already have one for the clock, it does seem reasonable to replace it with a constant of some sort that we can re-use, for example with the notification icons as you're proposing. Making it user-configurable seems like unnecessary overkill to me though.

@davidedmundson, what do you think about that plan? We'd get rid of the existing magic number, which would be nice, right?

The part I care about is deciding what the problem is, and understanding the code around it.

The goal apparently was to match the application icon on the left, which means any patch needs to be able to describe the current padding that happens there, and the current padding that happens here.

Without that anything is guesswork.

This patch adds padding directly through resizing (but only in one of the two paths of the size if statement) and then there's a completely separate additional path with the marginHints in tasksRow going through to AbstractItem doing the same thing, but differently.

I'm fine with changing it, but not blindly.

pgkos added a comment.Feb 14 2018, 5:45 PM

There is something wrong and inconstent about SVG icon scaling. For example, if I put this line in systemtray ui main QML code instead:

property int itemSize: Math.min(width, height)

then the KDE Connect icon completely fills the height of the panel.

Which is obviously wrong, because if we open /usr/share/plasma/desktoptheme/default/icons/kdeconnect.svgz in an SVG editor we see that the icon is drawn on a 32x32px canvas ("page") and it has 2px top and bottom padding inside the canvas.

That padding disappears when the icon is drawn on the panel. That is wrong because if it was a 32x32px bitmap icon, the padding (transparent pixels) would be preserved.

That is the reason why we need to force padding (0.85*height) in the tray, but not in the application shortcut icons - the tray icons are SVG icons, but the application shorcut icons are bitmaps.

anthonyfieroni added inline comments.
applets/systemtray/package/contents/ui/main.qml
42

I see it has configuration for icon size ? What about to use it?

Thanks, that's much more the sort of thought process I'm looking for!

the tray icons are SVG icons, but the application shorcut icons are bitmaps.

All application icons are SVGs and they both ultimately come from IconItem?

I think we might find IconItem has internal margins between the item and the drawn shape rounding to the icon size stuff.
Gammaray is a useful tool here.

pgkos added a comment.Feb 14 2018, 6:29 PM

It seems the problem is not with SVG icons, but with the systemtray code. If I make the systemtray's Flow's marginHints zero, the padding is preserved correctly! No magic constant is needed anymore:

//Do spacing with margins, to correctly compute the number of lines
property QtObject marginHints: QtObject {
    property int left: Math.round(0)
    property int top: Math.round(0)
    property int right: Math.round(0)
    property int bottom: Math.round(0)
}

But I have no idea what this code was supposed to be doing.

pgkos updated this revision to Diff 27248.Feb 15 2018, 1:24 PM

Cancel out the enlargement of AbstractItem by the tasksRow's marginHints.

pgkos added a comment.Feb 15 2018, 1:42 PM

With this new version the tray icons scale exactly like the application icons on the left (the "jumps" between icon sizes happen at exactly the same time).

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

The goal of this patch is to make the size of the application icons on the left side of the panel match the size of the tray icons.
Additionally, the System Settings -> Icons -> Panel icon size adjustment will affect the tray icons too.

pgkos added a comment.Feb 15 2018, 2:10 PM

Things look bad when a bitmap icon appears in the tray... (e.g. Remmina) It seems roundToIconSize is necessary for bitmap icons.

I think I am going to abandon this patch, as the necessary changes are simply too big:

  • all references to marginHints in the systemtray should be removed, and spacing used instead
  • Units::roundToIconSize has to be changed so it works on real pixels and not on density-independent pixels - now it breaks down on HiDPI screens

I think no of these two changes are going to be accepted, unless someone knows why exactly these two things are implemented the current way.