A hyperlink can point at a text anchor (#1446) (#1555)

* A hyperlink can point at a text anchor (#1446)

A frame or a text range could link to a URL, an email address or a page
number. A page number is the wrong thing to store for "this thumbnail links to
that photo": the photo moves and the link does not. InDesign stores the
destination, and IDML files do too — a `HyperlinkPageItemSource` pointing at a
`HyperlinkTextDestination` is how a report links its thumbnails to the
full-size pictures (#1446).

`HyperlinkDest::Anchor(id)` is a text anchor, the same destination a
cross-reference targets, and the PDF export resolves it the same way: through
the cross-reference index, to whichever page the anchor's text has landed on.
A link to an anchor that no longer exists is left out of the file rather than
pointing somewhere wrong.

`hyperlink.create` and `hyperlink.edit` take `anchor` — an anchor's id or its
name — and a link that is given no name of its own takes the anchor's.

Measured: a thumbnail on page 1 linked to an anchor in text on page 3 exports
`/Dest [<page 3> 0 R /XYZ 0 300 0]`; insert a page in front and the same link
exports page 4, with no edit to the link.

This is the groundwork for the IDML half of #1446 (import and export of
page-item and text hyperlinks), which is the next change and needs a
destination to put them in.

* An anchor a hyperlink cannot mean is refused, not ignored

This PR added anchors as link destinations and did not check that the anchor
exists, so the feature was wrong on its own and review was right not to take the
fix from a PR stacked above it.

`dest_param` returns `Result<Option<_>>` now. `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.

Two behaviours follow. An unknown *id* was accepted outright -- `as_u64`
answered for any number -- so a link was stored to an anchor nobody has and the
export quietly left it out. And 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`", which named the wrong problem; it says "no text
anchor X (anchor.list names them)" now.

The test covers the id, the name, both on `edit`, that a refused edit leaves the
destination, the name and the exported page alone, and that a rename naming no
destination still works -- that last one is what `None` is for and it must not
have become an error on the way.

The page-number validation #1564 adds stays there; this is the anchor half,
which belongs with the anchor feature.

* A name two anchors share cannot pick one of them

`anchor.create` does not require an anchor's name to be unique, and an IDML
import can bring a document that already has two of the same. The lookup took
the first match, so a link made on a shared name went to whichever anchor came
first -- a different page, with nothing to say so (noted in review).

An id still names one anchor or none, and is checked as before. A name that two
anchors answer to is refused, with their ids in the message so the caller can
say which one they meant.

Not uniqueness at creation, which was the other way offered: a document can
already hold duplicates, so enforcing it there would refuse to open files that
are fine today, and the ambiguity only matters where a name is being resolved.

The test builds two anchors of one name on different pages, asserts both
`hyperlink.create` and `hyperlink.edit` refuse it and say what to do instead,
that the refused create made no link and the refused edit left the destination
alone, and that each anchor is still reachable by its id. It is a test of its
own because the twin anchor outlives the deletion the other test ends with.

* Test: two anchors sharing a name come from a file, now that anchor.create refuses a name in use

main's anchor.create now refuses a second anchor of a name, so the test makes the first and renames the second in the document, as a file opened with two can hold them. The comment on the guard says the same.

crates/doc/src/lib.rs

@@ -766,6 +766,21 @@ pub enum HyperlinkDest {
     Email(String),
     /// Absolute page index.
     Page(usize),
+    /// A text anchor ([`TextAnchor::id`]), as a cross-reference targets one. The page is
+    /// whichever page the anchor's text has landed on when the document is exported, so the link
+    /// follows its destination as the text reflows (#1446).
+    Anchor(u64),
+}
+
+impl HyperlinkDest {
+    /// The name a hyperlink is given when none is supplied.
+    pub fn label(&self) -> String {
+        match self {
+            HyperlinkDest::Url(u) | HyperlinkDest::Email(u) => u.clone(),
+            HyperlinkDest::Page(p) => format!("Page {}", p + 1),
+            HyperlinkDest::Anchor(a) => format!("Anchor {a}"),
+        }
+    }
 }
 
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]

crates/engine/src/cmd/datamerge/fill.rs

@@ -410,10 +410,7 @@ fn create_link(doc: &mut designcraft_doc::Document, anchor: PlaceholderAnchor, d
         }
     };
     let id = doc.alloc();
