Skip to content

Commit

Permalink
Add support for switching the scheme based on the app's theme (#14064)
Browse files Browse the repository at this point in the history 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
This pull request solved the problem of users not being able to set color schemes specifically for dark or light mode. Now the code has been updated to accept a dark and light color scheme in the json. The old setting is still compatible. Keep in mind if you update your color scheme through the settings UI, it will set both dark and light to the color scheme selected. This is because the settings UI update for selecting both Dark and Light color schemes is not supported yet.

This also solves the problem of the UI not using the system OS theme. Now you can select system theme and your color scheme will be selected based on if the system theme is dark or light.


<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References
#4066 

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #4066 
* [x] Closes #14050
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA. 
* [x] Tests added/passed I believe so, added one test to ColorSchemeTests.cpp and I believe it passed. Also had to modify TerminalSettingsTests.cpp to accept the new ApplyAppearanceSettings function template
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [x] Schema updated.
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #4066 and also teams messages with @carlos-zamora 

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
-Removed ColorSchemeName from MTSMSettings.h in order to process the setting for both string and object.
-Added DarkColorSchemeName and LightColorSchemeName properties to the AppearanceConfig to replace ColorSchemeName.
-Hacked a few processes to play nice with all 3 properties listed above as in some cases around the UI, we need to still use the ColorSchemeName. Once we change the UI I believe we can go back to just Dark and LightColorSchemeName
-Added and Updated Test to align to the new code.

Acceptable Json values,

"colorScheme": 
                {
                    "dark": "Campbell",
                    "light": "Campbell"
                }
or

"colorScheme": "Campbell"

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Individual testing along with the test case added.
  • Loading branch information
@bennettnicholas
bennettnicholas authored Dec 6, 2022
1 parent 32bf894 commit da2b80b
Show file tree
Hide file tree
Showing 33 changed files with 347 additions and 143 deletions.
33 changes: 31 additions & 2 deletions doc/cascadia/profiles.schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,14 @@
"properties": {
"colorScheme": {
"description": "The name of a color scheme to use when unfocused.",
"type": "string"
"oneOf": [
{
"$ref": "#/$defs/SchemePair"
},
{
"type": "string"
}
]
},
"foreground": {
"$ref": "#/$defs/Color",
Expand Down Expand Up @@ -233,6 +240,21 @@
},
"type": "object"
},
"SchemePair": {
"description": "Contains both a light and dark color scheme for the Terminal to use, depending on the theme of the application.",
"properties": {
"light": {
"default": "Campbell",
"description": "Name of the scheme to use when the app is using light theme",
"type": "string"
},
"dark": {
"default": "Campbell",
"description": "Name of the scheme to use when the app is using dark theme",
"type": "string"
},
}
},
"FontConfig": {
"properties": {
"face": {
Expand Down Expand Up @@ -2170,7 +2192,14 @@
"colorScheme": {
"default": "Campbell",
"description": "Name of the terminal color scheme to use. Color schemes are defined under \"schemes\".",
"type": "string"
"oneOf": [
{
"$ref": "#/$defs/SchemePair"
},
{
"type": "string"
}
]
},
"commandline": {
"description": "Executable used in the profile.",
Expand Down
98 changes: 88 additions & 10 deletions src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,30 @@ namespace SettingsModelLocalTests
{
"name": "Different reference",
"colorScheme": "One Half Dark"
},
{
"name": "rename neither",
"colorScheme":
{
"dark": "One Half Dark",
"light": "One Half Light"
}
},
{
"name": "rename only light",
"colorScheme":
{
"dark": "One Half Dark",
"light": "Campbell"
}
},
{
"name": "rename only dark",
"colorScheme":
{
"dark": "Campbell",
"light": "One Half Light"
}
}
]
},
Expand Down Expand Up @@ -289,38 +313,92 @@ namespace SettingsModelLocalTests
"selectionBackground": "#FFFFFF",
"white": "#DCDFE4",
"yellow": "#E5C07B"
},
{
"name": "One Half Light",
"foreground": "#383A42",
"background": "#FAFAFA",
"cursorColor": "#4F525D",
"black": "#383A42",
"red": "#E45649",
"green": "#50A14F",
"yellow": "#C18301",
"blue": "#0184BC",
"purple": "#A626A4",
"cyan": "#0997B3",
"white": "#FAFAFA",
"brightBlack": "#4F525D",
"brightRed": "#DF6C75",
"brightGreen": "#98C379",
"brightYellow": "#E4C07A",
"brightBlue": "#61AFEF",
"brightPurple": "#C577DD",
"brightCyan": "#56B5C1",
"brightWhite": "#FFFFFF"
}
]
})json" };

const auto settings{ winrt::make_self<CascadiaSettings>(settingsString) };

const auto newName{ L"Campbell (renamed)" };

settings->UpdateColorSchemeReferences(L"Campbell", newName);

