`NeedsLayout` is buggy

[tig]

The implementation and design of View.NeedsLayout and related APIs (e.g. SetNeedsLayout) is buggy:

On one hand, there are places where the layout (and Content area) code creates false-positives, setting NeedsLayout to true, when, in fact, no layout is needed. E.g. See PosDimSet.

On the other hand, there are places where calls to Layout have been added to work around cases where the next Application iteration should cause a layout, but doesn't; leading someone (often me) to add a Layout call. Some of these are marked with // BUGBUG:s but not all. We should view ANY call to Layout directly as a bug.

We should revamp this to be more deterministic and clear. One place to start is to remove the setter for NeedsLayout completely. This comment in View.Layout.cs is bogus:

 #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.

 /// <summary>
 ///     Indicates the View's Frame or the layout of the View's subviews (including Adornments) have
 ///     changed since the last time the View was laid out.
 /// </summary>
 /// <remarks>
 ///     <para>
 ///         Used to prevent <see cref="Layout()"/> from needlessly computing
 ///         layout.
 ///     </para>
 /// </remarks>
 /// <value>
 ///     <see langword="true"/> if layout is needed.
 /// </value>
 public bool NeedsLayout { get; private set; } = true;

Any code that sets NeedsLayout directly should be viewed as suspicious:

Here's a analysis I had the AIs create. I have not reviewed it for accuracy, so treat it with grains of salt:

NeedsLayout System Analysis - Latent Bugs and Issues

Date: 2025-01-XX
Project: Terminal.Gui
Scope: View.Layout.cs NeedsLayout mechanism and related APIs

---

Table of Contents

1. Executive Summary
2. NeedsLayout Flow Overview
3. Latent Bugs Identified
4. Test Coverage Analysis
5. Recommendations

---

Executive Summary

The NeedsLayout system in Terminal.Gui is a critical mechanism for optimizing layout operations by tracking when a view needs to be laid out. However, analysis reveals 6 latent bugs that could cause:

• Performance issues (O(N²) propagation in deep hierarchies)
• Infinite layout loops (SetFrame always sets NeedsLayout=true)
• Event system failures (property change events not firing)
• Logic errors (incorrect SuperView checks)
• Timing issues (flag cleared before events fire)

The system has NOT exploded primarily due to:

1. Workarounds in PosDimSet() that force re-layout
2. Application iteration loop that keeps trying
3. Limited deep view hierarchies in typical usage
4. Test coverage focused on end results, not intermediate states

---

NeedsLayout Flow Overview

Key Components

1. NeedsLayout Property (Line 840)

public bool NeedsLayout { get; private set; } = true;

• Boolean flag indicating layout needed
• Private setter ensures only modified through SetNeedsLayout()
• Initially true for all views

2. SetNeedsLayout() Method (Lines 852-923)

public void SetNeedsLayout()
{
    NeedsLayout = true;
    
    // 1. Set on adornments (Margin, Border, Padding)
    if (Margin is { SubViews.Count: > 0 })
        Margin.SetNeedsLayout();
    if (Border is { SubViews.Count: > 0 })
        Border.SetNeedsLayout();
    if (Padding is { SubViews.Count: > 0 })
        Padding.SetNeedsLayout();
    
    // 2. Propagate DOWN to all descendants (using stack to avoid recursion)
    Stack<View> stack = new(InternalSubViews.Snapshot().ToList());
    while (stack.Count > 0)
    {
        View current = stack.Pop();
        if (!current.NeedsLayout)
        {
            current.NeedsLayout = true;
            // ... propagate to descendants ...
        }
    }
    
    // 3. Propagate UP to SuperView
    if (SuperView is { NeedsLayout: false })
        SuperView?.SetNeedsLayout();
    
    // 4. If this is an Adornment, propagate to Parent
    if (this is Adornment adornment && adornment.Parent is { NeedsLayout: false })
        adornment.Parent?.SetNeedsLayout();
}

Propagation Direction:

• DOWN: To all descendants (subviews and their adornments)
• UP: To SuperView and (for Adornments) to Parent

3. Layout() Flow (Lines 520-534)

public bool Layout(Size contentSize)
{
    if (SetRelativeLayout(contentSize))  // Calculate THIS view's Frame
    {
        LayoutSubViews();                // Layout descendants
        SetNeedsDraw();                  // Mark for redraw
        return true;                     // Success
    }
    return false;                        // Failed (dependency not ready)
}

4. LayoutSubViews() Flow (Lines 709-784)

internal void LayoutSubViews()
{
    if (!NeedsLayout)
        return;  // Early exit optimization
    
    // 1. Get content size
    Size contentSize = GetContentSize();
    
    // 2. Fire pre-layout events
    OnSubViewLayout(new(contentSize));
    SubViewLayout?.Invoke(this, new(contentSize));
    
    // 3. Layout adornments
    if (Margin is { SubViews.Count: > 0 })
        Margin.LayoutSubViews();
    if (Border is { SubViews.Count: > 0 })
        Border.LayoutSubViews();
    if (Padding is { SubViews.Count: > 0 })
        Padding.LayoutSubViews();
    
    // 4. Topological sort for dependencies
    HashSet<View> nodes = new();
    HashSet<(View, View)> edges = new();
    CollectAll(this, ref nodes, ref edges);
    List<View> ordered = TopologicalSort(SuperView!, nodes, edges);
    
    // 5. Two-pass layout
    List<View> redo = new();
    foreach (View v in ordered.Snapshot())
    {
        if (!v.Layout(contentSize))
            redo.Add(v);  // Dependency not ready
    }
    
    if (redo.Count > 0)
    {
        foreach (View v in ordered)
        {
            if (!v.Layout(contentSize))
                layoutStillNeeded = true;
        }
    }
    
    // 6. Clear the flag
    NeedsLayout = layoutStillNeeded;
    
    // 7. Fire post-layout events
    OnSubViewsLaidOut(new(contentSize));
    SubViewsLaidOut?.Invoke(this, new(contentSize));
}

