From 6d1d94bf111a08372960886a60f10cb15cf4b8f8 Mon Sep 17 00:00:00 2001 From: theironchef <6278036+theironchef@users.noreply.github.com> Date: Wed, 7 Oct 2026 08:09:03 -0700 Subject: [PATCH 1/2] Fix #340: new layers and sublayers share one "Layer N" numbering A new sublayer was named "Layer {node_count}", counting every object, so an empty document's first sublayer was "Layer 1" like its parent and a document with five objects got "Layer 6". Top-level layers counted only top-level names and could repeat a sublayer's. Document::next_layer_name now counts every layer, sublayers included, and skips any layer's name; layer.newSublayer uses it too. Co-Authored-By: Claude Opus 5.5 --- crates/doc/src/lib.rs | 12 ++++++++++-- crates/engine/src/tests.rs | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index fada7e98..e8b59a3e 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -810,11 +810,19 @@ impl Document { self.layers.push(Arc::new(Node::layer(id, &name, LayerColor::Preset((n % LAYER_COLORS.len()) as u8)))); id } + /// The default name of a new layer or sublayer, "Layer N". Layers and sublayers share the + /// numbering: it starts after the number of layers there are and skips any layer's name. pub fn next_layer_name(&self) -> String { - let mut i = self.layers.len() + 1; + let mut names = Vec::new(); + self.walk(|n| { + if n.is_layer() { + names.push(n.name.as_deref().unwrap_or_default()); + } + }); + let mut i = names.len() + 1; loop { let name = format!("Layer {i}"); - if !self.layers.iter().any(|l| l.name.as_deref() == Some(&name)) { + if !names.contains(&name.as_str()) { return name; } i += 1; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 132ec54c..7f8adc4f 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -570,3 +570,20 @@ fn a_panicking_command_rolls_back_instead_of_crashing() { assert!(s.doc().unwrap().doc.node(b).is_some()); assert_eq!(s.journal.last().map(|j| j.0.as_str()), Some("shape.rectangle")); } + +/// New layers and sublayers share one "Layer N" numbering, whatever art the document holds, and +/// never repeat a name. +#[test] +fn new_layers_and_sublayers_are_numbered_together() { + let name = |s: &Session, id: &Value| s.doc().unwrap().doc.node(NodeId(id["id"].as_u64().unwrap())).unwrap().display_name().to_string(); + let mut s = session(); + let sub = s.execute("layer.newSublayer", &json!({})).unwrap(); + assert_eq!(name(&s, &sub), "Layer 2", "not its parent's name"); + for x in [0.0, 60.0, 120.0, 180.0, 240.0] { + rect(&mut s, x, 0.0, 50.0, 50.0); + } + let layer = s.execute("layer.new", &json!({})).unwrap(); + assert_eq!(name(&s, &layer), "Layer 3", "after the sublayer, not a repeat of it"); + let sub = s.execute("layer.newSublayer", &json!({})).unwrap(); + assert_eq!(name(&s, &sub), "Layer 4", "the art doesn't count"); +} From ad8c31b3afbbadcca5141d0b98c3e532b94ed58b Mon Sep 17 00:00:00 2001 From: bflatastic Date: Thu, 8 Oct 2026 01:15:46 +0700 Subject: [PATCH 2/2] Layers: one next_layer_name, in the document (review of #341) The engine kept its own copy of Document::next_layer_name after #353; the document's now counts sublayers too, so the engine uses it. Co-Authored-By: Claude Opus 5.5 --- crates/doc/src/lib.rs | 14 ++++---------- crates/engine/src/cmd/layer.rs | 16 +--------------- 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index e8b59a3e..c2aab92e 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -813,20 +813,14 @@ impl Document { /// The default name of a new layer or sublayer, "Layer N". Layers and sublayers share the /// numbering: it starts after the number of layers there are and skips any layer's name. pub fn next_layer_name(&self) -> String { - let mut names = Vec::new(); + let (mut count, mut taken) = (0, std::collections::HashSet::new()); self.walk(|n| { if n.is_layer() { - names.push(n.name.as_deref().unwrap_or_default()); + count += 1; + taken.extend(n.name.as_deref()); } }); - let mut i = names.len() + 1; - loop { - let name = format!("Layer {i}"); - if !names.contains(&name.as_str()) { - return name; - } - i += 1; - } + (count + 1..).map(|i| format!("Layer {i}")).find(|name| !taken.contains(name.as_str())).unwrap_or_default() } /// Visit every node depth first in paint order (bottom to top). diff --git a/crates/engine/src/cmd/layer.rs b/crates/engine/src/cmd/layer.rs index 7139d008..9fc9353d 100644 --- a/crates/engine/src/cmd/layer.rs +++ b/crates/engine/src/cmd/layer.rs @@ -228,24 +228,10 @@ fn all_layers(d: &Document) -> Vec<&Node> { out } -/// The next unused `Layer N` name, counting sublayers too. -pub(crate) fn next_layer_name(d: &Document) -> String { - let layers = all_layers(d); - let taken: HashSet<&str> = layers.iter().filter_map(|l| l.name.as_deref()).collect(); - let mut i = layers.len() + 1; - loop { - let name = format!("Layer {i}"); - if !taken.contains(name.as_str()) { - return name; - } - i += 1; - } -} - /// A new layer named `name` (default: the next `Layer N`) in the next layer colour. pub(crate) fn make_layer(d: &mut Document, name: Option<&str>) -> Node { let n = all_layers(d).len(); - let name = name.map(str::to_string).unwrap_or_else(|| next_layer_name(d)); + let name = name.map(str::to_string).unwrap_or_else(|| d.next_layer_name()); let id = d.alloc_id(); Node::layer(id, &name, LayerColor::Preset((n % LAYER_COLORS.len()) as u8)) }