Code cleanup and refactor

Terminal.Gui/Application/Application.Run.cs

@@ -503,83 +503,20 @@ public static partial class Application // Run (Begin, Run, End, Stop)
     /// <param name="forceDraw">If <see langword="true"/> the entire View hierarchy will be redrawn. The default is <see langword="false"/> and should only be overriden for testing.</param>
     public static void LayoutAndDrawToplevels (bool forceDraw = false)
     {
-        bool neededLayout = false;
-        neededLayout = LayoutToplevels ();
+        bool neededLayout = View.Layout (TopLevels.Reverse (), Screen.Size);
 
         if (forceDraw)
         {
             Driver?.ClearContents ();
         }
 
-        DrawToplevels (neededLayout || forceDraw);
+        Application.ClipToScreen ();
+        View.Draw (TopLevels, neededLayout || forceDraw);
+        Application.ClipToScreen ();
 
         Driver?.Refresh ();
     }
 
-    // TODO: Rename this to LayoutRunnables in https://github.com/gui-cs/Terminal.Gui/issues/2491
-
-    private static bool LayoutToplevels ()
-    {
-        bool neededLayout = false;
-
-        foreach (Toplevel tl in TopLevels.Reverse ())
-        {
-            if (tl.NeedsLayout)
-            {
-                neededLayout = true;
-                tl.Layout (Screen.Size);
-            }
-        }
-
-        return neededLayout;
-    }
-
-    // TODO: Rename this to DrawRunnables in https://github.com/gui-cs/Terminal.Gui/issues/2491
-    private static void DrawToplevels (bool forceDraw)
-    {
-        ClipToScreen ();
-
-        foreach (Toplevel tl in TopLevels)
-        {
-            if (forceDraw)
-            {
-                tl.SetNeedsDraw ();
-            }
-
-            tl.Draw ();
-        }
-
-        DrawMargins (TopLevels.Cast<View> ().ToList ());
-
-        ClipToScreen ();
-    }
-
-    // TODO: This is inefficent
-    private static bool DrawMargins (List<View> peers)
-    {
-        if (peers.Count == 0)
-        {
-            return false;
-        }
-        foreach (View view in peers)
-        {
-            if (view.Margin is { CachedClip: { }})
-            {
-                view.Margin.NeedsDraw = true;
-                Region? saved = Driver?.Clip;
-                Application.SetClip (view.Margin.CachedClip);
-                view.Margin.Draw ();
-                Application.SetClip (saved);
-            }
-            view.Margin.CachedClip = null;
-
-            DrawMargins (view.Subviews.ToList ());
-            view.NeedsDraw = false;
-        }
-
-        return true;
-    }
-
     /// <summary>This event is raised on each iteration of the main loop.</summary>
     /// <remarks>See also <see cref="Timeout"/></remarks>
     public static event EventHandler<IterationEventArgs>? Iteration;

Terminal.Gui/View/Adornment/Adornment.cs

@@ -168,9 +168,6 @@ public class Adornment : View, IDesignable
             return true;
         }
 
-        Attribute normalAttr = GetNormalColor ();
-        SetAttribute (normalAttr);
-
         // This just draws/clears the thickness, not the insides.
         Thickness.Draw (ViewportToScreen (Viewport), Diagnostics, ToString ());
 

Terminal.Gui/View/Adornment/Margin.cs

@@ -1,16 +1,14 @@
 #nullable enable
 
-using Microsoft.VisualBasic;
-using static Terminal.Gui.SpinnerStyle;
-using static Unix.Terminal.Curses;
-using System.Text;
-
 namespace Terminal.Gui;
 
 /// <summary>The Margin for a <see cref="View"/>. Accessed via <see cref="View.Margin"/></summary>
 /// <remarks>
 ///     <para>
-///         The margin is typically transparent. This can be overriden by explicitly setting <see cref="ColorScheme"/>.
+///         The Margin is transparent by default. This can be overriden by explicitly setting <see cref="ColorScheme"/>.
+///     </para>
+///     <para>
+///         Margins are drawn after all other Views in the application View hierarchy are drawn.
 ///     </para>
 ///     <para>See the <see cref="Adornment"/> class.</para>
 /// </remarks>
@@ -40,12 +38,45 @@ public class Margin : Adornment
         CanFocus = false;
     }
 
-    public Region? CachedClip { get; set; }
+    // When the Parent is drawn, we cache the clip region so we can draw the Margin after all other Views
+    // QUESTION: Why can't this just be the NeedsDisplay region?
+    internal Region? CachedClip { get; set; }
 
