Review Request 124648: Handle clients having a different device pixel ratio

[Martin Gräßlin]

> On April 7, 2016, 9:45 a.m., Martin Gräßlin wrote:
> > src/kwindowsystem.cpp, lines 495-496
> > <https://git.reviewboard.kde.org/r/124648/diff/1/?file=390797#file390797line495>
> >
> >     This part looks wrong to me and I think is the reason why reading icons fails in kwin_wayland now.
> >     
> >     We are dealing here with real pixels. E.g. kwin asking for the icon in size 16x16, because the icon is in 16x16 on the window property. By multiplying here it picks a different icon, e.g. 32x32, which might not even exist on the window.
> >     
> >     I think the scaling needs to be done by the user of the API.
> 
> David Edmundson wrote:
>     kwin doesn't have a DPR set, and shouldn't. I doubt that's the reason.
>     Also how are you checking what icons exist in the window? Arguably they should be scaled down.

> Also how are you checking what icons exist in the window?

KWin code just tries to get all available icon sizes which are normally installed on a window. That is currently:


    readIcon(16);
    readIcon(32);
    readIcon(48, false);
    readIcon(64, false);
    readIcon(128, false);

All those pixmaps are combined into one QIcon. So it just shouldn't need any scaled values. Also note that the method call takes an argument "bool scale" - I find it strange that I have an API where I say "don't scale the icon" and it scales it.

The problem with adjusting the value here is that this is a really low level method with different behavior. Depending on the flags it tries to get the icon from different sources. E.g. it first tries to get the icon installed as a property, then tries to resolve from the legacy installed bitmap, then from class hint. Due to the added scaling it can happen that a smaller icon gets returned. E.g. try Firefox, it installs a max icon of 48, but also has the legacy bitmap set. When you now request the icon in size 48, you cannot get it (there is never an icon with size 96) and falls back to the 16x16 legacy bitmap. Not what one wants.


> On April 7, 2016, 9:45 a.m., Martin Gräßlin wrote:
> > src/kwindowsystem.cpp, line 588
> > <https://git.reviewboard.kde.org/r/124648/diff/1/?file=390797#file390797line588>
> >
> >     This looks wrong to me! This method is supposed to return the actual work area as set on X11, not something multiplied by magic values.
> 
> David Edmundson wrote:
>     This one is right.
>     
>     The whole point of the dpr is that the client code sees *everything* scaled by the "magic value. 
>     QScreen's size of the monitor is scaled, the QWindow x/y are scaled. We need our library calls to be scaled too, otherwise they're just broken.

to me that looks wrong and confusing. Could you please add that to the documentation of the API that the devicePixelRatio is considered and that if one doesn't want that one should use the more low level NETWM API?


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124648/#review94357
-----------------------------------------------------------


On Aug. 7, 2015, 11:47 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124648/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2015, 11:47 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> Qt scales down the size of QScreens by the device pixel ratio; we should
> make our windowing APIs match.
> 
> X (and KWin) deal with device dependent pixels, so everything needs to
> be converted when communitcating geometry.
> 
> Abstraction happens in the main kwindowsystem so X and Wayland are both
> supported.
> 
> BUG: 350865
> BUG: 350614
> BUG: 347951
> 
> 
> Diffs
> -----
> 
>   src/kwindowsystem.cpp 0f8ec0ef470b3a3dcd353a1052dc80ed2bb3f992 
> 
> Diff: https://git.reviewboard.kde.org/r/124648/diff/
> 
> 
> Testing
> -------
> 
> Ran yakuake, it looks all right again.
> Re-enabled system DPR support in plasmashell (which is currently disabled) and tested notifications and panel struts are all sensible.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160407/e1157cb8/attachment-0001.html>