Allow editing actions in the settings UI #18917

PankajBhojwani · 2025-05-15T18:59:32Z

Summary of the Pull Request

Targets #18915

The actions page now has a list of all the commands (default, user, fragments etc) and clicking a command from that page brings you to an "Edit action" page where you can fully view and edit both the action type and any additional arguments.

One todo remaining that shouldn't block reviews:

Accessibility fixes

Detailed Description of the Pull Request / Additional comments

Actions View Model

Added several new view models
- CommandViewModel (view model for a Command), a list of these is created and managed by ActionsViewModel
- ActionArgsViewModel (view model for an ActionArgs), created and managed by CommandViewModel
- ArgWrapper (view model for each individual argument inside an ActionArgs), created and managed by ActionArgsViewModel

Actions page

No longer a list of only keybindings, instead it is a list of every command Terminal knows about

EditAction page

New page that you get to by clicking a command from the Actions page
Bound to a CommandViewModel
Allows editing the type of shortcut action and the command name
Depending on the shortcut action, displays a list of additional arguments allowed for the command with the appropriate templating (bool arguments are switches, flags are checkboxes etc)

Validation Steps Performed

Bug bashed

PR Checklist

Closes Add an actions editor to the Settings UI #19019
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated (if necessary)

…ettings_actions_editor

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

…ettings_actions_editor

carlos-zamora

Other feedback:

"Clear buffer" action is being shown as "Clear mark" (binding to ctrl+shift+k shown correctly)

src/cascadia/TerminalSettingsEditor/Actions.xaml

carlos-zamora · 2025-05-20T21:09:43Z

src/cascadia/TerminalSettingsEditor/Actions.xaml

-                        <!--  Key Chord Text  -->
-                        <Border Grid.Column="1"
-                                Padding="2,0,2,0"
-                                HorizontalAlignment="Right"
-                                VerticalAlignment="Center"
-                                Style="{ThemeResource KeyChordBorderStyle}"
-                                Visibility="{x:Bind mtu:Converters.InvertedBooleanToVisibility(IsInEditMode), Mode=OneWay}">
-
-                            <TextBlock FontSize="14"
-                                       Style="{ThemeResource KeyChordTextBlockStyle}"
-                                       Text="{x:Bind KeyChordText, Mode=OneWay}"
-                                       TextWrapping="WrapWholeWords" />
-                        </Border>


Key bindings aren't shown anymore

This is intentional for now since some commands can have multiple keybindings bound to them. As a follow up we can convert these list view items into expanders that show 1-3 of the associated keybindings

src/cascadia/TerminalSettingsEditor/Actions.xaml

src/cascadia/TerminalSettingsEditor/EditAction.xaml

carlos-zamora

It'd be really nice if you added a comment by each of the ArgWrapper data templates saying what's a good action type and arg combo to test this out. I've been manually going through ActionArgs.h to figure this out. Not the end of the world, but it'd be a nice QOL improvement 😁

Almost done reviewing this. Just need to go over ActionsViewModel

src/cascadia/TerminalSettingsEditor/EditAction.xaml

carlos-zamora · 2025-05-21T00:57:11Z

src/cascadia/TerminalSettingsEditor/EditAction.xaml

+                       Grid.Row="3"
+                       Grid.Column="0"
+                       VerticalAlignment="Center" />
+            <ListView Grid.Row="3"


Consider ItemsRepeater instead of ListView. We're not using the functionality of the ListView, really. We just want a stack panel layout of our own controls.

ItemsControl works too. I used it over in my extensions page:

terminal/src/cascadia/TerminalSettingsEditor/Extensions.xaml

Line 404 in a282164

<ItemsControl x:Name="ActiveExtensionsList"

We do use ListView.Header which neither ItemsRepeater nor ItemsControl have, we could mimic it with a Grid or something but might as well just use ListView.Header I think

src/cascadia/TerminalSettingsEditor/EditAction.xaml

carlos-zamora

Alright! Finally finished reviewing everything. Left a bunch of comments all around. Tried to test things out with the bug bash build as I went and did some accessibility testing too. I recommend that after you get the accessibility stuff in, you run it through Accessibility Insights' FastPass to check for common issues. That said, I think once you fix the comments I added in, you should be pretty much good to go there.

