From 013482a7a7722140a544bd88c730764fb262a395 Mon Sep 17 00:00:00 2001 From: Tig Date: Mon, 1 Apr 2024 11:40:46 -0600 Subject: [PATCH 1/4] Updated docs --- docfx/docs/newinv2.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docfx/docs/newinv2.md b/docfx/docs/newinv2.md index b9d4ea9ef..43c4e0a4a 100644 --- a/docfx/docs/newinv2.md +++ b/docfx/docs/newinv2.md @@ -11,5 +11,11 @@ Check out this Discussion: https://github.com/gui-cs/Terminal.Gui/discussions/24 * *Modern File Dialog* - Terminal.Gui now supports a modern file dialog that includes icons (in TUI!) for files/folders, search, and a `TreeView``. See [FileDialog](https://gui-cs.github.io/Terminal.GuiV2Docs/docs/overview.html#filedialog) for details. * *Configuration Manager* - Terminal.Gui now supports a configuration manager enabling library and app settings to be persisted and loaded from the file system. See [Configuration Manager](https://gui-cs.github.io/Terminal.GuiV2Docs/docs/overview.html#configuration-manager) for details. * *Simplified API* - The entire library has been reviewed and simplified. As a result, the API is more consistent and uses modern .NET API standards (e.g. for events). This refactoring resulted in the removal of thousands of lines of code, better unit tests, and higher performance than v1. See [Simplified API](https://gui-cs.github.io/Terminal.GuiV2Docs/docs/overview.html#simplified-api) for details. +* *View Lifetime Management is Now Deterministic* - In v1 the rules ofr lifetime management of `View` objects was unclear and led to non-dterministic behavior and hard to diagnose bugs. This was particularly acute in the behavior of `Application.Run`. In v2, the rules are clear and the code and unit test infrastructure tries to enforce them. + * `View` and all subclasses support `IDisposable` and must be disposed (by calling `view.Dispose ()`) by whatever code owns the instance when the instance is longer needed. + * To simplify programming, any `View` added as a Subview another `View` will have it's lifecycle owned by the Superview; when a `View` is disposed, it will call `Dispose` on all the items in the `Subviews` property. Note this behavior is the same as it was in v1, just clarified. + * In v1, `Application.End` called `Dispose ()` on `Application.Top` (via `Runstate.Toplevel`). This was incorrect as it meant that after `Application.Run` returned, `Application.Top` had been disposed, and any code that wanted to interogate the results of `Run` by accessing `Application.Top` only worked by accident. This is because GC had not actually happened; if it had the application would have crashed. In v2 `Application.End` does NOT call `Dispose`, and it is the caller to `Application.Run` who is responsible for disposing the `Toplevel` that was either passed to `Application.Run (View)` or created by `Application.Run ()`. + * Any code that creates a `Toplevel`, either by using `top = new()` or by calling either `top = Application.Run ()` or `top = ApplicationRun()` must call `top.Dispose` when complete. + * The exception to this is if `top` is passed to `myView.Add(top)` making it a subview of `myView`. This is because the semantics of `Add` are that the `myView` takes over responsibility for the subviews lifetimes. Of course, if someone calls `myView.Remove(top)` to remove said subview, they then re-take responsbility for `top`'s lifetime and they must call `top.Dispose`. ... \ No newline at end of file From dfe746a10a0d66e2b79f37f46d56c5141ae83e5f Mon Sep 17 00:00:00 2001 From: Tig Date: Sun, 31 Mar 2024 21:30:08 -0600 Subject: [PATCH 2/4] Updated View.Add/Remove API docs --- Terminal.Gui/View/ViewSubViews.cs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Terminal.Gui/View/ViewSubViews.cs b/Terminal.Gui/View/ViewSubViews.cs index b9cacc711..794c3e1ed 100644 --- a/Terminal.Gui/View/ViewSubViews.cs +++ b/Terminal.Gui/View/ViewSubViews.cs @@ -185,7 +185,12 @@ public partial class View } /// Removes a subview added via or from this View. - /// + /// + /// + /// Normally Subviews will be disposed when this View is disposed. Removing a Subview causes ownership of the Subview's + /// lifecycle to be transferred to the caller; the caller must call . + /// + /// public virtual void Remove (View view) { if (view is null || _subviews is null) @@ -217,7 +222,15 @@ public partial class View } } - /// Removes all subviews (children) added via or from this View. + /// + /// Removes all subviews (children) added via or from this View. + /// + /// + /// + /// Normally Subviews will be disposed when this View is disposed. Removing a Subview causes ownership of the Subview's + /// lifecycle to be transferred to the caller; the caller must call on any Views that were added. + /// + /// public virtual void RemoveAll () { if (_subviews is null) From 327957a060e3a18c299fbcc130ebc717dfbb3403 Mon Sep 17 00:00:00 2001 From: Tig Date: Mon, 1 Apr 2024 11:22:27 -0600 Subject: [PATCH 3/4] Fixed some unit tests. Prepared for Issue #3368 --- UnitTests/TestHelpers.cs | 6 ++++++ UnitTests/View/Layout/DimTests.cs | 1 + UnitTests/View/ViewTests.cs | 3 +++ UnitTests/Views/ButtonTests.cs | 2 ++ 4 files changed, 12 insertions(+) diff --git a/UnitTests/TestHelpers.cs b/UnitTests/TestHelpers.cs index 5842b3a38..ee95c3706 100644 --- a/UnitTests/TestHelpers.cs +++ b/UnitTests/TestHelpers.cs @@ -122,6 +122,8 @@ public class TestRespondersDisposed : BeforeAfterTestAttribute public override void After (MethodInfo methodUnderTest) { Debug.WriteLine ($"After: {methodUnderTest.Name}"); + base.After (methodUnderTest); + #if DEBUG_IDISPOSABLE Assert.Empty (Responder.Instances); #endif @@ -130,6 +132,7 @@ public class TestRespondersDisposed : BeforeAfterTestAttribute public override void Before (MethodInfo methodUnderTest) { Debug.WriteLine ($"Before: {methodUnderTest.Name}"); + base.Before (methodUnderTest); #if DEBUG_IDISPOSABLE // Clear out any lingering Responder instances from previous tests @@ -139,6 +142,7 @@ public class TestRespondersDisposed : BeforeAfterTestAttribute } } +// TODO: Make this inherit from TestRespondersDisposed so that all tests that don't dispose Views correctly can be identified and fixed [AttributeUsage (AttributeTargets.Class | AttributeTargets.Method)] public class SetupFakeDriverAttribute : BeforeAfterTestAttribute { @@ -156,6 +160,7 @@ public class SetupFakeDriverAttribute : BeforeAfterTestAttribute View.Diagnostics = ViewDiagnosticFlags.Off; Application.Driver = null; + base.After (methodUnderTest); } public override void Before (MethodInfo methodUnderTest) @@ -163,6 +168,7 @@ public class SetupFakeDriverAttribute : BeforeAfterTestAttribute Debug.WriteLine ($"Before: {methodUnderTest.Name}"); Assert.Null (Application.Driver); Application.Driver = new FakeDriver { Rows = 25, Cols = 25 }; + base.Before (methodUnderTest); } } diff --git a/UnitTests/View/Layout/DimTests.cs b/UnitTests/View/Layout/DimTests.cs index 40bcb2ecf..b64d37439 100644 --- a/UnitTests/View/Layout/DimTests.cs +++ b/UnitTests/View/Layout/DimTests.cs @@ -68,6 +68,7 @@ public class DimTests top.Add (win); Application.Run (top); + top.Dispose (); Assert.Equal (20, count); } diff --git a/UnitTests/View/ViewTests.cs b/UnitTests/View/ViewTests.cs index de9de98c3..852899091 100644 --- a/UnitTests/View/ViewTests.cs +++ b/UnitTests/View/ViewTests.cs @@ -70,6 +70,7 @@ public class ViewTests "; pos = TestHelpers.AssertDriverContentsWithFrameAre (expected, _output); + top.Dispose (); } [Fact] @@ -133,6 +134,8 @@ public class ViewTests "; pos = TestHelpers.AssertDriverContentsWithFrameAre (expected, _output); + + top.Dispose (); } [Theory] diff --git a/UnitTests/Views/ButtonTests.cs b/UnitTests/Views/ButtonTests.cs index d64214832..e06afe7d7 100644 --- a/UnitTests/Views/ButtonTests.cs +++ b/UnitTests/Views/ButtonTests.cs @@ -172,6 +172,7 @@ public class ButtonTests "; TestHelpers.AssertDriverContentsWithFrameAre (expected, _output); + top.Dispose (); } [Fact] @@ -547,6 +548,7 @@ public class ButtonTests btn.HotKey = KeyCode.E; Assert.True (btn.NewKeyDownEvent (Key.E.WithAlt)); Assert.True (clicked); + top.Dispose (); } /// From 3b3906a7f18aef7eac088821856a84f8eb34d4d4 Mon Sep 17 00:00:00 2001 From: Tig Date: Mon, 1 Apr 2024 11:52:11 -0600 Subject: [PATCH 4/4] Updated Add/Remove API docs --- Terminal.Gui/View/View.cs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Terminal.Gui/View/View.cs b/Terminal.Gui/View/View.cs index 07235e638..e3b22cebe 100644 --- a/Terminal.Gui/View/View.cs +++ b/Terminal.Gui/View/View.cs @@ -514,9 +514,23 @@ public partial class View : Responder, ISupportInitializeNotification /// public override string ToString () { return $"{GetType ().Name}({Id}){Frame}"; } - /// + /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources. + /// + /// + /// Subviews added to this view via have their lifetime owned by this view and will + /// dispose them. To prevent this, and have the creator of the Subview instance handle disposal, use to remove + /// the subview first. + /// + /// + /// If disposing equals true, the method has been called directly or indirectly by a user's code. Managed and + /// unmanaged resources can be disposed. If disposing equals false, the method has been called by the runtime from + /// inside the finalizer, and you should not reference other objects. Only unmanaged resources can be disposed. + /// + /// + /// protected override void Dispose (bool disposing) { + // BUGBUG: We should only dispose these objects if disposing == true LineCanvas.Dispose (); DisposeAdornments ();