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

Allow setting the action on Actions page #10220

Merged
merged 8 commits into from
Jul 2, 2021
Merged

Allow setting the action on Actions page #10220

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c3568ac
[Actions Page] Allow changes to action and args
carlos-zamora Apr 25, 2021
53ff37c
remove ActionAndArgsViewModel; rely on action names instead
carlos-zamora Jun 7, 2021
1453376
remove unnecessary ActionAndArgs::Equals changes
carlos-zamora Jun 7, 2021
e262055
move ActionAndArgs::Copy back to impl type
carlos-zamora Jun 7, 2021
652650b
code format
carlos-zamora Jun 7, 2021
a159fb8
Merge branch 'main' into dev/cazamor/actions-page/change-action
carlos-zamora Jul 2, 2021
9b896ce
apply PR feedback
carlos-zamora Jul 2, 2021
3499a72
add comment & combine lambdas
carlos-zamora Jul 2, 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
96 changes: 75 additions & 21 deletions src/cascadia/TerminalSettingsEditor/Actions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "KeyBindingViewModel.g.cpp"
#include "ActionsPageNavigationState.g.cpp"
#include "LibraryResources.h"
#include "../TerminalSettingsModel/AllShortcutActions.h"

using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::Foundation::Collections;
Expand All @@ -20,10 +21,12 @@ using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
KeyBindingViewModel::KeyBindingViewModel(const Control::KeyChord& keys, const Model::Command& cmd) :
KeyBindingViewModel::KeyBindingViewModel(const Control::KeyChord& keys, const hstring actionName, const IObservableVector<hstring>& availableActions) :
_Keys{ keys },
_KeyChordText{ Model::KeyChordSerialization::ToString(keys) },
_Command{ cmd }
_CurrentAction{ actionName },
_ProposedAction{ box_value(actionName) },
_AvailableActions{ availableActions }
{
// Add a property changed handler to our own property changed event.
// This propagates changes from the settings model to anybody listening to our
Expand All @@ -43,6 +46,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
_NotifyChanges(L"ShowEditButton");
}
else if (viewModelProperty == L"CurrentAction")
{
_NotifyChanges(L"Name");
}
});
}

Expand All @@ -63,8 +70,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
if (_IsInEditMode)
{
// if we're in edit mode,
// pre-populate the text box with the current keys
// - pre-populate the text box with the current keys
// - reset the combo box with the current action
ProposedKeys(KeyChordText());
ProposedAction(box_value(CurrentAction()));
}
}

Expand All @@ -75,13 +84,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void KeyBindingViewModel::AttemptAcceptChanges(hstring newKeyChordText)
{
auto args{ make_self<RebindKeysEventArgs>(_Keys, _Keys) };
auto args{ make_self<ModifyKeyBindingEventArgs>(_Keys, _Keys, _CurrentAction, unbox_value<hstring>(_ProposedAction)) };

// Key Chord Text
try
{
// Attempt to convert the provided key chord text
const auto newKeyChord{ KeyChordSerialization::FromString(newKeyChordText) };
args->NewKeys(newKeyChord);
_RebindKeysRequestedHandlers(*this, *args);
}
catch (hresult_invalid_argument)
{
Expand All @@ -94,6 +104,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// Alternatively, we want a full key chord editor/listener.
// If we implement that, we won't need this validation or error message.
}

_ModifyKeyBindingRequestedHandlers(*this, *args);
}

Actions::Actions()
Expand All @@ -118,16 +130,28 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
_State = e.Parameter().as<Editor::ActionsPageNavigationState>();

// Populate AvailableActionAndArgs
_AvailableActionMap = single_threaded_map<hstring, Model::ActionAndArgs>();
std::vector<hstring> availableActionAndArgs;
for (const auto& [name, actionAndArgs] : _State.Settings().ActionMap().AvailableActions())
{
availableActionAndArgs.push_back(name);
_AvailableActionMap.Insert(name, actionAndArgs);
}
std::sort(begin(availableActionAndArgs), end(availableActionAndArgs));
_AvailableActionAndArgs = single_threaded_observable_vector(std::move(availableActionAndArgs));

