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