From 46b2fe2f7a85763afbb6f6718805753f94d80239 Mon Sep 17 00:00:00 2001 From: CouchRiv <270190108+CouchRiv@users.noreply.github.com> Date: Mon, 12 Oct 2026 22:50:48 -0400 Subject: [PATCH 01/28] Row, column and freeze commands act on the sheet they are given view.freezePanes, home.rowHeight, home.columnWidth, the AutoFit, Hide and Unhide row/column commands and home.defaultWidth always changed the active sheet: a `sheet` parameter was silently ignored, so a programmatic caller laying out a sheet that isn't active changed the wrong one, and `Sheet!` prefixes on `rows`/`cols` were refused as "not a range". They now take `sheet` (name or index) like the commands that already use target_sheet, accept a `Sheet!` prefix on `rows`/`cols` (and on Freeze Panes' `cell`, which was refused), and refuse a prefix that contradicts `sheet`. The selection belongs to the active sheet, so another sheet needs explicit lines or a cell. Freeze Top Row, Freeze First Column and Unfreeze Panes take `sheet` too. Without `sheet`, nothing changes. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/format.rs | 75 +++++++++++++++++++++------------ crates/engine/src/cmd/view.rs | 34 ++++++++++----- crates/engine/src/tests.rs | 41 ++++++++++++++++++ crates/mcp/src/tests.rs | 6 ++- 4 files changed, 116 insertions(+), 40 deletions(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index dde5fa120..0c0c546aa 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -156,7 +156,7 @@ pub fn specs() -> Vec { "Row Height…", ["Home", "Cells", "Format"], None, - "{rows?: \"2:5\" | \"2:2,4:5\", height: 20 (points on screen)}", + "{sheet?, rows?: \"2:5\" | \"2:2,4:5\" | \"Sheet2!2:5\", height: 20 (points on screen)}", has_doc, row_height ), @@ -165,28 +165,34 @@ pub fn specs() -> Vec { "Column Width…", ["Home", "Cells", "Format"], None, - "{cols?: \"B:D\" | \"B:B,D:F\", width: 64 (points) | chars: 8.43}", + "{sheet?, cols?: \"B:D\" | \"B:B,D:F\" | \"Sheet2!B:D\", width: 64 (points) | chars: 8.43}", has_doc, col_width ), - cmd!("home.autofitRowHeight", "AutoFit Row Height", ["Home", "Cells", "Format"], None, "{rows?}", has_doc, autofit_rows), - cmd!("home.autofitColumnWidth", "AutoFit Column Width", ["Home", "Cells", "Format"], None, "{cols?}", has_doc, autofit_cols), - cmd!("home.defaultWidth", "Default Width…", ["Home", "Cells", "Format"], None, "{width: 64}", has_doc, default_width), - cmd!("home.hideRows", "Hide Rows", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+9"), "{rows?}", has_doc, |s, p| hide( + cmd!("home.autofitRowHeight", "AutoFit Row Height", ["Home", "Cells", "Format"], None, "{sheet?, rows?}", has_doc, autofit_rows), + cmd!("home.autofitColumnWidth", "AutoFit Column Width", ["Home", "Cells", "Format"], None, "{sheet?, cols?}", has_doc, autofit_cols), + cmd!("home.defaultWidth", "Default Width…", ["Home", "Cells", "Format"], None, "{sheet?, width: 64}", has_doc, default_width), + cmd!("home.hideRows", "Hide Rows", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+9"), "{sheet?, rows?}", has_doc, |s, p| hide( s, p, true, true )), - cmd!("home.hideColumns", "Hide Columns", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+0"), "{cols?}", has_doc, |s, p| hide( - s, p, false, true - )), - cmd!("home.unhideRows", "Unhide Rows", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+Shift+9"), "{rows?}", has_doc, |s, p| hide( - s, p, true, false - )), + cmd!("home.hideColumns", "Hide Columns", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+0"), "{sheet?, cols?}", has_doc, |s, p| { + hide(s, p, false, true) + }), + cmd!( + "home.unhideRows", + "Unhide Rows", + ["Home", "Cells", "Format", "Hide & Unhide"], + Some("Cmd+Shift+9"), + "{sheet?, rows?}", + has_doc, + |s, p| hide(s, p, true, false) + ), cmd!( "home.unhideColumns", "Unhide Columns", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+Shift+0"), - "{cols?}", + "{sheet?, cols?}", has_doc, |s, p| hide(s, p, false, false) ), @@ -921,19 +927,37 @@ fn cell_style(s: &mut Session, p: &Json) -> Result { // ---------------------------------------------------------------- sizes -fn line_targets(s: &Session, p: &Json, rows: bool) -> Result> { +/// The sheet and the row (or column) spans a size or visibility command works on. The lines are +/// `rows`/`cols` (`"2:5"`, `"B:B,D:F"`, optionally `Sheet!`-prefixed), else the selection's; the +/// sheet is `sheet`, else the prefix, else the active one. The selection belongs to the active +/// sheet, so another sheet needs explicit lines. +fn line_targets(s: &Session, p: &Json, rows: bool) -> Result<(usize, Vec<(u32, u32)>)> { let key = if rows { "rows" } else { "cols" }; + let mut sheet = target_sheet(s, p)?; if let Some(t) = str_param(p, key) { + let (prefix, body) = split_sheet(t); + if let Some(name) = prefix { + let named = s.doc()?.wb.sheet_index(&name).ok_or_else(|| bad(key, format!("no sheet named `{name}`")))?; + if p.get("sheet").is_some() && named != sheet { + return Err(bad(key, format!("`{t}` names a sheet other than `sheet`"))); + } + sheet = named; + } // A comma-separated list ("B:B,D:F") targets several areas, like a multi-area selection. - return t + let lines = body .split(',') .map(|part| { let r = RangeRef::parse(part).ok_or_else(|| bad(key, format!("not a range: {part}")))?; Ok(if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }) }) - .collect(); + .collect::>>()?; + return Ok((sheet, lines)); } - Ok(s.doc()?.selection.ranges.iter().map(|r| if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }).collect()) + let d = s.doc()?; + if sheet != d.wb.active_sheet { + return Err(bad(key, format!("missing `{key}`: the selection is on the active sheet"))); + } + Ok((sheet, d.selection.ranges.iter().map(|r| if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }).collect())) } fn row_height(s: &mut Session, p: &Json) -> Result { @@ -941,9 +965,8 @@ fn row_height(s: &mut Session, p: &Json) -> Result { if !(0.0..=546.0).contains(&h) { return Err(EngineError::Other("Row height must be between 0 and 409 points.".into())); } - let lines = line_targets(s, p, true)?; + let (si, lines) = line_targets(s, p, true)?; edit(s, |cx| { - let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; for (a, b) in lines { for r in a..=b.min(a.saturating_add(1_048_575)) { @@ -966,9 +989,8 @@ fn col_width(s: &mut Session, p: &Json) -> Result { if !(0.0..=1800.0).contains(&w) { return Err(EngineError::Other("Column width must be between 0 and 255 characters.".into())); } - let lines = line_targets(s, p, false)?; + let (si, lines) = line_targets(s, p, false)?; edit(s, |cx| { - let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; for (a, b) in lines { for c in a..=b { @@ -1015,9 +1037,8 @@ pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { } fn autofit_cols(s: &mut Session, p: &Json) -> Result { - let lines = line_targets(s, p, false)?; + let (si, lines) = line_targets(s, p, false)?; edit(s, |cx| { - let si = cx.sheet_index(); let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) }; let mut widths = Vec::new(); let used = sh.used_range(); @@ -1054,9 +1075,8 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { } fn autofit_rows(s: &mut Session, p: &Json) -> Result { - let lines = line_targets(s, p, true)?; + let (si, lines) = line_targets(s, p, true)?; edit(s, |cx| { - let si = cx.sheet_index(); for (a, b) in lines { let b = b.min(a + 100_000); if let Some(sh) = cx.wb.sheet_mut(si) { @@ -1075,17 +1095,16 @@ fn autofit_rows(s: &mut Session, p: &Json) -> Result { fn default_width(s: &mut Session, p: &Json) -> Result { let w = f64_param(p, "width").ok_or_else(|| bad("home.defaultWidth", "missing `width`"))?; + let si = target_sheet(s, p)?; edit(s, |cx| { - let si = cx.sheet_index(); cx.sheet_mut(si)?.default_col_width = w.clamp(0.0, 1800.0) as f32; Ok(Json::Null) }) } fn hide(s: &mut Session, p: &Json, rows: bool, on: bool) -> Result { - let lines = line_targets(s, p, rows)?; + let (si, lines) = line_targets(s, p, rows)?; edit(s, |cx| { - let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; for (a, b) in lines { // Unhiding a single selected line also reveals hidden neighbours (Excel needs the diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index edcacf57d..8c9389543 100644 --- a/crates/engine/src/cmd/view.rs +++ b/crates/engine/src/cmd/view.rs @@ -13,16 +13,25 @@ pub fn specs() -> Vec { "Freeze Panes", ["View", "Window", "Freeze Panes"], None, - "{cell?: \"B2\" (rows above and columns left of it freeze)}", + "{sheet?, cell?: \"B2\" (rows above and columns left of it freeze); without `cell`, unfreezes a frozen sheet}", has_doc, freeze_panes ), - cmd!("view.freezeTopRow", "Freeze Top Row", ["View", "Window", "Freeze Panes"], None, "{}", has_doc, |s, _| set_freeze(s, Some((1, 0)))), - cmd!("view.freezeFirstColumn", "Freeze First Column", ["View", "Window", "Freeze Panes"], None, "{}", has_doc, |s, _| set_freeze( + cmd!("view.freezeTopRow", "Freeze Top Row", ["View", "Window", "Freeze Panes"], None, "{sheet?}", has_doc, |s, p| set_freeze( s, + target_sheet(s, p)?, + Some((1, 0)) + )), + cmd!("view.freezeFirstColumn", "Freeze First Column", ["View", "Window", "Freeze Panes"], None, "{sheet?}", has_doc, |s, p| set_freeze( + s, + target_sheet(s, p)?, Some((0, 1)) )), - cmd!("view.unfreezePanes", "Unfreeze Panes", ["View", "Window", "Freeze Panes"], None, "{}", has_doc, |s, _| set_freeze(s, None)), + cmd!("view.unfreezePanes", "Unfreeze Panes", ["View", "Window", "Freeze Panes"], None, "{sheet?}", has_doc, |s, p| set_freeze( + s, + target_sheet(s, p)?, + None + )), cmd!("view.gridlines", "Gridlines", ["View", "Show"], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "gridlines")), cmd!("view.headings", "Headings", ["View", "Show"], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "headings")), cmd!("view.showZeros", "Show Zeros", [], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "zeros")), @@ -113,23 +122,28 @@ pub fn specs() -> Vec { } fn freeze_panes(s: &mut Session, p: &Json) -> Result { + let si = target_sheet(s, p)?; let d = s.doc()?; - let sh = d.wb.active().ok_or(EngineError::NoDocument)?; + let sh = d.wb.sheet(si).ok_or(EngineError::NoDocument)?; if sh.freeze.is_some() && p.get("cell").is_none() { - return set_freeze(s, None); + return set_freeze(s, si, None); } - let at = cell_param(s, p, "cell")?.unwrap_or(d.selection.active); + let at = match cell_param_on(s, p, "cell", si)? { + Some(c) => c, + None if si == d.wb.active_sheet => d.selection.active, + // The selection belongs to the active sheet. + None => return Err(bad("view.freezePanes", "missing `cell`: the selection is on the active sheet")), + }; let top = sh.view_top_left; let rows = at.row.saturating_sub(top.row); let cols = at.col.saturating_sub(top.col); let _ = (rows, cols); let (rows, cols) = (at.row, at.col); - set_freeze(s, if rows == 0 && cols == 0 { None } else { Some((rows, cols)) }) + set_freeze(s, si, if rows == 0 && cols == 0 { None } else { Some((rows, cols)) }) } -fn set_freeze(s: &mut Session, f: Option<(u32, u32)>) -> Result { +fn set_freeze(s: &mut Session, si: usize, f: Option<(u32, u32)>) -> Result { edit(s, |cx| { - let si = cx.sheet_index(); cx.sheet_mut(si)?.freeze = f; Ok(json!({"freeze": f})) }) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 33fdc113e..5db59594d 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2636,6 +2636,47 @@ fn row_height_and_column_width_take_a_list_of_areas() { assert_eq!(s.doc().unwrap().wb.active().unwrap().col_width(1), 120.0, "a bad list changes nothing"); } +/// Programmatic callers lay out a sheet that isn't the active one: `sheet` (or a `Sheet!` prefix on +/// `rows`, `cols` or `cell`) picks it, instead of being ignored in favour of the active sheet. +#[test] +fn line_sizes_and_freeze_panes_take_a_sheet() { + let mut s = s(); + s.execute("home.insertSheet", json!({"name": "Data"})).unwrap(); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + let sheet = |s: &Session, i: usize| s.doc().unwrap().wb.sheets[i].clone(); + let before = sheet(&s, 0); + + s.execute("view.freezePanes", json!({"sheet": "Data", "cell": "A2"})).unwrap(); + s.execute("home.columnWidth", json!({"sheet": "Data", "cols": "A:A", "width": 150})).unwrap(); + s.execute("home.columnWidth", json!({"cols": "Data!B:B,D:D", "chars": 20})).unwrap(); + s.execute("home.rowHeight", json!({"sheet": 1, "rows": "1:1", "height": 40})).unwrap(); + s.execute("home.hideColumns", json!({"sheet": "Data", "cols": "E:E"})).unwrap(); + s.execute("home.defaultWidth", json!({"sheet": "Data", "width": 80})).unwrap(); + let data = sheet(&s, 1); + assert_eq!(data.freeze, Some((1, 0))); + assert_eq!((data.col_width(0), data.col_width(1), data.col_width(3)), (150.0, 145.0, 145.0)); + assert_eq!(data.row_height(0), 40.0); + assert!(data.cols.get(&4).is_some_and(|c| c.hidden)); + assert_eq!(data.default_col_width, 80.0); + assert_eq!(sheet(&s, 0), before, "the active sheet is untouched"); + assert_eq!(s.doc().unwrap().wb.active_sheet, 0); + + s.execute("view.freezePanes", json!({"cell": "Data!B3"})).unwrap(); + assert_eq!(sheet(&s, 1).freeze, Some((2, 1))); + s.execute("view.unfreezePanes", json!({"sheet": "Data"})).unwrap(); + s.execute("view.freezeTopRow", json!({"sheet": "Data"})).unwrap(); + assert_eq!(sheet(&s, 1).freeze, Some((1, 0))); + + // The selection belongs to the active sheet: another sheet needs explicit lines or a cell, and + // a prefix can't contradict `sheet`. + assert!(s.execute("home.columnWidth", json!({"sheet": "Data", "width": 10})).is_err()); + s.execute("view.unfreezePanes", json!({"sheet": "Data"})).unwrap(); + assert!(s.execute("view.freezePanes", json!({"sheet": "Data"})).is_err()); + assert!(s.execute("home.columnWidth", json!({"sheet": "Sheet1", "cols": "Data!B:B", "width": 10})).is_err()); + assert!(s.execute("home.rowHeight", json!({"rows": "Nope!1:1", "height": 10})).is_err()); + assert_eq!(sheet(&s, 0), before); +} + #[test] fn copying_areas_that_share_rows_or_columns_pastes_them_as_one_block() { // Issue #348: Excel slides lined-up areas together; formulas travel as their values. diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index eb1f20503..ce915d1c1 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -463,7 +463,6 @@ fn other_sheet_prefix_is_error() { ok(&mut s, "execute_command", json!({"command": "sheet.activate", "params": {"sheet": 0}})); fails(&mut s, "set_cell", json!({"cell": "Nope!B2", "input": "x"})); // A command that cannot honour a sheet prefix must not drop it and use the active sheet. - cmd_fails(&mut s, "view.freezePanes", json!({"cell": "Other!B2"})); cmd_fails(&mut s, "formulas.tracePrecedents", json!({"cell": "Other!B2"})); ok(&mut s, "set_cell", json!({"cell": "Sheet1!B2", "input": "here"})); assert_eq!(ok(&mut s, "get_cell", json!({"cell": "B2"}))["text"], "here"); @@ -495,7 +494,10 @@ fn cross_sheet_cell_refs_resolve() { // The active sheet may be a quoted name with spaces. ok(&mut s, "execute_command", json!({"command": "sheet.activate", "params": {"sheet": "My Sheet"}})); ok(&mut s, "execute_command", json!({"command": "view.freezePanes", "params": {"cell": "'My Sheet'!B2"}})); - cmd_fails(&mut s, "view.freezePanes", json!({"cell": "Other!B2"})); + // Freeze Panes honours another sheet's prefix (or `sheet`) and leaves the active sheet alone. + ok(&mut s, "execute_command", json!({"command": "view.freezePanes", "params": {"cell": "Other!C4"}})); + let sheets = ok(&mut s, "inspect_workbook", json!({}))["sheets"].clone(); + assert_eq!((sheets[1]["freeze"].clone(), sheets[2]["freeze"].clone()), (json!([3, 2]), json!([1, 1])), "{sheets}"); // A prefix naming a different sheet than the `sheet` argument is an error, not resolved in favour of either. cmd_fails(&mut s, "cell.get", json!({"cell": "Other!B3", "sheet": "Sheet1"})); } From 9a816f80f7c635e05f8b5054bacae01385e3b09d Mon Sep 17 00:00:00 2001 From: CouchRiv <270190108+CouchRiv@users.noreply.github.com> Date: Mon, 12 Oct 2026 22:54:25 -0400 Subject: [PATCH 02/28] Format: Row Height and Column Width say pixels, and take points like Excel home.rowHeight and home.columnWidth take pixels at 96 dpi, the model's unit (header drags and the Row Height dialog send pixels), but their parameter docs said points. An agent asking for a 30 pt row got `ht="22.5"` in the file, and the range check accepted 500 while its message said "between 0 and 409 points". - The params docs say pixels: `height: 20 (pixels) | points: 15`, `width: 64 (pixels) | chars: 8.43`, and `{width: 64 (pixels)}` for home.defaultWidth. Pixel values behave as before. - home.rowHeight takes `points`, Excel's unit (Row Height dialog, `ht` in the file), as home.columnWidth already takes `chars`. - Range errors give the limit in the unit passed: 546 pixels (409.5 points), 409.5 points, 1800 pixels, 255 characters. `chars` is checked against 255, Excel's limit and the widest column a file holds (it went to 256.4 through the pixel limit). - The model's size comments say pixels. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/format.rs | 49 +++++++++++++++++++++++---------- crates/engine/src/tests.rs | 42 ++++++++++++++++++++++++++++ crates/model/src/features.rs | 2 +- crates/model/src/sheet.rs | 4 +-- 4 files changed, 80 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index dde5fa120..5b550d480 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -156,7 +156,7 @@ pub fn specs() -> Vec { "Row Height…", ["Home", "Cells", "Format"], None, - "{rows?: \"2:5\" | \"2:2,4:5\", height: 20 (points on screen)}", + "{rows?: \"2:5\" | \"2:2,4:5\", height: 20 (pixels) | points: 15}", has_doc, row_height ), @@ -165,13 +165,13 @@ pub fn specs() -> Vec { "Column Width…", ["Home", "Cells", "Format"], None, - "{cols?: \"B:D\" | \"B:B,D:F\", width: 64 (points) | chars: 8.43}", + "{cols?: \"B:D\" | \"B:B,D:F\", width: 64 (pixels) | chars: 8.43}", has_doc, col_width ), cmd!("home.autofitRowHeight", "AutoFit Row Height", ["Home", "Cells", "Format"], None, "{rows?}", has_doc, autofit_rows), cmd!("home.autofitColumnWidth", "AutoFit Column Width", ["Home", "Cells", "Format"], None, "{cols?}", has_doc, autofit_cols), - cmd!("home.defaultWidth", "Default Width…", ["Home", "Cells", "Format"], None, "{width: 64}", has_doc, default_width), + cmd!("home.defaultWidth", "Default Width…", ["Home", "Cells", "Format"], None, "{width: 64 (pixels)}", has_doc, default_width), cmd!("home.hideRows", "Hide Rows", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+9"), "{rows?}", has_doc, |s, p| hide( s, p, true, true )), @@ -936,11 +936,25 @@ fn line_targets(s: &Session, p: &Json, rows: bool) -> Result> { Ok(s.doc()?.selection.ranges.iter().map(|r| if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }).collect()) } +/// Sizes are pixels at 96 dpi, the model's unit (header drags send pixels). `points` (row height) +/// and `chars` (column width) take Excel's units, those of its Row Height and Column Width dialogs +/// and of the file (`ht` is in points). fn row_height(s: &mut Session, p: &Json) -> Result { - let h = f64_param(p, "height").ok_or_else(|| bad("home.rowHeight", "missing `height`"))?; - if !(0.0..=546.0).contains(&h) { - return Err(EngineError::Other("Row height must be between 0 and 409 points.".into())); - } + let h = match (f64_param(p, "height"), f64_param(p, "points")) { + (Some(h), _) => { + if !(0.0..=546.0).contains(&h) { + return Err(EngineError::Other("Row height must be between 0 and 546 pixels (409.5 points).".into())); + } + h + } + (None, Some(pt)) => { + if !(0.0..=409.5).contains(&pt) { + return Err(EngineError::Other("Row height must be between 0 and 409.5 points.".into())); + } + pt * 4.0 / 3.0 + } + _ => return Err(bad("home.rowHeight", "missing `height` (pixels) or `points`")), + }; let lines = line_targets(s, p, true)?; edit(s, |cx| { let si = cx.sheet_index(); @@ -959,13 +973,20 @@ fn row_height(s: &mut Session, p: &Json) -> Result { fn col_width(s: &mut Session, p: &Json) -> Result { let w = match (f64_param(p, "width"), f64_param(p, "chars")) { - (Some(w), _) => w, - (None, Some(c)) => chars_to_points(c), - _ => return Err(bad("home.columnWidth", "missing `width`")), + (Some(w), _) => { + if !(0.0..=1800.0).contains(&w) { + return Err(EngineError::Other("Column width must be between 0 and 1800 pixels.".into())); + } + w + } + (None, Some(c)) => { + if !(0.0..=255.0).contains(&c) { + return Err(EngineError::Other("Column width must be between 0 and 255 characters.".into())); + } + chars_to_points(c) + } + _ => return Err(bad("home.columnWidth", "missing `width` (pixels) or `chars`")), }; - if !(0.0..=1800.0).contains(&w) { - return Err(EngineError::Other("Column width must be between 0 and 255 characters.".into())); - } let lines = line_targets(s, p, false)?; edit(s, |cx| { let si = cx.sheet_index(); @@ -981,7 +1002,7 @@ fn col_width(s: &mut Session, p: &Json) -> Result { }) } -/// Excel column width in characters (of the default font's digit width, 7 px) → points. +/// Excel column width in characters (of the default font's digit width, 7 px) → pixels. pub fn chars_to_points(c: f64) -> f64 { if c <= 0.0 { 0.0 diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 33fdc113e..2ffb096fb 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2636,6 +2636,48 @@ fn row_height_and_column_width_take_a_list_of_areas() { assert_eq!(s.doc().unwrap().wb.active().unwrap().col_width(1), 120.0, "a bad list changes nothing"); } +#[test] +fn row_height_and_column_width_take_pixels_or_excel_units() { + // `height` and `width` are pixels at 96 dpi (the model's unit); `points` and `chars` are + // Excel's units, those of its Row Height and Column Width dialogs. A row is saved in points. + let mut s = s(); + s.execute("home.rowHeight", json!({"rows": "1:1", "points": 30})).unwrap(); + s.execute("home.rowHeight", json!({"rows": "2:2", "height": 30})).unwrap(); + s.execute("home.rowHeight", json!({"rows": "3:3", "points": 409.5})).unwrap(); + s.execute("home.columnWidth", json!({"cols": "A:A", "chars": 8.43})).unwrap(); + s.execute("home.columnWidth", json!({"cols": "B:B", "width": 100})).unwrap(); + let sheet = s.doc().unwrap().wb.active().unwrap(); + assert_eq!([sheet.row_height(0), sheet.row_height(1), sheet.row_height(2)], [40.0, 30.0, 546.0]); + assert_eq!([sheet.col_width(0), sheet.col_width(1)], [64.0, 100.0]); + let bytes = gridcraft_xlsx::write_xlsx(&s.doc().unwrap().wb).unwrap(); + let mut zip = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap(); + let mut xml = String::new(); + std::io::Read::read_to_string(&mut zip.by_name("xl/worksheets/sheet1.xml").unwrap(), &mut xml).unwrap(); + for row in [r#" u64 { /// Outline grouping info for a row or column. #[derive(Clone, Copy, Debug, Default, PartialEq, Serialize, Deserialize)] pub struct LineInfo { - /// Size in points; `None` = default (rows auto-fit to content). + /// Size in pixels at 96 dpi; `None` = default (rows auto-fit to content). pub size: Option, pub hidden: bool, pub outline: u8, diff --git a/crates/model/src/sheet.rs b/crates/model/src/sheet.rs index e05d3bfb5..5b5d1e294 100644 --- a/crates/model/src/sheet.rs +++ b/crates/model/src/sheet.rs @@ -64,7 +64,7 @@ pub struct Sheet { pub spill_ranges: BTreeMap, pub cols: BTreeMap, pub rows: BTreeMap, - /// Default column width and row height in points. + /// Default column width and row height in pixels at 96 dpi (a file stores row heights in points). pub default_col_width: f32, pub default_row_height: f32, pub merges: Vec, @@ -113,7 +113,7 @@ pub struct Sheet { pub preserved: Option>, } -/// Default width of a column (points) and height of a row (points) for the default font. +/// Default width of a column and height of a row (pixels at 96 dpi) for the default font. pub const DEFAULT_COL_WIDTH: f32 = 64.0; pub const DEFAULT_ROW_HEIGHT: f32 = 20.0; From 7b19b74a5964f428083f1d3c13281407d523a8f6 Mon Sep 17 00:00:00 2001 From: CouchRiv <270190108+CouchRiv@users.noreply.github.com> Date: Mon, 12 Oct 2026 23:37:45 -0400 Subject: [PATCH 03/28] Size commands: a null `sheet` is no `sheet` `{"sheet": null, "cols": "Data!B:B"}` was refused as naming a sheet other than `sheet`, while `target_sheet` and the cell parameters read a null `sheet` as absent. The prefix now decides, as it does when `sheet` is left out. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/format.rs | 2 +- crates/engine/src/tests.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index 0c0c546aa..ee178c0d3 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -938,7 +938,7 @@ fn line_targets(s: &Session, p: &Json, rows: bool) -> Result<(usize, Vec<(u32, u let (prefix, body) = split_sheet(t); if let Some(name) = prefix { let named = s.doc()?.wb.sheet_index(&name).ok_or_else(|| bad(key, format!("no sheet named `{name}`")))?; - if p.get("sheet").is_some() && named != sheet { + if p.get("sheet").is_some_and(|v| !v.is_null()) && named != sheet { return Err(bad(key, format!("`{t}` names a sheet other than `sheet`"))); } sheet = named; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 5db59594d..339d11b2a 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2649,12 +2649,14 @@ fn line_sizes_and_freeze_panes_take_a_sheet() { s.execute("view.freezePanes", json!({"sheet": "Data", "cell": "A2"})).unwrap(); s.execute("home.columnWidth", json!({"sheet": "Data", "cols": "A:A", "width": 150})).unwrap(); s.execute("home.columnWidth", json!({"cols": "Data!B:B,D:D", "chars": 20})).unwrap(); + // A null `sheet` is no `sheet`, as everywhere else: the prefix decides. + s.execute("home.columnWidth", json!({"sheet": null, "cols": "Data!C:C", "chars": 20})).unwrap(); s.execute("home.rowHeight", json!({"sheet": 1, "rows": "1:1", "height": 40})).unwrap(); s.execute("home.hideColumns", json!({"sheet": "Data", "cols": "E:E"})).unwrap(); s.execute("home.defaultWidth", json!({"sheet": "Data", "width": 80})).unwrap(); let data = sheet(&s, 1); assert_eq!(data.freeze, Some((1, 0))); - assert_eq!((data.col_width(0), data.col_width(1), data.col_width(3)), (150.0, 145.0, 145.0)); + assert_eq!((data.col_width(0), data.col_width(1), data.col_width(2), data.col_width(3)), (150.0, 145.0, 145.0, 145.0)); assert_eq!(data.row_height(0), 40.0); assert!(data.cols.get(&4).is_some_and(|c| c.hidden)); assert_eq!(data.default_col_width, 80.0); From 9939819ee335ef790fba762ddb1e47e971e2a602 Mon Sep 17 00:00:00 2001 From: nihilist-narwhal <240798405+nihilist-narwhal@users.noreply.github.com> Date: Sat, 10 Oct 2026 03:03:55 -0500 Subject: [PATCH 04/28] rows/cols parameters: accept a single row or column home.insertRows {"rows":"3"} and home.insertColumns {"cols":"B"} failed with "not a range": only spans such as 3:3 or B:B parsed. The same went for deleting, hiding, sizing and grouping rows and columns. A single row number or column letter now means that one line. --- crates/engine/src/cmd/data.rs | 18 ++++++++++++++++-- crates/engine/src/cmd/format.rs | 2 +- crates/engine/src/cmd/mod.rs | 5 +++++ crates/engine/src/cmd/sheet.rs | 2 +- 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..3a2e476ac 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -1127,10 +1127,10 @@ fn circle_invalid(s: &mut Session, _: &Json) -> Result { // ---------------------------------------------------------------- outline fn outline_lines(s: &Session, p: &Json) -> Result<(bool, u32, u32)> { - if let Some(r) = str_param(p, "rows").and_then(RangeRef::parse) { + if let Some(r) = str_param(p, "rows").and_then(parse_lines) { return Ok((true, r.start.row, r.end.row)); } - if let Some(r) = str_param(p, "cols").and_then(RangeRef::parse) { + if let Some(r) = str_param(p, "cols").and_then(parse_lines) { return Ok((false, r.start.col, r.end.col)); } let r = s.doc()?.selection.current(); @@ -1493,6 +1493,20 @@ mod tests { } } + #[test] + fn single_row_and_column_parameters() { + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [[1, 2], [3, 4]]})).unwrap(); + s.execute("home.insertRows", json!({"rows": "2"})).unwrap(); + s.execute("home.insertColumns", json!({"cols": "B"})).unwrap(); + let v = |s: &mut Session, a: &str| s.execute("cell.get", json!({"cell": a})).unwrap()["value"].clone(); + assert_eq!(v(&mut s, "C3"), json!(4.0)); + s.execute("home.hideRows", json!({"rows": "1"})).unwrap(); + s.execute("data.group", json!({"cols": "C"})).unwrap(); + assert!(s.execute("home.deleteRows", json!({"rows": "0"})).is_err()); + } + #[test] fn split() { assert_eq!(split_delimited("a,b,,c", &[','], false, '"'), ["a", "b", "", "c"]); diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index 4712351ca..b7f2e13f8 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -940,7 +940,7 @@ fn line_targets(s: &Session, p: &Json, rows: bool) -> Result> { return t .split(',') .map(|part| { - let r = RangeRef::parse(part).ok_or_else(|| bad(key, format!("not a range: {part}")))?; + let r = parse_lines(part).ok_or_else(|| bad(key, format!("not a range: {part}")))?; Ok(if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }) }) .collect(); diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 50865a2b7..2be06318c 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -246,6 +246,11 @@ pub(crate) fn target_ranges(s: &Session, p: &Json) -> Result> { Ok(s.doc()?.selection.ranges.clone()) } +/// A `rows`/`cols` parameter: `3:5` and `B:C`, or a single `3` or `B`. +pub(crate) fn parse_lines(t: &str) -> Option { + RangeRef::parse(t).or_else(|| if t.contains(':') { None } else { RangeRef::parse(&format!("{t}:{t}")) }) +} + pub(crate) fn target_range(s: &Session, p: &Json) -> Result { Ok(target_ranges(s, p)?.first().copied().unwrap_or_default()) } diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index 7870dee4e..a81634e9a 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -63,7 +63,7 @@ pub fn specs() -> Vec { fn lines(s: &Session, p: &Json, axis: Axis) -> Result<(u32, u32)> { let key = if axis == Axis::Rows { "rows" } else { "cols" }; let r = match str_param(p, key).or_else(|| str_param(p, "range")) { - Some(t) => RangeRef::parse(t).ok_or_else(|| bad(key, format!("not a range: {t}")))?, + Some(t) => parse_lines(t).ok_or_else(|| bad(key, format!("not a range: {t}")))?, None => s.doc()?.selection.current(), }; let (a, b) = if axis == Axis::Rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }; From be1af2d6ac84693849f3ea0adf9f898434428dc9 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 12:08:41 +0200 Subject: [PATCH 05/28] Sort: a Sheet! prefix on the range sorts that sheet, not the active one (fixes #1283) --- crates/engine/src/cmd/data.rs | 32 ++++++++++++++++++++++++-------- crates/engine/src/tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..508ceb465 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -182,19 +182,36 @@ struct SortKey { list: Option>, } +/// The sheet and range a sort works on: the sheet `sheet` or a `Sheet!` prefix on `range` names, +/// else the active one. The selection belongs to the active sheet, so another sheet needs `range`. +fn sort_target(s: &Session, p: &Json) -> Result<(usize, RangeRef)> { + let sheet = target_sheet(s, p)?; + let d = s.doc()?; + if let Some(t) = str_param(p, "range") + && let (Some(name), _) = split_sheet(t) + && d.wb.sheet_index(&name) != Some(sheet) + { + return Err(bad("data.sort", format!("`{t}` names a sheet other than `sheet`"))); + } + if sheet != d.wb.active_sheet && p.get("range").is_none() { + return Err(bad("data.sort", "missing `range`: the selection is on the active sheet")); + } + Ok((sheet, data_range(s, p)?)) +} + fn quick_sort(s: &mut Session, p: &Json, asc: bool) -> Result { - let r = data_range(s, p)?; + let (sheet, r) = sort_target(s, p)?; let col = col_param(p.get("column"), r.start.col).unwrap_or(s.doc()?.selection.active.col).clamp(r.start.col, r.end.col); let d = s.doc()?; - let sh = d.wb.active().ok_or(EngineError::NoDocument)?; + let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; let header = guess_header(&d.wb, sh, r); - do_sort(s, r, header, vec![SortKey { col, desc: !asc, by: "values".into(), list: None }], false, false) + do_sort(s, sheet, r, header, vec![SortKey { col, desc: !asc, by: "values".into(), list: None }], false, false) } fn sort(s: &mut Session, p: &Json) -> Result { - let r = data_range(s, p)?; + let (sheet, r) = sort_target(s, p)?; let d = s.doc()?; - let sh = d.wb.active().ok_or(EngineError::NoDocument)?; + let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; let header = bool_param(p, "header").unwrap_or_else(|| guess_header(&d.wb, sh, r)); let by_cols = str_param(p, "orientation") == Some("columns"); let case = bool_param(p, "matchCase").unwrap_or(false); @@ -228,10 +245,10 @@ fn sort(s: &mut Session, p: &Json) -> Result { if keys.len() > 64 { return Err(bad("data.sort", "too many sort levels (64 max)")); } - do_sort(s, r, header, keys, by_cols, case) + do_sort(s, sheet, r, header, keys, by_cols, case) } -fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec, by_cols: bool, case: bool) -> Result { +fn do_sort(s: &mut Session, sheet: usize, r: RangeRef, header: bool, keys: Vec, by_cols: bool, case: bool) -> Result { let body = if by_cols { if header && r.width() > 1 { RangeRef::new(CellRef::new(r.start.row, r.start.col + 1), r.end) } else { r } } else if header && r.height() > 1 { @@ -242,7 +259,6 @@ fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec, by_co if body.count() > 20_000_000 { return Err(bad("data.sort", "range too large")); } - let sheet = s.doc()?.wb.active_sheet; edit(s, |cx| { let Some(sh) = cx.wb.sheet(sheet) else { return Ok(Json::Null) }; if sh.merges.iter().any(|m| m.intersects(&body) && !m.is_single()) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..7d7a53157 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1027,6 +1027,31 @@ fn case_sensitive_sort_puts_lowercase_first() { assert_eq!(col, ["a", "A", "aB", "Ab", "b", "B"].map(Value::from)); } +#[test] +fn sort_with_a_sheet_prefix_sorts_the_named_sheet() { + let col = |s: &Session, sheet: &str| -> Vec { + let wb = &s.doc().unwrap().wb; + let sh = wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap(); + (1..=3).map(|r| sh.value(CellRef::new(r, 0))).collect() + }; + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Name", "Qty"], ["a", 3], ["b", 1], ["c", 2]]})).unwrap(); + s.execute("home.insertSheet", json!({"name": "Prices"})).unwrap(); + s.execute("range.setValues", json!({"range": "Prices!A1", "values": [["Item", "Price"], ["p1", 10], ["p2", 30], ["p3", 20]]})).unwrap(); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + let r = s.execute("data.sort", json!({"range": "Prices!A1:B4", "header": true, "keys": [{"column": "B", "order": "desc"}]})).unwrap(); + assert_eq!(r["sorted"], "A2:B4"); + assert_eq!(col(&s, "Prices"), ["p2", "p3", "p1"].map(Value::from)); + assert_eq!(col(&s, "Sheet1"), ["a", "b", "c"].map(Value::from)); + s.execute("data.sortAscending", json!({"range": "Prices!A2:B4", "column": "B"})).unwrap(); + assert_eq!(col(&s, "Prices"), ["p1", "p3", "p2"].map(Value::from)); + assert_eq!(col(&s, "Sheet1"), ["a", "b", "c"].map(Value::from)); + // The selection belongs to the active sheet, so another sheet needs its range. + assert!(s.execute("data.sort", json!({"sheet": "Prices", "keys": [{"column": "B"}]})).is_err()); + assert!(s.execute("data.sort", json!({"range": "Nowhere!A1:B4", "keys": [{"column": "B"}]})).is_err()); + assert_eq!(col(&s, "Sheet1"), ["a", "b", "c"].map(Value::from)); +} + #[test] fn remove_duplicates_compares_shown_text() { let mut s = s(); From 13691e95e5148db55e370b195dc7b9e7ed5414ed Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 12:51:05 +0200 Subject: [PATCH 06/28] Remove Duplicates: a Sheet! prefix on the range works on that sheet, not the active one (refs #1283) --- crates/engine/src/cmd/data.rs | 22 +++++++++++++++++++--- crates/engine/src/tests.rs | 23 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..2e7b379d0 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -147,6 +147,23 @@ fn data_range(s: &Session, p: &Json) -> Result { Ok(current_region(sh, d.selection.active)) } +/// The sheet and range a data command works on: the sheet `sheet` or a `Sheet!` prefix on `range` +/// names, else the active one. The selection belongs to the active sheet, so another sheet needs `range`. +fn data_target(s: &Session, p: &Json, cmd: &str) -> Result<(usize, RangeRef)> { + let sheet = target_sheet(s, p)?; + let d = s.doc()?; + if let Some(t) = str_param(p, "range") + && let (Some(name), _) = split_sheet(t) + && d.wb.sheet_index(&name) != Some(sheet) + { + return Err(bad(cmd, format!("`{t}` names a sheet other than `sheet`"))); + } + if sheet != d.wb.active_sheet && p.get("range").is_none() { + return Err(bad(cmd, "missing `range`: the selection is on the active sheet")); + } + Ok((sheet, data_range(s, p)?)) +} + /// Excel's header guess: the first row is text over a column of numbers, or is bold. fn guess_header(wb: &Workbook, sh: &Sheet, r: RangeRef) -> bool { if let Some(t) = sh.table_at(r.start) { @@ -736,9 +753,9 @@ fn sort_key(v: &Value) -> (u8, i64, String) { // ---------------------------------------------------------------- tools fn remove_duplicates(s: &mut Session, p: &Json) -> Result { - let r = data_range(s, p)?; + let (sheet, r) = data_target(s, p, "data.removeDuplicates")?; let d = s.doc()?; - let sh = d.wb.active().ok_or(EngineError::NoDocument)?; + let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; // Rows and columns past the data are blank: only the part with data is compared (whole // columns compared a million blank rows and reported them as removed duplicates). let r = sh.used_range().and_then(|u| r.intersection(&u)).unwrap_or(RangeRef::cell(r.start)); @@ -748,7 +765,6 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result { .and_then(Json::as_array) .map(|a| a.iter().filter_map(|c| col_param(Some(c), r.start.col)).collect()) .unwrap_or_else(|| (r.start.col..=r.end.col).collect()); - let sheet = d.wb.active_sheet; let start = r.start.row + header as u32; let mut removed = 0; edit(s, |cx| { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..19a512d73 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1039,6 +1039,29 @@ fn remove_duplicates_compares_shown_text() { assert_eq!(r["removed"], 1); } +#[test] +fn remove_duplicates_with_a_sheet_prefix_works_on_the_named_sheet() { + let col = |s: &Session, sheet: &str| -> Vec { + let wb = &s.doc().unwrap().wb; + let sh = wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap(); + (1..=3).map(|r| sh.value(CellRef::new(r, 0))).collect() + }; + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Name"], ["a"], ["a"], ["a"]]})).unwrap(); + s.execute("home.insertSheet", json!({"name": "Prices"})).unwrap(); + s.execute("range.setValues", json!({"range": "Prices!A1", "values": [["Item"], ["p1"], ["p1"], ["p2"]]})).unwrap(); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + let r = s.execute("data.removeDuplicates", json!({"range": "Prices!A1:A4", "header": true})).unwrap(); + assert_eq!(r["removed"], 1); + assert_eq!(col(&s, "Prices"), [Value::from("p1"), Value::from("p2"), Value::Empty]); + assert_eq!(col(&s, "Sheet1"), ["a", "a", "a"].map(Value::from)); + // The selection belongs to the active sheet, so another sheet needs its range. + assert!(s.execute("data.removeDuplicates", json!({"sheet": "Prices", "header": true})).is_err()); + assert!(s.execute("data.removeDuplicates", json!({"sheet": "Sheet1", "range": "Prices!A1:A4", "header": true})).is_err()); + assert!(s.execute("data.removeDuplicates", json!({"range": "Nowhere!A1:A4", "header": true})).is_err()); + assert_eq!(col(&s, "Sheet1"), ["a", "a", "a"].map(Value::from)); +} + #[test] fn formatting_and_styles() { let mut s = s(); From 39602e6ce11fbc92e2495a1e694db0d23d34f1b4 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 13:00:17 +0200 Subject: [PATCH 07/28] Filter and Subtotal: a Sheet! prefix on the range works on that sheet, not the active one (refs #1283) --- crates/engine/src/cmd/data.rs | 46 ++++++++++++++----------- crates/engine/src/tests.rs | 64 +++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 20 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 2e7b379d0..78604cd35 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -80,13 +80,13 @@ pub fn specs() -> Vec { "Group", ["Data", "Outline"], Some("Alt+Shift+Right"), - "{rows?: \"2:5\" | cols?: \"B:C\" | by?: rows|cols (the selection's rows or columns; a block of cells groups rows by default)}", + "{rows?: \"2:5\" | cols?: \"B:C\" | by?: rows|cols (the selection's rows or columns; a block of cells groups rows by default), sheet?}", has_doc, |s, p| group(s, p, true) ), - cmd!("data.ungroup", "Ungroup", ["Data", "Outline"], Some("Alt+Shift+Left"), "{rows? | cols? | by?: rows|cols}", has_doc, |s, p| group( - s, p, false - )), + cmd!("data.ungroup", "Ungroup", ["Data", "Outline"], Some("Alt+Shift+Left"), "{rows? | cols? | by?: rows|cols, sheet?}", has_doc, |s, p| { + group(s, p, false) + }), cmd!( "data.hideDetail", "Hide Detail", @@ -454,10 +454,11 @@ fn sorted_anchor(sh: &Sheet, a: Anchor, bottom: (u32, u32, f32, f32), by_cols: b // ---------------------------------------------------------------- filter fn toggle_filter(s: &mut Session, p: &Json) -> Result { - let sheet = s.doc()?.wb.active_sheet; + let (sheet, r) = data_target(s, p, "data.filter")?; + // The active cell belongs to the active sheet: on another sheet the table is the one at `range`. + let at = if sheet == s.doc()?.wb.active_sheet { s.doc()?.selection.active } else { r.start }; let has = s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.autofilter.is_some()); - let in_table = - s.doc()?.wb.sheet(sheet).and_then(|sh| sh.table_at(s.doc().map(|d| d.selection.active).unwrap_or_default()).map(|t| t.name.clone())); + let in_table = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.table_at(at).map(|t| t.name.clone())); if let Some(name) = in_table { return edit(s, |cx| { let sh = cx.sheet_mut(sheet)?; @@ -490,8 +491,7 @@ fn toggle_filter(s: &mut Session, p: &Json) -> Result { /// Turn a sheet AutoFilter on over `p.range`, or over the data range of the selection. fn install_filter(s: &mut Session, p: &Json) -> Result { - let sheet = s.doc()?.wb.active_sheet; - let mut r = data_range(s, p)?; + let (sheet, mut r) = data_target(s, p, "data.filter")?; // A selected header row filters the data below it (#201). if p.get("range").is_none() && r.height() == 1 { let d = s.doc()?; @@ -1159,9 +1159,13 @@ fn outline_lines(s: &Session, p: &Json) -> Result<(bool, u32, u32)> { } fn group(s: &mut Session, p: &Json, on: bool) -> Result { + let si = target_sheet(s, p)?; + // The selection belongs to the active sheet, so another sheet needs its lines. + if si != s.doc()?.wb.active_sheet && p.get("rows").is_none() && p.get("cols").is_none() { + return Err(bad(if on { "data.group" } else { "data.ungroup" }, "missing `rows` or `cols`: the selection is on the active sheet")); + } let (rows, a, b) = outline_lines(s, p)?; edit(s, |cx| { - let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; let map = if rows { &mut sh.rows } else { &mut sh.cols }; let b = b.min(a.saturating_add(1_048_575)); @@ -1261,7 +1265,7 @@ fn show_levels(s: &mut Session, p: &Json) -> Result { } fn subtotal(s: &mut Session, p: &Json) -> Result { - let r = data_range(s, p)?; + let (sheet, r) = data_target(s, p, "data.subtotal")?; let group_col = col_param(p.get("groupBy"), r.start.col).unwrap_or(r.start.col); let func = str_param(p, "function").unwrap_or("sum"); let code = match func { @@ -1285,7 +1289,6 @@ fn subtotal(s: &mut Session, p: &Json) -> Result { "product" => "Product", _ => "Total", }; - let sheet = s.doc()?.wb.active_sheet; // Group boundaries (header in the first row). let d = s.doc()?; let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; @@ -1302,25 +1305,28 @@ fn subtotal(s: &mut Session, p: &Json) -> Result { // Insert a total row after each group, bottom-up so indices stay valid. let mut inserted = 0; for (g0, g1, name) in groups.iter().rev() { - s.execute("home.insertRows", json!({"rows": format!("{}:{}", g1 + 2, g1 + 2)}))?; + s.execute("home.insertRows", json!({"sheet": sheet, "rows": format!("{}:{}", g1 + 2, g1 + 2)}))?; let row = g1 + 1; let lbl = CellRef::new(row, group_col).a1(); - s.execute("cell.set", json!({"cell": lbl, "input": format!("{name} {label}")}))?; + s.execute("cell.set", json!({"sheet": sheet, "cell": lbl, "input": format!("{name} {label}")}))?; for c in &cols { let range = RangeRef::new(CellRef::new(*g0, *c), CellRef::new(*g1, *c)).a1(); - s.execute("cell.set", json!({"cell": CellRef::new(row, *c).a1(), "input": format!("=SUBTOTAL({code},{range})")}))?; + s.execute("cell.set", json!({"sheet": sheet, "cell": CellRef::new(row, *c).a1(), "input": format!("=SUBTOTAL({code},{range})")}))?; } - s.execute("home.bold", json!({"range": RangeRef::new(CellRef::new(row, r.start.col), CellRef::new(row, r.end.col)).a1(), "on": true}))?; - s.execute("data.group", json!({"rows": format!("{}:{}", g0 + 1, g1 + 1)}))?; + s.execute( + "home.bold", + json!({"sheet": sheet, "range": RangeRef::new(CellRef::new(row, r.start.col), CellRef::new(row, r.end.col)).a1(), "on": true}), + )?; + s.execute("data.group", json!({"sheet": sheet, "rows": format!("{}:{}", g0 + 1, g1 + 1)}))?; inserted += 1; } let last = r.end.row + inserted + 1; let lbl = CellRef::new(last, group_col).a1(); - s.execute("home.insertRows", json!({"rows": format!("{}:{}", last + 1, last + 1)}))?; - s.execute("cell.set", json!({"cell": lbl, "input": format!("Grand {label}")}))?; + s.execute("home.insertRows", json!({"sheet": sheet, "rows": format!("{}:{}", last + 1, last + 1)}))?; + s.execute("cell.set", json!({"sheet": sheet, "cell": lbl, "input": format!("Grand {label}")}))?; for c in &cols { let range = RangeRef::new(CellRef::new(r.start.row + 1, *c), CellRef::new(last - 1, *c)).a1(); - s.execute("cell.set", json!({"cell": CellRef::new(last, *c).a1(), "input": format!("=SUBTOTAL({code},{range})")}))?; + s.execute("cell.set", json!({"sheet": sheet, "cell": CellRef::new(last, *c).a1(), "input": format!("=SUBTOTAL({code},{range})")}))?; } Ok(json!({"groups": inserted})) } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 19a512d73..0d5dd5f8d 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1062,6 +1062,70 @@ fn remove_duplicates_with_a_sheet_prefix_works_on_the_named_sheet() { assert_eq!(col(&s, "Sheet1"), ["a", "a", "a"].map(Value::from)); } +#[test] +fn filter_with_a_sheet_prefix_toggles_the_named_sheet() { + let sheet = |s: &Session, name: &str| -> gridcraft_model::Sheet { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index(name).unwrap()).unwrap().clone() + }; + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Name", "Qty"], ["a", 3], ["b", 1]]})).unwrap(); + s.execute("home.insertSheet", json!({"name": "Prices"})).unwrap(); + s.execute("range.setValues", json!({"range": "Prices!A1", "values": [["Item", "Price"], ["p1", 10], ["p2", 30]]})).unwrap(); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + let r = s.execute("data.filter", json!({"range": "Prices!A1:B3"})).unwrap(); + assert_eq!(r["range"], "A1:B3"); + assert_eq!(sheet(&s, "Prices").autofilter.map(|a| a.range.a1()), Some("A1:B3".to_string())); + assert!(sheet(&s, "Sheet1").autofilter.is_none()); + s.execute("data.filter", json!({"range": "Prices!A1:B3"})).unwrap(); + assert!(sheet(&s, "Prices").autofilter.is_none()); + assert!(sheet(&s, "Sheet1").autofilter.is_none()); + assert!(s.execute("data.filter", json!({"sheet": "Prices"})).is_err()); +} + +#[test] +fn filter_with_a_sheet_prefix_toggles_the_table_at_the_range() { + let mut s = s(); + s.execute("home.insertSheet", json!({"name": "Prices"})).unwrap(); + s.execute("range.setValues", json!({"range": "Prices!A1", "values": [["Item", "Price"], ["p1", 10], ["p2", 30]]})).unwrap(); + s.execute("insert.table", json!({"range": "A1:B3", "header": true})).unwrap(); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + // The active cell is outside the table's cells, so only `range` can find the table. + s.execute("selection.set", json!({"range": "E10"})).unwrap(); + let prices = |s: &Session| { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index("Prices").unwrap()).unwrap().clone() + }; + assert!(prices(&s).tables[0].filter_button); + s.execute("data.filter", json!({"range": "Prices!A2:B3"})).unwrap(); + assert!(!prices(&s).tables[0].filter_button); + assert!(prices(&s).autofilter.is_none()); + assert!(s.doc().unwrap().wb.active().unwrap().autofilter.is_none()); +} + +#[test] +fn subtotal_with_a_sheet_prefix_works_on_the_named_sheet() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Name", "Qty"], ["x", 1], ["x", 2], ["y", 3]]})).unwrap(); + s.execute("home.insertSheet", json!({"name": "Prices"})).unwrap(); + s.execute("range.setValues", json!({"range": "Prices!A1", "values": [["Item", "Qty"], ["a", 1], ["a", 2], ["b", 4]]})).unwrap(); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + let r = s.execute("data.subtotal", json!({"range": "Prices!A1:B4", "groupBy": "A", "columns": ["B"]})).unwrap(); + assert_eq!(r["groups"], 2); + let wb = &s.doc().unwrap().wb; + let prices = wb.sheet(wb.sheet_index("Prices").unwrap()).unwrap(); + let col = |sh: &gridcraft_model::Sheet, c: u32| -> Vec { (0..7).map(|r| sh.value(CellRef::new(r, c))).collect() }; + assert_eq!(col(prices, 0), ["Item", "a", "a", "a Total", "b", "b Total", "Grand Total"].map(Value::from)); + assert_eq!(col(prices, 1)[1..], [1.0, 2.0, 3.0, 4.0, 4.0, 7.0].map(Value::from)); + assert!(wb.styles.get(prices.style_id(CellRef::new(3, 0))).font.bold); + assert_eq!(prices.rows.get(&1).map(|l| l.outline), Some(1)); + let sheet1 = wb.sheet(wb.sheet_index("Sheet1").unwrap()).unwrap(); + assert_eq!(col(sheet1, 0)[..4], ["Name", "x", "x", "y"].map(Value::from)); + assert!(sheet1.value(CellRef::new(4, 0)).is_empty()); + assert!(sheet1.rows.values().all(|l| l.outline == 0)); + assert!(s.execute("data.group", json!({"sheet": "Prices"})).is_err()); +} + #[test] fn formatting_and_styles() { let mut s = s(); From 139098e17479b8674112f9757b4abb0fb997a16b Mon Sep 17 00:00:00 2001 From: MarekSpodBiedry <73122968+MarekSpodBiedry@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:43:39 +0200 Subject: [PATCH 08/28] Filter: count empty text and ="" results as (Blanks) (#1294) --- crates/engine/src/cmd/data.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..cd6e5771e 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -581,6 +581,11 @@ fn custom_ok(v: &Value, text: &str, op: &str, crit: &str) -> bool { } } +/// The filter's "(Blanks)" group: truly empty cells and zero-length text such as a `=""` result. +fn filter_blank(v: &Value) -> bool { + v.is_empty() || matches!(v, Value::Text(t) if t.is_empty()) +} + /// Hides rows of the autofilter range that fail any criterion. pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { let Some(sh) = wb.sheet(sheet) else { return }; @@ -614,7 +619,7 @@ pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { let text = crate::display::cell_text(wb, sh, c); let ok = match crit { FilterCriterion::Values { values, blanks } => { - if v.is_empty() { + if filter_blank(&v) { *blanks } else { values.iter().any(|x| x.eq_ignore_ascii_case(&text) || x.eq_ignore_ascii_case(&v.display())) @@ -701,10 +706,11 @@ pub fn filter_values(wb: &Workbook, sheet: usize, col: u32) -> Vec<(String, bool for row in af.range.start.row + 1..=af.range.end.row.min(af.range.start.row + 1_000_000) { let c = CellRef::new(row, col); let v = sh.value(c); - let t = if v.is_empty() { "(Blanks)".to_string() } else { crate::display::cell_text(wb, sh, c) }; + let blank = filter_blank(&v); + let t = if blank { "(Blanks)".to_string() } else { crate::display::cell_text(wb, sh, c) }; let on = match selected { Some(FilterCriterion::Values { values, blanks }) => { - if v.is_empty() { + if blank { *blanks } else { values.iter().any(|x| x.eq_ignore_ascii_case(&t)) @@ -712,7 +718,8 @@ pub fn filter_values(wb: &Workbook, sheet: usize, col: u32) -> Vec<(String, bool } _ => true, }; - seen.entry((sort_key(&v), t)).or_insert(on); + let key = if blank { sort_key(&Value::Empty) } else { sort_key(&v) }; + seen.entry((key, t)).or_insert(on); } seen.into_iter().map(|((_, t), on)| (t, on)).collect() } @@ -1313,6 +1320,21 @@ fn subtotal(s: &mut Session, p: &Json) -> Result { mod tests { use super::*; + #[test] + fn filter_blanks_include_empty_text_and_formula_results() { + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["id", "f"], ["e", null], ["ef", "=\"\""], ["sp", " "], ["z", 0]]})).unwrap(); + s.execute("data.filter", json!({"range": "A1:B5"})).unwrap(); + let r = s.execute("data.filterBy", json!({"column": "B", "values": [], "blanks": true})).unwrap(); + assert_eq!(r["hiddenRows"], json!(2)); + let vals = filter_values(&s.doc().unwrap().wb, 0, 1); + // The `=""` result joins (Blanks) instead of showing as its own empty entry. + assert!(!vals.iter().any(|(t, _)| t.is_empty()), "{vals:?}"); + let blanks: Vec<_> = vals.into_iter().filter(|(t, _)| t == "(Blanks)").collect(); + assert_eq!(blanks, vec![("(Blanks)".to_string(), true)]); + } + #[test] fn filter_by_huge_numeric_column_is_rejected_not_overflowed() { let mut s = Session::new(); From 450ef1aea042e362833546c06f86875f526501db Mon Sep 17 00:00:00 2001 From: midasmatterhorn <248507632+midasmatterhorn@users.noreply.github.com> Date: Wed, 14 Oct 2026 18:01:42 +0300 Subject: [PATCH 09/28] engine: refuse removeDuplicates with no valid column instead of deleting all rows --- crates/engine/src/cmd/data.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..596a61303 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -748,6 +748,12 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result { .and_then(Json::as_array) .map(|a| a.iter().filter_map(|c| col_param(Some(c), r.start.col)).collect()) .unwrap_or_else(|| (r.start.col..=r.end.col).collect()); + // With no column to compare, every row key is empty: the first row would survive and every + // other row would be deleted as a duplicate. A column outside the range compares data the + // user didn't select. Excel refuses both. + if cols.is_empty() || cols.iter().any(|c| !(r.start.col..=r.end.col).contains(c)) { + return Err(bad("data.removeDuplicates", "`columns` must name at least one column of the range")); + } let sheet = d.wb.active_sheet; let start = r.start.row + header as u32; let mut removed = 0; @@ -1499,4 +1505,20 @@ mod tests { assert_eq!(split_delimited("a,b,,c", &[','], true, '"'), ["a", "b", "c"]); assert_eq!(split_delimited("\"x,y\",z", &[','], false, '"'), ["x,y", "z"]); } + + #[test] + fn remove_duplicates_without_a_valid_column_is_refused() { + // `columns: []` (or only unusable names) used to compare nothing and delete every row + // but the first. + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["a"], ["b"], ["c"]]})).unwrap(); + for columns in [json!([]), json!(["ZZZ"])] { + let err = s.execute("data.removeDuplicates", json!({"range": "A1:A3", "columns": columns})).unwrap_err(); + assert!(err.to_string().contains("columns"), "{err}"); + } + let sh = s.doc().unwrap().wb.active().unwrap(); + assert_eq!(sh.value(CellRef::new(0, 0)), Value::text("a")); + assert_eq!(sh.value(CellRef::new(2, 0)), Value::text("c")); + } } From f92b4d30cb2cd74d83ca1000da1ba9566787e14e Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 18:01:11 +0200 Subject: [PATCH 10/28] AutoFit Column Width measures text in the font the grid draws it with (fixes #1316) --- crates/engine/src/cmd/format.rs | 11 +++++++---- crates/engine/src/lib.rs | 7 +++++++ crates/ui-egui/src/lib.rs | 4 ++++ crates/ui-egui/src/theme.rs | 11 +++++++++++ crates/ui-egui/tests/header_autofit.rs | 27 ++++++++++++++++++++++++++ 5 files changed, 56 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index 4712351ca..c7e970fa5 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -1008,7 +1008,7 @@ pub fn chars_to_points(c: f64) -> f64 { pub use gridcraft_model::points_to_chars; /// Approximate text width in points for autofit (no font metrics in the engine: average glyph -/// widths for the default font; the UI refines with real measurement when it autofits). +/// widths for the default font; a front end with fonts measures instead, see `Session::text_width`). pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { let mut w = 0.0; for c in text.chars() { @@ -1027,6 +1027,7 @@ pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { fn autofit_cols(s: &mut Session, p: &Json) -> Result { let lines = line_targets(s, p, false)?; + let measure = s.text_width.clone(); edit(s, |cx| { let si = cx.sheet_index(); let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) }; @@ -1044,9 +1045,11 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { let st = cx.wb.styles.get(sh.style_id(cell)); let text = crate::display::cell_text(&cx.wb, sh, cell); for line in text.lines() { - max = max.max( - approx_text_width(line, st.font.size, st.font.bold) + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell), - ); + let w = match &measure { + Some(m) => m(line, &st.font), + None => approx_text_width(line, st.font.size, st.font.bold), + }; + max = max.max(w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell)); } } if max > 0.0 { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 258a1cb72..daa9875e5 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -227,6 +227,10 @@ pub enum UiRequest { OpenUrl(String), } +/// Width in points of one line of text drawn in a cell font, measured by a front end that has +/// the font the grid draws with. +pub type TextWidth = Arc f32 + Send + Sync>; + #[derive(Default)] pub struct Session { docs: Vec, @@ -256,6 +260,9 @@ pub struct Session { /// Review › Notes › Show All Notes: every note is displayed. An application display setting /// (like Excel's comment indicator option), so it's not saved in the file. pub show_all_notes: bool, + /// Measures cell text for AutoFit Column Width. Without it (CLI, MCP) AutoFit uses + /// [`cmd::format::approx_text_width`]. + pub text_width: Option, } impl Session { diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f96cdac3c..c9616bc0e 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -990,6 +990,10 @@ impl SheetApp { /// Per-frame logic (control channel, screenshots). Call before `ui`. pub fn logic(&mut self, ctx: &egui::Context) { + if self.session.text_width.is_none() { + let ctx = ctx.clone(); + self.session.text_width = Some(std::sync::Arc::new(move |text, font| theme::cell_text_width(&ctx, text, font))); + } // A keytip control request lives one frame: the ribbon consumes it during `ui`, and // anything that never reached a control (collapsed ribbon, tab switch) is dropped. self.ui.keytip_invoke = None; diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 485b9a2f3..23c7fd35e 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -192,6 +192,17 @@ pub fn cell_family(name: &str, bold: bool, italic: bool) -> FontFamily { ) } +/// Width in points, at 100% zoom, of one line of cell text drawn as the grid draws it: in the +/// face [`cell_family`] picks, which on a machine without the named font is a wider or narrower +/// substitute (#1316). A family the active font set doesn't have yet (bound fonts apply from the +/// next frame) falls back to the engine's estimate rather than letting egui panic on it. +pub fn cell_text_width(ctx: &egui::Context, text: &str, font: &gridcraft_engine::model::Font) -> f32 { + let family = cell_family(&font.name, font.bold, font.italic); + let id = FontId::new(font.size * 96.0 / 72.0, family); + ctx.fonts_mut(|f| f.families().contains(&id.family).then(|| f.layout_no_wrap(text.to_owned(), id, Color32::PLACEHOLDER).size().x)) + .unwrap_or_else(|| gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold)) +} + /// Whether epaint can open face `index` of this font file and it maps characters to glyphs. /// epaint panics (in `Context::set_fonts`, i.e. on startup) on a face it can't open, so every font /// file we did not ship is checked first with the same parser and the same call epaint makes diff --git a/crates/ui-egui/tests/header_autofit.rs b/crates/ui-egui/tests/header_autofit.rs index 5e123809f..b4f104f4b 100644 --- a/crates/ui-egui/tests/header_autofit.rs +++ b/crates/ui-egui/tests/header_autofit.rs @@ -191,3 +191,30 @@ fn table_filter_autofit_works_through_command_and_header_double_click() { } } } + +#[test] +fn header_double_click_fits_the_text_as_the_grid_draws_it() { + // Issue #1316: `@` and `%` are far wider in every real font than the engine's average-glyph + // estimate, so AutoFit has to measure with the font the grid draws the cell in. + let text = "sarmstrong@example.com %%% @@@"; + let mut h = harness("A1"); + h.state_mut().session.execute("range.setValues", json!({"range":"A1", "values":[[text]]})).unwrap(); + h.run_steps(3); + let (pos, _) = edge(&h, false, 0); + button(&mut h, pos, true); + button(&mut h, pos, false); + h.step(); + button(&mut h, pos, true); + button(&mut h, pos, false); + h.run_steps(2); + + let app = h.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let font = &app.session.active().unwrap().wb.styles.get(sheet.style_id(gridcraft_engine::core::CellRef::new(0, 0))).font; + let id = egui::FontId::new(font.size * 96.0 / 72.0, gridcraft_ui_egui::theme::cell_family(&font.name, font.bold, font.italic)); + let drawn = h.ctx.fonts_mut(|f| f.layout_no_wrap(text.to_string(), id, egui::Color32::BLACK)).size().x; + let estimate = gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold); + assert!(drawn > estimate + 6.0, "fixture: the drawn text ({drawn}) must be wider than the estimate ({estimate}) plus padding"); + // Two 2.5 px paddings and the gridline, as the grid lays out the cell. + assert!(sheet.col_width(0) >= drawn + 6.0, "column {} is narrower than the drawn text {drawn} + padding", sheet.col_width(0)); +} From f36bc100d4b72ca25800e7b55bd8860e1ae1e5df Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 18:14:09 +0200 Subject: [PATCH 11/28] AutoFit measures each distinct text once, and quickly on large columns (#1316) --- crates/engine/src/cmd/format.rs | 22 ++++++++++++++++-- crates/engine/src/lib.rs | 7 ++++-- crates/engine/tests/autofit_filter_buttons.rs | 23 +++++++++++++++++++ crates/ui-egui/src/lib.rs | 2 +- crates/ui-egui/src/theme.rs | 19 ++++++++++++--- crates/ui-egui/tests/header_autofit.rs | 23 ++++++++++++++++++- 6 files changed, 87 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index c7e970fa5..585f68dc0 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -1025,6 +1025,10 @@ pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { w * size * if bold { 1.07 } else { 1.0 } * 96.0 / 72.0 } +/// Distinct texts AutoFit measures exactly; past this many, `Session::text_width` is asked for a +/// quick measure so a huge column doesn't lay out every value. +const EXACT_TEXT_WIDTHS: usize = 2_000; + fn autofit_cols(s: &mut Session, p: &Json) -> Result { let lines = line_targets(s, p, false)?; let measure = s.text_width.clone(); @@ -1032,6 +1036,8 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { let si = cx.sheet_index(); let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) }; let mut widths = Vec::new(); + let mut measured: std::collections::HashMap> = Default::default(); + let mut distinct = 0usize; let used = sh.used_range(); for (a, b) in lines { for c in a..=b.min(a + 16383) { @@ -1042,11 +1048,23 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { if sh.merge_at(cell).is_some_and(|m| !m.is_single()) { continue; } - let st = cx.wb.styles.get(sh.style_id(cell)); + let id = sh.style_id(cell); + let st = cx.wb.styles.get(id); let text = crate::display::cell_text(&cx.wb, sh, cell); for line in text.lines() { let w = match &measure { - Some(m) => m(line, &st.font), + Some(m) => { + let known = measured.entry(id).or_default(); + match known.get(line) { + Some(w) => *w, + None => { + let w = m(line, &st.font, distinct < EXACT_TEXT_WIDTHS); + known.insert(line.to_owned(), w); + distinct += 1; + w + } + } + } None => approx_text_width(line, st.font.size, st.font.bold), }; max = max.max(w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell)); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index daa9875e5..f52984c4c 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -228,8 +228,11 @@ pub enum UiRequest { } /// Width in points of one line of text drawn in a cell font, measured by a front end that has -/// the font the grid draws with. -pub type TextWidth = Arc f32 + Send + Sync>; +/// the font the grid draws with. With `exact` false the front end may answer with a cheaper +/// measure that is never narrower (AutoFit asks for it on columns with many distinct values). +/// Commands call it while they run, so a front end must not run commands while holding a lock +/// the measure takes (egui's: see `theme::cell_text_width` in the UI crate). +pub type TextWidth = Arc f32 + Send + Sync>; #[derive(Default)] pub struct Session { diff --git a/crates/engine/tests/autofit_filter_buttons.rs b/crates/engine/tests/autofit_filter_buttons.rs index 307bb998d..dbf25eced 100644 --- a/crates/engine/tests/autofit_filter_buttons.rs +++ b/crates/engine/tests/autofit_filter_buttons.rs @@ -100,3 +100,26 @@ fn autofit_filter_width_is_undoable_without_changing_selection() { s.execute("edit.undo", json!({})).unwrap(); assert_eq!(s.doc().unwrap().wb.active().unwrap().col_width(0), 80.0); } + +#[test] +fn autofit_measures_each_distinct_text_once_and_quickly_past_the_limit() { + use std::sync::{Arc, Mutex}; + let mut s = Session::new(); + s.new_workbook(); + // 2,500 distinct texts, each in two cells. + let values: Vec<_> = (0..5000).map(|i| json!([format!("v{}", i % 2500)])).collect(); + s.execute("range.setValues", json!({"range":"A1", "values": values})).unwrap(); + let calls = Arc::new(Mutex::new(Vec::new())); + let seen = calls.clone(); + s.text_width = Some(Arc::new(move |text: &str, _: &gridcraft_engine::model::Font, exact: bool| { + seen.lock().unwrap().push((text.to_owned(), exact)); + if text == "v2499" { 300.0 } else { 10.0 } + })); + s.execute("home.autofitColumnWidth", json!({"cols":"A:A"})).unwrap(); + let calls = calls.lock().unwrap(); + assert_eq!(calls.len(), 2500, "one measure per distinct text"); + assert_eq!(calls.iter().filter(|(_, exact)| *exact).count(), 2000); + assert!(calls.iter().skip(2000).all(|(_, exact)| !exact)); + // The quick measures still size the column. + assert_eq!(s.doc().unwrap().wb.active().unwrap().col_width(0), 306.0); +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index c9616bc0e..3bb674dce 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -992,7 +992,7 @@ impl SheetApp { pub fn logic(&mut self, ctx: &egui::Context) { if self.session.text_width.is_none() { let ctx = ctx.clone(); - self.session.text_width = Some(std::sync::Arc::new(move |text, font| theme::cell_text_width(&ctx, text, font))); + self.session.text_width = Some(std::sync::Arc::new(move |text, font, exact| theme::cell_text_width(&ctx, text, font, exact))); } // A keytip control request lives one frame: the ribbon consumes it during `ui`, and // anything that never reached a control (collapsed ribbon, tab switch) is dropped. diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 23c7fd35e..46ac1ea4a 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -196,11 +196,24 @@ pub fn cell_family(name: &str, bold: bool, italic: bool) -> FontFamily { /// face [`cell_family`] picks, which on a machine without the named font is a wider or narrower /// substitute (#1316). A family the active font set doesn't have yet (bound fonts apply from the /// next frame) falls back to the engine's estimate rather than letting egui panic on it. -pub fn cell_text_width(ctx: &egui::Context, text: &str, font: &gridcraft_engine::model::Font) -> f32 { +/// +/// `exact` lays the text out (a galley kept until the end of the frame); otherwise the glyph +/// advances are summed without kerning, which is cheap and never narrower. This takes the +/// Context lock (`fonts_mut`): never call it, or run a command, while an egui lock is held. +pub fn cell_text_width(ctx: &egui::Context, text: &str, font: &gridcraft_engine::model::Font, exact: bool) -> f32 { let family = cell_family(&font.name, font.bold, font.italic); let id = FontId::new(font.size * 96.0 / 72.0, family); - ctx.fonts_mut(|f| f.families().contains(&id.family).then(|| f.layout_no_wrap(text.to_owned(), id, Color32::PLACEHOLDER).size().x)) - .unwrap_or_else(|| gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold)) + ctx.fonts_mut(|f| { + f.families().contains(&id.family).then(|| { + if exact { + f.layout_no_wrap(text.to_owned(), id, Color32::PLACEHOLDER).size().x + } else { + // Half a point covers the galley's rounding of its width up to the pixel grid. + text.chars().map(|c| f.glyph_width(&id, c)).sum::() + 0.5 + } + }) + }) + .unwrap_or_else(|| gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold)) } /// Whether epaint can open face `index` of this font file and it maps characters to glyphs. diff --git a/crates/ui-egui/tests/header_autofit.rs b/crates/ui-egui/tests/header_autofit.rs index b4f104f4b..e5cf0cc21 100644 --- a/crates/ui-egui/tests/header_autofit.rs +++ b/crates/ui-egui/tests/header_autofit.rs @@ -199,7 +199,10 @@ fn header_double_click_fits_the_text_as_the_grid_draws_it() { let text = "sarmstrong@example.com %%% @@@"; let mut h = harness("A1"); h.state_mut().session.execute("range.setValues", json!({"range":"A1", "values":[[text]]})).unwrap(); - h.run_steps(3); + // Let the installed-font scan finish and bind its faces first, so the cell's font can't + // change between the AutoFit and the measurement below. + gridcraft_ui_egui::fonts::catalog(); + h.run_steps(6); let (pos, _) = edge(&h, false, 0); button(&mut h, pos, true); button(&mut h, pos, false); @@ -218,3 +221,21 @@ fn header_double_click_fits_the_text_as_the_grid_draws_it() { // Two 2.5 px paddings and the gridline, as the grid lays out the cell. assert!(sheet.col_width(0) >= drawn + 6.0, "column {} is narrower than the drawn text {drawn} + padding", sheet.col_width(0)); } + +#[test] +fn the_quick_measure_is_never_narrower_than_the_drawn_text() { + // Columns with many distinct values are measured by summing glyph advances; that must cover + // the laid-out text, or AutoFit would clip again on large columns. + let h = harness("A1"); + let app = h.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let mut font = app.session.active().unwrap().wb.styles.get(sheet.style_id(gridcraft_engine::core::CellRef::new(0, 0))).font.clone(); + for bold in [false, true] { + font.bold = bold; + for text in ["sarmstrong@example.com", "Quarterly revenue summary", "WWW MMM Wide Headers", "AVATAR To Ty", "fi ffl 1,234.56"] { + let quick = gridcraft_ui_egui::theme::cell_text_width(&h.ctx, text, &font, false); + let exact = gridcraft_ui_egui::theme::cell_text_width(&h.ctx, text, &font, true); + assert!(exact > 0.0 && quick >= exact, "{text:?} bold={bold}: quick {quick} < drawn {exact}"); + } + } +} From 2c23b61908e326b5534e10a6d3ad56fb304e785a Mon Sep 17 00:00:00 2001 From: midasmatterhorn <248507632+midasmatterhorn@users.noreply.github.com> Date: Wed, 14 Oct 2026 23:51:59 +0300 Subject: [PATCH 12/28] engine: subtotal honours header:false instead of dropping the first data row --- crates/engine/src/cmd/data.rs | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..75ab9228e 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -1246,6 +1246,7 @@ fn show_levels(s: &mut Session, p: &Json) -> Result { fn subtotal(s: &mut Session, p: &Json) -> Result { let r = data_range(s, p)?; + let header = bool_param(p, "header").unwrap_or(true); let group_col = col_param(p.get("groupBy"), r.start.col).unwrap_or(r.start.col); let func = str_param(p, "function").unwrap_or("sum"); let code = match func { @@ -1274,8 +1275,9 @@ fn subtotal(s: &mut Session, p: &Json) -> Result { let d = s.doc()?; let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; let mut groups: Vec<(u32, u32, String)> = Vec::new(); - let mut start = r.start.row + 1; - for row in r.start.row + 1..=r.end.row + 1 { + let first = r.start.row + u32::from(header); + let mut start = first; + for row in first..=r.end.row + 1 { let cur = sh.value(CellRef::new(row, group_col)).display(); let prev = sh.value(CellRef::new(start, group_col)).display(); if row > r.end.row || cur != prev { @@ -1303,7 +1305,7 @@ fn subtotal(s: &mut Session, p: &Json) -> Result { s.execute("home.insertRows", json!({"rows": format!("{}:{}", last + 1, last + 1)}))?; s.execute("cell.set", json!({"cell": lbl, "input": format!("Grand {label}")}))?; for c in &cols { - let range = RangeRef::new(CellRef::new(r.start.row + 1, *c), CellRef::new(last - 1, *c)).a1(); + let range = RangeRef::new(CellRef::new(first, *c), CellRef::new(last - 1, *c)).a1(); s.execute("cell.set", json!({"cell": CellRef::new(last, *c).a1(), "input": format!("=SUBTOTAL({code},{range})")}))?; } Ok(json!({"groups": inserted})) @@ -1313,6 +1315,22 @@ fn subtotal(s: &mut Session, p: &Json) -> Result { mod tests { use super::*; + #[test] + fn subtotal_honours_header_false() { + // #131: with header:false the first data row was treated as a header and left out of its + // group's subtotal and the grand total. + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["East", 10], ["East", 20], ["West", 30], ["West", 40]]})).unwrap(); + s.execute("data.subtotal", json!({"range": "A1:B4", "groupBy": "A", "function": "sum", "columns": ["B"], "header": false})).unwrap(); + let get = |s: &mut Session, a: &str| s.execute("cell.get", json!({"cell": a})).unwrap(); + assert_eq!(get(&mut s, "A1")["value"], json!("East")); + assert_eq!(get(&mut s, "A3")["text"], json!("East Total")); + assert_eq!(get(&mut s, "B3")["value"], json!(30.0), "East's subtotal counts both East rows"); + assert_eq!(get(&mut s, "A7")["text"], json!("Grand Total")); + assert_eq!(get(&mut s, "B7")["value"], json!(100.0), "the grand total counts every data row"); + } + #[test] fn filter_by_huge_numeric_column_is_rejected_not_overflowed() { let mut s = Session::new(); From 2bb222aea68e6101406cf1b50a03d0b55ae20ae5 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:48:47 +0200 Subject: [PATCH 13/28] Sort and de-duplicate data.textToColumns fixed-width breaks Found while testing crates/engine/src/cmd/data.rs:826 Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- crates/engine/src/cmd/data.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..7e090753e 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -824,7 +824,10 @@ fn text_to_columns(s: &mut Session, p: &Json) -> Result { Some(w) => { let chars: Vec = text.chars().collect(); let mut cuts = vec![0]; - cuts.extend(w.iter().copied().filter(|x| *x < chars.len())); + let mut breaks: Vec = w.iter().copied().filter(|x| *x > 0 && *x < chars.len()).collect(); + breaks.sort_unstable(); + breaks.dedup(); + cuts.extend(breaks); cuts.push(chars.len()); cuts.windows(2) .map(|p| chars.get(p[0]..p[1]).map(|s| s.iter().collect::().trim().to_string()).unwrap_or_default()) @@ -1493,6 +1496,26 @@ mod tests { } } + #[test] + fn t2c_fixed_widths_are_positions_in_any_order_without_duplicates() { + let cases: [(Json, [&str; 4]); 5] = [ + (json!([6, 3]), ["abc", "def", "ghij", ""]), + (json!([3, 3]), ["abc", "defghij", "", ""]), + (json!([0]), ["abcdefghij", "", "", ""]), + (json!([20]), ["abcdefghij", "", "", ""]), + (json!([7, 0, 3, 7, 10]), ["abc", "defg", "hij", ""]), + ]; + for (widths, want) in cases { + let mut s = Session::new(); + s.new_workbook(); + s.execute("cell.set", json!({"cell": "A1", "input": "abcdefghij"})).unwrap(); + s.execute("data.textToColumns", json!({"range": "A1:A1", "fixedWidths": widths})).unwrap(); + let sh = s.doc().unwrap().wb.active().unwrap(); + let got: Vec = (0..4).map(|c| sh.value(CellRef::new(0, c)).display()).collect(); + assert_eq!(got, want, "{widths}"); + } + } + #[test] fn split() { assert_eq!(split_delimited("a,b,,c", &[','], false, '"'), ["a", "b", "", "c"]); From 47185eef1f32ddd96e07fcacbd21201471dd52c7 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Tue, 13 Oct 2026 13:42:33 +0200 Subject: [PATCH 14/28] XLSX export: a picture shared by several sheets is one media part (#985) The writer gave every drawing its own xl/media/imageN part even when the pictures had the same bytes, so a workbook with one logo on 16 sheets grew 16 copies of it on a plain open and save. Out::image_part now returns the existing part when an identical picture was already written (same extension and bytes), and every drawing's relationship points to it, as Excel stores a copied picture. --- crates/xlsx/src/sheet_write.rs | 6 +- crates/xlsx/src/tests/mod.rs | 1 + crates/xlsx/src/tests/shared_media.rs | 87 +++++++++++++++++++++++++++ crates/xlsx/src/write.rs | 14 +++++ 4 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 crates/xlsx/src/tests/shared_media.rs diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index 78e1afaae..f961baac3 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -498,12 +498,10 @@ pub fn write_sheet(wb: &Workbook, si: usize, selected: bool, out: &mut Out) -> ( if img.data.is_empty() { continue; } - out.images += 1; let ext = image_ext(&img.mime, &img.data); - let name = format!("xl/media/image{}.{ext}", out.images); out.default_type(ext, image_content_type(ext)); - out.part(&name, None, img.data.clone()); - let rid = drels.add("image", &format!("../media/image{}.{ext}", out.images)); + let file = out.image_part(ext, &img.data); + let rid = drels.add("image", &format!("../media/{file}")); objs.push(Obj::Image(img, rid)); } for ch in &sheet.charts { diff --git a/crates/xlsx/src/tests/mod.rs b/crates/xlsx/src/tests/mod.rs index 517113d0f..387e7d603 100644 --- a/crates/xlsx/src/tests/mod.rs +++ b/crates/xlsx/src/tests/mod.rs @@ -13,6 +13,7 @@ mod passthrough_tests; mod pivot_tests; mod richdata_tests; mod roundtrip; +mod shared_media; mod sheet_names; mod smooth_roundtrip; mod trendline_roundtrip; diff --git a/crates/xlsx/src/tests/shared_media.rs b/crates/xlsx/src/tests/shared_media.rs new file mode 100644 index 000000000..1422a2663 --- /dev/null +++ b/crates/xlsx/src/tests/shared_media.rs @@ -0,0 +1,87 @@ +//! A picture used on several sheets is written as one media part that every drawing points to. + +use std::io::Read; +use std::sync::Arc; + +use gridcraft_core::CellRef; +use gridcraft_model::{Anchor, Image, Sheet, Workbook}; + +use crate::{read_xlsx, write_xlsx}; + +fn png(fill: u8) -> Vec { + let mut v = b"\x89PNG\r\n\x1a\n".to_vec(); + v.extend(std::iter::repeat_n(fill, 4096)); + v +} + +fn sheet_with(name: &str, pictures: &[(u32, &[u8])]) -> Arc { + let mut s = Sheet::new(name); + for (id, data) in pictures { + let anchor = Anchor { cell: CellRef::parse("B2").unwrap(), width: 40.0, height: 30.0, ..Default::default() }; + s.images.push(Image { id: *id, anchor, data: data.to_vec(), mime: "image/png".into(), alt: String::new() }); + } + Arc::new(s) +} + +fn names(bytes: &[u8]) -> Vec { + let z = zip::ZipArchive::new(std::io::Cursor::new(bytes.to_vec())).unwrap(); + z.file_names().map(str::to_string).collect() +} + +fn text(bytes: &[u8], name: &str) -> String { + let mut z = zip::ZipArchive::new(std::io::Cursor::new(bytes.to_vec())).unwrap(); + let mut s = String::new(); + z.by_name(name).unwrap().read_to_string(&mut s).unwrap(); + s +} + +#[test] +fn a_picture_on_several_sheets_is_one_media_part() { + let (a, b) = (png(1), png(2)); + // `a` on three sheets (and twice on one of them), `b` on one: two media parts, not five. + let wb = Workbook { + sheets: vec![sheet_with("One", &[(1, &a)]), sheet_with("Two", &[(1, &a), (2, &b)]), sheet_with("Three", &[(1, &a), (2, &a)])], + ..Default::default() + }; + let bytes = write_xlsx(&wb).unwrap(); + let media: Vec = names(&bytes).into_iter().filter(|n| n.starts_with("xl/media/")).collect(); + assert_eq!(media.len(), 2, "{media:?}"); + // Every drawing relationship points to an existing part, and the three sheets that show `a` + // point to the same one. + let mut targets = vec![]; + for d in 1..=3 { + let rels = text(&bytes, &format!("xl/drawings/_rels/drawing{d}.xml.rels")); + for t in rels.split("Target=\"").skip(1) { + let t = t.split('"').next().unwrap(); + let part = format!("xl/media/{}", t.trim_start_matches("../media/")); + assert!(media.contains(&part), "drawing{d}: dangling {t}"); + targets.push((d, part)); + } + } + assert_eq!(targets.len(), 5, "{targets:?}"); + let distinct: std::collections::BTreeSet<&String> = targets.iter().map(|(_, p)| p).collect(); + assert_eq!(distinct.len(), 2, "{targets:?}"); + let first_of = |d: usize| targets.iter().find(|(x, _)| *x == d).map(|(_, p)| p.clone()); + assert_eq!(first_of(1), first_of(2), "sheets One and Two show the same picture"); + assert_eq!(first_of(1), first_of(3)); + // They read back with the right bytes on the right sheets. + let back = read_xlsx(&bytes).unwrap().0; + let data = |i: usize| back.sheets[i].images.iter().map(|im| im.data.clone()).collect::>(); + assert_eq!(data(0), vec![a.clone()]); + assert_eq!(data(1), vec![a.clone(), b.clone()]); + assert_eq!(data(2), vec![a.clone(), a.clone()]); + // A second save of what was read back is stable. + let again = write_xlsx(&back).unwrap(); + assert_eq!(names(&again).iter().filter(|n| n.starts_with("xl/media/")).count(), 2); +} + +#[test] +fn different_pictures_with_the_same_length_stay_separate() { + let (a, b) = (png(1), png(2)); + assert_eq!(a.len(), b.len()); + let wb = Workbook { sheets: vec![sheet_with("One", &[(1, &a), (2, &b)])], ..Default::default() }; + let bytes = write_xlsx(&wb).unwrap(); + assert_eq!(names(&bytes).iter().filter(|n| n.starts_with("xl/media/")).count(), 2); + let back = read_xlsx(&bytes).unwrap().0; + assert_eq!(back.sheets[0].images.iter().map(|i| i.data.clone()).collect::>(), vec![a, b]); +} diff --git a/crates/xlsx/src/write.rs b/crates/xlsx/src/write.rs index 06761f01d..7c63e59b4 100644 --- a/crates/xlsx/src/write.rs +++ b/crates/xlsx/src/write.rs @@ -135,6 +135,20 @@ impl Out { } self.parts.push((name.to_string(), data)); } + /// The `xl/media` file name (`imageN.ext`) holding `data`. A picture used on several sheets, + /// or twice on one, is written once and every drawing points to that part, as in Excel. + pub fn image_part(&mut self, ext: &str, data: &[u8]) -> String { + let existing = self.parts.iter().find(|(name, bytes)| { + name.strip_prefix("xl/media/image").is_some_and(|rest| rest.ends_with(&format!(".{ext}"))) && bytes.as_slice() == data + }); + if let Some((name, _)) = existing { + return name.trim_start_matches("xl/media/").to_string(); + } + self.images += 1; + let file = format!("image{}.{ext}", self.images); + self.part(&format!("xl/media/{file}"), None, data.to_vec()); + file + } pub fn default_type(&mut self, ext: &str, ct: &str) { self.defaults.entry(ext.to_string()).or_insert_with(|| ct.to_string()); } From 9f6bd5848cf3aa8bc7002906e39aea130e78dc8a Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Tue, 13 Oct 2026 13:42:33 +0200 Subject: [PATCH 15/28] Docs: update the progress docs for this change --- ROADMAP.md | 2 ++ docs/file-format-parity.md | 9 +++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..b444df236 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-13 | XLSX export writes a picture used on several sheets once, as one `xl/media` part every drawing points to (it wrote a copy per drawing: a 16-sheet workbook with one 112 KB logo grew from 0.4 to 1.9 MB) (#985) | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-13 | trivial | Progress log: shared pictures written once on XLSX export (#985) | | 2026-10-12 | minor | Rows auto-fit oversized fonts on open and whole-row/column formatting (#833); progress log row | | 2026-10-12 | minor | Progress log: multi-area row/column delete (#758) | | 2026-10-12 | trivial | Progress log: XLSX chart sheets kept on save (#727) | diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index 158149fb3..fd78e0288 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -1,6 +1,6 @@ # File-format parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** trivial (formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; previously trivial: XLSX chart sheets are kept on save (#727)) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** trivial (XLSX export keeps a picture shared by several sheets as one media part (#985); previously trivial: formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; previously trivial: XLSX chart sheets are kept on save (#727)) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Every format Excel opens or saves, with GridCraft's read and write support, fidelity and tests. Part of [target-app-parity.md](target-app-parity.md). **File formats: ~50% ready, 80–120 h** @@ -22,9 +22,9 @@ and repair-free opening remain unverified. | Format | Excel | GridCraft read | GridCraft write | Fidelity / notes | Tests | |---|---|---|---|---|---| -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Kept unchanged on save (pass-through, `passthrough.rs`):** custom document properties, ribbon customisation and add-in task panes, custom XML, links to external workbooks (never followed or refreshed), data connections, sheet query tables, printer settings (#324), form controls, ActiveX controls, OLE objects and sheet backgrounds (with their VML shapes), and chart sheets (#727: the sheet, drawing, chart, style and colour parts are written back unchanged in their place, and left out when a sheet they plot was deleted or renamed or a worksheet took their name; they can't be edited or created); dropped when their sheet is deleted or their parts point at content GridCraft rewrites. **Not preserved:** slicers and slicer caches, timelines, data model, query tables attached to tables, sheet and workbook `extLst` entries we don't model, chart details; Strict Open XML files keep nothing. Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, notes (with their shown/hidden state) and threaded comments with replies, resolved state and timestamps (`threadedComments` and `persons` parts, #300), hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, Allow Edit Ranges (`protectedRanges` with their password hashes, #307), calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Kept unchanged on save (pass-through, `passthrough.rs`):** custom document properties, ribbon customisation and add-in task panes, custom XML, links to external workbooks (never followed or refreshed), data connections, sheet query tables, printer settings (#324), form controls, ActiveX controls, OLE objects and sheet backgrounds (with their VML shapes), and chart sheets (#727: the sheet, drawing, chart, style and colour parts are written back unchanged in their place, and left out when a sheet they plot was deleted or renamed or a worksheet took their name; they can't be edited or created); dropped when their sheet is deleted or their parts point at content GridCraft rewrites. **Not preserved:** slicers and slicer caches, timelines, data model, query tables attached to tables, sheet and workbook `extLst` entries we don't model, chart details; Strict Open XML files keep nothing. Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` A picture used on several sheets is written once (#985). | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, notes (with their shown/hidden state) and threaded comments with replies, resolved state and timestamps (`threadedComments` and `persons` parts, #300), hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` A picture used on several sheets is written once (#985). | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, Allow Edit Ranges (`protectedRanges` with their password hashes, #307), calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` A picture used on several sheets is written once (#985). | | `.xlsx` Strict Open XML | R/W | yes | no (writes transitional) | Excel also saves transitional by default | fixtures | | `.xlsm` Macro-Enabled Workbook | R/W | yes | yes, macros kept, not run | The VBA project is kept byte-for-byte and written back only into `.xlsm`/`.xltm` (#6); macros never run. Saved with the macro-enabled workbook content type | `roundtrip.rs` (`vba_project_roundtrip`, `package_kind_sets_workbook_content_type`), engine `io.rs` (`macros_are_saved_only_into_xlsm`) | | `.xltx` / `.xltm` templates | R/W | yes | partial | Saved with the template (and macro-enabled template) content type, chosen by extension; `.xltm` keeps a VBA project, `.xltx` drops it; "new from template" behaviour missing | `roundtrip.rs` (`package_kind_sets_workbook_content_type`), engine `io.rs` | @@ -86,6 +86,7 @@ refresh. Counted under features (Get & Transform, ~10% ready), not here. | Date | Change | Summary | |---|---|---| | 2026-10-14 | trivial | Formula parser: text longer than Excel's 8,192 characters, and postfix chains (`1%%%…`, `f(1)(1)…`) past the 256-level nesting limit, are parse errors (the cell keeps its formula text and cached value) instead of trees deep enough to overflow the stack when a hostile file is opened, saved or dropped | +| 2026-10-13 | trivial | XLSX export keeps a picture shared by several sheets as one media part (#985) | | 2026-10-12 | trivial | XLSX chart sheets are kept on save (#727) | | 2026-10-12 | trivial | CSV and TSV export is capped at 512 MiB and refuses a larger used range with an error (#528) | | 2026-10-12 | minor | `.xls` import reads the calculation settings (CalcMode, CalcIter, CalcCount, CalcDelta, CalcPrecision), so circular models calculate instead of showing `#CIRC!` (#724) | From d50e32bf250cfd3715006d1d249e44260d99c1ae Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Tue, 13 Oct 2026 15:05:56 +0200 Subject: [PATCH 16/28] Docs: put the new sentence in the Fidelity / notes column Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- docs/file-format-parity.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index fd78e0288..fa0c2aec5 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -22,9 +22,9 @@ and repair-free opening remain unverified. | Format | Excel | GridCraft read | GridCraft write | Fidelity / notes | Tests | |---|---|---|---|---|---| -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Kept unchanged on save (pass-through, `passthrough.rs`):** custom document properties, ribbon customisation and add-in task panes, custom XML, links to external workbooks (never followed or refreshed), data connections, sheet query tables, printer settings (#324), form controls, ActiveX controls, OLE objects and sheet backgrounds (with their VML shapes), and chart sheets (#727: the sheet, drawing, chart, style and colour parts are written back unchanged in their place, and left out when a sheet they plot was deleted or renamed or a worksheet took their name; they can't be edited or created); dropped when their sheet is deleted or their parts point at content GridCraft rewrites. **Not preserved:** slicers and slicer caches, timelines, data model, query tables attached to tables, sheet and workbook `extLst` entries we don't model, chart details; Strict Open XML files keep nothing. Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` A picture used on several sheets is written once (#985). | -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, notes (with their shown/hidden state) and threaded comments with replies, resolved state and timestamps (`threadedComments` and `persons` parts, #300), hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` A picture used on several sheets is written once (#985). | -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, Allow Edit Ranges (`protectedRanges` with their password hashes, #307), calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` A picture used on several sheets is written once (#985). | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Kept unchanged on save (pass-through, `passthrough.rs`):** custom document properties, ribbon customisation and add-in task panes, custom XML, links to external workbooks (never followed or refreshed), data connections, sheet query tables, printer settings (#324), form controls, ActiveX controls, OLE objects and sheet backgrounds (with their VML shapes), and chart sheets (#727: the sheet, drawing, chart, style and colour parts are written back unchanged in their place, and left out when a sheet they plot was deleted or renamed or a worksheet took their name; they can't be edited or created); dropped when their sheet is deleted or their parts point at content GridCraft rewrites. **Not preserved:** slicers and slicer caches, timelines, data model, query tables attached to tables, sheet and workbook `extLst` entries we don't model, chart details; Strict Open XML files keep nothing. Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) A picture used on several sheets is written once (#985). | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, notes (with their shown/hidden state) and threaded comments with replies, resolved state and timestamps (`threadedComments` and `persons` parts, #300), hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) A picture used on several sheets is written once (#985). | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, Allow Edit Ranges (`protectedRanges` with their password hashes, #307), calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) A picture used on several sheets is written once (#985). | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | | `.xlsx` Strict Open XML | R/W | yes | no (writes transitional) | Excel also saves transitional by default | fixtures | | `.xlsm` Macro-Enabled Workbook | R/W | yes | yes, macros kept, not run | The VBA project is kept byte-for-byte and written back only into `.xlsm`/`.xltm` (#6); macros never run. Saved with the macro-enabled workbook content type | `roundtrip.rs` (`vba_project_roundtrip`, `package_kind_sets_workbook_content_type`), engine `io.rs` (`macros_are_saved_only_into_xlsm`) | | `.xltx` / `.xltm` templates | R/W | yes | partial | Saved with the template (and macro-enabled template) content type, chosen by extension; `.xltm` keeps a VBA project, `.xltx` drops it; "new from template" behaviour missing | `roundtrip.rs` (`package_kind_sets_workbook_content_type`), engine `io.rs` | From efc8171609c7c2351868cf41f1da613b07e38ae3 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Fri, 16 Oct 2026 07:03:58 +0200 Subject: [PATCH 17/28] Keyboard: Ctrl+6 hides and shows objects, saved as workbookPr showObjects --- crates/engine/src/cmd/view.rs | 11 +++ crates/model/src/lib.rs | 2 +- crates/model/src/workbook.rs | 18 ++++ crates/ui-egui/src/chartview.rs | 15 +++- crates/ui-egui/src/grid.rs | 6 +- crates/ui-egui/src/keymap.rs | 2 + crates/ui-egui/tests/keyboard_shortcuts.rs | 14 ++++ crates/ui-egui/tests/show_objects.rs | 96 ++++++++++++++++++++++ crates/xlsx/src/read.rs | 15 ++-- crates/xlsx/src/tests/roundtrip.rs | 45 ++++++++++ crates/xlsx/src/write.rs | 13 ++- docs/file-format-parity.md | 9 +- docs/ui-parity.md | 6 +- 13 files changed, 235 insertions(+), 17 deletions(-) create mode 100644 crates/ui-egui/tests/show_objects.rs diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index edcacf57d..d21c2cab9 100644 --- a/crates/engine/src/cmd/view.rs +++ b/crates/engine/src/cmd/view.rs @@ -26,6 +26,7 @@ pub fn specs() -> Vec { cmd!("view.gridlines", "Gridlines", ["View", "Show"], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "gridlines")), cmd!("view.headings", "Headings", ["View", "Show"], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "headings")), cmd!("view.showZeros", "Show Zeros", [], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "zeros")), + cmd!("view.showObjects", "Show Objects", [], Some("Ctrl+6"), "{on?}", has_doc, show_objects), cmd!("view.rightToLeft", "Sheet Right-to-Left", ["Page Layout", "Sheet Options"], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "rtl")), cmd!("view.zoom", "Zoom", ["View", "Zoom"], None, "{percent: 10..400}", has_doc, zoom), cmd!("view.zoom100", "100%", ["View", "Zoom"], None, "{}", has_doc, |s, _| zoom(s, &json!({"percent": 100}))), @@ -152,6 +153,16 @@ fn sheet_flag(s: &mut Session, p: &Json, which: &str) -> Result { }) } +/// Ctrl+6: hides every object, or shows them again. Placeholders (from a file) count as shown. +fn show_objects(s: &mut Session, p: &Json) -> Result { + use gridcraft_model::ShowObjects; + edit(s, |cx| { + let on = bool_param(p, "on").unwrap_or(cx.wb.show_objects == ShowObjects::None); + cx.wb.show_objects = if on { ShowObjects::All } else { ShowObjects::None }; + Ok(json!({"on": on})) + }) +} + fn zoom(s: &mut Session, p: &Json) -> Result { let z = f64_param(p, "percent").ok_or_else(|| bad("view.zoom", "missing `percent`"))?.clamp(10.0, 400.0) as u16; let d = s.doc_mut()?; diff --git a/crates/model/src/lib.rs b/crates/model/src/lib.rs index a76f30fc3..23fa4f50b 100644 --- a/crates/model/src/lib.rs +++ b/crates/model/src/lib.rs @@ -25,7 +25,7 @@ pub use style::{ Alignment, BorderLine, BorderStyle, Borders, Color, Fill, Font, HAlign, NumFmt, PatternType, Style, StyleId, StyleTable, Theme, Underline, VAlign, VertAlign, }; -pub use workbook::{CalcMode, CalcSettings, DefinedName, DocProps, Workbook}; +pub use workbook::{CalcMode, CalcSettings, DefinedName, DocProps, ShowObjects, Workbook}; /// Font used for new workbooks. Only a name: renderers substitute an available open font /// (e.g. the metric-compatible Carlito) when it isn't installed. diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 828ca692e..c9662bacd 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -18,6 +18,17 @@ pub enum CalcMode { Manual, } +/// Options › Advanced › "For objects, show" (``, ECMA-376 `ST_Objects`): +/// whether charts, pictures, shapes and slicers are drawn. Ctrl+6 switches between `All` and +/// `None`; `Placeholders` only comes from files and is kept as it is. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub enum ShowObjects { + #[default] + All, + Placeholders, + None, +} + /// File › Options › Formulas › Calculation options, stored per workbook (``). #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct CalcSettings { @@ -93,6 +104,8 @@ pub struct Workbook { pub theme: Theme, pub date_system: DateSystem, pub calc: CalcSettings, + #[serde(default, skip_serializing_if = "is_all")] + pub show_objects: ShowObjects, pub active_sheet: usize, pub props: DocProps, /// Workbook structure protection. @@ -122,6 +135,10 @@ pub struct Workbook { pub source_dir: Option, } +fn is_all(s: &ShowObjects) -> bool { + *s == ShowObjects::All +} + impl Default for Workbook { fn default() -> Self { Workbook::new() @@ -138,6 +155,7 @@ impl Workbook { theme: Theme::default(), date_system: DateSystem::D1900, calc: CalcSettings::default(), + show_objects: ShowObjects::All, active_sheet: 0, props: DocProps::default(), protected_structure: false, diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 180f15c21..f54e609cd 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -258,8 +258,17 @@ pub(crate) struct PaintedObject { pub rect: Rect, } +/// Ctrl+6 (Options › Advanced › "For objects, show: Nothing"): objects are neither drawn nor +/// clickable. Placeholders, which Excel no longer offers, show the objects. +pub(crate) fn objects_hidden(wb: &Workbook) -> bool { + wb.show_objects == gridcraft_engine::model::ShowObjects::None +} + pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: usize, sh: &Sheet) -> Vec { let mut painted = Vec::new(); + if objects_hidden(wb) { + return painted; + } let mut done = |start: egui::layers::ShapeIdx, rect: Rect| painted.push(PaintedObject { shapes: start.0..p.add(egui::Shape::Noop).0, rect }); for sp in &sh.shapes { let r = anchor_rect(geo, sh, &sp.anchor); @@ -472,7 +481,7 @@ pub enum SlicerAction { /// A click on a slicer's value tile or clear button, if any. A click elsewhere in a slicer returns /// `None` so the caller can treat it as a select/drag on the object. pub fn slicer_click(geo: &Geo, sh: &Sheet, wb: &Workbook, si: usize, p: Pos2) -> Option<(u32, SlicerAction)> { - if !geo.cells.contains(p) { + if !geo.cells.contains(p) || objects_hidden(wb) { return None; } for sl in sh.slicers.iter().rev() { @@ -506,7 +515,9 @@ fn selection_frame(p: &Painter, r: Rect) { /// The topmost object under `p`: (kind, id, rect). pub fn hit(app: &SheetApp, geo: &Geo, sh: &Sheet, p: Pos2) -> Option<(&'static str, u32, Rect)> { - let _ = app; + if app.session.active().is_some_and(|d| objects_hidden(&d.wb)) { + return None; + } for sl in sh.slicers.iter().rev() { let r = anchor_rect(geo, sh, &sl.anchor); if r.contains(p) && geo.cells.contains(p) { diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 00e0c18be..5cf95cadf 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -726,7 +726,11 @@ pub fn show(app: &mut SheetApp, ui: &mut egui::Ui) { } else { paint_panes(app, &painter, &geo, &wb, si, sh, &sel, outline, &mut cf, &t) }; - // Objects above cells. + // Objects above cells. A hidden object can't stay selected: Delete or the Format pane would act + // on something the user can't see. + if crate::chartview::objects_hidden(&wb) { + app.selected_chart = None; + } let objects = crate::chartview::paint_objects(app, &painter.with_clip_rect(geo.cells), &geo, &wb, si, sh); paint_overlays(app, ui, &painter.with_clip_rect(geo.cells), &geo, &wb, si, sh, &sel); if sh.view == gridcraft_engine::model::SheetViewMode::PageBreakPreview { diff --git a/crates/ui-egui/src/keymap.rs b/crates/ui-egui/src/keymap.rs index 032db1a2d..358db5a5f 100644 --- a/crates/ui-egui/src/keymap.rs +++ b/crates/ui-egui/src/keymap.rs @@ -92,6 +92,8 @@ pub fn lookup(key: Key, physical: Option, m: Modifiers, mac: bool) -> Optio Num3 if ctrl && !mac => run("home.italic"), Num4 if ctrl && !mac => run("home.underline"), Num5 if ctrl && !mac => run("home.strikethrough"), + // Ctrl+6 hides and shows objects (Windows, and Control+6 in Excel for Mac). + Num6 if ctrl && !s => run("view.showObjects"), // Letters that differ between Windows and Mac. L if (cmd || ctrl) && s => run("data.filter"), L if cmd && mac => run("home.alignLeft"), diff --git a/crates/ui-egui/tests/keyboard_shortcuts.rs b/crates/ui-egui/tests/keyboard_shortcuts.rs index c0a1307b3..3e2829a66 100644 --- a/crates/ui-egui/tests/keyboard_shortcuts.rs +++ b/crates/ui-egui/tests/keyboard_shortcuts.rs @@ -214,3 +214,17 @@ fn alt_down_only_opens_enabled_table_filter_buttons() { assert_eq!(h.state().session.active().unwrap().selection.active.a1(), "B1"); } } + +#[test] +fn ctrl_6_is_show_objects_and_nothing_else() { + let show = Some(Shortcut::Run("view.showObjects", json!({}))); + let mac_control = Modifiers::CTRL; + assert_eq!(lookup(Key::Num6, Some(Key::Num6), CTRL, false), show); + assert_eq!(lookup(Key::Num6, Some(Key::Num6), mac_control, true), show); + // Its neighbours keep theirs: Ctrl+Shift+6 (Ctrl+^) is Scientific, Cmd+6 on a Mac is free. + assert_eq!(lookup(Key::Num6, Some(Key::Num6), CTRL_SHIFT, false), Some(Shortcut::Run("home.numberFormat", json!({"code": "0.00E+00"})))); + assert_eq!(lookup(Key::Num6, Some(Key::Num6), Modifiers::MAC_CMD, true), None); + // No other command claims Ctrl+6. + let claims: Vec<&str> = Session::new().commands().iter().filter(|c| c.shortcut == Some("Ctrl+6")).map(|c| c.id).collect(); + assert_eq!(claims, ["view.showObjects"]); +} diff --git a/crates/ui-egui/tests/show_objects.rs b/crates/ui-egui/tests/show_objects.rs new file mode 100644 index 000000000..9dcac7ce8 --- /dev/null +++ b/crates/ui-egui/tests/show_objects.rs @@ -0,0 +1,96 @@ +//! Ctrl+6 hides and shows every object (Options › Advanced › "For objects, show"), through real input. + +use egui::{Color32, Event, Key, Modifiers, PointerButton, epaint::Shape, vec2}; +use gridcraft_engine::Session; +use gridcraft_engine::core::CellRef; +use gridcraft_engine::model::ShowObjects; +use gridcraft_ui_egui::{SheetApp, grid::Geo}; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +const FILL: Color32 = Color32::from_rgb(0x12, 0x34, 0x56); +/// Ctrl on Windows and Linux sets both `ctrl` and `command`. +const CTRL: Modifiers = Modifiers { alt: false, ctrl: true, shift: false, mac_cmd: false, command: true }; + +/// A rectangle at D3, 144 × 96 points, filled with a colour nothing else on screen uses. +fn fixture() -> (Harness, u32) { + let mut session = Session::new(); + session.new_workbook(); + let shape = session.execute("insert.shape", json!({"kind": "rectangle", "at": "D3", "fill": "123456"})).unwrap(); + let id = shape["shape"].as_u64().unwrap() as u32; + session.execute("selection.set", json!({"range": "A1"})).unwrap(); + let mut h = egui_kittest::Harness::builder().with_size(vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + SheetApp::new(session, Default::default()), + ); + h.run_steps(4); + (h, id) +} + +fn ctrl_6(h: &mut Harness) { + h.input_mut().events.push(Event::Key { key: Key::Num6, physical_key: Some(Key::Num6), pressed: true, repeat: false, modifiers: CTRL }); + h.input_mut().events.push(Event::Key { key: Key::Num6, physical_key: Some(Key::Num6), pressed: false, repeat: false, modifiers: CTRL }); + h.run_steps(2); +} + +fn show_objects(h: &Harness) -> ShowObjects { + h.state().session.active().unwrap().wb.show_objects +} + +/// Whether the last frame painted the rectangle's fill. +fn painted(h: &Harness) -> bool { + h.output().shapes.iter().any(|c| matches!(&c.shape, Shape::Rect(r) if r.fill == FILL)) +} + +/// Inside the rectangle, in the cell E4 (D3 is 64 points wide and 20 high at 100%). +fn inside(h: &Harness) -> egui::Pos2 { + let app = h.state(); + let sh = app.session.active().unwrap().wb.active().unwrap(); + let geo = Geo::new(sh, app.grid.rect.unwrap(), app.view().scroll); + geo.cell_rect(sh, CellRef::parse("E4").unwrap()).center() +} + +fn click(h: &mut Harness, pos: egui::Pos2) { + h.input_mut().events.push(Event::PointerMoved(pos)); + h.run_steps(2); + for pressed in [true, false] { + h.input_mut().events.push(Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }); + h.run_steps(2); + } +} + +fn active_cell(h: &Harness) -> CellRef { + h.state().session.active().unwrap().selection.active +} + +#[test] +fn ctrl_6_hides_objects_and_a_second_ctrl_6_shows_them() { + let (mut h, _) = fixture(); + assert_eq!(show_objects(&h), ShowObjects::All); + assert!(painted(&h), "the rectangle is drawn to begin with"); + ctrl_6(&mut h); + assert_eq!(show_objects(&h), ShowObjects::None); + assert!(!painted(&h), "hidden objects are not drawn"); + ctrl_6(&mut h); + assert_eq!(show_objects(&h), ShowObjects::All); + assert!(painted(&h), "a second Ctrl+6 draws them again"); +} + +#[test] +fn hidden_objects_do_not_take_clicks() { + let (mut h, id) = fixture(); + let at = inside(&h); + click(&mut h, at); + assert_eq!(h.state().selected_chart, Some(id), "the click lands on the rectangle while it shows"); + assert_eq!(active_cell(&h), CellRef::parse("A1").unwrap()); + ctrl_6(&mut h); + assert_eq!(h.state().selected_chart, None, "hiding objects drops the selection on one"); + click(&mut h, at); + assert_eq!(h.state().selected_chart, None); + assert_eq!(active_cell(&h), CellRef::parse("E4").unwrap(), "the click selects the cell under the hidden rectangle"); +} diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 1a479081c..4dfb18a28 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use gridcraft_core::{DateSystem, RangeRef}; -use gridcraft_model::{CalcMode, CellPicture, DefinedName, Sheet, Style, StyleId, StyleTable, Visibility, Workbook}; +use gridcraft_model::{CalcMode, CellPicture, DefinedName, Sheet, ShowObjects, Style, StyleId, StyleTable, Visibility, Workbook}; use crate::package::{Package, Rel}; use crate::styles::{StylesIn, read_styles}; @@ -169,10 +169,15 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { } // Workbook properties. - if let Some(p) = wb_xml.child("workbookPr") - && p.flag("date1904", false) - { - cx.date_system = DateSystem::D1904; + if let Some(p) = wb_xml.child("workbookPr") { + if p.flag("date1904", false) { + cx.date_system = DateSystem::D1904; + } + wb.show_objects = match p.attr("showObjects") { + Some("none") => ShowObjects::None, + Some("placeholders") => ShowObjects::Placeholders, + _ => ShowObjects::All, + }; } wb.date_system = cx.date_system; if let Some(c) = wb_xml.child("calcPr") { diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 901802905..945c3da53 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -679,6 +679,51 @@ fn iterative_calculation_settings_roundtrip() { } } +#[test] +fn show_objects_roundtrip() { + let saved = |wb: &Workbook| { + let bytes = write_xlsx(wb).unwrap(); + let mut pkg = Package::open(&bytes).unwrap(); + let book = pkg.read_xml("xl/workbook.xml").unwrap().unwrap(); + let attr = book.child("workbookPr").and_then(|p| p.attr("showObjects")).map(str::to_string); + (attr, read_xlsx(&bytes).unwrap().0.show_objects) + }; + // Default: nothing written, All read back. + let mut wb = Workbook::new(); + assert_eq!(saved(&wb), (None, ShowObjects::All)); + // Beside date1904, which shares the element. + wb.date_system = DateSystem::D1904; + for (show, attr) in [(ShowObjects::None, "none"), (ShowObjects::Placeholders, "placeholders")] { + wb.show_objects = show; + assert_eq!(saved(&wb), (Some(attr.to_string()), show)); + assert_eq!(read_xlsx(&write_xlsx(&wb).unwrap()).unwrap().0.date_system, DateSystem::D1904); + } + // A file written elsewhere: the attribute among the others Excel writes, or absent. + let book = |pr: &str| { + let wbx = format!( + r#"{pr}"# + ); + let rels = r#""#; + let ws = r#""#; + let bytes = super::make_zip(&[ + ("[Content_Types].xml", super::CT.as_bytes()), + ("_rels/.rels", super::ROOT_RELS.as_bytes()), + ("xl/workbook.xml", wbx.as_bytes()), + ("xl/_rels/workbook.xml.rels", rels.as_bytes()), + ("xl/worksheets/sheet1.xml", ws.as_bytes()), + ]); + read_xlsx(&bytes).unwrap().0 + }; + let hidden = book(r#""#); + assert_eq!(hidden.show_objects, ShowObjects::None); + assert_eq!(saved(&hidden).0.as_deref(), Some("none")); + for pr in [r#""#, "", r#""#] { + let wb = book(pr); + assert_eq!(wb.show_objects, ShowObjects::All, "{pr}"); + assert_eq!(saved(&wb).0, None, "{pr}: a file without the attribute stays without it"); + } +} + #[test] fn empty_workbook_roundtrip() { let wb = Workbook::new(); diff --git a/crates/xlsx/src/write.rs b/crates/xlsx/src/write.rs index 06761f01d..01655f931 100644 --- a/crates/xlsx/src/write.rs +++ b/crates/xlsx/src/write.rs @@ -5,7 +5,7 @@ use std::fmt::Write as _; use gridcraft_core::RangeRef; use gridcraft_formula::quote_sheet; -use gridcraft_model::{CalcMode, CalcSettings, Style, Visibility, Workbook}; +use gridcraft_model::{CalcMode, CalcSettings, ShowObjects, Style, Visibility, Workbook}; use crate::package::ZipOut; use crate::xml::{esc, esc_attr}; @@ -315,7 +315,16 @@ pub fn write_xlsx(wb: &Workbook, kind: XlsxKind) -> Result, IoError> { let mut w = format!("{XML_DECL}"); w.push_str(&crate::passthrough::workbook_elements(wb, "fileVersion")); - w.push_str(if wb.date_system == gridcraft_core::DateSystem::D1904 { "" } else { "" }); + w.push_str(" {} + ShowObjects::Placeholders => w.push_str(" showObjects=\"placeholders\""), + ShowObjects::None => w.push_str(" showObjects=\"none\""), + } + if wb.date_system == gridcraft_core::DateSystem::D1904 { + w.push_str(" date1904=\"1\""); + } + w.push_str("/>"); if wb.protected_structure { let pw = wb.structure_password.as_ref().map(|h| crate::sheet_write::password_attrs(h, "workbook")).unwrap_or_default(); let _ = write!(w, ""); diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index 158149fb3..a6f9d2639 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -1,6 +1,6 @@ # File-format parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** trivial (formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; previously trivial: XLSX chart sheets are kept on save (#727)) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-16 · **Change:** trivial (XLSX keeps the workbook's show-objects setting, `workbookPr showObjects`; previously trivial: formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; previously trivial: XLSX chart sheets are kept on save (#727)) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Every format Excel opens or saves, with GridCraft's read and write support, fidelity and tests. Part of [target-app-parity.md](target-app-parity.md). **File formats: ~50% ready, 80–120 h** @@ -22,9 +22,9 @@ and repair-free opening remain unverified. | Format | Excel | GridCraft read | GridCraft write | Fidelity / notes | Tests | |---|---|---|---|---|---| -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Kept unchanged on save (pass-through, `passthrough.rs`):** custom document properties, ribbon customisation and add-in task panes, custom XML, links to external workbooks (never followed or refreshed), data connections, sheet query tables, printer settings (#324), form controls, ActiveX controls, OLE objects and sheet backgrounds (with their VML shapes), and chart sheets (#727: the sheet, drawing, chart, style and colour parts are written back unchanged in their place, and left out when a sheet they plot was deleted or renamed or a worksheet took their name; they can't be edited or created); dropped when their sheet is deleted or their parts point at content GridCraft rewrites. **Not preserved:** slicers and slicer caches, timelines, data model, query tables attached to tables, sheet and workbook `extLst` entries we don't model, chart details; Strict Open XML files keep nothing. Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, notes (with their shown/hidden state) and threaded comments with replies, resolved state and timestamps (`threadedComments` and `persons` parts, #300), hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | -| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, Allow Edit Ranges (`protectedRanges` with their password hashes, #307), calc settings. **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings, show objects (`workbookPr showObjects`). **Kept unchanged on save (pass-through, `passthrough.rs`):** custom document properties, ribbon customisation and add-in task panes, custom XML, links to external workbooks (never followed or refreshed), data connections, sheet query tables, printer settings (#324), form controls, ActiveX controls, OLE objects and sheet backgrounds (with their VML shapes), and chart sheets (#727: the sheet, drawing, chart, style and colour parts are written back unchanged in their place, and left out when a sheet they plot was deleted or renamed or a worksheet took their name; they can't be edited or created); dropped when their sheet is deleted or their parts point at content GridCraft rewrites. **Not preserved:** slicers and slicer caches, timelines, data model, query tables attached to tables, sheet and workbook `extLst` entries we don't model, chart details; Strict Open XML files keep nothing. Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, notes (with their shown/hidden state) and threaded comments with replies, resolved state and timestamps (`threadedComments` and `persons` parts, #300), hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, calc settings, show objects (`workbookPr showObjects`). **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | +| `.xlsx` Excel Workbook (transitional) | R/W, native | yes | yes | Cells, shared/array/dynamic formulas, styles, themes, merges, CF, validation, tables, comments and notes, hyperlinks, names, freeze panes, outline, print settings, pictures, shapes, charts (simplified, see [chart-parity.md](chart-parity.md)), sparklines, PivotTables (caches and tables), protection, Allow Edit Ranges (`protectedRanges` with their password hashes, #307), calc settings, show objects (`workbookPr showObjects`). **Not preserved:** unmodelled parts and extensions (form controls, slicer caches, timelines, external links, OLE objects, custom XML, data model, query tables, chart details). Never checked in Excel across a corpus: repair prompts unknown. 100 MB files fail (#175) | `roundtrip.rs`, `fixtures.rs`, `malformed.rs`, `pivot_tests.rs` | | `.xlsx` Strict Open XML | R/W | yes | no (writes transitional) | Excel also saves transitional by default | fixtures | | `.xlsm` Macro-Enabled Workbook | R/W | yes | yes, macros kept, not run | The VBA project is kept byte-for-byte and written back only into `.xlsm`/`.xltm` (#6); macros never run. Saved with the macro-enabled workbook content type | `roundtrip.rs` (`vba_project_roundtrip`, `package_kind_sets_workbook_content_type`), engine `io.rs` (`macros_are_saved_only_into_xlsm`) | | `.xltx` / `.xltm` templates | R/W | yes | partial | Saved with the template (and macro-enabled template) content type, chosen by extension; `.xltm` keeps a VBA project, `.xltx` drops it; "new from template" behaviour missing | `roundtrip.rs` (`package_kind_sets_workbook_content_type`), engine `io.rs` | @@ -85,6 +85,7 @@ refresh. Counted under features (Get & Transform, ~10% ready), not here. | Date | Change | Summary | |---|---|---| +| 2026-10-16 | trivial | XLSX reads and writes the workbook's show-objects setting (`workbookPr showObjects`: all, placeholders, none), which Ctrl+6 switches; a file without it is saved without it | | 2026-10-14 | trivial | Formula parser: text longer than Excel's 8,192 characters, and postfix chains (`1%%%…`, `f(1)(1)…`) past the 256-level nesting limit, are parse errors (the cell keeps its formula text and cached value) instead of trees deep enough to overflow the stack when a hostile file is opened, saved or dropped | | 2026-10-12 | trivial | XLSX chart sheets are kept on save (#727) | | 2026-10-12 | trivial | CSV and TSV export is capped at 512 MiB and refuses a larger used range with an error (#528) | diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 22e221e69..8c0bd9e5a 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -1,6 +1,6 @@ # UI parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-13 · **Change:** minor (horizontal scrollbar in the sheet tab bar, #840) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-16 · **Change:** minor (Ctrl+6 hides and shows objects) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Interaction, controls and feel, against Excel. Part of [target-app-parity.md](target-app-parity.md). **UI/UX fidelity: ~55% ready, 50–80 h** (estimated). Ribbon command coverage is measured @@ -103,6 +103,7 @@ otherwise. Number-format and border keys match the physical key (Ctrl+Shift+4, w | Ctrl+Shift+~ ! @ # $ % ^ | Control+Shift | General, Number, Time, Date, Currency, Percent, Scientific | **added** | | Ctrl+Shift+& / Ctrl+Shift+_ | Cmd+Option+0 / Cmd+Option+- (and Cmd+Option+arrows for one side) | outline border / remove borders | **added** | | Ctrl+2 / 3 / 4 / 5 | — | bold / italic / underline / strikethrough | **added** | +| Ctrl+6 | Control+6 | hide / show objects (charts, pictures, shapes, slicers), saved as `workbookPr showObjects` | **added** | | Ctrl+Shift+: | Cmd+; | current time | **added** | | Ctrl+' / Ctrl+Shift+" | Control | formula / value from the cell above | **added** | | Ctrl+L | Cmd+L is Align Left, Cmd+E Center, Control+E Flash Fill | create table | **added** | @@ -124,7 +125,7 @@ otherwise. Number-format and border keys match the physical key (Ctrl+Shift+4, w | F5, Shift+F5 (Mac), F7, Shift+F9, Ctrl+Alt+F9, F12, Ctrl+F4 | same | Go To, Find, Spelling, calculate sheet / all, Save As, close | **added** | | Shift+F10, Alt+Shift+F10, Ctrl+F1 | Shift+F10, Cmd+Option+R | context menu, Error Checking, ribbon | **added** | | F1, F3, F4 (repeat), F6 / Shift+F6, F8 / Shift+F8, F11, End mode | same | help, Paste Name, Repeat, pane cycling, extend/add mode, chart sheet | missing (no such feature yet) | -| Ctrl+6, Ctrl+8, Ctrl+Q, Ctrl+Shift+A, Ctrl+A after a function name, Ctrl+Shift+U, Ctrl+Alt+P | same | objects, outline symbols, Quick Analysis, argument names, Function Arguments, formula bar height, tooltips | missing | +| Ctrl+8, Ctrl+Q, Ctrl+Shift+A, Ctrl+A after a function name, Ctrl+Shift+U, Ctrl+Alt+P | same | outline symbols, Quick Analysis, argument names, Function Arguments, formula bar height, tooltips | missing | | Ctrl+Shift+Page Up / Down, Ctrl+Alt+5, Ctrl+Shift+F1, Ctrl+F5–F10, Alt+F8 / F11 / F12 | same | group sheets, shape cycling, full screen, window keys, macros, VBA, Power Query | missing | | — | Cmd+= (calculate), Cmd+T (references while editing; F4 works), Shift+F7, Cmd+Shift+L | Mac-only extras | missing (Cmd+= is egui's zoom key; no thesaurus or Modify Cell Style) | @@ -161,6 +162,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-16 | minor | Ctrl+6 hides every object (charts, pictures, shapes, slicers) and shows them again; hidden objects are not drawn, take no clicks and drop their selection, and the setting is saved with the workbook (`workbookPr showObjects`) | | 2026-10-12 | minor | The File view's recent list tells when each workbook was last changed, read off the UI thread | | 2026-10-13 | minor | Horizontal scrollbar in the sheet tab bar: drag, track click pages, frozen columns excluded from the page (#840) | | 2026-10-12 | minor | Ctrl+wheel zoom steps along a ladder of percentages about 20% apart that never skips 100%, gliding to each step with animations on, so a notch out undoes a notch in (it drifted: 100 → 122 → 101); Excel steps 15 points, see decisions.md | From a09d3f6934d43b4da9f9d592d674c685c1508537 Mon Sep 17 00:00:00 2001 From: Bryan Mark Date: Fri, 16 Oct 2026 14:26:53 +0900 Subject: [PATCH 18/28] Memory: undo steps share picture and macro bytes instead of copying them Every edit copies the workbook for its undo step (copy-on-write: sheets and 64-row bands of cells are shared). A floating picture's bytes (Image.data) and the VBA project (Workbook.vba_project) were plain Vec, so each edit on a sheet with a picture, and every edit in a workbook with macros, kept another full copy of them, up to the 100 steps of the history. They are Arc> now, as cell pictures already are, with base64 helpers for serde (b64::shared, b64::opt). JSON and XLSX are unchanged. Measured on macOS 27 (Apple M3 Max), gridcraft-cli, kernel peak footprint: a 19 MB picture and 50 cell edits on its sheet, 992 -> 22 MB; 10 edits, 215 -> 21 MB. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +++- crates/engine/src/cmd/insert.rs | 2 +- crates/engine/src/cmd/print.rs | 2 +- crates/engine/src/io.rs | 2 +- crates/engine/src/tests.rs | 31 ++++++++++++++++++++++++++++- crates/model/src/b64.rs | 26 ++++++++++++++++++++---- crates/model/src/features.rs | 7 ++++--- crates/model/src/workbook.rs | 5 +++-- crates/ui-egui/tests/interaction.rs | 6 +++--- crates/xlsx/src/drawing.rs | 2 +- crates/xlsx/src/read.rs | 2 +- crates/xlsx/src/sheet_write.rs | 2 +- crates/xlsx/src/tests/fixtures.rs | 2 +- crates/xlsx/src/tests/roundtrip.rs | 6 +++--- crates/xlsx/src/write.rs | 2 +- 15 files changed, 76 insertions(+), 25 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..b9846d57f 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-16 · **Change:** minor (undo steps share picture and macro bytes) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-16 | Memory: undo steps share a sheet's picture bytes and the workbook's VBA project instead of copying them: 50 edits on a sheet with a 20 MB picture keep 22 MB instead of 992 MB | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-16 | minor | Progress log: undo steps share picture and macro bytes | | 2026-10-12 | minor | Rows auto-fit oversized fonts on open and whole-row/column formatting (#833); progress log row | | 2026-10-12 | minor | Progress log: multi-area row/column delete (#758) | | 2026-10-12 | trivial | Progress log: XLSX chart sheets kept on save (#727) | diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index d878e4469..38028b0e6 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -977,7 +977,7 @@ fn insert_picture(s: &mut Session, p: &Json) -> Result { // Excel inserts pictures as "Move but don't size with cells" (`editAs="oneCell"`). mode: anchor_mode_param(p).unwrap_or(gridcraft_model::AnchorMode::MoveOnly), }, - data, + data: data.into(), mime, alt: str_param(p, "alt").unwrap_or("").into(), }; diff --git a/crates/engine/src/cmd/print.rs b/crates/engine/src/cmd/print.rs index a10f19d46..0b8d22ade 100644 --- a/crates/engine/src/cmd/print.rs +++ b/crates/engine/src/cmd/print.rs @@ -2000,7 +2000,7 @@ mod tests { wb.sheet_mut(0).unwrap().images.push(Image { id, anchor: Anchor { cell: CellRef::new(5, 5), dx: 0.0, dy: 0.0, width: 40.0, height: 40.0, mode: gridcraft_model::AnchorMode::MoveAndSize }, - data: png, + data: png.into(), mime: "image/png".into(), alt: String::new(), }); diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 3e84036cb..067a15441 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -517,7 +517,7 @@ mod tests { #[test] fn macros_are_saved_only_into_xlsm() { let mut wb = Workbook::new(); - wb.vba_project = Some(b"vba".to_vec()); + wb.vba_project = Some(b"vba".to_vec().into()); let has_macros = |path: &str| { let bytes = save_bytes(&wb, path).unwrap(); gridcraft_xlsx::read_xlsx(&bytes).unwrap().0.vba_project.is_some() diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..7b335c474 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2549,7 +2549,7 @@ fn add_image(s: &mut Session, cell: &str, mode: gridcraft_model::AnchorMode) -> wb.sheet_mut(0).unwrap().images.push(gridcraft_model::Image { id, anchor: gridcraft_model::Anchor { cell: CellRef::parse(cell).unwrap(), dx: 0.0, dy: 0.0, width: 40.0, height: 40.0, mode }, - data: tiny_png(), + data: tiny_png().into(), mime: "image/png".into(), alt: String::new(), }); @@ -2560,6 +2560,35 @@ fn image_cell(s: &Session, id: u32) -> Option { s.doc().unwrap().wb.active().unwrap().images.iter().find(|i| i.id == id).map(|i| i.anchor.cell) } +/// Every undo step holds a copy of the workbook. A picture's bytes and the VBA project are shared +/// between those copies instead of copied into each: a 20 MB picture used to cost another 20 MB +/// for every cell typed on its sheet, up to 100 steps (2 GB). +#[test] +fn undo_steps_share_picture_and_macro_bytes() { + let mut s = s(); + s.execute("insert.picture", json!({"base64": crate::io::base64_encode(&tiny_png()), "at": "C3"})).unwrap(); + std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).vba_project = Some(b"vba".to_vec().into()); + for i in 1..=3 { + s.execute("cell.set", json!({"cell": format!("A{i}"), "input": i.to_string()})).unwrap(); + } + let d = s.doc().unwrap(); + let live = &d.wb.active().unwrap().images[0].data; + let vba = d.wb.vba_project.as_ref().unwrap(); + assert_eq!(**live, tiny_png()); + assert!(d.undo.len() >= 3); + for step in d.undo.iter().filter(|h| !h.wb.active().unwrap().images.is_empty()) { + assert!(std::sync::Arc::ptr_eq(&step.wb.active().unwrap().images[0].data, live), "{}", step.label); + } + for step in d.undo.iter().rev().take(3) { + assert!(std::sync::Arc::ptr_eq(step.wb.vba_project.as_ref().unwrap(), vba), "{}", step.label); + } + // Undo and redo still give the picture back as it was. + s.execute("edit.undo", json!({"steps": 3})).unwrap(); + assert_eq!(*s.doc().unwrap().wb.active().unwrap().images[0].data, tiny_png()); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(*s.doc().unwrap().wb.active().unwrap().images[0].data, tiny_png()); +} + /// Issue #5: images anchored in cells follow their row when the sheet is sorted. #[test] fn sort_moves_anchored_images_with_their_rows() { diff --git a/crates/model/src/b64.rs b/crates/model/src/b64.rs index 30db4b43d..b377fe451 100644 --- a/crates/model/src/b64.rs +++ b/crates/model/src/b64.rs @@ -49,22 +49,40 @@ pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result, D::Error> decode(&s).ok_or_else(|| serde::de::Error::custom("invalid base64")) } -/// Base64 for optional byte blobs (VBA projects): `null`/missing stays `None`. +/// Base64 for byte blobs shared through `Arc` (floating pictures), so copies of a workbook, such as +/// its undo steps, share one payload. +pub mod shared { + use std::sync::Arc; + + use serde::{Deserializer, Serializer}; + + pub fn serialize(v: &Arc>, s: S) -> Result { + super::serialize(v, s) + } + + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result>, D::Error> { + super::deserialize(d).map(Arc::new) + } +} + +/// Base64 for optional shared byte blobs (VBA projects): `null`/missing stays `None`. pub mod opt { + use std::sync::Arc; + use serde::{Deserialize, Deserializer, Serializer}; - pub fn serialize(v: &Option>, s: S) -> Result { + pub fn serialize(v: &Option>>, s: S) -> Result { match v { Some(data) => s.serialize_str(&super::encode(data)), None => s.serialize_none(), } } - pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result>, D::Error> { + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result>>, D::Error> { let s = Option::::deserialize(d)?; match s { None => Ok(None), - Some(s) => super::decode(&s).map(Some).ok_or_else(|| serde::de::Error::custom("invalid base64")), + Some(s) => super::decode(&s).map(|v| Some(Arc::new(v))).ok_or_else(|| serde::de::Error::custom("invalid base64")), } } } diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index b51f7bc98..2dafb1c5c 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -508,9 +508,10 @@ pub struct Chart { pub struct Image { pub id: u32, pub anchor: Anchor, - /// Encoded bytes (PNG/JPEG), base64 in JSON. - #[serde(with = "crate::b64")] - pub data: Vec, + /// Encoded bytes (PNG/JPEG), base64 in JSON. Shared, so a copy of the sheet (an undo step) + /// doesn't copy the picture. + #[serde(with = "crate::b64::shared")] + pub data: std::sync::Arc>, pub mime: String, pub alt: String, } diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 828ca692e..fd300b9fa 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -106,9 +106,10 @@ pub struct Workbook { pub cell_styles: Vec<(String, crate::style::Style)>, /// VBA project (`xl/vbaProject.bin`) carried through unchanged. GridCraft does not run /// macros; the blob is preserved so that saving a macro-enabled workbook never destroys the - /// macros it arrived with. base64 in JSON. `None` = the workbook has no macros. + /// macros it arrived with. base64 in JSON. `None` = the workbook has no macros. Shared, so a + /// copy of the workbook (every edit's undo step) doesn't copy it. #[serde(default, with = "crate::b64::opt")] - pub vba_project: Option>, + pub vba_project: Option>>, /// Field separator of the CSV this workbook was read from, so saving it as CSV writes the same /// separator back (a `;` file stays a `;` file). `None` = not read from CSV: CSV saves use `,`. #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index b40769cf8..0af83753b 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -750,7 +750,7 @@ fn column_autocomplete_can_be_turned_off() { /// An .xlsm carrying a VBA project, saved to bytes. fn macro_workbook_bytes() -> Vec { let mut wb = gridcraft_engine::model::Workbook::new(); - wb.vba_project = Some(b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec()); + wb.vba_project = Some(b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec().into()); gridcraft_engine::io::save_bytes(&wb, "macros.xlsm").unwrap() } @@ -798,7 +798,7 @@ fn plain_open_warning_becomes_an_info_notice() { fn macros_dialog_lists_vba_and_scripts() { let mut s = blank(); let mut wb = gridcraft_engine::model::Workbook::new(); - wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]); + wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1].into()); s.active_mut().unwrap().wb = std::sync::Arc::new(wb); let mut h = harness(s); h.state_mut().open_dialog("macros", json!({})); @@ -814,7 +814,7 @@ fn macros_dialog_lists_vba_and_scripts() { fn status_bar_shows_macro_badge() { let mut s = blank(); let mut wb = gridcraft_engine::model::Workbook::new(); - wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]); + wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1].into()); s.active_mut().unwrap().wb = std::sync::Arc::new(wb); // Rendering the status bar must not panic with a VBA project present. let mut h = harness(s); diff --git a/crates/xlsx/src/drawing.rs b/crates/xlsx/src/drawing.rs index 6dbd17e7e..8cd63873a 100644 --- a/crates/xlsx/src/drawing.rs +++ b/crates/xlsx/src/drawing.rs @@ -156,7 +156,7 @@ pub fn read_drawing(cx: &mut Ctx<'_>, part: &str, sheet: &mut Sheet) -> Result<( }; let alt = obj.path(&["nvPicPr", "cNvPr"]).and_then(|c| c.attr("descr")).unwrap_or("").to_string(); let id = cx.next_object_id(); - sheet.images.push(Image { id, anchor, data, mime: mime_for(&target).to_string(), alt }); + sheet.images.push(Image { id, anchor, data: data.into(), mime: mime_for(&target).to_string(), alt }); } "graphicFrame" => { let Some(gd) = obj.path(&["graphic", "graphicData"]) else { continue }; diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 1a479081c..160e838d6 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -331,7 +331,7 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { if let Some(r) = find_rel(&wb_rels, "vbaProject").cloned() { match cx.pkg.read(&r.target) { Ok(Some(data)) => { - wb.vba_project = Some(data); + wb.vba_project = Some(data.into()); cx.warn("macros (VBA project) were kept but not executed"); } Ok(None) => cx.warn("macros (VBA project) reference a missing part and were dropped"), diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index 78e1afaae..b5fcaa052 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -502,7 +502,7 @@ pub fn write_sheet(wb: &Workbook, si: usize, selected: bool, out: &mut Out) -> ( let ext = image_ext(&img.mime, &img.data); let name = format!("xl/media/image{}.{ext}", out.images); out.default_type(ext, image_content_type(ext)); - out.part(&name, None, img.data.clone()); + out.part(&name, None, img.data.to_vec()); let rid = drels.add("image", &format!("../media/image{}.{ext}", out.images)); objs.push(Obj::Image(img, rid)); } diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index 39ea30951..4ab178159 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -381,7 +381,7 @@ fn drawing_with_picture_and_chart() { let (wb, _) = read_xlsx(&bytes).unwrap(); let s = wb.sheet(0).unwrap(); let img = &s.images[0]; - assert_eq!(img.data, png); + assert_eq!(*img.data, png); assert_eq!(img.mime, "image/png"); assert_eq!(img.alt, "logo"); assert_eq!(img.anchor.cell, at("B3")); diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 901802905..5cd835878 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -326,7 +326,7 @@ fn sample() -> Workbook { id: 10, // A non-default anchor mode, to prove it survives an xlsx round-trip. anchor: Anchor { cell: at("C10"), dx: 5.0, dy: 4.0, width: 120.0, height: 80.0, mode: AnchorMode::MoveOnly }, - data: png, + data: png.into(), mime: "image/png".into(), alt: "Logo \"x\"".into(), }); @@ -737,7 +737,7 @@ fn vba_project_roundtrip() { // carries the macro-enabled content type plus the vbaProject relationship. let vba = b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec(); let mut wb = Workbook::new(); - wb.vba_project = Some(vba.clone()); + wb.vba_project = Some(vba.clone().into()); let bytes = write_xlsx(&wb).unwrap(); assert_eq!(write_xlsx(&wb).unwrap(), bytes, "output is deterministic"); @@ -756,7 +756,7 @@ fn vba_project_roundtrip() { assert!(rels.contains("relationships/vbaProject"), "vbaProject relationship: {rels}"); let (back, rep) = read_xlsx(&bytes).unwrap(); - assert_eq!(back.vba_project.as_deref(), Some(vba.as_slice()), "vbaProject survives the read"); + assert_eq!(back.vba_project.as_deref(), Some(&vba), "vbaProject survives the read"); assert!(rep.warnings.iter().any(|w| w.contains("not executed")), "{:?}", rep.warnings); // A workbook without macros stays macro-free and free of VBA warnings. diff --git a/crates/xlsx/src/write.rs b/crates/xlsx/src/write.rs index 06761f01d..6920bec55 100644 --- a/crates/xlsx/src/write.rs +++ b/crates/xlsx/src/write.rs @@ -404,7 +404,7 @@ pub fn write_xlsx(wb: &Workbook, kind: XlsxKind) -> Result, IoError> { // A macro-free package can't hold a VBA project (Excel refuses such a file). if let Some(data) = wb.vba_project.as_ref().filter(|_| kind.is_macro_enabled()) { wb_rels.add("vbaProject", "vbaProject.bin"); - out.part("xl/vbaProject.bin", Some(CT_VBA), data.clone()); + out.part("xl/vbaProject.bin", Some(CT_VBA), data.to_vec()); } out.part("xl/_rels/workbook.xml.rels", None, wb_rels.xml().into_bytes()); From 3684349d9ccd00cdac98ddc09d6602e81daea68f Mon Sep 17 00:00:00 2001 From: Bryan Mark Date: Fri, 16 Oct 2026 14:26:53 +0900 Subject: [PATCH 19/28] Memory: floating pictures' textures are bounded, sized to what is drawn, and dropped with their workbook Floating pictures were decoded at full size, with no limits, into one process-wide map of textures that never let go, not even when their workbook closed. Its key was the picture's id and byte length, and ids are per workbook, so a second open workbook could show the first one's picture. The new floating_pictures module, built like cell_pictures, keeps a cache per egui context keyed by workbook, picture id and size: - bounded at 128 MB, least recently drawn first, never evicting a picture drawn in the last frame, so none turns into a placeholder; - a workbook's textures go when it closes (checked every frame); - a picture's header is read first (16,384 px a side, 64 MP at most), it is decoded with limits, and to the next size step of what is drawn (a power of two or three quarters of one, up to 4,096 px or the GPU's limit); it is decoded again only when drawn larger; - a picture that can't be decoded is remembered, not retried each frame. Measured on macOS 27 (Apple M3 Max): 12 pictures of 2600 x 2600 opened from an .xlsx and scrolled through, textures held by egui 258 -> 40 MB, settled footprint 738 -> 509 MB; after closing the workbook, textures 258 -> 0 MB, footprint 504 -> 236 MB. 24 pictures: textures 490 -> 76 MB. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +- crates/ui-egui/src/chartview.rs | 20 +- crates/ui-egui/src/floating_pictures.rs | 274 ++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 4 + crates/ui-egui/tests/floating_pictures.rs | 68 ++++++ 5 files changed, 352 insertions(+), 18 deletions(-) create mode 100644 crates/ui-egui/src/floating_pictures.rs create mode 100644 crates/ui-egui/tests/floating_pictures.rs diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..dd6e1cc4f 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-16 · **Change:** minor (floating pictures' textures bounded and kept per workbook) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-16 | Memory: floating pictures are decoded at about the size they are drawn, with limits, into a cache bounded by bytes and dropped with their workbook, instead of at full size into one that never let go; two open workbooks no longer show each other's picture. 12 large pictures scrolled through keep 40 MB of textures instead of 258 MB, and none once the workbook closes | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-16 | minor | Progress log: floating pictures' textures bounded and kept per workbook | | 2026-10-12 | minor | Rows auto-fit oversized fonts on open and whole-row/column formatting (#833); progress log row | | 2026-10-12 | minor | Progress log: multi-area row/column delete (#758) | | 2026-10-12 | trivial | Progress log: XLSX chart sheets kept on save (#727) | diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 180f15c21..6f2f62a95 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -1,9 +1,6 @@ //! Floating objects on the sheet: charts (rendered by `gridcraft-chart` into primitives that //! we paint with egui), pictures and shapes. -use std::collections::HashMap; -use std::sync::Mutex; - use egui::epaint::{PathShape, PathStroke}; use egui::{Align2, Color32, FontId, Painter, Pos2, Rect, Stroke, StrokeKind, pos2, vec2}; use gridcraft_chart::{HAlign, Prim, VAlign}; @@ -249,8 +246,6 @@ pub(crate) fn anchor_rect(geo: &Geo, sh: &Sheet, a: &Anchor) -> Rect { Rect::from_min_size(pos2(x, y), vec2(a.width * geo.z, a.height.max(1.0) * geo.z)) } -static TEXTURES: std::sync::LazyLock>> = std::sync::LazyLock::new(|| Mutex::new(HashMap::new())); - /// The shapes one floating object painted (indices into the painter's layer) and its logical /// rectangle, so a right-to-left sheet can move it as a whole instead of mirroring it. pub(crate) struct PaintedObject { @@ -261,6 +256,8 @@ pub(crate) struct PaintedObject { pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: usize, sh: &Sheet) -> Vec { let mut painted = Vec::new(); let mut done = |start: egui::layers::ShapeIdx, rect: Rect| painted.push(PaintedObject { shapes: start.0..p.add(egui::Shape::Noop).0, rect }); + // The grid paints the active workbook: its pictures are cached under it. + let doc = app.session.active().map_or(0, |d| d.uid); for sp in &sh.shapes { let r = anchor_rect(geo, sh, &sp.anchor); if !r.intersects(p.clip_rect()) { @@ -332,18 +329,7 @@ pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workboo continue; } let start = p.add(egui::Shape::Noop); - let key = (im.id, im.data.len()); - let mut cache = TEXTURES.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - if !cache.contains_key(&key) - && let Ok(img) = image::load_from_memory(&im.data) - { - let rgba = img.to_rgba8(); - let size = [rgba.width() as usize, rgba.height() as usize]; - let ci = egui::ColorImage::from_rgba_unmultiplied(size, rgba.as_raw()); - let tex = p.ctx().load_texture(format!("img{}", im.id), ci, egui::TextureOptions::LINEAR); - cache.insert(key, tex); - } - if let Some(tex) = cache.get(&key) { + if let Some(tex) = crate::floating_pictures::texture(p.ctx(), doc, im, r) { p.image(tex.id(), r, Rect::from_min_max(pos2(0.0, 0.0), pos2(1.0, 1.0)), Color32::WHITE); } else { p.rect_filled(r, 0.0, Color32::from_gray(230)); diff --git a/crates/ui-egui/src/floating_pictures.rs b/crates/ui-egui/src/floating_pictures.rs new file mode 100644 index 000000000..68de6e203 --- /dev/null +++ b/crates/ui-egui/src/floating_pictures.rs @@ -0,0 +1,274 @@ +//! Floating pictures (Insert › Pictures) as textures: decoded once at about the size they are +//! drawn, and kept in a cache per egui context that is bounded by bytes. The cache used to be +//! process-wide, with no limit, decoding every picture at full size and keeping it until the app +//! closed, after its workbook was closed too. + +use std::collections::HashMap; +use std::io::Cursor; +use std::sync::{Arc, Mutex}; + +use egui::TextureHandle; +use gridcraft_engine::model::Image; + +/// Larger pictures are not decoded: the header is read first, so nothing is allocated for them. +const MAX_SIDE: u32 = 16_384; +const MAX_PIXELS: u64 = 64_000_000; +const MAX_DECODE_BYTES: u64 = 512 * 1024 * 1024; +/// A texture is never wider or taller than this (nor than the GPU allows). +const MAX_TEXTURE_SIDE: u32 = 4096; +/// The smallest texture side, so a picture drawn small and zoomed in a little isn't decoded again. +const MIN_TEXTURE_SIDE: u32 = 64; +/// Pictures drawn in the last frame stay whatever this says, so none turns into a placeholder; +/// the others are dropped, least recently drawn first, to keep the cache under it. +const MAX_CACHE_BYTES: usize = 128 * 1024 * 1024; +const MAX_CACHE_ITEMS: usize = 4096; + +/// A picture: its workbook, its object id there, and its size in bytes (object ids are per +/// workbook, so two open workbooks can both have a picture 1). +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] +struct Key { + doc: u64, + id: u32, + len: usize, +} + +#[derive(Default)] +struct Cache { + entries: HashMap, + bytes: usize, + /// The open workbooks when the cache was last pruned. + docs: Vec, +} + +struct Entry { + /// `None` when the picture can't be decoded: kept, so it isn't tried again every frame. + texture: Option, + bytes: usize, + /// The longest side it was decoded for; `u32::MAX` when it holds all there is (the source + /// size or the largest texture allowed), or when it can't be decoded. + side: u32, + used: u64, +} + +impl Cache { + /// Drops the pictures of workbooks that are no longer open. + fn forget_closed(&mut self, open: &[u64]) { + if self.docs == open { + return; + } + self.entries.retain(|key, entry| { + let keep = open.contains(&key.doc); + if !keep { + self.bytes = self.bytes.saturating_sub(entry.bytes); + } + keep + }); + self.docs = open.to_vec(); + } + + /// The texture for picture `key` drawn with its longer side `want` pixels long. + fn texture(&mut self, ctx: &egui::Context, key: Key, data: &[u8], want: u32) -> Option { + let frame = ctx.cumulative_frame_nr(); + let side = texture_side(want, ctx.input(|i| i.max_texture_side)); + if let Some(entry) = self.entries.get_mut(&key) { + entry.used = frame; + if entry.side >= side { + return entry.texture.clone(); + } + } + // Not cached, or drawn larger than it was decoded for: decode it again at the new size. + if let Some(old) = self.entries.remove(&key) { + self.bytes = self.bytes.saturating_sub(old.bytes); + } + let decoded = decode(data, side); + let bytes = decoded.as_ref().map_or(0, |(image, _)| image.pixels.len().saturating_mul(4)); + while self.entries.len() >= MAX_CACHE_ITEMS || self.bytes.saturating_add(bytes) > MAX_CACHE_BYTES { + let oldest = self.entries.iter().filter(|(_, e)| e.used.saturating_add(1) < frame).min_by_key(|(_, e)| e.used).map(|(key, _)| *key); + let Some(oldest) = oldest else { break }; + if let Some(entry) = self.entries.remove(&oldest) { + self.bytes = self.bytes.saturating_sub(entry.bytes); + } + } + let (texture, side) = match decoded { + Some((image, complete)) => { + (Some(ctx.load_texture(format!("img{}", key.id), image, egui::TextureOptions::LINEAR)), if complete { u32::MAX } else { side }) + } + None => (None, u32::MAX), + }; + self.bytes = self.bytes.saturating_add(bytes); + self.entries.insert(key, Entry { texture: texture.clone(), bytes, side, used: frame }); + texture + } +} + +/// The texture side for a picture drawn `want` pixels long: the next power of two or three +/// quarters of one (…, 768, 1024, 1536, 2048), so zooming in decodes it again only at every +/// half or third larger, within the smallest and largest sides allowed. +fn texture_side(want: u32, gpu_max: usize) -> u32 { + let max = MAX_TEXTURE_SIDE.min(u32::try_from(gpu_max).unwrap_or(u32::MAX)).max(1); + let want = want.max(MIN_TEXTURE_SIDE); + let Some(p) = want.checked_next_power_of_two() else { return max }; + let side = if want <= p / 4 * 3 { p / 4 * 3 } else { p }; + side.min(max) +} + +/// Decodes a picture to fit a `side` × `side` square, and says whether that is the whole picture +/// (it was no larger, or `side` is as large as a texture may be). +fn decode(data: &[u8], side: u32) -> Option<(egui::ColorImage, bool)> { + let format = image::guess_format(data).ok()?; + let (w, h) = image::ImageReader::with_format(Cursor::new(data), format).into_dimensions().ok()?; + if w == 0 || h == 0 || w > MAX_SIDE || h > MAX_SIDE || u64::from(w) * u64::from(h) > MAX_PIXELS { + return None; + } + let mut reader = image::ImageReader::with_format(Cursor::new(data), format); + let mut limits = image::Limits::default(); + limits.max_image_width = Some(MAX_SIDE); + limits.max_image_height = Some(MAX_SIDE); + limits.max_alloc = Some(MAX_DECODE_BYTES); + reader.limits(limits); + let mut picture = reader.decode().ok()?; + let complete = w.max(h) <= side || side >= MAX_TEXTURE_SIDE; + if w.max(h) > side { + picture = picture.thumbnail(side, side); + } + let rgba = picture.into_rgba8(); + Some((egui::ColorImage::from_rgba_unmultiplied([rgba.width() as usize, rgba.height() as usize], rgba.as_raw()), complete)) +} + +fn cache(ctx: &egui::Context) -> Arc> { + // Context-local, like cell pictures: texture handles never leak between offscreen, native + // and browser contexts. + ctx.data_mut(|data| data.get_temp_mut_or_default::>>(egui::Id::new("gridcraft.floating_pictures")).clone()) +} + +/// Drops the textures of workbooks that are no longer open. Called every frame the grid paints. +pub(crate) fn forget_closed(ctx: &egui::Context, open: &[u64]) { + cache(ctx).lock().unwrap_or_else(std::sync::PoisonError::into_inner).forget_closed(open); +} + +/// The texture of picture `image` of workbook `doc`, drawn in `rect` (points); `None` when it +/// can't be decoded or is too large to be. +pub(crate) fn texture(ctx: &egui::Context, doc: u64, image: &Image, rect: egui::Rect) -> Option { + let px = (rect.width().max(rect.height()) * ctx.pixels_per_point()).ceil(); + let want = if px.is_finite() && px > 0.0 { px.min(u32::MAX as f32) as u32 } else { 1 }; + let key = Key { doc, id: image.id, len: image.data.len() }; + cache(ctx).lock().unwrap_or_else(std::sync::PoisonError::into_inner).texture(ctx, key, &image.data, want) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn png(w: u32, h: u32, color: [u8; 4]) -> Vec { + let image = image::RgbaImage::from_pixel(w, h, image::Rgba(color)); + let mut bytes = Cursor::new(Vec::new()); + image.write_to(&mut bytes, image::ImageFormat::Png).unwrap(); + bytes.into_inner() + } + + fn key(doc: u64, id: u32, data: &[u8]) -> Key { + Key { doc, id, len: data.len() } + } + + /// Runs one frame, so the frame number moves on as it does when the app paints. + fn frame(ctx: &egui::Context) { + let mut out = ctx.run_ui(Default::default(), |_| {}); + out.textures_delta.clear(); // nothing consumes the uploads in a headless test + } + + #[test] + fn pictures_are_decoded_at_about_the_size_drawn() { + let ctx = egui::Context::default(); + let mut cache = Cache::default(); + let data = png(3000, 1000, [10, 120, 200, 255]); + let k = key(1, 1, &data); + let small = cache.texture(&ctx, k, &data, 300).unwrap(); + assert_eq!(small.size(), [384, 128], "drawn 300 px long: decoded for 384"); + assert_eq!(cache.texture(&ctx, k, &data, 380).unwrap().id(), small.id(), "still within its size: not decoded again"); + let larger = cache.texture(&ctx, k, &data, 900).unwrap(); + assert_eq!(larger.size(), [1024, 341], "drawn larger: decoded again for 1024"); + assert_eq!(cache.entries.len(), 1); + assert_eq!(cache.bytes, 1024 * 341 * 4, "the smaller texture's bytes are given back"); + assert_eq!(texture_side(1280, 8192), 1536); + assert_eq!(texture_side(1600, 8192), 2048); + assert_eq!(texture_side(1, 8192), MIN_TEXTURE_SIDE); + assert_eq!(texture_side(u32::MAX, 8192), MAX_TEXTURE_SIDE); + // Never past the GPU's limit (2048 in a default context) nor the source. + assert_eq!(cache.texture(&ctx, k, &data, 50_000).unwrap().size(), [2048, 683]); + let tiny = png(40, 20, [0, 0, 0, 255]); + assert_eq!(cache.texture(&ctx, key(1, 2, &tiny), &tiny, 4000).unwrap().size(), [40, 20]); + assert_eq!(cache.entries.get(&key(1, 2, &tiny)).map(|e| e.side), Some(u32::MAX), "the whole picture: never decoded again"); + } + + #[test] + fn the_cache_stays_within_its_budget_but_keeps_what_is_on_screen() { + let ctx = egui::Context::default(); + let mut cache = Cache::default(); + // 40 pictures of 2048 × 2048 drawn in turn, one frame each: 640 MB decoded in all. + let data: Vec> = (0..40u8).map(|i| png(2048, 2048, [i, 0, 0, 255])).collect(); + for (i, d) in data.iter().enumerate() { + assert!(cache.texture(&ctx, key(1, i as u32, d), d, 2048).is_some()); + frame(&ctx); + } + assert!(cache.bytes <= MAX_CACHE_BYTES, "{} bytes kept", cache.bytes); + assert!(cache.entries.len() < data.len()); + // Many on screen at once, over the budget: every one keeps its texture, frame after frame. + let shown: Vec<_> = data.iter().enumerate().take(12).map(|(i, d)| cache.texture(&ctx, key(1, i as u32, d), d, 2048).unwrap().id()).collect(); + for _ in 0..3 { + frame(&ctx); + for (i, d) in data.iter().enumerate().take(12) { + assert_eq!(cache.texture(&ctx, key(1, i as u32, d), d, 2048).map(|t| t.id()), Some(shown[i])); + } + } + } + + #[test] + fn a_closed_workbook_takes_its_pictures_with_it() { + let ctx = egui::Context::default(); + let mut cache = Cache::default(); + let data = png(64, 64, [0, 200, 0, 255]); + let first = cache.texture(&ctx, key(1, 1, &data), &data, 64).unwrap(); + // Another workbook's picture 1, of the same size: a texture of its own. + let other = png(64, 64, [200, 0, 0, 255]); + assert_eq!(other.len(), data.len()); + assert_ne!(cache.texture(&ctx, key(2, 1, &other), &other, 64).unwrap().id(), first.id()); + cache.forget_closed(&[1, 2]); + assert_eq!(cache.entries.len(), 2); + cache.forget_closed(&[2]); + assert_eq!(cache.entries.len(), 1); + assert_eq!(cache.bytes, 64 * 64 * 4); + cache.forget_closed(&[]); + assert!(cache.entries.is_empty()); + assert_eq!(cache.bytes, 0); + } + + #[test] + fn broken_and_oversized_pictures_are_refused_once() { + let ctx = egui::Context::default(); + let mut cache = Cache::default(); + let garbage = vec![0xAB; 100]; + assert!(cache.texture(&ctx, key(1, 1, &garbage), &garbage, 100).is_none()); + assert_eq!(cache.entries.get(&key(1, 1, &garbage)).map(|e| (e.side, e.bytes)), Some((u32::MAX, 0)), "remembered, not retried"); + // A PNG whose header says 20,000 × 20,000 is refused from its header: nothing is decoded. + let mut huge = png(1, 1, [0, 0, 0, 255]); + let ihdr = huge.windows(4).position(|w| w == b"IHDR").unwrap(); + huge[ihdr + 4..ihdr + 8].copy_from_slice(&20_000u32.to_be_bytes()); + huge[ihdr + 8..ihdr + 12].copy_from_slice(&20_000u32.to_be_bytes()); + let crc = crc32(&huge[ihdr..ihdr + 17]); + huge[ihdr + 17..ihdr + 21].copy_from_slice(&crc.to_be_bytes()); + assert_eq!(image::ImageReader::with_format(Cursor::new(&huge), image::ImageFormat::Png).into_dimensions().unwrap(), (20_000, 20_000)); + assert!(decode(&huge, 64).is_none()); + assert!(decode(&png(4, 2, [255, 0, 0, 255]), 64).is_some_and(|(i, complete)| i.size == [4, 2] && complete)); + } + + fn crc32(bytes: &[u8]) -> u32 { + let mut crc = !0u32; + for &b in bytes { + crc ^= u32::from(b); + for _ in 0..8 { + crc = if crc & 1 == 1 { (crc >> 1) ^ 0xEDB8_8320 } else { crc >> 1 }; + } + } + !crc + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f96cdac3c..00519680d 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -14,6 +14,7 @@ pub mod control; pub mod credits; pub mod dialogs; pub mod editor; +mod floating_pictures; pub mod fonts; pub mod formula_bar; mod formula_locale; @@ -993,6 +994,9 @@ impl SheetApp { // A keytip control request lives one frame: the ribbon consumes it during `ui`, and // anything that never reached a control (collapsed ribbon, tab switch) is dropped. self.ui.keytip_invoke = None; + // Floating pictures' textures go with their workbook, even when no sheet is painted. + let open: Vec = self.session.documents().iter().map(|d| d.uid).collect(); + floating_pictures::forget_closed(ctx, &open); if self.fonts_han.is_some_and(|han| han != theme::HanOrder::of(self.ui.language)) { self.fonts_ready = false; self.fonts_set = false; diff --git a/crates/ui-egui/tests/floating_pictures.rs b/crates/ui-egui/tests/floating_pictures.rs new file mode 100644 index 000000000..f14d40d85 --- /dev/null +++ b/crates/ui-egui/tests/floating_pictures.rs @@ -0,0 +1,68 @@ +//! Floating pictures on the sheet, with original synthetic pictures. + +use std::io::Cursor; + +use egui::{Shape, TextureId, vec2}; +use gridcraft_engine::Session; +use gridcraft_ui_egui::SheetApp; +use serde_json::json; + +fn png(color: [u8; 4]) -> Vec { + let image = image::RgbaImage::from_pixel(120, 60, image::Rgba(color)); + let mut bytes = Cursor::new(Vec::new()); + image.write_to(&mut bytes, image::ImageFormat::Png).unwrap(); + bytes.into_inner() +} + +fn harness() -> egui_kittest::Harness<'static, SheetApp> { + let mut session = Session::new(); + session.new_workbook(); + let mut h = egui_kittest::Harness::builder().with_size(vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(&ui.ctx().clone()); + app.ui(ui); + }, + SheetApp::new(session, Default::default()), + ); + h.run_steps(4); + h +} + +fn picture_textures(h: &egui_kittest::Harness<'static, SheetApp>) -> Vec { + h.output() + .shapes + .iter() + .filter_map(|shape| match &shape.shape { + Shape::Mesh(mesh) if mesh.texture_id != TextureId::default() => Some(mesh.texture_id), + _ => None, + }) + .collect() +} + +/// Picture ids are per workbook, so two open workbooks both have a picture 1. Each must show its +/// own picture, even when the two are the same number of bytes: the texture cache used to tell +/// pictures apart by id and size alone, for the whole app. +#[test] +fn each_workbook_shows_its_own_picture() { + let (green, red) = (png([24, 150, 110, 255]), png([200, 40, 40, 255])); + assert_eq!(green.len(), red.len()); + let mut h = harness(); + h.state_mut().run("insert.picture", json!({"base64": gridcraft_engine::io::base64_encode(&green), "at": "B2"})).unwrap(); + h.run_steps(4); + let first = picture_textures(&h); + assert_eq!(first.len(), 1, "the picture is painted"); + + h.state_mut().run("file.new", json!({})).unwrap(); + h.state_mut().run("insert.picture", json!({"base64": gridcraft_engine::io::base64_encode(&red), "at": "B2"})).unwrap(); + h.run_steps(4); + let doc = h.state().session.active().unwrap(); + assert_eq!(doc.wb.active().unwrap().images[0].id, h.state().session.documents()[0].wb.active().unwrap().images[0].id); + let second = picture_textures(&h); + assert_eq!(second.len(), 1); + assert_ne!(second, first, "another workbook's picture 1 has a texture of its own"); + + // Closing the workbook and painting again leaves the first one's picture as it was. + h.state_mut().run("file.close", json!({"force": true})).unwrap(); + h.run_steps(4); + assert_eq!(picture_textures(&h), first); +} From 94af08ad200b5c034cd3508d6446eb6a2ddba686 Mon Sep 17 00:00:00 2001 From: aviiciii Date: Sat, 17 Oct 2026 01:23:26 +0530 Subject: [PATCH 20/28] XLSX: new workbooks get (fixes #1699) A workbook GridCraft created had no in workbook.xml; it now names GridCraft, and keeps it on later saves. A workbook read from an Excel file still keeps its as it was, appName="xl" included. Observed in Excel 16.114: with the calcFeatures extension present, Excel uses the saved cell values only when appName is "xl". appName="gridcraft", "Calc" or no appName makes it recalculate everything on open, which brings back #799 (lookups into closed workbooks open as #CALC!). The application that last saved the file is recorded in docProps/app.xml GridCraft, which GridCraft already writes on every save and which Excel's recalculation ignores. --- crates/xlsx/src/tests/passthrough_tests.rs | 15 ++++++++++++--- crates/xlsx/src/write.rs | 5 ++++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/crates/xlsx/src/tests/passthrough_tests.rs b/crates/xlsx/src/tests/passthrough_tests.rs index d844daa05..f3bf89919 100644 --- a/crates/xlsx/src/tests/passthrough_tests.rs +++ b/crates/xlsx/src/tests/passthrough_tests.rs @@ -819,7 +819,16 @@ fn file_version_and_calc_features_survive_a_save() { assert!(text.contains(&format!("{calc_features}")), "{text}"); } - // A workbook that had neither gets neither. - let plain = Unpacked::new(&write_xlsx(&gridcraft_model::Workbook::new()).unwrap()).text("xl/workbook.xml"); - assert!(!plain.contains("fileVersion") && !plain.contains("extLst"), "{plain}"); + // A workbook that had neither gets a naming GridCraft (#1699), kept on the next + // save, and no calcFeatures: Excel recalculates it on open. + let new = write_xlsx(&gridcraft_model::Workbook::new()).unwrap(); + let (reread, _) = read_xlsx(&new).unwrap(); + for saved in [new, write_xlsx(&reread).unwrap()] { + let u = Unpacked::new(&saved); + u.assert_valid(); + let plain = u.text("xl/workbook.xml"); + let root = xml::parse(plain.as_bytes()).unwrap(); + assert_eq!(root.children.first().map(|e| e.name.as_str()), Some("fileVersion"), "{plain}"); + assert!(plain.contains(r#""#) && !plain.contains("extLst"), "{plain}"); + } } diff --git a/crates/xlsx/src/write.rs b/crates/xlsx/src/write.rs index 06761f01d..a4c5d1d70 100644 --- a/crates/xlsx/src/write.rs +++ b/crates/xlsx/src/write.rs @@ -314,7 +314,10 @@ pub fn write_xlsx(wb: &Workbook, kind: XlsxKind) -> Result, IoError> { } let mut w = format!("{XML_DECL}"); - w.push_str(&crate::passthrough::workbook_elements(wb, "fileVersion")); + // A workbook read from a file keeps its as it was: Excel trusts the saved values + // only when appName is "xl" (#799). One without names GridCraft (#1699), and Excel recalculates it. + let file_version = crate::passthrough::workbook_elements(wb, "fileVersion"); + w.push_str(if file_version.is_empty() { "" } else { &file_version }); w.push_str(if wb.date_system == gridcraft_core::DateSystem::D1904 { "" } else { "" }); if wb.protected_structure { let pw = wb.structure_password.as_ref().map(|h| crate::sheet_write::password_attrs(h, "workbook")).unwrap_or_default(); From aff9153dac69a9ab292ad47ba4089e54a3c1dd04 Mon Sep 17 00:00:00 2001 From: Hamza Date: Fri, 16 Oct 2026 01:09:12 +0500 Subject: [PATCH 21/28] Calc: CELL("filename") names the workbook's file (#1489) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It returned an empty string for every workbook, saved or not, so the classic sheet-name formula `MID(CELL("filename",A1),FIND("]",…)+1,31)` gave #VALUE!: with no `]` in the string there is nothing for FIND to find. Excel returns `\[]`, and empty only before the first save. The model had no file to report. `source_dir` could not serve: it names the folder relative links to other workbooks are relative to, and must keep naming it after a Save As into another folder, because the links held in memory are still written for the old one — only the saved copy is rebased. `source_path` is therefore its own field, with that distinction written down. It is set when a workbook is opened, from the path made absolute so the folder is the one the user sees, and when a full-fidelity save makes a file the document's own — before the `saved` snapshot, or the workbook would read as dirty the moment it was saved, and only when it changes, so an ordinary Save does not clone the workbook for nothing. A workbook that arrived as bytes knows its name but never a folder and reports `[name]Sheet`, which still splits on the `]`; an import reports the file it came from, as Excel does. The sheet is the one the reference is on, not the active one. CELL is volatile and already in the volatile sets, so a cell holding it answers from the next recalculation; after a Save As that means F9 rather than the save itself, which the engine test asserts rather than papers over. Making a save recalculate is a larger decision than this issue. `check` still skips CELL("filename") as volatile, as docs/cli.md says: the cached value in a file is the folder of the machine that saved it. --- crates/calc/src/eval.rs | 11 ++++++- crates/calc/src/tests.rs | 37 +++++++++++++++++++++++ crates/engine/src/cmd/file.rs | 14 +++++++++ crates/engine/src/io.rs | 6 ++++ crates/engine/src/tests.rs | 55 +++++++++++++++++++++++++++++++++++ crates/model/src/workbook.rs | 8 +++++ 6 files changed, 130 insertions(+), 1 deletion(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 0f9cfb03d..b1bd40cdb 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -1373,7 +1373,16 @@ impl<'h> Evaluator<'h> { })) } "width" => Ev::V(Value::number(sheet.map_or(8.43, |s| gridcraft_model::points_to_chars(s.col_width(c.col) as f64)).round())), - "filename" => Ev::V(Value::text(String::new())), + // Excel's `/[]`, which the classic sheet-name formula splits on + // the `]`. Empty until the workbook has a file of its own, as in Excel (#1489). + "filename" => Ev::V(Value::text(match &wb.source_path { + Some(file) => { + let name = file.file_name().unwrap_or_default().to_string_lossy(); + let on = sheet.map_or("", |s| s.name.as_str()); + file.parent().unwrap_or(std::path::Path::new("")).join(format!("[{name}]{on}")).to_string_lossy().into_owned() + } + None => String::new(), + })), "sheetname" => Ev::V(Value::text(sheet.map(|s| s.name.clone()).unwrap_or_default())), "protect" => Ev::V(Value::number(if sheet.is_some_and(|s| wb.styles.get(s.style_id(c)).protection.locked) { 1.0 } else { 0.0 })), "format" => { diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 97f7a8350..6d1b73b5d 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -1294,3 +1294,40 @@ fn median_large_values_recalculate_dependents() { assert!((t.num(cell) + 1.3).abs() < 1e-14); } } + +#[test] +fn cell_filename_names_the_workbook_file() { + // Excel returns `/[]`, and the classic sheet-name formula splits it on + // the `]`; an empty string broke that formula in every German workbook that used it (#1489). + let dir = std::path::Path::new("books"); + let mut t = T::new(); + t.wb.sheet_mut(0).unwrap().name = "Info".into(); + + // No file yet: empty, as in a workbook Excel has never saved. + t.set("A1", "1"); + t.set("B1", "=CELL(\"filename\",A1)"); + assert_eq!(t.get("B1"), Value::from("")); + + t.wb.source_path = Some(dir.join("gc_cell_info_de.xlsx")); + t.calc = Calc::new(); + let expect = dir.join("[gc_cell_info_de.xlsx]Info").to_string_lossy().into_owned(); + + t.set("B2", "=CELL(\"filename\",A1)"); + assert_eq!(t.get("B2"), Value::from(expect.as_str())); + + // The two formulas from the report: the sheet name, and the test that the `]` is there. + t.set("B3", "=MID(CELL(\"filename\",A1),FIND(\"]\",CELL(\"filename\",A1))+1,31)"); + assert_eq!(t.get("B3"), Value::from("Info"), "the sheet-name formula"); + t.set("B4", "=ISNUMBER(FIND(\"]\",CELL(\"filename\",A1)))"); + assert_eq!(t.get("B4"), Value::Bool(true)); + // The sheet is the one the reference is on, not the active one. + t.wb.sheets.push(std::sync::Arc::new(gridcraft_model::Sheet::new("Second"))); + t.set("B6", "=CELL(\"filename\",Second!A1)"); + assert_eq!(t.get("B6"), Value::from(dir.join("[gc_cell_info_de.xlsx]Second").to_string_lossy().into_owned().as_str())); + + // A workbook that arrived as bytes knows its name but no folder: still splittable. + t.wb.source_path = Some(std::path::PathBuf::from("Book.xlsx")); + t.calc = Calc::new(); + t.set("B7", "=CELL(\"filename\",A1)"); + assert_eq!(t.get("B7"), Value::from("[Book.xlsx]Info")); +} diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 9ddc05611..2a925f951 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -58,6 +58,13 @@ fn open(s: &mut Session, p: &Json) -> Result { let encrypted = gridcraft_xlsx::sniff(&bytes) == gridcraft_xlsx::Format::Encrypted; let (mut wb, warnings) = crate::io::open_bytes_with(&name, &bytes, str_param(p, "password"), bool_param(p, "csvAsText").unwrap_or(false))?; wb.source_dir = str_param(p, "path").and_then(crate::io::source_dir); + // What `CELL("filename")` reports (#1489). A workbook opened from bytes knows its name but + // never a folder, so it reports `[name]Sheet` with none; an import reports the file it came + // from, as Excel does, even though Save must still ask where to put the .xlsx. + wb.source_path = match str_param(p, "path") { + Some(path) => crate::io::source_path(path), + None => Some(std::path::PathBuf::from(&name)), + }; let imported = matches!(crate::io::FileKind::from_path(&name), Some(crate::io::FileKind::Ods | crate::io::FileKind::Xlsb | crate::io::FileKind::Xls)) || matches!(gridcraft_xlsx::sniff(&bytes), gridcraft_xlsx::Format::Ods | gridcraft_xlsx::Format::Xlsb | gridcraft_xlsx::Format::Xls); @@ -122,6 +129,13 @@ fn save(s: &mut Session, p: &Json) -> Result { crate::io::write_file(&path, &bytes)?; // Only a full-fidelity format clears the dirty flag and becomes the document's file. if matches!(kind, Some(crate::io::FileKind::Xlsx | crate::io::FileKind::Json)) { + // The new file is the workbook's own now, so `CELL("filename")` reports it (#1489). Set + // before the `saved` snapshot, or the workbook would read as dirty the moment it was + // saved; and only when it changes, so an ordinary Save does not clone the workbook. + let file = crate::io::source_path(&path); + if d.wb.source_path != file { + std::sync::Arc::make_mut(&mut d.wb).source_path = file; + } d.saved = d.wb.clone(); d.path = Some(path.clone()); } diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 14717a3df..50fbb6f42 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -235,6 +235,12 @@ pub fn source_dir(path: &str) -> Option { std::path::absolute(path).ok()?.parent().map(std::path::Path::to_path_buf) } +/// The file at `path` as `CELL("filename")` reports it: absolute, so the folder is the one the +/// user sees. A workbook that arrived as bytes has no path and keeps only its name. +pub fn source_path(path: &str) -> Option { + std::path::absolute(path).ok() +} + /// Save As CSV/Text writes "the text and values as they are displayed in cells" with "all /// characters in each cell" (Microsoft Support, *Excel formatting and features that are not /// transferred to other file formats*): number formats applied, never column-width `#####`. diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 2ac290654..605b89450 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3344,3 +3344,58 @@ fn csv_as_text_keeps_identifier_columns_as_written() { let saved = s.execute("file.saveBytes", json!({"format": "csv"})).unwrap(); assert_eq!(crate::io::base64_decode(saved["base64"].as_str().unwrap()).unwrap(), csv.replace('\n', "\r\n").as_bytes()); } + +/// Open a workbook by path, and `CELL("filename")` names the file it came from, as Excel does; +/// Save As makes the new file the one it names. The sheet-name formula of #1489 depends on it. +#[test] +#[cfg(not(target_arch = "wasm32"))] +fn cell_filename_follows_the_file_the_workbook_lives_in() { + #[allow(clippy::disallowed_methods)] // native-only test: a unique temp dir name + let nonce = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); + let dir = std::env::temp_dir().join(format!("gridcraft-cellname-{}-{nonce}", std::process::id())); + std::fs::create_dir(&dir).unwrap(); + let name = "=CELL(\"filename\",A1)"; + let sheet_of = "=MID(CELL(\"filename\",A1),FIND(\"]\",CELL(\"filename\",A1))+1,31)"; + + // A new workbook has no file: empty, and the sheet-name formula cannot work yet. + let mut fresh = s(); + fresh.execute("cell.set", json!({"cell": "A1", "input": name})).unwrap(); + fresh.execute("cell.set", json!({"cell": "A2", "input": sheet_of})).unwrap(); + assert_eq!(v(&fresh, "A1"), Value::from("")); + assert_eq!(v(&fresh, "A2"), Value::Error(gridcraft_core::CellError::Value)); + + // Saved: it names the file. `CELL` is volatile, so the cells holding it answer from the next + // recalculation (F9), the same as every other volatile function; the save itself is not one. + let first = dir.join("book one.xlsx"); + fresh.execute("file.saveAs", json!({"path": first})).unwrap(); + let sheet = fresh.doc().unwrap().wb.active().unwrap().name.clone(); + // Saving did not leave the workbook looking modified. + assert!(!fresh.doc().unwrap().is_dirty()); + fresh.execute("formulas.calculateNow", json!({})).unwrap(); + assert_eq!(v(&fresh, "A1"), Value::from(dir.join(format!("[book one.xlsx]{sheet}")).to_string_lossy().into_owned().as_str())); + assert_eq!(v(&fresh, "A2"), Value::from(sheet.as_str())); + + // Opened from that path in another session: the same answer, from the file alone. + let mut opened = s(); + opened.execute("file.open", json!({"path": first})).unwrap(); + opened.take_ui_requests(); + assert_eq!(v(&opened, "A2"), Value::from(sheet.as_str())); + + // Save As into another folder: it follows the new file, not the old one. + let other = dir.join("sub"); + std::fs::create_dir(&other).unwrap(); + opened.execute("file.saveAs", json!({"path": other.join("book two.xlsx")})).unwrap(); + assert!(!opened.doc().unwrap().is_dirty()); + opened.execute("formulas.calculateNow", json!({})).unwrap(); + assert_eq!(v(&opened, "A1"), Value::from(other.join(format!("[book two.xlsx]{sheet}")).to_string_lossy().into_owned().as_str())); + + // Opened from bytes there is no folder, only a name — still splittable on the `]`. + let saved = opened.execute("file.saveBytes", json!({"format": "xlsx"})).unwrap(); + let mut from_bytes = s(); + from_bytes.execute("file.open", json!({"name": "from bytes.xlsx", "base64": saved["base64"]})).unwrap(); + from_bytes.take_ui_requests(); + assert_eq!(v(&from_bytes, "A1"), Value::from(format!("[from bytes.xlsx]{sheet}").as_str())); + assert_eq!(v(&from_bytes, "A2"), Value::from(sheet.as_str())); + + std::fs::remove_dir_all(dir).unwrap(); +} diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 828ca692e..ff9d851bd 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -120,6 +120,13 @@ pub struct Workbook { /// `preserved` are relative to. Saving into another folder rewrites them from there. Not in JSON. #[serde(skip)] pub source_dir: Option, + /// The file this workbook reports as its own, which is what `CELL("filename")` returns. + /// `None` = no file yet, which Excel reports as an empty string until the workbook has been + /// saved once. Deliberately separate from `source_dir`: that one names the folder relative + /// links are relative to and must keep naming it after a Save As into another folder, since + /// the links in memory are still written for the old one. Not in JSON. + #[serde(skip)] + pub source_path: Option, } impl Default for Workbook { @@ -148,6 +155,7 @@ impl Workbook { csv_delimiter: None, preserved: None, source_dir: None, + source_path: None, } } pub fn sheet(&self, i: usize) -> Option<&Sheet> { From e5aeea7a949ca7e18a4594a0f74a67ccc1a0c54b Mon Sep 17 00:00:00 2001 From: Hamza Date: Sat, 17 Oct 2026 16:59:27 +0500 Subject: [PATCH 22/28] Sheet protection: obey the Sort and Use AutoFilter boxes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SheetProtection` carries eleven permissions. `format_cells` is checked in `apply_style`, and an edit to a locked cell is caught after the fact by `check_protection`, but `sort` and `autofilter` had no reader anywhere outside `sheet_read.rs`, `sheet_write.rs` and a round-trip test: they only ever travelled through the file. So a protected sheet could still be sorted, and Ctrl+Shift+L still put an AutoFilter on it. `refuse_unless_allowed` sits beside `locked_cell_error`, with three call sites: `do_sort` (which both Sort… and the A-to-Z buttons come through), `toggle_filter` and `filter_by`. Where the box decides is worth knowing. Sorting a range of locked cells was already refused, by the locked-cell guard rather than the permission, because a sort moves values between locked cells — which is Excel's rule too ("users cannot sort ranges that contain locked cells on a protected worksheet, regardless of this setting"). So the Sort box decides for a range that has been unlocked, while an AutoFilter changes no cell's contents and the box is the only thing that ever decided it. The test asserts both, and that an unprotected sheet still sorts and filters. Not fixed here: Delete Rows and Delete Columns are refused even when their boxes are ticked, because deleting removes locked cells and trips the same post-hoc guard, while Insert Rows and Insert Columns work. Letting the guard know which operation ran is a change to code every command passes through and wants its own PR. --- crates/engine/src/cmd/data.rs | 5 +++ crates/engine/src/cmd/mod.rs | 12 +++++ crates/engine/src/tests.rs | 85 +++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 24bd043f7..e003cfffe 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -232,6 +232,9 @@ fn sort(s: &mut Session, p: &Json) -> Result { } fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec, by_cols: bool, case: bool) -> Result { + // Protect Sheet ▸ Sort. Both Sort… and the A-to-Z buttons come through here. + let sheet = s.doc()?.wb.active_sheet; + super::refuse_unless_allowed(s, sheet, "Sorting", |p| p.sort)?; let body = if by_cols { if header && r.width() > 1 { RangeRef::new(CellRef::new(r.start.row, r.start.col + 1), r.end) } else { r } } else if header && r.height() > 1 { @@ -438,6 +441,7 @@ fn sorted_anchor(sh: &Sheet, a: Anchor, bottom: (u32, u32, f32, f32), by_cols: b fn toggle_filter(s: &mut Session, p: &Json) -> Result { let sheet = s.doc()?.wb.active_sheet; + super::refuse_unless_allowed(s, sheet, "Filtering", |p| p.autofilter)?; let has = s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.autofilter.is_some()); let in_table = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.table_at(s.doc().map(|d| d.selection.active).unwrap_or_default()).map(|t| t.name.clone())); @@ -492,6 +496,7 @@ fn install_filter(s: &mut Session, p: &Json) -> Result { fn filter_by(s: &mut Session, p: &Json) -> Result { let sheet = s.doc()?.wb.active_sheet; + super::refuse_unless_allowed(s, sheet, "Filtering", |p| p.autofilter)?; let Some(sh) = s.doc()?.wb.sheet(sheet) else { return Err(EngineError::NoDocument) }; let af = match &sh.autofilter { Some(a) => a.clone(), diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 50865a2b7..9b44e6741 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -394,6 +394,18 @@ fn check_protection(old: &Workbook, new: &Workbook, unlocked: &[(String, String) Ok(()) } +/// Refuses an action a protected sheet does not allow, by the tick box for it in Review ▸ Protect +/// Sheet. `what` names the action in the message. An unprotected sheet allows everything. +/// +/// Cell edits are guarded separately, by whether the cell is locked (`locked_cell_error`): these +/// permissions are the ones that are not about one cell's contents. +pub(crate) fn refuse_unless_allowed(s: &Session, sheet: usize, what: &str, pick: fn(&gridcraft_model::SheetProtection) -> bool) -> Result<()> { + if s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.protection.as_ref().is_some_and(|pr| !pick(pr))) { + return Err(EngineError::Other(format!("{what} isn't allowed on this protected sheet (Review › Unprotect Sheet)."))); + } + Ok(()) +} + /// Whether a locked cell on a protected sheet stays locked: `None` when it's in an Allow Edit /// Range without a password or one unlocked in this session, else the message to show. pub(crate) fn locked_cell_error(sh: &gridcraft_model::Sheet, c: CellRef, unlocked: &[(String, String)]) -> Option { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 2ac290654..0e648fd22 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3344,3 +3344,88 @@ fn csv_as_text_keeps_identifier_columns_as_written() { let saved = s.execute("file.saveBytes", json!({"format": "csv"})).unwrap(); assert_eq!(crate::io::base64_decode(saved["base64"].as_str().unwrap()).unwrap(), csv.replace('\n', "\r\n").as_bytes()); } + +/// Protect Sheet's Sort and Use AutoFilter tick boxes are obeyed. They were read from the file and +/// written back but never consulted, so a protected sheet could still be sorted and filtered. +/// +/// Sorting locked cells stays refused whichever way the box is set, which is Excel's rule too +/// ("users cannot sort ranges that contain locked cells on a protected worksheet"), so the box +/// decides for a range that has been unlocked. An AutoFilter changes no cell's contents, so there +/// the box is the only thing that decides. +#[test] +fn a_protected_sheet_allows_sorting_and_filtering_only_when_it_says_so() { + let allow = |s: &mut Session, f: fn(&mut gridcraft_model::SheetProtection)| { + let d = s.doc_mut().unwrap(); + let mut wb = (*d.wb).clone(); + if let Some(p) = wb.sheet_mut(0).and_then(|sh| sh.protection.as_mut()) { + f(p); + } + d.wb = std::sync::Arc::new(wb); + }; + // Three rows under a header, and the cells unlocked so that the tick box is what decides. + let book = || { + let mut s = s(); + for (c, v) in [("A1", "Head"), ("A2", "2"), ("A3", "1")] { + s.execute("cell.set", json!({"cell": c, "input": v})).unwrap(); + } + s.execute("home.lockCell", json!({"range": "A1:A3", "on": false})).unwrap(); + s + }; + let order = |s: &Session| (v(s, "A2"), v(s, "A3")); + + // Unprotected: both work, so the guard has not made the ordinary case stricter. + let mut s = book(); + s.execute("data.sortAscending", json!({})).unwrap(); + assert_eq!(order(&s), (Value::Number(1.0), Value::Number(2.0))); + s.execute("data.filter", json!({})).unwrap(); + + // Protected with nothing ticked, which is what Excel's dialog opens with: both refused. + let mut s = book(); + s.execute("review.protectSheet", json!({})).unwrap(); + for (cmd, args) in [ + ("data.sortAscending", json!({})), + ("data.sortDescending", json!({})), + // Sort… needs a key to be a sort at all: with none it does nothing either way. + ("data.sort", json!({"keys": [{"column": "A", "order": "asc"}]})), + ("data.filter", json!({})), + ("data.filterBy", json!({})), + ] { + let e = s.execute(cmd, args).expect_err(cmd).to_string(); + assert!(e.contains("protected sheet"), "{cmd}: {e}"); + } + // Refused, not half-applied: the rows are where they were. + assert_eq!(order(&s), (Value::Number(2.0), Value::Number(1.0))); + assert!(s.doc().unwrap().wb.sheet(0).unwrap().autofilter.is_none()); + + // Sort ticked: sorting works on the unlocked range; filtering is still refused. + let mut s = book(); + s.execute("review.protectSheet", json!({})).unwrap(); + allow(&mut s, |p| p.sort = true); + s.execute("data.sortAscending", json!({})).unwrap(); + assert_eq!(order(&s), (Value::Number(1.0), Value::Number(2.0)), "sorted under the header"); + assert!(s.execute("data.filter", json!({})).is_err(), "Use AutoFilter is still off"); + + // Use AutoFilter ticked: the filter goes on, and sorting is refused. + let mut s = book(); + s.execute("review.protectSheet", json!({})).unwrap(); + allow(&mut s, |p| p.autofilter = true); + s.execute("data.filter", json!({})).unwrap(); + assert!(s.doc().unwrap().wb.sheet(0).unwrap().autofilter.is_some(), "the AutoFilter is on"); + assert!(s.execute("data.sortAscending", json!({})).is_err(), "Sort is still off"); + // Unprotecting gives sorting back. + s.execute("review.unprotectSheet", json!({})).unwrap(); + s.execute("data.sortAscending", json!({})).unwrap(); + + // Locked cells: ticking Sort does not let them be reordered, as in Excel. + let mut s = { + let mut t = super::tests::s(); + for (c, v) in [("A1", "Head"), ("A2", "2"), ("A3", "1")] { + t.execute("cell.set", json!({"cell": c, "input": v})).unwrap(); + } + t + }; + s.execute("review.protectSheet", json!({})).unwrap(); + allow(&mut s, |p| p.sort = true); + assert!(s.execute("data.sortAscending", json!({})).is_err(), "locked cells cannot be sorted"); + assert_eq!(order(&s), (Value::Number(2.0), Value::Number(1.0))); +} From ef339b9f2c3817fbb4adb3f05c18790298da9e83 Mon Sep 17 00:00:00 2001 From: MH Date: Mon, 19 Oct 2026 21:40:23 +0300 Subject: [PATCH 23/28] Grid: the selection outline follows a mouse drag at once The outline glides 0.12 s toward each new selection. While selecting with the mouse that makes it trail the pointer, which reads as lag (the grid itself paints in under a millisecond per frame). With a mouse button held it is now drawn on its cells at once, as in Excel; keyboard moves still glide. Co-Authored-By: Claude Opus 5.5 Signed-off-by: MH --- crates/ui-egui/src/grid.rs | 5 ++++- crates/ui-egui/tests/selection_glide.rs | 26 +++++++++++++++++++++++++ docs/ui-parity.md | 3 ++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 106bc6684..1b8808906 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -722,6 +722,9 @@ pub fn show(app: &mut SheetApp, ui: &mut egui::Ui) { app.grid.cells_rect = Some(geo.mirror_rect(geo.cells)); // The selection outline glides to a new selection. It stays on its cells when Animations is // off, when egui's animations are (the headless harness), and while a screenshot is on its way. + // A mouse button held down (a click or a drag selecting cells) moves it at once, as in Excel: + // a gliding outline trails the pointer and reads as lag. + let pointer_selecting = ui.input(|i| i.pointer.any_down()); let outline = app.grid.glide.frame(crate::selection_glide::Frame { place, range: sel.current(), @@ -729,7 +732,7 @@ pub fn show(app: &mut SheetApp, ui: &mut egui::Ui) { view: geo.cells, now, frame_nr: ui.ctx().cumulative_frame_nr(), - animate: app.ui.animations && ui.ctx().global_style().animation_time > 0.0 && !app.shots.in_flight(), + animate: app.ui.animations && ui.ctx().global_style().animation_time > 0.0 && !app.shots.in_flight() && !pointer_selecting, }); if app.grid.glide.is_gliding() { ui.ctx().request_repaint(); diff --git a/crates/ui-egui/tests/selection_glide.rs b/crates/ui-egui/tests/selection_glide.rs index fd8bf4e73..c8c253ac8 100644 --- a/crates/ui-egui/tests/selection_glide.rs +++ b/crates/ui-egui/tests/selection_glide.rs @@ -201,3 +201,29 @@ fn agents_find_run_and_read_the_setting_over_the_control_channel() { assert_eq!(set["result"]["animations"], json!(true)); assert!(a.ui.animations); } + +#[test] +fn a_mouse_drag_keeps_the_outline_on_its_cells() { + // A gliding outline trails the pointer while selecting with the mouse and reads as lag; Excel + // draws it on the selection at once. Keyboard moves still glide (above). + let mut h = animated(); + let (b2, e8) = (cell_rect(&h, "B2"), cell_rect(&h, "E8")); + let m = egui::Modifiers::NONE; + h.input_mut().events.extend([ + egui::Event::PointerMoved(b2.center()), + egui::Event::PointerButton { pos: b2.center(), button: egui::PointerButton::Primary, pressed: true, modifiers: m }, + ]); + h.step(); + for k in 1..=6 { + let p = b2.center() + (e8.center() - b2.center()) * (k as f32 / 6.0); + h.input_mut().events.push(egui::Event::PointerMoved(p)); + steps(&mut h, 2); + let sel = h.state().session.active().map(|d| d.selection.current()).unwrap(); + let shown = cell_rect(&h, &sel.start.a1()).union(cell_rect(&h, &sel.end.a1())); + assert!(!gliding(&h), "no glide while the button is held"); + assert_eq!(outline(&h), shown, "the outline is on the dragged range {}", sel.a1()); + } + h.input_mut().events.push(egui::Event::PointerButton { pos: e8.center(), button: egui::PointerButton::Primary, pressed: false, modifiers: m }); + h.step(); + assert_eq!(h.state().session.active().map(|d| d.selection.current().a1()).as_deref(), Some("B2:E8")); +} diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 34cb502f5..8310036c3 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -1,6 +1,6 @@ # UI parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-19 · **Change:** minor (Ctrl+Enter fill gives each cell the automatic number format typing gives it; keytip letters typed with Alt held, #1846) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-19 · **Change:** trivial (the selection outline follows a mouse drag at once; it still glides for keyboard moves) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Interaction, controls and feel, against Excel. Part of [target-app-parity.md](target-app-parity.md). **UI/UX fidelity: ~55% ready, 50–80 h** (estimated). Ribbon command coverage is measured @@ -161,6 +161,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-19 | trivial | While a mouse button is held (a click or a drag selecting cells) the selection outline is drawn on its cells at once instead of gliding after the pointer, which read as lag; keyboard moves still glide | | 2026-10-19 | minor | Ctrl+Enter (`range.fill`) gives each cell what typing the input there gives: a General cell gets the automatic number format (`5%`, `$5`, `3/4/2026`, `=TODAY()`) even when it already held a value, and a cell with its own format, a formatted row or column, or Text keeps it. Cells that held a value used to keep General (`5%` showed 0.05), and empty cells took the active cell's format | | 2026-10-18 | minor | Keytip letters typed with left Alt held continue the sequence (Alt+H, O, A / Alt+H, O, I ran nothing before: Alt+letter cancelled keytips); other Alt combinations, AltGr and macOS Option are unchanged (#1846) | | 2026-10-12 | minor | The File view's recent list tells when each workbook was last changed, read off the UI thread | From 40803e6acaaddd3d116a7df075b6c64401fe3d39 Mon Sep 17 00:00:00 2001 From: midasdf <205497066+midasdf@users.noreply.github.com> Date: Sat, 17 Oct 2026 01:58:52 +0900 Subject: [PATCH 24/28] =?UTF-8?q?XLSX:=20keep=20Japanese=20phonetic=20runs?= =?UTF-8?q?=20(=E3=81=B5=E3=82=8A=E3=81=8C=E3=81=AA)=20on=20open=20and=20s?= =?UTF-8?q?ave?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit xl/sharedStrings.xml can carry phonetic runs (ふりがな) and a beside a string item. GridCraft read only the text and wrote every string back as …, so a plain open + save deleted a Japanese workbook's furigana. Both are now read — offsets validated against the base text, runs with bad or missing offsets, sb >= eb or no reading dropped with a warning, and never used to index — and kept on the workbook as Workbook::phonetics, one entry per string text. The writer emits them next to the of the same text. The saved table holds each text once, so two identical texts cannot keep two different readings: the reader keeps the first and warns. Strings without phonetic data are written byte for byte as before. Behaviour reference: ISO/IEC 29500-1 §18.4.6 rPh and §18.4.7 phoneticPr (Microsoft Learn PhoneticRun/PhoneticProperties). Tests: crates/xlsx/src/tests/phonetic_tests.rs (round trip, plain string, hostile offsets, shared text, unused data, comments and inline strings). Docs: file-format-parity.md, localization-parity.md. --- crates/model/src/lib.rs | 2 +- crates/model/src/workbook.rs | 70 +++++++++++ crates/xlsx/src/read.rs | 52 +++++++- crates/xlsx/src/tests/mod.rs | 1 + crates/xlsx/src/tests/phonetic_tests.rs | 151 ++++++++++++++++++++++++ crates/xlsx/src/write.rs | 7 +- crates/xlsx/src/xml.rs | 144 +++++++++++++++++++++- docs/file-format-parity.md | 8 +- docs/localization-parity.md | 5 +- 9 files changed, 426 insertions(+), 14 deletions(-) create mode 100644 crates/xlsx/src/tests/phonetic_tests.rs diff --git a/crates/model/src/lib.rs b/crates/model/src/lib.rs index a76f30fc3..c236d5b2e 100644 --- a/crates/model/src/lib.rs +++ b/crates/model/src/lib.rs @@ -25,7 +25,7 @@ pub use style::{ Alignment, BorderLine, BorderStyle, Borders, Color, Fill, Font, HAlign, NumFmt, PatternType, Style, StyleId, StyleTable, Theme, Underline, VAlign, VertAlign, }; -pub use workbook::{CalcMode, CalcSettings, DefinedName, DocProps, Workbook}; +pub use workbook::{CalcMode, CalcSettings, DefinedName, DocProps, PhoneticProps, PhoneticRun, PhoneticString, Workbook}; /// Font used for new workbooks. Only a name: renderers substitute an available open font /// (e.g. the metric-compatible Carlito) when it isn't installed. diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 828ca692e..12e4bb6af 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -85,6 +85,47 @@ pub struct DocProps { pub description: String, } +/// One phonetic (furigana) run of a string item (``): the reading `text` and the range of the +/// base text it annotates. +/// +/// `start` and `end` are zero-based offsets into the base text (`sb` and `eb` in the file) with +/// `start < end`; reading keeps them within the text's length in UTF-16 code units. Behaviour +/// reference: ISO/IEC 29500-1 §18.4.6 `rPh` and the Microsoft Learn `PhoneticRun` class. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +pub struct PhoneticRun { + pub start: u32, + pub end: u32, + pub text: String, +} + +/// The `` of a string item: how its phonetic runs are displayed. `kind` (`type`) and +/// `alignment` only ever hold values ISO/IEC 29500-1 §18.18.57 `ST_PhoneticType` / §18.18.56 +/// `ST_PhoneticAlignment` define, so damaged files cannot be written back invalid. +#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct PhoneticProps { + /// `fontId`: a required attribute, an index into the style sheet's fonts. A file without one + /// gets 0 — the Normal style's font, which an out-of-range index falls back to anyway. + #[serde(default)] + pub font_id: u32, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub kind: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub alignment: Option, +} + +/// The phonetic (furigana) data of one shared string: its `` runs, in document order, and its +/// `` when the file had one. +/// +/// `text` is the string's base text — the only handle a cell keeps on its shared string — and is +/// the key the writer uses to re-emit the runs next to the right ``. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +pub struct PhoneticString { + pub text: String, + pub runs: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub props: Option, +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct Workbook { pub sheets: Vec>, @@ -104,6 +145,17 @@ pub struct Workbook { pub custom_lists: Vec>, /// Named cell styles (Cell Styles gallery): name → style. pub cell_styles: Vec<(String, crate::style::Style)>, + /// Phonetic (furigana) data of shared strings (`xl/sharedStrings.xml`), one entry per string + /// text, so that an open + save keeps a Japanese workbook's readings. Written back next to the + /// `` holding the same text. + /// + /// Keyed by text because that is all a cell keeps of its shared string: the saved table holds + /// each text once (the format's `uniqueCount`), so two `` items that share a base text can + /// only keep one reading — the reader keeps the first and warns. Empty for workbooks without + /// phonetic data; not present in older JSON. Modelled data (unlike [`Workbook::preserved`], + /// which holds unmodelled parts), so it is saved to JSON as well as to XLSX. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub phonetics: Vec, /// VBA project (`xl/vbaProject.bin`) carried through unchanged. GridCraft does not run /// macros; the blob is preserved so that saving a macro-enabled workbook never destroys the /// macros it arrived with. base64 in JSON. `None` = the workbook has no macros. @@ -144,6 +196,7 @@ impl Workbook { structure_password: None, custom_lists: vec![], cell_styles: vec![], + phonetics: vec![], vba_project: None, csv_delimiter: None, preserved: None, @@ -296,6 +349,23 @@ fn spell_calls(e: Expr, lambdas: &[String]) -> Expr { mod tests { use super::*; + #[test] + fn phonetics_are_additive_in_json() { + let mut wb = Workbook::new(); + assert!(wb.phonetics.is_empty(), "new workbooks have no phonetic data"); + wb.phonetics.push(PhoneticString { + text: "東".into(), + runs: vec![PhoneticRun { start: 0, end: 1, text: "ヒガシ".into() }], + props: Some(PhoneticProps { font_id: 3, kind: Some("Hiragana".into()), alignment: None }), + }); + let json = serde_json::to_string(&wb).expect("serialize"); + assert_eq!(serde_json::from_str::(&json).expect("deserialize").phonetics, wb.phonetics); + // JSON written before the field existed still loads. + let mut value: serde_json::Value = serde_json::from_str(&json).expect("value"); + value.as_object_mut().expect("an object").remove("phonetics"); + assert!(serde_json::from_value::(value).expect("older JSON").phonetics.is_empty()); + } + #[test] fn sheet_names() { let wb = Workbook::new(); diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 1a479081c..56858cb86 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use gridcraft_core::{DateSystem, RangeRef}; -use gridcraft_model::{CalcMode, CellPicture, DefinedName, Sheet, Style, StyleId, StyleTable, Visibility, Workbook}; +use gridcraft_model::{CalcMode, CellPicture, DefinedName, PhoneticString, Sheet, Style, StyleId, StyleTable, Visibility, Workbook}; use crate::package::{Package, Rel}; use crate::styles::{StylesIn, read_styles}; @@ -15,6 +15,8 @@ pub struct Ctx<'a> { pub pkg: Package<'a>, pub report: ReadReport, pub sst: Vec>, + /// Phonetic (furigana) data of the shared strings, one entry per base text. + pub phonetics: Vec, /// Cell format index → model style. pub xf_map: Vec, pub dxfs: Vec