diff --git a/crates/engine/src/cmd/cull.rs b/crates/engine/src/cmd/cull.rs index 030cb6e93..1908bb70d 100644 --- a/crates/engine/src/cmd/cull.rs +++ b/crates/engine/src/cmd/cull.rs @@ -87,7 +87,7 @@ fn analyze(s: &mut Session, p: &Value) -> Result { /// content), most similar first; the view is filtered to them. fn find_similar(s: &mut Session, p: &Value) -> Result { const C: &str = "library.findSimilar"; - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| super::bad(C, "no photo"))?; + let id = super::photo_id(p, C)?.or(s.active()).ok_or_else(|| super::bad(C, "no photo"))?; let min = p.get("similarity").and_then(Value::as_f64).unwrap_or(0.8) as f32; let all: Vec = s.catalog.photos().filter(|q| q.in_library()).map(|q| q.id).collect(); let sig = |s: &mut Session, id: PhotoId| -> Option<[f32; 64]> { diff --git a/crates/engine/src/cmd/denoise.rs b/crates/engine/src/cmd/denoise.rs index f93a2434a..644d3b89e 100644 --- a/crates/engine/src/cmd/denoise.rs +++ b/crates/engine/src/cmd/denoise.rs @@ -513,10 +513,6 @@ fn settings(s: &mut Session, p: &Value) -> Result { Ok(json!({"auto": st.auto(), "cacheGb": st.cache_bytes() >> 30, "threads": st.threads, "runOn": st.run_on().name()})) } -fn ids_param(p: &Value) -> Option> { - p.get("ids").and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(PhotoId).collect()) -} - fn status_json(s: &Session, photo: Option) -> Value { let (files, bytes) = s.denoise_cache_usage(); let st = &s.denoise.settings; @@ -594,7 +590,7 @@ fn queue(s: &mut Session, p: &Value) -> Result { if s.denoise_products_dir().is_none() { return Err(bad(C, "denoise keeps its pictures in the library's folder: open a library on disk first")); } - let ids = match ids_param(p) { + let ids = match super::photo_ids(p, C)? { Some(ids) => ids, None => match str_param(p, "scope").unwrap_or("selected") { "selected" => s.selection.active.iter().chain(s.selection.ids.iter()).copied().collect(), @@ -612,14 +608,14 @@ fn queue(s: &mut Session, p: &Value) -> Result { } fn cancel(s: &mut Session, p: &Value) -> Result { - let photo = p.get("id").and_then(Value::as_u64).map(PhotoId); + let photo = super::photo_id(p, "denoise.cancel")?; let n = s.denoise_cancel(photo, bool_or(p, "all", photo.is_none())); Ok(json!({"cancelled": n})) } fn status(s: &mut Session, p: &Value) -> Result { s.denoise_refresh_active(false); - Ok(status_json(s, p.get("id").and_then(Value::as_u64).map(PhotoId))) + Ok(status_json(s, super::photo_id(p, "denoise.status")?)) } /// `denoise.pump {pace?}`: cheap to call every frame. Takes in what the background job has finished and keeps one going. diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index 4e2ee412f..fbc3d4041 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -208,8 +208,7 @@ pub fn specs() -> Vec { } refit_crop_angle(angle, before, d, dims); }; - if let Some(ids) = p.get("ids").and_then(Value::as_array) { - let ids: Vec = ids.iter().filter_map(Value::as_u64).map(PhotoId).collect(); + if let Some(ids) = super::photo_ids(p, "develop.set")? { let ops = ids .iter() .filter_map(|id| s.develop_of(*id).map(|d| (*id, d))) diff --git a/crates/engine/src/cmd/face_recognize.rs b/crates/engine/src/cmd/face_recognize.rs index 5114ee575..e66bb56f3 100644 --- a/crates/engine/src/cmd/face_recognize.rs +++ b/crates/engine/src/cmd/face_recognize.rs @@ -82,7 +82,7 @@ fn name_faces(s: &mut Session, p: &Value) -> Result { /// makes the face yours: a later "Detect Faces" run no longer replaces it. fn set_name(s: &mut Session, p: &Value) -> Result { const C: &str = "faces.setName"; - let id = p.get("id").and_then(Value::as_u64).map(lightcraft_catalog::PhotoId).or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; + let id = super::photo_id(p, C)?.or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; let index = p.get("index").and_then(Value::as_u64).and_then(|i| usize::try_from(i).ok()).ok_or_else(|| bad(C, "missing or invalid `index`"))?; let name = name_param(p, C)?; let mut meta = s.catalog.photo(id).ok_or_else(|| bad(C, "no such photo"))?.meta.clone(); @@ -470,8 +470,7 @@ mod imp { let embedder = current(s, C)?; collect(s); let budget = Duration::from_millis(p.get("budgetMs").and_then(Value::as_u64).unwrap_or(200).min(600_000)); - let first: Vec = - p.get("ids").and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(PhotoId).collect()).unwrap_or_default(); + let first: Vec = super::super::photo_ids(p, C)?.unwrap_or_default(); let todo = pending(s, &first, false); let started = Instant::now(); let (mut embedded, mut failed, mut done) = (0, 0, 0); diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 5567e0abf..ff275fb6e 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -145,10 +145,6 @@ fn strs(p: &Value, key: &str) -> Vec { p.get(key).and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_str).map(str::to_string).collect()).unwrap_or_default() } -fn ids_param(p: &Value) -> Option> { - p.get("ids").and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(PhotoId).collect()) -} - /// The targets an album can hold and how many were browsed (Local) photos, which album views /// never show (issue #684): those are skipped, and reported, instead of becoming invisible members. fn album_targets(s: &Session, targets: Vec) -> (Vec, usize) { @@ -623,7 +619,7 @@ pub fn specs() -> Vec { "{ids: [id], active?: id, mode?: replace|add|toggle|range} — every id must be a photo of the library", always, |s, p| { - let ids = ids_param(p).unwrap_or_default(); + let ids = super::photo_ids(p, "library.select")?.unwrap_or_default(); let active = p.get("active").and_then(Value::as_u64).map(PhotoId); let mode = str_param(p, "mode").unwrap_or("replace"); // An id that is not in the library is an error, and the selection stays as it was: @@ -808,9 +804,13 @@ pub fn specs() -> Vec { "{label: red|yellow|green|blue|purple|none, ids?}", has_selection, |s, p| { - let l = match str_param(p, "label") { - None | Some("none") => None, - Some(x) => Some(ColorLabel::parse(x).ok_or_else(|| bad("photo.label", "unknown label"))?), + // `none` (or null) clears; a missing label or one that isn't text is an error, not a clear + let l = match p.get("label") { + Some(Value::Null) => None, + Some(Value::String(x)) if x == "none" => None, + Some(Value::String(x)) => Some(ColorLabel::parse(x).ok_or_else(|| bad("photo.label", "unknown label"))?), + None => return Err(bad("photo.label", "missing `label` (red|yellow|green|blue|purple|none)")), + Some(_) => return Err(bad("photo.label", "label must be red|yellow|green|blue|purple|none")), }; for_targets(s, p, "Set Color Label", |id| Some(Op::SetLabel { id, label: l })) } @@ -829,8 +829,7 @@ pub fn specs() -> Vec { "{id?, index} — remove one face / pet region (by its position in the photo's regions) from the photo in the catalog; undoable. The XMP sidecar is never rewritten for this, even with auto-write on, so reading the metadata from the file brings the region back", always, |s, p| { - let id = - p.get("id").and_then(Value::as_u64).map(PhotoId).or_else(|| s.active()).ok_or_else(|| bad("photo.removeRegion", "no photo"))?; + let id = super::photo_id(p, "photo.removeRegion")?.or_else(|| s.active()).ok_or_else(|| bad("photo.removeRegion", "no photo"))?; let index = p .get("index") .and_then(Value::as_u64) @@ -856,7 +855,7 @@ pub fn specs() -> Vec { |s, p| { const C: &str = "photo.setRegion"; // a drag is one undo step however many frames it took: the caller commits once, on release - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; + let id = super::photo_id(p, C)?.or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; let index = p .get("index") .and_then(Value::as_u64) @@ -1267,7 +1266,7 @@ pub fn specs() -> Vec { has_library_selection, |s, p| { let id = album_param(p, "id", "album.addPhotos")?; - let targets = ids_param(p).unwrap_or_else(|| s.selected()); + let targets = super::photo_ids(p, "album.addPhotos")?.unwrap_or_else(|| s.selected()); let al = s.catalog.album(id).ok_or_else(|| bad("album.addPhotos", "no such album"))?; if al.is_smart() || al.folder { return Err(bad("album.addPhotos", "smart albums and folders can't hold photos")); @@ -1296,7 +1295,8 @@ pub fn specs() -> Vec { "{ids?} — B: adds the photos to the target album (the Quick Collection unless one is set), or removes them when they're all in it → {album, added, skipped}; Local photos are skipped", has_library_selection, |s, p| { - let targets = album_targets_or_err(s, "album.toggleTarget", ids_param(p).unwrap_or_else(|| s.selected()))?; + let targets = + album_targets_or_err(s, "album.toggleTarget", super::photo_ids(p, "album.toggleTarget")?.unwrap_or_else(|| s.selected()))?; let (targets, skipped) = targets; let target = s.target_album.filter(|a| s.catalog.album(*a).is_some_and(|al| !al.is_smart() && !al.folder)); let id = match target.or_else(|| s.catalog.quick_collection()) { @@ -1458,7 +1458,7 @@ pub fn specs() -> Vec { _ => return Err(bad("album.removePhotos", "no album: pass `id` or show an album")), }, }; - let targets = ids_param(p).unwrap_or_else(|| s.selected()); + let targets = super::photo_ids(p, "album.removePhotos")?.unwrap_or_else(|| s.selected()); let al = s.catalog.album(id).ok_or_else(|| bad("album.removePhotos", "no such album"))?; if al.is_smart() { return Err(bad("album.removePhotos", "smart albums update automatically: change their rules")); diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 6f194c652..1bff30d2b 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -58,7 +58,7 @@ pub(crate) fn relink_op(id: PhotoId, path: &str) -> Op { fn relink(s: &mut Session, p: &Value) -> Result { const C: &str = "photo.relink"; - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad(C, "no photo (give `id`)"))?; + let id = super::photo_id(p, C)?.or(s.active()).ok_or_else(|| bad(C, "no photo (give `id`)"))?; let path = str_param(p, "path").ok_or_else(|| bad(C, "missing `path`"))?; let abs = std::path::absolute(path).map_err(|e| bad(C, e.to_string()))?; if !abs.is_file() { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 22af48f44..9b8dc3dbd 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -193,6 +193,32 @@ pub(crate) fn names_photos(p: &Value) -> bool { ["ids", "id"].iter().any(|k| p.get(k).is_some_and(|v| !v.is_null())) } +/// The photos `ids` names: `None` when it is missing or null (the command then falls back to the +/// selection, or its own default), else every entry as a photo id. An `ids` that isn't a list of +/// whole numbers is an error, never `None`: falling back would act on the selection instead of +/// the photos the call meant (`{"ids": 3}`, `{"ids": ["3"]}`). +pub(crate) fn photo_ids(p: &Value, cmd: &str) -> Result>> { + match p.get("ids") { + None | Some(Value::Null) => Ok(None), + Some(Value::Array(a)) => a + .iter() + .map(|v| v.as_u64().map(lightcraft_catalog::PhotoId).ok_or_else(|| bad(cmd, "`ids` must be photo ids (whole numbers)"))) + .collect::>>() + .map(Some), + Some(_) => Err(bad(cmd, "`ids` must be a list of photo ids")), + } +} + +/// The photo `id` names: `None` when it is missing or null (the command then uses the active +/// photo), else that id. An `id` that isn't a whole number is an error, never `None`: falling back +/// would act on the active photo instead of the one the call meant (`{"id": "5"}`). +pub(crate) fn photo_id(p: &Value, cmd: &str) -> Result> { + match p.get("id") { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().map(|n| Some(lightcraft_catalog::PhotoId(n))).ok_or_else(|| bad(cmd, "`id` must be a photo id (a whole number)")), + } +} + pub(crate) fn bad(cmd: &str, msg: impl Into) -> EngineError { EngineError::BadParams { cmd: cmd.into(), msg: msg.into() } } diff --git a/crates/engine/src/cmd/organize.rs b/crates/engine/src/cmd/organize.rs index 50807e7cf..dbd0aa842 100644 --- a/crates/engine/src/cmd/organize.rs +++ b/crates/engine/src/cmd/organize.rs @@ -161,17 +161,17 @@ pub fn specs() -> Vec { Ok(json!({"changed": commit_ops(s, "Remove from Stack", ops)?})) }), cmd!("stack.setTop", "Set as Top of Stack", ["Photo", "Stack"], Some("Shift+S"), "{id?} (default: the active photo)", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.setTop", "no photo"))?; + let id = super::photo_id(p, "stack.setTop")?.or(s.active()).ok_or_else(|| bad("stack.setTop", "no photo"))?; let op = s.catalog.set_top_ops(id); Ok(json!({"changed": commit_ops(s, "Set Top of Stack", op.into_iter().collect())?})) }), cmd!("stack.moveUp", "Move Up in Stack", ["Photo", "Stack"], None, "{id?} — one place towards the top", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.moveUp", "no photo"))?; + let id = super::photo_id(p, "stack.moveUp")?.or(s.active()).ok_or_else(|| bad("stack.moveUp", "no photo"))?; let op = s.catalog.move_in_stack_ops(id, -1); Ok(json!({"changed": commit_ops(s, "Move in Stack", op.into_iter().collect())?})) }), cmd!("stack.moveDown", "Move Down in Stack", ["Photo", "Stack"], None, "{id?} — one place away from the top", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.moveDown", "no photo"))?; + let id = super::photo_id(p, "stack.moveDown")?.or(s.active()).ok_or_else(|| bad("stack.moveDown", "no photo"))?; let op = s.catalog.move_in_stack_ops(id, 1); Ok(json!({"changed": commit_ops(s, "Move in Stack", op.into_iter().collect())?})) }), @@ -183,7 +183,7 @@ pub fn specs() -> Vec { "{id?} — this photo and the ones after it become their own stack", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.split", "no photo"))?; + let id = super::photo_id(p, "stack.split")?.or(s.active()).ok_or_else(|| bad("stack.split", "no photo"))?; let op = s.catalog.split_stack_ops(id).ok_or_else(|| bad("stack.split", "pick a photo below the top of a stack"))?; Ok(json!({"changed": commit_ops(s, "Split Stack", vec![op])?})) } diff --git a/crates/engine/src/cmd/query.rs b/crates/engine/src/cmd/query.rs index fbe4608f0..d131f4704 100644 --- a/crates/engine/src/cmd/query.rs +++ b/crates/engine/src/cmd/query.rs @@ -62,7 +62,7 @@ fn album_json(a: &Album, all: &[Album], cat: &lightcraft_catalog::Catalog, depth /// The photo a query is about: `id` when given (which must be in the library), else the active one. fn photo_arg(s: &Session, p: &Value, c: &str) -> crate::Result { - match p.get("id").and_then(Value::as_u64).map(PhotoId) { + match super::photo_id(p, c)? { Some(id) if s.catalog.photo(id).is_some() => Ok(id), Some(id) => Err(bad(c, format!("no such photo {}", id.0))), None => s.active().ok_or_else(|| bad(c, "no photo")), diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index ec7c9f7e7..ba5a25ff8 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1328,6 +1328,8 @@ mod tests_history_cursor; #[cfg(test)] mod tests_hold_view; #[cfg(test)] +mod tests_ids_params; +#[cfg(test)] mod tests_import; #[cfg(test)] mod tests_import_move; diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs new file mode 100644 index 000000000..8ce772ded --- /dev/null +++ b/crates/engine/src/tests_ids_params.rs @@ -0,0 +1,205 @@ +//! `ids` that aren't photo ids are an error, never "the selection": a command that fell back to the +//! selection on `{"ids": 3}` acted on photos the call didn't name. + +use lightcraft_catalog::{AlbumId, PhotoId}; +use serde_json::{Value, json}; + +use crate::Session; + +/// A demo session with the first two photos selected, the third active elsewhere in the grid, and +/// an album holding the selection. +fn session() -> (Session, Vec, AlbumId) { + let mut s = Session::with_demo(); + let vis = s.visible_cloned(); + s.execute("library.select", &json!({"ids": [vis[0].0, vis[1].0], "active": vis[0].0})).unwrap(); + let r = s.execute("album.create", &json!({"name": "Trip", "addSelected": true})).unwrap(); + let album = AlbumId(r["id"].as_u64().unwrap()); + (s, vis.to_vec(), album) +} + +const BAD_IDS: [fn() -> Value; 5] = [|| json!(3), || json!("3"), || json!(["3"]), || json!([3, -1]), || json!({"id": 3})]; + +#[test] +fn album_commands_refuse_ids_that_are_not_photo_ids() { + let (mut s, vis, album) = session(); + let members = |s: &Session| s.catalog.album(album).unwrap().photos.clone(); + let before = members(&s); + assert_eq!(before.len(), 2); + for bad in BAD_IDS.map(|f| f()) { + for cmd in ["album.removePhotos", "album.addPhotos"] { + let e = s.execute(cmd, &json!({"id": album.0, "ids": bad})).unwrap_err(); + assert!(e.to_string().contains("ids"), "{cmd} {bad}: {e}"); + assert_eq!(members(&s), before, "{cmd} {bad} changed the album"); + } + assert!(s.execute("album.toggleTarget", &json!({"ids": bad})).is_err(), "album.toggleTarget {bad}"); + } + // well-formed ids still work, and no ids still means the selection + s.execute("album.removePhotos", &json!({"id": album.0, "ids": [vis[1].0]})).unwrap(); + assert_eq!(members(&s), vec![vis[0]]); + s.execute("album.addPhotos", &json!({"id": album.0, "ids": null})).unwrap(); + assert_eq!(members(&s).len(), 2); +} + +#[test] +fn develop_set_refuses_ids_that_are_not_photo_ids() { + let (mut s, vis, _) = session(); + let exposure = |s: &Session, id: PhotoId| s.develop_of(id).unwrap().light.exposure; + let before: Vec = vis.iter().map(|id| exposure(&s, *id)).collect(); + for bad in BAD_IDS.map(|f| f()) { + let e = s.execute("develop.set", &json!({"control": "light.exposure", "value": 2.5, "ids": bad})).unwrap_err(); + assert!(e.to_string().contains("ids"), "{bad}: {e}"); + assert_eq!(vis.iter().map(|id| exposure(&s, *id)).collect::>(), before, "{bad} edited a photo"); + } + s.execute("develop.set", &json!({"control": "light.exposure", "value": 2.5, "ids": [vis[2].0]})).unwrap(); + assert_eq!((exposure(&s, vis[0]), exposure(&s, vis[2])), (before[0], 2.5)); +} + +#[test] +fn library_select_refuses_ids_that_are_not_photo_ids() { + let (mut s, vis, _) = session(); + let before = s.selection.ids.clone(); + for bad in BAD_IDS.map(|f| f()) { + assert!(s.execute("library.select", &json!({"ids": bad})).is_err(), "{bad}"); + assert_eq!(s.selection.ids, before, "{bad} changed the selection"); + } + s.execute("library.select", &json!({"ids": [vis[2].0]})).unwrap(); + assert_eq!(s.selection.ids, vec![vis[2]]); +} + +/// The same for a single `id`: a value that isn't a photo id used to mean the active photo +/// (`photo.relink` then relinked the active photo to the file, `photo.removeRegion` removed its face +/// box), or, for `denoise.cancel`, every photo. (The stack commands share the helper; `in_stack` +/// keeps them disabled on a photo outside a stack, so they aren't called here.) +#[test] +fn single_photo_commands_refuse_an_id_that_is_not_a_photo_id() { + let (mut s, vis, _) = session(); + let active = s.active().unwrap(); + let mut meta = s.catalog.photo(active).unwrap().meta.clone(); + meta.regions = vec![lightcraft_meta::Region { + rect: lightcraft_geom::Rect { x0: 0.1, y0: 0.1, x1: 0.3, y1: 0.3 }, + kind: lightcraft_meta::RegionKind::Face, + name: None, + description: None, + }]; + s.commit("setup", lightcraft_catalog::Op::SetMeta { id: active, meta: Box::new(meta) }).unwrap(); + let source = |s: &Session| format!("{:?}", s.catalog.photo(active).unwrap().source); + let before = source(&s); + let commits = s.commits(); + for bad in [json!("5"), json!(-1), json!(1.5), json!([vis[2].0]), json!({"id": 1})] { + for (cmd, mut params) in [ + ("photo.relink", json!({"path": "/elsewhere/other.jpg"})), + ("photo.removeRegion", json!({"index": 0})), + ("photo.setRegion", json!({"index": 0, "rect": {"x0": 0.2, "y0": 0.2, "x1": 0.4, "y1": 0.4}})), + ("faces.setName", json!({"index": 0, "name": "Ann"})), + ("library.findSimilar", json!({})), + ("denoise.cancel", json!({})), + ("photo.inspect", json!({})), + ] { + params["id"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`id`"), "{cmd} {bad}: {e}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(source(&s), before); + assert_eq!(s.catalog.photo(active).unwrap().meta.regions.len(), 1); +} + +/// Mask ids and spot indexes the same way: `mask.delete {"id": "1"}` deleted the selected mask, and an +/// id past `u32` wrapped (`4294967297 as u32` is 1) onto another mask. +#[test] +fn mask_and_spot_commands_refuse_ids_that_are_not_ids() { + let (mut s, _, _) = session(); + s.execute("mask.add", &json!({"kind": "linear"})).unwrap(); + s.execute("mask.add", &json!({"kind": "radial"})).unwrap(); + s.execute("spot.add", &json!({"mode": "heal", "points": [[0.4, 0.5]], "size": 0.02, "source": [0.1, 0.0]})).unwrap(); + let state = |s: &Session| { + let d = s.develop_of(s.active().unwrap()).unwrap(); + (d.masks.iter().map(|m| (m.id, m.name.clone(), m.visible)).collect::>(), d.spots.len()) + }; + let before = state(&s); + assert_eq!((before.0.len(), before.1), (2, 1)); + let first = before.0[0].0; + let commits = s.commits(); + for bad in [json!(u64::from(u32::MAX) + 1 + u64::from(first)), json!("1"), json!(-1), json!(1.5), json!([first])] { + for (cmd, params) in [ + ("mask.delete", json!({})), + ("mask.rename", json!({"name": "renamed"})), + ("mask.visible", json!({"visible": false})), + ("mask.select", json!({})), + ] { + let mut params = params; + params["id"] = bad.clone(); + assert!(s.execute(cmd, ¶ms).is_err(), "{cmd} {bad}"); + } + for cmd in ["spot.delete", "spot.select"] { + assert!(s.execute(cmd, &json!({"index": bad})).is_err(), "{cmd} {bad}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(state(&s), before); + // well-formed ids and a null still work + s.execute("mask.delete", &json!({"id": first})).unwrap(); + s.execute("spot.select", &json!({"index": null})).unwrap(); + assert_eq!(state(&s).0.len(), 1); +} + +/// `photo.label` without a label, or with one that isn't text, cleared the colour label of every +/// selected photo (a missing `label` read as `none`); now it is an error and only `none` or null clear. +#[test] +fn photo_label_refuses_a_missing_or_non_text_label() { + let (mut s, vis, _) = session(); + s.execute("photo.label", &json!({"label": "red"})).unwrap(); + let labels = |s: &Session| vis.iter().take(2).map(|id| s.catalog.photo(*id).unwrap().label).collect::>(); + let before = labels(&s); + assert!(before.iter().all(Option::is_some)); + for params in [json!({}), json!({"label": 3}), json!({"label": true}), json!({"label": ["red"]}), json!({"label": "crimson"})] { + assert!(s.execute("photo.label", ¶ms).is_err(), "{params}"); + assert_eq!(labels(&s), before, "{params} changed the labels"); + } + s.execute("photo.label", &json!({"label": null})).unwrap(); + assert!(labels(&s).iter().all(Option::is_none)); + s.execute("photo.label", &json!({"label": "green"})).unwrap(); + s.execute("photo.label", &json!({"label": "none"})).unwrap(); + assert!(labels(&s).iter().all(Option::is_none)); +} + +/// The mask-effect commands and a mask's `component` the same way: `mask.saveEffect {"id": "2"}` +/// saved the selected mask's sliders, and `mask.update {"component": "1"}` reshaped component 0. +#[test] +fn mask_effects_and_components_refuse_values_that_are_not_ids() { + let (mut s, _, _) = session(); + s.execute("mask.add", &json!({"kind": "linear"})).unwrap(); + s.execute("mask.add", &json!({"kind": "radial"})).unwrap(); + s.execute("mask.saveEffect", &json!({"name": "Kept"})).unwrap(); + let state = |s: &Session| (format!("{:?}", s.develop_of(s.active().unwrap()).unwrap().masks), s.mask_effects.len()); + let before = state(&s); + let commits = s.commits(); + let d = s.develop_of(s.active().unwrap()).unwrap(); + let shape = serde_json::to_value(&d.masks.last().unwrap().components[0].shape).unwrap(); + for bad in [json!("1"), json!(-1), json!(1.5), json!([1]), json!(u64::from(u32::MAX) + 2)] { + for (cmd, params) in [ + ("mask.applyEffect", json!({"name": "Kept"})), + ("mask.saveEffect", json!({"name": "New"})), + ("mask.updateEffect", json!({"name": "Kept"})), + ] { + let mut params = params; + params["id"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`id`"), "{cmd} {bad}: {e}"); + } + if bad.is_u64() { + continue; // a well-formed index past the end is "no such component", not a type error + } + for (cmd, params) in + [("mask.update", json!({"shape": shape})), ("mask.component", json!({"action": "invert"})), ("mask.refineDetail", json!({}))] + { + let mut params = params; + params["component"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`component`"), "{cmd} {bad}: {e}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(state(&s), before); +} diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 953874f6a..efb3ea3ac 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -438,7 +438,13 @@ pub fn handle(app: &mut LightcraftApp, ctx: &egui::Context, req: &ControlRequest Outcome::Screenshot { path: s("path").map(str::to_string), headless: p.get("headless").and_then(Value::as_bool).unwrap_or(false) } } "ui.render" => { - let id = p.get("id").and_then(Value::as_u64).map(lightcraft_catalog::PhotoId).or(app.session.active()); + let id = match p.get("id").filter(|v| !v.is_null()) { + Some(v) => match v.as_u64() { + Some(n) => Some(lightcraft_catalog::PhotoId(n)), + None => return err("`id` must be a photo id (a whole number)"), + }, + None => app.session.active(), + }; let Some(id) = id else { return err("no photo") }; if let Some(Err(e)) = s("path").map(|path| app.session.check_write_target(path)) { return err(e); @@ -486,8 +492,14 @@ pub fn export_active(app: &mut LightcraftApp, p: &Value) -> Result = match p.get("ids").and_then(Value::as_array) { - Some(a) => a.iter().filter_map(|v| serde_json::from_value(v.clone()).ok()).collect(), + let ids: Vec<_> = match p.get("ids").filter(|v| !v.is_null()) { + // `ids` that aren't photo ids are an error, not "export the selection" + Some(v) => v + .as_array() + .ok_or("app.export: `ids` must be a list of photo ids")? + .iter() + .map(|v| v.as_u64().map(lightcraft_catalog::PhotoId).ok_or("app.export: `ids` must be photo ids (whole numbers)")) + .collect::>()?, None if !app.session.selection.ids.is_empty() => { // Batch order (and `{seq}`) follows the grid order. let sel: std::collections::HashSet<_> = app.session.selection.ids.iter().copied().collect();