zed icon indicating copy to clipboard operation
zed copied to clipboard

Auto switching light/dark theme according to system settings

Open Yesterday17 opened this issue 1 year ago • 11 comments

Resolves #4970 Resolves #5326

Checklist

  • [x] Added themes configuration
  • [x] ~~Added an action to toggle between different theme modes~~ Removed as auto mode has been implemented
  • [x] Added appearance observer to GPUI
  • [x] Subscribe appearance change and change theme

Yesterday17 avatar Jan 27 '24 17:01 Yesterday17

We require contributors to sign our Contributor License Agreement, and we don't have @Yesterday17 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

cla-bot[bot] avatar Jan 27 '24 17:01 cla-bot[bot]

@cla-bot check

Yesterday17 avatar Jan 27 '24 17:01 Yesterday17

The cla-bot has been summoned, and re-checked this pull request!

cla-bot[bot] avatar Jan 27 '24 17:01 cla-bot[bot]

When the editor is launched in a mode that is not equal to the system appearance, it won't change themes. Only when the system appearance transition is triggered the theme is switched.

Maybe we can introduce a callback on the launch of the editor that checks for the current system appearance?

TheDarkchip avatar Jan 29 '24 18:01 TheDarkchip

When the editor is launched in a mode that is not equal to the system appearance, it won't change themes.

It seems that https://github.com/zed-industries/zed/blob/e4ae67fd6f2e3c9318ccf2770a34f25ca10f7d25/crates/workspace/src/workspace.rs#L626 is not early enough for themes to know the system appearance. Let me check how to fix it.

Yesterday17 avatar Jan 29 '24 18:01 Yesterday17

This PR is awesome! I did a few tiny touch ups and adjusted how the theme is reloaded. As soon as CI passes, I'll merge this. Great work :)

@mikayla-maki Let's hold off on merging this one until I figure out how it works into the theme stuff I'm working on now.

Want to make sure we have a cohesive narrative around theming.

maxdeviant avatar Jan 30 '24 01:01 maxdeviant

As @maxdeviant has informed me, this PR is blocked on https://github.com/zed-industries/zed/pull/7027

Shoulda looked more carefully at our PRs :D

mikayla-maki avatar Jan 30 '24 02:01 mikayla-maki

While the detection on editor startup now works, there is some trouble left with more than one system theme change.

Notably the themes.list() of the ThemeRegistry returns only one theme (One Dark). As follows the .get(name) method also fails on a differing name, resulting in no theme change, even when the system theme changes.

TheDarkchip avatar Jan 30 '24 09:01 TheDarkchip

I think I'm going to get this shipped @Yesterday17, thank you for getting the ball rolling :D

mikayla-maki avatar Feb 01 '24 17:02 mikayla-maki

I think I'm going to get this shipped @Yesterday17, thank you for getting the ball rolling :D

@mikayla-maki I was thinking it'd be nice to get the GPUI changes landed first, as those seem pretty independent.

And then I'd be down to pair on how we expose this in the settings, as I think there's some refinement we could do there.

maxdeviant avatar Feb 01 '24 17:02 maxdeviant

@maxdeviant which GPUI changes are you referring to?

mikayla-maki avatar Feb 01 '24 17:02 mikayla-maki

@Yesterday17 Thank you for your work on this!

We ended up taking a slightly different approach with #7404, but we appreciate your jumping in and getting the ball rolling on this feature 🙂

maxdeviant avatar Feb 05 '24 20:02 maxdeviant