Merge pull request #5428 from dotCooCoo/fix/effect-dialog-compound-target

Read compound effects when opening effect dialogs

crates/engine/src/cmd/appearance.rs

@@ -178,6 +178,23 @@ pub(crate) struct ActiveItem {
 }
 
 impl Session {
+    /// The method returns explicit target parameters and the selected appearance effect list.
+    pub fn appearance_effect_target(&self, params: &Value) -> Option<(Value, &[vectorcraft_doc::Effect])> {
+        let mut ids = appearance_targets(self, params).ok()?;
+        let item = item_target(self, params, "effect.apply").ok()?;
+        let anchor = item.anchor(self, &ids);
+        let node = self.active()?.doc.node(*ids.get(anchor)?)?;
+        let at = item.effects_item(&node.appearance, "effect.apply").ok()?;
+        let effects = node.appearance.effects_at(at)?;
+        ids.swap(0, anchor);
+        let ids: Vec<u64> = ids.into_iter().map(|id| id.0).collect();
+        let aligned = match item {
+            ItemTarget::Item { aligned, .. } => aligned,
+            ItemTarget::Top => aligned(params),
+        };
+        Some((json!({"ids": ids, "item": at, "aligned": aligned}), effects.as_slice()))
+    }
+
     /// The Appearance panel's active fill/stroke: a paint-order index into the first selected
     /// object's stack, while the selection it was chosen for is unchanged.
     pub fn appearance_item(&self) -> Option<usize> {

crates/ui-egui/src/dialogs/effect.rs

@@ -43,6 +43,7 @@ fn heading_label(d: &Dialog) -> String {
 /// Fields: `__effect`, `__label`, `__item?` (as given to `effect.dialog`), `__target` and
 /// `__index` (the applied effect's item and position). OK edits it; `discard: true` then OK adds
 /// a new one.
+/// `__editTarget` stores the targets used when editing the applied effect.
 pub const EXISTS: &str = "effectExists";
 
 pub(super) const EXISTS_SPEC: DialogSpec = DialogSpec {
@@ -67,7 +68,8 @@ pub fn open(app: &mut VectorcraftApp, p: &Value) -> Result<Value, String> {
     // The list the effect goes to: `item` as given, else the active item's (the object's without
     // one). A new effect keeps `item` unresolved, so `effect.apply` targets each object as usual.
     let target = p.get("item").cloned().unwrap_or_else(|| json!(app.session.appearance_item()));
-    let applied = target_effects(app, &target);
+    let (edit_target, applied) = target_effects(app, p);
+    let target = edit_target.get("item").cloned().unwrap_or(target);
     let mut fields = Map::new();
     fields.insert("__effect".into(), json!(id));
     fields.insert("__label".into(), json!(info.label.trim_end_matches('…')));
@@ -85,12 +87,14 @@ pub fn open(app: &mut VectorcraftApp, p: &Value) -> Result<Value, String> {
                 return Err(format!("effect {k} is `{}`, not `{id}`", e.id));
             }
             fields.insert("__item".into(), target);
+            fields.insert("__editTarget".into(), edit_target);
             fields.insert("__index".into(), json!(k));
             fields.extend(params_of(e));
         }
         None => {
             if let Some(k) = applied.iter().rposition(|e| e.id == id) {
                 fields.insert("__target".into(), target);
+                fields.insert("__editTarget".into(), edit_target);
                 fields.insert("__index".into(), json!(k));
                 app.ui.dialog = Some(Dialog { kind: EXISTS.into(), fields });
                 return Ok(json!({ "pending": EXISTS }));
@@ -106,11 +110,14 @@ pub fn open(app: &mut VectorcraftApp, p: &Value) -> Result<Value, String> {
     Ok(Value::Null)
 }
 
-/// The function reads effects at `item` (an index or null) from the targeted node or first selected object.
-fn target_effects(app: &VectorcraftApp, item: &Value) -> Vec<vectorcraft_doc::Effect> {
-    let Some(st) = app.session.active() else { return vec![] };
-    let node = st.selection.subjects().first().and_then(|id| st.doc.node(*id));
-    node.and_then(|n| n.appearance.effects_at(item.as_u64().map(|i| i as usize))).cloned().unwrap_or_default()
+/// The function returns target parameters and effects at `item` (an index or null).
+/// An omitted `item` uses the active row.
+fn target_effects(app: &VectorcraftApp, p: &Value) -> (Value, Vec<vectorcraft_doc::Effect>) {
+    let mut params = json!({"aligned": p.get("aligned").and_then(Value::as_bool).unwrap_or(false)});
+    if let Some(item) = p.get("item") {
+        params["item"] = item.clone();
+    }
+    app.session.appearance_effect_target(&params).map(|(target, effects)| (target, effects.to_vec())).unwrap_or_else(|| (json!({}), vec![]))
 }
 
 /// An applied effect's values over its defaults.
@@ -128,6 +135,7 @@ fn confirm_exists(app: &mut VectorcraftApp, d: &Dialog) -> Result<Value, String>
     let target = fields.remove("__target").unwrap_or(Value::Null);
     if d.bool("discard") {
         fields.remove("__index");
+        fields.remove("__editTarget");
         let id = d.str("__effect");
         let defaults = vectorcraft_effects::default_params(&id);
         fields.extend(defaults.and_then(|v| v.as_object().cloned()).unwrap_or_default());
@@ -136,7 +144,10 @@ fn confirm_exists(app: &mut VectorcraftApp, d: &Dialog) -> Result<Value, String>
         }
     } else {
         let k = fields.get("__index").and_then(Value::as_u64).unwrap_or(0) as usize;
-        fields.extend(target_effects(app, &target).get(k).map(params_of).unwrap_or_default());
+        let edit_target = fields.get("__editTarget").cloned().unwrap_or_else(|| json!({"item": target}));
+        let applied = app.session.appearance_effect_target(&edit_target).map(|(_, effects)| effects.to_vec()).unwrap_or_default();
+        let params = applied.get(k).map(params_of).unwrap_or_default();
+        fields.extend(params);
         fields.insert("__item".into(), target);
     }
     fields.insert("preview".into(), json!(true));
@@ -804,7 +815,10 @@ fn command(d: &Dialog) -> (&'static str, Value) {
     let params = form::params(d);
     let item = d.fields.get("__item").cloned();
     if let Some(index) = d.fields.get("__index") {
-        let mut p = json!({"index": index, "item": item.unwrap_or(Value::Null), "params": params});
+        let mut p = d.fields.get("__editTarget").filter(|p| p.is_object()).cloned().unwrap_or_else(|| json!({}));
+        p["index"] = index.clone();
+        p["item"] = item.unwrap_or(Value::Null);
+        p["params"] = params;
         let id = d.str("__effect");
         // The displayed style comes from the first target. Preserve other targets' Warp IDs
         // unless the user deliberately chooses Style, including after a preview rollback.

crates/ui-egui/src/panels/tests_appearance.rs

@@ -870,6 +870,164 @@ fn transform_effect_name_edits_the_displayed_appearance_target() {
     }
 }
 
+#[test]
+fn transform_effect_dialog_reads_the_selected_compound_members_owner() {
+    for item in [None, Some(0)] {
+        let mut app = app_with_rect();
+        let member = app.session.doc().unwrap().selection.subjects()[0];
+        let other = app.run("shape.rectangle", json!({"x": 30, "y": 30, "width": 20, "height": 20})).unwrap()["id"].clone();
+        app.run("select.set", json!({"ids": [member.0, other]})).unwrap();
+        let compound = app.run("object.compoundPath.make", json!({})).unwrap()["id"].as_u64().unwrap();
+        app.run("effect.apply", json!({"effect": "distort.transform", "item": item, "params": {"moveH": 12, "copies": 2}})).unwrap();
+        app.run("select.set", json!({"ids": [member.0]})).unwrap();
+        app.run("effect.dialog", json!({"effect": "distort.transform", "item": item, "index": 0})).unwrap();
+        let d = app.ui.dialog.as_mut().unwrap();
+        assert_eq!(d.f64("moveH", 0.0), 12.0);
+        assert_eq!(d.f64("copies", 0.0), 2.0);
+        d.fields.insert("moveH".into(), json!(35));
+        crate::tests_labels::painted_text(&mut app, |app, ui| crate::dialogs::show(app, ui.ctx()));
+        crate::dialogs::confirm(&mut app).unwrap();
+        let effects = &app.session.doc().unwrap().doc.node(vectorcraft_doc::NodeId(compound)).unwrap().appearance;
+        assert_eq!(effects.effects_at(item).unwrap()[0].params["moveH"].as_f64(), Some(35.0));
+        assert_eq!(effects.effects_at(item).unwrap()[0].params["copies"].as_f64(), Some(2.0));
+        app.run("edit.undo", json!({})).unwrap();
+        app.run("effect.dialog", json!({"effect": "distort.transform", "item": item})).unwrap();
+        assert_eq!(app.ui.dialog.as_ref().unwrap().kind, "effectExists");
+        crate::dialogs::confirm(&mut app).unwrap();
+        let d = app.ui.dialog.as_mut().unwrap();
+        assert_eq!(d.f64("moveH", 0.0), 12.0);
+        d.fields.insert("moveH".into(), json!(60));
+        crate::tests_labels::painted_text(&mut app, |app, ui| crate::dialogs::show(app, ui.ctx()));
+        crate::dialogs::cancel(&mut app);
+        let effects = &app.session.doc().unwrap().doc.node(vectorcraft_doc::NodeId(compound)).unwrap().appearance;
+        assert_eq!(effects.effects_at(item).unwrap()[0].params["moveH"].as_f64(), Some(12.0));
+    }
+}
+
+#[test]
+fn transform_effect_dialog_keeps_the_active_items_selection_anchor() {
+    for item in [None, Some(json!(0)), Some(Value::Null)] {
+        let mut app = app_with_rect();
+        let back = app.session.doc().unwrap().selection.subjects()[0];
+        for at in [json!(0), Value::Null] {
+            app.run("effect.apply", json!({"effect": "distort.transform", "item": at, "params": {"moveH": 99}})).unwrap();
+        }
+        let front = app.run("shape.rectangle", json!({"x": 30, "y": 30, "width": 20, "height": 20})).unwrap()["id"].clone();
+        for at in [json!(0), Value::Null] {
+            app.run("effect.apply", json!({"effect": "distort.transform", "item": at, "params": {"moveH": 12}})).unwrap();
+        }
+        app.run("select.set", json!({"ids": [front, back.0]})).unwrap();
+        app.run("appearance.setActiveItem", json!({"index": 0})).unwrap();
+        let mut p = json!({"effect": "distort.transform"});
+        if let Some(item) = &item {
+            p["item"] = item.clone();
+        }
+        app.run("effect.dialog", p).unwrap();
+        assert_eq!(app.ui.dialog.as_ref().unwrap().kind, "effectExists");
+        crate::dialogs::confirm(&mut app).unwrap();
+        let expected = if item.is_none() { 12.0 } else { 99.0 };
+        assert_eq!(app.ui.dialog.as_ref().unwrap().f64("moveH", 0.0), expected);
+    }
+}
+
+#[test]
+fn explicit_effect_items_use_paint_order_with_a_selected_compound_member() {
+    let mut app = app_with_rect();
+    let back = app.session.doc().unwrap().selection.subjects()[0];
+    for item in [json!(0), Value::Null] {
+        app.run("effect.apply", json!({"effect": "distort.transform", "item": item, "params": {"moveH": 99}})).unwrap();
+    }
+    let member = app.run("shape.rectangle", json!({"x": 20, "y": 20, "width": 80, "height": 80})).unwrap()["id"].clone();
+    let inner = app.run("shape.rectangle", json!({"x": 40, "y": 40, "width": 20, "height": 20})).unwrap()["id"].clone();
+    app.run("select.set", json!({"ids": [member, inner]})).unwrap();
+    app.run("object.compoundPath.make", json!({})).unwrap();
+    for item in [json!(0), Value::Null] {
+        app.run("effect.apply", json!({"effect": "distort.transform", "item": item, "params": {"moveH": 12}})).unwrap();
+    }
+    app.run("select.set", json!({"ids": [member, back.0]})).unwrap();
+    for item in [json!(0), Value::Null] {
+        app.run("effect.dialog", json!({"effect": "distort.transform", "item": item, "index": 0})).unwrap();
+        assert_eq!(app.ui.dialog.as_ref().unwrap().f64("moveH", 0.0), 99.0);
+        crate::dialogs::cancel(&mut app);
+    }
+}
+
+#[test]
+fn editing_an_active_effect_keeps_aligned_rows_in_mixed_stacks() {
+    let mut app = app_with_rect();
+    let back = app.session.doc().unwrap().selection.subjects()[0];
+    app.run("appearance.addStroke", json!({})).unwrap();
+    app.run("appearance.moveItem", json!({"from": 2, "to": 0})).unwrap();
+    app.run("effect.apply", json!({"effect": "distort.transform", "item": 0, "params": {"moveH": 99}})).unwrap();
+    app.run("effect.apply", json!({"effect": "distort.transform", "item": 1, "params": {"moveH": 12}})).unwrap();
+    let front = app.run("shape.rectangle", json!({"x": 30, "y": 30, "width": 20, "height": 20})).unwrap()["id"].as_u64().unwrap();
+    app.run("effect.apply", json!({"effect": "distort.transform", "item": 0, "params": {"moveH": 12}})).unwrap();
+    app.run("select.set", json!({"ids": [front, back.0]})).unwrap();
+    app.run("appearance.setActiveItem", json!({"index": 0})).unwrap();
+    app.run("effect.dialog", json!({"effect": "distort.transform", "index": 0})).unwrap();
+    let d = app.ui.dialog.as_mut().unwrap();
+    assert_eq!(d.f64("moveH", 0.0), 12.0);
+    d.fields.insert("moveH".into(), json!(35));
+    app.run("appearance.setActiveItem", json!({"index": null})).unwrap();
+    crate::tests_labels::painted_text(&mut app, |app, ui| crate::dialogs::show(app, ui.ctx()));
+    crate::dialogs::confirm(&mut app).unwrap();
+    let doc = &app.session.doc().unwrap().doc;
+    let front = &doc.node(vectorcraft_doc::NodeId(front)).unwrap().appearance;
+    let back = &doc.node(back).unwrap().appearance;
+    assert_eq!(front.effects_at(Some(0)).unwrap()[0].params["moveH"], 35);
+    assert_eq!(back.effects_at(Some(1)).unwrap()[0].params["moveH"], 35);
+    assert_eq!(back.effects_at(Some(0)).unwrap()[0].params["moveH"], 99);
+}
+
+#[test]
+fn editing_an_effect_keeps_its_original_item_when_the_active_row_changes() {
+    for prompt in [false, true] {
+        let mut app = app_with_rect();
+        let id = app.session.doc().unwrap().selection.subjects()[0];
+        app.run("effect.apply", json!({"effect": "distort.transform", "item": null, "params": {"moveH": 99}})).unwrap();
+        app.run("effect.apply", json!({"effect": "distort.transform", "item": 0, "params": {"moveH": 12}})).unwrap();
+        app.run("appearance.setActiveItem", json!({"index": 0})).unwrap();
+        let mut p = json!({"effect": "distort.transform"});
+        if !prompt {
+            p["index"] = json!(0);
+        }
+        app.run("effect.dialog", p).unwrap();
+        app.run("appearance.setActiveItem", json!({"index": null})).unwrap();
+        if prompt {
+            crate::dialogs::confirm(&mut app).unwrap();
+        }
+        let d = app.ui.dialog.as_mut().unwrap();
+        assert_eq!(d.f64("moveH", 0.0), 12.0);
+        d.fields.insert("moveH".into(), json!(35));
+        crate::tests_labels::painted_text(&mut app, |app, ui| crate::dialogs::show(app, ui.ctx()));
+        crate::dialogs::confirm(&mut app).unwrap();
+        let appearance = &app.session.doc().unwrap().doc.node(id).unwrap().appearance;
+        assert_eq!(appearance.effects_at(Some(0)).unwrap()[0].params["moveH"].as_f64(), Some(35.0));
+        assert_eq!(appearance.effects[0].params["moveH"].as_f64(), Some(99.0));
+    }
+}
+
+#[test]
+fn new_effects_keep_implicit_item_targeting_until_confirmation() {
+    for prompt in [false, true] {
+        let mut app = app_with_rect();
+        let id = app.session.doc().unwrap().selection.subjects()[0];
+        app.run("effect.apply", json!({"effect": "distort.transform", "item": null, "params": {"moveH": 99}})).unwrap();
+        app.run("effect.apply", json!({"effect": "distort.transform", "item": 0, "params": {"moveH": 12}})).unwrap();
+        app.run("appearance.setActiveItem", json!({"index": 0})).unwrap();
+        app.run("effect.dialog", json!({"effect": if prompt { "distort.transform" } else { "distort.roughen" }})).unwrap();
+        app.run("appearance.setActiveItem", json!({"index": null})).unwrap();
+        if prompt {
+            app.ui.dialog.as_mut().unwrap().fields.insert("discard".into(), json!(true));
+            crate::dialogs::confirm(&mut app).unwrap();
+        }
+        crate::dialogs::confirm(&mut app).unwrap();
+        let appearance = &app.session.doc().unwrap().doc.node(id).unwrap().appearance;
+        assert_eq!(appearance.effects_at(Some(0)).unwrap().len(), 1);
+        assert_eq!(appearance.effects.len(), 2);
+    }
+}
+
 #[test]
 fn transform_effect_typed_scale_changes_the_painted_object() {
     for layer_target in [false, true] {

crates/ui-egui/src/panels/tests_effectedit.rs

@@ -59,7 +59,10 @@ fn an_effect_name_opens_its_dialog_prefilled_and_ok_edits_it_in_place() {
     let texts = frame_events(&ctx, &mut app, vec![], appearance::show);
     click(&ctx, &mut app, text_rect(&texts, "Roughen").center(), appearance::show);
     let d = app.ui.dialog.clone().expect("the effect's dialog");
-    assert_eq!((d.kind.as_str(), d.f64("size", 0.0), d.fields["__index"].clone()), ("effect", 9.0, json!(0)));
+    assert_eq!(
+        (d.kind.as_str(), d.f64("size", 0.0), d.fields["__index"].clone(), d.fields["__item"].clone()),
+        ("effect", 9.0, json!(0), Value::Null)
+    );
     // The dialog previews; OK keeps the new value on the same effect as one undo step.
     let mut out = ctx.run_ui(Default::default(), |ui| crate::dialogs::show(&mut app, ui.ctx()));
     out.textures_delta.clear();
@@ -115,9 +118,10 @@ fn choosing_an_applied_effect_asks_to_edit_it_or_add_another() {
     assert_eq!(app.run("effect.dialog", json!({"effect": "distort.roughen"})).unwrap(), json!({"pending": "effectExists"}));
     crate::dialogs::confirm(&mut app).unwrap();
     let d = app.ui.dialog.clone().unwrap();
-    assert_eq!((d.kind.as_str(), d.f64("size", 0.0), d.fields["__index"].clone(), d.fields["__item"].clone()), ("effect", 9.0, json!(0), json!(0)));
+    assert_eq!((d.kind.as_str(), d.f64("size", 0.0), d.fields["__index"].clone()), ("effect", 9.0, json!(0)));
     crate::dialogs::confirm(&mut app).unwrap();
     assert_eq!(effects(&app, Some(0)).len(), 1);
+    assert!(effects(&app, None).is_empty());
     // "Add New": a fresh dialog with the defaults, which adds a second one.
     app.run("effect.dialog", json!({"effect": "distort.roughen"})).unwrap();
     app.ui.dialog.as_mut().unwrap().fields.insert("discard".into(), json!(true));