diff --git a/crates/engine/src/export.rs b/crates/engine/src/export.rs index b9e414d51..fae08b3ad 100644 --- a/crates/engine/src/export.rs +++ b/crates/engine/src/export.rs @@ -185,6 +185,17 @@ impl DestinationMode { } } +impl Conflict { + pub fn parse(s: &str) -> Option { + match s.to_ascii_lowercase().as_str() { + "unique" => Some(Self::Unique), + "overwrite" => Some(Self::Overwrite), + "skip" => Some(Self::Skip), + _ => None, + } + } +} + /// A named set of `app.export` params. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct ExportPreset { @@ -214,6 +225,18 @@ pub enum SharpenFor { Glossy, } +impl SharpenFor { + pub fn parse(s: &str) -> Option { + match s.to_ascii_lowercase().as_str() { + "none" => Some(Self::None), + "screen" => Some(Self::Screen), + "matte" => Some(Self::Matte), + "glossy" => Some(Self::Glossy), + _ => None, + } + } +} + #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum SharpenAmount { @@ -223,6 +246,17 @@ pub enum SharpenAmount { High, } +impl SharpenAmount { + pub fn parse(s: &str) -> Option { + match s.to_ascii_lowercase().as_str() { + "low" => Some(Self::Low), + "standard" => Some(Self::Standard), + "high" => Some(Self::High), + _ => None, + } + } +} + /// Where a watermark sits. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -537,6 +571,18 @@ pub enum MetadataPolicy { None, } +impl MetadataPolicy { + pub fn parse(s: &str) -> Option { + match s.to_ascii_lowercase().as_str() { + "all" => Some(Self::All), + "allexceptcamera" => Some(Self::AllExceptCamera), + "copyright" => Some(Self::Copyright), + "none" => Some(Self::None), + _ => None, + } + } +} + /// Transfer curve of an HDR AVIF export (`hdrTransfer`): PQ (absolute, SDR white at 203 cd/m²) or /// HLG (relative to the display; BT.2100 with SDR white at signal 0.75). #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] @@ -814,6 +860,10 @@ impl ExportOptions { let s = string(cmd, k, v)?; if options.iter().any(|o| o.eq_ignore_ascii_case(s)) { Ok(()) } else { Err(bad(format!("`{k}` must be one of {}", options.join("|")))) } }; + // a choice `from_json` reads with the same `parse`, so what passes here is what gets exported + let parses = |k: &str, v: &Value, parse: fn(&str) -> bool, options: &[&str]| -> crate::Result<()> { + if parse(string(cmd, k, v)?) { Ok(()) } else { Err(bad(format!("`{k}` must be one of {}", options.join("|")))) } + }; for (k, v) in obj { if v.is_null() { continue; @@ -869,10 +919,10 @@ impl ExportOptions { "naming" | "subfolder" | "path" | "dir" | "preset" => { string(cmd, k, v)?; } - "sharpen" => one_of(k, v, &["none", "screen", "matte", "glossy"])?, - "sharpenAmount" => one_of(k, v, &["low", "standard", "high"])?, + "sharpen" => parses(k, v, |s| SharpenFor::parse(s).is_some(), &["none", "screen", "matte", "glossy"])?, + "sharpenAmount" => parses(k, v, |s| SharpenAmount::parse(s).is_some(), &["low", "standard", "high"])?, "hdrTransfer" => one_of(k, v, &["pq", "hlg"])?, - "conflict" => one_of(k, v, &["unique", "overwrite", "skip"])?, + "conflict" => parses(k, v, |s| Conflict::parse(s).is_some(), &["unique", "overwrite", "skip"])?, "destinationMode" => one_of(k, v, &["specificFolder", "sameAsOriginal"])?, "tiffCompression" => one_of(k, v, &["none", "lzw", "zip", "deflate"])?, "jxlEffort" => one_of(k, v, &["fast", "normal"])?, @@ -880,7 +930,7 @@ impl ExportOptions { serde_json::from_value::(v.clone()) .map_err(|_| bad(format!("`{k}` must be one of lossless|deflate|uncompressed|lossy")))?; } - "metadata" => one_of(k, v, &["all", "allExceptCamera", "copyright", "none"])?, + "metadata" => parses(k, v, |s| MetadataPolicy::parse(s).is_some(), &["all", "allExceptCamera", "copyright", "none"])?, "colorSpace" => { if OutputSpace::parse(string(cmd, k, v)?).is_none() { return Err(bad(format!("`{k}` must be one of srgb|displayP3|adobeRgb|proPhoto|rec2020"))); @@ -982,13 +1032,13 @@ impl ExportOptions { resize: Self::resize_from_json(p), ppi: u("ppi").filter(|v| *v > 0).map_or(d.ppi, |v| v.min(u16::MAX as u64) as u16), limit_kb: u("limitKb").filter(|v| *v > 0).map(|v| v.min(u32::MAX as u64) as u32), - sharpen: enm(p, "sharpen").unwrap_or(d.sharpen), - sharpen_amount: enm(p, "sharpenAmount").unwrap_or(d.sharpen_amount), + sharpen: s("sharpen").and_then(SharpenFor::parse).unwrap_or(d.sharpen), + sharpen_amount: s("sharpenAmount").and_then(SharpenAmount::parse).unwrap_or(d.sharpen_amount), naming: s("naming").map_or(d.naming, str::to_string), start_number: u("startNumber").map_or(d.start_number, |v| v.min(999_999) as u32), destination_mode: s("destinationMode").and_then(DestinationMode::parse).unwrap_or(d.destination_mode), subfolder: s("subfolder").map_or(d.subfolder, |v| v.trim().replace(['\\', ':', '\0'], "_")), - conflict: enm(p, "conflict").unwrap_or(d.conflict), + conflict: s("conflict").and_then(Conflict::parse).unwrap_or(d.conflict), tiff_compression: match s("tiffCompression").map(str::to_ascii_lowercase).as_deref() { Some("none") => TiffCompression::None, Some("lzw") => TiffCompression::Lzw, @@ -996,7 +1046,7 @@ impl ExportOptions { _ => d.tiff_compression, }, dng_compression: enm(p, "dngCompression").unwrap_or(d.dng_compression), - metadata: enm(p, "metadata").unwrap_or(d.metadata), + metadata: s("metadata").and_then(MetadataPolicy::parse).unwrap_or(d.metadata), remove_location: p.get("removeLocation").and_then(Value::as_bool).unwrap_or(d.remove_location), watermark: match p.get("watermark") { Some(Value::String(t)) => Some(Watermark { text: t.clone(), ..Default::default() }), @@ -3182,6 +3232,58 @@ mod tests { assert_eq!(ExportOptions::from_params(&ExportOptions::known_keys_only(&j)).unwrap(), o); } + /// A choice the validation accepts is the one exported: `metadata: "NONE"` passed the + /// case-insensitive check, then failed the exact-case read and fell back to `all`, so GPS and + /// camera data went out in a file meant to carry none. `sharpen`, `sharpenAmount` and + /// `conflict` had the same gap. + #[test] + fn accepted_choices_are_the_ones_exported() { + use serde_json::json; + let read = |k: &str, v: &str| ExportOptions::from_params(&json!({ k: v })).map_err(|e| e.to_string()); + let metadata = [ + ("all", MetadataPolicy::All), + ("allExceptCamera", MetadataPolicy::AllExceptCamera), + ("copyright", MetadataPolicy::Copyright), + ("none", MetadataPolicy::None), + ]; + for (name, want) in metadata { + for v in [name.to_string(), name.to_ascii_uppercase(), name.to_ascii_lowercase()] { + assert_eq!(read("metadata", &v).unwrap().metadata, want, "metadata: {v}"); + } + } + for (name, want) in [("none", SharpenFor::None), ("screen", SharpenFor::Screen), ("matte", SharpenFor::Matte), ("glossy", SharpenFor::Glossy)] + { + assert_eq!(read("sharpen", &name.to_ascii_uppercase()).unwrap().sharpen, want); + } + for (name, want) in [("low", SharpenAmount::Low), ("standard", SharpenAmount::Standard), ("high", SharpenAmount::High)] { + assert_eq!(read("sharpenAmount", &name.to_ascii_uppercase()).unwrap().sharpen_amount, want); + } + for (name, want) in [("unique", Conflict::Unique), ("overwrite", Conflict::Overwrite), ("skip", Conflict::Skip)] { + assert_eq!(read("conflict", &name.to_ascii_uppercase()).unwrap().conflict, want); + } + // an explicit value nothing reads as is an error, never the permissive default + for v in ["", " none", "nothing", "all-except-camera"] { + let e = read("metadata", v).unwrap_err(); + assert!(e.contains("`metadata` must be one of all|allExceptCamera|copyright|none"), "{v:?}: {e}"); + } + assert!(read("sharpenAmount", "max").unwrap_err().contains("`sharpenAmount` must be one of low|standard|high")); + assert!(read("conflict", "replace").unwrap_err().contains("`conflict` must be one of unique|overwrite|skip")); + // absent (or null) keeps the defaults + let d = ExportOptions::from_params(&json!({"metadata": null})).unwrap(); + assert_eq!( + (d.metadata, d.sharpen, d.sharpen_amount, d.conflict), + (MetadataPolicy::All, SharpenFor::None, SharpenAmount::Standard, Conflict::Unique) + ); + // `removeLocation` stays independent of the policy's spelling + let o = ExportOptions::from_params(&json!({"metadata": "AllExceptCamera", "removeLocation": true})).unwrap(); + assert_eq!((o.metadata, o.remove_location), (MetadataPolicy::AllExceptCamera, true)); + // stored settings (lenient `from_json`) read the same spellings + assert_eq!(ExportOptions::from_json(&json!({"metadata": "None"})).metadata, MetadataPolicy::None); + // and `to_json` still writes what the MCP schema lists + let j = ExportOptions { metadata: MetadataPolicy::AllExceptCamera, conflict: Conflict::Skip, ..Default::default() }.to_json(); + assert_eq!((j["metadata"].as_str(), j["conflict"].as_str()), (Some("allExceptCamera"), Some("skip"))); + } + /// Issue #183: the `watermark` object's keys, kinds and ranges are checked (and documented in /// the `app.export` param string), instead of unknown keys passing and sizes clamping silently. #[test] diff --git a/crates/engine/src/tests_export.rs b/crates/engine/src/tests_export.rs index 7b52f3d2f..2f1a05116 100644 --- a/crates/engine/src/tests_export.rs +++ b/crates/engine/src/tests_export.rs @@ -579,6 +579,30 @@ fn remove_location_holds_for_original_and_dng_exports() { let _ = std::fs::remove_dir_all(&dir); } +/// `metadata: "NONE"` as `app.export` sends it: the JPEG carries no location or camera data +/// (it used to be read as `all` and carry both). +#[test] +fn metadata_none_in_any_case_exports_no_metadata() { + let mut s = Session::with_demo(); + let id = s.active().unwrap(); + s.execute("photo.setMeta", &json!({"ids": [id.0], "gps": [48.85, 2.35], "city": "Paris"})).unwrap(); + let export = |s: &mut Session, extra: serde_json::Value| { + let mut p = json!({"format": "jpeg", "longEdge": 64}); + for (k, v) in extra.as_object().unwrap() { + p[k] = v.clone(); + } + lightcraft_meta::extract(&export_photo(s, id, &ExportOptions::from_params(&p).unwrap(), 1).unwrap().bytes) + }; + let all = export(&mut s, json!({})); + assert!(all.gps.is_some() && all.city.is_some(), "the default keeps everything: {all:?}"); + for v in ["none", "NONE", "None"] { + let m = export(&mut s, json!({"metadata": v})); + assert!(m.gps.is_none() && m.city.is_none() && m.model.is_none(), "{v}: {m:?}"); + } + let m = export(&mut s, json!({"metadata": "ALL", "removeLocation": true})); + assert!(m.gps.is_none() && m.city.is_none(), "{m:?}"); +} + /// An original raw whose own metadata holds GPS is refused under "Remove location info". #[test] fn remove_location_refuses_a_raw_that_embeds_gps() {