Skip to content

Partially addresses #4522: document NeedsLayout invariant + lock in layout-convergence guards#5500

Merged
harder merged 6 commits into
tui-cs:developfrom
harder:kh/4522-needslayout-lifecycle
Jun 27, 2026
Merged

Partially addresses #4522: document NeedsLayout invariant + lock in layout-convergence guards#5500
harder merged 6 commits into
tui-cs:developfrom
harder:kh/4522-needslayout-lifecycle

Conversation

@harder

@harder harder commented Jun 18, 2026

Copy link
Copy Markdown
Member

Partially addresses #4522 (NeedsLayout is buggy).

⚠️ Draft. This intentionally does not fully close #4522 — see "What this deliberately does not do".

Summary

This PR delivers the safe, verifiable portion of #4522 and — critically — measures the current layout-convergence behavior to determine whether the long-feared NeedsLayout/SetFrame convergence redesign is still warranted after the #4973-era fixes (#5357 / #5358 / #5359). It is not, and this PR locks in the good behavior with regression guards instead of taking on a high-risk rewrite.

What changed

Library (View.Layout.cs)

  • Replaced the misleading NeedsLayout region comment — which the issue explicitly calls "bogus" ("We expose no setter…") — with an accurate description of the real invariant and the internal write sites, and a note on why the setter stays internal (test seam) pending full removal.
  • LayoutSubViews now re-reads GetContentSize() after the SubViewLayout event as well. It already did so after the OnSubViewLayout virtual (Partially Fixes #4522 - Stale ContentSize capture #4863); this closes the symmetric gap so a handler on the event that calls SetContentSize is honored.

Tests

Measured convergence (the key finding)

Fan-out of a single property change on current develop:

Tree (depth × breadth) total views application layout passes views that re-ran LayoutSubViews FrameChanged
3 × 3 10 1 5 1
6 × 3 19 1 8 1
8 × 4 33 1 10 1

The only residual is O(depth) ancestor re-layout that changes no frame. Removing it needs dependency-aware invalidation, which is high-risk to Dim.Auto correctness for an already-single-pass gain.

What this deliberately does NOT do

These remain as deferred, now data-backed as low priority.

Verification

  • All-views render diff: SHA of Driver.ToString() for all 61 concrete view types with the LayoutSubViews change applied vs. reverted → byte-identical (combined hash CCF30A9EA3C45AB5 both ways). The change is provably render- and layout-neutral across all views.
  • Existing AllViewsDrawTests passes (every view draws with exactly one layout pass).
  • UnitTestsParallelizable: 17,382 passed, 0 failed. UnitTests.NonParallelizable: 74 passed, 0 failed.

To pull down this PR locally

git remote add copilot https://github.com/harder/Terminal.Gui.git
git fetch copilot kh/4522-needslayout-lifecycle
git checkout kh/4522-needslayout-lifecycle

🤖 Generated with Claude Code

…-read, add layout-convergence guards

Addresses the safe, verifiable portion of tui-cs#4522 (`NeedsLayout` is buggy) and
documents — with measurements — why the remaining "convergence redesign" is not
warranted after the tui-cs#4973-era fixes (tui-cs#5357/tui-cs#5358/tui-cs#5359).

Library:
- View.Layout.cs: replace the "bogus" NeedsLayout region comment (called out in
  the issue) with an accurate description of the real invariant and the internal
  write sites; note why the setter is internal (test seam) pending full removal.
- View.Layout.cs: LayoutSubViews now re-reads GetContentSize() after the
  SubViewLayout *event* too (it already did after the OnSubViewLayout virtual,
  tui-cs#4863), so a handler on that event that calls SetContentSize is honored.

Tests:
- StaleContentSizeCaptureTests: +1 regression test for the SubViewLayout-event
  content-size case.
- LayoutConvergenceTests (new): permanent guards locking in the deterministic
  properties established by tui-cs#5357/tui-cs#5358/tui-cs#5359 — single-pass convergence, no
  spurious FrameChanged, bounded ancestor-chain fan-out (no subtree fan-out),
  Dim.Auto growth correctness, and Pos.Right sibling repositioning.
- AllViewsRenderFingerprintTests (new): all-views smoke guard (every concrete
  View lays out + draws in design mode without throwing); also the harness used
  to prove the LayoutSubViews change is byte-identical in rendering across all
  61 view types.

Verification: UnitTestsParallelizable 17,382 passed; UnitTests.NonParallelizable
74 passed; all-views Driver.ToString() fingerprint identical with/without the
library change.

See plans/4522-needslayout-lifecycle.md for the full assessment and measured
fan-out numbers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@harder harder mentioned this pull request Jun 18, 2026
harder and others added 4 commits June 26, 2026 16:45
…ssues

P1 (bug): AllViewsRenderFingerprintTests now separates filesystem
environment exceptions (UnauthorizedAccessException / IOException from
FileDialog EnableForDesign) from real layout/draw exceptions. ENV:
exceptions are logged but do not count as test failures.

P2a (weak test): StaleContentSizeCaptureTests regression for the
SubViewLayout event reread is now a genuine guard. NewContentSize is
nullable and armed AFTER EndInit; SetContentSize(20×10) establishes a
concrete baseline. Without the production reread the test fails with
Expected:50 / Actual:20 — confirmed by mutation testing.

P2b (overclaim): The 'SetFrame NeedsLayout mark is load-bearing for
Dim.Auto' comment and plan wording softened to reflect that synthetic
tests don't exercise that path (child.Width= already marks ancestors);
the ad-hoc guard is still not recommended for other reasons.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AllViewsRenderFingerprintTests:
- Fix resource leak: catch block now disposes view and driver before
  returning |EX:, matching all other exit paths
- Track 'successful' (laid out + drawn without exception) separately
  from 'rendered' (total processed); assert successful > 30 so the
  smoke check is meaningful, not a type-count tautology

StaleContentSizeCaptureTests:
- Rename Test 1: LayoutSubViews_Uses_Stale_ContentSize_... ->
  LayoutSubViews_Honors_SetContentSize_From_OnSubViewLayout (method
  name and summary both claimed to prove a bug; assertions verify the
  fixed behavior)
- Rename Test 3: Dialog_..._Diverges_From_Captured_Value ->
  Dialog_..._Does_Not_Diverge_From_Layout_Value; update summary to
  match (Assert.Equal asserts NO divergence, not divergence)

plans/4522-needslayout-lifecycle.md:
- Fix §18-26 mechanism description: SetNeedsLayout propagates upward
  only when SuperView.NeedsLayout is false; during a layout pass the
  SuperView is already marked so SetFrame's call is effectively a
  no-op. DimAuto resolves in-place via SetRelativeLayout(), not via
  a 'mark + re-layout next iteration' mechanism. Reconcile with §57.
- Fix stale line references: :862/:864-865 -> :866/:868-869 for the
  NeedsLayout clear and SubViewsLaidOut fire sites

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens and documents View.NeedsLayout invariants and layout-convergence behavior (issue #4522), closes a remaining LayoutSubViews content-size re-read gap for SubViewLayout event handlers, and adds regression tests that lock in the post-#5357/#5358/#5359 deterministic layout properties.

Changes:

  • View.LayoutSubViews () now re-reads GetContentSize () after the SubViewLayout event (in addition to the existing post-OnSubViewLayout re-read).
  • Updates the NeedsLayout region comment to describe the actual invariant and internal write sites.
  • Adds regression tests for the SubViewLayout event SetContentSize scenario, plus new convergence + all-views smoke/fingerprint tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
Terminal.Gui/ViewBase/View.Layout.cs Re-reads content size after SubViewLayout event; updates NeedsLayout invariant documentation.
Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs Adds regression coverage for SubViewLayout event handlers calling SetContentSize.
Tests/UnitTestsParallelizable/ViewBase/Layout/LayoutConvergenceTests.cs New convergence regression guards for single-pass layout + bounded fan-out.
Tests/UnitTestsParallelizable/ViewBase/Layout/AllViewsRenderFingerprintTests.cs New all-views design-mode layout/draw smoke + render fingerprint harness.

Comment thread Tests/UnitTestsParallelizable/ViewBase/Layout/LayoutConvergenceTests.cs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@harder harder marked this pull request as ready for review June 27, 2026 00:41
@harder harder requested a review from tig as a code owner June 27, 2026 00:41

@tig tig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

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.

NeedsLayout is buggy

3 participants