diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ab87094886..b3ac430ab4 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1193,11 +1193,15 @@ impl PdfCraftApp { open_worker::ResultKind::Failed(reason) => { self.notify_fmt("Couldn't open {name}: {reason}", &[("name", &pending.name), ("reason", &reason)]); } - open_worker::ResultKind::Conversion(bytes) => { - if let Some(Err(error)) = self.open_converted(&pending.name, &bytes) { - self.notify_fmt("Couldn't open {name}: {e}", &[("name", &pending.name), ("e", &error)]); + open_worker::ResultKind::Conversion(bytes) => match self.open_converted(&pending.name, &bytes) { + Some(Ok(())) => { + if let Some(command) = pending.then { + self.execute(command); + } } - } + Some(Err(error)) => self.notify_fmt("Couldn't open {name}: {e}", &[("name", &pending.name), ("e", &error)]), + None => {} + }, open_worker::ResultKind::Prepared { bytes, result } => match *result { Ok(prepared) => match self.finish_open(&pending.name, pending.path.clone(), bytes.len(), prepared, pending.activate) { Ok(id) => { @@ -1727,6 +1731,13 @@ impl PdfCraftApp { #[cfg(not(target_arch = "wasm32"))] pub fn open_path(&mut self, path: &str) { + self.open_path_attempt(path); + } + + /// Whether this attempt opened a document, created a password prompt, or queued an open. + /// Failure can leave an older password prompt intact; it does not belong to this attempt. + #[cfg(not(target_arch = "wasm32"))] + fn open_path_attempt(&mut self, path: &str) -> bool { let name = std::path::Path::new(path).file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| path.to_string()); // Taken before reading: a build that rewrites the file meanwhile still counts as a change. let stamp = file_watch::Stamp::at(path); @@ -1734,20 +1745,26 @@ impl PdfCraftApp { // deterministic inline startup and test flows. Once live, reading and preparation use // the bounded worker and commit on the UI thread. if self.ctx.is_none() || self.run_inline { - match std::fs::read(path) { + return match std::fs::read(path) { Ok(bytes) => { let before = self.views.len(); if let Err(error) = self.open_bytes(&name, Some(path.to_string()), bytes) { self.notify_fmt("Couldn't open {name}: {e}", &[("name", &name), ("e", &error)]); - } else if self.views.len() > before - && let Some(doc) = self.views.last().map(|view| view.id) - { - self.watch.known_as(doc, stamp); + false + } else { + if self.views.len() > before + && let Some(doc) = self.views.last().map(|view| view.id) + { + self.watch.known_as(doc, stamp); + } + true } } - Err(error) => self.notify_fmt("Couldn't read {name}: {e}", &[("name", &name), ("e", &error.to_string())]), - } - return; + Err(error) => { + self.notify_fmt("Couldn't read {name}: {e}", &[("name", &name), ("e", &error.to_string())]); + false + } + }; } self.startup_superseded = true; if let Err(error) = self.schedule_open(OpenRequest { @@ -1762,6 +1779,9 @@ impl PdfCraftApp { then: None, }) { self.notify_fmt("Couldn't open {name}: {e}", &[("name", &name), ("e", &error)]); + false + } else { + true } } @@ -2064,12 +2084,21 @@ impl PdfCraftApp { self.ask_one(pickers::Ask::File(dialog), None, move |app, path| { let path = path.to_string_lossy().into_owned(); let before = app.views.len(); - app.open_path(&path); - if let Some(prompt) = app.password_prompt.as_mut().filter(|p| p.path.as_deref() == Some(path.as_str())) { - // Encrypted: the tool runs once the password is in. - prompt.then = Some(command); + let pending_before = app.pending_opens.len(); + if !app.open_path_attempt(&path) { + return; + } + if app.ctx.is_some() && !app.run_inline { + // UI-thread completion cannot run inside open_path: only its newly queued + // request owns this continuation, including through password retries. + if let Some(pending) = app.pending_opens.get_mut(pending_before) { + pending.then = Some(command); + } } else if app.views.len() > before && app.active.is_some() { app.execute(command); + } else if let Some(prompt) = app.password_prompt.as_mut().filter(|p| p.path.as_deref() == Some(path.as_str())) { + // Encrypted: the tool runs once the password is in. + prompt.then = Some(command); } }); } diff --git a/crates/ui-egui/src/open_worker_tests.rs b/crates/ui-egui/src/open_worker_tests.rs index 0e5775feb1..f71c9fbebe 100644 --- a/crates/ui-egui/src/open_worker_tests.rs +++ b/crates/ui-egui/src/open_worker_tests.rs @@ -89,6 +89,191 @@ fn open_prepare_keeps_control_and_existing_tabs_live_until_atomic_commit() { let _ = std::fs::remove_file(path); } +#[test] +fn inline_failed_tool_open_does_not_steal_an_existing_password_prompt() { + let mut doc = pdfcraft_cos::Document::open(Arc::new(fixture(2))).unwrap(); + doc.set_encryption(&pdfcraft_cos::NewEncryption { + algorithm: pdfcraft_cos::Algorithm::Aes256, + user_password: "reader", + owner_password: "owner", + permissions: -4, + encrypt_metadata: true, + seed: [4; 32], + }) + .unwrap(); + let bytes = pdfcraft_cos::write_full(&doc, &Default::default()).unwrap(); + let nonce = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); + let path = std::env::temp_dir().join(format!("pdfcraft-inline-owner-{}-{nonce}.pdf", std::process::id())); + assert!(!path.exists(), "the new opening attempt must fail reading"); + let mut h = Harness::builder().with_size(egui::vec2(1000.0, 700.0)).build_eframe(|_| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.run_inline = true; + app.open_bytes("locked.pdf", Some(path.to_string_lossy().into_owned()), bytes).unwrap(); + app.pick_override = Some(vec![path.to_string_lossy().into_owned()]); + app + }); + h.step(); + let cached = h.state().password_prompt.as_ref().unwrap().bytes.clone(); + assert!(h.state().password_prompt.as_ref().unwrap().then.is_none()); + h.state_mut().run_command("export.text"); + h.step(); + assert!(h.state().toast.as_ref().is_some_and(|toast| toast.0.starts_with("Couldn't read"))); + let prompt = h.state().password_prompt.as_ref().unwrap(); + assert!(Arc::ptr_eq(&prompt.bytes, &cached), "the older encrypted bytes remain available"); + assert!(prompt.then.is_none(), "a failed attempt must not attach its tool to the older prompt"); + h.state_mut().submit_password(Some("wrong".into())); + assert!(h.state().password_prompt.as_ref().unwrap().then.is_none()); + h.state_mut().submit_password(Some("reader".into())); + assert_eq!(h.state().views.len(), 1, "the older document can still be unlocked"); + assert!(h.state().password_prompt.is_none()); + h.run_steps(2); + assert!(h.state().dialog.is_none(), "the failed attempt's tool never runs"); + + // Accepting the current attempt must still run ordinary/conversion tools and retain + // newly created password continuations through retries or discard them on cancellation. + for case in ["plain", "converted", "password", "password-cancel"] { + let path = path.with_extension(if case == "converted" { "txt" } else { "pdf" }); + let bytes = match case { + "plain" => fixture(2), + "converted" => b"Inline text converted to PDF".to_vec(), + _ => cached.as_ref().clone(), + }; + std::fs::write(&path, bytes).unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(1000.0, 700.0)).build_eframe(|_| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.run_inline = true; + app.pick_override = Some(vec![path.to_string_lossy().into_owned()]); + app + }); + h.step(); + let defaults = h.state().persist(); + h.state_mut().run_command("export.text"); + h.step(); + if case.starts_with("password") { + assert_eq!(h.state().password_prompt.as_ref().unwrap().then, Some("export.text")); + h.state_mut().submit_password(Some("wrong".into())); + assert_eq!(h.state().password_prompt.as_ref().unwrap().then, Some("export.text")); + h.state_mut().submit_password((case == "password").then(|| "reader".into())); + } + assert_eq!(h.state().views.len(), usize::from(case != "password-cancel")); + assert_eq!(matches!(h.state().dialog, Some(crate::Dialog::Export(_))), case != "password-cancel"); + assert!(h.state().password_prompt.is_none()); + assert_eq!(h.state().view_defaults.layout, PdfCraftApp::new().view_defaults.layout); + assert_eq!(h.state().view_defaults.continuous, PdfCraftApp::new().view_defaults.continuous); + // Persistence includes recent files; compare only the saved display defaults. + let before: Value = serde_json::from_str(&defaults).unwrap(); + let after: Value = serde_json::from_str(&h.state().persist()).unwrap(); + assert_eq!(before["default_layout"], after["default_layout"]); + let _ = std::fs::remove_file(path); + } +} + +#[test] +fn tool_open_continuation_follows_its_worker_and_password_retries() { + for case in ["plain", "converted", "invalid-image", "password", "password-cancel", "wrong-password-cancel", "read-failure", "invalid-pdf"] { + let extension = match case { + "converted" => "txt", + "invalid-image" => "png", + _ => "pdf", + }; + let path = std::env::temp_dir().join(format!("pdfcraft-tool-worker-{}-{case}.{extension}", std::process::id())); + let encrypted = case.contains("password"); + if case != "read-failure" { + let bytes = if encrypted { + let mut doc = pdfcraft_cos::Document::open(Arc::new(fixture(2))).unwrap(); + doc.set_encryption(&pdfcraft_cos::NewEncryption { + algorithm: pdfcraft_cos::Algorithm::Aes256, + user_password: "reader", + owner_password: "owner", + permissions: -4, + encrypt_metadata: true, + seed: [4; 32], + }) + .unwrap(); + pdfcraft_cos::write_full(&doc, &Default::default()).unwrap() + } else if case == "converted" { + b"A text file opened as a PDF".to_vec() + } else if case == "invalid-image" { + b"\x89PNG\r\n\x1a\n".to_vec() + } else if case == "invalid-pdf" { + b"not a PDF".to_vec() + } else { + fixture(2) + }; + std::fs::write(&path, bytes).unwrap(); + } + let mut h = Harness::builder().with_size(egui::vec2(1000.0, 700.0)).build_eframe(|_| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.pick_override = Some(vec![path.to_string_lossy().into_owned()]); + app + }); + h.step(); + let (gate, started, release, finished) = open_worker::prepare_gate(); + h.state_mut().open_prepare_gate = Some(gate); + h.state_mut().run_command("export.text"); + h.step(); + started.recv_timeout(Duration::from_secs(5)).unwrap(); + assert_eq!(h.state().pending_opens.len(), 1); + assert_eq!(h.state().pending_opens[0].then, Some("export.text"), "{case}: command belongs to the pending request"); + + // A different document becoming active must neither run nor inherit the tool. + h.state_mut().open_bytes("unrelated.pdf", None, fixture(1)).unwrap(); + let unrelated = h.state().views[0].id; + h.step(); + assert!(h.state().dialog.is_none()); + release.send(()).unwrap(); + finished.recv_timeout(Duration::from_secs(5)).unwrap(); + h.step(); + + if encrypted { + assert_eq!(h.state().password_prompt.as_ref().unwrap().then, Some("export.text")); + assert!(h.state().dialog.is_none()); + if case != "password-cancel" { + h.state_mut().submit_password(Some("wrong".into())); + started.recv_timeout(Duration::from_secs(5)).unwrap(); + release.send(()).unwrap(); + finished.recv_timeout(Duration::from_secs(5)).unwrap(); + h.step(); + let prompt = h.state().password_prompt.as_ref().unwrap(); + assert!(prompt.error.is_some()); + assert_eq!(prompt.then, Some("export.text")); + assert!(h.state().dialog.is_none()); + } + if case == "password" { + h.state_mut().submit_password(Some("reader".into())); + started.recv_timeout(Duration::from_secs(5)).unwrap(); + release.send(()).unwrap(); + finished.recv_timeout(Duration::from_secs(5)).unwrap(); + h.step(); + } else { + h.state_mut().submit_password(None); + } + } + if matches!(case, "plain" | "converted" | "password") { + assert_eq!(h.state().views.len(), 2); + let app = h.state(); + let opened = &app.views[app.active.unwrap()]; + assert_ne!(opened.id, unrelated); + assert_eq!(app.session.get(opened.id).unwrap().path.as_deref(), if case == "converted" { None } else { path.to_str() }); + assert!(matches!(app.dialog, Some(crate::Dialog::Export(_)))); + // Closing the dialog and processing more frames must not execute it again. + h.state_mut().dialog = None; + } else { + assert_eq!(h.state().views.len(), 1); + assert!(h.state().dialog.is_none()); + } + assert!(h.state().pending_opens.is_empty()); + assert!(h.state().password_prompt.is_none()); + h.state_mut().open_bytes("later.pdf", None, fixture(1)).unwrap(); + h.run_steps(2); + assert!(h.state().dialog.is_none(), "{case}: continuation cannot run on another open"); + let _ = std::fs::remove_file(path); + } +} + #[test] fn launch_layout_waits_for_cli_opens_and_preserves_restored_tabs() { use crate::canvas::PageLayout; diff --git a/crates/ui-egui/tests/commands.rs b/crates/ui-egui/tests/commands.rs index 47bbc21a8f..734f35099b 100644 --- a/crates/ui-egui/tests/commands.rs +++ b/crates/ui-egui/tests/commands.rs @@ -44,6 +44,15 @@ fn harness() -> Harness<'static, PdfCraftApp> { h } +fn wait_for(h: &mut Harness<'static, PdfCraftApp>, description: &str, ready: impl Fn(&PdfCraftApp) -> bool) { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while !ready(h.state()) { + assert!(std::time::Instant::now() < deadline, "timed out waiting for {description}"); + h.step(); + std::thread::yield_now(); + } +} + /// Commands that open a native file picker can't run headless. /// (Create ▸ Clipboard reads the system clipboard: tests use `create_from_clip`.) const PICKERS: &[&str] = &[ @@ -184,7 +193,7 @@ fn a_tool_chosen_with_no_document_asks_for_one_and_runs_on_it() { h.run_steps(2); h.state_mut().run_command("export.text"); assert!(h.state().toast.as_ref().is_none_or(|t| t.0 != "Open a document first")); - h.run_steps(3); + wait_for(&mut h, "picked document and export dialog", |app| app.active.is_some() && matches!(app.dialog, Some(Dialog::Export(_)))); assert!(h.state().active.is_some(), "the picked file is open"); assert!(matches!(h.state().dialog, Some(Dialog::Export(_))), "and the tool runs on it: {:?}", h.state().dialog); // Cancelled: nothing opens and nothing runs. @@ -287,7 +296,11 @@ fn the_open_recent_menu_lists_files_and_opens_one() { h.get_by_label("Open Recent ⏵").hover(); h.run_steps(3); h.get_by_label_contains("recent.pdf").click(); - h.run_steps(4); + wait_for(&mut h, "recent document", |app| { + app.views.len() == 2 + && app.active.and_then(|index| app.views.get(index)).and_then(|view| app.session.get(view.id)).and_then(|doc| doc.path.as_deref()) + == Some(path.as_str()) + }); let app = h.state(); assert_eq!(app.views.len(), 2, "the recent file opened in a new tab"); let active = app.active.unwrap();