Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix CommandPalette to prefer inner interactions over bindings #9056

Merged
merged 4 commits into from
Feb 10, 2021
Merged

Fix CommandPalette to prefer inner interactions over bindings #9056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54c2060
Fix CommandPalette to prefer inner interactions over bindings
Don-Vito Feb 5, 2021
20e2682
Merge remote-tracking branch 'upstream/main' into 9044-prefer-palette…
Don-Vito Feb 9, 2021
698aa10
Fix line endings
Don-Vito Feb 9, 2021
f6ada94
Merge remote-tracking branch 'upstream/main' into 9044-prefer-palette…
Don-Vito Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 25 additions & 40 deletions src/cascadia/TerminalApp/CommandPalette.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ namespace winrt::TerminalApp::implementation
Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e)
{
auto key = e.OriginalKey();
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);

// Some keypresses such as Tab, Return, Esc, and Arrow Keys are ignored by controls because
// they're not considered input key presses. While they don't raise KeyDown events,
Expand All @@ -250,10 +253,6 @@ namespace winrt::TerminalApp::implementation
// a really widely used keyboard navigation key.
if (_currentMode == CommandPaletteMode::TabSwitchMode && _keymap)
{
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)
Expand All @@ -269,41 +268,21 @@ namespace winrt::TerminalApp::implementation
e.Handled(true);
}
}

return;
}
else if (key == VirtualKey::Home)

if (key == VirtualKey::Home && ctrlDown)
{
auto const state = CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Control);
if (WI_IsFlagSet(state, CoreVirtualKeyStates::Down))
{
ScrollToTop();
e.Handled(true);
}
ScrollToTop();
e.Handled(true);
}
else if (key == VirtualKey::End)
else if (key == VirtualKey::End && ctrlDown)
{
auto const state = CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Control);
if (WI_IsFlagSet(state, CoreVirtualKeyStates::Down))
{
ScrollToBottom();
e.Handled(true);
}
ScrollToBottom();
e.Handled(true);
}
}

// Method Description:
// - Process keystrokes in the input box. This is used for moving focus up
// and down the list of commands in Action mode, and for executing
// commands in both Action mode and Commandline mode.
// Arguments:
// - e: the KeyRoutedEventArgs containing info about the keystroke.
// Return Value:
// - <none>
void CommandPalette::_keyDownHandler(IInspectable const& /*sender*/,
Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e)
{
auto key = e.OriginalKey();

if (key == VirtualKey::Up)
else if (key == VirtualKey::Up)
{
// Action Mode: Move focus to the next item in the list.
SelectNextItem(false);
Expand Down Expand Up @@ -349,16 +328,22 @@ namespace winrt::TerminalApp::implementation

e.Handled(true);
}
else if (key == VirtualKey::Back)
else if (key == VirtualKey::Back && _searchBox().Text().empty() && _lastFilterTextWasEmpty && _currentMode == CommandPaletteMode::ActionMode)
{
// If the last filter text was empty, and we're backspacing from
// that state, then the user "backspaced" the virtual '>' we're
// using as the action mode indicator. Switch into commandline mode.
if (_searchBox().Text().empty() && _lastFilterTextWasEmpty && _currentMode == CommandPaletteMode::ActionMode)
{
_switchToMode(CommandPaletteMode::CommandlineMode);
}

_switchToMode(CommandPaletteMode::CommandlineMode);
e.Handled(true);
}
else if (key == VirtualKey::C && ctrlDown)
{
_searchBox().CopySelectionToClipboard();
e.Handled(true);
}
else if (key == VirtualKey::V && ctrlDown)
{
_searchBox().PasteFromClipboard();
e.Handled(true);
}
Comment on lines +339 to 348
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also hardcode some other text box default bindings like ctrl+a, ctrl+left/right, ctrl+x, and home/end? Or what makes ctrl+c and ctrl+v so special here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a way to ask the text box to handle the event first, and if it can't then we handle it? Or how does the order of handling work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Probably. I handled copy and paste explicitly as right now it is the same default binding for pasting in the terminal. Which means that the current behavior upon ctrl+v is to paste in the terminal.

}
Expand Down
3 changes: 1 addition & 2 deletions src/cascadia/TerminalApp/CommandPalette.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ namespace winrt::TerminalApp::implementation
Windows::UI::Xaml::RoutedEventArgs const& args);
void _previewKeyDownHandler(Windows::Foundation::IInspectable const& sender,
Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);
void _keyDownHandler(Windows::Foundation::IInspectable const& sender,
Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);

