diff --git a/Cargo.toml b/Cargo.toml index 7a04d74d..86138e9c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,11 @@ name = "grid_projection" harness = false test = false +[[bench]] +name = "reshape_observation" +harness = false +test = false + [dependencies] gp-tree = { path = "crates/gp-tree" } improvise-core = { path = "crates/improvise-core" } diff --git a/benches/reshape_observation.rs b/benches/reshape_observation.rs new file mode 100644 index 00000000..a685a475 --- /dev/null +++ b/benches/reshape_observation.rs @@ -0,0 +1,96 @@ +//! What a model batch costs on the keypress path, which is where the reshape +//! observation sits (improvise-ni6k). Run with +//! `cargo bench --bench reshape_observation`. + +use std::time::{Duration, Instant}; + +use improvise::model::cell::{CellKey, CellValue}; +use improvise::view::Axis; +use improvise::workbook::Workbook; +use improvise_command::ui::app::App; +use improvise_command::ui::effect::{Effect, SetCell}; + +/// The fixture `grid_projection` uses, so the two numbers are comparable. +fn dense_app(regions: usize, products: usize, measures: usize) -> App { + let mut wb = Workbook::new("Bench"); + wb.add_category("Region").unwrap(); + wb.add_category("Product").unwrap(); + let regions: Vec = (0..regions).map(|i| format!("Region{i}")).collect(); + let products: Vec = (0..products).map(|i| format!("Product{i}")).collect(); + let measures: Vec = (0..measures).map(|i| format!("Measure{i}")).collect(); + for r in ®ions { + wb.add_item("Region", r.clone()).unwrap(); + } + for p in &products { + wb.add_item("Product", p.clone()).unwrap(); + } + for m in &measures { + wb.add_item("_Measure", m.clone()).unwrap(); + } + { + let v = wb.active_view_mut(); + v.set_axis("Region", Axis::Row); + v.set_axis("Product", Axis::Row); + v.set_axis("_Measure", Axis::Column); + } + let mut n = 0.0f64; + for r in ®ions { + for p in &products { + for m in &measures { + n += 1.0; + let key = CellKey::new(vec![ + ("Region".to_string(), r.clone()), + ("Product".to_string(), p.clone()), + ("_Measure".to_string(), m.clone()), + ]); + wb.model.set_cell(key, CellValue::Number(n)); + } + } + } + // More than one view, because the observation runs over all of them. + wb.create_view("Second"); + wb.switch_view("Default").unwrap(); + let mut app = App::new(wb, None); + app.rebuild_layout(); + app +} + +/// Min-of-N for the reason `grid_projection` says: a mean over a few runs +/// measures the machine, not the code. +fn time_batch(iters: u32, app: &mut App, key: &CellKey) -> Duration { + let mut best = Duration::MAX; + for i in 0..iters.max(3) { + let effects: Vec> = vec![Box::new(SetCell( + key.clone(), + CellValue::Number(i as f64 + 1.0), + ))]; + let start = Instant::now(); + std::hint::black_box(app.apply_effects(effects)); + best = best.min(start.elapsed()); + } + best +} + +fn case(label: &str, regions: usize, products: usize, measures: usize, iters: u32) { + let mut app = dense_app(regions, products, measures); + let (rows, cols) = (app.layout.row_count(), app.layout.col_count()); + let key = CellKey::new(vec![ + ("Region".to_string(), "Region0".to_string()), + ("Product".to_string(), "Product0".to_string()), + ("_Measure".to_string(), "Measure0".to_string()), + ]); + let batch = time_batch(iters, &mut app, &key); + println!( + "{label:<22} {rows:>5} x {cols:>3} = {:>7} cells one SetCell batch {batch:>11.3?}", + rows * cols + ); +} + +fn main() { + println!("App::apply_effects, one model batch = one keypress that writes a cell"); + println!("a 60fps frame budget is 16.7ms\n"); + case("typical", 20, 5, 4, 200); + case("large", 60, 20, 8, 50); + case("18k cells (gotcha 41)", 150, 20, 6, 20); + case("pathological", 300, 30, 10, 5); +} diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 60776c0d..1cfe84d2 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -70,6 +70,69 @@ fn view_shape(model: &crate::model::types::Model, view: &View) -> u64 { h.finish() } +/// One view's drawn **frame**, as a hash: the axis entries a grid would draw, in +/// order. A different question from [`view_shape`] — `context/code/command.md` +/// §"two observations" (improvise-ni6k). +fn view_frame(model: &crate::model::types::Model, view: &View) -> u64 { + use std::hash::{Hash, Hasher}; + // `with_frozen_records`, not `new`: `new` does NOT apply `prune_empty`, and a + // pruned row is exactly the kind of reframing this observation exists for. + let layout = crate::view::layout::GridLayout::with_frozen_records(model, view, None); + let mut h = std::collections::hash_map::DefaultHasher::new(); + layout.row_cats.hash(&mut h); + layout.col_cats.hash(&mut h); + for entries in [&layout.row_items, &layout.col_items] { + entries.len().hash(&mut h); + for entry in entries { + match entry { + crate::view::layout::AxisEntry::GroupHeader { + cat_name, + group_name, + } => { + 0u8.hash(&mut h); + cat_name.hash(&mut h); + group_name.hash(&mut h); + view.is_group_collapsed(cat_name, group_name).hash(&mut h); + } + crate::view::layout::AxisEntry::DataItem(tuple) => { + 1u8.hash(&mut h); + tuple.hash(&mut h); + } + } + } + } + // Not a label, but it is what a Page coordinate names: two pages with the + // same row labels are different grids, and the cells under them differ. + let mut pages: Vec<(&String, &String)> = view.page_selections.iter().collect(); + pages.sort_unstable(); + pages.hash(&mut h); + h.finish() +} + +/// The names whose value differs between two shape maps. The union of the key +/// sets is the domain: a view gained or lost differs at that name. +fn differing_keys( + before: &std::collections::BTreeMap, + after: &std::collections::BTreeMap, +) -> std::collections::BTreeSet { + before + .keys() + .chain(after.keys()) + .filter(|name| before.get(*name) != after.get(*name)) + .cloned() + .collect() +} + +/// Every view's drawn frame, keyed by view name. The cursor's half of the pair +/// `view_shapes` opens. +fn view_frames(workbook: &Workbook) -> std::collections::BTreeMap { + workbook + .views + .iter() + .map(|(name, view)| (name.clone(), view_frame(&workbook.model, view))) + .collect() +} + /// Every view's drawn shape, keyed by view name. The map answers two questions: /// did *anything* move, and *which views* moved (improvise-nynk). fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap { @@ -229,6 +292,9 @@ pub struct App { /// The views the **last** batch was observed to reshape. The outcome, where /// `view_shapes` is the mechanism — the shape `batch_aborted` takes. last_reshaped_views: std::collections::BTreeSet, + /// The views the **last** batch was observed to REFRAME: different rows or + /// columns, in a different order. What invalidates a cursor (improvise-ni6k). + last_reframed_views: std::collections::BTreeSet, /// **Config**: assembled at startup by `KeymapSet::default_keymaps()`. Read /// every keypress, never mutated during a session. keymap_set: KeymapSet, @@ -282,6 +348,7 @@ impl App { layout_version: 0, cursor_epoch: 0, last_reshaped_views: std::collections::BTreeSet::new(), + last_reframed_views: std::collections::BTreeSet::new(), keymap_set: KeymapSet::default_keymaps(), storage, } @@ -454,6 +521,12 @@ impl App { &self.last_reshaped_views } + /// Which views the last batch actually **reframed** — different rows or + /// columns. Empty means "not known", never "none"; a reader must fail safe. + pub fn last_reframed_views(&self) -> &std::collections::BTreeSet { + &self.last_reframed_views + } + /// Rebuild the grid layout from current workbook, active view, and drill /// state. Note: `with_frozen_records` already handles pruning internally. pub fn rebuild_layout(&mut self) { @@ -575,6 +648,8 @@ impl App { .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData))) .then(|| record_fingerprints(&self.model_state.workbook.model.data)); let shapes_before = touches_model.then(|| view_shapes(&self.model_state.workbook)); + let mut frame_basis = touches_model.then(|| view_frames(&self.model_state.workbook)); + self.last_reframed_views.clear(); let mut config_basis = touches_model.then(|| document_config(&self.model_state.workbook)); let mut dirty = Dirty::NOTHING; for effect in effects { @@ -604,6 +679,20 @@ impl App { self.model_state.dirty |= now != basis; config_basis = Some(now); } + // The cursor's observation, **inside the loop** for the same reason: + // a batch that reframes and then places the cursor deliberately must + // keep the placement — `context/code/command.md` (improvise-ni6k). + if matches!(kind, EffectKind::Model(_)) + && let Some(basis) = frame_basis.as_ref() + { + let now = view_frames(&self.model_state.workbook); + let moved = differing_keys(basis, &now); + if moved.contains(self.view_name()) { + self.reset_cursor(); + } + self.last_reframed_views.extend(moved); + frame_basis = Some(now); + } if self.abort_effects { // An abort-setting effect asked for the rest to be skipped. Reset // the flag, and record the outcome where a caller can read it. @@ -626,14 +715,7 @@ impl App { self.last_reshaped_views.clear(); let Some(before) = before else { return }; let after = view_shapes(&self.model_state.workbook); - // A view gained or lost is a reshape of that view's name, so the union of - // the two key sets is the domain — not either one alone. - self.last_reshaped_views = before - .keys() - .chain(after.keys()) - .filter(|name| before.get(*name) != after.get(*name)) - .cloned() - .collect(); + self.last_reshaped_views = differing_keys(&before, &after); if !self.last_reshaped_views.is_empty() { self.layout_version += 1; } diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index babc9d6a..2170a340 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -66,21 +66,6 @@ macro_rules! simple_effect { // ── Model mutations ────────────────────────────────────────────────────────── -/// The items this category contributes to the grid. Compared across a mutation to -/// see whether it moved anything — see `context/code/command.md` on no-op resets. -fn effective_items(app: &App, category: &str) -> Vec { - app.model_state - .workbook - .model - .effective_item_names(category) -} - -/// Does the view draw this item at all? `hide_item` records a name whether or not -/// the model has one, and hiding what was never drawn moves nothing. -fn item_is_drawn(app: &App, category: &str, item: &str) -> bool { - effective_items(app, category).iter().any(|i| i == item) -} - #[derive(Debug)] pub struct AddCategory(pub String); impl Effect for AddCategory { @@ -88,7 +73,7 @@ impl Effect for AddCategory { // A refusal aborts the batch for gotcha 21's reason: what follows // assumes the category exists (improvise-2zq). match app.model_state.workbook.add_category(&self.0) { - Ok(_) => app.reset_cursor(), + Ok(_) => {} Err(e) => { app.view_state.status_msg = e.to_string(); app.abort_effects = true; @@ -107,7 +92,6 @@ pub struct AddItem { } impl Effect for AddItem { fn apply(&self, app: &mut App) { - let before = effective_items(app, &self.category); if let Err(e) = app .model_state .workbook @@ -115,9 +99,6 @@ impl Effect for AddItem { { app.view_state.status_msg = e.to_string(); } - if effective_items(app, &self.category) != before { - app.reset_cursor(); - } } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::CellData) @@ -132,7 +113,6 @@ pub struct AddItemInGroup { } impl Effect for AddItemInGroup { fn apply(&self, app: &mut App) { - let before = effective_items(app, &self.category); if let Err(e) = app.model_state .workbook @@ -140,9 +120,6 @@ impl Effect for AddItemInGroup { { app.view_state.status_msg = e.to_string(); } - if effective_items(app, &self.category) != before { - app.reset_cursor(); - } } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::CellData) @@ -269,7 +246,6 @@ impl Effect for TogglePruneEmpty { fn apply(&self, app: &mut App) { let v = app.view_mut(); v.prune_empty = !v.prune_empty; - app.reset_cursor(); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -296,13 +272,9 @@ pub struct RemoveItem { } impl Effect for RemoveItem { fn apply(&self, app: &mut App) { - let before = effective_items(app, &self.category); app.model_state .workbook .remove_item(&self.category, &self.item); - if effective_items(app, &self.category) != before { - app.reset_cursor(); - } } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::CellData) @@ -313,11 +285,7 @@ impl Effect for RemoveItem { pub struct RemoveCategory(pub String); impl Effect for RemoveCategory { fn apply(&self, app: &mut App) { - let existed = app.model_state.workbook.model.category(&self.0).is_some(); app.model_state.workbook.remove_category(&self.0); - if existed { - app.reset_cursor(); - } } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -419,14 +387,7 @@ pub struct SetAxis { } impl Effect for SetAxis { fn apply(&self, app: &mut App) { - let view = app.view_mut(); - let moved = view - .try_axis_of(&self.category) - .is_some_and(|a| a != self.axis); - view.set_axis(&self.category, self.axis); - if moved { - app.reset_cursor(); - } + app.view_mut().set_axis(&self.category, self.axis); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -455,21 +416,8 @@ pub struct ToggleGroup { } impl Effect for ToggleGroup { fn apply(&self, app: &mut App) { - let real = app - .model_state - .workbook - .model - .category(&self.category) - .is_some_and(|c| { - c.items - .values() - .any(|i| i.group.as_deref() == Some(self.group.as_str())) - }); app.view_mut() .toggle_group_collapse(&self.category, &self.group); - if real { - app.reset_cursor(); - } } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -483,13 +431,7 @@ pub struct HideItem { } impl Effect for HideItem { fn apply(&self, app: &mut App) { - let drawn = item_is_drawn(app, &self.category, &self.item); - let view = app.view_mut(); - let dropped = drawn && !view.is_hidden(&self.category, &self.item); - view.hide_item(&self.category, &self.item); - if dropped { - app.reset_cursor(); - } + app.view_mut().hide_item(&self.category, &self.item); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -503,13 +445,7 @@ pub struct ShowItem { } impl Effect for ShowItem { fn apply(&self, app: &mut App) { - let drawn = item_is_drawn(app, &self.category, &self.item); - let view = app.view_mut(); - let restored = drawn && view.is_hidden(&self.category, &self.item); - view.show_item(&self.category, &self.item); - if restored { - app.reset_cursor(); - } + app.view_mut().show_item(&self.category, &self.item); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -521,9 +457,6 @@ pub struct TransposeAxes; impl Effect for TransposeAxes { fn apply(&self, app: &mut App) { app.view_mut().transpose_axes(); - // Swapping the axes renumbers every row and column, so the old indices - // name different cells (improvise-8uz, gotcha 24). - app.reset_cursor(); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -535,9 +468,6 @@ pub struct CycleAxis(pub String); impl Effect for CycleAxis { fn apply(&self, app: &mut App) { app.view_mut().cycle_axis(&self.0); - // Same reasoning as TransposeAxes: the grid changes shape underneath - // the cursor, so it goes back to the origin (improvise-8uz). - app.reset_cursor(); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index ebf5508d..b11d9432 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -971,7 +971,9 @@ impl Sessions { if app.cursor_epoch() == before { return Vec::new(); } - let reshaped = app.last_reshaped_views(); + // The REFRAME set, not the reshape set: a cursor is invalidated by the + // rows moving, which is a different question from the basis's (ni6k). + let reshaped = app.last_reframed_views(); self.sessions .iter_mut() .filter(|(_, session)| cursor_invalidated(reshaped, &session.view.active_view)) @@ -2268,6 +2270,16 @@ mod tests { category: "Product".into(), item: "Widget".into(), }, + // The class improvise-ni6k added, none of which any effect declares: + // a formula target is a `_Measure` item, so it is a column (gotcha 7). + ModelCommand::AddFormula { + raw: "Margin = Revenue - Cost".into(), + target_category: "_Measure".into(), + }, + ModelCommand::RemoveFormula { + target: "Profit".into(), + target_category: "_Measure".into(), + }, ] } @@ -2403,6 +2415,92 @@ mod tests { } } + /// A grid whose rows depend on the page: `Service` has data on Q1 only, so + /// pruning drops it on Q2 and every row below it renumbers (improvise-7kvw). + fn app_with_a_page_dependent_grid() -> App { + let mut wb = Workbook::new("Paged"); + wb.add_category("Region").unwrap(); + wb.add_category("Quarter").unwrap(); + for region in ["North", "Service", "South"] { + wb.add_item("Region", region).unwrap(); + } + for quarter in ["Q1", "Q2"] { + wb.add_item("Quarter", quarter).unwrap(); + } + wb.add_item("_Measure", "Revenue").unwrap(); + { + let v = wb.active_view_mut(); + v.set_axis("Region", Axis::Row); + v.set_axis("_Measure", Axis::Column); + v.set_axis("Quarter", Axis::Page); + v.set_page_selection("Quarter", "Q1"); + v.prune_empty = true; + } + for (region, quarter) in [ + ("North", "Q1"), + ("North", "Q2"), + ("Service", "Q1"), + ("South", "Q1"), + ("South", "Q2"), + ] { + let key = CellKey::new(vec![ + ("Region".to_string(), region.to_string()), + ("Quarter".to_string(), quarter.to_string()), + ("_Measure".to_string(), "Revenue".to_string()), + ]); + wb.model.set_cell(key, CellValue::Number(1.0)); + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + app + } + + /// improvise-7kvw, closed as a CONSEQUENCE of improvise-ni6k rather than by its + /// own arm: `SetPageSelection` declares nothing and is observed like the rest. + #[test] + fn paging_across_a_prune_that_drops_a_row_invalidates_the_cursor() { + use improvise_protocol::ModelCommand; + + let mut app = app_with_a_page_dependent_grid(); + let before = grid_shape(&app); + let mut sessions = Sessions::default(); + let (asker, _) = sessions.connect(&app, 100, 100); + let (bystander, _) = sessions.connect(&app, 100, 100); + sessions.apply(&mut app, bystander, &Command::SetRowOffset(1)); + + let applied = sessions.apply_caught_up( + &mut app, + asker, + ModelCommand::SetPageSelection { + category: "Quarter".into(), + item: "Q2".into(), + }, + ); + + assert_ne!( + before, + grid_shape(&app), + "the fixture only says anything if pruning really drops a row on Q2" + ); + assert_eq!( + sessions + .get_mut(bystander) + .unwrap() + .view() + .cursor() + .row_offset, + 0, + "a page flip renumbered the grid and left a bystander indexing the old one" + ); + let broadcast = diff(applied).1; + for (who, id) in [("asker", asker), ("bystander", bystander)] { + assert!( + broadcast.contains(&Broadcast::To(id, Command::ResetCursor)), + "the {who}'s mirror was reset and told nothing about it" + ); + } + } + /// The second negative arm: a command the document REFUSES reshaped nothing, so /// it owes nobody an invalidation. Every refusable effect, not one of them. #[test]