VERIFY_ARE_EQUAL(newName, settings->ProfileDefaults().DefaultAppearance().ColorSchemeName());
VERIFY_IS_TRUE(settings->ProfileDefaults().DefaultAppearance().HasColorSchemeName());
VERIFY_ARE_EQUAL(newName, settings->ProfileDefaults().DefaultAppearance().DarkColorSchemeName());
VERIFY_ARE_EQUAL(newName, settings->ProfileDefaults().DefaultAppearance().LightColorSchemeName());
VERIFY_IS_TRUE(settings->ProfileDefaults().DefaultAppearance().HasDarkColorSchemeName());
VERIFY_IS_TRUE(settings->ProfileDefaults().DefaultAppearance().HasLightColorSchemeName());

const auto& profiles{ settings->AllProfiles() };
{
const auto& prof{ profiles.GetAt(0) };
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().ColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasColorSchemeName());
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().DarkColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasDarkColorSchemeName());
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().LightColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasLightColorSchemeName());
}
{
const auto& prof{ profiles.GetAt(1) };
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().ColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasColorSchemeName());
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().DarkColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasDarkColorSchemeName());
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().LightColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasLightColorSchemeName());
}
{
const auto& prof{ profiles.GetAt(2) };
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().ColorSchemeName());
VERIFY_IS_FALSE(prof.DefaultAppearance().HasColorSchemeName());
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().DarkColorSchemeName());
VERIFY_IS_FALSE(prof.DefaultAppearance().HasDarkColorSchemeName());
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().LightColorSchemeName());
VERIFY_IS_FALSE(prof.DefaultAppearance().HasLightColorSchemeName());
}
{
const auto& prof{ profiles.GetAt(3) };
VERIFY_ARE_EQUAL(L"One Half Dark", prof.DefaultAppearance().ColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasColorSchemeName());
VERIFY_ARE_EQUAL(L"One Half Dark", prof.DefaultAppearance().DarkColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasDarkColorSchemeName());
VERIFY_ARE_EQUAL(L"One Half Dark", prof.DefaultAppearance().LightColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasLightColorSchemeName());
}
{
const auto& prof{ profiles.GetAt(4) };
VERIFY_ARE_EQUAL(L"One Half Dark", prof.DefaultAppearance().DarkColorSchemeName());
VERIFY_ARE_EQUAL(L"One Half Light", prof.DefaultAppearance().LightColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasDarkColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasLightColorSchemeName());
}
{
const auto& prof{ profiles.GetAt(5) };
VERIFY_ARE_EQUAL(L"One Half Dark", prof.DefaultAppearance().DarkColorSchemeName());
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().LightColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasDarkColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasLightColorSchemeName());
}
{
const auto& prof{ profiles.GetAt(6) };
VERIFY_ARE_EQUAL(newName, prof.DefaultAppearance().DarkColorSchemeName());
VERIFY_ARE_EQUAL(L"One Half Light", prof.DefaultAppearance().LightColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasDarkColorSchemeName());
VERIFY_IS_TRUE(prof.DefaultAppearance().HasLightColorSchemeName());
}
}
}
12 changes: 9 additions & 3 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,8 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(3u, settings->AllProfiles().Size());
for (const auto& profile : settings->AllProfiles())
{
VERIFY_ARE_EQUAL(L"Campbell", profile.DefaultAppearance().ColorSchemeName());
VERIFY_ARE_EQUAL(L"Campbell", profile.DefaultAppearance().DarkColorSchemeName());
VERIFY_ARE_EQUAL(L"Campbell", profile.DefaultAppearance().LightColorSchemeName());
}
}

Expand Down Expand Up @@ -959,6 +960,10 @@ namespace SettingsModelLocalTests
},
{
"name": "profile3",
"closeOnExit": "automatic"
},
{
"name": "profile4",
"closeOnExit": null
}
]
Expand All @@ -968,9 +973,10 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings->AllProfiles().GetAt(0).CloseOnExit());
VERIFY_ARE_EQUAL(CloseOnExitMode::Always, settings->AllProfiles().GetAt(1).CloseOnExit());
VERIFY_ARE_EQUAL(CloseOnExitMode::Never, settings->AllProfiles().GetAt(2).CloseOnExit());
VERIFY_ARE_EQUAL(CloseOnExitMode::Automatic, settings->AllProfiles().GetAt(3).CloseOnExit());

// Unknown modes parse as "Graceful"
VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings->AllProfiles().GetAt(3).CloseOnExit());
// Unknown modes parse as "Automatic"
VERIFY_ARE_EQUAL(CloseOnExitMode::Automatic, settings->AllProfiles().GetAt(4).CloseOnExit());
}

void DeserializationTests::TestCloseOnExitCompatibilityShim()
Expand Down
16 changes: 8 additions & 8 deletions src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ namespace SettingsModelLocalTests
"font": {
"face": "Cascadia Mono",
"size": 12,
"size": 12.0,
"weight": "normal"
},
"padding": "8, 8, 8, 8",
Expand Down Expand Up @@ -250,8 +250,8 @@ namespace SettingsModelLocalTests

// complex command with key chords
static constexpr std::string_view actionsString4A{ R"([
{ "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+c" },
{ "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+d" }
{ "command": { "action": "adjustFontSize", "delta": 1.0 }, "keys": "ctrl+c" },
{ "command": { "action": "adjustFontSize", "delta": 1.0 }, "keys": "ctrl+d" }
])" };
// GH#13323 - these can be fragile. In the past, the order these get
// re-serialized as has been not entirely stable. We don't really care
Expand All @@ -260,7 +260,7 @@ namespace SettingsModelLocalTests
// itself. Feel free to change as needed.
static constexpr std::string_view actionsString4B{ R"([
{ "command": { "action": "findMatch", "direction": "prev" }, "keys": "ctrl+shift+r" },
{ "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+d" }
{ "command": { "action": "adjustFontSize", "delta": 1.0 }, "keys": "ctrl+d" }
])" };

// command with name and icon and multiple key chords
Expand All @@ -284,8 +284,8 @@ namespace SettingsModelLocalTests
{
"name": "Change font size...",
"commands": [
{ "command": { "action": "adjustFontSize", "delta": 1 } },
{ "command": { "action": "adjustFontSize", "delta": -1 } },
{ "command": { "action": "adjustFontSize", "delta": 1.0 } },
{ "command": { "action": "adjustFontSize", "delta": -1.0 } },
{ "command": "resetFontSize" },
]
}
Expand Down Expand Up @@ -478,7 +478,7 @@ namespace SettingsModelLocalTests
"name": "Profile with legacy font settings",
"fontFace": "Cascadia Mono",
"fontSize": 12,
"fontSize": 12.0,
"fontWeight": "normal"
})" };

Expand All @@ -488,7 +488,7 @@ namespace SettingsModelLocalTests
"font": {
"face": "Cascadia Mono",
"size": 12,
"size": 12.0,
"weight": "normal"
}
})" };
Expand Down
17 changes: 9 additions & 8 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,21 +779,22 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(6u, settings->ActiveProfiles().Size());
VERIFY_ARE_EQUAL(2u, settings->GlobalSettings().ColorSchemes().Size());

auto createTerminalSettings = [&](const auto& profile, const auto& schemes) {
auto createTerminalSettings = [&](const auto& profile, const auto& schemes, const auto& Theme) {
auto terminalSettings{ winrt::make_self<implementation::TerminalSettings>() };
terminalSettings->_ApplyProfileSettings(profile);
terminalSettings->_ApplyAppearanceSettings(profile.DefaultAppearance(), schemes);
terminalSettings->_ApplyAppearanceSettings(profile.DefaultAppearance(), schemes, Theme);
return terminalSettings;
};

const auto activeProfiles = settings->ActiveProfiles();
const auto colorSchemes = settings->GlobalSettings().ColorSchemes();
const auto terminalSettings0 = createTerminalSettings(activeProfiles.GetAt(0), colorSchemes);
const auto terminalSettings1 = createTerminalSettings(activeProfiles.GetAt(1), colorSchemes);
const auto terminalSettings2 = createTerminalSettings(activeProfiles.GetAt(2), colorSchemes);
const auto terminalSettings3 = createTerminalSettings(activeProfiles.GetAt(3), colorSchemes);
const auto terminalSettings4 = createTerminalSettings(activeProfiles.GetAt(4), colorSchemes);
const auto terminalSettings5 = createTerminalSettings(activeProfiles.GetAt(5), colorSchemes);
const auto currentTheme = settings->GlobalSettings().CurrentTheme();
const auto terminalSettings0 = createTerminalSettings(activeProfiles.GetAt(0), colorSchemes, currentTheme);
const auto terminalSettings1 = createTerminalSettings(activeProfiles.GetAt(1), colorSchemes, currentTheme);
const auto terminalSettings2 = createTerminalSettings(activeProfiles.GetAt(2), colorSchemes, currentTheme);
const auto terminalSettings3 = createTerminalSettings(activeProfiles.GetAt(3), colorSchemes, currentTheme);
const auto terminalSettings4 = createTerminalSettings(activeProfiles.GetAt(4), colorSchemes, currentTheme);
const auto terminalSettings5 = createTerminalSettings(activeProfiles.GetAt(5), colorSchemes, currentTheme);

VERIFY_ARE_EQUAL(til::color(0x12, 0x34, 0x56), terminalSettings0->CursorColor()); // from color scheme
VERIFY_ARE_EQUAL(DEFAULT_CURSOR_COLOR, terminalSettings1->CursorColor()); // default
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/LocalTests_SettingsModel/pch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Author(s):
#include <Windows.Graphics.Imaging.Interop.h>
#include <winrt/windows.ui.core.h>
#include <winrt/Windows.ui.input.h>
#include <winrt/Windows.UI.ViewManagement.h>
#include <winrt/Windows.UI.Xaml.Controls.h>
#include <winrt/Windows.UI.Xaml.Controls.Primitives.h>
#include <winrt/Windows.ui.xaml.media.h>
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/App.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ namespace winrt::TerminalApp::implementation
{
auto logic = Logic();
logic.RunAsUwp(); // Must set UWP status first, settings might change based on it.
logic.LoadSettings();
logic.ReloadSettings();
logic.Create();

auto page = logic.GetRoot().as<TerminalPage>();
Expand Down
Loading

0 comments on commit da2b80b

Please sign in to comment.