diff --git a/Examples/UICatalog/Scenarios/EditorsAndHelpers/EditorBase.cs b/Examples/UICatalog/Scenarios/EditorsAndHelpers/EditorBase.cs index 9556be4a2..b37b9f02b 100644 --- a/Examples/UICatalog/Scenarios/EditorsAndHelpers/EditorBase.cs +++ b/Examples/UICatalog/Scenarios/EditorsAndHelpers/EditorBase.cs @@ -158,14 +158,23 @@ public abstract class EditorBase : View } /// - protected override void Dispose (bool disposing) + protected override bool OnSuperViewChanging (ValueChangingEventArgs args) { - if (disposing && App is {}) + // Clean up event handlers before SuperView is set to null + // This ensures App is still accessible for proper cleanup + if (App is {}) { App.Navigation!.FocusedChanged -= NavigationOnFocusedChanged; App.Mouse.MouseEvent -= ApplicationOnMouseEvent; } + return base.OnSuperViewChanging (args); + } + + /// + protected override void Dispose (bool disposing) + { + // Event handlers are now cleaned up in OnSuperViewChanging base.Dispose (disposing); } } diff --git a/Terminal.Gui/App/CWP/CWPPropertyHelper.cs b/Terminal.Gui/App/CWP/CWPPropertyHelper.cs index d7095fd7e..739502115 100644 --- a/Terminal.Gui/App/CWP/CWPPropertyHelper.cs +++ b/Terminal.Gui/App/CWP/CWPPropertyHelper.cs @@ -20,6 +20,8 @@ public static class CWPPropertyHelper /// The type of the property value, which may be a nullable reference type (e.g., /// ?). /// + /// The sender of the event. Will be provided as the sender in + /// and . /// /// Reference to the current property value, which may be null for nullable types. If the change is not cancelled, this /// will be set to . @@ -53,6 +55,7 @@ public static class CWPPropertyHelper /// /// public static bool ChangeProperty ( + object? sender, ref T currentValue, T newValue, Func, bool>? onChanging, @@ -85,7 +88,7 @@ public static class CWPPropertyHelper } // BUGBUG: This should pass this not null; need to test - changingEvent?.Invoke (null, args); + changingEvent?.Invoke (sender, args); if (args.Handled) { diff --git a/Terminal.Gui/ViewBase/View.Drawing.Scheme.cs b/Terminal.Gui/ViewBase/View.Drawing.Scheme.cs index 5ad8c0101..4ecf644b3 100644 --- a/Terminal.Gui/ViewBase/View.Drawing.Scheme.cs +++ b/Terminal.Gui/ViewBase/View.Drawing.Scheme.cs @@ -25,6 +25,7 @@ public partial class View set { CWPPropertyHelper.ChangeProperty ( + this, ref _schemeName, value, OnSchemeNameChanging, @@ -208,6 +209,7 @@ public partial class View public bool SetScheme (Scheme? scheme) { return CWPPropertyHelper.ChangeProperty ( + this, ref _scheme, scheme, OnSettingScheme, diff --git a/Terminal.Gui/ViewBase/View.Hierarchy.cs b/Terminal.Gui/ViewBase/View.Hierarchy.cs index 53848fbfd..9c27dfdc6 100644 --- a/Terminal.Gui/ViewBase/View.Hierarchy.cs +++ b/Terminal.Gui/ViewBase/View.Hierarchy.cs @@ -1,4 +1,3 @@ -using System.Collections.ObjectModel; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -6,7 +5,6 @@ namespace Terminal.Gui.ViewBase; public partial class View // SuperView/SubView hierarchy management (SuperView, SubViews, Add, Remove, etc.) { - [SuppressMessage ("Style", "IDE1006:Naming Styles", Justification = "")] private static readonly IReadOnlyCollection _empty = []; private readonly List? _subviews = []; @@ -27,48 +25,73 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// Gets this Views SuperView (the View's container), or if this view has not been added as a /// SubView. /// + /// + /// /// /// - public View? SuperView - { - get => _superView!; - private set => SetSuperView (value); - } + public View? SuperView => _superView!; - private void SetSuperView (View? value) + /// + /// INTERNAL: Sets the SuperView of this View. + /// + /// + /// if the SuperView was changed; otherwise, . + private bool SetSuperView (View? value) { if (_superView == value) { - return; + return true; } - _superView = value; - RaiseSuperViewChanged (); + return CWPPropertyHelper.ChangeProperty ( + this, + ref _superView, + value, + OnSuperViewChanging, + SuperViewChanging, + newValue => _superView = newValue, + OnSuperViewChanged, + SuperViewChanged, + out View? _); } - private void RaiseSuperViewChanged () - { - SuperViewChangedEventArgs args = new (SuperView, this); - OnSuperViewChanged (args); + /// + /// Called when the SuperView of this View is about to be changed. This is called before the SuperView property + /// is updated, allowing access to the current SuperView and its resources (such as ) for + /// cleanup purposes. + /// + /// Hold the new SuperView that will be set, or if being removed. + /// to cancel the change; to allow it. + protected virtual bool OnSuperViewChanging (ValueChangingEventArgs args) => false; - SuperViewChanged?.Invoke (this, args); - } + /// + /// Raised when the SuperView of this View is about to be changed. This is raised before the SuperView property + /// is updated, allowing access to the current SuperView and its resources (such as ) for + /// cleanup purposes. + /// + /// + /// + /// This event follows the Cancellable Work Pattern (CWP). Set + /// to in the event args to cancel the change. + /// + /// + public event EventHandler>? SuperViewChanging; /// /// Called when the SuperView of this View has changed. /// - /// - protected virtual void OnSuperViewChanged (SuperViewChangedEventArgs e) { } + protected virtual void OnSuperViewChanged (ValueChangedEventArgs args) { } /// Raised when the SuperView of this View has changed. - public event EventHandler? SuperViewChanged; + public event EventHandler>? SuperViewChanged; #region AddRemove + // TODO: Make this non-virtual once WizardStep is refactored to use events /// Adds a SubView (child) to this view. /// /// - /// The Views that have been added to this view can be retrieved via the property. + /// The Views that have been added to this view can be retrieved via the property. /// /// /// To check if a View has been added to this View, compare it's property to this View. @@ -90,7 +113,10 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// /// /// - + /// + /// + /// + /// public virtual View? Add (View? view) { if (view is null) @@ -99,7 +125,7 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, } //Debug.Assert (view.SuperView is null, $"{view} already has a SuperView: {view.SuperView}."); - if (view.SuperView is {}) + if (view.SuperView is { }) { Logging.Warning ($"{view} already has a SuperView: {view.SuperView}."); } @@ -110,12 +136,19 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, Logging.Warning ($"{view} has already been Added to {this}."); } - // Ensure views don't have focus when being added - view.HasFocus = false; - // TODO: Make this thread safe InternalSubViews.Add (view); - view.SuperView = this; + + // Try to set the SuperView - this may be cancelled + if (!view.SetSuperView (this)) + { + InternalSubViews.Remove (view); + // The change was cancelled + return null; + } + + // Ensure views don't have focus when being added + view.HasFocus = false; if (view is { Enabled: true, Visible: true, CanFocus: true }) { @@ -193,6 +226,7 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// public event EventHandler? SubViewAdded; + // TODO: Make this non-virtual once WizardStep is refactored to use events /// Removes a SubView added via or from this View. /// /// @@ -210,8 +244,15 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// /// The removed View. if the View could not be removed. /// + /// + /// + /// /// - /// "/> + /// + /// + /// + /// + /// public virtual View? Remove (View? view) { if (view is null) @@ -221,7 +262,7 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, if (InternalSubViews.Count == 0) { - return view; + return view; } if (view.SuperView is null) @@ -256,18 +297,28 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, Debug.Assert (!view.HasFocus); + View? previousSuperView = view.SuperView; + + // Try to clear the SuperView - this may be cancelled + if (!view.SetSuperView (null)) + { + // The change was cancelled, restore state and return null + view.CanFocus = couldFocus; + + return null; + } + + Debug.Assert(view.SuperView is null); InternalSubViews.Remove (view); // Clean up focus stuff _previouslyFocused = null; - if (view.SuperView is { } && view.SuperView._previouslyFocused == this) + if (previousSuperView is { } && previousSuperView._previouslyFocused == this) { - view.SuperView._previouslyFocused = null; + previousSuperView._previouslyFocused = null; } - view.SuperView = null; - SetNeedsLayout (); SetNeedsDraw (); @@ -306,6 +357,7 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// Raised when a SubView has been added to this View. public event EventHandler? SubViewRemoved; + // TODO: Make this non-virtual once WizardStep is refactored to use events /// /// Removes all SubViews added via or from this View. /// @@ -320,12 +372,23 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// /// A list of removed Views. /// + /// + /// + /// + /// + /// + /// + /// + /// + /// public virtual IReadOnlyCollection RemoveAll () { - List removedList = new List (); + List removedList = new (); + while (InternalSubViews.Count > 0) { View? removed = Remove (InternalSubViews [0]); + if (removed is { }) { removedList.Add (removed); @@ -349,14 +412,16 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// /// A list of removed Views. /// - public virtual IReadOnlyCollection RemoveAll () where TView : View + public IReadOnlyCollection RemoveAll () where TView : View { - List removedList = new List (); + List removedList = new (); + foreach (TView view in InternalSubViews.OfType ().ToList ()) { Remove (view); removedList.Add (view); } + return removedList.AsReadOnly (); } diff --git a/Terminal.Gui/ViewBase/View.Layout.cs b/Terminal.Gui/ViewBase/View.Layout.cs index 618acb6e3..e5915bc5e 100644 --- a/Terminal.Gui/ViewBase/View.Layout.cs +++ b/Terminal.Gui/ViewBase/View.Layout.cs @@ -327,6 +327,7 @@ public partial class View // Layout APIs set { CWPPropertyHelper.ChangeProperty ( + this, ref _height, value, OnHeightChanging, @@ -415,6 +416,7 @@ public partial class View // Layout APIs set { CWPPropertyHelper.ChangeProperty ( + this, ref _width, value, OnWidthChanging, diff --git a/Terminal.Gui/Views/Menu/MenuBar.cs b/Terminal.Gui/Views/Menu/MenuBar.cs index 82882b979..1c02d6735 100644 --- a/Terminal.Gui/Views/Menu/MenuBar.cs +++ b/Terminal.Gui/Views/Menu/MenuBar.cs @@ -93,7 +93,6 @@ public class MenuBar : Menu, IDesignable BorderStyle = DefaultBorderStyle; ConfigurationManager.Applied += OnConfigurationManagerApplied; - SuperViewChanged += OnSuperViewChanged; return; @@ -102,7 +101,8 @@ public class MenuBar : Menu, IDesignable bool? MoveRight (ICommandContext? ctx) { return AdvanceFocus (NavigationDirection.Forward, TabBehavior.TabStop); } } - private void OnSuperViewChanged (object? sender, SuperViewChangedEventArgs e) + /// + protected override void OnSuperViewChanged (ValueChangedEventArgs e) { if (SuperView is null) { @@ -758,7 +758,6 @@ public class MenuBar : Menu, IDesignable if (disposing) { - SuperViewChanged += OnSuperViewChanged; ConfigurationManager.Applied -= OnConfigurationManagerApplied; } } diff --git a/Terminal.Gui/Views/StatusBar.cs b/Terminal.Gui/Views/StatusBar.cs index f2bc31af6..022ba0033 100644 --- a/Terminal.Gui/Views/StatusBar.cs +++ b/Terminal.Gui/Views/StatusBar.cs @@ -31,10 +31,10 @@ public class StatusBar : Bar, IDesignable SchemeName = SchemeManager.SchemesToSchemeName (Schemes.Menu); ConfigurationManager.Applied += OnConfigurationManagerApplied; - SuperViewChanged += OnSuperViewChanged; } - private void OnSuperViewChanged (object? sender, SuperViewChangedEventArgs e) + /// + protected override void OnSuperViewChanged (ValueChangedEventArgs e) { if (SuperView is null) { @@ -174,7 +174,6 @@ public class StatusBar : Bar, IDesignable { base.Dispose (disposing); - SuperViewChanged -= OnSuperViewChanged; ConfigurationManager.Applied -= OnConfigurationManagerApplied; } } diff --git a/Terminal.Gui/Views/TextInput/TextField.cs b/Terminal.Gui/Views/TextInput/TextField.cs index ca9e6f519..72d2dc8ec 100644 --- a/Terminal.Gui/Views/TextInput/TextField.cs +++ b/Terminal.Gui/Views/TextInput/TextField.cs @@ -42,8 +42,6 @@ public class TextField : View, IDesignable Initialized += TextField_Initialized; - SuperViewChanged += TextField_SuperViewChanged; - // Things this view knows how to do AddCommand ( Command.DeleteCharRight, @@ -1776,9 +1774,11 @@ public class TextField : View, IDesignable } } - private void TextField_SuperViewChanged (object sender, SuperViewChangedEventArgs e) + /// + protected override void OnSuperViewChanged (ValueChangedEventArgs args) { - if (e.SuperView is { }) + base.OnSuperViewChanged (args); + if (SuperView is { }) { if (Autocomplete.HostControl is null) { @@ -1792,6 +1792,7 @@ public class TextField : View, IDesignable } } + private void TextField_Initialized (object sender, EventArgs e) { _cursorPosition = Text.GetRuneCount (); diff --git a/Terminal.Gui/Views/TextInput/TextView.cs b/Terminal.Gui/Views/TextInput/TextView.cs index 8e438abb7..30c486518 100644 --- a/Terminal.Gui/Views/TextInput/TextView.cs +++ b/Terminal.Gui/Views/TextInput/TextView.cs @@ -120,8 +120,6 @@ public class TextView : View, IDesignable Initialized += TextView_Initialized!; - SuperViewChanged += TextView_SuperViewChanged!; - SubViewsLaidOut += TextView_LayoutComplete; // Things this view knows how to do @@ -4626,9 +4624,12 @@ public class TextView : View, IDesignable return Encoding.Unicode.GetString (encoded, 0, offset); } - private void TextView_SuperViewChanged (object sender, SuperViewChangedEventArgs e) + + /// + protected override void OnSuperViewChanged (ValueChangedEventArgs args) { - if (e.SuperView is { }) + base.OnSuperViewChanged (args); + if (SuperView is { }) { if (Autocomplete.HostControl is null) { diff --git a/Tests/UnitTestsParallelizable/ViewBase/SubviewTests.cs b/Tests/UnitTestsParallelizable/ViewBase/SubviewTests.cs index 3835dfa88..cbbcc9081 100644 --- a/Tests/UnitTestsParallelizable/ViewBase/SubviewTests.cs +++ b/Tests/UnitTestsParallelizable/ViewBase/SubviewTests.cs @@ -18,7 +18,7 @@ public class SubViewTests }; sub.SuperViewChanged += (s, e) => { - if (e.SuperView is { }) + if (sub.SuperView is { }) { subRaisedCount++; } @@ -46,7 +46,7 @@ public class SubViewTests }; sub.SuperViewChanged += (s, e) => { - if (e.SuperView is null) + if (sub.SuperView is null) { subRaisedCount++; } @@ -393,23 +393,23 @@ public class SubViewTests var superView = new View (); int superViewChangedCount = 0; - //int superViewChangingCount = 0; + int superViewChangingCount = 0; view.SuperViewChanged += (s, e) => { superViewChangedCount++; }; - //view.SuperViewChanging += (s, e) => - //{ - // superViewChangingCount++; - //}; + view.SuperViewChanging += (s, e) => + { + superViewChangingCount++; + }; // Act superView.Add (view); // Assert - //Assert.Equal (1, superViewChangingCount); + Assert.Equal (1, superViewChangingCount); Assert.Equal (1, superViewChangedCount); } @@ -692,4 +692,295 @@ public class SubViewTests Assert.Single (removedViews); Assert.Contains (subView2, removedViews); } + + [Fact] + public void SuperViewChanging_Raised_Before_SuperViewChanged () + { + // Arrange + var superView = new View (); + var subView = new View (); + + var events = new List (); + + subView.SuperViewChanging += (s, e) => { events.Add ("SuperViewChanging"); }; + + subView.SuperViewChanged += (s, e) => { events.Add ("SuperViewChanged"); }; + + // Act + superView.Add (subView); + + // Assert + Assert.Equal (2, events.Count); + Assert.Equal ("SuperViewChanging", events [0]); + Assert.Equal ("SuperViewChanged", events [1]); + } + + [Fact] + public void SuperViewChanging_Provides_OldSuperView_On_Add () + { + // Arrange + var superView = new View (); + var subView = new View (); + + View? currentValueInEvent = new View (); // Set to non-null to ensure it gets updated + View? newValueInEvent = null; + + subView.SuperViewChanging += (s, e) => + { + currentValueInEvent = e.CurrentValue; + newValueInEvent = e.NewValue; + }; + + // Act + superView.Add (subView); + + // Assert + Assert.Null (currentValueInEvent); // Was null before add + Assert.Equal (superView, newValueInEvent); // Will be superView after add + } + + [Fact] + public void SuperViewChanging_Provides_OldSuperView_On_Remove () + { + // Arrange + var superView = new View (); + var subView = new View (); + + superView.Add (subView); + + View? currentValueInEvent = null; + View? newValueInEvent = new View (); // Set to non-null to ensure it gets updated + + subView.SuperViewChanging += (s, e) => + { + currentValueInEvent = e.CurrentValue; + newValueInEvent = e.NewValue; + }; + + // Act + superView.Remove (subView); + + // Assert + Assert.Equal (superView, currentValueInEvent); // Was superView before remove + Assert.Null (newValueInEvent); // Will be null after remove + } + + [Fact] + public void SuperViewChanging_Allows_Access_To_App_Before_Remove () + { + // Arrange + using IApplication app = Application.Create (); + var runnable = new Runnable (); + var subView = new View (); + + runnable.Add (subView); + SessionToken? token = app.Begin (runnable); + + IApplication? appInEvent = null; + + subView.SuperViewChanging += (s, e) => + { + Assert.NotNull (s); + // At this point, SuperView is still set, so App should be accessible + appInEvent = (s as View)?.App; + }; + + + Assert.NotNull (runnable.App); + + // Act + runnable.Remove (subView); + + // Assert + Assert.NotNull (appInEvent); + Assert.Equal (app, appInEvent); + + app.End (token!); + runnable.Dispose (); + } + + [Fact] + public void OnSuperViewChanging_Called_Before_OnSuperViewChanged () + { + // Arrange + var superView = new View (); + var events = new List (); + + var subView = new TestViewWithSuperViewEvents (events); + + // Act + superView.Add (subView); + + // Assert + Assert.Equal (2, events.Count); + Assert.Equal ("OnSuperViewChanging", events [0]); + Assert.Equal ("OnSuperViewChanged", events [1]); + } + + [Fact] + public void SuperViewChanging_Raised_When_Changing_Between_SuperViews () + { + // Arrange + var superView1 = new View (); + var superView2 = new View (); + var subView = new View (); + + superView1.Add (subView); + + View? currentValueInEvent = null; + View? newValueInEvent = null; + + subView.SuperViewChanging += (s, e) => + { + currentValueInEvent = e.CurrentValue; + newValueInEvent = e.NewValue; + }; + + // Act + superView2.Add (subView); + + // Assert + Assert.Equal (superView1, currentValueInEvent); + Assert.Equal (superView2, newValueInEvent); + } + + // Helper class for testing virtual method calls + private class TestViewWithSuperViewEvents : View + { + private readonly List _events; + + public TestViewWithSuperViewEvents (List events) { _events = events; } + + protected override bool OnSuperViewChanging (ValueChangingEventArgs args) + { + _events.Add ("OnSuperViewChanging"); + return base.OnSuperViewChanging (args); + } + + protected override void OnSuperViewChanged (ValueChangedEventArgs args) + { + _events.Add ("OnSuperViewChanged"); + base.OnSuperViewChanged (args); + } + } + + [Fact] + public void SuperViewChanging_Can_Be_Cancelled_Via_Event () + { + // Arrange + var superView = new View (); + var subView = new View (); + + subView.SuperViewChanging += (s, e) => + { + e.Handled = true; // Cancel the change + }; + + // Act + superView.Add (subView); + + // Assert - SuperView should not be set because the change was cancelled + Assert.Null (subView.SuperView); + Assert.Empty (superView.SubViews); + } + + [Fact] + public void SuperViewChanging_Can_Be_Cancelled_Via_Virtual_Method () + { + // Arrange + var superView = new View (); + var subView = new TestViewThatCancelsChange (); + + // Act + superView.Add (subView); + + // Assert - SuperView should not be set because the change was cancelled + Assert.Null (subView.SuperView); + Assert.Empty (superView.SubViews); + } + + [Fact] + public void SuperViewChanging_Virtual_Method_Cancellation_Prevents_Event () + { + // Arrange + var superView = new View (); + var subView = new TestViewThatCancelsChange (); + + var eventRaised = false; + subView.SuperViewChanging += (s, e) => + { + eventRaised = true; + }; + + // Act + superView.Add (subView); + + // Assert - Event should not be raised because virtual method cancelled first + Assert.False (eventRaised); + Assert.Null (subView.SuperView); + } + + [Fact] + public void SuperViewChanging_Cancellation_On_Remove () + { + // Arrange + var superView = new View (); + var subView = new View (); + + superView.Add (subView); + Assert.Equal (superView, subView.SuperView); + + subView.SuperViewChanging += (s, e) => + { + // Cancel removal if trying to set to null + if (e.NewValue is null) + { + e.Handled = true; + } + }; + + // Act + superView.Remove (subView); + + // Assert - SuperView should still be set because removal was cancelled + Assert.Equal (superView, subView.SuperView); + Assert.Single (superView.SubViews); + } + + [Fact] + public void SuperViewChanging_Cancellation_When_Changing_Between_SuperViews () + { + // Arrange + var superView1 = new View (); + var superView2 = new View (); + var subView = new View (); + + superView1.Add (subView); + + subView.SuperViewChanging += (s, e) => + { + // Cancel if trying to move to superView2 + if (e.NewValue == superView2) + { + e.Handled = true; + } + }; + + // Act + superView2.Add (subView); + + // Assert - Should still be in superView1 because change was cancelled + Assert.Equal (superView1, subView.SuperView); + Assert.Single (superView1.SubViews); + Assert.Empty (superView2.SubViews); + } + + // Helper class for testing cancellation + private class TestViewThatCancelsChange : View + { + protected override bool OnSuperViewChanging (ValueChangingEventArgs args) + { + return true; // Always cancel the change + } + } }