some todos for later

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

@@ -65,8 +65,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  {
  AddAction(*Command::FromJson(jsonBlock, warnings, origin, withKeybindings));
 
- // if we're in a 'command' block and there are keys, this is the legacy style
- // let the parse know that fixups are needed
+ // this is a 'command' block and there are keys - meaning this is the legacy style
+ // let the loader know that fixups are needed
  if (jsonBlock.isMember(JsonKey("keys")))
  {
  _fixUpsAppliedDuringLoad = true;

src/cascadia/TerminalSettingsModel/Command.cpp

@@ -318,7 +318,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  // create an "invalid" ActionAndArgs
  result->_ActionAndArgs = make<implementation::ActionAndArgs>();
  result->_nestedCommand = true;
- // todo: new implementation ignores nested commands, we need to not ignore this though
  }
 
  JsonUtils::GetValueForKey(json, IconKey, result->_iconPath);
@@ -330,14 +329,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  {
  result->_ActionAndArgs = *ActionAndArgs::FromJson(actionJson, warnings);
 
- // if this is a user-defined command and they did not provide an id, generate one for them
- if (result->_ID.empty() && result->_ActionAndArgs.Action() != ShortcutAction::Invalid && origin == OriginTag::User)
+ // if this is a user-defined, non-iterable, valid command and they did not provide an id, generate one for them
+ if (result->_ID.empty() && result->_IterateOn == ExpandCommandType::None && result->_ActionAndArgs.Action() != ShortcutAction::Invalid && origin == OriginTag::User)
  {
- // There is currently a minor bug here that doesn't affect anything -
- // we will reach this point for each command in an 'unpacked' nested/iterable command
- // which means we will generate IDs for them. These don't get written in the json
- // or stored anywhere though, but since they're also not used for anything we should
- // probably just not generate them at all
+ // todo: stage 3
+ // couple of issues -
+ // 1. we reach this point for 'unpacked' nested commands, which means we generate IDs for them
+ // these IDs aren't used anywhere or written to the json, which is intentional, but we should
+ // figure out a way to not generate them at all
+ // 2. if we generate an ID for a command here, we need to let the loader know that fixups are needed -
+ // ideally via action map's _fixUpsAppliedDuringLoad, because having one of those flags for each command sounds horrendous
+ // however to do this without false positives, we need to fix 1 first
  result->GenerateID();
  }
  }
@@ -424,6 +426,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
  else
  {
  // Override commands with the same name
+ // todo: stage 3 - we may not need to do this anymore
  commands.Insert(result->Name(), *result);
  }
  }