diff --git a/crates/engine/src/presets/patterns.rs b/crates/engine/src/presets/patterns.rs index f4ecb15c3c..2425dd0900 100644 --- a/crates/engine/src/presets/patterns.rs +++ b/crates/engine/src/presets/patterns.rs @@ -125,13 +125,32 @@ fn edit(s: &mut Session, p: &Value) -> Result { "rename" => super::call(s, "pattern.rename", json!({"pattern": req_str(p, "preset", CMD)?, "name": req_str(p, "name", CMD)?}))?, "delete" => { let keys: Vec = match p.get("preset") { - Some(Value::Array(a)) => a.iter().filter_map(Value::as_str).map(str::to_string).collect(), + Some(Value::Array(a)) => a + .iter() + .map(|v| v.as_str().map(str::to_string).ok_or_else(|| bad(CMD, "`preset` must contain only pattern ids or names"))) + .collect::>()?, _ => vec![req_str(p, "preset", CMD)?.to_string()], }; - for k in &keys { - super::call(s, "pattern.delete", json!({"pattern": k}))?; + if keys.is_empty() { + return Err(bad(CMD, "missing `preset`")); } - json!({"deleted": keys.len()}) + // Resolve every reference first. Deleting as we go left the library partly + // removed when a later name was missing, and Undo cannot put a preset back (#4103). + let mut ids = Vec::new(); + for k in &keys { + // Only library patterns can be deleted here; a document-only pattern must + // fail the whole batch up front rather than midway through it. + let id = photocraft_doc::pattern::find(&s.patterns.items, k, k) + .map(|q| q.id.clone()) + .ok_or_else(|| bad(CMD, format!("no library pattern \"{k}\"")))?; + if !ids.contains(&id) { + ids.push(id); + } + } + for id in &ids { + super::call(s, "pattern.delete", json!({"pattern": id}))?; + } + json!({"deleted": ids.len()}) } "deleteGroup" => { // Deleting a folder deletes its patterns, as in Photoshop. diff --git a/crates/engine/src/presets/tests.rs b/crates/engine/src/presets/tests.rs index 7ebf2145b1..dcfc4e94d8 100644 --- a/crates/engine/src/presets/tests.rs +++ b/crates/engine/src/presets/tests.rs @@ -385,6 +385,43 @@ fn pattern_groups_select_apply_and_sync() { assert!(crate::pattern_cmds::resolve(&s, "Weave").is_none()); } +#[test] +fn pattern_batch_delete_keeps_the_library_when_one_name_is_missing() { + let mut s = session(8); + let before = s.execute("pattern.presets.list", json!({})).unwrap(); + let err = s.execute("pattern.presets.edit", json!({"action": "delete", "preset": ["Bricks", "missing"]})).unwrap_err(); + assert!(err.to_string().contains("missing"), "{err}"); + assert_eq!(s.execute("pattern.presets.list", json!({})).unwrap(), before); + assert!(crate::pattern_cmds::resolve(&s, "Bricks").is_some()); +} + +#[test] +fn pattern_batch_delete_rejects_non_string_members_without_deleting() { + for invalid in [json!(null), json!(7), json!({})] { + let mut s = session(8); + let before = s.execute("pattern.presets.list", json!({})).unwrap(); + assert!(s.execute("pattern.presets.edit", json!({"action": "delete", "preset": ["Bricks", invalid]})).is_err()); + assert_eq!(s.execute("pattern.presets.list", json!({})).unwrap(), before); + } +} + +#[test] +fn pattern_batch_delete_refuses_a_document_only_pattern_up_front() { + let mut s = session(8); + let added = s.execute("edit.definePattern", json!({"name": "Document Only"})).unwrap(); + let id = added["pattern"].as_str().unwrap().to_string(); + let pattern = s.patterns.items.iter().find(|p| p.id == id).unwrap().clone(); + s.edit("Embed pattern fixture", |doc, _| { + doc.patterns.push(pattern.clone()); + Ok(()) + }) + .unwrap(); + s.execute("pattern.delete", json!({"pattern": id})).unwrap(); + let before = s.patterns.items.clone(); + assert!(s.execute("pattern.presets.edit", json!({"action": "delete", "preset": ["Bricks", id]})).is_err()); + assert_eq!(s.patterns.items, before, "nothing is deleted when one entry is not in the library"); +} + // ------------------------------------------------------------------ styles #[test]