From 24fbf4a84fe64f37422ca797a00a3457aaf9841b Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Sun, 11 Oct 2026 00:12:15 +0700 Subject: [PATCH] fix(document): restore layer comps when duplicating documents (#2429) * fix(document): keep layer comps valid across document duplication Remap saved comp and Last Document State references when cloned layers receive new IDs, including historical states and nested groups. Drop obsolete comp metadata in merged-only copies instead of carrying references to removed layers. Add focused engine regressions for #2382 and related copy workflows. * style(test): apply rustfmt layout to layer-comp regression tests --- crates/engine/src/history_cmds.rs | 4 +- crates/engine/src/image_cmds.rs | 37 ++++- crates/engine/tests/duplicate_layer_comps.rs | 149 +++++++++++++++++++ 3 files changed, 186 insertions(+), 4 deletions(-) create mode 100644 crates/engine/tests/duplicate_layer_comps.rs diff --git a/crates/engine/src/history_cmds.rs b/crates/engine/src/history_cmds.rs index 652e2326b4..d9ca98f962 100644 --- a/crates/engine/src/history_cmds.rs +++ b/crates/engine/src/history_cmds.rs @@ -7,7 +7,7 @@ use serde_json::{Value, json}; use crate::commands::{CommandSpec, int}; -use crate::image_cmds::for_each_layer; +use crate::image_cmds::reidentify_copied_layers; use crate::{EngineError, Result, Session}; fn bad(cmd: &str, msg: impl Into) -> EngineError { @@ -66,7 +66,7 @@ fn new_document(s: &mut Session, p: &Value) -> Result { _ if !label.trim().is_empty() => label, _ => format!("{} copy", doc.name), }; - for_each_layer(&mut doc.layers, &mut |l| l.id = photocraft_doc::LayerId::fresh()); + reidentify_copied_layers(&mut doc); let name = doc.name.clone(); let index = s.add_document(doc, None); Ok(json!({ "document": index, "name": name })) diff --git a/crates/engine/src/image_cmds.rs b/crates/engine/src/image_cmds.rs index 077512047b..d18fc810ec 100644 --- a/crates/engine/src/image_cmds.rs +++ b/crates/engine/src/image_cmds.rs @@ -3,7 +3,7 @@ use photocraft_algo::resample::{Resample, crop_surface, resize_surface_in_canvas, translate_surface}; use photocraft_color::{ColorMode, SampleType}; -use photocraft_doc::{Document, Effect, Effects, FxPaint, Layer, LayerContent, Size}; +use photocraft_doc::{Document, Effect, Effects, FxPaint, Layer, LayerContent, LayerId, Size}; use photocraft_geom::Rect; use photocraft_raster::Surface; use serde_json::{Value, json}; @@ -65,6 +65,34 @@ pub(crate) fn for_each_layer(layers: &mut [Layer], f: &mut dyn FnMut(&mut Layer) } } +/// Assign new layer ids to a copied document and update every saved layer-comp reference. +/// +/// A document copy needs fresh identities, but its comps must still target the copied +/// layers. Deliberately leave references to already-deleted layers untouched so their +/// missing-layer warnings remain meaningful. The same applies to Last Document State. +pub(crate) fn reidentify_copied_layers(doc: &mut Document) { + let mut ids = std::collections::HashMap::new(); + for_each_layer(&mut doc.layers, &mut |layer| { + let previous = layer.id; + layer.id = LayerId::fresh(); + ids.insert(previous, layer.id); + }); + + let remap = |comp: &mut photocraft_doc::LayerComp| { + for state in &mut comp.states { + if let Some(&new_id) = ids.get(&state.layer) { + state.layer = new_id; + } + } + }; + for comp in &mut doc.layer_comps { + remap(comp); + } + if let Some(last) = &mut doc.last_document_state { + remap(last); + } +} + pub(crate) fn scale_effects(fx: &mut Effects, k: f32) { for e in &mut fx.items { match e { @@ -479,8 +507,13 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { surf.write_region(doc.bounds(), &vals); surf.prune(); doc.layers = vec![Layer::new("Background", LayerContent::Raster(surf))]; + // A merged-only copy has no original layers for saved comps to restore. + // Don't carry dangling comp state into a single-layer flattened document. + doc.layer_comps.clear(); + doc.last_applied_comp = None; + doc.last_document_state = None; } - for_each_layer(&mut doc.layers, &mut |l| l.id = photocraft_doc::LayerId::fresh()); + reidentify_copied_layers(&mut doc); let name = doc.name.clone(); let index = s.add_document(doc, None); Ok(json!({ "document": index, "name": name })) diff --git a/crates/engine/tests/duplicate_layer_comps.rs b/crates/engine/tests/duplicate_layer_comps.rs new file mode 100644 index 0000000000..ecd8b0230b --- /dev/null +++ b/crates/engine/tests/duplicate_layer_comps.rs @@ -0,0 +1,149 @@ +//! Layer comps must continue targeting their layers after a copied document gets fresh ids. +//! Cover current/old history states, nested layers, Last Document State and merged-only copies. + +use photocraft_doc::{CompLayerState, Layer, LayerId}; +use photocraft_engine::Session; +use serde_json::json; + +fn scene() -> (Session, LayerId) { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 16, "height": 16, "background": "white"})).unwrap(); + let id = LayerId(s.execute("layer.new.layer", json!({"name": "L"})).unwrap()["layer"].as_u64().unwrap()); + (s, id) +} + +fn copied_layer(s: &Session, name: &str) -> LayerId { + s.active().unwrap().doc.walk().into_iter().find(|(_, _, l)| l.name == name).unwrap().2.id +} + +#[test] +fn image_duplicate_and_history_copy_keep_saved_comp_visibility() { + for command in ["image.duplicate", "history.newDocument"] { + let (mut s, source) = scene(); + let original = s.active().unwrap().doc.id; + s.execute("layerComp.new", json!({"name": "Shown"})).unwrap(); + s.execute("layer.setProps", json!({"layer": source.0, "visible": false})).unwrap(); + s.execute(command, json!({})).unwrap(); + + let id = copied_layer(&s, "L"); + let doc = &s.active().unwrap().doc; + assert_ne!(doc.id, original); + assert_ne!(id, source, "{command}: layers must get new identities"); + assert!(!doc.layer(id).unwrap().visible, "{command}: copied current layout"); + let comp = doc.layer_comps.iter().find(|c| c.name == "Shown").unwrap(); + assert!(comp.missing_layers(doc).is_empty(), "{command}: all captured layers still exist"); + assert_eq!(comp.state(id).unwrap().visible, Some(true), "{command}: preserve recorded state"); + + let applied = s.execute("layerComp.apply", json!({"comp": "Shown"})).unwrap(); + assert_eq!(applied["missingLayers"], 0, "{command}: no spurious missing layers"); + assert!(s.active().unwrap().doc.layer(id).unwrap().visible, "{command}: recorded visibility restored"); + + s.set_active(0); + assert!(!s.active().unwrap().doc.layer(source).unwrap().visible, "{command}: source untouched"); + assert_eq!(s.execute("layerComp.apply", json!({"comp": "Shown"})).unwrap()["missingLayers"], 0); + assert!(s.active().unwrap().doc.layer(source).unwrap().visible, "{command}: source comp still works"); + } +} + +#[test] +fn a_historical_state_uses_its_own_saved_comp_and_fresh_layer_ids() { + let (mut s, source) = scene(); + s.execute("layerComp.new", json!({"name": "Shown"})).unwrap(); + s.execute("layer.setProps", json!({"layer": source.0, "visible": false})).unwrap(); + let saved_state = s.active().unwrap().history.past_len(); + s.execute("layer.setProps", json!({"layer": source.0, "visible": true})).unwrap(); + + s.execute("history.newDocument", json!({"state": saved_state, "name": "From History"})).unwrap(); + assert_eq!(s.active().unwrap().doc.name, "From History"); + let id = copied_layer(&s, "L"); + assert_ne!(id, source); + assert!(!s.active().unwrap().doc.layer(id).unwrap().visible, "the historical hidden layout survives"); + assert_eq!(s.execute("layerComp.apply", json!({"comp": "Shown"})).unwrap()["missingLayers"], 0); + assert!(s.active().unwrap().doc.layer(id).unwrap().visible, "historical comp can restore visibility"); + s.set_active(0); + assert!(s.active().unwrap().doc.layer(source).unwrap().visible, "current source state remains unchanged"); +} + +#[test] +fn nested_group_states_are_remapped_but_genuinely_missing_layers_stay_missing() { + let (mut s, _) = scene(); + let (group, child) = s + .edit("Add nested group", |doc, _| { + let child = Layer::raster("Child", doc.pixel_format()); + let child_id = child.id; + let group = Layer::group("Group", vec![child]); + let group_id = group.id; + doc.layers.push(group); + Ok((group_id, child_id)) + }) + .unwrap(); + s.execute("layerComp.new", json!({"name": "Nested"})).unwrap(); + // An old comp may already refer to a deleted layer before duplication. + // Copying should not silently rewrite that warning to an unrelated new id. + let missing = LayerId(u64::MAX); + s.edit("Record deleted layer", |doc, _| { + let comp = doc.layer_comps.iter_mut().find(|c| c.name == "Nested").unwrap(); + comp.states.push(CompLayerState { layer: missing, visible: Some(false), position: None, appearance: None }); + Ok(()) + }) + .unwrap(); + s.execute("layer.setProps", json!({"layer": child.0, "visible": false})).unwrap(); + s.execute("image.duplicate", json!({})).unwrap(); + + let new_group = copied_layer(&s, "Group"); + let new_child = copied_layer(&s, "Child"); + assert_ne!(new_group, group); + assert_ne!(new_child, child); + let doc = &s.active().unwrap().doc; + let comp = doc.layer_comps.iter().find(|c| c.name == "Nested").unwrap(); + assert!(comp.state(new_group).is_some()); + assert_eq!(comp.state(new_child).unwrap().visible, Some(true)); + assert_eq!(comp.missing_layers(doc), vec![missing]); + assert_eq!(s.execute("layerComp.apply", json!({"comp": "Nested"})).unwrap()["missingLayers"], 1); + assert!(s.active().unwrap().doc.layer(new_child).unwrap().visible); +} + +#[test] +fn duplicated_last_document_state_restores_the_copied_layout() { + for command in ["image.duplicate", "history.newDocument"] { + let (mut s, source) = scene(); + s.execute("layerComp.new", json!({"name": "Shown"})).unwrap(); + s.execute("layer.setProps", json!({"layer": source.0, "visible": false})).unwrap(); + s.execute("layerComp.apply", json!({"comp": "Shown"})).unwrap(); + assert!(s.active().unwrap().doc.last_document_state.is_some()); + s.execute(command, json!({})).unwrap(); + + let new_id = copied_layer(&s, "L"); + let doc = &s.active().unwrap().doc; + assert!(doc.layer(new_id).unwrap().visible); + let backup = doc.last_document_state.as_ref().unwrap(); + assert_eq!(backup.state(new_id).unwrap().visible, Some(false)); + assert!(backup.missing_layers(doc).is_empty(), "{command}: backup references copied ids"); + + s.execute("layerComp.restoreLastDocumentState", json!({})).unwrap(); + assert!(!s.active().unwrap().doc.layer(new_id).unwrap().visible, "{command}: backup restores hidden layout"); + s.set_active(0); + assert!(s.active().unwrap().doc.layer(source).unwrap().visible, "{command}: source remains visible"); + } +} + +#[test] +fn merged_only_duplicate_does_not_carry_comps_for_removed_source_layers() { + let (mut s, source) = scene(); + s.execute("layerComp.new", json!({"name": "Original"})).unwrap(); + s.execute("layer.setProps", json!({"layer": source.0, "visible": false})).unwrap(); + s.execute("layerComp.apply", json!({})).unwrap(); + assert!(s.active().unwrap().doc.last_document_state.is_some()); + + s.execute("image.duplicate", json!({"mergedOnly": true})).unwrap(); + let d = &s.active().unwrap().doc; + assert_eq!(d.layers.len(), 1, "merged-only copy is a single composite layer"); + assert!(d.layer_comps.is_empty(), "old comps cannot address discarded layers"); + assert_eq!(d.last_applied_comp, None); + assert!(d.last_document_state.is_none(), "no stale saved layout on a flattened copy"); + assert!(!s.is_enabled("layerComp.apply")); + + s.set_active(0); + assert_eq!(s.active().unwrap().doc.layer_comps.len(), 1, "original comps are preserved"); + assert!(s.active().unwrap().doc.last_document_state.is_some(), "original backup is preserved"); +}