diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 18e1c8d7..abf11ea1 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1142,7 +1142,11 @@ pub fn show(app: &mut PdfCraftApp, ctx: &egui::Context) { app.discard_recovered(&keys); } } - if replace_now && let Some(d) = app.replace_draft.take() { + // The pages to replace belong to the document the dialog was opened on (#167). + if replace_now + && let Some(d) = app.replace_draft.take() + && app.still_pick_target(d.target) + { let n = d.to - d.from + 1; app.apply_edit(Edit::ReplacePages { pages: (d.from - 1..d.to).collect(), @@ -1151,8 +1155,9 @@ pub fn show(app: &mut PdfCraftApp, ctx: &egui::Context) { src_pages: (d.src_from - 1..d.src_from - 1 + n).collect(), }); } - if print_go { - app.print_now(); + // A print or save that fails keeps the dialog open, with the reason in a notice. + if print_go && !app.print_now() { + close = false; } if revert_now { app.revert_active(); diff --git a/crates/ui-egui/src/files.rs b/crates/ui-egui/src/files.rs index 0ca18b95..f7cc0714 100644 --- a/crates/ui-egui/src/files.rs +++ b/crates/ui-egui/src/files.rs @@ -5,7 +5,7 @@ use std::sync::Arc; -use pdfcraft_engine::{Edit, SplitBy}; +use pdfcraft_engine::{DocId, Edit, SplitBy}; use crate::PdfCraftApp; @@ -28,10 +28,39 @@ pub struct ReplaceDraft { pub from: usize, pub to: usize, pub src_from: usize, + /// The document whose pages are replaced, as it was when the dialog opened: OK replaces + /// nothing if another document is active by then (a file opened meanwhile), or it changed. + pub(crate) target: Option, } -/// Files picked asynchronously (web), waiting to be used: (purpose, [(name, bytes)]). -pub type Requests = Arc)>)>>>; +impl FilePurpose { + /// Insert and Replace edit the active document, at its selection. + pub(crate) fn edits_active_document(self) -> bool { + matches!(self, Self::InsertPages | Self::ReplacePages) + } +} + +/// The document a pick edits, as it was when the picker opened. Page positions captured then +/// are only valid while it is unchanged, and the pick acts on the active document; so a pick +/// that arrives after the document was edited, or stopped being the active one, is refused +/// (see [`PdfCraftApp::still_pick_target`]). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct PickTarget { + doc: DocId, + generation: Option, +} + +/// Files picked asynchronously in a browser, waiting for the next frame (see +/// [`PdfCraftApp::file_request`]). +#[derive(Debug)] +pub struct FileRequest { + purpose: FilePurpose, + target: Option, + files: Vec<(String, Vec)>, +} + +/// Files picked asynchronously (web), waiting to be used. +pub type Requests = Arc>>; /// Settings for the Split dialog. #[derive(Clone, Debug, PartialEq)] @@ -121,6 +150,10 @@ impl PdfCraftApp { #[cfg(target_arch = "wasm32")] { let requests = self.requests.clone(); + let ctx = self.ctx.clone(); + // The document to insert into is the one active now, not whichever is active when + // the browser has finished reading the file (#167). + let request = self.file_request(purpose, Vec::new()); wasm_bindgen_futures::spawn_local(async move { let dialog = rfd::AsyncFileDialog::new().add_filter("PDF", &["pdf"]); let handles = if multiple { dialog.pick_files().await.unwrap_or_default() } else { dialog.pick_file().await.into_iter().collect() }; @@ -131,7 +164,11 @@ impl PdfCraftApp { if !files.is_empty() && let Ok(mut q) = requests.lock() { - q.push((purpose, files)); + q.push(FileRequest { files, ..request }); + } + // The read may finish while the app is idle: wake it to use the files. + if let Some(ctx) = ctx { + ctx.request_repaint(); } }); } @@ -156,11 +193,43 @@ impl PdfCraftApp { } } + /// A request to use `files` for `purpose`, bound to the document it will edit: the active + /// one now, for Insert and Replace. Browsers make it when the picker opens and queue it on + /// `requests` once the files are read; it is used on a later frame. + pub fn file_request(&self, purpose: FilePurpose, files: Vec<(String, Vec)>) -> FileRequest { + let target = if purpose.edits_active_document() { self.pick_target(self.active_ids().map(|(_, id)| id)) } else { None }; + FileRequest { purpose, target, files } + } + + /// `doc` as it is now, for a pick that will edit it. + pub(crate) fn pick_target(&self, doc: Option) -> Option { + doc.map(|doc| PickTarget { doc, generation: self.session.get(doc).map(|d| d.edit_generation()) }) + } + + /// Whether a pick for `target` may still be used: the document is still the active one and + /// unedited. Tells the user when not. + pub(crate) fn still_pick_target(&mut self, target: Option) -> bool { + let Some(t) = target else { return true }; + let active = self.active_ids().map(|(_, id)| id); + let generation = self.session.get(t.doc).map(|d| d.edit_generation()); + if active == Some(t.doc) && generation == t.generation { + return true; + } + self.notify_tr("The document changed while you were choosing a file, so nothing was changed."); + false + } + /// Handle files picked asynchronously. pub(crate) fn process_file_requests(&mut self) { let pending: Vec<_> = self.requests.lock().map(|mut q| std::mem::take(&mut *q)).unwrap_or_default(); - for (purpose, files) in pending { - self.use_files(purpose, files); + for FileRequest { purpose, target, files } in pending { + if !self.still_pick_target(target) { + continue; + } + // Last-resort guard (AGENTS.md §4), as for desktop picks and commands. + if let Err(m) = pdfcraft_engine::guard(|| self.use_files(purpose, files)) { + self.notify_fmt("That didn't work: an internal error stopped it ({m}).", &[("m", m.as_str())]); + } } } @@ -203,7 +272,8 @@ impl PdfCraftApp { }; let targets = self.views[i].target_pages(); let (from, to) = (targets.first().map_or(1, |p| p + 1), targets.last().map_or(1, |p| p + 1)); - self.replace_draft = Some(ReplaceDraft { name, bytes, src_pages, from, to, src_from: 1 }); + let target = self.pick_target(self.active_ids().map(|(_, id)| id)); + self.replace_draft = Some(ReplaceDraft { name, bytes, src_pages, from, to, src_from: 1, target }); self.dialog = Some(crate::Dialog::ReplacePages); } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 2fb5df7d..67afcf8a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -84,7 +84,7 @@ use pdfcraft_engine::{DocId, Session}; pub use canvas::DocView; pub use editing::{CloseRequest, SaveTarget}; -pub use files::{ExtractDraft, FilePurpose, RotateDraft, SplitDraft, SplitMode, SplitPlan}; +pub use files::{ExtractDraft, FilePurpose, FileRequest, RotateDraft, SplitDraft, SplitMode, SplitPlan}; pub use recovery::{AUTOSAVE_SECS, RecoveryMeta, RecoveryStore}; use theme::{ThemeKind, ThemePreference}; @@ -882,6 +882,7 @@ impl PdfCraftApp { #[cfg(target_arch = "wasm32")] { let inbox = self.inbox.clone(); + let ctx = self.ctx.clone(); wasm_bindgen_futures::spawn_local(async move { if let Some(h) = rfd::AsyncFileDialog::new() .add_filter("PDF", &["pdf"]) @@ -893,6 +894,10 @@ impl PdfCraftApp { if let Ok(mut q) = inbox.lock() { q.push((h.file_name(), bytes)); } + // The read may finish while the app is idle: wake it to open the file. + if let Some(ctx) = ctx { + ctx.request_repaint(); + } } }); } diff --git a/crates/ui-egui/src/pickers.rs b/crates/ui-egui/src/pickers.rs index decbd744..dcb01208 100644 --- a/crates/ui-egui/src/pickers.rs +++ b/crates/ui-egui/src/pickers.rs @@ -31,7 +31,7 @@ use std::sync::{Arc, Mutex, PoisonError}; use pdfcraft_engine::DocId; use crate::PdfCraftApp; -use crate::files::FilePurpose; +use crate::files::{FilePurpose, PickTarget}; /// What the chosen files are for. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -57,18 +57,9 @@ pub(crate) enum Ask { /// What to do with the chosen paths, on a later frame. type Then = Box) + Send>; -/// The document a pick edits, as it was when the picker opened. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -struct Target { - doc: DocId, - /// Its edit generation then: pages and objects the pick refers to by position are only - /// valid while it is unchanged. - generation: Option, -} - /// A finished pick, waiting for the next frame. struct Picked { - target: Option, + target: Option, /// The chosen paths; empty when the picker was cancelled. paths: Vec, then: Then, @@ -111,7 +102,7 @@ impl Pickers { /// Wait for `pick` on a worker thread and queue its result for the next frame. On macOS /// `pick` must already have shown the picker (rfd starts the panel when the future is /// created, on the main thread). Returns false when the worker couldn't start. - fn spawn(&self, target: Option, ctx: Option, pick: F, then: Then) -> bool + fn spawn(&self, target: Option, ctx: Option, pick: F, then: Then) -> bool where F: Future> + Send + 'static, { @@ -119,7 +110,7 @@ impl Pickers { } /// [`Self::spawn`], returning the worker so tests can wait for it instead of polling. - fn spawn_worker(&self, target: Option, ctx: Option, pick: F, then: Then) -> Option> + fn spawn_worker(&self, target: Option, ctx: Option, pick: F, then: Then) -> Option> where F: Future> + Send + 'static, { @@ -140,7 +131,7 @@ impl Pickers { /// Queue a pick that is already known (tests and automation). Like a real pick, it keeps /// the flag until the frame uses it. - fn deliver(&self, target: Option, paths: Vec, then: Then) { + fn deliver(&self, target: Option, paths: Vec, then: Then) { self.done.lock().unwrap_or_else(PoisonError::into_inner).push(Picked { target, paths, then }); } @@ -155,7 +146,7 @@ impl PdfCraftApp { pub(crate) fn pick(&mut self, pick_for: PickFor, dialog: rfd::AsyncFileDialog, multiple: bool) { // Insert and Replace edit the active document: remember which one. let target = match pick_for { - PickFor::Files(FilePurpose::InsertPages | FilePurpose::ReplacePages) => self.active_ids().map(|(_, id)| id), + PickFor::Files(purpose) if purpose.edits_active_document() => self.active_ids().map(|(_, id)| id), _ => None, }; let ask = if multiple { Ask::Files(dialog) } else { Ask::File(dialog) }; @@ -181,7 +172,7 @@ impl PdfCraftApp { self.notify_tr("Another file dialog is still open. Finish with it first."); return false; } - let target = target.map(|doc| Target { doc, generation: self.session.get(doc).map(|d| d.edit_generation()) }); + let target = self.pick_target(target); let then: Then = Box::new(then); if let Some(paths) = self.pick_override.clone() { self.pickers.deliver(target, paths.into_iter().map(PathBuf::from).collect(), then); @@ -237,13 +228,8 @@ impl PdfCraftApp { if paths.is_empty() { continue; } - if let Some(t) = target { - let active = self.active_ids().map(|(_, id)| id); - let generation = self.session.get(t.doc).map(|d| d.edit_generation()); - if active != Some(t.doc) || generation != t.generation { - self.notify_tr("The document changed while you were choosing a file, so nothing was changed."); - continue; - } + if !self.still_pick_target(target) { + continue; } // Last-resort guard (AGENTS.md §4), as for commands: this work used to run inside // the command that asked, and a panic in it must not take the app down. diff --git a/crates/ui-egui/src/print_ui.rs b/crates/ui-egui/src/print_ui.rs index e7428e5b..b207bace 100644 --- a/crates/ui-egui/src/print_ui.rs +++ b/crates/ui-egui/src/print_ui.rs @@ -180,31 +180,51 @@ impl PdfCraftApp { false } }, + None => self.save_print_pdf(&name, bytes), + } + } + + /// Print ▸ Save as PDF on the desktop: write to `save_override` (tests and automation), or + /// ask where and write on a later frame. Returns `true` once written, or once the save picker + /// is showing. + #[cfg(not(target_arch = "wasm32"))] + fn save_print_pdf(&mut self, name: &str, bytes: Vec) -> bool { + let write = move |app: &mut Self, path: std::path::PathBuf| match crate::editing::write_atomically(&path.to_string_lossy(), &bytes) { + Ok(()) => { + app.notify_fmt("Saved the print-ready PDF to {path}", &[("path", &path.display().to_string())]); + true + } + Err(e) => { + app.notify_fmt("Could not save: {e}", &[("e", &e.to_string())]); + false + } + }; + match self.save_override.clone() { + Some(p) => write(self, p.into()), None => { - let write = move |app: &mut Self, path: std::path::PathBuf| match crate::editing::write_atomically(&path.to_string_lossy(), &bytes) { - Ok(()) => { - app.notify_fmt("Saved the print-ready PDF to {path}", &[("path", &path.display().to_string())]); - true - } - Err(e) => { - app.notify_fmt("Could not save: {e}", &[("e", &e.to_string())]); - false - } - }; - match self.save_override.clone() { - Some(p) => write(self, p.into()), - #[cfg(not(target_arch = "wasm32"))] - None => { - // Saved on a later frame, once the user has chosen where. - let stem = name.trim_end_matches(".pdf").trim_end_matches(".PDF"); - let dialog = rfd::AsyncFileDialog::new().add_filter("PDF", &["pdf"]).set_file_name(format!("{stem} (print).pdf")); - self.ask_one(crate::pickers::Ask::Save(dialog), None, move |app, path| { - write(app, path); - }) - } - #[cfg(target_arch = "wasm32")] - None => false, - } + let stem = name.trim_end_matches(".pdf").trim_end_matches(".PDF"); + let dialog = rfd::AsyncFileDialog::new().add_filter("PDF", &["pdf"]).set_file_name(format!("{stem} (print).pdf")); + self.ask_one(crate::pickers::Ask::Save(dialog), None, move |app, path| { + write(app, path); + }) + } + } + } + + /// Print ▸ Save as PDF in a browser: download it, as Save does (#170). There is no folder to + /// choose and no file system to write to, so `save_override` doesn't apply. + #[cfg(target_arch = "wasm32")] + fn save_print_pdf(&mut self, name: &str, bytes: Vec) -> bool { + let stem = name.trim_end_matches(".pdf").trim_end_matches(".PDF"); + let file = format!("{stem} (print).pdf"); + match crate::editing::download(&file, &bytes) { + Ok(()) => { + self.notify_fmt("Downloaded {name}", &[("name", &file)]); + true + } + Err(e) => { + self.notify_fmt("Couldn't download {name}: {e}", &[("name", &file), ("e", &e)]); + false } } } diff --git a/crates/ui-egui/tests/pickers.rs b/crates/ui-egui/tests/pickers.rs index c9402462..5ce792ef 100644 --- a/crates/ui-egui/tests/pickers.rs +++ b/crates/ui-egui/tests/pickers.rs @@ -342,3 +342,70 @@ fn a_watermark_file_from_browse_lands_only_in_the_dialog_that_asked() { } let _ = std::fs::remove_dir_all(&dir); } + +/// Browsers read picked files asynchronously and queue them on `requests` (#167): the request +/// is made when the picker opens, so it is bound to the document active then. +fn browser_insert(h: &Harness<'static, PdfCraftApp>, pages: usize) -> pdfcraft_ui_egui::FileRequest { + h.state().file_request(pdfcraft_ui_egui::FilePurpose::InsertPages, vec![("b.pdf".into(), fixture(pages))]) +} + +#[test] +fn a_browser_insert_goes_into_the_document_it_was_started_on() { + let mut h = harness(&[3, 1]); + h.state_mut().active = Some(0); + let request = browser_insert(&h, 1); + h.state_mut().requests.lock().unwrap().push(request); + h.run_steps(2); + assert_eq!(pages(h.state(), 0), 4); + assert_eq!(pages(h.state(), 1), 1); +} + +#[test] +fn a_browser_insert_that_arrives_after_switching_documents_changes_neither() { + // Issue #167: insert into A, open C before the file is read: the pages went into C. + let mut h = harness(&[3, 1]); + h.state_mut().active = Some(0); + let request = browser_insert(&h, 1); + h.state_mut().active = Some(1); + h.state_mut().requests.lock().unwrap().push(request); + h.run_steps(2); + assert_eq!(pages(h.state(), 0), 3, "A is unchanged"); + assert_eq!(pages(h.state(), 1), 1, "C is unchanged"); + let toast = h.state().toast.as_ref().map(|(m, _)| m.clone()).unwrap_or_default(); + assert!(toast.contains("document changed"), "the user is told why nothing happened: {toast:?}"); +} + +#[test] +fn a_browser_insert_into_a_document_edited_meanwhile_is_refused() { + let mut h = harness(&[3]); + let request = browser_insert(&h, 1); + h.state_mut().apply_edit(pdfcraft_engine::Edit::DeletePages { pages: vec![0] }); + h.state_mut().requests.lock().unwrap().push(request); + h.run_steps(2); + assert_eq!(pages(h.state(), 0), 2, "only the user's own edit applied"); +} + +#[test] +fn replace_pages_applies_only_to_the_document_its_dialog_was_opened_on() { + for switch in [false, true] { + let mut h = harness(&[3, 3]); + h.state_mut().active = Some(0); + h.state_mut().use_files(pdfcraft_ui_egui::FilePurpose::ReplacePages, vec![("r.pdf".into(), fixture(1))]); + h.run_steps(2); + assert_eq!(h.state().dialog, Some(pdfcraft_ui_egui::Dialog::ReplacePages)); + if switch { + // A file opened meanwhile (a browser read finishing) becomes the active document. + h.state_mut().active = Some(1); + h.run_steps(2); + } + h.get_by_label("OK").click(); + h.run_steps(2); + if switch { + assert!(!dirty(h.state(), 0) && !dirty(h.state(), 1), "neither document is changed"); + let toast = h.state().toast.as_ref().map(|(m, _)| m.clone()).unwrap_or_default(); + assert!(toast.contains("document changed"), "the user is told why: {toast:?}"); + } else { + assert!(dirty(h.state(), 0) && !dirty(h.state(), 1), "the first document's page is replaced"); + } + } +} diff --git a/crates/ui-egui/tests/print.rs b/crates/ui-egui/tests/print.rs index b2f07acb..d471f6c2 100644 --- a/crates/ui-egui/tests/print.rs +++ b/crates/ui-egui/tests/print.rs @@ -91,3 +91,20 @@ fn cut_stack_dialog_previews_saves_and_refuses_duplex() { assert_eq!(pdfcraft_model::pages(&printed).len(), 3); let _ = std::fs::remove_file(out); } + +#[test] +fn a_failed_save_as_pdf_keeps_the_dialog_open() { + let mut h = harness(); + assert!(h.state_mut().execute("print.dialog")); + h.run_steps(3); + h.state_mut().print_draft.printer = None; + // A folder that doesn't exist: the write fails. + let out = std::env::temp_dir().join(format!("pdfcraft-missing-{}", std::process::id())).join("x.pdf"); + h.state_mut().save_override = Some(out.to_string_lossy().into_owned()); + h.run_steps(2); + h.get_by_label("Save as PDF").click(); + h.run_steps(3); + assert_eq!(h.state().dialog, Some(Dialog::Print), "the dialog stays open, with its settings"); + let toast = h.state().toast.as_ref().map(|(m, _)| m.clone()).unwrap_or_default(); + assert!(toast.contains("Could not save"), "the user is told why: {toast:?}"); +}