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

Add a KeyChordListener to the Settings UI #10652

Merged
3 commits merged into from
Jul 16, 2021
Merged

Add a KeyChordListener to the Settings UI #10652

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c81c07
Add a KeyChordListener to the Settings UI
carlos-zamora Jul 13, 2021
bbaf737
apply Dustin's feedback
carlos-zamora Jul 14, 2021
dc38f9f
address suggestions from Leonard
carlos-zamora Jul 16, 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
76 changes: 13 additions & 63 deletions src/cascadia/TerminalSettingsEditor/Actions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
KeyBindingViewModel(nullptr, availableActions.First().Current(), availableActions) {}

KeyBindingViewModel::KeyBindingViewModel(const Control::KeyChord& keys, const hstring& actionName, const IObservableVector<hstring>& availableActions) :
_Keys{ keys },
_KeyChordText{ Model::KeyChordSerialization::ToString(keys) },
_CurrentKeys{ keys },
_KeyChordText{ KeyChordSerialization::ToString(keys) },
_CurrentAction{ actionName },
_ProposedAction{ box_value(actionName) },
_AvailableActions{ availableActions }
Expand All @@ -36,9 +36,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// unique view model members.
PropertyChanged([this](auto&&, const PropertyChangedEventArgs& args) {
const auto viewModelProperty{ args.PropertyName() };
if (viewModelProperty == L"Keys")
if (viewModelProperty == L"CurrentKeys")
{
_KeyChordText = Model::KeyChordSerialization::ToString(_Keys);
_KeyChordText = Model::KeyChordSerialization::ToString(_CurrentKeys);
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
_NotifyChanges(L"KeyChordText");
}
else if (viewModelProperty == L"IsContainerFocused" ||
Expand Down Expand Up @@ -75,7 +75,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// if we're in edit mode,
// - pre-populate the text box with the current keys
// - reset the combo box with the current action
ProposedKeys(KeyChordText());
ProposedKeys(CurrentKeys());
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
ProposedAction(box_value(CurrentAction()));
}
}
Expand All @@ -85,35 +85,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
AttemptAcceptChanges(_ProposedKeys);
}

void KeyBindingViewModel::AttemptAcceptChanges(hstring newKeyChordText)
void KeyBindingViewModel::AttemptAcceptChanges(const Control::KeyChord newKeys)
{
try
{
// empty string --> don't accept changes
if (newKeyChordText.empty())
{
return;
}

// ModifyKeyBindingEventArgs
const auto args{ make_self<ModifyKeyBindingEventArgs>(_Keys, // OldKeys
KeyChordSerialization::FromString(newKeyChordText), // NewKeys: Attempt to convert the provided key chord text
_IsNewlyAdded ? hstring{} : _CurrentAction, // OldAction
unbox_value<hstring>(_ProposedAction)) }; //
_ModifyKeyBindingRequestedHandlers(*this, *args);
}
catch (hresult_invalid_argument)
{
// Converting the text into a key chord failed.
// Don't accept the changes.
// TODO GH #6900:
// This is tricky. I still haven't found a way to reference the
// key chord text box. It's hidden behind the data template.
// Ideally, some kind of notification would alert the user, but
// to make it look nice, we need it to somehow target the text box.
// Alternatively, we want a full key chord editor/listener.
// If we implement that, we won't need this validation or error message.
}
const auto args{ make_self<ModifyKeyBindingEventArgs>(_CurrentKeys, // OldKeys
newKeys, // NewKeys
_IsNewlyAdded ? hstring{} : _CurrentAction, // OldAction
unbox_value<hstring>(_ProposedAction)) }; // NewAction
_ModifyKeyBindingRequestedHandlers(*this, *args);
}

void KeyBindingViewModel::CancelChanges()
Expand Down Expand Up @@ -179,34 +157,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_KeyBindingList = single_threaded_observable_vector(std::move(keyBindingList));
}

void Actions::KeyChordEditor_KeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e)
{
const auto& senderTB{ sender.as<TextBox>() };
const auto& kbdVM{ senderTB.DataContext().as<Editor::KeyBindingViewModel>() };
if (e.OriginalKey() == VirtualKey::Enter)
{
// Fun fact: this is happening _before_ "_ProposedKeys" gets updated
// with the two-way data binding. So we need to directly extract the text
// and tell the view model to update itself.
get_self<KeyBindingViewModel>(kbdVM)->AttemptAcceptChanges(senderTB.Text());

// For an unknown reason, when 'AcceptChangesFlyout' is set in the code above,
// the flyout isn't shown, forcing the 'Enter' key to do nothing.
// To get around this, detect if the flyout was set, and display it
// on the text box.
if (kbdVM.AcceptChangesFlyout() != nullptr)
{
kbdVM.AcceptChangesFlyout().ShowAt(senderTB);
}
e.Handled(true);
}
else if (e.OriginalKey() == VirtualKey::Escape)
{
kbdVM.CancelChanges();
e.Handled(true);
}
}

