Completely remove action dispatching from Command Palette #8628
Conversation
|
@zadjii-msft, @DHowett - bumping this (as a unified bump for all PRs) as the weekend (aka Terminal time) is approaching and I feel somewhat blocked: advancing further in the existing threads feels irrational, and I am not sure if there is a review capacity for additional ones 😊 |
|
Sorry for the delays, coming back from the holidays is always a diceroll of productivity 😄 (I'm giving Dustin some time to take a look, but I doubt there'll be much contention) |
|
@msftbot merge this in 6 hours |
ghost
added
the
AutoMerge
|
Hello @zadjii-msft! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
The delays are fine - I just don't want to create too much of a backlog! If you are not worried with me openning additional threads, I will 😊 |
DHowett
removed
the
AutoMerge
|
Does this get rid of the ability to activate key bindings while the command palette is open? Like, if it says "Close Application - alt+f4", should the user be able to press alt+f4 to close? There was an a11y bug about that. Or, did this get fixed by us using a preview handler on TerminalPage? |
This was fixed in the original PR, everything that is not mentioned for the internal navigation inside the palette will be routed to terminal page, which in turn will try to resolve the binding. |
|
LOVE IT. |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 1 hour 45 minutes. No worries though, I will be back when the time is right! 😉 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 (
|
Actually, the lack of practice has its toll and I miss from time to time 😊 Hope my team doesn't see my contributions before your reviews 😄 And thank You - I really enjoy as the team is great and I learn a lot! |
ghost
merged commit
|
🎉 Handy links: |
…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 .
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:
(while commands issued from TerminalPage do).
Implementation details:
if previous tab or next tab bindings were engaged.
No need to handle Ctrl+Tab explicitly anymore -
it is handled as any other binding.
that checks if CommandPalette is visible.
It is not required anymore, as visible palette would intercept the call.
that was introduced lately .