Merge pull request #7039 from dotCooCoo/fix/package-after-failed-save

Package: a failed save before packaging writes no package

crates/ui-egui/src/background.rs

@@ -190,6 +190,19 @@ pub fn run_with_progress(
     then(app, r)
 }
 
+/// Run `f` on the UI thread once job `id` has finished, right after the job's own result is
+/// applied, with that applied result (an error when the work failed). Nothing runs when no job
+/// `id` is in progress.
+pub fn after(app: &mut VectorcraftApp, id: u64, f: impl FnOnce(&mut VectorcraftApp, &Result<Value, String>) + 'static) {
+    let Some(job) = app.background.jobs.iter_mut().find(|j| j.id == id) else { return };
+    let then = std::mem::replace(&mut job.then, Box::new(|_, r| r));
+    job.then = Box::new(move |app, r| {
+        let r = then(app, r);
+        f(app, &r);
+        r
+    });
+}
+
 /// Run `f` with every save and export it starts written at once, as with the preferences off, so
 /// its result is the written file or the error (a control client's `wait: true`). The jobs already
 /// running finish first, so none of them writes over its files afterwards.

crates/ui-egui/src/dialogs/package.rs

@@ -1,14 +1,14 @@
 //! File → Package…: where the package folder goes, its name, and what it collects (the linked
 //! files, in a folder of their own or not, relinked or not; the fonts; the report). A document
-//! never saved is saved first (Save As); unsaved changes are saved before packaging. The web
-//! downloads the package as a zip.
+//! never saved is saved first (Save As); unsaved changes are saved before packaging, and when that
+//! save fails no package is written. The web downloads the package as a zip.
 //!
 //! Fields: `folder` (the location), `name` (the package folder), `copyLinks`, `linksFolder`,
 //! `relink`, `copyFonts`, `report`. OK runs `file.package` with them, then offers to show the
 //! package folder (`file.showPackage`). On the desktop with Background Export on, the package is
 //! collected and written on the background thread ([`crate::background`]) after the dialog closes.
 
-use std::cell::Cell;
+use std::cell::{Cell, RefCell};
 use std::rc::Rc;
 
 use serde_json::{Value, json};
@@ -108,21 +108,35 @@ fn confirm(app: &mut VectorcraftApp, d: &Dialog) -> Result<Value, String> {
     } else if d.str("folder").trim().is_empty() {
         return Err("choose a location for the package".into());
     }
-    // The package holds the document as saved.
+    // The package holds the document as saved: when the save fails, or asks a question and writes
+    // nothing yet, no package is written.
+    let mut saving = None;
     if app.session.active().is_some_and(|st| st.is_dirty()) {
         let r = io::save(app, vectorcraft_engine::cmd::fileio::SaveMode::Save, &json!({}), false)?;
         // The save asks first (it would replace a file that loses what opening it left out).
         if r.get("pending").is_some() {
             return Ok(r);
         }
+        // Background Save: the save is written on the worker, and its result decides the package.
+        if r["background"] == true {
+            saving = app.background.jobs.last().map(background::Job::id);
+        }
     }
     // Background Export (Preferences › File Handling): the linked files and fonts are collected
     // and the package is written from the document as it is now, while editing goes on. The jobs
-    // started before it (a save started above, an earlier package to the same folder) are written
-    // first: the worker runs its jobs in order, and a package made at once waits for them.
+    // started before it (an earlier package to the same folder) are written first: the worker
+    // runs its jobs in order, and a package made at once waits for them.
     let enabled = app.session.prefs.background_export;
     if !enabled {
+        let outcome: Rc<RefCell<Option<Result<Value, String>>>> = Rc::default();
+        if let Some(id) = saving {
+            let applied = outcome.clone();
+            background::after(app, id, move |_, r| *applied.borrow_mut() = Some(r.clone()));
+        }
         background::wait_all(app);
+        if let Some(r) = outcome.take().filter(|r| !saved(r)) {
+            return r;
+        }
     }
     let package = app.session.prepare_package(&params).map_err(|e| e.to_string()).inspect_err(|e| app.ui.status = e.clone())?;
     let label = format!("Packaging {}", d.str("name"));
@@ -134,7 +148,23 @@ fn confirm(app: &mut VectorcraftApp, d: &Dialog) -> Result<Value, String> {
             |path: &str, bytes: &[u8]| write(path, bytes).map_err(|e| EngineError::Other(if e.contains(path) { e } else { format!("{path}: {e}") }));
         package.write(&mut write).map_err(|e| e.to_string())
     };
