diff --git a/Terminal.Gui/ViewBase/View.Layout.cs b/Terminal.Gui/ViewBase/View.Layout.cs index c6688cdd4e..495789d6c6 100644 --- a/Terminal.Gui/ViewBase/View.Layout.cs +++ b/Terminal.Gui/ViewBase/View.Layout.cs @@ -67,11 +67,14 @@ public Rectangle Frame return; } - // BUGBUG: We set the internal fields here to avoid recursion. However, this means that - // BUGBUG: other logic in the property setters does not get executed. Specifically: - // BUGBUG: - Reset TextFormatter - // BUGBUG: - SetLayoutNeeded (not an issue as we explicitly call Layout below) - // BUGBUG: - If we add property change events for X/Y/Width/Height they will not be invoked + // We set the internal fields directly (not via the X/Y/Width/Height setters) to avoid recursion. + // This is intentional and the two side effects of the setters are accounted for elsewhere (#5498): + // - TextFormatter constraints: recomputed by the Layout () -> SetRelativeLayout call below + // (SetTextFormatterSize + FinalizeTextFormatterConstraints), so no reset is needed here. + // - Width/Height (and X/Y) change events: deliberately NOT raised here. Those events observe + // declarative Dim/Pos assignment; this path overwrites them with Absolute values as bookkeeping + // to keep state consistent with an imperatively-set Frame. Use FrameChanged (raised by SetFrame + // above) to observe resolved-size changes from any cause. // If Frame gets set, set all Pos/Dim to Absolute values. _x = _frame!.Value.X; _y = _frame!.Value.Y; @@ -152,6 +155,14 @@ protected virtual void OnFrameChanged (in Rectangle frame) { } /// Raised when the changes. This event is raised after the has been /// updated. /// + /// + /// This is the canonical event for observing resolved-size and position changes. It fires whenever the + /// absolute changes for any reason: assigning , , + /// , or ; setting directly; or a layout pass + /// resolving a relative / (e.g. ). By contrast, the + /// / events observe assignment of the declarative + /// / only. + /// public event EventHandler>? FrameChanged; /// Gets the with a screen-relative location. @@ -368,6 +379,13 @@ public Pos Y /// allowing customization or cancellation of the change. The event /// is raised before the change, and is raised after. /// + /// + /// / observe assignment of this declarative + /// only. They are deliberately not raised when a layout pass resolves the view's size + /// or when is set directly, because those paths change the resolved + /// rather than the declarative . To observe resolved-size + /// changes from any cause, subscribe to . + /// /// The default value is Dim.Absolute (0). /// /// @@ -462,6 +480,13 @@ protected virtual void OnHeightChanged (ValueChangedEventArgs args) { } /// allowing customization or cancellation of the change. The event /// is raised before the change, and is raised after. /// + /// + /// / observe assignment of this declarative + /// only. They are deliberately not raised when a layout pass resolves the view's size + /// or when is set directly, because those paths change the resolved + /// rather than the declarative . To observe resolved-size + /// changes from any cause, subscribe to . + /// /// The default value is Dim.Absolute (0). /// /// @@ -686,11 +711,14 @@ public bool SetRelativeLayout (Size superviewContentSize) _suppressNeedsDrawAfterLayout = false; } - // BUGBUG: We set the internal fields here to avoid recursion. However, this means that - // BUGBUG: other logic in the property setters does not get executed. Specifically: - // BUGBUG: - Reset TextFormatter - // BUGBUG: - SetLayoutNeeded (not an issue as we explicitly call Layout below) - // BUGBUG: - If we add property change events for X/Y/Width/Height they will not be invoked + // We update the internal fields directly (not via the X/Y/Width/Height setters) to avoid recursion. + // This is intentional and the two side effects of the setters are accounted for here (#5498): + // - TextFormatter constraints: already recomputed by SetTextFormatterSize () above and finalized by + // FinalizeTextFormatterConstraints () below, so no reset is needed here. + // - Width/Height (and X/Y) change events: deliberately NOT raised. A layout pass resolves the Frame + // without changing the declarative Dim/Pos (e.g. Dim.Fill () stays Dim.Fill ()); these assignments + // only collapse an already-Absolute dim onto its resolved value. FrameChanged (raised by SetFrame + // above) is the event for resolved-size changes. if (_x is PosAbsolute) { _x = Frame.X; diff --git a/Tests/UnitTestsParallelizable/ViewBase/Layout/ViewLayoutEventTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Layout/ViewLayoutEventTests.cs index 6583f0a0b7..193bad175d 100644 --- a/Tests/UnitTestsParallelizable/ViewBase/Layout/ViewLayoutEventTests.cs +++ b/Tests/UnitTestsParallelizable/ViewBase/Layout/ViewLayoutEventTests.cs @@ -283,6 +283,172 @@ public void View_SubViewLayout_SubViewsLaidOut_Events_Fires_EvenWidthOrHeightIsZ Assert.Equal (new Size (0, 0), oldValue); } + // The following tests lock the Width/Height vs Frame event contract (#5498): + // WidthChanged/HeightChanged observe declarative Dim assignment only; FrameChanged observes + // resolved-size changes from any cause (Frame setter, declarative assignment, or a layout pass). + + // Claude - Opus 4.8 + [Fact] + public void View_FrameSetter_DoesNotRaise_WidthChanged () + { + View view = new () { Width = Dim.Fill (), Height = Dim.Fill () }; + bool fired = false; + + view.WidthChanged += (_, _) => fired = true; + + view.Frame = new (0, 0, 10, 5); + + // The Frame setter converts Width to Dim.Absolute as bookkeeping, but that is not a + // declarative Width assignment, so WidthChanged must not fire. + Assert.False (fired); + Assert.Equal (Dim.Absolute (10), view.Width); + } + + // Claude - Opus 4.8 + [Fact] + public void View_FrameSetter_DoesNotRaise_HeightChanged () + { + View view = new () { Width = Dim.Fill (), Height = Dim.Fill () }; + bool fired = false; + + view.HeightChanged += (_, _) => fired = true; + + view.Frame = new (0, 0, 10, 5); + + Assert.False (fired); + Assert.Equal (Dim.Absolute (5), view.Height); + } + + // Claude - Opus 4.8 + [Fact] + public void View_FrameSetter_Raises_FrameChanged () + { + View view = new (); + Rectangle? observed = null; + + view.FrameChanged += (_, args) => observed = args.Value; + + view.Frame = new (1, 2, 10, 5); + + Assert.Equal (new Rectangle (1, 2, 10, 5), observed); + } + + [Fact] + public void View_FrameSetter_DoesNotRaise_WidthHeightChangingChanged_Or_OnWidthHeightChangingChanged () + { + EventProbeView view = new () { Width = Dim.Fill (), Height = Dim.Fill () }; + view.ResetCounts (); + int changingEvents = 0; + int changedEvents = 0; + + view.WidthChanging += (_, _) => changingEvents++; + view.HeightChanging += (_, _) => changingEvents++; + view.WidthChanged += (_, _) => changedEvents++; + view.HeightChanged += (_, _) => changedEvents++; + + view.Frame = new (0, 0, 10, 5); + + Assert.Equal (0, changingEvents); + Assert.Equal (0, changedEvents); + Assert.Equal (0, view.WidthChangingCount); + Assert.Equal (0, view.HeightChangingCount); + Assert.Equal (0, view.WidthChangedCount); + Assert.Equal (0, view.HeightChangedCount); + } + + // Claude - Opus 4.8 + [Fact] + public void View_LayoutDrivenResize_DoesNotRaise_WidthChanged_Or_HeightChanged () + { + View container = new () { Width = 20, Height = 10 }; + View child = new () { Width = Dim.Fill (), Height = Dim.Fill () }; + container.Add (child); + container.Layout (); + Assert.Equal (20, child.Frame.Width); + + bool fired = false; + child.WidthChanged += (_, _) => fired = true; + child.HeightChanged += (_, _) => fired = true; + + // Growing the container re-resolves the child's Dim.Fill () to a new absolute size during + // layout. The declarative Width/Height (still Dim.Fill ()) does not change, so no event fires. + container.Width = 30; + container.Height = 14; + container.Layout (); + + Assert.False (fired); + Assert.Equal (30, child.Frame.Width); + Assert.Equal (14, child.Frame.Height); + Assert.Equal (Dim.Fill (), child.Width); + Assert.Equal (Dim.Fill (), child.Height); + } + + [Fact] + public void View_LayoutDrivenResize_DoesNotRaise_WidthHeightChangingChanged_Or_OnWidthHeightChangingChanged () + { + View container = new () { Width = 20, Height = 10 }; + EventProbeView child = new () { Width = Dim.Fill (), Height = Dim.Fill () }; + container.Add (child); + container.Layout (); + child.ResetCounts (); + + int changingEvents = 0; + int changedEvents = 0; + child.WidthChanging += (_, _) => changingEvents++; + child.HeightChanging += (_, _) => changingEvents++; + child.WidthChanged += (_, _) => changedEvents++; + child.HeightChanged += (_, _) => changedEvents++; + + container.Width = 30; + container.Height = 14; + container.Layout (); + + Assert.Equal (0, changingEvents); + Assert.Equal (0, changedEvents); + Assert.Equal (0, child.WidthChangingCount); + Assert.Equal (0, child.HeightChangingCount); + Assert.Equal (0, child.WidthChangedCount); + Assert.Equal (0, child.HeightChangedCount); + } + + // Claude - Opus 4.8 + [Fact] + public void View_LayoutDrivenResize_Raises_FrameChanged () + { + View container = new () { Width = 20, Height = 10 }; + View child = new () { Width = Dim.Fill (), Height = Dim.Fill () }; + container.Add (child); + container.Layout (); + + bool fired = false; + child.FrameChanged += (_, _) => fired = true; + + container.Width = 30; + container.Layout (); + + Assert.True (fired); + Assert.Equal (30, child.Frame.Width); + } + + // Claude - Opus 4.8 + [Fact] + public void View_DeclarativeWidthAssignment_Raises_FrameChanged_AfterLayout () + { + View container = new () { Width = 50, Height = 20 }; + View child = new () { Width = 10, Height = 5 }; + container.Add (child); + container.Layout (); + + bool fired = false; + child.FrameChanged += (_, _) => fired = true; + + child.Width = 25; + container.Layout (); + + Assert.True (fired); + Assert.Equal (25, child.Frame.Width); + } + private class TestView : View { public bool CancelWidthChange { get; set; } @@ -298,4 +464,44 @@ protected override bool OnHeightChanging (ValueChangingEventArgs args) return CancelHeightChange; } } + + private class EventProbeView : View + { + public int HeightChangedCount { get; private set; } + public int HeightChangingCount { get; private set; } + public int WidthChangedCount { get; private set; } + public int WidthChangingCount { get; private set; } + + public void ResetCounts () + { + HeightChangedCount = 0; + HeightChangingCount = 0; + WidthChangedCount = 0; + WidthChangingCount = 0; + } + + protected override void OnHeightChanged (ValueChangedEventArgs args) + { + HeightChangedCount++; + } + + protected override bool OnHeightChanging (ValueChangingEventArgs args) + { + HeightChangingCount++; + + return false; + } + + protected override void OnWidthChanged (ValueChangedEventArgs args) + { + WidthChangedCount++; + } + + protected override bool OnWidthChanging (ValueChangingEventArgs args) + { + WidthChangingCount++; + + return false; + } + } }