Fix issue where the TestViewsDisposeCorrectly was not doing what it was supposed to do (#2964)

* add Disposal Test and fix an ssue where the CopyClipboard test was failing

* Update ViewDisposalTest.cs

* Update ViewDisposalTest.cs: Some Formatting, and adding code comments.

* Fix ViewDisposalTests (Wasn't working the way it was supposed to)

* update test

* update test

* update test

* try to fix as many conflicts as possible

* make test output prettier

* fix formatting

* Fix Subviews not being empty after disposing on all views.

* The fail cause was Application.Top not being disposed.

* Fix others containers that weren't being removed.

* Revert "The fail cause was Application.Top not being disposed."

This reverts commit 0c2183ed9ed11d0b35e1404497fea08a528d4f01.

* Application.Top isn't null and need disposing.

* Fixes #2985. Application.RunState must be responsible for dispose the Toplevel property.

* Change the unit test with ans without Application.Shutdown method.

* Update ViewDisposeTests to actually check wether ALL views have been disposed (not just container)

* small additional check just to be safe

* Update ViewDisposalTest.cs: Formatting

* Update ViewDisposalTest.cs: Minor change to re-trigger Action

TestVKPacket is acting up again. Maybe the test is running async and is receiving scan codes from other instances?

---------

Co-authored-by: John Züchler <[email protected]>
Co-authored-by: BDisp <[email protected]>
Co-authored-by: Tig <[email protected]>

Terminal.Gui/Core/Application.cs

@@ -86,7 +86,7 @@ namespace Terminal.Gui {
 		/// </summary>
 		public static Toplevel MdiTop {
 			get {
-				if (Top.IsMdiContainer) {
+				if (Top?.IsMdiContainer == true) {
 					return Top;
 				}
 				return null;
@@ -516,11 +516,8 @@ namespace Terminal.Gui {
 			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 @@ namespace Terminal.Gui {
 			// 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 @@ namespace Terminal.Gui {
 				Refresh ();
 			}
 
-			runState.Toplevel?.Dispose ();
-			runState.Toplevel = null;
 			runState.Dispose ();
 		}
 

Terminal.Gui/Core/Border.cs

@@ -234,7 +234,11 @@ namespace Terminal.Gui {
 
 				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;

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 @@ namespace Terminal.Gui {
 				subview.Dispose ();
 			}
 			base.Dispose (disposing);
+			System.Diagnostics.Debug.Assert (InternalSubviews.Count == 0);
 		}
 
 		/// <summary>

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 @@ namespace Terminal.Gui {
 			}
 
 			SetNeedsDisplay ();
-			contentView.Remove (view);
+			if (view == contentView) {
+				base.Remove (view);
+			} else {
+				contentView.Remove (view);
+			}
 
 			RemoveMenuStatusBar (view);
 			if (view != contentView && Focused == null) {

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 @@ namespace Terminal.Gui {
 
 			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;

Terminal.Gui/Views/TextView.cs

@@ -1441,8 +1441,10 @@ namespace Terminal.Gui {
 		{
 			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 ();
 		}
 

Terminal.Gui/Windows/Wizard.cs

@@ -316,7 +316,11 @@ namespace Terminal.Gui {
 
 				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;

UICatalog/Scenarios/SingleBackgroundWorker.cs

@@ -15,7 +15,7 @@ namespace UICatalog.Scenarios {
 
 			Application.Run<MainApp> ();
 
-			Application.Top.Dispose ();
+			System.Diagnostics.Debug.Assert (Application.Top == null);
 		}
 
 		public class MainApp : Toplevel {

UnitTests/Application/ApplicationTests.cs

@@ -159,13 +159,12 @@ namespace Terminal.Gui.ApplicationTests {
 			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 @@ namespace Terminal.Gui.ApplicationTests {
 			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 @@ namespace Terminal.Gui.ApplicationTests {
 
 			Application.Run (t1);
 
-			Assert.Equal (t1, Application.Top);
+			Assert.Null (Application.Top);
+#if DEBUG_IDISPOSABLE
+			Assert.True (t1.WasDisposed);
+#endif
 		}
 
 		[Fact]

UnitTests/Application/RunStateTests.cs

@@ -48,12 +48,11 @@ namespace Terminal.Gui.ApplicationTests {
 			rs = new Application.RunState (top);
 			Assert.NotNull (rs);
 
-			// Should throw because Toplevel was not cleaned up
-			Assert.Throws<InvalidOperationException> (() => 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 @@ namespace Terminal.Gui.ApplicationTests {
 		void Init ()
 		{
 			Application.Init (new FakeDriver ());
-			
+
 			Assert.NotNull (Application.Driver);
 			Assert.NotNull (Application.MainLoop);
 			Assert.NotNull (SynchronizationContext.Current);
@@ -74,7 +73,7 @@ namespace Terminal.Gui.ApplicationTests {
 			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 @@ namespace Terminal.Gui.ApplicationTests {
 			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 @@ namespace Terminal.Gui.ApplicationTests {
 			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);
 		}

UnitTests/Views/ButtonTests.cs

@@ -16,8 +16,9 @@ namespace Terminal.Gui.ViewTests {
 		{
 			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 @@ namespace Terminal.Gui.ViewTests {
 			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 @@ namespace Terminal.Gui.ViewTests {
 			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);

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 @@ namespace UnitTests.ViewsTests {
 			}
 		}
 
-		[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 @@ namespace UnitTests.ViewsTests {
 			//special_params.Add (typeof (LineView), new object [] { Orientation.Horizontal });
 		}
 
-		WeakReference DoTest ()
+		List<WeakReference> DoTest (bool callShutdown)
 		{
+			var driver = new FakeDriver ();
+			Application.Init (driver, new FakeMainLoop (driver));
 			getSpecialParams ();
 			View Container = new View ();
-			Toplevel top = Application.Top;
+			List<WeakReference> refs = new List<WeakReference> { 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 @@ namespace UnitTests.ViewsTests {
 
 				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 @@ namespace UnitTests.ViewsTests {
 			}
 
 			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;
 		}
 
 		/// <summary>