Fraction typesetter — style override, cfrac struts, thinspace, numerator alignment#203
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…MTFraction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…le wrappers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kostub
left a comment
There was a problem hiding this comment.
Code Review — PR #203: Fraction typesetter (style override, cfrac struts, thinspace, numerator alignment)
Overall this is a clean, well-scoped implementation. The three main additions (style override, cfrac struts, thinspace wrapper) are logically separated and easy to follow. A few items worth addressing before merge:
Bug: initWithDisplays:range: runs recomputeDimensions before position is set, then the code silently overrides the result
In MTTypesetter.m, the thinspace-wrapping block:
result.position = CGPointMake(thinspace, 0); // ← set BEFORE init
MTMathListDisplay* wrapped = [[MTMathListDisplay alloc] initWithDisplays:@[result]
range:frac.indexRange];
// recomputeDimensions runs inside initWithDisplays and computes width = result.width + thinspace
// (only one side of padding, not two). Then:
wrapped.width = result.width + 2.0 * thinspace; // ← manual overrideinitWithDisplays:range: calls recomputeDimensions, which computes width = atom.width + atom.position.x = result.width + thinspace. The code then immediately overrides wrapped.width to result.width + 2*thinspace, so the final value is correct — but it relies on a fragile ordering where recomputeDimensions's width output is always wrong and always discarded.
A cleaner approach: set result.position after initWithDisplays:, so the wrapper's recomputeDimensions starts from {0,0} (natural position), and then set the position and override width in a single explicit pass. Or, simpler: just leave result.position = CGPointZero until after init:
MTMathListDisplay* wrapped = [[MTMathListDisplay alloc] initWithDisplays:@[result]
range:frac.indexRange];
result.position = CGPointMake(thinspace, 0); // set after init
wrapped.position = _currentPosition;
wrapped.ascent = result.ascent;
wrapped.descent = result.descent;
wrapped.width = result.width + 2.0 * thinspace;Nit: didOverrideStyle guard slightly over-narrow — style can be the same value but still benefit from explicit save/restore
The condition if (overrideStyle != _style) skips setting didOverrideStyle = YES when the override happens to equal the current style. This is correct for avoiding a no-op setStyle: call, but it also suppresses the setStyle:savedStyle restore, which doesn't matter since nothing changed. The logic is sound; this is just worth a brief comment so future readers don't wonder whether the != _style guard is load-bearing:
// Only mutate _style (and _styleFont) when the override actually differs.
// When they're equal, the frame metrics are already correct and no restore is needed.
if (overrideStyle != _style) {Test issue: testCfracThinspaceWrap computes muUnit from the wrong font size
// "Get the muUnit for the *display* style (cfrac forces Display)."
MTFont* styleFont = [font copyFontWithSize:font.fontSize]; // ← same as font
CGFloat thinspace = 3.0 * styleFont.mathTable.muUnit;The comment says "display style" but [font copyFontWithSize:font.fontSize] is identical to font — no style scaling is applied. This works accidentally because kMTLineStyleDisplay uses font.fontSize unchanged (see +getStyleSize:font:), so the muUnit is the same. The code is correct for the current default font size, but the comment is misleading. Consider removing the intermediate styleFont variable and just writing:
CGFloat thinspace = 3.0 * font.mathTable.muUnit;Minor: \cfrac + delimiter combination not defensively guarded
The isContinuedFraction thinspace-wrapping block runs regardless of whether addDelimitersToFractionDisplay: was already called (i.e., \cfrac with explicit leftDelimiter/rightDelimiter). Since the parser never produces that combination, this is not a current bug, but the code doesn't assert it. A brief NSAssert(!frac.leftDelimiter && !frac.rightDelimiter, @"cfrac should not have delimiters") inside the isContinuedFraction branch would make the assumption explicit.
Style observation: strut is applied before gap-adjustment, which is intentional but worth a comment
The strut floors on numeratorDisplay.ascent/descent and denominatorDisplay.ascent/descent are applied at lines 1260–1263, before numeratorShiftUp / denominatorShiftDown are computed and gap-adjusted. This means the strut inflates the operands and the gap logic then re-expands the shift values to maintain the required clearance from the bar. This matches AMSMath's behavior (the strut is a floor on the operand box, not on the shift), but it's non-obvious. A one-line comment would help:
// Apply cfrac strut floors to operand boxes *before* gap adjustment so that
// the bar-clearance logic in the next block sees the inflated extents.All four new tests are well-structured and cover the key invariants. The testDfracInlineStylePicksDisplayMetrics cross-comparison approach (render at the forced style, compare metrics) is particularly clean.
kostub
left a comment
There was a problem hiding this comment.
Test comment to verify posting works
There was a problem hiding this comment.
Code Review
This pull request implements support for fraction alignment and continued fractions (\cfrac). Key changes include adding a numeratorAlignment property to MTMathListDisplay, updating the typesetter to handle style overrides, and implementing struts and thinspace wrapping for continued fractions. A comprehensive suite of unit tests was added to verify these new features. Review feedback identifies that the \cfrac implementation should force DisplayStyle and avoid cramping the denominator to correctly match AMSMath behavior.
| MTLineStyle fractionStyle = self.fractionStyle; | ||
| MTMathListDisplay* numeratorDisplay = [MTTypesetter createLineForMathList:frac.numerator font:_font style:fractionStyle cramped:false]; | ||
| MTMathListDisplay* denominatorDisplay = [MTTypesetter createLineForMathList:frac.denominator font:_font style:fractionStyle cramped:true]; |
There was a problem hiding this comment.
AMSMath's \cfrac implementation requires that both the numerator and denominator are typeset in \displaystyle (regardless of the surrounding style) and that the denominator is not cramped. The current implementation uses the standard fraction style increment (which results in TextStyle for a DisplayStyle fraction) and always cramps the denominator.
To match AMSMath behavior, fractionStyle should be forced to kMTLineStyleDisplay and the cramped parameter for the denominator should be false when isContinuedFraction is true.
MTLineStyle fractionStyle = frac.isContinuedFraction ? kMTLineStyleDisplay : self.fractionStyle;
MTMathListDisplay* numeratorDisplay = [MTTypesetter createLineForMathList:frac.numerator font:_font style:fractionStyle cramped:false];
MTMathListDisplay* denominatorDisplay = [MTTypesetter createLineForMathList:frac.denominator font:_font style:fractionStyle cramped:!frac.isContinuedFraction];There was a problem hiding this comment.
Fixed in 37ef1f3 — \cfrac numerator/denominator now render in kMTLineStyleDisplay (not fractionStyle) and the denominator is passed cramped:false.
Per PR #202 review feedback: - MTMathList.h: note on isContinuedFraction and numeratorAlignment that appendLaTeXToString:/+mathListToString: do not persist these fields. - testDfrac: comment explaining why the round-trip emits operand-wrapped \displaystyle rather than \dfrac directly (partial-fidelity trade-off per LLD 3.3.5 / 5.1). - testCfracLeftAlign / testCfracRightAlign: assert the serialized output to pin the lossy contract. - testTfrac / testDbinom / testTbinom: XCTAssertNotNil(list) guards so a parse failure becomes a clean test failure instead of a nil-deref. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pesetter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lays Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ef51448 to
12257e4
Compare
- Force kMTLineStyleDisplay for cfrac numerator/denominator and pass cramped:false for the denominator (Gemini): AMSMath \cfrac requires both operands at full display style, not at fractionStyle (_style+1), and explicitly notes the denominator must not be cramped. - Reorder thinspace wrap so result.position is set after the wrapper's initWithDisplays runs: avoids relying on recomputeDimensions producing a wrong (one-sided) width that is then silently overridden. - Drop the no-op [font copyFontWithSize:font.fontSize] in testCfracThinspaceWrap; muUnit is taken from the base font. - Add clarifying comments on the (overrideStyle != _style) guard and on why cfrac struts are applied before the gap-clearance logic. All 217 tests still pass.
Summary
Wires the
styleOverride,isContinuedFraction, andnumeratorAlignmentfields (added in PR 1) into the typesetter. This is PR 2 of 4 in the AMSMath fractions + multi-integral stack.Plan:
docs/plans/2026-05-25-amsmath-fractions-and-multi-integrals.mdLLD:
docs/lld/2026-05-25-amsmath-fractions-and-multi-integrals.mdGoal
Render
\dfrac/\tfrac/\dbinom/\tbinomat their forced styles; render\cfracwith AMSMath strut equivalents (0.85em/0.35em) and 3mu thinspace padding; honor\cfrac[l|c|r]numerator alignment. Existing\frac/\binomrendering remains pixel-identical.Commits
[item 10]Add-lineStyleForFractionStyle:helper to MTTypesetter[item 11]AddnumeratorAlignmentto MTFractionDisplay positioning[item 12]HonorMTFraction.styleOverrideandnumeratorAlignmentin typesetter[item 13]Apply\cfracstrut floors (0.85em / 0.35em) to operand displays[item 14]Wrap\cfracwith 3mu thinspace padding via MTMathListDisplay[item 15]Typesetter test for\cfrac[l]/[r]numerator alignmentStack
Test plan
testDfracInlineStylePicksDisplayMetrics—\dfracin text mode produces display-style bar/gap metricstestCfracStrutAppliedToOperands— cfrac operand ascent/descent floored at 0.85em/0.35em; plain frac is nottestCfracThinspaceWrap— cfrac top display is MTMathListDisplay wrapper with width = inner + 2×thinspacetestCfracLeftAlignmentNumeratorOffset—\cfrac[l]/\cfrac[r]/\cfracnumerator x-offsets correct