Merge pull request #1314 from denisg0mes/keep-images-past-budget

M7: don't save over a file whose images past the budget weren't loaded

Cargo.lock

@@ -1625,6 +1625,7 @@ dependencies = [
  "image",
  "log",
  "regex",
+ "same-file",
  "serde",
  "serde_json",
  "thiserror 2.0.21",

Cargo.toml

@@ -49,6 +49,7 @@ serde = { version = "1", features = ["derive", "rc"] }
 serde_json = { version = "1", features = ["float_roundtrip"] }
 regex = "1.13"
 log = "0.4"
+same-file = "1.0.6"
 # All date/time conversion goes through chrono (no hand-rolled calendar code beyond the 400-year range extension; AGENTS.md).
 chrono = { version = "0.4.45", default-features = false, features = ["std"] }
 proptest = "1"

apps/figcraft-cli/src/files.rs

@@ -292,8 +292,29 @@ pub fn export(file: &str, out: &str, t: &Target) -> CliResult<ExitCode> {
     Ok(ExitCode::SUCCESS)
 }
 
+fn refuse_dropping_images(input: &str, out: &str, images_left_out: usize) -> CliResult<()> {
+    if images_left_out > 0 && figcraft_engine::file_access::same_file(input, out) {
+        return Err(CliError::Failed(format!(
+            "{out} wasn't written: {images_left_out} of its images weren't loaded (together they pass what FigCraft reads from one file), and writing over it would delete them. Write to another path; {input} keeps them."
+        )));
+    }
+    Ok(())
+}
+
+fn refuse_dropping_images_from_script(input: Option<(&str, usize)>, active: &figcraft_engine::DocState, out: &str) -> CliResult<()> {
+    if let Some((input, images_left_out)) = input {
+        refuse_dropping_images(input, out, images_left_out)?;
+    }
+    if let Some(images_left_out) = active.images_lost_saving_to(out) {
+        refuse_dropping_images(out, out, images_left_out)?;
+    }
+    Ok(())
+}
+
 pub fn convert(input: &str, out: &str) -> CliResult<ExitCode> {
-    let doc = load(input)?;
+    let (loaded, _) = figcraft_engine::cmd::file::load_path(input, None).map_err(|e| CliError::Failed(format!("can't open {input}: {e}")))?;
+    refuse_dropping_images(input, out, loaded.images_past_budget)?;
+    let doc = loaded.doc;
     let page = doc
         .current_page
         .filter(|p| doc.pages.contains(p))
@@ -353,13 +374,14 @@ pub fn run_script(script: &str, input: Option<&str>, out: Option<&str>) -> CliRe
     let bytes = figcraft_engine::file_access::read(script).map_err(|e| CliError::Failed(e.to_string()))?;
     let text = String::from_utf8(bytes).map_err(|_| CliError::Failed(format!("{script}: not UTF-8")))?;
     let steps = parse_script(&text).map_err(|e| CliError::Failed(format!("{script}: {e}")))?;
-    let mut s = match input {
+    let (mut s, input_images_left_out) = match input {
         Some(path) => {
             let mut s = Session::empty();
             s.execute("file.open", &json!({"path": path})).map_err(|e| CliError::Failed(format!("can't open {path}: {e}")))?;
-            s
+            let left_out = s.ds().map_err(|e| CliError::Failed(e.to_string()))?.images_lost_saving_to(path).unwrap_or(0);
+            (s, left_out)
         }
-        None => Session::new(),
+        None => (Session::new(), 0),
     };
     // One undo step and all-or-nothing, the way the same commands behave through `command.batch`
     // (MCP, the control channel, plugins): `{"$step": n}` and `{"$ref": "name"}` resolve against the
@@ -389,6 +411,7 @@ pub fn run_script(script: &str, input: Option<&str>, out: Option<&str>) -> CliRe
     }
     if let Some(out) = out {
         let ds = s.ds().map_err(|e| CliError::Failed(e.to_string()))?;
+        refuse_dropping_images_from_script(input.map(|input| (input, input_images_left_out)), ds, out)?;
         if ext(out) == "figcraft" {
             // Through the engine, so the version history is kept.
             write(out, &figcraft_engine::cmd::file::file_bytes(ds).map_err(|e| CliError::Failed(e.to_string()))?)?;
@@ -567,4 +590,24 @@ mod tests {
         assert!(!out.exists(), "a script that failed writes nothing");
         std::fs::remove_dir_all(dir).unwrap();
     }
+
+    #[test]
+    fn a_script_output_over_its_input_is_refused_whichever_tab_the_script_left_active() {
+        let dir = temp_dir("script-images-left-out");
+        let input = dir.join("big.figcraft");
+        std::fs::write(&input, b"kept").unwrap();
+        let link = dir.join("link.figcraft");
+        std::fs::hard_link(&input, &link).unwrap();
+        let (input, link, other) = (input.to_str().unwrap(), link.to_str().unwrap(), dir.join("other.figcraft"));
+        let duplicate = Session::new();
+        let active = duplicate.ds().unwrap();
+
+        for out in [input, link] {
+            let refused = refuse_dropping_images_from_script(Some((input, 3)), active, out);
+            assert!(matches!(&refused, Err(CliError::Failed(m)) if m.contains("3 of its images")), "{out}");
+        }
+        assert!(refuse_dropping_images_from_script(Some((input, 3)), active, other.to_str().unwrap()).is_ok());
+        assert!(refuse_dropping_images_from_script(Some((input, 0)), active, input).is_ok());
+        std::fs::remove_dir_all(dir).unwrap();
+    }
 }

crates/engine/Cargo.toml

@@ -28,6 +28,7 @@ thiserror = { workspace = true }
 log = { workspace = true }
 base64 = { workspace = true }
 regex = { workspace = true }
+same-file = { workspace = true }
 web-time = "1.1.0"
 
 [dev-dependencies]

crates/engine/src/cmd/file.rs

@@ -163,7 +163,10 @@ pub fn specs() -> Vec<CommandSpec> {
             let i = s.active;
             let uid = s.ds()?.uid;
             if let Some(slot) = s.docs.get_mut(i) {
-                *slot = DocState::new(loaded.doc, Some(path)).with_versions(loaded.versions).with_branches(loaded.branches);
+                *slot = DocState::new(loaded.doc, Some(path.clone()))
+                    .with_versions(loaded.versions)
+                    .with_branches(loaded.branches)
+                    .with_images_left_in_file(&path, loaded.images_past_budget);
             }
             // Edits on its branch tabs are reverted too.
             crate::branches::close_tabs(s, uid, None);
@@ -738,7 +741,13 @@ fn open(s: &mut Session, p: &Value) -> Result<Value> {
         });
     }
     let summary = import_summary(&loaded.doc);
-    s.docs.push(DocState::new(loaded.doc, native.then_some(path)).with_versions(loaded.versions).with_branches(loaded.branches));
+    let images_left_out = if native { loaded.images_past_budget } else { 0 };
+    s.docs.push(
+        DocState::new(loaded.doc, native.then(|| path.clone()))
+            .with_versions(loaded.versions)
+            .with_branches(loaded.branches)
+            .with_images_left_in_file(&path, images_left_out),
+    );
     s.active = s.docs.len() - 1;
     let mut out = json!({"index": s.active, "summary": summary});
     out[if warnings { "warnings" } else { "repairs" }] = json!(loaded.issues);
@@ -826,7 +835,9 @@ impl FileSnapshot {
         let Some(ds) = s.docs.iter_mut().find(|d| d.uid == self.uid && d.path == self.path && Arc::ptr_eq(&d.saved_doc, &self.saved)) else {
             return Ok(false);
         };
-        write(self.path.as_deref().ok_or(EngineError::NoDocument)?, bytes)?;
+        let path = self.path.as_deref().ok_or(EngineError::NoDocument)?;
+        refuse_dropping_images(ds, path)?;
+        write(path, bytes)?;
         // The history or branches may have changed since: then they still count as unsaved.
         if ds.revision == self.revision {
             ds.versions_changed = false;
@@ -838,6 +849,16 @@ impl FileSnapshot {
     }
 }
 
+fn refuse_dropping_images(ds: &DocState, path: &str) -> Result<()> {
+    match ds.images_lost_saving_to(path) {
+        Some(count) => Err(EngineError::Other(format!(
+            "{} wasn't saved: {count} of its images weren't loaded (together they pass what FigCraft reads from one file), and saving over it would delete them. Save it under another name; this file keeps them.",
+            file_stem(path)
+        ))),
+        None => Ok(()),
+    }
+}
+
 /// The branch tabs of the file in tab `uid` were saved with it.
 fn branch_tabs_saved(s: &mut Session, uid: u64) {
     for tab in s.docs.iter_mut().filter(|d| d.branch_of.as_ref().is_some_and(|(m, _)| *m == uid)) {
@@ -879,6 +900,10 @@ fn save_to(s: &mut Session, path: Option<String>) -> Result<Value> {
     }
     let i = crate::branches::file_tab(s);
     let ds = s.docs.get_mut(i).ok_or(EngineError::NoDocument)?;
+    // A download (no path) writes over no file.
+    if let Some(path) = path.as_deref() {
+        refuse_dropping_images(ds, path)?;
+    }
     let mut snapshot = FileSnapshot::of(ds);
     let follows_path = ds.path.as_deref().map_or(ds.doc.name == "Untitled", |old| ds.doc.name == file_stem(old));
     if let Some(path) = path.as_deref()

crates/engine/src/file_access.rs

@@ -115,6 +115,11 @@ pub(crate) fn within_app_folder(path: &Path, folder: &Path) -> bool {
     matches!((std::fs::canonicalize(path), std::fs::canonicalize(folder)), (Ok(p), Ok(f)) if p.starts_with(&f))
 }
 
+pub fn same_file(a: impl AsRef<Path>, b: impl AsRef<Path>) -> bool {
+    let (a, b) = (a.as_ref(), b.as_ref());
+    a == b || same_file::is_same_file(a, b).unwrap_or(false)
+}
+
 /// `p` without Windows' `\\?\` prefix, for messages.
 fn shown(p: &Path) -> String {
     let s = p.to_string_lossy();

crates/engine/src/images_left_in_file_tests.rs

@@ -0,0 +1,81 @@
+use serde_json::json;
+
+use crate::Session;
+use crate::cmd::file::FileSnapshot;
+
+fn rect(s: &mut Session, x: f64) {
+    s.execute("shape.rectangle", &json!({"x": x, "y": 0, "width": 10, "height": 10})).unwrap();
+}
+
+fn opened_with_images_left_in_file(name: &str) -> (std::path::PathBuf, String, Session) {
+    let dir = std::env::temp_dir().join(format!("figcraft-images-left-{name}-{}", std::process::id()));
+    std::fs::create_dir_all(&dir).unwrap();
+    let path = dir.join("big.figcraft").to_string_lossy().to_string();
+    let mut s = Session::new();
+    rect(&mut s, 0.0);
+    s.execute("file.save", &json!({"path": path})).unwrap();
+    let mut t = Session::empty();
+    t.execute("file.open", &json!({"path": path})).unwrap();
+    let i = t.active;
+    let tab = t.docs.remove(i).with_images_left_in_file(&path, 3);
+    t.docs.insert(i, tab);
+    (dir, path, t)
+}
+
+#[test]
+fn a_file_with_images_left_out_is_not_saved_over() {
+    let (dir, path, mut s) = opened_with_images_left_in_file("save");
+    let before = std::fs::read(&path).unwrap();
+    rect(&mut s, 20.0);
+    let err = s.execute("file.save", &json!({})).unwrap_err().to_string();
+    assert!(err.contains("3 of its images") && err.contains("another name"), "{err}");
+    assert_eq!(std::fs::read(&path).unwrap(), before);
+    assert!(s.ds().unwrap().is_dirty());
+
+    let copy = dir.join("copy.figcraft").to_string_lossy().to_string();
+    s.execute("file.saveAs", &json!({"path": copy})).unwrap();
+    assert!(!s.ds().unwrap().is_dirty());
+    rect(&mut s, 40.0);
+    s.execute("file.save", &json!({})).unwrap();
+    assert!(s.execute("file.saveAs", &json!({"path": path})).is_err());
+    assert_eq!(std::fs::read(&path).unwrap(), before);
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
+#[test]
+fn a_snapshot_encoded_in_the_background_does_not_save_over_it() {
+    let (dir, path, mut s) = opened_with_images_left_in_file("snapshot");
+    let before = std::fs::read(&path).unwrap();
+    rect(&mut s, 20.0);
+    let file = FileSnapshot::of(s.ds().unwrap());
+    let bytes = file.encode().unwrap();
+    assert!(file.save(&mut s, &bytes).is_err());
+    assert_eq!(std::fs::read(&path).unwrap(), before);
+    assert!(s.ds().unwrap().is_dirty());
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
+#[test]
+fn a_file_without_images_left_out_saves_as_before() {
+    let (dir, path, mut s) = opened_with_images_left_in_file("none");
+    let i = s.active;
+    let tab = s.docs.remove(i).with_images_left_in_file(&path, 0);
+    s.docs.insert(i, tab);
+    rect(&mut s, 20.0);
+    s.execute("file.save", &json!({})).unwrap();
+    assert!(!s.ds().unwrap().is_dirty());
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
+#[test]
+fn a_hard_link_to_the_file_is_not_saved_over() {
+    let (dir, path, mut s) = opened_with_images_left_in_file("hard-link");
+    let link = dir.join("link.figcraft");
+    let _ = std::fs::remove_file(&link);
+    std::fs::hard_link(&path, &link).unwrap();
+    let before = std::fs::read(&path).unwrap();
+    rect(&mut s, 20.0);
+    assert!(s.execute("file.saveAs", &json!({"path": link.to_string_lossy()})).is_err());
+    assert_eq!(std::fs::read(&path).unwrap(), before);
+    let _ = std::fs::remove_dir_all(&dir);
+}

crates/engine/src/lib.rs

@@ -372,6 +372,7 @@ pub struct DocState {
     /// The last duplicate while its copies are the selection: Duplicate repeats its offset (see
     /// [`cmd::edit::Duplicated`]; not saved).
     pub(crate) last_duplicate: Option<cmd::edit::Duplicated>,
+    images_left_in_file: Option<(String, usize)>,
 }
 
 /// A version shown on the canvas in place of the document: read-only until it's restored or the
@@ -434,11 +435,21 @@ impl DocState {
             rotation_origin: None,
             view_from_prefs: false,
             last_duplicate: None,
+            images_left_in_file: None,
         };
         st.zoom_to_fit();
         st
     }
 
+    pub fn with_images_left_in_file(mut self, path: &str, count: usize) -> Self {
+        self.images_left_in_file = (count > 0).then(|| (path.to_string(), count));
+        self
+    }
+
+    pub fn images_lost_saving_to(&self, path: &str) -> Option<usize> {
+        self.images_left_in_file.as_ref().filter(|(file, _)| file_access::same_file(file, path)).map(|(_, count)| *count)
+    }
+
     /// The selected ruler guide (its owner and index): it counts while no layer is selected and it
     /// is still there on this page.
     pub fn selected_guide(&self) -> Option<(NodeId, usize)> {
@@ -1727,6 +1738,8 @@ mod huge_stroke_tests;
 #[cfg(test)]
 mod id_params_behaviour_tests;
 #[cfg(test)]
+mod images_left_in_file_tests;
+#[cfg(test)]
 mod import_export_tests;
 #[cfg(test)]
 mod import_journal_tests;

crates/format/src/image_budget_tests.rs

@@ -66,6 +66,19 @@ fn images_past_the_byte_budget_are_left_out_and_reported() {
     assert!(loaded.issues.iter().any(|i| i.contains(lost[0].as_str()) && i.contains("together")), "{:?}", loaded.issues);
 }
 
+#[test]
+fn images_past_the_budget_are_counted_and_stay_in_the_file() {
+    let (bytes, _, total) = file_with_images(3);
+    let loaded = load_within(&bytes, ImageBudget { images: 100, bytes: total - 1 }).unwrap();
+    assert_eq!(loaded.images_past_budget, 1);
+    assert!(loaded.issues.iter().any(|i| i.contains("stays in the file")), "{:?}", loaded.issues);
+    assert!(!loaded.issues.iter().any(|i| i.contains("drops it")), "{:?}", loaded.issues);
+    let counted = load_within(&bytes, ImageBudget { images: 2, bytes: u64::MAX }).unwrap();
+    assert_eq!(counted.images_past_budget, 1);
+    let within = load_within(&bytes, ImageBudget { images: 3, bytes: total }).unwrap();
+    assert_eq!(within.images_past_budget, 0);
+}
+
 #[test]
 fn images_past_the_count_budget_are_left_out_and_reported() {
     let (bytes, _, _) = file_with_images(4);

crates/format/src/lib.rs

@@ -190,11 +190,12 @@ pub struct Loaded {
     pub issues: Vec<String>,
     pub versions: Vec<Version>,
     pub branches: Vec<Branch>,
+    pub images_past_budget: usize,
 }
 
 impl From<Document> for Loaded {
     fn from(doc: Document) -> Self {
-        Self { doc, issues: Vec::new(), versions: Vec::new(), branches: Vec::new() }
+        Self { doc, issues: Vec::new(), versions: Vec::new(), branches: Vec::new(), images_past_budget: 0 }
     }
 }
 
@@ -508,8 +509,9 @@ pub(crate) fn load_within(bytes: &[u8], budget: ImageBudget) -> Result<Loaded, F
     let limit = document_limit(packed, bytes.len() as u64);
     let text = String::from_utf8(read_member(&mut z, "document.json", limit)?).map_err(|e| FormatError::Corrupt(e.to_string()))?;
     let (mut doc, mut issues, own_images) = parse_json(&text)?;
-    let (images, unread) = read_images(&mut z, own_images.as_ref(), budget);
-    report_lost_images(&mut issues, &unread, own_images.as_ref(), &images);
+    let ImagesRead { images, unread, past_budget } = read_images(&mut z, own_images.as_ref(), budget);
+    report_lost_images(&mut issues, &unread, &past_budget, own_images.as_ref(), &images);
+    let images_past_budget = past_budget.len();
     match library::read(&mut z, &images, u64::MAX) {
         Ok(lib) => doc.published = lib.map(Arc::new),
         Err(e) => issues.push(format!("the published library was skipped: {e}")),
@@ -527,17 +529,13 @@ pub(crate) fn load_within(bytes: &[u8], budget: ImageBudget) -> Result<Loaded, F
         .filter(|h| own_images.as_ref().is_some_and(|o| !o.contains(*h)))
         .collect();
     doc.images = images.into_iter().filter(|(h, _)| !history_only.contains(h)).collect();
-    Ok(Loaded { doc, issues, versions, branches })
+    Ok(Loaded { doc, issues, versions, branches, images_past_budget })
 }
 
 /// The archive's `images/` members by hash, within `budget`, and the ones that couldn't be read
 /// with why. The document's own images (`own`) are read first, so a file past the budget loses
 /// the images only its history uses before its own.
-fn read_images<R: Read + Seek>(
-    z: &mut zip::ZipArchive<R>,
-    own: Option<&BTreeSet<String>>,
-    budget: ImageBudget,
-) -> (BTreeMap<String, ImageData>, Vec<(String, FormatError)>) {
+fn read_images<R: Read + Seek>(z: &mut zip::ZipArchive<R>, own: Option<&BTreeSet<String>>, budget: ImageBudget) -> ImagesRead {
     let mut hashes: Vec<String> = z.file_names().filter_map(|n| n.strip_prefix("images/")).filter(|h| !h.is_empty()).map(str::to_string).collect();
     hashes.sort_by_key(|h| !own.is_some_and(|o| o.contains(h)));
     let mut images = BTreeMap::new();
@@ -551,14 +549,17 @@ fn read_images<R: Read + Seek>(
         ok
     });
     let mut left = budget.bytes;
+    let mut past_budget = BTreeSet::new();
     for (i, hash) in hashes.into_iter().enumerate() {
         let name = format!("images/{hash}");
         if i >= budget.images {
+            past_budget.insert(hash.clone());
             unread.push((hash, FormatError::Corrupt(format!("the file has more than {} images", budget.images))));
             continue;
         }
         if z.by_name(&name).is_ok_and(|f| f.size() > left) {
             let mb = budget.bytes / (1024 * 1024);
+            past_budget.insert(hash.clone());
             unread.push((hash, FormatError::Corrupt(format!("the file's images together pass {mb} MB"))));
             continue;
         }
@@ -570,18 +571,22 @@ fn read_images<R: Read + Seek>(
             Err(e) => unread.push((hash, e)),
         }
     }
-    (images, unread)
+    ImagesRead { images, unread, past_budget }
+}
+
+struct ImagesRead {
+    images: BTreeMap<String, ImageData>,
+    unread: Vec<(String, FormatError)>,
+    past_budget: BTreeSet<String>,
 }
 
 /// Most lost images reported one by one (the rest as a count).
 const MAX_IMAGE_ISSUES: usize = 20;
 
-/// Report the images a file lost on loading: members that couldn't be read (damaged, too large)
-/// and images its document lists but the archive doesn't hold. They are left out, so saving the
-/// file again would drop them for good.
 fn report_lost_images(
     issues: &mut Vec<String>,
     unread: &[(String, FormatError)],
+    past_budget: &BTreeSet<String>,
     listed: Option<&BTreeSet<String>>,
     images: &BTreeMap<String, ImageData>,
 ) {
@@ -592,7 +597,11 @@ fn report_lost_images(
                 FormatError::Corrupt(m) | FormatError::Io(m) => m.clone(),
                 FormatError::Newer(v) => format!("format version {v}"),
             };
-            format!("image {hash} couldn't be read ({why}): it is left out, and saving this file again drops it")
+            if past_budget.contains(hash) {
+                format!("image {hash} couldn't be read ({why}): it is left out and stays in the file, which can't be saved over without losing it")
+            } else {
+                format!("image {hash} couldn't be read ({why}): it is left out, and saving this file again drops it")
+            }
         })
         .collect();
     let unread_hashes: BTreeSet<&str> = unread.iter().map(|(h, _)| h.as_str()).collect();

crates/ui-egui/src/lib.rs

@@ -1810,7 +1810,11 @@ impl FigApp {
         let on = !cfg!(target_arch = "wasm32") && self.session.prefs.auto_save;
         let saving = &self.ui.auto_saving;
         let due = |d: &&figcraft_engine::DocState| {
-            d.path.is_some() && d.branch_of.is_none() && d.preview.is_none() && d.is_dirty() && !saving.iter().any(|(uid, _)| *uid == d.uid)
+            d.path.as_deref().is_some_and(|p| d.images_lost_saving_to(p).is_none())
+                && d.branch_of.is_none()
+                && d.preview.is_none()
+                && d.is_dirty()
+                && !saving.iter().any(|(uid, _)| *uid == d.uid)
         };
         let changes = if on { self.session.docs.iter().filter(due).map(|d| d.revision).reduce(u64::wrapping_add) } else { None };
         match changes {