Add reflection to the ActionArgs in the settings model #18915

PankajBhojwani · 2025-05-14T23:12:45Z

Summary of the Pull Request

Implements reflection to the various ActionArg types in the settings model, which allows these structs to provide information about themselves (i.e. what args they contain and what types they are). This is necessary as a pre-requisite for the Settings Editor to display and modify these arg values.

Detailed Description of the Pull Request / Additional comments

The IActionArgs interface now has additional methods:
- Get the number of args
- Get/Set an arg at a specific index
- Get the arg description of an arg at a specific index; the arg description contains:
  - name of the arg
  - type of the arg
  - whether the arg is required
  - a tag, this is to cover special cases (for example the ColorScheme argument is technically of type "string", but only allows specific values)
All the macros in ActionArgsMagic have been updated to support the new interface
ActionMap has been updated to support adding/editing/deleting actions and keybindings from outside the SettingsModel
- It now also tracks for ID changes to commands and updates accordingly
Command has been updated to allow setting its values from outside the SettingsModel
EnumMappings have been added to various ActionArg enums that weren't there before

Validation Steps Performed

Bug bashed in conjunction with #18917

PR Checklist

Closes #xxx
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)

carlos-zamora · 2025-05-15T16:28:40Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.idl

+        static Windows.Foundation.Collections.IMap<Microsoft.Terminal.Settings.Model.ShortcutAction, String> AvailableShortcutActionsAndNames { get; };
+        static IActionArgs GetEmptyArgsForAction(Microsoft.Terminal.Settings.Model.ShortcutAction shortcutAction);
+


ActionMap already contains:

Windows.Foundation.Collections.IMapView<String, ActionAndArgs> AvailableActions { get; };

I feel like these make more sense to be over there

I would swing hard in the other direction actually.

Since they're static, they don't need to be on an existing class at all.

We could put them on a new static class. That way nobody has to instantiate one OR know about CascadiaSettings to do it.

static runtimeclass ActionArgFactory { static ...; }

src/cascadia/TerminalSettingsModel/Command.cpp

carlos-zamora · 2025-05-15T20:41:48Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

+            _ActionMap.erase(oldID);
+            _ActionMap.emplace(newID, senderCmd);
+
+            // update _KeyMap so that all keys that pointed to the old ID now point to the new ID


We should probably do the same thing for action IDs in the new tab menu (not accessible via ActionMap, but it's something we'll need).

src/cascadia/TerminalSettingsModel/ActionArgs.idl

carlos-zamora · 2025-05-15T20:48:36Z

src/cascadia/TerminalSettingsModel/ActionArgsMagic.h

    _##name{ name##Param },

+// append this argument's description to the internal vector
+#define APPEND_ARG_DESCRIPTION(type, name, jsonKey, required, tag, ...) \
+    _argDescriptions.push_back({ L## #name, L## #type, std::wstring_view(L## #required) != L"false", tag });


std::wstring_view(L## #required) != L"false"

wait... why are we converting/comparing required as a string? It looks like it's already a bool in the other macros. Can't we just pass the bool directly?

Ah, that's because the term required in these macros is a little misleading - it's not an actual bool, it's a validation check. For example, the ResizeDirection arg has the required parameter as args->ResizeDirection() == ResizeDirection::None, which checks the current actual value of the arg, it's not a general "this arg is required" bool.

Tbh we should probably find a way at some point to transfer over the actual validation check in the ArgDescription so the Settings Editor can use it and display correct error messaging, but for now I think it makes sense to just go with this

src/cascadia/TerminalSettingsModel/ActionArgs.h

carlos-zamora · 2025-05-15T20:59:46Z

src/cascadia/TerminalSettingsModel/ActionArgs.h

-        WINRT_PROPERTY(Model::INewContentArgs, ContentArgs, nullptr);
+        WINRT_PROPERTY(Model::INewContentArgs, ContentArgs, Model::NewTerminalArgs{});


Huh, can we keep it as nullptr?

For now I am leaving the "allow editing different new content types (like creating a snippets pane or a markdown pane etc)" as a future todo, so for that reason we need to initialize this as a NewTerminalArgs here but it shouldn't change anything functionally anywhere else

DHowett

noted

DHowett · 2025-05-15T22:04:04Z

src/cascadia/TerminalSettingsModel/ActionArgs.idl

@@ -5,12 +5,32 @@ import "Command.idl";

 namespace Microsoft.Terminal.Settings.Model
 {
+    enum ArgTag


nit: ArgTypeHint

DHowett · 2025-05-15T22:04:34Z

src/cascadia/TerminalSettingsModel/ActionArgs.idl

+        ColorScheme
+    };
+
+    struct ArgDescription


very-nit: more commonly something like this would be called a Descriptor

DHowett · 2025-05-15T22:16:54Z

src/cascadia/TerminalSettingsModel/ActionArgs.idl

@@ -168,6 +188,11 @@ namespace Microsoft.Terminal.Settings.Model
        UInt64 ContentId{ get; set; };

        String ToCommandline();
+
+        UInt32 GetArgCount();


It might be better to put these four methods in their own interface (IActionArgsDescriptorAccess), and inherit that interface in IActionArgs and NewTerminalArgs.

Right now, they look the same but they cannot be used in the same way.

If it is literally the same interface, you can unify any codepaths that try to access INewTerminalArgs.GetArgCount and IActionArgs.GetArgCount (et al).

The true benefit of IDL is that you can define interfaces that encapsulate common behavior, after all. :)

carlos-zamora · 2025-05-20T19:38:22Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -741,6 +816,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
        }
    }

