diff --git a/crates/engine/src/cmd/mask_effects.rs b/crates/engine/src/cmd/mask_effects.rs index 387909a6d..d791b4260 100644 --- a/crates/engine/src/cmd/mask_effects.rs +++ b/crates/engine/src/cmd/mask_effects.rs @@ -287,12 +287,7 @@ fn effect_index(s: &Session, name: &str, c: &str) -> Result { /// The selected mask's adjustments on the active photo. fn selected_mask(s: &Session, p: &Value, c: &str) -> Result { let id = s.active().ok_or_else(|| bad(c, "no active photo"))?; - let mid = p - .get("id") - .and_then(Value::as_u64) - .and_then(|v| u32::try_from(v).ok()) - .or(s.active_mask) - .ok_or_else(|| bad(c, "no mask selected (give `id`)"))?; + let mid = super::masks::mask_id(p, s.active_mask, c)?; let d = s.develop_of(id).unwrap_or_default(); d.masks.iter().find(|m| m.id == mid).cloned().ok_or_else(|| bad(c, format!("no mask {mid}"))) } @@ -341,10 +336,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 { - vec![selected_mask(s, p, C)?.id] + match p.get("ids") { + None | Some(Value::Null) => vec![selected_mask(s, p, C)?.id], + Some(Value::Array(a)) => a.iter().map(|v| super::masks::parse_mask_id(v, C)).collect::>()?, + Some(_) => return Err(bad(C, "`ids` must be a list of mask IDs")), + } }; if ids.is_empty() { return Err(bad(C, "no masks to apply the effect to")); diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index 0efb3b658..dc403df9b 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -199,8 +199,28 @@ fn connected_strokes(from: Point, last: &BrushStroke, to: BrushStroke) -> Vec
, c: &str) -> Result { - p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)")) +pub(super) fn parse_mask_id(v: &Value, c: &str) -> Result { + v.as_u64().and_then(|v| u32::try_from(v).ok()).ok_or_else(|| bad(c, "`id` must be a non-negative integer that fits in u32")) +} + +/// Omitted or null targets keep the command's default; malformed explicit targets never do. +fn optional_mask_id(p: &Value, c: &str) -> Result> { + p.get("id").filter(|v| !v.is_null()).map(|v| parse_mask_id(v, c)).transpose() +} + +pub(super) fn mask_id(p: &Value, active: Option, c: &str) -> Result { + optional_mask_id(p, c)?.or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)")) +} + +fn optional_index(p: &Value, key: &str, c: &str) -> Result> { + p.get(key) + .filter(|v| !v.is_null()) + .map(|v| { + v.as_u64() + .and_then(|v| usize::try_from(v).ok()) + .ok_or_else(|| bad(c, format!("`{key}` must be a non-negative integer that fits in usize"))) + }) + .transpose() } fn find(masks: &mut [Mask], id: u32, c: &str) -> Result { @@ -307,7 +327,7 @@ pub fn specs() -> Vec { let connect = bool_or(p, "connect", false); let mut connected = false; let next = s.active().and_then(|id| s.develop_of(id)).map(|d| d.next_mask_id()).unwrap_or(1); - let want = p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(s.active_mask); + let want = optional_mask_id(p, "mask.brushStroke")?.or(s.active_mask); let adjust = super::mask_effects::new_mask_adjust(s, "brush"); let mut out = masks_edit(s, "mask.brushStroke", "Brush", |masks, active| { let i = match want.and_then(|m| masks.iter().position(|x| x.id == m)) { @@ -363,7 +383,7 @@ pub fn specs() -> Vec { cmd!("mask.update", "Update Mask Shape", [], None, "{id?, component?: index (0), shape: MaskShape JSON}", has_active, |s, p| { let shape: MaskShape = serde_json::from_value(p.get("shape").cloned().unwrap_or_default()).map_err(|e| bad("mask.update", e.to_string()))?; - let comp = p.get("component").and_then(Value::as_u64).unwrap_or(0) as usize; + let comp = optional_index(p, "component", "mask.update")?.unwrap_or(0); let mid = mask_id(p, s.active_mask, "mask.update")?; masks_edit(s, "mask.update", "Edit Mask", |masks, _| { let i = find(masks, mid, "mask.update")?; @@ -381,6 +401,8 @@ pub fn specs() -> Vec { has_active, |s, p| { let c = "mask.sampleColor"; + let mid = mask_id(p, s.active_mask, c)?; + let comp = optional_index(p, "component", c)?; let (x, y) = (super::f64_req(p, "x", c)?, super::f64_req(p, "y", c)?); let add = p.get("add").and_then(Value::as_bool).unwrap_or(false); let id = s.active().ok_or_else(|| bad(c, "no active photo"))?; @@ -390,8 +412,6 @@ pub fn specs() -> Vec { let req = lightcraft_pipeline::RenderRequest::fit(384, 384); let lab = lightcraft_pipeline::color_range_sample(&src, &info, &d, &req, lightcraft_geom::Point::new(x, y)) .ok_or_else(|| bad(c, "the point is outside the photo"))?; - let mid = mask_id(p, s.active_mask, c)?; - let comp = p.get("component").and_then(Value::as_u64).map(|v| v as usize); let mut out = Vec::new(); masks_edit(s, c, "Color Range", |masks, _| { let i = find(masks, mid, c)?; @@ -482,8 +502,8 @@ pub fn specs() -> Vec { let mid = mask_id(p, s.active_mask, c)?; let d = s.develop_of(id).unwrap_or_default(); let m = d.masks.iter().find(|m| m.id == mid).ok_or_else(|| bad(c, format!("no mask {mid}")))?; - let k = match p.get("component").and_then(Value::as_u64) { - Some(k) => k as usize, + let k = match optional_index(p, "component", c)? { + Some(k) => k, None => m .components .iter() @@ -604,7 +624,7 @@ pub fn specs() -> Vec { } ), cmd!("mask.select", "Select Mask", [], None, "{id|null}", has_active, |s, p| { - s.active_mask = p.get("id").and_then(Value::as_u64).map(|v| v as u32); + s.active_mask = optional_mask_id(p, "mask.select")?; ok() }), cmd!("mask.delete", "Delete Mask", [], None, "{id?}", has_active, |s, p| { @@ -684,7 +704,7 @@ pub fn specs() -> Vec { |s, p| { let c = "mask.component"; let mid = mask_id(p, s.active_mask, c)?; - let k = p.get("component").and_then(Value::as_u64).ok_or_else(|| bad(c, "missing component"))? as usize; + let k = optional_index(p, "component", c)?.ok_or_else(|| bad(c, "missing component"))?; let action = str_param(p, "action").ok_or_else(|| bad(c, "missing action"))?.to_string(); let op: Option = match p.get("op") { Some(v) => Some(serde_json::from_value(v.clone()).map_err(|e| bad(c, e.to_string()))?), @@ -879,8 +899,8 @@ pub fn specs() -> Vec { ), cmd!("spot.select", "Select Spot", [], None, "{index|null}", has_active, |s, p| { let n = spots_len(s); - s.active_spot = match p.get("index").and_then(Value::as_u64) { - Some(i) if (i as usize) < n => Some(i as usize), + s.active_spot = match optional_index(p, "index", "spot.select")? { + Some(i) if i < n => Some(i), Some(_) => return Err(bad("spot.select", "no such spot")), None => None, }; @@ -1040,7 +1060,7 @@ fn spots_len(s: &Session) -> usize { /// `index` from the params, else the selected spot. fn spot_index(s: &Session, p: &Value, c: &str) -> Result { - let i = p.get("index").and_then(Value::as_u64).map(|v| v as usize).or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?; + let i = optional_index(p, "index", c)?.or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?; if i >= spots_len(s) { return Err(bad(c, "no such spot")); } diff --git a/crates/engine/src/tests_spots.rs b/crates/engine/src/tests_spots.rs index 988e7aac1..28a9fca0b 100644 --- a/crates/engine/src/tests_spots.rs +++ b/crates/engine/src/tests_spots.rs @@ -82,3 +82,242 @@ fn a_content_aware_spot_has_no_source() { assert_eq!(serde_json::to_value(sp).unwrap()["mode"], "fill"); assert!(s.execute("spot.update", &json!({"mode": "nonsense"})).is_err()); } + +fn target_session() -> Session { + let mut s = Session::with_demo(); + s.execute("mask.add", &json!({"kind": "colorRange"})).unwrap(); + s.execute("mask.add", &json!({"kind": "radial"})).unwrap(); + s.execute("mask.addComponent", &json!({"kind": "colorRange"})).unwrap(); + s.execute("mask.saveEffect", &json!({"id": 1, "name": "Existing"})).unwrap(); + s.execute("mask.adjust", &json!({"values": {"exposure": 1.0}})).unwrap(); + for x in [0.3, 0.7] { + s.execute("spot.add", &json!({"points": [[x, 0.5]], "source": [0.1, 0.0]})).unwrap(); + } + // A rejected command must preserve a redo branch as well as existing undo steps. + s.execute("mask.rename", &json!({"name": "Temporary"})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + s +} + +fn target_state(s: &Session) -> serde_json::Value { + let photo = s.catalog.photo(s.active().unwrap()).unwrap(); + json!({ + "develop": photo.develop, + "history": photo.history, + "selection": s.selection, + "activeMask": s.active_mask, + "activeSpot": s.active_spot, + "undo": format!("{:?}", s.undo), + "redo": format!("{:?}", s.redo), + "commits": s.commits(), + "effects": s.mask_effects, + "rememberedEffects": s.mask_tool_effects, + "journal": s.journal, + "revision": s.catalog.revision, + }) +} + +fn rejects_target(s: &mut Session, command: &str, params: &serde_json::Value) { + let before = target_state(s); + let result = s.execute(command, params); + let after = target_state(s); + assert!(result.is_err(), "{command} {params} returned {result:?}; state unchanged: {}", before == after); + assert_eq!(after, before, "{command} {params} changed edits, selection or history on rejection"); + for field in ["id", "component", "index"] { + if let Some(value) = params.get(field) + && !value.is_null() + && value.as_u64().is_none() + { + assert!( + result.unwrap_err().to_string().contains(&format!("`{field}`")), + "{command} must reject the malformed {field} before doing other work" + ); + break; + } + } +} + +fn malformed_targets() -> Vec { + vec![json!(-1), json!(0.5), json!("1"), json!(true), json!([]), json!({}), json!(u64::MAX)] +} + +#[test] +fn malformed_mask_ids_are_rejected_without_changes() { + let mut s = target_session(); + let commands = [ + ("mask.delete", json!({})), + ("mask.rename", json!({"name": "Wrong mask"})), + ("mask.invert", json!({})), + ("mask.visible", json!({"visible": false})), + ("mask.duplicate", json!({})), + ("mask.move", json!({"to": 0})), + ("mask.adjust", json!({"values": {"exposure": 1.0}})), + ("mask.update", json!({"shape": {"kind": "brush", "strokes": []}})), + ("mask.addComponent", json!({"kind": "brush"})), + ("mask.component", json!({"component": 0, "action": "invert"})), + ("mask.brushStroke", json!({"points": [[0.5, 0.5]]})), + ("mask.sampleColor", json!({"x": 0.5, "y": 0.5})), + ("mask.objectPoint", json!({"x": 0.5, "y": 0.5})), + ("mask.refineDetail", json!({})), + ]; + for (command, mut params) in commands { + for target in malformed_targets().into_iter().chain([json!(4_294_967_297_u64)]) { + params["id"] = target; + rejects_target(&mut s, command, ¶ms); + } + } +} + +#[test] +fn malformed_mask_selection_is_rejected_without_changes() { + let mut s = target_session(); + for target in malformed_targets().into_iter().filter(|v| !v.is_null()).chain([json!(4_294_967_297_u64)]) { + rejects_target(&mut s, "mask.select", &json!({"id": target})); + } +} + +#[test] +fn overflowing_mask_id_does_not_wrap_to_an_existing_mask() { + let mut s = target_session(); + rejects_target(&mut s, "mask.delete", &json!({"id": 4_294_967_297_u64})); +} + +#[test] +fn malformed_mask_effect_targets_are_rejected_without_changes() { + let mut s = target_session(); + for (command, mut params) in [ + ("mask.saveEffect", json!({"name": "Wrong effect"})), + ("mask.updateEffect", json!({"name": "Existing"})), + ("mask.applyEffect", json!({"name": "Existing"})), + ] { + for target in malformed_targets().into_iter().chain([json!(4_294_967_297_u64)]) { + params["id"] = target; + rejects_target(&mut s, command, ¶ms); + } + } + for target in malformed_targets().into_iter().chain([json!([1, -1]), json!([1, null]), json!([1, 4_294_967_297_u64])]) { + rejects_target(&mut s, "mask.applyEffect", &json!({"name": "Existing", "ids": target})); + } +} + +#[test] +fn mask_effect_targets_preserve_explicit_defaults_and_lists() { + let mut s = target_session(); + s.execute("mask.saveEffect", &json!({"id": 1, "name": "Explicit"})).unwrap(); + assert_eq!(s.mask_effects.last().unwrap().adjust.exposure, 0.0); + for (name, target) in [("Omitted", json!({})), ("Null", json!({"id": null}))] { + let mut params = target; + params["name"] = json!(name); + s.execute("mask.saveEffect", ¶ms).unwrap(); + assert_eq!(s.mask_effects.last().unwrap().adjust.exposure, 1.0); + params["name"] = json!("Existing"); + s.execute("mask.updateEffect", ¶ms).unwrap(); + assert_eq!(s.mask_effects[0].adjust.exposure, 1.0); + } + s.execute("mask.updateEffect", &json!({"id": 1, "name": "Existing"})).unwrap(); + assert_eq!(s.mask_effects[0].adjust.exposure, 0.0); + for mut params in [json!({}), json!({"id": null}), json!({"ids": null})] { + s.execute("mask.adjust", &json!({"values": {"exposure": 1.0}})).unwrap(); + params["name"] = json!("Existing"); + s.execute("mask.applyEffect", ¶ms).unwrap(); + assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks[1].adjust.exposure, 0.0); + } + s.execute("mask.adjust", &json!({"id": 1, "values": {"exposure": 1.0}})).unwrap(); + s.execute("mask.adjust", &json!({"id": 2, "values": {"exposure": 2.0}})).unwrap(); + s.execute("mask.applyEffect", &json!({"name": "Existing", "ids": [1, 2]})).unwrap(); + assert!(s.develop_of(s.active().unwrap()).unwrap().masks.iter().all(|m| m.adjust.exposure == 0.0)); + rejects_target(&mut s, "mask.applyEffect", &json!({"name": "Existing", "ids": []})); +} + +#[test] +fn malformed_mask_components_are_rejected_without_changes() { + let mut s = target_session(); + for (command, mut params) in [ + ("mask.update", json!({"shape": {"kind": "brush", "strokes": []}})), + ("mask.component", json!({"action": "invert"})), + ("mask.sampleColor", json!({"x": 0.5, "y": 0.5})), + ("mask.refineDetail", json!({})), + ] { + for target in malformed_targets().into_iter().chain([json!(4_294_967_296_u64)]) { + params["component"] = target; + rejects_target(&mut s, command, ¶ms); + } + } +} + +#[test] +fn malformed_spot_indices_are_rejected_without_changes() { + let mut s = target_session(); + for (command, mut params) in [("spot.delete", json!({})), ("spot.update", json!({"size": 0.1})), ("spot.refreshSource", json!({}))] { + for target in malformed_targets().into_iter().chain([json!(4_294_967_296_u64)]) { + params["index"] = target; + rejects_target(&mut s, command, ¶ms); + } + } +} + +#[test] +fn malformed_spot_selection_is_rejected_without_changes() { + let mut s = target_session(); + for target in malformed_targets().into_iter().filter(|v| !v.is_null()).chain([json!(4_294_967_296_u64)]) { + rejects_target(&mut s, "spot.select", &json!({"index": target})); + } +} + +#[test] +fn mask_and_spot_targets_preserve_explicit_defaults_and_selection_clearing() { + use lightcraft_develop::MaskShape; + let mut s = target_session(); + for target in [json!({}), json!({"id": null})] { + let mut params = target; + params["name"] = json!("Selected mask"); + s.execute("mask.rename", ¶ms).unwrap(); + assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks[1].name, "Selected mask"); + } + s.execute("mask.rename", &json!({"id": 1, "name": "Explicit mask"})).unwrap(); + assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks[0].name, "Explicit mask"); + assert_eq!(s.active_mask, Some(2)); + for params in [json!({"shape": {"kind": "brush", "strokes": []}}), json!({"component": null, "shape": {"kind": "brush", "strokes": []}})] { + s.execute("mask.update", ¶ms).unwrap(); + let d = s.develop_of(s.active().unwrap()).unwrap(); + assert!(matches!(d.masks[1].components[0].shape, MaskShape::Brush { .. })); + assert!(matches!(d.masks[1].components[1].shape, MaskShape::ColorRange { .. })); + } + s.execute("mask.update", &json!({"id": 1, "component": 0, "shape": {"kind": "sky"}})).unwrap(); + assert!(matches!(s.develop_of(s.active().unwrap()).unwrap().masks[0].components[0].shape, MaskShape::Sky)); + for params in [json!({"points": [[0.2, 0.2]]}), json!({"id": null, "points": [[0.4, 0.4]]})] { + s.execute("mask.brushStroke", ¶ms).unwrap(); + } + assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks.len(), 2); + for params in [json!({"x": 0.5, "y": 0.5}), json!({"component": null, "x": 0.5, "y": 0.5})] { + s.execute("mask.sampleColor", ¶ms).unwrap(); + let d = s.develop_of(s.active().unwrap()).unwrap(); + let MaskShape::ColorRange { samples, .. } = &d.masks[1].components[1].shape else { panic!("color range") }; + assert_eq!(samples.len(), 1); + } + for params in [json!({"size": 0.04}), json!({"index": null, "size": 0.04})] { + s.execute("spot.update", ¶ms).unwrap(); + assert_eq!(spots(&s)[1].size, 0.04); + } + s.execute("spot.update", &json!({"index": 0, "size": 0.03})).unwrap(); + assert_eq!(spots(&s)[0].size, 0.03); + assert_eq!(s.active_spot, Some(1)); + rejects_target(&mut s, "mask.component", &json!({"component": null, "action": "delete"})); + for params in [json!({"id": null}), json!({})] { + s.execute("mask.select", &json!({"id": 1})).unwrap(); + assert_eq!(s.active_mask, Some(1)); + s.execute("mask.select", ¶ms).unwrap(); + assert_eq!(s.active_mask, None); + } + for params in [json!({"index": null}), json!({})] { + s.execute("spot.select", &json!({"index": 0})).unwrap(); + assert_eq!(s.active_spot, Some(0)); + s.execute("spot.select", ¶ms).unwrap(); + assert_eq!(s.active_spot, None); + } + rejects_target(&mut s, "mask.delete", &json!({})); + rejects_target(&mut s, "spot.delete", &json!({})); + // An omitted mask still creates a brush mask when no mask is selected. + s.execute("mask.brushStroke", &json!({"points": [[0.5, 0.5]]})).unwrap(); + assert_eq!(s.develop_of(s.active().unwrap()).unwrap().masks.len(), 3); +}