From eee986352c32f72f66abaa3c9c947d9cabe48a80 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Thu, 3 Sep 2026 17:24:42 +0800 Subject: [PATCH 1/2] Fix formatter and add block charts --- .github/workflows/rust.yaml | 3 + crates/controller/src/api/test.rs | 310 +++++ crates/controller/src/api/worksheet.rs | 98 +- .../src/calc_engine/calculator/funcs/round.rs | 197 +++- .../src/chart_manager/block_source.rs | 140 +++ .../controller/src/chart_manager/executor.rs | 80 +- crates/controller/src/chart_manager/mod.rs | 53 +- crates/controller/src/controller/display.rs | 15 + crates/controller/src/controller/executor.rs | 32 +- crates/controller/src/edit_action/mod.rs | 24 + crates/controller/src/file_loader/mod.rs | 30 +- crates/controller/src/file_saver/workbook.rs | 68 +- crates/ssf-rs/NOTICE | 27 + crates/ssf-rs/src/jsnum.rs | 98 +- crates/ssf-rs/src/writenum.rs | 108 +- crates/ssf-rs/tests/format_diff.rs | 8 + crates/ssf-rs/tests/unit.rs | 89 ++ crates/workbook/src/logisheets.rs | 29 + docs/chart.md | 35 +- .../engine/src/lib/chart/ChartSettings.svelte | 36 + packages/engine/src/lib/chart/from-info.ts | 4 +- packages/engine/src/lib/chart/types.ts | 14 + packages/logician/package.json | 6 +- packages/logician/src/index.ts | 1 + packages/logician/src/tools/charts.test.ts | 875 ++++++++++++++ packages/logician/src/tools/charts.ts | 1048 +++++++++++++++++ packages/logician/src/tools/taxonomy.ts | 2 + src/components/watson/index.tsx | 2 + tests/proptest_engine.proptest-regressions | 2 + tests/proptest_engine.rs | 56 +- yarn.lock | 133 ++- 31 files changed, 3539 insertions(+), 84 deletions(-) create mode 100644 crates/controller/src/chart_manager/block_source.rs create mode 100644 packages/logician/src/tools/charts.test.ts create mode 100644 packages/logician/src/tools/charts.ts diff --git a/.github/workflows/rust.yaml b/.github/workflows/rust.yaml index a1b6c7be..23e11e99 100644 --- a/.github/workflows/rust.yaml +++ b/.github/workflows/rust.yaml @@ -46,3 +46,6 @@ jobs: - name: Test logisheets-runtime run: yarn workspace logisheets-runtime test + + - name: Test logisheets-logician + run: yarn workspace logisheets-logician test diff --git a/crates/controller/src/api/test.rs b/crates/controller/src/api/test.rs index 0fea83a6..ecd95244 100644 --- a/crates/controller/src/api/test.rs +++ b/crates/controller/src/api/test.rs @@ -128,6 +128,7 @@ fn create_chart_from_scratch() { series_type: None, }, ], + block_source: None, })], undoable: true, init: false, @@ -4548,6 +4549,7 @@ fn chart_categories_and_formats_are_live() { size_ref: None, series_type: None, }], + block_source: None, })], undoable: true, init: false, @@ -4841,6 +4843,7 @@ fn chart_edits_are_undoable() { size_ref: None, series_type: None, }], + block_source: None, })], undoable: true, init: false, @@ -4903,6 +4906,7 @@ fn create_bubble_chart_with_live_sizes() { size_ref: Some("Sheet1!$C$1:$C$2".to_string()), series_type: None, }], + block_source: None, })], undoable: true, init: false, @@ -5034,6 +5038,7 @@ fn create_stock_of_pie_and_surface_charts() { title: None, categories_ref: Some("Sheet1!$A$1:$A$4".to_string()), series, + block_source: None, })], undoable: true, init: false, @@ -5144,6 +5149,7 @@ fn update_chart_sets_the_of_pie_split() { size_ref: None, series_type: None, }], + block_source: None, })], undoable: true, init: false, @@ -5255,6 +5261,7 @@ fn create_a_combo_chart_and_keep_it_through_edits() { series_type: Some("area".to_string()), }, ], + block_source: None, })], undoable: true, init: false, @@ -5383,6 +5390,7 @@ fn three_d_chart_types_round_trip_through_the_api() { size_ref: None, series_type: None, }], + block_source: None, })], undoable: true, init: false, @@ -5403,3 +5411,305 @@ fn three_d_chart_types_round_trip_through_the_api() { vec![Some(5.0), Some(10.0), Some(15.0), Some(20.0)] ); } + +/// A chart bound to a block plots the block, not a snapshot of where it was: +/// records appended to it appear without the chart being touched, and edits +/// elsewhere on the sheet cannot leave it pointing at the wrong cells. +#[test] +fn chart_bound_to_block_follows_it() { + use crate::edit_action::{ + BindFormSchema, ChartBlockSource, CreateBlock, InsertRows, InsertRowsInBlock, + }; + + let mut wb = Workbook::default(); + let bid = wb.get_available_block_id(0).unwrap(); + let cell = |row: usize, col: usize, content: &str| { + EditPayload::CellInput(CellInput { + sheet_idx: 0, + row, + col, + content: content.to_string(), + }) + }; + + // A 3x3 block at B2 with a row schema: name / qty / price, one record a row. + // The header that named the fields is *outside* the block, so every row of + // the block is a record. + let mut payloads = vec![ + EditPayload::CreateBlock(CreateBlock { + sheet_idx: 0, + id: bid, + master_row: 1, + master_col: 1, + row_cnt: 3, + col_cnt: 3, + owner: None, + modify_policy: None, + }), + EditPayload::BindFormSchema(BindFormSchema { + ref_name: "sales".into(), + sheet_idx: 0, + block_id: bid, + field_from: 0, + key_idx: 0, + fields: vec!["name".into(), "qty".into(), "price".into()], + render_ids: vec!["r0".into(), "r1".into(), "r2".into()], + row: true, + field_formulas: vec![], + validation_formulas: vec![], + editability_formulas: vec![], + }), + ]; + for (i, (name, qty)) in [("a", "10"), ("b", "20"), ("c", "30")].iter().enumerate() { + payloads.push(cell(1 + i, 1, name)); + payloads.push(cell(1 + i, 2, qty)); + } + wb.handle_action(EditAction::Payloads(PayloadsAction { + payloads, + undoable: false, + init: false, + })); + + wb.handle_action(EditAction::Payloads(PayloadsAction { + payloads: vec![EditPayload::CreateChart(CreateChart { + sheet_idx: 0, + chart_id: "chart1".to_string(), + chart_type: "col".to_string(), + from_row: 6, + from_col: 1, + from_col_off: 0, + from_row_off: 0, + to_row: 16, + to_col: 6, + to_col_off: 0, + to_row_off: 0, + title: Some("Sales".to_string()), + // Named fields, not ranges: the block says where they are. + categories_ref: None, + series: vec![], + block_source: Some(ChartBlockSource { + block_id: bid, + category_field: Some("name".to_string()), + value_fields: vec!["qty".to_string()], + }), + })], + undoable: true, + init: false, + })); + + { + let ws = wb.get_sheet_by_idx(0).unwrap(); + let c = &ws.get_charts()[0]; + assert_eq!(c.cat_ref.as_deref(), Some("Sheet1!$B$2:$B$4")); + assert_eq!(c.series.len(), 1); + assert_eq!(c.series[0].name.as_deref(), Some("qty")); + assert_eq!(c.series[0].val_ref.as_deref(), Some("Sheet1!$C$2:$C$4")); + assert_eq!(c.series[0].values, vec![Some(10.0), Some(20.0), Some(30.0)]); + assert_eq!(c.categories, vec!["a", "b", "c"]); + } + + // Append a record. Nothing touches the chart. + wb.handle_action(EditAction::Payloads(PayloadsAction { + payloads: vec![ + EditPayload::InsertRowsInBlock(InsertRowsInBlock { + sheet_idx: 0, + block_id: bid, + start: 3, + cnt: 1, + }), + cell(4, 1, "d"), + cell(4, 2, "40"), + ], + undoable: false, + init: false, + })); + { + let ws = wb.get_sheet_by_idx(0).unwrap(); + let c = &ws.get_charts()[0]; + assert_eq!( + c.series[0].val_ref.as_deref(), + Some("Sheet1!$C$2:$C$5"), + "the range grew with the block" + ); + assert_eq!( + c.series[0].values, + vec![Some(10.0), Some(20.0), Some(30.0), Some(40.0)] + ); + assert_eq!(c.categories, vec!["a", "b", "c", "d"]); + } + + // A row inserted above the block pushes it down. A stored A1 ref would now + // be a row short of the data; a bound one is recomputed and still right. + wb.handle_action(EditAction::Payloads(PayloadsAction { + payloads: vec![EditPayload::InsertRows(InsertRows { + sheet_idx: 0, + start: 0, + count: 1, + })], + undoable: false, + init: false, + })); + { + let ws = wb.get_sheet_by_idx(0).unwrap(); + let c = &ws.get_charts()[0]; + assert_eq!( + c.series[0].val_ref.as_deref(), + Some("Sheet1!$C$3:$C$6"), + "the range shifted with the block" + ); + assert_eq!( + c.series[0].values, + vec![Some(10.0), Some(20.0), Some(30.0), Some(40.0)] + ); + } +} + +/// The binding survives a save: the xlsx carries real A1 ranges so Excel can +/// draw the chart, and logisheets.xml carries what they were derived from, so +/// reopening here leaves the chart still following the block. +#[test] +fn block_bound_chart_survives_save() { + use crate::edit_action::{BindFormSchema, ChartBlockSource, CreateBlock, InsertRowsInBlock}; + + let mut wb = Workbook::default(); + let bid = wb.get_available_block_id(0).unwrap(); + let cell = |row: usize, col: usize, content: &str| { + EditPayload::CellInput(CellInput { + sheet_idx: 0, + row, + col, + content: content.to_string(), + }) + }; + wb.handle_action(EditAction::Payloads(PayloadsAction { + payloads: vec![ + EditPayload::CreateBlock(CreateBlock { + sheet_idx: 0, + id: bid, + master_row: 0, + master_col: 0, + row_cnt: 2, + col_cnt: 2, + owner: None, + modify_policy: None, + }), + EditPayload::BindFormSchema(BindFormSchema { + ref_name: "rec".into(), + sheet_idx: 0, + block_id: bid, + field_from: 0, + key_idx: 0, + fields: vec!["name".into(), "qty".into()], + render_ids: vec!["r0".into(), "r1".into()], + row: true, + field_formulas: vec![], + validation_formulas: vec![], + editability_formulas: vec![], + }), + cell(0, 0, "a"), + cell(0, 1, "10"), + cell(1, 0, "b"), + cell(1, 1, "20"), + EditPayload::CreateChart(CreateChart { + sheet_idx: 0, + chart_id: "chart1".to_string(), + chart_type: "col".to_string(), + from_row: 5, + from_col: 0, + from_col_off: 0, + from_row_off: 0, + to_row: 15, + to_col: 5, + to_col_off: 0, + to_row_off: 0, + title: None, + categories_ref: None, + series: vec![], + block_source: Some(ChartBlockSource { + block_id: bid, + category_field: Some("name".to_string()), + value_fields: vec!["qty".to_string()], + }), + }), + ], + undoable: false, + init: false, + })); + + let bytes = wb.save().unwrap(); + let mut wb2 = Workbook::from_file(&bytes, "reloaded".to_string()).unwrap(); + { + let ws = wb2.get_sheet_by_idx(0).unwrap(); + let c = &ws.get_charts()[0]; + assert_eq!(c.series[0].val_ref.as_deref(), Some("Sheet1!$B$1:$B$2")); + assert_eq!(c.series[0].values, vec![Some(10.0), Some(20.0)]); + } + // Still bound, not frozen: growing the reloaded block grows the chart. + let bid2 = { + let ws = wb2.get_sheet_by_idx(0).unwrap(); + ws.get_all_blocks()[0].block_id + }; + wb2.handle_action(EditAction::Payloads(PayloadsAction { + payloads: vec![ + EditPayload::InsertRowsInBlock(InsertRowsInBlock { + sheet_idx: 0, + block_id: bid2, + start: 2, + cnt: 1, + }), + cell(2, 0, "c"), + cell(2, 1, "30"), + ], + undoable: false, + init: false, + })); + let ws = wb2.get_sheet_by_idx(0).unwrap(); + let c = &ws.get_charts()[0]; + assert_eq!( + c.series[0].val_ref.as_deref(), + Some("Sheet1!$B$1:$B$3"), + "the reloaded chart is still bound to the block" + ); + assert_eq!(c.series[0].values, vec![Some(10.0), Some(20.0), Some(30.0)]); +} + +/// A cell's displayed value and `ROUND` must agree about the same number. +/// Before, the display path rounded on the exact double (JavaScript's rule, +/// inherited from the `ssf` port) while `ROUND` scaled by a power of ten, so +/// the two could disagree — and the money formats disagreed with `0.00`. +#[test] +fn text_and_round_agree_at_excel_precision() { + let mut wb = Workbook::default(); + let cases = [ + ("=TEXT(1.005,\"0.00\")", "1.01"), + ("=TEXT(1.005,\"#,##0.00\")", "1.01"), + ("=TEXT(4.935,\"0.00\")", "4.94"), + ("=TEXT(2.675,\"#,##0.00\")", "2.68"), + ("=FIXED(1.005,2)", "1.01"), + ("=DOLLAR(1.005,2)", "$1.01"), + ("=TEXT(ROUND(1.005,2),\"0.00\")", "1.01"), + ]; + let mut payloads = vec![]; + for (i, (e, _)) in cases.iter().enumerate() { + payloads.push(EditPayload::CellInput(CellInput { + sheet_idx: 0, + row: i, + col: 0, + content: e.to_string(), + })); + } + wb.handle_action(EditAction::Payloads(PayloadsAction { + payloads, + undoable: false, + init: false, + })); + let ws = wb.get_sheet_by_idx(0).unwrap(); + for (i, (e, want)) in cases.iter().enumerate() { + // `Value` has no `PartialEq`, and a formatted cell is a string anyway. + let got = match ws.get_value(i, 0).unwrap() { + crate::controller::display::Value::Str(s) => s, + other => panic!("{e} did not render as text: {other:?}"), + }; + assert_eq!(got, *want, "{e}"); + } +} diff --git a/crates/controller/src/api/worksheet.rs b/crates/controller/src/api/worksheet.rs index 5fa46001..3f6a78c1 100644 --- a/crates/controller/src/api/worksheet.rs +++ b/crates/controller/src/api/worksheet.rs @@ -722,7 +722,7 @@ impl<'a> Worksheet<'a> { /// `data` (type, series, cached values) comes from the chart's parsed /// OOXML; the frontend re-reads live values from the source ranges. pub fn get_charts(&self) -> Vec { - use logisheets_workbook::prelude::{ChartType, LegendPos}; + use logisheets_workbook::prelude::{ChartSeries, ChartType, LegendPos}; let chart_type_str = |t: &ChartType| -> String { match t { @@ -779,6 +779,47 @@ impl<'a> Worksheet<'a> { } }; let d = &chart.data; + // A chart bound to a block reads the block as it is *now*: the + // refs stored in `data` are only the last resolution, so a + // record appended to the block is plotted without the chart + // having been touched. + let live = chart.source.as_ref().and_then(|src| { + let name = self + .controller + .status + .sheet_id_manager + .get_string(&self.sheet_id)?; + crate::chart_manager::resolve_block_refs( + nav, + &self.controller.status.block_schema_manager, + self.sheet_id, + &name, + src, + ) + }); + // What to draw and where each series reads from. A bound chart + // takes both from the block; the stored series then contribute + // only their look. + let planned: Vec<(Option, Option, Option<&ChartSeries>)> = + match &live { + Some(l) => l + .series + .iter() + .enumerate() + .map(|(i, (name, r))| { + (Some(name.clone()), Some(r.clone()), d.series.get(i)) + }) + .collect(), + None => d + .series + .iter() + .map(|s| (s.name.clone(), s.val_ref.clone(), Some(s))) + .collect(), + }; + let cat_ref = match &live { + Some(l) => l.cat_ref.clone(), + None => d.cat_ref.clone(), + }; Some(ChartInfo { chart_id: chart.id.clone(), from_row, @@ -797,54 +838,58 @@ impl<'a> Worksheet<'a> { legend_pos: d.legend_pos.as_ref().map(legend_pos_str), // Labels follow the sheet like values do; the file's cache // is the fallback. - categories: d - .cat_ref + categories: cat_ref .as_deref() .and_then(|r| self.resolve_category_labels(r)) .unwrap_or_else(|| d.categories.clone()), - cat_ref: d.cat_ref.clone(), - series: d - .series + cat_ref: cat_ref.clone(), + series: planned .iter() - .map(|s| { + .map(|(name, val_ref, s)| { // Live values re-read from the source range so the // chart reflects edits; fall back to the OOXML cache // if the reference can't be resolved. - let values = s - .val_ref + let values = val_ref .as_deref() .and_then(|r| self.resolve_series_values(r)) - .unwrap_or_else(|| s.cached_values.clone()); + .unwrap_or_else(|| { + s.map(|s| s.cached_values.clone()).unwrap_or_default() + }); // The source cells' format wins over the cached // one, so reformatting the data reformats the // chart. An explicit label format beats both. - let num_fmt = s - .val_ref + let num_fmt = val_ref .as_deref() .and_then(|r| self.resolve_ref_num_fmt(r)) - .or_else(|| s.format_code.clone()); + .or_else(|| s.and_then(|s| s.format_code.clone())); let label_fmt = d.data_labels.num_fmt.as_ref().or(num_fmt.as_ref()); let formatted_values = values .iter() .map(|v| v.map(|n| format_with(label_fmt.map(|f| f.as_str()), n))) .collect(); + let size_ref = s.and_then(|s| s.size_ref.clone()); ChartSeriesInfo { - name: s.name.clone(), + name: name.clone(), values, formatted_values, // A bubble chart's third dimension, live like // the values are. - sizes: s - .size_ref + sizes: size_ref .as_deref() .and_then(|r| self.resolve_series_values(r)) - .unwrap_or_else(|| s.cached_sizes.clone()), - size_ref: s.size_ref.clone(), - color: s.color.as_ref().and_then(|c| self.resolve_series_color(c)), - val_ref: s.val_ref.clone(), + .unwrap_or_else(|| { + s.map(|s| s.cached_sizes.clone()).unwrap_or_default() + }), + size_ref, + color: s + .and_then(|s| s.color.as_ref()) + .and_then(|c| self.resolve_series_color(c)), + val_ref: val_ref.clone(), // Set only on a combo chart's overridden // series; `None` means it follows the chart. - series_type: s.series_type.as_ref().map(chart_type_str), + series_type: s + .and_then(|s| s.series_type.as_ref()) + .map(chart_type_str), num_fmt, } }) @@ -868,11 +913,18 @@ impl<'a> Worksheet<'a> { val_axis_scale: axis_scale_info(&d.val_axis_scale), cat_axis_scale: axis_scale_info(&d.cat_axis_scale), val_axis_num_fmt: d.val_axis_num_fmt.clone().or_else(|| { - d.series + planned .first() - .and_then(|s| s.val_ref.as_deref()) + .and_then(|(_, r, _)| r.as_deref()) .and_then(|r| self.resolve_ref_num_fmt(r)) }), + block_source: chart.source.as_ref().map(|src| { + crate::controller::display::ChartBlockSourceInfo { + block_id: src.block_id, + category_field: src.category_field.clone(), + value_fields: src.value_fields.clone(), + } + }), }) }) .collect() diff --git a/crates/controller/src/calc_engine/calculator/funcs/round.rs b/crates/controller/src/calc_engine/calculator/funcs/round.rs index 342e5efd..c28e1b63 100644 --- a/crates/controller/src/calc_engine/calculator/funcs/round.rs +++ b/crates/controller/src/calc_engine/calculator/funcs/round.rs @@ -19,6 +19,103 @@ where CalcVertex::from_number(r) } +/// How a decimal digit past the cut is resolved. +#[derive(Clone, Copy)] +enum Mode { + /// ROUND: half away from zero. + HalfAway, + /// ROUNDUP: away from zero whenever anything is dropped. + Away, + /// ROUNDDOWN: toward zero always. + Toward, +} + +/// Round `num` at `digits` decimal places, deciding on the number's +/// 15-significant-digit DECIMAL form rather than on the binary double. +/// +/// The obvious `(num * 10f64.powi(digits)).round() / …` is wrong twice over, +/// and both ways showed up as real answers: +/// +/// - As a double, `4.935` is `4.93499999999999961`, so scaling and rounding +/// gives 4.93. Excel says 4.94 — it never sees those trailing digits, +/// because 15 significant decimal digits is all it keeps. +/// - The scaling itself has error, and its direction depends on how the value +/// was produced. `4.935 * 100` is `493.49999999999994`, while the same +/// number reconstructed from a parsed literal overshoots to +/// `493.50000000000006`. That is why `ROUND(4.935,2)` used to answer 4.94 +/// while `ROUND(B1,2)` over a cell holding 4.935 answered 4.93 — the same +/// number, rounded two ways. +/// +/// Working on the decimal digits removes both. The kept digits form an integer, +/// the carry is ordinary decimal carrying, and the result is rebuilt as a +/// decimal string so the returned double is the correctly-rounded one rather +/// than the product of another float multiply. +fn round_decimal(num: f64, digits: i32, mode: Mode) -> f64 { + if !num.is_finite() || num == 0. { + return num; + } + // `{:.14e}` is exactly 15 significant digits, as `d.dddddddddddddde±x`. + let s = format!("{:.14e}", num.abs()); + let Some((mantissa, exp)) = s.split_once('e') else { + return num; + }; + let Ok(exp) = exp.parse::() else { + return num; + }; + let ds: Vec = mantissa + .bytes() + .filter(u8::is_ascii_digit) + .map(|b| b - b'0') + .collect(); + + // `ds` is the digit string of `0.d1d2… × 10^(exp+1)`, so the last digit + // kept when rounding at `digits` places sits at index `exp + digits`. + let keep = exp + digits + 1; + if keep >= ds.len() as i32 { + // More places asked for than the decimal form holds: already exact. + return num; + } + let (kept, dropped) = if keep <= 0 { + (&[][..], &ds[..]) + } else { + ds.split_at(keep as usize) + }; + let mut n = kept.iter().fold(0u64, |acc, d| acc * 10 + *d as u64); + let round_away = match mode { + // With `keep` negative the cut falls to the left of every significant + // digit, so the digit deciding the tie is one of the zeros in between + // — not `dropped[0]`. The value is then under half a unit at this + // place and stays put, which is why `ROUND(0.0005,2)` is 0 and not + // 0.01. `ROUNDUP` still moves it: something nonzero was dropped. + Mode::HalfAway => keep >= 0 && dropped.first().is_some_and(|d| *d >= 5), + Mode::Away => dropped.iter().any(|d| *d != 0), + Mode::Toward => false, + }; + if round_away { + // Ordinary decimal carry: 999 + 1 = 1000, no special case needed. + n += 1; + } + if n == 0 { + return 0f64.copysign(num); + } + + // Rebuilt as a decimal string: `n` is the value in units of 10^-digits. + let mut text = n.to_string(); + if digits > 0 { + let point = digits as usize; + if text.len() <= point { + text = format!("{}{}", "0".repeat(point - text.len() + 1), text); + } + text.insert(text.len() - point, '.'); + } else { + text.push_str(&"0".repeat((-digits) as usize)); + } + if num < 0. { + text.insert(0, '-'); + } + text.parse().unwrap_or(num) +} + pub fn calc_mround(args: Vec, fetcher: &mut C) -> CalcVertex where C: Connector, @@ -43,10 +140,7 @@ pub fn calc_round(args: Vec, fetcher: &mut C) -> CalcVertex where C: Connector, { - let f = |num: f64, digits: i32| -> f64 { - let shift_factor = 10_f64.powi(digits); - (num * shift_factor).round() / shift_factor - }; + let f = |num: f64, digits: i32| -> f64 { round_decimal(num, digits, Mode::HalfAway) }; calc(args, fetcher, &f) } @@ -55,10 +149,7 @@ pub fn calc_rounddown(args: Vec, fetcher: &mut C) -> CalcVertex where C: Connector, { - let f = |num: f64, digits: i32| -> f64 { - let shift_factor = 10_f64.powi(digits); - (num * shift_factor).trunc() / shift_factor - }; + let f = |num: f64, digits: i32| -> f64 { round_decimal(num, digits, Mode::Toward) }; calc(args, fetcher, &f) } @@ -67,16 +158,7 @@ pub fn calc_roundup(args: Vec, fetcher: &mut C) -> CalcVertex where C: Connector, { - let f = |num: f64, digits: i32| -> f64 { - let shift_factor = 10_f64.powi(digits); - if num > 0. { - (num * shift_factor).ceil() / shift_factor - } else if num == 0. { - 0. - } else { - (num * shift_factor).floor() / shift_factor - } - }; + let f = |num: f64, digits: i32| -> f64 { round_decimal(num, digits, Mode::Away) }; calc(args, fetcher, &f) } @@ -140,3 +222,82 @@ where let rounded = if up { q.ceil() } else { q.floor() }; CalcVertex::from_number(rounded * sig) } + +#[cfg(test)] +mod tests { + use super::{Mode, round_decimal}; + + fn round(num: f64, digits: i32) -> f64 { + round_decimal(num, digits, Mode::HalfAway) + } + + /// The case that started this: as a double, 4.935 is 4.93499999999999961, + /// so scaling by 100 and rounding gives 4.93. Excel answers 4.94, because + /// 15 significant decimal digits is all it keeps and 4.93500000000000 is a + /// tie, resolved away from zero. + #[test] + fn half_way_decimals_round_away_from_zero_like_excel() { + assert_eq!(round(4.935, 2), 4.94); + assert_eq!(round(1.005, 2), 1.01); + assert_eq!(round(2.675, 2), 2.68); + assert_eq!(round(-4.935, 2), -4.94); + assert_eq!(round(2.5, 0), 3.0); + assert_eq!(round(-2.5, 0), -3.0); + } + + /// The old implementation's answer depended on how the value reached it: + /// `4.935 * 100` is 493.49999999999994, while the same number rebuilt from + /// a parsed literal overshoots to 493.50000000000006. So `ROUND(4.935,2)` + /// gave 4.94 and `ROUND(B1,2)` over a cell holding 4.935 gave 4.93. + #[test] + fn the_same_number_rounds_the_same_way_however_it_arrived() { + let computed = 9.87 / 2.0; + assert_eq!(computed, 4.935, "the double is the same either way"); + assert_eq!(round(computed, 2), round(4.935, 2)); + assert_eq!(round(computed, 2), 4.94); + } + + #[test] + fn negative_digits_round_to_tens_and_hundreds() { + assert_eq!(round(1250.0, -2), 1300.0); + assert_eq!(round(1234.5, -2), 1200.0); + assert_eq!(round(-1250.0, -2), -1300.0); + // Every digit falls past the cut, so there is nothing left to keep. + assert_eq!(round(4.935, -3), 0.0); + } + + /// A cut left of every significant digit is decided by the zeros in + /// between, not by the first digit it finds: 0.0005 at two places is under + /// half a unit, so it stays at 0 — but rounding *up* still moves it. + #[test] + fn a_value_under_half_a_unit_stays_put() { + assert_eq!(round(0.0005, 2), 0.0); + assert_eq!(round_decimal(0.0005, 2, Mode::Away), 0.01); + assert_eq!(round_decimal(0.0005, 2, Mode::Toward), 0.0); + // One place further in it is exactly the tie, and goes away from zero. + assert_eq!(round(0.005, 2), 0.01); + assert_eq!(round(0.05, 1), 0.1); + assert_eq!(round(0.04, 1), 0.0); + } + + /// Scaling by a power of ten used to leave `ROUNDDOWN(2.1,2)` at 2.09, + /// because `2.1 * 100` is 209.99999999999997. + #[test] + fn rounding_toward_and_away_do_not_lose_an_exact_value() { + assert_eq!(round_decimal(2.1, 2, Mode::Toward), 2.1); + assert_eq!(round_decimal(2.1, 2, Mode::Away), 2.1); + assert_eq!(round_decimal(3.14159, 3, Mode::Away), 3.142); + // Toward zero, so a negative goes up rather than down. + assert_eq!(round_decimal(-3.14159, 3, Mode::Toward), -3.141); + assert_eq!(round_decimal(-3.14159, 3, Mode::Away), -3.142); + } + + #[test] + fn asking_for_more_places_than_the_number_has_changes_nothing() { + assert_eq!(round(4.935, 10), 4.935); + assert_eq!(round(0.0, 2), 0.0); + assert_eq!(round(1.0e20, 2), 1.0e20); + assert!(round(f64::NAN, 2).is_nan()); + assert_eq!(round(f64::INFINITY, 2), f64::INFINITY); + } +} diff --git a/crates/controller/src/chart_manager/block_source.rs b/crates/controller/src/chart_manager/block_source.rs new file mode 100644 index 00000000..99614e5c --- /dev/null +++ b/crates/controller/src/chart_manager/block_source.rs @@ -0,0 +1,140 @@ +//! Turning a [`ChartBlockSource`] into the A1 ranges it currently covers. +//! +//! Everything here is derived, never stored: the block's own extent and field +//! layout are the truth, so a chart bound to a block is re-resolved on every +//! read and every save. That is what makes such a chart follow the block — +//! records appended to it are plotted without the chart being touched, and rows +//! inserted above it do not leave the chart pointing at the wrong cells. + +use logisheets_base::{SheetId, index_to_column_label}; + +use crate::block_manager::schema_manager::{SchemaManager, schema::Schema}; +use crate::navigator::Navigator; + +use super::ChartBlockSource; + +/// The ranges a block source resolves to right now. +pub struct ResolvedBlockRefs { + /// Category labels, `None` when the source names no category field. + pub cat_ref: Option, + /// `(field name, value ref)` for each plotted field, in source order. + /// A field the schema no longer has is dropped rather than faked. + pub series: Vec<(String, String)>, +} + +/// One field's range across every record of the block, plus the category range. +/// +/// Returns `None` when the block or its schema is gone, or when the schema is +/// `random` — a random schema has no field axis, so there is no column to plot. +pub fn resolve_block_refs( + navigator: &Navigator, + schemas: &SchemaManager, + sheet_id: SheetId, + sheet_name: &str, + source: &ChartBlockSource, +) -> Option { + let bp = navigator + .get_block_place(&sheet_id, &source.block_id) + .ok()?; + let (row_start, col_start) = navigator + .fetch_normal_cell_idx(&sheet_id, &bp.master) + .ok()?; + let schema = schemas.schemas.get(&(sheet_id, source.block_id))?; + + // Which way the block runs. A row schema puts fields on columns and one + // record per row; a col schema is the transpose. `random` has neither. + let fields_are_columns = match schema { + Schema::RowSchema(_) => true, + Schema::ColSchema(_) => false, + Schema::RandomSchema(_) => return None, + }; + + // Every line of the block is a record: a block covers the data only, and + // the header that named its fields sits outside it. + let record_cnt = if fields_are_columns { + bp.rows.len() + } else { + bp.cols.len() + }; + if record_cnt == 0 { + return None; + } + + // Where a field sits *now*: its axis id is stable, its position is not, so + // the position is looked up on each resolution. + let field_offset = |name: &str| -> Option { + let id = schema_resolve_field_id(schema, name)?; + if fields_are_columns { + bp.cols.iter().position(|c| *c == id) + } else { + bp.rows.iter().position(|r| *r == id) + } + }; + + let range_for = |field_offset: usize| -> String { + let (sr, er, sc, ec) = if fields_are_columns { + ( + row_start, + row_start + record_cnt - 1, + col_start + field_offset, + col_start + field_offset, + ) + } else { + ( + row_start + field_offset, + row_start + field_offset, + col_start, + col_start + record_cnt - 1, + ) + }; + format!( + "{}!${}${}:${}${}", + quote_sheet(sheet_name), + index_to_column_label(sc), + sr + 1, + index_to_column_label(ec), + er + 1, + ) + }; + + let cat_ref = source + .category_field + .as_deref() + .and_then(field_offset) + .map(range_for); + let series = source + .value_fields + .iter() + .filter_map(|name| field_offset(name).map(|o| (name.clone(), range_for(o)))) + .collect::>(); + if series.is_empty() { + return None; + } + Some(ResolvedBlockRefs { cat_ref, series }) +} + +/// `Schema` does not expose `resolve_field_id` on the enum itself, and a +/// `random` schema has no fields to resolve. +fn schema_resolve_field_id(schema: &Schema, name: &str) -> Option { + match schema { + Schema::RowSchema(s) => s.field_axis_by_name(name), + Schema::ColSchema(s) => s.field_axis_by_name(name), + Schema::RandomSchema(_) => None, + } +} + +/// A sheet name goes into a ref quoted only when it has to be, matching what +/// Excel writes — an unnecessary quote would still parse, but every stored ref +/// would then differ from the one the user sees in the formula bar. +fn quote_sheet(name: &str) -> String { + let needs = name.is_empty() + || name + .chars() + .any(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) + || name.chars().next().is_some_and(|c| c.is_ascii_digit()); + if needs { + format!("'{}'", name.replace('\'', "''")) + } else { + name.to_string() + } +} diff --git a/crates/controller/src/chart_manager/executor.rs b/crates/controller/src/chart_manager/executor.rs index af27ec9a..a1a27d18 100644 --- a/crates/controller/src/chart_manager/executor.rs +++ b/crates/controller/src/chart_manager/executor.rs @@ -11,7 +11,7 @@ use logisheets_workbook::prelude::{ use crate::{Error, edit_action::EditPayload}; -use super::{Chart, ChartManager, ChartMarker}; +use super::{Chart, ChartBlockSource, ChartManager, ChartMarker, ResolvedBlockRefs}; pub struct ChartExecutor { pub manager: ChartManager, @@ -24,10 +24,17 @@ impl ChartExecutor { /// Handle chart payloads. Returns `(self, changed)`; `changed` is `false` /// for payloads this executor does not care about. + /// + /// `block_refs` carries the ranges a `block_source` on the payload resolves + /// to right now. It is resolved by the caller because it needs the block + /// places and schemas, which this executor's context does not carry — and + /// `None` here means the source named a block that cannot be charted, so + /// the payload is refused rather than turned into a chart of nothing. pub fn execute( mut self, ctx: &mut C, payload: EditPayload, + block_refs: Option, ) -> Result<(Self, bool), Error> { match payload { EditPayload::MoveChart(p) => { @@ -68,13 +75,25 @@ impl ChartExecutor { let from_cell = ctx.fetch_cell_id(&sheet_id, p.from_row, p.from_col)?; let to_cell = ctx.fetch_cell_id(&sheet_id, p.to_row, p.to_col)?; let chart_type = chart_type_from_str(&p.chart_type); - let series = p.series.iter().map(new_series).collect(); - let spec = ChartData::new( - chart_type, - p.title.clone(), - p.categories_ref.clone(), - series, - ); + // A block-bound chart states fields, not ranges; the ranges + // come from where the block's fields sit at this moment. + let (series, cat_ref) = match (&p.block_source, &block_refs) { + (Some(_), Some(refs)) => ( + refs.series + .iter() + .map(|(name, r)| ChartSeries::new(Some(name.clone()), r.clone())) + .collect(), + refs.cat_ref.clone(), + ), + // Asked for a block we could not resolve: refuse rather + // than silently create an empty chart. + (Some(_), None) => return Ok((self, false)), + (None, _) => ( + p.series.iter().map(new_series).collect(), + p.categories_ref.clone(), + ), + }; + let spec = ChartData::new(chart_type, p.title.clone(), cat_ref, series); let bytes = build_chart_xml(&spec).into_bytes(); let data = match parse_chart(&bytes) { Some(d) => d, @@ -104,6 +123,7 @@ impl ChartExecutor { part_path, data, raw, + source: p.block_source.as_ref().map(block_source), }, ); Ok((self, true)) @@ -166,6 +186,34 @@ impl ChartExecutor { if let Some(r) = &p.categories_ref { spec.cat_ref = non_empty(r); } + // Rebinding to a block replaces the series wholesale; stating + // `series` instead unbinds, because naming fixed ranges is + // exactly the statement that the chart no longer tracks one. + let new_source = match (&p.block_source, &block_refs) { + (Some(bs), Some(refs)) => { + let previous = std::mem::take(&mut spec.series); + spec.cat_ref = refs.cat_ref.clone(); + spec.series = refs + .series + .iter() + .enumerate() + .map(|(i, (name, r))| { + let mut ns = ChartSeries::new(Some(name.clone()), r.clone()); + // Keep the slot's look, so re-resolving a block + // does not reshuffle the chart's palette. + if let Some(old) = previous.get(i) { + ns.color = old.color.clone(); + ns.series_type = old.series_type.clone(); + ns.preserved = old.preserved.clone(); + } + ns + }) + .collect(); + Some(Some(block_source(bs))) + } + (Some(_), None) => return Ok((self, false)), + (None, _) => p.series.as_ref().map(|_| None), + }; if let Some(sc) = &p.val_axis_scale { spec.val_axis_scale = axis_scale(sc); } @@ -180,7 +228,7 @@ impl ChartExecutor { second_size: sp.second_size.filter(|v| (5.0..=200.0).contains(v)), }; } - if let Some(series) = &p.series { + if let (None, Some(series)) = (&p.block_source, &p.series) { let previous = std::mem::take(&mut spec.series); spec.series = series .iter() @@ -248,6 +296,9 @@ impl ChartExecutor { let changed = self .manager .update_content(sheet_id, &p.chart_id, data, raw); + if let Some(source) = new_source { + self.manager.set_source(sheet_id, &p.chart_id, source); + } Ok((self, changed)) } _ => Ok((self, false)), @@ -255,6 +306,17 @@ impl ChartExecutor { } } +/// A payload's block binding → the stored one. Only the field *names* are +/// kept: positions are looked up against the block on each resolution, which +/// is the whole point of binding to it. +fn block_source(p: &crate::edit_action::ChartBlockSource) -> ChartBlockSource { + ChartBlockSource { + block_id: p.block_id, + category_field: p.category_field.as_ref().and_then(|f| non_empty(f)), + value_fields: p.value_fields.clone(), + } +} + /// A payload series → the workbook model. An explicit color is taken as a /// literal RGB hex (what a color picker produces); theme-scheme colors only /// come from the file. diff --git a/crates/controller/src/chart_manager/mod.rs b/crates/controller/src/chart_manager/mod.rs index aef4e1e3..d533b84e 100644 --- a/crates/controller/src/chart_manager/mod.rs +++ b/crates/controller/src/chart_manager/mod.rs @@ -12,14 +12,16 @@ //! (`c:chartSpace`), not this struct: `data` is derived for rendering and may be //! lossy, while `raw` is authoritative for persistence. +pub mod block_source; pub mod executor; +pub use block_source::{ResolvedBlockRefs, resolve_block_refs}; pub use executor::ChartExecutor; use std::sync::Arc; use imbl::{HashMap, Vector}; -use logisheets_base::{CellId, SheetId}; +use logisheets_base::{BlockId, CellId, SheetId}; use logisheets_workbook::prelude::{ChartData, PassthroughPart}; /// A chart anchor corner: a stable cell plus an EMU offset into that cell. @@ -44,6 +46,28 @@ pub enum ChartExtent { Size { cx: i64, cy: i64 }, } +/// A chart whose ranges are a block's, not a fixed rectangle. +/// +/// Charts normally carry A1 text (`Sheet1!$B$2:$B$5`), which is what OOXML +/// stores and all Excel understands. That is a snapshot: append a record to the +/// block and it falls outside the range. A block already knows its own extent +/// and where each field lives, so a chart bound to one states *what* to plot +/// and lets the block say *where* — the range is recomputed on every read and +/// every save, so it grows and shifts with the block for free. +/// +/// Fields are held by name, the same identity `#FIELD("qty")` formulas use, so +/// inserting or moving a column inside the block keeps the link. Renaming a +/// field breaks it, exactly as it breaks those formulas. +#[derive(Debug, Clone, PartialEq)] +pub struct ChartBlockSource { + pub block_id: BlockId, + /// Field whose values label the category axis. `None` numbers the + /// categories 1..n instead. + pub category_field: Option, + /// Fields plotted as series, in series order. + pub value_fields: Vec, +} + #[derive(Debug, Clone)] pub struct Chart { /// Stable id (currently the chart part's file stem, e.g. `chart1`). @@ -58,6 +82,10 @@ pub struct Chart { /// Original chart part tree (chart XML + style/color satellites) preserved /// verbatim for lossless save. Behind an `Arc` to keep snapshots cheap. pub raw: Arc>, + /// Set when the chart plots a block rather than a fixed range. The refs in + /// `data` are then a cache of the last resolution, not the truth — see + /// [`ChartBlockSource`]. + pub source: Option, } #[derive(Debug, Clone, Default)] @@ -139,6 +167,29 @@ impl ChartManager { true } + /// Bind (or, with `None`, unbind) a chart's block source. Returns whether + /// a matching chart was found. + pub fn set_source( + &mut self, + sheet_id: SheetId, + chart_id: &str, + source: Option, + ) -> bool { + let mut v = match self.charts.get(&sheet_id) { + Some(v) => v.clone(), + None => return false, + }; + let idx = match v.iter().position(|c| c.id == chart_id) { + Some(i) => i, + None => return false, + }; + let mut chart = v[idx].clone(); + chart.source = source; + v.set(idx, chart); + self.charts.insert(sheet_id, v); + true + } + /// Remove the chart with `chart_id` from `sheet_id`. Returns whether it /// existed. pub fn remove_chart(&mut self, sheet_id: SheetId, chart_id: &str) -> bool { diff --git a/crates/controller/src/controller/display.rs b/crates/controller/src/controller/display.rs index 29fe4676..63b05343 100644 --- a/crates/controller/src/controller/display.rs +++ b/crates/controller/src/controller/display.rs @@ -357,6 +357,21 @@ pub struct ChartInfo { /// Number-format code for the value axis, live from the first series' /// source cells when the chart does not set one itself. pub val_axis_num_fmt: Option, + /// Set when the chart plots a block rather than fixed ranges. The refs + /// above are then derived from the block as it is right now, so a host + /// showing a chart's source should say which block it follows rather than + /// present the range as something the user typed. + pub block_source: Option, +} + +/// The block a chart follows: its id, the field labelling the categories, and +/// the fields plotted as series. +#[derive(Debug, Clone, TS)] +#[ts(file_name = "chart_block_source_info.ts", rename_all = "camelCase")] +pub struct ChartBlockSourceInfo { + pub block_id: usize, + pub category_field: Option, + pub value_fields: Vec, } /// A person referenced by a comment (author or mention). Enterprise builds diff --git a/crates/controller/src/controller/executor.rs b/crates/controller/src/controller/executor.rs index b9a2efe5..67951510 100644 --- a/crates/controller/src/controller/executor.rs +++ b/crates/controller/src/controller/executor.rs @@ -544,6 +544,9 @@ impl<'a> Executor<'a> { } fn execute_chart(&mut self, payload: EditPayload) -> Result<(ChartExecutor, bool), Error> { + // Resolved out here because a block's ranges come from its place and + // its schema, which the chart executor's context does not carry. + let block_refs = self.resolve_chart_block_source(&payload); let mut ctx = CellAttachmentsConnector { sheet_pos_manager: &self.status.sheet_info_manager, navigator: &self.status.navigator, @@ -554,7 +557,34 @@ impl<'a> Executor<'a> { text_id_manager: &mut self.status.text_id_manager, }; let executor = ChartExecutor::new(self.status.chart_manager.clone()); - executor.execute(&mut ctx, payload) + executor.execute(&mut ctx, payload, block_refs) + } + + /// The ranges a chart payload's `block_source` currently covers, or `None` + /// when the payload has no block source (and when it names one that cannot + /// be charted — the executor treats both as "do not create a chart"). + fn resolve_chart_block_source( + &self, + payload: &EditPayload, + ) -> Option { + let (sheet_idx, source) = match payload { + EditPayload::CreateChart(p) => (p.sheet_idx, p.block_source.as_ref()?), + EditPayload::UpdateChart(p) => (p.sheet_idx, p.block_source.as_ref()?), + _ => return None, + }; + let sheet_id = self.status.sheet_info_manager.get_sheet_id(sheet_idx)?; + let sheet_name = self.status.sheet_id_manager.get_string(&sheet_id)?; + crate::chart_manager::resolve_block_refs( + &self.status.navigator, + &self.status.block_schema_manager, + sheet_id, + &sheet_name, + &crate::chart_manager::ChartBlockSource { + block_id: source.block_id, + category_field: source.category_field.clone(), + value_fields: source.value_fields.clone(), + }, + ) } fn execute_container( diff --git a/crates/controller/src/edit_action/mod.rs b/crates/controller/src/edit_action/mod.rs index 79602ed9..8c6f5cf6 100644 --- a/crates/controller/src/edit_action/mod.rs +++ b/crates/controller/src/edit_action/mod.rs @@ -389,6 +389,12 @@ pub struct UpdateChart { /// Replace how a pie-of-pie / bar-of-pie splits its series. Like the axis /// scales this is all-or-nothing rather than a patch. pub of_pie_split: Option, + /// Bind the chart to a block, or rebind it to different fields. The series + /// then come from the block and `series`/`categories_ref` are ignored. + /// + /// To unbind, state `series` instead: naming fixed ranges is exactly the + /// statement that the chart no longer tracks a block. + pub block_source: Option, } /// The division between an of-pie chart's two plots. @@ -468,6 +474,24 @@ pub struct CreateChart { pub title: Option, pub categories_ref: Option, pub series: Vec, + /// Plot a block instead of fixed ranges. When set, `series` and + /// `categories_ref` are ignored — the block says where its fields are, and + /// the chart re-reads that on every render and every save, so it follows + /// the block as records are added or columns move. + pub block_source: Option, +} + +/// Binds a chart to a block: which block, which of its fields to plot, and +/// which field labels the categories. Fields are named, the identity +/// `#FIELD("qty")` formulas use — a renamed field breaks the link, a moved one +/// does not. A block with a `random` schema has no field axis and cannot be +/// charted this way. +#[derive(Debug, Clone, TS)] +#[ts(file_name = "chart_block_source.ts", builder, rename_all = "camelCase")] +pub struct ChartBlockSource { + pub block_id: usize, + pub category_field: Option, + pub value_fields: Vec, } /// Add a conditional-formatting rule over `start`..`end` (corners may be given diff --git a/crates/controller/src/file_loader/mod.rs b/crates/controller/src/file_loader/mod.rs index 7197838a..25bbff31 100644 --- a/crates/controller/src/file_loader/mod.rs +++ b/crates/controller/src/file_loader/mod.rs @@ -135,6 +135,11 @@ pub fn load_file(wb: Wb, book_name: String) -> Controller { } }); let mut app_data = vec![]; + // Chart-to-block bindings are restored after the sheet walk: the charts + // themselves are loaded from each sheet's drawing further down, so there is + // nothing to bind to yet. + let mut pending_chart_sources: Vec<(SheetId, logisheets_workbook::logisheets::ChartSourceXml)> = + vec![]; if let Some(logisheets) = logisheets { app_data = logisheets.apps; @@ -167,6 +172,7 @@ pub fn load_file(wb: Wb, book_name: String) -> Controller { col_schemas, random_schemas, link_ranges, + chart_sources, } = sheet_data; let sheet_id = sheet_info_manager.get_sheet_id(idx).unwrap(); navigator.add_sheet_id(&sheet_id); @@ -250,6 +256,9 @@ pub fn load_file(wb: Wb, book_name: String) -> Controller { link_ranges .into_iter() .for_each(|lr| pending_links.push((sheet_id, lr))); + chart_sources + .into_iter() + .for_each(|cs| pending_chart_sources.push((sheet_id, cs))); }); // All blocks on all sheets now exist — restore each link. The source range // is on its own sheet; the target block may be on another (`block_sheet_idx`). @@ -456,7 +465,23 @@ pub fn load_file(wb: Wb, book_name: String) -> Controller { block_schema_manager, field_render_manager, image_manager, - chart_manager, + chart_manager: { + // Every chart is loaded by now, so a saved binding has something + // to attach to. A binding whose chart is gone is dropped. + let mut cm = chart_manager; + for (sheet_id, cs) in pending_chart_sources { + cm.set_source( + sheet_id, + &cs.chart_id, + Some(crate::chart_manager::ChartBlockSource { + block_id: cs.block_id, + category_field: cs.category_field, + value_fields: cs.value_fields.into_iter().map(|f| f.name).collect(), + }), + ); + } + cm + }, data_validation_manager, conditional_formatting_manager, }; @@ -907,6 +932,9 @@ fn load_charts( part_path: part.path.clone(), data, raw: raw.clone(), + // Restored from logisheets.xml further down; an xlsx written by + // Excel has no block bindings at all. + source: None, }, ); } diff --git a/crates/controller/src/file_saver/workbook.rs b/crates/controller/src/file_saver/workbook.rs index ff652a65..2aa4141e 100644 --- a/crates/controller/src/file_saver/workbook.rs +++ b/crates/controller/src/file_saver/workbook.rs @@ -1,7 +1,10 @@ use itertools::Itertools; use logisheets_base::NormalRange; use logisheets_workbook::{ - logisheets::{AppData, CellAppendix, LinkRangeXml, LogiSheetsData, Sheet}, + logisheets::{ + AppData, CellAppendix, ChartSourceFieldXml, ChartSourceXml, LinkRangeXml, LogiSheetsData, + Sheet, + }, prelude::{ChartAnchor, ChartAnchorExtent, PassthroughPart}, prelude::{ CtConditionalFormatting, CtExternalReference, CtExternalReferences, CtPerson, CtSheet, @@ -256,8 +259,10 @@ pub fn save_workbook( // images share one drawing part. let mut chart_anchors: Vec = vec![]; let mut chart_parts: Vec = vec![]; + let mut chart_sources: Vec = vec![]; let mut seen_parts: std::collections::HashSet = std::collections::HashSet::new(); + let sheet_name = sheet_id_manager.get_string(&sheet_id).unwrap_or_default(); for chart in chart_manager.charts_of_sheet(sheet_id) { let Ok((fr, fc)) = navigator.fetch_cell_idx(&sheet_id, &chart.from.cell) else { continue; @@ -288,9 +293,67 @@ pub fn save_workbook( chart_path: chart.part_path.clone(), name: format!("Chart {}", chart.id), }); + // A block-bound chart is written out with the ranges the block + // covers right now. Excel has no idea what a block is, so the + // file has to carry real A1 refs — and the ones sitting in + // `data` are only as fresh as the last resolution. The binding + // itself goes to logisheets.xml below, so reopening here picks + // the chart back up as bound rather than frozen. + let regenerated = chart.source.as_ref().and_then(|src| { + let refs = crate::chart_manager::resolve_block_refs( + navigator, + block_schema_manager, + sheet_id, + &sheet_name, + src, + )?; + let mut spec = chart.data.clone(); + spec.cat_ref = refs.cat_ref.clone(); + let previous = std::mem::take(&mut spec.series); + spec.series = refs + .series + .iter() + .enumerate() + .map(|(i, (name, r))| { + let mut ns = logisheets_workbook::prelude::ChartSeries::new( + Some(name.clone()), + r.clone(), + ); + if let Some(old) = previous.get(i) { + ns.color = old.color.clone(); + ns.series_type = old.series_type.clone(); + ns.preserved = old.preserved.clone(); + } + ns + }) + .collect(); + Some(logisheets_workbook::prelude::build_chart_xml(&spec).into_bytes()) + }); + if let Some(src) = &chart.source { + chart_sources.push(ChartSourceXml { + chart_id: chart.id.clone(), + block_id: src.block_id, + category_field: src.category_field.clone(), + value_fields: src + .value_fields + .iter() + .map(|name| ChartSourceFieldXml { name: name.clone() }) + .collect(), + }); + } for p in chart.raw.iter() { if seen_parts.insert(p.path.clone()) { - chart_parts.push(p.clone()); + match ®enerated { + // Only the chart part itself is replaced; the style + // and color satellites go out untouched. + Some(bytes) if p.path == chart.part_path => { + chart_parts.push(PassthroughPart { + data: bytes.clone(), + ..p.clone() + }); + } + _ => chart_parts.push(p.clone()), + } } } } @@ -395,6 +458,7 @@ pub fn save_workbook( col_schemas, random_schemas, link_ranges, + chart_sources, }; sheets.push(sheet); }); diff --git a/crates/ssf-rs/NOTICE b/crates/ssf-rs/NOTICE index cdf05c18..64a383a7 100644 --- a/crates/ssf-rs/NOTICE +++ b/crates/ssf-rs/NOTICE @@ -21,3 +21,30 @@ Changes made in this port relative to the upstream JavaScript source: and independent of the host timezone/DST. - The public formatting entry points return `Result` values instead of throwing on unsupported/invalid input. + - Numeric rendering rounds at Excel's precision rather than JavaScript's. + Upstream reaches for `Math.round(val * 10^d) / 10^d` and `toFixed`, which + decide a halfway case on the exact binary double: 1.005 is really + 1.00499999999999989, so `0.00` renders it "1.00". Excel keeps only 15 + significant decimal digits, sees the tie in 1.00500000000000, and shows + "1.01". Since the point of this port is Excel's `TEXT()`, the number paths + (`rnd`, `dec`, `carry`, and the fixed-decimal `to_fixed` calls in + `writenum`) now settle the rounding on the value's 15-significant-digit + decimal form, via `jsnum::Precision::Excel15`. So `0.00`, `#,##0.00` and + `$#,##0.00` all render 1.005 as "1.01", 2.675 as "2.68" and 4.935 as + "4.94" — and they agree with each other, which they did not before + (`#,##0.00` went through a different rounding helper and answered "1.00" + and "2.67"). + + Scoped to magnitudes under 1e15. At or above that the 15-digit window + closes before the decimal point, so there is no fractional tie to settle + and applying the rule would rewrite integer digits instead. Excel does + shorten those as well, but uniformly across every numeric format; doing it + only in the helpers that happen to route through here would leave the + formats disagreeing with each other. Large-value output is therefore + unchanged from upstream. + + The JavaScript primitives in `jsnum` are NOT changed — `to_fixed`, + `to_exponential` and `to_precision` still reproduce ECMAScript exactly, so + `tests/jsnum_diff.rs` remains a true differential test against node. Only + the Excel-facing callers opt into `Precision::Excel15`, which does mean + `tests/format_diff.rs` diverges from the JavaScript `ssf` on those values. diff --git a/crates/ssf-rs/src/jsnum.rs b/crates/ssf-rs/src/jsnum.rs index 815c4e5c..529477e6 100644 --- a/crates/ssf-rs/src/jsnum.rs +++ b/crates/ssf-rs/src/jsnum.rs @@ -39,6 +39,82 @@ fn exact_nonneg(x: f64) -> Exact { } } +/// Which decimal a formatter should treat a double as being. +/// +/// JavaScript's `Number` methods round the *exact* binary value, so `1.005` +/// (really `1.00499999999999989…`) renders as `"1.00"`. Excel keeps only 15 +/// significant decimal digits and rounds that, so it renders `"1.01"`. Both are +/// correct about their own spec, and this crate needs both: [`Exact`] to stay a +/// faithful port that can be diffed against Node, [`Excel15`] for what a +/// spreadsheet is supposed to show. +/// +/// [`Exact`]: Precision::Exact +/// [`Excel15`]: Precision::Excel15 +#[derive(Clone, Copy, PartialEq, Eq, Debug, Default)] +pub enum Precision { + /// The exact binary value — JavaScript's rule. + #[default] + Exact, + /// Rounded to 15 significant decimal digits first — Excel's rule. + Excel15, +} + +/// How many significant decimal digits Excel keeps. +const EXCEL_SIG_DIGITS: usize = 15; + +/// The expansion a formatter should work from, under `precision`. +/// +/// Collapsing to 15 significant digits is what turns a value sitting a hair +/// below a tie into the tie itself, which is the whole difference between the +/// two rules: `4.935` is exactly `4.93499999999999960920…`, and its 15-digit +/// form is `4.93500000000000` — so rounding to 2 places goes up, not down. +fn expansion(x: f64, precision: Precision) -> Exact { + let e = exact_nonneg(x); + match precision { + Precision::Exact => e, + // At 1e15 and above the 15-digit window closes before the decimal + // point, so collapsing would rewrite the INTEGER digits rather than + // settle a fractional tie. Excel does shorten those too, but it does + // so on every numeric format at once; doing it here would only shift + // the formats that happen to route through this function and leave + // them disagreeing with the rest. Left alone, deliberately. + Precision::Excel15 if x.abs() < 1e15 => { + let (digits, exp) = round_sig(&e, EXCEL_SIG_DIGITS); + from_sig_digits(&digits, exp) + } + Precision::Excel15 => e, + } +} + +/// Rebuild an [`Exact`] from a significant-digit string and the exponent of its +/// leading digit — the inverse of what [`round_sig`] takes apart. +fn from_sig_digits(digits: &str, exp: i32) -> Exact { + if exp >= 0 { + let intlen = (exp + 1) as usize; + if digits.len() <= intlen { + // More integer places than digits: pad with the zeros they stand for. + Exact { + int: format!("{}{}", digits, "0".repeat(intlen - digits.len())), + frac: String::new(), + } + } else { + Exact { + int: digits[..intlen].to_string(), + frac: digits[intlen..].trim_end_matches('0').to_string(), + } + } + } else { + // Value below 1: the leading digit sits `-exp - 1` zeros in. + let zeros = (-exp - 1) as usize; + Exact { + int: "0".to_string(), + frac: format!("{}{}", "0".repeat(zeros), digits) + .trim_end_matches('0') + .to_string(), + } + } +} + /// Increment a non-negative decimal digit string by 1 (e.g. `"199"` -> `"200"`, /// `"999"` -> `"1000"`). Input/output have no decimal point. fn inc_digits(s: &str) -> String { @@ -90,6 +166,12 @@ pub fn round(x: f64) -> f64 { /// `frac` in `0..=100`. Returns the value with exactly `frac` digits after the /// decimal point, correctly rounded (ties up) from the exact binary value. pub fn to_fixed(x: f64, frac: usize) -> String { + to_fixed_with(x, frac, Precision::Exact) +} + +/// [`to_fixed`], reading the value at `precision`. `Precision::Excel15` is what +/// a spreadsheet's fixed-decimal formats (`0.00`) want. +pub fn to_fixed_with(x: f64, frac: usize, precision: Precision) -> String { if x.is_nan() { return "NaN".to_string(); } @@ -101,7 +183,7 @@ pub fn to_fixed(x: f64, frac: usize) -> String { return to_string_js(x); } let sign = if x < 0.0 { "-" } else { "" }; - let e = exact_nonneg(x); + let e = expansion(x, precision); // Build the integer formed by (int . first `frac` frac-digits), rounding. let fbytes = e.frac.as_bytes(); @@ -230,6 +312,11 @@ fn round_sig_even(e: &Exact, sig: usize) -> (String, i32) { /// JavaScript `Number.prototype.toExponential(frac)`. pub fn to_exponential(x: f64, frac: usize) -> String { + to_exponential_with(x, frac, Precision::Exact) +} + +/// [`to_exponential`], reading the value at `precision`. +pub fn to_exponential_with(x: f64, frac: usize, precision: Precision) -> String { if x.is_nan() { return "NaN".to_string(); } @@ -237,7 +324,7 @@ pub fn to_exponential(x: f64, frac: usize) -> String { return if x < 0.0 { "-Infinity" } else { "Infinity" }.to_string(); } let sign = if x < 0.0 && x != 0.0 { "-" } else { "" }; - let e = exact_nonneg(x); + let e = expansion(x, precision); let (digits, exp) = round_sig(&e, frac + 1); let mantissa = if frac == 0 { digits[..1].to_string() @@ -250,6 +337,11 @@ pub fn to_exponential(x: f64, frac: usize) -> String { /// JavaScript `Number.prototype.toPrecision(prec)`. pub fn to_precision(x: f64, prec: usize) -> String { + to_precision_with(x, prec, Precision::Exact) +} + +/// [`to_precision`], reading the value at `precision`. +pub fn to_precision_with(x: f64, prec: usize, precision: Precision) -> String { if x.is_nan() { return "NaN".to_string(); } @@ -258,7 +350,7 @@ pub fn to_precision(x: f64, prec: usize) -> String { } let prec = prec.max(1); let sign = if x < 0.0 && x != 0.0 { "-" } else { "" }; - let e = exact_nonneg(x); + let e = expansion(x, precision); let (digits, exp) = round_sig(&e, prec); if exp < -6 || exp >= prec as i32 { diff --git a/crates/ssf-rs/src/writenum.rs b/crates/ssf-rs/src/writenum.rs index b25a603e..b306cce1 100644 --- a/crates/ssf-rs/src/writenum.rs +++ b/crates/ssf-rs/src/writenum.rs @@ -5,7 +5,7 @@ use regex::Regex; use std::sync::OnceLock; use crate::helpers::{commaify, fill, hashq, pad0, pad0_i, pad0r, pad_, rpad_, strrev}; -use crate::jsnum; +use crate::jsnum::{self, Precision}; fn re(cell: &'static OnceLock, pat: &str) -> &'static Regex { cell.get_or_init(|| Regex::new(pat).unwrap()) @@ -79,14 +79,92 @@ fn log10_floor(v: f64) -> i32 { (v.abs().ln() * std::f64::consts::LOG10_E).floor() as i32 } -/// `ssf.rnd(val, d)`. +/// `ssf.rnd(val, d)` — the value rounded to `d` decimal places, as a bare +/// number string (no padding; callers widen it to the format's own shape). +/// +/// Upstream is `Math.round(val * 10^d) / 10^d`, which is wrong for a +/// spreadsheet twice over. It decides the halfway case on the scaled binary +/// product, so `0.00` rendered 1.005 as "1.00" and 4.935 as "4.93" where Excel +/// shows "1.01" and "4.94" — Excel keeps 15 significant decimal digits, and +/// both of those are ties in that form. And the scaling multiply adds error of +/// its own: `2.1 * 100` is 209.99999999999997, so an exact value could drop a +/// place. Rounding the decimal expansion removes both. fn rnd(val: f64, d: i32) -> String { - let dd = 10f64.powi(d); - s(jsnum::round(val * dd) / dd) + // Past 1e15 there is no fractional tie to settle, and `to_fixed` would + // spell out the double's exact integer digits where every other numeric + // format shows the shortest ones. Keep the original route so they agree. + if !val.is_finite() || val.abs() >= 1e15 { + let dd = 10f64.powi(d); + return s(jsnum::round(val * dd) / dd); + } + // Both call sites pass a count of format placeholders, so `d` is never + // negative; clamp anyway to stay inside `to_fixed`'s domain. + let fixed = jsnum::to_fixed_with(val, d.clamp(0, 100) as usize, Precision::Excel15); + strip_trailing_frac_zeros(&fixed) +} + +/// Trailing fractional zeros, and a bare trailing point, are not part of a +/// number's string form — `rnd` has to look like `s()` did. +fn strip_trailing_frac_zeros(x: &str) -> String { + if !x.contains('.') { + return x.to_string(); + } + let trimmed = x.trim_end_matches('0').trim_end_matches('.'); + // `-0.00` trims to `-0`, and `0.00` to `0`; upstream produced "0" for both. + if trimmed.is_empty() || trimmed == "-" { + return "0".to_string(); + } + trimmed.to_string() +} + +/// The `d` fractional digits of `val` and whether rounding them carried into +/// the integer part, both read off ONE Excel-precision rendering. +/// +/// The pair has to agree — `#,##0.00` builds its output from +/// `s(floor + carry)` and `dec` separately, so a carry that only one of them +/// saw would print 9.999 as "10.00" or "9.00" instead of "10.00". Sharing the +/// rendering makes that impossible. +/// +/// Returns `(carry, frac_digits)`. +fn rounded_parts(val: f64, d: i32) -> (i64, i64) { + let places = d.clamp(0, 100) as usize; + let fixed = jsnum::to_fixed_with(val, places, Precision::Excel15); + let (int_str, frac_str) = match fixed.split_once('.') { + Some((i, f)) => (i, f), + None => (fixed.as_str(), ""), + }; + // Compared as strings: the integer part can exceed `i64` and `s()` renders + // it the same way `to_fixed` does below 1e21. + let carry = i64::from(int_str != s(val.floor())); + let frac = frac_str.parse::().unwrap_or(0); + (carry, frac) } -/// `ssf.dec(val, d)`. +/// `ssf.dec(val, d)` — the fractional digits, at Excel's precision. +/// +/// Upstream rounds `(val - floor(val)) * 10^d` as a double, which is how +/// `#,##0.00` came to render 2.675 as "2.67" and 1.005 as "1.00" while the +/// plain `0.00` format got them right. fn dec(val: f64, d: i32) -> i64 { + if !val.is_finite() || val.abs() >= 1e15 { + return dec_legacy(val, d); + } + rounded_parts(val, d).1 +} + +/// `ssf.carry(val, d)` — whether rounding the fraction carries into the +/// integer part. Shares [`rounded_parts`] with [`dec`] so the two cannot +/// disagree about it. +fn carry(val: f64, d: i32) -> i64 { + if !val.is_finite() || val.abs() >= 1e15 { + return carry_legacy(val, d); + } + rounded_parts(val, d).0 +} + +/// The upstream `dec`, kept for magnitudes where there is no fraction left to +/// round and the decimal path would only change how integer digits print. +fn dec_legacy(val: f64, d: i32) -> i64 { let frac = val - val.floor(); let dd = 10f64.powi(d); let rr = jsnum::round(frac * dd); @@ -97,8 +175,8 @@ fn dec(val: f64, d: i32) -> i64 { } } -/// `ssf.carry(val, d)`. -fn carry(val: f64, d: i32) -> i64 { +/// The upstream `carry`, paired with [`dec_legacy`]. +fn carry_legacy(val: f64, d: i32) -> i64 { let dd = 10f64.powi(d); let rr = jsnum::round((val - val.floor()) * dd); if (d as usize) < s(rr).len() { @@ -196,7 +274,7 @@ fn write_num_exp(fmt: &str, val: f64, v2: bool) -> String { ee += period; } let prec = (idx + 1 + (period + ee) % period).max(0) as usize; - o = jsnum::to_precision(val / 10f64.powi(ee), prec.max(1)); + o = jsnum::to_precision_with(val / 10f64.powi(ee), prec.max(1), Precision::Excel15); let no_exp = if v2 { !o.contains('e') && !o.contains('E') @@ -232,7 +310,7 @@ fn write_num_exp(fmt: &str, val: f64, v2: bool) -> String { // .replace(/^([+-]?)(\d*)\.(\d*)[Ee]/, cb) o = replace_exp_mantissa(&o, period, ee); } else { - o = jsnum::to_exponential(val, idx.max(0) as usize); + o = jsnum::to_exponential_with(val, idx.max(0) as usize, Precision::Excel15); } if re_eplus00().is_match(fmt) && re_e_single().is_match(&o) { @@ -519,7 +597,11 @@ fn write_num_flt(t: &str, fmt: &str, val: f64) -> Result { } if let Some(cap) = re_numdotnum_flt().captures(fmt) { let r2 = cap.get(2).unwrap().as_str(); - let mut o = jsnum::to_fixed(val, r2.len().min(10)); + // Excel's precision, not JavaScript's: a spreadsheet showing `0.00` + // renders 1.005 as "1.01", because the only thing it keeps of the + // double is 15 significant digits and that form is a tie. `toFixed` + // would say "1.00" — right about ECMAScript, wrong about Excel. + let mut o = jsnum::to_fixed_with(val, r2.len().min(10), Precision::Excel15); o = re_trail_after_nonzero().replace(&o, "$1").to_string(); let ri = o.find('.').map(|x| x as i32).unwrap_or(-1); let lres = fmt.find('.').unwrap() as i32 - ri; @@ -694,7 +776,11 @@ fn write_num_int(t: &str, fmt: &str, val: f64) -> Result { } if let Some(cap) = re_numdotnum_int().captures(fmt) { let r2 = cap.get(2).unwrap().as_str(); - let mut o = jsnum::to_fixed(val, r2.len().min(10)); + // Excel's precision, not JavaScript's: a spreadsheet showing `0.00` + // renders 1.005 as "1.01", because the only thing it keeps of the + // double is 15 significant digits and that form is a tie. `toFixed` + // would say "1.00" — right about ECMAScript, wrong about Excel. + let mut o = jsnum::to_fixed_with(val, r2.len().min(10), Precision::Excel15); o = re_trail_after_nonzero().replace(&o, "$1").to_string(); let ri = o.find('.').map(|x| x as i32).unwrap_or(-1); let lres = fmt.find('.').unwrap() as i32 - ri; diff --git a/crates/ssf-rs/tests/format_diff.rs b/crates/ssf-rs/tests/format_diff.rs index 3acd967b..8b768f12 100644 --- a/crates/ssf-rs/tests/format_diff.rs +++ b/crates/ssf-rs/tests/format_diff.rs @@ -4,6 +4,14 @@ //! //! Ignored by default (needs `node` + a local `ssf`). Run with: //! cargo test -p ssf-rs --test format_diff -- --ignored --nocapture +//! +//! NOTE: this crate deliberately diverges from the JavaScript reference on +//! numeric rounding — it rounds at Excel's 15-significant-digit precision, +//! where `ssf` inherits JavaScript's exact-binary rounding, so `0.00` renders +//! 1.005 as "1.01" here and "1.00" there. See `NOTICE`. Divergences reported +//! by this test are only interesting once that class is excluded; +//! `jsnum_diff.rs` is the one that must stay clean, since the primitives it +//! tests are still a faithful port. use std::io::Write; use std::process::Command; diff --git a/crates/ssf-rs/tests/unit.rs b/crates/ssf-rs/tests/unit.rs index 8b976c7f..ff0e1b34 100644 --- a/crates/ssf-rs/tests/unit.rs +++ b/crates/ssf-rs/tests/unit.rs @@ -51,3 +51,92 @@ fn readme_examples() { assert_eq!(format_str("#,##0.00", 1234.5).unwrap(), "1,234.50"); assert_eq!(format_str("yyyy-mm-dd", 45000.0).unwrap(), "2023-03-15"); } + +/// Excel's precision, not JavaScript's. These are the values where the two +/// rules disagree, and a spreadsheet has to answer the way the spreadsheet +/// does — see `NOTICE`. Expectations captured from Excel, NOT from `ssf`. +#[test] +fn numbers_round_at_excel_precision() { + let cases: &[(&str, f64, &str)] = &[ + // As doubles these all sit a hair BELOW the halfway point, so + // JavaScript rounds them down. Their 15-significant-digit form is the + // tie, which Excel resolves away from zero. + ("0.00", 1.005, "1.01"), + ("0.00", 4.935, "4.94"), + ("0.00", 0.015, "0.02"), + ("0.00", 0.045, "0.05"), + ("0.0", 0.15, "0.2"), + ("#,##0.00", 1234.565, "1,234.57"), + // Already above the tie: unchanged, and a check that the collapse to + // 15 digits does not disturb the ordinary cases. + ("0.00", 2.675, "2.68"), + ("0.00", 0.135, "0.14"), + ("0.000", 1.0005, "1.001"), + // Negative ties go away from zero too, as Excel does. + ("0.00", -1.005, "-1.01"), + ("0.00", -4.935, "-4.94"), + // Scaling by a power of ten used to cost an exact value a place: + // `2.1 * 100` is 209.99999999999997. + ("0.00", 2.1, "2.10"), + ("0.0", 8.7, "8.7"), + // A carry out of the fraction has to reach the integer part. + ("0.00", 9.999, "10.00"), + ("0.00", 0.995, "1.00"), + ]; + for (fmt, v, want) in cases { + assert_eq!( + format_str(fmt, *v).unwrap(), + *want, + "format {fmt:?} of {v:?}" + ); + } +} + +/// The comma formats reach the number through a different helper than the bare +/// `0.00` ones do. They used to round differently as a result — `#,##0.00` said +/// "1.00" and "2.67" where `0.00` said "1.01" and "2.68" — so the point here is +/// that the families agree, not just that each is right on its own. +#[test] +fn every_fixed_decimal_format_rounds_alike() { + let vals: &[(f64, &str)] = &[ + (1.005, "1.01"), + (4.935, "4.94"), + (2.675, "2.68"), + (0.015, "0.02"), + (2.1, "2.10"), + (9.999, "10.00"), + (0.995, "1.00"), + ]; + for (v, want) in vals { + assert_eq!(format_str("0.00", *v).unwrap(), *want, "0.00 of {v:?}"); + assert_eq!( + format_str("#,##0.00", *v).unwrap(), + *want, + "#,##0.00 of {v:?}" + ); + assert_eq!( + format_str("$#,##0.00", *v).unwrap(), + format!("${want}"), + "$#,##0.00 of {v:?}" + ); + } +} + +/// Above 1e15 the rule is deliberately not applied, so every numeric format +/// keeps rendering the same digits it always did. Left unguarded, only the +/// formats routing through the changed helpers would have shortened, and they +/// would no longer have matched the others. +#[test] +fn large_values_render_the_same_across_formats() { + let big = 26925224612816314368.0; + assert_eq!(format_str("0", big).unwrap(), "26925224612816314000"); + assert_eq!(format_str("0.00", big).unwrap(), "26925224612816314000.00"); + assert_eq!( + format_str("#,##0", big).unwrap(), + "26,925,224,612,816,314,000" + ); + assert_eq!( + format_str("#,##0.00", big).unwrap(), + "26,925,224,612,816,314,000.00" + ); +} diff --git a/crates/workbook/src/logisheets.rs b/crates/workbook/src/logisheets.rs index 47969d85..833979b8 100644 --- a/crates/workbook/src/logisheets.rs +++ b/crates/workbook/src/logisheets.rs @@ -144,6 +144,35 @@ pub struct Sheet { /// restored on load. See the controller's `range_manager::link`. #[xmlserde(name = b"linkRange", ty = "child")] pub link_ranges: Vec, + /// Charts bound to a block rather than to fixed ranges. The chart part in + /// the xlsx always holds real A1 ranges so Excel can draw it; this records + /// what those ranges were derived *from*, so reopening in LogiSheets keeps + /// the chart following the block instead of freezing it at the last save. + #[xmlserde(name = b"chartSource", ty = "child")] + pub chart_sources: Vec, +} + +/// A chart's block binding: which block, which field labels the categories, +/// and which fields are plotted. Fields are named because that is the identity +/// the schema exposes and the one `#FIELD("qty")` formulas already use. +#[derive(Debug, XmlSerialize, XmlDeserialize)] +pub struct ChartSourceXml { + #[xmlserde(name = b"chartId", ty = "attr")] + pub chart_id: String, + #[xmlserde(name = b"blockId", ty = "attr")] + pub block_id: usize, + #[xmlserde(name = b"categoryField", ty = "attr")] + pub category_field: Option, + /// Child elements rather than one delimited attribute: a field name is + /// user-supplied text and may contain whatever separator we picked. + #[xmlserde(name = b"valueField", ty = "child")] + pub value_fields: Vec, +} + +#[derive(Debug, XmlSerialize, XmlDeserialize)] +pub struct ChartSourceFieldXml { + #[xmlserde(name = b"name", ty = "attr")] + pub name: String, } fn default_zero_usize() -> usize { diff --git a/docs/chart.md b/docs/chart.md index c5ceb81e..7e7040eb 100644 --- a/docs/chart.md +++ b/docs/chart.md @@ -34,7 +34,19 @@ xl/charts/chartN.xml (c:chartSpace) ← source of truth, in the .xlsx parts ride along too. Adding a typed field means adding it to *both* `ChartData` and `build_chart_xml`. - **Anchored by stable `CellId`.** Charts shift with row/column insert/delete, - like images. + like images. Their *data ranges* do not — see the known bug below. +- **A chart can be bound to a block instead of a range.** `ChartBlockSource` + (block id + field names) is stored on the controller's `Chart`, outside the + OOXML model, and `chart_manager::resolve_block_refs` turns it into A1 ranges + from the block's *current* place and schema. That resolution runs on every + `get_charts` and every save, so the chart follows the block: records appended + to it are plotted with nothing touching the chart, and a row inserted above + the block cannot leave it pointing at the wrong cells. Fields are held by + name — the identity `#FIELD("qty")` formulas use — so a moved column keeps + the link and a renamed field breaks it. The `.xlsx` still carries real A1 + refs (Excel has no idea what a block is); the binding itself is persisted in + `logisheets.xml` as `` and restored on load, so reopening in + LogiSheets picks the chart back up as bound rather than frozen. - **Rendering library: ECharts**, bundled in `logisheets-engine` as an external dependency + tree-shaken (`echarts/core` + `use()`), rendered as a DOM overlay (`.chart-layer`) positioned from the grid. @@ -45,6 +57,7 @@ xl/charts/chartN.xml (c:chartSpace) ← source of truth, in the .xlsx | --- | --- | --- | | Read & display charts from `.xlsx` | ✅ | user + tests | | Chart follows data edits (live values) | ✅ | `chart_reflects_live_data` | +| Chart bound to a block, follows it as it grows | ✅ | `chart_bound_to_block_follows_it`, `block_bound_chart_survives_save` | | Select (click) | ✅ | user | | Move (drag) | ✅ | `move_chart_updates_anchor` + user | | Resize (corner handles) | ✅ | reuses MoveChart + user | @@ -97,6 +110,17 @@ small subset renderer (separators, decimals, percent, currency affixes). ## What's remaining +**Known bug: a plain chart's ranges do not track edits** +- A chart that holds A1 text is *anchored* by `CellId` but its refs are plain + strings that nothing rewrites. Insert a row above its data and the chart goes + **blank**: the ref still names the old rows, which are now empty, so the live + resolution succeeds and returns nothing. (`resolve_series_values` returning + `Some(vec![None, …])` also means the `numCache` fallback never kicks in.) + Proven by hand on `tests/graph.xlsx`; no test covers it yet. +- The fix is the same shape as the anchor's: hold the range as `CellId` + endpoints and render A1 only at save. Block-bound charts already sidestep it, + because their ranges are recomputed rather than stored. + **Fidelity gaps** - Fonts, fills, gridline styling and 3-D effects survive an edit but are not *rendered* — the ECharts layer draws its own defaults. They are preserved for @@ -197,8 +221,13 @@ service, so the inference is unit-tested without a workbook - `crates/workbook/src/ooxml/chart.rs` — parse (`parse_chart`) + generate (`build_chart_xml`). - `crates/workbook/src/ooxml/drawing_part.rs` — `graphicFrame` anchor model. -- `crates/controller/src/chart_manager/` — `ChartManager` + executor. -- `crates/controller/src/api/worksheet.rs` — `get_charts` (live-value resolution). +- `crates/controller/src/chart_manager/` — `ChartManager` + executor, plus + `block_source.rs` (block binding → live A1 ranges). +- `crates/controller/src/api/worksheet.rs` — `get_charts` (live-value resolution + and live block-range resolution). +- `crates/workbook/src/logisheets.rs` — ``, the persisted binding. +- `packages/logician/src/tools/charts.ts` — Watson's chart tools, including + `chart__from_block`, which states fields and lets the engine find them. - `packages/engine/src/lib/chart/` — ECharts setup, model, renderer, `ChartSettings.svelte` (the editor), `num-format.ts` (axis ticks), `from-info.ts` (binding → model), `from-selection.ts` (insert inference) and diff --git a/packages/engine/src/lib/chart/ChartSettings.svelte b/packages/engine/src/lib/chart/ChartSettings.svelte index 9affdbca..5dd81d5c 100644 --- a/packages/engine/src/lib/chart/ChartSettings.svelte +++ b/packages/engine/src/lib/chart/ChartSettings.svelte @@ -431,6 +431,21 @@
Data
+ {#if chart.blockSource} +
+ Follows block {chart.blockSource.blockId} + + Plots {chart.blockSource.valueFields.join(', ')}{chart + .blockSource.categoryField + ? ` by ${chart.blockSource.categoryField}` + : ''}. The ranges below are worked out from the block, + so records added to it show up on their own — editing a + range by hand detaches the chart from the block. + +
+ {/if}