BLOCKREPLACE: a dynamic block or a replacement that references the block is refused when its name is given, and the prompt asks again

crates/engine/src/cmd/blocks/blockreplace.rs

@@ -27,6 +27,20 @@ fn named_block(d: &Drawing, name: &str) -> Option<String> {
     d.block(name.trim()).filter(|b| !b.anonymous && !b.name.starts_with('*') && !b.name.contains('|')).map(|b| b.name.clone())
 }
 
+/// Why the references of block `old` cannot be replaced: a dynamic block's references keep their
+/// property values in data that names the block.
+fn not_replaceable(d: &Drawing, old: &str) -> Option<String> {
+    cadcraft_doc::dynblock::dynamic_block(d, old).map(|_| format!("Block \"{old}\" is a dynamic block; its references cannot be replaced."))
+}
+
+/// Why block `new` cannot replace block `old`: references of `old` inside `new` would make `new`
+/// reference itself.
+fn cannot_replace_with(d: &Drawing, old: &str, new: &str) -> Option<String> {
+    let blk = d.block(new)?;
+    references_block(d, &blk.entities.iter().map(|e| (**e).clone()).collect::<Vec<_>>(), old)
+        .then(|| format!("Block \"{new}\" references \"{old}\", so it cannot replace it."))
+}
+
 /// Replace every reference of `old` by a reference of `new`; with `purge`, remove `old`'s
 /// definition when nothing references it afterwards. Returns (references replaced, purged).
 fn replace(s: &mut Session, old: &str, new: &str, purge: bool) -> Result<(usize, bool)> {
@@ -46,15 +60,10 @@ fn replace(s: &mut Session, old: &str, new: &str, purge: bool) -> Result<(usize,
     if old.eq_ignore_ascii_case(&new) {
         return Err(bad(cmd, "the replacement block must be a different block"));
     }
-    // A dynamic block's references keep their property values in data that names the block.
-    if cadcraft_doc::dynblock::dynamic_block(d, &old).is_some() {
-        return Err(bad(cmd, format!("Block \"{old}\" is a dynamic block; its references cannot be replaced.")));
+    if let Some(why) = not_replaceable(d, &old).or_else(|| cannot_replace_with(d, &old, &new)) {
+        return Err(bad(cmd, why));
     }
     let blk = d.block(&new).cloned().ok_or_else(|| bad(cmd, format!("Block \"{new}\" not found.")))?;
-    // References of `old` inside `new` would make `new` reference itself.
-    if references_block(d, &blk.entities.iter().map(|e| (**e).clone()).collect::<Vec<_>>(), &old) {
-        return Err(bad(cmd, format!("Block \"{new}\" references \"{old}\", so it cannot replace it.")));
-    }
     let defs = attdefs(&blk);
     let scale = d.current_anno_scale();
     let d = s.doc_mut()?;
@@ -132,17 +141,24 @@ struct BlockreplaceM {
 }
 
 impl BlockreplaceM {
-    /// Take `name` as the answer to the current name prompt.
+    /// Take `name` as the answer to the current name prompt. A block that cannot take part is
+    /// refused here, so the prompt asks again.
     fn take(&mut self, s: &Session, stage: Stage, name: &str) -> Result<()> {
         let d = s.doc()?;
         let name = named_block(d, name).ok_or_else(|| EngineError::Other(format!("Block \"{}\" not found.", name.trim())))?;
         if stage == Stage::Old {
+            if let Some(why) = not_replaceable(d, &name) {
+                return Err(EngineError::Other(why));
+            }
             self.old = name;
             self.stage = Stage::New;
         } else {
             if name.eq_ignore_ascii_case(&self.old) {
                 return Err(EngineError::Other("The replacement block must be a different block.".into()));
             }
+            if let Some(why) = cannot_replace_with(d, &self.old, &name) {
+                return Err(EngineError::Other(why));
+            }
             self.new = name;
             self.stage = Stage::Purge;
         }

crates/engine/tests/it/blockreplace.rs

@@ -2,7 +2,8 @@
 //! other blocks, becomes a reference of another at the same insertion point, scale and rotation.
 
 use cadcraft_engine::doc::geom::Vec3;
-use cadcraft_engine::doc::{Block, Common, Entity, EntityKind, Handle, Insert, Space};
+use cadcraft_engine::doc::foreign::{Foreign, ForeignExtra, ForeignObject};
+use cadcraft_engine::doc::{Block, Common, Entity, EntityKind, Handle, Insert, RawTag, Space};
 use cadcraft_engine::{Input, Session};
 use serde_json::{Value, json};
 
@@ -184,3 +185,35 @@ fn blockreplace_no_keeps_the_replaced_definition() {
     assert!(d.block("A").is_some());
     assert_eq!(refs(d.model.iter()), ["N", "B"]);
 }
+
+#[test]
+fn blockreplace_refuses_a_block_as_soon_as_its_name_is_given() {
+    let (mut s, _) = drawing();
+    s.cmdline("blockreplace").unwrap();
+    s.cmdline("A").unwrap();
+    // N holds a reference of A: refused at once, and the prompt asks for another name.
+    s.cmdline("N").unwrap();
+    assert!(s.log.iter().any(|l| l.contains("Block \"N\" references \"A\"")), "{:?}", s.log);
+    assert!(s.prompt_text().contains("replacement block for A"), "{}", s.prompt_text());
+    s.cmdline("B").unwrap();
+    assert!(s.prompt_text().contains("Purge the unreferenced block definition A?"), "{}", s.prompt_text());
+    s.input(Input::Cancel).unwrap();
+
+    // A dynamic block is refused as the block to replace.
+    let mut d = s.doc().unwrap().clone();
+    let mut f = Foreign::default();
+    f.records.insert(("BLOCK_RECORD".into(), "A".into()), ForeignExtra { xdict: Some(Handle(0xF00)), ..Default::default() });
+    let tags = |t: &[(i32, &str)]| t.iter().map(|(c, v)| RawTag { code: *c, value: (*v).into() }).collect();
+    f.objects.push(ForeignObject { handle: Handle(0xF00), kind: "DICTIONARY".into(), tags: tags(&[(100, "AcDbDictionary"), (3, "ACAD_ENHANCEDBLOCK"), (360, "F01")]) });
+    f.objects.push(ForeignObject { handle: Handle(0xF01), kind: "ACAD_EVALUATION_GRAPH".into(), tags: tags(&[(100, "AcDbEvalGraph")]) });
+    d.foreign = std::sync::Arc::new(f);
+    s.open_drawing(d, "dynamic", None);
+    let e = s.execute("blockreplace", &json!({"block": "A", "with": "B"})).unwrap_err().to_string();
+    assert!(e.contains("dynamic block"), "{e}");
+    s.cmdline("blockreplace").unwrap();
+    s.cmdline("A").unwrap();
+    assert!(s.log.iter().any(|l| l.contains("Block \"A\" is a dynamic block")), "{:?}", s.log);
+    assert!(s.prompt_text().contains("Enter the name of the block to replace"), "{}", s.prompt_text());
+    s.input(Input::Cancel).unwrap();
+    assert_eq!(refs(s.doc().unwrap().model.iter()), ["N", "A"]);
+}