From c24194a3f3cdfe00e12bf32b9fcd3deb0ae3ff3a Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 10 Feb 2021 14:11:56 -0800 Subject: [PATCH] Bugfix: update color scheme references properly (#9103) `CascadiaSettings::UpdateColorSchemeReferences` had two bugs in it: 1. we would never check/update the base layer 2. we would explicitly set the color scheme on a profile referencing the old name This PR fixes both of those issues by checking/updating the base layer, and ensuring that we check if a profile has an explicit reference before updating it. Since the affected code is in TSM, I also created an automated local test. ## Validation Steps Performed Bug repro steps. Specifically tested [DHowett's scenario] too. Test added. Closes #9094 [DHowett's scenario]: https://github.com/microsoft/terminal/issues/9094#issuecomment-776412781 (cherry picked from commit 42511265e55ac80433cd1b27b55fe42de6268c0f) --- .../ColorSchemeTests.cpp | 78 +++++++++++++++++++ .../CascadiaSettings.cpp | 10 ++- 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp b/src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp index 64b96dd8490..d2a87b995b1 100644 --- a/src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp @@ -35,6 +35,7 @@ namespace SettingsModelLocalTests TEST_METHOD(CanLayerColorScheme); TEST_METHOD(LayerColorSchemeProperties); TEST_METHOD(LayerColorSchemesOnArray); + TEST_METHOD(UpdateSchemeReferences); TEST_CLASS_SETUP(ClassSetup) { @@ -290,4 +291,81 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(ARGB(0, 7, 7, 7), scheme2->_Background); } } + + void ColorSchemeTests::UpdateSchemeReferences() + { + const std::string settingsString{ R"json({ + "defaultProfile": "Inherited reference", + "profiles": { + "defaults": { + "colorScheme": "Scheme 1" + }, + "list": [ + { + "name": "Explicit scheme reference", + "colorScheme": "Scheme 1" + }, + { + "name": "Explicit reference; hidden", + "colorScheme": "Scheme 1", + "hidden": true + }, + { + "name": "Inherited reference" + }, + { + "name": "Different reference", + "colorScheme": "Scheme 2" + } + ] + }, + "schemes": [ + { "name": "Scheme 1" }, + { "name": "Scheme 2" }, + { "name": "Scheme 1 (renamed)" } + ] + })json" }; + + auto settings{ winrt::make_self(false) }; + settings->_ParseJsonString(settingsString, false); + settings->_ApplyDefaultsFromUserSettings(); + settings->LayerJson(settings->_userSettings); + settings->_ValidateSettings(); + + // update all references to "Scheme 1" + const auto newName{ L"Scheme 1 (renamed)" }; + settings->UpdateColorSchemeReferences(L"Scheme 1", newName); + + // verify profile defaults + Log::Comment(L"Profile Defaults"); + VERIFY_ARE_EQUAL(newName, settings->ProfileDefaults().ColorSchemeName()); + VERIFY_IS_TRUE(settings->ProfileDefaults().HasColorSchemeName()); + + // verify all other profiles + const auto& profiles{ settings->AllProfiles() }; + { + const auto& prof{ profiles.GetAt(0) }; + Log::Comment(prof.Name().c_str()); + VERIFY_ARE_EQUAL(newName, prof.ColorSchemeName()); + VERIFY_IS_TRUE(prof.HasColorSchemeName()); + } + { + const auto& prof{ profiles.GetAt(1) }; + Log::Comment(prof.Name().c_str()); + VERIFY_ARE_EQUAL(newName, prof.ColorSchemeName()); + VERIFY_IS_TRUE(prof.HasColorSchemeName()); + } + { + const auto& prof{ profiles.GetAt(2) }; + Log::Comment(prof.Name().c_str()); + VERIFY_ARE_EQUAL(newName, prof.ColorSchemeName()); + VERIFY_IS_FALSE(prof.HasColorSchemeName()); + } + { + const auto& prof{ profiles.GetAt(3) }; + Log::Comment(prof.Name().c_str()); + VERIFY_ARE_EQUAL(L"Scheme 2", prof.ColorSchemeName()); + VERIFY_IS_TRUE(prof.HasColorSchemeName()); + } + } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 450e0d547de..52c4024a2ab 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -874,10 +874,18 @@ winrt::Microsoft::Terminal::Settings::Model::ColorScheme CascadiaSettings::GetCo // - void CascadiaSettings::UpdateColorSchemeReferences(const hstring oldName, const hstring newName) { + // update profiles.defaults, if necessary + if (_userDefaultProfileSettings && + _userDefaultProfileSettings->HasColorSchemeName() && + _userDefaultProfileSettings->ColorSchemeName() == oldName) + { + _userDefaultProfileSettings->ColorSchemeName(newName); + } + // update all profiles referencing this color scheme for (const auto& profile : _allProfiles) { - if (profile.ColorSchemeName() == oldName) + if (profile.HasColorSchemeName() && profile.ColorSchemeName() == oldName) { profile.ColorSchemeName(newName); }