diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 2c9d6b4579..91bad575c4 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -520,11 +520,11 @@ impl eframe::App for PdfCraftApp { let dropped = self.drop_on_signature_pad(dropped); #[cfg(not(target_arch = "wasm32"))] let dropped = self.drop_on_grid(dropped, ctx); - for f in dropped { - // Files dropped on the Combine files tab join its list instead of opening. - if self.combine_showing() { - self.drop_into_combine(f, ctx); - } else { + // Files dropped on the Combine files tab join its list, all at once, instead of opening. + if self.combine_showing() && !dropped.is_empty() { + self.drop_into_combine(dropped, ctx); + } else { + for f in dropped { self.open_dropped(f, ctx); } } diff --git a/crates/ui-egui/src/app/input.rs b/crates/ui-egui/src/app/input.rs index cda282e9ed..4d6742cf70 100644 --- a/crates/ui-egui/src/app/input.rs +++ b/crates/ui-egui/src/app/input.rs @@ -19,7 +19,7 @@ impl PdfCraftApp { self.open_path(&p); return; } - let name = f.path().file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "dropped.pdf".into()); + let name = crate::files::dropped_name(&f); match f.bytes() { Ok(bytes) => { if self.ctx.is_some() && !self.run_inline { @@ -76,7 +76,7 @@ impl PdfCraftApp { }; let mut files = Vec::new(); for f in &dropped { - let name = f.path().file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "dropped.pdf".into()); + let name = crate::files::dropped_name(f); if f.path().is_absolute() { files.push((name, Err(f.path().to_path_buf()))); continue; @@ -130,7 +130,7 @@ impl PdfCraftApp { let inbox = self.inbox.clone(); let ctx = ctx.clone(); wasm_bindgen_futures::spawn_local(async move { - let name = f.path().file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "dropped.pdf".into()); + let name = crate::files::dropped_name(&f); if let Ok(bytes) = f.bytes_async().await && let Ok(mut q) = inbox.lock() { diff --git a/crates/ui-egui/src/workflows/combine_ui/files.rs b/crates/ui-egui/src/workflows/combine_ui/files.rs index 4947e4f8b1..85634c2aff 100644 --- a/crates/ui-egui/src/workflows/combine_ui/files.rs +++ b/crates/ui-egui/src/workflows/combine_ui/files.rs @@ -274,6 +274,13 @@ pub(crate) struct Incoming { pub open_doc: Option, } +impl Incoming { + /// A file known only by its name and bytes. + pub(crate) fn new(name: String, bytes: Vec) -> Self { + Incoming { name, bytes: Arc::new(bytes), modified: None, note: None, open_doc: None } + } +} + /// A new value for [`CombineFile::auth`]: counted for the whole run, outside the undo history. fn next_auth() -> u64 { static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); diff --git a/crates/ui-egui/src/workflows/combine_ui/parts.rs b/crates/ui-egui/src/workflows/combine_ui/parts.rs index 5cba711a03..a64d0eeb34 100644 --- a/crates/ui-egui/src/workflows/combine_ui/parts.rs +++ b/crates/ui-egui/src/workflows/combine_ui/parts.rs @@ -38,6 +38,12 @@ fn combinable(ext: &str) -> bool { std::iter::once("pdf").chain(pdfcraft_engine::CONVERTIBLE).any(|c| ext.eq_ignore_ascii_case(c)) } +/// Whether Combine takes a file by what it is: a PDF (by its header or its name) or an image or +/// text file it converts, as `Session::as_pdf` tells them apart. +pub(super) fn takes(name: &str, bytes: &[u8]) -> bool { + pdfcraft_engine::source_kind(name, bytes).is_some() || name.to_ascii_lowercase().ends_with(".pdf") +} + /// A file's bookmark in the combined PDF: its name without ".pdf", or without the extension of /// the image or text file it was made from. pub(super) fn title_of(name: &str) -> &str { diff --git a/crates/ui-egui/src/workflows/combine_ui/session.rs b/crates/ui-egui/src/workflows/combine_ui/session.rs index 594b5f745a..7ef50d5ffc 100644 --- a/crates/ui-egui/src/workflows/combine_ui/session.rs +++ b/crates/ui-egui/src/workflows/combine_ui/session.rs @@ -3,7 +3,7 @@ use super::MAX_FOLDER_FILES; use super::files::{AssessError, Assessed, CombineFile, Incoming, Secret, Snapshot, SortKey, UnlockPrompt, assess}; #[cfg(not(target_arch = "wasm32"))] use super::parts::combinable_in; -use super::parts::{Part, items_of, moved_order, natural, parts_of, range_of, title_of}; +use super::parts::{Part, items_of, moved_order, natural, parts_of, range_of, takes, title_of}; use super::{Arc, BTreeSet, CombineTab, HISTORY, Modifiers, PdfCraftApp}; impl PdfCraftApp { @@ -246,9 +246,21 @@ impl PdfCraftApp { /// Add picked files to the Combine files list (and show it). pub(crate) fn stage_combine(&mut self, files: Vec<(String, Vec)>) { - let incoming = - files.into_iter().map(|(name, bytes)| Incoming { name, bytes: Arc::new(bytes), modified: None, note: None, open_doc: None }).collect(); - self.stage_combine_with(incoming); + self.stage_combine_with(files.into_iter().map(|(name, bytes)| Incoming::new(name, bytes)).collect()); + } + + /// Files dropped on the Combine files tab join the end of its list in the order dropped, as + /// one undo step. A drop isn't filtered like a pick, so files of a kind Combine doesn't take + /// are left out, and named. + pub(crate) fn add_dropped_to_combine(&mut self, files: Vec) { + let (taken, refused): (Vec<_>, Vec<_>) = files.into_iter().partition(|f| takes(&f.name, &f.bytes)); + if !refused.is_empty() { + let names: Vec<&str> = refused.iter().map(|f| f.name.as_str()).collect(); + self.notify_fmt("Not added: {names}. Combine takes PDFs, images and text files.", &[("names", &names.join(", "))]); + } + if !taken.is_empty() { + self.stage_combine_with(taken); + } } /// Add files to the Combine files list, with what can be known about them up front. diff --git a/crates/ui-egui/src/workflows/files.rs b/crates/ui-egui/src/workflows/files.rs index b8bda7183a..6a7a0434b3 100644 --- a/crates/ui-egui/src/workflows/files.rs +++ b/crates/ui-egui/src/workflows/files.rs @@ -149,6 +149,8 @@ pub struct FileRequest { purpose: FilePurpose, target: Option, files: Vec<(String, Vec)>, + /// Dropped on the Combine files tab rather than picked: what Combine can't take is left out. + dropped: bool, } /// Files picked asynchronously (web), waiting to be used. @@ -310,6 +312,11 @@ fn read_named(path: &std::path::Path) -> Result<(String, Vec), (String, Stri } } +/// The name of a file dropped on the window ("dropped.pdf" when it has none). +pub(crate) fn dropped_name(f: &egui::DroppedFileHandle) -> String { + f.path().file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "dropped.pdf".into()) +} + impl PdfCraftApp { /// Ask for files to add to the Combine files list (its Add files… button). pub fn combine_dialog(&mut self) { @@ -451,37 +458,53 @@ impl PdfCraftApp { } } - /// A file dropped on the Combine files tab: added to its list. + /// Files dropped on the Combine files tab join its list in the order dropped (see + /// [`Self::add_dropped_to_combine`]); a folder adds the files in it and its subfolders. #[cfg(not(target_arch = "wasm32"))] - pub(crate) fn drop_into_combine(&mut self, f: egui::DroppedFileHandle, _ctx: &egui::Context) { - // A folder: the PDFs in it and in the folders inside it. - if f.path().is_absolute() && f.path().is_dir() { - self.add_folder_to_combine(f.path(), true); - return; - } - if f.path().is_absolute() { - self.use_paths(FilePurpose::Combine, &[f.path().to_path_buf()]); - return; - } - let name = f.path().file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "dropped.pdf".into()); - match f.bytes() { - Ok(bytes) => self.use_files(FilePurpose::Combine, vec![(name, bytes)]), - Err(e) => self.notify_fmt("Couldn't read {name}: {e}", &[("name", &name), ("e", &e.to_string())]), + pub(crate) fn drop_into_combine(&mut self, dropped: Vec, _ctx: &egui::Context) { + let mut files = Vec::new(); + for f in dropped { + let path = f.path(); + if path.is_absolute() && path.is_dir() { + // After the files dropped before it. + self.add_dropped_to_combine(std::mem::take(&mut files)); + self.add_folder_to_combine(path, true); + continue; + } + let name = dropped_name(&f); + // A file on disk is read from there, with when it was last modified; a drop that + // carries only bytes (no path) as it is. + let (read, modified) = if path.is_absolute() { + (std::fs::read(path).map_err(|e| e.to_string()), std::fs::metadata(path).and_then(|m| m.modified()).ok()) + } else { + (f.bytes(), None) + }; + match read { + Ok(bytes) => files.push(crate::combine_ui::Incoming { modified, ..crate::combine_ui::Incoming::new(name, bytes) }), + Err(e) => self.notify_fmt("Couldn't read {name}: {e}", &[("name", &name), ("e", &e)]), + } } + self.add_dropped_to_combine(files); } /// Browsers read dropped files asynchronously; they join the Combine files list next frame. #[cfg(target_arch = "wasm32")] - pub(crate) fn drop_into_combine(&mut self, f: egui::DroppedFileHandle, ctx: &egui::Context) { + pub(crate) fn drop_into_combine(&mut self, dropped: Vec, ctx: &egui::Context) { let requests = self.requests.clone(); - let request = self.file_request(FilePurpose::Combine, Vec::new()); + let request = FileRequest { dropped: true, ..self.file_request(FilePurpose::Combine, Vec::new()) }; let ctx = ctx.clone(); wasm_bindgen_futures::spawn_local(async move { - let name = f.path().file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "dropped.pdf".into()); - if let Ok(bytes) = f.bytes_async().await + // One after another, so they join the list in the order dropped. + let mut files = Vec::new(); + for f in dropped { + if let Ok(bytes) = f.bytes_async().await { + files.push((dropped_name(&f), bytes)); + } + } + if !files.is_empty() && let Ok(mut q) = requests.lock() { - q.push(FileRequest { files: vec![(name, bytes)], ..request }); + q.push(FileRequest { files, ..request }); ctx.request_repaint(); } }); @@ -492,7 +515,7 @@ impl PdfCraftApp { /// `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 } + FileRequest { purpose, target, files, dropped: false } } /// `doc` as it is now, for a pick that will edit it. @@ -516,12 +539,19 @@ impl PdfCraftApp { /// 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 FileRequest { purpose, target, files } in pending { + for FileRequest { purpose, target, files, dropped } 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)) { + let used = pdfcraft_engine::guard(|| { + if dropped { + self.add_dropped_to_combine(files.into_iter().map(|(name, bytes)| crate::combine_ui::Incoming::new(name, bytes)).collect()); + } else { + self.use_files(purpose, files); + } + }); + if let Err(m) = used { self.notify_fmt("That didn't work: an internal error stopped it ({m}).", &[("m", m.as_str())]); } } diff --git a/crates/ui-egui/tests/it/ui.rs b/crates/ui-egui/tests/it/ui.rs index a7f5b4cf7f..a464595615 100644 --- a/crates/ui-egui/tests/it/ui.rs +++ b/crates/ui-egui/tests/it/ui.rs @@ -1166,6 +1166,86 @@ fn a_folder_dropped_on_the_combine_tab_adds_its_pdfs() { assert_eq!(names, ["two.pdf", "one.pdf"], "subfolders included, in path order"); } +/// The Combine files tab, showing, with one file in its list already. +fn combine_tab_with_one_file() -> Harness<'static, PdfCraftApp> { + let mut h = Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_eframe(|_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app + }); + h.run_steps(3); + h.state_mut().set_option("combine-view", "list").unwrap(); + h.state_mut().execute("page.combine"); + h.state_mut().use_files(pdfcraft_ui_egui::FilePurpose::Combine, vec![("first.pdf".into(), FIXTURE.to_vec())]); + h.run_steps(2); + h +} + +fn combine_names(h: &Harness<'_, PdfCraftApp>) -> Vec { + h.state().combine_draft.iter().map(|f| f.name.clone()).collect() +} + +#[test] +fn files_dropped_on_the_combine_tab_join_the_end_of_its_list_in_the_order_dropped() { + let mut h = combine_tab_with_one_file(); + let dir = std::env::temp_dir().join(format!("pdfcraft-combine-drop-order-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + // Files on disk come with their path (and no bytes); browsers hand over only a name and bytes. + let on_disk = |name: &str| -> egui::DroppedFileHandle { + std::fs::write(dir.join(name), FIXTURE).unwrap(); + std::sync::Arc::new(Dropped { path: dir.join(name), bytes: Vec::new() }) + }; + let in_memory = + |name: &str, bytes: &[u8]| -> egui::DroppedFileHandle { std::sync::Arc::new(Dropped { path: name.into(), bytes: bytes.to_vec() }) }; + let dropped = [on_disk("c.pdf"), in_memory("a.pdf", FIXTURE), in_memory("notes.txt", b"Some notes"), on_disk("b.pdf")]; + h.input_mut().dropped_files.extend(dropped); + h.run_steps(3); + let _ = std::fs::remove_dir_all(&dir); + assert!(h.state().views.is_empty(), "nothing opens"); + assert_eq!(combine_names(&h), ["first.pdf", "c.pdf", "a.pdf", "notes.txt", "b.pdf"]); + let draft = &h.state().combine_draft; + assert!(draft.iter().all(|f| f.problem.is_none()), "every file can be combined"); + assert!(draft[3].pages == 1, "the text file became a page"); + assert!(draft[1].modified.is_some() && draft[2].modified.is_none()); + // One drop, one step to undo. + h.state_mut().execute("edit.undo"); + h.run_steps(2); + assert_eq!(combine_names(&h), ["first.pdf"]); +} + +#[test] +fn a_file_combine_does_not_take_is_left_out_of_a_drop_and_named() { + let mut h = combine_tab_with_one_file(); + let path = std::env::temp_dir().join(format!("pdfcraft-combine-drop-{}.zip", std::process::id())); + std::fs::write(&path, b"PK\x03\x04").unwrap(); + let dropped: [egui::DroppedFileHandle; 4] = [ + std::sync::Arc::new(Dropped { path: "report.docx".into(), bytes: b"PK\x03\x04".to_vec() }), + std::sync::Arc::new(Dropped { path: "one.pdf".into(), bytes: FIXTURE.to_vec() }), + std::sync::Arc::new(Dropped { path: path.clone(), bytes: Vec::new() }), + // A PDF without the name of one is still a PDF. + std::sync::Arc::new(Dropped { path: "scan".into(), bytes: FIXTURE.to_vec() }), + ]; + h.input_mut().dropped_files.extend(dropped); + h.run_steps(3); + std::fs::remove_file(&path).ok(); + assert_eq!(combine_names(&h), ["first.pdf", "one.pdf", "scan"]); + let zip = path.file_name().unwrap().to_string_lossy().into_owned(); + h.get_by_label(&format!("Not added: report.docx, {zip}. Combine takes PDFs, images and text files.")); +} + +#[test] +fn a_pdf_dropped_on_a_document_opens_while_the_combine_tab_waits_behind_it() { + let mut h = combine_tab_with_one_file(); + h.state_mut().open_bytes("open.pdf", None, FIXTURE.to_vec()).unwrap(); + h.run_steps(2); + assert!(!h.state().combine_showing() && h.state().combine_tab.open); + let file: egui::DroppedFileHandle = std::sync::Arc::new(Dropped { path: "dropped.pdf".into(), bytes: FIXTURE.to_vec() }); + h.input_mut().dropped_files.push(file); + support::wait_until(&mut h, "dropped document", |app| app.views.len() == 2); + assert_eq!(combine_names(&h), ["first.pdf"], "the Combine files list is left as it was"); +} + #[test] fn files_and_quit_from_the_operating_system() { // #73: macOS hands Finder double-clicks, Open With and Dock drops over as Apple events.