From 74de1b32ebc2097fb5545f3851c3db0ae6b136c7 Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Wed, 14 Oct 2026 18:14:07 -0500 Subject: [PATCH 01/21] Keytips: Alt, H, B border letters apply their own preset Every Alt, H, B letter ran home.borders with no params, which defaults to a bottom border, so H,B,N (No Border) and H,B,A (All Borders) both drew a bottom border. keytips.json entries now take optional fixed `params` (Action::CommandWith / DialogWith), and each Borders menu letter passes the preset Excel's menu gives it. H,B,M opens Format Cells on the Border tab. Erase Border, Line Color and Line Style are reserved (pending) instead of drawing a bottom border. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 3 +- crates/ui-egui/src/keytips.rs | 5 + crates/ui-egui/src/keytips/keytip_table.rs | 4 +- .../src/keytips/keytip_table/generated.rs | 64 ++++++------- crates/ui-egui/src/keytips/keytips.json | 90 +++++++++++++----- crates/ui-egui/src/lib.rs | 12 +++ crates/ui-egui/tests/keytips.rs | 93 +++++++++++++++++++ crates/ui-egui/tests/keytips_generated.rs | 17 +++- docs/ui-parity.md | 5 +- xtask/src/keytips.rs | 39 ++++++-- 10 files changed, 260 insertions(+), 72 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..708f80383 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-14 · **Change:** minor (Alt, H, B border keytips apply their own preset) · **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-14 | Border keytips: each Alt, H, B letter applies the preset Excel's Borders menu gives it (H,B,O bottom, H,B,B bottom double, H,B,N no border, H,B,A all borders and the rest); they all drew a bottom border before. H,B,M opens Format Cells on the Border tab | | 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) | diff --git a/crates/ui-egui/src/keytips.rs b/crates/ui-egui/src/keytips.rs index e1163d9bb..07324dac7 100644 --- a/crates/ui-egui/src/keytips.rs +++ b/crates/ui-egui/src/keytips.rs @@ -10,7 +10,12 @@ pub mod keytip_table; pub enum Action { Tab(&'static str), Command(&'static str), + /// A command run with fixed params (a JSON object literal from keytips.json `params`), for + /// menu items that share one command id, like the Borders presets (HBO bottom, HBN none). + CommandWith(&'static str, &'static str), Dialog(&'static str), + /// A dialog opened with fixed params, like Format Cells on its Border tab (HBM). + DialogWith(&'static str, &'static str), /// The target command does not exist yet: the sequence is reserved, its badge shows, and /// invoking it is a silent no-op until the command lands (see keytips.json `pending`). Pending(&'static str), diff --git a/crates/ui-egui/src/keytips/keytip_table.rs b/crates/ui-egui/src/keytips/keytip_table.rs index 5415b9d2b..d14af141b 100644 --- a/crates/ui-egui/src/keytips/keytip_table.rs +++ b/crates/ui-egui/src/keytips/keytip_table.rs @@ -50,8 +50,8 @@ pub fn is_group_level(seq: &str) -> bool { fn matches_target(action: &Action, target: &str) -> bool { match action { - Action::Command(id) | Action::Pending(id) | Action::Control(id) => *id == target, - Action::Dialog(name) => format!("dialog:{name}") == target, + Action::Command(id) | Action::CommandWith(id, _) | Action::Pending(id) | Action::Control(id) => *id == target, + Action::Dialog(name) | Action::DialogWith(name, _) => format!("dialog:{name}") == target, Action::Tab(_) => false, } } diff --git a/crates/ui-egui/src/keytips/keytip_table/generated.rs b/crates/ui-egui/src/keytips/keytip_table/generated.rs index 79920b5b8..5f750e770 100644 --- a/crates/ui-egui/src/keytips/keytip_table/generated.rs +++ b/crates/ui-egui/src/keytips/keytip_table/generated.rs @@ -37,40 +37,40 @@ pub const SEQUENCES: &[(&str, Entry)] = &[ ("HAN", Entry::Finish(Action::Command("home.accounting"))), // Excel: Accounting Format; fires immediately in Excel; deeper aliases dropped as unreachable ("HAS", Entry::Finish(Action::Command("home.addIns"))), // Excel: Add-ins; containment(Add-ins) ("HB", Entry::Continue), // Excel: Borders Menu; group level: - ("HBA", Entry::Finish(Action::Command("home.borders"))), // Excel: All Borders; containment(Borders) - ("HBB", Entry::Finish(Action::Command("home.borders"))), // Excel: Bottom Double Border; border-side TBD - ("HBC", Entry::Finish(Action::Command("home.borders"))), // Excel: Top and Thick Bottom Border; border-side TBD - ("HBD", Entry::Finish(Action::Command("home.borders"))), // Excel: Top and Bottom Border; border-side TBD - ("HBE", Entry::Finish(Action::Command("home.borders"))), // Excel: Erase Border; border-side TBD - ("HBG", Entry::Finish(Action::Command("home.drawBorder"))), // Excel: Draw Border Grid; containment(Draw Border) - ("HBH", Entry::Finish(Action::Command("home.borders"))), // Excel: Thick Bottom Border; border-side TBD - ("HBI", Entry::Finish(Action::Command("home.borders"))), // Excel: Line Color; Line Color (submenu TBD) - ("HBL", Entry::Finish(Action::Command("home.borders"))), // Excel: Left Border; border-side TBD - ("HBM", Entry::Finish(Action::Command("home.borders"))), // Excel: More Borders; containment(Borders) - ("HBN", Entry::Finish(Action::Command("home.borders"))), // Excel: No Border; border-side TBD - ("HBO", Entry::Finish(Action::Command("home.borders"))), // Excel: Bottom Border; border-side TBD - ("HBP", Entry::Finish(Action::Command("home.borders"))), // Excel: Top Border; border-side TBD - ("HBR", Entry::Finish(Action::Command("home.borders"))), // Excel: Right Border; border-side TBD - ("HBS", Entry::Finish(Action::Command("home.borders"))), // Excel: Outside Borders; containment(Borders) - ("HBT", Entry::Finish(Action::Command("home.borders"))), // Excel: Thick Outside Borders; border-side TBD - ("HBU", Entry::Finish(Action::Command("home.borders"))), // Excel: Top and Double Bottom Border; border-side TBD - ("HBW", Entry::Finish(Action::Command("home.drawBorder"))), // Excel: Draw Border - ("HBY", Entry::Finish(Action::Command("home.borders"))), // Excel: Line Style; Line Style (submenu TBD) - ("HC", Entry::Finish(Action::Command("edit.copy"))), // Excel: Copy; fires immediately in Excel; deeper aliases dropped as unreachable - ("HDC", Entry::Finish(Action::Command("home.deleteColumns"))), // Excel: Delete Columns; Delete Columns - ("HDD", Entry::Finish(Action::Dialog("deleteCells"))), // Excel: Delete Cells - ("HDR", Entry::Finish(Action::Command("home.deleteRows"))), // Excel: Delete Rows; Delete Rows - ("HDS", Entry::Finish(Action::Command("home.deleteSheet"))), // Excel: Delete Sheet - ("HEA", Entry::Finish(Action::Command("edit.clearAll"))), // Excel: Clear All - ("HEC", Entry::Finish(Action::Command("edit.clearContents"))), // Excel: Clear Contents - ("HEF", Entry::Finish(Action::Command("edit.clearFormats"))), // Excel: Clear Formats + ("HBA", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"all\"}"))), // Excel: All Borders; Borders menu item + ("HBB", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"doubleBottom\"}"))), // Excel: Bottom Double Border; Borders menu item + ("HBC", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"topThickBottom\"}"))), // Excel: Top and Thick Bottom Border; Borders menu item + ("HBD", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"topBottom\"}"))), // Excel: Top and Bottom Border; Borders menu item + ("HBE", Entry::Finish(Action::Pending("home.eraseBorder"))), // pending; Excel: Erase Border; pending: Excel arms an eraser pen; GridCraft has no border pen mode yet + ("HBG", Entry::Finish(Action::Command("home.drawBorder"))), // Excel: Draw Border Grid; containment(Draw Border) + ("HBH", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"thickBottom\"}"))), // Excel: Thick Bottom Border; Borders menu item + ("HBI", Entry::Finish(Action::Pending("home.borderLineColor"))), // pending; Excel: Line Color; pending: Line Color submenu + ("HBL", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"left\"}"))), // Excel: Left Border; Borders menu item + ("HBM", Entry::Finish(Action::DialogWith("formatCells", "{\"tab\":\"Border\"}"))), // Excel: More Borders; Format Cells on its Border tab + ("HBN", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"none\"}"))), // Excel: No Border; Borders menu item + ("HBO", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"bottom\"}"))), // Excel: Bottom Border; Borders menu item + ("HBP", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"top\"}"))), // Excel: Top Border; Borders menu item + ("HBR", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"right\"}"))), // Excel: Right Border; Borders menu item + ("HBS", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"outside\"}"))), // Excel: Outside Borders; Borders menu item + ("HBT", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"thickOutside\"}"))), // Excel: Thick Outside Borders; Borders menu item + ("HBU", Entry::Finish(Action::CommandWith("home.borders", "{\"preset\":\"topDoubleBottom\"}"))), // Excel: Top and Double Bottom Border; Borders menu item + ("HBW", Entry::Finish(Action::Command("home.drawBorder"))), // Excel: Draw Border + ("HBY", Entry::Finish(Action::Pending("home.borderLineStyle"))), // pending; Excel: Line Style; pending: Line Style submenu + ("HC", Entry::Finish(Action::Command("edit.copy"))), // Excel: Copy; fires immediately in Excel; deeper aliases dropped as unreachable + ("HDC", Entry::Finish(Action::Command("home.deleteColumns"))), // Excel: Delete Columns; Delete Columns + ("HDD", Entry::Finish(Action::Dialog("deleteCells"))), // Excel: Delete Cells + ("HDR", Entry::Finish(Action::Command("home.deleteRows"))), // Excel: Delete Rows; Delete Rows + ("HDS", Entry::Finish(Action::Command("home.deleteSheet"))), // Excel: Delete Sheet + ("HEA", Entry::Finish(Action::Command("edit.clearAll"))), // Excel: Clear All + ("HEC", Entry::Finish(Action::Command("edit.clearContents"))), // Excel: Clear Contents + ("HEF", Entry::Finish(Action::Command("edit.clearFormats"))), // Excel: Clear Formats ("HEL", Entry::Finish(Action::Command("edit.clearHyperlinks"))), // Excel: Clear Hyperlinks - ("HEM", Entry::Finish(Action::Command("edit.clearComments"))), // Excel: Clear Comments and Notes; containment(Clear Comments and Notes) - ("HFC", Entry::Finish(Action::Control("home.fontColor"))), // Excel: Font Color; opens the font-color palette + ("HEM", Entry::Finish(Action::Command("edit.clearComments"))), // Excel: Clear Comments and Notes; containment(Clear Comments and Notes) + ("HFC", Entry::Finish(Action::Control("home.fontColor"))), // Excel: Font Color; opens the font-color palette ("HFDC", Entry::Finish(Action::Command("home.conditionalFormat"))), // Excel: Find Conditional Formatting; containment(Conditional Formatting) - ("HFDF", Entry::Finish(Action::Dialog("find"))), // Excel: Find - ("HFDG", Entry::Finish(Action::Dialog("goTo"))), // Excel: Go To - ("HFDM", Entry::Finish(Action::Command("edit.find"))), // Excel: Find Notes; containment(Find) + ("HFDF", Entry::Finish(Action::Dialog("find"))), // Excel: Find + ("HFDG", Entry::Finish(Action::Dialog("goTo"))), // Excel: Go To + ("HFDM", Entry::Finish(Action::Command("edit.find"))), // Excel: Find Notes; containment(Find) ("HFDO", Entry::Finish(Action::Pending("home.selectObjects"))), // pending; Excel: Select Objects; Select Objects; reserved until the command lands ("HFDP", Entry::Finish(Action::Command("arrange.selectionPane"))), // Excel: Selection Pane ("HFDR", Entry::Finish(Action::Dialog("find"))), // Excel: Replace; opens Find and Replace diff --git a/crates/ui-egui/src/keytips/keytips.json b/crates/ui-egui/src/keytips/keytips.json index 59aeadcfb..b24241c3c 100644 --- a/crates/ui-egui/src/keytips/keytips.json +++ b/crates/ui-egui/src/keytips/keytips.json @@ -8,6 +8,7 @@ " \"continue\" (an explicit group level with no deeper sequences yet).", " - Intermediate levels need no entries: the matcher waits automatically when deeper sequences exist.", " - pending: true = the command does not exist yet; the sequence stays reserved and silent until it lands.", + " - params: an optional JSON object passed to a command or dialog, for menu items that share one id (the Borders presets: HBO {\"preset\": \"bottom\"}).", " - Keep entries grouped by tab and sorted by sequence within a group; the FIRST entry for a tab is its canonical badge (legacy aliases come after it)." ], "sequences": [ @@ -196,36 +197,49 @@ "seq": "HBA", "kind": "command", "target": "home.borders", + "params": { + "preset": "all" + }, "excel": "All Borders", - "note": "containment(Borders)" + "note": "Borders menu item" }, { "seq": "HBB", "kind": "command", "target": "home.borders", + "params": { + "preset": "doubleBottom" + }, "excel": "Bottom Double Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBC", "kind": "command", "target": "home.borders", + "params": { + "preset": "topThickBottom" + }, "excel": "Top and Thick Bottom Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBD", "kind": "command", "target": "home.borders", + "params": { + "preset": "topBottom" + }, "excel": "Top and Bottom Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBE", "kind": "command", - "target": "home.borders", + "target": "home.eraseBorder", + "pending": true, "excel": "Erase Border", - "note": "border-side TBD" + "note": "pending: Excel arms an eraser pen; GridCraft has no border pen mode yet" }, { "seq": "HBG", @@ -238,78 +252,109 @@ "seq": "HBH", "kind": "command", "target": "home.borders", + "params": { + "preset": "thickBottom" + }, "excel": "Thick Bottom Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBI", "kind": "command", - "target": "home.borders", + "target": "home.borderLineColor", + "pending": true, "excel": "Line Color", - "note": "Line Color (submenu TBD)" + "note": "pending: Line Color submenu" }, { "seq": "HBL", "kind": "command", "target": "home.borders", + "params": { + "preset": "left" + }, "excel": "Left Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBM", - "kind": "command", - "target": "home.borders", + "kind": "dialog", + "target": "formatCells", + "params": { + "tab": "Border" + }, "excel": "More Borders", - "note": "containment(Borders)" + "note": "Format Cells on its Border tab" }, { "seq": "HBN", "kind": "command", "target": "home.borders", + "params": { + "preset": "none" + }, "excel": "No Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBO", "kind": "command", "target": "home.borders", + "params": { + "preset": "bottom" + }, "excel": "Bottom Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBP", "kind": "command", "target": "home.borders", + "params": { + "preset": "top" + }, "excel": "Top Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBR", "kind": "command", "target": "home.borders", + "params": { + "preset": "right" + }, "excel": "Right Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBS", "kind": "command", "target": "home.borders", + "params": { + "preset": "outside" + }, "excel": "Outside Borders", - "note": "containment(Borders)" + "note": "Borders menu item" }, { "seq": "HBT", "kind": "command", "target": "home.borders", + "params": { + "preset": "thickOutside" + }, "excel": "Thick Outside Borders", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBU", "kind": "command", "target": "home.borders", + "params": { + "preset": "topDoubleBottom" + }, "excel": "Top and Double Bottom Border", - "note": "border-side TBD" + "note": "Borders menu item" }, { "seq": "HBW", @@ -321,9 +366,10 @@ { "seq": "HBY", "kind": "command", - "target": "home.borders", + "target": "home.borderLineStyle", + "pending": true, "excel": "Line Style", - "note": "Line Style (submenu TBD)" + "note": "pending: Line Style submenu" }, { "seq": "HC", diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f96cdac3c..4e7de595a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1193,7 +1193,19 @@ impl SheetApp { keytips::Action::Command(id @ ("edit.copy" | "edit.cut")) => self.copy_to_clipboard(ctx, id), keytips::Action::Command("edit.paste") => self.paste_from_clipboard(), keytips::Action::Command(id) => self.run_or_alert(id, json!({})), + keytips::Action::CommandWith(id, params) => { + let params: Json = serde_json::from_str(params).unwrap_or_else(|_| json!({})); + // A Borders preset picked by key tip becomes the split button's face, as a + // click on the same menu item does. + if id == "home.borders" + && let Some(preset) = params.get("preset").and_then(Json::as_str) + { + self.grid.last_border = preset.to_string(); + } + self.run_or_alert(id, params); + } keytips::Action::Dialog(name) => self.open_dialog(name, json!({})), + keytips::Action::DialogWith(name, params) => self.open_dialog(name, serde_json::from_str(params).unwrap_or_else(|_| json!({}))), // Reserved sequence: fires silently while its command is missing. When the // command lands in the catalog it goes live with no table edit. keytips::Action::Pending(id) => { diff --git a/crates/ui-egui/tests/keytips.rs b/crates/ui-egui/tests/keytips.rs index 24be742a5..76ddd4467 100644 --- a/crates/ui-egui/tests/keytips.rs +++ b/crates/ui-egui/tests/keytips.rs @@ -550,3 +550,96 @@ fn every_sequence_terminates_without_panicking() { } assert_eq!(SEQUENCES.len(), SEQUENCES.len(), "every sequence ran"); } + +fn borders(h: &Harness, cell: &str) -> gridcraft_engine::model::Borders { + let d = h.state().session.active().unwrap(); + let sh = d.wb.active().unwrap(); + d.wb.styles.get(sh.style_id(CellRef::parse(cell).unwrap())).borders +} + +/// [left, right, top, bottom] line styles of `cell`. +fn sides(h: &Harness, cell: &str) -> [gridcraft_engine::model::BorderStyle; 4] { + let b = borders(h, cell); + [b.left.style, b.right.style, b.top.style, b.bottom.style] +} + +fn selected(range: &str) -> Session { + let mut s = blank(); + s.execute("selection.set", json!({"range": range})).unwrap(); + s +} + +#[test] +fn border_keytips_apply_their_own_preset() { + use gridcraft_engine::model::BorderStyle::{Double, None as N, Thick, Thin}; + // Alt, H, B, letter on A1 alone: each Borders menu letter draws its own preset (they all drew + // a bare bottom border before). Letters observed in Excel's Borders menu. + for (k, want) in [ + (Key::O, [N, N, N, Thin]), + (Key::P, [N, N, Thin, N]), + (Key::L, [Thin, N, N, N]), + (Key::R, [N, Thin, N, N]), + (Key::A, [Thin, Thin, Thin, Thin]), + (Key::S, [Thin, Thin, Thin, Thin]), + (Key::T, [Thick, Thick, Thick, Thick]), + (Key::B, [N, N, N, Double]), + (Key::H, [N, N, N, Thick]), + (Key::D, [N, N, Thin, Thin]), + (Key::C, [N, N, Thin, Thick]), + (Key::U, [N, N, Thin, Double]), + ] { + let mut h = harness(selected("A1")); + sequence(&mut h, &[Key::H, Key::B, k], false); + assert!(h.state().message.is_none() && h.state().toast.is_none(), "H,B,{k:?} must run cleanly"); + assert_eq!(sides(&h, "A1"), want, "H,B,{k:?}"); + } +} + +#[test] +fn border_keytip_no_border_clears_a_bordered_cell() { + use gridcraft_engine::model::BorderStyle::None as N; + let mut s = selected("A1"); + s.execute("home.borders", json!({"range": "A1", "preset": "all"})).unwrap(); + let mut h = harness(s); + sequence(&mut h, &[Key::H, Key::B, Key::N], false); + assert_eq!(sides(&h, "A1"), [N, N, N, N], "H,B,N must clear every side, not add a bottom border"); + assert_eq!(h.state().grid.last_border, "none", "the picked preset becomes the Borders button's face"); +} + +#[test] +fn border_keytip_all_borders_covers_every_side_of_a_block() { + use gridcraft_engine::model::BorderStyle::Thin; + let mut h = harness(selected("A1:B2")); + sequence(&mut h, &[Key::H, Key::B, Key::A], false); + for cell in ["A1", "B1", "A2", "B2"] { + assert_eq!(sides(&h, cell), [Thin; 4], "H,B,A must border every side of {cell}"); + } +} + +#[test] +fn border_keytip_outside_borders_skips_the_inside_edges() { + use gridcraft_engine::model::BorderStyle::{None as N, Thin}; + let mut h = harness(selected("A1:B2")); + sequence(&mut h, &[Key::H, Key::B, Key::S], false); + assert_eq!(sides(&h, "A1"), [Thin, N, Thin, N]); + assert_eq!(sides(&h, "B2"), [N, Thin, N, Thin]); +} + +#[test] +fn border_keytip_more_borders_opens_format_cells_on_the_border_tab() { + let mut h = harness(blank()); + sequence(&mut h, &[Key::H, Key::B, Key::M], false); + let d = h.state().dialog.as_ref().expect("H,B,M opens Format Cells"); + assert_eq!(d.tab, "Border"); +} + +#[test] +fn border_keytips_without_a_command_stay_silent() { + // Erase Border (a pen mode), Line Color and Line Style are reserved: no border is drawn. + for k in [Key::E, Key::I, Key::Y] { + let mut h = harness(selected("A1")); + sequence(&mut h, &[Key::H, Key::B, k], false); + assert!(!h.state().session.journal.iter().any(|(id, _)| id.starts_with("home.")), "H,B,{k:?} must not run a command"); + assert_eq!(borders(&h, "A1"), Default::default(), "H,B,{k:?} must not draw a border"); + } +} diff --git a/crates/ui-egui/tests/keytips_generated.rs b/crates/ui-egui/tests/keytips_generated.rs index 653554857..dcf71cde1 100644 --- a/crates/ui-egui/tests/keytips_generated.rs +++ b/crates/ui-egui/tests/keytips_generated.rs @@ -308,7 +308,10 @@ fn keytip_hbd_() { fn keytip_hbe_() { let mut h = harness(blank()); sequence(&mut h, "HBE", false); - assert!(h.state().session.journal.iter().any(|(id, _)| id == "home.borders"), "HBE must journal home.borders"); + assert!(h.state().dialog.is_none(), "HBE must not open a dialog"); + assert!(h.state().message.is_none(), "HBE must not open a message box"); + assert!(h.state().toast.is_none(), "HBE must not raise a toast"); + assert!(h.state().session.journal.is_empty(), "HBE must not journal"); } #[test] @@ -329,7 +332,10 @@ fn keytip_hbh_() { fn keytip_hbi_() { let mut h = harness(blank()); sequence(&mut h, "HBI", false); - assert!(h.state().session.journal.iter().any(|(id, _)| id == "home.borders"), "HBI must journal home.borders"); + assert!(h.state().dialog.is_none(), "HBI must not open a dialog"); + assert!(h.state().message.is_none(), "HBI must not open a message box"); + assert!(h.state().toast.is_none(), "HBI must not raise a toast"); + assert!(h.state().session.journal.is_empty(), "HBI must not journal"); } #[test] @@ -343,7 +349,7 @@ fn keytip_hbl_() { fn keytip_hbm_() { let mut h = harness(blank()); sequence(&mut h, "HBM", false); - assert!(h.state().session.journal.iter().any(|(id, _)| id == "home.borders"), "HBM must journal home.borders"); + assert!(h.state().dialog.is_some(), "HBM must open the formatCells dialog"); } #[test] @@ -406,7 +412,10 @@ fn keytip_hbw_() { fn keytip_hby_() { let mut h = harness(blank()); sequence(&mut h, "HBY", false); - assert!(h.state().session.journal.iter().any(|(id, _)| id == "home.borders"), "HBY must journal home.borders"); + assert!(h.state().dialog.is_none(), "HBY must not open a dialog"); + assert!(h.state().message.is_none(), "HBY must not open a message box"); + assert!(h.state().toast.is_none(), "HBY must not raise a toast"); + assert!(h.state().session.journal.is_empty(), "HBY must not journal"); } #[test] diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 22e221e69..209c55d93 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-14 · **Change:** minor (Alt, H, B border keytips apply their own preset) · **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 @@ -77,7 +77,7 @@ this logic, with one undo step. | Ctrl+1, Ctrl+; and Ctrl+Shift+; , Ctrl+D/R, Ctrl+E, Ctrl+T/L, Ctrl+K, Ctrl+Shift+L, Ctrl+Shift+~!@#$%^&_ | yes | done | | F2, F4, F5, F7, F9 family, Shift+F2/F3/F10/F11, F12 | yes | done; F11 chart sheet, F8 extend mode, F3 Paste Name, F4 Repeat missing | | Mac-specific (Cmd variants, Control variants, Fn keys) | yes | done where a feature exists (`keymap.rs` picks the Mac variant) | -| Sequential Alt access keys (Alt, H, N, …) on every ribbon tab | yes | 364-sequence keytip table for all tabs: per-control badges, prefix waiting, dropdowns open for editing (type/arrows/Enter), pending bindings for not-yet-built commands; QAT/Backstage excluded | +| Sequential Alt access keys (Alt, H, N, …) on every ribbon tab | yes | 364-sequence keytip table for all tabs: per-control badges, prefix waiting, dropdowns open for editing (type/arrows/Enter), pending bindings for not-yet-built commands; QAT/Backstage excluded. Keytips can carry fixed params, so each Borders menu letter (Alt, H, B, O/P/L/R/N/A/S/T/B/H/D/C/U) draws its own preset and B, M opens Format Cells on the Border tab; Erase Border, Line Color and Line Style are reserved | | Requests | — | #110 (more shortcuts and hot keys), #33 (sequential Alt; initial support landed in #43) | ### Shortcut audit (#110) @@ -161,6 +161,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | Alt, H, B border keytips apply the preset each letter names in Excel's Borders menu (they all drew a bottom border, so H,B,N No Border added one): keytips.json entries take optional `params`; H,B,M opens Format Cells on the Border tab; Erase Border, Line Color and Line Style stay reserved | | 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 | diff --git a/xtask/src/keytips.rs b/xtask/src/keytips.rs index 36847222f..11e7773aa 100644 --- a/xtask/src/keytips.rs +++ b/xtask/src/keytips.rs @@ -44,25 +44,35 @@ struct Entry { kind: String, target: String, pending: bool, + /// Fixed params as a compact JSON object (`params` in keytips.json), empty for none. + params: String, excel: String, note: String, } +impl Entry { + /// The probe key: a command and the params its keytip passes. + fn probe_key(&self) -> String { + if self.params.is_empty() { self.target.clone() } else { format!("{} {}", self.target, self.params) } + } +} + /// 0 = ran and journaled, 1 = ran without a journal step, 2 = errored (params -/// missing, disabled, or environment-dependent). +/// missing, disabled, or environment-dependent). Keyed by [`Entry::probe_key`]. type Probe = std::collections::HashMap; #[cfg(feature = "engine")] -fn probe_live_commands(ids: &[String]) -> Probe { +fn probe_live_commands(calls: &[(String, String)]) -> Probe { use gridcraft_engine::Session; let mut map = Probe::new(); - for id in ids { + for (id, params) in calls { if ENV_SENSITIVE.contains(&id.as_str()) { continue; } + let params = if params.is_empty() { serde_json::json!({}) } else { serde_json::from_str(params).unwrap_or_default() }; let mut s = Session::new(); s.new_workbook(); - let status = match s.execute(id, serde_json::json!({})) { + let status = match s.execute(id, params.clone()) { Ok(_) => { if s.journal.iter().any(|(jid, _)| jid == id) { 0 @@ -72,7 +82,8 @@ fn probe_live_commands(ids: &[String]) -> Probe { } Err(_) => 2, }; - map.insert(id.clone(), status); + let key = if params.as_object().is_some_and(|o| !o.is_empty()) { format!("{id} {params}") } else { id.clone() }; + map.insert(key, status); } map } @@ -132,6 +143,13 @@ fn parse_and_probe(source: &str) -> Result<(Vec, Probe), String> { if pending && kind != "command" { return Err(format!("{at}: `pending` is only valid with kind `command`")); } + let params = match e.get("params") { + None => String::new(), + Some(v) if v.is_object() && matches!(kind.as_str(), "command" | "dialog") && !pending => { + serde_json::to_string(v).map_err(|e| format!("{at}: params: {e}"))? + } + Some(_) => return Err(format!("{at}: `params` must be a JSON object, on a non-pending command or dialog")), + }; match kind.as_str() { "tab" => { if target.is_empty() || !TABS.contains(&target.as_str()) { @@ -163,6 +181,7 @@ fn parse_and_probe(source: &str) -> Result<(Vec, Probe), String> { kind, target, pending, + params, excel: e.get("excel").and_then(|v| v.as_str()).unwrap_or("").to_string(), note: e.get("note").and_then(|v| v.as_str()).unwrap_or("").to_string(), }); @@ -170,10 +189,10 @@ fn parse_and_probe(source: &str) -> Result<(Vec, Probe), String> { // JSON order is authoritative and curated: the first entry for a tab is its canonical // badge (legacy aliases come later), and groups stay together. Do not sort. - let live_ids: Vec = entries + let live_calls: Vec<(String, String)> = entries .iter() .filter(|e| e.kind == "command" && !e.pending) - .map(|e| e.target.clone()) + .map(|e| (e.target.clone(), e.params.clone())) .collect::>() .into_iter() .collect(); @@ -187,7 +206,7 @@ fn parse_and_probe(source: &str) -> Result<(Vec, Probe), String> { e.target.clone() }) .collect(); - let mut probe = probe_live_commands(&live_ids); + let mut probe = probe_live_commands(&live_calls); for id in landed { let mut s = gridcraft_engine::Session::new(); s.new_workbook(); @@ -219,7 +238,9 @@ fn render_table(entries: &[Entry]) -> String { let value = match e.kind.as_str() { "tab" => format!("Entry::Tab({:?})", e.target), "command" if e.pending => format!("Entry::Finish(Action::Pending({:?}))", e.target), + "command" if !e.params.is_empty() => format!("Entry::Finish(Action::CommandWith({:?}, {:?}))", e.target, e.params), "command" => format!("Entry::Finish(Action::Command({:?}))", e.target), + "dialog" if !e.params.is_empty() => format!("Entry::Finish(Action::DialogWith({:?}, {:?}))", e.target, e.params), "dialog" => format!("Entry::Finish(Action::Dialog({:?}))", e.target), "control" => format!("Entry::Finish(Action::Control({:?}))", e.target), "continue" => "Entry::Continue".to_string(), @@ -269,7 +290,7 @@ fn render_tests(entries: &[Entry], probe: &Probe) -> String { "sequence(&mut h, {seq:?}, false);\n assert!(h.state().dialog.is_none(), \"{seq} must not open a dialog\");\n assert!(h.state().message.is_none(), \"{seq} must not open a message box\");\n assert!(h.state().toast.is_none(), \"{seq} must not raise a toast\");\n assert!(h.state().session.journal.is_empty(), \"{seq} must not journal\");", seq = e.seq, ), - "command" => match probe.get(&e.target).copied().unwrap_or(2) { + "command" => match probe.get(&e.probe_key()).copied().unwrap_or(2) { 0 => format!( "sequence(&mut h, {seq:?}, false);\n assert!(h.state().session.journal.iter().any(|(id, _)| id == {id:?}), \"{seq} must journal {id}\");", seq = e.seq, From ab4d58ca6ffd65a8e9e71fe0470916343ea41133 Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Wed, 14 Oct 2026 18:26:06 -0500 Subject: [PATCH 02/21] Lookups compare numbers exactly, like Excel MATCH, XMATCH, VLOOKUP, HLOOKUP, XLOOKUP and LOOKUP compared numbers at 15 significant digits (through compare / compare_numbers), so a computed key such as =0.1+0.2 (0.30000000000000004) found 0.3. Excel 365 compares lookup keys exactly in exact, approximate and binary search modes, while =, SWITCH and the criteria functions (COUNTIF(S), SUMIF(S), AVERAGEIF, MAXIFS) keep the 15-digit comparison. Observed in Excel via COM with synthetic values. lookup_equal now compares numbers with ==, and the lookup searches order values with lookup_order (exact for numbers, compare otherwise). compare_numbers and the criteria path are unchanged. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +- crates/calc/src/tests.rs | 143 +++++++++++++++++++++++++++++++ crates/functions/src/criteria.rs | 9 +- crates/functions/src/lookup.rs | 63 ++++++++++++-- docs/function-parity.md | 3 +- 5 files changed, 211 insertions(+), 11 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..fba2c6a35 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-14 · **Change:** minor (lookups compare numbers exactly, like Excel) · **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-14 | Lookups (MATCH, XMATCH, VLOOKUP, HLOOKUP, XLOOKUP, LOOKUP) compare numbers exactly like Excel, so a computed key such as `=0.1+0.2` no longer finds 0.3; `=` and COUNTIF/SUMIF keep their 15-digit comparison | | 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-14 | minor | Progress log: lookups compare numbers exactly | | 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/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..4a2eb701a 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -310,6 +310,149 @@ fn date_text_coerces_in_the_workbook_date_system() { } } +/// Lookups compare numbers exactly while `=` and the criteria functions compare them at 15 +/// significant digits. Every expectation was observed in Excel 365 with these fixtures +/// (`=0.1+0.2` is 0.30000000000000004, `=100+1E-14` is 100.00000000000001). +#[test] +fn lookups_compare_numbers_exactly() { + let mut t = T::new(); + for (at, input) in [ + ("A1", "=0.1+0.2"), + ("A2", "0.3"), + ("A3", "=100+1E-14"), + ("A4", "100"), + ("A5", "=1E15+0.5"), + ("A6", "1E15"), + // Ascending 0.1, 0.3+e, 0.5 and the same descending. + ("B1", "0.1"), + ("B2", "=0.1+0.2"), + ("B3", "0.5"), + ("C1", "0.5"), + ("C2", "=0.1+0.2"), + ("C3", "0.1"), + // 0.3 next to 0.3+e, ascending and descending. + ("D1", "0.3"), + ("D2", "=0.1+0.2"), + ("D3", "0.7"), + ("E1", "0.7"), + ("E2", "=0.1+0.2"), + ("E3", "0.3"), + ("G1", "=0.1+0.2"), + ("H1", "x"), + ("G2", "row2"), + // The bisection probes 0.3 first and must not run on into 0.3+e. + ("J1", "0.1"), + ("J2", "0.2"), + ("J3", "0.3"), + ("J4", "=0.1+0.2"), + ("J5", "0.4"), + ("K1", "a"), + ("K2", "b"), + ("K3", "c"), + ("K4", "d"), + ("K5", "e"), + ] { + t.set(at, input); + } + let e = 0.1 + 0.2; + let n = Value::Number; + let s = |x: &str| Value::from(x); + let na = Value::Error(CellError::NA); + let cases: Vec<(&str, Value)> = vec![ + // `=` and friends stay at 15 digits. + ("=A1=A2", Value::Bool(true)), + ("=A3=A4", Value::Bool(true)), + ("=A5=A6", Value::Bool(true)), + ("=SWITCH(A1,0.3,\"hit\",\"miss\")", s("hit")), + // Exact match. + ("=MATCH(A1,A2:A2,0)", na.clone()), + ("=MATCH(A2,A1:A1,0)", na.clone()), + ("=VLOOKUP(A1,A2:A2,1,FALSE)", na.clone()), + ("=HLOOKUP(A1,A2,1,FALSE)", na.clone()), + ("=HLOOKUP(0.3,G1:H2,2,FALSE)", na.clone()), + ("=XLOOKUP(A1,A2:A2,A2:A2,\"nf\")", s("nf")), + ("=XMATCH(A1,A2:A2)", na.clone()), + ("=MATCH(0.1+0.2,{0.3},0)", na.clone()), + ("=MATCH(0.3,A1:A2,0)", n(2.0)), + ("=MATCH(A1,A1:A2,0)", n(1.0)), + ("=MATCH(0.3,INDEX(A1:A2,0),0)", n(2.0)), + ("=MATCH(100,A3:A3,0)", na.clone()), + ("=MATCH(1E15,A5:A5,0)", na.clone()), + ("=XLOOKUP(0.3,A1:A2,{1;2},\"nf\",0,-1)", n(2.0)), + ("=XMATCH(A1,A1:A2,0,-1)", n(1.0)), + // Approximate match against a single entry. + ("=MATCH(0.3,A1:A1,1)", na.clone()), + ("=MATCH(0.3,A1:A1,-1)", n(1.0)), + ("=LOOKUP(0.3,A1:A1)", na.clone()), + ("=LOOKUP(0.3,A1:A1,G2)", na.clone()), + ("=VLOOKUP(0.3,A1:A1,1,TRUE)", na.clone()), + ("=HLOOKUP(0.3,G1:H2,2,TRUE)", na.clone()), + ("=XLOOKUP(0.3,A1:A1,A1:A1,\"nf\",-1)", s("nf")), + ("=XLOOKUP(0.3,A1:A1,A1:A1,\"nf\",1)", n(e)), + ("=XMATCH(0.3,A1:A1,-1)", na.clone()), + ("=XMATCH(0.3,A1:A1,1)", n(1.0)), + ("=MATCH(100,A3:A3,1)", na.clone()), + ("=MATCH(A3,A4:A4,1)", n(1.0)), + // Approximate match in sorted lists. + ("=MATCH(0.3,B1:B3,1)", n(1.0)), + ("=MATCH(A1,B1:B3,1)", n(2.0)), + ("=MATCH(0.3,C1:C3,-1)", n(2.0)), + ("=MATCH(0.3,D1:D3,1)", n(1.0)), + ("=MATCH(A1,D1:D3,1)", n(2.0)), + ("=MATCH(0.3,E1:E3,-1)", n(3.0)), + ("=MATCH(A1,E1:E3,-1)", n(2.0)), + ("=MATCH(0.3,J1:J5,1)", n(3.0)), + ("=MATCH(A1,J1:J5,1)", n(4.0)), + ("=MATCH(0.3,J1:J5,0)", n(3.0)), + ("=MATCH(A1,J1:J5,0)", n(4.0)), + ("=VLOOKUP(0.3,J1:K5,2,TRUE)", s("c")), + ("=VLOOKUP(A1,J1:K5,2,TRUE)", s("d")), + ("=LOOKUP(0.3,J1:J5,K1:K5)", s("c")), + ("=LOOKUP(A1,J1:J5,K1:K5)", s("d")), + // XLOOKUP / XMATCH match and search modes. + ("=XLOOKUP(0.3,B1:B3,B1:B3,\"nf\",0,2)", s("nf")), + ("=XLOOKUP(0.3,B1:B3,B1:B3,\"nf\",-1,2)", n(0.1)), + ("=XLOOKUP(0.3,B1:B3,B1:B3,\"nf\",1,2)", n(e)), + ("=XMATCH(0.3,B1:B3,-1,2)", n(1.0)), + ("=XMATCH(0.3,B1:B3,1,2)", n(2.0)), + ("=XMATCH(0.3,C1:C3,1,-2)", n(2.0)), + ("=XMATCH(0.3,C1:C3,-1,-2)", n(3.0)), + ("=XMATCH(0.3,C1:C3,0,-2)", na.clone()), + ("=XMATCH(0.3,J1:J5,0,2)", n(3.0)), + ("=XMATCH(A1,J1:J5,0,2)", n(4.0)), + ("=XMATCH(0.3,J1:J5,1,2)", n(3.0)), + ("=XMATCH(0.3,J1:J5,-1)", n(3.0)), + ("=XMATCH(0.3,J1:J5,1)", n(3.0)), + ("=XMATCH(A1,J1:J5,-1)", n(4.0)), + ("=XMATCH(0.3,B1:B3,1)", n(2.0)), + ("=XMATCH(0.3,B1:B3,-1)", n(1.0)), + ("=XMATCH(0.3,B1:B3,0,-1)", na.clone()), + ("=XMATCH(0.3,D1:D3,1,-1)", n(1.0)), + // The criteria functions keep 15 digits. + ("=COUNTIF(A1:A1,0.3)", n(1.0)), + ("=COUNTIF(A1:A1,\"<0.3\")", n(0.0)), + ("=COUNTIF(A1:A1,\">0.3\")", n(0.0)), + ("=COUNTIF(A2:A2,A1)", n(1.0)), + ("=COUNTIFS(A1:A1,0.3)", n(1.0)), + ("=SUMIF(A1:A1,0.3)", n(e)), + ("=SUMIFS(A1:A1,A1:A1,0.3)", n(e)), + ("=AVERAGEIF(A1:A1,0.3)", n(e)), + ("=MAXIFS(A1:A1,A1:A1,0.3)", n(e)), + ("=COUNTIF(A3:A3,\">100\")", n(0.0)), + ("=COUNTIF(J1:J5,0.3)", n(2.0)), + ("=ROWS(FILTER(A1:A2,A1:A2=0.3))", n(2.0)), + ("=MATCH(TRUE,A1:A2=0.3,0)", n(1.0)), + ]; + let mut wrong = Vec::new(); + for (formula, expected) in cases { + let got = crate::evaluate(&t.wb, 0, c("Z1"), formula); + if got != expected { + wrong.push(format!("{formula}: got {got:?}, Excel {expected:?}")); + } + } + assert!(wrong.is_empty(), "{}", wrong.join("\n")); +} + #[test] fn operators() { let mut t = T::new(); diff --git a/crates/functions/src/criteria.rs b/crates/functions/src/criteria.rs index e326267ce..58f0e6594 100644 --- a/crates/functions/src/criteria.rs +++ b/crates/functions/src/criteria.rs @@ -314,7 +314,9 @@ pub(crate) fn wildcard_find(pattern: &str, text: &str, start: usize) -> Option bool { match (needle, hay) { (Value::Text(p), Value::Text(t)) => { @@ -324,7 +326,7 @@ pub(crate) fn lookup_equal(needle: &Value, hay: &Value, wild: bool) -> bool { compare_text(p, t) == Ordering::Equal } } - (Value::Number(a), Value::Number(b)) => compare_numbers(*a, *b) == Ordering::Equal, + (Value::Number(a), Value::Number(b)) => a == b, (Value::Bool(a), Value::Bool(b)) => a == b, (Value::Error(a), Value::Error(b)) => a == b, (Value::Empty, Value::Empty) => true, @@ -370,7 +372,8 @@ mod tests { assert!(m("#N/A", Value::Error(CellError::NA))); assert!(Criterion::parse(&Value::Number(3.0)).matches(&Value::Number(3.0))); assert!(Criterion::parse(&Value::Number(0.3)).matches(&Value::Number(0.1 + 0.2))); - assert!(lookup_equal(&Value::Number(0.3), &Value::Number(0.1 + 0.2), false)); + assert!(!lookup_equal(&Value::Number(0.3), &Value::Number(0.1 + 0.2), false)); + assert!(lookup_equal(&Value::Number(0.0), &Value::Number(-0.0), false)); assert!(m("<>a*", Value::from("bcd"))); assert!(m(">1/1/2020", Value::Number(44000.0))); // Text patterns never match blank cells, but an empty string is text. diff --git a/crates/functions/src/lookup.rs b/crates/functions/src/lookup.rs index 56739c933..89ea86ad7 100644 --- a/crates/functions/src/lookup.rs +++ b/crates/functions/src/lookup.rs @@ -23,6 +23,19 @@ fn needle(args: &[Arg], i: usize) -> R { } } +/// Order of two values for the approximate and binary searches: [`compare`], except that numbers +/// compare exactly. Excel's `=` treats numbers equal to 15 significant digits as equal, but its +/// lookups don't. Observed in Excel 365 with A1 holding +/// `=0.1+0.2` (0.30000000000000004): `A1=0.3` is TRUE, yet `MATCH(0.3,A1,1)` is #N/A because +/// A1 is larger, and in {0.1;0.2;0.3;A1;0.4} `MATCH(0.3,…,1)` is 3 and `MATCH(A1,…,1)` is 4. +/// The same holds for `=100+1E-14` against 100, and in every XLOOKUP / XMATCH mode. +fn lookup_order(a: &Value, b: &Value) -> Ordering { + match (a, b) { + (Value::Number(x), Value::Number(y)) => x.partial_cmp(y).unwrap_or(Ordering::Equal), + _ => compare(a, b), + } +} + fn same_type(a: &Value, b: &Value) -> bool { matches!((a, b), (Value::Number(_), Value::Number(_)) | (Value::Text(_), Value::Text(_)) | (Value::Bool(_), Value::Bool(_))) } @@ -59,10 +72,10 @@ fn bsearch_last(x: &Value, hay: &[Value], desc: bool) -> Option { continue; }; let v = hay.get(j)?; - let o = compare(v, x); + let o = lookup_order(v, x); if o == Ordering::Equal { let mut k = j; - while hay.get(k + 1).is_some_and(|w| same_type(x, w) && compare(w, x) == Ordering::Equal) { + while hay.get(k + 1).is_some_and(|w| same_type(x, w) && lookup_order(w, x) == Ordering::Equal) { k += 1; } return Some(k); @@ -88,8 +101,8 @@ fn bsearch_last(x: &Value, hay: &[Value], desc: bool) -> Option { fn binary_order(v: &Value, x: &Value, desc: bool) -> Ordering { match v { Value::Empty => Ordering::Greater, - _ if desc => compare(v, x).reverse(), - _ => compare(v, x), + _ if desc => lookup_order(v, x).reverse(), + _ => lookup_order(v, x), } } @@ -261,7 +274,7 @@ fn xsearch(x: &Value, hay: &[Value], match_mode: i64, search_mode: i64) -> R R true, Some(bv) => { - let c = compare(v, bv); + let c = lookup_order(v, bv); if match_mode == -1 { c == Ordering::Greater } else { c == Ordering::Less } } }; @@ -747,6 +760,44 @@ mod tests { assert_eq!(rows_of(&r), vec![vec![nv(3.0), nv(1.0)]]); } + /// Lookups compare numbers exactly, unlike `=` (15 significant digits). Observed in Excel 365. + #[test] + fn lookups_compare_numbers_exactly() { + let e = 0.1 + 0.2; + let one = || rf(col(&[e])); + // Exact modes miss a key one bit away. + is_err(ev("MATCH", vec![n(0.3), one(), n(0.0)]), CellError::NA); + is_err(ev("VLOOKUP", vec![n(0.3), one(), n(1.0), b(false)]), CellError::NA); + is_err(ev("XMATCH", vec![n(0.3), one()]), CellError::NA); + close(ev("MATCH", vec![n(0.3), rf(col(&[e, 0.3])), n(0.0)]), 2.0); + close(ev("MATCH", vec![n(e), rf(col(&[e, 0.3])), n(0.0)]), 1.0); + is_err(ev("MATCH", vec![n(100.0), rf(col(&[100.0 + 1e-14])), n(0.0)]), CellError::NA); + // 0 and -0 are still the same key. + close(ev("MATCH", vec![n(-0.0), rf(col(&[0.0])), n(0.0)]), 1.0); + // Approximate modes: 0.3 sorts below 0.30000000000000004. + is_err(ev("MATCH", vec![n(0.3), one(), n(1.0)]), CellError::NA); + close(ev("MATCH", vec![n(0.3), one(), n(-1.0)]), 1.0); + is_err(ev("LOOKUP", vec![n(0.3), one()]), CellError::NA); + is_err(ev("XMATCH", vec![n(0.3), one(), n(-1.0)]), CellError::NA); + close(ev("XMATCH", vec![n(0.3), one(), n(1.0)]), 1.0); + // The bisection stops on 0.3 and doesn't run on into the next entry. + let list = || rf(col(&[0.1, 0.2, 0.3, e, 0.4])); + close(ev("MATCH", vec![n(0.3), list(), n(1.0)]), 3.0); + close(ev("MATCH", vec![n(e), list(), n(1.0)]), 4.0); + close(ev("XMATCH", vec![n(0.3), list(), n(0.0), n(2.0)]), 3.0); + close(ev("XMATCH", vec![n(e), list(), n(0.0), n(2.0)]), 4.0); + let asc = || rf(col(&[0.1, e, 0.5])); + let desc = || rf(col(&[0.5, e, 0.1])); + close(ev("MATCH", vec![n(0.3), asc(), n(1.0)]), 1.0); + close(ev("MATCH", vec![n(0.3), desc(), n(-1.0)]), 2.0); + is_err(ev("XMATCH", vec![n(0.3), asc(), n(0.0), n(2.0)]), CellError::NA); + close(ev("XMATCH", vec![n(0.3), asc(), n(-1.0), n(2.0)]), 1.0); + close(ev("XMATCH", vec![n(0.3), asc(), n(1.0), n(2.0)]), 2.0); + close(ev("XMATCH", vec![n(0.3), desc(), n(-1.0), n(-2.0)]), 3.0); + close(ev("XMATCH", vec![n(0.3), asc(), n(-1.0)]), 1.0); + close(ev("XMATCH", vec![n(0.3), asc(), n(1.0)]), 2.0); + } + #[test] fn index() { let t3 = table_rows3(); diff --git a/docs/function-parity.md b/docs/function-parity.md index 760ffd6ac..6dad80804 100644 --- a/docs/function-parity.md +++ b/docs/function-parity.md @@ -1,6 +1,6 @@ # Function parity: worksheet functions and formulas -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (USDOLLAR added as a compatibility function; MIRR, COUNTIF and INDIRECT edge cases match Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** minor (lookups compare numbers exactly, like Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Excel's worksheet functions, category by category, against GridCraft's function library. Part of [target-app-parity.md](target-app-parity.md); gaps feed [gaps.md](gaps.md). @@ -108,6 +108,7 @@ workbooks; results checked in as data, not Excel files) and fixing what it finds | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | MATCH, XMATCH, VLOOKUP, HLOOKUP, XLOOKUP and LOOKUP compare numbers exactly in every match and search mode, as Excel 365 does: `MATCH(0.1+0.2,{0.3},0)` is `#N/A` and 0.3 sorts below 0.30000000000000004 in approximate and binary search. `=`, SWITCH and the criteria functions (COUNTIF(S), SUMIF(S), AVERAGEIF(S), MAXIFS…) keep comparing at 15 significant digits, also as observed | | 2026-10-12 | minor | USDOLLAR added as a Compatibility function (not on Microsoft's list, so the counts don't change); MIRR with no positive flow is -1, COUNTIF's bare `"<="` / `">="` skip empty strings, INDIRECT with `a1` = FALSE gives `#REF!` for A1-style text (#738) | | 2026-10-11 | minor | TEXT / number formats fixed against Excel-cached results: fraction literals and zero fractions, `0` denominators zero padded, `?` pads the thousands separator, booleans use the text section (226 mismatches → 0 in 352 TEXT cells) | | 2026-10-11 | minor | Iterative calculation: circular references iterate up to Maximum Iterations / Maximum Change when enabled | From 5c20f164342f0b443fb7e6897f52f6e0b3934163 Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Wed, 14 Oct 2026 18:30:16 -0500 Subject: [PATCH 03/21] core: read accounting negatives like $(1,234) in number text Excel 365 reads a dollar sign before accounting parentheses ($(1,234), $ (5)) and a percent sign after them ((5)%) as negative numbers when the text is typed, coerced by arithmetic or passed to VALUE. GridCraft gave #VALUE! for these. The parentheses branch no longer recurses: Excel reads only an unsigned plain number or fraction inside, so (-5), ((5)), ($-5), and dates or times in parentheses are now #VALUE! as in Excel (they used to give 5 or a negative serial). $5% is #VALUE! too: Excel never combines the currency and percent signs. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +- crates/calc/src/tests.rs | 15 +++++ crates/core/src/parse.rs | 115 ++++++++++++++++++++++++++++++++++- crates/functions/src/text.rs | 7 +++ docs/function-parity.md | 3 +- 5 files changed, 139 insertions(+), 5 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..610011d87 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-14 · **Change:** minor (accounting negatives such as `$(1,234)` read as numbers) · **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-14 | Accounting negatives in text: `$(1,234)`, `$ (5)` and `(5)%` read as negative numbers when typed, coerced by arithmetic or passed to VALUE, like Excel 365; `(-5)`, `((5))`, `$5%` and dates or times in parentheses stay text | | 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-14 | minor | Progress log: accounting negatives in number text | | 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/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..1d5a1a464 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -310,6 +310,21 @@ fn date_text_coerces_in_the_workbook_date_system() { } } +#[test] +fn accounting_negative_text_coerces() { + let mut t = T::new(); + t.set("A1", "'$(1,234)"); + t.set("B1", "=A1+0"); + t.set("C1", "=VALUE(A1)"); + t.set("D1", "=--\"$ (5)\""); + t.set("E1", "=\"(5)%\"*100"); + t.set("F1", "=\"(-5)\"+0"); + t.set("G1", "$(1,234)"); + assert_eq!((t.num("B1"), t.num("C1"), t.num("D1"), t.num("E1")), (-1234.0, -1234.0, -5.0, -5.0)); + assert_eq!(t.get("F1"), Value::Error(CellError::Value)); + assert_eq!(t.num("G1"), -1234.0); +} + #[test] fn operators() { let mut t = T::new(); diff --git a/crates/core/src/parse.rs b/crates/core/src/parse.rs index b465beaf9..bf8487622 100644 --- a/crates/core/src/parse.rs +++ b/crates/core/src/parse.rs @@ -54,8 +54,13 @@ fn parse_numeric(t: &str, sys: DateSystem) -> Option<(f64, Option<&'static str>) return None; } // Parenthesised negatives: (100) = -100. - if let Some(inner) = t.strip_prefix('(').and_then(|x| x.strip_suffix(')')) { - return parse_numeric(inner.trim(), sys).map(|(n, f)| (-n, f)); + if let Some(inner) = parenthesized(t) { + // Excel reads no sign and no nested parentheses inside: `(-5)`, `($-5)`, `((5))`. + if inner.strip_prefix('$').map_or(inner.as_str(), str::trim_start).starts_with(['-', '+', '(']) { + return None; + } + let r = parse_plain(&inner).or_else(|| parse_fraction(&inner).map(|n| (n, Some("# ?/?")))); + return r.map(|(n, f)| (-n, f)); } if let Some(r) = parse_plain(t) { return Some(r); @@ -69,6 +74,22 @@ fn parse_numeric(t: &str, sys: DateSystem) -> Option<(f64, Option<&'static str>) None } +/// The number inside accounting parentheses, as plain text: `(5)` gives `5`. A dollar sign may +/// stand before them (`$(5)` reads as `($5)`) and a percent sign after them (`(5)%` reads as +/// `(5%)`), with spaces around either. The caller reads only a plain number or a fraction from it. +fn parenthesized(t: &str) -> Option { + let (currency, rest) = match t.strip_prefix('$') { + Some(r) => ("$", r.trim_start()), + None => ("", t), + }; + let rest = rest.strip_prefix('(')?; + let (body, percent) = match rest.strip_suffix(')') { + Some(b) => (b, ""), + None => (rest.strip_suffix('%')?.trim_end().strip_suffix(')')?, "%"), + }; + Some(format!("{currency}{}{percent}", body.trim())) +} + /// Signs, currency, thousands separators, percent, exponent. fn parse_plain(t: &str) -> Option<(f64, Option<&'static str>)> { let mut s = t; @@ -93,7 +114,8 @@ fn parse_plain(t: &str) -> Option<(f64, Option<&'static str>)> { percent = true; s = r.trim_end(); } - if s.is_empty() { + // Excel reads `$5` and `5%` but not `$5%`. + if s.is_empty() || currency && percent { return None; } let has_sep = s.contains(','); @@ -352,6 +374,93 @@ mod tests { assert_eq!(p(".5").value, Value::Number(0.5)); } + /// Observed in Excel 365 (en-US): typed entry, VALUE and `--"text"` agree. + #[test] + fn accounting_parentheses() { + let accepted: [(&str, f64, Option<&str>); 22] = [ + ("$(5)", -5.0, Some("\"$\"#,##0")), + ("$ (5)", -5.0, Some("\"$\"#,##0")), + ("$( 5 )", -5.0, Some("\"$\"#,##0")), + ("$ ( 1,234 )", -1234.0, Some("\"$\"#,##0")), + ("$(1,234)", -1234.0, Some("\"$\"#,##0")), + ("$(1,000.50)", -1000.5, Some("\"$\"#,##0.00")), + ("$(1,234.)", -1234.0, Some("\"$\"#,##0.00")), + ("$(.5)", -0.5, Some("\"$\"#,##0.00")), + ("$(0)", 0.0, Some("\"$\"#,##0")), + (" $(5) ", -5.0, Some("\"$\"#,##0")), + ("($5)", -5.0, Some("\"$\"#,##0")), + ("( $ 1,234 )", -1234.0, Some("\"$\"#,##0")), + ("(5)", -5.0, None), + ("( 5 )", -5.0, None), + ("(1,234.5)", -1234.5, Some("#,##0.00")), + ("(1e-3)", -0.001, Some("0.00E+00")), + ("(5)%", -0.05, Some("0%")), + ("( 5 ) %", -0.05, Some("0%")), + ("(5%)", -0.05, Some("0%")), + ("(5.)%", -0.05, Some("0.00%")), + ("(1,234)%", -12.34, Some("0%")), + ("(1 1/2)", -1.5, Some("# ?/?")), + ]; + for (s, n, f) in accepted { + assert_eq!(p(s), Parsed { value: Value::Number(n), format: f }, "{s}"); + assert_eq!(parse_number_text(s), Some(n), "{s}"); + } + for s in [ + "$5%", + "$(5%)", + "$(5)%", + "($5%)", + "($5)%", + "$ ( 5 ) %", + "(5)%%", + "(5%)%", + "((5))", + "(-5)", + "(+5)", + "($-5)", + "($ -5)", + "($+5)", + "(-$5)", + "$(-5)", + "$ (-5)", + "$(+5)", + "-(5)", + "-$(5)", + "+$(5)", + "$+(5)", + "$$(5)", + "$ $(5)", + "$((5))", + "($(5))", + "(-1 1/2)", + "()", + "$()", + "(5)$", + "$(5)$", + "(5)(5)", + "$(5)0", + "(5)e3", + "(5)-", + "(5-)", + "$(5-)", + "$(5", + "$5)", + "(5", + "$(1,23)", + "$(,5)", + "(5e)", + "(1/2)", + "$(1/2)", + "(3/4/2026)", + "$(3/4/2026)", + "(10:30)", + "$(10:30)", + ] { + assert_eq!(p(s).value, Value::text(s), "{s}"); + assert_eq!(parse_number_text(s), None, "{s}"); + } + } + #[test] fn texts_and_specials() { assert_eq!(p("hello").value, Value::text("hello")); diff --git a/crates/functions/src/text.rs b/crates/functions/src/text.rs index 91962d6e7..84fad9898 100644 --- a/crates/functions/src/text.rs +++ b/crates/functions/src/text.rs @@ -1092,6 +1092,13 @@ mod tests { close(ev("VALUE", vec![t("12:00")]), 0.5); close(ev("VALUE", vec![t("2020-01-01")]), 43831.0); is_err(ev("VALUE", vec![t("abc")]), CellError::Value); + // Accounting negatives, as Excel 365 reads them. + close(ev("VALUE", vec![t("$(5)")]), -5.0); + close(ev("VALUE", vec![t("$ (1,000.50)")]), -1000.5); + close(ev("VALUE", vec![t("(5)%")]), -0.05); + for s in ["$(5)%", "$(5%)", "$5%", "(-5)", "((5))", "$(-5)", "(1/2)"] { + is_err(ev("VALUE", vec![t(s)]), CellError::Value); + } close(ev("NUMBERVALUE", vec![t("2.500,27"), t(","), t(".")]), 2500.27); close(ev("NUMBERVALUE", vec![t("3.5%")]), 0.035); close(ev("NUMBERVALUE", vec![t(" 1 000 ")]), 1000.0); diff --git a/docs/function-parity.md b/docs/function-parity.md index 760ffd6ac..3b25b144f 100644 --- a/docs/function-parity.md +++ b/docs/function-parity.md @@ -1,6 +1,6 @@ # Function parity: worksheet functions and formulas -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (USDOLLAR added as a compatibility function; MIRR, COUNTIF and INDIRECT edge cases match Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** minor (number text with accounting parentheses coerces like Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Excel's worksheet functions, category by category, against GridCraft's function library. Part of [target-app-parity.md](target-app-parity.md); gaps feed [gaps.md](gaps.md). @@ -108,6 +108,7 @@ workbooks; results checked in as data, not Excel files) and fixing what it finds | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | Number text coercion (VALUE, arithmetic, typed entry): a dollar sign before accounting parentheses (`$(1,234)`, `$ (5)`) and a percent sign after them (`(5)%`) read as negatives; `(-5)`, `((5))`, `($-5)`, `$5%`, `$(5)%` and dates or times in parentheses are `#VALUE!`, as observed in Excel 365 | | 2026-10-12 | minor | USDOLLAR added as a Compatibility function (not on Microsoft's list, so the counts don't change); MIRR with no positive flow is -1, COUNTIF's bare `"<="` / `">="` skip empty strings, INDIRECT with `a1` = FALSE gives `#REF!` for A1-style text (#738) | | 2026-10-11 | minor | TEXT / number formats fixed against Excel-cached results: fraction literals and zero fractions, `0` denominators zero padded, `?` pads the thousands separator, booleans use the text section (226 mismatches → 0 in 352 TEXT cells) | | 2026-10-11 | minor | Iterative calculation: circular references iterate up to Maximum Iterations / Maximum Change when enabled | From ea190adad6a1edb792521a7818ef426570a8229c Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Wed, 14 Oct 2026 19:55:46 -0500 Subject: [PATCH 04/21] functions: Analysis ToolPak value arguments reject multi-cell references EOMONTH, EDATE, WORKDAY(.INTL), NETWORKDAYS(.INTL), YEARFRAC, WEEKNUM, the coupon, bond and T-bill functions, DISC, BIN2DEC, IM* and the other Analysis ToolPak functions give #VALUE! in Excel 365 for a reference to more than one cell in a value argument, in dynamic-array and legacy formulas alike, where GridCraft lifted the range and spilled. Array constants and computed arrays still lift, and range arguments (holidays, cash flows, schedules) and the .INTL weekend still take ranges. FnSpec gains a value_args mask, filled at registration from one table in lift.rs; call_lifted turns a multi-cell reference at a value position into a scalar #VALUE! before lifting, so EOMONTH(A1:A3,{1;2}) is a 2x1 array of #VALUE! as in Excel. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +- crates/calc/src/tests.rs | 85 +++++++++++++++++++++++++++++++++++ crates/functions/src/lib.rs | 18 ++++++++ crates/functions/src/lift.rs | 87 ++++++++++++++++++++++++++++++++++++ docs/function-parity.md | 4 +- 5 files changed, 196 insertions(+), 2 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..2abac7262 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-14 · **Change:** minor (Analysis ToolPak functions reject multi-cell references in value arguments) · **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-14 | Functions: EOMONTH, EDATE, WORKDAY, NETWORKDAYS, YEARFRAC, WEEKNUM, the bond and coupon functions, DISC, BIN2DEC, IM* and the other Analysis ToolPak functions give `#VALUE!` for a reference to more than one cell in a value argument, as Excel 365 does, instead of spilling; array constants and computed arrays still lift | | 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-14 | minor | Progress log: Analysis ToolPak value arguments reject multi-cell references | | 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/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..a9317528b 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -1266,3 +1266,88 @@ fn offset_extreme_arguments_return_ref() { assert_eq!(crate::evaluate(&wb, 0, c("D1"), formula), Value::Number(expected), "{formula}"); } } + +#[test] +fn analysis_toolpak_value_params_reject_multi_cell_references() { + // Excel 365 (observed): EOMONTH, WORKDAY, COUPDAYS, BIN2DEC... give #VALUE! for a reference to more + // than one cell, however the reference is formed, and lift array constants and computed arrays. + let mut t = T::new(); + for (a, v) in [ + ("A1", "45000"), + ("A2", "45031"), + ("A3", "45061"), + ("B1", "1"), + ("B2", "2"), + ("B3", "3"), + ("C2", "7"), + ("F1", "1"), + ("F2", "2"), + ("F3", "11"), + ] { + t.set(a, v); + } + t.wb.names.push(gridcraft_model::DefinedName { + name: "Dates".into(), + scope: None, + formula: "Sheet1!$A$1:$A$3".into(), + comment: String::new(), + hidden: false, + }); + let value = Value::Error(CellError::Value); + for f in [ + "=EOMONTH(A1:A3,1)", + "=EOMONTH(A1,B1:B3)", + "=EDATE(A1:A3,1)", + "=SUMPRODUCT(EOMONTH(A1:A3,0))", + "=EOMONTH(INDEX(A1:A3,0,1),0)", + "=EOMONTH(OFFSET(A1,0,0,3,1),0)", + "=EOMONTH(IF(TRUE,A1:A3),0)", + "=EOMONTH(CHOOSE(1,A1:A3),0)", + "=LET(x,A1:A3,EOMONTH(x,0))", + "=EOMONTH(Dates,0)", + "=EOMONTH((A1,A2),0)", + "=EOMONTH(Z:Z,0)", + "=EOMONTH(A1:INDEX(A1:A3,3),0)", + "=WORKDAY(A1:A3,1)", + "=NETWORKDAYS(A1,A3:A4)", + "=YEARFRAC(A1,A3,B1:B2)", + "=WEEKNUM(A1,B1:B3)", + "=COUPDAYS(A1:A3,46000,2,0)", + "=DISC(A1,46000,98,100,B1:B3)", + "=BIN2DEC(B1:B3)", + "=ISEVEN(B1:B3)", + ] { + t.set("H5", f); + assert_eq!((t.get("H5"), t.get("H6")), (value.clone(), Value::Empty), "{f}"); + t.set("H5", ""); + } + // One-cell references, intersections, @ and the range parameters are fine. + for (f, want) in [ + ("=EOMONTH(A1:A1,0)", 45016.0), + ("=EOMONTH(INDEX(A1:A3,2),0)", 45046.0), + ("=EOMONTH(OFFSET(A1,0,0,1,1),0)", 45016.0), + ("=EOMONTH(A1:A3 A2:C2,0)", 45046.0), + ("=EOMONTH(@A1:A3,0)", 45046.0), + ("=WORKDAY(A1,1,A1:A3)", 45001.0), + ("=XNPV(0,B1:B3,A1:A3)", 6.0), + ] { + t.set("H2", f); + assert_eq!(t.num("H2"), want, "{f}"); + t.set("H2", ""); + } + // Arrays that aren't references lift; so does the WORKDAY.INTL/NETWORKDAYS.INTL weekend. + for (f, below, right) in [ + ("=EOMONTH(A1:A3+0,0)", Value::Number(45046.0), Value::Empty), + ("=EOMONTH(A1,{1,2})", Value::Empty, Value::Number(45077.0)), + ("=EOMONTH(TRANSPOSE(A1:A3),0)", Value::Empty, Value::Number(45046.0)), + ("=MAP(A1:A3,LAMBDA(x,EOMONTH(x,0)))", Value::Number(45046.0), Value::Empty), + ("=NETWORKDAYS.INTL(A1,A3+30,F1:F3)", Value::Number(66.0), Value::Empty), + ("=ISOWEEKNUM(A1:A3)", Value::Number(15.0), Value::Empty), + // The rejected reference adds no shape: a 2x1 array of #VALUE!. + ("=EOMONTH(A1:A3,{1;2})", value.clone(), Value::Empty), + ] { + t.set("H5", f); + assert_eq!((t.get("H6"), t.get("I5")), (below, right), "{f}"); + t.set("H5", ""); + } +} diff --git a/crates/functions/src/lib.rs b/crates/functions/src/lib.rs index 148d68221..5f9dcb67d 100644 --- a/crates/functions/src/lib.rs +++ b/crates/functions/src/lib.rs @@ -30,6 +30,7 @@ macro_rules! f { category: $crate::Category::$cat, volatile: true, scalar_args: $scal, + value_args: &[], signature: $sig, description: $desc, imp: { @@ -48,6 +49,7 @@ macro_rules! f { category: $crate::Category::$cat, volatile: false, scalar_args: $scal, + value_args: &[], signature: $sig, description: $desc, imp: { @@ -147,6 +149,11 @@ pub struct FnSpec { /// (broadcast like Excel's dynamic arrays). The last entry repeats for variadic tails; an /// empty slice means no lifting. pub scalar_args: &'static [bool], + /// Parameter positions that take a single value and reject a reference to more than one + /// cell with `#VALUE!` (the Analysis ToolPak functions: EOMONTH, WORKDAY, COUPDAYS, BIN2DEC…), + /// where other scalar parameters lift the range. Arrays that aren't references still lift. + /// Same layout as `scalar_args`; filled at registration from [`lift::value_args`]. + pub value_args: &'static [bool], pub signature: &'static str, pub description: &'static str, pub imp: FnImpl, @@ -167,6 +174,14 @@ impl FnSpec { } } + /// Whether parameter `i` takes a single value, so a multi-cell reference there is `#VALUE!`. + pub fn is_value_arg(&self, i: usize) -> bool { + match self.value_args.get(i) { + Some(b) => *b, + None => self.value_args.last().copied().unwrap_or(false), + } + } + /// Whether a call with `n` arguments is valid (Excel allows at most 255). pub fn accepts(&self, n: usize) -> bool { (self.min_args..=self.max_args).contains(&n) && n <= 255 @@ -195,6 +210,9 @@ fn registry() -> &'static (Vec, HashMap<&'static str, usize>) { ] { v.extend(list); } + for s in &mut v { + s.value_args = lift::value_args(s.name); + } let mut map = HashMap::with_capacity(v.len()); for (i, s) in v.iter().enumerate() { map.entry(s.name).or_insert(i); diff --git a/crates/functions/src/lift.rs b/crates/functions/src/lift.rs index cea0067ee..056cb6d31 100644 --- a/crates/functions/src/lift.rs +++ b/crates/functions/src/lift.rs @@ -6,7 +6,61 @@ use gridcraft_core::{Array, CellError, Value}; use crate::util::MAX_CELLS; use crate::{Arg, Ctx, FnSpec}; +/// Value-parameter masks (see [`FnSpec::value_args`]), observed in Excel 365: a reference to more +/// than one cell at these positions gives `#VALUE!` in dynamic-array and legacy formulas alike (no +/// implicit intersection), while array constants and computed arrays lift. Range parameters +/// (holidays, schedules, cash flows, coefficients) and the WORKDAY.INTL/NETWORKDAYS.INTL weekend +/// (which lifts a range) are `false`. +pub(crate) const VALUE_ARGS: &[(&str, &[bool])] = &[ + // Date and time. + ("EOMONTH EDATE YEARFRAC WEEKNUM", &[true]), + ("WORKDAY NETWORKDAYS", &[true, true, false]), + ("WORKDAY.INTL NETWORKDAYS.INTL", &[true, true, false, false]), + // Financial. + ( + "ACCRINT ACCRINTM AMORDEGRC AMORLINC COUPDAYBS COUPDAYS COUPDAYSNC COUPNCD COUPNUM COUPPCD CUMIPMT CUMPRINC DISC \ + DOLLARDE DOLLARFR DURATION EFFECT INTRATE MDURATION NOMINAL ODDFPRICE ODDFYIELD ODDLPRICE ODDLYIELD PRICE PRICEDISC \ + PRICEMAT RECEIVED TBILLEQ TBILLPRICE TBILLYIELD YIELD YIELDDISC YIELDMAT", + &[true], + ), + ("FVSCHEDULE XNPV", &[true, false]), + ("XIRR", &[false, false, true]), + // Math and information. + ("ISEVEN ISODD QUOTIENT MROUND FACTDOUBLE SQRTPI RANDBETWEEN", &[true]), + ("SERIESSUM", &[true, true, true, false]), + // Engineering (IMSUM and IMPRODUCT aggregate, so they aren't here). + ( + "BIN2DEC BIN2HEX BIN2OCT DEC2BIN DEC2HEX DEC2OCT HEX2BIN HEX2DEC HEX2OCT OCT2BIN OCT2DEC OCT2HEX CONVERT DELTA GESTEP \ + ERF ERF.PRECISE ERFC ERFC.PRECISE BESSELI BESSELJ BESSELK BESSELY COMPLEX IMABS IMAGINARY IMARGUMENT IMCONJUGATE \ + IMCOS IMCOSH IMCOT IMCSC IMCSCH IMDIV IMEXP IMLN IMLOG10 IMLOG2 IMPOWER IMREAL IMSEC IMSECH IMSIN IMSINH IMSQRT IMSUB \ + IMTAN", + &[true], + ), +]; + +/// The value-parameter mask of `name` (empty for functions without value parameters). +pub(crate) fn value_args(name: &str) -> &'static [bool] { + VALUE_ARGS.iter().find(|(names, _)| names.split_ascii_whitespace().any(|n| n == name)).map_or(&[], |(_, mask)| mask) +} + +/// A reference to more than one cell at a value parameter becomes a scalar `#VALUE!`, so it adds +/// no shape: `EOMONTH(A1:A3, {1;2})` is a 2x1 array of `#VALUE!`, as in Excel. +fn reject_multi_cell_refs(spec: &FnSpec, args: &[Arg]) -> Option> { + let multi = |i: usize, a: &Arg| a.from_ref && spec.is_value_arg(i) && matches!(&a.value, Value::Array(arr) if arr.data.len() > 1); + if !args.iter().enumerate().any(|(i, a)| multi(i, a)) { + return None; + } + Some( + args.iter() + .enumerate() + .map(|(i, a)| if multi(i, a) { Arg { value: Value::Error(CellError::Value), from_ref: false } } else { a.clone() }) + .collect(), + ) +} + pub(crate) fn call_lifted(spec: &FnSpec, args: &[Arg], ctx: &mut dyn Ctx) -> Value { + let rejected = if spec.value_args.is_empty() { None } else { reject_multi_cell_refs(spec, args) }; + let args = rejected.as_deref().unwrap_or(args); let mut rows = 0usize; let mut cols = 0usize; let mut any = false; @@ -94,4 +148,37 @@ mod tests { ); is_err(huge, CellError::Num); } + + #[test] + fn value_params_reject_multi_cell_references() { + let s = FnSpec { value_args: &[true, false], ..spec() }; + let mut ctx = TestCtx::default(); + // A multi-cell reference is #VALUE!; a one-cell reference and a computed array are fine. + is_err(crate::call(&s, &[rf(col(&[1.0, 2.0, 3.0])), n(1.0)], &mut ctx), CellError::Value); + assert_eq!(crate::call(&s, &[rf(col(&[4.0])), n(1.0)], &mut ctx), nv(5.0)); + assert_eq!(rows_of(&crate::call(&s, &[av(col(&[1.0, 2.0])), n(1.0)], &mut ctx)), vec![vec![nv(2.0)], vec![nv(3.0)]]); + // The rejected reference adds no shape: the other argument still lifts. + let v = Value::Error(CellError::Value); + assert_eq!(rows_of(&crate::call(&s, &[rf(col(&[1.0, 2.0, 3.0])), av(col(&[1.0, 2.0]))], &mut ctx)), vec![vec![v.clone()], vec![v]]); + // A position that isn't a value parameter still lifts the reference. + assert_eq!(rows_of(&crate::call(&s, &[n(1.0), rf(row(&[1.0, 2.0]))], &mut ctx)), vec![vec![nv(2.0), nv(3.0)]]); + } + + #[test] + fn value_arg_table_names_registered_scalar_positions() { + for (names, mask) in super::VALUE_ARGS { + assert!(!mask.is_empty()); + for name in names.split_ascii_whitespace() { + let Some(s) = crate::lookup(name) else { panic!("{name} isn't registered") }; + assert_eq!(s.value_args, *mask, "{name}"); + for i in 0..s.max_args.min(12) { + assert!(!s.is_value_arg(i) || s.is_scalar_arg(i), "{name} argument {i} is a value parameter but doesn't lift"); + } + } + } + assert!(crate::lookup("WORKDAY").is_some_and(|s| s.is_value_arg(1) && !s.is_value_arg(2))); + assert!(crate::lookup("WORKDAY.INTL").is_some_and(|s| !s.is_value_arg(2) && s.is_scalar_arg(2))); + assert!(crate::lookup("SUM").is_some_and(|s| s.value_args.is_empty())); + assert!(crate::lookup("ISOWEEKNUM").is_some_and(|s| s.value_args.is_empty())); + } } diff --git a/docs/function-parity.md b/docs/function-parity.md index 760ffd6ac..91283721d 100644 --- a/docs/function-parity.md +++ b/docs/function-parity.md @@ -1,6 +1,6 @@ # Function parity: worksheet functions and formulas -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (USDOLLAR added as a compatibility function; MIRR, COUNTIF and INDIRECT edge cases match Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** minor (Analysis ToolPak functions give #VALUE! for a multi-cell reference in a value argument, like Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Excel's worksheet functions, category by category, against GridCraft's function library. Part of [target-app-parity.md](target-app-parity.md); gaps feed [gaps.md](gaps.md). @@ -77,6 +77,7 @@ PIVOTBY, REGEXTEST…) are all present. | Defined names (workbook and sheet scope), Create from Selection | done | | Dynamic arrays, spill, `#SPILL!`, spill operator `A1#` | done; stale-spill bugs (#133, #139; PRs #148, #157) | | Implicit intersection `@`, legacy CSE array formulas | done; legacy array range fill (PR #156) | +| Array lifting: an array in a scalar argument evaluates the function per element | done. The Analysis ToolPak functions (EOMONTH, EDATE, WORKDAY(.INTL), NETWORKDAYS(.INTL), YEARFRAC, WEEKNUM, the COUP*, PRICE*, YIELD*, ODD*, TBILL*, ACCRINT*, AMOR*, DISC, DURATION, CUMIPMT/CUMPRINC, EFFECT, NOMINAL, DOLLARDE/FR, XNPV/XIRR rate and guess, FVSCHEDULE principal, ISEVEN/ISODD, QUOTIENT, MROUND, FACTDOUBLE, SQRTPI, RANDBETWEEN, SERIESSUM, the base conversions, CONVERT, DELTA, GESTEP, ERF/ERFC, BESSEL*, COMPLEX and IM* except IMSUM/IMPRODUCT) give `#VALUE!` for a reference to more than one cell in a value argument, as Excel 365 does in both dynamic-array and legacy formulas; array constants and computed arrays (`A1:A3+0`) still lift, and range arguments (holidays, schedule, cash flows) and the .INTL weekend accept ranges. Difference: BYROW/BYCOL pass their row or column to the LAMBDA as values, so `BYROW(A1:B3,LAMBDA(r,EOMONTH(r,0)))` lifts where Excel gives `#VALUE!` | | Trim references `A1:.E10` (trailing), `A1.:E10` (leading), `A1.:.E10` (both), also on whole rows/columns (`A:.A`) | done; evaluated as `TRIMRANGE(range, code, code)`, adjusted on insert/delete; files store `_xlfn.TRIMRANGE(range,code,code)` (Excel's own file spelling unconfirmed in public docs), `_xlfn._TRO_ALL/_TRO_LEADING/_TRO_TRAILING(...)` read too | | LET, LAMBDA, named LAMBDAs, ISOMITTED, MAP/REDUCE/SCAN/BYROW/BYCOL/MAKEARRAY | done | | Iterative calculation for circular references | done: with Enable Iterative Calculation on (`formulas.calculationOptions` `{iterative, maxIterations, maxChange}`, saved as ``), loops are evaluated again from their previous values (blank = 0) until no cell on a loop changes by Maximum Change or Maximum Iterations is reached; off, cycles give `#CIRC!`. Differences: no Options dialog yet (command only); GridCraft recalculates on open, so a loop that never settles (`=A1+1`) advances again each time the file is opened | @@ -108,6 +109,7 @@ workbooks; results checked in as data, not Excel files) and fixing what it finds | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | Analysis ToolPak value arguments (EOMONTH, WORKDAY, YEARFRAC, COUPDAYS, DISC, BIN2DEC, IM* and the rest, observed in Excel 365) give `#VALUE!` for a multi-cell reference instead of lifting it | | 2026-10-12 | minor | USDOLLAR added as a Compatibility function (not on Microsoft's list, so the counts don't change); MIRR with no positive flow is -1, COUNTIF's bare `"<="` / `">="` skip empty strings, INDIRECT with `a1` = FALSE gives `#REF!` for A1-style text (#738) | | 2026-10-11 | minor | TEXT / number formats fixed against Excel-cached results: fraction literals and zero fractions, `0` denominators zero padded, `?` pads the thousands separator, booleans use the text section (226 mismatches → 0 in 352 TEXT cells) | | 2026-10-11 | minor | Iterative calculation: circular references iterate up to Maximum Iterations / Maximum Change when enabled | From 6db94d154a14697cb8a0d3b85c77f8a5c4c33efb Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Thu, 15 Oct 2026 05:19:32 -0500 Subject: [PATCH 05/21] Calc: INDEX with one index on a 2-D reference is #REF!, as in Excel INDEX(A1:C3,2) returned the whole second row. Excel gives #REF! when a reference has several rows and columns and the column argument is omitted, so the error also carries into ranges built from it (A1:INDEX(A1:C3,2)), position arrays (INDEX(A1:C3,{1;2})) and outer functions. An empty column argument (INDEX(A1:C3,2,)) still gives the row, and arrays are unchanged: INDEX({1,2;3,4},2) is the row. Observed in Excel that a column array with one index, INDEX({10;20;30},2), is a 1x1 array (TYPE 64), which is what calc already returned; the new test pins it. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 ++- crates/calc/src/eval.rs | 8 ++++- crates/calc/src/tests.rs | 75 ++++++++++++++++++++++++++++++++++++++++ docs/function-parity.md | 3 +- 4 files changed, 87 insertions(+), 3 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..3292b595f 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-15 · **Change:** minor (INDEX with one index on a 2-D reference is #REF! as in Excel) · **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-15 | Calc: `INDEX(A1:C3,2)` (one index on a reference of several rows and columns) is `#REF!` as in Excel, instead of the whole row; `INDEX(A1:C3,2,)` still gives the row | | 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-15 | minor | Progress log: INDEX with one index on a 2-D reference is #REF! | | 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/calc/src/eval.rs b/crates/calc/src/eval.rs index 0f9cfb03d..1f58c50be 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -1251,7 +1251,13 @@ impl<'h> Evaluator<'h> { let area = usize::try_from((area_n as u64).saturating_sub(1)).ok(); let Some(a) = area.and_then(|i| areas.get(i)).copied() else { return err(CellError::Ref) }; let r = a.range; - // A single row or column with one index picks along it. + // A single row or column with one index picks along it. An area with several rows + // and columns needs both: `INDEX(A1:C3,2)` is #REF! in Excel (an empty third + // argument, `INDEX(A1:C3,2,)`, still means the whole row). Arrays differ: + // `INDEX({1,2;3,4},2)` is the row. + if two_args && r.height() > 1 && r.width() > 1 { + return err(CellError::Ref); + } let (row, col) = if two_args && r.height() == 1 { (1, row) } else { (row, col) }; if row as u64 > r.height() as u64 || col as u64 > r.width() as u64 { return err(CellError::Ref); diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..5abedec60 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -172,6 +172,81 @@ fn index_area_number_overflow() { } } +/// INDEX with one index, as observed in Excel: a 2-D reference needs both indexes (#REF!), a +/// column array gives a 1×1 array (TYPE 64), a row array or 1-D reference gives one value. +#[test] +fn index_with_one_index() { + let mut t = T::new(); + for (i, v) in [10, 20, 30].iter().enumerate() { + t.set(&format!("A{}", i + 1), &v.to_string()); + } + let mut k = 1; + for r in 5..=7 { + for col in ["A", "B", "C"] { + t.set(&format!("{col}{r}"), &k.to_string()); + k += 1; + } + } + for (row, (formula, expected)) in [ + // Column arrays keep Excel's 1×1 array; row arrays and explicit columns give a value. + ("=INDEX({10;20;30},2)", 20.0), + ("=TYPE(INDEX({10;20;30},2))", 64.0), + ("=TYPE(INDEX(SEQUENCE(3),2))", 64.0), + ("=TYPE(INDEX({10;20;30},2,1))", 1.0), + ("=TYPE(INDEX({10,20,30},2))", 1.0), + ("=TYPE(INDEX(INDEX({10;20;30},2),1))", 1.0), + ("=SUM(INDEX({10;20;30},2))", 20.0), + // 1-D references with one index are single-cell references. + ("=TYPE(INDEX(A1:A3,2))", 1.0), + ("=SUM(INDEX(A1:A3,1):INDEX(A1:A3,3))", 60.0), + ("=SUM(INDEX((A1:A3,A5:C7),2))", 20.0), + // A 2-D array with one index is the row. + ("=SUM(INDEX({1,2;3,4},2))", 7.0), + ("=SUM(INDEX({1,2;3,4},0))", 10.0), + // A 2-D reference with an empty (not omitted) column argument is the row. + ("=SUM(INDEX(A5:C7,2,))", 15.0), + ("=SUM(INDEX(A5:C7,2,,1))", 15.0), + ("=COLUMNS(INDEX((A1:A3,A5:C7),2,,2))", 3.0), + ("=INDEX(A5:C7,2,2)", 5.0), + // A 2-D reference with one index is #REF!, and the error carries through. + ("=TYPE(INDEX(A5:C7,2))", 16.0), + ("=ISREF(INDEX(A5:C7,2))", 0.0), + ] + .iter() + .enumerate() + { + let at = format!("J{}", row * 4 + 1); + t.set(&at, formula); + let got = match t.get(&at) { + Value::Bool(b) => f64::from(u8::from(b)), + v => v.as_f64().unwrap_or_else(|| panic!("{formula} = {v:?}")), + }; + assert_eq!(got, *expected, "{formula}"); + } + for formula in [ + "=INDEX(A5:C7,2)", + "=INDEX(A5:C7,0)", + "=INDEX(A5:C7,1.9)", + "=INDEX(A5:C7,)", + "=INDEX(A5:B6,1)", + "=INDEX(A5:C7,2)+1", + "=SUM(INDEX(A5:C7,{1;2}))", + "=ROWS(INDEX(INDEX(A5:C7,2),3))", + "=SUM(A5:INDEX(A5:C7,2))", + "=SUM(INDEX(A5:C7,2):C7)", + ] { + t.set("L1", formula); + assert_eq!(t.get("L1"), Value::Error(CellError::Ref), "{formula}"); + } + t.set("L1", "=INDEX(A5:C7,{1;2})"); + assert_eq!([t.get("L1"), t.get("L2")], [Value::Error(CellError::Ref), Value::Error(CellError::Ref)]); + t.set("L1", "=IFERROR(INDEX(A5:C7,2),\"e\")"); + assert_eq!(t.get("L1"), Value::from("e")); + // A bad position is still #VALUE!. + t.set("L1", "=INDEX(A5:C7,\"x\")"); + assert_eq!(t.get("L1"), Value::Error(CellError::Value)); +} + #[test] fn errors_and_cycles() { let mut t = T::new(); diff --git a/docs/function-parity.md b/docs/function-parity.md index 760ffd6ac..12bef8ee9 100644 --- a/docs/function-parity.md +++ b/docs/function-parity.md @@ -1,6 +1,6 @@ # Function parity: worksheet functions and formulas -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (USDOLLAR added as a compatibility function; MIRR, COUNTIF and INDIRECT edge cases match Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-15 · **Change:** minor (INDEX with one index on a reference of several rows and columns is #REF! as in Excel) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Excel's worksheet functions, category by category, against GridCraft's function library. Part of [target-app-parity.md](target-app-parity.md); gaps feed [gaps.md](gaps.md). @@ -108,6 +108,7 @@ workbooks; results checked in as data, not Excel files) and fixing what it finds | Date | Change | Summary | |---|---|---| +| 2026-10-15 | minor | INDEX with only a row index on a reference of several rows and columns (`INDEX(A1:C3,2)`, `INDEX(A1:C3,0)`) is `#REF!`, as observed in Excel, so it also fails as a range endpoint and inside other functions; an empty column argument (`INDEX(A1:C3,2,)`) still gives the row. Checked that a column array with one index stays a 1×1 array (`TYPE` 64), which is what Excel returns | | 2026-10-12 | minor | USDOLLAR added as a Compatibility function (not on Microsoft's list, so the counts don't change); MIRR with no positive flow is -1, COUNTIF's bare `"<="` / `">="` skip empty strings, INDIRECT with `a1` = FALSE gives `#REF!` for A1-style text (#738) | | 2026-10-11 | minor | TEXT / number formats fixed against Excel-cached results: fraction literals and zero fractions, `0` denominators zero padded, `?` pads the thousands separator, booleans use the text section (226 mismatches → 0 in 352 TEXT cells) | | 2026-10-11 | minor | Iterative calculation: circular references iterate up to Maximum Iterations / Maximum Change when enabled | From d5aa6fbcbcabaf195c904202bcf3c44a5b094f9e Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Fri, 23 Oct 2026 21:56:39 +0200 Subject: [PATCH 06/21] Conditional formatting: New Rule's formula box takes references clicked or dragged on the grid (#2348) --- crates/ui-egui/src/dialogs.rs | 84 +++++++++++++ crates/ui-egui/src/grid.rs | 22 +++- crates/ui-egui/tests/it/cf_rule_pick.rs | 160 ++++++++++++++++++++++++ crates/ui-egui/tests/it/main.rs | 1 + docs/ui-parity.md | 6 +- 5 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 crates/ui-egui/tests/it/cf_rule_pick.rs diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index bfb203c16..8be7f55cb 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -7,6 +7,7 @@ use egui::{Color32, Key, Sense, Stroke, StrokeKind, vec2}; use crate::icons::{self, Icon}; +use gridcraft_engine::core::{CellRef, RangeRef}; use serde_json::{Map, Value as Json, json}; use crate::SheetApp; @@ -44,6 +45,24 @@ pub struct Dialog { /// The dialog this one was opened from (Name Manager › New…): it comes back when this closes. back: Option>, focus_initialized: bool, + /// The formula box that takes references clicked or dragged on the grid (New Formatting Rule). + pick: Option, +} + +/// A formula box in point mode, as in Excel: a click on the grid writes the cell's absolute +/// reference at the caret instead of selecting it, and a drag grows it into a range. +#[derive(Clone, Debug)] +struct RefPick { + key: &'static str, + /// The sheet the dialog was opened on: references to another sheet carry its name. + sheet: usize, + /// Caret (char index), mirrored from the text box while it has the keyboard. + caret: usize, + /// The reference last inserted (byte span), replaced by the next click or the drag. + point: Option<(usize, usize)>, + anchor: CellRef, + /// Give the box the keyboard back, caret after the reference, on the next frame. + refocus: bool, } impl Dialog { @@ -77,6 +96,43 @@ impl Dialog { self.values.insert(key.into(), value); } + /// Whether a click on the grid goes to a formula box of this dialog rather than the selection. + pub fn picks_references(&self) -> bool { + self.pick.is_some() + } + + /// A click (`extend` false) or a drag reaching `cell`: writes the absolute reference at the + /// formula box's caret, replacing the reference the press inserted, as in the formula bar's + /// point mode. An empty box starts with `=`. After an operand there is no place for a + /// reference, so nothing changes. `active_sheet` names the sheet shown when it is not the + /// dialog's own. + pub fn pick_reference(&mut self, cell: CellRef, active_sheet: (usize, &str), extend: bool) { + let Some(pick) = self.pick.as_mut() else { return }; + let text = self.values.get(pick.key).and_then(Json::as_str).unwrap_or("").to_string(); + let mut ed = crate::editor::EditState::new(pick.sheet, cell, text, true, false); + ed.caret = pick.caret.min(ed.text.chars().count()); + ed.point = pick.point; + if ed.text.trim().is_empty() { + ed.text = "=".into(); + ed.caret = 1; + ed.point = None; + } + if !ed.can_point() { + return; + } + if !extend { + pick.anchor = cell; + } + let range = abs(&RangeRef::new(pick.anchor, cell)); + let reference = + if active_sheet.0 == pick.sheet { range } else { format!("{}!{range}", gridcraft_engine::formula::quote_sheet(active_sheet.1)) }; + ed.insert_ref(&reference); + pick.caret = ed.caret; + pick.point = ed.point; + pick.refocus = true; + self.values.insert(pick.key.into(), json!(ed.text)); + } + fn form(name: &str, title: &str, command: &'static str, fields: Vec, defaults: Json) -> Dialog { Dialog { name: name.into(), @@ -94,6 +150,7 @@ impl Dialog { picture_target: None, back: None, focus_initialized: false, + pick: None, } } @@ -114,6 +171,7 @@ impl Dialog { picture_target: None, back: None, focus_initialized: false, + pick: None, } } @@ -690,6 +748,10 @@ impl Dialog { fields, json!({"preset": "lightRedFillDarkRedText", "period": "today"}), ); + if ty == "expression" { + let sheet = app.session.active().map(|doc| doc.wb.active_sheet).unwrap_or(0); + d.pick = Some(RefPick { key: "formula", sheet, caret: 0, point: None, anchor: CellRef::new(0, 0), refocus: false }); + } d.values.insert("type".into(), json!(ty)); d.values.insert("operator".into(), p("operator")); d @@ -1634,6 +1696,25 @@ fn take_first_focus(d: &mut Dialog, r: &egui::Response) { d.focus_first_field = false; } +/// Keeps a picking formula box's caret, and gives it the keyboard back after a grid click put a +/// reference in it (the click took the focus), its caret after the reference. +fn track_pick(pick: &mut RefPick, r: &egui::Response, typed: bool) { + if typed { + pick.point = None; + } + if pick.refocus { + let mut state = egui::text_edit::TextEditState::load(&r.ctx, r.id).unwrap_or_default(); + state.cursor.set_char_range(Some(egui::text::CCursorRange::one(egui::text::CCursor::new(pick.caret)))); + state.store(&r.ctx, r.id); + r.request_focus(); + pick.refocus = false; + } else if r.has_focus() + && let Some(range) = egui::text_edit::TextEditState::load(&r.ctx, r.id).and_then(|state| state.cursor.char_range()) + { + pick.caret = range.primary.index.into(); + } +} + /// Draws a dialog's `fields`, editing its values. fn form_fields(ui: &mut egui::Ui, d: &mut Dialog) { // Field labels, choices and notes are English source strings, translated as they are drawn. @@ -1659,6 +1740,9 @@ fn form_fields(ui: &mut egui::Ui, d: &mut Dialog) { if r.changed() { d.values.insert(key.into(), json!(s)); } + if let Some(pick) = d.pick.as_mut().filter(|pick| pick.key == key) { + track_pick(pick, &r, r.changed()); + } }); } Field::Password { key, label } => { diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 590294904..157f608fe 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2336,6 +2336,19 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & app.grid.filter_menu = Some((c.col, c.row, geo.mirror_pos(p))); return; } + // A dialog's formula box in point mode (New Formatting Rule) takes the reference; the + // selection, which the dialog acts on, stays. + if let Some(d) = app.dialog.as_mut().filter(|d| d.picks_references()) { + d.pick_reference(c, (wb.active_sheet, &sh.name), false); + app.grid.drag = Drag::Select; + app.grid.drag_select = true; + if resp.drag_started() + && let Some(p) = pos + { + select_to_pointer(app, geo, sh, wb, p); + } + return; + } // Point mode while editing a formula: clicking inserts a reference. if let Some(ed) = app.editor.as_mut() && ed.can_point() @@ -2643,7 +2656,10 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & /// An armed Format Painter pastes onto the cells just clicked or dragged over (not while /// pointing a formula reference, which also drags a Select). fn format_painter_apply(app: &mut SheetApp) { - if app.session.format_painter.is_some() && !app.editor.as_ref().is_some_and(|ed| ed.can_point()) { + if app.session.format_painter.is_some() + && !app.editor.as_ref().is_some_and(|ed| ed.can_point()) + && !app.dialog.as_ref().is_some_and(|d| d.picks_references()) + { let dest = app.session.active().map(|d| d.selection.current().a1()); if let Some(dest) = dest && let Err(e) = app.run("edit.formatPainter", json!({"apply": dest})) @@ -2696,7 +2712,9 @@ fn select_to_pointer(app: &mut SheetApp, geo: &Geo, sh: &Sheet, wb: &Workbook, p geo.row_at(sh, p.y.clamp(geo.cells.top() + 1.0, geo.cells.bottom() - 1.0)), geo.col_at(sh, p.x.clamp(geo.cells.left() + 1.0, geo.cells.right() - 1.0)), ); - if let Some(ed) = app.editor.as_mut() + if let Some(d) = app.dialog.as_mut().filter(|d| d.picks_references()) { + d.pick_reference(cell, (wb.active_sheet, &sh.name), true); + } else if let Some(ed) = app.editor.as_mut() && ed.point.is_some() { if let Some(start) = ed.point_anchor.filter(|(sheet, _)| *sheet == wb.active_sheet).map(|(_, cell)| cell).or(ed.point_cell) { diff --git a/crates/ui-egui/tests/it/cf_rule_pick.rs b/crates/ui-egui/tests/it/cf_rule_pick.rs new file mode 100644 index 000000000..dc004ff25 --- /dev/null +++ b/crates/ui-egui/tests/it/cf_rule_pick.rs @@ -0,0 +1,160 @@ +//! New Formatting Rule ▸ formula: clicking or dragging on the grid writes the absolute reference +//! into the formula box at its caret, as in Excel, and leaves the selection the rule applies to alone. + +use egui::{Event, Key, Modifiers, PointerButton, Pos2, pos2, vec2}; +use gridcraft_engine::Session; +use gridcraft_ui_egui::{SheetApp, grid::Geo}; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +fn harness() -> Harness { + let mut session = Session::new(); + session.new_workbook(); + session.execute("home.insertSheet", json!({})).unwrap(); + session.execute("sheet.activate", json!({"sheet": 0})).unwrap(); + session.execute("selection.set", json!({"range": "F8:G9"})).unwrap(); + let mut h = egui_kittest::Harness::builder().with_step_dt(1.0 / 60.0).with_size(vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(ui.ctx()); + app.ui(ui); + }, + SheetApp::new(session, Default::default()), + ); + h.run_steps(4); + h.state_mut().open_dialog("cfQuick", json!({"type": "expression", "title": "New Formatting Rule"})); + h.run_steps(6); + h +} + +fn formula(h: &Harness) -> String { + h.state().dialog.as_ref().and_then(|d| d.values.get("formula")).and_then(|v| v.as_str()).unwrap_or("").to_string() +} + +fn selection(h: &Harness) -> String { + h.state().session.active().unwrap().selection.current().a1() +} + +fn type_text(h: &mut Harness, text: &str) { + for c in text.chars() { + h.input_mut().events.push(Event::Text(c.to_string())); + h.run_steps(1); + } +} + +fn cell_center(h: &Harness, a1: &str) -> Pos2 { + let app = h.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let geo = Geo::new(sheet, app.grid.rect.unwrap(), app.view().scroll); + let c = gridcraft_engine::core::CellRef::parse(a1).unwrap(); + pos2(geo.x(sheet, c.col) + sheet.col_width(c.col) * geo.z * 0.5, geo.y(sheet, c.row) + sheet.row_height(c.row) * geo.z * 0.5) +} + +fn click(h: &mut Harness, a1: &str) { + let pos = cell_center(h, a1); + for pressed in [true, false] { + h.input_mut() + .events + .extend([Event::PointerMoved(pos), Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }]); + h.step(); + } + h.run_steps(3); +} + +fn drag(h: &mut Harness, from: &str, to: &str) { + let (a, b) = (cell_center(h, from), cell_center(h, to)); + h.input_mut() + .events + .extend([Event::PointerMoved(a), Event::PointerButton { pos: a, button: PointerButton::Primary, pressed: true, modifiers: Modifiers::NONE }]); + h.step(); + for i in 1..=6 { + let p = a + (b - a) * (i as f32 / 6.0); + h.input_mut().events.push(Event::PointerMoved(p)); + h.step(); + } + h.input_mut().events.push(Event::PointerButton { pos: b, button: PointerButton::Primary, pressed: false, modifiers: Modifiers::NONE }); + h.step(); + h.run_steps(3); +} + +#[test] +fn clicking_a_cell_after_equals_inserts_its_absolute_reference() { + let mut h = harness(); + type_text(&mut h, "="); + assert_eq!(formula(&h), "=", "the formula box had the keyboard"); + click(&mut h, "B2"); + assert_eq!(formula(&h), "=$B$2"); + assert_eq!(selection(&h), "F8:G9", "the range the rule applies to is still selected"); +} + +#[test] +fn clicking_a_cell_in_an_empty_box_starts_the_formula() { + let mut h = harness(); + click(&mut h, "C3"); + assert_eq!(formula(&h), "=$C$3"); + assert_eq!(selection(&h), "F8:G9"); +} + +#[test] +fn a_second_click_replaces_the_reference_just_inserted() { + let mut h = harness(); + type_text(&mut h, "="); + click(&mut h, "B2"); + click(&mut h, "D5"); + assert_eq!(formula(&h), "=$D$5"); +} + +#[test] +fn dragging_inserts_a_range() { + let mut h = harness(); + type_text(&mut h, "="); + drag(&mut h, "B2", "C4"); + assert_eq!(formula(&h), "=$B$2:$C$4"); + assert_eq!(selection(&h), "F8:G9"); +} + +#[test] +fn the_reference_goes_at_the_caret_and_typing_continues_after_it() { + let mut h = harness(); + type_text(&mut h, "=SUM()"); + h.key_press(Key::ArrowLeft); + h.run_steps(2); + click(&mut h, "B2"); + assert_eq!(formula(&h), "=SUM($B$2)"); + type_text(&mut h, "+1"); + assert_eq!(formula(&h), "=SUM($B$2+1)", "the box has the keyboard back, its caret after the reference"); +} + +#[test] +fn a_click_after_an_operand_changes_nothing() { + let mut h = harness(); + type_text(&mut h, "=A1>5"); + click(&mut h, "B2"); + assert_eq!(formula(&h), "=A1>5"); + assert_eq!(selection(&h), "F8:G9"); +} + +#[test] +fn ok_applies_the_picked_formula_to_the_selection() { + let mut h = harness(); + type_text(&mut h, "="); + click(&mut h, "B2"); + type_text(&mut h, ">0"); + h.key_press(Key::Enter); + h.run_steps(4); + assert!(h.state().dialog.is_none(), "Enter confirmed the rule"); + let sheet = h.state().session.active().unwrap().wb.active().unwrap(); + let cf = sheet.cond_formats.last().expect("a rule was added"); + assert_eq!(cf.ranges.iter().map(|r| r.a1()).collect::>(), ["F8:G9"]); + assert!(format!("{:?}", cf.rule).contains("$B$2>0"), "rule: {:?}", cf.rule); +} + +#[test] +fn other_dialogs_leave_grid_clicks_alone() { + let mut h = harness(); + h.state_mut().dialog = None; + h.state_mut().open_dialog("cfQuick", json!({"type": "containsText", "title": "Text that Contains"})); + h.run_steps(6); + click(&mut h, "B2"); + assert_eq!(selection(&h), "B2", "a box that is not a formula does not take references"); +} diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs index b89253bb9..94d654089 100644 --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -10,6 +10,7 @@ mod autosave_persist; mod borders; mod cell_editor_caret; mod cell_pictures; +mod cf_rule_pick; mod chart_resize; mod clipboard; mod comments_pane; diff --git a/docs/ui-parity.md b/docs/ui-parity.md index c7cbc349e..4795fb196 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-22 · **Change:** minor (Page Up and Ctrl+Up into frozen rows, Ctrl+Left and Alt+Page Up into frozen columns bring the scrolled area back; Auto Fill Options button after a left-drag of the fill handle (#1403); Show Outline Symbols, Ctrl+8; a too-wide number in a repeat-fill format such as Accounting shows #### instead of losing its leading digits; Esc and Enter close a data validation alert over the cell editor; Ctrl+Enter fill gives each cell the automatic number format typing gives it; keytip letters typed with Alt held, #1846; Number Stored as Text indicator and Convert to Number) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-23 · **Change:** minor (New Formatting Rule's formula box takes references clicked or dragged on the grid, #2348; Page Up and Ctrl+Up into frozen rows, Ctrl+Left and Alt+Page Up into frozen columns bring the scrolled area back; Auto Fill Options button after a left-drag of the fill handle (#1403); Show Outline Symbols, Ctrl+8; a too-wide number in a repeat-fill format such as Accounting shows #### instead of losing its leading digits; Esc and Enter close a data validation alert over the cell editor; Ctrl+Enter fill gives each cell the automatic number format typing gives it; keytip letters typed with Alt held, #1846; Number Stored as Text indicator and Convert to Number) · **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 @@ -152,6 +152,9 @@ Seek, Scenario Manager, Data Analysis, Forecast Sheet and others exist. Missing Solver, Power Query editor, Insert Object, Equation editor, Protect/Allow Edit Ranges, Workbook Links, Accessibility checker. Like Excel's, every dialog opens centred and moves by its title bar, keeping its place while it is open (#1250). +New Formatting Rule's formula box points like the formula bar: a click on the grid writes the +cell's absolute reference at the caret (`=$B$2`) and a drag a range, leaving the selection the rule +applies to as it is (#2348). Other dialogs' reference fields don't take clicks yet. ## Accessibility @@ -175,6 +178,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-23 | minor | Conditional Formatting ▸ New Rule, formula: clicking a cell on the grid writes its absolute reference at the formula box's caret (`=$B$2`, an empty box starts with `=`), a drag writes a range (`$B$2:$C$4`) and a second click replaces the reference just written, as in Excel ("Selecting cells on the worksheet inserts absolute cell references"). The selection the rule applies to doesn't move, and the box keeps the keyboard. After an operand (`=A1>5`) the click does nothing (#2348) | | 2026-10-23 | minor | Frozen panes: Page Up, Ctrl+Up and Shift variants that move from the scrolled rows into the frozen ones bring the scrolled rows back to the top, so the rows in between are seen again (Page Up from far down used to end on row 1 with the view still a page down); Ctrl+Left and Alt+Page Up do the same for frozen columns. Other ways of selecting a frozen cell (click, Find, Go To, Undo, formula pointing) leave the view where it is | | 2026-10-23 | minor | After a left-drag of the fill handle an Auto Fill Options button sits below the filled cells; its menu redoes the fill as Copy Cells, Fill Series, Fill Formatting Only or Fill Without Formatting (undoes the drag's fill, then `edit.autoFill` in that mode). It goes away when the workbook changes or the selection moves (#1403) | | 2026-10-22 | minor | Show Outline Symbols (`view.showOutlineSymbols`, Ctrl+8): a sheet can hide the outline's level buttons and brackets while keeping its outline, as Excel saves with `showOutlineSymbols="0"` (read and written); the grid then gives the bands no room | From 45f06f50da6c32fd2f76b8a9de0673d5dd2e3675 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Fri, 23 Oct 2026 22:05:24 +0200 Subject: [PATCH 07/21] Conditional formatting: New Rule keeps the range it was opened for when pointing on another sheet (#2348) --- crates/ui-egui/src/dialogs.rs | 15 +++++++++++++-- crates/ui-egui/tests/it/cf_rule_pick.rs | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 8be7f55cb..95855e56a 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -63,6 +63,9 @@ struct RefPick { anchor: CellRef, /// Give the box the keyboard back, caret after the reference, on the next frame. refocus: bool, + /// The selection the rule applies to, fixed when the dialog opened as Excel's "Applies to": + /// pointing on another sheet must not move the rule there. + applies_to: String, } impl Dialog { @@ -749,8 +752,12 @@ impl Dialog { json!({"preset": "lightRedFillDarkRedText", "period": "today"}), ); if ty == "expression" { - let sheet = app.session.active().map(|doc| doc.wb.active_sheet).unwrap_or(0); - d.pick = Some(RefPick { key: "formula", sheet, caret: 0, point: None, anchor: CellRef::new(0, 0), refocus: false }); + let (sheet, applies_to) = app + .session + .active() + .map(|doc| (doc.wb.active_sheet, doc.selection.ranges.iter().map(RangeRef::a1).collect::>().join(","))) + .unwrap_or_default(); + d.pick = Some(RefPick { key: "formula", sheet, caret: 0, point: None, anchor: CellRef::new(0, 0), refocus: false, applies_to }); } d.values.insert("type".into(), json!(ty)); d.values.insert("operator".into(), p("operator")); @@ -1177,6 +1184,10 @@ pub(crate) fn build_params(d: &Dialog) -> Json { rule.insert(k.clone(), val.clone()); } v = json!({"rule": Json::Object(rule)}); + if let Some(pick) = d.pick.as_ref().filter(|pick| !pick.applies_to.is_empty()) { + v["sheet"] = json!(pick.sheet); + v["range"] = json!(pick.applies_to); + } } "series" => { if let Some(s) = d.values.get("stop").and_then(Json::as_str).and_then(|s| s.parse::().ok()) { diff --git a/crates/ui-egui/tests/it/cf_rule_pick.rs b/crates/ui-egui/tests/it/cf_rule_pick.rs index dc004ff25..642a91a18 100644 --- a/crates/ui-egui/tests/it/cf_rule_pick.rs +++ b/crates/ui-egui/tests/it/cf_rule_pick.rs @@ -158,3 +158,20 @@ fn other_dialogs_leave_grid_clicks_alone() { click(&mut h, "B2"); assert_eq!(selection(&h), "B2", "a box that is not a formula does not take references"); } + +#[test] +fn pointing_on_another_sheet_keeps_the_rule_on_the_sheet_it_was_opened_for() { + let mut h = harness(); + type_text(&mut h, "="); + h.state_mut().session.execute("sheet.activate", json!({"sheet": 1})).unwrap(); + h.run_steps(3); + click(&mut h, "A1"); + assert_eq!(formula(&h), "=Sheet2!$A$1"); + h.key_press(Key::Enter); + h.run_steps(4); + assert!(h.state().dialog.is_none(), "Enter confirmed the rule"); + let wb = &h.state().session.active().unwrap().wb; + assert!(wb.sheet(1).unwrap().cond_formats.is_empty(), "nothing lands on the sheet pointed at"); + let cf = wb.sheet(0).unwrap().cond_formats.last().expect("the rule is on the sheet the dialog was opened on"); + assert_eq!(cf.ranges.iter().map(|r| r.a1()).collect::>(), ["F8:G9"]); +} From 4b2ee2e2dd8c4e71793fa8a658242627e356fd22 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Fri, 23 Oct 2026 22:07:22 +0200 Subject: [PATCH 08/21] Conditional formatting: New Rule remembers its sheet by name, so moving or inserting sheets while it is open keeps the rule there (#2348) --- crates/ui-egui/src/dialogs.rs | 27 ++++++++++++++++--------- crates/ui-egui/src/grid.rs | 4 ++-- crates/ui-egui/tests/it/cf_rule_pick.rs | 20 ++++++++++++++++++ 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 95855e56a..e83db2fa1 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -54,8 +54,9 @@ pub struct Dialog { #[derive(Clone, Debug)] struct RefPick { key: &'static str, - /// The sheet the dialog was opened on: references to another sheet carry its name. - sheet: usize, + /// The sheet the dialog was opened on, by name: its index changes when sheets are inserted or + /// moved while the dialog is open. References to another sheet carry that sheet's name. + sheet: String, /// Caret (char index), mirrored from the text box while it has the keyboard. caret: usize, /// The reference last inserted (byte span), replaced by the next click or the drag. @@ -109,10 +110,10 @@ impl Dialog { /// point mode. An empty box starts with `=`. After an operand there is no place for a /// reference, so nothing changes. `active_sheet` names the sheet shown when it is not the /// dialog's own. - pub fn pick_reference(&mut self, cell: CellRef, active_sheet: (usize, &str), extend: bool) { + pub fn pick_reference(&mut self, cell: CellRef, active_sheet: &str, extend: bool) { let Some(pick) = self.pick.as_mut() else { return }; let text = self.values.get(pick.key).and_then(Json::as_str).unwrap_or("").to_string(); - let mut ed = crate::editor::EditState::new(pick.sheet, cell, text, true, false); + let mut ed = crate::editor::EditState::new(0, cell, text, true, false); ed.caret = pick.caret.min(ed.text.chars().count()); ed.point = pick.point; if ed.text.trim().is_empty() { @@ -127,8 +128,11 @@ impl Dialog { pick.anchor = cell; } let range = abs(&RangeRef::new(pick.anchor, cell)); - let reference = - if active_sheet.0 == pick.sheet { range } else { format!("{}!{range}", gridcraft_engine::formula::quote_sheet(active_sheet.1)) }; + let reference = if active_sheet.eq_ignore_ascii_case(&pick.sheet) { + range + } else { + format!("{}!{range}", gridcraft_engine::formula::quote_sheet(active_sheet)) + }; ed.insert_ref(&reference); pick.caret = ed.caret; pick.point = ed.point; @@ -755,7 +759,12 @@ impl Dialog { let (sheet, applies_to) = app .session .active() - .map(|doc| (doc.wb.active_sheet, doc.selection.ranges.iter().map(RangeRef::a1).collect::>().join(","))) + .map(|doc| { + ( + doc.wb.active().map(|sh| sh.name.clone()).unwrap_or_default(), + doc.selection.ranges.iter().map(RangeRef::a1).collect::>().join(","), + ) + }) .unwrap_or_default(); d.pick = Some(RefPick { key: "formula", sheet, caret: 0, point: None, anchor: CellRef::new(0, 0), refocus: false, applies_to }); } @@ -1184,8 +1193,8 @@ pub(crate) fn build_params(d: &Dialog) -> Json { rule.insert(k.clone(), val.clone()); } v = json!({"rule": Json::Object(rule)}); - if let Some(pick) = d.pick.as_ref().filter(|pick| !pick.applies_to.is_empty()) { - v["sheet"] = json!(pick.sheet); + if let Some(pick) = d.pick.as_ref().filter(|pick| !pick.applies_to.is_empty() && !pick.sheet.is_empty()) { + v["sheet"] = json!(pick.sheet.as_str()); v["range"] = json!(pick.applies_to); } } diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 157f608fe..87ac5787e 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2339,7 +2339,7 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & // A dialog's formula box in point mode (New Formatting Rule) takes the reference; the // selection, which the dialog acts on, stays. if let Some(d) = app.dialog.as_mut().filter(|d| d.picks_references()) { - d.pick_reference(c, (wb.active_sheet, &sh.name), false); + d.pick_reference(c, &sh.name, false); app.grid.drag = Drag::Select; app.grid.drag_select = true; if resp.drag_started() @@ -2713,7 +2713,7 @@ fn select_to_pointer(app: &mut SheetApp, geo: &Geo, sh: &Sheet, wb: &Workbook, p geo.col_at(sh, p.x.clamp(geo.cells.left() + 1.0, geo.cells.right() - 1.0)), ); if let Some(d) = app.dialog.as_mut().filter(|d| d.picks_references()) { - d.pick_reference(cell, (wb.active_sheet, &sh.name), true); + d.pick_reference(cell, &sh.name, true); } else if let Some(ed) = app.editor.as_mut() && ed.point.is_some() { diff --git a/crates/ui-egui/tests/it/cf_rule_pick.rs b/crates/ui-egui/tests/it/cf_rule_pick.rs index 642a91a18..a523c9c6d 100644 --- a/crates/ui-egui/tests/it/cf_rule_pick.rs +++ b/crates/ui-egui/tests/it/cf_rule_pick.rs @@ -175,3 +175,23 @@ fn pointing_on_another_sheet_keeps_the_rule_on_the_sheet_it_was_opened_for() { let cf = wb.sheet(0).unwrap().cond_formats.last().expect("the rule is on the sheet the dialog was opened on"); assert_eq!(cf.ranges.iter().map(|r| r.a1()).collect::>(), ["F8:G9"]); } + +#[test] +fn the_rule_follows_its_sheet_when_sheets_move_while_the_dialog_is_open() { + let mut h = harness(); + type_text(&mut h, "="); + click(&mut h, "B2"); + h.state_mut().session.execute("sheet.move", json!({"sheet": 0, "to": 1})).unwrap(); + h.run_steps(3); + let names = |h: &Harness| h.state().session.active().unwrap().wb.sheets.iter().map(|s| s.name.clone()).collect::>(); + assert_eq!(names(&h), ["Sheet2", "Sheet1"], "Sheet1 now sits at index 1"); + h.key_press(Key::Enter); + h.run_steps(4); + assert!(h.state().dialog.is_none(), "Enter confirmed the rule"); + let wb = &h.state().session.active().unwrap().wb; + let sheet1 = wb.sheets.iter().find(|s| s.name == "Sheet1").unwrap(); + let sheet2 = wb.sheets.iter().find(|s| s.name == "Sheet2").unwrap(); + assert!(sheet2.cond_formats.is_empty(), "nothing lands on the sheet that took index 0"); + let cf = sheet1.cond_formats.last().expect("the rule stays on Sheet1"); + assert_eq!(cf.ranges.iter().map(|r| r.a1()).collect::>(), ["F8:G9"]); +} From 1ce4781d3e059e14696cd4a351f61e4242062767 Mon Sep 17 00:00:00 2001 From: Harrison Booth <73434116+Harrison-Booth@users.noreply.github.com> Date: Fri, 23 Oct 2026 16:24:18 -0500 Subject: [PATCH 09/21] Accuracy: PMT to Excel's last bits via ln_1p/exp_m1 PMT computed (1 + rate)^nper with powf, which rounds 1 + rate first and drops the low bits of a small rate. Over 400 synthetic probes that left PMT up to about 1000 ULPs from Excel (59 bit-exact); the ln_1p/exp_m1 form is within 4 ULPs (244 bit-exact). A loan schedule guarded by IF(p - SUM(principal) > 0, ...) turns on those bits: in 3000 synthetic schedules main disagreed with Excel on the number of live periods in 1382, the fix in 823. Also follows Excel at the edges: a rate of -1 or less is #NUM!, a huge nper gives -pv*rate instead of #NUM!, a tiny rate no longer collapses, and a small negative rate keeps its payment. IPMT, PPMT, CUMIPMT and CUMPRINC pick this up through pmt_calc. FV and PV are unchanged: Excel's own FV/PV sit further from the exact value than the current powf form does. The tests allow the one-ULP libm difference between platforms. --- crates/calc/src/tests.rs | 37 ++++++++++++++++++++ crates/functions/src/financial.rs | 58 +++++++++++++++++++++++++++++-- 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 622ed6cc9..e71015213 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -1781,3 +1781,40 @@ fn text_before_after_empty_instance_defaults_to_one() { t.set("A4", "=TEXTBEFORE(\"a-b\",\"-\",0)"); assert_eq!(t.get("A4"), Value::Error(CellError::Value)); } + +#[test] +fn loan_schedule_residue_keeps_excels_sign() { + // A synthetic monthly loan schedule: interest on the opening balance, principal = PMT - interest, + // each later period guarded by IF(p - SUM(principal so far) > 0, ...). After the 12th payment + // Excel is left with 2^-30 > 0, so it takes a 13th, tiny-interest period. That last bit comes + // from PMT; the old (1 + rate)^nper form left -2.8e-8 and returned "" instead. + let mut t = T::new(); + t.set("B1", "4653300"); + t.set("B2", "1"); + t.set("B3", "0.141"); + let col = |i: u8| char::from(b'A' + i).to_string(); + t.set("C8", "=$B$1*($B$3/12)"); + t.set("C7", "=PMT($B$3/12,$B$2*12,-$B$1)-C8"); + t.set("D8", "=($B1-C7)*($B$3/12)"); + t.set("D7", "=PMT($B$3/12,$B$2*12,-$B$1)-D8"); + for i in 4..16 { + let (c, p) = (col(i), col(i - 1)); + t.set(&format!("{c}8"), &format!("=IF($B1-SUM($C7:{p}7)>0,($B1-SUM($C7:{p}7))*($B$3/12),\"\")")); + t.set(&format!("{c}7"), &format!("=IF($B1-SUM($C7:{p}7)>0,PMT($B$3/12,$B$2*12,-$B$1)-{c}8,\"\")")); + } + t.set("N4", "=IF($B$1-SUM($C7:N7)>0,$B$1-SUM($C7:N7),\"\")"); + // Excel's values (Value2) for the same synthetic inputs. + // The leftover (2^-30 in Excel) is 12 payments' worth of rounding, so a one-ULP libm difference + // moves it by as much as its own size: only its sign is checked, and the 13th period must exist. + let positive = |cell: &str| match t.get(cell) { + Value::Number(x) => assert!(x > 0.0 && x < 1e-6, "{cell}: {x}"), + other => panic!("{cell}: expected a small positive number, got {other:?}"), + }; + positive("N4"); + positive("O8"); + match t.get("O7") { + Value::Number(x) => assert!((x - 418025.37230211904).abs() < 1e-3, "O7: {x}"), + other => panic!("O7: expected a number, got {other:?}"), + } + assert_eq!(t.get("P7"), Value::Text("".into())); +} diff --git a/crates/functions/src/financial.rs b/crates/functions/src/financial.rs index af28326e7..043c09ed6 100644 --- a/crates/functions/src/financial.rs +++ b/crates/functions/src/financial.rs @@ -13,12 +13,31 @@ use crate::{Arg, Ctx, FnSpec, VAR}; // Time value of money // --------------------------------------------------------------------------------------------- +/// The payment per period. `(1 + rate)^nper` comes from `nper * ln_1p(rate)` and `p - 1` from +/// `exp_m1`: rounding `1 + rate` first throws away the low bits of a small rate, which left PMT up to +/// about 1000 ULPs from Excel (observed over 400 synthetic probes; this form is within 4). A loan +/// schedule's last `IF(p - SUM(principal) > 0, ...)` turns on those bits. Excel gives #NUM! for a +/// rate of -1 or less, and -pv*rate (no divide) once `(1 + rate)^nper` overflows. fn pmt_calc(rate: f64, nper: f64, pv: f64, fv: f64, typ: f64) -> f64 { if rate == 0.0 { return -(pv + fv) / nper; } - let p = (1.0 + rate).powf(nper); - -(rate * (fv + pv * p)) / ((1.0 + rate * typ) * (p - 1.0)) + if rate <= -1.0 { + return f64::NAN; + } + let l = nper * rate.ln_1p(); + let m = l.exp_m1(); + let k = 1.0 + rate * typ; + if m > 0.0 { + // (fv + pv*p) / (p - 1) = pv + (pv + fv) / (p - 1), without the cancellation in p - 1. + -(pv + (pv + fv) / m) * rate / k + } else if m > -0.5 { + // p near 1 from below: fv + pv*p = (pv + fv) + pv*m keeps the small terms that rounding p would drop. + -(rate * (pv + fv + pv * m)) / (k * m) + } else { + // p well below 1: m nears -1 and pv*m would swallow pv + fv, so use p itself. + -(rate * (fv + pv * l.exp())) / (k * m) + } } fn fv_calc(rate: f64, nper: f64, pmt: f64, pv: f64, typ: f64) -> f64 { @@ -1790,6 +1809,41 @@ mod tests { is_err(ev("CUMIPMT", vec![n(0.01), n(360.0), n(125000.0), n(1.0), n(1.0), n(2.0)]), CellError::Num); } + /// PMT within about 4 ULPs of Excel's (Value2 of synthetic probes; libm differs by an ULP across + /// platforms). The old `(1 + rate)^nper` form was 25 to 91 ULPs off on these, enough to flip a loan schedule's final `IF(p - SUM(r) > 0, ...)`. + #[test] + fn pmt_matches_excel_within_a_few_ulps() { + for (rate, nper, pv, fv, typ, excel) in [ + (0.141 / 12.0, 12.0, -4653300.0, 0.0, 0.0, 418025.37230211904), + (0.005716666666666666, 26.0, 42678411.0, -26010.62, 0.0, -1770235.3828507778), + (0.012164, 29.0, 16722649.0, 0.0, 1.0, -679516.0784762775), + (0.009916666666666666, 5.0, 8590842.8, 0.0, 0.0, -1769620.32624324), + (0.011158333333333333, 36.0, 21457108.0, 49218.26, 1.0, -720095.3912155787), + (-0.5, 2.5, 100.0, 0.0, 0.0, -10.736861692729645), + ] { + let got = ev("PMT", vec![n(rate), n(nper), n(pv), n(fv), n(typ)]); + close_tol(got, excel, 1e-15); + } + } + + /// Edge cases observed in Excel: a rate of -1 or less is #NUM!, a huge nper leaves -pv*rate, and a + /// tiny rate no longer loses its low digits in 1 + rate. + #[test] + fn pmt_edge_cases_follow_excel() { + is_err(ev("PMT", vec![n(-2.0), n(3.0), n(100.0)]), CellError::Num); + is_err(ev("PMT", vec![n(-1.0), n(3.0), n(100.0)]), CellError::Num); + is_err(ev("PMT", vec![n(-2.0), n(2.5), n(100.0)]), CellError::Num); + assert_eq!(ev("PMT", vec![n(0.1), n(10000.0), n(100.0)]), Value::Number(-10.0)); + assert_eq!(ev("PMT", vec![n(5.0), n(1000.0), n(100.0)]), Value::Number(-500.0)); + assert_eq!(ev("PMT", vec![n(1e-300), n(10.0), n(100.0)]), Value::Number(-10.0)); + close_tol(ev("PMT", vec![n(1e-12), n(10.0), n(100.0)]), -10.000000000055001, 1e-15); + close_tol(ev("PMT", vec![n(-0.999), n(5.0), n(100.0)]), -9.990_000_000_000_092e-14, 1e-15); + close_tol(ev("PMT", vec![n(0.05), n(-10.0), n(100.0)]), 7.950_457_496_545_667, 1e-15); + close_tol(ev("PMT", vec![n(-1e-17), n(1.0), n(1e18), n(-1e18)]), 10.0, 1e-9); + close_tol(ev("PMT", vec![n(-0.5), n(60.0), n(1e18)]), -0.433_680_868_994_201_8, 1e-6); + close_tol(ev("PMT", vec![n(0.05), n(10.0), n(100.0), n(50.0), n(1.0)]), -16.119701185541427, 1e-15); + } + #[test] fn cash_flows() { close_tol(ev("NPV", vec![n(0.1), n(-10000.0), n(3000.0), n(4200.0), n(6800.0)]), 1188.443412, 1e-9); From b47c83f6aa3b730c9bc2fce74913ee1121f199cf Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:48:12 +0000 Subject: [PATCH 10/21] Reject unmatched '.' and ',' in NUMBERVALUE text Found while testing crates/functions/src/text.rs:326-340 Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- crates/functions/src/text.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/functions/src/text.rs b/crates/functions/src/text.rs index b9b06e608..2d7fca13a 100644 --- a/crates/functions/src/text.rs +++ b/crates/functions/src/text.rs @@ -334,6 +334,8 @@ fn numbervalue(a: &[Arg], c: &mut dyn Ctx) -> R { if seen_dec { return Err(CellError::Value); } + } else if c == '.' || c == ',' { + return Err(CellError::Value); } else { out.push(c); } @@ -1216,6 +1218,8 @@ mod tests { close(ev("NUMBERVALUE", vec![t(" 1 000 ")]), 1000.0); close(ev("NUMBERVALUE", vec![t("")]), 0.0); is_err(ev("NUMBERVALUE", vec![t("1.2.3")]), CellError::Value); + is_err(ev("NUMBERVALUE", vec![t("1.5"), t(","), t(" ")]), CellError::Value); + is_err(ev("NUMBERVALUE", vec![t("1,5"), t("."), t(" ")]), CellError::Value); is_text(ev("FIXED", vec![n(1234.567), n(1.0)]), "1,234.6"); is_text(ev("FIXED", vec![n(1234.567), n(-1.0)]), "1,230"); is_text(ev("FIXED", vec![n(-1234.567), n(1.0), b(true)]), "-1234.6"); From b389d9067a46539540bc6007b7bb81630d80bff9 Mon Sep 17 00:00:00 2001 From: Helen Kwok <107752876+helenkwok@users.noreply.github.com> Date: Sat, 24 Oct 2026 10:17:32 +0800 Subject: [PATCH 11/21] XLSX: write accounting formats 42 and 44 with their code builtin_id already wrote the currency formats 5-8 as custom formats because readers resolve those ids by locale. The accounting formats with a currency, 42 and 44, are resolved the same way but were written as bare ids, so LibreOffice showed a 44 cell holding -0.14 as "- 0.14" where US Excel shows "$ (0.14)". Treat them like 5-8. --- crates/xlsx/src/tables.rs | 7 +-- crates/xlsx/src/tests/locale_numfmt.rs | 61 ++++++++++++++++++++++++++ crates/xlsx/src/tests/mod.rs | 1 + 3 files changed, 66 insertions(+), 3 deletions(-) create mode 100644 crates/xlsx/src/tests/locale_numfmt.rs diff --git a/crates/xlsx/src/tables.rs b/crates/xlsx/src/tables.rs index 141d8673a..23c936c4a 100644 --- a/crates/xlsx/src/tables.rs +++ b/crates/xlsx/src/tables.rs @@ -46,10 +46,11 @@ pub fn builtin_format(id: u32) -> Option<&'static str> { BUILTIN_FORMATS.iter().find(|(i, _)| *i == id).map(|(_, c)| *c) } -/// Id to write for a format code when it is a locale-independent built-in (currency formats 5–8 -/// are written as custom formats because their meaning depends on the reader's locale). +/// Id to write for a format code when it is a locale-independent built-in. The currency formats +/// 5–8 and the accounting formats with a currency, 42 and 44, are written as custom formats because +/// their meaning depends on the reader's locale. pub fn builtin_id(code: &str) -> Option { - BUILTIN_FORMATS.iter().find(|(i, c)| *c == code && !(5..=8).contains(i)).map(|(i, _)| *i) + BUILTIN_FORMATS.iter().find(|(i, c)| *c == code && !matches!(i, 5..=8 | 42 | 44)).map(|(i, _)| *i) } pub use gridcraft_core::color::INDEXED; diff --git a/crates/xlsx/src/tests/locale_numfmt.rs b/crates/xlsx/src/tests/locale_numfmt.rs new file mode 100644 index 000000000..3259a3657 --- /dev/null +++ b/crates/xlsx/src/tests/locale_numfmt.rs @@ -0,0 +1,61 @@ +//! Number formats whose meaning depends on the reader's locale are written with their code: the +//! currency formats 5–8 and the accounting formats with a currency, 42 and 44. Written as a bare +//! built-in id, LibreOffice and Excel in other locales show their own version (a 44 cell holding +//! -0.14 reads `$ (0.14)` in US Excel but `- 0.14` in LibreOffice). + +use std::io::Read; +use std::sync::Arc; + +use gridcraft_core::{CellRef, Value}; +use gridcraft_model::*; + +use crate::tables::builtin_format; +use crate::{read_xlsx, write_xlsx}; + +fn styles_xml(bytes: &[u8]) -> String { + let mut zip = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap(); + let mut s = String::new(); + zip.by_name("xl/styles.xml").unwrap().read_to_string(&mut s).unwrap(); + s +} + +/// Writes one cell formatted with `code`; returns the `numFmtId` its cell format points at, the +/// `styles.xml` and the code read back. +fn round_trip(code: &str) -> (u32, String, String) { + let mut wb = Workbook::new(); + let style = wb.styles.derive(StyleId::DEFAULT, |s| s.num_fmt = NumFmt::new(code)); + let mut sheet = Sheet::new("Sheet1"); + sheet.cells.set(CellRef::new(0, 0), Cell { value: Value::Number(-0.14), formula: None, style }); + wb.sheets = vec![Arc::new(sheet)]; + let bytes = write_xlsx(&wb).unwrap(); + let xml = styles_xml(&bytes); + let xfs = &xml[xml.find("().unwrap()) + .find(|&id| id != 0) + .unwrap(); + let (back, _) = read_xlsx(&bytes).unwrap(); + let st = back.styles.get(back.sheet(0).unwrap().cell(CellRef::new(0, 0)).unwrap().style); + (id, xml, st.num_fmt.as_str().to_string()) +} + +#[test] +fn currency_and_accounting_formats_are_written_with_their_code() { + for builtin in [5, 6, 7, 8, 42, 44] { + let code = builtin_format(builtin).unwrap(); + let (id, xml, back) = round_trip(code); + assert!(id >= 164, "format {builtin} written as built-in id {id}"); + assert!(xml.contains(&format!(" for id {id}"); + assert_eq!(back, code, "format {builtin}"); + } +} + +#[test] +fn locale_independent_built_ins_keep_their_id() { + for builtin in [3, 4, 10, 14, 41, 43, 49] { + let (id, _, back) = round_trip(builtin_format(builtin).unwrap()); + assert_eq!(id, builtin); + assert_eq!(back, builtin_format(builtin).unwrap()); + } +} diff --git a/crates/xlsx/src/tests/mod.rs b/crates/xlsx/src/tests/mod.rs index c8f199a47..a7f6f1eb7 100644 --- a/crates/xlsx/src/tests/mod.rs +++ b/crates/xlsx/src/tests/mod.rs @@ -11,6 +11,7 @@ mod encrypted_tests; mod fixtures; mod ink_roundtrip; mod literal_category_roundtrip; +mod locale_numfmt; mod malformed; mod multilevel_category_roundtrip; mod note_box; From e93d15b50a00d87ff70ab1523bf4d604b3ead149 Mon Sep 17 00:00:00 2001 From: Helen Kwok <107752876+helenkwok@users.noreply.github.com> Date: Sat, 24 Oct 2026 10:17:32 +0800 Subject: [PATCH 12/21] Contributors: credit helenkwok as Helen Kwok --- contributors/people.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contributors/people.toml b/contributors/people.toml index 88e496fc7..43252507d 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -96,3 +96,6 @@ sync_github_name = true [people.tyrel] real_name = "Tyrel Haveman" + +[people.helenkwok] +real_name = "Helen Kwok" From 6ead1fb51936f7d368c09e8ce68806f51589b6c3 Mon Sep 17 00:00:00 2001 From: Pranav <53838599+prxnvt@users.noreply.github.com> Date: Sat, 24 Oct 2026 02:06:44 -0400 Subject: [PATCH 13/21] Engine: building blocks for multi-command actions (undo groups, recording buffer, document trust, chunked hidden names) (#462) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Engine: undo groups make several commands one undo step Session::grouped(label, f) runs f as one undo step on the active document. On success the group leaves exactly one undo entry holding the workbook from before the group; on error it restores the workbook, selection, undo and redo stacks as they were and returns the error. group_begin, group_end and group_rollback do the same across separate calls, for an action that runs one step per call. While a group is open on a document, undoable commands on it add no undo steps of their own, so a long group neither grows the history nor pushes older entries past the undo limit, and Undo and Redo are disabled there. Groups nest: the outermost one makes the step, and a failing inner group rolls back only its own changes. Closing a document drops its groups, and a command that panics drops the groups it opened. history.list reports whether a group is open. Insert Copied Cells folds its nested steps into one undo step by counting revisions; inside a group the nested steps push no entries, so it now leaves the history alone there instead of removing entries from before the group. * Engine: a recording buffer of top-level commands, apart from the journal A macro recorder needs more than a journal index: the journal is trimmed past 10,000 entries, it also holds the commands that other commands run (so playback would run them twice), and it keeps no selection, which relative references need. Session now tracks how deeply execute calls are nested. While recording, a buffer (record.rs) keeps each successful, journaled top-level call with the document uid, the active sheet (index and name) and the selection it ran in, then its id and parameters without secrets, as the journal stores them. Queries, failed commands and the command that starts recording are not kept. The buffer holds up to 100,000 steps and counts any it drops. Session::start_recording, stop_recording, is_recording and depth are the interface. Automate › Record Actions keeps recording through the journal as before. * Engine: document trust for workbooks opened from files Active content such as macros must not run only because a workbook was opened. DocState gets a trust state: Local for workbooks made in the session (new workbooks and samples) and FromFile for anything opened with file.open. Active content in a FromFile workbook asks before it runs. A second window on a workbook shares its trust. The clipboard carries the trust of its source document. Pasting formulas, formats (conditional formats hold formulas) or validation from a FromFile workbook into another workbook makes that workbook FromFile too; pasting values, column widths or notes carries nothing active and changes nothing. Sheet moves and copies stay inside one workbook, so its trust is unchanged. document.inspect reports the trust. * Engine: workbook data in chunked hidden names (cmd/stash.rs) GridCraft keeps workbook data in hidden defined names so it travels inside the xlsx file. The existing items write one name holding one string literal, which past Excel's 255-character literal and 8,192-character formula limits gives a file Excel does not accept. cmd/stash.rs stores an item `key` as hidden workbook-scope names _gc__0, _gc__1, ..., each a formula of string literals of at most 255 characters (UTF-16 units, as Excel counts them) joined with &, at most 8,192 characters per name. Reading concatenates the chunks in order and stops at a gap; it also accepts the older single _sc_ name. Names are matched case-insensitively and only at workbook scope. Keys are limited to letters, digits, _ and ., items to 4,194,304 characters, and junk around or inside the literals, duplicate or out-of-order chunks, non-JSON and over-deep JSON all read safely. keys(prefix) lists the stored items that start with a prefix. Custom views, scripts, the watch list and scenarios keep their current storage; nothing reads or writes the new names yet. docs/architecture.md describes the engine building blocks added in this series: undo groups, the recording buffer, document trust and this storage. * Engine: a linked paste of values from a file carries the file's trust A linked paste writes formulas even when it pastes values, but the values exception skipped the trust update, so a Local workbook stayed Local after linking to a FromFile one. The exception now applies only to unlinked pastes. * Engine: a stash item with only later chunks reads as missing and falls back to the legacy name With chunk _gc_k_2 but no _gc_k_0, get returned Some("") and never looked at _sc_k. It now takes the legacy path unless chunk 0 exists. * Engine: a command that runs its own undo groups makes one undo step for itself execute treated any change in the group epoch as 'the group owns the step'. A command that edited outside its own group, or whose group rolled back before it edited, left that edit with no undo step or only part of it. Now only a group that was open before the command (or is still open after) owns the step; a command that opened and closed its own groups replaces their steps with one entry for the command. * Engine: a full undo history keeps one step per command that runs its own undo groups When the history held 100 steps, group_end pushed its step and dropped the oldest one at once, so execute's collapse compared against a length that no longer matched. A command that was one group kept the group's step and added its own, losing two old steps. After test.editThenGroup, the second Undo brought back the intermediate state after the outside edit. The history limit now applies after the outermost command has collapsed its steps, and when a group ends outside any command. A document with an open group is trimmed when the group ends. * Engine: pasting only formats from a file leaves the destination's trust alone Paste Special › Formats copies cell styles and merged areas. It copies no conditional formats, formulas or validation, so it carries nothing that runs. It still marked the destination FromFile: copying a formula cell from an opened file and pasting its formats into a new workbook made that workbook ask before running its own active content. Formats pastes now count as passive, like values, column widths and notes. The comment and docs/architecture.md no longer say that pasted formats carry trust. * Engine: a command keeps its group's name only for a group it ended itself The record of the last group step (document, history length, epoch) outlived the Undo of that step. A later command that opened and closed its own groups at the same history length, after rolling one back, passed the epoch check (the record's epoch merely differed from the one the command started with) and took the name of the nested edit it had made instead of its own. The step must now come from a group ended during the command (an epoch past the command's starting one), and a rollback drops a record of a step it truncated away. * Engine: a group rolled back inside a command keeps the redo history A command that changed the workbook cleared the redo history even when an open group owned its step. When the group was then rolled back inside the same command (the debugger's Stop with Undo Changes), the rollback put the redo history back and the command's bookkeeping cleared it again. Now only a command that makes a step of its own clears the redo history; a group clears it when it ends as a step, and a rollback restores it. * Engine: a visible name of the item shape is not workbook data The item store read, listed and removed every workbook-scope name of the `_gc__` shape, visible or not, so saving an item removed a person's own visible name that happened to look like one. Items are the hidden names the store writes; a legacy `_sc_` name still reads. A visible `_gc_` name is left alone by every operation, and writing an item that would need that name is refused before anything changes: a hidden name beside it with the same name is the one an xlsx file drops, so the item would be lost on save. * Engine: a command keeps its group's name only for a step that is exactly the group's The record of the last group step held the document, the history length and a group epoch, and a command reused the group's label when the lengths matched and the epoch was newer. A direct edit after the group (no nested command, so the length still matches) then left the step named after the group alone. The record now holds the workbook the group's step led to (a weak reference, so it keeps nothing alive). A command keeps the group's name only when the one entry it added starts from the command's own starting workbook and ends at its result, which is provenance by identity rather than by position: an entry a nested command made, a step of an earlier command undone since, and a group step the command edited past all name the step after the command. A later group that rolled back changes nothing, and the rollback no longer needs to clear the record. * Engine: the recording buffer is bounded in bytes and keeps the start of a recording The buffer kept at most 100,000 steps but each step keeps its full parameters, and a file.open step holds the whole file as base64, so a recording of a few opens of a large workbook kept tens of MB alive. Like the journal, the buffer now also stops at 64 MB of steps serialized as JSON. A step that does not fit is counted as dropped, and once a step is dropped every later one is too, so what the buffer holds is always how the recording began, never a sequence with gaps. * Docs: the architecture revision row carries the date the change lands; document trust beside script trust The header and the revision row said 2026-10-11, older than the row above them. The document trust paragraph now says how DocState::trust (provenance) differs from DocState::scripts_trusted (per-session consent for stored scripts, #2178). * Engine: writing a stash item checks for a clashing visible name in one pass over the names The check ran once per chunk, each a scan of every defined name: an item of many chunks in a workbook with thousands of names compared every pair. One pass now parses each visible workbook-scope name and refuses when it is a chunk of this key below the chunk count. The behaviour and its test are unchanged. * Engine: a command's changes after ending a group, or before opening one it leaves open, are not lost or a step of their own Two boundaries of the group rules were wrong. A command that ended a group open before it was treated as grouped whatever it did next: with an unchanged group (no step) its later edit had no undo entry at all, and with nested edits after the end the entry was the nested command's. The group's step now stays the group's, and the command makes one step of its own, under its name, for what changed after the end, from the workbook the group ended at. A command that edited and then opened a group it leaves open (a debugger's start) left its nested entries in the history, so one Undo after the group ended brought back a state from inside the command. Such a group now takes the command's start as its own, and its step covers the command's earlier edits. The record of where the last group ended now holds the workbook and selection it ended at, the history length then and whether it made a step; it lives until the outermost command finishes. A rollback records where it ended too, unless the document is back exactly where the last recorded step left it. * Engine: the undo steps made inside a command are bounded while it runs The history limit is applied once the outermost command has finished, so the lengths its bookkeeping compares stay true (round 3). That left the steps nested commands make unbounded until then: a script of up to 100,000 steps, or a subtotal with a step per group, kept every one of them alive, each with the cells it retains, and before this branch every push trimmed the history at once. Each nested command now bounds the steps made since the latest mark on its document to the undo limit and byte budget, dropping the oldest of them. The marks are the starts of the running commands, the starts of the open groups and the end of the last group: the positions a command or group collapses its steps by when it finishes, so nothing before them moves. What the bound drops is only what the final trim would drop. * Engine: a grouped action that closes its document succeeds Session::grouped ended its group after the action, and when the action had closed the group's document (file.close, or file.open taking the place of an untouched new workbook) the group had gone with it, so the action was reported as failed although it had done what it was asked. A group that is gone because its document is gone now counts as ended; a group ended by someone else while the document is open is still an error. * Docs: undo groups at a command's boundaries, and the bound on steps made inside a command * Engine: a linked paste carries the file's trust whatever it pastes Paste Link writes a formula into every cell whatever `what` says, so a linked paste of formats from a file now makes the destination FromFile as a paste of formulas does. Column widths and notes write no cells, linked or not, and still carry nothing. Regression test for the three kinds. * Engine: a stash chunk number with leading zeros names someone else's name `_gc_watch_00` parsed as chunk 0, so a hidden one was read as the item's first chunk and a visible one stopped `_gc_watch_0` from being written. Only the chunk number as `set` writes it counts now. Regression test. * Engine: a command that panics leaves the undo history as it found it The panic guard restored the workbook and selection but not the history, so a nested edit made before the panic left a no-op undo step and the redo history it had cleared stayed lost. The guard now truncates the steps made during the command and puts the redo history back. Regression test, and the architecture note says what the guard restores. * Engine: a headless call that fails for want of a file is not recorded file.open with no file succeeds by queuing a dialog, which execute_headless turns into an error for callers with no UI; the recording kept the step. Recorder::truncate forgets it and keeps the byte count right. Regression test. * Engine: a later stash chunk on its own is not a listed item stash::keys reported a key that get read as missing, because only a later chunk of it existed. keys now counts chunk 0 and legacy names only, as get does. Regression assertions in the hostile-names test. * Tests: a linked paste of column widths or notes copies its metadata and no cell The source now carries a note and a column width, and the test checks that the destination received them while its trust stays Local and no cell is written. * Engine: the panic guard restores entries a nested Undo removed Truncating the undo history to its length at the command's start cannot bring back an entry that a nested edit.undo popped. The guard now keeps the undo and redo history as they were and puts both back. The regression test gains the undo-then-panic case. * Engine: a group a panicking command ended keeps its step A group open before a command and ended or rolled back inside it lost its undo step when the command then panicked: the restored history held no entry for the group's changes, so Undo skipped the state before the group. The panic path now pushes the outermost such group's step from its start to the restored workbook. The group stays ended, because its token went with the command. Regression test for a group ended and one rolled back. * Docs: list every paste that carries a file's trust The architecture note named formulas and validation only; it now lists the Paste Special kinds and linked pastes that write formulas, and the kinds that carry nothing. * Engine: a grouped action that panics is rolled back before the panic goes on Session::grouped ran its action without a panic guard, so a caller outside a command that caught the panic found the group still open: Undo and Redo disabled, the partial edits kept, and the next grouped action nested in the abandoned group. The group is now rolled back and the panic resumed. Regression test. * Engine: the recorder counts a dropped step without serializing it Once a step has been dropped, or the step count is full, push serialized each later step only to count it; a file.open step holds a whole file. The checks that need no size come first. * Engine: a failed command rolls back the undo groups it opened A command that opened an undo group and then returned an error (a `?` between `group_begin` and `group_end`) left the group open for good: Undo and Redo refused with "finish or stop the running action first", later edits made no undo steps, and nothing could stop the group. `execute` now rolls back every group a failed command opened, as `group_rollback` does, restoring the workbook, selection, undo and redo history to where each group began. Only a command that succeeds keeps a group it opened, which is what a debugger's start needs. `Session::abort_groups(doc)` and the menu-less command `automate.stopAction` roll back the groups left open on a document, for a debugger the person abandons or a caller that dropped its token. The command is not undoable and works while a group is open, and the Undo refusal now names it. * Engine: the recorder skips a call that only opened a dialog A menu-style call with no parameters, such as `file.open`, `data.fromTextCsv` or `home.formatCells` with `{}`, only queues its dialog and succeeds. The recorder kept it as a step with empty parameters, and then kept the dialog's confirming call as well, so playing the recording back ran the command twice, or failed on the empty step when played back headless. Only `execute_headless` took the step back, and only for the open and save-as dialogs. `execute` now compares the queued UI requests before and after a top-level command and does not record one that queued a dialog. The special case in `execute_headless` goes (it keeps turning a missing file into an error), and with it `Recorder::truncate`, which had no other caller. * Engine: stash items keep characters XML can't carry The xlsx writer drops characters XML 1.0 can't carry: controls other than tab, line feed and carriage return, and U+FFFE and U+FFFF. A stash item holding one lost it on save without a warning, and so did a JSON item, since serde_json leaves U+FFFE and U+FFFF unescaped: a stored macro parameter changed silently. `chunk_formulas` now writes each such character, and the marker itself, as the noncharacter U+FDD0 followed by four hex digits, and `get` turns them back for `_gc_` items. A marker that isn't followed by four hex digits naming a character is read as it is. An escape counts as five units against the 255-unit literal limit and is never split across literals, and the guard on the stored size allows the longest item even when every character is escaped. Legacy `_sc_` items are read unchanged. * Engine: list stash items in one pass over the names Each `stash::get` looks at every defined name, so listing the items with `keys` and then reading each with `get` took time quadratic in the number of names. A file with 20,000 hidden `_gc_macro__0` names took about ten seconds in a debug build, and the macro list reads its items this way from data that comes from the file. `stash::items(wb, prefix)` now groups the names by item in one pass and returns each item's text by key, and `keys` lists from the same pass. Both return at most `MAX_ITEMS` (10,000) items, the first keys in sorted order, so a file with any number of hidden names can't make a listing unbounded. A test counts the names each listing looks at. * Engine: the name commands leave GridCraft's data names alone `formulas.defineName {"name": "_gc_macro_1_0"}` replaced the hidden chunk of a stored item with a visible name: the item vanished from `stash::get` and `stash::keys`, and its other chunks stayed behind as orphan hidden names. `formulas.deleteName` could remove chunks the same way, and `formulas.createFromSelection` with `replace` could replace one from a cell label. `stash::set` refused a clash with a visible name, but nothing guarded the other direction. Defining, renaming or deleting a name that `stash::item_key` recognises (`_gc__` and legacy `_sc_`) is now refused with an error that names `force: true`, which lets the call go ahead. Create from Selection lists such labels in `skipped` unless `force` is true. The params docs of the three commands say so. * Engine: queries take no copy of the undo history `execute` copied the document's undo and redo history on every call, so a panic could put it back: up to 100 entries per stack, each with a label and a selection. Queries paid for it too, although no query runs a command or changes the history, and front ends and agents run queries all the time. The copy is now skipped for queries (commands that neither journal nor make undo steps). A test runs every query, with empty and with typical parameters, and checks the history is unchanged, so a query that ever changes it fails there. Commands that can change the history, nested ones included, still take the copy: a nested command that panics is restored at its own level, and the outer command can carry on, so relying on the outermost copy would weaken that restore. * Engine: a document's maker says where its content came from `Trust` defaulted to `Local`, and `DocState::new` set it, so a document was trusted unless its maker remembered to mark it. Every current path that reads a file goes through `file.open`, which marks it `FromFile`, but a later way of building a document from bytes (recovery, templates, a new import) would have come out trusted by omission. `DocState::new` now takes the `Trust`, and `Trust` has no default. New workbooks and samples pass `Local`, `file.open` passes `FromFile`, a new window passes the trust of its workbook, and the tests that build documents from bytes pass `FromFile`. * Engine: the name commands refuse only names that hold GridCraft's data The guard refused any name of the stash's shape, so a visible name a person had defined, such as _gc_rate_0, could not be edited or deleted from Name Manager, which cannot pass force. It now refuses a name only when the workbook holds a hidden workbook-scope item under it; a visible name of the shape is the person's own, as the stash itself treats it. The test covers such a name. The query comment says what the copy skip relies on: a query runs nothing that changes the history. * UI: the toolbar's Undo and Redo follow the commands' own availability The ribbon enabled them from the history lengths alone, so while an undo group was open on the document (an action spanning several calls) a click ran edit.undo and got its refusal. The buttons now ask cmd::can_undo and cmd::can_redo, the undo list is empty while a group is open, and the two buttons carry a name and their enabled state for assistive technology. A UI test opens a group, clicks both and checks the history and their state. * Engine: the name commands use the stash's own rule for what holds its data The guard asked for a hidden name, but the stash reads a legacy _sc_ name whether it is hidden or not, so a visible one could be removed or replaced without force. stash::holds_item is the one rule (item_of), shared by the guard; the test covers a visible legacy name. * Docs: the panic guard's note says what happens to a group the command ended It keeps a step from the group's start to the restored workbook, so Undo still returns to the state before the group; the note said the history came back exactly. --- crates/engine/src/cmd/edit.rs | 12 +- crates/engine/src/cmd/extra.rs | 18 +- crates/engine/src/cmd/file.rs | 5 +- crates/engine/src/cmd/formulas.rs | 97 +- crates/engine/src/cmd/inspect.rs | 13 +- crates/engine/src/cmd/mod.rs | 25 + crates/engine/src/cmd/sheet.rs | 8 +- crates/engine/src/cmd/stash.rs | 606 ++++++++++ crates/engine/src/lib.rs | 448 ++++++- crates/engine/src/record.rs | 66 ++ crates/engine/src/sample.rs | 2 +- crates/engine/src/tests.rs | 6 + crates/engine/src/tests/seams.rs | 1052 +++++++++++++++++ crates/engine/tests/it/dynamicmetadata.rs | 2 +- .../engine/tests/it/long_operator_chains.rs | 2 +- crates/engine/tests/it/stored_list_shape.rs | 3 +- crates/ui-egui/src/ribbon.rs | 22 +- crates/ui-egui/tests/it/interaction.rs | 2 +- crates/ui-egui/tests/it/main.rs | 1 + crates/ui-egui/tests/it/scroll_position.rs | 2 +- .../ui-egui/tests/it/undo_while_grouping.rs | 57 + docs/architecture.md | 51 +- 22 files changed, 2461 insertions(+), 39 deletions(-) create mode 100644 crates/engine/src/cmd/stash.rs create mode 100644 crates/engine/src/record.rs create mode 100644 crates/engine/src/tests/seams.rs create mode 100644 crates/ui-egui/tests/it/undo_while_grouping.rs diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 352797eab..edb0e2d7c 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -741,7 +741,7 @@ fn copy_impl(s: &mut Session, p: &Json, cut: bool) -> Result { // The HTML rendering is for the host clipboard (UI); it can reach 4 MB, so agents and // scripts get it only when they ask for it. let html = if p.get("html").and_then(Json::as_bool) == Some(true) { crate::io::range_html(&wb, sheet, range, &text) } else { None }; - let clip = Clipboard { wb, sheet, range, cut, doc_uid: d.uid, text: text.clone(), areas, selected }; + let clip = Clipboard { wb, sheet, range, cut, doc_uid: d.uid, text: text.clone(), areas, selected, trust: d.trust }; s.clipboard = Some(clip); let mut result = json!({"range": range.a1(), "text": text}); if let Some(html) = html { @@ -1264,6 +1264,16 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { *cx.sel = crate::Selection { active: at, anchor: at, ranges: vec![RangeRef::new(at, end)] }; Ok(()) })?; + // Formulas and validation from an untrusted file leave the destination untrusted; plain values, + // formats (cell styles and merged areas, no conditional formats), column widths and notes + // carry nothing active. A linked paste writes a formula into every cell whatever `what` says, + // except for column widths and notes, which write no cells at all. + let passive = + matches!(what, What::ColumnWidths | What::Comments) || (!link && matches!(what, What::Formats | What::Values | What::ValuesAndNumberFormats)); + if !same_doc && !passive { + let d = s.doc_mut()?; + d.trust = d.trust.receive(clip.trust); + } if cut_move { s.clipboard = None; } diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs index f248c59f2..009d77f2a 100644 --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -212,6 +212,15 @@ pub fn specs() -> Vec { trust_scripts ), cmd!("automate.deleteScript", "Delete Script", [], None, "{name}", has_doc, delete_script), + cmd!( + noundo "automate.stopAction", + "Stop Action", + [], + None, + "{} → {stopped: bool}: stops an action left running on the active workbook (an open undo group, such as a debugger's) and puts back the workbook and its undo history as they were when it began", + has_doc, + stop_action + ), // Cell controls, themes, borders, styles cmd!( "insert.checkbox", @@ -800,7 +809,7 @@ fn split(s: &mut Session, _: &Json) -> Result { fn new_window(s: &mut Session, _: &Json) -> Result { let d = s.doc()?.clone(); - let mut copy = crate::DocState::new((*d.wb).clone(), d.path.clone(), format!("{}:2", d.display_title())); + let mut copy = crate::DocState::new((*d.wb).clone(), d.path.clone(), format!("{}:2", d.display_title()), d.trust); copy.selection = d.selection.clone(); copy.scripts_trusted = d.scripts_trusted; let i = s.add_document(copy); @@ -1087,6 +1096,13 @@ fn run_script(s: &mut Session, p: &Json) -> Result { Ok(json!({"ran": n})) } +/// Automate › Stop Action: rolls back the undo groups open on the active workbook, so Undo and +/// Redo work there again. +fn stop_action(s: &mut Session, _: &Json) -> Result { + let uid = s.doc()?.uid; + Ok(json!({"stopped": s.abort_groups(uid)})) +} + fn delete_script(s: &mut Session, p: &Json) -> Result { let name = str_param(p, "name").ok_or_else(|| bad("automate.deleteScript", "missing `name`"))?.to_string(); edit(s, |cx| { diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index a61c6298f..b3e7309e2 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -34,7 +34,7 @@ fn new_workbook(s: &mut Session, p: &Json) -> Result { Some(name) => { let wb = crate::sample::build(name).ok_or_else(|| bad("file.new", format!("unknown sample `{name}`")))?; let title = crate::sample::title(name); - s.add_document(DocState::new(wb, None, title)) + s.add_document(DocState::new(wb, None, title, crate::Trust::Local)) } None => s.new_workbook(), }; @@ -83,7 +83,8 @@ fn open(s: &mut Session, p: &Json) -> Result { { s.close_document(0); } - let mut doc = DocState::new(wb, path, title); + // Someone else may have made the file: its active content asks before it runs. + let mut doc = DocState::new(wb, path, title, crate::Trust::FromFile); if crate::io::FileKind::from_path(&name).is_none() && gridcraft_xlsx::sniff(&bytes) == gridcraft_xlsx::Format::Xlsx { doc.content_kind = Some(crate::io::FileKind::Xlsx); } diff --git a/crates/engine/src/cmd/formulas.rs b/crates/engine/src/cmd/formulas.rs index 5334784fa..7718279b2 100644 --- a/crates/engine/src/cmd/formulas.rs +++ b/crates/engine/src/cmd/formulas.rs @@ -1,7 +1,7 @@ //! Formulas tab: AutoSum, Insert Function, names, auditing, calculation. use gridcraft_core::{CellError, CellRef, RangeRef, Value}; -use gridcraft_model::{CalcMode, DefinedName, Formula}; +use gridcraft_model::{CalcMode, DefinedName, Formula, Workbook}; use serde_json::{Value as Json, json}; use super::*; @@ -32,18 +32,26 @@ pub fn specs() -> Vec { "Define Name…", ["Formulas", "Defined Names"], None, - "{name, refersTo?: \"=Sheet1!$A$1:$A$10\" (default: the selection), scope?: \"Workbook\"|sheet name, comment?, oldName?, oldScope?} (oldName: edit that name (Name Manager › Edit); a new name also renames it in formulas)", + "{name, refersTo?: \"=Sheet1!$A$1:$A$10\" (default: the selection), scope?: \"Workbook\"|sheet name, comment?, oldName?, oldScope?, force?: bool} (oldName: edit that name (Name Manager › Edit); a new name also renames it in formulas; a hidden name GridCraft keeps its own data in, `_gc__` or `_sc_`, is refused unless force is true)", has_doc, define_name ), - cmd!("formulas.deleteName", "Delete Name", [], None, "{name, scope?: \"Workbook\"|sheet name (default: every scope)}", has_doc, delete_name), + cmd!( + "formulas.deleteName", + "Delete Name", + [], + None, + "{name, scope?: \"Workbook\"|sheet name (default: every scope), force?: bool} (a hidden name GridCraft keeps its own data in, `_gc__` or `_sc_`, is refused unless force is true)", + has_doc, + delete_name + ), cmd!(query "formulas.nameManager", "Name Manager", ["Formulas", "Defined Names"], Some("Cmd+F3"), "{} → names with values", has_doc, name_manager), cmd!( "formulas.createFromSelection", "Create from Selection", ["Formulas", "Defined Names"], Some("Cmd+Shift+F3"), - "{range?, top?: true, left?: false, bottom?: false, right?: false, replace?: false (existing names are kept and listed in `skipped`)}", + "{range?, top?: true, left?: false, bottom?: false, right?: false, replace?: false (existing names are kept and listed in `skipped`), force?: false (labels of the shape GridCraft keeps its own data in, `_gc__` and `_sc_`, are listed in `skipped` unless force is true)}", has_doc, create_from_selection ), @@ -268,6 +276,9 @@ fn define_name(s: &mut Session, p: &Json) -> Result { if !valid_name(&name) { return Err(EngineError::Other("The name that you entered is not valid.".into())); } + for n in [Some(name.as_str()), str_param(p, "oldName")].into_iter().flatten() { + refuse_stash_name(&s.doc()?.wb, p, n)?; + } let d = s.doc()?; let scope = match str_param(p, "scope") { None | Some("Workbook") => None, @@ -374,8 +385,29 @@ fn rename_name_in_formulas(wb: &mut gridcraft_model::Workbook, old: &str, old_sc } } +/// Refuses a name of the shape GridCraft keeps its own workbook data in ([`super::stash`]): +/// defining, renaming or deleting one would replace or remove part of an item (a stored macro, +/// script or view) and leave the rest behind. `force: true` lets the call go ahead. +/// Whether `name` is one the workbook holds GridCraft's own data in: a hidden workbook-scope +/// name of an item's shape. A visible name of that shape is the person's own (see `stash`), and +/// the name commands may change it. +fn holds_gridcraft_data(wb: &Workbook, name: &str) -> bool { + // The stash's own rule, so the two never disagree: a legacy `_sc_` item is read visible or not. + stash::holds_item(wb, name) +} + +fn refuse_stash_name(wb: &Workbook, p: &Json, name: &str) -> Result<()> { + if holds_gridcraft_data(wb, name) && !bool_param(p, "force").unwrap_or(false) { + return Err(EngineError::Other(format!( + "'{name}' is a name GridCraft keeps its own data in (stored macros, scripts or views). Pass `force: true` to change it anyway." + ))); + } + Ok(()) +} + fn delete_name(s: &mut Session, p: &Json) -> Result { let name = str_param(p, "name").ok_or_else(|| bad("formulas.deleteName", "missing `name`"))?.to_string(); + refuse_stash_name(&s.doc()?.wb, p, &name)?; let scope = match str_param(p, "scope") { None => None, Some("Workbook") => Some(None), @@ -461,7 +493,11 @@ fn create_from_selection(s: &mut Session, p: &Json) -> Result { // Excel asks before replacing an existing name; here it's kept unless `replace` is set, and // reported. let exists = |n: &str| d.wb.names.iter().any(|x| x.name.eq_ignore_ascii_case(n) && x.scope.is_none()); - let (made, skipped): (Vec<_>, Vec<_>) = made.into_iter().partition(|(n, _)| replace || !exists(n)); + // A label that names one of GridCraft's own hidden data names would replace part of an item + // (see `refuse_stash_name`). + let force = bool_param(p, "force").unwrap_or(false); + let ours = |n: &str| !force && holds_gridcraft_data(&d.wb, n); + let (made, skipped): (Vec<_>, Vec<_>) = made.into_iter().partition(|(n, _)| !ours(n) && (replace || !exists(n))); let skipped: Vec = skipped.into_iter().map(|(n, _)| n).collect(); let n = made.len(); edit(s, |cx| { @@ -742,6 +778,57 @@ mod tests { assert_eq!(r["refersTo"], "=Sheet1!$A$1"); } + #[test] + fn the_name_commands_leave_gridcraft_data_alone() { + // Defining, renaming or deleting a name of a stored item's shape would replace or remove + // one of its chunks and leave the rest behind: refused unless `force` is true. + let mut s = Session::new(); + s.new_workbook(); + let wb = std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb); + let text = "m".repeat(9_000); + stash::set(wb, "macro_1", &text).unwrap(); + wb.names.push(DefinedName { name: "_sc_watch".into(), scope: None, formula: "\"[]\"".into(), comment: String::new(), hidden: true }); + // A legacy item is read whether or not it is hidden, so a visible one is data too. + wb.names.push(DefinedName { name: "_sc_old".into(), scope: None, formula: "\"[]\"".into(), comment: String::new(), hidden: false }); + let names = |s: &Session| s.doc().unwrap().wb.names.clone(); + let before = names(&s); + assert!(before.len() >= 3, "the item takes two chunks"); + for params in [ + json!({"name": "_gc_macro_1_0", "refersTo": "=1"}), + json!({"name": "_SC_Watch", "refersTo": "=1"}), + json!({"name": "Mine", "refersTo": "=1", "oldName": "_gc_macro_1_1"}), + json!({"name": "_gc_macro_1_0", "refersTo": "=1", "force": false}), + json!({"name": "Mine2", "refersTo": "=1", "oldName": "_SC_old"}), + ] { + let err = s.execute("formulas.defineName", params.clone()).unwrap_err().to_string(); + assert!(err.contains("force"), "{params}: {err}"); + } + for name in ["_gc_macro_1_1", "_sc_watch", "_sc_old"] { + let err = s.execute("formulas.deleteName", json!({"name": name})).unwrap_err().to_string(); + assert!(err.contains("force"), "{name}: {err}"); + } + s.execute("range.setValues", json!({"range": "A1:B2", "values": [["_gc_macro_1_0", "Rate"], [1, 2]]})).unwrap(); + let r = s.execute("formulas.createFromSelection", json!({"range": "A1:B2", "top": true, "replace": true})).unwrap(); + assert_eq!((r["created"].clone(), r["skipped"].clone()), (json!(1), json!(["_gc_macro_1_0"]))); + let after = names(&s); + assert_eq!(after.iter().filter(|n| n.name != "Rate").cloned().collect::>(), before); + assert_eq!(stash::get(&s.doc().unwrap().wb, "macro_1").as_deref(), Some(text.as_str())); + // A visible name of the shape is the person's own (the stash never reads it): the name + // commands change and remove it without `force`, as Name Manager cannot pass one. + let own = DefinedName { name: "_gc_rate_0".into(), scope: None, formula: "=1".into(), comment: String::new(), hidden: false }; + std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).names.push(own.clone()); + s.execute("formulas.defineName", json!({"name": "Rate0", "refersTo": "=2", "oldName": "_gc_rate_0"})).unwrap(); + s.execute("formulas.deleteName", json!({"name": "Rate0"})).unwrap(); + std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).names.push(own); + s.execute("formulas.deleteName", json!({"name": "_gc_rate_0"})).unwrap(); + assert_eq!(stash::get(&s.doc().unwrap().wb, "macro_1").as_deref(), Some(text.as_str()), "the hidden item is untouched"); + // Ordinary names and forced calls go ahead. + s.execute("formulas.deleteName", json!({"name": "Rate"})).unwrap(); + s.execute("formulas.defineName", json!({"name": "_gc_macro_1_1", "refersTo": "=1", "force": true})).unwrap(); + s.execute("formulas.deleteName", json!({"name": "_gc_macro_1_1", "force": true})).unwrap(); + assert!(!names(&s).iter().any(|n| n.name == "_gc_macro_1_1")); + } + #[test] fn arabic_defined_names_count_characters_not_bytes() { let mut s = Session::new(); diff --git a/crates/engine/src/cmd/inspect.rs b/crates/engine/src/cmd/inspect.rs index f373de489..f5b478ccf 100644 --- a/crates/engine/src/cmd/inspect.rs +++ b/crates/engine/src/cmd/inspect.rs @@ -7,11 +7,11 @@ use super::*; pub fn specs() -> Vec { vec![ - cmd!(query "document.inspect", "Inspect Workbook", [], None, "{} → sheets, selection, names, tables, charts, dirty, history", has_doc, inspect), + cmd!(query "document.inspect", "Inspect Workbook", [], None, "{} → sheets, selection, names, tables, charts, dirty, trust (local|fromFile), history", has_doc, inspect), cmd!(query "sheet.read", "Read Range", [], None, "{range?: \"A1:D10\" (default: used range), sheet?, formulas?: bool, formatted?: bool} → rows of values", has_doc, read), cmd!(query "cell.get", "Get Cell", [], None, "{cell?, sheet?} → value, formula, display text, style, comment, link", has_doc, cell_get), cmd!(query "selection.stats", "Selection Statistics", [], None, "{} → count, sum, average, min, max", has_doc, sel_stats), - cmd!(query "history.list", "Undo History", [], None, "{} → undo/redo labels", has_doc, history), + cmd!(query "history.list", "Undo History", [], None, "{} → undo/redo labels, grouping (an undo group is open)", has_doc, history), cmd!(query "app.commands", "List Commands", [], None, "{search?} → ids, labels, params", always, list_commands), ] } @@ -63,6 +63,7 @@ fn inspect(s: &mut Session, _: &Json) -> Result { "title": d.display_title(), "path": d.path, "dirty": d.is_dirty(), + "trust": d.trust, "activeSheet": wb.active_sheet, "sheets": sheets, "selection": d.selection.a1(), @@ -148,9 +149,11 @@ fn sel_stats(s: &mut Session, _: &Json) -> Result { fn history(s: &mut Session, _: &Json) -> Result { let d = s.doc()?; - Ok( - json!({"undo": d.undo.iter().rev().map(|e| e.label.clone()).collect::>(), "redo": d.redo.iter().rev().map(|e| e.label.clone()).collect::>()}), - ) + Ok(json!({ + "undo": d.undo.iter().rev().map(|e| e.label.clone()).collect::>(), + "redo": d.redo.iter().rev().map(|e| e.label.clone()).collect::>(), + "grouping": s.in_group(d.uid), + })) } fn list_commands(s: &mut Session, p: &Json) -> Result { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 1f15ddfb8..8a0bb1052 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -19,6 +19,7 @@ pub mod sheet; pub mod slicer; pub mod solver; pub mod spelling; +pub mod stash; pub mod view; pub mod whatif; @@ -96,12 +97,19 @@ pub fn has_doc(s: &Session) -> std::result::Result<(), String> { } pub fn can_undo(s: &Session) -> std::result::Result<(), String> { has_doc(s)?; + not_grouping(s)?; if s.active().is_some_and(|d| !d.undo.is_empty()) { Ok(()) } else { Err("nothing to undo".into()) } } pub fn can_redo(s: &Session) -> std::result::Result<(), String> { has_doc(s)?; + not_grouping(s)?; if s.active().is_some_and(|d| !d.redo.is_empty()) { Ok(()) } else { Err("nothing to redo".into()) } } +/// Undo and Redo wait while an undo group is open on the active document (an action that spans +/// several calls is still running). +fn not_grouping(s: &Session) -> std::result::Result<(), String> { + if s.active().is_some_and(|d| s.in_group(d.uid)) { Err("finish or stop the running action first (automate.stopAction)".into()) } else { Ok(()) } +} pub fn has_clipboard(s: &Session) -> std::result::Result<(), String> { has_doc(s)?; if s.clipboard.is_some() { Ok(()) } else { Err("the clipboard is empty".into()) } @@ -179,6 +187,23 @@ pub fn register_test_command(id: &'static str, run: Run) { test_commands().push(spec); } +/// Test-only: an undoable command, to exercise how `execute` records its step. +#[cfg(test)] +pub(crate) fn register_undoable_test_command(id: &'static str, run: Run) { + let spec: &'static CommandSpec = Box::leak(Box::new(CommandSpec { + id, + label: id, + menu: &[], + shortcut: None, + params: "", + enabled: |_| Ok(()), + run, + journal: false, + undoable: true, + })); + test_commands().push(spec); +} + fn test_commands() -> std::sync::MutexGuard<'static, Vec<&'static CommandSpec>> { #[allow(clippy::unwrap_used)] // a poisoned test registry has no meaningful recovery match TEST_COMMANDS.lock() { diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index 555c2fb79..a453f250b 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -586,11 +586,15 @@ fn insert_copied_cells(s: &mut Session, p: &Json) -> Result { } let revision = d.revision; let before = (d.wb.clone(), d.selection.clone()); + // Inside an undo group the nested steps add no undo entries of their own. + let nested_entries = !s.in_group(d.uid); let result = insert_copied_steps(s, &clip, sheet, at, shift, cut); let d = s.doc_mut()?; // The nested steps collapse into this command's single undo step. - for _ in 0..d.revision.saturating_sub(revision) { - d.undo.pop(); + if nested_entries { + for _ in 0..d.revision.saturating_sub(revision) { + d.undo.pop(); + } } match result { Ok((final_range, out)) => { diff --git a/crates/engine/src/cmd/stash.rs b/crates/engine/src/cmd/stash.rs new file mode 100644 index 000000000..c0e8302ca --- /dev/null +++ b/crates/engine/src/cmd/stash.rs @@ -0,0 +1,606 @@ +//! GridCraft data kept in hidden workbook-scope defined names, so that it travels inside the xlsx +//! file. +//! +//! An item `key` is stored as names `_gc__0`, `_gc__1`, …, each a formula of string +//! literals joined with `&`: `"…"&"…"`. Excel limits a string literal to 255 characters and a +//! formula to 8,192, so a long value is split over as many names as it needs and the file stays +//! one that Excel accepts. Older items (custom views, scripts, the watch list and scenarios) keep +//! one name each, `_sc_`; reading accepts that form as well, so those items can move to this +//! format later, and a write of the same key replaces it. +//! +//! XML can't carry every character (controls other than tab, line feed and carriage return, and +//! U+FFFE and U+FFFF), and the xlsx writer drops those. In `_gc_` items each of them, and the +//! marker itself, is written as [`ESCAPE`] followed by its code as four upper-case hex digits +//! (`U+0001` becomes `\u{FDD0}0001`), and reading turns them back. The marker is a Unicode +//! noncharacter, which XML carries but real text does not use, so the escape costs nothing on +//! ordinary text. Legacy `_sc_` items are read as they are. + +use std::collections::BTreeMap; +use std::fmt::Write as _; + +use gridcraft_model::{DefinedName, Workbook}; +use serde_json::Value as Json; + +use crate::{EngineError, Result}; + +const PREFIX: &str = "_gc_"; +const LEGACY_PREFIX: &str = "_sc_"; +/// Characters (UTF-16 units, as Excel counts them) in one string literal. +const LITERAL_UNITS: usize = 255; +/// Characters in one name's formula. +const FORMULA_UNITS: usize = 8_192; +/// The longest key: `_gc__` stays well inside Excel's 255-character names. +const MAX_KEY: usize = 200; +/// The longest item: 4,194,304 characters. +pub const MAX_ITEM_CHARS: usize = 4 * 1024 * 1024; +/// The most bytes the stored (escaped) form of an item can take: an escaped one-byte character +/// takes seven (the three-byte marker and four digits). +const MAX_STORED_BYTES: usize = MAX_ITEM_CHARS * 7; +/// Starts an escaped character in a `_gc_` item: U+FDD0, a noncharacter. +pub const ESCAPE: char = '\u{FDD0}'; +/// The most items [`items`] and [`keys`] return: the first this many keys in sorted order. A +/// file can carry any number of hidden names; the rest are not listed. +pub const MAX_ITEMS: usize = 10_000; + +/// The names that hold one item: its `_gc_` chunks by number (the first of each number) and its +/// legacy `_sc_` name. +#[derive(Default)] +struct Parts<'a> { + chunks: BTreeMap, + legacy: Option<&'a str>, +} + +impl<'a> Parts<'a> { + fn add(&mut self, chunk: Option, formula: &'a str) { + match chunk { + Some(i) => { + self.chunks.entry(i).or_insert(formula); + } + None => self.legacy = self.legacy.or(Some(formula)), + } + } + + /// Whether the names make an item: a later chunk on its own does not. + fn is_item(&self) -> bool { + self.chunks.contains_key(&0) || self.legacy.is_some() + } +} + +#[cfg(test)] +thread_local! { + /// Names [`group`] has looked at on this thread, so a test can bound the work of a listing. + static NAME_VISITS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// The workbook's items whose lower-cased key starts with `prefix` (lower-cased), with the names +/// that hold them, in one pass over the names. +fn group<'a>(wb: &'a Workbook, prefix: &str) -> BTreeMap> { + let prefix = prefix.to_ascii_lowercase(); + let mut items: BTreeMap> = BTreeMap::new(); + for n in wb.names.iter() { + #[cfg(test)] + NAME_VISITS.with(|v| v.set(v.get().saturating_add(1))); + let Some((k, chunk)) = item_of(n) else { continue }; + let k = k.to_ascii_lowercase(); + if k.starts_with(&prefix) { + items.entry(k).or_default().add(chunk, &n.formula); + } + } + items +} + +/// The item text stored under `key`, or `None` when there is none (or it is over the size limit). +pub fn get(wb: &Workbook, key: &str) -> Option { + let mut parts = Parts::default(); + for n in wb.names.iter() { + if let Some((k, chunk)) = item_of(n) + && k.eq_ignore_ascii_case(key) + { + parts.add(chunk, &n.formula); + } + } + assemble(&parts) +} + +/// The items whose key starts with `prefix` (`"macro_"`), by lower-cased key, read in one pass +/// over the names: the first [`MAX_ITEMS`] of [`keys`], without those over the size limit. +pub fn items(wb: &Workbook, prefix: &str) -> BTreeMap { + group(wb, prefix) + .into_iter() + .filter(|(_, parts)| parts.is_item()) + .take(MAX_ITEMS) + .filter_map(|(k, parts)| assemble(&parts).map(|text| (k, text))) + .collect() +} + +/// The text of the item held by `parts`, as [`get`] reads it. +fn assemble(parts: &Parts) -> Option { + let chunks = &parts.chunks; + let mut out = String::new(); + if !chunks.contains_key(&0) { + out = string_literals(parts.legacy?); + } else { + // Chunks run 0, 1, 2, …; anything after a gap isn't part of the item. + for (want, (i, f)) in chunks.iter().enumerate() { + if *i != want as u64 { + break; + } + out.push_str(&string_literals(f)); + if out.len() > MAX_STORED_BYTES { + return None; + } + } + out = unescape(&out); + } + (out.chars().count() <= MAX_ITEM_CHARS).then_some(out) +} + +/// Whether `c` is stored escaped: XML can't carry it, or it is the marker. +fn needs_escape(c: char) -> bool { + c == ESCAPE || !matches!(c, '\t' | '\n' | '\r' | '\u{20}'..='\u{D7FF}' | '\u{E000}'..='\u{FFFD}' | '\u{10000}'..='\u{10FFFF}') +} + +/// Turns [`ESCAPE`] and four hex digits back into the character. A marker not followed by four +/// hex digits naming a character (a hand-made name) stays as it is. +fn unescape(s: &str) -> String { + if !s.contains(ESCAPE) { + return s.to_string(); + } + let mut out = String::with_capacity(s.len()); + let mut rest = s; + while let Some(at) = rest.find(ESCAPE) { + out.push_str(rest.get(..at).unwrap_or("")); + let after = rest.get(at + ESCAPE.len_utf8()..).unwrap_or(""); + let code = after.get(..4).filter(|h| h.bytes().all(|b| b.is_ascii_hexdigit())).and_then(|h| u32::from_str_radix(h, 16).ok()); + match code.and_then(char::from_u32) { + Some(c) => { + out.push(c); + rest = after.get(4..).unwrap_or(""); + } + None => { + out.push(ESCAPE); + rest = after; + } + } + } + out.push_str(rest); + out +} + +/// Stores `text` under `key`, replacing what was there (in either format). Empty text removes it. +pub fn set(wb: &mut Workbook, key: &str, text: &str) -> Result<()> { + check_key(key)?; + if text.chars().count() > MAX_ITEM_CHARS { + return Err(EngineError::Other(format!("The data is too large to keep in the workbook ({} MB at most).", MAX_ITEM_CHARS / (1024 * 1024)))); + } + let formulas = chunk_formulas(text); + // A visible name of the item's shape is someone's own (see `item_of`); a hidden name beside + // it with the same name would be the one an xlsx file drops. Refuse before changing anything, + // in one pass over the names (a file can carry many thousands). + let chunks = formulas.len() as u64; + let taken = |n: &&DefinedName| { + n.scope.is_none() && !n.hidden && parse_name(&n.name).is_some_and(|(k, i)| k.eq_ignore_ascii_case(key) && i.is_some_and(|i| i < chunks)) + }; + if let Some(n) = wb.names.iter().find(taken) { + return Err(EngineError::Other(format!( + "The workbook has a defined name '{}', which GridCraft needs for its own data. Rename it first.", + n.name + ))); + } + remove(wb, key); + for (i, formula) in formulas.into_iter().enumerate() { + wb.names.push(DefinedName { name: format!("{PREFIX}{key}_{i}"), scope: None, formula, comment: "GridCraft data".into(), hidden: true }); + } + Ok(()) +} + +/// Removes the item stored under `key`. +pub fn remove(wb: &mut Workbook, key: &str) { + wb.names.retain(|n| !item_of(n).is_some_and(|(k, _)| k.eq_ignore_ascii_case(key))); +} + +/// The item under `key` as JSON (`None` when missing or not JSON). +pub fn get_json(wb: &Workbook, key: &str) -> Option { + serde_json::from_str(&get(wb, key)?).ok() +} + +pub fn set_json(wb: &mut Workbook, key: &str, v: &Json) -> Result<()> { + set(wb, key, &v.to_string()) +} + +/// The keys of the items stored in the workbook that start with `prefix` (`"macro_"`), sorted +/// and lower-cased: the first [`MAX_ITEMS`] of them. To read them all, [`items`] takes one pass +/// where a [`get`] per key would take one each. +pub fn keys(wb: &Workbook, prefix: &str) -> Vec { + group(wb, prefix).into_iter().filter(|(_, parts)| parts.is_item()).map(|(k, _)| k).take(MAX_ITEMS).collect() +} + +/// Whether the workbook holds an item under `name` (compared without case): a hidden workbook-scope +/// `_gc__` name, or a workbook-scope legacy `_sc_` name, which is read whether or not +/// it is hidden. A visible `_gc_` name is the person's own. +pub fn holds_item(wb: &Workbook, name: &str) -> bool { + wb.names.iter().any(|n| n.name.eq_ignore_ascii_case(name) && item_of(n).is_some()) +} + +/// The item a hidden name belongs to, lower-cased: `_gc__` and `_sc_` give ``. +pub fn item_key(name: &str) -> Option { + parse_name(name).map(|(k, _)| k.to_ascii_lowercase()) +} + +/// The item a name holds a part of: a hidden workbook-scope `_gc__` name, or a legacy +/// `_sc_` one. A visible name of the `_gc_` shape is someone's own name, not an item: it is +/// neither read, nor listed, nor removed on write. +fn item_of(n: &DefinedName) -> Option<(&str, Option)> { + if n.scope.is_some() { + return None; + } + parse_name(&n.name).filter(|(_, chunk)| chunk.is_none() || n.hidden) +} + +/// `_gc__` → (`key`, `Some(k)`); `_sc_` → (`key`, `None`). +fn parse_name(name: &str) -> Option<(&str, Option)> { + if let Some(rest) = strip_prefix_ci(name, PREFIX) { + let (key, k) = rest.rsplit_once('_')?; + // Only the chunk number as `set` writes it: `_gc_watch_00` is someone else's name, not + // chunk 0 under another spelling, so it is neither read nor in the way of `_gc_watch_0`. + let canonical = !k.is_empty() && k.len() <= 9 && k.bytes().all(|b| b.is_ascii_digit()) && (k == "0" || !k.starts_with('0')); + if key.is_empty() || !canonical { + return None; + } + return k.parse().ok().map(|i| (key, Some(i))); + } + strip_prefix_ci(name, LEGACY_PREFIX).filter(|k| !k.is_empty()).map(|k| (k, None)) +} + +fn strip_prefix_ci<'a>(s: &'a str, prefix: &str) -> Option<&'a str> { + let head = s.get(..prefix.len())?; + head.eq_ignore_ascii_case(prefix).then(|| s.get(prefix.len()..)).flatten() +} + +fn check_key(key: &str) -> Result<()> { + let ok = !key.is_empty() && key.len() <= MAX_KEY && key.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'.'); + if ok { Ok(()) } else { Err(EngineError::Other(format!("`{key}` can't name workbook data: use letters, digits, `_` and `.`"))) } +} + +/// `text` as name formulas: string literals of at most 255 characters joined with `&`, each formula +/// at most 8,192 characters, with the characters XML can't carry escaped (see [`ESCAPE`]). +/// Concatenating [`string_literals`] of each and turning the escapes back gives `text` back. +pub fn chunk_formulas(text: &str) -> Vec { + let mut formulas = Vec::new(); + let (mut formula, mut units) = (String::new(), 0usize); + let (mut literal, mut lit_units) = (String::from("\""), 0usize); + let mut flush_literal = |literal: &mut String, lit_units: &mut usize, formula: &mut String, units: &mut usize| { + literal.push('"'); + let cost = literal.encode_utf16().count() + usize::from(!formula.is_empty()); + if *units + cost > FORMULA_UNITS && !formula.is_empty() { + formulas.push(std::mem::take(formula)); + *units = 0; + } + if !formula.is_empty() { + formula.push('&'); + *units += 1; + } + *units += literal.encode_utf16().count(); + formula.push_str(literal); + *literal = String::from("\""); + *lit_units = 0; + }; + for ch in text.chars() { + // An escaped character is the marker and four digits, kept in one literal. + let escaped = needs_escape(ch); + let cost = if escaped { ESCAPE.len_utf16() + 4 } else { ch.len_utf16() }; + if lit_units + cost > LITERAL_UNITS { + flush_literal(&mut literal, &mut lit_units, &mut formula, &mut units); + } + lit_units += cost; + if escaped { + literal.push(ESCAPE); + let _ = write!(literal, "{:04X}", u32::from(ch)); + } else { + if ch == '"' { + literal.push('"'); + } + literal.push(ch); + } + } + if lit_units > 0 { + flush_literal(&mut literal, &mut lit_units, &mut formula, &mut units); + } + if !formula.is_empty() { + formulas.push(formula); + } + formulas +} + +/// Concatenates the string literals of a `"a"&"b""c"` formula (anything outside them is ignored). +pub fn string_literals(f: &str) -> String { + let mut out = String::new(); + let mut it = f.chars().peekable(); + let mut in_str = false; + while let Some(c) = it.next() { + if !in_str { + if c == '"' { + in_str = true; + } + continue; + } + if c == '"' { + if it.peek() == Some(&'"') { + it.next(); + out.push('"'); + } else { + in_str = false; + } + } else { + out.push(c); + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + fn name(n: &str, formula: &str) -> DefinedName { + DefinedName { name: n.into(), scope: None, formula: formula.into(), comment: String::new(), hidden: true } + } + + fn excel_limits_hold(wb: &Workbook) { + for n in &wb.names { + assert!(n.formula.encode_utf16().count() <= FORMULA_UNITS, "{} is {} long", n.name, n.formula.len()); + // Every literal (quotes undoubled) is at most 255 characters. + for lit in n.formula.split("\"&\"") { + let inner = lit.trim_start_matches('"').trim_end_matches('"').replace("\"\"", "\""); + assert!(inner.encode_utf16().count() <= LITERAL_UNITS, "{}", inner.len()); + } + } + } + + #[test] + fn long_values_split_over_names_and_come_back() { + let mut wb = Workbook::new(); + // Quotes double when written; characters outside the BMP count twice. + let text: String = (0..30_000).map(|i| ['a', '"', 'é', '\u{1D11E}', '&', '\n'][i % 6]).collect(); + set(&mut wb, "big", &text).unwrap(); + let names: Vec<&str> = wb.names.iter().map(|n| n.name.as_str()).collect(); + assert!(names.len() > 5, "{names:?}"); + assert_eq!(names[0], "_gc_big_0"); + assert!(wb.names.iter().all(|n| n.hidden && n.scope.is_none())); + excel_limits_hold(&wb); + assert_eq!(get(&wb, "big").as_deref(), Some(text.as_str())); + assert_eq!(get(&wb, "BIG").as_deref(), Some(text.as_str()), "names are case-insensitive"); + // Shorter again: the extra chunks go. + set(&mut wb, "big", "short").unwrap(); + assert_eq!(wb.names.len(), 1); + assert_eq!(wb.names[0].formula, "\"short\""); + set(&mut wb, "big", "").unwrap(); + assert!(wb.names.is_empty()); + assert_eq!(get(&wb, "big"), None); + } + + #[test] + fn chunks_and_literals_respect_the_boundaries() { + for len in [0, 1, 254, 255, 256, 8_000, 8_192, 8_193, 20_000] { + let text = "q".repeat(len); + let fs = chunk_formulas(&text); + assert_eq!(fs.iter().map(|f| string_literals(f)).collect::(), text, "{len}"); + assert!(fs.iter().all(|f| f.len() <= FORMULA_UNITS), "{len}"); + } + // All quotes: each literal doubles in the formula. + let text = "\"".repeat(9_000); + let fs = chunk_formulas(&text); + assert_eq!(fs.iter().map(|f| string_literals(f)).collect::(), text); + assert!(fs.iter().all(|f| f.len() <= FORMULA_UNITS)); + } + + #[test] + fn the_old_single_name_still_reads_and_is_replaced_on_write() { + let mut wb = Workbook::new(); + wb.names.push(name("_sc_scripts", "\"{\"\"A\"\":[1]}\"")); + wb.names.push(name("_sc_scenarios", "\"[1,\"&\"2]\"")); + assert_eq!(get_json(&wb, "scripts"), Some(serde_json::json!({"A": [1]}))); + assert_eq!(get_json(&wb, "scenarios"), Some(serde_json::json!([1, 2]))); + set_json(&mut wb, "scripts", &serde_json::json!({"B": []})).unwrap(); + let names: Vec<&str> = wb.names.iter().map(|n| n.name.as_str()).collect(); + assert_eq!(names, ["_sc_scenarios", "_gc_scripts_0"]); + assert_eq!(get_json(&wb, "scripts"), Some(serde_json::json!({"B": []}))); + } + + #[test] + fn hostile_names_read_safely() { + let mut wb = Workbook::new(); + // A gap: chunk 2 without chunk 1. + wb.names.push(name("_gc_gap_0", "\"a\"")); + wb.names.push(name("_gc_gap_2", "\"c\"")); + assert_eq!(get(&wb, "gap").as_deref(), Some("a")); + // Only a later chunk: no item, not listed, and the legacy name is still read. + wb.names.push(name("_gc_orphan_2", "\"c\"")); + assert_eq!(get(&wb, "orphan"), None); + assert_eq!(keys(&wb, "orphan"), Vec::::new()); + wb.names.push(name("_sc_orphan", "\"old\"")); + assert_eq!(get(&wb, "orphan").as_deref(), Some("old")); + assert_eq!(keys(&wb, "orphan"), ["orphan"]); + // Chunks out of order, a duplicate, junk around the literals, an unterminated literal. + wb.names.push(name("_gc_mix_1", "\"b\"&A1&")); + wb.names.push(name("_gc_mix_0", "SUM(\"a\")")); + wb.names.push(name("_gc_mix_0", "\"ignored\"")); + wb.names.push(name("_gc_mix_2", "\"c")); + assert_eq!(get(&wb, "mix").as_deref(), Some("abc")); + // Names that only look like chunks. + for odd in ["_gc_", "_gc__0", "_gc_x_", "_gc_x_-1", "_gc_x_99999999999999999999", "_sc_", "_gcx_0"] { + wb.names.push(name(odd, "\"zzz\"")); + } + assert_eq!(get(&wb, "x"), None); + assert_eq!(get(&wb, ""), None); + // Not JSON, or JSON nested deeper than the parser allows. + wb.names.push(name("_gc_notjson_0", "\"{\"")); + assert_eq!(get_json(&wb, "notjson"), None); + let deep = format!("\"{}{}\"", "[".repeat(10_000), "]".repeat(10_000)); + wb.names.push(name("_gc_deep_0", &deep)); + assert_eq!(get_json(&wb, "deep"), None); + // Sheet-scoped names are someone else's. + wb.names.push(DefinedName { scope: Some(0), ..name("_gc_scoped_0", "\"s\"") }); + assert_eq!(get(&wb, "scoped"), None); + // Bad keys are refused. + for bad in ["", "has space", "quote\"", "a/b", &"k".repeat(201)] { + assert!(set(&mut wb, bad, "v").is_err(), "{bad}"); + } + assert!(set(&mut wb, "big", &"x".repeat(MAX_ITEM_CHARS + 1)).is_err()); + } + + #[test] + fn oversized_items_are_not_read() { + let mut wb = Workbook::new(); + let chunk = format!("\"{}\"", "x".repeat(250)); + let big = vec![chunk.as_str(); 32].join("&"); + for i in 0..(MAX_ITEM_CHARS / 8_000 + 2) { + wb.names.push(name(&format!("_gc_huge_{i}"), &big)); + } + assert_eq!(get(&wb, "huge"), None); + } + + #[test] + fn a_visible_name_of_the_item_shape_is_not_an_item() { + // Someone's own visible `_gc_watch_0` is not storage: it is not read or listed, and + // writing or removing the item leaves it alone. + let mut wb = Workbook::new(); + let own = DefinedName { hidden: false, ..name("_gc_watch_0", "\"mine\"") }; + wb.names.push(own.clone()); + assert_eq!(get(&wb, "watch"), None); + assert!(keys(&wb, "").is_empty()); + // Writing the item would put a hidden name of the same name beside it, and an xlsx file + // keeps only one of them: refused, with the names as they were. + let err = set(&mut wb, "watch", "theirs").unwrap_err().to_string(); + assert!(err.contains("_gc_watch_0"), "{err}"); + assert_eq!(wb.names, vec![own.clone()]); + assert_eq!(get(&wb, "watch"), None); + set(&mut wb, "other", "theirs").unwrap(); + assert_eq!(get(&wb, "other").as_deref(), Some("theirs")); + remove(&mut wb, "watch"); + remove(&mut wb, "other"); + assert_eq!(wb.names, vec![own]); + } + + #[test] + fn a_chunk_number_with_leading_zeros_is_another_name() { + // `_gc_watch_00` is not chunk 0 under another spelling: a hidden one is not read, and a + // visible one is not in the way of writing `_gc_watch_0`. + let mut wb = Workbook::new(); + wb.names.push(name("_gc_watch_00", "\"theirs\"")); + assert_eq!(get(&wb, "watch"), None); + assert!(keys(&wb, "").is_empty()); + let own = DefinedName { hidden: false, ..name("_gc_watch_00", "\"mine\"") }; + wb.names.push(own.clone()); + set(&mut wb, "watch", "ours").unwrap(); + assert_eq!(get(&wb, "watch").as_deref(), Some("ours")); + assert_eq!(wb.names.len(), 3, "both other names stay"); + remove(&mut wb, "watch"); + assert_eq!(wb.names, vec![name("_gc_watch_00", "\"theirs\""), own]); + } + + #[test] + fn keys_list_items_by_prefix() { + let mut wb = Workbook::new(); + set(&mut wb, "macro_2", "b").unwrap(); + set(&mut wb, "macro_1", &"a".repeat(9_000)).unwrap(); + set(&mut wb, "scripts", "{}").unwrap(); + assert_eq!(keys(&wb, "macro_"), ["macro_1", "macro_2"]); + assert_eq!(item_key("_gc_macro_1_3").as_deref(), Some("macro_1")); + assert_eq!(item_key("_SC_Scripts").as_deref(), Some("scripts")); + assert_eq!(item_key("Total"), None); + } + + #[test] + fn listing_many_items_takes_one_pass_and_is_capped() { + // 20,000 hidden items, as a file can carry: a `get` per key looked at every name for each + // key (400 million visits); `items` looks at each name once, and lists at most MAX_ITEMS. + let mut wb = Workbook::new(); + let total = 20_000; + for i in 0..total { + wb.names.push(name(&format!("_gc_macro_{i:05}_0"), &format!("\"m{i}\""))); + } + set(&mut wb, "macro_big", &"b".repeat(9_000)).unwrap(); + wb.names.push(name("_sc_macro_legacy", "\"old\"")); + wb.names.push(name("_gc_macro_orphan_1", "\"no chunk 0\"")); + wb.names.push(name("_gc_other_0", "\"not a macro\"")); + NAME_VISITS.with(|v| v.set(0)); + let all = items(&wb, "MACRO_"); + assert_eq!(NAME_VISITS.with(|v| v.get()), wb.names.len(), "one pass over the names"); + assert_eq!(all.len(), MAX_ITEMS); + let first: Vec<&String> = all.keys().take(2).collect(); + assert_eq!(first, ["macro_00000", "macro_00001"]); + assert_eq!(all.get("macro_09999").map(String::as_str), Some("m9999")); + assert!(!all.contains_key("macro_10000"), "past the cap"); + NAME_VISITS.with(|v| v.set(0)); + let listed = keys(&wb, "macro_"); + assert_eq!(NAME_VISITS.with(|v| v.get()), wb.names.len()); + assert_eq!(listed, all.keys().cloned().collect::>(), "keys and items agree"); + + // Under the cap every item is there, read as `get` reads it. + wb.names.retain(|n| !n.name.starts_with("_gc_macro_0") && !n.name.starts_with("_gc_macro_1")); + let few = items(&wb, "macro_"); + let want: BTreeMap = + [("macro_big", "b".repeat(9_000)), ("macro_legacy", "old".into())].into_iter().map(|(k, t)| (k.to_string(), t)).collect(); + assert_eq!(few, want); + for (k, text) in &few { + assert_eq!(get(&wb, k).as_ref(), Some(text)); + } + assert_eq!(keys(&wb, "macro_"), ["macro_big", "macro_legacy"]); + assert_eq!(items(&wb, "").len(), 3, "the other item too"); + } + + #[test] + fn items_round_trip_through_xlsx() { + let mut wb = Workbook::new(); + let text: String = (0..20_000).map(|i| ['x', '"', 'ü', '\u{1D11E}', ' '][i % 5]).collect(); + set(&mut wb, "blob", &text).unwrap(); + wb.names.push(name("_sc_watch", "\"[\"\"A1\"\"]\"")); + let bytes = gridcraft_xlsx::write_xlsx(&wb).unwrap(); + let (back, warnings) = crate::io::open_bytes("data.xlsx", &bytes).unwrap(); + assert!(warnings.is_empty(), "{warnings:?}"); + assert_eq!(get(&back, "blob").as_deref(), Some(text.as_str())); + assert_eq!(get_json(&back, "watch"), Some(serde_json::json!(["A1"]))); + + // Characters XML can't carry, and the escape marker itself, come back too, in text and + // in JSON (which leaves U+FFFE and U+FFFF unescaped). + let mut wb = Workbook::new(); + let odd: String = (0u32..0x20).filter_map(char::from_u32).chain(['\u{FFFE}', '\u{FFFF}', ESCAPE, 'a', '\u{7F}', '\u{FDD1}']).collect(); + let text: String = odd.chars().cycle().take(odd.chars().count() * 300).collect(); + set(&mut wb, "odd", &text).unwrap(); + excel_limits_hold(&wb); + let json = serde_json::json!({"s": "x\u{FFFF}y\u{1}z\u{FDD0}0041"}); + set_json(&mut wb, "json", &json).unwrap(); + // All escapes: each literal holds whole escapes, within the limits. + let controls = "\u{1}".repeat(9_000); + set(&mut wb, "controls", &controls).unwrap(); + excel_limits_hold(&wb); + let bytes = gridcraft_xlsx::write_xlsx(&wb).unwrap(); + let (back, warnings) = crate::io::open_bytes("data.xlsx", &bytes).unwrap(); + assert!(warnings.is_empty(), "{warnings:?}"); + assert_eq!(get(&back, "odd").as_deref(), Some(text.as_str())); + assert_eq!(get_json(&back, "json"), Some(json)); + assert_eq!(get(&back, "controls").as_deref(), Some(controls.as_str())); + } + + #[test] + fn escapes_count_against_the_limits_and_odd_markers_read_as_they_are() { + // Excel's limits hold however many characters are escaped, and the longest item of + // characters that all need escaping still reads. + for len in [1, 50, 51, 52, 1_638, 1_639, 5_000] { + let text = "\u{2}".repeat(len); + let fs = chunk_formulas(&text); + assert_eq!(unescape(&fs.iter().map(|f| string_literals(f)).collect::()), text, "{len}"); + assert!(fs.iter().all(|f| f.encode_utf16().count() <= FORMULA_UNITS), "{len}"); + } + let mut wb = Workbook::new(); + let longest = "\u{0}".repeat(MAX_ITEM_CHARS); + set(&mut wb, "longest", &longest).unwrap(); + excel_limits_hold(&wb); + assert_eq!(get(&wb, "longest").map(|t| t.len()), Some(MAX_ITEM_CHARS)); + // A marker in a hand-made name that is not an escape stays as it is. + let mut wb = Workbook::new(); + wb.names.push(name("_gc_hand_0", "\"a\u{FDD0}zz\u{FDD0}D800\u{FDD0}004\u{FDD0}0041\u{FDD0}\"")); + assert_eq!(get(&wb, "hand").as_deref(), Some("a\u{FDD0}zz\u{FDD0}D800\u{FDD0}004A\u{FDD0}")); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 066a9e559..b4ab60f3a 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -19,6 +19,7 @@ pub mod outline; pub mod paper; pub mod pivot; pub mod pivot_calc; +pub mod record; pub mod sample; pub mod selection; pub mod tables; @@ -66,6 +67,51 @@ pub struct HistoryEntry { pub retained_bytes: usize, } +/// An undo group opened with [`Session::group_begin`]. Hand it back to [`Session::group_end`] +/// (keep the changes as one undo step) or [`Session::group_rollback`] (undo them all). +#[derive(Debug, PartialEq, Eq)] +#[must_use = "an undo group stays open until it is ended or rolled back"] +pub struct GroupToken { + id: u64, + doc: u64, +} + +impl GroupToken { + /// The uid of the document the group belongs to. + pub fn doc(&self) -> u64 { + self.doc + } +} + +/// An open undo group: what runs on `doc` until it ends becomes one undo step. +#[derive(Clone, Debug)] +struct OpenGroup { + id: u64, + doc: u64, + label: String, + /// Undo depth when the group began; the group's own steps are cut back to it. + start: usize, + wb: Arc, + selection: Selection, + /// The redo stack when the group began (a rollback puts it back). + redo: Vec, +} + +/// Where the last [`Session::group_end`] or [`Session::group_rollback`] left its document: the +/// workbook and selection it ended at, the undo length then, and whether it made a step. A +/// command that ended the group tells its own later changes from the group's step by it, and +/// keeps the group's name only when the group's step is exactly its own (the entry holds the +/// workbook the step started from). Dropped when the outermost command finishes, so it keeps no +/// workbook alive between commands. +#[derive(Debug)] +struct GroupStep { + doc: u64, + after: Arc, + selection: Selection, + len: usize, + stepped: bool, +} + /// The clipboard: a block of cells (with formulas and styles) or marching-ants reference. #[derive(Clone, Debug)] pub struct Clipboard { @@ -84,6 +130,34 @@ pub struct Clipboard { /// A multi-area copy: the areas as selected (for the marquee). `wb` then holds them slid /// together into one block at `range`. Empty for a single-range copy. pub areas: Vec, + /// Trust of the source document: pasting content from a file someone else made carries it. + pub trust: Trust, +} + +/// Whether a document's active content (macros) may run without asking the person first. There +/// is no default: whoever makes a document says where it came from ([`DocState::new`]), so a new +/// way of building one from bytes can't come out trusted by omission. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub enum Trust { + /// Made in this session: a new workbook or a sample. + Local, + /// Opened from a file, or holding content pasted from one: active content asks before it + /// runs. + FromFile, +} + +impl Trust { + /// Whether active content may run without asking. + pub fn allows_code(self) -> bool { + self == Trust::Local + } + + /// The trust of a document after it receives content from a document with trust `source`: + /// content from an untrusted file leaves the receiver untrusted. + pub fn receive(self, source: Trust) -> Trust { + if source == Trust::FromFile { Trust::FromFile } else { self } + } } /// In-cell edit mode as shown in the status bar. @@ -120,6 +194,11 @@ pub struct DocState { /// user trusts it in this session (`automate.trustScripts`); one with no scripts starts /// trusted, since any script it gains later was written here. pub scripts_trusted: bool, + /// Where the workbook's content came from: whether its active content may run without + /// asking. Unlike `scripts_trusted`, this is provenance, not consent: it is set when the + /// workbook is opened from a file, carried by content pasted from such a workbook, and never + /// changed by a trust decision. + pub trust: Trust, } static NEXT_UID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); @@ -160,17 +239,31 @@ fn push_history_with_budget( selection: Selection, byte_budget: usize, ) { + push_entry(history, label, wb, next, selection); + trim_to_budget(history, byte_budget); +} + +/// Adds a step without applying the limits: inside a command they wait for the outermost +/// command to collapse its steps (see [`Session::trim_history`]). +fn push_entry(history: &mut Vec, label: String, wb: Arc, next: &Workbook, selection: Selection) { let retained_bytes = history_retained_bytes(&wb, next); history.push(HistoryEntry { label, wb, selection, retained_bytes }); +} + +/// Drops the oldest steps past [`UNDO_LIMIT`] or `byte_budget`, always keeping the latest one +/// (even if that one edit exceeds the budget). +fn trim_to_budget(history: &mut Vec, byte_budget: usize) { let mut total = history.iter().fold(0usize, |sum, entry| sum.saturating_add(entry.retained_bytes)); - // Always preserve the latest action, even if that one edit exceeds the budget. while history.len() > 1 && (history.len() > UNDO_LIMIT || total > byte_budget) { total = total.saturating_sub(history.remove(0).retained_bytes); } } impl DocState { - pub fn new(mut wb: Workbook, path: Option, title: String) -> DocState { + /// A document for `wb`, which came from where `trust` says: [`Trust::FromFile`] for anything + /// read from bytes (a file, a download, a recovery copy), [`Trust::Local`] only for a workbook + /// made in this session. + pub fn new(mut wb: Workbook, path: Option, title: String, trust: Trust) -> DocState { let mut calc = Calc::new(); calc.recalc_all(&mut wb); let scripts_trusted = !cmd::extra::has_scripts(&wb); @@ -191,6 +284,7 @@ impl DocState { unlocked_ranges: vec![], content_kind: None, scripts_trusted, + trust, } } pub fn is_dirty(&self) -> bool { @@ -307,8 +401,13 @@ pub struct Session { pub last_find: Option, /// View › Show › Ruler is off (rulers in Page Layout view; an application option). pub hide_ruler: bool, - /// Journal position where action recording started. + /// Journal position where Automate › Record Actions started. pub recording: Option, + /// The recording buffer while a recorder runs ([`Session::start_recording`]): top-level + /// commands with the place they ran in, kept apart from the journal. + pub recorder: Option, + /// How many `execute` calls are running (0 between top-level commands). + depth: u32, /// Windows hidden with View › Hide. pub hidden_windows: Vec, /// Draw tab tool: "" (select), "pen" or "eraser"; pen colour and width. @@ -320,6 +419,17 @@ 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, + /// Open undo groups, innermost last. + groups: Vec, + next_group: u64, + /// Counts group begins and ends, so `execute` can tell that a command opened or closed one. + group_epoch: u64, + /// Where the last group ended, so `execute` can tell a command's own changes from its group's. + group_step: Option, + /// For each running command, innermost last: its document and that document's undo length + /// when it began. Steps before that length are not touched while the command runs (see + /// [`Session::bound_tail`]). + marks: Vec<(u64, usize)>, /// Measures cell text for AutoFit Column Width. Without it (CLI, MCP) AutoFit uses /// [`cmd::format::approx_text_width`]. pub text_width: Option, @@ -359,7 +469,8 @@ impl Session { } pub fn close_document(&mut self, i: usize) { if i < self.docs.len() { - self.docs.remove(i); + let d = self.docs.remove(i); + self.groups.retain(|g| g.doc != d.uid); if self.active >= self.docs.len() { self.active = self.docs.len().saturating_sub(1); } @@ -373,7 +484,7 @@ impl Session { let name = wb.next_sheet_name(); wb.sheets.push(Arc::new(gridcraft_model::Sheet::new(name))); } - self.add_document(DocState::new(wb, None, format!("Book{n}"))) + self.add_document(DocState::new(wb, None, format!("Book{n}"), Trust::Local)) } /// Journals a command, then trims the oldest entries past either cap. A recording in progress @@ -412,7 +523,8 @@ impl Session { command_specs().iter().map(|c| c.info(self)).collect() } - /// Runs a command by id. Undoable commands record a history step when the workbook changed. + /// Runs a command by id. Undoable commands record a history step when the workbook changed, + /// unless an undo group is open on the document (the group then makes the step). /// Panics inside commands are caught and reported; the workbook is restored. pub fn execute(&mut self, id: &str, params: Json) -> Result { let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.to_string()))?; @@ -421,36 +533,352 @@ impl Session { } let before = self.active().map(|d| (d.uid, d.wb.clone(), d.selection.clone())); let params = if params.is_null() { Json::Object(Default::default()) } else { params }; + let (first_group, group_epoch) = (self.next_group, self.group_epoch); + // Whether a group was open on the document before the command, and how long its history was. + let start = before.as_ref().and_then(|(uid, ..)| self.docs.iter().find(|d| d.uid == *uid).map(|d| (self.in_group(*uid), d.undo.len()))); + // A command that panics leaves the history as it found it: the steps commands nested in it + // made or undid, and the redo history they cleared, come back. A length is not enough, + // because a nested `edit.undo` removes entries. A query runs nothing that changes the + // history (`every_query_leaves_the_history_alone` runs each one), so it needs no + // copy; front ends run queries all the time. + let query = !spec.journal && !spec.undoable; + let history_before = before.as_ref().filter(|_| !query).and_then(|(uid, ..)| { + #[cfg(test)] + tests::HISTORY_COPIES.with(|n| n.set(n.get().saturating_add(1))); + self.docs.iter().find(|d| d.uid == *uid).map(|d| (d.undo.clone(), d.redo.clone())) + }); + // The groups open on the document before the command (usually none), for the same reason. + let groups_before: Vec<(u64, String, Arc, Selection)> = match &before { + Some((uid, ..)) => { + self.groups.iter().filter(|g| g.doc == *uid).map(|g| (g.id, g.label.clone(), g.wb.clone(), g.selection.clone())).collect() + } + None => Vec::new(), + }; + // The recorder keeps top-level calls with the place they ran in. + let place = (self.depth == 0 && self.recorder.is_some()).then(|| match self.active() { + Some(d) => (d.uid, d.wb.active_sheet, d.wb.active().map(|sh| sh.name.clone()).unwrap_or_default(), d.selection.clone()), + None => (0, 0, String::new(), Selection::default()), + }); + let doc_uid = before.as_ref().map(|(uid, ..)| *uid); + let requests_before = self.ui_requests.len(); + let marked = doc_uid.zip(start).map(|(uid, (_, len))| (uid, len)); + self.marks.extend(marked); + self.depth = self.depth.saturating_add(1); let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| (spec.run)(self, ¶ms))); + self.depth = self.depth.saturating_sub(1); + if marked.is_some() { + self.marks.pop(); + } let result = match result { Ok(r) => r, Err(p) => { let msg = p.downcast_ref::<&str>().map(|s| s.to_string()).or_else(|| p.downcast_ref::().cloned()).unwrap_or_else(|| "panic".into()); - // Restore the workbook as it was. + // Restore the workbook and its history as they were, and drop the undo groups the + // command opened. + self.groups.retain(|g| g.id < first_group); + let group_open = before.as_ref().is_some_and(|(uid, ..)| self.groups.iter().any(|g| g.doc == *uid)); if let Some((uid, wb, sel)) = &before && let Some(d) = self.docs.iter_mut().find(|d| d.uid == *uid) { d.wb = wb.clone(); d.selection = sel.clone(); + if let Some((undo, redo)) = history_before { + d.undo = undo; + d.redo = redo; + } + // A group open before the command and ended or rolled back inside it stays + // ended (its token went with the command), and keeps a step from its start to + // the restored workbook, so Undo still returns to the state before the group. + if !group_open + && let Some((_, label, wb, selection)) = groups_before.into_iter().min_by_key(|g| g.0) + && !Arc::ptr_eq(&wb, &d.wb) + { + push_entry(&mut d.undo, label, wb, &d.wb, selection); + d.redo.clear(); + } d.calc.rebuild(&d.wb); } + if self.depth == 0 { + self.trim_history(); + self.group_step = None; + } return Err(EngineError::Internal(id.to_string(), msg)); } }; + // A command that fails leaves none of its own undo groups open (a `?` between + // `group_begin` and `group_end`): each is rolled back as `group_rollback` does. Only a + // command that succeeds keeps a group it opened (a debugger's start). + if result.is_err() { + self.rollback_groups_from(first_group); + } if spec.journal && result.is_ok() { + // A command that started recording is not part of the recording; one that stopped it + // finds no recorder. A menu-style call that only opened a dialog (`file.open` with no + // file, `home.formatCells` with no style) is not recorded either: the dialog's own + // call is, so playing the recording back runs the command once, with its parameters. + let opened_dialog = self.ui_requests.get(requests_before..).is_some_and(|new| new.iter().any(|q| matches!(q, UiRequest::Dialog(..)))); + if let Some((doc, sheet, sheet_name, selection)) = place.filter(|_| !opened_dialog) + && let Some(rec) = self.recorder.as_mut() + { + rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params: cmd::journal_params(¶ms) }); + } self.push_journal(id, cmd::journal_params(¶ms)); } + // Inside an undo group the group owns the step. A command that ended a group open before + // it leaves the group its step and makes one of its own for what it changed after the end. + // A command that opened and closed its own groups makes one step of its own, which + // replaces the steps those groups made. + let epoch_changed = self.group_epoch != group_epoch; + let in_group_now = before.as_ref().is_some_and(|(uid, ..)| self.in_group(*uid)); + let was_open = start.is_some_and(|(was_open, _)| was_open); if spec.undoable && let Some((uid, wb, sel)) = before && let Some(d) = self.docs.iter_mut().find(|d| d.uid == uid) && !Arc::ptr_eq(&d.wb, &wb) { - push_history(&mut d.undo, spec.label.trim_end_matches('…').to_string(), wb, &d.wb, sel); + let label = spec.label.trim_end_matches('…').to_string(); + if in_group_now { + // A group this command opened and left open (a debugger's start) takes the + // command's start as its own, so what the command changed before opening it is + // part of the group's step and not a step of its own. + if !was_open + && let Some((_, len)) = start + && let Some(g) = self.groups.iter_mut().filter(|g| g.doc == uid && g.id >= first_group).min_by_key(|g| g.id) + { + g.start = len; + g.wb = wb; + g.selection = sel; + } + } else if epoch_changed && was_open { + // The group it ended made its step; changes after the end, whether direct or by + // nested commands, are the command's own step from where the group ended. + if let Some(step) = self.group_step.as_ref().filter(|g| g.doc == uid) + && (d.undo.len() > step.len || !Arc::ptr_eq(&step.after, &d.wb)) + { + d.undo.truncate(step.len); + push_entry(&mut d.undo, label, step.after.clone(), &d.wb, step.selection.clone()); + d.redo.clear(); + } + } else { + let mut label = label; + if epoch_changed && let Some((_, len)) = start { + // A command whose only step is its group's keeps that name ("Run Macro: Totals"). + // That is so only when the one entry the command added is a group's step from + // the command's own start to its own result: an entry a nested command made, + // a group step of an earlier command (undone since), or a group's step that + // the command edited past all name the step after the command instead. + if d.undo.len() == len + 1 + && let Some(last) = d.undo.last() + && Arc::ptr_eq(&last.wb, &wb) + && self.group_step.as_ref().is_some_and(|g| g.doc == uid && g.stepped && Arc::ptr_eq(&g.after, &d.wb)) + { + label = last.label.clone(); + } + d.undo.truncate(len); + } + push_entry(&mut d.undo, label, wb, &d.wb, sel); + d.redo.clear(); + } + // Inside a group the redo history waits for the group: `group_end` clears it when the + // group becomes a step, and `group_rollback` puts it back as it was. + d.revision += 1; + } + // The history limit applies once the outermost command has collapsed its steps, so the + // lengths it compared stay true. Meanwhile the steps made inside it are bounded on their own. + if self.depth == 0 { + self.trim_history(); + self.group_step = None; + } else if let Some(uid) = doc_uid { + self.bound_tail(uid); + } + result + } + + /// Runs `f` as one undo step labelled `label` on the active document: whatever commands `f` + /// runs, one Undo brings back the workbook as it was before. If `f` fails, every change it made + /// is rolled back and its error is returned. Groups nest; the outermost one makes the step. + pub fn grouped(&mut self, label: &str, f: impl FnOnce(&mut Session) -> Result) -> Result { + let token = self.group_begin(label)?; + let doc = token.doc(); + // A panic in `f` rolls the group back before it goes on, so a caller that catches it + // (outside `execute`, which drops a command's groups itself) finds no group left open. + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self))) { + Ok(Ok(r)) => match self.group_end(token) { + Ok(_) => Ok(r), + // `f` closed the group's document (`file.close`, or `file.open` taking the place of + // an untouched new workbook), and the group went with it: nothing is left to undo. + Err(_) if !self.docs.iter().any(|d| d.uid == doc) => Ok(r), + Err(e) => Err(e), + }, + Ok(Err(e)) => { + // The error from `f` is the one to report. A rollback fails only when the group + // has already ended, and then there is nothing left to undo. + let _ = self.group_rollback(token); + Err(e) + } + Err(panic) => { + let _ = self.group_rollback(token); + std::panic::resume_unwind(panic) + } + } + } + + /// Opens an undo group on the active document that can span several calls (for example a + /// debugger that runs one step per call). Until [`Session::group_end`] or + /// [`Session::group_rollback`], undoable commands on that document add no undo steps of their + /// own, and Undo and Redo are unavailable there. + pub fn group_begin(&mut self, label: &str) -> Result { + let d = self.doc()?; + let g = OpenGroup { + id: self.next_group, + doc: d.uid, + label: label.trim_end_matches('…').to_string(), + start: d.undo.len(), + wb: d.wb.clone(), + selection: d.selection.clone(), + redo: d.redo.clone(), + }; + let token = GroupToken { id: g.id, doc: g.doc }; + self.next_group = self.next_group.saturating_add(1); + self.group_epoch = self.group_epoch.wrapping_add(1); + self.groups.push(g); + Ok(token) + } + + /// Ends an undo group. If the workbook changed, the group becomes one undo step, unless it is + /// nested in another open group on the same document, which then owns the step. Groups opened + /// inside it and still open end with it. Returns whether the workbook changed. + pub fn group_end(&mut self, token: GroupToken) -> Result { + let g = self.take_group(&token)?; + let outer = self.in_group(g.doc); + let Some(d) = self.docs.iter_mut().find(|d| d.uid == g.doc) else { return Ok(false) }; + // Steps pushed directly while the group was open fold into the group's step. + d.undo.truncate(g.start); + let changed = !Arc::ptr_eq(&d.wb, &g.wb); + if changed && !outer { + push_entry(&mut d.undo, g.label, g.wb, &d.wb, g.selection); d.redo.clear(); d.revision += 1; } - result + if !outer { + self.group_step = + Some(GroupStep { doc: g.doc, after: d.wb.clone(), selection: d.selection.clone(), len: d.undo.len(), stepped: changed }); + } + // Inside a command, the outermost command applies the history limit when it finishes. + if self.depth == 0 { + self.trim_history(); + } + Ok(changed) + } + + /// Stops the running action on the document with uid `doc`: every undo group open on it is + /// rolled back as [`Session::group_rollback`] does, putting back the workbook, selection, undo + /// and redo history as they were when the outermost one began. Their tokens are then spent + /// (ending or rolling one back reports that it has already ended). Returns whether a group + /// was open. `automate.stopAction` runs it on the active document. + pub fn abort_groups(&mut self, doc: u64) -> bool { + let Some(id) = self.groups.iter().filter(|g| g.doc == doc).map(|g| g.id).min() else { return false }; + // Rolling back the outermost group ends the ones opened inside it. + self.group_rollback(GroupToken { id, doc }).is_ok() + } + + /// Rolls back every open undo group numbered `first` or later (the groups a failed command + /// opened), outermost first on each document. + fn rollback_groups_from(&mut self, first: u64) { + let opened: Vec<(u64, u64)> = self.groups.iter().filter(|g| g.id >= first).map(|g| (g.id, g.doc)).collect(); + for (id, doc) in opened { + // A group inside one rolled back before it is already gone. + if self.groups.iter().any(|g| g.id == id) { + let _ = self.group_rollback(GroupToken { id, doc }); + } + } + } + + /// Bounds the undo steps pushed while a command runs (a script of many steps, a subtotal per + /// group): at most [`UNDO_LIMIT`] of them within [`UNDO_BYTES_LIMIT`], dropping the oldest. + /// Only steps after the latest mark go: a running command's start, an open group's start and + /// the end of the last group are positions the command or group collapses its steps by when + /// it finishes, so what is before them stays as it is until [`Session::trim_history`]. + fn bound_tail(&mut self, uid: u64) { + let Some(d) = self.docs.iter_mut().find(|d| d.uid == uid) else { return }; + let marks = self.marks.iter().filter(|(doc, _)| *doc == uid).map(|(_, len)| *len); + let groups = self.groups.iter().filter(|g| g.doc == uid).map(|g| g.start); + let step = self.group_step.as_ref().filter(|g| g.doc == uid).map(|g| g.len); + let mark = marks.chain(groups).chain(step).max().unwrap_or(0).min(d.undo.len()); + let mut bytes = d.undo.iter().skip(mark).fold(0usize, |sum, e| sum.saturating_add(e.retained_bytes)); + while d.undo.len() > mark + 1 && (d.undo.len() - mark > UNDO_LIMIT || bytes > UNDO_BYTES_LIMIT) { + bytes = bytes.saturating_sub(d.undo.remove(mark).retained_bytes); + } + } + + /// Drops the oldest undo steps past [`UNDO_LIMIT`] or [`UNDO_BYTES_LIMIT`]. A document with + /// an open group waits for the group to end, because the group remembers where its steps start. + fn trim_history(&mut self) { + let groups = &self.groups; + for d in self.docs.iter_mut().filter(|d| !groups.iter().any(|g| g.doc == d.uid)) { + trim_to_budget(&mut d.undo, UNDO_BYTES_LIMIT); + } + } + + /// Ends an undo group by putting back the workbook, selection, undo and redo history as they + /// were when it began. + pub fn group_rollback(&mut self, token: GroupToken) -> Result<()> { + let g = self.take_group(&token)?; + let Some(d) = self.docs.iter_mut().find(|d| d.uid == g.doc) else { return Ok(()) }; + if !Arc::ptr_eq(&d.wb, &g.wb) { + d.wb = g.wb; + d.calc.rebuild(&d.wb); + } + d.selection = g.selection; + d.undo.truncate(g.start); + d.redo = g.redo; + d.revision += 1; + // The record of the last group step stays when the document is back exactly where that + // step left it (this group began there); otherwise the rollback is where the group ended. + let kept = self.group_step.as_ref().is_some_and(|st| st.doc == g.doc && st.len == g.start && Arc::ptr_eq(&st.after, &d.wb)); + if !kept && !self.groups.iter().any(|o| o.doc == g.doc) { + self.group_step = Some(GroupStep { doc: g.doc, after: d.wb.clone(), selection: d.selection.clone(), len: g.start, stepped: false }); + } + Ok(()) + } + + /// Starts recording top-level commands into an empty buffer, replacing any recording in + /// progress. Queries, failed commands and commands run by other commands are not recorded. + pub fn start_recording(&mut self) { + self.recorder = Some(record::Recorder::default()); + } + + /// Stops recording and returns what was recorded (`None` when nothing was recording). + pub fn stop_recording(&mut self) -> Option { + self.recorder.take() + } + + /// Whether the recording buffer is collecting commands. + pub fn is_recording(&self) -> bool { + self.recorder.is_some() + } + + /// How many commands are running: 0 between top-level commands, 1 inside one, 2 inside a + /// command run by a command, and so on. + pub fn depth(&self) -> u32 { + self.depth + } + + /// Whether an undo group is open on the document with this uid. + pub fn in_group(&self, doc: u64) -> bool { + self.groups.iter().any(|g| g.doc == doc) + } + + /// Removes the token's group and any group on the same document opened after it. + fn take_group(&mut self, token: &GroupToken) -> Result { + let ended = || EngineError::Other("That undo group has already ended.".into()); + let at = self.groups.iter().position(|g| g.id == token.id).ok_or_else(ended)?; + let mut later = self.groups.split_off(at).into_iter(); + let g = later.next().ok_or_else(ended)?; + self.groups.extend(later.filter(|o| o.doc != g.doc)); + self.group_epoch = self.group_epoch.wrapping_add(1); + Ok(g) } pub fn run(&mut self, id: &str, params: Json) -> std::result::Result { @@ -468,6 +896,8 @@ impl Session { let r = self.execute(id, params); let requests = self.take_ui_requests(); let asked_for_file = requests.iter().any(|q| matches!(q, UiRequest::Dialog(name, _) if name == "open" || name == "saveAs")); + // A call that only opened a dialog is not recorded (see `execute`), so the recording + // has no step to take back here. if r.is_ok() && asked_for_file { let what = match id { "file.open" => "`path` or `base64`", diff --git a/crates/engine/src/record.rs b/crates/engine/src/record.rs new file mode 100644 index 000000000..7804be6c9 --- /dev/null +++ b/crates/engine/src/record.rs @@ -0,0 +1,66 @@ +//! The recording buffer: the commands a person (or an agent) ran while recording, each with the +//! document, sheet and selection it ran in. +//! +//! Only top-level calls are kept. A command that runs other commands (Custom Views › Show, a +//! script) is one step, so playing the recording back does not run the inner commands twice. The +//! buffer is separate from the journal, so trimming the journal never loses recorded steps, and +//! secret parameters are left out as they are in the journal ([`crate::cmd::journal_params`]). + +use serde::Serialize; +use serde_json::Value as Json; + +use crate::Selection; + +/// The most steps one recording keeps; later ones are counted in [`Recorder::dropped`]. +pub const RECORD_LIMIT: usize = 100_000; +/// The most bytes (steps serialized as JSON) one recording keeps. A step keeps its full +/// parameters so it can be played back, and a `file.open` holds the whole file as base64, so +/// without a size cap a recording of a few opens of a large workbook keeps tens of MB alive. +pub const RECORD_MAX_BYTES: usize = 64 * 1024 * 1024; + +/// One recorded command and where it ran. +#[derive(Clone, Debug, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct RecordedStep { + /// Uid of the document that was active when the command ran (0 when none was open). + pub doc: u64, + /// That document's active sheet (index and name) just before the command ran. + pub sheet: usize, + pub sheet_name: String, + /// That document's selection just before the command ran. + pub selection: Selection, + /// The command id and its parameters, without secrets. + pub id: String, + pub params: Json, +} + +/// The steps recorded since recording started. +#[derive(Clone, Debug, Default)] +pub struct Recorder { + pub steps: Vec, + /// Steps not kept because the recording reached [`RECORD_LIMIT`] or [`RECORD_MAX_BYTES`]. + /// Once a step is dropped, every later one is too, so `steps` is always the beginning of + /// what ran, never a sequence with gaps. + pub dropped: usize, + /// The size of `steps` serialized as JSON. + bytes: usize, +} + +impl Recorder { + /// Adds a step, or counts it as dropped once the buffer is full. + pub fn push(&mut self, step: RecordedStep) { + // Once a step has been dropped, or the step count is full, the rest are counted without + // being serialized (a `file.open` step holds a whole file). + if self.dropped > 0 || self.steps.len() >= RECORD_LIMIT { + self.dropped = self.dropped.saturating_add(1); + return; + } + let size = serde_json::to_vec(&step).map_or(0, |v| v.len()); + if self.bytes.saturating_add(size) <= RECORD_MAX_BYTES { + self.bytes = self.bytes.saturating_add(size); + self.steps.push(step); + } else { + self.dropped = self.dropped.saturating_add(1); + } + } +} diff --git a/crates/engine/src/sample.rs b/crates/engine/src/sample.rs index 129fe2171..90029bac3 100644 --- a/crates/engine/src/sample.rs +++ b/crates/engine/src/sample.rs @@ -26,7 +26,7 @@ pub fn build(name: &str) -> Option { _ => return None, }; } - s.add_document(DocState::new(wb, None, title(name))); + s.add_document(DocState::new(wb, None, title(name), crate::Trust::Local)); let ok = match name { "budget" => budget(&mut s), "sales" => sales(&mut s), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 756a4d090..1be90d56c 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3,6 +3,11 @@ use serde_json::json; use crate::Session; +thread_local! { + /// Copies of a document's undo and redo history `execute` has taken on this thread. + pub(crate) static HISTORY_COPIES: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + fn s() -> Session { let mut s = Session::new(); s.new_workbook(); @@ -4599,3 +4604,4 @@ fn html_export_without_pictures_or_charts_has_no_fixed_geometry() { let html = crate::io::to_html(&s.doc().unwrap().wb, 0).unwrap(); assert!(!html.contains("") && !html.contains("table-layout") && !html.contains("