Skip to content

Commit

Permalink
Completely remove action dispatching from Command Palette (#8628)
Browse files Browse the repository at this point in the history 
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 .
  • Loading branch information
@Don-Vito
Don-Vito committed Jan 7, 2021
1 parent 7d503a4 commit c4c3c31
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 54 deletions.
56 changes: 20 additions & 36 deletions src/cascadia/TerminalApp/CommandPalette.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,18 +245,26 @@ namespace winrt::TerminalApp::implementation
// Only give anchored tab switcher the ability to cycle through tabs with the tab button.
// For unanchored mode, accessibility becomes an issue when we try to hijack tab since it's
// a really widely used keyboard navigation key.
if (_currentMode == CommandPaletteMode::TabSwitchMode && key == VirtualKey::Tab)
if (_currentMode == CommandPaletteMode::TabSwitchMode && _keymap)
{
auto const state = CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Shift);
if (WI_IsFlagSet(state, CoreVirtualKeyStates::Down))
{
SelectNextItem(false);
e.Handled(true);
}
else
auto const ctrlDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Control), CoreVirtualKeyStates::Down);
auto const altDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Menu), CoreVirtualKeyStates::Down);
auto const shiftDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Shift), CoreVirtualKeyStates::Down);

winrt::Microsoft::Terminal::TerminalControl::KeyChord kc{ ctrlDown, altDown, shiftDown, static_cast<int32_t>(key) };
const auto action = _keymap.TryLookup(kc);
if (action)
{
SelectNextItem(true);
e.Handled(true);
if (action.Action() == ShortcutAction::PrevTab)
{
SelectNextItem(false);
e.Handled(true);
}
else if (action.Action() == ShortcutAction::NextTab)
{
SelectNextItem(true);
e.Handled(true);
}
}
}
else if (key == VirtualKey::Home)
Expand Down Expand Up @@ -350,30 +358,6 @@ namespace winrt::TerminalApp::implementation

e.Handled(true);
}
else
{
const auto vkey = ::gsl::narrow_cast<WORD>(e.OriginalKey());

// In the interest of not telling all modes to check for keybindings, limit to TabSwitch mode for now.
if (_currentMode == CommandPaletteMode::TabSwitchMode)
{
auto const ctrlDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Control), CoreVirtualKeyStates::Down);
auto const altDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Menu), CoreVirtualKeyStates::Down);
auto const shiftDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Shift), CoreVirtualKeyStates::Down);

auto success = _bindings.TryKeyChord({
ctrlDown,
altDown,
shiftDown,
vkey,
});

if (success)
{
e.Handled(true);
}
}
}
}

// Method Description:
Expand Down Expand Up @@ -855,9 +839,9 @@ namespace winrt::TerminalApp::implementation
return _filteredActions;
}

void CommandPalette::SetKeyBindings(Microsoft::Terminal::TerminalControl::IKeyBindings bindings)
void CommandPalette::SetKeyMap(const Microsoft::Terminal::Settings::Model::KeyMapping& keymap)
{
_bindings = bindings;
_keymap = keymap;
}

void CommandPalette::SetCommands(Collections::IVector<Command> const& actions)
Expand Down
5 changes: 2 additions & 3 deletions src/cascadia/TerminalApp/CommandPalette.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace winrt::TerminalApp::implementation

void SetCommands(Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::Command> const& actions);
void SetTabs(Windows::Foundation::Collections::IVector<winrt::TerminalApp::TabBase> const& tabs, const bool clearList);
void SetKeyBindings(Microsoft::Terminal::TerminalControl::IKeyBindings bindings);
void SetKeyMap(const Microsoft::Terminal::Settings::Model::KeyMapping& keymap);

void EnableCommandPaletteMode(Microsoft::Terminal::Settings::Model::CommandPaletteLaunchMode const launchMode);

Expand All @@ -47,7 +47,6 @@ namespace winrt::TerminalApp::implementation

