Skip to content

Commit

Permalink
Add a warning when a profile has an unknown color scheme (#3033)
Browse files Browse the repository at this point in the history 
Add a warning when the user sets their colorScheme to a scheme that doesn't exist. When that occurs, we'll set their color table to the campbell scheme, to prevent it from being just entirely black.

This commit also switches scheme storage to a map keyed on name.

Closes #2547
  • Loading branch information
@zadjii-msft @DHowett
zadjii-msft authored and DHowett committed Oct 15, 2019
1 parent 6708556 commit 5d17557
Show file tree
Hide file tree
Showing 13 changed files with 249 additions and 158 deletions.
112 changes: 75 additions & 37 deletions src/cascadia/LocalTests_TerminalApp/ColorSchemeTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ namespace TerminalAppLocalTests
"background": "#050505"
})" };
const std::string scheme3String{ R"({
// "name": "scheme3",
// by not providing a name, the scheme will have the name ""
"foreground": "#060606",
"background": "#070707"
})" };
Expand All @@ -188,47 +188,85 @@ namespace TerminalAppLocalTests
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));

settings._LayerOrCreateColorScheme(scheme0Json);
VERIFY_ARE_EQUAL(1u, settings._globals.GetColorSchemes().size());
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), settings._globals.GetColorSchemes().at(0)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), settings._globals.GetColorSchemes().at(0)._defaultBackground);
{
for (auto& kv : settings._globals._colorSchemes)
{
Log::Comment(NoThrowString().Format(
L"kv:%s->%s", kv.first.data(), kv.second.GetName().data()));
}
VERIFY_ARE_EQUAL(1u, settings._globals.GetColorSchemes().size());

VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;

VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), scheme0._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), scheme0._defaultBackground);
}

settings._LayerOrCreateColorScheme(scheme1Json);
VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), settings._globals.GetColorSchemes().at(0)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), settings._globals.GetColorSchemes().at(0)._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), settings._globals.GetColorSchemes().at(1)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), settings._globals.GetColorSchemes().at(1)._defaultBackground);

{
VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());

VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;
VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme1") != settings._globals._colorSchemes.end());
auto scheme1 = settings._globals._colorSchemes.find(L"scheme1")->second;

VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 0, 0, 0), scheme0._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 1, 1, 1), scheme0._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), scheme1._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), scheme1._defaultBackground);
}
settings._LayerOrCreateColorScheme(scheme2Json);
VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), settings._globals.GetColorSchemes().at(0)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), settings._globals.GetColorSchemes().at(0)._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), settings._globals.GetColorSchemes().at(1)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), settings._globals.GetColorSchemes().at(1)._defaultBackground);

{
VERIFY_ARE_EQUAL(2u, settings._globals.GetColorSchemes().size());

VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;
VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme1") != settings._globals._colorSchemes.end());
auto scheme1 = settings._globals._colorSchemes.find(L"scheme1")->second;

VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), scheme0._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), scheme0._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), scheme1._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), scheme1._defaultBackground);
}
settings._LayerOrCreateColorScheme(scheme3Json);
VERIFY_ARE_EQUAL(3u, settings._globals.GetColorSchemes().size());
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), settings._globals.GetColorSchemes().at(0)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), settings._globals.GetColorSchemes().at(0)._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), settings._globals.GetColorSchemes().at(1)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), settings._globals.GetColorSchemes().at(1)._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 6, 6, 6), settings._globals.GetColorSchemes().at(2)._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 7, 7, 7), settings._globals.GetColorSchemes().at(2)._defaultBackground);

{
VERIFY_ARE_EQUAL(3u, settings._globals.GetColorSchemes().size());

VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme0") != settings._globals._colorSchemes.end());
auto scheme0 = settings._globals._colorSchemes.find(L"scheme0")->second;
VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"scheme1") != settings._globals._colorSchemes.end());
auto scheme1 = settings._globals._colorSchemes.find(L"scheme1")->second;
VERIFY_IS_TRUE(settings._globals._colorSchemes.find(L"") != settings._globals._colorSchemes.end());
auto scheme2 = settings._globals._colorSchemes.find(L"")->second;

VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme0Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme1Json));
VERIFY_IS_NOT_NULL(settings._FindMatchingColorScheme(scheme2Json));
VERIFY_IS_NULL(settings._FindMatchingColorScheme(scheme3Json));
VERIFY_ARE_EQUAL(ARGB(0, 4, 4, 4), scheme0._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 5, 5, 5), scheme0._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 2, 2, 2), scheme1._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 3, 3, 3), scheme1._defaultBackground);
VERIFY_ARE_EQUAL(ARGB(0, 6, 6, 6), scheme2._defaultForeground);
VERIFY_ARE_EQUAL(ARGB(0, 7, 7, 7), scheme2._defaultBackground);
}
}
}
67 changes: 65 additions & 2 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ namespace TerminalAppLocalTests
TEST_METHOD(TestLayeringNameOnlyProfiles);
TEST_METHOD(TestExplodingNameOnlyProfiles);
TEST_METHOD(TestHideAllProfiles);
TEST_METHOD(TestInvalidColorSchemeName);

TEST_METHOD(TestLayerGlobalsOnRoot);

Expand Down Expand Up @@ -395,9 +396,10 @@ namespace TerminalAppLocalTests

settings->_ValidateSettings();

VERIFY_ARE_EQUAL(2u, settings->_warnings.size());
VERIFY_ARE_EQUAL(3u, settings->_warnings.size());
VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::DuplicateProfile, settings->_warnings.at(0));
VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingDefaultProfile, settings->_warnings.at(1));
VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme, settings->_warnings.at(2));

VERIFY_ARE_EQUAL(3u, settings->_profiles.size());
VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid());
Expand Down Expand Up @@ -795,6 +797,9 @@ namespace TerminalAppLocalTests
{
"name" : "profile1"
}
],
"schemes": [
{ "name": "Campbell" }
]
})" };

Expand Down Expand Up @@ -1194,6 +1199,65 @@ namespace TerminalAppLocalTests
}
}

void SettingsTests::TestInvalidColorSchemeName()
{
Log::Comment(NoThrowString().Format(
L"Ensure that setting a profile's scheme to a non-existent scheme causes a warning."));

const std::string settings0String{ R"(
{
"profiles": [
{
"name" : "profile0",
"colorScheme": "schemeOne"
},
{
"name" : "profile1",
"colorScheme": "InvalidSchemeName"
},
{
"name" : "profile2"
// Will use the Profile default value, "Campbell"
}
],
"schemes": [
{
"name": "schemeOne",
"foreground": "#111111"
},
{
"name": "schemeTwo",
"foreground": "#222222"
}
]
})" };

VerifyParseSucceeded(settings0String);

CascadiaSettings settings;
settings._ParseJsonString(settings0String, false);
settings.LayerJson(settings._userSettings);

VERIFY_ARE_EQUAL(3u, settings._profiles.size());
VERIFY_ARE_EQUAL(2u, settings._globals._colorSchemes.size());

VERIFY_ARE_EQUAL(L"schemeOne", settings._profiles.at(0)._schemeName.value());
VERIFY_ARE_EQUAL(L"InvalidSchemeName", settings._profiles.at(1)._schemeName.value());
VERIFY_ARE_EQUAL(L"Campbell", settings._profiles.at(2)._schemeName.value());

settings._ValidateAllSchemesExist();

VERIFY_ARE_EQUAL(1u, settings._warnings.size());
VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme, settings._warnings.at(0));

VERIFY_ARE_EQUAL(3u, settings._profiles.size());
VERIFY_ARE_EQUAL(2u, settings._globals._colorSchemes.size());

VERIFY_ARE_EQUAL(L"schemeOne", settings._profiles.at(0)._schemeName.value());
VERIFY_ARE_EQUAL(L"Campbell", settings._profiles.at(1)._schemeName.value());
VERIFY_ARE_EQUAL(L"Campbell", settings._profiles.at(2)._schemeName.value());
}