-    private bool _pressed;
+    // PERFORMANCE: Margins are ALWAYS drawn. This may be an issue for apps that have a large number of views with shadows.
+    /// <summary>
+    ///     INTERNAL API - Draws the margins for the specified views. This is called by the <see cref="Application"/> on each
+    ///     iteration of the main loop after all Views have been drawn.
+    /// </summary>
+    /// <param name="margins"></param>
+    /// <returns><see langword="true"/></returns>
+    internal static bool DrawMargins (IEnumerable<View> margins)
+    {
+        Stack<View> stack = new (margins);
 
-    private ShadowView? _bottomShadow;
-    private ShadowView? _rightShadow;
+        while (stack.Count > 0)
+        {
+            var view = stack.Pop ();
+
+            if (view.Margin is { CachedClip: { } })
+            {
+                view.Margin.NeedsDraw = true;
+                Region? saved = Driver?.Clip;
+                Application.SetClip (view.Margin.CachedClip);
+                view.Margin.Draw ();
+                Application.SetClip (saved);
+                view.Margin.CachedClip = null;
+            }
+
+            foreach (var subview in view.Subviews)
+            {
+                stack.Push (subview);
+            }
+
+            view.NeedsDraw = false;
+        }
+
+        return true;
+    }
 
     /// <inheritdoc/>
     public override void BeginInit ()
@@ -61,8 +92,7 @@ public class Margin : Adornment
     }
 
     /// <summary>
-    ///     The color scheme for the Margin. If set to <see langword="null"/>, gets the <see cref="Adornment.Parent"/>'s
-    ///     <see cref="View.SuperView"/> scheme. color scheme.
+    ///     The color scheme for the Margin. If set to <see langword="null"/> (the default), the margin will be transparent.
     /// </summary>
     public override ColorScheme? ColorScheme
     {
@@ -82,7 +112,7 @@ public class Margin : Adornment
         }
     }
 
-    /// <inheritdoc />
+    /// <inheritdoc/>
     protected override bool OnClearingViewport ()
     {
         if (Thickness == Thickness.Empty)
@@ -92,6 +122,12 @@ public class Margin : Adornment
 
         Rectangle screen = ViewportToScreen (Viewport);
 
+        // This just draws/clears the thickness, not the insides.
+        if (Diagnostics.HasFlag (ViewDiagnosticFlags.Thickness) || base.ColorScheme is { })
+        {
+            Thickness.Draw (screen, Diagnostics, ToString ());
+        }
+
         if (ShadowStyle != ShadowStyle.None)
         {
             // Don't clear where the shadow goes
@@ -101,6 +137,12 @@ public class Margin : Adornment
         return true;
     }
 
+    #region Shadow
+
+    private bool _pressed;
+    private ShadowView? _bottomShadow;
+    private ShadowView? _rightShadow;
+
     /// <summary>
     ///     Sets whether the Margin includes a shadow effect. The shadow is drawn on the right and bottom sides of the
     ///     Margin.
@@ -152,7 +194,7 @@ public class Margin : Adornment
                 Width = Dim.Fill (),
                 Height = SHADOW_HEIGHT,
                 ShadowStyle = style,
-                Orientation = Orientation.Horizontal,
+                Orientation = Orientation.Horizontal
             };
             Add (_rightShadow, _bottomShadow);
         }
@@ -167,7 +209,6 @@ public class Margin : Adornment
         set => base.ShadowStyle = SetShadow (value);
     }
 