// Tab Switcher
void EnableTabSwitcherMode(const bool searchMode, const uint32_t startIdx);
void SetTabSwitchOrder(const Microsoft::Terminal::Settings::Model::TabSwitcherMode order);

WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
OBSERVABLE_GETSET_PROPERTY(winrt::hstring, NoMatchesText, _PropertyChangedHandlers);
Expand Down Expand Up @@ -109,7 +108,7 @@ namespace winrt::TerminalApp::implementation
std::wstring _getTrimmedInput();
void _evaluatePrefix();

Microsoft::Terminal::TerminalControl::IKeyBindings _bindings;
Microsoft::Terminal::Settings::Model::KeyMapping _keymap{ nullptr };

// Tab Switcher
Windows::Foundation::Collections::IVector<winrt::TerminalApp::FilteredCommand> _tabActions{ nullptr };
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/CommandPalette.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace TerminalApp

void SetCommands(Windows.Foundation.Collections.IVector<Microsoft.Terminal.Settings.Model.Command> actions);
void SetTabs(Windows.Foundation.Collections.IVector<TabBase> tabs, Boolean clearList);
void SetKeyBindings(Microsoft.Terminal.TerminalControl.IKeyBindings bindings);
void SetKeyMap(Microsoft.Terminal.Settings.Model.KeyMapping keymap);
void EnableCommandPaletteMode(Microsoft.Terminal.Settings.Model.CommandPaletteLaunchMode launchMode);

void SelectNextItem(Boolean moveDown);
Expand Down
17 changes: 3 additions & 14 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include "pch.h"
#include "TerminalPage.h"
#include "Utils.h"
#include "AppLogic.h"
#include "../../types/inc/utils.hpp"

#include <LibraryResources.h>
Expand Down Expand Up @@ -108,7 +107,7 @@ namespace winrt::TerminalApp::implementation
if (auto page{ weakThis.get() })
{
_UpdateCommandsForPalette();
CommandPalette().SetKeyBindings(*_bindings);
CommandPalette().SetKeyMap(_settings.KeyMap());
}
}

Expand Down Expand Up @@ -945,9 +944,7 @@ namespace winrt::TerminalApp::implementation
auto const shiftDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Shift), CoreVirtualKeyStates::Down);

winrt::Microsoft::Terminal::TerminalControl::KeyChord kc{ ctrlDown, altDown, shiftDown, static_cast<int32_t>(key) };
auto setting = AppLogic::CurrentAppSettings();
auto keymap = setting.GlobalSettings().KeyMap();
const auto actionAndArgs = keymap.TryLookup(kc);
const auto actionAndArgs = _settings.KeyMap().TryLookup(kc);
if (actionAndArgs)
{
if (CommandPalette().Visibility() == Visibility::Visible && actionAndArgs.Action() != ShortcutAction::ToggleCommandPalette)
Expand Down Expand Up @@ -1318,14 +1315,6 @@ namespace winrt::TerminalApp::implementation
// - Sets focus to the tab to the right or left the currently selected tab.
void TerminalPage::_SelectNextTab(const bool bMoveRight)
{
if (CommandPalette().Visibility() == Visibility::Visible)
{
// If the tab switcher is currently open, don't change its mode.
// Just select the new tab.
CommandPalette().SelectNextItem(bMoveRight);
return;
}

const auto tabSwitchMode = _settings.GlobalSettings().TabSwitcherMode();
const bool useInOrderTabIndex = tabSwitchMode != TabSwitcherMode::MostRecentlyUsed;

Expand Down Expand Up @@ -2207,7 +2196,7 @@ namespace winrt::TerminalApp::implementation
// here, then the user won't be able to navigate the ATS any
// longer.
//
// When the tab swither is eventually dismissed, the focus will
// When the tab switcher is eventually dismissed, the focus will
// get tossed back to the focused terminal control, so we don't
// need to worry about focus getting lost.
if (CommandPalette().Visibility() != Visibility::Visible)
Expand Down

0 comments on commit c4c3c31

Please sign in to comment.