Merge branch 'main' into worktree-agent-a471cf08030a9af4c

This commit is contained in:
Edward Langley
2026-09-20 21:11:31 -07:00
20 changed files with 1729 additions and 217 deletions
+74 -13
View File
@@ -477,20 +477,30 @@ the top edge (improvise-jjx).
**`body_from(idx)`** drops the group header that owns the first drawn item: a **`body_from(idx)`** drops the group header that owns the first drawn item: a
frame opening mid-group carries that header so `group_of` can answer for it, frame opening mid-group carries that header so `group_of` can answer for it,
but it is *above* the first drawn row and must not be painted as one. This is but it is *above* the first drawn row and must not be painted as one. This is
the same cut `GridLayout::data_row_to_visual` has always made. Its argument the same cut `GridLayout::data_row_to_visual` has always made.
exists only because the two front ends scroll differently — a browser's frame
is already windowed and passes `0`, the TUI's covers the whole grid and passes
its row offset (gotcha 44) — **not** because the rule differs.
**`group_of` answers "nearest preceding header", not membership, and Two things about its argument, and the first of them **used to be the whole
deliberately so: it is what the TUI has always painted.** `expand_category` story and was wrong**. It exists partly because the two front ends scroll
emits a header on *entering* a group and no boundary on leaving one, so an differently — a browser's frame is already windowed and passes `0`, the TUI's
ungrouped item following a grouped run is answered with the run's group — and covers the whole grid and passes its row offset (gotcha 44). But `idx` alone
the TUI's column-group row spans it under that label today. Encoding cannot say whether the frame *leads the axis*, and at the top there is no
membership, or an end boundary, means changing `AxisEntry` in header "above" to drop: it is content, and a collapsed first group has no
`improvise-core` and changing what the TUI paints: a fix to the layout, not to other affordance. That is `AxisFrame::first` and improvise-0o6x — §3a.
this projection of it, and it is filed rather than smuggled into the migration
that made the two ends agree (improvise-anr9, raised by Copilot on PR #23). **`group_of` answers membership**, and the paragraph that stood here said the
opposite: that it answers "nearest preceding header", deliberately, because
that is what the TUI had always painted, and that encoding an end boundary
would mean changing `AxisEntry` in `improvise-core` and was "filed rather than
smuggled into" improvise-anr9. All of that was true when written. **The filed
ticket is improvise-gd83 and it has landed** — `AxisEntry::GroupEnd` closes a
run, `groups()` clears on it, and the TUI's painted span changed with it. The
mechanism and its two emission arms are §3a.
That correction is here rather than only in §3a on purpose. This paragraph
forbade, in terms, the change §3a describes; a reader arriving at the stale
copy first would conclude the rule had been smuggled past its own prohibition.
When a fact is stated twice, changing it is a grep (gotcha 48) — and the two
copies being in *one file* is what made this worth saying out loud.
### The width rule has one home ### The width rule has one home
@@ -751,6 +761,57 @@ Nothing is defined for the help overlay: its content is static and
--- ---
## §3a. The frame's group boundaries and its position in the axis
Two facts `AxisFrame` gained after `improvise-anr9`, both about group runs.
**`FrameEntry::GroupEnd` closes a run.** `group_of` used to answer "nearest
preceding header", which is not membership: `expand_category` emitted a header
on *entering* a group and nothing on leaving one, so an ungrouped item after a
grouped run inherited it (`improvise-gd83`). The §3 paragraph that recorded the
old rule — and forbade this change — is corrected there, not merely superseded
here. `AxisEntry::GroupEnd` is
the boundary, carried onto the wire as `FrameEntry::GroupEnd`; `groups()` clears
`current` on it and `group_of` stops walking. It is emitted only on
`Some -> None`, never between two groups — the next header already ends the
previous run — and at the end of an expansion whose last item was grouped, which
is the across-prefix arm `cross_product` creates.
Nothing draws a `GroupEnd`. Both front ends skip it and it consumes no data
index, so it is invisible except through `group_of`/`groups`.
**`AxisFrame::first` is the absolute data index of the frame's first item.** The
windowing rule carries the nearest header *above* a scrolled window so
`group_of` can answer, and `body_from` drops it on the way to the screen. At the
top of an axis there is no "above", so that same leading header is content — a
collapsed first group has no other affordance (`improvise-0o6x`). `idx` alone
cannot tell the two apart, because the browser's frame is already windowed and
always passes `0`; `first + idx == 0` can.
The TUI builds over the whole grid so `first` is always `0` there, and the
browser's comes from the projected window. A frame built by hand in a test must
set it, which is why it is a plain field rather than derived.
It carries `#[serde(default)]`, and **that is a defect rather than a decision**
(improvise-1wl0): `first: 0` is not an absence but the claim "this frame leads
the axis", which is the claim `body_from` acts on — so a frame that loses the
field deserializes as leading. No skew ships today, both ends being one
workspace at one commit. Same argument as `Basis` having no `Default`.
**`group_runs` / `group_run_heads` are the coalescing rule's one home.** The TUI
and `render.rs::col_groups` each coalesced adjacent columns sharing a group,
kept in step by a comment (`improvise-8qlr`). Runs coalesce by group **name**,
so two adjacent runs under two different headers of the same name read as one —
that is what both ends did before and is preserved deliberately, because
changing the rule in the commit that moves it would leave the agreement test
unable to tell "both ends agree" from "both ends changed together".
`FrameGroup` carries `cat` and could fix it, and **a single home reads as
decided** — which is the new risk and why it is filed as improvise-h0iv rather
than left as a note. The TUI's one
divergence is that it labels its first *visible* column even mid-run, so a
scrolled grid does not draw an unlabelled group line.
## 4. `frame.rs` — one envelope pair, two deployments ## 4. `frame.rs` — one envelope pair, two deployments
### The framing adds as little as it can get away with ### The framing adds as little as it can get away with
+28 -5
View File
@@ -121,11 +121,34 @@ enum, and only the last translation is local.**
`mode_style(&AppMode) -> Style` stays here for the same reason (`Style` and `mode_style(&AppMode) -> Style` stays here for the same reason (`Style` and
`Color` are ratatui types and `improvise-command`, where `AppMode` lives, `Color` are ratatui types and `improvise-command`, where `AppMode` lives,
carries no rendering-framework dependency) — **but it is not the same shape.** carries no rendering-framework dependency), and since `improvise-zx8t` it is
It matches on `AppMode` directly with a `_ =>` wildcard for "the default chrome **exhaustive**: the fourteen variants that take the default chrome are spelled
for every non-highlighted mode", so a new `AppMode` variant silently inherits out rather than swept up by a `_ =>` wildcard, so adding an `AppMode` fails the
that chrome instead of failing to compile, and there is no semantic enum in the build here until someone chooses its colour.
shared layer to decide it. That is `improvise-zx8t`, filed and not fixed.
It is still not quite the same shape as `axis_fg` and its two siblings, and the
difference is deliberate rather than pending. Those three map a *semantic enum*
the shared layer owns; `mode_style` matches `AppMode` itself, because there is
no `ModeColor`. `improvise-zx8t` offered making one, and it was declined here
for two reasons: the colour is the only thing at stake, and the ticket says to
decide it together with `improvise-k099`, which asks the same question of
`hint_text`/`label` and is not in this unit. Exhaustiveness is what the
repo map's "the compiler names every one you must fill in" promise needed; a
semantic enum is a separate, larger answer and is still open.
**The guard is the compiler, not a test.** Nothing asserts the colours, and a
test enumerating the variants would not fail when a fifteenth is added — only
the missing match arm does. Verified by deleting the `AppMode::Help` arm:
`error[E0004]: non-exhaustive patterns: `&AppMode::Help` not covered`.
### The minibuffer shares the bottom row with the status line
`draw_bottom_bar` used to render the minibuffer *instead of* `draw_status`, so a
status message written while any text-entry mode was open was never drawn
(`improvise-mai9`, and the stranding gotcha 21 describes). `minibuffer_line`
composes both: the prompt and buffer on the left, any pending `status_msg`
right-aligned. The alternatives and why they lost are in
`context/test-evidence/tui.md`.
### The caches are built here, per frame ### The caches are built here, per frame
+261
View File
@@ -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 the `EffectKind` now (improvise-gv9r, gotcha 45), so driving the effect directly
would assert a flag nothing in production sets. 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` ## `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 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. 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` ## `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 backend to write to `""`, and the user then reads whatever I/O error that produces
instead of "you typed nothing". 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` ## `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 entirely; `parse` takes the first match, so every surviving entry still agrees with
its own name. Uniqueness is what catches it. 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` ## `command/cmd/text_buffer.rs`
+54
View File
@@ -543,3 +543,57 @@ uses. Fault injected into that arm alone: it fails, `Number(0.0)` against
The general shape: an injected fault proves coverage only of the code path it is The general shape: an injected fault proves coverage only of the code path it is
injected into. Two arms of the same rule need two injections, and if one of them injected into. Two arms of the same rule need two injections, and if one of them
turns out to be unreachable, that is a fact about the design worth writing down. turns out to be unreachable, that is a fact about the design worth writing down.
### a_user_category_whose_name_leads_with_an_underscore_is_seated_as_regular
improvise-bidq. `View::on_category_added`'s early return read
`cat_name.starts_with('_') || self.is_records_layout()` — the **same
expression** mixing two derivations one operator apart, the left a name prefix
and the right `CategoryKind`. Four lines up, `is_records_layout` asks the enum.
`Model::add_category` does not validate names, so a user category literally
named `_Foo` was `CategoryKind::Regular` to `records_columns`, `cell_id` and
`is_empty_model`, and "virtual" to these two lines — seated on `Axis::None`,
so it never appeared in a pivot while every other consumer treated it as
ordinary. The second site (`holds_a_regular`) had the same split, which is why
the test asserts a *second* category's seat too: fixing only the first leaves
`_Foo` on Row while `Region` also takes Row.
Decided explicitly, per the ticket: `add_category` is **not** made to refuse a
leading underscore. The three reserved names are the ones `CategoryKind::of_name`
knows, `_Foo` is a legal document, and a user-visible restriction is a larger
change than making one answer out of two. The fix is that both sites now ask
`CategoryKind::of_name(..).is_virtual()`.
Fault injected by restoring either `starts_with('_')`: the test fails, `None`
against `Row` on the first site and `Row` against `Column` on the second.
### an_ungrouped_item_after_a_grouped_run_belongs_to_no_group
### a_grouped_run_does_not_leak_into_the_next_prefix
improvise-gd83, the two arms of one defect. `expand_category` emitted a
`GroupHeader` on **entering** a group and no boundary on leaving one, so
`row_group_for`/`col_group_for`'s nearest-preceding walk answered with a group
that had already ended.
The arms are separate because the two escapes are different and a fix can close
one and miss the other. The first is *within* one expansion — `Jan[Q1]`,
`Feb[Q1]`, `Sep` — closed by emitting `AxisEntry::GroupEnd` on a
`Some -> None` transition. The second is *across* expansions: `cross_product`
expands the inner category once per prefix, each call starting with
`last_group = None`, so `Food/Bob[Family]` is immediately followed by
`Clothing/Carol`, which is ungrouped and inherited Family with no transition
anywhere to hook onto. That one needs a `GroupEnd` at the **end** of an
expansion whose last item was grouped.
`GroupEnd` is deliberately NOT emitted on `Some(A) -> Some(B)`: B's own header
already ends A's run, and emitting one there moved
`data_row_to_visual_skips_headers` and `data_col_to_visual_skips_headers` for
no gain. Those two are untouched, which is the evidence the boundary is as
narrow as it claims.
Fault injected by deleting either emission: the corresponding test fails with
`Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and
`the_two_front_ends_draw_the_same_column_header` fails with it.
+128
View File
@@ -414,3 +414,131 @@ a word pool instead of random letters, `pipe_inner` is never empty,
`rest_of_line` always produces at least one character, and repetitions (`*`) `rest_of_line` always produces at least one character, and repetitions (`*`)
produce 1–4 items rather than 0. Each invocation is seeded from the current time produce 1–4 items rather than 0. Each invocation is seeded from the current time
plus the PID. plus the PID.
### a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open
### the_minibuffer_prompt_is_not_displaced_by_the_message
### an_empty_message_never_pushes_a_row_that_fits_past_its_width
improvise-mai9. `draw_bottom_bar` rendered the minibuffer **instead of** the
status line, so a message written while a text-entry mode was open was never
drawn — not truncated, not overwritten. A test asserting `app.status_msg` was
set passed in every one of those cases, which is how this survived
improvise-6t3's careful effect ordering.
Of the ticket's two candidate fixes, the row is **shared** rather than the
callers being made structurally unable to report: giving the status a second
row changes frame geometry (improvise-q0vb's cost), and "always leave the mode
before reporting" is what `CommitSave` does by hand and would have to be
repeated at every text-entry commit — the thing the ticket calls a workaround.
**The first version of these tests could not fail for the bug, and that is the
entry worth reading.** They called the composing helper `minibuffer_line`
directly. The defect was in the *wiring* — `draw_bottom_bar` building its own
`format!` and never consulting `status_msg` — so reverting that one line
restored the bug in full with all 1411 tests green. The helper did not even go
dead: the test module's `use` kept it alive while it asserted nothing about the
product. Found by review (PR #77), reproduced here before rewriting.
So the guard is now `Buffer`-backed: a real `Terminal<TestBackend>` renders
`draw_bottom_bar` and the assertion reads the row the terminal received. That
is the only assertion that can fail for mai9's actual defect. Fault injected by
restoring `format!("{}{}", mb.prompt, buf)` plus the cursor glyph — both wiring
tests fail, the first naming the missing message and printing the row it did
compose.
The second test exists because a "fix" that dropped to `draw_status` would
satisfy the first on the message alone; it pins that the prompt still leads.
**The third test is the negative arm, and its first version was unfalsifiable
for arithmetic reasons.** `fill_line` computes `pad = width - left.len() -
right.len()`, so with an empty status an unconditional `format!(" {msg} ")`
produces a byte-identical row at any width where the line fits: `left` plus
`width - left.len()` spaces, either way. The `match status_msg.is_empty()` is
observable **only** in the band where the prompt fits but `left.len() + 4`
does not — widths 16..=19 for this fixture, where the unguarded arm returns a
row wider than the terminal.
That test then failed a second time in a more interesting way: it derived its
lower bound by *calling `minibuffer_line` itself*, so the injected fault moved
the bound with it and the loop skipped the failing band entirely. The bound is
now spelled locally. **A bound read off the function under test is not a
bound** — it is the fault grading its own exam.
Known and not fixed here: `fill_line` pads on `left.len()`, which is bytes, so
the right-hand message sits two cells further right than intended whenever the
left contains the multi-byte cursor glyph. `draw_status` has always done this
with its own multi-byte content; it is pre-existing and out of scope. It is
also what makes the band above four columns wide rather than two.
### a_group_header_first_on_its_axis_reaches_both_front_ends
improvise-0o6x. `AxisFrame::body_from(idx)` started at the first **item**, so at
the top of the axis a leading `FrameEntry::Group` was skipped on the way to the
screen — in both front ends. A collapsed group first on its axis therefore had
its rows hidden and its header nowhere, so there was no affordance to get them
back. The frame carried the header correctly; `body_from` dropped it.
It is a boundary condition, as the ticket suspected, but the boundary is not
`idx == 0`. The windowing rule carries the nearest header ABOVE a scrolled
window for `group_of` and `body_from` discards it — correctly. At the top of
the axis there is no "above", and a frame cannot tell the two apart from `idx`
alone, because the browser's frame is windowed and always passes `0`. So
`AxisFrame` gained `first`, the absolute data index of its first item, and the
rule is `first + idx == 0`. Without it the browser would have drawn an extra
header row at every scrolled position that the TUI does not.
This test is separate from `the_two_front_ends_draw_the_same_row_header`
because that fixture orders `Carol` before `Family` to dodge this case **on
purpose**, and says so. Both collapse states are driven: the uncollapsed arm
alone passes against a frame that leads with a header only when it has items
under it, which is the shape the windowing bug below produced.
The windowing half was found by this test and not predicted by the ticket. A
collapsed leading group has NO item to carry its header into the window, so it
reaches the frame only through `axis_frame`'s `pending` queue — and a first
version of the fix had `GroupEnd` **clear** `pending` above the window, which
deleted exactly that header. `GroupEnd` is now queued like a header and clears
nothing.
Fault injected by forcing `body_from`'s match to `false`: this test fails and
`the_whole_frame_is_unchanged` fails with it.
### the_whole_frame_is_unchanged
Re-pinned for improvise-0o6x. `GROUPED_FRAME` moved by exactly one line: the
leading `▼ Co…` header now draws above `East`, and everything below shifts
down one. Nothing else in the buffer changed. That the group line, the column
headers, the separator, the data rows and the totals strip are byte-identical
is the evidence the change is the boundary condition it claims to be and not a
reshuffle of the draw loop.
### the_two_front_ends_draw_the_same_column_header
Two changes, from two tickets.
improvise-gd83: `THE_MEMBERSHIP_CORRECT_SPANS` and its `assert_ne!` tripwire are
gone, and the value they predicted — `[(Q1,2), (None,1), (Q1,2), (None,1)]` —
is now what the test asserts, element for element. `Sep` is ungrouped, so the
run ends before it and resumes under the second measure's own `Q1` header. A
precondition asserts the fixture still contains an ungrouped column, because
without one the test cannot see a leaked group at all.
improvise-8qlr: the TUI arm was `group_line.contains("▼ Q1")`, which is
one-directional and passes while the TUI labels **every** column. The run rule
now lives once, as `AxisFrame::group_runs`/`group_run_heads`, and both ends
call it; the TUI's group line is compared cell for cell against spans derived
from the browser's own runs.
Fault injected exactly as the ticket specifies — forcing the TUI to emit the
group label on every column (`head = col_groups.get(ci)`, dropping the run-head
lookup). Before: `cargo test -p improvise` stayed green. After: this test fails
with `["▼ Q1","▼ Q1","","▼ Q1","▼ Q1",""]` against
`["▼ Q1","","","▼ Q1","",""]`.
One thing the shared rule does **not** flatten: the TUI labels its first
*visible* column even mid-run, because a scrolled grid would otherwise draw an
unlabelled group line. That is a scrolling concession, kept explicitly, and it
is why the comparison is run at `col_offset == 0`.
@@ -195,6 +195,32 @@ mod tests {
let dbg = effects_debug(&effects); let dbg = effects_debug(&effects);
assert!(dbg.contains("ExportCsv"), "Expected ExportCsv, got: {dbg}"); assert!(dbg.contains("ExportCsv"), "Expected ExportCsv, got: {dbg}");
assert!(dbg.contains("Normal"), "Expected Normal mode, 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, &reg);
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 /// Naming a new measure in the trailing `+` column at the bottom-right sent the
@@ -696,10 +722,18 @@ impl Cmd for CommitExport {
"commit-export" "commit-export"
} }
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> { 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![ vec![
Box::new(effect::ExportCsv(std::path::PathBuf::from(buf))),
effect::change_mode(AppMode::Normal), 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) .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 { pub fn row_count(&self) -> usize {
self.layout.row_count() self.layout.row_count()
} }
@@ -355,11 +355,7 @@ pub fn default_registry() -> CmdRegistry {
require_args("toggle-panel-and-focus", args, 1)?; require_args("toggle-panel-and-focus", args, 1)?;
let panel = parse_panel(&args[0])?; let panel = parse_panel(&args[0])?;
// Default interactive: if already open+focused -> close, else open+focus // Default interactive: if already open+focused -> close, else open+focus
let currently_open = match panel { let currently_open = ctx.panel_open(panel);
Panel::Formula => ctx.formula_panel_open,
Panel::Category => ctx.category_panel_open,
Panel::View => ctx.view_panel_open,
};
let currently_focused = ctx.mode.panel() == Some(panel); let currently_focused = ctx.mode.panel() == Some(panel);
let (open, focused) = if currently_open && currently_focused { let (open, focused) = if currently_open && currently_focused {
(false, false) // close (false, false) // close
@@ -389,11 +385,7 @@ pub fn default_registry() -> CmdRegistry {
|args, ctx| { |args, ctx| {
require_args("toggle-panel-visibility", args, 1)?; require_args("toggle-panel-visibility", args, 1)?;
let panel = parse_panel(&args[0])?; let panel = parse_panel(&args[0])?;
let currently_open = match panel { let currently_open = ctx.panel_open(panel);
Panel::Formula => ctx.formula_panel_open,
Panel::Category => ctx.category_panel_open,
Panel::View => ctx.view_panel_open,
};
Ok(Box::new(TogglePanelVisibility { Ok(Box::new(TogglePanelVisibility {
panel, panel,
currently_open, 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}"
);
}
}
}
+326 -34
View File
@@ -80,8 +80,26 @@ fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap<String, u64> {
.collect() .collect()
} }
/// Everything a `LayoutGeometry` effect can write that the file would carry. Not the /// `document_config` plus the cells and the formulas. `CellData` is wider than its
/// cell data — `record_fingerprints` answers for that — so it is cheap per batch. /// 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 { fn document_config(workbook: &Workbook) -> u64 {
use std::hash::{Hash, Hasher}; use std::hash::{Hash, Hasher};
let mut h = std::collections::hash_map::DefaultHasher::new(); let mut h = std::collections::hash_map::DefaultHasher::new();
@@ -131,6 +149,26 @@ fn sorted_pairs(
out 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 /// 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`. /// never on a wire, unlike [`RecordHash`] — see `context/code/command.md`.
fn record_fingerprints(data: &DataStore) -> std::collections::HashMap<RecordHash, u64> { 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() { for (key, value) in data.iter_cells() {
let mut hasher = std::collections::hash_map::DefaultHasher::new(); let mut hasher = std::collections::hash_map::DefaultHasher::new();
key.hash(&mut hasher); key.hash(&mut hasher);
// Exhaustive, and by bits rather than by `Debug`: two values that print hash_value(value, &mut hasher);
// 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);
}
}
*out.entry(RecordHash::of(&key)).or_insert(0) ^= hasher.finish(); *out.entry(RecordHash::of(&key)).or_insert(0) ^= hasher.finish();
} }
out out
@@ -413,7 +436,13 @@ impl App {
Some((key, view)) => (key.as_str(), view), Some((key, view)) => (key.as_str(), view),
None => { None => {
let default = workbook.active_view.as_str(); 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). /// 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 { pub(crate) fn display_value_in(&self, layout: &GridLayout, key: &CellKey) -> String {
match crate::view::synthetic_record_info(key) { match crate::view::synthetic_record_info(key) {
Some((idx, col)) => layout Some((idx, col)) => self
.record_col(col) .staged_drill_edit(layout, idx, col)
.and_then(|rc| {
self.drill_state
.as_ref()
.and_then(|s| s.pending_edits.get(&(idx, rc.clone())).cloned())
})
.or_else(|| layout.resolve_display(key)) .or_else(|| layout.resolve_display(key))
.unwrap_or_default(), .unwrap_or_default(),
None => self 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<'_> { pub fn cmd_context(&self, key: KeyCode, _mods: KeyModifiers) -> CmdContext<'_> {
let view = self.view(); let view = self.view();
let layout = &self.layout; let layout = &self.layout;
@@ -570,12 +606,19 @@ impl App {
let touches_model = effects let touches_model = effects
.iter() .iter()
.any(|e| matches!(e.kind(), EffectKind::Model(_))); .any(|e| matches!(e.kind(), EffectKind::Model(_)));
let records_before = effects let writes_cells = effects
.iter() .iter()
.any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData))) .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData)));
.then(|| record_fingerprints(&self.model_state.workbook.model.data)); 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 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; let mut dirty = Dirty::NOTHING;
for effect in effects { for effect in effects {
let kind = effect.kind(); let kind = effect.kind();
@@ -585,11 +628,11 @@ impl App {
// rather than defaulting into the observed half by omission. // rather than defaulting into the observed half by omission.
let declared_dirty = match kind { let declared_dirty = match kind {
EffectKind::ViewOnly => false, EffectKind::ViewOnly => false,
// Things no structural fingerprint sees: a formula, a sort order, // A replaced document differs from the file its path names even
// a replaced document. // where the content matches, so it is declared (improvise-748f).
EffectKind::Model(ModelChange::CellData) => true,
EffectKind::Model(ModelChange::WholeWorkbook) => true, 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, EffectKind::Model(ModelChange::LayoutGeometry) => false,
}; };
if declared_dirty { if declared_dirty {
@@ -597,6 +640,13 @@ impl App {
} }
// The observed half, read **inside the loop** so `Save` still clears it // The observed half, read **inside the loop** so `Save` still clears it
// in loop order (gotcha 45) — the position is the property. // 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)) if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry))
&& let Some(basis) = config_basis && 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, /// improvise-8uz: moving the cursor must leave the shared document untouched,
/// so one client scrolling cannot move another's viewport. /// so one client scrolling cannot move another's viewport.
#[test] #[test]
+251 -42
View File
@@ -440,8 +440,14 @@ pub struct SetPageSelection {
} }
impl Effect for SetPageSelection { impl Effect for SetPageSelection {
fn apply(&self, app: &mut App) { fn apply(&self, app: &mut App) {
let moved = app.view().page_selection(&self.category) != Some(self.item.as_str());
app.view_mut() app.view_mut()
.set_page_selection(&self.category, &self.item); .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 { fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry) EffectKind::Model(ModelChange::LayoutGeometry)
@@ -744,24 +750,29 @@ impl Effect for SetDrillPendingEdit {
pub struct Save; pub struct Save;
impl Effect for Save { impl Effect for Save {
fn apply(&self, app: &mut App) { fn apply(&self, app: &mut App) {
match save_to_current_path(&*app.storage, &app.model_state) { let outcome = save_to_current_path(&*app.storage, &app.model_state);
Ok(msg) => { if outcome.is_ok() {
app.model_state.dirty = false; 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;
}
} }
report_file_write(app, outcome);
} }
fn kind(&self) -> EffectKind { fn kind(&self) -> EffectKind {
EffectKind::ViewOnly 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 /// Write the workbook to its current file path, returning the status
/// message either way. Missing path and I/O failure are both errors. /// message either way. Missing path and I/O failure are both errors.
fn save_to_current_path( fn save_to_current_path(
@@ -782,19 +793,15 @@ fn save_to_current_path(
pub struct SaveAs(pub PathBuf); pub struct SaveAs(pub PathBuf);
impl Effect for SaveAs { impl Effect for SaveAs {
fn apply(&self, app: &mut App) { fn apply(&self, app: &mut App) {
match crate::persistence::save_to(&*app.storage, &app.model_state.workbook, &self.0) { let outcome =
Ok(()) => { crate::persistence::save_to(&*app.storage, &app.model_state.workbook, &self.0)
app.model_state.file_path = Some(self.0.clone()); .map(|()| format!("Saved to {}", self.0.display()))
app.model_state.dirty = false; .map_err(|e| format!("Save error: {e}"));
app.view_state.status_msg = format!("Saved to {}", self.0.display()); if outcome.is_ok() {
} app.model_state.file_path = Some(self.0.clone());
// The same abort `Save` owes: `SaveAs` is the arm improvise-8tk app.model_state.dirty = false;
// 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;
}
} }
report_file_write(app, outcome);
} }
fn kind(&self) -> EffectKind { fn kind(&self) -> EffectKind {
EffectKind::ViewOnly EffectKind::ViewOnly
@@ -930,19 +937,15 @@ pub struct ExportCsv(pub PathBuf);
impl Effect for ExportCsv { impl Effect for ExportCsv {
fn apply(&self, app: &mut App) { fn apply(&self, app: &mut App) {
let view_name = app.view_name().to_string(); let view_name = app.view_name().to_string();
match crate::persistence::export_csv_to( let outcome = crate::persistence::export_csv_to(
&*app.storage, &*app.storage,
&app.model_state.workbook, &app.model_state.workbook,
&view_name, &view_name,
&self.0, &self.0,
) { )
Ok(()) => { .map(|()| format!("Exported to {}", self.0.display()))
app.view_state.status_msg = format!("Exported to {}", self.0.display()); .map_err(|e| format!("Export error: {e}"));
} report_file_write(app, outcome);
Err(e) => {
app.view_state.status_msg = format!("Export error: {e}");
}
}
} }
fn kind(&self) -> EffectKind { fn kind(&self) -> EffectKind {
EffectKind::ViewOnly 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, /// The positive arm: a `SaveAs` that succeeds must not abort. Without it,
/// "always abort" passes the test above. /// "always abort" passes the test above.
#[test] #[test]
@@ -2327,19 +2357,198 @@ mod tests {
app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
assert!(app.drill_state.is_none()); assert!(app.drill_state.is_none());
assert!( assert!(
app.model_state.dirty, !app.model_state.dirty,
"a drill exited with no edits still marks the document dirty, \ "a drill exited with no edits wrote nothing, so the document is \
because `apply_effects` raises the flag off the declared \ unchanged and `:q` must not be refused (improvise-748f)"
`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."
); );
} }
/// 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). /// A staged edit to a measure column writes that one cell (improvise-c5v).
#[test] #[test]
fn apply_and_clear_drill_with_measure_edit() { fn apply_and_clear_drill_with_measure_edit() {
@@ -197,6 +197,9 @@ fn axis_frame(
collapsed: view.is_group_collapsed(cat_name, group_name), collapsed: view.is_group_collapsed(cat_name, group_name),
})); }));
} }
// Queued like a header, never clearing one: a COLLAPSED group has
// no item to carry it and reaches the frame only through `pending`.
AxisEntry::GroupEnd => pending.push(FrameEntry::GroupEnd),
AxisEntry::DataItem(tuple) => { AxisEntry::DataItem(tuple) => {
let idx = data_idx; let idx = data_idx;
data_idx += 1; data_idx += 1;
@@ -212,6 +215,7 @@ fn axis_frame(
} }
AxisFrame { AxisFrame {
cats: cats.to_vec(), cats: cats.to_vec(),
first: window.start,
entries, entries,
} }
} }
@@ -1234,6 +1238,7 @@ mod tests {
.iter() .iter()
.map(|e| match e { .map(|e| match e {
FrameEntry::Group(g) => format!("[{}]", g.name), FrameEntry::Group(g) => format!("[{}]", g.name),
FrameEntry::GroupEnd => "[/]".to_string(),
FrameEntry::Item(v) => v.join("+"), FrameEntry::Item(v) => v.join("+"),
}) })
.collect() .collect()
@@ -1755,6 +1760,7 @@ mod tests {
let cell = |n: &str| CellDisplay::number(n); let cell = |n: &str| CellDisplay::number(n);
let mirror = GridCache { let mirror = GridCache {
row_frame: AxisFrame { row_frame: AxisFrame {
first: 0,
cats: vec!["Region".into()], cats: vec!["Region".into()],
entries: vec![FrameEntry::Item(vec!["North".into()])], entries: vec![FrameEntry::Item(vec!["North".into()])],
}, },
@@ -1766,6 +1772,7 @@ mod tests {
}; };
let desired = GridCache { let desired = GridCache {
row_frame: AxisFrame { row_frame: AxisFrame {
first: 0,
cats: vec!["Department".into()], cats: vec!["Department".into()],
entries: mirror.row_frame.entries.clone(), entries: mirror.row_frame.entries.clone(),
}, },
+1 -1
View File
@@ -82,7 +82,7 @@ pub fn compute_col_widths(
pub fn compute_row_header_width(layout: &GridLayout) -> u16 { pub fn compute_row_header_width(layout: &GridLayout) -> u16 {
let items = layout.row_items.iter().filter_map(|e| match e { let items = layout.row_items.iter().filter_map(|e| match e {
AxisEntry::DataItem(v) => Some(v), AxisEntry::DataItem(v) => Some(v),
AxisEntry::GroupHeader { .. } => None, AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None,
}); });
header_level_widths(items, layout.row_cats.len().max(1)) header_level_widths(items, layout.row_cats.len().max(1))
.iter() .iter()
+102 -25
View File
@@ -93,6 +93,9 @@ pub enum AxisEntry {
cat_name: String, cat_name: String,
group_name: String, group_name: String,
}, },
/// The run opened by the last `GroupHeader` ends here (improvise-gd83).
/// Nothing is drawn for it; it exists so membership is not inherited.
GroupEnd,
DataItem(Vec<String>), DataItem(Vec<String>),
} }
@@ -101,7 +104,7 @@ pub enum AxisEntry {
fn data_items(items: &[AxisEntry]) -> impl Iterator<Item = &Vec<String>> { fn data_items(items: &[AxisEntry]) -> impl Iterator<Item = &Vec<String>> {
items.iter().filter_map(|e| match e { items.iter().filter_map(|e| match e {
AxisEntry::DataItem(v) => Some(v), AxisEntry::DataItem(v) => Some(v),
AxisEntry::GroupHeader { .. } => None, AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None,
}) })
} }
@@ -134,6 +137,12 @@ fn prune_axis_items(items: &mut Vec<AxisEntry>, keep: impl Fn(usize) -> bool) {
AxisEntry::GroupHeader { .. } => { AxisEntry::GroupHeader { .. } => {
pending_header = Some(entry); pending_header = Some(entry);
} }
// The run it closes may have been pruned away entirely, in which
// case the header is still pending and both are dropped together.
AxisEntry::GroupEnd => match pending_header.take() {
Some(_) => {}
None => kept.push(entry),
},
AxisEntry::DataItem(_) => { AxisEntry::DataItem(_) => {
if keep(data_idx) { if keep(data_idx) {
if let Some(h) = pending_header.take() { if let Some(h) = pending_header.take() {
@@ -529,34 +538,36 @@ impl GridLayout {
/// the Nth data row. /// the Nth data row.
pub fn row_group_for(&self, data_row: usize) -> Option<(String, String)> { pub fn row_group_for(&self, data_row: usize) -> Option<(String, String)> {
let vi = self.data_row_to_visual(data_row)?; let vi = self.data_row_to_visual(data_row)?;
self.row_items[..vi].iter().rev().find_map(|e| { self.row_items[..vi]
if let AxisEntry::GroupHeader { .iter()
cat_name, .rev()
group_name, .find_map(|e| match e {
} = e AxisEntry::GroupHeader {
{ cat_name,
Some((cat_name.clone(), group_name.clone())) group_name,
} else { } => Some(Some((cat_name.clone(), group_name.clone()))),
None AxisEntry::GroupEnd => Some(None),
} AxisEntry::DataItem(_) => None,
}) })
.flatten()
} }
/// The `(cat_name, group_name)` of the nearest `GroupHeader` preceding /// The `(cat_name, group_name)` of the nearest `GroupHeader` preceding
/// the Nth data column. /// the Nth data column.
pub fn col_group_for(&self, data_col: usize) -> Option<(String, String)> { pub fn col_group_for(&self, data_col: usize) -> Option<(String, String)> {
let vi = self.data_col_to_visual(data_col)?; let vi = self.data_col_to_visual(data_col)?;
self.col_items[..vi].iter().rev().find_map(|e| { self.col_items[..vi]
if let AxisEntry::GroupHeader { .iter()
cat_name, .rev()
group_name, .find_map(|e| match e {
} = e AxisEntry::GroupHeader {
{ cat_name,
Some((cat_name.clone(), group_name.clone())) group_name,
} else { } => Some(Some((cat_name.clone(), group_name.clone()))),
None AxisEntry::GroupEnd => Some(None),
} AxisEntry::DataItem(_) => None,
}) })
.flatten()
} }
} }
@@ -585,8 +596,12 @@ fn expand_category(
.get(item_name.as_str()) .get(item_name.as_str())
.and_then(|i| i.group.as_deref()); .and_then(|i| i.group.as_deref());
// Emit a group header at each group boundary. // A header on entering a group; a `GroupEnd` only on leaving one for
// NO group — another group's header already ends the run.
if item_group != last_group { if item_group != last_group {
if last_group.is_some() && item_group.is_none() {
result.push(AxisEntry::GroupEnd);
}
if let Some(g) = item_group { if let Some(g) = item_group {
result.push(AxisEntry::GroupHeader { result.push(AxisEntry::GroupHeader {
cat_name: cat_name.to_string(), cat_name: cat_name.to_string(),
@@ -605,6 +620,11 @@ fn expand_category(
row.push(item_name.to_string()); row.push(item_name.to_string());
result.push(AxisEntry::DataItem(row)); result.push(AxisEntry::DataItem(row));
} }
// The expansion is the end of the run too: the next prefix's items follow
// immediately and must not inherit it (improvise-gd83).
if last_group.is_some() {
result.push(AxisEntry::GroupEnd);
}
result result
} }
@@ -620,10 +640,14 @@ fn cross_product(model: &Model, view: &View, cats: &[String]) -> Vec<AxisEntry>
.into_iter() .into_iter()
.flat_map(|entry| match entry { .flat_map(|entry| match entry {
AxisEntry::DataItem(prefix) => expand_category(model, view, cat_name, prefix), AxisEntry::DataItem(prefix) => expand_category(model, view, cat_name, prefix),
header @ AxisEntry::GroupHeader { .. } => vec![header], marker @ (AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd) => vec![marker],
}) })
.collect(); .collect();
} }
// The axis itself ends the last run; a trailing marker names nothing.
while matches!(result.last(), Some(AxisEntry::GroupEnd)) {
result.pop();
}
result result
} }
@@ -1114,6 +1138,59 @@ mod tests {
assert!(jan.is_none()); assert!(jan.is_none());
} }
/// improvise-gd83: two arms — an ungrouped item AFTER a grouped run, and
/// the same item under a later prefix. context/test-evidence/core.md.
#[test]
fn an_ungrouped_item_after_a_grouped_run_belongs_to_no_group() {
let mut wb = Workbook::new("T");
wb.add_category("Month").unwrap();
wb.add_category("Type").unwrap();
wb.add_item_in_group("Month", "Jan", "Q1").unwrap();
wb.add_item_in_group("Month", "Feb", "Q1").unwrap();
wb.add_item("Month", "Sep").unwrap();
wb.add_item("Type", "Food").unwrap();
let layout = GridLayout::new(&wb.model, wb.active_view());
assert_eq!(
layout.row_group_for(0),
Some(("Month".into(), "Q1".into())),
"precondition: Jan really is in Q1"
);
assert_eq!(
layout.row_group_for(2),
None,
"Sep is in no group — the run ended before it"
);
}
/// The second arm: the run ends at the end of ONE expansion, so the next
/// prefix's first item does not inherit it. context/test-evidence/core.md.
#[test]
fn a_grouped_run_does_not_leak_into_the_next_prefix() {
let mut wb = Workbook::new("T");
wb.add_category("Type").unwrap();
wb.add_category("Recipient").unwrap();
wb.add_item("Type", "Food").unwrap();
wb.add_item("Type", "Clothing").unwrap();
wb.add_item("Recipient", "Carol").unwrap();
wb.add_item_in_group("Recipient", "Alice", "Family")
.unwrap();
wb.add_item_in_group("Recipient", "Bob", "Family").unwrap();
wb.active_view_mut().set_axis("Recipient", Axis::Row);
let layout = GridLayout::new(&wb.model, wb.active_view());
assert_eq!(
layout.row_group_for(2),
Some(("Recipient".into(), "Family".into())),
"precondition: Food/Bob is in Family"
);
assert_eq!(
layout.row_group_for(3),
None,
"Clothing/Carol opens a new prefix and is in no group"
);
}
#[test] #[test]
fn ungrouped_items_produce_no_headers() { fn ungrouped_items_produce_no_headers() {
let wb = two_cat_workbook(); let wb = two_cat_workbook();
+23 -2
View File
@@ -70,13 +70,15 @@ impl View {
/// first slot no regular holds. A virtual is **never** bumped (§7). /// first slot no regular holds. A virtual is **never** bumped (§7).
pub fn on_category_added(&mut self, cat_name: &str) { pub fn on_category_added(&mut self, cat_name: &str) {
if !self.category_axes.contains_key(cat_name) { if !self.category_axes.contains_key(cat_name) {
let axis = if cat_name.starts_with('_') || self.is_records_layout() { let virtual_or_records =
CategoryKind::of_name(cat_name).is_virtual() || self.is_records_layout();
let axis = if virtual_or_records {
Axis::None Axis::None
} else { } else {
let holds_a_regular = |axis: Axis| { let holds_a_regular = |axis: Axis| {
self.categories_on(axis) self.categories_on(axis)
.into_iter() .into_iter()
.any(|c| !c.starts_with('_')) .any(|c| !CategoryKind::of_name(c).is_virtual())
}; };
match (holds_a_regular(Axis::Row), holds_a_regular(Axis::Column)) { match (holds_a_regular(Axis::Row), holds_a_regular(Axis::Column)) {
(false, _) => Axis::Row, (false, _) => Axis::Row,
@@ -292,6 +294,25 @@ mod tests {
); );
} }
/// improvise-bidq: `_Foo` is `CategoryKind::Regular` to every other
/// consumer — see context/test-evidence/core.md.
#[test]
fn a_user_category_whose_name_leads_with_an_underscore_is_seated_as_regular() {
let v = view_with_cats(&["_Foo"]);
assert_eq!(
v.axis_of("_Foo"),
Axis::Row,
"of_name says Regular, so the first slot is Row — not None"
);
let v = view_with_cats(&["_Foo", "Region"]);
assert_eq!(
v.axis_of("Region"),
Axis::Column,
"_Foo holds Row as a regular category, so Region takes Column"
);
}
#[test] #[test]
fn first_category_assigned_to_row() { fn first_category_assigned_to_row() {
let v = view_with_cats(&["Region"]); let v = view_with_cats(&["Region"]);
+62 -11
View File
@@ -163,6 +163,9 @@ pub struct FrameGroup {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub enum FrameEntry { pub enum FrameEntry {
Group(FrameGroup), Group(FrameGroup),
/// The run opened by the last `Group` ends here — `AxisEntry::GroupEnd`
/// carried onto the wire (improvise-gd83). Nothing is drawn for it.
GroupEnd,
/// One label per header level, outermost first. Data indices — the /// One label per header level, outermost first. Data indices — the
/// `row`/`col` of every other field on [`GridCache`] — count only these. /// `row`/`col` of every other field on [`GridCache`] — count only these.
Item(Vec<String>), Item(Vec<String>),
@@ -175,6 +178,10 @@ pub struct AxisFrame {
/// The categories on this axis, outermost first. Empty is a real layout /// The categories on this axis, outermost first. Empty is a real layout
/// (one nameless row or column), not a gap — hence [`AxisFrame::levels`]. /// (one nameless row or column), not a gap — hence [`AxisFrame::levels`].
pub cats: Vec<String>, pub cats: Vec<String>,
/// The **absolute** data index of this frame's first item. Zero means the
/// frame leads the axis, which is what [`AxisFrame::body_from`] asks.
#[serde(default)]
pub first: usize,
pub entries: Vec<FrameEntry>, pub entries: Vec<FrameEntry>,
} }
@@ -184,6 +191,7 @@ impl AxisFrame {
pub fn flat<S: Into<String>>(labels: impl IntoIterator<Item = S>) -> Self { pub fn flat<S: Into<String>>(labels: impl IntoIterator<Item = S>) -> Self {
Self { Self {
cats: Vec::new(), cats: Vec::new(),
first: 0,
entries: labels entries: labels
.into_iter() .into_iter()
.map(|l| FrameEntry::Item(vec![l.into()])) .map(|l| FrameEntry::Item(vec![l.into()]))
@@ -196,7 +204,7 @@ impl AxisFrame {
pub fn items(&self) -> impl Iterator<Item = &Vec<String>> { pub fn items(&self) -> impl Iterator<Item = &Vec<String>> {
self.entries.iter().filter_map(|e| match e { self.entries.iter().filter_map(|e| match e {
FrameEntry::Item(v) => Some(v), FrameEntry::Item(v) => Some(v),
FrameEntry::Group(_) => None, FrameEntry::Group(_) | FrameEntry::GroupEnd => None,
}) })
} }
@@ -293,20 +301,51 @@ impl AxisFrame {
for entry in &self.entries { for entry in &self.entries {
match entry { match entry {
FrameEntry::Group(g) => current = Some(g), FrameEntry::Group(g) => current = Some(g),
FrameEntry::GroupEnd => current = None,
FrameEntry::Item(_) => out.push(current), FrameEntry::Item(_) => out.push(current),
} }
} }
out out
} }
/// The **nearest preceding** group header — not the item's membership, /// The item's group **membership** — the nearest preceding header, unless
/// and deliberately so. **Not for a draw loop**; both points in §3. /// a `GroupEnd` closed it first (improvise-gd83). Not for a draw loop; §3.
pub fn group_of(&self, idx: usize) -> Option<&FrameGroup> { pub fn group_of(&self, idx: usize) -> Option<&FrameGroup> {
let visual = self.visual_of(idx)?; let visual = self.visual_of(idx)?;
self.entries[..visual].iter().rev().find_map(|e| match e { self.entries[..visual]
FrameEntry::Group(g) => Some(g), .iter()
FrameEntry::Item(_) => None, .rev()
}) .find_map(|e| match e {
FrameEntry::Group(g) => Some(Some(g)),
FrameEntry::GroupEnd => Some(None),
FrameEntry::Item(_) => None,
})
.flatten()
}
/// The runs of adjacent positions sharing a group, coalesced by NAME —
/// the coalescing rule's **one home**, both front ends (improvise-8qlr).
pub fn group_runs(&self) -> Vec<(Option<&FrameGroup>, usize)> {
let mut runs: Vec<(Option<&FrameGroup>, usize)> = Vec::new();
for group in self.groups() {
let name = group.map(|g| g.name.as_str());
match runs.last_mut() {
Some((prev, span)) if prev.map(|p| p.name.as_str()) == name => *span += 1,
_ => runs.push((group, 1)),
}
}
runs
}
/// The group label to paint above each position, `None` where the run
/// continues. Derived from [`AxisFrame::group_runs`], never beside it.
pub fn group_run_heads(&self) -> Vec<Option<&FrameGroup>> {
self.group_runs()
.into_iter()
.flat_map(|(group, span)| {
std::iter::once(group).chain(std::iter::repeat_n(None, span - 1))
})
.collect()
} }
/// Position of the data item `idx` within `entries`, group headers /// Position of the data item `idx` within `entries`, group headers
@@ -320,11 +359,16 @@ impl AxisFrame {
.nth(idx) .nth(idx)
} }
/// The entries a renderer walks from data item `idx`, dropping the group /// The entries a renderer walks from data item `idx`. The header above it
/// header above it. The argument is scrolling, not the rule — see §3. /// is dropped, EXCEPT at the top of the axis — §3a, improvise-0o6x.
pub fn body_from(&self, idx: usize) -> &[FrameEntry] { pub fn body_from(&self, idx: usize) -> &[FrameEntry] {
let start = self.visual_of(idx).unwrap_or(self.entries.len()); match self.first + idx == 0 {
&self.entries[start..] true => &self.entries[..],
false => {
let start = self.visual_of(idx).unwrap_or(self.entries.len());
&self.entries[start..]
}
}
} }
/// The width of each header level, in display cells. Derived rather than /// The width of each header level, in display cells. Derived rather than
@@ -921,6 +965,7 @@ mod tests {
CellDisplay::number("3"), CellDisplay::number("3"),
])); ]));
grid.apply(&GridUpdate::RowFrame(AxisFrame { grid.apply(&GridUpdate::RowFrame(AxisFrame {
first: 0,
cats: vec!["Zone".into()], cats: vec!["Zone".into()],
entries: grid.row_frame.entries.clone(), entries: grid.row_frame.entries.clone(),
})); }));
@@ -953,6 +998,7 @@ mod tests {
/// A two-level axis: [A,x], [A,y], [B,z], with `B` in a collapsed group. /// A two-level axis: [A,x], [A,y], [B,z], with `B` in a collapsed group.
fn two_level_frame() -> AxisFrame { fn two_level_frame() -> AxisFrame {
AxisFrame { AxisFrame {
first: 0,
cats: vec!["Type".into(), "Who".into()], cats: vec!["Type".into(), "Who".into()],
entries: vec![ entries: vec![
FrameEntry::Item(vec!["A".into(), "x".into()]), FrameEntry::Item(vec!["A".into(), "x".into()]),
@@ -1034,6 +1080,7 @@ mod tests {
#[test] #[test]
fn header_widths_are_per_level_and_measured_in_display_cells() { fn header_widths_are_per_level_and_measured_in_display_cells() {
let frame = AxisFrame { let frame = AxisFrame {
first: 0,
cats: vec!["A".into(), "B".into()], cats: vec!["A".into(), "B".into()],
entries: vec![FrameEntry::Item(vec!["Clothing".into(), "日本".into()])], entries: vec![FrameEntry::Item(vec!["Clothing".into(), "日本".into()])],
}; };
@@ -1071,6 +1118,7 @@ mod tests {
#[test] #[test]
fn an_axis_with_no_categories_still_has_one_level() { fn an_axis_with_no_categories_still_has_one_level() {
let frame = AxisFrame { let frame = AxisFrame {
first: 0,
cats: Vec::new(), cats: Vec::new(),
entries: vec![FrameEntry::Item(Vec::new())], entries: vec![FrameEntry::Item(Vec::new())],
}; };
@@ -1200,6 +1248,7 @@ mod tests {
})) }))
}; };
let axis = |cat: &str, labels: [&str; 2]| AxisFrame { let axis = |cat: &str, labels: [&str; 2]| AxisFrame {
first: 0,
cats: vec![cat.into()], cats: vec![cat.into()],
entries: labels entries: labels
.iter() .iter()
@@ -1225,6 +1274,7 @@ mod tests {
fn renaming_a_row_category_invalidates_every_cell_under_it() { fn renaming_a_row_category_invalidates_every_cell_under_it() {
let mut grid = identified_grid(); let mut grid = identified_grid();
let renamed = AxisFrame { let renamed = AxisFrame {
first: 0,
cats: vec!["Department".into()], cats: vec!["Department".into()],
entries: grid.row_frame.entries.clone(), entries: grid.row_frame.entries.clone(),
}; };
@@ -1264,6 +1314,7 @@ mod tests {
fn renaming_a_column_category_invalidates_every_cell_under_it() { fn renaming_a_column_category_invalidates_every_cell_under_it() {
let mut grid = identified_grid(); let mut grid = identified_grid();
let renamed = AxisFrame { let renamed = AxisFrame {
first: 0,
cats: vec!["Metric".into()], cats: vec!["Metric".into()],
entries: grid.col_frame.entries.clone(), entries: grid.col_frame.entries.clone(),
}; };
+18 -19
View File
@@ -216,7 +216,10 @@ pub fn render_model(state: &ClientState) -> RenderModel {
.body_from(0) .body_from(0)
.iter() .iter()
.scan(0usize, |data_idx, entry| { .scan(0usize, |data_idx, entry| {
// `GroupEnd` is a boundary, not a row: it draws nothing and
// consumes no data index (improvise-gd83).
let row = match entry { let row = match entry {
FrameEntry::GroupEnd => return Some(None),
FrameEntry::Group(group) => RenderRow::Group { FrameEntry::Group(group) => RenderRow::Group {
label: group.name.clone(), label: group.name.clone(),
collapsed: group.collapsed, collapsed: group.collapsed,
@@ -244,8 +247,9 @@ pub fn render_model(state: &ClientState) -> RenderModel {
} }
} }
}; };
Some(row) Some(Some(row))
}) })
.flatten()
.collect(), .collect(),
categories: entries categories: entries
.iter() .iter()
@@ -443,26 +447,20 @@ fn col_levels(cols: &AxisFrame) -> Vec<Vec<String>> {
/// The column groups as runs, coalescing equal answers from /// The column groups as runs, coalescing equal answers from
/// `AxisFrame::group_of`. Nothing at all when no column is in a group. /// `AxisFrame::group_of`. Nothing at all when no column is in a group.
fn col_groups(cols: &AxisFrame) -> Vec<RenderColGroup> { fn col_groups(cols: &AxisFrame) -> Vec<RenderColGroup> {
let mut runs: Vec<RenderColGroup> = Vec::new(); // The coalescing rule is `AxisFrame::group_runs`, which the TUI draws
let mut any = false; // from too — one home, not two agreeing copies (improvise-8qlr).
for group in cols.groups() { let runs = cols.group_runs();
any |= group.is_some(); let any = runs.iter().any(|(group, _)| group.is_some());
let label = group.map(|g| g.name.clone());
let collapsed = group.is_some_and(|g| g.collapsed);
// Coalesced by NAME alone, as `src/ui/grid.rs` does, and the run's
// indicator is its FIRST group's. See `context/code/clients.md`.
match runs.last_mut() {
Some(run) if run.label == label => run.span += 1,
_ => runs.push(RenderColGroup {
label,
collapsed,
span: 1,
}),
}
}
match any { match any {
true => runs,
false => Vec::new(), false => Vec::new(),
true => runs
.into_iter()
.map(|(group, span)| RenderColGroup {
label: group.map(|g| g.name.clone()),
collapsed: group.is_some_and(|g| g.collapsed),
span,
})
.collect(),
} }
} }
@@ -520,6 +518,7 @@ mod tests {
#[test] #[test]
fn column_groups_coalesce_by_name_as_the_tui_does() { fn column_groups_coalesce_by_name_as_the_tui_does() {
let frame = AxisFrame { let frame = AxisFrame {
first: 0,
cats: vec!["Month".into()], cats: vec!["Month".into()],
entries: vec![ entries: vec![
group("Month", "Q1", false), group("Month", "Q1", false),
+21 -11
View File
@@ -10,6 +10,15 @@ import { test, expect } from "@playwright/test";
/// answers a missing seed with an EMPTY model -- so everything below asserts /// answers a missing seed with an EMPTY model -- so everything below asserts
/// labels the fixture really carries. "A grid is drawn" would pass on the /// labels the fixture really carries. "A grid is drawn" would pass on the
/// empty document a typo in the filename produces. /// empty document a typo in the filename produces.
/// The first row that carries DATA. Not `tbody tr` first: since improvise-0o6x
/// a group header first on its axis is drawn, and `Coastal` is exactly that in
/// `grouped.improv` -- so the first row is a header alone, with no `td` at all.
const firstDataRow = (page) =>
page
.locator("#grid tbody tr")
.filter({ hasNot: page.locator("th.group") })
.first();
async function openGrouped(page, doc) { async function openGrouped(page, doc) {
await page.goto(`/app/index.html?doc=${doc}&seed=grouped.improv`); await page.goto(`/app/index.html?doc=${doc}&seed=grouped.improv`);
await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 });
@@ -20,9 +29,7 @@ async function openGrouped(page, doc) {
// half-drawn, and a one-shot read cannot wait. // half-drawn, and a one-shot read cannot wait.
await expect(page.locator("#grid thead tr")).toHaveCount(3); await expect(page.locator("#grid thead tr")).toHaveCount(3);
await expect(page.locator("#grid thead th.group").first()).toHaveText("▼ Q1"); await expect(page.locator("#grid thead th.group").first()).toHaveText("▼ Q1");
await expect(page.locator("#grid tbody tr").first().locator("th").last()).toHaveText( await expect(firstDataRow(page).locator("th").last()).toHaveText("Main");
"Main",
);
await page.locator("#grid").focus(); await page.locator("#grid").focus();
} }
@@ -58,7 +65,7 @@ test.describe("a grouped, multi-level document", () => {
// Two row categories, so the corner spans two — and the row headers below // Two row categories, so the corner spans two — and the row headers below
// must agree with it, or the table is ragged. // must agree with it, or the table is ragged.
expect(lines.every((l) => l.cornerSpan === 2)).toBe(true); expect(lines.every((l) => l.cornerSpan === 2)).toBe(true);
await expect(page.locator("#grid tbody tr").first().locator("th")).toHaveCount(2); await expect(firstDataRow(page).locator("th")).toHaveCount(2);
// The level lines. The blanks are repeat suppression, decided in Rust by // The level lines. The blanks are repeat suppression, decided in Rust by
// `AxisFrame::header_labels` — NOT an absent label, and not the page's // `AxisFrame::header_labels` — NOT an absent label, and not the page's
@@ -111,7 +118,7 @@ test.describe("a grouped, multi-level document", () => {
// that were merely "big enough" passes the pair above and fails here. // that were merely "big enough" passes the pair above and fails here.
const columns = lines[2].cells.length; const columns = lines[2].cells.length;
expect(groups.reduce((n, c) => n + c.span, 0)).toBe(columns); expect(groups.reduce((n, c) => n + c.span, 0)).toBe(columns);
await expect(page.locator("#grid tbody tr").first().locator("td")).toHaveCount(columns); await expect(firstDataRow(page).locator("td")).toHaveCount(columns);
}); });
// `RenderRow::Group` carries no cells, so the page CANNOT draw one with // `RenderRow::Group` carries no cells, so the page CANNOT draw one with
@@ -125,12 +132,15 @@ test.describe("a grouped, multi-level document", () => {
const groupRows = page.locator("#grid tbody tr", { const groupRows = page.locator("#grid tbody tr", {
has: page.locator("th.group"), has: page.locator("th.group"),
}); });
// Two, not three: `Coastal` is the frame's FIRST entry and a leading group // Three, not two: this WAS two, because `Coastal` is the frame's FIRST
// header is dropped before it reaches the page — improvise-0o6x, open. If // entry and a leading group header was dropped on the way to the page.
// this count becomes 3, that bug is fixed and this line is the record of // improvise-0o6x fixed it, and this line is the record of when.
// when. It is not an endorsement. await expect(groupRows).toHaveCount(3);
await expect(groupRows).toHaveCount(2); await expect(groupRows).toHaveText([
await expect(groupRows).toHaveText(["▼ Inland", "▶ Upland"]); "▼ Coastal",
"▼ Inland",
"▶ Upland",
]);
// The glyph is `indicator(collapsed)`: a page that hard-coded either one // The glyph is `indicator(collapsed)`: a page that hard-coded either one
// passes on one of these rows and fails on the other. // passes on one of these rows and fails on the other.
expect(await groupRows.locator("td").count()).toBe(0); expect(await groupRows.locator("td").count()).toBe(0);
+124 -11
View File
@@ -193,17 +193,28 @@ fn draw_popup_frame(f: &mut Frame, popup: Rect, title: &str, border_color: Color
inner inner
} }
/// Terminal styling for the mode indicator. Its wildcard arm is a known /// Terminal styling for the mode indicator. Exhaustive on purpose, so a new
/// defect (improvise-zx8t) — see `context/code/tui.md`. /// `AppMode` fails the build here (improvise-zx8t) — `context/code/tui.md`.
fn mode_style(mode: &AppMode) -> Style { fn mode_style(mode: &AppMode) -> Style {
match mode { let bg = match mode {
AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => { AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => Color::Green,
Style::default().fg(Color::Black).bg(Color::Green) AppMode::CommandMode { .. } => Color::Yellow,
} AppMode::TileSelect => Color::Magenta,
AppMode::CommandMode { .. } => Style::default().fg(Color::Black).bg(Color::Yellow), AppMode::Normal
AppMode::TileSelect => Style::default().fg(Color::Black).bg(Color::Magenta), | AppMode::RecordsNormal
_ => Style::default().fg(Color::Black).bg(Color::DarkGray), | AppMode::FormulaEdit { .. }
} | AppMode::FormulaPanel
| AppMode::CategoryPanel
| AppMode::CategoryAdd { .. }
| AppMode::ItemAdd { .. }
| AppMode::ViewPanel
| AppMode::ImportWizard
| AppMode::ExportPrompt { .. }
| AppMode::SavePrompt { .. }
| AppMode::Help
| AppMode::Quit => Color::DarkGray,
};
Style::default().fg(Color::Black).bg(bg)
} }
fn draw(f: &mut Frame, app: &App) { fn draw(f: &mut Frame, app: &App) {
@@ -377,6 +388,17 @@ fn draw_tile_bar(f: &mut Frame, area: Rect, app: &App) {
); );
} }
/// The minibuffer row, with any pending status message riding on its right.
/// The minibuffer owns the bottom row and used to hide it — tui.md §3.
fn minibuffer_line(prompt: &str, buf: &str, status_msg: &str, width: u16) -> String {
let left = format!("{prompt}{buf}▌");
let right = match status_msg.is_empty() {
true => String::new(),
false => format!(" {status_msg} "),
};
fill_line(left, &right, width)
}
fn draw_bottom_bar(f: &mut Frame, area: Rect, app: &App) { fn draw_bottom_bar(f: &mut Frame, area: Rect, app: &App) {
if let Some(mb) = app.view_state.mode.minibuffer() { if let Some(mb) = app.view_state.mode.minibuffer() {
let buf = app let buf = app
@@ -385,7 +407,7 @@ fn draw_bottom_bar(f: &mut Frame, area: Rect, app: &App) {
.get(&mb.buffer_key) .get(&mb.buffer_key)
.map(|s| s.as_str()) .map(|s| s.as_str())
.unwrap_or(""); .unwrap_or("");
let text = format!("{}{}▌", mb.prompt, buf); let text = minibuffer_line(&mb.prompt, buf, &app.view_state.status_msg, area.width);
f.render_widget( f.render_widget(
Paragraph::new(text).style( Paragraph::new(text).style(
Style::default() Style::default()
@@ -516,6 +538,97 @@ fn draw_welcome(f: &mut Frame, area: Rect) {
} }
} }
#[cfg(test)]
mod bottom_bar_tests {
use super::{draw_bottom_bar, minibuffer_line};
use crate::ui::app::{App, AppMode};
use crate::workbook::Workbook;
use ratatui::{Terminal, backend::TestBackend, layout::Rect};
/// The bottom row as the terminal really receives it, so the assertion is
/// about `draw_bottom_bar`'s wiring and not about a helper it may not call.
fn bottom_row(app: &App, width: u16) -> String {
let mut terminal = Terminal::new(TestBackend::new(width, 1)).expect("test backend");
terminal
.draw(|f| draw_bottom_bar(f, Rect::new(0, 0, width, 1), app))
.expect("draw");
terminal
.backend()
.buffer()
.content()
.iter()
.map(|c| c.symbol())
.collect()
}
fn app_with_minibuffer_and_message() -> App {
let mut app = App::new(Workbook::new("T"), None);
app.view_state.mode = AppMode::category_add();
let key = app
.view_state
.mode
.minibuffer()
.expect("CategoryAdd carries a MinibufferConfig")
.buffer_key
.clone();
app.view_state.buffers.insert(key, "North".to_string());
app.view_state.status_msg = "category already exists".to_string();
app
}
/// improvise-mai9, and the ONE assertion that can fail for the real defect
/// — see context/test-evidence/tui.md.
#[test]
fn a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open() {
let app = app_with_minibuffer_and_message();
let row = bottom_row(&app, 80);
assert!(
row.contains("North"),
"the minibuffer must still own the left of the row: {row:?}"
);
assert!(
row.contains("category already exists"),
"the status message is never drawn — the bug is in draw_bottom_bar, \
which used to render the minibuffer INSTEAD of the status line, so \
a test that calls the composing helper directly cannot see it: {row:?}"
);
}
/// The minibuffer is still the thing that owns the row: a fix that dropped
/// to `draw_status` would pass the assertion above on the message alone.
#[test]
fn the_minibuffer_prompt_is_not_displaced_by_the_message() {
let app = app_with_minibuffer_and_message();
let row = bottom_row(&app, 80);
let prompt_at = row.find("North").expect("the buffer is drawn");
let message_at = row.find("category already exists").expect("the message");
assert!(
prompt_at < message_at,
"the prompt leads and the message follows it: {row:?}"
);
}
/// The guard's ONLY observable effect, in the narrow band where the two
/// arms differ at all. Why the obvious negative arm cannot: tui.md.
#[test]
fn an_empty_message_never_pushes_a_row_that_fits_past_its_width() {
// Spelled here, NOT taken from `minibuffer_line`: a bound read off the
// function under test moves with the fault and skips the failing band.
let fits_from = "Category: North\u{258c}".chars().count() as u16;
for width in fits_from..fits_from + 24 {
let line = minibuffer_line("Category: ", "North", "", width);
assert!(
line.chars().count() <= width as usize,
"width {width}: an empty status still composed {} columns \
from {line:?} — an unconditional append pads for a message \
that is not there",
line.chars().count()
);
}
}
}
#[cfg(test)] #[cfg(test)]
mod key_conversion_tests { mod key_conversion_tests {
use super::*; use super::*;
+16 -8
View File
@@ -140,20 +140,23 @@ impl<'a> GridWidget<'a> {
format!("{:<width$}", "", width = row_header_width as usize), format!("{:<width$}", "", width = row_header_width as usize),
Style::default(), Style::default(),
); );
let mut prev_group: Option<String> = None; // The run rule's one home, plus the TUI's one divergence from
// it at a scrolled offset — `context/code/protocol.md` §3a.
let run_heads = cols.group_run_heads();
for ci in visible_col_range.clone() { for ci in visible_col_range.clone() {
let x = col_x_at(ci); let x = col_x_at(ci);
if x >= area.x + area.width { if x >= area.x + area.width {
break; break;
} }
let cw = col_w_at(ci) as usize; let cw = col_w_at(ci) as usize;
let col_group = col_groups.get(ci).copied().flatten(); let head = match ci == visible_col_range.start {
let group_name = col_group.map(|g| g.name.clone()); true => col_groups.get(ci).copied().flatten(),
let label = match (group_name != prev_group, col_group) { false => run_heads.get(ci).copied().flatten(),
(true, Some(g)) => format!("{} {}", indicator(g.collapsed), g.name), };
_ => String::new(), let label = match head {
Some(g) => format!("{} {}", indicator(g.collapsed), g.name),
None => String::new(),
}; };
prev_group = group_name;
buf.set_string( buf.set_string(
x, x,
y, y,
@@ -231,6 +234,9 @@ impl<'a> GridWidget<'a> {
break; break;
} }
match entry { match entry {
// A boundary, not a row: nothing is painted and no line is
// consumed (improvise-gd83).
FrameEntry::GroupEnd => continue,
FrameEntry::Group(group) => { FrameEntry::Group(group) => {
let label = format!("{} {}", indicator(group.collapsed), group.name); let label = format!("{} {}", indicator(group.collapsed), group.name);
buf.set_string( buf.set_string(
@@ -895,12 +901,15 @@ mod tests {
assert_eq!(text, GROUPED_FRAME, "\n--- got ---\n{text}\n--- end ---"); assert_eq!(text, GROUPED_FRAME, "\n--- got ---\n{text}\n--- end ---");
} }
/// Re-pinned for improvise-0o6x: the leading `▼ Co…` is the ONE line that
/// moved, and its absence was the bug. context/test-evidence/tui.md.
const GROUPED_FRAME: &str = concat!( const GROUPED_FRAME: &str = concat!(
"┌ View: Default ───────────────────────────┐\n", "┌ View: Default ───────────────────────────┐\n",
"│ [Payer = Alice] │\n", "│ [Payer = Alice] │\n",
"│ ▼ Q1 ▼ Q2 │\n", "│ ▼ Q1 ▼ Q2 │\n",
"│ Jan Apr │\n", "│ Jan Apr │\n",
"│──────────────────────────────────────────│\n", "│──────────────────────────────────────────│\n",
"│▼ Co…────────── │\n",
"│East 100 100 │\n", "│East 100 100 │\n",
"│▼ In…────────── │\n", "│▼ In…────────── │\n",
"│West 50 50 │\n", "│West 50 50 │\n",
@@ -910,7 +919,6 @@ mod tests {
"│ │\n", "│ │\n",
"│ │\n", "│ │\n",
"│ │\n", "│ │\n",
"│ │\n",
"└──────────────────────────────────────────┘", "└──────────────────────────────────────────┘",
); );
} }
+82 -23
View File
@@ -7,7 +7,7 @@ use improvise::ui::grid::GridWidget;
use improvise::view::{Axis, GridLayout}; use improvise::view::{Axis, GridLayout};
use improvise::workbook::Workbook; use improvise::workbook::Workbook;
use improvise_command::ui::project; use improvise_command::ui::project;
use improvise_protocol::{GridCache, Snapshot, ViewState}; use improvise_protocol::{FrameEntry, GridCache, Snapshot, ViewState};
use improvise_wasm_client::render::{RenderModel, RenderRow}; use improvise_wasm_client::render::{RenderModel, RenderRow};
use ratatui::{buffer::Buffer, layout::Rect, widgets::Widget}; use ratatui::{buffer::Buffer, layout::Rect, widgets::Widget};
@@ -278,37 +278,96 @@ fn the_two_front_ends_draw_the_same_column_header() {
.iter() .iter()
.map(|g| (g.label.as_deref(), g.span)) .map(|g| (g.label.as_deref(), g.span))
.collect(); .collect();
// improvise-gd83: MEMBERSHIP, not nearest-preceding. Sep is ungrouped, so
// the run ends before it and resumes under the next measure's Q1 header.
assert_eq!( assert_eq!(
named, named,
vec![(Some("Q1"), 6)], vec![(Some("Q1"), 2), (None, 1), (Some("Q1"), 2), (None, 1)],
"Q1 covers all six columns, INCLUDING the two under the ungrouped \ "the two Q1 runs are separated by the ungrouped Sep; a run that \
Sep — `expand_category` emits a header on entering a group and no \ swallowed it would be `expand_category` leaking a group across its \
boundary on leaving one, so the nearest-preceding walk keeps \ own boundary (see `AxisFrame::group_of`)"
answering Q1 (see `AxisFrame::group_of`). That is `GridLayout`'s \
rule and the TUI paints the same span; it is reproduced here rather \
than corrected, because correcting it changes what the TUI draws. \
See `THE_MEMBERSHIP_CORRECT_SPANS` for what improvise-gd83 makes \
this line say."
); );
// A DELIBERATELY WRONG constant and a live tripwire — do not remove it. assert!(
// Derived, not guessed: see `context/test-evidence/tui.md`. named.iter().any(|(label, _)| label.is_none()),
const THE_MEMBERSHIP_CORRECT_SPANS: [(Option<&str>, usize); 4] = "precondition: the fixture must contain an UNGROUPED column, or this \
[(Some("Q1"), 2), (None, 1), (Some("Q1"), 2), (None, 1)]; test cannot see a leaked group at all"
assert_ne!(
named.as_slice(),
THE_MEMBERSHIP_CORRECT_SPANS.as_slice(),
"when this stops differing, improvise-gd83 is fixed and the \
assertion above should become the constant"
); );
// And the group line the TUI paints names the same group. // improvise-8qlr: the TUI's group line, compared EXACTLY. A `contains`
// passes while the TUI labels every column, which is a different grid.
let group_line = &lines[rule - levels - 1]; let group_line = &lines[rule - levels - 1];
assert!( let expected: Vec<String> = model
group_line.contains("▼ Q1"), .col_groups
"the TUI paints the same group above the same columns: {group_line:?}" .iter()
.flat_map(|g| {
let head = match &g.label {
Some(name) => format!("{} {name}", indicator(g.collapsed)),
None => String::new(),
};
std::iter::once(head).chain(std::iter::repeat_n(String::new(), g.span - 1))
})
.collect();
assert_eq!(
&tui_header_cells(group_line, &cache),
&expected,
"the TUI's group line and the browser's runs disagree — one of them \
coalesces the runs itself instead of asking `group_runs`"
); );
} }
/// improvise-0o6x: a group FIRST on its axis. Separate from `two_level_rows`,
/// which orders Carol first to dodge this case on purpose.
fn leading_group_rows() -> Workbook {
let mut m = Workbook::new("Test");
m.add_category("Recipient").unwrap();
m.add_item_in_group("Recipient", "Alice", "Fam").unwrap();
m.add_item_in_group("Recipient", "Bob", "Fam").unwrap();
m.add_item("Recipient", "Carol").unwrap();
m.add_item("_Measure", "Revenue").unwrap();
m.active_view_mut().set_axis("Recipient", Axis::Row);
m.active_view_mut().set_axis("_Measure", Axis::Column);
for r in ["Alice", "Bob", "Carol"] {
m.model.set_cell(
coord(&[("Recipient", r), ("_Measure", "Revenue")]),
CellValue::Number(1.0),
);
}
m
}
/// A collapsed group at position zero is otherwise a trap: its rows are hidden
/// and its header is the only affordance. context/test-evidence/tui.md.
#[test]
fn a_group_header_first_on_its_axis_reaches_both_front_ends() {
for collapse in [false, true] {
let mut wb = leading_group_rows();
if collapse {
wb.active_view_mut()
.toggle_group_collapse("Recipient", "Fam");
}
let (_app, cache) = whole_grid(&mut wb);
assert!(
matches!(cache.row_frame.entries.first(), Some(FrameEntry::Group(g)) if g.name == "Fam"),
"precondition (collapse={collapse}): the frame must LEAD with the \
header, or body_from has nothing at position zero to drop"
);
let browser = browser_rows(&cache);
assert!(
matches!(browser.first(), Some(RenderRow::Group { label, .. }) if label == "Fam"),
"the browser dropped the leading group header: {browser:?}"
);
let painted = tui_row_headers(&wb, &cache);
assert_eq!(
painted.first().map(|r| r.join("").trim().to_string()),
Some(format!("{} Fam", indicator(collapse))),
"the TUI dropped the leading group header: {painted:?}"
);
}
}
/// The bug's own words, separate because the agreement above would also hold /// The bug's own words, separate because the agreement above would also hold
/// if *both* ends started slash-joining. /// if *both* ends started slash-joining.
#[test] #[test]