-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
fix(linux): Fix Window::theme
may return incorrect theme
#800
Conversation
if let Some(settings) = Settings::default() { | ||
let theme_name = settings.gtk_theme_name().map(|s| s.as_str().to_owned()); | ||
if let Some(theme) = theme_name { | ||
if GTK_THEME_SUFFIX_LIST.iter().any(|t| theme.ends_with(t)) { |
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.
If we choose to keep a theme state, we probably don't need this constant.
Can you remove GTK_THEME_SUFFIX_LIST
's definition as well?
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.
We still need to check using GTK_THEME_SUFFIX_LIST
because a developer may not specify an explicit theme to be set so it could be either light or dark but this PR implementation assumes that it will always be light theme.
src/platform_impl/linux/window.rs
Outdated
@@ -185,10 +186,14 @@ impl Window { | |||
|
|||
let settings = Settings::default(); | |||
|
|||
let mut theme = Theme::Light; |
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.
Instead of assuming that it is always light we should:
- change the newly added
window.theme
to beexplicit_theme: Option<Theme>
- in the
Window.theme()
getter we check ifself.explicit_theme.unwrap_or_else(|| /* old logic here */)
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.
OK, that also works in my use case. Since the configuration name is preferred_theme
, I think it represents the field well. I'll update this branch.
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.
I applied the change at 1236933.
Signed-off-by: rhysd <[email protected]>
Signed-off-by: rhysd <[email protected]>
Signed-off-by: rhysd <[email protected]>
I removed two redundant clones ( |
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
Window::theme
may return incorrect theme on Linux #799Other information
On Ubuntu 22.04, the default theme name is "Yaru" in both dark mode and light mode. So it is not possible to check the theme mode by suffix like "-dark". The only way I could come up with was remembering what theme was set on building the window and returning the value at
Window::theme
. This PR adopts the idea.