This contains the various changes necessary to get the full unit test
suite passing Valgrind, and configures CI to run this.
I disabled relatively few (less than 10) tests under Valgrind because
they're way too slow: all `verifyIntegrity` tests, because those run
anyways in debug and check their own memory health, a font test that
fills out font map, and the sprite render test. Everything else runs
as-is.
I found a number of issues, most were in the tests themselves. A couple
in actual code. A funny one was some undefined memory on tabstop resize
if you exceed the default number of tabstops. I don't know any real
world program that ever even did that (memory issue aside), and that
whole file hasn't been touched since 2022, so that was funny.
No memory leaks in actual code, but a number of leaks in tests. All
resolved.
I think we're still missing some reports because of the Zig bug:
https://github.com/ziglang/zig/issues/19148 so I'm gong to audit our
codebase after this and look for cases of that.
When constraints increased or decreased the size significantly, the
fractional position was getting messed up by the scale. This change
separates that out so that it applies correctly.
This release contains performance and memory use improvements.
Some of the sprite font test renders had to be updated due to very minor
differences in the anti-aliasing, since the default anti-aliasing method
in z2d has been changed to MSAA rather than SSAA.
Similar tests should be added throughout the codebase for any function
that's supposed to gracefully handle OOM conditions. This one was added
because grow previously had a use-after-free bug under OOM, which this
would have caught.
Reordered to form a more logical sequence of steps, cleaned up and
clarified comments, fixed invalid `appendAssumeCapacity` call which
erroneously passed `alloc`, so this compiles again.
Grow needs to allocate and might fail midway. It tries to handle this
using "undo" pattern, and restoring old state on error. But this is
exactly what steps into UAF, as, on error, both errdefer and defer are
run, and the old data is freed.
Instead, use a more robust "reservation" pattern, where we first
fallibly resrve all the resources we need, without applying any changes,
and than do the actual change once we are sure that cannot fail.
I've cleaned up the code we use for scaling and positioning glyphs for
raster, under both CoreText and FreeType. Before we had some
imprecision, and under CoreText we were sometimes stretching glyphs in
unseemly ways. These changes make it so that our constraints can
position and size glyphs *exactly* and we don't have any chopped-off
row/column issues for CoreText. With this, PowerLine Extra symbols now
always align *perfectly* with the cell height:
||Before|After|
|-:|-|-|
|**CoreText**|<img width="105" height="245" alt="image"
src="https://github.com/user-attachments/assets/d3c1b1cb-a798-4e18-a0e0-59551893369c"
/>|<img width="106" height="246" alt="image"
src="https://github.com/user-attachments/assets/dac10c49-9ec1-4f4f-8825-a5e8c2fd3402"
/>|
|**FreeType**|<img width="105" height="245" alt="image"
src="https://github.com/user-attachments/assets/160e1e35-4a3c-42d0-9042-215301e636a1"
/>|<img width="106" height="245" alt="image"
src="https://github.com/user-attachments/assets/89bf1538-7271-4baf-88c0-51ebc4d360df"
/>|
The other changes are mainly just cleanup stuff, though one of the
changes makes it so that we do once again properly apply constraints to
symbols from the dingbats block (it was a regression, noted in #7955,
that we stopped doing that).
### Future work
This has been a problem since we introduced the custom constraints, but
I noticed it while preparing the before/after images: the left-edge PLE
symbols (meant to connect to a full block on the right) expand out to
the *right*, so if they're followed immediately by another character
than they actually get squished and don't match the right-edge symbols:
<img width="75" height="114" alt="image"
src="https://github.com/user-attachments/assets/1420b9a5-9950-4210-9934-8ef7cd7a1e19"
/>
I have a WIP change to move constraint logic to the shapers, and at that
point we can maybe do something to allow the constraint to grow in to
whitespace on the left side instead of on the right side.
We now also have absolute perfect control over the raster position under
FreeType as well. This means that, for example, powerline extended chars
are appropriately clamped to the cell edges at all sizes.
This should be purely an improvement over what we had before, and now it
also matches what we do for CoreText.
Freetype encodes some font names internally in formats other than UTF-8.
This only affects debug logs but it was annoying me so I fixed it. There
may be other encodings that might need to be dealt with but I took care
of the one that I ran across.
The monochrome hinter is very aggressive but makes text actually look
tolerable when rendered in monochrome. If for some god forsaken reason
we get complaints about this, that someone wanted improperly hinted mono
glyphs, we can introduce additonal configuration; but for now, this is
just a straight improvement.
The old method was nice, but had an issue that's intractible without
significant reworking in how we do shaping: combining glyphs need to
position relative to the glyph they're combining with, but if we re-
center that glyph, it will be off by some amount.
Apologies to Apple, the previous comments in this section of the code
were not correct-- `shouldSubpixelQuantizeFonts` does pretty much what
the minimal documentation for it says it does, it simply quantizes the
position of the glyph and nothing more. Various bugs when testing while
writing the old code that led me to include those comments made me not
realize that the positioning is actually a lot simpler than it seems.
With this version of the positioning there are never any cut-off rows or
columns of pixels on the edges of anything and everything scales as it
should... I hope. I checked pretty thoroughly this time and I'm like 99%
sure this is correct in all cases.
Closes#5372
Discretionary ligatures (denoted by the OpenType feature tag `dlig`) are
sometimes used by programming fonts (e.g. Iosevka) to provide more
"complex" and uncommon ligatures that may be useful in a programming
context. Unfortunately, this has some nasty side effects with certain
Japanese fallback fonts (#5372) due to perhaps a misaligned understanding
of the OpenType spec[^spec].
The spec details that `dlig` ligatures should only be used to contract
sequences of glyphs together into one glyph, and that it should be used
only for "special effect", **at the user's preference** (emphasis mine).
Indeed, it also suggests that:
> UI suggestion: This feature should be off by default.
All of this, combined with the fact that historical, nowadays unused and
even unintelligible Kanji ligatures are explicitly included as examples
of discretionary ligatures, shows that in the Japanese context at least
that the "level of discretion" is significantly higher than what is found
in programming fonts, where it is more understood to be "opinionated and
uncommon", rather than "obsolete and unreadable".
Furthermore, it appears that a lot of common programming fonts don't even
make use of the `dlig` feature — JetBrains Mono, FiraCode and MonoLisa
lack a `dlig` feature altogether, while Inconsolata seems to only use it
for ligatures that are more commonly found in `liga` or `calt`, such as
the `->` ligature. To a lot of people, then, this change would literally
alter nothing.
Therefore, it's my opinion that we should disable `dlig` by default.
It's arguably not being used correctly in the programming font space
(or at least not in a way that's coherent with other fonts), and it only
provides a marginal benefit while potentially rendering entire sentences
in Japanese (and possibly other languages) unreadable out of the box.
If someone upgrades to tip or 1.2 and then asks "why aren't the ligatures
working anymore", then at least they can always just turn on `dlig` by
themselves.
[^spec]: https://learn.microsoft.com/en-us/typography/opentype/spec/features_ae#tag-dlig
This adds functionality for choosing different normalization metrics for
each fallback font. It's not exposed as a config option, but could be in
the future, which would probably go a long way towards addressing
concerns like #7929.
The currently available reference metrics are, in priority order:
`ic_width, ex_height, cap_height, line_height, em_size`. The default
value is `ic_width`.
By priority order, I mean that if the chosen metric is not defined in
the fallback font, we move to the next metric in the list---we don't
normalize by an estimated metric from the fallback font (however, we're
happy to use an estimated metric from the primary font, that's how
`ic_width` normalization between CJK and Latin fonts work). This extends
the pattern that was used between `ic_width` and `ex_height` in the
existing hardcoded rule. `line_height` is always defined, so the buck
stops there.
What motivated me to implement this was the fact that, with the existing
hardcoded rule, the embedded symbols-only Nerd Font was always scaled up
by a factor of 1.2, which turned out to be an important reason why it's
been difficult to make icon scaling work to everyone's satisfaction.
Accordingly, the symbols-only font is the first to take advantage of the
new functionality. If this PR is merged, #7917 is no longer needed. (To
limit the scope of this PR, it only includes the minimal changes to let
icon scaling take advantage of this functionality. I may submit a
follow-up PR with some further icon scaling improvement enabled by
this.)
I changed my mind, this is a pretty small change and relevant to the
intent of the PR. This brings the appearance of the embedded symbols
much closer to patched fonts.
With this, the sizes of most symbols are nearly identical to a patched
font, the only big difference is positioning (and TBH I think we do a
better job positioning than the patcher does, since we have knowledge
about the cell size).
Move size adjustment logic out of `Entry`, I understand the impulse to
put it there but it results in passing a lot of stuff around which isn't
great.
Rework `add(...)` in to `add(...)` and `addDeferred(...)`, faces are
passed directly now instead of passing an entry, and an options struct
is used instead of positional arguments for things like style, fallback,
and size adjustment.
Change size adjustment test back to a half pixel tolerance instead of 5%
because the previous commit (allowing fractional pixel sizes) fixed the
root cause of large differences.
This makes the `new_window` action properly inherit properties from the
parent surface that initiated the action. Today, that is only the pwd
and font size.
A variety of naming, commenting, and formatting improvements + a few
explicit error sets. This commit has no functional changes, though it
does remove a couple functions that didn't really need to exist.
The old math didn't allow fractional pixels on the left and bottom, and
stretched glyphs vertically since the height was always rounded up. At
very small font sizes this looked good, but at medium and even large
sizes this just made things inconsistent and janky.
These new calculations are practically pixel-identical to whatever
CoreText is doing in 99% of cases, and the remaining cases seem to be
some sort of auto-hinting since it's internal features of the glyph
getting repositioned.
Over all, I still prefer this to CoreText's quantize option, but if this
causes further issues we should probably just revert the whole thing and
go ahead and add an extra pixel of padding to the bottom and left...
The old math didn't allow fractional pixels on the left and bottom, and
stretched glyphs vertically since the height was always rounded up. At
very small font sizes this looked good, but at medium and even large
sizes this just made things inconsistent and janky.
These new calculations are practically pixel-identical to whatever
CoreText is doing in 99% of cases, and the remaining cases seem to be
some sort of auto-hinting since it's internal features of the glyph
getting repositioned.
Over all, I still prefer this to CoreText's quantize option, but if this
causes further issues we should probably just revert the whole thing and
go ahead and add an extra pixel of padding to the bottom and left...
We do this by characterizing the shared bounding boxes in a static copy
of the symbols only nerd font when we're doing the codegen. This allows
us to get results of our scaling that are just as good as in a patched
font, since related glyphs can now be sized and positioned relative to
each other.
This stops things like folder icons from becoming over-wide. The patcher
typically makes these glyphs always 1 cell wide, but since we know how
it will be displayed we have the benefit of being able to make it more
than 1 cell when there's room. This makes our dynamic scaling *better*
than a static patched font :D
Using the "subpixel quantization" option for rendering our glyph was
creating bad edge cases where we'd lose the bottom or left row / column
of pixels in a glyph sometimes. I investigated the exact effect of this
option and it seems like beyond quantizing the position and scale it's
also doing some rudimentary auto-hinting. That said, the auto-hinting
doesn't do that much for us, and the fact that it horizontally snaps
coordinates to thirds of a pixel instead of whole pixels makes things
worse in terms of legibility at small pixel sizes, so ultimately it's
better with our own handling anyway.
I extensively compared the result of Apple's option with our own manual
quantization here and I'm pretty sure this will always match the whole
pixel sizes, and where it differs (other than things like crossbars) it
seems to make glyphs generally more legible not less.
Nerd font icons were ***WAY*** too big depending on your font setup,
this is because we were always using the full cell height when the nerd
font patcher instead uses an "icon height" for most things. The patcher
calculates the icon height as two thirds of the font's cap height and
one third of the line height, but I've chosen to instead use 1.2 times
the cap height for more consistent results across fonts-- if the user
wants their icons bigger, they can use the `adjust-icon-height` metric
modifier (and they can also use it to make them smaller if they want
that for some reason).
I also adjusted the attributes to user horizontal cover + vertical fit
for `^` stretch modes (proportional scaling but scale up), which makes
it so that it never exceeds the cell size, since first it covers
horizontally and then scales down to fit vertically if necessary;
previously, if there were a particularly wide glyph that was scaled with
cover/cover it would exceed the available width and overflow in to
neighboring cells which wasn't good.
Icons were often WAY too big before because they were filling the whole
cell in height, which isn't great lol. This commit adds an `icon_height`
metric which is used to constrain glyphs that shouldn't be the size of
the entire cell.
This was subtly wrong in a way that was most obvious when text switched
from regular to bold, where it would seem to wiggle since the bearings
of each letter would shift by a pixel in either direction. This affected
applications like fzf which uses bold to dynamically highlight the line
you have selected.
This PR changes `font.Collection` to automagically adjust the sizes of
added fonts so that their metrics (specifically their ex height, or
their ideograph character width if they have one) match the primary
font. This is like
[`font-size-adjust`](https://developer.mozilla.org/en-US/docs/Web/CSS/font-size-adjust)
from CSS.
This is a big win for users who use mixed writing systems and rely
heavily on fallback fonts. For example, in #7774 it's pointed out that
CJK characters are not very well harmonized with existing Latin glyphs,
well:
|Before (`main`)|After (this PR)|
|-|-|
|<img width="326" alt="image"
src="https://github.com/user-attachments/assets/c11d372d-ec69-426d-b008-1f56a7430f23"
/>|<img width="326" alt="image"
src="https://github.com/user-attachments/assets/efcb56ea-0572-481a-b632-a0b5cd170fa9"
/>|
This also improves our handling of the horizontal alignment of fallback
glyphs. It's not an ideal solution; it only works for glyphs narrower
than the cell width because it messes with ligatures if we include
glyphs wider than the cell width; and most things would look better if
the center were proportionally remapped based on the ratio from the
glyph advance to the cell width, but that messes with glyphs designed to
align vertically so it can't be done, instead the original advance width
is centered in the cell width.
Previously produced very wrong values when calling Collection.setSize,
since it was assuming that the provided face had the same point size as
the primary face, which isn't true during resize-- so instead we just
have faces keep track of their set size, this is generally useful.
The nerd font patcher uses `ypadding` as a single subtraction from the
cell height, which means that half of it should go to the top padding
and the other half to the bottom, this was making the heavy brackets way
too small lol (0.4 of the cell height instead of 0.7)
