Bug 454131

Summary: Search icon in search fields around Plasmashell is too large when NOT using Plasma scaling
Product: [Frameworks and Libraries] libplasma Reporter: Jin Liu <ad.liu.jin>
Component: componentsAssignee: Plasma Bugs List <plasma-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: carl, kde, me, nate, noahadvs, notmart, qydwhotmail
Priority: HI    
Version: 5.94.0   
Target Milestone: ---   
Platform: Other   
OS: Linux   
See Also: https://bugs.kde.org/show_bug.cgi?id=356446
Latest Commit: Version Fixed In: 5.96
Sentry Crash Report:
Attachments: search box in plasmashell vs. systemsettings

Description Jin Liu 2022-05-21 07:54:05 UTC
Created attachment 149056 [details]
search box in plasmashell vs. systemsettings

SUMMARY
The search icon in search box is too large, on HiDPI screen (240dpi, 3200x2000, X11).
This only happens in plasmashell, e.g. Kickoff, Krunner, Klipper, NetworkManager. Search boxes in systemsettings and Discover are OK.

SOFTWARE/OS VERSIONS
Linux/KDE Plasma: archlinux
(available in About System)
KDE Plasma Version: 5.24.90
KDE Frameworks Version: 5.94.0
Qt Version: 5.15.4
Comment 1 Nate Graham 2022-05-23 17:44:17 UTC
Did you scale the system by changing the Font DPI instead of using the scaling slider in System Settings > Display and Monitor?

If so that's not supported, because it causes issues like this. I would recommend using the scaling slider instead, and also use the Wayland session instead of the X11 session

