From f6546156408fd4809689b3305358b47281e22b7b Mon Sep 17 00:00:00 2001 From: justinjohn0306 <34035011+justinjohn0306@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:04:14 +0530 Subject: [PATCH 01/27] Fix: list and render installed system fonts (#4) The font dropdown showed a hardcoded 17-name list and cells always rendered in the built-in Calibri/role font, so a font installed on the machine could not be listed or chosen. Both trace to one constraint: egui panics when asked for a FontFamily::Name that was never bound to font data, so only the 8 built-in roles were ever bound. Scan the platform font folders for installed families (a bounded read of each file's name table via skrifa, already compiled transitively through epaint), bind the faces a workbook actually uses into egui as fnt:{family}:{b|n}{i|n} roles chained to the defaults, and offer the installed families in the font combos after the standard set. cell_family returns the installed role when it is bound, else the previous bucketing, so it never names an unbound family. The scan runs in the background and the first frame that rebinds skips painting, like the existing startup gate. Native only; wasm keeps the standard list. Incidental: newer local clippy flagged pre-existing lints in untouched crates (no toolchain is pinned); fixed behavior-preservingly so the gate passes. --- Cargo.lock | 2 + Cargo.toml | 4 + crates/core/src/date.rs | 2 +- crates/engine/src/cmd/file.rs | 2 +- crates/engine/src/cmd/spelling.rs | 26 +- crates/functions/src/math.rs | 2 +- crates/functions/src/stat.rs | 2 +- crates/functions/src/util.rs | 2 +- crates/numfmt/src/render.rs | 2 +- crates/ui-egui/Cargo.toml | 2 + crates/ui-egui/src/dialogs.rs | 4 +- crates/ui-egui/src/fonts.rs | 423 ++++++++++++++++++++++++++++++ crates/ui-egui/src/formula_bar.rs | 2 +- crates/ui-egui/src/grid.rs | 9 +- crates/ui-egui/src/lib.rs | 52 +++- crates/ui-egui/src/ribbon.rs | 6 +- crates/ui-egui/src/theme.rs | 71 ++++- 17 files changed, 580 insertions(+), 33 deletions(-) create mode 100644 crates/ui-egui/src/fonts.rs diff --git a/Cargo.lock b/Cargo.lock index f3b1e64..0855bf0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1522,12 +1522,14 @@ version = "0.1.0" dependencies = [ "egui", "egui_kittest", + "epaint_default_fonts", "gridcraft-chart", "gridcraft-engine", "image", "log", "serde", "serde_json", + "skrifa", "toml", ] diff --git a/Cargo.toml b/Cargo.toml index a7f99e0..e87627d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,6 +43,10 @@ eframe = { version = "0.36", default-features = false } egui_kittest = "0.36" rfd = "0.17" arboard = { version = "3.6", default-features = false } +# Font name-table reading. The same version epaint already depends on, so it adds no new build. +skrifa = "0.44" +# Real font bytes for tests (Ubuntu Light, compiled into epaint). +epaint_default_fonts = "0.36" [workspace.lints.rust] unsafe_code = "forbid" diff --git a/crates/core/src/date.rs b/crates/core/src/date.rs index 7fba23f..d27ee15 100644 --- a/crates/core/src/date.rs +++ b/crates/core/src/date.rs @@ -97,7 +97,7 @@ pub fn serial_from_ymd(sys: DateSystem, year: i64, month: i64, day: i64) -> Opti /// Date/time from a serial. `None` for negative or too-large serials. pub fn datetime_from_serial(sys: DateSystem, serial: f64) -> Option { - if !serial.is_finite() || serial < 0.0 || serial >= 2_958_466.0 { + if !(0.0..2_958_466.0).contains(&serial) { return None; } let mut day = serial.floor() as i64; diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 37a58dd..34b957c 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -48,7 +48,7 @@ fn open(s: &mut Session, p: &Json) -> Result { }; let (wb, warnings) = crate::io::open_bytes(&name, &bytes)?; let title = std::path::Path::new(&name).file_name().and_then(|n| n.to_str()).unwrap_or("Book").to_string(); - let path = str_param(p, "path").map(str::to_string).filter(|p| crate::io::FileKind::from_path(p) == Some(crate::io::FileKind::Xlsx) || true); + let path = str_param(p, "path").map(str::to_string); // Replace an untouched blank Book1 like Excel does. if s.documents().len() == 1 && s.active().is_some_and(|d| d.path.is_none() && !d.is_dirty() && d.undo.is_empty() && d.wb.sheets.iter().all(|sh| sh.cells.is_empty())) diff --git a/crates/engine/src/cmd/spelling.rs b/crates/engine/src/cmd/spelling.rs index 10892bc..e149120 100644 --- a/crates/engine/src/cmd/spelling.rs +++ b/crates/engine/src/cmd/spelling.rs @@ -21,17 +21,23 @@ pub fn specs() -> Vec { fn dictionary() -> &'static HashSet { static DICT: OnceLock> = OnceLock::new(); DICT.get_or_init(|| { - let mut set = HashSet::new(); - #[cfg(not(target_arch = "wasm32"))] - for path in ["/usr/share/dict/words", "/usr/share/dict/american-english", "/usr/share/dict/british-english"] { - if let Ok(text) = std::fs::read_to_string(path) { - for w in text.lines().take(2_000_000) { - set.insert(w.trim().to_lowercase()); - } - break; - } + #[cfg(target_arch = "wasm32")] + { + HashSet::new() + } + #[cfg(not(target_arch = "wasm32"))] + { + let mut set = HashSet::new(); + for path in ["/usr/share/dict/words", "/usr/share/dict/american-english", "/usr/share/dict/british-english"] { + if let Ok(text) = std::fs::read_to_string(path) { + for w in text.lines().take(2_000_000) { + set.insert(w.trim().to_lowercase()); + } + break; + } + } + set } - set }) } diff --git a/crates/functions/src/math.rs b/crates/functions/src/math.rs index 3a0bab3..654395f 100644 --- a/crates/functions/src/math.rs +++ b/crates/functions/src/math.rs @@ -487,7 +487,7 @@ fn integer_args(a: &[Arg]) -> R> { v.iter() .map(|x| { let t = x.trunc(); - if t < 0.0 || t >= 9.007_199_254_740_992e15 { Err(CellError::Num) } else { Ok(t as u64) } + if !(0.0..9.007_199_254_740_992e15).contains(&t) { Err(CellError::Num) } else { Ok(t as u64) } }) .collect() } diff --git a/crates/functions/src/stat.rs b/crates/functions/src/stat.rs index abdb4e7..51a5f32 100644 --- a/crates/functions/src/stat.rs +++ b/crates/functions/src/stat.rs @@ -1185,7 +1185,7 @@ fn binom_dist_range(a: &[Arg], _: C) -> R { let p = num(a, 1)?; let s1 = num(a, 2)?.trunc(); let s2 = if has(a, 3) { num(a, 3)?.trunc() } else { s1 }; - if n < 0.0 || n > 1e15 || !(0.0..=1.0).contains(&p) || s1 < 0.0 || s1 > n || s2 < s1 || s2 > n { + if !(0.0..=1e15).contains(&n) || !(0.0..=1.0).contains(&p) || s1 < 0.0 || s1 > n || s2 < s1 || s2 > n { return Err(CellError::Num); } if s2 - s1 > 10_000.0 { diff --git a/crates/functions/src/util.rs b/crates/functions/src/util.rs index fca5dc4..fbd6649 100644 --- a/crates/functions/src/util.rs +++ b/crates/functions/src/util.rs @@ -51,7 +51,7 @@ pub(crate) fn arg(args: &[Arg], i: usize) -> R<&Arg> { /// Whether argument `i` was supplied (present and not an empty literal like `F(1,)`). pub(crate) fn has(args: &[Arg], i: usize) -> bool { - matches!(args.get(i), Some(a) if !(matches!(a.value, Value::Empty) && !a.from_ref)) + matches!(args.get(i), Some(a) if !matches!(a.value, Value::Empty) || a.from_ref) } /// Number coercion for a scalar parameter: empty = 0, TRUE = 1, numeric text parses. diff --git a/crates/numfmt/src/render.rs b/crates/numfmt/src/render.rs index 363dec1..523b77d 100644 --- a/crates/numfmt/src/render.rs +++ b/crates/numfmt/src/render.rs @@ -58,7 +58,7 @@ pub(crate) fn render_general(sec: &Section, v: f64, width: usize) -> Out { /// Renders a date/time. `None` when the serial is outside the supported range. pub(crate) fn render_date(sec: &Section, v: f64, sys: DateSystem) -> Option { - if !v.is_finite() || v < 0.0 || v >= 2_958_466.0 { + if !(0.0..2_958_466.0).contains(&v) { return None; } let sub_digits = sec.toks.iter().filter_map(|t| if let Tok::Date(DatePart::SubSec(n)) = t { Some(*n) } else { None }).max().unwrap_or(0).min(3); diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index 01d7c00..283d53f 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -16,9 +16,11 @@ serde = { workspace = true } serde_json = { workspace = true } log = { workspace = true } image = { workspace = true } +skrifa = { workspace = true } [dev-dependencies] egui_kittest = { workspace = true, features = ["wgpu", "snapshot"] } +epaint_default_fonts = { workspace = true } [build-dependencies] serde_json = { workspace = true } diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index b6c186d..e921cdc 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1020,8 +1020,8 @@ fn format_cells(app: &mut SheetApp, ui: &mut egui::Ui, d: &mut Dialog, confirm: } "Font" => { egui::ComboBox::from_label("Font").selected_text(st.font.name.clone()).show_ui(ui, |ui| { - for f in crate::ribbon::FONTS { - ui.selectable_value(&mut st.font.name, f.to_string(), *f); + for f in crate::fonts::families() { + ui.selectable_value(&mut st.font.name, f.clone(), f.clone()); } }); ui.add(egui::Slider::new(&mut st.font.size, 6.0..=72.0).text("Size")); diff --git a/crates/ui-egui/src/fonts.rs b/crates/ui-egui/src/fonts.rs new file mode 100644 index 0000000..940bca9 --- /dev/null +++ b/crates/ui-egui/src/fonts.rs @@ -0,0 +1,423 @@ +//! The fonts installed on the machine, so the font list can offer them and cells can render with +//! them (#4). +//! +//! egui has no font discovery of its own and panics when asked for a family that was never bound +//! to font data. So we do both halves: scan the platform's font folders for family names (a read +//! of each font file's `name` table — damaged or unreadable files yield nothing, never a panic), +//! and bind the files a workbook actually uses into the families +//! [`theme::font_definitions`](crate::theme::font_definitions) assembles. +//! +//! Native only: on wasm there are no system fonts, every function here reports "nothing", and the +//! font list is the standard [`crate::ribbon::FONTS`] set. + +use std::collections::HashSet; +use std::path::PathBuf; +use std::sync::OnceLock; + +/// A font file is skipped by the scan past this size: a catalog only needs each face's name, and a +/// damaged or enormous file must not turn startup into a long read. +#[cfg(not(target_arch = "wasm32"))] +const MAX_FONT_FILE: u64 = 64 << 20; + +/// One face of one installed font file. +#[derive(Clone, Debug)] +pub struct Face { + /// Typographic family name, e.g. `"Segoe UI"`. + pub family: String, + /// The file the face lives in. + pub path: PathBuf, + /// Face index within the file (collections hold several, e.g. a family's Regular and Bold). + pub index: u32, + /// Weight implied by the face's style name (400 = regular). + pub weight: f32, + /// Whether the style name reads as italic or oblique. + pub italic: bool, +} + +/// The font folders of the platform (the system's and the user's). Empty on wasm. +pub fn system_font_dirs() -> Vec { + #[cfg(target_arch = "wasm32")] + { + Vec::new() + } + #[cfg(not(target_arch = "wasm32"))] + { + let mut dirs: Vec = Vec::new(); + let home = std::env::var_os("HOME").or_else(|| std::env::var_os("USERPROFILE")).map(PathBuf::from); + if cfg!(target_os = "macos") { + dirs.extend(["/System/Library/Fonts", "/System/Library/Fonts/Supplemental", "/Library/Fonts"].map(Into::into)); + if let Some(h) = &home { + dirs.push(h.join("Library/Fonts")); + } + } else if cfg!(windows) { + let root = std::env::var_os("WINDIR").map(PathBuf::from).unwrap_or_else(|| PathBuf::from("C:\\Windows")); + dirs.push(root.join("Fonts")); + if let Some(l) = std::env::var_os("LOCALAPPDATA") { + dirs.push(PathBuf::from(l).join("Microsoft\\Windows\\Fonts")); + } + } else { + dirs.extend(["/usr/share/fonts", "/usr/local/share/fonts", "/usr/share/fonts/TTF"].map(Into::into)); + if let Some(h) = &home { + dirs.push(h.join(".fonts")); + dirs.push(h.join(".local/share/fonts")); + } + } + dirs + } +} + +/// Weight implied by a style name (`"Bold"` → 700, `"Light"` → 300). +#[cfg(not(target_arch = "wasm32"))] +fn style_weight(style: &str) -> f32 { + let s: String = style.chars().filter(|c| c.is_alphanumeric()).flat_map(|c| c.to_lowercase()).collect(); + const TABLE: &[(&str, f32)] = &[ + ("hairline", 100.0), + ("thin", 100.0), + ("extralight", 200.0), + ("ultralight", 200.0), + ("light", 300.0), + ("medium", 500.0), + ("demibold", 600.0), + ("semibold", 600.0), + ("extrabold", 800.0), + ("ultrabold", 800.0), + ("bold", 700.0), + ("black", 900.0), + ("heavy", 900.0), + ]; + TABLE.iter().find(|(k, _)| s.contains(k)).map(|(_, w)| *w).unwrap_or(400.0) +} + +/// Whether a style name reads as italic or oblique. +#[cfg(not(target_arch = "wasm32"))] +fn style_italic(style: &str) -> bool { + let s: String = style.chars().filter(|c| c.is_alphanumeric()).flat_map(|c| c.to_lowercase()).collect(); + s.contains("italic") || s.contains("oblique") +} + +/// Every face in `data` as `(index, family, style)`. Damaged data yields nothing, never a panic. +#[cfg(not(target_arch = "wasm32"))] +fn file_faces(data: &[u8]) -> Vec<(u32, String, String)> { + use skrifa::raw::FileRef; + use skrifa::{MetadataProvider, string::StringId}; + let count = match FileRef::new(data) { + Ok(FileRef::Font(_)) => 1, + Ok(FileRef::Collection(c)) => c.len(), + Err(_) => return Vec::new(), + }; + let name = |f: &skrifa::FontRef<'_>, ids: &[StringId]| -> Option { + ids.iter().find_map(|id| f.localized_strings(*id).english_or_first().map(|s| s.to_string()).filter(|s| !s.is_empty())) + }; + (0..count) + .filter_map(|i| { + let f = skrifa::FontRef::from_index(data, i).ok()?; + let family = name(&f, &[StringId::TYPOGRAPHIC_FAMILY_NAME, StringId::FAMILY_NAME])?; + let style = name(&f, &[StringId::TYPOGRAPHIC_SUBFAMILY_NAME, StringId::SUBFAMILY_NAME]).unwrap_or_else(|| "Regular".into()); + Some((i, family, style)) + }) + .collect() +} + +/// Catalog every face under `dirs`, recursing into subfolders. A folder reached twice (through a +/// symlink) is read once, so a link back to a parent ends the walk. +#[cfg(not(target_arch = "wasm32"))] +pub fn scan_dirs(dirs: &[PathBuf]) -> Vec { + let mut out: Vec = Vec::new(); + let mut stack: Vec = dirs.to_vec(); + let mut seen_dirs: HashSet = HashSet::new(); + while let Some(dir) = stack.pop() { + let key = std::fs::canonicalize(&dir).unwrap_or_else(|_| dir.clone()); + if !seen_dirs.insert(key) { + continue; + } + let Ok(entries) = std::fs::read_dir(&dir) else { continue }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + stack.push(path); + continue; + } + let is_font = + path.extension().and_then(|e| e.to_str()).is_some_and(|e| matches!(e.to_ascii_lowercase().as_str(), "ttf" | "otf" | "ttc" | "otc")); + if !is_font || std::fs::metadata(&path).map(|m| m.len() > MAX_FONT_FILE).unwrap_or(true) { + continue; + } + let Ok(data) = std::fs::read(&path) else { continue }; + for (index, family, style) in file_faces(&data) { + out.push(Face { family, path: path.clone(), index, weight: style_weight(&style), italic: style_italic(&style) }); + } + } + } + out +} + +#[cfg(target_arch = "wasm32")] +pub fn scan_dirs(_dirs: &[PathBuf]) -> Vec { + Vec::new() +} + +static CATALOG: OnceLock> = OnceLock::new(); + +/// The catalog of installed faces, scanned on first use. Blocks until the scan finishes. +pub fn catalog() -> &'static [Face] { + CATALOG.get_or_init(|| scan_dirs(&system_font_dirs())) +} + +/// The catalog if the scan has finished, else `None` — never blocks, for use while painting. +fn catalog_now() -> Option<&'static [Face]> { + CATALOG.get().map(Vec::as_slice) +} + +/// Start cataloging the installed fonts on a background thread, so the first frame doesn't wait for +/// it. A no-op on wasm and after the scan has started. +pub fn start_scan() { + static STARTED: OnceLock<()> = OnceLock::new(); + STARTED.get_or_init(|| { + #[cfg(not(target_arch = "wasm32"))] + if CATALOG.get().is_none() { + // A failed spawn leaves the scan to the first blocking lookup. + let _ = std::thread::Builder::new().name("font-scan".into()).spawn(|| { + catalog(); + }); + } + }); +} + +/// Whether the background scan has finished. +pub fn scan_done() -> bool { + CATALOG.get().is_some() +} + +/// The faces of one installed family, regular-weight first (the order roles pick from). +pub fn faces_of(family: &str) -> Vec { + let Some(cat) = catalog_now() else { return Vec::new() }; + let mut v: Vec = cat.iter().filter(|f| f.family.eq_ignore_ascii_case(family)).cloned().collect(); + v.sort_by(|a, b| a.weight.total_cmp(&b.weight).then(a.italic.cmp(&b.italic)).then(a.index.cmp(&b.index))); + v +} + +/// The subset of `names` that is installed, sorted and deduped. These are the families whose files +/// get bound for a workbook. +pub fn installed_among(names: &[String]) -> Vec { + let Some(cat) = catalog_now() else { return Vec::new() }; + let mut out: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); + for wanted in names { + if !seen.insert(wanted.to_lowercase()) { + continue; + } + if let Some(f) = cat.iter().find(|f| f.family.eq_ignore_ascii_case(wanted)) { + out.push(f.family.clone()); + } + } + out.sort_by_key(|a| a.to_lowercase()); + out +} + +/// The font list a combo box shows: the standard [`crate::ribbon::FONTS`] names first (what Excel +/// offers, and what we substitute), then every other installed family A–Z. Never blocks: until the +/// background scan finishes it returns just the standard names. +pub fn families() -> Vec { + let mut out: Vec = crate::ribbon::FONTS.iter().map(|s| s.to_string()).collect(); + let Some(cat) = catalog_now() else { return out }; + let mut seen: HashSet = out.iter().map(|s| s.to_lowercase()).collect(); + let mut installed: Vec = cat.iter().map(|f| f.family.clone()).collect(); + installed.sort_by_key(|a| a.to_lowercase()); + installed.dedup_by(|a, b| a.eq_ignore_ascii_case(b)); + for f in installed { + if seen.insert(f.to_lowercase()) { + out.push(f); + } + } + out +} + +/// The egui family name for one role of one family: `fnt:{family}:{b|n}{i|n}`. +pub fn role_key(family: &str, bold: bool, italic: bool) -> String { + format!("fnt:{family}:{}{}", if bold { 'b' } else { 'n' }, if italic { 'i' } else { 'n' }) +} + +/// The face to use for a role, or `None` when the family has no faces. Missing bold/italic faces +/// fall back to the family's regular face, so every bound role has data behind it. +fn pick_face(faces: &[Face], bold: bool, italic: bool) -> Option<&Face> { + let regular = || faces.iter().find(|f| !f.italic && (f.weight - 400.0).abs() < 1.0); + let exact = |wanted_bold: bool, wanted_italic: bool| { + faces.iter().find(|f| f.italic == wanted_italic && if wanted_bold { f.weight >= 600.0 } else { f.weight < 600.0 }) + }; + (match (bold, italic) { + (true, true) => exact(true, true).or_else(|| exact(true, false)).or_else(|| exact(false, true)), + (true, false) => exact(true, false), + (false, true) => exact(false, true), + (false, false) => None, + }) + .or_else(regular) + .or_else(|| faces.first()) +} + +/// A stable key for a face's font data, unique per file/index within the process. +fn face_key(family: &str, face: &Face) -> String { + let file = face.path.file_name().and_then(|s| s.to_str()).unwrap_or("font"); + format!("fnt-src:{family}:{file}:{}", face.index) +} + +/// Bind one installed family's faces into `fonts`, one egui family per cell role, and return the +/// role keys that are now bound. A role with no exact face chains to the family's regular face, so +/// no role is ever left pointing at missing data (egui panics on that). A family with no faces +/// binds nothing. +/// +/// Each role's chain ends with egui's default proportional fonts, so glyphs the installed font +/// lacks (emoji, a stray script) still render. +pub fn bind_faces(fonts: &mut egui::FontDefinitions, family: &str, faces: &[Face]) -> Vec { + if faces.is_empty() { + return Vec::new(); + } + let base: Vec = fonts.families.get(&egui::FontFamily::Proportional).cloned().unwrap_or_default(); + let mut bound: Vec = Vec::new(); + for (bold, italic) in [(false, false), (true, false), (false, true), (true, true)] { + let Some(primary) = pick_face(faces, bold, italic) else { continue }; + let mut chain: Vec = Vec::new(); + for face in [Some(primary), pick_face(faces, false, false)] { + let Some(face) = face else { continue }; + let key = face_key(family, face); + if chain.contains(&key) { + continue; + } + let Ok(bytes) = std::fs::read(&face.path) else { continue }; + let mut fd = egui::FontData::from_owned(bytes); + fd.index = face.index; + fonts.font_data.entry(key.clone()).or_insert_with(|| std::sync::Arc::new(fd)); + chain.push(key); + } + if chain.is_empty() { + continue; + } + chain.extend(base.iter().cloned()); + let role = role_key(family, bold, italic); + fonts.families.insert(egui::FontFamily::Name(role.clone().into()), chain); + bound.push(role); + } + bound +} + +/// Bind every installed face of `family` (see [`bind_faces`]). +pub fn bind_family(fonts: &mut egui::FontDefinitions, family: &str) -> Vec { + bind_faces(fonts, family, &faces_of(family)) +} + +/// The role families currently bound to font data. `cell_family` consults this, so it never names a +/// family egui hasn't been given. +static BOUND: OnceLock>> = OnceLock::new(); + +fn bound() -> &'static std::sync::RwLock> { + BOUND.get_or_init(|| std::sync::RwLock::new(HashSet::new())) +} + +/// Record the role families a freshly built [`egui::FontDefinitions`] bound. +pub fn set_bound(keys: impl IntoIterator) { + *bound().write().unwrap_or_else(|e| e.into_inner()) = keys.into_iter().collect(); +} + +/// Is this exact role family bound to font data? +pub fn is_bound(role: &str) -> bool { + bound().read().unwrap_or_else(|e| e.into_inner()).contains(role) +} + +/// Serializes tests that mutate the process-wide bound-family registry. +#[cfg(test)] +pub(crate) static TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn style_names_map_to_weight_and_slant() { + assert_eq!(style_weight("Bold"), 700.0); + assert_eq!(style_weight("SemiBold Italic"), 600.0); + assert_eq!(style_weight("Regular"), 400.0); + assert_eq!(style_weight("Light Oblique"), 300.0); + assert!(style_italic("Bold Italic") && style_italic("Oblique")); + assert!(!style_italic("Bold")); + // The weight table is checked in order, so "extralight" wins over "light". + assert_eq!(style_weight("ExtraLight"), 200.0); + } + + #[test] + fn role_keys_are_stable_and_distinct() { + assert_eq!(role_key("Segoe UI", false, false), "fnt:Segoe UI:nn"); + assert_eq!(role_key("Segoe UI", true, false), "fnt:Segoe UI:bn"); + assert_eq!(role_key("Segoe UI", false, true), "fnt:Segoe UI:ni"); + assert_eq!(role_key("Segoe UI", true, true), "fnt:Segoe UI:bi"); + } + + #[test] + fn nothing_is_bound_until_font_definitions_says_so() { + let _g = TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + // A role key is only safe to name after font_definitions bound it. + let k = role_key("Some Installed Font", true, false); + assert!(!is_bound(&k)); + set_bound([k.clone()]); + assert!(is_bound(&k)); + set_bound([]); + assert!(!is_bound(&k)); + } + + #[test] + fn damaged_or_non_font_files_contribute_nothing() { + assert!(file_faces(b"ttcf\x00\x01\x00\x00\xff\xff\xff\xff").is_empty()); + assert!(file_faces(b"not a font at all").is_empty()); + assert!(file_faces(&[]).is_empty()); + } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn the_scan_catalogs_fonts_and_reads_each_folder_once() { + let dir = std::env::temp_dir().join(format!("gridcraft-fonts-scan-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(dir.join("Sub")).expect("temp dir"); + std::fs::write(dir.join("Ubuntu-Light.ttf"), epaint_default_fonts::UBUNTU_LIGHT).expect("write"); + std::fs::write(dir.join("damaged.ttf"), b"ttcf\x00\x01\x00\x00\xff\xff\xff\xff").expect("write"); + std::fs::write(dir.join("readme.txt"), b"not a font").expect("write"); + // The same folder listed twice (once through `Sub/..`) is read once. + let faces = scan_dirs(&[dir.clone(), dir.join("Sub/..")]); + let families: Vec<&str> = faces.iter().map(|f| f.family.as_str()).collect(); + assert_eq!(families, ["Ubuntu"], "one face, family read from the name table"); + assert_eq!(faces[0].weight, 300.0, "“Light” reads as weight 300"); + assert!(!faces[0].italic); + let _ = std::fs::remove_dir_all(&dir); + } + + #[cfg(unix)] + #[test] + fn a_link_back_to_a_parent_folder_ends_the_scan() { + let dir = std::env::temp_dir().join(format!("gridcraft-fonts-loop-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(dir.join("Sub")).expect("temp dir"); + std::fs::write(dir.join("Sub/Ubuntu-Light.ttf"), epaint_default_fonts::UBUNTU_LIGHT).expect("write"); + std::os::unix::fs::symlink(&dir, dir.join("Sub/loop")).expect("symlink"); + assert_eq!(scan_dirs(&[dir.clone()]).len(), 1, "the loop doesn't multiply faces"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn binding_a_family_binds_all_four_roles_and_never_stores_missing_data() { + let dir = std::env::temp_dir().join(format!("gridcraft-fonts-bind-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("temp dir"); + let path = dir.join("Ubuntu-Light.ttf"); + std::fs::write(&path, epaint_default_fonts::UBUNTU_LIGHT).expect("write"); + let face = Face { family: "Ubuntu".into(), path, index: 0, weight: 300.0, italic: false }; + + let mut fonts = egui::FontDefinitions::default(); + let bound = bind_faces(&mut fonts, "Ubuntu", std::slice::from_ref(&face)); + assert_eq!(bound.len(), 4, "regular, bold, italic and bold-italic roles"); + for key in &bound { + let chain = fonts.families.get(&egui::FontFamily::Name(key.as_str().into())).expect("role family"); + assert!(chain.iter().all(|k| fonts.font_data.contains_key(k)), "every chained key has data"); + } + // A family with no faces binds nothing and never touches the map. + assert!(bind_faces(&mut fonts, "No Such Font", &[]).is_empty()); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/ui-egui/src/formula_bar.rs b/crates/ui-egui/src/formula_bar.rs index 753585f..48141c3 100644 --- a/crates/ui-egui/src/formula_bar.rs +++ b/crates/ui-egui/src/formula_bar.rs @@ -166,7 +166,7 @@ pub fn editor_widget(app: &mut SheetApp, ui: &mut egui::Ui, id: egui::Id, font: ed.caret_to_end = false; ed.sync_caret = true; } - if !ed.autocomplete.is_empty() && (tab || (enter && false)) { + if !ed.autocomplete.is_empty() && tab { ed.accept_autocomplete(); } else if enter { action = Some((if shift { -1 } else { 1 }, 0, false, false)); diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4b326cc..4faf43a 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -577,7 +577,7 @@ fn paint_quadrant( }; let indent = st.align.indent as f32 * 9.0 * z; let pad = 2.5 * z; - if st.align.wrap || st.align.h == HAlign::Justify || text.contains('\n') && st.align.wrap { + if st.align.wrap || st.align.h == HAlign::Justify { let wrap_w = (rect.width() - 2.0 * pad - indent).max(4.0); let mut job = egui::text::LayoutJob::simple(text.clone(), font.clone(), color, wrap_w); job.halign = match halign { @@ -1651,12 +1651,7 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { let Some(sh) = wb.sheet(si) else { return }; let _ = geo; // Tables get an autofilter on demand. - let table_range = sh - .tables - .iter() - .find(|t| t.range.start.row <= at.y as u32 || true) - .filter(|t| col >= t.range.start.col && col <= t.range.end.col) - .map(|t| t.range); + let table_range = sh.tables.first().filter(|t| col >= t.range.start.col && col <= t.range.end.col).map(|t| t.range); if sh.autofilter.is_none() && let Some(r) = table_range { diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 8eb1eca..fd08ba3 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -11,6 +11,7 @@ pub mod control; pub mod credits; pub mod dialogs; pub mod editor; +pub mod fonts; pub mod formula_bar; pub mod grid; pub mod icons; @@ -104,6 +105,14 @@ pub struct SheetApp { pub perf: Perf, pub fonts_ready: bool, fonts_set: bool, + /// Workbook fonts bound into the current font set; a change rebinds (installed fonts only). + fonts_bound: Vec, + /// The (doc uid, revision) `fonts_bound` was computed for, so the style table is walked only + /// when the workbook changes. + fonts_seen: (u64, u64), + /// Set while a font-set change is pending activation: skip this frame's paint, like the + /// startup `fonts_ready` gate (egui applies new definitions at the next pass). + fonts_rebind: bool, pub name_box: Option, pub(crate) shots: control::Shots, /// Chart selected on the sheet (id). @@ -133,6 +142,9 @@ impl SheetApp { perf: Perf::default(), fonts_ready: false, fonts_set: false, + fonts_bound: Vec::new(), + fonts_seen: (0, 0), + fonts_rebind: false, name_box: None, shots: control::Shots::default(), selected_chart: None, @@ -140,12 +152,28 @@ impl SheetApp { } } - /// One-time context setup: fonts and visuals. + /// One-time context setup: start cataloging the machine's fonts, then install the fonts and + /// visuals. Workbook-specific fonts are bound later, once the scan is done (see [`logic`]). + /// + /// [`logic`]: SheetApp::logic pub fn setup_context(ctx: &egui::Context, dark: bool) { - ctx.set_fonts(theme::font_definitions()); + fonts::start_scan(); + ctx.set_fonts(theme::font_definitions(&[])); theme::apply(ctx, dark); } + /// The font names the active workbook uses: every style's font plus the theme's heading/body + /// fonts. These are the families [`fonts`] binds an installed file for. + pub fn used_fonts(&self) -> Vec { + let mut names: Vec = Vec::new(); + if let Some(d) = self.session.active() { + names.extend(d.wb.styles.iter().map(|(_, s)| s.font.name.clone())); + names.push(d.wb.theme.major_font.clone()); + names.push(d.wb.theme.minor_font.clone()); + } + names + } + /// Runs an engine command (or a UI command) and handles UI requests it makes. pub fn run(&mut self, id: &str, params: Json) -> Result { if let Some(r) = self.run_ui_command(id, ¶ms) { @@ -361,6 +389,24 @@ impl SheetApp { self.fonts_set = true; ctx.request_repaint(); } + } else if self.fonts_rebind { + // A font-set change made last frame is active now. + self.fonts_rebind = false; + } else if fonts::scan_done() { + // Bind the workbook's installed fonts once the background scan has finished. Adding + // fonts re-applies the font set, which egui activates at the next pass, so skip this + // frame's paint (same one-frame cost as startup). Only when the workbook changed. + let seen = self.session.active().map(|d| (d.uid, d.revision)).unwrap_or((0, 0)); + if seen != self.fonts_seen { + let used = self.used_fonts(); + self.fonts_seen = seen; + if used != self.fonts_bound { + ctx.set_fonts(theme::font_definitions(&used)); + self.fonts_bound = used; + self.fonts_rebind = true; + ctx.request_repaint(); + } + } } if ctx.global_style().visuals.dark_mode != self.ui.dark { theme::apply(ctx, self.ui.dark); @@ -394,7 +440,7 @@ impl SheetApp { /// Lays out the whole window. pub fn ui(&mut self, ui: &mut egui::Ui) { let ctx = ui.ctx().clone(); - if !self.fonts_ready { + if !self.fonts_ready || self.fonts_rebind { ctx.request_repaint(); return; } diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 47c5432..6abe3f3 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -688,9 +688,9 @@ fn font_combo(app: &mut SheetApp, ui: &mut Ui, st: &Style) { let mut name = st.font.name.clone(); let before = name.clone(); egui::ComboBox::from_id_salt("font_name").width(150.0).selected_text(egui::RichText::new(&name).font(theme::ui_font(12.5))).show_ui(ui, |ui| { - for f in FONTS { - let fam = theme::cell_family(f, false, false); - ui.selectable_value(&mut name, f.to_string(), egui::RichText::new(*f).font(egui::FontId::new(14.0, fam))); + for f in crate::fonts::families() { + let fam = theme::cell_family(&f, false, false); + ui.selectable_value(&mut name, f.clone(), egui::RichText::new(&f).font(egui::FontId::new(14.0, fam))); } }); if name != before { diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 2dbd8df..1b51559 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -142,7 +142,16 @@ pub fn ui_bold(size: f32) -> FontId { } /// The font family to use for a cell font name and weight. +/// +/// When the name is an installed font whose faces were bound by [`font_definitions`], that family +/// is used; otherwise the name is bucketed to one of the built-in roles (mono/serif/cell). egui +/// panics on a family that was never bound, so the installed branch is gated on +/// [`crate::fonts::is_bound`] and never names a family egui hasn't been given. pub fn cell_family(name: &str, bold: bool, italic: bool) -> FontFamily { + let role = crate::fonts::role_key(name, bold, italic); + if crate::fonts::is_bound(&role) { + return FontFamily::Name(role.into()); + } let lower = name.to_ascii_lowercase(); if lower.contains("courier") || lower.contains("mono") || lower.contains("consolas") { return FontFamily::Name(MONO.into()); @@ -288,10 +297,16 @@ fn candidates(role: &str) -> Vec<(String, u32)> { } /// Builds the font set from system fonts (with egui's defaults as fallback). -pub fn font_definitions() -> FontDefinitions { +/// +/// `used` names the workbook's fonts (its styles' fonts and the theme's heading/body fonts). Any of +/// them that is installed is bound by name, so cells set in it render with it; the rest fall back +/// to the roles above. The set of bound role families is recorded in [`crate::fonts`] so +/// [`cell_family`] only ever names a family egui has data for. +pub fn font_definitions(used: &[String]) -> FontDefinitions { let mut fonts = FontDefinitions::default(); let base_prop: Vec = fonts.families.get(&FontFamily::Proportional).cloned().unwrap_or_default(); let base_mono: Vec = fonts.families.get(&FontFamily::Monospace).cloned().unwrap_or_default(); + let mut bound: Vec = Vec::new(); for role in [UI, UI_BOLD, CELL, CELL_BOLD, CELL_ITALIC, CELL_BOLD_ITALIC, SERIF, MONO] { let paths = candidates(role); let refs: Vec<(&str, u32)> = paths.iter().map(|(p, i)| (p.as_str(), *i)).collect(); @@ -308,11 +323,17 @@ pub fn font_definitions() -> FontDefinitions { chain.extend(if role == MONO { base_mono.clone() } else { base_prop.clone() }); chain.retain(|k| fonts.font_data.contains_key(k)); fonts.families.insert(FontFamily::Name(role.into()), chain); + bound.push(role.to_string()); + } + // Installed workbook fonts, so a cell's chosen font actually renders in it. + for family in crate::fonts::installed_among(used) { + bound.extend(crate::fonts::bind_family(&mut fonts, &family)); } // Default proportional text in widgets uses the UI font. if let Some(ui) = fonts.families.get(&FontFamily::Name(UI.into())).cloned() { fonts.families.insert(FontFamily::Proportional, ui); } + crate::fonts::set_bound(bound); fonts } @@ -351,3 +372,51 @@ pub fn apply(ctx: &egui::Context, dark: bool) { pub fn color32(rgb: [u8; 3]) -> Color32 { Color32::from_rgb(rgb[0], rgb[1], rgb[2]) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The families [`font_definitions`] always binds, whatever is installed. + fn always_bound() -> Vec { + [UI, UI_BOLD, CELL, CELL_BOLD, CELL_ITALIC, CELL_BOLD_ITALIC, SERIF, MONO].iter().map(|s| s.to_string()).collect() + } + + #[test] + fn a_name_that_is_not_an_installed_font_gets_a_built_in_role() { + let _g = crate::fonts::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + // Nothing installed: every name buckets to a role that font_definitions bound. + crate::fonts::set_bound(always_bound()); + for name in ["", "Calibri", "Courier New", "Times New Roman", "Grossly Missing Font", "\u{1F600}"] { + for (b, i) in [(false, false), (true, false), (false, true), (true, true)] { + let FontFamily::Name(fam) = cell_family(name, b, i) else { panic!("{name:?} should name a family") }; + assert!(crate::fonts::is_bound(&fam), "{name:?} {b} {i} → unbound {fam:?}"); + } + } + } + + #[test] + fn a_bound_installed_font_is_used_by_name() { + let _g = crate::fonts::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let role = crate::fonts::role_key("Ubuntu", true, false); + crate::fonts::set_bound(always_bound().into_iter().chain([role.clone()])); + assert!(matches!(cell_family("Ubuntu", true, false), FontFamily::Name(f) if *f == *role)); + // The same family in a style that wasn't bound falls back to a built-in role that is. + let FontFamily::Name(fallback) = cell_family("Ubuntu", false, true) else { panic!("should name a family") }; + assert!(crate::fonts::is_bound(&fallback), "the fallback is a bound role"); + crate::fonts::set_bound(always_bound()); + } + + #[test] + fn arbitrary_names_never_yield_an_unbound_family() { + let _g = crate::fonts::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + // A workbook can hold any font name; none may name a family egui has no data for. + crate::fonts::set_bound(always_bound()); + for seed in 0..500u32 { + let name: String = + (0..(seed % 40)).map(|k| char::from_u32(0x20 + (seed.wrapping_mul(31).wrapping_add(k) % 0x5F)).unwrap_or('x')).collect(); + let FontFamily::Name(fam) = cell_family(&name, seed % 2 == 0, seed % 3 == 0) else { continue }; + assert!(crate::fonts::is_bound(&fam), "{name:?} → unbound {fam:?}"); + } + } +} From d9fd1b7305a1e5cda8a2d0a688950d86a642ab8b Mon Sep 17 00:00:00 2001 From: justinjohn0306 <34035011+justinjohn0306@users.noreply.github.com> Date: Fri, 9 Oct 2026 00:53:54 +0530 Subject: [PATCH 02/27] CI: satisfy newer clippy lints (toolchain drift, behavior-preserving) Local clippy (1.96) flags pre-existing code that an older toolchain accepted; these lints fire on clean main and block `cargo xtask ci`. All rewrites are behavior-preserving: - core/date.rs, numfmt/render.rs, functions/math.rs, functions/stat.rs: `!x.is_finite() || x < a || x >= b` -> `!(a..b).contains(&x)` (Range::contains is false for NaN/inf, matching the existing is_finite guard). - functions/util.rs: `!(A && !B)` -> `!A || B`. - engine/cmd/file.rs: drop a dead `.filter(|| true)`. - ui-egui/grid.rs: `find(|| true)` was always the first table (use `.first()`); `wrap || Justify || (nl && wrap)` -> `wrap || Justify`. - ui-egui/formula_bar.rs: `tab || (enter && false)` -> `tab`. - engine/cmd/spelling.rs: split the wasm/non-wasm dictionary init so the non-wasm `mut` no longer warns under wasm check. --- crates/core/src/date.rs | 2 +- crates/engine/src/cmd/file.rs | 2 +- crates/engine/src/cmd/spelling.rs | 26 ++++++++++++++++---------- crates/functions/src/math.rs | 2 +- crates/functions/src/stat.rs | 2 +- crates/functions/src/util.rs | 2 +- crates/numfmt/src/render.rs | 2 +- crates/ui-egui/src/formula_bar.rs | 2 +- crates/ui-egui/src/grid.rs | 9 ++------- 9 files changed, 25 insertions(+), 24 deletions(-) diff --git a/crates/core/src/date.rs b/crates/core/src/date.rs index 7fba23f..167522b 100644 --- a/crates/core/src/date.rs +++ b/crates/core/src/date.rs @@ -97,7 +97,7 @@ pub fn serial_from_ymd(sys: DateSystem, year: i64, month: i64, day: i64) -> Opti /// Date/time from a serial. `None` for negative or too-large serials. pub fn datetime_from_serial(sys: DateSystem, serial: f64) -> Option { - if !serial.is_finite() || serial < 0.0 || serial >= 2_958_466.0 { + if !serial.is_finite() || !(0.0..2_958_466.0).contains(&serial) { return None; } let mut day = serial.floor() as i64; diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 37a58dd..34b957c 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -48,7 +48,7 @@ fn open(s: &mut Session, p: &Json) -> Result { }; let (wb, warnings) = crate::io::open_bytes(&name, &bytes)?; let title = std::path::Path::new(&name).file_name().and_then(|n| n.to_str()).unwrap_or("Book").to_string(); - let path = str_param(p, "path").map(str::to_string).filter(|p| crate::io::FileKind::from_path(p) == Some(crate::io::FileKind::Xlsx) || true); + let path = str_param(p, "path").map(str::to_string); // Replace an untouched blank Book1 like Excel does. if s.documents().len() == 1 && s.active().is_some_and(|d| d.path.is_none() && !d.is_dirty() && d.undo.is_empty() && d.wb.sheets.iter().all(|sh| sh.cells.is_empty())) diff --git a/crates/engine/src/cmd/spelling.rs b/crates/engine/src/cmd/spelling.rs index 10892bc..e149120 100644 --- a/crates/engine/src/cmd/spelling.rs +++ b/crates/engine/src/cmd/spelling.rs @@ -21,17 +21,23 @@ pub fn specs() -> Vec { fn dictionary() -> &'static HashSet { static DICT: OnceLock> = OnceLock::new(); DICT.get_or_init(|| { - let mut set = HashSet::new(); - #[cfg(not(target_arch = "wasm32"))] - for path in ["/usr/share/dict/words", "/usr/share/dict/american-english", "/usr/share/dict/british-english"] { - if let Ok(text) = std::fs::read_to_string(path) { - for w in text.lines().take(2_000_000) { - set.insert(w.trim().to_lowercase()); - } - break; - } + #[cfg(target_arch = "wasm32")] + { + HashSet::new() + } + #[cfg(not(target_arch = "wasm32"))] + { + let mut set = HashSet::new(); + for path in ["/usr/share/dict/words", "/usr/share/dict/american-english", "/usr/share/dict/british-english"] { + if let Ok(text) = std::fs::read_to_string(path) { + for w in text.lines().take(2_000_000) { + set.insert(w.trim().to_lowercase()); + } + break; + } + } + set } - set }) } diff --git a/crates/functions/src/math.rs b/crates/functions/src/math.rs index 3a0bab3..654395f 100644 --- a/crates/functions/src/math.rs +++ b/crates/functions/src/math.rs @@ -487,7 +487,7 @@ fn integer_args(a: &[Arg]) -> R> { v.iter() .map(|x| { let t = x.trunc(); - if t < 0.0 || t >= 9.007_199_254_740_992e15 { Err(CellError::Num) } else { Ok(t as u64) } + if !(0.0..9.007_199_254_740_992e15).contains(&t) { Err(CellError::Num) } else { Ok(t as u64) } }) .collect() } diff --git a/crates/functions/src/stat.rs b/crates/functions/src/stat.rs index abdb4e7..51a5f32 100644 --- a/crates/functions/src/stat.rs +++ b/crates/functions/src/stat.rs @@ -1185,7 +1185,7 @@ fn binom_dist_range(a: &[Arg], _: C) -> R { let p = num(a, 1)?; let s1 = num(a, 2)?.trunc(); let s2 = if has(a, 3) { num(a, 3)?.trunc() } else { s1 }; - if n < 0.0 || n > 1e15 || !(0.0..=1.0).contains(&p) || s1 < 0.0 || s1 > n || s2 < s1 || s2 > n { + if !(0.0..=1e15).contains(&n) || !(0.0..=1.0).contains(&p) || s1 < 0.0 || s1 > n || s2 < s1 || s2 > n { return Err(CellError::Num); } if s2 - s1 > 10_000.0 { diff --git a/crates/functions/src/util.rs b/crates/functions/src/util.rs index fca5dc4..fbd6649 100644 --- a/crates/functions/src/util.rs +++ b/crates/functions/src/util.rs @@ -51,7 +51,7 @@ pub(crate) fn arg(args: &[Arg], i: usize) -> R<&Arg> { /// Whether argument `i` was supplied (present and not an empty literal like `F(1,)`). pub(crate) fn has(args: &[Arg], i: usize) -> bool { - matches!(args.get(i), Some(a) if !(matches!(a.value, Value::Empty) && !a.from_ref)) + matches!(args.get(i), Some(a) if !matches!(a.value, Value::Empty) || a.from_ref) } /// Number coercion for a scalar parameter: empty = 0, TRUE = 1, numeric text parses. diff --git a/crates/numfmt/src/render.rs b/crates/numfmt/src/render.rs index 363dec1..79fe490 100644 --- a/crates/numfmt/src/render.rs +++ b/crates/numfmt/src/render.rs @@ -58,7 +58,7 @@ pub(crate) fn render_general(sec: &Section, v: f64, width: usize) -> Out { /// Renders a date/time. `None` when the serial is outside the supported range. pub(crate) fn render_date(sec: &Section, v: f64, sys: DateSystem) -> Option { - if !v.is_finite() || v < 0.0 || v >= 2_958_466.0 { + if !v.is_finite() || !(0.0..2_958_466.0).contains(&v) { return None; } let sub_digits = sec.toks.iter().filter_map(|t| if let Tok::Date(DatePart::SubSec(n)) = t { Some(*n) } else { None }).max().unwrap_or(0).min(3); diff --git a/crates/ui-egui/src/formula_bar.rs b/crates/ui-egui/src/formula_bar.rs index 753585f..48141c3 100644 --- a/crates/ui-egui/src/formula_bar.rs +++ b/crates/ui-egui/src/formula_bar.rs @@ -166,7 +166,7 @@ pub fn editor_widget(app: &mut SheetApp, ui: &mut egui::Ui, id: egui::Id, font: ed.caret_to_end = false; ed.sync_caret = true; } - if !ed.autocomplete.is_empty() && (tab || (enter && false)) { + if !ed.autocomplete.is_empty() && tab { ed.accept_autocomplete(); } else if enter { action = Some((if shift { -1 } else { 1 }, 0, false, false)); diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4b326cc..4faf43a 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -577,7 +577,7 @@ fn paint_quadrant( }; let indent = st.align.indent as f32 * 9.0 * z; let pad = 2.5 * z; - if st.align.wrap || st.align.h == HAlign::Justify || text.contains('\n') && st.align.wrap { + if st.align.wrap || st.align.h == HAlign::Justify { let wrap_w = (rect.width() - 2.0 * pad - indent).max(4.0); let mut job = egui::text::LayoutJob::simple(text.clone(), font.clone(), color, wrap_w); job.halign = match halign { @@ -1651,12 +1651,7 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { let Some(sh) = wb.sheet(si) else { return }; let _ = geo; // Tables get an autofilter on demand. - let table_range = sh - .tables - .iter() - .find(|t| t.range.start.row <= at.y as u32 || true) - .filter(|t| col >= t.range.start.col && col <= t.range.end.col) - .map(|t| t.range); + let table_range = sh.tables.first().filter(|t| col >= t.range.start.col && col <= t.range.end.col).map(|t| t.range); if sh.autofilter.is_none() && let Some(r) = table_range { From 726a31740e63cb65de3d0c4cbdf8147d297224d4 Mon Sep 17 00:00:00 2001 From: justinjohn0306 <34035011+justinjohn0306@users.noreply.github.com> Date: Fri, 9 Oct 2026 00:54:10 +0530 Subject: [PATCH 03/27] Preserve VBA macros on round-trip; never destroy a macro-enabled workbook (#6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening an .xlsm and saving it silently stripped xl/vbaProject.bin, turning a macro-enabled workbook into a macro-free one — data loss for anyone whose work depends on macros. GridCraft still does not run VBA, but it now carries the project through unchanged. - model: `Workbook.vba_project: Option>` (base64 in JSON via a new `b64::opt` helper). `None` = no macros. - xlsx read: capture the `vbaProject` relationship's bytes instead of warning and dropping them; warn that macros are kept but not executed. - xlsx write: when present, emit `xl/vbaProject.bin` byte-identical, add the `.../relationships/vbaProject` relationship, switch the main part to the macro-enabled content type (`...sheet.macroEnabled.main+xml`) and add the `application/vnd.ms-office.vbaProject` override. - engine: `view.macros` lists the preserved project alongside GridCraft's own scripts (`{"kind":"vba","executed":false}`), so callers can see it is there. - ROADMAP: scope now "preserve, don't execute" (was "out of scope"). Tests: round-trip byte-identical blob + correct content types/relationship; capture from a foreign-produced .xlsm; a macro-free workbook stays clean. --- ROADMAP.md | 10 +++--- crates/engine/src/cmd/extra.rs | 14 ++++++--- crates/model/src/b64.rs | 20 ++++++++++++ crates/model/src/workbook.rs | 6 ++++ crates/xlsx/src/read.rs | 14 +++++++-- crates/xlsx/src/tests/roundtrip.rs | 50 ++++++++++++++++++++++++++++++ crates/xlsx/src/write.rs | 14 ++++++++- 7 files changed, 117 insertions(+), 11 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 1b0cd34..99c1237 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -44,10 +44,12 @@ without losing data, and not hit a crash in a normal session. The gates: weeks running continuously with 3–4 parallel agents), so **≈ 215 hours in total from today**. Breakdown under "What's left" below. -Scope decisions that affect the count: VBA is out of scope (GridCraft has its own recorded and -editable scripts instead), and features that need Microsoft cloud services (Copilot, live Stocks/ -Geography data types, co-authoring through OneDrive, Smart Lookup, Translate) will be replaced -by open equivalents or left out, not cloned. +Scope decisions that affect the count: VBA macro projects in opened workbooks are **preserved** +(GridCraft carries `xl/vbaProject.bin` through unchanged so saving a macro-enabled workbook never +destroys its macros) but **not executed** — GridCraft's own recorded and editable scripts are the +supported automation path. A VBA interpreter is not planned. Features that need Microsoft cloud +services (Copilot, live Stocks/Geography data types, co-authoring through OneDrive, Smart Lookup, +Translate) will be replaced by open equivalents or left out, not cloned. ### What's left diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs index 5ebc8b8..b690866 100644 --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -786,10 +786,16 @@ fn new_script(s: &mut Session, p: &Json) -> Result { } fn all_scripts(s: &mut Session, _: &Json) -> Result { - let all = stash_get(&s.doc()?.wb, SCRIPTS_NAME); - Ok(Json::Array( - all.as_object().map(|o| o.iter().map(|(k, v)| json!({"name": k, "steps": v.as_array().map_or(0, Vec::len)})).collect()).unwrap_or_default(), - )) + let wb = &s.doc()?.wb; + let all = stash_get(wb, SCRIPTS_NAME); + let mut list: Vec = + all.as_object().map(|o| o.iter().map(|(k, v)| json!({"name": k, "steps": v.as_array().map_or(0, Vec::len)})).collect()).unwrap_or_default(); + // A preserved VBA project is reported alongside GridCraft's own scripts; it is carried on + // save but never executed here, so it has no step count. + if let Some(bytes) = &wb.vba_project { + list.push(json!({"name": "(VBA)", "steps": 0, "kind": "vba", "size": bytes.len(), "executed": false})); + } + Ok(Json::Array(list)) } fn run_script(s: &mut Session, p: &Json) -> Result { diff --git a/crates/model/src/b64.rs b/crates/model/src/b64.rs index 6354952..30db4b4 100644 --- a/crates/model/src/b64.rs +++ b/crates/model/src/b64.rs @@ -49,6 +49,26 @@ pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result, D::Error> decode(&s).ok_or_else(|| serde::de::Error::custom("invalid base64")) } +/// Base64 for optional byte blobs (VBA projects): `null`/missing stays `None`. +pub mod opt { + use serde::{Deserialize, Deserializer, Serializer}; + + pub fn serialize(v: &Option>, s: S) -> Result { + match v { + Some(data) => s.serialize_str(&super::encode(data)), + None => s.serialize_none(), + } + } + + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result>, D::Error> { + let s = Option::::deserialize(d)?; + match s { + None => Ok(None), + Some(s) => super::decode(&s).map(Some).ok_or_else(|| serde::de::Error::custom("invalid base64")), + } + } +} + #[cfg(test)] mod tests { #[test] diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 8652b20..ea404ba 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -69,6 +69,11 @@ pub struct Workbook { pub custom_lists: Vec>, /// Named cell styles (Cell Styles gallery): name → style. pub cell_styles: Vec<(String, crate::style::Style)>, + /// VBA project (`xl/vbaProject.bin`) carried through unchanged. GridCraft does not run + /// macros; the blob is preserved so that saving a macro-enabled workbook never destroys the + /// macros it arrived with. base64 in JSON. `None` = the workbook has no macros. + #[serde(default, with = "crate::b64::opt")] + pub vba_project: Option>, } impl Default for Workbook { @@ -92,6 +97,7 @@ impl Workbook { protected_structure: false, custom_lists: vec![], cell_styles: vec![], + vba_project: None, } } pub fn sheet(&self, i: usize) -> Option<&Sheet> { diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 915d8e4..017658c 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -273,8 +273,18 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { wb.props.company = x.child("Company").map(|e| e.text.trim().to_string()).unwrap_or_default(); } - if wb_rels.iter().any(|r| r.kind == "vbaProject") { - cx.warn("macros (VBA project) are not supported and were dropped"); + // A VBA project is carried through unchanged: GridCraft never runs macros, but preserving the + // binary keeps macro-enabled workbooks from being silently stripped on save. + if let Some(r) = find_rel(&wb_rels, "vbaProject").cloned() { + match cx.pkg.read(&r.target) { + Ok(Some(data)) => { + wb.vba_project = Some(data); + cx.warn("macros (VBA project) were kept but not executed"); + } + Ok(None) => cx.warn("macros (VBA project) reference a missing part and were dropped"), + Err(IoError::TooLarge(m)) => return Err(IoError::TooLarge(m)), + Err(e) => cx.warn(format!("macros (VBA project) could not be read and were dropped: {e}")), + } } if wb_rels.iter().any(|r| r.kind == "externalLink") { cx.warn("links to external workbooks are not supported"); diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index bfc83b9..b747cbb 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -563,3 +563,53 @@ fn writing_repairs_invalid_models() { assert_eq!(s.tables[0].name, "Table1"); assert_eq!(s.cell(at("B2")).unwrap().style, StyleId::DEFAULT); } + +#[test] +fn vba_project_roundtrip() { + // A VBA project blob is preserved byte-identically through write and read, and the package + // carries the macro-enabled content type plus the vbaProject relationship. + let vba = b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec(); + let mut wb = Workbook::new(); + wb.vba_project = Some(vba.clone()); + let bytes = write_xlsx(&wb).unwrap(); + assert_eq!(write_xlsx(&wb).unwrap(), bytes, "output is deterministic"); + + let mut z = zip::ZipArchive::new(std::io::Cursor::new(bytes.clone())).unwrap(); + let names: Vec = (0..z.len()).map(|i| z.name_for_index(i).unwrap().to_string()).collect(); + assert!(names.iter().any(|x| x == "xl/vbaProject.bin"), "missing vbaProject.bin"); + let mut bin = vec![]; + z.by_name("xl/vbaProject.bin").unwrap().read_to_end(&mut bin).unwrap(); + assert_eq!(bin, vba, "vbaProject.bin written byte-identically"); + let mut ct = String::new(); + z.by_name("[Content_Types].xml").unwrap().read_to_string(&mut ct).unwrap(); + assert!(ct.contains("macroEnabled.main+xml"), "macro-enabled main content type: {ct}"); + assert!(ct.contains("/xl/vbaProject.bin"), "vbaProject content type override: {ct}"); + let mut rels = String::new(); + z.by_name("xl/_rels/workbook.xml.rels").unwrap().read_to_string(&mut rels).unwrap(); + assert!(rels.contains("relationships/vbaProject"), "vbaProject relationship: {rels}"); + + let (back, rep) = read_xlsx(&bytes).unwrap(); + assert_eq!(back.vba_project.as_deref(), Some(vba.as_slice()), "vbaProject survives the read"); + assert!(rep.warnings.iter().any(|w| w.contains("not executed")), "{:?}", rep.warnings); + + // A workbook without macros stays macro-free and free of VBA warnings. + let (plain, rp) = read_xlsx(&write_xlsx(&Workbook::new()).unwrap()).unwrap(); + assert_eq!(plain.vba_project, None); + assert!(rp.warnings.iter().all(|w| !w.contains("VBA")), "{:?}", rp.warnings); +} + +#[test] +fn vba_project_read_from_foreign_file() { + // An .xlsm built by another producer is captured on read (not dropped with a plain warning). + let vba: &[u8] = b"PK\x03\x04-not-really-but-opaque-bytes"; + let bytes = super::minimal( + "", + &[("xl/vbaProject.bin", "ignored-as-text")], + r#""#, + "", + ); + let (wb, rep) = read_xlsx(&bytes).unwrap(); + assert!(wb.vba_project.is_some(), "VBA blob captured"); + assert!(rep.warnings.iter().any(|w| w.contains("not executed")), "{:?}", rep.warnings); + let _ = vba; +} diff --git a/crates/xlsx/src/write.rs b/crates/xlsx/src/write.rs index 073dd36..c20c554 100644 --- a/crates/xlsx/src/write.rs +++ b/crates/xlsx/src/write.rs @@ -20,6 +20,11 @@ pub const CT_DRAWING: &str = "application/vnd.openxmlformats-officedocument.draw pub const CT_CHART: &str = "application/vnd.openxmlformats-officedocument.drawingml.chart+xml"; pub const CT_TABLE: &str = "application/vnd.openxmlformats-officedocument.spreadsheetml.table+xml"; pub const CT_COMMENTS: &str = "application/vnd.openxmlformats-officedocument.spreadsheetml.comments+xml"; +/// Main workbook part: macro-free and macro-enabled variants (Excel refuses a `.xlsm` whose main +/// part uses the macro-free content type, and vice versa). +pub const CT_WORKBOOK_MAIN: &str = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.main+xml"; +pub const CT_WORKBOOK_MAIN_MACRO: &str = "application/vnd.ms-excel.sheet.macroEnabled.main+xml"; +pub const CT_VBA: &str = "application/vnd.ms-office.vbaProject"; /// A part's relationships. #[derive(Default)] @@ -33,6 +38,9 @@ impl Rels { let id = format!("rId{}", self.items.len() + 1); let ty = match kind { "core-properties" => "http://schemas.openxmlformats.org/package/2006/relationships/metadata/core-properties".to_string(), + // The VBA project relationship lives in the Microsoft-documented namespace, not the + // OpenXML one; Excel rejects the file if this type is wrong. + "vbaProject" => "http://schemas.microsoft.com/office/2006/relationships/vbaProject".to_string(), k => format!("{NS_REL}/{k}"), }; self.items.push((id.clone(), ty, target.to_string(), false)); @@ -273,7 +281,11 @@ pub fn write_xlsx(wb: &Workbook) -> Result, IoError> { ); wb_rels.add("sheetMetadata", "metadata.xml"); } - out.part("xl/workbook.xml", Some("application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.main+xml"), w.into_bytes()); + out.part("xl/workbook.xml", Some(if wb.vba_project.is_some() { CT_WORKBOOK_MAIN_MACRO } else { CT_WORKBOOK_MAIN }), w.into_bytes()); + if let Some(data) = &wb.vba_project { + wb_rels.add("vbaProject", "vbaProject.bin"); + out.part("xl/vbaProject.bin", Some(CT_VBA), data.clone()); + } out.part("xl/_rels/workbook.xml.rels", None, wb_rels.xml().into_bytes()); // Document properties. From 9c1d2c3b3a94a5c0a903fda3f00d8e76cae83ae8 Mon Sep 17 00:00:00 2001 From: justinjohn0306 <34035011+justinjohn0306@users.noreply.github.com> Date: Fri, 9 Oct 2026 01:58:29 +0530 Subject: [PATCH 04/27] Add scripts/check-vba-roundtrip.sh: verify VBA preservation on a real file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds a macro-enabled .xlsm with a fake xl/vbaProject.bin, runs it through `gridcraft-cli run`, and asserts the blob comes back byte-identical with the macro-enabled content type and vbaProject relationship intact — the open → edit → save path a user would exercise by hand. --- scripts/check-vba-roundtrip.sh | 86 ++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 scripts/check-vba-roundtrip.sh diff --git a/scripts/check-vba-roundtrip.sh b/scripts/check-vba-roundtrip.sh new file mode 100644 index 0000000..5aef3a5 --- /dev/null +++ b/scripts/check-vba-roundtrip.sh @@ -0,0 +1,86 @@ +#!/usr/bin/env bash +# Verify that GridCraft preserves VBA macros (xl/vbaProject.bin) across a round-trip. +# +# bash scripts/check-vba-roundtrip.sh +# +# What it does: builds a macro-enabled .xlsm containing a fake vbaProject.bin, runs it +# through `gridcraft-cli run`, and checks the blob comes back byte-identical with the +# macro-enabled content type + relationship intact. Requires python3 (zip) and a +# release build of gridcraft-cli. +set -euo pipefail + +ROOT="$(cd "$(dirname "$0")/.." && pwd)" +CLI="$ROOT/target/release/gridcraft-cli.exe" +[ -x "$CLI" ] || CLI="$ROOT/target/release/gridcraft-cli" +PYTHON="${PYTHON:-python3}" + +# A fresh temp dir. Under Git Bash (MSYS) the shell's /tmp is not a path the native +# Windows python.exe or the engine understand when embedded in JSON, so convert to the +# platform-native form when cygpath is available. +TMP="$(mktemp -d)" +if command -v cygpath >/dev/null 2>&1; then + NATIVE_TMP="$(cygpath -w "$TMP")" # for python (native Windows path) + JSON_TMP="$(cygpath -m "$TMP")" # for JSON params (forward slashes: no escaping) +else + NATIVE_TMP="$TMP" + JSON_TMP="$TMP" +fi + +# 1. Build a macro-enabled .xlsm with a fake OLE/CFB vbaProject.bin. +"$PYTHON" - "$NATIVE_TMP" <<'PY' +import zipfile, os, sys +tmp = sys.argv[1] +main_ct = "application/vnd.ms-excel.sheet.macroEnabled.main+xml" +ct = ('' + '' + '' + f'' + '' + '') +root = ('' + '') +wb = ('' + '') +rel = ('' + '' + '') +sheet = '' +vba = b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1" + b"HELLO-VBA-PROJECT-BYTES" + bytes(range(32)) +with zipfile.ZipFile(os.path.join(tmp, "macro_demo.xlsm"), "w", zipfile.ZIP_DEFLATED) as z: + z.writestr("[Content_Types].xml", ct) + z.writestr("_rels/.rels", root) + z.writestr("xl/workbook.xml", wb) + z.writestr("xl/_rels/workbook.xml.rels", rel) + z.writestr("xl/worksheets/sheet1.xml", sheet) + z.writestr("xl/vbaProject.bin", vba) +open(os.path.join(tmp, "vba_expected.bin"), "wb").write(vba) +print("built", os.path.join(tmp, "macro_demo.xlsm")) +PY + +# 2. Round-trip through the engine. +echo "--- view.macros on the input (what the app sees) ---" +"$CLI" run --in "$NATIVE_TMP/macro_demo.xlsm" --cmd 'view.macros={}' | tail -1 +"$CLI" run --in "$NATIVE_TMP/macro_demo.xlsm" --cmd "file.save={\"path\":\"$JSON_TMP/macro_out.xlsm\"}" | tail -1 + +# 3. Assert the blob survived byte-identical with correct package wiring. +"$PYTHON" - "$NATIVE_TMP" <<'PY' +import zipfile, os, sys +tmp = sys.argv[1] +exp = open(os.path.join(tmp, "vba_expected.bin"), "rb").read() +with zipfile.ZipFile(os.path.join(tmp, "macro_out.xlsm")) as z: + got = z.read("xl/vbaProject.bin") + ct = z.read("[Content_Types].xml").decode() + rels = z.read("xl/_rels/workbook.xml.rels").decode() +ok = (got == exp + and "macroEnabled.main+xml" in ct + and "/xl/vbaProject.bin" in ct + and "relationships/vbaProject" in rels) +print("blob byte-identical:", got == exp) +print("macro-enabled main content type:", "macroEnabled.main+xml" in ct) +print("vbaProject content-type override:", "/xl/vbaProject.bin" in ct) +print("vbaProject relationship:", "relationships/vbaProject" in rels) +sys.exit(0 if ok else 1) +PY + +echo "PASS: the VBA project survived the round-trip." \ No newline at end of file From 596b465bb1b149e2bf7603565e6e40b122a230ea Mon Sep 17 00:00:00 2001 From: justinjohn0306 <34035011+justinjohn0306@users.noreply.github.com> Date: Fri, 9 Oct 2026 05:20:04 +0530 Subject: [PATCH 05/27] Macros UX: Excel-style info bar, Macros dialog, status badge (issue #6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine now preserves a workbook's VBA project, but the UI had no surface for it and silently dropped every open-time warning. Give preserved macros an honest, Excel-shaped home without a false "Enable Content": - Info bar under the ribbon (Excel's message-bar slot) showing the warning a `file.open` result carries. A VBA project gets an amber bar reading "This workbook contains VBA macros. GridCraft keeps them intact when you save, but does not run them." with a "What this means" button; anything else becomes a blue informational bar. Dismissable; a clean open clears a stale notice. - Macros dialog (Automate ▸ Macros, Alt+F8): lists GridCraft's own scripts and the preserved VBA project (presence + size, not module names — the blob is opaque). Mirrors Excel's Developer ▸ Macros without claiming to run VBA. - Status-bar badge "Macros preserved (not executed)" whenever the workbook carries a VBA project. New panel `notice_bar` reuses theme tokens (notice_warn_*/notice_info_*), so it follows light/dark. Two new UI commands, `ui.notice` and `ui.notice.dismiss`, reach it from the control channel. Tests: info bar appears for a macro workbook and clears on dismiss; a non-VBA warning becomes an info bar; clean open clears the notice; the macros dialog lists the VBA entry; status bar renders with a VBA project; Alt+F8 opens the dialog. Snapshot-verified in light and dark. --- crates/ui-egui/src/dialogs.rs | 36 +++++++++++- crates/ui-egui/src/grid.rs | 3 + crates/ui-egui/src/lib.rs | 72 +++++++++++++++++++++++- crates/ui-egui/src/ribbon.rs | 3 + crates/ui-egui/src/tabs.rs | 4 ++ crates/ui-egui/src/theme.rs | 13 +++++ crates/ui-egui/src/widgets.rs | 48 ++++++++++++++++ crates/ui-egui/tests/interaction.rs | 85 +++++++++++++++++++++++++++++ 8 files changed, 260 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index b6c186d..9fa6d15 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -512,6 +512,11 @@ impl Dialog { d } "journal" => Dialog::custom("journal", "Action Journal", json!({})), + "macros" => { + let mut d = Dialog::custom("macros", "Macros", json!({})); + d.result = app.session.run("view.macros", json!({})).ok(); + d + } "statistics" => { let mut d = Dialog::custom("statistics", "Workbook Statistics", json!({})); d.result = app.session.run("review.workbookStatistics", json!({})).ok(); @@ -638,7 +643,7 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { let width = match d.name.as_str() { "formatCells" => 560.0, "about" => 660.0, - "insertFunction" | "commandSearch" | "nameManager" | "manageRules" | "journal" | "agents" => 520.0, + "insertFunction" | "commandSearch" | "nameManager" | "manageRules" | "journal" | "agents" | "macros" => 520.0, _ => 380.0, }; egui::Window::new(d.title.clone()) @@ -676,6 +681,7 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { } }); } + "macros" => macros(ui, &mut d), "statistics" | "accessibility" | "errorChecking" | "evaluateFormula" | "comments" => { let r = if d.name == "comments" { app.session.active().and_then(|doc| doc.wb.active().map(|s| json!(s.comments.iter().map(|(c, m)| json!({"cell": c.a1(), "author": m.author, "text": m.text})).collect::>()))) @@ -925,6 +931,34 @@ fn about(ui: &mut egui::Ui, d: &mut Dialog) { } } +/// The Macros dialog: lists GridCraft's own recorded scripts and (if the file carries one) the +/// preserved VBA project. Mirrors Excel's Developer ▸ Macros list, but is honest that VBA is +/// preserved and not executed. +fn macros(ui: &mut egui::Ui, d: &mut Dialog) { + ui.label("Automation in this workbook. GridCraft runs its own scripts; a file's VBA project is preserved but not executed."); + ui.add_space(6.0); + let items: Vec = d.result.as_ref().and_then(Json::as_array).cloned().unwrap_or_default(); + if items.is_empty() { + ui.weak("This workbook has no scripts or macros."); + return; + } + egui::ScrollArea::vertical().max_height(300.0).show(ui, |ui| { + for it in &items { + let name = it.get("name").and_then(Json::as_str).unwrap_or("(unnamed)"); + let is_vba = it.get("kind").and_then(Json::as_str) == Some("vba"); + ui.horizontal(|ui| { + ui.label(egui::RichText::new(name).strong()); + if is_vba { + ui.weak("— VBA project (preserved, not run)"); + } else { + let steps = it.get("steps").and_then(Json::as_u64).unwrap_or(0); + ui.weak(format!("— {steps} step{}", if steps == 1 { "" } else { "s" })); + } + }); + } + }); +} + fn format_cells(app: &mut SheetApp, ui: &mut egui::Ui, d: &mut Dialog, confirm: &mut bool) { ui.horizontal(|ui| { for tab in ["Number", "Alignment", "Font", "Border", "Fill", "Protection"] { diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4faf43a..5683f64 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -1603,6 +1603,9 @@ fn keyboard(app: &mut SheetApp, ctx: &egui::Context, resp: &egui::Response, geo: Key::Space if !cmd && !shift && is_checkbox(sh, app) => app.run_or_alert("cell.toggleCheckbox", json!({})), Key::Space if cmd && !shift => app.run_or_alert("selection.column", json!({})), Key::Space if shift && !cmd => app.run_or_alert("selection.row", json!({})), + Key::F8 if m.alt => { + app.open_dialog("macros", json!({})); + } Key::F9 => app.run_or_alert("formulas.calculateNow", json!({})), Key::F4 if !cmd => {} _ if cmd => crate::ribbon::shortcut(app, key, m), diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 8eb1eca..f907aa2 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -63,6 +63,23 @@ pub struct SheetView { pub scroll: egui::Vec2, } +/// Severity of a notice shown in the info bar under the ribbon. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum NoticeKind { + Info, + Warning, +} + +/// A dismissible bar under the ribbon, in the spirit of Excel's message bar. Used to tell the +/// user what an opened file carried that GridCraft handled differently (e.g. preserved macros). +#[derive(Clone, Debug, PartialEq)] +pub struct Notice { + pub text: String, + pub kind: NoticeKind, + /// Optional button: `(label, dialog name)` — opens that dialog when clicked. + pub action: Option<(String, String)>, +} + /// Host services the platform shell provides (file pickers, opening URLs). #[derive(Default)] pub struct Services { @@ -96,6 +113,8 @@ pub struct SheetApp { pub dialog: Option, pub message: Option<(String, String)>, pub toast: Option<(String, f64)>, + /// Dismissible info bar under the ribbon (open-time notices). + pub notice: Option, pub services: Services, /// Synthetic input from agents, one batch per frame (drags need several frames). pub synthetic: std::collections::VecDeque>, @@ -121,6 +140,7 @@ impl SheetApp { dialog: None, message: None, toast: None, + notice: None, services, synthetic: Default::default(), control_rx: None, @@ -196,6 +216,18 @@ impl SheetApp { self.message = None; Ok(Json::Null) } + "ui.notice" => Ok(match &self.notice { + Some(n) => json!({ + "text": n.text, + "kind": match n.kind { NoticeKind::Warning => "warning", NoticeKind::Info => "info" }, + "action": n.action, + }), + None => Json::Null, + }), + "ui.notice.dismiss" => { + self.notice = None; + Ok(Json::Null) + } _ => return None, }; Some(r) @@ -249,16 +281,48 @@ impl SheetApp { pub fn open_path(&mut self, path: &str) { match self.session.run("file.open", json!({"path": path})) { - Ok(_) => { + Ok(r) => { self.ui.recent.retain(|p| p != path); self.ui.recent.insert(0, path.to_string()); self.ui.recent.truncate(20); + self.show_open_warnings(&r); } Err(e) => self.message = Some(("GridCraft".into(), clean_error(&e))), } self.after_engine(); } + /// Turns the `warnings` a `file.open` result carries into an info-bar notice, choosing the + /// most important one (a preserved VBA project outranks, say, a skipped optional part). + /// Kept deliberately generic: any open-time warning can reach the user this way. + pub fn show_open_warnings(&mut self, r: &Json) { + let Some(list) = r.get("warnings").and_then(Json::as_array) else { + self.notice = None; + return; + }; + let msgs: Vec<&str> = list.iter().filter_map(Json::as_str).collect(); + if msgs.is_empty() { + // A clean open clears any notice left over from the previous document. + self.notice = None; + return; + } + let vba: Vec<&str> = msgs.iter().copied().filter(|m| m.to_lowercase().contains("macro") || m.contains("VBA")).collect(); + if !vba.is_empty() { + self.notice = Some(Notice { + text: "This workbook contains VBA macros. GridCraft keeps them intact when you save, but does not run them.".into(), + kind: NoticeKind::Warning, + action: Some(("What this means".into(), "macros".into())), + }); + } else { + let text = if msgs.len() == 1 { + msgs[0].to_string() + } else { + format!("{} items in this file were handled differently. First: {}", msgs.len(), msgs[0]) + }; + self.notice = Some(Notice { text, kind: NoticeKind::Info, action: None }); + } + } + pub fn view_key(&self) -> Option<(u64, usize)> { self.session.active().map(|d| (d.uid, d.wb.active_sheet)) } @@ -375,8 +439,9 @@ impl SheetApp { .unwrap_or_default(); for (name, bytes) in arrived { let b64 = gridcraft_engine::io::base64_encode(&bytes); - if let Err(e) = self.session.run("file.open", json!({"name": name, "base64": b64})) { - self.message = Some(("GridCraft".into(), clean_error(&e))); + match self.session.run("file.open", json!({"name": name, "base64": b64})) { + Ok(r) => self.show_open_warnings(&r), + Err(e) => self.message = Some(("GridCraft".into(), clean_error(&e))), } self.after_engine(); } @@ -402,6 +467,7 @@ impl SheetApp { let t = theme::Tokens::get(&ctx); ribbon::title_bar(self, ui); ribbon::show(self, ui); + widgets::notice_bar(self, ui); if self.ui.formula_bar { formula_bar::show(self, ui); } diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 47c5432..d4b0a0b 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -1574,6 +1574,9 @@ fn view(app: &mut SheetApp, ui: &mut Ui) { } fn automate(app: &mut SheetApp, ui: &mut Ui) { + if big_button(ui, Icon::Script, "Macros", "Macros and scripts in this workbook (Alt+F8)", false).clicked() { + app.open_dialog("macros", json!({})); + } if big_button(ui, Icon::Script, "Command\nPalette", "Search and run any command", false).clicked() { app.open_dialog("commandSearch", json!({})); } diff --git a/crates/ui-egui/src/tabs.rs b/crates/ui-egui/src/tabs.rs index 0258169..df1e3f1 100644 --- a/crates/ui-egui/src/tabs.rs +++ b/crates/ui-egui/src/tabs.rs @@ -192,6 +192,10 @@ pub fn status_bar(app: &mut SheetApp, ui: &mut Ui) { }; ui.label(egui::RichText::new(mode).font(theme::ui_font(12.5)).color(t.text_dim)); if let Some(d) = app.session.active() { + if d.wb.vba_project.is_some() { + ui.add_space(12.0); + ui.label(egui::RichText::new("Macros preserved (not executed)").font(theme::ui_font(12.5)).color(t.danger)); + } if d.wb.calc.mode == gridcraft_engine::model::CalcMode::Manual { ui.add_space(12.0); ui.label(egui::RichText::new("Calculate").font(theme::ui_font(12.5)).color(t.text_dim)); diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 2dbd8df..36c2621 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -43,6 +43,11 @@ pub struct Tokens { pub menu_bg: Color32, pub shadow: Color32, pub danger: Color32, + /// Info-bar (notice strip) fills, by severity. + pub notice_warn_bg: Color32, + pub notice_warn_border: Color32, + pub notice_info_bg: Color32, + pub notice_info_border: Color32, } impl Tokens { @@ -80,6 +85,10 @@ impl Tokens { menu_bg: Color32::WHITE, shadow: Color32::from_black_alpha(40), danger: Color32::from_rgb(0xC4, 0x2B, 0x1C), + notice_warn_bg: Color32::from_rgb(0xFF, 0xF4, 0xCE), + notice_warn_border: Color32::from_rgb(0xF0, 0xC3, 0x6E), + notice_info_bg: Color32::from_rgb(0xEA, 0xF2, 0xFB), + notice_info_border: Color32::from_rgb(0xB9, 0xD2, 0xEC), } } @@ -117,6 +126,10 @@ impl Tokens { menu_bg: Color32::from_rgb(0x30, 0x30, 0x30), shadow: Color32::from_black_alpha(90), danger: Color32::from_rgb(0xF1, 0x70, 0x5F), + notice_warn_bg: Color32::from_rgb(0x4A, 0x3F, 0x1E), + notice_warn_border: Color32::from_rgb(0x7A, 0x67, 0x30), + notice_info_bg: Color32::from_rgb(0x24, 0x33, 0x44), + notice_info_border: Color32::from_rgb(0x3B, 0x55, 0x72), } } diff --git a/crates/ui-egui/src/widgets.rs b/crates/ui-egui/src/widgets.rs index 139256f..503288c 100644 --- a/crates/ui-egui/src/widgets.rs +++ b/crates/ui-egui/src/widgets.rs @@ -198,3 +198,51 @@ pub fn toast(app: &mut SheetApp, ctx: &egui::Context) { ); ctx.request_repaint_after(std::time::Duration::from_millis(200)); } + +/// The info bar under the ribbon (Excel's message-bar slot). Shows an open-time notice with an +/// optional action button and a dismiss ✕. Returns nothing; dismissing clears `app.notice`. +pub fn notice_bar(app: &mut SheetApp, ui: &mut Ui) { + let Some(notice) = app.notice.clone() else { return }; + let t = Tokens::get(ui.ctx()); + let (bg, border) = match notice.kind { + crate::NoticeKind::Warning => (t.notice_warn_bg, t.notice_warn_border), + crate::NoticeKind::Info => (t.notice_info_bg, t.notice_info_border), + }; + let mut dismiss = false; + let mut action: Option<(String, String)> = None; + egui::Panel::top("notice_bar").frame(egui::Frame::NONE.fill(bg).inner_margin(egui::Margin::symmetric(12, 0))).show(ui, |ui| { + // Bottom border as a 1px strip spanning the bar. + let bar = ui.max_rect(); + ui.painter().rect_stroke(bar, 0.0, Stroke::new(1.0, border), StrokeKind::Inside); + ui.horizontal_centered(|ui| { + let (r, _) = ui.allocate_exact_size(vec2(16.0, 16.0), Sense::hover()); + icons::paint( + ui.painter(), + r, + match notice.kind { + crate::NoticeKind::Warning => Icon::Lock, + crate::NoticeKind::Info => Icon::Note, + }, + border, + ); + ui.add_space(6.0); + ui.label(egui::RichText::new(¬ice.text).font(theme::ui_font(12.5)).color(t.text)); + ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { + if icon_button(ui, Icon::Close, t.text_dim, "Dismiss", vec2(20.0, 20.0)).clicked() { + dismiss = true; + } + if let Some((label, dialog)) = ¬ice.action + && ui.button(egui::RichText::new(label).font(theme::ui_font(12.5))).clicked() + { + action = Some((label.clone(), dialog.clone())); + } + }); + }); + }); + if let Some((_, dialog)) = action { + app.open_dialog(&dialog, serde_json::json!({})); + } + if dismiss { + app.notice = None; + } +} diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index 74bbeb4..302a2f9 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -113,6 +113,7 @@ fn dialogs_open_and_close() { "spelling", "goTo", "about", + "macros", ] { h.state_mut().open_dialog(name, json!({})); h.run_steps(2); @@ -146,3 +147,87 @@ fn column_autocomplete_completes_on_enter() { key(&mut h, Key::Enter, Modifiers::NONE); assert_eq!(value(&h, "A5"), Value::from("Ea")); } + +/// An .xlsm carrying a VBA project, saved to bytes. +fn macro_workbook_bytes() -> Vec { + let mut wb = gridcraft_engine::model::Workbook::new(); + wb.vba_project = Some(b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec()); + gridcraft_engine::io::save_bytes(&wb, "macros.xlsm").unwrap() +} + +#[test] +fn opening_a_macro_workbook_shows_a_notice() { + // The reader's "macros kept but not executed" warning must reach a dismissible info bar, + // not be silently dropped (issue #6 follow-up: Excel-style message bar). + let mut h = harness(blank()); + let b64 = gridcraft_engine::io::base64_encode(¯o_workbook_bytes()); + let res = h.state_mut().session.run("file.open", json!({"name": "macros.xlsm", "base64": b64})).unwrap(); + assert!( + res.get("warnings").and_then(|w| w.as_array()).is_some_and(|a| a.iter().any(|m| m.as_str().is_some_and(|s| s.contains("macros")))), + "engine reports the macro warning: {res}" + ); + // The UI-side handler turns that open result into the notice (open_path does this in the app). + h.state_mut().show_open_warnings(&res); + let n = h.state().notice.clone().expect("a notice was shown"); + assert!(n.text.contains("VBA"), "{}", n.text); + assert_eq!(n.kind, gridcraft_ui_egui::NoticeKind::Warning); + assert_eq!(n.action.as_ref().map(|a| a.1.as_str()), Some("macros"), "action opens the Macros dialog"); + + // The notice bar renders without panic; `ui.notice` surfaces it; dismissing clears it. + h.run_steps(2); + let got = h.state_mut().run("ui.notice", json!({})).unwrap(); + assert_eq!(got.get("kind").and_then(|k| k.as_str()), Some("warning")); + h.state_mut().run("ui.notice.dismiss", json!({})).unwrap(); + h.run_steps(2); + assert!(h.state().notice.is_none()); +} + +#[test] +fn plain_open_warning_becomes_an_info_notice() { + let mut h = harness(blank()); + h.state_mut().show_open_warnings(&json!({"warnings": ["links to external workbooks are not supported"]})); + let n = h.state().notice.clone().unwrap(); + assert_eq!(n.kind, gridcraft_ui_egui::NoticeKind::Info); + assert!(n.action.is_none()); + assert!(n.text.contains("external")); + // No warnings → no notice, no panic. + h.state_mut().show_open_warnings(&json!({"warnings": []})); + assert!(h.state().notice.is_none()); +} + +#[test] +fn macros_dialog_lists_vba_and_scripts() { + let mut s = blank(); + let mut wb = gridcraft_engine::model::Workbook::new(); + wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]); + s.active_mut().unwrap().wb = std::sync::Arc::new(wb); + let mut h = harness(s); + h.state_mut().open_dialog("macros", json!({})); + h.run_steps(3); + let d = h.state().dialog.as_ref().expect("macros dialog open"); + assert_eq!(d.name(), "macros"); + let items = d.result.as_ref().and_then(|r| r.as_array()).cloned().unwrap_or_default(); + assert!(items.iter().any(|i| i.get("kind").and_then(|k| k.as_str()) == Some("vba")), "{items:?}"); + h.state_mut().dialog = None; +} + +#[test] +fn status_bar_shows_macro_badge() { + let mut s = blank(); + let mut wb = gridcraft_engine::model::Workbook::new(); + wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]); + s.active_mut().unwrap().wb = std::sync::Arc::new(wb); + // Rendering the status bar must not panic with a VBA project present. + let mut h = harness(s); + h.run_steps(3); + h.state_mut().ui.dark = true; + h.run_steps(2); +} + +#[test] +fn alt_f8_opens_the_macros_dialog() { + let mut h = harness(blank()); + key(&mut h, Key::F8, Modifiers::ALT); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name().to_string()).as_deref(), Some("macros")); + h.state_mut().dialog = None; +} From fd2243df95ce2d967e90deefbf10f6dd32b0a82a Mon Sep 17 00:00:00 2001 From: Zaid Shurbaji Date: Sat, 10 Oct 2026 21:27:13 +0300 Subject: [PATCH 06/27] Add table slicers (insert.slicer / table.insertSlicer) Implements the two slicer commands from the parity catalog as first-class floating objects that filter a table column. - model: Slicer struct on Sheet (#[serde(default)]), included in Workbook::next_object_id; slicer anchors use AnchorMode::MoveOnly. - engine: cmd/slicer.rs with insert.slicer, table.insertSlicer, slicer.toggle (single + multi), slicer.selectAll, slicer.clear, slicer.delete. Selection is written as a FilterCriterion::Values into the sheet AutoFilter and applied via the existing apply_filter. object.move / object.delete handle "slicer". - ui-egui: slicers rendered, hit-tested, selected and dragged like charts; value tiles toggle on click (Ctrl/Cmd = multi), funnel button clears. Ribbon buttons under Insert > Filters and Table Design > Tools. - tests: slicer_filters_table; the every_command_survives_empty_params fuzz covers the new commands. Native-only for now; .xlsx slicer-cache round-trip is a follow-up. Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/cmd/insert.rs | 2 + crates/engine/src/cmd/mod.rs | 2 + crates/engine/src/cmd/slicer.rs | 229 ++++++++++++++++++++++++++++++++ crates/engine/src/tests.rs | 53 ++++++++ crates/model/src/features.rs | 20 +++ crates/model/src/sheet.rs | 4 + crates/model/src/workbook.rs | 3 + crates/ui-egui/src/chartview.rs | 112 ++++++++++++++++ crates/ui-egui/src/grid.rs | 21 ++- crates/ui-egui/src/ribbon.rs | 8 ++ 10 files changed, 453 insertions(+), 1 deletion(-) create mode 100644 crates/engine/src/cmd/slicer.rs diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 8e08ea6..6f9d369 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -869,6 +869,7 @@ fn delete_object(s: &mut Session, p: &Json) -> Result { match kind.as_str() { "chart" => sh.charts.retain(|c| c.id != id), "image" => sh.images.retain(|c| c.id != id), + "slicer" => sh.slicers.retain(|c| c.id != id), _ => sh.shapes.retain(|c| c.id != id), } Ok(Json::Null) @@ -884,6 +885,7 @@ fn move_object(s: &mut Session, p: &Json) -> Result { let anchor = match kind.as_str() { "chart" => sh.charts.iter_mut().find(|c| c.id == id).map(|c| &mut c.anchor), "image" => sh.images.iter_mut().find(|c| c.id == id).map(|c| &mut c.anchor), + "slicer" => sh.slicers.iter_mut().find(|c| c.id == id).map(|c| &mut c.anchor), "shape" => sh.shapes.iter_mut().find(|c| c.id == id).map(|c| &mut c.anchor), _ => None, }; diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index d238ef7..7172bcb 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -14,6 +14,7 @@ pub mod pivot; pub mod print; pub mod review; pub mod sheet; +pub mod slicer; pub mod spelling; pub mod view; pub mod whatif; @@ -119,6 +120,7 @@ pub fn command_specs() -> &'static [CommandSpec] { v.extend(draw::specs()); v.extend(spelling::specs()); v.extend(pivot::specs()); + v.extend(slicer::specs()); v.extend(whatif::specs()); v.extend(print::specs()); v diff --git a/crates/engine/src/cmd/slicer.rs b/crates/engine/src/cmd/slicer.rs new file mode 100644 index 0000000..907ac7e --- /dev/null +++ b/crates/engine/src/cmd/slicer.rs @@ -0,0 +1,229 @@ +//! Slicers: on-sheet widgets that filter one column of a table by clicking value tiles. +//! +//! A slicer is a thin front-end over the existing filter machinery: its selection is written as a +//! [`FilterCriterion::Values`] into the sheet's [`AutoFilter`] (over the table's range) and +//! [`super::data::apply_filter`] hides the rows. Creating a slicer (`insert.slicer` / +//! `table.insertSlicer`) selects everything, i.e. applies no filter. + +use gridcraft_core::CellRef; +use gridcraft_model::*; +use serde_json::{Value as Json, json}; + +use super::*; + +pub fn specs() -> Vec { + vec![ + cmd!( + "insert.slicer", + "Slicer", + ["Insert", "Filters"], + None, + "{table?, column?: \"Region\", columns?: 1, at?: \"H2\", width?, height?}", + has_doc, + insert_slicer + ), + cmd!( + "table.insertSlicer", + "Insert Slicer", + ["Table", "Tools"], + None, + "{table?, column?: \"Region\", columns?: 1, at?, width?, height?}", + has_doc, + insert_slicer + ), + cmd!("slicer.toggle", "Toggle Slicer Value", [], None, "{slicer?: id, value: \"East\", multi?: false}", has_doc, slicer_toggle), + cmd!("slicer.selectAll", "Clear Slicer Filter", [], None, "{slicer?: id}", has_doc, slicer_select_all), + cmd!("slicer.clear", "Clear Slicer Filter", [], None, "{slicer?: id}", has_doc, slicer_select_all), + cmd!("slicer.delete", "Delete Slicer", [], None, "{slicer?: id}", has_doc, slicer_delete), + ] +} + +/// Distinct, non-blank display values of a table column, in first-seen order. Used to populate a +/// slicer's tiles and to detect "everything selected". +pub fn column_values(wb: &Workbook, sheet: usize, table: &str, column: &str) -> Vec { + let Some(sh) = wb.sheet(sheet) else { return vec![] }; + let Some(t) = sh.tables.iter().find(|t| t.name.eq_ignore_ascii_case(table)) else { return vec![] }; + let Some(ci) = t.column_index(column) else { return vec![] }; + let Some(dr) = t.data_range() else { return vec![] }; + let col = dr.start.col + ci as u32; + let mut seen = std::collections::BTreeSet::new(); + let mut out = Vec::new(); + for row in dr.start.row..=dr.end.row { + let text = crate::display::cell_text(wb, sh, CellRef::new(row, col)); + if text.is_empty() { + continue; + } + if seen.insert(text.to_lowercase()) { + out.push(text); + } + } + out +} + +/// A slicer's tiles as `(value, selected)`, for the UI. `selected` is true for every tile when the +/// slicer has no filter (`selected == None`). +pub fn slicer_items(wb: &Workbook, sheet: usize, sl: &Slicer) -> Vec<(String, bool)> { + column_values(wb, sheet, &sl.table, &sl.column) + .into_iter() + .map(|v| { + let on = match &sl.selected { + None => true, + Some(set) => set.iter().any(|x| x.eq_ignore_ascii_case(&v)), + }; + (v, on) + }) + .collect() +} + +/// Writes the slicer's current selection into the sheet filter and re-applies it. +fn reapply_slicer(wb: &mut Workbook, sheet: usize, id: u32) { + let info = wb.sheet(sheet).and_then(|sh| { + let sl = sh.slicers.iter().find(|s| s.id == id)?; + let t = sh.tables.iter().find(|t| t.name.eq_ignore_ascii_case(&sl.table))?; + let ci = t.column_index(&sl.column)?; + Some((t.range, t.range.start.col + ci as u32, sl.selected.clone())) + }); + let Some((table_range, abs_col, selected)) = info else { return }; + { + let Some(sh) = wb.sheet_mut(sheet) else { return }; + let af = sh.autofilter.get_or_insert_with(|| AutoFilter { range: table_range, criteria: vec![] }); + let off = abs_col.saturating_sub(af.range.start.col); + af.criteria.retain(|(o, _)| *o != off); + if let Some(values) = selected { + af.criteria.push((off, FilterCriterion::Values { values, blanks: false })); + } + } + super::data::apply_filter(wb, sheet); +} + +fn insert_slicer(s: &mut Session, p: &Json) -> Result { + let sheet = target_sheet(s, p)?; + let d = s.doc()?; + let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; + let t = match str_param(p, "table") { + Some(name) => sh.tables.iter().find(|t| t.name.eq_ignore_ascii_case(name)), + None => sh.table_at(d.selection.active), + } + .ok_or_else(|| EngineError::Other("Select a cell inside a table first, or pass `table`.".into()))?; + let column = match str_param(p, "column") { + Some(c) => t + .columns + .iter() + .find(|tc| tc.name.eq_ignore_ascii_case(c)) + .map(|tc| tc.name.clone()) + .ok_or_else(|| bad("insert.slicer", format!("table has no column `{c}`")))?, + None => { + // Default to the active cell's column within the table, else the first column. + let idx = d.selection.active.col.checked_sub(t.range.start.col).map(|o| o as usize).filter(|o| *o < t.columns.len()).unwrap_or(0); + t.columns.get(idx).map(|tc| tc.name.clone()).ok_or_else(|| bad("insert.slicer", "the table has no columns"))? + } + }; + let table = t.name.clone(); + let at = cell_param(p, "at").unwrap_or_else(|| CellRef::new(t.range.start.row, t.range.end.col.saturating_add(2))); + let columns = u32_param(p, "columns").unwrap_or(1).clamp(1, 10); + let width = f64_param(p, "width").unwrap_or(168.0).clamp(60.0, 10000.0) as f32; + let height = f64_param(p, "height").unwrap_or(200.0).clamp(60.0, 10000.0) as f32; + let id = d.wb.next_object_id(); + let slicer = Slicer { + id, + table, + column: column.clone(), + caption: column, + anchor: Anchor { cell: at, dx: 4.0, dy: 4.0, width, height, mode: AnchorMode::MoveOnly }, + selected: None, + columns, + }; + edit(s, |cx| { + cx.sheet_mut(sheet)?.slicers.push(slicer); + Ok(json!({"slicer": id})) + }) +} + +/// The target slicer: by `slicer` id, else the last one on the active sheet. +fn find_slicer(s: &Session, p: &Json) -> Result<(usize, u32)> { + let d = s.doc()?; + let sh = d.wb.active().ok_or(EngineError::NoDocument)?; + let idx = match u32_param(p, "slicer") { + Some(id) => sh.slicers.iter().position(|x| x.id == id), + None => sh.slicers.len().checked_sub(1), + }; + idx.and_then(|i| sh.slicers.get(i).map(|x| (i, x.id))).ok_or_else(|| bad("slicer", "no slicer on this sheet")) +} + +fn slicer_toggle(s: &mut Session, p: &Json) -> Result { + let (idx, id) = find_slicer(s, p)?; + let value = str_param(p, "value").ok_or_else(|| bad("slicer.toggle", "missing `value`"))?.to_string(); + let multi = bool_param(p, "multi").unwrap_or(false); + let sheet = s.doc()?.wb.active_sheet; + let (table, column, current) = { + let d = s.doc()?; + let sl = d.wb.active().and_then(|sh| sh.slicers.get(idx)).ok_or_else(|| bad("slicer.toggle", "no slicer"))?; + (sl.table.clone(), sl.column.clone(), sl.selected.clone()) + }; + let all = column_values(&s.doc()?.wb, sheet, &table, &column); + if !all.iter().any(|v| v.eq_ignore_ascii_case(&value)) { + return Err(bad("slicer.toggle", "value is not in the column")); + } + let new_selected: Option> = if multi { + let mut set: Vec = current.unwrap_or_else(|| all.clone()); + match set.iter().position(|x| x.eq_ignore_ascii_case(&value)) { + Some(pos) => { + set.remove(pos); + } + None => set.push(value.clone()), + } + // Everything selected is the same as no filter. + if set.len() == all.len() && all.iter().all(|a| set.iter().any(|x| x.eq_ignore_ascii_case(a))) { None } else { Some(set) } + } else { + // Single click: show only this value; clicking the sole selected value clears the filter. + match ¤t { + Some(set) if set.len() == 1 && set.iter().any(|x| x.eq_ignore_ascii_case(&value)) => None, + _ => Some(vec![value.clone()]), + } + }; + edit(s, |cx| { + if let Some(sl) = cx.sheet_mut(sheet)?.slicers.get_mut(idx) { + sl.selected = new_selected; + } + reapply_slicer(&mut cx.wb, sheet, id); + cx.structural = true; + Ok(Json::Null) + }) +} + +fn slicer_select_all(s: &mut Session, p: &Json) -> Result { + let (idx, id) = find_slicer(s, p)?; + let sheet = s.doc()?.wb.active_sheet; + edit(s, |cx| { + if let Some(sl) = cx.sheet_mut(sheet)?.slicers.get_mut(idx) { + sl.selected = None; + } + reapply_slicer(&mut cx.wb, sheet, id); + cx.structural = true; + Ok(Json::Null) + }) +} + +fn slicer_delete(s: &mut Session, p: &Json) -> Result { + let (idx, id) = find_slicer(s, p)?; + let sheet = s.doc()?.wb.active_sheet; + // Absolute column of the slicer, so its filter criterion can be removed too. + let abs_col = s.doc().ok().and_then(|d| { + let sh = d.wb.active()?; + let sl = sh.slicers.get(idx)?; + let t = sh.tables.iter().find(|t| t.name.eq_ignore_ascii_case(&sl.table))?; + let ci = t.column_index(&sl.column)?; + Some(t.range.start.col + ci as u32) + }); + edit(s, |cx| { + let sh = cx.sheet_mut(sheet)?; + sh.slicers.retain(|x| x.id != id); + if let (Some(abs), Some(af)) = (abs_col, sh.autofilter.as_mut()) { + let off = abs.saturating_sub(af.range.start.col); + af.criteria.retain(|(o, _)| *o != off); + } + super::data::apply_filter(&mut cx.wb, sheet); + cx.structural = true; + Ok(Json::Null) + }) +} diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 9121a06..ad30855 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -647,6 +647,59 @@ fn tables_and_structured_refs() { assert_eq!(v(&s, "B4"), Value::Number(7.0)); } +#[test] +fn slicer_filters_table() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Region", "Qty"], ["East", 2], ["West", 5], ["East", 3], ["North", 1]]})).unwrap(); + s.execute("insert.table", json!({"range": "A1:B5", "name": "Sales"})).unwrap(); + s.execute("selection.set", json!({"cell": "A2"})).unwrap(); + + // A new slicer on the Region column filters nothing (all values selected). + let r = s.execute("insert.slicer", json!({})).unwrap(); + let id = r["slicer"].as_u64().unwrap() as u32; + let hidden = |s: &Session| { + let d = s.doc().unwrap(); + let sh = d.wb.active().unwrap(); + (1..=4).filter(|row| sh.is_row_hidden(*row)).count() // data rows are sheet rows 1..=4 (0-based header at 0) + }; + assert_eq!(hidden(&s), 0); + { + let d = s.doc().unwrap(); + let sh = d.wb.active().unwrap(); + assert_eq!(sh.slicers.len(), 1); + assert_eq!(sh.slicers[0].column, "Region"); + } + + // Single-click "East": only the two East rows remain (West row 2 and North row 4 hidden). + s.execute("slicer.toggle", json!({"slicer": id, "value": "East"})).unwrap(); + assert_eq!(hidden(&s), 2); + { + let d = s.doc().unwrap(); + let sh = d.wb.active().unwrap(); + assert!(sh.is_row_hidden(2) && sh.is_row_hidden(4)); + assert!(!sh.is_row_hidden(1) && !sh.is_row_hidden(3)); + } + + // Ctrl-click "West" adds it: East + West visible, only North hidden. + s.execute("slicer.toggle", json!({"slicer": id, "value": "West", "multi": true})).unwrap(); + assert_eq!(hidden(&s), 1); + + // Clear restores everything. + s.execute("slicer.clear", json!({"slicer": id})).unwrap(); + assert_eq!(hidden(&s), 0); + assert!(s.doc().unwrap().wb.active().unwrap().slicers[0].selected.is_none()); + + // Deleting the slicer drops it and clears its filter. + s.execute("slicer.toggle", json!({"slicer": id, "value": "East"})).unwrap(); + assert_eq!(hidden(&s), 2); + s.execute("slicer.delete", json!({"slicer": id})).unwrap(); + assert_eq!(hidden(&s), 0); + assert!(s.doc().unwrap().wb.active().unwrap().slicers.is_empty()); + + // A bad value is rejected, not panicked. + assert!(s.execute("slicer.toggle", json!({"value": "Nowhere"})).is_err()); +} + #[test] fn create_names_from_selection() { let names = |s: &Session| -> Vec<(String, String)> { diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index b9fe2ce..be10c91 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -479,6 +479,26 @@ pub struct AutoFilter { pub criteria: Vec<(u32, FilterCriterion)>, } +/// A slicer: an on-sheet filter widget bound to one column of a table. Clicking its value tiles +/// sets that column's `FilterCriterion::Values` in the sheet's [`AutoFilter`] and re-applies it, +/// so a slicer is just a visual front-end over the existing filter machinery. +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +pub struct Slicer { + pub id: u32, + /// Name of the table this slicer filters. + pub table: String, + /// Header name of the filtered column within the table. + pub column: String, + /// Caption shown in the slicer header (defaults to the column name). + pub caption: String, + pub anchor: Anchor, + /// Selected display values. `None` means every value (no filter from this slicer); an empty + /// list means nothing is selected (all rows hidden), matching Excel's "clear all buttons". + pub selected: Option>, + /// Number of button columns in the layout (Excel default 1). + pub columns: u32, +} + #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] pub enum Orientation { #[default] diff --git a/crates/model/src/sheet.rs b/crates/model/src/sheet.rs index 4b4200d..5e84bb4 100644 --- a/crates/model/src/sheet.rs +++ b/crates/model/src/sheet.rs @@ -54,6 +54,9 @@ pub struct Sheet { pub images: Vec, pub shapes: Vec, pub sparklines: Vec, + /// On-sheet slicer widgets (filter a table column by clicking value tiles). + #[serde(default)] + pub slicers: Vec, /// PivotTables whose output lives on this sheet. #[serde(default)] pub pivots: Vec, @@ -99,6 +102,7 @@ impl Sheet { images: vec![], shapes: vec![], sparklines: vec![], + slicers: vec![], pivots: vec![], protection: None, print: PrintSettings::default(), diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 6ccef9e..b1587ff 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -216,6 +216,9 @@ impl Workbook { for c in &s.tables { m = m.max(c.id); } + for c in &s.slicers { + m = m.max(c.id); + } } m + 1 } diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index adeeddb..188fbad 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -308,6 +308,112 @@ pub fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: } } } + paint_slicers(app, p, geo, wb, si, sh); +} + +/// Geometry of a slicer's parts within its screen rect. +pub struct SlicerLayout { + pub header: Rect, + /// Clear-filter button in the header. + pub clear: Rect, + /// One rect per value tile, in order. + pub tiles: Vec, +} + +pub fn slicer_layout(rect: Rect, z: f32, n: usize, columns: u32) -> SlicerLayout { + let header_h = 26.0 * z; + let pad = 6.0 * z; + let gap = 3.0 * z; + let tile_h = 22.0 * z; + let header = Rect::from_min_size(rect.min, vec2(rect.width(), header_h)); + let cb = 16.0 * z; + let clear = Rect::from_min_size(pos2(header.right() - pad - cb, header.top() + (header_h - cb) / 2.0), vec2(cb, cb)); + let cols = columns.max(1) as f32; + let inner_left = rect.left() + pad; + let inner_top = header.bottom() + pad; + let inner_w = (rect.width() - 2.0 * pad).max(1.0); + let tile_w = ((inner_w - (cols - 1.0) * gap) / cols).max(1.0); + let mut tiles = Vec::with_capacity(n); + for i in 0..n { + let col = (i as f32) % cols; + let row = (i as f32 / cols).floor(); + let x = inner_left + col * (tile_w + gap); + let y = inner_top + row * (tile_h + gap); + tiles.push(Rect::from_min_size(pos2(x, y), vec2(tile_w, tile_h))); + } + SlicerLayout { header, clear, tiles } +} + +fn paint_slicers(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: usize, sh: &Sheet) { + let tk = theme::Tokens::get(p.ctx()); + for sl in &sh.slicers { + let r = anchor_rect(geo, sh, &sl.anchor); + if !r.intersects(p.clip_rect()) { + continue; + } + let items = gridcraft_engine::cmd::slicer::slicer_items(wb, si, sl); + let lay = slicer_layout(r, geo.z, items.len(), sl.columns); + let radius = 4.0 * geo.z; + // Card + soft shadow. + p.rect_filled(r.translate(vec2(0.0, 1.5)), radius, Color32::from_black_alpha(18)); + p.rect_filled(r, radius, tk.window); + p.rect_stroke(r, radius, Stroke::new(1.0, tk.separator), StrokeKind::Inside); + // Header. + p.rect_filled(lay.header, radius, tk.accent_soft); + p.rect_filled(Rect::from_min_max(pos2(lay.header.left(), lay.header.bottom() - radius), lay.header.max), 0.0, tk.accent_soft); + let cap = if sl.caption.is_empty() { sl.column.clone() } else { sl.caption.clone() }; + p.text(lay.header.left_center() + vec2(8.0 * geo.z, 0.0), Align2::LEFT_CENTER, cap, theme::ui_bold(12.5 * geo.z), tk.accent_dark); + // Clear-filter button (tinted when a filter is active). + let filtered = sl.selected.is_some(); + if let Some(icon) = crate::icons::from_name("filter") { + crate::icons::paint(p, lay.clear, icon, if filtered { tk.accent } else { tk.text_dim }); + } + // Value tiles. + for ((val, on), tr) in items.iter().zip(&lay.tiles) { + if !r.contains(tr.left_top()) { + continue; // overflowed the card; skip (no scroll yet) + } + let (fill, fg) = if *on { (tk.accent, Color32::WHITE) } else { (tk.hover, tk.text_dim) }; + p.rect_filled(*tr, 3.0 * geo.z, fill); + let cp = p.with_clip_rect(tr.intersect(p.clip_rect())); + cp.text(tr.left_center() + vec2(6.0 * geo.z, 0.0), Align2::LEFT_CENTER, val, theme::ui_font(12.0 * geo.z), fg); + } + if app.selected_chart == Some(sl.id) { + selection_frame(p, r); + } + } +} + +/// What a click inside a slicer hit. +pub enum SlicerAction { + Toggle(String), + Clear, +} + +/// A click on a slicer's value tile or clear button, if any. A click elsewhere in a slicer returns +/// `None` so the caller can treat it as a select/drag on the object. +pub fn slicer_click(geo: &Geo, sh: &Sheet, wb: &Workbook, si: usize, p: Pos2) -> Option<(u32, SlicerAction)> { + if !geo.cells.contains(p) { + return None; + } + for sl in sh.slicers.iter().rev() { + let r = anchor_rect(geo, sh, &sl.anchor); + if !r.contains(p) { + continue; + } + let items = gridcraft_engine::cmd::slicer::slicer_items(wb, si, sl); + let lay = slicer_layout(r, geo.z, items.len(), sl.columns); + if lay.clear.contains(p) { + return Some((sl.id, SlicerAction::Clear)); + } + for ((val, _on), tr) in items.iter().zip(&lay.tiles) { + if r.contains(tr.left_top()) && tr.contains(p) { + return Some((sl.id, SlicerAction::Toggle(val.clone()))); + } + } + return None; // inside this (topmost) slicer, but on its header/background → select/drag + } + None } fn selection_frame(p: &Painter, r: Rect) { @@ -322,6 +428,12 @@ fn selection_frame(p: &Painter, r: Rect) { /// The topmost object under `p`: (kind, id, rect). pub fn hit(app: &SheetApp, geo: &Geo, sh: &Sheet, p: Pos2) -> Option<(&'static str, u32, Rect)> { let _ = app; + for sl in sh.slicers.iter().rev() { + let r = anchor_rect(geo, sh, &sl.anchor); + if r.contains(p) && geo.cells.contains(p) { + return Some(("slicer", sl.id, r)); + } + } for ch in sh.charts.iter().rev() { let r = anchor_rect(geo, sh, &ch.anchor); if r.expand(4.0).contains(p) && geo.cells.contains(p) { diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4bd50dd..b989d25 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -1126,7 +1126,25 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & return; } } - // Objects (charts, pictures, shapes) take clicks first. + // A plain click on a slicer value tile toggles it (Ctrl/Cmd for multi-select); the clear + // button resets it. Runs before the generic object select/drag so tiles stay clickable. + if primary_clicked + && !resp.drag_started() + && let Some(p) = pos + && let Some((id, act)) = crate::chartview::slicer_click(geo, sh, wb, wb.active_sheet, p) + { + app.selected_chart = Some(id); + match act { + crate::chartview::SlicerAction::Toggle(value) => { + let _ = app.run("slicer.toggle", json!({"slicer": id, "value": value, "multi": mods.command || mods.ctrl})); + } + crate::chartview::SlicerAction::Clear => { + let _ = app.run("slicer.clear", json!({"slicer": id})); + } + } + return; + } + // Objects (charts, pictures, shapes, slicers) take clicks first. if resp.drag_started() || primary_clicked { if let Some(p) = pos && let Some((kind, id, rect)) = crate::chartview::hit(app, geo, sh, p) @@ -1597,6 +1615,7 @@ fn object_anchor(sh: &Sheet, kind: &str, id: u32) -> Option sh.charts.iter().find(|c| c.id == id).map(|c| c.anchor), "image" => sh.images.iter().find(|c| c.id == id).map(|c| c.anchor), + "slicer" => sh.slicers.iter().find(|c| c.id == id).map(|c| c.anchor), _ => sh.shapes.iter().find(|c| c.id == id).map(|c| c.anchor), } } diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 28d025c..17c7633 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -1213,6 +1213,10 @@ fn insert(app: &mut SheetApp, ui: &mut Ui) { }); }); sep(ui); + if big_button(ui, Icon::Filter, "Slicer", "Insert a slicer to filter a table", false).clicked() { + act(app, "insert.slicer", json!({})); + } + sep(ui); let sp = big_button(ui, Icon::Sparkline, "Sparklines", "Sparklines", true); egui::Popup::menu(&sp).show(|ui| { for (label, kind) in [("Line", "line"), ("Column", "column"), ("Win/Loss", "winLoss")] { @@ -1738,6 +1742,10 @@ fn table_design(app: &mut SheetApp, ui: &mut Ui) { act(app, "table.convertToRange", json!({"table": tname})); } sep(ui); + if big_button(ui, Icon::Filter, "Insert\nSlicer", "Insert Slicer", false).clicked() { + act(app, "table.insertSlicer", json!({"table": tname})); + } + sep(ui); ui.vertical(|ui| { for (label, cmd, v) in [ ("Header Row", "table.headerRow", header && filter), From b40fdc247097f052cd780d2809b22f5c35fb5740 Mon Sep 17 00:00:00 2001 From: Zaid Shurbaji Date: Sat, 10 Oct 2026 21:27:14 +0300 Subject: [PATCH 07/27] ci: add macOS universal DMG build workflow Builds GridCraft.app into a universal .dmg on a GitHub-hosted macOS runner, ad-hoc signed, uploaded as a workflow artifact. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/macos-build.yml | 36 +++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 .github/workflows/macos-build.yml diff --git a/.github/workflows/macos-build.yml b/.github/workflows/macos-build.yml new file mode 100644 index 0000000..2180636 --- /dev/null +++ b/.github/workflows/macos-build.yml @@ -0,0 +1,36 @@ +# Builds the macOS universal DMG (Apple silicon + Intel) on a GitHub-hosted Mac runner. +# +# Added for the table-slicers branch so a macOS build can be produced without a Mac: a .dmg +# can't be built on Windows/Linux (it needs Apple-only tools: lipo, codesign, hdiutil, iconutil). +# +# No signing secrets are used, so packaging/macos/package.sh signs the app AD-HOC and skips +# notarization. Gatekeeper will warn on first open; right-click the app > Open, or run +# `xattr -dr com.apple.quarantine /Applications/GridCraft.app` after installing. +# +# Run it: Actions tab > "macOS build" > Run workflow. The DMG is uploaded as a build artifact. +name: macOS build + +on: + workflow_dispatch: + push: + branches: [feature/table-slicers] + +jobs: + macos: + name: macOS universal (ad-hoc) + runs-on: macos-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@686976e191b89faba57d3206551f0f330d8cb249 # stable + with: + targets: aarch64-apple-darwin, x86_64-apple-darwin + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + with: + key: macos-slicers-universal + - name: Build and package (ad-hoc signed, not notarized) + run: packaging/macos/package.sh --arch universal + - uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: gridcraft-macos-universal-dmg + path: dist/release/*.dmg + if-no-files-found: error From e8d935298f45de4922cecdf9396b2e91b88db4d5 Mon Sep 17 00:00:00 2001 From: GyuriX Date: Sat, 10 Oct 2026 21:36:26 +0200 Subject: [PATCH 08/27] Linux: the display theme asks the desktop, not winit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit winit reports no system theme on Linux — `ActiveEventLoop::system_theme()` is `None` there on every desktop — so `System` in the appearance setting never saw the desktop's choice and fell back to light. A new `desktop_theme` module asks the XDG Desktop Portal's Settings interface for `org.gnome.desktop.interface color-scheme` on a thread of its own, so no D-Bus call is ever made from the frame loop, and the app follows the answer, including a change made while it runs. egui resolves `ThemePreference::System` from what winit reports and offers no way to set that itself, so where a desktop answered, the app hands egui the concrete theme instead. What is drawn is unchanged on macOS and Windows, where winit answers and the same resolution happens a step earlier; the saved preference is still `system`, so a file keeps meaning the same thing. The colour scheme is asked for under its GNOME schema name first, which GNOME's portal and XFCE's both answer, then under the older freedesktop one, which KDE answers. A desktop that answers neither leaves the user's own choice alone. --- Cargo.lock | 1 + crates/ui-egui/Cargo.toml | 6 ++ crates/ui-egui/src/desktop_theme.rs | 132 +++++++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 19 +++- crates/ui-egui/tests/system_theme.rs | 4 +- 5 files changed, 159 insertions(+), 3 deletions(-) create mode 100644 crates/ui-egui/src/desktop_theme.rs diff --git a/Cargo.lock b/Cargo.lock index 6ff01cc..a4631e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1546,6 +1546,7 @@ dependencies = [ "sys-locale", "toml", "web-sys", + "zbus", ] [[package]] diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index a6a2da2..bd48b92 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -17,6 +17,12 @@ serde_json = { workspace = true } log = { workspace = true } image = { workspace = true } +# Linux only, where winit reports no system theme: the desktop portal's light/dark answer (see +# `desktop_theme.rs`). Already in the tree, with these features, through egui's own +# accessibility bus; pinned to one version so nothing is built twice. +[target.'cfg(target_os = "linux")'.dependencies] +zbus = "=5.19.0" + [dev-dependencies] egui_kittest = { workspace = true, features = ["wgpu", "snapshot"] } diff --git a/crates/ui-egui/src/desktop_theme.rs b/crates/ui-egui/src/desktop_theme.rs new file mode 100644 index 0000000..2eb5d83 --- /dev/null +++ b/crates/ui-egui/src/desktop_theme.rs @@ -0,0 +1,132 @@ +//! The desktop's own light/dark choice, for an interface setting that follows it. +//! +//! winit reports no system theme on Linux — `ActiveEventLoop::system_theme` is `None` on every +//! desktop there — so "Use system setting" has to ask the desktop itself. The XDG Desktop Portal's +//! Settings interface answers on GNOME, KDE and XFCE alike, over X11 and Wayland alike. + +use std::sync::mpsc::{Receiver, TryRecvError, channel}; +use std::time::Duration; + +/// How often [`DesktopTheme::start`] re-reads the desktop's answer. The portal also publishes a +/// change signal, but subscribing needs a connection kept alive for the life of every window; one +/// round trip every few seconds is cheap and reacts soon enough after a theme switch. +const POLL: Duration = Duration::from_secs(4); + +/// Whether the desktop asks applications to draw dark. `None` when it says nothing, and when +/// there is no session bus or no portal to ask (a bare X server, a container), so the caller +/// keeps whatever it chose itself. +pub fn prefers_dark() -> Option { + read() +} + +/// Watches the desktop and reports its choice when it changes. [`prefers_dark`] covers the +/// answer at start-up; this covers the user switching theme while the app runs. +#[derive(Default)] +pub struct DesktopTheme { + /// `None` when no watcher thread could be started; the one-shot read still works. + changes: Option>>, +} + +impl DesktopTheme { + /// Start watching. Nothing runs before this is called, and calling it once is enough. The + /// first answer is sent straight away, so the caller does not have to ask for it separately. + pub fn start() -> Self { + let (tx, rx) = channel(); + let started = std::thread::Builder::new().name("gridcraft-desktop-theme".into()).spawn(move || { + let mut last = read(); + if tx.send(last).is_err() { + return; + } + loop { + std::thread::sleep(POLL); + let now = read(); + if now != last { + last = now; + if tx.send(now).is_err() { + return; + } + } + } + }); + Self { changes: started.ok().map(|_| rx) } + } + + /// The desktop's new answer if it changed since the last call, to be polled once a frame. + /// Several changes in one frame collapse to the newest, and a desktop that stops answering + /// leaves the last answer in place. + pub fn take_change(&mut self) -> Option { + let Some(changes) = &self.changes else { return None }; + let mut newest = None; + loop { + match changes.try_recv() { + Ok(changed) => newest = changed, + Err(TryRecvError::Empty) | Err(TryRecvError::Disconnected) => return newest, + } + } + } +} + +/// Ask the portal once. D-Bus calls block, so this belongs off the UI thread: the watcher's +/// thread is its only caller. +#[cfg(target_os = "linux")] +fn read() -> Option { + const PORTAL: &str = "org.freedesktop.portal.Desktop"; + const SETTINGS: &str = "org.freedesktop.portal.Settings"; + const SCHEMAS: [(&str, &str); 2] = [("org.gnome.desktop.interface", "color-scheme"), ("org.freedesktop.interface.color-scheme", "color-scheme")]; + let connection = zbus::blocking::Connection::session().ok()?; + for (schema, key) in SCHEMAS { + let Ok(reply) = connection.call_method(Some(PORTAL), "/org/freedesktop/portal/desktop", Some(SETTINGS), "ReadOne", &(schema, key)) else { + continue; + }; + let Ok((answer,)) = reply.body().deserialize::<(zbus::zvariant::OwnedValue,)>() else { continue }; + if let Ok(scheme) = String::try_from(&*answer) { + return parse(&scheme); + } + } + None +} + +/// The portal's `color-scheme` answer. `default` means the desktop states no preference, so +/// neither does the application. +#[cfg(target_os = "linux")] +fn parse(scheme: &str) -> Option { + match scheme { + "prefer-dark" => Some(true), + "prefer-light" => Some(false), + _ => None, + } +} + +/// macOS and Windows answer through winit, which the caller already has. +#[cfg(not(target_os = "linux"))] +fn read() -> Option { + None +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The portal answers with one of three strings; only the two preferences are a choice. + #[test] + #[cfg(target_os = "linux")] + fn the_portal_answer_reads_as_a_choice_or_as_none() { + assert_eq!(parse("prefer-dark"), Some(true)); + assert_eq!(parse("prefer-light"), Some(false)); + assert_eq!(parse("default"), None); + assert_eq!(parse(""), None); + } + + /// Off Linux the caller already has winit's answer, so this adds nothing. + #[test] + #[cfg(not(target_os = "linux"))] + fn other_platforms_answer_through_winit() { + assert_eq!(prefers_dark(), None); + } + + /// A watcher that was never started is inert rather than a panic. + #[test] + fn a_watcher_without_a_thread_is_inert() { + assert_eq!(DesktopTheme::default().take_change(), None); + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index b039943..b6ef62b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -35,6 +35,7 @@ use serde::{Deserialize, Serialize}; use serde_json::{Value as Json, json}; pub use control::{ControlRequest, ControlResponse}; +pub mod desktop_theme; /// Persisted UI preferences. #[derive(Clone, Debug, Serialize, Deserialize)] @@ -155,6 +156,10 @@ pub struct SheetApp { /// only when it changes this (see `theme::HanOrder`). fonts_han: Option, effective_dark: bool, + /// The desktop's light/dark choice, watched while the app runs (Linux has no winit answer). + desktop_theme: desktop_theme::DesktopTheme, + /// The last answer [`desktop_theme`] gave, so a frame that asks costs nothing. + desktop_dark: Option, pub name_box: Option, /// Transient ribbon keyboard navigation; never saved with UI preferences. pub keytips: keytips::KeyTips, @@ -189,6 +194,8 @@ impl SheetApp { fonts_set: false, fonts_han: None, effective_dark: false, + desktop_theme: desktop_theme::DesktopTheme::start(), + desktop_dark: None, name_box: None, keytips: keytips::KeyTips::default(), shots: control::Shots::default(), @@ -543,7 +550,17 @@ impl SheetApp { ctx.request_repaint(); } } - let preference = self.ui.theme_preference(); + if let Some(answer) = self.desktop_theme.take_change() { + self.desktop_dark = Some(answer); + } + let preference = match ( + self.ui.system_theme, + ctx.system_theme().or_else(|| self.desktop_dark.map(|d| if d { egui::Theme::Dark } else { egui::Theme::Light })), + ) { + (true, Some(egui::Theme::Dark)) => egui::ThemePreference::Dark, + (true, Some(egui::Theme::Light)) => egui::ThemePreference::Light, + _ => self.ui.theme_preference(), + }; if ctx.options(|o| o.theme_preference) != preference { ctx.set_theme(preference); ctx.request_repaint(); diff --git a/crates/ui-egui/tests/system_theme.rs b/crates/ui-egui/tests/system_theme.rs index 0d2cdea..c92ca91 100644 --- a/crates/ui-egui/tests/system_theme.rs +++ b/crates/ui-egui/tests/system_theme.rs @@ -99,7 +99,7 @@ fn system_tracks_live_os_changes_with_both_gridcraft_palettes_and_light_fallback system_theme(&mut h, reported); assert_appearance(&h, dark); assert_eq!(preferences(&h)["systemTheme"], true); - assert_eq!(h.ctx.options(|o| o.theme_preference), ThemePreference::System); + assert_eq!(h.ctx.theme(), if dark { Theme::Dark } else { Theme::Light }); } } @@ -112,7 +112,7 @@ fn ribbon_selects_system_and_fixed_modes_ignore_subsequent_os_changes() { h.run_steps(2); select_ribbon_theme(&mut h, "System"); assert_eq!(preferences(&h)["systemTheme"], true); - assert_eq!(h.ctx.options(|o| o.theme_preference), ThemePreference::System); + assert_eq!(h.ctx.theme(), Theme::Dark); assert_appearance(&h, true); for (label, dark, preference) in [("Dark", true, ThemePreference::Dark), ("Light", false, ThemePreference::Light)] { select_ribbon_theme(&mut h, label); From d27dec031206600b118abacd99b6438b9ffe5a19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dejan=20Koci=C4=87?= Date: Sat, 10 Oct 2026 12:05:32 +0200 Subject: [PATCH 09/27] web: print straight to the system print dialog, not just export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The web build had no way to actually print — Ctrl+P (Cmd+P, declared on file.print's shortcut) did nothing at all, and the ribbon's "Print…" menu item ran file.print and silently discarded the result. file.print already builds a PDF and returns it as base64 specifically "for the UI to hand to the system print dialog" per its own doc comment — this wires that up. - ui-egui: new Services::print hook (web: hand PDF bytes to the browser's print flow directly, no download); SheetApp::print_now runs file.print, decodes the base64 result, and calls the hook. - ui-egui: Key::P now dispatches to print_now (was unbound); the ribbon's "Print…" menu item calls print_now instead of discarding file.print's result. - gridcraft-web: implements the hook (Blob -> object URL -> hidden iframe -> iframe.contentWindow.print() on load). Desktop builds are unaffected (no Services::print set there; print_now falls back to the existing alert-on-error behaviour with no UI action on success, same as before this change). --- apps/gridcraft-web/Cargo.toml | 2 +- apps/gridcraft-web/src/web.rs | 47 +++++++++++++++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 29 +++++++++++++++++++++ crates/ui-egui/src/ribbon.rs | 8 +++++- 4 files changed, 84 insertions(+), 2 deletions(-) diff --git a/apps/gridcraft-web/Cargo.toml b/apps/gridcraft-web/Cargo.toml index 356c63b..0f69e38 100644 --- a/apps/gridcraft-web/Cargo.toml +++ b/apps/gridcraft-web/Cargo.toml @@ -26,4 +26,4 @@ rfd = { workspace = true } wasm-bindgen = "0.2" wasm-bindgen-futures = "0.4" js-sys = "0.3" -web-sys = { version = "0.3", features = ["Window", "Document", "Element", "HtmlElement", "HtmlCanvasElement", "HtmlAnchorElement", "Blob", "BlobPropertyBag", "Url", "Location", "CssStyleDeclaration", "Clipboard", "ClipboardItem", "Navigator"] } +web-sys = { version = "0.3", features = ["Window", "Document", "Element", "HtmlElement", "HtmlCanvasElement", "HtmlAnchorElement", "HtmlIFrameElement", "Blob", "BlobPropertyBag", "Url", "Location", "CssStyleDeclaration", "Clipboard", "ClipboardItem", "Navigator"] } diff --git a/apps/gridcraft-web/src/web.rs b/apps/gridcraft-web/src/web.rs index f5744bd..9f758b4 100644 --- a/apps/gridcraft-web/src/web.rs +++ b/apps/gridcraft-web/src/web.rs @@ -136,6 +136,11 @@ fn services(inbox: Inbox, ctx: egui::Context) -> Services { log::error!("download of {name} failed: {e}"); } })), + print: Some(Box::new(|bytes: &[u8]| { + if let Err(e) = print_pdf(bytes) { + log::error!("print failed: {e}"); + } + })), open_url: Some(Box::new(|url: &str| { if let Some(w) = web_sys::window() { let _ = w.open_with_url_and_target(url, "_blank"); @@ -179,6 +184,48 @@ fn copy_html(html: &str, text: &str) -> Result<(), String> { Ok(()) } +/// Opens `bytes` (a PDF) as an object URL in a hidden iframe and calls the iframe's own +/// `print()` once it has finished loading — the standard way to drive the browser's native print +/// dialog on a PDF without a download or a popup window the browser might block. The blob URL and +/// the iframe are both cleaned up once the print dialog closes, so repeated prints don't leak +/// them. +fn print_pdf(bytes: &[u8]) -> Result<(), String> { + let js = |e: wasm_bindgen::JsValue| format!("{e:?}"); + let window = web_sys::window().ok_or("no window")?; + let document = window.document().ok_or("no document")?; + + let parts = js_sys::Array::of1(&js_sys::Uint8Array::from(bytes)); + let opts = web_sys::BlobPropertyBag::new(); + opts.set_type("application/pdf"); + let blob = web_sys::Blob::new_with_u8_array_sequence_and_options(&parts, &opts).map_err(js)?; + let url = web_sys::Url::create_object_url_with_blob(&blob).map_err(js)?; + + let iframe: web_sys::HtmlIFrameElement = document.create_element("iframe").map_err(js)?.dyn_into().map_err(|_| "not an iframe")?; + iframe.style().set_property("display", "none").map_err(js)?; + document.body().ok_or("no body")?.append_child(&iframe).map_err(js)?; + + // `onload` fires once the PDF has actually rendered inside the iframe; + // printing before that would show a blank page. Set the handler BEFORE + // `src` so a fast/cached load can't fire before we're listening. + let iframe_for_load = iframe.clone(); + let url_for_cleanup = url.clone(); + let on_load = wasm_bindgen::closure::Closure::once_into_js(move || { + let Some(content_window) = iframe_for_load.content_window() else { return }; + let _ = content_window.print(); + // `afterprint` fires once the print dialog (or print preview) closes, whether the user + // printed or cancelled — the right moment to release the blob URL and drop the iframe. + let iframe_for_cleanup = iframe_for_load.clone(); + let on_after_print = wasm_bindgen::closure::Closure::once_into_js(move || { + web_sys::Url::revoke_object_url(&url_for_cleanup).ok(); + iframe_for_cleanup.remove(); + }); + let _ = content_window.add_event_listener_with_callback("afterprint", on_after_print.unchecked_ref()); + }); + iframe.set_onload(Some(on_load.unchecked_ref())); + iframe.set_src(&url); + Ok(()) +} + /// Triggers a browser download of `bytes`. fn download(name: &str, bytes: &[u8]) -> Result<(), String> { let js = |e: wasm_bindgen::JsValue| format!("{e:?}"); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1225eec..6ca6f05 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -121,6 +121,9 @@ pub struct Services { pub open_async: Option>, /// Files delivered asynchronously (name, bytes), opened on the next frame. pub inbox: Option, + /// Web: hand PDF bytes to the browser's print flow directly (no download, + /// no intermediate file) — opens the system print dialog on that PDF. + pub print: Option>, } /// Shared queue of files read asynchronously (browser file picker, drag and drop). @@ -287,6 +290,32 @@ impl SheetApp { } } + /// Runs `file.print`, builds the PDF and (where the host provides the hook — the web + /// build) hands the bytes straight to the browser's print flow; errors show the usual + /// alert. Desktop builds have no `services.print`, so this falls back to `run_or_alert` + /// (which still produces the PDF — just without a UI to act on the result yet). + pub fn print_now(&mut self) { + match self.run("file.print", json!({})) { + Ok(r) => { + let Some(b64) = r.get("base64").and_then(Json::as_str) else { return }; + let Some(bytes) = gridcraft_engine::io::base64_decode(b64) else { + self.message = Some(("GridCraft".into(), "Couldn't decode the generated PDF.".into())); + return; + }; + if let Some(print) = &self.services.print { + print(&bytes); + } + } + Err(e) => { + if e.contains("is not available right now") { + self.toast = Some((e, now_ms())); + } else { + self.message = Some(("GridCraft".into(), clean_error(&e))); + } + } + } + } + /// UI-only commands (dialogs, view toggles that aren't saved in the file). fn run_ui_command(&mut self, id: &str, p: &Json) -> Option> { let r = match id { diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 264479a..5fc91f8 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -76,7 +76,6 @@ pub fn title_bar(app: &mut SheetApp, ui: &mut Ui) { ("New Workbook", "file.new"), ("Open…", "file.open"), ("Save As…", "file.saveAs"), - ("Print…", "file.print"), ("Sort A to Z", "data.sortAscending"), ("Calculate Now", "formulas.calculateNow"), ] { @@ -84,6 +83,9 @@ pub fn title_bar(app: &mut SheetApp, ui: &mut Ui) { app.run_or_alert(id, json!({})); } } + if ui.button("Print…").clicked() { + app.print_now(); + } }); // Centered title. let title = app.session.active().map(|d| d.display_title()).unwrap_or_default(); @@ -1850,6 +1852,10 @@ pub fn shortcut(app: &mut SheetApp, key: Key, m: Modifiers) { Key::O => Some(("file.open", json!({}))), Key::N => Some(("file.new", json!({}))), Key::W => Some(("file.close", json!({}))), + Key::P => { + app.print_now(); + None + } Key::K => { app.open_dialog("insertLink", json!({})); None From 7a7e23cab71676e51ee2741bc6d88661756161aa Mon Sep 17 00:00:00 2001 From: Zarvinx Date: Sun, 11 Oct 2026 00:27:50 +0300 Subject: [PATCH 10/27] Grid: reorder selected columns by header drag --- crates/engine/src/cmd/sheet.rs | 100 +++++++++++++++++++++ crates/engine/src/tests.rs | 52 +++++++++++ crates/formula/src/adjust.rs | 19 ++-- crates/formula/src/tests.rs | 2 + crates/ui-egui/src/grid.rs | 35 +++++++- crates/ui-egui/tests/header_column_drag.rs | 65 ++++++++++++++ docs/ui-parity.md | 4 +- 7 files changed, 263 insertions(+), 14 deletions(-) create mode 100644 crates/ui-egui/tests/header_column_drag.rs diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index 24e6268..7affcdb 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -35,6 +35,7 @@ pub fn specs() -> Vec { p, Axis::Cols )), + cmd!("sheet.moveColumns", "Move Columns", [], None, "{cols?: \"B:C\", before: zero-based column index}", has_doc, move_columns), cmd!("home.insertCells", "Insert Cells…", ["Home", "Cells", "Insert"], None, "{range?, shift: right|down|row|column}", has_doc, insert_cells), cmd!("home.deleteCells", "Delete Cells…", ["Home", "Cells", "Delete"], None, "{range?, shift: left|up|row|column}", has_doc, delete_cells), cmd!("home.insertSheet", "Insert Sheet", ["Home", "Cells", "Insert"], Some("Shift+F11"), "{name?, before?: index}", has_doc, insert_sheet), @@ -321,6 +322,105 @@ fn delete_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { Ok(out) } +/// Reorders a contiguous block of whole columns without replacing the destination columns. +/// `before` is a boundary in the original sheet: moving B:C before E produces A,D,B,C,E. +fn move_columns(s: &mut Session, p: &Json) -> Result { + let (from, count) = lines(s, p, Axis::Cols)?; + let before = u32_param(p, "before").ok_or_else(|| bad("sheet.moveColumns", "missing `before`"))?; + let end = + from.checked_add(count).filter(|end| *end <= MAX_COLS).ok_or_else(|| bad("sheet.moveColumns", "source columns are outside the worksheet"))?; + if before >= MAX_COLS || before.checked_add(count).is_none_or(|end| end > MAX_COLS) { + return Err(bad("sheet.moveColumns", "destination columns are outside the worksheet")); + } + if (from..=end).contains(&before) { + return Ok(json!({"cols": RangeRef::cols(from, end.saturating_sub(1)).a1()})); + } + let sheet = target_sheet(s, p)?; + if let Some(u) = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.used_range()) + && u.end.col >= before + && u.end.col as u64 + count as u64 >= MAX_COLS as u64 + { + return Err(EngineError::Other("To prevent possible loss of data, columns cannot be shifted off of the worksheet.".into())); + } + check_lines_protection(s, &json!({"cols": RangeRef::cols(before, before).a1()}), Axis::Cols, before, count, true)?; + check_lines_protection(s, &json!({"cols": RangeRef::cols(from, end - 1).a1()}), Axis::Cols, from, count, false)?; + + let moved_from = if before <= from { from + count } else { from }; + if moved_from.checked_add(count).is_none_or(|end| end > MAX_COLS) { + return Err(bad("sheet.moveColumns", "the move is too close to the worksheet boundary")); + } + let destination = before; + let delete_at = moved_from; + let final_start = if before > from { before - count } else { before }; + let active_row = s.doc()?.selection.active.row; + let source = RangeRef::cols(moved_from, moved_from + count - 1); + let out = edit(s, |cx| { + let name = cx.wb.sheet(sheet).map(|sh| sh.name.clone()).unwrap_or_default(); + cx.protection_checked = true; + + { + let sh = cx.sheet_mut(sheet)?; + sh.cells.shift_cols(before, count as i64); + shift_sheet_features(sh, Axis::Cols, before, count, true); + } + rewrite(&mut cx.wb, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); + + let mut touched = Vec::new(); + { + let sh = cx.sheet_mut(sheet)?; + let pictures: Vec<_> = + sh.cell_pictures.iter().filter(|(cell, _)| source.contains(**cell)).map(|(cell, picture)| (*cell, picture.clone())).collect(); + let cells = sh.take_cells(source); + let moved_lines: Vec<_> = (0..count).filter_map(|offset| sh.cols.remove(&(moved_from + offset)).map(|info| (offset, info))).collect(); + let movement = Edit::Move { from: source, to_row: 0, to_col: destination }; + shift_cell_features(sh, &movement); + for object in sh + .charts + .iter_mut() + .map(|o| &mut o.anchor) + .chain(sh.images.iter_mut().map(|o| &mut o.anchor)) + .chain(sh.shapes.iter_mut().map(|o| &mut o.anchor)) + { + if object.mode != AnchorMode::Absolute && (moved_from..moved_from + count).contains(&object.cell.col) { + object.cell.col = destination + (object.cell.col - moved_from); + } + } + for (cell, value) in cells { + let dest = CellRef::new(cell.row, destination + (cell.col - moved_from)); + sh.cells.set(dest, value); + touched.extend([cell, dest]); + } + for (cell, picture) in pictures { + let dest = CellRef::new(cell.row, destination + (cell.col - moved_from)); + sh.cell_pictures.insert(dest, picture); + } + for (offset, info) in moved_lines { + sh.cols.insert(destination + offset, info); + } + } + for cell in touched { + cx.touch(sheet, cell); + } + rewrite(&mut cx.wb, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); + + { + let sh = cx.sheet_mut(sheet)?; + sh.cells.shift_cols(delete_at + count, -(count as i64)); + shift_sheet_features(sh, Axis::Cols, delete_at, count, false); + } + rewrite(&mut cx.wb, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); + *cx.sel = crate::Selection { + active: CellRef::new(active_row, final_start), + anchor: CellRef::new(active_row, final_start), + ranges: vec![RangeRef::cols(final_start, final_start + count - 1)], + }; + cx.structural = true; + Ok(json!({"cols": RangeRef::cols(final_start, final_start + count - 1).a1()})) + })?; + cancel_copy_mode(s); + Ok(out) +} + fn insert_cells(s: &mut Session, p: &Json) -> Result { let shift = str_param(p, "shift").unwrap_or("down"); let r = target_range(s, p)?; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6bd03d5..890d297 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -417,6 +417,58 @@ fn insert_delete_rows_adjust() { assert_eq!(v(&s, "A1"), Value::Number(10.0)); } +#[test] +fn move_columns_reorders_without_overwriting_and_undoes_once() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [[1, 2, 3, 4, 5, 6, 7, 8]]})).unwrap(); + s.execute("cell.set", json!({"cell": "H2", "input": "=B1+D1"})).unwrap(); + s.execute("cell.set", json!({"cell": "H3", "input": "=SUM(B:B)"})).unwrap(); + s.execute("home.columnWidth", json!({"cols": "B:B", "width": 101})).unwrap(); + s.execute("home.columnWidth", json!({"cols": "C:C", "width": 102})).unwrap(); + let before = s.doc().unwrap().wb.clone(); + s.doc_mut().unwrap().undo.clear(); + + let result = s.execute("sheet.moveColumns", json!({"cols": "B:C", "before": 4})).unwrap(); + assert_eq!(result["cols"], "C:D"); + assert_eq!( + (0..8).map(|col| v(&s, &CellRef::new(0, col).a1())).collect::>(), + vec![ + Value::Number(1.0), + Value::Number(4.0), + Value::Number(2.0), + Value::Number(3.0), + Value::Number(5.0), + Value::Number(6.0), + Value::Number(7.0), + Value::Number(8.0), + ] + ); + assert_eq!(s.execute("cell.get", json!({"cell": "H2"})).unwrap()["formula"], "=C1+B1"); + assert_eq!(s.execute("cell.get", json!({"cell": "H3"})).unwrap()["formula"], "=SUM(C:C)"); + let sh = s.doc().unwrap().wb.active().unwrap(); + assert_eq!(sh.col_width(2), 101.0); + assert_eq!(sh.col_width(3), 102.0); + assert_eq!(s.doc().unwrap().selection.current().a1(), "C:D"); + assert_eq!(s.doc().unwrap().undo.len(), 1); + + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(*s.doc().unwrap().wb, *before); +} + +#[test] +fn move_columns_can_reorder_left_and_ignores_drops_inside_the_selection() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["A", "B", "C", "D", "E"]]})).unwrap(); + s.execute("sheet.moveColumns", json!({"cols": "D:E", "before": 1})).unwrap(); + assert_eq!( + (0..5).map(|col| v(&s, &CellRef::new(0, col).a1())).collect::>(), + vec!["A".into(), "D".into(), "E".into(), "B".into(), "C".into()] + ); + let undo = s.doc().unwrap().undo.len(); + s.execute("sheet.moveColumns", json!({"cols": "B:C", "before": 2})).unwrap(); + assert_eq!(s.doc().unwrap().undo.len(), undo); +} + #[test] fn inserting_or_deleting_cells_cancels_copy_mode() { let mut s = s(); diff --git a/crates/formula/src/adjust.rs b/crates/formula/src/adjust.rs index 7601674..9424d7d 100644 --- a/crates/formula/src/adjust.rs +++ b/crates/formula/src/adjust.rs @@ -195,17 +195,6 @@ fn adjust_ref(r: &Reference, edit: &Edit) -> Option { let kind = match &r.kind { RefKind::Cell(a) => RefKind::Cell(mv(a)?), RefKind::Range(a, b) => RefKind::Range(mv(a)?, mv(b)?), - k => k.clone(), - }; - Some(Reference { sheet: r.sheet.clone(), kind }) - } - Edit::MoveToSheet { from, to_sheet, to_row, to_col } => { - if !from.contains_range(&r.range()) { - return Some(r.clone()); - } - let mut moved = adjust_ref(r, &Edit::Move { from: *from, to_row: *to_row, to_col: *to_col })?; - // Whole-row/column references have no anchors, but their indices must move too. - moved.kind = match &r.kind { RefKind::Rows(a, aa, b, ba) => { let dr = *to_row as i64 - from.start.row as i64; RefKind::Rows(shift_line(*a, false, dr, MAX_ROWS)?, *aa, shift_line(*b, false, dr, MAX_ROWS)?, *ba) @@ -214,8 +203,14 @@ fn adjust_ref(r: &Reference, edit: &Edit) -> Option { let dc = *to_col as i64 - from.start.col as i64; RefKind::Cols(shift_line(*a, false, dc, MAX_COLS)?, *aa, shift_line(*b, false, dc, MAX_COLS)?, *ba) } - _ => moved.kind, }; + Some(Reference { sheet: r.sheet.clone(), kind }) + } + Edit::MoveToSheet { from, to_sheet, to_row, to_col } => { + if !from.contains_range(&r.range()) { + return Some(r.clone()); + } + let mut moved = adjust_ref(r, &Edit::Move { from: *from, to_row: *to_row, to_col: *to_col })?; moved.sheet = SheetSel::Named(to_sheet.clone()); Some(moved) } diff --git a/crates/formula/src/tests.rs b/crates/formula/src/tests.rs index 9a6dab3..78dd7eb 100644 --- a/crates/formula/src/tests.rs +++ b/crates/formula/src/tests.rs @@ -171,6 +171,8 @@ fn moves_and_renames() { let mv = Edit::Move { from: gridcraft_core::RangeRef::parse("A1:B2").unwrap(), to_row: 9, to_col: 3 }; let e = parse("A1+B2+C3+SUM(A1:B2)").unwrap(); assert_eq!(print(&adjust(e, "S", "S", &mv)), "D10+E11+C3+SUM(D10:E11)"); + let mv = Edit::Move { from: gridcraft_core::RangeRef::parse("B:C").unwrap(), to_row: 0, to_col: 4 }; + assert_eq!(print(&adjust(parse("SUM(B:B)+SUM($C:$C)+D1").unwrap(), "S", "S", &mv)), "SUM(E:E)+SUM($F:$F)+D1"); let e = parse("Old!A1+'Old'!B2").unwrap(); assert_eq!(print(&rename_sheet(e, "Old", "New Name")), "'New Name'!A1+'New Name'!B2"); } diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 72d33e2..7180d46 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -27,6 +27,11 @@ pub enum Drag { Select, Rows(u32), Cols(u32), + /// Reordering one contiguous whole-column selection by its header. + MoveCols { + source: RangeRef, + before: Option, + }, ResizeCol { col: u32, start: f32, @@ -962,6 +967,10 @@ fn paint_selection(p: &Painter, geo: &Geo, sh: &Sheet, sel: &gridcraft_engine::S let rr = geo.range_rect(sh, moved); p.rect_stroke(rr, 0.0, Stroke::new(2.0, Color32::from_gray(110)), StrokeKind::Middle); } + if let crate::grid::Drag::MoveCols { before: Some(before), .. } = &app.grid.drag { + let x = geo.x(sh, *before); + p.line_segment([pos2(x, geo.rect.top()), pos2(x, geo.rect.bottom())], Stroke::new(3.0, t.accent)); + } // Marching ants around the copied range. if let Some(clip) = &app.session.clipboard && app.session.active().is_some_and(|d| d.uid == clip.doc_uid && d.wb.active_sheet == clip.sheet) @@ -1086,7 +1095,9 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & } else if in_cells { ctx.set_cursor_icon(CursorIcon::Cell); } else if in_col_header { - ctx.set_cursor_icon(CursorIcon::Default); + let col = geo.col_at(sh, p.x); + let selected = sel.ranges.len() == 1 && sel.current().is_full_cols() && sel.current().contains(CellRef::new(0, col)); + ctx.set_cursor_icon(if selected { CursorIcon::Move } else { CursorIcon::Default }); } app.grid.hover_cell = in_cells.then(|| geo.cell_at(sh, p)); } @@ -1202,6 +1213,15 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & return; } let c = geo.col_at(sh, p.x); + if resp.drag_started() + && !mods.any() + && sel.ranges.len() == 1 + && sel.current().is_full_cols() + && sel.current().contains(CellRef::new(0, c)) + { + app.grid.drag = Drag::MoveCols { source: sel.current(), before: None }; + return; + } let range = if mods.shift { RangeRef::cols(sel.anchor.col.min(c), sel.anchor.col.max(c)) } else { RangeRef::cols(c, c) }; select_ranges(app, range, mods.command, CellRef::new(geo.row_at(sh, geo.cells.top() + 1.0), c)); app.grid.drag = Drag::Cols(if mods.shift { sel.anchor.col } else { c }); @@ -1355,6 +1375,14 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & app.session.run("selection.set", json!({"range": RangeRef::cols(anchor.min(c), anchor.max(c)).a1(), "active": sel.active.a1()})); scroll_by(app, vec2(auto.x, 0.0)); } + Drag::MoveCols { source, .. } => { + let col = geo.col_at(sh, p.x.clamp(geo.cells.left() + 1.0, geo.cells.right() - 1.0)); + let midpoint = geo.x(sh, col) + sh.col_width(col) * geo.z * 0.5; + let before = (col + u32::from(p.x >= midpoint)).min(MAX_COLS - 1); + let before = (!(source.start.col..=source.end.col + 1).contains(&before)).then_some(before); + app.grid.drag = Drag::MoveCols { source, before }; + scroll_by(app, vec2(auto.x, 0.0)); + } Drag::ResizeCol { col, start, orig } => { let w = (orig + (p.x - start) / geo.z).max(0.0); app.toast = @@ -1434,6 +1462,11 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & let _ = app.run("edit.paste", json!({"at": to.a1()})); } } + Drag::MoveCols { source, before: Some(before) } => { + if let Err(e) = app.run("sheet.moveColumns", json!({"cols": source.a1(), "before": before})) { + app.message = Some(("GridCraft".into(), crate::clean_error(&e))); + } + } _ => {} } app.grid.drag_select = false; diff --git a/crates/ui-egui/tests/header_column_drag.rs b/crates/ui-egui/tests/header_column_drag.rs new file mode 100644 index 0000000..d28f9ae --- /dev/null +++ b/crates/ui-egui/tests/header_column_drag.rs @@ -0,0 +1,65 @@ +//! Whole-column reordering driven through real egui pointer events. + +use egui::{Event, Modifiers, PointerButton, Pos2, pos2, vec2}; +use gridcraft_engine::{Session, core::CellRef}; +use gridcraft_ui_egui::{SheetApp, grid::Geo}; +use serde_json::json; + +fn harness() -> egui_kittest::Harness<'static, SheetApp> { + let mut session = Session::new(); + session.new_workbook(); + session.execute("range.setValues", json!({"range": "A1", "values": [["A", "B", "C", "D", "E"]]})).unwrap(); + session.execute("selection.set", json!({"range": "B:C"})).unwrap(); + let mut harness = 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()), + ); + harness.run_steps(4); + harness.state_mut().session.active_mut().unwrap().undo.clear(); + harness.state_mut().session.journal.clear(); + harness +} + +fn header_center(harness: &egui_kittest::Harness<'_, SheetApp>, col: u32) -> Pos2 { + let app = harness.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let geo = Geo::new(sheet, app.grid.rect.unwrap(), app.view().scroll); + pos2(geo.x(sheet, col) + sheet.col_width(col) * geo.z * 0.5, geo.rect.top() + geo.header_h * 0.5) +} + +fn pointer_button(harness: &mut egui_kittest::Harness<'_, SheetApp>, pos: Pos2, pressed: bool) { + harness + .input_mut() + .events + .extend([Event::PointerMoved(pos), Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }]); + harness.step(); +} + +#[test] +fn dragging_selected_column_headers_reorders_the_block() { + let mut harness = harness(); + let start = header_center(&harness, 1); + let target = { + let center = header_center(&harness, 4); + pos2(center.x - 20.0, center.y) + }; + + pointer_button(&mut harness, start, true); + harness.input_mut().events.push(Event::PointerMoved(target)); + harness.step(); + harness.input_mut().events.push(Event::PointerMoved(target + vec2(1.0, 0.0))); + harness.step(); + pointer_button(&mut harness, target + vec2(1.0, 0.0), false); + harness.run_steps(2); + + let app = harness.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let values: Vec<_> = (0..5).map(|col| sheet.value(CellRef::new(0, col))).collect(); + assert_eq!(values, vec!["A".into(), "D".into(), "B".into(), "C".into(), "E".into()]); + assert_eq!(app.session.active().unwrap().selection.current().a1(), "C:D"); + assert_eq!(app.session.journal.iter().filter(|(id, _)| id == "sheet.moveColumns").count(), 1); + assert_eq!(app.session.active().unwrap().undo.len(), 1); +} diff --git a/docs/ui-parity.md b/docs/ui-parity.md index e85bca1..4d1feb4 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-10 · **Change:** major (created from a source audit and the issue tracker) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (selected columns can be reordered by dragging their headers) · **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 @@ -38,6 +38,7 @@ Sources: `crates/ui-egui/src` (ribbon, grid, editor, formula bar, dialogs, panes | Point mode (click/drag/arrow references), reference colouring, F4 | done | | AutoComplete in columns, function autocomplete and argument tips | done | | Header resize and double-click AutoFit | done (#42, #88 for header dividers) | +| Reorder selected whole columns by dragging their headers | done (10-11) | | Freeze panes, split panes | freeze done; split to verify | | Copy as HTML table for other apps | done (#36, 10-10) | | Context menu on cells | 16 items (Cut, Copy, Paste, Paste Special, Insert, Delete, Clear Contents, Filter by value, Sort, New Comment, New Note, Format Cells, Pick From Drop-down List, Define Name, Link). Missing: Insert Copied/Cut Cells (#21), paste-option icons, Quick Analysis, Get Data from Table, Show Changes, Smart Lookup | @@ -91,5 +92,6 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-11 | minor | Selected whole columns can be reordered by dragging their headers to an insertion point | | 2026-10-10 | minor | Chart resize bug (#190) | | 2026-10-10 | major | Created: chrome, grid, keyboard, dialogs, accessibility; key tips, Page Layout view and accessibility tree flagged | From 2ded7bf19333492e869442784475083e814c4c00 Mon Sep 17 00:00:00 2001 From: nihilist-narwhal <240798405+nihilist-narwhal@users.noreply.github.com> Date: Sat, 10 Oct 2026 01:39:01 -0500 Subject: [PATCH 11/27] XLSX: write the _xlfn. prefix for GROUPBY, PIVOTBY, ENCODEURL and the REGEX functions These were missing from the list of functions newer than the original file format, so they were saved without the prefix, and Excel shows #NAME? for such formulas. --- crates/xlsx/src/fmla.rs | 2 ++ crates/xlsx/src/tables.rs | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/crates/xlsx/src/fmla.rs b/crates/xlsx/src/fmla.rs index 9ac4ccf..c483414 100644 --- a/crates/xlsx/src/fmla.rs +++ b/crates/xlsx/src/fmla.rs @@ -132,6 +132,8 @@ mod tests { assert_eq!(to_file(&Formula::new("FILTER(A1:A3,B1:B3)")), "_xlfn._xlws.FILTER(A1:A3,B1:B3)"); assert_eq!(from_file("_xlfn.XLOOKUP(1,A:A,B:B)"), "XLOOKUP(1,A:A,B:B)"); assert_eq!(text_to_file("=IFS(A1>0,1)"), "_xlfn.IFS(A1>0,1)"); + assert_eq!(to_file(&Formula::new("REGEXTEST(A1,\"x\")&ENCODEURL(B1)")), "_xlfn.REGEXTEST(A1,\"x\")&_xlfn.ENCODEURL(B1)"); + assert_eq!(to_file(&Formula::new("GROUPBY(A1:A3,B1:B3,\"SUM\")")), "_xlfn.GROUPBY(A1:A3,B1:B3,\"SUM\")"); } #[test] diff --git a/crates/xlsx/src/tables.rs b/crates/xlsx/src/tables.rs index 2d63dec..fe7cbdc 100644 --- a/crates/xlsx/src/tables.rs +++ b/crates/xlsx/src/tables.rs @@ -133,6 +133,7 @@ const FUTURE_FUNCTIONS: &[&str] = &[ "DAYS", "DECIMAL", "DROP", + "ENCODEURL", "ERF.PRECISE", "ERFC.PRECISE", "EXPAND", @@ -157,6 +158,7 @@ const FUTURE_FUNCTIONS: &[&str] = &[ "GAMMA.INV", "GAMMALN.PRECISE", "GAUSS", + "GROUPBY", "HSTACK", "HYPGEOM.DIST", "IFNA", @@ -199,6 +201,7 @@ const FUTURE_FUNCTIONS: &[&str] = &[ "PERCENTRANK.INC", "PERMUTATIONA", "PHI", + "PIVOTBY", "POISSON.DIST", "QUARTILE.EXC", "QUARTILE.INC", @@ -206,6 +209,9 @@ const FUTURE_FUNCTIONS: &[&str] = &[ "RANK.AVG", "RANK.EQ", "REDUCE", + "REGEXEXTRACT", + "REGEXREPLACE", + "REGEXTEST", "RRI", "SCAN", "SEC", From 85ca8f6af57cca475886d6b9179abb4cfb057060 Mon Sep 17 00:00:00 2001 From: nihilist-narwhal <240798405+nihilist-narwhal@users.noreply.github.com> Date: Sat, 10 Oct 2026 02:54:04 -0500 Subject: [PATCH 12/27] INDEX: arrays of row and column numbers pick a value each INDEX took one row and one column number, so INDEX(A1:C3,{1;3},{1,3}) gave a single value and SUM(INDEX(A1:C3,{1,2,3},2)) summed one cell. In Excel, arrays of positions return an array of the values at each (broadcast) position. A position that picks a whole row or column gives #CALC! there, as arrays can't nest. --- crates/calc/src/eval.rs | 57 +++++++++++++++++++++++++++++++++------- crates/calc/src/tests.rs | 18 +++++++++++++ 2 files changed, 66 insertions(+), 9 deletions(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 3d4a743..9aa80fd 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -1085,18 +1085,57 @@ impl<'h> Evaluator<'h> { return err(CellError::Value); } let src = self.eval(&args[0]); - let row = match self.arg_num(args, 1, 0.0) { - Ok(n) => n.trunc() as i64, - Err(e) => return err(e), - }; - let col = match self.arg_num(args, 2, 0.0) { - Ok(n) => n.trunc() as i64, - Err(e) => return err(e), + let mut arg = |i: usize| match args.get(i) { + None | Some(Expr::Missing) => None, + Some(e) => Some(self.value(e)), }; + let (rows, cols) = (arg(1), arg(2)); let area_n = match self.arg_num(args, 3, 1.0) { Ok(n) => n.trunc() as i64, Err(e) => return err(e), }; + let two = args.len() == 2; + let num = |v: &Option| v.as_ref().map_or(Ok(0.0), |v| v.to_number()).map(|n| n.trunc() as i64); + // Arrays of positions pick a value for each: INDEX(A1:C10,{1;2},{1,3}) is 2×2. + if let (Some(Value::Array(_)), _) | (_, Some(Value::Array(_))) = (&rows, &cols) { + let as_array = |v: &Option| match v { + Some(Value::Array(a)) => Arc::clone(a), + Some(v) => Arc::new(Array::scalar(v.clone())), + None => Arc::new(Array::scalar(Value::Number(0.0))), + }; + let (ra, ca) = (as_array(&rows), as_array(&cols)); + let (h, w) = (ra.rows.max(ca.rows), ra.cols.max(ca.cols)); + if (h * w) as u64 > MAX_CELLS { + return err(CellError::Num); + } + let mut out = Vec::with_capacity(h * w); + for r in 0..h { + for c in 0..w { + let pos = (num(&Some(ra.get_broadcast(r, c))), num(&Some(ca.get_broadcast(r, c)))); + let v = match pos { + (Ok(row), Ok(col)) => { + let one = self.index_one(src.clone(), row, col, area_n, two); + match self.deref(one) { + Value::Array(a) if a.rows * a.cols > 1 => Value::Error(CellError::Calc), + v => v.scalar(), + } + } + (Err(e), _) | (_, Err(e)) => Value::Error(e), + }; + out.push(v); + } + } + return Ev::V(Array::new(h, w, out).map(Value::from).unwrap_or(Value::Error(CellError::Value))); + } + let (row, col) = match (num(&rows), num(&cols)) { + (Ok(r), Ok(c)) => (r, c), + (Err(e), _) | (_, Err(e)) => return err(e), + }; + self.index_one(src, row, col, area_n, two) + } + + /// INDEX for one row and column position (0 = the whole column or row). + fn index_one(&mut self, src: Ev, row: i64, col: i64, area_n: i64, two_args: bool) -> Ev { if row < 0 || col < 0 { return err(CellError::Value); } @@ -1105,7 +1144,7 @@ impl<'h> Evaluator<'h> { let Some(a) = areas.get((area_n - 1).max(0) as usize).copied() else { return err(CellError::Ref) }; let r = a.range; // A single row or column with one index picks along it. - let (row, col) = if args.len() == 2 && r.height() == 1 { (1, row) } else { (row, col) }; + 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); } @@ -1114,7 +1153,7 @@ impl<'h> Evaluator<'h> { Ev::R(vec![Area { sheet: a.sheet, range: RangeRef::new(CellRef::new(r0, c0), CellRef::new(r1, c1)) }]) } Ev::V(Value::Array(arr)) => { - let (row, col) = if args.len() == 2 && arr.rows == 1 { (1, row) } else { (row, col) }; + let (row, col) = if two_args && arr.rows == 1 { (1, row) } else { (row, col) }; if row as usize > arr.rows || col as usize > arr.cols { return err(CellError::Ref); } diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 2205a1e..50c4353 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -106,6 +106,24 @@ fn long_chain() { assert_eq!(t.num("A3000"), 3009.0); } +#[test] +fn index_with_arrays_of_positions() { + let mut t = T::new(); + for (i, row) in [[1, 2, 3], [4, 5, 6], [7, 8, 9]].iter().enumerate() { + for (j, v) in row.iter().enumerate() { + t.set(&format!("{}{}", ["A", "B", "C"][j], i + 1), &v.to_string()); + } + } + t.set("E1", "=INDEX(A1:C3,{1;3},{1,3})"); + assert_eq!([t.num("E1"), t.num("F1"), t.num("E2"), t.num("F2")], [1.0, 3.0, 7.0, 9.0]); + t.set("E4", "=SUM(INDEX(A1:C3,{1,2,3},2))"); + assert_eq!(t.num("E4"), 15.0); + t.set("E5", "=INDEX({10,20,30},{3,1})"); + assert_eq!([t.num("E5"), t.num("F5")], [30.0, 10.0]); + t.set("E7", "=INDEX(A1:C3,2,3)"); + assert_eq!(t.num("E7"), 6.0); +} + #[test] fn errors_and_cycles() { let mut t = T::new(); From 683ff6003c3f80975d383d732d80fb08b3ee5521 Mon Sep 17 00:00:00 2001 From: nihilist-narwhal <240798405+nihilist-narwhal@users.noreply.github.com> Date: Sat, 10 Oct 2026 02:31:01 -0500 Subject: [PATCH 13/27] Defined names with relative references A name whose references have no $ was evaluated as if they were absolute. In Excel such references are relative: defined from B2, =Sheet1!A2 means "the cell to the left", and the file stores it relative to A1 (Sheet1!XFD1, wrapping around the sheet's edge). Name formulas with relative references are now moved by the using cell's offset from A1, wrapping at the edges, and formulas.defineName takes relative references as relative to the active cell, as Excel's Name Manager does, and stores them relative to A1. --- crates/calc/src/eval.rs | 7 +++++++ crates/calc/src/tests.rs | 26 +++++++++++++++++++++++ crates/engine/src/cmd/formulas.rs | 34 ++++++++++++++++++++++++++++++- crates/formula/src/adjust.rs | 29 ++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 1 deletion(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 3d4a743..e5d621d 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -249,6 +249,13 @@ impl<'h> Evaluator<'h> { let scope = def.scope.unwrap_or(self.sheet); return match gridcraft_formula::parse(&text) { Ok(expr) => { + // Relative references in a name are relative to A1: used here, they move by + // this cell's offset from it. + let expr = if gridcraft_formula::adjust::has_relative_refs(&expr) { + gridcraft_formula::adjust::shift_relative_wrapping(expr, self.at.row as i64, self.at.col as i64) + } else { + expr + }; let saved = self.sheet; // Unqualified references in a name refer to the scope sheet. self.sheet = scope; diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 2205a1e..bf64b5c 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -261,6 +261,32 @@ fn references_functions() { assert_eq!(t.num("B6"), 105.0); } +#[test] +fn relative_names() { + let mut t = T::new(); + // As files store them: relative to A1. `Left` is the cell to the left, `Above3` the three + // cells above, `Mixed` column A of the same row. + for (name, formula) in [("Left", "Sheet1!XFD1"), ("Above3", "Sheet1!A1048574:A1048576"), ("Mixed", "Sheet1!$A1")] { + t.wb.names.push(gridcraft_model::DefinedName { + name: name.into(), + scope: None, + formula: formula.into(), + comment: String::new(), + hidden: false, + }); + } + t.calc.rebuild(&t.wb); + for r in 1..=5 { + t.set(&format!("A{r}"), &(r * 10).to_string()); + } + t.set("B2", "=Left"); + assert_eq!(t.num("B2"), 20.0); + t.set("B4", "=Left+Mixed"); + assert_eq!(t.num("B4"), 80.0); + t.set("A6", "=SUM(Above3)"); + assert_eq!(t.num("A6"), 120.0); +} + #[test] fn spills() { let mut t = T::new(); diff --git a/crates/engine/src/cmd/formulas.rs b/crates/engine/src/cmd/formulas.rs index db8176b..df48a2c 100644 --- a/crates/engine/src/cmd/formulas.rs +++ b/crates/engine/src/cmd/formulas.rs @@ -253,7 +253,15 @@ fn define_name(s: &mut Session, p: &Json) -> Result { Some(sh) => Some(d.wb.sheet_index(sh).ok_or_else(|| bad("formulas.defineName", "no such sheet"))?), }; let refers = match str_param(p, "refersTo") { - Some(r) => r.trim_start_matches('=').to_string(), + // As in Excel, relative references are relative to the active cell; names keep them + // relative to A1, as files do. + Some(r) => match gridcraft_formula::parse(r.trim_start_matches('=')) { + Ok(e) if gridcraft_formula::adjust::has_relative_refs(&e) => { + let at = d.selection.active; + gridcraft_formula::print(&gridcraft_formula::adjust::shift_relative_wrapping(e, -(at.row as i64), -(at.col as i64))) + } + _ => r.trim_start_matches('=').to_string(), + }, None => { let sh = d.wb.active().ok_or(EngineError::NoDocument)?; let r = d.selection.current(); @@ -564,3 +572,27 @@ fn calc_options(s: &mut Session, p: &Json) -> Result { Ok(json!({"mode": format!("{:?}", cx.wb.calc.mode)})) }) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn names_with_relative_references() { + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [[1], [2], [3]]})).unwrap(); + // Defined from B2, `=Sheet1!A2` is "the cell to the left". + s.execute("selection.set", json!({"range": "B2"})).unwrap(); + let r = s.execute("formulas.defineName", json!({"name": "LeftCell", "refersTo": "=Sheet1!A2"})).unwrap(); + assert_eq!(r["refersTo"], "=Sheet1!XFD1"); + s.execute("cell.set", json!({"cell": "B1", "input": "=LeftCell"})).unwrap(); + s.execute("cell.set", json!({"cell": "B3", "input": "=LeftCell*10"})).unwrap(); + let v = |s: &mut Session, a: &str| s.execute("cell.get", json!({"cell": a})).unwrap()["value"].clone(); + assert_eq!(v(&mut s, "B1"), json!(1.0)); + assert_eq!(v(&mut s, "B3"), json!(30.0)); + // Absolute references are kept as they are. + let r = s.execute("formulas.defineName", json!({"name": "First", "refersTo": "=Sheet1!$A$1"})).unwrap(); + assert_eq!(r["refersTo"], "=Sheet1!$A$1"); + } +} diff --git a/crates/formula/src/adjust.rs b/crates/formula/src/adjust.rs index 7601674..a54eb0d 100644 --- a/crates/formula/src/adjust.rs +++ b/crates/formula/src/adjust.rs @@ -16,6 +16,35 @@ pub fn shift_relative(e: Expr, dr: i64, dc: i64) -> Expr { }) } +/// Defined names: relative parts move by `(dr, dc)`, wrapping around the sheet's edges. Files +/// store a name's relative references as seen from A1 (a name for "the cell to the left" defined +/// in B1 is `Sheet1!XFD1`), and a use in another cell moves them by that cell's offset from A1. +pub fn shift_relative_wrapping(e: Expr, dr: i64, dc: i64) -> Expr { + let wrap = |v: u32, abs: bool, d: i64, max: u32| if abs { v } else { (v as i64 + d).rem_euclid(max as i64) as u32 }; + let anchor = |a: Anchor| Anchor { row: wrap(a.row, a.row_abs, dr, MAX_ROWS), col: wrap(a.col, a.col_abs, dc, MAX_COLS), ..a }; + e.map(&mut |x| match x { + Expr::Ref(r) => { + let kind = match r.kind { + RefKind::Cell(a) => RefKind::Cell(anchor(a)), + RefKind::Range(a, b) => RefKind::Range(anchor(a), anchor(b)), + RefKind::Rows(r0, a0, r1, a1) => RefKind::Rows(wrap(r0, a0, dr, MAX_ROWS), a0, wrap(r1, a1, dr, MAX_ROWS), a1), + RefKind::Cols(c0, a0, c1, a1) => RefKind::Cols(wrap(c0, a0, dc, MAX_COLS), a0, wrap(c1, a1, dc, MAX_COLS), a1), + }; + Expr::Ref(Reference { sheet: r.sheet, kind }) + } + other => other, + }) +} + +/// Whether the formula has a relative (no `$`) row or column anywhere. +pub fn has_relative_refs(e: &Expr) -> bool { + e.references().iter().any(|r| match r.kind { + RefKind::Cell(a) => !a.row_abs || !a.col_abs, + RefKind::Range(a, b) => !a.row_abs || !a.col_abs || !b.row_abs || !b.col_abs, + RefKind::Rows(_, a0, _, a1) | RefKind::Cols(_, a0, _, a1) => !a0 || !a1, + }) +} + fn shift_anchor(a: &Anchor, dr: i64, dc: i64) -> Option { let row = if a.row_abs { a.row as i64 } else { a.row as i64 + dr }; let col = if a.col_abs { a.col as i64 } else { a.col as i64 + dc }; From 686501c7de991ccd3fa830c94281df6ab1047af4 Mon Sep 17 00:00:00 2001 From: nihilist-narwhal <240798405+nihilist-narwhal@users.noreply.github.com> Date: Sat, 10 Oct 2026 04:01:24 -0500 Subject: [PATCH 14/27] Data validation: relative formulas, checked with the value being entered Validation formulas were evaluated as written, at every cell of the range, and against the cell's old value. A Custom rule =A1>10 on A1:A9 rejected 20 typed into A1 (A1 was still empty) and checked A1, not A5, when typing into A5; a Whole number rule "<= B1" on C1:C9 compared C2 with B1. As in Excel, the formulas are now relative to the first cell of the range and see the value being entered. --- crates/engine/src/cmd/data.rs | 35 +++++++++++++++++++++++++++++++++-- crates/engine/src/tests.rs | 13 +++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 32ec866..cb793e7 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -952,10 +952,26 @@ pub fn check_validation(wb: &Workbook, sheet: usize, at: CellRef, input: &str) - return None; } let v = gridcraft_core::parse::parse_input(input, wb.date_system).value; - let num = |f: &str| gridcraft_calc::evaluate(wb, sheet, at, f).to_number().ok(); + // The formulas are written for the first cell of the range and see the value being entered, + // as in Excel (a custom `=A1>10` on A1:A9 checks A5's new value at A5). + let first = dv.ranges.first().map(|r| r.start).unwrap_or(at); + let entered; + let wb = if sh.value(at) == v { + wb + } else { + let mut w = wb.clone(); + if let Some(sh) = w.sheet_mut(sheet) { + let style = sh.style_id(at); + sh.cells.set(at, Cell { value: v.clone(), style, ..Default::default() }); + } + entered = w; + &entered + }; + let eval = |f: &str| gridcraft_calc::evaluate(wb, sheet, at, &formula_at(f, first, at)); + let num = |f: &str| eval(f).to_number().ok(); let ok = match dv.kind { ValidationKind::List => list_items(wb, sheet, dv).iter().any(|x| x.eq_ignore_ascii_case(input)), - ValidationKind::Custom => gridcraft_calc::evaluate(wb, sheet, at, &dv.f1).to_bool().unwrap_or(false), + ValidationKind::Custom => eval(&dv.f1).to_bool().unwrap_or(false), ValidationKind::TextLength => { let n = input.chars().count() as f64; compare_op(dv.op, n, num(&dv.f1), dv.f2.as_deref().and_then(num)) @@ -982,6 +998,21 @@ pub fn check_validation(wb: &Workbook, sheet: usize, at: CellRef, input: &str) - Some((dv.clone(), msg)) } +/// A formula written for `first`, as seen from `at` (relative references shifted). +fn formula_at(f: &str, first: CellRef, at: CellRef) -> String { + if first == at { + return f.to_string(); + } + match gridcraft_formula::parse(f.trim_start_matches('=')) { + Ok(e) => gridcraft_formula::print(&gridcraft_formula::adjust::shift_relative( + e, + at.row as i64 - first.row as i64, + at.col as i64 - first.col as i64, + )), + Err(_) => f.to_string(), + } +} + fn compare_op(op: CfOperator, n: f64, a: Option, b: Option) -> bool { let Some(a) = a else { return false }; match op { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 575cc77..0322095 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1152,6 +1152,19 @@ fn ods_import_never_reuses_source_as_save_target() { std::fs::remove_dir_all(dir).unwrap(); } +#[test] +fn validation_formulas_are_relative_and_see_the_new_value() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "B1", "values": [[1], [100]]})).unwrap(); + s.execute("data.validation", json!({"range": "A1:A2", "type": "custom", "formula1": "=A1>10"})).unwrap(); + s.execute("data.validation", json!({"range": "C1:C2", "type": "whole", "operator": "lessOrEqual", "formula1": "=B1"})).unwrap(); + let ok = |s: &mut Session, cell: &str, input: &str| s.execute("data.validate", json!({"cell": cell, "input": input})).unwrap()["ok"] == true; + assert!(ok(&mut s, "A1", "20")); + assert!(!ok(&mut s, "A2", "5")); + assert!(ok(&mut s, "C2", "50")); + assert!(!ok(&mut s, "C1", "50")); +} + #[test] fn samples_build() { for (name, _) in crate::sample::SAMPLES { From 8aa9cad7039f44fa813555cb8c573e3d542c3121 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 11 Oct 2026 14:58:29 -0400 Subject: [PATCH 15/27] Slicers: the anchor uses #206's checked cell_param and stays on the sheet; drop the personal macOS workflow (review fix-up for #193) Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/slicer.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/slicer.rs b/crates/engine/src/cmd/slicer.rs index 907ac7e..00342a5 100644 --- a/crates/engine/src/cmd/slicer.rs +++ b/crates/engine/src/cmd/slicer.rs @@ -119,7 +119,8 @@ fn insert_slicer(s: &mut Session, p: &Json) -> Result { } }; let table = t.name.clone(); - let at = cell_param(p, "at").unwrap_or_else(|| CellRef::new(t.range.start.row, t.range.end.col.saturating_add(2))); + let at = cell_param(s, p, "at")? + .unwrap_or_else(|| CellRef::new(t.range.start.row, t.range.end.col.saturating_add(2).min(gridcraft_core::MAX_COLS - 1))); let columns = u32_param(p, "columns").unwrap_or(1).clamp(1, 10); let width = f64_param(p, "width").unwrap_or(168.0).clamp(60.0, 10000.0) as f32; let height = f64_param(p, "height").unwrap_or(200.0).clamp(60.0, 10000.0) as f32; From 7a4aaebab0f60b3b5511aa14d1ca49743db676dc Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 20:58:49 +0200 Subject: [PATCH 16/27] Grid: merged ranges draw their right and bottom borders from the edge cells (#376) Excel stores a merged range's outline on the cells along each edge; the grid only used the anchor cell's borders, so a boxed merge showed only top and left. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 1 + crates/ui-egui/src/grid.rs | 62 +++++++++++++++++++++++++++++---- crates/ui-egui/tests/borders.rs | 33 ++++++++++++++++++ 3 files changed, 89 insertions(+), 7 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index a62418e..aef29c1 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Merged cells draw their whole outline: right and bottom edges come from the last column's and last row's cells (#376) | | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) | | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) | | 2026-10-11 | Data › Forecast › Forecast Sheet: a new sheet with the history, FORECAST.ETS forecast, confidence bounds, statistics and a chart (#322) | diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index f6dc683..468b406 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -8,7 +8,7 @@ use std::sync::Arc; use egui::{Align2, Color32, CursorIcon, FontId, Painter, Pos2, Rect, Sense, Stroke, StrokeKind, pos2, vec2}; use gridcraft_engine::cf::{CfCache, CfLook}; use gridcraft_engine::core::{CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value, col_to_letters}; -use gridcraft_engine::model::{BorderStyle, Color, HAlign, LineIndex, PatternType, Sheet, Style, Underline, VAlign, Workbook}; +use gridcraft_engine::model::{BorderLine, BorderStyle, Color, HAlign, LineIndex, PatternType, Sheet, Style, Underline, VAlign, Workbook}; use serde_json::json; use crate::SheetApp; @@ -1037,12 +1037,19 @@ pub(crate) fn paint_quadrant( // Borders (over fills and text). for (c, rect, lk, _) in &looks { let b = &lk.style.borders; - let edges = [ - (b.top, [rect.left_top(), rect.right_top()]), - (b.bottom, [rect.left_bottom(), rect.right_bottom()]), - (b.left, [rect.left_top(), rect.left_bottom()]), - (b.right, [rect.right_top(), rect.right_bottom()]), - ]; + let merge = merges.iter().find(|m| m.start == *c); + let edges = match merge { + // A merged area's outline lives on the cells along each of its edges (Excel writes the + // right edge on the last column's cells and the bottom edge on the last row's), so each + // edge is drawn cell by cell from the cell it belongs to. + Some(m) => merged_edges(wb, si, sh, geo, *m, *rect, (r0, r1), (c0, c1), cf), + None => vec![ + (b.top, [rect.left_top(), rect.right_top()]), + (b.bottom, [rect.left_bottom(), rect.right_bottom()]), + (b.left, [rect.left_top(), rect.left_bottom()]), + (b.right, [rect.right_top(), rect.right_bottom()]), + ], + }; for (line, pts) in edges { if line.is_none() { continue; @@ -1113,6 +1120,47 @@ pub fn filter_button_rect(geo: &Geo, sh: &Sheet, c: CellRef) -> Rect { Rect::from_min_size(pos2(rect.right() - s - 2.0, rect.bottom() - s - 2.0), vec2(s, s)) } +/// The outline segments of merged area `m` drawn at `rect`: the top and left edges come from the +/// first row's and first column's cells, the bottom and right edges from the last row's and last +/// column's cells, one segment per visible cell (limited to the visible `rows` × `cols`). +#[allow(clippy::too_many_arguments)] +fn merged_edges( + wb: &Workbook, + si: usize, + sh: &Sheet, + geo: &Geo, + m: RangeRef, + rect: Rect, + rows: (u32, u32), + cols: (u32, u32), + cf: &mut CfCache, +) -> Vec<(BorderLine, [Pos2; 2])> { + let mut out = Vec::new(); + let (cs, ce) = (m.start.col.max(cols.0), m.end.col.min(cols.1)); + for col in cs..=ce { + if sh.col_width(col) == 0.0 { + continue; + } + let (x0, x1) = (geo.x(sh, col), geo.x(sh, col.saturating_add(1))); + let top = look(wb, si, sh, CellRef::new(m.start.row, col), cf).0.style.borders.top; + let bottom = look(wb, si, sh, CellRef::new(m.end.row, col), cf).0.style.borders.bottom; + out.push((top, [pos2(x0, rect.top()), pos2(x1, rect.top())])); + out.push((bottom, [pos2(x0, rect.bottom()), pos2(x1, rect.bottom())])); + } + let (rs, re) = (m.start.row.max(rows.0), m.end.row.min(rows.1)); + for row in rs..=re { + if sh.row_height(row) == 0.0 { + continue; + } + let (y0, y1) = (geo.y(sh, row), geo.y(sh, row.saturating_add(1))); + let left = look(wb, si, sh, CellRef::new(row, m.start.col), cf).0.style.borders.left; + let right = look(wb, si, sh, CellRef::new(row, m.end.col), cf).0.style.borders.right; + out.push((left, [pos2(rect.left(), y0), pos2(rect.left(), y1)])); + out.push((right, [pos2(rect.right(), y0), pos2(rect.right(), y1)])); + } + out +} + pub(crate) fn paint_border(p: &Painter, pts: [Pos2; 2], style: BorderStyle, col: Color32) { let w = style.width(); let (a, b) = (pts[0], pts[1]); diff --git a/crates/ui-egui/tests/borders.rs b/crates/ui-egui/tests/borders.rs index af8a1a9..f248fc8 100644 --- a/crates/ui-egui/tests/borders.rs +++ b/crates/ui-egui/tests/borders.rs @@ -127,3 +127,36 @@ fn border_preview_uses_the_colored_draft_until_ok_and_cancel_discards_it() { h.state_mut().run("edit.undo", json!({})).unwrap(); assert_eq!(borders(&h, "A1"), original); } + +/// #376: a merged range's outline lives on the cells along each edge (the right edge on the last +/// column's cells, the bottom on the last row's), not on the anchor alone. All four sides are drawn. +#[test] +fn merged_range_draws_the_outline_from_its_edge_cells() { + let mut h = harness(); + h.state_mut().run("home.borders", json!({"range": "B2:E5", "preset": "thickOutside", "color": "#C81E1E"})).unwrap(); + h.state_mut().run("home.mergeCells", json!({"range": "B2:E5"})).unwrap(); + h.state_mut().run("selection.set", json!({"range": "H10"})).unwrap(); + h.run_steps(6); + assert!(borders(&h, "B2").right.is_none(), "the anchor carries only the top and left edges"); + let red = Color32::from_rgb(0xC8, 0x1E, 0x1E); + let segs: Vec<[egui::Pos2; 2]> = h + .output() + .shapes + .iter() + .filter_map(|shape| match &shape.shape { + Shape::LineSegment { points, stroke } if stroke.color == red => Some(*points), + _ => None, + }) + .collect(); + let vertical = |s: &[egui::Pos2; 2]| (s[0].x - s[1].x).abs() < 0.5; + let xs: Vec = segs.iter().filter(|s| vertical(s)).map(|s| s[0].x).collect(); + let ys: Vec = segs.iter().filter(|s| !vertical(s)).map(|s| s[0].y).collect(); + let (min_x, max_x) = xs.iter().fold((f32::MAX, f32::MIN), |(a, b), x| (a.min(*x), b.max(*x))); + let (min_y, max_y) = ys.iter().fold((f32::MAX, f32::MIN), |(a, b), y| (a.min(*y), b.max(*y))); + // Left and right edges (four rows each), top and bottom edges (four columns each). + assert_eq!(xs.iter().filter(|x| (**x - min_x).abs() < 0.5).count(), 4, "left edge"); + assert_eq!(xs.iter().filter(|x| (**x - max_x).abs() < 0.5).count(), 4, "right edge"); + assert_eq!(ys.iter().filter(|y| (**y - min_y).abs() < 0.5).count(), 4, "top edge"); + assert_eq!(ys.iter().filter(|y| (**y - max_y).abs() < 0.5).count(), 4, "bottom edge"); + assert!(max_x - min_x > 100.0 && max_y - min_y > 40.0, "the outline spans the whole merged area"); +} From 347d2a5bc2e8d8f89c093e20fbbed4b67d5ac28f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 11 Oct 2026 14:59:45 -0400 Subject: [PATCH 17/27] Docs: catalog 274 / 290 after #211 and #193 Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 ++-- docs/parity-checklist.md | 17 +++++++---------- docs/target-app-parity.md | 4 ++-- docs/ui-parity.md | 2 +- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index a62418e..03830fb 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-11 · **Change:** minor (community merges: worksheet functions 513 / 523, catalog 272 / 290) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-11 · **Change:** minor (community merges: worksheet functions 513 / 523, catalog 274 / 290) · **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. @@ -21,7 +21,7 @@ This page is the summary. The detail is in: | Number | Value | Kind | |---|---|---| -| **Feature breadth** (weighted) | **~75%** | estimated, anchored on two measurements: ribbon catalog 272 / 290 (93.8%), worksheet functions 513 / 523 (98.1%; 504 working, 96.4%) | +| **Feature breadth** (weighted) | **~75%** | estimated, anchored on two measurements: ribbon catalog 274 / 290 (94.5%), worksheet functions 513 / 523 (98.1%; 504 working, 96.4%) | | **Ready for real work** | **~50%** | estimated from written weights (51.6); [target-app-parity.md](docs/target-app-parity.md) | | **Mainstream practitioner** | **~50%** | estimated: typical analyst/finance user, weekly features, × 0.90 interaction × 0.92 stability × 0.88 file exchange; [method](docs/target-app-parity.md#mainstream-practitioner) | | **Essentials user** | **~63%** | estimated: core features only, × 0.92 launch/stability × 0.95 clarity × 0.90 opening sent files; [method](docs/target-app-parity.md#essentials-user) | diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 3fa0ef0..d218c81 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -4,13 +4,13 @@ Each entry is an Excel ribbon or menu command (feature names only). It counts as implemented when GridCraft registers a command with the same id. This measures breadth only; the overall assessment (depth, fidelity, effort) is in [target-app-parity.md](target-app-parity.md). -**Overall: 272 / 290 commands implemented (93.8%).** +**Overall: 274 / 290 commands implemented (94.5%).** | Tab | Implemented | Total | Parity | |---|---|---|---| | File | 11 | 11 | 100% | | Home | 94 | 94 | 100% | -| Insert | 16 | 25 | 64% | +| Insert | 17 | 25 | 68% | | Draw | 4 | 4 | 100% | | Page Layout | 23 | 23 | 100% | | Formulas | 23 | 23 | 100% | @@ -18,7 +18,7 @@ Each entry is an Excel ribbon or menu command (feature names only). It counts as | Review | 15 | 18 | 83% | | View | 20 | 20 | 100% | | Automate | 3 | 3 | 100% | -| Table Design | 12 | 13 | 92% | +| Table Design | 13 | 13 | 100% | | Chart Design | 8 | 8 | 100% | | Format | 3 | 3 | 100% | | PivotTable Analyze | 13 | 13 | 100% | @@ -139,7 +139,7 @@ Implemented: - `edit.goToSpecial` Go To Special (Editing) - `home.addIns` Add-ins (Add-ins) -## Insert (16 / 25, 64%) +## Insert (17 / 25, 68%) Implemented: @@ -153,6 +153,7 @@ Implemented: - `insert.chart` Charts (Charts) - `insert.pivotChart` PivotChart (Charts) - `insert.sparkline` Sparklines (Sparklines) +- `insert.slicer` Slicer (Filters) - `insert.link` Link (Links) - `review.newComment` Comment (Comments) - `insert.textBox` Text Box (Text) @@ -165,7 +166,6 @@ Missing: - `insert.smartArt` SmartArt (Illustrations) - `insert.screenshot` Screenshot (Illustrations) - `insert.map` Maps (Charts) -- `insert.slicer` Slicer (Filters) - `insert.timeline` Timeline (Filters) - `insert.wordArt` WordArt (Text) - `insert.signatureLine` Signature Line (Text) @@ -332,7 +332,7 @@ Implemented: - `automate.allScripts` All Scripts (Scripting Tools) - `automate.recordActions` Record Actions (Scripting Tools) -## Table Design (12 / 13, 92%) +## Table Design (13 / 13, 100%) Implemented: @@ -341,6 +341,7 @@ Implemented: - `table.summarizePivot` Summarize with PivotTable (Tools) - `table.removeDuplicates` Remove Duplicates (Tools) - `table.convertToRange` Convert to Range (Tools) +- `table.insertSlicer` Insert Slicer (Tools) - `table.headerRow` Header Row (Table Style Options) - `table.totalRow` Total Row (Table Style Options) - `table.bandedRows` Banded Rows (Table Style Options) @@ -349,10 +350,6 @@ Implemented: - `table.bandedColumns` Banded Columns (Table Style Options) - `table.style` Table Styles (Table Styles) -Missing: - -- `table.insertSlicer` Insert Slicer (Tools) - ## Chart Design (8 / 8, 100%) Implemented: diff --git a/docs/target-app-parity.md b/docs/target-app-parity.md index f68d716..8a00f5a 100644 --- a/docs/target-app-parity.md +++ b/docs/target-app-parity.md @@ -1,6 +1,6 @@ # Target-app parity: GridCraft vs Microsoft Excel -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-11 · **Change:** minor (community merges: worksheet functions 513 / 523, catalog 272 / 290) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-11 · **Change:** minor (community merges: worksheet functions 513 / 523, catalog 274 / 290) · **Target:** Microsoft Excel (Microsoft 365) This is the authoritative parity assessment. [`ROADMAP.md`](../ROADMAP.md) summarizes it, [`gaps.md`](gaps.md) lists every shortfall, and the area checklists hold the detail: @@ -13,7 +13,7 @@ generated ribbon checklist is [`parity-checklist.md`](parity-checklist.md). | Number | Value | Kind | |---|---|---| | Feature breadth (weighted by use) | **~75%** | estimated from the area table below; anchored on two measurements | -| Ribbon and menu catalog | **272 / 290 (93.8%)** | measured: `cargo xtask parity` (`crates/engine/src/catalog.rs` ∩ registered command ids) | +| Ribbon and menu catalog | **274 / 290 (94.5%)** | measured: `cargo xtask parity` (`crates/engine/src/catalog.rs` ∩ registered command ids) | | Worksheet functions, by name | **513 / 523 (98.1%)** | measured: registered functions vs Microsoft's published function list (method below) | | Worksheet functions that do real work | **504 / 523 (96.4%)** | measured: excludes 9 stubs (7 CUBE functions, WEBSERVICE, FILTERXML) | | **Ready for real work** | **~50%** | estimated; written weights below (51.6 computed) | diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 152884c..fcea332 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -4,7 +4,7 @@ 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 -separately in [parity-checklist.md](parity-checklist.md) (272 / 290). +separately in [parity-checklist.md](parity-checklist.md) (274 / 290). Sources: `crates/ui-egui/src` (ribbon, grid, editor, formula bar, dialogs, panes), `crates/engine/src/cmd` (shortcuts in command specs), the owner's black-box notes From 3ae319154cb653341d30340d2d30d257380edb8f Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 21:01:12 +0200 Subject: [PATCH 18/27] Grid: render Shrink to fit (#375) Cells with shrinkToFit drew at full size: text spilled or was clipped and numbers showed ####. Too-wide content now gets a smaller font that fits the cell or merged area, matching the PDF/print path. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 1 + crates/ui-egui/src/grid.rs | 22 +++++++++- crates/ui-egui/tests/shrink_to_fit.rs | 61 +++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 crates/ui-egui/tests/shrink_to_fit.rs diff --git a/ROADMAP.md b/ROADMAP.md index a62418e..d642931 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Shrink to fit draws too-wide text and numbers in a smaller font inside the cell or merged area (#375) | | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) | | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) | | 2026-10-11 | Data › Forecast › Forecast Sheet: a new sheet with the history, FORECAST.ETS forecast, confidence bounds, statistics and a chart (#322) | diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index f6dc683..2b018fe 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -934,10 +934,17 @@ pub(crate) fn paint_quadrant( continue; } let first_line = text.lines().next().unwrap_or("").to_string(); + // Shrink to fit: too-wide content is drawn in a smaller font so it fits the cell (or the + // merged area): it neither spills, nor is clipped, nor turns into ####. + let small = (st.align.shrink && !rotated && st.align.h != HAlign::Fill && fill_char.is_none()) + .then(|| shrink_font(p, &first_line, &font, avail_w - 2.0 * pad - indent)) + .flatten(); + let shrunk = small.is_some(); + let font = small.unwrap_or(font); let mut galley = p.layout_no_wrap(first_line.clone(), font.clone(), color); let mut w = galley.size().x; // Numbers and dates never overflow: they show #### when too narrow. - if (numeric || matches!(v, Value::Number(_))) && w > avail_w - 2.0 * pad && st.align.h != HAlign::Fill { + if !shrunk && (numeric || matches!(v, Value::Number(_))) && w > avail_w - 2.0 * pad && st.align.h != HAlign::Fill { let n = ((avail_w - 2.0 * pad) / char_w).floor().max(1.0) as usize; galley = p.layout_no_wrap("#".repeat(n), font.clone(), color); w = galley.size().x; @@ -1113,6 +1120,19 @@ pub fn filter_button_rect(geo: &Geo, sh: &Sheet, c: CellRef) -> Rect { Rect::from_min_size(pos2(rect.right() - s - 2.0, rect.bottom() - s - 2.0), vec2(s, s)) } +/// The smaller font that makes `text` fit in `room` points of width (Shrink to fit), or `None` +/// when it already fits at its own size. +fn shrink_font(p: &Painter, text: &str, font: &FontId, room: f32) -> Option { + let natural = p.layout_no_wrap(text.to_string(), font.clone(), Color32::PLACEHOLDER).size().x; + let room = room.max(1.0); + if natural.is_nan() || natural <= room { + return None; + } + // Glyph advances don't scale exactly linearly (rounding), so leave a little slack. + let size = (font.size * room / natural * 0.97).max(1.0); + Some(FontId::new(size, font.family.clone())) +} + pub(crate) fn paint_border(p: &Painter, pts: [Pos2; 2], style: BorderStyle, col: Color32) { let w = style.width(); let (a, b) = (pts[0], pts[1]); diff --git a/crates/ui-egui/tests/shrink_to_fit.rs b/crates/ui-egui/tests/shrink_to_fit.rs new file mode 100644 index 0000000..a9428fa --- /dev/null +++ b/crates/ui-egui/tests/shrink_to_fit.rs @@ -0,0 +1,61 @@ +//! Shrink to fit (#375): too-wide content is drawn in a smaller font inside its cell. + +use egui::Shape; +use gridcraft_engine::Session; +use gridcraft_engine::core::CellRef; +use gridcraft_ui_egui::{SheetApp, grid::Geo}; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +fn harness() -> Harness { + let mut s = Session::new(); + s.new_workbook(); + let shrink = json!({"align": {"shrink": true}}); + s.execute("cell.set", json!({"cell": "A1", "input": "Executive Producer"})).unwrap(); + s.execute("home.formatCells", json!({"range": "A1", "style": shrink})).unwrap(); + s.execute("cell.set", json!({"cell": "A2", "input": "Control Producer"})).unwrap(); + s.execute("cell.set", json!({"cell": "A4", "input": "123456789012"})).unwrap(); + s.execute("home.formatCells", json!({"range": "A4", "style": {"num_fmt": "#,##0", "align": {"shrink": true}}})).unwrap(); + s.execute("cell.set", json!({"cell": "A5", "input": "123456789012"})).unwrap(); + s.execute("home.formatCells", json!({"range": "A5", "style": {"num_fmt": "#,##0"}})).unwrap(); + s.execute("selection.set", json!({"range": "H20"})).unwrap(); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(ui.ctx()); + app.ui(ui); + }, + SheetApp::new(s, Default::default()), + ); + h.run_steps(4); + h +} + +/// The painted text shape whose text is `text`: (font size, width, left x). +fn text_shape(h: &Harness, text: &str) -> Option<(f32, f32, f32)> { + h.output().shapes.iter().find_map(|s| match &s.shape { + Shape::Text(t) if t.galley.job.text == text => { + let size = t.galley.job.sections.first().map(|s| s.format.font_id.size)?; + Some((size, t.galley.size().x, t.pos.x)) + } + _ => None, + }) +} + +#[test] +fn shrink_to_fit_text_and_numbers_fit_their_cell() { + let h = harness(); + 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 a1 = geo.cell_rect(sheet, CellRef::parse("A1").unwrap()); + let (small, w, x) = text_shape(&h, "Executive Producer").expect("A1 is drawn"); + let (normal, cw, _) = text_shape(&h, "Control Producer").expect("A2 is drawn"); + assert!(small < normal, "A1 is drawn smaller ({small} vs {normal})"); + assert!(cw > a1.width(), "the control is wider than the column"); + assert!(x >= a1.left() && x + w <= a1.right(), "A1 fits inside column A"); + // A number shrinks instead of turning into ####; its unshrunk twin still shows ####. + let (num, nw, _) = text_shape(&h, "123,456,789,012").expect("A4 shows the number"); + assert!(num < normal && nw <= a1.width()); + assert!(h.output().shapes.iter().any(|s| matches!(&s.shape, Shape::Text(t) if t.galley.job.text.starts_with("##")))); +} From 806a39a5142904d7640b32c53b6ef333bd84c15a Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 21:02:20 +0200 Subject: [PATCH 19/27] Windows: compile DirectX 12 shaders with FXC, never a stray dxcompiler.dll (#339, #354) wgpu's default DX12 compiler (Auto) loads the first dxcompiler.dll on the DLL search path. GridCraft ships none, so another program's copy is used; without a matching dxil.dll its 'DXIL signing library not found' warning fails wgpu's internal compute pipelines, the device is lost and the app exits before the first frame. Pin FXC (part of Windows) unless WGPU_DX12_COMPILER asks for another compiler. Co-Authored-By: Claude Opus 5.5 --- README.md | 5 ++++ ROADMAP.md | 1 + apps/gridcraft/src/graphics.rs | 44 ++++++++++++++++++++++++++++++++-- docs/hardware-parity.md | 5 ++-- 4 files changed, 51 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 164c270..678495f 100644 --- a/README.md +++ b/README.md @@ -242,6 +242,11 @@ the first renderer fails to start, it tries the other one once. The log file rec (lines starting with `graphics:`). `WGPU_BACKEND` still overrides it (and then nothing else is tried), e.g. `$env:WGPU_BACKEND = "gl"` or `"dx12"`. +DirectX 12 shaders are compiled with FXC, which is part of Windows, so no `dxcompiler.dll` or +`dxil.dll` is needed, and a copy another program put on `PATH` can no longer stop the app at start +("Parent device is lost", #339). `$env:WGPU_DX12_COMPILER = "dxc"` (or `"auto"`) selects wgpu's DXC +handling again for troubleshooting. + ### macOS | Build | File | Notes | diff --git a/ROADMAP.md b/ROADMAP.md index a62418e..9589334 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Windows: DirectX 12 shaders compile with FXC, so a missing or stray `dxcompiler.dll` no longer stops the app at start (#339, #354) | | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) | | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) | | 2026-10-11 | Data › Forecast › Forecast Sheet: a new sheet with the history, FORECAST.ETS forecast, confidence bounds, statistics and a chart (#322) | diff --git a/apps/gridcraft/src/graphics.rs b/apps/gridcraft/src/graphics.rs index 14344b4..83478ed 100644 --- a/apps/gridcraft/src/graphics.rs +++ b/apps/gridcraft/src/graphics.rs @@ -3,11 +3,15 @@ //! Vulkan is never initialized unless `WGPU_BACKEND` asks for it: enumerating Vulkan adapters //! crashes inside Intel's `igvk64.dll` on some UHD Graphics drivers (#120), and that access //! violation happens in the driver, where no Rust error handling can catch it. +//! +//! DirectX 12 shaders are compiled with FXC (`d3dcompiler_47.dll`, part of Windows), never with +//! whichever `dxcompiler.dll` the DLL search path happens to find (#339, #354): see +//! [`dx12_compiler`]. use eframe::{ NativeOptions, egui_wgpu::WgpuSetup, - wgpu::{Backends, DeviceType}, + wgpu::{Backends, DeviceType, Dx12Compiler}, }; /// What the start-up probe learned about one DirectX 12 adapter. @@ -59,9 +63,24 @@ pub fn fallback_backends(backend_override: Option, first: Backends) -> (!rest.is_empty()).then_some(rest) } -/// Restricts the wgpu instance eframe creates to `backends`. +/// The DirectX 12 shader compiler: `env_choice` (normally `Dx12Compiler::from_env()`, i.e. +/// `WGPU_DX12_COMPILER`, for troubleshooting) or else FXC. +/// +/// wgpu's default (`Auto`) loads the first `dxcompiler.dll` on the DLL search path. GridCraft ships +/// none, so that is another program's copy (a Windows SDK, an Edge or WebView folder, anything on +/// `PATH`). Without a matching `dxil.dll` beside it, DXC prints "DXIL signing library not found", +/// wgpu treats that warning as a compile error, its internal compute pipelines fail, the device is +/// lost and the app quits before the first frame ("Parent device is lost"). FXC is part of Windows +/// and needs no extra DLL; egui's shaders don't need anything newer. +pub fn dx12_compiler(env_choice: Option) -> Dx12Compiler { + env_choice.unwrap_or(Dx12Compiler::Fxc) +} + +/// Restricts the wgpu instance eframe creates to `backends`, and pins its DirectX 12 shader +/// compiler ([`dx12_compiler`]). pub fn configure_backends(options: &mut NativeOptions, backends: Backends) { if let WgpuSetup::CreateNew(create) = &mut options.wgpu_options.wgpu_setup { + create.instance_descriptor.backend_options.dx12.shader_compiler = dx12_compiler(Dx12Compiler::from_env()); // eframe's default includes Vulkan and GL. Creating the GL backend can crash inside AMD's // atio6axx.dll, and enumerating Vulkan adapters inside Intel's igvk64.dll (#120), before // adapter selection or any Rust error handling runs, so the window never appears. @@ -189,4 +208,25 @@ mod tests { // Both were already tried: nothing left to fall back to. assert_eq!(fallback_backends(None, Backends::DX12 | Backends::GL), None); } + + #[test] + fn dx12_compiles_shaders_with_fxc_unless_asked() { + // #339, #354: wgpu's `Auto` would load a stray dxcompiler.dll and lose the device. + assert!(matches!(dx12_compiler(None), Dx12Compiler::Fxc)); + assert!(matches!(dx12_compiler(Some(Dx12Compiler::Auto)), Dx12Compiler::Auto)); + assert!(matches!(dx12_compiler(Some(Dx12Compiler::default_dynamic_dxc())), Dx12Compiler::DynamicDxc { .. })); + // eframe's own default is `Auto`; the configured instance (first start and retry) uses FXC. + let WgpuSetup::CreateNew(eframe_default) = NativeOptions::default().wgpu_options.wgpu_setup else { + panic!("default native options must create a wgpu instance"); + }; + if Dx12Compiler::from_env().is_none() { + assert!(matches!(eframe_default.instance_descriptor.backend_options.dx12.shader_compiler, Dx12Compiler::Auto)); + let mut options = NativeOptions::default(); + configure_backends(&mut options, Backends::DX12); + let WgpuSetup::CreateNew(create) = options.wgpu_options.wgpu_setup else { + panic!("default native options must create a wgpu instance"); + }; + assert!(matches!(create.instance_descriptor.backend_options.dx12.shader_compiler, Dx12Compiler::Fxc)); + } + } } diff --git a/docs/hardware-parity.md b/docs/hardware-parity.md index f8ea38f..9d2c5e7 100644 --- a/docs/hardware-parity.md +++ b/docs/hardware-parity.md @@ -1,6 +1,6 @@ # Hardware parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** trivial (Windows renderer fallback, #120) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** trivial (Windows DirectX 12 shaders with FXC, #339) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 The hardware Excel uses, per platform, against GridCraft. Part of [target-app-parity.md](target-app-parity.md). **Hardware: ~40% ready, 25–40 h** (estimated). @@ -8,7 +8,7 @@ The hardware Excel uses, per platform, against GridCraft. Part of | Hardware feature | Excel | GridCraft (macOS · Windows · Linux · BSD · web) | Gap and estimate | |---|---|---|---| | Multi-core recalculation | Multi-threaded recalculation on all cores (Windows and Mac), configurable thread count | Single-threaded everywhere (no threading in `crates/calc`) | Parallel recalc of independent dependency chains on native builds; WASM stays single-threaded or uses workers later: 12–18 h | -| GPU rendering of the grid and charts | DirectX / Metal accelerated drawing | wgpu: Metal · DirectX 12 (default since 10-08; OpenGL when DirectX 12 has no GPU driver or fails to start; Vulkan only on request, so the Intel UHD `igvk64.dll` crash (#120) can't happen) · Vulkan/GL · Vulkan/GL · WebGPU or WebGL2 | At parity in kind. Driver problems: screen flashing on Windows 11 (#76): 2–4 h | +| GPU rendering of the grid and charts | DirectX / Metal accelerated drawing | wgpu: Metal · DirectX 12 (default since 10-08; OpenGL when DirectX 12 has no GPU driver or fails to start; Vulkan only on request, so the Intel UHD `igvk64.dll` crash (#120) can't happen; shaders compiled with FXC, so no DXC runtime is needed, #339) · Vulkan/GL · Vulkan/GL · WebGPU or WebGL2 | At parity in kind. Driver problems: screen flashing on Windows 11 (#76): 2–4 h | | Pen and touch (Draw tab) | Pen pressure and tilt, eraser end, touch mode, Ink to Shape, ink replay | Pen, highlighter, eraser, Ink to Shape with mouse/pen input; **no pressure or tilt**, no touch mode | Pressure-sensitive strokes where winit reports force; touch-friendly ribbon spacing: 4–6 h | | Trackpad and mouse | Smooth scrolling, pinch zoom, horizontal scroll, Magic Mouse | Smooth scrolling and zoom via egui | Verify pinch zoom on each platform: 1–2 h | | Multiple windows and monitors | One window per workbook, New Window for the same workbook, per-monitor DPI | One window; multiple workbooks switch inside it (#162; PR #171) | Native window per workbook, New Window, Arrange All across monitors: 6–8 h | @@ -20,5 +20,6 @@ The hardware Excel uses, per platform, against GridCraft. Part of | Date | Change | Summary | |---|---|---| +| 2026-10-11 | trivial | Windows: DirectX 12 shaders compiled with FXC, no DXC runtime needed (#339, #354) | | 2026-10-11 | trivial | Windows: OpenGL fallback when DirectX 12 has no GPU or fails to start; Vulkan never by default (#120) | | 2026-10-10 | major | Created: single-threaded calc, wgpu rendering, no pen pressure, single window | From 04fb60ea5d9d53e45e739cd816642e53dc24a80b Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 11 Oct 2026 15:04:09 -0400 Subject: [PATCH 20/27] Printing from the UI (Backstage, Ctrl+P, menu) goes through print_now, so the web build opens the browser's print dialog (review fix-up for #209) Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/lib.rs | 16 +++++++--------- crates/ui-egui/src/ribbon.rs | 4 ++-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 0213fc8..7ff4ff7 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -332,6 +332,10 @@ impl SheetApp { /// Runs a command and shows its error in a message box (for menu/ribbon clicks). pub fn run_or_alert(&mut self, id: &str, params: Json) { + // Printing from the UI also hands the PDF to the host's print flow (web). + if id == "file.print" { + return self.print_now(params); + } if let Err(e) = self.run(id, params) { self.alert(e); } @@ -350,8 +354,8 @@ impl SheetApp { /// build) hands the bytes straight to the browser's print flow; errors show the usual /// alert. Desktop builds have no `services.print`, so this falls back to `run_or_alert` /// (which still produces the PDF — just without a UI to act on the result yet). - pub fn print_now(&mut self) { - match self.run("file.print", json!({})) { + pub fn print_now(&mut self, params: Json) { + match self.run("file.print", params) { Ok(r) => { let Some(b64) = r.get("base64").and_then(Json::as_str) else { return }; let Some(bytes) = gridcraft_engine::io::base64_decode(b64) else { @@ -362,13 +366,7 @@ impl SheetApp { print(&bytes); } } - Err(e) => { - if e.contains("is not available right now") { - self.toast = Some((e, now_ms())); - } else { - self.message = Some(("GridCraft".into(), clean_error(&e))); - } - } + Err(e) => self.alert(e), } } diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index fe32ee7..f91c8d3 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -106,7 +106,7 @@ pub fn title_bar(app: &mut SheetApp, ui: &mut Ui) { } } if ui.button("Print…").clicked() { - app.print_now(); + app.print_now(json!({})); } }); // Centered title. @@ -1994,7 +1994,7 @@ pub fn shortcut(app: &mut SheetApp, key: Key, m: Modifiers) { Key::N => Some(("file.new", json!({}))), Key::W => Some(("file.close", json!({}))), Key::P => { - app.print_now(); + app.print_now(json!({})); None } Key::K => { From 55d41eb13c0b748630c40aaf9d67dbe062bc3c6f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 11 Oct 2026 15:09:43 -0400 Subject: [PATCH 21/27] Desktop theme watcher starts only in the desktop app, not in every SheetApp (tests, embedders) (review fix-up for #203) Co-Authored-By: Claude Opus 5.5 --- apps/gridcraft/src/main.rs | 1 + crates/ui-egui/src/lib.rs | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/apps/gridcraft/src/main.rs b/apps/gridcraft/src/main.rs index aa22826..b7e15cf 100644 --- a/apps/gridcraft/src/main.rs +++ b/apps/gridcraft/src/main.rs @@ -272,6 +272,7 @@ fn main() -> eframe::Result<()> { session.new_workbook(); } let mut app = SheetApp::new(session, services()); + app.watch_desktop_theme(); app.after_engine(); // Show warnings from files opened on the command line. let control_port = control_port; load_prefs(&mut app); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 24dbfeb..144622b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -234,7 +234,7 @@ impl SheetApp { fonts_set: false, fonts_han: None, effective_dark: false, - desktop_theme: desktop_theme::DesktopTheme::start(), + desktop_theme: desktop_theme::DesktopTheme::default(), desktop_dark: None, name_box: None, keytips: keytips::KeyTips::default(), @@ -338,6 +338,12 @@ impl SheetApp { } /// Runs a command and shows its error in a message box (for menu/ribbon clicks). + /// Follow the desktop's light/dark choice while the app runs (Linux; the desktop app calls + /// this once — tests and embedders don't start the watcher thread). + pub fn watch_desktop_theme(&mut self) { + self.desktop_theme = desktop_theme::DesktopTheme::start(); + } + pub fn run_or_alert(&mut self, id: &str, params: Json) { // Printing from the UI also hands the PDF to the host's print flow (web). if id == "file.print" { From 04001fd94394d7f83aaf318d08cc77ad78b460fa Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 21:11:23 +0200 Subject: [PATCH 22/27] View: restore and save the scroll position (sheetView topLeftCell) (#378) The model read topLeftCell but the grid always opened at A1 and never wrote the current scroll back. Sheets now open scrolled to their saved first visible cell; saves record the current one (and the active cell) without dirtying the workbook. Frozen panes use the pane's topLeftCell. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 1 + crates/engine/src/lib.rs | 29 ++++++++++++ crates/ui-egui/src/backstage.rs | 1 + crates/ui-egui/src/lib.rs | 46 +++++++++++++++++-- crates/ui-egui/tests/scroll_position.rs | 60 +++++++++++++++++++++++++ crates/xlsx/src/sheet_read.rs | 4 ++ crates/xlsx/src/sheet_write.rs | 6 ++- crates/xlsx/src/tests/roundtrip.rs | 25 +++++++++++ 8 files changed, 167 insertions(+), 5 deletions(-) create mode 100644 crates/ui-egui/tests/scroll_position.rs diff --git a/ROADMAP.md b/ROADMAP.md index a62418e..85fb5ae 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Workbooks reopen at their saved scroll position (`topLeftCell`, also with frozen panes) and save the current one back (#378) | | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) | | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) | | 2026-10-11 | Data › Forecast › Forecast Sheet: a new sheet with the history, FORECAST.ETS forecast, confidence bounds, statistics and a chart (#322) | diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index c6f0fb9..8dc28d1 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -139,6 +139,35 @@ impl DocState { pub fn sheet_index(&self) -> usize { self.wb.active_sheet } + /// Records the view state that is saved with the file (`sheetView topLeftCell`, the active + /// cell): the first visible cell of the scrolling area per sheet index in `tops`, and the + /// active sheet's active cell. View state never makes the workbook dirty. + pub fn store_view(&mut self, tops: &[(usize, CellRef)]) { + let active = self.selection.active; + let si = self.wb.active_sheet; + let changed = tops.iter().any(|(i, c)| self.wb.sheets.get(*i).is_some_and(|sh| sh.view_top_left != *c)) + || self.wb.sheets.get(si).is_some_and(|sh| sh.view_active != active); + if !changed { + return; + } + let dirty = self.is_dirty(); + let wb = Arc::make_mut(&mut self.wb); + for (i, c) in tops { + if let Some(sh) = wb.sheets.get_mut(*i) + && sh.view_top_left != *c + { + Arc::make_mut(sh).view_top_left = *c; + } + } + if let Some(sh) = wb.sheets.get_mut(si) + && sh.view_active != active + { + Arc::make_mut(sh).view_active = active; + } + if !dirty { + self.saved = self.wb.clone(); + } + } pub fn display_title(&self) -> String { self.path .as_deref() diff --git a/crates/ui-egui/src/backstage.rs b/crates/ui-egui/src/backstage.rs index 32000f7..2c48b24 100644 --- a/crates/ui-egui/src/backstage.rs +++ b/crates/ui-egui/src/backstage.rs @@ -724,6 +724,7 @@ pub fn export_as(app: &mut SheetApp, fmt: &str) -> bool { } }; } + app.store_view_state(); match app.session.run("file.saveBytes", json!({"format": fmt})) { Ok(r) => { let bytes = r.get("base64").and_then(Json::as_str).and_then(gridcraft_engine::io::base64_decode); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ea7404e..8268745 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -247,6 +247,9 @@ impl SheetApp { /// Runs an engine command (or a UI command) and handles UI requests it makes. pub fn run(&mut self, id: &str, params: Json) -> Result { + if matches!(id, "file.save" | "file.saveAs" | "file.saveBytes") { + self.store_view_state(); + } if id.starts_with("file.") || matches!( id, @@ -485,8 +488,9 @@ impl SheetApp { let path = if std::path::Path::new(&path).extension().is_none() { format!("{path}.xlsx") } else { path }; self.run_or_alert("file.save", json!({"path": path})); } - } else if let Some(dl) = &self.services.download - && let Ok(r) = self.session.run("file.saveBytes", json!({"format": "xlsx"})) + } else if self.services.download.is_some() + && let Ok(r) = self.run("file.saveBytes", json!({"format": "xlsx"})) + && let Some(dl) = &self.services.download && let Some(b) = r.get("base64").and_then(Json::as_str).and_then(gridcraft_engine::io::base64_decode) { let name = r.get("name").and_then(Json::as_str).unwrap_or("Book.xlsx").to_string(); @@ -532,12 +536,45 @@ impl SheetApp { } pub fn view(&self) -> SheetView { - self.view_key().and_then(|k| self.views.get(&k).copied()).unwrap_or_default() + self.view_key().and_then(|k| self.views.get(&k).copied()).unwrap_or_else(|| self.saved_view()) } pub fn view_mut(&mut self) -> Option<&mut SheetView> { let k = self.view_key()?; - Some(self.views.entry(k).or_default()) + let saved = self.saved_view(); + Some(self.views.entry(k).or_insert(saved)) + } + + /// The view a sheet opens with: scrolled to the file's saved first visible cell (`topLeftCell`). + fn saved_view(&self) -> SheetView { + let Some(sh) = self.session.active().and_then(|d| d.wb.active()) else { return SheetView::default() }; + if sh.view != gridcraft_engine::model::SheetViewMode::Normal { + return SheetView::default(); + } + let (fr, fc) = sh.freeze.unwrap_or((0, 0)); + let top = sh.view_top_left; + let y = sh.row_top(top.row.max(fr)) - sh.row_top(fr); + let x = sh.col_left(top.col.max(fc)) - sh.col_left(fc); + SheetView { scroll: egui::vec2(x.max(0.0) as f32, y.max(0.0) as f32) } + } + + /// Hands the active document's scroll positions (first visible cell per sheet) and active + /// cell to the engine so a save writes them (`sheetView topLeftCell`). Normal view only: + /// Page Layout view scrolls over pages, not cells. + pub fn store_view_state(&mut self) { + let Some(d) = self.session.active_mut() else { return }; + let mut tops = Vec::new(); + for (&(uid, si), v) in &self.views { + let Some(sh) = d.wb.sheets.get(si).filter(|sh| uid == d.uid && sh.view == gridcraft_engine::model::SheetViewMode::Normal) else { + continue; + }; + let (fr, fc) = sh.freeze.unwrap_or((0, 0)); + // A hair past the edge so a row that starts exactly at the scroll offset is the one found. + let row = sh.row_at(sh.row_top(fr) + v.scroll.y.max(0.0) as f64 + 0.01); + let col = sh.col_at(sh.col_left(fc) + v.scroll.x.max(0.0) as f64 + 0.01); + tops.push((si, CellRef::new(row, col))); + } + d.store_view(&tops); } /// Starts editing the active cell. `text` replaces the content (typing) or pre-fills it. @@ -769,6 +806,7 @@ impl SheetApp { && self.session.active().is_some_and(|d| d.is_dirty() && d.path.as_deref().is_some_and(|p| p.ends_with(".xlsx"))) { self.grid.last_autosave = now; + self.store_view_state(); let _ = self.session.run("file.save", json!({})); } // Window title. diff --git a/crates/ui-egui/tests/scroll_position.rs b/crates/ui-egui/tests/scroll_position.rs new file mode 100644 index 0000000..7945eb3 --- /dev/null +++ b/crates/ui-egui/tests/scroll_position.rs @@ -0,0 +1,60 @@ +//! The saved scroll position (`sheetView topLeftCell`) is restored on open and saved back (#378). + +use gridcraft_engine::core::{CellRef, Value}; +use gridcraft_engine::model::Workbook; +use gridcraft_engine::{DocState, Session}; +use gridcraft_ui_egui::{SheetApp, grid::Geo}; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +fn harness(freeze: Option<(u32, u32)>) -> Harness { + let mut wb = Workbook::new(); + let sh = wb.sheet_mut(0).unwrap(); + for r in 0..60 { + sh.set_value(CellRef::new(r, 0), Value::text(format!("row {}", r + 1))); + } + sh.view_top_left = CellRef::parse("A31").unwrap(); + sh.view_active = CellRef::parse("A31").unwrap(); + sh.freeze = freeze; + let mut session = Session::new(); + session.add_document(DocState::new(wb, None, "Book1".into())); + let mut h = egui_kittest::Harness::builder().with_size(egui::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 +} + +/// The first row painted at the top of the scrolling area. +fn first_scrolled_row(h: &Harness) -> u32 { + 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 (fr, _) = sheet.freeze.unwrap_or((0, 0)); + let top = geo.cells.top() + sheet.row_top(fr) as f32 * geo.z; + (fr..200).find(|r| geo.y(sheet, *r) <= top + 0.5 && geo.y(sheet, *r + 1) > top + 0.5).unwrap() +} + +#[test] +fn saved_scroll_position_is_restored_and_saved_back() { + for freeze in [None, Some((1, 0))] { + let mut h = harness(freeze); + assert_eq!(first_scrolled_row(&h), 30, "row 31 is the first visible row ({freeze:?})"); + // Scroll to row 51 and save: the file gets the new position, the document stays clean. + { + let app = h.state_mut(); + let sh = app.session.active().unwrap().wb.active().unwrap().clone(); + let fr = freeze.map_or(0, |f| f.0); + app.view_mut().unwrap().scroll.y = (sh.row_top(50) - sh.row_top(fr)) as f32; + app.run("file.saveBytes", json!({"format": "xlsx"})).unwrap(); + } + let d = h.state().session.active().unwrap(); + assert_eq!(d.wb.active().unwrap().view_top_left, CellRef::parse("A51").unwrap()); + assert!(!d.is_dirty(), "scrolling and saving view state does not dirty the workbook"); + } +} diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 07ee340..8eac285 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -256,6 +256,10 @@ fn read_views(e: &El, sheet: &mut Sheet) { let rows = p.attr_f64("ySplit").unwrap_or(0.0).clamp(0.0, (MAX_ROWS - 1) as f64) as u32; if rows > 0 || cols > 0 { sheet.freeze = Some((rows, cols)); + // With frozen panes the scroll position is the scrolling pane's first cell. + if let Some(c) = p.attr("topLeftCell").and_then(CellRef::parse) { + sheet.view_top_left = c; + } } } active_pane = p.attr("activePane"); diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index fee804c..94f1663 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -182,7 +182,11 @@ pub fn write_sheet(wb: &Workbook, si: usize, selected: bool, out: &mut Out) -> ( let _ = write!( s, " topLeftCell=\"{}\" activePane=\"{pane}\" state=\"frozen\"/>", - CellRef::new(rows.min(gridcraft_core::MAX_ROWS - 1), cols.min(gridcraft_core::MAX_COLS - 1)).a1() + CellRef::new( + rows.max(sheet.view_top_left.row).min(gridcraft_core::MAX_ROWS - 1), + cols.max(sheet.view_top_left.col).min(gridcraft_core::MAX_COLS - 1) + ) + .a1() ); let _ = write!(s, ""); } diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 2385b8e..a4f7b18 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -763,3 +763,28 @@ fn bad_comment_dates_are_left_out() { let c = &back.sheets[0].comments[&at("A1")]; assert!(c.threaded && c.dates.is_empty()); } + +/// #378: the saved scroll position survives a round trip, with and without frozen panes (where it +/// is the scrolling pane's `topLeftCell`). +#[test] +fn scroll_position_round_trips_with_and_without_frozen_panes() { + let mut wb = Workbook::new(); + wb.sheet_mut(0).unwrap().view_top_left = at("A31"); + let (back, _) = read_xlsx(&write_xlsx(&wb).unwrap()).unwrap(); + assert_eq!(back.sheet(0).unwrap().view_top_left, at("A31")); + + let s = wb.sheet_mut(0).unwrap(); + s.freeze = Some((1, 0)); + s.view_top_left = at("C40"); + let bytes = write_xlsx(&wb).unwrap(); + let (xml, _) = sheet1_xml(&bytes); + assert!(xml.contains(r#" Date: Sun, 11 Oct 2026 21:12:24 +0200 Subject: [PATCH 23/27] Review: Protect Workbook dialog with an Excel-compatible password (#345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ribbon button toggled structure protection silently. It now opens a Protect Structure and Windows dialog with an optional password; when a password was set, unprotecting asks for it and really verifies it (SHA-512, spin count 100000, salted). The password hash is read from and written to XLSX workbookProtection (workbookAlgorithmName/HashValue/ SaltValue/SpinCount or the legacy workbookPassword); an unreadable one stays locked. File › Info uses the same flow. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 1 + crates/engine/src/cmd/review.rs | 65 ++++++++++++++++++++++++++---- crates/model/src/workbook.rs | 4 ++ crates/ui-egui/src/backstage.rs | 16 +++++++- crates/ui-egui/src/dialogs.rs | 17 ++++++++ crates/ui-egui/src/ribbon.rs | 2 +- crates/xlsx/src/read.rs | 1 + crates/xlsx/src/sheet_read.rs | 44 +++++++++++++------- crates/xlsx/src/sheet_write.rs | 35 +++++++++------- crates/xlsx/src/tests/fixtures.rs | 5 ++- crates/xlsx/src/tests/roundtrip.rs | 3 ++ crates/xlsx/src/write.rs | 3 +- 12 files changed, 156 insertions(+), 40 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index a62418e..b927fc4 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Review › Protect Workbook opens a dialog with an optional password (Excel-compatible hash, kept in XLSX `workbookProtection`); unprotecting verifies it (#345) | | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) | | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) | | 2026-10-11 | Data › Forecast › Forecast Sheet: a new sheet with the history, FORECAST.ETS forecast, confidence bounds, statistics and a chart (#322) | diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index 70823cd..8931bf5 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -19,7 +19,15 @@ pub fn specs() -> Vec { protect_sheet ), cmd!("review.unprotectSheet", "Unprotect Sheet", ["Review", "Protect"], None, "{password?}", has_doc, unprotect_sheet), - cmd!("review.protectWorkbook", "Protect Workbook", ["Review", "Protect"], None, "{on?: bool}", has_doc, protect_workbook), + cmd!( + "review.protectWorkbook", + "Protect Workbook", + ["Review", "Protect"], + None, + "{on?: bool (default: toggle), password?} (structure protection; unprotecting needs the password when one was set)", + has_doc, + protect_workbook + ), cmd!(query "review.workbookStatistics", "Workbook Statistics", ["Review", "Proofing"], None, "{}", has_doc, stats), // Not journaled: the params can hold a password. CommandSpec { @@ -159,6 +167,15 @@ fn unprotect_sheet(s: &mut Session, p: &Json) -> Result { }) } +/// A new Excel-compatible (SHA-512, salted) hash of `pw`. The salt mixes `what` is protected +/// with the document, a counter and the clock, so equal passwords get different hashes. +fn new_password_hash(d: &DocState, what: &str, pw: &str) -> PasswordHash { + static SALT_COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let n = SALT_COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let seed = format!("{what}\u{0}{n}\u{0}{}\u{0}{}", d.uid, gridcraft_calc::now_serial().to_bits()); + PasswordHash::new_sha512(pw, &gridcraft_model::password::salt_from(seed.as_bytes())) +} + fn range_text(r: &EditRange) -> String { r.ranges.iter().map(|x| x.a1()).collect::>().join(",") } @@ -166,7 +183,6 @@ fn range_text(r: &EditRange) -> String { /// Review › Allow Edit Ranges: list, add, modify or delete the ranges that stay editable on a /// protected sheet, or unlock a password range for this session. fn allow_edit_ranges(s: &mut Session, p: &Json) -> Result { - static SALT_COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); let sheet = target_sheet(s, p)?; let action = str_param(p, "action").unwrap_or("list"); let title = str_param(p, "title").map(str::trim).filter(|t| !t.is_empty()); @@ -180,11 +196,7 @@ fn allow_edit_ranges(s: &mut Session, p: &Json) -> Result { .position(|r| r.title.eq_ignore_ascii_case(t)) .ok_or_else(|| bad("review.allowEditRanges", format!("no range titled `{t}`"))) }; - let hash = |pw: &str, title: &str| { - let n = SALT_COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); - let seed = format!("{sheet_name}\u{0}{title}\u{0}{n}\u{0}{}\u{0}{}", d.uid, gridcraft_calc::now_serial().to_bits()); - PasswordHash::new_sha512(pw, &gridcraft_model::password::salt_from(seed.as_bytes())) - }; + let hash = |pw: &str, title: &str| new_password_hash(d, &format!("{sheet_name}\u{0}{title}"), pw); if matches!(action, "add" | "modify" | "delete") && sh.is_protected() { return Err(EngineError::Other("Unprotect the sheet to change its edit ranges.".into())); } @@ -270,11 +282,28 @@ fn allow_edit_ranges(s: &mut Session, p: &Json) -> Result { )) } +/// Review › Protect Workbook: lock the workbook structure (sheets can't be added, moved, +/// renamed, hidden or deleted), optionally with a password that unprotecting must match. fn protect_workbook(s: &mut Session, p: &Json) -> Result { - let cur = s.doc()?.wb.protected_structure; + let d = s.doc()?; + let cur = d.wb.protected_structure; let on = bool_param(p, "on").unwrap_or(!cur); + if on == cur { + return Ok(json!({"on": on})); + } + let password = if on { + str_param(p, "password").filter(|pw| !pw.is_empty()).map(|pw| new_password_hash(d, "workbook", pw)) + } else { + if let Some(h) = &d.wb.structure_password + && !h.verify(str_param(p, "password").unwrap_or("")).map_err(EngineError::Other)? + { + return Err(EngineError::Other("The password you supplied is not correct.".into())); + } + None + }; edit(s, |cx| { cx.wb.protected_structure = on; + cx.wb.structure_password = password.clone(); Ok(json!({"on": on})) }) } @@ -355,6 +384,26 @@ mod tests { assert_eq!(at(&s), (0, "A1".to_string())); } + #[test] + fn protect_workbook_with_a_password_really_verifies() { + let mut s = book(); + s.execute("review.protectWorkbook", json!({"password": "pa55"})).unwrap(); + assert!(s.doc().unwrap().wb.protected_structure); + assert!(s.execute("home.insertSheet", json!({"name": "Three"})).is_err(), "the structure is locked"); + // A second Protect keeps it protected; unprotecting needs the right password. + assert_eq!(s.execute("review.protectWorkbook", json!({"on": true})).unwrap()["on"], true); + assert!(s.execute("review.protectWorkbook", json!({})).is_err()); + assert!(s.execute("review.protectWorkbook", json!({"on": false, "password": "pass"})).is_err()); + assert!(s.doc().unwrap().wb.protected_structure); + s.execute("review.protectWorkbook", json!({"on": false, "password": "pa55"})).unwrap(); + let wb = &s.doc().unwrap().wb; + assert!(!wb.protected_structure && wb.structure_password.is_none()); + // Without a password it toggles freely. + s.execute("review.protectWorkbook", json!({})).unwrap(); + s.execute("review.protectWorkbook", json!({})).unwrap(); + assert!(!s.doc().unwrap().wb.protected_structure); + } + #[test] fn convert_to_comments_turns_every_note_into_a_comment_and_undoes() { let mut s = book(); diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 7e03cb3..6ea5f99 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -67,6 +67,9 @@ pub struct Workbook { pub props: DocProps, /// Workbook structure protection. pub protected_structure: bool, + /// Password of the structure protection (XLSX `workbookProtection`), never the password. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub structure_password: Option, /// Custom fill lists (Options › Custom Lists) beyond the built-in ones. pub custom_lists: Vec>, /// Named cell styles (Cell Styles gallery): name → style. @@ -95,6 +98,7 @@ impl Workbook { active_sheet: 0, props: DocProps::default(), protected_structure: false, + structure_password: None, custom_lists: vec![], cell_styles: vec![], preserved: None, diff --git a/crates/ui-egui/src/backstage.rs b/crates/ui-egui/src/backstage.rs index 32000f7..07e5705 100644 --- a/crates/ui-egui/src/backstage.rs +++ b/crates/ui-egui/src/backstage.rs @@ -422,6 +422,20 @@ fn open_page(app: &mut SheetApp, ui: &mut Ui, out: &mut Outcome) { recent_list(app, ui, out, usize::MAX); } +/// Review › Protect Workbook and File › Info › Protect Workbook: the protect dialog (with an +/// optional password) when the structure is open; when it's locked, the password prompt if one +/// was set, else unprotect right away. +pub fn protect_workbook(app: &mut SheetApp) { + let (on, password) = app.session.active().map(|d| (d.wb.protected_structure, d.wb.structure_password.is_some())).unwrap_or((false, false)); + if !on { + app.open_dialog("protectWorkbook", json!({})); + } else if password { + app.open_dialog("unprotectWorkbook", json!({})); + } else { + app.run_or_alert("review.protectWorkbook", json!({"on": false})); + } +} + fn info(app: &mut SheetApp, ui: &mut Ui, bs: &mut Backstage) { let t = Tokens::get(ui.ctx()); let lang = app.ui.language; @@ -451,7 +465,7 @@ fn info(app: &mut SheetApp, ui: &mut Ui, bs: &mut Backstage) { ui.add_space(18.0); ui.horizontal(|ui| { if square_button(ui, Icon::Lock, lang.tr("Protect Workbook")).clicked() { - app.run_or_alert("review.protectWorkbook", json!({"on": !protected})); + protect_workbook(app); } ui.add_space(12.0); ui.vertical(|ui| { diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 196d47a..0291ebb 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -219,6 +219,23 @@ impl Dialog { json!({"action": "add", "range": sel}), ) } + "protectWorkbook" => Dialog::form( + "protectWorkbook", + "Protect Structure and Windows", + "review.protectWorkbook", + vec![ + Password { key: "password", label: "Password (optional):" }, + Note("Protect workbook for: Structure (sheets can't be added, moved, renamed, hidden or deleted).".into()), + ], + json!({"on": true}), + ), + "unprotectWorkbook" => Dialog::form( + "unprotectWorkbook", + "Unprotect Workbook", + "review.protectWorkbook", + vec![Password { key: "password", label: "Password:" }], + json!({"on": false}), + ), "unprotectSheet" => Dialog::form( "unprotectSheet", "Unprotect Sheet", diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index ccc51eb..af602b2 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -1714,7 +1714,7 @@ fn review(app: &mut SheetApp, ui: &mut Ui) { } } if big_button(ui, Icon::Book, "Protect\nWorkbook", "Protect Workbook structure", false).clicked() { - act(app, "review.protectWorkbook", json!({})); + crate::backstage::protect_workbook(app); } if big_button(ui, Icon::Validation, "Allow Edit\nRanges", "Allow Edit Ranges", false).clicked() { app.open_dialog("allowEditRanges", json!({})); diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 7c00d01..d1da00a 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -185,6 +185,7 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { } if let Some(p) = wb_xml.child("workbookProtection") { wb.protected_structure = p.flag("lockStructure", false); + wb.structure_password = if wb.protected_structure { crate::sheet_read::read_password(p, "workbook") } else { None }; } let active_tab = wb_xml.path(&["bookViews", "workbookView"]).and_then(|v| v.attr_u32("activeTab")).unwrap_or(0) as usize; diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 07ee340..2c33d1f 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -208,6 +208,35 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result Option { + let attr = |base: &str| e.attr(&password_attr(prefix, base)); + if attr("algorithmName").is_some() || attr("hashValue").is_some() { + return Some(PasswordHash::Iterated { + algorithm: attr("algorithmName").unwrap_or("").to_string(), + hash: attr("hashValue").unwrap_or("").to_string(), + salt: attr("saltValue").unwrap_or("").to_string(), + spin_count: e.attr_u32(&password_attr(prefix, "spinCount")).unwrap_or(0), + }); + } + attr("password").map(|h| match u16::from_str_radix(h.trim(), 16) { + Ok(h) => PasswordHash::Legacy(h), + Err(_) => PasswordHash::Iterated { algorithm: String::new(), hash: String::new(), salt: String::new(), spin_count: 0 }, + }) +} + +/// `base` preceded by `prefix` in camel case: ("workbook", "hashValue") → `workbookHashValue`. +pub(crate) fn password_attr(prefix: &str, base: &str) -> String { + let mut chars = base.chars(); + match chars.next() { + Some(c) if !prefix.is_empty() => format!("{prefix}{}{}", c.to_ascii_uppercase(), chars.as_str()), + _ => base.to_string(), + } +} + /// Allow Edit Ranges. A range whose password can't be read keeps a hash that never matches /// (or an unsupported algorithm), so it stays locked rather than open. fn read_protected_ranges(e: &El, sheet: &mut Sheet) { @@ -216,20 +245,7 @@ fn read_protected_ranges(e: &El, sheet: &mut Sheet) { if ranges.is_empty() { continue; } - let password = if p.attr("algorithmName").is_some() || p.attr("hashValue").is_some() { - Some(PasswordHash::Iterated { - algorithm: p.attr("algorithmName").unwrap_or("").to_string(), - hash: p.attr("hashValue").unwrap_or("").to_string(), - salt: p.attr("saltValue").unwrap_or("").to_string(), - spin_count: p.attr_u32("spinCount").unwrap_or(0), - }) - } else { - p.attr("password").map(|h| match u16::from_str_radix(h.trim(), 16) { - Ok(h) => PasswordHash::Legacy(h), - // Unreadable: keep it locked. - Err(_) => PasswordHash::Iterated { algorithm: String::new(), hash: String::new(), salt: String::new(), spin_count: 0 }, - }) - }; + let password = read_password(p, ""); let title = p.attr("name").unwrap_or("").to_string(); sheet.edit_ranges.push(EditRange { title, ranges, password, security_descriptor: p.attr("securityDescriptor").map(str::to_string) }); } diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index fee804c..acc98f6 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -1118,26 +1118,33 @@ fn threaded_xml(sheet: &Sheet, ids: &BTreeMap>, persons: &m s } +/// A protection password hash as attributes, each name preceded by `prefix` (see +/// [`crate::sheet_read::read_password`]). +pub(crate) fn password_attrs(h: &gridcraft_model::PasswordHash, prefix: &str) -> String { + let name = |base: &str| crate::sheet_read::password_attr(prefix, base); + match h { + gridcraft_model::PasswordHash::Legacy(h) => format!(" {}=\"{h:04X}\"", name("password")), + gridcraft_model::PasswordHash::Iterated { algorithm, hash, salt, spin_count } => format!( + " {}=\"{}\" {}=\"{}\" {}=\"{}\" {}=\"{spin_count}\"", + name("algorithmName"), + esc_attr(algorithm), + name("hashValue"), + esc_attr(hash), + name("saltValue"), + esc_attr(salt), + name("spinCount") + ), + } +} + /// Allow Edit Ranges: `` (after `sheetProtection`). fn protected_ranges_xml(sheet: &Sheet) -> String { let mut s = String::new(); for r in sheet.edit_ranges.iter().filter(|r| !r.ranges.is_empty()) { let sqref: Vec = r.ranges.iter().map(|x| x.a1()).collect(); let _ = write!(s, " { - let _ = write!(s, " password=\"{h:04X}\""); - } - Some(gridcraft_model::PasswordHash::Iterated { algorithm, hash, salt, spin_count }) => { - let _ = write!( - s, - " algorithmName=\"{}\" hashValue=\"{}\" saltValue=\"{}\" spinCount=\"{spin_count}\"", - esc_attr(algorithm), - esc_attr(hash), - esc_attr(salt) - ); - } - None => {} + if let Some(h) = &r.password { + s.push_str(&password_attrs(h, "")); } if let Some(sd) = &r.security_descriptor { let _ = write!(s, " securityDescriptor=\"{}\"", esc_attr(sd)); diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index 5533e01..42adcb5 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -48,7 +48,7 @@ fn shared_formulas_and_value_types() { #[test] fn workbook_settings_names_and_states() { - let wb_extra = r#""#; + let wb_extra = r#""#; // Defined names must come after ; build the workbook by hand. let wbx = format!( r#"{wb_extra} @@ -69,6 +69,9 @@ fn workbook_settings_names_and_states() { assert_eq!(wb.sheets.len(), 3); assert_eq!(wb.date_system, DateSystem::D1904); assert!(wb.protected_structure); + // The legacy structure password ("password" hashes to 83AF) is kept and really verifies. + let pw = wb.structure_password.as_ref().unwrap(); + assert!(pw.verify("password").unwrap() && !pw.verify("Password").unwrap()); assert_eq!(wb.calc.mode, CalcMode::Manual); assert!(wb.calc.iterative); assert_eq!(wb.calc.max_iterations, 50); diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 2385b8e..4038908 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -22,6 +22,7 @@ fn sample() -> Workbook { wb.date_system = DateSystem::D1904; wb.calc = CalcSettings { mode: CalcMode::Manual, iterative: true, max_iterations: 42, max_change: 0.5, precision_as_displayed: true }; wb.protected_structure = true; + wb.structure_password = Some(gridcraft_model::PasswordHash::new_sha512("s3cret", b"0123456789abcdef")); wb.props = DocProps { title: "T & ".into(), subject: "S".into(), @@ -412,6 +413,8 @@ fn full_roundtrip() { assert_eq!(back.date_system, wb.date_system); assert_eq!(back.calc, wb.calc); assert!(back.protected_structure); + assert_eq!(back.structure_password, wb.structure_password); + assert!(back.structure_password.as_ref().unwrap().verify("s3cret").unwrap()); assert_eq!(back.props, wb.props); assert_eq!(back.theme, wb.theme); assert_eq!(back.names, wb.names); diff --git a/crates/xlsx/src/write.rs b/crates/xlsx/src/write.rs index dc2ddc4..6aae60e 100644 --- a/crates/xlsx/src/write.rs +++ b/crates/xlsx/src/write.rs @@ -268,7 +268,8 @@ pub fn write_xlsx(wb: &Workbook) -> Result, IoError> { let mut w = format!("{XML_DECL}"); w.push_str(if wb.date_system == gridcraft_core::DateSystem::D1904 { "" } else { "" }); if wb.protected_structure { - w.push_str(""); + let pw = wb.structure_password.as_ref().map(|h| crate::sheet_write::password_attrs(h, "workbook")).unwrap_or_default(); + let _ = write!(w, ""); } let _ = write!(w, "{sheets_xml}"); w.push_str(&crate::passthrough::write_workbook(wb, &mut wb_rels, &mut out)); From 22978a7e62f64d5a8d5a9cb1d71dc3d28f84928b Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 21:29:10 +0200 Subject: [PATCH 24/27] Review: Protect Sheet passwords use Excel-compatible hashes, kept in XLSX Sheet protection stored an internal FNV hash that was never written to XLSX, and an Excel sheet password was dropped on import, so imported protected sheets could be unprotected without it. Protect Sheet now stores a salted SHA-512 hash (spin count 100000) via PasswordHash; the XLSX sheetProtection password (iterated or legacy) is read and written, and Unprotect Sheet really verifies it. Hashes from older GridCraft JSON workbooks are still read and verified. Refs #346 Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 1 + crates/engine/src/cmd/review.rs | 45 ++++++++++++++++++++---- crates/engine/src/tests.rs | 2 +- crates/model/src/features.rs | 14 +++++--- crates/ui-egui/tests/protect_password.rs | 2 +- crates/xlsx/src/sheet_read.rs | 6 ++-- crates/xlsx/src/sheet_write.rs | 3 ++ crates/xlsx/src/tests/fixtures.rs | 7 ++-- crates/xlsx/src/tests/roundtrip.rs | 9 ++++- 9 files changed, 70 insertions(+), 19 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index b927fc4..1d31b92 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Protect Sheet passwords use Excel-compatible hashes and round-trip in XLSX `sheetProtection` (imported passwords verify instead of being dropped) | | 2026-10-11 | Review › Protect Workbook opens a dialog with an optional password (Excel-compatible hash, kept in XLSX `workbookProtection`); unprotecting verifies it (#345) | | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) | | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) | diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index 8931bf5..0dc5962 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -134,7 +134,8 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result { let sheet = target_sheet(s, p)?; let b = |k: &str| bool_param(p, k).unwrap_or(false); let prot = SheetProtection { - password_hash: str_param(p, "password").filter(|x| !x.is_empty()).map(password_hash), + password: None, + legacy_password_hash: None, select_locked: true, select_unlocked: true, format_cells: b("formatCells"), @@ -147,6 +148,9 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result { sort: b("sort"), autofilter: b("autofilter"), }; + let d = s.doc()?; + let what = d.wb.sheet(sheet).map(|sh| sh.name.clone()).unwrap_or_default(); + let prot = SheetProtection { password: str_param(p, "password").filter(|x| !x.is_empty()).map(|pw| new_password_hash(d, &what, pw)), ..prot }; edit(s, |cx| { cx.sheet_mut(sheet)?.protection = Some(prot.clone()); Ok(Json::Null) @@ -155,11 +159,16 @@ fn protect_sheet(s: &mut Session, p: &Json) -> Result { fn unprotect_sheet(s: &mut Session, p: &Json) -> Result { let sheet = target_sheet(s, p)?; - let ph = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.protection.as_ref()).and_then(|pr| pr.password_hash); - if let Some(h) = ph - && str_param(p, "password").map(password_hash) != Some(h) - { - return Err(EngineError::Other("The password you supplied is not correct.".into())); + if let Some(pr) = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.protection.as_ref()) { + let pw = str_param(p, "password").unwrap_or(""); + let ok = match (&pr.password, pr.legacy_password_hash) { + (Some(h), _) => h.verify(pw).map_err(EngineError::Other)?, + (None, Some(h)) => password_hash(pw) == h, + (None, None) => true, + }; + if !ok { + return Err(EngineError::Other("The password you supplied is not correct.".into())); + } } edit(s, |cx| { cx.sheet_mut(sheet)?.protection = None; @@ -384,6 +393,30 @@ mod tests { assert_eq!(at(&s), (0, "A1".to_string())); } + #[test] + fn sheet_passwords_use_excel_hashes_and_old_json_hashes_still_verify() { + let mut s = book(); + s.execute("review.protectSheet", json!({"password": "pa55"})).unwrap(); + let prot = s.doc().unwrap().wb.active().unwrap().protection.clone().unwrap(); + assert!(matches!(&prot.password, Some(PasswordHash::Iterated { algorithm, .. }) if algorithm == "SHA-512")); + assert!(s.execute("review.unprotectSheet", json!({})).is_err()); + assert!(s.execute("review.unprotectSheet", json!({"password": "pass"})).is_err()); + s.execute("review.unprotectSheet", json!({"password": "pa55"})).unwrap(); + assert!(!s.doc().unwrap().wb.active().unwrap().is_protected()); + // A GridCraft JSON workbook saved before carried the internal hash under `password_hash`. + let old = format!( + r#"{{"password_hash": {}, "select_locked": true, "select_unlocked": true, "format_cells": false, "format_columns": false, "format_rows": false, "insert_columns": false, "insert_rows": false, "delete_columns": false, "delete_rows": false, "sort": false, "autofilter": false}}"#, + password_hash("old") + ); + let prot: SheetProtection = serde_json::from_str(&old).unwrap(); + assert_eq!((prot.legacy_password_hash, prot.password.is_none()), (Some(password_hash("old")), true)); + let i = s.doc().unwrap().wb.active_sheet; + let wb = std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb); + std::sync::Arc::make_mut(&mut wb.sheets[i]).protection = Some(prot); + assert!(s.execute("review.unprotectSheet", json!({"password": "new"})).is_err()); + s.execute("review.unprotectSheet", json!({"password": "old"})).unwrap(); + } + #[test] fn protect_workbook_with_a_password_really_verifies() { let mut s = book(); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index dfe8254..9be4bb9 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -816,7 +816,7 @@ fn protection_passwords_stay_out_of_the_journal_and_recorded_scripts() { r.execute(id, p.clone()).unwrap_or_else(|e| panic!("{id}: {e}")); let prot = r.doc().unwrap().wb.active().unwrap().protection.clone(); if id == "review.protectSheet" { - assert!(prot.as_ref().is_some_and(|p| p.format_cells && p.password_hash.is_none()), "{prot:?}"); + assert!(prot.as_ref().is_some_and(|p| p.format_cells && p.password.is_none()), "{prot:?}"); } } assert!(r.doc().unwrap().wb.active().unwrap().protection.is_none()); diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index 1864d6d..c8dc28b 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -554,8 +554,14 @@ impl Default for PrintSettings { #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] pub struct SheetProtection { - /// Hash of the password (our own salted FNV; never the password itself). - pub password_hash: Option, + /// Hash of the password (Excel-compatible, written to XLSX `sheetProtection`; never the + /// password itself). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub password: Option, + /// The internal hash older GridCraft JSON workbooks stored ([`password_hash`]); still + /// verified when unprotecting, never written for new passwords. + #[serde(rename = "password_hash", default, skip_serializing_if = "Option::is_none")] + pub legacy_password_hash: Option, pub select_locked: bool, pub select_unlocked: bool, pub format_cells: bool, @@ -587,8 +593,8 @@ impl EditRange { } } -/// Simple non-cryptographic password hash for sheet protection (like Excel's, protection is a -/// convenience, not security). +/// The non-cryptographic password hash older GridCraft versions stored for sheet protection +/// (only verified now, see [`SheetProtection::legacy_password_hash`]). pub fn password_hash(pw: &str) -> u64 { let mut h: u64 = 0xcbf29ce484222325 ^ 0x5348_4545_5443; for b in pw.bytes() { diff --git a/crates/ui-egui/tests/protect_password.rs b/crates/ui-egui/tests/protect_password.rs index dc2b1c5..ee68cf2 100644 --- a/crates/ui-egui/tests/protect_password.rs +++ b/crates/ui-egui/tests/protect_password.rs @@ -57,7 +57,7 @@ fn protect_and_unprotect_sheet_mask_the_password() { h.get_by_label(" OK ").click(); h.run_steps(3); assert!(h.state().dialog.is_none()); - assert!(h.state().session.active().unwrap().wb.active().unwrap().protection.as_ref().is_some_and(|p| p.password_hash.is_some())); + assert!(h.state().session.active().unwrap().wb.active().unwrap().protection.as_ref().is_some_and(|p| p.password.is_some())); // Unprotect asks for the password with a masked field too, and it starts hidden again. h.state_mut().open_dialog("unprotectSheet", json!({})); diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 2c33d1f..3e9e374 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -143,11 +143,9 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result sheet.print.row_breaks = e.kids("brk").filter_map(|b| b.attr_u32("id")).filter(|&r| r > 0 && r < MAX_ROWS).collect(), "colBreaks" => sheet.print.col_breaks = e.kids("brk").filter_map(|b| b.attr_u32("id")).filter(|&c| c > 0 && c < MAX_COLS).collect(), "sheetProtection" if e.flag("sheet", false) => { - if e.attr("password").is_some() || e.attr("hashValue").is_some() { - cx.warn(format!("sheet \"{name}\": the protection password was not imported")); - } sheet.protection = Some(SheetProtection { - password_hash: None, + password: read_password(e, ""), + legacy_password_hash: None, select_locked: !e.flag("selectLockedCells", false), select_unlocked: !e.flag("selectUnlockedCells", false), format_cells: !e.flag("formatCells", true), diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index acc98f6..a56cf5e 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -332,6 +332,9 @@ pub fn write_sheet(wb: &Workbook, si: usize, selected: bool, out: &mut Out) -> ( if !p.select_unlocked { s.push_str(" selectUnlockedCells=\"1\""); } + if let Some(h) = &p.password { + s.push_str(&password_attrs(h, "")); + } s.push_str("/>"); } s.push_str(&protected_ranges_xml(sheet)); diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index 42adcb5..9cd09a3 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -223,7 +223,7 @@ fn hyperlinks_comments_tables_validation_filters() { &CTitlePage &P - + Other!$A$1:$A$3G1:G4 Data!A1:E1F1"#; @@ -270,7 +270,10 @@ Check this"#; assert_eq!(s.print.row_breaks, vec![10]); let p = s.protection.as_ref().unwrap(); assert!(p.format_cells && p.sort && !p.insert_rows && p.select_locked); - assert!(rep.warnings.iter().any(|w| w.contains("password"))); + // The legacy sheet password is imported (CBEB is the hash of "test") and verifies. + let pw = p.password.as_ref().unwrap(); + assert!(pw.verify("test").unwrap() && !pw.verify("Test").unwrap()); + assert!(!rep.warnings.iter().any(|w| w.contains("password")), "{:?}", rep.warnings); assert_eq!(s.sparklines.len(), 1); assert_eq!(s.sparklines[0].kind, gridcraft_model::SparklineKind::Column); } diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 4038908..be59af4 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -248,7 +248,14 @@ fn sample() -> Workbook { (3, FilterCriterion::FillColor(Color::Rgb(0xFF0000))), ], }); - s.protection = Some(SheetProtection { format_cells: true, sort: true, select_locked: true, select_unlocked: true, ..Default::default() }); + s.protection = Some(SheetProtection { + format_cells: true, + sort: true, + select_locked: true, + select_unlocked: true, + password: Some(gridcraft_model::PasswordHash::new_sha512("sheet pw", b"fedcba9876543210")), + ..Default::default() + }); s.print = PrintSettings { orientation: Orientation::Landscape, paper: "A4".into(), From 6290bf123a26dac914d9e33a0c79819b76f06122 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 21:32:57 +0200 Subject: [PATCH 25/27] Keyboard: Shift+Space and Ctrl+Space select without starting an edit (#338) Co-Authored-By: Claude Sonnet 5.5 --- ROADMAP.md | 1 + crates/ui-egui/src/grid.rs | 8 +++-- crates/ui-egui/src/keymap.rs | 5 +++ crates/ui-egui/tests/keyboard_shortcuts.rs | 36 ++++++++++++++++++++++ docs/ui-parity.md | 1 + 5 files changed, 49 insertions(+), 2 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 03830fb..4e55ad3 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Shift+Space and Ctrl+Space select the row / column without starting an edit; Control+Space on Mac (#338) | | 2026-10-11 | Legacy Excel 97-2003 `.xls` (BIFF8) import with formulas, number formats, merges, sizes and names (#121) | | 2026-10-11 | XLSX pass-through: custom XML, external links, connections, query tables, printer settings, form/ActiveX controls and OLE objects are kept on save (#330, #324) | | 2026-10-11 | Data › Forecast › Forecast Sheet: a new sheet with the history, FORECAST.ETS forecast, confidence bounds, statistics and a chart (#322) | diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 820acf7..9965a5c 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2192,6 +2192,11 @@ fn keyboard(app: &mut SheetApp, ctx: &egui::Context, resp: &egui::Response, geo: m.set_focus_lock_filter(resp.id, egui::EventFilter { tab: true, horizontal_arrows: true, vertical_arrows: true, escape: true }) }); let events = ctx.input(|i| i.events.clone()); + // A shortcut key that also types a character (Shift+Space sends Text(" ")) must not start an edit. + let shortcut_pressed = events.iter().any(|e| { + matches!(e, egui::Event::Key { pressed: true, modifiers: m, key, .. } + if !m.alt && (m.ctrl || m.command || (m.shift && *key == egui::Key::Space))) + }); let page = ((geo.cells.height() / geo.z) / sh.default_row_height).floor().max(1.0) as i64; for ev in events { match ev { @@ -2202,6 +2207,7 @@ fn keyboard(app: &mut SheetApp, ctx: &egui::Context, resp: &egui::Response, geo: app.copy_to_clipboard(ctx, "edit.cut"); } egui::Event::Paste(text) => app.paste_external(text), + egui::Event::Text(_) if shortcut_pressed => {} egui::Event::Text(text) if text.chars().all(|c| !c.is_control()) && !text.is_empty() => { app.begin_edit(Some(text), false); if let Some(ed) = app.editor.as_mut() { @@ -2270,8 +2276,6 @@ fn keyboard(app: &mut SheetApp, ctx: &egui::Context, resp: &egui::Response, geo: app.session.format_painter = None; } Key::Space if !cmd && !shift && is_checkbox(sh, app) => app.run_or_alert("cell.toggleCheckbox", json!({})), - Key::Space if cmd && !shift => app.run_or_alert("selection.column", json!({})), - Key::Space if shift && !cmd => app.run_or_alert("selection.row", json!({})), Key::F9 => app.run_or_alert("formulas.calculateNow", json!({})), Key::F4 if !cmd => {} _ if cmd => crate::ribbon::shortcut(app, key, m), diff --git a/crates/ui-egui/src/keymap.rs b/crates/ui-egui/src/keymap.rs index 54ab319..65dbcca 100644 --- a/crates/ui-egui/src/keymap.rs +++ b/crates/ui-egui/src/keymap.rs @@ -108,6 +108,11 @@ pub fn lookup(key: Key, physical: Option, m: Modifiers, mac: bool) -> Optio Period if cmd && s => run("home.increaseFontSize"), Comma if cmd && s => run("home.decreaseFontSize"), Space if cmd && s => run("edit.selectAll"), + // Shift+Space selects the row; Ctrl+Space (Windows) / Control+Space (Excel for Mac) the + // column. Cmd+Space stays free: it is the macOS Spotlight shortcut. + Space if plain && s => run("selection.row"), + Space if !mac && cmd && !s => run("selection.column"), + Space if mac && ctrl && !m.command && !s => run("selection.column"), Home if cmd && s => Shortcut::ExtendToStart, End if cmd && s => Shortcut::ExtendToEnd, Home if plain && s => Shortcut::ExtendToRowStart, diff --git a/crates/ui-egui/tests/keyboard_shortcuts.rs b/crates/ui-egui/tests/keyboard_shortcuts.rs index a06f918..8a2b195 100644 --- a/crates/ui-egui/tests/keyboard_shortcuts.rs +++ b/crates/ui-egui/tests/keyboard_shortcuts.rs @@ -106,3 +106,39 @@ fn windows_and_mac_variants() { assert_eq!(lookup(Key::A, Some(Key::A), Modifiers::NONE, false), None); assert_eq!(lookup(Key::F2, Some(Key::F2), Modifiers::NONE, false), None); } + +#[test] +fn space_selection_shortcuts_do_not_start_an_edit() { + let mut h = harness(book("B2")); + // Shift+Space: the key press, then the space the OS types with it. + let shift = Modifiers::SHIFT; + h.input_mut().events.push(Event::Key { key: Key::Space, physical_key: Some(Key::Space), pressed: true, repeat: false, modifiers: shift }); + h.input_mut().events.push(Event::Text(" ".into())); + h.run_steps(2); + assert!(h.state().editor.is_none(), "Shift+Space started an edit"); + assert_eq!(h.state().session.active().unwrap().selection.ranges[0].a1(), "2:2"); + h.state_mut().session.run("selection.set", json!({"range": "B2"})).unwrap(); + // Ctrl+Space selects the column, also without starting an edit. + h.input_mut().events.push(Event::Key { key: Key::Space, physical_key: Some(Key::Space), pressed: true, repeat: false, modifiers: CTRL }); + h.input_mut().events.push(Event::Text(" ".into())); + h.run_steps(2); + assert!(h.state().editor.is_none()); + assert_eq!(h.state().session.active().unwrap().selection.ranges[0].a1(), "B:B"); + // Plain typing still edits. + h.input_mut().events.push(Event::Text("x".into())); + h.run_steps(2); + assert!(h.state().editor.is_some()); +} + +#[test] +fn space_shortcuts_per_platform() { + let run = |id: &'static str| Some(Shortcut::Run(id, json!({}))); + let mac_cmd = Modifiers { mac_cmd: true, command: true, ..Modifiers::NONE }; + let space = |m, mac| lookup(Key::Space, Some(Key::Space), m, mac); + assert_eq!(space(Modifiers::SHIFT, false), run("selection.row")); + assert_eq!(space(Modifiers::SHIFT, true), run("selection.row")); + assert_eq!(space(CTRL, false), run("selection.column")); + assert_eq!(space(Modifiers::CTRL, true), run("selection.column")); + // Cmd+Space is Spotlight on macOS. + assert_eq!(space(mac_cmd, true), None); +} diff --git a/docs/ui-parity.md b/docs/ui-parity.md index fcea332..b6e3b91 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -150,6 +150,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-11 | minor | Shift+Space / Ctrl+Space no longer start an edit; Control+Space selects the column on Mac (#338) | | 2026-10-11 | minor | Selected whole columns can be reordered by dragging their headers to an insertion point | | 2026-10-11 | minor | Page Layout view with rulers and in-place headers and footers (#329) | | 2026-10-11 | minor | Forecast Sheet dialog (#322) | From 8587c2ba14433b10d3fbd6b9dc30b4c5c26a98dc Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 21:36:05 +0200 Subject: [PATCH 26/27] Review: Protect Sheet can't replace a sheet's password without unprotecting it Re-protecting a password-protected sheet would drop its hash and let a plain Unprotect succeed; it is now refused. The sheet tab menu offers Unprotect Sheet on a protected sheet, like the ribbon. Refs #346 Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/review.rs | 9 +++++++++ crates/ui-egui/src/tabs.rs | 5 +++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index 0dc5962..c55aac8 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -132,6 +132,12 @@ fn convert_notes(s: &mut Session, _: &Json) -> Result { fn protect_sheet(s: &mut Session, p: &Json) -> Result { let sheet = target_sheet(s, p)?; + // Re-protecting a password-protected sheet would replace the password without knowing it: + // unprotect it first, as in Excel. One without a password can take new options directly. + let has_password = |pr: &SheetProtection| pr.password.is_some() || pr.legacy_password_hash.is_some(); + if s.doc()?.wb.sheet(sheet).and_then(|sh| sh.protection.as_ref()).is_some_and(has_password) { + return Err(EngineError::Other("The sheet is protected with a password. Unprotect it first (Review › Unprotect Sheet).".into())); + } let b = |k: &str| bool_param(p, k).unwrap_or(false); let prot = SheetProtection { password: None, @@ -401,6 +407,9 @@ mod tests { assert!(matches!(&prot.password, Some(PasswordHash::Iterated { algorithm, .. }) if algorithm == "SHA-512")); assert!(s.execute("review.unprotectSheet", json!({})).is_err()); assert!(s.execute("review.unprotectSheet", json!({"password": "pass"})).is_err()); + // Protecting again can't swap the password out from under it. + assert!(s.execute("review.protectSheet", json!({})).is_err()); + assert_eq!(s.doc().unwrap().wb.active().unwrap().protection.as_ref(), Some(&prot)); s.execute("review.unprotectSheet", json!({"password": "pa55"})).unwrap(); assert!(!s.doc().unwrap().wb.active().unwrap().is_protected()); // A GridCraft JSON workbook saved before carried the internal hash under `password_hash`. diff --git a/crates/ui-egui/src/tabs.rs b/crates/ui-egui/src/tabs.rs index 6e68ab5..62a9940 100644 --- a/crates/ui-egui/src/tabs.rs +++ b/crates/ui-egui/src/tabs.rs @@ -164,8 +164,9 @@ pub fn sheet_tabs(app: &mut SheetApp, ui: &mut Ui) { app.run_or_alert("sheet.unhide", json!({})); } ui.separator(); - if ui.button("Protect Sheet…").clicked() { - app.open_dialog("protectSheet", json!({})); + let protected = app.session.active().and_then(|d| d.wb.active().map(|s| s.is_protected())).unwrap_or(false); + if ui.button(if protected { "Unprotect Sheet…" } else { "Protect Sheet…" }).clicked() { + app.open_dialog(if protected { "unprotectSheet" } else { "protectSheet" }, json!({})); } if ui.button("Select All Sheets").clicked() { ui.close(); From fca801c17e0ec88379839c99dafed25e5c38b603 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 11 Oct 2026 15:43:11 -0400 Subject: [PATCH 27/27] INDEX: compare the result size with saturating u64 math, so it can't overflow on 32-bit targets (review fix-up for #400) Co-Authored-By: Claude Opus 5.5 --- crates/calc/src/eval.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index adaeeba..a7c677d 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -1112,7 +1112,7 @@ impl<'h> Evaluator<'h> { }; let (ra, ca) = (as_array(&rows), as_array(&cols)); let (h, w) = (ra.rows.max(ca.rows), ra.cols.max(ca.cols)); - if (h * w) as u64 > MAX_CELLS { + if (h as u64).saturating_mul(w as u64) > MAX_CELLS { return err(CellError::Num); } let mut out = Vec::with_capacity(h * w);