8247: Custom key bindings are broken for tab navigation #8250
Conversation
|
@zadjii-msft - please review. |
@zadjii-msft - This one is tough to notice while coding, unless everything is covered with UTs or there are manual testers. You cannot expect to find such a thing in dogfooding (I never even considered of binding tabs). BTW, I am not coding too much these days, but when I do, I always ship a lot of bad code |
|
@msftbot merge this in 1 minute |
ghost
added
the
AutoMerge
|
Hello @DHowett! 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". |
ghost
merged commit 
There are two code paths for Ctrl+Tab and for everything else: Ctrl + Tab is working perfectly * On the first tab navigation TerminalPage::_SelectNextTab resets the tab commands, and since palette is not visible selects the relevant tab index * On the second navigation Ctrl+Tab is intercepted by CommandPalette::_previewKeyDownHandler and everything works fine But with custom binding things are screwed: * On the first tab navigation TerminalPage::_SelectNextTab resets the tab commands, and since palette is not visible selects the relevant tab index * On the second navigation keys are not intercepted and thus TerminalPage::_SelectNextTab is called again. It resets the commands, but now since the palette is visible we simply invoke CommandPalette::SelectNextItem. Which in turn misbehaves because no item is selected. The approach for the solution is not to reset anything if the command palette is already open. ## Validation Steps Performed * Manual testing of both custom and non-custom bindings with different amount of tabs. Closes #8247 (cherry picked from commit 0437fe9)
There are two code paths for Ctrl+Tab and for everything else: Ctrl + Tab is working perfectly * On the first tab navigation TerminalPage::_SelectNextTab resets the tab commands, and since palette is not visible selects the relevant tab index * On the second navigation Ctrl+Tab is intercepted by CommandPalette::_previewKeyDownHandler and everything works fine But with custom binding things are screwed: * On the first tab navigation TerminalPage::_SelectNextTab resets the tab commands, and since palette is not visible selects the relevant tab index * On the second navigation keys are not intercepted and thus TerminalPage::_SelectNextTab is called again. It resets the commands, but now since the palette is visible we simply invoke CommandPalette::SelectNextItem. Which in turn misbehaves because no item is selected. The approach for the solution is not to reset anything if the command palette is already open. * Manual testing of both custom and non-custom bindings with different amount of tabs. Closes #8247 (cherry picked from commit 0437fe9)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
There are two code paths for Ctrl+Tab and for everything else:
Ctrl + Tab is working perfectly
tab commands, and since palette is not visible selects the relevant
tab index
CommandPalette::_previewKeyDownHandler and everything works fine
But with custom binding things are screwed:
tab commands, and since palette is not visible selects the relevant
tab index
TerminalPage::_SelectNextTab is called again. It resets the commands,
but now since the palette is visible we simply invoke
CommandPalette::SelectNextItem. Which in turn misbehaves because no
item is selected.
The approach for the solution is not to reset anything if the command
palette is already open.
Validation Steps Performed
amount of tabs.
Closes #8247