From 2122eec18612ee8f06de8c48a3af3445fe6c28cc Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 7 Feb 2023 16:57:23 -0600 Subject: [PATCH] [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 --- src/cascadia/TerminalApp/AppLogic.cpp | 3 + src/cascadia/TerminalApp/TerminalPage.cpp | 83 +------------------ src/cascadia/TerminalApp/TerminalPage.h | 4 - .../TerminalSettingsModel/ActionMap.cpp | 75 +++++++++++++++++ .../TerminalSettingsModel/ActionMap.h | 8 ++ .../TerminalSettingsModel/ActionMap.idl | 2 + .../CascadiaSettings.cpp | 5 ++ .../TerminalSettingsModel/CascadiaSettings.h | 2 + .../CascadiaSettings.idl | 2 + .../GlobalAppSettings.cpp | 6 ++ .../TerminalSettingsModel/GlobalAppSettings.h | 3 + 11 files changed, 108 insertions(+), 85 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 65098b5582b..fbe0a21c750 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/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) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index ca2a801f846..f68d56a8a2c 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -99,36 +99,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 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; @@ -3316,50 +3286,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: - // - - // Return Value: - // - - IMap TerminalPage::_ExpandCommands(IMapView commandsToExpand, - IVectorView profiles, - IMapView schemes) - { - auto warnings{ winrt::single_threaded_vector() }; - - std::vector 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(); - 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 @@ -3370,15 +3296,10 @@ namespace winrt::TerminalApp::implementation // - 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(); - for (const auto& nameAndCommand : copyOfCommands) + for (const auto& nameAndCommand : expanded) { commandsCollection.Append(nameAndCommand.Value()); } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 3d5acda23d9..b393c142771 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -300,10 +300,6 @@ namespace winrt::TerminalApp::implementation void _UpdateCommandsForPalette(); void _SetBackgroundImage(const winrt::Microsoft::Terminal::Settings::Model::IAppearanceConfig& newAppearance); - static winrt::Windows::Foundation::Collections::IMap _ExpandCommands(Windows::Foundation::Collections::IMapView commandsToExpand, - Windows::Foundation::Collections::IVectorView profiles, - Windows::Foundation::Collections::IMapView schemes); - void _DuplicateFocusedTab(); void _DuplicateTab(const TerminalTab& tab); diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index a3a5124a3d0..d61d1fa2e2a 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/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& profiles, + const winrt::Windows::Foundation::Collections::IMapView& 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() }; + + std::vector 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(); + + const auto& commandsToExpand{ NameMap() }; + for (const auto& nameAndCommand : commandsToExpand) + { + copyOfCommands.Insert(nameAndCommand.Key(), nameAndCommand.Value()); + } + + Command::ExpandCommands(copyOfCommands, + profiles, + winrt::param::vector_view{ sortedSchemes }, + warnings); + + _ExpandedMapCache = copyOfCommands; + } + Windows::Foundation::Collections::IMapView ActionMap::ExpandedCommands() + { + return _ExpandedMapCache.GetView(); + } } diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index 6dbd2b04c23..7f71e09f19d 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/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 ExpandedCommands(); + void ExpandCommands(const Windows::Foundation::Collections::IVectorView& profiles, + const Windows::Foundation::Collections::IMapView& schemes); + private: std::optional _GetActionByID(const InternalActionID actionID) const; std::optional _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 _AvailableActionsCache{ nullptr }; Windows::Foundation::Collections::IMap _NameMapCache{ nullptr }; Windows::Foundation::Collections::IMap _GlobalHotkeysCache{ nullptr }; Windows::Foundation::Collections::IMap _KeyBindingMapCache{ nullptr }; + Windows::Foundation::Collections::IMap _ExpandedMapCache{ nullptr }; + std::unordered_map _NestedCommands; std::vector _IterableCommands; std::unordered_map _KeyMap; diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.idl b/src/cascadia/TerminalSettingsModel/ActionMap.idl index 806baa17a30..8bbc3b1233c 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.idl +++ b/src/cascadia/TerminalSettingsModel/ActionMap.idl @@ -20,6 +20,8 @@ namespace Microsoft.Terminal.Settings.Model Windows.Foundation.Collections.IMapView NameMap { get; }; Windows.Foundation.Collections.IMapView KeyBindings { get; }; Windows.Foundation.Collections.IMapView GlobalHotkeys { get; }; + + Windows.Foundation.Collections.IMapView ExpandedCommands { get; }; }; [default_interface] runtimeclass ActionMap : IActionMapView diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 42ec1d28725..086ffcec529 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -1214,3 +1214,8 @@ void CascadiaSettings::_validateThemeExists() } } } + +void CascadiaSettings::ExpandCommands() +{ + _globals->ExpandCommands(ActiveProfiles().GetView(), GlobalSettings().ColorSchemes()); +} diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index ac9049b4b12..e0cf1e96202 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/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(); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl index 5a1987dcf3e..58db9f20e87 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl @@ -53,5 +53,7 @@ namespace Microsoft.Terminal.Settings.Model static Boolean IsDefaultTerminalSet { get; }; IObservableVector DefaultTerminals { get; }; DefaultTerminal CurrentDefaultTerminal; + + void ExpandCommands(); } } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 55ec4d634bf..d6a5aa49e87 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -239,3 +239,9 @@ winrt::Windows::Foundation::Collections::IMapView& profiles, + const winrt::Windows::Foundation::Collections::IMapView& schemes) +{ + _actionMap->ExpandCommands(profiles, schemes); +} diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 4a594948eba..945c032c2bc 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/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& profiles, + const Windows::Foundation::Collections::IMapView& schemes); + INHERITABLE_SETTING(Model::GlobalAppSettings, hstring, UnparsedDefaultProfile, L""); #define GLOBAL_SETTINGS_INITIALIZE(type, name, jsonKey, ...) \