From 0bc619cd025a3a6754de81e92be6490c80caf26b Mon Sep 17 00:00:00 2001 From: DWAA1660 Date: Sat, 10 Oct 2026 06:20:08 -0600 Subject: [PATCH] Extract pages: a File name field names the extracted files (#737) (#751) * Extract pages: a File name field names the extracted files (#737) The Extract pages dialog had no way to name the result: separate files were always " (page N).pdf" in the chosen folder, and a new tab was " (extract).pdf". The dialog now has a File name field, filled in with the document's name when it opens. Separate files are " (page N).pdf"; a single renamed page is ".pdf"; extracting into a new tab names the tab (and so the Save As suggestion) ".pdf". The name is made safe for every platform (separators, reserved and control characters, trailing dots and spaces, any-case ".pdf"), and falls back to the document's name when nothing usable is left. Co-Authored-By: Claude Opus 5.5 * Extract pages: Windows device names get a prefix, and the name is capped at 200 bytes "NUL.pdf" writes nowhere on Windows, and 200 CJK characters overflow the 255-byte name limit. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: DWAA1660 Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/ui-egui/src/commands.rs | 2 +- crates/ui-egui/src/dialogs.rs | 6 +++ crates/ui-egui/src/editing.rs | 2 +- crates/ui-egui/src/files.rs | 71 ++++++++++++++++++++++++++++++++- crates/ui-egui/tests/editing.rs | 2 +- crates/ui-egui/tests/pickers.rs | 61 ++++++++++++++++++++++++++++ parity/acrobat-features.toml | 4 +- 7 files changed, 141 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/commands.rs b/crates/ui-egui/src/commands.rs index ad32892ae..8f117f6fd 100644 --- a/crates/ui-egui/src/commands.rs +++ b/crates/ui-egui/src/commands.rs @@ -553,7 +553,7 @@ impl PdfCraftApp { self.boxes_draft.seeded = None; self.dialog = Some(Dialog::PageBoxes); } - "page.extract" => self.dialog = Some(Dialog::Extract), + "page.extract" => self.open_extract_dialog(), "page.rotate_dialog" => { if let Some(i) = active { let n = self.session.get(self.views[i].id).map_or(1, |d| d.info.pages.len()); diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index d0a389ef8..e979eb044 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -561,6 +561,12 @@ pub fn show(app: &mut PdfCraftApp, ctx: &egui::Context) { crate::i18n::fmt(tl!("{n} pages selected."), &[("n", &count.to_string())]) }; ui.label(text); + ui.add_space(4.0); + // The name the pages are saved under (#737); separate files add " (page N)". + ui.horizontal(|ui| { + let l = ui.label(tl!("File name")); + ui.add(egui::TextEdit::singleline(&mut app.extract_draft.name).desired_width(240.0)).labelled_by(l.id); + }); ui.checkbox(&mut app.extract_draft.delete, tl!("Delete pages after extracting")); ui.checkbox(&mut app.extract_draft.separate, tl!("Extract pages as separate files")); ui.add_space(12.0); diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index ecce8dc7f..655a28164 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -143,7 +143,7 @@ impl PdfCraftApp { Some(crate::canvas::ViewAction::Save) => { self.save_active(SaveTarget::InPlace); } - Some(crate::canvas::ViewAction::Extract) => self.dialog = Some(crate::Dialog::Extract), + Some(crate::canvas::ViewAction::Extract) => self.open_extract_dialog(), Some(crate::canvas::ViewAction::Split) => self.dialog = Some(crate::Dialog::Split), Some(crate::canvas::ViewAction::CopyPages { cut }) => self.copy_pages(cut), Some(crate::canvas::ViewAction::PastePages) => self.paste_pages(), diff --git a/crates/ui-egui/src/files.rs b/crates/ui-egui/src/files.rs index c14dd6acf..ac377b1f3 100644 --- a/crates/ui-egui/src/files.rs +++ b/crates/ui-egui/src/files.rs @@ -115,6 +115,37 @@ pub struct ExtractDraft { pub separate: bool, /// Delete the pages after extracting them. pub delete: bool, + /// The name the extracted pages are saved under, without `.pdf` (#737). The dialog fills in + /// the document's name when it opens; empty means that name too. As separate files each + /// page is ` (page N).pdf`, and a single page the user renamed is `.pdf`. + pub name: String, +} + +/// `name` made safe as a file name on every platform: no path separators, reserved or control +/// characters, no trailing dots or spaces (Windows drops them), no `.pdf` (added back on +/// writing), at most 200 characters. Empty when nothing usable is left. +pub(crate) fn file_stem_from_user(name: &str) -> String { + let name = name.trim(); + // Any case of ".pdf": it's added back on writing. + let name = + name.len().checked_sub(4).and_then(|i| name.get(i..).filter(|ext| ext.eq_ignore_ascii_case(".pdf")).and(name.get(..i))).unwrap_or(name); + let cleaned: String = + name.chars().map(|c| if c.is_control() || matches!(c, '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|') { '_' } else { c }).collect(); + // At most 200 bytes, so " (page N).pdf" still fits the usual 255-byte file name limit. + let mut end = cleaned.len().min(200); + while !cleaned.is_char_boundary(end) { + end -= 1; + } + let cleaned = cleaned.get(..end).unwrap_or_default(); + // Trimming trailing dots also leaves "." and ".." empty. + let stem = cleaned.trim().trim_end_matches(['.', ' ']).trim_start(); + // Windows reserves these device names whatever the extension: "NUL.pdf" writes nowhere. + let device = stem.split('.').next().unwrap_or_default().to_ascii_uppercase(); + let reserved = matches!(device.as_str(), "CON" | "PRN" | "AUX" | "NUL") + || (device.len() == 4 + && (device.starts_with("COM") || device.starts_with("LPT")) + && device.as_bytes().get(3).is_some_and(|d| (b'1'..=b'9').contains(d))); + if reserved { format!("_{stem}") } else { stem.to_string() } } /// Pages ▸ Rotate Pages. @@ -382,6 +413,13 @@ impl PdfCraftApp { } } + /// Show Extract pages, its file name filled in with the active document's (#737). + pub fn open_extract_dialog(&mut self) { + self.extract_draft.name = + self.active_ids().and_then(|(_, id)| self.session.get(id)).map(|d| strip_pdf(&d.name).to_string()).unwrap_or_default(); + self.dialog = Some(crate::Dialog::Extract); + } + /// Copy the selected pages (or the current page) into a new unsaved document tab. pub fn extract_selection(&mut self) { // What's typed in a form field is part of the document (#166). @@ -392,12 +430,18 @@ impl PdfCraftApp { let pages = self.views[i].target_pages(); let stem = self.session.get(id).map(|d| strip_pdf(&d.name).to_string()).unwrap_or_default(); let opts = self.extract_draft.clone(); + // The name typed in the dialog (#737), else the document's. + let chosen = file_stem_from_user(&opts.name); + let renamed = !chosen.is_empty() && chosen != file_stem_from_user(&stem); + let base = if chosen.is_empty() { stem.clone() } else { chosen }; if opts.separate { // Each page as its own file, in a chosen folder. let mut named = Vec::new(); for &p in &pages { match self.session.extract(id, &[p]) { - Ok(bytes) => named.push((format!("{stem} (page {}).pdf", p + 1), bytes)), + // One page the user named is saved under exactly that name. + Ok(bytes) if renamed && pages.len() == 1 => named.push((format!("{base}.pdf"), bytes)), + Ok(bytes) => named.push((format!("{base} (page {}).pdf", p + 1), bytes)), Err(e) => { self.notify_fmt("Couldn't extract pages: {e}", &[("e", &e.to_string())]); return; @@ -429,7 +473,9 @@ impl PdfCraftApp { } else { crate::i18n::fmt(tl!("Extracted {n} pages"), &[("n", &pages.len().to_string())]) }; - self.open_created(&format!("{stem} (extract).pdf"), bytes, &message) + // Save As offers the tab's name, so a name typed in the dialog is kept. + let name = if renamed { format!("{base}.pdf") } else { format!("{stem} (extract).pdf") }; + self.open_created(&name, bytes, &message) } Err(e) => { self.notify_fmt("Couldn't extract pages: {e}", &[("e", &e.to_string())]); @@ -754,3 +800,24 @@ impl PdfCraftApp { } } } + +#[cfg(test)] +mod stem_tests { + use super::file_stem_from_user; + + #[test] + fn user_file_names_stay_inside_the_folder_and_writable_everywhere() { + assert_eq!(file_stem_from_user("../../etc/passwd"), ".._.._etc_passwd"); + assert_eq!(file_stem_from_user(".."), ""); + assert_eq!(file_stem_from_user("Report.PDF"), "Report"); + // Windows device names, in any case and with any extension, get a prefix. + assert_eq!(file_stem_from_user("nul"), "_nul"); + assert_eq!(file_stem_from_user("COM1.pdf"), "_COM1"); + assert_eq!(file_stem_from_user("lpt9.tar"), "_lpt9.tar"); + assert_eq!(file_stem_from_user("COM10"), "COM10"); + assert_eq!(file_stem_from_user("Console"), "Console"); + // At most 200 bytes, cut on a character boundary. + let long = file_stem_from_user(&"文".repeat(300)); + assert!(long.len() <= 200 && long.chars().all(|c| c == '文'), "{}", long.len()); + } +} diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index b7d96d1d0..58cf47daf 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -1503,7 +1503,7 @@ fn extract_options_and_rotate_pages_dialog() { h.state_mut().views[0].select_pages(&[1, 2]); h.state_mut().run_command("page.extract"); h.run_steps(2); - h.state_mut().extract_draft = pdfcraft_ui_egui::ExtractDraft { separate: true, delete: true }; + h.state_mut().extract_draft = pdfcraft_ui_egui::ExtractDraft { separate: true, delete: true, ..Default::default() }; h.get_all_by_label("Extract").last().unwrap().click(); h.run_steps(3); let mut names: Vec = std::fs::read_dir(&dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); diff --git a/crates/ui-egui/tests/pickers.rs b/crates/ui-egui/tests/pickers.rs index e29fcf464..99b3f0c5a 100644 --- a/crates/ui-egui/tests/pickers.rs +++ b/crates/ui-egui/tests/pickers.rs @@ -213,6 +213,67 @@ fn extract_and_delete_deletes_the_pages_only_after_their_files_are_written() { let _ = std::fs::remove_dir_all(&dir); } +/// #737: Extract pages names the files after the name typed in the dialog, which starts out +/// as the document's name. +#[test] +fn extract_pages_saves_under_the_file_name_typed_in_the_dialog() { + let dir = scratch("extract-named"); + let mut h = harness(&[3]); + assert!(h.state_mut().execute("page.extract")); + h.run_steps(2); + assert_eq!(h.state().extract_draft.name, "doc0", "the dialog starts with the document's name"); + // Several pages as separate files: each is " (page N).pdf". + h.state_mut().views[0].select_pages(&[0, 2]); + h.state_mut().extract_draft.name = "Invoices.pdf".into(); + h.state_mut().extract_draft.separate = true; + h.state_mut().pick_override = Some(vec![dir.to_string_lossy().into_owned()]); + h.state_mut().extract_selection(); + h.run_steps(2); + assert!(dir.join("Invoices (page 1).pdf").exists() && dir.join("Invoices (page 3).pdf").exists()); + // One renamed page as a separate file is saved under exactly that name; characters a file + // name can't hold become "_". + h.state_mut().views[0].select_pages(&[1]); + h.state_mut().extract_draft.name = " March: 2026/receipt ".into(); + h.state_mut().extract_selection(); + h.run_steps(2); + assert!(dir.join("March_ 2026_receipt.pdf").exists()); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 3, "nothing written outside those names"); + // Into a new tab: the tab, and so Save As, has the typed name. + h.state_mut().extract_draft.separate = false; + h.state_mut().extract_draft.name = "Chapter 2".into(); + h.state_mut().extract_selection(); + h.run_steps(2); + let s = h.state(); + let tab = s.active.and_then(|i| s.views.get(i)).and_then(|v| s.session.get(v.id)).map(|d| d.name.clone()); + assert_eq!(tab.as_deref(), Some("Chapter 2.pdf")); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Left as the document's name (or emptied), the names are the ones Extract always used. +#[test] +fn extract_pages_keeps_the_usual_names_when_the_file_name_is_unchanged() { + let dir = scratch("extract-default-name"); + let mut h = harness(&[3]); + h.state_mut().open_extract_dialog(); + h.state_mut().extract_draft.separate = true; + h.state_mut().pick_override = Some(vec![dir.to_string_lossy().into_owned()]); + h.state_mut().extract_selection(); + h.run_steps(2); + assert!(dir.join("doc0 (page 1).pdf").exists()); + h.state_mut().extract_draft.name = " .. ".into(); + h.state_mut().views[0].select_pages(&[1]); + h.state_mut().extract_selection(); + h.run_steps(2); + assert!(dir.join("doc0 (page 2).pdf").exists(), "a name with nothing usable falls back to the document's"); + h.state_mut().extract_draft.separate = false; + h.state_mut().extract_selection(); + h.run_steps(2); + let s = h.state(); + let tab = s.active.and_then(|i| s.views.get(i)).and_then(|v| s.session.get(v.id)).map(|d| d.name.clone()); + assert_eq!(tab.as_deref(), Some("doc0 (extract).pdf")); + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn a_cancelled_folder_pick_extracts_and_deletes_nothing() { let mut h = harness(&[3]); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index e10ea53fc..43d71dd8b 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -2722,8 +2722,8 @@ milestone = "M4" status = "shipped" commands = ["page.extract"] tools = ["page_extract"] -evidence = ["crates/ui-egui/tests/editing.rs::extract_options_and_rotate_pages_dialog", "crates/automation/tests/automation.rs::organizing_with_filters_bookmark_splits_and_extract_options"] -notes = "" +evidence = ["crates/ui-egui/tests/editing.rs::extract_options_and_rotate_pages_dialog", "crates/automation/tests/automation.rs::organizing_with_filters_bookmark_splits_and_extract_options", "crates/ui-egui/tests/pickers.rs::extract_pages_saves_under_the_file_name_typed_in_the_dialog"] +notes = "The dialog has a File name field (#737), filled in with the document's name: pages are written as ` (page N).pdf` into the chosen folder, and one renamed page as `.pdf`. Extracting into a new tab names the tab (and so Save As) after it." [[feature]] id = "organize.extract-and-delete"