-    let name = match &dest {
-        HyperlinkDest::Url(u) | HyperlinkDest::Email(u) => u.clone(),
-        HyperlinkDest::Page(p) => format!("Page {}", p + 1),
-    };
+    let name = dest.label();
     doc.hyperlinks.push(Hyperlink { id, name, source, dest });
     true
 }

crates/engine/src/cmd/datamerge/tests.rs

@@ -742,6 +742,7 @@ fn role_overrides_column_kind_qr_and_hyperlink() {
             HyperlinkDest::Email(v) => format!("email:{v}"),
             HyperlinkDest::Url(v) => format!("url:{v}"),
             HyperlinkDest::Page(n) => format!("page:{n}"),
+            HyperlinkDest::Anchor(a) => format!("anchor:{a}"),
         });
     }
     kinds.sort();

crates/engine/src/cmd/interactive.rs

@@ -14,7 +14,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "New Hyperlink…",
             ["Type", "Hyperlinks & Cross-References"],
             None,
-            "{url? | email? | page? (1-based), name?, ids?} — from the selected text, or the selected frames",
+            "{url? | email? | page? (1-based) | anchor? (a text anchor's id or name; the link follows it as the text reflows), name?, ids?} — from the selected text, or the selected frames",
             has_doc,
             create
         ),
@@ -26,9 +26,9 @@ pub fn specs() -> Vec<CommandSpec> {
             })
         }),
         cmd!(query "hyperlink.list", "Hyperlinks", [], None, "{}", has_doc, |s, _| Ok(serde_json::to_value(&s.doc()?.doc.hyperlinks).unwrap_or_default())),
