Skip to content

smallmatrix + gathered environments#246

Open
kostub wants to merge 4 commits into
feature/subordinate-envs-pr1from
feature/subordinate-envs-pr2
Open

smallmatrix + gathered environments#246
kostub wants to merge 4 commits into
feature/subordinate-envs-pr1from
feature/subordinate-envs-pr2

Conversation

@kostub

@kostub kostub commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Goal

Add the two subordinate amsmath environments that need no parser-argument changes — purely additive MTMathAtomFactory branches plus serialization and layout coverage. smallmatrix is a script-size centered matrix with no delimiters whose honest amsmath gap (5 mu \thickspace) renders correctly thanks to PR 1's cell-style scaling; gathered is layout-identical to the existing gather.

Stack

This PR is based on feature/subordinate-envs-pr1 and will auto-retarget to master once PR 1 merges.

Design docs

  • Plan: docs/plans/2026-06-30-smallmatrix-gathered-alignedat.md (PR 2 section)
  • LLD: docs/lld/2026-06-30-smallmatrix-gathered-alignedat.md

Commits

  • [item 3] Add smallmatrix environment (script-style centered matrix) — new factory branch + kSmallMatrixInterColumnSpacing = 5; widened serialization style-strip. Test testSmallMatrix.
  • [item 4] Add smallmatrix layout tests (gap fidelity + compactness)testSmallMatrixColumnGap, testSmallMatrixCompactVsMatrix.
  • [item 5] Add gathered environment (folds into gather branch) — widened the displaylines/gather condition. Test testGathered + stray-& error row.
  • [item 6] Add gathered/gather layout parity testtestGatheredMatchesGather.

🤖 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 smallmatrix and gathered LaTeX environments in iosMath. It adds parsing and layout logic for smallmatrix (compact inline matrix with script-style cells) and handles gathered as a single-column centered environment, along with comprehensive unit tests to verify their layout and serialization. The feedback suggests using NSUInteger instead of int for loop variables in MTMathAtomFactory.m to prevent signedness comparison warnings when iterating over collection counts.

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 +512 to +517
for (int i = 0; i < table.cells.count; i++) {
NSArray<MTMathList*>* row = table.cells[i];
for (int j = 0; j < row.count; j++) {
[row[j] insertAtom:style atIndex:0];
}
}

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 avoid signedness comparison warnings (e.g., -Wsign-compare) when comparing signed int loop counters with NSUInteger properties like table.cells.count and row.count, it is recommended to use NSUInteger for the loop variables i and j.

        for (NSUInteger i = 0; i < table.cells.count; i++) {
            NSArray<MTMathList*>* row = table.cells[i];
            for (NSUInteger j = 0; j < row.count; j++) {
                [row[j] insertAtom:style atIndex:0];
            }
        }

@kostub kostub left a comment

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.

Code Review — PR 2 (smallmatrix + gathered)

Reviewed the diff against base feature/subordinate-envs-pr1 (items 3-6). Built and ran the affected suites locally: all 6 new tests pass, and the full MTMathListBuilderTest (151) / MTMathListTest (10) / MTTypesetterTest (114) suites are green with no golden-value shifts. TEST SUCCEEDED.

Assessment: Ready to merge. No critical or important issues found.

The implementation matches the LLD (§3.3 factory/serialization) and the PR 2 plan (items 3-6) exactly:

  • smallmatrix factory branch (MTMathAtomFactory.m) — center default (unset alignments), interColumnSpacing = kSmallMatrixInterColumnSpacing (5), interRowAdditionalSpacing = 0, injects MTMathStyle(kMTLineStyleScript) per cell, no delimiters. The kSmallMatrixInterColumnSpacing = 5 constant with its amsmath/KaTeX-sourced comment is exactly the "honest 5mu, renderer scales it" design from the LLD; testSmallMatrixColumnGap cross-validates that PR 1's cell-style scaling produces 5 * muUnit * scriptScaleDown.
  • gathered — single-condition widen of the displaylines/gather branch; correctly reuses the existing numColumns == 1 loud error (verified by the new MTParseErrorInvalidNumColumns row + testGathered). Environment name is preserved (branch doesn't reassign table.environment), so it round-trips as \begin{gathered}.
  • SerializationserializedCellAtRow:column: style-strip widened to smallmatrix; round-trip asserted in testSmallMatrix.

Strengths

  • Purely additive; no existing branch behavior changed. matrix/cases/aligned/gather/eqnarray golden tests all unchanged.
  • Honest-value + renderer-scaling design avoids a pre-scaled magic constant in the model, and the layout tests are characterization tests that lock the PR1+PR2 interaction rather than re-asserting an arbitrary number.
  • Error path (gathered stray &) covered.

Minor / non-blocking observations

  1. Shared MTMathStyle atom instance across all cells (MTMathAtomFactory.m, smallmatrix branch): a single style object is insertAtom:'d into every cell. This is identical to the existing matrix (line ~398) and cases (line ~485) branches, so it is a consistent, already-tested pattern — not a regression. Flagging only for awareness: if any future code mutates a cell's leading style atom in place, the aliasing would bite all cells. No action needed for this PR.
  2. testSmallMatrix cell indexing uses table.cells[j][i] inside the i=column / j=row loops — correct (row-major), but slightly confusing to read since the outer loop variable i is the column. Cosmetic only; the test passes and asserts the right thing.
  3. Empty-environment safety: \begin{smallmatrix}\end{smallmatrix} — the cell loop simply doesn't execute and a bare empty table is returned, handled by the existing -makeTable: empty early-out (LLD §6). Not explicitly tested here, but covered by existing empty-table paths; acceptable.

Nothing requires changes before merge.

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