Connor Tsui created
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 ...
---------
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>crates/onboarding/src/basics_page.rs | 2
crates/theme/src/settings.rs | 57 ++++--
crates/theme/src/theme.rs | 9 +
crates/theme_selector/src/theme_selector.rs | 185 +++++++++++++++++++---
4 files changed, 203 insertions(+), 50 deletions(-)
@@ -220,7 +220,7 @@ fn render_theme_section(tab_index: &mut isize, cx: &mut App) -> impl IntoElement
});
} else {
let appearance = *SystemAppearance::global(cx);
- theme::set_theme(settings, theme, appearance);
+ theme::set_theme(settings, theme, appearance, appearance);
}
});
}
@@ -304,31 +304,50 @@ impl IconThemeSelection {
}
}
-// impl ThemeSettingsContent {
/// Sets the theme for the given appearance to the theme with the specified name.
+///
+/// The caller should make sure that the [`Appearance`] matches the theme associated with the name.
+///
+/// If the current [`ThemeAppearanceMode`] is set to [`System`] and the user's system [`Appearance`]
+/// is different than the new theme's [`Appearance`], this function will update the
+/// [`ThemeAppearanceMode`] to the new theme's appearance in order to display the new theme.
+///
+/// [`System`]: ThemeAppearanceMode::System
pub fn set_theme(
current: &mut SettingsContent,
theme_name: impl Into<Arc<str>>,
- appearance: Appearance,
+ theme_appearance: Appearance,
+ system_appearance: Appearance,
) {
- if let Some(selection) = current.theme.theme.as_mut() {
- let theme_to_update = match selection {
- settings::ThemeSelection::Static(theme) => theme,
- settings::ThemeSelection::Dynamic { mode, light, dark } => match mode {
- ThemeAppearanceMode::Light => light,
- ThemeAppearanceMode::Dark => dark,
- ThemeAppearanceMode::System => match appearance {
- Appearance::Light => light,
- Appearance::Dark => dark,
- },
- },
- };
+ let theme_name = ThemeName(theme_name.into());
- *theme_to_update = ThemeName(theme_name.into());
- } else {
- current.theme.theme = Some(settings::ThemeSelection::Static(ThemeName(
- theme_name.into(),
- )));
+ let Some(selection) = current.theme.theme.as_mut() else {
+ current.theme.theme = Some(settings::ThemeSelection::Static(theme_name));
+ return;
+ };
+
+ match selection {
+ settings::ThemeSelection::Static(theme) => {
+ *theme = theme_name;
+ }
+ settings::ThemeSelection::Dynamic { mode, light, dark } => {
+ // Update the appropriate theme slot based on appearance.
+ match theme_appearance {
+ Appearance::Light => *light = theme_name,
+ Appearance::Dark => *dark = theme_name,
+ }
+
+ // Don't update the theme mode if it is set to system and the new theme has the same
+ // appearance.
+ let should_update_mode =
+ !(mode == &ThemeAppearanceMode::System && theme_appearance == system_appearance);
+
+ if should_update_mode {
+ // Update the mode to the specified appearance (otherwise we might set the theme and
+ // nothing gets updated because the system specified the other mode appearance).
+ *mode = ThemeAppearanceMode::from(theme_appearance);
+ }
+ }
}
}
@@ -84,6 +84,15 @@ impl From<WindowAppearance> for Appearance {
}
}
+impl From<Appearance> for ThemeAppearanceMode {
+ fn from(value: Appearance) -> Self {
+ match value {
+ Appearance::Light => Self::Light,
+ Appearance::Dark => Self::Dark,
+ }
+ }
+}
+
/// Which themes should be loaded. This is used primarily for testing.
pub enum LoadThemes {
/// Only load the base theme.
@@ -9,7 +9,10 @@ use gpui::{
use picker::{Picker, PickerDelegate};
use settings::{Settings, SettingsStore, update_settings_file};
use std::sync::Arc;
-use theme::{Appearance, Theme, ThemeMeta, ThemeRegistry, ThemeSettings};
+use theme::{
+ Appearance, SystemAppearance, Theme, ThemeAppearanceMode, ThemeMeta, ThemeName, ThemeRegistry,
+ ThemeSelection, ThemeSettings,
+};
use ui::{ListItem, ListItemSpacing, prelude::*, v_flex};
use util::ResultExt;
use workspace::{ModalView, Workspace, ui::HighlightedLabel, with_active_or_new_workspace};
@@ -114,7 +117,14 @@ struct ThemeSelectorDelegate {
fs: Arc<dyn Fs>,
themes: Vec<ThemeMeta>,
matches: Vec<StringMatch>,
- original_theme: Arc<Theme>,
+ /// The theme that was selected before the `ThemeSelector` menu was opened.
+ ///
+ /// We use this to return back to theme that was set if the user dismisses the menu.
+ original_theme_settings: ThemeSettings,
+ /// The current system appearance.
+ original_system_appearance: Appearance,
+ /// The currently selected new theme.
+ new_theme: Arc<Theme>,
selection_completed: bool,
selected_theme: Option<Arc<Theme>>,
selected_index: usize,
@@ -129,6 +139,8 @@ impl ThemeSelectorDelegate {
cx: &mut Context<ThemeSelector>,
) -> Self {
let original_theme = cx.theme().clone();
+ let original_theme_settings = ThemeSettings::get_global(cx).clone();
+ let original_system_appearance = SystemAppearance::global(cx).0;
let registry = ThemeRegistry::global(cx);
let mut themes = registry
@@ -143,13 +155,15 @@ impl ThemeSelectorDelegate {
})
.collect::<Vec<_>>();
+ // Sort by dark vs light, then by name.
themes.sort_unstable_by(|a, b| {
a.appearance
.is_light()
.cmp(&b.appearance.is_light())
.then(a.name.cmp(&b.name))
});
- let matches = themes
+
+ let matches: Vec<StringMatch> = themes
.iter()
.map(|meta| StringMatch {
candidate_id: 0,
@@ -158,19 +172,25 @@ impl ThemeSelectorDelegate {
string: meta.name.to_string(),
})
.collect();
- let mut this = Self {
+
+ // The current theme is likely in this list, so default to first showing that.
+ let selected_index = matches
+ .iter()
+ .position(|mat| mat.string == original_theme.name)
+ .unwrap_or(0);
+
+ Self {
fs,
themes,
matches,
- original_theme: original_theme.clone(),
- selected_index: 0,
+ original_theme_settings,
+ original_system_appearance,
+ new_theme: original_theme, // Start with the original theme.
+ selected_index,
selection_completed: false,
selected_theme: None,
selector,
- };
-
- this.select_if_matching(&original_theme.name);
- this
+ }
}
fn show_selected_theme(
@@ -179,9 +199,10 @@ impl ThemeSelectorDelegate {
) -> Option<Arc<Theme>> {
if let Some(mat) = self.matches.get(self.selected_index) {
let registry = ThemeRegistry::global(cx);
+
match registry.get(&mat.string) {
Ok(theme) => {
- Self::set_theme(theme.clone(), cx);
+ self.set_theme(theme.clone(), cx);
Some(theme)
}
Err(error) => {
@@ -194,21 +215,122 @@ impl ThemeSelectorDelegate {
}
}
- fn select_if_matching(&mut self, theme_name: &str) {
- self.selected_index = self
- .matches
- .iter()
- .position(|mat| mat.string == theme_name)
- .unwrap_or(self.selected_index);
- }
-
- fn set_theme(theme: Arc<Theme>, cx: &mut App) {
+ fn set_theme(&mut self, new_theme: Arc<Theme>, cx: &mut App) {
+ // Update the global (in-memory) theme settings.
SettingsStore::update_global(cx, |store, _| {
- let mut theme_settings = store.get::<ThemeSettings>(None).clone();
- let name = theme.as_ref().name.clone().into();
- theme_settings.theme = theme::ThemeSelection::Static(theme::ThemeName(name));
- store.override_global(theme_settings);
+ override_global_theme(
+ store,
+ &new_theme,
+ &self.original_theme_settings.theme,
+ self.original_system_appearance,
+ )
});
+
+ self.new_theme = new_theme;
+ }
+}
+
+/// Overrides the global (in-memory) theme settings.
+///
+/// Note that this does **not** update the user's `settings.json` file (see the
+/// [`ThemeSelectorDelegate::confirm`] method and [`theme::set_theme`] function).
+fn override_global_theme(
+ store: &mut SettingsStore,
+ new_theme: &Theme,
+ original_theme: &ThemeSelection,
+ system_appearance: Appearance,
+) {
+ let theme_name = ThemeName(new_theme.name.clone().into());
+ let new_appearance = new_theme.appearance();
+ let new_theme_is_light = new_appearance.is_light();
+
+ let mut curr_theme_settings = store.get::<ThemeSettings>(None).clone();
+
+ match (original_theme, &curr_theme_settings.theme) {
+ // Override the currently selected static theme.
+ (ThemeSelection::Static(_), ThemeSelection::Static(_)) => {
+ curr_theme_settings.theme = ThemeSelection::Static(theme_name);
+ }
+
+ // If the current theme selection is dynamic, then only override the global setting for the
+ // specific mode (light or dark).
+ (
+ ThemeSelection::Dynamic {
+ mode: original_mode,
+ light: original_light,
+ dark: original_dark,
+ },
+ ThemeSelection::Dynamic { .. },
+ ) => {
+ let new_mode = update_mode_if_new_appearance_is_different_from_system(
+ original_mode,
+ system_appearance,
+ new_appearance,
+ );
+
+ let updated_theme = retain_original_opposing_theme(
+ new_theme_is_light,
+ new_mode,
+ theme_name,
+ original_light,
+ original_dark,
+ );
+
+ curr_theme_settings.theme = updated_theme;
+ }
+
+ // The theme selection mode changed while selecting new themes (someone edited the settings
+ // file on disk while we had the dialogue open), so don't do anything.
+ _ => return,
+ };
+
+ store.override_global(curr_theme_settings);
+}
+
+/// Helper function for determining the new [`ThemeAppearanceMode`] for the new theme.
+///
+/// If the the original theme mode was [`System`] and the new theme's appearance matches the system
+/// appearance, we don't need to change the mode setting.
+///
+/// Otherwise, we need to change the mode in order to see the new theme.
+///
+/// [`System`]: ThemeAppearanceMode::System
+fn update_mode_if_new_appearance_is_different_from_system(
+ original_mode: &ThemeAppearanceMode,
+ system_appearance: Appearance,
+ new_appearance: Appearance,
+) -> ThemeAppearanceMode {
+ if original_mode == &ThemeAppearanceMode::System && system_appearance == new_appearance {
+ ThemeAppearanceMode::System
+ } else {
+ ThemeAppearanceMode::from(new_appearance)
+ }
+}
+
+/// Helper function for updating / displaying the [`ThemeSelection`] while using the theme selector.
+///
+/// We want to retain the alternate theme selection of the original settings (before the menu was
+/// opened), not the currently selected theme (which likely has changed multiple times while the
+/// menu has been open).
+fn retain_original_opposing_theme(
+ new_theme_is_light: bool,
+ new_mode: ThemeAppearanceMode,
+ theme_name: ThemeName,
+ original_light: &ThemeName,
+ original_dark: &ThemeName,
+) -> ThemeSelection {
+ if new_theme_is_light {
+ ThemeSelection::Dynamic {
+ mode: new_mode,
+ light: theme_name,
+ dark: original_dark.clone(),
+ }
+ } else {
+ ThemeSelection::Dynamic {
+ mode: new_mode,
+ light: original_light.clone(),
+ dark: theme_name,
+ }
}
}
@@ -225,19 +347,20 @@ impl PickerDelegate for ThemeSelectorDelegate {
fn confirm(
&mut self,
- _: bool,
- window: &mut Window,
+ _secondary: bool,
+ _window: &mut Window,
cx: &mut Context<Picker<ThemeSelectorDelegate>>,
) {
self.selection_completed = true;
- let appearance = Appearance::from(window.appearance());
- let theme_name = ThemeSettings::get_global(cx).theme.name(appearance).0;
+ let theme_name: Arc<str> = self.new_theme.name.as_str().into();
+ let theme_appearance = self.new_theme.appearance;
+ let system_appearance = SystemAppearance::global(cx).0;
telemetry::event!("Settings Changed", setting = "theme", value = theme_name);
update_settings_file(self.fs.clone(), cx, move |settings, _| {
- theme::set_theme(settings, theme_name.to_string(), appearance);
+ theme::set_theme(settings, theme_name, theme_appearance, system_appearance);
});
self.selector
@@ -249,7 +372,9 @@ impl PickerDelegate for ThemeSelectorDelegate {
fn dismissed(&mut self, _: &mut Window, cx: &mut Context<Picker<ThemeSelectorDelegate>>) {
if !self.selection_completed {
- Self::set_theme(self.original_theme.clone(), cx);
+ SettingsStore::update_global(cx, |store, _| {
+ store.override_global(self.original_theme_settings.clone());
+ });
self.selection_completed = true;
}