Bug 455513

Summary: Small icons no longer respect the "Small Icon" size configurable in the Icons KCM
Product: [Plasma] Breeze Reporter: gianluca.pettinello <g_pet>
Component: QStyleAssignee: Plasma Bugs List <plasma-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: kde, kfm-devel, nate, noahadvs, sh200105, uhhadd
Priority: NOR Keywords: regression
Version: 5.25.3   
Target Milestone: ---   
Platform: Archlinux   
OS: Linux   
Latest Commit: Version Fixed In: 5.25.5
Attachments: Dolphin with screen resized 150%

Description gianluca.pettinello@gmail.com 2022-06-17 20:49:51 UTC
SUMMARY
***
NOTE: If you are reporting a crash, please try to attach a backtrace with debug symbols.
See https://community.kde.org/Guidelines_and_HOWTOs/Debugging/How_to_create_useful_crash_reports
***


STEPS TO REPRODUCE
1. Open dolphin with tree visible
2. Set font dpi 144 and video size 100% on a macbook pro 15" 2880x1800



OBSERVED RESULT
Icons are tiny even when configuring icon size more than 32 px

EXPECTED RESULT
Before plasma 5.25 the icons where 32 px and not 16 px in dolphin tree and in menu drop-down 

SOFTWARE/OS VERSIONS
Windows: 
macOS: 
Linux/KDE Plasma: Archlinux
(available in About System)
KDE Plasma Version: 5.25
KDE Frameworks Version: 5.95
Qt Version: 

ADDITIONAL INFORMATION
Comment 1 Nate Graham 2022-06-21 16:58:44 UTC
> 2. Set font dpi 144 and video size 100% on a macbook pro 15" 2880x1800

This is not the correct way to scale the system. When you use the font dpi setting to *approximate* scaling, it scales everything in your apps except icons. That's why you should use the scaling slider in System Settings > Display and Monitor instead.
Comment 2 gianluca.pettinello@gmail.com 2022-06-22 11:49:04 UTC
Hello Nate,
agreed but before 5.25 I could scale like this. The proble is that small icons are not sensitive to changing its size in system settings. I think it is a bug.
Gianluca
Comment 3 Nate Graham 2022-06-22 15:15:01 UTC
Changing what size where?
Comment 4 gianluca.pettinello@gmail.com 2022-06-22 20:10:02 UTC
System settings, appearance, icons, configure icons size. I normally set small icons to 32.
That's enough up to plasma 5.24.5.
Comment 5 Nate Graham 2022-06-22 22:35:01 UTC
Yeah approximating scaling with that setting plus the font DPI is, again, not an officially supported thing. :) It used to work by pure chance, but it's fragile and barely works and I would recommend using the official scaling system instead.
Comment 6 gianluca.pettinello@gmail.com 2022-06-24 19:33:41 UTC
Created attachment 150130 [details]
Dolphin with screen resized 150%

I tried to scale with video scaling to 150% and the result is ugly. I mean the font is OK but the widgets are all too big, buttons and scrollbars. And the tree like the menu icons are not resizing with configure icon size while all the other icons are sensitive to icon size change.
Definitely it is a bug
Comment 7 gianluca.pettinello@gmail.com 2022-06-24 19:37:20 UTC
Hello Nate,

I tried to scale the screen with video scaling as you suggested. You see Dolphin in the screenshot.
The tree and menu drop down icons are still insensitive to icon size change. The "small icons" are insensitive to any change of icon size through System Settings -> Appearance -> Icons -> Configure Icons Size.
So definitely it is a bug.
Comment 8 Nate Graham 2022-06-27 16:46:14 UTC
> I tried to scale with video scaling to 150% and the result is ugly. I mean the font is OK but the
> widgets are all too big, buttons and scrollbars. 
This is the way it's supposed to be; you just got used to the buggy state. :) When you set 150% scale, *everything* is supposed to become 150% bigger, not just some things.

> The "small icons" are insensitive to any change of icon size through System Settings ->
> Appearance -> Icons -> Configure Icons Size.
This is a different issue from what you originally reported, and also a different issue from how it looks with 150% scale. In general this is a perfect illustration of why it's so damaging to have so many different ways to scale UI elements on the screen: people no one setting works perfectly, and combining them results in weird non-deterministic outcomes because their scopes are unclear and they all interact with one another strangely, and some (like the icon size settings in the Icons KCM) don't even work at all most of the time).
Comment 9 gianluca.pettinello@gmail.com 2022-07-23 13:42:00 UTC
Hello Nate,

