From 38445aef47b536d67ac8b953fbfee4413356e4d5 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 29 Sep 2026 12:18:50 +0200 Subject: [PATCH] fix(icons): repair the attribute rewrite that stopped every icon rendering set_root_attribute kept the closing quote of the value it replaced and appended another, so an Iconify source carrying width="1em" came out as width="84"" and did not parse. Every icon has rendered nothing since #390, announced only by a warning on stderr under a zero exit code. The existing tests missed it because they assert contains("width=\"84\"") on the output, which is true of the broken string too. The new unit test parses the rewritten SVG with usvg instead, and the render tests go through a real scenario to pixel assertions -- 0 coloured pixels on the revert. render --frame did not preload at all, so an unresolvable icon warned and exited 0 there while still, sheet and the encoders exited 1. It now goes through preload_scenario_assets like the other single-frame paths. 674 files in a real cache still carry the old {slug}-{colour}-{w}x{h} name. They are recoloured copies, not sources, but the colour is in the filename, so the substitution is reversible: on a source miss the cache is scanned for one and currentColor is restored from it rather than refetching. That is what lets an offline render work from a cache filled before the rework. Parsing is anchored at the right of the name, since an icon's own name can contain a dash. --- .../src/engine/renderer/assets.rs | 146 +++++++++++++++++- crates/rustmotion/src/cli/commands/render.rs | 4 +- crates/rustmotion/src/tests.rs | 97 ++++++++++++ 3 files changed, 238 insertions(+), 9 deletions(-) diff --git a/crates/rustmotion-core/src/engine/renderer/assets.rs b/crates/rustmotion-core/src/engine/renderer/assets.rs index 8270908..e915d09 100644 --- a/crates/rustmotion-core/src/engine/renderer/assets.rs +++ b/crates/rustmotion-core/src/engine/renderer/assets.rs @@ -67,7 +67,7 @@ pub fn icon_cache_dir() -> PathBuf { base.join("rustmotion").join("icons") } -fn icon_source_cache_file(cache_dir: &Path, icon: &str) -> PathBuf { +pub fn icon_source_cache_file(cache_dir: &Path, icon: &str) -> PathBuf { cache_dir.join(format!("{}.svg", icon.replace(':', "_"))) } @@ -89,7 +89,7 @@ fn set_root_attribute(svg: &str, attribute: &str, value: &str) -> String { "{}{}\"{}", &tag[..value_start], value, - &tag[value_start + value_len..] + &tag[value_start + value_len + 1..] ), None => tag.to_string(), } @@ -112,6 +112,42 @@ fn recolour_and_resize(source: &[u8], hex_color: &str, width: u32, height: u32) set_root_attribute(&sized, "height", &height.to_string()).into_bytes() } +fn legacy_icon_source(cache_dir: &Path, icon: &str) -> Option> { + let slug = icon.replace(':', "_"); + let prefix = format!("{slug}-"); + let entries = std::fs::read_dir(cache_dir).ok()?; + for entry in entries.flatten() { + let name = entry.file_name(); + let name = name.to_str()?; + let Some(rest) = name.strip_prefix(&prefix) else { + continue; + }; + let Some(rest) = rest.strip_suffix(".svg") else { + continue; + }; + let Some((colour, size)) = rest.split_once('-') else { + continue; + }; + if colour.len() != 6 || !colour.chars().all(|c| c.is_ascii_hexdigit()) { + continue; + } + let Some((w, h)) = size.split_once('x') else { + continue; + }; + if w.parse::().is_err() || h.parse::().is_err() { + continue; + } + let baked = std::fs::read_to_string(entry.path()).ok()?; + let restored = baked + .replace(&format!("#{}", colour.to_lowercase()), "currentColor") + .replace(&format!("#{}", colour.to_uppercase()), "currentColor"); + if restored.contains("currentColor") { + return Some(restored.into_bytes()); + } + } + None +} + const ICON_FETCH_ATTEMPTS: u32 = 4; fn status_is_worth_retrying(error: &ureq::Error) -> bool { @@ -177,13 +213,19 @@ pub fn fetch_icon_svg_in( let source = match std::fs::read(&source_file) { Ok(data) if !data.is_empty() => data, - _ => { - let fetched = fetch_icon_source(icon, prefix, name)?; - if std::fs::create_dir_all(cache_dir).is_ok() { - let _ = std::fs::write(&source_file, &fetched); + _ => match legacy_icon_source(cache_dir, icon) { + Some(migrated) => { + let _ = std::fs::write(&source_file, &migrated); + migrated } - fetched - } + None => { + let fetched = fetch_icon_source(icon, prefix, name)?; + if std::fs::create_dir_all(cache_dir).is_ok() { + let _ = std::fs::write(&source_file, &fetched); + } + fetched + } + }, }; Ok(recolour_and_resize(&source, &hex_color, width, height)) @@ -741,6 +783,94 @@ mod icon_source_cache_tests { ); } + fn a_cache_dir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "rm_legacy_icons_{}_{}_{tag}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).expect("cache dir"); + dir + } + + #[test] + fn a_file_left_by_the_old_naming_is_migrated_instead_of_refetched() { + let dir = a_cache_dir("migrate"); + let baked = LUCIDE_CHECK.replace("currentColor", "#3B6FD4"); + std::fs::write(dir.join("lucide_check-3b6fd4-26x26.svg"), &baked).expect("seed"); + + let migrated = legacy_icon_source(&dir, "lucide:check") + .expect("674 of these sit in a real cache; an offline render must be able to use one"); + let text = String::from_utf8(migrated).unwrap(); + assert!( + text.contains("currentColor"), + "the baked colour has to come back out, or the icon can never be recoloured: {text}" + ); + assert!(!text.contains("3B6FD4"), "got {text}"); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn migration_survives_an_icon_whose_own_name_contains_a_dash() { + let dir = a_cache_dir("dashed"); + let baked = LUCIDE_CHECK.replace("currentColor", "#8b5cf6"); + std::fs::write(dir.join("lucide_arrow-down-8b5cf6-176x176.svg"), &baked).expect("seed"); + assert!( + legacy_icon_source(&dir, "lucide:arrow-down").is_some(), + "the colour and size have to be parsed from the right, not from the first dash" + ); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn an_unrelated_file_is_not_mistaken_for_a_legacy_entry() { + let dir = a_cache_dir("unrelated"); + std::fs::write(dir.join("lucide_check-notes.svg"), LUCIDE_CHECK).expect("seed"); + std::fs::write(dir.join("lucide_checkmark.svg"), LUCIDE_CHECK).expect("seed"); + assert!( + legacy_icon_source(&dir, "lucide:check").is_none(), + "only a name ending in -{{6 hex}}-{{w}}x{{h}} is a legacy entry" + ); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn the_rewritten_icon_still_parses_as_svg() { + let out = recolour_and_resize(LUCIDE_CHECK.as_bytes(), "2563eb", 84, 84); + let text = String::from_utf8(out.clone()).unwrap(); + assert!( + !text.contains("\"\""), + "the rewrite must not leave the previous value's closing quote behind: {text}" + ); + usvg::Tree::from_data(&out, &usvg::Options::default()).unwrap_or_else(|e| { + panic!( + "a rewritten icon has to parse, or nothing renders and the only sign is a \ + warning on stderr: {e}\n{text}" + ) + }); + } + + #[test] + fn rewriting_an_attribute_that_already_has_a_value_replaces_it_exactly_once() { + let source = r#""#; + let out = String::from_utf8(recolour_and_resize(source.as_bytes(), "000000", 84, 96)) + .expect("utf8"); + assert_eq!( + out.matches("width=").count(), + 1, + "one width attribute, not two: {out}" + ); + assert!(out.contains(r#"width="84""#), "got {out}"); + assert!(out.contains(r#"height="96""#), "got {out}"); + assert!( + !out.contains("1em"), + "the 1em Iconify ships must be gone: {out}" + ); + } + #[test] fn recolouring_substitutes_current_color_everywhere() { let out = recolour_and_resize(LUCIDE_CHECK.as_bytes(), "2a2f6b", 84, 84); diff --git a/crates/rustmotion/src/cli/commands/render.rs b/crates/rustmotion/src/cli/commands/render.rs index fa927eb..c258164 100644 --- a/crates/rustmotion/src/cli/commands/render.rs +++ b/crates/rustmotion/src/cli/commands/render.rs @@ -62,7 +62,9 @@ pub fn cmd_render( let start = std::time::Instant::now(); - if !scenario.fonts.is_empty() { + if frame.is_some() { + engine::preload::preload_scenario_assets(&scenario)?; + } else if !scenario.fonts.is_empty() { engine::renderer::load_custom_fonts(&scenario.fonts); } diff --git a/crates/rustmotion/src/tests.rs b/crates/rustmotion/src/tests.rs index 1cdf000..e1dc2dc 100644 --- a/crates/rustmotion/src/tests.rs +++ b/crates/rustmotion/src/tests.rs @@ -3822,3 +3822,100 @@ mod still_runs_the_audio_analysis_tests { ); } } + +#[cfg(test)] +mod icon_render_tests { + use crate::encode::video::{build_frame_tasks, render_frame_task, FrameTask}; + use crate::loader::load_scenario_from_source; + use rustmotion_core::engine::renderer::{icon_cache_dir, icon_source_cache_file}; + + const W: usize = 160; + const H: usize = 160; + + const A_BAR: &str = r#""#; + + struct SeededIcon { + name: String, + file: std::path::PathBuf, + } + + impl SeededIcon { + fn new(tag: &str) -> Self { + let name = format!("rmtest{tag}:bar"); + let dir = icon_cache_dir(); + std::fs::create_dir_all(&dir).expect("cache dir"); + let file = icon_source_cache_file(&dir, &name); + std::fs::write(&file, A_BAR).expect("seed the icon so the test needs no network"); + Self { name, file } + } + } + + impl Drop for SeededIcon { + fn drop(&mut self) { + std::fs::remove_file(&self.file).ok(); + } + } + + fn render_first_frame(icon: &str) -> Vec { + let json = format!( + r##"{{ + "version": "1.0", + "video": {{ "width": {W}, "height": {H}, "fps": 30, "background": "#FFFFFF" }}, + "scenes": [{{ "duration": 1.0, "children": [ + {{ "type": "icon", "icon": "{icon}", + "style": {{ "position": "absolute", "left": 38, "top": 38, + "width": 84, "height": 84, "color": "#2563EB" }} }} + ]}}] + }}"## + ); + let scenario = load_scenario_from_source(None, Some(&json)).expect("load"); + crate::engine::preload::preload_scenario_assets(&scenario).expect("preload"); + let tasks = build_frame_tasks(&scenario); + let task = tasks + .iter() + .find(|t| matches!(t, FrameTask::Normal { .. })) + .expect("a normal frame"); + render_frame_task(&scenario.video, &scenario, task).expect("render") + } + + fn blue_pixels(frame: &[u8]) -> usize { + (0..W * H) + .filter(|i| { + let p = i * 4; + frame[p] < 120 && frame[p + 2] > 150 + }) + .count() + } + + #[test] + fn an_icon_actually_paints_its_glyph_into_the_frame() { + let seeded = SeededIcon::new("paint"); + let frame = render_first_frame(&seeded.name); + let painted = blue_pixels(&frame); + assert!( + painted > 100, + "the icon painted {painted} coloured pixels. Rewriting its width left a stray quote, \ + so the SVG did not parse, so nothing was drawn — and the only sign was a warning on \ + stderr with a zero exit code" + ); + } + + #[test] + fn the_icon_takes_the_colour_the_scenario_asks_for() { + let seeded = SeededIcon::new("colour"); + let frame = render_first_frame(&seeded.name); + let mut found = None; + for i in 0..W * H { + let p = i * 4; + if frame[p] < 120 && frame[p + 2] > 150 && frame[p + 3] > 250 { + found = Some((frame[p], frame[p + 1], frame[p + 2])); + break; + } + } + let (r, g, b) = found.expect("an opaque coloured pixel"); + assert!( + r < 80 && g < 140 && b > 200, + "currentColor must become the declared #2563EB, got ({r}, {g}, {b})" + ); + } +}