5. SetFrame() Method (Lines 80-112)

private bool SetFrame(in Rectangle frame)
{
    if (_frame == frame)
        return false;
    
    Rectangle oldViewport = Rectangle.Empty;
    if (IsInitialized)
        oldViewport = Viewport;
    
    _frame = frame;
    SetAdornmentFrames();
    SetNeedsDraw();
    SetNeedsLayout();  // ⚠️ ALWAYS sets NeedsLayout = true
    
    OnFrameChanged(in frame);
    FrameChanged?.Invoke(this, new(in frame));
    
    if (oldViewport != Viewport)
        RaiseViewportChangedEvent(oldViewport);
    
    return true;
}

6. PosDimSet() Helper (Lines 184-199)

private void PosDimSet()
{
    SetNeedsLayout();
    
    if (_x is PosAbsolute && _y is PosAbsolute && 
        _width is DimAbsolute && _height is DimAbsolute)
    {
        // Implicit layout for all-absolute
        Layout();
        
        if (SuperView is { } || this is Adornment { Parent: null })
        {
            // Ensure next iteration tries again
            SetNeedsLayout();
        }
    }
}

Execution Timeline

1. User changes View.Width
   └─> Width setter calls PosDimSet()
       └─> SetNeedsLayout() [sets flag = true on this + all descendants + SuperView]
       └─> If all Pos/Dim are Absolute: Layout() [sets flag = false]
           └─> SetRelativeLayout()
               └─> If Frame changes: SetFrame() [sets flag = true again!]

2. Application iteration
   └─> Application.LayoutAndDraw()
       └─> For each runnable with NeedsLayout:
           └─> Layout()
               └─> SetRelativeLayout() [calculates Frame]
               └─> LayoutSubViews() [processes descendants]
                   └─> Sets NeedsLayout = false (or true if dependencies unresolved)

---

Latent Bugs Identified

🐛 Bug #1: SetFrame Always Sets NeedsLayout=true (CRITICAL)

Location: View.Layout.cs:80-112 (SetFrame method)

The Issue:

private bool SetFrame(in Rectangle frame)
{
    if (_frame == frame)
        return false;
    
    _frame = frame;
    SetAdornmentFrames();
    SetNeedsDraw();
    SetNeedsLayout();  // ⚠️ BUG: ALWAYS sets NeedsLayout = true
    
    // ... events ...
    return true;
}

Call Chain:

Layout()
 └─> SetRelativeLayout()
     └─> SetFrame()  // If Frame changes
         └─> SetNeedsLayout()  // Sets flag = true
 └─> LayoutSubViews()
     └─> NeedsLayout = false  // Clears flag at line 780

The Problem:
If SetRelativeLayout() changes the Frame, SetFrame() sets NeedsLayout = true. This happens during the layout operation. While LayoutSubViews() sets the flag to false at the end, the SetRelativeLayout() call happens between Layout() and LayoutSubViews().

Code Evidence:

// View.Layout.cs:520-534
public bool Layout(Size contentSize)
{
    if (SetRelativeLayout(contentSize))  // ← SetFrame called here, sets NeedsLayout=true
    {
        LayoutSubViews();                 // ← Clears NeedsLayout=false at line 780
        SetNeedsDraw();
        return true;
    }
    return false;
}

The Subtle Issue:
After Layout() completes, the view should have NeedsLayout = false. But if SetRelativeLayout() changed the Frame, it momentarily set NeedsLayout = true before LayoutSubViews() cleared it. This can cause race conditions if code checks NeedsLayout between these two calls.

Why It Hasn't Exploded:

1. The PosDimSet() workaround at line 193-197:

if (SuperView is { } || this is Adornment { Parent: null })
{
    SetNeedsLayout();  // Force re-layout in next iteration
}

The comment says "Ensure the next Application iteration tries to layout again" - the developers knew about this issue!

2. The Application iteration loop keeps trying until all views have NeedsLayout = false

Test Evidence:
From FrameTests.cs:228-244:

[Fact]
public void Frame_Set_Sets()
{
    View view = new();
    Assert.True(view.NeedsLayout);  // Initial state
    
    view.Frame = frame;
    Assert.Equal(frame, view.Frame);
    Assert.False(view.NeedsLayout);  // ✓ Passes because Frame setter calls Layout()
}

This test passes because the Frame property setter explicitly calls Layout() at line 70, which calls LayoutSubViews(), which clears the flag.

Impact:

• Low in practice due to workarounds
• High in theory - violates expected behavior
• Could cause issues in timing-sensitive code

