Repository navigation
fix(font): prefer the face width for non-ASCII fallback - #481
Conversation
Character fallback scored candidates by the fontset spec's width only. Specs such as (nil . "iso10646-1") leave it unset, so every width tied and discovery order decided: with Iosevka-Regular.ttc the Extended face (width 125) won for Greek, accented Latin and punctuation while ASCII used the normal Regular face. GNU font_select_entity fills an unset preferred width from the face, as it does for weight and slant. Score against the captured effective width instead of the explicit spec width.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCharacter fallback candidate scoring now uses the captured effective width, including the requested width when the fontset specification leaves width unset. Tests cover face selection by requested width and explicit-width precedence in live and frozen-worker lookups, including cache hits. ChangesCharacter Fallback Width
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Although I found no blocking issues, this change touches font-selection behavior affecting all non-ASCII text rendering, so final human verification of the visual/selection outcome is warranted before approval.
Review effort: Balanced
Findings: None
What changed in this PR
This PR fixes a font-fallback regression where font collections like Iosevka drew non-ASCII characters (Greek, Cyrillic, accented Latin, punctuation such as —) with a wider face than ASCII, diverging from GNU Emacs. The root cause was that character fallback scored candidate width only against the fontset spec's explicit width; fontset entries such as (nil . "iso10646-1") leave width unset, so every candidate tied and Fontconfig discovery order (which lists Iosevka's Extended face first) decided the winner. The change makes resolve_from_policy score width against the captured effective width (spec width, or the requested face width when unset), mirroring how GNU's font_select_entity fills an unset preferred width from the face, and how the resolver already handles weight and slant.
Changes:
- Score
SelectionRequest.widthusingSome(policy.width)(the effective captured width) instead of the explicit-only spec width inFontResolver::resolve_from_policy. - Remove the now-unused
explicit_widthfield fromCapturedCharacterPolicy(definition and both capture sites). - Add a regression test asserting the requested face width, not discovery order, decides among collection faces of differing widths.
| File | Description |
|---|---|
| crates/neomacs-layout-engine/src/font/resolver.rs | Scores width on the effective captured width and drops the explicit_width field from the struct and its capture site. |
| crates/neomacs-layout-engine/src/font/resolver/worker_policy.rs | Drops the explicit_width field from the worker capture_bounded path. |
| crates/neomacs-layout-engine/src/font/resolver/tests/mod.rs | Adds a test verifying the requested face width is preferred over Fontconfig discovery order. |
I reviewed the width-scoring change, confirmed the explicit_width field is fully removed with no dangling references, traced the scoring through candidate_selection_score (so Some(Normal) correctly penalizes wider faces while explicit-width specs are unchanged), and verified the new test exercises the generic-fallback (unset-width) path that the fix targets. The change is minimal, correct, and consistent with the stated scope; the two capture paths' differing width defaults (sync uses the requested width, worker hardcodes Normal and keys its cache on Normal) have no functional effect today since callers always pass Normal, matching the PR's acknowledged out-of-scope note. I found no issues to flag.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The face width only fills an unset fontset width. Check that an explicit Expanded fontset spec still wins over a Normal face request, on the live path and on the frozen worker path, on a fresh lookup and on a cache hit.
thanos already carries the content of these heads, verbatim or as the personal variant it ships; this merge keeps the tree and records the heads so later upstream merges of them need no re-resolution. - contrib/repeat-backpressure 551220e (eval-exec#459): carried; the rebase only moved to FrontendKey, as the main merge did. - contrib/nested-reader-recovery 91a4022 (eval-exec#460): carried, combined with the command-error reporting below. - fix/command-error-literal-message 271a202 (eval-exec#489): carried as 56339fa, 98b7742, 0ebd026. - fix/non-ascii-face-width 9a52a76 (eval-exec#481): carried as 13ad539 and 7eee365. - feature/builtin-mcp c9542d3 (eval-exec#480): personal endpoint variant with the same reply bound (677591e) and documentation (e0ee6c3). - fix/text-prop-interval-recycling 38b1e0a (eval-exec#479): personal interval recycling passes the same retention tests. - feature/gui-daemon-publication 1cd96f8 (eval-exec#454): personal deferred GUI daemon, a superset. Kept deliberately: terminal-live-p classifies every terminal by its output method (GNU Fterminal_live_p), an unregistered frame's native-window wait fails closed, and neomacs-set-frame-opacity passes integer percentages through unchanged, since alpha-background already reads a fixnum as a percentage (GNU gui_set_alpha_background).
eval-exec
left a comment
eval-exec
left a comment
There was a problem hiding this comment.
Verified against GNU 31.1 built from the emacs-mirror tree at a360712c (source plus a live batch run), and against a private worktree at 9a52a76 with cargo nextest run -p neomacs-layout-engine --lib.
GNU side — the claims hold.
font_select_entity (font.c:3171) builds prefer from the face's font spec fields, falls back to FONT_SET_STYLE (prefer, FONT_WIDTH_INDEX, attrs[LFACE_SWIDTH_INDEX]), then calls font_sort_entities: an unset width is filled from the face, for the same reason as weight and slant. font_score (font.c:2128) scores width as a ranking penalty and rejects only on size (factor-2). candidate_selection_score already has that shape, and GnuStyleScore's field order matches GNU's default face-font-selection-order — so the request is the right lever, in the right place.
Tests — reproduced, including the negative.
Both named tests pass here (2/2). As a control I reverted width: Some(policy.width) to width: None: character_fallback_prefers_the_requested_face_width then fails with exactly the PR body's values — left: (Some(Expanded), 3), right: (Some(Normal), 0), "requested Normal". The test catches the bug rather than passing incidentally.
For the record: query.requested_width was already policy.width before this change. The bug was that select_best_candidate re-ranks from candidate_score plus discovery ordinal, and with width: None every candidate scored 0 on width, so the tie-break discarded the width-aware ordering the backend had already been asked for. Query width-aware, selection width-blind.
GUI table — the non-ASCII row reproduced headlessly.
The real system Iosevka through the real fontconfig backend and the real resolver (font selection needs no display):
| build | α, é, — (non-ASCII, width-less fontset entry) |
|---|---|
pre-fix (width: None) |
Some(Expanded) → Iosevka-Extended.ttf |
| this PR | Some(Normal) → Iosevka-Regular.ttf |
Caveat: here Iosevka ships as per-style files rather than the Iosevka-Regular.ttc collection, so the in-collection face index isn't reproduced (the fixtures cover that synthetically), and I couldn't run GNU or watch pixels on this box — the local GNU build is GTK3, not pgtk. GNU's decision follows from font_select_entity filling the width from the face, so its row should be the Regular face.
Latent: the frozen worker path can't serve a non-Normal request at all.
capture_bounded (worker_policy.rs:108) takes no width and hardcodes spec.width.unwrap_or(FontWidth::Normal) — width is the only one of weight/slant/width the frozen capture doesn't receive from its caller. The sharper half is the key: FrozenCharacterPolicies::capture builds CharCacheKey { …, width: FontWidth::Normal.gnu_numeric(), … } (worker_policy.rs:47), so the snapshot is keyed Normal-only, and a miss in resolve_worker_character (worker_policy.rs:222) sets missing and returns None — no fallback.
Measured with a scratch test — no :width in the spec, Extended face listed first:
| request | live | frozen |
|---|---|---|
| Normal | Some((Normal, 0)) |
Some((Normal, 0)) |
| Expanded | Some((Expanded, 3)) |
None + worker_policy_missing() |
The two paths agree only while every caller requests Normal, which holds today only because metrics.rs:2828 hardcodes FontWidth::Normal. The scoped follow-up ("pass the face width through to both paths") would therefore make Extended-face text resolve to no font on the worker path while looking correct on the main thread. Threading the width into capture_bounded is necessary but not sufficient — the key has to carry it too, or stop keying on width.
The second test can't catch this: it always sets an explicit spec width, so unwrap_or never fires and both paths agree by construction. The scratch test above would; happy to send it as a patch.
Smaller
selection.rs:48-55: GNU skips a non-fixnum width field (&& FIXNUMP (AREF (entity, i))) — no penalty at all — whereascandidate_width.unwrap_or(FontWidth::Normal)scores an unknown-width candidate as if it were Normal. Only visible for non-Normal requests.- No
.min(127)on the width distance, unlike the weight field below it (selection.rs:67) and unlike GNU, which caps all three style fields. Only changes a winner when two candidates are both >127 units away. - A
metadata.width == Nonecandidate in the first test would pin the unknown-width treatment.
Follow-ups I'd like to pick up (happy to file these as an issue rather than leaving them only here):
- Give the frozen capture the width and key it on that width, then plumb the face width from
RealizedFaceFontSelection. - Treat unknown-width candidates the way GNU does — skip, not Normal.
- Cap the width distance at 127 like weight.
Fix is correct, lands in the right layer, and the tests are honest. Merging by rebase.
Why
With a font collection such as Iosevka (
Iosevka-Regular.ttc), Neomacs drew Greek, Cyrillic, accented Latin and punctuation such as—with the collection's Extended face (width 125), while ASCII used the normal-width Regular face. Text likeabc αβγ ἀλήθεια é —therefore looked visibly wider for every non-ASCII character. GNU Emacs uses the normal-width face for all of them.Character fallback scored candidates by the fontset spec's explicit width only. Fontset entries such as
(nil . "iso10646-1")leave the width unset, so every width tied and Fontconfig discovery order decided, and Iosevka lists its Extended face first. GNU'sfont_select_entity(src/font.c) fills an unset preferred width from the face (its font, else its:width), as it does for weight and slant, beforefont_sort_entities.Change
FontResolver::resolve_from_policyscores width against the captured effective width (the spec's width, or the requested face width when the spec has none), as it already does for weight and slant.explicit_widthfield from the captured character policy.Primary-font selection, fontset ordering and explicit spec widths are unchanged.
Out of scope: the layout caller still passes a normal width for every face (the primary-font path does the same), so a face with a non-normal
:widthor an Extended primary font is not yet matched by fallback. Fixing that means passing the face width through to both paths, which is a separate change.Verification
Regression test (fails on
mainat 7764ef7 with(Some(Expanded), 3)instead of(Some(Normal), 0), passes with this change):cargo nextest run -p neomacs-layout-engine --lib -E 'test(=font::resolver::tests::character_fallback_prefers_the_requested_face_width)'It lists an Extended collection face before a Normal one and checks that both a normal and an expanded requested width select the matching face, so the face width decides rather than discovery order.
explicit_fontset_width_beats_the_face_width_on_live_and_frozen_paths(9a52a76) checks the other direction: an explicit:width 'expandedin the fontset spec still beats a normal face width, on the live and frozen worker paths, fresh and cached. It fails if either path lets the face width override the spec width.Focused selection
cargo nextest run -p neomacs-layout-engine --lib -E 'test(~font::)'on Linux: 213 run, 209 passed. The same 4 tests fail identically on unmodifiedmain(212 run, 208 passed) on the test machine, because of missing or different system fonts (DejaVu Sans Mono not installed, static rather than variable Noto Sans Mono, and a host-dependent primary-face fixture).cargo fmt --checkandcargo clippy -p neomacs-layout-engine --lib --testsreport nothing new in the changed files.Isolated GUI comparison with the system
Iosevka-Regular.ttc(headless Weston,-Q,:family "Iosevka" :height 165, 22 px),font-atonabc αβγ ἀλήθεια é —:mainCompositor screenshots of that line show the same difference: on
mainthe non-ASCII glyphs are visibly wider than GNU's, and with this PR they match.This PR is agent-assisted.