Merge pull request #5626 from dotCooCoo/fix/edit-layer-nested-save

Symbols: saving leaves out a nested editing layer and keeps layers named in file data

crates/doc/src/document_ext.rs

@@ -99,13 +99,15 @@ impl Document {
     }
 
     /// Leave opacity-mask editing and symbol editing: drop the temporary editing layer, a working
-    /// copy of art the mask or the symbol already holds (the engine syncs it after every edit).
+    /// copy of art the mask or the symbol already holds (the engine syncs it after every edit). The
+    /// layer is removed wherever it is, also as a sublayer of another layer; an id that names
+    /// anything other than a layer is left alone.
     pub fn drop_edit_modes(&mut self) {
-        if let Some(me) = self.mask_edit.take() {
-            self.layers.retain(|l| l.id != me.layer);
-        }
-        if let Some(se) = self.symbol_edit.take() {
-            self.layers.retain(|l| l.id != se.layer);
+        let layers = [self.mask_edit.take().map(|me| me.layer), self.symbol_edit.take().map(|se| se.layer)];
+        for id in layers.into_iter().flatten() {
+            if self.node(id).is_some_and(Node::is_layer) {
+                let _ = self.remove(id);
+            }
         }
     }
 

crates/doc/src/lib.rs

@@ -228,8 +228,9 @@ pub struct Document {
     #[serde(default, skip_serializing)]
     pub mask_edit: Option<MaskEdit>,
     /// Symbol editing mode, while active. Never saved (see [`Document::without_edit_modes`]): the
-    /// symbol already holds the art of its editing layer.
-    #[serde(default, skip_serializing)]
+    /// symbol already holds the art of its editing layer. Never read either: a value in file data
+    /// is dropped, so the layer it names is kept and saving does not write the value again.
+    #[serde(default, skip_serializing, deserialize_with = "skip::read_as_none")]
     pub symbol_edit: Option<SymbolEdit>,
     next_id: u64,
     /// Foreign data preserved on round-trip.

crates/doc/src/skip.rs

@@ -13,3 +13,8 @@ pub fn is_empty_vec<T>(v: &[T]) -> bool {
 pub fn is_empty_map(v: &serde_json::Map<String, serde_json::Value>) -> bool {
     v.is_empty()
 }
+/// Read a field's value and drop it, giving `None`: for state that is never taken from a file.
+pub fn read_as_none<'de, D: serde::Deserializer<'de>, T>(d: D) -> Result<Option<T>, D::Error> {
+    <serde::de::IgnoredAny as serde::Deserialize>::deserialize(d)?;
+    Ok(None)
+}

crates/doc/src/tests.rs

@@ -451,3 +451,64 @@ fn pica_form_rejects_exponent_notation() {
         assert_eq!(Unit::Points.parse(s), None, "{s}");
     }
 }
+
+/// Symbol editing of symbol "S" on `layer`.
+fn symbol_edit_on(layer: NodeId) -> SymbolEdit {
+    SymbolEdit {
+        symbol: "S".into(),
+        layer,
+        instance: None,
+        xf: Affine::IDENTITY,
+        group: false,
+        changed: false,
+        back_isolation: None,
+        back_layer: None,
+    }
+}
+
+/// The opacity-mask and symbol editing layers are left out of the saved document wherever they
+/// are, also when `layer.move` made them sublayers of another layer.
+#[test]
+fn edit_mode_layers_are_dropped_as_sublayers_too() {
+    let mut d = Document::new(612.0, 792.0);
+    let host = d.layers[0].id;
+    let mask = d.add_layer(Some("Mask"));
+    let symbol = d.add_layer(Some("Symbol"));
+    d.move_node(mask, Some(host), 0).unwrap();
+    d.move_node(symbol, Some(host), usize::MAX).unwrap();
+    let object = d.alloc_id();
+    d.mask_edit = Some(MaskEdit { object, layer: mask });
+    d.symbol_edit = Some(symbol_edit_on(symbol));
+    let saved = d.without_edit_modes();
+    assert!(saved.node(mask).is_none(), "the mask editing layer is left out");
+    assert!(saved.node(symbol).is_none(), "the symbol editing layer is left out");
+    assert!(saved.node(host).is_some_and(|l| l.children().is_some_and(|c| c.is_empty())));
+    assert!(saved.mask_edit.is_none() && saved.symbol_edit.is_none());
+}
+
+/// Symbol editing is never read from a file, so a layer that file data names as the editing layer
+/// loads and stays in the document, and saving does not write the value again.
+#[test]
+fn symbol_edit_is_not_read_from_a_file() {
+    let mut d = Document::new(612.0, 792.0);
+    let layer = d.layers[0].id;
+    let mut v = serde_json::to_value(&d).unwrap();
+    v["symbol_edit"] = serde_json::to_value(symbol_edit_on(layer)).unwrap();
+    d = serde_json::from_value(v).unwrap();
+    assert_eq!(d.symbol_edit, None);
+    assert!(d.extra.is_empty(), "not kept as an unknown key");
+    d.drop_edit_modes();
+    assert!(d.node(layer).is_some(), "the layer is kept");
+    assert!(serde_json::to_value(&d).unwrap().get("symbol_edit").is_none(), "not saved again");
+}
+
+/// An opacity-mask editing id in file data that names art inside a layer, not a layer, leaves the
+/// art in place.
+#[test]
+fn mask_edit_naming_art_leaves_it() {
+    let (mut d, a, b) = doc_with_rects();
+    d.mask_edit = Some(MaskEdit { object: b, layer: a });
+    d.drop_edit_modes();
+    assert!(d.node(a).is_some() && d.node(b).is_some());
+    assert_eq!(d.mask_edit, None);
+}

crates/format/src/lib.rs

@@ -537,7 +537,11 @@ fn load_file_within(bytes: &[u8], max_nodes: usize, max_values: usize) -> Result
         }
         doc.images.insert(k, blob);
     }
