Hook up the keybindings to the SUI #8882

zadjii-msft · 2021-01-25T18:54:21Z

Summary of the Pull Request

This is really similar to what we're doing with the CommandPalette. We're adding a PreviewKeyDown 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.

PR Checklist

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

Validation Steps Performed

I performed all the actions that I changed. I did not test all actions exhaustively, because I felt that searching for callers of GetActiveTerminalControl() was exhaustive enough.

DHowett · 2021-01-25T19:35:54Z

"Copy text" definitely does nothing, same with paste.

Can people copy/paste in the text fields inside the SUI still?

DHowett · 2021-01-25T19:36:21Z

I wonder why we didn't put key binding handling in a Preview handler for Page

Don-Vito · 2021-01-25T19:36:55Z

I wonder why we didn't put key binding handling in a Preview handler for Page

Same here 😄
But actually we cannot - it will steal the navigation from palette.

Don-Vito · 2021-01-25T19:45:49Z

Guys do you have any idea, why KeyDown doesn't get propagated to the TerminalPage? This could be much safer.

DHowett · 2021-01-25T19:47:44Z

KeyDown should almost never get to the TerminalPage; it is "bubbled" up from the deepest layer to the outermost, so once TermControl marks the key event as handled (which it should do, because it sends every non-bound key to the connected process) it's done.

Don-Vito · 2021-01-25T19:49:05Z

KeyDown should almost never get to the TerminalPage; it is "bubbled" up from the deepest layer to the outermost, so once TermControl marks the key event as handled (which it should do, because it sends every non-bound key to the connected process) it's done.

This one I understand 😄
I am asking about the SUI.

DHowett · 2021-01-25T19:49:22Z

Oh. Hm. Great question 😄

carlos-zamora

This is remarkably straightforward. There's a ~~few cases~~ case I'm curious about though before signing off:

does "ctrl+c/v/a" still go through to the textbox when it's bound?
~~do "tab", "ctrl+tab", and "arrow keys" still go through through when bound?~~ "play stupid games, win stupid prizes" case

Can we also get a gif of you opening/interacting with the Command Palette in SUI? Aside from it being cool, just want to double check that CmdPlt eats key strokes like tab, text input, etc. properly.

zadjii-msft · 2021-01-25T20:21:11Z

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

Ah butts. It does not. The TerminalPage eats it before the text box has a chance at it.

We could manually exempt Ctrl+C, Ctrl+V, Tab, Backspace, but that's starting to feel like the wrong solution.

DHowett · 2021-01-25T20:22:40Z

I thought that it returned false in non-terminal cases, indicating that the event should continue propagating. Hmm.

zadjii-msft · 2021-01-25T20:32:00Z

Hmm. Binding MainPage's KeyDown as opposed to PreviewKeyDown seems to work the way we'd expect. Perhaps that lets the TextBox do it's own thing in PKD, and when nothing else has claimed the event, it gets another chance in KeyDown?

zadjii-msft · 2021-01-25T20:36:09Z

Guh ignore me. That does not always work as expected, especially with Tab. More often than not, the focus will move within the page, rather than bubble up to the _KeyDownHandler to give us a chance at the action. So that's not what we want 😕

Don-Vito · 2021-01-25T20:45:08Z

Guh ignore me. That does not always work as expected, especially with Tab. More often than not, the focus will move within the page, rather than bubble up to the _KeyDownHandler to give us a chance at the action. So that's not what we want 😕

I've written this 5 days ago in the ticket 😛
What about handling switch tab and close tab by now (via PreviewKeyHandler) until we understand what swallows the events?

zadjii-msft · 2021-01-25T22:05:50Z

I've written this 5 days ago in the ticket 😛

Yea you're not wrong. I thought I could solve it quick without reading 🤦‍♂️

As a team, we thought maybe the best way to go about this would be to do just the tab switching solution for the timeframe of 1.6, and come back to this discussion for 1.7. @Don-Vito You still cool spinning up that fix?

Don-Vito · 2021-01-25T22:07:31Z

I've written this 5 days ago in the ticket 😛

Yea you're not wrong. I thought I could solve it quick without reading 🤦‍♂️

