From 08efd913606575ca4199b298adb52834523a7cb2 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 04:44:54 -0800 Subject: [PATCH] Effects: a pasted SVG filter is not renamed to a name the document's effects use Paste renamed a pasted SVG filter to a name no filter of the document held, but an effect could still use that name, for example after its filter was deleted. Defining a filter of that name later changed both effects. Paste now also skips the names the document's effects use, read only when a pasted filter needs a new name. A new name with a tab, line feed or carriage return was written into the filter's id as is, which XML reads as a space, so the id no longer matched the effect's name. svg_filter::with_id now writes those characters as character references, and paste takes a name only when the filter's id reads back as that name. --- crates/effects/src/svg_filter.rs | 5 +-- crates/engine/src/cmd/clipboard/resources.rs | 34 ++++++++++++++++-- crates/engine/src/tests/svgfilter.rs | 38 ++++++++++++++++++++ 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/crates/effects/src/svg_filter.rs b/crates/effects/src/svg_filter.rs index 325ce9b38..3877a60ae 100644 --- a/crates/effects/src/svg_filter.rs +++ b/crates/effects/src/svg_filter.rs @@ -1101,8 +1101,9 @@ pub fn with_id(xml: &str, id: &str) -> Option { let f = filter_element(&doc).ok()?; let at = f.attributes().find(|a| a.name() == "id" && a.namespace().is_none())?.range_value(); let (start, end) = (at.start.checked_sub(head)?, at.end.checked_sub(head)?); - // The value may sit in single quotes: those are escaped too. - let id = escaped(id).replace('\'', "'"); + // The value may sit in single quotes: those are escaped too. A tab, line feed or carriage + // return is written as a character reference, which attribute-value normalization keeps. + let id = escaped(id).replace('\'', "'").replace('\t', " ").replace('\n', " ").replace('\r', " "); Some(format!("{}{}{}", xml.get(..start)?, id, xml.get(end..)?)) } diff --git a/crates/engine/src/cmd/clipboard/resources.rs b/crates/engine/src/cmd/clipboard/resources.rs index 22703b467..73bd0b249 100644 --- a/crates/engine/src/cmd/clipboard/resources.rs +++ b/crates/engine/src/cmd/clipboard/resources.rs @@ -165,6 +165,23 @@ fn node_filters(n: &Node, out: &mut BTreeSet<(String, String)>) { }); } +/// The names the SVG filter effects of appearance `a` give (its own and its fills' and strokes'). +fn appearance_filter_names(a: &Appearance, out: &mut BTreeSet) { + let items = a.items.iter().flat_map(|i| i.effects().iter()); + let named = a.effects.iter().chain(items).filter(|e| e.id == Appearance::SVG_FILTER); + out.extend(named.filter_map(|e| e.params.get("name").and_then(Value::as_str)).map(str::to_string)); +} + +/// The names the SVG filter effects in the subtree of `n` give (opacity-mask art included). +fn node_filter_names(n: &Node, out: &mut BTreeSet) { + n.walk(&mut |m| { + appearance_filter_names(&m.appearance, out); + if let Some(mask) = &m.extras().mask { + node_filter_names(&mask.art, out); + } + }); +} + /// A global or spot colour of its own (not a tint of another swatch): what colours link to. fn is_base(w: &Swatch) -> bool { (w.global || w.spot) && matches!(w.paint, Paint::Solid { swatch: None, .. }) @@ -1219,7 +1236,7 @@ impl Clipboard { /// How the pasted SVG filter effects relink ([`Relink::filters`]). Each names the document's /// filter of its name; one that draws another definition than that filter takes the document's /// filter when pasted back into the document it came from, and elsewhere keeps its own under a - /// free name, so editing or deleting the document's filter leaves it alone. When that name would + /// name no filter or effect of the document gives, so editing or deleting the document's filter leaves it alone. When that name would /// make the filter too long to draw, the free name is a shorter start of its name ("Shif"), else /// a free letter or digit; the filter's id is always its name. fn filter_links(&self, d: &Document, same_doc: bool) -> BTreeMap<(String, String), (String, String)> { @@ -1240,12 +1257,25 @@ impl Clipboard { appearance_filters(&g.appearance, &mut found); } let mut taken: BTreeSet = d.svg_filters.iter().map(|f| f.name.clone()).chain(found.iter().map(|(n, _)| n.clone())).collect(); + let mut effects_read = false; let mut links = BTreeMap::new(); for (name, xml) in found { let Some(held) = d.svg_filters.iter().find(|f| f.name == name).filter(|f| f.xml != xml.trim()) else { continue }; let to = if same_doc { (name.clone(), held.xml.clone()) } else { + // The names the document's effects give are taken too, those whose filter was + // deleted included: defining a filter of that name later changes every effect that + // names it. They are read at the first rename only. + if !effects_read { + effects_read = true; + for root in d.art_roots() { + node_filter_names(root, &mut taken); + } + for g in &d.graphic_styles { + appearance_filter_names(&g.appearance, &mut taken); + } + } // Free names made from the starts of the name, longest first, then free letters and // digits: the first under which the filter still draws. let mut ends: Vec = name.char_indices().map(|(i, _)| i).skip(1).collect(); @@ -1255,7 +1285,7 @@ impl Clipboard { let single = ('a'..='z').chain('0'..='9').map(String::from).filter(|n| !taken.contains(n)); let renamed = shorter.chain(single).find_map(|new| { let drawn = vectorcraft_render::effects::svg_filter::with_id(&xml, &new) - .filter(|x| vectorcraft_render::effects::svg_filter::check(x).is_ok())?; + .filter(|x| vectorcraft_render::effects::svg_filter::check(x).is_ok_and(|id| id == new))?; Some((new, drawn)) }); let Some((new, drawn)) = renamed else { continue }; diff --git a/crates/engine/src/tests/svgfilter.rs b/crates/engine/src/tests/svgfilter.rs index 7606cebe4..20d95f577 100644 --- a/crates/engine/src/tests/svgfilter.rs +++ b/crates/engine/src/tests/svgfilter.rs @@ -284,6 +284,44 @@ fn pasted_art_with_a_filter_at_the_size_limit_still_draws_when_renamed() { } } +/// A pasted filter is not renamed to a name an effect in the document already gives, even one whose +/// filter was deleted, so defining a filter of that name later changes only that effect. +#[test] +fn pasted_art_takes_no_name_the_documents_effects_use() { + let mut s = session(); + let a = rect(&mut s, 10.0); + run(&mut s, "svgFilter.define", json!({"xml": r#""#})); + run(&mut s, "svgFilter.apply", json!({"name": "Shift 2", "ids": [a.0]})); + run(&mut s, "svgFilter.delete", json!({"name": "Shift 2"})); + run(&mut s, "svgFilter.define", json!({"xml": SHIFT})); + let svg = format!( + r#"{SHIFT_MORE}"# + ); + run(&mut s, "clipboard.importSvg", json!({"svg": svg})); + run(&mut s, "edit.pasteInPlace", json!({})); + let names: Vec = filter_effects(&s).into_iter().map(|(n, _)| n).collect(); + assert_eq!(names, ["Shift 2", "Shift 3"]); +} + +/// A pasted filter whose name holds a tab, a line feed or a carriage return keeps those characters +/// in its id under its new name. +#[test] +fn a_renamed_paste_keeps_whitespace_characters_in_its_id() { + for c in [" ", " ", " "] { + let mut s = session(); + run(&mut s, "svgFilter.define", json!({"xml": format!(r#""#)})); + let svg = format!( + r#""# + ); + run(&mut s, "clipboard.importSvg", json!({"svg": svg})); + run(&mut s, "edit.pasteInPlace", json!({})); + let pasted = filter_effects(&s); + assert_eq!(pasted.len(), 1); + assert_ne!(pasted[0].0, names(&mut s)[0], "renamed"); + assert_eq!(vectorcraft_render::effects::svg_filter::check(&pasted[0].1), Ok(pasted[0].0.clone()), "{c}: its id is its name"); + } +} + /// Art in a symbol's definition that uses a filter follows its edits too. #[test] fn a_symbols_art_follows_a_filter_edit() {