From bd0b3fdb65d2c57a22c003d834720ecd6e96eee6 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo <103679419+marcoarnulfo@users.noreply.github.com> Date: Sun, 18 Oct 2026 09:48:35 +0200 Subject: [PATCH] Save As to another file writes a clean copy without earlier revisions (fixes #2165) (#2174) --- crates/ui-egui/src/editing.rs | 29 ++++++++++++++++++++--------- crates/ui-egui/tests/editing.rs | 21 +++++++++++++++++++++ crates/ui-egui/tests/signing.rs | 32 ++++++++++++++++++++++++++++++++ docs/architecture.md | 3 ++- 4 files changed, 75 insertions(+), 10 deletions(-) diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index 8b63c49fe9..0e5307f0e9 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -24,6 +24,7 @@ pub enum CloseRequest { pub struct PendingOverwrite { pub id: pdfcraft_engine::DocId, pub dest: String, + target: SaveTarget, /// What was to run once the document is written (close its tab, quit). after: Box, } @@ -621,12 +622,12 @@ impl PdfCraftApp { let dialog = rfd::AsyncFileDialog::new().add_filter("PDF", &["pdf"]).set_file_name(name); // The bytes are taken once the user has chosen, so edits made meanwhile are saved too. self.ask_one(crate::pickers::Ask::Save(dialog), None, move |app, dest| { - app.save_doc_to_then(id, &dest.to_string_lossy(), Box::new(after)); + app.save_doc_to_then(id, &dest.to_string_lossy(), target, Box::new(after)); }); return false; } }; - self.save_doc_to_then(id, &destination, Box::new(after)) + self.save_doc_to_then(id, &destination, target, Box::new(after)) } #[cfg(target_arch = "wasm32")] { @@ -681,14 +682,14 @@ impl PdfCraftApp { /// `pending_overwrite` behind [`Dialog::OverwriteChanged`](crate::Dialog::OverwriteChanged). /// Returns `true` if written now. #[cfg(not(target_arch = "wasm32"))] - fn save_doc_to_then(&mut self, id: pdfcraft_engine::DocId, dest: &str, after: Box) -> bool { + fn save_doc_to_then(&mut self, id: pdfcraft_engine::DocId, dest: &str, target: SaveTarget, after: Box) -> bool { let own = self.session.get(id).and_then(|d| d.path.as_deref()).is_some_and(|p| crate::same_file(p, dest)); if own && self.watch.disk_differs(id, dest) { - self.pending_overwrite = Some(PendingOverwrite { id, dest: dest.to_string(), after }); + self.pending_overwrite = Some(PendingOverwrite { id, dest: dest.to_string(), target, after }); self.dialog = Some(crate::Dialog::OverwriteChanged); return false; } - let saved = self.save_doc_to(id, dest); + let saved = self.save_doc_to(id, dest, target); if saved { after(self); } @@ -699,7 +700,7 @@ impl PdfCraftApp { #[cfg(not(target_arch = "wasm32"))] pub(crate) fn overwrite_changed_file(&mut self) { let Some(p) = self.pending_overwrite.take() else { return }; - if self.save_doc_to(p.id, &p.dest) { + if self.save_doc_to(p.id, &p.dest, p.target) { (p.after)(self); } } @@ -715,17 +716,27 @@ impl PdfCraftApp { /// Write document `id` to `dest` and make it the document's file. Returns `true` if written. #[cfg(not(target_arch = "wasm32"))] - fn save_doc_to(&mut self, id: pdfcraft_engine::DocId, dest: &str) -> bool { + fn save_doc_to(&mut self, id: pdfcraft_engine::DocId, dest: &str, target: SaveTarget) -> bool { // Save As answers on a later frame: commit what was typed meanwhile, too (#166), even // if another tab is active by then. if !self.commit_typing_in(id) || !self.flatten_fill_sign_for_save(id) { return false; } - let Some(name) = self.session.get(id).map(|d| d.name.clone()) else { + let Some((name, full)) = self.session.get(id).map(|d| { + // Save As to another file is a clean copy, without the earlier revisions that still + // hold deleted pages or replaced text (#2165). A signed document stays incremental so + // its signatures keep covering the file. + let full = target == SaveTarget::As + && !d.path.as_deref().is_some_and(|p| crate::same_file(p, dest)) + && d.read_only_reason.is_none() + && !d.is_signed(); + (d.name.clone(), full) + }) else { self.notify_tr("The document was closed before it could be saved."); return false; }; - let bytes = match self.session.save_bytes(id).and_then(|b| self.session.check_save(id, &b).map(|()| b)) { + let bytes = if full { self.session.save_full_bytes(id) } else { self.session.save_bytes(id) }; + let bytes = match bytes.and_then(|b| self.session.check_save(id, &b).map(|()| b)) { Ok(b) => b, Err(e) => { self.notify_fmt("Couldn't save {name}: {e}", &[("name", &name), ("e", &e.to_string())]); diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index 6957d47f06..05dbc7ca2a 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -520,6 +520,27 @@ fn save_writes_an_incremental_update_and_clears_dirty() { let _ = std::fs::remove_file(out); } +#[test] +fn save_as_to_a_new_file_drops_earlier_revisions() { + // #2165: Save As appended a revision, so a deleted page stayed in the new file's first one. + let out = temp_path("save-as-clean.pdf"); + let mut h = organize(3); + h.get_by_label("Page 2").hover(); + h.run_steps(2); + h.get_by_label("Delete page 2").click(); + h.run_steps(3); + h.state_mut().save_override = Some(out.to_string_lossy().into_owned()); + assert!(h.state_mut().save_active(pdfcraft_ui_egui::SaveTarget::As)); + let saved = std::fs::read(&out).expect("file written"); + let count = |needle: &[u8]| saved.windows(needle.len()).filter(|w| *w == needle).count(); + assert_eq!(count(b"%%EOF"), 1, "one revision only"); + assert_eq!((count(b"(Page 1)"), count(b"(Page 2)")), (1, 0), "the deleted page's content is gone"); + assert!(!dirty(&h)); + assert_eq!(h.state().session.get(h.state().views[0].id).unwrap().bytes.as_slice(), saved.as_slice()); + assert_eq!(page_texts(h.state()), ["Page 1", "Page 3"]); + let _ = std::fs::remove_file(out); +} + #[test] fn closing_a_dirty_tab_asks_and_cancel_keeps_it() { let mut h = organize(2); diff --git a/crates/ui-egui/tests/signing.rs b/crates/ui-egui/tests/signing.rs index ccb6dece7b..c7fd4c42fa 100644 --- a/crates/ui-egui/tests/signing.rs +++ b/crates/ui-egui/tests/signing.rs @@ -285,6 +285,38 @@ fn certifying_without_a_visible_signature() { h.get_by_label_contains("Certified by Test Signer EC"); } +#[test] +fn save_as_keeps_a_signed_document_incremental() { + // #2165: Save As to a new file rewrites unsigned documents in full; a signed one is appended + // to, or its signature would no longer cover the file. + let p12 = concat!(env!("CARGO_MANIFEST_DIR"), "/../sign/tests/data/ec-p256.p12"); + let digital_id = pdfcraft_engine::sign::pkcs12::open(&std::fs::read(p12).unwrap(), "test").unwrap(); + let mut s = pdfcraft_engine::Session::new(); + let id = s.open("contract.pdf", None, std::sync::Arc::new(FIXTURE.to_vec()), None).unwrap(); + let opts = pdfcraft_engine::SignOptions { page: 0, rect: Some([20.0, 20.0, 180.0, 60.0]), ..Default::default() }; + let signed = s.sign(id, &digital_id, opts).unwrap(); + let out = dir().join("copy.pdf"); + let (bytes, cert) = (signed.to_vec(), digital_id.certificate.clone()); + 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.open_bytes("contract.pdf", None, bytes).expect("opens"); + app.session.set_trusted_certificates(vec![cert]); + app + }); + h.run_steps(4); + let status = |h: &Harness<'static, PdfCraftApp>| { + let s = h.state(); + s.session.get(s.views[0].id).unwrap().signatures.iter().find(|x| x.signed).map(|x| x.status) + }; + assert_eq!(status(&h), Some(SignatureStatus::Valid)); + h.state_mut().save_override = Some(out.to_string_lossy().into_owned()); + assert!(h.state_mut().save_active(pdfcraft_ui_egui::SaveTarget::As), "a signed document can be saved as a copy"); + let saved = std::fs::read(&out).expect("file written"); + assert!(saved.starts_with(&signed), "the signed revision is kept as it was"); + assert_eq!(status(&h), Some(SignatureStatus::Valid)); +} + /// The fixture with an empty signature field across the bottom of the page. const WITH_FIELD: &[u8] = b"%PDF-1.7 1 0 obj << /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [6 0 R] >> >> endobj diff --git a/docs/architecture.md b/docs/architecture.md index aa91c36c4e..6b44817e15 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -49,7 +49,8 @@ The PDF object graph *is* the model (`crates/engine/README.md` §The editing mod 3. After every edit an incremental write produces the working file (original bytes plus one revision); the view is refreshed from those bytes, so what you see is what Save writes. 4. Saves are incremental and atomic. A full rewrite happens only where it must: redaction, PDF/A - conversion, print imposition, a changed password, Save As with garbage collection. + conversion, print imposition, a changed password, Save As to another file (garbage-collected, + unless the document is signed). 5. Autosave and crash recovery keep encrypted documents encrypted. Signed documents are protected from rewrites.