diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index d2c81e492..285604af5 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -512,11 +512,44 @@ pub fn color_value(v: &Value) -> Option { } } +/// `v` as an object id: an error unless it is a non-negative integer. +fn object_id(v: &Value) -> Result { + v.as_u64().map(NodeId).ok_or_else(|| EngineError::Other(format!("object ids are non-negative integers, not {v}"))) +} + +/// The error for `id` given as step reference `r`, which only a batch resolves. +fn unresolved_reference(r: &str) -> EngineError { + EngineError::Other(format!("`id` is the step reference {r:?}, which only `run` and `command_batch` resolve")) +} + +/// The objects `p` names for a command whose `id` can only be an object id: its `ids`, else its +/// `id` as `ids` of one. A value that isn't an object id is an error, never the selection (#785). +/// `None` when `p` gives neither (or gives them as `null`). The ids are not looked up in the +/// document. +pub(crate) fn named_ids(p: &Value) -> Result>> { + match p.get("ids").filter(|v| !v.is_null()) { + Some(Value::Array(a)) => return a.iter().map(object_id).collect::>>().map(Some), + Some(v) => return Err(EngineError::Other(format!("`ids` must be an array of object ids, not {v}"))), + None => {} + } + Ok(named_id(p)?.map(|id| vec![id])) +} + +/// The object `p`'s `id` names, for a command whose `id` can only be an object id: a value that +/// isn't an object id is an error, never the selection (#785). `None` when `p` leaves `id` out (or +/// gives it as `null`). The id is not looked up in the document. +pub(crate) fn named_id(p: &Value) -> Result> { + match p.get("id").filter(|v| !v.is_null()) { + None => Ok(None), + Some(Value::String(r)) if r.starts_with('$') => Err(unresolved_reference(r)), + Some(v) => object_id(v).map(Some), + } +} + /// Objects a command targets: explicit `ids` param or the selection. pub(crate) fn targets(s: &Session, p: &Value) -> Result> { // Objects given are used as given: a value that isn't an object id fails rather than the // command acting on the selection instead (#785). - let object_id = |v: &Value| v.as_u64().map(NodeId).ok_or_else(|| EngineError::Other(format!("object ids are non-negative integers, not {v}"))); match p.get("ids").filter(|v| !v.is_null()) { Some(Value::Array(a)) => return a.iter().map(object_id).collect(), Some(v) => return Err(EngineError::Other(format!("`ids` must be an array of object ids, not {v}"))), @@ -526,9 +559,7 @@ pub(crate) fn targets(s: &Session, p: &Value) -> Result> { Some(v @ Value::Number(_)) => Ok(vec![object_id(v)?]), // A step reference a batch couldn't resolve (other strings are other ids, such as an // effect's). - Some(Value::String(r)) if r.starts_with('$') => { - Err(EngineError::Other(format!("`id` is the step reference {r:?}, which only `run` and `command_batch` resolve"))) - } + Some(Value::String(r)) if r.starts_with('$') => Err(unresolved_reference(r)), _ => Ok(s.doc()?.selection.objects.clone()), } } diff --git a/crates/engine/src/cmd/brushsym.rs b/crates/engine/src/cmd/brushsym.rs index 6f7661afc..1c97ee74a 100644 --- a/crates/engine/src/cmd/brushsym.rs +++ b/crates/engine/src/cmd/brushsym.rs @@ -19,7 +19,7 @@ use super::edit::{outermost, selected_roots}; use super::pathops::{remove_each, separate}; use super::swatch::{anchor, names_param}; use super::{ - CommandSpec, Result, Session, bad, check_numbers, cmd, color_value, find_command, has_doc, has_selection, id_param, ids_param, number_or, ok, + CommandSpec, Result, Session, bad, check_numbers, cmd, color_value, find_command, has_doc, has_selection, id_param, named_ids, number_or, ok, str_param, switch_or, }; use crate::EngineError; @@ -314,7 +314,7 @@ fn ids_json(ids: &[NodeId]) -> Value { } pub(crate) fn roots(s: &Session, p: &Value) -> Result> { - match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) { + match named_ids(p)? { Some(mut v) => { // Explicit ids: only existing, non-layer objects, none inside another of them. let d = &s.doc()?.doc; diff --git a/crates/engine/src/cmd/buildcmds.rs b/crates/engine/src/cmd/buildcmds.rs index bdfed0175..56ff8a636 100644 --- a/crates/engine/src/cmd/buildcmds.rs +++ b/crates/engine/src/cmd/buildcmds.rs @@ -29,7 +29,7 @@ use super::edit::{outermost, selected_roots}; use super::paint::paint_from; use super::pathops::{Beside, shape_node}; use super::{ - CommandSpec, EngineError, Result, Session, always, bad, check_numbers, cmd, color_value, f64_or, finite_numbers, has_doc, id_param, ids_param, + CommandSpec, EngineError, Result, Session, always, bad, check_numbers, cmd, color_value, f64_or, finite_numbers, has_doc, id_param, named_ids, number_or, point_param, str_param, switch_or, }; @@ -220,7 +220,7 @@ fn has_multi_or_ids(s: &Session) -> std::result::Result<(), String> { } fn root_ids(s: &Session, p: &Value) -> Result> { - let ids = match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) { + let ids = match named_ids(p)? { Some(v) => v, None => selected_roots(s)?, }; diff --git a/crates/engine/src/cmd/distortcmds.rs b/crates/engine/src/cmd/distortcmds.rs index 7a0643ca5..9e0841de6 100644 --- a/crates/engine/src/cmd/distortcmds.rs +++ b/crates/engine/src/cmd/distortcmds.rs @@ -16,7 +16,7 @@ use vectorcraft_tools::distort::{PinSet, arap, collect_points, mesh_for, project use super::edit::{duplicate_in, outermost, selected_roots}; use super::{ CommandSpec, Result, Session, bad, check_numbers, check_switches, cmd, f64_or, f64_req, finite_numbers, has_doc, has_selection, id_param, - ids_param, matrix_param, number_or, ok, point_indices, point_param, str_param, switch_or, targets, + matrix_param, named_id, named_ids, number_or, ok, point_indices, point_param, str_param, switch_or, targets, }; use crate::EngineError; @@ -658,7 +658,7 @@ fn liquify(s: &mut Session, p: &Value) -> Result { if pts.iter().any(|(q, f)| !q.x.is_finite() || !q.y.is_finite() || !f.is_finite()) { return Err(bad(C, "points must be finite")); } - let roots = match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) { + let roots = match named_ids(p)? { Some(v) => v, None => s.doc()?.selection.objects.clone(), }; @@ -708,7 +708,7 @@ fn liquify(s: &mut Session, p: &Value) -> Result { /// The objects a Puppet Warp command works on: `id`/`ids`, else `default`. fn puppet_ids(p: &Value, default: impl FnOnce() -> Result>) -> Result> { - let ids = match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) { + let ids = match named_ids(p)? { Some(v) => v, None => default()?, }; diff --git a/crates/engine/src/cmd/distortcmds/perspective.rs b/crates/engine/src/cmd/distortcmds/perspective.rs index eda17fee8..786d0d956 100644 --- a/crates/engine/src/cmd/distortcmds/perspective.rs +++ b/crates/engine/src/cmd/distortcmds/perspective.rs @@ -1,6 +1,6 @@ use super::{ Affine, Arc, Document, EngineError, Homography, NodeId, NodeKind, PerspectiveGrid, Plane, Point, Result, Session, Value, bad, check_numbers, - check_switches, collect_points, duplicate_in, finite_numbers, id_param, ids_param, json, matrix_param, number_or, ok, outermost, persp, + check_switches, collect_points, duplicate_in, finite_numbers, json, matrix_param, named_id, named_ids, number_or, ok, outermost, persp, point_param, project_node, selected_roots, str_param, switch_or, }; @@ -92,7 +92,7 @@ fn plane_param(g: &PerspectiveGrid, p: &Value, cmd: &str) -> Result { } pub(super) fn roots(s: &Session, p: &Value) -> Result> { - match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) { + match named_ids(p)? { Some(v) => Ok(outermost(&s.doc()?.doc, v)), None => selected_roots(s), } @@ -478,9 +478,8 @@ pub(super) fn plane_move(s: &mut Session, p: &Value) -> Result { pub(super) fn plane_match(s: &mut Session, p: &Value) -> Result { let st = s.doc()?; - let id = id_param(p, "id") - .or_else(|| st.selection.objects.first().copied()) - .ok_or_else(|| EngineError::Other("select an object in perspective".into()))?; + let id = + named_id(p)?.or_else(|| st.selection.objects.first().copied()).ok_or_else(|| EngineError::Other("select an object in perspective".into()))?; let mut g = grid_of(&st.doc); let (plane, depth) = g.attachment_of(&st.doc, id).ok_or_else(|| EngineError::Other("the object isn't in perspective".into()))?; g.set_offset(plane, depth); @@ -586,8 +585,7 @@ pub fn perspective_click(s: &Session, cmd: &str, params: &Value, at: Point) -> O /// mode, to be edited; exiting isolation projects it again ([`finish_edit_text`]). pub(super) fn edit_text(s: &mut Session, p: &Value) -> Result { let st = s.doc()?; - let id = - id_param(p, "id").or_else(|| st.selection.objects.first().copied()).ok_or_else(|| EngineError::Other("select type in perspective".into()))?; + let id = named_id(p)?.or_else(|| st.selection.objects.first().copied()).ok_or_else(|| EngineError::Other("select type in perspective".into()))?; if !st.doc.node(id).is_some_and(|n| matches!(n.kind, NodeKind::Text(_)) && n.projection().is_some()) { return Err(EngineError::Other("select type in perspective".into())); } diff --git a/crates/engine/src/cmd/graph.rs b/crates/engine/src/cmd/graph.rs index b0d3f92ef..2b9bb775a 100644 --- a/crates/engine/src/cmd/graph.rs +++ b/crates/engine/src/cmd/graph.rs @@ -17,7 +17,7 @@ use vectorcraft_doc::{ use vectorcraft_geom::{Affine, BezPath, FillRule, PathData, Point, Rect, shapes}; use super::typecmd::refresh_bounds; -use super::{CommandSpec, EngineError, Result, Session, bad, bool_or, cmd, f64_or, f64_req, has_doc, has_selection, id_param, ok, str_param}; +use super::{CommandSpec, EngineError, Result, Session, bad, bool_or, cmd, f64_or, f64_req, has_doc, has_selection, named_id, ok, str_param}; mod axis; mod columns; diff --git a/crates/engine/src/cmd/graph/edit.rs b/crates/engine/src/cmd/graph/edit.rs index 525f43721..1ea621c63 100644 --- a/crates/engine/src/cmd/graph/edit.rs +++ b/crates/engine/src/cmd/graph/edit.rs @@ -4,7 +4,7 @@ use super::generate::{children_bounds, generate}; use super::series::{picked_series, reselect_series}; use super::{ Affine, Document, EngineError, GraphKind, GraphSpec, Node, NodeId, Rect, Result, Session, TickLength, Value, ValueAxisSide, bad, bool_or, f64_or, - f64_req, id_param, json, str_param, + f64_req, json, named_id, str_param, }; /// Regenerate graph `id` from `spec` (adjusting the plot rect for any move/scale since the last @@ -57,10 +57,14 @@ pub(super) fn create(s: &mut Session, p: &Value) -> Result { Ok(json!({ "id": id.0 })) } -/// The selected graph (or the graph containing the selection). +/// The graph `id` names (or the graph containing it), else the selected graph (or the graph +/// containing the selection). pub(super) fn target(s: &Session, p: &Value, cmd: &str) -> Result { let st = s.doc()?; - let start = id_param(p, "id").into_iter().chain(st.selection.objects.iter().copied()); + let start = match named_id(p)? { + Some(id) => vec![id], + None => st.selection.objects.clone(), + }; for id in start { let mut cur = Some(id); while let Some(c) = cur { diff --git a/crates/engine/src/cmd/layer.rs b/crates/engine/src/cmd/layer.rs index 8b0e9392a..3c7d00cb5 100644 --- a/crates/engine/src/cmd/layer.rs +++ b/crates/engine/src/cmd/layer.rs @@ -456,7 +456,12 @@ fn clip_toggle(s: &mut Session, p: &Value) -> Result { [id] if matches!(st.doc.node(*id).map(|n| &n.kind), Some(NodeKind::Group { .. })) => Some(*id), _ => None, }; - let id = id_param(p, "id").or(row).or(group).or(st.current_layer()).ok_or_else(|| bad("layer.clippingMask.toggle", "no layer"))?; + // An `id` given is used as given: one that isn't an object id fails (#785). + let given = match p.get("id").filter(|v| !v.is_null()) { + Some(v) => Some(checked_id(s, v, "layer.clippingMask.toggle")?), + None => None, + }; + let id = given.or(row).or(group).or(st.current_layer()).ok_or_else(|| bad("layer.clippingMask.toggle", "no layer"))?; let clip = s.edit("Make/Release Clipping Mask", |d, _| { let n = d.node(id).ok_or(EngineError::NoNode(id))?; if !matches!(n.kind, NodeKind::Layer { .. } | NodeKind::Group { .. }) { @@ -519,7 +524,12 @@ fn new_sublayer(s: &mut Session, p: &Value) -> Result { const C: &str = "layer.newSublayer"; let st = s.doc()?; let row = st.highlighted_rows().last().and_then(|r| st.doc.layer_containing(*r)); - let parent = id_param(p, "parent").or(row).or(st.current_layer()).ok_or_else(|| bad(C, "no parent layer"))?; + // A `parent` given is used as given: one that isn't an object id fails (#785). + let given = match p.get("parent").filter(|v| !v.is_null()) { + Some(v) => Some(checked_id(s, v, C)?), + None => None, + }; + let parent = given.or(row).or(st.current_layer()).ok_or_else(|| bad(C, "no parent layer"))?; if !st.doc.node(parent).is_some_and(Node::is_layer) { return Err(bad(C, "the parent must be a layer or sublayer")); } diff --git a/crates/engine/src/cmd/live.rs b/crates/engine/src/cmd/live.rs index 843445a2b..be2b694cd 100644 --- a/crates/engine/src/cmd/live.rs +++ b/crates/engine/src/cmd/live.rs @@ -21,7 +21,7 @@ use super::expand::gradient_sampler; use super::pathops::Beside; use super::{ CommandSpec, EngineError, Result, Session, always, bad, check_numbers, check_switches, cmd, color_value, f64_or, f64_req, finite_numbers, - has_doc, has_multi, has_selection, id_param, ids_param, number_or, ok, str_param, switch_or, + has_doc, has_multi, has_selection, id_param, named_ids, number_or, ok, str_param, switch_or, }; mod blend; diff --git a/crates/engine/src/cmd/live/common.rs b/crates/engine/src/cmd/live/common.rs index 48f335610..226a57a0d 100644 --- a/crates/engine/src/cmd/live/common.rs +++ b/crates/engine/src/cmd/live/common.rs @@ -1,16 +1,16 @@ use super::mesh::whole; use super::{ Arc, BlendEase, BlendEaseType, BlendOrientation, BlendSpacing, Document, EngineError, Node, NodeId, NodeKind, Result, Selection, Session, Value, - bad, check_numbers, has_selection, ids_param, json, selected_roots, str_param, switch_or, + bad, check_numbers, has_selection, json, named_ids, selected_roots, str_param, switch_or, }; pub(super) fn ids_json(ids: &[NodeId]) -> Value { json!({ "ids": ids.iter().map(|i| i.0).collect::>() }) } -/// Explicit `ids` (in paint order) or the selected roots. +/// Explicit `ids`, or `id` as `ids` of one (in paint order), or the selected roots. pub(super) fn roots_param(s: &Session, p: &Value) -> Result> { - if let Some(ids) = ids_param(p, "ids") { + if let Some(ids) = named_ids(p)? { let st = s.doc()?; for id in &ids { if st.doc.node(*id).is_none() { @@ -192,7 +192,7 @@ pub(super) fn starts_param(p: &Value, roots: &[NodeId]) -> Result