Foundations for editable math: replaceNode fix, caret preservation, baseline-correct cursor#129
Open
shubham030 wants to merge 5 commits intosimpleclub:mainfrom
Open
Foundations for editable math: replaceNode fix, caret preservation, baseline-correct cursor#129shubham030 wants to merge 5 commits intosimpleclub:mainfrom
shubham030 wants to merge 5 commits intosimpleclub:mainfrom
Conversation
`SyntaxTree.replaceNode` previously hit a runtime `_TypeError` on any
mutation whose parent was a concrete subclass of `GreenNode`:
type 'List<GreenNode?>' is not a subtype of
type 'List<GreenNode>' of 'newChildren'
The naïve `posParent.children.map((c) => identical(c, pos) ? newNode : c?.value).toList()`
builds a `List<GreenNode?>`, but every concrete subclass of `GreenNode`
narrows the parameter of `updateChildren` via the `covariant` keyword:
- EquationRowNode → List<GreenNode> (non-null)
- FracNode, AccentNode, etc. → List<EquationRowNode> (typed, non-null)
- MultiscriptsNode, SqrtNode, NaryOperatorNode → List<EquationRowNode?>
(nullable)
The covariant check fires at runtime whenever the static type differs
from the concrete override's expected list element type — which is
effectively always. Dispatch on the concrete parent type and materialise
a correctly-typed list before calling `updateChildren`.
Regression tests cover all three parent-type shapes plus root replacement.
`MathController.ast = newTree` hard-reset the selection to `TextSelection.collapsed(offset: -1)` on every tree change. This made the caret unusable for any editable-math use case — every insertion, deletion, or structural transform would erase the cursor position. Call `sanitizeSelection(_ast, _selection)` instead, which clamps the existing selection to the new tree's valid range (exactly what the `selection =` setter already does). Selection is preserved as long as the new tree contains the old cursor position; otherwise it's clamped to the valid range instead of blown away.
…r plumbing Two related changes required for downstream packages that want to build editable math (read: caret that survives tree mutations, driven by external input logic): 1. **External controller.** `SelectableMath` and `InternalSelectableMath` now accept an optional `controller: MathController`. When provided, the widget uses it instead of creating its own, and doesn't dispose it on unmount. `didUpdateWidget` handles both the owned-controller case (mutates `controller.ast`, preserving selection via the setter fix in the previous commit) and the external-controller case (passes through without re-creating). Also exports `MathController` from the public library surface so callers can actually construct one. 2. **showCursor in SelectionStyle.** `InternalSelectableMath.build` constructed the `SelectionStyle` Provider value without passing `widget.showCursor`. The render path (`EditableLine` → `RenderEditableLine.paint`) reads `showCursor` from that SelectionStyle, so the caret was never painted regardless of what the caller passed to `SelectableMath(showCursor: true)`. Now threaded through.
… empty rows Two caret-positioning bugs that only manifested for editable use: 1. **Caret drifted below text on lines with sup/sub/frac.** `RenderEditableLine.paint` anchored the caret at `Offset(cursorOffset, size.height)`. For any row containing a superscript, subscript, or fraction, `size.height` is substantially larger than the alphabetic baseline (it includes the ascent of the superscript / the height of the fraction). The caret ended up painted well below the main text line. Anchor at the row's actual alphabetic baseline (`maxHeightAboveBaseline`) instead. 2. **Empty rows reported zero baseline.** `computeDistanceToActualBaseline` on an empty row returned 0 (no children → `maxHeightAboveBaseline` is 0), which broke ancestor `Baseline` widgets: the row would align at Y=0 when empty, then jump when the first character was typed as the real baseline appeared. Override `computeDistanceToActualBaseline` on `RenderEditableLine` to fall back to `~80% of preferredLineHeight` when there are no children. Same fallback is used for the caret paint, so paint and layout always agree — empty↔non-empty transitions no longer shift the caret visually.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request exposes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/replace_node_test.dart (1)
62-91: Consider adding a test for replacing withnullin a nullable slot.The MultiscriptsNode test replaces a non-null superscript with another non-null value. Since
MultiscriptsNodesupports nullable children, consider adding a test that replaces a non-null slot with an empty/different structure to verify the nullable path in_rebuildParentworks correctly whenhasNullsis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/replace_node_test.dart` around lines 62 - 91, Add a new test that covers replacing a non-null nullable slot with null for MultiscriptsNode: use the existing parsing of r'x^{2}' to find the MultiscriptsNode and its superscript child (same logic as locating supRed), then call tree.replaceNode(supRed, null) (or the API variant that accepts null) and assert the resulting updated.greenRoot MultiscriptsNode has sup == null while base remains unchanged (base.children contains SymbolNode 'x'); this verifies the nullable path in _rebuildParent when hasNulls is true.lib/src/widgets/controller.dart (1)
34-37: Minor: Early return atselection.end <= 0may skip constraining for offset 0.The guard
selection.end <= 0returns early without constraining. Ifselection.end == 0is a valid position (e.g., caret before the first child), this works correctly. However, if the tree shrinks such that offset 0 becomes invalid, the selection won't be constrained.In practice this is likely fine since
ast.root.range.startis typically 0 or positive, but worth confirming the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/widgets/controller.dart` around lines 34 - 37, The early-return in sanitizeSelection (the selection.end <= 0 check) can skip constraining when the tree's root range doesn't start at 0; update the guard so the selection is constrained against ast.root.range in all relevant cases — either remove the early return and always return selection.constrainedBy(ast.root.range), or change the condition to compare against ast.root.range.start (e.g., only return early if selection.end < ast.root.range.start). Modify the sanitizeSelection function accordingly so it always uses ast.root.range to constrain the selection when needed.lib/src/ast/syntax_tree.dart (1)
70-103: Good type-dispatch implementation, but line 93 contains dead code.The
_rebuildParenthelper correctly handles the covariant type requirements of different parent node types. However, at line 93, the checkif (newNode == null) hasNulls = true;is unreachable becausenewNodeis typed asGreenNode(non-nullable) — it can never benull.🧹 Remove dead code
} else if (identical(child, pos)) { nullableChildren.add(newNode as EquationRowNode?); - if (newNode == null) hasNulls = true; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ast/syntax_tree.dart` around lines 70 - 103, The branch in _rebuildParent that does `if (newNode == null) hasNulls = true;` is dead because newNode is non-nullable; remove that unreachable check and its surrounding conditional (leave the nullableChildren.add(newNode as EquationRowNode?) cast in place), ensuring hasNulls is only set when encountering actual null children and the logic that chooses updateChildren(nullableChildren.cast<EquationRowNode>()) vs updateChildren(nullableChildren) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/widgets/selectable.dart`:
- Around line 457-471: In didUpdateWidget inside InternalSelectableMath, ensure
you dispose any previously owned MathController before replacing it when
widget.controller changes; specifically, when widget.controller !=
oldWidget.controller and the new widget.controller is non-null, if
_ownsController is true call controller.dispose() (or the appropriate cleanup)
before assigning controller = widget.controller! and setting _ownsController =
false; likewise, when creating a new owned controller (controller =
MathController(...)) first dispose the old controller if _ownsController was
true to avoid leaking the previous owned instance.
---
Nitpick comments:
In `@lib/src/ast/syntax_tree.dart`:
- Around line 70-103: The branch in _rebuildParent that does `if (newNode ==
null) hasNulls = true;` is dead because newNode is non-nullable; remove that
unreachable check and its surrounding conditional (leave the
nullableChildren.add(newNode as EquationRowNode?) cast in place), ensuring
hasNulls is only set when encountering actual null children and the logic that
chooses updateChildren(nullableChildren.cast<EquationRowNode>()) vs
updateChildren(nullableChildren) remains unchanged.
In `@lib/src/widgets/controller.dart`:
- Around line 34-37: The early-return in sanitizeSelection (the selection.end <=
0 check) can skip constraining when the tree's root range doesn't start at 0;
update the guard so the selection is constrained against ast.root.range in all
relevant cases — either remove the early return and always return
selection.constrainedBy(ast.root.range), or change the condition to compare
against ast.root.range.start (e.g., only return early if selection.end <
ast.root.range.start). Modify the sanitizeSelection function accordingly so it
always uses ast.root.range to constrain the selection when needed.
In `@test/replace_node_test.dart`:
- Around line 62-91: Add a new test that covers replacing a non-null nullable
slot with null for MultiscriptsNode: use the existing parsing of r'x^{2}' to
find the MultiscriptsNode and its superscript child (same logic as locating
supRed), then call tree.replaceNode(supRed, null) (or the API variant that
accepts null) and assert the resulting updated.greenRoot MultiscriptsNode has
sup == null while base remains unchanged (base.children contains SymbolNode
'x'); this verifies the nullable path in _rebuildParent when hasNulls is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbe4a5af-53a4-4721-9e52-c36367ed2fdc
📒 Files selected for processing (6)
lib/flutter_math.dartlib/src/ast/syntax_tree.dartlib/src/render/layout/line_editable.dartlib/src/widgets/controller.dartlib/src/widgets/selectable.darttest/replace_node_test.dart
Two small fixes from the automated PR review: 1. `selectable.dart`: `InternalSelectableMathState.didUpdateWidget` was overwriting the `controller` field when `widget.controller` changed without first disposing the previously-owned instance. This leaked the internally-created MathController's listeners + notifiers whenever the caller swapped from null → external (or back). Now disposes the owned controller before the reassignment. 2. `syntax_tree.dart`: Removed a dead `newNode == null` check inside `_rebuildParent` — `newNode` is statically typed `GreenNode` (non-nullable), so the branch was unreachable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four small, independently reviewable fixes that together make
flutter_math_forkusable as the rendering core of an editable math widget. Each commit stands alone; the test suite passes after each one.Summary
fix(ast)—SyntaxTree.replaceNodecurrently throws a runtime_TypeErroron any in-tree mutation because it hands aList<GreenNode?>to concreteupdateChildrenoverrides that narrow the parameter type via covariance. Dispatched on concrete parent type to build a correctly-typed list.fix(widgets)—MathController.ast =hard-reset the selection to offset -1 on every tree change, making the caret unusable for any editing. CallssanitizeSelectioninstead (same logic theselection =setter already uses).feat(widgets)—SelectableMathandInternalSelectableMathnow accept an optional externalMathController. When provided, the widget forwards it to the rendering Provider tree instead of creating its own. Lets downstream code drive selection from outside. Also fixes a plumbing bug where `showCursor` was never passed into the `SelectionStyle` Provider value, so the caret never painted regardless of `SelectableMath(showCursor: true)`. Exports `MathController` from the public library surface.fix(render)— The caret was anchored at `size.height` in `RenderEditableLine.paint`, which put it visually below the main text on any line containing a superscript or fraction. Anchored at `maxHeightAboveBaseline` instead, and overrode `computeDistanceToActualBaseline` to return a stable fallback (`preferredLineHeight * 0.8`) for empty rows so ancestor `Baseline` widgets don't see a 0→nonzero jump when the first character is typed.Test plan
Why this bundle
These four changes are coupled in practice — fixing any one in isolation doesn't unlock editing because the other three still block it (no mutation → no replaceNode needed; mutation but reset selection → cursor dies; preserved cursor but not painted → invisible; painted but at size.height → visually wrong). Splitting further would produce dependent PRs.
Happy to split into separate PRs per commit if preferred.