[ainulindale] Expand commands in the AppLogic, not on each page

  TerminalPage is the thing that ends up expanding iterable Command. It does
  this largely with copies - it makes a new map, a new vector, copies the
  Commands over, and does the work there before setting up the cmdpal.

  Except, it's not making a copy of the Commands, it's making a copy of the
  vector, with winrt objects all pointing at the Command objects that are
  ultimately owned by CascadiaSettings.

  This doesn't matter if there's only one TerminalPage - we'll only ever do that once.

  If there's many, on different threads, then one tpage will end up expanding
  the subcommands of one Command while another tpage is ALSO iterating on those
  subcommands. Hence why I'm getting `hresult_changed_state`s

(cherry picked from commit 2122eec)

src/cascadia/TerminalApp/AppLogic.cpp

@@ -285,6 +285,9 @@ namespace winrt::TerminalApp::implementation
             }
 
             _settings = std::move(newSettings);
+
+            _settings.ExpandCommands();
+
             hr = (_warnings.Size()) == 0 ? S_OK : S_FALSE;
         }
         catch (const winrt::hresult_error& e)

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -97,36 +97,6 @@ namespace winrt::TerminalApp::implementation
         return S_OK;
     }
 
-    // Function Description:
-    // - Recursively check our commands to see if there's a keybinding for
-    //   exactly their action. If there is, label that command with the text
-    //   corresponding to that key chord.
-    // - Will recurse into nested commands as well.
-    // Arguments:
-    // - settings: The settings who's keybindings we should use to look up the key chords from
-    // - commands: The list of commands to label.
-    static void _recursiveUpdateCommandKeybindingLabels(CascadiaSettings settings,
-                                                        IMapView<winrt::hstring, Command> commands)
-    {
-        for (const auto& nameAndCmd : commands)
-        {
-            const auto& command = nameAndCmd.Value();
-            if (command.HasNestedCommands())
-            {
-                _recursiveUpdateCommandKeybindingLabels(settings, command.NestedCommands());
-            }
-            else
-            {
-                // If there's a keybinding that's bound to exactly this command,
-                // then get the keychord and display it as a
-                // part of the command in the UI.
-                // We specifically need to do this for nested commands.
-                const auto keyChord{ settings.ActionMap().GetKeyBindingForAction(command.ActionAndArgs().Action(), command.ActionAndArgs().Args()) };
-                command.RegisterKey(keyChord);
-            }
-        }
-    }
-
     winrt::fire_and_forget TerminalPage::SetSettings(CascadiaSettings settings, bool needRefreshUI)
     {
         _settings = settings;
@@ -2974,50 +2944,6 @@ namespace winrt::TerminalApp::implementation
         }
     }
 
-    // This is a helper to aid in sorting commands by their `Name`s, alphabetically.
-    static bool _compareSchemeNames(const ColorScheme& lhs, const ColorScheme& rhs)
-    {
-        std::wstring leftName{ lhs.Name() };
-        std::wstring rightName{ rhs.Name() };
-        return leftName.compare(rightName) < 0;
-    }
-
-    // Method Description:
-    // - Takes a mapping of names->commands and expands them
-    // Arguments:
-    // - <none>
-    // Return Value:
-    // - <none>
-    IMap<winrt::hstring, Command> TerminalPage::_ExpandCommands(IMapView<winrt::hstring, Command> commandsToExpand,
-                                                                IVectorView<Profile> profiles,
-                                                                IMapView<winrt::hstring, ColorScheme> schemes)
-    {
-        auto warnings{ winrt::single_threaded_vector<SettingsLoadWarnings>() };
-
-        std::vector<ColorScheme> sortedSchemes;
-        sortedSchemes.reserve(schemes.Size());
-
-        for (const auto& nameAndScheme : schemes)
-        {
-            sortedSchemes.push_back(nameAndScheme.Value());
-        }
-        std::sort(sortedSchemes.begin(),
-                  sortedSchemes.end(),
-                  _compareSchemeNames);
-
-        auto copyOfCommands = winrt::single_threaded_map<winrt::hstring, Command>();
-        for (const auto& nameAndCommand : commandsToExpand)
-        {
-            copyOfCommands.Insert(nameAndCommand.Key(), nameAndCommand.Value());
-        }
-
-        Command::ExpandCommands(copyOfCommands,
-                                profiles,
-                                { sortedSchemes },
-                                warnings);
-
-        return copyOfCommands;
-    }
     // Method Description:
     // - Repopulates the list of commands in the command palette with the
     //   current commands in the settings. Also updates the keybinding labels to
