fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #70 from fiddlerwoaroof/wave/effect-classification

Browse Source 
fix(command): a counter is bumped on an observed change, not a declared intent
This commit is contained in:
Ed Langley
2026-09-20 16:59:18 -07:00
committed by GitHub
parent 40fc0c7dc7 14757a753f
commit 4e48a6ecb4
13 changed files with 897 additions and 143 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/clients.md
+2 -2
View File
@@ -538,8 +538,8 @@ gotcha 47 asks for — closing e24v has to come to that line and delete it.
off the effect's own `EffectKind`, so there is nothing to mirror and nothing
missing. The cursor reset is not this client's to send either: the effect calls
`App::reset_cursor`, and `adopt_cursor_epoch` broadcasts `Command::ResetCursor`
to *everyone* in answer — so sending one here would be a second,
differently-addressed copy.
to every client on a view the batch reshaped (improvise-nynk) — so sending one
here would be a second, differently-addressed copy.
- **`toggle-panel-and-focus`.** The interactive arm of `TogglePanelAndFocus`:
open-and-focus unless this panel is already both, in which case close and
unfocus. The decision is made in `registry.rs` on the host and here on the
context/code/command.md
+98 -29
View File
@@ -490,8 +490,9 @@ direction the code moves in.
**The type-level documentation exists and is checked.** `Effect::kind()` has no
default, so the compiler demands the classification (gotcha 3), and
`apply_effects` reads that one answer three times — `Dirty`, `layout_version`
and `model_state.dirty` (gotchas 40, 45). A narrowed signature would be a
`apply_effects` reads that one answer for the `Dirty`, and for the `CellData` and
`WholeWorkbook` half of `model_state.dirty` (gotchas 40, 45; the shape tiers are
observed rather than declared — see `apply_effects` below). A narrowed signature would be a
*second* statement of what an effect touches, free to disagree with the first.
Two answers to one question is the defect this repo names, not the cure.
@@ -850,13 +851,13 @@ to reach the session list (improvise-cqq). Not part of `ViewState`: it describes
the document's identity over this process's lifetime, not a client's view.
`layout_version` — shared document state: how many times the grid's *shape* has
moved, bumped once per `ModelChange::LayoutGeometry` effect that actually runs
(ADR 001, improvise-xwi6). The middle tier of the basis: a `CellData` batch
moved, bumped once per batch that is **observed** to move it, never off a
declaration (ADR 001, improvise-xwi6; improvise-pdvm). The middle tier of the basis: a `CellData` batch
composed before this moved is stale even if no record it names was touched,
because the keys it carries hang off a shape that has changed under them.
`cursor_epoch` — shared document state: how many times a shape change has declared
every cursor into this document invalid. Bumped by `reset_cursor` and nothing else
a cursor into this document invalid. Bumped by `reset_cursor` and nothing else
(improvise-gvnb). **It is a counter rather than a list of the commands that reset
the cursor**, for the reason gotcha 39 gives for watching `document_generation`
instead of matching on `LoadModel`: *which* effects renumber the grid is a
@@ -897,6 +898,28 @@ way, and the effect that got here cannot reach the session list. Bumping
`document_generation` is how the reset reaches them: `ClientSession::project`
compares it and reseeds.
### `adopt_cursor_epoch` — the epoch is the trigger, the view is the audience
`reset_cursor` resets the host's own slice; every connected client owes the same
invalidation, and `cursor_epoch` is how the reset reaches them. **The trigger stays
the epoch moving** — which effects renumber the grid is a per-effect decision, so a
table of variants here would be a second description of it (gotcha 24).
**The audience is not everybody**, and has not been since improvise-jytm made
"which view a client is on" per-client: a reshape of view A threw away the cursor of
a client sitting on view B, which nothing reshaped. `cursor_invalidated` asks
`App::last_reshaped_views` — the observed set, so `AddCategory`, which seats itself
on an axis in *every* view, still reaches everybody — and the relay is one
`Broadcast::To(id, ResetCursor)` per affected client rather than one `ToAll`.
An **empty** reshaped set means "not known", never "nobody": a batch that moved the
cursor with no view observed to change shape gets the safe answer, everybody.
Telling too few is strictly worse than telling too many — the diff then believes
each client already holds origin-positioned content and sends nothing, so they stay
stale permanently rather than until their next scroll. That is the half-fix
gotcha 24 warns about, and `cursor_invalidated` is a named function with both arms
tested so the fail-safe cannot be tidied away.
### `record_version` / `resolve_record` / `resolve_aggregate`
`record_version` takes a cell key or the record's own key indifferently —
@@ -973,37 +996,83 @@ with connected clients knows what to project (improvise-cqq). **The `Dirty`
accumulates inside the loop: an aborted batch must not claim the effects it never
ran** (gotcha 21).
The two lower basis tiers are read off the same `EffectKind` the `Dirty` is
(ADR 001, improvise-xwi6) — one classification with three readers in one place,
never a second table to drift from gotcha 35's.
**`layout_version` and `model_state.dirty` are no longer read off the
`EffectKind`** (improvise-75ld, improvise-pdvm). A kind says what an effect *means*
to do, and both counters were believing it. Two failures, in opposite directions,
from the one assumption:
**`layout_version`** is per effect and inside the loop, for the same reason the
`Dirty` is.
- **`AddItem`/`AddItemInGroup`/`RemoveItem` declare `CellData` and reshape the
grid.** Since improvise-9mo9 they call `reset_cursor`, so `cursor_epoch` moved
while `layout_version` did not — one event moving one of two counters that
answer the same question, and a client told its cursor is invalid while its
layout tier still reads current.
- **A refused or duplicate `AddCategory` declares `LayoutGeometry` and changes
nothing.** `Model::add_category` returns `Ok` for a name it already has, so the
reachable case does not even abort. Every other client's next `Command::Model`
was refused with `Rejection::LayoutMoved`, and the browser worker rewrote the
whole OPFS document, for a keystroke that did nothing.
**`model_state.dirty`** is read there too, and for the reason the other two are: it
is a fact about the document, so the classification the effect already carries is
the only honest source for it. It used to be a separate `MarkDirty` effect that
each *command* composed alongside its mutation — reasonable on the key path and
unreachable from anywhere else, because improvise-kmgs derived `ModelCommand` from
the effects one at a time and so put `SetCell` on the wire without its companion
(improvise-gv9r). A derivation correct per element and wrong for a set. Raising it
here instead makes it unrepresentable rather than validated: there is no way to
compose a document change that omits it.
So **a counter is bumped on an observed change, not on a declared intent** — the
answer `record_versions` already had, and the reason `cursor_epoch` is a counter
rather than a table of variants. Two fingerprints are taken around the batch, both
gated on the batch containing *any* `EffectKind::Model(_)`:
All three `ModelChange` levels count, `WholeWorkbook` included. A replaced
document is not the document `file_path` names, whether it arrived from the import
wizard or from a `LoadModel` — which does not move `file_path` — so in both cases
the bytes on disk and the bytes in memory have parted company, which is all
`dirty` claims.
`view_shape` is one view's **drawn** shape: the axis map, the effective item names
minus this view's hidden ones, the collapsed groups that have something drawn under
them, the page selection, `prune_empty`. Drawn, not configured — `hide_item`
records a name whether or not the model has one, and a hidden ghost moves no row.
`view_shapes` is that per view name, and it answers two questions: did anything
move (`layout_version`), and *which views* moved (`Sessions::adopt_cursor_epoch`,
improvise-nynk). `layout_version` is therefore bumped **once per batch** rather
than once per effect; nothing reads it but "did it move".
`document_config` is everything a `LayoutGeometry` effect can write that the file
would carry: the categories with their items and groups, and every view's
configuration whether it draws or not. It is what raises `model_state.dirty`, and
it is a *different* hash from `view_shape` on purpose — the number format and a
hidden ghost belong to the bytes on disk and not to the shape. The cell data is
absent from both, because `record_fingerprints` answers for it and a whole-document
hash per keystroke is not affordable (gotcha 41: `Storage` is whole-document).
`ModelChange::CellData` and `ModelChange::WholeWorkbook` still raise `dirty` off
the declaration, because they write things no structural fingerprint can see: a
formula, a sort order, a replaced document.
**Both halves are read inside the loop, and the position is the property.** The
observed half compares `document_config` after each `LayoutGeometry` effect against
the value it held after the previous one, rather than once around the whole batch.
An after-the-loop comparison is *correct* about whether the document changed and
still wrong, because it re-raises a flag `Save` cleared **earlier in the same
batch**: `[SetNumberFormat, Save]` — reachable from `:format ,.2f . w` and from any
`Binding::Sequence` pairing a geometry effect with a save — then reported a
successful save and a dirty document, so the TUI drew "Saved to …" while `:q`
refused, and the worker paid a whole-document OPFS rewrite. That is the failure
class this whole section exists to close, reintroduced one level up. Guarded by
`a_batch_that_changes_the_layout_and_then_saves_ends_clean`, over both arms. It still
over-approximates there — a `LoadModel` that cannot read its path leaves the flag
raised, costing one redundant write, where the other direction costs an edit. It
used to be a separate `MarkDirty` effect that each *command* composed alongside its
mutation, which improvise-kmgs's one-at-a-time derivation put `SetCell` on the wire
without (improvise-gv9r): a derivation correct per element and wrong for a set.
`Save` is `ViewOnly` and clears the flag itself, so a batch that edits and then
saves ends clean: the order in the loop is the order the effects were composed in.
That is why the observation is in the loop and not after it.
It over-approximates in the same direction and for the same reason
`layout_version` does: an effect that *declares* a document change and turns out to
do nothing — a `LoadModel` that cannot read its path, an `AcceptWizardModel` with
nothing built — leaves the flag raised. That costs one redundant write. The other
direction costs an edit.
`layout_version` **is** bumped after the loop, and that is not the same hazard:
nothing clears it, and an aborted batch's effects never ran, so a comparison taken
at the end sees only what did run. Neither arm of that is tested today; filed with
the orchestrator rather than cited here, because an id that was never read back is
worse than no id.
**What the split does not reach**, and is not a regression because the declared
scheme did not reach it either: `prune_empty` makes a view's shape depend on the
data, so a cell edit that empties a row prunes it without moving `view_shape`. The
record tier covers the write; the shape tier does not see the reshape.
Both fingerprints sort every `HashMap`/`HashSet` they touch. That is not tidiness:
an unsorted iteration order is per-process (gotcha 18), and a fingerprint that
re-randomises every run would bump both counters on every batch.
**`record_versions` cannot be read off the kind**, because an `EffectKind` says
*that* a batch writes cells, never *which* — so the records are read off the data
context/code/protocol.md
+1 -1
View File
@@ -926,7 +926,7 @@ reshaped grid (gotcha 24) and the session mirror after a document swap (gotcha
| tier | moved by | a batch is stale if |
|---|---|---|
| `document_generation` | a `WholeWorkbook` effect | it moved at all |
| `layout_version` | a `LayoutGeometry` effect | it moved at all |
| `layout_version` | an observed change to any view's drawn shape | it moved at all |
| `records` | a `CellData` effect on that record | any record the batch names moved |
A cell edit is stale when the **layout** moved, even though no record it names
context/test-evidence/clients.md
+40 -7
View File
@@ -1576,21 +1576,54 @@ is asserted. Injecting the no-op now fails this test.
Note the harness cannot see this one, and that is correct rather than a gap: the
browser folds `Command::SetPanelOpen` through `reduce_view`, a *different*
three-way match in a different crate from `effect::SetPanelOpen`'s. Two
derivations of one mapping — which is why `panel_commands_address_the_named_panel_only`
needed the same repair independently.
derivations of one mapping — which is why the `reduce_view` guard needed the same
repair independently. **Since improvise-56ek there is one derivation**, so that is
history rather than a standing hazard; see below.
### panel_commands_address_the_named_panel_only
### a_panel_command_writes_that_panel_and_no_other
The `reduce_view` half of the one above, and **the first repair of it was still
vacuous** — worth recording because the mistake is subtle. Folding
`[open: false, open: true]` and asserting the final state is *true* passes
against a reducer that folds nothing at all: the default is already true, so
neither command needs to land. Only asserting the **`false`** direction
discriminates, because that is the one the default cannot supply.
discriminates, because that is the one the default cannot supply. Caught by
injection rather than by reading — the no-op fold reported NONE failing after the
first fix.
Caught by injection rather than by reading — the no-op fold reported NONE
failing after the first fix. Now a separate `closed` fold asserts the flag
moved, before the original assertions run.
improvise-56ek then removed the second derivation entirely: `ViewState::panel_open_mut`
and `panel_cursor_mut` (plus their read halves) are the one place a `Panel` becomes
a field, and both `effect::SetPanelOpen::apply` and `reduce_view` call them. The
cure is one home, not a test holding two copies in step
(`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)`), so
there is deliberately no "the two agree" test — a wrong field in the one home is a
failure in every caller.
This test replaces `panel_commands_address_the_named_panel_only`, which drove
`Panel::Category` only and so could not tell one panel from another. It keeps that
test's lesson by zeroing every panel before opening one.
Injected fault: point `Panel::Category`'s arm at `formula_panel_open`. Fails by
name for every panel in `Panel::ALL`.
**That injection alone was not enough, and the gap is gotcha 3's shape.** The first
version asserted only through `panel_open`/`panel_cursor` — the read halves of the
mapping it was testing. A *consistent* permutation of all four accessors is a
self-consistent bijection: injecting Formula↔Category into `panel_open_mut`,
`panel_open`, `panel_cursor_mut` and `panel_cursor` together left the test passing,
while two unrelated tests that read raw fields failed. It caught only a
**non-injective** mapping, which is exactly the injection first recorded here. The
test now reads the three field pairs by name once per panel, and the consistent
permutation fails it: "Formula is not the field Formula names".
Also corrected here: the claim that consolidation left "no second copy to agree
with" was false when written. `resolve.rs` carried a fourth copy of the same
`Panel` → `ViewState` three-way match, **in the same crate on the same type**; it
is deleted and its three call sites go through `ViewState::panel_open`. What
remains is *not* the same mapping: `registry.rs` maps `Panel` to `CmdContext`
fields, a different type whose values are copied from `view_state`, and
`resolve.rs`/`registry.rs` each map `Panel` to a (cursor, collection length) pair,
which is a second derivation of something else and is `lockstep.rs`'s subject.
### a_fresh_session_opens_with_the_category_panel
context/test-evidence/command.md
+121
View File
@@ -2644,3 +2644,124 @@ entirely. Found in re-review. It now records **every** marker the client is
told and asserts `[true, false]` for the edit, with the bootstrap's own clean
marker cleared first so the sequence is about the edit rather than the
handshake. The stronger form fails under both injections.
### a_refused_or_duplicate_add_category_moves_no_counter
improvise-75ld. `App::apply_effects` bumped `layout_version` and raised
`model_state.dirty` off the `EffectKind` **before** the `abort_effects` check, so
the refusing effect counted itself. Two arms, and the second is the reachable one:
a 13th category is refused (`MAX_CATEGORIES`) and aborts, while a *duplicate*
`add-cat Region` returns `Ok` and does not abort at all — a pure no-op that still
refused every other client's next `Command::Model` with `Rejection::LayoutMoved`
and made the browser worker rewrite the whole OPFS document (gotcha 41).
Injected fault: put the two bumps back on the declared kind, inside the loop
(`if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry))`). Both no-op
arms fail; so do `neither_shape_counter_moves_for_an_accepted_no_op`,
`both_shape_counters_move_for_every_reshaping_command` and
`a_configuration_change_that_draws_nothing_is_still_a_document_change`, with 450
other tests still passing — the build ran.
The positive arm is not decoration: without it, "never bump anything" passes the
two no-op arms. It also fails under a second injection — dropping the
`document_config` comparison — because `AddCategory` is `LayoutGeometry` and the
declared half of `dirty` no longer covers it.
### both_shape_counters_move_for_every_reshaping_command
improvise-pdvm. `AddItem`, `AddItemInGroup` and `RemoveItem` declare
`ModelChange::CellData` and reshape the grid. Since improvise-9mo9 they call
`App::reset_cursor`, so `cursor_epoch` moved while `layout_version` did not: a
client was told its cursor was invalid while its layout basis tier still read
current, and a batch composed against the old shape was still accepted.
It drives `reshaping_commands()` — the same table
`every_reshaping_model_command_invalidates_every_cursor` drives, extracted into a
shared function precisely so the two cannot answer for different cases — and
asserts the grid shape really moved before asserting either counter, so a case
that reshapes nothing fails loudly rather than proving nothing.
Injected fault: as above. Fails naming `SetAxis`, then `AddItem` once the earlier
cases are removed.
### a_layout_change_moves_the_layout_version_and_no_record
Pre-existing test, fixture repaired under improvise-75ld. It asked for
`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where
`on_category_added` had already seated `Month` on Column — so the "layout change"
was a no-op, and the test passed only because the tier was bumped off the
declaration. It now asserts the axis before and after, and asks for `Axis::Row`.
This is the failure that turned "reclassify three effects" into "stop reading the
tier off the classification".
### a_configuration_change_that_draws_nothing_is_still_a_document_change
The third case the shape/config split creates, and the one that forced two
fingerprints rather than one. `hide_item` records a name whether or not the model
has one, and `SetNumberFormat` writes the file while moving no row or column: both
are `dirty` and neither is `layout_version`. A single fingerprint covering the
persisted configuration made a ghost hide bump the basis; one covering only the
drawn shape let a format change go unsaved.
Injected fault: remove the `document_config` comparison in `observe_reshape`.
Fails naming `HideItem { category: "Region", item: "NoSuchItem" }`.
### a_reshape_of_one_view_leaves_a_client_on_another_view_alone
improvise-nynk. improvise-jytm made "which view a client is on" per-client;
`Sessions::adopt_cursor_epoch` was not updated and still reset **every** mirror
and returned one `Broadcast::ToAll(Command::ResetCursor)`. A reshape of view
`Default` therefore threw away the cursor of a client sitting on `Second`, which
nothing reshaped.
The fixture is two views of one document with different axes, and the bystander
switches to `Second` and scrolls there before the asker reshapes `Default`. The
positive arm is kept in the same test — the asker is on the reshaped view, is
reset, and is told — because without it "never invalidate anyone" passes.
Injected fault: drop the `cursor_invalidated` filter (`.filter(|_| true)`). Fails
naming the bystander's thrown-away cursor, with 453 other tests passing.
### an_unknown_reshape_set_invalidates_everybody
The fail-safe arm of the same narrowing, tested separately because it is the half
gotcha 24 calls strictly worse than doing nothing. An empty reshaped set means the
batch moved a cursor with no view observed to change shape; the answer must be
"everybody", not "nobody", or the diff believes each client already holds
origin-positioned content and sends nothing, leaving them stale permanently.
Injected fault: delete the `reshaped.is_empty() ||` disjunct. Fails by name. That
injection is why the predicate is a named function rather than a closure — a
fail-safe nothing can point at is a fail-safe that gets tidied away.
### a_batch_that_changes_the_layout_and_then_saves_ends_clean
A regression introduced by the first version of the observed-`dirty` change and
caught in review of PR #70, not by the suite — the workspace was green at 1400 with
this broken. `observe_reshape` ran **after** the effect loop and re-raised
`model_state.dirty` from the `document_config` comparison, while `Save` had cleared
it **inside** the loop. So gotcha 45's stated ordering property — "`Save` clears it
in loop order, so a batch that edits and then saves ends clean" — survived for
`CellData`/`WholeWorkbook` and was broken for `LayoutGeometry`.
`[SetNumberFormat, Save]` reported a successful save and left the document dirty:
the TUI draws "Saved to …" while `Quit`'s check refuses a later `:q`, and the
browser worker — whose persist trigger *is* the flag — pays a whole-document OPFS
rewrite. Reachable from `:format ,.2f . w` and from any `Binding::Sequence` pairing
a geometry effect with a save.
The fix is position, not logic: the comparison moved inside the loop, taken after
each `LayoutGeometry` effect against the value held after the previous one. Both
arms are in the test — `[SetCell, Save]` never left the loop, so the geometry arm
alone could not say whether the rule is general — and each asserts the save really
happened (`last_batch_aborted`, and a status line starting "Saved") before
asserting the flag, or a failed save would pass the dirty assertion for the wrong
reason.
Injected fault: move the comparison back after the loop. Fails naming the layout
arm, with 454 others passing.
The lesson worth carrying: **an observation that is correct about *what* changed
can still be wrong about *when* it is read.** Everything above this entry argues
that observing beats declaring; this is the cost of that move, and it lands
precisely on the one effect that *unsets* what the observation sets.
crates/improvise-command/src/ui/app.rs
+284 -10
View File
@@ -33,6 +33,104 @@ use improvise_protocol::RecordCell;
/// ADR 001 settles on, and not the cell — see `context/code/command.md`.
pub use improvise_core::model::record::record_of;
/// One view's **drawn** shape, as a hash. Drawn, not configured — the other half is
/// [`document_config`]; see `context/code/command.md` (improvise-pdvm, improvise-75ld).
fn view_shape(model: &crate::model::types::Model, view: &View) -> u64 {
use std::hash::{Hash, Hasher};
let mut h = std::collections::hash_map::DefaultHasher::new();
for (cat, axis) in &view.category_axes {
cat.hash(&mut h);
axis.hash(&mut h);
let hidden = view.hidden_items.get(cat);
let drawn: Vec<String> = model
.effective_item_names(cat)
.into_iter()
.filter(|item| !hidden.is_some_and(|h| h.contains(item)))
.collect();
drawn.hash(&mut h);
// Off the drawn items, not `Category::groups`: `add_item_in_group` does not
// populate that, and a group with nothing drawn under it is not shape.
let mut collapsed: Vec<&str> = model
.category(cat)
.into_iter()
.flat_map(|c| drawn.iter().filter_map(|item| c.items.get(item)))
.filter_map(|item| item.group.as_deref())
.filter(|group| {
view.collapsed_groups
.get(cat)
.is_some_and(|c| c.contains(*group))
})
.collect();
collapsed.sort_unstable();
collapsed.dedup();
collapsed.hash(&mut h);
view.page_selections.get(cat).hash(&mut h);
}
view.prune_empty.hash(&mut h);
h.finish()
}
/// Every view's drawn shape, keyed by view name. The map answers two questions:
/// did *anything* move, and *which views* moved (improvise-nynk).
fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap<String, u64> {
workbook
.views
.iter()
.map(|(name, view)| (name.clone(), view_shape(&workbook.model, view)))
.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.
fn document_config(workbook: &Workbook) -> u64 {
use std::hash::{Hash, Hasher};
let mut h = std::collections::hash_map::DefaultHasher::new();
for (name, cat) in &workbook.model.categories {
name.hash(&mut h);
cat.kind.hash(&mut h);
for (item_name, item) in &cat.items {
item_name.hash(&mut h);
item.group.hash(&mut h);
}
for group in &cat.groups {
group.name.hash(&mut h);
group.parent.hash(&mut h);
}
}
for (name, view) in &workbook.views {
name.hash(&mut h);
// `IndexMap` is ordered; everything below it is a `HashMap`/`HashSet`, so
// sorting is what separates a fingerprint from a coin toss (gotcha 18).
for (cat, axis) in &view.category_axes {
cat.hash(&mut h);
axis.hash(&mut h);
}
sorted_pairs(&view.hidden_items).hash(&mut h);
sorted_pairs(&view.collapsed_groups).hash(&mut h);
let mut pages: Vec<(&String, &String)> = view.page_selections.iter().collect();
pages.sort_unstable();
pages.hash(&mut h);
view.number_format.hash(&mut h);
view.prune_empty.hash(&mut h);
}
h.finish()
}
fn sorted_pairs(
map: &std::collections::HashMap<String, std::collections::HashSet<String>>,
) -> Vec<(&String, Vec<&String>)> {
let mut out: Vec<(&String, Vec<&String>)> = map
.iter()
.map(|(k, v)| {
let mut items: Vec<&String> = v.iter().collect();
items.sort_unstable();
(k, items)
})
.collect();
out.sort_unstable();
out
}
/// 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> {
@@ -125,9 +223,12 @@ pub struct App {
/// **Shared document state**: how many times the grid's *shape* has moved.
/// The basis's middle tier (ADR 001, improvise-xwi6).
layout_version: u64,
/// **Shared document state**: how many times a shape change invalidated every
/// **Shared document state**: how many times a shape change invalidated a
/// cursor. A counter, not a table of commands (improvise-gvnb, gotcha 39).
cursor_epoch: u64,
/// The views the **last** batch was observed to reshape. The outcome, where
/// `view_shapes` is the mechanism — the shape `batch_aborted` takes.
last_reshaped_views: std::collections::BTreeSet<String>,
/// **Config**: assembled at startup by `KeymapSet::default_keymaps()`. Read
/// every keypress, never mutated during a session.
keymap_set: KeymapSet,
@@ -180,6 +281,7 @@ impl App {
document_generation: 0,
layout_version: 0,
cursor_epoch: 0,
last_reshaped_views: std::collections::BTreeSet::new(),
keymap_set: KeymapSet::default_keymaps(),
storage,
}
@@ -346,6 +448,12 @@ impl App {
self.cursor_epoch
}
/// Which views the last batch actually reshaped. **Empty means "not known"**,
/// never "none" — a reader must fail safe (improvise-nynk).
pub fn last_reshaped_views(&self) -> &std::collections::BTreeSet<String> {
&self.last_reshaped_views
}
/// Rebuild the grid layout from current workbook, active view, and drill
/// state. Note: `with_frozen_records` already handles pruning internally.
pub fn rebuild_layout(&mut self) {
@@ -453,26 +561,48 @@ impl App {
}
/// Apply a batch of effects and report which widgets it could have changed
/// (improvise-cqq). Three tiers off one `EffectKind` — see context/code/.
/// (improvise-cqq). One tier off the `EffectKind`, two observed — context/code/.
pub fn apply_effects(&mut self, effects: Vec<Box<dyn super::effect::Effect>>) -> Dirty {
self.abort_effects = false;
self.batch_aborted = false;
// `layout_version`, `model_state.dirty` and the `Dirty` are all read off
// the same `EffectKind`, inside the loop. `record_versions` cannot be.
let before = effects
// The `Dirty` is read off the `EffectKind`; the two lower basis tiers are
// **observed** around the loop (improvise-75ld, improvise-pdvm).
let touches_model = effects
.iter()
.any(|e| matches!(e.kind(), EffectKind::Model(_)));
let records_before = effects
.iter()
.any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData)))
.then(|| record_fingerprints(&self.model_state.workbook.model.data));
let shapes_before = touches_model.then(|| view_shapes(&self.model_state.workbook));
let mut config_basis = touches_model.then(|| document_config(&self.model_state.workbook));
let mut dirty = Dirty::NOTHING;
for effect in effects {
let kind = effect.kind();
effect.apply(self);
dirty = dirty.union(Dirty::of(kind));
if matches!(kind, EffectKind::Model(_)) {
// Exhaustive, so a fourth `ModelChange` has to answer this question
// 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,
EffectKind::Model(ModelChange::WholeWorkbook) => true,
// Observed instead, in `observe_reshape` (improvise-75ld).
EffectKind::Model(ModelChange::LayoutGeometry) => false,
};
if declared_dirty {
self.model_state.dirty = true;
}
if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry)) {
self.layout_version += 1;
// 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::LayoutGeometry))
&& let Some(basis) = config_basis
{
let now = document_config(&self.model_state.workbook);
self.model_state.dirty |= now != basis;
config_basis = Some(now);
}
if self.abort_effects {
// An abort-setting effect asked for the rest to be skipped. Reset
@@ -482,13 +612,33 @@ impl App {
break;
}
}
if let Some(before) = before {
if let Some(before) = records_before {
self.bump_changed_records(&before);
}
self.observe_reshape(shapes_before);
self.rebuild_layout();
dirty
}
/// Bump `layout_version` for the shape the batch moved and record which views
/// moved — observed, never declared (improvise-pdvm, improvise-nynk).
fn observe_reshape(&mut self, before: Option<std::collections::BTreeMap<String, u64>>) {
self.last_reshaped_views.clear();
let Some(before) = before else { return };
let after = view_shapes(&self.model_state.workbook);
// A view gained or lost is a reshape of that view's name, so the union of
// the two key sets is the domain — not either one alone.
self.last_reshaped_views = before
.keys()
.chain(after.keys())
.filter(|name| before.get(*name) != after.get(*name))
.cloned()
.collect();
if !self.last_reshaped_views.is_empty() {
self.layout_version += 1;
}
}
/// Bump every record whose raw cells read differently than at `before`. Gained,
/// lost and rewritten all count — each moves under an older basis.
fn bump_changed_records(&mut self, before: &std::collections::HashMap<RecordHash, u64>) {
@@ -2794,12 +2944,20 @@ mod basis_tiers {
fn a_layout_change_moves_the_layout_version_and_no_record() {
let mut app = app_with_two_records();
let before = app.layout_version();
// `on_category_added` seats `Month` on Column, so asking for Column is the
// no-op the tier used to be *told* about rather than asked (improvise-75ld).
assert_eq!(app.view().axis_of("Month"), Axis::Column);
app.apply_effects(vec![Box::new(SetAxis {
category: "Month".into(),
axis: Axis::Column,
axis: Axis::Row,
})]);
assert_eq!(
app.view().axis_of("Month"),
Axis::Row,
"the axis did not move"
);
assert_eq!(app.layout_version(), before + 1);
for region in ["North", "South"] {
assert_eq!(
@@ -2810,6 +2968,122 @@ mod basis_tiers {
}
}
/// gotcha 45: `Save` clears `dirty` **in loop order**, so a batch that changes
/// the document and then saves ends clean — for a geometry change too.
#[test]
fn a_batch_that_changes_the_layout_and_then_saves_ends_clean() {
use crate::storage::MemoryFs;
use crate::ui::effect::{Save, SetNumberFormat};
// Both arms, because the CellData half never left the loop: a test over
// the geometry arm alone cannot say whether the ordering rule is general.
let geometry: Vec<Box<dyn Effect>> =
vec![Box::new(SetNumberFormat(",.2f".into())), Box::new(Save)];
let data: Vec<Box<dyn Effect>> = vec![
Box::new(SetCell(cell("North", "Revenue"), CellValue::Number(99.0))),
Box::new(Save),
];
for (label, effects) in [("layout", geometry), ("data", data)] {
let mut app = app_with_two_records();
app.storage = Box::new(MemoryFs::new());
app.model_state.file_path = Some("doc.improv".into());
app.model_state.dirty = false;
app.apply_effects(effects);
assert!(
!app.last_batch_aborted(),
"the {label} batch's save failed, so it proves nothing"
);
assert!(
app.view_state.status_msg.starts_with("Saved"),
"the {label} batch did not report a save: {}",
app.view_state.status_msg
);
assert!(
!app.model_state.dirty,
"the {label} batch saved at the end and the document still reads \
dirty: the TUI says Saved and :q refuses, and the browser worker \
rewrites the whole OPFS document"
);
}
}
/// improvise-75ld: a counter is bumped on an **observed** change, never on a
/// declared intent. Both arms, plus the positive one "never bump" would pass.
#[test]
fn a_refused_or_duplicate_add_category_moves_no_counter() {
use crate::ui::effect::AddCategory;
let mut app = app_with_two_records();
app.model_state.dirty = false;
let version = app.layout_version();
app.apply_effects(vec![Box::new(AddCategory("Zone".into()))]);
assert!(
app.model_state.workbook.model.category("Zone").is_some(),
"the positive arm did not reach the document, so it proves nothing"
);
assert_eq!(
app.layout_version(),
version + 1,
"a real category moved nothing"
);
assert!(
app.model_state.dirty,
"a real category left the document clean"
);
// The duplicate is the reachable silent case: `Model::add_category` returns
// `Ok` for a name it already has, so this arm does not abort.
let mut app = app_with_two_records();
app.model_state.dirty = false;
let version = app.layout_version();
app.apply_effects(vec![Box::new(AddCategory("Region".into()))]);
assert!(
!app.last_batch_aborted(),
"the duplicate arm aborted, so it is the refusal arm wearing its name"
);
assert_eq!(
app.layout_version(),
version,
"a duplicate category changed nothing and still refused every other \
client's next batch with Rejection::LayoutMoved"
);
assert!(
!app.model_state.dirty,
"a duplicate category changed nothing and still made the worker rewrite \
the whole OPFS document"
);
// And the refusal, which aborts: the bumps used to run before the check.
let mut app = app_with_two_records();
let filled = (0..64).take_while(|i| {
app.model_state
.workbook
.add_category(format!("Filler{i}"))
.is_ok()
});
assert!(filled.count() > 0, "the fixture is already at the limit");
app.rebuild_layout();
app.model_state.dirty = false;
let version = app.layout_version();
app.apply_effects(vec![Box::new(AddCategory("Overflow".into()))]);
assert!(
app.last_batch_aborted(),
"the refusal arm was accepted, so it proves nothing"
);
assert_eq!(
app.layout_version(),
version,
"a refused category moved the basis"
);
assert!(
!app.model_state.dirty,
"a refused category dirtied the document"
);
}
/// improvise-gv9r: a loaded document is dirty, because `LoadModel` does not
/// move `file_path`. A behaviour change with a reason — see the evidence.
#[test]
crates/improvise-command/src/ui/effect.rs
+2 -10
View File
@@ -1109,11 +1109,7 @@ pub struct SetPanelOpen {
impl Effect for SetPanelOpen {
fn apply(&self, app: &mut App) {
match self.panel {
Panel::Formula => app.view_state.formula_panel_open = self.open,
Panel::Category => app.view_state.category_panel_open = self.open,
Panel::View => app.view_state.view_panel_open = self.open,
}
*app.view_state.panel_open_mut(self.panel) = self.open;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
@@ -1127,11 +1123,7 @@ pub struct SetPanelCursor {
}
impl Effect for SetPanelCursor {
fn apply(&self, app: &mut App) {
match self.panel {
Panel::Formula => app.view_state.formula_cursor = self.cursor,
Panel::Category => app.view_state.cat_panel_cursor = self.cursor,
Panel::View => app.view_state.view_panel_cursor = self.cursor,
}
*app.view_state.panel_cursor_mut(self.panel) = self.cursor;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
crates/improvise-command/src/ui/project.rs
+262 -34
View File
@@ -813,6 +813,12 @@ pub enum Broadcast {
To(SessionId, Command),
}
/// Is a client on `active_view` looking at a grid this batch renumbered? Empty
/// means "not known", never "nobody" — `context/code/command.md` (improvise-nynk).
fn cursor_invalidated(reshaped: &std::collections::BTreeSet<String>, active_view: &str) -> bool {
reshaped.is_empty() || reshaped.contains(active_view)
}
/// What one client's command did, as the host needs to hear it. An enum, so a host
/// that wants the [`Dirty`] must name the other cases (gotcha 39).
#[derive(Debug, Clone, PartialEq)]
@@ -953,16 +959,21 @@ impl Sessions {
.collect()
}
/// If the batch renumbered the grid, invalidate every mirror's cursor **and**
/// say so on the wire; neither half works alone (improvise-gvnb).
fn adopt_cursor_epoch(&mut self, app: &App, before: u64) -> Option<Broadcast> {
/// If the batch renumbered the grid, invalidate the mirrors that were looking
/// at it **and** say so on the wire; neither half works alone (improvise-gvnb).
fn adopt_cursor_epoch(&mut self, app: &App, before: u64) -> Vec<Broadcast> {
if app.cursor_epoch() == before {
return None;
return Vec::new();
}
for session in self.sessions.values_mut() {
reduce_view(&mut session.view, &Command::ResetCursor);
}
Some(Broadcast::ToAll(Command::ResetCursor))
let reshaped = app.last_reshaped_views();
self.sessions
.iter_mut()
.filter(|(_, session)| cursor_invalidated(reshaped, &session.view.active_view))
.map(|(id, session)| {
reduce_view(&mut session.view, &Command::ResetCursor);
Broadcast::To(*id, Command::ResetCursor)
})
.collect()
}
/// Re-attach every session to a document that has just replaced theirs, and
@@ -2106,7 +2117,10 @@ mod tests {
);
assert_eq!(
diff(applied).1,
vec![Broadcast::ToAll(Command::ResetCursor)],
vec![
Broadcast::To(asker, Command::ResetCursor),
Broadcast::To(bystander, Command::ResetCursor),
],
"the mirrors were reset and the clients were never told, so the \
diff now believes each of them already holds origin-positioned \
content and will send nothing — stale forever rather than until \
@@ -2171,13 +2185,11 @@ mod tests {
format!("{:?}|{:?}", app.layout.row_items, app.layout.col_items)
}
/// Every model command that renumbers the grid must invalidate every cursor, and
/// the case is only evidence if the grid really moved (improvise-9mo9).
#[test]
fn every_reshaping_model_command_invalidates_every_cursor() {
/// The commands that renumber the grid. A **shared** table: the cursor audience
/// and the two shape counters must answer for the same cases (improvise-pdvm).
fn reshaping_commands() -> Vec<improvise_protocol::ModelCommand> {
use improvise_protocol::ModelCommand;
let reshaping = vec![
vec![
ModelCommand::SetAxis {
category: "Region".into(),
axis: Axis::Column,
@@ -2210,9 +2222,14 @@ mod tests {
category: "Product".into(),
item: "Widget".into(),
},
];
]
}
for cmd in reshaping {
/// Every model command that renumbers the grid must invalidate every cursor, and
/// the case is only evidence if the grid really moved (improvise-9mo9).
#[test]
fn every_reshaping_model_command_invalidates_every_cursor() {
for cmd in reshaping_commands() {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
let before = grid_shape(&app);
@@ -2238,24 +2255,23 @@ mod tests {
0,
"{label} left a bystander indexing a grid that renumbered under it"
);
assert!(
diff(applied)
.1
.contains(&Broadcast::ToAll(Command::ResetCursor)),
"{label} reset the mirrors and told no client, which is worse than \
doing neither: the diff now believes they hold origin-positioned \
content and will send nothing"
);
let broadcast = diff(applied).1;
for (who, id) in [("asker", asker), ("bystander", bystander)] {
assert!(
broadcast.contains(&Broadcast::To(id, Command::ResetCursor)),
"{label} reset the {who}'s mirror and told it nothing, which is \
worse than doing neither: the diff now believes it holds \
origin-positioned content and will send nothing"
);
}
}
}
/// The third negative arm: a command the document ACCEPTS but which changes
/// nothing reshaped nothing either (found in review of improvise-9mo9).
#[test]
fn an_accepted_no_op_invalidates_no_cursor() {
/// The commands the document ACCEPTS while changing nothing. Shared with the
/// counter test for the reason `reshaping_commands` is (improvise-pdvm).
fn no_op_commands() -> Vec<improvise_protocol::ModelCommand> {
use improvise_protocol::ModelCommand;
let no_ops = vec![
vec![
ModelCommand::SetAxis {
category: "Region".into(),
axis: Axis::Row,
@@ -2304,9 +2320,14 @@ mod tests {
category: "_Measure".into(),
item: "Profit".into(),
},
];
]
}
for cmd in no_ops {
/// The third negative arm: a command the document ACCEPTS but which changes
/// nothing reshaped nothing either (found in review of improvise-9mo9).
#[test]
fn an_accepted_no_op_invalidates_no_cursor() {
for cmd in no_op_commands() {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
let before = grid_shape(&app);
@@ -2398,6 +2419,213 @@ mod tests {
}
}
/// `Default` and `Second` are two differently shaped views of one document, so
/// a reshape of one is observably not a reshape of the other (improvise-nynk).
fn app_with_two_reshapeable_views() -> App {
let mut app = app_with_a_reshapeable_grid();
{
let second = app.model_state.workbook.create_view("Second");
second.set_axis("Channel", Axis::Row);
second.set_axis("_Measure", Axis::Column);
}
app.rebuild_layout();
app
}
/// improvise-nynk: which view a client is on is that client's, so a reshape of
/// one view is not addressed to a client sitting on another.
#[test]
fn a_reshape_of_one_view_leaves_a_client_on_another_view_alone() {
use improvise_protocol::ModelCommand;
let mut app = app_with_two_reshapeable_views();
let mut sessions = Sessions::default();
let (asker, _) = sessions.connect(&app, 100, 100);
let (bystander, _) = sessions.connect(&app, 100, 100);
sessions.apply(&mut app, bystander, &Command::SwitchView("Second".into()));
sessions.apply(&mut app, bystander, &Command::SetRowOffset(1));
sessions.apply(&mut app, asker, &Command::SetRowOffset(1));
assert_eq!(
sessions.get_mut(bystander).unwrap().view().active_view,
"Second",
"the bystander never left the view the reshape is aimed at"
);
// Aimed at the host's own view, which is `Default`: `HideItem` writes
// `view_mut()`, so `Second`'s shape is untouched by construction.
let applied = sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::HideItem {
category: "Region".into(),
item: "North".into(),
},
);
let broadcast = diff(applied).1;
// The positive arm stays, or "never invalidate anyone" passes this test.
assert_eq!(
sessions.get_mut(asker).unwrap().view().cursor().row_offset,
0,
"the asker is on the reshaped view and still indexes the old grid"
);
assert!(
broadcast.contains(&Broadcast::To(asker, Command::ResetCursor)),
"the asker's mirror was reset and the asker was never told"
);
// And the negative arm, which is the ticket.
assert_eq!(
sessions
.get_mut(bystander)
.unwrap()
.view()
.cursor()
.row_offset,
1,
"a reshape of `Default` threw away the cursor of a client on `Second`"
);
assert!(
!broadcast
.iter()
.any(|b| matches!(b, Broadcast::To(id, _) if *id == bystander)),
"a client on a view nothing reshaped was told its cursor is invalid"
);
assert!(
!broadcast.contains(&Broadcast::ToAll(Command::ResetCursor)),
"a cursor invalidation addressed to everybody is addressed to nobody \
in particular, which is the whole of improvise-nynk"
);
}
/// The empty set is "not known", never "nobody": a reader must fail safe, or the
/// narrowing turns into the half-fix that is worse than no fix (gotcha 24).
#[test]
fn an_unknown_reshape_set_invalidates_everybody() {
let none = std::collections::BTreeSet::new();
assert!(cursor_invalidated(&none, "Default"));
assert!(cursor_invalidated(&none, "Second"));
let default_only: std::collections::BTreeSet<String> =
["Default".to_string()].into_iter().collect();
assert!(cursor_invalidated(&default_only, "Default"));
assert!(!cursor_invalidated(&default_only, "Second"));
}
/// improvise-pdvm: `layout_version` and `cursor_epoch` answer the same question
/// -- "has the grid's shape moved under you" -- so they answer over one table.
#[test]
fn both_shape_counters_move_for_every_reshaping_command() {
for cmd in reshaping_commands() {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
app.model_state.dirty = false;
let shape = grid_shape(&app);
let (layout, epoch) = (app.layout_version(), app.cursor_epoch());
let mut sessions = Sessions::default();
let (asker, _) = sessions.connect(&app, 100, 100);
sessions.apply_caught_up(&mut app, asker, cmd);
assert_ne!(
shape,
grid_shape(&app),
"{label} reshaped nothing, so this case is not evidence of anything"
);
assert!(
app.model_state.dirty,
"{label} reshaped the grid and left the document reading clean, so \
the browser worker never writes it back to OPFS"
);
assert_ne!(
app.cursor_epoch(),
epoch,
"{label} reshaped the grid and no cursor was invalidated"
);
assert_ne!(
app.layout_version(),
layout,
"{label} reshaped the grid and told every client's basis it had not, \
so a batch composed against the old shape is still accepted"
);
}
}
/// The third case the split creates: a change the file carries but the grid does
/// not draw. `dirty` must rise where `layout_version` must not (improvise-75ld).
#[test]
fn a_configuration_change_that_draws_nothing_is_still_a_document_change() {
use improvise_protocol::ModelCommand;
// `hide_item` records a name whether or not the model has one, and the
// number format is written to the file while changing no row or column.
let config_only = vec![
ModelCommand::HideItem {
category: "Region".into(),
item: "NoSuchItem".into(),
},
ModelCommand::SetNumberFormat(",.4f".into()),
];
for cmd in config_only {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
app.model_state.dirty = false;
let shape = grid_shape(&app);
let layout = app.layout_version();
let mut sessions = Sessions::default();
let (asker, _) = sessions.connect(&app, 100, 100);
sessions.apply_caught_up(&mut app, asker, cmd);
assert_eq!(
shape,
grid_shape(&app),
"{label} reshaped the grid, so it is not a configuration-only change"
);
assert!(
app.model_state.dirty,
"{label} is written to the file and left the document reading clean"
);
assert_eq!(
app.layout_version(),
layout,
"{label} draws nothing new and still refused every other client's \
next batch"
);
}
}
/// The other direction, over the other table: a command that changes nothing
/// moves neither counter and does not raise `dirty` (improvise-75ld).
#[test]
fn neither_shape_counter_moves_for_an_accepted_no_op() {
for cmd in no_op_commands() {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
app.model_state.dirty = false;
let shape = grid_shape(&app);
let (layout, epoch) = (app.layout_version(), app.cursor_epoch());
let mut sessions = Sessions::default();
let (asker, _) = sessions.connect(&app, 100, 100);
sessions.apply_caught_up(&mut app, asker, cmd);
assert_eq!(
shape,
grid_shape(&app),
"{label} reshaped the grid, so it is not a no-op and proves nothing here"
);
assert_eq!(app.cursor_epoch(), epoch, "{label} invalidated a cursor");
assert_eq!(
app.layout_version(),
layout,
"{label} changed nothing and still refused every other client's \
next batch with Rejection::LayoutMoved"
);
}
}
/// The other arm: a command that leaves the shape alone must not throw anyone's
/// cursor away, or "reset on every model command" passes the test above.
#[test]
@@ -2489,7 +2717,7 @@ mod tests {
let (dirty, broadcast) = diff(applied);
assert_eq!(
broadcast,
vec![Broadcast::ToAll(Command::ResetCursor)],
vec![Broadcast::To(id, Command::ResetCursor)],
"a new category lands on an axis in every view, so every cursor is \
invalidated — but no client folded an active view, so no SwitchView \
correction is owed"
crates/improvise-core/src/model/category.rs
+1 -1
View File
@@ -50,7 +50,7 @@ impl Group {
/// Regular categories store their items explicitly; virtual ones are
/// synthesized at query time by the layout layer. context/code/core.md §9.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, Default)]
pub enum CategoryKind {
#[default]
Regular,
crates/improvise-core/src/view/axis.rs
+1 -1
View File
@@ -1,6 +1,6 @@
use serde::{Deserialize, Serialize};
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
pub enum Axis {
Row,
crates/improvise-protocol/src/lib.rs
+39
View File
@@ -403,6 +403,45 @@ impl ViewState {
pub fn cursor_mut(&mut self) -> &mut ViewCursor {
self.cursors.entry(self.active_view.clone()).or_default()
}
/// The **one** place a [`Panel`] becomes a field (improvise-56ek). The TUI's
/// `SetPanelOpen` effect and `reduce_view`'s arm both come here.
pub fn panel_open_mut(&mut self, panel: Panel) -> &mut bool {
match panel {
Panel::Formula => &mut self.formula_panel_open,
Panel::Category => &mut self.category_panel_open,
Panel::View => &mut self.view_panel_open,
}
}
/// Read side of [`ViewState::panel_open_mut`]. A caller that matched on the
/// panel itself would be the second derivation this pair exists to remove.
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,
}
}
/// The same single home for the panel cursors, which carried the identical
/// three-way match in the identical two crates.
pub fn panel_cursor_mut(&mut self, panel: Panel) -> &mut usize {
match panel {
Panel::Formula => &mut self.formula_cursor,
Panel::Category => &mut self.cat_panel_cursor,
Panel::View => &mut self.view_panel_cursor,
}
}
/// Read side of [`ViewState::panel_cursor_mut`].
pub fn panel_cursor(&self, panel: Panel) -> usize {
match panel {
Panel::Formula => self.formula_cursor,
Panel::Category => self.cat_panel_cursor,
Panel::View => self.view_panel_cursor,
}
}
}
#[cfg(test)]
crates/improvise-protocol/src/reduce.rs
+43 -37
View File
@@ -204,16 +204,8 @@ pub fn reduce_view(view: &mut ViewState, cmd: &Command) {
view.buffers.remove(name);
}
Command::SetTileCatIdx(i) => view.tile_cat_idx = *i,
Command::SetPanelOpen { panel, open } => match panel {
Panel::Formula => view.formula_panel_open = *open,
Panel::Category => view.category_panel_open = *open,
Panel::View => view.view_panel_open = *open,
},
Command::SetPanelCursor { panel, cursor } => match panel {
Panel::Formula => view.formula_cursor = *cursor,
Panel::Category => view.cat_panel_cursor = *cursor,
Panel::View => view.view_panel_cursor = *cursor,
},
Command::SetPanelOpen { panel, open } => *view.panel_open_mut(*panel) = *open,
Command::SetPanelCursor { panel, cursor } => *view.panel_cursor_mut(*panel) = *cursor,
Command::ToggleCatExpand(cat) => {
if !view.expanded_cats.remove(cat) {
view.expanded_cats.insert(cat.clone());
@@ -303,33 +295,6 @@ mod tests {
assert_eq!(view.cursor().selected, (1, 1));
}
#[test]
fn panel_commands_address_the_named_panel_only() {
// Closing it is what discriminates: the category panel is OPEN at
// launch, so any assertion that it is open passes against a no-op.
let closed = fold(&[Command::SetPanelOpen {
panel: Panel::Category,
open: false,
}]);
assert!(!closed.category_panel_open, "the fold moved the flag");
let view = fold(&[
Command::SetPanelOpen {
panel: Panel::Category,
open: true,
},
Command::SetPanelCursor {
panel: Panel::Category,
cursor: 4,
},
]);
assert!(view.category_panel_open);
assert_eq!(view.cat_panel_cursor, 4);
assert!(!view.formula_panel_open && !view.view_panel_open);
assert_eq!(view.formula_cursor, 0);
assert_eq!(view.view_panel_cursor, 0);
}
#[test]
fn toggle_cat_expand_is_involutive() {
let view = fold(&[
@@ -492,4 +457,45 @@ mod tests {
assert_eq!(&back, cmd, "round trip changed {cmd:?}");
}
}
/// improvise-56ek: `Panel` becomes a field in one place, and the fold reaches it.
/// Replaces a `Panel::Category`-only test that could not tell panels apart.
#[test]
fn a_panel_command_writes_that_panel_and_no_other() {
use crate::Panel;
for panel in Panel::ALL {
// Zeroed first, so opening is a move for every panel: the category
// panel is OPEN at launch, and asserting it is open passes a no-op.
let mut view = ViewState::default();
for p in Panel::ALL {
*view.panel_open_mut(p) = false;
*view.panel_cursor_mut(p) = 0;
}
reduce_view(&mut view, &Command::SetPanelOpen { panel, open: true });
reduce_view(&mut view, &Command::SetPanelCursor { panel, cursor: 7 });
for other in Panel::ALL {
assert_eq!(
view.panel_open(other),
other == panel,
"SetPanelOpen({panel:?}) wrote {other:?}'s open flag"
);
assert_eq!(
view.panel_cursor(other),
if other == panel { 7 } else { 0 },
"SetPanelCursor({panel:?}) wrote {other:?}'s cursor"
);
}
// By name, once per panel: asserting only through the accessors asserts
// through the mapping under test, and a bijection passes (gotcha 3).
let (open, cursor) = match panel {
Panel::Formula => (view.formula_panel_open, view.formula_cursor),
Panel::Category => (view.category_panel_open, view.cat_panel_cursor),
Panel::View => (view.view_panel_open, view.view_panel_cursor),
};
assert!(open, "{panel:?} is not the field {panel:?} names");
assert_eq!(cursor, 7, "{panel:?}'s cursor is not the field it names");
}
}
}
crates/improvise-protocol/src/resolve.rs
+3 -11
View File
@@ -176,7 +176,7 @@ fn click_cell(state: &ClientState, row: usize, col: usize) -> Option<Vec<Command
/// click inside the focused panel is not a mode change every time.
fn focus(state: &ClientState, panel: Panel) -> Vec<Command> {
let mut commands = Vec::new();
if !panel_open(&state.view, panel) {
if !state.view.panel_open(panel) {
commands.push(Command::SetPanelOpen { panel, open: true });
}
if state.view.mode.panel() != Some(panel) {
@@ -246,14 +246,14 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Op
let panel = named_panel(args.first()?)?;
Some(vec![Command::SetPanelOpen {
panel,
open: !panel_open(&state.view, panel),
open: !state.view.panel_open(panel),
}])
}
// The interactive arm of `TogglePanelAndFocus`: open-and-focus unless
// this panel is already both. See `context/code/clients.md`.
"toggle-panel-and-focus" => {
let panel = named_panel(args.first()?)?;
let closing = panel_open(&state.view, panel) && state.view.mode.panel() == Some(panel);
let closing = state.view.panel_open(panel) && state.view.mode.panel() == Some(panel);
Some(vec![
Command::SetPanelOpen {
panel,
@@ -364,14 +364,6 @@ fn named_panel(name: &str) -> Option<Panel> {
Panel::ALL.into_iter().find(|p| p.name() == name)
}
fn panel_open(view: &ViewState, panel: Panel) -> bool {
match panel {
Panel::Formula => view.formula_panel_open,
Panel::Category => view.category_panel_open,
Panel::View => view.view_panel_open,
}
}
/// One numeric argument, as the registry's interactive parsers read it.
fn num(args: &[String], i: usize) -> Option<i32> {
args.get(i)?.parse().ok()