zed-industries
Allow dynamic `set_theme` based on `Appearance`
Tracking Issue (does not close): https://github.com/zed-industries/zed/issues/35552
This is somewhat of a blocker for https://github.com/zed-industries/zed/pull/40035 (but also the current behavior doesn't really make sense).
The current behavior of ThemeSelectorDelegate::set_theme (the theme selector menu) is to simply set the in-memory settings to Static, regardless of if it is currently Dynamic. The reason this doesn't matter now is that the theme::set_theme function that updates the user's settings file will make this check, so dynamic settings stay dynamic in settings.json, but not in memory.
But this is also sort of strange, because theme::set_theme will set the setting of whatever the old appearance was to the new theme name. In other words, if I am currently on a light mode theme and I change my theme to a dark mode theme using the theme selector, the light field of theme in settings.json is set to a dark mode theme!
I think this is because displaying the new theme in the theme selector does not update the global context, so ThemeSettings::get_global(cx).theme.name(appearance).0 returns the original theme appearance, not the new one.
This PR makes ThemeSelectorDelegate::set_theme keep the current ThemeSelection, as well as changes the behavior of the theme::set_theme call to always choose the correct setting to update.
One edge case that might be slightly strange now is that if the user has specified the mode as System, this will now override that with the appearance of the new theme. I think this is fine, as otherwise a user might set a dark theme and nothing will change because the ThemeAppearanceMode is set to light or system (where system is also light).
I also have an unreachable! in there that I'm pretty sure is true but I don't really know how to formally prove that...
Release Notes:
- N/A or Added/Fixed/Improved ...
| Messages | |
|---|---|
| :book: |
This PR includes links to the following GitHub Issues: #35552
If this PR aims to close an issue, please include a |
Generated by :no_entry_sign: dangerJS against 579c388387cd1f3edff3a2ac15bab7a9a0d4b460
Nice, thanks!
I think the unreachable! can be reached if you edit the settings file on disk while the dialogue is open; so we can just return in that case instead of crashing.
@ConradIrwin Apologies, usually I get to these kinds of things on the weekends.
Though I also have a question of if it is worth adding functionality to check if the system setting is the same as the selected theme, and in that case we don't update "system" to "dark" for example (let me know if that doesn't make sense).
Sorry for the early ping, I usually wait at least a week but was going too fast.
I also spent some time thinking about that; I think there's a big mismatch between what we want the picker to do and what it actually does. For now I think this is fine, and I think it's better that selecting a theme makes it "always" the theme (as you already implemented).
There is a bigger problem here though, which is that the picker doesn't match up with the data model in the settings file.
It seems like the primary workflow should be:
- Select which theme-pair you want (that comes with a light and dark mode). We'd need to update our theme schema to allow associating two themes together as a pair, which seems worth doing (but probably out of scope for this change).
The secondary workflow would then be:
- Select which specific theme you want to use for light mode or dark mode. We could do this using the UI we have today if we added something in the footer that lets you choose whether you wanted your choice to apply in light mode, dark mode, or always. (That could maybe be scope-creeped into this PR if you wanted, but no need). but cc @danilo-leal for ideas on making it feel good.
@ConradIrwin Let me know if the change above makes sense for the scope of this PR!
Heya, just circling back to this — agree with adding a button in the theme picker footer to choose between these different options. I sort of think the default behavior of the picker should be to set the theme that will always be displayed, and have the ability to set different themes for different modes as a secondary action, where you'd need to opt into that and then be able to choose a theme for each mode. If you both agree with that and are down to set up the actions to do that, I can work on the UI from within the picker!
@connortsui20 I am happy with either:
- (preferred) tidy up this branch to remove the unreachable, merge it, and send UI improvements as a new PR
- (also ok) close this and work more directly on the target version.
@danilo-leal thanks for the offer on making the UI feel good!
@ConradIrwin I believe I have tidied things up? Let me know if there's anything else that needs cleaning up!