Skip to content

Behavior-preserving refactor — remove dead row-style state, share limit-gap math #218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kostub merged 2 commits into from
Jun 7, 2026
+45 −26
Merged

Behavior-preserving refactor — remove dead row-style state, share limit-gap math #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd235f1
[item 1] Remove dead row-style state; derive stack row style at types…
kostub Jun 7, 2026
25206e9
[item 2] Extract upperLimitGapFor:/lowerLimitGapFor: limit-gap helpers
kostub Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions iosMath/lib/MTMathList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,17 +499,13 @@ typedef NS_ENUM(NSUInteger, MTMathStackConstructionKind) {

// MathList fields (kind == kMTMathStackConstructionMathList)
@property (nonatomic, readonly, nullable) MTMathList* list;
@property (nonatomic, readonly) MTLineStyle listStyle;
@property (nonatomic, readonly) BOOL listCramped;

// Rule fields (kind == kMTMathStackConstructionRule)
/// Rule thickness in points. 0 means use the font's default.
@property (nonatomic, readonly) CGFloat ruleThickness;

+ (instancetype)extensibleWithGlyph:(NSString*)glyph;
+ (instancetype)mathListWithList:(MTMathList*)list
style:(MTLineStyle)style
cramped:(BOOL)cramped;
+ (instancetype)mathListWithList:(MTMathList*)list;
+ (instancetype)ruleWithThickness:(CGFloat)thickness;

@end
Expand Down
16 changes: 2 additions & 14 deletions iosMath/lib/MTMathList.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,15 +1173,11 @@ + (instancetype)extensibleWithGlyph:(NSString*)glyph
}

+ (instancetype)mathListWithList:(MTMathList*)list
style:(MTLineStyle)style
cramped:(BOOL)cramped
{
NSParameterAssert(list);
MTMathStackConstruction* c = [[self alloc] init];
c->_kind = kMTMathStackConstructionMathList;
c->_list = [list copy];
c->_listStyle = style;
c->_listCramped = cramped;
return c;
}

Expand All @@ -1199,8 +1195,6 @@ - (id)copyWithZone:(NSZone*)zone
copy->_kind = _kind;
copy->_glyph = [_glyph copyWithZone:zone];
copy->_list = [_list copyWithZone:zone];
copy->_listStyle = _listStyle;
copy->_listCramped = _listCramped;
copy->_ruleThickness = _ruleThickness;
return copy;
}
Expand Down Expand Up @@ -1245,16 +1239,10 @@ - (instancetype)finalized
MTMathStack* newStack = [super finalized];
newStack.innerList = newStack.innerList.finalized;
if (newStack.over && newStack.over.kind == kMTMathStackConstructionMathList) {
MTMathStackConstruction* overConst = newStack.over;
newStack.over = [MTMathStackConstruction mathListWithList:overConst.list.finalized
style:overConst.listStyle
cramped:overConst.listCramped];
newStack.over = [MTMathStackConstruction mathListWithList:newStack.over.list.finalized];
}
if (newStack.under && newStack.under.kind == kMTMathStackConstructionMathList) {
MTMathStackConstruction* underConst = newStack.under;
newStack.under = [MTMathStackConstruction mathListWithList:underConst.list.finalized
style:underConst.listStyle
cramped:underConst.listCramped];
newStack.under = [MTMathStackConstruction mathListWithList:newStack.under.list.finalized];
}
return newStack;
}
Expand Down
25 changes: 18 additions & 7 deletions iosMath/render/internal/MTTypesetter.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1707,6 +1707,18 @@ - (MTDisplay*) makeLargeOp:(MTLargeOperator*) op
}
}

- (CGFloat) upperLimitGapFor:(MTDisplay*)limit
{
return MAX(_styleFont.mathTable.upperLimitGapMin,
_styleFont.mathTable.upperLimitBaselineRiseMin - limit.descent);
}
Comment on lines +1710 to +1714
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure robust defensive programming and prevent potential issues if this helper is reused in the future, add a parameter assertion to verify that limit is not nil before accessing its properties.

- (CGFloat) upperLimitGapFor:(MTDisplay*)limit
{
    NSParameterAssert(limit);
    return MAX(_styleFont.mathTable.upperLimitGapMin,
               _styleFont.mathTable.upperLimitBaselineRiseMin - limit.descent);
}

Copy link
Copy Markdown
Owner Author

@kostub kostub Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining. Both call sites guard with if (superScript) / if (subScript) immediately above (MTTypesetter.m:1740-1745), so nil can't reach the helper today. And even if it did, sending descent/ascent to a nil receiver returns 0 in Obj-C, so MAX(upperLimitGapMin, upperLimitBaselineRiseMin - 0) would yield a valid CGFloat — not a crash. NSParameterAssert is also a no-op in release builds (NS_BLOCK_ASSERTIONS), so it would only signal in debug. Helper is private to MTTypesetter; no future caller to defend against (YAGNI).


