Invalidate the cursor by observing the grid's frame, not by a per-effect table

improvise-ni6k. Twelve effects called `app.reset_cursor()`; that table is
the shape gotcha 24 names as the disease, and it had a hole the day it
was written. The observation replaces it: `view_frames` fingerprints
every view's drawn frame, `apply_effects` diffs it per model effect, and
the cursor resets when the view it is in reframes. No effect declares
anything, and `adopt_cursor_epoch` reads the same set.

FOUR THINGS THE TICKET DID NOT PREDICT, each one a fixture or a design
question rather than a line:

1. The frame fingerprint and `layout_version` answer DIFFERENT questions,
   so they are two observations and not one. `AddCategory` of an empty
   category moves no row or column but does change the coordinates a
   client's CellKey hangs off — layout_version must bump, the cursor need
   not. `view_shape`/`last_reshaped_views` (config) is untouched and still
   drives `layout_version`; `view_frame`/`last_reframed_views` (drawn) is
   new and drives the cursor. Unifying them broke two tests in both
   directions, which is how the distinction was found.

2. The observation must run INSIDE the effect loop, not after it. At
   end-of-batch it clobbered deliberate cursor placement in the same batch
   — `[AddItem(measure), <place cursor>]` sent the cursor home, failing
   three records-mode tests. In-loop, a later effect wins, which is the
   same ordering property gotcha 45 gives `document_config`.

3. `GridLayout::new` does NOT apply `prune_empty`; `with_frozen_records`
   does. A frame hash built on `new` cannot see a pruned row, which is
   exactly the class improvise-7kvw is in.

4. Two per-effect no-op guards were carrying the whole weight and are now
   orphaned: `effective_items` and `item_is_drawn` are deleted, along with
   six inline `moved`/`drawn`/`real`/`existed` predicates.

COST, measured as the ticket required, `benches/reshape_observation.rs`
(new), one SetCell batch, same fixtures as `grid_projection`:

    typical   400 cells   172µs -> 206µs   (+20%)
    large    9.6k cells  3.70ms -> 4.17ms  (+13%)
    18k cells            7.09ms -> 8.41ms  (+19%)
    pathological 90k    35.2ms -> 39.8ms   (+13%)

The batch is dominated by `rebuild_layout`/`recompute_formulas`, not by
the fingerprint.

WHAT CLOSES AS A CONSEQUENCE: improvise-7kvw, asserted by
`paging_across_a_prune_that_drops_a_row_invalidates_the_cursor`.
AddFormula/RemoveFormula join the shared `reshaping_commands` table (a
formula target is a `_Measure` item, so it is a column — gotcha 7), each
proved reshaping by the table's own `assert_ne!` on the grid shape.

WHAT DOES NOT CLOSE, and this is the honest part: `SortData`. A pure
record reorder changes no label and no count, and records-mode `row_items`
are positional, so a row_items fingerprint cannot see it. I wrote the
mechanism (hashing `layout.records`' coords) and then REMOVED it, because
I could not build a fixture where `SortData` actually reorders anything —
records came back in key order whatever order the cells went in. Shipping
a confident comment over a mechanism I could not demonstrate is this
repo's named defect, so it is reported instead. See the hand-back.

Negative arms unchanged and still passing, including the refused-command
arm and `SetNumberFormat`; disabling the observation fails the whole
reshaping table, which is the proof it is what does the work.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-20 20:45:34 -07:00
parent 05a28b0d45
commit 585f25bf78
5 changed files with 294 additions and 83 deletions
+5
View File
@@ -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" }
+96
View File
@@ -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<String> = (0..regions).map(|i| format!("Region{i}")).collect();
let products: Vec<String> = (0..products).map(|i| format!("Product{i}")).collect();
let measures: Vec<String> = (0..measures).map(|i| format!("Measure{i}")).collect();
for r in &regions {
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 &regions {
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<Box<dyn Effect>> = 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);
}
+90 -8
View File
@@ -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<String, u64>,
after: &std::collections::BTreeMap<String, u64>,
) -> std::collections::BTreeSet<String> {
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<String, u64> {
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<String, u64> {
@@ -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<String>,
/// 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<String>,
/// **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<String> {
&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;
}
+4 -74
View File
@@ -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<String> {
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)
+99 -1
View File
@@ -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]