diff --git a/crates/engine/src/cmd/blocks.rs b/crates/engine/src/cmd/blocks.rs index c2caf136..24360859 100644 --- a/crates/engine/src/cmd/blocks.rs +++ b/crates/engine/src/cmd/blocks.rs @@ -108,8 +108,41 @@ fn references_block(d: &cadcraft_doc::Drawing, ents: &[Entity], name: &str) -> b false } +/// What BLOCK does with the selected objects once the block is defined (the dialog's Convert to +/// block, Retain and Delete). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Keep { + /// Replace the objects by a reference to the new block. + Convert, + /// Leave the objects in the drawing. + Retain, + /// Erase the objects. + Delete, +} + +impl Keep { + /// The `keep` parameter's documented values, in any case; `None` for anything else. + pub(crate) fn parse(value: &str) -> Option { + match value.trim().to_ascii_lowercase().as_str() { + "convert" => Some(Keep::Convert), + "retain" => Some(Keep::Retain), + "delete" => Some(Keep::Delete), + _ => None, + } + } +} + +/// The `keep` parameter of `cmd`, `default` when absent. A value that is not documented is an +/// error (issue #935: a misspelt value used to erase the objects without converting them). +fn keep_param(cmd: &str, p: &Value, default: Keep) -> Result { + match str_param(p, "keep") { + None => Ok(default), + Some(v) => Keep::parse(v).ok_or_else(|| bad(cmd, format!("`keep` must be \"convert\", \"retain\" or \"delete\", not {v:?}"))), + } +} + /// Create a block from entities. Returns the insert handle when converting. -pub(crate) fn make_block(s: &mut Session, name: &str, base: Vec2, hs: &[Handle], keep: &str, description: &str) -> Result> { +pub(crate) fn make_block(s: &mut Session, name: &str, base: Vec2, hs: &[Handle], keep: Keep, description: &str) -> Result> { if !valid_block_name(name) { return Err(bad("block", "invalid block name")); } @@ -136,13 +169,13 @@ pub(crate) fn make_block(s: &mut Session, name: &str, base: Vec2, hs: &[Handle], b.entities.push(e); } doc.blocks.insert(name.to_string(), Arc::new(b)); - if keep != "retain" { + if keep != Keep::Retain { for h in hs { doc.remove_entity(*h); } } s.set_selection(Vec::new()); - if keep == "convert" { + if keep == Keep::Convert { let h = insert(s, name, base, 1.0, 0.0, &serde_json::Map::new())?; return Ok(Some(h)); } @@ -219,8 +252,8 @@ fn run_block(s: &mut Session, p: &Value) -> Result { let name = str_param(p, "name").ok_or_else(|| bad("block", "`name` is required"))?.to_string(); let base = point_param(p, "base").unwrap_or(Vec2::ZERO); let hs = targets(s, p)?; - let keep = str_param(p, "keep").unwrap_or("convert").to_string(); - let r = make_block(s, &name, base, &hs, &keep, str_param(p, "description").unwrap_or(""))?; + let keep = keep_param("block", p, Keep::Convert)?; + let r = make_block(s, &name, base, &hs, keep, str_param(p, "description").unwrap_or(""))?; // An annotative block: its references (the converted selection too) are annotative. if bool_or(p, "annotative", false) { let d = s.doc_mut()?; @@ -495,7 +528,7 @@ impl BlockM { fn finish(&mut self, s: &mut Session, hs: &[Handle]) -> Result { let (Some(name), Some(base)) = (self.name.clone(), self.base) else { return Ok(Step::Cancel) }; // A definition that can't be made (e.g. one that would reference itself) ends the command. - let keep = if self.dash { "delete" } else { "convert" }; + let keep = if self.dash { Keep::Delete } else { Keep::Convert }; if let Err(e) = make_block(s, &name, base, hs, keep, "") { s.echo(e.to_string()); return Ok(Step::Done); diff --git a/crates/engine/tests/block_keep.rs b/crates/engine/tests/block_keep.rs new file mode 100644 index 00000000..d870bd98 --- /dev/null +++ b/crates/engine/tests/block_keep.rs @@ -0,0 +1,63 @@ +//! BLOCK's `keep` parameter takes convert, retain or delete, in any case. Anything else is an +//! error that changes nothing (issue #935: a misspelt value erased the objects without making a +//! reference to the new block). + +use cadcraft_engine::Session; +use cadcraft_engine::doc::{EntityKind, Handle}; +use serde_json::{Value, json}; + +fn line(s: &mut Session) -> Value { + s.execute("line", &json!({ "points": [[0, 0], [10, 0]] })).unwrap()["handles"][0].clone() +} + +fn references(s: &Session) -> usize { + s.doc().unwrap().model.iter().filter(|e| matches!(e.kind, EntityKind::Insert(_))).count() +} + +/// Everything a refused BLOCK must leave alone: the objects, the blocks, the selection and the +/// undo history. +fn state(s: &Session) -> (Vec, Vec, Vec, usize) { + let d = s.doc().unwrap(); + let objects = d.model.iter().map(|e| format!("{:?}", e.kind)).collect(); + let blocks = d.blocks.keys().cloned().collect(); + let selection = s.selection().iter().map(|h| h.hex()).collect(); + (objects, blocks, selection, s.state().unwrap().undo.len()) +} + +#[test] +fn a_value_that_is_not_documented_is_an_error_and_changes_nothing() { + let mut s = Session::new(); + let h = line(&mut s); + s.set_selection(vec![Handle::parse_hex(h.as_str().unwrap()).unwrap()]); + for (cmd, keep) in [("block", "retian"), ("block", ""), ("block", "erase"), ("-block", "convert-all"), ("-block", "keep")] { + let before = state(&s); + let err = s.execute(cmd, &json!({ "name": "B", "base": [0, 0], "handles": [h], "keep": keep })).unwrap_err(); + assert!(err.to_string().contains("`keep`"), "{cmd} {keep:?}: {err}"); + assert_eq!(state(&s), before, "{cmd} {keep:?} changed the drawing"); + assert!(s.doc().unwrap().block("B").is_none(), "{cmd} {keep:?} defined the block"); + } + assert_eq!(s.doc().unwrap().model.len(), 1, "the line is still there"); + assert_eq!(references(&s), 0); + assert_eq!(s.selection().len(), 1, "the line is still selected"); +} + +#[test] +fn the_documented_values_work_in_any_case() { + let mut s = Session::new(); + let h = line(&mut s); + let r = s.execute("block", &json!({ "name": "R", "base": [0, 0], "handles": [h], "keep": "Retain" })).unwrap(); + assert!(r["insert"].is_null()); + assert_eq!(s.doc().unwrap().model.len(), 1, "the line stays"); + assert!(s.doc().unwrap().block("R").is_some()); + + let h = line(&mut s); + let r = s.execute("block", &json!({ "name": "D", "base": [0, 0], "handles": [h], "keep": "DELETE" })).unwrap(); + assert!(r["insert"].is_null()); + assert_eq!(s.doc().unwrap().model.len(), 1, "the second line is erased"); + + let h = line(&mut s); + let r = s.execute("-block", &json!({ "name": "C", "base": [0, 0], "handles": [h], "keep": " convert " })).unwrap(); + assert!(r["insert"].is_string()); + assert_eq!(references(&s), 1); + assert_eq!(s.doc().unwrap().model.len(), 2, "the retained line and C's reference"); +}