fix(themes): Restore default theme, refactor (#2294)

* fix(theme): let the base colour remain unchanged

* fix(theme): split out default

* fix(theme): make base theme 'default' not an empty string

* wip(theme): return styles, not colors

* wip(theme): tidy up module structure a little

* wip(theme): removed unhandled references to foreground_color

* chore: fix cargo fmt

* feat(theme): allow crossterm-deserializable colors

Cargo.lock

@@ -1001,6 +1001,7 @@ dependencies = [
  "libc",
  "mio",
  "parking_lot",
+ "serde",
  "signal-hook",
  "signal-hook-mio",
  "winapi",

crates/atuin-client/Cargo.toml

@@ -71,7 +71,7 @@ indicatif = "0.17.7"
 tiny-bip39 = "1"
 
 # theme
-crossterm = "0.27.0"
+crossterm = { version = "0.27.0", features = ["serde"] }
 palette = { version = "0.7.5", features = ["serializing"] }
 lazy_static = "1.4.0"
 strum_macros = "0.26.3"

crates/atuin-client/config.toml

@@ -234,7 +234,7 @@ records = true
 
 # [theme]
 ## Color theme to use for rendering in the terminal.
-## There are some built-in themes, including the base theme which has the default colors,
+## There are some built-in themes, including the base theme ("default"),
 ## "autumn" and "marine". You can add your own themes to the "./themes" subdirectory of your
 ## Atuin config (or ATUIN_THEME_DIR, if provided) as TOML files whose keys should be one or
 ## more of AlertInfo, AlertWarn, AlertError, Annotation, Base, Guidance, Important, and

crates/atuin-client/src/settings.rs

@@ -340,7 +340,7 @@ pub struct Preview {
 
 #[derive(Clone, Debug, Deserialize, Serialize)]
 pub struct Theme {
-    /// Name of desired theme ("" for base)
+    /// Name of desired theme ("default" for base)
     pub name: String,
 
     /// Whether any available additional theme debug should be shown
@@ -756,7 +756,7 @@ impl Settings {
             .set_default("daemon.socket_path", socket_path.to_str())?
             .set_default("daemon.systemd_socket", false)?
             .set_default("daemon.tcp_port", 8889)?
-            .set_default("theme.name", "")?
+            .set_default("theme.name", "default")?
             .set_default("theme.debug", None::<bool>)?
             .set_default(
                 "prefers_reduced_motion",

crates/atuin-client/src/theme.rs

@@ -3,6 +3,7 @@ use lazy_static::lazy_static;
 use log;
 use palette::named;
 use serde::{Deserialize, Serialize};
+use serde_json;
 use std::collections::HashMap;
 use std::error;
 use std::io::{Error, ErrorKind};
@@ -29,6 +30,7 @@ pub enum Meaning {
     Guidance,
     Important,
     Title,
+    Muted,
 }
 
 #[derive(Clone, Debug, Deserialize, Serialize)]
@@ -42,7 +44,7 @@ pub struct ThemeConfig {
 
 #[derive(Clone, Debug, Deserialize, Serialize)]
 pub struct ThemeDefinitionConfigBlock {
-    /// Name of theme ("" for base)
+    /// Name of theme ("default" for base)
     pub name: String,
 
     /// Whether any theme should be treated as a parent _if available_
@@ -56,7 +58,7 @@ use crossterm::style::{Color, ContentStyle};
 pub struct Theme {
     pub name: String,
     pub parent: Option<String>,
-    pub colors: HashMap<Meaning, Color>,
+    pub styles: HashMap<Meaning, ContentStyle>,
 }
 
 // Themes have a number of convenience functions for the most commonly used meanings.
@@ -64,38 +66,42 @@ pub struct Theme {
 // theme-related boilerplate minimal, the convenience functions give a color.
 impl Theme {
     // This is the base "default" color, for general text
-    pub fn get_base(&self) -> Color {
+    pub fn get_base(&self) -> ContentStyle {
-        self.colors[&Meaning::Base]
+        self.styles[&Meaning::Base]
     }
 
-    pub fn get_info(&self) -> Color {
+    pub fn get_info(&self) -> ContentStyle {
         self.get_alert(log::Level::Info)
     }
 
-    pub fn get_warning(&self) -> Color {
+    pub fn get_warning(&self) -> ContentStyle {
         self.get_alert(log::Level::Warn)
     }
 
-    pub fn get_error(&self) -> Color {
+    pub fn get_error(&self) -> ContentStyle {
         self.get_alert(log::Level::Error)
     }
 
     // The alert meanings may be chosen by the Level enum, rather than the methods above
     // or the full Meaning enum, to simplify programmatic selection of a log-level.
-    pub fn get_alert(&self, severity: log::Level) -> Color {
+    pub fn get_alert(&self, severity: log::Level) -> ContentStyle {
-        self.colors[ALERT_TYPES.get(&severity).unwrap()]
+        self.styles[ALERT_TYPES.get(&severity).unwrap()]
     }
 
-    pub fn new(name: String, parent: Option<String>, colors: HashMap<Meaning, Color>) -> Theme {
+    pub fn new(
+        name: String,
+        parent: Option<String>,
+        styles: HashMap<Meaning, ContentStyle>,
+    ) -> Theme {
         Theme {
             name,
             parent,
-            colors,
+            styles,
         }
     }
 
     pub fn closest_meaning<'a>(&self, meaning: &'a Meaning) -> &'a Meaning {
-        if self.colors.contains_key(meaning) {
+        if self.styles.contains_key(meaning) {
             meaning
         } else if MEANING_FALLBACKS.contains_key(meaning) {
             self.closest_meaning(&MEANING_FALLBACKS[meaning])
@@ -106,10 +112,7 @@ impl Theme {
 
     // General access - if you have a meaning, this will give you a (crossterm) style
     pub fn as_style(&self, meaning: Meaning) -> ContentStyle {
-        ContentStyle {
+        self.styles[self.closest_meaning(&meaning)]
-            foreground_color: Some(self.colors[self.closest_meaning(&meaning)]),
-            ..ContentStyle::default()
-        }
     }
 
     // Turns a map of meanings to colornames into a theme
@@ -117,27 +120,52 @@ impl Theme {
     // but we do not have this on in general, as it could print unfiltered text to the terminal
     // from a theme TOML file. However, it will always return a theme, falling back to
     // defaults on error, so that a TOML file does not break loading
-    pub fn from_map(
+    pub fn from_foreground_colors(
         name: String,
         parent: Option<&Theme>,
-        colors: HashMap<Meaning, String>,
+        foreground_colors: HashMap<Meaning, String>,
         debug: bool,
     ) -> Theme {
-        let colors: HashMap<Meaning, Color> = colors
+        let styles: HashMap<Meaning, ContentStyle> = foreground_colors
             .iter()
             .map(|(name, color)| {
                 (
                     *name,
-                    from_string(color).unwrap_or_else(|msg: String| {
+                    StyleFactory::from_fg_string(color).unwrap_or_else(|err| {
                         if debug {
-                            log::warn!("Could not load theme color: {} -> {}", msg, color);
+                            log::warn!(
+                                "Tried to load string as a color unsuccessfully: ({}={}) {}",
+                                name,
+                                color,
+                                err
+                            );
                         }
-                        Color::Grey
+                        ContentStyle::default()
                     }),
                 )
             })
             .collect();
-        make_theme(name, parent, &colors)
+        Theme::from_map(name, parent, &styles)
+    }
+
+    // Boil down a meaning-color hashmap into a theme, by taking the defaults
+    // for any unknown colors
+    fn from_map(
+        name: String,
+        parent: Option<&Theme>,
+        overrides: &HashMap<Meaning, ContentStyle>,
+    ) -> Theme {
+        let styles = match parent {
+            Some(theme) => Box::new(theme.styles.clone()),
+            None => Box::new(DEFAULT_THEME.styles.clone()),
+        }
+        .iter()
+        .map(|(name, color)| match overrides.get(name) {
+            Some(value) => (*name, *value),
+            None => (*name, *color),
+        })
+        .collect();
+        Theme::new(name, parent.map(|p| p.name.clone()), styles)
     }
 }
 
@@ -146,61 +174,65 @@ fn from_string(name: &str) -> Result<Color, String> {
     if name.is_empty() {
         return Err("Empty string".into());
     }
-    if let Some(name) = name.strip_prefix('#') {
+    let first_char = name.chars().next().unwrap();
-        let hexcode = name;
+    match first_char {
-        let vec: Vec<u8> = hexcode
+        '#' => {
-            .chars()
+            let hexcode = &name[1..];
-            .collect::<Vec<char>>()
+            let vec: Vec<u8> = hexcode
-            .chunks(2)
+                .chars()
-            .map(|pair| u8::from_str_radix(pair.iter().collect::<String>().as_str(), 16))
+                .collect::<Vec<char>>()
-            .filter_map(|n| n.ok())
+                .chunks(2)
-            .collect();
+                .map(|pair| u8::from_str_radix(pair.iter().collect::<String>().as_str(), 16))
-        if vec.len() != 3 {
+                .filter_map(|n| n.ok())
-            return Err("Could not parse 3 hex values from string".into());
+                .collect();
+            if vec.len() != 3 {
+                return Err("Could not parse 3 hex values from string".into());
+            }
+            Ok(Color::Rgb {
+                r: vec[0],
+                g: vec[1],
+                b: vec[2],
+            })
+        }
+        '@' => {
+            // For full fleixibility, we need to use serde_json, given
+            // crossterm's approach.
+            serde_json::from_str::<Color>(format!("\"{}\"", &name[1..]).as_str())
+                .map_err(|_| format!("Could not convert color name {} to Crossterm color", name))
+        }
+        _ => {
+            let srgb = named::from_str(name).ok_or("No such color in palette")?;
+            Ok(Color::Rgb {
+                r: srgb.red,
+                g: srgb.green,
+                b: srgb.blue,
+            })
         }
-        Ok(Color::Rgb {
-            r: vec[0],
-            g: vec[1],
-            b: vec[2],
-        })
-    } else {
-        let srgb = named::from_str(name).ok_or("No such color in palette")?;
-        Ok(Color::Rgb {
-            r: srgb.red,
-            g: srgb.green,
-            b: srgb.blue,
-        })
     }
 }
 
-// For succinctness, if we are confident that the name will be known,
+pub struct StyleFactory {}
-// this routine is available to keep the code readable
-fn _from_known(name: &str) -> Color {
-    from_string(name).unwrap()
-}
 
-// Boil down a meaning-color hashmap into a theme, by taking the defaults
+impl StyleFactory {
-// for any unknown colors
+    fn from_fg_string(name: &str) -> Result<ContentStyle, String> {
-fn make_theme(name: String, parent: Option<&Theme>, overrides: &HashMap<Meaning, Color>) -> Theme {
+        match from_string(name) {
-    let colors = match parent {
+            Ok(color) => Ok(Self::from_fg_color(color)),
-        Some(theme) => Box::new(theme.colors.clone()),
+            Err(err) => Err(err),
-        None => Box::new(HashMap::from([
+        }
-            (Meaning::AlertError, Color::Red),
+    }
-            (Meaning::AlertWarn, Color::Yellow),
+
-            (Meaning::AlertInfo, Color::Green),
+    // For succinctness, if we are confident that the name will be known,
-            (Meaning::Annotation, Color::DarkGrey),
+    // this routine is available to keep the code readable
-            (Meaning::Guidance, Color::Blue),
+    fn known_fg_string(name: &str) -> ContentStyle {
-            (Meaning::Important, Color::White),
+        Self::from_fg_string(name).unwrap()
-            (Meaning::Base, Color::Grey),
+    }
-        ])),
+
+    fn from_fg_color(color: Color) -> ContentStyle {
+        ContentStyle {
+            foreground_color: Some(color),
+            ..ContentStyle::default()
+        }
     }
-    .iter()
-    .map(|(name, color)| match overrides.get(name) {
-        Some(value) => (*name, *value),
-        None => (*name, *color),
-    })
-    .collect();
-    Theme::new(name, parent.map(|p| p.name.clone()), colors)
 }
 
 // Built-in themes. Rather than having extra files added before any theming
@@ -221,33 +253,82 @@ lazy_static! {
             (Meaning::Title, Meaning::Important),
         ])
     };
+    static ref DEFAULT_THEME: Theme = {
+        Theme::new(
+            "default".to_string(),
+            None,
+            HashMap::from([
+                (Meaning::AlertError, StyleFactory::from_fg_color(Color::Red)),
+                (
+                    Meaning::AlertWarn,
+                    StyleFactory::from_fg_color(Color::Yellow),
+                ),
+                (
+                    Meaning::AlertInfo,
+                    StyleFactory::from_fg_color(Color::Green),
+                ),
+                (
+                    Meaning::Annotation,
+                    StyleFactory::from_fg_color(Color::DarkGrey),
+                ),
+                (Meaning::Guidance, StyleFactory::from_fg_color(Color::Blue)),
+                (
+                    Meaning::Important,
+                    StyleFactory::from_fg_color(Color::White),
+                ),
+                (Meaning::Muted, StyleFactory::from_fg_color(Color::Grey)),
+                (Meaning::Base, ContentStyle::default()),
+            ]),
+        )
+    };
     static ref BUILTIN_THEMES: HashMap<&'static str, Theme> = {
         HashMap::from([
-            ("", HashMap::new()),
+            ("default", HashMap::new()),
             (
                 "autumn",
                 HashMap::from([
-                    (Meaning::AlertError, _from_known("saddlebrown")),
+                    (
-                    (Meaning::AlertWarn, _from_known("darkorange")),
+                        Meaning::AlertError,
-                    (Meaning::AlertInfo, _from_known("gold")),
+                        StyleFactory::known_fg_string("saddlebrown"),
-                    (Meaning::Annotation, Color::DarkGrey),
+                    ),
-                    (Meaning::Guidance, _from_known("brown")),
+                    (
+                        Meaning::AlertWarn,
+                        StyleFactory::known_fg_string("darkorange"),
+                    ),
+                    (Meaning::AlertInfo, StyleFactory::known_fg_string("gold")),
+                    (
+                        Meaning::Annotation,
+                        StyleFactory::from_fg_color(Color::DarkGrey),
+                    ),
+                    (Meaning::Guidance, StyleFactory::known_fg_string("brown")),
                 ]),
             ),
             (
                 "marine",
                 HashMap::from([
-                    (Meaning::AlertError, _from_known("yellowgreen")),
+                    (
-                    (Meaning::AlertWarn, _from_known("cyan")),
+                        Meaning::AlertError,
-                    (Meaning::AlertInfo, _from_known("turquoise")),
+                        StyleFactory::known_fg_string("yellowgreen"),
-                    (Meaning::Annotation, _from_known("steelblue")),
+                    ),
-                    (Meaning::Base, _from_known("lightsteelblue")),
+                    (Meaning::AlertWarn, StyleFactory::known_fg_string("cyan")),
-                    (Meaning::Guidance, _from_known("teal")),
+                    (
+                        Meaning::AlertInfo,
+                        StyleFactory::known_fg_string("turquoise"),
+                    ),
+                    (
+                        Meaning::Annotation,
+                        StyleFactory::known_fg_string("steelblue"),
+                    ),
+                    (
+                        Meaning::Base,
+                        StyleFactory::known_fg_string("lightsteelblue"),
+                    ),
+                    (Meaning::Guidance, StyleFactory::known_fg_string("teal")),
                 ]),
             ),
         ])
         .iter()
-        .map(|(name, theme)| (*name, make_theme(name.to_string(), None, theme)))
+        .map(|(name, theme)| (*name, Theme::from_map(name.to_string(), None, theme)))
         .collect()
     };
 }
@@ -354,7 +435,7 @@ impl ThemeManager {
             );
         }
 
-        let theme = Theme::from_map(theme_config.theme.name, parent, colors, debug);
+        let theme = Theme::from_foreground_colors(theme_config.theme.name, parent, colors, debug);
         let name = name.to_string();
         self.loaded_themes.insert(name.clone(), theme);
         let theme = self.loaded_themes.get(&name).unwrap();
@@ -374,7 +455,7 @@ impl ThemeManager {
                 Ok(theme) => theme,
                 Err(err) => {
                     log::warn!("Could not load theme {}: {}", name, err);
-                    built_ins.get("").unwrap()
+                    built_ins.get("default").unwrap()
                 }
             },
         }
@@ -401,7 +482,10 @@ mod theme_tests {
         let mytheme = Theme::new(
             "mytheme".to_string(),
             None,
-            HashMap::from([(Meaning::AlertError, _from_known("yellowgreen"))]),
+            HashMap::from([(
+                Meaning::AlertError,
+                StyleFactory::known_fg_string("yellowgreen"),
+            )]),
         );
         manager.loaded_themes.insert("mytheme".to_string(), mytheme);
         let theme = manager.load_theme("mytheme", None);
@@ -417,7 +501,7 @@ mod theme_tests {
 
         // We use title as an example of a meaning that is not defined
         // even in the base theme.
-        assert!(!BUILTIN_THEMES[""].colors.contains_key(&Meaning::Title));
+        assert!(!DEFAULT_THEME.styles.contains_key(&Meaning::Title));
 
         let config = Config::builder()
             .add_source(ConfigFile::from_str(
@@ -443,11 +527,11 @@ mod theme_tests {
             from_string("white").ok()
         );
 
-        // Falls back to grey as general "unknown" color.
+        // Does not fall back to any color.
-        assert_eq!(
+        assert_eq!(theme.as_style(Meaning::AlertInfo).foreground_color, None);
-            theme.as_style(Meaning::AlertInfo).foreground_color,
+
-            Some(Color::Grey)
+        // Even for the base.
-        );
+        assert_eq!(theme.as_style(Meaning::Base).foreground_color, None);
 
         // Falls back to red as meaning missing from theme, so picks base default.
         assert_eq!(
@@ -496,12 +580,15 @@ mod theme_tests {
     #[test]
     fn test_can_get_colors_via_convenience_functions() {
         let mut manager = ThemeManager::new(Some(true), Some("".to_string()));
-        let theme = manager.load_theme("", None);
+        let theme = manager.load_theme("default", None);
-        assert_eq!(theme.get_error(), Color::Red);
+        assert_eq!(theme.get_error().foreground_color.unwrap(), Color::Red);
-        assert_eq!(theme.get_warning(), Color::Yellow);
+        assert_eq!(theme.get_warning().foreground_color.unwrap(), Color::Yellow);
-        assert_eq!(theme.get_info(), Color::Green);
+        assert_eq!(theme.get_info().foreground_color.unwrap(), Color::Green);
-        assert_eq!(theme.get_base(), Color::Grey);
+        assert_eq!(theme.get_base().foreground_color, None);
-        assert_eq!(theme.get_alert(log::Level::Error), Color::Red)
+        assert_eq!(
+            theme.get_alert(log::Level::Error).foreground_color.unwrap(),
+            Color::Red
+        )
     }
 
     #[test]
@@ -640,7 +727,7 @@ mod theme_tests {
                     assert_eq!(captured_logs[0].level, log::Level::Warn);
                     assert_eq!(
                         captured_logs[1].body,
-                        "Could not load theme color: No such color in palette -> xinetic"
+                        "Tried to load string as a color unsuccessfully: (AlertInfo=xinetic) No such color in palette"
                     );
                     assert_eq!(captured_logs[1].level, log::Level::Warn)
                 } else {
@@ -683,5 +770,27 @@ mod theme_tests {
                 Err("Could not parse 3 hex values from string".into())
             );
         });
+
+        assert_eq!(from_string("@dark_grey").unwrap(), Color::DarkGrey);
+        assert_eq!(
+            from_string("@rgb_(255,255,255)").unwrap(),
+            Color::Rgb {
+                r: 255,
+                g: 255,
+                b: 255
+            }
+        );
+        assert_eq!(from_string("@ansi_(255)").unwrap(), Color::AnsiValue(255));
+        ["@", "@DarkGray", "@Dark 4ay", "@ansi(256)"]
+            .iter()
+            .for_each(|inp| {
+                assert_eq!(
+                    from_string(inp),
+                    Err(format!(
+                        "Could not convert color name {} to Crossterm color",
+                        inp
+                    ))
+                );
+            });
     }
 }

crates/atuin-history/src/stats.rs

@@ -1,10 +1,10 @@
 use std::collections::{HashMap, HashSet};
 
-use crossterm::style::{ResetColor, SetAttribute, SetForegroundColor};
+use crossterm::style::{Color, ResetColor, SetAttribute, SetForegroundColor};
 use serde::{Deserialize, Serialize};
 use unicode_segmentation::UnicodeSegmentation;
 
-use atuin_client::{history::History, settings::Settings, theme::Theme};
+use atuin_client::{history::History, settings::Settings, theme::Meaning, theme::Theme};
 
 #[derive(Debug, Serialize, Deserialize)]
 pub struct Stats {
@@ -126,21 +126,42 @@ pub fn pretty_print(stats: Stats, ngram_size: usize, theme: &Theme) {
         });
 
     for (command, count) in stats.top {
-        let gray = SetForegroundColor(theme.get_base());
+        let gray = SetForegroundColor(match theme.as_style(Meaning::Muted).foreground_color {
+            Some(color) => color,
+            None => Color::Grey,
+        });
         let bold = SetAttribute(crossterm::style::Attribute::Bold);
 
         let in_ten = 10 * count / max;
 
         print!("[");
-        print!("{}", SetForegroundColor(theme.get_error()));
+        print!(
+            "{}",
+            SetForegroundColor(match theme.get_error().foreground_color {
+                Some(color) => color,
+                None => Color::Red,
+            })
+        );
 
         for i in 0..in_ten {
             if i == 2 {
-                print!("{}", SetForegroundColor(theme.get_warning()));
+                print!(
+                    "{}",
+                    SetForegroundColor(match theme.get_warning().foreground_color {
+                        Some(color) => color,
+                        None => Color::Yellow,
+                    })
+                );
             }
 
             if i == 5 {
-                print!("{}", SetForegroundColor(theme.get_info()));
+                print!(
+                    "{}",
+                    SetForegroundColor(match theme.get_info().foreground_color {
+                        Some(color) => color,
+                        None => Color::Green,
+                    })
+                );
             }
 
             print!("▮");

crates/atuin/src/command/client/search/interactive.rs

@@ -766,18 +766,16 @@ impl State {
 
     fn build_title(&mut self, theme: &Theme) -> Paragraph {
         let title = if self.update_needed.is_some() {
+            let error_style: Style = theme.get_error().into();
             Paragraph::new(Text::from(Span::styled(
                 format!("Atuin v{VERSION} - UPGRADE"),
-                Style::default()
+                error_style.add_modifier(Modifier::BOLD),
-                    .add_modifier(Modifier::BOLD)
-                    .fg(theme.get_error().into()),
             )))
         } else {
+            let style: Style = theme.as_style(Meaning::Base).into();
             Paragraph::new(Text::from(Span::styled(
                 format!("Atuin v{VERSION}"),
-                Style::default()
+                style.add_modifier(Modifier::BOLD),
-                    .add_modifier(Modifier::BOLD)
-                    .fg(theme.get_base().into()),
             )))
         };
         title.alignment(Alignment::Left)