Never fuse atoms across a font-style change - #269
Conversation
fuse: keeps the first atom's fontStyle, so 1\mathit{2} silently dropped
the italic and \mathit{1}2 spread it onto a plain digit. Never fuse
across a font-style change; uniform-style runs still fuse.
f\mathit{x} fused into one Default atom and lost the italic on x. Rule
14 now merges ordinary atoms only within one font style, matching the
site A guard from item 1.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change prevents number and ordinary-atom fusion across different font styles. Uniformly styled atoms continue to fuse. Regression tests cover mixed, default, italic, bold, and unstyled inputs. ChangesFont-style-aware atom fusion
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
kostub
left a comment
There was a problem hiding this comment.
Code review — PR 1 of 3 (fontStyle guard at both fusion sites)
No blocking findings. The diff is two extra conjuncts and their tests; I could not find a defect, and there is no machinery here to collapse.
What I verified
- Both guards are load-bearing; neither is redundant. Site A fuses before
changeFontruns, so with only the site B guard1\mathbf{2}still arrives atpreprocessMathList:as a singleDefaultNumber and the bold2is lost. With only the site A guard, the two resulting Ordinary atoms re-fuse at site B. Reverting either source hunk failstestPreprocessDoesNotFuseAcrossFontStyles. - The new tests genuinely fail without the change. Reverted
MTMathList.m+MTTypesetter.mtomasteron top of the PR's test files:testFinalizedDoesNotFuseDigitsAcrossFontStylesandtestPreprocessDoesNotFuseAcrossFontStylesfail; the three "still fuses" / "unchanged" methods pass, so they work as regression pins. grepconfirmsfuse:has exactly two call sites (MTMathList.m:1712,MTTypesetter.m:557) — no third merge path is left unguarded.- Geometrically neutral, including for the newly-split mixed-style case. Splitting an Ord run into two Ord atoms adds nothing:
getInterElementSpaces()[0][0]iskMTSpaceNone, socreateDisplayAtomsadds no kern, and both nuclei are appended to the same_currentLinebefore a singleaddDisplayLine, so theCTLinestring is byte-identical to the fused one. The superscript path is also unaffected — the italic correction is taken from the last character of the nucleus, which is the same character either way, and!prevNode.subScript && !prevNode.superScriptalready blocked fusing into a scripted atom. _currentAtoms/ index-range bookkeeping is unchanged, because a fused atom already contributed its_fusedAtomscopies rather than itself.- The
\mathbfdigit fix does not hit a font-coverage hole. U+1D7CE/U+1D7D0 are present in all eight bundled.otfs (checked viafontToolscmap), so the newly-preserved bold digit resolves in every bundled font. - Full
swift testatfdd18ab: 451 tests, 0 failures.
Notes, not requests
- The test-only
@interface MTTypesetter (FusionTesting)category is the right call over adding anMTTypesetter+Internal.h: it exposes nothing outside the test binary and costs three lines. \mathsfdigits map to U+1D7E2, whichfiramath.otflacks (the only bundled font that does). This PR widens the reach of that pre-existing gap from\mathsf{2}to1\mathsf{2}, but the gap and its CoreText-substitution behaviour are unchanged by this diff — flagging only so it isn't a surprise later.
Not approving — leaving the merge call to you.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Plan:
docs/plans/2026-08-04-mathit-text-italic-routing.md(PR 1 of 3, items 1-2)LLD:
docs/lld/2026-07-27-mathit-text-italic-routing.mdGoal
Both atom-merge sites decline to fuse atoms whose
fontStylediffers, so a style set on part of a run survives to the typesetter.fuse:keeps the first atom's properties, including itsfontStyle, so an unguarded merge either drops a style or spreads it onto an unstyled atom. Neither site checked style, and they cover disjoint inputs: site A merges onlyNumberatoms, site B merges Ordinary atoms afterpreprocessMathList:has rewritten variables and numbers.1\mathit{2}is fixed only by A,f\mathit{x}only by B.This independently fixes a live rendering bug:
1\mathbf{2}renders its2non-bold today, because site A's merge discards the bold beforegetBoldcan map the digit to U+1D7CE (LLD §2.6). Geometrically neutral for uniform-style input.Commits
[item 1] Guard digit fusion on fontStyle in MTMathList.finalized[item 2] Guard Rule-14 fusion on fontStyle in preprocessMathListFull
swift testgreen at each commit (451 tests).Summary by CodeRabbit