void _keyUpHandler(Windows::Foundation::IInspectable const& sender,
Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);

Expand Down
32 changes: 15 additions & 17 deletions src/cascadia/TerminalApp/CommandPalette.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ the MIT License. See LICENSE in the project root for license information. -->
AllowFocusOnInteraction="True"
PointerPressed="_rootPointerPressed"
PreviewKeyDown="_previewKeyDownHandler"
KeyDown="_keyDownHandler"
PreviewKeyUp="_keyUpHandler"
LostFocus="_lostFocusHandler"
mc:Ignorable="d"
Expand Down Expand Up @@ -47,13 +46,13 @@ the MIT License. See LICENSE in the project root for license information. -->

<Grid HorizontalAlignment="Stretch" ColumnSpacing="8" >
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<ColumnDefinition Width="16"/>
<!-- icon -->
<ColumnDefinition Width="Auto"/>
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<ColumnDefinition Width="*"/>
<!-- key chord -->
<ColumnDefinition Width="16"/>
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>

Expand Down Expand Up @@ -119,15 +118,15 @@ the MIT License. See LICENSE in the project root for license information. -->

<Grid HorizontalAlignment="Stretch" ColumnSpacing="8" >
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<ColumnDefinition Width="16"/>
<!-- icon / progress -->
<ColumnDefinition Width="Auto"/>
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<ColumnDefinition Width="*"/>
<!-- gutter for indicators -->
<ColumnDefinition Width="Auto"/>
<!-- Indicators -->
<ColumnDefinition Width="16"/>
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>

Expand Down Expand Up @@ -160,12 +159,12 @@ the MIT License. See LICENSE in the project root for license information. -->
VerticalAlignment="Center"
HorizontalAlignment="Right"
Orientation="Horizontal">
<FontIcon
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).BellIndicator, Mode=OneWay}"
Glyph="&#xEA8F;"
FontSize="12"

<FontIcon
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).BellIndicator, Mode=OneWay}"
Glyph="&#xEA8F;"
FontSize="12"
Margin="0,0,8,0"/>

<FontIcon
Expand All @@ -187,7 +186,7 @@ the MIT License. See LICENSE in the project root for license information. -->
</ListViewItem>
</DataTemplate>

<local:PaletteItemTemplateSelector x:Key="PaletteItemTemplateSelector" TabItemTemplate="{StaticResource TabItemTemplate}" GeneralItemTemplate="{StaticResource GeneralItemTemplate}"/>
<local:PaletteItemTemplateSelector x:Key="PaletteItemTemplateSelector" TabItemTemplate="{StaticResource TabItemTemplate}" GeneralItemTemplate="{StaticResource GeneralItemTemplate}"/>

<ResourceDictionary.ThemeDictionaries>
<ResourceDictionary x:Key="Dark">
Expand Down Expand Up @@ -431,7 +430,6 @@ the MIT License. See LICENSE in the project root for license information. -->
AllowDrop="False"
IsItemClickEnabled="True"
ItemClick="_listItemClicked"
PreviewKeyDown="_keyDownHandler"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double-checking. So if we have focus on a list view item, any key stroke we make will go to the UserControl's PreviewKeyDown handler on line 18? And that'll properly handle navigating the list view with the arrow keys and home/end?

Copy link
Contributor Author

@Don-Vito Don-Vito Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Actually Preview is going top down... and if the key is handled by the _keyDownHandler the propagation stops. In other words, this line was doing nothing functional (because if the handler handles the event, the propagation would stop on the palette level, and if the handler does not - there is no need to propagate) 😄

ItemsSource="{x:Bind FilteredActions}"
ItemTemplateSelector="{StaticResource PaletteItemTemplateSelector}">
</ListView>
Expand Down