From 801976a574e3143a7dc1b3d60db2b0c173557ee0 Mon Sep 17 00:00:00 2001 From: KenM76 Date: Wed, 4 Nov 2026 02:47:50 -0500 Subject: [PATCH 1/3] feat(organize): export, import and copy bookmark trees (#6018) bookmark_export returns an outline, or one bookmark's subtree, as JSON: titles, nesting, open state, colour, bold/italic and destinations as page plus view in PDF user space (ISO 32000-2 12.3.2.2), optionally written to a file. bookmark_import takes it back, inline or from that file, under any bookmark at any position or in place of the outline, as one undoable edit; bookmark_copy does the same from another open document by page number. A bookmark whose page the target lacks keeps its place without a destination and is reported in `unmapped`. Every node is checked before anything is written (title, page, colour, view type and arity, 33 levels, 100,000 bookmarks), and the error names the node. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SqmHYx8FiWM5PcsoiErgp9 --- crates/automation/src/bookmark_io.rs | 188 +++++++++++++++++ crates/automation/src/dispatch.rs | 3 + crates/automation/src/lib.rs | 1 + crates/automation/src/tools.rs | 43 ++++ crates/automation/tests/it/automation.rs | 92 +++++++++ crates/engine/src/edit.rs | 9 + crates/engine/src/edit/labels.rs | 2 + crates/engine/src/edit/run.rs | 4 + crates/engine/src/lib.rs | 5 +- crates/engine/src/session/view.rs | 8 + crates/engine/src/tests.rs | 32 +++ crates/organize/src/lib.rs | 2 + crates/organize/src/outline.rs | 16 +- crates/organize/src/outline_io.rs | 253 +++++++++++++++++++++++ crates/organize/src/tests.rs | 157 ++++++++++++++ parity/acrobat-features.toml | 12 ++ 16 files changed, 822 insertions(+), 5 deletions(-) create mode 100644 crates/automation/src/bookmark_io.rs create mode 100644 crates/organize/src/outline_io.rs diff --git a/crates/automation/src/bookmark_io.rs b/crates/automation/src/bookmark_io.rs new file mode 100644 index 0000000000..84f3c61abc --- /dev/null +++ b/crates/automation/src/bookmark_io.rs @@ -0,0 +1,188 @@ +//! Bookmark trees in and out of documents: `bookmark_export`, `bookmark_import`, `bookmark_copy`. +//! +//! The portable form is JSON: `{"bookmarks": [node, …]}`, where a node is `title`, `page` +//! (1-based, or `null` for one that goes nowhere), `view` (`{"type", "values"}`: a destination +//! type and its values in PDF user space, as the file stores them), `color`, `bold`, `italic`, +//! `open` and `children`. What export writes, import takes back unchanged. +use crate::{Args, Automation, Result, ToolError, failed}; +use pdfcraft_engine::{DocId, Edit, OutlineNode}; +use serde_json::{Value, json}; + +/// Deepest nesting a bookmark tree may have (the listing reaches 33 levels). +const MAX_DEPTH: usize = 33; + +impl Automation { + pub(crate) fn bookmark_export(&self, a: &Args) -> Result { + let id = self.doc(a)?.id; + let path = a.opt_path("path")?.unwrap_or_default(); + let tree = self.outline_of(id, &path)?; + let mut out = json!({ "bookmarks": tree.nodes.iter().map(node_json).collect::>(), "truncated": tree.truncated }); + if let Some(file) = a.opt_str("out")? { + let target = self.resolve(file, true)?; + let text = serde_json::to_vec_pretty(&json!({ "bookmarks": out["bookmarks"] })).map_err(failed)?; + crate::write_atomic(&target, &text)?; + out["path"] = json!(target); + } + Ok(out) + } + + pub(crate) fn bookmark_import(&mut self, a: &Args) -> Result { + let nodes = match (a.get("bookmarks"), a.opt_str("file")?) { + (Some(list), None) => nodes_of(list)?, + (None, Some(file)) => { + let path = self.resolve(file, false)?; + let value: Value = serde_json::from_slice(&crate::read_input(&path)?).map_err(|e| failed(format!("not a bookmark file: {e}")))?; + nodes_of(value.get("bookmarks").ok_or_else(|| failed("not a bookmark file: it has no \"bookmarks\" list"))?)? + } + _ => return Err(ToolError::InvalidArgs("give either bookmarks or file".into())), + }; + self.write_tree(a, nodes) + } + + pub(crate) fn bookmark_copy(&mut self, a: &Args) -> Result { + let from = a.int("from")?; + let from = DocId(u64::try_from(from).map_err(|_| ToolError::InvalidArgs("from must be positive".into()))?); + let path = a.opt_path("path")?.unwrap_or_default(); + let tree = self.outline_of(from, &path)?; + self.write_tree(a, tree.nodes) + } + + fn outline_of(&self, id: DocId, path: &[usize]) -> Result { + match self.session.outline_tree(id, path) { + Some(tree) => tree.map_err(failed), + None => Err(failed(format!("document {} is not open, or its bookmarks can't be read for editing", id.0))), + } + } + + /// Write `nodes` into `doc` under `parent` at `position` (or in place of the outline with + /// `replace`), and say which of them went nowhere because the document lacks their page. + fn write_tree(&mut self, a: &Args, bookmarks: Vec) -> Result { + let parent = a.opt_path("parent")?.unwrap_or_default(); + let index = a.opt_int("position")?.map_or(usize::MAX, |p| usize::try_from(p.max(1) - 1).unwrap_or(usize::MAX)); + let replace = a.opt_bool("replace")?.unwrap_or(false); + let count = self.doc(a)?.info.pages.len(); + let unmapped = pdfcraft_engine::outline_unmapped(&bookmarks, count); + let mut out = self.apply(a, Edit::ImportBookmarks { parent, index, bookmarks, replace })?; + out["unmapped"] = json!(unmapped.iter().map(|p| one_based(p)).collect::>()); + Ok(out) + } +} + +fn one_based(path: &[usize]) -> Vec { + path.iter().map(|i| i.saturating_add(1)).collect() +} + +/// A node as JSON, leaving out what is at its default (no view, black, regular, no children). +fn node_json(n: &OutlineNode) -> Value { + let mut v = json!({ "title": n.title, "page": n.page.map(|p| p.saturating_add(1)), "open": n.open }); + if let Some((kind, values)) = &n.view { + v["view"] = json!({ "type": String::from_utf8_lossy(kind), "values": values }); + } + if let Some(c) = n.color { + v["color"] = json!(c); + } + if n.bold { + v["bold"] = json!(true); + } + if n.italic { + v["italic"] = json!(true); + } + if !n.children.is_empty() { + v["children"] = json!(n.children.iter().map(node_json).collect::>()); + } + v +} + +fn nodes_of(list: &Value) -> Result> { + let mut count = 0; + list_of(list, &[], &mut count) +} + +/// The nodes of one level; `at` is the 1-based path of their parent, for messages. +fn list_of(list: &Value, at: &[usize], count: &mut usize) -> Result> { + let items = list.as_array().ok_or_else(|| bad(at, "children must be a list of bookmarks"))?; + if at.len() >= MAX_DEPTH && !items.is_empty() { + return Err(bad(at, &format!("bookmarks may nest at most {MAX_DEPTH} levels deep"))); + } + *count = count.saturating_add(items.len()); + if *count > pdfcraft_engine::MAX_IMPORT_BOOKMARKS { + return Err(ToolError::InvalidArgs(format!("at most {} bookmarks can be imported at once", pdfcraft_engine::MAX_IMPORT_BOOKMARKS))); + } + let mut out = Vec::with_capacity(items.len()); + for (i, item) in items.iter().enumerate() { + let mut path = at.to_vec(); + path.push(i.saturating_add(1)); + out.push(node_of(item, &path, count)?); + } + Ok(out) +} + +/// One node and, through [`list_of`], its children (recursion bounded by `MAX_DEPTH`). +fn node_of(v: &Value, at: &[usize], count: &mut usize) -> Result { + let o = v.as_object().ok_or_else(|| bad(at, "must be an object"))?; + let field = |k: &str| o.get(k).filter(|x| !x.is_null()); + let title = field("title").and_then(Value::as_str).filter(|t| !t.trim().is_empty()).ok_or_else(|| bad(at, "needs a title"))?.to_string(); + let page = match field("page") { + None => None, + Some(p) => Some(p.as_u64().filter(|p| *p >= 1).and_then(|p| usize::try_from(p - 1).ok()).ok_or_else(|| bad(at, "page must be 1 or more"))?), + }; + let flag = |k: &str| field(k).map_or(Ok(None), |x| x.as_bool().map(Some).ok_or_else(|| bad(at, &format!("{k} must be true or false")))); + let color = match field("color") { + None => None, + Some(c) => { + let rgb: Option> = c.as_array().and_then(|c| c.iter().map(|x| x.as_f64().map(|x| x as f32)).collect()); + match rgb.as_deref() { + Some(&[r, g, b]) if [r, g, b].iter().all(|c| (0.0..=1.0).contains(c)) => Some([r, g, b]), + _ => return Err(bad(at, "color must be [r, g, b], each from 0 to 1")), + } + } + }; + let children = match field("children") { + None => Vec::new(), + Some(list) => list_of(list, at, count)?, + }; + Ok(OutlineNode { + title, + page, + view: field("view").map(|x| view_of(x, at)).transpose()?, + color, + italic: flag("italic")?.unwrap_or(false), + bold: flag("bold")?.unwrap_or(false), + open: flag("open")?.unwrap_or(true), + children, + }) +} + +/// `{"type": "XYZ", "values": [left, top, zoom]}`: a destination type with exactly its values. +fn view_of(v: &Value, at: &[usize]) -> Result<(Vec, Vec>)> { + let kind = v.get("type").and_then(Value::as_str).ok_or_else(|| bad(at, "view needs a type such as XYZ or Fit"))?; + let values = match v.get("values").filter(|x| !x.is_null()) { + None => Vec::new(), + Some(list) => { + let list = list.as_array().filter(|l| l.len() <= 4).ok_or_else(|| bad(at, "view values must be a list of at most 4 numbers or nulls"))?; + list.iter() + .map(|x| match x { + Value::Null => Ok(None), + x => x.as_f64().map(Some).ok_or_else(|| bad(at, "view values must be numbers or null")), + }) + .collect::>()? + } + }; + let arity = pdfcraft_engine::outline_view_arity(kind.as_bytes()).ok_or_else(|| bad(at, VIEW_TYPES))?; + if values.len() != arity { + return Err(bad(at, &format!("a {kind} view takes {arity} values (numbers, or null to keep the reader's)"))); + } + if kind == "FitR" && values.iter().any(Option::is_none) { + return Err(bad(at, "a FitR view needs all four values: left, bottom, right, top")); + } + Ok((kind.as_bytes().to_vec(), values)) +} + +const VIEW_TYPES: &str = "view type must be XYZ (3 values: left, top, zoom), Fit, FitB (none), FitH, FitV, FitBH, FitBV (1) or FitR (4)"; + +fn bad(at: &[usize], what: &str) -> ToolError { + if at.is_empty() { + return ToolError::InvalidArgs(format!("bookmarks: {what}")); + } + ToolError::InvalidArgs(format!("bookmark {at:?}: {what}")) +} diff --git a/crates/automation/src/dispatch.rs b/crates/automation/src/dispatch.rs index f3c51d22e0..4106ef8bdb 100644 --- a/crates/automation/src/dispatch.rs +++ b/crates/automation/src/dispatch.rs @@ -196,6 +196,9 @@ impl Automation { out } "bookmark_list" => self.bookmark_listing(&a)?, + "bookmark_export" => self.bookmark_export(&a)?, + "bookmark_import" => self.bookmark_import(&a)?, + "bookmark_copy" => self.bookmark_copy(&a)?, "bookmark_add" => { let page = self.page(&a)?; let parent = a.opt_path("parent")?.unwrap_or_default(); diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 5bcb032fce..62e33a9d04 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -50,6 +50,7 @@ use file_access::foreign_share; use text_cache::TextCache; mod assemble; +mod bookmark_io; mod dispatch; mod documents; mod error; diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 4672e49886..374e2e704b 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -323,6 +323,49 @@ pub fn tools() -> Vec { }), &["doc", "title", "page"], )), + t( + "bookmark_export", + "Export bookmarks", + "The bookmark tree, or the subtree at path, as a list of nodes bookmark_import takes back unchanged. A node has title, page (1-based, null when it goes nowhere), open, and when set view ({type, values}: a destination type from ISO 32000-2 §12.3.2.2 Table 149 and its values in PDF user space, as the file stores them), color ([r, g, b], 0-1), bold, italic and children. truncated is true when bookmarks nested deeper than 33 levels were left out. With out, the tree is also written there as a JSON file {\"bookmarks\": [...]}.", + ) + .with(schema( + json!({ "doc": doc(), "path": path("The bookmark whose subtree to export (omit for the whole outline)"), "out": path_arg() }), + &["doc"], + )), + t( + "bookmark_import", + "Import bookmarks", + "Add a tree of bookmarks, as bookmark_export gives it (bookmarks) or as a JSON file it wrote (file), under a parent bookmark (or at the top level) at a position, or with replace in place of the whole outline. A bookmark whose page this document lacks is kept without a destination and listed in unmapped (1-based paths into the given tree). A view takes exactly its type's values (XYZ 3, FitR 4, FitH/FitV/FitBH/FitBV 1, Fit/FitB none). Give exactly one of bookmarks and file. Every node is checked before anything is written: at most 100000 bookmarks, nested at most 33 levels counting the parent's. Undoable.", + ) + .destructive() + .with(schema( + json!({ + "doc": doc(), + "bookmarks": { "type": "array", "items": { "type": "object" }, "minItems": 1, "description": "Nodes as bookmark_export returns them." }, + "file": path_arg(), + "parent": path("Parent bookmark (omit for the top level)"), + "position": { "type": "integer", "minimum": 1, "description": "1-based position among the parent's children (default: last)." }, + "replace": { "type": "boolean", "description": "Replace the document's bookmarks instead of adding to them (default false; leave parent out)." } + }), + &["doc"], + )), + t( + "bookmark_copy", + "Copy bookmarks from another document", + "Copy the bookmarks of document from (or the subtree at path) into doc, by page number: under a parent bookmark (or at the top level) at a position, or with replace in place of doc's outline. A bookmark whose page doc lacks is kept without a destination and listed in unmapped (1-based paths into the copied tree). Undoable.", + ) + .destructive() + .with(schema( + json!({ + "doc": doc(), + "from": { "type": "integer", "minimum": 1, "description": "Id of the document to copy from." }, + "path": path("The bookmark whose subtree to copy (omit for the whole outline)"), + "parent": path("Parent bookmark in doc (omit for the top level)"), + "position": { "type": "integer", "minimum": 1, "description": "1-based position among the parent's children (default: last)." }, + "replace": { "type": "boolean", "description": "Replace doc's bookmarks instead of adding to them (default false; leave parent out)." } + }), + &["doc", "from"], + )), t("bookmark_rename", "Rename a bookmark", "Change a bookmark's title. Undoable.") .with(schema(json!({ "doc": doc(), "path": path("The bookmark"), "title": { "type": "string", "minLength": 1 } }), &["doc", "path", "title"])), t("bookmark_delete", "Delete a bookmark", "Delete a bookmark and the bookmarks under it. Undoable.") diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 13f9af4371..6e9d2148ae 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -10642,3 +10642,95 @@ fn multiline_fill_sign_engine_round_trip_preserves_lines_and_appearance() { assert_eq!(rendered, re.call("page_render", &json!({"doc":reopened,"page":1,"dpi":72})).unwrap(), "saved appearances are identical"); std::fs::remove_dir_all(dir).unwrap(); } + +/// Export, then import into another document and copy between open documents: the tree comes +/// back unchanged, a page the target lacks is reported, and the import undoes in one step. +#[test] +fn bookmarks_export_import_and_copy_between_documents() { + let dir = workdir("bookmark-io"); + let mut a = auto(&dir); + let src = ok(&mut a, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap(); + ok(&mut a, "bookmark_add", json!({ "doc": src, "title": "Intro", "page": 1 })); + ok(&mut a, "bookmark_add", json!({ "doc": src, "title": "Detail", "page": 3, "parent": [1] })); + ok(&mut a, "bookmark_add", json!({ "doc": src, "title": "End", "page": 2 })); + ok(&mut a, "bookmark_set_style", json!({ "doc": src, "path": [1], "color": [1, 0, 0], "bold": true })); + let exported = ok(&mut a, "bookmark_export", json!({ "doc": src, "out": "marks.json" })); + assert_eq!(exported["truncated"], false); + let tree = exported["bookmarks"].clone(); + assert_eq!((tree[0]["title"].clone(), tree[0]["color"].clone(), tree[0]["bold"].clone()), (json!("Intro"), json!([1.0, 0.0, 0.0]), json!(true))); + assert_eq!((tree[0]["children"][0]["title"].clone(), tree[0]["children"][0]["page"].clone()), (json!("Detail"), json!(3))); + assert_eq!(tree[0]["view"]["type"], "XYZ"); + let subtree = ok(&mut a, "bookmark_export", json!({ "doc": src, "path": [1, 1] }))["bookmarks"].clone(); + assert_eq!(subtree, json!([tree[0]["children"][0]])); + + // b.pdf has two pages: Detail goes to the third, so it arrives without a destination. + let dst = ok(&mut a, "doc_open", json!({ "path": "b.pdf" }))["doc"].as_u64().unwrap(); + let imported = ok(&mut a, "bookmark_import", json!({ "doc": dst, "file": "marks.json" })); + assert_eq!(imported["unmapped"], json!([[1, 1]])); + let back = ok(&mut a, "bookmark_export", json!({ "doc": dst }))["bookmarks"].clone(); + assert_eq!(back[0]["children"][0]["page"], json!(null)); + assert_eq!((back[0]["title"].clone(), back[1].clone()), (tree[0]["title"].clone(), tree[1].clone())); + ok(&mut a, "edit_undo", json!({ "doc": dst })); + assert_eq!(ok(&mut a, "bookmark_list", json!({ "doc": dst }))["bookmarks"], json!([])); + + // Into a document with every page, the tree is unchanged, wherever it is copied from. + let same = ok(&mut a, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap(); + ok(&mut a, "bookmark_add", json!({ "doc": same, "title": "Old", "page": 1 })); + let copied = ok(&mut a, "bookmark_copy", json!({ "doc": same, "from": src, "replace": true })); + assert_eq!(copied["unmapped"], json!([])); + assert_eq!(ok(&mut a, "bookmark_export", json!({ "doc": same }))["bookmarks"], tree); + ok(&mut a, "bookmark_import", json!({ "doc": same, "bookmarks": subtree, "parent": [2], "position": 1 })); + assert_eq!(ok(&mut a, "bookmark_list", json!({ "doc": same }))["bookmarks"][1]["children"][0]["title"], "Detail"); +} + +#[test] +fn a_bad_bookmark_tree_is_refused_whole_and_names_the_node() { + let dir = workdir("bookmark-io-bad"); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap(); + let refused = |a: &mut Automation, bookmarks: Value| match a.call("bookmark_import", &json!({ "doc": doc, "bookmarks": bookmarks })) { + Err(ToolError::InvalidArgs(m)) => m, + other => panic!("expected InvalidArgs, got {other:?}"), + }; + let fine = json!({ "title": "Fine", "page": 1 }); + let m = refused(&mut a, json!([fine, { "title": "Parent", "children": [{ "page": 1 }] }])); + assert!(m.contains("[2, 1]") && m.contains("title"), "{m}"); + let m = refused(&mut a, json!([{ "title": " " }])); + assert!(m.contains("[1]") && m.contains("title"), "{m}"); + let m = refused(&mut a, json!([fine, { "title": "Bad", "page": 0 }])); + assert!(m.contains("[2]") && m.contains("page"), "{m}"); + let m = refused(&mut a, json!([{ "title": "Bad", "color": [2, 0, 0] }])); + assert!(m.contains("color"), "{m}"); + let m = refused(&mut a, json!([{ "title": "Bad", "view": { "type": "XYZ", "values": [1, 2, 3, 4, 5] } }])); + assert!(m.contains("view"), "{m}"); + let mut deep = json!({ "title": "Leaf" }); + for _ in 0..40 { + deep = json!({ "title": "Level", "children": [deep] }); + } + assert!(refused(&mut a, json!([deep])).contains("33 levels")); + let m = refused(&mut a, json!([fine, { "title": "Z", "page": 1, "view": { "type": "Zoom" } }])); + assert!(m.contains("[2]") && m.contains("XYZ (3 values"), "{m}"); + let m = refused(&mut a, json!([fine, { "title": "Z", "page": 1, "view": { "type": "FitH", "values": [1, 2] } }])); + assert!(m.contains("[2]") && m.contains("FitH view takes 1"), "{m}"); + let m = refused(&mut a, json!([{ "title": "R", "view": { "type": "FitR", "values": [0, null, 1, 1] } }])); + assert!(m.contains("all four"), "{m}"); + assert!(matches!(a.call("bookmark_import", &json!({ "doc": doc })), Err(ToolError::InvalidArgs(_)))); + assert!(matches!(a.call("bookmark_import", &json!({ "doc": doc, "bookmarks": [fine], "file": "x.json" })), Err(ToolError::InvalidArgs(_)))); + // A real bookmark file outside the root is refused for being outside, not for being missing. + let outside = dir.with_extension("outside.json"); + std::fs::write(&outside, br#"{"bookmarks": [{"title": "Outside", "page": 1}]}"#).unwrap(); + let name = format!("../{}", outside.file_name().unwrap().to_string_lossy()); + match a.call("bookmark_import", &json!({ "doc": doc, "file": name })) { + Err(ToolError::Failed(m)) => assert!(m.contains("outside the allowed directory"), "{m}"), + other => panic!("expected the root to refuse it, got {other:?}"), + } + let _ = std::fs::remove_file(&outside); + let out = dir.with_extension("out.json"); + let name = format!("../{}", out.file_name().unwrap().to_string_lossy()); + match a.call("bookmark_export", &json!({ "doc": doc, "out": name })) { + Err(ToolError::Failed(m)) => assert!(m.contains("outside the allowed directory"), "{m}"), + other => panic!("expected the root to refuse it, got {other:?}"), + } + assert!(!out.exists()); + assert_eq!(ok(&mut a, "bookmark_list", json!({ "doc": doc }))["bookmarks"], json!([])); +} diff --git a/crates/engine/src/edit.rs b/crates/engine/src/edit.rs index 8f43689aa9..759fe89638 100644 --- a/crates/engine/src/edit.rs +++ b/crates/engine/src/edit.rs @@ -119,6 +119,15 @@ pub enum Edit { bold: Option, italic: Option, }, + /// Write `bookmarks` as children of `parent` from child `index` on, or with `replace` in + /// place of the whole outline (`parent` must then be `[]`). A bookmark whose page this + /// document lacks is kept without a destination (see [`pdfcraft_organize::import_outline`]). + ImportBookmarks { + parent: Vec, + index: usize, + bookmarks: Vec, + replace: bool, + }, /// Bookmarks for the tagged headings (H, H1–H6), nested by level under a new first /// "Untitled" bookmark (New Bookmarks from Structure). BookmarksFromStructure, diff --git a/crates/engine/src/edit/labels.rs b/crates/engine/src/edit/labels.rs index 5343804ef3..fd246b3eac 100644 --- a/crates/engine/src/edit/labels.rs +++ b/crates/engine/src/edit/labels.rs @@ -38,6 +38,7 @@ pub const EDIT_LABELS: &[&str] = &[ "Set bookmark destination", "Colour", "Style", + "Import bookmarks", "New bookmarks from structure", "New bookmarks from text styles", "Number pages", @@ -193,6 +194,7 @@ impl Edit { // The bookmark Style and Colour menus' own names, already said in every language. Edit::SetBookmarkColor { .. } => Label::of("Colour"), Edit::SetBookmarkStyle { .. } => Label::of("Style"), + Edit::ImportBookmarks { .. } => Label::of("Import bookmarks"), Edit::BookmarksFromStructure => Label::of("New bookmarks from structure"), Edit::BookmarksFromTextStyles { .. } => Label::of("New bookmarks from text styles"), Edit::NumberPages { .. } => Label::of("Number pages"), diff --git a/crates/engine/src/edit/run.rs b/crates/engine/src/edit/run.rs index 5ed70aed7a..6fc751e90a 100644 --- a/crates/engine/src/edit/run.rs +++ b/crates/engine/src/edit/run.rs @@ -27,6 +27,7 @@ pub(crate) fn check_permission(edit: &Edit, p: &pdfcraft_cos::Permissions) -> Re | Edit::SetBookmarkPage { .. } | Edit::SetBookmarkColor { .. } | Edit::SetBookmarkStyle { .. } + | Edit::ImportBookmarks { .. } | Edit::BookmarksFromStructure | Edit::NumberPages { .. } => { if p.assemble() { @@ -454,6 +455,9 @@ pub(crate) fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut E Edit::SetBookmarkPage { path, page } => pdfcraft_organize::set_bookmark_page(doc, path, *page)?, Edit::SetBookmarkColor { path, color } => pdfcraft_organize::set_bookmark_color(doc, path, *color)?, Edit::SetBookmarkStyle { path, bold, italic } => pdfcraft_organize::set_bookmark_style(doc, path, *bold, *italic)?, + Edit::ImportBookmarks { parent, index, bookmarks, replace } => { + pdfcraft_organize::import_outline(doc, parent, *index, bookmarks, *replace)?; + } Edit::BookmarksFromStructure => { let entries: Vec<_> = pdfcraft_a11y::headings(doc) .into_iter() diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index fe139164be..97ac16746a 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -71,7 +71,10 @@ pub use signature_scan::{PhotoRect, ScanError, SignaturePhoto, Turn}; pub use stamp_preview::{CommentLayer, StampPreview}; pub use text_preview::{TextEditPreview, UnmarkedPages}; -pub use pdfcraft_organize::{BoxSpec, FieldConflicts, PageBox, SplitBy, Transition, TransitionSpec, mime_for_name, split_ranges}; +pub use pdfcraft_organize::{ + BoxSpec, FieldConflicts, MAX_IMPORT_BOOKMARKS, OutlineNode, OutlineTree, PageBox, SplitBy, Transition, TransitionSpec, mime_for_name, + split_ranges, unmapped as outline_unmapped, view_arity as outline_view_arity, +}; pub use pdfcraft_organize::LabelStyle; pub use pdfcraft_organize::Standard; diff --git a/crates/engine/src/session/view.rs b/crates/engine/src/session/view.rs index c0e7417c8e..5ef135d6c8 100644 --- a/crates/engine/src/session/view.rs +++ b/crates/engine/src/session/view.rs @@ -168,6 +168,14 @@ impl Session { Some(BookmarkPage { rows, next: (end < all.len()).then_some(end), truncated: doc.info.outline_more }) } + /// The bookmark at `path` with everything under it, or the whole outline for `[]`, as a tree + /// another document can take ([`crate::Edit::ImportBookmarks`]). `None` when no document has + /// `id`, or its structure could not be read for editing. + pub fn outline_tree(&self, id: DocId, path: &[usize]) -> Option> { + let editor = self.get(id)?.editor.as_ref()?; + Some(pdfcraft_organize::outline_tree(&editor.cos, path)) + } + /// Top-level bookmarks as split points: (first page of each part, its bookmark's title). pub fn bookmark_splits(&self, id: DocId) -> Vec<(usize, String)> { let Some(doc) = self.get(id) else { return Vec::new() }; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index a77e471a7d..ccffb39c61 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -10268,3 +10268,35 @@ fn fill_sign_multiline_text_survives_engine_save_reopen_and_render() { assert_eq!(reopened.get(again).unwrap().info.annotations[0].contents.as_deref(), Some(text), "reopening preserves all blank lines"); assert_eq!(before.rgba, render_page(&reopened, again).rgba, "the saved appearance is identical"); } + +/// A whole tree goes in as one undoable step, and a refused tree changes nothing. +#[test] +fn imported_bookmarks_undo_in_one_step() { + let (mut s, id) = session_with(2); + let node = |title: &str, page, children| pdfcraft_organize::OutlineNode { + title: title.into(), + page, + view: None, + color: None, + italic: false, + bold: false, + open: true, + children, + }; + let bookmarks = vec![node("Intro", Some(0), vec![node("Detail", Some(5), vec![])]), node("End", Some(1), vec![])]; + s.apply(id, Edit::ImportBookmarks { parent: vec![], index: 0, bookmarks: bookmarks.clone(), replace: false }).unwrap(); + assert_eq!(outline_titles(&s.get(id).unwrap().info.outline), ["Intro→1[Detail→0]", "End→2"]); + assert_eq!(s.get(id).unwrap().can_undo(), Some("Import bookmarks")); + assert_eq!(s.outline_tree(id, &[]).unwrap().unwrap().nodes[0].title, "Intro"); + s.undo(id).unwrap(); + assert!(s.get(id).unwrap().info.outline.is_empty()); + s.apply(id, Edit::ImportBookmarks { parent: vec![], index: 0, bookmarks: bookmarks.clone(), replace: false }).unwrap(); + s.apply(id, Edit::ImportBookmarks { parent: vec![], index: 0, bookmarks: vec![node("New", Some(1), vec![])], replace: true }).unwrap(); + assert_eq!(outline_titles(&s.get(id).unwrap().info.outline), ["New→2"]); + s.undo(id).unwrap(); + assert_eq!(outline_titles(&s.get(id).unwrap().info.outline), ["Intro→1[Detail→0]", "End→2"]); + s.undo(id).unwrap(); + let err = s.apply(id, Edit::ImportBookmarks { parent: vec![4], index: 0, bookmarks, replace: false }).unwrap_err(); + assert!(err.to_string().contains("no bookmark"), "{err}"); + assert!(s.get(id).unwrap().info.outline.is_empty()); +} diff --git a/crates/organize/src/lib.rs b/crates/organize/src/lib.rs index b890965d0c..3b3d56cd83 100644 --- a/crates/organize/src/lib.rs +++ b/crates/organize/src/lib.rs @@ -20,6 +20,7 @@ mod dedupe; mod import; mod labels; mod outline; +mod outline_io; mod pdfx; mod prune; mod transition; @@ -40,6 +41,7 @@ pub use outline::{ Bookmark, BookmarkPage, OutlineEntry, OutlineError, add_bookmark, add_bookmark_at, add_bookmark_tree, bookmark_page, bookmarks, delete_bookmark, move_bookmark, rename_bookmark, set_bookmark_color, set_bookmark_open, set_bookmark_page, set_bookmark_style, top_level_bookmarks, }; +pub use outline_io::{MAX_IMPORT_BOOKMARKS, OutlineNode, OutlineTree, import_outline, outline_tree, unmapped, view_arity}; pub use transition::{TRANSITION_DIRECTIONS, TRANSITION_STYLES, Transition, TransitionSpec, set_transition}; pub use view::{InitialView, displays_doc_title, initial_view, set_initial_view}; diff --git a/crates/organize/src/outline.rs b/crates/organize/src/outline.rs index ff7d3d8da7..13c02eb662 100644 --- a/crates/organize/src/outline.rs +++ b/crates/organize/src/outline.rs @@ -19,7 +19,7 @@ use crate::{OrganizeError, walk}; /// Deepest level a listing enters (the top level is 0). Bookmarks deeper than this are not /// listed; edits still reach them by path. -const LIST_DEPTH: usize = 32; +pub(crate) const LIST_DEPTH: usize = 32; /// Deepest name-tree node read when looking up named destinations. const NAME_TREE_DEPTH: usize = 32; @@ -72,6 +72,14 @@ pub enum OutlineError { BadColor, #[error("this document has no tagged headings to make bookmarks from")] NoEntries, + #[error("there are no bookmarks to add")] + NothingToImport, + #[error("the bookmark at {0:?} has a view that is not a known destination type with its values")] + BadView(Vec), + #[error("bookmarks may nest at most {0} levels deep")] + TooDeep(usize), + #[error("at most {0} bookmarks can be added at once")] + TooManyBookmarks(usize), #[error("{0}")] Organize(#[from] OrganizeError), #[error("{0}")] @@ -449,7 +457,7 @@ impl From for OrganizeError { /// Make `kids` the children of `parent`, in order: rewires `/Parent`, `/Prev`, `/Next`, /// `/First` and `/Last`. -fn relink(doc: &mut Document, parent: ObjRef, kids: &[ObjRef]) -> Result<()> { +pub(crate) fn relink(doc: &mut Document, parent: ObjRef, kids: &[ObjRef]) -> Result<()> { for (i, k) in kids.iter().enumerate() { put(doc, *k, b"Parent", Some(Object::Ref(parent)))?; put(doc, *k, b"Prev", i.checked_sub(1).map(|p| Object::Ref(kids[p])))?; @@ -473,7 +481,7 @@ struct Counting { /// Recompute every `/Count` (§12.3.3: open items count their visible descendants, closed items /// the negative of what opening them would show; the root counts all visible items). -fn recount(doc: &mut Document) -> Result<()> { +pub(crate) fn recount(doc: &mut Document) -> Result<()> { let Some(root) = outline_root(doc) else { return Ok(()) }; let mut seen = HashSet::from([root]); let kids = children_of(doc, root, &mut seen); @@ -714,7 +722,7 @@ pub fn move_bookmark(doc: &mut Document, from: &[usize], to_parent: &[usize], in /// Open `parent` so a bookmark just added or moved into it shows (the root is always open). /// Only the sign of `/Count` matters here: [`recount`] sets its value. -fn show_children(doc: &mut Document, parent: ObjRef, parent_path: &[usize]) -> Result<()> { +pub(crate) fn show_children(doc: &mut Document, parent: ObjRef, parent_path: &[usize]) -> Result<()> { if !parent_path.is_empty() && doc.get(parent).as_dict().and_then(|d| d.int(b"Count")).is_none_or(|c| c <= 0) { put(doc, parent, b"Count", Some(Object::Int(1)))?; } diff --git a/crates/organize/src/outline_io.rs b/crates/organize/src/outline_io.rs new file mode 100644 index 0000000000..053be74672 --- /dev/null +++ b/crates/organize/src/outline_io.rs @@ -0,0 +1,253 @@ +//! Moving a whole outline (or one bookmark's subtree) out of a document and into another +//! (ISO 32000-2 §12.3.3 outlines, §12.3.2.2 explicit destinations). +//! +//! [`outline_tree`] reads bookmarks as a portable tree of [`OutlineNode`]s: titles, nesting, open +//! state, colour, style and destinations as page index plus view in the file's own user space, +//! so writing a tree back with [`import_outline`] gives the same structure. A node whose page the +//! target lacks keeps its place in the tree without a destination, and [`unmapped`] names it, so +//! nothing is dropped silently. Not carried: actions other than a GoTo, named destinations (they +//! travel resolved to their page), and structure links (`/SE`). + +use pdfcraft_cos::{Dict, Document, ObjRef, Object, PdfString}; + +use crate::outline::{LIST_DEPTH, children, container, recount, relink, show_children}; +use crate::{OrganizeError, OutlineError, walk}; + +/// Most bookmarks one import writes. +pub const MAX_IMPORT_BOOKMARKS: usize = 100_000; + +/// The destination types (Table 149) and how many numbers follow each. +const VIEWS: [(&[u8], usize); 8] = [(b"XYZ", 3), (b"Fit", 0), (b"FitH", 1), (b"FitV", 1), (b"FitR", 4), (b"FitB", 0), (b"FitBH", 1), (b"FitBV", 1)]; + +/// One bookmark and the bookmarks under it, independent of any document. +#[derive(Clone, Debug, PartialEq)] +pub struct OutlineNode { + pub title: String, + /// The 0-based page it goes to; `None` for a bookmark that goes nowhere in its document. + pub page: Option, + /// Where on the page: a destination type from Table 149 and exactly as many values as that + /// type takes, in PDF user space (`None` keeps the reader's value). `None` goes to the page at + /// the reader's position and zoom. + pub view: Option<(Vec, Vec>)>, + /// The title's colour (`/C`), each component from 0 to 1; `None` is the default black. + pub color: Option<[f32; 3]>, + pub italic: bool, + pub bold: bool, + /// Shown expanded, when it has children. + pub open: bool, + pub children: Vec, +} + +/// The tree [`outline_tree`] read. +#[derive(Clone, Debug, PartialEq)] +pub struct OutlineTree { + pub nodes: Vec, + /// Bookmarks nested deeper than the listing reaches (32 levels) were left out. + pub truncated: bool, +} + +/// The bookmark at `path` with everything under it, or the whole outline for `[]`. +pub fn outline_tree(doc: &Document, path: &[usize]) -> Result { + // A whole outline's top level sits at depth 0; a subtree's root is the bookmark at `path`. + let base = path.len().max(1); + let mut truncated = false; + // The open nodes, outermost first, each with its depth. + let mut open: Vec<(usize, OutlineNode)> = Vec::new(); + let mut roots = Vec::new(); + for b in crate::bookmarks(doc).into_iter().filter(|b| b.path.starts_with(path) && !b.path.is_empty()) { + let depth = b.path.len().saturating_sub(base); + if b.path.len() > LIST_DEPTH && has_children(doc, b.obj) { + truncated = true; + } + while open.last().is_some_and(|(d, _)| *d >= depth) { + close(&mut open, &mut roots); + } + let view = b.view.and_then(|(kind, values)| portable_view(kind, values)); + let node = + OutlineNode { title: b.title, page: b.page, view, color: b.color, italic: b.italic, bold: b.bold, open: b.open, children: Vec::new() }; + open.push((depth, node)); + } + while !open.is_empty() { + close(&mut open, &mut roots); + } + if !path.is_empty() && roots.is_empty() { + return Err(OutlineError::NoSuchBookmark(path.to_vec())); + } + Ok(OutlineTree { nodes: roots, truncated }) +} + +fn close(open: &mut Vec<(usize, OutlineNode)>, roots: &mut Vec) { + if let Some((_, node)) = open.pop() { + match open.last_mut() { + Some((_, parent)) => parent.children.push(node), + None => roots.push(node), + } + } +} + +fn has_children(doc: &Document, r: ObjRef) -> bool { + doc.get(r).as_dict().is_some_and(|d| d.reference(b"First").is_some()) +} + +/// A destination's view as [`import_outline`] takes it: a known type with exactly its number of +/// values (extra ones dropped, missing ones `None`). An unknown type gives `None`. +fn portable_view(kind: Vec, mut values: Vec>) -> Option<(Vec, Vec>)> { + let (_, arity) = VIEWS.iter().find(|(k, _)| *k == kind.as_slice())?; + values.resize(*arity, None); + Some((kind, values)) +} + +/// The nodes of `nodes` whose page `page_count` pages don't have, as paths into `nodes` +/// (0-based child indices), in tree order. [`import_outline`] keeps these without a destination. +pub fn unmapped(nodes: &[OutlineNode], page_count: usize) -> Vec> { + let mut out = Vec::new(); + visit(nodes, |path, node| { + if node.page.is_some_and(|p| p >= page_count) { + out.push(path.to_vec()); + } + }); + out +} + +/// How many values a view of destination type `kind` takes (Table 149), or `None` for a type +/// that is not one of them. +pub fn view_arity(kind: &[u8]) -> Option { + VIEWS.iter().find(|(k, _)| *k == kind).map(|(_, arity)| *arity) +} + +/// Call `f` with each node's path and the node, parents first. Iterative. +fn visit<'a>(nodes: &'a [OutlineNode], mut f: impl FnMut(&[usize], &'a OutlineNode)) { + // One entry per open level: its nodes and how many of them have been visited. + let mut stack: Vec<(&'a [OutlineNode], usize)> = vec![(nodes, 0)]; + while let Some(level) = stack.last_mut() { + let list = level.0; + let Some(node) = list.get(level.1) else { + stack.pop(); + continue; + }; + level.1 = level.1.saturating_add(1); + let path: Vec = stack.iter().map(|(_, visited)| visited.saturating_sub(1)).collect(); + f(&path, node); + if !node.children.is_empty() { + stack.push((&node.children, 0)); + } + } +} + +/// Check every node before anything is written: depth (counting the `base` levels above the +/// tree), count, colour and view. +fn check(nodes: &[OutlineNode], base: usize) -> Result<(), OutlineError> { + let mut problem = None; + let mut count = 0usize; + visit(nodes, |path, node| { + count = count.saturating_add(1); + if problem.is_some() { + return; + } + if base.saturating_add(path.len()) > LIST_DEPTH.saturating_add(1) { + problem = Some(OutlineError::TooDeep(LIST_DEPTH.saturating_add(1))); + } else if node.color.is_some_and(|c| !c.iter().all(|v| (0.0..=1.0).contains(v))) { + problem = Some(OutlineError::BadColor); + } else if node.view.as_ref().is_some_and(|v| !view_is_valid(v)) { + problem = Some(OutlineError::BadView(path.to_vec())); + } + }); + if count > MAX_IMPORT_BOOKMARKS { + return Err(OutlineError::TooManyBookmarks(MAX_IMPORT_BOOKMARKS)); + } + problem.map_or(Ok(()), Err) +} + +/// A known type with its number of values, each finite; `/FitR` needs all four. +fn view_is_valid((kind, values): &(Vec, Vec>)) -> bool { + VIEWS.iter().any(|(k, arity)| *k == kind.as_slice() && *arity == values.len()) + && values.iter().flatten().all(|v| v.is_finite()) + && (kind.as_slice() != b"FitR" || values.iter().all(Option::is_some)) +} + +/// Write `nodes` as children of the bookmark at `parent_path` (`[]` = top level), starting at +/// child `index` (past the end appends), or, with `replace`, in place of the outline's +/// top-level bookmarks (then `parent_path` must be `[]`). Nodes whose page this document lacks +/// are written without a destination (see [`unmapped`]). Everything is checked before anything +/// is written. Returns the path of the first node written. +pub fn import_outline( + doc: &mut Document, + parent_path: &[usize], + index: usize, + nodes: &[OutlineNode], + replace: bool, +) -> Result, OutlineError> { + if nodes.is_empty() { + return Err(OutlineError::NothingToImport); + } + if replace && !parent_path.is_empty() { + return Err(OrganizeError::Invalid("replace applies to the whole outline: leave the parent out".into()).into()); + } + check(nodes, parent_path.len())?; + let pages: Vec = walk(doc)?.into_iter().map(|(r, _)| r).collect(); + let parent = container(doc, parent_path)?; + let top = write_nodes(doc, &pages, nodes)?; + let mut kids = if replace { Vec::new() } else { children(doc, parent) }; + let at = index.min(kids.len()); + kids.splice(at..at, top); + relink(doc, parent, &kids)?; + show_children(doc, parent, parent_path)?; + recount(doc)?; + let mut path = parent_path.to_vec(); + path.push(at); + Ok(path) +} + +/// Add one item per node, linked under each other, and return the top-level items in order. +fn write_nodes(doc: &mut Document, pages: &[ObjRef], nodes: &[OutlineNode]) -> Result, OutlineError> { + // Each written item with its node, then its children's items once they are written. + let mut written: Vec<(ObjRef, Vec)> = Vec::new(); + let mut top = Vec::new(); + // (the nodes of one level, the next to write, the index in `written` of their parent) + let mut stack: Vec<(&[OutlineNode], usize, Option)> = vec![(nodes, 0, None)]; + while let Some(level) = stack.last_mut() { + let (list, parent) = (level.0, level.2); + let Some(node) = list.get(level.1) else { + stack.pop(); + continue; + }; + level.1 = level.1.saturating_add(1); + let item = doc.add(Object::Dict(item_dict(node, pages))); + match parent.and_then(|p| written.get_mut(p)) { + Some((_, kids)) => kids.push(item), + None => top.push(item), + } + written.push((item, Vec::new())); + if !node.children.is_empty() { + stack.push((&node.children, 0, Some(written.len().saturating_sub(1)))); + } + } + for (item, kids) in &written { + relink(doc, *item, kids)?; + } + Ok(top) +} + +/// The item's own entries; the links and `/Count`'s value come from [`relink`] and [`recount`] +/// (only the sign of the `/Count` written here matters: it keeps the open state). +fn item_dict(node: &OutlineNode, pages: &[ObjRef]) -> Dict { + let mut d = Dict::new(); + d.set(b"Title".to_vec(), Object::String(PdfString::text(&node.title))); + if let Some(page) = node.page.and_then(|p| pages.get(p)) { + let (kind, values) = node.view.clone().unwrap_or_else(|| (b"XYZ".to_vec(), vec![None; 3])); + let mut dest = vec![Object::Ref(*page), Object::Name(kind)]; + dest.extend(values.into_iter().map(|v| v.map_or(Object::Null, Object::Real))); + d.set(b"Dest".to_vec(), Object::Array(dest)); + } + if let Some(c) = node.color.filter(|c| *c != [0.0; 3]) { + d.set(b"C".to_vec(), Object::Array(c.iter().map(|v| Object::Real(f64::from(*v))).collect())); + } + let flags = i64::from(node.italic) | i64::from(node.bold) << 1; + if flags != 0 { + d.set(b"F".to_vec(), Object::Int(flags)); + } + if !node.children.is_empty() { + d.set(b"Count".to_vec(), Object::Int(if node.open { 1 } else { -1 })); + } + d +} diff --git a/crates/organize/src/tests.rs b/crates/organize/src/tests.rs index 33dc262fce..fe5981be8c 100644 --- a/crates/organize/src/tests.rs +++ b/crates/organize/src/tests.rs @@ -3813,3 +3813,160 @@ fn contents_pieces_join_with_a_space_across_a_trailing_comment() { ctrl_names.sort(); assert_eq!(ctrl_names, [b"ImHidden".to_vec(), b"ImShown".to_vec()]); } + +/// Intro (closed, red bold, at a point) > [Part (italic, FitH), Figure (FitR)], then Results. +fn outline_source() -> Document { + let mut d = doc_a(); + crate::add_bookmark_at(&mut d, &[], 0, "Intro", 0, Some(20.0), Some(380.0)).unwrap(); + crate::add_bookmark(&mut d, &[0], 0, "Part", 1).unwrap(); + crate::add_bookmark(&mut d, &[0], 1, "Figure", 2).unwrap(); + crate::add_bookmark(&mut d, &[], 1, "Results", 2).unwrap(); + crate::set_bookmark_color(&mut d, &[0], Some([1.0, 0.0, 0.0])).unwrap(); + crate::set_bookmark_style(&mut d, &[0], Some(true), None).unwrap(); + crate::set_bookmark_style(&mut d, &[0, 0], None, Some(true)).unwrap(); + let pages = crate::walk(&d).unwrap(); + let (part, figure) = (crate::bookmarks(&d)[1].obj, crate::bookmarks(&d)[2].obj); + let dest = |p: usize, rest: Vec| Object::Array([vec![Object::Ref(pages[p].0)], rest].concat()); + d.update_dict(part, |x| x.set(b"Dest".to_vec(), dest(1, vec![Object::name("FitH"), Object::Real(300.0)]))).unwrap(); + let fit_r = vec![Object::name("FitR"), Object::Int(10), Object::Int(20), Object::Int(110), Object::Int(220)]; + d.update_dict(figure, |x| x.set(b"Dest".to_vec(), dest(2, fit_r))).unwrap(); + crate::set_bookmark_open(&mut d, &[0], false).unwrap(); + d +} + +fn node(title: &str, page: Option, children: Vec) -> crate::OutlineNode { + crate::OutlineNode { title: title.into(), page, view: None, color: None, italic: false, bold: false, open: true, children } +} + +/// The `/Prev`, `/Last` and `/Parent` links agree with `/First`–`/Next` at every level. +fn assert_back_links(doc: &Document) { + let root = doc.get(doc.root().unwrap()).as_dict().unwrap().reference(b"Outlines").unwrap(); + let mut stack = vec![root]; + while let Some(parent) = stack.pop() { + let kids = crate::outline::children(doc, parent); + let d = doc.get(parent).as_dict().cloned().unwrap(); + assert_eq!(d.reference(b"Last"), kids.last().copied()); + for (i, k) in kids.iter().enumerate() { + let kd = doc.get(*k).as_dict().cloned().unwrap(); + assert_eq!(kd.reference(b"Prev"), i.checked_sub(1).map(|p| kids[p])); + assert_eq!(kd.reference(b"Parent"), Some(parent)); + } + stack.extend(kids); + } +} + +#[test] +fn an_exported_outline_imports_into_another_document_unchanged() { + let source = outline_source(); + let tree = crate::outline_tree(&source, &[]).unwrap(); + assert!(!tree.truncated); + let intro = &tree.nodes[0]; + assert_eq!((intro.title.as_str(), intro.page, intro.open, intro.bold, intro.color), ("Intro", Some(0), false, true, Some([1.0, 0.0, 0.0]))); + assert_eq!(intro.view, Some((b"XYZ".to_vec(), vec![Some(20.0), Some(380.0), None]))); + assert_eq!(intro.children[0].view, Some((b"FitH".to_vec(), vec![Some(300.0)]))); + assert!(intro.children[0].italic); + assert_eq!(intro.children[1].view, Some((b"FitR".to_vec(), vec![Some(10.0), Some(20.0), Some(110.0), Some(220.0)]))); + + let mut target = doc_a(); + assert_eq!(crate::import_outline(&mut target, &[], 0, &tree.nodes, false).unwrap(), vec![0]); + assert_back_links(&target); + let back = full_roundtrip(&target); + assert_eq!(crate::outline_tree(&back, &[]).unwrap(), tree); + // The closed parent stays closed: its count is negative and the root counts only what shows. + let first = crate::bookmarks(&back)[0].obj; + let root = back.get(back.root().unwrap()).as_dict().unwrap().reference(b"Outlines").unwrap(); + assert_eq!((count_of(&back, first), count_of(&back, root)), (Some(-2), Some(2))); +} + +#[test] +fn a_subtree_exports_alone_and_a_missing_path_is_refused() { + let source = outline_source(); + let tree = crate::outline_tree(&source, &[0]).unwrap(); + assert_eq!(tree.nodes.len(), 1); + assert_eq!(tree.nodes[0].title, "Intro"); + assert_eq!(tree.nodes[0].children.iter().map(|n| n.title.as_str()).collect::>(), ["Part", "Figure"]); + let leaf = crate::outline_tree(&source, &[0, 1]).unwrap(); + assert_eq!((leaf.nodes.len(), leaf.nodes[0].title.as_str()), (1, "Figure")); + assert_eq!(crate::outline_tree(&source, &[5]), Err(crate::OutlineError::NoSuchBookmark(vec![5]))); + assert_eq!(crate::outline_tree(&doc_a(), &[]).unwrap().nodes, []); +} + +#[test] +fn a_page_the_target_lacks_keeps_its_bookmark_without_a_destination() { + let tree = crate::outline_tree(&outline_source(), &[]).unwrap(); + // doc_b has two pages: Figure and Results go to the third. + assert_eq!(crate::unmapped(&tree.nodes, 2), [vec![0, 1], vec![1]]); + let mut target = doc_b(); + crate::import_outline(&mut target, &[], 0, &tree.nodes, false).unwrap(); + let listed = crate::bookmarks(&target); + assert_eq!(titles(&listed), ["Intro[Part,Figure]", "Results"]); + assert_eq!(listed.iter().map(|b| b.page).collect::>(), [Some(0), Some(1), None, None]); + assert!(target.get(listed[2].obj).as_dict().unwrap().get(b"Dest").is_none()); +} + +#[test] +fn an_import_appends_inserts_or_replaces() { + let mut d = doc_a(); + crate::add_bookmark(&mut d, &[], 0, "Old A", 0).unwrap(); + crate::add_bookmark(&mut d, &[], 1, "Old B", 1).unwrap(); + let new = [node("New", Some(2), vec![node("Child", Some(1), vec![])])]; + assert_eq!(crate::import_outline(&mut d, &[], 1, &new, false).unwrap(), vec![1]); + assert_eq!(titles(&crate::bookmarks(&d)), ["Old A", "New[Child]", "Old B"]); + assert_eq!(crate::import_outline(&mut d, &[2], 9, &new, false).unwrap(), vec![2, 0]); + assert_eq!(titles(&crate::bookmarks(&d)), ["Old A", "New[Child]", "Old B[New[Child]]"]); + assert_back_links(&d); + assert_eq!(crate::import_outline(&mut d, &[], 5, &new, true).unwrap(), vec![0]); + assert_eq!(titles(&crate::bookmarks(&d)), ["New[Child]"]); + assert_back_links(&d); + let refused = crate::import_outline(&mut d, &[0], 0, &new, true); + assert!(matches!(refused, Err(crate::OutlineError::Organize(crate::OrganizeError::Invalid(_))))); + assert_eq!(crate::import_outline(&mut d, &[], 0, &[], false), Err(crate::OutlineError::NothingToImport)); + assert_eq!(crate::import_outline(&mut d, &[7], 0, &new, false), Err(crate::OutlineError::NoSuchBookmark(vec![7]))); +} + +#[test] +fn a_bad_node_anywhere_refuses_the_whole_import() { + let mut d = doc_a(); + crate::add_bookmark(&mut d, &[], 0, "Kept", 0).unwrap(); + let before = crate::bookmarks(&d); + let with = |f: &dyn Fn(&mut crate::OutlineNode)| { + let mut child = node("Child", Some(0), vec![]); + f(&mut child); + vec![node("Fine", Some(0), vec![]), node("Parent", Some(0), vec![child])] + }; + let bad_views: [(&[u8], Vec>); 4] = + [(b"XYZ", vec![Some(1.0)]), (b"Zoom", vec![]), (b"FitH", vec![Some(f64::NAN)]), (b"FitR", vec![Some(0.0), None, Some(1.0), Some(1.0)])]; + for (kind, values) in bad_views { + let nodes = with(&|n| n.view = Some((kind.to_vec(), values.clone()))); + assert_eq!(crate::import_outline(&mut d, &[], 0, &nodes, false), Err(crate::OutlineError::BadView(vec![1, 0])), "{kind:?}"); + } + let nodes = with(&|n| n.color = Some([0.5, 1.5, 0.0])); + assert_eq!(crate::import_outline(&mut d, &[], 0, &nodes, false), Err(crate::OutlineError::BadColor)); + let nodes = with(&|n| n.color = Some([f32::NAN, 0.0, 0.0])); + assert_eq!(crate::import_outline(&mut d, &[], 0, &nodes, false), Err(crate::OutlineError::BadColor)); + let mut deep = node("Leaf", None, vec![]); + for _ in 0..33 { + deep = node("Level", None, vec![deep]); + } + assert_eq!(crate::import_outline(&mut d, &[], 0, &[deep], false), Err(crate::OutlineError::TooDeep(33))); + // 33 levels fit at the top, but not under a bookmark: the deepest would be past the listing. + let mut chain = node("Leaf", None, vec![]); + for _ in 0..32 { + chain = node("Level", None, vec![chain]); + } + assert_eq!(crate::import_outline(&mut d, &[0], 0, std::slice::from_ref(&chain), false), Err(crate::OutlineError::TooDeep(33))); + let many = vec![node("Item", None, vec![]); crate::MAX_IMPORT_BOOKMARKS + 1]; + assert_eq!(crate::import_outline(&mut d, &[], 0, &many, false), Err(crate::OutlineError::TooManyBookmarks(crate::MAX_IMPORT_BOOKMARKS))); + assert_eq!(crate::bookmarks(&d), before); +} + +#[test] +fn an_outline_deeper_than_the_listing_is_exported_as_truncated() { + let tree = crate::outline_tree(&open(deep_outline(40)), &[]).unwrap(); + assert!(tree.truncated); + assert!(!crate::outline_tree(&open(deep_outline(33)), &[]).unwrap().truncated); + // What was exported imports back: 33 levels is the deepest an import takes. + let mut target = doc_a(); + crate::import_outline(&mut target, &[], 0, &tree.nodes, false).unwrap(); + assert_eq!(crate::bookmarks(&target).len(), 33); +} diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 8b7ac909a0..909cec18da 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -3421,6 +3421,18 @@ tools = ["text_styles", "bookmark_from_text_styles"] evidence = ["crates/engine/src/tests.rs::bookmarks_from_text_styles_nest_the_headings_and_undo", "crates/engine/src/text_style_bookmarks.rs::subset_tags_are_left_out_of_the_font_name", "crates/engine/src/text_style_bookmarks.rs::styles_match_within_a_quarter_point", "crates/automation/tests/it/automation.rs::bookmarks_from_text_styles_through_tools", "crates/ui-egui/tests/it/bookmarks.rs::new_bookmarks_from_text_styles_from_the_bookmark_options"] notes = "Bookmark options (and Pages) ▸ New bookmarks from text styles…, for documents whose headings aren't tagged (#1693): the dialog lists the document's text styles (font without its subset tag, the size shown at, bold, italic, line count and a sample), the largest first and offered for level 1; one style per level, up to four levels (nine headlessly). Every line in a level's style becomes a bookmark of that level under a new first \"Untitled\" bookmark, a heading set on several rows is one bookmark, and each goes to the top of its heading (/XYZ). Lines are read down each page, then left to right, so a multi-column page's headings are taken row by row across the columns; sizes within a quarter point match. Not yet: several styles for one level, colour as part of a style, and the first 5000 pages only. Acrobat has no such command (its bookmarks come from structure); this is beyond it" +[[feature]] +id = "organize.bookmarks-import-export" +title = "Export, import and copy bookmarks" +area = "D" +tier = "P2" +milestone = "M4" +status = "partial" +commands = [] +tools = ["bookmark_export", "bookmark_import", "bookmark_copy"] +evidence = ["crates/organize/src/tests.rs::an_exported_outline_imports_into_another_document_unchanged", "crates/organize/src/tests.rs::a_page_the_target_lacks_keeps_its_bookmark_without_a_destination", "crates/organize/src/tests.rs::an_import_appends_inserts_or_replaces", "crates/organize/src/tests.rs::a_bad_node_anywhere_refuses_the_whole_import", "crates/automation/tests/it/automation.rs::bookmarks_export_import_and_copy_between_documents", "crates/automation/tests/it/automation.rs::a_bad_bookmark_tree_is_refused_whole_and_names_the_node"] +notes = "#6018, headless only. The bookmark tree (or one bookmark's subtree) goes out as JSON with titles, nesting, open state, colour, bold/italic and destinations as page plus view in PDF user space, and comes back unchanged: into another document, under any bookmark, at any position, or in place of its outline, as one undoable step; bookmark_copy does the same between open documents by page number. A bookmark whose page the target lacks keeps its place without a destination and is reported, never dropped. Not yet: a panel command and preview, CSV/Markdown/pdftk formats, remapping by named destination (named destinations travel resolved to their page), actions other than go-to-page, structure links (/SE)" + [[feature]] id = "organize.attachments-add" title = "Add file attachments" From 43939cc13f1112f09c76aad9eeb7e357ac0595fd Mon Sep 17 00:00:00 2001 From: KenM76 Date: Wed, 4 Nov 2026 04:39:20 -0500 Subject: [PATCH 2/3] M4: bookmark tree round-trips what real files hold; review fixes (#6018) - organize: a /FitR destination missing one of its four numbers is copied with its page and no view, instead of failing the whole copy (the rectangle can't be kept, and import refuses a partial one). - organize: `visit` keeps the visited node's path in step with its stack instead of rebuilding it per node (O(depth) per node before). - automation: an empty title is accepted when present, so a bookmark a file left untitled exports and imports back; a missing title is still refused. - automation: "no bookmark at" names the path 1-based, as it was given. - automation: `bookmark_copy` and `bookmark_import` report `truncated`; an exported file carries `"truncated": true` when the export left bookmarks out, and importing it says so. - automation: `bookmark_export` is marked destructive (it overwrites `out`) and idempotent for MCP. Tests: a_fit_r_missing_a_value_copies_with_its_page_and_no_view (organize), odd_bookmarks_round_trip_and_a_cut_tree_says_so (automation). Each fix reverted alone fails one of them; the visit change is behaviour-neutral and covered by the existing outline tests. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SqmHYx8FiWM5PcsoiErgp9 --- crates/automation/src/bookmark_io.rs | 33 +++++++++---- crates/automation/src/mcp.rs | 2 +- crates/automation/src/tools.rs | 7 +-- crates/automation/tests/it/automation.rs | 61 +++++++++++++++++++++++- crates/organize/src/outline_io.rs | 16 +++++-- crates/organize/src/tests.rs | 15 ++++++ 6 files changed, 116 insertions(+), 18 deletions(-) diff --git a/crates/automation/src/bookmark_io.rs b/crates/automation/src/bookmark_io.rs index 84f3c61abc..82c4927f62 100644 --- a/crates/automation/src/bookmark_io.rs +++ b/crates/automation/src/bookmark_io.rs @@ -3,7 +3,8 @@ //! The portable form is JSON: `{"bookmarks": [node, …]}`, where a node is `title`, `page` //! (1-based, or `null` for one that goes nowhere), `view` (`{"type", "values"}`: a destination //! type and its values in PDF user space, as the file stores them), `color`, `bold`, `italic`, -//! `open` and `children`. What export writes, import takes back unchanged. +//! `open` and `children`. What export writes, import takes back unchanged. A file also carries +//! `"truncated": true` when the export left out bookmarks nested past the listing limit. use crate::{Args, Automation, Result, ToolError, failed}; use pdfcraft_engine::{DocId, Edit, OutlineNode}; use serde_json::{Value, json}; @@ -19,7 +20,11 @@ impl Automation { let mut out = json!({ "bookmarks": tree.nodes.iter().map(node_json).collect::>(), "truncated": tree.truncated }); if let Some(file) = a.opt_str("out")? { let target = self.resolve(file, true)?; - let text = serde_json::to_vec_pretty(&json!({ "bookmarks": out["bookmarks"] })).map_err(failed)?; + let mut saved = json!({ "bookmarks": out["bookmarks"] }); + if tree.truncated { + saved["truncated"] = json!(true); + } + let text = serde_json::to_vec_pretty(&saved).map_err(failed)?; crate::write_atomic(&target, &text)?; out["path"] = json!(target); } @@ -27,16 +32,19 @@ impl Automation { } pub(crate) fn bookmark_import(&mut self, a: &Args) -> Result { - let nodes = match (a.get("bookmarks"), a.opt_str("file")?) { - (Some(list), None) => nodes_of(list)?, + let (nodes, truncated) = match (a.get("bookmarks"), a.opt_str("file")?) { + (Some(list), None) => (nodes_of(list)?, false), (None, Some(file)) => { let path = self.resolve(file, false)?; let value: Value = serde_json::from_slice(&crate::read_input(&path)?).map_err(|e| failed(format!("not a bookmark file: {e}")))?; - nodes_of(value.get("bookmarks").ok_or_else(|| failed("not a bookmark file: it has no \"bookmarks\" list"))?)? + let list = value.get("bookmarks").ok_or_else(|| failed("not a bookmark file: it has no \"bookmarks\" list"))?; + (nodes_of(list)?, value.get("truncated") == Some(&Value::Bool(true))) } _ => return Err(ToolError::InvalidArgs("give either bookmarks or file".into())), }; - self.write_tree(a, nodes) + let mut out = self.write_tree(a, nodes)?; + out["truncated"] = json!(truncated); + Ok(out) } pub(crate) fn bookmark_copy(&mut self, a: &Args) -> Result { @@ -44,12 +52,18 @@ impl Automation { let from = DocId(u64::try_from(from).map_err(|_| ToolError::InvalidArgs("from must be positive".into()))?); let path = a.opt_path("path")?.unwrap_or_default(); let tree = self.outline_of(from, &path)?; - self.write_tree(a, tree.nodes) + let mut out = self.write_tree(a, tree.nodes)?; + out["truncated"] = json!(tree.truncated); + Ok(out) } fn outline_of(&self, id: DocId, path: &[usize]) -> Result { match self.session.outline_tree(id, path) { - Some(tree) => tree.map_err(failed), + // The engine names the path 0-based; the caller gave it 1-based. + Some(tree) => tree.map_err(|e| match e { + pdfcraft_organize::OutlineError::NoSuchBookmark(_) => failed(format!("there is no bookmark at {:?}", one_based(path))), + e => failed(e), + }), None => Err(failed(format!("document {} is not open, or its bookmarks can't be read for editing", id.0))), } } @@ -121,7 +135,8 @@ fn list_of(list: &Value, at: &[usize], count: &mut usize) -> Result Result { let o = v.as_object().ok_or_else(|| bad(at, "must be an object"))?; let field = |k: &str| o.get(k).filter(|x| !x.is_null()); - let title = field("title").and_then(Value::as_str).filter(|t| !t.trim().is_empty()).ok_or_else(|| bad(at, "needs a title"))?.to_string(); + // An empty title is kept: files have them, and export writes them as "". + let title = field("title").and_then(Value::as_str).ok_or_else(|| bad(at, "needs a title"))?.to_string(); let page = match field("page") { None => None, Some(p) => Some(p.as_u64().filter(|p| *p >= 1).and_then(|p| usize::try_from(p - 1).ok()).ok_or_else(|| bad(at, "page must be 1 or more"))?), diff --git a/crates/automation/src/mcp.rs b/crates/automation/src/mcp.rs index 84cc21b258..b033871867 100644 --- a/crates/automation/src/mcp.rs +++ b/crates/automation/src/mcp.rs @@ -557,7 +557,7 @@ fn tool_json(t: &crate::ToolDef) -> Value { "title": t.title, "description": t.description, "inputSchema": t.input_schema, - "annotations": { "title": t.title, "readOnlyHint": t.read_only, "destructiveHint": t.destructive, "idempotentHint": t.read_only || matches!(t.name, "doc_save" | "doc_optimize" | "doc_export_data" | "measure_export" | "doc_export_images" | "doc_export_office" | "doc_export_all_images" | "doc_export_text" | "image_save"), "openWorldHint": false }, + "annotations": { "title": t.title, "readOnlyHint": t.read_only, "destructiveHint": t.destructive, "idempotentHint": t.read_only || matches!(t.name, "doc_save" | "doc_optimize" | "doc_export_data" | "measure_export" | "doc_export_images" | "doc_export_office" | "doc_export_all_images" | "doc_export_text" | "image_save" | "bookmark_export"), "openWorldHint": false }, }) } diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 374e2e704b..b25a653a83 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -326,8 +326,9 @@ pub fn tools() -> Vec { t( "bookmark_export", "Export bookmarks", - "The bookmark tree, or the subtree at path, as a list of nodes bookmark_import takes back unchanged. A node has title, page (1-based, null when it goes nowhere), open, and when set view ({type, values}: a destination type from ISO 32000-2 §12.3.2.2 Table 149 and its values in PDF user space, as the file stores them), color ([r, g, b], 0-1), bold, italic and children. truncated is true when bookmarks nested deeper than 33 levels were left out. With out, the tree is also written there as a JSON file {\"bookmarks\": [...]}.", + "The bookmark tree, or the subtree at path, as a list of nodes bookmark_import takes back unchanged. A node has title, page (1-based, null when it goes nowhere), open, and when set view ({type, values}: a destination type from ISO 32000-2 §12.3.2.2 Table 149 and its values in PDF user space, as the file stores them), color ([r, g, b], 0-1), bold, italic and children. truncated is true when bookmarks nested deeper than 33 levels were left out. With out, the tree is also written there as a JSON file {\"bookmarks\": [...]}, with \"truncated\": true when that is so; out is overwritten.", ) + .destructive() .with(schema( json!({ "doc": doc(), "path": path("The bookmark whose subtree to export (omit for the whole outline)"), "out": path_arg() }), &["doc"], @@ -335,7 +336,7 @@ pub fn tools() -> Vec { t( "bookmark_import", "Import bookmarks", - "Add a tree of bookmarks, as bookmark_export gives it (bookmarks) or as a JSON file it wrote (file), under a parent bookmark (or at the top level) at a position, or with replace in place of the whole outline. A bookmark whose page this document lacks is kept without a destination and listed in unmapped (1-based paths into the given tree). A view takes exactly its type's values (XYZ 3, FitR 4, FitH/FitV/FitBH/FitBV 1, Fit/FitB none). Give exactly one of bookmarks and file. Every node is checked before anything is written: at most 100000 bookmarks, nested at most 33 levels counting the parent's. Undoable.", + "Add a tree of bookmarks, as bookmark_export gives it (bookmarks) or as a JSON file it wrote (file), under a parent bookmark (or at the top level) at a position, or with replace in place of the whole outline. A bookmark whose page this document lacks is kept without a destination and listed in unmapped (1-based paths into the given tree). A view takes exactly its type's values (XYZ 3, FitR 4, FitH/FitV/FitBH/FitBV 1, Fit/FitB none). Give exactly one of bookmarks and file. A title may be empty but not missing. Every node is checked before anything is written: at most 100000 bookmarks, nested at most 33 levels counting the parent's. truncated is true when the file says its export left bookmarks out. Undoable.", ) .destructive() .with(schema( @@ -352,7 +353,7 @@ pub fn tools() -> Vec { t( "bookmark_copy", "Copy bookmarks from another document", - "Copy the bookmarks of document from (or the subtree at path) into doc, by page number: under a parent bookmark (or at the top level) at a position, or with replace in place of doc's outline. A bookmark whose page doc lacks is kept without a destination and listed in unmapped (1-based paths into the copied tree). Undoable.", + "Copy the bookmarks of document from (or the subtree at path) into doc, by page number: under a parent bookmark (or at the top level) at a position, or with replace in place of doc's outline. A bookmark whose page doc lacks is kept without a destination and listed in unmapped (1-based paths into the copied tree). truncated is true when bookmarks nested deeper than 33 levels were left out of the copy. Undoable.", ) .destructive() .with(schema( diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 6e9d2148ae..4a36e3630a 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -10683,6 +10683,65 @@ fn bookmarks_export_import_and_copy_between_documents() { assert_eq!(ok(&mut a, "bookmark_list", json!({ "doc": same }))["bookmarks"][1]["children"][0]["title"], "Detail"); } +/// A one-page document whose outline has an untitled item, a /FitR destination with three of +/// its four numbers, and a chain nested `deep` levels below them. +fn odd_outline(deep: usize) -> Vec { + let first_deep = 7; + let mut objs = vec![ + "<< /Type /Catalog /Pages 2 0 R /Outlines 4 0 R >>".to_string(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 300] >>".into(), + "<< /Type /Page /Parent 2 0 R >>".into(), + "<< /Type /Outlines /First 5 0 R /Last 6 0 R >>".into(), + "<< /Title () /Parent 4 0 R /Next 6 0 R /Dest [3 0 R /Fit] >>".into(), + format!( + "<< /Title (Short) /Parent 4 0 R /Prev 5 0 R /Dest [3 0 R /FitR 0 0 10]{} >>", + if deep > 0 { format!(" /First {first_deep} 0 R /Last {first_deep} 0 R") } else { String::new() } + ), + ]; + for i in 0..deep { + let (me, parent) = (first_deep + i, if i == 0 { 6 } else { first_deep + i - 1 }); + let child = if i + 1 < deep { format!(" /First {0} 0 R /Last {0} 0 R", me + 1) } else { String::new() }; + objs.push(format!("<< /Title (Level) /Parent {parent} 0 R /Dest [3 0 R /Fit]{child} >>")); + } + pdf_of(&objs.iter().map(String::as_str).collect::>()) +} + +/// What a real file holds and the JSON form can't say exactly still copies and round-trips: an +/// empty title stays empty, a short /FitR keeps its page and drops the view, and bookmarks past +/// the listing depth are reported as left out at every step. +#[test] +fn odd_bookmarks_round_trip_and_a_cut_tree_says_so() { + let dir = workdir("bookmark-io-odd"); + std::fs::write(dir.join("odd.pdf"), odd_outline(0)).unwrap(); + std::fs::write(dir.join("deep.pdf"), odd_outline(40)).unwrap(); + let mut a = auto(&dir); + let src = ok(&mut a, "doc_open", json!({ "path": "odd.pdf" }))["doc"].as_u64().unwrap(); + let tree = ok(&mut a, "bookmark_export", json!({ "doc": src, "out": "odd.json" }))["bookmarks"].clone(); + assert_eq!((tree[0]["title"].clone(), tree[1]["page"].clone(), tree[1].get("view")), (json!(""), json!(1), None)); + let dst = ok(&mut a, "doc_open", json!({ "path": "b.pdf" }))["doc"].as_u64().unwrap(); + assert_eq!(ok(&mut a, "bookmark_import", json!({ "doc": dst, "file": "odd.json", "replace": true }))["truncated"], false); + // A destination with no view is written as XYZ with nulls: the same page, the reader's view. + let back = ok(&mut a, "bookmark_export", json!({ "doc": dst }))["bookmarks"].clone(); + assert_eq!((back[0].clone(), back[1]["title"].clone(), back[1]["page"].clone()), (tree[0].clone(), json!("Short"), json!(1))); + assert_eq!(back[1]["view"], json!({ "type": "XYZ", "values": [null, null, null] })); + let copied = ok(&mut a, "bookmark_copy", json!({ "doc": dst, "from": src, "replace": true })); + assert_eq!((copied["unmapped"].clone(), copied["truncated"].clone()), (json!([]), json!(false))); + match a.call("bookmark_export", &json!({ "doc": src, "path": [3] })) { + Err(ToolError::Failed(m)) => assert!(m.contains("no bookmark at [3]"), "{m}"), + other => panic!("expected no bookmark, got {other:?}"), + } + + let deep = ok(&mut a, "doc_open", json!({ "path": "deep.pdf" }))["doc"].as_u64().unwrap(); + assert_eq!(ok(&mut a, "bookmark_export", json!({ "doc": deep, "out": "deep.json" }))["truncated"], true); + let saved: Value = serde_json::from_slice(&std::fs::read(dir.join("deep.json")).unwrap()).unwrap(); + assert_eq!(saved["truncated"], true); + assert_eq!(ok(&mut a, "bookmark_import", json!({ "doc": dst, "file": "deep.json", "replace": true }))["truncated"], true); + assert_eq!(ok(&mut a, "bookmark_copy", json!({ "doc": dst, "from": deep, "replace": true }))["truncated"], true); + let export = tools().into_iter().find(|t| t.name == "bookmark_export").unwrap(); + assert!(export.destructive && !export.read_only, "bookmark_export overwrites out"); + std::fs::remove_dir_all(dir).unwrap(); +} + #[test] fn a_bad_bookmark_tree_is_refused_whole_and_names_the_node() { let dir = workdir("bookmark-io-bad"); @@ -10695,7 +10754,7 @@ fn a_bad_bookmark_tree_is_refused_whole_and_names_the_node() { let fine = json!({ "title": "Fine", "page": 1 }); let m = refused(&mut a, json!([fine, { "title": "Parent", "children": [{ "page": 1 }] }])); assert!(m.contains("[2, 1]") && m.contains("title"), "{m}"); - let m = refused(&mut a, json!([{ "title": " " }])); + let m = refused(&mut a, json!([{ "title": 5 }])); assert!(m.contains("[1]") && m.contains("title"), "{m}"); let m = refused(&mut a, json!([fine, { "title": "Bad", "page": 0 }])); assert!(m.contains("[2]") && m.contains("page"), "{m}"); diff --git a/crates/organize/src/outline_io.rs b/crates/organize/src/outline_io.rs index 053be74672..1fc3afc4f3 100644 --- a/crates/organize/src/outline_io.rs +++ b/crates/organize/src/outline_io.rs @@ -90,10 +90,14 @@ fn has_children(doc: &Document, r: ObjRef) -> bool { } /// A destination's view as [`import_outline`] takes it: a known type with exactly its number of -/// values (extra ones dropped, missing ones `None`). An unknown type gives `None`. +/// values (extra ones dropped, missing ones `None`). An unknown type, or a FitR missing one of +/// its four values (the rectangle can't be kept), gives `None`. fn portable_view(kind: Vec, mut values: Vec>) -> Option<(Vec, Vec>)> { let (_, arity) = VIEWS.iter().find(|(k, _)| *k == kind.as_slice())?; values.resize(*arity, None); + if kind.as_slice() == b"FitR" && values.iter().any(Option::is_none) { + return None; + } Some((kind, values)) } @@ -119,14 +123,18 @@ pub fn view_arity(kind: &[u8]) -> Option { fn visit<'a>(nodes: &'a [OutlineNode], mut f: impl FnMut(&[usize], &'a OutlineNode)) { // One entry per open level: its nodes and how many of them have been visited. let mut stack: Vec<(&'a [OutlineNode], usize)> = vec![(nodes, 0)]; + // The visited node's path, kept in step with the stack rather than rebuilt per node. + let mut path: Vec = Vec::new(); while let Some(level) = stack.last_mut() { let list = level.0; - let Some(node) = list.get(level.1) else { + let index = level.1; + let Some(node) = list.get(index) else { stack.pop(); continue; }; - level.1 = level.1.saturating_add(1); - let path: Vec = stack.iter().map(|(_, visited)| visited.saturating_sub(1)).collect(); + level.1 = index.saturating_add(1); + path.truncate(stack.len().saturating_sub(1)); + path.push(index); f(&path, node); if !node.children.is_empty() { stack.push((&node.children, 0)); diff --git a/crates/organize/src/tests.rs b/crates/organize/src/tests.rs index fe5981be8c..d5ff2116ff 100644 --- a/crates/organize/src/tests.rs +++ b/crates/organize/src/tests.rs @@ -3904,6 +3904,21 @@ fn a_page_the_target_lacks_keeps_its_bookmark_without_a_destination() { assert!(target.get(listed[2].obj).as_dict().unwrap().get(b"Dest").is_none()); } +#[test] +fn a_fit_r_missing_a_value_copies_with_its_page_and_no_view() { + let mut src = outline_source(); + let pages = crate::walk(&src).unwrap(); + let figure = crate::bookmarks(&src)[2].obj; + let short = vec![Object::Ref(pages[2].0), Object::name("FitR"), Object::Int(10), Object::Int(20), Object::Int(110)]; + src.update_dict(figure, |x| x.set(b"Dest".to_vec(), Object::Array(short))).unwrap(); + let tree = crate::outline_tree(&src, &[]).unwrap(); + let copied = &tree.nodes[0].children[1]; + assert_eq!((copied.page, copied.view.clone()), (Some(2), None)); + let mut target = doc_a(); + crate::import_outline(&mut target, &[], 0, &tree.nodes, false).unwrap(); + assert_eq!(titles(&crate::bookmarks(&target)), ["Intro[Part,Figure]", "Results"]); +} + #[test] fn an_import_appends_inserts_or_replaces() { let mut d = doc_a(); From e79b5594557f6cc10d41874f5dbeb0e3e5431b79 Mon Sep 17 00:00:00 2001 From: KenM76 Date: Wed, 4 Nov 2026 07:10:55 -0500 Subject: [PATCH 3/3] automation: bookmark_import refuses a node key it does not know node_of read only the keys it knew and ignored the rest, so a misspelt key such as "colour" was dropped and the bookmark imported without its colour and without an error. A node may now carry only the eight keys bookmark_export writes (title, page, view, color, bold, italic, open, children); any other key refuses the whole import, naming the node's path and the key, before anything is written. The tool description says so. Test: a_misspelt_bookmark_key_is_refused_not_dropped puts "colour" on a nested node and checks the import is refused and the outline left empty; it fails with the check reverted. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SqmHYx8FiWM5PcsoiErgp9 --- crates/automation/src/bookmark_io.rs | 6 ++++++ crates/automation/src/tools.rs | 2 +- crates/automation/tests/it/automation.rs | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/crates/automation/src/bookmark_io.rs b/crates/automation/src/bookmark_io.rs index 82c4927f62..2a06d912f4 100644 --- a/crates/automation/src/bookmark_io.rs +++ b/crates/automation/src/bookmark_io.rs @@ -12,6 +12,9 @@ use serde_json::{Value, json}; /// Deepest nesting a bookmark tree may have (the listing reaches 33 levels). const MAX_DEPTH: usize = 33; +/// The keys a node may have: the ones [`node_json`] writes. +const NODE_KEYS: [&str; 8] = ["title", "page", "view", "color", "bold", "italic", "open", "children"]; + impl Automation { pub(crate) fn bookmark_export(&self, a: &Args) -> Result { let id = self.doc(a)?.id; @@ -134,6 +137,9 @@ fn list_of(list: &Value, at: &[usize], count: &mut usize) -> Result Result { let o = v.as_object().ok_or_else(|| bad(at, "must be an object"))?; + if let Some(k) = o.keys().find(|k| !NODE_KEYS.contains(&k.as_str())) { + return Err(bad(at, &format!("unknown key {k:?} (a bookmark has {})", NODE_KEYS.join(", ")))); + } let field = |k: &str| o.get(k).filter(|x| !x.is_null()); // An empty title is kept: files have them, and export writes them as "". let title = field("title").and_then(Value::as_str).ok_or_else(|| bad(at, "needs a title"))?.to_string(); diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index b25a653a83..553f79b12d 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -336,7 +336,7 @@ pub fn tools() -> Vec { t( "bookmark_import", "Import bookmarks", - "Add a tree of bookmarks, as bookmark_export gives it (bookmarks) or as a JSON file it wrote (file), under a parent bookmark (or at the top level) at a position, or with replace in place of the whole outline. A bookmark whose page this document lacks is kept without a destination and listed in unmapped (1-based paths into the given tree). A view takes exactly its type's values (XYZ 3, FitR 4, FitH/FitV/FitBH/FitBV 1, Fit/FitB none). Give exactly one of bookmarks and file. A title may be empty but not missing. Every node is checked before anything is written: at most 100000 bookmarks, nested at most 33 levels counting the parent's. truncated is true when the file says its export left bookmarks out. Undoable.", + "Add a tree of bookmarks, as bookmark_export gives it (bookmarks) or as a JSON file it wrote (file), under a parent bookmark (or at the top level) at a position, or with replace in place of the whole outline. A bookmark whose page this document lacks is kept without a destination and listed in unmapped (1-based paths into the given tree). A view takes exactly its type's values (XYZ 3, FitR 4, FitH/FitV/FitBH/FitBV 1, Fit/FitB none). Give exactly one of bookmarks and file. A title may be empty but not missing; a key a node does not have is refused. Every node is checked before anything is written: at most 100000 bookmarks, nested at most 33 levels counting the parent's. truncated is true when the file says its export left bookmarks out. Undoable.", ) .destructive() .with(schema( diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 4a36e3630a..2d6c92fdc1 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -10742,6 +10742,20 @@ fn odd_bookmarks_round_trip_and_a_cut_tree_says_so() { std::fs::remove_dir_all(dir).unwrap(); } +#[test] +fn a_misspelt_bookmark_key_is_refused_not_dropped() { + let dir = workdir("bookmark-io-key"); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap(); + let tree = json!([{ "title": "Parent", "children": [{ "title": "Red", "page": 1, "colour": [1, 0, 0] }] }]); + match a.call("bookmark_import", &json!({ "doc": doc, "bookmarks": tree })) { + Err(ToolError::InvalidArgs(m)) => assert!(m.contains("[1, 1]") && m.contains("\"colour\""), "{m}"), + other => panic!("expected InvalidArgs, got {other:?}"), + } + assert_eq!(ok(&mut a, "bookmark_list", json!({ "doc": doc }))["bookmarks"], json!([])); + std::fs::remove_dir_all(dir).unwrap(); +} + #[test] fn a_bad_bookmark_tree_is_refused_whole_and_names_the_node() { let dir = workdir("bookmark-io-bad");