From 0cf2aa3f79464230074ec4942f57fe4bbaf5a411 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 15 Nov 2026 08:36:31 -0800 Subject: [PATCH] Effects: removing or duplicating several aligned effects of a design's art refuses one that no object has With the art of a graph design in a series given first and aligned: true, effect.remove and effect.duplicate with `indices` skipped an effect of the art that no other object has, and edited the others. With art [shadow, roughen], a mark [shadow] and indices [0, 1], effect.remove took the mark's shadow off and succeeded. Such an edit is now an error that leaves the document and its undo history as they are, as the same index given alone already was. An object that lacks one of the effects while another object has it is still left out for that effect. --- crates/engine/src/cmd/effectcmd.rs | 10 ++++++-- crates/engine/src/cmd/graph/tests.rs | 36 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/effectcmd.rs b/crates/engine/src/cmd/effectcmd.rs index 42113c9578..30b3dd6304 100644 --- a/crates/engine/src/cmd/effectcmd.rs +++ b/crates/engine/src/cmd/effectcmd.rs @@ -367,7 +367,8 @@ fn edit_effects( /// [`edit_effects`] for several effects of one list (`indices`): `f` gets each target's list and /// the positions of `indices` there, ascending; with `aligned`, a position a target doesn't share -/// is left out of it, and a target sharing none of them is left out. +/// is left out of it, and a target sharing none of them is left out. A position no target has is +/// an error with `none`, and nothing changes. #[allow(clippy::too_many_arguments)] fn edit_effects_at( s: &mut Session, @@ -396,7 +397,12 @@ fn edit_effects_at( // Each target's list and the positions in it, in the order the targets first come up. let mut targets: Vec<(NodeId, Option, Vec)> = vec![]; for &index in indices { - for (id, at, i) in effect_targets(s, p, item, index, cmd)? { + let found = effect_targets(s, p, item, index, cmd)?; + // An aligned effect of a design's art that no other object has is refused, as it is given alone. + if found.is_empty() { + return Err(EngineError::Other(none.into())); + } + for (id, at, i) in found { match targets.iter_mut().find(|t| t.0 == id && t.1 == at) { Some(t) => t.2.push(i), None => targets.push((id, at, vec![i])), diff --git a/crates/engine/src/cmd/graph/tests.rs b/crates/engine/src/cmd/graph/tests.rs index ca1f1d2c45..c533cb5e4e 100644 --- a/crates/engine/src/cmd/graph/tests.rs +++ b/crates/engine/src/cmd/graph/tests.rs @@ -1109,6 +1109,42 @@ fn aligned_effect_indices_past_the_end_of_a_graph_designs_art_are_refused() { assert!(rows(&s, mark).effects.is_empty()); } +/// With a graph design's art given first and `aligned: true`, `effect.remove` and `effect.duplicate` with `indices` are an +/// error and change nothing when one of the art's effects is one no other object has, as with that index given alone. +#[test] +fn aligned_effect_indices_of_a_graph_designs_art_that_no_object_has_are_refused() { + let rows = |s: &Session, id: NodeId| (*s.doc().unwrap().doc.node(id).unwrap().appearance).clone(); + let (mut s, _, art, mark) = aligned_design_art(); + // The art has a shadow and a roughen, the mark the shadow only. + s.execute("effect.apply", &json!({"effect": "stylize.dropShadow", "ids": [mark.0], "item": null})).unwrap(); + s.execute("effect.apply", &json!({"effect": "distort.roughen", "ids": [mark.0], "item": null})).unwrap(); + s.edit("Effects", |d, _| { + let effects = d.node(mark).unwrap().appearance.effects.clone(); + d.node_mut(art).unwrap().appearance.effects = effects; + d.node_mut(mark).unwrap().appearance.effects.pop(); + Ok(()) + }) + .unwrap(); + let (art_was, mark_was) = (rows(&s, art), rows(&s, mark)); + let steps = s.doc().unwrap().history.undo.len(); + for cmd in ["effect.remove", "effect.duplicate"] { + assert!(s.execute(cmd, &json!({"ids": [art.0, mark.0], "item": null, "index": 1, "aligned": true})).is_err(), "{cmd}"); + let e = s.execute(cmd, &json!({"ids": [art.0, mark.0], "item": null, "indices": [0, 1], "aligned": true})).unwrap_err(); + assert!(e.to_string().contains("no effects at indices 0, 1"), "{cmd}: {e}"); + assert_eq!((rows(&s, art), rows(&s, mark)), (art_was.clone(), mark_was.clone()), "{cmd}"); + assert_eq!(s.doc().unwrap().history.undo.len(), steps, "{cmd}"); + } + // With the roughen on the mark too, both of its effects are removed. + s.edit("Roughen", |d, _| { + let roughen = d.node(art).unwrap().appearance.effects[1].clone(); + d.node_mut(mark).unwrap().appearance.effects.push(roughen); + Ok(()) + }) + .unwrap(); + s.execute("effect.remove", &json!({"ids": [art.0, mark.0], "item": null, "indices": [0, 1], "aligned": true})).unwrap(); + assert!(rows(&s, mark).effects.is_empty()); +} + /// A blur on a series group makes the whole group an image, which draws the design's art with the series' other art: /// Expand Appearance does not put the art back beside the image. #[test]