Merge pull request #76 from fiddlerwoaroof/wave/command-effect-gaps
command: seven edges — dirtiness, aborts, the cursor and three unguarded mappings
This commit is contained in:
Ed Langley
GitHub
@@ -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`
|
||||
|
||||
@@ -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<Box<dyn Effect>> {
|
||||
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))),
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -80,8 +80,26 @@ fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap<String, u64> {
|
||||
.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<RecordHash, u64> {
|
||||
@@ -139,22 +177,7 @@ fn record_fingerprints(data: &DataStore) -> std::collections::HashMap<RecordHash
|
||||
for (key, value) in data.iter_cells() {
|
||||
let mut hasher = std::collections::hash_map::DefaultHasher::new();
|
||||
key.hash(&mut hasher);
|
||||
// Exhaustive, and by bits rather than by `Debug`: two values that print
|
||||
// alike must not fingerprint alike.
|
||||
match value {
|
||||
CellValue::Number(n) => {
|
||||
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<String> {
|
||||
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<String> {
|
||||
(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]
|
||||
|
||||
@@ -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<String, String>) {
|
||||
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<dyn Effect>;
|
||||
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<dyn Effect>;
|
||||
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<dyn Effect>;
|
||||
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() {
|
||||
|
||||
Reference in New Issue
Block a user