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

eframe: Automatically change theme when system dark/light mode changes #2750

Merged
merged 17 commits into from
Mar 29, 2023

Conversation

bash
Copy link
Contributor

@bash bash commented Feb 18, 2023

I noticed that eframe does not change the theme when the system dark/light mode is changed while the app is running.

For winit reacting to the change is fairly easy: There's a ThemeChanged event. It's not supported everywhere, but I guess it's better than nothing :).

For web, there's a change event on MediaQueryList.

dark_light_mode_change.mp4

@bash bash marked this pull request as ready for review February 18, 2023 15:43
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Really cool, thank you so much for working on this!

Since winit now has WindowEvent::ThemeChanged I wonder if we even need the dark-light feature. Can we detect the initial system theme with just winit? There is Window::theme but it seems to be Mac only (Windows not mentioned) and it's unclear if it reads it from system settings or just the previous call to window.set_theme.

In either case, I think we should listen for ThemeChanged regardless of whether or not the dark-light feature is enabled.

Comment on lines 435 to 436
#[cfg(feature = "dark-light")]
WindowEvent::ThemeChanged(winit_theme) if self.follow_system_theme => {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see any reason to limit this on the dark-light feature.

dark-light pulls in a specific dependency for detecting the system theme on start-up. Maybe we can do that via winit instead?

Copy link
Contributor Author

@bash bash Mar 3, 2023

Choose a reason for hiding this comment

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

I have removed #[cfg(feature = "dark-light")] from all places where it's not necessary.

Detecting the theme via winit's theme function works for Windows and macOS—I have tested the latest change on both my Windows 11 and my macOS devices.
The theme is read from the system preferences when the window is created.

The theme function always returns None on Linux though (for both X11 and Wayland), so there we still have to rely on dark-light.

The NativeOptions::system_theme function has become a bit of a liar, since it no longer corresponds with the effectively used system theme on Windows and macOS.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting! Sounds like we maybe don't need dark-light at all then? (since it already doesn't work well on Linux)

@bash bash requested a review from emilk March 12, 2023 18:42
@emilk emilk merged commit 977749b into emilk:master Mar 29, 2023
@bash bash deleted the theme-changed branch March 29, 2023 15:05
@emilk emilk added web Related to running Egui on the web eframe Relates to epi and eframe and removed web Related to running Egui on the web labels Apr 18, 2023
emilk added a commit that referenced this pull request Apr 18, 2023
Since #2750 we now get what we need
straight from `winit`.
emilk added a commit that referenced this pull request Apr 18, 2023
* Remove dark-light dependency

Since #2750 we now get what we need
straight from `winit`.

* fix warning

* Docstring formatting

* fix typo in check.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eframe Relates to epi and eframe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants