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

Detect light/dark OS theme #150

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

KeKsBoTer
Copy link

Hey,

I implemented basic light/dark theme detection for macOS.
I am new to the project and would appreciate your input.

Location in Code

At this point in time I am just reading the OS theme as the default value for the Theme Enum.
This seems a bit sketchy to me but I am not sure where else to put it.

Reactive Implementation

My approach only reads the OS theme once (at the application start) and does not react to theme changes during runtime.
This project could serve as a basic implementation for a reactive implementation.
Since a reactive implementation is much more complicated I would strive for a basic approach at this point in time.

Error Handling

I was unable to find a suitable error handling approach for my implementation.
Neither panic::catch_unwind nor reading the OS value in a separate thread prevents the program from crashing when reading the OS theme fails ("rust cannot catch foreign exceptions").
This is not optimal since an theme reading error should not result in the program crashing.
Do you have any idea how to fix this problem?

@Kethku
Copy link
Contributor

Kethku commented Aug 26, 2021

I think this should be implemented upstream in druid. This is a general feature that should be handled and exposed by the druid-shell component rather than individual apps themselves

@jpochyla
Copy link
Owner

jpochyla commented Aug 29, 2021

Thanks a lot for the PR @KeKsBoTer! I also think this would be best suited for druid-shell, do you feel like opening a PR over there? If not, I can do it. winit also have some support for light/dark theme detection, although only on Windows, IIRC.

@KeKsBoTer
Copy link
Author

Hey,

thanks for the feedback!
I agree with your suggestion of implementing it in druid-shell.
My plan is to implement it there and open the PR. I just don't have time for it before next week.
Should I close the pull request here for now?

@jpochyla
Copy link
Owner

Thanks! I think we can leave this open for now.

@jpochyla
Copy link
Owner

jpochyla commented Sep 1, 2021

Looks like #162 is a bit further with this, through using 3rd party crates for the detection? I haven't investigated it much, yet.

@edfloreshz
Copy link

edfloreshz commented Sep 4, 2021

I'm making some changes to that 3rd party crate to also support Linux, I'm using my own forks for now until my changes are merged upstream. I also thought that druid should implement dark mode detection and not the apps itself.

We could try to implement this functionality in @jpochyla's fork of druid in case that they aren't already working on that.

Let me know if i should stop working on it until we make changes to druid.

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.

4 participants