Refactor ActionMap and Command to use ActionIDs

src/cascadia/TerminalApp/TabBase.cpp

@@ -192,7 +192,8 @@ namespace winrt::TerminalApp::implementation
  // - <none>
  void TabBase::_UpdateSwitchToTabKeyChord()
  {
- const auto keyChord = _actionMap ? _actionMap.GetKeyBindingForAction(ShortcutAction::SwitchToTab, SwitchToTabArgs{ _TabViewIndex }) : nullptr;
+ const auto id = fmt::format(FMT_COMPILE(L"Terminal.SwitchToTab{}"), _TabViewIndex);
+ const auto keyChord{ _actionMap.GetKeyBindingForAction(id) };
  const auto keyChordText = keyChord ? KeyChordSerialization::ToString(keyChord) : L"";
 
  if (_keyChord == keyChordText)

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -826,7 +826,7 @@ namespace winrt::TerminalApp::implementation
  newTabFlyout.Items().Append(settingsItem);
 
  auto actionMap = _settings.ActionMap();
- const auto settingsKeyChord{ actionMap.GetKeyBindingForAction(ShortcutAction::OpenSettings, OpenSettingsArgs{ SettingsTarget::SettingsUI }) };
+ const auto settingsKeyChord{ actionMap.GetKeyBindingForAction(L"Terminal.OpenSettingsUI") };
  if (settingsKeyChord)
  {
  _SetAcceleratorForMenuItem(settingsItem, settingsKeyChord);
@@ -848,7 +848,7 @@ namespace winrt::TerminalApp::implementation
  commandPaletteFlyout.Click({ this, &TerminalPage::_CommandPaletteButtonOnClick });
  newTabFlyout.Items().Append(commandPaletteFlyout);
 
- const auto commandPaletteKeyChord{ actionMap.GetKeyBindingForAction(ShortcutAction::ToggleCommandPalette) };
+ const auto commandPaletteKeyChord{ actionMap.GetKeyBindingForAction(L"Terminal.ToggleCommandPalette") };
  if (commandPaletteKeyChord)
  {
  _SetAcceleratorForMenuItem(commandPaletteFlyout, commandPaletteKeyChord);
@@ -1023,7 +1023,8 @@ namespace winrt::TerminalApp::implementation
  // NewTab(ProfileIndex=N) action
  NewTerminalArgs newTerminalArgs{ profileIndex };
  NewTabArgs newTabArgs{ newTerminalArgs };
- auto profileKeyChord{ _settings.ActionMap().GetKeyBindingForAction(ShortcutAction::NewTab, newTabArgs) };
+ const auto id = fmt::format(FMT_COMPILE(L"Terminal.OpenNewTabProfile{}"), profileIndex);
+ const auto profileKeyChord{ _settings.ActionMap().GetKeyBindingForAction(id) };
 
  // make sure we find one to display
  if (profileKeyChord)

src/cascadia/TerminalSettingsModel/ActionMap.h

@@ -59,8 +59,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  // queries
  Model::Command GetActionByKeyChord(const Control::KeyChord& keys) const;
  bool IsKeyChordExplicitlyUnbound(const Control::KeyChord& keys) const;
- Control::KeyChord GetKeyBindingForAction(const ShortcutAction& action) const;
- Control::KeyChord GetKeyBindingForAction(const ShortcutAction& action, const IActionArgs& actionArgs) const;
+ Control::KeyChord GetKeyBindingForAction(winrt::hstring cmdID) const;
 
  // population
  void AddAction(const Model::Command& cmd);
@@ -69,9 +68,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  static com_ptr<ActionMap> FromJson(const Json::Value& json, const OriginTag origin = OriginTag::None);
  std::vector<SettingsLoadWarnings> LayerJson(const Json::Value& json, const OriginTag origin, const bool withKeybindings = true);
  Json::Value ToJson() const;
+ Json::Value KeyBindingsToJson() const;
+ bool FixUpsAppliedDuringLoad() 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);
@@ -83,46 +83,50 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  winrt::Windows::Foundation::Collections::IVector<Model::Command> FilterToSendInput(winrt::hstring currentCommandline);
 
  private:
- std::optional<Model::Command> _GetActionByID(const InternalActionID actionID) const;
+ Model::Command _GetActionByID(const winrt::hstring actionID) const;
+ std::optional<winrt::hstring> _GetActionIdByKeyChordInternal(const Control::KeyChord& keys) const;
  std::optional<Model::Command> _GetActionByKeyChordInternal(const Control::KeyChord& keys) const;
 
  void _RefreshKeyBindingCaches();
  void _PopulateAvailableActionsWithStandardCommands(std::unordered_map<hstring, Model::ActionAndArgs>& availableActions, std::unordered_set<InternalActionID>& visitedActionIDs) const;
  void _PopulateNameMapWithSpecialCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
  void _PopulateNameMapWithStandardCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
- void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<Control::KeyChord, KeyChordHash, KeyChordEquality>& unboundKeys) const;
- std::vector<Model::Command> _GetCumulativeActions() const noexcept;
 
- void _TryUpdateActionMap(const Model::Command& cmd, Model::Command& oldCmd, Model::Command& consolidatedCmd);
- 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);
+ std::vector<Model::Command> _GetCumulativeActions() const noexcept;
+ void _PopulateCumulativeKeyMap(std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality>& keyBindingsMap);
+ void _PopulateCumulativeActionMap(std::unordered_map<hstring, Model::Command>& actionMap);
 
