Refactor theme parsing (#570)

helix-view/src/graphics.rs

@@ -1,5 +1,8 @@
 use bitflags::bitflags;
-use std::cmp::{max, min};
+use std::{
+    cmp::{max, min},
+    str::FromStr,
+};
 
 #[derive(Debug, Clone, Copy, PartialEq)]
 /// UNSTABLE
@@ -237,6 +240,25 @@ bitflags! {
     }
 }
 
+impl FromStr for Modifier {
+    type Err = &'static str;
+
+    fn from_str(modifier: &str) -> Result<Self, Self::Err> {
+        match modifier {
+            "bold" => Ok(Self::BOLD),
+            "dim" => Ok(Self::DIM),
+            "italic" => Ok(Self::ITALIC),
+            "underlined" => Ok(Self::UNDERLINED),
+            "slow_blink" => Ok(Self::SLOW_BLINK),
+            "rapid_blink" => Ok(Self::RAPID_BLINK),
+            "reversed" => Ok(Self::REVERSED),
+            "hidden" => Ok(Self::HIDDEN),
+            "crossed_out" => Ok(Self::CROSSED_OUT),
+            _ => Err("Invalid modifier"),
+        }
+    }
+}
+
 /// Style let you control the main characteristics of the displayed elements.
 ///
 /// ```rust

helix-view/src/theme.rs

@@ -1,5 +1,6 @@
 use std::{
     collections::HashMap,
+    convert::TryFrom,
     path::{Path, PathBuf},
 };
 
@@ -11,8 +12,6 @@ use toml::Value;
 
 pub use crate::graphics::{Color, Modifier, Style};
 
-/// Color theme for syntax highlighting.
-
 pub static DEFAULT_THEME: Lazy<Theme> = Lazy::new(|| {
     toml::from_slice(include_bytes!("../../theme.toml")).expect("Failed to parse default theme")
 });
