Merge pull request #3330 from tig/v2_3313-Application-End-Top

Fixes #3313. `Application.End` leaves `Top` invalid

Terminal.Gui/Application.cs

@@ -383,6 +383,10 @@ public static partial class Application
             throw new ArgumentNullException (nameof (Toplevel));
         }
 
+#if DEBUG_IDISPOSABLE
+        Debug.Assert (!Toplevel.WasDisposed);
+#endif
+
         if (Toplevel.IsOverlappedContainer && OverlappedTop != Toplevel && OverlappedTop is { })
         {
             throw new InvalidOperationException ("Only one Overlapped Container is allowed.");
@@ -539,9 +543,17 @@ public static partial class Application
     {
         if (_initialized)
         {
+            // Init created Application.Top. If it hasn't been disposed...
+            if (Top is { })
+            {
+                Top.Dispose ();
+                Top = null;
+            }
+
             if (Driver is { })
             {
                 // Init() has been called and we have a driver, so just run the app.
+                // This Toplevel will get disposed in `Shutdown`
                 var top = new T ();
                 Type type = top.GetType ().BaseType;
 
@@ -1028,11 +1040,10 @@ public static partial class Application
             Refresh ();
         }
 
-        runState.Toplevel?.Dispose ();
+        // Do NOT dispose .Toplevel here. It was not created by
+        // Run, but Init or someone else.
         runState.Toplevel = null;
         runState.Dispose ();
-
-        // BUGBUG: Application.Top is now invalid?!?!
     }
 
     #endregion Run (Begin, Run, End)

Terminal.Gui/RunState.cs

@@ -32,8 +32,11 @@ public class RunState : IDisposable
     {
         if (Toplevel is { } && disposing)
         {
+            // Previously we were requiring Toplevel be disposed here.
+            // But that is not correct becaue `Begin` didn't create the TopLevel, `Init` did; thus
+            // disposing should be done by `Shutdown`, not `End`.
             throw new InvalidOperationException (
-                                                 "You must clean up (Dispose) the Toplevel before calling Application.RunState.Dispose"
+                                                 "Toplevel must be null before calling Application.RunState.Dispose"
                                                 );
         }
     }

UnitTests/Application/ApplicationTests.cs

@@ -46,6 +46,32 @@ public class ApplicationTests
         Assert.Equal (new Rectangle (0, 0, 5, 5), Application.Top.Frame);
     }
 
+    [Fact]
+    public void End_Should_Not_Dispose_ApplicationTop_Shutdown_Should ()
+    {
+        Init ();
+
+        RunState rs = Application.Begin (Application.Top);
+        Application.End (rs);
+
+#if DEBUG_IDISPOSABLE
+        Assert.True (rs.WasDisposed);
+        Assert.False (Application.Top.WasDisposed);
+#endif
+
+        Assert.Null (rs.Toplevel);
+
+        var top = Application.Top;
+
+        Shutdown ();
+
+#if DEBUG_IDISPOSABLE
+        Assert.True (top.WasDisposed);
+#endif
+
+        Assert.Null (Application.Top);
+    }
+
     [Fact]
     public void Init_Begin_End_Cleans_Up ()
     {
@@ -470,12 +496,13 @@ public class ApplicationTests
     #region RunTests
 
     [Fact]
+
     public void Run_T_After_InitWithDriver_with_TopLevel_Throws ()
     {
         // Setup Mock driver
         Init ();
 
-        // Run<Toplevel> when already initialized with a Driver will throw (because Toplevel is not dervied from TopLevel)
+        // Run<Toplevel> when already initialized with a Driver will throw (because Toplevel is not derived from TopLevel)
         Assert.Throws<ArgumentException> (() => Application.Run<Toplevel> ());
 
         Shutdown ();
@@ -486,12 +513,13 @@ public class ApplicationTests
     }
 
     [Fact]
+
     public void Run_T_After_InitWithDriver_with_TopLevel_and_Driver_Throws ()
     {
         // Setup Mock driver
         Init ();
 
-        // Run<Toplevel> when already initialized with a Driver will throw (because Toplevel is not dervied from TopLevel)
+        // Run<Toplevel> when already initialized with a Driver will throw (because Toplevel is not derivied from TopLevel)
         Assert.Throws<ArgumentException> (() => Application.Run<Toplevel> (null, new FakeDriver ()));
 
         Shutdown ();
@@ -502,6 +530,40 @@ public class ApplicationTests
     }
 
     [Fact]
+    [TestRespondersDisposed]
+
+    public void Run_T_After_Init_Disposes_Application_Top ()
+    {
+        Init ();
+
+        // Init created a Toplevel and assigned it to Application.Top
+        var initTop = Application.Top;
+
+        Application.Iteration += (s, a) =>
+                                 {
+                                     Assert.NotEqual(initTop, Application.Top);
+#if DEBUG_IDISPOSABLE
+                                     Assert.True (initTop.WasDisposed);
+#endif
+                                     Application.RequestStop ();
+                                 };
+
+        Application.Run<TestToplevel> ();
+
+#if DEBUG_IDISPOSABLE
+        Assert.True (initTop.WasDisposed);
+#endif
+
+        Shutdown ();
+
+        Assert.Null (Application.Top);
+        Assert.Null (Application.MainLoop);
+        Assert.Null (Application.Driver);
+    }
+
+    [Fact]
+    [TestRespondersDisposed]
+
     public void Run_T_After_InitWithDriver_with_TestTopLevel_DoesNotThrow ()
     {
         // Setup Mock driver
@@ -520,7 +582,9 @@ public class ApplicationTests
     }
 
     [Fact]
-    public void Run_T_After_InitNullDriver_with_TestTopLevel_Throws ()
+    [TestRespondersDisposed]
+
+    public void Run_T_After_InitNullDriver_with_TestTopLevel_DoesNotThrow ()
     {
         Application.ForceDriver = "FakeDriver";
 
@@ -529,7 +593,7 @@ public class ApplicationTests
 
         Application.Iteration += (s, a) => { Application.RequestStop (); };
 
-        // Init has been called without selecting a driver and we're passing no driver to Run<TestTopLevel>. Bad
+        // Init has been called, selecting FakeDriver; we're passing no driver to Run<TestTopLevel>. Should be fine.
         Application.Run<TestToplevel> ();
 
         Shutdown ();
@@ -540,6 +604,8 @@ public class ApplicationTests
     }
 
     [Fact]
+    [TestRespondersDisposed]
+
     public void Run_T_Init_Driver_Cleared_with_TestTopLevel_Throws ()
     {
         Init ();
@@ -559,6 +625,7 @@ public class ApplicationTests
     }
 
     [Fact]
+    [TestRespondersDisposed]
     public void Run_T_NoInit_DoesNotThrow ()
     {
         Application.ForceDriver = "FakeDriver";
@@ -576,6 +643,8 @@ public class ApplicationTests
     }
 
     [Fact]
+    [TestRespondersDisposed]
+
     public void Run_T_NoInit_WithDriver_DoesNotThrow ()
     {
         Application.Iteration += (s, a) => { Application.RequestStop (); };
@@ -591,6 +660,8 @@ public class ApplicationTests
     }
 
     [Fact]
+    [TestRespondersDisposed]
+
     public void Run_RequestStop_Stops ()
     {
         // Setup Mock driver
@@ -613,6 +684,8 @@ public class ApplicationTests
     }
 
     [Fact]
+    [TestRespondersDisposed]
+
     public void Run_RunningFalse_Stops ()
     {
         // Setup Mock driver
@@ -635,6 +708,8 @@ public class ApplicationTests
     }
 
     [Fact]
+    [TestRespondersDisposed]
+
     public void Run_Loaded_Ready_Unlodaded_Events ()
     {
         Init ();
@@ -678,7 +753,7 @@ public class ApplicationTests
                                          Application.OnMouseEvent (
                                                                    new MouseEventEventArgs (
                                                                                             new MouseEvent
-                                                                                                { X = 0, Y = 0, Flags = MouseFlags.ReportMousePosition }
+                                                                                            { X = 0, Y = 0, Flags = MouseFlags.ReportMousePosition }
                                                                                            )
                                                                   );
                                          Assert.False (top.NeedsDisplay);

UnitTests/Views/LabelTests.cs

@@ -201,52 +201,15 @@ public class LabelTests
     }
 
     [Fact]
-    [AutoInitShutdown]
     public void Constructors_Defaults ()
     {
         var label = new Label ();
         Assert.Equal (string.Empty, label.Text);
-        Application.Top.Add (label);
-        RunState rs = Application.Begin (Application.Top);
-
         Assert.Equal (TextAlignment.Left, label.TextAlignment);
         Assert.True (label.AutoSize);
         Assert.False (label.CanFocus);
         Assert.Equal (new Rectangle (0, 0, 0, 0), label.Frame);
         Assert.Equal (KeyCode.Null, label.HotKey);
-        var expected = @"";
-        TestHelpers.AssertDriverContentsWithFrameAre (expected, _output);
-        Application.End (rs);
-
-        label = new Label { Text = "Test" };
-        Assert.True (label.AutoSize);
-        Assert.Equal ("Test", label.Text);
-        Application.Top.Add (label);
-        rs = Application.Begin (Application.Top);
-
-        Assert.Equal ("Test", label.TextFormatter.Text);
-        Assert.Equal (new Rectangle (0, 0, 4, 1), label.Frame);
-
-        expected = @"
-Test
-";
-        TestHelpers.AssertDriverContentsWithFrameAre (expected, _output);
-        Application.End (rs);
-
-        label = new Label { X = 3, Y = 4, Text = "Test" };
-        Assert.Equal ("Test", label.Text);
-        Application.Top.Add (label);
-        rs = Application.Begin (Application.Top);
-
-        Assert.Equal ("Test", label.TextFormatter.Text);
-        Assert.Equal (new Rectangle (3, 4, 4, 1), label.Frame);
-
-        expected = @"
-   Test
-";
-        TestHelpers.AssertDriverContentsWithFrameAre (expected, _output);
-
-        Application.End (rs);
     }
 
     [Fact]

UnitTests/Views/ToplevelTests.cs

@@ -2030,6 +2030,7 @@ public class ToplevelTests
                                    );
 
             Application.Run (firstWindow);
+            firstWindow.Dispose ();
         }
 
         void SecondWindow (object sender, EventArgs args)
@@ -2060,6 +2061,7 @@ public class ToplevelTests
                                    );
 
             Application.Run (testWindow);
+            testWindow.Dispose ();
         }
 
         Application.Run ();