Unnamed repository; edit this file 'description' to name the repository.
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
| -rw-r--r-- | .github/workflows/build.yml | 4 | ||||
| -rw-r--r-- | helix-view/src/theme.rs | 116 | ||||
| -rw-r--r-- | runtime/themes/autumn.toml | 6 | ||||
| -rw-r--r-- | runtime/themes/emacs.toml | 3 | ||||
| -rw-r--r-- | runtime/themes/flatwhite.toml | 7 | ||||
| -rw-r--r-- | runtime/themes/kanagawa.toml | 3 | ||||
| -rw-r--r-- | runtime/themes/monokai.toml | 3 | ||||
| -rw-r--r-- | runtime/themes/monokai_aqua.toml | 9 | ||||
| -rw-r--r-- | runtime/themes/zed_onedark.toml | 3 | ||||
| -rw-r--r-- | xtask/src/main.rs | 7 | ||||
| -rw-r--r-- | xtask/src/path.rs | 10 | ||||
| -rw-r--r-- | xtask/src/theme_check.rs | 33 |
12 files changed, 143 insertions, 61 deletions
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 93fcb981..c9f198d0 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/[email protected] @@ -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 4acc5664..9dc32644 100644 --- a/helix-view/src/theme.rs +++ b/helix-view/src/theme.rs @@ -53,20 +53,34 @@ impl Loader { /// 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 @@ impl Loader { 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 @@ impl Loader { }) .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 @@ impl<'de> Deserialize<'de> for Theme { 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 @@ impl Theme { .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 @@ impl ThemePalette { 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 @@ impl ThemePalette { } } - 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 @@ impl ThemePalette { 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 @@ impl ThemePalette { "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 @@ impl ThemePalette { } 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 @@ impl ThemePalette { } } } - _ => 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 7e9b3a7e..d7d9c00b 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 c60afe99..dfd55e07 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 cc8f536c..1da20350 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 dabece68..8d5a80ee 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 b0faac15..22e6d063 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 0cb9300d..b818a09e 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 74db8f75..160320de 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 aba5c449..fcb462f2 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -2,6 +2,7 @@ mod docgen; 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 mod tasks { 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 62d884f2..cf8e8792 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 00000000..a2719ede --- /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(()) + } + } +} |