-        cmd!("hyperlink.edit", "Hyperlink Options…", [], None, "{id, name?, url? | email? | page? (1-based)}", has_doc, |s, p| {
+        cmd!("hyperlink.edit", "Hyperlink Options…", [], None, "{id, name?, url? | email? | page? (1-based) | anchor?}", has_doc, |s, p| {
             let id = p.get("id").and_then(Value::as_u64).ok_or_else(|| bad("hyperlink.edit", "missing id"))?;
-            let dest = dest_param(p);
+            let dest = dest_param("hyperlink.edit", &s.doc()?.doc, p)?;
             let name = str_param(p, "name").map(str::to_string);
             s.edit(|d, _| {
                 let h = d.hyperlinks.iter_mut().find(|h| h.id == id).ok_or_else(|| bad("hyperlink.edit", format!("no hyperlink {id}")))?;
@@ -91,19 +91,62 @@ pub fn specs() -> Vec<CommandSpec> {
     ]
 }
 
-fn dest_param(p: &Value) -> Option<HyperlinkDest> {
+/// `url`, `email`, `page` (1-based) or `anchor` (a text anchor's id, or its name).
+///
+/// `None` is "no destination given", which `hyperlink.edit` needs for an edit that only renames.
+/// An `anchor` that is given and names none is an error, not `None`: an unknown name made
+/// `hyperlink.create` fall through to `page` and an unknown id was stored as it stood, so the
+/// link went to an anchor nobody has and the export quietly left it out -- and on
+/// `hyperlink.edit` the `None` read as "no destination given", so the command reported success
+/// and kept the old destination (noted in review).
+fn dest_param(id: &str, d: &designcraft_doc::Document, p: &Value) -> Result<Option<HyperlinkDest>> {
     if let Some(u) = str_param(p, "url") {
-        Some(HyperlinkDest::Url(u.to_string()))
-    } else if let Some(e) = str_param(p, "email") {
-        Some(HyperlinkDest::Email(e.to_string()))
-    } else {
-        p.get("page").and_then(Value::as_u64).map(|pg| HyperlinkDest::Page((pg as usize).saturating_sub(1)))
+        return Ok(Some(HyperlinkDest::Url(u.to_string())));
     }
+    if let Some(e) = str_param(p, "email") {
+        return Ok(Some(HyperlinkDest::Email(e.to_string())));
+    }
+    if let Some(a) = p.get("anchor").filter(|a| !a.is_null()) {
+        let anchors = || d.stories.values().flat_map(|st| st.anchors.iter());
+        // An id names one anchor or none.
+        if let Some(n) = a.as_u64() {
+            return match anchors().any(|x| x.id == n) {
+                true => Ok(Some(HyperlinkDest::Anchor(n))),
+                false => Err(bad(id, format!("no text anchor {a} (anchor.list names them)"))),
+            };
+        }
+        // A name need not. `anchor.create` refuses a name in use, but an IDML import can bring a
+        // document that already has two of the same, so taking the first match stored whichever
+        // anchor happened to come first, and a link on a name shared by anchors on two pages went
+        // to the wrong page with nothing to say so (noted in review). The id still reaches either
+        // of them.
+        let Some(name) = a.as_str() else { return Err(bad(id, format!("no text anchor {a} (anchor.list names them)"))) };
+        let mut named = anchors().filter(|x| x.name == name);
+        return match (named.next(), named.next()) {
+            (Some(x), None) => Ok(Some(HyperlinkDest::Anchor(x.id))),
+            (Some(_), Some(_)) => {
+                let ids: Vec<String> = anchors().filter(|x| x.name == name).map(|x| x.id.to_string()).collect();
+                Err(bad(id, format!("more than one text anchor is named {name:?} ({}); give the id instead", ids.join(", "))))
+            }
+            (None, _) => Err(bad(id, format!("no text anchor {a} (anchor.list names them)"))),
+        };
+    }
+    Ok(p.get("page").and_then(Value::as_u64).map(|pg| HyperlinkDest::Page((pg as usize).saturating_sub(1))))
+}
+
+/// The anchor's own name, for a link to one that was not given a name.
+fn dest_label(d: &designcraft_doc::Document, dest: &HyperlinkDest) -> String {
+    if let HyperlinkDest::Anchor(a) = dest
+        && let Some(x) = d.stories.values().flat_map(|st| st.anchors.iter()).find(|x| x.id == *a)
+    {
+        return x.name.clone();
+    }
+    dest.label()
 }
 
 fn create(s: &mut Session, p: &Value) -> Result<Value> {
-    let dest = dest_param(p).ok_or_else(|| bad("hyperlink.create", "give `url`, `email` or `page`"))?;
     let st = s.doc()?;
+    let dest = dest_param("hyperlink.create", &st.doc, p)?.ok_or_else(|| bad("hyperlink.create", "give `url`, `email`, `page` or `anchor`"))?;
     let sources: Vec<HyperlinkSource> = if let Some(ids) = super::ids_param(p, "ids") {
         ids.into_iter().map(|id| HyperlinkSource::Item { id }).collect()
     } else if let Some(t) = st.selection.text.filter(|t| !t.is_caret()) {
@@ -119,11 +162,7 @@ fn create(s: &mut Session, p: &Value) -> Result<Value> {
         let mut ids = vec![];
         for src in sources {
             let id = d.alloc();
-            let name = name.clone().unwrap_or_else(|| match &dest {
-                HyperlinkDest::Url(u) => u.clone(),
-                HyperlinkDest::Email(e) => e.clone(),
-                HyperlinkDest::Page(p) => format!("Page {}", p + 1),
-            });
+            let name = name.clone().unwrap_or_else(|| dest_label(d, &dest));
             d.hyperlinks.push(Hyperlink { id, name, source: src, dest: dest.clone() });
             ids.push(id);
         }
@@ -157,3 +196,162 @@ mod panel_tests {
         assert_eq!(s.execute("bookmark.list", &json!({})).unwrap()[0]["name"], "Back");
     }
 }
+
+#[cfg(test)]
+mod anchor_dest_tests {
+    use serde_json::json;
+
+    use crate::Session;
+
+    /// The page object numbers of a PDF, in order, and the page each link annotation points at
+    /// (as an index into that list).
+    fn link_pages(s: &mut Session) -> Vec<usize> {
+        let r = s.execute("file.exportPdf", &json!({})).unwrap();
+        let bytes = super::super::file::base64_decode(r["base64"].as_str().unwrap());
+        let pdf = String::from_utf8_lossy(&bytes).into_owned();
+        let kids: Vec<&str> = pdf
+            .split("/Kids[")
+            .nth(1)
+            .and_then(|k| k.split(']').next())
+            .map(|k| k.split(" 0 R").map(str::trim).filter(|s| !s.is_empty()).collect())
+            .unwrap_or_default();
+        let object =
+            |n: &str| -> String { pdf.split(&format!("\n{n} 0 obj\n")).nth(1).and_then(|o| o.split("\nendobj").next()).unwrap_or("").to_string() };
+        let mut out = Vec::new();
+        for part in pdf.split("/Subtype/Link").skip(1) {
+            let Some(dest) = part.split("/Dest ").nth(1).and_then(|d| d.split(" 0 R").next()) else { continue };
+            // `[<page> 0 R/XYZ …]`
+            let page = object(dest.trim());
+            let Some(page) = page.trim_start_matches('[').split(" 0 R").next() else { continue };
+            if let Some(i) = kids.iter().position(|k| *k == page.trim()) {
+                out.push(i);
+            }
+        }
+        out
+    }
+
+    /// `anchor.create` does not require a name to be unique -- and an IDML import can bring a
+    /// document that already has two of the same -- so the lookup taking the first match meant a
+    /// link made on a shared name went to whichever anchor came first, a different page, with
+    /// nothing to say so (noted in review). The id still reaches either of them.
+    #[test]
+    fn a_name_two_anchors_share_cannot_pick_one_of_them() {
+        let mut s = Session::new();
+        s.execute("file.new", &json!({"width": 300, "height": 300, "pages": 3, "facingPages": false, "margins": 0})).unwrap();
+
+        // Two anchors with one name, in text on different pages. `anchor.create` refuses a name
+        // in use, but a document opened from a file can still hold two, so the second is renamed
+        // in the document directly.
+        let mut made = Vec::new();
+        for (page, name) in [(1usize, "Photo 1"), (2, "Photo 1 (second)")] {
+            let sp = s.doc().unwrap().doc.page_loc(page).unwrap().0;
+            let f = s
+                .execute("frame.create", &json!({"spread": sp, "rect": [20, 20, 280, 100], "content": "text", "text": "Photo 1 full size"}))
+                .unwrap();
+            s.execute("text.select", &json!({"story": f["story"].clone(), "anchor": 0, "focus": 7})).unwrap();
+            made.push(s.execute("anchor.create", &json!({"name": name})).unwrap());
+        }
+        s.edit(|d, _| {
+            for sid in d.stories.keys().copied().collect::<Vec<_>>() {
+                for a in &mut d.story_mut(sid).unwrap().anchors {
+                    if a.name == "Photo 1 (second)" {
+                        std::sync::Arc::make_mut(a).name = "Photo 1".into();
+                    }
+                }
+            }
+            Ok(())
+        })
+        .unwrap();
+        assert_ne!(made[0]["id"], made[1]["id"], "two anchors, one name");
+        let thumb = s.execute("frame.create", &json!({"rect": [20, 20, 120, 80], "content": "unassigned"})).unwrap();
+
+        // Creating on the shared name is refused, and says what to do instead.
+        let e = s.execute("hyperlink.create", &json!({"anchor": "Photo 1", "ids": [thumb["id"].clone()]})).expect_err("ambiguous").to_string();
+        assert!(e.contains("more than one text anchor"), "{e}");
+        assert!(e.contains("give the id instead"), "it says what to do instead: {e}");
+        assert!(s.execute("hyperlink.list", &json!({})).unwrap().as_array().is_some_and(|l| l.is_empty()), "and no link was made");
+
+        // By id it works, and the id picks the one that was asked for.
+        s.execute("hyperlink.create", &json!({"anchor": made[1]["id"].clone(), "ids": [thumb["id"].clone()]})).unwrap();
+        let live = s.execute("hyperlink.list", &json!({})).unwrap()[0]["id"].clone();
+        let dest = |s: &mut Session| s.execute("hyperlink.list", &json!({})).unwrap()[0]["dest"].clone();
+        assert_eq!(dest(&mut s), json!({"kind": "anchor", "value": made[1]["id"].clone()}));
+
+        // An edit on the shared name is refused and leaves the destination as it was.
+        let e = s.execute("hyperlink.edit", &json!({"id": live.clone(), "anchor": "Photo 1"})).expect_err("ambiguous").to_string();
+        assert!(e.contains("more than one text anchor"), "{e}");
+        assert_eq!(dest(&mut s), json!({"kind": "anchor", "value": made[1]["id"].clone()}), "the refused edit changed nothing");
+
+        // And the other anchor is still reachable by its id.
+        s.execute("hyperlink.edit", &json!({"id": live, "anchor": made[0]["id"].clone()})).unwrap();
+        assert_eq!(dest(&mut s), json!({"kind": "anchor", "value": made[0]["id"].clone()}));
+    }
+
+    /// A frame can link to a text anchor, as a cross-reference refers to one, and the link lands
+    /// on whichever page the anchor's text is on when the PDF is written — IDML files link photo
+    /// thumbnails to their full-size pictures that way (#1446).
+    #[test]
+    fn a_frame_can_link_to_a_text_anchor_and_follows_it() {
+        let mut s = Session::new();
+        s.execute("file.new", &json!({"width": 300, "height": 300, "pages": 3, "facingPages": false, "margins": 0})).unwrap();
+        // The destination: an anchor in text on page 3.
+        let sp = s.doc().unwrap().doc.page_loc(2).unwrap().0;
+        let dest =
+            s.execute("frame.create", &json!({"spread": sp, "rect": [20, 20, 280, 100], "content": "text", "text": "Photo 1 full size"})).unwrap();
+        s.execute("text.select", &json!({"story": dest["story"].clone(), "anchor": 0, "focus": 7})).unwrap();
+        let anchor = s.execute("anchor.create", &json!({"name": "Photo 1"})).unwrap();
+        // The source: a thumbnail frame on page 1, linked by the anchor's name.
+        let thumb = s.execute("frame.create", &json!({"rect": [20, 20, 120, 80], "content": "unassigned"})).unwrap();
+        s.execute("hyperlink.create", &json!({"anchor": "Photo 1", "ids": [thumb["id"].clone()]})).unwrap();
+        let links = s.execute("hyperlink.list", &json!({})).unwrap();
+        assert_eq!(links[0]["dest"], json!({"kind": "anchor", "value": anchor["id"].clone()}), "{links}");
+        // Named after the anchor, since no name was given.
+        assert_eq!(links[0]["name"], "Photo 1");
+        assert_eq!(link_pages(&mut s), vec![2], "the link lands on the anchor's page (the third)");
+
+        // A page inserted in front moves the anchor's text to page 4, and the link follows it
+        // without being edited.
+        s.execute("layout.pages.insert", &json!({"count": 1, "after": 0})).unwrap();
+        assert_eq!(s.execute("anchor.list", &json!({})).unwrap()[0]["page"], "4");
+        assert_eq!(link_pages(&mut s), vec![3], "the link follows the text");
+
+        // An anchor's id works as well as its name, and an unknown name is refused.
+        s.execute("hyperlink.delete", &json!({"id": links[0]["id"].clone()})).unwrap();
+        s.execute("hyperlink.create", &json!({"anchor": anchor["id"].clone(), "ids": [thumb["id"].clone()], "name": "Thumbnail"})).unwrap();
+        assert_eq!(s.execute("hyperlink.list", &json!({})).unwrap()[0]["name"], "Thumbnail");
+        assert_eq!(link_pages(&mut s), vec![3]);
+        let e = s.execute("hyperlink.create", &json!({"anchor": "No Such Anchor", "ids": [thumb["id"].clone()]})).expect_err("refused").to_string();
+        assert!(e.contains("no text anchor"), "and it says why, rather than that no destination was given: {e}");
+
+        // An unknown anchor *id* is refused as well. `as_u64` answered for any number, so a link
+        // was stored to an anchor nobody has and the export quietly left it out (noted in review).
+        let live = s.execute("hyperlink.list", &json!({})).unwrap()[0]["id"].clone();
+        let missing = anchor["id"].as_u64().expect("the anchor's id") + 9_999;
+        let e = s.execute("hyperlink.create", &json!({"anchor": missing, "ids": [thumb["id"].clone()]})).expect_err("refused").to_string();
+        assert!(e.contains("no text anchor"), "an unknown id is refused: {e}");
+
+        // And `hyperlink.edit` reports it rather than reading it as "no destination given", which
+        // made the command succeed and keep the old destination (noted in review).
+        for unknown in [json!(missing), json!("No Such Anchor")] {
+            let e = s.execute("hyperlink.edit", &json!({"id": live.clone(), "anchor": unknown.clone()})).expect_err("refused").to_string();
+            assert!(e.contains("no text anchor"), "{unknown}: {e}");
+        }
+        let after = s.execute("hyperlink.list", &json!({})).unwrap();
+        assert_eq!(after[0]["dest"], json!({"kind": "anchor", "value": anchor["id"].clone()}), "the destination is unchanged: {after}");
+        assert_eq!(after[0]["name"], "Thumbnail", "and so is the name");
+        assert_eq!(link_pages(&mut s), vec![3], "and the export still lands on the anchor's page");
+
+        // A rename that names no destination still works: that is what `None` is for, and it must
+        // not have become an error on the way.
+        s.execute("hyperlink.edit", &json!({"id": live.clone(), "name": "Thumb"})).unwrap();
+        let renamed = s.execute("hyperlink.list", &json!({})).unwrap();
+        assert_eq!(renamed[0]["name"], "Thumb");
+        assert_eq!(renamed[0]["dest"], json!({"kind": "anchor", "value": anchor["id"].clone()}), "the destination survives a rename");
+
+        // A link to a deleted anchor is not exported, rather than pointing at the wrong page.
+        s.execute("text.select", &json!({"story": dest["story"].clone(), "anchor": 0, "focus": 8})).unwrap();
+        s.execute("text.delete", &json!({})).unwrap();
+        assert!(s.execute("anchor.list", &json!({})).unwrap().as_array().unwrap().is_empty());
+        assert_eq!(link_pages(&mut s), Vec::<usize>::new(), "no link rather than a wrong one");
+    }
+}

crates/pdf/src/links.rs

@@ -73,6 +73,11 @@ pub(crate) fn annotations(doc: &Document, cache: &designcraft_compose::Cache, sh
                     Some(i) => Target::Destination(XyzDestination::new(i, krilla::geom::Point::from_xy(0.0, 0.0)).into()),
                     None => continue,
                 },
+                // The page the anchor's text has landed on, as a cross-reference resolves it.
+                HyperlinkDest::Anchor(a) => match cache.xref_index(doc).page_index(*a).and_then(|p| sheet_of_page(doc, sheets, p)) {
+                    Some(i) => Target::Destination(XyzDestination::new(i, krilla::geom::Point::from_xy(0.0, 0.0)).into()),
+                    None => continue,
+                },
             };
             out.push(Annotation::new_link(LinkAnnotation::new(rect, target), Some(h.name.clone())));
         }