aleleba/lightcraft
1
0
Fork 0
mirror of https://github.com/storytold/lightcraft.git synced 2026-10-24 10:09:01 -06:00
Code Issues Packages Projects Releases Wiki Activity

Merge PR #2456: Fix/strict ids params

Browse Source
This commit is contained in:
aiongg
2026-10-22 02:39:38 +00:00
parent b81b6d9f3d f32116bee1
commit fdb93026b4
12 changed files with 275 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/engine/src/cmd/cull.rs
+1 -1
View File
@@ -87,7 +87,7 @@ fn analyze(s: &mut Session, p: &Value) -> Result<Value> {
/// content), most similar first; the view is filtered to them.
fn find_similar(s: &mut Session, p: &Value) -> Result<Value> {
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<PhotoId> = s.catalog.photos().filter(|q| q.in_library()).map(|q| q.id).collect();
let sig = |s: &mut Session, id: PhotoId| -> Option<[f32; 64]> {
crates/engine/src/cmd/denoise.rs
+3 -7
View File
@@ -513,10 +513,6 @@ fn settings(s: &mut Session, p: &Value) -> Result<Value> {
Ok(json!({"auto": st.auto(), "cacheGb": st.cache_bytes() >> 30, "threads": st.threads, "runOn": st.run_on().name()}))
}
fn ids_param(p: &Value) -> Option<Vec<PhotoId>> {
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<PhotoId>) -> 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<Value> {
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<Value> {
}
fn cancel(s: &mut Session, p: &Value) -> Result<Value> {
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<Value> {
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.
crates/engine/src/cmd/develop.rs
+1 -2
View File
@@ -208,8 +208,7 @@ pub fn specs() -> Vec<CommandSpec> {
}
refit_crop_angle(angle, before, d, dims);
};
if let Some(ids) = p.get("ids").and_then(Value::as_array) {
let ids: Vec<PhotoId> = 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)))
crates/engine/src/cmd/face_recognize.rs
+2 -3
View File
@@ -82,7 +82,7 @@ fn name_faces(s: &mut Session, p: &Value) -> Result<Value> {
/// makes the face yours: a later "Detect Faces" run no longer replaces it.
fn set_name(s: &mut Session, p: &Value) -> Result<Value> {
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<PhotoId> =
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<PhotoId> = 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);
crates/engine/src/cmd/library.rs
+14 -14
View File
@@ -145,10 +145,6 @@ fn strs(p: &Value, key: &str) -> Vec<String> {
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<Vec<PhotoId>> {
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<PhotoId>) -> (Vec<PhotoId>, usize) {
@@ -623,7 +619,7 @@ pub fn specs() -> Vec<CommandSpec> {
"{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<CommandSpec> {
"{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<CommandSpec> {
"{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<CommandSpec> {
|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<CommandSpec> {
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<CommandSpec> {
"{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<CommandSpec> {
_ => 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"));
crates/engine/src/cmd/missing.rs
+1 -1
View File
@@ -58,7 +58,7 @@ pub(crate) fn relink_op(id: PhotoId, path: &str) -> Op {
fn relink(s: &mut Session, p: &Value) -> Result<Value> {
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() {
crates/engine/src/cmd/mod.rs
+26
View File
@@ -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<Option<Vec<lightcraft_catalog::PhotoId>>> {
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::<Result<Vec<_>>>()
.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<Option<lightcraft_catalog::PhotoId>> {
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<String>) -> EngineError {
EngineError::BadParams { cmd: cmd.into(), msg: msg.into() }
}
crates/engine/src/cmd/organize.rs
+4 -4
View File
@@ -161,17 +161,17 @@ pub fn specs() -> Vec<CommandSpec> {
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<CommandSpec> {
"{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])?}))
}
crates/engine/src/cmd/query.rs
+1 -1
View File
@@ -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<PhotoId> {
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")),
crates/engine/src/lib.rs
+2
View File
@@ -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;
crates/engine/src/tests_ids_params.rs
+205
View File
@@ -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<PhotoId>, 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<f64> = 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::<Vec<_>>(), 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, &params).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::<Vec<_>>(), 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, &params).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::<Vec<_>>();
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", &params).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, &params).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, &params).unwrap_err();
assert!(e.to_string().contains("`component`"), "{cmd} {bad}: {e}");
}
}
assert_eq!(s.commits(), commits, "nothing was changed");
assert_eq!(state(&s), before);
}
crates/ui-egui/src/control.rs
+15 -3
View File
@@ -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<Value, String
let p = &app.session.export_params(p)?;
// the format follows `path` (JPEG when it names none), and `bitDepth` is checked against it
let opts = ExportOptions::from_params(p).map_err(|e| e.to_string())?;
let ids: Vec<_> = 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::<Result<_, _>>()?,
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();