From 0365cf3ae3926b934adacdcc3e937242ff4ad4c7 Mon Sep 17 00:00:00 2001 From: ham9zah Date: Tue, 27 Oct 2026 10:19:07 +0300 Subject: [PATCH] Appearance: shared rows map one by one, and effect dialogs edit each copy (#4174) Review follow-ups: - items_per_target maps each requested row on its own. Rows deleted or duplicated together, one shared and one not, fell back to the literal indices in every object, so the other objects could lose the wrong items. The first object takes its own rows; each other object takes its copies of the shared ones only. - effect.dialog takes `aligned` (the panel passes it on a mixed selection's shared effect row), and its OK and preview run effect.setParams with it, so the dialog edits each object's own copy of the effect instead of the first object's index in every object. Tests: removing a shared stroke with an unshared fill leaves the other object's fills; an aligned dialog's command carries `aligned`. --- crates/doc/src/appearance/tests.rs | 7 ++----- crates/engine/src/cmd/appearance.rs | 11 +++++----- crates/engine/src/tests/appearance.rs | 13 ++++++++++++ crates/ui-egui/src/dialogs/effect.rs | 28 +++++++++++++++++++++++++ crates/ui-egui/src/menus/ui_commands.rs | 2 +- crates/ui-egui/src/panels/appearance.rs | 4 ++-- 6 files changed, 52 insertions(+), 13 deletions(-) diff --git a/crates/doc/src/appearance/tests.rs b/crates/doc/src/appearance/tests.rs index 5225c7152..3e3d610f4 100644 --- a/crates/doc/src/appearance/tests.rs +++ b/crates/doc/src/appearance/tests.rs @@ -326,11 +326,8 @@ fn mixed_appearances_share_their_equal_rows() { assert_eq!(shared.items, vec![vec![1, 1]], "the stroke, not the fills"); assert_eq!(shared.effects, vec![vec![0, 0]]); // The stroke under an extra fill in the second object. - let c = Appearance { - items: vec![fill(Color::BLACK), fill(Color::WHITE), stroke.clone()], - effects: vec![feather.clone()], - ..Appearance::default() - }; + let c = + Appearance { items: vec![fill(Color::BLACK), fill(Color::WHITE), stroke.clone()], effects: vec![feather.clone()], ..Appearance::default() }; let shared = SharedStack::of(&[&a, &c]); assert_eq!(shared.items, vec![vec![1, 2]]); assert_eq!(shared.item_in(1, 1), Some(2)); diff --git a/crates/engine/src/cmd/appearance.rs b/crates/engine/src/cmd/appearance.rs index 8c1678ec9..3e1173d8b 100644 --- a/crates/engine/src/cmd/appearance.rs +++ b/crates/engine/src/cmd/appearance.rs @@ -313,12 +313,13 @@ pub(crate) fn appearances_differ(d: &vectorcraft_doc::Document, ids: &[NodeId]) aps.any(|a| a != first) } -/// Indices `idx` of the first target's items (aligned with `aligned`) in each of `ids`: where -/// each target has the item equal to the first's (see [`SharedStack`]), else the same indices. -/// A target missing one of them gets only those it has. +/// Indices `idx` of the first target's items (aligned with `aligned`) in each of `ids`: the first +/// target's own, and in each other one the items equal to them there (see [`SharedStack`]), each +/// index on its own: one the targets don't share is left out of the others (never taken as the +/// same index there). Not aligned, or one target: the same indices in each. pub(crate) fn items_per_target(d: &vectorcraft_doc::Document, ids: &[NodeId], idx: &[usize], aligned: bool) -> Vec> { - match shared_stack(d, ids).filter(|sh| aligned && idx.iter().all(|i| sh.has_item(*i))) { - Some(sh) => (0..ids.len()).map(|k| idx.iter().filter_map(|i| sh.item_in(*i, k)).collect()).collect(), + match shared_stack(d, ids).filter(|_| aligned) { + Some(sh) => (0..ids.len()).map(|k| if k == 0 { idx.to_vec() } else { idx.iter().filter_map(|i| sh.item_in(*i, k)).collect() }).collect(), None => vec![idx.to_vec(); ids.len()], } } diff --git a/crates/engine/src/tests/appearance.rs b/crates/engine/src/tests/appearance.rs index 0530b3d28..d6336b86d 100644 --- a/crates/engine/src/tests/appearance.rs +++ b/crates/engine/src/tests/appearance.rs @@ -585,3 +585,16 @@ fn shared_effects_and_new_fills_reach_every_selected_object() { // Rows of different stacks aren't reordered together. assert!(s.execute("appearance.moveItem", &json!({"from": 1, "to": 0, "aligned": true})).is_err()); } + +/// Rows deleted together, one shared (the stroke) and one not (a's fill): each object loses its +/// own copy of the shared row, and the other object keeps the items at those indices of a's stack. +#[test] +fn removing_shared_and_unshared_rows_removes_only_the_shared_ones_elsewhere() { + let mut s = session(); + let (a, b) = mixed_circles(&mut s, true); + // b is [Fill, Fill, Stroke]; a's rows 0 (its fill) and 1 (the shared stroke). + let fills_b: Vec<_> = node(&s, b).appearance.items.iter().take(2).cloned().collect(); + run(&mut s, "appearance.removeItem", json!({"indices": [0, 1], "aligned": true})); + assert!(node(&s, a).appearance.items.is_empty(), "a loses both"); + assert_eq!(node(&s, b).appearance.items, fills_b, "b loses only its stroke, keeping both fills"); +} diff --git a/crates/ui-egui/src/dialogs/effect.rs b/crates/ui-egui/src/dialogs/effect.rs index 4bc5bde5b..b668f0791 100644 --- a/crates/ui-egui/src/dialogs/effect.rs +++ b/crates/ui-egui/src/dialogs/effect.rs @@ -74,6 +74,10 @@ pub fn open(app: &mut VectorcraftApp, p: &Value) -> Result { if let Some(item) = p.get("item") { fields.insert("__item".into(), item.clone()); } + // A shared row of a mixed selection: OK edits each object's own copy of the effect. + if p.get("aligned").and_then(Value::as_bool) == Some(true) { + fields.insert("__aligned".into(), json!(true)); + } match p.get("index").and_then(Value::as_u64) { Some(k) => { let e = applied.get(k as usize).ok_or_else(|| format!("no effect at index {k}"))?; @@ -737,6 +741,9 @@ fn command(d: &Dialog) -> (&'static str, Value) { if super::warp_effect::is_warp(&id) && d.bool("__warp_style_changed") { p["warpStyle"] = json!(id.strip_prefix("warp.")); } + if d.bool("__aligned") { + p["aligned"] = json!(true); + } return ("effect.setParams", p); } let mut p = json!({"effect": d.str("__effect"), "params": params}); @@ -766,6 +773,27 @@ fn confirm(app: &mut VectorcraftApp, d: &Dialog) -> Result { mod tests { use super::*; + /// An applied effect's dialog opened on a shared row of a mixed selection (`aligned`) edits + /// each object's own copy on OK; opened otherwise it edits the index as given. + #[test] + fn a_shared_rows_dialog_edits_each_objects_copy() { + let dialog = |aligned: bool| { + let mut fields = Map::new(); + fields.insert("__effect".into(), json!("stylize.feather")); + fields.insert("__index".into(), json!(0)); + fields.insert("__item".into(), Value::Null); + fields.insert("radius".into(), json!(8)); + if aligned { + fields.insert("__aligned".into(), json!(true)); + } + Dialog { kind: "effect".into(), fields } + }; + let (cmd, p) = command(&dialog(true)); + assert_eq!((cmd, &p["aligned"], &p["params"]["radius"]), ("effect.setParams", &json!(true), &json!(8))); + assert!(p["params"].get("__aligned").is_none()); + assert!(command(&dialog(false)).1.get("aligned").is_none()); + } + #[test] fn fields_that_pick_a_value_are_dropdowns_of_its_values() { // The values come from the effect's parameter documentation (`joins: "miter"|"round"|…`). diff --git a/crates/ui-egui/src/menus/ui_commands.rs b/crates/ui-egui/src/menus/ui_commands.rs index 9867228b5..63b60e2b5 100644 --- a/crates/ui-egui/src/menus/ui_commands.rs +++ b/crates/ui-egui/src/menus/ui_commands.rs @@ -506,7 +506,7 @@ pub const UI_COMMANDS: &[(&str, &str, &str, &str)] = &[ "effect.dialog", "Effect…", "", - "{effect: id, index?: int (edit that applied effect in place, prefilled; OK runs effect.setParams), item?: appearance item index|null (the fill/stroke whose effects, null the object's; default: the Appearance panel's active item)} open the effect's dialog with live preview; without `index`, an effect already in that list first opens `effectExists` (confirm edits it, discard adds another) → {pending: \"effectExists\"}", + "{effect: id, index?: int (edit that applied effect in place, prefilled; OK runs effect.setParams), item?: appearance item index|null (the fill/stroke whose effects, null the object's; default: the Appearance panel's active item), aligned?: bool (index and item are rows of the first selected object's stack, which OK edits in each object where it has them, as effect.setParams does)} open the effect's dialog with live preview; without `index`, an effect already in that list first opens `effectExists` (confirm edits it, discard adds another) → {pending: \"effectExists\"}", ), ("ui.paramDialog", "Command Dialog", "", "{command, label?, params} open a parameter dialog for any command"), ( diff --git a/crates/ui-egui/src/panels/appearance.rs b/crates/ui-egui/src/panels/appearance.rs index bc760c5ac..b1337be67 100644 --- a/crates/ui-egui/src/panels/appearance.rs +++ b/crates/ui-egui/src/panels/appearance.rs @@ -774,7 +774,7 @@ fn effect_row(app: &mut VectorcraftApp, ui: &mut Ui, item: Option, k: usi if link(ui, pos2(lx, r.center().y), ("ap-fx-link", item, k), &shown_effect_label(&e.id)) { select_row(app, ui.ctx(), this); if has_options(&e.id) { - app.run("effect.dialog", json!({"effect": e.id, "index": k, "item": item})).ok(); + app.run("effect.dialog", aligned(app, json!({"effect": e.id, "index": k, "item": item}))).ok(); } else { set_pstate(ui.ctx(), &open_key, !open); } @@ -916,7 +916,7 @@ fn effect_editor(app: &mut VectorcraftApp, ui: &mut Ui, item: Option, k: app.run("effect.setParams", aligned(app, json!({"item": item, "index": k, "params": {key: v}}))).ok(); } if edit { - app.run("effect.dialog", json!({"effect": e.id, "index": k, "item": item})).ok(); + app.run("effect.dialog", aligned(app, json!({"effect": e.id, "index": k, "item": item}))).ok(); } }