-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Support to set window theme and expose system window theme changed event #8593
Support to set window theme and expose system window theme changed event #8593
Conversation
could you add something in example https://github.com/bevyengine/bevy/blob/main/examples/window/window_settings.rs showing how to change the theme on a key press? |
Took a shot at adding the above to the example here: lewiszlw#1 |
Toggle the window's theme in the example
Thanks @hate to update example! |
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.
Leaving a few comments and questions. Thanks a lot for this addition!
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.
Looks good to me. Let's wait for @mockersf blessing as he also reviewed.
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 think this can be merged once the few bugs, misleading doc and unused field are fixed.
crates/bevy_window/src/window.rs
Outdated
/// | ||
/// ## Platform-specific | ||
/// | ||
/// - iOS / Android / Web / x11: Unsupported. |
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.
/// - iOS / Android / Web / x11: Unsupported. | |
/// Unsupported on iOS, Android, Web and X11. The return value is always the last set value or `None` on those platforms. |
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 is a bit of an issue here.
On unsupported platforms, winit always returns None
when using window.theme()
. However, in this bevy implementation, it's possible to set self.internal.window_theme
to Some(Theme)
in set_window_theme
. Now this will return a misleading value.
I suggest conditionally returning None
here, something like
if cfg!(any(target_os = "ios", target_os = "android", target_family = "wasm", feature = "x11")) {
None
} else {
self.internal.window_theme
}
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
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 a small change request and this looks good to me.
Revert From trait implementation
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.
Great! I think this is ready to merge
Co-authored-by: François <mockersf@gmail.com>
Objective
Solution
preferred_theme
field toWindow
and set it when window creationwindow_theme
field toInternalWindowState
to store current window themeWindowThemeChanged
event