Fix:

private bool _isLayouting = false;

private bool SetFrame(in Rectangle frame)
{
    if (_frame == frame)
        return false;
    
    _frame = frame;
    SetAdornmentFrames();
    SetNeedsDraw();
    
    if (!_isLayouting)  // ← Only set flag if NOT during layout
        SetNeedsLayout();
    
    OnFrameChanged(in frame);
    FrameChanged?.Invoke(this, new(in frame));
    
    if (oldViewport != Viewport)
        RaiseViewportChangedEvent(oldViewport);
    
    return true;
}

public bool SetRelativeLayout(Size superviewContentSize)
{
    _isLayouting = true;
    try
    {
        // ... existing code ...
    }
    finally
    {
        _isLayouting = false;
    }
}

---

🐛 Bug #2: SetNeedsLayout Has O(N²) Propagation (PERFORMANCE)

Location: View.Layout.cs:852-923 (SetNeedsLayout method)

The Issue:

public void SetNeedsLayout()
{
    NeedsLayout = true;
    
    // ... set on adornments ...
    
    // Propagate DOWN to descendants
    Stack<View> stack = new(InternalSubViews.Snapshot().ToList());
    while (stack.Count > 0)
    {
        View current = stack.Pop();
        if (!current.NeedsLayout)  // ✓ Good: stops recursion
        {
            current.NeedsLayout = true;
            // ... propagate to descendants ...
        }
    }
    
    // Propagate UP to SuperView
    if (SuperView is { NeedsLayout: false })  // ✓ Checks first
    {
        SuperView?.SetNeedsLayout();  // ⚠️ BAD: Calls full method again!
    }
    
    // Propagate UP to Parent (for Adornments)
    if (this is Adornment adornment && adornment.Parent is { NeedsLayout: false })
    {
        adornment.Parent?.SetNeedsLayout();  // ⚠️ BAD: Calls full method again!
    }
}

The Problem:
When propagating UP to the SuperView, the code checks if SuperView.NeedsLayout == false, then calls SuperView.SetNeedsLayout(). This causes the SuperView to iterate through ALL its descendants again, even though we just came from one of them!

Example Scenario:

Window (SuperView)
  ├─ Border (Adornment)
  │   └─ Button
  ├─ View1
  │   └─ View2
  │       └─ View3 (calls SetNeedsLayout)
  └─ View4

Execution Trace:

1. View3.SetNeedsLayout()
   - Sets View3.NeedsLayout = true
   - No descendants to process
   - Checks View2.NeedsLayout == false → calls View2.SetNeedsLayout()

2. View2.SetNeedsLayout()
   - Sets View2.NeedsLayout = true
   - Iterates descendants: [View3]  ← View3 already set, skipped ✓
   - Checks View1.NeedsLayout == false → calls View1.SetNeedsLayout()

3. View1.SetNeedsLayout()
   - Sets View1.NeedsLayout = true
   - Iterates descendants: [View2, View3]  ← Already set, skipped ✓
   - Checks Window.NeedsLayout == false → calls Window.SetNeedsLayout()

4. Window.SetNeedsLayout()
   - Sets Window.NeedsLayout = true
   - Sets Border.NeedsLayout = true
   - Iterates Border descendants: [Button]
   - Iterates Window descendants: [View1, View2, View3, View4]

Processing Count:

• View3: Processed 1 time (initial call)
• View2: Processed 2 times (own call + Window iteration)
• View1: Processed 2 times (own call + Window iteration)
• View4: Processed 1 time (Window iteration)
• Button: Processed 1 time (Border adornment)

Complexity:

• Worst case: O(N²) where N is the depth of the hierarchy
• In a 10-level deep hierarchy, the bottom view causes ~55 iterations (1+2+3+...+10)

Why It Hasn't Exploded:

1. The check if (!current.NeedsLayout) at line 881 prevents re-setting descendants that are already marked
2. Most Terminal.Gui UIs don't have deeply nested hierarchies (typically 3-5 levels)
3. The operation is still fast enough on modern hardware for typical use cases

Test Coverage Gap:
No tests verify the number of times SetNeedsLayout() is called in a deep hierarchy.

Fix:

public void SetNeedsLayout()
{
    if (NeedsLayout)  // ← Early exit if already set
        return;
    
    NeedsLayout = true;
    
    // Set on adornments
    if (Margin is { SubViews.Count: > 0 })
        Margin.SetNeedsLayout();
    if (Border is { SubViews.Count: > 0 })
        Border.SetNeedsLayout();
    if (Padding is { SubViews.Count: > 0 })
        Padding.SetNeedsLayout();
    
    // Propagate DOWN to descendants
    foreach (View subview in InternalSubViews)
        subview.SetNeedsLayout();  // Will early-exit if already set
    
    // Propagate UP to SuperView
    SuperView?.SetNeedsLayout();  // Will early-exit if already set
    
    // Propagate UP to Parent (for Adornments)
    if (this is Adornment adornment)
        adornment.Parent?.SetNeedsLayout();  // Will early-exit if already set
}

With early-exit, the complexity becomes O(N) because each view is only processed once.

---

🐛 Bug #3: PosDimSet Logic Error with SuperView Check (LOGIC ERROR)

Location: View.Layout.cs:184-199 (PosDimSet method)