- void _recursiveUpdateCommandKeybindingLabels();
+ void _TryUpdateActionMap(const Model::Command& cmd);
+ void _TryUpdateKeyChord(const Model::Command& cmd);
 
  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::IVector<Model::Command> _ExpandedCommandsCache{ nullptr };
 
  std::unordered_map<winrt::hstring, Model::Command> _NestedCommands;
  std::vector<Model::Command> _IterableCommands;
- std::unordered_map<Control::KeyChord, InternalActionID, KeyChordHash, KeyChordEquality> _KeyMap;
- std::unordered_map<InternalActionID, Model::Command> _ActionMap;
-
- // Masking Actions:
- // These are actions that were introduced in an ancestor,
- // but were edited (or unbound) in the current layer.
- // _ActionMap shows a Command with keys that were added in this layer,
- // whereas _MaskingActions provides a view that encompasses all of
- // the valid associated key chords.
- // Maintaining this map allows us to return a valid Command
- // in GetKeyBindingForAction.
- // Additionally, these commands to not need to be serialized,
- // whereas those in _ActionMap do. These actions provide more data
- // than is necessary to be serialized.
- std::unordered_map<InternalActionID, Model::Command> _MaskingActions;
+
+ bool _fixUpsAppliedDuringLoad;
+
+ void _AddKeyBindingHelper(const Json::Value& json, std::vector<SettingsLoadWarnings>& warnings);
+
+ // _KeyMap is the map of key chords -> action IDs defined in this layer
+ // _ActionMap is the map of action IDs -> commands defined in this layer
+ // These maps are the ones that we deserialize into when parsing the user json and vice-versa
+ std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality> _KeyMap;
+ std::unordered_map<winrt::hstring, Model::Command> _ActionMap;
+
+ // _CumulativeKeyMapCache is the map of key chords -> action IDs defined in all layers, with child layers overriding parent layers
+ Windows::Foundation::Collections::IMap<Control::KeyChord, winrt::hstring> _CumulativeKeyMapCache{ nullptr };
+ // _CumulativeActionMapCache is the map of action IDs -> commands defined in all layers, with child layers overriding parent layers
+ Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command> _CumulativeActionMapCache{ nullptr };
+
+ // _ResolvedKeyActionMapCache is the map of key chords -> commands defined in all layers, with child layers overriding parent layers
+ // This is effectively a combination of _CumulativeKeyMapCache and _CumulativeActionMapCache and its purpose is so that
+ // we can give the SUI a view of the key chords and the commands they map to
+ Windows::Foundation::Collections::IMap<Control::KeyChord, Model::Command> _ResolvedKeyActionMapCache{ nullptr };
 
  friend class SettingsModelUnitTests::KeyBindingsTests;
  friend class SettingsModelUnitTests::DeserializationTests;

src/cascadia/TerminalSettingsModel/ActionMap.idl

