diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index ad18f7112..88856305b 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -932,11 +932,24 @@ impl Automation { // A save that adds or removes encryption, or must not keep the old revision (Sanitize, // redaction), is rewritten in full even when an incremental save was asked for. let incremental = !full && bytes.starts_with(&self.doc(a)?.saved_bytes()); + self.publish_save(id, &target, bytes, incremental, write) + } + + /// Validate the prepared bytes before publishing them or marking the document saved. + fn publish_save( + &mut self, + id: DocId, + target: &Path, + bytes: Arc>, + incremental: bool, + write: impl FnOnce(&Path, &[u8]) -> Result<()>, + ) -> Result { self.session.check_save(id, &bytes).map_err(failed)?; - write(&target, &bytes)?; + write(target, &bytes)?; let path = target.to_string_lossy().into_owned(); self.session.mark_saved(id, bytes.clone(), Some(path.clone())).map_err(failed)?; - Ok(json!({ "path": path, "bytes": bytes.len(), "incremental": incremental, "document": summary(self.doc(a)?) })) + let doc = self.session.get(id).ok_or_else(|| failed("the saved document is no longer open"))?; + Ok(json!({ "path": path, "bytes": bytes.len(), "incremental": incremental, "document": summary(doc) })) } fn doc_revisions(&mut self, a: &Args) -> Result { @@ -2958,6 +2971,37 @@ mod tests { } } + #[test] + fn invalid_save_bytes_never_reach_publication_or_mark_saved() { + let dir = staging_dir("invalid-save-bytes"); + let target = dir.join("out.pdf"); + let mut a = Automation::new().with_root(&dir).unwrap(); + let id = search_document(&mut a, 2); + json(&mut a, "doc_save", json!({"doc": id.0, "path": "out.pdf"})); + let old = std::fs::read(&target).unwrap(); + json(&mut a, "page_rotate", json!({"doc": id.0, "pages": [1], "degrees": 90})); + let before = summary(a.session.get(id).unwrap()); + // Exercise both parse failure and a valid PDF that loses a page. This does not + // depend on a malformed input remaining unrepaired by future reader improvements. + for bytes in [b"not a PDF".to_vec(), numbered_pages(1)] { + let mut published = false; + let result = a.publish_save(id, &target, Arc::new(bytes), false, |_, _| { + published = true; + Ok(()) + }); + assert!(result.is_err(), "invalid output must be refused"); + assert!(result.unwrap_err().to_string().contains("could not be reopened")); + assert!(!published, "validation must precede the writer"); + assert_eq!(summary(a.session.get(id).unwrap()), before, "failed validation must preserve document state"); + assert_eq!(std::fs::read(&target).unwrap(), old); + assert_eq!(listing(&dir), ["out.pdf"]); + } + json(&mut a, "doc_save", json!({"doc": id.0})); + assert!(!a.session.get(id).unwrap().dirty); + assert_ne!(std::fs::read(&target).unwrap(), old); + std::fs::remove_dir_all(dir).unwrap(); + } + #[test] fn a_failed_save_sync_keeps_the_document_dirty_and_can_be_retried() { let dir = staging_dir("sync-dirty"); diff --git a/crates/automation/tests/automation.rs b/crates/automation/tests/automation.rs index ff1a759d6..1e662d8f3 100644 --- a/crates/automation/tests/automation.rs +++ b/crates/automation/tests/automation.rs @@ -711,11 +711,10 @@ fn an_in_place_save_reports_a_required_rewrite_as_not_incremental() { } } -/// Issue #1698: a save whose bytes don't reopen is refused before it touches the file. The -/// catalog's `/Pages` is not a page tree, so the reader finds the page by scanning, while a -/// full rewrite keeps only what the catalog reaches and drops it. +/// #1664 repairs the missing page tree on open. The former #1698 failure fixture now +/// saves successfully, keeping its page and pixels through a full rewrite. #[test] -fn a_save_that_does_not_reopen_leaves_the_file_as_it_was() { +fn a_recovered_page_tree_survives_a_full_save() { let dir = workdir("save-verify-before-write"); let objs: [&[u8]; 4] = [ b"<< /Type /Catalog /Pages 3 0 R >>", @@ -743,10 +742,19 @@ fn a_save_that_does_not_reopen_leaves_the_file_as_it_was() { let opened = ok(&mut a, "doc_open", json!({ "path": "damaged.pdf" })); assert_eq!(opened["pages"], 1, "{opened}"); let doc = opened["doc"].as_u64().unwrap(); - let e = err(&mut a, "doc_save", json!({ "doc": doc, "full": true })); - assert!(e.to_string().contains("could not be reopened"), "{e}"); + let (width, height, pixels) = a.render_pixels(doc, 1, 1.0).unwrap(); + assert!(pixels.as_ref().as_chunks::<4>().0.iter().any(|p| p[..3] != [255, 255, 255]), "the original page is not blank"); + let saved = ok(&mut a, "doc_save", json!({ "doc": doc, "full": true })); + assert_eq!(saved["document"]["dirty"], false); let on_disk = std::fs::read(dir.join("damaged.pdf")).unwrap(); - assert!(on_disk == original, "the file was replaced by {} bytes that don't reopen", on_disk.len()); + assert_ne!(on_disk, original, "a full save writes the repaired page tree"); + let mut reopened = auto(&dir); + let opened = ok(&mut reopened, "doc_open", json!({ "path": "damaged.pdf" })); + assert_eq!(opened["pages"], 1); + let again = opened["doc"].as_u64().unwrap(); + let (new_width, new_height, new_pixels) = reopened.render_pixels(again, 1, 1.0).unwrap(); + assert_eq!((new_width, new_height), (width, height)); + assert_eq!(new_pixels.as_ref(), pixels.as_ref(), "saving must preserve the recovered page's drawing"); } /// organize.field-name-conflicts: combining files whose forms share a field name keeps the diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 8bc3a5c7a..5941f6a4a 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -5107,6 +5107,7 @@ trailer << /Root 1 0 R >> fn explicit_search_jumps_record_history_and_keep_match_destination() { for step in [None, Some(true), Some(false)] { let mut v = history_search_view(); + v.page_input = "draft in progress".into(); v.goto_point = Some((3, Some(0.8), Some(0.9), Vec2::ZERO)); if let Some(forward) = step { v.find_step(forward); @@ -5115,7 +5116,11 @@ trailer << /Root 1 0 R >> } assert_eq!(v.back, vec![0], "explicit search jump must remember its origin"); assert_eq!(v.goto, Some((3, 0.5)), "keep the match offset instead of the page top"); - assert_eq!(v.page_input, "4"); + assert_eq!(v.current, 3); + assert_eq!(v.current_page_label(None), "4"); + // Chrome refreshes the idle page box; navigation must not overwrite a draft + // that may still be focused. DocView does not own the editor's focus state. + assert_eq!(v.page_input, "draft in progress"); assert!(v.goto_point.is_none(), "an earlier link destination cannot override the search offset"); v.go_to_match(1); assert_eq!(v.back, vec![0], "same-page matches do not duplicate history");