Repository: microsoft/terminal (language: cpp)

Issue:
[1.21 Canary] User actions have IDs added to them in the JSON
Unclear which version exactly. Was working in between `main` and `1.21.1094.0`, I think. mostly trying to debug #17089. And look at that, all my actions have `id`'s added to them (I did not add these):

```json
        {
            "command":
            {
                "action": "sendInput",
                "input": "git status"
            },
            "id": "User.sendInput.B1352E5E"
        },
        {
            "command":
            {
                "action": "selectOutput",
                "direction": "prev"
            },
            "id": "User.selectOutput.D3F0B923",
            "keys": "ctrl+shift+up"
        },
        {
            "command":
            {
                "action": "splitPane"
            },
            "id": "User.splitPane.9AF9A264"
        },
        {
            "command":
            {
                "action": "selectOutput",
                "direction": "next"
            },
            "id": "User.selectOutput.2A0DA8E0",
            "keys": "ctrl+shift+down"
        },
        {
            "command":
            {
                "action": "selectCommand",
                "direction": "next"
            },
            "id": "User.selectCommand.2A0DA8E0",
            "keys": "ctrl+alt+shift+down"
        },
        {
            "command":
            {
                "action": "selectCommand",
                "direction": "next"
            },
            "keys": "ctrl+g"
        },
        {
            "command":
            {
                "action": "selectCommand",
                "direction": "prev"
            },
            "id": "User.selectCommand.D3F0B923",
            "keys": "ctrl+alt+shift+up"
        },
```


Discussion / hints:
This is by design and mentioned in Pankaj's action IDs spec. This is the way that we will give durable IDs to user-originated actions so that we can pull apart key/button/menu bindings and actual commands.

If you're expecting something different, we should have a team sync about it!

PR #16904, spec #16816
> As for user-specified commands, if no `id` is set, we will auto-generate one for that command based on the action and any additional arguments. For example, the `split pane right` command above might result in an autogenerated id `User.SplitPaneRight`.

Ah, so, this might be under spec'd? I'd assume that we generate IDs for those commands, but _not_ reserialize those "generated" IDs. So that people don't need to know anything about the existence of IDs, unless they want to add the action to {{menu}}
> We will then restructure ... users' settings files ...:
> * ...
>    * There will now be one json block ... which will also contain the `id`.
I think it's important for the IDs to become stable at the first opportunity, otherwise somebody could add an action to the menu with its internal ID, and then change something about it (which generates a new ID) and break the link with the one in the menu.
> otherwise somebody could add an action to the menu with its internal ID

How? If we don't give the action an ID in the json, then how would they be using that action's internally generated ID somewhere else in the settings?

(unless you're assuming that there's some UI-based way of doing that sometime in the next 10 years)
The reason we're tackling action things _now_ is to enable UI-based stuff in 1.21. Is there a problem with writing IDs to the user's settings?
> Is there a problem with writing IDs to the user's settings

It's extremely noisy, if users aren't using that feature (which none will be in 1.21)

(unless actions in the new tab menu et. al will actually be landing in 1.21, which seems like a lot of a lift in the next 7 days)
I've got a couple thoughts...

- The JSON file is much less of a "UI" than it used to be, which frees us up to do things like this. That's not to say it's not a UI.
- Getting to the ideal form of the settings file early is fine, if we're going to get there eventually.
- It's like profiles that only have names: we _always_ serialize GUIDs for them, because that's the only durable way to refer to them.
- We made a mistake making action names or values into the hash keys, because it is complicated when you're using a resource or when you want to bind an entire action to multiple keys[^1] and stuff like that. Putting the durable ID up front immediately eliminates all of those concerns.

[^1]: Right now, I think you need N copies of the action (one per key) and if you want to change what it does you need to update all N copies (right?) That's a scenario _today_ that would benefit from durable IDs.
> Right now, I think you need N copies of the action (one per key) and if you want to change what it does you need to update all N copies (right?) 

We don't allow for actions anywhere other than in `actions` right now, actually. We've been holding off adding support for actions literally anywhere else _until_ we got action IDs. New Tab Menu? We explicitly skipped actions in the v1 PR for that, because this wasn't done yet. 

It feels like the point in the Command Palette spec where we were thinking "Okay, all actions need to be given a name if they want to show up in the command palette", and we decided immediately after: "what if we could just generate names for everything"? 

Users don't need to known about the action IDs, certainly not in 1.21. And if we do add a "new tab menu" SUI page in 1.22, and a user wants to add one of their custom actions then - we can always decide to commit that ID to the file _then_. 
> We don't allow for actions anywhere other than in actions right now, actually.

That's not required for my scenario to exist!

![image](https://github.com/microsoft/terminal/assets/189190/150c8e27-0ff1-4216-b8ea-c0826d29e9a4)

What if I want to make alt-shift-d/e/l `split: left` instead? Gotta edit all three.

If you want to change the spec, propose it and talk to @PankajBhojwani about it. We're going to have action IDs eventually, so why pull them now?
The thing is, the immediate next PR is going to degranulate the actions from the key bindings in the settings file, so there's a map of keys to actions and a map of actions to their actual commands. That practically requires action IDs as the linkage token between them. We should meet to go over the final goal of the spec if we're misaligned on that!

Fix patch:
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
index 37112fe76b6..1deaa9eb50e 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -506,7 +506,7 @@ bool SettingsLoader::FixupUserSettings()
 
     // we need to generate an ID for a command in the user settings if it doesn't already have one
     auto actionMap{ winrt::get_self<ActionMap>(userSettings.globals->ActionMap()) };
-    fixedUp = actionMap->GenerateIDsForActions() || fixedUp;
+    actionMap->GenerateIDsForActions();
 
     return fixedUp;
 }
diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp
index c20be960a67..a32ed4efa79 100644
--- a/src/cascadia/TerminalSettingsModel/Command.cpp
+++ b/src/cascadia/TerminalSettingsModel/Command.cpp
@@ -129,6 +129,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty())
             {
                 _ID = generatedID;
+                _IDWasGenerated = true;
                 return true;
             }
         }
@@ -445,7 +446,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             Json::Value cmdJson{ Json::ValueType::objectValue };
             JsonUtils::SetValueForKey(cmdJson, IconKey, _iconPath);
             JsonUtils::SetValueForKey(cmdJson, NameKey, _name);
-            if (!_ID.empty())
+            if (!_ID.empty() && !_IDWasGenerated)
             {
                 JsonUtils::SetValueForKey(cmdJson, IDKey, _ID);
             }
diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h
index f22e35348c7..a6821f55f35 100644
--- a/src/cascadia/TerminalSettingsModel/Command.h
+++ b/src/cascadia/TerminalSettingsModel/Command.h
@@ -88,6 +88,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         std::vector<Control::KeyChord> _keyMappings;
         std::optional<std::wstring> _name;
         std::wstring _ID;
+        bool _IDWasGenerated{ false };
         std::optional<std::wstring> _iconPath;
         bool _nestedCommand{ false };
 
diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp
index 8d9c55b6f89..82f9f924de3 100644
--- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp
+++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp
@@ -972,36 +972,17 @@ namespace SettingsModelUnitTests
             ]
         })" };
 
-        // Key differences: - the sendInput action now has a generated ID
-        //                  - this generated ID was created at the time of writing this test,
-        //                    and should remain robust (i.e. every time we hash the args we should get the same result)
-        static constexpr std::string_view newSettingsJson{ R"(
-        {
-            "actions": [
-                {
-                    "name": "foo",
-                    "command": { "action": "sendInput", "input": "just some input" },
-                    "keys": "ctrl+shift+w",
-                    "id" : "User.sendInput.)" SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH R"("
-                }
-            ]
-        })" };
+        implementation::SettingsLoader loader{ oldSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) };
+        loader.MergeInboxIntoUserSettings();
+        loader.FinalizeLayering();
+        loader.FixupUserSettings();
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+        const auto oldResult{ settings->ToJson() };
+        const auto sendInputCmd = settings->ActionMap().GetActionByKeyChord(KeyChord{ true, false, true, false, 87, 0 });
 
-        implementation::SettingsLoader oldLoader{ oldSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) };
-        oldLoader.MergeInboxIntoUserSettings();
-        oldLoader.FinalizeLayering();
-        VERIFY_IS_TRUE(oldLoader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
-        const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(oldLoader));
-        const auto oldResult{ oldSettings->ToJson() };
-
-        implementation::SettingsLoader newLoader{ newSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) };
-        newLoader.MergeInboxIntoUserSettings();
-        newLoader.FinalizeLayering();
-        newLoader.FixupUserSettings();
-        const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
-        const auto newResult{ newSettings->ToJson() };
+        std::string_view expectedID{ R"(User.sendInput.)" SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH };
 
-        VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult));
+        VERIFY_ARE_EQUAL(sendInputCmd.ID(), winrt::to_hstring(expectedID));
     }
 
     void SerializationTests::NoGeneratedIDsForIterableAndNestedCommands()
@@ -1068,17 +1049,20 @@ namespace SettingsModelUnitTests
         implementation::SettingsLoader loader1{ settingsJson1, implementation::LoadStringResource(IDR_DEFAULTS) };
         loader1.MergeInboxIntoUserSettings();
         loader1.FinalizeLayering();
-        VERIFY_IS_TRUE(loader1.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+        loader1.FixupUserSettings();
         const auto settings1 = winrt::make_self<implementation::CascadiaSettings>(std::move(loader1));
         const auto result1{ settings1->ToJson() };
 
         implementation::SettingsLoader loader2{ settingsJson2, implementation::LoadStringResource(IDR_DEFAULTS) };
         loader2.MergeInboxIntoUserSettings();
         loader2.FinalizeLayering();
-        VERIFY_IS_TRUE(loader2.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+        loader2.FixupUserSettings();
         const auto settings2 = winrt::make_self<implementation::CascadiaSettings>(std::move(loader2));
         const auto result2{ settings2->ToJson() };
 
-        VERIFY_ARE_EQUAL(toString(result1), toString(result2));
+        const auto sendInputCmd1 = settings1->ActionMap().GetActionByKeyChord(KeyChord{ true, false, true, false, 87, 0 });
+        const auto sendInputCmd2 = settings2->ActionMap().GetActionByKeyChord(KeyChord{ true, false, true, false, 87, 0 });
+
+        VERIFY_ARE_EQUAL(sendInputCmd1.ID(), sendInputCmd1.ID());
     }
 }


Test patch: