Route \mathit through the companion face - #271
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesMath italic routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MTTypesetter
participant RenderedAtom
participant CoreText
MTTypesetter->>RenderedAtom: preprocess routable math-italic characters
MTTypesetter->>RenderedAtom: assign regular and companion font ranges
RenderedAtom->>CoreText: shape mixed font runs
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
kostub
left a comment
kostub
left a comment
There was a problem hiding this comment.
Reviewed the diff of this PR only, against LLD §3 / §3.3 A/C/D/G. I verified the two load-bearing claims empirically on the PR head (swift test: 466 pass), plus a sweep asserting no character in any MTCTLineDisplay is left without kCTFontAttributeName across 22 inputs (\dfrac/\tfrac style overrides, \left…\right, matrices, \color/\colorbox, \text, nested scripts, \hat{\mathit{f}}, \sqrt[\mathit{n}]{…}). No unstamped ranges. The UTF-16 run arithmetic in applyMathitFontToRoutableCharactersInRange: is correct — no surrogate code unit (U+D800–U+DFFF) falls in the routable domain, so \mathit{a\theta b} closes runs at {0,1} and {3,1} exactly as the test asserts.
Two things.
1. NSAssert(atom.type == kMTMathAtomOrdinary, …) fires on a documented public API — and the fix is shorter than the assert (MTTypesetter.m:1079)
MTMathAtomFactory addLatexSymbol:value: is public and documented in the header. Registering any non-Ordinary atom whose value contains a Latin letter, digit or capital Greek and then using it under \mathit aborts every Debug build:
[MTMathAtomFactory addLatexSymbol:@"bmod"
value:[MTMathAtom atomWithType:kMTMathAtomBinaryOperator value:@"mod"]];
// \mathit{a \bmod b}Verified on PR head with [MTMathAtom atomWithType:kMTMathAtomRelation value:@"R"] and a \mathit{\myrel} b:
MTTypesetter.m:1079: Routable character in non-Ordinary atom: Relation: R (NSInternalInconsistencyException)
LLD §3.3 C justifies the assert with "no LaTeX input produces a non-Ordinary atom with a routable nucleus". That is true of the built-in table — I decoded all 290 entries in supportedLatexSymbols and every routable-nucleus atom is kMTMathAtomVariable, which preprocessMathList: rewrites to Ordinary — but addLatexSymbol: lets a caller add one, and \bmod is a plausible thing to add given iosMath doesn't ship it.
Making it the third condition instead of an assert is a net deletion — the forAtom: parameter exists only to feed the assert:
// at the append site
if (atom.fontStyle == kMTFontStyleItalic && atom.type == kMTMathAtomOrdinary) {
[self applyMathitFontToRoutableCharactersInRange:appendedRange];
}and drop forAtom:(MTMathAtom*) atom from the signature and line 1079 from the loop. Four lines and a parameter out, and it's also the behaviour TeX gives: \mathit selects a family for class-7 mathchars, so a class-3 relation should keep the math font rather than crash.
2. The stated invariant behind deleting the addDisplayLine stamp is wrong; the real one is different (no behaviour bug)
The PR body and LLD §3.3 C/D justify the deletion with "_styleFont is assigned once per typesetter and never reassigned [verified: MTTypesetter.m:593 is its only assignment]". That line is the body of - (void) setStyle:(MTLineStyle) style (MTTypesetter.m:608-611 here), which makeFraction calls for \dfrac/\tfrac style overrides and restores afterwards — so _styleFont is reassigned mid-typesetter.
The deletion is still safe, for a different reason: the fraction case flushes _currentLine before makeFraction (MTTypesetter.m:827-830), and setStyle: has no other caller, so _styleFont is constant for the lifetime of any single _currentLine. My \dfrac{\mathit{ab}}{\mathit{cd}} / \tfrac{\mathit{ab}}{x} sweep cases confirm it. Worth correcting the wording, since the deletion's whole safety argument rests on it and the next person to touch this will re-derive from what's written.
The "one append site" half of the claim is exact: MTTypesetter.m:1014 is the only appendAttributedString: into _currentLine.
Everything else in the diff looks right to me. §3.3 A, C, D and G land as designed, the exclusion set matches the LLD's contract table, and the \mathit{f}i / \mathit{fi} pair is a good way to pin that the attribution actually partitions CoreText shaping.
d4b2927 to
894f8de
Compare
a00f775 to
87ac799
Compare
styleCharacter's italic case no longer remaps Latin letters, digits, and capital Greek into the math-italic Unicode block. Those are TeX's class-7 mathchars (LLD 2026-07-27 mathit-text-italic-routing.md §2.1) and now stay plain code points so they can be drawn from a text-italic companion face in the next item; getItalicized still handles lowercase Greek and Greek symbol variants, which the companion face cannot serve (LLD §2.3).
Every nucleus append now stamps _styleFont.ctFont over its own range, then applyMathitFontToRoutableCharactersInRange:forAtom: overrides maximal runs of routable characters within an Italic atom with _styleFont.mathitCTFont. The whole-line font stamp in addDisplayLine is deleted since every character already carries a font by the time flush runs, so nothing can silently overwrite a \mathit range (LLD §3.3 C-D).
Integration tests for items 5-6: companion advances for f and 1, unrouted
categories left byte-identical to plain, companion pair-kerning on AV,
the f_i ligature shaping to one glyph, and no ligature crossing a
\mathit{f}i style boundary. Written after the implementation per plan;
all pass immediately.
87ac799 to
3604aac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@iosMath/render/internal/MTTypesetter.m`:
- Around line 385-395: Update getItalicized so its capital-Greek handling
explicitly excludes U+03A2 before applying the math-italic offset, matching
MTIsMathItalicRoutable’s existing exclusion. Preserve the current mapping for
assigned capital Greek letters and leave U+03A2 unchanged or rejected according
to the existing fallback behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57e6a62b-2292-45a6-b5a4-57f93cb5184e
📒 Files selected for processing (2)
iosMath/render/internal/MTTypesetter.miosMathTests/MTTypesetterTest.m
+addLatexSymbol:value: is public, so a caller can register a non-Ordinary atom whose nucleus is routable — e.g. a BinaryOperator with value "mod". Under \mathit that reached an NSAssert and aborted every Debug build. Make the atom type a third routing condition instead. This is also what TeX gives: \mathit selects a family for class-7 mathchars, so a class-3 relation keeps the math font. changeFont/styleCharacter run only for variables and numbers, so such an atom's nucleus is not remapped either and it renders exactly as it does without \mathit. The forAtom: parameter existed only to feed the assert; drop it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
|
Both points actioned in a33c04b. 1. The assert on a public API. Confirmed and fixed as suggested — atom type is now the third routing condition, the assert and the One thing worth adding: the fix is behaviour-preserving, not just crash-preserving. 2. The The §9 risk row also needed rewriting rather than just correcting: it described the atom-type check as a Debug-only assert that does not fire in Release. That is now a real condition in both configurations, so the residual risk it named is gone.
CodeRabbit’s U+03A2 comment is declined on the thread — the offset arithmetic is correct by Unicode’s design (U+1D6F3 is MATHEMATICAL ITALIC CAPITAL THETA SYMBOL, occupying exactly that slot), and its suggested fix would shift Sigma through Omega by one. |
MTIsMathItalicRoutable excluded U+03A2, the unassigned code point in the Greek capital block, so it fell through to getItalicized and was mapped to U+1D6F3 — MATHEMATICAL ITALIC CAPITAL THETA SYMBOL, a real glyph every bundled math font has. An unassigned input rendered as a different, real letter. Route it with the rest instead. The companion has no glyph for it, so it draws .notdef, which is a visible failure and matches what the character already gets without \mathit: getDefaultStyle returns capital Greek unchanged and no bundled math font carries U+03A2 either. The exclusion was also guarding an input the parser cannot produce -- atomForCharacter: returns nil for every non-ASCII literal, so U+03A2 reaches a math list only via direct MTMathAtom construction. Lets the function reuse IS_CAPITAL_GREEK instead of open-coding the range. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
1 participant
Plan:
docs/plans/2026-08-04-mathit-text-italic-routing.md(PR 3 of 3, items 5-7)LLD:
docs/lld/2026-07-27-mathit-text-italic-routing.mdGoal
\mathitstops being a no-op. Routable characters — A-Z, a-z, 0-9, and capital Greek minus the unassigned U+03A2 — keep their plain code points and are drawn frommathitCTFontvia per-range font attributes, matching pdflatex's cmti10 behaviour. Everything else inside\mathitis unchanged.\mathit{f}was byte-identical tof(U+1D453, math font, 0.490 em); it now draws U+0066 from the companion at 0.307 em, against cmti10's 0.30667 em.Three things worth review attention:
(,+,), lowercase Greek,\sinand the symbol families are deliberately excluded — iosMath already agrees with pdflatex on all of them, and italicising them is MathJax's behaviour, not LaTeX's.addDisplayLineis deleted rather than made range-aware: every character now gets its font at append time, so no later pass can silently overwrite a routed range. This is exact, not approximate — there is one append site into_currentLineand_styleFontis assigned once (LLD §3.3 C/D).\mathit{…}group into one atom, so\mathit{a\theta b}is a single atom whose nucleus mixes routable and non-routable characters — theta must stay in the math font, the only font that has it.Italic correction is deliberately not addressed here; it is LLD 2's subject.
\mathit{f}is now correct in face and code point but still short by the correction, with a small documented lookup drift (LLD §3.3 E).Commits
[item 5] Keep routable characters plain ASCII under \mathit[item 6] Draw \mathit routable runs with the companion face[item 7] Pin \mathit geometry and shaping with integration testsFull
swift testgreen (466 tests).Stack
\mathitcompanion font onMTFontSummary by CodeRabbit
\mathitrendering for Latin letters, digits, and supported Greek characters.