+    void ActionMap::AddKeyBinding(Control::KeyChord keys, const winrt::hstring& cmdID)
+    {
+        _KeyMap.insert_or_assign(keys, cmdID);


Suggested change

_KeyMap.insert_or_assign(keys, cmdID);

_KeyMap.insert_or_assign(keys, cmdID);

_changeLog.emplace(KeysKey);

We probably want to update the settings logger here to say that we've changed keys.

carlos-zamora · 2025-05-20T20:13:25Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -757,6 +838,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
        AddAction(*cmd, keys);
    }

+    void ActionMap::DeleteUserCommand(const winrt::hstring& cmdID)
+    {
+        _ActionMap.erase(cmdID);


Suggested change

_ActionMap.erase(cmdID);

_ActionMap.erase(cmdID);

_changeLog.emplace(IDKey)

This one I'm iffy on, so I want to discuss here.

Currently, there isn't a place where we're tracking if a user added an ID. In the JSON, the user would be using "id" in two cases for Action Map:

create a new action in the "actions" block (i.e. {"command": "terminalChat", "id": "User.terminalChat"})

That's fine? We care about action arg usage and that's already being handled.

create a key binding in the "keybindings"block (i.e. {"id": "User.terminalChat", "keys": "ctrl+?})

That's fine too. We care about "keys" usage and that's already being handled.

(related: "id" is used in the new tab menu, but we're just tracking "action" there which is sufficient)

Long-term, however, we want _changeLog to be a way for us to know if the user changed anything. So maybe we do want to apply the code suggestion and update case 1 to add "id" too? Thoughts?

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

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

@@ -740,4 +740,274 @@
  <data name="OpenCWDCommandKey" xml:space="preserve">
    <value>Open current working directory</value>
  </data>
+  <data name="CopyText" xml:space="preserve">


Lightly pushing back on the capitalization of these. We should probably capitalize the first letter of the first word and that's it.

carlos-zamora · 2025-05-21T00:08:39Z

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

+  <data name="QuickFix" xml:space="preserve">
+    <value>Quick Fix</value>
+  </data>


So, the type here is "Quick Fix", but there's no action args and the generated name is "Open quick fix menu". Should we just use the generated name here?

Generalize this comment to all the other actions that have no args. I think the list can basically be found in ActionAndArgs.cpp by looking at GenerateName()

carlos-zamora · 2025-05-21T00:36:22Z

src/cascadia/TerminalSettingsModel/ActionArgs.h

+////////////////////////////////////////////////////////////////////////////////
+#define NEW_TERMINAL_ARGS(X)                                                                                                     \
+    X(winrt::hstring, Commandline, "commandline", false, ArgTag::None, L"")                                                      \
+    X(winrt::hstring, StartingDirectory, "startingDirectory", false, ArgTag::None, L"")                                          \


Suggested change

X(winrt::hstring, StartingDirectory, "startingDirectory", false, ArgTag::None, L"") \

X(winrt::hstring, StartingDirectory, "startingDirectory", false, ArgTag::FilePath, L"") \

Or maybe we need a new one for directory path?

…/pabhoj/settings_model_reflection

PankajBhojwani added 2 commits May 14, 2025 13:33

settings model reflection

d973836

use x-macro here instead

ce8b31b

PankajBhojwani mentioned this pull request May 15, 2025

Allow editing actions in the settings UI #18917

Draft

5 tasks

format

72e9b64

carlos-zamora requested changes May 15, 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 15, 2025

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

DHowett reviewed May 15, 2025

View reviewed changes

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

PRE-MERGE #18915 Add reflection to the ActionArgs in the settings model

49a2074

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 May 16, 2025

first nits

9618831

carlos-zamora reviewed May 20, 2025

View reviewed changes

carlos-zamora reviewed May 21, 2025

View reviewed changes

PankajBhojwani added 3 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

change this back

18830be

		static Windows.Foundation.Collections.IMap<Microsoft.Terminal.Settings.Model.ShortcutAction, String> AvailableShortcutActionsAndNames { get; };
		static IActionArgs GetEmptyArgsForAction(Microsoft.Terminal.Settings.Model.ShortcutAction shortcutAction);

		WINRT_PROPERTY(Model::INewContentArgs, ContentArgs, nullptr);
		WINRT_PROPERTY(Model::INewContentArgs, ContentArgs, Model::NewTerminalArgs{});

	_KeyMap.insert_or_assign(keys, cmdID);
	_KeyMap.insert_or_assign(keys, cmdID);
	_changeLog.emplace(KeysKey);

	_ActionMap.erase(cmdID);
	_ActionMap.erase(cmdID);
	_changeLog.emplace(IDKey)

	X(winrt::hstring, StartingDirectory, "startingDirectory", false, ArgTag::None, L"") \
	X(winrt::hstring, StartingDirectory, "startingDirectory", false, ArgTag::FilePath, L"") \

Add reflection to the ActionArgs in the settings model #18915

Are you sure you want to change the base?

Add reflection to the ActionArgs in the settings model #18915

Uh oh!

Conversation

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

Uh oh!

Summary of the Pull Request

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

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!

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!

DHowett 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!

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!

Uh oh!

PankajBhojwani commented May 14, 2025 •

edited

Loading