diff --git a/crates/engine/src/cmd/pivot.rs b/crates/engine/src/cmd/pivot.rs index 229ff22b6..ae5446715 100644 --- a/crates/engine/src/cmd/pivot.rs +++ b/crates/engine/src/cmd/pivot.rs @@ -2580,4 +2580,35 @@ mod tests { } assert_eq!(s.doc().unwrap().wb.sheets.len(), 2); } + + #[test] + fn detail_cells_are_the_cells_show_details_takes() { + let mut s = sales(); + s.execute("insert.pivotTable", json!({"rows": ["Region"], "columns": ["Product"], "values": ["Sales"]})).unwrap(); + for layout in 0..2 { + if layout == 1 { + // A two-level row axis, with subtotals at the top of each group. + s.execute("pivot.moveField", json!({"field": "Product", "from": "columns", "to": "rows"})).unwrap(); + } + let d = s.doc().unwrap(); + let si = d.wb.active_sheet; + let pt = &d.wb.active().unwrap().pivots[0]; + let r = pt.last_range.unwrap(); + let cells = crate::pivot::detail_cells(&d.wb, pt, si).unwrap(); + let mut values = 0; + for row in 0..r.height() { + for col in 0..r.width() { + let one = crate::pivot::detail_records(&d.wb, pt, si, row, col).unwrap(); + assert_eq!(cells.contains(&(row, col)), one.is_some(), "layout {layout}, cell ({row}, {col})"); + values += usize::from(one.is_some()); + } + } + assert!(values > 4, "layout {layout} has value cells"); + } + // An out-of-date report answers as Show Details does. + s.execute("range.setValues", json!({"range": "Sheet1!A2", "values": [["Central"]]})).unwrap(); + let d = s.doc().unwrap(); + let pt = &d.wb.active().unwrap().pivots[0]; + assert!(crate::pivot::detail_cells(&d.wb, pt, d.wb.active_sheet).unwrap_err().contains("out of date")); + } } diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index 397dd5ad2..a490824da 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -816,11 +816,30 @@ pub const NOT_A_VALUE_CELL: &str = "select a value cell in a PivotTable"; /// on the sheet is no longer what the source computes: the cell's records would be another cell's. pub fn detail_records(wb: &Workbook, pt: &PivotTable, pivot_sheet: usize, row: u32, col: u32) -> Result)>, String> { let src = read_source(wb, &pt.source, pivot_sheet)?; - let mut probe = Probe { at: (row as usize, col as usize), hit: None, paths: Vec::new() }; + let mut probe = Probe { at: Some((row as usize, col as usize)), hit: None, paths: Some(Vec::new()), cells: None }; let out = compute_from(wb, pt, &src, Some(&mut probe))?; - // Excel reads its PivotTable cache; without one, the report written by the last refresh - // must still be what the source gives. Checked first: in a stale report even a cell that - // computes as a label may show a value. + // Checked first: in a stale report even a cell that computes as a label may show a value. + check_current(wb, pt, pivot_sheet, &out)?; + let Some((rp, cp)) = probe.hit else { return Ok(None) }; + let records = + probe.paths.unwrap_or_default().into_iter().filter(|(_, r, c)| r.starts_with(&rp) && c.starts_with(&cp)).map(|(i, _, _)| i).collect(); + Ok(Some((src, records))) +} + +/// The cells of the report grid (relative to its top-left) where [`detail_records`] finds +/// records: its value, subtotal and grand total cells, from one computation of the report. The +/// same errors as [`detail_records`]. +pub fn detail_cells(wb: &Workbook, pt: &PivotTable, pivot_sheet: usize) -> Result, String> { + let src = read_source(wb, &pt.source, pivot_sheet)?; + let mut probe = Probe { at: None, hit: None, paths: None, cells: Some(HashSet::new()) }; + let out = compute_from(wb, pt, &src, Some(&mut probe))?; + check_current(wb, pt, pivot_sheet, &out)?; + Ok(probe.cells.unwrap_or_default().into_iter().map(|(r, c)| (r as u32, c as u32)).collect()) +} + +/// An error when the report on the sheet is not what `out` computes. Excel reads its PivotTable +/// cache; without one, the report written by the last refresh must still be what the source gives. +fn check_current(wb: &Workbook, pt: &PivotTable, pivot_sheet: usize, out: &Output) -> Result<(), String> { let sh = wb.sheet(pivot_sheet).ok_or("There's no such sheet.")?; let current = pt.last_range.is_some_and(|r| { r.height() == out.height().max(1) @@ -834,17 +853,17 @@ pub fn detail_records(wb: &Workbook, pt: &PivotTable, pivot_sheet: usize, row: u if !current { return Err("The PivotTable report is out of date. Refresh it first.".into()); } - let Some((rp, cp)) = probe.hit else { return Ok(None) }; - let records = probe.paths.into_iter().filter(|(_, r, c)| r.starts_with(&rp) && c.starts_with(&cp)).map(|(i, _, _)| i).collect(); - Ok(Some((src, records))) + Ok(()) } -/// What [`detail_records`] asks of a report computation: the row and column item paths of the -/// value cell at `at`, and the paths of every record the filters let through. +/// What [`detail_records`] and [`detail_cells`] ask of a report computation: the row and column +/// item paths of the value cell at `at`, the paths of every record the filters let through (when +/// `paths` is set), and every value cell (when `cells` is set). struct Probe { - at: (usize, usize), + at: Option<(usize, usize)>, hit: Option<(Vec, Vec)>, - paths: Vec<(usize, Vec, Vec)>, + paths: Option, Vec)>>, + cells: Option>, } fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut probe: Option<&mut Probe>) -> Result { @@ -959,8 +978,8 @@ fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut probe: Option< if !col_leaves.contains(&cp[..shown]) { col_leaves.insert(cp[..shown].to_vec()); } - if let Some(pr) = probe.as_deref_mut() { - pr.paths.push((ri, rp.clone(), cp.clone())); + if let Some(paths) = probe.as_deref_mut().and_then(|pr| pr.paths.as_mut()) { + paths.push((ri, rp.clone(), cp.clone())); } row_set.insert(rp); col_set.insert(cp); @@ -1326,10 +1345,13 @@ fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut probe: Option< _ => raw, }, }; - if let Some(pr) = probe.as_deref_mut() - && pr.at == (r, l + j) - { - pr.hit = Some((rp.to_vec(), cp.to_vec())); + if let Some(pr) = probe.as_deref_mut() { + if pr.at == Some((r, l + j)) { + pr.hit = Some((rp.to_vec(), cp.to_vec())); + } + if let Some(cells) = pr.cells.as_mut() { + cells.insert((r, l + j)); + } } g.set(r, l + j, shown, ck.1, 0, data_fmt.get(*vi).cloned().flatten()); } diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index abe8bcf42..8b36f520b 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -163,9 +163,11 @@ pub struct GridState { /// The PivotTable Fields pane's `pivot.fields` answer, cached per (doc uid, revision, pivot): /// it scans the whole source list, far too slow to repeat every frame (#2113). pub pivot_fields: Option<((u64, u64, String), serde_json::Value)>, - /// Whether the cell menu offers Show Details, cached per (doc uid, revision, sheet, active - /// cell): finding out computes the whole PivotTable report. - pub show_details_item: Option<((u64, u64, usize, CellRef), bool)>, + /// The cells of a PivotTable report where the cell menu offers Show Details, relative to the + /// report's top-left and cached per (doc uid, revision, sheet, pivot): finding them computes + /// the whole report. `None` when the report can't be computed or is out of date: every cell + /// of it offers Show Details, and running it says why. + pub show_details_cells: Option<((u64, u64, usize, usize), Option>)>, pub header_menu: Option<(Pos2, bool)>, /// Page Layout view pagination, cached per (doc uid, revision, sheet, rows, columns). pub paper: Option, @@ -3424,20 +3426,21 @@ fn context_menu(app: &mut SheetApp, ui: &mut egui::Ui) { fn show_details_offered(app: &mut SheetApp) -> bool { let Some(d) = app.session.active() else { return false }; let (si, at) = (d.wb.active_sheet, d.selection.active); - let key = (d.uid, d.revision, si, at); - if let Some((k, on)) = app.grid.show_details_item - && k == key + let Some((pi, pt)) = d.wb.active().and_then(|sh| sh.pivots.iter().enumerate().find(|(_, pt)| pt.last_range.is_some_and(|r| r.contains(at)))) + else { + return false; + }; + let origin = pt.last_range.map(|r| r.start).unwrap_or_default(); + let cell = (at.row.saturating_sub(origin.row), at.col.saturating_sub(origin.col)); + let key = (d.uid, d.revision, si, pi); + if let Some((k, cells)) = &app.grid.show_details_cells + && *k == key { - return on; + return cells.as_ref().is_none_or(|c| c.contains(&cell)); } - let on = d.wb.active().and_then(|sh| sh.pivots.iter().find(|pt| pt.last_range.is_some_and(|r| r.contains(at)))).is_some_and(|pt| { - let origin = pt.last_range.map(|r| r.start).unwrap_or_default(); - !matches!( - gridcraft_engine::pivot::detail_records(&d.wb, pt, si, at.row.saturating_sub(origin.row), at.col.saturating_sub(origin.col)), - Ok(None) - ) - }); - app.grid.show_details_item = Some((key, on)); + let cells = gridcraft_engine::pivot::detail_cells(&d.wb, pt, si).ok(); + let on = cells.as_ref().is_none_or(|c| c.contains(&cell)); + app.grid.show_details_cells = Some((key, cells)); on } diff --git a/crates/ui-egui/tests/it/pivot_show_details_menu.rs b/crates/ui-egui/tests/it/pivot_show_details_menu.rs index 02527d8a3..73172155e 100644 --- a/crates/ui-egui/tests/it/pivot_show_details_menu.rs +++ b/crates/ui-egui/tests/it/pivot_show_details_menu.rs @@ -4,7 +4,7 @@ use egui::{Event, Modifiers, PointerButton, Pos2, vec2}; use egui_kittest::kittest::Queryable as _; use gridcraft_engine::Session; -use gridcraft_engine::core::CellRef; +use gridcraft_engine::core::{CellRef, Value}; use gridcraft_ui_egui::{SheetApp, grid::Geo}; use serde_json::json; @@ -90,6 +90,23 @@ fn labels_of_the_report_and_ordinary_cells_have_no_show_details_item() { } } +#[test] +fn the_cell_menu_computes_the_report_once_for_all_its_cells() { + let mut h = harness(); + right_click(&mut h, "E4"); + assert!(h.query_by_label("Show Details").is_some()); + let escape = Event::Key { key: egui::Key::Escape, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::NONE }; + h.input_mut().events.push(escape); + h.run_steps(2); + // West goes away behind the session's back, so the revision stays the same. A menu that + // computed the report again for D4 would find it out of date and offer Show Details there. + let doc = h.state_mut().session.active_mut().unwrap(); + let si = doc.wb.active_sheet; + std::sync::Arc::make_mut(&mut doc.wb).sheet_mut(si).unwrap().set_value(CellRef::parse("A3").unwrap(), Value::text("East")); + right_click(&mut h, "D4"); + assert!(h.query_by_label("Show Details").is_none(), "D4 is a label of the report computed for E4"); +} + #[test] fn show_details_in_an_out_of_date_report_asks_for_a_refresh() { let mut h = harness();