Merge pull request #1005 from kkilchrist/fix-992-reparent-index

node.reparent: index is the layer's final position

crates/engine/src/cmd/edit.rs

@@ -216,7 +216,13 @@ pub fn specs() -> Vec<CommandSpec> {
             let i = req_id("node.get", p, "id")?;
             Ok(crate::inspect::node_json(s.doc()?, i, 1))
         }),
-        spec!("node.reparent", "Move layer into", "{ids?, parent, index?}", true, reparent),
+        spec!(
+            "node.reparent",
+            "Move layer into",
+            "{ids?, parent, index? (final position among parent's children, bottom first; default top)}",
+            true,
+            reparent
+        ),
         spec!("node.reorder", "Reorder layer", "{id, index}", true, |s, p| {
             let i = req_id("node.reorder", p, "id")?;
             let idx = req_num("node.reorder", p, "index")?.max(0.0) as usize;
@@ -1312,9 +1318,10 @@ fn reparent(s: &mut Session, p: &Value) -> Result<Value> {
     let index = num(p, "index").map(|i| i.max(0.0) as usize);
     let doc = s.doc_mut()?;
     let ids = z_sorted(doc, &roots(doc, &ids));
-    // The layers land together under the first sibling at or above `index` that stays put (none: on
+    // `index` is where the first moved layer ends up among the parent's children (bottom first),
-    // top), wherever they came from.
+    // counted after the move, the way `node.reorder` counts it. The layers land together, keeping
-    let above = index.and_then(|at| doc.get(parent).ok()?.children.iter().skip(at).find(|c| !ids.contains(c)).copied());
+    // their order, under the sibling that stays put and ends up just after them (none: on top).
+    let above = index.and_then(|at| doc.get(parent).ok()?.children.iter().filter(|c| !ids.contains(c)).nth(at).copied());
     for i in &ids {
         // Counted with the layer still in place, as `reparent` expects.
         let at = above.and_then(|a| doc.node(parent)?.children.iter().position(|c| *c == a));

crates/engine/src/panels_behaviour_tests.rs

@@ -99,16 +99,18 @@ fn scene(s: &mut Session) -> Scene {
 
 #[test]
 fn dragging_in_the_layers_panel_reorders_by_the_documented_rules() {
-    // The panel drops layers with `node.reparent {ids, parent, index}`: `index` counts the
+    // The panel drops layers with `node.reparent {ids, parent, index}`: `index` is where the first
-    // parent's children bottom first, as they are before the move.
+    // moved layer ends up among the parent's children, bottom first, counted after the move.
     // (moved, index in F, F's children after (bottom first), derivation)
-    let rows: [(&[&str], usize, [&str; 4], &str); 6] = [
+    let rows: [(&[&str], usize, [&str; 4], &str); 8] = [
-        (&["a"], 4, ["b", "c", "d", "a"], "a layer dragged to the top of its parent's list comes to the front"),
+        (&["a"], 3, ["b", "c", "d", "a"], "a layer dragged to the top of its parent's list comes to the front"),
+        (&["a"], 9, ["b", "c", "d", "a"], "an index past the end is the front"),
         (&["d"], 0, ["d", "a", "b", "c"], "a layer dragged to the bottom of the list goes to the back"),
-        (&["a"], 2, ["b", "a", "c", "d"], "a layer dropped between two layers lands between them"),
+        (&["a"], 1, ["b", "a", "c", "d"], "a layer moved up one place goes up one place (#992)"),
-        (&["a", "b"], 4, ["c", "d", "a", "b"], "several selected layers move together, keeping their order"),
+        (&["a"], 0, ["a", "b", "c", "d"], "silent: a layer asked to stay where it is stays (pinned)"),
-        (&["a", "d"], 2, ["b", "a", "d", "c"], "several layers from apart land together where they are dropped"),
+        (&["a", "b"], 2, ["c", "d", "a", "b"], "several selected layers move together, keeping their order"),
-        (&["b", "c"], 1, ["a", "b", "c", "d"], "silent: dropping a block of layers onto itself changes nothing (pinned)"),
+        (&["a", "d"], 1, ["b", "a", "d", "c"], "several layers from apart land together where they are asked to"),
+        (&["b", "c"], 1, ["a", "b", "c", "d"], "silent: moving a block of layers to where it is changes nothing (pinned)"),
     ];
     for (moved, index, after, why) in rows {
         let mut s = Session::new();
@@ -172,6 +174,32 @@ fn dragging_into_frames_groups_and_auto_layout() {
     );
 }
 
+#[test]
+fn reparent_index_is_the_final_position_in_auto_layout_and_across_parents() {
+    // #992: `index` is the layer's place among the parent's children after the move.
+    let mut s = Session::new();
+    let row = id(&ex(&mut s, "node.create", json!({"type": "frame", "name": "Row", "props": {"layout": "horizontal"}})));
+    let text = |s: &mut Session, t: &str, parent: NodeId| id(&ex(s, "node.create", json!({"type": "text", "name": t, "text": t, "parent": parent})));
+    let (first, second, third) = (text(&mut s, "first", row), text(&mut s, "second", row), text(&mut s, "third", row));
+    let names = |s: &Session, p: NodeId| kids(s, p).iter().map(|i| name(s, *i)).collect::<Vec<_>>();
+    ex(&mut s, "node.reparent", json!({"ids": [first], "parent": row, "index": 1}));
+    assert_eq!(names(&s, row), ["second", "first", "third"], "moved down one place");
+    ex(&mut s, "node.reparent", json!({"ids": [first], "parent": row, "index": 2}));
+    assert_eq!(names(&s, row), ["second", "third", "first"], "moved to the end");
+    ex(&mut s, "node.reparent", json!({"ids": [first], "parent": row, "index": 0}));
+    assert_eq!(names(&s, row), ["first", "second", "third"], "moved to the start");
+    let x = |s: &Session, i: NodeId| s.doc().unwrap().node(i).unwrap().transform.as_coeffs()[4];
+    assert!(x(&s, first) < x(&s, second) && x(&s, second) < x(&s, third), "the flow follows the new order");
+    // Into another parent, `index` means the same thing.
+    let other = id(&ex(&mut s, "node.create", json!({"type": "frame", "name": "Other", "props": {"layout": "horizontal"}})));
+    let (p, q) = (text(&mut s, "p", other), text(&mut s, "q", other));
+    ex(&mut s, "node.reparent", json!({"ids": [second], "parent": other, "index": 1}));
+    assert_eq!(kids(&s, other), [p, second, q], "lands at index 1 of the new parent");
+    ex(&mut s, "node.reparent", json!({"ids": [first, third], "parent": other, "index": 3}));
+    assert_eq!(kids(&s, other), [p, second, q, first, third], "several layers start at index 3, keeping their order");
+    assert!(kids(&s, row).is_empty());
+}
+
 #[test]
 fn renaming_several_layers_by_the_documented_codes() {
     // Three layers named `names`, bottom to top; (names, params, new names, derivation).

crates/ui-egui/src/layers.rs

@@ -1085,8 +1085,10 @@ fn layers(app: &mut FigApp, ui: &mut Ui) {
                     // Above in the list = higher z = after the target among siblings.
                     if let Some(parent) = doc.node(target).and_then(|n| n.parent) {
                         let idx = doc.index_in_parent(target).unwrap_or(0);
-                        let index = if z < 0 { idx + 1 } else { idx };
+                        let gap = if z < 0 { idx + 1 } else { idx };
-                        app.run("node.reparent", json!({"ids": ids, "parent": parent, "index": index}));
+                        // `index` is the final position: the dragged layers below the gap leave it.
+                        let below = doc.node(parent).map_or(0, |p| p.children.iter().take(gap).filter(|c| ids.contains(c)).count());
+                        app.run("node.reparent", json!({"ids": ids, "parent": parent, "index": gap - below}));
                     }
                 }
             }

crates/ui-egui/src/layers_drag_tests.rs

@@ -165,3 +165,26 @@ fn dropping_layers_on_another_page_moves_them_there() {
     let doc = u.app.session.doc().unwrap();
     assert_eq!(doc.ancestors(rect).last().copied(), Some(p2), "moved to page 2");
 }
+
+#[test]
+fn dropping_a_layer_one_place_over_moves_it_one_place() {
+    // #992: the panel drops on a gap between rows; the layer ends up in that gap.
+    let mut s = Session::new();
+    let rect =
+        |s: &mut Session, x: i32| id(s.execute("node.create", &json!({"type": "rectangle", "x": x, "y": 0, "width": 10, "height": 10})).unwrap());
+    let (a, b, c) = (rect(&mut s, 0), rect(&mut s, 20), rect(&mut s, 40));
+    s.execute("select.none", &json!({})).unwrap();
+    let page = s.page().unwrap();
+    let mut u = Ui::new(s);
+    let kids = |u: &Ui| u.app.session.doc().unwrap().node(page).unwrap().children.clone();
+    // The list shows c, b, a top to bottom: `a` dragged onto the top edge of `b` goes just above it.
+    let (from, to) = (u.row_at(a), u.row_at(b) - vec2(0.0, 12.0));
+    u.drag_to(from, to);
+    u.button(to, false);
+    assert_eq!(kids(&u), [b, a, c], "dropped between b and c");
+    // And `c` dragged onto the bottom edge of `a` goes just below it.
+    let (from, to) = (u.row_at(c), u.row_at(a) + vec2(0.0, 12.0));
+    u.drag_to(from, to);
+    u.button(to, false);
+    assert_eq!(kids(&u), [b, c, a], "dropped between a and b");
+}

docs/mcp.md

@@ -196,6 +196,12 @@ Or in a project's `.mcp.json`:
     the CSS `fr` unit: a track never gets less than its content (a fixed-size child bigger than
     its share keeps it; the other `fr` tracks share the rest). `inspect` reports a grid child's
     resolved `gridCell`.
+- Layer order: `node.reparent {ids?, parent, index?}` moves layers into `parent` (keeping their place
+  on the canvas, or taking their place in its auto layout flow) and `node.reorder {id, index}` moves
+  one within its parent. `index` counts the parent's children bottom first (in auto layout, the
+  first in the flow) and is where the (first) moved layer ends up after the move, in the same parent
+  or a new one; several layers land together in their stacking order. No `index`, or one past the
+  end, puts them on top (last in the flow).
 - **Every menu action is a command** (`list_commands`), and most commands act on `ids` when given,
   else on the selection. New layers become the selection.
 - Colours are `"#RRGGBB"` or `"#RRGGBBAA"`. Inside fills and strokes, `color` may also be a hex