There's a bit more cacheing that can be done. I think it's worth it since we've got A TON of commands/actions. There was also a concern I had about null-checking somewhere in there so we probably want to fix that.

The ArgsWrapper stuff is really cool. Good work all-around dude 😊. Excited to see this land when you make the changes. We're close!

carlos-zamora · 2025-05-21T17:07:04Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

@@ -18,383 +24,1194 @@ using namespace winrt::Windows::UI::Xaml::Data;
 using namespace winrt::Windows::UI::Xaml::Navigation;
 using namespace winrt::Microsoft::Terminal::Settings::Model;

+// todo:


Fair. We should file an issue and link it here for tracking.

I wonder if in the near term, we could add support for them, but just expose a text box. It wouldn't be ideal, but we'd still show the actions in the main list and users would be able to view/modify them. Then a cleaner UI could be added in later (possibly even by the community).

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

carlos-zamora · 2025-05-21T17:21:10Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

+    std::vector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> enumList;                                                                          \
+    const auto mappings = winrt::Microsoft::Terminal::Settings::Model::EnumMappings::enumMappingsName();                                                    \
+    enumType unboxedValue;                                                                                                                                  \
+    auto nullEntry = winrt::make<winrt::Microsoft::Terminal::Settings::Editor::implementation::EnumEntry>(RS_(L"Actions_NullEnumValue"), nullptr);          \


Somewhere I mentioned that we should provide a more specific string for the null value. Looks like this is the spot to do it. You can probably just move the resource ID into the macro definition

There are only two nullable args here that this applies to - CopyFormatting (in CopyText) and TabSwitcherMode (in PrevTab/NextTab) and setting those args to null means the same for both of them: follow the global setting. Any ideas for the more specific string for either?

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

carlos-zamora · 2025-05-21T20:35:52Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h

+        // unboxing functions
+        winrt::hstring UnboxString(const Windows::Foundation::IInspectable& value);
+        winrt::hstring UnboxGuid(const Windows::Foundation::IInspectable& value);
+        int32_t UnboxInt32(const Windows::Foundation::IInspectable& value);
+        float UnboxInt32Optional(const Windows::Foundation::IInspectable& value);
+        uint32_t UnboxUInt32(const Windows::Foundation::IInspectable& value);
+        float UnboxUInt32Optional(const Windows::Foundation::IInspectable& value);
+        float UnboxUInt64(const Windows::Foundation::IInspectable& value);
+        float UnboxFloat(const Windows::Foundation::IInspectable& value);
+        bool UnboxBool(const Windows::Foundation::IInspectable& value);
+        winrt::Windows::Foundation::IReference<bool> UnboxBoolOptional(const Windows::Foundation::IInspectable& value);
+        winrt::Windows::Foundation::IReference<Microsoft::Terminal::Core::Color> UnboxTerminalCoreColorOptional(const Windows::Foundation::IInspectable& value);
+        winrt::Windows::Foundation::IReference<Microsoft::Terminal::Core::Color> UnboxWindowsUIColorOptional(const Windows::Foundation::IInspectable& value);
+
+        // bind back functions
+        void StringBindBack(const winrt::hstring& newValue);
+        void GuidBindBack(const winrt::hstring& newValue);
+        void Int32BindBack(const double newValue);
+        void Int32OptionalBindBack(const double newValue);
+        void UInt32BindBack(const double newValue);
+        void UInt32OptionalBindBack(const double newValue);
+        void UInt64BindBack(const double newValue);
+        void FloatBindBack(const double newValue);
+        void BoolOptionalBindBack(const Windows::Foundation::IReference<bool> newValue);
+        void TerminalCoreColorBindBack(const winrt::Windows::Foundation::IReference<Microsoft::Terminal::Core::Color> newValue);
+        void WindowsUIColorBindBack(const winrt::Windows::Foundation::IReference<Microsoft::Terminal::Core::Color> newValue);


These are all pretty simple functions, and that's great. Would there be any benefit to moving these over to be converters so they can be used throughout the project? Or am I overengineering it? 😅

The BindBack functions all need to stay here since it involves modifying the underlying data, so I figured it would be cleaner if we just leave the Unbox functions here too

Happy to move just the unbox ones over to converters if we feel strongly about it though

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

