Merge pull request #341 from theironchef/fix/sublayer-default-names

Tests: new layers and sublayers share one "Layer N" numbering

crates/doc/src/lib.rs

@@ -810,15 +810,17 @@ 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;
-        loop {
-            let name = format!("Layer {i}");
-            if !self.layers.iter().any(|l| l.name.as_deref() == Some(&name)) {
-                return name;
+        let (mut count, mut taken) = (0, std::collections::HashSet::new());
+        self.walk(|n| {
+            if n.is_layer() {
+                count += 1;
+                taken.extend(n.name.as_deref());
             }
-            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).

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))
 }

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");
+}