Kh/5498 width height event contract

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 <see cref="Frame"/> changes. This event is raised after the <see cref="Frame"/> has been
     ///     updated.
     /// </summary>
+    /// <remarks>
+    ///     This is the canonical event for observing resolved-size and position changes. It fires whenever the
+    ///     absolute <see cref="Frame"/> changes for any reason: assigning <see cref="X"/>, <see cref="Y"/>,
+    ///     <see cref="Width"/>, or <see cref="Height"/>; setting <see cref="Frame"/> directly; or a layout pass
+    ///     resolving a relative <see cref="Dim"/>/<see cref="Pos"/> (e.g. <see cref="DimFill"/>). By contrast, the
+    ///     <see cref="WidthChanged"/>/<see cref="HeightChanged"/> events observe assignment of the declarative
+    ///     <see cref="Width"/>/<see cref="Height"/> <see cref="Dim"/> only.
+    /// </remarks>
     public event EventHandler<EventArgs<Rectangle>>? FrameChanged;
 
     /// <summary>Gets the <see cref="Frame"/> with a screen-relative location.</summary>
@@ -368,6 +379,13 @@ public Pos Y
     ///         allowing customization or cancellation of the change. The <see cref="HeightChanging"/> event
     ///         is raised before the change, and <see cref="HeightChanged"/> is raised after.
     ///     </para>
+    ///     <para>
+    ///         <see cref="HeightChanging"/>/<see cref="HeightChanged"/> observe assignment of this declarative
+    ///         <see cref="Dim"/> only. They are deliberately not raised when a layout pass resolves the view's size
+    ///         or when <see cref="Frame"/> is set directly, because those paths change the resolved
+    ///         <see cref="Frame"/> rather than the declarative <see cref="Height"/>. To observe resolved-size
+    ///         changes from any cause, subscribe to <see cref="FrameChanged"/>.
+    ///     </para>
     ///     <para>The default value is <c>Dim.Absolute (0)</c>.</para>
     /// </remarks>
     /// <seealso cref="HeightChanging"/>
@@ -462,6 +480,13 @@ protected virtual void OnHeightChanged (ValueChangedEventArgs<Dim> args) { }
     ///         allowing customization or cancellation of the change. The <see cref="WidthChanging"/> event
     ///         is raised before the change, and <see cref="WidthChanged"/> is raised after.
     ///     </para>
+    ///     <para>
+    ///         <see cref="WidthChanging"/>/<see cref="WidthChanged"/> observe assignment of this declarative
+    ///         <see cref="Dim"/> only. They are deliberately not raised when a layout pass resolves the view's size
+    ///         or when <see cref="Frame"/> is set directly, because those paths change the resolved
+    ///         <see cref="Frame"/> rather than the declarative <see cref="Width"/>. To observe resolved-size
+    ///         changes from any cause, subscribe to <see cref="FrameChanged"/>.
+    ///     </para>
     ///     <para>The default value is <c>Dim.Absolute (0)</c>.</para>
     /// </remarks>
     /// <seealso cref="WidthChanging"/>
@@ -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;

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<Dim> 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<Dim> args)
+        {
+            HeightChangedCount++;
+        }
+
+        protected override bool OnHeightChanging (ValueChangingEventArgs<Dim> args)
+        {
+            HeightChangingCount++;
+
+            return false;
+        }
+
+        protected override void OnWidthChanged (ValueChangedEventArgs<Dim> args)
+        {
+            WidthChangedCount++;
+        }
+
+        protected override bool OnWidthChanging (ValueChangingEventArgs<Dim> args)
+        {
+            WidthChangingCount++;
+
+            return false;
+        }
+    }
 }