diff --git a/Terminal.Gui/Drawing/Thickness.cs b/Terminal.Gui/Drawing/Thickness.cs index b2f4b0313b..aa025dc8bb 100644 --- a/Terminal.Gui/Drawing/Thickness.cs +++ b/Terminal.Gui/Drawing/Thickness.cs @@ -22,7 +22,7 @@ namespace Terminal.Gui.Drawing; public record struct Thickness { /// Initializes a new instance of the class with all widths set to 0. - public Thickness () { _sides = Vector4.Zero; } + public Thickness () => _sides = Vector4.Zero; /// Initializes a new instance of the class with a uniform width to each side. /// @@ -52,15 +52,11 @@ public Thickness (int left, int top, int right, int bottom) /// /// /// - public readonly Thickness Add (Thickness other) { return new (Left + other.Left, Top + other.Top, Right + other.Right, Bottom + other.Bottom); } + public readonly Thickness Add (Thickness other) => new (Left + other.Left, Top + other.Top, Right + other.Right, Bottom + other.Bottom); /// Gets or sets the width of the lower side of the rectangle. [JsonInclude] - public int Bottom - { - readonly get => (int)_sides.W; - set => _sides.W = value; - } + public int Bottom { readonly get => (int)_sides.W; set => _sides.W = value; } /// /// Gets whether the specified coordinates lie within the thickness (inside the bounding rectangle but outside @@ -120,39 +116,25 @@ public Rectangle Draw (IDriver? driver, Rectangle rect, ViewDiagnosticFlags diag // Draw the Top side if (Top > 0) { - driver?.FillRect (rect with { Height = Math.Min (rect.Height, Top) }, topChar); + driver.FillRect (rect with { Height = Math.Min (rect.Height, Top) }, topChar); } // Draw the Left side if (Left > 0) { - driver?.FillRect (rect with { Width = Math.Min (rect.Width, Left) }, leftChar); + driver.FillRect (rect with { Width = Math.Min (rect.Width, Left) }, leftChar); } // Draw the Right side if (Right > 0) { - driver?.FillRect ( - rect with - { - X = Math.Max (0, rect.X + rect.Width - Right), - Width = Math.Min (rect.Width, Right) - }, - rightChar - ); + driver.FillRect (rect with { X = Math.Max (0, rect.X + rect.Width - Right), Width = Math.Min (rect.Width, Right) }, rightChar); } // Draw the Bottom side if (Bottom > 0) { - driver?.FillRect ( - rect with - { - Y = rect.Y + Math.Max (0, rect.Height - Bottom), - Height = Bottom - }, - bottomChar - ); + driver.FillRect (rect with { Y = rect.Y + Math.Max (0, rect.Height - Bottom), Height = Bottom }, bottomChar); } if (diagnosticFlags.HasFlag (ViewDiagnosticFlags.Ruler)) @@ -163,7 +145,7 @@ rect with if (Top > 0) { - hRuler.Draw (driver: driver, location: rect.Location); + hRuler.Draw (driver, rect.Location); } //Left @@ -177,36 +159,35 @@ rect with // Bottom if (Bottom > 0) { - hRuler.Draw (driver: driver, location: rect.Location with { Y = rect.Y + rect.Height - 1 }); + hRuler.Draw (driver, rect.Location with { Y = rect.Y + rect.Height - 1 }); } // Right if (Right > 0) { - vRuler.Draw (driver, new (rect.X + rect.Width - 1, rect.Y + 1), 1); + vRuler.Draw (driver, new Point (rect.X + rect.Width - 1, rect.Y + 1), 1); } } - if (diagnosticFlags.HasFlag (ViewDiagnosticFlags.Thickness)) + if (!diagnosticFlags.HasFlag (ViewDiagnosticFlags.Thickness)) { - // Draw the diagnostics label on the bottom - string text = label is null ? string.Empty : $"{label} {this}"; - - TextFormatter tf = new () - { - Text = text, - Alignment = Alignment.Center, - VerticalAlignment = Alignment.End, - ConstrainToWidth = text.GetColumns (), - ConstrainToHeight = 1 - }; - - if (driver?.CurrentAttribute is { }) - { - tf.Draw (driver, rect, driver!.CurrentAttribute, driver!.CurrentAttribute, rect); - } + return GetInside (rect); } + // Draw the diagnostics label on the bottom + string text = label is null ? string.Empty : $"{label} {this}"; + + TextFormatter tf = new () + { + Text = text, + Alignment = Alignment.Center, + VerticalAlignment = Alignment.End, + ConstrainToWidth = text.GetColumns (), + ConstrainToHeight = 1 + }; + + tf.Draw (driver, rect, driver.CurrentAttribute, driver.CurrentAttribute, rect); + return GetInside (rect); } @@ -233,7 +214,7 @@ public Rectangle GetInside (Rectangle rect) int width = Math.Max (0, rect.Size.Width - Horizontal); int height = Math.Max (0, rect.Size.Height - Vertical); - return new (x, y, width, height); + return new Rectangle (x, y, width, height); } /// @@ -253,19 +234,11 @@ public Region AsRegion (Rectangle rect) /// Gets the total width of the left and right sides of the rectangle. Sets the width of the left and right sides /// of the rectangle to half the specified value. /// - public int Horizontal - { - get => Left + Right; - set => Left = Right = value / 2; - } + public int Horizontal { get => Left + Right; set => Left = Right = value / 2; } /// Gets or sets the width of the left side of the rectangle. [JsonInclude] - public int Left - { - readonly get => (int)_sides.X; - set => _sides.X = value; - } + public int Left { readonly get => (int)_sides.X; set => _sides.X = value; } /// /// Adds the thickness widths of another to another . @@ -273,35 +246,23 @@ public int Left /// /// /// - public static Thickness operator + (Thickness a, Thickness b) { return a.Add (b); } + public static Thickness operator + (Thickness a, Thickness b) => a.Add (b); /// Gets or sets the width of the right side of the rectangle. [JsonInclude] - public int Right - { - readonly get => (int)_sides.Z; - set => _sides.Z = value; - } + public int Right { readonly get => (int)_sides.Z; set => _sides.Z = value; } /// Gets or sets the width of the upper side of the rectangle. [JsonInclude] - public int Top - { - readonly get => (int)_sides.Y; - set => _sides.Y = value; - } + public int Top { readonly get => (int)_sides.Y; set => _sides.Y = value; } /// Returns the thickness widths of the Thickness formatted as a string. /// The thickness widths as a string. - public override string ToString () { return $"(Left={Left},Top={Top},Right={Right},Bottom={Bottom})"; } + public override string ToString () => $"(Left={Left},Top={Top},Right={Right},Bottom={Bottom})"; /// /// Gets the total height of the top and bottom sides of the rectangle. Sets the height of the top and bottom /// sides of the rectangle to half the specified value. /// - public int Vertical - { - get => Top + Bottom; - set => Top = Bottom = value / 2; - } + public int Vertical { get => Top + Bottom; set => Top = Bottom = value / 2; } } diff --git a/Terminal.Gui/ViewBase/View.Layout.cs b/Terminal.Gui/ViewBase/View.Layout.cs index fcabb72e03..5f9d53cf69 100644 --- a/Terminal.Gui/ViewBase/View.Layout.cs +++ b/Terminal.Gui/ViewBase/View.Layout.cs @@ -721,6 +721,10 @@ internal void LayoutSubViews () Size contentSize = GetContentSize (); OnSubViewLayout (new LayoutEventArgs (contentSize)); + + // Re-read content size — OnSubViewLayout handlers (e.g. Dialog.UpdateSizes) may have called SetContentSize. + contentSize = GetContentSize (); + SubViewLayout?.Invoke (this, new LayoutEventArgs (contentSize)); // The Adornments already have their Frame's set by SetRelativeLayout so we call LayoutSubViews vs. Layout here. diff --git a/Terminal.Gui/ViewBase/View.ScrollBars.cs b/Terminal.Gui/ViewBase/View.ScrollBars.cs index d8f1e1e54b..2cb0d380de 100644 --- a/Terminal.Gui/ViewBase/View.ScrollBars.cs +++ b/Terminal.Gui/ViewBase/View.ScrollBars.cs @@ -2,6 +2,12 @@ namespace Terminal.Gui.ViewBase; public partial class View { + // Track whether scrollbar padding has been applied to prevent accumulation + // during layout re-entry. The VisibleChanged handlers use +1/-1 relative + // adjustments, which compound when the event fires multiple times per cycle. + private bool _verticalScrollBarPaddingApplied; + private bool _horizontalScrollBarPaddingApplied; + private Lazy _horizontalScrollBar = null!; /// @@ -130,18 +136,30 @@ private void OnScrollBarInitialized (object? sender, EventArgs e) if (scrollBar.Orientation == Orientation.Vertical) { - Padding.Thickness = Padding.Thickness with { Right = scrollBar.Visible ? Padding.Thickness.Right + 1 : Padding.Thickness.Right }; + if (scrollBar.Visible && !_verticalScrollBarPaddingApplied) + { + Padding.Thickness = Padding.Thickness with { Right = Padding.Thickness.Right + 1 }; + _verticalScrollBarPaddingApplied = true; + } } else { - Padding.Thickness = Padding.Thickness with { Bottom = scrollBar.Visible ? Padding.Thickness.Bottom + 1 : Padding.Thickness.Bottom }; + if (scrollBar.Visible && !_horizontalScrollBarPaddingApplied) + { + Padding.Thickness = Padding.Thickness with { Bottom = Padding.Thickness.Bottom + 1 }; + _horizontalScrollBarPaddingApplied = true; + } } scrollBar.Layout (); } private void ConfigureVerticalScrollBarEvents (ScrollBar scrollBar) { - Padding.Thickness = Padding.Thickness with { Right = scrollBar.Visible ? Padding.Thickness.Right + 1 : Padding.Thickness.Right }; + if (scrollBar.Visible && !_verticalScrollBarPaddingApplied) + { + Padding.Thickness = Padding.Thickness with { Right = Padding.Thickness.Right + 1 }; + _verticalScrollBarPaddingApplied = true; + } scrollBar.ValueChanged += (_, args) => { @@ -156,16 +174,26 @@ private void ConfigureVerticalScrollBarEvents (ScrollBar scrollBar) Viewport = Viewport with { Y = 0 }; } - Padding.Thickness = Padding.Thickness with + if (scrollBar.Visible && !_verticalScrollBarPaddingApplied) { - Right = scrollBar.Visible ? Padding.Thickness.Right + 1 : Padding.Thickness.Right - 1 - }; + Padding.Thickness = Padding.Thickness with { Right = Padding.Thickness.Right + 1 }; + _verticalScrollBarPaddingApplied = true; + } + else if (!scrollBar.Visible && _verticalScrollBarPaddingApplied) + { + Padding.Thickness = Padding.Thickness with { Right = Padding.Thickness.Right - 1 }; + _verticalScrollBarPaddingApplied = false; + } }; } private void ConfigureHorizontalScrollBarEvents (ScrollBar scrollBar) { - Padding.Thickness = Padding.Thickness with { Bottom = scrollBar.Visible ? Padding.Thickness.Bottom + 1 : Padding.Thickness.Bottom }; + if (scrollBar.Visible && !_horizontalScrollBarPaddingApplied) + { + Padding.Thickness = Padding.Thickness with { Bottom = Padding.Thickness.Bottom + 1 }; + _horizontalScrollBarPaddingApplied = true; + } scrollBar.ValueChanged += (_, args) => { @@ -180,10 +208,16 @@ private void ConfigureHorizontalScrollBarEvents (ScrollBar scrollBar) Viewport = Viewport with { X = 0 }; } - Padding.Thickness = Padding.Thickness with + if (scrollBar.Visible && !_horizontalScrollBarPaddingApplied) { - Bottom = scrollBar.Visible ? Padding.Thickness.Bottom + 1 : Padding.Thickness.Bottom - 1 - }; + Padding.Thickness = Padding.Thickness with { Bottom = Padding.Thickness.Bottom + 1 }; + _horizontalScrollBarPaddingApplied = true; + } + else if (!scrollBar.Visible && _horizontalScrollBarPaddingApplied) + { + Padding.Thickness = Padding.Thickness with { Bottom = Padding.Thickness.Bottom - 1 }; + _horizontalScrollBarPaddingApplied = false; + } }; } diff --git a/Terminal.Gui/Views/DialogTResult.cs b/Terminal.Gui/Views/DialogTResult.cs index a8f3184618..d79f2b77c0 100644 --- a/Terminal.Gui/Views/DialogTResult.cs +++ b/Terminal.Gui/Views/DialogTResult.cs @@ -178,13 +178,11 @@ protected override bool OnAccepting (CommandEventArgs args) } + /// /// protected override void OnViewportChanged (DrawEventArgs e) { - //if (!IsInitialized) - { - SetContentSize (new Size (Math.Max (_minimumButtonsSize.Width, Viewport.Width), Math.Max (_minimumButtonsSize.Height, Viewport.Height))); - } + SetContentSize (new Size (Math.Max (_minimumButtonsSize.Width, Viewport.Width), Math.Max (_minimumButtonsSize.Height, Viewport.Height))); base.OnViewportChanged (e); } @@ -192,23 +190,25 @@ private void UpdateSizes () { if (SubViews.Count == 0) { - // This is primarily to support MessageBox where there are no subviews but - // Text is used. return; } + // For DimAuto dialogs, content must be at least as large as the subviews require + // so the dialog grows to fit. For fixed-size dialogs, content should match the + // Viewport directly; using _minimumSubViewsSize as a floor causes height drift + // on maximize/restore because the minimum captures the high-water mark and never + // shrinks back down. int subViewsWidth = _minimumSubViewsSize.Width; + int subViewsHeight = _minimumSubViewsSize.Height; if (!Width.Has (out _)) { - subViewsWidth = Math.Max (subViewsWidth, Viewport.Width); + subViewsWidth = Viewport.Width; } - int subViewsHeight = _minimumSubViewsSize.Height; - if (!Height.Has (out _)) { - subViewsHeight = Math.Max (subViewsHeight, Viewport.Height); + subViewsHeight = Viewport.Height; } SetContentSize (new Size (Math.Max (_minimumButtonsSize.Width, subViewsWidth), Math.Max (_minimumButtonsSize.Height, subViewsHeight))); diff --git a/Tests/UnitTestsParallelizable/ViewBase/Layout/ScrollBarPaddingAccumulationTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Layout/ScrollBarPaddingAccumulationTests.cs new file mode 100644 index 0000000000..572e315b92 --- /dev/null +++ b/Tests/UnitTestsParallelizable/ViewBase/Layout/ScrollBarPaddingAccumulationTests.cs @@ -0,0 +1,113 @@ +namespace ViewBaseTests.Layout; + +/// +/// Tests for the scrollbar padding accumulation bug discovered during investigation of +/// . +/// +/// Root cause: ScrollBar.VisibleChanged handlers in View.ScrollBars.cs use +/// relative Padding.Thickness +1/-1 adjustments. During layout re-entry (e.g. maximize/ +/// restore cycles), ShowHide fires multiple times per visibility change, compounding +/// the adjustment. Each cycle adds +2 to Padding.Bottom (horizontal scrollbar) and +/// +1 to Padding.Right (vertical scrollbar), causing the Viewport to shrink +/// permanently. +/// +/// Fix: track whether scrollbar padding has been applied via boolean flags +/// (_verticalScrollBarPaddingApplied / _horizontalScrollBarPaddingApplied) +/// and make the VisibleChanged handlers idempotent. +/// +public class ScrollBarPaddingAccumulationTests +{ + /// + /// Proves that repeated maximize/restore (resize) cycles on a Dialog do not cause + /// Padding.Thickness.Bottom to ratchet upward. Before the fix, each cycle added +2 + /// to Bottom, shrinking the Viewport by 2 rows per cycle. + /// + [Fact] + public void Dialog_Resize_Cycles_Do_Not_Accumulate_Padding () + { + Dialog dialog = new () + { + Width = 60, + Height = 20 + }; + + View child = new () + { + Width = Dim.Fill (), + Height = Dim.Fill () + }; + + dialog.Add (child); + dialog.AddButton (new Button { Title = "_OK" }); + dialog.BeginInit (); + dialog.EndInit (); + + // Initial layout at "normal" size + dialog.Layout (); + Thickness initialPadding = dialog.Padding.Thickness; + int initialViewportHeight = dialog.Viewport.Height; + + // Simulate 5 maximize/restore cycles + for (int i = 0; i < 5; i++) + { + // Maximize + dialog.Width = 200; + dialog.Height = 80; + dialog.SetNeedsLayout (); + dialog.Layout (); + + // Restore + dialog.Width = 60; + dialog.Height = 20; + dialog.SetNeedsLayout (); + dialog.Layout (); + } + + Thickness finalPadding = dialog.Padding.Thickness; + int finalViewportHeight = dialog.Viewport.Height; + + // Assert: Padding should not have grown + Assert.Equal (initialPadding.Bottom, finalPadding.Bottom); + Assert.Equal (initialPadding.Right, finalPadding.Right); + + // Assert: Viewport should be the same size as initially + Assert.Equal (initialViewportHeight, finalViewportHeight); + } + + /// + /// Proves that scrollbar visibility toggling (show/hide) applies padding exactly once + /// and removes it exactly once, regardless of how many layout passes occur. + /// + [Fact] + public void ScrollBar_Visibility_Toggle_Is_Idempotent () + { + View view = new () + { + Width = 20, + Height = 10, + ViewportSettings = ViewportSettingsFlags.HasScrollBars + }; + + // Force content larger than viewport to trigger scrollbars + view.SetContentSize (new Size (40, 30)); + + view.BeginInit (); + view.EndInit (); + view.Layout (); + + Thickness afterFirstLayout = view.Padding.Thickness; + + // Re-layout multiple times (simulating resize churn) + for (int i = 0; i < 10; i++) + { + view.SetNeedsLayout (); + view.Layout (); + } + + Thickness afterManyLayouts = view.Padding.Thickness; + + // Padding should be identical after repeated layouts + Assert.Equal (afterFirstLayout.Bottom, afterManyLayouts.Bottom); + Assert.Equal (afterFirstLayout.Right, afterManyLayouts.Right); + } +} diff --git a/Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs new file mode 100644 index 0000000000..69a0c92169 --- /dev/null +++ b/Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs @@ -0,0 +1,407 @@ +// Copilot + +namespace ViewBaseTests.Layout; + +/// +/// Tests for the stale content size capture bug in . +/// See: https://github.com/gui-cs/Terminal.Gui/issues/4522 +/// +/// The core bug: LayoutSubViews captures contentSize once at the top, then fires +/// OnSubViewLayout. If a subclass (e.g. Dialog) calls SetContentSize from that callback, +/// the captured value diverges from the actual content size. +/// +/// Additionally, line 771 sets NeedsLayout directly instead of calling SetNeedsLayout(), +/// bypassing upward propagation. +/// +/// Some tests demonstrate bugs by failing; others serve as regression tests ensuring correct +/// behavior is preserved. +/// +public class StaleContentSizeCaptureTests +{ + #region Helper SubView classes + + /// + /// A View subclass that calls from . + /// + private class ContentSizeChangingOnSubViewLayoutView : View + { + public Size NewContentSize { get; set; } + + /// Gets the content size that was passed to OnSubViewLayout. + public Size? CapturedLayoutContentSize { get; private set; } + + protected override void OnSubViewLayout (LayoutEventArgs args) + { + CapturedLayoutContentSize = args.OldContentSize; + SetContentSize (NewContentSize); + base.OnSubViewLayout (args); + } + } + + /// + /// A View subclass that calls from the SubViewsLaidOut + /// event (simulating HexView behavior). + /// + private class ContentSizeChangingOnSubViewsLaidOutView : View + { + public Size NewContentSize { get; set; } + + public ContentSizeChangingOnSubViewsLaidOutView () + { + SubViewsLaidOut += (_, _) => { SetContentSize (NewContentSize); }; + } + } + + #endregion + + #region Test 1: Core stale capture + + /// + /// Proves the core bug: LayoutSubViews captures contentSize before + /// OnSubViewLayout fires, so SubViews are laid out with the wrong size. + /// + [Fact] + public void LayoutSubViews_Uses_Stale_ContentSize_When_OnSubViewLayout_Changes_It () + { + // Arrange: SuperView that changes content size in OnSubViewLayout + ContentSizeChangingOnSubViewLayoutView superView = new () + { + Width = 20, + Height = 10, + NewContentSize = new Size (50, 20) + }; + + View child = new () + { + Width = Dim.Fill (), + Height = Dim.Fill () + }; + + superView.Add (child); + superView.BeginInit (); + superView.EndInit (); + superView.Layout (); + + // Assert: The child should fill the NEW content size (50, 20), + // not the stale pre-callback value. + Assert.Equal (50, child.Frame.Width); + Assert.Equal (20, child.Frame.Height); + } + + #endregion + + #region Test 2: Dialog resize scenario + + /// + /// Proves the reported real-world scenario: a Dialog with Dim.Fill SubViews + /// gets the wrong layout after a screen size change (simulating maximize/restore). + /// + [Fact] + public void Dialog_Children_Use_Stale_ContentSize_After_Screen_Resize () + { + // Arrange: Dialog with explicit dimensions and a Dim.Fill child + Dialog dialog = new () + { + Width = 40, + Height = 15 + }; + + View child = new () + { + Width = Dim.Fill (), + Height = Dim.Fill () + }; + + dialog.Add (child); + dialog.BeginInit (); + dialog.EndInit (); + + // First layout at initial size + dialog.Layout (); + Size firstContentSize = dialog.GetContentSize (); + Rectangle firstChildFrame = child.Frame; + + // Resize the dialog (simulating maximize) + dialog.Width = 120; + dialog.Height = 40; + dialog.SetNeedsLayout (); + dialog.Layout (); + + Size secondContentSize = dialog.GetContentSize (); + + // Assert: content size should have grown + Assert.True (secondContentSize.Width > firstContentSize.Width, + $"Content width should have grown: was {firstContentSize.Width}, now {secondContentSize.Width}"); + + Assert.True (secondContentSize.Height > firstContentSize.Height, + $"Content height should have grown: was {firstContentSize.Height}, now {secondContentSize.Height}"); + + // Assert: child frame should match the new content size, not the old one + Assert.Equal (secondContentSize.Width, child.Frame.Width); + Assert.Equal (secondContentSize.Height, child.Frame.Height); + } + + #endregion + + #region Test 3: Dialog OnSubViewLayout divergence + + /// + /// Proves that Dialog.OnSubViewLayout → UpdateSizes → SetContentSize + /// produces a content size that differs from what LayoutSubViews used to lay out SubViews. + /// + [Fact] + public void Dialog_OnSubViewLayout_SetContentSize_Diverges_From_Captured_Value () + { + // Arrange: Dialog with explicit (non-Auto) Width/Height + Dialog dialog = new () + { + Width = 60, + Height = 20 + }; + + // Add several subviews to create a _minimumSubViewsSize that differs from Viewport + View tall = new () + { + Width = 10, + Height = 30 // Taller than dialog viewport + }; + + View wide = new () + { + X = 0, + Y = Pos.Bottom (tall), + Width = 80, // Wider than dialog viewport + Height = 5 + }; + + dialog.Add (tall, wide); + dialog.BeginInit (); + dialog.EndInit (); + + // Capture the content size that exists before layout + Size? preLayoutContentSize = null; + Size? postLayoutContentSize = null; + + dialog.SubViewLayout += (_, args) => { preLayoutContentSize = args.OldContentSize; }; + dialog.SubViewsLaidOut += (_, _) => { postLayoutContentSize = dialog.GetContentSize (); }; + + dialog.Layout (); + + // Assert: If the bug exists, preLayoutContentSize (what children were laid out with) + // differs from postLayoutContentSize (the final content size after SetContentSize calls) + Assert.NotNull (preLayoutContentSize); + Assert.NotNull (postLayoutContentSize); + + // After layout, the child with Dim.Fill should be sized to the FINAL content size. + // This tests that the content size used for layout is consistent with the final value. + Assert.Equal (postLayoutContentSize, dialog.GetContentSize ()); + + // The key assertion: the content size should NOT have diverged. + // If it diverged, children were laid out with the wrong (stale) value. + Assert.Equal (postLayoutContentSize, preLayoutContentSize); + } + + #endregion + + #region Test 4: NeedsLayout propagation bypass + + /// + /// Proves that line 771 sets NeedsLayout = false directly, bypassing + /// SetNeedsLayout propagation. If content size changed during layout, + /// neither the view nor its SuperView know they need another pass. + /// + [Fact] + public void NeedsLayout_Direct_Set_Does_Not_Propagate_To_SuperView () + { + // Arrange: grandSuperView → customView → child + View grandSuperView = new () + { + Width = 80, + Height = 25 + }; + + ContentSizeChangingOnSubViewLayoutView customView = new () + { + Width = Dim.Fill (), + Height = Dim.Fill (), + NewContentSize = new Size (100, 50) // Will change content size during OnSubViewLayout + }; + + View child = new () + { + Width = Dim.Fill (), + Height = Dim.Fill () + }; + + customView.Add (child); + grandSuperView.Add (customView); + grandSuperView.BeginInit (); + grandSuperView.EndInit (); + + grandSuperView.Layout (); + + // After layout, SetContentSize was called from OnSubViewLayout which calls SetNeedsLayout(). + // But LayoutSubViews line 771 then sets NeedsLayout = false directly. + // The content size changed, so ideally NeedsLayout should still be true + // (or should have triggered a corrective layout pass). + + // The content size was changed to (100, 50). If a corrective pass happened, + // the child would be sized to (100, 50). If not, the child has the stale size. + Assert.Equal (100, child.Frame.Width); + Assert.Equal (50, child.Frame.Height); + } + + #endregion + + #region Test 5: SubViewsLaidOut SetContentSize is too late + + /// + /// Proves that views calling SetContentSize from SubViewsLaidOut + /// (like HexView) change content size after SubViews have already been laid out. + /// + [Fact] + public void LayoutSubViews_OnSubViewsLaidOut_SetContentSize_Is_Too_Late () + { + ContentSizeChangingOnSubViewsLaidOutView superView = new () + { + Width = 20, + Height = 10, + NewContentSize = new Size (80, 30) + }; + + View child = new () + { + Width = Dim.Fill (), + Height = Dim.Fill () + }; + + superView.Add (child); + superView.BeginInit (); + superView.EndInit (); + superView.Layout (); + + // Assert: The child should reflect the content size (80, 30) set in SubViewsLaidOut. + // On current code, the child was already laid out before SubViewsLaidOut fired. + Assert.Equal (80, child.Frame.Width); + Assert.Equal (30, child.Frame.Height); + } + + #endregion + + #region Test 6: ListView stale capture + + /// + /// Proves that ListView.OnViewportChanged calls SetContentSize during layout. + /// The parent's LayoutSubViews captures contentSize before the ListView's viewport + /// change triggers SetContentSize. A sibling using Dim.Fill should reflect the + /// updated content size, not the stale pre-callback value. + /// + [Fact] + public void ListView_OnViewportChanged_SetContentSize_Creates_Stale_Capture () + { + // Arrange: ListView with items, plus a sibling, in a container + ListView listView = new () + { + Width = Dim.Fill (), + Height = 5 + }; + + listView.Source = new ListWrapper (["Item 1", "Item 2", "Item 3", "Item 4", "Item 5", + "Item 6", "Item 7", "Item 8", "Item 9", "Item 10"]); + + View container = new () + { + Width = 30, + Height = 10 + }; + + container.Add (listView); + container.BeginInit (); + container.EndInit (); + + // Initial layout + container.Layout (); + Size firstContentSize = listView.GetContentSize (); + + // Resize container (simulating terminal resize) + container.Width = 60; + container.Height = 20; + container.SetNeedsLayout (); + container.Layout (); + + Size secondContentSize = listView.GetContentSize (); + + // Assert: ListView frame should match the new container + Assert.Equal (60, listView.Frame.Width); + Assert.Equal (5, listView.Frame.Height); + + // ListView content height = Source.Count (10 items), width = EffectiveMaxItemLength + Assert.Equal (10, secondContentSize.Height); + + // After resize, content size should have been recalculated + // (EffectiveMaxItemLength doesn't change, but height = Source.Count stays consistent) + Assert.Equal (firstContentSize.Height, secondContentSize.Height); + } + + #endregion + + #region Test 7: TableView stale capture + + /// + /// Proves that TableView.OnViewportChanged → RefreshContentSize calls + /// SetContentSize during layout. + /// + [Fact] + public void TableView_RefreshContentSize_During_Layout_Creates_Stale_Capture () + { + // Arrange: TableView with data in a container + TableView tableView = new () + { + Width = Dim.Fill (), + Height = Dim.Fill () + }; + + System.Data.DataTable dt = new (); + dt.Columns.Add ("Name"); + dt.Columns.Add ("Value"); + + for (var i = 0; i < 20; i++) + { + dt.Rows.Add ($"Row {i}", $"Val {i}"); + } + + tableView.Table = new DataTableSource (dt); + + View container = new () + { + Width = 30, + Height = 10 + }; + + container.Add (tableView); + container.BeginInit (); + container.EndInit (); + + // Initial layout + container.Layout (); + Size firstContentSize = tableView.GetContentSize (); + + // Resize container + container.Width = 80; + container.Height = 25; + container.SetNeedsLayout (); + container.Layout (); + + Size secondContentSize = tableView.GetContentSize (); + + // Assert: TableView frame should match the new container size + Assert.Equal (80, tableView.Frame.Width); + Assert.Equal (25, tableView.Frame.Height); + + // The content size should have been recalculated for the new viewport + Assert.True (secondContentSize.Width >= tableView.Viewport.Width, + $"Content width {secondContentSize.Width} should be >= viewport width {tableView.Viewport.Width}"); + } + + #endregion +} diff --git a/plans/border-padding-transparency-spec.md b/plans/border-padding-transparency-spec.md deleted file mode 100644 index 99bf9fc972..0000000000 --- a/plans/border-padding-transparency-spec.md +++ /dev/null @@ -1,475 +0,0 @@ -# Specification: Adornment Transparency Support (Border & Padding) - -## Issue - -**#4834**: `Border` (and `Padding`) should support `ViewportSettingsFlags.Transparent` and `ViewportSettingsFlags.TransparentMouse`. - -**Prerequisite for**: TabView redesign (#4183), where each Tab's Border renders a small header rectangle and the rest must be transparent. - -## Problem Statement - -Border draws **non-rectangular** content — an outline of line segments plus optional title text. The interior is empty. When transparency is enabled: - -- **Transparent (visual)**: Only border lines and title text should be drawn; the empty interior should show underlying views. -- **TransparentMouse**: Clicks on border lines/title go to the Border; clicks on the empty interior pass through to views beneath. - -Neither works on `v2_develop` today. - ---- - -## Root Causes (on v2_develop) - -### 1. Visual transparency fails for Border - -`DoDrawComplete` (in `View.Drawing.cs`) has an `if (this is not Adornment)` guard that skips **all** clip exclusion logic for adornments. Setting `Transparent` on a Border has no effect — the parent view's opaque path excludes the entire border frame from the clip regardless. - -### 2. Border writes to Parent.LineCanvas, not its own - -`Border.OnDrawingContent` writes all border lines to `Parent.LineCanvas`, not to Border's own LineCanvas. The parent renders these later in its own `DoRenderLineCanvas`, and the drawn region gets reported to the **parent's** DrawContext — not the Border's. So the Border's own DrawContext never knows what cells the border lines actually occupy. - -Additionally, `Adornment.SuperViewRendersLineCanvas` throws `InvalidOperationException` — adornments are blocked from using their own LineCanvas. - -### 3. TransparentMouse is blanket per-view - -`GetViewsUnderLocation` (in `View.Layout.cs`) does `RemoveAll(v => v!.ViewportSettings.HasFlag(excludeViewportSettingsFlags))` — blanket removal. There is no per-cell hit-testing. - -### 4. Parent clears viewport even when Border is transparent - -`DoClearViewport` clears the viewport unconditionally for opaque views. When the Border is transparent, the parent must NOT clear the border's interior area, otherwise the underlying content gets overwritten with blanks. - ---- - -## Architecture of the Fix - -The fix has two phases: - -1. **Phase 1 — Visual Transparency**: Make `Border.Transparent` actually work (show through to underlying content). -2. **Phase 2 — Per-Cell TransparentMouse**: Make `TransparentMouse` respect which cells were actually drawn vs. empty. - -Both phases apply to Padding as well, though Border is the primary use case. - ---- - -## Phase 1: Visual Transparency for Border - -### 1a. Fix Border LineCanvas Ownership - -**Files**: `Adornment.cs`, `Border.cs` - -**Problem**: Border writes to `Parent.LineCanvas`. The drawn region is tracked on the parent, not the Border. - -**Changes**: - -1. **`Adornment.cs`** — Change the `SuperViewRendersLineCanvas` property override. Remove the `InvalidOperationException` throw. Replace with a standard auto-property: - - ```csharp - /// Gets or sets whether this Adornment's LineCanvas will be merged into - /// its Parent view's LineCanvas for rendering. - public override bool SuperViewRendersLineCanvas { get; set; } - ``` - - Default is `false` (backward-compatible). When `true`, the adornment adds lines to its own LineCanvas and the parent merges and renders them. - -2. **`Border.cs` (`OnDrawingContent`)** — At the top of the method, set: - ```csharp - SuperViewRendersLineCanvas = true; - ``` - Then change all references from `Parent?.LineCanvas` / `Parent.LineCanvas` to `this.LineCanvas` / `LineCanvas`. Border now writes to its own LineCanvas. - -3. **`Border.cs` (`OnDrawingContent`)** — For the title text exclusion, change from `Parent?.LineCanvas.Exclude(...)` to `LineCanvas.Exclude(...)`. Also add `context?.AddDrawnRectangle(titleRect)` to report the title rectangle to the DrawContext for clip exclusion. - -**Effect**: Border adds lines to its own LineCanvas → parent's draw loop merges them (via existing `SuperViewRendersLineCanvas` merge in `DoDrawSubViews`) → parent's `DoRenderLineCanvas` renders all lines. The Border's own DrawContext tracks what IT drew (title text). - -### 1b. Merge Border LineCanvas in Parent Draw Cycle - -**File**: `View.Drawing.cs` (in `Draw` method, after `DoClearViewport`/`DoDrawSubViews` but before `DoRenderLineCanvas`) - -**Change**: Add a merge step for Border's LineCanvas: - -```csharp -if (Border is { SuperViewRendersLineCanvas: true }) -{ - LineCanvas.Merge (Border.LineCanvas); - Border.LineCanvas.Clear (); -} -``` - -This must happen **after** `ClearNeedsDraw` (which clears our LineCanvas) but **before** `DoRenderLineCanvas` — so border lines participate in line-join resolution with any lines SubViews added. - -**Note**: The existing merge in `DoDrawSubViews` handles SubViews with `SuperViewRendersLineCanvas`, but Border is an adornment (drawn via `DrawAdornments`), not a SubView. The merge for Border needs its own explicit step. - -### 1c. Make Border Participate in Clip Exclusion When Transparent - -**File**: `View.Drawing.cs` (`DoDrawComplete` method) - -**Change**: Relax the adornment guard. The current code: -```csharp -if (this is not Adornment) { /* clip exclusion */ } -``` - -Must become: -```csharp -if (this is Adornment && !ViewportSettings.HasFlag(ViewportSettingsFlags.Transparent)) -{ - return; // Opaque adornments skip — their parent handles them. -} -``` - -This lets transparent adornments participate in the drawn-region clip exclusion path. - -### 1d. Prevent Viewport Clearing for Views with Transparent Border - -**File**: `View.Drawing.cs` (`DoClearViewport` method) - -**Change**: Skip clearing if the view's Border has `Transparent` set: - -```csharp -if (ViewportSettings.HasFlag(ViewportSettingsFlags.Transparent) - || (Border is { } && Border.ViewportSettings.HasFlag(ViewportSettingsFlags.Transparent))) -{ - return; // Don't clear — transparent content should show through. -} - -ClearViewport(context); -OnClearedViewport(); -ClearedViewport?.Invoke(this, new DrawEventArgs(Viewport, Viewport, null)); -``` - -### 1e. "Effectively Transparent" Concept in Clip Exclusion - -**File**: `View.Drawing.cs` (`DoDrawComplete`) - -A view is treated as "effectively transparent" if: -- It has `ViewportSettingsFlags.Transparent` set directly, **OR** -- Its `Border` has `Transparent` set (the border interior should show through). - -In both cases, the **transparent path** is used for clip exclusion: -- Only actually-drawn cells are excluded from `Driver.Clip`. -- Padding is always excluded (it draws fills that fully occupy its thickness area). -- Border is NOT excluded from clip if it's transparent — its drawn cells are already in the context's drawn region via LineCanvas rendering. -- The drawn region is clamped to the Border's frame (for transparent Border) or the Viewport (for directly-transparent views). - -**Opaque path** (default, unchanged): -- Exclude the entire view frame (Border frame inward). Margin is never included here. -- Record the rectangle in DrawContext so the SuperView knows what area this subview occupied. - -### 1f. Pass DrawContext Through to Adornments - -**File**: `View.Drawing.cs` - -**Changes**: -1. `DoDrawAdornments` signature: add `DrawContext? drawContext` parameter. -2. `DrawAdornments` signature: add `DrawContext? drawContext` parameter. -3. Pass `drawContext` through to `Margin?.Draw(drawContext)`, `Border?.Draw(drawContext)`, `Padding?.Draw(drawContext)`. - -This enables adornments to receive and use the DrawContext for region tracking. - ---- - -## Phase 2: Per-Cell TransparentMouse Hit-Testing - -### 2a. Add `CachedDrawnRegion` Property to View - -**File**: `View.Drawing.cs` - -```csharp -/// Gets the cached drawn region from the last draw pass. Populated during -/// DoDrawComplete for views with TransparentMouse set. Used by mouse -/// hit-testing to determine which cells should receive mouse events. -/// Returns null if not drawn yet or TransparentMouse not set. -/// Invalidated by SetNeedsDraw(). -internal Region? CachedDrawnRegion { get; set; } -``` - -### 2b. Add `_lastLineCanvasRegion` to Track LineCanvas Cells - -**File**: `View.Drawing.cs` (in `DoRenderLineCanvas`) - -```csharp -private Region? _lastLineCanvasRegion; -``` - -After rendering the line canvas and reporting to context, cache the region: -```csharp -_lastLineCanvasRegion = cellMap.Count > 0 ? lineRegion : null; -``` - -This captures the exact cells where lines were rendered, including merged Border lines. - -### 2c. Cache Drawn Region in `DoDrawComplete` - -**File**: `View.Drawing.cs` (`DoDrawComplete`) - -**Before** clip exclusion (so we get fine-grained drawn regions, not the full frame rectangle), add caching logic for adornments and the view itself: - -**For adornments** (only when `this is not Adornment` — i.e., from the parent view's perspective): - -```csharp -// Border: Build from LineCanvas region intersected with Border frame + title rect -if (Border is { } && Border.ViewportSettings.HasFlag(TransparentMouse)) -{ - Region borderDrawnRegion = new(); - - if (_lastLineCanvasRegion is { }) - { - Region lineRegion = _lastLineCanvasRegion.Clone(); - lineRegion.Intersect(Border.FrameToScreen()); - borderDrawnRegion.Union(lineRegion); - } - - if (Border.LastTitleRect is { } titleRect) - { - borderDrawnRegion.Union(titleRect); - } - - Border.CachedDrawnRegion = borderDrawnRegion; -} - -// Margin: Build from context drawn region intersected with Margin frame -if (Margin is { } && Margin.ViewportSettings.HasFlag(TransparentMouse)) -{ - Region marginDrawnRegion = context.GetDrawnRegion().Clone(); - marginDrawnRegion.Intersect(Margin.FrameToScreen()); - Margin.CachedDrawnRegion = marginDrawnRegion; -} - -// Padding: Same approach as Margin -if (Padding is { } && Padding.ViewportSettings.HasFlag(TransparentMouse)) -{ - Region paddingDrawnRegion = context.GetDrawnRegion().Clone(); - paddingDrawnRegion.Intersect(Padding.FrameToScreen()); - Padding.CachedDrawnRegion = paddingDrawnRegion; -} -``` - -**For the view itself** (after clip exclusion): -```csharp -if (ViewportSettings.HasFlag(TransparentMouse)) -{ - if (isEffectivelyTransparent) - { - CachedDrawnRegion = context!.GetDrawnRegion().Clone(); - } - else - { - // Opaque view with TransparentMouse — cache the entire border frame. - Rectangle frame = Border is { } ? Border.FrameToScreen() : FrameToScreen(); - CachedDrawnRegion = new Region(frame); - } -} -``` - -### 2d. Add `LastTitleRect` to Border - -**File**: `Border.cs` - -```csharp -/// Gets the screen-coordinate rectangle of the title text from the last draw pass. -/// Used by the parent view to build CachedDrawnRegion for TransparentMouse hit-testing. -internal Rectangle? LastTitleRect { get; set; } -``` - -Set it in `OnDrawingContent` after drawing the title: -```csharp -LastTitleRect = titleRect; -``` - -This is needed because the title is drawn directly (not via LineCanvas), so it won't appear in `_lastLineCanvasRegion`. - -### 2e. Invalidate Cache in `SetNeedsDraw` - -**File**: `View.NeedsDraw.cs` (at the start of `SetNeedsDraw(Rectangle)`) - -```csharp -// Invalidate the cached drawn region for TransparentMouse hit-testing. -CachedDrawnRegion = null; -``` - -### 2f. Report Drawn Region in ShadowView - -**File**: `ShadowView.cs` (at the end of `OnDrawingContent`) - -```csharp -context?.AddDrawnRectangle(ViewportToScreen(Viewport)); -``` - -This ensures shadow cells are included in the Margin's CachedDrawnRegion. - -### 2g. Build Margin CachedDrawnRegion in `DrawShadows` - -**File**: `Margin.cs` (in `DrawShadows`, after drawing and clearing clip) - -Since Margin with shadows is drawn in a separate pass (not through the normal `Draw` flow), we build its `CachedDrawnRegion` here by iterating visible SubViews (the ShadowViews): - -```csharp -if (margin.ViewportSettings.HasFlag(TransparentMouse)) -{ - Region marginDrawnRegion = new(); - foreach (View subView in margin.InternalSubViews) - { - if (subView.Visible) - { - marginDrawnRegion.Union(subView.FrameToScreen()); - } - } - margin.CachedDrawnRegion = marginDrawnRegion.IsEmpty() ? null : marginDrawnRegion; -} -``` - -### 2h. Modify Hit-Testing in `GetViewsUnderLocation` - -**File**: `View.Layout.cs` (in `GetViewsUnderLocation`) - -**Stage 1 — Adornment filtering**: For each adornment (Margin, Border, Padding), change from blanket removal to per-cell check: - -```csharp -if (viewsUnderLocation.Contains(v.Margin) && v.Margin!.ViewportSettings.HasFlag(excludeFlags)) -{ - if (v.Margin.CachedDrawnRegion is null - || !v.Margin.CachedDrawnRegion.Contains(location.X, location.Y)) - { - ret = true; - } -} -// Same pattern for Border and Padding -``` - -**Stage 2 — Direct view filtering**: Change from `RemoveAll(v => v!.ViewportSettings.HasFlag(...))` to: - -```csharp -viewsUnderLocation.RemoveAll(v => -{ - if (!v!.ViewportSettings.HasFlag(excludeFlags)) - return false; - - if (v.CachedDrawnRegion is { } drawnRegion) - return !drawnRegion.Contains(location.X, location.Y); - - return true; // null cache = blanket removal (fallback) -}); -``` - -**Note**: `screenLocation` is an `in` parameter, so it must be captured into a local variable for use inside the lambda. - ---- - -## Rename: `DrawMargins` → `DrawShadows` - -**File**: `Margin.cs`, `View.Drawing.cs` - -The method `DrawMargins` is renamed to `DrawShadows` because it only draws Margins that have shadows. All call sites are updated. Comments referencing "Transparent margins" are updated to reference "shadows". - ---- - -## Files Changed Summary - -| File | Changes | -|------|---------| -| `Adornment.cs` | Remove `SuperViewRendersLineCanvas` throw; make it a standard auto-property | -| `Border.cs` | Write to own `LineCanvas`; set `SuperViewRendersLineCanvas = true`; add `LastTitleRect`; report title to DrawContext | -| `View.Drawing.cs` | Add `CachedDrawnRegion`; add `_lastLineCanvasRegion`; merge Border LineCanvas; relax adornment guard in `DoDrawComplete`; cache drawn regions; skip viewport clear for transparent Border; pass `DrawContext` to adornments | -| `View.NeedsDraw.cs` | Clear `CachedDrawnRegion` in `SetNeedsDraw` | -| `View.Layout.cs` | Per-cell hit-testing in `GetViewsUnderLocation` | -| `ShadowView.cs` | Report drawn region to DrawContext | -| `Margin.cs` | Rename `DrawMargins`→`DrawShadows`; build `CachedDrawnRegion` for shadow Margins | -| `Padding.cs` | *(Minor early-return refactoring only — no transparency-specific changes)* | -| `View.Adornments.cs` | *(Pass-through only — `DrawAdornments` signature gains `DrawContext?` parameter)* | - ---- - -## Edge Cases - -| Case | Expected Behavior | -|------|-------------------| -| Before first draw | `CachedDrawnRegion` is `null`; TransparentMouse falls back to blanket removal (no regression) | -| View never reports drawn regions | Same — null cache, blanket behavior | -| Stale cache between draws | Invalidated by `SetNeedsDraw`; between draws, cache reflects last visual state | -| Layout changes | Trigger `SetNeedsDraw`, which clears cache | -| Border with `Thickness = 0` | No drawn region, fully transparent to mouse | -| Border with no title and no lines | Empty drawn region — all mouse events pass through | -| Thick border (e.g., `Thickness(2)`) | Only the actual line cells are drawn; gap cells between lines pass through | -| Opaque view with `TransparentMouse` | Cache is the entire border frame (all cells receive clicks) | -| Transparent adornment drawn without context (Margin via `DrawShadows`) | Cannot participate in clip exclusion — early return | - ---- - -## Tests - -Two new test files were created (can be copied as-is from this branch): - -### `Tests/UnitTestsParallelizable/ViewBase/Adornment/BorderTransparentTests.cs` - -Tests for Border visual transparency and TransparentMouse: - -| Test | What It Proves | -|------|----------------| -| `Border_Transparent_Shows_Underlying_Content_In_Interior` | Interior of transparent border shows underlying view content | -| `Border_TransparentMouse_Interior_Clicks_Pass_Through` | Mouse clicks on empty interior pass through | -| `Border_TransparentMouse_BorderLine_Clicks_Are_Captured` | Mouse clicks on border lines are received by the border | -| `Border_TransparentMouse_Title_Clicks_Are_Captured` | Mouse clicks on title text are received by the border | -| `View_TransparentMouse_DrawnCells_Captured_UndrawnCells_PassThrough` | Per-cell hit-testing works for regular views | -| `View_TransparentMouse_NullCache_FallsBackToBlanketRemoval` | Null cache = blanket fallback (backward compat) | -| `Border_TransparentMouse_ThickBorder_EmptyCells_PassThrough` | Thick border gap cells pass through | -| `Margin_TransparentMouse_Shadow_Clicks_Are_Captured` | Shadow cells in Margin receive clicks | - -### `Tests/UnitTestsParallelizable/ViewBase/Draw/DoDrawCompleteTests.cs` - -Baseline tests for the clip exclusion logic in `DoDrawComplete`: - -| Test | What It Proves | -|------|----------------| -| `OpaqueView_ExcludesEntireFrameFromClip` | Opaque view punches out its entire borderFrame | -| `OpaqueView_UsesBorderFrameNotViewFrame` | Uses `Border.FrameToScreen()` when Border exists | -| `OpaqueView_UpdatesDrawContext` | Opaque view calls `context.AddDrawnRectangle(borderFrame)` | -| `TransparentView_ExcludesOnlyDrawnRegion` | Transparent view excludes only drawn cells | -| `TransparentView_ClampsDrawnRegionToViewport` | Drawn region is clipped to Viewport bounds | -| `TransparentView_ExcludesBorderAndPadding` | Border/Padding thickness areas always excluded for transparent views | -| `Adornment_SkipsClipExclusion` | Opaque adornments skip clip exclusion | -| `TransparentParent_OpaqueChild_ContextFlows` | Opaque subview's frame added to parent's DrawContext | -| `CachedDrawnRegion_PopulatedAfterDraw_WhenTransparentMouse` | Cache populated after Draw() | -| `CachedDrawnRegion_Null_WhenNotTransparentMouse` | No caching without flag | -| `CachedDrawnRegion_TransparentView_ContainsOnlyDrawnCells` | Only drawn cells cached | -| `CachedDrawnRegion_BorderAdornment_PopulatedAfterDraw` | Border adornment gets cache | -| `CachedDrawnRegion_Border_IncludesTitleAndLines` | Cache has lines + title | -| `CachedDrawnRegion_ClearedBySetNeedsDraw` | SetNeedsDraw invalidates cache | -| `CachedDrawnRegion_RepopulatedAfterRedraw` | Full invalidate-redraw cycle | -| `ShadowView_ReportsDrawnRegionToContext` | Shadow cells in DrawContext | - -### Other modified test files (incidental changes) - -These files had changes in the branch but primarily for adapting to signature changes or refactoring. Review before copying: - -- `Tests/UnitTestsParallelizable/ViewBase/Adornment/AdornmentTests.cs` -- `Tests/UnitTestsParallelizable/ViewBase/Adornment/MarginTests.cs` -- `Tests/UnitTestsParallelizable/ViewBase/Draw/StaticDrawTests.cs` -- `Tests/UnitTestsParallelizable/ViewBase/Draw/ViewDrawingFlowTests.cs` -- `Tests/UnitTests/View/Adornment/BorderTests.cs` -- `Tests/UnitTests/View/Adornment/AdornmentTests.cs` -- `Tests/UnitTests/View/Draw/TransparentTests.cs` - ---- - -## Code Style Changes (Non-Functional) - -The branch also made several code style fixes that are **not related to transparency** but will appear in the diff. These are independent refactorings applied to files being modified: - -1. **Early-return refactoring** — Nested `if/else` blocks converted to guard clauses (in `Margin.cs`, `Padding.cs`, `Adornment.cs`, `View.Drawing.cs`) -2. **Target-typed `new`** — `new()` replaced with `new TypeName()` per project conventions (throughout `Border.cs`, `Margin.cs`, `Adornment.cs`) -3. **`field` keyword** — Backing field `_shadowSize` replaced with C# 12 `field` keyword (in `Margin.cs`, `View.Adornments.cs`) -4. **Null-conditional simplification** — `if (x is { }) { x.Visible = true; }` → `x?.Visible = true` (in `Margin.cs`) -5. **Removed unused `ClearFrame` method** (in `View.Drawing.cs`) - -These should be applied in a separate commit or alongside the transparency work. - ---- - -## Verification Checklist - -1. `dotnet build --no-restore` — zero errors, zero new warnings -2. `dotnet test --project Tests/UnitTestsParallelizable --no-build` — all pass -3. `dotnet test --project Tests/UnitTests --no-build` — all pass -4. `Border_Transparent_Shows_Underlying_Content_In_Interior` — passes (interior shows underlying content) -5. `Border_TransparentMouse_Interior_Clicks_Pass_Through` — passes (correct reason: per-cell, not blanket) -6. `Border_TransparentMouse_BorderLine_Clicks_Are_Captured` — passes -7. UICatalog `Transparent` scenario — transparent views still work -8. UICatalog scenarios with borders/windows — no visual regressions diff --git a/plans/per-cell-transparent-mouse.md b/plans/per-cell-transparent-mouse.md deleted file mode 100644 index 410360f9fd..0000000000 --- a/plans/per-cell-transparent-mouse.md +++ /dev/null @@ -1,232 +0,0 @@ -# Plan: Border Transparent & TransparentMouse Support (Issue #4834) - -## Status - -| Item | Status | -|------|--------| -| Border transparency failing tests | Done | -| DoDrawComplete comment rewrite | Done | -| DoDrawComplete baseline tests | Done — 8 tests, all passing | -| Phase 1: Visual transparency for Border | **DONE** | -| Phase 1a: Fix Border LineCanvas ownership | Done | -| Phase 1b: Make Border participate in clip exclusion when Transparent | Done | -| Phase 1 extras: title occlusion, clip clamping, AnchorEnd tests | Done | -| Phase 2: Drawn-region-aware TransparentMouse | **DONE** — all 28 tests passing | - -## Context - -Issue #4834: `Border` should support `ViewportSettingsFlags.Transparent` and `ViewportSettingsFlags.TransparentMouse`. Prerequisite for TabView redesign (#4183), where each Tab's Border renders a small header rectangle and the rest must be transparent. - -Border draws **non-rectangular** content — an outline of line segments plus optional title text. The interior is empty: - -- **Transparent**: Only border lines and title text are drawn; the empty interior shows underlying views. -- **TransparentMouse**: Clicks on border lines/title go to the Border; clicks on the empty interior pass through. - -## Failing Tests - -Three tests in `Tests/UnitTestsParallelizable/ViewBase/Adornment/BorderTransparentTests.cs`: - -| Test | Status | Why | -|------|--------|-----| -| `Border_Transparent_Shows_Underlying_Content_In_Interior` | **Skip** (not yet implemented) | Border doesn't honor `Transparent` — interior shows spaces, not underlying content | -| `Border_TransparentMouse_Interior_Clicks_Pass_Through` | passes (wrong reason) | Blanket `TransparentMouse` removes Border entirely | -| `Border_TransparentMouse_BorderLine_Clicks_Are_Captured` | **Skip** (not yet implemented) | Blanket `TransparentMouse` removes Border from ALL hits, including border line cells | - -## Baseline Tests for DoDrawComplete - -Eight tests in `Tests/UnitTestsParallelizable/ViewBase/Draw/DoDrawCompleteTests.cs` — all passing: - -| Test | What it verifies | -|------|-----------------| -| `OpaqueView_ExcludesEntireFrameFromClip` | Opaque view punches out its entire borderFrame from Driver.Clip | -| `OpaqueView_UsesBorderFrameNotViewFrame` | When Border exists, uses Border.FrameToScreen() not View.FrameToScreen() | -| `OpaqueView_UpdatesDrawContext` | Opaque view calls `context.AddDrawnRectangle(borderFrame)` so SuperView knows | -| `TransparentView_ExcludesOnlyDrawnRegion` | Transparent view excludes only the actually-drawn cells, not the full frame | -| `TransparentView_ClampsDrawnRegionToViewport` | DrawnRegion is clipped to Viewport bounds before exclusion | -| `TransparentView_ExcludesBorderAndPadding` | Border and Padding thickness areas are always excluded, even for transparent views | -| `Adornment_SkipsClipExclusion` | Adornments (Margin/Border/Padding) do NOT modify Driver.Clip in DoDrawComplete | -| `TransparentParent_OpaqueChild_ContextFlows` | Opaque child's frame is added to parent's DrawContext via AddDrawnRectangle | - -## Root Cause Analysis - -### Problem 1: Visual Transparency doesn't work for Border - -`DoDrawComplete` (View.Drawing.cs:821) has an `if (this is not Adornment)` guard that skips ALL clip exclusion logic for adornments. This means setting `Transparent` on a Border has no effect — the parent view's opaque path excludes the entire border frame from the clip regardless. - -### Problem 2: Border draws to Parent.LineCanvas, not its own - -Border.cs `OnDrawingContent` writes all border lines to `Parent.LineCanvas` (line 309), not to Border's own LineCanvas. The parent renders these later in its own `DoRenderLineCanvas` (View.Drawing.cs:144), and the drawn region gets reported to the **parent's** DrawContext — not the Border's. So the Border's own DrawContext never knows what cells the border lines occupy. - -This was the user's observation: "I suspect [writing to Parent.LineCanvas] was a poor decision back when I did it." The fix should make Border use its own LineCanvas with `SuperViewRendersLineCanvas = true`, so lines are drawn by Border but merged into the parent for rendering. However, `Adornment.SuperViewRendersLineCanvas` currently throws `InvalidOperationException` (Adornment.cs:215-218). - -### Problem 3: TransparentMouse is blanket per-view - -`GetViewsUnderLocation` (View.Layout.cs:1303) does `RemoveAll(v => v!.ViewportSettings.HasFlag(excludeViewportSettingsFlags))` — blanket removal. No per-cell checking. - -## Implementation - -### Phase 1: Visual Transparency for Border - -#### 1a. Fix Border LineCanvas ownership - DONE - -**Problem**: Border writes to `Parent.LineCanvas`. This means the drawn region is tracked on the parent, not the Border. - -**Fix in `Adornment.cs` (~line 215)**: Change `SuperViewRendersLineCanvas` override to allow setting it (remove the throw). Or better: have Adornment return `Parent?.SuperViewRendersLineCanvas ?? false` — meaning adornments' line canvases get merged into their Parent view's LineCanvas (which is where Border already writes today, but through the proper merge mechanism). - -**Fix in `Border.cs` (`OnDrawingContent`, ~line 309)**: Change from writing to `Parent?.LineCanvas` directly to writing to `this.LineCanvas` (Border's own). Set Border's `SuperViewRendersLineCanvas = true` so the parent merges and renders it. - -**Fix in `View.Drawing.cs` (`DoDrawSubViews`, ~line 703-718)**: The merge logic at line 715 (`LineCanvas.Merge(view.LineCanvas)`) uses `SuperView.LineCanvas`. For adornments, the "SuperView" in this context is actually the `Parent` view. Need to ensure the merge target is correct — when the view being drawn is an Adornment, merge into `Parent.LineCanvas` instead of `SuperView.LineCanvas`. - -This change means: -- Border adds lines to its own LineCanvas -- Border's `RenderLineCanvas` is skipped (because `SuperViewRendersLineCanvas = true`) -- Parent's draw loop merges Border's LineCanvas into its own -- Parent's `RenderLineCanvas` renders all lines and reports the drawn region to the parent's context -- Border's own DrawContext tracks what IT drew (title text, etc.) - -#### 1b. Make Border participate in clip exclusion when Transparent - DONE - -**Fix in `View.Drawing.cs` (`DoDrawComplete`, ~line 821)**: Relax the Adornment guard. When an Adornment has `Transparent` set, it should participate in the drawn-region clip exclusion path: - -```csharp -if (this is not Adornment || ViewportSettings.HasFlag (ViewportSettingsFlags.Transparent)) -{ - if (ViewportSettings.HasFlag (ViewportSettingsFlags.Transparent)) - { - // Transparent path: only exclude drawn regions - context!.ClipDrawnRegion (ViewportToScreen (Viewport)); - ExcludeFromClip (context.GetDrawnRegion ()); - } - else if (this is not Adornment) - { - // Opaque non-adornment path (existing code) - ... - } -} -``` - -#### 1c. Prevent viewport clearing for Transparent Border - DONE - -When Border has `Transparent` set, its viewport should NOT be cleared during `DoClearViewport`. The existing `Transparent` flag logic in `ClearViewport` (View.Drawing.cs:414) should handle this — but need to verify it works for adornments too. - -### Phase 2: Drawn-Region-Aware TransparentMouse - -**FIRST**: Plan tests. For each sub-phase, create the tests that will prove functionality. If they will fail until a later sub-phase is completed, mark them with Skip= with an explanaition. **DONE** — 14 tests total (see below). - -**Phase 2 Tests** (all Skip'd until implementation): - -| Test | File | Requires | What it proves | -|------|------|----------|---------------| -| `CachedDrawnRegion_PopulatedAfterDraw_WhenTransparentMouse` | DoDrawCompleteTests | 2a/2b | Cache populated after Draw() | -| `CachedDrawnRegion_Null_WhenNotTransparentMouse` | DoDrawCompleteTests | 2a/2b | No caching without flag | -| `CachedDrawnRegion_TransparentView_ContainsOnlyDrawnCells` | DoDrawCompleteTests | 2a/2b | Only drawn cells cached | -| `CachedDrawnRegion_BorderAdornment_PopulatedAfterDraw` | DoDrawCompleteTests | 2a/2b | Border adornment gets cache | -| `CachedDrawnRegion_Border_IncludesTitleAndLines` | DoDrawCompleteTests | 2a/2b | Cache has lines + title | -| `CachedDrawnRegion_ClearedBySetNeedsDraw` | DoDrawCompleteTests | 2c | SetNeedsDraw invalidates cache | -| `CachedDrawnRegion_RepopulatedAfterRedraw` | DoDrawCompleteTests | 2a/2b/2c | Full invalidate-redraw cycle | -| `ShadowView_ReportsDrawnRegionToContext` | DoDrawCompleteTests | 2d | Shadow cells in DrawContext | -| `Border_TransparentMouse_BorderLine_Clicks_Are_Captured` | BorderTransparentTests | 2e | Border lines receive clicks | -| `Border_TransparentMouse_Title_Clicks_Are_Captured` | BorderTransparentTests | 2e | Title text receives clicks | -| `View_TransparentMouse_DrawnCells_Captured_UndrawnCells_PassThrough` | BorderTransparentTests | 2a/2b/2e | Per-cell hit-testing works | -| `View_TransparentMouse_NullCache_FallsBackToBlanketRemoval` | BorderTransparentTests | 2e | Null cache = blanket fallback | -| `Border_TransparentMouse_ThickBorder_EmptyCells_PassThrough` | BorderTransparentTests | 2a/2b/2e | Thick border gap cells pass through | -| `Margin_TransparentMouse_Shadow_Clicks_Are_Captured` | BorderTransparentTests | 2d/2e | Shadow cells receive clicks | - -#### 2a. Add `_cachedDrawnRegion` to View (`View.Drawing.cs`) - -```csharp -private Region? _cachedDrawnRegion; -internal Region? CachedDrawnRegion => _cachedDrawnRegion; -``` - -#### 2b. Cache drawn region in `DoDrawComplete` (`View.Drawing.cs` ~line 810) - -**Only if `TransparentMouse` is set**: - -In the Transparent path (after clip exclusion), cache `context.GetDrawnRegion()`. For adornments with `TransparentMouse`, add caching after the Adornment guard. - -For Border specifically, the cached region must include both: -- Lines drawn via LineCanvas (reported to context by `RenderLineCanvas`) -- Title text (reported to context by `DrawText`) - -After Phase 1 fixes, the Border's own DrawContext should have the title text region. The LineCanvas region is in the parent's context. We need to ensure the Border's cached region includes both — this may mean the parent passes the line region back to the Border, or the Border caches its region from the parent's context after merge. - -**Alternative approach**: Cache the region at the parent level for all adornments. When hit-testing asks "is this point on the Border?", check the parent view's line canvas drawn region intersected with the Border's frame. This avoids the cross-context issue. - -**Simplest approach**: After the parent renders its LineCanvas (which includes Border's lines), have the parent set `_cachedDrawnRegion` on its Border adornment by intersecting the line canvas region with the Border's frame. This way the Border's cached region accurately reflects the cells where border lines were drawn. - -#### 2c. Invalidate cache in `SetNeedsDraw` (`View.NeedsDraw.cs` ~line 58) - -Clear `_cachedDrawnRegion = null` at the start of `SetNeedsDraw(Rectangle)`. - -#### 2d. Report drawn region in ShadowView (`ShadowView.cs` ~line 47) - -Add `context?.AddDrawnRectangle (ViewportToScreen (Viewport));` at the end of `OnDrawingContent`. - -#### 2e. Modify hit-testing (`View.Layout.cs` ~line 1262) - -In `GetViewsUnderLocation`, change both filtering stages: - -**Stage 1 — Adornment filtering (lines ~1275-1300)**: -```csharp -if (viewsUnderLocation.Contains (v.Margin) && v.Margin!.ViewportSettings.HasFlag (excludeViewportSettingsFlags)) -{ - // Per-cell check: if the adornment has a cached drawn region, check it - if (v.Margin.CachedDrawnRegion is null || !v.Margin.CachedDrawnRegion.Contains (screenLocation.X, screenLocation.Y)) - { - ret = true; - } -} -``` - -Same pattern for Border and Padding checks. - -**Stage 2 — Direct view filtering (line ~1303)**: -```csharp -viewsUnderLocation.RemoveAll (v => -{ - if (!v!.ViewportSettings.HasFlag (excludeViewportSettingsFlags)) - { - return false; - } - - if (v.CachedDrawnRegion is { } drawnRegion) - { - return !drawnRegion.Contains (screenLocation.X, screenLocation.Y); - } - - return true; // null cache = blanket removal (fallback) -}); -``` - -## Files to Modify - -| File | Change | -|------|--------| -| `Terminal.Gui/ViewBase/Adornment/Adornment.cs` | Allow `SuperViewRendersLineCanvas` to be set (remove throw) | -| `Terminal.Gui/ViewBase/Adornment/Border.cs` | Write to own LineCanvas instead of `Parent.LineCanvas`; set `SuperViewRendersLineCanvas = true` | -| `Terminal.Gui/ViewBase/View.Drawing.cs` | Add `_cachedDrawnRegion`; relax Adornment guard in `DoDrawComplete`; cache drawn region; ensure LineCanvas merge works for adornments | -| `Terminal.Gui/ViewBase/View.NeedsDraw.cs` | Clear `_cachedDrawnRegion` in `SetNeedsDraw` | -| `Terminal.Gui/ViewBase/View.Layout.cs` | Drawn-region check in `GetViewsUnderLocation` | -| `Terminal.Gui/ViewBase/Adornment/ShadowView.cs` | Report drawn region to DrawContext | - -## Edge Cases - -| Case | Behavior | -|------|----------| -| Before first draw | `CachedDrawnRegion` is null; falls back to blanket `TransparentMouse` (no regression) | -| View doesn't report drawn regions | Same — null cache, blanket behavior | -| Stale cache between draws | Invalidated by `SetNeedsDraw`; between draws, cache reflects last visual state | -| Layout changes | Trigger `SetNeedsDraw`, which clears cache | -| Border with no lines (Thickness=0) | No drawn region, fully transparent to mouse | -| Existing code that reads `Parent.LineCanvas` from Border | Must be updated to use `this.LineCanvas` or verified to still work after merge | - -## Verification - -1. `dotnet build --no-restore` — zero errors -2. `dotnet test --project Tests/UnitTestsParallelizable --no-build` — no regressions -3. `Border_Transparent_Shows_Underlying_Content_In_Interior` — passes (interior shows underlying content) -4. `Border_TransparentMouse_Interior_Clicks_Pass_Through` — passes (for the right reason now) -5. `Border_TransparentMouse_BorderLine_Clicks_Are_Captured` — passes (border lines receive mouse events) -6. UICatalog `Transparent` scenario — transparent views still work -7. UICatalog scenarios with borders/windows — no visual regressions diff --git a/plans/unified-adornment-transparency.md b/plans/unified-adornment-transparency.md deleted file mode 100644 index fd9dbad7ed..0000000000 --- a/plans/unified-adornment-transparency.md +++ /dev/null @@ -1,107 +0,0 @@ -# Unified Adornment Transparency Plan - -## Problem - -`DoDrawComplete` special-cases Border (uses `_lastLineCanvasRegion` + `LastTitleRect`) while -Margin/Padding get a generic `context.GetDrawnRegion()` intersected with their frame. This is -wrong — all three adornment types should be treated identically: - -**Any adornment that draws content (text, subviews, line canvas) within its thickness should have -those drawn cells be opaque. Undrawn cells in a transparent adornment should be transparent.** - -`LastTitleRect` is just one example of an adornment drawing text. Padding and Margin can draw -text too (`Padding.View.Text = "Pad"`). The current code doesn't handle Padding/Margin text as -opaque cells within a transparent adornment. - -### Current Bugs - -1. **Padding visual transparency doesn't work** — Transparent Padding fills with spaces instead - of showing underlying content. Test `Padding_Transparent_Text_Is_Opaque_Over_Peer_SubViews` - fails: actual shows the underlying window's Text where the padding Text should occlude it. - -2. **Border special-casing** — `DoDrawComplete` builds Border's `CachedDrawnRegion` from - `_lastLineCanvasRegion` + `LastTitleRect`, but Padding/Margin get `context.GetDrawnRegion()` - intersected with their frame. This asymmetry means Border drawn cells are tracked precisely, - but Padding/Margin get the cumulative drawn region of ALL layers. - -3. **Missing test coverage** — No Padding or Margin visual transparency tests. No Padding or - Margin TransparentMouse drawn-cell tests. - -## Design Principle - -Each adornment tracks its OWN drawn region. The parent's `DoDrawComplete` uses each adornment's -individual drawn region uniformly — no special cases for Border vs Padding vs Margin. - -## Approach - -1. **Each AdornmentView reports its drawn cells** during its own draw cycle. BorderView already - does this for `LastTitleRect`. The LineCanvas is rendered by the parent (`DoRenderLineCanvas`) - and stored in `_lastLineCanvasRegion`. Padding/Margin text rendering needs to similarly report - drawn regions. - -2. **DoDrawComplete uses a uniform approach** for all three adornments — no Border special-casing. - For each transparent adornment, build CachedDrawnRegion the same way. - -3. **Per-adornment drawn region tracking** — each AdornmentView (or AdornmentImpl) accumulates - what it drew. Options: - - AdornmentView stores a `DrawnRegion` property set during Draw() - - Parent passes per-adornment DrawContext - - AdornmentImpl stores the drawn region populated by AdornmentView - -## Todos - -### Phase A: Fix Padding/Margin visual transparency - -- [ ] **A1: Investigate why transparent Padding clears to spaces** — Verify PaddingView respects - `ViewportSettings.Transparent` in DoClearViewport. If clearing happens in the parent's draw - cycle (not PaddingView's own Draw), that's the bug. - -- [ ] **A2: Fix transparent Padding/Margin to not clear their area** — When - `Padding.ViewportSettings.Transparent` is set, undrawn cells remain untouched. - -- [ ] **A3: Make Padding/Margin text opaque within transparent adornment** — Drawn text cells - should be excluded from clip, same as Border's title/lines are. - -### Phase B: Unify CachedDrawnRegion logic - -- [ ] **B1: Remove Border-specific CachedDrawnRegion logic** — Replace the special - `_lastLineCanvasRegion` + `LastTitleRect` composition with a uniform approach. - -- [ ] **B2: Implement per-adornment drawn region tracking** — Each AdornmentView accumulates - what it drew during Draw(). Parent's DoDrawComplete reads this. - -- [ ] **B3: Update DoDrawComplete** — Replace three separate code blocks with one uniform - helper that processes all three adornments identically. - -### Phase C: Comprehensive tests - -- [ ] **C1: Padding visual transparency tests** - - `Padding_Transparent_Undrawn_Cells_Show_Underlying_Content` - - `Padding_Transparent_Text_Is_Opaque_Over_Peer_SubViews` (existing, currently failing) - - `Padding_Transparent_SubViews_Are_Opaque` - -- [ ] **C2: Margin visual transparency tests** - - `Margin_Transparent_Undrawn_Cells_Show_Underlying_Content` - - `Margin_Transparent_Text_Is_Opaque` - -- [ ] **C3: Padding TransparentMouse drawn-cell tests** - - `Padding_TransparentMouse_DrawnText_Clicks_Are_Captured` - - `Padding_TransparentMouse_UndrawnCells_PassThrough` - -- [ ] **C4: Margin TransparentMouse drawn-cell tests** - - `Margin_TransparentMouse_DrawnText_Clicks_Are_Captured` - - `Margin_TransparentMouse_UndrawnCells_PassThrough` - -- [ ] **C5: Verify Border tests still pass** — Existing 8 Border tests must continue passing. - -- [ ] **C6: Cross-adornment test** — All three adornments transparent, each with text. Verify - each adornment's text is opaque while undrawn cells are transparent. - -## Key Code Locations - -- `View.Drawing.cs:DoDrawComplete` (~line 846) — clip exclusion + CachedDrawnRegion -- `View.Drawing.cs:DoDrawAdornments` (~line 300) — draws adornment views -- `View.Drawing.cs:DoRenderLineCanvas` (~line 807) — caches `_lastLineCanvasRegion` -- `BorderView.cs:OnDrawingContent` — draws title, reports LastTitleRect -- `PaddingView.cs` / `MarginView.cs` — draw text but don't report drawn regions -- `View.Layout.cs:GetViewsUnderLocation` (~line 1260) — per-cell hit-testing