-
     private void Margin_Highlight (object? sender, CancelEventArgs<HighlightStyle> e)
     {
         if (Thickness == Thickness.Empty || ShadowStyle == ShadowStyle.None)
@@ -180,7 +221,11 @@ public class Margin : Adornment
             // If the view is pressed and the highlight is being removed, move the shadow back.
             // Note, for visual effects reasons, we only move horizontally.
             // TODO: Add a setting or flag that lets the view move vertically as well.
-            Thickness = new (Thickness.Left - PRESS_MOVE_HORIZONTAL, Thickness.Top - PRESS_MOVE_VERTICAL, Thickness.Right + PRESS_MOVE_HORIZONTAL, Thickness.Bottom + PRESS_MOVE_VERTICAL);
+            Thickness = new (
+                             Thickness.Left - PRESS_MOVE_HORIZONTAL,
+                             Thickness.Top - PRESS_MOVE_VERTICAL,
+                             Thickness.Right + PRESS_MOVE_HORIZONTAL,
+                             Thickness.Bottom + PRESS_MOVE_VERTICAL);
 
             if (_rightShadow is { })
             {
@@ -202,7 +247,11 @@ public class Margin : Adornment
             // If the view is not pressed and we want highlight move the shadow
             // Note, for visual effects reasons, we only move horizontally.
             // TODO: Add a setting or flag that lets the view move vertically as well.
-            Thickness = new (Thickness.Left + PRESS_MOVE_HORIZONTAL, Thickness.Top + PRESS_MOVE_VERTICAL, Thickness.Right - PRESS_MOVE_HORIZONTAL, Thickness.Bottom - PRESS_MOVE_VERTICAL);
+            Thickness = new (
+                             Thickness.Left + PRESS_MOVE_HORIZONTAL,
+                             Thickness.Top + PRESS_MOVE_VERTICAL,
+                             Thickness.Right - PRESS_MOVE_HORIZONTAL,
+                             Thickness.Bottom - PRESS_MOVE_VERTICAL);
             _pressed = true;
 
             if (_rightShadow is { })
@@ -227,20 +276,25 @@ public class Margin : Adornment
                 case ShadowStyle.Transparent:
                     // BUGBUG: This doesn't work right for all Border.Top sizes - Need an API on Border that gives top-right location of line corner.
                     _rightShadow.Y = Parent!.Border.Thickness.Top > 0 ? ScreenToViewport (Parent.Border.GetBorderRectangle ().Location).Y + 1 : 0;
+
                     break;
 
                 case ShadowStyle.Opaque:
                     // BUGBUG: This doesn't work right for all Border.Top sizes - Need an API on Border that gives top-right location of line corner.
                     _rightShadow.Y = Parent!.Border.Thickness.Top > 0 ? ScreenToViewport (Parent.Border.GetBorderRectangle ().Location).Y + 1 : 0;
                     _bottomShadow.X = Parent.Border.Thickness.Left > 0 ? ScreenToViewport (Parent.Border.GetBorderRectangle ().Location).X + 1 : 0;
+
                     break;
 
                 case ShadowStyle.None:
                 default:
                     _rightShadow.Y = 0;
                     _bottomShadow.X = 0;
+
                     break;
             }
         }
     }
+
+    #endregion Shadow
 }

Terminal.Gui/View/View.Drawing.cs

@@ -7,6 +7,25 @@ namespace Terminal.Gui;
 
 public partial class View // Drawing APIs
 {
+
+    internal static void Draw (IEnumerable<View> views, bool force)
+    {
+        IEnumerable<View> viewsArray = views as View [] ?? views.ToArray ();
+
+        foreach (View view in viewsArray)
+        {
+            if (force)
+            {
+                view.SetNeedsDraw ();
+            }
+
+            view.Draw ();
+        }
+
+        Margin.DrawMargins (viewsArray);
+    }
+
+
     /// <summary>
     ///     Draws the view if it needs to be drawn.
     /// </summary>

Terminal.Gui/View/View.Layout.cs

@@ -1,6 +1,7 @@
 #nullable enable
 using System.Diagnostics;
 using Microsoft.CodeAnalysis;
+using static Unix.Terminal.Curses;
 
 namespace Terminal.Gui;
 
@@ -352,6 +353,28 @@ public partial class View // Layout APIs
 
     #region Core Layout API
 
+    /// <summary>
+    ///     INTERNAL API - Performs layout of the specified views within the specified content size. Called by the Application main loop.
+    /// </summary>
+    /// <param name="views">The views to layout.</param>
+    /// <param name="contentSize">The size to bound the views by.</param>
+    /// <returns><see langword="true"/>If any of the views needed to be laid out.</returns>
+    internal static bool Layout (IEnumerable<View> views, Size contentSize)
+    {
+        bool neededLayout = false;
+
+        foreach (View v in views)
+        {
+            if (v.NeedsLayout)
+            {
+                neededLayout = true;
+                v.Layout (contentSize);
+            }
+        }
+
+        return neededLayout;
+    }
+
     /// <summary>
     ///     Performs layout of the view and its subviews within the specified content size.
     /// </summary>

UnitTests/View/Adornment/MarginTests.cs

@@ -6,32 +6,29 @@ public class MarginTests (ITestOutputHelper output)
 {
     [Fact]
     [SetupFakeDriver]
-    public void Margin_Uses_SuperView_ColorScheme ()
+    public void Margin_Is_Transparent ()
     {
         ((FakeDriver)Application.Driver!).SetBufferSize (5, 5);
-        View.Diagnostics = ViewDiagnosticFlags.Thickness;
 
         var view = new View { Height = 3, Width = 3 };
+        view.Margin.Diagnostics = ViewDiagnosticFlags.Thickness;
         view.Margin.Thickness = new (1);
 
-        var superView = new View ();
+        Application.Top = new Toplevel ();
+        Application.TopLevels.Push (Gui.Application.Top);
 
-        superView.ColorScheme = new()
+        Application.Top.ColorScheme = new()
         {
             Normal = new (Color.Red, Color.Green), Focus = new (Color.Green, Color.Red)
         };
 
-        superView.Add (view);
+        Application.Top.Add (view);
         Assert.Equal (ColorName16.Red, view.Margin.GetNormalColor ().Foreground.GetClosestNamedColor16 ());
-        Assert.Equal (ColorName16.Red, superView.GetNormalColor ().Foreground.GetClosestNamedColor16 ());
-        Assert.Equal (superView.GetNormalColor (), view.Margin.GetNormalColor ());
-        Assert.Equal (superView.GetFocusColor (), view.Margin.GetFocusColor ());
+        Assert.Equal (ColorName16.Red, Application.Top.GetNormalColor ().Foreground.GetClosestNamedColor16 ());
 
-        superView.BeginInit ();
-        superView.EndInit ();
-        view.SetNeedsDraw();
-        view.Draw ();
-        View.Diagnostics = ViewDiagnosticFlags.Off;
+        Application.Top.BeginInit ();
+        Application.Top.EndInit ();
+        Application.LayoutAndDrawToplevels();
 
         TestHelpers.AssertDriverContentsAre (
                                              @"
@@ -40,6 +37,8 @@ M M
 MMM",
                                              output
                                             );
-        TestHelpers.AssertDriverAttributesAre ("0", output, null, superView.GetNormalColor ());
+        TestHelpers.AssertDriverAttributesAre ("0", output, null, Application.Top.GetNormalColor ());
+
+        Application.ResetState (true);
     }
 }

UnitTests/View/Draw/DrawTests.cs

@@ -42,8 +42,8 @@ public class DrawTests (ITestOutputHelper _output)
             Y = 1,
             Width = 3, Height = 3
         };
