From 9cbd70dd3f2d0bc75bf57f9d66c9d3d63e423d30 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 5 Jun 2026 14:23:55 -0700 Subject: [PATCH] Convert Color Schemes + Themes --- .../TerminalSettingsModel/ColorScheme.cpp | 183 ++++++++++++++---- .../TerminalSettingsModel/ColorScheme.h | 33 +++- .../TerminalSettingsModel/JsonUtils.h | 38 ++++ src/cascadia/TerminalSettingsModel/Theme.cpp | 79 ++++---- src/cascadia/TerminalSettingsModel/Theme.h | 43 +++- .../ColorSchemeTests.cpp | 70 +++++++ .../UnitTests_SettingsModel/ThemeTests.cpp | 44 +++++ 7 files changed, 400 insertions(+), 90 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ColorScheme.cpp b/src/cascadia/TerminalSettingsModel/ColorScheme.cpp index 5fc9c725ad..8f98a45e4b 100644 --- a/src/cascadia/TerminalSettingsModel/ColorScheme.cpp +++ b/src/cascadia/TerminalSettingsModel/ColorScheme.cpp @@ -16,10 +16,6 @@ using namespace winrt::Microsoft::Terminal::Settings::Model::implementation; using namespace winrt::Windows::UI; static constexpr std::string_view NameKey{ "name" }; -static constexpr std::string_view ForegroundKey{ "foreground" }; -static constexpr std::string_view BackgroundKey{ "background" }; -static constexpr std::string_view SelectionBackgroundKey{ "selectionBackground" }; -static constexpr std::string_view CursorColorKey{ "cursorColor" }; static constexpr size_t ColorSchemeExpectedSize = 16; static constexpr std::array, 18> TableColorsMapping{ { @@ -46,28 +42,48 @@ static constexpr std::array, 18> TableColors { "brightMagenta", 13 }, } }; +#define COLORSCHEME_NAMED_COLORS(GEN) \ + GEN(Foreground, "foreground", DEFAULT_FOREGROUND) \ + GEN(Background, "background", DEFAULT_BACKGROUND) \ + GEN(SelectionBackground, "selectionBackground", DEFAULT_FOREGROUND) \ + GEN(CursorColor, "cursorColor", DEFAULT_CURSOR_COLOR) + +// The named colors paired with their default values. Used to drive the +// constructor seed, _layerJson validation, and ToJson copy from a single list. +// The individual getters still reference their DEFAULT_* constant directly. +#define GEN_NAMED_COLOR_DEFAULT(name, jsonKey, defaultVal) \ + { jsonKey, static_cast(defaultVal) }, +static const std::array, 4> NamedColorDefaults{ { + COLORSCHEME_NAMED_COLORS(GEN_NAMED_COLOR_DEFAULT) +} }; +#undef GEN_NAMED_COLOR_DEFAULT + ColorScheme::ColorScheme() noexcept : ColorScheme{ winrt::hstring{} } { } ColorScheme::ColorScheme(const winrt::hstring& name) noexcept : - _Name{ name }, _Origin{ OriginTag::User } { + JsonUtils::SetValueForKey(_json, NameKey, name); + for (const auto& [key, defaultValue] : NamedColorDefaults) + { + JsonUtils::SetValueForKey(_json, key, defaultValue); + } + const auto table = Utils::CampbellColorTable(); - std::copy_n(table.data(), table.size(), _table.data()); + for (size_t i = 0; i < ColorSchemeExpectedSize; ++i) + { + const Core::Color color = til::at(table, i); + JsonUtils::SetValueForKey(_json, til::at(TableColorsMapping, i).first, color); + } } winrt::com_ptr ColorScheme::Copy() const { - auto scheme{ winrt::make_self() }; - scheme->_Name = _Name; - scheme->_Foreground = _Foreground; - scheme->_Background = _Background; - scheme->_SelectionBackground = _SelectionBackground; - scheme->_CursorColor = _CursorColor; - scheme->_table = _table; + auto scheme{ winrt::make_self(uninitialized_t{}) }; + scheme->_json = _json; scheme->_Origin = _Origin; return scheme; } @@ -97,19 +113,22 @@ winrt::com_ptr ColorScheme::FromJson(const Json::Value& json) bool ColorScheme::_layerJson(const Json::Value& json) { // Required fields - auto isValid = JsonUtils::GetValueForKey(json, NameKey, _Name); + winrt::hstring name{}; + auto isValid = JsonUtils::GetValueForKey(json, NameKey, name); - // Optional fields (they have defaults in ColorScheme.h) - JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground); - JsonUtils::GetValueForKey(json, BackgroundKey, _Background); - JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground); - JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor); + // Optional fields; parsing them here only validates their type. + Core::Color namedColor{}; + for (const auto& [key, _] : NamedColorDefaults) + { + JsonUtils::GetValueForKey(json, key, namedColor); + } // Required fields + std::array table{}; size_t colorCount = 0; for (const auto& [key, index] : TableColorsMapping) { - colorCount += JsonUtils::GetValueForKey(json, key, til::at(_table, index)); + colorCount += JsonUtils::GetValueForKey(json, key, til::at(table, index)); if (colorCount == ColorSchemeExpectedSize) { break; @@ -117,8 +136,43 @@ bool ColorScheme::_layerJson(const Json::Value& json) } isValid &= (colorCount == 16); // Valid schemes should have exactly 16 colors + if (!isValid) + { + return false; + } - return isValid; + JsonUtils::MergeJsonKeys(json, _json); + _normalizeTableAliasKeys(); + return true; +} + +// Moves any alias color key (magenta -> purple, brightMagenta -> brightPurple; +// GH#11456) onto its canonical key so getters/ToJson only deal with canonical +// keys. The canonical key wins if both are present (matching the _layerJson +// validation loop's canonical-first preference). The node is copied whole, so an +// attached comment travels with the value. +void ColorScheme::_normalizeTableAliasKeys() +{ + static constexpr std::array, 2> aliases{ { + { "magenta", "purple" }, + { "brightMagenta", "brightPurple" }, + } }; + + for (const auto& [alias, canonical] : aliases) + { + const auto aliasKey = JsonKey(alias); + if (_json.isMember(aliasKey)) + { + const auto canonicalKey = JsonKey(canonical); + if (!_json.isMember(canonicalKey)) + { + // Copy via a temporary to avoid jsoncpp self-aliasing concerns. + const Json::Value moved = _json[aliasKey]; + _json[canonicalKey] = moved; + } + _json.removeMember(aliasKey); + } + } } // Method Description: @@ -131,24 +185,72 @@ Json::Value ColorScheme::ToJson() const { Json::Value json{ Json::ValueType::objectValue }; - JsonUtils::SetValueForKey(json, NameKey, _Name); - JsonUtils::SetValueForKey(json, ForegroundKey, _Foreground); - JsonUtils::SetValueForKey(json, BackgroundKey, _Background); - JsonUtils::SetValueForKey(json, SelectionBackgroundKey, _SelectionBackground); - JsonUtils::SetValueForKey(json, CursorColorKey, _CursorColor); - - for (size_t i = 0; i < ColorSchemeExpectedSize; ++i) + JsonUtils::CopyKeyIfPresent(_json, json, NameKey); + for (const auto& [key, _] : NamedColorDefaults) { - const auto& key = til::at(TableColorsMapping, i).first; - JsonUtils::SetValueForKey(json, key, til::at(_table, i)); + JsonUtils::CopyKeyIfPresent(_json, json, key); + } + + for (uint32_t i = 0; i < ColorSchemeExpectedSize; ++i) + { + JsonUtils::CopyKeyIfPresent(_json, json, til::at(TableColorsMapping, i).first); } return json; } -winrt::com_array ColorScheme::Table() const noexcept +winrt::hstring ColorScheme::Name() const { - return winrt::com_array{ _table }; + winrt::hstring name{}; + JsonUtils::GetValueForKey(_json, NameKey, name); + return name; +} + +void ColorScheme::Name(const winrt::hstring& value) +{ + JsonUtils::SetValueForKeyPreservingComments(_json, NameKey, value); +} + +// Reads a color from _json, falling back to defaultValue when the key is absent. +winrt::Microsoft::Terminal::Core::Color ColorScheme::_getColor(std::string_view key, const Core::Color& defaultValue) const +{ + auto value = defaultValue; + JsonUtils::GetValueForKey(_json, key, value); + return value; +} + +// Writes a color to _json while preserving any comment attached to the key. +void ColorScheme::_setColor(std::string_view key, const Core::Color& value) +{ + JsonUtils::SetValueForKeyPreservingComments(_json, key, value); +} + +#define GEN_NAMED_COLOR_ACCESSOR(name, jsonKey, defaultVal) \ + winrt::Microsoft::Terminal::Core::Color ColorScheme::name() const \ + { \ + return _getColor(jsonKey, static_cast(defaultVal)); \ + } \ + void ColorScheme::name(const Core::Color& value) \ + { \ + _setColor(jsonKey, value); \ + } +COLORSCHEME_NAMED_COLORS(GEN_NAMED_COLOR_ACCESSOR) +#undef GEN_NAMED_COLOR_ACCESSOR + +#undef COLORSCHEME_NAMED_COLORS + +// Method Description: +// - Returns the resolved 16-entry color table, reading each entry from _json and +// falling back to the Campbell value for any entry not set. +winrt::com_array ColorScheme::Table() const +{ + std::array table{}; + const auto campbell = Utils::CampbellColorTable(); + for (size_t i = 0; i < ColorSchemeExpectedSize; ++i) + { + til::at(table, i) = _getColor(til::at(TableColorsMapping, i).first, static_cast(til::at(campbell, i))); + } + return winrt::com_array{ table }; } // Method Description: @@ -158,10 +260,10 @@ winrt::com_array ColorScheme::Table() c // - value: the color value we are setting the color table color to // Return Value: // - none -void ColorScheme::SetColorTableEntry(uint8_t index, const Core::Color& value) noexcept +void ColorScheme::SetColorTableEntry(uint8_t index, const Core::Color& value) { - THROW_HR_IF(E_INVALIDARG, index >= _table.size()); - _table[index] = value; + THROW_HR_IF(E_INVALIDARG, index >= COLOR_TABLE_SIZE); + _setColor(til::at(TableColorsMapping, index).first, value); } bool ColorScheme::IsEquivalentForSettingsMergePurposes(const winrt::com_ptr& other) noexcept @@ -169,5 +271,14 @@ bool ColorScheme::IsEquivalentForSettingsMergePurposes(const winrt::com_ptr_table && _Background == other->_Background && _Foreground == other->_Foreground; + const auto thisTable = Table(); + const auto otherTable = other->Table(); + for (uint32_t i = 0; i < thisTable.size(); ++i) + { + if (!(thisTable[i] == otherTable[i])) + { + return false; + } + } + return Background() == other->Background() && Foreground() == other->Foreground(); } diff --git a/src/cascadia/TerminalSettingsModel/ColorScheme.h b/src/cascadia/TerminalSettingsModel/ColorScheme.h index cc634f1f09..5106874ea6 100644 --- a/src/cascadia/TerminalSettingsModel/ColorScheme.h +++ b/src/cascadia/TerminalSettingsModel/ColorScheme.h @@ -18,6 +18,7 @@ Author(s): #include "../../inc/conattrs.hpp" #include "DefaultSettings.h" +#include "JsonUtils.h" #include "ColorScheme.g.h" @@ -26,7 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation struct ColorScheme : ColorSchemeT { // A ColorScheme constructed with uninitialized_t - // leaves _table uninitialized. + // leaves _json empty to be populated lazily. struct uninitialized_t { }; @@ -46,22 +47,36 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static com_ptr FromJson(const Json::Value& json); Json::Value ToJson() const; - com_array Table() const noexcept; - void SetColorTableEntry(uint8_t index, const Core::Color& value) noexcept; + com_array Table() const; + void SetColorTableEntry(uint8_t index, const Core::Color& value); bool IsEquivalentForSettingsMergePurposes(const winrt::com_ptr& other) noexcept; - WINRT_PROPERTY(winrt::hstring, Name); + winrt::hstring Name() const; + void Name(const winrt::hstring& value); + + Core::Color Foreground() const; + void Foreground(const Core::Color& value); + + Core::Color Background() const; + void Background(const Core::Color& value); + + Core::Color SelectionBackground() const; + void SelectionBackground(const Core::Color& value); + + Core::Color CursorColor() const; + void CursorColor(const Core::Color& value); + WINRT_PROPERTY(OriginTag, Origin, OriginTag::None); - WINRT_PROPERTY(Core::Color, Foreground, static_cast(DEFAULT_FOREGROUND)); - WINRT_PROPERTY(Core::Color, Background, static_cast(DEFAULT_BACKGROUND)); - WINRT_PROPERTY(Core::Color, SelectionBackground, static_cast(DEFAULT_FOREGROUND)); - WINRT_PROPERTY(Core::Color, CursorColor, static_cast(DEFAULT_CURSOR_COLOR)); private: bool _layerJson(const Json::Value& json); + void _normalizeTableAliasKeys(); - std::array _table; + Core::Color _getColor(std::string_view key, const Core::Color& defaultValue) const; + void _setColor(std::string_view key, const Core::Color& value); + + Json::Value _json{ Json::ValueType::objectValue }; }; } diff --git a/src/cascadia/TerminalSettingsModel/JsonUtils.h b/src/cascadia/TerminalSettingsModel/JsonUtils.h index a7073fd23a..982d2ec68c 100644 --- a/src/cascadia/TerminalSettingsModel/JsonUtils.h +++ b/src/cascadia/TerminalSettingsModel/JsonUtils.h @@ -250,6 +250,44 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils SetValueForKey(json, key, target, ConversionTrait>{}); } + template + void SetValueForKeyPreservingComments(Json::Value& json, std::string_view key, const T& target) + { + const auto jsonKey = JsonKey(key); + + std::array savedComments; + std::array hadComment{}; + if (json.isMember(jsonKey)) + { + const auto& node = json[jsonKey]; + for (auto p = 0; p < Json::numberOfCommentPlacement; ++p) + { + const auto placement = static_cast(p); + if (node.hasComment(placement)) + { + savedComments[p] = node.getComment(placement); + hadComment[p] = true; + } + } + } + + SetValueForKey(json, key, target); + + // SetValueForKey skips empty optionals, so the key may legitimately not + // exist afterward -- only restore comments if the node is present. + if (json.isMember(jsonKey)) + { + auto& node = json[jsonKey]; + for (auto p = 0; p < Json::numberOfCommentPlacement; ++p) + { + if (hadComment[p]) + { + node.setComment(savedComments[p], static_cast(p)); + } + } + } + } + // CopyKeyIfPresent: copies a key's raw Json::Value from source to target // if it exists in source. No type conversion — just raw JSON copy. // Used in ToJson() to copy JSON-backed settings from _json to output. diff --git a/src/cascadia/TerminalSettingsModel/Theme.cpp b/src/cascadia/TerminalSettingsModel/Theme.cpp index e53c72a79e..fa3023a1bf 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.cpp +++ b/src/cascadia/TerminalSettingsModel/Theme.cpp @@ -35,25 +35,22 @@ static constexpr std::string_view DarkNameKey{ "dark" }; static constexpr wchar_t RegKeyDwm[] = L"Software\\Microsoft\\Windows\\DWM"; static constexpr wchar_t RegKeyAccentColor[] = L"AccentColor"; -#define THEME_SETTINGS_COPY(type, name, jsonKey, ...) \ - result->_##name = _##name; - -#define THEME_SETTINGS_TO_JSON(type, name, jsonKey, ...) \ - JsonUtils::SetValueForKey(json, jsonKey, _##name); - -#define THEME_OBJECT(className, macro) \ - winrt::com_ptr className::Copy() \ - { \ - auto result{ winrt::make_self() }; \ - macro(THEME_SETTINGS_COPY); \ - return result; \ - } \ - \ - Json::Value className::ToJson() \ - { \ - Json::Value json{ Json::ValueType::objectValue }; \ - macro(THEME_SETTINGS_TO_JSON); \ - return json; \ +#define THEME_OBJECT(className, macro) \ + winrt::com_ptr className::Copy() \ + { \ + auto result{ winrt::make_self() }; \ + result->_json = _json; \ + return result; \ + } \ + \ + Json::Value className::ToJson() \ + { \ + return _json; \ + } \ + \ + void className::LayerJson(const Json::Value& json) \ + { \ + JsonUtils::MergeJsonKeys(json, _json); \ } THEME_OBJECT(WindowTheme, MTSM_THEME_WINDOW_SETTINGS); @@ -61,8 +58,6 @@ THEME_OBJECT(SettingsTheme, MTSM_THEME_SETTINGS_SETTINGS); THEME_OBJECT(TabRowTheme, MTSM_THEME_TABROW_SETTINGS); THEME_OBJECT(TabTheme, MTSM_THEME_TAB_SETTINGS); -#undef THEME_SETTINGS_COPY -#undef THEME_SETTINGS_TO_JSON #undef THEME_OBJECT winrt::Microsoft::Terminal::Settings::Model::ThemeColor ThemeColor::FromColor(const winrt::Microsoft::Terminal::Core::Color& coreColor) noexcept @@ -183,14 +178,6 @@ uint8_t ThemeColor::UnfocusedTabOpacity() const noexcept return 0; } -#define THEME_SETTINGS_FROM_JSON(type, name, jsonKey, ...) \ - { \ - std::optional _val; \ - _val = JsonUtils::GetValueForKey>(json, jsonKey); \ - if (_val) \ - result->name(*_val); \ - } - #define THEME_OBJECT_CONVERTER(nameSpace, name, macro) \ template<> \ struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait \ @@ -200,7 +187,7 @@ uint8_t ThemeColor::UnfocusedTabOpacity() const noexcept if (json == Json::Value::null) \ return nullptr; \ auto result = winrt::make_self(); \ - macro(THEME_SETTINGS_FROM_JSON); \ + result->LayerJson(json); \ return *result; \ } \ \ @@ -225,8 +212,6 @@ THEME_OBJECT_CONVERTER(winrt::Microsoft::Terminal::Settings::Model, SettingsThem THEME_OBJECT_CONVERTER(winrt::Microsoft::Terminal::Settings::Model, TabRowTheme, MTSM_THEME_TABROW_SETTINGS); THEME_OBJECT_CONVERTER(winrt::Microsoft::Terminal::Settings::Model, TabTheme, MTSM_THEME_TAB_SETTINGS); -#undef THEME_SETTINGS_FROM_JSON -#undef THEME_SETTINGS_TO_JSON #undef THEME_OBJECT_CONVERTER Theme::Theme(const winrt::WUX::ElementTheme& requestedTheme) noexcept @@ -240,6 +225,7 @@ winrt::com_ptr Theme::Copy() const { auto theme{ winrt::make_self() }; + theme->_json = _json; theme->_Name = _Name; if (_Window) @@ -271,6 +257,7 @@ winrt::com_ptr Theme::Copy() const winrt::com_ptr Theme::FromJson(const Json::Value& json) { auto result = winrt::make_self(); + result->_json = json; JsonUtils::GetValueForKey(json, NameKey, result->_Name); @@ -347,14 +334,30 @@ void Theme::LogSettingChanges(std::set& changes, const std::string_ // - the JsonObject representing this instance Json::Value Theme::ToJson() const { - Json::Value json{ Json::ValueType::objectValue }; + Json::Value json{ _json }; - JsonUtils::SetValueForKey(json, NameKey, _Name); + // Overlay Name, preserving its comment if present. + auto nameJson{ JsonUtils::ConversionTrait().ToJson(_Name) }; + if (json.isMember(JsonKey(NameKey))) + { + json[JsonKey(NameKey)].copyPayload(nameJson); + } + else + { + json[JsonKey(NameKey)] = nameJson; + } - // Don't serialize anything if the object is null. -#define THEME_SETTINGS_TO_JSON(type, name, jsonKey, ...) \ - if (_##name) \ - JsonUtils::SetValueForKey(json, jsonKey, _##name); +#define THEME_SETTINGS_TO_JSON(type, name, jsonKey, ...) \ + if (_##name) \ + { \ + Json::Value subJson{ Json::ValueType::objectValue }; \ + JsonUtils::SetValueForKey(subJson, jsonKey, _##name); \ + json[JsonKey(jsonKey)].copyPayload(subJson[JsonKey(jsonKey)]); \ + } \ + else \ + { \ + json.removeMember(JsonKey(jsonKey)); \ + } MTSM_THEME_SETTINGS(THEME_SETTINGS_TO_JSON) #undef THEME_SETTINGS_TO_JSON diff --git a/src/cascadia/TerminalSettingsModel/Theme.h b/src/cascadia/TerminalSettingsModel/Theme.h index e0892733ae..7c3a9371cc 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.h +++ b/src/cascadia/TerminalSettingsModel/Theme.h @@ -72,13 +72,40 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation #define THEME_SETTINGS_INITIALIZE(type, name, jsonKey, ...) \ WINRT_PROPERTY(type, name, ##__VA_ARGS__) -#define THEME_OBJECT(className, macro) \ - struct className : className##T \ - { \ - winrt::com_ptr Copy(); \ - Json::Value ToJson(); \ - \ - macro(THEME_SETTINGS_INITIALIZE); \ +#define THEME_SETTINGS_JSON_INITIALIZE(type, name, jsonKey, ...) \ +public: \ + type name() const \ + { \ + if (auto val{ ::Microsoft::Terminal::Settings::Model::JsonUtils::GetValueForKey>(_json, jsonKey) }) \ + { \ + return *val; \ + } \ + return type{ __VA_ARGS__ }; \ + } \ + void name(const type& value) \ + { \ + ::Microsoft::Terminal::Settings::Model::JsonUtils::SetValueForKey(_json, jsonKey, value); \ + } \ + bool Has##name() const \ + { \ + return _json.isMember(jsonKey) && !_json[jsonKey].isNull(); \ + } \ + void Clear##name() \ + { \ + _json.removeMember(jsonKey); \ + } + +#define THEME_OBJECT(className, macro) \ + struct className : className##T \ + { \ + winrt::com_ptr Copy(); \ + Json::Value ToJson(); \ + void LayerJson(const Json::Value& json); \ + \ + macro(THEME_SETTINGS_JSON_INITIALIZE); \ + \ + private: \ + Json::Value _json{ Json::ValueType::objectValue }; \ }; THEME_OBJECT(WindowTheme, MTSM_THEME_WINDOW_SETTINGS); @@ -109,9 +136,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation MTSM_THEME_SETTINGS(THEME_SETTINGS_INITIALIZE) private: + Json::Value _json{ Json::ValueType::objectValue }; }; #undef THEME_SETTINGS_INITIALIZE +#undef THEME_SETTINGS_JSON_INITIALIZE #undef THEME_OBJECT } diff --git a/src/cascadia/UnitTests_SettingsModel/ColorSchemeTests.cpp b/src/cascadia/UnitTests_SettingsModel/ColorSchemeTests.cpp index 593ded5d43..ebedb06bb8 100644 --- a/src/cascadia/UnitTests_SettingsModel/ColorSchemeTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/ColorSchemeTests.cpp @@ -25,6 +25,7 @@ namespace SettingsModelUnitTests TEST_METHOD(ParseSimpleColorScheme); TEST_METHOD(LayerColorSchemesOnArray); TEST_METHOD(UpdateSchemeReferences); + TEST_METHOD(RoundtripPreservesComments); TEST_METHOD(LayerColorSchemesWithUserOwnedCollision); TEST_METHOD(LayerColorSchemesWithUserOwnedCollisionRetargetsAllProfiles); @@ -87,6 +88,75 @@ namespace SettingsModelUnitTests VERIFY_ARE_EQUAL(schemeObject, outJson); } + void ColorSchemeTests::RoundtripPreservesComments() + { + // A complete scheme carrying comments on a few keys: + // - FG_NOTE: a standalone comment line before "foreground" (commentBefore) + // - BG_NOTE: a trailing same-line comment after "background" + // - ALIAS_NOTE: a comment before the alias key "magenta", which must + // travel with the value when it is normalized onto "purple" + // Distinct marker text avoids substring collisions when we scan the output. + const std::string schemeWithComments{ R"({ + "name": "Commented", + // FG_NOTE + "foreground": "#F2F2F2", + "background": "#0C0C0C", // BG_NOTE + "selectionBackground": "#131313", + "cursorColor": "#FFFFFF", + "black": "#0C0C0C", + "red": "#C50F1F", + "green": "#13A10E", + "yellow": "#C19C00", + "blue": "#0037DA", + // ALIAS_NOTE + "magenta": "#881798", + "cyan": "#3A96DD", + "white": "#CCCCCC", + "brightBlack": "#767676", + "brightRed": "#E74856", + "brightGreen": "#16C60C", + "brightYellow": "#F9F1A5", + "brightBlue": "#3B78FF", + "brightPurple": "#B4009E", + "brightCyan": "#61D6D6", + "brightWhite": "#F2F2F2" + })" }; + + const auto schemeObject = VerifyParseSucceeded(schemeWithComments); + const auto scheme = ColorScheme::FromJson(schemeObject); + VERIFY_IS_NOT_NULL(scheme); + + auto serialize = [](const Json::Value& json) { + Json::StreamWriterBuilder wbuilder; + wbuilder.settings_["commentStyle"] = "All"; + return Json::writeString(wbuilder, json); + }; + + Log::Comment(L"Comments on known keys survive a load -> save roundtrip"); + auto outJson{ scheme->ToJson() }; + + // Precise node-level check for the most reliable placement. + VERIFY_IS_TRUE(outJson["foreground"].hasComment(Json::commentBefore)); + + // End-to-end: the comment text lands in the serialized output. + auto serialized = serialize(outJson); + VERIFY_IS_TRUE(serialized.find("FG_NOTE") != std::string::npos); + VERIFY_IS_TRUE(serialized.find("BG_NOTE") != std::string::npos); + + Log::Comment(L"A comment on an alias key moves to its canonical key"); + VERIFY_IS_FALSE(outJson.isMember("magenta")); + VERIFY_IS_TRUE(outJson.isMember("purple")); + VERIFY_IS_TRUE(outJson["purple"].hasComment(Json::commentBefore)); + VERIFY_IS_TRUE(serialized.find("ALIAS_NOTE") != std::string::npos); + + Log::Comment(L"Editing a property preserves its existing comment"); + scheme->Foreground(rgb(0x12, 0x34, 0x56)); + outJson = scheme->ToJson(); + VERIFY_IS_TRUE(outJson["foreground"].hasComment(Json::commentBefore)); + serialized = serialize(outJson); + VERIFY_IS_TRUE(serialized.find("FG_NOTE") != std::string::npos); + } + void ColorSchemeTests::LayerColorSchemesOnArray() { static constexpr std::string_view inboxSettings{ R"({ diff --git a/src/cascadia/UnitTests_SettingsModel/ThemeTests.cpp b/src/cascadia/UnitTests_SettingsModel/ThemeTests.cpp index 34b4e5eb73..97769f0c29 100644 --- a/src/cascadia/UnitTests_SettingsModel/ThemeTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/ThemeTests.cpp @@ -28,6 +28,7 @@ namespace SettingsModelUnitTests TEST_METHOD(ParseNullWindowTheme); TEST_METHOD(ParseThemeWithNullThemeColor); TEST_METHOD(InvalidCurrentTheme); + TEST_METHOD(RoundtripThemeComments); static Core::Color rgb(uint8_t r, uint8_t g, uint8_t b) noexcept { @@ -265,4 +266,47 @@ namespace SettingsModelUnitTests throw e; } } + + void ThemeTests::RoundtripThemeComments() + { + Log::Comment(L"A theme loaded then re-serialized should preserve JSON comments and unknown keys (GH#12424)."); + + // Comments are placed at representative positions: + // - before/after the "name" member + // - before a sub-object member ("window") -- the outer comment + // - before a key inside a sub-object ("useMica") -- an inner comment + // An unknown key ("unknownThemeKey") exercises forward-compatible retention. + static constexpr std::string_view commentedTheme{ R"json({ + // comment-before-name + "name": "commented", // comment-after-name + // comment-before-window + "window": + { + // comment-before-useMica + "useMica": true + }, + "unknownThemeKey": "keep-me" + })json" }; + + const auto themeObject = VerifyParseSucceeded(commentedTheme); + const auto theme = Theme::FromJson(themeObject); + VERIFY_ARE_EQUAL(L"commented", theme->Name()); + + const auto output = toString(theme->ToJson()); + Log::Comment(NoThrowString().Format(til::u8u16(output).c_str())); + + // Every comment must survive the round-trip. + VERIFY_IS_TRUE(output.find("comment-before-name") != std::string::npos, L"comment before 'name' preserved"); + VERIFY_IS_TRUE(output.find("comment-after-name") != std::string::npos, L"trailing comment after 'name' preserved"); + VERIFY_IS_TRUE(output.find("comment-before-window") != std::string::npos, L"outer comment before sub-object 'window' preserved"); + VERIFY_IS_TRUE(output.find("comment-before-useMica") != std::string::npos, L"inner comment inside sub-object preserved"); + + // Unknown keys must round-trip rather than being silently dropped. + VERIFY_IS_TRUE(output.find("unknownThemeKey") != std::string::npos, L"unknown key preserved"); + VERIFY_IS_TRUE(output.find("keep-me") != std::string::npos, L"unknown key value preserved"); + + // And the actual values must still be correct. + VERIFY_IS_NOT_NULL(theme->Window()); + VERIFY_ARE_EQUAL(true, theme->Window().UseMica()); + } }