diff --git a/Cargo.lock b/Cargo.lock index f772f83..7d0b77e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1555,6 +1555,7 @@ dependencies = [ "log", "serde", "serde_json", + "skrifa", "sys-locale", "toml", "web-sys", @@ -1584,7 +1585,7 @@ dependencies = [ "gridcraft-core", "gridcraft-formula", "gridcraft-model", - "quick-xml 0.38.4", + "quick-xml", "thiserror 2.0.21", "zip", ] @@ -2739,15 +2740,6 @@ version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a993555f31e5a609f617c12db6250dedcac1b0a85076912c436e6fc9b2c8e6a3" -[[package]] -name = "quick-xml" -version = "0.38.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b66c2058c55a409d601666cffe35f04333cf1013010882cec174a7467cd4e21c" -dependencies = [ - "memchr", -] - [[package]] name = "quick-xml" version = "0.41.0" @@ -3725,7 +3717,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "338e30461b3a2b67d70eb30a6d89f8e0c93a833e07d2ae89085cd070c4a00ac0" dependencies = [ "proc-macro2", - "quick-xml 0.41.0", + "quick-xml", "quote", ] diff --git a/Cargo.toml b/Cargo.toml index d7fad0f..fb1d798 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ serde_json = { version = "1", features = ["float_roundtrip"] } log = "0.4" rayon = "1" regex = { version = "1", default-features = false, features = ["std", "unicode", "perf"] } -quick-xml = "0.38" +quick-xml = "0.41" zip = { version = "3", default-features = false, features = ["deflate"] } image = { version = "0.25", default-features = false, features = ["png", "jpeg"] } @@ -45,6 +45,9 @@ rfd = "0.17" arboard = { version = "3.6", default-features = false } # The desktop's preferred language, for the first-run interface language (native only). sys-locale = "0.3" +# Font parser egui's epaint uses; the UI validates system font files with it before handing them +# to egui, which panics on a file it can't parse. Same version as epaint's, so no second copy. +skrifa = { version = "0.44", default-features = false, features = ["std"] } [workspace.lints.rust] unsafe_code = "forbid" diff --git a/ROADMAP.md b/ROADMAP.md index 8817691..c195cff 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -131,6 +131,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-11 | Startup no longer panics on a system font egui can't parse: fonts are validated and skipped (#226) | | 2026-10-10 | Ribbon translated into Japanese, Chinese, Korean, Brazilian Portuguese and Russian with system-language default (#31, #104, #82); CJK/Thai/Hebrew font fallback (#38); clear messages for encrypted and `.xls` files (#17); smooth line charts (#62). Progress docs restructured to the craftrules standard; full re-measure (this page). Initial ribbon key tips (#43), in-place text box editing (#84), copy as HTML table (#36). ODS and XLSB data import (#100, #101); recalc linear in the number of range formulas (#66); RISC-V Linux build; Flatpak repository on Pages; web cell-commit trap fixed | | 2026-10-09 | Community fixes: spill range operator `A1#`, sheet-scoped names, protection refuses locked-cell edits, Insert/Delete Cells adjust references, `DATE(1900,2,29)`, SUMIF sizing, `ROUND` at 15 digits, `_xlpm.` LAMBDA parameters in XLSX, vertical scrollbar, system appearance, border previews; v0.4.0 | | 2026-10-08 | v0.2.0 and v0.3.0 releases; Flatpak, AppImage zsync, branded DMG; desktop log file; DirectX 12 default on Windows | diff --git a/apps/gridcraft/src/main.rs b/apps/gridcraft/src/main.rs index 3c73a95..dc70a09 100644 --- a/apps/gridcraft/src/main.rs +++ b/apps/gridcraft/src/main.rs @@ -165,6 +165,9 @@ fn services() -> Services { } fn icon() -> Option { + #[cfg(target_os = "macos")] + let bytes = include_bytes!("../../../assets/app-icon/gridcraft-macos-512.png"); + #[cfg(not(target_os = "macos"))] let bytes = include_bytes!("../../../assets/app-icon/hicolor/256x256/apps/ai.storyteller.gridcraft.png"); let img = image::load_from_memory(bytes).ok()?.to_rgba8(); Some(egui::IconData { width: img.width(), height: img.height(), rgba: img.into_raw() }) diff --git a/contributors/people.toml b/contributors/people.toml index 8086cf1..90b4cda 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -29,5 +29,15 @@ sync_github_name = false [people.edurbs] sync_github_name = true +[people.vangel76] +real_name = "Valentin Klein" +sync_github_name = true + +[people.MacJedi42] +sync_github_name = true + +[people.alexanderalber] +real_name = "Alexander Alber" + [people.Losminek] sync_github_name = true diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index eff659d..2c0dbeb 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -554,7 +554,20 @@ impl<'h> Evaluator<'h> { if args.len() < 2 { return err(CellError::Value); } - let i = match self.arg_num(args, 0, 0.0) { + let index = self.value(&args[0]); + if let Value::Array(_) = index { + // An array of indexes picks element by element: `CHOOSE({1,2},A1:A3,B1:B3)` + // is a two-column array. The library CHOOSE lifts over its arguments. + let Some(spec) = gridcraft_functions::lookup(name) else { return err(CellError::Name) }; + let mut evaluated = vec![Arg { value: index, from_ref: false }]; + for a in &args[1..] { + let value = self.value(a); + evaluated.push(Arg { value, from_ref: false }); + } + let mut ctx = FnCtx { ev: self }; + return gridcraft_functions::call(spec, &evaluated, &mut ctx).pipe(Ev::V); + } + let i = match index.to_number() { Ok(n) => n.trunc() as i64, Err(e) => return err(e), }; diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index d1e8576..a7f1faa 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -219,6 +219,25 @@ fn ifs_and_lazy() { assert_eq!(t.get("B4"), Value::from("five")); } +#[test] +fn choose_with_an_array_index() { + let mut t = T::new(); + for (i, (k, v)) in [(1, 10), (2, 20), (3, 30)].iter().enumerate() { + t.set(&format!("A{}", i + 1), &k.to_string()); + t.set(&format!("B{}", i + 1), &v.to_string()); + } + t.set("D1", "=CHOOSE({1,2},\"a\",\"b\")"); + assert_eq!(t.get("D1"), Value::from("a")); + assert_eq!(t.get("E1"), Value::from("b")); + // The usual "look left" idiom: a two-column array from two ranges. + t.set("D3", "=VLOOKUP(20,CHOOSE({1,2},B1:B3,A1:A3),2,FALSE)"); + assert_eq!(t.num("D3"), 2.0); + t.set("D4", "=SUM(CHOOSE({1,2},A1:A3,B1:B3))"); + assert_eq!(t.num("D4"), 66.0); + t.set("D5", "=CHOOSE(2,A1:A3,B1:B3)"); + assert_eq!(t.num("D7"), 30.0); +} + #[test] fn references_functions() { let mut t = T::new(); diff --git a/crates/core/src/addr.rs b/crates/core/src/addr.rs index 8a64910..2bdfe79 100644 --- a/crates/core/src/addr.rs +++ b/crates/core/src/addr.rs @@ -30,14 +30,14 @@ impl CellRef { } /// Moved by `(dr, dc)`, `None` when the result leaves the sheet. pub fn offset(&self, dr: i64, dc: i64) -> Option { - let r = self.row as i64 + dr; - let c = self.col as i64 + dc; + let r = (self.row as i64).checked_add(dr)?; + let c = (self.col as i64).checked_add(dc)?; if (0..MAX_ROWS as i64).contains(&r) && (0..MAX_COLS as i64).contains(&c) { Some(CellRef::new(r as u32, c as u32)) } else { None } } /// Moved by `(dr, dc)` and clamped to the sheet. pub fn offset_clamped(&self, dr: i64, dc: i64) -> CellRef { - let r = (self.row as i64 + dr).clamp(0, MAX_ROWS as i64 - 1); - let c = (self.col as i64 + dc).clamp(0, MAX_COLS as i64 - 1); + let r = (self.row as i64).saturating_add(dr).clamp(0, MAX_ROWS as i64 - 1); + let c = (self.col as i64).saturating_add(dc).clamp(0, MAX_COLS as i64 - 1); CellRef::new(r as u32, c as u32) } /// Parses `A1`, `$A$1`, `xfd1048576` (case-insensitive). `None` if malformed or out of range. @@ -272,6 +272,21 @@ mod tests { assert_eq!(a.iter().count(), 9); } + #[test] + fn extreme_offsets() { + let cell = CellRef::new(1, 1); + for delta in [i64::MIN, i64::MAX] { + assert_eq!(cell.offset(delta, 0), None); + assert_eq!(cell.offset(0, delta), None); + assert_eq!(cell.offset(delta, delta), None); + } + assert_eq!(cell.offset_clamped(i64::MAX, i64::MAX), CellRef::new(MAX_ROWS - 1, MAX_COLS - 1)); + assert_eq!(cell.offset_clamped(i64::MIN, i64::MIN), CellRef::new(0, 0)); + assert_eq!(cell.offset_clamped(i64::MAX, i64::MIN), CellRef::new(MAX_ROWS - 1, 0)); + assert_eq!(cell.offset_clamped(i64::MIN, i64::MAX), CellRef::new(0, MAX_COLS - 1)); + assert_eq!(cell.offset(2, -1), Some(CellRef::new(3, 0))); + } + #[test] fn offsets() { assert_eq!(CellRef::new(0, 0).offset(-1, 0), None); diff --git a/crates/engine/src/cf.rs b/crates/engine/src/cf.rs index a102863..25fa49f 100644 --- a/crates/engine/src/cf.rs +++ b/crates/engine/src/cf.rs @@ -90,10 +90,7 @@ fn lerp(a: [u8; 3], b: [u8; 3], t: f64) -> [u8; 3] { ] } -/// Number of icons in a set name like `3TrafficLights1`, `5Quarters`. -pub fn icon_count(set: &str) -> usize { - set.chars().next().and_then(|c| c.to_digit(10)).unwrap_or(3) as usize -} +pub use gridcraft_model::icon_count; impl CfCache { pub fn new() -> Self { @@ -261,11 +258,18 @@ impl CfCache { CfRule::IconSet { set, thresholds, reverse, show_value } => { if let Some(n) = v.as_f64() { let k = icon_count(set); - let cuts: Vec = if thresholds.len() + 1 == k { - thresholds.iter().map(|t| threshold(wb, sheet, c, &st, t)).collect() - } else { - (1..k).map(|i| percentile(&st.sorted, i as f64 * 100.0 / k as f64)).collect() + // One threshold per icon, the first being the lower bound (as in the file); + // also accept the lower bound left out, else Excel's defaults. + let defaults; + let lower = match thresholds.len() { + n if n == k => thresholds.get(1..).unwrap_or_default(), + n if n + 1 == k => thresholds.as_slice(), + _ => { + defaults = default_icon_thresholds(set); + defaults.get(1..).unwrap_or_default() + } }; + let cuts: Vec = lower.iter().map(|t| threshold(wb, sheet, c, &st, t)).collect(); let mut idx = cuts.iter().filter(|cut| n >= **cut).count(); if *reverse { idx = k - 1 - idx.min(k - 1); diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index fd50afb..7372380 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -248,7 +248,7 @@ fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec, by_co let pos = |v: &Value| v.as_text().and_then(|t| list.iter().position(|x| x.eq_ignore_ascii_case(t))).unwrap_or(usize::MAX); pos(va).cmp(&pos(vb)) } else if case && va.is_text() && vb.is_text() { - va.as_text().unwrap_or("").cmp(vb.as_text().unwrap_or("")) + sort_compare(va, vb).then_with(|| case_order(va.as_text().unwrap_or(""), vb.as_text().unwrap_or(""))) } else { sort_compare(va, vb) }; @@ -653,6 +653,12 @@ pub fn filter_values(wb: &Workbook, sheet: usize, col: u32) -> Vec<(String, bool seen.into_iter().map(|((_, t), on)| (t, on)).collect() } +/// Case-sensitive tie-break for text that is equal ignoring case: as in Excel, lowercase sorts +/// before uppercase at the first letter that differs in case. +fn case_order(a: &str, b: &str) -> Ordering { + a.chars().zip(b.chars()).find(|(x, y)| x != y).map_or(Ordering::Equal, |(x, y)| y.is_lowercase().cmp(&x.is_lowercase())) +} + fn sort_key(v: &Value) -> (u8, i64, String) { match v { Value::Number(n) => (0, (n * 1e6) as i64, String::new()), @@ -685,8 +691,11 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result { let Some(sh) = cx.wb.sheet(sheet) else { return Ok(()) }; let mut seen = std::collections::HashSet::new(); let mut keep: Vec = Vec::new(); + // Cells are compared by the text they show, as in Excel: the same date in two formats + // isn't a duplicate, while 1.01 and 1.02 shown as 1.0 are. + let shown = |c: CellRef| crate::display::format(&sh.value(c), cx.wb.styles.get(sh.style_id(c)).num_fmt.as_str(), &cx.wb).text; for row in start..=r.end.row { - let key: Vec = cols.iter().map(|c| sh.value(CellRef::new(row, *c)).display().to_lowercase()).collect(); + let key: Vec = cols.iter().map(|c| shown(CellRef::new(row, *c)).to_lowercase()).collect(); if seen.insert(key) { keep.push(row); } else { diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index bb54fe0..2dba66f 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -1,6 +1,6 @@ //! Cell entry, selection, undo/redo, clipboard, clear, fill, find & replace. -use gridcraft_core::{CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value}; +use gridcraft_core::{CellRef, MAX_ROWS, RangeRef, Value}; use gridcraft_model::{Cell, Formula, StyleId}; use serde_json::{Value as Json, json}; @@ -408,9 +408,9 @@ fn selection_move(s: &mut Session, p: &Json) -> Result { if !extend && let Some(m) = sheet.merge_at(from) { // Leaving a merged cell moves from its edge. if dr > 0 { - t = CellRef::new((m.end.row + dr as u32).min(MAX_ROWS - 1), from.col); + t = CellRef::new(m.end.offset_clamped(dr, 0).row, from.col); } else if dc > 0 { - t = CellRef::new(from.row, (m.end.col + dc as u32).min(MAX_COLS - 1)); + t = CellRef::new(from.row, m.end.offset_clamped(0, dc).col); } } t diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6bd03d5..bb63d67 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -606,6 +606,27 @@ fn hidden_formulas_stay_hidden_on_protected_sheets() { assert_eq!(r["results"][0]["value"], "7"); } +#[test] +fn case_sensitive_sort_puts_lowercase_first() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["b"], ["B"], ["Ab"], ["a"], ["aB"], ["A"]]})).unwrap(); + s.execute("data.sort", json!({"range": "A1:A6", "header": false, "matchCase": true, "keys": [{"column": "A"}]})).unwrap(); + let col: Vec = (1..=6).map(|r| v(&s, &format!("A{r}"))).collect(); + assert_eq!(col, ["a", "A", "aB", "Ab", "b", "B"].map(Value::from)); +} + +#[test] +fn remove_duplicates_compares_shown_text() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Date", "Price"], ["2006-03-08", 1.01], ["2006-03-08", 1.02]]})).unwrap(); + s.execute("home.numberFormat", json!({"range": "A3", "code": "mmm d, yyyy"})).unwrap(); + s.execute("home.numberFormat", json!({"range": "B2:B3", "code": "0.0"})).unwrap(); + let r = s.execute("data.removeDuplicates", json!({"range": "A1:A3", "header": true})).unwrap(); + assert_eq!(r["removed"], 0); + let r = s.execute("data.removeDuplicates", json!({"range": "B1:B3", "header": true})).unwrap(); + assert_eq!(r["removed"], 1); +} + #[test] fn formatting_and_styles() { let mut s = s(); @@ -1633,3 +1654,32 @@ fn notes_comments_and_links_survive_a_json_save() { assert_eq!(after.hyperlinks, before.hyperlinks); assert_eq!(after.cells, before.cells); } + +/// Icon-set thresholds as read from a file (one per icon, the first the lower bound) decide the +/// icon; a rule without thresholds uses Excel's defaults (percent 33 and 67) (#235). +#[test] +fn icon_set_thresholds_from_files_pick_the_icon() { + use gridcraft_model::{CfRule, CfValueKind, CondFormat}; + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [[0], [10], [20], [30], [100]]})).unwrap(); + s.execute("home.conditionalFormat", json!({"range": "A1:A5", "rule": {"type": "iconSet", "set": "3Arrows"}})).unwrap(); + let icon = |s: &Session, a: &str| { + let wb = &s.doc().unwrap().wb; + crate::cf::CfCache::new().look(wb, 0, CellRef::parse(a).unwrap()).and_then(|l| l.icon).map(|(_, i)| i) + }; + // Defaults: percent of the 0..100 span, so 30 is still the lowest icon. + assert_eq!([icon(&s, "A1"), icon(&s, "A4"), icon(&s, "A5")], [Some(0), Some(0), Some(2)]); + let sh = std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).sheet_mut(0).unwrap(); + sh.cond_formats = vec![CondFormat { + ranges: vec![gridcraft_core::RangeRef::parse("A1:A5").unwrap()], + rule: CfRule::IconSet { + set: "3Arrows".into(), + thresholds: vec![CfValueKind::Percent(0.0), CfValueKind::Number(10.0), CfValueKind::Number(25.0)], + reverse: false, + show_value: true, + }, + priority: 1, + stop_if_true: false, + }]; + assert_eq!([icon(&s, "A1"), icon(&s, "A2"), icon(&s, "A3"), icon(&s, "A4")], [Some(0), Some(1), Some(1), Some(2)]); +} diff --git a/crates/engine/tests/extreme_selection_moves.rs b/crates/engine/tests/extreme_selection_moves.rs new file mode 100644 index 0000000..f6f5737 --- /dev/null +++ b/crates/engine/tests/extreme_selection_moves.rs @@ -0,0 +1,40 @@ +use gridcraft_core::{CellRef, MAX_COLS, MAX_ROWS}; +use gridcraft_engine::Session; +use serde_json::json; + +#[test] +fn selection_move_clamps_extreme_displacements() { + let mut session = Session::new(); + session.new_workbook(); + for (dr, dc, expected) in [ + (i64::MAX, 0, CellRef::new(MAX_ROWS - 1, 1)), + (0, i64::MAX, CellRef::new(1, MAX_COLS - 1)), + (i64::MAX, i64::MAX, CellRef::new(MAX_ROWS - 1, MAX_COLS - 1)), + (i64::MIN, i64::MIN, CellRef::new(0, 0)), + (i64::MAX, i64::MIN, CellRef::new(MAX_ROWS - 1, 0)), + (i64::MIN, i64::MAX, CellRef::new(0, MAX_COLS - 1)), + (2, -1, CellRef::new(3, 0)), + ] { + session.execute("selection.set", json!({"cell": "B2"})).unwrap(); + session.execute("selection.move", json!({"dr": dr, "dc": dc})).unwrap(); + assert_eq!(session.doc().unwrap().selection.active, expected); + } +} + +#[test] +fn selection_move_from_a_merged_cell_clamps_extreme_displacements() { + let mut session = Session::new(); + session.new_workbook(); + session.execute("selection.set", json!({"range": "B2:C3"})).unwrap(); + session.execute("home.mergeCells", json!({})).unwrap(); + for (dr, dc, expected) in [ + (i64::MAX, 0, CellRef::new(MAX_ROWS - 1, 1)), + (0, i64::MAX, CellRef::new(1, MAX_COLS - 1)), + (4_294_967_297, 0, CellRef::new(MAX_ROWS - 1, 1)), + (1, 0, CellRef::new(3, 1)), + ] { + session.execute("selection.set", json!({"cell": "B2"})).unwrap(); + session.execute("selection.move", json!({"dr": dr, "dc": dc})).unwrap(); + assert_eq!(session.doc().unwrap().selection.active, expected); + } +} diff --git a/crates/formula/src/parser.rs b/crates/formula/src/parser.rs index 754e986..ee2354e 100644 --- a/crates/formula/src/parser.rs +++ b/crates/formula/src/parser.rs @@ -236,6 +236,13 @@ impl Parser { { return Ok(Expr::Unary(UnOp::Spill, Box::new(r))); } + // Files write `@x` as `_xlfn.SINGLE(x)`. + if name == "SINGLE" + && args.len() == 1 + && let Some(x) = args.pop() + { + return Ok(Expr::Unary(UnOp::At, Box::new(x))); + } Ok(Expr::Call(name, args)) } Tok::Sheet(a, b) => { diff --git a/crates/functions/src/criteria.rs b/crates/functions/src/criteria.rs index 49d84a3..20410c8 100644 --- a/crates/functions/src/criteria.rs +++ b/crates/functions/src/criteria.rs @@ -127,9 +127,9 @@ impl Criterion { }, Operand::Text(pat) => match self.op { Op::Eq | Op::Ne => { + // Blank cells hold no text: `"*"` skips them and `"<>*"` counts them. let cell_text = match &v { Value::Text(t) => Some(t.to_string()), - Value::Empty => Some(String::new()), _ => None, }; let m = cell_text.is_some_and(|t| wildcard_match(pat, &t)); @@ -298,6 +298,13 @@ mod tests { assert!(lookup_equal(&Value::Number(0.3), &Value::Number(0.1 + 0.2), false)); assert!(m("<>a*", Value::from("bcd"))); assert!(m(">1/1/2020", Value::Number(44000.0))); + // Text patterns never match blank cells, but an empty string is text. + assert!(!m("*", Value::Empty)); + assert!(m("*", Value::from(""))); + assert!(m("<>*", Value::Empty)); + assert!(!m("<>*", Value::from(""))); + assert!(!m("?*", Value::Empty)); + assert!(m("<>a*", Value::Empty)); } #[test] diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index b9fe2ce..2d2518d 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -196,6 +196,18 @@ pub enum CfRule { }, } +/// Number of icons in an icon set (`3Arrows` → 3, `5Quarters` → 5), always 3 to 5. +pub fn icon_count(set: &str) -> usize { + set.chars().next().and_then(|c| c.to_digit(10)).unwrap_or(3).clamp(3, 5) as usize +} + +/// Excel's default icon-set thresholds: one per icon, the first being the lower bound, +/// then even percent steps (`0, 33, 67` for three icons). +pub fn default_icon_thresholds(set: &str) -> Vec { + let k = icon_count(set); + (0..k).map(|i| CfValueKind::Percent((i as f64 * 100.0 / k as f64).round())).collect() +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct CondFormat { pub ranges: Vec, diff --git a/crates/numfmt/src/decimal.rs b/crates/numfmt/src/decimal.rs index 15cfcd3..67071d4 100644 --- a/crates/numfmt/src/decimal.rs +++ b/crates/numfmt/src/decimal.rs @@ -9,17 +9,70 @@ pub(crate) const MAX_DECIMALS: usize = 400; /// The 15 significant digits of a positive finite number and the power of ten of the first /// digit: `a ≈ d0.d1d2… × 10^exp`. +/// +/// The value is rounded to the nearest 15-digit decimal. An exact tie at the 16th digit (the exact +/// decimal expansion is 15 digits, a 5, then only zeros, as for 1234567890123455 or +/// 123456789012345.5) goes toward zero, as Excel does: `1234567890123455` shows `1234567890123450`, +/// not `…460`. Excel observed (Windows 16.0.20430, TEXT()). pub(crate) fn decimal_digits(a: f64) -> ([u8; 15], i32) { let mut digits = [0u8; 15]; if !a.is_finite() || a <= 0.0 { return (digits, 0); } - let s = format!("{:.14e}", a); - let (mant, exp) = s.split_once('e').unwrap_or((s.as_str(), "0")); - for (slot, b) in digits.iter_mut().zip(mant.bytes().filter(u8::is_ascii_digit)) { - *slot = b - b'0'; + // 16 digits, rounded. Unless the 16th is a 5 it decides the 15th exactly (4 or less rounds + // down, 6 or more up), so one format serves almost every number. + let (d16, exp) = significant_digits(a, 16); + for (slot, d) in digits.iter_mut().zip(&d16) { + *slot = *d; } - (digits, exp.parse().unwrap_or(0)) + match d16.get(15).copied().unwrap_or(0) { + 0..=4 => (digits, exp), + 5 if is_exact_tie(a) => (digits, exp), + 5 => { + // Not a tie: round the exact value (the 16-digit rounding above may be a double rounding). + let (d15, exp15) = significant_digits(a, 15); + let mut out = [0u8; 15]; + for (slot, d) in out.iter_mut().zip(&d15) { + *slot = *d; + } + (out, exp15) + } + _ => { + let mut exp = exp; + let mut carry = true; + for d in digits.iter_mut().rev() { + if *d == 9 { + *d = 0; + } else { + *d += 1; + carry = false; + break; + } + } + if carry { + digits = [0; 15]; + digits[0] = 1; + exp += 1; + } + (digits, exp) + } + } +} + +/// `n` significant digits of `a` (rounded half to even on the exact binary value) and the power of +/// ten of the first. +fn significant_digits(a: f64, n: usize) -> (Vec, i32) { + let s = format!("{:.*e}", n.saturating_sub(1), a); + let (mant, exp) = s.split_once('e').unwrap_or((s.as_str(), "0")); + (mant.bytes().filter(u8::is_ascii_digit).map(|b| b - b'0').collect(), exp.parse().unwrap_or(0)) +} + +/// True when `a`'s exact decimal expansion has a 5 as its 16th significant digit and only zeros after. +fn is_exact_tie(a: f64) -> bool { + let is_tie = |ds: &[u8]| ds.get(15) == Some(&5) && ds.get(16..).is_some_and(|t| t.iter().all(|&d| d == 0)); + // A cheap screen first (30 digits), then the exact expansion: a double has at most 767 + // significant digits and Rust prints every one of them. + is_tie(&significant_digits(a, 30).0) && is_tie(&significant_digits(a, 800).0) } /// Rounds the number `digits × 10^exp` (see [`decimal_digits`]) to `decimals` places, half away diff --git a/crates/numfmt/src/tests.rs b/crates/numfmt/src/tests.rs index ea40753..0742b26 100644 --- a/crates/numfmt/src/tests.rs +++ b/crates/numfmt/src/tests.rs @@ -421,3 +421,73 @@ fn fuzz_no_panic() { } } } + +// ---- 15 significant digits: an exact tie at the 16th digit goes toward zero ---- +// Windows Excel 16.0.20430 TEXT() over the exact doubles below. An odd 15th digit is where +// round-half-even and toward-zero differ; an even one is where they agree. + +#[test] +fn an_exact_tie_at_the_sixteenth_digit_goes_toward_zero() { + for (v, whole) in [ + (1234567890123405.0, "1234567890123400"), + (1234567890123415.0, "1234567890123410"), + (1234567890123425.0, "1234567890123420"), + (1234567890123435.0, "1234567890123430"), + (1234567890123455.0, "1234567890123450"), + (1234567890123475.0, "1234567890123470"), + (1234567890123495.0, "1234567890123490"), + (6631679258820775.0, "6631679258820770"), + (8052991635850695.0, "8052991635850690"), + ] { + assert_eq!(f("0", v), whole, "0 over {v}"); + assert_eq!(f("0.00", v), format!("{whole}.00"), "0.00 over {v}"); + assert_eq!(f("0", -v), format!("-{whole}"), "0 over -{v}"); + } + assert_eq!(f("#,##0", 1234567890123455.0), "1,234,567,890,123,450"); + assert_eq!(f("#,##0", -3267527853107975.0), "-3,267,527,853,107,970"); + // A fraction is a tie as well, when it is exact in binary. + assert_eq!(f("0", 123456789012345.5), "123456789012345"); + assert_eq!(f("0.00", 123456789012345.5), "123456789012345.00"); + // Excel observed: no carry after fifteen 9s. + assert_eq!(f("0", 999999999999999.5), "999999999999999"); + assert_eq!(f("0", -999999999999999.5), "-999999999999999"); + assert_eq!(f("0", 4999999999999995.0), "4999999999999990"); + assert_eq!(f("#,##0", -4999999999999995.0), "-4,999,999,999,999,990"); +} + +#[test] +fn only_exact_ties_go_toward_zero() { + // Just above and just below a tie round to the nearer 15-digit value (Excel observed). + assert_eq!(f("0", 1234567890123455.0 + 0.25), "1234567890123460"); + assert_eq!(f("0", 1234567890123455.5), "1234567890123460"); + assert_eq!(f("0", 1234567890123455.0 - 0.25), "1234567890123450"); + // 16 digits that are not a tie, and the ordinary decimal cases, are untouched. + assert_eq!(f("0", 1234567890123456.0), "1234567890123460"); + assert_eq!(f("0", 1234567890123454.0), "1234567890123450"); + assert_eq!(f("0.00", 2.675), "2.68"); + assert_eq!(f("0.00", 1.005), "1.01"); + assert_eq!(f("0.0", 0.285), "0.3"); + assert_eq!(f("0.0", 99999999999999.95), "100000000000000.0"); + // Ten digits and a half are not a 16th-digit tie. + assert_eq!(f("0", 0.5), "1"); + assert_eq!(f("0", 2.5), "3"); +} + +#[test] +fn decimal_digits_keeps_fifteen_digits_and_truncates_a_tie() { + use crate::decimal::decimal_digits; + let (d, e) = decimal_digits(1234567890123455.0); + assert_eq!((d, e), ([1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5], 15)); + let (d, e) = decimal_digits(1234567890123456.0); + assert_eq!((d, e), ([1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 6], 15)); + let (d, e) = decimal_digits(123456789012345.5); + assert_eq!((d, e), ([1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5], 14)); + // Excel observed: a tie after fifteen 9s does not carry (999999999999999.5 shows 999999999999999). + let (d, e) = decimal_digits(999999999999999.5); + assert_eq!((d, e), ([9; 15], 14)); + let (d, e) = decimal_digits(4999999999999995.0); + assert_eq!((d, e), ([4, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9], 15)); + // A 16th digit above 5 carries as usual. + let (d, e) = decimal_digits(999999999999999.6); + assert_eq!((d, e), ([1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], 15)); +} diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index a6a2da2..4f44fd9 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -16,6 +16,7 @@ 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"] } diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 7dd3acb..8d6c8ad 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -810,7 +810,8 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { if ui.button("Cancel").clicked() { open = false; } - if ui.button("Save").clicked() { + // File operations must include the cell the user is still editing (as the File > Save menu item already does). + if ui.button("Save").clicked() && app.commit_edit(0, 0, false, false) { app.run_or_alert("file.save", json!({})); if app.session.active().is_some_and(|x| !x.is_dirty()) { app.run_or_alert("file.close", json!({"force": true})); diff --git a/crates/ui-egui/src/formula_bar.rs b/crates/ui-egui/src/formula_bar.rs index 6cca110..a693ee9 100644 --- a/crates/ui-egui/src/formula_bar.rs +++ b/crates/ui-egui/src/formula_bar.rs @@ -112,6 +112,10 @@ fn name_box(app: &mut SheetApp, ui: &mut egui::Ui, t: &Tokens) { } if resp.lost_focus() { if ui.input(|i| i.key_pressed(Key::Enter)) { + // The Enter that commits the Name Box is used up here. Left in the frame's input, the + // grid (drawn after the formula bar, and focused again now the box let go) would read + // it as its own Enter and move the active cell one row down from the jump target (#247). + ui.input_mut(|i| i.events.retain(|e| !matches!(e, egui::Event::Key { key: Key::Enter, .. }))); let target = text.trim().to_string(); if !target.is_empty() && target != shown { // A new name for the selection, or a place to go. diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 264479a..5c0d7f1 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -44,7 +44,10 @@ pub fn title_bar(app: &mut SheetApp, ui: &mut Ui) { if icon_button(ui, Icon::Home, t.text_dim, "Home", vec2(26.0, 26.0)).clicked() { app.open_dialog("start", json!({})); } - if icon_button(ui, Icon::Save, t.text_dim, "Save (⌘S)", vec2(26.0, 26.0)).clicked() { + if icon_button(ui, Icon::Save, t.text_dim, "Save (⌘S)", vec2(26.0, 26.0)).clicked() + // File operations must include the cell the user is still editing (as the File > Save menu item already does). + && app.commit_edit(0, 0, false, false) + { app.run_or_alert("file.save", json!({})); } let can_undo = app.session.active().is_some_and(|d| !d.undo.is_empty()); @@ -332,10 +335,13 @@ fn home(app: &mut SheetApp, ui: &mut Ui) { // Keep a keyboard-opened menu anchored to the button when a pointer click // cancels keytips, so its items remain in place and can still be clicked. let anchor_id = paste.id.with("keytip_anchor"); + // Query response state before taking the egui context write lock. + // Calling secondary_clicked() inside data_mut can deadlock. + let paste_secondary_clicked = paste.secondary_clicked(); let anchored = ui.ctx().data_mut(|data| { let anchored = if paste_keys { true - } else if paste.secondary_clicked() { + } else if paste_secondary_clicked { false } else { data.get_temp::(anchor_id).unwrap_or(false) diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 25b9ca8..fa938c4 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -166,10 +166,28 @@ pub fn cell_family(name: &str, bold: bool, italic: bool) -> FontFamily { ) } +/// Whether epaint can open face `index` of this font file and it maps characters to glyphs. +/// epaint panics (in `Context::set_fonts`, i.e. on startup) on a face it can't open, so every font +/// file we did not ship is checked first with the same parser and the same call epaint makes +/// (`skrifa::FontRef::from_index`): a corrupt file, something that is not an sfnt at all, or a +/// collection index past the end is rejected instead of crashing the app (#226). A face whose +/// character map is unreadable (a truncated file) would open but draw nothing, so it is refused +/// too and the next candidate gets its turn. +pub(crate) fn font_parses(bytes: &[u8], index: u32) -> bool { + use skrifa::MetadataProvider; + skrifa::FontRef::from_index(bytes, index).is_ok_and(|font| font.charmap().has_map()) +} + +/// Loads the first candidate that exists and parses. A file that exists but can't be parsed is +/// skipped with a warning and the next candidate is tried. #[cfg(not(target_arch = "wasm32"))] fn try_load(paths: &[(&str, u32)]) -> Option { for (p, index) in paths { if let Ok(bytes) = std::fs::read(p) { + if !font_parses(&bytes, *index) { + log::warn!("skipping font {p} (face {index}): not a font file egui can read"); + continue; + } let mut fd = FontData::from_owned(bytes); fd.index = *index; return Some(fd); @@ -178,6 +196,21 @@ fn try_load(paths: &[(&str, u32)]) -> Option { None } +/// Last line of defence before [`FontDefinitions`] reach egui: drops every face epaint could not +/// parse (and its key from every family), so a bad font costs a fallback, never a crash. +pub(crate) fn drop_unparseable_fonts(fonts: &mut FontDefinitions) { + let bad: Vec = fonts.font_data.iter().filter(|(_, fd)| !font_parses(&fd.font, fd.index)).map(|(k, _)| k.clone()).collect(); + for key in &bad { + log::warn!("dropping font {key:?}: egui can't parse it"); + fonts.font_data.remove(key); + } + if !bad.is_empty() { + for chain in fonts.families.values_mut() { + chain.retain(|k| !bad.contains(k)); + } + } +} + #[cfg(target_arch = "wasm32")] fn try_load(_paths: &[(&str, u32)]) -> Option { None @@ -340,6 +373,7 @@ pub fn font_definitions_for_language(language: crate::i18n::Language) -> FontDef if let Some(ui) = fonts.families.get(&FontFamily::Name(UI.into())).cloned() { fonts.families.insert(FontFamily::Proportional, ui); } + drop_unparseable_fonts(&mut fonts); fonts } @@ -652,4 +686,51 @@ mod tests { } } } + + #[test] + fn unparseable_font_files_are_rejected_not_fatal() { + // #226: epaint panics in `set_fonts` on a face it can't parse ("An offset was out of + // bounds"). Garbage, a truncated real font and a bad collection index must all be refused. + let real = FontDefinitions::default().font_data.values().next().map(|fd| fd.font.to_vec()).unwrap_or_default(); + assert!(font_parses(&real, 0), "egui's own font must parse"); + assert!(!font_parses(b"", 0)); + assert!(!font_parses(&[0xAB; 64], 0)); + assert!(!font_parses(real.get(..20).unwrap_or_default(), 0), "a truncated font must be refused"); + assert!(!font_parses(&real, 3), "a single-face file has no face 3"); + + // A bad face that slipped into the definitions is dropped (key and family entries), and the + // context builds its fonts without panicking. + let mut fonts = font_definitions(); + fonts.font_data.insert("sys-hebrew".into(), Arc::new(FontData::from_owned(vec![0, 1, 0, 0, 0, 0xFF, 0, 0]))); + for chain in fonts.families.values_mut() { + chain.push("sys-hebrew".into()); + } + drop_unparseable_fonts(&mut fonts); + assert!(!fonts.font_data.contains_key("sys-hebrew")); + assert!(fonts.families.values().all(|c| !c.iter().any(|k| k == "sys-hebrew"))); + let ctx = egui::Context::default(); + ctx.set_fonts(fonts); + let mut out = ctx.run_ui(egui::RawInput::default(), |ui| { + ui.label("Hebrew עברית"); + }); + out.textures_delta.clear(); // nothing consumes the atlas in a headless test + } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn try_load_skips_a_broken_candidate() { + // A candidate that exists but is not a readable font must not win over a good one behind it. + let dir = std::env::temp_dir().join(format!("gridcraft-font-test-{}", std::process::id())); + let _ = std::fs::create_dir_all(&dir); + let bad = dir.join("broken.ttf"); + let good = dir.join("good.ttf"); + let real = FontDefinitions::default().font_data.values().next().map(|fd| fd.font.to_vec()).unwrap_or_default(); + let _ = std::fs::write(&bad, real.get(..40).unwrap_or_default()); + let _ = std::fs::write(&good, &real); + let (bad_s, good_s) = (bad.to_string_lossy().into_owned(), good.to_string_lossy().into_owned()); + assert!(try_load(&[(bad_s.as_str(), 0)]).is_none()); + let loaded = try_load(&[(bad_s.as_str(), 0), (good_s.as_str(), 0)]); + assert_eq!(loaded.map(|fd| fd.font.len()), Some(real.len())); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index 1b71c82..839a370 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -125,6 +125,26 @@ fn space_toggles_the_active_checkbox_without_clicking_the_hovered_cell() { assert!(h.state().editor.is_none()); } +#[test] +fn name_box_enter_goes_to_the_typed_cell_not_the_one_below() { + // #247: typing A1 in the Name Box and pressing Enter selected A2; the grid replayed the Enter. + let mut h = harness(blank()); + h.state_mut().session.run("selection.set", json!({"cell": "C5"})).unwrap(); + h.run_steps(2); + h.ctx.memory_mut(|m| m.request_focus(egui::Id::new("gridcraft.name_box"))); + h.run_steps(2); + h.input_mut().events.push(Event::Key { key: Key::A, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::COMMAND }); + h.input_mut().events.push(Event::Text("A1".into())); + h.run_steps(2); + key(&mut h, Key::Enter, Modifiers::NONE); + h.run_steps(2); + assert_eq!(h.state().session.active().unwrap().selection.active, CellRef::new(0, 0)); + assert!(h.state().name_box.is_none()); + // The grid has the focus back: a plain Enter there still moves down. + key(&mut h, Key::Enter, Modifiers::NONE); + assert_eq!(h.state().session.active().unwrap().selection.active, CellRef::new(1, 0)); +} + #[test] fn typing_enters_values_and_moves_down() { let mut h = harness(blank()); diff --git a/crates/xlsx/src/array_formula.rs b/crates/xlsx/src/array_formula.rs index 2512cc0..01c9d92 100644 --- a/crates/xlsx/src/array_formula.rs +++ b/crates/xlsx/src/array_formula.rs @@ -24,7 +24,10 @@ pub(crate) fn needs_array(wb: &Workbook, sheet: usize, at: CellRef, formula: &Fo let Some(expr) = formula.expr_arc() else { return false }; let mut scan = Scan { wb, at, remaining: 4096, locals: Vec::new() }; let shape = scan.expr(&expr, sheet, false, 0); - shape.array || shape.affected + // An explicit `@` is written as `_xlfn.SINGLE`, which only dynamic-array formulas carry. + let mut at = false; + expr.walk(&mut |e| at |= matches!(e, Expr::Unary(UnOp::At, _))); + shape.array || shape.affected || at } struct Scan<'a> { diff --git a/crates/xlsx/src/fmla.rs b/crates/xlsx/src/fmla.rs index 018cbf2..9ac4ccf 100644 --- a/crates/xlsx/src/fmla.rs +++ b/crates/xlsx/src/fmla.rs @@ -78,6 +78,8 @@ fn expr_to_file(e: Expr) -> String { }, // Files have no `#` operator: Excel writes `A1#` as `_xlfn.ANCHORARRAY(A1)`. Expr::Unary(gridcraft_formula::UnOp::Spill, r) => Expr::Call("_xlfn.ANCHORARRAY".into(), vec![*r]), + // Nor an `@` operator: Excel writes `@x` as `_xlfn.SINGLE(x)`. + Expr::Unary(gridcraft_formula::UnOp::At, x) => Expr::Call("_xlfn.SINGLE".into(), vec![*x]), other => other, }); gridcraft_formula::print(&e) @@ -115,6 +117,15 @@ mod tests { assert_eq!(from_file("SUM(_xlfn.ANCHORARRAY(Sheet1!$D$1))+_xlfn.ANCHORARRAY(B2)"), "SUM(Sheet1!$D$1#)+B2#"); } + #[test] + fn implicit_intersection() { + assert_eq!(to_file(&Formula::new("SUM(@A1:A3*B1:B3)")), "SUM(_xlfn.SINGLE(A1:A3)*B1:B3)"); + assert_eq!(to_file(&Formula::new("@A1:A3*2")), "_xlfn.SINGLE(A1:A3)*2"); + assert_eq!(text_to_file("=@Sheet1!$A$1:$A$3"), "_xlfn.SINGLE(Sheet1!$A$1:$A$3)"); + assert_eq!(from_file("_xlfn.SINGLE(A1:A3)*2"), "@A1:A3*2"); + assert_eq!(from_file("SUM(_xlfn.SINGLE(A1:A3)*B1:B3)"), "SUM(@A1:A3*B1:B3)"); + } + #[test] fn prefixes() { assert_eq!(to_file(&Formula::new("XLOOKUP(1,A:A,B:B)+SUM(A1)")), "_xlfn.XLOOKUP(1,A:A,B:B)+SUM(A1)"); diff --git a/crates/xlsx/src/ods.rs b/crates/xlsx/src/ods.rs index 4f63fd1..8415ce3 100644 --- a/crates/xlsx/src/ods.rs +++ b/crates/xlsx/src/ods.rs @@ -10,6 +10,7 @@ use gridcraft_core::date::{days_in_month, serial_from_ymd}; use gridcraft_core::{CellRef, DateSystem, MAX_COLS, MAX_ROWS, RangeRef, Value}; use gridcraft_model::{Cell, NumFmt, Sheet, StyleId, Workbook}; use quick_xml::NsReader; +use quick_xml::XmlVersion; use quick_xml::events::{BytesStart, Event}; use quick_xml::name::ResolveResult; @@ -67,13 +68,13 @@ fn is_ns(ns: ResolveResult<'_>, expected: &[u8]) -> bool { matches!(ns, ResolveResult::Bound(n) if n.as_ref() == expected) } -fn attr(reader: &NsReader<&[u8]>, e: &BytesStart<'_>, ns: &[u8], name: &[u8]) -> Result, IoError> { +fn attr(reader: &mut NsReader<&[u8]>, e: &BytesStart<'_>, ns: &[u8], name: &[u8]) -> Result, IoError> { let mut found = None; for a in e.attributes() { let a = a.map_err(|e| IoError::Xml(e.to_string()))?; - let (resolved, local) = reader.resolve_attribute(a.key); + let (resolved, local) = reader.resolver_mut().resolve_attribute(a.key); if is_ns(resolved, ns) && local.as_ref() == name { - let value = a.unescape_value().map_err(|e| IoError::Xml(e.to_string()))?; + let value = a.normalized_value(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?; if value.len() > MAX_CELL_TEXT { return Err(IoError::TooLarge("ODS attribute exceeds 1 MiB".into())); } @@ -106,7 +107,7 @@ fn check_manifest(bytes: &[u8]) -> Result<(), IoError> { let event = reader.read_event().map_err(xml_error)?; match &event { Event::Start(e) | Event::Empty(e) => { - let (ns, name) = reader.resolve_element(e.name()); + let (ns, name) = reader.resolver_mut().resolve_element(e.name()); if is_ns(ns.clone(), MANIFEST) && name.as_ref() == b"encryption-data" { return Err(IoError::Format("encrypted ODS files are not supported".into())); } @@ -153,8 +154,8 @@ enum Tag { Other, } -fn tag(reader: &NsReader<&[u8]>, e: &BytesStart<'_>) -> Tag { - let (ns, name) = reader.resolve_element(e.name()); +fn tag(reader: &mut NsReader<&[u8]>, e: &BytesStart<'_>) -> Tag { + let (ns, name) = reader.resolver_mut().resolve_element(e.name()); let local = name.as_ref(); if is_ns(ns.clone(), OFFICE) { return match local { @@ -253,23 +254,23 @@ impl Import { return Err(xml_error("XML nested too deeply")); } if self.stack.is_empty() { - if root_seen || tag(&reader, &e) != Tag::Document { + if root_seen || tag(&mut reader, &e) != Tag::Document { return Err(xml_error("expected one office:document-content root")); } root_seen = true; } - self.start(&reader, &e)?; + self.start(&mut reader, &e)?; } Event::Empty(e) => { if self.stack.is_empty() { return Err(xml_error("expected office:document-content with a spreadsheet")); } - self.start(&reader, &e)?; + self.start(&mut reader, &e)?; self.end()?; } Event::End(_) => self.end()?, Event::Text(t) => { - let text = t.xml_content().map_err(xml_error)?; + let text = t.xml_content(XmlVersion::Explicit1_0).map_err(xml_error)?; if self.stack.is_empty() && !text.trim().is_empty() { return Err(xml_error("text outside the document")); } @@ -301,7 +302,7 @@ impl Import { Ok(()) } - fn start(&mut self, reader: &NsReader<&[u8]>, e: &BytesStart<'_>) -> Result<(), IoError> { + fn start(&mut self, reader: &mut NsReader<&[u8]>, e: &BytesStart<'_>) -> Result<(), IoError> { let t = tag(reader, e); let depth = self.stack.len(); match t { diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index b1e3fca..313839f 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -6,7 +6,7 @@ use std::fmt::Write as _; use gridcraft_core::{CellError, CellRef, RangeRef, Value}; use gridcraft_model::{ AutoFilter, Cell, CfOperator, CfRule, CfValueKind, Color, CondFormat, ErrorStyle, FilterCriterion, LineInfo, Orientation, Sheet, SparklineKind, - Style, Table, TotalsFn, Validation, ValidationKind, Workbook, + Style, Table, TotalsFn, Validation, ValidationKind, Workbook, default_icon_thresholds, icon_count, }; use crate::drawing::{Obj, image_content_type, image_ext}; @@ -778,7 +778,15 @@ fn cf_rule_xml(cf: &CondFormat, priority: u32, out: &mut Out) -> String { b.push_str(" showValue=\"0\""); } b.push('>'); - for t in thresholds { + // CT_IconSet needs one cfvo per icon (minOccurs 2); the first is the lower bound. + let k = icon_count(set); + let defaults = default_icon_thresholds(set); + let cfvos: Vec<&CfValueKind> = match thresholds.len() { + n if n == k => thresholds.iter().collect(), + n if n + 1 == k => defaults.iter().take(1).chain(thresholds).collect(), + _ => defaults.iter().collect(), + }; + for t in cfvos { b.push_str(&cfvo_xml(t)); } b.push_str(""); diff --git a/crates/xlsx/src/tests/array_export.rs b/crates/xlsx/src/tests/array_export.rs index d92f473..4bed1e5 100644 --- a/crates/xlsx/src/tests/array_export.rs +++ b/crates/xlsx/src/tests/array_export.rs @@ -236,8 +236,10 @@ fn implicit_intersection_metadata_respects_function_argument_context() { ("SUM(A1:A3)", false), ("SUMPRODUCT(A1:A3*B1:B3)", false), ("SUMPRODUCT(SQRT(A1:A3))", false), - ("SQRT(@A1:A3)", false), - ("@A1:A3*2", false), + // An explicit `@` is written as `_xlfn.SINGLE`, a dynamic-array formula. Dropping it + // instead would need the reader to put it back on legacy formulas. + ("SQRT(@A1:A3)", true), + ("@A1:A3*2", true), ("@(A1:A3*2)", true), ("SQRT(INDIRECT(\"A1\"))", false), ("SUM(INDIRECT(\"A1:A3\"))", false), diff --git a/crates/xlsx/src/tests/ods_tests.rs b/crates/xlsx/src/tests/ods_tests.rs index c0f2e0e..47c2586 100644 --- a/crates/xlsx/src/tests/ods_tests.rs +++ b/crates/xlsx/src/tests/ods_tests.rs @@ -23,6 +23,22 @@ fn text(s: &str) -> Value { Value::Text(s.into()) } +/// A start tag with thousands of `xmlns:` declarations is refused instead of allocated +/// (RUSTSEC-2026-0195: the resolver kept a binding per declaration, with no cap). +#[test] +fn ods_namespace_declaration_bomb_is_rejected() { + let bomb = |n: usize| -> String { (0..n).map(|i| format!(" xmlns:p{i}=\"urn:p{i}\"")).collect() }; + let doc = |decls: &str| { + format!( + r#"1"# + ) + }; + let build = |decls: &str| make_zip(&[("mimetype", MIME), ("META-INF/manifest.xml", MANIFEST), ("content.xml", doc(decls).as_bytes())]); + let ok = read_ods(&build(&bomb(8))); + assert!(ok.is_ok(), "a handful of namespaces still reads: {:?}", ok.err()); + assert!(read_ods(&build(&bomb(10_000))).is_err(), "a tag with thousands of declarations is refused"); +} + #[test] fn ods_typed_values_and_formula_caches_are_constants_and_survive_xlsx() { let bytes = row(r#" diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 12bba62..f8d695b 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -600,3 +600,74 @@ fn spill_and_calc_errors_are_cached_as_value_errors() { assert_eq!(cell.child("v").unwrap().text, value); } } + +/// `CT_IconSet` needs one `cfvo` per icon (at least two); a rule without stored thresholds +/// gets Excel's defaults, one without the lower bound gets it prepended (#235). +#[test] +fn icon_sets_always_write_one_cfvo_per_icon() { + let mut wb = Workbook::new(); + let s = wb.sheet_mut(0).unwrap(); + for (i, (set, thresholds)) in [ + ("3Arrows", vec![]), + ("4Rating", vec![CfValueKind::Number(2.0), CfValueKind::Number(5.0), CfValueKind::Number(8.0)]), + ("9Bogus", vec![CfValueKind::Percent(50.0)]), + ] + .into_iter() + .enumerate() + { + s.cond_formats.push(CondFormat { + ranges: vec![rr(&format!("A{}", i + 1))], + rule: CfRule::IconSet { set: set.into(), thresholds, reverse: false, show_value: true }, + priority: i as u32 + 1, + stop_if_true: false, + }); + } + let bytes = write_xlsx(&wb).unwrap(); + let (xml, _) = sheet1_xml(&bytes); + assert!( + xml.contains( + "" + ), + "{xml}" + ); + assert!( + xml.contains(""), + "{xml}" + ); + assert!(xml.contains(""), "{xml}"); + let (back, _) = read_xlsx(&bytes).unwrap(); + let rules: Vec = back + .sheet(0) + .unwrap() + .cond_formats + .iter() + .map(|cf| match &cf.rule { + CfRule::IconSet { thresholds, .. } => thresholds.len(), + _ => 0, + }) + .collect(); + assert_eq!(rules, [3, 4, 5]); +} + +/// Files have no `@` operator: Excel writes `@x` as `_xlfn.SINGLE(x)` in a dynamic-array +/// formula, and reads it back as `@x`. +#[test] +fn implicit_intersection_is_written_as_single() { + let mut wb = Workbook::new(); + let s = wb.sheet_mut(0).unwrap(); + for (r, n) in [(0, 1.0), (1, 3.0), (2, 5.0)] { + s.set_value(CellRef::new(r, 0), Value::Number(n)); + } + s.cells.set(at("D3"), Cell { value: Value::Number(10.0), ..Cell::formula(Formula::new("@A1:A3*2")) }); + s.cells.set(at("D4"), Cell { value: Value::Number(2.0), ..Cell::formula(Formula::new("A1*2")) }); + let bytes = write_xlsx(&wb).unwrap(); + let (xml, metadata) = sheet1_xml(&bytes); + assert!(xml.contains("_xlfn.SINGLE(A1:A3)*210"), "{xml}"); + assert!(xml.contains("A1*2"), "{xml}"); + assert!(!xml.contains('@'), "{xml}"); + assert!(metadata); + let (back, _) = read_xlsx(&bytes).unwrap(); + let sh = back.sheet(0).unwrap(); + assert_eq!(sh.cell(at("D3")).and_then(|c| c.formula.as_ref()).map(|f| f.text.as_str()), Some("@A1:A3*2")); + assert_eq!(sh.cell(at("D3")).and_then(|c| c.formula.as_ref()).and_then(|f| f.array), None); +} diff --git a/crates/xlsx/src/xlsb.rs b/crates/xlsx/src/xlsb.rs index 897a40b..3c84547 100644 --- a/crates/xlsx/src/xlsb.rs +++ b/crates/xlsx/src/xlsb.rs @@ -7,6 +7,7 @@ use std::sync::Arc; use gridcraft_core::{CellError, CellRef, DateSystem, MAX_COLS, MAX_ROWS, RangeRef, Value}; use gridcraft_model::{Cell, Sheet, Visibility, Workbook}; use quick_xml::NsReader; +use quick_xml::XmlVersion; use quick_xml::events::Event; use quick_xml::name::ResolveResult; @@ -160,7 +161,7 @@ fn metadata(bytes: &[u8], ns: &[u8], root_name: &[u8]) -> Result, IoE let event = reader.read_event().map_err(|e| IoError::Xml(e.to_string()))?; match &event { Event::Start(e) | Event::Empty(e) => { - let (namespace, local) = reader.resolve_element(e.name()); + let (namespace, local) = reader.resolver_mut().resolve_element(e.name()); let valid_ns = matches!(namespace, ResolveResult::Bound(n) if n.as_ref() == ns); if depth == 0 { if seen || !valid_ns || local.as_ref() != root_name { @@ -174,7 +175,7 @@ fn metadata(bytes: &[u8], ns: &[u8], root_name: &[u8]) -> Result, IoE if !a.key.as_ref().contains(&b':') { attrs.push(( String::from_utf8_lossy(a.key.as_ref()).into_owned(), - a.unescape_value().map_err(|e| IoError::Xml(e.to_string()))?.into_owned(), + a.normalized_value(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?.into_owned(), )); } } @@ -188,7 +189,7 @@ fn metadata(bytes: &[u8], ns: &[u8], root_name: &[u8]) -> Result, IoE } } Event::End(_) => depth = depth.checked_sub(1).ok_or_else(|| invalid("unexpected metadata end"))?, - Event::Text(t) if depth == 0 && !t.xml_content().map_err(|e| IoError::Xml(e.to_string()))?.trim().is_empty() => { + Event::Text(t) if depth == 0 && !t.xml_content(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?.trim().is_empty() => { return Err(invalid("text outside package metadata")); } Event::DocType(_) => return Err(invalid("DOCTYPE in package metadata is unsupported")), diff --git a/crates/xlsx/src/xml.rs b/crates/xlsx/src/xml.rs index efa5cdc..9e03f5f 100644 --- a/crates/xlsx/src/xml.rs +++ b/crates/xlsx/src/xml.rs @@ -2,6 +2,7 @@ //! shared strings) so large parts never need a full tree, plus escaping helpers for writing. use quick_xml::Reader; +use quick_xml::XmlVersion; use quick_xml::events::{BytesStart, Event}; use crate::IoError; @@ -124,7 +125,7 @@ fn start_el(e: &BytesStart<'_>) -> El { if key == b"xmlns" || key.starts_with(b"xmlns:") { continue; } - let v = match a.unescape_value() { + let v = match a.normalized_value(XmlVersion::Explicit1_0) { Ok(v) => v.into_owned(), Err(_) => String::from_utf8_lossy(&a.value).into_owned(), }; @@ -203,7 +204,7 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res } Event::Text(t) => { if let Some(top) = stack.last_mut() { - let s = t.xml_content().map_err(|e| IoError::Xml(e.to_string()))?; + let s = t.xml_content(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?; top.text.push_str(&s); } }