[204_30] Fix mismatched bracket sizes in multi-line formulas#2946
[204_30] Fix mismatched bracket sizes in multi-line formulas#2946divyansharma001 wants to merge 1 commit intoMoganLab:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0789a71 to
e28cd72
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #2932, where brackets (e.g. [ and ]) spanning multiple rows in multi-line math environments (such as align) were rendered with inconsistent sizes. The fix introduces a "bracket-pending" mechanism that uses environment variables to propagate bracket height information across table rows.
Changes:
- New helper struct
bracket_match_stateand functions to classify, get, set, and clear the cross-row bracket pending state using named environment variables. handle_matchingsignature extended withuse_pending,pending_y1,pending_y2,out_y1,out_y2parameters to incorporate cross-row height information.handle_bracketsupdated to manage the pending state lifecycle across rows.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Typeset/Concat/concat_post.cpp |
Core logic for bracket-pending state management; extends handle_matching and handle_brackets to propagate cross-row bracket extents |
src/Typeset/Concat/concater.hpp |
Updated handle_matching declaration to match the new signature |
devel/204_30.md |
Developer documentation describing the problem, root cause, and implementation approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bracket_pending_key (edit_env env) { | ||
| tree row= env->read (CELL_ROW_NR); | ||
| tree col= env->read (CELL_COL_NR); | ||
| if (!is_atomic (row) || !is_int (row)) return ""; |
There was a problem hiding this comment.
The variable row at line 340 is read from the environment but is only used for validation (to check we're inside a table cell). It is not used in constructing the pending key. This will likely produce a compiler warning about an unused variable. Consider either using (void) row; to suppress the warning, replacing the named variable with a direct is-validation expression, or explicitly marking the intention in a comment.
| if (!is_atomic (row) || !is_int (row)) return ""; | |
| if (!is_atomic (row) || !is_int (row)) return ""; | |
| // 'row' is read to ensure we are inside a table cell; its value is not | |
| // needed for constructing the pending key. | |
| (void) row; |
|
|
||
| if (has_pending) |
There was a problem hiding this comment.
The guard at line 554–555 clears the pending state for any row that had a pending bracket from a previous row but didn't touch the pending state (no bracket items encountered). This prevents stale state from leaking across unrelated rows, but it also prevents the pending state from being propagated through middle rows that happen to have no bracket items at all.
For example, in a 3-row align environment:
- Row 1:
f(x) \leq \left[ \int dx \right.→ sets pending ✓ - Row 2:
+ x(no brackets) → pending is cleared by line 554–555 ✗ - Row 3:
\right]→ never sees the pending, sizes incorrectly ✗
The PR description claims "Supports brackets spanning three or more rows", but this only works when every middle row also contains bracket pair items (which is typically the case when TeXmacs automatically inserts implicit \left. and \right. markers). If a user explicitly writes a middle row with no implicit bracket markers, the fix breaks down. A comment noting this limitation would be helpful.
| has_pending && (st.start_empty || (!st.left_seen && st.right_seen)); | ||
| SI match_y1= 0, match_y2= 0; | ||
| handle_scripts (succ (start), prec (i)); | ||
| handle_matching (start, i); | ||
| handle_matching (start, i, use_pending, pending_y1, pending_y2, match_y1, | ||
| match_y2); | ||
| if (st.end_empty || (st.left_seen && !st.right_seen)) { | ||
| pending_y1 = match_y1; | ||
| pending_y2 = match_y2; | ||
| has_pending = true; | ||
| pending_touched= true; | ||
| } | ||
| else if ((st.start_empty && !st.end_empty) || | ||
| (!st.left_seen && st.right_seen)) { | ||
| has_pending = false; | ||
| pending_touched= true; | ||
| } | ||
| if (first != -1) i= first - 1; | ||
| start= 0; | ||
| first= -1; | ||
| } | ||
| i++; | ||
| } | ||
| if (N (a) > 0) { | ||
| bracket_match_state st= classify_bracket_match (a, 0, N (a) - 1); | ||
| bool use_pending= | ||
| has_pending && (st.start_empty || (!st.left_seen && st.right_seen)); | ||
| SI match_y1= 0, match_y2= 0; | ||
| handle_scripts (0, N (a) - 1); | ||
| handle_matching (0, N (a) - 1); | ||
| handle_matching (0, N (a) - 1, use_pending, pending_y1, pending_y2, | ||
| match_y1, match_y2); | ||
| if (st.end_empty || (st.left_seen && !st.right_seen)) { | ||
| pending_y1 = match_y1; | ||
| pending_y2 = match_y2; | ||
| has_pending = true; | ||
| pending_touched= true; | ||
| } | ||
| else if ((st.start_empty && !st.end_empty) || | ||
| (!st.left_seen && st.right_seen)) { | ||
| has_pending = false; | ||
| pending_touched= true; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The pending-state update logic in lines 509-526 (inside the while loop) and lines 534-551 (the final block) are nearly identical, duplicating about 12 lines of complex conditional logic. This creates a maintenance burden since both blocks must be updated together if the logic changes. Consider extracting these repeated blocks into a shared helper function to improve maintainability.
Fixes #2932
When using \left[ on one line and \right] on another in multi-line math environments (e.g. align), the bracket sizes now match correctly.
Introduces a bracket-pending mechanism that propagates bracket height information across table rows via environment variables, so closing delimiters on subsequent lines are sized consistently with their opening counterparts.