From e28d47888c992f05fa9b467946c6a8374d3538be Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 24 Apr 2024 16:04:39 -0700 Subject: [PATCH] some todos for later --- .../ActionMapSerialization.cpp | 4 ++-- .../TerminalSettingsModel/Command.cpp | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index 6cc158892c7..5e1c4dd5878 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/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; diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 4cc2d997eda..afd2e2f28d3 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -318,7 +318,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // create an "invalid" ActionAndArgs result->_ActionAndArgs = make(); 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); } }