From 3146ad2b2f87331f3d3c90bed51ee4fc997e1848 Mon Sep 17 00:00:00 2001 From: Kameron Date: Wed, 21 Oct 2026 11:03:08 -0700 Subject: [PATCH] node.reparent: index is the layer's final position (#992) `node.reparent {ids, parent, index}` read `index` against the parent's children as they were before the move, so within the same parent moving a layer up one place did nothing and asking for two places moved it one. `index` now counts the parent's children after the move, the way `node.reorder` already does, in the same parent or a new one; several layers still land together in their stacking order. The layers panel was the only caller passing an index other than 0. It drops on a gap between rows counted before the move, so it now subtracts the dragged layers below that gap and drags behave as before. Documents `index` in the command's params and in docs/mcp.md. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/edit.rs | 15 +++++-- crates/engine/src/panels_behaviour_tests.rs | 44 +++++++++++++++++---- crates/ui-egui/src/layers.rs | 6 ++- crates/ui-egui/src/layers_drag_tests.rs | 23 +++++++++++ docs/mcp.md | 6 +++ 5 files changed, 80 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index a16fbb38..4ec26887 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -216,7 +216,13 @@ pub fn specs() -> Vec { 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; @@ -1302,9 +1308,10 @@ fn reparent(s: &mut Session, p: &Value) -> Result { 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 - // top), wherever they came from. - let above = index.and_then(|at| doc.get(parent).ok()?.children.iter().skip(at).find(|c| !ids.contains(c)).copied()); + // `index` is where the first moved layer ends up among the parent's children (bottom first), + // counted after the move, the way `node.reorder` counts it. The layers land together, keeping + // 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)); diff --git a/crates/engine/src/panels_behaviour_tests.rs b/crates/engine/src/panels_behaviour_tests.rs index d299b0df..9870aa31 100644 --- a/crates/engine/src/panels_behaviour_tests.rs +++ b/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 - // parent's children bottom first, as they are before the move. + // The panel drops layers with `node.reparent {ids, parent, index}`: `index` is where the first + // 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] = [ - (&["a"], 4, ["b", "c", "d", "a"], "a layer dragged to the top of its parent's list comes to the front"), + let rows: [(&[&str], usize, [&str; 4], &str); 8] = [ + (&["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", "b"], 4, ["c", "d", "a", "b"], "several selected layers move together, keeping their order"), - (&["a", "d"], 2, ["b", "a", "d", "c"], "several layers from apart land together where they are dropped"), - (&["b", "c"], 1, ["a", "b", "c", "d"], "silent: dropping a block of layers onto itself changes nothing (pinned)"), + (&["a"], 1, ["b", "a", "c", "d"], "a layer moved up one place goes up one place (#992)"), + (&["a"], 0, ["a", "b", "c", "d"], "silent: a layer asked to stay where it is stays (pinned)"), + (&["a", "b"], 2, ["c", "d", "a", "b"], "several selected layers move together, keeping their order"), + (&["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::>(); + 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). diff --git a/crates/ui-egui/src/layers.rs b/crates/ui-egui/src/layers.rs index f172b309..81d3fbcb 100644 --- a/crates/ui-egui/src/layers.rs +++ b/crates/ui-egui/src/layers.rs @@ -1075,8 +1075,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 }; - app.run("node.reparent", json!({"ids": ids, "parent": parent, "index": index})); + let gap = if z < 0 { idx + 1 } else { idx }; + // `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})); } } } diff --git a/crates/ui-egui/src/layers_drag_tests.rs b/crates/ui-egui/src/layers_drag_tests.rs index 37002f9f..fb74e155 100644 --- a/crates/ui-egui/src/layers_drag_tests.rs +++ b/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"); +} diff --git a/docs/mcp.md b/docs/mcp.md index dfea95cc..64ff024e 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -190,6 +190,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