aleleba/gridcraft
1
0
Fork 0
mirror of https://github.com/storytold/gridcraft.git synced 2026-10-28 12:46:48 -06:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #826 from marcoarnulfo:fix/xlsx-external-file-hyperlinks

Browse Source
This commit is contained in:
Brandon Thomas
2026-10-12 23:49:53 -04:00
parent a76759cba6 52b4f873c7
commit efed7caa01
8 changed files with 145 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/engine/src/cmd/insert.rs
+21 -2
View File
@@ -153,7 +153,7 @@ pub fn specs() -> Vec<CommandSpec> {
"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<Json> {
};
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<Json> {
} 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<Json> {
let sheet = target_sheet(s, p)?;
let at = cell_param_on(s, p, "cell", sheet)?.unwrap_or(s.doc()?.selection.active);
crates/engine/src/tests.rs
+50
View File
@@ -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();
crates/model/src/cellmap.rs
+1 -1
View File
@@ -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);
crates/model/src/features.rs
+6 -1
View File
@@ -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<String>,
/// 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
crates/xlsx/src/sheet_read.rs
+3 -1
View File
@@ -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"));
crates/xlsx/src/sheet_write.rs
+4 -1
View File
@@ -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:");
crates/xlsx/src/tests/fixtures.rs
+57
View File
@@ -278,6 +278,63 @@ Check this</t></r></text></comment></commentList></comments>"#;
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<String>, Option<String>)> {
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#"<sheetData/><hyperlinks><hyperlink ref="C16" r:id="rId2"/><hyperlink ref="C17" r:id="rId3" location="Sheet1!A2"/>
<hyperlink ref="C18" r:id="rId4"/><hyperlink ref="C19" location="Data!B2"/><hyperlink ref="C20" r:id="rId5"/></hyperlinks>"#;
let rel = |id: &str, target: &str| {
format!(
r#"<Relationship Id="{id}" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink" Target="{target}" TargetMode="External"/>"#
)
};
let sheet_rels = format!(
r#"<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships">{}{}{}{}</Relationships>"#,
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";
crates/xlsx/src/tests/roundtrip.rs
+3 -3
View File
@@ -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 {