Fixes #3338. Application.Run/End -> Callers must dispose Toplevel

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;
+
     /// <summary>
     ///     Notify that a new <see cref="RunState"/> was created (<see cref="Begin(Toplevel)"/> was called). The token is
     ///     created in <see cref="Begin(Toplevel)"/> 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)

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

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 ();
+        }
+    }
 }

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]