Implement MVVM for the Actions page #14292

PankajBhojwani · 2022-10-25T23:00:51Z

Summary of the Pull Request

Implements an ActionsViewModel for the Actions page in the SUI

References

PR Checklist

Closes #xxx
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

A few annoyances:

Because of the shifts in the UI when switching between edit mode/regular mode on the KeyBindingViewModels, the page used to manually handle moving focus accordingly so that focus does not get lost. However, the page no longer owns the KeyBindingViewModel, instead the ActionsViewModel owns them but the ActionsViewModel cannot manually move focus around since it does not own the UI Element. So, the ActionsViewModel emits an event for the page to catch that tells the page to move focus (FocusContainer).
Similarly, the page used to manually update the ContainerBackground of the KeyBindingViewModel when the kbdVM enters EditMode. The ActionsViewModel does not have access to the page's resources though (to determine the correct background brush to use). So, the ActionsViewModel emits another event for the page to catch (UpdateBackground).

Validation Steps Performed

Actions page still works as before

src/cascadia/TerminalSettingsEditor/Converters.cpp

src/cascadia/TerminalSettingsEditor/ActionsViewModel.idl

carlos-zamora · 2022-10-26T18:30:35Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h

+    struct KeyBindingViewModelComparator
+    {
+        bool operator()(const Editor::KeyBindingViewModel& lhs, const Editor::KeyBindingViewModel& rhs) const
+        {
+            return lhs.Name() < rhs.Name();
+        }
+    };
+
+    struct ModifyKeyBindingEventArgs : ModifyKeyBindingEventArgsT<ModifyKeyBindingEventArgs>
+    {
+    public:
+        ModifyKeyBindingEventArgs(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys, const hstring oldActionName, const hstring newActionName) :
+            _OldKeys{ oldKeys },
+            _NewKeys{ newKeys },
+            _OldActionName{ std::move(oldActionName) },
+            _NewActionName{ std::move(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 Windows::Foundation::Collections::IObservableVector<hstring>& availableActions);
+        KeyBindingViewModel(const Control::KeyChord& keys, const hstring& name, const Windows::Foundation::Collections::IObservableVector<hstring>& availableActions);
+
+        hstring Name() const { return _CurrentAction; }
+        hstring KeyChordText() const { return _KeyChordText; }
+
+        // UIA Text
+        hstring EditButtonName() const noexcept;
+        hstring CancelButtonName() const noexcept;
+        hstring AcceptButtonName() const noexcept;
+        hstring DeleteButtonName() const noexcept;
+
+        void EnterHoverMode() { IsHovered(true); };
+        void ExitHoverMode() { IsHovered(false); };
+        void ActionGotFocus() { IsContainerFocused(true); };
+        void ActionLostFocus() { IsContainerFocused(false); };
+        void EditButtonGettingFocus() { IsEditButtonFocused(true); };
+        void EditButtonLosingFocus() { IsEditButtonFocused(false); };
+        bool ShowEditButton() const noexcept;
+        void ToggleEditMode();
+        void DisableEditMode() { IsInEditMode(false); }
+        void AttemptAcceptChanges();
+        void AttemptAcceptChanges(const Control::KeyChord newKeys);
+        void CancelChanges();
+        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.
+        // AvailableActions: the list of options in the combo box; both actions above must be in this list.
+        // NOTE: ProposedAction and CurrentAction may disagree mainly due to the "edit mode" system in place.
+        //       Current Action 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
+        //       We would also need an ActionMap reference to remove this, but this is a better separation
+        //       of responsibilities.
+        VIEW_MODEL_OBSERVABLE_PROPERTY(IInspectable, ProposedAction);
+        VIEW_MODEL_OBSERVABLE_PROPERTY(hstring, CurrentAction);
+        WINRT_PROPERTY(Windows::Foundation::Collections::IObservableVector<hstring>, AvailableActions, 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);
+        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(ModifyKeyBindingRequested, Editor::KeyBindingViewModel, Editor::ModifyKeyBindingEventArgs);
+        TYPED_EVENT(DeleteKeyBindingRequested, Editor::KeyBindingViewModel, Terminal::Control::KeyChord);
+        TYPED_EVENT(DeleteNewlyAddedKeyBinding, Editor::KeyBindingViewModel, IInspectable);
+
+    private:
+        hstring _KeyChordText{};
+    };


Note to other reviewers: this is copy-pasted from Actions.h

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

carlos-zamora · 2022-10-26T19:06:21Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

+        kbdVM->PropertyChanged({ this, &ActionsViewModel::_ViewModelPropertyChangedHandler });
+        kbdVM->DeleteKeyBindingRequested({ this, &ActionsViewModel::_ViewModelDeleteKeyBindingHandler });
+        kbdVM->ModifyKeyBindingRequested({ this, &ActionsViewModel::_ViewModelModifyKeyBindingHandler });


Idk if this is possible, but it'd be nice if we didn't need to register ActionsViewModel methods to KeyBindingViewModel objects. Could we just define these methods directly in the KeyBindingViewModel? This sounds like a pain, but I think it would involve KBVM to store a reference to stuff that ActionsVM owns.

The way I see it, the ActionsViewModel owns the list of KeyBindingViewModels, and so it makes sense for the ActionsVM to be listening to events from the kbdVMs... Why would we want to have a reference to the ActionsVM within each kdbVM?

src/cascadia/TerminalSettingsEditor/Actions.xaml

carlos-zamora · 2022-10-26T20:06:02Z

src/cascadia/TerminalSettingsEditor/Actions.cpp

@@ -4,108 +4,14 @@
 #include "pch.h"


Note to other reviewers: go to ... --> "View File". This file is super short now and is much easier to review that way.

carlos-zamora

Yup, a lot of this is moving things around. Makes sense. Thanks!

Edit: I did download the PR and play around the actions page for a bit. Feels pretty normal, so that's good.

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

Fixes a regression from the actions MVVM change in #14292 - attempting to overwrite a keybinding was displaying a warning but propagating the change before the user acknowledged it. The overwrite key binding warning in the SUI works like before Closes #17754

Fixes a regression from the actions MVVM change in #14292 - attempting to overwrite a keybinding was displaying a warning but propagating the change before the user acknowledged it. The overwrite key binding warning in the SUI works like before Closes #17754 (cherry picked from commit ce92b18) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSF01Y Service-Version: 1.21

PankajBhojwani added 3 commits October 18, 2022 12:43

boilerplate

d604ab0

all works except background

9363254

background

f5bc3d4

carlos-zamora reviewed Oct 26, 2022

View reviewed changes

PankajBhojwani and others added 3 commits October 27, 2022 16:42

nits first

f87eac1

rename

035ed88

Migrate spelling-0.0.21 changes from main

d088ae0

carlos-zamora approved these changes Nov 11, 2022

View reviewed changes

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp Show resolved Hide resolved

zadjii-msft added the zBugBash-Consider label Dec 6, 2022

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Dec 7, 2022

ghost requested review from zadjii-msft, DHowett and lhecker December 7, 2022 17:02

carlos-zamora added a commit that referenced this pull request Dec 14, 2022

PRE-MERGE #14292 Implement MVVM for the Actions page

3be7a78

conflict

338a05b

zadjii-msft modified the milestones: Terminal v1.17, Terminal v1.18 Jan 10, 2023

zadjii-msft removed the zBugBash-Consider label Apr 27, 2023

microsoft-github-policy-service bot requested a review from carlos-zamora April 27, 2023 21:51

zadjii-msft modified the milestones: Terminal v1.18, Terminal v1.19 Jul 5, 2023

PankajBhojwani added 2 commits February 8, 2024 14:00

Conflict

e43dc1e

update

ab12137

zadjii-msft approved these changes Feb 12, 2024

View reviewed changes

microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Feb 12, 2024

PankajBhojwani added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 13, 2024

microsoft-github-policy-service bot merged commit f30cbef into main Feb 13, 2024
20 checks passed

microsoft-github-policy-service bot deleted the dev/pabhoj/actions_vm branch February 13, 2024 19:25

DHowett mentioned this pull request Aug 21, 2024

The "Overwrite key binding" warning is very broken #17754

Closed

PankajBhojwani mentioned this pull request Aug 21, 2024

Fix overwrite key binding warning in the SUI #17763

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement MVVM for the Actions page #14292

Implement MVVM for the Actions page #14292

PankajBhojwani commented Oct 25, 2022 •

edited

Loading

carlos-zamora Oct 26, 2022

carlos-zamora Oct 26, 2022

PankajBhojwani Oct 27, 2022

carlos-zamora Oct 26, 2022

carlos-zamora left a comment •

edited

Loading

Implement MVVM for the Actions page #14292

Implement MVVM for the Actions page #14292

Conversation

PankajBhojwani commented Oct 25, 2022 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

carlos-zamora Oct 26, 2022

Choose a reason for hiding this comment

carlos-zamora Oct 26, 2022

Choose a reason for hiding this comment

PankajBhojwani Oct 27, 2022

Choose a reason for hiding this comment

carlos-zamora Oct 26, 2022

Choose a reason for hiding this comment

carlos-zamora left a comment • edited Loading

Choose a reason for hiding this comment

PankajBhojwani commented Oct 25, 2022 •

edited

Loading

carlos-zamora left a comment •

edited

Loading