diff --git a/Cargo.lock b/Cargo.lock index 5a77cda3..0541c98a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1625,6 +1625,7 @@ dependencies = [ "image", "log", "regex", + "same-file", "serde", "serde_json", "thiserror 2.0.21", diff --git a/Cargo.toml b/Cargo.toml index 2be79ed1..3e1101e1 100644 --- a/Cargo.toml +++ b/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" diff --git a/apps/figcraft-cli/src/files.rs b/apps/figcraft-cli/src/files.rs index a03d5b26..f746becd 100644 --- a/apps/figcraft-cli/src/files.rs +++ b/apps/figcraft-cli/src/files.rs @@ -292,8 +292,29 @@ pub fn export(file: &str, out: &str, t: &Target) -> CliResult { 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 { - 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(); + } } diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 545d872c..1ac9f0e1 100644 --- a/crates/engine/Cargo.toml +++ b/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] diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 97419c60..d75ede3e 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -163,7 +163,10 @@ pub fn specs() -> Vec { 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 { }); } 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) -> Result { } 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() diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 709b34a7..7a8ab8c9 100644 --- a/crates/engine/src/file_access.rs +++ b/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, b: impl AsRef) -> 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(); diff --git a/crates/engine/src/images_left_in_file_tests.rs b/crates/engine/src/images_left_in_file_tests.rs new file mode 100644 index 00000000..98e1cd8b --- /dev/null +++ b/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); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index f47e2ea2..9afb394c 100644 --- a/crates/engine/src/lib.rs +++ b/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, + 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 { + 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; diff --git a/crates/format/src/image_budget_tests.rs b/crates/format/src/image_budget_tests.rs index 58bb4da1..8e10cd71 100644 --- a/crates/format/src/image_budget_tests.rs +++ b/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); diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index f8a6120c..33101b62 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -190,11 +190,12 @@ pub struct Loaded { pub issues: Vec, pub versions: Vec, pub branches: Vec, + pub images_past_budget: usize, } impl From 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 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( - z: &mut zip::ZipArchive, - own: Option<&BTreeSet>, - budget: ImageBudget, -) -> (BTreeMap, Vec<(String, FormatError)>) { +fn read_images(z: &mut zip::ZipArchive, own: Option<&BTreeSet>, budget: ImageBudget) -> ImagesRead { let mut hashes: Vec = 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( 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( Err(e) => unread.push((hash, e)), } } - (images, unread) + ImagesRead { images, unread, past_budget } +} + +struct ImagesRead { + images: BTreeMap, + unread: Vec<(String, FormatError)>, + past_budget: BTreeSet, } /// 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, unread: &[(String, FormatError)], + past_budget: &BTreeSet, listed: Option<&BTreeSet>, images: &BTreeMap, ) { @@ -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(); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1196df82..b88c2493 100644 --- a/crates/ui-egui/src/lib.rs +++ b/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 {