The Issue:

private void PosDimSet()
{
    SetNeedsLayout();
    
    if (_x is PosAbsolute && _y is PosAbsolute && 
        _width is DimAbsolute && _height is DimAbsolute)
    {
        Layout();  // Implicit layout
        
        if (SuperView is { } || this is Adornment { Parent: null })
        {
            SetNeedsLayout();  // ⚠️ Re-set flag for next iteration
        }
    }
}

The Condition Analysis:

if (SuperView is { } || this is Adornment { Parent: null })

This evaluates to true when:

• The view has a SuperView (any non-null SuperView), OR
• The view is an Adornment without a Parent

Current Behavior:

View with SuperView     → SetNeedsLayout() is called ✓ (but why?)
View without SuperView  → SetNeedsLayout() is NOT called
Adornment with Parent   → SetNeedsLayout() is NOT called
Adornment without Parent → SetNeedsLayout() is called ✓

The Logic Problem:
Why would we want to call SetNeedsLayout() when the view has a SuperView? If it has a SuperView, the SuperView's layout cycle will handle it in the next iteration.

Probable Intent:
The condition should be:

if (SuperView is null && this is not Adornment { Parent: null })

This would mean: "Set NeedsLayout if the view is a root (no SuperView) but is NOT an orphaned Adornment."

Alternative Intent:
Perhaps the code wants to ensure the SuperView gets flagged:

if (SuperView is { })
{
    SuperView.SetNeedsLayout();  // Flag the SuperView, not this view
}
else if (this is not Adornment { Parent: null })
{
    SetNeedsLayout();  // Flag this view if it's a root
}

Why It Hasn't Exploded:
The Application iteration loop keeps calling Layout() on views with NeedsLayout = true, so even if this creates redundant flagging, it eventually settles.

Test Coverage Gap:
No tests verify:

• The behavior when a view with all-absolute Pos/Dim has a SuperView
• The behavior when an Adornment with all-absolute Pos/Dim has/doesn't have a Parent
• Whether NeedsLayout is set correctly after setting all-absolute Pos/Dim

Impact:

• Causes unnecessary layout iterations
• May cause one extra layout pass per view change
• Low severity but indicates confused intent

Recommendation:

1. Add tests to verify the expected behavior
2. Document the intent of this condition
3. Fix if the logic is indeed incorrect

---

🐛 Bug #4: Frame Setter Bypasses Property Change Events (BREAKING)

Location: View.Layout.cs:54-72 (Frame setter), View.Layout.cs:641-664 (SetRelativeLayout)

The Issue:

public Rectangle Frame
{
    set
    {
        if (SetFrame(value with { Width = Math.Max(value.Width, 0), Height = Math.Max(value.Height, 0) }))
        {
            // BUGBUG: We set the internal fields here to avoid recursion. However, this means that
            // BUGBUG: other logic in the property setters does not get executed. Specifically:
            // BUGBUG: - Reset TextFormatter
            // BUGBUG: - SetLayoutNeeded (not an issue as we explicitly call Layout below)
            // BUGBUG: - If we add property change events for X/Y/Width/Height they will not be invoked
            _x = _frame!.Value.X;
            _y = _frame!.Value.Y;
            _width = _frame!.Value.Width;   // ⚠️ Bypasses Width setter
            _height = _frame!.Value.Height; // ⚠️ Bypasses Height setter
            
            Layout();
        }
    }
}

The Same Issue in SetRelativeLayout:

// Lines 641-664
if (Frame != newFrame)
{
    SetFrame(newFrame);
    
    // BUGBUG: We set the internal fields here to avoid recursion. However, this means that
    // BUGBUG: other logic in the property setters does not get executed. Specifically:
    // BUGBUG: - Reset TextFormatter
    // BUGBUG: - SetLayoutNeeded (not an issue as we explicitly call Layout below)
    // BUGBUG: - If we add property change events for X/Y/Width/Height they will not be invoked
    if (_x is PosAbsolute)
        _x = Frame.X;
    if (_y is PosAbsolute)
        _y = Frame.Y;
    if (_width is DimAbsolute)
        _width = Frame.Width;   // ⚠️ Bypasses Width setter
    if (_height is DimAbsolute)
        _height = Frame.Height; // ⚠️ Bypasses Height setter
}

What Gets Bypassed:
The Width setter (lines 413-437) does:

public Dim Width
{
    set
    {
        CWPPropertyHelper.ChangeProperty(
            this,
            ref _width,
            value,
            OnWidthChanging,    // ⚠️ NOT CALLED
            WidthChanging,      // ⚠️ NOT RAISED
            newValue =>
            {
                _width = newValue;
                TextFormatter.ConstrainToWidth = null;  // ⚠️ NOT RESET
                PosDimSet();
            },
            OnWidthChanged,     // ⚠️ NOT CALLED
            WidthChanged,       // ⚠️ NOT RAISED
            out Dim _
        );
    }
}

Consequences:

1. WidthChanging event is NOT raised - handlers can't intercept/cancel the change
2. OnWidthChanging() is NOT called - subclass overrides are bypassed
3. WidthChanged event is NOT raised - handlers don't know Width changed
4. OnWidthChanged() is NOT called - subclass overrides are bypassed
5. TextFormatter.ConstrainToWidth is NOT reset - may use stale value
6. Same issues for Height, HeightChanging, HeightChanged

