diff --git a/Examples/UICatalog/Scenarios/CombiningMarks.cs b/Examples/UICatalog/Scenarios/CombiningMarks.cs index 4ffa787b9..eae0d05f1 100644 --- a/Examples/UICatalog/Scenarios/CombiningMarks.cs +++ b/Examples/UICatalog/Scenarios/CombiningMarks.cs @@ -10,11 +10,8 @@ public class CombiningMarks : Scenario Application.Init (); var top = new Runnable (); - top.DrawComplete += (s, e) => + top.DrawingContent += (s, e) => { - // Forces reset _lineColsOffset because we're dealing with direct draw - Application.TopRunnableView!.SetNeedsDraw (); - var i = -1; top.Move (0, ++i); top.AddStr ("Terminal.Gui supports all combining sequences that can be rendered as an unique grapheme."); diff --git a/Terminal.Gui/ViewBase/Adornment/Adornment.cs b/Terminal.Gui/ViewBase/Adornment/Adornment.cs index 6852efea4..19056322d 100644 --- a/Terminal.Gui/ViewBase/Adornment/Adornment.cs +++ b/Terminal.Gui/ViewBase/Adornment/Adornment.cs @@ -88,7 +88,7 @@ public class Adornment : View, IDesignable protected override IApplication? GetApp () => Parent?.App; /// - protected override IDriver? GetDriver () => Parent?.Driver ?? base.GetDriver(); + protected override IDriver? GetDriver () => Parent?.Driver ?? base.GetDriver (); // If a scheme is explicitly set, use that. Otherwise, use the scheme of the parent view. private Scheme? _scheme; @@ -187,7 +187,7 @@ public class Adornment : View, IDesignable Thickness.Draw (Driver, ViewportToScreen (Viewport), Diagnostics, ToString ()); } - NeedsDraw = true; + SetNeedsDraw (); return true; } diff --git a/Terminal.Gui/ViewBase/Adornment/Margin.cs b/Terminal.Gui/ViewBase/Adornment/Margin.cs index ae65cbff9..2c123796b 100644 --- a/Terminal.Gui/ViewBase/Adornment/Margin.cs +++ b/Terminal.Gui/ViewBase/Adornment/Margin.cs @@ -79,7 +79,7 @@ public class Margin : Adornment if (view.Margin is { } margin && margin.Thickness != Thickness.Empty && margin.GetCachedClip () != null) { - margin.NeedsDraw = true; + margin.SetNeedsDraw (); Region? saved = view.GetClip (); view.SetClip (margin.GetCachedClip ()); margin.Draw (); @@ -88,7 +88,7 @@ public class Margin : Adornment } Debug.Assert (view.NeedsDraw == false); - view.NeedsDraw = false; + view.ClearNeedsDraw (); foreach (var subview in view.SubViews) { diff --git a/Terminal.Gui/ViewBase/View.Drawing.cs b/Terminal.Gui/ViewBase/View.Drawing.cs index cd3bd6905..7ed6db098 100644 --- a/Terminal.Gui/ViewBase/View.Drawing.cs +++ b/Terminal.Gui/ViewBase/View.Drawing.cs @@ -28,17 +28,33 @@ public partial class View // Drawing APIs view.Draw (context); } - // Draw the margins (those with Shadows) last to ensure they are drawn on top of the content. + // Draw the margins last to ensure they are drawn on top of the content. Margin.DrawMargins (viewsArray); // DrawMargins may have caused some views have NeedsDraw/NeedsSubViewDraw set; clear them all. foreach (View view in viewsArray) { view.ClearNeedsDraw (); - // ClearNeedsDraw does not clear view.SuperView.SubViewsNeedDraw, so we have to do it here - if (view.SuperView is { }) + } + + // After all peer views have been drawn and cleared, we can now clear the SuperView's SubViewNeedsDraw flag. + // ClearNeedsDraw() does not clear SuperView.SubViewNeedsDraw (by design, to avoid premature clearing + // when siblings still need drawing), so we must do it here after ALL peers are processed. + // We only clear the flag if ALL the SuperView's subviews no longer need drawing. + View? lastSuperView = null; + foreach (View view in viewsArray) + { + if (view is not Adornment && view.SuperView is { } && view.SuperView != lastSuperView) { - view.SuperView.SubViewNeedsDraw = false; + // Check if ANY subview of this SuperView still needs drawing + bool anySubViewNeedsDrawing = view.SuperView.InternalSubViews.Any (v => v.NeedsDraw || v.SubViewNeedsDraw); + + if (!anySubViewNeedsDrawing) + { + view.SuperView.SubViewNeedsDraw = false; + } + + lastSuperView = view.SuperView; } } } @@ -216,7 +232,7 @@ public partial class View // Drawing APIs { Margin.NeedsLayout = false; Margin?.Thickness.Draw (Driver, FrameToScreen ()); - Margin?.Parent?.SetSubViewNeedsDraw (); + Margin?.Parent?.SetSubViewNeedsDrawDownHierarchy (); } if (SubViewNeedsDraw) @@ -466,7 +482,7 @@ public partial class View // Drawing APIs } // We assume that the text has been drawn over the entire area; ensure that the subviews are redrawn. - SetSubViewNeedsDraw (); + SetSubViewNeedsDrawDownHierarchy (); } /// @@ -478,6 +494,7 @@ public partial class View // Drawing APIs public event EventHandler? DrewText; #endregion DrawText + #region DrawContent private void DoDrawContent (DrawContext? context = null) @@ -599,7 +616,7 @@ public partial class View // Drawing APIs // TODO: HACK - This forcing of SetNeedsDraw with SuperViewRendersLineCanvas enables auto line join to work, but is brute force. if (view.SuperViewRendersLineCanvas || view.ViewportSettings.HasFlag (ViewportSettingsFlags.Transparent)) { - //view.SetNeedsDraw (); + //view.SetNeedsDraw (); } view.Draw (context); diff --git a/Terminal.Gui/ViewBase/View.NeedsDraw.cs b/Terminal.Gui/ViewBase/View.NeedsDraw.cs index 7af94f287..b4dee8763 100644 --- a/Terminal.Gui/ViewBase/View.NeedsDraw.cs +++ b/Terminal.Gui/ViewBase/View.NeedsDraw.cs @@ -2,18 +2,18 @@ public partial class View { - // TODO: Change NeedsDraw to use a Region instead of Rectangle - // TODO: Make _needsDrawRect nullable instead of relying on Empty - // TODO: If null, it means ? - // TODO: If Empty, it means no need to redraw - // TODO: If not Empty, it means the region that needs to be redrawn - + // NOTE: NeedsDrawRect is not currently used to clip drawing to only the invalidated region. + // It is only used within SetNeedsDraw to propagate redraw requests to subviews. + // NOTE: Consider changing NeedsDrawRect from Rectangle to Region for more precise invalidation + // NeedsDraw is already efficiently cached via NeedsDrawRect. It checks: + // 1. NeedsDrawRect (cached by SetNeedsDraw/ClearNeedsDraw) + // 2. Adornment NeedsDraw flags (each cached separately) /// - /// The viewport-relative region that needs to be redrawn. Marked internal for unit tests. + /// INTERNAL: Gets the viewport-relative region that needs to be redrawn. /// - internal Rectangle NeedsDrawRect { get; set; } = Rectangle.Empty; + internal Rectangle NeedsDrawRect { get; private set; } = Rectangle.Empty; - /// Gets or sets whether the view needs to be redrawn. + /// Gets whether the view needs to be redrawn. /// /// /// Will be if the property is or if @@ -23,34 +23,11 @@ public partial class View /// Setting has no effect on . /// /// - public bool NeedsDraw - { - get => Visible && (NeedsDrawRect != Rectangle.Empty || Margin?.NeedsDraw == true || Border?.NeedsDraw == true || Padding?.NeedsDraw == true); - set - { - if (value) - { - SetNeedsDraw (); - } - else - { - ClearNeedsDraw (); - } - } - } + public bool NeedsDraw => Visible && (NeedsDrawRect != Rectangle.Empty || Margin?.NeedsDraw == true || Border?.NeedsDraw == true || Padding?.NeedsDraw == true); - // TODO: This property is decoupled from the actual state of the subviews (and adornments) - // TODO: It is a 'cache' that is set when any subview or adornment requests a redraw - // TODO: As a result the code is fragile and can get out of sync. - // TODO: Consider making this a computed property that checks all subviews and adornments for their NeedsDraw state - // TODO: But that may have performance implications. - - /// Gets whether any SubViews need to be redrawn. - public bool SubViewNeedsDraw { get; private set; } - - /// Sets that the of this View needs to be redrawn. + /// Sets to indicating the of this View needs to be redrawn. /// - /// If the view has not been initialized ( is ), this method + /// If the view is not visible ( is ), this method /// does nothing. /// public void SetNeedsDraw () @@ -109,14 +86,13 @@ public partial class View Padding?.SetNeedsDraw (); } - SuperView?.SetSubViewNeedsDraw (); + SuperView?.SetSubViewNeedsDrawDownHierarchy (); if (this is Adornment adornment) { - adornment.Parent?.SetSubViewNeedsDraw (); + adornment.Parent?.SetSubViewNeedsDrawDownHierarchy (); } - // There was multiple enumeration error here, so calling new snapshot collection - probably a stop gap foreach (View subview in InternalSubViews.Snapshot ()) { if (subview.Frame.IntersectsWith (viewPortRelativeRegion)) @@ -129,8 +105,51 @@ public partial class View } } - /// Sets to for this View and all Superviews. - public void SetSubViewNeedsDraw () + /// INTERNAL: Clears and for this view and all SubViews. + /// + /// See is a cached value that is set when any subview or adornment requests a redraw. + /// It may not always be in sync with the actual state of the subviews. + /// + internal void ClearNeedsDraw () + { + NeedsDrawRect = Rectangle.Empty; + + Margin?.ClearNeedsDraw (); + Border?.ClearNeedsDraw (); + Padding?.ClearNeedsDraw (); + + foreach (View subview in InternalSubViews.Snapshot ()) + { + subview.ClearNeedsDraw (); + } + + SubViewNeedsDraw = false; + + // This ensures LineCanvas' get redrawn + if (!SuperViewRendersLineCanvas) + { + LineCanvas.Clear (); + } + } + + // NOTE: SubViewNeedsDraw is decoupled from the actual state of the subviews (and adornments). + // It is a performance optimization to avoid having to traverse all subviews and adornments to check if any need redraw. + // As a result the code is fragile and can get out of sync; care must be taken to ensure it is set and cleared correctly. + /// + /// INTERNAL: Gets whether any SubViews need to be redrawn. + /// + /// + /// See is a cached value that is set when any subview or adornment requests a redraw. + /// It may not always be in sync with the actual state of the subviews. + /// + internal bool SubViewNeedsDraw { get; private set; } + + /// INTERNAL: Sets to for this View and all Superviews. + /// + /// See is a cached value that is set when any subview or adornment requests a redraw. + /// It may not always be in sync with the actual state of the subviews. + /// + internal void SetSubViewNeedsDrawDownHierarchy () { if (!Visible) { @@ -141,51 +160,12 @@ public partial class View if (this is Adornment adornment) { - adornment.Parent?.SetSubViewNeedsDraw (); + adornment.Parent?.SetSubViewNeedsDrawDownHierarchy (); } if (SuperView is { SubViewNeedsDraw: false }) { - SuperView.SetSubViewNeedsDraw (); - } - } - - /// Clears and . - protected void ClearNeedsDraw () - { - NeedsDrawRect = Rectangle.Empty; - - Margin?.ClearNeedsDraw (); - Border?.ClearNeedsDraw (); - Padding?.ClearNeedsDraw (); - - // There was multiple enumeration error here, so calling new snapshot collection - probably a stop gap - foreach (View subview in InternalSubViews.Snapshot ()) - { - subview.ClearNeedsDraw (); - } - - SubViewNeedsDraw = false; - - // DO NOT clear SuperView.SubViewNeedsDraw here! - // The SuperView is responsible for clearing its own SubViewNeedsDraw flag. - // Previously this code cleared it: - //if (SuperView is { }) - //{ - // SuperView.SubViewNeedsDraw = false; - //} - // This caused a bug where drawing one subview would incorrectly clear the SuperView's - // SubViewNeedsDraw flag even when sibling subviews still needed drawing. - // - // The SuperView will clear its own SubViewNeedsDraw after all its subviews are drawn, - // either via: - // 1. The superview's own Draw() method calling ClearNeedsDraw() - // 2. The static View.Draw(peers) method calling ClearNeedsDraw() on all peers - - // This ensures LineCanvas' get redrawn - if (!SuperViewRendersLineCanvas) - { - LineCanvas.Clear (); + SuperView.SetSubViewNeedsDrawDownHierarchy (); } } } diff --git a/Tests/UnitTestsParallelizable/Drawing/Lines/StraightLineTests.cs b/Tests/UnitTestsParallelizable/Drawing/Lines/StraightLineTests.cs index 38d8e223f..327c3f232 100644 --- a/Tests/UnitTestsParallelizable/Drawing/Lines/StraightLineTests.cs +++ b/Tests/UnitTestsParallelizable/Drawing/Lines/StraightLineTests.cs @@ -1,6 +1,6 @@ using Xunit.Abstractions; -namespace UnitTests.Parallelizable.Drawing.Lines; +namespace DrawingTests.Lines; public class StraightLineTests (ITestOutputHelper output) { diff --git a/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs index eac7c4ee0..ccbe2a888 100644 --- a/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs +++ b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs @@ -69,7 +69,7 @@ public class NeedsDrawTests : FakeDriverBase view.BeginInit (); Assert.True (view.NeedsDraw); - view.NeedsDraw = false; + view.ClearNeedsDraw (); view.BeginInit (); Assert.False (view.NeedsDraw); // Because layout is still needed @@ -94,7 +94,7 @@ public class NeedsDrawTests : FakeDriverBase view = new () { Width = 2, Height = 2, BorderStyle = LineStyle.Single }; view.BeginInit (); - view.NeedsDraw = false; + view.ClearNeedsDraw (); view.EndInit (); Assert.True (view.NeedsDraw); } @@ -145,7 +145,7 @@ public class NeedsDrawTests : FakeDriverBase Assert.True (view.NeedsDraw); Assert.False (view.NeedsLayout); - view.NeedsDraw = false; + view.ClearNeedsDraw (); // SRL won't change anything since the view frame wasn't changed. However, Layout has not been called view.SetRelativeLayout (new (10, 10)); @@ -199,7 +199,7 @@ public class NeedsDrawTests : FakeDriverBase superView.Layout (); Assert.True (superView.NeedsDraw); - superView.NeedsDraw = false; + superView.ClearNeedsDraw (); superView.SetRelativeLayout (new (10, 10)); Assert.True (superView.NeedsDraw); } @@ -647,7 +647,7 @@ public class NeedsDrawTests : FakeDriverBase grandparent.EndInit (); grandparent.LayoutSubViews (); - child.SetSubViewNeedsDraw (); + child.SetSubViewNeedsDrawDownHierarchy (); Assert.True (child.SubViewNeedsDraw); Assert.True (parent.SubViewNeedsDraw);