-    let r = background::run(app, enabled, label, None, work, move |app, r| packaged(app, r, web, in_background.get()))?;
+    let start =
+        move |app: &mut VectorcraftApp| background::run(app, enabled, label, None, work, move |app, r| packaged(app, r, web, in_background.get()));
+    // Both in the background: the package, collected from the document as it is now, starts once
+    // the save is written.
+    if let Some(id) = saving.filter(|_| enabled) {
+        later.set(true);
+        app.ui.dialog = None;
+        background::after(app, id, move |app, r| {
+            if saved(r)
+                && let Err(e) = start(app)
+            {
+                app.status(e);
+            }
+        });
+        return Ok(json!({ "background": true }));
+    }
+    let r = start(app)?;
     if r["background"] == true {
         later.set(true);
         app.ui.dialog = None;
@@ -142,6 +172,12 @@ fn confirm(app: &mut VectorcraftApp, d: &Dialog) -> Result<Value, String> {
     Ok(r)
 }
 
+/// Did a save whose applied result is `r` write the document (no error, and no question asked
+/// before writing)?
+fn saved(r: &Result<Value, String>) -> bool {
+    r.as_ref().is_ok_and(|r| r.get("pending").is_none())
+}
+
 /// Report the package `r` (`file.package`'s reply): the status bar, the web's download, and the
 /// offer to show the package folder. `later`: it was written in the background, after the dialog
 /// closed, and a dialog open now is another one, which stays (the status bar names the folder).

crates/ui-egui/src/dialogs/tests_package.rs

@@ -181,7 +181,7 @@ fn a_dirty_documents_save_is_written_before_its_package() {
     let (mut app, files) = saved_with_worker(&dir, &gate);
     let (doc, out) = (dir.path("card.vectorcraft"), dir.path("out"));
     // With Background Export on, the save and the package both run in the background, and the
-    // worker writes the save first.
+    // package starts once the save is written.
     app.run("shape.rectangle", json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap();
     package_dialog(&mut app, &out);
     // A thread opens the gate after 10 s at the latest, so a confirm that waits for the save ends.
@@ -191,7 +191,7 @@ fn a_dirty_documents_save_is_written_before_its_package() {
         opener.open();
     });
     assert_eq!(confirm(&mut app).unwrap()["background"], true);
-    assert_eq!(app.background.jobs.len(), 2, "the save and the package");
+    assert_eq!(app.background.jobs.len(), 1, "the save; the package waits for it");
     gate.open();
     background::wait_all(&mut app);
     let paths = written(&files);
@@ -216,6 +216,38 @@ fn a_dirty_documents_save_is_written_before_its_package() {
     assert!(app.ui.status.starts_with("Packaged 3 files in "), "the package's status came last: {}", app.ui.status);
 }
 
+/// A save before packaging that fails writes no package, with Background Export on or off.
+#[test]
+fn a_failed_save_before_packaging_writes_no_package() {
+    let dir = Folder::new("failed-save");
+    let (doc, out) = (dir.path("card.vectorcraft"), dir.path("out"));
+    let root = std::path::Path::new(&out).join("card Folder");
+    for background_export in [true, false] {
+        let mut app = saved(&dir);
+        let (files, refused) = (Files::default(), doc.clone());
+        let recorded = files.clone();
+        app.services.write_shared = Some(Arc::new(move |p: &str, b: &[u8]| {
+            if p == refused {
+                return Err("Access is denied. (os error 5)".into());
+            }
+            recorded.lock().unwrap().push((p.to_string(), b.to_vec()));
+            Ok(())
+        }));
+        app.session.execute("prefs.set", &json!({"values": {"backgroundExport": background_export}})).unwrap();
+        app.run("shape.rectangle", json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap();
+        package_dialog(&mut app, &out);
+        let r = confirm(&mut app);
+        background::wait_all(&mut app);
+        assert!(written(&files).is_empty(), "Background Export {background_export}: {:?}", written(&files));
+        assert!(!root.exists(), "Background Export {background_export}: no package folder");
+        assert!(app.session.active().unwrap().is_dirty(), "the save failed");
+        match r {
+            Ok(r) => assert!(background_export && r["background"] == true, "{r}"),
+            Err(e) => assert!(!background_export && e.contains("Access is denied"), "{e}"),
+        }
+    }
+}
+
 #[test]
 fn a_package_made_at_once_waits_for_one_in_the_background() {
     let dir = Folder::new("twice");

docs/control-protocol.md

@@ -762,8 +762,8 @@ the `PATH` elsewhere), and any other file, or a raster image without PhotoCraft
 
 Package: File → Package… (`ui.packageDialog`) opens the `package` dialog for a saved document (a document never saved
 gets a `confirm` dialog whose OK runs `file.saveAs`). Fields: `folder` (the location), `name` (the package folder),
-`copyLinks`, `linksFolder`, `relink`, `copyFonts`, `report`. `ui.dialog.confirm` saves unsaved changes, runs
-`file.package` and then opens a `confirm` dialog whose OK runs `file.showPackage {folder}`; the web downloads the zip
+`copyLinks`, `linksFolder`, `relink`, `copyFonts`, `report`. `ui.dialog.confirm` saves unsaved changes (when that save
+fails, no package is written, also with Background Save and Background Export on), runs `file.package` and then opens a `confirm` dialog whose OK runs `file.showPackage {folder}`; the web downloads the zip
 instead. Document Info's flyout picks a category and Save… runs `docInfo.save {path?, selectionOnly?, category?}` with the
 category the panel shows (none for All Categories, which writes every category).