Add support for a right-click context menu by zadjii-msft · Pull Request #14775 · microsoft/terminal

zadjii-msft · 2023-02-02T23:42:45Z

Experimental for now. experimental.rightClickContextMenu, a per-profile setting. Long term we want to enable full mouse bindings, at which point this would be replaced.

Closes #3337

This adds two context menus to the TermControl - one for right-clicking with a selection, and one without. The implementation is designed to follows the API experience of the context menu on something like a RichEditBox. The hosting application adds a handler for the menu's Opening event, and appends whatever items it wants at that time.

So TermControl only implements a few "actions" by default - copy, past, find. TerminalApp is then responsible for adding anything else it needs. Right now, those actions are:

Duplicate tab
Duplicate pane
Close Tab
Close pane

Screenshots in #14775 (comment)

…way from the kid

…o friggin big.

…then released

…t-for-funsies

…leave

…ht place

zadjii-msft · 2023-02-03T11:24:19Z

The default context menu:

With a selection:

When there are multiple panes, "Close pane" gets added

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

src/cascadia/TerminalApp/TerminalPage.h

src/cascadia/TerminalApp/TerminalPage.cpp

carlos-zamora · 2023-02-03T17:12:54Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        // Only wire up "Close Pane" if there's multiple panes.
+        if (_GetFocusedTabImpl()->GetLeafPaneCount() > 1)
+        {
+            makeItem(RS_(L"ClosePaneContextMenuEntryText"), L"\xE89F", ActionAndArgs{ ShortcutAction::ClosePane, nullptr });
+        }


I feel like guidance is that we should always include this option, but disable it if it can't be used. That way, the user can always see what options are available.

Ironically, ClosePane is still valid though, so maybe we shouldn't even disable/remove it at all haha.

Curious what others think

My consideration was that when there's only one pane, closeTab == closePane. So putting closePane there, even if it's disabled, just adds noise. That's my thought. Furthermore, just doing closePane when there's only one pane might confuse people who don't get that there are panes (probably most people)

src/cascadia/TerminalControl/EventArgs.idl

src/cascadia/TerminalControl/TermControl.h

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalControl/TermControl.xaml

src/inc/til/winrt.h

This reverts commit 520c89f.

… hours away from the kid" This reverts commit b535a5f.

This reverts commit eb88cd3.

This reverts commit 7001e93.

This reverts commit eaf1a92.

zadjii-msft · 2023-03-03T22:16:11Z

up next, once this and #14807 merge:

dev/migrie/f/3337-just-for-funsies...dev/migrie/f/rclick-select-command

DHowett

partway through (11/24) but I've got to go for now

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2023-03-07T23:44:05Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        makeItem(RS_(L"SplitPaneText"), L"\xF246", ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Duplicate } });
+        makeItem(RS_(L"DuplicateTabText"), L"\xF5ED", ActionAndArgs{ ShortcutAction::DuplicateTab, nullptr });
+
+        // Only wire up "Close Pane" if there's multiple panes.


Design q: should we display this but leave it disabled?

Given past conversations on the repo, I reckon that most users don't even know what a Pane is, so that might be confusing. I erred on the side of "display the fewest possible valid options".

I am open to being overruled, idgaf.

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2023-03-07T23:46:27Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

+            ctrlEnabled &&
+            !hyperlink.empty())


nit: change req'd?

src/cascadia/TerminalControl/ControlInteractivity.cpp

DHowett · 2023-03-07T23:51:37Z

src/cascadia/TerminalControl/TermControl.cpp

+    {
+        // formats = nullptr -> copy all formats
+        _interactivity.CopySelectionToClipboard(false, nullptr);
+        ContextMenu().Hide();


If somebody else provides the implementation of the command handler, how do THEY get to hide the menu?

I suppose makeCallback could have called Hide itself. I guess I never hit that in selfhosting. Interesting...

…t-for-funsies

src/cascadia/TerminalSettingsModel/ActionArgs.idl

DHowett

I believe your build failure is due to TerminalControl rolling up a duplicate copy of MUX (if i had to guess). Otherwise... looks good.

…t-for-funsies

carlos-zamora

Approving, but not adding "Automerge" in case you want to go in and resolve my suggestions.

carlos-zamora · 2023-03-17T17:47:56Z

src/cascadia/TerminalControl/EventArgs.idl


+    runtimeclass ContextMenuRequestedEventArgs
+    {
+        Windows.Foundation.Point Point { get; };


this is such a nit, but I don't like that it's a Point Point. Maybe rename it to Position?

carlos-zamora · 2023-03-17T17:53:47Z

src/cascadia/TerminalControl/TermControl.cpp

+                control->ContextMenu().PrimaryCommands().Clear();
+                control->ContextMenu().SecondaryCommands().Clear();
+                for (const auto& e : control->_originalPrimaryElements)
+                {
+                    control->ContextMenu().PrimaryCommands().Append(e);
+                }
+                for (const auto& e : control->_originalSecondaryElements)
+                {
+                    control->ContextMenu().SecondaryCommands().Append(e);
+                }


Suggested change

control->ContextMenu().PrimaryCommands().Clear();

control->ContextMenu().SecondaryCommands().Clear();

for (const auto& e : control->_originalPrimaryElements)

{

control->ContextMenu().PrimaryCommands().Append(e);

}

for (const auto& e : control->_originalSecondaryElements)

{

control->ContextMenu().SecondaryCommands().Append(e);

}

auto& menu = control->ContextMenu();

menu.PrimaryCommands().Clear();

menu.SecondaryCommands().Clear();

for (const auto& e : control->_originalPrimaryElements)

{

menu.PrimaryCommands().Append(e);

}

for (const auto& e : control->_originalSecondaryElements)

{

menu.SecondaryCommands().Append(e);

}

carlos-zamora · 2023-03-17T17:54:31Z

src/cascadia/TerminalControl/TermControl.cpp

+                control->SelectionContextMenu().PrimaryCommands().Clear();
+                control->SelectionContextMenu().SecondaryCommands().Clear();
+                for (const auto& e : control->_originalSelectedPrimaryElements)
+                {
+                    control->SelectionContextMenu().PrimaryCommands().Append(e);
+                }
+                for (const auto& e : control->_originalSelectedSecondaryElements)
+                {
+                    control->SelectionContextMenu().SecondaryCommands().Append(e);
+                }


Suggested change

control->SelectionContextMenu().PrimaryCommands().Clear();

control->SelectionContextMenu().SecondaryCommands().Clear();

for (const auto& e : control->_originalSelectedPrimaryElements)

{

control->SelectionContextMenu().PrimaryCommands().Append(e);

}

for (const auto& e : control->_originalSelectedSecondaryElements)

{

control->SelectionContextMenu().SecondaryCommands().Append(e);

}

auto& menu = control->SelectionContextMenu();

menu.PrimaryCommands().Clear();

menu.SecondaryCommands().Clear();

for (const auto& e : control->_originalSelectedPrimaryElements)

{

menu.PrimaryCommands().Append(e);

}

for (const auto& e : control->_originalSelectedSecondaryElements)

{

menu.SecondaryCommands().Append(e);

}

carlos-zamora · 2023-03-17T17:55:52Z

src/cascadia/TerminalControl/TermControl.cpp

+        _useRightClickContextMenu = settings.RightClickContextMenu();
+


Do we use _useRightClickContextMenu anywhere? Why can't we call _settings.RightClickContextMenu() when we need it?

Adds ``` { "command": "showContextMenu", "keys": "menu" }, ``` as a default action. This will manually invoke the control context menu (from #14775), even with the setting disabled. As discussed with Dustin.

EduardDurech · 2023-11-18T21:05:14Z

Would it be possible to allow the "See less" and "See more" to be persistent? At the moment, when I click "See less" it only stays while active, consecutive right-clicks show the expanded context menu

zadjii-msft and others added 20 commits July 23, 2022 11:58

okay maybe I have silly free time ideas too

eaf1a92

forwarded_event, string tests

7001e93

practical use, look

eb88cd3

I suppose this is what happens when I have four uninterrupted hours a…

b535a5f

…way from the kid

sad beeps

520c89f

I am incapable of not making 100 tiny commits, this was already way t…

eb7d2be

…o friggin big.

yea that's everything

e6b33c3

90% plumbing, 10% bad implementation of making sure we right-clicked …

29d454b

…then released

move this code to interactivity. Scaling of event is wrong now

451d8f3

Merge remote-tracking branch 'origin/main' into dev/migrie/f/3337-jus…

99b84a6

…t-for-funsies

use resource strings too. Last commit of stuff from before paternity …

dc2ebaf

…leave

Migrate spelling-0.0.21 changes from main

61a9c80

Merge branch 'main' into dev/migrie/f/3337-just-for-funsies

191bca8

a commandbarflyout intsead, with a slightly more idomatic implementation

6a60053

open at the mouse

2dd01fb

adapt the previous action adder for the new menu

2ee4111

cleanup; don't duplicate selection entries each time; open in the rig…

854335b

…ht place

icons, and dismiss the menu when the action's done

0328b4f

cleanup for review

08ae0ec

'cleanup

d6fe520

This comment has been minimized.

Sign in to view

bommand

7aef3af

carlos-zamora reviewed Feb 3, 2023

View reviewed changes

zadjii-msft added 6 commits February 5, 2023 15:29

PR nits

c77f959

Revert "sad beeps"

ab55a8d

This reverts commit 520c89f.

Revert "I suppose this is what happens when I have four uninterrupted…

559ecdb

… hours away from the kid" This reverts commit b535a5f.

Revert "practical use, look"

9ff0abe

This reverts commit eb88cd3.

Revert "forwarded_event, string tests"

564102b

This reverts commit 7001e93.

Revert "okay maybe I have silly free time ideas too"

a1d5329

This reverts commit eaf1a92.

zadjii-msft added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Feb 5, 2023

zadjii-msft mentioned this pull request Feb 13, 2023

Feature Request: Mouse/Touch/Pointer Bindings (like middle-click paste, right-click context menu, etc.) #1553

Open

14 tasks

Merge branch 'main' into dev/migrie/f/3337-just-for-funsies

d01cab3

zadjii-msft assigned carlos-zamora Feb 14, 2023

DHowett reviewed Mar 8, 2023

View reviewed changes

zadjii-msft added 2 commits March 8, 2023 11:32

Merge remote-tracking branch 'origin/main' into dev/migrie/f/3337-jus…

33b3eac

…t-for-funsies

the easy nits

daddcf1

zadjii-msft added a commit that referenced this pull request Mar 11, 2023

PRE-MERGE #14775 Add support for a right-click context menu

cedfd3f

Merge remote-tracking branch 'origin/main' into dev/migrie/f/3337-jus…

4dcc1a3

…t-for-funsies

DHowett reviewed Mar 16, 2023

View reviewed changes

src/cascadia/TerminalSettingsModel/ActionArgs.idl Outdated Show resolved Hide resolved

Use MUX in TermControl for fun and profit and faster menu opening

6674df7

DHowett approved these changes Mar 16, 2023

View reviewed changes

UT are important too

35455f1

zadjii-msft assigned carlos-zamora and unassigned carlos-zamora Mar 17, 2023

zadjii-msft requested a review from carlos-zamora March 17, 2023 12:06

zadjii-msft added 2 commits March 17, 2023 09:39

Merge branch 'main' into dev/migrie/f/3337-just-for-funsies

263786c

Merge remote-tracking branch 'origin/main' into dev/migrie/f/3337-jus…

1a29af2

…t-for-funsies

carlos-zamora approved these changes Mar 17, 2023

View reviewed changes

carlos-zamora removed their assignment Mar 17, 2023

yea these are reasonable nits

bede33c

DHowett merged commit 7383b26 into main Mar 17, 2023

DHowett deleted the dev/migrie/f/3337-just-for-funsies branch March 17, 2023 18:54

This was referenced Apr 26, 2023

Added Close Pane to Context Menu #15198

Merged

Add an action for manually invoking the control context menu #15254

Merged

zadjii-msft mentioned this pull request Jul 20, 2023

Limit triple-click selection to command field only #9881

Open

		_useRightClickContextMenu = settings.RightClickContextMenu();

Conversation

zadjii-msft commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

zadjii-msft commented Feb 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft commented Mar 3, 2023

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EduardDurech commented Nov 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zadjii-msft commented Feb 2, 2023 •

edited

Loading