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 e3d59c567..8074fc6bf 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -199,8 +199,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 228a1ce3d..4ac7c4628 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 860cf089f..5a295202b 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: @@ -829,8 +825,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 +851,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 +1262,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 +1291,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 +1454,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/mask_effects.rs b/crates/engine/src/cmd/mask_effects.rs index 387909a6d..270abeed5 100644 --- a/crates/engine/src/cmd/mask_effects.rs +++ b/crates/engine/src/cmd/mask_effects.rs @@ -341,8 +341,12 @@ fn apply_effect(s: &mut Session, p: &Value) -> Result { // the masks: `ids`, `all`, else `id` or the selected one let ids: Vec = if p.get("all").and_then(Value::as_bool).unwrap_or(false) { d.masks.iter().map(|m| m.id).collect() - } else if let Some(a) = p.get("ids").and_then(Value::as_array) { - a.iter().filter_map(Value::as_u64).filter_map(|v| u32::try_from(v).ok()).collect() + } else if let Some(v) = p.get("ids").filter(|v| !v.is_null()) { + // mask ids; one that isn't (`ids: 3`, a string, a negative) is an error, not the selected mask + let a = v.as_array().ok_or_else(|| bad(C, "`ids` must be a list of mask ids"))?; + a.iter() + .map(|v| v.as_u64().and_then(|v| u32::try_from(v).ok()).ok_or_else(|| bad(C, "`ids` must be mask ids (whole numbers)"))) + .collect::>>()? } else { vec![selected_mask(s, p, C)?.id] }; 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 4b86d4012..762c357d7 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -192,6 +192,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 24cc2061c..df71695b9 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 25e6ef52f..50a86a786 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1275,6 +1275,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..1eae90473 --- /dev/null +++ b/crates/engine/src/tests_ids_params.rs @@ -0,0 +1,106 @@ +//! `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); +} diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 097b13c4d..ad2193daf 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); @@ -489,8 +495,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();