Impact:

// Developer writes this code:
view.WidthChanged += (s, e) =>
{
    Debug.WriteLine($"Width changed from {e.OldValue} to {e.NewValue}");
};

// Then calls:
view.Frame = new Rectangle(0, 0, 100, 50);

// Expected: WidthChanged event fires
// Actual: Event does NOT fire! ❌

Why It Hasn't Exploded:

1. Most code doesn't subscribe to WidthChanging/WidthChanged events
2. The TextFormatter reset issue is mitigated by SetTextFormatterSize() being called
3. Layout still works because Layout() is called explicitly

Test Coverage Gap:
No tests verify that WidthChanged/HeightChanged events fire when:

• Frame is set directly
• SetRelativeLayout() changes the frame

Severity: MEDIUM-HIGH
This is a breaking change to the event system and could prevent developers from:

• Implementing validation logic in WidthChanging
• Reacting to size changes in WidthChanged
• Using data binding libraries that rely on property change notifications

Possible Fixes:

Option 1: Manually raise events

if (_width is DimAbsolute)
{
    Dim oldWidth = _width;
    _width = Frame.Width;
    TextFormatter.ConstrainToWidth = null;
    
    OnWidthChanged(new ValueChangedEventArgs<Dim>(oldWidth, _width));
    WidthChanged?.Invoke(this, new ValueChangedEventArgs<Dim>(oldWidth, _width));
}

Option 2: Call property setters (may cause recursion)

if (_width is DimAbsolute)
{
    Width = Frame.Width;  // Calls setter, triggers events
}

Option 3: Document the limitation
Add to XML docs:

/// <remarks>
/// NOTE: When setting Frame directly or during layout operations,
/// the WidthChanging, WidthChanged, HeightChanging, and HeightChanged
/// events will NOT be raised. Subscribe to FrameChanged instead.
/// </remarks>

---

🐛 Bug #5: LayoutSubViews Clears NeedsLayout Before Events Fire (TIMING)

Location: View.Layout.cs:709-784 (LayoutSubViews method)

The Issue:

internal void LayoutSubViews()
{
    if (!NeedsLayout)
        return;
    
    // ... layout logic ...
    
    // Clear the flag BEFORE events
    NeedsLayout = layoutStillNeeded;  // ⚠️ Line 780
    
    // Events fire AFTER flag is cleared
    OnSubViewsLaidOut(new(contentSize));           // Line 782
    SubViewsLaidOut?.Invoke(this, new(contentSize)); // Line 783
}

The Problem:
If an event handler modifies the layout, it will call SetNeedsLayout(), which sets NeedsLayout = true. However, this happens after the layout has been marked as complete!

Example Scenario:

view.SubViewsLaidOut += (s, e) =>
{
    // Adjust child based on final layout
    child.Width = view.Frame.Width / 2;  // ← Triggers SetNeedsLayout()
};

view.Layout();
// At this point:
// - NeedsLayout was set to false at line 780
// - SubViewsLaidOut event fired, handler called SetNeedsLayout()
// - NeedsLayout is now true again!
// - But Layout() thinks it succeeded and returns true

Code Flow:

1. Layout() calls LayoutSubViews()
2. LayoutSubViews() does layout work
3. LayoutSubViews() sets NeedsLayout = false  (line 780)
4. LayoutSubViews() fires SubViewsLaidOut event (line 783)
5. Event handler modifies child.Width
6. child.Width setter calls SetNeedsLayout()
7. child.NeedsLayout = true
8. child propagates up: view.SetNeedsLayout()
9. view.NeedsLayout = true  ← But layout just finished!

Actual vs Expected:

