diff --git a/Terminal.Gui/ViewBase/View.Layout.cs b/Terminal.Gui/ViewBase/View.Layout.cs index a740587f30..37f3a997eb 100644 --- a/Terminal.Gui/ViewBase/View.Layout.cs +++ b/Terminal.Gui/ViewBase/View.Layout.cs @@ -814,6 +814,10 @@ internal void LayoutSubViews () SubViewLayout?.Invoke (this, new LayoutEventArgs (contentSize)); + // Re-read again — SubViewLayout event handlers may also have called SetContentSize, and the + // value captured below is what every SubView is laid out against. See issue #4522 / #4863. + contentSize = GetContentSize (); + // The Adornments already have their Frame's set by SetRelativeLayout so we call LayoutSubViews vs. Layout here. Margin.View?.LayoutSubViews (); Border.View?.LayoutSubViews (); @@ -906,10 +910,13 @@ protected virtual void OnSubViewsLaidOut (LayoutEventArgs args) #region NeedsLayout - // We expose no setter for this to ensure that the ONLY place it's changed is in SetNeedsLayout - - // BUGBUG: The above statement is misleading. There are still cases internally where this property - // BUGBUG: is being set directly without calling SetNeedsLayout. We should remove the setter completely. + // NeedsLayout is mutated only from within View: + // * Set true by SetNeedsLayout and its helpers MarkSubtreeNeedsLayout / MarkAncestorsNeedLayout. + // * Cleared by LayoutSubViews once the view has been laid out, and by the Margin fast-path in + // View.Drawing.Adornments. + // The setter is internal (rather than private) only so layout unit tests can establish a known + // baseline; external code must never assign it directly — call SetNeedsLayout instead. Fully + // removing the setter is tracked as part of the broader NeedsLayout lifecycle cleanup (issue #4522). /// /// Indicates the View's Frame or the layout of the View's subviews (including Adornments) have diff --git a/Tests/UnitTestsParallelizable/ViewBase/Layout/AllViewsRenderFingerprintTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Layout/AllViewsRenderFingerprintTests.cs new file mode 100644 index 0000000000..bc60f07ad3 --- /dev/null +++ b/Tests/UnitTestsParallelizable/ViewBase/Layout/AllViewsRenderFingerprintTests.cs @@ -0,0 +1,131 @@ +// Claude - Opus 4.8 +// All-views render-fingerprint harness for issue #4522 verification. Draws every concrete View type +// (via EnableForDesign) onto a test driver and emits a hash of the rendered screen per view. Used to +// diff rendering before/after the LayoutSubViews content-size change: identical fingerprints prove the +// change is render-neutral across all views. Kept as a coarse all-views smoke test. + +using System.Security.Cryptography; +using System.Text; +using UnitTests; + +namespace ViewBaseTests.Layout; + +public class AllViewsRenderFingerprintTests (ITestOutputHelper output) : TestsAllViews +{ + private static string Hash (string s) + { + byte [] bytes = SHA256.HashData (Encoding.UTF8.GetBytes (s)); + + return Convert.ToHexString (bytes) [..16]; + } + + [Fact] + public void AllViews_Render_Fingerprint () + { + List types = GetAllViewClasses () + .Where (t => !t.IsGenericType) + .OrderBy (t => t.FullName, StringComparer.Ordinal) + .ToList (); + + StringBuilder combined = new (); + List threw = []; + int successful = 0; + + foreach (Type type in types) + { + string line = FingerprintOne (type); + output.WriteLine (line); + combined.AppendLine (line); + + if (line.Contains ("|EX:")) + { + threw.Add (line); + } + else if (!line.Contains ("|GENERIC") && !line.Contains ("|ENV:")) + { + successful++; + } + } + + output.WriteLine ($"--- {successful} view types successfully rendered ---"); + output.WriteLine ($"COMBINED={Hash (combined.ToString ())}"); + + // Guarantee we're actually exercising a meaningful number of concrete view types. + Assert.True (successful > 30, $"Expected to render many view types, got {successful}."); + + // No concrete view may throw while being laid out and drawn in design mode. + Assert.True (threw.Count == 0, $"View(s) threw during layout/draw:\n{string.Join ("\n", threw)}"); + } + + private static string FingerprintOne (Type type) + { + IDriver driver = CreateTestDriver (60, 20); + + View? view = CreateInstanceIfNotGeneric (type); + + if (view is null) + { + driver.Dispose (); + + return $"{type.FullName}|GENERIC"; + } + + view.Driver = driver; + + // EnableForDesign for filesystem-interactive views (FileDialog family) attempts to list + // directories during design setup or the later initialization/layout pass. That is an + // environment constraint, not a layout bug — separate it from layout/draw exceptions. + try + { + if (view is IDesignable designable) + { + designable.EnableForDesign (); + } + } + catch (Exception ex) when (IsFileDialogEnvironmentException (view, ex)) + { + view.Dispose (); + driver.Dispose (); + + return $"{type.FullName}|ENV:{ex.GetType ().Name}"; + } + + try + { + view.BeginInit (); + view.EndInit (); + view.Layout (); + + string fingerprint = $"frame={view.Frame};needsLayout={view.NeedsLayout}"; + + if (view.Visible) + { + view.SetNeedsDraw (); + view.Draw (); + fingerprint += $";screen={Hash (driver.ToString () ?? string.Empty)}"; + } + + view.Dispose (); + driver.Dispose (); + + return $"{type.FullName}|{fingerprint}"; + } + catch (Exception ex) when (IsFileDialogEnvironmentException (view, ex)) + { + view.Dispose (); + driver.Dispose (); + + return $"{type.FullName}|ENV:{ex.GetType ().Name}"; + } + catch (Exception ex) + { + view.Dispose (); + driver.Dispose (); + + return $"{type.FullName}|EX:{ex.GetType ().Name}"; + } + } + + private static bool IsFileDialogEnvironmentException (View view, Exception ex) => + view is FileDialog && ex is UnauthorizedAccessException or IOException; +} diff --git a/Tests/UnitTestsParallelizable/ViewBase/Layout/LayoutConvergenceTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Layout/LayoutConvergenceTests.cs new file mode 100644 index 0000000000..328f0fa0d4 --- /dev/null +++ b/Tests/UnitTestsParallelizable/ViewBase/Layout/LayoutConvergenceTests.cs @@ -0,0 +1,194 @@ +// Claude - Opus 4.8 + +namespace ViewBaseTests.Layout; + +/// +/// Regression guards for the deterministic-layout properties relevant to issue #4522 +/// (NeedsLayout is buggy). These pin the behavior established by #5357 (split upward / +/// downward propagation), #5358 and #5359 (region-aware, +/// non-escalating draw): a single property change must converge in one layout pass, must not +/// change frames it didn't need to, and must not fan out layout work across sibling subtrees. +/// +/// +/// Before those fixes, changing one view re-laid-out and redrew large portions of the tree +/// (see the analysis in #4522 and the measured fan-out in #4973). These tests fail loudly if +/// that regression returns. +/// +public class LayoutConvergenceTests (ITestOutputHelper output) +{ + private sealed class Counters + { + public int LaidOut; + public int FrameChanged; + + public void Attach (View v) + { + v.SubViewsLaidOut += (_, _) => LaidOut++; + v.FrameChanged += (_, _) => FrameChanged++; + } + } + + private static void ClearTree (View v) + { + v.NeedsLayout = false; + + foreach (View sv in v.SubViews) + { + ClearTree (sv); + } + } + + // Builds root -> chain of `depth` levels, with `breadth` ABSOLUTE-sized siblings at each level. + // The chain follows the first sibling at each level. Returns the deepest leaf to mutate. + private static (View root, View leaf, List all) BuildAbsoluteTree (int depth, int breadth) + { + View root = new () { Id = "root", Width = 200, Height = 200 }; + List all = [root]; + View current = root; + View leaf = root; + + for (var d = 0; d < depth; d++) + { + View next = current; + + for (var b = 0; b < breadth; b++) + { + View subView = new () { Id = $"d{d}b{b}", X = b * 10, Y = d, Width = 8, Height = 1 }; + current.Add (subView); + all.Add (subView); + + if (b == 0) + { + next = subView; + } + } + + current = next; + leaf = current; + } + + return (root, leaf, all); + } + + [Theory] + [InlineData (3, 3)] + [InlineData (6, 3)] + [InlineData (8, 4)] + public void Absolute_Child_Change_Converges_Single_Pass_Without_FanOut (int depth, int breadth) + { + (View root, View leaf, List all) = BuildAbsoluteTree (depth, breadth); + root.BeginInit (); + root.EndInit (); + root.Layout (); + + Counters c = new (); + + foreach (View v in all) + { + c.Attach (v); + } + + ClearTree (root); + + // A single absolute-size change on the deepest leaf. + leaf.Width = leaf.Frame.Width + 1; + + // Drive application-style layout passes; a healthy engine converges immediately. + var passes = 0; + + while (root.NeedsLayout && passes < 20) + { + root.Layout (); + passes++; + } + + output.WriteLine ($"depth={depth} breadth={breadth} totalViews={all.Count} passes={passes} laidOut={c.LaidOut} frameChanged={c.FrameChanged}"); + + // Converges in a single application layout pass — no multi-iteration thrashing (Bug #6). + Assert.Equal (1, passes); + + // Only the leaf's frame actually changed — no spurious frame churn up/across the tree (Bug #1). + Assert.Equal (1, c.FrameChanged); + + // Layout work stays on the affected ancestor chain (root..leaf); siblings/cousins do not + // re-run LayoutSubViews. The chain is depth+1 views; allow one extra for the root pass. + Assert.True ( + c.LaidOut <= depth + 2, + $"Layout fan-out {c.LaidOut} exceeded the ancestor chain bound {depth + 2}."); + + // And with wide breadth the fan-out is far below the total view count (no subtree fan-out, #5357). + Assert.True ( + c.LaidOut < all.Count, + $"Layout fan-out {c.LaidOut} touched the whole tree of {all.Count} views."); + + root.Dispose (); + } + + [Fact] + public void DimAuto_Parent_Converges_Single_Pass_And_Tracks_Child_Growth () + { + View root = new () { Id = "root", Width = 200, Height = 200 }; + View autoParent = new () { Id = "autoParent", Width = Dim.Auto (), Height = Dim.Auto () }; + View subView = new () { Id = "subView", Width = 10, Height = 3 }; + autoParent.Add (subView); + root.Add (autoParent); + + root.BeginInit (); + root.EndInit (); + root.Layout (); + + subView.Width = 40; + var passes = 0; + + while (root.NeedsLayout && passes < 20) + { + root.Layout (); + passes++; + } + + output.WriteLine ($"Dim.Auto parent grew to {autoParent.Frame.Size} in {passes} pass(es)"); + + // The parent must track the child's new width and converge in a single pass. + // (In this test the upward mark from child.Width = 40 is sufficient; the SetFrame + // upward mark is not additionally exercised by this synthetic case.) + Assert.Equal (40, autoParent.Frame.Width); + Assert.Equal (1, passes); + + root.Dispose (); + } + + [Fact] + public void Sibling_Reference_ReLayout_Stays_Single_Pass () + { + // A sibling that references the changed view (Pos.Right) must be repositioned — this is the + // case where the ancestor re-layout is necessary, not wasteful. It must still be single-pass. + View root = new () { Id = "root", Width = 80, Height = 25 }; + View anchor = new () { Id = "anchor", X = 0, Y = 0, Width = 10, Height = 1 }; + View follower = new () { Id = "follower", X = Pos.Right (anchor), Y = 0, Width = 5, Height = 1 }; + root.Add (anchor, follower); + + root.BeginInit (); + root.EndInit (); + root.Layout (); + + Assert.Equal (10, follower.Frame.X); + + anchor.Width = 20; + + var passes = 0; + + while (root.NeedsLayout && passes < 20) + { + root.Layout (); + passes++; + } + + output.WriteLine ($"follower repositioned to X={follower.Frame.X} in {passes} pass(es)"); + + // The follower must track the anchor's new right edge, and converge in one pass. + Assert.Equal (20, follower.Frame.X); + Assert.Equal (1, passes); + + root.Dispose (); + } +} diff --git a/Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs index 69a0c92169..6d5ca4d9e8 100644 --- a/Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs +++ b/Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs @@ -52,16 +52,36 @@ public ContentSizeChangingOnSubViewsLaidOutView () } } + /// + /// A View subclass that calls from the SubViewLayout + /// event (raised before SubViews are laid out, after ). + /// is nullable so the event handler can be armed after + /// EndInit and won't corrupt state during initialization. + /// + private class ContentSizeChangingOnSubViewLayoutEventView : View + { + public Size? NewContentSize { get; set; } + + public ContentSizeChangingOnSubViewLayoutEventView () + { + SubViewLayout += (_, _) => + { + if (NewContentSize.HasValue) { SetContentSize (NewContentSize.Value); } + }; + } + } + #endregion #region Test 1: Core stale capture /// - /// Proves the core bug: LayoutSubViews captures contentSize before - /// OnSubViewLayout fires, so SubViews are laid out with the wrong size. + /// Regression guard: LayoutSubViews re-reads contentSize after + /// OnSubViewLayout fires, so SubViews are laid out with the updated size. + /// Fixed by #4863. /// [Fact] - public void LayoutSubViews_Uses_Stale_ContentSize_When_OnSubViewLayout_Changes_It () + public void LayoutSubViews_Honors_SetContentSize_From_OnSubViewLayout () { // Arrange: SuperView that changes content size in OnSubViewLayout ContentSizeChangingOnSubViewLayoutView superView = new () @@ -143,14 +163,16 @@ public void Dialog_Children_Use_Stale_ContentSize_After_Screen_Resize () #endregion - #region Test 3: Dialog OnSubViewLayout divergence + #region Test 3: Dialog OnSubViewLayout non-divergence /// - /// Proves that Dialog.OnSubViewLayout → UpdateSizes → SetContentSize - /// produces a content size that differs from what LayoutSubViews used to lay out SubViews. + /// Regression guard: after the fix, Dialog.OnSubViewLayout → UpdateSizes → + /// SetContentSize does NOT produce a content size that differs from what + /// LayoutSubViews uses to lay out SubViews. The pre-event and post-event sizes + /// must be equal (no divergence). Fixed by #4863. /// [Fact] - public void Dialog_OnSubViewLayout_SetContentSize_Diverges_From_Captured_Value () + public void Dialog_OnSubViewLayout_SetContentSize_Does_Not_Diverge_From_Layout_Value () { // Arrange: Dialog with explicit (non-Auto) Width/Height Dialog dialog = new () @@ -288,6 +310,63 @@ public void LayoutSubViews_OnSubViewsLaidOut_SetContentSize_Is_Too_Late () #endregion + #region Test 5b: SubViewLayout event SetContentSize is honored + + // Claude - Sonnet 4.6 + /// + /// Regression test for the SubViewLayout event companion to the + /// OnSubViewLayout virtual fixed by #4863. A handler on the SubViewLayout event + /// that calls must be honored: + /// re-reads the content size after raising the event, so SubViews are laid out against the + /// updated value rather than the size captured before the event fired. See issue #4522. + /// + /// + /// The test arms the event handler after EndInit and explicitly resets the + /// content size to a known 20×10 baseline. This ensures the captured contentSize local + /// variable in LayoutSubViews starts at 20×10, so removing the post-event reread would + /// give the child a width of 20 (not 50) — making the test a genuine regression guard for the + /// specific line added in this fix. + /// + [Fact] + public void LayoutSubViews_Honors_SetContentSize_From_SubViewLayout_Event () + { + // NewContentSize is NOT set here; the event handler does nothing during EndInit, + // so it cannot pre-seed the content size to 50×20 before the guarded layout pass. + ContentSizeChangingOnSubViewLayoutEventView superView = new () + { + Width = 20, + Height = 10 + }; + + View child = new () + { + Width = Dim.Fill (), + Height = Dim.Fill () + }; + + superView.Add (child); + superView.BeginInit (); + superView.EndInit (); + + // Establish a concrete 20×10 baseline. SetContentSize also calls SetNeedsLayout so + // the next Layout() call will run LayoutSubViews and not return early. + superView.SetContentSize (new Size (20, 10)); + + // Now arm the event: the handler will inject 50×20 during the next SubViewLayout event. + superView.NewContentSize = new Size (50, 20); + + superView.Layout (); + + // The child must fill the content size injected by the SubViewLayout event handler (50×20), + // not the baseline value captured before the event fired (20×10). + Assert.Equal (50, child.Frame.Width); + Assert.Equal (20, child.Frame.Height); + + superView.Dispose (); + } + + #endregion + #region Test 6: ListView stale capture ///