I found the root cause:


it is in kstyle/breezestyle.cpp

***
//______________________________________________________________
int Style::pixelMetric(PixelMetric metric, const QStyleOption *option, const QWidget *widget) const
{
    // handle special cases
    switch (metric) {
    case PM_MenuHMargin:
    case PM_MenuVMargin:
        return Metrics::MenuItem_HighlightGap;

    // small icon size
    case PM_SmallIconSize:
        if (isTabletMode()) {
            return 22;
        } else {
            return 16; //<-- here is the root cause
        }
***

This override causes small icons to be insensitive to a change in size in system settings.
I'm not a programmer but I think that forcing these numbers is not elegant.
I suggest to first read the size and then increase by one level for tablet mode instead of fixing absolute values
What  do you think?
Regards
Gianluca
Comment 10 Nate Graham 2022-07-24 21:05:59 UTC
Right, that makes sense. The code assumes that a small icon is always 16, probably because the author didn't know or forgot that the small icon size is configurable in the Icons KCM. I really dislike this configurability and I think it leads to people using it as a sort of ad-hoc scaling rather than using the officially supported methods, which also reduces the pressure to fix the official method for the cases where it doesn't work properly. In my ideal world, we'd label that functionality obsolete and delete it entirely.

However in the absence of such a thing happening, we should fix this.

Probably what we should do is change the code to consult the Small Icon size from the Icons KCM, and when when in Tablet mode, go up one size.

If instead we don't want to fix this, it's time to remove the Small Icons configurability in the Icons KCM.
Comment 11 gianluca.pettinello@gmail.com 2022-07-25 06:49:48 UTC
I agree with you that the way I (and probably others) scale the screen is weird. The reasons are:
- widgets size is too big with uniform scaling. I saw there is some discussion to have an adjustable parameter to reduce padding/thickness
- I prefer colored folder icons, which are only from 32 px size
Both reasons are "silly" vs the principle of having only one way of scaling the screen, which I fully support. 
Maybe by having a widget adjustable size parameter the weird scaling way can be removed.
Comment 12 Alexander Kernozhitsky 2022-07-30 13:06:52 UTC
> The code assumes that a small icon is always 16, probably because the author didn't know or forgot that the small icon size is configurable in the Icons KCM.
The code is also Breeze-specific, meaning that if I switch the application theme to something else, then it will use the icon size configured in the KCM (which resides in SmallIcons section in ~/.config/kdeglobals).

Also, magic hard-coded values in the source code are usually bad.

I don't really know how Tablet mode is made, but I assume that this fix should go to System settings or whatever sets the icon sizes. If the Tablet mode is Breeze-specific itself, then making the icons one step bigger is also a good solution.
Comment 13 Nate Graham 2022-08-02 18:55:00 UTC
I guess to fix this in Breeze, we would need to:
1. look at the small size value set in system settings
2. map it to a KIconLoader icon size constant (e.g. small, smallMedium, etc)
3. figure out the next largest KIconLoader icon size from that
4. when in Tablet Mode, use that larger value

Or we could delete this setting and tell people to use the supported scaling tools to scale things on their screens.
Comment 14 Bug Janitor Service 2022-08-04 22:12:14 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/breeze/-/merge_requests/234
Comment 15 Alexander Kernozhitsky 2022-08-04 22:17:56 UTC
> I guess to fix this in Breeze, we would need to:
> 1. look at the small size value set in system settings
> 2. map it to a KIconLoader icon size constant (e.g. small, smallMedium, etc)
> 3. figure out the next largest KIconLoader icon size from that
> 4. when in Tablet Mode, use that larger value

I created a merge request that does exactly what you described :)
Comment 16 Nate Graham 2022-08-05 13:00:13 UTC
Git commit 7c31ab635758401bd0e155f82538093577132d32 by Nate Graham, on behalf of Alexander Kernozhitsky.
Committed on 05/08/2022 at 13:00.
Pushed by ngraham into branch 'master'.

Consider small icon size from system

Before that, the values were hardcoded (i.e. 16 for non-tablet mode
and 22 for tablet mode). Now, the value for non-tablet mode is taken
from the system, and the value for tablet mode is one step bigger.
FIXED-IN: 5.25.5

M  +18   -5    kstyle/breezestyle.cpp

https://invent.kde.org/plasma/breeze/commit/7c31ab635758401bd0e155f82538093577132d32