-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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, redux #10121
Conversation
This is in selfhost today. |
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.
huh, I can't remember why we didn't do this. I bet we'll find out and it will be catastrophic. 😄
@carlos-zamora can you circle up on this? |
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.
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?
sui.PreviewKeyDown({ this, &TerminalPage::_SUIPreviewKeyDownHandler }); | ||
// GH#8767 - let unhandled keys in the SUI try to run commands too. | ||
sui.KeyDown({ this, &TerminalPage::_KeyDownHandler }); |
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.
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).
Hello @DHowett! Because this pull request has the 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 (
|
yeppers
-> #10169 |
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
🎉 Handy links: |
🎉 Handy links: |
Thanks @DHowett and @zadjii-msft for working on this issue and fixing it. It works on the download from the msft store. |
I uh, drew it in paint.net IIRC 😆. Either paint.net or some other sprite editor that I've long since forgotten |
@tabedzki I made mine at "FaceYourManga", which is pretty much an RPG character builder but for tiny avatar images |
Summary of the Pull Request
This is a redux of #8882.
From the original:
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 inPreviewKeyDown
. It can just be in the SUI'sKeyDown
.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
The special case handler from #8885 is no longer needed
Validation Steps Performed