diff --git a/crates/engine/src/cmd/textwrap.rs b/crates/engine/src/cmd/textwrap.rs index 6da8b453..6e9333a3 100644 --- a/crates/engine/src/cmd/textwrap.rs +++ b/crates/engine/src/cmd/textwrap.rs @@ -36,9 +36,11 @@ pub fn specs() -> Vec { ] } -fn targets(s: &Session, p: &Value) -> Result> { - match p.get("ids").and_then(Value::as_array) { - Some(a) => Ok(a.iter().filter_map(Value::as_u64).map(NodeId).collect()), +/// An explicit target list must be all valid ids: a partially applied text wrap is +/// particularly surprising when some of the chosen objects are silently skipped. +fn targets(s: &Session, p: &Value, command: &str) -> Result> { + match p.get("ids") { + Some(_) => checked_ids_param(s, p, "ids", command), None => selected_roots(s), } } @@ -48,7 +50,7 @@ fn wrap_param(p: &Value, base: TextWrap) -> TextWrap { } fn make(s: &mut Session, p: &Value) -> Result { - let ids = targets(s, p)?; + let ids = targets(s, p, "object.textWrap.make")?; let w = wrap_param(p, TextWrap::default()); s.edit("Make Text Wrap", |d, _| { for id in &ids { @@ -66,7 +68,7 @@ fn make(s: &mut Session, p: &Value) -> Result { fn release(s: &mut Session, p: &Value) -> Result { let ids: Vec = { let d = &s.doc()?.doc; - targets(s, p)?.into_iter().filter(|i| d.node(*i).is_some_and(|n| n.wrap.is_some())).collect() + targets(s, p, "object.textWrap.release")?.into_iter().filter(|i| d.node(*i).is_some_and(|n| n.wrap.is_some())).collect() }; if ids.is_empty() { return Err(EngineError::Other("Text Wrap: select a wrap object".into())); @@ -83,7 +85,7 @@ fn release(s: &mut Session, p: &Value) -> Result { } fn options(s: &mut Session, p: &Value) -> Result { - let ids = targets(s, p)?; + let ids = targets(s, p, "object.textWrap.options")?; let current = { let d = &s.doc()?.doc; ids.iter().find_map(|i| d.node(*i).and_then(|n| n.wrap)).unwrap_or_default() @@ -212,6 +214,35 @@ mod tests { vectorcraft_text::layout(vectorcraft_text::FontDb::global(), &t).lines.iter().map(|l| l.avail).collect() } + /// Invalid explicit ids must not turn a requested multi-object edit into a partial one. + #[test] + fn text_wrap_commands_reject_malformed_target_lists_without_editing() { + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 300, "height": 300})).unwrap(); + let id = s.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 80, "height": 80})).unwrap()["id"].as_u64().unwrap(); + let other = s.execute("shape.rectangle", &json!({"x": 120, "y": 10, "width": 80, "height": 80})).unwrap()["id"].as_u64().unwrap(); + s.execute("select.set", &json!({"ids": [id]})).unwrap(); + + let before = s.doc().unwrap().history.undo.len(); + for command in ["object.textWrap.make", "object.textWrap.release", "object.textWrap.options"] { + for ids in [json!([id, "invalid"]), json!([id, 99999]), json!({"not": "an array"})] { + let e = s.execute(command, &json!({"ids": ids, "offset": 12})).unwrap_err().to_string(); + assert!(e.contains("id") || e.contains("array"), "{command}: {e}"); + assert_eq!(s.doc().unwrap().history.undo.len(), before, "{command} added history on error"); + assert!(s.doc().unwrap().doc.node(NodeId(id)).unwrap().wrap.is_none()); + assert!(s.doc().unwrap().doc.node(NodeId(other)).unwrap().wrap.is_none()); + } + } + + s.execute("object.textWrap.make", &json!({"ids": [id]})).unwrap(); + assert!(s.doc().unwrap().doc.node(NodeId(id)).unwrap().wrap.is_some()); + assert!(s.doc().unwrap().doc.node(NodeId(other)).unwrap().wrap.is_none()); + s.execute("object.textWrap.options", &json!({"ids": [id], "offset": 12})).unwrap(); + assert_eq!(s.doc().unwrap().doc.node(NodeId(id)).unwrap().wrap.unwrap().offset, 12.0); + s.execute("object.textWrap.release", &json!({"ids": [id]})).unwrap(); + assert!(s.doc().unwrap().doc.node(NodeId(id)).unwrap().wrap.is_none()); + } + #[test] fn area_type_wraps_around_objects_above_and_follows_edits() { let mut s = Session::new();