@@ -12,8 +12,7 @@ namespace Microsoft.Terminal.Settings.Model
 
  Command GetActionByKeyChord(Microsoft.Terminal.Control.KeyChord keys);
 
- Microsoft.Terminal.Control.KeyChord GetKeyBindingForAction(ShortcutAction action);
- [method_name("GetKeyBindingForActionWithArgs")] Microsoft.Terminal.Control.KeyChord GetKeyBindingForAction(ShortcutAction action, IActionArgs actionArgs);
+ Microsoft.Terminal.Control.KeyChord GetKeyBindingForAction(String cmdID);
 
  Windows.Foundation.Collections.IMapView<String, ActionAndArgs> AvailableActions { get; };
 

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

@@ -27,12 +27,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  }
 
  // Method Description:
- // - Deserialize an ActionMap from the array `json`. The json array should contain
- //  an array of serialized `Command` objects.
- // - These actions are added to the `ActionMap`, where we automatically handle
- //  overwriting and unbinding actions.
+ // - Deserialize an ActionMap from the array `json`
+ // - The json array either contains an array of serialized `Command` objects,
+ //  or an array of keybindings
+ // - The actions are added to _ActionMap and the keybindings are added to _KeyMap
  // Arguments:
- // - json: an array of Json::Value's to deserialize into our ActionMap.
+ // - json: an array of Json::Value's to deserialize into our _ActionMap and _KeyMap
  // Return value:
  // - a list of warnings encountered while deserializing the json
  std::vector<SettingsLoadWarnings> ActionMap::LayerJson(const Json::Value& json, const OriginTag origin, const bool withKeybindings)
@@ -43,14 +43,50 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  // settings phase, so we'll collect them now.
  std::vector<SettingsLoadWarnings> warnings;
 
- for (const auto& cmdJson : json)
+ for (const auto& jsonBlock : json)
  {
- if (!cmdJson.isObject())
+ if (!jsonBlock.isObject())
  {
  continue;
  }
 
- AddAction(*Command::FromJson(cmdJson, warnings, origin, withKeybindings));
+ // the json block may be 1 of 3 things:
+ // - the legacy style command block, that has the action, args and keys in it
+ // - the modern style command block, that has the action, args and an ID
+ // - the modern style keys block, that has the keys and an ID
+
+ // if the block contains a "command" field, it is either a legacy or modern style command block
+ // and we can call Command::FromJson on it (Command::FromJson can handle parsing both legacy or modern)
+
+ // if there is no "command" field, then it is a modern style keys block
+ // todo: use the CommandsKey / ActionKey static string view in Command.cpp somehow
+ if (jsonBlock.isMember(JsonKey("commands")) || jsonBlock.isMember(JsonKey("command")))
+ {
+ AddAction(*Command::FromJson(jsonBlock, warnings, origin, withKeybindings));
+
+ // for non-nested non-iterable commands,
+ // check if this is a legacy-style command block so we can inform the loader that fixups are needed
+ if (jsonBlock.isMember(JsonKey("command")) && !jsonBlock.isMember(JsonKey("iterateOn")))
+ {
+ if (jsonBlock.isMember(JsonKey("keys")))
+ {
+ // there are keys in this command block - its the legacy style
+ _fixUpsAppliedDuringLoad = true;
+ }
+
+ if (origin == OriginTag::User && !jsonBlock.isMember(JsonKey("id")))
+ {
+ // there's no ID in this command block - we will generate one for the user
+ // inform the loader that the ID needs to be written into the json
+ _fixUpsAppliedDuringLoad = true;
+ }
+ }
+ }
+ else
+ {
+ // this is not a command block, so it is a keybinding block
+ _AddKeyBindingHelper(jsonBlock, warnings);
+ }
  }
 
  return warnings;
