diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 5d469e4e..ad73a6df 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -403,6 +403,113 @@ than `effect.apply(&mut app)`. That is deliberate: the dirty flag is raised off the `EffectKind` now (improvise-gv9r, gotcha 45), so driving the effect directly would assert a flag nothing in production sets. +### every_file_writing_effect_aborts_the_batch_on_failure + +improvise-z05r, the **third** instance of one rule: improvise-8tk fixed `Save` and +not `SaveAs`; PR #27 found `SaveAs`; `ExportCsv` was the arm still missing, so a +`Binding::Sequence` or a `:` line composing an export with anything after it +continued past a failed write. + +The fix is structural rather than a third patch: all three now report through one +`report_file_write(app, outcome)`, which writes the status message either way and +sets `abort_effects` on `Err`. That is not sufficient by itself — a fourth writer +could still be written without calling it — so the test names the three, and a +fourth has to come here and be named. Naming is the mechanism; the shared function +is what makes the naming cheap to honour. + +Each case drives `App::apply_effects` with `[writer, ChangeMode(Quit)]` against a +path under a directory that does not exist, and asserts both `last_batch_aborted()` +and that the trailing mode change never landed. + +Injected fault: drop `app.abort_effects = true` from `report_file_write`. Fails by +name, along with the four older abort tests — which is the point: they and this one +now share one mechanism. + +Injected fault, narrower: route `ExportCsv` around the reporter and write its status +directly. Only this test fails, naming `ExportCsv`, with 460 passing — the exact +state the ticket describes. + +### a_batch_that_writes_nothing_leaves_the_document_clean + +improvise-748f. `ViewNavigate { forward: false }` emits `ApplyAndClearDrill` +unconditionally, and that effect returns immediately when `drill_state` is `None`. +It declares `Model(CellData)` regardless — `Effect::kind` takes no `App`, so the +classification cannot be state-dependent — so `apply_effects` raised +`model_state.dirty` on a document nothing had touched. Press view-back, press `:q`, +and the quit is refused; autosave arms thirty seconds later. + +The older test here, `start_drill_and_apply_clear_drill_with_no_edits`, pinned the +wrong behaviour in an assertion message arguing it was the same over-approximation a +failed `LoadModel` gets. It is not: a succeeding `LoadModel` leaves a document that +really does differ from the file its path names, and that remains declared. This was +a wrong answer to no divergence at all. + +### a_cell_data_effect_no_raw_cell_records_still_dirties_the_document + +The guard against the cure improvise-748f **proposed**, which is unsound. The ticket +suggested raising `dirty` off `bump_changed_records`' per-record fingerprint of the +raw cells, on the reading that `CellData` means cells. Five of the ten `CellData` +effects change the document without writing a raw cell: `AddItem`, `AddItemInGroup` +and `SortData` (which reorders — and `.improv` carries the order), and `AddFormula` +and `RemoveFormula`, whose results live in `Model::formula_cache` and not in the +store. Observing raw cells would have left every one of them reading clean, which is +a *lost edit* — the opposite direction from the spurious prompt it was fixing. + +So the observation is `document_content` — `document_config` plus the cells in +iteration order and the formulas — and **only the `CellData` arm reads it**. The +`LayoutGeometry` arm keeps the narrow `document_config`. A first version pointed both +arms at the wide fingerprint, and review benchmarked what that costs at 18,000 cells +in release over 200 batches after warm-up: one `SetCell` batch 8.07 ms → 10.21 ms +(+27%, a constant factor on a path that was already O(cells) through +`record_fingerprints`), but one `SetNumberFormat` batch **78 µs → 2.63 ms, 33×** — +an *asymptotic* change from O(config) to O(cells + config) on the held-key path that +`[`/`]`, axis cycle, hide/show, group toggle, transpose and prune toggle all take. +~380/sec where they had been ~12,800/sec. The split costs no correctness because all +five blind effects are `CellData`; that every `LayoutGeometry` effect really is +visible to the narrow fingerprint is asserted separately, and not left as a reading. + +`WholeWorkbook` stays *declared*, and that is the remaining over-report: a `LoadModel` +whose incoming content happens to be identical still marks the document dirty, and a +failed one does too. Both cost one redundant write. + +The `SortData` case needs two cells inserted out of sorted order, or it reorders +nothing and its case asserts the opposite of what it names. The `AddFormula` case +removes the fixture's formula first, or adding it is a no-op. + +Injected fault: narrow `document_content` back to the configuration it hashed before. +Fails naming `SortData`, with five other dirty-flag tests. + +### every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint + +The guard that makes the two-fingerprint split safe rather than a trap. Splitting +`CellData` (wide) from `LayoutGeometry` (narrow) is only sound while every effect +wearing `LayoutGeometry` writes something `document_config` hashes; an effect it +cannot see would leave the document reading clean **forever**, which is the same +lost-edit failure the wide fingerprint exists to prevent, arriving by the other door. + +The thirteen are enumerated by brace-matching the `impl Effect` blocks, not by +grepping — the naive grep reports fourteen, picking up `HelpPageSet`, which is +`ViewOnly` and is simply the last `pub struct` before `#[cfg(test)]` (gotcha 38 names +this exact artefact). Each case asserts its effect really declares +`Model(LayoutGeometry)` before asserting anything else, so a reclassified effect fails +here rather than silently leaving the list unrepresentative. + +**Four** cases need a `prepare` step run **directly against the workbook** and not +through an effect, because the interesting state is the precondition, and this is +the reason the test is not vacuous: `DeleteView` needs a second view, `ShowItem` +needs a hidden item, `ToggleGroup` needs a real grouped item (it checks membership +and is otherwise a no-op that would assert nothing), and `TransposeAxes` needs two +axes that differ (transposing two identical axes is the identity). + +`dirty` is cleared **after** `prepare` and before the effect, or the preparation +itself satisfies the assertion and all four cases pass against any fingerprint. + +Injected fault: `document_config` returns `0`. Fails, with five other dirty-flag tests +across three files. Injected fault, narrower and the one that proves the test is +pointed at the right thing: drop `view.number_format` from `document_config`. Fails +naming **`SetNumberFormat`**, with only one other test failing anywhere. + + --- ## `ui/app.rs` @@ -862,6 +969,99 @@ The counters are shared document state on `App`, never on the portable session slice (gotcha 30): a client inheriting another's idea of how far the document has moved is exactly the stale basis this machinery exists to catch. +### a_page_flip_under_prune_empty_invalidates_the_cursor + +improvise-7kvw, the case improvise-9mo9 decided the other nine and deliberately left +open. `SetPageSelection` changes which records are visible and not which categories +frame them, so a grid position normally keeps naming the same cell. Under +`View::prune_empty` that is false: which rows survive pruning is a function of the +page selection, so a page flip renumbers the grid while neither `reset_cursor` nor +`cursor_epoch` moved — gotcha 24's failure in both directions at once, the host's own +cursor stale and no client told. + +9mo9 did not patch it because `[`/`]` paging is constant navigation and sending the +cursor home on every flip is worse than the hole when `prune_empty` is off, which is +the default. So the reset is conditional on `prune_empty`, and on the selection +actually moving. + +The fixture has three regions and two quarters whose non-empty rows differ, so +position 0 names `North` on Q1 and `South` on Q2. The test asserts the row labels +really differ before asserting the epoch moved; the negative arm asserts they really +*do not* differ with pruning off, so neither case can pass vacuously. + +Injected fault: drop the reset. The prune arm fails. Injected fault: make the reset +unconditional. The no-prune arm fails. + +### a_page_flip_without_prune_empty_leaves_the_cursor_alone + +The negative arm of the above, and it is load-bearing for the same reason gotcha 24 +gives for `SetNumberFormat`: without it, "reset on every page flip" passes the +positive test and costs the user their position on every `[`/`]`. + +### selecting_the_page_already_selected_invalidates_no_cursor + +The third arm, and the one the user actually hits. `SetPageSelection` resets the +cursor only when the selection **moved**, and that guard shipped untested: deleting +`moved &&` left the whole workspace green at 1416, which review caught. + +It is not an edge case. `PageNext`/`PagePrev` are odometer-style and emit a +`SetPageSelection` for **every** page category on every keypress, so with more than +one page category the non-carrying categories are always no-ops, and a single +one-item category re-selects the same item every time. Unguarded, each such keystroke +bumps `cursor_epoch`, sends the host's cursor to the origin and broadcasts +`Command::ResetCursor` to every client on that view — improvise-75ld's "a no-op that +refused everybody else", one tier down and aimed at the cursor instead of the basis. + +The test asserts the fixture really is on `Q1` and that the row labels really did not +move before asserting the epoch stood still, so it cannot pass by re-selecting a page +that was never selected or by testing a grid that does not prune. + +Injected fault: delete `moved &&`. Fails by name, alone. + +### a_foreign_layout_is_not_shown_the_hosts_staged_drill_text + +improvise-pubt, filed with an argument for unreachability that the test disproves. +`project::cell_display` fills `edit_text` from `App::display_value_in(layout, key)`, +and since improvise-jytm that layout may be another client's. +`drill_state.pending_edits` are keyed by `(row index, RecordCol)` against the +**host's** drill layout (gotcha 1), and a row index in one layout names nothing in +another. + +The ticket read the arm as unreachable because "a foreign layout built by +`GridLayout::new` carries no frozen records, so it should not produce a synthetic +key". That is wrong: records mode **auto-detects** from the axes (gotcha 1), so an +ordinary saved view with `_Index` on Row and `_Dim` on Column gives `GridLayout::new` +a records layout whose `cell_key` returns exactly the synthetic `(_Index, _Dim)` key +the drill arm matches on. The test fails on the original code, naming the host's +staged text read out of a layout it was never staged against. + +The fix is `App::staged_drill_edit`, which consults `drill_state` only when +`std::ptr::eq(layout, &self.layout)`. A pointer comparison rather than a name +comparison because `grid_in` reuses `&app.layout` by reference where the view names +agree and derives a fresh one otherwise, so identity *is* the question being asked. +Whether the host's pending drill text should reach any other client at all is +improvise-tom1 and is untouched here. + +The positive arm — the host's own layout still shows `STAGED` — is in the same test, +or "never consult the drill" passes, which would blank the TUI's own drill editor. +The drill fixture needs a real `RecordRow`, not an empty `Vec`: the host's layout +iterates the frozen snapshot, so an empty one gives a zero-row grid and the positive +arm cannot be built. + +### a_default_that_names_nothing_says_which_invariant_broke + +improvise-h074. `App::resolve_view` falls back to `workbook.active_view` for a name +the document has not got and then indexes `workbook.views[default]`. Several of these +call sites previously went through `Workbook::active_view()`, which carries +`.expect("active_view always names an existing view")`; `IndexMap`'s `Index` impl +panics with no message at all, so a workbook whose default names nothing — reachable +only by constructing the struct directly, since `create_view`/`delete_view` maintain +it — failed with a bare index panic where it used to name the broken invariant. + +Injected fault: restore `&workbook.views[default]`. The test fails on the expected +substring. + + --- ## `ui/project.rs` @@ -2180,6 +2380,21 @@ An empty prompt must not reach `SaveAs`: `SaveAs(PathBuf::from(""))` asks the backend to write to `""`, and the user then reads whatever I/O error that produces instead of "you typed nothing". +### commit_export_bails_on_an_empty_path + +improvise-z05r's first arm. `CommitExport` did not bail on an empty path where +`CommitBuffer`'s three kinds all do (gotcha 20: buffer to trim to bail-on-empty to +dirty to status is the shared spine), so Enter on an empty export prompt asked the +backend to write to `""` and the user read whatever I/O error that produced. + +The same change reorders the batch: the mode change now comes **first**, because +`ExportCsv` aborts on a failed write as of z05r's other arm, and an abort at the tail +would strand the user in `ExportPrompt` — where `draw_bottom_bar` renders the +minibuffer *instead of* the status line, so the export error is never drawn at all +and the operation appears to do nothing (gotcha 21). That ordering is asserted in +`commit_export_produces_export_and_normal_mode`, by index and not by presence. + + --- ## `command/cmd/fit.rs` @@ -2362,6 +2577,52 @@ second entry under "view-back" and removes "view-forward" from the registry entirely; `parse` takes the first match, so every surviving entry still agrees with its own name. Uniqueness is what catches it. +### the_context_reports_each_panels_own_open_flag + +improvise-y4xg and improvise-qsmp, two shapes of one defect. `registry.rs` mapped +`Panel` to a `CmdContext` field in two places, and `App::cmd_context` copies the +three flags out of `ViewState` in a third. **Both were demonstrated unguarded**: +swapping Formula and Category at the two registry sites left the workspace at 1400 +passing, and hardcoding the `category_panel_open` propagator to `true` left it at +1394. + +Nothing exercised either, because `cmd/mod.rs::test_helpers` sets the panel flags +explicitly on the context it builds — the command tests construct the value they then +assert on, so no test ever drove the real propagator. + +The registry mapping is gone: both sites call `CmdContext::panel_open(panel)`, the +one mapping on this side of the wall. That leaves two permutable sites, and the test +is aimed at both from a path neither controls — it writes `view_state`'s three fields +**by name** and reads back through the enum accessor on a context built by the real +`App::cmd_context`. PR #70's review caught the weaker shape of this: a panel test that +zeroed and read through the same four accessors passed a consistent permutation. + +Injected fault: swap `Panel::Formula` and `Panel::Category` in `panel_open`. Fails. +Injected fault: hardcode the `category_panel_open` propagator to `true`. Fails. + +### toggle_panel_visibility_reads_the_panel_it_names + +The behavioural half, and the one that reaches the registry. Its interactive arm +passes `currently_open` straight through, so with exactly one panel open the command +built for each of the three names carries its own panel's flag. Both injections above +fail here too. + +### a_focused_open_panel_closes_and_a_closed_one_opens + +improvise-qsmp's own scenario: `toggle-panel-and-focus` decides close only when the +panel is *both* open and focused, so the case that separates a right propagator from +a stuck-true one needs the mode set to the panel's own. With the flag stuck true, +pressing a panel's key on a closed panel decides to *close* it and the panel becomes +unopenable by its own key — which is what this test's second arm names. + +### Helper — `open_only` + +Not a test. It spells `formula_panel_open`, `category_panel_open` and +`view_panel_open` as fields rather than looping over `Panel::ALL` through an +accessor, and that is the whole evidentiary value: a permutation in any mapping +between the `Panel` enum and those three booleans has to disagree with this helper. + + --- ## `command/cmd/text_buffer.rs` diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index c3be37c7..803d3f13 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -195,6 +195,32 @@ mod tests { let dbg = effects_debug(&effects); assert!(dbg.contains("ExportCsv"), "Expected ExportCsv, got: {dbg}"); assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}"); + assert!( + dbg.find("Normal").unwrap() < dbg.find("ExportCsv").unwrap(), + "the mode change must precede the write an abort can reach: {dbg}" + ); + } + + /// BUG (improvise-z05r): the fourth commit kind did not bail on an empty + /// path, so Enter on an empty prompt exported to "". + #[test] + fn commit_export_bails_on_an_empty_path() { + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + for buffered in ["", " "] { + let mut bufs = HashMap::new(); + bufs.insert("export".to_string(), buffered.to_string()); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.buffers = &bufs; + let effects = CommitExport.execute(&ctx); + let dbg = effects_debug(&effects); + assert!( + !dbg.contains("ExportCsv"), + "an empty export path must write nothing, got: {dbg}" + ); + assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}"); + } } /// Naming a new measure in the trailing `+` column at the bottom-right sent the @@ -696,10 +722,18 @@ impl Cmd for CommitExport { "commit-export" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let buf = ctx.buffers.get("export").cloned().unwrap_or_default(); + let path = read_buffer(ctx, "export").trim().to_string(); + if path.is_empty() { + return vec![ + effect::set_status("No path given — not exported"), + effect::change_mode(AppMode::Normal), + ]; + } + // The mode change comes **first**, or a failed export aborts with the + // minibuffer still drawn over its error (gotcha 21). vec![ - Box::new(effect::ExportCsv(std::path::PathBuf::from(buf))), effect::change_mode(AppMode::Normal), + Box::new(effect::ExportCsv(std::path::PathBuf::from(path))), ] } } diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 56a5d897..04024746 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -81,6 +81,16 @@ impl<'a> CmdContext<'a> { .and_then(crate::view::synthetic_record_info) } + /// Whether `panel` is open. The one `Panel` → flag mapping on this side of + /// the wall, mirroring `ViewState::panel_open` (improvise-y4xg). + pub fn panel_open(&self, panel: Panel) -> bool { + match panel { + Panel::Formula => self.formula_panel_open, + Panel::Category => self.category_panel_open, + Panel::View => self.view_panel_open, + } + } + pub fn row_count(&self) -> usize { self.layout.row_count() } diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index f47b7d35..d288c0bc 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -355,11 +355,7 @@ pub fn default_registry() -> CmdRegistry { require_args("toggle-panel-and-focus", args, 1)?; let panel = parse_panel(&args[0])?; // Default interactive: if already open+focused -> close, else open+focus - let currently_open = match panel { - Panel::Formula => ctx.formula_panel_open, - Panel::Category => ctx.category_panel_open, - Panel::View => ctx.view_panel_open, - }; + let currently_open = ctx.panel_open(panel); let currently_focused = ctx.mode.panel() == Some(panel); let (open, focused) = if currently_open && currently_focused { (false, false) // close @@ -389,11 +385,7 @@ pub fn default_registry() -> CmdRegistry { |args, ctx| { require_args("toggle-panel-visibility", args, 1)?; let panel = parse_panel(&args[0])?; - let currently_open = match panel { - Panel::Formula => ctx.formula_panel_open, - Panel::Category => ctx.category_panel_open, - Panel::View => ctx.view_panel_open, - }; + let currently_open = ctx.panel_open(panel); Ok(Box::new(TogglePanelVisibility { panel, currently_open, @@ -725,3 +717,106 @@ mod tests { } } } + +#[cfg(test)] +mod panel_open_tests { + use super::*; + use crate::command::cmd::test_helpers::two_cat_model; + use crate::command::key::{KeyCode, KeyModifiers}; + use crate::ui::app::{App, AppMode}; + + /// Write the raw field each `Panel` names. Spelling the fields here is what + /// gives the read side a second path — context/test-evidence/command.md. + fn open_only(app: &mut App, panel: Panel) { + app.view_state.formula_panel_open = panel == Panel::Formula; + app.view_state.category_panel_open = panel == Panel::Category; + app.view_state.view_panel_open = panel == Panel::View; + } + + fn panel_mode(panel: Panel) -> AppMode { + match panel { + Panel::Formula => AppMode::FormulaPanel, + Panel::Category => AppMode::CategoryPanel, + Panel::View => AppMode::ViewPanel, + } + } + + #[test] + fn the_context_reports_each_panels_own_open_flag() { + for opened in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + open_only(&mut app, opened); + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + for asked in Panel::ALL { + assert_eq!( + ctx.panel_open(asked), + asked == opened, + "with only {opened:?} open, the context answers wrongly for {asked:?}" + ); + } + } + } + + #[test] + fn toggle_panel_visibility_reads_the_panel_it_names() { + for opened in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + open_only(&mut app, opened); + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + for asked in Panel::ALL { + let args = [asked.name().to_string()]; + let cmd = ctx + .registry + .interactive("toggle-panel-visibility", &args, &ctx) + .expect("toggle-panel-visibility resolves"); + let dbg = format!("{cmd:?}"); + let expected = format!("currently_open: {}", asked == opened); + assert!( + dbg.contains(&expected), + "with only {opened:?} open, toggling {asked:?} read another \ + panel's flag: {dbg}" + ); + } + } + } + + #[test] + fn a_focused_open_panel_closes_and_a_closed_one_opens() { + for panel in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + open_only(&mut app, panel); + app.view_state.mode = panel_mode(panel); + let args = [panel.name().to_string()]; + + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let dbg = format!( + "{:?}", + ctx.registry + .interactive("toggle-panel-and-focus", &args, &ctx) + .expect("toggle-panel-and-focus resolves") + ); + assert!( + dbg.contains("open: false"), + "{panel:?} is open and focused, so its key must close it: {dbg}" + ); + + let mut app = App::new(two_cat_model(), None); + app.view_state.formula_panel_open = false; + app.view_state.category_panel_open = false; + app.view_state.view_panel_open = false; + app.view_state.mode = panel_mode(panel); + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let dbg = format!( + "{:?}", + ctx.registry + .interactive("toggle-panel-and-focus", &args, &ctx) + .expect("toggle-panel-and-focus resolves") + ); + assert!( + dbg.contains("open: true"), + "{panel:?} is closed, so its own key must open it rather than \ + leave it unopenable: {dbg}" + ); + } + } +} diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index a7e16c2d..56a30005 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -80,8 +80,26 @@ fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap { .collect() } -/// Everything a `LayoutGeometry` effect can write that the file would carry. Not the -/// cell data — `record_fingerprints` answers for that — so it is cheap per batch. +/// `document_config` plus the cells and the formulas. `CellData` is wider than its +/// name (improvise-748f), and only that arm pays the walk — context/code/command.md. +fn document_content(workbook: &Workbook) -> u64 { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); + for (key, value) in workbook.model.data.iter_cells() { + key.hash(&mut h); + hash_value(value, &mut h); + } + for formula in workbook.model.formulas() { + formula.raw.hash(&mut h); + formula.target.hash(&mut h); + formula.target_category.hash(&mut h); + } + document_config(workbook).hash(&mut h); + h.finish() +} + +/// Everything a `LayoutGeometry` effect can write that the file would carry. Not +/// the cells, so it stays O(config) on the held-key path (improvise-748f). fn document_config(workbook: &Workbook) -> u64 { use std::hash::{Hash, Hasher}; let mut h = std::collections::hash_map::DefaultHasher::new(); @@ -131,6 +149,26 @@ fn sorted_pairs( out } +/// Exhaustive, and by bits rather than by `Debug`: two values that print alike +/// must not fingerprint alike. +fn hash_value(value: &CellValue, hasher: &mut impl std::hash::Hasher) { + use std::hash::Hash; + match value { + CellValue::Number(n) => { + 0u8.hash(hasher); + n.to_bits().hash(hasher); + } + CellValue::Text(t) => { + 1u8.hash(hasher); + t.hash(hasher); + } + CellValue::Error(e) => { + 2u8.hash(hasher); + e.hash(hasher); + } + } +} + /// A per-record XOR fingerprint of the **raw** cells it holds. Process-local and /// never on a wire, unlike [`RecordHash`] — see `context/code/command.md`. fn record_fingerprints(data: &DataStore) -> std::collections::HashMap { @@ -139,22 +177,7 @@ fn record_fingerprints(data: &DataStore) -> std::collections::HashMap { - 0u8.hash(&mut hasher); - n.to_bits().hash(&mut hasher); - } - CellValue::Text(t) => { - 1u8.hash(&mut hasher); - t.hash(&mut hasher); - } - CellValue::Error(e) => { - 2u8.hash(&mut hasher); - e.hash(&mut hasher); - } - } + hash_value(value, &mut hasher); *out.entry(RecordHash::of(&key)).or_insert(0) ^= hasher.finish(); } out @@ -413,7 +436,13 @@ impl App { Some((key, view)) => (key.as_str(), view), None => { let default = workbook.active_view.as_str(); - (default, &workbook.views[default]) + // The message `Workbook::active_view()` carried: `IndexMap`'s + // `Index` panics bare, naming no invariant (improvise-h074). + let view = workbook + .views + .get(default) + .expect("active_view always names an existing view"); + (default, view) } } } @@ -488,13 +517,8 @@ impl App { /// The same text against a layout that may be another client's (improvise-jytm). pub(crate) fn display_value_in(&self, layout: &GridLayout, key: &CellKey) -> String { match crate::view::synthetic_record_info(key) { - Some((idx, col)) => layout - .record_col(col) - .and_then(|rc| { - self.drill_state - .as_ref() - .and_then(|s| s.pending_edits.get(&(idx, rc.clone())).cloned()) - }) + Some((idx, col)) => self + .staged_drill_edit(layout, idx, col) .or_else(|| layout.resolve_display(key)) .unwrap_or_default(), None => self @@ -507,6 +531,18 @@ impl App { } } + /// The host's own staged drill text at a records position, and only for the + /// host's own layout — a row index names nothing in another (improvise-pubt). + fn staged_drill_edit(&self, layout: &GridLayout, idx: usize, col: usize) -> Option { + let own = std::ptr::eq(layout, &self.layout); + let col = layout.record_col(col).filter(|_| own)?; + self.drill_state + .as_ref()? + .pending_edits + .get(&(idx, col.clone())) + .cloned() + } + pub fn cmd_context(&self, key: KeyCode, _mods: KeyModifiers) -> CmdContext<'_> { let view = self.view(); let layout = &self.layout; @@ -570,12 +606,19 @@ impl App { let touches_model = effects .iter() .any(|e| matches!(e.kind(), EffectKind::Model(_))); - let records_before = effects + let writes_cells = effects .iter() - .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData))) - .then(|| record_fingerprints(&self.model_state.workbook.model.data)); + .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData))); + let moves_geometry = effects + .iter() + .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::LayoutGeometry))); + let records_before = + writes_cells.then(|| record_fingerprints(&self.model_state.workbook.model.data)); let shapes_before = touches_model.then(|| view_shapes(&self.model_state.workbook)); - let mut config_basis = touches_model.then(|| document_config(&self.model_state.workbook)); + // Two bases, because the cheap one is the held-key path: `[`/`]`, axis + // cycle and hide/show must not walk 18,000 cells (improvise-748f). + let mut content_basis = writes_cells.then(|| document_content(&self.model_state.workbook)); + let mut config_basis = moves_geometry.then(|| document_config(&self.model_state.workbook)); let mut dirty = Dirty::NOTHING; for effect in effects { let kind = effect.kind(); @@ -585,11 +628,11 @@ impl App { // rather than defaulting into the observed half by omission. let declared_dirty = match kind { EffectKind::ViewOnly => false, - // Things no structural fingerprint sees: a formula, a sort order, - // a replaced document. - EffectKind::Model(ModelChange::CellData) => true, + // A replaced document differs from the file its path names even + // where the content matches, so it is declared (improvise-748f). EffectKind::Model(ModelChange::WholeWorkbook) => true, - // Observed instead, in `observe_reshape` (improvise-75ld). + // Observed below, each against the fingerprint its tier needs. + EffectKind::Model(ModelChange::CellData) => false, EffectKind::Model(ModelChange::LayoutGeometry) => false, }; if declared_dirty { @@ -597,6 +640,13 @@ impl App { } // The observed half, read **inside the loop** so `Save` still clears it // in loop order (gotcha 45) — the position is the property. + if matches!(kind, EffectKind::Model(ModelChange::CellData)) + && let Some(basis) = content_basis + { + let now = document_content(&self.model_state.workbook); + self.model_state.dirty |= now != basis; + content_basis = Some(now); + } if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry)) && let Some(basis) = config_basis { @@ -939,6 +989,248 @@ mod tests { ); } + /// BUG (improvise-pubt): `pending_edits` are keyed by a row index into the + /// host's drill, and a position in one layout names nothing in another. + #[test] + fn a_foreign_layout_is_not_shown_the_hosts_staged_drill_text() { + use crate::model::cell::{CellKey, CellValue}; + use crate::view::{Axis, GridLayout, RecordCol}; + use std::rc::Rc; + + let mut wb = Workbook::new("Drill"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "East".into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(7.0), + ); + wb.create_view("Records"); + { + let v = &mut wb.views["Records"]; + v.set_axis("_Index", Axis::Row); + v.set_axis("_Dim", Axis::Column); + v.set_axis("Region", Axis::None); + v.set_axis("_Measure", Axis::None); + } + let mut app = App::new(wb, None); + + let foreign = GridLayout::new( + &app.model_state.workbook.model, + &app.model_state.workbook.views["Records"], + ); + assert!( + foreign.is_records_mode(), + "the other client's view is not records mode, so no synthetic key exists" + ); + let col = (0..foreign.col_count()) + .find( + |c| matches!(foreign.record_col(*c), Some(RecordCol::Measure(m)) if m == "Revenue"), + ) + .expect("the records grid has a Revenue column"); + let key = foreign + .cell_key(0, col) + .expect("the records grid names a cell at (0, Revenue)"); + assert!( + crate::view::synthetic_record_info(&key).is_some(), + "the foreign layout produced no synthetic key, so the drill arm is unreachable" + ); + + let mut pending = std::collections::HashMap::new(); + pending.insert( + (0, RecordCol::Measure("Revenue".into())), + "STAGED".to_string(), + ); + let record = crate::view::RecordRow { + coords: CellKey::new(vec![("Region".into(), "East".into())]), + values: vec![("Revenue".to_string(), CellValue::Number(7.0))], + }; + app.drill_state = Some(DrillState { + records: Rc::new(vec![record]), + pending_edits: pending, + }); + + assert_ne!( + app.display_value_in(&foreign, &key), + "STAGED", + "another client was shown the host's uncommitted drill text against \ + coordinates it was never staged for" + ); + + // The positive arm: on the host's own layout the staged text is exactly + // what the editor must open with, or "never consult the drill" passes. + app.switch_to_view("Records"); + app.rebuild_layout(); + let own_key = app + .layout + .cell_key(0, col) + .expect("the host's own records grid names a cell at (0, Revenue)"); + assert_eq!( + app.display_value_of(&own_key), + "STAGED", + "the host's own staged drill text was dropped" + ); + } + + /// Two pages whose non-empty rows differ, so pruning renumbers the grid when + /// the page flips — improvise-7kvw. `prune_empty` is the caller's choice. + fn paged_model(prune: bool) -> App { + use crate::model::cell::{CellKey, CellValue}; + use crate::view::Axis; + let mut wb = Workbook::new("Paged"); + wb.add_category("Region").unwrap(); + wb.add_category("Quarter").unwrap(); + for item in ["North", "South", "East"] { + wb.add_item("Region", item).unwrap(); + } + for item in ["Q1", "Q2"] { + wb.add_item("Quarter", item).unwrap(); + } + wb.add_item("_Measure", "Revenue").unwrap(); + for (quarter, region) in [ + ("Q1", "North"), + ("Q1", "South"), + ("Q2", "South"), + ("Q2", "East"), + ] { + wb.model.set_cell( + CellKey::new(vec![ + ("Quarter".into(), quarter.into()), + ("Region".into(), region.into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(1.0), + ); + } + { + 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 = prune; + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + app + } + + fn row_labels(app: &App) -> Vec { + (0..app.layout.row_count()) + .map(|r| app.layout.row_label(r)) + .collect() + } + + fn flip_to_q2(app: &mut App) { + app.apply_effects(vec![Box::new(super::super::effect::SetPageSelection { + category: "Quarter".to_string(), + item: "Q2".to_string(), + })]); + } + + /// BUG (improvise-7kvw): pruning depends on the page, so a page flip renumbers + /// the grid and left both the host's cursor and every client's stale. + #[test] + fn a_page_flip_under_prune_empty_invalidates_the_cursor() { + let mut app = paged_model(true); + let before_labels = row_labels(&app); + let epoch = app.cursor_epoch(); + app.cursor_mut().selected = (1, 0); + + flip_to_q2(&mut app); + + assert_ne!( + before_labels, + row_labels(&app), + "the two pages prune the same rows, so this case proves nothing" + ); + assert_ne!( + app.cursor_epoch(), + epoch, + "the grid renumbered and no cursor was invalidated" + ); + assert_eq!( + app.cursor().selected, + (0, 0), + "the host's own cursor is stale" + ); + } + + /// The third arm, and the one `[`/`]` hits constantly: odometer paging emits a + /// `SetPageSelection` per page category per keypress, most of them no-ops. + #[test] + fn selecting_the_page_already_selected_invalidates_no_cursor() { + let mut app = paged_model(true); + let before_labels = row_labels(&app); + app.cursor_mut().selected = (1, 0); + let epoch = app.cursor_epoch(); + + app.apply_effects(vec![Box::new(super::super::effect::SetPageSelection { + category: "Quarter".to_string(), + item: "Q1".to_string(), + })]); + + assert_eq!( + app.model_state + .workbook + .active_view() + .page_selection("Quarter"), + Some("Q1"), + "the fixture was not on Q1, so re-selecting it is not the no-op case" + ); + assert_eq!( + before_labels, + row_labels(&app), + "re-selecting the same page renumbered the grid, so this proves nothing" + ); + assert_eq!( + app.cursor_epoch(), + epoch, + "a page that did not move threw every client's cursor away, and \ + odometer paging emits one of these per page category per keypress" + ); + assert_eq!(app.cursor().selected, (1, 0)); + } + + /// The other arm: with pruning off the page names the same rows, so paging + /// must not throw the user's position away on an ordinary `[`/`]`. + #[test] + fn a_page_flip_without_prune_empty_leaves_the_cursor_alone() { + let mut app = paged_model(false); + let before_labels = row_labels(&app); + let epoch = app.cursor_epoch(); + app.cursor_mut().selected = (1, 0); + + flip_to_q2(&mut app); + + assert_eq!( + before_labels, + row_labels(&app), + "the page flip renumbered the grid, so this is not the no-reshape arm" + ); + assert_eq!( + app.cursor_epoch(), + epoch, + "paging with nothing pruned threw a cursor away" + ); + assert_eq!(app.cursor().selected, (1, 0)); + } + + /// improvise-h074: the fallback indexes `views` by the document's default, and + /// a bare `IndexMap` panic names no invariant where `active_view()` did. + #[test] + #[should_panic(expected = "active_view always names an existing view")] + fn a_default_that_names_nothing_says_which_invariant_broke() { + let mut app = two_col_model(); + // Only a hand-built or foreign-deserialized workbook can be here: + // `create_view`/`delete_view` both maintain the invariant. + app.model_state.workbook.active_view = "NoSuchView".to_string(); + let _ = app.resolve_view("AlsoNoSuchView"); + } + /// improvise-8uz: moving the cursor must leave the shared document untouched, /// so one client scrolling cannot move another's viewport. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 3ba27903..fdc1d804 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -440,8 +440,14 @@ pub struct SetPageSelection { } impl Effect for SetPageSelection { fn apply(&self, app: &mut App) { + let moved = app.view().page_selection(&self.category) != Some(self.item.as_str()); app.view_mut() .set_page_selection(&self.category, &self.item); + // Which rows survive pruning is a function of the page, so under + // `prune_empty` a page flip renumbers the grid (improvise-7kvw). + if moved && app.view().prune_empty { + app.reset_cursor(); + } } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -744,24 +750,29 @@ impl Effect for SetDrillPendingEdit { pub struct Save; impl Effect for Save { fn apply(&self, app: &mut App) { - match save_to_current_path(&*app.storage, &app.model_state) { - Ok(msg) => { - app.model_state.dirty = false; - app.view_state.status_msg = msg; - } - // A failed save aborts the rest of the batch, so the `Quit` that - // `wq`/`ZZ` chains never lands (improvise-8tk, gotcha 21). - Err(msg) => { - app.view_state.status_msg = msg; - app.abort_effects = true; - } + let outcome = save_to_current_path(&*app.storage, &app.model_state); + if outcome.is_ok() { + app.model_state.dirty = false; } + report_file_write(app, outcome); } fn kind(&self) -> EffectKind { EffectKind::ViewOnly } } +/// Every effect that writes a file reports through here: the status message either +/// way, and a failure aborts the rest of the batch (improvise-z05r, gotcha 21). +fn report_file_write(app: &mut App, outcome: Result) { + match outcome { + Ok(msg) => app.view_state.status_msg = msg, + Err(msg) => { + app.view_state.status_msg = msg; + app.abort_effects = true; + } + } +} + /// Write the workbook to its current file path, returning the status /// message either way. Missing path and I/O failure are both errors. fn save_to_current_path( @@ -782,19 +793,15 @@ fn save_to_current_path( pub struct SaveAs(pub PathBuf); impl Effect for SaveAs { fn apply(&self, app: &mut App) { - match crate::persistence::save_to(&*app.storage, &app.model_state.workbook, &self.0) { - Ok(()) => { - app.model_state.file_path = Some(self.0.clone()); - app.model_state.dirty = false; - app.view_state.status_msg = format!("Saved to {}", self.0.display()); - } - // The same abort `Save` owes: `SaveAs` is the arm improvise-8tk - // left out, so `:w /bad/path . q` quit on a failed write. - Err(e) => { - app.view_state.status_msg = format!("Save error: {e}"); - app.abort_effects = true; - } + let outcome = + crate::persistence::save_to(&*app.storage, &app.model_state.workbook, &self.0) + .map(|()| format!("Saved to {}", self.0.display())) + .map_err(|e| format!("Save error: {e}")); + if outcome.is_ok() { + app.model_state.file_path = Some(self.0.clone()); + app.model_state.dirty = false; } + report_file_write(app, outcome); } fn kind(&self) -> EffectKind { EffectKind::ViewOnly @@ -930,19 +937,15 @@ pub struct ExportCsv(pub PathBuf); impl Effect for ExportCsv { fn apply(&self, app: &mut App) { let view_name = app.view_name().to_string(); - match crate::persistence::export_csv_to( + let outcome = crate::persistence::export_csv_to( &*app.storage, &app.model_state.workbook, &view_name, &self.0, - ) { - Ok(()) => { - app.view_state.status_msg = format!("Exported to {}", self.0.display()); - } - Err(e) => { - app.view_state.status_msg = format!("Export error: {e}"); - } - } + ) + .map(|()| format!("Exported to {}", self.0.display())) + .map_err(|e| format!("Export error: {e}")); + report_file_write(app, outcome); } fn kind(&self) -> EffectKind { EffectKind::ViewOnly @@ -2004,6 +2007,33 @@ mod tests { ); } + /// BUG (improvise-z05r): the third arm of a rule with three. Named, so a + /// fourth file-writing effect has to come here — see the evidence. + #[test] + fn every_file_writing_effect_aborts_the_batch_on_failure() { + let bad = PathBuf::from("/nonexistent-dir-improvise-z05r/out"); + type Writer = fn(&PathBuf) -> Box; + let writers: [(&str, Writer); 3] = [ + ("Save", |_| Box::new(Save)), + ("SaveAs", |p| Box::new(SaveAs(p.clone()))), + ("ExportCsv", |p| Box::new(ExportCsv(p.clone()))), + ]; + for (label, make) in writers { + let mut app = test_app(); + app.model_state.file_path = Some(bad.clone()); + app.apply_effects(vec![make(&bad), Box::new(ChangeMode(AppMode::Quit))]); + assert!( + app.last_batch_aborted(), + "{label} failed to write and did not stop the batch" + ); + assert_ne!( + app.view_state.mode, + AppMode::Quit, + "{label} failed to write and the trailing effect applied anyway" + ); + } + } + /// The positive arm: a `SaveAs` that succeeds must not abort. Without it, /// "always abort" passes the test above. #[test] @@ -2327,19 +2357,198 @@ mod tests { app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); assert!(app.drill_state.is_none()); assert!( - app.model_state.dirty, - "a drill exited with no edits still marks the document dirty, \ - because `apply_effects` raises the flag off the declared \ - `EffectKind` and this effect declares `Model(CellData)` whether \ - or not it wrote anything (improvise-gv9r). That is the same \ - over-approximation a failed `LoadModel` gets, in the same \ - direction: one redundant write rather than a lost edit. It has \ - been true since the flag moved; this test could not see it \ - because it drove `apply` directly and so measured the effect in \ - isolation rather than the path production takes." + !app.model_state.dirty, + "a drill exited with no edits wrote nothing, so the document is \ + unchanged and `:q` must not be refused (improvise-748f)" ); } + /// improvise-748f: `ApplyAndClearDrill` on a document with no drill is what + /// view-back emits unconditionally, and it writes nothing at all. + #[test] + fn a_batch_that_writes_nothing_leaves_the_document_clean() { + let mut app = test_app(); + assert!(app.drill_state.is_none()); + app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); + assert!( + !app.model_state.dirty, + "view-back on a document with no drill open refused the next `:q`" + ); + } + + /// `CellData` is wider than its name, and a raw-cell fingerprint sees none of + /// these — the cheap cure improvise-748f proposed would have lost them. + #[test] + fn a_cell_data_effect_no_raw_cell_records_still_dirties_the_document() { + type Writer = fn() -> Box; + let writers: [(&str, Writer); 5] = [ + ("AddItem", || { + Box::new(AddItem { + category: "Type".into(), + item: "Toys".into(), + }) + }), + ("AddItemInGroup", || { + Box::new(AddItemInGroup { + category: "Type".into(), + item: "Toys".into(), + group: "Leisure".into(), + }) + }), + ("SortData", || Box::new(SortData)), + ("AddFormula", || { + Box::new(AddFormula { + raw: "Profit = Revenue - Cost".into(), + target_category: "_Measure".into(), + }) + }), + ("RemoveFormula", || { + Box::new(RemoveFormula { + target: "Profit".into(), + target_category: "_Measure".into(), + }) + }), + ]; + for (label, make) in writers { + let mut app = test_app(); + app.apply_effects(vec![Box::new(AddFormula { + raw: "Profit = Revenue - Cost".into(), + target_category: "_Measure".into(), + })]); + // Inserted out of sorted order, or `SortData` reorders nothing and + // its case proves the opposite of what it names. + for (n, (ty, month)) in [("Food", "Jan"), ("Clothing", "Feb")].iter().enumerate() { + app.model_state.workbook.model.set_cell( + CellKey::new(vec![ + ("Type".into(), (*ty).into()), + ("Month".into(), (*month).into()), + ]), + CellValue::Number(n as f64), + ); + } + if label == "AddFormula" { + app.apply_effects(vec![Box::new(RemoveFormula { + target: "Profit".into(), + target_category: "_Measure".into(), + })]); + } + app.model_state.dirty = false; + + app.apply_effects(vec![make()]); + assert!( + app.model_state.dirty, + "{label} changed the document and left it reading clean, so the \ + edit is lost on quit and the worker never writes it back" + ); + } + } + + /// improvise-748f: the `LayoutGeometry` arm is observed against the **narrow** + /// `document_config`, so an effect it cannot see would read clean forever. + #[test] + fn every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint() { + type Prepare = fn(&mut App); + type Make = fn() -> Box; + let nothing: Prepare = |_| {}; + let cases: [(&str, Prepare, Make); 13] = [ + ("AddCategory", nothing, || { + Box::new(AddCategory("Region".into())) + }), + ("RemoveCategory", nothing, || { + Box::new(RemoveCategory("Month".into())) + }), + ("CreateView", nothing, || { + Box::new(CreateView("Second".into())) + }), + ( + "DeleteView", + |app| { + app.model_state.workbook.create_view("Second"); + }, + || Box::new(DeleteView("Second".into())), + ), + ("SetAxis", nothing, || { + Box::new(SetAxis { + category: "Type".into(), + axis: Axis::Column, + }) + }), + ("SetPageSelection", nothing, || { + Box::new(SetPageSelection { + category: "Type".into(), + item: "Food".into(), + }) + }), + ( + "ToggleGroup", + |app| { + app.model_state + .workbook + .add_item_in_group("Type", "Cake", "Baked") + .unwrap(); + }, + || { + Box::new(ToggleGroup { + category: "Type".into(), + group: "Baked".into(), + }) + }, + ), + ("HideItem", nothing, || { + Box::new(HideItem { + category: "Type".into(), + item: "Food".into(), + }) + }), + ( + "ShowItem", + |app| { + app.view_mut().hide_item("Type", "Food"); + }, + || { + Box::new(ShowItem { + category: "Type".into(), + item: "Food".into(), + }) + }, + ), + ( + "TransposeAxes", + |app| { + app.view_mut().set_axis("Type", Axis::Row); + app.view_mut().set_axis("Month", Axis::Column); + }, + || Box::new(TransposeAxes), + ), + ("CycleAxis", nothing, || Box::new(CycleAxis("Type".into()))), + ("SetNumberFormat", nothing, || { + Box::new(SetNumberFormat(",.4f".into())) + }), + ("TogglePruneEmpty", nothing, || Box::new(TogglePruneEmpty)), + ]; + + for (label, prepare, make) in cases { + let mut app = test_app(); + prepare(&mut app); + app.model_state.dirty = false; + + let effect = make(); + assert!( + matches!( + effect.kind(), + EffectKind::Model(ModelChange::LayoutGeometry) + ), + "{label} is not a LayoutGeometry effect, so this case belongs elsewhere" + ); + app.apply_effects(vec![effect]); + assert!( + app.model_state.dirty, + "{label} changed the document and `document_config` could not see it, \ + so the document reads clean forever" + ); + } + } + /// A staged edit to a measure column writes that one cell (improvise-c5v). #[test] fn apply_and_clear_drill_with_measure_edit() {