diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 4f388a7d9..e589e60a2 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -153,7 +153,7 @@ pub fn specs() -> Vec { "Link", ["Insert", "Links"], Some("Cmd+K"), - "{cell?, target: \"https://…\" | \"Sheet2!A1\" | \"mailto:…\", text?, tooltip?, remove?: bool}", + "{cell?, target: \"https://…\" | \"Book.xlsx\" | \"Sheet2!A1\" | \"mailto:…\", text?, tooltip?, remove?: bool}", has_doc, insert_link ), @@ -1166,6 +1166,7 @@ fn insert_link(s: &mut Session, p: &Json) -> Result { }; let text = str_param(p, "text").map(str::to_string); let tooltip = str_param(p, "tooltip").map(str::to_string); + let external = links_outside(&s.doc()?.wb, sheet, &target); edit(s, |cx| { let link_style = cx.wb.styles.derive(cx.wb.sheet(sheet).map(|sh| sh.style_id(at)).unwrap_or_default(), |st| { st.font.color = Color::Theme(10, 0); @@ -1177,12 +1178,30 @@ fn insert_link(s: &mut Session, p: &Json) -> Result { } else { sh.set_style(at, link_style); } - sh.hyperlinks.insert(at, Hyperlink { target: target.clone(), tooltip: tooltip.clone() }); + sh.hyperlinks.insert(at, Hyperlink { target: target.clone(), tooltip: tooltip.clone(), external }); cx.touch(sheet, at); Ok(Json::Null) }) } +/// Sorts a link target as the two pages of Excel's Insert Hyperlink dialog do: "Place in This +/// Document" is a sheet reference, a defined name or `#…`; anything else ("Existing File or Web +/// Page") is outside the workbook and saved as a relationship. +fn links_outside(wb: &Workbook, sheet: usize, target: &str) -> bool { + let t = target.trim(); + if t.starts_with('#') || wb.name(t, sheet).is_some() || wb.table(t).is_some() { + return false; + } + let (on, body) = split_sheet(t); + // Sheet names can't hold `: \ / ? * [ ]`, so `https://…!A1` or `C:\…!A1` is no sheet reference, + // and `Book.xlsx#Sheet1!A1` is a place in another file unless a sheet has that name. + let on_sheet = on.is_none_or(|name| { + wb.sheets.iter().any(|s| s.name.eq_ignore_ascii_case(&name)) + || (!name.is_empty() && !name.contains([':', '\\', '/', '?', '*', '[', ']', '#'])) + }); + !(on_sheet && RangeRef::parse(body).is_some()) +} + fn comment(s: &mut Session, p: &Json, threaded: bool) -> Result { let sheet = target_sheet(s, p)?; let at = cell_param_on(s, p, "cell", sheet)?.unwrap_or(s.doc()?.selection.active); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index a19913213..41a8392d1 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2657,6 +2657,56 @@ fn notes_comments_and_links_survive_a_json_save() { assert_eq!(after.cells, before.cells); } +/// Insert › Link sorts a target as Excel's dialog does: a sheet reference, a defined name or `#…` is +/// a place in this document, anything else an existing file or web page. A file saved as a +/// `location` opens nothing in Excel (#800). +#[test] +fn links_to_files_are_saved_as_external_relationships() { + use std::io::Read; + let mut s = s(); + s.execute("formulas.defineName", json!({"name": "Price", "refersTo": "=Sheet1!$A$1"})).unwrap(); + s.execute("insert.table", json!({"range": "F1:G3", "name": "Sales"})).unwrap(); + s.execute("home.insertSheet", json!({"name": "Q#1"})).unwrap(); + s.execute("sheet.activate", json!({"sheet": 0})).unwrap(); + let links = [ + ("A1", "Source.xlsx", true), + ("A2", r"C:\Temp\Book.xlsx", true), + ("A3", r"..\Reports\Q1.xlsx", true), + ("A4", "Other.xlsx#Sheet1!A2", true), + ("A5", "https://example.com/a.xlsx", true), + ("A6", "Sheet1!B2", false), + ("A7", "#Sheet1!B3", false), + ("A8", "Price", false), + ("A9", "'Sheet1'!C4:D5", false), + ("A10", "Sales", false), + ("A11", "'Q#1'!B2", false), + ]; + for (cell, target, _) in links { + s.execute("insert.link", json!({"cell": cell, "target": target})).unwrap(); + } + let bytes = gridcraft_xlsx::write_xlsx(&s.doc().unwrap().wb).unwrap(); + let mut zip = zip::ZipArchive::new(std::io::Cursor::new(bytes.clone())).unwrap(); + let mut part = |name: &str| { + let mut text = String::new(); + zip.by_name(name).unwrap().read_to_string(&mut text).unwrap(); + text + }; + let (sheet, rels) = (part("xl/worksheets/sheet1.xml"), part("xl/worksheets/_rels/sheet1.xml.rels")); + for (cell, target, external) in links { + let file = target.split_once('#').map_or(target, |(f, _)| f); + let as_rel = format!("Target=\"{file}\" TargetMode=\"External\""); + let as_location = format!("location=\"{}\"", target.trim_start_matches('#')); + assert_eq!(rels.contains(&as_rel), external, "{cell} {target}: {rels}"); + assert_eq!(sheet.contains(&as_location), !external, "{cell} {target}: {sheet}"); + } + assert!(sheet.contains("location=\"Sheet1!A2\""), "{sheet}"); + let (back, _) = gridcraft_xlsx::read_xlsx(&bytes).unwrap(); + for (cell, target, external) in links { + let h = &back.sheet(0).unwrap().hyperlinks[&CellRef::parse(cell).unwrap()]; + assert_eq!((h.target.as_str(), h.external), (target.trim_start_matches('#'), external), "{cell}"); + } +} + #[test] fn vertical_alignment_commands_and_roundtrip() { let mut s = s(); diff --git a/crates/model/src/cellmap.rs b/crates/model/src/cellmap.rs index 0f2c685a7..dbbe202f5 100644 --- a/crates/model/src/cellmap.rs +++ b/crates/model/src/cellmap.rs @@ -43,7 +43,7 @@ mod tests { dates: vec![], }, ); - s.hyperlinks.insert(c3, Hyperlink { target: "Sheet1!A1".into(), tooltip: Some("jump".into()) }); + s.hyperlinks.insert(c3, Hyperlink { target: "Sheet1!A1".into(), tooltip: Some("jump".into()), external: false }); let json = serde_json::to_string(&s).expect("serialize"); let back: Sheet = serde_json::from_str(&json).expect("deserialize"); assert_eq!(back.comments, s.comments); diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index 498d513d2..7e8e08bf7 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -304,9 +304,14 @@ pub struct Comment { #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct Hyperlink { - /// URL, `mailto:`, or an in-workbook location like `Sheet2!A1`. + /// URL, `mailto:`, a file path (`Book.xlsx`, `Book.xlsx#Sheet1!A2`), or an in-workbook location + /// like `Sheet2!A1`. pub target: String, pub tooltip: Option, + /// The target is outside this workbook (Excel's "Existing File or Web Page"): a relationship in + /// the file, not a `location`. A bare file name can't be told from a defined name by its text. + #[serde(default)] + pub external: bool, } // ---------------------------------------------------------------- drawing objects diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 2c8865e03..0d50a303d 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -714,6 +714,8 @@ fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &m for h in e.kids("hyperlink") { let Some(range) = h.attr("ref").and_then(RangeRef::parse) else { continue }; let mut target = h.attr("id").and_then(|id| rels.iter().find(|r| r.id == id)).map(|r| r.target.clone()).unwrap_or_default(); + // A relationship is a file or web page; a bare `location` is a place in this workbook. + let external = !target.is_empty(); if let Some(loc) = h.attr("location").filter(|l| !l.is_empty()) { if target.is_empty() { target = loc.trim_start_matches('#').to_string(); @@ -724,7 +726,7 @@ fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &m if target.is_empty() { continue; } - let link = Hyperlink { target, tooltip: h.attr("tooltip").map(str::to_string) }; + let link = Hyperlink { target, tooltip: h.attr("tooltip").map(str::to_string), external }; for c in range.iter() { if cells >= MAX_HYPERLINK_CELLS { cx.warn(format!("sheet \"{name}\": hyperlinks past the first {MAX_HYPERLINK_CELLS} cells were dropped")); diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index 0020adf8b..db348cc41 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -388,7 +388,10 @@ pub fn write_sheet(wb: &Workbook, si: usize, selected: bool, out: &mut Out) -> ( if t.is_empty() { continue; } - let external = t.contains("://") + // `external` is what the file said, or what Insert › Link chose; the scheme test covers + // links made before the flag existed. + let external = h.external + || t.contains("://") || t.to_ascii_lowercase().starts_with("mailto:") || t.starts_with("\\\\") || t.to_ascii_lowercase().starts_with("file:"); diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index 99221dd19..39ea30951 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -278,6 +278,63 @@ Check this"#; assert_eq!(s.sparklines[0].kind, gridcraft_model::SparklineKind::Column); } +/// The sheet's hyperlinks as written: cell, relationship target (external only), location. +fn written_links(bytes: &[u8]) -> Vec<(String, Option, Option)> { + let mut pkg = crate::package::Package::open(bytes).unwrap(); + let rels = pkg.rels("xl/worksheets/sheet1.xml").unwrap(); + let sheet = pkg.read_xml("xl/worksheets/sheet1.xml").unwrap().unwrap(); + let Some(links) = sheet.child("hyperlinks") else { return vec![] }; + links + .kids("hyperlink") + .map(|h| { + let rel = h.attr("id").map(|id| rels.iter().find(|r| r.id == id).unwrap()); + let target = rel.map(|r| { + assert!(r.external && r.kind == "hyperlink", "{r:?}"); + r.target.clone() + }); + (h.attr("ref").unwrap().to_string(), target, h.attr("location").map(str::to_string)) + }) + .collect() +} + +/// A link to another file is a relationship (`TargetMode="External"`) in the file, with the place in +/// it as `location`. Saved as a bare `location`, Excel takes it for a place in this workbook and the +/// link no longer opens the file (#800). C16 is Apache POI's `WithMoreVariousData.xlsx`. +#[test] +fn hyperlink_to_another_file_stays_external() { + let body = r#" + "#; + let rel = |id: &str, target: &str| { + format!( + r#""# + ) + }; + let sheet_rels = format!( + r#"{}{}{}{}"#, + rel("rId2", "WithVariousData.xlsx"), + rel("rId3", "Source.xlsx"), + rel("rId4", r"..\Reports\Q1.xlsx"), + rel("rId5", "https://example.com/a.xlsx"), + ); + let bytes = minimal(body, &[("xl/worksheets/_rels/sheet1.xml.rels", &sheet_rels)], "", ""); + let (wb, _) = read_xlsx(&bytes).unwrap(); + let s = wb.sheet(0).unwrap(); + assert!(s.hyperlinks[&at("C16")].external && !s.hyperlinks[&at("C19")].external); + + let some = |s: &str| Some(s.to_string()); + let links = written_links(&crate::write_xlsx(&wb).unwrap()); + assert_eq!( + links, + vec![ + ("C16".into(), some("WithVariousData.xlsx"), None), + ("C17".into(), some("Source.xlsx"), some("Sheet1!A2")), + ("C18".into(), some(r"..\Reports\Q1.xlsx"), None), + ("C19".into(), None, some("Data!B2")), + ("C20".into(), some("https://example.com/a.xlsx"), None), + ] + ); +} + #[test] fn drawing_with_picture_and_chart() { let png: &[u8] = b"\x89PNG\r\n\x1a\nfakeimagedata"; diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 07b103945..5228a411b 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -204,9 +204,9 @@ fn sample() -> Workbook { security_descriptor: None, }, ]; - s.hyperlinks.insert(at("A2"), Hyperlink { target: "https://example.com/?a=1&b=2".into(), tooltip: Some("go".into()) }); - s.hyperlinks.insert(at("A3"), Hyperlink { target: "'Data & Stuff'!B2".into(), tooltip: None }); - s.hyperlinks.insert(at("A4"), Hyperlink { target: "mailto:me@example.com".into(), tooltip: None }); + s.hyperlinks.insert(at("A2"), Hyperlink { target: "https://example.com/?a=1&b=2".into(), tooltip: Some("go".into()), external: true }); + s.hyperlinks.insert(at("A3"), Hyperlink { target: "'Data & Stuff'!B2".into(), tooltip: None, external: false }); + s.hyperlinks.insert(at("A4"), Hyperlink { target: "mailto:me@example.com".into(), tooltip: None, external: true }); s.comments.insert( at("B2"), Comment {