diff --git a/Terminal.Gui/Core/Application.cs b/Terminal.Gui/Core/Application.cs index d01b338a29..b6aac9d2ac 100644 --- a/Terminal.Gui/Core/Application.cs +++ b/Terminal.Gui/Core/Application.cs @@ -86,7 +86,7 @@ public static List MdiChildes { /// public static Toplevel MdiTop { get { - if (Top.IsMdiContainer) { + if (Top?.IsMdiContainer == true) { return Top; } return null; @@ -516,11 +516,8 @@ public void Dispose () protected virtual void Dispose (bool disposing) { if (Toplevel != null && disposing) { - throw new InvalidOperationException ("You must clean up (Dispose) the Toplevel before calling Application.RunState.Dispose"); - // BUGBUG: It's insidious that we call EndFirstTopLevel here so I moved it to End. - //EndFirstTopLevel (Toplevel); - //Toplevel.Dispose (); - //Toplevel = null; + Toplevel.Dispose (); + Toplevel = null; } } } @@ -1062,6 +1059,7 @@ public static void End (RunState runState) // Set Current and Top to the next TopLevel on the stack if (toplevels.Count == 0) { Current = null; + Top = null; } else { Current = toplevels.Peek (); if (toplevels.Count == 1 && Current == MdiTop) { @@ -1074,8 +1072,6 @@ public static void End (RunState runState) Refresh (); } - runState.Toplevel?.Dispose (); - runState.Toplevel = null; runState.Dispose (); } diff --git a/Terminal.Gui/Core/Border.cs b/Terminal.Gui/Core/Border.cs index 0ff2da0e61..dcc8697819 100644 --- a/Terminal.Gui/Core/Border.cs +++ b/Terminal.Gui/Core/Border.cs @@ -234,7 +234,11 @@ public override void Remove (View view) SetNeedsDisplay (); var touched = view.Frame; - Border.Child.Remove (view); + if (view == Border.Child) { + base.Remove (view); + } else { + Border.Child.Remove (view); + } if (Border.Child.InternalSubviews.Count < 1) { CanFocus = false; diff --git a/Terminal.Gui/Core/View.cs b/Terminal.Gui/Core/View.cs index 2d34647a08..0d4a398180 100644 --- a/Terminal.Gui/Core/View.cs +++ b/Terminal.Gui/Core/View.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using System.ComponentModel; using System.Linq; -using System.Reflection; using NStack; namespace Terminal.Gui { @@ -2944,6 +2943,7 @@ protected override void Dispose (bool disposing) subview.Dispose (); } base.Dispose (disposing); + System.Diagnostics.Debug.Assert (InternalSubviews.Count == 0); } /// diff --git a/Terminal.Gui/Core/Window.cs b/Terminal.Gui/Core/Window.cs index 129a10c462..8fbe05642b 100644 --- a/Terminal.Gui/Core/Window.cs +++ b/Terminal.Gui/Core/Window.cs @@ -10,7 +10,6 @@ // Any udpates done here should probably be done in FrameView as well; TODO: Merge these classes using System; -using System.Collections; using System.Linq; using NStack; @@ -271,7 +270,11 @@ public override void Remove (View view) } SetNeedsDisplay (); - contentView.Remove (view); + if (view == contentView) { + base.Remove (view); + } else { + contentView.Remove (view); + } RemoveMenuStatusBar (view); if (view != contentView && Focused == null) { diff --git a/Terminal.Gui/Views/FrameView.cs b/Terminal.Gui/Views/FrameView.cs index c6ca31e63a..6fec8bf091 100644 --- a/Terminal.Gui/Views/FrameView.cs +++ b/Terminal.Gui/Views/FrameView.cs @@ -9,7 +9,6 @@ // - Does not support IEnumerable // Any udpates done here should probably be done in Window as well; TODO: Merge these classes -using System; using System.Linq; using NStack; @@ -202,7 +201,11 @@ public override void Remove (View view) SetNeedsDisplay (); var touched = view.Frame; - contentView.Remove (view); + if (view == contentView) { + base.Remove (view); + } else { + contentView.Remove (view); + } if (contentView.InternalSubviews.Count < 1) this.CanFocus = false; diff --git a/Terminal.Gui/Views/TextView.cs b/Terminal.Gui/Views/TextView.cs index 5493ff13bf..38f650801e 100644 --- a/Terminal.Gui/Views/TextView.cs +++ b/Terminal.Gui/Views/TextView.cs @@ -1441,8 +1441,10 @@ void TextView_Initialized (object sender, EventArgs e) { Autocomplete.HostControl = this; - Application.Top.AlternateForwardKeyChanged += Top_AlternateForwardKeyChanged; - Application.Top.AlternateBackwardKeyChanged += Top_AlternateBackwardKeyChanged; + if (Application.Top != null) { + Application.Top.AlternateForwardKeyChanged += Top_AlternateForwardKeyChanged; + Application.Top.AlternateBackwardKeyChanged += Top_AlternateBackwardKeyChanged; + } OnContentsChanged (); } diff --git a/Terminal.Gui/Windows/Wizard.cs b/Terminal.Gui/Windows/Wizard.cs index f288b5f669..d63ae274d7 100644 --- a/Terminal.Gui/Windows/Wizard.cs +++ b/Terminal.Gui/Windows/Wizard.cs @@ -316,7 +316,11 @@ public override void Remove (View view) SetNeedsDisplay (); var touched = view.Frame; - contentView.Remove (view); + if (view == contentView || view.GetType().Name == "ContentView") { + base.Remove (view); + } else { + contentView.Remove (view); + } if (contentView.InternalSubviews.Count < 1) this.CanFocus = false; diff --git a/UICatalog/Scenarios/SingleBackgroundWorker.cs b/UICatalog/Scenarios/SingleBackgroundWorker.cs index e0268ab419..460880b24e 100644 --- a/UICatalog/Scenarios/SingleBackgroundWorker.cs +++ b/UICatalog/Scenarios/SingleBackgroundWorker.cs @@ -15,7 +15,7 @@ public override void Run () Application.Run (); - Application.Top.Dispose (); + System.Diagnostics.Debug.Assert (Application.Top == null); } public class MainApp : Toplevel { diff --git a/UnitTests/Application/ApplicationTests.cs b/UnitTests/Application/ApplicationTests.cs index d36a1c8cd0..4204abf7c6 100644 --- a/UnitTests/Application/ApplicationTests.cs +++ b/UnitTests/Application/ApplicationTests.cs @@ -159,13 +159,12 @@ public void Init_Begin_End_Cleans_Up () Application.End (runstate); Assert.Null (Application.Current); - Assert.NotNull (Application.Top); + Assert.Null (Application.Top); Assert.NotNull (Application.MainLoop); Assert.NotNull (Application.Driver); Shutdown (); - Assert.Null (Application.Top); Assert.Null (Application.MainLoop); Assert.Null (Application.Driver); } @@ -203,13 +202,12 @@ public void InitWithTopLevelFactory_Begin_End_Cleans_Up () Application.End (runstate); Assert.Null (Application.Current); - Assert.NotNull (Application.Top); + Assert.Null (Application.Top); Assert.NotNull (Application.MainLoop); Assert.NotNull (Application.Driver); Shutdown (); - Assert.Null (Application.Top); Assert.Null (Application.MainLoop); Assert.Null (Application.Driver); } @@ -529,7 +527,10 @@ public void SetCurrentAsTop_Run_A_Not_Modal_Toplevel_Make_It_The_Current_Applica Application.Run (t1); - Assert.Equal (t1, Application.Top); + Assert.Null (Application.Top); +#if DEBUG_IDISPOSABLE + Assert.True (t1.WasDisposed); +#endif } [Fact] diff --git a/UnitTests/Application/RunStateTests.cs b/UnitTests/Application/RunStateTests.cs index 500af598a8..4b28fc6c7a 100644 --- a/UnitTests/Application/RunStateTests.cs +++ b/UnitTests/Application/RunStateTests.cs @@ -48,12 +48,11 @@ public void Dispose_Cleans_Up_RunState () rs = new Application.RunState (top); Assert.NotNull (rs); - // Should throw because Toplevel was not cleaned up - Assert.Throws (() => rs.Dispose ()); + // Should not throw because Toplevel was cleaned up + var exception = Record.Exception (() => rs.Dispose ()); + Assert.Null (exception); - rs.Toplevel.Dispose (); - rs.Toplevel = null; - rs.Dispose (); + Assert.Null (rs.Toplevel); #if DEBUG_IDISPOSABLE Assert.True (rs.WasDisposed); Assert.True (top.WasDisposed); @@ -63,7 +62,7 @@ public void Dispose_Cleans_Up_RunState () void Init () { Application.Init (new FakeDriver ()); - + Assert.NotNull (Application.Driver); Assert.NotNull (Application.MainLoop); Assert.NotNull (SynchronizationContext.Current); @@ -74,7 +73,7 @@ void Shutdown () Application.Shutdown (); #if DEBUG_IDISPOSABLE // Validate there are no outstanding RunState-based instances left - foreach (var inst in Application.RunState.Instances) Assert.True (inst.WasDisposed); + foreach (var inst in Application.RunState.Instances) Assert.True (inst.WasDisposed); #endif } @@ -94,7 +93,7 @@ public void Begin_End_Cleans_Up_RunState () Application.End (rs); Assert.Null (Application.Current); - Assert.NotNull (Application.Top); + Assert.Null (Application.Top); Assert.NotNull (Application.MainLoop); Assert.NotNull (Application.Driver); @@ -104,7 +103,46 @@ public void Begin_End_Cleans_Up_RunState () Assert.True (rs.WasDisposed); #endif + Assert.Null (Application.MainLoop); + Assert.Null (Application.Driver); + } + + WeakReference CreateToplevelInstance () + { + // Setup Mock driver + Init (); + + var top = new Toplevel (); + var rs = Application.Begin (top); + + Assert.NotNull (rs); + Assert.Equal (top, Application.Current); + Assert.Equal (top, Application.Top); + Application.End (rs); +#if DEBUG_IDISPOSABLE + Assert.True (rs.WasDisposed); + Assert.True (top.WasDisposed); +#endif + Assert.Null (Application.Current); Assert.Null (Application.Top); + Assert.NotNull (top); + Assert.NotNull (Application.MainLoop); + Assert.NotNull (Application.Driver); + + return new WeakReference (top, true); + } + + [Fact] + public void Begin_End_Cleans_Up_RunState_Without_Shutdown () + { + WeakReference wrInstance = CreateToplevelInstance (); + + GC.Collect (); + GC.WaitForPendingFinalizers (); + Assert.False (wrInstance.IsAlive); + + // Shutdown Mock driver + Shutdown (); Assert.Null (Application.MainLoop); Assert.Null (Application.Driver); } diff --git a/UnitTests/Views/ButtonTests.cs b/UnitTests/Views/ButtonTests.cs index 01fd330f26..0202db0e3b 100644 --- a/UnitTests/Views/ButtonTests.cs +++ b/UnitTests/Views/ButtonTests.cs @@ -16,8 +16,9 @@ public void Constructors_Defaults () { var btn = new Button (); Assert.Equal (string.Empty, btn.Text); - Application.Top.Add (btn); - var rs = Application.Begin (Application.Top); + var top = Application.Top; + top.Add (btn); + var rs = Application.Begin (top); Assert.Equal ("[ ]", btn.TextFormatter.Text); Assert.False (btn.IsDefault); @@ -34,8 +35,8 @@ [ ] Application.End (rs); btn = new Button ("ARGS", true) { Text = "Test" }; Assert.Equal ("Test", btn.Text); - Application.Top.Add (btn); - rs = Application.Begin (Application.Top); + top.Add (btn); + rs = Application.Begin (top); Assert.Equal ("[◦ Test ◦]", btn.TextFormatter.Text); Assert.True (btn.IsDefault); @@ -52,8 +53,8 @@ [ ] Application.End (rs); btn = new Button (3, 4, "Test", true); Assert.Equal ("Test", btn.Text); - Application.Top.Add (btn); - rs = Application.Begin (Application.Top); + top.Add (btn); + rs = Application.Begin (top); Assert.Equal ("[◦ Test ◦]", btn.TextFormatter.Text); Assert.True (btn.IsDefault); diff --git a/UnitTests/Views/ViewDisposalTest.cs b/UnitTests/Views/ViewDisposalTest.cs index 3ae3df6243..177179bab4 100644 --- a/UnitTests/Views/ViewDisposalTest.cs +++ b/UnitTests/Views/ViewDisposalTest.cs @@ -1,3 +1,4 @@ +using SixLabors.ImageSharp.Processing.Processors.Quantization; using System; using System.Collections.Generic; using System.Linq; @@ -23,21 +24,38 @@ public ViewDisposalTest (ITestOutputHelper output) } } - [Fact] - [AutoInitShutdown] - public void TestViewsDisposeCorrectly () + [Theory] + [InlineData (true)] + [InlineData (false)] + public void TestViewsDisposeCorrectly (bool callShutdown) { - var reference = DoTest (); - for (var i = 0; i < 10 && reference.IsAlive; i++) { + var refs = DoTest (callShutdown); + //var reference = refs [0]; + for (var i = 0; i < 10 && refs [0].IsAlive; i++) { GC.Collect (); GC.WaitForPendingFinalizers (); } + foreach (var reference in refs) { + if (reference.IsAlive) { #if DEBUG_IDISPOSABLE - if (reference.IsAlive) { - Assert.True (((View)reference.Target).WasDisposed); - Assert.Fail ($"Some Views didnt get Garbage Collected: {((View)reference.Target).Subviews}"); - } + Assert.True (((View)reference.Target).WasDisposed); #endif + string alive = ""; // Instead of just checking the subviews of the container, we now iterate through a list + foreach (var r in refs) { // of Weakreferences Referencing every View that was tested. This makes more sense because + if (r.IsAlive) { // View.Dispose removes all of its subviews, wich is why View.Subviews is always empty + if (r == refs [0]) { // after View.Dispose has run. Luckily I didnt discover any more bugs or this wouldv'e + alive += "\n View (Container)"; // been a little bit annoying to find an answer for. Thanks to BDisp for listening to + } // me and giving his best to help me fix this thing. If you take a look at the commit log + alive += ",\n--"; // you will find that he did most of the work. -a-usr + alive += r.Target.GetType ().Name; + } // NOTE: DELETE BEFORE NEXT COMMIT + } + Assert.Fail ($"Some Views didnt get Garbage Collected: {alive}"); + } + } + if (!callShutdown) { + Application.Shutdown (); + } } void getSpecialParams () @@ -46,11 +64,16 @@ void getSpecialParams () //special_params.Add (typeof (LineView), new object [] { Orientation.Horizontal }); } - WeakReference DoTest () + List DoTest (bool callShutdown) { + var driver = new FakeDriver (); + Application.Init (driver, new FakeMainLoop (driver)); getSpecialParams (); View Container = new View (); - Toplevel top = Application.Top; + List refs = new List { new WeakReference (Container, true) }; + Container.Add (new View ()); + Toplevel top = new (); + var state = Application.Begin (top); var views = GetViews (); foreach (var view in views) { View instance; @@ -63,6 +86,8 @@ WeakReference DoTest () Assert.NotNull (instance); Container.Add (instance); + + refs.Add (new WeakReference (instance, true)); output.WriteLine ($"Added instance of {view}!"); } top.Add (Container); @@ -72,9 +97,22 @@ WeakReference DoTest () } top.Remove (Container); - WeakReference reference = new (Container, true); + Application.End (state); + Assert.True (refs.All (r => r.IsAlive)); +#if DEBUG_IDISPOSABLE + Assert.True (top.WasDisposed); + Assert.False (Container.WasDisposed); +#endif + Assert.Null (Application.Top); Container.Dispose (); - return reference; +#if DEBUG_IDISPOSABLE + Assert.True (Container.WasDisposed); +#endif + if (callShutdown) { + Application.Shutdown (); + } + + return refs; } ///