Combine files: a drop is one undo step, in order, and names what it can't take (#6645)

* Combine files: add a drop as one step, in order, and name what it can't take

Files dropped on the Combine files tab are handled as one batch: they join
the end of the list in the order dropped, as one undo step, from their path
or, when a drop carries only bytes, from those. A folder still adds the
files in it and its subfolders. Files of a kind Combine doesn't take are
left out and named in a notice, instead of joining the list as files that
can't be read. In a browser the dropped files are read one after another,
so they keep their order too.

* Combine files: let the web build use Incoming for dropped files

* Parity: Combine files takes drops in order, as one step, and names what it leaves out

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);
             }
         }

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()
             {

crates/ui-egui/src/workflows/combine_ui.rs

@@ -23,7 +23,6 @@ mod session;
 mod table;
 mod view;
 
-#[cfg(not(target_arch = "wasm32"))]
 pub(crate) use files::Incoming;
 pub use files::{Columns, CombineFile, CombineView, ConflictPreview, Lock, SortKey};
 pub(crate) use files::{Preview, RowAction};

crates/ui-egui/src/workflows/combine_ui/files.rs

@@ -274,6 +274,13 @@ pub(crate) struct Incoming {
     pub open_doc: Option<pdfcraft_engine::DocId>,
 }
 
+impl Incoming {
+    /// A file known only by its name and bytes.
+    pub(crate) fn new(name: String, bytes: Vec<u8>) -> 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);

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 {

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<u8>)>) {
-        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<Incoming>) {
+        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.

crates/ui-egui/src/workflows/files.rs

@@ -149,6 +149,8 @@ pub struct FileRequest {
     purpose: FilePurpose,
     target: Option<PickTarget>,
     files: Vec<(String, Vec<u8>)>,
+    /// 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<u8>), (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<egui::DroppedFileHandle>, _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<egui::DroppedFileHandle>, 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<u8>)>) -> 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())]);
             }
         }

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<String> {
+    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.

parity/acrobat-features.toml

@@ -3034,8 +3034,8 @@ milestone = "M4"
 status = "shipped"
 commands = ["page.combine"]
 tools = ["doc_combine"]
-evidence = ["crates/organize/src/tests.rs::combine_concatenates_with_a_bookmark_per_file", "crates/ui-egui/tests/it/editing.rs::combining_files_opens_a_new_unsaved_tab", "crates/ui-egui/tests/it/editing.rs::combine_files_opens_in_its_own_tab", "crates/ui-egui/tests/it/editing.rs::combine_lists_size_and_warns_before_combining", "crates/ui-egui/tests/it/editing.rs::a_password_protected_file_is_unlocked_with_its_password_and_combined", "crates/ui-egui/tests/it/editing.rs::a_folder_adds_its_pdfs_images_and_text_in_order_and_optionally_its_subfolders", "crates/ui-egui/tests/it/editing.rs::images_and_text_files_join_the_combine_list_as_pdfs", "crates/ui-egui/tests/it/editing.rs::files_that_cannot_be_read_are_listed_and_left_out_once_the_user_agrees", "crates/ui-egui/tests/it/ui.rs::pdfs_dropped_on_the_combine_tab_join_its_list", "crates/automation/tests/it/automation.rs::combine_extract_and_split", "crates/automation/tests/it/automation.rs::combine_opens_protected_files_with_their_passwords", "crates/automation/tests/it/automation.rs::combine_takes_images_and_text_files", "crates/engine/src/tests.rs::files_for_combine_are_made_into_pdfs_and_pdfs_pass_through", "crates/organize/src/tests.rs::combine_keeps_pdfx_when_every_source_agrees", "crates/organize/src/tests.rs::combine_claims_no_pdfx_when_the_sources_disagree"]
-notes = "Combine files is its own tab: a table of the files (name, pages, size, modified, warnings) with column sorting, resizing and moving; files, folders (with subfolders) and open documents can be added, or dropped on the tab. Images (PNG, JPEG, TIFF, GIF, BMP, JPEG 2000) and .txt files become PDFs as they are added (#569; also in doc_combine). A file that can't be read stays in the list with why, and Combine asks before leaving it out; Office documents, HTML and other formats are not converted yet. Problems show before Combine (password, security settings that withhold page assembly, bad page ranges, duplicates); protected files are unlocked with their open or permissions password (also doc_combine passwords). The result is never encrypted, and is not yet offered for protection in the same step. A PDF/X result (output intent, GTS_PDFXVersion, Trapped, XMP pdfxid) only when every source declares the same version and output intent (#263). When they differ the result claims none, and nothing reports that yet; Acrobat behaviour not observed."
+evidence = ["crates/organize/src/tests.rs::combine_concatenates_with_a_bookmark_per_file", "crates/ui-egui/tests/it/editing.rs::combining_files_opens_a_new_unsaved_tab", "crates/ui-egui/tests/it/editing.rs::combine_files_opens_in_its_own_tab", "crates/ui-egui/tests/it/editing.rs::combine_lists_size_and_warns_before_combining", "crates/ui-egui/tests/it/editing.rs::a_password_protected_file_is_unlocked_with_its_password_and_combined", "crates/ui-egui/tests/it/editing.rs::a_folder_adds_its_pdfs_images_and_text_in_order_and_optionally_its_subfolders", "crates/ui-egui/tests/it/editing.rs::images_and_text_files_join_the_combine_list_as_pdfs", "crates/ui-egui/tests/it/editing.rs::files_that_cannot_be_read_are_listed_and_left_out_once_the_user_agrees", "crates/ui-egui/tests/it/ui.rs::pdfs_dropped_on_the_combine_tab_join_its_list", "crates/ui-egui/tests/it/ui.rs::files_dropped_on_the_combine_tab_join_the_end_of_its_list_in_the_order_dropped", "crates/ui-egui/tests/it/ui.rs::a_file_combine_does_not_take_is_left_out_of_a_drop_and_named", "crates/ui-egui/tests/it/ui.rs::a_pdf_dropped_on_a_document_opens_while_the_combine_tab_waits_behind_it", "crates/automation/tests/it/automation.rs::combine_extract_and_split", "crates/automation/tests/it/automation.rs::combine_opens_protected_files_with_their_passwords", "crates/automation/tests/it/automation.rs::combine_takes_images_and_text_files", "crates/engine/src/tests.rs::files_for_combine_are_made_into_pdfs_and_pdfs_pass_through", "crates/organize/src/tests.rs::combine_keeps_pdfx_when_every_source_agrees", "crates/organize/src/tests.rs::combine_claims_no_pdfx_when_the_sources_disagree"]
+notes = "Combine files is its own tab: a table of the files (name, pages, size, modified, warnings) with column sorting, resizing and moving; files, folders (with subfolders) and open documents can be added, or dropped on the tab (desktop and web): a drop joins the end of the list in the order dropped, as one undo step, not at the place under the pointer; files of a kind Combine does not take are left out and named. Dropped elsewhere, files open as before. Dropping adds no engine behaviour, so it has no tool of its own (doc_combine takes the files). Images (PNG, JPEG, TIFF, GIF, BMP, JPEG 2000) and .txt files become PDFs as they are added (#569; also in doc_combine). A file that can't be read stays in the list with why, and Combine asks before leaving it out; Office documents, HTML and other formats are not converted yet. Problems show before Combine (password, security settings that withhold page assembly, bad page ranges, duplicates); protected files are unlocked with their open or permissions password (also doc_combine passwords). The result is never encrypted, and is not yet offered for protection in the same step. A PDF/X result (output intent, GTS_PDFXVersion, Trapped, XMP pdfxid) only when every source declares the same version and output intent (#263). When they differ the result claims none, and nothing reports that yet; Acrobat behaviour not observed."
 
 [[feature]]
 id = "organize.combine-bookmarks"