diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index f0f7492c..45c74ad6 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -24,7 +24,7 @@ use crate::persistence; use crate::ui::effect::{EffectKind, ModelChange, ViewTarget}; use crate::ui::project::Dirty; use crate::ui::viewport::{ - compute_col_widths, compute_row_header_width, compute_visible_cols, grid_width, + GridSummary, compute_row_header_width, compute_visible_cols, grid_summary, grid_width, }; use crate::view::{GridLayout, View}; use crate::workbook::Workbook; @@ -444,7 +444,7 @@ struct LayoutBasis { /// What the keypress path measures of a built layout. pub(crate) struct GridGeometry { - pub col_widths: Vec, + pub summary: GridSummary, pub row_header_width: u16, } @@ -468,6 +468,7 @@ pub struct App { pub term_height: u16, /// **Derived cache**: a pure function of the slices, rebuilt by /// `rebuild_layout()`. Owned by `App` because the rebuild is the host's job. + /// A writer that bypasses effects must call it: a keypress no longer does. pub layout: GridLayout, /// How many times `rebuild_layout` has run: what a keypress that changes /// neither the model nor the view must leave alone (improvise-felo.13). @@ -584,7 +585,7 @@ impl App { width_passes: std::cell::Cell::new(0), geometry: std::cell::OnceCell::new(), layout_basis: LayoutBasis::default(), - layout_stale: true, + layout_stale: false, abort_effects: false, batch_aborted: false, batch_refused: false, @@ -598,6 +599,7 @@ impl App { keymap_set: KeymapSet::default_keymaps(), storage, }; + app.layout_basis = app.current_layout_basis(); app.settle_normal_mode(); app } @@ -963,7 +965,7 @@ impl App { let model = &self.model_state.workbook.model; let plan = FormatPlan::of(model, self.view()); GridGeometry { - col_widths: compute_col_widths(model, &self.layout, &plan), + summary: grid_summary(model, &self.layout, &plan), row_header_width: compute_row_header_width(&self.layout), } }) @@ -1049,7 +1051,7 @@ impl App { visible_cols: { let geometry = self.grid_geometry(); compute_visible_cols( - &geometry.col_widths, + &geometry.summary.col_widths, geometry.row_header_width, self.grid_width(), cursor.col_offset, @@ -1986,7 +1988,7 @@ mod tests { let mut app = two_col_model(); let down = || KeyEvent::new(KeyCode::Down, KeyModifiers::NONE); app.dispatch_key(down()); - let narrow = app.grid_geometry().col_widths.clone(); + let narrow = app.grid_geometry().summary.col_widths.clone(); let (builds, widths) = (app.layout_builds, app.width_passes.get()); let key = CellKey::new(vec![("Row".into(), "A".into()), ("Col".into(), "X".into())]); let long = CellValue::Text("a value far wider than a column".into()); @@ -1995,8 +1997,8 @@ mod tests { app.dispatch_key(down()); assert!(app.width_passes.get() > widths, "an edit must remeasure"); let model = &app.model_state.workbook.model; - let fresh = compute_col_widths(model, &app.layout, &FormatPlan::of(model, app.view())); - assert_eq!(app.grid_geometry().col_widths, fresh, "stale widths"); + let fresh = crate::ui::viewport::compute_col_widths(model, &app.layout, &FormatPlan::of(model, app.view())); + assert_eq!(app.grid_geometry().summary.col_widths, fresh, "stale widths"); assert_ne!(fresh, narrow, "the edit widened a column, or this proves nothing"); } @@ -2249,6 +2251,7 @@ mod tests { .unwrap() .add_item(format!("R{i}")); } + app.rebuild_layout(); app.term_height = 28; // ~20 visible rows → delta = 15 app.cursor_mut().selected = (0, 0); app.handle_key(KeyEvent::new(KeyCode::PageDown, KeyModifiers::NONE)) @@ -2273,6 +2276,7 @@ mod tests { .unwrap() .add_item(format!("R{i}")); } + app.rebuild_layout(); app.term_height = 28; app.cursor_mut().selected = (20, 0); app.handle_key(KeyEvent::new(KeyCode::PageUp, KeyModifiers::NONE)) @@ -2292,6 +2296,7 @@ mod tests { .unwrap() .add_item(format!("R{i}")); } + app.rebuild_layout(); app.term_height = 13; // ~5 visible rows app.cursor_mut().selected = (0, 0); // G jumps to last row (row 12) @@ -2319,6 +2324,7 @@ mod tests { .unwrap() .add_item(format!("R{i}")); } + app.rebuild_layout(); app.term_height = 13; // ~5 visible rows app.cursor_mut().selected = (0, 0); // Ctrl+d scrolls by 5 rows diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 2204ba03..f9593b7e 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -25,7 +25,7 @@ use improvise_protocol::{ use crate::ui::app::App; use crate::ui::effect::{EffectKind, ModelChange}; use crate::ui::reduce::{Checked, apply_command}; -use crate::ui::viewport::{GridBody, grid_body}; +use crate::ui::viewport::{GridBody, grid_body, grid_window}; /// Which widgets a batch of effects could have changed. A gate, not a description: /// over-reporting costs work, under-reporting is a correctness bug (gotcha 35). @@ -144,7 +144,13 @@ pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache { let cols = viewport.cols_in(layout.col_count()); // ONE walk: the widths, the totals and this window's values all come off the // same evaluation — `context/code/command.md` (improvise-jmjz). - let body = grid_body(model, layout, &plan, rows.clone(), cols.clone()); + // The host's own layout keeps its whole-grid measurements until it is rebuilt, so + // a window costs its own cells. Another view's grid has no such cache. + let hosted = elsewhere.is_none() && derived.is_none(); + let body = match hosted { + true => grid_window(model, layout, &app.grid_geometry().summary, rows.clone(), cols.clone()), + false => grid_body(model, layout, &plan, rows.clone(), cols.clone()), + }; GridCache { row_frame: axis_frame(&layout.row_cats, &layout.row_items, rows.clone()), diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index e71519ba..6ab2b84f 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -101,6 +101,50 @@ pub fn grid_body( } } +/// What of a grid build no window changes: the widths, each column's total +/// format and the totals. Measured over every cell, so a host keeps one per layout. +pub struct GridSummary { + pub col_widths: Vec, + pub col_formats: Vec, + pub totals: Vec>, +} + +/// The whole-grid walk, answering only what a window cannot. +pub fn grid_summary(model: &Model, layout: &GridLayout, plan: &FormatPlan) -> GridSummary { + let body = grid_body(model, layout, plan, 0..0, 0..0); + GridSummary { + col_widths: body.col_widths, + col_formats: body.col_formats, + totals: body.totals, + } +} + +/// [`grid_body`] for a window, given the `summary` of the same layout: evaluates +/// only the window's cells. Equal to `grid_body` where `summary` is current. +pub fn grid_window( + model: &Model, + layout: &GridLayout, + summary: &GridSummary, + rows: std::ops::Range, + cols: std::ops::Range, +) -> GridBody { + let eval = layout.evaluator(model); + let records = layout.is_records_mode(); + let window = rows + .map(|ri| { + cols.clone() + .map(|ci| cell_value(layout, &eval, records, ri, ci).1) + .collect() + }) + .collect(); + GridBody { + col_widths: summary.col_widths.clone(), + col_formats: summary.col_formats.clone(), + totals: summary.totals.clone(), + window, + } +} + /// The one evaluation. Records mode reads its wide row; pivot mode aggregates. fn cell_value( layout: &GridLayout, @@ -146,7 +190,7 @@ fn widen(widths: &mut [u16], col: usize, width: usize) { /// Adaptive column widths alone, for the keypress path, which needs no cells. /// It is [`grid_body`] with an empty window so there is one width rule. pub fn compute_col_widths(model: &Model, layout: &GridLayout, plan: &FormatPlan) -> Vec { - grid_body(model, layout, plan, 0..0, 0..0).col_widths + grid_summary(model, layout, plan).col_widths } /// The total row-header width for a layout. The rule is the protocol's; this is the @@ -272,4 +316,26 @@ mod tests { "F0..F8 once for the Q1 row, once for the totals" ); } + + /// improvise-felo.13: a window read off a held summary is the window `grid_body` + /// builds. Evidence: `context/test-evidence/layout-cost.md`. + #[test] + fn a_window_over_a_held_summary_equals_the_one_walk_grid_body() { + let wb = folded_chain(); + let view = wb.active_view(); + let layout = GridLayout::new(&wb.model, view); + let plan = FormatPlan::of(&wb.model, view); + let summary = grid_summary(&wb.model, &layout, &plan); + let (rows, cols) = (layout.row_count(), layout.col_count()); + assert!(rows >= 2 && cols >= 3, "a window strictly inside is possible"); + for (r, c) in [(0..rows, 0..cols), (1..2, 1..3), (rows - 1..rows, 0..1), (0..0, 0..0)] { + let walked = grid_body(&wb.model, &layout, &plan, r.clone(), c.clone()); + let held = grid_window(&wb.model, &layout, &summary, r.clone(), c.clone()); + assert_eq!(held.window, walked.window, "window {r:?} x {c:?}"); + assert_eq!(held.col_widths, walked.col_widths); + assert_eq!(held.totals, walked.totals); + assert_eq!(held.col_formats, walked.col_formats); + } + } + } diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index dbae8faa..e31c87a3 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -194,6 +194,12 @@ impl RecordEntry { } } +#[cfg(test)] +thread_local! { + /// How many lookups fell through to set narrowing, for the exact-path test. + static NARROWED: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + #[derive(Clone, Default)] pub struct DataStore { /// Primary storage: interned keys, insertion-ordered so records mode can @@ -206,6 +212,9 @@ pub struct DataStore { /// Reverse index for the client-facing surrogate (ADR 002), plus each /// record's write counter. Why it lives here: core.md §8. records: HashMap, + /// How many stored cells have each number of coordinates. Its largest key + /// says when a full-width partial can only match one exact cell (§8). + arities: std::collections::BTreeMap, } /// Resolved through the symbol table and sorted wherever the storage is a @@ -217,6 +226,7 @@ impl std::fmt::Debug for DataStore { symbols, index, records, + arities: _, } = self; let cells: Vec<_> = cells .iter() @@ -291,7 +301,11 @@ impl DataStore { for pair in &ikey.0 { self.index.entry(*pair).or_default().insert(ikey.clone()); } + let width = ikey.0.len(); let is_new_cell = self.cells.insert(ikey, value).is_none(); + if is_new_cell { + *self.arities.entry(width).or_default() += 1; + } let entry = self.records.entry(hash).or_insert(RecordEntry { record, cells: 0, @@ -327,6 +341,12 @@ impl DataStore { return; }; if self.cells.shift_remove(&ikey).is_some() { + if let Some(n) = self.arities.get_mut(&ikey.0.len()) { + *n -= 1; + if *n == 0 { + self.arities.remove(&ikey.0.len()); + } + } for pair in &ikey.0 { if let Some(set) = self.index.get_mut(pair) { set.remove(&ikey); @@ -378,11 +398,29 @@ impl DataStore { /// Positions in `cells` of every cell matching `partial`, **in insertion /// order**. The index narrows; the primary map orders (§8). fn matching_positions(&self, partial: &[(String, String)]) -> Vec { + if let Some(exact) = self.exact_positions(partial) { + return exact; + } self.intern_partial(partial) .map(|pairs| self.positions_of(&pairs)) .unwrap_or_default() } + /// The answer without narrowing, when `partial` names as many coordinates as + /// the widest stored cell has: only a cell equal to it can match (core.md §8). + /// `None` where that does not hold, and the narrowing path answers. + fn exact_positions(&self, partial: &[(String, String)]) -> Option> { + let widest = *self.arities.keys().next_back()?; + // Strictly sorted by category, the order a stored key is in, so one probe is the lookup. + let canonical = partial.windows(2).all(|w| w[0].0 < w[1].0); + (!partial.is_empty() && widest <= partial.len() && canonical).then(|| { + let found = self.intern_partial(partial).and_then(|pairs| { + self.cells.get_index_of(&InternedKey(pairs)) + }); + found.into_iter().collect() + }) + } + /// `partial` as symbols, or `None` when a name was never interned, which /// no stored cell can then match. fn intern_partial(&self, partial: &[(String, String)]) -> Option> { @@ -412,6 +450,8 @@ impl DataStore { } sets.sort_by_key(|s| s.len()); + #[cfg(test)] + NARROWED.with(|n| n.set(n.get() + 1)); let mut positions: Vec = match sets.split_first() { Some((first, rest)) => first @@ -1356,3 +1396,76 @@ mod record_index { } } } + +#[cfg(test)] +mod exact_path_tests { + use super::{CellKey, CellValue, DataStore, NARROWED}; + + fn key(a: &str, b: &str, c: &str) -> CellKey { + CellKey::new(vec![("A".into(), a.into()), ("B".into(), b.into()), ("C".into(), c.into())]) + } + + fn store() -> DataStore { + let mut d = DataStore::new(); + for i in 0..6 { + for j in 0..6 { + d.set(key(&format!("a{i}"), &format!("b{j}"), "c"), CellValue::Number((i * 6 + j) as f64)); + } + } + d + } + + fn narrowings(f: impl FnOnce()) -> usize { + NARROWED.with(|n| n.set(0)); + f(); + NARROWED.with(|n| n.get()) + } + + /// improvise-felo.13: context/test-evidence/layout-cost.md. + #[test] + fn a_full_width_partial_probes_one_cell_instead_of_narrowing() { + let d = store(); + let full = key("a2", "b3", "c").0; + let mut got = Vec::new(); + assert_eq!(narrowings(|| got = d.matching_values(&full).into_iter().cloned().collect()), 0); + assert_eq!(got, vec![CellValue::Number(15.0)]); + let missing = key("a2", "b9", "c").0; + assert!(d.matching_values(&missing).is_empty(), "an absent cell matches nothing"); + let unknown = key("a2", "b3", "zzz").0; + assert!(d.matching_values(&unknown).is_empty(), "an unseen item matches nothing"); + } + + /// The control: a narrower partial still sums its supersets, so the shortcut + /// is not a blanket exact lookup. + #[test] + fn a_narrower_partial_still_narrows_and_matches_every_superset() { + let d = store(); + let narrow = vec![("A".to_string(), "a2".to_string()), ("C".to_string(), "c".to_string())]; + let mut n = 0; + assert_eq!(narrowings(|| n = d.matching_values(&narrow).len()), 1); + assert_eq!(n, 6); + } + + /// A wider cell appearing, or leaving, moves the line the shortcut is allowed at. + #[test] + fn a_wider_cell_turns_the_shortcut_off_until_it_is_removed() { + let mut d = store(); + let wide = CellKey::new(vec![ + ("A".into(), "a2".into()), ("B".into(), "b3".into()), ("C".into(), "c".into()), ("D".into(), "d".into()), + ]); + d.set(wide.clone(), CellValue::Number(100.0)); + let full = key("a2", "b3", "c").0; + assert_eq!(d.matching_values(&full).len(), 2, "the exact cell and the wider cell under it"); + d.remove(&wide); + assert_eq!(narrowings(|| assert_eq!(d.matching_values(&full).len(), 1)), 0); + } + + /// An unsorted partial is not a stored key's order, so it must not be probed. + #[test] + fn an_unsorted_full_width_partial_is_still_answered() { + let d = store(); + let mut full = key("a2", "b3", "c").0; + full.reverse(); + assert_eq!(d.matching_values(&full).len(), 1); + } +}