Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a warning when a profile has an unknown color scheme #3033

Merged
merged 5 commits into from
Oct 15, 2019
Merged

Add a warning when a profile has an unknown color scheme #3033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a4fc24
fixes #2547
zadjii-msft Oct 2, 2019
3206644
Well this was an enormous pain
zadjii-msft Oct 3, 2019
bfc6c61
Final bits of polish on the vector->map change
zadjii-msft Oct 4, 2019
5988373
CR feedback
DHowett Oct 15, 2019
6bca16e
Merge remote-tracking branch 'github/master' into HEAD
DHowett Oct 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 75 additions & 37 deletions src/cascadia/LocalTests_TerminalApp/ColorSchemeTests.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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" });
DHowett-MSFT marked this conversation as resolved.
Show resolved Hide resolved
foundInvalidScheme = true;
}
}
}

if (foundInvalidScheme)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme);
}
}
3 changes: 1 addition & 2 deletions src/cascadia/TerminalApp/CascadiaSettings.h
View file
Open in desktop
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