From c8890628e91311559949b22cab2aa5cefb8a8509 Mon Sep 17 00:00:00 2001 From: BDisp Date: Wed, 20 Mar 2024 17:34:20 +0000 Subject: [PATCH] Fixes #3338. Application.Run/End -> Callers must dispose Toplevel --- Terminal.Gui/Application.cs | 14 ++++++-- UnitTests/Application/ApplicationTests.cs | 44 +++++++++++++++++++++-- UnitTests/Dialogs/DialogTests.cs | 33 +++++++++++++++++ UnitTests/Views/OverlappedTests.cs | 32 ++++++++++++----- 4 files changed, 111 insertions(+), 12 deletions(-) diff --git a/Terminal.Gui/Application.cs b/Terminal.Gui/Application.cs index 929c13fa8..8297b9e9c 100644 --- a/Terminal.Gui/Application.cs +++ b/Terminal.Gui/Application.cs @@ -294,6 +294,7 @@ public static partial class Application Top = topLevelFactory (); Current = Top; + _initialTop = Top; // Ensure Top's layout is up to date. Current.SetRelativeLayout (Driver.Bounds); @@ -345,6 +346,8 @@ public static partial class Application #region Run (Begin, Run, End, Stop) + private static Toplevel _initialTop; + /// /// Notify that a new was created ( was called). The token is /// created in and this event will be fired before that function exits. @@ -1040,10 +1043,17 @@ public static partial class Application Refresh (); } - // Do NOT dispose .Toplevel here. It was not created by - // Run, but Init or someone else. + // Always dispose runState.Toplevel here. If it is not the same as + // the current in the RunIteration, it will be fixed later in the + // next RunIteration. + runState.Toplevel?.Dispose (); runState.Toplevel = null; runState.Dispose (); + + if (_topLevels.Count == 0) + { + Top = _initialTop; + } } #endregion Run (Begin, Run, End) diff --git a/UnitTests/Application/ApplicationTests.cs b/UnitTests/Application/ApplicationTests.cs index 9105fe7bb..c67166e22 100644 --- a/UnitTests/Application/ApplicationTests.cs +++ b/UnitTests/Application/ApplicationTests.cs @@ -52,11 +52,12 @@ public class ApplicationTests Init (); RunState rs = Application.Begin (Application.Top); + Assert.Equal (rs.Toplevel, Application.Top); Application.End (rs); #if DEBUG_IDISPOSABLE Assert.True (rs.WasDisposed); - Assert.False (Application.Top.WasDisposed); + Assert.True (Application.Top.WasDisposed); // Is true because the rs.Toplevel is the same as Application.Top #endif Assert.Null (rs.Toplevel); @@ -365,6 +366,8 @@ public class ApplicationTests [AutoInitShutdown] public void SetCurrentAsTop_Run_A_Not_Modal_Toplevel_Make_It_The_Current_Application_Top () { + var top = Application.Top; + var t1 = new Toplevel (); var t2 = new Toplevel (); var t3 = new Toplevel (); @@ -455,7 +458,11 @@ public class ApplicationTests Application.Run (t1); - Assert.Equal (t1, Application.Top); + Assert.NotEqual (t1, Application.Top); + Assert.Equal (top, Application.Top); +#if DEBUG_IDISPOSABLE + Assert.True (Application.Top.WasDisposed); +#endif } private void Init () @@ -876,6 +883,39 @@ public class ApplicationTests Application.Shutdown (); } + [Fact] + public void End_Disposing_Correctly () + { + Init (); + + var top = Application.Top; + + Window w = new (); + w.Ready += (s, e) => Application.RequestStop (); // Causes `End` to be called + Application.Run(w); + +#if DEBUG_IDISPOSABLE + Assert.True (w.WasDisposed); +#endif + + Assert.NotNull (w); + Assert.Equal (string.Empty, w.Title); // Invalid - w has been disposed -> Valid - w isn't Application.Top but the original created by Init + Assert.NotNull (Application.Top); + Assert.NotEqual(w, Application.Top); + Assert.Equal(top, Application.Top); + Assert.Null (Application.Current); + + var exception = Record.Exception (() => Application.Run(w)); // Invalid - w has been disposed. Run it in debug mode will throw, otherwise the user may want to run it again + Assert.NotNull (exception); + + Application.Shutdown (); + Assert.NotNull (w); + Assert.Equal (string.Empty, w.Title); // Invalid - w has been disposed -> Valid - w isn't Application.Top but the original created by Init + Assert.Null (Application.Top); + Assert.Null (Application.Current); + Assert.NotNull (top); + } + // TODO: Add tests for Run that test errorHandler #endregion diff --git a/UnitTests/Dialogs/DialogTests.cs b/UnitTests/Dialogs/DialogTests.cs index 2008b16ce..a4ee55997 100644 --- a/UnitTests/Dialogs/DialogTests.cs +++ b/UnitTests/Dialogs/DialogTests.cs @@ -1331,4 +1331,37 @@ public class DialogTests return (Begin (dlg), dlg); } + + [Fact] + [AutoInitShutdown] + public void Run_Dispose_Dialog () + { + var top = Top; + + Dialog dlg = new (); + + dlg.Ready += Dlg_Ready; + + Run (dlg); + +#if DEBUG_IDISPOSABLE + Assert.True (dlg.WasDisposed); + Assert.True (Top.WasDisposed); + Assert.Equal (top, Top); +#endif + + // This is instance and the user is responsible to set to null or not + // because in the Application it's all working as expected. + // Fortunately, this instance is not null so that we can obtain the value of its properties + Assert.NotNull (dlg); + + Shutdown(); + + return; + + void Dlg_Ready (object sender, EventArgs e) + { + RequestStop (); + } + } } diff --git a/UnitTests/Views/OverlappedTests.cs b/UnitTests/Views/OverlappedTests.cs index 41270d558..3f3d1a924 100644 --- a/UnitTests/Views/OverlappedTests.cs +++ b/UnitTests/Views/OverlappedTests.cs @@ -97,7 +97,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact] @@ -332,7 +334,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact] @@ -421,7 +425,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact] @@ -504,7 +510,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact] @@ -597,7 +605,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact] @@ -689,7 +699,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact] @@ -766,7 +778,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact] @@ -842,7 +856,9 @@ public class OverlappedTests Application.Run (overlapped); - Assert.Empty (Application.OverlappedChildren); + Assert.Null (Application.OverlappedChildren); + Assert.Null (Application.OverlappedTop); + Assert.NotNull (Application.Top); } [Fact]