From ddd7d210ef71874ba7c0ba96c0036eef98ad531d Mon Sep 17 00:00:00 2001 From: Harrison Booth <73434116+Harrison-Booth@users.noreply.github.com> Date: Tue, 27 Oct 2026 21:02:22 -0500 Subject: [PATCH] Duplicate: number the copy from the shape count, not the count of distinct names --- crates/engine/src/cmd/edit.rs | 13 +++++++++---- crates/engine/src/cmd/insert.rs | 9 +++++---- crates/engine/src/tests.rs | 20 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 8a220c17..ff69575f 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -330,7 +330,7 @@ fn paste(s: &mut Session, p: &Value) -> Result { && !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit()) { - sh.name = super::insert::first_free_name(base, &taken); + sh.name = super::insert::first_free_name(base, &taken, taken.len()); } taken.insert(sh.name.clone()); if let Some(x) = sh.xfrm.as_mut() { @@ -512,11 +512,11 @@ fn paste_text(s: &mut Session, p: &Value) -> Result { /// PowerPoint does, and keeps a name the user chose. PowerPoint tells the two apart by comparing /// the number with the shape's id; ids here are numbered across the deck, so any "{kind} {n}" /// counts as automatic. -fn rename_automatic(sh: &mut Shape, taken: &mut std::collections::HashSet) { +fn rename_automatic(sh: &mut Shape, taken: &mut std::collections::HashSet, count: usize) { let base = if sh.is_group() { "Group".to_string() } else { super::insert::auto_name_base(sh) }; let n = sh.name.strip_prefix(&base).and_then(|r| r.strip_prefix(' ')); if n.is_some_and(|n| !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit())) { - sh.name = super::insert::first_free_name(&base, taken); + sh.name = super::insert::first_free_name(&base, taken, count); taken.insert(sh.name.clone()); } } @@ -547,10 +547,15 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { reid("edit.duplicate", doc, &mut copies)?; // Every name on the slide, group members included, so a copy never repeats one. let mut taken = std::collections::HashSet::new(); + let mut count = 0; deckcraft_model::walk(&list, &mut |sh, _| { taken.insert(sh.name.clone()); + count += 1; + }); + walk_mut(&mut copies, &mut |c| { + rename_automatic(c, &mut taken, count); + count += 1; }); - walk_mut(&mut copies, &mut |c| rename_automatic(c, &mut taken)); for (c, b) in copies.iter_mut().zip(boxes) { let mut x = b; x.x += dx; diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index f2a2b9bd..95af342c 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -172,9 +172,10 @@ pub(crate) fn auto_name_base(shape: &Shape) -> String { } /// "{base} n" with the first `n` from the shape count + 1 that no shape on the slide already uses, -/// so a gap left by a deleted shape never makes a second shape with the same name. -pub(crate) fn first_free_name(base: &str, taken: &std::collections::HashSet) -> String { - (taken.len() + 1..).map(|n| format!("{base} {n}")).find(|c| !taken.contains(c)).unwrap_or_default() +/// so a gap left by a deleted shape never makes a second shape with the same name. `count` is the +/// number of shapes, which exceeds the number of names when two shapes share one. +pub(crate) fn first_free_name(base: &str, taken: &std::collections::HashSet, count: usize) -> String { + (count + 1..).map(|n| format!("{base} {n}")).find(|c| !taken.contains(c)).unwrap_or_default() } pub(crate) fn add_shape(s: &mut Session, cmd: &str, mut shape: Shape, select: bool) -> Result { @@ -183,7 +184,7 @@ pub(crate) fn add_shape(s: &mut Session, cmd: &str, mut shape: Shape, select: bo if shape.name.is_empty() { let base = auto_name_base(&shape); let taken = super::current_names(doc, sel); - shape.name = first_free_name(&base, &taken); + shape.name = first_free_name(&base, &taken, taken.len()); } let id = shape.id; let list = crate::shapes_mut(doc, sel).ok_or_else(|| bad("insert", "no slide"))?; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 133e5a46..b2298100 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -268,6 +268,26 @@ fn duplicate_gives_the_next_automatic_name_and_keeps_a_chosen_one() { assert_eq!(&all[n + 2..], ["Hero", "Hero"], "{all:?}"); } +/// Two shapes sharing a chosen name are still two shapes: the next number follows the count. +#[test] +fn duplicate_numbers_from_the_shape_count_when_names_repeat() { + let mut s = session(); + s.execute("shape.insert", &json!({"preset": "rect", "rect": [10, 10, 50, 50]})).unwrap(); + let rect = s.doc().unwrap().selection.shapes[0].0; + s.execute("shape.insert", &json!({"preset": "rect", "rect": [100, 10, 50, 50]})).unwrap(); + let hero = s.doc().unwrap().selection.shapes[0].0; + s.execute("shape.rename", &json!({"id": hero, "name": "Hero"})).unwrap(); + s.execute("edit.duplicate", &json!({"ids": [hero]})).unwrap(); + let before = s.doc().unwrap().current_slide().unwrap().shapes.len(); + s.execute("edit.duplicate", &json!({"ids": [rect]})).unwrap(); + let doc = s.doc().unwrap(); + let shapes = &doc.current_slide().unwrap().shapes; + let source = shapes.iter().find(|x| x.id.0 == rect).unwrap(); + let base = source.name.rsplit_once(' ').unwrap().0; + let names: Vec<_> = shapes.iter().map(|x| x.name.as_str()).collect(); + assert_eq!(shapes[before].name, format!("{base} {}", before + 1), "{names:?}"); +} + /// The next automatic name looks at the names inside groups too. #[test] fn duplicating_a_group_does_not_repeat_the_names_of_its_members() {