From 8fce28258def76e0079848933dbc31c9665b6767 Mon Sep 17 00:00:00 2001 From: Hamza Naveed <45464935+Hamza0101@users.noreply.github.com> Date: Fri, 6 Nov 2026 14:01:02 +0500 Subject: [PATCH] IDML export: a document's hyperlinks are written out (#1446) (#1659) * IDML import: a page item's or a text run's hyperlink survives (#1446) IDML carries a hyperlink as three elements: a source, a destination and a `Hyperlink` joining them by `Self`. Only `CrossReferenceSource` was mapped, so an image frame linked to a photo and a run of text linked to a page were dropped on import, although both are native here and both export to PDF. The names are collected as the package is read and the two ends put together in `finish`, once the pages, page items and anchors they name exist. A link whose source or destination is not in the file is dropped with a warning saying which, rather than silently or as a link that goes nowhere. Carries all three review fixes: - a source recorded in a table cell's or a footnote's story is dropped. Those stories are built under the StoryId(0) placeholder, so the link named a story nothing can look up, with offsets into text that is not there. The warning says where the text was instead of claiming it was not text. - the same for a HyperlinkTextDestination in such a story: its anchor id is (0 << 24) | k, so it collides with other cells' anchors and belongs to no story the cross-reference index holds; the link resolved to Anchor(1) and the PDF left the annotation out without a word. - the index's topic cross-references go in at the start of the first story with text after the ranges are recorded, so the ranges for that story are shifted with the markers; a range left alone covered the wrong nine bytes. Rebased onto the post-split main, hunk by hunk: the Importer struct, its fields, the element scan, hyperlink_list and finish are in import.rs, while StoryBuilder, walk_story and the anchor list are in import/story.rs. finish answers (Document, Vec) so the warnings reach the caller. * An anchor a hyperlink cannot mean is refused, not ignored This branch carries its own copy of `dest_param`'s anchor arm -- the IDML import needed anchors as link destinations and added it here independently of #1555 -- and it has the same hole, so review is right that the fix has to be here as well rather than only on that PR. `dest_param` returns `Result>`. `None` still means no destination was given, which `hyperlink.edit` needs for an edit that only renames; an `anchor` that is given and names none is an error. An unknown *id* was accepted outright, since `as_u64` answers for any number, so a link was stored to an anchor nobody has and the export quietly left it out. On `hyperlink.edit` the `None` read as "no destination given", so the command reported success and kept the old destination, which is the case review reported. An unknown *name* did already fail on `hyperlink.create`, but with "give `url`, `email`, `page` or `anchor`" -- the wrong problem named. The same change is on #1555, deliberately: each PR introduces this code on its own line and each has to be right on its own. Whichever lands first, the other's conflict is the same text on both sides. * IDML export: a document's hyperlinks are written out (#1446) Import reads hyperlinks now, but export dropped them, so a round trip lost every link the document had. The three elements IDML wants are written: a destination, a source, and a `Hyperlink` joining them. A link on a run of text is a `HyperlinkTextSource` around the ranges it covers, and the element nests inside a `ParagraphStyleRange`, so a link whose text crosses a paragraph is written as one source per paragraph and each piece stays clickable. Consecutive ranges of one link share a single source element. Carries both review fixes: - a link to an anchor whose text has been deleted is not written. The anchor is resolved first, as the page destination's page already was, so the file cannot hold a `Hyperlink` whose `Destination` names no element. - every `Self` names one element. Three links on one frame wrote three `HyperlinkPageItemSource` elements all called i12, and each link to a page pushed another `HyperlinkPageDestination`; the item source carries the link's index now, and the page destination is written once per page as url_dest_el already did for URLs. The round-trip test collects every `Self` in the designmap and asserts none repeats, so an element added later is covered without anyone remembering to extend it. Rebased onto the post-split main, on top of the rebased #1652: the `Ex` state is in export.rs, the hyperlink elements are written in export/designmap.rs with url_dest_el, fnv and has_self beside their only caller, and the run cutting and wrapping is in export/story.rs. * Split the hyperlink and index-marker steps out of Importer::run, under clippy's complexity limit * Split the hyperlink and index-marker steps out of Importer::run, under clippy's complexity limit * Split the hyperlink steps out of story_para, under clippy's complexity limit * IDML import: say when a link's frame is anchored in text, which PDF export cannot place yet --------- Co-authored-by: skylar taylor-barrick --- crates/idml/src/export.rs | 5 + crates/idml/src/export/designmap.rs | 100 +++++++++++++++++++ crates/idml/src/export/story.rs | 77 ++++++++++++++- crates/idml/src/tests.rs | 147 ++++++++++++++++++++++++++++ 4 files changed, 328 insertions(+), 1 deletion(-) diff --git a/crates/idml/src/export.rs b/crates/idml/src/export.rs index 35aab7bf4..717bb5d6c 100644 --- a/crates/idml/src/export.rs +++ b/crates/idml/src/export.rs @@ -99,6 +99,10 @@ struct Ex<'a> { threads: HashMap, Option)>, /// Cross-reference sources written so far: (Self, target anchor). xref_sources: Vec<(String, u64)>, + /// `HyperlinkTextSource`s written into the stories: (Self, index in `Document::hyperlinks`). + /// A link whose text crosses a paragraph gets one source per paragraph, since the element + /// nests inside a `ParagraphStyleRange` (#1446). + link_text_sources: Vec<(String, usize)>, /// Index topic path → first serialized sort key; See / See also cross-references. index_topics: BTreeMap, String>, index_xrefs: Vec<(Vec, &'static str, String)>, @@ -114,6 +118,7 @@ impl<'a> Ex<'a> { fonts: BTreeMap::new(), threads: HashMap::new(), xref_sources: Vec::new(), + link_text_sources: Vec::new(), index_topics: BTreeMap::new(), index_xrefs: Vec::new(), }; diff --git a/crates/idml/src/export/designmap.rs b/crates/idml/src/export/designmap.rs index bc489a8a0..ee1060d7a 100644 --- a/crates/idml/src/export/designmap.rs +++ b/crates/idml/src/export/designmap.rs @@ -156,6 +156,78 @@ impl<'a> Ex<'a> { for (name, _) in stories { root.push(El::new("idPkg:Story").attr("src", name)); } + // The document's own hyperlinks (#1446): a destination element, a source element for a + // page item (a text source is already in its story) and a `Hyperlink` joining them. + // Stories are written before this, so `link_text_sources` is complete. + let text_sources: Vec<(String, usize)> = std::mem::take(&mut self.link_text_sources); + for (i, h) in d.hyperlinks.iter().enumerate() { + use designcraft_doc::{HyperlinkDest as De, HyperlinkSource as So}; + // A text destination is already written where its anchor sits. + let dest = match &h.dest { + // Resolved first, as the page branch resolves its page: an anchor whose text was + // deleted names no element, so the file would hold a `Hyperlink` whose + // `Destination` points at nothing -- import only warns and InDesign may refuse + // the file (noted in review). + De::Anchor(a) => d.find_anchor(*a).map(|_| anchor_self(*a)), + De::Url(u) => Some(self.url_dest_el(&mut root, u, &h.name)), + De::Email(e) => Some(self.url_dest_el(&mut root, &format!("mailto:{e}"), &h.name)), + De::Page(p) => d.page(*p).map(|pg| { + // One element per page, as `url_dest_el` writes one per URL: every link to + // the same page pushed another `HyperlinkPageDestination` with the same + // `Self`, and IDML's references rely on a `Self` naming one element (noted + // in review). + let me = format!("HyperlinkPageDestination/p{}", pg.id.0); + if !has_self(&root, "HyperlinkPageDestination", &me) { + root.push( + El::new("HyperlinkPageDestination") + .attr("Self", &me) + .attr("Name", &h.name) + .attr("DestinationPage", uid(pg.id.0)) + .attr("ViewSetting", "FitPage") + .attr("Hidden", "false"), + ); + } + me + }), + }; + let Some(dest) = dest else { continue }; + // One `Hyperlink` per source element: a link whose text crosses a paragraph has one + // source per paragraph, so each piece stays clickable. + let sources: Vec = match &h.source { + So::Item { id } if d.item(*id).is_some() => { + // The link's index as well as the item's: InDesign joins one source to one + // destination, so three links on one frame are three source elements, and + // naming them all `i{item}` gave three elements one `Self` (noted in review). + let me = format!("HyperlinkPageItemSource/i{}k{i}", id.0); + root.push( + El::new("HyperlinkPageItemSource") + .attr("Self", &me) + .attr("Name", &h.name) + .attr("SourcePageItem", uid(id.0)) + .attr("Hidden", "false"), + ); + vec![me] + } + So::Item { .. } => vec![], + So::Text { .. } => text_sources.iter().filter(|(_, k)| *k == i).map(|(me, _)| me.clone()).collect(), + }; + for src in sources { + let mut el = El::new("Hyperlink") + .attr("Self", self.fresh()) + .attr("Name", &h.name) + .attr("Source", src) + .attr("Visible", "true") + .attr("Highlight", "None") + .attr("Width", "Thin") + .attr("BorderStyle", "Solid") + .attr("Hidden", "false") + .attr("DestinationUniqueKey", h.id & 0xFFFF_FFFF); + let mut props = El::new("Properties"); + props.push(p("Destination", "object", &dest)); + el.push(props); + root.push(el); + } + } for (src, target) in self.xref_sources.clone() { let mut h = El::new("Hyperlink") .attr("Self", self.fresh()) @@ -358,6 +430,34 @@ fn text_variable_el(v: &designcraft_doc::vars::TextVariable) -> El { e } +impl Ex<'_> { + /// A `HyperlinkURLDestination` for `url`, written once however many links lead to it. + fn url_dest_el(&mut self, root: &mut El, url: &str, name: &str) -> String { + let me = format!("HyperlinkURLDestination/u{:x}", fnv(url)); + if !has_self(root, "HyperlinkURLDestination", &me) { + root.push( + El::new("HyperlinkURLDestination") + .attr("Self", &me) + .attr("Name", if name.is_empty() { url } else { name }) + .attr("DestinationURL", url) + .attr("Hidden", "false"), + ); + } + me + } +} + +/// A short stable name for a string, so one URL gives one destination element. +fn fnv(s: &str) -> u64 { + s.bytes().fold(0xcbf2_9ce4_8422_2325u64, |h, b| (h ^ u64::from(b)).wrapping_mul(0x100_0000_01b3)) +} + +/// Whether `root` already holds an element of this name with this `Self`. IDML references name +/// one element, so a `Self` written twice is a broken file. +fn has_self(root: &El, name: &str, me: &str) -> bool { + root.children.iter().any(|n| matches!(n, crate::xml::Node::El(e) if e.name == name && e.get("Self") == Some(me))) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/idml/src/export/story.rs b/crates/idml/src/export/story.rs index 9e013e0a4..437376190 100644 --- a/crates/idml/src/export/story.rs +++ b/crates/idml/src/export/story.rs @@ -359,7 +359,13 @@ impl<'a> Ex<'a> { let mut psr = self.psr_el(&pf); let ends_with_break = s.text.get(r.clone()).and_then(|t| t.chars().next_back()).is_some_and(st::is_break_char); let segs = Self::paragraph_runs(s, &r); + // A hyperlink on a run of text is written as a `HyperlinkTextSource` around the + // ranges it covers, so the runs are cut at its edges first (#1446). + let (links, segs) = self.cut_runs_at_links(s, &r, segs); let nseg = segs.len(); + // Each segment's ranges, with the link they belong to; wrapped once the paragraph is + // built so that consecutive ranges of one link share a single source element. + let mut psr_csrs: Vec<(Option, El)> = Vec::new(); for (k, (rr, f)) in segs.into_iter().enumerate() { let seg_start = rr.start; let text = &s.text[rr]; @@ -520,13 +526,82 @@ impl<'a> Ex<'a> { csrs.push(c); } self.apply_tracked_changes(s, &f, &mut csrs, para_br, r.end); + let link = links.iter().find(|(_, l)| l.contains(&seg_start) || (l.start == seg_start && l.start == l.end)).map(|(i, _)| *i); for c in csrs { - psr.push(c); + psr_csrs.push((link, c)); } } + self.wrap_link_sources(&mut psr, psr_csrs); psr } + /// The hyperlinks on story text that fall in the paragraph `r` (each link's index and its + /// range clipped to `r`), and the paragraph's runs `segs` cut at their edges, so a link's + /// ranges can be wrapped on their own (#1446). + fn cut_runs_at_links( + &self, + s: &Story, + r: &std::ops::Range, + segs: Vec<(std::ops::Range, CharFormat)>, + ) -> (Vec<(usize, std::ops::Range)>, Vec<(std::ops::Range, CharFormat)>) { + let links: Vec<(usize, std::ops::Range)> = self + .d + .hyperlinks + .iter() + .enumerate() + .filter_map(|(i, h)| match &h.source { + designcraft_doc::HyperlinkSource::Text { story, start, end } if *story == s.id => { + let (a, b) = ((*start).max(r.start), (*end).min(r.end)); + (a < b).then_some((i, a..b)) + } + _ => None, + }) + .collect(); + let mut cuts: Vec = links.iter().flat_map(|(_, l)| [l.start, l.end]).collect(); + cuts.sort_unstable(); + cuts.dedup(); + let segs = segs.into_iter().fold(Vec::new(), |mut out: Vec<(std::ops::Range, CharFormat)>, (rr, f)| { + let mut at = rr.start; + for c in cuts.iter().copied().filter(|c| rr.start < *c && *c < rr.end) { + out.push((at..c, f.clone())); + at = c; + } + out.push((at..rr.end, f)); + out + }); + (links, segs) + } + + /// Push the paragraph's character ranges into `psr`, with each run of consecutive ranges of + /// one link inside a single `HyperlinkTextSource` (#1446). + fn wrap_link_sources(&mut self, psr: &mut El, psr_csrs: Vec<(Option, El)>) { + let mut groups: Vec<(Option, Vec)> = Vec::new(); + for (link, c) in psr_csrs { + match groups.last_mut() { + Some((l, v)) if *l == link => v.push(c), + _ => groups.push((link, vec![c])), + } + } + for (link, cs) in groups { + match link.and_then(|i| self.d.hyperlinks.get(i).map(|h| (i, h.name.clone()))) { + None => { + for c in cs { + psr.push(c); + } + } + Some((i, name)) => { + let me = self.fresh(); + let mut src = El::new("HyperlinkTextSource").attr("Self", &me).attr("Name", name).attr("Hidden", "false"); + for c in cs { + src.push(c); + } + psr.push(src); + self.link_text_sources.push((me, i)); + } + } + } + } + pub(super) fn psr_el(&mut self, pf: &ParaFormat) -> El { let mut el = El::new("ParagraphStyleRange").attr("AppliedParagraphStyle", names::style_self("ParagraphStyle", PARA_BUILTINS, &pf.style)); let mut props = Vec::new(); diff --git a/crates/idml/src/tests.rs b/crates/idml/src/tests.rs index d48e7a732..63ec2f08b 100644 --- a/crates/idml/src/tests.rs +++ b/crates/idml/src/tests.rs @@ -1585,6 +1585,153 @@ fn page_item_and_text_hyperlinks_survive_import() { ); } +/// The other direction of #1446: a document's own hyperlinks are written to IDML and read back. +/// Export wrote none of them, so a link made here (or imported from an InDesign file) was lost +/// the moment the document went out as IDML. +#[test] +fn round_trips_page_item_and_text_hyperlinks() { + use designcraft_doc::{Hyperlink, HyperlinkDest as D, HyperlinkSource as S}; + + let mut d = Document::new(&NewDocument { pages: 2, ..NewDocument::default() }); + let lid = d.default_layer(); + let (_, sid) = + d.add_text_frame(SpreadRef::Doc(0), Rect::new(40.0, 40.0, 400.0, 400.0), lid, "See the plate overleaf.", ParaFormat::default()).unwrap(); + let photo = designcraft_doc::ItemId(d.alloc()); + let frame = designcraft_doc::Item::new(photo, lid, Shape::Rectangle, shapes::rectangle(Rect::new(40.0, 420.0, 200.0, 500.0))); + d.insert_item(SpreadRef::Doc(0), frame, None).unwrap(); + let caption = d.paragraph_anchor(sid, 0).unwrap(); + + // One of each kind a link can have: a frame to a URL, a frame to an email address, a frame to + // a text anchor, and a run of text to a page. + let at = d.story(sid).unwrap().text.find("the plate").unwrap(); + for (name, source, dest) in [ + ("Photo", S::Item { id: photo }, D::Url("https://example.com/photo.jpg".into())), + ("Studio", S::Item { id: photo }, D::Email("studio@example.com".into())), + ("Caption", S::Item { id: photo }, D::Anchor(caption)), + ("Plate", S::Text { story: sid, start: at, end: at + "the plate".len() }, D::Page(1)), + ] { + let id = d.alloc(); + d.hyperlinks.push(Hyperlink { id, name: name.into(), source, dest }); + } + d.check().unwrap(); + + let idml = export_idml(&d); + let back = import_idml(&idml).unwrap(); + back.check().unwrap(); + + let named = |n: &str| back.hyperlinks.iter().find(|h| h.name == n).unwrap_or_else(|| panic!("{n} came back: {:?}", back.hyperlinks)); + assert_eq!(back.hyperlinks.len(), 4, "every link came back: {:?}", back.hyperlinks); + assert_eq!(named("Photo").dest, D::Url("https://example.com/photo.jpg".into())); + assert_eq!(named("Studio").dest, D::Email("studio@example.com".into()), "an email link is not written as a bare URL"); + assert_eq!(named("Plate").dest, D::Page(1), "the page destination is the same page"); + + // The frame links point at the frame, which has a new id on the way through. + let S::Item { id } = named("Photo").source else { panic!("a page item source: {:?}", named("Photo").source) }; + assert!(back.item(id).is_some(), "the source frame is in the document"); + assert_eq!(back.item(id).map(|i| i.shape), Some(Shape::Rectangle), "and it is the frame the link was on"); + + // The text link covers the same words, and the anchor link resolves to the same paragraph. + let S::Text { story, start, end } = named("Plate").source else { panic!("a text source: {:?}", named("Plate").source) }; + assert_eq!(&back.story(story).unwrap().text[start..end], "the plate"); + let D::Anchor(a) = named("Caption").dest else { panic!("an anchor destination: {:?}", named("Caption").dest) }; + assert_eq!(back.find_anchor(a).map(|(_, pos)| pos), Some(0), "the anchor is the paragraph it was on"); + + // One destination element per URL, however many links lead to it. + let text = zip_text(&idml, "designmap.xml"); + assert_eq!(text.matches(" = text.match_indices("Self=\"").map(|(i, _)| &text[i + 6..][..text[i + 6..].find('"').unwrap_or(0)]).collect(); + let mut seen = std::collections::HashSet::new(); + let repeated: Vec<&&str> = selfs.iter().filter(|s| !seen.insert(**s)).collect(); + assert!(repeated.is_empty(), "no Self is written twice, but these are: {repeated:?}"); + assert_eq!(text.matches(" = back.hyperlinks.iter().filter(|h| h.name == "Across").collect(); + assert_eq!(across.len(), 2, "each piece is a link of its own: {:?}", back.hyperlinks); + let piece = |h: &Hyperlink| { + let S::Text { story, start, end } = h.source else { panic!("a text source: {:?}", h.source) }; + back.story(story).unwrap().text[start..end].to_string() + }; + let mut pieces: Vec = across.iter().map(|h| piece(h)).collect(); + pieces.sort(); + // The first piece keeps the paragraph break, because the link's range covered it: the break + // is the last character of its paragraph, so the source that wraps that paragraph's text + // wraps the break with it. Between them the pieces cover exactly what the link covered. + assert_eq!(pieces, vec!["line\n".to_string(), "second".to_string()], "between them they cover the text the link was on"); + assert_eq!(pieces.concat(), d.story(sid).unwrap().text[start..end], "and nothing of it is lost or added"); + assert!(across.iter().all(|h| h.dest == D::Url("https://example.com/".into())), "both pieces lead where the link led"); +} + /// A link from a frame anchored in text is kept, but PDF export finds a link's frame among the /// spreads' items, where an anchored frame is not: the import report says the PDF will not have it, /// rather than the link going missing from the PDF without a word.