Merge pull request #6116 from dotCooCoo/fix/type-find-params

Type: find and replace and Type menu commands reject a param of the wrong type

crates/engine/src/cmd/typemenu.rs

@@ -6,9 +6,11 @@ use serde_json::{Value, json};
 use vectorcraft_doc::{CharStyle, Document, Justify, LockedArtboards, Node, NodeId, NodeKind, Quotes, TextKind, TextObject, TextRun};
 use vectorcraft_geom::{Affine, Rect, shapes};
 
+use super::object::choice;
 use super::typecmd::refresh_bounds;
 use super::{
-    CommandSpec, EngineError, Result, Session, bad, bool_or, cmd, find_command, has_clipboard, has_doc, has_selection, ids_param, str_param, targets,
+    CommandSpec, EngineError, Result, Session, bad, checked_ids_param, cmd, find_command, has_clipboard, has_doc, has_selection, number_or,
+    switch_or, targets, text_param,
 };
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -182,7 +184,7 @@ struct CaseState {
 
 fn change_case(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "type.changeCase";
-    let case = str_param(p, "case").ok_or_else(|| bad(C, "missing case"))?.to_ascii_lowercase();
+    let case = text_param(p, "case", C)?.ok_or_else(|| bad(C, "missing case"))?.to_ascii_lowercase();
     if !["upper", "uppercase", "lower", "lowercase", "title", "sentence"].contains(&case.as_str()) {
         return Err(bad(C, "case must be upper|lower|title|sentence"));
     }
@@ -253,11 +255,14 @@ pub(crate) fn smarten(text: &str, prev: &mut Option<char>, quotes: Option<&Quote
 }
 
 fn smart_punctuation(s: &mut Session, p: &Value) -> Result<Value> {
-    let (d, e) = (bool_or(p, "dashes", true), bool_or(p, "ellipsis", true));
+    const C: &str = "type.smartPunctuation";
+    let (d, e) = (switch_or(p, "dashes", true, C)?, switch_or(p, "ellipsis", true, C)?);
+    let quotes = switch_or(p, "quotes", true, C)?;
+    let document = choice(p, C, "scope", &["selection", "document"])? == Some("document");
     let st = s.doc()?;
     // Quotes in the document's style (Document Setup → Type).
-    let q = bool_or(p, "quotes", true).then_some(st.doc.setup.quotes);
-    let ids = if str_param(p, "scope") == Some("document") || st.selection.is_empty() {
+    let q = quotes.then_some(st.doc.setup.quotes);
+    let ids = if document || st.selection.is_empty() {
         let mut v = vec![];
         st.doc.walk(|n| {
             if matches!(n.kind, NodeKind::Text(_)) {
@@ -436,16 +441,19 @@ fn path_options(s: &mut Session, p: &Value) -> Result<Value> {
     let start = fraction("start")?;
     // `end: null` puts the end bracket back at the end of the path.
     let end = p.get("end").map(|_| fraction("end")).transpose()?;
-    let flip = bool_or(p, "flip", false);
-    let effect = match str_param(p, "effect") {
+    let flip = switch_or(p, "flip", false, C)?;
+    let effect = match text_param(p, "effect", C)? {
         Some(e) => Some(vectorcraft_doc::PathEffect::parse(e).ok_or_else(|| bad(C, format!("unknown effect `{e}`")))?),
         None => None,
     };
-    let align = match str_param(p, "alignToPath") {
+    let align = match text_param(p, "alignToPath", C)? {
         Some(a) => Some(vectorcraft_doc::PathAlign::parse(a).ok_or_else(|| bad(C, format!("unknown alignToPath `{a}`")))?),
         None => None,
     };
-    let spacing = p.get("spacing").and_then(Value::as_f64).filter(|x| x.is_finite()).map(|x| x.clamp(-1000.0, 1000.0));
+    let spacing = match p.get("spacing") {
+        None | Some(Value::Null) => None,
+        Some(_) => Some(number_or(p, "spacing", 0.0, C)?.clamp(-1000.0, 1000.0)),
+    };
     // Without a flip, when every object already has every value given, the document and its undo
     // history stay as they are.
     let has_values = |t: &TextObject| match &t.kind {
@@ -608,16 +616,21 @@ fn unicode_char(u: &str) -> Option<char> {
 
 fn insert_char(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "type.insert";
-    let text = match (str_param(p, "char"), str_param(p, "text")) {
-        (Some(c), _) => special_char(c).ok_or_else(|| bad(C, format!("unknown character `{c}`")))?.to_string(),
-        (None, Some(t)) => t.to_string(),
-        _ => match (p.get("code"), str_param(p, "unicode")) {
-            (Some(code), _) => alt_code_char(code.as_u64().ok_or_else(|| bad(C, "`code` must be a whole number"))?)
-                .ok_or_else(|| bad(C, format!("no character has code {code}")))?
-                .to_string(),
-            (None, Some(u)) => unicode_char(u).ok_or_else(|| bad(C, format!("`{u}` is no Unicode character")))?.to_string(),
-            _ => return Err(bad(C, "give `char`, `text`, `code` or `unicode`")),
-        },
+    // Every source is type-checked before one is chosen, so a bad value is refused even when a
+    // source ahead of it is given.
+    let ch = text_param(p, "char", C)?;
+    let txt = text_param(p, "text", C)?;
+    let code = match p.get("code").filter(|v| !v.is_null()) {
+        Some(v) => Some(v.as_u64().ok_or_else(|| bad(C, "`code` must be a whole number"))?),
+        None => None,
+    };
+    let uni = text_param(p, "unicode", C)?;
+    let text = match (ch, txt, code, uni) {
+        (Some(c), ..) => special_char(c).ok_or_else(|| bad(C, format!("unknown character `{c}`")))?.to_string(),
+        (None, Some(t), ..) => t.to_string(),
+        (None, None, Some(code), _) => alt_code_char(code).ok_or_else(|| bad(C, format!("no character has code {code}")))?.to_string(),
+        (None, None, None, Some(u)) => unicode_char(u).ok_or_else(|| bad(C, format!("`{u}` is no Unicode character")))?.to_string(),
+        (None, None, None, None) => return Err(bad(C, "give `char`, `text`, `code` or `unicode`")),
     };
     let ids = texts(s, p, C)?;
     // When `text` is empty and every object ends with a run of text, the document and its undo
@@ -722,11 +735,11 @@ fn all_texts(d: &Document) -> Vec<NodeId> {
 
 fn find_replace(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "edit.findReplace";
-    let find = str_param(p, "find").filter(|f| !f.is_empty()).ok_or_else(|| bad(C, "missing `find`"))?.to_string();
-    let rep = str_param(p, "replace").unwrap_or("").to_string();
-    let (mc, ww) = (bool_or(p, "matchCase", false), bool_or(p, "wholeWord", false));
-    let ids = match ids_param(p, "ids") {
-        Some(v) => text_ids(&s.doc()?.doc, &v),
+    let find = text_param(p, "find", C)?.filter(|f| !f.is_empty()).ok_or_else(|| bad(C, "missing `find`"))?.to_string();
+    let rep = text_param(p, "replace", C)?.unwrap_or("").to_string();
+    let (mc, ww) = (switch_or(p, "matchCase", false, C)?, switch_or(p, "wholeWord", false, C)?);
+    let ids = match p.get("ids").filter(|v| !v.is_null()) {
+        Some(_) => text_ids(&s.doc()?.doc, &checked_ids_param(s, p, "ids", C)?),
         None => all_texts(&s.doc()?.doc),
     };
     let st = s.doc()?;
@@ -779,8 +792,8 @@ fn find_replace(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn find_next(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "edit.findNext";
-    let find = str_param(p, "find").filter(|f| !f.is_empty()).ok_or_else(|| bad(C, "missing `find`"))?.to_string();
-    let (mc, ww) = (bool_or(p, "matchCase", false), bool_or(p, "wholeWord", false));
+    let find = text_param(p, "find", C)?.filter(|f| !f.is_empty()).ok_or_else(|| bad(C, "missing `find`"))?.to_string();
+    let (mc, ww) = (switch_or(p, "matchCase", false, C)?, switch_or(p, "wholeWord", false, C)?);
     let st = s.doc()?;
     let all = all_texts(&st.doc);
     let cur = st.selection.objects.first().and_then(|id| all.iter().position(|x| x == id));

crates/engine/src/tests/menucmds.rs

@@ -2087,6 +2087,63 @@ fn find_next_cycles_through_text_objects() {
     assert!(s.execute("edit.findNext", &json!({"find": "zzz"})).unwrap()["id"].is_null());
 }
 
+/// The Type menu's commands and Find and Replace and Find Next refuse a param of the wrong type, or
+/// `ids` that name no object, with an error naming the param, and leave the text, the selection and
+/// the undo history as they were, even when Insert Character is also given a source it would use
+/// first. Null counts as a param left out, which keeps its default.
+#[test]
+fn type_menu_and_find_commands_refuse_a_param_of_the_wrong_type() {
+    let mut s = session();
+    let t = text(&mut s, "\"cat\" -- cat catalog...");
+    let pid = s.execute("path.create", &json!({"d": "M100 300 L500 300"})).unwrap()["id"].as_u64().unwrap();
+    let r = s.execute("text.createInPath", &json!({"path": pid, "mode": "onPath", "text": "cat", "size": 12, "at": [300, 310]})).unwrap();
+    let on_path = id_of(&r);
+    select_with_a_step_to_redo(&mut s, &[t, on_path]);
+    let before = (history(&s), plain(&s, t), plain(&s, on_path), selected(&s));
+    let string = |k: &str| format!("`{k}` must be a string");
+    let switch = |k: &str| format!("`{k}` must be true or false");
+    let scope = "`scope` must be one of selection, document".to_string();
+    let not_a_list = "ids must be an array of non-negative integer object ids".to_string();
+    let not_an_id = "invalid object id \"x\": expected a non-negative integer".to_string();
+    for (cmd, p, want) in [
+        ("edit.findReplace", json!({"find": "cat", "replace": 5}), string("replace")),
+        ("edit.findReplace", json!({"find": 5, "replace": "dog"}), string("find")),
+        ("edit.findReplace", json!({"find": "CAT", "replace": "dog", "matchCase": "true"}), switch("matchCase")),
+        ("edit.findReplace", json!({"find": "cat", "replace": "dog", "wholeWord": "yes"}), switch("wholeWord")),
+        ("edit.findReplace", json!({"find": "cat", "replace": "dog", "ids": t.0}), not_a_list),
+        ("edit.findReplace", json!({"find": "cat", "replace": "dog", "ids": [t.0, 999_999]}), "no such object id 999999".to_string()),
+        ("edit.findReplace", json!({"find": "cat", "replace": "dog", "ids": ["x"]}), not_an_id),
+        ("edit.findNext", json!({"find": ["cat"]}), string("find")),
+        ("edit.findNext", json!({"find": "cat", "matchCase": 1}), switch("matchCase")),
+        ("edit.findNext", json!({"find": "cat", "wholeWord": "false"}), switch("wholeWord")),
+        ("type.smartPunctuation", json!({"dashes": "false"}), switch("dashes")),
+        ("type.smartPunctuation", json!({"ellipsis": 0}), switch("ellipsis")),
+        ("type.smartPunctuation", json!({"quotes": "no"}), switch("quotes")),
+        ("type.smartPunctuation", json!({"scope": 5}), scope.clone()),
+        ("type.smartPunctuation", json!({"scope": "page"}), scope),
+        ("type.changeCase", json!({"case": 5}), string("case")),
+        ("type.insert", json!({"char": 5}), string("char")),
+        ("type.insert", json!({"text": 5}), string("text")),
+        ("type.insert", json!({"unicode": 176}), string("unicode")),
+        ("type.insert", json!({"char": "bullet", "unicode": 176}), string("unicode")),
+        ("type.insert", json!({"text": "x", "code": "a"}), "`code` must be a whole number".to_string()),
+        ("type.insert", json!({"text": "x", "code": 1.5}), "`code` must be a whole number".to_string()),
+        ("type.pathOptions", json!({"flip": "true"}), switch("flip")),
+        ("type.pathOptions", json!({"effect": 1}), string("effect")),
+        ("type.pathOptions", json!({"alignToPath": true}), string("alignToPath")),
+        ("type.pathOptions", json!({"spacing": "4"}), "`spacing` must be a number, not \"4\"".to_string()),
+    ] {
+        let r = s.execute(cmd, &p);
+        assert!(matches!(&r, Err(EngineError::BadParams { msg, .. }) if *msg == want), "{cmd} {p}: {r:?}");
+        assert_eq!((history(&s), plain(&s, t), plain(&s, on_path), selected(&s)), before, "{cmd} {p}");
+    }
+    // `code: null` gives way to `unicode`, and null switches and ids take their defaults.
+    s.execute("type.insert", &json!({"code": null, "unicode": "00B0", "ids": [on_path.0]})).unwrap();
+    assert_eq!(plain(&s, on_path), "cat°");
+    let r = s.execute("edit.findReplace", &json!({"find": "cat", "replace": "dog", "matchCase": null, "wholeWord": null, "ids": null})).unwrap();
+    assert_eq!((r["count"].as_u64(), plain(&s, t), plain(&s, on_path)), (Some(4), "\"dog\" -- dog dogalog...".to_string(), "dog°".to_string()));
+}
+
 // ---------- Select ----------
 
 #[test]

docs/target-app-parity.md

@@ -1206,6 +1206,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur
   Duplicate, Copy, Cut and Move Artboard and Rearrange All Artboards take `lockedAndHidden` (default: Move Locked and Hidden Artwork with Artboard) and refuse a value other than true, false or null. When art goes along, the journal records the value, so a replay under the other preference takes the same art.
   The Layers panel's New Layer (on top), Select Art and Target (Shift-click), Move Rows (the copy), Template, Paste Remembers Layers and Release to Layers (Build) refuse a value other than true, false or null, such as "true", and leave the document, the selection, the highlighted rows and the undo history as they were; New Sublayer refuses such a `top` too.
   Change Case, Smart Punctuation, Find and Replace, Convert To Area Type, Convert To Point Type, Type on a Path Options, the Type on a Path effects and Fill with Placeholder Text record no undo step when they change nothing.
+  Find and Replace, Find Next, Change Case, Smart Punctuation, Insert Character and Type on a Path Options refuse a text option other than text or null, such as a replacement of 5, and a switch other than true, false or null, such as "true". Smart Punctuation also refuses a scope other than selection or document, Type on a Path Options a spacing given as text, and Find and Replace `ids` that are not a list of existing object ids. Each leaves the text, the selection and the undo history as they were.
   The commands that take `keepModel` (fills and strokes, Appearance rows, Live Paint, Shape Builder fills, gradient stops and freeform points, gradient mesh points, type colors, the Symbol Stainer and the swatch commands) refuse a `keepModel` other than true, false or null, such as "true". Toggle Fill/Stroke Focus (`fill`), Invert, Complement, Apply Last Color and Apply Last Gradient (`stroke`), `paint.setFill` and `paint.setStroke` (`focus`, `none`), and `appearance.setItem`, `livePaint.fill`, `livePaint.strokeEdge`, a `shapeBuilder.merge` `fill`, `swatch.new` and `swatch.edit`'s `paint` (`none`) refuse such a value too. Each leaves the document, its undo history and the defaults for new art as they were.
   The shape tools' commands (Rectangle, Polygon, Star, Flare, Spiral, Arc, Rectangular Grid and Polar Grid), `path.create` (with `anchors` or `d`), the type commands (new type, type in a path, character formats, the byte range a text command takes, Touch Type, font size steps, `text.getRange`, Optical Margin Alignment, System Layout, Fractional Widths, Ruby and Area Type Options), the symbol commands (Place, Delete, the Symbol Sprayer and the symbolism tools), the swatch commands, the Appearance panel's commands, the gradient and freeform gradient commands, Shape Builder, Live Paint and Image Trace refuse an option other than true, false or null and a number given as text, such as "true" or "40", and leave the document and its undo history as they were.
 - **Fonts:** a font is found by any of its names (family and style in every language of its name table, legacy family/style pairs, PostScript name), so documents from other apps open with the font they name; Find Font and `text.fonts` say for each font whether it was found exactly, by another name, substituted or missing, and how many glyphs are missing. PDF and `.ai` import brings Japanese text back as live type: CID fonts without a ToUnicode map read through the standard CMaps, vertical columns (WMode 1) as vertical type, letter spacing as tracking; a glyph is live type only when the installed font (in the closest style of its family) draws the same glyph, and text in a font that isn't installed comes in as live type shown in the fallback font until the font is installed (exports that draw it — PDF, EPS, EMF/WMF, raster, SVG outlines or embedded fonts — warn that they wrote the fallback font, and Print and Print Preview warn about the type that prints in it; `textAs: outlines` keeps the PDF's embedded glyphs). Two installed versions of one family and style (a font library keeping an old file beside a new one) both load, the first staying the one the family and style resolve to; PDF and `.ai` import tries each version when the glyphs differ, and type set in a version other than that one names it (`CharStyle::font_version`), which layout, the substituted-glyph highlight, Find Font, Package and Document Info follow. Fonts outside the font folders are listed too: those font services such as Adobe Fonts load on Windows, through DirectWrite and, for those only GDI lists, through GDI (#1258), and those apps and font managers register with CoreText on macOS (#579). When a document opens with fonts that aren't available, fonts used only in symbols included, a Missing Fonts dialog lists them after the linked-file questions, with Find Fonts (Type › Find Font on them) and Close. Its Find in Folder… (in Find Font too) searches a folder on several threads, reads only what the font scan reads of font files and skips app packages, the Program Files and ProgramData folders, the system's folders and the app data folders in a home folder; Add Fonts copies the font files the user chooses, suitcase fonts with their resource fork, into VectorCraft's own Fonts folder, which the desktop app and `vectorcraft-cli` read (`text.missingFonts`, `text.findFontFiles`, `text.addFontFiles`). The font scan on Linux also reads the folders fontconfig's configuration lists, such as TeX fonts under `/usr/share/texmf/fonts` (#1548, fixes #1529; not verified on Linux).