From e0e361a834c8fe1b0f47e964ea67c19a06c27457 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 06:08:58 -0800 Subject: [PATCH] Effects: a pasted SVG filter avoids brush effect names and is refused with no free name Follows #6592. Paste now also skips the names that effects in the document's brush art use when it picks a free name for a pasted SVG filter. When no free name keeps the filter within the 64 KB limit (a one-letter name at the limit with every free letter and digit taken), the paste is refused instead of keeping the name of the document's filter. Clipboard::import_into and import_text_styles return a Result, and their callers pass the error on before the document changes. --- crates/engine/src/cmd/brushlib.rs | 2 +- crates/engine/src/cmd/clipboard/resources.rs | 36 ++++++++++------- crates/engine/src/cmd/edit.rs | 2 +- crates/engine/src/cmd/library.rs | 4 +- crates/engine/src/cmd/patternlib.rs | 7 ++-- crates/engine/src/cmd/swatchlib.rs | 2 +- crates/engine/src/cmd/symbollib.rs | 4 +- crates/engine/src/cmd/textstyles.rs | 4 +- crates/engine/src/cmd/textstyles/tests.rs | 14 +++---- crates/engine/src/tests/svgfilter.rs | 42 ++++++++++++++++++++ docs/mcp.md | 3 +- 11 files changed, 86 insertions(+), 34 deletions(-) diff --git a/crates/engine/src/cmd/brushlib.rs b/crates/engine/src/cmd/brushlib.rs index a5ff58ddf..b58af91ff 100644 --- a/crates/engine/src/cmd/brushlib.rs +++ b/crates/engine/src/cmd/brushlib.rs @@ -496,7 +496,7 @@ fn add(s: &mut Session, p: &Value) -> Result { // nothing to apply is no edit, and a click that only applies brushes the document has is // named for that. let mut probe = (*st.doc).clone(); - let imported = clip.import_into(&mut probe, &choices, false); + let imported = clip.import_into(&mut probe, &choices, false)?; let doc_names: Vec = picked.iter().map(|n| target(&imported.renamed, n)).collect(); let paths = if apply { brush_targets(&st.doc, &roots(s, p)?) } else { vec![] }; let paint = stroke_paint(s); diff --git a/crates/engine/src/cmd/clipboard/resources.rs b/crates/engine/src/cmd/clipboard/resources.rs index 73bd0b249..534e0ec89 100644 --- a/crates/engine/src/cmd/clipboard/resources.rs +++ b/crates/engine/src/cmd/clipboard/resources.rs @@ -28,7 +28,7 @@ use vectorcraft_doc::{ use super::super::brushsym::{set_natural_size, stored_natural_size}; use super::super::patterncmds::add_pattern; -use super::super::{Result, bad, unique_name}; +use super::super::{EngineError, Result, bad, unique_name}; use super::Clipboard; /// A resource kind objects refer to (graphic styles by id, the others by name). @@ -1055,8 +1055,9 @@ impl Clipboard { } /// Add the resources the objects use to `d` (inside the paste's undo step) and work out how - /// pasted copies relink to them. - pub(crate) fn import_into(&self, d: &mut Document, choices: &SwatchChoices, same_doc: bool) -> Imported { + /// pasted copies relink to them. An error, with `d` unchanged, when a pasted SVG filter finds no + /// free name ([`Self::filter_links`]). + pub(crate) fn import_into(&self, d: &mut Document, choices: &SwatchChoices, same_doc: bool) -> Result { self.import_used(d, &self.used(), choices, same_doc, false) } @@ -1065,9 +1066,9 @@ impl Clipboard { /// each style, symbol, pattern and brush comes after the styles, symbols, patterns and brushes it uses /// ([`use_order`]), so that it is compared and relinked with their names here, and each loop /// of definitions that use each other is matched on its own ([`loop_groups`]). - fn import_used(&self, d: &mut Document, used: &Used, choices: &SwatchChoices, same_doc: bool, in_use_order: bool) -> Imported { + fn import_used(&self, d: &mut Document, used: &Used, choices: &SwatchChoices, same_doc: bool, in_use_order: bool) -> Result { let mut out = Imported::default(); - out.relink.filters = self.filter_links(d, same_doc); + out.relink.filters = self.filter_links(d, same_doc)?; self.import_images(d, used, same_doc, &mut out); self.import_swatches(d, used, choices, same_doc, &mut out); // Brushes whose art shows no symbols or patterns, before the symbols and patterns whose art @@ -1230,7 +1231,7 @@ impl Clipboard { && out.relink.styles.is_empty() && out.relink.colors.is_empty() && used.styles.is_empty()); - out + Ok(out) } /// How the pasted SVG filter effects relink ([`Relink::filters`]). Each names the document's @@ -1238,8 +1239,8 @@ impl Clipboard { /// filter when pasted back into the document it came from, and elsewhere keeps its own under a /// 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)> { + /// a free letter or digit; the filter's id is always its name. An error when no free name fits. + fn filter_links(&self, d: &Document, same_doc: bool) -> Result> { let mut found = BTreeSet::new(); for n in &self.nodes { node_filters(n, &mut found); @@ -1272,6 +1273,9 @@ impl Clipboard { for root in d.art_roots() { node_filter_names(root, &mut taken); } + for a in vectorcraft_brush::library(d).iter().flat_map(Brush::art) { + node_filter_names(a, &mut taken); + } for g in &d.graphic_styles { appearance_filter_names(&g.appearance, &mut taken); } @@ -1288,13 +1292,17 @@ impl Clipboard { .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 }; + let Some((new, drawn)) = renamed else { + return Err(EngineError::Other(format!( + "the pasted SVG filter {name:?} draws differently from the document's filter of that name, and no free name fits it" + ))); + }; taken.insert(new.clone()); (new, drawn) }; links.insert((name, xml), to); } - links + Ok(links) } /// Image blobs: a different blob under a key the document uses gets a new key. @@ -1409,13 +1417,13 @@ pub(crate) struct LoadedStyle { /// a pattern added under a free name and a pattern follows a style renamed on the way in. A style /// `d` has under its name, or a numbered copy of it ("Body 2"), that is the same once relinked is /// used; any other is added under a free name. → each style of `src` placed, in order, character -/// styles first. -pub(crate) fn import_text_styles(src: &Document, d: &mut Document, chars: bool, paras: bool) -> Vec { +/// styles first. An error, as [`Clipboard::import_into`] gives. +pub(crate) fn import_text_styles(src: &Document, d: &mut Document, chars: bool, paras: bool) -> Result> { let library = OnceCell::new(); let used = Used::of_text_styles(&Defs::of(src, &library), chars, paras); let clip = Clipboard::with_used(src, &used, &library, vec![]); let before = [&d.char_styles, &d.para_styles].map(|l| l.iter().map(|s| s.name.clone()).collect::>()); - let out = clip.import_used(d, &used, &SwatchChoices::default(), false, true); + let out = clip.import_used(d, &used, &SwatchChoices::default(), false, true)?; let mut placed: Vec = vec![]; for (para, r, list) in [(false, Res::CharStyle, &clip.char_styles), (true, Res::ParaStyle, &clip.para_styles)] { for s in list { @@ -1425,7 +1433,7 @@ pub(crate) fn import_text_styles(src: &Document, d: &mut Document, chars: bool, placed.push(LoadedStyle { para, name, added }); } } - placed + Ok(placed) } /// Swatch `w` in `d` ([`existing`]), else added under a free name (swatches and colour groups diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 024ba535b..3558f1b56 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -1332,7 +1332,7 @@ fn paste_clip(s: &mut Session, p: &Value, mode: PasteMode, clip: &Clipboard, cho vec![Affine::IDENTITY] }; let (ids, imported, artboards, into) = s.edit(mode.label(), |d, sel| { - let imported = clip.import_into(d, choices, same_doc); + let imported = clip.import_into(d, choices, same_doc)?; // Draw Inside: the clip group of the object drawn into, made on the first paste. let into = inside.map(|t| super::create::draw_inside_group(d, t)).transpose()?; let artboards: Vec = boards diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 48e2d1793..a6d2cbf51 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -1303,7 +1303,7 @@ fn use_pattern(s: &mut Session, data: &Value, stroke: bool) -> Result { let choices = SwatchChoices::default(); // The pattern's name in the document, worked out on a copy: nothing to add is no edit. let mut probe = (*s.doc()?.doc).clone(); - let imported = clip.import_into(&mut probe, &choices, false); + let imported = clip.import_into(&mut probe, &choices, false)?; let renamed = imported.renamed.iter().find(|r| r["kind"] == "pattern" && r["from"] == name.as_str()).and_then(|r| r["to"].as_str()); let target = renamed.unwrap_or(&name).to_string(); let paint = vectorcraft_doc::pattern::pattern_paint(&target); @@ -1315,7 +1315,7 @@ fn use_pattern(s: &mut Session, data: &Value, stroke: bool) -> Result { in_one_step(s, |s| { if added { s.edit("Add to Swatches", |d, _| { - clip.import_into(d, &choices, false); + clip.import_into(d, &choices, false)?; if !has_swatch(d) { let swatch = d.free_swatch_name(&target); d.swatches.push(vectorcraft_color::Swatch { name: swatch, paint: paint.clone(), global: false, spot: false }); diff --git a/crates/engine/src/cmd/patternlib.rs b/crates/engine/src/cmd/patternlib.rs index 3e15c563f..011dfc131 100644 --- a/crates/engine/src/cmd/patternlib.rs +++ b/crates/engine/src/cmd/patternlib.rs @@ -10,6 +10,7 @@ use vectorcraft_doc::pattern::pattern_paint; use vectorcraft_doc::{Appearance, Document, Node, NodeId, PatternDef, TileType}; use vectorcraft_geom::{PathData, Point, Rect, SubPath, shapes}; +use super::Result; use super::clipboard::Clipboard; use super::clipboard::resources::SwatchChoices; use super::swatchlib::LibraryInfo; @@ -84,11 +85,11 @@ pub fn swatch_source(w: &Swatch) -> Option> { /// Add pattern `name` of library `lib` to `d` with the resources its art uses, as pasting art /// painted with it does: the same pattern there already is used, and a name another pattern or /// swatch has gets a number → the pattern's name in `d`. -pub(crate) fn import(d: &mut Document, lib: &Document, name: &str) -> String { +pub(crate) fn import(d: &mut Document, lib: &Document, name: &str) -> Result { let user = Node::path(NodeId(0), shapes::rectangle(Rect::new(0.0, 0.0, 1.0, 1.0)), Appearance::basic(pattern_paint(name), Paint::None, 0.0)); - let imported = Clipboard::gather(lib, vec![user]).import_into(d, &SwatchChoices::default(), false); + let imported = Clipboard::gather(lib, vec![user]).import_into(d, &SwatchChoices::default(), false)?; let renamed = imported.renamed.iter().find(|r| r["kind"] == "pattern" && r["from"] == name).and_then(|r| r["to"].as_str()); - renamed.unwrap_or(name).to_string() + Ok(renamed.unwrap_or(name).to_string()) } /// The pattern of `d` that is pattern `name` of library `lib`: the same tile options and art under diff --git a/crates/engine/src/cmd/swatchlib.rs b/crates/engine/src/cmd/swatchlib.rs index eaa5b8c54..fe2816ed1 100644 --- a/crates/engine/src/cmd/swatchlib.rs +++ b/crates/engine/src/cmd/swatchlib.rs @@ -753,7 +753,7 @@ fn add(s: &mut Session, p: &Value) -> Result { if let (Paint::Pattern { pattern, .. }, Some(src)) = (&w.paint, &patterns) { // The pattern comes with its swatch; a pattern the document has without one // gets one. - let target = patternlib::import(d, src, pattern); + let target = patternlib::import(d, src, pattern)?; let name = match patternlib::pattern_swatch(d, &target) { Some(name) => name, None => { diff --git a/crates/engine/src/cmd/symbollib.rs b/crates/engine/src/cmd/symbollib.rs index c757d93ea..02767aef3 100644 --- a/crates/engine/src/cmd/symbollib.rs +++ b/crates/engine/src/cmd/symbollib.rs @@ -405,7 +405,7 @@ fn add(s: &mut Session, p: &Value) -> Result { }; // Where the symbols land, worked out on a copy: nothing to add and nothing to place is no edit. let mut probe = (*st.doc).clone(); - let imported = clip.import_into(&mut probe, &choices, false); + let imported = clip.import_into(&mut probe, &choices, false)?; let doc_names: Vec = picked.iter().map(|n| target(&imported.renamed, n)).collect(); let centre = st.doc.artboards.first().map(|a| a.rect.center()).unwrap_or(Point::ZERO); let at = if place { Some(Point::new(number_or(p, "x", centre.x, C)?, number_or(p, "y", centre.y, C)?)) } else { None }; @@ -413,7 +413,7 @@ fn add(s: &mut Session, p: &Value) -> Result { let mut out = json!({ "library": info.id }); if imported.added > 0 || place { let placed = s.edit("Add to Symbols", |d, sel| { - let imported = clip.import_into(d, &choices, false); + let imported = clip.import_into(d, &choices, false)?; // Every symbol brought in (nested ones too) takes its Symbol Options along. for sym in &clip.symbols { let to = target(&imported.renamed, &sym.name); diff --git a/crates/engine/src/cmd/textstyles.rs b/crates/engine/src/cmd/textstyles.rs index 3fd8e8f23..89ec47b27 100644 --- a/crates/engine/src/cmd/textstyles.rs +++ b/crates/engine/src/cmd/textstyles.rs @@ -976,12 +976,12 @@ fn load(s: &mut Session, p: &Value, kinds: &[Kind]) -> Result { } let (chars, paras) = (kinds.contains(&Kind::Char), kinds.contains(&Kind::Para)); let mut trial = (*s.doc()?.doc).clone(); - let mut placed = import_text_styles(&src, &mut trial, chars, paras); + let mut placed = import_text_styles(&src, &mut trial, chars, paras)?; // An undo step only when the document changes. The step takes the document the import made, // unless the edit first ends a typing session, which can change the document it starts from. if trial != *s.doc()?.doc { if super::typing_in_progress(s) { - placed = s.edit(label, |d, _| Ok(import_text_styles(&src, d, chars, paras)))?; + placed = s.edit(label, |d, _| import_text_styles(&src, d, chars, paras))?; } else { s.edit(label, |d, _| { *d = trial; diff --git a/crates/engine/src/cmd/textstyles/tests.rs b/crates/engine/src/cmd/textstyles/tests.rs index 73c938dc1..ab0c7d342 100644 --- a/crates/engine/src/cmd/textstyles/tests.rs +++ b/crates/engine/src/cmd/textstyles/tests.rs @@ -1105,7 +1105,7 @@ fn self_referencing_pattern_is_not_added_again() { let different = dst.execute("shape.rectangle", &json!({"x": 200, "y": 200, "width": 30, "height": 30})).unwrap()["id"].clone(); dst.execute("object.pattern.make", &json!({"name": "Dots", "edit": false, "ids": [different]})).unwrap(); let target = std::sync::Arc::make_mut(&mut dst.doc_mut().unwrap().doc); - super::super::clipboard::resources::import_text_styles(&source, target, true, true); + super::super::clipboard::resources::import_text_styles(&source, target, true, true).unwrap(); assert!(dst.doc().unwrap().doc.pattern("Dots 2").is_some()); assert!( dst.doc().unwrap().doc.pattern("Dots 2").unwrap().art[0] @@ -1116,7 +1116,7 @@ fn self_referencing_pattern_is_not_added_again() { ); let before = dst.doc().unwrap().doc.clone(); let target = std::sync::Arc::make_mut(&mut dst.doc_mut().unwrap().doc); - let again = super::super::clipboard::resources::import_text_styles(&source, target, true, true); + let again = super::super::clipboard::resources::import_text_styles(&source, target, true, true).unwrap(); assert!(again.iter().all(|s| !s.added)); assert_eq!(dst.doc().unwrap().doc, before); } @@ -1148,13 +1148,13 @@ fn separate_self_referencing_patterns_are_not_added_again() { dst.execute("object.pattern.make", &json!({"name": format!("Dots{k}"), "edit": false, "ids": [different]})).unwrap(); } let target = std::sync::Arc::make_mut(&mut dst.doc_mut().unwrap().doc); - super::super::clipboard::resources::import_text_styles(&source, target, true, true); + super::super::clipboard::resources::import_text_styles(&source, target, true, true).unwrap(); for k in 0..9 { assert!(dst.doc().unwrap().doc.pattern(&format!("Dots{k} 2")).is_some(), "{k}"); } let before = dst.doc().unwrap().doc.clone(); let target = std::sync::Arc::make_mut(&mut dst.doc_mut().unwrap().doc); - let again = super::super::clipboard::resources::import_text_styles(&source, target, true, true); + let again = super::super::clipboard::resources::import_text_styles(&source, target, true, true).unwrap(); assert!(again.iter().all(|s| !s.added)); assert_eq!(dst.doc().unwrap().doc.patterns.len(), before.patterns.len()); assert_eq!(dst.doc().unwrap().doc, before); @@ -1162,7 +1162,7 @@ fn separate_self_referencing_patterns_are_not_added_again() { let target = std::sync::Arc::make_mut(&mut dst.doc_mut().unwrap().doc); target.patterns.retain(|p| !p.name.starts_with("Dots8")); let left = target.patterns.len(); - let third = super::super::clipboard::resources::import_text_styles(&source, target, true, true); + let third = super::super::clipboard::resources::import_text_styles(&source, target, true, true).unwrap(); assert_eq!(dst.doc().unwrap().doc.patterns.len(), left + 1); assert!(third.iter().filter(|s| s.name != "Patterned8" && !s.name.starts_with("Patterned8 ")).all(|s| !s.added)); } @@ -1195,13 +1195,13 @@ fn a_long_loop_of_patterns_is_not_added_again() { dst.execute("object.pattern.make", &json!({"name": format!("Dots{k}"), "edit": false, "ids": [different]})).unwrap(); } let target = std::sync::Arc::make_mut(&mut dst.doc_mut().unwrap().doc); - super::super::clipboard::resources::import_text_styles(&source, target, true, true); + super::super::clipboard::resources::import_text_styles(&source, target, true, true).unwrap(); for k in 0..9 { assert!(dst.doc().unwrap().doc.pattern(&format!("Dots{k} 2")).is_some(), "{k}"); } let before = dst.doc().unwrap().doc.clone(); let target = std::sync::Arc::make_mut(&mut dst.doc_mut().unwrap().doc); - let again = super::super::clipboard::resources::import_text_styles(&source, target, true, true); + let again = super::super::clipboard::resources::import_text_styles(&source, target, true, true).unwrap(); assert!(again.iter().all(|s| !s.added)); assert_eq!(dst.doc().unwrap().doc.patterns.len(), before.patterns.len()); assert_eq!(dst.doc().unwrap().doc, before); diff --git a/crates/engine/src/tests/svgfilter.rs b/crates/engine/src/tests/svgfilter.rs index 20d95f577..92f1e4987 100644 --- a/crates/engine/src/tests/svgfilter.rs +++ b/crates/engine/src/tests/svgfilter.rs @@ -303,6 +303,48 @@ fn pasted_art_takes_no_name_the_documents_effects_use() { assert_eq!(names, ["Shift 2", "Shift 3"]); } +/// A pasted filter is not renamed to a name an effect in a brush's art gives either. +#[test] +fn pasted_art_takes_no_name_the_documents_brushes_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, "select.set", json!({"ids": [a.0]})); + run(&mut s, "brush.new", json!({"type": "art", "name": "Fx"})); + run(&mut s, "edit.clear", json!({"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 3"]); +} + +/// A pasted filter at the size limit with a one-letter name, when every free name it could take is +/// in use, is refused rather than pasted under the name of the document's filter. +#[test] +fn a_paste_with_no_free_name_for_its_filter_is_refused() { + let mut s = session(); + for n in ('a'..='z').chain('0'..='9') { + run(&mut s, "svgFilter.define", json!({"xml": format!(r#""#)})); + } + run(&mut s, "svgFilter.define", json!({"xml": r#""#})); + let (open, close) = (r#""#); + let big = format!("{open}{}{close}", "a".repeat(vectorcraft_doc::SvgFilter::MAX_XML - open.len() - close.len())); + let svg = format!( + r#"{big}"# + ); + run(&mut s, "clipboard.importSvg", json!({"svg": svg})); + let before = steps(&s); + assert!(err(&mut s, "edit.pasteInPlace", json!({})).contains("no free name")); + assert!(filter_effects(&s).is_empty(), "nothing pasted"); + assert_eq!(steps(&s), before); +} + /// 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] diff --git a/docs/mcp.md b/docs/mcp.md index 9c2a286b7..23349c348 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -3764,7 +3764,8 @@ what every object that uses it draws, in the edit's one undo step, and an applic filter. Deleting a filter that objects use leaves their effects in place, still named, drawing nothing (their `xml` empty) until a filter of that name is defined or imported again. A pasted effect that names one of the document's filters but draws another definition keeps its own filter under a free name (`Soft 2`); pasted back into the document -it was copied from, it takes the document's filter. Opened from such an effect, Apply SVG Filter shows the +it was copied from, it takes the document's filter. The free name is one that no filter of the document and no effect in +its art, brushes or graphic styles uses; when no such name keeps the filter within 64 KB, the paste is refused. Opened from such an effect, Apply SVG Filter shows the name above the list, not selectable. `effect.apply {effect: "svg.filter", params: {xml}}` takes a filter directly and names it by its `id`; an `xml` that isn't one such filter is an error. `ui.svgFilterDialog {}` opens Apply SVG Filter (New, Edit, Delete, Import…), and from the Appearance panel it edits which filter an applied effect runs;