Add ReactivePage.InvalidateLayout() for runtime layout rebuilds#220
Conversation
`OnNavigatedTo` builds and caches `_layoutRoot` exactly once (`if (_layoutRoot == null) _layoutRoot = BuildLayout();`). Values captured inside `BuildLayout()` — e.g., terminal dimensions baked into a `HeightAuto(min, max)` `SizeConstraint.Auto` record, theme colors frozen on nodes — are permanently fixed at first-navigation time. There is no way for consumer code to react to runtime state changes that affect layout structure (terminal resize being the obvious case). Add `protected void InvalidateLayout()` to `ReactivePage<TViewModel>` that discards the cached layout root so the next render rebuilds via `BuildLayout()`. User subscriptions in `Subscriptions` and registered `KeyBindings` are preserved; the framework-internal invalidation→redraw subscription is moved to a dedicated `_layoutSubscriptions` composite so `InvalidateLayout` can replace it without accumulating dead subscriptions or leaking through user code. The old layout is deactivated but NOT disposed — page authors commonly hold field references to nodes constructed in `OnBound` and reused as content inside `BuildLayout` (e.g., a `StreamingTextNode` that wraps a scroll buffer); disposing would destroy that state. The orphan wrapper nodes built only inside `BuildLayout` are garbage-collected. Adds `ReactivePageInvalidateLayoutTests` covering: BuildLayout called again, layout root replaced (different instance), user subscriptions preserved, key bindings preserved, and a consumer-shaped scenario where external state captured in BuildLayout updates after invalidate. Discovered while implementing terminal-size-aware approval prompts in netclaw — the static panel cap captured at first navigation never updates on resize, even though the inner DynamicLayoutNode does. InvalidateLayout gives consumers a clean way to fix that downstream without requiring framework auto-invalidation that could surprise existing Termina users.
Initial commit shipped a working but fragile InvalidateLayout API. Code review surfaced multiple lifecycle / exception-safety / UX gaps. This commit addresses all of them and locks behavior in with tests. Lifecycle state (new _isActivated/_isDisposed/_isRebuilding flags): - Throw ObjectDisposedException when InvalidateLayout or OnNavigatedTo is called after Dispose. Previously R3 CompositeDisposable.Clear / Add silently no-op'd on a disposed bag, so a late InvalidateLayout would build a fresh tree, OnActivate every node (starting timers/ animations), then leak the whole tree since Dispose had already run. - Throw InvalidOperationException when InvalidateLayout re-enters via BuildLayout or a node lifecycle callback. Previously unbounded recursion or a divergent state (inner tree leaked, outer tree overwrites _layoutRoot) was possible. - No-op InvalidateLayout when the page is not currently active (before first OnNavigatedTo, or between OnNavigatingFrom and the next OnNavigatedTo). Previously the page would silently re-activate while the framework still thought it was inactive, then double- subscribe + double-activate on the next navigation. - Idempotent OnNavigatedTo: skip the wiring path when already active so a user override calling base.OnNavigatedTo() twice doesn't pile up duplicate _layoutSubscriptions entries. - Idempotent Dispose: short-circuit on _isDisposed. Exception-safe build-then-swap: - InvalidateLayout now builds the new tree FIRST, OnActivates it, then atomically swaps _layoutRoot, then deactivates the old tree. If BuildLayout throws, the page keeps its existing layout intact — no half-state with cleared subscriptions and a null root. - BuildLayout returning null throws InvalidOperationException with a clear message instead of silently leaving _layoutRoot null and rendering blank. Focus preservation: - InvalidateLayout no longer unconditionally re-applies FocusPolicy. If the user has Tab'd to a non-default focusable AND that node still exists in the new tree (common case when BuildLayout reuses page- field nodes), focus is preserved. Only when the focused node is orphaned by the rebuild does the policy default re-apply, so the user always has somewhere to land. Redraw trigger: - InvalidateLayout now explicitly calls ViewModel.RequestRedraw() at the end. Previously the new tree was built and activated but no redraw was triggered until some other reactive emission happened, so a resize handler that did nothing but invalidate produced no visible change. Documentation: - Expanded InvalidateLayout XML remarks: now enumerates the preserved vs reset state (subscriptions/keybindings/focus), the build-then- swap guarantee, the known orphan-container leak (ContainerNode keeps child-invalidation subscriptions to user-held nodes until page Dispose — Dispose of orphan would recurse into user leaves, so we can't), the user-Subscriptions-targeting-BuildLayout-created-nodes hazard, the throw contract, and the thread affinity requirement. Tests: Adds 12 new tests covering every contract: - BeforeOnNavigatedTo_IsNoOp, AfterOnNavigatingFrom_IsNoOp - AfterDispose_Throws, OnNavigatedTo_AfterDispose_Throws, Dispose_IsIdempotent - ReEntrant_Throws (via test page that re-enters from BuildLayout) - NullBuildLayout_Throws, BuildLayoutThrows_LeavesLayoutIntact (verifies build-then-swap rollback) - RequestsRedraw - OldInvalidatingNode_DoesNotDriveRedraws, NewInvalidatingNode_DrivesRedraws, RepeatedCalls_DoNotAccumulateSubscriptions (the IInvalidatingNode surface the original tests never exercised — they all used TextNode which is NOT IInvalidatingNode) Existing 5 tests still pass; full Termina suite 1116/1116 green.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Two concrete issues worth fixing before merge:\n\n1. abandons the old layout root without disposing it, but wrapper nodes such as and only detach child invalidation subscriptions in . If a page reuses a child node instance across rebuilds, those old wrapper nodes can stay alive through the child -> parent invalidation subscription chain instead of being garbage-collected.\n\n2. rebuilds the tree without reconciling focus. On pages using manual focus, can keep pointing at a control from the discarded tree, so subsequent input is routed to a stale node instead of the rebuilt layout.\n\nPlease add a regression test for the focus case as part of the fix.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Two concrete issues worth fixing before merge:
-
InvalidateLayout()abandons the old layout root without disposing it, but wrapper nodes such asPanelNodeandContainerNodeonly detach child invalidation subscriptions inDispose(). If a page reuses a child node instance across rebuilds, those old wrapper nodes can stay alive through the child -> parent invalidation subscription chain instead of being garbage-collected. -
InvalidateLayout()rebuilds the tree without reconciling focus. On pages using manual focus,IFocusManager.CurrentFocuscan keep pointing at a control from the discarded tree, so subsequent input is routed to a stale node instead of the rebuilt layout.
Please add a regression test for the focus case as part of the fix.
Problem
ReactivePage<TViewModel>.OnNavigatedTobuilds and caches_layoutRootexactly once and never re-runsBuildLayout(). Any value captured insideBuildLayout()and baked into a layout-node field (likeHeightAuto'sSizeConstraint.Autorecord) is permanently frozen at first-navigation time. Consumers have no way to trigger a rebuild — terminal resize handlers can't force the layout to re-evaluate height/width-dependent constraints.Fix
Add
protected void InvalidateLayout()toReactivePage<TViewModel>. Discards the cached_layoutRootand rebuilds viaBuildLayout(), preserving everything the user cares about (Subscriptions,KeyBindings, and user focus when the target node survives the rebuild).Refactor: move the framework's
IInvalidatingNode.Invalidated → RequestRedrawsubscription to a dedicated_layoutSubscriptionsCompositeDisposablesoInvalidateLayoutcan tear it down without touching user code.Consumer usage
Hardened lifecycle contract (post review)
Initial commit was functional but fragile. Code review surfaced lifecycle / exception-safety / UX gaps that this PR now addresses:
Lifecycle state (
_isActivated/_isDisposed/_isRebuildingflags):ObjectDisposedExceptionifInvalidateLayoutorOnNavigatedTois called afterDispose. Verified R3CompositeDisposable.Clear/Addsilently no-op on a disposed bag, so without the guard a lateInvalidateLayoutwould build a fresh tree,OnActivateevery node (starting timers/animations), then leak the whole tree.InvalidOperationExceptionon re-entrantInvalidateLayout(from insideBuildLayoutor a node lifecycle callback) — prevents unbounded recursion and divergent state.OnNavigatedTo, or betweenOnNavigatingFromand the nextOnNavigatedTo) — prevents resurrecting an inactive page and the resulting double-subscribe + double-activate on the next navigation.OnNavigatedTo— skips wiring path when already active so a user override callingbase.OnNavigatedTo()twice doesn't pile up duplicate_layoutSubscriptions.Dispose.Exception-safe build-then-swap:
OnActivate's it, then atomically swaps_layoutRoot, then deactivates the old. IfBuildLayoutthrows, the page keeps its existing layout — no half-state with cleared subscriptions and a null root.BuildLayoutreturningnullthrowsInvalidOperationExceptionwith a clear message instead of silently leaving_layoutRootnull and rendering blank.Focus preservation:
FocusPolicy. If the user has Tab'd to a focusable AND that node still exists in the new tree (common case whenBuildLayoutreuses page-field nodes), focus is preserved. Only when the focused node is orphaned does the policy default re-apply.Redraw trigger:
InvalidateLayoutexplicitly callsViewModel.RequestRedraw()at the end so a resize handler that only invalidates produces a visible change.What's documented (not enforced)
ContainerNode.Dispose()recurses into children, so we can't Dispose the old tree without destroying user-held leaves (e.g., aStreamingTextNodepage field). The old container's_childInvalidationSubscriptionsto those leaves stay live until page Dispose. Repeated invalidate accumulates dead containers. Documented in the XML remarks; consumers using heavy reusable nodes should prefer reactive bindings insideBuildLayoutover frequent invalidate.SubscriptionstargetingBuildLayout-created nodes: those subscriptions keep firing into orphaned nodes after invalidate. Documented; consumers should bind to page-field nodes (initialized inOnBoundand reused) or re-subscribe at the start of eachBuildLayout.LayoutNodeILayoutNodewrappers: not deactivated by the pattern match (pre-existing behavior inOnNavigatingFrom/Dispose). Documented in remarks.InvalidateLayoutmust be called on the same dispatcher thread that drives navigation/rendering.Tests
tests/Termina.Tests/Reactive/ReactivePageInvalidateLayoutTests.cs— 17 tests total (5 original + 12 new):TriggersBuildLayoutAgain,ReplacesLayoutRootPreservesUserSubscriptions,PreservesKeyBindingsSeesUpdatedExternalStateInBuildLayoutBeforeOnNavigatedTo_IsNoOpAfterOnNavigatingFrom_IsNoOpAfterDispose_Throws,OnNavigatedTo_AfterDispose_ThrowsDispose_IsIdempotentReEntrant_ThrowsBuildLayout)NullBuildLayout_ThrowsBuildLayoutThrows_LeavesLayoutIntactRequestsRedrawOldInvalidatingNode_DoesNotDriveRedrawsNewInvalidatingNode_DrivesRedrawsRepeatedCalls_DoNotAccumulateSubscriptionsThe last three close the gap the original tests had — all originals used
TextNodewhich is NOTIInvalidatingNode, so the entire subscription-leak surface was untested.Test plan
dotnet test tests/Termina.Tests— 1116/1116 pass (17 InvalidateLayout + 1099 existing).InvalidateLayoutor override the lifecycle methods.protectedmethod (InvalidateLayout).