From 82dd96369302f60a9c83a2d54d021458f82bcd36 Mon Sep 17 00:00:00 2001
From: Tim <63202655+sarsapar1lla@users.noreply.github.com>
Date: Sat, 28 Sep 2024 12:52:09 +0100
Subject: [PATCH] Add: validation of bundled themes in build workflow (#11627)

* Add: xtask to check themes for validation warnings

* Update: tidied up runtime paths

* Update: test build workflow

* Update: address clippy lints

* Revert: only trigger workflow on push to master branch

* Add: Theme::from_keys factory method to construct theme from Toml keys

* Update: returning validation failures in Loader.load method

* Update: commented out invalid keys from affected themes

* Update: correct invalid keys so that valid styles still applied

* Update: include default and base16_default themes in check

* Update: renamed validation_failures to load_errors

* Update: introduce load_with_warnings helper function and centralise logging of theme warnings

* Update: use consistent naming throughout
---
 .github/workflows/build.yml      |   4 ++
 helix-view/src/theme.rs          | 116 ++++++++++++++++++-------------
 runtime/themes/autumn.toml       |   6 +-
 runtime/themes/emacs.toml        |   3 +-
 runtime/themes/flatwhite.toml    |   7 +-
 runtime/themes/kanagawa.toml     |   3 +-
 runtime/themes/monokai.toml      |   3 +-
 runtime/themes/monokai_aqua.toml |   9 ++-
 runtime/themes/zed_onedark.toml  |   3 +-
 xtask/src/main.rs                |   7 ++
 xtask/src/path.rs                |  10 ++-
 xtask/src/theme_check.rs         |  33 +++++++++
 12 files changed, 143 insertions(+), 61 deletions(-)
 create mode 100644 xtask/src/theme_check.rs

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 93fcb9816..c9f198d0c 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -16,6 +16,7 @@ jobs:
     steps:
       - name: Checkout sources
         uses: actions/checkout@v4
+
       - name: Install stable toolchain
         uses: dtolnay/rust-toolchain@1.70
 
@@ -107,6 +108,9 @@ jobs:
       - name: Validate queries
         run: cargo xtask query-check
 
+      - name: Validate themes
+        run: cargo xtask theme-check
+
       - name: Generate docs
         run: cargo xtask docgen
 
diff --git a/helix-view/src/theme.rs b/helix-view/src/theme.rs
index 4acc56648..9dc326444 100644
--- a/helix-view/src/theme.rs
+++ b/helix-view/src/theme.rs
@@ -53,20 +53,34 @@ pub fn new(dirs: &[PathBuf]) -> Self {
 
     /// Loads a theme searching directories in priority order.
     pub fn load(&self, name: &str) -> Result<Theme> {
+        let (theme, warnings) = self.load_with_warnings(name)?;
+
+        for warning in warnings {
+            warn!("Theme '{}': {}", name, warning);
+        }
+
+        Ok(theme)
+    }
+
+    /// Loads a theme searching directories in priority order, returning any warnings
+    pub fn load_with_warnings(&self, name: &str) -> Result<(Theme, Vec<String>)> {
         if name == "default" {
-            return Ok(self.default());
+            return Ok((self.default(), Vec::new()));
         }
         if name == "base16_default" {
-            return Ok(self.base16_default());
+            return Ok((self.base16_default(), Vec::new()));
         }
 
         let mut visited_paths = HashSet::new();
-        let theme = self.load_theme(name, &mut visited_paths).map(Theme::from)?;
+        let (theme, warnings) = self
+            .load_theme(name, &mut visited_paths)
+            .map(Theme::from_toml)?;
 
-        Ok(Theme {
+        let theme = Theme {
             name: name.into(),
             ..theme
-        })
+        };
+        Ok((theme, warnings))
     }
 
     /// Recursively load a theme, merging with any inherited parent themes.
@@ -87,10 +101,7 @@ fn load_theme(&self, name: &str, visited_paths: &mut HashSet<PathBuf>) -> Result
 
         let theme_toml = if let Some(parent_theme_name) = inherits {
             let parent_theme_name = parent_theme_name.as_str().ok_or_else(|| {
-                anyhow!(
-                    "Theme: expected 'inherits' to be a string: {}",
-                    parent_theme_name
-                )
+                anyhow!("Expected 'inherits' to be a string: {}", parent_theme_name)
             })?;
 
             let parent_theme_toml = match parent_theme_name {
@@ -181,9 +192,9 @@ fn path(&self, name: &str, visited_paths: &mut HashSet<PathBuf>) -> Result<PathB
             })
             .ok_or_else(|| {
                 if cycle_found {
-                    anyhow!("Theme: cycle found in inheriting: {}", name)
+                    anyhow!("Cycle found in inheriting: {}", name)
                 } else {
-                    anyhow!("Theme: file not found for: {}", name)
+                    anyhow!("File not found for: {}", name)
                 }
             })
     }
@@ -220,19 +231,11 @@ pub struct Theme {
 
 impl From<Value> for Theme {
     fn from(value: Value) -> Self {
-        if let Value::Table(table) = value {
-            let (styles, scopes, highlights) = build_theme_values(table);
-
-            Self {
-                styles,
-                scopes,
-                highlights,
-                ..Default::default()
-            }
-        } else {
-            warn!("Expected theme TOML value to be a table, found {:?}", value);
-            Default::default()
+        let (theme, warnings) = Theme::from_toml(value);
+        for warning in warnings {
+            warn!("{}", warning);
         }
+        theme
     }
 }
 
@@ -242,31 +245,29 @@ fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
         D: Deserializer<'de>,
     {
         let values = Map::<String, Value>::deserialize(deserializer)?;
-
-        let (styles, scopes, highlights) = build_theme_values(values);
-
-        Ok(Self {
-            styles,
-            scopes,
-            highlights,
-            ..Default::default()
-        })
+        let (theme, warnings) = Theme::from_keys(values);
+        for warning in warnings {
+            warn!("{}", warning);
+        }
+        Ok(theme)
     }
 }
 
 fn build_theme_values(
     mut values: Map<String, Value>,
-) -> (HashMap<String, Style>, Vec<String>, Vec<Style>) {
+) -> (HashMap<String, Style>, Vec<String>, Vec<Style>, Vec<String>) {
     let mut styles = HashMap::new();
     let mut scopes = Vec::new();
     let mut highlights = Vec::new();
 
+    let mut warnings = Vec::new();
+
     // TODO: alert user of parsing failures in editor
     let palette = values
         .remove("palette")
         .map(|value| {
             ThemePalette::try_from(value).unwrap_or_else(|err| {
-                warn!("{}", err);
+                warnings.push(err);
                 ThemePalette::default()
             })
         })
@@ -279,7 +280,7 @@ fn build_theme_values(
     for (name, style_value) in values {
         let mut style = Style::default();
         if let Err(err) = palette.parse_style(&mut style, style_value) {
-            warn!("{}", err);
+            warnings.push(err);
         }
 
         // these are used both as UI and as highlights
@@ -288,7 +289,7 @@ fn build_theme_values(
         highlights.push(style);
     }
 
-    (styles, scopes, highlights)
+    (styles, scopes, highlights, warnings)
 }
 
 impl Theme {
@@ -354,6 +355,27 @@ pub fn is_16_color(&self) -> bool {
                 .all(|color| !matches!(color, Some(Color::Rgb(..))))
         })
     }
+
+    fn from_toml(value: Value) -> (Self, Vec<String>) {
+        if let Value::Table(table) = value {
+            Theme::from_keys(table)
+        } else {
+            warn!("Expected theme TOML value to be a table, found {:?}", value);
+            Default::default()
+        }
+    }
+
+    fn from_keys(toml_keys: Map<String, Value>) -> (Self, Vec<String>) {
+        let (styles, scopes, highlights, load_errors) = build_theme_values(toml_keys);
+
+        let theme = Self {
+            styles,
+            scopes,
+            highlights,
+            ..Default::default()
+        };
+        (theme, load_errors)
+    }
 }
 
 struct ThemePalette {
@@ -408,7 +430,7 @@ fn ansi_string_to_rgb(s: &str) -> Result<Color, String> {
         if let Ok(index) = s.parse::<u8>() {
             return Ok(Color::Indexed(index));
         }
-        Err(format!("Theme: malformed ANSI: {}", s))
+        Err(format!("Malformed ANSI: {}", s))
     }
 
     fn hex_string_to_rgb(s: &str) -> Result<Color, String> {
@@ -422,13 +444,13 @@ fn hex_string_to_rgb(s: &str) -> Result<Color, String> {
             }
         }
 
-        Err(format!("Theme: malformed hexcode: {}", s))
+        Err(format!("Malformed hexcode: {}", s))
     }
 
     fn parse_value_as_str(value: &Value) -> Result<&str, String> {
         value
             .as_str()
-            .ok_or(format!("Theme: unrecognized value: {}", value))
+            .ok_or(format!("Unrecognized value: {}", value))
     }
 
     pub fn parse_color(&self, value: Value) -> Result<Color, String> {
@@ -445,14 +467,14 @@ 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))
+            .ok_or(format!("Invalid modifier: {}", value))
     }
 
     pub fn parse_underline_style(value: &Value) -> Result<UnderlineStyle, String> {
         value
             .as_str()
             .and_then(|s| s.parse().ok())
-            .ok_or(format!("Theme: invalid underline style: {}", value))
+            .ok_or(format!("Invalid underline style: {}", value))
     }
 
     pub fn parse_style(&self, style: &mut Style, value: Value) -> Result<(), String> {
@@ -462,9 +484,7 @@ pub fn parse_style(&self, style: &mut Style, value: Value) -> Result<(), String>
                     "fg" => *style = style.fg(self.parse_color(value)?),
                     "bg" => *style = style.bg(self.parse_color(value)?),
                     "underline" => {
-                        let table = value
-                            .as_table_mut()
-                            .ok_or("Theme: underline must be table")?;
+                        let table = value.as_table_mut().ok_or("Underline must be table")?;
                         if let Some(value) = table.remove("color") {
                             *style = style.underline_color(self.parse_color(value)?);
                         }
@@ -473,13 +493,11 @@ pub fn parse_style(&self, style: &mut Style, value: Value) -> Result<(), String>
                         }
 
                         if let Some(attr) = table.keys().next() {
-                            return Err(format!("Theme: invalid underline attribute: {attr}"));
+                            return Err(format!("Invalid underline attribute: {attr}"));
                         }
                     }
                     "modifiers" => {
-                        let modifiers = value
-                            .as_array()
-                            .ok_or("Theme: modifiers should be an array")?;
+                        let modifiers = value.as_array().ok_or("Modifiers should be an array")?;
 
                         for modifier in modifiers {
                             if modifier
@@ -492,7 +510,7 @@ pub fn parse_style(&self, style: &mut Style, value: Value) -> Result<(), String>
                             }
                         }
                     }
