[WIP] Per-screen scale factors on X11 using QT_SCREEN_SCALE_FACTORS
Needs ReviewPublic

Authored by fvogt on Apr 21 2018, 1:51 PM.

Details

Reviewers
None
Group Reviewers
Plasma
Summary

Qt computes the scale factor relative to the primary screen. This means
we can support per-screen DPI on X11 by setting QT_SCREEN_SCALE_FACTORS
appropriately, while keeping Xft.dpi synced to the primary screen's DPI.
This means applications which don't read QT_SCREEN_SCALE_FACTORS use the
scale of the primary screen. This is a good compromise, especially for
those who either need fractional scaling or can't use wayland for various
reasons.

WIP because this doesn't track changes of the primary screen and I'm not
sure how to implement this the best way. The issue is that the
QT_SCREEN_SCALE_FACTORS management is not done by the backend, but only by
the KCM. Ideas welcome.

Test Plan

VM with multihead QXL. I can now assign a per-screen scale in the KCM
and after a relogin applications scale correctly based on the screen they're on.

Diff Detail

Repository
R104 KScreen
Branch
perscreenxcb
Lint
No Linters Available
Unit
No Unit Test Coverage
fvogt created this revision.Apr 21 2018, 1:51 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 21 2018, 1:51 PM
fvogt requested review of this revision.Apr 21 2018, 1:51 PM

I can't imagine this working. It didn't when I've tried the same thing.
Docs say QT_SCREEN_SCALE_FACTORS doesn't affect the logical DPI. which means you have a 1x scale on one screen 2x on another..they both have the same font size which will just be really wrong.

Also I bet spectacle's area selection is bust on at least one of the monitor variants. There's a whole lot of confusion (with no real right answer) as to what the size of a 1x monitor is in logical co-ordinates when your window is on the 2x screen and vice versa.

fvogt added a comment.EditedApr 21 2018, 2:33 PM

Docs say QT_SCREEN_SCALE_FACTORS doesn't affect the logical DPI. which means you have a 1x scale on one screen 2x on another..they both have the same font size which will just be really wrong.

Exactly that is not the case.

Let me give you an example for broken font size: Screen "DP-0" (primary) has Scale 2 and "DP-1" has scale 1. You set "Xft.dpi: 96". The result is 96 dpi font on DP-0 and 48 dpi font on DP-1.
This can only happen if Qt scales the font DPI depending on the QT_SCREEN_SCALE_FACTORS values.

Screenshot of a working setup:

I also cannot imagine this to work due to the fact how X11 works. There is just no mapping from window to screen. No window can know on which screen it is. Not even KWin knows that as the window manager (screen is not a constant property, but evaluated every time it is accessed, it's based on the distance to closest screen). Especially for overlapping windows it's very difficult to try to get to which screen it belongs. It gets even more complicated when things like panning and overlapping screens get into it. So I wouldn't trust this thing in Qt to work due to the pain we have in KWin especially with these problems.

fvogt added a comment.Apr 21 2018, 7:45 PM

So I wouldn't trust this thing in Qt to work due to the pain we have in KWin especially with these problems.

Yes, it might not be perfect. It works well enough that not exposing this in the settings is a wasted opportunity though.
I used this extensively for half a year with a 4k non-primary monitor, where the only other option is to meet halfway at 1.5x scale or move the screen so close that it's touching your nose.

I doubt that users would expect this to work perfectly with any unusual configurations such as overlapping screens, but even then it's an improvement to the current situation.

+1 for not letting the perfect be the enemy of the good here. Multi-monitor support is a pain point for many of our users.

I'm pointing out that I'm in general against any risky changes on X11. If users want to use this features: Wayland is there. KWin is feature frozen on X11 and I highly suggest to the Plasma community to decide the same at the sprint. We will have less maintenance issues due to it.

I can promise you that this feature will create bug reports, it comes with a cost to add this on X11. While we have a almost finished Wayland solution which is hold back by among other developers still working on X11.

hein added a subscriber: hein.Apr 21 2018, 8:00 PM

In principle I'm inclined to side with Martin here. At this point, I no longer want the extra burden/distraction of working on newly-introduced X11-related bugs. It's frustrating to spend manhours on code you know will be obsolete, and that delays getting work done to make it obsolete. I still have many scaling problems on Wayland that I think should be higher priority.

I can't give a qualified assessment of the specific risk factor here though.

fvogt added a comment.Apr 21 2018, 8:01 PM

I'm pointing out that I'm in general against any risky changes on X11. If users want to use this features: Wayland is there. KWin is feature frozen on X11 and I highly suggest to the Plasma community to decide the same at the sprint. We will have less maintenance issues due to it.

Yes, but many can't or don't want to use Wayland.
In fact, the machine where I tried this on last had to use the proprietary nvidia driver to work properly. The wayland session doesn't work, it just freezes (I'll try to have a look at that though).

If I'm not mistaken scaling is currently broken on Wayland due to use of XCB as Qt's default platform.

I can promise you that this feature will create bug reports, it comes with a cost to add this on X11. While we have a almost finished Wayland solution which is hold back by among other developers still working on X11.

Sure, but by setting the same scale on each screen you get the exact same behaviour as with the current state. I'd say that's as low risk as it can possibly be.

mart added a subscriber: mart.May 2 2018, 12:30 PM

-1 from here as well, it won't and it can't work reliably, it would be pretty much a false promise

fvogt added a comment.May 2 2018, 12:31 PM

-1 from here as well, it won't and it can't work reliably, it would be pretty much a false promise

It does though.

With https://bugreports.qt.io/browse/QTBUG-67928 fixed, the Xft.dpi setting code shouldn't be needed anymore either.

With https://bugreports.qt.io/browse/QTBUG-67928 fixed, the Xft.dpi setting code shouldn't be needed anymore either.

