Units: make smallSpacing 4px and largeSpacing 8px
TL;DR
Convert spacings to static amounts of pixels because when designing UIs, this is how we tend to think of them in practice and our theming systems also use static amounts of pixels (QStyles, plasma themes).
In theory, making spacing dynamic based on the font height allows the spacings to scale with the size of content directly and in many cases it does. Unfortunately, since our theming systems don't really do this, we sometimes end up with inconsistent looking spacing and padding.
How fonts and units currently work
gridUnit is QFontMetricsF::height(). It is 18px with Noto Sans 10 and 22px with Noto Sans 11. This is a huge jump in font height (1.22x) even though characters have only increased in size by 1.1x. Perhaps this is a problem with Noto Sans 11, but there simply aren't enough rules in place to keep this kind of problem from happening. It also might not be a problem with the font, just a part of Noto Sans' goal to support every character, including large characters like ꙲. Maybe the problem is in QFontMetricsF or even deeper in FreeType (QFontMetricsF gets its info from FreeType), but I doubt any of us are going to fix it (if it even is a bug) and I doubt a fix will come from someone outside of our group. Fonts are an extremely complex subject and fonts have to deal with extremely touchy and complex real world subjects, like how to faithfully represent a wide variety of scripts that maybe have inconsistent rules which must be respected.
smallSpacing is std::floor(gridUnit / 4). With Noto Sans 10, this is 4px. With Noto Sans 11, this is 5px; a 1.25x increase.
- If it rounded instead, it would be 5px with Noto Sans 10 and 6px with Noto Sans 11; a 1.2x increase.
largeSpacing is smallSpacing * 2.
Do not assume that smallSpacing * 4 or largeSpacing * 2 will be equal to gridUnit.
Alternative font based solutions for spacing units that I have pondered
What about pixelSize? Could we use that instead of font height? It
might be better in some ways, but worse in others. pixelSize is a pixel
based way to measure fonts that is actually arbitrary in practice. Noto
Sans' Latin and CJK characters happen to take up roughly the same amount
of vertical space as their pixelSize, so maybe we could base the spacing
on the pixelSize instead. However, not all fonts use a pixelSize the
same as the height of their tallest and lowest reaching Latin
characters. I use kpAJE| as a basic Latin string and pixeltool for
comparing pixelSize to real size. GNOME's Cantarell font shoud be 16px
from the top of k to the bottom of p if the pixelSize is 16px, but it is
actually 15px. Hack's Latin text is 17px high with a pixelSize of 16
(some poeple like monospace for everything). These two aren't large
differences, but the differences could still affect the values of
spacing units if based on pixelSize and other fonts have larger
differences. pixelSize is less variable (it's basically just pointSize * 1.33 or pointSize / 0.75), but doesn't necessarily represent what
is actually in the font. QFontMetricsF::height() represents what is
actually in the font and is the default height of all text elements, but
can vary a lot. Tradeoffs have to be made.
- Noto Sans 10 has a pixelSize of 13.33 (10/0.75) and Noto Sans 11 has a pixelSize of 14.67 (11/0.75). Making consistent/good integer based spacing units from those might be a little tricky.
What about measuring the real height of some characters and basing spacings off of that? That could work. Maybe not for highly stylized fonts, but it could work for common sans serif fonts. We still have to make a lot of assumptions about how fonts should be though and fonts can still behave differently at different sizes. Making good/consistent spacing units from these will also be a little tricky.
Why switch to logical pixels?
6px is the standard amount of margin/spacing that the Breeze QStyle and many parts of the Breeze Plasma style use. It doesn't change with font size, so basing spacing units on font size makes us less likely to get the right size. Could we make the QStyle use font based units instead? Probably, but maybe we shouldn't. We can't make Plasma theme margins based on font size. You can't embed dynamic system font size into an SVG in a way that works for our theming system. However, these are technicalities.
It's just difficult to control all the variables when dealing with font sizes. Switching to pixels would make things less dynamic, but it would also make it easier to design a good looking UI. We can finally make assumptions about whether or not a unit will be divisible by 2, 3 or 4 without having to round. Rounding isn't a big deal on its own, but it's another place for inconsistency to occur. In one place, something rounds down and you get less padding than average. In another place, something rounds up and you get more padding than average. Some people floor, some people round and some people ceil in various situations, leading to more opporunities for inconsistencies.
Most people don't increase or decrease font sizes so much that scaling spacings to font size matters much. Most people who increase font size a lot have difficulty seeing text. We should probably do some research into whether or not they need more spacing too, but the problem with increasing text size is you also have less space for text. If you increase spacing on top of that, you have even less space for text. Maybe they don't need their spacings to scale?
The lack of a 6px unit will not be solved by this patch. I intend to add
a 6px mediumSpacing property in a different patch: !481 (merged)
History of our spacing units
As far as I can tell, smallSpacing and largeSpacing originally came from
Plasma, which doesn't support Qt UI scaling on X11. In Plasma, we
normally use Math.round(value * PlasmaCore.Units.devicePixelRatio) if
we want to scale some raw pixels with the system scale because
PlasmaCore.Units.devicePixelRatio is based on the font and the font size
scales with the system. smallSpacing in Kirigami is pretty much
unchanged from how it was in Plasma, but largeSpacing in Plasma is
identical to the font height/gridUnit.