void Actions::AddNew_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*eventArgs*/)
{
// Create the new key binding and register all of the event handlers.
Expand Down Expand Up @@ -314,7 +264,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

// update view model
auto senderVMImpl{ get_self<KeyBindingViewModel>(senderVM) };
senderVMImpl->Keys(args.NewKeys());
senderVMImpl->CurrentKeys(args.NewKeys());
}

// If the action was changed,
Expand Down Expand Up @@ -418,7 +368,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
for (uint32_t i = 0; i < _KeyBindingList.Size(); ++i)
{
const auto kbdVM{ get_self<KeyBindingViewModel>(_KeyBindingList.GetAt(i)) };
const auto& otherKeys{ kbdVM->Keys() };
const auto& otherKeys{ kbdVM->CurrentKeys() };
if (keys.Modifiers() == otherKeys.Modifiers() && keys.Vkey() == otherKeys.Vkey())
{
return i;
Expand Down
13 changes: 6 additions & 7 deletions src/cascadia/TerminalSettingsEditor/Actions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void ToggleEditMode();
void DisableEditMode() { IsInEditMode(false); }
void AttemptAcceptChanges();
void AttemptAcceptChanges(hstring newKeyChordText);
void AttemptAcceptChanges(const Control::KeyChord newKeys);
void CancelChanges();
void DeleteKeyBinding() { _DeleteKeyBindingRequestedHandlers(*this, _Keys); }
void DeleteKeyBinding() { _DeleteKeyBindingRequestedHandlers(*this, _CurrentKeys); }

// ProposedAction: the entry selected by the combo box; may disagree with the settings model.
// CurrentAction: the combo box item that maps to the settings model value.
Expand All @@ -77,10 +77,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
VIEW_MODEL_OBSERVABLE_PROPERTY(hstring, CurrentAction);
WINRT_PROPERTY(Windows::Foundation::Collections::IObservableVector<hstring>, AvailableActions, nullptr);

// ProposedKeys: the text shown in the text box; may disagree with the settings model.
// Keys: the key chord bound in the settings model.
VIEW_MODEL_OBSERVABLE_PROPERTY(hstring, ProposedKeys);
VIEW_MODEL_OBSERVABLE_PROPERTY(Control::KeyChord, Keys, nullptr);
// ProposedKeys: the keys proposed by the control; may disagree with the settings model.
// CurrentKeys: the key chord bound in the settings model.
VIEW_MODEL_OBSERVABLE_PROPERTY(Control::KeyChord, ProposedKeys);
VIEW_MODEL_OBSERVABLE_PROPERTY(Control::KeyChord, CurrentKeys, nullptr);

VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsInEditMode, false);
VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsNewlyAdded, false);
Expand Down Expand Up @@ -114,7 +114,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);
Windows::UI::Xaml::Automation::Peers::AutomationPeer OnCreateAutomationPeer();
void KeyChordEditor_KeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);
void AddNew_Click(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);

WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsEditor/Actions.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace Microsoft.Terminal.Settings.Editor
Boolean ShowEditButton { get; };
Boolean IsInEditMode { get; };
Boolean IsNewlyAdded { get; };
String ProposedKeys;
Microsoft.Terminal.Control.KeyChord ProposedKeys;
Object ProposedAction;
Windows.UI.Xaml.Controls.Flyout AcceptChangesFlyout;
String EditButtonName { get; };
Expand Down
21 changes: 8 additions & 13 deletions src/cascadia/TerminalSettingsEditor/Actions.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,9 @@
<Setter Property="TextWrapping" Value="WrapWholeWords" />
</Style>
<Style x:Key="KeyChordEditorStyle"
BasedOn="{StaticResource DefaultTextBoxStyle}"
TargetType="TextBox">
TargetType="local:KeyChordListener">
<Setter Property="HorizontalAlignment" Value="Right" />
<Setter Property="VerticalAlignment" Value="Center" />
<Setter Property="TextAlignment" Value="Right" />
<Setter Property="TextWrapping" Value="Wrap" />
<Setter Property="IsSpellCheckEnabled" Value="False" />
</Style>
<x:Int32 x:Key="EditButtonSize">32</x:Int32>
<x:Double x:Key="EditButtonIconSize">15</x:Double>
Expand Down Expand Up @@ -202,7 +198,8 @@
Visibility="{x:Bind IsInEditMode, Mode=OneWay, Converter={StaticResource InvertedBooleanToVisibilityConverter}}" />

<!-- Edit Mode: Action Combo-box -->
<ComboBox Grid.Column="0"
<ComboBox x:Uid="Actions_ActionComboBox"
Grid.Column="0"
VerticalAlignment="Center"
ItemsSource="{x:Bind AvailableActions, Mode=OneWay}"
SelectedItem="{x:Bind ProposedAction, Mode=TwoWay}"
Expand All @@ -222,13 +219,11 @@
TextWrapping="WrapWholeWords" />
</Border>

<!-- Edit Mode: Key Chord Text Box -->
<TextBox Grid.Column="1"
DataContext="{x:Bind Mode=OneWay}"
KeyDown="KeyChordEditor_KeyDown"
Style="{StaticResource KeyChordEditorStyle}"
Text="{x:Bind ProposedKeys, Mode=TwoWay}"
Visibility="{x:Bind IsInEditMode, Mode=OneWay}" />
<!-- Edit Mode: Key Chord Listener -->
<local:KeyChordListener Grid.Column="1"
Keys="{x:Bind ProposedKeys, Mode=TwoWay}"
Style="{StaticResource KeyChordEditorStyle}"
Visibility="{x:Bind IsInEditMode, Mode=OneWay}" />

<!-- Edit Button -->
<Button x:Uid="Actions_EditButton"
Expand Down
143 changes: 143 additions & 0 deletions src/cascadia/TerminalSettingsEditor/KeyChordListener.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "KeyChordListener.h"
#include "KeyChordListener.g.cpp"
#include "LibraryResources.h"

using namespace winrt::Windows::UI::Core;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Windows::UI::Xaml::Data;
using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::System;
using namespace winrt::Windows::UI::Xaml::Input;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
DependencyProperty KeyChordListener::_KeysProperty{ nullptr };

static constexpr std::array<VirtualKey, 10> ModifierKeys{
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
VirtualKey::Menu,
VirtualKey::RightMenu,
VirtualKey::LeftMenu,
VirtualKey::Control,
VirtualKey::RightControl,
VirtualKey::LeftControl,
VirtualKey::Shift,
VirtualKey::LeftShift,
VirtualKey::RightWindows,
VirtualKey::LeftWindows
};

static VirtualKeyModifiers _GetModifiers()
{
const auto window{ CoreWindow::GetForCurrentThread() };

VirtualKeyModifiers flags = VirtualKeyModifiers::None;
for (const auto mod : ModifierKeys)
{
const auto state = window.GetKeyState(mod);
const auto isDown = WI_IsFlagSet(state, CoreVirtualKeyStates::Down);

if (isDown)
{
switch (mod)
{
case VirtualKey::Control:
case VirtualKey::LeftControl:
case VirtualKey::RightControl:
{
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
flags |= VirtualKeyModifiers::Control;
break;
}
case VirtualKey::Menu:
case VirtualKey::LeftMenu:
case VirtualKey::RightMenu:
{
flags |= VirtualKeyModifiers::Menu;
break;
}
case VirtualKey::Shift:
case VirtualKey::LeftShift:
{
flags |= VirtualKeyModifiers::Shift;
break;
}
case VirtualKey::LeftWindows:
case VirtualKey::RightWindows:
{
flags |= VirtualKeyModifiers::Windows;
break;
}
}
}
}
return flags;
}

KeyChordListener::KeyChordListener()
{
InitializeComponent();
_InitializeProperties();
}

void KeyChordListener::_InitializeProperties()
{
// Initialize any KeyChordListener dependency properties here.
// This performs a lazy load on these properties, instead of
// initializing them when the DLL loads.
if (!_KeysProperty)
{
_KeysProperty =
DependencyProperty::Register(
L"Keys",
xaml_typename<Control::KeyChord>(),
xaml_typename<Editor::KeyChordListener>(),
PropertyMetadata{ nullptr, PropertyChangedCallback{ &KeyChordListener::_OnKeysChanged } });
}
}

void KeyChordListener::_OnKeysChanged(DependencyObject const& d, DependencyPropertyChangedEventArgs const& e)
{
if (auto control{ d.try_as<Editor::KeyChordListener>() })
{
auto controlImpl{ get_self<KeyChordListener>(control) };
TextBox tb{ controlImpl->FindName(L"KeyChordTextBox").as<TextBox>() };
tb.Text(Model::KeyChordSerialization::ToString(unbox_value<Control::KeyChord>(e.NewValue())));
if (auto automationPeer{ Automation::Peers::FrameworkElementAutomationPeer::FromElement(tb) })
{
automationPeer.RaiseNotificationEvent(
Automation::Peers::AutomationNotificationKind::ActionCompleted,
Automation::Peers::AutomationNotificationProcessing::MostRecent,
tb.Text(),
L"KeyChordListenerText");
}
}
}

