Browse Source

Refactor LayoutAndDraw and fix ClearNeedsDraw bugs

Refactored the `LayoutAndDraw` method in `ApplicationImpl.Screen.cs` to improve clarity, naming consistency, and redraw logic. Enhanced handling of the `Driver` object to optimize redraws.

Simplified `IterationImpl` in `ApplicationMainLoop.cs` by commenting out redundant checks. Fixed a bug in `SetCursor` to ensure null safety and improve cursor positioning logic.

Modified `ClearNeedsDraw` in `View.Drawing.cs` to prevent premature clearing of the `SuperView`'s `SubViewNeedsDraw` flag. Added explanatory comments to clarify the behavior.

Introduced new unit tests in `NeedsDrawTests.cs` to verify the correctness of `ClearNeedsDraw`:
- Ensured sibling views do not prematurely clear `SuperView`'s flags.
- Verified proper clearing of flags for views, adornments, and descendants.

Improved test coverage and added detailed comments to document expected behavior.
Tig 1 week ago
parent
commit
6049857813

+ 17 - 14
Terminal.Gui/App/ApplicationImpl.Screen.cs

@@ -1,4 +1,3 @@
-
 namespace Terminal.Gui.App;
 
 internal partial class ApplicationImpl
@@ -150,17 +149,6 @@ internal partial class ApplicationImpl
     /// <inheritdoc/>
     public void LayoutAndDraw (bool forceRedraw = false)
     {
-        List<View?> tops = [.. SessionStack!.Select(r => r.Runnable! as View)!];
-
-        if (Popover?.GetActivePopover () as View is { Visible: true } visiblePopover)
-        {
-            visiblePopover.SetNeedsDraw ();
-            visiblePopover.SetNeedsLayout ();
-            tops.Insert (0, visiblePopover);
-        }
-
-        bool neededLayout = View.Layout (tops.ToArray ().Reverse ()!, Screen.Size);
-
         if (ClearScreenNextIteration)
         {
             forceRedraw = true;
@@ -172,13 +160,28 @@ internal partial class ApplicationImpl
             Driver?.ClearContents ();
         }
 
-        if (Driver is { })
+        List<View?> views = [.. SessionStack!.Select (r => r.Runnable! as View)!];
+
+        if (Popover?.GetActivePopover () as View is { Visible: true } visiblePopover)
+        {
+            visiblePopover.SetNeedsDraw ();
+            visiblePopover.SetNeedsLayout ();
+            views.Insert (0, visiblePopover);
+        }
+
+        bool neededLayout = View.Layout (views.ToArray ().Reverse ()!, Screen.Size);
+
+        bool needsDraw = forceRedraw || views.Any (v => v is { NeedsDraw: true } or { SubViewNeedsDraw: true });
+
+        if (Driver is { } && (neededLayout || needsDraw))
         {
             Logging.Redraws.Add (1);
+            Logging.Trace ("LayoutAndDraw");
 
             Driver.Clip = new (Screen);
 
-            View.Draw (views: tops!, neededLayout || forceRedraw);
+            View.Draw (views: views.ToArray ().Cast<View> ()!, neededLayout || forceRedraw);
+
             Driver.Clip = new (Screen);
             Driver?.Refresh ();
         }

+ 5 - 4
Terminal.Gui/App/MainLoop/ApplicationMainLoop.cs

@@ -137,7 +137,7 @@ public class ApplicationMainLoop<TInputRecord> : IApplicationMainLoop<TInputReco
         // Pull any input events from the input queue and process them
         InputProcessor.ProcessQueue ();
 
-        if (App?.TopRunnableView != null)
+        //if (App?.TopRunnableView != null)
         {
             SizeMonitor.Poll ();
 
@@ -155,10 +155,11 @@ public class ApplicationMainLoop<TInputRecord> : IApplicationMainLoop<TInputReco
 
     private void SetCursor ()
     {
-        View? mostFocused = App?.TopRunnableView!.MostFocused;
+        View? mostFocused = App?.TopRunnableView?.MostFocused;
 
         if (mostFocused == null)
         {
+            Output.SetCursorVisibility (CursorVisibility.Invisible);
             return;
         }
 
@@ -167,9 +168,9 @@ public class ApplicationMainLoop<TInputRecord> : IApplicationMainLoop<TInputReco
         if (to.HasValue)
         {
             // Translate to screen coordinates
-            to = mostFocused.ViewportToScreen (to.Value);
+            Point screenPos = mostFocused.ViewportToScreen (to.Value);
 
-            Output.SetCursorPosition (to.Value.X, to.Value.Y);
+            Output.SetCursorPosition (screenPos.X, screenPos.Y);
             Output.SetCursorVisibility (mostFocused.CursorVisibility);
         }
         else

+ 3 - 4
Terminal.Gui/ViewBase/View.Drawing.cs

@@ -901,10 +901,9 @@ public partial class View // Drawing APIs
             subview.ClearNeedsDraw ();
         }
 
-        if (SuperView is { })
-        {
-            SuperView.SubViewNeedsDraw = false;
-        }
+        // DO NOT clear SuperView.SubViewNeedsDraw here!
+        // The SuperView will clear its own SubViewNeedsDraw after it has drawn all its subviews.
+        // If we clear it here, and this view has siblings that still need drawing, we'll break the draw system.
 
         // This ensures LineCanvas' get redrawn
         if (!SuperViewRendersLineCanvas)

+ 170 - 0
Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs

@@ -311,4 +311,174 @@ public class NeedsDrawTests : FakeDriverBase
         Assert.Equal (new (1, 1, 5, 5), view.Viewport);
         Assert.Equal (new (1, 1, 5, 5), view.NeedsDrawRect);
     }
+
+    [Fact]
+    public void ClearNeedsDraw_WithSiblings_DoesNotClearSuperViewSubViewNeedsDraw ()
+    {
+        // This test verifies the fix for the bug where a subview clearing its NeedsDraw
+        // would incorrectly clear the superview's SubViewNeedsDraw flag, even if other siblings
+        // still needed drawing.
+
+        IDriver driver = CreateFakeDriver (80, 25);
+        driver.Clip = new Region (driver.Screen);
+
+        var superView = new View
+        {
+            X = 0,
+            Y = 0,
+            Width = 50,
+            Height = 50,
+            Driver = driver
+        };
+
+        var subView1 = new View { X = 0, Y = 0, Width = 10, Height = 10, Id = "SubView1" };
+        var subView2 = new View { X = 0, Y = 10, Width = 10, Height = 10, Id = "SubView2" };
+        var subView3 = new View { X = 0, Y = 20, Width = 10, Height = 10, Id = "SubView3" };
+
+        superView.Add (subView1, subView2, subView3);
+        superView.BeginInit ();
+        superView.EndInit ();
+        superView.LayoutSubViews ();
+
+        // All subviews should need drawing initially
+        Assert.True (subView1.NeedsDraw);
+        Assert.True (subView2.NeedsDraw);
+        Assert.True (subView3.NeedsDraw);
+        Assert.True (superView.SubViewNeedsDraw);
+
+        // Draw subView1 - this will call ClearNeedsDraw() on subView1
+        subView1.Draw ();
+
+        // SubView1 should no longer need drawing
+        Assert.False (subView1.NeedsDraw);
+
+        // But subView2 and subView3 still need drawing
+        Assert.True (subView2.NeedsDraw);
+        Assert.True (subView3.NeedsDraw);
+
+        // THE BUG: Before the fix, subView1.ClearNeedsDraw() would set superView.SubViewNeedsDraw = false
+        // even though subView2 and subView3 still need drawing.
+        // After the fix, superView.SubViewNeedsDraw should still be true because subView2 and subView3 need drawing.
+        Assert.True (superView.SubViewNeedsDraw, "SuperView's SubViewNeedsDraw should still be true because subView2 and subView3 still need drawing");
+
+        // Now draw subView2
+        subView2.Draw ();
+        Assert.False (subView2.NeedsDraw);
+        Assert.True (subView3.NeedsDraw);
+
+        // SuperView should still have SubViewNeedsDraw = true because subView3 needs drawing
+        Assert.True (superView.SubViewNeedsDraw, "SuperView's SubViewNeedsDraw should still be true because subView3 still needs drawing");
+
+        // Now draw subView3
+        subView3.Draw ();
+        Assert.False (subView3.NeedsDraw);
+
+        // SuperView should STILL have SubViewNeedsDraw = true because it hasn't been cleared by the superview itself
+        // Only the superview's own ClearNeedsDraw() should clear this flag
+        Assert.True (superView.SubViewNeedsDraw, "SuperView's SubViewNeedsDraw should only be cleared by superView.ClearNeedsDraw(), not by subviews");
+
+        // Finally, draw the superview - this will clear SubViewNeedsDraw
+        superView.Draw ();
+        Assert.False (superView.SubViewNeedsDraw, "SuperView's SubViewNeedsDraw should now be false after superView.Draw()");
+        Assert.False (subView1.NeedsDraw);
+        Assert.False (subView2.NeedsDraw);
+        Assert.False (subView3.NeedsDraw);
+    }
+
+    [Fact]
+    public void ClearNeedsDraw_ClearsOwnFlags ()
+    {
+        // Verify that ClearNeedsDraw properly clears the view's own flags
+        IDriver driver = CreateFakeDriver (80, 25);
+        driver.Clip = new Region (driver.Screen);
+
+        var view = new View
+        {
+            X = 0,
+            Y = 0,
+            Width = 20,
+            Height = 20,
+            Driver = driver
+        };
+        view.BeginInit ();
+        view.EndInit ();
+        view.LayoutSubViews ();
+
+        Assert.True (view.NeedsDraw);
+        Assert.Equal (view.Viewport, view.NeedsDrawRect);
+
+        view.Draw ();
+
+        Assert.False (view.NeedsDraw);
+        Assert.Equal (Rectangle.Empty, view.NeedsDrawRect);
+        Assert.False (view.SubViewNeedsDraw);
+    }
+
+    [Fact]
+    public void ClearNeedsDraw_ClearsAdornments ()
+    {
+        // Verify that ClearNeedsDraw clears adornment flags
+        IDriver driver = CreateFakeDriver (80, 25);
+        driver.Clip = new Region (driver.Screen);
+
+        var view = new View
+        {
+            X = 0,
+            Y = 0,
+            Width = 20,
+            Height = 20,
+            Driver = driver
+        };
+        view.Border!.Thickness = new Thickness (1);
+        view.Padding!.Thickness = new Thickness (1);
+        view.BeginInit ();
+        view.EndInit ();
+        view.LayoutSubViews ();
+
+        Assert.True (view.Border!.NeedsDraw);
+        Assert.True (view.Padding!.NeedsDraw);
+
+        view.Draw ();
+
+        Assert.False (view.Border!.NeedsDraw);
+        Assert.False (view.Padding!.NeedsDraw);
+    }
+
+    [Fact]
+    public void ClearNeedsDraw_PropagatesDownToAllSubViews ()
+    {
+        // Verify that ClearNeedsDraw clears flags on all descendants
+        IDriver driver = CreateFakeDriver (80, 25);
+        driver.Clip = new Region (driver.Screen);
+
+        var topView = new View
+        {
+            X = 0,
+            Y = 0,
+            Width = 100,
+            Height = 100,
+            Driver = driver
+        };
+
+        var middleView = new View { X = 10, Y = 10, Width = 50, Height = 50 };
+        var bottomView = new View { X = 5, Y = 5, Width = 20, Height = 20 };
+
+        topView.Add (middleView);
+        middleView.Add (bottomView);
+        topView.BeginInit ();
+        topView.EndInit ();
+        topView.LayoutSubViews ();
+
+        Assert.True (topView.NeedsDraw);
+        Assert.True (middleView.NeedsDraw);
+        Assert.True (bottomView.NeedsDraw);
+
+        topView.Draw ();
+
+        Assert.False (topView.NeedsDraw);
+        Assert.False (topView.SubViewNeedsDraw);
+        Assert.False (middleView.NeedsDraw);
+        Assert.False (middleView.SubViewNeedsDraw);
+        Assert.False (bottomView.NeedsDraw);
+    }
 }