Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

Fixes #3691. Adds `Popover`

Open tig opened this issue 1 year ago • 13 comments

Fixes

  • Fixes #3691

fykztjo 1

fImBrr3 1

  • Fixes #3757

Proposed Changes/Todos

  • [x] Prototype in View Experiments the concept described in #3691 to prove concept.
  • [x] Prototype in Bars to show how ContextMenu and MenuBar could utilize
  • [ ] Re-implement ContextMenu using MenuV2 - Initially no cascading menus for simplicity
  • [ ] Re-implement AutocompletePopup
  • [ ] Re-implement Combobox
  • [ ] Add more ShadowView unit tests (see #3757)

Future PRs

  • [ ] Replace Menu and MenuBar using Bar (aka finish Menuv2

Pull Request checklist:

  • [x] I've named my PR in the form of "Fixes #issue. Terse description."
  • [x] My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • [x] My code follows the Terminal.Gui library design guidelines
  • [x] I ran dotnet test before commit
  • [x] I have made corresponding changes to the API documentation (using /// style comments)
  • [x] My changes generate no new warnings
  • [x] I have checked my code and corrected any poor grammar or misspellings
  • [ ] I conducted basic QA to assure all features are working

tig avatar Sep 19 '24 22:09 tig

FWIW, here's the API in action (the prototype for the File menubar item in Bars):

var fileMenuBarItem = new Shortcut
{
    Title = "_File",
    HelpText = "File Menu",
    Key = Key.D0.WithAlt,
};

fileMenuBarItem.Accept += (sender, args) =>
                          {
                              var fileMenu = new Menuv2
                              {
                                  Id = "fileMenu",
                              };
                              ConfigureMenu (fileMenu);
                              fileMenu.VisibleChanged += (sender, args) =>
                                                         {
                                                             if (Application.Popover == _popoverMenu && Application.Popover is { Visible: false })
                                                             {
                                                                 Application.Popover?.Dispose ();
                                                                 Application.Popover = null;
                                                             }
                                                         };
                              Application.Popover = fileMenu;
                              Rectangle screen = fileMenuBarItem.FrameToScreen ();
                              fileMenu.X = screen.X;
                              fileMenu.Y = screen.Y + screen.Height;
                              fileMenu.Visible = true;

                          };

Here's the entirety of the new API (on Application):

    /// <summary>Gets or sets the Application Popover View.</summary>
    /// <remarks>
    ///     <para>
    ///         To show or hide the Popover, set it's <see cref="View.Visible"/> property.
    ///     </para>
    /// </remarks>
    public static View? Popover

tig avatar Sep 19 '24 22:09 tig

ZtNvfaH 1

tig avatar Sep 23 '24 21:09 tig

Kinda cool. Mouse wheel will now be able to nav within a Bar (or Menu).

AKE5pyM 1

tig avatar Sep 23 '24 21:09 tig

n4wBc8q 1

A new ContextMenuv2 implementation.

TextField now uses it instead of the old one.

tig avatar Sep 26 '24 21:09 tig

A new ContextMenuv2 implementation.

TextField now uses it instead of the old one.

But the start position isn't the same as the old one. Please pay attention to that detail.

BDisp avatar Sep 26 '24 22:09 BDisp

Will do. Just work in progress for now.

tig avatar Sep 26 '24 23:09 tig

A new ContextMenuv2 implementation. TextField now uses it instead of the old one.

But the start position isn't the same as the old one. Please pay attention to that detail.

@BDisp Can you explain the logic you used for the old design?

I implemented what's in this PR without actually looking at the old behavior. I implemented what I thought made sense. Here's my thinking, for TextField:

  • The ContextMenu should be 1 row below or above the TextView. Below if it will fit below. Above if it will not fit below.
    • If it won't fit either, then it can appear over the TextView.
  • The left side of the ContextMenu should line up with the cursor position in the TextField. This makes sense because the actions like Paste impact the cursor position. If the menu won't fit such that this works, it will be positioned such that it fits.

I now see, the old behavior didn't work like this. The old behavior for TextView is:

  • The ContextMenu should be 1 row below or above the TextView. Below if it will fit below. Above if it will not fit below.
    • If it won't fit either, then it can appear over the TextView.
  • The left side of the ContextMenu should line up with left side of TextField. If there's not room it can be positioned so it fits.

For an arbirary View, with the mouse, in this PR:

  • The top-left corner of the ContextMenu appears where the mouse is, unless it can't fit on the screen. Then it will appear such that it is fully visible.

The old behavior:

  • The first cell of the first menuitem is positioned where the mouse is.

I think the old behavior in this case is correct and will change this PR to match that.

What are your thoughts on the above?

tig avatar Sep 27 '24 02:09 tig

The view that use ContextMenu can customize his position if he want. The TextField has no sense to start his position at cursor position, TextView has. TextField must start his position at (0,1), but never over it unless it doesn't fit. If doesn't fit below put it above the top and not over it, unless no screen space is available. In TextView always put it in the line below at cursor position if it fits, otherwise put it above the line where the cursor is positioned, but never over it, unless no screen space is available.

BDisp avatar Sep 27 '24 07:09 BDisp

The TextField has no sense to start his position at cursor position, TextView has. TextField must start his position at (0,1), but never over it unless it doesn't fit.

I don't understand.

TextView does know the cursor pos. I've used it to position the CM in this PR. I'm travelling today so can't send a video. Try it and let me know what you think.

tig avatar Sep 27 '24 14:09 tig

Nothing of this is working in your PR. You put a zig zag menus for what? To show that this will stopping working? UseSubMenusSingleFrame will also not working in the new Popover? To me a ContextMenu must work like a Menu but without a visible MenuBar. Popover should be used for floated view that will show content on front of others views. But that only my opinion.

image

BDisp avatar Sep 27 '24 18:09 BDisp

In this PR:

WindowsTerminal_sfT355oQXG

In v2_develop the MenuBar is not showing.

WindowsTerminal_TFdFrY5i9k

BDisp avatar Sep 27 '24 19:09 BDisp

Nothing of this is working in your PR. You put a zig zag menus for what? To show that this will stopping working? UseSubMenusSingleFrame will also not working in the new Popover? To me a ContextMenu must work like a Menu but without a visible MenuBar. Popover should be used for floated view that will show content on front of others views. But that only my opinion.

image

This is a WIP. I will add the more advanced features as I progress.

tig avatar Sep 27 '24 19:09 tig

In this PR:

WindowsTerminal_sfT355oQXG

In v2_develop the MenuBar is not showing.

WindowsTerminal_TFdFrY5i9k

This is because Window is set for Overlapped/Movable/Sizable.

Please file a bug for me to make it so arrange mode is disabled if Top.

tig avatar Sep 27 '24 19:09 tig

Here's how this PR current works:

There's a new property on Application:

    /// <summary>Gets or sets the Application Popover View.</summary>
    /// <remarks>
    ///     <para>
    ///         To show or hide the Popover, set it's <see cref="View.Visible"/> property.
    ///     </para>
    /// </remarks>
    public static View? Popover

When set, if there's an Popover it's made Visible = false. Then the new View is set as the popover, initialized if needed.

A handler for _popover.VisibleChanged is added which does the heavy lifting.

To show a Popover, e.g. on a right mouse click, simply :

  • Set your View's X/Y to the Screen-relative coords you want
  • Set Application.Popover to your View
  • Set yourView.Visible = true
        void TestFrameOnMouseClick (object sender, MouseEventArgs e)
        {
            if (e.Flags == MouseFlags.Button3Clicked)
            {
                popoverView.X = e.ScreenPosition.X;
                popoverView.Y = e.ScreenPosition.Y;
                Application.Popover = popoverView;
                Application.Popover!.Visible = true;
            }
        }

The View will remain visible and will prevent any other view from getting mouse/keyboard input until

  • The user clicks outside of the popover's fame
  • The user presses QuitKey
  • Someone sets Applicaiton.Popover.Visible = false
  • Someone sets a new View to Application.Popover

This works fairly brilliantly for:

Standalone use-cases as shown in ViewExperiments.cs

06cV1tE 1 S

ContextMenu (hacked to use Menuv2/Bar - No sub-menus

Q7F4sIY 1

Menuv (Prototype)

TXHKxsX 1

Problems

  • It's really not a good design to couple more View logic in Application. I would really prefer that Application just knows about a single Top. This design actually works against what I'm trying to do with

    • #2491
  • MenuBar scenarios require the MenuBar, which is not a Popover, to still receive input when a sub-menu is shown as the Popo er. All modern menu systems, esp on Windows, let you hover over the menu bar once activated and sub-menus are automatcially shown/hidden as you do so:

Ifu2n1N 1

qy3du96 1

With this design, there's no clean way to make this happen. Here's what it looks like now (I have to click on a menu bar item to activate it's submenu): NfYHxD5 1

There's also no good way, with this design, to enable sub-menus.

I've spent a bunch of time noodling on this particular issue and tried several ideas, all of which seem like hacks and just cause more issues:

  • Transform this._menuBar into a Popover dynamically (e.g.. clone it and set the clone to Application.Popover).
  • Make Application.Popover be Stack<View> Popovers in conjunction with the clone idea.

So, for this reason, for now, I'm going to abandon this PR, fork the branch, and create a new PR that will try a different approach:

  • Add ViewArrangement.Popover
  • Any view with this flag set, will be "homed" from a layout/mouse/keyboard input perspective as though they were subviews of Application.Top

tig avatar Nov 25 '24 16:11 tig