D7849: Fix the tray icon scaling on HiDPI screens
ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.
Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks.
This does not actually compile for me against current git master:
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp: In
static member function ‘static int Units::roundToIconSize(int)’:
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:160:65:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
} else if (size < devicePixelIconSize(KIconLoader::SizeSmall)) {
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:161:58:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
return devicePixelIconSize(KIconLoader::SizeSmall) / 2;
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:162:71:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
} else if (size < devicePixelIconSize(KIconLoader::SizeSmallMedium)) {
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:163:58:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
return devicePixelIconSize(KIconLoader::SizeSmall);
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:164:66:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
} else if (size < devicePixelIconSize(KIconLoader::SizeMedium)) {
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:165:64:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
return devicePixelIconSize(KIconLoader::SizeSmallMedium);
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:166:65:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
} else if (size < devicePixelIconSize(KIconLoader::SizeLarge)) {
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:167:59:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
return devicePixelIconSize(KIconLoader::SizeMedium);
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:168:64:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
} else if (size < devicePixelIconSize(KIconLoader::SizeHuge)) {
^
/home/dev/repos/plasma-framework/src/declarativeimports/core/units.cpp:169:58:
error: cannot call member function ‘int Units::devicePixelIconSize(int) const’
without object
return devicePixelIconSize(KIconLoader::SizeLarge);
^
src/plasmaquick/CMakeFiles/KF5PlasmaQuick.dir/build.make:374: recipe for
target
'src/plasmaquick/CMakeFiles/KF5PlasmaQuick.dir/__/declarativeimports/core/units.cpp.o'
failed
make[2]: ***
[src/plasmaquick/CMakeFiles/KF5PlasmaQuick.dir/__/declarativeimports/core/units.cpp.o]
Error 1
CMakeFiles/Makefile2:1782: recipe for target
'src/plasmaquick/CMakeFiles/KF5PlasmaQuick.dir/all' failed
make[1]: *** [src/plasmaquick/CMakeFiles/KF5PlasmaQuick.dir/all] Error 2
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D7849
To: pgkos, #plasma, ngraham
Cc: kde-frameworks-devel, aspotashev, ngraham, anthonyfieroni, broulik,
davidedmundson, plasma-devel, ragreen, Pitel, michaelh, ZrenBot, bruns,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, #frameworks
D7849: Fix the tray icon scaling on HiDPI screens
ngraham added a comment. Does this actually fully fix https://bugs.kde.org/show_bug.cgi?id=360333? REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: aspotashev, ngraham, anthonyfieroni, broulik, #frameworks, davidedmundson, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
aspotashev added a comment. related bug report: https://bugs.kde.org/show_bug.cgi?id=360333 REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: aspotashev, ngraham, anthonyfieroni, broulik, #frameworks, davidedmundson, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
pgkos added a comment. In my opinion, roundToIconSize should operate on scaled units - it is used multiple times in a few plasmoids - all calls from them to roundToIconSize assume it will operate on scaled units. The method, as it is now, is useless, because QML code has no access to KIconLoader sizes and devicePixelIconSize() method, so it cannot operate on unscaled units or convert them on the fly. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: ngraham, anthonyfieroni, broulik, #frameworks, davidedmundson, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
pgkos updated this revision to Diff 26878. pgkos added a comment. This is a simpler implementation - the diff changes roundToIconSize so it uses scaled units. REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7849?vs=19612&id=26878 REVISION DETAIL https://phabricator.kde.org/D7849 AFFECTED FILES src/declarativeimports/core/units.cpp To: pgkos, #plasma Cc: ngraham, anthonyfieroni, broulik, #frameworks, davidedmundson, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
ngraham added a comment. Any chance we can reach some kind of consensus here? REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: ngraham, anthonyfieroni, broulik, #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
pgkos added a comment. @anthonyfieroni my first version of the diff changed only the tray icon QML file, but I think that it is better to fix it in the plasma framework, as that function is used in multiple other places and there is the same problem with wrongly sized icons on hidpi screens. REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: anthonyfieroni, broulik, #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
anthonyfieroni added a comment. So, Kai is right, it roundToIconSize only round size independent from dpi. Then when it used, it should be multiplyed by dpi. REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: anthonyfieroni, broulik, #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
pgkos added a comment. @broulik consider this case: The tray icons' size is defined by default in `org.kde.plasma.private.systemtray/contents/config/main.xml` to `smallMedium`, which means 22 pixels on a 96-dpi screen. On a 192-dpi screen, the icons' real size is 44 pixels. Now, in `org.kde.plasma.private.systemtray/contents/ui/main.qml` there is this line: units.roundToIconSize(Math.min(Math.min(width, height), units.iconSizes[iconSizes[plasmoid.configuration.iconSize]])) The `height` property is the height of the panel (assuming the panel is horizontal). On my hidpi screen the default panel height is around 72 pixels (`32 px * 2`). So the above line evaluates to: `units.roundToIconSize(Math.min(72, units.iconSizes.smallMedium))` `units.roundToIconSize(Math.min(72, 44))` `units.roundToIconSize(44)` and that gets rounded to `medium` size, which is incorrect, because we want a `smallMedium` size multiplied by two (`32 px != 22 px * 2`). So, on a hidpi screen the `roundToIconSize` function returns a wrong size, and additionally it (wrongly) does not multiply the size by the icon dpi scaling factor (e.g. `2.0`). REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: broulik, #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
broulik added a comment. I don't understand this. RoundToIconSize is supposed to round down and that's it. If I pass it 100 px because I'm on a high dpi screen, it will still return 100. Only if I pass it e.g. 34 it will change it to 32. REVISION DETAIL https://phabricator.kde.org/D7849 To: pgkos, #plasma Cc: broulik, #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
pgkos updated this revision to Diff 19612. pgkos added a comment. Fixed the previous diff. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7849?vs=19592&id=19612 REVISION DETAIL https://phabricator.kde.org/D7849 AFFECTED FILES src/declarativeimports/core/units.cpp src/declarativeimports/core/units.h To: pgkos, #plasma Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7849: Fix the tray icon scaling on HiDPI screens
pgkos updated this revision to Diff 19592. pgkos changed the repository for this revision from R120 Plasma Workspace to R242 Plasma Framework (Library). pgkos added a comment. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. Edited the function `Units::roundToIconSize` so it both accepts and outputs a dpi-scaled pixel value. This might break some things, but on the other hand there are places (other than the system tray) which also have exactly the same problem with icon scaling on hidpi screens (see e.g. https://cgit.kde.org/plasma-desktop.git/tree/applets/taskmanager/package/contents/ui/Task.qml), this patch will also fix all these other cases. REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7849?vs=19585&id=19592 REVISION DETAIL https://phabricator.kde.org/D7849 AFFECTED FILES src/declarativeimports/core/units.cpp To: pgkos, #plasma Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
