diff --git a/crates/engine/src/cmd/export.rs b/crates/engine/src/cmd/export.rs index e93d04175..472d5e7bd 100644 --- a/crates/engine/src/cmd/export.rs +++ b/crates/engine/src/cmd/export.rs @@ -17,6 +17,12 @@ impl Session { /// `app.export` params with a named `preset` expanded: the preset's params, overridden by the /// call's own (`preset` itself removed). Unknown preset names are an error. pub fn export_params(&self, p: &Value) -> std::result::Result { + self.export_params_or(p, crate::export::ExportFormat::default()) + } + + /// [`Session::export_params`] for a caller that writes `fallback` to a `path` naming no format + /// (see [`crate::export::ExportOptions::export_format`]). + pub fn export_params_or(&self, p: &Value, fallback: crate::export::ExportFormat) -> std::result::Result { let Some(name) = p.get("preset").and_then(Value::as_str) else { return Ok(p.clone()) }; let (preset, _) = self .all_export_presets() @@ -24,12 +30,17 @@ impl Session { .find(|(x, _)| x.name.eq_ignore_ascii_case(name.trim())) .ok_or_else(|| format!("unknown export preset `{name}` (see export.presets)"))?; // a preset saved by another version may hold keys this one doesn't know: not the caller's typo - let mut out = crate::export::ExportOptions::known_keys_only(&preset.params); + let mut out = crate::export::ExportOptions::known_keys(&preset.params); if let (Some(o), Some(own)) = (out.as_object_mut(), p.as_object()) { for (k, v) in own.iter().filter(|(k, _)| *k != "preset") { o.insert(k.clone(), v.clone()); } } + // the preset's depth is judged against the format written, known only now (`path`); a + // depth the call passes itself is the caller's, and validated as such + if !p.get("bitDepth").is_some_and(|d| !d.is_null()) { + crate::export::ExportOptions::drop_unfit_depth(&mut out, fallback); + } Ok(out) } } @@ -55,8 +66,10 @@ fn save(s: &mut Session, p: &Value) -> Result { if let Some(o) = params.as_object_mut() { o.retain(|k, _| !NOT_SETTINGS.contains(&k.as_str())); } - // a preset that app.export would refuse is refused here, where the typo is on screen - crate::export::ExportOptions::validate(ID, ¶ms)?; + // a preset that app.export would refuse is refused here, where the typo is on screen; without + // `format`, its `bitDepth` is judged when it exports, against the format written then + let format = params.get("format").and_then(Value::as_str).map(|_| crate::export::ExportOptions::export_format(¶ms, Default::default())); + crate::export::ExportOptions::validate_as(ID, ¶ms, format)?; let preset = ExportPreset { name: name.to_string(), params }; match s.export_presets.iter_mut().find(|x| x.name.eq_ignore_ascii_case(name)) { Some(x) => *x = preset, diff --git a/crates/engine/src/export.rs b/crates/engine/src/export.rs index ce8a34f0c..d8c56ac0e 100644 --- a/crates/engine/src/export.rs +++ b/crates/engine/src/export.rs @@ -819,15 +819,17 @@ impl ExportOptions { /// for good after an update. A `bitDepth` the stored format doesn't have (written at the /// format's own depth before issue #1952) is dropped too. pub fn known_keys_only(p: &serde_json::Value) -> serde_json::Value { + let mut p = Self::known_keys(p); + Self::drop_unfit_depth(&mut p, ExportFormat::default()); + p + } + + /// [`ExportOptions::known_keys_only`] keeping `bitDepth`, for a preset whose format may only be + /// known once the call's own params (`path`) are merged in. + pub(crate) fn known_keys(p: &serde_json::Value) -> serde_json::Value { let mut p = p.clone(); if let Some(o) = p.as_object_mut() { o.retain(|k, _| OPTION_PARAMS.contains(&k.as_str()) || TARGET_PARAMS.contains(&k.as_str())); - let format = o.get("format").and_then(serde_json::Value::as_str).map_or(Some(ExportFormat::default()), ExportFormat::parse); - if let (Some(format), Some(depth)) = (format, o.get("bitDepth").and_then(whole)) - && !Self::bit_depths(format).iter().any(|d| i64::from(d.0) == depth) - { - o.remove("bitDepth"); - } if let Some(w) = o.get_mut("watermark").and_then(serde_json::Value::as_object_mut) { w.retain(|k, _| WATERMARK_PARAMS.contains(&k.as_str())); } @@ -835,6 +837,18 @@ impl ExportOptions { p } + /// Drop a stored `bitDepth` the format `p` exports ([`ExportOptions::export_format`] with + /// `fallback`) doesn't have: it was written at the format's own depth before issue #1952. + pub(crate) fn drop_unfit_depth(p: &mut serde_json::Value, fallback: ExportFormat) { + let format = Self::export_format(p, fallback); + if let Some(o) = p.as_object_mut() + && let Some(depth) = o.get("bitDepth").and_then(whole) + && !Self::bit_depths(format).iter().any(|d| i64::from(d.0) == depth) + { + o.remove("bitDepth"); + } + } + /// Strict [`ExportOptions::from_json`] for a command that is about to export (issue #181): an /// unknown parameter or a value that cannot be read as what it is for is an error, never a /// silent default. `app.export` targets (`id`, `ids`, `path`, `dir`, `preset`, `background`) @@ -847,7 +861,7 @@ impl ExportOptions { /// format; the format returned is [`ExportOptions::export_format`], the one written. pub fn from_params_or(p: &serde_json::Value, fallback: ExportFormat) -> crate::Result { let format = Self::export_format(p, fallback); - Self::validate_as("app.export", p, format)?; + Self::validate_as("app.export", p, Some(format))?; Ok(Self { format, ..Self::from_json(p) }) } @@ -868,11 +882,12 @@ impl ExportOptions { /// kind of thing its key takes, and a `watermark` object must hold only [`WATERMARK_PARAMS`] /// with its sizes in range (issue #183). `null` means "not given", as a dropped key does. pub fn validate(cmd: &str, p: &serde_json::Value) -> crate::Result<()> { - Self::validate_as(cmd, p, Self::export_format(p, ExportFormat::default())) + Self::validate_as(cmd, p, Some(Self::export_format(p, ExportFormat::default()))) } - /// [`ExportOptions::validate`] with `bitDepth` checked against `format`, the one exported. - fn validate_as(cmd: &str, p: &serde_json::Value, format: ExportFormat) -> crate::Result<()> { + /// [`ExportOptions::validate`] with `bitDepth` checked against `format`, the one exported + /// (`None`: not known yet, as for a preset without `format`). + pub(crate) fn validate_as(cmd: &str, p: &serde_json::Value, format: Option) -> crate::Result<()> { use serde_json::Value; let bad = |msg: String| crate::cmd::bad(cmd, msg); let Some(obj) = p.as_object() else { return Err(bad("params must be an object".into())) }; @@ -981,7 +996,7 @@ impl ExportOptions { } } // issue #1952: a depth the format doesn't have would be written at the format's own depth - if let Some(depth) = obj.get("bitDepth").and_then(whole) { + if let (Some(format), Some(depth)) = (format, obj.get("bitDepth").and_then(whole)) { let mut depths: Vec = Self::bit_depths(format).iter().map(|d| d.0).collect(); if !depths.iter().any(|d| i64::from(*d) == depth) { depths.sort_unstable(); diff --git a/crates/engine/src/tests_export.rs b/crates/engine/src/tests_export.rs index 2f1a05116..8814f7581 100644 --- a/crates/engine/src/tests_export.rs +++ b/crates/engine/src/tests_export.rs @@ -98,6 +98,28 @@ fn dng_export_rejects_non_raw_photos() { let _ = std::fs::remove_dir_all(&dir); } +/// Review of #1966: a preset without `format` keeps its `bitDepth` until the format written is +/// known. With `path: a.png` it writes the 16-bit PNG it asks for; a format without that depth +/// gets its own, as stored settings do, while a depth the call itself passes is still refused. +#[test] +fn a_preset_depth_is_judged_against_the_format_written() { + use crate::export::ExportPreset; + let mut s = Session::with_demo(); + let id = s.active().unwrap(); + s.export_presets.push(ExportPreset { name: "Deep".into(), params: json!({"bitDepth": 16, "longEdge": 64}) }); + let p = s.export_params(&json!({"preset": "deep", "path": "/out/a.png"})).unwrap(); + let o = ExportOptions::from_params(&p).unwrap(); + assert_eq!((o.format, o.bit_depth), (ExportFormat::Png, Some(16))); + assert_eq!(export_photo(&mut s, id, &o, 1).unwrap().bytes[24], 16, "a 16-bit PNG"); + let p = s.export_params(&json!({"preset": "deep", "path": "/out/a.jpg"})).unwrap(); + assert_eq!(ExportOptions::from_params(&p).unwrap().bit_depth, None); + let p = s.export_params(&json!({"preset": "deep", "path": "/out/a.jpg", "bitDepth": 16})).unwrap(); + assert!(ExportOptions::from_params(&p).unwrap_err().to_string().contains("`bitDepth` must be 8 for jpeg")); + // such a preset can be saved: its depth is judged when it exports; with a format, at once + s.execute("export.savePreset", &json!({"name": "Deep 2", "params": {"bitDepth": 16}})).unwrap(); + assert!(s.execute("export.savePreset", &json!({"name": "Deep 3", "params": {"format": "jpeg", "bitDepth": 16}})).is_err()); +} + #[test] fn presets_conflicts_and_subfolders() { use crate::export::{Destination, export_batch}; diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index 6b3606ba6..fcdfcd622 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -61,7 +61,7 @@ impl Headless { /// With `path` and no `format`, the format follows the path's extension. fn export(&mut self, p: &Value) -> Result { use lightcraft_engine::export::{Destination, ExportFormat, ExportOptions, Resize, prepare_batch, run_batch}; - let p = &self.session.export_params(p)?; + let p = &self.session.export_params_or(p, ExportFormat::Png)?; // PNG for a `path` that names no format; `bitDepth` is checked against the format written let mut opts = ExportOptions::from_params_or(p, ExportFormat::Png).map_err(|e| e.to_string())?; if !ExportOptions::has_size_param(p) { diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index e2e8596f5..15b32fa5c 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -169,6 +169,21 @@ fn export_checks_the_depth_of_the_format_a_bare_path_gets() { let _ = std::fs::remove_dir_all(&base); } +/// Review of #1966: a preset's `bitDepth` is judged against the format this backend writes, PNG +/// for a `path` that names none, so a 16-bit preset without `format` writes a 16-bit PNG. +#[test] +fn a_preset_depth_is_judged_against_the_format_written() { + let mut b = Headless::demo(); + let base = std::env::temp_dir().join(format!("lc-mcp-preset-depth-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + b.session.export_presets.push(lightcraft_engine::export::ExportPreset { name: "Deep".into(), params: json!({"bitDepth": 16}) }); + b.call("app.export", json!({"preset": "Deep", "path": base.join("output").to_string_lossy(), "longEdge": 32})).unwrap(); + let bytes = std::fs::read(base.join("output")).unwrap(); + assert!(bytes.starts_with(b"\x89PNG") && bytes[24] == 16, "a 16-bit PNG: {:?}", bytes.get(..25)); + let _ = std::fs::remove_dir_all(&base); +} + /// Every option the `export` tool advertises reaches `app.export` (they were accepted, then dropped). #[test] fn export_tool_forwards_its_advertised_options() {