Skip to content

Add LaTeX array environment (PR 3/3): renderer — rules + shared column offsets#254

Open
kostub wants to merge 4 commits into
masterfrom
feature/array-environment-pr3
Open

Add LaTeX array environment (PR 3/3): renderer — rules + shared column offsets#254
kostub wants to merge 4 commits into
masterfrom
feature/array-environment-pr3

Conversation

@kostub

@kostub kostub commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Goal

Render array vertical/horizontal rules. Adds a lightweight MTRuleDisplay leaf; factors cell x-advance into one -columnOffsetsForTable: helper so cells and vertical rules cannot drift; emits rule displays spanning a shared frame and folds them into table bounds so \left…\right sizes correctly around the rule-inclusive table. The matrix / every-existing-env path is byte-for-byte unchanged (empty verticalLines/horizontalLines ⇒ identical running sum, empty rule set).

Commits

  • [item 9] Add MTRuleDisplay leaf for array rules
  • [item 10] Factor shared column offsets; keep matrix path identical
  • [item 11] Emit array rule displays; fold into table bounds
  • [item 12] Integration: array in delimiters + matrix non-regression

Verification

  • Full suite green: swift test → 402/402.
  • Regression guard: existing matrix/table layout tests (testMathTable, testMatrixColumnGapTracksCellStyle, testMatrixRowSpacingTracksCellStyle, testSmallMatrixColumnGap, testSmallMatrixCompactVsMatrix, testGatheredMatchesGather, testAlignedatLayout) unchanged after the item-10 offsets refactor.
  • testMatrixLayoutUnchangedByArraySupport asserts a plain matrix emits no MTRuleDisplay and exactly two row displays.

Stack

  1. Add LaTeX array environment (PR 1/3): model fields + factory constructor #251 — PR 1/3: model fields + array factory constructor (base: master)
  2. Add LaTeX array environment (PR 2/3): parser + serialization #253 — PR 2/3: parser + serialization (base: feature/array-environment-pr1)
  3. This PR — PR 3/3: renderer — rules + shared column offsets (base: feature/array-environment-pr2)

References

  • Plan: docs/plans/2026-07-03-array-environment.md (PR 3, items 9-12)
  • LLD: docs/lld/2026-06-29-array-environment.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 rendering horizontal and vertical rules in math arrays (such as \hline and | column separators) by implementing a new MTRuleDisplay class. It updates the typesetter to calculate column offsets with padding/rules and position these rules correctly within tables. Unit tests are also added to verify rule metrics, offsets, and layout regression. The review feedback suggests adding a safety check for the outWidth pointer in columnOffsetsForTable:... to prevent potential null pointer dereferences.

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.

x += columnWidths[i];
}
}
*outWidth = x;

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

To prevent potential null pointer dereference crashes, it is safer to guard the assignment to outWidth with a NULL check before dereferencing it.

if (outWidth) {
    *outWidth = x;
}

@kostub kostub force-pushed the feature/array-environment-pr2 branch from 7570801 to 843a1c8 Compare July 6, 2026 11:08
@kostub kostub force-pushed the feature/array-environment-pr3 branch from ca36098 to 9320825 Compare July 6, 2026 11:08
@kostub kostub force-pushed the feature/array-environment-pr2 branch from 843a1c8 to 7972159 Compare July 9, 2026 16:14
@kostub kostub force-pushed the feature/array-environment-pr3 branch from 9320825 to cded87a Compare July 9, 2026 16:16
@kostub kostub force-pushed the feature/array-environment-pr3 branch from cded87a to c573116 Compare July 9, 2026 16:48
@kostub kostub changed the base branch from feature/array-environment-pr2 to master July 9, 2026 16:48
@kostub

kostub commented Jul 9, 2026

Copy link
Copy Markdown
Owner Author

Code Review — PR 3/3: Array environment renderer (rules + shared column offsets)

Assessment: Ready to merge with fixes (minor). The non-regression running-sum algebra was verified by hand, the matrix fast path confirmed inert, and fold-into-bounds delimiter sizing is sound. Full suite: 406/406 green.

