From d57d0b8e675532b54c70bf651977986c10b9ee84 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 31 Oct 2026 23:00:53 -0700 Subject: [PATCH 1/3] Appearance: effect.move reports the effect's index in the object whose row it names With the Appearance panel's active row, the row belongs to the first selected object, which need not paint first. effect.move reported where the effect landed in the first target in paint order; it now reports where it landed in the first selected object for the active row, and in the first target for an explicit fromItem, as effect.apply does. --- crates/engine/src/cmd/effectcmd.rs | 11 +++++++---- crates/engine/src/tests/appearance.rs | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/effectcmd.rs b/crates/engine/src/cmd/effectcmd.rs index feee6913f..a62a694cd 100644 --- a/crates/engine/src/cmd/effectcmd.rs +++ b/crates/engine/src/cmd/effectcmd.rs @@ -304,8 +304,8 @@ fn move_effect(s: &mut Session, p: &Value) -> Result { if aligned(p) && super::appearance::appearances_differ(&s.doc()?.doc, &roots) { return Err(bad(C, "effects of objects whose appearances differ can't be moved together")); } - // Where the effect landed in the first object. - let mut landed = None; + // Where the effect landed in each object it moved in, in the order of `ids`. + let mut landed = vec![]; let ids = s.edit(if copy { "Duplicate Effect" } else { "Move Effect" }, |d, _| { let mut done = vec![]; for id in &roots { @@ -317,7 +317,7 @@ fn move_effect(s: &mut Session, p: &Value) -> Result { let at = to.min(fx.len()); fx.insert(at, e); check_revolve_count(n, C)?; - landed.get_or_insert((at, di)); + landed.push((at, di)); done.push(*id); } if done.is_empty() { @@ -325,7 +325,10 @@ fn move_effect(s: &mut Session, p: &Value) -> Result { } Ok(done) })?; - let (index, item) = landed.unzip(); + // The reply names the object whose row the active item is (the first selected object, which + // need not paint first), as `effect.apply` does; the first target otherwise. + let anchor = if matches!(src, ItemTarget::Item { explicit: false, .. }) { src } else { dst }; + let (index, item) = landed.get(anchor.anchor(s, &ids)).copied().unzip(); Ok(json!({ "ids": ids_json(&ids), "index": index, "item": item.flatten() })) } diff --git a/crates/engine/src/tests/appearance.rs b/crates/engine/src/tests/appearance.rs index e1728c1ad..372c653d7 100644 --- a/crates/engine/src/tests/appearance.rs +++ b/crates/engine/src/tests/appearance.rs @@ -534,6 +534,28 @@ fn apply_reports_the_index_in_the_object_whose_row_it_names() { assert_eq!((r["item"].clone(), r["index"].clone()), (json!(0), json!(1)), "a's row 0 holds two effects"); } +/// `effect.move`'s reply names where the effect landed in the object whose row the active item is +/// (the first selected object), also when that object paints above the others. +#[test] +fn move_reports_the_index_in_the_object_whose_row_it_names() { + let mut s = session(); + let (a, b) = reversed_pair(&mut s); + run(&mut s, "select.set", json!({"ids": [a.0]})); + run(&mut s, "effect.apply", json!({"effect": "distort.twist", "item": 0})); + run(&mut s, "select.set", json!({"ids": [b.0]})); + run(&mut s, "effect.apply", json!({"effect": "distort.twist", "item": 0})); + run(&mut s, "effect.apply", json!({"effect": "distort.twist", "item": 0})); + run(&mut s, "select.set", json!({"ids": [b.0, a.0]})); + run(&mut s, "appearance.setActiveItem", json!({"index": 0})); + // Row 0's first effect goes to the end of the row: index 1 in b, 0 in a, which paints first. + let r = run(&mut s, "effect.move", json!({"from": 0, "to": 9})); + assert_eq!((node(&s, a).appearance.items[0].effects().len(), node(&s, b).appearance.items[0].effects().len()), (1, 2)); + assert_eq!((r["item"].clone(), r["index"].clone()), (json!(0), json!(1)), "b's row 0 holds two effects"); + // An explicit item is a row of the first target, a. + let r = run(&mut s, "effect.move", json!({"from": 0, "to": 9, "fromItem": 0})); + assert_eq!((r["item"].clone(), r["index"].clone()), (json!(0), json!(0)), "a's row 0 holds one effect"); +} + /// How many steps there are to undo and to redo, and whether the document has unsaved changes. fn history(s: &Session) -> (usize, usize, bool) { let st = s.doc().unwrap(); From 468102e1013e3654c776b6f70b8b98fc73aa571a Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 1 Nov 2026 07:03:30 -0800 Subject: [PATCH 2/3] Appearance: test effect.move's reply when the first selected object has no effect to move --- crates/engine/src/tests/appearance.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/engine/src/tests/appearance.rs b/crates/engine/src/tests/appearance.rs index 372c653d7..4e9292d7e 100644 --- a/crates/engine/src/tests/appearance.rs +++ b/crates/engine/src/tests/appearance.rs @@ -554,6 +554,12 @@ fn move_reports_the_index_in_the_object_whose_row_it_names() { // An explicit item is a row of the first target, a. let r = run(&mut s, "effect.move", json!({"from": 0, "to": 9, "fromItem": 0})); assert_eq!((r["item"].clone(), r["index"].clone()), (json!(0), json!(0)), "a's row 0 holds one effect"); + // When the first selected object has no effect to move, the reply names the first object that moved. + run(&mut s, "select.set", json!({"ids": [a.0, b.0]})); + run(&mut s, "appearance.setActiveItem", json!({"index": 0})); + let r = run(&mut s, "effect.move", json!({"from": 1, "to": 0})); + assert_eq!(r["ids"], json!([b.0]), "only b has a second effect in row 0"); + assert_eq!((r["item"].clone(), r["index"].clone()), (json!(0), json!(0)), "b's effect moved to the start of its row 0"); } /// How many steps there are to undo and to redo, and whether the document has unsaved changes. From e4a066f12a84ab0bb464e28781771892b17bbdd9 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 1 Nov 2026 07:20:22 -0800 Subject: [PATCH 3/3] Appearance: effect.move's reply follows an explicit toItem to the first target --- crates/engine/src/cmd/effectcmd.rs | 8 ++++---- crates/engine/src/tests/appearance.rs | 9 +++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/effectcmd.rs b/crates/engine/src/cmd/effectcmd.rs index a62a694cd..9f1e20121 100644 --- a/crates/engine/src/cmd/effectcmd.rs +++ b/crates/engine/src/cmd/effectcmd.rs @@ -325,10 +325,10 @@ fn move_effect(s: &mut Session, p: &Value) -> Result { } Ok(done) })?; - // The reply names the object whose row the active item is (the first selected object, which - // need not paint first), as `effect.apply` does; the first target otherwise. - let anchor = if matches!(src, ItemTarget::Item { explicit: false, .. }) { src } else { dst }; - let (index, item) = landed.get(anchor.anchor(s, &ids)).copied().unzip(); + // The reply names the object whose row the destination is: the first selected object for the + // active item (which need not paint first), as `effect.apply` does; the first target for an + // explicit `toItem` or the object's effects. + let (index, item) = landed.get(dst.anchor(s, &ids)).copied().unzip(); Ok(json!({ "ids": ids_json(&ids), "index": index, "item": item.flatten() })) } diff --git a/crates/engine/src/tests/appearance.rs b/crates/engine/src/tests/appearance.rs index 4e9292d7e..a49bae19a 100644 --- a/crates/engine/src/tests/appearance.rs +++ b/crates/engine/src/tests/appearance.rs @@ -560,6 +560,15 @@ fn move_reports_the_index_in_the_object_whose_row_it_names() { let r = run(&mut s, "effect.move", json!({"from": 1, "to": 0})); assert_eq!(r["ids"], json!([b.0]), "only b has a second effect in row 0"); assert_eq!((r["item"].clone(), r["index"].clone()), (json!(0), json!(0)), "b's effect moved to the start of its row 0"); + // From the active row to an explicit destination (here the object's effects, where b already + // has one), the reply names the first target, a. + run(&mut s, "select.set", json!({"ids": [b.0]})); + run(&mut s, "effect.apply", json!({"effect": "distort.twist", "item": null})); + run(&mut s, "select.set", json!({"ids": [b.0, a.0]})); + run(&mut s, "appearance.setActiveItem", json!({"index": 0})); + let r = run(&mut s, "effect.move", json!({"from": 0, "to": 9, "toItem": null})); + assert_eq!((node(&s, a).appearance.effects.len(), node(&s, b).appearance.effects.len()), (1, 2)); + assert_eq!((r["item"].clone(), r["index"].clone()), (json!(null), json!(0)), "a's effects hold one effect"); } /// How many steps there are to undo and to redo, and whether the document has unsaved changes.