-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hook up the keybindings to the SUI #8882
Conversation
Can people copy/paste in the text fields inside the SUI still? |
I wonder why we didn't put key binding handling in a Preview handler for Page |
Same here 😄 |
Guys do you have any idea, why KeyDown doesn't get propagated to the TerminalPage? This could be much safer. |
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 😄 |
Oh. Hm. Great question 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Ah butts. It does not. The We could manually exempt Ctrl+C, Ctrl+V, Tab, Backspace, but that's starting to feel like the wrong solution. |
I thought that it returned false in non-terminal cases, indicating that the event should continue propagating. Hmm. |
Hmm. Binding |
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 |
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. |
@Don-Vito You're the best, thanks 😄 |
@zadjii-msft, @carlos-zamora - please see #8885 |
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
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 SUIMainPage
, that connects toTerminalPage::_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
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.