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

[Daniel d'Andrada]

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

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.

[Lukáš Tinkl (lukas-kde)]

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)]

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)]

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)]

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)]

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)]

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)]

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)]

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)]

> 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)]

> 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)]

> 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)]

> 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]

386. By Daniel d'Andrada on 2017-03-28

Forget about GRID_UNIT_PX

[Gerry Boland (gerboland)]

lgtm

review: Approve

[Lukáš Tinkl (lukas-kde)]

Tested as well, worked fine

review: Approve

=== modified file 'src/ubuntumirclient/qmirclientscreen.cpp'
--- src/ubuntumirclient/qmirclientscreen.cpp	2017-02-07 15:37:20 +0000
+++ src/ubuntumirclient/qmirclientscreen.cpp	2017-03-28 12:14:55 +0000
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2014-2016 Canonical, Ltd.
+** Copyright (C) 2014-2017 Canonical, Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the plugins of the Qt Toolkit.
@@ -56,8 +56,12 @@
 
 #include <memory>
 
+#define ENV_LOGICAL_DPI "QT_LOGICAL_DPI"
+
 static const int overrideDevicePixelRatio = qgetenv("QT_DEVICE_PIXEL_RATIO").toInt();
 
+const float QMirClientScreen::mMirScaleToLogicalDpiMultiplier = 96.0; // by definition in qtmir
+
 static const char *orientationToStr(Qt::ScreenOrientation orientation) {
     switch (orientation) {
         case Qt::PrimaryOrientation:
@@ -82,12 +86,23 @@
     : mDevicePixelRatio(1.0)
     , mFormat(QImage::Format_RGB32)
     , mDepth(32)
-    , mDpi{0}
     , mFormFactor{mir_form_factor_unknown}
     , mScale{1.0}
     , mOutputId(0)
     , mCursor(connection)
 {
+
+    if (qEnvironmentVariableIsSet(ENV_LOGICAL_DPI)) {
+        QByteArray stringValue = qgetenv(ENV_LOGICAL_DPI);
+        bool ok;
+        float value = stringValue.toFloat(&ok);
+        if (ok && value > 0) {
+            mLogicalDpiEnv = value;
+        }
+    }
+
+    updateLogicalDpi();
+
     setMirOutput(output);
 }
 
@@ -183,6 +198,7 @@
 
     // UI scale & DPR
     mScale = mir_output_get_scale_factor(output);
+    updateLogicalDpi();
     if (overrideDevicePixelRatio > 0) {
         mDevicePixelRatio = overrideDevicePixelRatio;
     } else {
@@ -234,16 +250,12 @@
     }
 }
 
-void QMirClientScreen::setAdditionalMirDisplayProperties(float scale, MirFormFactor formFactor, int dpi)
+void QMirClientScreen::setAdditionalMirDisplayProperties(float scale, MirFormFactor formFactor, int /*dpi*/)
 {
-    if (mDpi != dpi) {
-        mDpi = dpi;
-        QWindowSystemInterface::handleScreenLogicalDotsPerInchChange(screen(), dpi, dpi);
-    }
-
     auto nativeInterface = static_cast<QMirClientNativeInterface *>(qGuiApp->platformNativeInterface());
     if (!qFuzzyCompare(mScale, scale)) {
         mScale = scale;
+        updateLogicalDpi();
         nativeInterface->screenPropertyChanged(this, QStringLiteral("scale"));
     }
     if (mFormFactor != formFactor) {
@@ -254,9 +266,20 @@
 
 QDpi QMirClientScreen::logicalDpi() const
 {
-    if (mDpi > 0) {
-        return QDpi(mDpi, mDpi);
+    return mLogicalDpi;
+}
+
+void QMirClientScreen::updateLogicalDpi()
+{
+    auto oldDpi = mLogicalDpi;
+
+    if (mLogicalDpiEnv != 0) {
+        mLogicalDpi.first = mLogicalDpi.second = mLogicalDpiEnv;
     } else {
-        return QPlatformScreen::logicalDpi();
+        mLogicalDpi.first = mLogicalDpi.second = mScale * mMirScaleToLogicalDpiMultiplier;
+    }
+
+    if (oldDpi.first != 0 && oldDpi.second != 0 && oldDpi != mLogicalDpi) {
+        QWindowSystemInterface::handleScreenLogicalDotsPerInchChange(screen(), mLogicalDpi.first, mLogicalDpi.second);
     }
 }
 
=== modified file 'src/ubuntumirclient/qmirclientscreen.h'
--- src/ubuntumirclient/qmirclientscreen.h	2017-02-07 15:37:20 +0000
+++ src/ubuntumirclient/qmirclientscreen.h	2017-03-28 12:14:55 +0000
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2014-2016 Canonical, Ltd.
+** Copyright (C) 2014-2017 Canonical, Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the plugins of the Qt Toolkit.
@@ -85,6 +85,7 @@
 
 private:
     void setMirOutput(const MirOutput *output);
+    void updateLogicalDpi();
 
     QRect mGeometry, mNativeGeometry;
     QSizeF mPhysicalSize;
@@ -93,12 +94,15 @@
     Qt::ScreenOrientation mCurrentOrientation;
     QImage::Format mFormat;
     int mDepth;
-    int mDpi;
     qreal mRefreshRate;
     MirFormFactor mFormFactor;
     float mScale;
     int mOutputId;
     QMirClientCursor mCursor;
+    float mLogicalDpiEnv{0};
+    QDpi mLogicalDpi;
+
+    static const float mMirScaleToLogicalDpiMultiplier;
 
     friend class QMirClientNativeInterface;
 };