[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
This commit is contained in:
Mike Griese
2023-02-07 16:57:23 -06:00
parent 81524ea71e
commit 2122eec186
11 changed files with 108 additions and 85 deletions

View File

@@ -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)

View File

@@ -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<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;
@@ -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:
// - <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
@@ -3370,15 +3296,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());
}

View File

@@ -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<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);

View File

@@ -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();
}
}

View File

@@ -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;

View File

@@ -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

View File

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

View File

@@ -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();

View File

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

View File

@@ -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);
}

View File

@@ -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, ...) \