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

carlos-zamora · 2025-05-20T23:22:27Z

src/cascadia/TerminalSettingsEditor/EditAction.xaml

+            <DataTemplate x:Key="ListItemTemplate"
+                          x:DataType="local:ArgWrapper">
+                <ListViewItem HorizontalContentAlignment="Stretch" />
+            </DataTemplate>


Looks like ListItemTemplate is not being used anywhere?

It's used in EditAction.cpp!

src/cascadia/TerminalSettingsEditor/EditAction.xaml

carlos-zamora · 2025-05-20T23:44:35Z

src/cascadia/TerminalSettingsEditor/EditAction.xaml

+
+            <DataTemplate x:Key="Int32Template"
+                          x:DataType="local:ArgWrapper">
+                <ListViewItem>


[Keyboard navigation] Tabbing lets us move our focus into 3 spots here (in order): (1) 1st list view item -> (2) 2nd list view item -> (3) number box

I suspect that the redundant list view item is from the first ListViewItem in the data template. I think you should be able to remove it because the data template controls what's inside the ListViewItem already. You can repeat this for all the other data templates here.

As for the other list view item, you'll want to unset it as a tab stop. I think you can do that with ItemContainerStyle on the ListView itself. Create a style with IsTabStop = False and pass it in. That way, the user can tab directly into the number box (or whatever control is being used).

It's worth checking the UIA tree with Accessibility Insights too. Idk if you'll need to change the AccessibilityView on these to hide them from the tree, of if just removing the tab stop is enough.

src/cascadia/TerminalSettingsEditor/EditAction.xaml

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

src/cascadia/TerminalSettingsEditor/EditAction.xaml

+                        <TextBlock Grid.Column="0"
+                                   VerticalAlignment="Center"
+                                   Text="{x:Bind Name}" />
+                        <local:NullableColorPicker x:Uid="Actions_NullableColorPicker"


Null color is "No color"? Hmm... but I'm not sure how to handle this one exactly either. For Tab color, it opens the color picker. But for new terminal args, it... what, uses the theme color?

carlos-zamora · 2025-05-21T00:56:06Z

src/cascadia/TerminalSettingsEditor/EditAction.xaml

+            <TextBlock x:Uid="Actions_Name"
+                       Grid.Row="0"
+                       Grid.Column="0"
+                       VerticalAlignment="Center" />
+            <TextBox Grid.Row="0"
+                     Grid.Column="1"
+                     Width="300"
+                     HorizontalAlignment="Left"
+                     PlaceholderText="{x:Bind ViewModel.DisplayName, Mode=OneWay}"
+                     Text="{x:Bind ViewModel.Name, Mode=TwoWay}" />


Each of these pairs has the same problem I mentioned earlier with a screen reader. Visually, we can tell that the text block is the header for the control. With a screen reader, we don't have that, so we need to set the AutomationProperty.Name manually

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

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

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

+    }                                                                                                                                                       \
+    std::sort(enumList.begin(), enumList.end(), EnumEntryReverseComparator<enumType>());                                                                    \
+    _EnumList = winrt::single_threaded_observable_vector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>(std::move(enumList));                     \
+    _NotifyChanges(L"EnumList", L"EnumValue");


(Regarding this _NotifyChanges() and all the other macros that have a similar one)

Who's listening to these? It looks like we don't have a PropertyChanged handler for specifically these values.

Conversely, shouldn't the property changed events be more specific? Like, shouldn't it notify L## #enumMappingsName L"EnumList" to pass over the specific one?

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

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

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

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


Same here. You can probably move this up into the macro definition to have it be a custom string

carlos-zamora · 2025-05-21T20:31:40Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

+            // even though the arg type is technically a string, we want an enum list for color schemes specifically
+            std::vector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> namesList;
+            const auto currentSchemeName = unbox_value<winrt::hstring>(_Value);
+            auto nullEntry = winrt::make<winrt::Microsoft::Terminal::Settings::Editor::implementation::EnumEntry>(RS_(L"Actions_NullEnumValue"), nullptr);


This may be where you can add a custom value for the null enum value. Maybe hardcode a Name check so that we do a different one depending on the setting?

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

carlos-zamora · 2025-05-21T20:43:53Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

+        // Populate AvailableActionsAndNames
+        _AvailableActionsAndNamesMap = Model::CascadiaSettings::AvailableShortcutActionsAndNames();
+        for (const auto unimplemented : UnimplementedShortcutActions)
+        {
+            _AvailableActionsAndNamesMap.Remove(unimplemented);
+        }


We can probably cache this, right? Looks like CascadiaSettings::AvailableShortcutActionsAndNames does the work each time we call it, then we'll remove the same 2 unimplemented shortcut actions. Maybe we should just store it as a static so we don't regenerate it? It shouldn't change at runtime.

carlos-zamora · 2025-05-21T20:48:51Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

+
+    void ActionsViewModel::AttemptDeleteKeyChord(const Control::KeyChord& keys)
+    {
+        // Update the settings model


Suggested change

// Update the settings model

// Update the settings model

assert(keys);

Thoughts on adding an assert here? If somebody is calling this with an empty keys, they're probably holding it wrong, right? So maybe we should notify the developer that they're not using it right.

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 The original author of the issue/PR needs to come back and respond to something label May 21, 2025

carlos-zamora requested changes May 21, 2025

View reviewed changes

microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label May 28, 2025

carlos-zamora removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further 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 The original author of the issue/PR needs to come back and respond to something 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 4 commits June 16, 2025 15:56

spell

7db0cd6

uia names

d862d67

...xaml side too

c24035f

automation props, tab stop fixes

bafe0b1

	// Update the settings model
	// Update the settings model
	assert(keys);

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!

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!

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!

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!

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!

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!

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!

PankajBhojwani commented May 15, 2025 •

edited

Loading