From 98ce1b35be50d2f6ee6a4ad2dbedf883d171b6f8 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Sat, 24 Oct 2026 22:52:52 +0200 Subject: [PATCH 1/2] Pasted shapes get the next automatic name instead of keeping the original's (fixes #1290) --- crates/engine/src/cmd/edit.rs | 10 +++++++++- crates/engine/src/cmd/insert.rs | 35 +++++++++++++++++++-------------- crates/engine/src/tests.rs | 23 ++++++++++++++++++++++ 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index fa13825f..cdbb2356 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -358,7 +358,15 @@ fn paste(s: &mut Session, p: &Value) -> Result { let mut shapes = clip.shapes.clone(); reid("edit.paste", doc, &mut shapes)?; remap_media(&mut shapes, &media); - for sh in &mut shapes { + let taken = super::current_shapes(doc, sel).map_or(0, Vec::len); + for (k, sh) in shapes.iter_mut().enumerate() { + // An automatic name ("Rectangle 3") counts on from this slide, as a new shape would; + // a name the user typed stays. + let base = super::insert::auto_name_base(sh); + let numbered = sh.name.strip_prefix(base.as_str()).and_then(|r| r.strip_prefix(' ')); + if numbered.is_some_and(|d| !d.is_empty() && d.bytes().all(|b| b.is_ascii_digit())) { + sh.name = format!("{base} {}", taken + k + 1); + } if let Some(x) = sh.xfrm.as_mut() { x.x += off; x.y += off; diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index a9ab3644..0acf9676 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -149,25 +149,30 @@ fn default_rect(s: &Session, w: f64, h: f64) -> Xfrm { Xfrm::new((size.width - w) / 2.0, (size.height - h) / 2.0, w, h) } +/// The word an automatic shape name starts with ("Rectangle" in "Rectangle 3"). +pub(crate) fn auto_name_base(shape: &Shape) -> String { + match &shape.kind { + ShapeKind::Picture { .. } => "Picture".to_string(), + ShapeKind::Table(_) => "Table".to_string(), + ShapeKind::Chart(_) => "Chart".to_string(), + ShapeKind::Media(m) if m.video => "Video".to_string(), + ShapeKind::Media(_) => "Audio".to_string(), + ShapeKind::Model3d { .. } => "3D Model".to_string(), + _ if shape.text_box => "TextBox".to_string(), + _ => shape + .geom + .preset_name() + .and_then(deckcraft_geom::preset::info) + .map(|i| i.label.split(':').next().unwrap_or(i.label).trim().to_string()) + .unwrap_or_else(|| "Shape".into()), + } +} + pub(crate) fn add_shape(s: &mut Session, cmd: &str, mut shape: Shape, select: bool) -> Result { s.edit(|doc, sel| { shape.id = ShapeId(doc.alloc_id().ok_or_else(|| bad(cmd, NO_ID_SPACE))?); if shape.name.is_empty() { - let base = match &shape.kind { - ShapeKind::Picture { .. } => "Picture".to_string(), - ShapeKind::Table(_) => "Table".to_string(), - ShapeKind::Chart(_) => "Chart".to_string(), - ShapeKind::Media(m) if m.video => "Video".to_string(), - ShapeKind::Media(_) => "Audio".to_string(), - ShapeKind::Model3d { .. } => "3D Model".to_string(), - _ if shape.text_box => "TextBox".to_string(), - _ => shape - .geom - .preset_name() - .and_then(deckcraft_geom::preset::info) - .map(|i| i.label.split(':').next().unwrap_or(i.label).trim().to_string()) - .unwrap_or_else(|| "Shape".into()), - }; + let base = auto_name_base(&shape); let n = super::current_shapes(doc, sel).map(|v| v.len()).unwrap_or(0) + 1; shape.name = format!("{base} {n}"); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index fc5cd2c9..478ef496 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -210,6 +210,29 @@ fn clipboard_copy_paste_shapes_and_slides() { assert!(s.doc().unwrap().doc.validate().is_empty()); } +#[test] +fn pasted_shapes_get_the_next_automatic_name() { + let mut s = session(); + let names = |s: &Session| s.doc().unwrap().current_slide().unwrap().shapes.iter().map(|x| x.name.clone()).collect::>(); + let before = names(&s).len(); + s.execute("shape.insert", &json!({"preset": "rect", "rect": [10, 10, 50, 50]})).unwrap(); + let auto = names(&s)[before].clone(); + s.execute("edit.copy", &json!({})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + let all = names(&s); + let base = auto.rsplit_once(' ').unwrap().0; + assert_eq!(all[before + 1], format!("{base} {}", before + 2)); + assert_eq!(all[before + 2], format!("{base} {}", before + 3)); + + // A name the user chose is not an automatic one and stays on the copy. + let id = s.doc().unwrap().selection.shapes[0].0; + s.execute("shape.rename", &json!({"id": id, "name": "Hero"})).unwrap(); + s.execute("edit.copy", &json!({})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + assert_eq!(names(&s).last().unwrap(), "Hero"); +} + #[test] fn bold_copy_is_html_and_pages_html_pastes_bold() { let mut s = session(); From a7ba879a3c3aef3cde93d68651f6b6f2051bbc89 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Sat, 24 Oct 2026 23:10:35 +0200 Subject: [PATCH 2/2] Pasted shapes are renamed only when the name is already taken on the slide; new shapes skip names in use (fixes #1290) --- crates/engine/src/cmd/edit.rs | 19 +++++++------ crates/engine/src/cmd/insert.rs | 10 +++++-- crates/engine/src/cmd/mod.rs | 4 +++ crates/engine/src/tests.rs | 48 +++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index cdbb2356..84842520 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -358,15 +358,18 @@ fn paste(s: &mut Session, p: &Value) -> Result { let mut shapes = clip.shapes.clone(); reid("edit.paste", doc, &mut shapes)?; remap_media(&mut shapes, &media); - let taken = super::current_shapes(doc, sel).map_or(0, Vec::len); - for (k, sh) in shapes.iter_mut().enumerate() { - // An automatic name ("Rectangle 3") counts on from this slide, as a new shape would; - // a name the user typed stays. - let base = super::insert::auto_name_base(sh); - let numbered = sh.name.strip_prefix(base.as_str()).and_then(|r| r.strip_prefix(' ')); - if numbered.is_some_and(|d| !d.is_empty() && d.bytes().all(|b| b.is_ascii_digit())) { - sh.name = format!("{base} {}", taken + k + 1); + let mut taken = super::current_names(doc, sel); + for sh in &mut shapes { + // Only a name already on the slide is changed (two shapes with one name are what the + // issue is about); otherwise it stays, as Morph pairs shapes across slides by name. + if taken.contains(&sh.name) + && let Some((base, n)) = sh.name.rsplit_once(' ') + && !n.is_empty() + && n.bytes().all(|b| b.is_ascii_digit()) + { + sh.name = super::insert::first_free_name(base, &taken); } + taken.insert(sh.name.clone()); if let Some(x) = sh.xfrm.as_mut() { x.x += off; x.y += off; diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 0acf9676..e8b18147 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -168,13 +168,19 @@ 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() +} + pub(crate) fn add_shape(s: &mut Session, cmd: &str, mut shape: Shape, select: bool) -> Result { s.edit(|doc, sel| { shape.id = ShapeId(doc.alloc_id().ok_or_else(|| bad(cmd, NO_ID_SPACE))?); if shape.name.is_empty() { let base = auto_name_base(&shape); - let n = super::current_shapes(doc, sel).map(|v| v.len()).unwrap_or(0) + 1; - shape.name = format!("{base} {n}"); + let taken = super::current_names(doc, sel); + shape.name = first_free_name(&base, &taken); } let id = shape.id; let list = crate::shapes_mut(doc, sel).ok_or_else(|| bad("insert", "no slide"))?; diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index fa3f6459..280428a5 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -497,6 +497,10 @@ pub(crate) fn current_shapes<'a>(doc: &'a deckcraft_model::Presentation, sel: &c } } +pub(crate) fn current_names(doc: &deckcraft_model::Presentation, sel: &crate::Selection) -> std::collections::HashSet { + current_shapes(doc, sel).map(|v| v.iter().map(|s| s.name.clone()).collect()).unwrap_or_default() +} + /// The command palette runs a command with `{}`. True when the params doc names a value that /// call would have to supply (`path`, `rows`, `path? | data`), so the palette must not offer it. /// A name ending in `?` is optional. Text after `→` is the result, not an input. diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 478ef496..74225af9 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -233,6 +233,54 @@ fn pasted_shapes_get_the_next_automatic_name() { assert_eq!(names(&s).last().unwrap(), "Hero"); } +#[test] +fn pasting_after_a_deleted_shape_does_not_repeat_a_name() { + let mut s = session(); + let names = |s: &Session| s.doc().unwrap().current_slide().unwrap().shapes.iter().map(|x| x.name.clone()).collect::>(); + let mut ids = vec![]; + for _ in 0..3 { + s.execute("shape.insert", &json!({"preset": "rect", "rect": [10, 10, 50, 50]})).unwrap(); + ids.push(s.doc().unwrap().selection.shapes[0].0); + } + s.execute("edit.delete", &json!({"ids": [ids[1]]})).unwrap(); + s.execute("slide.go", &json!({"index": 0})).unwrap(); + s.doc_mut().unwrap().selection.shapes = vec![deckcraft_model::ShapeId(ids[2])]; + s.execute("edit.copy", &json!({})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + let all = names(&s); + let mut unique = all.clone(); + unique.sort(); + unique.dedup(); + assert_eq!(unique.len(), all.len(), "{all:?}"); +} + +#[test] +fn pasting_on_another_slide_keeps_names_so_morph_still_pairs_them() { + let mut s = session(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + s.execute("shape.insert", &json!({"preset": "rect", "rect": [0, 0, 20, 20]})).unwrap(); + let first = s.doc().unwrap().selection.shapes[0].0; + s.execute("shape.insert", &json!({"preset": "rect", "rect": [10, 10, 50, 50]})).unwrap(); + s.execute("shape.insert", &json!({"preset": "rect", "rect": [200, 200, 80, 80]})).unwrap(); + s.execute("edit.delete", &json!({"ids": [first]})).unwrap(); + s.execute("edit.selectAll", &json!({})).unwrap(); + s.execute("edit.copy", &json!({})).unwrap(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + let st = s.doc().unwrap(); + let (a, b) = (&st.doc.slides[1], &st.doc.slides[2]); + let an: Vec<_> = a.shapes.iter().map(|x| x.name.clone()).collect(); + let bn: Vec<_> = b.shapes.iter().map(|x| x.name.clone()).collect(); + assert_eq!(an, bn); + let pairs = deckcraft_anim::morph_pairs(a, b); + assert_eq!(pairs.len(), 2); + for (ia, ib) in pairs { + let na = &a.shapes.iter().find(|x| x.id == ia).unwrap().name; + let nb = &b.shapes.iter().find(|x| x.id == ib).unwrap().name; + assert_eq!(na, nb); + } +} + #[test] fn bold_copy_is_html_and_pages_html_pastes_bold() { let mut s = session();