Partially Fixes #2975 - Replaces old ContextMenu with new Bar/Shortcut based implementation (#4008)

* touching publish.yml

* Nuked ContextMenuv2 - use PopverMenu instead

* WIP context menu stuff

* More robust dispose

* Removed ConextMenu; use PopoverMenu instead

* Code cleanup

* Code cleanup2

Author: tig

Terminal.Gui/Application/Application.Initialization.cs

@@ -82,7 +82,10 @@ internal static void InternalInit (
             ResetState (ignoreDisposed: true);
         }
 
+        Debug.Assert (Navigation is null);
         Navigation = new ();
+
+        Debug.Assert(Popover is null);
         Popover = new ();
 
         // For UnitTests

Terminal.Gui/Application/Application.Run.cs

@@ -564,7 +564,7 @@ public static void End (RunState runState)
     {
         ArgumentNullException.ThrowIfNull (runState);
 
-        Popover?.HidePopover (Popover?.GetActivePopover ());
+        Popover?.Hide (Popover?.GetActivePopover ());
 
         runState.Toplevel.OnUnloaded ();
 

Terminal.Gui/Application/Application.cs

@@ -153,6 +153,7 @@ internal static void ResetState (bool ignoreDisposed = false)
         {
             popover.Visible = false;
         }
+        Popover?.Dispose ();
         Popover = null;
 
         TopLevels.Clear ();

Terminal.Gui/Application/ApplicationPopover.cs

@@ -1,13 +1,12 @@
 #nullable enable
 
-using System.Diagnostics;
-
 namespace Terminal.Gui;
 
 /// <summary>
-///     Helper class for support of <see cref="IPopover"/> views for <see cref="Application"/>. Held by <see cref="Application.Popover"/>
+///     Helper class for support of <see cref="IPopover"/> views for <see cref="Application"/>. Held by
+///     <see cref="Application.Popover"/>
 /// </summary>
-public class ApplicationPopover
+public sealed class ApplicationPopover : IDisposable
 {
     /// <summary>
     ///     Initializes a new instance of the <see cref="ApplicationPopover"/> class.
@@ -16,27 +15,41 @@ public ApplicationPopover () { }
 
     private readonly List<IPopover> _popovers = [];
 
-    /// <summary></summary>
+    /// <summary>
+    ///     Gets the list of popovers registered with the application.
+    /// </summary>
     public IReadOnlyCollection<IPopover> Popovers => _popovers.AsReadOnly ();
 
     /// <summary>
     ///     Registers <paramref name="popover"/> with the application.
-    ///     This enables the popover to receive keyboard events even when when it is not active.
+    ///     This enables the popover to receive keyboard events even when it is not active.
     /// </summary>
+    /// <remarks>
+    ///     When a popover is registered, the View instance lifetime is managed by the application. Call
+    ///     <see cref="DeRegister"/>
+    ///     to manage the lifetime of the popover directly.
+    /// </remarks>
     /// <param name="popover"></param>
-    public void Register (IPopover? popover)
+    /// <returns><paramref name="popover"/>, after it has been registered.</returns>
+    public IPopover? Register (IPopover? popover)
     {
         if (popover is { } && !_popovers.Contains (popover))
         {
             _popovers.Add (popover);
-
         }
+
+        return popover;
     }
 
     /// <summary>
     ///     De-registers <paramref name="popover"/> with the application. Use this to remove the popover and it's
     ///     keyboard bindings from the application.
     /// </summary>
+    /// <remarks>
+    ///     When a popover is registered, the View instance lifetime is managed by the application. Call
+    ///     <see cref="DeRegister"/>
+    ///     to manage the lifetime of the popover directly.
+    /// </remarks>
     /// <param name="popover"></param>
     /// <returns></returns>
     public bool DeRegister (IPopover? popover)
@@ -61,22 +74,25 @@ public bool DeRegister (IPopover? popover)
     /// <summary>
     ///     Gets the active popover, if any.
     /// </summary>
+    /// <remarks>
+    ///     Note, the active pop over does not necessarily to be registered with the application.
+    /// </remarks>
     /// <returns></returns>
     public IPopover? GetActivePopover () { return _activePopover; }
 
     /// <summary>
-    ///     Shows <paramref name="popover"/>. IPopover implementations should use OnVisibleChnaged/VisibleChanged to be
+    ///     Shows <paramref name="popover"/>. IPopover implementations should use OnVisibleChanaged/VisibleChanged to be
     ///     notified when the user has done something to cause the popover to be hidden.
     /// </summary>
     /// <remarks>
     ///     <para>
-    ///         Note, this API calls <see cref="Register"/>. To disable the popover from processing keyboard events,
+    ///         This API calls <see cref="Register"/>. To disable the popover from processing keyboard events,
     ///         either call <see cref="DeRegister"/> to
     ///         remove the popover from the application or set <see cref="View.Enabled"/> to <see langword="false"/>.
     ///     </para>
     /// </remarks>
     /// <param name="popover"></param>
-    public void ShowPopover (IPopover? popover)
+    public void Show (IPopover? popover)
     {
         // If there's an existing popover, hide it.
         if (_activePopover is View popoverView)
@@ -87,8 +103,6 @@ public void ShowPopover (IPopover? popover)
 
         if (popover is View newPopover)
         {
-            Register (popover);
-
             if (!newPopover.IsInitialized)
             {
                 newPopover.BeginInit ();
@@ -103,10 +117,11 @@ public void ShowPopover (IPopover? popover)
 
     /// <summary>
     ///     Causes the specified popover to be hidden.
-    ///     If the popover is dervied from <see cref="PopoverBaseImpl"/>, this is the same as setting <see cref="View.Visible"/> to <see langword="false"/>.
+    ///     If the popover is dervied from <see cref="PopoverBaseImpl"/>, this is the same as setting
+    ///     <see cref="View.Visible"/> to <see langword="false"/>.
     /// </summary>
     /// <param name="popover"></param>
-    public void HidePopover (IPopover? popover)
+    public void Hide (IPopover? popover)
     {
         // If there's an existing popover, hide it.
         if (_activePopover is View popoverView && popoverView == popover)
@@ -117,7 +132,6 @@ public void HidePopover (IPopover? popover)
         }
     }
 
-
     /// <summary>
     ///     Called when the user presses a key. Dispatches the key to the active popover, if any,
     ///     otherwise to the popovers in the order they were registered. Inactive popovers only get hotkeys.
@@ -127,9 +141,11 @@ public void HidePopover (IPopover? popover)
     internal bool DispatchKeyDown (Key key)
     {
         // Do active first - Active gets all key down events.
-        if (GetActivePopover () as View is { Visible: true } visiblePopover)
+        var activePopover = GetActivePopover () as View;
+
+        if (activePopover is { Visible: true })
         {
-            if (visiblePopover.NewKeyDownEvent (key))
+            if (activePopover.NewKeyDownEvent (key))
             {
                 return true;
             }
@@ -141,7 +157,7 @@ internal bool DispatchKeyDown (Key key)
 
         foreach (IPopover popover in _popovers)
         {
-            if (GetActivePopover () == popover || popover is not View popoverView)
+            if (popover == activePopover || popover is not View popoverView)
             {
                 continue;
             }
@@ -157,4 +173,18 @@ internal bool DispatchKeyDown (Key key)
 
         return hotKeyHandled is true;
     }
+
+    /// <inheritdoc/>
+    public void Dispose ()
+    {
+        foreach (IPopover popover in _popovers)
+        {
+            if (popover is View view)
+            {
+                view.Dispose ();
+            }
+        }
+
+        _popovers.Clear ();
+    }
 }

Terminal.Gui/Application/PopoverBaseImpl.cs

@@ -6,19 +6,18 @@ namespace Terminal.Gui;
 /// </summary>
 /// <remarks>
 ///     <para>
-///         To show a Popover, use <see cref="ApplicationPopover.ShowPopover"/>. To hide a popover,
-///         call <see cref="ApplicationPopover.ShowPopover"/> with <see langword="null"/> set <see cref="View.Visible"/> to <see langword="false"/>.
+///         To show a Popover, use <see cref="ApplicationPopover.Show"/>. To hide a popover,
+///         call <see cref="ApplicationPopover.Show"/> with <see langword="null"/> set <see cref="View.Visible"/> to <see langword="false"/>.
 ///     </para>
 ///     <para>
-///         If the user clicks anywhere not occulded by a SubView of the Popover, presses <see cref="Application.QuitKey"/>,
+///         If the user clicks anywhere not occluded by a SubView of the Popover, presses <see cref="Application.QuitKey"/>,
 ///         or causes another popover to show, the Popover will be hidden.
 ///     </para>
 /// </remarks>
-
 public abstract class PopoverBaseImpl : View, IPopover
 {
     /// <summary>
-    /// 
+    ///     Creates a new PopoverBaseImpl.
     /// </summary>
     protected PopoverBaseImpl ()
     {
@@ -28,10 +27,10 @@ protected PopoverBaseImpl ()
         Height = Dim.Fill ();
         ViewportSettings = ViewportSettings.Transparent | ViewportSettings.TransparentMouse;
 
-        //// TODO: Add a diagnostic setting for this?
-        TextFormatter.VerticalAlignment = Alignment.End;
-        TextFormatter.Alignment = Alignment.End;
-        base.Text = "popover";
+        // TODO: Add a diagnostic setting for this?
+        //TextFormatter.VerticalAlignment = Alignment.End;
+        //TextFormatter.Alignment = Alignment.End;
+        //base.Text = "popover";
 
         AddCommand (Command.Quit, Quit);
         KeyBindings.Add (Application.QuitKey, Command.Quit);
@@ -55,24 +54,13 @@ protected PopoverBaseImpl ()
     protected override bool OnVisibleChanging ()
     {
         bool ret = base.OnVisibleChanging ();
-        if (!ret & !Visible)
+        if (!ret && !Visible)
         {
-            // Whenvver visible is changing to true, we need to resize;
+            // Whenever visible is changing to true, we need to resize;
             // it's our only chance because we don't get laid out until we're visible
             Layout (Application.Screen.Size);
         }
 
         return ret;
     }
-
-    // TODO: Pretty sure this is not needed. set_Visible SetFocus already
-    ///// <inheritdoc />
-    //protected override void OnVisibleChanged ()
-    //{
-    //    base.OnVisibleChanged ();
-    //    if (Visible)
-    //    {
-    //        //SetFocus ();
-    //    }
-    //}
 }

Terminal.Gui/ConsoleDrivers/V2/ApplicationV2.cs

@@ -1,5 +1,6 @@
 #nullable enable
 using System.Collections.Concurrent;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using Microsoft.Extensions.Logging;
 
@@ -63,7 +64,10 @@ public override void Init (IConsoleDriver? driver = null, string? driverName = n
             _driverName = driverName;
         }
 
+        Debug.Assert(Application.Navigation is null);
         Application.Navigation = new ();
+
+        Debug.Assert (Application.Popover is null);
         Application.Popover = new ();
 
         Application.AddKeyBindings ();

Terminal.Gui/Drawing/Color/ColorScheme.Colors.cs

@@ -44,7 +44,7 @@ static Colors ()
     ///             <item>
     ///                 <term>Menu</term>
     ///                 <description>
-    ///                     The menu color scheme; used for <see cref="MenuBar"/>, <see cref="ContextMenu"/>, and
+    ///                     The menu color scheme; used for <see cref="Menu"/>, <see cref="MenuBar"/>, and
     ///                     <see cref="StatusBar"/>.
     ///                 </description>
     ///             </item>

Terminal.Gui/Resources/config.json

@@ -33,7 +33,7 @@
 
 
   // --------------- View Specific Settings ---------------
-  "ContextMenu.DefaultKey": "Shift+F10",
+  "PopoverMenu.DefaultKey": "Shift+F10",
   "FileDialog.MaxSearchResults": 10000,
   "FileDialogStyle.DefaultUseColors": false,
   "FileDialogStyle.DefaultUseUnicodeCharacters": false,

Terminal.Gui/Terminal.Gui.csproj

@@ -81,7 +81,7 @@
     <!-- Assembly names for which internal items are visible -->
     <!-- =================================================================== -->
     <ItemGroup>
-        <InternalsVisibleTo Include="Benchmarks"/>
+        <InternalsVisibleTo Include="Benchmarks" />
         <InternalsVisibleTo Include="UnitTests" />
         <InternalsVisibleTo Include="UnitTests.Parallelizable" />
         <InternalsVisibleTo Include="StressTests" />

Terminal.Gui/Views/CharMap/CharMap.cs

@@ -18,8 +18,6 @@ public class CharMap : View, IDesignable
     private const int HEADER_HEIGHT = 1; // Height of the header
     private int _rowHeight = 1; // Height of each row of 16 glyphs - changing this is not tested
 
-    private ContextMenu _contextMenu = new ();
-
     /// <summary>
     ///     Initializes a new instance.
     /// </summary>
@@ -58,7 +56,7 @@ public CharMap ()
         KeyBindings.Add (Key.PageDown, Command.PageDown);
         KeyBindings.Add (Key.Home, Command.Start);
         KeyBindings.Add (Key.End, Command.End);
-        KeyBindings.Add (ContextMenu.DefaultKey, Command.Context);
+        KeyBindings.Add (PopoverMenu.DefaultKey, Command.Context);
 
         MouseBindings.Add (MouseFlags.Button1DoubleClicked, Command.Accept);
         MouseBindings.ReplaceCommands (MouseFlags.Button3Clicked, Command.Context);
@@ -505,32 +503,20 @@ public static string ToCamelCase (string str)
 
         SelectedCodePoint = newCodePoint;
 
-        _contextMenu = new ()
-        {
-            Position = ViewportToScreen (GetCursor (SelectedCodePoint))
-        };
+        // This demonstrates how to create an ephemeral Popover; one that exists
+        // ony as long as the popover is visible.
+        // Note, for ephemeral Popovers, hotkeys are not supported.
+        PopoverMenu? contextMenu = new (
+                                        [
+                                            new (Strings.charMapCopyGlyph, string.Empty, CopyGlyph),
+                                            new (Strings.charMapCopyCP, string.Empty, CopyCodePoint)
+                                        ]);
+
+        // Registering with the PopoverManager will ensure that the context menu is closed when the view is no longer focused
+        // and the context menu is disposed when it is closed.
+        Application.Popover?.Register (contextMenu);
 
-        MenuBarItem menuItems = new (
-                                     [
-                                         new (
-                                              Strings.charMapCopyGlyph,
-                                              "",
-                                              CopyGlyph,
-                                              null,
-                                              null,
-                                              (KeyCode)Key.G.WithCtrl
-                                             ),
-                                         new (
-                                              Strings.charMapCopyCP,
-                                              "",
-                                              CopyCodePoint,
-                                              null,
-                                              null,
-                                              (KeyCode)Key.P.WithCtrl
-                                             )
-                                     ]
-                                    );
-        _contextMenu.Show (menuItems);
+        contextMenu?.MakeVisible (ViewportToScreen (GetCursor (SelectedCodePoint)));
 
         return true;
     }