// Convert the key bindings from our settings into a view model representation
const auto& keyBindingMap{ _State.Settings().ActionMap().KeyBindings() };
std::vector<Editor::KeyBindingViewModel> keyBindingList;
keyBindingList.reserve(keyBindingMap.Size());
for (const auto& [keys, cmd] : keyBindingMap)
{
auto container{ make_self<KeyBindingViewModel>(keys, cmd) };
// convert the cmd into a KeyBindingViewModel
auto container{ make_self<KeyBindingViewModel>(keys, cmd.Name(), _AvailableActionAndArgs) };
container->PropertyChanged({ this, &Actions::_ViewModelPropertyChangedHandler });
container->DeleteKeyBindingRequested({ this, &Actions::_ViewModelDeleteKeyBindingHandler });
container->RebindKeysRequested({ this, &Actions::_ViewModelRebindKeysHandler });
container->ModifyKeyBindingRequested({ this, &Actions::_ViewModelModifyKeyBindingHandler });
container->IsAutomationPeerAttached(_AutomationPeerAttached);
keyBindingList.push_back(*container);
}
Expand Down Expand Up @@ -228,12 +252,44 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
}

void Actions::_ViewModelRebindKeysHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::RebindKeysEventArgs& args)
void Actions::_ViewModelModifyKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::ModifyKeyBindingEventArgs& args)
{
// If the key chord was changed,
// update the settings model and view model appropriately
auto attemptRebindKeys = [=]() {
if (args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey())
{
// update settings model
_State.Settings().ActionMap().RebindKeys(args.OldKeys(), args.NewKeys());

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

// If the action was changed,
// update the settings model and view model appropriately
auto attemptSetAction = [=]() {
Copy link
Member

Choose a reason for hiding this comment

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

attemptSetAction is always called when attemptRebindKeys is.
You can just merge the two lambdas into one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! I really only separated them out so that it's easier to read. I'm down to combine them, but I'm still worried about readability. Would something like this be clear maybe?

auto applyChangesToSettingsModel = [](){
   // code for 'attemptRebindKeys'
   // code for 'attemptSetAction'
};

if (args.OldActionName() != args.NewActionName())
{
// convert the action's name into a view model.
const auto& newAction{ _AvailableActionMap.Lookup(args.NewActionName()) };

// update settings model
_State.Settings().ActionMap().RegisterKeyBinding(args.NewKeys(), newAction);

// update view model
auto senderVMImpl{ get_self<KeyBindingViewModel>(senderVM) };
senderVMImpl->CurrentAction(args.NewActionName());
}
};

// Check for this special case:
// we're changing the key chord,
// but the new key chord is already in use
if (args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey())
Copy link
Contributor

@PankajBhojwani PankajBhojwani Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Question: do we also want to display a warning for something like "this action already has a keybinding, are you sure you want to add another?"

I know having multiple keybindings for an action is totally fine and not really 'conflicting', but with the long list of actions we currently show it might be easy for a user to miss that they've already set a keybinding for an action but forgot about it

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally okay with having this be a follow-up or a "wait until someone asks for it before we implement it" kinda thing, just wanted to know if it's been discussed already

{
// We're actually changing the key chord
const auto senderVMImpl{ get_self<KeyBindingViewModel>(senderVM) };
const auto& conflictingCmd{ _State.Settings().ActionMap().GetActionByKeyChord(args.NewKeys()) };
if (conflictingCmd)
{
Expand Down Expand Up @@ -262,8 +318,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
senderVM.AcceptChangesFlyout(nullptr);

// update settings model and view model
_State.Settings().ActionMap().RebindKeys(args.OldKeys(), args.NewKeys());
senderVMImpl->Keys(args.NewKeys());
attemptRebindKeys();
attemptSetAction();
senderVM.ToggleEditMode();
});

Expand All @@ -278,17 +334,15 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
senderVM.AcceptChangesFlyout(acceptChangesFlyout);
return;
}
else
{
// update settings model
_State.Settings().ActionMap().RebindKeys(args.OldKeys(), args.NewKeys());

// update view model (keys)
senderVMImpl->Keys(args.NewKeys());
}
}

// update view model (exit edit mode)
// update settings model and view model
attemptRebindKeys();
attemptSetAction();

// We NEED to toggle the edit mode here,
// so that if nothing changed, we still exit
// edit mode.
senderVM.ToggleEditMode();
}

Expand Down
36 changes: 25 additions & 11 deletions src/cascadia/TerminalSettingsEditor/Actions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#include "Actions.g.h"
#include "KeyBindingViewModel.g.h"
#include "ActionsPageNavigationState.g.h"
#include "RebindKeysEventArgs.g.h"
#include "ModifyKeyBindingEventArgs.g.h"
#include "Utils.h"
#include "ViewModelHelpers.h"