As a team, we thought maybe the best way to go about this would be to do just the tab switching solution for the timeframe of 1.6, and come back to this discussion for 1.7. @Don-Vito You still cool spinning up that fix?

@zadjii-msft - waited to make sure you are not working on this. Can try to do it now. I hope it won't take too long.

zadjii-msft · 2021-01-25T22:10:59Z

@Don-Vito You're the best, thanks 😄

Don-Vito · 2021-01-25T22:24:22Z

@zadjii-msft, @carlos-zamora - please see #8885

zadjii-msft · 2021-01-25T22:46:22Z

Frankly, I don't think this is needed with #8885, even in 1.7. I bet what we have will be good enough, now that I've seen it.

A temporary solution for #8882

This is an extension of #8885. A lot of users have grown accustomed to using `closePane` to close a tab. This adds `closePane` to the list of keybindings accepted by #8885, and modifies the `closePane` code to close the Settings UI if we are in a `SettingsTab`. ## References #6800: Settings UI Epic #8885: PR - Settings UI should respect key bindings (temporary solution) #8882: Issue - Settings UI should respect key bindings

…8885) A temporary solution for microsoft#8882

This is an extension of microsoft#8885. A lot of users have grown accustomed to using `closePane` to close a tab. This adds `closePane` to the list of keybindings accepted by microsoft#8885, and modifies the `closePane` code to close the Settings UI if we are in a `SettingsTab`. ## References microsoft#6800: Settings UI Epic microsoft#8885: PR - Settings UI should respect key bindings (temporary solution) microsoft#8882: Issue - Settings UI should respect key bindings

## 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

## 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

zadjii-msft added 2 commits January 25, 2021 12:26

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

6a8b670

That was suspiciously easy

52884d5

zadjii-msft requested review from DHowett and carlos-zamora January 25, 2021 18:54

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 Jan 25, 2021

Don-Vito approved these changes Jan 25, 2021

View reviewed changes

carlos-zamora reviewed Jan 25, 2021

View reviewed changes

Don-Vito mentioned this pull request Jan 25, 2021

Teach SUI tab to respect close / prev / next tab bindings #8885

Merged

zadjii-msft closed this Jan 25, 2021

DHowett pushed a commit that referenced this pull request Jan 25, 2021

Teach SUI tab to respect close / prev / next tab bindings (#8885)

65269c8

A temporary solution for #8882

carlos-zamora mentioned this pull request Jan 26, 2021

Allow ClosePane to close the Settings UI #8903

Merged

33b5e5 mentioned this pull request Jan 26, 2021

Can't exit new SUI with Focus mode enabled #8905

Closed

mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021

Teach SUI tab to respect close / prev / next tab bindings (microsoft#…

e43bac0

…8885) A temporary solution for microsoft#8882

zadjii-msft mentioned this pull request May 18, 2021

Hook up the keybindings to the SUI, redux #10121

Merged

3 tasks

DHowett deleted the dev/migrie/b/8767-sui-keybindings branch October 26, 2021 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hook up the keybindings to the SUI #8882

Hook up the keybindings to the SUI #8882

zadjii-msft commented Jan 25, 2021 •

edited by DHowett

Loading

DHowett commented Jan 25, 2021

DHowett commented Jan 25, 2021

Don-Vito commented Jan 25, 2021 •

edited

Loading

Don-Vito commented Jan 25, 2021

DHowett commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

DHowett commented Jan 25, 2021

carlos-zamora left a comment

zadjii-msft commented Jan 25, 2021

DHowett commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

Hook up the keybindings to the SUI #8882

Hook up the keybindings to the SUI #8882

Conversation

zadjii-msft commented Jan 25, 2021 • edited by DHowett Loading

Summary of the Pull Request

PR Checklist

Validation Steps Performed

DHowett commented Jan 25, 2021

DHowett commented Jan 25, 2021

Don-Vito commented Jan 25, 2021 • edited Loading

Don-Vito commented Jan 25, 2021

DHowett commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

DHowett commented Jan 25, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

zadjii-msft commented Jan 25, 2021

DHowett commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

Don-Vito commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021

zadjii-msft commented Jan 25, 2021 •

edited by DHowett

Loading

Don-Vito commented Jan 25, 2021 •

edited

Loading