- (CGFloat) lowerLimitGapFor:(MTDisplay*)limit
{
return MAX(_styleFont.mathTable.lowerLimitGapMin,
_styleFont.mathTable.lowerLimitBaselineDropMin - limit.ascent);
}
Comment on lines +1716 to +1720
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure robust defensive programming and prevent potential issues if this helper is reused in the future, add a parameter assertion to verify that limit is not nil before accessing its properties.

- (CGFloat) lowerLimitGapFor:(MTDisplay*)limit
{
    NSParameterAssert(limit);
    return MAX(_styleFont.mathTable.lowerLimitGapMin,
               _styleFont.mathTable.lowerLimitBaselineDropMin - limit.ascent);
}

Copy link
Copy Markdown
Owner Author

@kostub kostub Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining for the same reason as the upper-limit thread: caller guards with if (subScript) (MTTypesetter.m:1743), Obj-C nil-messaging would return a safe 0 anyway, and NSParameterAssert is a release-build no-op. Private helper, no speculative callers. YAGNI.


- (MTDisplay*) addLimitsToDisplay:(MTDisplay*) display forOperator:(MTLargeOperator*) op delta:(CGFloat)delta
{
// If there is no subscript or superscript, just return the current display
Expand All @@ -1726,12 +1738,10 @@ - (MTDisplay*) addLimitsToDisplay:(MTDisplay*) display forOperator:(MTLargeOpera
NSAssert(superScript || subScript, @"Atleast one of superscript or subscript should have been present.");
MTLargeOpLimitsDisplay* opsDisplay = [[MTLargeOpLimitsDisplay alloc] initWithNucleus:display upperLimit:superScript lowerLimit:subScript limitShift:delta/2 extraPadding:0];
if (superScript) {
CGFloat upperLimitGap = MAX(_styleFont.mathTable.upperLimitGapMin, _styleFont.mathTable.upperLimitBaselineRiseMin - superScript.descent);
opsDisplay.upperLimitGap = upperLimitGap;
opsDisplay.upperLimitGap = [self upperLimitGapFor:superScript];
}
if (subScript) {
CGFloat lowerLimitGap = MAX(_styleFont.mathTable.lowerLimitGapMin, _styleFont.mathTable.lowerLimitBaselineDropMin - subScript.ascent);
opsDisplay.lowerLimitGap = lowerLimitGap;
opsDisplay.lowerLimitGap = [self lowerLimitGapFor:subScript];
}
opsDisplay.position = _currentPosition;
opsDisplay.range = op.indexRange;
Expand Down Expand Up @@ -2042,9 +2052,10 @@ - (MTDisplay*) buildStackConstruction:(MTMathStackConstruction*)construction

case kMTMathStackConstructionMathList: {
return [MTTypesetter createLineForMathList:construction.list
font:_font
style:construction.listStyle
cramped:construction.listCramped];
font:_font
style:self.scriptStyle
cramped:(role == kMTStackRoleOver ? self.superScriptCramped
: self.subscriptCramped)];
}

case kMTMathStackConstructionRule:
Expand Down
24 changes: 24 additions & 0 deletions iosMathTests/MTTypesetterTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2598,4 +2598,28 @@ - (void) testTextInMixedLine {
XCTAssertEqualWithAccuracy(text.position.y, math.position.y, 0.001);
}

- (void)testStackMathListRowRendersAtScriptStyle
{
// 6.2-a: a MathList over-row is typeset at script style derived from the live
// style, so for the same glyph it is smaller than the display-style base.
MTMathList* baseList = [MTMathList new];
[baseList addAtom:[MTMathAtomFactory atomForCharacter:'X']];
MTMathList* overList = [MTMathList new];
[overList addAtom:[MTMathAtomFactory atomForCharacter:'X']];

MTMathStack* stack = [MTMathStack new];
stack.innerList = baseList;
stack.over = [MTMathStackConstruction mathListWithList:overList];

MTMathList* list = [MTMathList new];
[list addAtom:stack];

MTMathListDisplay* display =
[MTTypesetter createLineForMathList:list font:self.font style:kMTLineStyleDisplay];
XCTAssertEqual(display.subDisplays.count, 1u);
MTStackDisplay* sd = (MTStackDisplay*)display.subDisplays[0];
XCTAssertTrue([sd.over isKindOfClass:[MTMathListDisplay class]]);
XCTAssertLessThan(sd.over.ascent, sd.base.ascent);
}

@end
Loading