@@ -3028,15 +2954,10 @@ namespace winrt::TerminalApp::implementation
     // - <none>
     void TerminalPage::_UpdateCommandsForPalette()
     {
-        auto copyOfCommands = _ExpandCommands(_settings.GlobalSettings().ActionMap().NameMap(),
-                                              _settings.ActiveProfiles().GetView(),
-                                              _settings.GlobalSettings().ColorSchemes());
-
-        _recursiveUpdateCommandKeybindingLabels(_settings, copyOfCommands.GetView());
-
         // Update the command palette when settings reload
+        const auto& expanded{ _settings.GlobalSettings().ActionMap().ExpandedCommands() };
         auto commandsCollection = winrt::single_threaded_vector<Command>();
-        for (const auto& nameAndCommand : copyOfCommands)
+        for (const auto& nameAndCommand : expanded)
         {
             commandsCollection.Append(nameAndCommand.Value());
         }

src/cascadia/TerminalApp/TerminalPage.h

@@ -297,10 +297,6 @@ namespace winrt::TerminalApp::implementation
         void _UpdateCommandsForPalette();
         void _SetBackgroundImage(const winrt::Microsoft::Terminal::Settings::Model::IAppearanceConfig& newAppearance);
 
-        static winrt::Windows::Foundation::Collections::IMap<winrt::hstring, Microsoft::Terminal::Settings::Model::Command> _ExpandCommands(Windows::Foundation::Collections::IMapView<winrt::hstring, Microsoft::Terminal::Settings::Model::Command> commandsToExpand,
-                                                                                                                                            Windows::Foundation::Collections::IVectorView<Microsoft::Terminal::Settings::Model::Profile> profiles,
-                                                                                                                                            Windows::Foundation::Collections::IMapView<winrt::hstring, Microsoft::Terminal::Settings::Model::ColorScheme> schemes);
-
         void _DuplicateFocusedTab();
         void _DuplicateTab(const TerminalTab& tab);
 

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -854,4 +854,79 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         cmd->ActionAndArgs(action);
         AddAction(*cmd);
     }
+
+    void ActionMap::_recursiveUpdateCommandKeybindingLabels()
+    {
+        const auto& commands{ _ExpandedMapCache };
+
+        for (const auto& nameAndCmd : commands)
+        {
+            const auto& command = nameAndCmd.Value();
+            if (command.HasNestedCommands())
+            {
+                _recursiveUpdateCommandKeybindingLabels();
+            }
+            else
+            {
+                // If there's a keybinding that's bound to exactly this command,
+                // then get the keychord and display it as a
+                // part of the command in the UI.
+                // We specifically need to do this for nested commands.
+                const auto keyChord{ GetKeyBindingForAction(command.ActionAndArgs().Action(),
+                                                            command.ActionAndArgs().Args()) };
+                command.RegisterKey(keyChord);
+            }
+        }
+    }
+
+    // This is a helper to aid in sorting commands by their `Name`s, alphabetically.
+    static bool _compareSchemeNames(const ColorScheme& lhs, const ColorScheme& rhs)
+    {
+        std::wstring leftName{ lhs.Name() };
+        std::wstring rightName{ rhs.Name() };
+        return leftName.compare(rightName) < 0;
+    }
+
+    void ActionMap::ExpandCommands(const winrt::Windows::Foundation::Collections::IVectorView<Model::Profile>& profiles,
+                                   const winrt::Windows::Foundation::Collections::IMapView<winrt::hstring, Model::ColorScheme>& schemes)
+    {
+        // TODO in review - It's a little weird to stash the expanded commands
+        // into a separate map. Is it possible to just replace the name map with
+        // the post-expanded commands?
+        //
+        // WHILE also making sure that upon re-saving the commands, we don't
+        // actually serialize the results of the expansion. I don't think it is.
+
+        auto warnings{ winrt::single_threaded_vector<SettingsLoadWarnings>() };
+
+        std::vector<Model::ColorScheme> sortedSchemes;
+        sortedSchemes.reserve(schemes.Size());
+
+        for (const auto& nameAndScheme : schemes)
+        {
+            sortedSchemes.push_back(nameAndScheme.Value());
+        }
+        std::sort(sortedSchemes.begin(),
+                  sortedSchemes.end(),
+                  _compareSchemeNames);
+
+        auto copyOfCommands = winrt::single_threaded_map<winrt::hstring, Model::Command>();
+
+        const auto& commandsToExpand{ NameMap() };
+        for (const auto& nameAndCommand : commandsToExpand)
+        {
+            copyOfCommands.Insert(nameAndCommand.Key(), nameAndCommand.Value());
+        }
+
+        Command::ExpandCommands(copyOfCommands,
+                                profiles,
+                                winrt::param::vector_view<Model::ColorScheme>{ sortedSchemes },
+                                warnings);
+
+        _ExpandedMapCache = copyOfCommands;
+    }
+    Windows::Foundation::Collections::IMapView<hstring, Model::Command> ActionMap::ExpandedCommands()
+    {
+        return _ExpandedMapCache.GetView();
+    }
 }

src/cascadia/TerminalSettingsModel/ActionMap.h

@@ -75,6 +75,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void DeleteKeyBinding(const Control::KeyChord& keys);
         void RegisterKeyBinding(Control::KeyChord keys, Model::ActionAndArgs action);
 
