Provide global setting to use ATS for nextTab and prevTab #7321

leonMSFT · 2020-08-17T23:02:10Z

Summary of the Pull Request

This PR splits the anchored and unanchored tab switcher into two. The anchored tab switcher is now baked into nextTab/prevTab, and the unanchored tab switcher command is just named tabSearch. tabSearch takes no arguments. To reflect this distinction, CommandPalette.cpp now refers to one as TabSwitchMode and the other as TabSearchMode.

I've added a global setting named useTabSwitcher (name up for debate) that makes the Terminal use the anchored tab switcher experience for nextTab and prevTab.

I've also given the control the ability to detect Alt KeyUp events and to dispatch keybinding events. By listening for keybindings, the ATS can react to nextTab/prevTab invocations for navigation in addition to listening for tab and the arrow keys.

Closes #7178

PR Checklist

CLA signed.
Documentation updates: Add keybinding docs for tabSwitcher MicrosoftDocs/terminal#107
Schema updated.

…tab switching

…horedMode

… mods

…pal the power to detect alts

carlos-zamora

Could you update the PR description with more details? All this stuff with "tabSearch" was throwing me off.

src/cascadia/TerminalApp/defaults.json

src/cascadia/TerminalControl/IControlSettings.idl

carlos-zamora · 2020-08-18T22:47:53Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -119,6 +119,7 @@ namespace winrt::TerminalApp::implementation
        if (auto page{ weakThis.get() })
        {
            _UpdateCommandsForPalette();
+            CommandPalette().SetKeyBindings(_settings->GetKeybindings());


(curious) why?

This isn't actually "set the bindings on the command palette", this is "attach the combined [key binding list and key binding key-receiver action dispatcher] to the palette" which is absolutely required for the palette to dispatch key binding events.

if we had a better separation of responsibilities here, we would be able to give command palette a key binding handler whose only bound actions were next/prev tab

(and then it could handle them itself). but right now our key binding command dispatch interface is an interface containing 100 functions it would need to implement and it is tied up with the actual deserializer code. heck.

Carlos, looks like you've made a couple comments below about the same keybindings topic so I'll move all the discussion here:

Yeah I honestly didn't know how to trim down the given bindings to just nextTab and prevTab 😢
I'm mostly hoping that because the control needs to be kept open with a modifier key, and there's no search bar and the only interactable interface is a list view, the range of actions a user would want to do is limited.

Like, if you've hit ctrl+tab to open the tab switcher, and for some odd reason you shift your fingers to invoke find which could be something like ctrl+shift+f, it'll show the find dialog and leave the tab switcher open. But IMO that's pretty inconvenient considering the whole time you'd have to hold down ctrl and shift your hand/fingers. Also, If you're holding down ctrl, you're saying you want the tab switcher to stay open while opening the find dialog?

But, I guess, then again, the whole "bad faith actor" is a valid reason to rip this out and just not allow users to use their nT/pT keybinding to navigate.

Downloaded the branch and tested it. Yeah, this works fine. And scenarios like "opening the search box while the tab switcher is open" would probably fall under the "play stupid games" category. Fixing this is probably more effort than it's worth right now, so good enough for me!

Also, I want this for "dispatch a keybinding while the palette is open". Like, if the user opens the palette, sees the keybinding for "new tab" is ctrl+shift+t, they should be able to just press that and have it do the right thing, you know?

src/cascadia/TerminalApp/ActionAndArgs.cpp

src/cascadia/TerminalApp/AppActionHandlers.cpp

src/cascadia/TerminalApp/CommandPalette.idl

src/cascadia/TerminalApp/CommandPalette.cpp

leonMSFT · 2020-08-18T23:35:55Z

My bad 😅 the PR description was looking very bare.

DHowett

this seems way cleaner (even though it added more direct manipulation from the page to the switcher control). interested to see how @zadjii-msft feels about it

src/cascadia/TerminalApp/AppActionHandlers.cpp

DHowett · 2020-08-19T20:51:32Z

src/cascadia/TerminalApp/AppLogic.cpp

+                    // Use the VisualTreeHelper's GetParent as a fallback.
+                    if (!focusedObject)
+                    {
+                        focusedObject = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(focusedElement);


i'm surprised this mostly just worked -- binding the direct handler, that is :)

Co-authored-by: Dustin L. Howett <[email protected]>

DHowett · 2020-08-20T01:14:57Z

i'm blocking on @zadjii-msft having a closer look, but I am not horribly worried about anything in here

zadjii-msft

At first from reading the description, I thought I'd hate this. But after playing with it, I really liked how it felt, and I especially love how much simpler a lot of the code is. I'm not blocking this because overall I think this is good. I just have like the tiniest nits, but you need to fix the merge conflicts anyways 😄

zadjii-msft · 2020-08-20T12:22:21Z

doc/cascadia/profiles.schema.json

@@ -68,7 +68,7 @@
        "wt",
        "closeOtherTabs",
        "closeTabsAfter",
-        "tabSwitcher",


useTabSwitcher probably needs to get added to this file too

zadjii-msft · 2020-08-20T12:24:48Z

src/cascadia/TerminalApp/CommandPalette.cpp

@@ -7,6 +7,7 @@
 #include "ActionArgs.h"
 #include "Command.h"

+#include <WinUser.h>


Did we need this just for VK_MENU? IMO we might as well include it in the pch, since I doubt this is the last time we'll need it

Yeah just for VK_MENU, I'll throw it in pch

zadjii-msft · 2020-08-20T12:27:30Z

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

@@ -576,6 +576,9 @@
  <data name="SetColorSchemeParentCommandName" xml:space="preserve">
    <value>Select color scheme...</value>
  </data>
+  <data name="TabSearchCommandKey" xml:space="preserve">
+    <value>Tab Search</value>


Do we want to make this a tad bit more... interactive? Perhaps something like "Search for tab..." (kinda like "Set the tab color..." - it's not technically a nested command, but it is a sequence of actions)

oh yeah "Search for tab..." looks pretty decent 👍

zadjii-msft · 2020-08-20T12:32:55Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -119,6 +119,7 @@ namespace winrt::TerminalApp::implementation
        if (auto page{ weakThis.get() })
        {
            _UpdateCommandsForPalette();
+            CommandPalette().SetKeyBindings(_settings->GetKeybindings());


Also, I want this for "dispatch a keybinding while the palette is open". Like, if the user opens the palette, sees the keybinding for "new tab" is ctrl+shift+t, they should be able to just press that and have it do the right thing, you know?

zadjii-msft

…aptain-now # Conflicts: # src/cascadia/TerminalApp/Resources/en-US/Resources.resw

ghost · 2020-08-21T15:02:00Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2020-08-26T17:04:45Z

🎉Windows Terminal Preview v1.3.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

leonMSFT added 11 commits August 12, 2020 17:11

for science - nt/pt hijacked, unanchored ats now tabSearch

b8e9b3b

change keyup to select on any modifier release

e4de51a

add global setting to swap between boring tab switching and exciting …

4fd1862

…tab switching

now that we don't have a designated anchor key, change the var to anc…

43241cf

…horedMode

go back to anchorKey, priority set anchorkey at appactionhandlers level

e15672e

detect modifiers pressed at handlenT/handlepT, do boring switch if no…

33a513c

… mods

let OnDirectKeyEvent find parents of ItemTemplated elements, give cmd…

9d188f0

…pal the power to detect alts

separate mode into tabswitch and tabsearch

c62bfb2

Merge branch 'master' into dev/lelian/im-the-captain-now

9e8d92a

cue don't let your dreams be dreams meme

433ec74

a bit more cleanup

3799be2

ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 17, 2020

leonMSFT changed the title ~~Provide global setting to use ATS for nextTab/prevTab~~ Provide global setting to use ATS for nextTab and prevTab Aug 17, 2020

leonMSFT added 3 commits August 17, 2020 16:06

schema updated

9306f0d

a whole lotta thinking resulting in just one line changed

c293055

i didn't realize i was in the spelling bee

b43342e

leonMSFT marked this pull request as ready for review August 18, 2020 22:32

leonMSFT assigned DHowett, carlos-zamora and zadjii-msft Aug 18, 2020

carlos-zamora mentioned this pull request Aug 18, 2020

Add keybinding docs for tabSwitcher MicrosoftDocs/terminal#107

Merged

carlos-zamora requested changes Aug 18, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 18, 2020

carlos-zamora removed their assignment Aug 18, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 18, 2020

leonMSFT added 2 commits August 19, 2020 11:18

removing setting from control

ebff42c

merge master

859544a

might as well move this here, it's a bit cleaner

c74f29c

leonMSFT assigned carlos-zamora Aug 19, 2020

DHowett requested a review from zadjii-msft August 19, 2020 20:50

DHowett reviewed Aug 19, 2020

View reviewed changes

value or

5f9a25f

Co-authored-by: Dustin L. Howett <[email protected]>

carlos-zamora approved these changes Aug 19, 2020

View reviewed changes

zadjii-msft reviewed Aug 20, 2020

View reviewed changes

carlos-zamora removed their assignment Aug 20, 2020

pr updates

1553354

cinnamon-msft mentioned this pull request Aug 20, 2020

Document 1.3 release features MicrosoftDocs/terminal#117

Closed

14 tasks

leonMSFT added 2 commits August 20, 2020 19:11

merge conflicts

e820bc5

missed this one

054219a

zadjii-msft approved these changes Aug 21, 2020

View reviewed changes

zadjii-msft removed their assignment Aug 21, 2020

Merge remote-tracking branch 'origin/master' into dev/lelian/im-the-c…

7f1adff

…aptain-now # Conflicts: # src/cascadia/TerminalApp/Resources/en-US/Resources.resw

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 21, 2020

DHowett approved these changes Aug 21, 2020

View reviewed changes

ghost merged commit 3d370dc into master Aug 21, 2020

ghost deleted the dev/lelian/im-the-captain-now branch August 21, 2020 15:39

ghost mentioned this pull request Aug 26, 2020

Allow the Tab Switcher to initialize going backwards #7178

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide global setting to use ATS for nextTab and prevTab #7321

Provide global setting to use ATS for nextTab and prevTab #7321

leonMSFT commented Aug 17, 2020 •

edited

Loading

carlos-zamora left a comment

carlos-zamora Aug 18, 2020

DHowett Aug 18, 2020

DHowett Aug 18, 2020

DHowett Aug 18, 2020

leonMSFT Aug 19, 2020

carlos-zamora Aug 19, 2020

zadjii-msft Aug 20, 2020

leonMSFT commented Aug 18, 2020

DHowett left a comment

DHowett Aug 19, 2020

DHowett Aug 19, 2020

DHowett commented Aug 20, 2020

zadjii-msft left a comment

zadjii-msft Aug 20, 2020

zadjii-msft Aug 20, 2020

leonMSFT Aug 20, 2020

zadjii-msft Aug 20, 2020

leonMSFT Aug 20, 2020

zadjii-msft Aug 20, 2020

zadjii-msft left a comment

ghost commented Aug 21, 2020

ghost commented Aug 26, 2020

Provide global setting to use ATS for nextTab and prevTab #7321

Provide global setting to use ATS for nextTab and prevTab #7321

Conversation

leonMSFT commented Aug 17, 2020 • edited Loading

Summary of the Pull Request

PR Checklist

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonMSFT commented Aug 18, 2020

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Aug 20, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Aug 21, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 26, 2020

leonMSFT commented Aug 17, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.