Fixes #3678. ContextMenu accesses disposed MenuBar. (#3681)

* Fixes #3678. ContextMenu accesses disposed MenuBar.

* Preserve always the last menu opened.

* Re-added MenuAllClosed event.

* Ensures call CleanUp if Application.MouseGrabView hold another MenuBar than the calling one.

* Fix unit tests.

* Passing the offending object instead of the View type.

* Add that to the dictionary to shut respeller up.

* Address a few warnings

* A question

* Just a little more cleanup

* Combine these and comment

* Slight additional cleanup

* More minor cleanup

We already know it is null. just make it the else instead of a new condition

* Nullable != true ===> is not true

* Unconditional break at the top is just a part of the while statement (inverted)

* Fixes #3687. ColorPicker isn't respecting the current UI culture.

* Fix @dodexahedron erroneous code.

* Cleanup comments.

---------

Co-authored-by: Brandon Thetford <[email protected]>
Co-authored-by: Tig <[email protected]>

Terminal.Gui/Application/Application.Mouse.cs

@@ -1,5 +1,6 @@
-#nullable enable
+#nullable enable
 namespace Terminal.Gui;
+
 public static partial class Application // Mouse handling
 {
     #region Mouse handling
@@ -36,16 +37,13 @@ public static partial class Application // Mouse handling
     /// <param name="view">View that will receive all mouse events until <see cref="UngrabMouse"/> is invoked.</param>
     public static void GrabMouse (View? view)
     {
-        if (view is null)
+        if (view is null || OnGrabbingMouse (view))
         {
             return;
         }
 
-        if (!OnGrabbingMouse (view))
-        {
-            OnGrabbedMouse (view);
-            MouseGrabView = view;
-        }
+        OnGrabbedMouse (view);
+        MouseGrabView = view;
     }
 
     /// <summary>Releases the mouse grab, so mouse events will be routed to the view on which the mouse is.</summary>
@@ -56,6 +54,10 @@ public static partial class Application // Mouse handling
             return;
         }
 
+#if DEBUG_IDISPOSABLE
+        ObjectDisposedException.ThrowIf (MouseGrabView.WasDisposed, MouseGrabView);
+#endif
+
         if (!OnUnGrabbingMouse (MouseGrabView))
         {
             View view = MouseGrabView;
@@ -64,6 +66,7 @@ public static partial class Application // Mouse handling
         }
     }
 
+    /// <exception cref="Exception">A delegate callback throws an exception.</exception>
     private static bool OnGrabbingMouse (View? view)
     {
         if (view is null)
@@ -77,6 +80,7 @@ public static partial class Application // Mouse handling
         return evArgs.Cancel;
     }
 
+    /// <exception cref="Exception">A delegate callback throws an exception.</exception>
     private static bool OnUnGrabbingMouse (View? view)
     {
         if (view is null)
@@ -90,6 +94,7 @@ public static partial class Application // Mouse handling
         return evArgs.Cancel;
     }
 
+    /// <exception cref="Exception">A delegate callback throws an exception.</exception>
     private static void OnGrabbedMouse (View? view)
     {
         if (view is null)
@@ -100,6 +105,7 @@ public static partial class Application // Mouse handling
         GrabbedMouse?.Invoke (view, new (view));
     }
 
+    /// <exception cref="Exception">A delegate callback throws an exception.</exception>
     private static void OnUnGrabbedMouse (View? view)
     {
         if (view is null)
@@ -110,8 +116,6 @@ public static partial class Application // Mouse handling
         UnGrabbedMouse?.Invoke (view, new (view));
     }
 
-#nullable enable
-
     // Used by OnMouseEvent to track the last view that was clicked on.
     internal static View? MouseEnteredView { get; set; }
 
@@ -166,51 +170,49 @@ public static partial class Application // Mouse handling
             if ((MouseGrabView.Viewport with { Location = Point.Empty }).Contains (viewRelativeMouseEvent.Position) is false)
             {
                 // The mouse has moved outside the bounds of the view that grabbed the mouse
-                MouseGrabView?.NewMouseLeaveEvent (mouseEvent);
+                MouseGrabView.NewMouseLeaveEvent (mouseEvent);
             }
 
             //System.Diagnostics.Debug.WriteLine ($"{nme.Flags};{nme.X};{nme.Y};{mouseGrabView}");
-            if (MouseGrabView?.NewMouseEvent (viewRelativeMouseEvent) == true)
+            if (MouseGrabView.NewMouseEvent (viewRelativeMouseEvent) is true)
             {
                 return;
             }
         }
 
