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

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.

« Back to merge proposal