Add IDs to Commands (#16904)

As laid out in #16816, adds an `ID` field to `Command`. **This first PR only adds IDs for built-in commands in defaults, and generates IDs for user-created commands that don't define an ID.** Also note that for now we **will not** be allowing IDs for iterable/nested commands. The follow-up PR is where we will actually use the IDs by referring to commands with them. Refs #16816 ## Validation Steps Performed User-created commands in the settings file get rewritten with generated IDs
microsoft · Apr 18, 2024 · 06ab6f3 · 06ab6f3
1 parent 643f716
commit 06ab6f3
Show file tree

Hide file tree

Showing 10 changed files with 352 additions and 127 deletions.
diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
@@ -450,6 +450,36 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         return found != GeneratedActionNames.end() ? found->second : L"";
     }
 
+    // Function Description:
+    // - This will generate an ID for this ActionAndArgs, based on the ShortcutAction and the Args
+    // - It will always create the same ID if the ShortcutAction and the Args are the same
+    // - Note: this should only be called for User-created actions
+    // - Example: The "SendInput 'abc'" action will have the generated ID "User.sendInput.<hash of 'abc'>"
+    // Return Value:
+    // - The ID, based on the ShortcutAction and the Args
+    winrt::hstring ActionAndArgs::GenerateID() const
+    {
+        if (_Action != ShortcutAction::Invalid)
+        {
+            auto actionKeyString = ActionToStringMap.find(_Action)->second;
+            auto result = fmt::format(FMT_COMPILE(L"User.{}"), actionKeyString);
+            if (_Args)
+            {
+                // If there are args, we need to append the hash of the args
+                // However, to make it a little more presentable we
+                // 1. truncate the hash to 32 bits
+                // 2. convert it to a hex string
+                // there is a _tiny_ chance of collision because of the truncate but unlikely for
+                // the number of commands a user is expected to have
+                const auto argsHash32 = static_cast<uint32_t>(_Args.Hash() & 0xFFFFFFFF);
+                // {0:X} formats the truncated hash to an uppercase hex string
+                fmt::format_to(std::back_inserter(result), FMT_COMPILE(L".{:X}"), argsHash32);
+            }
+            return winrt::hstring{ result };
+        }
+        return L"";
+    }
+
     winrt::hstring ActionAndArgs::Serialize(const winrt::Windows::Foundation::Collections::IVector<Model::ActionAndArgs>& args)
     {
         Json::Value json{ Json::objectValue };

diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.h b/src/cascadia/TerminalSettingsModel/ActionAndArgs.h
@@ -27,6 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         com_ptr<ActionAndArgs> Copy() const;
 
         hstring GenerateName() const;
+        hstring GenerateID() const;
 
         WINRT_PROPERTY(ShortcutAction, Action, ShortcutAction::Invalid);
         WINRT_PROPERTY(IActionArgs, Args, nullptr);

diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp
@@ -795,6 +795,25 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         return nullptr;
     }
 
+    bool ActionMap::GenerateIDsForActions()
+    {
+        bool fixedUp{ false };
+        for (auto actionPair : _ActionMap)
+        {
+            auto cmdImpl{ winrt::get_self<Command>(actionPair.second) };
+
+            // Note: this function should ONLY be called for the action map in the user's settings file
+            //       this debug assert should verify that for debug builds
+            assert(cmdImpl->Origin() == OriginTag::User);
+
+            if (cmdImpl->ID().empty())
+            {
+                fixedUp = cmdImpl->GenerateID() || fixedUp;
+            }
+        }
+        return fixedUp;
+    }
+
     // Method Description:
     // - Rebinds a key binding to a new key chord
     // Arguments:

diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h
@@ -71,6 +71,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         Json::Value ToJson() const;
 
         // modification
+        bool GenerateIDsForActions();
         bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys);
         void DeleteKeyBinding(const Control::KeyChord& keys);
         void RegisterKeyBinding(Control::KeyChord keys, Model::ActionAndArgs action);

diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -504,6 +504,10 @@ bool SettingsLoader::FixupUserSettings()
         fixedUp = true;
     }
 
+    // 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;
+
     return fixedUp;
 }
 

diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp
@@ -21,6 +21,7 @@ namespace winrt
 }
 
 static constexpr std::string_view NameKey{ "name" };
+static constexpr std::string_view IDKey{ "id" };
 static constexpr std::string_view IconKey{ "icon" };
 static constexpr std::string_view ActionKey{ "command" };
 static constexpr std::string_view IterateOnKey{ "iterateOn" };
@@ -39,7 +40,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         auto command{ winrt::make_self<Command>() };
         command->_name = _name;
-        command->_Origin = OriginTag::User;
+        command->_Origin = _Origin;
+        command->_ID = _ID;
         command->_ActionAndArgs = *get_self<implementation::ActionAndArgs>(_ActionAndArgs)->Copy();
         command->_keyMappings = _keyMappings;
         command->_iconPath = _iconPath;
@@ -114,6 +116,25 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         }
     }
 
+    hstring Command::ID() const noexcept
+    {
+        return hstring{ _ID };
+    }
+
+    bool Command::GenerateID()
+    {
+        if (_ActionAndArgs)
+        {
+            auto actionAndArgsImpl{ winrt::get_self<implementation::ActionAndArgs>(_ActionAndArgs) };
+            if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty())
+            {
+                _ID = generatedID;
+                return true;
+            }
+        }
+        return false;
+    }
+
     void Command::Name(const hstring& value)
     {
         if (!_name.has_value() || _name.value() != value)
@@ -264,6 +285,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         auto result = winrt::make_self<Command>();
         result->_Origin = origin;
+        JsonUtils::GetValueForKey(json, IDKey, result->_ID);
 
         auto nested = false;
         JsonUtils::GetValueForKey(json, IterateOnKey, result->_IterateOn);
@@ -423,6 +445,10 @@ 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())
+            {
+                JsonUtils::SetValueForKey(cmdJson, IDKey, _ID);
+            }
 
             if (_ActionAndArgs)
             {
@@ -443,6 +469,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                     // First iteration also writes icon and name
                     JsonUtils::SetValueForKey(cmdJson, IconKey, _iconPath);
                     JsonUtils::SetValueForKey(cmdJson, NameKey, _name);
+                    if (!_ID.empty())
+                    {
+                        JsonUtils::SetValueForKey(cmdJson, IDKey, _ID);
+                    }
                 }
 
                 if (_ActionAndArgs)

diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h
@@ -61,6 +61,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         hstring Name() const noexcept;
         void Name(const hstring& name);
 
+        hstring ID() const noexcept;
+        bool GenerateID();
+
         Control::KeyChord Keys() const noexcept;
         hstring KeyChordText() const noexcept;
         std::vector<Control::KeyChord> KeyMappings() const noexcept;
@@ -84,6 +87,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command> _subcommands{ nullptr };
         std::vector<Control::KeyChord> _keyMappings;
         std::optional<std::wstring> _name;
+        std::wstring _ID;
         std::optional<std::wstring> _iconPath;
         bool _nestedCommand{ false };
 

diff --git a/src/cascadia/TerminalSettingsModel/Command.idl b/src/cascadia/TerminalSettingsModel/Command.idl
@@ -36,6 +36,7 @@ namespace Microsoft.Terminal.Settings.Model
         Command();
 
         String Name { get; };
+        String ID { get; };
         ActionAndArgs ActionAndArgs { get; };
         Microsoft.Terminal.Control.KeyChord Keys { get; };
         void RegisterKey(Microsoft.Terminal.Control.KeyChord keys);