…ettings_actions_editor

carlos-zamora · 2025-05-30T00:14:55Z

Oh! Also add in the "New" badge. You should be able to add it like the same way I did for the Extensions page. Just look at ExtensionsViewModel::DisplayBadge and how it's used/set-up.

…ettings_actions_editor

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h

…ettings_actions_editor

PankajBhojwani added 4 commits May 14, 2025 13:35

settings actions editor

2f6108d

Merge branch 'dev/pabhoj/settings_model_reflection' into dev/pabhoj/s…

acad388

…ettings_actions_editor

remove old actions page

63ec868

remove old stuff

8d0f947

github-advanced-security bot found potential problems May 15, 2025

View reviewed changes

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp Fixed Show fixed Hide fixed

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp Fixed Show fixed Hide fixed

PankajBhojwani mentioned this pull request May 15, 2025

Add reflection to the ActionArgs in the settings model #18915

Open

4 tasks

PankajBhojwani added 2 commits May 15, 2025 12:01

Merge branch 'dev/pabhoj/settings_model_reflection' into dev/pabhoj/s…

23a0432

…ettings_actions_editor

format

eecb32d

This comment has been minimized.

Sign in to view

carlos-zamora added the zBugBash-Consider label May 15, 2025

carlos-zamora added a commit that referenced this pull request May 16, 2025

PRE-MERGE #18917 Allow editing actions in the settings UI

3c8e262

Merge branch 'dev/pabhoj/settings_model_reflection' into dev/pabhoj/s…

5b617e0

…ettings_actions_editor

carlos-zamora reviewed May 20, 2025

View reviewed changes

carlos-zamora requested changes May 21, 2025

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback label May 21, 2025

carlos-zamora requested changes May 21, 2025

View reviewed changes

microsoft-github-policy-service bot added the No-Recent-Activity label May 28, 2025

carlos-zamora removed the No-Recent-Activity label May 29, 2025

Merge branch 'dev/pabhoj/settings_model_reflection' into dev/pabhoj/s…

ed25720

…ettings_actions_editor

PankajBhojwani added 3 commits June 3, 2025 14:21

Merge branch 'dev/pabhoj/settings_model_reflection' into dev/pabhoj/s…

4a972cc

…ettings_actions_editor

post merge updates

c55ba1b

spelling

4e87177

microsoft-github-policy-service bot removed the Needs-Author-Feedback label Jun 3, 2025

github-advanced-security bot found potential problems Jun 3, 2025

View reviewed changes

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

how about now

21e1e15

carlos-zamora mentioned this pull request Jun 4, 2025

[UX] Settings UI refinements #19001

Open

add examples, remove some stuff

5dde183

PankajBhojwani added 5 commits June 10, 2025 11:50

remvoe more

3e426d7

no more no arg

54b850e

some wrapping

7fac89c

folder picker

2e5c54c

abstract

f853aa4

github-advanced-security bot found potential problems Jun 11, 2025

View reviewed changes

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h Fixed Show fixed Hide fixed

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h Fixed Show fixed Hide fixed

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

we're not listening to this

9fa1c94

This comment has been minimized.

Sign in to view

PankajBhojwani added 2 commits June 13, 2025 13:19

Merge branch 'dev/pabhoj/settings_model_reflection' into dev/pabhoj/s…

78395fb

…ettings_actions_editor

keyboard nav, event handler cleanup

f601fc7

This comment has been minimized.

Sign in to view

PankajBhojwani added 14 commits June 16, 2025 15:56

spell

7db0cd6

uia names

d862d67

...xaml side too

c24035f

automation props, tab stop fixes

bafe0b1

items control here

caedc8f

switch to items control

c47a353

uia text

c4529aa

remove these notifies

b23ce87

proposed shortcut action name

43ad62c

getter only

c24293c

name fixes

ad0f79d

access maps through page vm

849eb1d

assert

cd424f4

init to first item in the enum list

ccc7292

Allow editing actions in the settings UI #18917

Are you sure you want to change the base?

Allow editing actions in the settings UI #18917

Uh oh!

Conversation

PankajBhojwani commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

Targets #18915

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora commented May 30, 2025

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

PankajBhojwani commented May 15, 2025 •

edited

Loading