Skip to content

Commit

Permalink
Bugfix: update color scheme references properly (#9103)
Browse files Browse the repository at this point in the history
`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]: #9094 (comment)

(cherry picked from commit 4251126)
  • Loading branch information
carlos-zamora authored and DHowett committed Feb 10, 2021
1 parent 3ab1d72 commit c24194a
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 1 deletion.
78 changes: 78 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ namespace SettingsModelLocalTests
TEST_METHOD(CanLayerColorScheme);
TEST_METHOD(LayerColorSchemeProperties);
TEST_METHOD(LayerColorSchemesOnArray);
TEST_METHOD(UpdateSchemeReferences);

TEST_CLASS_SETUP(ClassSetup)
{
Expand Down Expand Up @@ -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<CascadiaSettings>(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());
}
}
}
10 changes: 9 additions & 1 deletion src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -874,10 +874,18 @@ winrt::Microsoft::Terminal::Settings::Model::ColorScheme CascadiaSettings::GetCo
// - <none>
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);
}
Expand Down

0 comments on commit c24194a

Please sign in to comment.