qtubuntu

Code review comment for lp:~dandrader/qtubuntu/logicalDpi

logicalDpi
Merge into trunk

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-28:

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

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

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.

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

« Back to merge proposal