Hook up the keybindings to the SUI, redux #10121

zadjii-msft · 2021-05-18T15:55:17Z

Summary of the Pull Request

This is a redux of #8882.

From the original:

This is really similar to what we're doing with the CommandPalette. We're adding a ~~Preview~~KeyDown handler to the SUI MainPage, that connects to TerminalPage::_HandleKey. That allows the SUI a chance to search the keymap to dispatch actions for keybindings, similar to how the command palette does it.

This also means it's now possible for the SUI to invoke all the actions available to the Terminal. This includes the ones like IncreaseFontSize, which require a Terminal to actually do something. So we have to make sure all the calls to _GetActiveControl actually check that the result is non-null before using it.

A bunch of the actions do nothing now from a SUI tab, others behave weird. Like "Rename tab" / "Open Tab Renamer" do nothing. "Duplicate Tab" again does nothing - we try making a new settings tab, which just focuses the settings tab again. "Copy text" definitely does nothing, same with paste.

I don't know why I thought this wouldn't work. I thought we'd have to do this in PreviewKeyDown or something, which led to weirdness. Turns out, we don't need it to be in PreviewKeyDown. It can just be in the SUI's KeyDown.

References

Original: Hook up the keybindings to the SUI #8882
Workaround was in Teach SUI tab to respect close / prev / next tab bindings #8885

PR Checklist

Closes Make SettingsTab respect keybindings #8767
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

The special case handler from #8885 is no longer needed

Validation Steps Performed

Switching tabs with Ctrl+Tab works
Command palette works
fullscreen, focus mode works
close window works
copy paste on Ctrl+C/V works, even when bound
Select all text in textboxes works
tab navigation through UI elements works

DHowett · 2021-05-19T22:22:39Z

This is in selfhost today.

DHowett

huh, I can't remember why we didn't do this. I bet we'll find out and it will be catastrophic. 😄

DHowett · 2021-05-24T16:59:55Z

@carlos-zamora can you circle up on this?

carlos-zamora

LOVE IT! Tested it locally and it works well. Specifically kept an eye out for...

does "ctrl+c/v/a" still go through to the textbox when it's bound?

The only complaint I have is that the command palette is filled with some actions that don't do anything (i.e. "split pane"). It'd be nice to filter out the ones that don't do anything, but that's definitely complicated enough to be a follow-up. Mind filing that?

carlos-zamora · 2021-05-24T18:09:15Z

src/cascadia/TerminalApp/TerminalPage.cpp

-            sui.PreviewKeyDown({ this, &TerminalPage::_SUIPreviewKeyDownHandler });
+            // GH#8767 - let unhandled keys in the SUI try to run commands too.
+            sui.KeyDown({ this, &TerminalPage::_KeyDownHandler });


FYI: this seems like it was the only difference from #8882. We're doing a sui.KeyDown instead of a sui.PreviewKeyDown. That's probably why key chords like ctrl+c were being eaten by us instead of XAML controls (like text boxes).

ghost · 2021-05-24T19:18:29Z

Hello @DHowett!

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.

zadjii-msft · 2021-05-24T19:21:07Z

does "ctrl+c/v/a" still go through to the textbox when it's bound?

yeppers

It'd be nice to filter out the ones that don't do anything, but that's definitely complicated enough to be a follow-up. Mind filing that?

-> #10169

DHowett · 2021-05-24T22:06:41Z

There's enough user DSAT around this that I am backporting it into 1.8. We'll see how that goes!