It is, for things that aren't Qt

fvogt added a comment.May 2 2018, 12:41 PM

It is, for things that aren't Qt

Yup. Just for Qt it's not required anymore.

As for this patch. It needs Qt patches, so it's definitely not 5.13, possibly not even 5.14 material.

After 5.13.0 I'm willing to give this a fair test.
If it indeed does work great, cool.

However, I dont' want to merge something that replaces one set of bugs with a different set of bugs. The number of conflicting scaling approaches we have at once is becoming very difficult to manage.

fvogt added a comment.May 2 2018, 12:49 PM

As for this patch. It needs Qt patches, so it's definitely not 5.13, possibly not even 5.14 material.

After 5.13.0 I'm willing to give this a fair test.
If it indeed does work great, cool.

That's a misunderstanding. The code here as-is works with Qt 5.6 and up.

However, I dont' want to merge something that replaces one set of bugs with a different set of bugs. The number of conflicting scaling approaches we have at once is becoming very difficult to manage.

The mechanism is the same, it just uses more than one value for QT_SCREEN_SCALE_FACTORS.

davidedmundson added inline comments.May 2 2018, 8:03 PM
kcm/src/outputconfig.cpp
143

QPushButton*

fvogt updated this revision to Diff 33520.May 2 2018, 8:33 PM

Fix build

The good:

In one setup I was pleasantly surpised at how well it worked. Fonts did behave as expected.
Specatcle was able to capture all screens and individual rects correctly.

The less good:

When I switched round the primary monitor all through the GUI and rebooting I had a right mess. The @1x screen fonts were double the size, the @2x screen were 4x the size.
In this setup spectacle had an issue with "current screen" when it was on the non primary. I assume this problem: https://bugreports.qt.io/browse/QTBUG-64992 .

Possibly that's just a oneline fix wrt correct font DPI saving.

Does this change make sense whilst Plasma is not using Qt scaling?

fvogt added a comment.May 2 2018, 9:04 PM

The good:

In one setup I was pleasantly surpised at how well it worked. Fonts did behave as expected.
Specatcle was able to capture all screens and individual rects correctly.

The less good:

When I switched round the primary monitor all through the GUI and rebooting I had a right mess. The @1x screen fonts were double the size, the @2x screen were 4x the size.
In this setup spectacle had an issue with "current screen" when it was on the non primary. I assume this problem: https://bugreports.qt.io/browse/QTBUG-64992 .

That's possible - but unlikely: Except for the logical DPI difference the setups are identical.
What happens with Xft.dpi: 96 and non-primary as 2x?

Possibly that's just a oneline fix wrt correct font DPI saving.

Yup, that's the WIP part of this diff. Not sure how to do that though, as the code for this is in the frontend and not the (xrandr) backend.
So if you unplug the primary monitor, newly started applications will behave strangely. But then again, this bug is in the current code as well:
If you plug in a new monitor, the scale is 1 (never actually tried that).

Does this change make sense whilst Plasma is not using Qt scaling?

It means that plasma uses the scale of the primary monitor (actually the font DPI, but that should be the same). So neither better nor worse than the current state.

With Qt scaling, Plasma behaves as expected: Panels and menus scale depending on the monitor they're on. In exchange, some parts (like window tooltips) appear slightly misplaced.

kcm/src/outputconfig.cpp
143

Indeed, how did that build? Maybe the diff actually failed to build but I never noticed.

I've been running with QT_SCREEN_SCALE_FACTORS set manually for at least half a year and it works for me. Except for suspending, so that seems to be a wide spread issue, maybe even in Qt. I'm strongly in favor of this, it is the only thing that works sensibly on xorg. Currently I haven't found a way to have one of the big browsers work with highdpi on wayland, so I'm stuck with X for the time being.

gladhorn added inline comments.Jul 10 2018, 2:37 PM
kcm/src/outputconfig.cpp
145

Why not use QScopedPointer here?

fvogt added inline comments.Jul 10 2018, 2:39 PM
kcm/src/outputconfig.cpp
145

I just moved the code from kcm/src/widget.cpp.

We're past 5.14 and into 5.15 territory now. Does this deserve another look?

My personal opinion is that it's not wise to under-invest in the X11 code while Wayland is still very much a work in progress. I get that it's frustrating to work on code that you know is going to be obsolete, but the time before that happens may be longer than we all think: we've been promising Wayland for years and it's still not at feature parity with X11, and on top of that there are many showstopping user-visible Wayland-specific bugs. I know for me personally it's unusable due to https://bugs.kde.org/show_bug.cgi?id=387156 and https://bugs.kde.org/show_bug.cgi?id=387721 and https://bugs.kde.org/show_bug.cgi?id=384637 and https://bugreports.qt.io/browse/QTBUG-51640 and https://community.kde.org/Plasma/Wayland_Showstoppers#No_remote_support. I would love to use our Wayland session, but because of those issues, I just can't. I suspect a lot of users are in the same boat--many of them because of the NVIDIA situation. Realistically, most will not be running on Wayland until a distro makes it the default (like Fedora did with GNOME), which won't happen until there's feature parity with X11. In all likelihood, Wayland is not going to be a practical choice for a great many of our users for years. So it's not like X11-specific code is just totally worthless. That code is what most of our users rely on every day and would like to see improved.

The negative aspects of this patch are not related to whether Wayland exists.

The negative aspects are that we have ui options that only affect a subset of applications, and merging this will make merging the patch to make plasma use Qt scaling a /lot/ harder. Possibly unfeasibly so.

Also more importantly there's a big with font sizes in some setups to fix.

Ok, thanks for the info. I'll excuse myself now since the technical discussion is way over my head. I just wanted to express support for the idea behind this patch.