Skip to content

Commit e938c9e

Browse files
fix(calc): NOW/TODAY clock at autofill/paste/command, formatted-value tests
### NOW/TODAY clock at user-action boundaries beyond editing Tier 2 fixed the interactive-edit path (editing.rs). The other user- action eval sites had the same NOW/TODAY divergence on multi-cell ops: * autofill.rs — both row and column autofill paths * clipboard.rs — multi-cell paste + single-cell paste * command.rs — `:table create` initial value eval Each now wraps its `evaluator.evaluate_formula` call in `with_recalc_clock(now_serial(), || ...)` so any volatile cells in the filled / pasted / created formulas see the same clock the immediately- following auto-recalc will. Previously, an autofilled column of `=NOW()` formulas could briefly flash with multiple distinct wall-clock times before the recalc collapsed them onto a single snapshot — now the filled values agree from the start. ### Expanded executor-parity fuzz to 6 formula shapes Was: only `+ - *` on cell refs. Now picks per cell from: * arithmetic chain (original) * IF with `>`-comparison branches * SUM over a B-column helper sub-range * MAX over a B-column helper sub-range * ABS of a referenced cell * VLOOKUP (approximate-match) into the helper column A small mirror block in column B (cells 0..helpers) gives range-aware functions something contiguous to consult. 20 random seeds still pass — the property is "Sequential and Parallel agree on every cell," and the expanded coverage exercises far more of the executor's per-purity dispatch surface. ### Number-formatting scenario + 2 product fixes it caught New scenario `formatting` puts (raw value, format command) pairs into column A and asserts the rendered cell text contains the expected formatted substring (e.g. raw=9876.54 + currency → "$9,876.54"). Framework gains a `rendered_text_checks` method on Scenario (defaults empty) and a `RenderedTextCheck` type. Runner asserts post-recalc and post-auto-fit. The scenario caught two real product bugs: * **Currency rendered sign in wrong place.** -42.5 with currency format displayed as `$-42.50`. Excel convention is `-$42.50` — sign BEFORE the symbol. Fixed in `style::format_cell_value` (Currency branch): format the absolute value, then prepend sign. * **Auto-resize ignored formatted width.** `auto_resize_column` and `auto_resize_all_columns` measured `cell.value.width()` (raw), so a `9876.54` raw value sized to 7 chars even though the currency-formatted display needs 9. Both helpers now call `format_cell_value` to size by what the user actually sees. Tests: 592 lib + 19 PTY scenarios (+1 formatting) + all 11 PTY suites green. Zero warnings.
1 parent 807b54d commit e938c9e

9 files changed

Lines changed: 357 additions & 49 deletions

File tree

src/application/state/autofill.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,13 @@ impl App {
105105
continue;
106106
}
107107

