Bug 431947 - GridUnit increases in size by a greater percentage than the increase in the font size
Summary: GridUnit increases in size by a greater percentage than the increase in the f...
Status: RESOLVED UPSTREAM
Alias: None
Product: frameworks-kirigami
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.78.0
Platform: Other Linux
: NOR normal
Target Milestone: Not decided
Assignee: Marco Martin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-22 18:14 UTC by Nate Graham
Modified: 2021-03-05 15:57 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Test case (377 bytes, text/x-qml)
2021-01-22 18:16 UTC, Nate Graham
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Graham 2021-01-22 18:14:58 UTC
GridUnit bases its size on the font. It is GridUnit is 18px with the default 10pt Noto Sans.

If you increase the font size to 11pt Noto Sans (10% bigger), GridUnit becomes 22px, which is 22% bigger. I would expect it to become 20px, which is 11% bigger.

As a result, increasing the font size from 10 to 11 to make text more readable results in UI elements throughout Plasma and QML apps becoming proportionally larger than they should be.
Comment 1 Nate Graham 2021-01-22 18:16:48 UTC
Created attachment 135066 [details]
Test case

Run this in qmlscene before and after changing the font size from 10pt Noto Sans to 11pt Noto Sans
Comment 2 David Redondo 2021-01-27 08:14:57 UTC
I noticed that the 18px which is supposed the height of "M" has nothing to do with the actual height of the rendered letter. If we look at tightBoundingRect instead of boundingRect, then the height increases from 9 to 10 which is an 11% increase.
Looking at the width it increases from 10 to 12 for both properties (20% increase).

Funnily enough the comment talks about width of "M" but we query the height.
Comment 3 Nate Graham 2021-01-27 16:33:24 UTC
Yeah that seems wrong. Going from 9 to 10 seems like what I would expect. Then GridUnit could be that value multiplied by 2, making it 18px with 10pt Noto Sans and 20px with 11pt Noto Sans.
Comment 4 David Redondo 2021-01-27 17:21:17 UTC
(In reply to Nate Graham from comment #3)
> Yeah that seems wrong. Going from 9 to 10 seems like what I would expect.
> Then GridUnit could be that value multiplied by 2, making it 18px with 10pt
> Noto Sans and 20px with 11pt Noto Sans.

We would need to be very careful switching this and also look at other fonts
Comment 5 Nate Graham 2021-02-13 15:50:05 UTC
On further investigation, this isn't a bug in the unit code, but rather if anything a bug in Noto Sans itself. The size difference between 10pt and 11pt is huge; much larger than you would expect. This is what's behind GridUnit being larger than expected: the text itself is larger than expected!
Comment 6 Noah Davis 2021-03-05 10:11:07 UTC
One potential workaround is to adjust the padding in places where gridUnit and anything that shows text are used to compensate for the jump in size.

Here's an example of what I mean:

Units.qml:

    // The boundingRect height for Noto Sans 96 is 175 px and the pixelSize is 128.
    property var __boundingRectRatioFontMetrics: FontMetrics {
        font.pointSize: 96
    }
    
    // The value is exactly 1.3671875 with Noto Sans.
    property real __expectedPixelSizeBoundingRectRatio: (__boundingRectRatioFontMetrics.height / __boundingRectRatioFontMetrics.font.pixelSize)
    
    /* pixelSize is always pointSize / 0.75, pointSize is always pixelSize * 0.75.
     * Noto Sans 10 has a boundingRect height of 18.
     * Noto Sans 10.5 has a boundingRect height of 20.
     * Noto Sans 11 has a boundingRect height of 22.
     * Internally, QFontMetricsF rounds the font height (not floor, not ceil), so we can't get the true height, but maybe it doesn't matter that much.
     *
     * With Noto Sans 10 (13.33... pixelSize), 13.33... * 1.3671875 is 18.229166...
     * 18 - 18.229166... is less than 0, so don't reduce padding.
     *
     * With Noto Sans 10.5 (14 pixelSize), 14 * 1.3671875 is 19.140625.
     * 20 - 19.140625 is 0.859375
     * Divide that by 2 since padding is applied for 2 sides on the same axis.
     * Round 0.4296875 to 0, so don't reduce padding.
     *
     * With Noto Sans 11, (14.66... pixelSize) 14.66... * 1.3671875 is 20.0520833...
     * 22 - 20.0520833... is 1.9479166...
     * Divide that by 2 since padding is applied for 2 sides on the same axis.
     * Round 0.97395833... to 1, so reduce padding by 1.
     */
    property int paddingOffset: Math.min(0, -Math.round(Math.max(0, gridUnit - (fontMetrics.font.pixelSize * __expectedPixelSizeBoundingRectRatio))/2))

You could then use it like this:

    padding: someValue + paddingOffset

This isn't very elegant though. We could apply it directly to the spacings in Kirigami.Units, but I'm not sure if we should.

I don't think we should reduce gridUnit because controls that show text will still use the full font height as the content height. That could lead to alignment issues.
Comment 7 Nate Graham 2021-03-05 15:57:44 UTC
As I mentioned earlier, I don't think our code is at fault here. It seems to be doing what's expected of it, and the problem--if anything--is in Noto Sans itself, for having such a huge difference in size between the 10pt and 11pt sizes. If we reduce the paddings to work around that, then people using 11pt Noto Sans will have a UI with less relative padding than one with 10pt Noto Sans.