Skip to content

Commit 82dd963

Browse files
sarsapar1llasarsapar1lla
authored
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
1 parent 70bbc9d commit 82dd963

File tree

12 files changed

+143
-61
lines changed

12 files changed

+143
-61
lines changed

‎.github/workflows/build.yml

+4
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ jobs:
1616
steps:
1717
- name: Checkout sources
1818
uses: actions/checkout@v4
19+
1920
- name: Install stable toolchain
2021
uses: dtolnay/rust-toolchain@1.70
2122

@@ -107,6 +108,9 @@ jobs:
107108
- name: Validate queries
108109
run: cargo xtask query-check
109110

111+
- name: Validate themes
112+
run: cargo xtask theme-check
113+
110114
- name: Generate docs
111115
run: cargo xtask docgen
112116

‎helix-view/src/theme.rs

+67-49
Original file line numberDiff line numberDiff line change
@@ -53,20 +53,34 @@ impl Loader {
5353

5454
/// Loads a theme searching directories in priority order.
5555
pub fn load(&self, name: &str) -> Result<Theme> {
56+
let (theme, warnings) = self.load_with_warnings(name)?;
57+
58+
for warning in warnings {
59+
warn!("Theme '{}': {}", name, warning);
60+
}
61+
62+
Ok(theme)
63+
}
64+
65+
/// Loads a theme searching directories in priority order, returning any warnings
66+
pub fn load_with_warnings(&self, name: &str) -> Result<(Theme, Vec<String>)> {
5667
if name == "default" {
57-
return Ok(self.default());
68+
return Ok((self.default(), Vec::new()));
5869
}
5970
if name == "base16_default" {
60-
return Ok(self.base16_default());
71+
return Ok((self.base16_default(), Vec::new()));
6172
}
6273

6374
let mut visited_paths = HashSet::new();
64-
let theme = self.load_theme(name, &mut visited_paths).map(Theme::from)?;
75+
let (theme, warnings) = self
76+
.load_theme(name, &mut visited_paths)
77+
.map(Theme::from_toml)?;
6578

66-
Ok(Theme {
79+
let theme = Theme {
6780
name: name.into(),
6881
..theme
69-
})
82+
};
83+
Ok((theme, warnings))
7084
}
7185

7286
/// Recursively load a theme, merging with any inherited parent themes.
@@ -87,10 +101,7 @@ impl Loader {
87101

88102
let theme_toml = if let Some(parent_theme_name) = inherits {
89103
let parent_theme_name = parent_theme_name.as_str().ok_or_else(|| {
90-
anyhow!(
91-
"Theme: expected 'inherits' to be a string: {}",
92-
parent_theme_name
93-
)
104+
anyhow!("Expected 'inherits' to be a string: {}", parent_theme_name)
94105
})?;
95106

96107
let parent_theme_toml = match parent_theme_name {
@@ -181,9 +192,9 @@ impl Loader {
181192
})
182193
.ok_or_else(|| {
183194
if cycle_found {
184-
anyhow!("Theme: cycle found in inheriting: {}", name)
195+
anyhow!("Cycle found in inheriting: {}", name)
185196
} else {
186-
anyhow!("Theme: file not found for: {}", name)
197+
anyhow!("File not found for: {}", name)
187198
}
188199
})
189200
}
@@ -220,19 +231,11 @@ pub struct Theme {
220231

221232
impl From<Value> for Theme {
222233
fn from(value: Value) -> Self {
223-
if let Value::Table(table) = value {
224-
let (styles, scopes, highlights) = build_theme_values(table);
225-
226-
Self {
227-
styles,
228-
scopes,
229-
highlights,
230-
..Default::default()
231-
}
232-
} else {
233-
warn!("Expected theme TOML value to be a table, found {:?}", value);
234-
Default::default()
234+
let (theme, warnings) = Theme::from_toml(value);
235+
for warning in warnings {
236+
warn!("{}", warning);
235237
}
238+
theme
236239
}
237240
}
238241

@@ -242,31 +245,29 @@ impl<'de> Deserialize<'de> for Theme {
242245
D: Deserializer<'de>,
243246
{
244247
let values = Map::<String, Value>::deserialize(deserializer)?;
245-
246-
let (styles, scopes, highlights) = build_theme_values(values);
247-
248-
Ok(Self {
249-
styles,
250-
scopes,
251-
highlights,
252-
..Default::default()
253-
})
248+
let (theme, warnings) = Theme::from_keys(values);
249+
for warning in warnings {
250+
warn!("{}", warning);
251+
}
252+
Ok(theme)
254253
}
255254
}
256255

257256
fn build_theme_values(
258257
mut values: Map<String, Value>,
259-
) -> (HashMap<String, Style>, Vec<String>, Vec<Style>) {
258+
) -> (HashMap<String, Style>, Vec<String>, Vec<Style>, Vec<String>) {
260259
let mut styles = HashMap::new();
261260
let mut scopes = Vec::new();
262261
let mut highlights = Vec::new();
263262

263+
let mut warnings = Vec::new();
264+
264265
// TODO: alert user of parsing failures in editor
265266
let palette = values
266267
.remove("palette")
267268
.map(|value| {
268269
ThemePalette::try_from(value).unwrap_or_else(|err| {
269-
warn!("{}", err);
270+
warnings.push(err);
270271
ThemePalette::default()
271272
})
272273
})
@@ -279,7 +280,7 @@ fn build_theme_values(
279280
for (name, style_value) in values {
280281
let mut style = Style::default();
281282
if let Err(err) = palette.parse_style(&mut style, style_value) {
282-
warn!("{}", err);
283+
warnings.push(err);
283284
}
284285

285286
// these are used both as UI and as highlights
@@ -288,7 +289,7 @@ fn build_theme_values(
288289
highlights.push(style);
289290
}
290291

291-
(styles, scopes, highlights)
292+
(styles, scopes, highlights, warnings)
292293
}
293294

294295
impl Theme {
@@ -354,6 +355,27 @@ impl Theme {
354355
.all(|color| !matches!(color, Some(Color::Rgb(..))))
355356
})
356357
}
358+
359+
fn from_toml(value: Value) -> (Self, Vec<String>) {
360+
if let Value::Table(table) = value {
361+
Theme::from_keys(table)
362+
} else {
363+
warn!("Expected theme TOML value to be a table, found {:?}", value);
364+
Default::default()
365+
}
366+
}
367+
368+
fn from_keys(toml_keys: Map<String, Value>) -> (Self, Vec<String>) {
369+
let (styles, scopes, highlights, load_errors) = build_theme_values(toml_keys);
370+
371+
let theme = Self {
372+
styles,
373+
scopes,
374+
highlights,
375+
..Default::default()
376+
};
377+
(theme, load_errors)
378+
}
357379
}
358380

359381
struct ThemePalette {
@@ -408,7 +430,7 @@ impl ThemePalette {
408430
if let Ok(index) = s.parse::<u8>() {
409431
return Ok(Color::Indexed(index));
410432
}
411-
Err(format!("Theme: malformed ANSI: {}", s))
433+
Err(format!("Malformed ANSI: {}", s))
412434
}
413435

414436
fn hex_string_to_rgb(s: &str) -> Result<Color, String> {
@@ -422,13 +444,13 @@ impl ThemePalette {
422444
}
423445
}
424446

425-
Err(format!("Theme: malformed hexcode: {}", s))
447+
Err(format!("Malformed hexcode: {}", s))
426448
}
427449

428450
fn parse_value_as_str(value: &Value) -> Result<&str, String> {
429451
value
430452
.as_str()
431-
.ok_or(format!("Theme: unrecognized value: {}", value))
453+
.ok_or(format!("Unrecognized value: {}", value))
432454
}
433455

434456
pub fn parse_color(&self, value: Value) -> Result<Color, String> {
@@ -445,14 +467,14 @@ impl ThemePalette {
445467
value
446468
.as_str()
447469
.and_then(|s| s.parse().ok())
448-
.ok_or(format!("Theme: invalid modifier: {}", value))
470+
.ok_or(format!("Invalid modifier: {}", value))
449471
}
450472

451473
pub fn parse_underline_style(value: &Value) -> Result<UnderlineStyle, String> {
452474
value
453475
.as_str()
454476
.and_then(|s| s.parse().ok())
455-
.ok_or(format!("Theme: invalid underline style: {}", value))
477+
.ok_or(format!("Invalid underline style: {}", value))
456478
}
457479

458480
pub fn parse_style(&self, style: &mut Style, value: Value) -> Result<(), String> {
@@ -462,9 +484,7 @@ impl ThemePalette {
462484
"fg" => *style = style.fg(self.parse_color(value)?),
463485
"bg" => *style = style.bg(self.parse_color(value)?),
464486
"underline" => {
465-
let table = value
466-
.as_table_mut()
467-
.ok_or("Theme: underline must be table")?;
487+
let table = value.as_table_mut().ok_or("Underline must be table")?;
468488
if let Some(value) = table.remove("color") {
469489
*style = style.underline_color(self.parse_color(value)?);
470490
}
@@ -473,13 +493,11 @@ impl ThemePalette {
473493
}
474494

475495
if let Some(attr) = table.keys().next() {
476-
return Err(format!("Theme: invalid underline attribute: {attr}"));
496+
return Err(format!("Invalid underline attribute: {attr}"));
477497
}
478498
}
479499
"modifiers" => {
480-
let modifiers = value
481-
.as_array()
482-
.ok_or("Theme: modifiers should be an array")?;
500+
let modifiers = value.as_array().ok_or("Modifiers should be an array")?;
483501

484502
for modifier in modifiers {
485503
if modifier
@@ -492,7 +510,7 @@ impl ThemePalette {
492510
}
493511
}
494512
}
495-
_ => return Err(format!("Theme: invalid style attribute: {}", name)),
513+
_ => return Err(format!("Invalid style attribute: {}", name)),
496514
}
497515
}
498516
} else {

‎runtime/themes/autumn.toml

+4-2
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,10 @@
6868
"ui.statusline.select" = { fg = "my_gray7", bg = "my_black", modifiers = ["bold"] }
6969
"ui.text.focus" = "my_white1"
7070
"ui.text" = "my_white1"
71-
"ui.virtual.inlay-hint" = { fg = "my_gray4", bg="my_black", modifiers = ["normal"] }
72-
"ui.virtual.inlay-hint.parameter" = { fg = "my_gray4", modifiers = ["normal"] }
71+
# Invalid modifier: "normal". See 'https://github.com/helix-editor/helix/issues/5709'
72+
"ui.virtual.inlay-hint" = { fg = "my_gray4", bg="my_black" } #, modifiers = ["normal"] }
73+
# "ui.virtual.inlay-hint.parameter" = { fg = "my_gray4", modifiers = ["normal"] }
74+
"ui.virtual.inlay-hint.parameter" = "my_gray4"
7375
"ui.virtual.inlay-hint.type" = { fg = "my_gray4", modifiers = ["italic"] }
7476
"ui.virtual.jump-label" = { fg = "my_yellow2", modifiers = ["bold"] }
7577
"ui.virtual.ruler" = { bg = "my_gray1" }

‎runtime/themes/emacs.toml

+2-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,8 @@
6868
"ui.menu.selected" = { fg = "dark_red", bg = "light_blue" }
6969
"ui.selection" = { bg = "lightgoldenrod1" }
7070
"ui.selection.primary" = { bg = "lightgoldenrod2" }
71-
"ui.virtual.whitespace" = "highlight"
71+
# Malformed ANSI: highlight. See 'https://github.com/helix-editor/helix/issues/5709'
72+
# "ui.virtual.whitespace" = "highlight"
7273
"ui.virtual.ruler" = { bg = "gray95" }
7374
"ui.virtual.inlay-hint" = { fg = "gray75" }
7475
"ui.cursorline.primary" = { bg = "darkseagreen2" }

‎runtime/themes/flatwhite.toml

+5-2
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,11 @@
6161
"ui.virtual" = { fg = "base5", bg = "base6" }
6262
"ui.virtual.whitespace" = { fg = "base5" }
6363
"ui.virtual.ruler" = { bg = "base6" }
64-
"ui.virtual.inlay-hint" = { fg = "base4", modifiers = ["normal"] }
65-
"ui.virtual.inlay-hint.parameter" = { fg = "base3", modifiers = ["normal"] }
64+
# Invalid modifier: "normal". See 'https://github.com/helix-editor/helix/issues/5709'
65+
# "ui.virtual.inlay-hint" = { fg = "base4", modifiers = ["normal"] }
66+
# "ui.virtual.inlay-hint.parameter" = { fg = "base3", modifiers = ["normal"] }
67+
"ui.virtual.inlay-hint" = "base4"
68+
"ui.virtual.inlay-hint.parameter" = "base3"
6669
"ui.virtual.inlay-hint.type" = { fg = "base3", modifiers = ["italic"] }
6770

6871
"ui.linenr" = { bg = "base6" }

‎runtime/themes/kanagawa.toml

+2-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@
2525
"ui.statusline.normal" = { fg = "sumiInk0", bg = "crystalBlue", modifiers = ["bold"] }
2626
"ui.statusline.insert" = { fg = "sumiInk0", bg = "autumnGreen", modifiers = ["bold"] }
2727
"ui.statusline.select" = { fg = "sumiInk0", bg = "oniViolet", modifiers = ["bold"] }
28-
"ui.statusline.separator" = { fg = "", bg = "" }
28+
# Malformed ANSI: "". See 'https://github.com/helix-editor/helix/issues/5709'
29+
# "ui.statusline.separator" = { fg = "", bg = "" }
2930

3031
"ui.bufferline" = { fg = "fujiGray", bg = "sumiInk0" }
3132
"ui.bufferline.active" = { fg = "oldWhite", bg = "sumiInk0" }

‎runtime/themes/monokai.toml

+2-1
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,8 @@
7979
"ui.statusline" = { fg = "active_text", bg = "#414339" }
8080
"ui.statusline.inactive" = { fg = "active_text", bg = "#75715e" }
8181

82-
"ui.bufferline" = { fg = "grey2", bg = "bg3" }
82+
# Malformed ANSI: grey2, bg3. See 'https://github.com/helix-editor/helix/issues/5709'
83+
# "ui.bufferline" = { fg = "grey2", bg = "bg3" }
8384
"ui.bufferline.active" = { fg = "active_text", bg = "selection", modifiers = [
8485
"bold",
8586
] }

‎runtime/themes/monokai_aqua.toml

+6-3
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@ inherits = "monokai"
88

99
"type" = { fg = "type", modifiers = ["bold"] }
1010

11-
"ui.statusline.normal" = { fg = "light-black", bg = "cyan" }
12-
"ui.statusline.insert" = { fg = "light-black", bg = "green" }
13-
"ui.statusline.select" = { fg = "light-black", bg = "purple" }
11+
# Malformed ANSI: light-black, purple. See 'https://github.com/helix-editor/helix/issues/5709'
12+
# "ui.statusline.normal" = { fg = "light-black", bg = "cyan" }
13+
"ui.statusline.normal" = { bg = "cyan" }
14+
# "ui.statusline.insert" = { fg = "light-black", bg = "green" }
15+
"ui.statusline.insert" = { bg = "green" }
16+
# "ui.statusline.select" = { fg = "light-black", bg = "purple" }
1417

1518
"ui.virtual.jump-label" = { fg = "cyan", modifiers = ["bold"] }
1619

‎runtime/themes/zed_onedark.toml

+2-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@
6161
"ui.cursor" = { fg = "white", modifiers = ["reversed"] }
6262
"ui.cursor.primary" = { fg = "white", modifiers = ["reversed"] }
6363
"ui.cursor.match" = { fg = "blue", modifiers = ["underlined"] }
64-
"ui.cursor.insert" = { fg = "dark-blue" }
64+
# Malformed ANSI: dark-blue. See 'https://github.com/helix-editor/helix/issues/5709'
65+
# "ui.cursor.insert" = { fg = "dark-blue" }
6566

6667
"ui.selection" = { bg = "faint-gray" }
6768
"ui.selection.primary" = { bg = "#293b5bff" }

‎xtask/src/main.rs

+7
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ mod docgen;
22
mod helpers;
33
mod path;
44
mod querycheck;
5+
mod theme_check;
56

67
use std::{env, error::Error};
78

@@ -11,6 +12,7 @@ pub mod tasks {
1112
use crate::docgen::{lang_features, typable_commands, write};
1213
use crate::docgen::{LANG_SUPPORT_MD_OUTPUT, TYPABLE_COMMANDS_MD_OUTPUT};
1314
use crate::querycheck::query_check;
15+
use crate::theme_check::theme_check;
1416
use crate::DynError;
1517

1618
pub fn docgen() -> Result<(), DynError> {
@@ -23,6 +25,10 @@ pub mod tasks {
2325
query_check()
2426
}
2527

28+
pub fn themecheck() -> Result<(), DynError> {
29+
theme_check()
30+
}
31+
2632
pub fn print_help() {
2733
println!(
2834
"
@@ -43,6 +49,7 @@ fn main() -> Result<(), DynError> {
4349
Some(t) => match t.as_str() {
4450
"docgen" => tasks::docgen()?,
4551
"query-check" => tasks::querycheck()?,
52+
"theme-check" => tasks::themecheck()?,
4653
invalid => return Err(format!("Invalid task name: {}", invalid).into()),
4754
},
4855
};

0 commit comments

Comments
 (0)