108-
let new_value = evaluator.evaluate_formula(&adjusted_formula);
108+
// Publish a clock so any NOW()/TODAY() in the autofilled
109+
// formula uses the same snapshot the auto-recalc will,
110+
// and so all cells filled in this batch agree on time.
111+
let new_value = crate::domain::parser::with_recalc_clock(
112+
crate::domain::parser::now_serial(),
113+
|| evaluator.evaluate_formula(&adjusted_formula),
114+
);
109115
changes.push((*target_row, col, CellData {
110116
value: new_value,
111117
formula: Some(adjusted_formula),
@@ -189,7 +195,11 @@ impl App {
189195
continue;
190196
}
191197

192-
let new_value = evaluator.evaluate_formula(&adjusted_formula);
198+
// See row-autofill path above for the clock-publish rationale.
199+
let new_value = crate::domain::parser::with_recalc_clock(
200+
crate::domain::parser::now_serial(),
201+
|| evaluator.evaluate_formula(&adjusted_formula),
202+
);
193203
changes.push((row, *target_col, CellData {
194204
value: new_value,
195205
formula: Some(adjusted_formula),

src/application/state/clipboard.rs

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -202,27 +202,33 @@ impl App {
202202
let dest_row = self.selected_row;
203203
let dest_col = self.selected_col;
204204

205-
// Compute all new cells first (evaluator borrows spreadsheet immutably)
206-
let new_cells: Vec<_> = {
207-
let evaluator = crate::domain::FormulaEvaluator::new(self.workbook.current_sheet());
208-
clipboard.cells.iter().filter_map(|(row_off, col_off, cell)| {
209-
let target_row = dest_row + row_off;
210-
let target_col = dest_col + col_off;
211-
if target_row >= self.workbook.current_sheet().rows || target_col >= self.workbook.current_sheet().cols {
212-
return None;
213-
}
214-
let new_cell = if let Some(ref formula) = cell.formula {
215-
let row_offset = target_row as i32 - (clipboard.source_row + row_off) as i32;
216-
let col_offset = target_col as i32 - (clipboard.source_col + col_off) as i32;
217-
let adjusted = evaluator.adjust_formula_references(formula, row_offset, col_offset);
218-
let value = evaluator.evaluate_formula(&adjusted);
219-
CellData { value, formula: Some(adjusted), format: cell.format.clone(), comment: cell.comment.clone(), spill_anchor: None }
220-
} else {
221-
cell.clone()
222-
};
223-
Some((target_row, target_col, new_cell))
224-
}).collect()
225-
};
205+
// Compute all new cells first (evaluator borrows spreadsheet immutably).
206+
// One clock snapshot for the whole paste so any pasted =NOW() cells
207+
// agree on time, and so they match what the auto-recalc immediately
208+
// following will use.
209+
let new_cells: Vec<_> = crate::domain::parser::with_recalc_clock(
210+
crate::domain::parser::now_serial(),
211+
|| {
212+
let evaluator = crate::domain::FormulaEvaluator::new(self.workbook.current_sheet());
213+
clipboard.cells.iter().filter_map(|(row_off, col_off, cell)| {
214+
let target_row = dest_row + row_off;
215+
let target_col = dest_col + col_off;
216+
if target_row >= self.workbook.current_sheet().rows || target_col >= self.workbook.current_sheet().cols {
217+
return None;
218+
}
219+
let new_cell = if let Some(ref formula) = cell.formula {
220+
let row_offset = target_row as i32 - (clipboard.source_row + row_off) as i32;
221+
let col_offset = target_col as i32 - (clipboard.source_col + col_off) as i32;
222+
let adjusted = evaluator.adjust_formula_references(formula, row_offset, col_offset);
223+
let value = evaluator.evaluate_formula(&adjusted);
224+
CellData { value, formula: Some(adjusted), format: cell.format.clone(), comment: cell.comment.clone(), spill_anchor: None }
225+
} else {
226+
cell.clone()
227+
};
228+
Some((target_row, target_col, new_cell))
229+
}).collect()
230+
},
231+
);
226232

227233
// Now apply changes (mutably borrows spreadsheet) — collect the
228234
// undo batch, then push all writes through `set_many` in one shot
@@ -315,7 +321,11 @@ impl App {
315321
}
316322
}
317323
let evaluator = FormulaEvaluator::new(self.workbook.current_sheet());
318-
let evaluated = evaluator.evaluate_formula(value);
324+
// Same clock-publish rationale as the multi-cell paste path.
325+
let evaluated = crate::domain::parser::with_recalc_clock(
326+
crate::domain::parser::now_serial(),
327+
|| evaluator.evaluate_formula(value),
328+
);
319329
CellData {
320330
value: evaluated,
321331
formula: Some(value.to_string()),

src/application/state/command.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -742,7 +742,12 @@ impl App {
742742
self.workbook.current_sheet(),
743743
&self.workbook.named_ranges,
744744
);
745-
let initial = evaluator.evaluate_formula(&formula);
745+
// Publish a clock for the same NOW()/TODAY()
746+
// consistency reason as the editing/autofill/paste paths.
747+
let initial = crate::domain::parser::with_recalc_clock(
748+
crate::domain::parser::now_serial(),
749+
|| evaluator.evaluate_formula(&formula),
750+
);
746751
rows.push((
747752
t.0 + 1 + i,
748753
t.1 + 1,

src/domain/models/spreadsheet.rs

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -585,11 +585,21 @@ impl Spreadsheet {
585585
/// viewport so a single wide cell can't push other columns off-screen).
586586
pub fn auto_resize_column_with_cap(&mut self, col: usize, max_cap: usize) {
587587
use unicode_width::UnicodeWidthStr;
588+
use super::style::format_cell_value;
588589
let mut max_width = Self::column_label(col).width();
589590

590591
for (&(_, c), cell) in &self.cells {
591592
if c == col {
592-
let value_width = cell.value.width();
593+
// The rendered width drives the auto-fit, not the raw
594+
// value's width. A cell with raw "9876.54" and currency
595+
// format displays as "$9,876.54" (9 chars vs 7); sizing
596+
// by raw width would clip the displayed text.
597+
let displayed = if let Some(ref fmt) = cell.format {
598+
format_cell_value(&cell.value, fmt)
599+
} else {
600+
cell.value.clone()
601+
};
602+
let value_width = displayed.width();
593603
let formula_width = cell.formula.as_ref().map(|f| f.width()).unwrap_or(0);
594604
let content_width = value_width.max(formula_width);
595605
max_width = max_width.max(content_width);
@@ -606,12 +616,19 @@ impl Spreadsheet {
606616
/// O(cols × cells) — the per-column loop iterated the entire HashMap.
607617
pub fn auto_resize_all_columns(&mut self) {
608618
use unicode_width::UnicodeWidthStr;
619+
use super::style::format_cell_value;
609620
let mut widths: HashMap<usize, usize> = HashMap::with_capacity(self.cols);
610621
for col in 0..self.cols {
611622
widths.insert(col, Self::column_label(col).width());
612623
}
613624
for (&(_, c), cell) in &self.cells {
614-
let value_width = cell.value.width();
625+
// Width the user sees, not raw — see auto_resize_column_with_cap.
626+
let displayed = if let Some(ref fmt) = cell.format {
627+
format_cell_value(&cell.value, fmt)
628+
} else {
629+
cell.value.clone()
630+
};
631+
let value_width = displayed.width();
615632
let formula_width = cell.formula.as_ref().map(|f| f.width()).unwrap_or(0);
616633
let content_width = value_width.max(formula_width);
617634
let entry = widths.entry(c).or_insert(3);

src/domain/models/style.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,18 @@ pub fn format_cell_value(value: &str, format: &CellFormat) -> String {
109109
}
110110
NumberFormat::Currency { symbol, decimals } => {
111111
if let Ok(n) = value.parse::<f64>() {
112-
let formatted = format!("{:.prec$}", n, prec = *decimals as usize);
113-
format!("{}{}", symbol, add_thousands_separator(&formatted))
112+
// Excel convention: sign goes BEFORE the currency symbol
113+
// ("-$42.50"), not between the symbol and the magnitude
114+
// ("$-42.50"). Format the absolute value, then prepend
115+
// the sign manually so the symbol always sits next to
116+
// the digits.
117+
let abs_formatted = format!("{:.prec$}", n.abs(), prec = *decimals as usize);
118+
let body = add_thousands_separator(&abs_formatted);
119+
if n < 0.0 {
120+
format!("-{}{}", symbol, body)
121+
} else {
122+
format!("{}{}", symbol, body)
123+
}
114124
} else {
115125
value.to_string()
116126
}

src/domain/services/executor.rs

Lines changed: 80 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -811,36 +811,97 @@ mod tests {
811811
let n_literals = (n_cells as u32 / 3).max(3); // ~1/3 literals
812812
let mut cells: Vec<(usize, usize, String)> = Vec::new();
813813
// Literals come first so later formulas always have something
814-
// to reference.
814+
// to reference. Mix integers and small positive numbers so
815+
// SUMPRODUCT and IF-comparison branches see varied inputs.
815816
for i in 0..n_literals {
816-
// Random integer in [-50, 50]. Avoid floats here so string
817-
// comparison is stable across the two executors.
818817
let v = ((next() % 100) as i64) - 50;
819818
cells.push((i as usize, 0, v.to_string()));
820819
}
820+
// Build a few helper cells in column B so range-aware functions
821+
// (SUM, AVERAGE, VLOOKUP) have a contiguous block to consult.
822+
// These are mirrors of column A's first ten literals.
823+
let n_helpers = n_literals.min(10) as usize;
824+
for i in 0..n_helpers {
825+
cells.push((i, 1, format!("=A{}", i + 1)));
826+
}
821827
for i in n_literals..(n_cells as u32) {
822828
let row = i as usize;
823-
// Pick 1..=3 earlier cells to combine.
824-
let n_refs = 1 + (next() % 3) as usize;
825-
let mut formula = String::from("=");
826-
for j in 0..n_refs {
827-
if j > 0 {
828-
let op = match next() % 4 {
829-
0 => '+',
830-
1 => '-',
831-
2 => '*',
832-
_ => '+', // skip '/' to avoid #DIV/0! noise
833-
};
834-
formula.push(op);
835-
}
836-
let pick = (next() as usize) % (i as usize);
837-
formula.push_str(&format!("A{}", pick + 1));
838-
}
829+
// Pick a random shape per cell — operator chain, IF, SUM,
830+
// MAX, ABS, or VLOOKUP. Each path exercises a different
831+
// executor surface; if Sequential and Parallel diverge on
832+
// any of them, the fuzz catches it.
833+
let formula = match next() % 6 {
834+
0 => gen_arith(&mut next, i),
835+
1 => gen_if(&mut next, i),
836+
2 => gen_sum_range(&mut next, i, n_helpers),
837+
3 => gen_max_range(&mut next, i, n_helpers),
838+
4 => gen_abs(&mut next, i),
839+
_ => gen_vlookup(&mut next, i, n_helpers),
840+
};
839841
cells.push((row, 0, formula));
840842
}
841843
cells
842844
}
843845

846+
fn gen_arith(next: &mut impl FnMut() -> u32, i: u32) -> String {
847+
let n_refs = 1 + (next() % 3) as usize;
848+
let mut f = String::from("=");
849+
for j in 0..n_refs {
850+
if j > 0 {
851+
let op = match next() % 4 {
852+
0 => '+',
853+
1 => '-',
854+
2 => '*',
855+
_ => '+', // skip '/' to avoid #DIV/0! noise
856+
};
857+
f.push(op);
858+
}
859+
let pick = (next() as usize) % (i as usize);
860+
f.push_str(&format!("A{}", pick + 1));
861+
}
862+
f
863+
}
864+
865+
fn gen_if(next: &mut impl FnMut() -> u32, i: u32) -> String {
866+
let a = (next() as usize) % (i as usize) + 1;
867+
let b = (next() as usize) % (i as usize) + 1;
868+
let c = (next() as usize) % (i as usize) + 1;
869+
let d = (next() as usize) % (i as usize) + 1;
870+
format!("=IF(A{}>A{},A{},A{})", a, b, c, d)
871+
}
872+
873+
fn gen_sum_range(next: &mut impl FnMut() -> u32, _i: u32, helpers: usize) -> String {
874+
if helpers < 2 { return "=0".to_string(); }
875+
// Sum a sub-range of column B (the helpers).
876+
let lo = (next() as usize) % helpers;
877+
let hi_extra = (next() as usize) % (helpers - lo);
878+
let hi = lo + hi_extra;
879+
format!("=SUM(B{}:B{})", lo + 1, hi + 1)
880+
}
881+
882+
fn gen_max_range(next: &mut impl FnMut() -> u32, _i: u32, helpers: usize) -> String {
883+
if helpers < 2 { return "=0".to_string(); }
884+
let lo = (next() as usize) % helpers;
885+
let hi_extra = (next() as usize) % (helpers - lo);
886+
let hi = lo + hi_extra;
887+
format!("=MAX(B{}:B{})", lo + 1, hi + 1)
888+
}
889+
890+
fn gen_abs(next: &mut impl FnMut() -> u32, i: u32) -> String {
891+
let pick = (next() as usize) % (i as usize) + 1;
892+
format!("=ABS(A{})", pick)
893+
}
894+
895+
fn gen_vlookup(next: &mut impl FnMut() -> u32, _i: u32, helpers: usize) -> String {
896+
if helpers < 2 { return "=0".to_string(); }
897+
// VLOOKUP into B's helper column. Use approximate-match
898+
// (TRUE) since helpers may not be sorted — Excel returns the
899+
// last-matching key in approximate mode regardless. Both
900+
// executors should behave identically.
901+
let target = (next() as usize) % helpers + 1;
902+
format!("=IFERROR(VLOOKUP(A{},B1:B{},1,TRUE),0)", target, helpers)
903+
}
904+
844905
/// Single-snapshot recalc must propagate per-level writes into the
845906
/// snapshot the next level reads from. Without that, a depth-3
846907
/// chain (A1=lit, A2=A1+1, A3=A2+1) would have A3 reading A2's

0 commit comments

Comments
 (0)