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

Add Theme Switch Widget #4805

Closed
wants to merge 11 commits into from
Closed

Add Theme Switch Widget #4805

wants to merge 11 commits into from

Conversation

bash
Copy link
Contributor

@bash bash commented Jul 8, 2024

I got a juust a bit sidetracked while working on #4490 because I realized that the global dark/light mode toggle would be missing a "follow system" option.

My solution is a custom switch widget with three options: follow system, dark and light.

Screencast from 2024-07-08 21-46-32

(follow system is disabled for now but is supported by the widget)

The icons are hand-drawn because:

  1. I couldn't for the life of me figure out how to properly center the icons from the icon font.
  2. The sun, moon and gear icons in the built-in icon font don't look tooo nice on small sizes.
  3. I can smoothly animate them between different sizes
  4. Drawing the icons by hand is fun?

I tested the widget on my Mac with VoiceOver on to ensure that the widget is announced as Theme, radio group and each radio button is announced as <label>, <n> out of 3.

The widget and the accompanying ThemePreference type are both pub(crate) for now, but I intend to make them public in #4744.

  • I have followed the instructions in the PR template

@bash bash marked this pull request as ready for review July 8, 2024 19:57
@bash
Copy link
Contributor Author

bash commented Jul 10, 2024

I forgot to mention that I'd be happy to split this up into smaller PRs if needed :)

Some individual parts that could be extracted:

  • Return value from with_accessibility_parent
  • arc drawing could be moved to epaint and exposed as public API
  • general shape rotation would remove the need for my rotated_rect workaround.

@AurevoirXavier
Copy link
Contributor

Just curious, how did you obtain the current system theme in egui?

@bash
Copy link
Contributor Author

bash commented Jul 13, 2024

@AurevoirXavier egui itself currently doesn't know about the system theme. It relies on eframe (or your integration of choice) to set the Visuals to match the system theme (controlled by follow_system_theme).

The way that the switch widget currently recognizes dark vs. light mode is by checking Visuals.dark_mode.

If you want to know the system theme and are using eframe then you can get system_theme via frame.info().

@emilk
Copy link
Owner

emilk commented Jul 15, 2024

This is a very pretty theme selector, but I worry it is a bit overkill for the default one in egui for several reasons:

A) it adds a lot of code to maintain
B) it adds a lot of code => increase .wasm size
C) the style of the buttons don't fit in with the rest of egui's default theme (they are too pretty)

I suggest you create this as a 3rd party crate instead, and for egui make a much simpler drop-down menu.

@bash
Copy link
Contributor Author

bash commented Jul 15, 2024

Thank you for the kind words ☺️

I totally understand the reasoning behind not wanting this in egui itself. Time for egui-theme-switch 😀

Would you be interested in upstreaming some of the following things from this PR (as individual PRs of course):

  • Add a return value to with_accessibility_parent
  • WidgetType::ButtonGroup
  • Arc drawing

@AurevoirXavier
Copy link
Contributor

AurevoirXavier commented Jul 15, 2024

This is a very pretty theme selector, but I worry it is a bit overkill for the default one in egui for several reasons:

A) it adds a lot of code to maintain B) it adds a lot of code => increase .wasm size C) the style of the buttons don't fit in with the rest of egui's default theme (they are too pretty)

I suggest you create this as a 3rd party crate instead, and for egui make a much simpler drop-down menu.

I agree with this. To be honest, I ask that question because I want one for my app, but in a combobox. This one feels a bit overkill for my app. (just as @emilk mentioned, it's too pretty and will look out of place in my poor UI)

@bash
Copy link
Contributor Author

bash commented Sep 6, 2024

I'm closing this as I have published a crate for this and extracted the "salvageable" parts into two separate PRs.

@bash bash closed this Sep 6, 2024
emilk pushed a commit that referenced this pull request Sep 9, 2024
Extracted out of #4805

I'm using this widget type in [`egui-theme-switch`] but since it's not
built in I have to call `accesskit_node_builder` which is a bit
cumbersome :)

* [x] I have followed the instructions in the PR template

[`egui-theme-switch`]:
https://github.com/bash/egui-theme-switch/blob/main/src/lib.rs
emilk pushed a commit that referenced this pull request Sep 9, 2024
Extracted out of #4805

In [`egui-theme-switch`] I'm allocating a response inside the closure
passed to `with_accessibility_parent` so that my radio buttons have the
radio group as parent.

I'm working around the lack of return value with a custom extension
trait for now: [`ContextExt`]

* [x] I have followed the instructions in the PR template

[`egui-theme-switch`]:
https://github.com/bash/egui-theme-switch/blob/main/src/lib.rs
[`ContextExt`]:
https://github.com/bash/egui-theme-switch/blob/main/src/context_ext.rs
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
Extracted out of emilk#4805

I'm using this widget type in [`egui-theme-switch`] but since it's not
built in I have to call `accesskit_node_builder` which is a bit
cumbersome :)

* [x] I have followed the instructions in the PR template

[`egui-theme-switch`]:
https://github.com/bash/egui-theme-switch/blob/main/src/lib.rs
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
Extracted out of emilk#4805

In [`egui-theme-switch`] I'm allocating a response inside the closure
passed to `with_accessibility_parent` so that my radio buttons have the
radio group as parent.

I'm working around the lack of return value with a custom extension
trait for now: [`ContextExt`]

* [x] I have followed the instructions in the PR template

[`egui-theme-switch`]:
https://github.com/bash/egui-theme-switch/blob/main/src/lib.rs
[`ContextExt`]:
https://github.com/bash/egui-theme-switch/blob/main/src/context_ext.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants