From 192d6debba3fa7d1204515e59023da000c315ee8 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 7 Jul 2021 16:43:40 -0700 Subject: [PATCH] Add the 'Add new' button to the Actions page (#10550) ## Summary of the Pull Request This adds the "add new" button to the actions page. It build on the work of #10220 by basically just adding a new list item to the top of the key binding list. This also makes it so that if you click the "accept changes" button when you have an invalid key chord, we don't do anything. ## References #6900 - Actions page Epic #9427 - Actions page design doc #10220 - Actions page PR - set action ## Detailed Description of the Pull Request / Additional comments - `ModifyKeyBindingEventArgs` is used to introduce new key bindings. We just ignore `OldKeys` and `OldActionName` because both didn't exist before. - `IsNewlyAdded` tracks if this is an action that was added, but has not been confirmed to add to the settings model. - `CancelChanges()` is directly bound to the cancel button. This allows us to delete the key binding when it's clicked on a "newly added" action. ## Validation Steps Performed - Cancel: - Deletes the action (because it doesn't truly exist until you confirm changes) - Accept: - Adds the new action. - If you attempt to edit it, the delete button is back. - Add Action: - Delete button should not be visible (redundant with 'Cancel') - Action should be initialized to a value - Key chord should be empty - Cannot add another action if a newly added action exists - Keyboard interaction: - escape --> cancel - enter --> accept - Accessibility: - "add new" button has a name - Interaction with other key bindings: - editing another action --> delete the "newly added" action (it hasn't been added yet) - only one action can be edited at a time --- .../TerminalSettingsEditor/Actions.cpp | 111 +++++++++++++++--- src/cascadia/TerminalSettingsEditor/Actions.h | 7 ++ .../TerminalSettingsEditor/Actions.idl | 2 + .../TerminalSettingsEditor/Actions.xaml | 19 ++- .../Resources/en-US/Resources.resw | 4 + 5 files changed, 122 insertions(+), 21 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/Actions.cpp b/src/cascadia/TerminalSettingsEditor/Actions.cpp index 62ec0700b27..ffd2ae343c8 100644 --- a/src/cascadia/TerminalSettingsEditor/Actions.cpp +++ b/src/cascadia/TerminalSettingsEditor/Actions.cpp @@ -21,6 +21,9 @@ using namespace winrt::Microsoft::Terminal::Settings::Model; namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { + KeyBindingViewModel::KeyBindingViewModel(const Windows::Foundation::Collections::IObservableVector& availableActions) : + KeyBindingViewModel(nullptr, availableActions.First().Current(), availableActions) {} + KeyBindingViewModel::KeyBindingViewModel(const Control::KeyChord& keys, const hstring& actionName, const IObservableVector& availableActions) : _Keys{ keys }, _KeyChordText{ Model::KeyChordSerialization::ToString(keys) }, @@ -84,18 +87,25 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void KeyBindingViewModel::AttemptAcceptChanges(hstring newKeyChordText) { - auto args{ make_self(_Keys, _Keys, _CurrentAction, unbox_value(_ProposedAction)) }; - - // Key Chord Text try { - // Attempt to convert the provided key chord text - const auto newKeyChord{ KeyChordSerialization::FromString(newKeyChordText) }; - args->NewKeys(newKeyChord); + // empty string --> don't accept changes + if (newKeyChordText.empty()) + { + return; + } + + // ModifyKeyBindingEventArgs + const auto args{ make_self(_Keys, // OldKeys + KeyChordSerialization::FromString(newKeyChordText), // NewKeys: Attempt to convert the provided key chord text + _IsNewlyAdded ? hstring{} : _CurrentAction, // OldAction + unbox_value(_ProposedAction)) }; // + _ModifyKeyBindingRequestedHandlers(*this, *args); } catch (hresult_invalid_argument) { - // Converting the text into a key chord failed + // 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. @@ -104,13 +114,25 @@ 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); + void KeyBindingViewModel::CancelChanges() + { + if (_IsNewlyAdded) + { + _DeleteNewlyAddedKeyBindingHandlers(*this, nullptr); + } + else + { + ToggleEditMode(); + } } Actions::Actions() { InitializeComponent(); + + Automation::AutomationProperties::SetName(AddNewButton(), RS_(L"Actions_AddNewTextBlock/Text")); } Automation::Peers::AutomationPeer Actions::OnCreateAutomationPeer() @@ -149,10 +171,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { // convert the cmd into a KeyBindingViewModel auto container{ make_self(keys, cmd.Name(), _AvailableActionAndArgs) }; - container->PropertyChanged({ this, &Actions::_ViewModelPropertyChangedHandler }); - container->DeleteKeyBindingRequested({ this, &Actions::_ViewModelDeleteKeyBindingHandler }); - container->ModifyKeyBindingRequested({ this, &Actions::_ViewModelModifyKeyBindingHandler }); - container->IsAutomationPeerAttached(_AutomationPeerAttached); + _RegisterEvents(container); keyBindingList.push_back(*container); } @@ -183,11 +202,30 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation } else if (e.OriginalKey() == VirtualKey::Escape) { - kbdVM.ToggleEditMode(); + 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. + auto kbdVM{ make_self(_AvailableActionAndArgs) }; + _RegisterEvents(kbdVM); + kbdVM->DeleteNewlyAddedKeyBinding({ this, &Actions::_ViewModelDeleteNewlyAddedKeyBindingHandler }); + + // Manually add the editing background. This needs to be done in Actions not the view model. + // We also have to do this manually because it hasn't been added to the list yet. + kbdVM->IsInEditMode(true); + const auto& containerBackground{ Resources().Lookup(box_value(L"ActionContainerBackgroundEditing")).as() }; + kbdVM->ContainerBackground(containerBackground); + + // IMPORTANT: do this _after_ setting IsInEditMode. Otherwise, it'll get deleted immediately + // by the PropertyChangedHandler below (where we delete any IsNewlyAdded items) + kbdVM->IsNewlyAdded(true); + _KeyBindingList.InsertAt(0, *kbdVM); + } + void Actions::_ViewModelPropertyChangedHandler(const IInspectable& sender, const Windows::UI::Xaml::Data::PropertyChangedEventArgs& args) { const auto senderVM{ sender.as() }; @@ -198,8 +236,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { // Ensure that... // 1. we move focus to the edit mode controls - // 2. this is the only entry that is in edit mode - for (uint32_t i = 0; i < _KeyBindingList.Size(); ++i) + // 2. any actions that were newly added are removed + // 3. this is the only entry that is in edit mode + for (int32_t i = _KeyBindingList.Size() - 1; i >= 0; --i) { const auto& kbdVM{ _KeyBindingList.GetAt(i) }; if (senderVM == kbdVM) @@ -210,6 +249,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const auto& container{ KeyBindingsListView().ContainerFromIndex(i).try_as() }; container.Focus(FocusState::Programmatic); } + else if (kbdVM.IsNewlyAdded()) + { + // Remove any actions that were newly added + _KeyBindingList.RemoveAt(i); + } else { // Exit edit mode for all other containers @@ -254,13 +298,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void Actions::_ViewModelModifyKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::ModifyKeyBindingEventArgs& args) { + const auto isNewAction{ !args.OldKeys() && args.OldActionName().empty() }; + auto applyChangesToSettingsModel = [=]() { // If the key chord was changed, // update the settings model and view model appropriately - if (args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey()) + // NOTE: we still need to update the view model if we're working with a newly added action + if (isNewAction || args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey()) { - // update settings model - _State.Settings().ActionMap().RebindKeys(args.OldKeys(), args.NewKeys()); + if (!isNewAction) + { + // update settings model + _State.Settings().ActionMap().RebindKeys(args.OldKeys(), args.NewKeys()); + } // update view model auto senderVMImpl{ get_self(senderVM) }; @@ -269,6 +319,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // If the action was changed, // update the settings model and view model appropriately + // NOTE: no need to check for "isNewAction" here. != already. if (args.OldActionName() != args.NewActionName()) { // convert the action's name into a view model. @@ -280,13 +331,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // update view model auto senderVMImpl{ get_self(senderVM) }; senderVMImpl->CurrentAction(args.NewActionName()); + senderVMImpl->IsNewlyAdded(false); } }; // 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()) + if (isNewAction || args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey()) { const auto& conflictingCmd{ _State.Settings().ActionMap().GetActionByKeyChord(args.NewKeys()) }; if (conflictingCmd) @@ -342,6 +394,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation senderVM.ToggleEditMode(); } + void Actions::_ViewModelDeleteNewlyAddedKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const IInspectable& /*args*/) + { + for (uint32_t i = 0; i < _KeyBindingList.Size(); ++i) + { + const auto& kbdVM{ _KeyBindingList.GetAt(i) }; + if (kbdVM == senderVM) + { + _KeyBindingList.RemoveAt(i); + return; + } + } + } + // Method Description: // - performs a search on KeyBindingList by key chord. // Arguments: @@ -365,4 +430,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // to quickly search through the sorted list. return std::nullopt; } + + void Actions::_RegisterEvents(com_ptr& kbdVM) + { + kbdVM->PropertyChanged({ this, &Actions::_ViewModelPropertyChangedHandler }); + kbdVM->DeleteKeyBindingRequested({ this, &Actions::_ViewModelDeleteKeyBindingHandler }); + kbdVM->ModifyKeyBindingRequested({ this, &Actions::_ViewModelModifyKeyBindingHandler }); + kbdVM->IsAutomationPeerAttached(_AutomationPeerAttached); + } } diff --git a/src/cascadia/TerminalSettingsEditor/Actions.h b/src/cascadia/TerminalSettingsEditor/Actions.h index e98cc915e6c..14d060a8a0d 100644 --- a/src/cascadia/TerminalSettingsEditor/Actions.h +++ b/src/cascadia/TerminalSettingsEditor/Actions.h @@ -38,6 +38,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation struct KeyBindingViewModel : KeyBindingViewModelT, ViewModelHelper { public: + KeyBindingViewModel(const Windows::Foundation::Collections::IObservableVector& availableActions); KeyBindingViewModel(const Control::KeyChord& keys, const hstring& name, const Windows::Foundation::Collections::IObservableVector& availableActions); hstring Name() const { return _CurrentAction; } @@ -60,6 +61,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void DisableEditMode() { IsInEditMode(false); } void AttemptAcceptChanges(); void AttemptAcceptChanges(hstring newKeyChordText); + void CancelChanges(); void DeleteKeyBinding() { _DeleteKeyBindingRequestedHandlers(*this, _Keys); } // ProposedAction: the entry selected by the combo box; may disagree with the settings model. @@ -81,6 +83,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation VIEW_MODEL_OBSERVABLE_PROPERTY(Control::KeyChord, Keys, nullptr); VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsInEditMode, false); + VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsNewlyAdded, 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); @@ -89,6 +92,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation VIEW_MODEL_OBSERVABLE_PROPERTY(Windows::UI::Xaml::Media::Brush, ContainerBackground, nullptr); TYPED_EVENT(ModifyKeyBindingRequested, Editor::KeyBindingViewModel, Editor::ModifyKeyBindingEventArgs); TYPED_EVENT(DeleteKeyBindingRequested, Editor::KeyBindingViewModel, Terminal::Control::KeyChord); + TYPED_EVENT(DeleteNewlyAddedKeyBinding, Editor::KeyBindingViewModel, IInspectable); private: hstring _KeyChordText{}; @@ -111,6 +115,7 @@ 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); WINRT_PROPERTY(Editor::ActionsPageNavigationState, State, nullptr); @@ -120,8 +125,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation 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 _ViewModelModifyKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::ModifyKeyBindingEventArgs& args); + void _ViewModelDeleteNewlyAddedKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const IInspectable& args); std::optional _GetContainerIndexByKeyChord(const Control::KeyChord& keys); + void _RegisterEvents(com_ptr& kbdVM); bool _AutomationPeerAttached{ false }; Windows::Foundation::Collections::IObservableVector _AvailableActionAndArgs; diff --git a/src/cascadia/TerminalSettingsEditor/Actions.idl b/src/cascadia/TerminalSettingsEditor/Actions.idl index 26e52ce0d21..98d8ee3e6f6 100644 --- a/src/cascadia/TerminalSettingsEditor/Actions.idl +++ b/src/cascadia/TerminalSettingsEditor/Actions.idl @@ -22,6 +22,7 @@ namespace Microsoft.Terminal.Settings.Editor // UI side Boolean ShowEditButton { get; }; Boolean IsInEditMode { get; }; + Boolean IsNewlyAdded { get; }; String ProposedKeys; Object ProposedAction; Windows.UI.Xaml.Controls.Flyout AcceptChangesFlyout; @@ -40,6 +41,7 @@ namespace Microsoft.Terminal.Settings.Editor IObservableVector AvailableActions { get; }; void ToggleEditMode(); void AttemptAcceptChanges(); + void CancelChanges(); void DeleteKeyBinding(); event Windows.Foundation.TypedEventHandler ModifyKeyBindingRequested; diff --git a/src/cascadia/TerminalSettingsEditor/Actions.xaml b/src/cascadia/TerminalSettingsEditor/Actions.xaml index 549c54f2781..d1d75c8a0bc 100644 --- a/src/cascadia/TerminalSettingsEditor/Actions.xaml +++ b/src/cascadia/TerminalSettingsEditor/Actions.xaml @@ -286,7 +286,7 @@ + Edit Text label for a button that can be used to begin making changes to a key binding entry. + + Add new + Button label that creates a new action on the actions page. + \ No newline at end of file