Merge pull request #4863 from dotCooCoo/fix/add-new-fill-above-selected

Appearance: Add New Fill copies the selected fill row right above it

crates/engine/src/cmd/appearance.rs

@@ -44,7 +44,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Add New Fill",
             ["Window", "Appearance"],
             Some("Cmd+/"),
-            "{ids?|id?, target?: \"object\"|\"contents\"} add a fill on top of each selected object's own stack, on an image right below its Contents row (a copy of its top fill; type without one: its characters' fill; else the default fill)",
+            "{ids?|id?, target?: \"object\"|\"contents\"} add a fill on top of each selected object's own stack, on an image right below its Contents row (a copy of its top fill; type without one: its characters' fill; else the default fill). With a fill row active (appearance.setActiveItem), no `ids` and no `target: \"contents\"`, the new fill is a copy of that row, right above it, and becomes the active row",
             has_doc,
             |s, p| add_item(s, p, true)
         ),
@@ -470,12 +470,45 @@ fn set_active_item(s: &mut Session, p: &Value) -> Result<Value> {
 fn add_item(s: &mut Session, p: &Value, fill: bool) -> Result<Value> {
     let ids = require_targets(s, p, if fill { "appearance.addFill" } else { "appearance.addStroke" })?;
     let defaults = s.paint.clone();
+    // The panel's active Fill row, when the new fill goes on the selection: the new fill is a copy
+    // of that row, right above it in each target that has it (others get a copy of their top fill).
     // Where the new item went in on the active item's object, when it is an image.
     let active = s.active_appearance_item.as_ref().and_then(|a| a.objects.first().copied());
+    // The active row is a row of the active object's stack, which need not come first in `ids`
+    // (they are in paint order): match the others against it.
+    let anchor = active.and_then(|a| ids.iter().position(|id| *id == a));
+    let selected = match (s.appearance_item(), anchor) {
+        (Some(index), Some(_)) if fill && p.get("ids").is_none() && p.get("id").is_none() && !targets_contents(p)? => {
+            ItemTarget::Item { index, explicit: false, aligned: true }.of_kind(s, true)
+        }
+        _ => ItemTarget::Top,
+    };
+    let per: Vec<Option<usize>> = match (selected, anchor) {
+        (ItemTarget::Item { index, .. }, Some(k)) => {
+            let mut order = ids.clone();
+            order.swap(0, k);
+            let mut per: Vec<Option<usize>> =
+                items_per_target(&s.doc()?.doc, &order, &[index], true).into_iter().map(|v| v.first().copied()).collect();
+            per.swap(0, k);
+            per
+        }
+        _ => vec![None; ids.len()],
+    };
     let mut inserted = None;
+    // Where the new fill went in on the active item's object, when it went above the active row.
+    let mut above = None;
     s.edit(if fill { "Add New Fill" } else { "Add New Stroke" }, |d, _| {
-        for id in &ids {
+        for (id, row) in ids.iter().zip(&per) {
             if let Some(n) = d.node_mut(*id) {
+                if let Some(i) = n.appearance.item_of_kind(*row, true)
+                    && let Some(paint) = n.appearance.paint_at(Some(i), true).cloned()
+                {
+                    n.appearance.insert_item(i + 1, AppearanceItem::Fill(FillLayer::new(paint)));
+                    if active == Some(*id) {
+                        above = Some(i + 1);
+                    }
+                    continue;
+                }
                 // A copy of the top fill (stroke); without one, type's characters' and otherwise the
                 // default paint, so a new fill on a group or type shows.
                 let paint = match (n.appearance.paint_at(None, fill), &n.kind) {
@@ -509,8 +542,9 @@ fn add_item(s: &mut Session, p: &Value, fill: bool) -> Result<Value> {
         }
         Ok(())
     })?;
-    // The rows above the new item move up by one.
-    s.remap_appearance_item(&ids, |a| Some(a + usize::from(inserted.is_some_and(|k| k <= a))));
+    // A fill added above the active row becomes the active row; otherwise the rows above the new
+    // item move up by one.
+    s.remap_appearance_item(&ids, |a| above.or(Some(a + usize::from(inserted.is_some_and(|k| k <= a)))));
     ok()
 }
 

crates/engine/src/tests/appearance.rs

@@ -288,6 +288,74 @@ fn add_fill_and_stroke_copy_the_top_ones() {
     assert_eq!(node(&s, id).appearance.items.len(), 3);
 }
 
+/// With a fill row active, Add New Fill puts a copy of that row right above it and makes the copy
+/// the active row; the rows above it stay where they are.
+#[test]
+fn add_fill_copies_the_active_fill_row_right_above_it() {
+    let mut s = session();
+    let id = rect(&mut s, 100.0);
+    run(&mut s, "paint.setFill", json!({"color": "#ff0000"}));
+    run(&mut s, "appearance.addFill", json!({}));
+    run(&mut s, "appearance.setItem", json!({"index": 2, "color": "#0000ff"})); // [red, Stroke, blue]
+    run(&mut s, "appearance.setActiveItem", json!({"index": 0}));
+    let steps = s.doc().unwrap().history.undo.len();
+    run(&mut s, "appearance.addFill", json!({}));
+    let n = node(&s, id);
+    assert_eq!(n.appearance.items.iter().map(AppearanceItem::kind_name).collect::<Vec<_>>(), ["fill", "fill", "stroke", "fill"]);
+    let hex = |i: usize| fill_paint(&n, i).color().unwrap().to_hex();
+    assert_eq!((hex(0), hex(1), hex(3)), ("#ff0000".to_string(), "#ff0000".to_string(), "#0000ff".to_string()));
+    assert_eq!(s.appearance_item(), Some(1), "the new fill is the active row");
+    // The next edit goes to the new fill.
+    run(&mut s, "paint.setFill", json!({"color": "#00ff00"}));
+    let n = node(&s, id);
+    assert_eq!((fill_paint(&n, 0).color().unwrap().to_hex(), fill_paint(&n, 1).color().unwrap().to_hex()), ("#ff0000".into(), "#00ff00".into()));
+    assert_eq!(s.doc().unwrap().history.undo.len(), steps + 2);
+    run(&mut s, "edit.undo", json!({}));
+    run(&mut s, "edit.undo", json!({}));
+    assert_eq!(node(&s, id).appearance.items.len(), 3);
+    // An active stroke row, or ids, leave the new fill on top.
+    run(&mut s, "appearance.setActiveItem", json!({"index": 1}));
+    run(&mut s, "appearance.addFill", json!({}));
+    assert_eq!(node(&s, id).appearance.items.iter().map(AppearanceItem::kind_name).collect::<Vec<_>>(), ["fill", "stroke", "fill", "fill"]);
+    run(&mut s, "appearance.setActiveItem", json!({"index": 0}));
+    run(&mut s, "appearance.addFill", json!({"ids": [id.0]}));
+    assert_eq!(fill_paint(&node(&s, id), 4).color().unwrap().to_hex(), "#0000ff");
+    assert_eq!(s.appearance_item(), Some(0));
+}
+
+/// In a mixed selection the active fill row goes above itself in the first object; an object that
+/// doesn't share that row gets a copy of its top fill on top, as without an active row.
+#[test]
+fn add_fill_above_an_unshared_active_row_tops_the_other_objects() {
+    let mut s = session();
+    let (a, b) = mixed_circles(&mut s, true); // a [yellow, Stroke], b [red, green, Stroke]
+    run(&mut s, "appearance.setActiveItem", json!({"index": 0}));
+    run(&mut s, "appearance.addFill", json!({}));
+    let (na, nb) = (node(&s, a), node(&s, b));
+    assert_eq!(na.appearance.items.iter().map(AppearanceItem::kind_name).collect::<Vec<_>>(), ["fill", "fill", "stroke"]);
+    assert_eq!(fill_paint(&na, 1).color().unwrap().to_hex(), "#ffff00");
+    assert_eq!(nb.appearance.items.iter().map(AppearanceItem::kind_name).collect::<Vec<_>>(), ["fill", "fill", "stroke", "fill"]);
+    assert_eq!(fill_paint(&nb, 3).color().unwrap().to_hex(), "#00ff00");
+    assert_eq!(s.appearance_item(), Some(1));
+}
+
+/// The active row is a row of the first selected object, also when that object paints above the
+/// others.
+#[test]
+fn add_fill_goes_above_the_active_row_of_the_first_selected_object() {
+    let mut s = session();
+    let (a, b) = mixed_circles(&mut s, true); // a [yellow, Stroke], b [red, green, Stroke]
+    run(&mut s, "select.set", json!({"ids": [b.0, a.0]}));
+    run(&mut s, "appearance.setActiveItem", json!({"index": 1})); // b's green fill
+    run(&mut s, "appearance.addFill", json!({}));
+    let (na, nb) = (node(&s, a), node(&s, b));
+    assert_eq!(nb.appearance.items.iter().map(AppearanceItem::kind_name).collect::<Vec<_>>(), ["fill", "fill", "fill", "stroke"]);
+    assert_eq!(fill_paint(&nb, 2).color().unwrap().to_hex(), "#00ff00");
+    assert_eq!(na.appearance.items.iter().map(AppearanceItem::kind_name).collect::<Vec<_>>(), ["fill", "stroke", "fill"]);
+    assert_eq!(fill_paint(&na, 2).color().unwrap().to_hex(), "#ffff00");
+    assert_eq!(s.appearance_item(), Some(2));
+}
+
 #[test]
 fn set_item_and_remove_item() {
     let mut s = session();

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

@@ -88,15 +88,16 @@ pub(crate) fn select_row(app: &mut VectorcraftApp, ctx: &egui::Context, sel: Sel
     }
     let owner = app.session.active().and_then(|d| d.selection.subjects().first().copied());
     set_pstate(ctx, "ap-sel", (owner, sel));
-    set_pstate(ctx, "ap-multi", (owner, sel.item().into_iter().collect::<Vec<usize>>()));
+    set_pstate(ctx, "ap-multi", (owner, sel.item().into_iter().collect::<Vec<usize>>(), sel.item()));
 }
 
 /// The selected fill/stroke rows of `n` (paint order, ascending): the active row plus those
-/// Cmd/Shift-clicked with it.
+/// Cmd/Shift-clicked with it. When the active row has changed since (Add New Fill makes the new
+/// fill active), only the active row.
 fn selected_items(ctx: &egui::Context, n: Option<&Node>, sel: Sel) -> Vec<usize> {
     let (Some(n), Sel::Item(active)) = (n, sel) else { return vec![] };
-    let (owner, mut set): (Option<NodeId>, Vec<usize>) = pstate(ctx, "ap-multi");
-    if owner != Some(n.id) || !set.contains(&active) {
+    let (owner, mut set, at): (Option<NodeId>, Vec<usize>, Option<usize>) = pstate(ctx, "ap-multi");
+    if owner != Some(n.id) || at != Some(active) || !set.contains(&active) {
         return vec![active];
     }
     set.retain(|i| *i < n.appearance.items.len());
@@ -130,7 +131,7 @@ fn click_item(app: &mut VectorcraftApp, ctx: &egui::Context, n: &Node, sel: Sel,
         }
     };
     select_row(app, ctx, Sel::Item(active));
-    set_pstate(ctx, "ap-multi", (Some(n.id), set));
+    set_pstate(ctx, "ap-multi", (Some(n.id), set, Some(active)));
 }
 
 /// An effect as the fx menu lists it: (id, label without the ellipsis, Effect-menu path).

crates/ui-egui/src/panels/appearance/tests.rs

@@ -438,6 +438,28 @@ fn groups_show_a_contents_row_and_type_a_characters_row_in_their_slot() {
     assert_eq!(rows(frame(&mut app)), ["Fill:", "Characters"]);
 }
 
+/// Add New Fill (button, menu or shortcut) with rows selected: the copy of the active row lands
+/// right above it and is then the only selected row.
+#[test]
+fn add_new_fill_selects_the_new_fill_alone() {
+    let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());
+    app.run("file.new", json!({"width": 100, "height": 100})).unwrap();
+    let id = NodeId(app.run("shape.rectangle", json!({"x": 0, "y": 0, "width": 20, "height": 20})).unwrap()["id"].as_u64().unwrap());
+    app.run("appearance.addFill", json!({})).unwrap(); // [Fill, Stroke, Fill]
+    let ctx = egui::Context::default();
+    let n = app.session.doc().unwrap().doc.node(id).unwrap().clone();
+    // Rows 0 and 1 (the stroke) Cmd-clicked together, row 0 active.
+    click_item(&mut app, &ctx, &n, Sel::None, 0, egui::Modifiers::NONE);
+    click_item(&mut app, &ctx, &n, Sel::Item(0), 1, egui::Modifiers::COMMAND);
+    assert_eq!(selected_items(&ctx, Some(&n), Sel::Item(0)), [0, 1]);
+    app.run("appearance.addFill", json!({})).unwrap();
+    let n = app.session.doc().unwrap().doc.node(id).unwrap().clone();
+    assert_eq!(n.appearance.items.iter().map(AppearanceItem::kind_name).collect::<Vec<_>>(), ["fill", "fill", "stroke", "fill"]);
+    let sel = current_sel(&app, &ctx, Some(&n));
+    assert_eq!(sel, Sel::Item(1));
+    assert_eq!(selected_items(&ctx, Some(&n), sel), [1]);
+}
+
 #[test]
 fn images_show_a_contents_row_with_new_fills_and_strokes_below_it() {
     let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());