+        Windows::Foundation::Collections::IMapView<hstring, Model::Command> ExpandedCommands();
+        void ExpandCommands(const Windows::Foundation::Collections::IVectorView<Model::Profile>& profiles,
+                            const Windows::Foundation::Collections::IMapView<winrt::hstring, Model::ColorScheme>& schemes);
+
     private:
         std::optional<Model::Command> _GetActionByID(const InternalActionID actionID) const;
         std::optional<Model::Command> _GetActionByKeyChordInternal(const Control::KeyChord& keys) const;
@@ -90,11 +94,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void _TryUpdateName(const Model::Command& cmd, const Model::Command& oldCmd, const Model::Command& consolidatedCmd);
         void _TryUpdateKeyChord(const Model::Command& cmd, const Model::Command& oldCmd, const Model::Command& consolidatedCmd);
 
+        void _recursiveUpdateCommandKeybindingLabels();
+
         Windows::Foundation::Collections::IMap<hstring, Model::ActionAndArgs> _AvailableActionsCache{ nullptr };
         Windows::Foundation::Collections::IMap<hstring, Model::Command> _NameMapCache{ nullptr };
         Windows::Foundation::Collections::IMap<Control::KeyChord, Model::Command> _GlobalHotkeysCache{ nullptr };
         Windows::Foundation::Collections::IMap<Control::KeyChord, Model::Command> _KeyBindingMapCache{ nullptr };
 
+        Windows::Foundation::Collections::IMap<hstring, Model::Command> _ExpandedMapCache{ nullptr };
+
         std::unordered_map<winrt::hstring, Model::Command> _NestedCommands;
         std::vector<Model::Command> _IterableCommands;
         std::unordered_map<Control::KeyChord, InternalActionID, KeyChordHash, KeyChordEquality> _KeyMap;

src/cascadia/TerminalSettingsModel/ActionMap.idl

@@ -20,6 +20,8 @@ namespace Microsoft.Terminal.Settings.Model
         Windows.Foundation.Collections.IMapView<String, Command> NameMap { get; };
         Windows.Foundation.Collections.IMapView<Microsoft.Terminal.Control.KeyChord, Command> KeyBindings { get; };
         Windows.Foundation.Collections.IMapView<Microsoft.Terminal.Control.KeyChord, Command> GlobalHotkeys { get; };
+
+        Windows.Foundation.Collections.IMapView<String, Command> ExpandedCommands { get; };
     };
 
     [default_interface] runtimeclass ActionMap : IActionMapView

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

@@ -1214,3 +1214,8 @@ void CascadiaSettings::_validateThemeExists()
         }
     }
 }
+
+void CascadiaSettings::ExpandCommands()
+{
+    _globals->ExpandCommands(ActiveProfiles().GetView(), GlobalSettings().ColorSchemes());
+}

src/cascadia/TerminalSettingsModel/CascadiaSettings.h

@@ -143,6 +143,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         Model::DefaultTerminal CurrentDefaultTerminal() noexcept;
         void CurrentDefaultTerminal(const Model::DefaultTerminal& terminal);
 
+        void ExpandCommands();
+
     private:
         static const std::filesystem::path& _settingsPath();
         static const std::filesystem::path& _releaseSettingsPath();

src/cascadia/TerminalSettingsModel/CascadiaSettings.idl

@@ -53,5 +53,7 @@ namespace Microsoft.Terminal.Settings.Model
         static Boolean IsDefaultTerminalSet { get; };
         IObservableVector<DefaultTerminal> DefaultTerminals { get; };
         DefaultTerminal CurrentDefaultTerminal;
+
+        void ExpandCommands();
     }
 }

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

@@ -239,3 +239,9 @@ winrt::Windows::Foundation::Collections::IMapView<winrt::hstring, winrt::Microso
 {
     return _themes.GetView();
 }
+
+void GlobalAppSettings::ExpandCommands(const winrt::Windows::Foundation::Collections::IVectorView<Model::Profile>& profiles,
+                                       const winrt::Windows::Foundation::Collections::IMapView<winrt::hstring, Model::ColorScheme>& schemes)
+{
+    _actionMap->ExpandCommands(profiles, schemes);
+}

src/cascadia/TerminalSettingsModel/GlobalAppSettings.h

@@ -63,6 +63,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void AddTheme(const Model::Theme& theme);
         Model::Theme CurrentTheme() noexcept;
 
+        void ExpandCommands(const Windows::Foundation::Collections::IVectorView<Model::Profile>& profiles,
+                            const Windows::Foundation::Collections::IMapView<winrt::hstring, Model::ColorScheme>& schemes);
+
         INHERITABLE_SETTING(Model::GlobalAppSettings, hstring, UnparsedDefaultProfile, L"");
 
 #define GLOBAL_SETTINGS_INITIALIZE(type, name, jsonKey, ...) \