Skip to content

Box family — phantom, smash, lap (items 1-3)#244

Merged
kostub merged 5 commits into
masterfrom
feature/box-spacing-pr1
Jun 28, 2026
Merged

Box family — phantom, smash, lap (items 1-3)#244
kostub merged 5 commits into
masterfrom
feature/box-spacing-pr1

Conversation

@kostub

@kostub kostub commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Goal

End-to-end support for the box family: \phantom, \hphantom, \vphantom, \mathstrut, \smash (+[t]/[b]), and \llap/\rlap/\clap (+ \math*lap aliases). Adds a single parameterized atom (MTMathBox, 5-flag matrix) + one display (MTMathBoxDisplay) backing the entire family.

This is PR 1 of the box-and-spacing layer. It is independent of PR 2 and based directly on master.

Commits

  • [item 1] Add MTMathBox atom, MTBoxHAlign, and kMTMathAtomBox plumbing
  • [item 2] Parse phantom/smash/lap commands into MTMathBox
  • [item 3] Render MTMathBox via MTMathBoxDisplay and typesetter box case

Implementation

  • Atom model: kMTMathAtomBox = 20 (script-capable), MTBoxHAlign enum, MTMathBox : MTMathAtom with innerList + keepWidth/keepHeight/keepDepth/drawChild/hAlign flags; lossy stringValue/appendLaTeXToString: per the flag matrix.
  • Parser: +boxCommands table + dispatch branch in -atomForCommand: (synthetic ( for \mathstrut, optional [t]/[b] for \smash).
  • Render: MTMathBoxDisplay : MTDisplay reporting flag-selected geometry and drawing/suppressing/offsetting its child; new kMTMathAtomBox case in MTTypesetter -createDisplayAtoms: (reclassifies to Ordinary, scripts supported).

Tests

  • MTMathListTest: 10/10. MTMathListBuilderTest: 146/146. MTTypesetterTest: 108/108.
  • Full suite: swift test 354/354, Xcode 313/313. Includes integration (vphantom drives delimiter size, scripts on box, rlap advance, negative-origin lap).

Deviations (documented by implementer)

  1. Added a public initWithType:value: declaration on MTMathBox so the guard test can compile (base designated initializer lives in a private category).
  2. Added an appendLaTeXToString: override calling self.stringValue (the base does not route through stringValue), mirroring MTMathStyle.

References

  • Plan: docs/plans/2026-06-28-box-spacing-layer.md (PR 1, Tasks 1–3)
  • LLD: docs/lld/2026-06-28-box-spacing-layer.md

🤖 Generated with Claude Code

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for the box family of LaTeX commands (such as phantom, hphantom, vphantom, mathstrut, smash, and various lap commands) by implementing a new MTMathBox atom type and its corresponding MTMathBoxDisplay rendering class. Feedback on the implementation suggests improving the robustness of the parser when handling optional arguments for smash (e.g., [t] or [b]) by raising a parsing error if the closing bracket is missing.

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.

Comment on lines +973 to +983
if (ch == '[') {
NSMutableString* opt = [NSMutableString string];
while ([self hasCharacters]) {
unichar c = [self getNextCharacter];
if (c == ']') { break; }
[opt appendString:[NSString stringWithCharacters:&c length:1]];
}
NSString* o = [opt stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceCharacterSet]];
if ([o isEqualToString:@"t"]) { box.keepHeight = NO; box.keepDepth = YES; }
else if ([o isEqualToString:@"b"]) { box.keepHeight = YES; box.keepDepth = NO; }
// any other value: ignore, leave smash-both flags (no crash).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

When parsing the optional argument [t] or [b] for \smash, if the closing bracket ] is missing (e.g., \smash[t at the end of the input), the parser currently continues silently without raising an error. To be consistent with other optional argument parsing (like \sqrt) and standard LaTeX syntax, we should verify that the closing bracket was successfully found and raise a MTParseErrorCharacterNotFound error if it is missing.

            if (ch == '[') {
                NSMutableString* opt = [NSMutableString string];
                BOOL foundClose = NO;
                while ([self hasCharacters]) {
                    unichar c = [self getNextCharacter];
                    if (c == ']') {
                        foundClose = YES;
                        break;
                    }
                    [opt appendString:[NSString stringWithCharacters:&c length:1]];
                }
                if (!foundClose) {
                    [self setError:MTParseErrorCharacterNotFound message:@"Expected character not found: ]"];
                    return nil;
                }
                NSString* o = [opt stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceCharacterSet]];
                if ([o isEqualToString:@"t"]) { box.keepHeight = NO; box.keepDepth = YES; }
                else if ([o isEqualToString:@"b"]) { box.keepHeight = YES; box.keepDepth = NO; }
                // any other value: ignore, leave smash-both flags (no crash).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 0a8395f. The smash optional-arg loop now tracks the closing bracket and raises MTParseErrorCharacterNotFound when it is missing, matching the \sqrt[…] precedent. Added \smash[t to the parse-error test table.

@kostub

kostub commented Jun 28, 2026

Copy link
Copy Markdown
Owner Author

Code review (orchestrator inline — automated review agent stalled twice on a stream watchdog, so this is a focused static review)

Verdict: solid, no blocking issues. Faithful to the LLD; reuses the colorbox/overline idioms correctly. Full suite already green per the implementer (Xcode 313/313, swift test 354/354). Did not approve or merge.

What's right

  • Atom placement kMTMathAtomBox = 20 (< kMTMathAtomBoundary) → script-capable, matching the LLD intent; testScriptOnBox exercises it.
  • Typesetter case mirrors overline/accent: flush _currentLine, addInterElementSpace:…currentType:kMTMathAtomOrdinary, then atom.type = kMTMathAtomOrdinary before any spacing lookup — so it never hits the getInterElementSpace default: assert. Pen advances by display.width (0 for vphantom/laps). break (not continue) lets the normal prevNode bookkeeping run with the reclassified type. Correct.
  • Display geometry keep* flags map straight to writable width/ascent/descent; phantom suppression is a clean early return after [super draw:]. Lap offsets (-w, -w/2, 0) match the hAlign matrix.
  • copyWithZone:/finalized deep-copy innerList and every flag; super preserves the subclass. Good.
  • Deviations are justified: the public initWithType:value: declaration is needed because the base designated initializer lives in a private category (else the guard test can't compile); the appendLaTeXToString: override is needed because the base does not route through stringValue — mirrors MTMathStyle.

Non-blocking nits

  1. draw: mutates child.position = CGPointZero before drawing the translated child. Harmless (the box exclusively owns its child) but it's a side-effect in draw: that the sibling displays avoid — they keep subdisplay positions immutable and rely on CTM translation alone. Consider translating by position + offset and leaving child.position untouched, or setting it once at init.
  2. \smash[ with no closing ] consumes the rest of the input into the optional-arg buffer, then buildInternal: sees EOF and wraps an empty inner. No crash (fine), but silently swallows trailing content. A bounded peek or a ]-not-found fallback (un-look the buffer) would be friendlier. Edge-case only.
  3. \mathstrut synthesizes ( via atomForCharacter:'(' (an Ordinary atom) rather than an open-delimiter atom. Height is correct at normal size, so this matches \vphantom{(} in practice; just noting it's a plain glyph, not a delimiter.

None of these block merge.

kostub and others added 2 commits June 28, 2026 23:59
\smash[t / \smash[b without a closing ']' silently recovered instead of
erroring, unlike \sqrt[…] which raises MTParseErrorCharacterNotFound.
Track the closing bracket and fail loud when it is absent.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
… mutating in draw:

draw: translated the CTM and reset child.position = CGPointZero on every
frame, a draw-time side-effect that sibling displays avoid. Follow the
MTRadicalDisplay / MTInnerDisplay pattern: override setPosition: to push
the offset-adjusted absolute position to the child once, leaving draw:
to simply draw it. No behavior change (354/354 tests pass).

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@kostub kostub merged commit 9f53483 into master Jun 28, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant