From 30b199f8df40b603a218599a91a2e1ad500e3c19 Mon Sep 17 00:00:00 2001 From: David Mann Date: Thu, 5 Nov 2026 04:32:00 +0100 Subject: [PATCH] Port #8659: keep name, blend mode and clipping on adjustment-layer Make steps An adjustment layer's Make step now loads its `Nm `, `Md ` and `Grup` keys as the command's `name`, `mode` and `clipToPrevious` params and saves them back. An unreadable value keeps the step as an unsupported kept step; `layer.newAdjustmentLayer.*` validates the new params like New Layer and clips through the clip-group rules. Fixes the remainder of #8384. Co-Authored-By: bommerts <44821497+bommerts@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd.rs | 2 +- crates/engine/src/cmd/adjust_params.rs | 25 +++- crates/engine/src/cmd/atn_cmds/steps_make.rs | 103 ++++++++++---- .../tests/it/atn_adjustment_make_props.rs | 128 ++++++++++++++++++ crates/engine/tests/it/main.rs | 1 + 5 files changed, 233 insertions(+), 26 deletions(-) create mode 100644 crates/engine/tests/it/atn_adjustment_make_props.rs diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index 5249ccee0b..fcab20379a 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -1218,7 +1218,7 @@ fn build() -> Vec { label, menu: &["Layer", "New Adjustment Layer"], shortcut: None, - params: Box::leak(format!(r#"{params} + {{{NEW_LAYER_PATH},"opacity":0..1=1}}"#).into_boxed_str()), + params: Box::leak(format!(r#"{params} + {{{NEW_LAYER_PATH},"opacity":0..1=1,"name":str?=the adjustment's name + number,"mode":blend mode name="normal" (not Pass Through),"clipToPrevious":bool=false}}"#).into_boxed_str()), enabled: has_doc, run: |s, p| { let kind = p.get("__kind").and_then(Value::as_str).unwrap_or("invert").to_string(); diff --git a/crates/engine/src/cmd/adjust_params.rs b/crates/engine/src/cmd/adjust_params.rs index 6e89defef2..47ae39d2f9 100644 --- a/crates/engine/src/cmd/adjust_params.rs +++ b/crates/engine/src/cmd/adjust_params.rs @@ -1,3 +1,5 @@ +use photocraft_doc::BlendMode; + use super::enabled::{check_all_unlocked, is_mask_target, paint_surface}; use super::layer_params::{layer_param, mask_new_layer}; use super::{Adjustment, ColorMode, EngineError, Layer, LayerContent, Map, Result, Session, Value, json, pixels}; @@ -12,13 +14,32 @@ pub(super) fn new_adjustment(s: &mut Session, adj: Adjustment, p: &Value, cmd: & .filter(|o| o.is_finite() && (0.0..=1.0).contains(o)) .ok_or_else(|| EngineError::BadParams { cmd: cmd.into(), msg: "opacity must be a number in 0..1".into() })? as f32, }; + let bad = |msg: String| EngineError::BadParams { cmd: cmd.into(), msg }; + // Name, blend mode and clipping read like New Layer's (#8384). + let name = match p.get("name") { + None | Some(Value::Null) => None, + Some(v) => Some(v.as_str().ok_or_else(|| bad("`name` must be a string".into()))?.to_string()), + }; + let mode = match p.get("mode") { + None | Some(Value::Null) => BlendMode::Normal, + Some(v) => { + let m = v.as_str().ok_or_else(|| bad("`mode` must be a blend mode name".into()))?; + crate::commands::blend_from_str(m).filter(|b| *b != BlendMode::PassThrough).ok_or_else(|| bad(format!("unknown layer blend mode `{m}`")))? + } + }; + let clip = match p.get("clipToPrevious") { + None | Some(Value::Null) => false, + Some(v) => v.as_bool().ok_or_else(|| bad("`clipToPrevious` must be true or false".into()))?, + }; let label = format!("New {} Layer", adj.label()); - let name = adj.label().to_string(); + let default_name = adj.label().to_string(); let id = s.edit(&label, |doc, active| { - let mut l = Layer::new(doc.next_layer_name(&name), LayerContent::Adjustment(adj)); + let mut l = Layer::new(name.clone().unwrap_or_else(|| doc.next_layer_name(&default_name)), LayerContent::Adjustment(adj)); l.opacity = opacity; + l.blend = mode; mask_new_layer(doc, &mut l, p, cmd, true)?; let id = crate::layer_place::insert_new(doc, *active, l, crate::layer_place::Place::Auto); + crate::clip_group::settle(doc, &[id], &Default::default(), clip); *active = Some(id); Ok(id) })?; diff --git a/crates/engine/src/cmd/atn_cmds/steps_make.rs b/crates/engine/src/cmd/atn_cmds/steps_make.rs index eed2974b6a..9a3fc67dbb 100644 --- a/crates/engine/src/cmd/atn_cmds/steps_make.rs +++ b/crates/engine/src/cmd/atn_cmds/steps_make.rs @@ -241,6 +241,28 @@ pub(super) fn make_from(d: &Descriptor) -> Mapped { // Recorded layer opacity uses 0..255 even in a 16-bit document. params.insert("opacity".into(), ((opacity * 255.0 / 100.0).round() / 255.0).into()); } + // Name, blend mode and clipping travel on the same command; an unreadable value + // keeps the whole step rather than replaying a different layer (#8384). + if let Some(u) = using { + let Some((_, Value::Object(params))) = mapped.first_mut() else { return Mapped::Unsupported { harmless: false } }; + if u.get("Nm ").is_some() { + let Some(name) = get_text(u, "Nm ") else { return Mapped::Unsupported { harmless: false } }; + params.insert("name".into(), name.into()); + } + if u.get("Md ").is_some() { + let Some(mode) = get_enum(u, "Md ").and_then(|m| blend_label(&m)).filter(|m| *m != "Pass Through") else { + return Mapped::Unsupported { harmless: false }; + }; + params.insert("mode".into(), mode.into()); + } + match u.get("Grup") { + None => {} + Some(D::Boolean(clip)) => { + params.insert("clipToPrevious".into(), (*clip).into()); + } + Some(_) => return Mapped::Unsupported { harmless: false }, + } + } if let Some(c) = using.and_then(|u| get_enum(u, "Clr ")) { let Some(color) = lookup(LABEL_COLORS, &c) else { return Mapped::Unsupported { harmless: false } }; mapped.push(("layer.setLabelColor".into(), json!({"color": color}))); @@ -451,32 +473,67 @@ pub(super) fn make_to(id: &str, p: &Value) -> Option { n = n.with("Fl ", enm("Fl ", fill)).with("Dpth", long(jnum(p, "depth").unwrap_or(8.0))); desc("null").with("Nw ", D::Descriptor(n)) } - _ => { - let suffix = id.strip_prefix("layer.newAdjustmentLayer.")?; - let a = adjustment_by_suffix(suffix)?; - let opacity = match p.get("opacity") { - None => None, - Some(value) => Some(value.as_f64().filter(|o| o.is_finite() && (0.0..=1.0).contains(o))?), - }; - let mut params = p.clone(); - if let Some(params) = params.as_object_mut() { - params.remove("opacity"); - } - // Layer opacity must not turn default adjustment settings into a custom preset. - let t = if params.as_object().is_some_and(|o| o.is_empty()) { - desc(a.class).with("presetKind", enm("presetKindType", "presetKindDefault")) - } else { - (a.to)(¶ms)? - }; - let mut using = desc("AdjL").with("Type", D::Descriptor(t)); - if let Some(opacity) = opacity { - using = using.with("Opct", pct(recorded_percent(opacity))); - } - desc("null").with("null", class_ref("AdjL")).with("Usng", D::Descriptor(using)) - } + _ => return adjustment_layer_to(id, p), }) } +/// `layer.newAdjustmentLayer.*` as a Make step: the adjustment's settings under `Type`, the new +/// layer's opacity, name, blend mode and clipping beside it. +fn adjustment_layer_to(id: &str, p: &Value) -> Option { + let suffix = id.strip_prefix("layer.newAdjustmentLayer.")?; + let a = adjustment_by_suffix(suffix)?; + let opacity = match p.get("opacity") { + None => None, + Some(value) => Some(value.as_f64().filter(|o| o.is_finite() && (0.0..=1.0).contains(o))?), + }; + // Layer properties go on the `Usng` layer, not into the adjustment's own settings; a + // value the reference application cannot record makes the step unexportable (#8384). + let name = match p.get("name") { + None | Some(Value::Null) => None, + Some(v) => Some(v.as_str()?), + }; + let mode = match p.get("mode") { + None | Some(Value::Null) => None, + Some(v) => { + let label = v.as_str()?; + if crate::commands::blend_from_str(label)? == photocraft_doc::BlendMode::PassThrough { + return None; + } + Some(blend_ps(label)?) + } + }; + let clip = match p.get("clipToPrevious") { + None | Some(Value::Null) => None, + Some(v) => Some(v.as_bool()?), + }; + let mut params = p.clone(); + if let Some(params) = params.as_object_mut() { + for key in ["opacity", "name", "mode", "clipToPrevious"] { + params.remove(key); + } + } + // Layer opacity must not turn default adjustment settings into a custom preset. + let t = if params.as_object().is_some_and(|o| o.is_empty()) { + desc(a.class).with("presetKind", enm("presetKindType", "presetKindDefault")) + } else { + (a.to)(¶ms)? + }; + let mut using = desc("AdjL").with("Type", D::Descriptor(t)); + if let Some(opacity) = opacity { + using = using.with("Opct", pct(recorded_percent(opacity))); + } + if let Some(name) = name { + using = using.with("Nm ", text(name)); + } + if let Some(mode) = mode { + using = using.with("Md ", enm("BlnM", mode)); + } + if let Some(clip) = clip { + using = using.with("Grup", D::Boolean(clip)); + } + Some(desc("null").with("null", class_ref("AdjL")).with("Usng", D::Descriptor(using))) +} + /// Import selection geometry or a channel source, including its optional source layer. fn selection_from(d: &Descriptor) -> Mapped { let unsupported = Mapped::Unsupported { harmless: false }; diff --git a/crates/engine/tests/it/atn_adjustment_make_props.rs b/crates/engine/tests/it/atn_adjustment_make_props.rs new file mode 100644 index 0000000000..3cd7475428 --- /dev/null +++ b/crates/engine/tests/it/atn_adjustment_make_props.rs @@ -0,0 +1,128 @@ +//! #8384: an adjustment layer's Make step keeps the new layer's name (`Nm `), blend mode +//! (`Md `) and clipping (`Grup`) on load, playback and save. +use photocraft_doc::{BlendMode, LayerContent}; +use photocraft_engine::{Session, actions_cmds::Action, atn_cmds}; +use photocraft_psd::atn::{self, AtnAction, AtnEvent, AtnFile, AtnItem}; +use photocraft_psd::descriptor::{Class, Descriptor, Id, ReferenceItem, UnicodeString, Value as D}; +use serde_json::{Value, json}; + +const NAME: &str = "Independent named adjustment"; + +fn text(s: &str) -> D { + D::Text(UnicodeString::new_nul(s)) +} + +fn blend(code: &str) -> D { + D::Enumerated { type_id: Id::new("BlnM"), value: Id::new(code) } +} + +fn levels() -> Descriptor { + Descriptor::new("Lvls").with("presetKind", D::Enumerated { type_id: Id::new("presetKindType"), value: Id::new("presetKindDefault") }) +} + +/// `Make adjustment layer` with `extra` keys after `Type`, in the order Save Actions writes them. +fn make(extra: &[(&str, D)]) -> Descriptor { + let mut using = Descriptor::new("AdjL").with("Type", D::Descriptor(levels())); + for (k, v) in extra { + using = using.with(k, v.clone()); + } + let class_ref = ReferenceItem::Class(Class { name: UnicodeString::new_nul(""), class_id: Id::new("AdjL") }); + Descriptor::new("null").with("null", D::Reference(vec![class_ref])).with("Usng", D::Descriptor(using)) +} + +fn bytes(descriptor: Descriptor) -> Vec { + let file = AtnFile { + name: UnicodeString::new_nul("Props"), + actions: vec![AtnAction { + name: UnicodeString::new_nul("Make"), + items: vec![AtnItem { event: AtnEvent::Code(*b"Mk "), dictionary: b"Make".to_vec(), descriptor: Some(descriptor), ..Default::default() }], + ..Default::default() + }], + ..Default::default() + }; + atn::write(&file).unwrap() +} + +fn doc(depth: u32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 4, "height": 3, "depth": depth})).unwrap(); + s.execute("layer.new.layer", json!({"name": "Pixels"})).unwrap(); + s +} + +#[test] +fn make_keeps_name_mode_and_clipping_through_play_and_save_at_both_depths() { + let cases: [(&[(&str, D)], Value); 5] = [ + (&[("Nm ", text(NAME))], json!({"name": NAME})), + (&[("Md ", blend("Mltp"))], json!({"mode": "Multiply"})), + (&[("Grup", D::Boolean(true))], json!({"clipToPrevious": true})), + (&[("Grup", D::Boolean(false))], json!({"clipToPrevious": false})), + (&[("Nm ", text(NAME)), ("Md ", blend("Scrn")), ("Grup", D::Boolean(true))], json!({"name": NAME, "mode": "Screen", "clipToPrevious": true})), + ]; + for depth in [8, 16] { + for (extra, params) in &cases { + let descriptor = make(extra); + let mut s = doc(depth); + let base = s.active().unwrap().active_layer.unwrap(); + let loaded = s.execute("actions.load", json!({"data": photocraft_paint::tile::b64_encode(&bytes(descriptor.clone()))})).unwrap(); + assert_eq!(loaded["unsupported"], json!([]), "{params}"); + let steps = &s.actions.list[0].steps; + assert_eq!(steps.len(), 1, "{params}"); + assert_eq!(steps[0].0, "layer.newAdjustmentLayer.levels"); + for (k, v) in params.as_object().unwrap() { + assert_eq!(steps[0].1.get(k), Some(v), "{params}: {}", steps[0].1); + } + + let played = s.execute("actions.play", json!({"action": "Make"})).unwrap(); + assert!(played.get("failed").is_none(), "{params}: {played}"); + let st = s.active().unwrap(); + let layer = st.doc.layer(st.active_layer.unwrap()).unwrap(); + assert!(matches!(layer.content, LayerContent::Adjustment(_)), "{params}"); + assert_eq!(layer.name, if params.get("name").is_some() { NAME } else { "Levels 1" }, "{params}"); + let mode = match params.get("mode").and_then(Value::as_str) { + Some("Multiply") => BlendMode::Multiply, + Some("Screen") => BlendMode::Screen, + _ => BlendMode::Normal, + }; + assert_eq!(layer.blend, mode, "{params}"); + assert_eq!(layer.clipped, params.get("clipToPrevious") == Some(&json!(true)), "{params}"); + let base = st.doc.layer(base).unwrap(); + assert_eq!((base.name.as_str(), base.blend, base.clipped), ("Pixels", BlendMode::Normal, false), "{params}"); + + // Save Actions writes the descriptor the step was loaded from. + let saved = s.execute("actions.save", json!({"set": "Props"})).unwrap(); + let data = photocraft_paint::tile::b64_decode(saved["data"].as_str().unwrap()).unwrap(); + let file = atn::parse(&data).unwrap(); + assert_eq!(file.actions[0].items[0].descriptor.as_ref(), Some(&descriptor), "{params}"); + } + } +} + +#[test] +fn unreadable_layer_properties_keep_the_step() { + for extra in [("Nm ", D::Integer(2)), ("Md ", blend("unknownBlend")), ("Md ", blend("PsTh")), ("Md ", text("Multiply")), ("Grup", text("true"))] { + let imported = atn_cmds::import(&bytes(make(std::slice::from_ref(&extra)))).unwrap(); + assert_eq!(imported.unsupported.len(), 1, "{:?}", extra.0); + assert_eq!(imported.actions[0].steps[0].0, atn_cmds::PHOTOSHOP_STEP, "{:?}", extra.0); + } +} + +#[test] +fn bad_layer_properties_fail_without_a_layer_or_an_export() { + let mut s = doc(8); + for params in [ + json!({"name": false}), + json!({"name": 3}), + json!({"mode": false}), + json!({"mode": "unknown"}), + json!({"mode": "Pass Through"}), + json!({"clipToPrevious": 1}), + json!({"clipToPrevious": "true"}), + ] { + let layers = s.active().unwrap().doc.layers.len(); + assert!(s.execute("layer.newAdjustmentLayer.levels", params.clone()).is_err(), "{params}"); + assert_eq!(s.active().unwrap().doc.layers.len(), layers, "{params} must not add a layer"); + let action = Action::new("Make", vec![("layer.newAdjustmentLayer.levels".into(), params.clone())]); + assert!(atn_cmds::export("Props", &[&action], &[], false).is_err(), "{params} must not save as a default layer"); + } +} diff --git a/crates/engine/tests/it/main.rs b/crates/engine/tests/it/main.rs index 8edc7f7082..193884c495 100644 --- a/crates/engine/tests/it/main.rs +++ b/crates/engine/tests/it/main.rs @@ -3,6 +3,7 @@ mod actions_single_history; mod affinity; mod artboard_export_names; +mod atn_adjustment_make_props; mod atn_classic_gradient; mod atn_content_aware_fill; mod atn_crystallize_cell_size;