From af9e7f1f96b33011e3fbb865b2e55868346e2071 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Fri, 1 Nov 2024 10:19:09 -0400 Subject: [PATCH] theme: Turn `ThemeRegistry` into a trait (#20076) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR converts the `ThemeRegistry` type into a trait instead of a concrete implementation. This allows for the extension store to depend on an abstraction rather than the concrete theme registry implementation. We currently have two `ThemeRegistry` implementations: - `RealThemeRegistry` — this was previously the `ThemeRegistry` and contains the real implementation of the registry. - `VoidThemeRegistry` — a null object that doesn't have any behavior. Release Notes: - N/A --- Cargo.lock | 1 + crates/extension/src/extension_store.rs | 8 +- crates/extension/src/extension_store_test.rs | 6 +- crates/extension_cli/src/main.rs | 3 +- .../src/appearance_settings_controls.rs | 2 +- crates/storybook/src/storybook.rs | 2 +- crates/theme/Cargo.toml | 1 + crates/theme/src/registry.rs | 205 +++++++++++------- crates/theme/src/settings.rs | 8 +- crates/theme/src/theme.rs | 46 +++- crates/theme_selector/src/theme_selector.rs | 4 +- crates/zed/src/main.rs | 12 +- crates/zed/src/zed.rs | 4 +- 13 files changed, 184 insertions(+), 118 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e8a9a3d0ed..e268bd1174 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12078,6 +12078,7 @@ name = "theme" version = "0.1.0" dependencies = [ "anyhow", + "async-trait", "collections", "derive_more", "fs", diff --git a/crates/extension/src/extension_store.rs b/crates/extension/src/extension_store.rs index 0a9299a8be..f9b4523cac 100644 --- a/crates/extension/src/extension_store.rs +++ b/crates/extension/src/extension_store.rs @@ -114,7 +114,7 @@ pub struct ExtensionStore { outstanding_operations: BTreeMap, ExtensionOperation>, index_path: PathBuf, language_registry: Arc, - theme_registry: Arc, + theme_registry: Arc, slash_command_registry: Arc, indexed_docs_registry: Arc, snippet_registry: Arc, @@ -179,7 +179,7 @@ pub fn init( client: Arc, node_runtime: NodeRuntime, language_registry: Arc, - theme_registry: Arc, + theme_registry: Arc, cx: &mut AppContext, ) { ExtensionSettings::register(cx); @@ -230,7 +230,7 @@ impl ExtensionStore { telemetry: Option>, node_runtime: NodeRuntime, language_registry: Arc, - theme_registry: Arc, + theme_registry: Arc, slash_command_registry: Arc, indexed_docs_registry: Arc, snippet_registry: Arc, @@ -1358,7 +1358,7 @@ impl ExtensionStore { continue; }; - let Some(theme_family) = ThemeRegistry::read_user_theme(&theme_path, fs.clone()) + let Some(theme_family) = theme::read_user_theme(&theme_path, fs.clone()) .await .log_err() else { diff --git a/crates/extension/src/extension_store_test.rs b/crates/extension/src/extension_store_test.rs index 1274fafc3c..08dce1a98e 100644 --- a/crates/extension/src/extension_store_test.rs +++ b/crates/extension/src/extension_store_test.rs @@ -27,7 +27,7 @@ use std::{ path::{Path, PathBuf}, sync::Arc, }; -use theme::ThemeRegistry; +use theme::{RealThemeRegistry, ThemeRegistry}; use util::test::temp_tree; #[cfg(test)] @@ -260,7 +260,7 @@ async fn test_extension_store(cx: &mut TestAppContext) { }; let language_registry = Arc::new(LanguageRegistry::test(cx.executor())); - let theme_registry = Arc::new(ThemeRegistry::new(Box::new(()))); + let theme_registry = Arc::new(RealThemeRegistry::new(Box::new(()))); let slash_command_registry = SlashCommandRegistry::new(); let indexed_docs_registry = Arc::new(IndexedDocsRegistry::new(cx.executor())); let snippet_registry = Arc::new(SnippetRegistry::new()); @@ -486,7 +486,7 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { let project = Project::test(fs.clone(), [project_dir.as_path()], cx).await; let language_registry = project.read_with(cx, |project, _cx| project.languages().clone()); - let theme_registry = Arc::new(ThemeRegistry::new(Box::new(()))); + let theme_registry = Arc::new(RealThemeRegistry::new(Box::new(()))); let slash_command_registry = SlashCommandRegistry::new(); let indexed_docs_registry = Arc::new(IndexedDocsRegistry::new(cx.executor())); let snippet_registry = Arc::new(SnippetRegistry::new()); diff --git a/crates/extension_cli/src/main.rs b/crates/extension_cli/src/main.rs index dd6f221378..ffa9555c21 100644 --- a/crates/extension_cli/src/main.rs +++ b/crates/extension_cli/src/main.rs @@ -15,7 +15,6 @@ use extension::{ }; use language::LanguageConfig; use reqwest_client::ReqwestClient; -use theme::ThemeRegistry; use tree_sitter::{Language, Query, WasmStore}; #[derive(Parser, Debug)] @@ -267,7 +266,7 @@ async fn test_themes( ) -> Result<()> { for relative_theme_path in &manifest.themes { let theme_path = extension_path.join(relative_theme_path); - let theme_family = ThemeRegistry::read_user_theme(&theme_path, fs.clone()).await?; + let theme_family = theme::read_user_theme(&theme_path, fs.clone()).await?; log::info!("loaded theme family {}", theme_family.name); } diff --git a/crates/settings_ui/src/appearance_settings_controls.rs b/crates/settings_ui/src/appearance_settings_controls.rs index c7686761c4..00dd8a4c01 100644 --- a/crates/settings_ui/src/appearance_settings_controls.rs +++ b/crates/settings_ui/src/appearance_settings_controls.rs @@ -83,7 +83,7 @@ impl RenderOnce for ThemeControl { "theme", value.clone(), ContextMenu::build(cx, |mut menu, cx| { - let theme_registry = ThemeRegistry::global(cx); + let theme_registry = ::global(cx); for theme in theme_registry.list_names(false) { menu = menu.custom_entry( diff --git a/crates/storybook/src/storybook.rs b/crates/storybook/src/storybook.rs index 9fe61a70c4..d751a5ed03 100644 --- a/crates/storybook/src/storybook.rs +++ b/crates/storybook/src/storybook.rs @@ -76,7 +76,7 @@ fn main() { let selector = story_selector; - let theme_registry = ThemeRegistry::global(cx); + let theme_registry = ::global(cx); let mut theme_settings = ThemeSettings::get_global(cx).clone(); theme_settings.active_theme = theme_registry.get(&theme_name).unwrap(); ThemeSettings::override_global(theme_settings, cx); diff --git a/crates/theme/Cargo.toml b/crates/theme/Cargo.toml index c3e3a197cb..b4fe396793 100644 --- a/crates/theme/Cargo.toml +++ b/crates/theme/Cargo.toml @@ -18,6 +18,7 @@ doctest = false [dependencies] anyhow.workspace = true +async-trait.workspace = true collections.workspace = true derive_more.workspace = true fs.workspace = true diff --git a/crates/theme/src/registry.rs b/crates/theme/src/registry.rs index 73e8fe8c66..38aadf6ece 100644 --- a/crates/theme/src/registry.rs +++ b/crates/theme/src/registry.rs @@ -1,7 +1,8 @@ use std::sync::Arc; use std::{fmt::Debug, path::Path}; -use anyhow::{anyhow, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; +use async_trait::async_trait; use collections::HashMap; use derive_more::{Deref, DerefMut}; use fs::Fs; @@ -10,7 +11,9 @@ use gpui::{AppContext, AssetSource, Global, SharedString}; use parking_lot::RwLock; use util::ResultExt; -use crate::{refine_theme_family, Appearance, Theme, ThemeFamily, ThemeFamilyContent}; +use crate::{ + read_user_theme, refine_theme_family, Appearance, Theme, ThemeFamily, ThemeFamilyContent, +}; /// The metadata for a theme. #[derive(Debug, Clone)] @@ -27,22 +30,34 @@ pub struct ThemeMeta { /// inserting the [`ThemeRegistry`] into the context as a global. /// /// This should not be exposed outside of this module. -#[derive(Default, Deref, DerefMut)] -struct GlobalThemeRegistry(Arc); +#[derive(Deref, DerefMut)] +struct GlobalThemeRegistry(Arc); impl Global for GlobalThemeRegistry {} -struct ThemeRegistryState { - themes: HashMap>, +/// A registry for themes. +#[async_trait] +pub trait ThemeRegistry: Send + Sync + 'static { + /// Returns the names of all themes in the registry. + fn list_names(&self, _staff: bool) -> Vec; + + /// Returns the metadata of all themes in the registry. + fn list(&self, _staff: bool) -> Vec; + + /// Returns the theme with the given name. + fn get(&self, name: &str) -> Result>; + + /// Loads the user theme from the specified path and adds it to the registry. + async fn load_user_theme(&self, theme_path: &Path, fs: Arc) -> Result<()>; + + /// Loads the user themes from the specified directory and adds them to the registry. + async fn load_user_themes(&self, themes_path: &Path, fs: Arc) -> Result<()>; + + /// Removes the themes with the given names from the registry. + fn remove_user_themes(&self, themes_to_remove: &[SharedString]); } -/// The registry for themes. -pub struct ThemeRegistry { - state: RwLock, - assets: Box, -} - -impl ThemeRegistry { +impl dyn ThemeRegistry { /// Returns the global [`ThemeRegistry`]. pub fn global(cx: &AppContext) -> Arc { cx.global::().0.clone() @@ -52,18 +67,37 @@ impl ThemeRegistry { /// /// Inserts a default [`ThemeRegistry`] if one does not yet exist. pub fn default_global(cx: &mut AppContext) -> Arc { - cx.default_global::().0.clone() - } + if let Some(registry) = cx.try_global::() { + return registry.0.clone(); + } + let registry = Arc::new(RealThemeRegistry::default()); + cx.set_global(GlobalThemeRegistry(registry.clone())); + + registry + } +} + +struct RealThemeRegistryState { + themes: HashMap>, +} + +/// The registry for themes. +pub struct RealThemeRegistry { + state: RwLock, + assets: Box, +} + +impl RealThemeRegistry { /// Sets the global [`ThemeRegistry`]. - pub(crate) fn set_global(assets: Box, cx: &mut AppContext) { - cx.set_global(GlobalThemeRegistry(Arc::new(ThemeRegistry::new(assets)))); + pub(crate) fn set_global(self: Arc, cx: &mut AppContext) { + cx.set_global(GlobalThemeRegistry(self)); } /// Creates a new [`ThemeRegistry`] with the given [`AssetSource`]. pub fn new(assets: Box) -> Self { let registry = Self { - state: RwLock::new(ThemeRegistryState { + state: RwLock::new(RealThemeRegistryState { themes: HashMap::default(), }), assets, @@ -98,49 +132,11 @@ impl ThemeRegistry { } } - /// Removes the themes with the given names from the registry. - pub fn remove_user_themes(&self, themes_to_remove: &[SharedString]) { - self.state - .write() - .themes - .retain(|name, _| !themes_to_remove.contains(name)) - } - /// Removes all themes from the registry. pub fn clear(&self) { self.state.write().themes.clear(); } - /// Returns the names of all themes in the registry. - pub fn list_names(&self, _staff: bool) -> Vec { - let mut names = self.state.read().themes.keys().cloned().collect::>(); - names.sort(); - names - } - - /// Returns the metadata of all themes in the registry. - pub fn list(&self, _staff: bool) -> Vec { - self.state - .read() - .themes - .values() - .map(|theme| ThemeMeta { - name: theme.name.clone(), - appearance: theme.appearance(), - }) - .collect() - } - - /// Returns the theme with the given name. - pub fn get(&self, name: &str) -> Result> { - self.state - .read() - .themes - .get(name) - .ok_or_else(|| anyhow!("theme not found: {}", name)) - .cloned() - } - /// Loads the themes bundled with the Zed binary and adds them to the registry. pub fn load_bundled_themes(&self) { let theme_paths = self @@ -165,9 +161,52 @@ impl ThemeRegistry { self.insert_user_theme_families([theme_family]); } } +} - /// Loads the user themes from the specified directory and adds them to the registry. - pub async fn load_user_themes(&self, themes_path: &Path, fs: Arc) -> Result<()> { +impl Default for RealThemeRegistry { + fn default() -> Self { + Self::new(Box::new(())) + } +} + +#[async_trait] +impl ThemeRegistry for RealThemeRegistry { + fn list_names(&self, _staff: bool) -> Vec { + let mut names = self.state.read().themes.keys().cloned().collect::>(); + names.sort(); + names + } + + fn list(&self, _staff: bool) -> Vec { + self.state + .read() + .themes + .values() + .map(|theme| ThemeMeta { + name: theme.name.clone(), + appearance: theme.appearance(), + }) + .collect() + } + + fn get(&self, name: &str) -> Result> { + self.state + .read() + .themes + .get(name) + .ok_or_else(|| anyhow!("theme not found: {}", name)) + .cloned() + } + + async fn load_user_theme(&self, theme_path: &Path, fs: Arc) -> Result<()> { + let theme = read_user_theme(theme_path, fs).await?; + + self.insert_user_theme_families([theme]); + + Ok(()) + } + + async fn load_user_themes(&self, themes_path: &Path, fs: Arc) -> Result<()> { let mut theme_paths = fs .read_dir(themes_path) .await @@ -186,40 +225,38 @@ impl ThemeRegistry { Ok(()) } - /// Asynchronously reads the user theme from the specified path. - pub async fn read_user_theme(theme_path: &Path, fs: Arc) -> Result { - let reader = fs.open_sync(theme_path).await?; - let theme_family: ThemeFamilyContent = serde_json_lenient::from_reader(reader)?; + fn remove_user_themes(&self, themes_to_remove: &[SharedString]) { + self.state + .write() + .themes + .retain(|name, _| !themes_to_remove.contains(name)) + } +} - for theme in &theme_family.themes { - if theme - .style - .colors - .deprecated_scrollbar_thumb_background - .is_some() - { - log::warn!( - r#"Theme "{theme_name}" is using a deprecated style property: scrollbar_thumb.background. Use `scrollbar.thumb.background` instead."#, - theme_name = theme.name - ) - } - } +/// A theme registry that doesn't have any behavior. +pub struct VoidThemeRegistry; - Ok(theme_family) +#[async_trait] +impl ThemeRegistry for VoidThemeRegistry { + fn list_names(&self, _staff: bool) -> Vec { + Vec::new() } - /// Loads the user theme from the specified path and adds it to the registry. - pub async fn load_user_theme(&self, theme_path: &Path, fs: Arc) -> Result<()> { - let theme = Self::read_user_theme(theme_path, fs).await?; + fn list(&self, _staff: bool) -> Vec { + Vec::new() + } - self.insert_user_theme_families([theme]); + fn get(&self, name: &str) -> Result> { + bail!("cannot retrieve theme {name:?} from a void theme registry") + } + async fn load_user_theme(&self, _theme_path: &Path, _fs: Arc) -> Result<()> { Ok(()) } -} -impl Default for ThemeRegistry { - fn default() -> Self { - Self::new(Box::new(())) + async fn load_user_themes(&self, _themes_path: &Path, _fs: Arc) -> Result<()> { + Ok(()) } + + fn remove_user_themes(&self, _themes_to_remove: &[SharedString]) {} } diff --git a/crates/theme/src/settings.rs b/crates/theme/src/settings.rs index 4d8158388c..fdae092c22 100644 --- a/crates/theme/src/settings.rs +++ b/crates/theme/src/settings.rs @@ -150,7 +150,7 @@ impl ThemeSettings { // If the selected theme doesn't exist, fall back to a default theme // based on the system appearance. - let theme_registry = ThemeRegistry::global(cx); + let theme_registry = ::global(cx); if theme_registry.get(theme_name).ok().is_none() { theme_name = Self::default_theme(*system_appearance); }; @@ -446,7 +446,7 @@ impl ThemeSettings { /// Returns a `Some` containing the new theme if it was successful. /// Returns `None` otherwise. pub fn switch_theme(&mut self, theme: &str, cx: &mut AppContext) -> Option> { - let themes = ThemeRegistry::default_global(cx); + let themes = ::default_global(cx); let mut new_theme = None; @@ -598,7 +598,7 @@ impl settings::Settings for ThemeSettings { type FileContent = ThemeSettingsContent; fn load(sources: SettingsSources, cx: &mut AppContext) -> Result { - let themes = ThemeRegistry::default_global(cx); + let themes = ::default_global(cx); let system_appearance = SystemAppearance::default_global(cx); let defaults = sources.default; @@ -710,7 +710,7 @@ impl settings::Settings for ThemeSettings { cx: &AppContext, ) -> schemars::schema::RootSchema { let mut root_schema = generator.root_schema_for::(); - let theme_names = ThemeRegistry::global(cx) + let theme_names = ::global(cx) .list_names(params.staff_mode) .into_iter() .map(|theme_name| Value::String(theme_name.to_string())) diff --git a/crates/theme/src/theme.rs b/crates/theme/src/theme.rs index 307ea6b287..103ad68486 100644 --- a/crates/theme/src/theme.rs +++ b/crates/theme/src/theme.rs @@ -17,17 +17,12 @@ mod schema; mod settings; mod styles; +use std::path::Path; use std::sync::Arc; use ::settings::{Settings, SettingsStore}; -pub use default_colors::*; -pub use font_family_cache::*; -pub use registry::*; -pub use scale::*; -pub use schema::*; -pub use settings::*; -pub use styles::*; - +use anyhow::Result; +use fs::Fs; use gpui::{ px, AppContext, AssetSource, HighlightStyle, Hsla, Pixels, Refineable, SharedString, WindowAppearance, WindowBackgroundAppearance, @@ -35,6 +30,14 @@ use gpui::{ use serde::Deserialize; use uuid::Uuid; +pub use crate::default_colors::*; +pub use crate::font_family_cache::*; +pub use crate::registry::*; +pub use crate::scale::*; +pub use crate::schema::*; +pub use crate::settings::*; +pub use crate::styles::*; + /// Defines window border radius for platforms that use client side decorations. pub const CLIENT_SIDE_DECORATION_ROUNDING: Pixels = px(10.0); /// Defines window shadow size for platforms that use client side decorations. @@ -85,10 +88,11 @@ pub fn init(themes_to_load: LoadThemes, cx: &mut AppContext) { LoadThemes::JustBase => (Box::new(()) as Box, false), LoadThemes::All(assets) => (assets, true), }; - ThemeRegistry::set_global(assets, cx); + let registry = Arc::new(RealThemeRegistry::new(assets)); + registry.clone().set_global(cx); if load_user_themes { - ThemeRegistry::global(cx).load_bundled_themes(); + registry.load_bundled_themes(); } ThemeSettings::register(cx); @@ -321,3 +325,25 @@ pub fn color_alpha(color: Hsla, alpha: f32) -> Hsla { color.a = alpha; color } + +/// Asynchronously reads the user theme from the specified path. +pub async fn read_user_theme(theme_path: &Path, fs: Arc) -> Result { + let reader = fs.open_sync(theme_path).await?; + let theme_family: ThemeFamilyContent = serde_json_lenient::from_reader(reader)?; + + for theme in &theme_family.themes { + if theme + .style + .colors + .deprecated_scrollbar_thumb_background + .is_some() + { + log::warn!( + r#"Theme "{theme_name}" is using a deprecated style property: scrollbar_thumb.background. Use `scrollbar.thumb.background` instead."#, + theme_name = theme.name + ) + } + } + + Ok(theme_family) +} diff --git a/crates/theme_selector/src/theme_selector.rs b/crates/theme_selector/src/theme_selector.rs index 2ce2b9ba89..47750c0ea0 100644 --- a/crates/theme_selector/src/theme_selector.rs +++ b/crates/theme_selector/src/theme_selector.rs @@ -97,7 +97,7 @@ impl ThemeSelectorDelegate { let original_theme = cx.theme().clone(); let staff_mode = cx.is_staff(); - let registry = ThemeRegistry::global(cx); + let registry = ::global(cx); let mut themes = registry .list(staff_mode) .into_iter() @@ -142,7 +142,7 @@ impl ThemeSelectorDelegate { fn show_selected_theme(&mut self, cx: &mut ViewContext>) { if let Some(mat) = self.matches.get(self.selected_index) { - let registry = ThemeRegistry::global(cx); + let registry = ::global(cx); match registry.get(&mat.string) { Ok(theme) => { Self::set_theme(theme, cx); diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index f366323ff5..688e217fb4 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -407,7 +407,7 @@ fn main() { app_state.client.clone(), app_state.node_runtime.clone(), app_state.languages.clone(), - ThemeRegistry::global(cx), + ::global(cx), cx, ); recent_projects::init(cx); @@ -1160,8 +1160,9 @@ fn load_user_themes_in_background(fs: Arc, cx: &mut AppContext) { cx.spawn({ let fs = fs.clone(); |cx| async move { - if let Some(theme_registry) = - cx.update(|cx| ThemeRegistry::global(cx).clone()).log_err() + if let Some(theme_registry) = cx + .update(|cx| ::global(cx).clone()) + .log_err() { let themes_dir = paths::themes_dir().as_ref(); match fs @@ -1200,8 +1201,9 @@ fn watch_themes(fs: Arc, cx: &mut AppContext) { while let Some(paths) = events.next().await { for event in paths { if fs.metadata(&event.path).await.ok().flatten().is_some() { - if let Some(theme_registry) = - cx.update(|cx| ThemeRegistry::global(cx).clone()).log_err() + if let Some(theme_registry) = cx + .update(|cx| ::global(cx).clone()) + .log_err() { if let Some(()) = theme_registry .load_user_theme(&event.path, fs.clone()) diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index bbe24bdaaf..c3d23572e2 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -1139,7 +1139,7 @@ mod tests { path::{Path, PathBuf}, time::Duration, }; - use theme::{ThemeRegistry, ThemeSettings}; + use theme::{RealThemeRegistry, ThemeRegistry, ThemeSettings}; use workspace::{ item::{Item, ItemHandle}, open_new, open_paths, pane, NewFile, OpenVisible, SaveIntent, SplitDirection, @@ -3419,7 +3419,7 @@ mod tests { .unwrap(), ]) .unwrap(); - let themes = ThemeRegistry::default(); + let themes = RealThemeRegistry::default(); settings::init(cx); theme::init(theme::LoadThemes::JustBase, cx);