Expand All @@ -20,25 +20,28 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
};

struct RebindKeysEventArgs : RebindKeysEventArgsT<RebindKeysEventArgs>
struct ModifyKeyBindingEventArgs : ModifyKeyBindingEventArgsT<ModifyKeyBindingEventArgs>
{
public:
RebindKeysEventArgs(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys) :
ModifyKeyBindingEventArgs(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys, const hstring oldActionName, const hstring newActionName) :
_OldKeys{ oldKeys },
_NewKeys{ newKeys } {}
_NewKeys{ newKeys },
_OldActionName{ oldActionName },
_NewActionName{ newActionName } {}

WINRT_PROPERTY(Control::KeyChord, OldKeys, nullptr);
WINRT_PROPERTY(Control::KeyChord, NewKeys, nullptr);
WINRT_PROPERTY(hstring, OldActionName);
WINRT_PROPERTY(hstring, NewActionName);
};

struct KeyBindingViewModel : KeyBindingViewModelT<KeyBindingViewModel>, ViewModelHelper<KeyBindingViewModel>
{
public:
KeyBindingViewModel(const Control::KeyChord& keys, const Settings::Model::Command& cmd);
KeyBindingViewModel(const Control::KeyChord& keys, const hstring name, const Windows::Foundation::Collections::IObservableVector<hstring>& availableActions);

hstring Name() const { return _Command.Name(); }
hstring Name() const { return _CurrentAction; }
hstring KeyChordText() const { return _KeyChordText; }
Settings::Model::Command Command() const { return _Command; };

// UIA Text
hstring EditButtonName() const noexcept;
Expand All @@ -59,20 +62,29 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void AttemptAcceptChanges(hstring newKeyChordText);
void DeleteKeyBinding() { _DeleteKeyBindingRequestedHandlers(*this, _Keys); }

VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsInEditMode, false);
// 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.
Comment on lines +65 to +66
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 this description... Why can it disagree with the settings model? And why does the combo box item map to a settings model value? The way I understand the code, CurrentAction is the actual action identifier/name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Really it's because we have the "cancel"/"ok" system (aka the "edit mode" system). CurrentAction serves as...

  1. a record of what to set ProposedAction to on a cancellation
  2. a form of translation between ProposedAction and the settings model

If we got rid of the "edit mode" system, we don't have to worry about (1) anymore. To get rid of (2), KeyBindingViewModel would need a reference to the ActionMap. But even then, I think it's better to keep the ActionMap reference at a higher layer so that we can identify conflicting key bindings and key chords before editing the settings model. I think that's a better separation of responsibilities imo.

// AvailableActions: the list of options in the combo box; both actions above must be in this list.
VIEW_MODEL_OBSERVABLE_PROPERTY(IInspectable, ProposedAction);
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);

VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsInEditMode, false);
VIEW_MODEL_OBSERVABLE_PROPERTY(Windows::UI::Xaml::Controls::Flyout, AcceptChangesFlyout, nullptr);
VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsAutomationPeerAttached, false);
VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsHovered, false);
VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsContainerFocused, false);
VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsEditButtonFocused, false);
VIEW_MODEL_OBSERVABLE_PROPERTY(Windows::UI::Xaml::Media::Brush, ContainerBackground, nullptr);
TYPED_EVENT(RebindKeysRequested, Editor::KeyBindingViewModel, Editor::RebindKeysEventArgs);
TYPED_EVENT(ModifyKeyBindingRequested, Editor::KeyBindingViewModel, Editor::ModifyKeyBindingEventArgs);
TYPED_EVENT(DeleteKeyBindingRequested, Editor::KeyBindingViewModel, Terminal::Control::KeyChord);

private:
Settings::Model::Command _Command{ nullptr };
hstring _KeyChordText{};
};

Expand Down Expand Up @@ -101,11 +113,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
private:
void _ViewModelPropertyChangedHandler(const Windows::Foundation::IInspectable& senderVM, const Windows::UI::Xaml::Data::PropertyChangedEventArgs& args);
void _ViewModelDeleteKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Control::KeyChord& args);
void _ViewModelRebindKeysHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::RebindKeysEventArgs& args);
void _ViewModelModifyKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::ModifyKeyBindingEventArgs& args);

std::optional<uint32_t> _GetContainerIndexByKeyChord(const Control::KeyChord& keys);

