From b90d68ec343c8f351852aa95d2aebfd3965e01be Mon Sep 17 00:00:00 2001 From: aiongg Date: Sun, 25 Oct 2026 04:17:52 +0000 Subject: [PATCH] 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 | 32 +++++++++++++----- crates/engine/tests/it/blockreplace.rs | 35 +++++++++++++++++++- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/cmd/blocks/blockreplace.rs b/crates/engine/src/cmd/blocks/blockreplace.rs index c20012784..03bac655d 100644 --- a/crates/engine/src/cmd/blocks/blockreplace.rs +++ b/crates/engine/src/cmd/blocks/blockreplace.rs @@ -27,6 +27,20 @@ fn named_block(d: &Drawing, name: &str) -> Option { 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 { + 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 { + let blk = d.block(new)?; + references_block(d, &blk.entities.iter().map(|e| (**e).clone()).collect::>(), 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::>(), &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; } diff --git a/crates/engine/tests/it/blockreplace.rs b/crates/engine/tests/it/blockreplace.rs index eefb23f1f..2cd18c81c 100644 --- a/crates/engine/tests/it/blockreplace.rs +++ b/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"]); +}