-                    _ => return Err(format!("Theme: invalid style attribute: {}", name)),
+                    _ => return Err(format!("Invalid style attribute: {}", name)),
                 }
             }
         } else {
diff --git a/runtime/themes/autumn.toml b/runtime/themes/autumn.toml
index 7e9b3a7e1..d7d9c00bd 100644
--- a/runtime/themes/autumn.toml
+++ b/runtime/themes/autumn.toml
@@ -68,8 +68,10 @@
 "ui.statusline.select" = { fg = "my_gray7", bg = "my_black", modifiers = ["bold"] }
 "ui.text.focus" = "my_white1"
 "ui.text" = "my_white1"
-"ui.virtual.inlay-hint" = { fg = "my_gray4", bg="my_black", modifiers = ["normal"] }
-"ui.virtual.inlay-hint.parameter" = { fg = "my_gray4", modifiers = ["normal"] }
+# Invalid modifier: "normal". See 'https://github.com/helix-editor/helix/issues/5709'
+"ui.virtual.inlay-hint" = { fg = "my_gray4", bg="my_black" } #, modifiers = ["normal"] }
+# "ui.virtual.inlay-hint.parameter" = { fg = "my_gray4", modifiers = ["normal"] }
+"ui.virtual.inlay-hint.parameter" = "my_gray4"
 "ui.virtual.inlay-hint.type" = { fg = "my_gray4", modifiers = ["italic"] }
 "ui.virtual.jump-label" = { fg = "my_yellow2", modifiers = ["bold"] }
 "ui.virtual.ruler" = { bg = "my_gray1" }
diff --git a/runtime/themes/emacs.toml b/runtime/themes/emacs.toml
index c60afe99e..dfd55e07b 100644
--- a/runtime/themes/emacs.toml
+++ b/runtime/themes/emacs.toml
@@ -68,7 +68,8 @@
 "ui.menu.selected" = { fg = "dark_red", bg = "light_blue" }
 "ui.selection" = { bg = "lightgoldenrod1" }
 "ui.selection.primary" = { bg = "lightgoldenrod2" }
-"ui.virtual.whitespace" = "highlight"
+# Malformed ANSI: highlight. See 'https://github.com/helix-editor/helix/issues/5709'
+# "ui.virtual.whitespace" = "highlight"
 "ui.virtual.ruler" = { bg = "gray95" }
 "ui.virtual.inlay-hint" = { fg = "gray75" }
 "ui.cursorline.primary" = { bg = "darkseagreen2" }
diff --git a/runtime/themes/flatwhite.toml b/runtime/themes/flatwhite.toml
index cc8f536cb..1da203502 100644
--- a/runtime/themes/flatwhite.toml
+++ b/runtime/themes/flatwhite.toml
@@ -61,8 +61,11 @@
 "ui.virtual" = { fg = "base5", bg = "base6" }
 "ui.virtual.whitespace" = { fg = "base5" }
 "ui.virtual.ruler" = { bg = "base6" }
-"ui.virtual.inlay-hint" = { fg = "base4", modifiers = ["normal"] }
-"ui.virtual.inlay-hint.parameter" = { fg = "base3", modifiers = ["normal"] }
+# Invalid modifier: "normal". See 'https://github.com/helix-editor/helix/issues/5709'
+# "ui.virtual.inlay-hint" = { fg = "base4", modifiers = ["normal"] }
+# "ui.virtual.inlay-hint.parameter" = { fg = "base3", modifiers = ["normal"] }
+"ui.virtual.inlay-hint" = "base4"
+"ui.virtual.inlay-hint.parameter" = "base3"
 "ui.virtual.inlay-hint.type" = { fg = "base3", modifiers = ["italic"] }
 
 "ui.linenr" = { bg = "base6" }
diff --git a/runtime/themes/kanagawa.toml b/runtime/themes/kanagawa.toml
index dabece68d..8d5a80eef 100644
--- a/runtime/themes/kanagawa.toml
+++ b/runtime/themes/kanagawa.toml
@@ -25,7 +25,8 @@
 "ui.statusline.normal" = { fg = "sumiInk0", bg = "crystalBlue", modifiers = ["bold"] }
 "ui.statusline.insert" = { fg = "sumiInk0", bg = "autumnGreen", modifiers = ["bold"] }
 "ui.statusline.select" = { fg = "sumiInk0", bg = "oniViolet", modifiers = ["bold"] }
-"ui.statusline.separator" = { fg = "", bg = "" }
+# Malformed ANSI: "". See 'https://github.com/helix-editor/helix/issues/5709'
+# "ui.statusline.separator" = { fg = "", bg = "" }
 
 "ui.bufferline" = { fg = "fujiGray", bg = "sumiInk0" }
 "ui.bufferline.active" = { fg = "oldWhite", bg = "sumiInk0" }
diff --git a/runtime/themes/monokai.toml b/runtime/themes/monokai.toml
index b0faac154..22e6d0639 100644
--- a/runtime/themes/monokai.toml
+++ b/runtime/themes/monokai.toml
@@ -79,7 +79,8 @@
 "ui.statusline" = { fg = "active_text", bg = "#414339" }
 "ui.statusline.inactive" = { fg = "active_text", bg = "#75715e" }
 
-"ui.bufferline" = { fg = "grey2", bg = "bg3" }
+# Malformed ANSI: grey2, bg3. See 'https://github.com/helix-editor/helix/issues/5709'
+# "ui.bufferline" = { fg = "grey2", bg = "bg3" }
 "ui.bufferline.active" = { fg = "active_text", bg = "selection", modifiers = [
   "bold",
 ] }
diff --git a/runtime/themes/monokai_aqua.toml b/runtime/themes/monokai_aqua.toml
index 0cb9300d0..b818a09e7 100644
--- a/runtime/themes/monokai_aqua.toml
+++ b/runtime/themes/monokai_aqua.toml
@@ -8,9 +8,12 @@ inherits = "monokai"
 
 "type" = { fg = "type", modifiers = ["bold"] }
 
-"ui.statusline.normal" = { fg = "light-black", bg = "cyan" }
-"ui.statusline.insert" = { fg = "light-black", bg = "green" }
-"ui.statusline.select" = { fg = "light-black", bg = "purple" }
+# Malformed ANSI: light-black, purple. See 'https://github.com/helix-editor/helix/issues/5709'
+# "ui.statusline.normal" = { fg = "light-black", bg = "cyan" }
+"ui.statusline.normal" = { bg = "cyan" }
+# "ui.statusline.insert" = { fg = "light-black", bg = "green" }
+"ui.statusline.insert" = { bg = "green" }
+# "ui.statusline.select" = { fg = "light-black", bg = "purple" }
 
 "ui.virtual.jump-label" = { fg = "cyan", modifiers = ["bold"] }
 
diff --git a/runtime/themes/zed_onedark.toml b/runtime/themes/zed_onedark.toml
index 74db8f75c..160320de1 100644
--- a/runtime/themes/zed_onedark.toml
+++ b/runtime/themes/zed_onedark.toml
@@ -61,7 +61,8 @@
 "ui.cursor" = { fg = "white", modifiers = ["reversed"] }
 "ui.cursor.primary" = { fg = "white", modifiers = ["reversed"] }
 "ui.cursor.match" = { fg = "blue", modifiers = ["underlined"] }
-"ui.cursor.insert" = { fg = "dark-blue" }
+# Malformed ANSI: dark-blue. See 'https://github.com/helix-editor/helix/issues/5709'
+# "ui.cursor.insert" = { fg = "dark-blue" }
 
 "ui.selection" = { bg = "faint-gray" }
 "ui.selection.primary" = { bg = "#293b5bff" }
diff --git a/xtask/src/main.rs b/xtask/src/main.rs
index aba5c4499..fcb462f25 100644
--- a/xtask/src/main.rs
+++ b/xtask/src/main.rs
@@ -2,6 +2,7 @@
 mod helpers;
 mod path;
 mod querycheck;
+mod theme_check;
 
 use std::{env, error::Error};
 
@@ -11,6 +12,7 @@ pub mod tasks {
     use crate::docgen::{lang_features, typable_commands, write};
     use crate::docgen::{LANG_SUPPORT_MD_OUTPUT, TYPABLE_COMMANDS_MD_OUTPUT};
     use crate::querycheck::query_check;
+    use crate::theme_check::theme_check;
     use crate::DynError;
 
     pub fn docgen() -> Result<(), DynError> {
@@ -23,6 +25,10 @@ pub fn querycheck() -> Result<(), DynError> {
         query_check()
     }
 
+    pub fn themecheck() -> Result<(), DynError> {
+        theme_check()
+    }
+
     pub fn print_help() {
         println!(
             "
@@ -43,6 +49,7 @@ fn main() -> Result<(), DynError> {
         Some(t) => match t.as_str() {
             "docgen" => tasks::docgen()?,
             "query-check" => tasks::querycheck()?,
+            "theme-check" => tasks::themecheck()?,
             invalid => return Err(format!("Invalid task name: {}", invalid).into()),
         },
     };
diff --git a/xtask/src/path.rs b/xtask/src/path.rs
index 62d884f2e..cf8e8792e 100644
--- a/xtask/src/path.rs
+++ b/xtask/src/path.rs
@@ -11,8 +11,16 @@ pub fn book_gen() -> PathBuf {
     project_root().join("book/src/generated/")
 }
 
+pub fn runtime() -> PathBuf {
+    project_root().join("runtime")
+}
+
 pub fn ts_queries() -> PathBuf {
-    project_root().join("runtime/queries")
+    runtime().join("queries")
+}
+
+pub fn themes() -> PathBuf {
+    runtime().join("themes")
 }
 
 pub fn lang_config() -> PathBuf {
diff --git a/xtask/src/theme_check.rs b/xtask/src/theme_check.rs
new file mode 100644
index 000000000..a2719ede8
--- /dev/null
+++ b/xtask/src/theme_check.rs
@@ -0,0 +1,33 @@
+use helix_view::theme::Loader;
+
+use crate::{path, DynError};
+
+pub fn theme_check() -> Result<(), DynError> {
+    let theme_names = [
+        vec!["default".to_string(), "base16_default".to_string()],
+        Loader::read_names(&path::themes()),
+    ]
+    .concat();
+    let loader = Loader::new(&[path::runtime()]);
+    let mut errors_present = false;
+
+    for name in theme_names {
+        let (_, warnings) = loader.load_with_warnings(&name).unwrap();
+
+        if !warnings.is_empty() {
+            errors_present = true;
+            println!("Theme '{name}' loaded with errors:");
+            for warning in warnings {
+                println!("\t* {}", warning);
+            }
+        }
+    }
+
+    match errors_present {
+        true => Err("Errors found when loading bundled themes".into()),
+        false => {
+            println!("Theme check successful!");
+            Ok(())
+        }
+    }
+}