-        if (view is { WantContinuousButtonPressed: true })
+        // We can combine this into the switch expression to reduce cognitive complexity even more and likely
+        // avoid one or two of these checks in the process, as well.
+        WantContinuousButtonPressedView = view switch
+                                          {
+                                              { WantContinuousButtonPressed: true } => view,
+                                              _                                     => null
+                                          };
+
+        if (view is not Adornment
+         && (view is null || view == ApplicationOverlapped.OverlappedTop)
+         && Current is { Modal: false }
+         && ApplicationOverlapped.OverlappedTop != null
+         && mouseEvent.Flags is not MouseFlags.ReportMousePosition and not 0)
         {
-            WantContinuousButtonPressedView = view;
-        }
-        else
-        {
-            WantContinuousButtonPressedView = null;
-        }
+            // This occurs when there are multiple overlapped "tops"
+            // E.g. "Mdi" - in the Background Worker Scenario
+            View? top = ApplicationOverlapped.FindDeepestTop (Top!, mouseEvent.Position);
+            view = View.FindDeepestView (top, mouseEvent.Position);
 
-        if (view is not Adornment)
-        {
-            if ((view is null || view == ApplicationOverlapped.OverlappedTop)
-                && Current is { Modal: false }
-                && ApplicationOverlapped.OverlappedTop != null
-                && mouseEvent.Flags != MouseFlags.ReportMousePosition
-                && mouseEvent.Flags != 0)
+            if (view is { } && view != ApplicationOverlapped.OverlappedTop && top != Current && top is { })
             {
-                // This occurs when there are multiple overlapped "tops"
-                // E.g. "Mdi" - in the Background Worker Scenario
-                View? top = ApplicationOverlapped.FindDeepestTop (Top!, mouseEvent.Position);
-                view = View.FindDeepestView (top, mouseEvent.Position);
-
-                if (view is { } && view != ApplicationOverlapped.OverlappedTop && top != Current && top is { })
-                {
-                    ApplicationOverlapped.MoveCurrent ((Toplevel)top);
-                }
+                ApplicationOverlapped.MoveCurrent ((Toplevel)top);
             }
         }
 
+        // May be null before the prior condition or the condition may set it as null.
+        // So, the checking must be outside the prior condition.
         if (view is null)
         {
             return;
         }
 
-        MouseEvent? me = null;
+        MouseEvent? me;
 
         if (view is Adornment adornment)
         {
@@ -236,8 +238,7 @@ public static partial class Application // Mouse handling
                 View = view
             };
         }
-
-        if (me is null)
+        else
         {
             return;
         }
@@ -263,13 +264,8 @@ public static partial class Application // Mouse handling
 
         //Debug.WriteLine ($"OnMouseEvent: ({a.MouseEvent.X},{a.MouseEvent.Y}) - {a.MouseEvent.Flags}");
 
-        while (view.NewMouseEvent (me) != true)
+        while (view.NewMouseEvent (me) is not true && MouseGrabView is not { })
         {
-            if (MouseGrabView is { })
-            {
-                break;
-            }
-
             if (view is Adornment adornmentView)
             {
                 view = adornmentView.Parent!.SuperView;

Terminal.Gui/Views/Menu/ContextMenu.cs

@@ -105,18 +105,17 @@ public sealed class ContextMenu : IDisposable
     /// <summary>Disposes the context menu object.</summary>
     public void Dispose ()
     {
-        if (IsShow)
-        {
-            _menuBar.MenuAllClosed -= MenuBar_MenuAllClosed;
-            _menuBar.Dispose ();
-            _menuBar = null;
-            IsShow = false;
-        }
+        _menuBar.MenuAllClosed -= MenuBar_MenuAllClosed;
+        Application.UngrabMouse ();
+        _menuBar?.Dispose ();
+        _menuBar = null;
+        IsShow = false;
 
         if (_container is { })
         {
             _container.Closing -= Container_Closing;
             _container.Deactivate -= Container_Deactivate;
+            _container.Disposing -= Container_Disposing;
         }
     }
 
@@ -124,7 +123,7 @@ public sealed class ContextMenu : IDisposable
     public void Hide ()
     {
         _menuBar?.CleanUp ();
-        Dispose ();
+        IsShow = false;
     }
 
     /// <summary>Event invoked when the <see cref="ContextMenu.Key"/> is changed.</summary>
@@ -139,11 +138,13 @@ public sealed class ContextMenu : IDisposable
         if (_menuBar is { })
         {
             Hide ();
+            Dispose ();
         }
 
         _container = Application.Current;
-        _container.Closing += Container_Closing;
+        _container!.Closing += Container_Closing;
         _container.Deactivate += Container_Deactivate;
+        _container.Disposing += Container_Disposing;
         Rectangle frame = Application.Screen;
         Point position = Position;
 
@@ -219,7 +220,9 @@ public sealed class ContextMenu : IDisposable
         _menuBar.OpenMenu ();
     }
 
-    private void Container_Deactivate (object sender, ToplevelEventArgs e) { Hide (); }
     private void Container_Closing (object sender, ToplevelClosingEventArgs obj) { Hide (); }
-    private void MenuBar_MenuAllClosed (object sender, EventArgs e) { Dispose (); }
+    private void Container_Deactivate (object sender, ToplevelEventArgs e) { Hide (); }
+    private void Container_Disposing (object sender, EventArgs e) { Dispose (); }
+
+    private void MenuBar_MenuAllClosed (object sender, EventArgs e) { Hide (); }
 }

