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

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.

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

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

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

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.

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.

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?

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.

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

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

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

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

lgtm

Tested as well, worked fine