-    // Saved mid-edit by an older version: drop the opacity-mask editing layer.
+    // Saved mid-edit by an older version: drop the opacity-mask editing layer. Those versions
+    // removed it only as a top-level layer, so an id naming anything else is ignored.
+    if doc.mask_edit.is_some_and(|me| !doc.layers.iter().any(|l| l.id == me.layer)) {
+        doc.mask_edit = None;
+    }
     doc.drop_edit_modes();
     // Saved before per-fill/stroke overprint: the Overprint Black list becomes overprint flags.
     doc.migrate_overprint_black();

crates/format/tests/it/native_v3.rs

@@ -157,6 +157,24 @@ fn keys_from_newer_versions_round_trip() {
     check_native_roundtrip(&d).unwrap();
 }
 
+/// An opacity-mask editing state in a file drops the editing layer it names only when that is a
+/// top-level layer, as files saved mid-edit have it: a sublayer it names loads and stays.
+#[test]
+fn saved_mask_editing_drops_only_a_top_level_layer() {
+    let mut d = Document::new(612.0, 792.0);
+    let host = d.layers[0].id;
+    let sub = d.add_layer(Some("Sub"));
+    d.move_node(sub, Some(host), 0).unwrap();
+    let top = d.add_layer(Some("Mask"));
+    for (layer, kept) in [(sub, true), (top, false)] {
+        let mut v = json_of(&save(&d, false));
+        v["document"]["mask_edit"] = json!({"object": host, "layer": layer});
+        let back = load(&serde_json::to_vec(&v).unwrap()).unwrap();
+        assert_eq!(back.node(layer).is_some(), kept, "layer {layer:?}");
+        assert!(back.mask_edit.is_none() && back.node(host).is_some());
+    }
+}
+
 /// The paragraph composer is saved only when it isn't the default (Every-line) and reads back.
 #[test]
 fn text_composer_round_trips() {