Keep the mask when the app closes, and when two devices disagree

The sidecar is authoritative — the catalog is a disposable index and the RAW is never written — so a mask that does not round-trip is not a persistence bug, it is lost work. Layers get their own `[mask <version> <id>]` blocks rather than being flattened into dotted keys. A layer is not a scalar: it carries a selection, a geometry and a chain of its own, and encoding a region set as `m1.region.0 = 12` would be neither readable nor mergeable. The version uuid is repeated in the header instead of relying on the block following its version, because "belongs to whichever version appeared above me" is a relationship that hand-editing, merging and older builds each break quietly. Region ids sort and deduplicate on read rather than being trusted from the file. The mask's identity is the *set*, so two devices writing the same selection in different orders must produce the same mask rather than argue about a difference that is not one. Masks merge by layer id under FR-NC-9, which is the disjoint-survives rule the parameters already follow one level up: a layer added on the phone and one added on the desktop both survive. A layer *both* sides edited resolves wholesale to the higher revision, because half of one selection plus half of another's opacity is a layer neither person made. A remote deletion is honoured, or a mask the user removed returns on every sync. An unknown mask source is skipped rather than guessed at. Applying a newer format's mask type as the nearest one this build knows would put a confidently wrong adjustment on the photograph, which is worse than applying none. 29 new tests. The interesting ones are about silence: a maskless version clearing the previous image's layers, a bare selection persisting even though it renders nothing, and a mask naming a version that is not in the file being dropped instead of landing on whichever block was open.
2026-08-22 08:39:17 +02:00
parent c6a846a1f9
commit 94cfea4748
3 changed files with 787 additions and 3 deletions
@@ -0,0 +1,385 @@
+//! Local adjustments must survive the sidecar.
+//!
+//! The sidecar is the authoritative store (ARCH §6.1) — the catalog is a
+//! disposable index and the RAW is never written. So a mask that does not
+//! round-trip is not a persistence bug, it is lost work, and the tests here
+//! are about the ways that happens quietly rather than loudly.
+
+use dr_pipeline::descriptor::ParamId;
+use dr_pipeline::mask::{MaskLayer, MaskSource};
+use dr_pipeline::{EditGraph, Sidecar, Version};
+
+fn regions(ids: &[u32]) -> MaskSource {
+    MaskSource::Regions {
+        signature: 0xdead_beef,
+        level: 300,
+        ids: ids.to_vec(),
+    }
+}
+
+/// A graph with one masked adjustment on it.
+fn graph_with_mask() -> EditGraph {
+    let mut graph = EditGraph::default_chain();
+    let mut layer = MaskLayer::new("m1", regions(&[3, 7, 12]));
+    layer.name = "Subject".into();
+    layer.set_param("exposure", ParamId("exposure"), 0.75);
+    graph.masks_mut().push(layer);
+    graph
+}
+
+fn round_trip(graph: &EditGraph) -> EditGraph {
+    let mut sidecar = Sidecar::new();
+    sidecar.put(Version::from_graph("default", "Default", graph));
+
+    let text = sidecar.to_text();
+    let parsed = Sidecar::parse(&text).expect("reparse");
+
+    let mut restored = EditGraph::default_chain();
+    parsed
+        .versions
+        .get("default")
+        .expect("version survived")
+        .apply(&mut restored);
+    restored
+}
+
+#[test]
+fn a_region_mask_survives_a_round_trip() {
+    let graph = graph_with_mask();
+    let restored = round_trip(&graph);
+
+    let layers = restored.masks().layers();
+    assert_eq!(layers.len(), 1, "the layer came back");
+
+    let layer = &layers[0];
+    assert_eq!(layer.id, "m1");
+    assert_eq!(layer.name, "Subject");
+    assert_eq!(layer.source, regions(&[3, 7, 12]));
+    assert_eq!(layer.ops.iter().find(|o| o.descriptor().id.0 == "exposure").map(|o| o.param(ParamId("exposure"))), Some(0.75));
+}
+
+#[test]
+fn writing_the_same_state_twice_is_byte_identical() {
+    // What lets a caller skip an upload by comparing content rather than
+    // trusting a dirty flag — the property the whole `versions` ordering
+    // exists for, now that masks are in the file too.
+    let graph = graph_with_mask();
+    let mut a = Sidecar::new();
+    a.put(Version::from_graph("default", "Default", &graph));
+
+    let once = a.to_text();
+    let twice = Sidecar::parse(&once).expect("reparse").to_text();
+    assert_eq!(once, twice);
+}
+
+#[test]
+fn gradients_keep_their_geometry() {
+    let mut graph = EditGraph::default_chain();
+
+    let mut linear = MaskLayer::new(
+        "m1",
+        MaskSource::Linear {
+            centre: (0.25, 0.75),
+            angle: 1.25,
+            width: 0.4,
+        },
+    );
+    linear.set_param("exposure", ParamId("exposure"), -1.0);
+    graph.masks_mut().push(linear);
+
+    let mut radial = MaskLayer::new(
+        "m2",
+        MaskSource::Radial {
+            centre: (0.6, 0.4),
+            radii: (0.3, 0.15),
+            angle: -0.5,
+            feather: 0.65,
+        },
+    );
+    radial.set_param("exposure", ParamId("exposure"), 0.5);
+    graph.masks_mut().push(radial);
+
+    let restored = round_trip(&graph);
+    let layers = restored.masks().layers();
+    assert_eq!(layers.len(), 2);
+    assert_eq!(layers[0].source, graph.masks().layers()[0].source);
+    assert_eq!(layers[1].source, graph.masks().layers()[1].source);
+}
+
+#[test]
+fn flags_and_opacity_survive() {
+    let mut graph = EditGraph::default_chain();
+    let mut layer = MaskLayer::new("m1", regions(&[1]));
+    layer.set_param("exposure", ParamId("exposure"), 1.0);
+    layer.invert = true;
+    layer.opacity = 0.35;
+    layer.enabled = false;
+    graph.masks_mut().push(layer);
+
+    let restored = round_trip(&graph);
+    let layer = &restored.masks().layers()[0];
+    assert!(layer.invert);
+    assert!((layer.opacity - 0.35).abs() < 1e-6);
+    assert!(!layer.enabled, "a disabled layer must stay disabled, not vanish");
+}
+
+/// A selection's identity is the *set*. Two files naming the same regions in
+/// different orders describe one mask and must compare equal, or the two
+/// devices that wrote them will fight forever over a difference that is not
+/// one.
+#[test]
+fn region_order_is_normalised_on_read() {
+    let text = "drsc 1\n\
+        \n[version default]\n\
+        name = Default\n\
+        revision = 1\n\
+        modified = 0\n\
+        \n[mask default m1]\n\
+        source = regions\n\
+        signature = 5\n\
+        level = 10\n\
+        regions = 12 3 7 3\n\
+        exposure.exposure = 1\n";
+
+    let parsed = Sidecar::parse(text).expect("parse");
+    let layer = &parsed.versions["default"].masks.layers()[0];
+    assert_eq!(
+        layer.source,
+        MaskSource::Regions {
+            signature: 5,
+            level: 10,
+            ids: vec![3, 7, 12],
+        },
+        "ids sort and deduplicate on the way in"
+    );
+}
+
+/// The version skew case. An older build must not delete a mask type it has
+/// never heard of *silently* — but neither may it apply one it cannot read.
+#[test]
+fn an_unknown_mask_source_is_skipped_not_guessed() {
+    let text = "drsc 1\n\
+        \n[version default]\n\
+        name = Default\n\
+        revision = 1\n\
+        modified = 0\n\
+        \n[mask default m1]\n\
+        source = luminosity\n\
+        threshold = 0.5\n\
+        exposure.exposure = 1\n";
+
+    let parsed = Sidecar::parse(text).expect("parse");
+    assert!(
+        parsed.versions["default"].masks.is_empty(),
+        "an unreadable mask must not become a wrong one"
+    );
+}
+
+#[test]
+fn a_mask_naming_no_known_version_is_dropped() {
+    let text = "drsc 1\n\
+        \n[version default]\n\
+        name = Default\n\
+        revision = 1\n\
+        modified = 0\n\
+        \n[mask ghost m1]\n\
+        source = regions\n\
+        signature = 1\n\
+        level = 1\n\
+        regions = 1\n";
+
+    let parsed = Sidecar::parse(text).expect("parse");
+    assert!(parsed.versions["default"].masks.is_empty());
+}
+
+/// Masks name their version rather than relying on file order, so a block
+/// that appears before its version still lands on it.
+#[test]
+fn a_mask_block_need_not_follow_its_version() {
+    let text = "drsc 1\n\
+        \n[mask second m1]\n\
+        source = regions\n\
+        signature = 1\n\
+        level = 1\n\
+        regions = 4 5\n\
+        exposure.exposure = 1\n\
+        \n[version first]\n\
+        name = First\n\
+        revision = 1\n\
+        modified = 0\n\
+        \n[version second]\n\
+        name = Second\n\
+        revision = 1\n\
+        modified = 0\n";
+
+    let parsed = Sidecar::parse(text).expect("parse");
+    assert!(parsed.versions["first"].masks.is_empty());
+    assert_eq!(
+        parsed.versions["second"].masks.len(),
+        1,
+        "the block named its version and should have reached it"
+    );
+}
+
+/// Loading is a replacement, not an overlay: opening an unedited image after
+/// an edited one must not leave the previous image's masks on screen.
+#[test]
+fn applying_a_maskless_version_clears_existing_masks() {
+    let mut graph = graph_with_mask();
+    assert_eq!(graph.masks().len(), 1);
+
+    let plain = Version::from_graph("clean", "Clean", &EditGraph::default_chain());
+    plain.apply(&mut graph);
+    assert!(graph.masks().is_empty());
+}
+
+#[test]
+fn a_graph_with_only_a_masked_edit_is_not_neutral() {
+    let graph = graph_with_mask();
+    assert!(!graph.is_neutral(), "a local adjustment is still an edit");
+}
+
+/// A bare selection with nothing applied to it changes no pixel, so the image
+/// is unedited — but the layer must still be written, or the selection the
+/// user made is lost on reload.
+#[test]
+fn a_selection_with_no_adjustment_still_persists() {
+    let mut graph = EditGraph::default_chain();
+    graph.masks_mut().push(MaskLayer::new("m1", regions(&[9])));
+    assert!(graph.is_neutral(), "no adjustment means no pixel changes");
+
+    let restored = round_trip(&graph);
+    assert_eq!(
+        restored.masks().len(),
+        1,
+        "the selection is work and must survive even though it renders nothing"
+    );
+}
+
+#[test]
+fn unknown_top_level_keys_still_round_trip_alongside_masks() {
+    let text = "drsc 1\n\
+        \n[version default]\n\
+        name = Default\n\
+        revision = 1\n\
+        modified = 0\n\
+        future.thing = 3\n\
+        \n[mask default m1]\n\
+        source = regions\n\
+        signature = 1\n\
+        level = 1\n\
+        regions = 2\n\
+        exposure.exposure = 1\n";
+
+    let out = Sidecar::parse(text).expect("parse").to_text();
+    assert!(out.contains("future.thing"), "unknown keys are still preserved");
+    assert!(out.contains("[mask default m1]"));
+}
+
+// ---------------------------------------------------------------------------
+// Sync merge (FR-NC-9)
+// ---------------------------------------------------------------------------
+
+fn version_with(uuid: &str, revision: u64, build: impl FnOnce(&mut EditGraph)) -> Version {
+    let mut graph = EditGraph::default_chain();
+    build(&mut graph);
+    let mut v = Version::from_graph(uuid, "Default", &graph);
+    v.revision = revision;
+    v
+}
+
+fn lit(id: &str, ids: &[u32], ev: f32) -> MaskLayer {
+    let mut layer = MaskLayer::new(id, regions(ids));
+    layer.set_param("exposure", ParamId("exposure"), ev);
+    layer
+}
+
+/// The case the whole key-wise merge exists for, one level up: a layer added
+/// on the phone and a layer added on the desktop are not a conflict.
+#[test]
+fn disjoint_layers_from_two_devices_both_survive() {
+    let base = version_with("default", 1, |_| {});
+
+    let mut ours = version_with("default", 2, |g| {
+        g.masks_mut().push(lit("m1", &[1], 1.0));
+    });
+    let theirs = version_with("default", 2, |g| {
+        g.masks_mut().push(lit("m2", &[2], -1.0));
+    });
+
+    let conflicts = ours.merge(&theirs, Some(&base));
+    assert!(conflicts.is_empty(), "different layers are not a conflict");
+    assert_eq!(ours.masks.len(), 2);
+    assert!(ours.masks.get("m1").is_some());
+    assert!(ours.masks.get("m2").is_some());
+}
+
+#[test]
+fn a_layer_only_the_remote_added_arrives() {
+    let base = version_with("default", 1, |_| {});
+    let mut ours = version_with("default", 2, |_| {});
+    let theirs = version_with("default", 3, |g| {
+        g.masks_mut().push(lit("m1", &[5], 2.0));
+    });
+
+    assert!(ours.merge(&theirs, Some(&base)).is_empty());
+    assert_eq!(ours.masks.len(), 1);
+}
+
+#[test]
+fn both_editing_one_layer_is_a_conflict_resolved_by_revision() {
+    let base = version_with("default", 1, |g| {
+        g.masks_mut().push(lit("m1", &[1], 0.5));
+    });
+
+    let mut ours = version_with("default", 2, |g| {
+        g.masks_mut().push(lit("m1", &[1], 1.0));
+    });
+    let theirs = version_with("default", 9, |g| {
+        g.masks_mut().push(lit("m1", &[1], -1.0));
+    });
+
+    let conflicts = ours.merge(&theirs, Some(&base));
+    assert_eq!(conflicts, vec![("mask".to_string(), "m1".to_string())]);
+
+    let layer = ours.masks.get("m1").expect("layer survived");
+    let ev = layer
+        .ops
+        .iter()
+        .find(|o| o.descriptor().id.0 == "exposure")
+        .map(|o| o.param(ParamId("exposure")));
+    assert_eq!(ev, Some(-1.0), "the higher revision wins the layer whole");
+}
+
+/// A layer deleted on one device and untouched on the other must stay
+/// deleted, or a mask the user removed reappears on every sync.
+#[test]
+fn a_remote_deletion_is_honoured() {
+    let base = version_with("default", 1, |g| {
+        g.masks_mut().push(lit("m1", &[1], 1.0));
+    });
+    let mut ours = version_with("default", 2, |g| {
+        g.masks_mut().push(lit("m1", &[1], 1.0));
+    });
+    let theirs = version_with("default", 3, |_| {});
+
+    assert!(ours.merge(&theirs, Some(&base)).is_empty());
+    assert!(ours.masks.is_empty(), "the deletion should not be undone");
+}
+
+#[test]
+fn an_untouched_layer_is_left_alone() {
+    let base = version_with("default", 1, |g| {
+        g.masks_mut().push(lit("m1", &[1], 1.0));
+    });
+    let mut ours = version_with("default", 2, |g| {
+        g.masks_mut().push(lit("m1", &[1], 1.0));
+        g.masks_mut().push(lit("m2", &[2], 0.5));
+    });
+    let theirs = version_with("default", 3, |g| {
+        g.masks_mut().push(lit("m1", &[1], 1.0));
+    });
+
+    assert!(ours.merge(&theirs, Some(&base)).is_empty());
+    assert_eq!(ours.masks.len(), 2, "our new layer is not a conflict");
+}