BUG: 360333
This fixes the bug 360333 by making the tray icons scale exactly like the application icons on the left.
davidedmundson |
Plasma: Workspaces |
BUG: 360333
This fixes the bug 360333 by making the tray icons scale exactly like the application icons on the left.
Before:
After:
No Linters Available |
No Unit Test Coverage |
Thanks for the patch!
The 0.8 is used to create a small padding, so the icons never touch the panel's borders.
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.
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.
I have tested it with the panel in both horizontal and vertical orientations, and 0.85 looks optimal for me.
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.
So maybe you tell me where this comes from? https://cgit.kde.org/plasma-workspace.git/commit/applets/digital-clock/package/contents/ui/DigitalClock.qml?id=90ea27f49c84f0dffbbf79b29eaa14c57f31a24d
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.
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:
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.
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.
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.
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.
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.
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. |
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:
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.