[KWindowSystem] in icon() return realistic icon size
ClosedPublic

Authored by meven on Oct 16 2019, 5:40 AM.

Details

Summary

The icon function did not take into account the fact that icons have limited size options available.
Also take into account scale parameter.

Diff Detail

Repository
R130 Frameworks integration plugin using KWayland
Branch
arcpatch-D24683
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17741
Build 17759: arc lint + arc unit
meven created this revision.Oct 16 2019, 5:40 AM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 16 2019, 5:40 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Oct 16 2019, 5:40 AM
meven updated this revision to Diff 68012.Oct 16 2019, 5:41 AM

Clean up

meven added a subscriber: ngraham.Oct 16 2019, 5:44 AM

@ngraham In Wayland this allows the Tooltips from the taskmanager to display centered icon for wayland windows.

zzag added a subscriber: zzag.Oct 16 2019, 7:39 AM
zzag added inline comments.
src/windowsystem/windowsystem.cpp
188

You forgot to assign the copied pixmap to something. However, I think you could just return it from this point.

zzag added inline comments.Oct 16 2019, 7:47 AM
src/windowsystem/windowsystem.cpp
188

edit: scaled pixmap

meven updated this revision to Diff 68022.Oct 16 2019, 7:48 AM
meven marked an inline comment as done.

Actually use the scaled pixmap

meven marked an inline comment as done.Oct 16 2019, 7:49 AM
mart added a subscriber: mart.Oct 16 2019, 8:34 AM
mart added inline comments.
src/windowsystem/windowsystem.cpp
173

what's the logic behind the numbers 40, 56, 96 ans 192?
should at least be in a comment

meven added inline comments.Oct 16 2019, 8:51 AM
src/windowsystem/windowsystem.cpp
173

It was copied from kwindowsystem/src/platforms/xcb/kwindowsytem.cpp
So I don't know precisely but at least it is coherent with X.
Unfortunately the commit history does not tell much.

meven added inline comments.Oct 16 2019, 10:56 AM
src/windowsystem/windowsystem.cpp
173

I can't explain the current logic.

@mart Do you think I should change the logic to give it more sense ?

Perhaps make the 24, 40.. values match the iconWidth i.e :

if (width <= 16){
iconWidth = 16;
else if (width <= 32)
iconWidth = 32
else if (width <= 48){
iconWidth = 48
...

ping @zzag @mart Any feedback

src/windowsystem/windowsystem.cpp
173

@anthonyfieroni Hey Anthony
Do you remember where the if comparison width values come from in your commit https://cgit.kde.org/kwindowsystem.git/commit/?id=e8762b96ae7a9fabc6af8fc5dcf2b82a7206053f ?

src/windowsystem/windowsystem.cpp
173

That's adjustment, 16, 32, 48, 64, 128, 256 are standard icon sizes, when you got a value between 2 sizes you adjust it by lower bound. Say (256 - 128) / 2 = 64, so 128 + 64 = 192, when request is smaller than 192 and bigger than 96 (which is same calculated for previous values) it returns 128. If you want write an algorithm for that.

meven marked 5 inline comments as done.Oct 17 2019, 3:14 PM
meven added inline comments.
src/windowsystem/windowsystem.cpp
173

Thank you @anthonyfieroni for the explanation
So in fact the comment stated already this simply:

take the nearest value for best results

I don't think more comment is necessary.

meven marked 2 inline comments as done.Oct 17 2019, 3:56 PM

Anyone to care to review and accept this small code change ?

zzag accepted this revision.Oct 17 2019, 4:23 PM
This revision is now accepted and ready to land.Oct 17 2019, 4:23 PM
This revision was automatically updated to reflect the committed changes.