@@ -54,22 +53,10 @@ impl Loader {
             .map(|entries| {
                 entries
                     .filter_map(|entry| {
-                        if let Ok(entry) = entry {
+                        let entry = entry.ok()?;
                         let path = entry.path();
-                            if let Some(ext) = path.extension() {
+                        (path.extension()? == "toml")
-                                if ext != "toml" {
+                            .then(|| path.file_stem().unwrap().to_string_lossy().into_owned())
-                                    return None;
-                                }
-                                return Some(
-                                    entry
-                                        .file_name()
-                                        .to_string_lossy()
-                                        .trim_end_matches(".toml")
-                                        .to_owned(),
-                                );
-                            }
-                        }
-                        None
                     })
                     .collect()
             })
@@ -103,13 +90,23 @@ impl<'de> Deserialize<'de> for Theme {
         let mut styles = HashMap::new();
 
         if let Ok(mut colors) = HashMap::<String, Value>::deserialize(deserializer) {
-            let palette = parse_palette(colors.remove("palette"));
+            // TODO: alert user of parsing failures in editor
-            // scopes.reserve(colors.len());
+            let palette = colors
+                .remove("palette")
+                .map(|value| {
+                    ThemePalette::try_from(value).unwrap_or_else(|err| {
+                        warn!("{}", err);
+                        ThemePalette::default()
+                    })
+                })
+                .unwrap_or_default();
+
             styles.reserve(colors.len());
             for (name, style_value) in colors {
                 let mut style = Style::default();
-                parse_style(&mut style, style_value, &palette);
+                if let Err(err) = palette.parse_style(&mut style, style_value) {
-                // scopes.push(name);
+                    warn!("{}", err);
+                }
                 styles.insert(name, style);
             }
         }
@@ -119,121 +116,120 @@ impl<'de> Deserialize<'de> for Theme {
     }
 }
 
-fn parse_palette(value: Option<Value>) -> HashMap<String, Color> {
+impl Theme {
-    match value {
+    pub fn get(&self, scope: &str) -> Style {
-        Some(Value::Table(entries)) => entries,
+        self.try_get(scope)
-        _ => return HashMap::default(),
+            .unwrap_or_else(|| Style::default().fg(Color::Rgb(0, 0, 255)))
-    }
-    .into_iter()
-    .filter_map(|(name, value)| {
-        let color = parse_color(value, &HashMap::default())?;
-        Some((name, color))
-    })
-    .collect()
     }
 
-fn parse_style(style: &mut Style, value: Value, palette: &HashMap<String, Color>) {
+    pub fn try_get(&self, scope: &str) -> Option<Style> {
-    //TODO: alert user of parsing failures
+        self.styles.get(scope).copied()
-    if let Value::Table(entries) = value {
-        for (name, value) in entries {
-            match name.as_str() {
-                "fg" => {
-                    if let Some(color) = parse_color(value, palette) {
-                        *style = style.fg(color);
-                    }
-                }
-                "bg" => {
-                    if let Some(color) = parse_color(value, palette) {
-                        *style = style.bg(color);
-                    }
     }
-                "modifiers" => {
+
-                    if let Value::Array(arr) = value {
+    #[inline]
-                        for modifier in arr.iter().filter_map(parse_modifier) {
+    pub fn scopes(&self) -> &[String] {
-                            *style = style.add_modifier(modifier);
+        &self.scopes
     }
+
+    pub fn find_scope_index(&self, scope: &str) -> Option<usize> {
+        self.scopes().iter().position(|s| s == scope)
     }
 }
-                _ => (),
+
+struct ThemePalette {
+    palette: HashMap<String, Color>,
 }
+
+impl Default for ThemePalette {
+    fn default() -> Self {
+        Self::new(HashMap::new())
     }
-    } else if let Some(color) = parse_color(value, palette) {
-        *style = style.fg(color);
 }
+
+impl ThemePalette {
+    pub fn new(palette: HashMap<String, Color>) -> Self {
+        Self { palette }
     }
 
-fn hex_string_to_rgb(s: &str) -> Option<(u8, u8, u8)> {
+    pub fn hex_string_to_rgb(s: &str) -> Result<Color, String> {
         if s.starts_with('#') && s.len() >= 7 {
             if let (Ok(red), Ok(green), Ok(blue)) = (
                 u8::from_str_radix(&s[1..3], 16),
                 u8::from_str_radix(&s[3..5], 16),
                 u8::from_str_radix(&s[5..7], 16),
             ) {
-            Some((red, green, blue))
+                return Ok(Color::Rgb(red, green, blue));
-        } else {
-            None
             }
-    } else {
-        None
         }
+
+        Err(format!("Theme: malformed hexcode: {}", s))
     }
 
-fn parse_color(value: Value, palette: &HashMap<String, Color>) -> Option<Color> {
+    fn parse_value_as_str(value: &Value) -> Result<&str, String> {
-    if let Value::String(s) = value {
+        value
-        if let Some(color) = palette.get(&s) {
+            .as_str()
-            Some(*color)
+            .ok_or(format!("Theme: unrecognized value: {}", value))
-        } else if let Some((red, green, blue)) = hex_string_to_rgb(&s) {
-            Some(Color::Rgb(red, green, blue))
-        } else {
-            warn!("malformed hexcode in theme: {}", s);
-            None
     }
-    } else {
+
-        warn!("unrecognized value in theme: {}", value);
+    pub fn parse_color(&self, value: Value) -> Result<Color, String> {
-        None
+        let value = Self::parse_value_as_str(&value)?;
+
+        self.palette
+            .get(value)
+            .copied()
+            .ok_or("")
+            .or_else(|_| Self::hex_string_to_rgb(value))
     }
+
+    pub fn parse_modifier(value: &Value) -> Result<Modifier, String> {
+        value
+            .as_str()
+            .and_then(|s| s.parse().ok())
+            .ok_or(format!("Theme: invalid modifier: {}", value))
     }
 
-fn parse_modifier(value: &Value) -> Option<Modifier> {
+    pub fn parse_style(&self, style: &mut Style, value: Value) -> Result<(), String> {
-    if let Value::String(s) = value {
+        if let Value::Table(entries) = value {
-        match s.as_str() {
+            for (name, value) in entries {
-            "bold" => Some(Modifier::BOLD),
+                match name.as_str() {
-            "dim" => Some(Modifier::DIM),
+                    "fg" => *style = style.fg(self.parse_color(value)?),
-            "italic" => Some(Modifier::ITALIC),
+                    "bg" => *style = style.bg(self.parse_color(value)?),
-            "underlined" => Some(Modifier::UNDERLINED),
+                    "modifiers" => {
-            "slow_blink" => Some(Modifier::SLOW_BLINK),
+                        let modifiers = value
-            "rapid_blink" => Some(Modifier::RAPID_BLINK),
+                            .as_array()
-            "reversed" => Some(Modifier::REVERSED),
+                            .ok_or("Theme: modifiers should be an array")?;
-            "hidden" => Some(Modifier::HIDDEN),
+
-            "crossed_out" => Some(Modifier::CROSSED_OUT),
+                        for modifier in modifiers {
-            _ => {
+                            *style = style.add_modifier(Self::parse_modifier(modifier)?);
-                warn!("unrecognized modifier in theme: {}", s);
+                        }
-                None
+                    }
+                    _ => return Err(format!("Theme: invalid style attribute: {}", name)),
                 }
             }
         } else {
-        warn!("unrecognized modifier in theme: {}", value);
+            *style = style.fg(self.parse_color(value)?);
-        None
         }
+        Ok(())
     }
-
-impl Theme {
-    pub fn get(&self, scope: &str) -> Style {
-        self.try_get(scope)
-            .unwrap_or_else(|| Style::default().fg(Color::Rgb(0, 0, 255)))
 }
 
-    pub fn try_get(&self, scope: &str) -> Option<Style> {
+impl TryFrom<Value> for ThemePalette {
-        self.styles.get(scope).copied()
+    type Error = String;
-    }
 
-    #[inline]
+    fn try_from(value: Value) -> Result<Self, Self::Error> {
-    pub fn scopes(&self) -> &[String] {
+        let map = match value {
-        &self.scopes
+            Value::Table(entries) => entries,
+            _ => return Ok(Self::default()),
+        };
+
+        let mut palette = HashMap::with_capacity(map.len());
+        for (name, value) in map {
+            let value = Self::parse_value_as_str(&value)?;
+            let color = Self::hex_string_to_rgb(value)?;
+            palette.insert(name, color);
         }
 
-    pub fn find_scope_index(&self, scope: &str) -> Option<usize> {
+        Ok(Self::new(palette))
-        self.scopes().iter().position(|s| s == scope)
     }
 }
 
@@ -242,23 +238,21 @@ fn test_parse_style_string() {
     let fg = Value::String("#ffffff".to_string());
 
     let mut style = Style::default();
-    parse_style(&mut style, fg, &HashMap::default());
+    let palette = ThemePalette::default();
+    palette.parse_style(&mut style, fg).unwrap();
 
     assert_eq!(style, Style::default().fg(Color::Rgb(255, 255, 255)));
 }
 
 #[test]
 fn test_palette() {
+    use helix_core::hashmap;
     let fg = Value::String("my_color".to_string());
 
     let mut style = Style::default();
-    parse_style(
+    let palette =
-        &mut style,
+        ThemePalette::new(hashmap! { "my_color".to_string() => Color::Rgb(255, 255, 255) });
-        fg,
+    palette.parse_style(&mut style, fg).unwrap();
-        &vec![("my_color".to_string(), Color::Rgb(255, 255, 255))]
-            .into_iter()
-            .collect(),
-    );
 
     assert_eq!(style, Style::default().fg(Color::Rgb(255, 255, 255)));
 }
@@ -274,9 +268,10 @@ fn test_parse_style_table() {
     };
 
     let mut style = Style::default();
+    let palette = ThemePalette::default();
     if let Value::Table(entries) = table {
         for (_name, value) in entries {
-            parse_style(&mut style, value, &HashMap::default());
+            palette.parse_style(&mut style, value).unwrap();
         }
     }