Terminal.Gui/Views/Menu/MenuBar.cs

@@ -479,6 +479,16 @@ public class MenuBar : View, IDesignable
         }
 
         SetNeedsDisplay ();
+
+        if (Application.MouseGrabView is { } && Application.MouseGrabView is MenuBar && Application.MouseGrabView != this)
+        {
+            var menuBar = Application.MouseGrabView as MenuBar;
+
+            if (menuBar!.IsMenuOpen)
+            {
+                menuBar.CleanUp ();
+            }
+        }
         Application.UngrabMouse ();
         _isCleaning = false;
     }

Terminal.sln.DotSettings

@@ -406,6 +406,7 @@
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=langword/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Roslynator/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Toplevel/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/UserDictionary/Words/=Ungrab/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=unsynchronized/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=BUGBUG/@EntryIndexedValue">True</s:Boolean>
 </wpf:ResourceDictionary>

UICatalog/Scenarios/ContextMenus.cs

@@ -93,9 +93,22 @@ public class ContextMenus : Scenario
                                 Application.MouseEvent -= ApplicationMouseEvent;
                             };
 
+        var menu = new MenuBar
+        {
+            Menus =
+            [
+                new (
+                     "File",
+                     new MenuItem [] { new ("Quit", "", () => Application.RequestStop (), null, null, Application.QuitKey) })
+            ]
+        };
+
+        var top = new Toplevel ();
+        top.Add (appWindow, menu);
+
         // Run - Start the application.
-        Application.Run (appWindow);
-        appWindow.Dispose ();
+        Application.Run (top);
+        top.Dispose ();
 
         // Shutdown - Calling Application.Shutdown is required.
         Application.Shutdown ();

UnitTests/Views/ContextMenuTests.cs

@@ -390,7 +390,8 @@ public class ContextMenuTests (ITestOutputHelper output)
         Assert.True (Application.OnKeyDown (ContextMenu.DefaultKey));
         Assert.True (tf.ContextMenu.MenuBar.IsMenuOpen);
         Assert.True (Application.OnKeyDown (ContextMenu.DefaultKey));
-        Assert.Null (tf.ContextMenu.MenuBar);
+        // The last context menu bar opened is always preserved
+        Assert.NotNull (tf.ContextMenu.MenuBar);
         top.Dispose ();
     }
 
@@ -1390,7 +1391,8 @@ public class ContextMenuTests (ITestOutputHelper output)
         Assert.True (tf1.HasFocus);
         Assert.False (tf2.HasFocus);
         Assert.Equal (2, win.Subviews.Count);
-        Assert.Null (tf2.ContextMenu.MenuBar);
+        // The last context menu bar opened is always preserved
+        Assert.NotNull (tf2.ContextMenu.MenuBar);
         Assert.Equal (win.Focused, tf1);
         Assert.Null (Application.MouseGrabView);
         Assert.Equal (tf1, Application.MouseEnteredView);
@@ -1400,7 +1402,8 @@ public class ContextMenuTests (ITestOutputHelper output)
         Assert.False (tf1.HasFocus);
         Assert.True (tf2.HasFocus);
         Assert.Equal (2, win.Subviews.Count);
-        Assert.Null (tf2.ContextMenu.MenuBar);
+        // The last context menu bar opened is always preserved
+        Assert.NotNull (tf2.ContextMenu.MenuBar);
         Assert.Equal (win.Focused, tf2);
         Assert.Null (Application.MouseGrabView);
         Assert.Equal (tf2, Application.MouseEnteredView);