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

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).
 
     /// <summary>
     ///     Indicates the View's Frame or the layout of the View's subviews (including Adornments) have

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<Type> types = GetAllViewClasses ()
+                           .Where (t => !t.IsGenericType)
+                           .OrderBy (t => t.FullName, StringComparer.Ordinal)
+                           .ToList ();
+
+        StringBuilder combined = new ();
+        List<string> 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;
+}

Tests/UnitTestsParallelizable/ViewBase/Layout/LayoutConvergenceTests.cs

@@ -0,0 +1,195 @@
+// Claude - Opus 4.8
+
+namespace ViewBaseTests.Layout;
+
+/// <summary>
+///     Regression guards for the deterministic-layout properties relevant to issue #4522
+///     (<c>NeedsLayout</c> is buggy). These pin the behavior established by #5357 (split upward /
+///     downward <see cref="View.SetNeedsLayout"/> 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.
+/// </summary>
+/// <remarks>
+///     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.
+/// </remarks>
+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<View> all) BuildAbsoluteTree (int depth, int breadth)
+    {
+        View root = new () { Id = "root", Width = 200, Height = 200 };
+        List<View> 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 child = new () { Id = $"d{d}b{b}", X = b * 10, Y = d, Width = 8, Height = 1 };
+                current.Add (child);
+                all.Add (child);
+
+                if (b == 0)
+                {
+                    next = child;
+                }
+            }
+
+            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<View> 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 child = new () { Id = "child", Width = 10, Height = 3 };
+        autoParent.Add (child);
+        root.Add (autoParent);
+
+        root.BeginInit ();
+        root.EndInit ();
+        root.Layout ();
+
+        child.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 ();
+    }
+}