Merge pull request #5405 from storytold/fix/sec-svg-name-suffixes

SVG: CSS class names and imported symbol names take linear work

crates/svg/src/css.rs

@@ -4,7 +4,7 @@
 //! here, colours are `#rrggbb` (or `rgba()` where CSS has no separate opacity), gradients take the
 //! stops SVG export writes, and numbers go through [`fmt_num`].
 
-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet};
 
 use serde::{Deserialize, Serialize};
 use vectorcraft_color::{BlendMode, Color, GradientKind, GradientPaint, Paint};
@@ -308,7 +308,7 @@ impl CssSheet {
 /// paragraph's alignment; shadows and glows give `box-shadow` (`text-shadow`), a Gaussian blur
 /// `filter: blur()`. Other art is described as its box (see [`CssOptions::rasterize`]).
 pub fn css_rules(doc: &Document, ids: &[NodeId], opts: &CssOptions) -> CssSheet {
-    let mut w = Rules { doc, opts, sheet: CssSheet::default(), seen: HashSet::new(), names: HashSet::new() };
+    let mut w = Rules { doc, opts, sheet: CssSheet::default(), seen: HashSet::new(), names: HashSet::new(), next_suffix: HashMap::new() };
     for n in ids.iter().filter_map(|id| doc.node(*id)) {
         w.object(n);
     }
@@ -323,6 +323,8 @@ struct Rules<'a> {
     seen: HashSet<NodeId>,
     /// Class names given, lower case.
     names: HashSet<String>,
+    /// The next suffix to try for each base class name, lower case.
+    next_suffix: HashMap<String, usize>,
 }
 
 /// The CSS of one object's art, and why it is approximate.
@@ -367,14 +369,18 @@ impl Rules<'_> {
     /// A class name for `name` (an object's, or its kind's), unique in the sheet.
     fn class_name(&mut self, name: &str) -> String {
         let base = sanitize_id(name).replace('.', "_");
-        let mut cand = base.clone();
-        for i in 2.. {
+        if self.names.insert(base.to_lowercase()) {
+            return base;
+        }
+        // Each base goes on from the suffix it last gave: objects sharing a name take linear work.
+        let next = self.next_suffix.entry(base.to_lowercase()).or_insert(2);
+        loop {
+            let cand = format!("{base}-{next}");
+            *next += 1;
             if self.names.insert(cand.to_lowercase()) {
-                break;
+                return cand;
             }
-            cand = format!("{base}-{i}");
         }
-        cand
     }
 
     fn rule(&mut self, n: &Node) {
@@ -729,4 +735,20 @@ mod tests {
         assert!(family("a(b").starts_with("'a(b'"));
         assert!(family("Arial").starts_with("Arial,"));
     }
+
+    #[test]
+    fn class_names_sharing_a_base_take_the_next_free_suffix() {
+        let doc = Document::new(10.0, 10.0);
+        let opts = CssOptions::default();
+        let mut w =
+            Rules { doc: &doc, opts: &opts, sheet: CssSheet::default(), seen: HashSet::new(), names: HashSet::new(), next_suffix: HashMap::new() };
+        assert_eq!(w.class_name("a-3"), "a-3");
+        let names: Vec<String> = (0..4).map(|_| w.class_name("A")).collect();
+        assert_eq!(names, ["A", "A-2", "A-4", "A-5"]);
+        // Many objects of one name: each suffix is tried once.
+        for _ in 0..20_000 {
+            w.class_name("b");
+        }
+        assert_eq!(w.next_suffix["b"], 20_001);
+    }
 }

crates/svg/src/import.rs

@@ -154,6 +154,9 @@ pub(crate) fn import(svg: &str, opts: &ImportOptions, max_nodes: usize) -> Resul
         hidden: found.hidden,
         uses: found.uses,
         symbols: HashMap::new(),
+        symbol_names: HashSet::new(),
+        symbol_suffixes: HashMap::new(),
+        pattern_suffixes: HashMap::new(),
         blends: fx::blends(&xml, &scan.markers),
         files,
         non_scaling: found.non_scaling,
@@ -304,6 +307,11 @@ struct Importer {
     uses: HashMap<String, String>,
     /// The symbols made so far, by `<symbol>` id.
     symbols: HashMap<String, SymbolMade>,
+    /// The names of the symbols made so far.
+    symbol_names: HashSet<String>,
+    /// Where the suffixes of each symbol and pattern name base go on from (`unique_name`).
+    symbol_suffixes: HashMap<String, usize>,
+    pattern_suffixes: HashMap<String, usize>,
     /// Blend modes of the shadows and glows our export wrote, by filter id ([`fx::blends`]).
     blends: HashMap<String, String>,
     /// The files `<image>` elements link to.

crates/svg/src/import/convert.rs

@@ -15,15 +15,20 @@ pub(super) fn without_ids(n: &Node) -> Node {
     n
 }
 
-/// `base`, or `base 2`, `base 3`… : the first that isn't `taken`.
-pub(super) fn unique_name(base: &str, taken: impl Fn(&str) -> bool) -> String {
-    let mut name = base.to_string();
-    let mut k = 2;
-    while taken(&name) {
-        name = format!("{base} {k}");
-        k += 1;
+/// `base`, or `base 2`, `base 3`… : the first that isn't `taken`. `next` keeps where each base's
+/// suffixes go on from, so names sharing a base take linear work in all.
+pub(super) fn unique_name(base: &str, next: &mut HashMap<String, usize>, taken: impl Fn(&str) -> bool) -> String {
+    if !taken(base) {
+        return base.to_string();
+    }
+    let k = next.entry(base.to_string()).or_insert(2);
+    loop {
+        let name = format!("{base} {k}");
+        *k += 1;
+        if !taken(&name) {
+            return name;
+        }
     }
-    name
 }
 
 /// The stops our export added for midpoints (`data-vc-midpoint`), by gradient id: for each stop,

crates/svg/src/import/importer.rs

@@ -183,7 +183,10 @@ impl Importer {
             Some(made) if whole && made.art == without_ids(&art) && made.reuse.allows(xf) => made.name.clone(),
             Some(_) => return None,
             None => {
-                let name = unique_name(self.name_of(&sym), |n| self.doc.symbols.iter().any(|s| s.name == n));
+                let base = self.name_of(&sym).to_string();
+                let names = &self.symbol_names;
+                let name = unique_name(&base, &mut self.symbol_suffixes, |n| names.contains(n));
+                self.symbol_names.insert(name.clone());
                 let reuse = Reuse::of(&self.doc, &art);
                 let made = SymbolMade { name: name.clone(), art: without_ids(&art), reuse };
                 self.doc.symbols.push(vectorcraft_doc::Symbol { dynamic: false, name: name.clone(), art: Arc::new(art) });
@@ -541,7 +544,8 @@ impl Importer {
         let art = self.named("", NodeKind::Group { children, clip: true });
         let Some(art) = art else { return Paint::None };
         let base = if pt.id().is_empty() { "Pattern" } else { pt.id() };
-        let name = unique_name(base, |n| self.doc.pattern(n).is_some());
+        let doc = &self.doc;
+        let name = unique_name(base, &mut self.pattern_suffixes, |n| doc.pattern(n).is_some());
         let mut def = PatternDef::new(&name, vec![Arc::new(art)]);
         def.tile = tile;
         self.doc.patterns.push(def);
@@ -714,6 +718,9 @@ mod tests {
             hidden: HashSet::new(),
             uses: HashMap::new(),
             symbols: HashMap::new(),
+            symbol_names: HashSet::new(),
+            symbol_suffixes: HashMap::new(),
+            pattern_suffixes: HashMap::new(),
             blends: HashMap::new(),
             files: Files::default(),
             non_scaling: HashSet::new(),