diff --git a/crates/ui-egui/src/file_watch.rs b/crates/ui-egui/src/file_watch.rs index da8521c86..e0e653e0e 100644 --- a/crates/ui-egui/src/file_watch.rs +++ b/crates/ui-egui/src/file_watch.rs @@ -68,11 +68,17 @@ pub struct Watched { pub path: String, /// The version PdfCraft last read or wrote (`None`: the first one the watcher sees). pub known: Option, + /// Which reading or writing of the file by PdfCraft this is for (an open, a save, Save As): + /// a version the watcher found for an earlier one is older than the document is now. + pub epoch: u64, } /// A new version of a watched file, read and ready to open. pub struct Changed { pub doc: DocId, + /// The file it was read from, and the epoch it was watched for (see [`Watched`]). + pub path: String, + pub epoch: u64, pub stamp: Stamp, pub bytes: Arc>, } @@ -81,6 +87,7 @@ pub struct Changed { struct Entry { doc: DocId, path: String, + epoch: u64, known: Option, /// A changed version seen once, waiting to be seen unchanged again. pending: Option, @@ -101,7 +108,13 @@ impl Watcher { .into_iter() .map(|w| { let prev = old.iter().find(|e| e.doc == w.doc && e.path == w.path); - Entry { known: w.known.or(prev.and_then(|e| e.known)), pending: prev.and_then(|e| e.pending), doc: w.doc, path: w.path } + Entry { + known: w.known.or(prev.and_then(|e| e.known)), + pending: prev.and_then(|e| e.pending), + doc: w.doc, + path: w.path, + epoch: w.epoch, + } }) .collect(); } @@ -132,7 +145,7 @@ impl Watcher { match read_whole_pdf(&e.path, now) { Version::Ready(bytes) => { e.known = Some(now); - found.push(Changed { doc: e.doc, stamp: now, bytes }); + found.push(Changed { doc: e.doc, path: e.path.clone(), epoch: e.epoch, stamp: now, bytes }); } // Not a whole PDF in this version (a build that failed or paused): wait for the // next one. @@ -234,16 +247,22 @@ pub struct FileWatch { inline: Watcher, /// The version of each document's file that PdfCraft last read or wrote. known: HashMap, + /// How many times PdfCraft has read or written each document's file: a version the watcher + /// found before the latest one is older than the document in memory, and is not applied + /// (#1802). + epoch: HashMap, /// The notice bar's Reload was pressed for this document. pub reload_request: Option, } impl FileWatch { - /// PdfCraft read or wrote `path` for `doc`: that version is not a change. + /// PdfCraft read or wrote `path` for `doc`: that version is not a change, and anything the + /// watcher found before it is stale. pub fn known_now(&mut self, doc: DocId, path: &str) { if let Some(stamp) = Stamp::at(path) { self.known.insert(doc, stamp); } + self.bump(doc); } /// Remember `stamp`, read before the file was, for `doc`. @@ -251,15 +270,25 @@ impl FileWatch { if let Some(stamp) = stamp { self.known.insert(doc, stamp); } + self.bump(doc); + } + + fn bump(&mut self, doc: DocId) { + let epoch = self.epoch.entry(doc).or_default(); + *epoch = epoch.wrapping_add(1); + } + + fn epoch(&self, doc: DocId) -> u64 { + self.epoch.get(&doc).copied().unwrap_or(0) } } impl PdfCraftApp { /// The documents to watch: every open one with a file, while the preference is on. - fn watched(&self) -> impl Iterator)> { + fn watched(&self) -> impl Iterator, u64)> { self.views.iter().filter(|_| self.reload_changed_files).filter_map(|v| { let path = self.session.get(v.id)?.path.as_deref()?; - Some((v.id, path, self.watch.known.get(&v.id).copied())) + Some((v.id, path, self.watch.known.get(&v.id).copied(), self.watch.epoch(v.id))) }) } @@ -270,12 +299,15 @@ impl PdfCraftApp { self.reload_from_disk(id); } let mut sent = self.watch.sent.iter(); - let unchanged = self.watched().all(|(doc, path, known)| sent.next().is_some_and(|w| w.doc == doc && w.path == path && w.known == known)) + let unchanged = self + .watched() + .all(|(doc, path, known, epoch)| sent.next().is_some_and(|w| w.doc == doc && w.path == path && w.known == known && w.epoch == epoch)) && sent.next().is_none(); if !unchanged { - let list: Vec = self.watched().map(|(doc, path, known)| Watched { doc, path: path.to_owned(), known }).collect(); + let list: Vec = self.watched().map(|(doc, path, known, epoch)| Watched { doc, path: path.to_owned(), known, epoch }).collect(); let open: Vec = self.views.iter().map(|v| v.id).collect(); self.watch.known.retain(|doc, _| open.contains(doc)); + self.watch.epoch.retain(|doc, _| open.contains(doc)); if self.send_to_watcher(list.clone()) { self.watch.sent = list; } @@ -321,6 +353,13 @@ impl PdfCraftApp { /// changes. fn file_changed(&mut self, c: Changed) { let Some(i) = self.views.iter().position(|v| v.id == c.doc) else { return }; + // Found for an earlier reading or writing of the file, or for another file than the + // document's now (a save, Save As or a reopen came between): older than the document in + // memory, so it is not applied over it (#1802). The watcher was told the newer version. + let same_file = self.session.get(c.doc).and_then(|d| d.path.as_deref()) == Some(c.path.as_str()); + if !same_file || self.watch.epoch(c.doc) != c.epoch { + return; + } // PdfCraft wrote this version itself (a save), or it was handled already. if self.watch.known.get(&c.doc) == Some(&c.stamp) { return; @@ -375,3 +414,133 @@ impl PdfCraftApp { } } } + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use egui_kittest::Harness; + + use super::*; + use crate::SaveTarget; + + /// An `n`-page PDF with a proper xref table. + fn fixture(n: usize) -> Vec { + let mut objs: Vec = vec!["<< /Type /Catalog /Pages 2 0 R >>".into()]; + let kids: Vec = (0..n).map(|i| format!("{} 0 R", 3 + i)).collect(); + objs.push(format!("<< /Type /Pages /Kids [{}] /Count {n} /MediaBox [0 0 200 300] >>", kids.join(" "))); + for _ in 0..n { + objs.push("<< /Type /Page /Parent 2 0 R >>".into()); + } + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + out + } + + /// A fresh temporary folder with `thesis.pdf` in it, removed when dropped. + struct TempDir(PathBuf); + + impl TempDir { + fn new(test: &str, bytes: &[u8]) -> (Self, String) { + let dir = std::env::temp_dir().join(format!("pdfcraft-watch-{test}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("thesis.pdf").to_string_lossy().into_owned(); + std::fs::write(&path, bytes).unwrap(); + (Self(dir), path) + } + } + + impl Drop for TempDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + /// Rewrite `path` as a build does, with a later modification time. + fn rebuild(path: &str, bytes: &[u8]) { + std::fs::write(path, bytes).unwrap(); + let file = std::fs::File::options().write(true).open(path).unwrap(); + file.set_modified(SystemTime::now() + Duration::from_secs(60)).unwrap(); + } + + fn harness(path: &str) -> Harness<'static, PdfCraftApp> { + let path = path.to_owned(); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.run_inline = true; + app.open_path(&path); + app + }); + h.run_steps(4); + h + } + + /// Version B of the open file, found by the watcher but not yet applied: queued as the worker + /// thread queues what it finds between frames. + fn find_new_version(app: &mut PdfCraftApp, path: &str, bytes: &[u8]) -> Vec { + rebuild(path, bytes); + // One look sees the change, the next sees it settled and reads it. + let (mut found, _) = app.watch.inline.tick(); + found.extend(app.watch.inline.tick().0); + assert_eq!(found.len(), 1, "the watcher found the new version"); + found + } + + fn doc<'a>(h: &'a Harness<'static, PdfCraftApp>) -> &'a pdfcraft_engine::Document { + let app = h.state(); + app.session.get(app.views[0].id).unwrap() + } + + /// #1802: the watcher found version B of the file, then the user saved version C over it + /// before B was applied. B is older than the document now: it is not applied, the undo history + /// stays, and C stays on disk. + #[test] + fn a_version_found_before_a_save_is_not_applied_over_it() { + let (_dir, path) = TempDir::new("stale-save", &fixture(2)); + let mut h = harness(&path); + let stale = find_new_version(h.state_mut(), &path, &fixture(3)); + let app = h.state_mut(); + app.views[0].select_pages(&[0]); + app.apply_edit(pdfcraft_engine::Edit::RotatePages { pages: vec![0], degrees: 90 }); + assert!(app.save_active(SaveTarget::InPlace), "saved version C"); + app.watch.found.lock().unwrap().extend(stale); + h.run_steps(3); + let d = doc(&h); + assert_eq!(d.info.pages.len(), 2, "the stale version was not applied over the saved one"); + assert!(d.can_undo().is_some() && !d.dirty, "the saved document and its history stay"); + assert_eq!(Stamp::at(&path), h.state().watch.known.get(&h.state().views[0].id).copied(), "C is still on disk"); + } + + /// #1802: version B found under the old path, then Save As to another file: B is not applied + /// to the document under its new path. + #[test] + fn a_version_found_before_save_as_is_not_applied_to_the_new_file() { + let (dir, path) = TempDir::new("stale-save-as", &fixture(2)); + let mut h = harness(&path); + let stale = find_new_version(h.state_mut(), &path, &fixture(3)); + let new_path = dir.0.join("copy.pdf").to_string_lossy().into_owned(); + let app = h.state_mut(); + app.views[0].select_pages(&[0]); + app.apply_edit(pdfcraft_engine::Edit::RotatePages { pages: vec![0], degrees: 90 }); + app.save_override = Some(new_path.clone()); + assert!(app.save_active(SaveTarget::As), "saved as the new file"); + app.watch.found.lock().unwrap().extend(stale); + h.run_steps(3); + let d = doc(&h); + assert_eq!(d.path.as_deref(), Some(new_path.as_str())); + assert_eq!(d.info.pages.len(), 2, "the old file's version was not applied to the new one"); + assert!(d.can_undo().is_some() && !d.dirty, "the saved document and its history stay"); + } +}