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.

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

Merged
merged 19 commits into from
Jun 5, 2023

Conversation

lewiszlw
Copy link
Member

@lewiszlw lewiszlw commented May 11, 2023

Objective

Solution

  • Add preferred_theme field to Window and set it when window creation
  • Add window_theme field to InternalWindowState to store current window theme
  • Expose winit WindowThemeChanged event

@mockersf
Copy link
Member

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?

@mockersf mockersf added the A-Windowing Platform-agnostic interface layer to run your app in label May 11, 2023
@hate
Copy link
Contributor

hate commented May 11, 2023

Took a shot at adding the above to the example here: lewiszlw#1

@lewiszlw
Copy link
Member Author

Thanks @hate to update example!

Copy link
Member

@mnmaita mnmaita left a 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!

crates/bevy_window/src/event.rs Outdated Show resolved Hide resolved
crates/bevy_winit/src/converters.rs Show resolved Hide resolved
examples/window/window_settings.rs Outdated Show resolved Hide resolved
Copy link
Member

@mnmaita mnmaita left a 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.

Copy link
Contributor

@nicopap nicopap left a 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 Show resolved Hide resolved
crates/bevy_window/src/event.rs Outdated Show resolved Hide resolved
///
/// ## Platform-specific
///
/// - iOS / Android / Web / x11: Unsupported.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - 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.

Copy link
Contributor

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
}

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_winit/src/converters.rs Show resolved Hide resolved
crates/bevy_winit/src/system.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
lewiszlw and others added 5 commits May 22, 2023 10:51
Copy link
Contributor

@nicopap nicopap left a 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.

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nicopap nicopap left a 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

@nicopap nicopap added C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 30, 2023
Co-authored-by: François <mockersf@gmail.com>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 5, 2023
Merged via the queue into bevyengine:main with commit b72b154 Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OS Dark/Light theme detection
6 participants