## Summary of the Pull Request This is a redux of #8882. From the original: > This is really similar to what we're doing with the `CommandPalette`. We're adding a ~~Preview~~`KeyDown` handler to the SUI `MainPage`, that connects to `TerminalPage::_HandleKey`. That allows the SUI a chance to search the keymap to dispatch actions for keybindings, similar to how the command palette does it. > > This also means it's now possible for the SUI to invoke _all_ the actions available to the Terminal. This includes the ones like `IncreaseFontSize`, which require a _Terminal_ to actually do something. So we have to make sure all the calls to `_GetActiveControl` actually check that the result is non-null before using it. > > A bunch of the actions do nothing now from a SUI tab, others behave _weird_. Like "Rename tab" / "Open Tab Renamer" do nothing. "Duplicate Tab" again does nothing - we try making a new settings tab, which just focuses the settings tab again. "Copy text" definitely does nothing, same with paste. I don't know why I thought this wouldn't work. I thought we'd have to do this in `PreviewKeyDown` or something, which led to [weirdness](#8882 (comment)). Turns out, we don't need it to be in `PreviewKeyDown`. It can just be in the SUI's `KeyDown`. ## References * Original: #8882 * Workaround was in #8885 ## PR Checklist * [x] Closes #8767 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments The special case handler from #8885 is no longer needed ## Validation Steps Performed * Switching tabs with Ctrl+Tab works * Command palette works * fullscreen, focus mode works * close window works * copy paste on Ctrl+C/V works, even when bound * Select all text in textboxes works * tab navigation through UI elements works (cherry picked from commit 52560ff) # Conflicts: # src/cascadia/TerminalApp/TerminalPage.cpp # src/cascadia/TerminalApp/TerminalPage.h

ghost · 2021-05-25T19:04:30Z

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

Handy links:

ghost · 2021-05-25T19:07:48Z

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

Handy links:

tabedzki · 2021-05-26T00:21:16Z

Thanks @DHowett and @zadjii-msft for working on this issue and fixing it. It works on the download from the msft store.
Just curious, what tools did you guys use to generate your avatar? Changing from the default github icon is a good idea

zadjii-msft · 2021-05-26T10:55:46Z

I uh, drew it in paint.net IIRC 😆. Either paint.net or some other sprite editor that I've long since forgotten

DHowett · 2021-05-26T16:00:00Z

@tabedzki I made mine at "FaceYourManga", which is pretty much an RPG character builder but for tiny avatar images

zadjii-msft added 4 commits May 18, 2021 06:22

That was suspiciously easy

f6898bc

take one down, pass it around, 137 bugs on the wall

c27b17c

Huh, this works shockingly well

aebff36

remove dead code

958d115

ghost added Area-Settings UI Anything specific to the SUI Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels May 18, 2021

DHowett approved these changes May 20, 2021

View reviewed changes

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label May 21, 2021

ghost requested review from miniksa, carlos-zamora, leonMSFT, PankajBhojwani and lhecker May 21, 2021 16:12

carlos-zamora approved these changes May 24, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 24, 2021

ghost merged commit 52560ff into main May 24, 2021

ghost deleted the dev/migrie/8767-take-two branch May 24, 2021 19:18

ghost mentioned this pull request May 25, 2021

Make SettingsTab respect keybindings #8767

Closed

zadjii-msft mentioned this pull request Mar 20, 2023

Entering the Settings menu in fullscreen can become stuck due to disabled keybinds #14051

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hook up the keybindings to the SUI, redux #10121

Hook up the keybindings to the SUI, redux #10121

zadjii-msft commented May 18, 2021

DHowett commented May 19, 2021

DHowett left a comment

DHowett commented May 24, 2021

carlos-zamora left a comment

carlos-zamora May 24, 2021

ghost commented May 24, 2021

zadjii-msft commented May 24, 2021

DHowett commented May 24, 2021

ghost commented May 25, 2021

ghost commented May 25, 2021

tabedzki commented May 26, 2021

zadjii-msft commented May 26, 2021

DHowett commented May 26, 2021

Hook up the keybindings to the SUI, redux #10121

Hook up the keybindings to the SUI, redux #10121

Conversation

zadjii-msft commented May 18, 2021

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett commented May 19, 2021

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented May 24, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora May 24, 2021

Choose a reason for hiding this comment

ghost commented May 24, 2021

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.

zadjii-msft commented May 24, 2021

DHowett commented May 24, 2021

ghost commented May 25, 2021

ghost commented May 25, 2021

tabedzki commented May 26, 2021

zadjii-msft commented May 26, 2021

DHowett commented May 26, 2021

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.