Skip to content

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!

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.

///
/// ## 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
}

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.

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-Feature A new feature, making something new possible 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 enabled auto-merge June 5, 2023 20:46
@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-Feature A new feature, making something new possible 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