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

Proposed by Daniel d'Andrada
Status: Merged
Approved by: Gerry Boland
Approved revision: 386
Merged at revision: 390
Proposed branch: lp://qastaging/~dandrader/qtubuntu/logicalDpi
Merge into: lp://qastaging/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://qastaging/~dandrader/qtubuntu/logicalDpi
Reviewer Review Type Date Requested Status
Lukáš Tinkl (community) Approve
Gerry Boland (community) Approve
Unity8 CI Bot continuous-integration Approve
Review via email: mp+320940@code.qastaging.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.
Revision history for this message
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
Revision history for this message
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

Revision history for this message
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)
Revision history for this message
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).

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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?

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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.

386. By Daniel d'Andrada

Forget about GRID_UNIT_PX

Revision history for this message
Gerry Boland (gerboland) wrote :

lgtm

review: Approve
Revision history for this message
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
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches