diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 8c84511..a78b7e7 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -2233,6 +2233,17 @@ impl DevelopSession { /// Captured at full scope — framing included — because the decision about /// what travels is made when the preset is *applied*. Copying, then /// changing one's mind about the crop, must not mean copying again. + /// TRACES: FR-DEV-3 | FR-CAT-8 + /// The local adjustment stack, for writing this image's edit back. + /// + /// Beside `copy_settings` rather than part of it: that returns a `Preset`, + /// which travels *between* photographs, and a mask must not — it is drawn + /// against one frame and describes nothing on another. The save path takes + /// both; the paste path takes only the preset. + pub fn masks(&self) -> &dr_pipeline::mask::MaskStack { + self.graph.masks() + } + pub fn copy_settings(&self) -> Preset { Preset::capture(&self.graph) } diff --git a/ui/dr-ui/src/library.rs b/ui/dr-ui/src/library.rs index ca54db6..65c57b8 100644 --- a/ui/dr-ui/src/library.rs +++ b/ui/dr-ui/src/library.rs @@ -375,6 +375,23 @@ pub enum Amendment { Settings { preset: dr_pipeline::Preset, scope: dr_pipeline::Scope, + /// TRACES: FR-DEV-3 | FR-CAT-8 + /// The local adjustments, when this is an image's own edit being + /// written back rather than a paste onto someone else's. + /// + /// A `Preset` is a parameter map, and a mask is not a parameter — it + /// is a rule about *where*, with a chain of its own. So a save that + /// carried only the preset wrote the sliders and silently dropped + /// every local adjustment: the sidecar format has stored masks since + /// they were added and `Version::apply` restores them, but nothing + /// ever put any there. The mask survived until the session ended and + /// then did not exist. + /// + /// `None` for a paste, which must not carry the source image's masks + /// onto the target: a mask is drawn against one photograph and means + /// nothing on another, and `Scope` cannot express that because it + /// filters parameters. + masks: Option, }, } @@ -591,8 +608,20 @@ fn amend(base: dr_pipeline::Sidecar, w: &SidecarWrite) -> dr_pipeline::Sidecar { version.rating = *rating; version.flag = *flag; } - Amendment::Settings { preset, scope } => { + Amendment::Settings { + preset, + scope, + masks, + } => { preset.amend(&mut version.params, *scope); + // Replaced wholesale rather than merged: this is the whole of the + // image's local adjustment stack as it stands, so a layer the user + // deleted has to leave the sidecar too. Cross-device merging of + // two stacks is `Sidecar::merge`'s job and happens on sync, not + // here (FR-NC-9). + if let Some(masks) = masks { + version.masks = masks.clone(); + } } } diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index 13d0f33..fde85db 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -2475,6 +2475,10 @@ fn collect_settings_writes( amendment: library::Amendment::Settings { preset: preset.clone(), scope, + // A paste carries no masks, and must not: a mask is drawn + // against one photograph and describes nothing on another. + // The target keeps whatever local adjustments it already had. + masks: None, }, }) }); diff --git a/ui/dr-ui/src/presets.rs b/ui/dr-ui/src/presets.rs index 618562d..5ca783d 100644 --- a/ui/dr-ui/src/presets.rs +++ b/ui/dr-ui/src/presets.rs @@ -177,7 +177,16 @@ pub fn load_local(image: &Path) -> Option { /// Read first for the same reason the remote writer does: the file may already /// hold a rating, or an operation this build does not know about, and writing /// a fresh document containing only the current edit would delete both. -pub fn save_local(image: &Path, preset: &Preset, scope: Scope) -> Result<(), String> { +/// `masks` carries the local adjustments when this is the image's own edit, +/// and is `None` for a paste, which must leave the target's masks alone — a +/// mask is drawn against one photograph and describes nothing on another. See +/// `library::Amendment::Settings` for the same distinction on the remote path. +pub fn save_local( + image: &Path, + preset: &Preset, + scope: Scope, + masks: Option<&dr_pipeline::mask::MaskStack>, +) -> Result<(), String> { let path = local_sidecar_path(image); // Distinguish "no sidecar yet" from "a sidecar this build cannot read". @@ -217,6 +226,11 @@ pub fn save_local(image: &Path, preset: &Preset, scope: Scope) -> Result<(), Str }); preset.amend(&mut version.params, scope); + // Wholesale, not merged: this is the stack as it stands, so a layer the + // user deleted must leave the file too. + if let Some(masks) = masks { + version.masks = masks.clone(); + } version.revision = version.revision.saturating_add(1); version.modified = now_secs(); sidecar.put(version); @@ -252,7 +266,13 @@ pub fn save_open_edit( session: &Rc>>, library: &Rc, ) { - let Some(preset) = session.borrow().as_ref().map(|s| s.copy_settings()) else { + // Both taken in one borrow: they are one edit, and a mask stack captured + // from a session that had already moved on would be a different image's. + let Some((preset, masks)) = session + .borrow() + .as_ref() + .map(|s| (s.copy_settings(), s.masks().clone())) + else { return; }; @@ -265,7 +285,7 @@ pub fn save_open_edit( // `Everything`: this is the image's *own* edit being written back, // not a paste onto someone else's. Excluding framing here would // make a crop the one adjustment that never survived a restart. - if let Err(e) = save_local(path, &preset, Scope::Everything) { + if let Err(e) = save_local(path, &preset, Scope::Everything, Some(&masks)) { log::warn!("saving {}: {e}", path.display()); } } @@ -277,6 +297,11 @@ pub fn save_open_edit( amendment: library::Amendment::Settings { preset, scope: Scope::Everything, + // The image's own edit, so the whole of it: `Everything` + // already says framing travels, and the local adjustments + // have to travel for the same reason. Without this the + // sliders were written and every mask was dropped. + masks: Some(masks), }, }; library_ui::start_sidecar_writes(window, library, vec![write]); @@ -521,6 +546,66 @@ mod tests { g } + /// The bug this exists to prevent: a local adjustment that survived until + /// the session ended and then did not exist. + /// + /// The sidecar format has stored masks since they were added, and + /// `Version::apply` restores them — but the save path wrote a `Preset`, + /// which is a parameter map, and a mask is not a parameter. So the sliders + /// came back and every mask was silently gone. Nothing reported it, + /// because nothing had failed. + #[test] + fn a_mask_survives_being_written_and_read_back() { + use dr_pipeline::mask::{MaskLayer, MaskSource}; + + let dir = tempdir("mask-roundtrip"); + let image = dir.join("a.CR2"); + + let mut graph = edited(); + let mut layer = MaskLayer::new( + "m1", + MaskSource::Radial { + centre: (0.4, 0.6), + radii: (0.2, 0.3), + angle: 0.0, + feather: 0.5, + }, + ); + layer.opacity = 0.75; + graph.masks_mut().push(layer); + assert_eq!(graph.masks().len(), 1, "the premise: the edit has a mask"); + + save_local( + &image, + &Preset::capture(&graph), + Scope::Everything, + Some(graph.masks()), + ) + .unwrap(); + + // Read it back the way opening the photograph again does. + let text = std::fs::read_to_string(local_sidecar_path(&image)).unwrap(); + let sidecar = Sidecar::parse(&text).unwrap(); + let version = sidecar.default_version().expect("a default version"); + + let mut reopened = EditGraph::default_chain(); + version.apply(&mut reopened); + + assert_eq!( + reopened.masks().len(), + 1, + "the mask must come back with the photograph" + ); + let back = &reopened.masks().layers()[0]; + assert_eq!(back.id, "m1"); + assert!((back.opacity - 0.75).abs() < 1e-6, "and with its settings"); + assert!( + matches!(back.source, MaskSource::Radial { .. }), + "and its kind: {:?}", + back.source.kind() + ); + } + #[test] fn a_sidecar_sits_beside_its_image_with_the_extension_replaced() { // Replaced, not appended, so a RAW and the JPEG beside it share one @@ -537,7 +622,7 @@ mod tests { let dir = tempdir("round-trip"); let image = dir.join("a.CR2"); - save_local(&image, &Preset::capture(&edited()), Scope::Everything).unwrap(); + save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap(); let sidecar = load_local(&image).expect("a sidecar was written"); let mut restored = EditGraph::default_chain(); @@ -563,8 +648,8 @@ mod tests { let dir = tempdir("one-version"); let image = dir.join("a.CR2"); - save_local(&image, &Preset::capture(&edited()), Scope::Everything).unwrap(); - save_local(&image, &Preset::capture(&edited()), Scope::Everything).unwrap(); + save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap(); + save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap(); assert_eq!(load_local(&image).expect("a sidecar").versions.len(), 1); } @@ -576,13 +661,13 @@ mod tests { let dir = tempdir("revision"); let image = dir.join("a.CR2"); - save_local(&image, &Preset::capture(&edited()), Scope::Everything).unwrap(); + save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap(); let first = load_local(&image) .unwrap() .default_version() .unwrap() .revision; - save_local(&image, &Preset::capture(&edited()), Scope::Everything).unwrap(); + save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap(); let second = load_local(&image) .unwrap() .default_version() @@ -610,7 +695,7 @@ mod tests { }); std::fs::write(local_sidecar_path(&image), sidecar.to_text()).unwrap(); - save_local(&image, &Preset::capture(&edited()), Scope::Everything).unwrap(); + save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap(); let back = load_local(&image).unwrap(); let v = back.default_version().unwrap(); @@ -627,7 +712,7 @@ mod tests { let image = dir.join("a.CR2"); std::fs::write(local_sidecar_path(&image), "drsc 99\n").unwrap(); - assert!(save_local(&image, &Preset::capture(&edited()), Scope::Everything).is_err()); + assert!(save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).is_err()); assert_eq!( std::fs::read_to_string(local_sidecar_path(&image)).unwrap(), "drsc 99\n", @@ -642,6 +727,7 @@ mod tests { &dir.join("a.CR2"), &Preset::capture(&edited()), Scope::Everything, + None, ) .unwrap();