Merge pull request #2625 from dotCooCoo/fix/puppet-pins-keep-save-clean

Background save: dropped Puppet Warp pins, ended Edit Text and font scans leave the document saved

crates/engine/src/cmd/distortcmds.rs

@@ -746,13 +746,9 @@ fn puppet_warp(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 /// Forget the Puppet Warp pins: not an undo step, and a saved document stays saved (pins are
-/// never saved).
+/// never saved, [`crate::DocState::change_unsaved`]).
 pub(crate) fn drop_puppet_pins(st: &mut crate::DocState) {
-    let clean = !st.is_dirty();
-    Arc::make_mut(&mut st.doc).puppet = None;
-    if clean {
-        st.mark_saved();
-    }
+    st.change_unsaved(|d| Arc::make_mut(d).puppet = None);
 }
 
 fn puppet_pins(s: &mut Session, p: &Value) -> Result<Value> {
@@ -1335,7 +1331,8 @@ fn edit_text(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 /// Edit Text ends (isolation mode on `id` exits): the type is projected again, in the document and
-/// in every undo state (the flat view is never an undo step of its own).
+/// in every undo state (the flat view is never an undo step of its own, and never saved: a saved
+/// state stays saved, [`crate::DocState::change_unsaved`]).
 pub(crate) fn finish_edit_text(st: &mut crate::DocState, id: NodeId) {
     let clear = |doc: &mut Arc<Document>| {
         // Undo steps keep no lookup cache: look without building one, and drop the one an edit
@@ -1346,14 +1343,14 @@ pub(crate) fn finish_edit_text(st: &mut crate::DocState, id: NodeId) {
             p.editing = false;
         }
     };
-    let saved = Arc::ptr_eq(&st.doc, &st.saved_doc);
-    clear(&mut st.doc);
-    if saved {
-        st.saved_doc = st.doc.clone();
-    }
+    st.change_unsaved(clear);
     for e in st.history.undo.iter_mut().chain(st.history.redo.iter_mut()) {
+        let saved = Arc::ptr_eq(&e.doc, &st.saved_doc);
         clear(&mut e.doc);
         e.doc.drop_lookup_cache();
+        if saved {
+            st.saved_doc = e.doc.clone();
+        }
     }
     if let Some(it) = &mut st.interaction {
         clear(&mut it.doc);

crates/engine/src/cmd/fileio/save.rs

@@ -648,7 +648,8 @@ impl SaveJob {
 
     /// Once written: Save and Save As (except to a template) make the file `st`'s own: path,
     /// title, format, options, and the snapshot as the saved state (edits made since the job was
-    /// made keep the document modified).
+    /// made keep the document modified; changes that are not edits to save don't,
+    /// [`DocState::same_content`]).
     pub fn finish(self, st: &mut DocState) {
         let retargets = self.plan.retargets();
         if let (true, Some(path)) = (retargets, self.plan.path) {
@@ -660,6 +661,12 @@ impl SaveJob {
             if let Some(at) = self.stamped {
                 if Arc::ptr_eq(&st.doc, &self.source) {
                     st.doc = self.snapshot.clone();
+                } else if st.same_content(&self.source) {
+                    // Changed while the save ran only by changes that are not edits to save
+                    // (Puppet Warp pins dropped, Edit Text ended, type laid out again after a font
+                    // scan): with the dates, the document counts as saved.
+                    stamp_save_dates(st, at);
+                    return st.mark_saved();
                 } else {
                     // Edited while the save ran: the dates still go on, the edits stay unsaved. The
                     // undo history holds the document as it was before the dates, so that is the

crates/engine/src/cmd/fonts.rs

@@ -475,16 +475,14 @@ pub(crate) fn rescan(s: &mut Session, _: &Value) -> Result<Value> {
             }
         });
         if !texts.is_empty() {
-            let saved = !d.is_dirty();
-            let doc = std::sync::Arc::make_mut(&mut d.doc);
-            for id in texts {
-                if let Some(NodeKind::Text(t)) = doc.node_mut(id).map(|n| &mut n.kind) {
-                    refresh_bounds(t);
+            d.change_unsaved(|doc| {
+                let doc = std::sync::Arc::make_mut(doc);
+                for id in texts {
+                    if let Some(NodeKind::Text(t)) = doc.node_mut(id).map(|n| &mut n.kind) {
+                        refresh_bounds(t);
+                    }
                 }
-            }
-            if saved {
-                d.mark_saved();
-            }
+            });
         }
         d.revision += 1;
     }

crates/engine/src/cmd/recovery.rs

@@ -697,7 +697,7 @@ pub fn skip_reason(s: &Session, st: &DocState) -> Option<&'static str> {
     if !st.is_dirty() {
         return Some("no unsaved changes");
     }
-    if st.recovery.as_ref().is_some_and(|c| Arc::ptr_eq(&c.doc, &st.doc)) {
+    if st.recovery.as_ref().is_some_and(|c| st.same_content(&c.doc)) {
         return Some("unchanged since its last recovery copy");
     }
     if s.prefs.recovery_off_for_complex && object_count(&st.doc) > COMPLEX_OBJECTS {

crates/engine/src/lib.rs

@@ -287,6 +287,8 @@ pub struct DocState {
     /// The document as last saved (or opened). Structural sharing makes "unchanged" a pointer
     /// comparison, and undoing back to the saved state counts as clean.
     saved_doc: Arc<Document>,
+    /// The documents that changes that are not edits to save replaced ([`DocState::same_content`]).
+    unsaved_changes: SameContent,
     /// The layer new art goes into (the "current layer" in the Layers panel).
     pub active_layer: Option<NodeId>,
     /// The rows highlighted in the Layers panel (layers, sublayers, groups or objects, in the order
@@ -452,6 +454,36 @@ impl RecalledSelection {
     }
 }
 
+/// The documents that changes that are not edits to save (Puppet Warp pins dropped, Edit Text ended,
+/// type laid out again after a font scan) replaced, each counted as having the content of the
+/// document it was replaced by ([`DocState::change_unsaved`]). A document that nothing holds any
+/// more is dropped the next time one is added.
+#[derive(Clone, Debug, Default)]
+struct SameContent {
+    /// The document the last such change made.
+    now: std::sync::Weak<Document>,
+    /// The documents with its content that such changes replaced, oldest first.
+    before: Vec<std::sync::Weak<Document>>,
+}
+
+impl SameContent {
+    /// Note that `now` replaced `before` without a change to its content.
+    fn replaced(&mut self, before: std::sync::Weak<Document>, now: &Arc<Document>) {
+        if !self.now.ptr_eq(&before) {
+            self.before.clear();
+        }
+        self.before.retain(|d| d.strong_count() > 0);
+        if before.strong_count() > 0 {
+            self.before.push(before);
+        }
+        self.now = Arc::downgrade(now);
+    }
+    /// Whether `doc` is one of the documents with the content of `now`.
+    fn holds(&self, now: &Arc<Document>, doc: &Arc<Document>) -> bool {
+        std::ptr::eq(self.now.as_ptr(), Arc::as_ptr(now)) && self.before.iter().any(|d| std::ptr::eq(d.as_ptr(), Arc::as_ptr(doc)))
+    }
+}
+
 impl DocState {
     pub fn new(mut doc: Document, path: Option<String>) -> Self {
         let active_layer = doc.default_layer();
@@ -463,6 +495,7 @@ impl DocState {
         let doc = Arc::new(doc);
         Self {
             saved_doc: doc.clone(),
+            unsaved_changes: SameContent::default(),
             doc,
             selection: Selection::default(),
             history: History { limit: 500, ..Default::default() },
@@ -493,9 +526,10 @@ impl DocState {
             last_perspective: None,
         }
     }
-    /// Unsaved changes: the document differs from the saved one (selection changes don't count).
+    /// Unsaved changes: the document differs from the saved one (selection changes and changes that
+    /// are not edits to save don't count, [`DocState::same_content`]).
     pub fn is_dirty(&self) -> bool {
-        !Arc::ptr_eq(&self.doc, &self.saved_doc)
+        !self.same_content(&self.saved_doc)
     }
     /// Record the current document as saved.
     pub fn mark_saved(&mut self) {
@@ -506,6 +540,26 @@ impl DocState {
     pub fn mark_saved_as(&mut self, snapshot: &Arc<Document>) {
         self.saved_doc = snapshot.clone();
     }
+    /// Whether `doc` counts as having the content of the document now: it is the document, or one
+    /// that changes that are not edits to save replaced since ([`DocState::change_unsaved`]).
+    pub fn same_content(&self, doc: &Arc<Document>) -> bool {
+        Arc::ptr_eq(&self.doc, doc) || self.unsaved_changes.holds(&self.doc, doc)
+    }
+    /// Make a change that is not an edit to save with `f`, without an undo step: editing state that
+    /// is never saved, or type laid out again after a font scan (which can also refit the frame of
+    /// Auto Size area type). A saved document stays saved, and a save or recovery copy taken of the
+    /// document before counts as having its content ([`DocState::same_content`]).
+    pub(crate) fn change_unsaved(&mut self, f: impl FnOnce(&mut Arc<Document>)) {
+        let saved = !self.is_dirty();
+        let before = Arc::downgrade(&self.doc);
+        f(&mut self.doc);
+        if !std::ptr::eq(before.as_ptr(), Arc::as_ptr(&self.doc)) {
+            self.unsaved_changes.replaced(before, &self.doc);
+        }
+        if saved {
+            self.mark_saved();
+        }
+    }
     /// Keep what interaction `it` (taken from this document) changed, as one undo step.
     pub(crate) fn keep_interaction(&mut self, it: Interaction) {
         if !Arc::ptr_eq(&it.doc, &self.doc) {

crates/engine/src/tests_fontlist.rs

@@ -44,3 +44,22 @@ fn refreshing_the_font_list_redraws_type_without_editing_the_document() {
     assert!(d.revision > revision, "the canvas redraws");
     assert!(!d.is_dirty(), "nothing to save");
 }
+
+/// Refreshing the font list while a background save writes the file leaves the document saved
+/// once the file is written.
+#[test]
+fn refreshing_the_font_list_during_a_save_leaves_the_document_saved() {
+    use crate::cmd::fileio::{SaveMode, save_job, save_plan};
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"width": 200, "height": 200})).unwrap();
+    s.execute("text.create", &json!({"x": 10, "y": 40, "text": "Type", "font": "No Such Font"})).unwrap();
+    let uid = s.doc().unwrap().uid;
+    let plan = save_plan(&s, SaveMode::SaveAs, &json!({"path": "/x/type.vectorcraft", "modified": 777})).unwrap();
+    let job = save_job(&mut s, plan).unwrap();
+    s.execute("text.rescanFonts", &json!({})).unwrap();
+    job.write(|_, _| Ok(())).unwrap();
+    job.complete(&mut s, uid);
+    let d = s.doc().unwrap();
+    assert_eq!(d.doc.metadata.modified, Some(777));
+    assert!(!d.is_dirty());
+}

crates/engine/src/tests_persp_text.rs

@@ -141,6 +141,37 @@ fn edit_text_shows_it_flat_in_isolation_then_projects_it_again() {
     assert!(s2.execute("perspective.editText", &json!({"id": rect.0})).is_err());
 }
 
+/// Edit Text ending while a background save writes the file leaves the document saved once the
+/// file is written: the flat view is never saved. Saved while editing and edited after: undoing
+/// the edit once Edit Text has ended is back at what the file holds.
+#[test]
+fn ending_edit_text_during_a_save_leaves_the_document_saved() {
+    use crate::cmd::fileio::{SaveMode, save_job, save_plan, save_with};
+    let (mut s, _) = persp_session();
+    let id = persp_text(&mut s);
+    s.execute("select.set", &json!({"ids": [id.0]})).unwrap();
+    s.execute("perspective.editText", &json!({})).unwrap();
+    let uid = s.doc().unwrap().uid;
+    let plan = save_plan(&s, SaveMode::SaveAs, &json!({"path": "/x/type.vectorcraft", "modified": 777})).unwrap();
+    let job = save_job(&mut s, plan).unwrap();
+    s.execute("object.exitIsolation", &json!({})).unwrap();
+    job.write(|_, _| Ok(())).unwrap();
+    job.complete(&mut s, uid);
+    assert!(node(&s, id).projection().is_some(), "projected again");
+    let st = s.doc().unwrap();
+    assert_eq!(st.doc.metadata.modified, Some(777));
+    assert!(!st.is_dirty());
+    s.execute("perspective.editText", &json!({})).unwrap();
+    let plan = save_plan(&s, SaveMode::Save, &json!({"modified": 778})).unwrap();
+    save_with(&mut s, plan, |_, _| Ok(())).unwrap();
+    assert!(!s.doc().unwrap().is_dirty());
+    s.execute("text.setText", &json!({"id": id.0, "text": "HELLO WORLD"})).unwrap();
+    s.execute("object.exitIsolation", &json!({})).unwrap();
+    assert!(s.doc().unwrap().is_dirty());
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert!(!s.doc().unwrap().is_dirty());
+}
+
 #[test]
 fn symbols_in_perspective_stay_instances() {
     let (mut s, rect) = persp_session();

crates/engine/src/tests_puppetwarp.rs

@@ -213,6 +213,39 @@ fn a_new_selection_clears_the_pins() {
     assert_eq!(p2["pins"], p2["moved"]);
 }
 
+/// Selecting other art while a background save writes the file drops the pins and leaves the
+/// document saved once the file is written; an edit made meanwhile leaves it modified until it is
+/// undone.
+#[test]
+fn a_new_selection_during_a_save_leaves_the_document_saved() {
+    use crate::cmd::fileio::{SaveMode, save_job, save_plan};
+    let mut s = session();
+    let a = rect(&mut s, 100.0, 100.0, 300.0, 60.0);
+    let b = rect(&mut s, 100.0, 300.0, 50.0, 50.0);
+    s.select_tool("puppetWarp", ViewInfo::default()).unwrap();
+    let uid = s.doc().unwrap().uid;
+    for edit in [false, true] {
+        s.execute("select.set", &json!({"ids": [a.0]})).unwrap();
+        click(&mut s, 250.0, 150.0);
+        assert!(s.doc().unwrap().doc.puppet.is_some());
+        let plan = save_plan(&s, SaveMode::SaveAs, &json!({"path": "/x/pins.vectorcraft", "modified": 777})).unwrap();
+        let job = save_job(&mut s, plan).unwrap();
+        s.execute("select.set", &json!({"ids": [b.0]})).unwrap();
+        if edit {
+            s.execute("object.move", &json!({"dx": 10, "dy": 0})).unwrap();
+        }
+        job.write(|_, _| Ok(())).unwrap();
+        job.complete(&mut s, uid);
+        let st = s.doc().unwrap();
+        assert_eq!(st.doc.metadata.modified, Some(777));
+        assert!(st.doc.puppet.is_none(), "the pins stay dropped");
+        assert_eq!(st.is_dirty(), edit, "modified only by the edit");
+    }
+    // Undoing the edit is back at what the file holds.
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert!(!s.doc().unwrap().is_dirty());
+}
+
 #[test]
 fn puppet_warp_works_on_groups_compound_paths_and_open_strokes() {
     for kind in ["group", "compound", "open"] {

crates/engine/src/tests_recovery.rs

@@ -88,6 +88,22 @@ fn recovery_save_writes_modified_documents_once() {
     assert!(offered(&mut s).is_empty());
 }
 
+/// Selecting other art drops the Puppet Warp pins, which a copy never holds: no new copy is written.
+#[test]
+fn dropping_puppet_warp_pins_writes_no_new_copy() {
+    let (mut s, _) = session();
+    let id = s.doc().unwrap().selection.objects[0];
+    s.execute("object.puppetWarp", &json!({"id": id.0, "pins": [[35, 20]], "moved": [[35, 20]], "rest": true})).unwrap();
+    assert!(s.doc().unwrap().doc.puppet.is_some());
+    let r = s.execute("file.recovery.save", &json!({})).unwrap();
+    assert_eq!(r["saved"].as_array().unwrap().len(), 1, "{r}");
+    s.execute("select.none", &json!({})).unwrap();
+    assert!(s.doc().unwrap().doc.puppet.is_none());
+    let again = s.execute("file.recovery.save", &json!({})).unwrap();
+    assert!(again["saved"].as_array().unwrap().is_empty(), "{again}");
+    assert_eq!(again["skipped"][0]["reason"], "unchanged since its last recovery copy");
+}
+
 #[test]
 fn a_crash_leaves_copies_that_restore_modified_and_titled() {
     let (mut s, store) = session();

crates/ui-egui/src/graphics.rs

@@ -97,7 +97,7 @@ impl VectorcraftApp {
         if let Err(e) = self.session.execute("file.recovery.save", &serde_json::json!({})) {
             log::warn!("vectorcraft: no recovery copies after losing the graphics ({why}): {e}");
         }
-        self.session.documents().iter().filter(|st| st.is_dirty()).all(|st| st.recovery.as_ref().is_some_and(|c| Arc::ptr_eq(&c.doc, &st.doc)))
+        self.session.documents().iter().filter(|st| st.is_dirty()).all(|st| st.recovery.as_ref().is_some_and(|c| st.same_content(&c.doc)))
     }
 
     /// Textures live in one egui context. When the host hands the app a new one (it started again