From a35d241caf654a4c320283ece685d0eeed369181 Mon Sep 17 00:00:00 2001 From: Gert Pellin Date: Fri, 16 Oct 2026 04:45:33 +0200 Subject: [PATCH] XLSX: a note keeps its box's place and size through open and save (#1608) The reader took only from a note's VML shape and vml_xml wrote every note with a fixed anchor (two columns right of the cell, four rows tall) and a 108 x 59.25 pt style, so a plain open and save moved and resized every note box Excel had stored. Comment gets an optional NoteBox: the corners relative to the note's cell (so the box follows the note when rows or columns move) and the shape's width and height. The writer puts it back, clamped to the sheet; notes made here, and boxes equal to the default, keep the default box. --- crates/engine/src/cmd/insert.rs | 11 ++- crates/model/src/cellmap.rs | 1 + crates/model/src/features.rs | 28 +++++++ crates/xlsx/src/sheet_read.rs | 69 ++++++++++++----- crates/xlsx/src/sheet_write.rs | 33 ++++++-- crates/xlsx/src/tests/malformed.rs | 11 ++- crates/xlsx/src/tests/mod.rs | 1 + crates/xlsx/src/tests/note_box.rs | 117 +++++++++++++++++++++++++++++ crates/xlsx/src/tests/roundtrip.rs | 36 ++++++++- docs/file-format-parity.md | 3 +- 10 files changed, 277 insertions(+), 33 deletions(-) create mode 100644 crates/xlsx/src/tests/note_box.rs diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 9f044613d..9987afa14 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1250,7 +1250,16 @@ fn comment(s: &mut Session, p: &Json, threaded: bool) -> Result { edit(s, |cx| { cx.sheet_mut(sheet)?.comments.insert( at, - Comment { author: author.clone(), text: text.clone(), replies: vec![], threaded, resolved: false, visible: false, dates: vec![] }, + Comment { + author: author.clone(), + text: text.clone(), + replies: vec![], + threaded, + resolved: false, + visible: false, + dates: vec![], + note_box: None, + }, ); Ok(json!({"cell": at.a1()})) }) diff --git a/crates/model/src/cellmap.rs b/crates/model/src/cellmap.rs index dbbe202f5..f72b7b5af 100644 --- a/crates/model/src/cellmap.rs +++ b/crates/model/src/cellmap.rs @@ -41,6 +41,7 @@ mod tests { resolved: false, visible: false, dates: vec![], + note_box: None, }, ); s.hyperlinks.insert(c3, Hyperlink { target: "Sheet1!A1".into(), tooltip: Some("jump".into()), external: false }); diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index c35254f0e..b04e148af 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -323,6 +323,34 @@ pub struct Comment { /// read from the file (`dT`); empty when unknown, e.g. for comments made here. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub dates: Vec, + /// Where the note's box sits and how big it is, as read from the file; `None` (a note made + /// here, or a file without the shape) writes the default box next to the cell. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub note_box: Option, +} + +/// A note box's place and size, from the note's VML shape: Excel's `` with its corners +/// stored relative to the note's cell, so the box moves with the note when rows or columns are +/// inserted or deleted, and the shape's width and height. +#[derive(Clone, Copy, Debug, PartialEq, Serialize, Deserialize)] +pub struct NoteBox { + /// Top-left corner: columns and rows from the note's cell (negative = left of or above it), + /// then the pixel offset into that column and row. + pub from: NoteCorner, + /// Bottom-right corner, the same way. + pub to: NoteCorner, + /// The shape's width and height in points (the VML style's `width`/`height`), when given. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub size_pt: Option<(f64, f64)>, +} + +/// One corner of a [`NoteBox`]. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct NoteCorner { + pub col: i64, + pub col_px: u32, + pub row: i64, + pub row_px: u32, } #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 0194d2268..7e75c1bc3 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -8,8 +8,8 @@ use gridcraft_core::{CellError, CellRef, DateSystem, MAX_COLS, MAX_ROWS, RangeRe use gridcraft_formula::Expr; use gridcraft_model::{ AutoFilter, Cell, CfOperator, CfRule, CfValueKind, Comment, CondFormat, EditRange, ErrorStyle, FilterCriterion, Formula, Hyperlink, LineInfo, - Orientation, PasswordHash, Sheet, SheetProtection, Sparkline, SparklineKind, Table, TableColumn, TotalsFn, Validation, ValidationKind, - XmlColumnPr, + NoteBox, NoteCorner, Orientation, PasswordHash, Sheet, SheetProtection, Sparkline, SparklineKind, Table, TableColumn, TotalsFn, Validation, + ValidationKind, XmlColumnPr, }; use crate::IoError; @@ -210,12 +210,13 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result`). Best effort: the VML is - // only read for that flag, so a part that doesn't parse is skipped without a warning. + // Which notes are displayed (``) and where their boxes sit live in the VML shapes. + // Best effort: the VML is only read for those, so a part that doesn't parse is skipped without + // a warning. if !sheet.comments.is_empty() { for rel in rels.iter().filter(|r| r.kind == "vmlDrawing") { if let Ok(Some(x)) = cx.pkg.read_xml(&rel.target) { - read_note_visibility(&x, &mut sheet, 0); + read_note_shapes(&x, &mut sheet, 0); } } } @@ -882,34 +883,63 @@ fn read_comments(x: &El, sheet: &mut Sheet) { { text = rest.strip_prefix('\n').unwrap_or(rest).to_string(); } - sheet.comments.insert(cell, Comment { author, text, replies: vec![], threaded: false, resolved: false, visible: false, dates: vec![] }); + sheet + .comments + .insert(cell, Comment { author, text, replies: vec![], threaded: false, resolved: false, visible: false, dates: vec![], note_box: None }); } } -/// Marks the notes whose VML shape carries `` as shown. Shapes may sit in groups. -fn read_note_visibility(x: &El, sheet: &mut Sheet, depth: usize) { +/// Reads the notes' VML shapes: a shape carrying `` marks its note as shown, and its +/// `` and style size become the note's box. Shapes may sit in groups. +fn read_note_shapes(x: &El, sheet: &mut Sheet, depth: usize) { if depth > xml::MAX_DEPTH { return; } for e in &x.children { if e.name != "shape" { - read_note_visibility(e, sheet, depth + 1); + read_note_shapes(e, sheet, depth + 1); continue; } let Some(cd) = e.child("ClientData").filter(|cd| cd.attr("ObjectType") == Some("Note")) else { continue }; - if cd.child("Visible").is_none() { - continue; - } let num = |k: &str| cd.child(k).and_then(|v| v.text.trim().parse::().ok()); - if let (Some(row), Some(col)) = (num("Row"), num("Column")) - && let Some(c) = sheet.comments.get_mut(&CellRef::new(row, col)) - && !c.threaded - { + let (Some(row), Some(col)) = (num("Row"), num("Column")) else { continue }; + let cell = CellRef::new(row, col); + let Some(c) = sheet.comments.get_mut(&cell) else { continue }; + if cd.child("Visible").is_some() && !c.threaded { c.visible = true; } + if let Some(anchor) = cd.child("Anchor") { + c.note_box = note_box(&anchor.text, e.attr("style").unwrap_or(""), cell).filter(|b| *b != crate::sheet_write::DEFAULT_NOTE_BOX); + } } } +/// A note's box from its VML `` (`LeftColumn, LeftOffset, TopRow, TopOffset, +/// RightColumn, RightOffset, BottomRow, BottomOffset`; offsets in pixels) and its shape's style, +/// the corners made relative to the note's `cell`. `None` when the anchor isn't eight in-sheet +/// numbers. +fn note_box(anchor: &str, style: &str, cell: CellRef) -> Option { + let v: Vec = anchor.split(',').map(|p| p.trim().parse::().ok()).collect::>()?; + let [c1, dx1, r1, dy1, c2, dx2, r2, dy2] = <[u32; 8]>::try_from(v).ok()?; + if c1.max(c2) >= MAX_COLS || r1.max(r2) >= MAX_ROWS { + return None; + } + let corner = |c: u32, dx: u32, r: u32, dy: u32| NoteCorner { + col: i64::from(c) - i64::from(cell.col), + col_px: dx, + row: i64::from(r) - i64::from(cell.row), + row_px: dy, + }; + let pt = |key: &str| { + style + .split(';') + .filter_map(|d| d.split_once(':')) + .find(|(k, _)| k.trim().eq_ignore_ascii_case(key)) + .and_then(|(_, v)| v.trim().strip_suffix("pt")?.trim().parse::().ok().filter(|n| n.is_finite() && *n > 0.0 && *n < 1e6)) + }; + Some(NoteBox { from: corner(c1, dx1, r1, dy1), to: corner(c2, dx2, r2, dy2), size_pt: pt("width").zip(pt("height")) }) +} + fn read_threaded(cx: &Ctx<'_>, x: &El, sheet: &mut Sheet) { let mut by_id: HashMap = HashMap::new(); let mut replies: Vec<(String, String, String, String)> = vec![]; @@ -925,9 +955,10 @@ fn read_threaded(cx: &Ctx<'_>, x: &El, sheet: &mut Sheet) { by_id.insert(id.to_string(), cell); } let dates = if date.is_empty() { vec![] } else { vec![date] }; - sheet - .comments - .insert(cell, Comment { author, text, replies: vec![], threaded: true, resolved: t.flag("done", false), visible: false, dates }); + sheet.comments.insert( + cell, + Comment { author, text, replies: vec![], threaded: true, resolved: t.flag("done", false), visible: false, dates, note_box: None }, + ); } } } diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index 78e1afaae..3946bc1ca 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -3,10 +3,10 @@ use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::fmt::Write as _; -use gridcraft_core::{CellError, CellRef, RangeRef, Value}; +use gridcraft_core::{CellError, CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value}; use gridcraft_model::{ - Cell, CfOperator, CfRule, CfValueKind, Color, CondFormat, ErrorStyle, FilterCriterion, LineInfo, Orientation, Sheet, SparklineKind, Style, Table, - TotalsFn, Validation, ValidationKind, Workbook, default_icon_thresholds, icon_count, + Cell, CfOperator, CfRule, CfValueKind, Color, CondFormat, ErrorStyle, FilterCriterion, LineInfo, NoteBox, NoteCorner, Orientation, Sheet, + SparklineKind, Style, Table, TotalsFn, Validation, ValidationKind, Workbook, default_icon_thresholds, icon_count, }; use crate::drawing::{Obj, image_content_type, image_ext}; @@ -1260,6 +1260,24 @@ fn protected_ranges_xml(sheet: &Sheet) -> String { if s.is_empty() { s } else { format!("{s}") } } +/// The box a note made here gets: two columns right of its cell, four rows tall, 108 x 59.25 pt. +/// A box read from a file that equals it is kept as `None`, so a note's model survives a save. +pub(crate) const DEFAULT_NOTE_BOX: NoteBox = NoteBox { + from: NoteCorner { col: 1, col_px: 15, row: 0, row_px: 2 }, + to: NoteCorner { col: 3, col_px: 15, row: 4, row_px: 16 }, + size_pt: Some(DEFAULT_NOTE_SIZE_PT), +}; +const DEFAULT_NOTE_SIZE_PT: (f64, f64) = (108.0, 59.25); + +/// A note box's `` text and its width and height in points: the box read from the file +/// or [`DEFAULT_NOTE_BOX`], its corners moved with the note's `cell` and kept on the sheet. +fn note_anchor(cell: CellRef, b: Option<&NoteBox>) -> (String, (f64, f64)) { + let b = b.unwrap_or(&DEFAULT_NOTE_BOX); + let at = |base: u32, d: i64, max: u32| (i64::from(base) + d).clamp(0, i64::from(max) - 1); + let corner = |k: &NoteCorner| format!("{}, {}, {}, {}", at(cell.col, k.col, MAX_COLS), k.col_px, at(cell.row, k.row, MAX_ROWS), k.row_px); + (format!("{}, {}", corner(&b.from), corner(&b.to)), b.size_pt.unwrap_or(DEFAULT_NOTE_SIZE_PT)) +} + /// Legacy VML shapes Excel uses to display notes. fn vml_xml(sheet: &Sheet, n: u32) -> String { let mut s = format!( @@ -1268,14 +1286,13 @@ fn vml_xml(sheet: &Sheet, n: u32) -> String { for (i, (cell, c)) in sheet.comments.iter().enumerate() { let id = n as u64 * 1024 + 1 + i as u64; let vis = if c.visible { "visible" } else { "hidden" }; + let (anchor, (w, h)) = note_anchor(*cell, c.note_box.as_ref()); let _ = write!( s, - "
{}, 15, {}, 2, {}, 15, {}, 16False{}{}{}", + "
{anchor}False{}{}{}", + num(w), + num(h), i + 1, - cell.col + 1, - cell.row, - cell.col + 3, - cell.row + 4, cell.row, cell.col, if c.visible { "" } else { "" } diff --git a/crates/xlsx/src/tests/malformed.rs b/crates/xlsx/src/tests/malformed.rs index d029acaa4..cedcec8dc 100644 --- a/crates/xlsx/src/tests/malformed.rs +++ b/crates/xlsx/src/tests/malformed.rs @@ -130,7 +130,16 @@ fn hostile_threaded_comments_persons_and_note_vml() { use gridcraft_model::{Comment, Workbook}; let mut wb = Workbook::new(); let s = std::sync::Arc::make_mut(&mut wb.sheets[0]); - let c = |threaded| Comment { author: "A".into(), text: "t".into(), replies: vec![], threaded, resolved: false, visible: false, dates: vec![] }; + let c = |threaded| Comment { + author: "A".into(), + text: "t".into(), + replies: vec![], + threaded, + resolved: false, + visible: false, + dates: vec![], + note_box: None, + }; s.comments.insert(CellRef::new(0, 0), c(true)); s.comments.insert(CellRef::new(1, 1), c(false)); let good = write_xlsx(&wb).unwrap(); diff --git a/crates/xlsx/src/tests/mod.rs b/crates/xlsx/src/tests/mod.rs index bdbd66e05..c9185bee7 100644 --- a/crates/xlsx/src/tests/mod.rs +++ b/crates/xlsx/src/tests/mod.rs @@ -8,6 +8,7 @@ mod fixtures; mod ink_roundtrip; mod literal_category_roundtrip; mod malformed; +mod note_box; mod ods_tests; mod passthrough_tests; mod pivot_tests; diff --git a/crates/xlsx/src/tests/note_box.rs b/crates/xlsx/src/tests/note_box.rs new file mode 100644 index 000000000..09a3d68e6 --- /dev/null +++ b/crates/xlsx/src/tests/note_box.rs @@ -0,0 +1,117 @@ +//! #1608: a note's box keeps the place and size the file gave it through open and save. + +use std::io::Read; + +use gridcraft_core::CellRef; +use gridcraft_model::{Comment, NoteBox, NoteCorner, Workbook}; + +use super::minimal; +use crate::{read_xlsx, write_xlsx}; + +const RELS: &str = r#""#; +const COMMENTS: &str = r#"Anote"#; +/// The issue's note on B2: Excel's box over B1, 224.25 x 89.25 pt. +const VML: &str = r##""##; + +fn package() -> Vec { + minimal( + r#"1"#, + &[("xl/worksheets/_rels/sheet1.xml.rels", RELS), ("xl/comments1.xml", COMMENTS), ("xl/drawings/vmlDrawing1.vml", VML)], + "", + "", + ) +} + +/// The written VML part (GridCraft writes one per sheet with notes). +fn written_vml(bytes: &[u8]) -> String { + let mut z = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap(); + let name = (0..z.len()).map(|i| z.by_index(i).unwrap().name().to_string()).find(|n| n.ends_with(".vml")).expect("a VML part"); + let mut s = String::new(); + z.by_name(&name).unwrap().read_to_string(&mut s).unwrap(); + s +} + +fn b2() -> CellRef { + CellRef::new(1, 1) +} + +#[test] +fn a_notes_box_is_read_relative_to_its_cell() { + let (wb, _) = read_xlsx(&package()).unwrap(); + let c = &wb.sheets[0].comments[&b2()]; + assert_eq!( + c.note_box, + Some(NoteBox { + from: NoteCorner { col: 0, col_px: 10, row: -1, row_px: 2 }, + to: NoteCorner { col: 4, col_px: 32, row: 4, row_px: 8 }, + size_pt: Some((224.25, 89.25)), + }) + ); +} + +#[test] +fn an_unedited_notes_box_is_saved_where_and_as_big_as_it_was() { + let (wb, _) = read_xlsx(&package()).unwrap(); + let vml = written_vml(&write_xlsx(&wb).unwrap()); + assert!(vml.contains("1, 10, 0, 2, 5, 32, 5, 8"), "{vml}"); + assert!(vml.contains("width:224.25pt;height:89.25pt"), "{vml}"); + // and once more through GridCraft's own output + let (wb2, _) = read_xlsx(&write_xlsx(&wb).unwrap()).unwrap(); + assert_eq!(wb2.sheets[0].comments[&b2()].note_box, wb.sheets[0].comments[&b2()].note_box); +} + +#[test] +fn a_notes_box_moves_with_its_note() { + // A row and a column inserted before the note move it to C3; its box follows it. + let (mut wb, _) = read_xlsx(&package()).unwrap(); + let s = wb.sheet_mut(0).unwrap(); + let c = s.comments.remove(&b2()).unwrap(); + s.comments.insert(CellRef::new(2, 2), c); + let vml = written_vml(&write_xlsx(&wb).unwrap()); + assert!(vml.contains("2, 10, 1, 2, 6, 32, 6, 8"), "{vml}"); +} + +#[test] +fn a_box_that_would_start_before_the_sheet_is_clamped_to_it() { + // The B2 note's box starts one row above it; moved to A1 that row doesn't exist. + let (mut wb, _) = read_xlsx(&package()).unwrap(); + let s = wb.sheet_mut(0).unwrap(); + let c = s.comments.remove(&b2()).unwrap(); + s.comments.insert(CellRef::new(0, 0), c); + let vml = written_vml(&write_xlsx(&wb).unwrap()); + assert!(vml.contains("0, 10, 0, 2, 4, 32, 4, 8"), "{vml}"); +} + +#[test] +fn a_new_note_gets_the_default_box() { + let mut wb = Workbook::new(); + let note = Comment { + author: "A".into(), + text: "t".into(), + replies: vec![], + threaded: false, + resolved: false, + visible: false, + dates: vec![], + note_box: None, + }; + wb.sheet_mut(0).unwrap().comments.insert(b2(), note); + let vml = written_vml(&write_xlsx(&wb).unwrap()); + assert!(vml.contains("2, 15, 1, 2, 4, 15, 5, 16") && vml.contains("width:108pt;height:59.25pt"), "{vml}"); +} + +#[test] +fn a_damaged_anchor_is_ignored() { + for anchor in ["1, 10, 0", "1, x, 0, 2, 5, 32, 5, 8", "-1, 10, 0, 2, 5, 32, 5, 8", "99999999999, 0, 0, 0, 0, 0, 0, 0"] { + let vml = VML.replace("\n 1, 10, 0, 2, 5, 32, 5, 8", anchor); + let bytes = minimal( + r#""#, + &[("xl/worksheets/_rels/sheet1.xml.rels", RELS), ("xl/comments1.xml", COMMENTS), ("xl/drawings/vmlDrawing1.vml", &vml)], + "", + "", + ); + let (wb, _) = read_xlsx(&bytes).unwrap(); + assert_eq!(wb.sheets[0].comments[&b2()].note_box, None, "{anchor}"); + } +} diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 7c7c19056..c2241fb6f 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -217,11 +217,21 @@ fn sample() -> Workbook { resolved: false, visible: false, dates: vec![], + note_box: None, }, ); s.comments.insert( at("C3"), - Comment { author: "Bob".into(), text: "second".into(), replies: vec![], threaded: false, resolved: false, visible: true, dates: vec![] }, + Comment { + author: "Bob".into(), + text: "second".into(), + replies: vec![], + threaded: false, + resolved: false, + visible: true, + dates: vec![], + note_box: None, + }, ); s.comments.insert( at("D4"), @@ -233,11 +243,21 @@ fn sample() -> Workbook { resolved: true, visible: false, dates: vec!["2026-10-11T09:30:00.00".into(), String::new(), "2026-10-11T10:15:00.00".into()], + note_box: None, }, ); s.comments.insert( at("E5"), - Comment { author: "Cy".into(), text: "open thread".into(), replies: vec![], threaded: true, resolved: false, visible: false, dates: vec![] }, + Comment { + author: "Cy".into(), + text: "open thread".into(), + replies: vec![], + threaded: true, + resolved: false, + visible: false, + dates: vec![], + note_box: None, + }, ); s.autofilter = Some(AutoFilter { range: rr("K1:N20"), @@ -436,7 +456,16 @@ fn sample() -> Workbook { s3.visibility = Visibility::VeryHidden; s3.comments.insert( at("A1"), - Comment { author: "Bob".into(), text: "other sheet".into(), replies: vec![], threaded: true, resolved: false, visible: false, dates: vec![] }, + Comment { + author: "Bob".into(), + text: "other sheet".into(), + replies: vec![], + threaded: true, + resolved: false, + visible: false, + dates: vec![], + note_box: None, + }, ); wb.sheets.push(Arc::new(s3)); wb.names.push(DefinedName { name: "Rate".into(), scope: None, formula: "0.07".into(), comment: "c".into(), hidden: false }); @@ -918,6 +947,7 @@ fn bad_comment_dates_are_left_out() { resolved: false, visible: false, dates: vec!["\"/>