Fix window frame rounding when scaling is used
Needs ReviewPublic

Authored by anemeth on May 10 2018, 11:58 AM.

Details

Reviewers
hpereiradacosta
davidedmundson
Group Reviewers
Breeze
VDG
Summary

The window corner frame rounding amount does not scale at all.
This makes the windows more rectangular then they should be when scaling is used.

Test Plan

No scaling, nice round corners:

2x scaled before, sharper corners:

2x scaled after, nice round corners:

Diff Detail

Repository
R31 Breeze
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 822
Build 835: arc lint + arc unit
anemeth created this revision.May 10 2018, 11:58 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 10 2018, 11:58 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
anemeth requested review of this revision.May 10 2018, 11:58 AM
anemeth edited the summary of this revision. (Show Details)May 10 2018, 12:02 PM
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: Breeze, VDG, hpereiradacosta.
anemeth edited the test plan for this revision. (Show Details)

Beautiful! +1.

mmm. Ok. It works. Now, why is it not handled upstream (by kwin or Qt), as it is in the widget style ?
(see how the QStyle rounding, for e.g. the inner frame of your list) is rounded properly when changing the scale factor.
And then, should it not be fixed there, that is, upstream, so that it applies to all decoration styles, rather than in every single decoration ?
I am ready to accept this patch, but am afraid it is a bit of a hack, and that it does not scale. (meaning: does not fix other decorations, such as oxygen, and will have to be copied if one implements other decoration components, such as e.g. rounded square buttons)
I'd prefer to have this handled upstream ...

Also: how does it work with multiple monitors ? (I see you get the info from ::primaryScreen, not for the screen the window is actually on ?

could you add @davidedmundson as a reviewer ? I think he knows a lot about how pixel scaling is/should be handled in kwin and plasma.
(definitly more than myself).

anemeth added a comment.EditedMay 10 2018, 1:32 PM

Also: how does it work with multiple monitors ? (I see you get the info from ::primaryScreen, not for the screen the window is actually on ?

As far as I know KWin X11 does not allow different scaling factors per monitor, so it should be fine.
And on Wayland QGuiApplication::primaryScreen()->logicalDotsPerInch() is always 96 so this only affects X11
@davidedmundson right?

zzag added a subscriber: zzag.EditedMay 10 2018, 2:17 PM

IIRC, decoration renderer sets device pixel ratio, so in theory, you should not multiply frame radius by dpr.

davidedmundson requested changes to this revision.May 10 2018, 3:56 PM
This revision now requires changes to proceed.May 10 2018, 3:56 PM

IIRC, decoration renderer sets device pixel ratio, so in theory, you should not multiply frame radius by dpr.

For some reason my comment went missing, but as zzag said, with high DPI here the only things we should ever change are pixmap sizes and QPixmap::setDevicePixelRatio not changing the (logical) size of what's actually being painted.

IIRC, decoration renderer sets device pixel ratio, so in theory, you should not multiply frame radius by dpr.

For some reason my comment went missing, but as zzag said, with high DPI here the only things we should ever change are pixmap sizes and QPixmap::setDevicePixelRatio not changing the (logical) size of what's actually being painted.

So do I interpret this right when saying that the issue (which is real, judging from the screenshots), is "elsewhere" ? meaning "upstream" ? Meaning also that fixing it here will break in the future, once the upstream issue is found and fixed ?

Most likely the actual issue is in kdecoration2 ...

zzag added a comment.EditedMay 14 2018, 2:21 PM

Most likely the actual issue is in kdecoration2 ...

I don't think so. Again, IIRC, decoration render does the following:

  • create a backing store
  • set device pixel ratio
  • create QPainter
  • pass the QPainter instance to decoration (->paint(&p, ...))

So, maybe, that's an issue in Qt or KWin.

zzag added a comment.EditedMay 14 2018, 2:45 PM

Also, what session it is? (X11 or Wayland)

I think you're facing one of X11 issues.

In D12804#262333, @zzag wrote:

Also, what session it is? (X11 or Wayland)

I think you're facing one of X11 issues.

X11
On Wayland it scales right

Here is Breeze close button modified to use QPainter::drawRoundedRect() and uses the same radius (3) as the titlebar:

This scales well, so I don't think it is a bug in Qt.

anemeth updated this revision to Diff 37718.Jul 13 2018, 10:45 PM

Rebase on master

anemeth added a comment.EditedJul 13 2018, 10:59 PM

As others said the dpi scaling should normally be dealt with QImage::setDevicePixelRatio()
As far as I can tell this is not working with Breeze for the following reason:

The decoration rendering starts here: https://github.com/KDE/kwin/blob/master/decorations/decorationrenderer.cpp#L66
The dpi is set here correctly with QImage::setDevicePixelRatio()
However this dpi value here is incorrect.
client()->client()->screenScale() is defined here: https://github.com/KDE/kwin/blob/master/toplevel.cpp#L316 and gets the value from here: https://github.com/KDE/kwin/blob/master/toplevel.cpp#L271
And in that function qreal newScale = screens()->scale(m_screen); gets the value from here: https://github.com/KDE/kwin/blob/master/screens.cpp#L114
You can see that this function just returns 1, and because of this the devicepixelratio of the decoration QImage will always be 1.

If I set Plasma scaling to 2 and change this value to 2 or the devicepixelratio here https://github.com/KDE/kwin/blob/master/decorations/decorationrenderer.cpp#L69 to 2 then the decoration renders very incorrectly. Not just simply double the size of everything, but invisible decoration and other artifacts.

We already have a workaround in Breeze for this in some parts. For example here https://github.com/KDE/breeze/blob/master/kdecoration/breezebutton.cpp#L166 when rendering the buttons for Breeze decoration we scale the QPainter manually.
This patch adds another workaround, but gets the job done, and fixing this whole thing would be a big effort.

And in that function qreal newScale = screens()->scale(m_screen); gets the value from here: https://github.com/KDE/kwin/blob/master/screens.cpp#L114
You can see that this function just returns 1

The method is virtual. It definitely is not always 1.

We already have a workaround in Breeze for this in some parts.

That code you link to is not related to scaling at all. Its normalising a viewport on a canvas to always be between 1 and 20.


However, there are some confusions so to back up a bit.

On X11 kwin does not use scaling at all. There is the old horrible thing where things are bigger if fonts are bigger.
On wayland we do proper scaling, which is why it works correctly

It's confusing when you say "when scaling is used" as you're actually referring to when we're not scaling - and doing the legacy thing.

I'm only the maintainer of when scaling is used. I don't like the other mode existing as it means we have two competing scaling systems at once which super complicates my stuff..but as a patch for that mode it does look kinda right.