-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Theme for settings #16479
Theme for settings #16479
Conversation
Updated with JSON entries. Update _UpdateBackgroundForMica
Hey so I spent another few minutes playing with this, and I think I realized what's wonky about mica and the settings UI. Basically, Mica will always be in the app theme, so a light themed Fortunately, the fix is actually pretty straightforward: diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp
index be390f728..ea98ee83f 100644
--- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp
+++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp
@@ -699,21 +699,23 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
}
- if (!isMicaAvailable)
- {
- return;
- }
const auto& theme = _settingsSource.GlobalSettings().CurrentTheme();
- const auto& requestedTheme = (theme.Settings() != nullptr) ? theme.Settings().RequestedTheme() : theme.RequestedTheme();
+ const bool hasThemeForSettings{ theme.Settings() != nullptr };
+ const auto& appTheme = theme.RequestedTheme();
+ const auto& requestedTheme = (hasThemeForSettings) ? theme.Settings().RequestedTheme() : appTheme;
RequestedTheme(requestedTheme);
- if (!isMicaAvailable)
- {
- return;
- }
+ // Mica gets it's appearance from the app's theme, not necessarily the
+ // Page's theme. In the case of dark app, light settings, mica will be a
+ // dark color, and the text will also be dark, making the UI _very_ hard
+ // to read. (and similarly in the inverse situation).
+ //
+ // To mitigate this, don't set the transparent background in the case
+ // that our theme is different than the app's.
+ const bool actuallyUseMica = isMicaAvailable && (appTheme == requestedTheme);
- const auto bgKey = (theme.Window() != nullptr && theme.Window().UseMica()) ?
+ const auto bgKey = (theme.Window() != nullptr && theme.Window().UseMica()) && actuallyUseMica ?
L"SettingsPageMicaBackground" :
L"SettingsPageBackground";
I think you should be able to take that delta and get this into a totally working state, and we can finally merge this in. Sorry for all the delays here. settings blob I was using {
"name": "GH#16479 Light app, no mica, light settings",
"window":
{
"applicationTheme": "light",
"useMica": false
},
"settings": { "theme": "light" }
},
{
"name": "GH#16479 Dark app, no mica, light settings",
"window":
{
"applicationTheme": "dark",
"useMica": false
},
"settings": { "theme": "light" }
},
{
"name": "GH#16479 Light app, yes mica, light settings",
"window":
{
"applicationTheme": "light",
"useMica": true
},
"settings": { "theme": "light" }
},
{
"name": "GH#16479 Dark app, yes mica, light settings",
"window":
{
"applicationTheme": "dark",
"useMica": true
},
"settings": { "theme": "light" }
},
{
"name": "GH#16479 Light app, no mica, dark settings",
"window":
{
"applicationTheme": "light",
"useMica": false
},
"settings": { "theme": "dark" }
},
{
"name": "GH#16479 Dark app, no mica, dark settings",
"window":
{
"applicationTheme": "dark",
"useMica": false
},
"settings": { "theme": "dark" }
},
{
"name": "GH#16479 Light app, yes mica, dark settings",
"window":
{
"applicationTheme": "light",
"useMica": true
},
"settings": { "theme": "dark" }
},
{
"name": "GH#16479 Dark app, yes mica, dark settings",
"window":
{
"applicationTheme": "dark",
"useMica": true
},
"settings": { "theme": "dark" }
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(as noted)
I'm just gonna push the above delta to this branch after Build and get this merged in, cause I think this is pretty straightforward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want a response to that one comment before reviewing. Also don't forget to update the...
- schema
- docs
@@ -148,6 +149,9 @@ Author(s): | |||
X(bool, RainbowFrame, "experimental.rainbowFrame", false) \ | |||
X(bool, UseMica, "useMica", false) | |||
|
|||
#define MTSM_THEME_SETTINGS_SETTINGS(X) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this macro get called anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk why ctrl+f didn't find the reference there... thanks!
Adds support for a new
settings
object in the theme settings. This includes a single property,theme
. This allows users to set a different theme from the app's requested theme, if they so choose.Closes #9231