void KeyChordListener::KeyChordTextBox_KeyDown(IInspectable const& /*sender*/, KeyRoutedEventArgs const& e)
{
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
const auto key{ e.OriginalKey() };
for (const auto mod : ModifierKeys)
{
if (key == mod)
Copy link
Member

Choose a reason for hiding this comment

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

some implementations show "partial" keys as they go. like, if you press shift it'll say "shift+". Is that desirable here, or too complicated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I played with that a little bit. I ended up not liking it because plain modifiers aren't a valid key chord (at least for a key binding, not the object itself).

We could change it at any time, but I vote we see how this feels in a selfhost/bug-bash, then iterate from there.

{
// Ignore modifier keys
return;
}
}

const auto modifiers{ _GetModifiers() };
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the need for _GetModifiers.
If you keep track over how many keys are simultaneously pressed you can use the key down events directly to assemble the modifiers bitfield. This removes the need for the majority of the _GetModifiers implementation.
(You'd also need to reset the modifiers when the control looses focus of course.)

Copy link
Member

Choose a reason for hiding this comment

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

My gut feeling is that introducing another place where state must be preserved and maintained is going to be worse for code clarity (and has the risk of introducing torn state) where here in a non-perf-sensitive context we can just use the state the UI framework is already keeping track of for us 😄

Copy link
Member

@lhecker lhecker Jul 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't tried out this branch locally but I assumed that it works similar to key bindings in Discord: All key presses are additive, until you let go of any key, after which the result is added.

Meaning if you press:

  • M Down
  • Ctrl Down
  • Shift Down
  • M Up

...in that order it'll save Ctrl+Shift+M as a keybinding. Even though I pressed M first, which is important because the UI should work even if I attempt to press all 3 keys simultaneously! (...and then any of those may be pressed first.)

If that's how this is supposed to work as well I disagree: Using the events lends itself to a clearer association between what happens in the UI and how the code would work (namely all key downs being additive, until a key up occurs).

Copy link
Member

Choose a reason for hiding this comment

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

I've tested it some more in Discord: If you let go of a modifier key for a while it'll count as a keybinding automatically. But I don't think we have to make it that complicated.

Also I just realized a problem with my approach: You still would have to detect when you have released all modifier keys. 😅 As such a good approach might be to count the concurrently pressed keys after all.
I'll approve the PR as I realize that the current code is already written and functional.

Copy link
Member

Choose a reason for hiding this comment

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

I've been talking with Dustin about this: Making all key presses purely additive would probably be the most consistent way to enter key chords. Discord's keybinding editor is a pretty great template for us.
As I said before we'd need a "concurrently pressed keys counter" though, in order to detect when all keys have been released. Because every time this counter reaches zero we'll have to call KeyChordSerialization::ToString and see if all added up keys formed a valid sequence, as well as to reset the modifiers and vkey member fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's funny that you say that, because PowerToys' hotkey listener kinda works like that. It constantly updates the control on every key press, but it only sets the setting on a key-up event.

I figured the current approach would be a bit easier and cleaner because we only accept key chords that have an associated non-modifier vkey. This also forces you to actually press the key combination you want to enact, because in TermControl pressing M-Ctrl-Shift would execute the M key binding.

if (key == VirtualKey::Tab && (modifiers == VirtualKeyModifiers::None || modifiers == VirtualKeyModifiers::Shift))
{
// [Shift]+[Tab] && [Tab] are needed for keyboard navigation
return;
}

// Permitted key events are used to update _Keys
Keys({ modifiers, static_cast<int32_t>(key) });
e.Handled(true);
}
}
29 changes: 29 additions & 0 deletions src/cascadia/TerminalSettingsEditor/KeyChordListener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "KeyChordListener.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
struct KeyChordListener : KeyChordListenerT<KeyChordListener>
{
public:
KeyChordListener();

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

DEPENDENCY_PROPERTY(Control::KeyChord, Keys);

private:
static void _InitializeProperties();
static void _OnKeysChanged(Windows::UI::Xaml::DependencyObject const& d, Windows::UI::Xaml::DependencyPropertyChangedEventArgs const& e);
};
}

namespace winrt::Microsoft::Terminal::Settings::Editor::factory_implementation
{
BASIC_FACTORY(KeyChordListener);
}
Loading