-        view.Margin.Thickness = new (1);
-        view.Margin.Diagnostics = ViewDiagnosticFlags.Thickness;
+        view.Padding.Thickness = new (1);
+        view.Padding.Diagnostics = ViewDiagnosticFlags.Thickness;
         view.BeginInit ();
         view.EndInit ();
         view.Draw ();
@@ -56,10 +56,10 @@ public class DrawTests (ITestOutputHelper _output)
         Assert.Equal ((Rune)' ', Application.Driver?.Contents! [2, 2].Rune);
 
         view.AddRune (-1, -1, Rune.ReplacementChar);
-        Assert.Equal ((Rune)'M', Application.Driver?.Contents! [1, 1].Rune);
+        Assert.Equal ((Rune)'P', Application.Driver?.Contents! [1, 1].Rune);
 
         view.AddRune (1, 1, Rune.ReplacementChar);
-        Assert.Equal ((Rune)'M', Application.Driver?.Contents! [3, 3].Rune);
+        Assert.Equal ((Rune)'P', Application.Driver?.Contents! [3, 3].Rune);
     }
 
     [Theory]

docfx/docs/drawing.md

@@ -43,9 +43,8 @@ If a View need to redraw because something changed within it's Content Area it c
 
 ### Clipping
 
-Clipping enables better performance by ensuring on regions of the terminal that need to be drawn actually get drawn by the @Terminal.Gui.ConsoleDriver. Terminal.Gui supports non-rectangular clip regions with @Terminal.Gui.Region. @Terminal.Gui.ConsoleDriver.Clip is the application managed clip region and is managed by @Terminal.Gui.Application. Developers cannot change this directly.
+Clipping enables better performance by ensuring on regions of the terminal that need to be drawn actually get drawn by the @Terminal.Gui.ConsoleDriver. Terminal.Gui supports non-rectangular clip regions with @Terminal.Gui.Region. @Terminal.Gui.ConsoleDriver.Clip is the application managed clip region and is managed by @Terminal.Gui.Application. Developers cannot change this directly, but can use @Terminal.Gui.Application.ClipToScreen, @Terminal.Gui.Application.SetClip(Region), @Terminal.Gui.View.ClipToFrame, and @Terminal.Gui.ClipToViewPort.
 
-The View drawing APIs, e.g. @Terminal.Gui.View.OnDrawingSubviews, are passed the clip region currently enforced in Viewport-relative coordinates. 
 
 ## Coordinate System for Drawing