From 8e7cad78488da6a436cc80b9822fdcaf9c177542 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Tue, 18 Feb 2025 10:49:55 -0500 Subject: [PATCH] theme: Return structured errors when a theme is not found (#25090) This PR updates the `ThemeRegistry` to return structured errors from the `get` and `get_icon_theme` methods (which are used to retrieve themes and icon themes, respectively). We want to be able to carry the name of the theme that was not found as state on the error, which is why we use a `Result` and not an `Option`. However, we also want to be able to accurately identify when the error case is "not found" so we can take appropriate action, based on the circumstances. By using a custom error type instead of an `anyhow::Error`, we get both. There isn't any functional change in this PR. This just sets us up for future improvements in this error. Release Notes: - N/A --- Cargo.lock | 1 + crates/theme/Cargo.toml | 1 + crates/theme/src/registry.rs | 23 +++++++++++++++++------ crates/theme/src/settings.rs | 33 ++++++++++++++++++++++++--------- 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7d504f2e481123932b18636cbe2b06dd34ef3548..52b017b87656251d26e89bc0ba87a77f86e67916 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13481,6 +13481,7 @@ dependencies = [ "serde_repr", "settings", "strum", + "thiserror 1.0.69", "util", "uuid", ] diff --git a/crates/theme/Cargo.toml b/crates/theme/Cargo.toml index e0a811335a62e4819e70623452f21202565876d0..05535eae8827fb56cef99e49bdab537b3ca84300 100644 --- a/crates/theme/Cargo.toml +++ b/crates/theme/Cargo.toml @@ -36,6 +36,7 @@ serde_json_lenient.workspace = true serde_repr.workspace = true settings.workspace = true strum.workspace = true +thiserror.workspace = true util.workspace = true uuid.workspace = true diff --git a/crates/theme/src/registry.rs b/crates/theme/src/registry.rs index f90b6df559a299402d0a5463daa5ed8705412be5..ed1d232cf5bf9b0c19cce2d45e4dcecccf212470 100644 --- a/crates/theme/src/registry.rs +++ b/crates/theme/src/registry.rs @@ -1,13 +1,14 @@ use std::sync::Arc; use std::{fmt::Debug, path::Path}; -use anyhow::{anyhow, Context as _, Result}; +use anyhow::{Context as _, Result}; use collections::HashMap; use derive_more::{Deref, DerefMut}; use fs::Fs; use futures::StreamExt; use gpui::{App, AssetSource, Global, SharedString}; use parking_lot::RwLock; +use thiserror::Error; use util::ResultExt; use crate::{ @@ -25,6 +26,16 @@ pub struct ThemeMeta { pub appearance: Appearance, } +/// An error indicating that the theme with the given name was not found. +#[derive(Debug, Error, Clone)] +#[error("theme not found: {0}")] +pub struct ThemeNotFoundError(pub SharedString); + +/// An error indicating that the icon theme with the given name was not found. +#[derive(Debug, Error, Clone)] +#[error("icon theme not found: {0}")] +pub struct IconThemeNotFoundError(pub SharedString); + /// The global [`ThemeRegistry`]. /// /// This newtype exists for obtaining a unique [`TypeId`](std::any::TypeId) when @@ -145,12 +156,12 @@ impl ThemeRegistry { } /// Returns the theme with the given name. - pub fn get(&self, name: &str) -> Result> { + pub fn get(&self, name: &str) -> Result, ThemeNotFoundError> { self.state .read() .themes .get(name) - .ok_or_else(|| anyhow!("theme not found: {}", name)) + .ok_or_else(|| ThemeNotFoundError(name.to_string().into())) .cloned() } @@ -209,7 +220,7 @@ impl ThemeRegistry { } /// Returns the default icon theme. - pub fn default_icon_theme(&self) -> Result> { + pub fn default_icon_theme(&self) -> Result, IconThemeNotFoundError> { self.get_icon_theme(DEFAULT_ICON_THEME_NAME) } @@ -227,12 +238,12 @@ impl ThemeRegistry { } /// Returns the icon theme with the specified name. - pub fn get_icon_theme(&self, name: &str) -> Result> { + pub fn get_icon_theme(&self, name: &str) -> Result, IconThemeNotFoundError> { self.state .read() .icon_themes .get(name) - .ok_or_else(|| anyhow!("icon theme not found: {name}")) + .ok_or_else(|| IconThemeNotFoundError(name.to_string().into())) .cloned() } diff --git a/crates/theme/src/settings.rs b/crates/theme/src/settings.rs index baa2130d8560f84fa43f59c2fdec814163172dcb..70e271a108a3d3e42176556dd58039509dbb188f 100644 --- a/crates/theme/src/settings.rs +++ b/crates/theme/src/settings.rs @@ -1,7 +1,7 @@ use crate::fallback_themes::zed_default_dark; use crate::{ - Appearance, IconTheme, SyntaxTheme, Theme, ThemeRegistry, ThemeStyleContent, - DEFAULT_ICON_THEME_NAME, + Appearance, IconTheme, IconThemeNotFoundError, SyntaxTheme, Theme, ThemeNotFoundError, + ThemeRegistry, ThemeStyleContent, DEFAULT_ICON_THEME_NAME, }; use anyhow::Result; use derive_more::{Deref, DerefMut}; @@ -578,9 +578,14 @@ impl ThemeSettings { let mut new_theme = None; - if let Some(theme) = themes.get(theme).log_err() { - self.active_theme = theme.clone(); - new_theme = Some(theme); + match themes.get(theme) { + Ok(theme) => { + self.active_theme = theme.clone(); + new_theme = Some(theme); + } + Err(err @ ThemeNotFoundError(_)) => { + log::error!("{err}"); + } } self.apply_theme_overrides(); @@ -838,8 +843,13 @@ impl settings::Settings for ThemeSettings { let theme_name = value.theme(*system_appearance); - if let Some(theme) = themes.get(theme_name).log_err() { - this.active_theme = theme; + match themes.get(theme_name) { + Ok(theme) => { + this.active_theme = theme; + } + Err(err @ ThemeNotFoundError(_)) => { + log::error!("{err}"); + } } } @@ -851,8 +861,13 @@ impl settings::Settings for ThemeSettings { let icon_theme_name = value.icon_theme(*system_appearance); - if let Some(icon_theme) = themes.get_icon_theme(icon_theme_name).log_err() { - this.active_icon_theme = icon_theme; + match themes.get_icon_theme(icon_theme_name) { + Ok(icon_theme) => { + this.active_icon_theme = icon_theme; + } + Err(err @ IconThemeNotFoundError(_)) => { + log::error!("{err}"); + } } }