theme: Don't log errors for missing themes until extensions have loaded (#25098)

Marshall Bowers created 10 months ago

This PR makes it so we don't log errors for missing themes or icon
themes until after the extensions have been loaded.

Currently, if you are using a theme that is defined in an extension it
is common to see one or more "theme not found" errors in the logs. This
is the result of us having to initialize the theme before the extensions
have actually finished loading.

This means that a theme that _may_ exist once extensions load is
considered non-existent before they have loaded.

To that end, we now wait until the extensions have loaded before we
start logging errors if we can't find the theme or icon theme.

Closes https://github.com/zed-industries/zed/issues/24539.

Release Notes:

- Reduced the number of "theme not found" and "icon theme not found"
errors in the logs for themes provided by extensions.

Change summary

crates/extension/src/extension_host_proxy.rs  | 10 ++++++++
crates/extension_host/src/extension_host.rs   |  1 
crates/theme/src/registry.rs                  | 13 +++++++++++
crates/theme/src/settings.rs                  | 24 ++++++++++++++------
crates/theme_extension/src/theme_extension.rs |  4 +++
5 files changed, 45 insertions(+), 7 deletions(-)

Detailed changes

crates/extension/src/extension_host_proxy.rs

@@ -96,6 +96,8 @@ impl ExtensionHostProxy {
 }
 
 pub trait ExtensionThemeProxy: Send + Sync + 'static {
+    fn set_extensions_loaded(&self);
+
     fn list_theme_names(&self, theme_path: PathBuf, fs: Arc<dyn Fs>) -> Task<Result<Vec<String>>>;
 
     fn remove_user_themes(&self, themes: Vec<SharedString>);
@@ -123,6 +125,14 @@ pub trait ExtensionThemeProxy: Send + Sync + 'static {
 }
 
 impl ExtensionThemeProxy for ExtensionHostProxy {
+    fn set_extensions_loaded(&self) {
+        let Some(proxy) = self.theme_proxy.read().clone() else {
+            return;
+        };
+
+        proxy.set_extensions_loaded()
+    }
+
     fn list_theme_names(&self, theme_path: PathBuf, fs: Arc<dyn Fs>) -> Task<Result<Vec<String>>> {
         let Some(proxy) = self.theme_proxy.read().clone() else {
             return Task::ready(Ok(Vec::new()));

crates/extension_host/src/extension_host.rs

@@ -1290,6 +1290,7 @@ impl ExtensionStore {
                 }
 
                 this.wasm_extensions.extend(wasm_extensions);
+                this.proxy.set_extensions_loaded();
                 this.proxy.reload_current_theme(cx);
                 this.proxy.reload_current_icon_theme(cx);
             })

crates/theme/src/registry.rs

@@ -50,6 +50,8 @@ impl Global for GlobalThemeRegistry {}
 struct ThemeRegistryState {
     themes: HashMap<SharedString, Arc<Theme>>,
     icon_themes: HashMap<SharedString, Arc<IconTheme>>,
+    /// Whether the extensions have been loaded yet.
+    extensions_loaded: bool,
 }
 
 /// The registry for themes.
@@ -82,6 +84,7 @@ impl ThemeRegistry {
             state: RwLock::new(ThemeRegistryState {
                 themes: HashMap::default(),
                 icon_themes: HashMap::default(),
+                extensions_loaded: false,
             }),
             assets,
         };
@@ -100,6 +103,16 @@ impl ThemeRegistry {
         registry
     }
 
+    /// Returns whether the extensions have been loaded.
+    pub fn extensions_loaded(&self) -> bool {
+        self.state.read().extensions_loaded
+    }
+
+    /// Sets the flag indicating that the extensions have loaded.
+    pub fn set_extensions_loaded(&self) {
+        self.state.write().extensions_loaded = true;
+    }
+
     fn insert_theme_families(&self, families: impl IntoIterator<Item = ThemeFamily>) {
         for family in families.into_iter() {
             self.insert_themes(family.themes);

crates/theme/src/settings.rs

@@ -157,7 +157,11 @@ 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);
-            if theme_registry.get(theme_name).ok().is_none() {
+            if let Err(err @ ThemeNotFoundError(_)) = theme_registry.get(theme_name) {
+                if theme_registry.extensions_loaded() {
+                    log::error!("{err}");
+                }
+
                 theme_name = Self::default_theme(*system_appearance);
             };
 
@@ -180,11 +184,13 @@ impl ThemeSettings {
 
             // If the selected icon theme doesn't exist, fall back to the default theme.
             let theme_registry = ThemeRegistry::global(cx);
-            if theme_registry
-                .get_icon_theme(icon_theme_name)
-                .ok()
-                .is_none()
+            if let Err(err @ IconThemeNotFoundError(_)) =
+                theme_registry.get_icon_theme(icon_theme_name)
             {
+                if theme_registry.extensions_loaded() {
+                    log::error!("{err}");
+                }
+
                 icon_theme_name = DEFAULT_ICON_THEME_NAME;
             };
 
@@ -848,7 +854,9 @@ impl settings::Settings for ThemeSettings {
                         this.active_theme = theme;
                     }
                     Err(err @ ThemeNotFoundError(_)) => {
-                        log::error!("{err}");
+                        if themes.extensions_loaded() {
+                            log::error!("{err}");
+                        }
                     }
                 }
             }
@@ -866,7 +874,9 @@ impl settings::Settings for ThemeSettings {
                         this.active_icon_theme = icon_theme;
                     }
                     Err(err @ IconThemeNotFoundError(_)) => {
-                        log::error!("{err}");
+                        if themes.extensions_loaded() {
+                            log::error!("{err}");
+                        }
                     }
                 }
             }

crates/theme_extension/src/theme_extension.rs

@@ -24,6 +24,10 @@ struct ThemeRegistryProxy {
 }
 
 impl ExtensionThemeProxy for ThemeRegistryProxy {
+    fn set_extensions_loaded(&self) {
+        self.theme_registry.set_extensions_loaded();
+    }
+
     fn list_theme_names(&self, theme_path: PathBuf, fs: Arc<dyn Fs>) -> Task<Result<Vec<String>>> {
         self.executor.spawn(async move {
             let themes = theme::read_user_theme(&theme_path, fs).await?;