From 8b1bb0fd0592ce1ace6a118032e07d0ba98a6060 Mon Sep 17 00:00:00 2001 From: kotulsky Date: Sat, 10 Oct 2026 08:01:55 +0200 Subject: [PATCH] fix(engine): select neighbouring layer after deletion (#2189) Co-authored-by: kotulsky <7805437+kotulsky@users.noreply.github.com> Co-authored-by: Brandon Thomas --- crates/engine/src/commands.rs | 5 +- crates/engine/src/layer_multi_cmds.rs | 34 ++++++++++--- crates/engine/src/tests.rs | 69 +++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index d82eb8ef..9449e2c9 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -568,12 +568,13 @@ fn build() -> Vec { } let id = layer_param(s, p)?; s.edit("Delete Layer", |doc, active| { + let neighbours = crate::layer_multi_cmds::deletion_neighbours(doc, id); doc.remove(id).ok_or(EngineError::NoLayer(id))?; if doc.layers.is_empty() { return Err(EngineError::Other("a document must keep at least one layer".into())); } - if *active == Some(id) { - *active = doc.top_layer(); + if active.is_some_and(|current| doc.layer(current).is_none()) { + *active = neighbours.into_iter().find(|candidate| doc.layer(*candidate).is_some()); } Ok(()) })?; diff --git a/crates/engine/src/layer_multi_cmds.rs b/crates/engine/src/layer_multi_cmds.rs index 3ac9f55d..ab0ba36d 100644 --- a/crates/engine/src/layer_multi_cmds.rs +++ b/crates/engine/src/layer_multi_cmds.rs @@ -1121,23 +1121,45 @@ fn merge_layers(s: &mut Session) -> Result { Ok(json!({ "layer": mid.0 })) } +/// Next visible layer row below the deleted target, then rows above in reverse order. +/// Group headers precede their children in the UI, unlike Document::walk's storage order. +pub(crate) fn deletion_neighbours(doc: &Document, target: LayerId) -> Vec { + let rows = |expand_all: bool| { + let mut stack: Vec<&Layer> = doc.layers.iter().collect(); + let mut order = Vec::new(); + while let Some(layer) = stack.pop() { + order.push(layer.id); + if let LayerContent::Group(group) = &layer.content + && (expand_all || group.expanded) + { + stack.extend(group.children.iter()); + } + } + order + }; + let mut order = rows(false); + if !order.contains(&target) { + order = rows(true); + } + let Some(index) = order.iter().position(|id| *id == target) else { + return Vec::new(); + }; + order.iter().skip(index + 1).chain(order.iter().take(index).rev()).copied().collect() +} + /// Delete Layer with several layers selected. pub fn delete_selected(s: &mut Session) -> Result { let sel = selected(s); s.edit("Delete Layers", |doc, active| { let ids = top_level(doc, &sel); - let below = ids.first().and_then(|id| { - let order: Vec = doc.walk().into_iter().map(|(_, _, l)| l.id).collect(); - let i = order.iter().position(|x| x == id)?; - order[..i].iter().rev().find(|x| !sel.contains(x)).copied() - }); + let neighbours = active.map(|id| deletion_neighbours(doc, id)).unwrap_or_default(); for id in &ids { doc.remove(*id).ok_or(EngineError::NoLayer(*id))?; } if doc.layers.is_empty() { return Err(EngineError::Other("a document must keep at least one layer".into())); } - *active = below.filter(|b| doc.layer(*b).is_some()).or_else(|| doc.top_layer()); + *active = neighbours.into_iter().find(|candidate| doc.layer(*candidate).is_some()); Ok(()) })?; Ok(json!({"deleted": sel.len()})) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 5c71341c..4d76ddce 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1284,3 +1284,72 @@ fn document_inspect_and_activate_invalid_index() { let err_act = s.execute("document.activate", json!({"document": 9})).unwrap_err(); assert_eq!(err_act.to_string(), "no document at index 9"); } + +#[test] +fn delete_layer_selects_neighbour_and_undo_restores_layer() { + for deleted_index in 0..3 { + let mut s = session_with_doc(); + let ids = ["bottom", "middle", "top"].map(|name| LayerId(s.execute("layer.new.layer", json!({"name":name})).unwrap()["layer"].as_u64().unwrap())); + if deleted_index == 0 { + let background = s.active().unwrap().doc.layers[0].id; + s.execute("layer.delete", json!({"layer":background.0})).unwrap(); + } + s.execute("layer.select", json!({"layer":ids[deleted_index].0})).unwrap(); + s.execute("layer.delete", json!({})).unwrap(); + let neighbour = if deleted_index == 0 { ids[1] } else { ids[deleted_index - 1] }; + assert_eq!(s.active().unwrap().active_layer, Some(neighbour)); + assert_eq!(s.active().unwrap().selected_layers, vec![neighbour]); + assert!(s.active().unwrap().doc.layer(ids[deleted_index]).is_none()); + s.undo(); + assert!(s.active().unwrap().doc.layer(ids[deleted_index]).is_some()); + assert!(s.active().unwrap().active_layer.is_some_and(|id| s.active().unwrap().doc.layer(id).is_some())); + s.redo(); + assert_eq!(s.active().unwrap().active_layer, Some(neighbour)); + } +} + +#[test] +fn delete_multiple_layers_selects_next_survivor() { + let mut s = session_with_doc(); + let ids = ["bottom", "middle", "top"].map(|name| LayerId(s.execute("layer.new.layer", json!({"name":name})).unwrap()["layer"].as_u64().unwrap())); + s.execute("layer.select", json!({"layer":ids[1].0})).unwrap(); + s.execute("layer.select", json!({"layer":ids[2].0,"mode":"add"})).unwrap(); + s.execute("layer.delete", json!({})).unwrap(); + assert_eq!(s.active().unwrap().active_layer, Some(ids[0])); + assert!(s.active().unwrap().doc.layer(ids[1]).is_none()); + assert!(s.active().unwrap().doc.layer(ids[2]).is_none()); + s.undo(); + assert_eq!(s.active().unwrap().active_layer, Some(ids[2])); +} + +#[test] +fn delete_layer_neighbours_follow_group_row_order() { + let mut s = session_with_doc(); + let background = s.active().unwrap().active_layer.unwrap(); + let lower = LayerId(s.execute("layer.new.layer", json!({"name":"Below group"})).unwrap()["layer"].as_u64().unwrap()); + let child = LayerId(s.execute("layer.new.layer", json!({"name":"Child"})).unwrap()["layer"].as_u64().unwrap()); + let group = LayerId(s.execute("layer.groupLayers", json!({"name":"Group"})).unwrap()["layer"].as_u64().unwrap()); + s.execute("layer.select", json!({"layer":child.0})).unwrap(); + s.execute("layer.delete", json!({})).unwrap(); + assert_eq!(s.active().unwrap().active_layer, Some(lower), "the next row below a group's final child is outside the group"); + s.undo(); + s.execute("layer.delete", json!({"layer":background.0})).unwrap(); + s.execute("layer.setExpanded", json!({"layer":group.0,"expanded":false})).unwrap(); + s.execute("layer.select", json!({"layer":lower.0})).unwrap(); + s.execute("layer.delete", json!({})).unwrap(); + assert_eq!(s.active().unwrap().active_layer, Some(group), "fallback above uses the collapsed group header, not its hidden child"); +} + +#[test] +fn delete_layer_preserves_an_unaffected_active_layer_and_last_layer_guard() { + let mut s = session_with_doc(); + let background = s.active().unwrap().active_layer.unwrap(); + let other = LayerId(s.execute("layer.new.layer", json!({"name":"Other"})).unwrap()["layer"].as_u64().unwrap()); + s.execute("layer.delete", json!({"layer":background.0})).unwrap(); + assert_eq!(s.active().unwrap().active_layer, Some(other)); + let before = s.active().unwrap().history.past_len(); + assert!(s.execute("layer.delete", json!({})).is_err()); + assert_eq!(s.active().unwrap().doc.layer_count(), 1); + assert_eq!(s.active().unwrap().active_layer, Some(other)); + assert_eq!(s.active().unwrap().history.past_len(), before); +}