Combine and Insert bring only the bookmarks and attachments of the pages chosen

With a selection of a file's pages, Combine and Insert Pages from File
brought every bookmark title along, the ones whose page was left out
without a destination, and Combine brought every attachment.

Only the bookmarks that lead to a copied page, and those above them,
come along now, as Extract keeps them since #4041. Attachments belong to
the whole file: they come along with all of its pages only.

crates/engine/src/session/files.rs

@@ -289,7 +289,8 @@ impl Session {
 
     /// [`Self::combine_unlocked`] where sources with the same `groups` key (by position) are one
     /// file split into several runs placed apart: each file is opened and copied once, so it keeps
-    /// one bookmark, its links between its own pages, its fields and its attachments whole (see
+    /// one bookmark, and its links between its own pages and its fields whole across its runs; its
+    /// attachments come along when its runs use all of its pages (see
     /// `pdfcraft_organize::combine_grouped`). A group's later sources use its first source's bytes
     /// and password; a source without a key is a file of its own.
     pub fn combine_grouped(&self, sources: &[CombineSource], groups: &[u64], passwords: &[Option<&str>]) -> Result<Arc<Vec<u8>>, EditError> {

crates/organize/src/import.rs

@@ -100,8 +100,8 @@ fn added_objects(dst: &Document, before: &HashSet<u32>) -> Vec<ObjRef> {
 /// Insert Pages from File: [`import_pages_with`], and the inserted file's bookmarks come along
 /// under one new bookmark titled `title` (the file's name) at its first inserted page, as Combine
 /// Files does for each file. That bookmark goes among the top-level bookmarks where the pages went:
-/// before the first one that points after them. Bookmarks to pages left out lose their destination;
-/// a file without bookmarks adds none.
+/// before the first one that points after them. Bookmarks to pages left out stay behind, unless a
+/// bookmark below them leads to an inserted page; a file without bookmarks adds none.
 pub fn insert_file_pages(
     dst: &mut Document,
     src: &Document,

crates/organize/src/import/carry.rs

@@ -98,9 +98,13 @@ pub(super) fn source_outline(src: &Document, pages: HashMap<ObjRef, ObjRef>) ->
         .and_then(|r| src.get(r).as_dict().cloned())
         .and_then(|c| c.get(b"Outlines").map(|o| src.resolve(o)))
         .and_then(|o| o.as_dict().and_then(|d| d.reference(b"First")));
+    let whole = crate::page_count(src).is_ok_and(|n| pages.len() >= n);
     let mapper = Copier::new(src, HashMap::new(), pages);
     let mut budget = 10_000usize;
-    outline_level(&mapper, first, 0, &mut budget)
+    let outline = outline_level(&mapper, first, 0, &mut budget);
+    // Of some of the pages, only the bookmarks that lead to a copied page and those above them:
+    // the others would carry their titles along into a file without their pages.
+    if whole { outline } else { kept_bookmarks(outline) }
 }
 
 /// `nodes` without the bookmarks whose page was not copied (no destination left), unless a bookmark

crates/organize/src/import/combine.rs

@@ -1,5 +1,5 @@
 use super::carry::{Mark, add_outline, collect_attachments, kept_bookmarks, set_attachments, source_outline, write_outline_level};
-use super::{Dict, Document, FieldConflicts, HashMap, ObjRef, Object, OrganizeError, import_pages_mapped, rebuild};
+use super::{Dict, Document, FieldConflicts, HashMap, HashSet, ObjRef, Object, OrganizeError, import_pages_mapped, rebuild};
 
 /// A new document containing copies of `pages` from `src` (Extract Pages / Split).
 /// Document information (title, author…) is carried over, and so is the print standard the
@@ -95,7 +95,8 @@ pub fn combine_with(sources: &[(&str, &Document)], conflicts: FieldConflicts) ->
 }
 
 /// Combine Files with chosen pages: each source contributes `pages` (0-based, in that order;
-/// `None` for all of them). Bookmarks that point at pages left out lose their destination.
+/// `None` for all of them). A source whose pages are not all used brings only the bookmarks that
+/// lead to its copied pages (and those above them), and none of its attachments.
 pub fn combine_selected(sources: &[(&str, &Document, Option<&[usize]>)]) -> Result<Document, OrganizeError> {
     combine_selected_with(sources, FieldConflicts::Rename).map(|o| o.doc)
 }
@@ -187,7 +188,11 @@ impl Combiner {
             let outline = source_outline(src, imported.page_map);
             self.marks.push(Mark { title: title.to_string(), page: *first, outline });
         }
-        collect_attachments(&mut self.out, src, &mut self.attachments)?;
+        // Attachments belong to the whole file: they come along with all of its pages, not with a
+        // selection of them.
+        if pages.iter().collect::<HashSet<_>>().len() == n {
+            collect_attachments(&mut self.out, src, &mut self.attachments)?;
+        }
         let declared = crate::pdfx::PrintStandard::of(src);
         match self.standard.as_ref().map(|first| first.declared.same_as(&declared)) {
             Some(same) => self.agreed &= same,
@@ -227,9 +232,9 @@ pub type Run<'a> = (usize, &'a str, &'a Document, Option<&'a [usize]>);
 
 /// Combine Files where a file's pages may be split into several runs with other files' pages
 /// between them. Each file (group) is copied once, all its pages together, then the pages are put
-/// in the runs' order: its links between its own pages, its form fields and its attachments stay
-/// whole, and it gets one top-level bookmark (the title of its first run) at its first page, its
-/// own bookmarks nested. Otherwise as [`combine_selected`] (print standard, shared resources).
+/// in the runs' order: its links between its own pages and its form fields stay whole across its
+/// runs, its attachments come along when its runs use all of its pages, and it gets one top-level
+/// bookmark (the title of its first run) at its first page, its own bookmarks nested. Otherwise as [`combine_selected`] (print standard, shared resources).
 pub fn combine_grouped(runs: &[Run<'_>]) -> Result<Document, OrganizeError> {
     combine_grouped_with(runs, FieldConflicts::Rename).map(|o| o.doc)
 }
@@ -281,7 +286,10 @@ pub fn combine_grouped_with(runs: &[Run<'_>], conflicts: FieldConflicts) -> Resu
         if let Some(first) = imported.pages.first() {
             marks.push(Mark { title: file.title.to_string(), page: *first, outline: source_outline(file.src, imported.page_map) });
         }
-        collect_attachments(&mut out, file.src, &mut attachments)?;
+        // Attachments come along with all of a file's pages, not with a selection of them.
+        if file.pages.iter().collect::<HashSet<_>>().len() == crate::page_count(file.src)? {
+            collect_attachments(&mut out, file.src, &mut attachments)?;
+        }
         let declared = crate::pdfx::PrintStandard::of(file.src);
         match &standard {
             None => standard = Some((declared, file.src, imported.objects)),

crates/organize/src/tests.rs

@@ -1591,13 +1591,12 @@ fn inserting_a_file_brings_its_bookmarks_under_one_named_after_it() {
     let page_of = |title: &str| marks.iter().find(|b| b.title == title).and_then(|b| b.page);
     assert_eq!((page_of("annex"), page_of("Chapter one"), page_of("Section"), page_of("End")), (Some(1), Some(1), Some(2), Some(4)));
     assert!(!marks.iter().find(|b| b.title == "annex").unwrap().open, "the file's bookmarks start collapsed");
-    // Inserted last, after every bookmark; only the pages chosen keep a destination.
+    // Inserted last, after every bookmark; a bookmark to a page left out stays behind with it.
     let mut host = open(fixture());
     add_bookmark(&mut host, &[], 0, "Start", 0).unwrap();
     insert_file_pages(&mut host, &doc_c(), "annex", &[0], 9, FieldConflicts::Rename).unwrap();
     let marks = bookmarks(&host);
-    assert_eq!(titles(&marks), ["Start", "annex[Chapter one[Section]]"]);
-    assert_eq!(marks.iter().find(|b| b.title == "Section").unwrap().page, None, "its page was left out");
+    assert_eq!(titles(&marks), ["Start", "annex[Chapter one]"], "Section's page was left out");
     // A file without bookmarks adds none, and a host without an outline gets one only when needed.
     let mut host = open(fixture());
     insert_file_pages(&mut host, &doc_b(), "plain", &[0], 0, FieldConflicts::Rename).unwrap();
@@ -3985,3 +3984,42 @@ fn an_outline_deeper_than_the_listing_is_exported_as_truncated() {
     crate::import_outline(&mut target, &[], 0, &tree.nodes, false).unwrap();
     assert_eq!(crate::bookmarks(&target).len(), 33);
 }
+
+/// The titles of a bookmark's children, each with its own children's titles.
+fn bookmark_titles(doc: &Document, first: Option<pdfcraft_cos::ObjRef>) -> Vec<(String, Vec<String>)> {
+    let mut out = Vec::new();
+    let mut at = first;
+    while let Some(r) = at {
+        let item = doc.get(r).as_dict().cloned().unwrap();
+        let title = item.get(b"Title").and_then(|t| t.as_string()).map(|t| t.to_text()).unwrap_or_default();
+        let children = bookmark_titles(doc, item.reference(b"First")).into_iter().map(|(t, _)| t).collect();
+        out.push((title, children));
+        at = item.reference(b"Next");
+    }
+    out
+}
+
+/// Combining some of a file's pages brings only the bookmarks that lead to them (and those above
+/// them), and none of the file's attachments.
+#[test]
+fn combining_some_pages_brings_only_their_bookmarks_and_no_attachments() {
+    let source_bookmarks = |out: &Document| {
+        let outlines = out.get(catalog(out).reference(b"Outlines").unwrap()).as_dict().cloned().unwrap();
+        let file = out.get(outlines.reference(b"First").unwrap()).as_dict().cloned().unwrap();
+        bookmark_titles(out, file.reference(b"First"))
+    };
+    let attachments = |out: &Document| {
+        catalog(out).get(b"Names").map(|n| out.resolve(n)).is_some_and(|n| n.as_dict().is_some_and(|d| d.contains(b"EmbeddedFiles")))
+    };
+    // Page 1 only: "Section" leads to page 2.
+    let first = full_roundtrip(&combine_selected(&[("C", &doc_c(), Some(&[0]))]).unwrap());
+    assert_eq!(source_bookmarks(&first), [("Chapter one".to_string(), vec![])]);
+    assert!(!attachments(&first), "an attachment came along with a selection of pages");
+    // Page 2 only: "Chapter one" stays, as the way to "Section".
+    let second = full_roundtrip(&combine_selected(&[("C", &doc_c(), Some(&[1]))]).unwrap());
+    assert_eq!(source_bookmarks(&second), [("Chapter one".to_string(), vec!["Section".to_string()])]);
+    // Every page: all of it, attachments too.
+    let all = full_roundtrip(&combine_selected(&[("C", &doc_c(), Some(&[0, 1]))]).unwrap());
+    assert_eq!(source_bookmarks(&all), [("Chapter one".to_string(), vec!["Section".to_string()])]);
+    assert!(attachments(&all));
+}