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!["\"/>