Typesetter cell-style gap scaling (foundational correctness fix)#245
Typesetter cell-style gap scaling (foundational correctness fix)#245kostub wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures that table inter-column and inter-row spacing are measured using the cell's actual rendering style rather than the surrounding style, fixing layout issues for environments like matrices. It also adds corresponding unit tests. The review feedback suggests optimizing performance by avoiding redundant font copies: first, by reusing the existing style font when the cell style matches the outer style, and second, by directly calculating the font size instead of copying the entire font object when only the size is needed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| MTLineStyle cellStyle = [self cellStyleForTable:table]; | ||
| MTFont* cellStyleFont = [_font copyFontWithSize:[[self class] getStyleSize:cellStyle font:_font]]; |
There was a problem hiding this comment.
For tables where no style atom is injected (such as aligned, gather, eqnarray, or alignedat), cellStyle is identical to _style. In these cases, copying the font is redundant and introduces unnecessary overhead. Reusing _styleFont directly when cellStyle == _style avoids this overhead.
MTLineStyle cellStyle = [self cellStyleForTable:table];
MTFont* cellStyleFont = (cellStyle == _style) ? _styleFont : [_font copyFontWithSize:[[self class] getStyleSize:cellStyle font:_font]];| MTFont* cellStyleFont = [_font copyFontWithSize:[[self class] getStyleSize:cellStyle font:_font]]; | ||
| CGFloat openup = table.interRowAdditionalSpacing * kJotMultiplier * cellStyleFont.fontSize; |
There was a problem hiding this comment.
In positionRows:forTable:, cellStyleFont is only created to access its fontSize property. Copying the entire font is highly inefficient and redundant here. We can directly calculate the font size using [[self class] getStyleSize:cellStyle font:_font] and avoid the font copy entirely.
CGFloat cellStyleFontSize = [[self class] getStyleSize:cellStyle font:_font];
CGFloat openup = table.interRowAdditionalSpacing * kJotMultiplier * cellStyleFontSize;
kostub
left a comment
There was a problem hiding this comment.
Code Review — PR 1: Typesetter cell-style gap scaling (items 1-2)
Assessment: Ready to merge after considering the minor items below. No blocking issues.
The change faithfully implements LLD §3.3 (Renderer) and the plan. I verified every load-bearing symbol against the source (kMTMathAtomStyle at MTMathList.h:84, MTMathStyle.style readonly MTLineStyle at :466, +getStyleSize:font: at MTTypesetter.m:571, copyFontWithSize: at MTFont.h:23, MTFont+Internal.h importing mathTable into the test target). The cellStyleFont construction is byte-for-byte the same pattern as the existing -setStyle: at MTTypesetter.m:589, so it composes correctly.
Strengths
- No-op invariant holds by construction. When no style atom is injected,
cellStyleForTable:returns_style, socellStyleFontis built identically to_styleFont— genuinely a no-op foraligned/gather/eqnarray/alignedatand formatrix/casesat top level (Text == Display/Text size). This is the central correctness claim and it checks out. - Font-exact scaling, no magic factor.
muUnit == fontSize/18, so a Script cell style yields exactlyscriptScaleDown× the gap from the font's own metric. Matches the LLD's amsmath/KaTeX rationale. - Both tests are behavioral and would actually catch a regression.
testMatrixColumnGapTracksCellStyleasserts Display==Script width (the fix),testEqnarrayColumnGapUnchangedByCellStyleasserts the no-style path scales uniformly (the guard). The two together lock both the corrected and the unchanged path. - Clear, accurate doc comment on the helper explaining why the gap must track cell style.
Minor (non-blocking)
-
Duplicated
cellStyleFontcomputation.-makeRowWithColumns:and-positionRows:each recomputecellStyle+cellStyleFontindependently (and-makeRowWithColumns:is called once per row, so the font is re-copied per row). For a small table this is negligible, but if you want to tidy:cellStyleForTable:could be resolved once in the-makeTable:caller and threaded down, or memoized. Not worth blocking PR 1; flagging in case PR 2 (which addssmallmatrix, the first Script-cell env) makes this path hotter. -
cellStyleForTable:returns on the first style-bearing cell without checking uniformity. This is correct today because the factory injects the style atom into every cell uniformly. Worth a one-line comment noting the "all cells share the injected style" precondition, so a future env that injects a style into only some cells doesn't silently mis-measure the whole table's gap. (The existing comment explains the mechanism but not this assumption.) -
testEqnarrayColumnGapUnchangedByCellStyleuses tolerance1.0on a width comparison, vs0.001in the matrix test. That's a fairly loose absolute tolerance for a width in points — likely chosen to absorb per-glyph subpixel scaling differences between the two independently-typeset tables rather than isolating just the gap term. It's defensible (the test's job is "no gross mis-scaling"), but the asymmetry with the sibling test is worth a brief inline note so a future reader doesn't tighten it and get flakiness.
Scope / plan adherence
- Diff is exactly the two files the plan names, +70/-2, matching the plan's task boundaries.
- The deliberate scope boundary —
openupmoves to cell style whilebaselineSkip/lineSkip/lineSkipLimitstay at_styleFont(MTTypesetter.m:2265-2267) — is implemented as specified in LLD §3.3. Correctly a no-op for all current envs (nonzero row spacing only occurs in no-style envs). - No new public API, no model changes — consistent with the LLD's "no new
MTMathTablefields."
Note: CI (Build & Test) was still pending at review time — confirm the 270-test suite goes green before merge.
Goal
Measure table inter-column and inter-row gaps in the cell-content style instead of the surrounding style. This is a no-op for every existing environment in its normal top-level context; the single behavioral change is a correction: a styled table (
matrix/cases) nested in a smaller style now renders its gap at the cell style instead ofscriptScaleDown× too tight.This is PR 1 of a 3-PR stack that adds the
smallmatrix,gathered, andalignedatamsmath environments. It stands alone as a reviewable correctness fix on the shared table-layout paths; PR 2/3 ride on it.Design docs
docs/plans/2026-06-30-smallmatrix-gathered-alignedat.mddocs/lld/2026-06-30-smallmatrix-gathered-alignedat.md(§3.3 Renderer, §6, §7)Commits
[item 1] Measure table gaps at the cell-content style— adds-cellStyleForTable:helper; scales the column-gap term in-makeRowWithColumns:forTable:columnWidths:and theopenupterm in-positionRows:forTable:to the cell style. New testtestMatrixColumnGapTracksCellStyle.[item 2] Add eqnarray no-op guard for cell-style gap scaling— regression testtestEqnarrayColumnGapUnchangedByCellStyleconfirming the no-style path is unchanged.Tests
Full
MTTypesetterTest/MTMathListBuilderTest/MTMathListTestsuites pass (270 tests) — the fix is a no-op at top level for all existing environments.🤖 Generated with Claude Code