diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 5a295202b..3c6cd3bbf 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -804,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 })) } diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs index 824dbedc2..daff1f7ae 100644 --- a/crates/engine/src/tests_ids_params.rs +++ b/crates/engine/src/tests_ids_params.rs @@ -143,3 +143,23 @@ fn mask_and_spot_commands_refuse_ids_that_are_not_ids() { 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)); +}