Strengths

  • Non-regression claim holds under algebraic verification, not just faith. -columnOffsetsForTable: produces byte-identical offsets to the old -makeRowWithColumns: running sum for the empty-lines case; the base = (i==0 || i==numColumns) ? 0 : spacing boundary handling correctly reproduces "spacing between columns only."
  • Matrix fast path is genuinely inert. -makeRuleDisplaysForTable: returns @[] early when there are no vertical/horizontal lines, so no MTRuleDisplay is appended and recomputeDimensions folds only row displays exactly as before. testMatrixLayoutUnchangedByArraySupport pins this; all pre-existing typesetter/matrix tests pass unchanged.
  • Fold-into-bounds mechanism is correct and elegant. MTRuleDisplay carries its own position/width/ascent/descent, so recomputeDimensions picks up rule extents for free; vertical rules fold frameTop/frameBot into the table's ascent/descent, which is what \left…\right reads via makeInner. testArrayInsideDelimitersScalesToRuleBounds exercises the full delimiter path.
  • draw: mirrors the established MTFractionDisplay line-drawing convention ([self.textColor setStroke], moveToPoint:self.position in table-local coords, [super draw:] first for localBackgroundColor).
  • Memory safety intact: new allocation lives inside the existing @try/@finally that frees columnWidths; no new C buffers; ARC handles the display tree; rules are leaves with no retain cycles.
  • Implementation matches plan items 9–12 essentially verbatim.

Issues

Important (Should Fix)

  1. makeRuleDisplaysForTable: can read rows[b] out of bounds if horizontalLines.count > numRows + 1. Today it's safe only because the parser caps \hline at numRows + 1 boundaries — an invariant enforced two PRs away with no local assertion. The public +arrayTableWithAlignments: factory can construct a model that violates it → out-of-bounds NSArray access → crash. Add NSAssert(hLines.count <= numRows + 1, …) or clamp b. Per the "fail loud" rule, an explicit assert beats a raw-index crash.
  2. Tests lock counts/monotonicity but pin almost no actual rule geometry. testArrayRuleDisplayCountAndBounds checks ruleCount == 5 and width > plainWidth; testArrayColumnOffsetsMatchAlignmentNoRules only checks ordering (x1 > x0 > x2). None assert a specific rule x/y, the frame padding, that a vertical rule's position.x == padding + thickness/2, or that an interior \hline lands at the row-gap midpoint. With the multipliers "snapshot-tuned" and no snapshot test, a refactor could shift every rule by a constant and stay green. Add at least one test asserting a vertical rule's position.x and a horizontal rule's position.y to numeric tolerance — the values are deterministic from font metrics.

Minor (Nice to Have — defer to the snapshot-tuning pass)

  1. Corner overhang. Horizontal rules start at x = 0 spanning contentWidth, but the leftmost vertical sits at x ≈ padding, and the top horizontal at frameTop + thickness vs. verticals topping out at frameTop. Outer rules overlap by padding/thickness rather than meeting flush; the "meet at the corners by construction" comment overstates it.
  2. Left/right frame asymmetry for edge |. A leading | leaves padding of blank space left of the leftmost rule; LaTeX puts the rule flush at the box edge.
  3. Rule scale tracks _styleFont while cells track cell style. Identical at top level, but an array nested in a script/scriptscript position would draw full-size rules around shrunk cells. Worth a one-line note on whether that's deliberate.
  4. Micro-DRY: padding/ruleGap are recomputed from the same multipliers in both columnOffsetsForTable: and makeRuleDisplaysForTable:; a single helper would prevent the two drifting.

Verdict

Core is correct — no Critical issues. Recommend landing the hLines.count assert (#1) and one numeric-geometry test (#2) before merge; both are small and materially raise confidence in a layout engine where "compiles + doesn't crash" is a weak signal. Issues 3–6 can ride the later snapshot-tuning pass.

🤖 Generated with Claude Code

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