bool _AutomationPeerAttached{ false };
Windows::Foundation::Collections::IObservableVector<hstring> _AvailableActionAndArgs;
Windows::Foundation::Collections::IMap<hstring, Model::ActionAndArgs> _AvailableActionMap;
};
}

Expand Down
8 changes: 6 additions & 2 deletions src/cascadia/TerminalSettingsEditor/Actions.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import "EnumEntry.idl";

namespace Microsoft.Terminal.Settings.Editor
{
runtimeclass RebindKeysEventArgs
runtimeclass ModifyKeyBindingEventArgs
{
Microsoft.Terminal.Control.KeyChord OldKeys { get; };
Microsoft.Terminal.Control.KeyChord NewKeys { get; };
String OldActionName { get; };
String NewActionName { get; };
}

runtimeclass KeyBindingViewModel : Windows.UI.Xaml.Data.INotifyPropertyChanged
Expand All @@ -21,6 +23,7 @@ namespace Microsoft.Terminal.Settings.Editor
Boolean ShowEditButton { get; };
Boolean IsInEditMode { get; };
String ProposedKeys;
Object ProposedAction;
Windows.UI.Xaml.Controls.Flyout AcceptChangesFlyout;
String EditButtonName { get; };
String CancelButtonName { get; };
Expand All @@ -34,11 +37,12 @@ namespace Microsoft.Terminal.Settings.Editor
void ActionLostFocus();
void EditButtonGettingFocus();
void EditButtonLosingFocus();
IObservableVector<String> AvailableActions { get; };
void ToggleEditMode();
void AttemptAcceptChanges();
void DeleteKeyBinding();

event Windows.Foundation.TypedEventHandler<KeyBindingViewModel, RebindKeysEventArgs> RebindKeysRequested;
event Windows.Foundation.TypedEventHandler<KeyBindingViewModel, ModifyKeyBindingEventArgs> ModifyKeyBindingRequested;
event Windows.Foundation.TypedEventHandler<KeyBindingViewModel, Microsoft.Terminal.Control.KeyChord> DeleteKeyBindingRequested;
}

Expand Down
10 changes: 9 additions & 1 deletion src/cascadia/TerminalSettingsEditor/Actions.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,15 @@
<!-- Command Name -->
<TextBlock Grid.Column="0"
Style="{StaticResource KeyBindingNameTextBlockStyle}"
Text="{x:Bind Name, Mode=OneWay}" />
Text="{x:Bind Name, Mode=OneWay}"
Visibility="{x:Bind IsInEditMode, Mode=OneWay, Converter={StaticResource InvertedBooleanToVisibilityConverter}}" />

<!-- Edit Mode: Action Combo-box -->
<ComboBox Grid.Column="0"
VerticalAlignment="Center"
ItemsSource="{x:Bind AvailableActions, Mode=OneWay}"
SelectedItem="{x:Bind ProposedAction, Mode=TwoWay}"
Visibility="{x:Bind IsInEditMode, Mode=OneWay}" />

<!-- Key Chord Text -->
<Border Grid.Column="1"
Expand Down
30 changes: 30 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "ActionAndArgs.g.cpp"

#include "JsonUtils.h"
#include "HashUtils.h"

#include <LibraryResources.h>

Expand Down Expand Up @@ -117,6 +118,35 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
#undef ON_ALL_ACTIONS_WITH_ARGS
};

ActionAndArgs::ActionAndArgs(ShortcutAction action)
{
// Find the deserializer
const auto deserializersIter = argSerializerMap.find(action);
if (deserializersIter != argSerializerMap.end())
{
auto pfn = deserializersIter->second.first;
if (pfn)
{
// Call the deserializer on an empty JSON object.
// This ensures that we have a valid ActionArgs
std::vector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> parseWarnings;
std::tie(_Args, parseWarnings) = pfn({});
}

// if an arg parser was registered, but failed,
// return the invalid ActionAndArgs we started with.
if (pfn && _Args == nullptr)
{
return;
}
}

// Either...
// (1) we don't have a deserializer, so it's ok for _Args to be null, or
// (2) we had one AND it worked, so _Args is set up properly
_Action = action;
}

// Function Description:
// - Attempts to match a string to a ShortcutAction. If there's no match, then
// returns ShortcutAction::Invalid
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/ActionAndArgs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
static Json::Value ToJson(const Model::ActionAndArgs& val);

ActionAndArgs() = default;
ActionAndArgs(ShortcutAction action);
ActionAndArgs(ShortcutAction action, IActionArgs args) :
_Action{ action },
_Args{ args } {};
Expand Down
Loading