@@ -60,23 +96,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  {
  Json::Value actionList{ Json::ValueType::arrayValue };
 
- // Command serializes to an array of JSON objects.
- // This is because a Command may have multiple key chords associated with it.
- // The name and icon are only serialized in the first object.
- // Example:
- // { "name": "Custom Copy", "command": "copy", "keys": "ctrl+c" }
- // { "command": "copy", "keys": "ctrl+shift+c" }
- // { "command": "copy", "keys": "ctrl+ins" }
  auto toJson = [&actionList](const Model::Command& cmd) {
  const auto cmdImpl{ winrt::get_self<implementation::Command>(cmd) };
- const auto& cmdJsonArray{ cmdImpl->ToJson() };
- for (const auto& cmdJson : cmdJsonArray)
- {
- actionList.append(cmdJson);
- }
+ const auto& cmdJson{ cmdImpl->ToJson() };
+ actionList.append(cmdJson);
  };
 
- // Serialize all standard Command objects in the current layer
  for (const auto& [_, cmd] : _ActionMap)
  {
  toJson(cmd);
@@ -96,4 +121,74 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
  return actionList;
  }
+
+ Json::Value ActionMap::KeyBindingsToJson() const
+ {
+ Json::Value keybindingsList{ Json::ValueType::arrayValue };
+
+ auto toJson = [&keybindingsList](const KeyChord kc, const winrt::hstring cmdID) {
+ Json::Value keyIDPair{ Json::ValueType::objectValue };
+ JsonUtils::SetValueForKey(keyIDPair, "keys", kc);
+ JsonUtils::SetValueForKey(keyIDPair, "id", cmdID);
+ keybindingsList.append(keyIDPair);
+ };
+
+ // Serialize all standard keybinding objects in the current layer
+ for (const auto& [keys, cmdID] : _KeyMap)
+ {
+ toJson(keys, cmdID);
+ }
+
+ return keybindingsList;
+ }
+
+ void ActionMap::_AddKeyBindingHelper(const Json::Value& json, std::vector<SettingsLoadWarnings>& warnings)
+ {
+ // There should always be a "keys" field
+ // - If there is also an "id" field - we add the pair to our _KeyMap
+ // - If there is no "id" field - this is an explicit unbinding, still add it to the _KeyMap,
+ // when this key chord is queried for we will know it is an explicit unbinding
+ // todo: use the KeysKey and IDKey static strings from Command.cpp
+ const auto keysJson{ json[JsonKey("keys")] };
+ if (keysJson.isArray() && keysJson.size() > 1)
+ {
+ warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord);
+ }
+ else
+ {
+ Control::KeyChord keys{ nullptr };
+ winrt::hstring idJson;
+ if (JsonUtils::GetValueForKey(json, "keys", keys))
+ {
+ // if these keys are already bound to some command,
+ // we need to update that command to erase these keys as we are about to overwrite them
+ if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand)
+ {
+ const auto foundCommandImpl{ get_self<implementation::Command>(*foundCommand) };
+ foundCommandImpl->EraseKey(keys);
+ }
+
+ // if the "id" field doesn't exist in the json, then idJson will be an empty string which is fine
+ JsonUtils::GetValueForKey(json, "id", idJson);
+
+ // any existing keybinding with the same keychord in this layer will get overwritten
+ _KeyMap.insert_or_assign(keys, idJson);
+
+ // make sure the command registers these keys
+ if (!idJson.empty())
+ {
+ // TODO GH#17160
+ // if the command with this id is only going to appear later during settings load
+ // then this will return null, meaning that the command created later on will not register this keybinding
+ // the keybinding will still work fine within the app, its just that the Command object itself won't know about this key mapping
+ // we are going to move away from Command needing to know its key mappings in a followup, so this shouldn't matter for very long
+ if (const auto cmd = _GetActionByID(idJson))
+ {
+ cmd.RegisterKey(keys);
+ }
+ }
+ }
+ }
+ return;
+ }
 }

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -461,6 +461,7 @@ bool SettingsLoader::FixupUserSettings()
  };
 
  auto fixedUp = userSettings.fixupsAppliedDuringLoad;
+ fixedUp = userSettings.globals->FixUpsAppliedDuringLoad() || fixedUp;
 
  fixedUp = RemapColorSchemeForProfile(userSettings.baseLayerProfile) || fixedUp;
  for (const auto& profile : userSettings.profiles)
@@ -504,10 +505,6 @@ 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()) };
- actionMap->GenerateIDsForActions();
-
  return fixedUp;
 }