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

[Martin Gräßlin]

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



Sorry that I didn't notice this review earlier. But the approach here looks wrong to me. This is a low level API which is not depending on any QScreen assumptions. It's for interacting with X11. Things have a size there, I don't expect my values being magically multiplied when calling into this API.

Please rethink whether the approach is correct. I'm quite certain that the adjustments to icons is definately incorrect - at least from KWin usage perspective.


src/kwindowsystem.cpp (lines 495 - 496)
<https://git.reviewboard.kde.org/r/124648/#comment64158>

    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.



src/kwindowsystem.cpp (line 588)
<https://git.reviewboard.kde.org/r/124648/#comment64159>

    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.


- Martin Gräßlin


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/bff48390/attachment-0001.html>