If you have to use the X11 session, use the scaling slider and set the `PLASMA_USE_QT_SCALING=1` environment variable in /etc/environment which will make HiDPI scaling in Plasma work better as long as you never need to connect a second screen with a different DPI. If you do, use the Wayland session.
Comment 2 Jin Liu 2022-05-24 07:42:50 UTC
(In reply to Nate Graham from comment #1)
> Did you scale the system by changing the Font DPI instead of using the
> scaling slider in System Settings > Display and Monitor?

No, I used the global scaling slider. But whenever I set the global scaling to 250%, it automatically enables the force font DPI and set it to 240dpi.

I add a new user and reboot into it. (also deleted /etc/sddm.conf.d/kde_settings.conf, because it has a "ServerArguments=-dpi 240" which I'm not sure if matters) The default state is "force font DPI" disabled and the number is 96, and global scaling at 100%. Both Plasmashell and apps have tiny icons and normal-sized texts. But the too-large-search-icon problem is the same.
Then I set the global scaling to 250%, still the same problem.

> If so that's not supported, because it causes issues like this. I would
> recommend using the scaling slider instead, and also use the Wayland session
> instead of the X11 session

I tried the Wayland session. Yes it works fine.

> If you have to use the X11 session, use the scaling slider and set the
> `PLASMA_USE_QT_SCALING=1` environment variable in /etc/environment which
> will make HiDPI scaling in Plasma work better as long as you never need to
> connect a second screen with a different DPI. If you do, use the Wayland
> session.

Also tried X11 session with PLASMA_USE_QT_SCALING=1, yes it works fine too. Thanks.
Comment 3 Nate Graham 2022-05-24 15:11:47 UTC
OK, seems like an issue with the icon in the Plasma search field component that is caused by using Plasma scaling, and solved by using Qt scaling. But Plasma scaling is still supported and used by default on X11, so we should fix it.

The icon has this set for its size policy:

implicitHeight: PlasmaCore.Units.iconSizes.sizeForLabels
implicitWidth: PlasmaCore.Units.iconSizes.sizeForLabels

So I wonder if sizeForLabels is generally broken.
Comment 4 Nate Graham 2022-06-03 21:53:56 UTC
Probably something here is incorrect:


int Units::roundToIconSize(int size)
{
    // If not using Qt scaling (e.g. on X11 by default), manually scale all sizes
    // according to the DPR because Qt hasn't done it for us. Otherwise all
    // returned sizes are too small and icons are only the right size by pure
    // chance for the larger sizes due to their large differences in size
    qreal multiplier = 1.0;
    QScreen *primary = QGuiApplication::primaryScreen();
    if (primary) {
        // Note that when using Qt scaling, the multiplier will always be 1
        // because Qt will scale everything for us. This is good and intentional.
        multiplier = bestIconScaleForDevicePixelRatio((qreal)primary->logicalDotsPerInchX() / (qreal)96);
Comment 5 Jin Liu 2022-06-04 07:12:58 UTC
(In reply to Nate Graham from comment #4)
> Probably something here is incorrect:
> 
> 
> int Units::roundToIconSize(int size)
> {
>     // If not using Qt scaling (e.g. on X11 by default), manually scale all
> sizes
>     // according to the DPR because Qt hasn't done it for us. Otherwise all
>     // returned sizes are too small and icons are only the right size by pure
>     // chance for the larger sizes due to their large differences in size
>     qreal multiplier = 1.0;
>     QScreen *primary = QGuiApplication::primaryScreen();
>     if (primary) {
>         // Note that when using Qt scaling, the multiplier will always be 1
>         // because Qt will scale everything for us. This is good and
> intentional.
>         multiplier =
> bestIconScaleForDevicePixelRatio((qreal)primary->logicalDotsPerInchX() /
> (qreal)96);

Indeed.

This roundToIconSize function is used here:
https://github.com/KDE/plasma-framework/blob/b5f8095d96320bd14f724d6bb1da17e3d1dc8bbf/src/declarativeimports/core/units.cpp#L107
> void Units::iconLoaderSettingsChanged()
> {
>     m_iconSizes->insert(QStringLiteral("desktop"), devicePixelIconSize(KIconLoader::global()->currentSize(KIconLoader::Desktop)));
> 
>     m_iconSizes->insert(QStringLiteral("tiny"), devicePixelIconSize(KIconLoader::SizeSmall) / 2);
>     m_iconSizes->insert(QStringLiteral("small"), devicePixelIconSize(KIconLoader::SizeSmall));
>     m_iconSizes->insert(QStringLiteral("smallMedium"), devicePixelIconSize(KIconLoader::SizeSmallMedium));
>     m_iconSizes->insert(QStringLiteral("medium"), devicePixelIconSize(KIconLoader::SizeMedium));
>     m_iconSizes->insert(QStringLiteral("large"), devicePixelIconSize(KIconLoader::SizeLarge));
>     m_iconSizes->insert(QStringLiteral("huge"), devicePixelIconSize(KIconLoader::SizeHuge));
>     m_iconSizes->insert(QStringLiteral("enormous"), devicePixelIconSize(KIconLoader::SizeEnormous));
>     // gridUnit is always the font height here
>     m_iconSizes->insert(QStringLiteral("sizeForLabels"), devicePixelIconSize(roundToIconSize(QFontMetrics(QGuiApplication::font()).height())));
> 
>     m_iconSizeHints->insert(QStringLiteral("panel"), devicePixelIconSize(KIconLoader::global()->currentSize(KIconLoader::Panel)));
>     m_iconSizeHints->insert(QStringLiteral("desktop"), devicePixelIconSize(KIconLoader::global()->currentSize(KIconLoader::Desktop)));

Note all these sizes are passed to devicePixelIconSize(), which is:
https://github.com/KDE/plasma-framework/blob/b5f8095d96320bd14f724d6bb1da17e3d1dc8bbf/src/declarativeimports/core/units.cpp#L194
> int Units::devicePixelIconSize(const int size) const
> {
>     /* in kiconloader.h
>     enum StdSizes {
>         SizeSmall=16,
>         SizeSmallMedium=22,
>         SizeMedium=32,
>         SizeLarge=48,
>         SizeHuge=64,
>         SizeEnormous=128
>     };
>     */
>     // Scale the icon sizes up using the devicePixelRatio
>     // This function returns the next stepping icon size
>     // and multiplies the global settings with the dpi ratio.
>     const qreal ratio = devicePixelRatio();
> 
>     return size * bestIconScaleForDevicePixelRatio(ratio);
> }

Note this function already multiplies its parameter by the zoom ratio. So roundToIconSize(QFontMetrics(QGuiApplication::font()).height()) shouldn't use the zoom ratio, otherwise the ratio is multiplied by twice.

However, roundToIconSize is called in many other places, e.g.
https://github.com/KDE/plasma-desktop/blob/48a0fc3719de3a87c394af905b16f8cedb9b6c08/applets/window-list/contents/ui/MenuButton.qml#L59
So I've no idea whether
A. Units::roundToIconSize should not multiply by zoom ratio, which affects sizes all around Plasma.
B. The line devicePixelIconSize(roundToIconSize(QFontMetrics(QGuiApplication::font()).height())) should be changed to roundToIconSize(QFontMetrics(QGuiApplication::font()).height()), which should only affect sizeForLabels.
Comment 6 Jin Liu 2022-06-04 07:22:37 UTC
BTW, seems this bug should happen on any HiDPI display, not limited to 2.5x. This makes me wonder how many developers are using HiDPI + X11/Plasma Scaling daily...
Comment 7 Fushan Wen 2022-06-22 06:52:20 UTC
(In reply to Jin Liu from comment #6)
> BTW, seems this bug should happen on any HiDPI display, not limited to 2.5x.
> This makes me wonder how many developers are using HiDPI + X11/Plasma
> Scaling daily...

Perhaps not many as X11 HiDPI users tend to enable Qt scaling.
Comment 8 Nate Graham 2022-06-23 18:41:59 UTC
It seems that using Plasma scaling actually fixes this. With Plasma scaling turned on on X11, I don't see the issue in Plasma or KRunner anymore (which respect the environment variable), but I do still see it in the search fields for KWin's Overview and Present Windows effects, because KWin doesn't respect that environment variable and always uses Plasma scaling on X11.

Keeping the HI priority since X11 with Plasma scaling are the default settings for now.
Comment 10 Nate Graham 2022-06-23 20:17:42 UTC
Git commit 26ae86dff198fbce05ba3824b89786224b3792a9 by Nate Graham.
Committed on 23/06/2022 at 18:58.
Pushed by ngraham into branch 'master'.

Units: Fix sizeForLabels double-scaling icons with Plasma scaling

The sizeForLabels unit internally uses roundToIconSize(), giving it a
value and feeding that into devicePixelIconSize() to take into account
required icons size differences when using Plasma scaling. But
roundToIconSize() internally does that automatically! So as a result,
the size is double-scaled and looks too big. To fix this, we simply have
to stop using devicePixelIconSize() for this unit.
FIXED-IN: 5.96

M  +3    -2    src/declarativeimports/core/units.cpp

https://invent.kde.org/frameworks/plasma-framework/commit/26ae86dff198fbce05ba3824b89786224b3792a9