From b75ed0877d930ed3ba54f26f97ae8803e5a7fae9 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Sun, 4 Oct 2026 18:37:37 +0700 Subject: [PATCH] M4.29: object ids and names survive the SVG round trip Layer Names writes readable ids plus data-name with the original name whenever the id differs; Minimal writes only the ids definitions need; Unique prefixes every id and CSS class with a content hash, so SVGs inlined in one page never collide. Import names objects and layers from data-name, inkscape:label, serif:id or aria-label. Co-Authored-By: Claude Opus 5.5 --- crates/svg/src/export.rs | 8 ++- crates/svg/src/import.rs | 41 +++++++++++++- crates/svg/tests/ids.rs | 104 ++++++++++++++++++++++++++++++++++ crates/svg/tests/options.rs | 77 +++++++++++++++++++++++++ crates/svg/tests/roundtrip.rs | 5 +- 5 files changed, 229 insertions(+), 6 deletions(-) create mode 100644 crates/svg/tests/ids.rs create mode 100644 crates/svg/tests/options.rs diff --git a/crates/svg/src/export.rs b/crates/svg/src/export.rs index 27d181d1..a67b3e3f 100644 --- a/crates/svg/src/export.rs +++ b/crates/svg/src/export.rs @@ -283,11 +283,17 @@ impl Writer<'_> { self.names.insert(id, cand); } } + /// ` id="…"` for a named object, plus ` data-name="…"` with the name itself when the id had to + /// differ from it (spaces, punctuation, duplicates, the unique prefix). fn id_attr(&self, n: &Node) -> String { if self.anonymous { return String::new(); } - self.names.get(&n.id).map(|id| format!(" id=\"{}\"", xml_escape(id))).unwrap_or_default() + let Some(id) = self.names.get(&n.id) else { return String::new() }; + match n.name.as_deref() { + Some(name) if name != id => format!(" id=\"{}\" data-name=\"{}\"", xml_escape(id), xml_escape(name)), + _ => format!(" id=\"{}\"", xml_escape(id)), + } } fn matrix(&self, m: Affine) -> String { let c = m.as_coeffs(); diff --git a/crates/svg/src/import.rs b/crates/svg/src/import.rs index f022eec8..1ecdb352 100644 --- a/crates/svg/src/import.rs +++ b/crates/svg/src/import.rs @@ -33,7 +33,16 @@ pub(crate) fn import(svg: &str) -> Result<(Document, Vec), SvgError> { let (kx, ky) = units.k; let mut doc = Document::new(size.width() as f64 * kx, size.height() as f64 * ky); doc.units = units.unit; - let mut im = Importer { doc, warnings, mask_flags: mask_flags(&xml), links, slots, patterns: HashMap::new(), midpoints: midpoint_stops(&xml) }; + let mut im = Importer { + doc, + warnings, + mask_flags: mask_flags(&xml), + links, + slots, + patterns: HashMap::new(), + midpoints: midpoint_stops(&xml), + labels: labels(&xml), + }; // usvg wraps everything in an id-less group carrying the viewBox transform when needed. let mut top = tree.root(); @@ -69,7 +78,8 @@ pub(crate) fn import(svg: &str) -> Result<(Document, Vec), SvgError> { children.extend(im.children(g, base * aff(g.transform()))); let id = im.doc.alloc_id(); let preset = (im.doc.layers.len() % vectorcraft_doc::LAYER_COLORS.len()) as u8; - let mut l = Node::layer(id, g.id(), LayerColor::Preset(preset)); + let name = im.name_of(g.id()).to_string(); + let mut l = Node::layer(id, &name, LayerColor::Preset(preset)); if let Some(ch) = l.children_mut() { *ch = children; } @@ -142,6 +152,8 @@ struct Importer { patterns: HashMap, /// Midpoint stops by gradient id (see [`midpoint_stops`]). midpoints: HashMap>>, + /// Object names by element id (see [`labels`]). + labels: HashMap, } /// The stops our export added for midpoints (`data-vc-midpoint`), by gradient id: for each stop, @@ -246,6 +258,24 @@ fn link(n: &mut Node, url: &str) { } } +/// The names apps keep beside element ids, by id: `data-name`, `inkscape:label`, `serif:id` or +/// `aria-label` (the first one present). +fn labels(xml: &roxmltree::Document) -> HashMap { + const INKSCAPE: &str = "http://www.inkscape.org/namespaces/inkscape"; + const SERIF: &str = "http://www.serif.com/"; + xml.descendants() + .filter_map(|n| { + let id = n.attribute("id").filter(|id| !id.is_empty())?; + let name = n + .attribute("data-name") + .or_else(|| n.attribute((INKSCAPE, "label"))) + .or_else(|| n.attribute((SERIF, "id"))) + .or_else(|| n.attribute("aria-label"))?; + Some((id.to_string(), name.to_string())) + }) + .collect() +} + fn aff(t: usvg::Transform) -> Affine { Affine::new([t.sx as f64, t.ky as f64, t.kx as f64, t.sy as f64, t.tx as f64, t.ty as f64]) } @@ -325,11 +355,16 @@ impl Importer { gradient_stops(g, self.midpoints.get(g.id()).map(Vec::as_slice)) } + /// The object name for an element id: its label (see [`labels`]), else the id itself. + fn name_of<'a>(&'a self, id: &'a str) -> &'a str { + self.labels.get(id).map_or(id, String::as_str) + } + fn named(&mut self, id: &str, kind: NodeKind) -> Node { let nid = self.doc.alloc_id(); let mut n = Node::new(nid, kind); if !id.is_empty() { - n.name = Some(id.to_string()); + n.name = Some(self.name_of(id).to_string()); } n } diff --git a/crates/svg/tests/ids.rs b/crates/svg/tests/ids.rs new file mode 100644 index 00000000..affd496e --- /dev/null +++ b/crates/svg/tests/ids.rs @@ -0,0 +1,104 @@ +//! Object IDs: names survive the round trip (Layer Names), Minimal writes only referenced ids, +//! Unique ids never collide, and other apps' label attributes become object names. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::collections::HashSet; + +use vectorcraft_color::{Color, Gradient, GradientPaint, Paint}; +use vectorcraft_doc::{Appearance, Document, Node}; +use vectorcraft_geom::{Rect, shapes}; +use vectorcraft_svg::{ExportOptions, ObjectIds, Styling, export, import}; + +/// A document with a named rectangle, an unnamed gradient-filled one and a second layer. +fn doc(title: &str, shift: f64) -> Document { + let mut d = Document::new(200.0, 200.0); + d.title = title.into(); + let l = d.layers[0].id; + let mut a = Node::path(d.alloc_id(), shapes::rectangle(Rect::new(10.0 + shift, 10.0, 60.0, 60.0)), Appearance::default_art()); + a.name = Some("Logo mark (v2)".into()); + d.insert(Some(l), usize::MAX, a).unwrap(); + let g = Paint::Gradient(Box::new(GradientPaint::new(Gradient::default()))); + let b = Node::path(d.alloc_id(), shapes::rectangle(Rect::new(80.0, 10.0, 120.0, 60.0)), Appearance::basic(g, Paint::None, 0.0)); + d.insert(Some(l), usize::MAX, b).unwrap(); + d.add_layer(Some("Top & Front")); + let top = d.layers[1].id; + let c = Node::path( + d.alloc_id(), + shapes::ellipse(Rect::new(0.0, 100.0, 50.0, 150.0)), + Appearance::basic(Paint::solid(Color::BLACK), Paint::None, 0.0), + ); + d.insert(Some(top), usize::MAX, c).unwrap(); + d +} + +/// Every `id="…"` value. +fn ids(svg: &str) -> Vec { + svg.split(" id=\"").skip(1).map(|s| s[..s.find('"').unwrap()].to_string()).collect() +} + +fn names(d: &Document) -> Vec { + let mut v = vec![]; + d.walk(|n| v.extend(n.name.clone())); + v +} + +#[test] +fn layer_names_round_trip_exactly() { + let d = doc("a", 0.0); + let s = export(&d, &ExportOptions::default()); + assert!(s.contains("id=\"Layer_1\" data-name=\"Layer 1\""), "{s}"); + assert!(s.contains("data-name=\"Top & Front\""), "{s}"); + let back = import(&s).unwrap(); + assert_eq!(back.layers.iter().map(|l| l.name.clone().unwrap()).collect::>(), ["Layer 1", "Top & Front"]); + assert!(names(&back).contains(&"Logo mark (v2)".to_string()), "{:?}", names(&back)); + // An id that is the name already gets no data-name. + let mut d = Document::new(10.0, 10.0); + std::sync::Arc::make_mut(&mut d.layers[0]).name = Some("Layer_1".into()); + let s = export(&d, &ExportOptions::default()); + assert!(s.contains(""), "{s}"); +} + +#[test] +fn minimal_writes_only_referenced_ids() { + let s = export(&doc("a", 0.0), &ExportOptions { object_ids: ObjectIds::Minimal, ..Default::default() }); + let ids = ids(&s); + assert!(!ids.is_empty(), "the gradient keeps its id"); + for id in &ids { + assert!(s.contains(&format!("#{id})")) || s.contains(&format!("\"#{id}\"")), "`{id}` is never referenced:\n{s}"); + } + assert!(!s.contains("data-name")); +} + +#[test] +fn unique_ids_never_collide() { + let opts = ExportOptions { object_ids: ObjectIds::Unique, styling: Styling::InternalCss, ..Default::default() }; + let a = export(&doc("a", 0.0), &opts); + let b = export(&doc("b", 5.0), &opts); + let (ia, ib) = (ids(&a), ids(&b)); + assert_eq!(ia.len(), ia.iter().collect::>().len(), "unique within a document"); + assert!(ia.iter().all(|i| !ib.contains(i)), "{ia:?} vs {ib:?}"); + // CSS class names are page-global too. + let classes = |s: &str| s.split("class=\"").skip(1).map(|s| s[..s.find('"').unwrap()].to_string()).collect::>(); + assert!(classes(&a).is_disjoint(&classes(&b))); + // The same document gets the same ids, and the names still come back. + assert_eq!(a, export(&doc("a", 0.0), &opts)); + let back = import(&a).unwrap(); + assert_eq!(back.layers[0].name.as_deref(), Some("Layer 1")); + assert!(names(&back).contains(&"Logo mark (v2)".to_string())); +} + +#[test] +fn label_attributes_name_objects() { + let svg = r##" + + + + + + + "##; + let d = import(svg).unwrap(); + assert_eq!(d.layers[0].name.as_deref(), Some("Background")); + assert_eq!(names(&d), ["Background", "Sky box", "Sun disc", "Badge", "plain"]); +} diff --git a/crates/svg/tests/options.rs b/crates/svg/tests/options.rs new file mode 100644 index 00000000..26d95c92 --- /dev/null +++ b/crates/svg/tests/options.rs @@ -0,0 +1,77 @@ +//! SVG Options across each other: every styling, id mode and tspan mode reads back, and linked +//! images are named after their bytes. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::sync::Arc; + +use vectorcraft_color::{Color, Paint}; +use vectorcraft_doc::{Appearance, AppearanceItem, CharStyle, Document, ImageBlob, ImageObject, Node, NodeKind, TextKind, TextObject, TextRun}; +use vectorcraft_geom::{Affine, Point, Rect, shapes}; +use vectorcraft_svg::{ExportOptions, ImageMode, ObjectIds, Styling, export, export_full, import}; + +/// A red rectangle with an awkward name, point type and area type of two paragraphs each. +fn doc() -> Document { + let mut d = Document::new(300.0, 200.0); + let l = d.layers[0].id; + let red = Appearance::basic(Paint::solid(Color::rgb8(255, 0, 0)), Paint::solid(Color::BLACK), 2.0); + let mut a = Node::path(d.alloc_id(), shapes::rectangle(Rect::new(10.0, 10.0, 60.0, 60.0)), red); + a.name = Some("A \"quoted\" & 100%".into()); + d.insert(Some(l), usize::MAX, a).unwrap(); + let st = CharStyle { size: 12.0, font_family: "Source Sans 3".into(), fill: Paint::solid(Color::BLACK), ..CharStyle::default() }; + let mut t = TextObject::point(Point::new(20.0, 120.0), "", st.clone()); + t.runs = vec![TextRun { text: "one two\nthree".into(), style: st.clone() }]; + let i = d.alloc_id(); + d.insert(Some(l), usize::MAX, Node::new(i, NodeKind::Text(Box::new(t.clone())))).unwrap(); + t.kind = TextKind::Area { frame: shapes::rectangle(Rect::new(0.0, 0.0, 200.0, 60.0)) }; + t.xf = Affine::translate((20.0, 140.0)); + t.runs = vec![TextRun { text: "para one\npara two".into(), style: st }]; + let i = d.alloc_id(); + d.insert(Some(l), usize::MAX, Node::new(i, NodeKind::Text(Box::new(t)))).unwrap(); + d +} + +#[test] +fn every_combination_reads_back() { + let d = doc(); + for styling in [Styling::PresentationAttributes, Styling::InlineStyle, Styling::StyleEntities, Styling::InternalCss] { + for object_ids in [ObjectIds::LayerNames, ObjectIds::Minimal, ObjectIds::Unique] { + for (fewer_tspans, minify) in [(false, false), (true, true)] { + let o = ExportOptions { styling, object_ids, fewer_tspans, minify, ..Default::default() }; + let s = export(&d, &o); + let back = import(&s).unwrap_or_else(|e| panic!("{o:?}: {e}\n{s}")); + let mut red = false; + back.walk(|n| { + red |= n.appearance.items.iter().any(|i| matches!(i, AppearanceItem::Fill(f) if f.paint == Paint::solid(Color::rgb8(255, 0, 0)))); + }); + assert!(red, "{o:?}: the fill came back\n{s}"); + // Paragraph breaks are new lines, never characters in a line. + assert!(!s.contains("\n") && !s.contains("two\n"), "{o:?}\n{s}"); + } + } + } +} + +/// A document with one embedded image under `key`. +fn image_doc(key: &str, bytes: Vec) -> Document { + let mut d = Document::new(100.0, 100.0); + d.images.insert(key.into(), ImageBlob { mime: "image/png".into(), bytes: Arc::new(bytes) }); + let l = d.layers[0].id; + let im = ImageObject { key: key.into(), width: 2, height: 2, xf: Affine::IDENTITY, link: None }; + let n = Node::new(d.alloc_id(), NodeKind::Image(im)); + d.insert(Some(l), usize::MAX, n).unwrap(); + d +} + +#[test] +fn linked_images_are_named_after_their_bytes() { + let link = ExportOptions { images: ImageMode::Link, ..Default::default() }; + // Keys such as "raster-1" repeat across documents: two exports into one folder must not clash. + let a = export_full(&image_doc("raster-1", vec![1, 2, 3]), &link, None); + let b = export_full(&image_doc("raster-1", vec![4, 5, 6]), &link, None); + assert_eq!((a.linked.len(), b.linked.len()), (1, 1)); + assert_ne!(a.linked[0].name, b.linked[0].name); + assert!(a.linked[0].name.ends_with(".png") && a.svg.contains(&format!("href=\"{}\"", a.linked[0].name)), "{}", a.svg); + // The same bytes under another key are the same file. + assert_eq!(export_full(&image_doc("other", vec![1, 2, 3]), &link, None).linked[0].name, a.linked[0].name); +} diff --git a/crates/svg/tests/roundtrip.rs b/crates/svg/tests/roundtrip.rs index c81a0cb7..3ed604fa 100644 --- a/crates/svg/tests/roundtrip.rs +++ b/crates/svg/tests/roundtrip.rs @@ -91,10 +91,11 @@ fn groups_and_names_roundtrip() { assert!(s.contains("id=\"My_Group\"") && s.contains("id=\"Box_B\"") && s.contains("id=\"Layer_1\""), "{s}"); let r = roundtrip(&d); assert_eq!(r.node_count(), d.node_count()); - assert_eq!(r.layers[0].name.as_deref(), Some("Layer_1")); + // data-name carries the names back exactly. + assert_eq!(r.layers[0].name.as_deref(), Some("Layer 1")); let a = art(&r); assert!(matches!(a[0].kind, NodeKind::Group { clip: false, .. })); - assert_eq!(a[0].name.as_deref(), Some("My_Group")); + assert_eq!(a[0].name.as_deref(), Some("My Group")); assert!(close_rect(a[0].geometric_bounds().unwrap(), Rect::new(0.0, 0.0, 30.0, 10.0), 0.01)); }