void SettingsTests::TestLayerGlobalsOnRoot()
{
// Test for microsoft/terminal#2906. We added the ability for the root
Expand Down Expand Up @@ -1302,5 +1366,4 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(guid3, settings._globals._defaultProfile);
}
}

}
5 changes: 3 additions & 2 deletions src/cascadia/TerminalApp/App.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ namespace winrt
// !!! IMPORTANT !!!
// Make sure that these keys are in the same order as the
// SettingsLoadWarnings/Errors enum is!
static const std::array<std::wstring_view, 2> settingsLoadWarningsLabels {
static const std::array<std::wstring_view, 3> settingsLoadWarningsLabels {
L"MissingDefaultProfileText",
L"DuplicateProfileText"
L"DuplicateProfileText",
L"UnknownColorSchemeText"
};
static const std::array<std::wstring_view, 2> settingsLoadErrorsLabels {
L"NoProfilesText",
Expand Down
36 changes: 35 additions & 1 deletion src/cascadia/TerminalApp/CascadiaSettings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,10 @@ void CascadiaSettings::_ValidateSettings()
_ValidateNoDuplicateProfiles();
_ValidateDefaultProfileExists();

// TODO:GH#2547 ensure that all the profile's color scheme names are
// Ensure that all the profile's color scheme names are
// actually the names of schemes we've parsed. If the scheme doesn't exist,
// just use the hardcoded defaults
_ValidateAllSchemesExist();

// TODO:GH#2548 ensure there's at least one key bound. Display a warning if
// there's _NO_ keys bound to any actions. That's highly irregular, and
Expand Down Expand Up @@ -366,3 +367,36 @@ void CascadiaSettings::_RemoveHiddenProfiles()
throw ::TerminalApp::SettingsException(::TerminalApp::SettingsLoadErrors::AllProfilesHidden);
}
}

// Method Description:
// - Ensures that every profile has a valid "color scheme" set. If any profile
// has a colorScheme set to a value which is _not_ the name of an actual color
// scheme, we'll set the color table of the profile to something reasonable.
// Arguments:
// - <none>
// Return Value:
// - <none>
// - Appends a SettingsLoadWarnings::UnknownColorScheme to our list of warnings if
// we find any such duplicate.
void CascadiaSettings::_ValidateAllSchemesExist()
{
bool foundInvalidScheme = false;
for (auto& profile : _profiles)
{
auto schemeName = profile.GetSchemeName();
if (schemeName.has_value())
{
const auto found = _globals.GetColorSchemes().find(schemeName.value());
if (found == _globals.GetColorSchemes().end())
{
profile.SetColorScheme({ L"Campbell" });
foundInvalidScheme = true;
}
}
}

if (foundInvalidScheme)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme);
}
}
3 changes: 1 addition & 2 deletions src/cascadia/TerminalApp/CascadiaSettings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ class TerminalApp::CascadiaSettings final

static std::unique_ptr<CascadiaSettings> LoadDefaults();
static std::unique_ptr<CascadiaSettings> LoadAll();
void SaveAll() const;

winrt::Microsoft::Terminal::Settings::TerminalSettings MakeSettings(std::optional<GUID> profileGuid) const;

Expand All @@ -58,7 +57,6 @@ class TerminalApp::CascadiaSettings final

winrt::TerminalApp::AppKeyBindings GetKeybindings() const noexcept;

Json::Value ToJson() const;
static std::unique_ptr<CascadiaSettings> FromJson(const Json::Value& json);
void LayerJson(const Json::Value& json);

Expand Down Expand Up @@ -104,6 +102,7 @@ class TerminalApp::CascadiaSettings final
void _ValidateNoDuplicateProfiles();
void _ReorderProfilesToMatchUserSettingsOrder();
void _RemoveHiddenProfiles();
void _ValidateAllSchemesExist();

friend class TerminalAppLocalTests::SettingsTests;
friend class TerminalAppLocalTests::ProfileTests;
Expand Down
Loading

0 comments on commit 5d17557

Please sign in to comment.