-
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
Enable shortcut while CommandPalette is open #8586
Conversation
You may need @Don-Vito's help since he removed |
@Hegunumo - please let me know if I can be useful 😊 |
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.
<requesting changes as a mental note that this needs to be merged with main
>
@Don-Vito I'm trying to include |
@Hegunumo - UPDATE: please see comment below for what I believe is much better approach |
@Hegunumo - I reviewed the solution and did a small PoC. Since key down sends RoutedEvent we can handle it at the relevant element in the hierarchy. Which means we can apply the following logic:
This approach will also solve the problems like:
So my suggested approach is:
And the handling in the TerminalPage::_KeyDownHandler will be quite straightforward. Something like:
@zadjii-msft - FYI. This will extend this contribution into "Make key bindings universally available" 💪 |
@Don-Vito It wored! Please review. |
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.
Besides the small nit, looks great. But might be a bit confusing, since in the tabswitcher mode the binding is still handled by Palette.
If possible, I would suggest to remove the _bindings
entirely from CommandPaltte
(I mean delete _bindings
attribute from palette and its usages in CommandPaltte::SetKeyBindings
and CommandPaltte::_keyDownHandler
)
// - e: the KeyRoutedEventArgs containing info about the keystroke. | ||
// Return Value: | ||
// - <none> | ||
void TerminalPage::_CommandPaletteKeyDownHandler(Windows::Foundation::IInspectable const& /*sender*/, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e) |
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.
Consider renaming this into _KeyDownHandler
because it now serves the TerminalPage
in general: it runs the bindings no matter where you are. It does have a specific logic for CommandPalette, but tomorrow we will probably want to do more stuff.
Just playing with this, it feels really good. I'm a little shocked that this doesn't work for the new tab dropdown. Maybe that's not technically a child of the I'll double check the code now, but from a UX perspective this seems good to me |
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.
Okay yea I only have the same nit as Don-Vito. Thanks for doing 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.
I am concerned. Does this break preview mode? The tab switcher UI that Ctrl-Tab activates relies on commands to be dispatched without the palette being hidden. Will this force the command palette to be hidden when it should not be?
Otherwise, this is exactly what I wanted. I have always wanted us to move the key binding handler into TerminalPage!!
@Hegunumo - wait 😊. The palette should collapse (it is ephemeral, if we don't collapse it might show invalid data). What @DHowett means is to make sure that the Palette is not collapsed in the tab switcher mode (especially when custom keybindings are used). |
To @DHowett's initial concern - Control+Tab is handled explicitly in And for my concern it appears that custom bindings are not triggering TabSwitcher. So @Hegunumo's original solution works (before he removed the collapsing of the palette). I still strongly recommend removing the _bindings from Palette - but we can do it in a follow-up PR. |
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.
Thank you for testing it and for explaining it. @Hegunumo, good work!
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 (
|
This reverts commit 01a0490.
Following up #8586 by @Hegunumo, fully remove the command dispatching logic from Command Palette. Currently Command Palette might dispatch command in Tab Switcher mode. This leads to several inconsistencies: * Only the commands with the same key modifier as an ATS anchor will be issued * This command will not close the TabSwitcher (while commands issued from TerminalPage do). Implementation details: * Pass KeyMapping rather than binding to CommandPalette * Use this mapping inside previewKeyDownHandler of ATS to detect if previous tab or next tab bindings were engaged. No need to handle Ctrl+Tab explicitly anymore - it is handled as any other binding. * Cleanup the logic in TerminalPage::_SelectNextTab that checks if CommandPalette is visible. It is not required anymore, as visible palette would intercept the call. * Remove dependency of TerminalPage on AppLogic that was introduced lately .
This commit introduces direct shortcut dispatch to TerminalPage, which allows it to respond to key bindings before the command palette. This allows the user to use shortcuts from the command palette while it's open. Closes microsoft#6679
…8628) Following up microsoft#8586 by @Hegunumo, fully remove the command dispatching logic from Command Palette. Currently Command Palette might dispatch command in Tab Switcher mode. This leads to several inconsistencies: * Only the commands with the same key modifier as an ATS anchor will be issued * This command will not close the TabSwitcher (while commands issued from TerminalPage do). Implementation details: * Pass KeyMapping rather than binding to CommandPalette * Use this mapping inside previewKeyDownHandler of ATS to detect if previous tab or next tab bindings were engaged. No need to handle Ctrl+Tab explicitly anymore - it is handled as any other binding. * Cleanup the logic in TerminalPage::_SelectNextTab that checks if CommandPalette is visible. It is not required anymore, as visible palette would intercept the call. * Remove dependency of TerminalPage on AppLogic that was introduced lately .
This commit introduces direct shortcut dispatch to TerminalPage, which
allows it to respond to key bindings before the command palette.
This allows the user to use shortcuts from the command palette while
it's open.
Closes #6679