diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 8e4acdf891..1941c1858c 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -761,9 +761,10 @@ impl Automation { }; let mime = pdfcraft_engine::mime_for_name(&name).map(str::to_string); let description = a.opt_str("description")?.map(str::to_string); - let before: Vec = self.doc(&a)?.info.attachments.iter().map(|x| x.name.clone()).collect(); + // Only the document-level keys: an annotation attachment of the same name is not the new one. + let before = document_attachment_keys(self.doc(&a)?); let out = self.apply(&a, Edit::AddAttachment { name, bytes, description, mime })?; - let key = self.doc(&a)?.info.attachments.iter().map(|x| x.name.clone()).find(|n| !before.contains(n)).unwrap_or_default(); + let key = document_attachment_keys(self.doc(&a)?).into_iter().find(|n| !before.contains(n)).unwrap_or_default(); json!({ "name": key, "document": out }) } "attachment_delete" => { @@ -2314,6 +2315,18 @@ impl Args<'_> { /// Bookmark rows in listing order as JSON: a row holds the rows that follow it on the page at the /// next level down in `children`, and paths and pages are 1-based. +/// The name-tree keys of a document's own attachments, leaving out the files attached to pages as comments. +fn document_attachment_keys(doc: &Document) -> Vec { + doc.info + .attachments + .iter() + .filter_map(|x| match &x.source { + pdfcraft_render::AttachmentSource::Document { key } => Some(key.clone()), + pdfcraft_render::AttachmentSource::Annotation { .. } => None, + }) + .collect() +} + fn nest_rows(rows: &[pdfcraft_engine::BookmarkRow], pages: &[pdfcraft_render::PageInfo]) -> Vec { // The open rows, outermost first, with their depth and the children gathered so far. let mut open: Vec<(usize, Value, Vec)> = Vec::new(); diff --git a/crates/automation/tests/attachment_key.rs b/crates/automation/tests/attachment_key.rs new file mode 100644 index 0000000000..25b0d6c602 --- /dev/null +++ b/crates/automation/tests/attachment_key.rs @@ -0,0 +1,34 @@ +//! `attachment_add` reports the name-tree key of the file it added, even when a file attached to +//! a page as a comment already has the same name. + +use pdfcraft_automation::{Automation, Content}; +use pdfcraft_engine::{AttachIcon, Edit, NewAnnotation, Session, Shape, Style}; +use serde_json::{Value, json}; + +fn tool(a: &mut Automation, name: &str, args: Value) -> Value { + match a.call(name, &args).unwrap().remove(0) { + Content::Json(value) => value, + other => panic!("expected JSON: {other:?}"), + } +} + +#[test] +fn attachment_add_reports_its_own_key_when_a_page_comment_has_the_same_file_name() { + let dir = tempfile::tempdir().unwrap(); + let mut session = Session::new(); + let bytes = session.create_blank(300.0, 400.0, 1).unwrap(); + let id = session.open("a.pdf", None, bytes, None).unwrap(); + let shape = Shape::Attachment { at: [20.0, 300.0], icon: AttachIcon::Paperclip, file: "notes.txt".into(), data: b"on the page".to_vec() }; + session + .apply(id, Edit::AddAnnotation(NewAnnotation { page: 0, shape, style: Style::default(), contents: String::new(), author: "Ada".into() })) + .unwrap(); + std::fs::write(dir.path().join("a.pdf"), session.save_bytes(id).unwrap().as_slice()).unwrap(); + std::fs::write(dir.path().join("notes.txt"), b"for the document").unwrap(); + + let mut auto = Automation::new().with_root(dir.path()).unwrap(); + let doc = tool(&mut auto, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap(); + let added = tool(&mut auto, "attachment_add", json!({ "doc": doc, "path": "notes.txt" })); + assert_eq!(added["name"], "notes.txt", "{added}"); + let again = tool(&mut auto, "attachment_add", json!({ "doc": doc, "path": "notes.txt" })); + assert_eq!(again["name"], "notes.txt (2)", "{again}"); +} diff --git a/crates/organize/src/attachments.rs b/crates/organize/src/attachments.rs index d785f1a4db..3705ad9ed3 100644 --- a/crates/organize/src/attachments.rs +++ b/crates/organize/src/attachments.rs @@ -103,9 +103,16 @@ fn clear_embedded_files(doc: &mut Document) -> Result<(), OrganizeError> { let names = doc.get(root).as_dict().and_then(|c| c.get(b"Names").cloned()); match names { Some(Object::Ref(r)) => { + let mut empty = false; doc.update_dict(r, |n| { n.remove(b"EmbeddedFiles"); + empty = n.is_empty(); })?; + if empty { + doc.update_dict(root, |c| { + c.remove(b"Names"); + })?; + } } Some(Object::Dict(mut n)) => { n.remove(b"EmbeddedFiles"); diff --git a/crates/organize/tests/attachment_names_cleanup.rs b/crates/organize/tests/attachment_names_cleanup.rs new file mode 100644 index 0000000000..3a5145e715 --- /dev/null +++ b/crates/organize/tests/attachment_names_cleanup.rs @@ -0,0 +1,48 @@ +//! Deleting the last document attachment when the catalog's `/Names` is an indirect dictionary +//! (as some producers write it) must not leave an empty `/Names` behind. + +use std::sync::Arc; + +use pdfcraft_cos::{Document, Object}; +use pdfcraft_organize::{add_attachment, attachment_keys, delete_attachment}; + +/// One page, and a catalog whose `/Names` is the indirect (empty) dictionary 4 0 R. +fn indirect_names_pdf() -> Vec { + let objs: [&[u8]; 4] = [ + b"<< /Type /Catalog /Pages 2 0 R /Names 4 0 R >>", + b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>", + b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 100 100] >>", + b"<< >>", + ]; + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n", i + 1).as_bytes()); + out.extend_from_slice(o); + out.extend_from_slice(b"\nendobj\n"); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + out +} + +#[test] +fn deleting_the_last_attachment_removes_an_indirect_empty_names_dictionary() { + let mut doc = Document::open(Arc::new(indirect_names_pdf())).unwrap(); + let key = add_attachment(&mut doc, "notes.txt", b"hello", None, None, None).unwrap(); + assert_eq!(attachment_keys(&doc), ["notes.txt"]); + delete_attachment(&mut doc, &key).unwrap(); + assert!(attachment_keys(&doc).is_empty()); + let root = doc.root().unwrap(); + let catalog = doc.get(root).as_dict().cloned().unwrap(); + assert!( + catalog.get(b"Names").is_none(), + "the catalog keeps no empty /Names: {:?}", + catalog.get(b"Names").map(|n| matches!(n, Object::Ref(_))) + ); +}