Merge lp:~dandrader/qtubuntu/logicalDpi into lp:qtubuntu

Proposed by Daniel d'Andrada on 2017-03-24
Status: Merged
Approved by: Gerry Boland on 2017-03-28
Approved revision: 386
Merged at revision: 390
Proposed branch: lp:~dandrader/qtubuntu/logicalDpi
Merge into: lp:qtubuntu
Diff against target: 137 lines (+40/-13)
2 files modified
src/ubuntumirclient/qmirclientscreen.cpp (+34/-11)
src/ubuntumirclient/qmirclientscreen.h (+6/-2)
To merge this branch: bzr merge lp:~dandrader/qtubuntu/logicalDpi
Reviewer Review Type Date Requested Status
Lukáš Tinkl (community) 2017-03-24 Approve on 2017-03-28
Gerry Boland (community) Approve on 2017-03-28
Unity8 CI Bot continuous-integration Approve on 2017-03-24
Review via email: mp+320940@code.launchpad.net

Commit message

Proper implementation of QPlatformScreen::logicalDpi

It's *not* the physical DPI.
It's just a value used for scaling point sized UI elements (mainly QWidget text)

You can view it as the value used to convert pt (point) to pixels

Description of the change

To test. Run kate or QtCreator without any arguments. Then run it with different GRID_UNIT_PX values.

A good reference is that when a QWidget-based application have the same grid unit value as unity8 itself, the text size in its buttons etc should be roughly the same as the size of the text in its window decorations.

To post a comment you must log in.
Lukáš Tinkl (lukas-kde) wrote :

if (mGridUnitPxEnv != 0) {
        mLogicalDpi.first = mGridUnitPxEnv * mGridUnitToLogicalDpiMultiplier;
        mLogicalDpi.second = mGridUnitPxEnv * mGridUnitToLogicalDpiMultiplier;
    } else {
        mLogicalDpi.first = mScale * mMirScaleToGridUnitMultiplier * mGridUnitToLogicalDpiMultiplier;
        mLogicalDpi.second = mScale * mMirScaleToGridUnitMultiplier * mGridUnitToLogicalDpiMultiplier;
    }

I think this could be simplified; if mScale is by default 1.0 and you'd initialize mGridUnitPxEnv to the default value of mMirScaleToGridUnitMultiplier (8.0), this could become one branch.

review: Needs Information
Lukáš Tinkl (lukas-kde) wrote :

And btw, looks like all other platform QPAs use roughly this code for logicalDpi():

458 QDpi QXcbScreen::virtualDpi() const
459 {
460 return QDpi(Q_MM_PER_INCH * m_virtualSize.width() / m_virtualSizeMillimeters.width(),
461 Q_MM_PER_INCH * m_virtualSize.height() / m_virtualSizeMillimeters.height());
462 }

https://code.woboq.org/qt5/qtbase/src/plugins/platforms/xcb/qxcbscreen.cpp.html#_ZNK10QXcbScreen10virtualDpiEv

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:385
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/212/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4683
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4711
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4534
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4534/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4534
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4534/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4534
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4534/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4534
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4534/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4534
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4534/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4534
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4534/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/212/rebuild

