Fixes #4122 - Improves `IPopover` (#4124)

* upgraded to gitversion 6 2

* Remove invalid prevent-increment property

* upgraded to gitversion 6 3

* Fixed gitversion

* cleanup

* Added namespace docs

* Improved API docs.
Added runtime checks.
Refactored mouse check code.

* Refinements

Terminal.Gui/App/Application.Mouse.cs

@@ -185,11 +185,7 @@ public static partial class Application // Mouse handling
             && Popover?.GetActivePopover () as View is { Visible: true } visiblePopover
             && View.IsInHierarchy (visiblePopover, deepestViewUnderMouse, includeAdornments: true) is false)
         {
-            // TODO: Build a use/test case for the popover not handling Quit
-            if (visiblePopover.InvokeCommand (Command.Quit) is true && visiblePopover.Visible)
-            {
-                visiblePopover.Visible = false;
-            }
+            ApplicationPopover.HideWithQuitCommand (visiblePopover);
 
             // Recurse once so the event can be handled below the popover
             RaiseMouseEvent (mouseEvent);
@@ -205,10 +201,10 @@ public static partial class Application // Mouse handling
         if (Initialized)
         {
             WantContinuousButtonPressedView = deepestViewUnderMouse switch
-                                              {
-                                                  { WantContinuousButtonPressed: true } => deepestViewUnderMouse,
-                                                  _ => null
-                                              };
+            {
+                { WantContinuousButtonPressed: true } => deepestViewUnderMouse,
+                _ => null
+            };
         }
 
         // May be null before the prior condition or the condition may set it as null.

Terminal.Gui/App/Application.Run.cs

@@ -577,11 +577,7 @@ public static partial class Application // Run (Begin, Run, End, Stop)
 
         if (Popover?.GetActivePopover () as View is { Visible: true } visiblePopover)
         {
-            // TODO: Build a use/test case for the popover not handling Quit
-            if (visiblePopover.InvokeCommand (Command.Quit) is true && visiblePopover.Visible)
-            {
-                visiblePopover.Visible = false;
-            }
+            ApplicationPopover.HideWithQuitCommand (visiblePopover);
         }
 
         runState.Toplevel.OnUnloaded ();

Terminal.Gui/App/ApplicationPopover.cs

@@ -1,5 +1,6 @@
 #nullable enable
 
+using System.Diagnostics;
 
 namespace Terminal.Gui.App;
 
@@ -36,12 +37,8 @@ public sealed class ApplicationPopover : IDisposable
     {
         if (popover is { } && !_popovers.Contains (popover))
         {
-
             // When created, set IPopover.Toplevel to the current Application.Top
-            if (popover.Toplevel is null)
-            {
-                popover.Toplevel = Application.Top;
-            }
+            popover.Toplevel ??= Application.Top;
 
             _popovers.Add (popover);
         }
@@ -62,19 +59,20 @@ public sealed class ApplicationPopover : IDisposable
     /// <returns></returns>
     public bool DeRegister (IPopover? popover)
     {
-        if (popover is { } && _popovers.Contains (popover))
+        if (popover is null || !_popovers.Contains (popover))
         {
-            if (GetActivePopover () == popover)
-            {
-                _activePopover = null;
-            }
-
-            _popovers.Remove (popover);
+            return false;
+        }
 
-            return true;
+        if (GetActivePopover () == popover)
+        {
+            _activePopover = null;
         }
 
-        return false;
+        _popovers.Remove (popover);
+
+        return true;
+
     }
 
     private IPopover? _activePopover;
@@ -111,6 +109,18 @@ public sealed class ApplicationPopover : IDisposable
 
         if (popover is View newPopover)
         {
+            if (!(newPopover.ViewportSettings.HasFlag (ViewportSettingsFlags.Transparent) &&
+                  newPopover.ViewportSettings.HasFlag (ViewportSettingsFlags.TransparentMouse)))
+            {
+                throw new InvalidOperationException ("Popovers must have ViewportSettings.Transparent and ViewportSettings.TransparentMouse set.");
+            }
+
+            if (newPopover.KeyBindings.GetFirstFromCommands (Command.Quit) is null)
+            {
+                throw new InvalidOperationException ("Popovers must have a key binding for Command.Quit.");
+            }
+
+
             Register (popover);
 
             if (!newPopover.IsInitialized)
@@ -127,7 +137,7 @@ public sealed class ApplicationPopover : IDisposable
 
     /// <summary>
     ///     Causes the specified popover to be hidden.
-    ///     If the popover is dervied from <see cref="PopoverBaseImpl"/>, this is the same as setting
+    ///     If the popover is derived from <see cref="PopoverBaseImpl"/>, this is the same as setting
     ///     <see cref="View.Visible"/> to <see langword="false"/>.
     /// </summary>
     /// <param name="popover"></param>
@@ -142,6 +152,22 @@ public sealed class ApplicationPopover : IDisposable
         }
     }
 
+    /// <summary>
+    ///     Hides a popover view if it supports the quit command and is currently visible. It checks for the command's
+    ///     support before hiding.
+    /// </summary>
+    /// <param name="visiblePopover">The view that is being checked and potentially hidden based on its visibility and command support.</param>
+    internal static void HideWithQuitCommand (View visiblePopover)
+    {
+        if (visiblePopover.Visible
+            && (!visiblePopover.GetSupportedCommands ().Contains (Command.Quit)
+            || (visiblePopover.InvokeCommand (Command.Quit) is true && visiblePopover.Visible)))
+        {
+            visiblePopover.Visible = false;
+        }
+    }
+
+
     /// <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.
@@ -155,7 +181,7 @@ public sealed class ApplicationPopover : IDisposable
 
         if (activePopover is { Visible: true })
         {
-            Logging.Debug ($"Active - Calling NewKeyDownEvent ({key}) on {activePopover.Title}");
+            //Logging.Debug ($"Active - Calling NewKeyDownEvent ({key}) on {activePopover.Title}");
 
             if (activePopover.NewKeyDownEvent (key))
             {
@@ -177,7 +203,7 @@ public sealed class ApplicationPopover : IDisposable
             }
 
             // hotKeyHandled = popoverView.InvokeCommandsBoundToHotKey (key);
-            Logging.Debug ($"Inactive - Calling NewKeyDownEvent ({key}) on {popoverView.Title}");
+            //Logging.Debug ($"Inactive - Calling NewKeyDownEvent ({key}) on {popoverView.Title}");
             hotKeyHandled = popoverView.NewKeyDownEvent (key);
 
             if (hotKeyHandled is true)

Terminal.Gui/App/IPopover.cs

@@ -3,8 +3,32 @@
 namespace Terminal.Gui.App;
 
 /// <summary>
-///     Interface identifying a View as being capable of being a Popover.
+///     Defines the contract for a popover view in Terminal.Gui.
 /// </summary>
+/// <remarks>
+///     <para>
+///         A popover is a transient UI element that appears above other content to display contextual information or UI,
+///         such as menus, tooltips, or dialogs.
+///         Popovers are managed by <see cref="ApplicationPopover"/> and are typically shown using
+///         <see cref="ApplicationPopover.Show"/>.
+///     </para>
+///     <para>
+///         Popovers are not modal; they do not block input to the rest of the application, but they do receive focus and
+///         input events while visible.
+///         When a popover is shown, it is responsible for handling its own layout and content.
+///     </para>
+///     <para>
+///         Popovers are automatically hidden when:
+///         <list type="bullet">
+///             <item>The user clicks outside the popover (unless occluded by a subview of the popover).</item>
+///             <item>The user presses <see cref="Application.QuitKey"/> (typically <c>Esc</c>).</item>
+///             <item>Another popover is shown.</item>
+///         </list>
+///     </para>
+///     <para>
+///         To implement a custom popover, inherit from <see cref="PopoverBaseImpl"/> or implement this interface directly.
+///     </para>
+/// </remarks>
 public interface IPopover
 {
     /// <summary>
@@ -15,5 +39,5 @@ public interface IPopover
     ///     When <see cref="ApplicationPopover.Register"/> is called, the <see cref="Toplevel"/> is set to the current
     ///     <see cref="Application.Top"/> if not already set.
     /// </summary>
-    public Toplevel? Toplevel { get; set; }
+    Toplevel? Toplevel { get; set; }
 }

Terminal.Gui/App/PopoverBaseImpl.cs

@@ -3,24 +3,41 @@
 namespace Terminal.Gui.App;
 
 /// <summary>
-///     Abstract base class for Popover Views.
+///     Abstract base class for popover views in Terminal.Gui.
 /// </summary>
 /// <remarks>
 ///     <para>
-///         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"/>.
+///         <b>Popover Lifecycle:</b><br/>
+///         To display a popover, use <see cref="ApplicationPopover.Show"/>. To hide a popover, either call
+///         <see cref="ApplicationPopover.Hide"/>,
+///         set <see cref="View.Visible"/> to <see langword="false"/>, or show another popover.
 ///     </para>
 ///     <para>
-///         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.
+///         <b>Focus and Input:</b><br/>
+///         When visible, a popover receives focus and input events. If the user clicks outside the popover (and not on a
+///         subview),
+///         presses <see cref="Application.QuitKey"/>, or another popover is shown, the popover will be hidden
+///         automatically.
+///     </para>
+///     <para>
+///         <b>Layout:</b><br/>
+///         When the popover becomes visible, it is automatically laid out to fill the screen by default. You can override
+///         this behavior
+///         by setting <see cref="View.Width"/> and <see cref="View.Height"/> in your derived class.
+///     </para>
+///     <para>
+///         <b>Custom Popovers:</b><br/>
+///         To create a custom popover, inherit from <see cref="PopoverBaseImpl"/> and add your own content and logic.
 ///     </para>
 /// </remarks>
 public abstract class PopoverBaseImpl : View, IPopover
 {
-
     /// <summary>
-    ///     Creates a new PopoverBaseImpl.
+    ///     Initializes a new instance of the <see cref="PopoverBaseImpl"/> class.
     /// </summary>
+    /// <remarks>
+    ///     By default, the popover fills the available screen area and is focusable.
+    /// </remarks>
     protected PopoverBaseImpl ()
     {
         Id = "popoverBaseImpl";
@@ -52,32 +69,43 @@ public abstract class PopoverBaseImpl : View, IPopover
         }
     }
 
-    /// <inheritdoc />
+    /// <inheritdoc/>
+    public Toplevel? Toplevel { get; set; }
+
+    /// <summary>
+    ///     Called when the <see cref="View.Visible"/> property is changing.
+    /// </summary>
+    /// <remarks>
+    ///     When becoming visible, the popover is laid out to fit the screen.
+    ///     When becoming hidden, focus is restored to the previous view.
+    /// </remarks>
+    /// <returns>
+    ///     <see langword="true"/> to cancel the visibility change; otherwise, <see langword="false"/>.
+    /// </returns>
     protected override bool OnVisibleChanging ()
     {
         bool ret = base.OnVisibleChanging ();
-        if (ret is not true)
+
+        if (ret)
         {
-            if (!Visible)
-            {
-                // 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);
-            }
-            else
+            return ret;
+        }
+
+        if (!Visible)
+        {
+            // 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);
+        }
+        else
+        {
+            // Whenever visible is changing to false, we need to reset the focus
+            if (ApplicationNavigation.IsInHierarchy (this, Application.Navigation?.GetFocused ()))
             {
-                // Whenever visible is changing to false, we need to reset the focus
-                if (ApplicationNavigation.IsInHierarchy(this, Application.Navigation?.GetFocused ()))
-                {
-                   Application.Navigation?.SetFocused (Application.Top?.MostFocused);
-                }
+                Application.Navigation?.SetFocused (Application.Top?.MostFocused);
             }
         }
 
         return ret;
     }
-
-    /// <inheritdoc />
-    public Toplevel? Toplevel { get; set; }
-
 }

Terminal.Gui/Input/InputBindings.cs

@@ -160,7 +160,7 @@ public abstract class InputBindings<TEvent, TBinding> where TBinding : IInputBin
     /// <param name="commands">The set of commands to search.</param>
     /// <returns>
     ///     The first matching <typeparamref name="TEvent"/> bound to the set of commands specified by
-    ///     <paramref name="commands"/>. <see langword="null"/> if the set of caommands was not found.
+    ///     <paramref name="commands"/>. <see langword="null"/> if the set of commands was not found.
     /// </returns>
     public TEvent? GetFirstFromCommands (params Command [] commands) { return _bindings.FirstOrDefault (a => a.Value.Commands.SequenceEqual (commands)).Key; }
 
@@ -169,7 +169,7 @@ public abstract class InputBindings<TEvent, TBinding> where TBinding : IInputBin
     /// <returns>
     ///     The <typeparamref name="TEvent"/>s bound to the set of commands specified by <paramref name="commands"/>. An empty list if
     ///     the
-    ///     set of caommands was not found.
+    ///     set of commands was not found.
     /// </returns>
     public IEnumerable<TEvent> GetAllFromCommands (params Command [] commands)
     {

Terminal.Gui/Views/Menu/PopoverMenu.cs

@@ -65,6 +65,8 @@ public class PopoverMenu : PopoverBaseImpl, IDesignable
         AddCommand (Command.Left, MoveLeft);
         KeyBindings.Add (Key.CursorLeft, Command.Left);
 
+        // PopoverBaseImpl sets a key binding for Quit, so we
+        // don't need to do it here.
         AddCommand (Command.Quit, Quit);
 
         return;

Tests/UnitTests/Application/ApplicationPopoverTests.cs

@@ -21,6 +21,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Application_Shutdown_Resets_PopoverManager ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());
@@ -37,6 +38,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Application_End_Does_Not_Reset_PopoverManager ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());
@@ -59,6 +61,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Application_End_Hides_Active ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());
@@ -87,6 +90,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Application_Shutdown_Disposes_Registered_Popovers ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());
@@ -104,6 +108,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Application_Shutdown_Does_Not_Dispose_DeRegistered_Popovers ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());
@@ -126,6 +131,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Application_Shutdown_Does_Not_Dispose_ActiveNotRegistered_Popover ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());
@@ -148,6 +154,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Register_SetsTopLevel ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());
@@ -166,6 +173,7 @@ public class ApplicationPopoverTests
     [Fact]
     public void Keyboard_Events_Go_Only_To_Popover_Associated_With_Toplevel ()
     {
+        Application.ResetState (true);
         // Arrange
         Assert.Null (Application.Popover);
         Application.Init (new FakeDriver ());

Tests/UnitTestsParallelizable/Application/ApplicationPopoverTests.cs

@@ -144,10 +144,28 @@ public class ApplicationPopoverTests
 
         public PopoverTestClass ()
         {
+            ViewportSettings = ViewportSettingsFlags.Transparent | ViewportSettingsFlags.TransparentMouse;
             CanFocus = true;
             AddCommand (Command.New, NewCommandHandler!);
             HotKeyBindings.Add (Key.N.WithCtrl, Command.New);
 
+            AddCommand (Command.Quit, Quit);
+            KeyBindings.Add (Application.QuitKey, Command.Quit);
+
+            return;
+
+            bool? Quit (ICommandContext? ctx)
+            {
+                if (!Visible)
+                {
+                    return false;
+                }
+
+                Visible = false;
+
+                return true;
+            }
+
             bool? NewCommandHandler (ICommandContext ctx)
             {
                 NewCommandInvokeCount++;

Tests/UnitTestsParallelizable/Application/PopoverBaseImplTests.cs

@@ -0,0 +1,68 @@
+using System;
+using Terminal.Gui;
+using Terminal.Gui.App;
+using Xunit;
+
+public class PopoverBaseImplTests
+{
+    // Minimal concrete implementation for testing
+    private class TestPopover : PopoverBaseImpl { }
+
+    [Fact]
+    public void Constructor_SetsDefaults ()
+    {
+        var popover = new TestPopover ();
+
+        Assert.Equal ("popoverBaseImpl", popover.Id);
+        Assert.True (popover.CanFocus);
+        Assert.Equal (Dim.Fill (), popover.Width);
+        Assert.Equal (Dim.Fill (), popover.Height);
+        Assert.True (popover.ViewportSettings.HasFlag (ViewportSettingsFlags.Transparent));
+        Assert.True (popover.ViewportSettings.HasFlag (ViewportSettingsFlags.TransparentMouse));
+    }
+
+    [Fact]
+    public void Toplevel_Property_CanBeSetAndGet ()
+    {
+        var popover = new TestPopover ();
+        var top = new Toplevel ();
+        popover.Toplevel = top;
+        Assert.Same (top, popover.Toplevel);
+    }
+
+    [Fact]
+    public void Show_ThrowsIfPopoverMissingRequiredFlags ()
+    {
+        var popover = new TestPopover ();
+
+        // Popover missing Transparent flags
+        popover.ViewportSettings = ViewportSettingsFlags.None; // Remove required flags
+
+        var popoverManager = new ApplicationPopover ();
+        // Test missing Transparent flags
+        Assert.ThrowsAny<Exception> (() => popoverManager.Show (popover));
+
+    }
+
+
+    [Fact]
+    public void Show_ThrowsIfPopoverMissingQuitCommand ()
+    {
+        var popover = new TestPopover ();
+
+        // Popover missing Command.Quit binding
+        popover.KeyBindings.Clear (); // Remove all key bindings
+
+        var popoverManager = new ApplicationPopover ();
+        Assert.ThrowsAny<Exception> (() => popoverManager.Show (popover));
+    }
+
+    [Fact]
+    public void Show_DoesNotThrow_BasePopoverImpl ()
+    {
+        var popover = new TestPopover ();
+
+        var popoverManager = new ApplicationPopover ();
+        popoverManager.Show (popover);
+    }
+}