Allow editing actions in the settings UI #18917

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

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.

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

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!

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

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?

Being able to deduce the value and show what the global setting is would be cool, but I'm not gonna block over that. It's good as is.

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

…/pabhoj/settings_model_reflection

…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.

src/cascadia/TerminalSettingsEditor/EditAction.cpp

src/cascadia/TerminalSettingsEditor/ArgsTemplateSelectors.cpp

lhecker · 2025-09-13T13:25:52Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.idl

+        String UnboxString(Object value);
+        UInt32 UnboxInt32(Object value);
+        Single UnboxInt32Optional(Object value);
+        UInt32 UnboxUInt32(Object value);
+        Single UnboxUInt32Optional(Object value);
+        Single UnboxUInt64(Object value);
+        Single UnboxFloat(Object value);
+        Boolean UnboxBool(Object value);


Do these need to be exposed? Same for the pack functions below. Perhaps others are also not needed.

Yep they need to be exposed for xaml to bind to, since in this case the bindings are being done in data templates

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

src/cascadia/TerminalSettingsEditor/MainPage.cpp

…/pabhoj/settings_actions_editor

DHowett · 2025-10-29T20:18:28Z

/azp run

azure-pipelines · 2025-10-29T20:18:39Z

Azure Pipelines successfully started running 1 pipeline(s).

src/cascadia/UIHelpers/Converters.cpp

src/cascadia/TerminalSettingsModel/Command.cpp

DHowett · 2025-11-03T17:56:24Z

src/cascadia/TerminalSettingsEditor/EnumEntry.idl

+        String FlagName { get; };
+        IInspectable FlagValue { get; };
+        Int32 IntValue { get; };
+        Boolean IsSet;


Careful! EnumEntries are immutable (and can be cached and reused across the settings model) but this makes it seem like FlagEntries are not?

Yeah, FlagEntries allow the flag to be switched on/off, would it help if I rename it so it doesn't mislead us into thinking it should be immutable like EnumEntries? Otherwise we would need 2 FlagEntries per flag value, one for on and one for off

src/cascadia/TerminalSettingsEditor/EnumEntry.h

DHowett

18/27

src/cascadia/TerminalSettingsEditor/Actions.cpp

src/cascadia/TerminalSettingsEditor/ArgsTemplateSelectors.idl

src/cascadia/TerminalSettingsEditor/EditAction.cpp

src/cascadia/TerminalSettingsEditor/ArgsTemplateSelectors.h

src/cascadia/TerminalSettingsEditor/EditAction.cpp

src/cascadia/TerminalSettingsEditor/ArgsTemplateSelectors.cpp

DHowett

22/22! Minor open questions and nits, but overall looking very very good. Well done :)

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalSettingsEditor/Actions.xaml

DHowett · 2025-11-07T21:06:36Z

src/cascadia/TerminalSettingsEditor/Actions.xaml

-                      SelectionMode="None" />
+            <!--  Commands  -->
+            <ListView x:Name="CommandsListView"
+                      Margin="-8,0,0,0"


Why -8? What does this look like when terminal is as narrow as possible?

So it aligns with the disclaimer and the add new action button. Looks fine in narrow:

DHowett · 2025-11-07T21:10:14Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.idl

+        void FloatBindBack(Double newValue);
+        void BoolOptionalBindBack(Windows.Foundation.IReference<Boolean> newValue);
+        void TerminalCoreColorBindBack(Windows.Foundation.IReference<Microsoft.Terminal.Core.Color> newValue);
+        void WindowsUIColorBindBack(Windows.Foundation.IReference<Microsoft.Terminal.Core.Color> newValue);


both the forward and backward bindings for "WindowsUIColor" use Microsoft.Terminal.Core.Color (!)

Yes that's because we use the NullableColorPicker, which only understands Terminal.Core.Color. So the point of these unboxing/bindback functions is to translate between WindowsUIColor and TerminalCoreColor

src/cascadia/TerminalSettingsEditor/ActionsViewModel.idl

DHowett · 2025-11-07T21:44:20Z

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

+    void ArgWrapper::UInt32BindBack(const double newValue)
+    {
+        if (UnboxUInt32(_Value) != newValue)
+        {
+            Value(box_value(static_cast<uint32_t>(newValue)));
+        }
+    }
+
+    void ArgWrapper::UInt32OptionalBindBack(const double newValue)


FYI these two take doubles!

Yep! Xaml's NumberBox isn't happy with other number types, so all of these take doubles and the unbox/bindback functions handle the logic according to what the underlying type actually is/should be

src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp

…/pabhoj/settings_actions_editor

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h

PankajBhojwani added 6 commits May 14, 2025 13:33

settings model reflection

d973836

settings actions editor

2f6108d

use x-macro here instead

ce8b31b

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

Merged

PankajBhojwani added 3 commits May 15, 2025 12:01

format

72e9b64

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

PankajBhojwani added 2 commits May 16, 2025 12:53

first nits

9618831

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

PankajBhojwani added 4 commits May 29, 2025 12:28

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

6557d17

…/pabhoj/settings_model_reflection

init as nullptr

d62ede4

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

ed25720

…ettings_actions_editor

change this back

18830be

PankajBhojwani added 2 commits May 30, 2025 10:55

static actionargfactory class

035b341

rename and separate interface

80924f2

format

2366b3f

lhecker reviewed Sep 13, 2025

View reviewed changes

PankajBhojwani added 3 commits September 25, 2025 15:32

first pass

6cffe9f

formatt

1693773

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

e0352e4

…/pabhoj/settings_actions_editor

PankajBhojwani force-pushed the dev/pabhoj/settings_actions_editor branch from 2e8c282 to e0352e4 Compare September 25, 2025 22:34

PankajBhojwani added 4 commits September 25, 2025 15:52

edit action resource

8b3c12e

function template instead

757bd15

expose int value on enum and flag entry

8d1f280

safer handling

294e2e3

microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Oct 29, 2025

DHowett reviewed Nov 3, 2025

View reviewed changes

first round

3a3ef4e

DHowett requested changes Nov 7, 2025

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 7, 2025

PankajBhojwani added 4 commits November 11, 2025 07:02

mostly renames

1a332c4

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

195c680

…/pabhoj/settings_actions_editor

next pass

69186ed

format

bbdf75d

DHowett reviewed Nov 12, 2025

View reviewed changes

src/cascadia/TerminalSettingsEditor/ActionsViewModel.h Outdated Show resolved Hide resolved

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

PankajBhojwani added 2 commits November 11, 2025 20:45

split size template

7bb10f2

& and move

f4eda1b

carlos-zamora added a commit that referenced this pull request Nov 18, 2025

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

079e070

carlos-zamora added a commit that referenced this pull request Nov 19, 2025

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

ceb291d

PankajBhojwani added 2 commits November 20, 2025 14:10

dedup

3163df0

format

f55a0bb

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

Conversation

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

Uh oh!

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!

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!

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!

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!

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!

Uh oh!

PankajBhojwani commented May 15, 2025 •

edited by DHowett

Loading

PankajBhojwani Nov 5, 2025 •

edited

Loading

PankajBhojwani Nov 10, 2025 •

edited

Loading