Actual:   Layout → Clear flag → Fire events → Events set flag
Expected: Layout → Fire events → Clear flag (if events didn't set it)

Why It Hasn't Exploded:
The Application iteration loop keeps calling Layout() on views with NeedsLayout = true, so the view gets laid out again in the next iteration.

Impact:

• Causes one extra layout pass
• Event handlers can't reliably modify layout without causing re-layout
• May cause confusion when debugging layout issues

Fix:

internal void LayoutSubViews()
{
    if (!NeedsLayout)
        return;
    
    // ... layout logic ...
    
    // Fire events BEFORE clearing flag
    OnSubViewsLaidOut(new(contentSize));
    SubViewsLaidOut?.Invoke(this, new(contentSize));
    
    // Clear flag AFTER events (unless events set it again)
    if (!layoutStillNeeded)
        NeedsLayout = false;
}

Alternative Fix: Document the behavior

/// <remarks>
/// The SubViewsLaidOut event is raised AFTER NeedsLayout has been cleared.
/// If your event handler modifies the layout, it will trigger another
/// layout pass in the next Application iteration.
/// </remarks>

---

🐛 Bug #6: SetRelativeLayout Returns True But Leaves NeedsLayout=True (CONSISTENCY)

Location: View.Layout.cs:577-692 (SetRelativeLayout method)

The Issue:

public bool SetRelativeLayout(Size superviewContentSize)
{
    // ... calculate newFrame ...
    
    if (Frame != newFrame)
    {
        SetFrame(newFrame);  // ⚠️ Sets NeedsLayout = true!
        // ... update internal fields ...
    }
    
    // ... set TextFormatter constraints ...
    
    return true;  // ⚠️ Returns "success", but NeedsLayout is true!
}

The Problem:
SetRelativeLayout() returns a boolean:

• true = "Successfully set relative layout"
• false = "Failed due to dependency not ready"

However, if it changes the Frame, SetFrame() sets NeedsLayout = true. So the method returns true (success) even though the view still NeedsLayout.

Caller's Perspective:

// View.Layout.cs:520-534
public bool Layout(Size contentSize)
{
    if (SetRelativeLayout(contentSize))  // Returns true
    {
        LayoutSubViews();                 // This will clear NeedsLayout
        SetNeedsDraw();
        return true;  // We return true = "layout complete"
    }
    return false;  // We return false = "layout failed"
}

The Layout() method assumes that if SetRelativeLayout() returns true, the layout is complete. But in reality, NeedsLayout was just set to true by SetFrame()!

State After Layout():

SetRelativeLayout() returns true
  → Layout() calls LayoutSubViews()
    → LayoutSubViews() sets NeedsLayout = false
      → Result: NeedsLayout is false ✓

BUT during SetRelativeLayout():
  → SetFrame() set NeedsLayout = true
    → This propagated to SuperView
      → SuperView now has NeedsLayout = true
        → Next iteration will layout SuperView again

Why It Hasn't Exploded:
The Application iteration loop processes all views with NeedsLayout = true, so the SuperView will get laid out in the next pass.

Impact:

• Causes cascading layout operations
• SuperView gets laid out even though its own properties didn't change
• Can cause performance issues in complex UIs

This is related to Bug #1 - both stem from SetFrame() always calling SetNeedsLayout().

Fix: Same as Bug #1 - use _isLayouting flag to prevent SetFrame() from setting NeedsLayout during layout operations.

---

Test Coverage Analysis

Existing Tests

Tests that verify NeedsLayout behavior:

1. LayoutTests.Set_X_PosAbsolute_Layout_Is_Implicit (lines 699-722)

   • ✓ Verifies NeedsLayout = false after setting all-absolute X
   • ✗ Doesn't verify SuperView propagation
   • ✗ Doesn't verify intermediate states

2. LayoutTests.Set_Y_PosAbsolute_Layout_Is_Implicit (lines 755-778)

   • ✓ Verifies NeedsLayout = false after setting all-absolute Y
   • ✗ Doesn't verify SuperView propagation
   • ✗ Doesn't verify intermediate states

3. LayoutTests.Set_Width_DimAbsolute_Layout_Is_Implicit (lines 617-640)

   • ✓ Verifies NeedsLayout = false after setting absolute Width
   • ✗ Doesn't verify SuperView propagation
   • ✗ Doesn't verify intermediate states

4. LayoutTests.Set_Height_DimAbsolute_Layout_Is_Implicit (lines 574-614)

   • ✓ Verifies NeedsLayout = false after setting absolute Height
   • ✗ Doesn't verify SuperView propagation
   • ✗ Doesn't verify intermediate states

5. LayoutTests.Set_X_Non_PosAbsolute_Explicit_Layout_Required (lines 669-696)

   • ✓ Verifies NeedsLayout = true after setting non-absolute X
   • ✗ Doesn't verify that Frame is NOT changed

6. LayoutTests.LayoutSubViews_LayoutStarted_Complete (lines 357-406)

   • ✓ Verifies events fire correct number of times
   • ✗ Doesn't verify NeedsLayout state before/after events

7. AdornmentTests.Setting_Thickness_Causes_Parent_Layout (lines 391-408)

   • ✓ Verifies parent.NeedsLayout = true after setting adornment thickness
   • ✓ Verifies adornment.NeedsLayout = true after setting thickness
   • ✗ Doesn't verify propagation to descendants

8. FrameTests.Frame_Set_Sets (lines 228-244)

   • ✓ Verifies NeedsLayout = false after setting Frame
   • ✗ Only tests end state, not intermediate states
   • ✗ Doesn't verify that Width/Height properties are updated

Test Coverage Gaps

Gap #1: No test verifies NeedsLayout state DURING layout

// Needed test:
[Fact]
public void SetRelativeLayout_Changing_Frame_Temporarily_Sets_NeedsLayout()
{
    View view = new();
    view.X = 1;
    view.Y = 2;
    view.Width = 3;
    view.Height = 4;
    
    bool needsLayoutDuringSetFrame = false;
    view.FrameChanged += (s, e) =>
    {
        needsLayoutDuringSetFrame = ((View)s!).NeedsLayout;
    };
    
    view.SetRelativeLayout(new Size(100, 100));
    
    // During SetFrame, NeedsLayout should be true
    Assert.True(needsLayoutDuringSetFrame);
    
    // But we haven't called LayoutSubViews yet
    // So NeedsLayout should STILL be true
    Assert.True(view.NeedsLayout);
}

Gap #2: No test verifies O(N²) propagation cost

// Needed test:
[Fact]
public void SetNeedsLayout_Deep_Hierarchy_Performance()
{
    // Create a 10-level deep hierarchy
    View root = new();
    View current = root;
    for (int i = 0; i < 10; i++)
    {
        View child = new() { Id = $"Level{i}" };
        current.Add(child);
        current = child;
    }
    
    // Count how many times SetNeedsLayout is called
    int callCount = 0;
    // Hook into SetNeedsLayout (would need to make it virtual or add event)
    
    // Call SetNeedsLayout on the deepest child
    current.SetNeedsLayout();
    
    // Should be ~11 calls (one per level), not ~55 (N²)
    Assert.InRange(callCount, 10, 15);  // Allow some overhead
}

Gap #3: No test verifies WidthChanged/HeightChanged events NOT firing

// Needed test:
[Fact]
public void Frame_Set_Does_Not_Fire_WidthChanged_Event()
{
    View view = new();
    view.Width = 10;  // Set initial width
    
    bool widthChangedFired = false;
    view.WidthChanged += (s, e) => widthChangedFired = true;
    
    view.Frame = new Rectangle(0, 0, 20, 10);  // Change width to 20
    
    // BUG: Event does NOT fire!
    Assert.False(widthChangedFired);  // Current behavior (documents the bug)
    // Assert.True(widthChangedFired);  // Expected behavior
}

Gap #4: No test verifies SubViewsLaidOut handler setting NeedsLayout

// Needed test:
[Fact]
public void SubViewsLaidOut_Handler_Setting_NeedsLayout_Causes_Relayout()
{
    View view = new();
    View child = new() { X = 0, Y = 0, Width = 10, Height = 10 };
    view.Add(child);
    
    int layoutCount = 0;
    view.SubViewsLaidOut += (s, e) =>
    {
        layoutCount++;
        if (layoutCount == 1)
        {
            // Modify child, which should trigger another layout
            child.Width = 20;
        }
    };
    
    view.Layout();
    
    // First layout completes, event fires, handler changes child
    // This should cause view.NeedsLayout = true
    Assert.True(view.NeedsLayout);
    
    // Second layout should happen
    view.Layout();
    
    // Event should have fired twice
    Assert.Equal(2, layoutCount);
}

Gap #5: No test verifies PosDimSet SuperView condition

// Needed test:
[Fact]
public void PosDimSet_With_SuperView_Sets_NeedsLayout()
{
    View superView = new() { Width = 100, Height = 100 };
    View view = new();
    superView.Add(view);
    
    view.X = 1;
    view.Y = 2;
    view.Width = 3;
    view.Height = 4;
    
    // After setting all absolute, NeedsLayout should be...?
    // Current behavior: NeedsLayout = true (due to PosDimSet logic)
    Assert.True(view.NeedsLayout);
    
    // Is this correct? Or should it be false?
}

Gap #6: No test verifies propagation to SuperView

// Needed test:
[Fact]
public void SetNeedsLayout_Propagates_To_SuperView()
{
    View superView = new();
    View child = new();
    superView.Add(child);
    
    superView.BeginInit();
    superView.EndInit();  // Clear initial NeedsLayout
    
    Assert.False(superView.NeedsLayout);
    Assert.False(child.NeedsLayout);
    
    child.SetNeedsLayout();
    
    Assert.True(child.NeedsLayout);
    Assert.True(superView.NeedsLayout);  // Should propagate up
}

Gap #7: No test verifies TextFormatter.ConstrainToWidth reset

// Needed test:
[Fact]
public void Frame_Set_Resets_TextFormatter_ConstrainToWidth()
{
    View view = new() { Width = 10, Height = 10 };
    view.BeginInit();
    view.EndInit();
    view.Layout();
    
    Assert.Equal(10, view.TextFormatter.ConstrainToWidth);
    
    view.Frame = new Rectangle(0, 0, 20, 10);
    
    // BUG: TextFormatter is NOT reset!
    Assert.Equal(10, view.TextFormatter.ConstrainToWidth);  // Current (bug)
    // Assert.Equal(20, view.TextFormatter.ConstrainToWidth);  // Expected
}

---

Recommendations

Priority 1: Critical Bugs (Fix Immediately)

1. Bug #1: SetFrame Always Sets NeedsLayout=true

• Impact: High in theory, low in practice due to workarounds
• Fix: Add _isLayouting flag to prevent setting NeedsLayout during layout
• Test: Add test to verify NeedsLayout state doesn't change during SetRelativeLayout

2. Bug #4: Frame Setter Bypasses Property Change Events

• Impact: High - breaks event system and data binding
• Fix: Either manually raise events or document the limitation
• Test: Add tests to verify events fire (or explicitly test they DON'T fire and document it)

Priority 2: Performance Issues (Fix Soon)

3. Bug #2: SetNeedsLayout Has O(N²) Propagation

• Impact: Medium - causes performance issues in deep hierarchies
• Fix: Add early-exit to SetNeedsLayout if already set
• Test: Add performance test for deep hierarchies

Priority 3: Logic Errors (Fix When Convenient)

4. Bug #3: PosDimSet Logic Error

• Impact: Low - causes extra layout passes but no functional issues
• Fix: Clarify intent, add tests, fix condition if wrong
• Test: Add tests to verify expected behavior with/without SuperView

5. Bug #5: LayoutSubViews Clears NeedsLayout Before Events

• Impact: Low - causes one extra layout pass if event handler modifies layout
• Fix: Move event firing before clearing NeedsLayout, or document behavior
• Test: Add test to verify event handler can modify layout

6. Bug #6: SetRelativeLayout Returns True But Leaves NeedsLayout=True

• Impact: Low - this is a consequence of Bug There is a problem with the high-intensity colors, they are not showing up #1
• Fix: Fixed automatically when Bug There is a problem with the high-intensity colors, they are not showing up #1 is fixed
• Test: Same as Bug There is a problem with the high-intensity colors, they are not showing up #1

Proposed Fix Implementation Order

1. Add tests first to document current behavior
2. Fix Bug There is a problem with the high-intensity colors, they are not showing up #1 (SetFrame NeedsLayout) - this also fixes Bug Bug in FindDeepestView  #6
3. Fix Bug Add resizing support to the core #2 (O(N²) propagation) - performance improvement
4. Fix or Document Bug Add mouse event support to the core. #4 (property events) - decide on breaking change policy
5. Fix or Document Bug Make Windows draggable with the mouse. #5 (event timing) - low priority
6. Investigate Bug Redraw problems #3 (PosDimSet logic) - may not be a bug, just unclear

Code Quality Improvements

1. Add XML documentation explaining NeedsLayout lifecycle
2. Add debug assertions to catch misuse
3. Add performance counters to detect O(N²) behavior
4. Refactor PosDimSet to make intent clear
5. Consider making SetNeedsLayout virtual to allow testing/hooking

---

Conclusion

The NeedsLayout system in Terminal.Gui has 6 latent bugs ranging from critical (event system failures) to minor (extra layout passes). The system works in practice due to:

1. Workarounds in PosDimSet() that force re-layout
2. Application iteration loop that keeps trying until all views settle
3. Limited deep hierarchies in typical usage
4. Test coverage focused on end results, not intermediate states

The most critical issues are:

• Bug There is a problem with the high-intensity colors, they are not showing up #1 & Bug in FindDeepestView  #6: SetFrame() always sets NeedsLayout = true, causing state inconsistency
• Bug Add mouse event support to the core. #4: Property change events don't fire when Frame is set, breaking event system
• Bug Add resizing support to the core #2: O(N²) propagation in deep hierarchies causes performance issues

Recommended Action:

1. Add tests to document current behavior
2. Fix Bug There is a problem with the high-intensity colors, they are not showing up #1 using _isLayouting flag
3. Fix Bug Add resizing support to the core #2 with early-exit optimization
4. Make decision on Bug Add mouse event support to the core. #4: fix or document as limitation
5. Document or fix remaining bugs based on priority

This analysis should guide future refactoring and help prevent similar issues.

---

Document Version: 1.0
Author: AI Analysis
Date: 2025-01-XX
Status: Draft for Review

[mrazza]

@tig curious about flow (2) and bug (2) here. It's surprising to me that the act of changing the layout for a single View requires all views in the entire application to re-layout. For instance, changing the text of a Label whose width is Dim.auto() results in SetNeedsLayout (and then SetNeedsDraw, as a result of the re-layout) for the entire application even if no other views were impacted. For views whose content change frequently (say, a clock) this means the application is continually recomputing layout and doing a full redraw.

It's possible I'm missing something here; I haven't had a chance to write a unit test sufficiently complex enough to confirm this behavior in a hermetic test but this appears to both:

1. Be the case in my application and
2. Seem the evident behavior because of the SuperView and ChildView logic described in (2) above.

[tznind]

Not exactly related, but you can see internal engine logs and metrics:

See
https://github.com/gui-cs/Terminal.Gui/blob/v2_develop/docfx%2Fdocs%2Flogging.md

This includes redraw counter

[tig]

As this issue says needslayout is buggy.

I'm not sure what to tell you other than this will eventually be fixed which will drastically improve performance.

The layout engine is quite robust at this point relative to actually how things are laid out. However it has never been efficient and attempts over the years to hack "needslayout" has left the things quite messy.

[BDisp]

If changing the text of a Label whose width is Dim.Auto() and its SuperView, which is not a Runnable, is also Dim.Auto, it will also increase the width of the SuperView. If there are other views that depend on the size of this SuperView to position or resize, it should also result in SetNeedsLayout for those views. The problem is that we only know about this change after the Layout is called and if these views were previously defined with SetNeedsDraw, otherwise, the Layout will not be processed.
It is much more complex when there are many conditions at play. One idea that comes to mind is that when the Layout is called, if it results in a change to the Frame, then scan to check if any views depend on the current view's layout, and if so, set those views with SetNeedsLayout so they can be properly arranged before drawing the entire application.

[tznind]

Layout and redraw should be capped at once per iteration. Please can you confirm if you are actually seeing a slow down or if it is just your gut feeling?

Also we should probably not raise layout needed if dim auto computes the same dimensiona as before text changed

[BDisp]

Layout and redraw should be capped at once per iteration. Please can you confirm if you are actually seeing a slow down or if it is just your gut feeling?

It is imperative that layout and redesign be limited to once per iteration. It used to be slower than it is now, although I sometimes notice a slight slowdown. Does this happen with any driver or a specific one?

Also we should probably not raise layout needed if dim auto computes the same dimensiona as before text changed

This would be ideal instead of propagating upwards in the hierarchy because it often doesn't affect other views. I understand that it's not a linear procedure and requires dealing with many constraints, although it can be improved.