review: Approve (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

On 24/03/2017 12:24, Lukáš Tinkl wrote:
> Review: Needs Information
>
> if (mGridUnitPxEnv != 0) {
> mLogicalDpi.first = mGridUnitPxEnv * mGridUnitToLogicalDpiMultiplier;
> mLogicalDpi.second = mGridUnitPxEnv * mGridUnitToLogicalDpiMultiplier;
> } else {
> mLogicalDpi.first = mScale * mMirScaleToGridUnitMultiplier * mGridUnitToLogicalDpiMultiplier;
> mLogicalDpi.second = mScale * mMirScaleToGridUnitMultiplier * mGridUnitToLogicalDpiMultiplier;
> }
>
> I think this could be simplified; if mScale is by default 1.0 and you'd initialize mGridUnitPxEnv to the default value of mMirScaleToGridUnitMultiplier (8.0), this could become one branch.

Then I wouldn't know whether the environment variable GRID_UNIT_PX is
defined or not. This is important since the environment var has a higher
precedence than what the server says (through the scale info).
Specifying a GRID_UNIT_PX when launching an app is a way of overriding
the default behavior (which is getting the scale from the server).

Daniel d'Andrada (dandrader) wrote :

On 24/03/2017 12:35, Lukáš Tinkl wrote:
> And btw, looks like all other platform QPAs use roughly this code for logicalDpi():
>
> 458 QDpi QXcbScreen::virtualDpi() const
> 459 {
> 460 return QDpi(Q_MM_PER_INCH * m_virtualSize.width() / m_virtualSizeMillimeters.width(),
> 461 Q_MM_PER_INCH * m_virtualSize.height() / m_virtualSizeMillimeters.height());
> 462 }
>
> https://code.woboq.org/qt5/qtbase/src/plugins/platforms/xcb/qxcbscreen.cpp.html#_ZNK10QXcbScreen10virtualDpiEv

Yes, I've seem it. logicalDpi() returning the physical DPI in xcb. Not
what we want.

Gerry Boland (gerboland) wrote :

TBH, I am not keen on introducing more dependency on the GRID_UNIT_PX env var, and this usage does not really match up with GRID_UNIT_PX really was intended for.

I also expect that this code will be removed when we implement QPlatformSCreen::pixelDensity(), as that does proper UI scaling of all components of the UI, not just fonts. So I wonder if introducing this as a "transition" to full UI scaling has any real value. Say this breaks fonts in some apps, then we land full UI scaling - will those fonts break again?

Also do we have any existing apps which use fonts with point sizes? Did you test this with them to ensure we don't break them (too badly)?

I do agree that this should be user adjustable, IMO it would be logical for Mir to have an api for this.

+const float QMirClientScreen::mGridUnitToLogicalDpiMultiplier = 14.0; // by experimentation
IMO should be 12, as on desktop 8*12=96 which is the typical value.

+ mLogicalDpi.first = 0;
+ mLogicalDpi.second = 0;
think unnecessary, as QPair claims to default construct the values.

Not a full review, want to ensure we're on the right track with this first.

Daniel d'Andrada (dandrader) wrote :

On 27/03/2017 10:54, Gerry Boland wrote:
> Also do we have any existing apps which use fonts with point sizes? Did you test this with them to ensure we don't break them (too badly)?
What do you mean by "existing apps"? All existing QWidged-based apps
which are exactly the topic of this patch or do you mean some other
class of applications?

Daniel d'Andrada (dandrader) wrote :

On 27/03/2017 10:54, Gerry Boland wrote:
> I also expect that this code will be removed when we implement QPlatformSCreen::pixelDensity(), as that does proper UI scaling of all components of the UI, not just fonts. So I wonder if introducing this as a "transition" to full UI scaling has any real value. Say this breaks fonts in some apps, then we land full UI scaling - will those fonts break again?

I don't think so. QPlatformSCreen::pixelDensity() is orthogonal to that. As I said, logicalDPI() is a conversion from point size to pixel (probably device independent ones). Those scaling factors act at pixel level, after the conversion from point size has been done.

That said, tested multiple values of QPlatformScreen::pixelDensity() and it had not impact whatsoever on QWidget apps (with or without this logicalDpi) patch. devicePixelRatio() seriously brake QWidget-based apps with or without this logicalDpi patch.

Gerry Boland (gerboland) wrote :

> On 27/03/2017 10:54, Gerry Boland wrote:
> > Also do we have any existing apps which use fonts with point sizes? Did you
> test this with them to ensure we don't break them (too badly)?
> What do you mean by "existing apps"? All existing QWidged-based apps
> which are exactly the topic of this patch or do you mean some other
> class of applications?

Well this impacts not just QWidget apps, it is anything Qt that draws fonts at point sizes. So in QML, people might be using Text & point sizes instead of Label (that is app bug though, should be using Label so I don't care about that case), and people can be using Canvas and drawing text.

OFC since the current code is wrong, us fixing this means the apps that we break need to be fixed too.

But realistically the battery usage image in System Settings is the only one I really care about. If we break that, we should fix it.

Daniel d'Andrada (dandrader) wrote :

> But realistically the battery usage image in System Settings is the only one I
> really care about. If we break that, we should fix it.

Didn't spot any visual differences in this application with and without this patch.

Daniel d'Andrada (dandrader) wrote :

> But realistically the battery usage image in System Settings is the only one I
> really care about. If we break that, we should fix it.

Didn't spot any visual differences in this application with and without this patch.

Gerry Boland (gerboland) wrote :

> On 27/03/2017 10:54, Gerry Boland wrote:
> > I also expect that this code will be removed when we implement
> QPlatformSCreen::pixelDensity(), as that does proper UI scaling of all
> components of the UI, not just fonts. So I wonder if introducing this as a
> "transition" to full UI scaling has any real value. Say this breaks fonts in
> some apps, then we land full UI scaling - will those fonts break again?
>
> I don't think so. QPlatformSCreen::pixelDensity() is orthogonal to that. As I
> said, logicalDPI() is a conversion from point size to pixel (probably device
> independent ones). Those scaling factors act at pixel level, after the
> conversion from point size has been done.

pixelDensity is intended for full UI scaling. You are scaling fonts with this. Full UI scaling will also scale fonts, so I don't think they are orthogonal. And the Grid Unit concept is for scaling whole UIs, not just the fonts.

If you used a different env var, and made the default 96, I'd be happier, as the ability to scale fonts alone is still something we want.

But this is combining full UI scaling with just-font scaling, which is incorrect. They are 2 different scaling mechanisms.

> That said, tested multiple values of QPlatformScreen::pixelDensity() and it
> had not impact whatsoever on QWidget apps (with or without this logicalDpi)
> patch. devicePixelRatio() seriously brake QWidget-based apps with or without
> this logicalDpi patch.

Sadly you can't just set pixelDensity to 2 and test apps - it requires a bit more work in the QPA to make pixelDensity actually scale the UI (c.f. QHighDpi).

Note: DevicePixelRatio is not what we need any more, that is for platforms like iOS and OSX that (for backward compatibility) tell clients they are on low dpi screens, and add extra api for newer clients to get the 2x/3x multiplier and draw at higher detail. It was enough to get the UI to scale back in Qt5.4, but now pixelDensity is considered the proper solution for full UI scaling that we want.

lp:~dandrader/qtubuntu/logicalDpi updated on 2017-03-28
386. By Daniel d'Andrada on 2017-03-28

Forget about GRID_UNIT_PX

Gerry Boland (gerboland) wrote :

lgtm

review: Approve
Lukáš Tinkl (lukas-kde) wrote :

Tested as well, worked fine

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ubuntumirclient/qmirclientscreen.cpp'
2--- src/ubuntumirclient/qmirclientscreen.cpp 2017-02-07 15:37:20 +0000
3+++ src/ubuntumirclient/qmirclientscreen.cpp 2017-03-28 12:14:55 +0000
4@@ -1,6 +1,6 @@
5 /****************************************************************************
6 **
7-** Copyright (C) 2014-2016 Canonical, Ltd.
8+** Copyright (C) 2014-2017 Canonical, Ltd.
9 ** Contact: https://www.qt.io/licensing/
10 **
11 ** This file is part of the plugins of the Qt Toolkit.
12@@ -56,8 +56,12 @@
13
14 #include <memory>
15
16+#define ENV_LOGICAL_DPI "QT_LOGICAL_DPI"
17+
18 static const int overrideDevicePixelRatio = qgetenv("QT_DEVICE_PIXEL_RATIO").toInt();
19
20+const float QMirClientScreen::mMirScaleToLogicalDpiMultiplier = 96.0; // by definition in qtmir
21+
22 static const char *orientationToStr(Qt::ScreenOrientation orientation) {
23 switch (orientation) {
24 case Qt::PrimaryOrientation:
25@@ -82,12 +86,23 @@
26 : mDevicePixelRatio(1.0)
27 , mFormat(QImage::Format_RGB32)
28 , mDepth(32)
29- , mDpi{0}
30 , mFormFactor{mir_form_factor_unknown}
31 , mScale{1.0}
32 , mOutputId(0)
33 , mCursor(connection)
34 {
35+
36+ if (qEnvironmentVariableIsSet(ENV_LOGICAL_DPI)) {
37+ QByteArray stringValue = qgetenv(ENV_LOGICAL_DPI);
38+ bool ok;
39+ float value = stringValue.toFloat(&ok);
40+ if (ok && value > 0) {
41+ mLogicalDpiEnv = value;
42+ }
43+ }
44+
45+ updateLogicalDpi();
46+
47 setMirOutput(output);
48 }
49
50@@ -183,6 +198,7 @@
51
52 // UI scale & DPR
53 mScale = mir_output_get_scale_factor(output);
54+ updateLogicalDpi();
55 if (overrideDevicePixelRatio > 0) {
56 mDevicePixelRatio = overrideDevicePixelRatio;
57 } else {
58@@ -234,16 +250,12 @@
59 }
60 }
61
62-void QMirClientScreen::setAdditionalMirDisplayProperties(float scale, MirFormFactor formFactor, int dpi)
63+void QMirClientScreen::setAdditionalMirDisplayProperties(float scale, MirFormFactor formFactor, int /*dpi*/)
64 {
65- if (mDpi != dpi) {
66- mDpi = dpi;
67- QWindowSystemInterface::handleScreenLogicalDotsPerInchChange(screen(), dpi, dpi);
68- }
69-
70 auto nativeInterface = static_cast<QMirClientNativeInterface *>(qGuiApp->platformNativeInterface());
71 if (!qFuzzyCompare(mScale, scale)) {
72 mScale = scale;
73+ updateLogicalDpi();
74 nativeInterface->screenPropertyChanged(this, QStringLiteral("scale"));
75 }
76 if (mFormFactor != formFactor) {
77@@ -254,9 +266,20 @@
78
79 QDpi QMirClientScreen::logicalDpi() const
80 {
81- if (mDpi > 0) {
82- return QDpi(mDpi, mDpi);
83+ return mLogicalDpi;
84+}
85+
86+void QMirClientScreen::updateLogicalDpi()
87+{
88+ auto oldDpi = mLogicalDpi;
89+
90+ if (mLogicalDpiEnv != 0) {
91+ mLogicalDpi.first = mLogicalDpi.second = mLogicalDpiEnv;
92 } else {
93- return QPlatformScreen::logicalDpi();
94+ mLogicalDpi.first = mLogicalDpi.second = mScale * mMirScaleToLogicalDpiMultiplier;
95+ }
96+
97+ if (oldDpi.first != 0 && oldDpi.second != 0 && oldDpi != mLogicalDpi) {
98+ QWindowSystemInterface::handleScreenLogicalDotsPerInchChange(screen(), mLogicalDpi.first, mLogicalDpi.second);
99 }
100 }
101
102=== modified file 'src/ubuntumirclient/qmirclientscreen.h'
103--- src/ubuntumirclient/qmirclientscreen.h 2017-02-07 15:37:20 +0000
104+++ src/ubuntumirclient/qmirclientscreen.h 2017-03-28 12:14:55 +0000
105@@ -1,6 +1,6 @@
106 /****************************************************************************
107 **
108-** Copyright (C) 2014-2016 Canonical, Ltd.
109+** Copyright (C) 2014-2017 Canonical, Ltd.
110 ** Contact: https://www.qt.io/licensing/
111 **
112 ** This file is part of the plugins of the Qt Toolkit.
113@@ -85,6 +85,7 @@
114
115 private:
116 void setMirOutput(const MirOutput *output);
117+ void updateLogicalDpi();
118
119 QRect mGeometry, mNativeGeometry;
120 QSizeF mPhysicalSize;
121@@ -93,12 +94,15 @@
122 Qt::ScreenOrientation mCurrentOrientation;
123 QImage::Format mFormat;
124 int mDepth;
125- int mDpi;
126 qreal mRefreshRate;
127 MirFormFactor mFormFactor;
128 float mScale;
129 int mOutputId;
130 QMirClientCursor mCursor;
131+ float mLogicalDpiEnv{0};
132+ QDpi mLogicalDpi;
133+
134+ static const float mMirScaleToLogicalDpiMultiplier;
135
136 friend class QMirClientNativeInterface;
137 };

Subscribers

People subscribed via source and target branches