diff --git a/context/code/clients.md b/context/code/clients.md index 3b40b0be..9d915e86 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -520,17 +520,109 @@ looks wrong and is exactly what the TUI does; mirroring it is the point. The re-entry is `reopened_at`, and it declines where the host's `EnterEditAtCursor` declines (improvise-enz6). The host asks `core::uneditable` of its layout. The browser asks the landing cell's projected -`CellId`, which is the same answer, because `cell_id` is what fills it. Two -limits follow from asking the cache: +`CellId`, which is the same answer, because `cell_id` is what fills it. Asking +the cache has two consequences: - **The wording differs on a fold.** The host can tell a fold from any other aggregate and names the fold. The browser sees only `CellId::Aggregate`, so it gives the host's generic aggregate text. `lockstep.rs` compares the mode, the cursor and the buffers here, never the status text. -- **A landing past the window opens anyway.** The cache holds only the - announced window, so a `Down` from the window's last row lands on a cell with - no projected id. The browser then opens the editor, where the host does not. - No write can follow it: once the host has projected that cell, - `record_at_cursor` finds no record there and the commit composes nothing. +- **A landing past the window is decided when the host answers** + (improvise-5m9z). The cache holds only the announced window, so a `Down` from + the window's last row lands on a cell with no projected id yet. `reopened_at` + opens the editor there provisionally, and the fold settles it: + `ClientState::settle_declined_editor` runs after every fold. It closes an + **untouched** editor whose cursor cell the host has now projected as one `i` + declines, and says why. The client then ends where the TUI does. Both ends + ask the same `resolve::declined_at`, so the immediate decline and the late + one cannot disagree. + + The decline waits for the host's answer, as #227's awaiting seed does. It does + not choose a seed, which stays `seed_at`'s and #227/#272's. + + **A touched editor is kept.** If the user typed before the answer came, the + editor stays open with their text. Closing it would throw input away. Its + commit is then refused out loud (improvise-ik6l): `refused_here` declines the + `commit-cell-edit` or `commit-and-advance-right` **step**, which takes the + whole binding with it. That includes the welded `clear-buffer`, as the TUI's + failed `committable_cell_key` does, so the status is said and the mode and + the typed text stay. Before this fix the commit composed nothing, and Enter + went `Upstream`, which no host answers: a silent dead key. + + **`declined_at` reads the cache through the frame's own origin** + (`AxisFrame::first`), never the cursor's offsets (improvise-y5eb). The settle + also runs when the client folds its **own** outgoing frame. By then the + offsets have moved but the cache still holds the old window. Indexing it with + the new offsets read the wrong cell. A `Down` that wraps from the bottom row + to the next column's top read the old window's top row, and closed an editor + on Revenue/Feb with Profit's formula refusal. Read against the frame's + origin, a stale cache answers `None` ("not projected yet"), and the host's + own reply decides. The earlier version of this section called that aliasing + unreachable. It is reachable, and the wrap is the case that reaches it. + + The settle is not specific to advancing. An untouched editor also closes + when the host re-projects its cell as declined for any other reason, such as + another client adding a formula over it. + + **Every cache read goes through the frame's origin, not only + `declined_at`** (improvise-7mro). `cell_at_cursor` and `seed_at` read through + `projected_at` too, and the cursor-offset read `cell_at` is gone. Before + this, every other read had the y5eb aliasing. A client folds its own scroll + at once, but its cache holds the old window until the host answers, so a key + resolved in that gap read another cell. Pipelined `$ i 7 Enter` in a + one-column window wrote **Jan/Revenue** where the TUI wrote Feb/Revenue: the + seed and the write's target both came from the cell at the old window's + index. `G i` and `gg i` refused an editable cell with a formula's refusal. + + The aliasing had a second form, inside the held window: after `k k j` the + cursor is on a cell the cache holds, but the offsets no longer match the + frame's origin, so index arithmetic through the offsets named the next row. + Reading at the frame's origin answers both forms with one rule. + + **A cell of the grid outside the held window is `unprojected`**: unknown, + never "the cell at the same index". Inside the grid's extent but outside + either frame's range. An empty grid has nothing to wait for, so it is not + unprojected. What each key does on such a cell: + - **`i` opens an editor awaiting its seed** (`OpenedEditor::awaiting()`). + The fold settles it once the host projects that cell + (`settle_awaited_seed`), and never over typing. The settle waits for a cell + with an **id**. Between a frame and its cells, a relabel leaves a blank + `CellDisplay` at the position, and settling on that seeded `""` before the + cell arrived. A declined cell closes through `settle_declined_editor`, + which runs first. + - **A commit, `x`, `yy` and `p` wait out loud.** `refused_here` declines the + step with "waiting for the host to show this cell", which keeps the mode + and the typed text, as ik6l's refusal does. Nothing is composed: the write + target is unknown, and guessing it is the bug. Pressing the key again after + the host answers does the write. This is the Codex finding on #282. + - **An advance's reopen keeps #282's shape.** It reads `seed_at`, which is + `""` for an unprojected landing. On this base the keymap's welded + `clear-buffer` empties the reopened buffer anyway, in the TUI too, so an + awaited seed there would be written into a buffer the TUI has cleared. + + **What a held reply still costs.** With every reply held, the browser cannot + know a seed the TUI shows, so `G i 7` types `7` into an empty editor, where + the TUI types `47`. The write lands on the TUI's record once the host has + answered, never on another. Matching the TUI exactly would need the client + to defer keys until the projection arrives. That means `on_frame` handing + queued frames back to `web/app.js`, which is a change to the page's surface + and is not made here. + + **Merging with #227/#272 and #274.** #272 adds the same `OpenedEditor::awaiting` + field and an `awaiting_seed` for the editor an advance reopens after a write, + settled once no write is unanswered. Merged, the settle must require both: + no write unanswered, and the cell projected (`projected_seed`). With #227's + commit-owned clear, the reopen then awaits for an unprojected landing too. + #274 captures the committed cell in `settle_opened` through + `opened_at_cursor`, which reads `cell_at_cursor`. Without this fix that + capture aliases as well: a horizontal wrap with replies **delivered** writes + Jan/Revenue on #274's head. With `cell_at_cursor` reading at the origin, the + capture waits for the projected cell, because `settle_opened` retries on + every fold. + + **When `CellId::NonItem` lands (#265), both `declined_at` and + `core::uneditable` need an arm for it.** Both matches are exhaustive, so the + compiler will ask. The trap is answering in the `None` arm: a non-item cell + would then open an editor whose commit writes a ghost record. `GridMove` is a hand-spelled mirror of `navigation.rs`'s `MoveKind`, because the command crate is not on a browser's dependency list and never will be. The @@ -781,7 +873,9 @@ fixture standing on one of them cannot see the other: even open an editor. Nothing happened and nothing was said. The ticket did not predict this one, and it is not a second bug: it is improvise-fdk1 seen from the key path, since a coordinate rename has no wire form precisely because the - `RecordHash` surrogate names records that already exist. + `RecordHash` surrogate names records that already exist. **fdk1 has since + closed it**: the coordinate is now the one records cell the browser edits. See + "A coordinate rename" below. So the guard goes at the **entrance** — the first statement of `edit_or_drill`, before the cell is even looked at — and not on the `CellId::Record` arm where @@ -3025,3 +3119,228 @@ files and refuses one that names the bundle directory, and its companion asserts `web/tests/bundle/` still holds a test that *does* — a scoping rule is uninteresting if nothing falls outside it. Evidence in `context/test-evidence/tooling.md`. + +## A coordinate rename (improvise-fdk1) + +The coordinate is the one records-mode cell the browser edits. `edit_or_drill` +still declines a records grid, except on a `CellId::Coordinate`. There it opens +the editor on the coordinate's `edit_text`, just as the host's `EditOrDrill` +does. `commit` sends a records grid to `rename_coordinate`, which composes +`ModelCommand::RenameRecordCoordinate` plus the mode change and the advance +that `CommitAndAdvance` makes. Every other records cell is still declined out +loud, and `the_tile_bar_route_into_records_mode_does_not_strand_the_user` walks +the whole row to check that both answers hold. + +**The target is captured when the editor opens, not when it commits.** The +record, the category and the record's write counter are captured into +`ClientState::opened` (session-local, never serialized). The commit renames that +record, and its basis claims that counter through `BasisCache::claim_as_read`, +the same door the inline editor's `EditTarget` uses (improvise-0j0f). So if +another client writes the record while the editor is open, the rename is refused +with `RecordMoved`, and the other client's value survives. + +**The capture waits for the host's answer, and that wait is the part easy to +get wrong.** A commit re-enters editing one row down in the same batch, and the +client folds that batch optimistically. At that moment its grid cache still +shows the rows as they were **before** its own rename. Records rows are sorted +by coordinate, so a rename moves its row, and the row now under the cursor +belongs to a different record on the host. So `settle_opened` captures only when +no awaited frame is in flight. It runs after every fold and after `accepted()`. +The host sends `Accepted` last, after the projections that describe the answer, +so the capture reads the refreshed cache. A commit made before any capture +exists (typing faster than the host answers) is declined out loud with "still +waiting". + +**After a rename, the browser's cursor goes to the origin, not one row down.** +The rename reframes the grid, so the host broadcasts `ResetCursor` to every +client, the asker included (gotcha 24). The TUI's key path stays one row down, +because `apply_effects` observes the reframe and `CommitAndAdvance` restores the +cursor afterwards. `tests/lockstep.rs` cannot see the difference: its twin host +has no `Sessions`, so nothing broadcasts. The difference predates this ticket +and applies to any browser write that reframes the grid. + +**An empty coordinate writes nothing, on both ends.** The host's key path +refuses it (`RecordRewrite` sets `refused`, and `App::refuse` does not abort), +says `RECORD_COORDS_CANNOT_BE_EMPTY`, and still advances. Composing the rename +would instead have the host undo the whole frame (`MutationRefused`) and show +the generic sentence. So the client composes the host's answer itself: the same +status, taken from the protocol's one copy of the message, plus the advance and +no write. + +**A coordinate editor opens the way a value editor does, and an untouched one +writes nothing.** `edit_or_drill` opens both through `opened_on`, so both record +`SetOpenedEditor` with the seed, as the host's `EditOrDrill` already did for +every cell it opens. `rename_coordinate` then applies improvise-ixxx's +`untouched` rule before anything else: `i` then `Enter` composes the mode change +and the advance and no rename, which is what the host's `CommitAndAdvance` does. +The untouched check comes before the "still waiting" decline and the `Gone` +decline below. An untouched `Enter` writes nothing either way, so it needs no +basis capture and cannot retarget. + +**The capture carries the document generation it was read in (improvise-vndi),** +and the rename claims it through `claim_as_read`, as the inline `EditTarget` +does. It cannot be observed: a document swap's rebase `Snapshot` closes the +rename editor and `settle_opened` drops the capture, so no rename crosses a +generation. `a_document_swap_closes_an_open_rename_editor` pins that +precondition. + +**Two captures, one editor.** When an editor opens, two things are recorded, and +they answer different questions: + +- `ViewState::opened_editor` (`OpenedEditor { seed, touched }`) answers **what + text it opened on, and whether the user has changed it**. It is portable + state. It is composed optimistically at the keypress and folded the same way + on the host, where `CommitAndAdvance` reads it. +- `ClientState::opened` (`Opened::Target(OpenedCell { record, target, version, + seen, document_generation })`, or `Opened::Retry`/`Opened::Gone` after a + refused commit, below) answers **which cell the commit writes, and which + record versions and document its basis claims**. `target` is + `Coordinate { category }` for a rename and `Value { measure }` for a stored + value; `seen` is filled for a rename only. It is session-local and never + serialized. + +Neither answers "is an editor open": the mode does, for both. `opened_editor` is +not cleared when the editor closes, only replaced when the next one opens. + +The record capture cannot move into `OpenedEditor`, and the reason is the +capture rule above. `settle_opened` waits for the host's answer, because a +capture read from the optimistic cache names the row as it stood before this +client's own rename. `SetOpenedEditor` is composed at exactly that optimistic +moment, so an `OpenedEditor` carrying a `RecordHash` would reintroduce the bug +the `capture_unanswered` rival demonstrates. It would also put a client-only +basis read into state the host folds and `tests/lockstep.rs` compares, where +the host's value would have no record in it. Gating the capture on +`opened_editor.is_some()` was considered and rejected: an editor entered by +`enter-mode` records no opening, yet the host still commits a rename from it, +so the gate would strand that path on "still waiting". + +**A retry after a refusal may write only the cell it named (improvise-20ek, +improvise-9ww8, improvise-8phw).** A refusal replays the view slice, so the +editor reopens with the typed text over whatever cell the cursor now shows. The +rule is about that cell, and it holds for a rename and for a value alike: + +- `refused()` pins `ClientState::opened` to `Opened::Retry(record, target)`, + naming the cell the refused frame wrote: a rename's record and category, or a + `SetCell`/`ClearCell`'s record and measure (`written_to`). The refusal's kind + plays no part. +- Once the host has answered, `settle_opened` captures what is under the cursor. + If that is the same record **and** the same `OpenedTarget`, it becomes the + target at its **current** version, so a second `Enter` goes through. That is + the one-keystroke force `RecordMoved` promises (gotcha 43), and the user has + just been told someone else changed the row. +- If it is anything else, or nothing, `opened` becomes `Opened::Gone`, and + `Enter` answers with a status and no write. Esc leaves the editor, and opening + it again captures afresh. + +The whole target is compared, not only the record, because a record is a row +and a value editor names a cell in it: a crossing `HideItem` on a measure leaves +the **same** record under the cursor in another column, and a record-only +comparison forces the retry onto a measure the user never typed for. `seen` +stays rename-only, because only a rename has a destination to claim. + +**Why the rule names the record rather than a rejection.** The first version +(20ek) pinned only on `RecordGone` of the renamed record. But `stale_basis` +checks the layout tier before the record tier, and another client's rename +bumps `layout_version`. So when A's `Enter` crosses B's rename in flight, which +takes nothing more than ordinary latency, the refusal is `LayoutMoved`, not +`RecordGone`. The retry then renamed whatever record had moved under the +cursor. That happened both when B had renamed A's own record, and when B's +rename merely re-sorted the rows so that a different, still-living record sat +under the cursor. So "is the source gone" is not the discriminator either. The +only question that separates a force from a retarget is whether the capture is +the record the user typed for. With that rule the explicit `RecordGone` pin is +subsumed: after `RecordGone(r)` the capture can never be `r`. It was removed +once a rival that dropped it failed no test. The reviewer's rival on PR #221, +which kept the opened target through every refusal, still breaks the reorder +case: after a *successful* rename, the next editor must capture the row it now +shows. + +**The destination is a record too (improvise-milr).** A rename onto an existing +coordinate overwrites that record (improvise-kndq), so it is a write to the +destination as well, and the destination has to answer to the basis. The +server decides that (`records_at_stake`, `context/code/command.md`). The client +cannot name the destination: it would need the hash of coordinates it has never +been sent, which ADR 002 forbids. So it claims **every record counter it held +when the editor opened**, captured into `OpenedCell::seen` at the same +moment as the target. Two consequences follow: + +- If another client writes the destination while the editor is open, the rename + is refused with `RecordMoved`. This holds even when the write has already + reached this client's cache, because the claim is the version at open time. +- If the destination is a record this client was never shown (outside its + window, say), the rename is refused with `RecordUnstated`. Overwriting a + record you have not seen is exactly the lost update the check exists to stop. + +**A value editor commits to the cell it opened on, too (improvise-8xq0).** The +keyboard commit over a plain grid cell used to read the cursor at `Enter`: the +record from `record_at_cursor`, the measure from the `CellId` under it, and the +basis from `claim`, which reads the version as it is *now*. Another client's +`HideItem` renumbers the rows under an open editor, the cursor then names a +different cell, and the write landed there with the text typed for the first. +The inline `EditTarget` fixed this for the mouse editor (improvise-2wu3), but it +lives in the page, so the keyboard path never saw it. The fix reuses the rename +capture rather than adding a second one: `settle_opened` captures a `Record` +cell as `OpenedTarget::Value`, and `written` builds the `RecordCell` and the +`claim_as_read` basis from it. Nothing about the write is read at `Enter` except +the text. **The capture reads the cell through the frame's own origin (improvise-7mro).** +`captured()` asks `opened_at_cursor`, which asks `cell_at_cursor`, and since +#286 that reads through `projected_at` (`AxisFrame::first`), never through the +cursor's offsets. That matters more here than for any other reader, because the +capture *sticks*: `settle_opened` takes it as soon as no awaited write is in +flight, which includes the fold of the client's own outgoing frame. An untouched +`Enter` that scrolls moves the offsets before the host has sent the new window, +so reading through the offsets captured the old window's cell at the same cache +index (Jan/Revenue where the TUI writes Feb/Revenue, with every reply delivered), +and every later `Enter` wrote it. Read through the origin, a cell outside the +held window is `None`, `settle_opened` simply tries again on the next fold, and +the capture lands once the host projects the cell. #274 took #286 by merge +rather than porting the reader; the PR body says why. + +**A captured commit is not judged by the cursor's cell either (improvise-3h9w).** +#286's `refused_here` declines a commit key whose cursor cell `i` would decline, +and waits on one that is unprojected. It runs in `step()` before `commit()`, +so it must skip an editor that captured its target: that commit writes, or +basis-refuses, the captured cell, whatever now sits under the cursor. The guard +is kept for an editor that captured nothing, which is the case it was written +for. + +**What the commit is follows the capture too.** Whether `Enter` writes a value +or renames a coordinate is read off `opened.target()`, not off the cell under +the cursor (ChatGPT Codex on PR #274): a relayed reframe can leave the cursor on +no cell, on a creating column, or on a records grid, and dispatching on that +sent a captured value editor's `Enter` upstream or down the wrong path. Only an +editor that captured nothing (`uncaptured`) is dispatched by the cursor, and +that is the creating columns plus the waiting cases. A records grid's measure +cell is deliberately not captured as a value, since the browser still declines +editing it (gotcha 1); `every_advertised_command_answers_in_its_own_mode` +fails if it is. + +Two things come with it. **The version is the one read at open**, so a write by +another client while the editor is open is refused `RecordMoved` on this path +as well: improvise-0j0f's rule, which the keyboard path had not had +(improvise-xz2z). And **a +value commit can now wait**: the capture waits for the host's answer to this +client's own last write, so `Enter` pressed again inside one round trip has no +target yet. It declines with the rename's "still waiting" status and writes +nothing. With no write in flight and no capture, the cursor is on no cell an +editor writes, and the commit answers `None` as before. + +**A waiting commit is a decline, so it keeps the typed text (improvise-joed).** +`Enter` in `Editing` is the sequence `[commit-cell-edit, clear-buffer edit]`, +and a sequence runs every step unless one is `Step::Declined`. The waiting +answer used to come back from `composed` as an ordinary `Local` step, so the +welded clear ran after it and emptied the editor it had just left open. The two +cell commits are therefore answered by `committed`, ahead of `composed`, and +`still_waiting` is a `Step::Declined`. That is the same channel `declined` uses +for commands the browser cannot compose, and the same rule gotcha 21 gives an +abort: what follows a refusal in one binding does not run. #251's other +decline, a retry whose cell is `Opened::Gone`, keeps its text the same way: +`renamed_away()` is a `Step::Declined` too, and it serves both the value and the +rename path. + +This also fits improvise-fy9o (PR #227), which drops the weld and has the +commit clear its own buffer at its head. There the binding is one command, so +`Declined` and `Local` answer the same, and the text survives only if the +waiting return comes **before** that clear. A merge of the two must keep that +order. The joed tests fail if it does not. + diff --git a/context/code/command.md b/context/code/command.md index 97bad13f..e574be86 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -368,6 +368,36 @@ Only `CellData` variants that carry a `RecordCell` name a record. The item and formula commands are `CellData` too but name no *record*: they move the axes a record's coordinates are drawn from, which is what the layout tier is for. +### `records_at_stake` — `records_named` plus a rename's destination + +`ui/reduce.rs::records_at_stake(app, cmd)` is what `stale_basis`, +`stale_records_in_a_frame` and `caught_up` ask. It is `records_named(cmd)`, plus +one extra record for `RenameRecordCoordinate`: the **destination**, meaning the +source's coordinates with the category replaced by the new item. That record is +added only when it exists in the document and differs from the source +(improvise-milr). A rename onto an existing record overwrites it +(improvise-kndq), so without this a write another client made to the +destination was lost without any check. + +**Why not in `records_named` itself,** which is what the ticket asked for. +`records_named` is pure and lives in `improvise-protocol`: `&ModelCommand -> +Vec`, with no document. The destination is not in the command. +Deriving it needs the source's coordinates, which only the document holds, +and "when it exists" is also a question about the document. Moving +`records_named` behind an `&App` would take away what makes it useful to the +client. `BasisCache::claim` and `caught_up` rely on it being pure, so a client +can claim exactly what the server will check. So the protocol keeps the +command's own records, and the server adds what only the document can say. + +The price is on the client side. It cannot compute the destination's hash (ADR +002: a client never computes a surrogate), so it claims every record counter it +held when the editor opened (`context/code/clients.md`, "A coordinate rename"). +An existing destination the client was never shown is refused with +`RecordUnstated`. That is a refusal, not an overwrite, and it is the right +direction to err. `broken_chain` still asks `records_named`: whether a later +command's record was renamed away by an earlier one in the frame is a question +about the source only. + ### `stale_basis` `None` means apply it. The three tiers are checked in order of how much they @@ -1109,6 +1139,32 @@ A blank coordinate edit is a different kind of refusal. It is refused **alone**, and the row's other edits still land, as before: that edit is invalid input, not a missing structure. +**The wire half: `effect::RenameRecordCoordinate` (improvise-fdk1).** One effect +serves both doors. `model_effects` maps `ModelCommand::RenameRecordCoordinate` to +it, after resolving the surrogate. The plain records key path emits it for a +category column. It reads the record's row off the store at apply time +(`view::record_row`, which groups the same way the records layout does) and +applies `RecordRewrite::of` with the one edit. The key path used to call +`RecordRewrite::of` itself, with the layout's row. In plain records mode that +row is the store's, so behaviour is unchanged. What changes is that the key path +and the wire now take one door, so a host and a browser renaming the same record +cannot drift. The name matches the variant, as +`every_model_command_names_the_effect_of_the_same_name` requires. The collision +rule is kept (improvise-kndq): a rename onto an existing record still +overwrites it, on both paths. + +**What `[RenameRecordCoordinate(r), SetCell(r)]` means depends on the host's +view, and that is filed, not fixed (improvise-8id2).** A surrogate names +coordinates, not an object. After the rename, `r` names the vacated coordinates. +On a **records** host the layout names positions, so `resolve_record` has no +fallback, `broken_chain` answers `ChainBroken(r)`, and clause 4 undoes the +rename too. That case is pinned by +`a_write_to_a_record_its_own_frame_renamed_is_a_broken_chain`. On a **pivot** +host the empty cell is still drawn, so `record_in_layout` resolves `r` and the +write re-creates a record at the old coordinates: measured, not pinned. The +shipped browser composes one rename per frame, so only a hand-built client sends +this pair. It is the same fallback as improvise-ciw8. + ### `Save` / `SaveAs` abort the batch (improvise-8tk / improvise-lae) A failed save aborts the rest of the batch, so the `Quit` that `wq`/`ZZ` chains diff --git a/context/code/protocol.md b/context/code/protocol.md index d4ddc61c..56d421a4 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -178,12 +178,14 @@ attempt in a fourth place is the disease. - The **model half** is `ModelCommand`: every `EffectKind::Model(..)` effect, one variant apiece, carrying that effect's own fields (improvise-kmgs). -The count is 27 model effects, minus two: `ApplyAndClearDrill` and +The count is 28 model effects, minus two: `ApplyAndClearDrill` and `AcceptWizardModel`, which are **not addressable** (improvise-3x3u). That -leaves **25**. `SwitchView`, `ViewBack` and `ViewForward` are no longer model -effects at all (improvise-jytm), so they are not subtracted. This paragraph -said "29 minus three, 26" until improvise-km71 added `ReplaceFormula`, and -that figure was already stale: the enum had 24. A variant with +leaves **26**: 24, plus `ReplaceFormula` (improvise-km71) and +`RenameRecordCoordinate` (improvise-fdk1). `SwitchView`, `ViewBack` and +`ViewForward` are no longer model effects at all (improvise-jytm), so they are +not subtracted. This paragraph said "29 minus three, 26" while the enum had 24, +and each of those two PRs then corrected it to 25 without seeing the other. +Count by brace-matching the `impl Effect` blocks, as below. A variant with no data mirrors a nullary effect whose operand is document state the authoritative `App` already holds. @@ -620,8 +622,8 @@ it, and the address it needs is the name. What was missing was therefore never an address but a **recognition**. `Create` carries the kind and nothing else, because the kind plus the typed name is the whole operand. It is not a `SetCell` target, so `render.rs::editable` stays -`Record`-only and `record_at_cursor` still answers `None` — both by matching, -which the compiler now forces. +`Record`-only and `opened_at_cursor` captures no editor target for it — both +by matching, which the compiler now forces. **No client composes it yet**, and that is one gap rather than this one: `keys.rs::commit` returns early for every records-mode commit, because a @@ -641,19 +643,33 @@ belongs to the host. So absence was encoding a permission, the same conflation `a_coordinate_column_is_projected_with_no_identity`, asserted the bug and is gone. -`Coordinate` carries **nothing**. Editing a coordinate renames it across every -measure cell in the row (gotcha 1), and no client composes that operation: it -needs `record_col`, and the browser declines every records-mode edit before -reaching a cell. Giving the variant a `RecordHash` would hand a client a -surrogate with nothing to spend it on. The operand belongs with the command -that consumes it, whenever that command exists. Nothing a client can do has -changed. `render.rs::editable` is now an exhaustive `match` rather than a -`matches!`, so the next variant has to be classified. `typed_cell_command`, -`record_at_cursor` and `record_cell_at_cursor` still answer `None` for it. -So paste onto a coordinate is still a silent `Upstream` in the browser. The -TUI has declined it out loud since PR #184. That divergence is -improvise-kkcy, and a named kind is what lets the browser decline deliberately -now. +**Since improvise-fdk1 it carries `{ record, category }`**, the operands of +`ModelCommand::RenameRecordCoordinate`. Editing a coordinate renames it across +every measure cell in the row (gotcha 1). When exdg landed, no client composed +that operation, so the variant was left empty on the grounds that the operand +belongs with the command that consumes it. That command now exists, and both +operands come from the **projection**: + +- **`record` is the row's own record.** A client could borrow the hash from a + sibling `Record` cell in the same row. That would be a second derivation of + which cells share a record, which is what ADR 002 forbids. It would also have + nothing to borrow in a document with no measures, where the coordinate is + the only cell a row has. For the same reason `published_basis` counts a + `Coordinate`'s record, or a rename of such a row would be `RecordUnstated`. +- **`category` is the column's identity.** Its label is not: a category and a + measure may share a name (gotcha 1), so reading the category off the column + label would be a guess. + +`RenameRecordCoordinate` is position-free (improvise-hkbc): it names the record +by surrogate and the new coordinate by value. `records_named` names the record, +because a rename moves it (gotcha 43). The destination, when it exists, is +added by the server's `records_at_stake` rather than here, because only the +document can derive it (improvise-milr, `context/code/command.md`). Creating the item is internal to the +effect. `render.rs::editable` still answers `false` for a coordinate: the page's +inline editor writes stored cells only, and the rename's producer is the +keyboard path (`context/code/clients.md`). `typed_cell_command` and +`record_cell_at_cursor` still answer `None` for it, so paste onto a coordinate +is still improvise-kkcy. ### `CellDisplay::id` — `None` is not "aggregate" diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 5112e6e6..49eb6f28 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4890,9 +4890,104 @@ Rivals: - The host opening everywhere: fails, the client now declining where the host opens. -One limit is not covered, because the window is 6×6 and every landing is -inside it. A landing past the announced window has no projected id, so the -browser still opens the editor there (`clients.md`). +The window is 6×6, so every landing here is inside it. A landing past the +announced window is improvise-5m9z, covered by +`advancing_the_editor_past_the_window_settles_where_the_tui_does`. + +### advancing_the_editor_past_the_window_settles_where_the_tui_does + +improvise-5m9z, filed from PR #277. `reopened_at` can decline only a cell +whose id the client holds. An advance past the announced window lands on one it +does not hold yet, so the browser opened the editor where the TUI declines. + +This test runs a real `Host` and a real `ClientState`. Each key is resolved on +the client, folded with `fold_outgoing`, sent through `Host::handle`, and the +whole reply is delivered. A TUI `App` presses the same keys through +`reduce_key`. Three cases, each landing one step past the window: +- `fold_host`, 2×6 window: `i` on Feb (1,1), then `Enter` onto the Q2 fold at + row 2. +- `yank_host`, 6×3 window: `i` on North/Cost (1,2), then `Tab` onto Profit at + column 3. +- The control, `fold_host` with Q2 expanded: `Enter` lands on Apr, a record, + and must stay editing on both ends. A settle that closed every editor landing + past the window cannot pass it. + +The test asserts both ends land on the same cell and end in the same mode. + +It was red at #277's head `aab62a36`, on the fold case: the client was +`Editing` and the TUI `Normal`. + +Rivals: +- No `settle_declined_editor`: fails on the fold case. +- `declined_at` letting a formula's result through: fails, as does the enz6 + lockstep test. + +### a_wrapping_advance_settles_on_the_cell_it_lands_on_not_a_stale_one + +improvise-y5eb, found by the adversarial review of #282 at `942f57e5`. The +reviewer's repro, made permanent. + +Setup (`wrapping_host`): +- `_Measure` is on rows: [Revenue, Profit, Cost]. Profit is an explicit item + and also `= Revenue - Cost`. +- Months [Jan, Feb] are on columns. +- The window is 2×6, scrolled to rows 1–2, with the cursor on Cost/Jan (2,0). + +`i` then `Enter` wraps to (0,1), Revenue/Feb, which is editable, and the TUI +edits it. The client first folds its own outgoing frame: the row offset drops +to 0, but the cache still holds rows 1–2. `settle_declined_editor` then read +the cache at `(0 - 0, 1 - 0)`, which is Profit/Feb, and closed the editor with +Profit's formula refusal, naming a cell that was not under the cursor. The +host's reply did not repair it, because the settle only ever closes. + +- **Red at `942f57e5`:** the client was `Normal`, the TUI `Editing`. +- **Fix:** `declined_at` reads through `AxisFrame::first`. +- **Rival:** reading through `cell_at` (the cursor's offsets) again fails this + test and nothing else. + +The same test passes at #277's head, so #282 introduced the bug. My hand-back +for #282 said the aliasing was unobservable through a one-step advance. That +was true for one step and false for the wrap. + +### enter_on_an_editor_over_a_declined_cell_declines_out_loud_on_both_ends + +improvise-ik6l. The editor is seated with `7` typed in and `touched: true`, on +two declined cells: the Q2 fold (`fold_host`, (2,1)) and Profit (`yank_host`, +(1,3)). Each is tried with `Enter` and with `Tab`, so four cases in all. + +The TUI refuses at `committable_cell_key` and names the reason. The whole +sequence goes with it: the welded `clear-buffer` does not run, so the text stays +and the mode stays `Editing`. The browser composed nothing, and the key went +`Upstream`, which no host answers: nothing happened and nothing was said. + +The test asserts four things: +- the key resolves locally; +- no write is composed; +- the mode and buffers equal the TUI's; +- both status lines are non-empty. + +Red at `100627fd`: the `(2,1)` fold case went upstream. + +Rivals: +- No `refused_here`: this test fails, going upstream. +- Refusing inside `commit` instead of at the step: this test fails on the + buffers (`""` against `"7"`). A step-level decline is what takes the welded + `clear-buffer` with it. That was my first attempt. +- Refusing `commit-cell-edit` only: this test fails on the `Tab` cases. + +### typing_before_the_host_answers_keeps_the_editor_past_the_window + +The half of improvise-5m9z that the TUI cannot see. The TUI never waits, so +nothing in it can be compared with this. On the same 2×6 `fold_host` as +above, `i`, `Enter` and `7` are all folded and sent **before** any of the +host's replies is delivered. The `7` is typed into the provisional editor while +the landing cell is still unprojected. Then the replies arrive in order. The +editor must still be open with `7` in its buffer. The fold under it is declined, +but the user's input is not thrown away. + +Rival: the settle ignoring `touched`, closing any editor on a declined cell. +This test fails and the one above passes, so the two tests pin the two +directions of the `touched` guard. ### the worker is not called up until its first frame has arrived @@ -5453,6 +5548,196 @@ the tile that moved. Injected #231's rule (`first Regular, else 0`): this case fails, because tile select opened on `_Index` and cycled it onto Row, which is the bug reproduced headless in the ticket. +## A coordinate rename from the browser (improvise-fdk1) + +The reasoning is in `context/code/clients.md`, "A coordinate rename". Every rival +below was applied by exact substitution, run, and reverted. Each is named with +the tests it failed. + +### a_locally_committed_coordinate_rename_agrees_with_the_host + +Two records, North and South. North is renamed to Zed with real keys: `i`, five +`Backspace`, `Z e d`, `Enter`. The host takes its `Cmd` path, a twin host +applies what the client composed, and `both_ends` compares the document, mode, +status and every cursor after each key. + +- **Red at `4e5299f6`:** it failed at `i`, "client and host disagree about the + mode". The client declined every records-mode edit, while the host opened an + editor. +- **Rival: the client composes the rename without the advance** (`no_advance`). + It fails here on the cursor. It also fails + `an_empty_coordinate_rename_is_refused_the_way_the_host_refuses_it` and + `a_second_rename_after_a_reorder_renames_the_row_the_host_shows`. +- **Rival: the effect copies instead of moving** (`copy_not_move`). It fails + here, but only through the final "Region=North is gone" assertion. The + document comparison itself passes, and that is by construction, not a gap: + the host's key path and the twin's wire path now apply the same + `effect::RenameRecordCoordinate`, so a fault in the effect moves both ends + together. Only the outcome assertions can see it. + +### an_empty_coordinate_rename_is_refused_the_way_the_host_refuses_it + +The host's key path refuses an empty coordinate with +`RECORD_COORDS_CANNOT_BE_EMPTY` and still advances. Red at the first fix: the +client composed the write, the twin refused it, and the status lines differed +("Records mode" against the refusal). The client now composes the host's answer +itself, with the status taken from the protocol's one copy of the message. The +test also asserts that the host really refused, so it cannot pass on a fixture +where nothing was empty. + +### a_browser_renames_a_records_coordinate_and_the_host_moves_the_record + +A real `Host`, and a client driven into records mode through the tile bar. It +renames North to Zed with keys. The rename must be accepted, Zed must hold +North's 1200, and North must be gone. This is the control for the refusal test +below, which reuses the same scenario helper (`rename_north`). + +- **Red at `4e5299f6`:** A's editor did not open. +- **Rival `copy_not_move`:** "the record moved, not copied". +- **Rival `wrong_surrogate`** (the projection names the record by a key that is + not the record's): fails here too, because the rename names nothing that + resolves. + +### a_coordinate_rename_another_client_moved_is_refused + +The same scenario, except that client B writes 999 to North's Revenue while A's +editor is open. A's rename must be `Rejected(RecordMoved)`, B's 999 must +survive, and no Zed may exist. The test also asserts that B's write was +accepted, or A would have nothing to be stale against. + +- **Rival `claim_at_commit`** (claim the version at commit, improvise-0j0f's + shape): fails **only** this test. The rename is accepted over B's write. +- **Rival `unnamed`** (`records_named` names no record for the rename): fails + here too, because the basis check never looks at the record. It also fails + `ui::host::tests::a_write_to_a_record_its_own_frame_renamed_is_a_broken_chain`. + +### a_second_rename_after_a_reorder_renames_the_row_the_host_shows + +Three records: North, South and West. The client renames South (row 1) to Zed, +which re-sorts the rows. `Enter` re-enters editing one row down, on West as the +client's cache stood **before** the host answered. The host's answer then +arrives, including a `ResetCursor` (see clients.md), and the test reads which +record the cursor shows **now**. It asserts that record is not West, so that +the two captures differ. The second rename, `Alpha`, must move the record the +client shows. + +- **Rival `capture_unanswered`** (capture the editor's target while the + client's own write is still in flight): fails **only** this test. The rename + moves West instead. +- The first version of this test renamed North, and there the stale capture and + the correct one landed on the same record, so it could not tell them apart. + It also asserted "one row down", which is false in the browser (the + `ResetCursor`). Both were caught by running it, before any rival. + +**Amending the entry for `the_tile_bar_route_into_records_mode_does_not_strand_the_user` +above.** Column 0 of its table is out of date: since improvise-fdk1, on a +coordinate column `i` opens an editor, +because its `Enter` composes a rename. Every other records column must still +decline out loud. The walk now also asserts that it covered at least one column +of each kind, so it cannot pass on a row made entirely of coordinates. + +**Amending the entry for `a_second_rename_after_a_reorder_renames_the_row_the_host_shows` +above, after #221 merged #227 (improvise-fy9o/p6v6).** The reopened editor is +no longer empty. It awaits the host's answer and is then seeded with the cell +the answered grid shows. So the test now asserts that the buffer equals that +cell's text, and backspaces over it before typing `Alpha`. + +`rename_coordinate` now takes the frame's own write as its `writes`, as +`commit` does. **Rival:** `writes = false`, so the reopen seeds at once from the +stale cache. It fails this test on the new assertion (`"West"` where `"Zed"` is +shown) and nothing else. The explicit `cleared` in `rename_coordinate` is not +observable. Every rename advances, and the reopen writes the buffer itself. The +one advance that goes nowhere is the bottom-right corner, which is a `+` +column, never a coordinate. + +### an_untouched_coordinate_enter_writes_nothing_on_either_end + +Added when #221 (improvise-fdk1) merged #205 (improvise-ixxx). #205 gave every +cell editor a recorded opening, `SetOpenedEditor(OpenedEditor::on(seed))`, and +made an untouched commit write nothing. The host's `EditOrDrill` already records +that opening for a records coordinate, and its `CommitAndAdvance` already skips +the write. So the client has to do both, or the two ends disagree. + +The test opens North's coordinate with `i` on a host, a wire twin and a client, +and asserts that the client's `opened_editor` equals the host's and is +`OpenedEditor::on("North")`. It then asserts that `Enter` composes no +`Command::Model`, runs `Enter` on all three ends, and checks the document is +unchanged. The document check alone would not discriminate: a rename from North +to North may land nothing either way, so the assertion that decides is on the +composed commands. A guard asserts that `Enter` took the commit path at all +(`ChangeMode(Normal)` is among the commands), so the test cannot pass on a key +that did nothing. + +- **Rival: the rename ignores `untouched`.** Fails only this test: the client + composes a `RenameRecordCoordinate`. +- **Rival: the coordinate editor opens with `SetBuffer` and `ChangeMode` only** + (#221's shape before the merge). Fails only this test: the client records no + opening, the host does, and the first assertion catches the difference. + +### a_retry_after_record_gone_renames_no_other_record + +improvise-20ek. Client A opens the editor on North's coordinate and types `Zed`. +Client B then renames North to `Yak`, so A's `Enter` is refused with +`RecordGone`. The refusal replays A's view, and the editor reopens over a row +that is no longer North. The second `Enter` must rename nothing: afterwards +South, West and Yak must hold their values and `Zed` must not exist. + +- **Red at the first commit of this branch:** the retry renamed South to Zed. + `settle_opened` had recaptured whatever record sat under the cursor, the same + recapture that makes `RecordMoved` a one-keystroke force. +- **Rival `never_set_gone`** (answer `RecordGone` like any other refusal): + fails only this test. +- **The setup assertion is load-bearing.** It checks that the editor is back + **and** over a record other than the one renamed away. Without that, a + fixture where the cursor still sat on the renamed record could not tell a + retarget from a correct decline. +- The reviewer's probe on PR #221 + (`probe_a_retry_after_record_gone_renames_no_other_record`) is the same + scenario and passes on this fix. + +**Amending the entry for +`a_second_rename_after_a_reorder_renames_the_row_the_host_shows` above.** It is +the test that separates this fix from its obvious rival. The reviewer's rival +`never_clear_opened` keeps the opened target through every refusal and every +return to `Normal`. It passes the `RecordGone` test above, and it fails this +one only: "the second rename must move North, the record the client showed". +The second rename still names South's old record, which the first rename moved +away, so nothing moves. So the fix has to distinguish the +two refusals. `RecordGone` on the renamed record pins `Opened::Gone`, while +`RecordMoved` and a normal return keep the recapture. + +### a_rename_onto_a_record_another_client_just_wrote_is_refused + +improvise-milr. A opens the editor on North and types `South`, which is the +coordinate of an existing record. B then writes South's Revenue as 999, and A +**receives** that projection before pressing `Enter`. The rename must be refused +with `RecordMoved(south)`, South must still hold 999, and North must still hold 1. + +- **Red at the first commit of this branch:** the rename was accepted, and B's + 999 was overwritten by North's 1 (kndq's overwrite rule, applied without any + check). +- **Rival `no_destination`** (the server checks the source record only): fails + only this test. +- **Rival `seen_at_commit`** (the client claims the other records as its cache + holds them at `Enter`, not at open): fails only this test. This was the first + version of the fix, and running this test is what caught it. Delivering B's + projection to A is the step that separates the two: a cache that has already + folded the write claims the new version and passes the check. +- **Rival `claim_source_only`** (the client claims just the opened record): + fails this test and the control below. Here it is refused with + `RecordUnstated` instead of `RecordMoved`. + +### a_rename_onto_a_record_this_client_has_seen_still_overwrites_it + +The control for the test above, and kndq's policy unchanged. With nobody else +writing, renaming North onto South is accepted, South holds North's 1, and North +is gone. + +- **Rival `claim_source_only`:** fails here with `RecordUnstated`. The server + now answers for the destination, so a client that does not claim it cannot + rename onto an existing record at all. This test is the only one that fails + if the client stops claiming what it saw. + ### `enter_enter_leaves_every_cell_unchanged_on_both_ends` **improvise-fy9o**, browser and host in lockstep. On the 2×2 `precise_host`, @@ -5757,6 +6042,371 @@ probe, whose frames carried no status, saw `""` instead. It passed at the base in-flight frame over the settled sentence. That is why the sentence goes to the tail of the queue rather than into `settled`. +### a_document_swap_closes_an_open_rename_editor + +Added when #221 merged #219 (improvise-vndi). vndi changed `claim_as_read` to +take the document generation captured when an editor opened, so an inline edit +opened before a `LoadModel` is refused `DocumentReplaced` rather than written +into the new document. The rename capture (`OpenedCoordinate`, now `OpenedCell`) records +`document_generation` at open for the same reason, and the rename claims it. + +**No rival of that choice is observable, and this test says why.** B loads a +document while A's rename editor is open. The rebase `Snapshot` rebuilds A's +view from a fresh mirror: A lands in `Normal`, outside records mode (`_Records` +is virtual and never persisted), and `settle_opened` drops the capture. So a +rename editor never survives a generation change, and claiming the current +generation instead of the captured one fails nothing. The first version of this +test asserted `DocumentReplaced` and got no frame at all, which is how that was +found. The inline editor differs because its `EditTarget` lives in the page, +outside `ViewState`, and survives the rebase. + +What the test pins is the precondition: the swap closes the editor, drops the +capture, and renames nothing (North survives, no Zed). If a later change lets a +`Snapshot` preserve an open editor, this test fails, and a real test of the +captured generation becomes both possible and necessary. + +**Amending the entry for +`a_second_rename_after_a_reorder_renames_the_row_the_host_shows` above.** Since +#230 (improvise-wdmp) the asker keeps its own placement after a reframing +write, where it used to be reset to the origin. So the re-entered editor now +sits on row 2, which in the answered grid is Zed, the record the first rename +produced. The test's principle is unchanged: the second rename moves the row +the client shows, and that row differs from the stale capture (West). Only its +value table assumed Zed could not come up, so Zed was added to it. + +### a_keyboard_edit_writes_the_cell_it_opened_on_after_the_rows_renumber + +improvise-8xq0, the ticket's probe, through a real `Host` with two clients. A +presses `l`, `i` on North/Cost at (0,1), and types `42`. B hides North, and A +folds the relayed frames, so the rows renumber under the open editor. A then +presses `Enter`. `resolve.rs::commit` composed `SetCell` from the cell under +the cursor **at Enter**. The cursor had been reset to the origin, which now +held South/Revenue, so South/Revenue became 42 and the host accepted it. That +failed at the `South was written` assertion, on `gh/main` at `202e7eac` and on +#221's head `39821d5f` alike, so #221's coordinate capture did not reach this +path. + +The editor opens on North/**Cost**, not the first column, so the cell at Enter +differs from the opened one in both record and measure. A fix that kept the +record and read the measure at Enter would still write the wrong column. + +- **Rival: `written` takes the whole cell from the cursor at Enter** (the bug). + Fails only this test, at `South was written`. +- **Rival: the record from the capture, the measure from the cursor.** Fails + only this test, at `the edit must land on the cell the editor opened on`: + North/Cost stays empty. +- **Rival: the basis claimed at commit (`claim`) instead of as read.** Fails + this test too: after the hide, A's basis no longer names North's record, so + `claim` leaves it out and the host answers `RecordUnstated`. + +### a_keyboard_edit_is_refused_when_its_record_was_written_while_open + +improvise-xz2z: improvise-0j0f's rule on the keyboard path, which it had not +had. xz2z was filed separately (by agent-227-fixes, scoping improvise-3al1) and +has the same cause as 8xq0: `written` read its operands at `Enter`. Same setup, +but B writes 999 into North/Revenue while A's editor is open on North/Cost. A +record is the coordinate tuple minus `_Measure` (gotcha 40), so that write +moves the version of the record A is editing. The keyboard commit claimed the +version current at Enter, which A had already folded, so the host accepted it. +It failed at the `RecordMoved` assertion, on `gh/main` and on #221's head. + +- **Rival: the basis claimed at commit instead of as read.** Fails this test + (and the renumber test, above). + +### a_second_keyboard_commit_writes_the_cell_the_advance_landed_on + +A regression guard for the capture's lifetime, which the fix made +load-bearing for value cells. `i`, `5`, `Enter` on North/Revenue writes 12005, +advances to South/Revenue and reopens the editor there; `7`, `Enter` must write +South/Revenue. The capture is dropped when the commit's `ChangeMode(Normal)` +folds and retaken for the reopened editor once the host answers. + +- **Rival: `settle_opened` keeps a capture after the editor closes.** Fails + this test, the second commit writing North again. Before this test only #221's + two coordinate tests saw that rival + (`a_second_rename_after_a_reorder_renames_the_row_the_host_shows` and + `a_document_swap_closes_an_open_rename_editor`); nothing on a value cell did. + +### a_value_commit_before_the_host_answers_the_last_one_waits_out_loud + +With the value target captured only after the host answers this client's own +last write, `Enter` pressed again before that answer has no target. The test +commits once, folds the frame as sent and not yet answered, and presses +`Enter` again in the reopened editor. The fixture's second row is given a +`Record` identity, or `commit` stops at the cursor's cell before it reaches the +capture and the test would pass for the wrong reason. + +- **Rival: no waiting decline (`None`, as for a cell no editor writes).** Fails + only this test: the key goes upstream, which in a browser does nothing. + +The test asserts the status and that nothing is written, not the exact +command list: the `Enter` binding's welded `clear-buffer` still runs after the +decline and empties the buffer. That loss is improvise-joed. + +**Amending the entry for `a_document_swap_closes_an_open_rename_editor` above.** +On the 20ek/milr branch the reviewer's rival `never_clear_opened`, which keeps +the capture through every refusal and every return to `Normal`, fails this test +as well as the reorder test: its capture survives the swap. So the test does +see one regression class, a capture that outlives its editor, even though no +rival of the captured *generation* is observable. + +### a_retry_after_a_crossing_rename_of_its_record_renames_no_other_record + +improvise-9ww8, found by the adversarial review of PR #251 at `29d2adef`. The +earlier fix pinned the editor only on `RecordGone` of the renamed record. But +`stale_basis` checks the layout tier before the record tier, and another +client's rename bumps `layout_version`. So a crossing is refused `LayoutMoved`, +and the retry recaptured whatever record was under the cursor. + +The test makes the crossing explicit. A opens North's coordinate and types `Zed`. +B renames North to `Yak`, and B's relays to A are **held**. A then composes +`Enter` without having heard B. Only after that does A receive B's relays and +then its own refusal, in the order a single socket delivers them. The setup +asserts that the refusal is `LayoutMoved`, which is the tier the old pin could +not see. The retry `Enter` must rename nothing: afterwards Yak holds 1, South 2, +West 3, and there is no Zed. + +- **Red at `4ff79ff2`:** the retry renamed South to Zed, the reviewer's probe 1. +- **Rival `no_pin_at_all`** (no pin after a refusal): fails this test, the + reorder-crossing test below and `a_retry_after_record_gone_renames_no_other_record`. +- **Rival `retry_any_capture`** (after a refusal, accept whatever capture is + under the cursor): fails the same three. +- The existing 20ek test delivers B's rename to A **before** `Enter`, so A's + layout claim is current there and only the record tier can fire. That is why + it could not see this. + +### a_retry_after_a_crossing_reorder_renames_no_other_record + +improvise-9ww8, the reviewer's probe 2, which shows that "is the source gone" is +not the discriminator. The crossing is the same, but A edits **South** (row 1) +and B renames North. South survives, but the re-sort moves it to row 0, so the +replayed editor sits over West. The retry must not rename West: West keeps its +3, Zed is never West's 3, and Yak holds 1. + +- **Red at `4ff79ff2`:** West was renamed to Zed while South still existed. +- **Rivals `no_pin_at_all` and `retry_any_capture`:** both fail this test. + +### a_retry_after_a_crossing_write_to_its_own_record_is_a_force + +The control for the two tests above. A rule of "decline after any refusal" would +pass both of them, so this test separates the fix from that rule. The crossing +is a `SetCell` by B on North's Revenue. It is refused `RecordMoved(north)` (a +write reshapes nothing, so only the record tier fires). The replayed editor is +still over North, so the retry is the one-keystroke force: Zed holds B's 9, and +North is gone. + +- **Rival `always_decline`** (every retry after a refusal declines): fails only + this test. +- **Rival `never_clear_opened`** (the #221 reviewer's rival: keep the opened + target through everything): fails this test as well as the reorder and swap + tests. Its stale target claims North at the version it opened with, so the + retry is refused again rather than forced. + +**Amending the entry for `a_retry_after_record_gone_renames_no_other_record` +above.** Its rival `never_set_gone` no longer exists as written. The explicit +`RecordGone` pin it removed has itself been removed, because the general rule +subsumes it: after `RecordGone(r)` the capture can never be `r`, so it becomes +`Gone(r)` anyway. A rival that dropped the explicit pin (`gone_as_retry`) failed +no test, which is how the redundancy was found. The test's live rivals are now +`no_pin_at_all` and `retry_any_capture`. + +### a_keyboard_edit_retried_after_a_crossing_hide_writes_no_other_cell + +improvise-8phw, the value twin of improvise-9ww8, found reviewing PR #274 at +`7bf8debd`. A presses `l i 4 2` on North/Cost. B hides North, and **B's relays +reach A only after A has sent `Enter`**, which is ordinary latency. The host +refuses A's frame `LayoutMoved`. The refusal replays the view, the editor +reopens at (0,1), and `settle_opened` recaptured what was now there, South/Cost. +The retry `Enter` was accepted and wrote 42 into South/Cost. It failed at +`South/Cost was written`. The 8xq0 test cannot see this: it delivers the hide +before `Enter`, so the capture from open time is still in use. + +The test also asserts the retry's status. Without that, a decline that goes +silently upstream would pass. + +- **Rival: `written_to` recognises renames only** (#251 as it was). Fails this + test and the column-hide test below. +- **Rival: a `Gone` value editor answers `None`**, which goes upstream. Fails + only this test, at the status assertion. + +### a_keyboard_edit_retried_after_a_crossing_column_hide_writes_no_other_measure + +improvise-8phw, the measure half. A opens on North/Revenue at (0,0) and types. +B's crossing `HideItem` hides the Revenue *measure*, so after the refusal the +cursor sits on North/Cost: the same **record**, another cell. This is the input +that separates "compare the whole `OpenedTarget`" from "compare the record", +which the row-hide test above cannot do, since there the record differs too. + +- **Rival: `Retry` compares the record only.** Fails only this test: the retry + writes the typed text into North/Cost. + +### a_keyboard_edit_retried_after_a_crossing_write_forces_the_same_cell + +improvise-8phw's control. B's crossing write to North/Revenue moves the +version of A's record, and the refusal is `RecordMoved`. The recapture is +North/Cost again, so the retry must go through: the one-keystroke force of +gotcha 43. A fix that declined every retry would fail here. It passed before +the fix too; it is a guard, not a reproduction. + +### a_keyboard_edit_whose_rows_all_vanished_writes_the_cell_it_opened_on + +ChatGPT Codex's P2 on PR #274 at `7bf8debd`. `commit` picked the operation from +the cell under the cursor before it consulted the capture. B hides both regions +while A's editor is open on North/Cost and A folds the relays, so there is no +cell under the cursor at all. `Enter` then answered `None`, which goes upstream +and does nothing in a browser. The capture was intact the whole time. It failed +at the `Accepted` assertion. + +- **Rival: dispatch on the cursor's cell** (the code before the fix). Fails only + this test. +- **Rival: capture a records grid's measure cell as a value.** Fails + `every_advertised_command_answers_in_its_own_mode`: `RECORDS INSERT` `Enter` + would start writing a cell the browser deliberately declines. + +### keys_sent_before_a_scroll_is_answered_agree_with_the_tui + +improvise-7mro, the offset-aliasing matrix from the review of #282. The fixture +is `wrapping_host`: `_Measure` rows Revenue, Profit (stored and a formula), +Cost, and Month columns Jan, Feb. Seven cases: the wrap, a horizontal wrap that +scrolls both axes, `G`, `$`, `gg`, `$ x` and `yy gg p`. Each runs three ways: +every reply delivered before the next key, every reply held to the end, and +replies held until the last two keys. Each run compares the host's document, +the cursor and the mode with the TUI. The delivered and held-then-typed runs +also compare the whole document text and the edit buffer. + +With every reply held, the browser cannot know the seed, so the held run asserts +something narrower. No write is composed before its target is projected. The +last key waits out loud, and an editor is open exactly when the keys contain +`i`. After the replies land, repeating the last key writes the cell the TUI +wrote. + +Red at `6942e53e` (#282), at gh/main `202e7eac` and at #274's `8d9de94e`. `$` +held wrote **Jan/Revenue**. `G` and `gg` held refused Cost with Profit's formula +refusal. The wrap and the horizontal wrap sent Enter `Upstream`, silently. At +#274's head, the horizontal wrap with replies **delivered** also wrote +Jan/Revenue, because its capture reads `cell_at_cursor`. + +**Rivals**, each failing here: +- `edit_or_drill` without the `unprojected` arm fails `G` held: Normal, no + status. +- `refused_here` without the wait on a commit fails the wrap held: Enter + silent. +- `refused_here` without the `clear-cell`/`yank`/`paste` arm fails `$ x` held. +- No `settle_awaited_seed` fails `G` held-then-typed: `7` where the TUI has `47`. +- A settle that accepts a cell with no id fails the same case. It seeded the + blank a relabel leaves between the frame and its cells. +- `cell_at_cursor` through the cursor offsets fails `G` held-then-typed here, + and the test below. + +### a_held_cell_is_read_at_its_position_and_not_its_cache_index + +improvise-7mro, the second form of the aliasing. `four_by_two_host` has four +regions by two measures, and no two cells are equal. The client's window is +rows 2 and 3. `k k j` puts the cursor on row 2, which the cache holds, with row +offset 1, so the offsets differ from the frame's origin. Every reply is held. +Two key runs, `i 7 Enter` and `i Tab`, are compared with the TUI: the document, +the cursor, the buffer and `opened_editor`. + +**Rivals:** `cell_at_cursor` through the cursor offsets fails on +`i 7 Enter`: it seeds and writes R3 where the TUI writes R2. `seed_at` through +the cursor offsets fails on `opened_editor`: the reopened editor's seed comes +from R3. The `#282` settle masks a bare `i` here: a Profit-like decline would +close it. That is why the fixture has no formula and the test goes on to +commit. + +**Amended when #227 (with #272, improvise-p6v6) merged #295.** Under p6v6 the +editor that `i 7 Enter` reopens awaits the seed, because its own write is still +unanswered. With every reply held, its buffer is `""` where the TUI's is `"13"`. +So the test now reads the editor twice: + +- **Before delivery**, only when the editor is not awaiting. That is the `i Tab` + run, whose reopen reads `seed_at` at once. +- **After delivering the held replies**, always. + +The `seed_at` rival still fails, on `i Tab` before delivery (`"23"` against +`"22"`), and fails nothing else. + +### resolve::tests::a_cursor_on_an_empty_grid_waits_for_nothing + +improvise-7mro. `unprojected` asks whether the position is inside the grid's +extent, not only outside both frames. An empty grid has empty frames, so +without the extent every position is outside them. **Rival:** drop the extent +guard. `i` then opens an editor awaiting a seed that no cell will ever supply. + +**Amending the entry for `keys_sent_before_a_scroll_is_answered_agree_with_the_tui` +above, for #274's capture.** When #286 was merged into #274's stack, this matrix +became the red test for the capture: at #274's `8d9de94e` its "horizontal wrap" +case with replies **delivered** wrote Jan/Revenue where the TUI writes +Feb/Revenue. That case is green on #286 alone, because only #274 makes a +captured cell stick. On the merged tree the rival that reads the capture +through the cursor's offsets (`opened_at_cursor` through `row_offset`/`col_offset` +rather than `cell_at_cursor`) fails exactly that case, with `["Jan/Revenue"]` +against `["Feb/Revenue"]`. + +### a_commit_that_waits_for_the_host_keeps_the_typed_text + +improvise-joed, on the value path. A commit is sent and not yet answered, its +advance reopens the editor, and the user types `43` and presses `Enter` again. +There is no capture yet, so the commit declines with "still waiting". But the +key is `[commit-cell-edit, clear-buffer edit]`, and the decline came back from +`composed` as an ordinary `Local` step, so the sequence ran the welded clear +after it. The editor stayed open with an empty buffer. It failed at the buffer +assertion with `Some("")`. The test folds the second `Enter` as sent, so it +checks what the user sees, not what the resolver returns. + +- **Rival: the waiting decline is `Step::Local` again.** Fails this test and + `a_rename_that_waits_for_the_host_keeps_the_typed_text`. +- **Rival: a `Declined` step no longer stops a sequence.** Fails both joed + tests and `every_advertised_command_answers_in_its_own_mode`. + +### a_rename_that_waits_for_the_host_keeps_the_typed_text + +improvise-joed, on #221's rename path, through a real `Host`. A records-mode +rename of South to Zed is composed and folded as sent, and never answered. The +commit reopens the editor, the user edits it, and presses `Enter` again. It +failed at the buffer assertion: `Some("")` where the typed text was expected. + +- **Rival: only the value path declines (`rename_coordinate`'s waiting arm + stays `Local`).** Fails only this test, so the value test alone could not + see the rename path. + +### a_retry_whose_cell_is_gone_keeps_the_typed_text + +improvise-joed on #251's other decline, as the review of #278 asked. This is +8phw's crossing: A's Enter on North/Cost crosses B's hide of North, the refusal +leaves `Opened::Gone`, and the retry `Enter` declines with "the row you were +editing has changed". That decline came back as a `Local` step too, so the +welded `clear-buffer` ran after it and emptied the editor the user must now Esc +away from. It failed at the buffer assertion with `Some("")` where `Some("42")` +was expected. The fix is the same as for the waiting decline: `renamed_away()` +is a `Step::Declined`, shared by the value and rename paths. + +- **Rival: `renamed_away()` is `Step::Local`** (the state before the fix). Fails + only this test. It drives the value path; the rename path reaches the same + function, so no rival can split them without also changing the code shape. + +### a_captured_edit_commits_when_the_cursor_now_sits_on_an_aggregate + +improvise-3h9w, a regression from merging #286 into #274, found by ChatGPT Codex +and the delta review at `8c28360f`. `step()` runs `refused_here` before +`commit()` consults the capture, and for the commit keys `refused_here` judged +the **cursor's** cell. A opens an editor on North/Cost and types `42`. B moves +Region to None, the relays reset A's cursor, and it now sits on an aggregate. +A's `Enter` was declined with "Cannot write to an aggregate" on every retry, no +frame was sent, and Esc lost the text, where `8d9de94e` wrote North/Cost. It +failed at the North/Cost assertion with `None`. + +The vanished-rows test could not see this, because `unprojected` exempts an +empty grid, so no guard fired there. This case needs the cell under the cursor +to change **kind** while it stays projected. + +- **Rival: `refused_here` without the `state.opened.is_some()` arm** (the state + before the fix). Fails only this test. +- **Not covered:** a captured editor whose cursor lands on a cell that is + inside the grid but outside the held window. The same arm exempts it, but no + test builds it, because every reframe here resets the cursor to the origin, + which is always inside the window. + ### `a_commit_that_prunes_its_row_reopens_on_the_row_that_moved_up` **improvise-3uzg**, found by the #227 review at `a093d126`. Fixture: diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index c58c267e..6b4fc410 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -6442,6 +6442,38 @@ SUM(Revenue)`: East's own `SUM` is 0, so its row draws `div/0`. The pooled total is 60/60 = **1.00**. It was green at `9948aaf5` and fails under the same rival as the test above. +## A coordinate rename on the wire (improvise-fdk1) + +### a_write_to_a_record_its_own_frame_renamed_is_a_broken_chain + +The frame is `[RenameRecordCoordinate(North -> West), SetCell(North's surrogate)]`, +on a **records** host. The frame must be `Rejected(ChainBroken(North))`: after +the rename, North's surrogate resolves to nothing, and clause 4 undoes the +rename too, so North keeps 1 and no West exists. The control sends the rename +alone: it is accepted and moves the record, so the refusal cannot pass by +"renames never work". + +- **First run, on the default pivot host:** the pair was **accepted**. West got + 1, and the `SetCell` re-created North at 7. After the rename, North's empty + cell is still drawn in a pivot, so `record_in_layout` resolved the old + surrogate. What the pair means therefore depends on the host's view. That is + filed as improvise-8id2 rather than pinned, and this test is pinned to the + records host, where the rename's producer lives. +- **Rival `unnamed`** (`records_named` names no record for the rename): fails + here, because the frame check no longer asks about North. + +### ui::project::cell_identity::a_record_named_only_by_its_coordinate_is_in_the_published_basis + +A document with no measures, whose only record is anchored by a cell with no +`_Measure`. In records mode the row projects only a coordinate cell, and the +test asserts that is so, or it proves nothing. The published basis must still +carry the record's version. Otherwise a client renaming that row states no +version for it and is refused `RecordUnstated`. + +- **Rival `unpublished`** (`published_basis` counts only `Record` cells): fails + **only** this test. Every other fixture has measure cells, and those publish + the same record anyway. That is why this test exists. + ### a_frame_that_reframes_and_then_places_the_cursor_leaves_the_asker_placed improvise-wdmp. A browser frame that reframes the grid and then places the cursor, diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 1a2ef3ec..802c72a1 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -933,7 +933,13 @@ fn commit_plain_records_edit( return; }; match col { - RecordCol::Category(_) | RecordCol::Measure(_) => { + // The one door the wire's `RenameRecordCoordinate` also takes (improvise-fdk1). + RecordCol::Category(category) => effects.push(Box::new(effect::RenameRecordCoordinate { + record: row.coords.clone(), + category: category.clone(), + item: value.to_string(), + })), + RecordCol::Measure(_) => { effects.extend(effect::RecordRewrite::of(ctx.model, row, [(col, value)]).effects()); } RecordCol::NewMeasure => add_measure(value, effects), diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 120f3431..7b9808e9 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -40,7 +40,7 @@ pub fn uneditable(model: &Model, layout: &GridLayout, row: usize, col: usize) -> ), Some(CellId::Derived { target }) => Some(derived_refusal(&target)), Some(CellId::NonItem { category, item }) => Some(non_item_refusal(&category, &item)), - Some(CellId::Record(_) | CellId::Create(_) | CellId::Coordinate) | None => None, + Some(CellId::Record(_) | CellId::Create(_) | CellId::Coordinate { .. }) | None => None, } } @@ -130,7 +130,9 @@ impl<'a> CmdContext<'a> { Some(CellId::Record(_)) => self.stored_cell_key(), Some(CellId::Aggregate(_)) => Err(self.aggregate_refusal()), Some(CellId::Derived { target }) => Err(derived_refusal(&target)), - Some(CellId::Create(_) | CellId::Coordinate) => Err(NO_VALUE_REFUSAL.to_string()), + Some(CellId::Create(_) | CellId::Coordinate { .. }) => { + Err(NO_VALUE_REFUSAL.to_string()) + } Some(CellId::NonItem { category, item }) => Err(non_item_refusal(&category, &item)), None => Err(match self.cell_key() { Some(_) => NO_VALUE_REFUSAL, diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index ed91d7ee..221dee52 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -13,7 +13,7 @@ use super::app::{App, AppMode, ViewFrame}; /// inverses) and re-exported here, where most callers already look for it. pub use super::app::Panel; -pub(crate) const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates cannot be empty"; +pub(crate) use improvise_protocol::RECORD_COORDS_CANNOT_BE_EMPTY; /// What an effect changes, so a server can decide whether connected clients /// have to be told about it (improvise-mae). @@ -759,6 +759,29 @@ impl Effect for ApplyAndClearDrill { } } +/// Move the record at `record` to `category = item`: the key path's and the wire's +/// one door, through the same [`RecordRewrite`] a drill uses (improvise-fdk1). +#[derive(Debug)] +pub struct RenameRecordCoordinate { + pub record: CellKey, + pub category: String, + pub item: String, +} +impl Effect for RenameRecordCoordinate { + fn apply(&self, app: &mut App) { + let model = &app.model_state.workbook.model; + let row = crate::view::record_row(model, &self.record); + let col = RecordCol::Category(self.category.clone()); + let rewrite = RecordRewrite::of(model, &row, [(&col, self.item.as_str())]); + for effect in rewrite.effects() { + effect.apply(app); + } + } + fn kind(&self) -> EffectKind { + EffectKind::Model(ModelChange::CellData) + } +} + /// A records row's edits composed into the one record they mean, read off the model /// before anything is written. Why composed: `context/code/command.md`, `RecordRewrite`. #[derive(Debug, Default, PartialEq)] diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index e39eeb52..cc988e09 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1926,6 +1926,80 @@ mod tests { ); } + /// improvise-fdk1: a surrogate names a record as it WAS, and a rename ends that + /// name. See `context/test-evidence/command.md`. + #[test] + fn a_write_to_a_record_its_own_frame_renamed_is_a_broken_chain() { + use improvise_protocol::ModelCommand; + + let mut host = host(); + host.app + .model_state + .workbook + .model + .set_cell(cell("North"), CellValue::Number(1.0)); + host.app.rebuild_layout(); + // A records host: its layout names positions, so an old surrogate cannot + // resolve through it. A pivot host differs — see the evidence. + crate::ui::reduce::reduce_key( + &mut host.app, + crate::command::key::KeyEvent::new( + crate::command::key::KeyCode::Char('R'), + crate::command::key::KeyModifiers::NONE, + ), + ); + assert!(host.app.layout.is_records_mode(), "setup: a records host"); + let (asker, _) = host.connect(10, 10); + let north = addressed("North").record; + let rename = || ModelCommand::RenameRecordCoordinate { + record: north, + category: "Region".into(), + item: "West".into(), + }; + let stored = |host: &Host, region: &str| { + host.app() + .model_state + .workbook + .model + .get_cell(&cell(region)) + .cloned() + }; + + let pair = vec![ + crate::ui::reduce::caught_up(host.app(), rename()), + crate::ui::reduce::caught_up( + host.app(), + ModelCommand::SetCell { + cell: addressed("North"), + value: CellValue::Number(7.0), + }, + ), + ]; + let answer = host.handle(asker, ClientFrame::Commands(pair)); + assert!( + answer.iter().any(|a| matches!( + a.msg, + ServerFrame::Rejected(Rejection::ChainBroken(h)) if h == north + )), + "the write names a record the rename moved away: {answer:?}" + ); + assert_eq!(stored(&host, "North"), Some(CellValue::Number(1.0))); + assert_eq!(stored(&host, "West"), None, "clause 4 undid the rename too"); + + // The control: the rename alone is accepted and moves the record, or + // the refusal above is "renames never work" and proves nothing. + let alone = crate::ui::reduce::caught_up(host.app(), rename()); + let answer = host.handle(asker, ClientFrame::Commands(vec![alone])); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Accepted)), + "{answer:?}" + ); + assert_eq!(stored(&host, "West"), Some(CellValue::Number(1.0))); + assert_eq!(stored(&host, "North"), None); + } + // ── the broadcast ─────────────────────────────────────────────────── /// Which view a client is on is that client's (improvise-jytm): an accepted diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 3ca91829..7900391a 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -355,8 +355,13 @@ pub(crate) fn cell_id( RecordCol::NewMeasure => return Some(CellId::Create(CreateKind::Measure)), RecordCol::NewCategory => return Some(CellId::Create(CreateKind::Category)), // Renaming a coordinate moves every measure cell in the row, which is a - // different intent from `SetCell` and still the host's (gotcha 1). - RecordCol::Category(_) => return Some(CellId::Coordinate), + // different intent from `SetCell`: its own operands (gotcha 1, fdk1). + RecordCol::Category(category) => { + return Some(CellId::Coordinate { + record: RecordHash::of(record), + category: category.clone(), + }); + } }; // The refusal's own question, asked here too, so no cell is offered // for a write the host would then refuse (improvise-gk51). @@ -547,11 +552,12 @@ pub fn published_basis(app: &App, grid: &GridCache) -> Basis { .iter() .flatten() .filter_map(|cell| match cell.id.as_ref()? { - CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))), + CellId::Record(RecordCell { record, .. }) | CellId::Coordinate { record, .. } => { + Some((*record, app.record_version_of(*record))) + } CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) - | CellId::Coordinate | CellId::NonItem { .. } => None, }) .collect(); @@ -5387,6 +5393,43 @@ mod cell_identity { ); } + /// improvise-fdk1: a record named only by its coordinate cell is still + /// published, or its rename is RecordUnstated. `context/test-evidence/command.md`. + #[test] + fn a_record_named_only_by_its_coordinate_is_in_the_published_basis() { + let mut wb = Workbook::new("Anchors"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + let north = CellKey::new(vec![("Region".to_string(), "North".to_string())]); + wb.model.set_cell(north.clone(), CellValue::Number(1.0)); + let mut app = App::new(wb, None); + app.handle_key(crate::command::key::KeyEvent::new( + crate::command::key::KeyCode::Char('R'), + crate::command::key::KeyModifiers::NONE, + )) + .expect("R enters records mode"); + let grid = grid_of(&app, WHOLE); + let ids: Vec = grid + .cells + .iter() + .flatten() + .filter_map(|c| c.id.clone()) + .collect(); + assert!( + !ids.iter().any(|id| matches!(id, CellId::Record(_))), + "setup: no measure column, so only the coordinate names the record: {ids:?}" + ); + assert!( + ids.iter().any(|id| matches!(id, CellId::Coordinate { .. })), + "setup: the coordinate is projected: {ids:?}" + ); + let basis = published_basis(&app, &grid); + assert!( + basis.record(RecordHash::of(&north)).is_some(), + "the coordinate's record must be published: {basis:?}" + ); + } + /// improvise-exdg: a coordinate cell is drawn and holds text, so it is not /// "no cell". `context/test-evidence/command.md`. #[test] @@ -5396,10 +5439,14 @@ mod cell_identity { let region = column_named(&app, "Region"); let north = grid.cell(1, region).expect("the second row's coordinate"); assert_eq!(north.text, "North", "setup: a visible coordinate cell"); + let record = app.layout.record_row(1).expect("row 1").coords.clone(); assert_eq!( north.id, - Some(CellId::Coordinate), - "a drawn coordinate is named" + Some(CellId::Coordinate { + record: RecordHash::of(&record), + category: "Region".into(), + }), + "a drawn coordinate is named, with its own record and category (fdk1)" ); // Each cell's kind against its column's, not merely "some kind" (improvise-w1bu). let misnamed: Vec<(usize, usize, Option)> = (0..app.layout.row_count()) @@ -5417,7 +5464,9 @@ mod cell_identity { /// so `Derived` is never right here (improvise-hoxy). fn names_its_column(col: Option<&RecordCol>, id: Option<&CellId>) -> bool { match (col, id) { - (Some(RecordCol::Category(_)), Some(CellId::Coordinate)) => true, + (Some(RecordCol::Category(c)), Some(CellId::Coordinate { category, .. })) => { + category == c + } (Some(RecordCol::Measure(m)), Some(CellId::Record(cell))) => { cell.measure.as_deref() == Some(m.as_str()) } diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 7a91b172..144ea684 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -149,6 +149,20 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec> { (None, None) => return vec![], (None, Some(key)) => Box::new(effect::ClearCell(key)), }, + ModelCommand::RenameRecordCoordinate { + record, + category, + item, + } => { + let Some(record) = app.resolve_record(*record) else { + return vec![]; + }; + Box::new(effect::RenameRecordCoordinate { + record, + category: category.clone(), + item: item.clone(), + }) + } ModelCommand::AddItem { category, item } => Box::new(effect::AddItem { category: category.clone(), item: item.clone(), @@ -268,16 +282,43 @@ pub fn stale_basis(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option Vec { + let mut records = records_named(cmd); + if let ModelCommand::RenameRecordCoordinate { + record, + category, + item, + } = cmd + { + let destination = app + .resolve_record(*record) + .map(|source| RecordHash::of(&source.with(category, item))) + .filter(|dest| dest != record) + .filter(|dest| { + app.model_state + .workbook + .model + .data + .resolve_record(*dest) + .is_some() + }); + records.extend(destination); + } + records +} + /// The record tier for a command inside a frame whose document tiers are /// already answered, skipping the records the frame is about to create. fn stale_records_in_a_frame(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option { - records_named(cmd) + records_at_stake(app, cmd) .into_iter() .filter(|hash| !will_be_created_by_the_frame(app, basis, *hash)) .find_map(|hash| stale_record(app, basis, hash)) @@ -306,7 +347,7 @@ pub fn caught_up(app: &App, cmd: ModelCommand) -> Command { basis: Basis { document_generation: app.document_generation(), layout_version: app.layout_version(), - records: records_named(&cmd) + records: records_at_stake(app, &cmd) .into_iter() .map(|h| (h, app.record_version_of(h))) .collect(), @@ -667,6 +708,11 @@ pub(crate) mod tests { value: CellValue::Number(1.0), }, ModelCommand::ClearCell(record_cell(&a_stored_cell())), + ModelCommand::RenameRecordCoordinate { + record: record_cell(&a_stored_cell()).record, + category: "Region".into(), + item: "West".into(), + }, ModelCommand::AddItem { category: "Region".into(), item: "South".into(), @@ -755,6 +801,7 @@ pub(crate) mod tests { match cmd { ModelCommand::SetCell { .. } => "SetCell", ModelCommand::ClearCell(_) => "ClearCell", + ModelCommand::RenameRecordCoordinate { .. } => "RenameRecordCoordinate", ModelCommand::AddItem { .. } => "AddItem", ModelCommand::AddItemInGroup { .. } => "AddItemInGroup", ModelCommand::RemoveItem { .. } => "RemoveItem", diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index a58aa24d..b139d4db 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -107,6 +107,18 @@ pub fn drill_snapshot(model: &Model, constraint: &CellKey) -> Vec { RecordRow::group(cells) } +/// The records row `record` is, as the records grid groups it; empty when no +/// cell stores it. The one lookup a rename by record uses (improvise-fdk1). +pub fn record_row(model: &Model, record: &CellKey) -> RecordRow { + drill_snapshot(model, record) + .into_iter() + .find(|row| &row.coords == record) + .unwrap_or_else(|| RecordRow { + coords: record.clone(), + values: Vec::new(), + }) +} + /// One step of a view configuration: **what** a view must be, with no answer /// to how a front end gets there. The model half of drill — core.md §8. #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/crates/improvise-core/src/view/mod.rs b/crates/improvise-core/src/view/mod.rs index 3bdea6ce..fe12c4aa 100644 --- a/crates/improvise-core/src/view/mod.rs +++ b/crates/improvise-core/src/view/mod.rs @@ -5,8 +5,8 @@ pub mod types; pub use axis::{Axis, AxisColor}; pub use layout::{ AxisEntry, GridEval, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow, - ViewStep, drill_constraint, drill_snapshot, drill_steps, records_spine, records_steps, - synthetic_record_info, + ViewStep, drill_constraint, drill_snapshot, drill_steps, record_row, records_spine, + records_steps, synthetic_record_info, }; pub use types::{ DEFAULT_VIEW, DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View, unused_view_name, diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index f97c3177..976ec218 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -3,9 +3,10 @@ use crate::{ BasisCache, CategoryTreeCache, Command, DocumentCache, EditorSession, FormulaCache, GridCache, - Projection, Rejection, Snapshot, TileBarCache, ViewListCache, ViewState, WidgetCache, - reduce_view, settled_mode, + ModelCommand, Projection, Rejection, Snapshot, TileBarCache, ViewListCache, ViewState, + WidgetCache, reduce_view, settled_mode, }; +use improvise_core::model::record::RecordHash; use serde::Serialize; /// One client's whole state: the portable session slice plus one cache per @@ -49,6 +50,50 @@ pub struct ClientState { pub status_seen: String, #[serde(skip)] pub status_owner: Option, + /// The cell an open editor commits to, captured as the user saw it. + /// When it is taken, and when not yet: `context/code/clients.md`. + #[serde(skip)] + pub opened: Option, +} + +/// What an open cell editor commits to. After a refusal the retry may name only +/// the cell it named (`Retry`) or none (`Gone`): improvise-20ek, -9ww8, -8phw. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Opened { + Target(OpenedCell), + Retry(RecordHash, OpenedTarget), + Gone(RecordHash, OpenedTarget), +} + +impl Opened { + /// Which kind of cell the editor commits to, whether or not it is captured. + pub fn target(&self) -> &OpenedTarget { + match self { + Opened::Target(cell) => &cell.target, + Opened::Retry(_, target) | Opened::Gone(_, target) => target, + } + } +} + +/// A cell editor's target: the record, which cell of it, the record's write +/// counter at open, and (for a rename) every counter shown: improvise-8xq0, -milr. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OpenedCell { + pub record: RecordHash, + pub target: OpenedTarget, + pub version: u64, + pub seen: Vec<(RecordHash, u64)>, + /// The document the editor opened on, so a swap refuses it (improvise-vndi). + pub document_generation: u64, +} + +/// Which cell of the record an editor opened on, as the projection named it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum OpenedTarget { + /// A stored value: `SetCell`/`ClearCell` names it (improvise-8xq0). + Value { measure: Option }, + /// A records-mode coordinate: `RenameRecordCoordinate` names it. + Coordinate { category: String }, } /// One frame this client has sent. `awaited` is true exactly when the host @@ -84,6 +129,7 @@ impl ClientState { } self.settle_mode(); self.settle_editor(); + self.settle_opened(); self.settle_status(); self.revision += 1; } @@ -105,6 +151,7 @@ impl ClientState { self.replay(); self.settle_mode(); self.settle_editor(); + self.settle_opened(); self.settle_status(); self.revision += 1; } @@ -139,6 +186,7 @@ impl ClientState { } self.settle_mode(); self.settle_editor(); + self.settle_opened(); self.settle_status(); self.revision += 1; } @@ -174,6 +222,7 @@ impl ClientState { } self.settle_sent(); self.settle_editor(); + self.settle_opened(); self.revision += 1; } @@ -183,13 +232,15 @@ impl ClientState { // A refused *save* answers no `Commands` frame, so it must not consume // one's entry — `Rejection::refused_an_edit` is that question. if rejection.refused_an_edit() { - self.in_flight.pop_front(); + let dropped = self.in_flight.pop_front(); self.settle_sent(); self.replay(); + self.opened = written_to(dropped.as_ref()).map(|(r, t)| Opened::Retry(r, t)); } self.last_rejection = Some(rejection); self.settle_mode(); self.settle_editor(); + self.settle_opened(); self.settle_status(); self.revision += 1; } @@ -233,6 +284,50 @@ impl ClientState { self.view.mode = settled_mode(&self.view.mode, self.grid.records); } + /// Whether a frame the host will answer is still in flight, so this + /// client's cache does not yet show the host's answer to its own writes. + pub(crate) fn awaiting_answer(&self) -> bool { + self.in_flight.iter().any(|sent| sent.awaited) + } + + /// Capture the cell an open editor commits to, but only from a cache that + /// shows the host's answer to this client's own writes (improvise-fdk1). + fn settle_opened(&mut self) { + let answered = !self.awaiting_answer(); + self.opened = match (self.view.mode.is_editing(), self.opened.take()) { + (false, _) => None, + (true, Some(Opened::Retry(record, target))) if answered => { + Some(match self.captured() { + Some(cell) if cell.record == record && cell.target == target => { + Opened::Target(cell) + } + _ => Opened::Gone(record, target), + }) + } + (true, Some(opened)) => Some(opened), + (true, None) if answered => self.captured().map(Opened::Target), + (true, None) => None, + }; + } + + /// The cell under the cursor, with its counter; a rename also keeps every + /// counter this client holds, so its destination is claimed (improvise-milr). + fn captured(&self) -> Option { + let (record, target) = crate::resolve::opened_at_cursor(self)?; + let basis = self.basis.current()?; + let seen = match target { + OpenedTarget::Coordinate { .. } => basis.records.clone(), + OpenedTarget::Value { .. } => Vec::new(), + }; + Some(OpenedCell { + record, + target, + version: basis.record(record)?, + seen, + document_generation: basis.document_generation, + }) + } + /// An editor that arrived without its buffer is seeded here, so what is /// drawn and what Enter commits read one buffer (improvise-ktlk). fn settle_editor(&mut self) { @@ -248,10 +343,11 @@ impl ClientState { self.view.opened_editor = Some(crate::OpenedEditor::on(seed)); } self.settle_awaited_seed(); + self.settle_declined_editor(); } - /// An editor opened after a write takes its seed from the grid the host - /// answered with, never from the cache the write made stale (improvise-3uzg). + /// An editor opened after a write, or on a cell not projected yet, takes its + /// seed once both are answered, and never over typing (improvise-3uzg/7mro). fn settle_awaited_seed(&mut self) { let waiting = self.view.mode.is_editing() && self @@ -260,7 +356,8 @@ impl ClientState { .as_ref() .is_some_and(|opened| opened.awaiting && !opened.touched); let answered = !self.in_flight.iter().any(|sent| sent.awaited); - if let (true, true, Some(seed)) = (waiting, answered, crate::resolve::edit_seed(self)) { + if let (true, true, Some(seed)) = (waiting, answered, crate::resolve::projected_seed(self)) + { self.view.buffers.insert("edit".to_string(), seed.clone()); self.view.opened_editor = Some(crate::OpenedEditor::on(seed)); } @@ -278,6 +375,22 @@ impl ClientState { } } + /// An untouched editor on a cell `i` declines closes once the host projects + /// that cell: an advance past the window opens before it can know (5m9z). + fn settle_declined_editor(&mut self) { + let (row, col) = self.view.cursor().selected; + let untouched = self.view.mode.is_editing() + && self + .view + .opened_editor + .as_ref() + .is_some_and(|opened| !opened.touched); + if let (true, Some(refusal)) = (untouched, crate::resolve::declined_at(self, row, col)) { + self.view.mode = settled_mode(&crate::AppMode::Normal, self.grid.records); + self.view.status_msg = refusal; + } + } + /// The widget dispatch: a projection names the widget it is addressed to. /// The `CategoryTreeCache`/`TileBarCache` overlap is deliberate (gotcha 32). fn apply_projection(&mut self, projection: &Projection) { @@ -293,6 +406,31 @@ impl ClientState { } } +/// The cell a refused frame wrote, if it wrote one: a rename's coordinate or a +/// value's record and measure (improvise-9ww8, improvise-8phw). +fn written_to(sent: Option<&Sent>) -> Option<(RecordHash, OpenedTarget)> { + sent?.cmds.iter().find_map(|cmd| match cmd { + Command::Model { cmd, .. } => match cmd { + ModelCommand::RenameRecordCoordinate { + record, category, .. + } => Some(( + *record, + OpenedTarget::Coordinate { + category: category.clone(), + }, + )), + ModelCommand::SetCell { cell, .. } | ModelCommand::ClearCell(cell) => Some(( + cell.record, + OpenedTarget::Value { + measure: cell.measure.clone(), + }, + )), + _ => None, + }, + _ => None, + }) +} + #[cfg(test)] mod tests { use super::*; @@ -577,6 +715,10 @@ mod tests { fn answered_with(state: &mut ClientState, edit_text: &str) { let mut cell = CellDisplay::number(edit_text); cell.edit_text = Some(edit_text.into()); + cell.id = Some(crate::CellId::Record(crate::RecordCell { + record: serde_json::from_str("\"fe4c8e5e9cbd578e0d388f4b2b5a7879\"").unwrap(), + measure: Some("Revenue".into()), + })); for cmd in [ GridUpdate::RowFrame(AxisFrame::flat(["North"])), GridUpdate::ColFrame(AxisFrame::flat(["Revenue"])), diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index fdef9860..97629b2d 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -27,7 +27,9 @@ pub use frame::{ClientFrame, ServerFrame}; pub use help::{HelpLine, Hint, hint_line, hints, mode_label}; pub use key::{KeyCode, KeyEvent, KeyModifiers}; pub use keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey}; -pub use reduce::{Command, HelpPage, ModelCommand, records_named, reduce_view}; +pub use reduce::{ + Command, HelpPage, ModelCommand, RECORD_COORDS_CANNOT_BE_EMPTY, records_named, reduce_view, +}; pub use render::{ AxisFrame, CategoryRow, CategoryTreeCache, CategoryTreeUpdate, CellDisplay, CellId, CreateKind, DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, GridCache, GridExtent, diff --git a/crates/improvise-protocol/src/reduce.rs b/crates/improvise-protocol/src/reduce.rs index 9806c553..fea82b5d 100644 --- a/crates/improvise-protocol/src/reduce.rs +++ b/crates/improvise-protocol/src/reduce.rs @@ -79,7 +79,7 @@ pub enum Command { Project(Projection), } -/// A mutation of the shared document: 25 variants, derived not invented (§2). A +/// A mutation of the shared document: 26 variants, derived not invented (§2). A /// view-configuration variant **names its view** (improvise-lltt). #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum ModelCommand { @@ -90,6 +90,13 @@ pub enum ModelCommand { value: CellValue, }, ClearCell(RecordCell), + /// Move a whole record to a new coordinate, creating the item if need be. + /// Position-free: it names the record, never a row (improvise-fdk1). + RenameRecordCoordinate { + record: RecordHash, + category: String, + item: String, + }, AddItem { category: String, item: String, @@ -176,11 +183,16 @@ pub enum ModelCommand { }, } +/// Why a rename to an empty coordinate is refused. One home, read by the host's +/// refusal and by a client composing the same answer (improvise-fdk1). +pub const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates cannot be empty"; + /// Which records a [`ModelCommand`] writes. The **one** home: the server /// checks against it and a client claims by it — `context/code/protocol.md` §2. pub fn records_named(cmd: &ModelCommand) -> Vec { match cmd { ModelCommand::SetCell { cell, .. } | ModelCommand::ClearCell(cell) => vec![cell.record], + ModelCommand::RenameRecordCoordinate { record, .. } => vec![*record], ModelCommand::AddItem { .. } | ModelCommand::AddItemInGroup { .. } @@ -623,6 +635,11 @@ mod tests { match cmd { ModelCommand::SetCell { cell, value: v } => record(cell) + value(v), ModelCommand::ClearCell(cell) => record(cell), + ModelCommand::RenameRecordCoordinate { + record: r, + category, + item, + } => surrogate(r) + name(category) + name(item), ModelCommand::AddItem { category, item } | ModelCommand::RemoveItem { category, item } => name(category) + name(item), ModelCommand::AddItemInGroup { diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index dc002d01..b8448f00 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -57,9 +57,12 @@ pub enum CellId { /// A records-mode creating column: typing a **name** into it adds that /// measure or category and writes no cell at all (gotcha 1, improvise-poia). Create(CreateKind), - /// A records-mode coordinate. Renaming it moves every measure cell in the - /// row (gotcha 1), which no client composes: `protocol.md` §3 (improvise-exdg). - Coordinate, + /// A records-mode coordinate: the row's record and the category this column + /// names. Renaming it moves the whole record — `protocol.md` §3 (improvise-fdk1). + Coordinate { + record: RecordHash, + category: String, + }, /// A stored cell whose `category` coordinate names no item of it, so no write /// can address it: `context/code/command.md` (improvise-gk51). NonItem { category: String, item: String }, diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 5152b650..5f83ef90 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -4,13 +4,14 @@ use improvise_core::view::Axis; use serde::{Deserialize, Serialize}; -use crate::client::ClientState; +use crate::client::{ClientState, Opened, OpenedCell, OpenedTarget}; use crate::frame::ClientFrame; use crate::keymap::{Binding, Keymaps, ModeKey}; use crate::viewmodel::{CatTreeEntry, axis_change_status, flatten_cat_tree}; use crate::{ AppMode, CellId, CellValue, Command, CreateKind, FormulaId, FormulaSlot, GridExtent, HelpPage, - KeyCode, KeyEvent, ModelCommand, Panel, RecordCell, ViewState, records_named, + KeyCode, KeyEvent, ModelCommand, Panel, RECORD_COORDS_CANNOT_BE_EMPTY, RecordCell, ViewState, + records_named, }; /// The virtual category a new measure is filed under, spelled as @@ -313,16 +314,48 @@ enum Step { fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Step { match requested(name) { Some(request) => Step::Request(request), - None => match declined(name, args) { + None => match declined(name, args).or_else(|| refused_here(state, name)) { Some(said) => Step::Declined(vec![Command::SetStatus(said)]), - None => match composed(state, event, name, args) { - Some(commands) => Step::Local(commands), - None => Step::Upstream, + None => match committed(state, name, args) { + Some(answer) => answer, + None => match composed(state, event, name, args) { + Some(commands) => Step::Local(commands), + None => Step::Upstream, + }, }, }, } } +/// The two cell commits, answered here rather than in [`composed`] because one +/// of their answers is a decline, which stops a sequence (improvise-joed). +fn committed(state: &ClientState, name: &str, args: &[String]) -> Option { + let advance = match name { + "commit-cell-edit" => Advance::Down, + "commit-and-advance-right" => Advance::Right, + _ => return None, + }; + let edit_mode = args.first().and_then(|mode| AppMode::from_name(mode)); + Some( + edit_mode + .and_then(|mode| commit(state, mode, advance)) + .unwrap_or(Step::Upstream), + ) +} + +/// A commit onto a cell `i` declines is refused as `committable_cell_key` refuses +/// it (ik6l); a cell op on an unprojected cell waits (7mro). Not a captured commit (3h9w). +fn refused_here(state: &ClientState, name: &str) -> Option { + let (row, col) = state.view.cursor().selected; + let waiting = unprojected(state, row, col).then(|| NOT_PROJECTED.to_string()); + match name { + "commit-cell-edit" | "commit-and-advance-right" if state.opened.is_some() => None, + "commit-cell-edit" | "commit-and-advance-right" => declined_at(state, row, col).or(waiting), + "clear-cell" | "yank" | "paste" => waiting, + _ => None, + } +} + /// The command names this client answers by **asking its host**, rather than /// by composing anything. See `context/code/clients.md` (improvise-jz6w). fn requested(name: &str) -> Option { @@ -478,10 +511,6 @@ fn composed( // ── editing ────────────────────────────────────────────────── "edit-or-drill" => edit_or_drill(state, AppMode::from_name(args.first()?)?), - "commit-cell-edit" => commit(state, AppMode::from_name(args.first()?)?, Advance::Down), - "commit-and-advance-right" => { - commit(state, AppMode::from_name(args.first()?)?, Advance::Right) - } // ── the category panel ─────────────────────────────────────── "cycle-axis-at-cursor" => Some(match cat_at_cursor(state) { @@ -835,28 +864,30 @@ fn tile_cursor(state: &ClientState, delta: i32) -> Option> { // ── editing a cell ────────────────────────────────────────────────────── -/// The cell under the cursor in this viewport-indexed cache. `None` when the -/// selection is outside the window — reachable only by a projection race. +/// The cell under the cursor, as the host projected it. `None` outside the +/// window the cache holds, which the cursor's own offsets may have left (7mro). fn cell_at_cursor(state: &ClientState) -> Option<&crate::CellDisplay> { - let cursor = state.view.cursor(); - let row = cursor.selected.0.checked_sub(cursor.row_offset)?; - let col = cursor.selected.1.checked_sub(cursor.col_offset)?; - state.grid.cell(row, col) + let (row, col) = state.view.cursor().selected; + projected_at(state, row, col) } /// `EditOrDrill::execute`, with ADR 002 answering the branch, and drill /// declining out loud rather than upstream. See `context/code/clients.md`. fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { // The grid, not the mode: a records GRID is the observed fact and the mode - // follows it (improvise-bd9o, `context/code/clients.md`). - if state.grid.records { + // follows it (improvise-bd9o). A coordinate is the one records cell it commits. + if state.grid.records && coordinate_at_cursor(state).is_none() { return Some(vec![Command::SetStatus(not_yet("editing in records mode"))]); } + let (row, col) = state.view.cursor().selected; + if unprojected(state, row, col) { + return Some(awaiting_seed(mode)); + } let cell = cell_at_cursor(state)?; match cell.id.as_ref()? { // `edit_text`, never `text`: the drawn text has been through // `format_f64`. `tests/lockstep.rs` found exactly that. - CellId::Record(_) => Some(opened_on( + CellId::Record(_) | CellId::Coordinate { .. } => Some(opened_on( state, cell.edit_text.clone().unwrap_or_default(), mode, @@ -869,13 +900,49 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { // Both creating columns are records-only, so the guard above takes // them; declining rather than `None` keeps that a door, not a hole. CellId::Create(_) => Some(vec![Command::SetStatus(not_yet("creating a column"))]), - CellId::Coordinate => Some(vec![Command::SetStatus(not_yet("renaming a coordinate"))]), CellId::NonItem { category, item } => Some(vec![Command::SetStatus( crate::non_item_refusal(category, item), )]), } } +/// The record and category of the coordinate cell under the cursor, as the +/// projection names them — never borrowed from a sibling cell (ADR 002). +fn coordinate_at_cursor(state: &ClientState) -> Option<(crate::RecordHash, String)> { + match cell_at_cursor(state)?.id.as_ref()? { + CellId::Coordinate { record, category } => Some((*record, category.clone())), + CellId::Record(_) + | CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::NonItem { .. } => None, + } +} + +/// The record and cell an editor opened on the cursor commits to, as the +/// projection names them (improvise-8xq0). `None` for a cell no editor writes. +pub(crate) fn opened_at_cursor(state: &ClientState) -> Option<(crate::RecordHash, OpenedTarget)> { + match cell_at_cursor(state)?.id.as_ref()? { + CellId::Record(cell) if !state.grid.records => Some(( + cell.record, + OpenedTarget::Value { + measure: cell.measure.clone(), + }, + )), + CellId::Coordinate { record, category } => Some(( + *record, + OpenedTarget::Coordinate { + category: category.clone(), + }, + )), + CellId::Record(_) + | CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::NonItem { .. } => None, + } +} + /// Open the cell editor on `seed`, or await it while any write is unanswered: the /// cache predates that write (improvise-mdfc/p6v6, `protocol.md` §3). fn opened_on(state: &ClientState, seed: String, mode: AppMode) -> Vec { @@ -898,6 +965,19 @@ fn seeded(seed: String, mode: AppMode) -> Vec { ] } +/// Open the cell editor with its seed still to come — after a write, or on a cell +/// not shown yet — settled by the fold once both are answered (3uzg/7mro). +fn awaiting_seed(mode: AppMode) -> Vec { + vec![ + Command::SetBuffer { + name: EDIT_BUFFER.to_string(), + value: String::new(), + }, + Command::SetOpenedEditor(Some(OpenedEditor::awaiting())), + Command::ChangeMode(mode), + ] +} + /// One direction of the view history. An empty stack is answered out loud: /// `reduce_view`'s own arm is silent there — `context/code/clients.md`. fn step_history(stack: &[crate::ViewFrame], step: Command, empty: &str) -> Vec { @@ -989,29 +1069,154 @@ enum Advance { /// `CommitAndAdvance::execute`, for the one case a thin client can compose: a /// plain grid cell. The three-way fork and the mirror order: clients.md. -fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option> { +fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option { let (window_rows, window_cols) = state.window; if window_rows == 0 || window_cols == 0 { return None; } let text = read_buffer(&state.view, "edit"); - let id = cell_at_cursor(state)?.id.as_ref()?; - // Records mode needs `record_col` (gotcha 1) — except the two CREATING - // columns, which name nothing but themselves (improvise-gcvb). - if let CellId::Create(kind) = id { - return create_column(state, edit_mode, advance, kind, &text); + // What the editor captured says what it writes, never the cell the cursor + // is on now (improvise-8xq0, Codex on PR #274). + match state.opened.as_ref().map(Opened::target) { + Some(OpenedTarget::Value { .. }) => commit_value(state, edit_mode, advance, &text), + Some(OpenedTarget::Coordinate { .. }) => { + rename_coordinate(state, edit_mode, advance, &text) + } + None => uncaptured(state, edit_mode, advance, &text), } - if state.grid.records { - return None; +} + +/// An editor that captured nothing: the cell under the cursor says what it is. +/// Records mode needs `record_col` for a measure cell (gotcha 1, improvise-gcvb). +fn uncaptured( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + text: &str, +) -> Option { + match cell_at_cursor(state)?.id.as_ref()? { + CellId::Create(kind) => { + create_column(state, edit_mode, advance, kind, text).map(Step::Local) + } + CellId::Coordinate { .. } => rename_coordinate(state, edit_mode, advance, text), + CellId::Record(_) if !state.grid.records => commit_value(state, edit_mode, advance, text), + CellId::Record(_) + | CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::NonItem { .. } => None, } - let mut commands = written(state, id, &text)?; +} + +/// `CommitAndAdvance` over a stored value: write the cell the editor OPENED on, +/// or decline when a refusal left it no longer there (improvise-8phw). +fn commit_value( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + text: &str, +) -> Option { + let unchanged = untouched(state.view.opened_editor.as_ref(), text); + let mut commands = match (unchanged, state.opened.as_ref()) { + (true, _) => Vec::new(), + (false, Some(Opened::Target(opened))) => written(state, opened, text)?, + (false, Some(Opened::Gone(..))) => return Some(renamed_away()), + (false, None | Some(Opened::Retry(..))) => { + return state.awaiting_answer().then(still_waiting); + } + }; commands.push(Command::ChangeMode(AppMode::Normal)); commands.extend(cleared(&edit_mode)); - let writes = !untouched(state.view.opened_editor.as_ref(), &text); + let writes = !untouched(state.view.opened_editor.as_ref(), text); commands.extend(advanced(state, edit_mode, advance, writes)?); - Some(commands) + Some(Step::Local(commands)) } +/// Why a retry after a refusal sends nothing: the cell it opened on is no longer +/// the one under the cursor, or is gone (improvise-20ek, -9ww8, -8phw). +const RECORD_RENAMED_AWAY: &str = "the row you were editing has changed; Esc to leave the editor"; + +/// A retry whose cell is gone. Declined, so no welded clear takes the text the +/// user must still Esc away from (improvise-joed, until #227 drops the weld). +fn renamed_away() -> Step { + Step::Declined(vec![Command::SetStatus(RECORD_RENAMED_AWAY.to_string())]) +} + +/// `CommitAndAdvance` over a coordinate: rename the record the editor OPENED on, +/// at the version it was shown — `context/code/clients.md` (improvise-fdk1). +fn rename_coordinate( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + text: &str, +) -> Option { + // Untouched writes nothing (improvise-ixxx); empty is refused as the host + // refuses it, where composing the write would undo the whole frame. + let unchanged = untouched(state.view.opened_editor.as_ref(), text); + let mut commands = match (unchanged, text.is_empty(), state.opened.as_ref()) { + (true, _, _) => Vec::new(), + (false, true, _) => vec![Command::SetStatus( + RECORD_COORDS_CANNOT_BE_EMPTY.to_string(), + )], + (false, false, None | Some(Opened::Retry(..))) => return Some(still_waiting()), + (false, false, Some(Opened::Gone(..))) => return Some(renamed_away()), + (false, false, Some(Opened::Target(opened))) => { + let OpenedTarget::Coordinate { category } = &opened.target else { + return Some(still_waiting()); + }; + vec![Command::Model { + basis: state + .basis + .claim_as_read(opened.document_generation, claimed(opened))?, + cmd: ModelCommand::RenameRecordCoordinate { + record: opened.record, + category: category.clone(), + item: text.to_string(), + }, + }] + } + }; + let writes = commands.iter().any(|c| matches!(c, Command::Model { .. })); + commands.push(Command::ChangeMode(AppMode::Normal)); + commands.extend(cleared(&edit_mode)); + commands.extend(advanced(state, edit_mode, advance, writes)?); + Some(Step::Local(commands)) +} + +/// The rename's claim: its own record, then every record shown when the editor +/// opened, so the destination is claimed if seen (improvise-milr). +fn claimed(opened: &OpenedCell) -> impl Iterator + '_ { + let others = opened.seen.iter().copied(); + std::iter::once((opened.record, opened.version)) + .chain(others.filter(|(h, _)| *h != opened.record)) +} + +/// The write to the value cell the editor OPENED on, at the version it read +/// then (improvise-8xq0, improvise-xz2z). +fn written(state: &ClientState, opened: &OpenedCell, text: &str) -> Option> { + let OpenedTarget::Value { measure } = &opened.target else { + return None; + }; + let cell = RecordCell { + record: opened.record, + measure: measure.clone(), + }; + Some(vec![Command::Model { + basis: state.basis.claim_as_read( + opened.document_generation, + [(opened.record, opened.version)], + )?, + cmd: typed_cell_command(&CellId::Record(cell), text)?, + }]) +} + +/// A commit whose target is not captured yet, because the host has not answered +/// this client's own last write. Declined, so no welded clear runs (improvise-joed). +fn still_waiting() -> Step { + Step::Declined(vec![Command::SetStatus(STILL_WAITING.to_string())]) +} + +const STILL_WAITING: &str = "still waiting for the host to answer your last edit"; + /// The commit's own clear of `edit_mode`'s buffer, at its head: the keymap /// welds none to these keys, so a re-seed survives (improvise-fy9o). fn cleared(edit_mode: &AppMode) -> Option { @@ -1020,18 +1225,6 @@ fn cleared(edit_mode: &AppMode) -> Option { .map(|mb| write_buffer(&mb.buffer_key, String::new())) } -/// The write a commit makes: none when the buffer is the seed the editor -/// captured as it opened, and only the write is skipped (improvise-ixxx). -fn written(state: &ClientState, id: &CellId, text: &str) -> Option> { - match untouched(state.view.opened_editor.as_ref(), text) { - true => Some(Vec::new()), - false => Some(vec![Command::Model { - basis: state.basis.claim([record_at_cursor(state)?])?, - cmd: typed_cell_command(id, text)?, - }]), - } -} - /// `CommitAndAdvance` over a creating column: a schema change over the typed /// NAME. Why +cat does not advance: clients.md (improvise-gcvb). fn create_column( @@ -1104,51 +1297,24 @@ fn advanced( Some(commands) } -/// Open the cell editor with its seed still to come: the client settles it from -/// the host's answer, once nothing this client sent is unanswered (improvise-3uzg). -fn awaiting_seed(mode: AppMode) -> Vec { - vec![ - Command::SetBuffer { - name: EDIT_BUFFER.to_string(), - value: String::new(), - }, - Command::SetOpenedEditor(Some(OpenedEditor::awaiting())), - Command::ChangeMode(mode), - ] -} - -/// The surrogate naming the record under the cursor, or `None` when the -/// cursor is not on a writable cell. -fn record_at_cursor(state: &ClientState) -> Option { - match cell_at_cursor(state)?.id.as_ref()? { - CellId::Record(cell) => Some(cell.record), - CellId::Aggregate(_) - | CellId::Derived { .. } - | CellId::Create(_) - | CellId::Coordinate - | CellId::NonItem { .. } => None, - } -} - /// What an editor opened on the cursor starts with, or `None` with no cell /// there to seed from. Read by the fold, never a renderer (improvise-ktlk). pub(crate) fn edit_seed(state: &ClientState) -> Option { cell_at_cursor(state)?.edit_text.clone() } -/// The display text at an absolute grid position, as the cache holds it. -fn seed_at(state: &ClientState, row: usize, col: usize) -> String { - cell_at(state, row, col) - .and_then(|cell| cell.edit_text.clone()) - .unwrap_or_default() +/// What an editor awaiting its seed takes once the host projects the cell under +/// the cursor: `None` until then, a relabel's blank included (improvise-7mro). +pub(crate) fn projected_seed(state: &ClientState) -> Option { + let cell = cell_at_cursor(state).filter(|cell| cell.id.is_some())?; + Some(cell.edit_text.clone().unwrap_or_default()) } -/// The projected cell at an absolute grid position, `None` outside the window. -fn cell_at(state: &ClientState, row: usize, col: usize) -> Option<&crate::CellDisplay> { - let cursor = state.view.cursor(); - let r = row.checked_sub(cursor.row_offset)?; - let c = col.checked_sub(cursor.col_offset)?; - state.grid.cell(r, c) +/// The edit text the host projected at an absolute grid position. +fn seed_at(state: &ClientState, row: usize, col: usize) -> String { + projected_at(state, row, col) + .and_then(|cell| cell.edit_text.clone()) + .unwrap_or_default() } /// `EnterEditAtCursor`: re-open the editor where the advance landed, unless `i` @@ -1160,19 +1326,48 @@ fn reopened_at( mode: AppMode, writes: bool, ) -> Vec { - match cell_at(state, row, col).and_then(|cell| cell.id.as_ref()) { - Some(CellId::Aggregate(_)) => vec![Command::SetStatus(AGGREGATE_DECLINE.to_string())], - Some(CellId::Derived { target }) => vec![Command::SetStatus(derived_decline(target))], - Some(CellId::NonItem { category, item }) => { - vec![Command::SetStatus(crate::non_item_refusal(category, item))] - } - Some(CellId::Record(_) | CellId::Create(_) | CellId::Coordinate) | None => match writes { - true => awaiting_seed(mode), - false => opened_on(state, seed_at(state, row, col), mode), - }, + match (declined_at(state, row, col), writes) { + (Some(refusal), _) => vec![Command::SetStatus(refusal)], + (None, true) => awaiting_seed(mode), + (None, false) if unprojected(state, row, col) => awaiting_seed(mode), + (None, false) => opened_on(state, seed_at(state, row, col), mode), } } +/// Why `i` declines the cell the host projected at `(row, col)`. `None` for an +/// editable cell, and for one not projected yet, which the fold settles later. +pub(crate) fn declined_at(state: &ClientState, row: usize, col: usize) -> Option { + match projected_at(state, row, col)?.id.as_ref()? { + CellId::Aggregate(_) => Some(AGGREGATE_DECLINE.to_string()), + CellId::Derived { target } => Some(derived_decline(target)), + CellId::NonItem { category, item } => Some(crate::non_item_refusal(category, item)), + CellId::Record(_) | CellId::Create(_) | CellId::Coordinate { .. } => None, + } +} + +/// The cell the host projected at an absolute position, read against the frame's +/// own origin: offsets the client has scrolled ahead of the cache cannot alias it. +fn projected_at(state: &ClientState, row: usize, col: usize) -> Option<&crate::CellDisplay> { + let r = row.checked_sub(state.grid.row_frame.first)?; + let c = col.checked_sub(state.grid.col_frame.first)?; + state.grid.cell(r, c) +} + +/// Whether `(row, col)` is a cell of the grid that the window the cache holds +/// does not cover: unknown, never the cell at the same cache index (7mro). +pub(crate) fn unprojected(state: &ClientState, row: usize, col: usize) -> bool { + let GridExtent { rows, cols } = state.grid.extent; + let outside = |frame: &crate::AxisFrame, at: usize| { + !(frame.first..frame.first + frame.len()).contains(&at) + }; + row < rows + && col < cols + && (outside(&state.grid.row_frame, row) || outside(&state.grid.col_frame, col)) +} + +/// What a cell op says while its cell is still unknown to this client. +const NOT_PROJECTED: &str = "waiting for the host to show this cell"; + /// What the host says of an aggregate, the fold of a collapsed group included. const AGGREGATE_DECLINE: &str = "Cannot write to an aggregate — it combines several records, so there is no one cell to write"; @@ -1189,8 +1384,8 @@ pub struct OpenedEditor { pub seed: String, /// Whether the user has edited the buffer since it opened (improvise-2054). pub touched: bool, - /// The seed is still to come from the host's answer (improvise-3uzg). Not - /// `serde(default)`: `false` is a claim — `protocol.md` §3, improvise-mgis. + /// The seed is still to come, from a write's answer or an unprojected cell + /// (3uzg/7mro). Not `serde(default)`: `false` is a claim (`protocol.md` §3, mgis). pub awaiting: bool, } @@ -1278,7 +1473,7 @@ fn record_cell_at_cursor(state: &ClientState) -> Option { CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) - | CellId::Coordinate + | CellId::Coordinate { .. } | CellId::NonItem { .. } => None, } } @@ -2402,7 +2597,7 @@ mod tests { // A records MODE over a non-records GRID is a state no host can // produce, so the fixture projects both (improvise-e5pf). crate::WidgetCache::apply(&mut state.grid, &GridUpdate::Records(mode.is_records())); - state.view.mode = mode; + state.fold(&Command::ChangeMode(mode)); state.grid.page_coords = vec![crate::PageAxis { cat: "Quarter".into(), selected: "Q2".into(), @@ -3328,7 +3523,7 @@ mod tests { // Half two: the predicate is about *finishing*, so something has to // press Enter and observe. let mut client = client_on(record_cell()); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), "42".into()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { panic!("Enter in Editing must resolve here — that is the arm"); @@ -3583,7 +3778,7 @@ mod tests { Projection::Grid(GridUpdate::ColFrame(AxisFrame::flat(["Revenue"]))), Projection::Grid(GridUpdate::Cells(vec![ ((0, 0), first), - ((1, 0), CellDisplay::number("900")), + ((1, 0), CellDisplay::number("900").with_id(record_cell())), ])), Projection::Grid(GridUpdate::Extent(GridExtent { rows: 2, cols: 1 })), Projection::Basis(published()), @@ -3825,12 +4020,75 @@ mod tests { ); } + /// improvise-8xq0: a commit whose target is not captured yet, because the + /// last write is unanswered, declines out loud. Evidence file. + #[test] + fn a_value_commit_before_the_host_answers_the_last_one_waits_out_loud() { + let client = reopened_before_the_host_answers(); + assert_eq!(client.opened, None, "setup: nothing captured yet"); + let KeyOutcome::Local(second) = resolve(&client, ev(KeyCode::Enter)) else { + panic!("a commit with no target yet must answer, not go upstream"); + }; + assert_eq!( + second[..1], + [Command::SetStatus(STILL_WAITING.into())], + "it says so: {second:?}" + ); + assert!( + !second.iter().any(|c| matches!(c, Command::Model { .. })), + "and it must write nothing: {second:?}" + ); + } + + /// A commit, sent and unanswered, whose advance reopened the editor on a + /// stored cell, where the user has since typed `43`. + fn reopened_before_the_host_answers() -> ClientState { + let mut client = client_on(record_cell()); + // The row the advance lands on is a stored cell too, so the commit gets + // past the cursor's cell and reaches the capture. + let mut below = CellDisplay::number("900"); + below.id = Some(record_cell()); + client.fold(&Command::Project(Projection::Grid(GridUpdate::Cells( + vec![((1, 0), below)], + )))); + client.fold(&Command::ChangeMode(AppMode::editing())); + client.view.buffers.insert("edit".into(), "42".into()); + let KeyOutcome::Local(first) = resolve(&client, ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + client.fold_outgoing(&first); + assert!(client.view.mode.is_editing(), "setup: the advance reopened"); + client.view.buffers.insert("edit".into(), "43".into()); + client + } + + /// improvise-joed: the waiting decline kept the editor open, but the key's + /// welded `clear-buffer` still ran and lost the text. Evidence file. + #[test] + fn a_commit_that_waits_for_the_host_keeps_the_typed_text() { + let mut client = reopened_before_the_host_answers(); + let KeyOutcome::Local(second) = resolve(&client, ev(KeyCode::Enter)) else { + panic!("a commit with no target yet must answer, not go upstream"); + }; + client.fold_outgoing(&second); + assert_eq!( + client.view.status_msg, STILL_WAITING, + "setup: this is the waiting decline" + ); + assert!(client.view.mode.is_editing(), "the editor stays open"); + assert_eq!( + client.view.buffers.get("edit").map(String::as_str), + Some("43"), + "the text the user typed must survive the decline: {second:?}" + ); + } + /// Enter in `Editing` is `[commit-cell-edit editing, clear-buffer edit]` /// and every part shows up here — including the odd-looking trailing clear. #[test] fn enter_commits_the_cell_advances_and_re_enters_editing() { let mut client = client_on(record_cell()); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), "42".into()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { panic!("Enter in Editing must resolve here"); @@ -3894,7 +4152,7 @@ mod tests { Projection::Grid(GridUpdate::Cells(vec![ ((0, 0), CellDisplay::number("900")), ((1, 0), cell), - ((2, 0), CellDisplay::number("800")), + ((2, 0), CellDisplay::number("800").with_id(record_cell())), ])), Projection::Grid(GridUpdate::Extent(GridExtent { rows: 3, cols: 1 })), Projection::Basis(published()), @@ -4201,7 +4459,7 @@ mod tests { #[test] fn committing_an_empty_buffer_clears_the_cell() { let mut client = client_on(record_cell()); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), String::new()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { panic!("Enter in Editing must resolve here"); @@ -4280,7 +4538,7 @@ mod tests { ], }); client.window = (1, 1); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), "7".into()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { @@ -4524,6 +4782,25 @@ mod tests { `KeyOutcome::Request` has nowhere to put one" ); } + + /// improvise-7mro: an empty grid has no cell to wait for, so nothing a cell op + /// says there is a wait. See `context/test-evidence/clients.md`. + #[test] + fn a_cursor_on_an_empty_grid_waits_for_nothing() { + let mut state = in_mode(AppMode::Normal); + state.window = (2, 2); + for key in ['i', 'x'] { + let said = match resolve(&state, ev(KeyCode::Char(key))) { + KeyOutcome::Local(cmds) => cmds, + _ => Vec::new(), + }; + assert!( + !said.contains(&Command::SetStatus(NOT_PROJECTED.to_string())) + && !said.contains(&Command::SetOpenedEditor(Some(OpenedEditor::awaiting()))), + "{key}: waits on a cell no grid has: {said:?}" + ); + } + } } #[cfg(test)] diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index 83efb7e9..88ae7c0e 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -201,7 +201,7 @@ impl Client { CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) - | CellId::Coordinate + | CellId::Coordinate { .. } | CellId::NonItem { .. } => None, } } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index cb90c310..146d7035 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -150,7 +150,7 @@ impl RenderCell { CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) - | CellId::Coordinate + | CellId::Coordinate { .. } | CellId::NonItem { .. }, ) | None => false, @@ -700,7 +700,10 @@ mod tests { ((1, 0), CellDisplay::text("pending")), ( (1, 2), - CellDisplay::text("North").with_id(CellId::Coordinate), + CellDisplay::text("North").with_id(CellId::Coordinate { + record: surrogate(), + category: "Region".into(), + }), ), ])), ], diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 1794d7fd..798412a6 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -500,6 +500,7 @@ fn the_tile_bar_route_into_records_mode_does_not_strand_the_user() { cols >= 3, "the fixture must reach past the coordinate column" ); + let mut coordinates = 0; for col in 0..cols { walk_to( &mut host, @@ -518,6 +519,17 @@ fn the_tile_bar_route_into_records_mode_does_not_strand_the_user() { // a keypress this client might decline. client.state_mut().view.status_msg = "\u{0}unsaid".to_string(); press(&mut host, id, &mut client, "i"); + // A coordinate's Enter composes a rename since improvise-fdk1, so its + // editor is a door rather than a black hole. + if kind.starts_with("Some(Coordinate") { + coordinates += 1; + assert!( + client.state().view.mode.is_editing(), + "column {col} ({kind}): a coordinate's editor must open" + ); + press(&mut host, id, &mut client, "Escape"); + continue; + } assert_ne!( client.state().view.status_msg, "\u{0}unsaid", @@ -530,6 +542,10 @@ fn the_tile_bar_route_into_records_mode_does_not_strand_the_user() { client cannot commit — an Enter that is a black hole" ); } + assert!( + coordinates > 0 && coordinates < cols, + "the walk must cover both a coordinate and a declined column" + ); } // ── Ctrl-S, all the way to storage (improvise-jz6w) ───────────────────── @@ -1018,3 +1034,1092 @@ fn a_typed_number_crosses_the_serialized_wire_exactly() { "the host must store the number the user typed, not its neighbour" ); } + +// ── a coordinate rename over the wire (improvise-fdk1) ─────────────────── + +/// Every frame the host addressed, to whichever of the two clients it names. +fn route( + frames: &[Addressed], + a: (improvise_command::ui::project::SessionId, &mut Client), + b: (improvise_command::ui::project::SessionId, &mut Client), +) { + for Addressed { to, msg } in frames { + let json = serde_json::to_string(msg).expect("a server frame serializes"); + if *to == a.0 { + a.1.on_frame(&json).expect("a folds every frame"); + } + if *to == b.0 { + b.1.on_frame(&json).expect("b folds every frame"); + } + } +} + +/// What client B does while A's rename editor is open. +#[derive(PartialEq)] +enum Race { + Quiet, + WriteRevenue, + SwapDocument, +} + +/// Client A renames North's `Region` to `Zed` in records mode, while B does +/// `race`. Evidence file. +fn rename_north(race: Race) -> (Host, Vec) { + let mut host = host(); + let path = std::path::PathBuf::from("other.improv"); + host.app_mut().storage = Box::new(improvise_command::storage::MemoryFs::new()); + let other = host.app().model_state.workbook.clone(); + improvise_command::persistence::save_to(&*host.app().storage, &other, &path).unwrap(); + let (mut a, mut b) = (Client::new(), Client::new()); + let (a_id, a_snap) = host.connect(10, 10); + let (b_id, b_snap) = host.connect(10, 10); + a.set_viewport(10, 10); + b.set_viewport(10, 10); + deliver(&mut a, &[a_snap]); + deliver(&mut b, &[b_snap]); + + let press_a = |host: &mut Host, a: &mut Client, b: &mut Client, key: &str| { + let Some(frame) = a.key(&BrowserKey::new(key)) else { + return Vec::new(); + }; + let json = serde_json::to_string(&frame).expect("a client frame serializes"); + let frames = send(host, a_id, &json); + route(&frames, (a_id, &mut *a), (b_id, &mut *b)); + frames + }; + + press_a(&mut host, &mut a, &mut b, "T"); + for (category, key) in [("_Index", "r"), ("_Dim", "c")] { + let at = a + .state() + .tiles + .tiles + .iter() + .position(|t| t.category == category) + .unwrap_or_else(|| panic!("the tile bar must carry {category}")); + while a.state().view.tile_cat_idx < at { + press_a(&mut host, &mut a, &mut b, "l"); + } + while a.state().view.tile_cat_idx > at { + press_a(&mut host, &mut a, &mut b, "h"); + } + press_a(&mut host, &mut a, &mut b, key); + } + press_a(&mut host, &mut a, &mut b, "Escape"); + assert!(a.state().grid.records, "A must be standing in records mode"); + + let coordinate = (0..a.state().grid.extent.cols) + .find(|&c| { + format!("{:?}", a.state().grid.cell(0, c).and_then(|x| x.id.clone())) + .starts_with("Some(Coordinate") + }) + .expect("records mode projects a coordinate column"); + while a.state().view.cursor().selected.1 < coordinate { + press_a(&mut host, &mut a, &mut b, "l"); + } + while a.state().view.cursor().selected.1 > coordinate { + press_a(&mut host, &mut a, &mut b, "h"); + } + press_a(&mut host, &mut a, &mut b, "i"); + assert!( + a.state().view.mode.is_editing(), + "A's editor must open on the coordinate, or nothing here is renamed" + ); + assert_eq!( + a.state().view.buffers.get("edit").map(String::as_str), + Some("North") + ); + + if race == Race::SwapDocument { + let generation = host.app().document_generation(); + let load = improvise_command::ui::reduce::caught_up( + host.app(), + improvise_protocol::ModelCommand::LoadModel(path), + ); + let frames = host.handle(b_id, ClientFrame::command(load)); + assert_eq!( + host.app().document_generation(), + generation + 1, + "setup: the load must really replace the document" + ); + route(&frames, (a_id, &mut a), (b_id, &mut b)); + assert!( + !a.state().view.mode.is_editing() && a.state().opened.is_none(), + "the rebase must close A's rename editor and drop its capture" + ); + return (host, frames); + } + if race == Race::WriteRevenue { + let revenue = (0..b.state().grid.extent.cols) + .find(|&c| { + b.record_cell_at(0, c) + .is_some_and(|cell| cell.measure.as_deref() == Some("Revenue")) + }) + .expect("B sees North's Revenue"); + let frame = b + .typed_cell_frame(0, revenue, "999") + .expect("B composes a write"); + let frames = send( + &mut host, + b_id, + &serde_json::to_string(&frame).expect("json"), + ); + assert!( + frames.iter().any(|f| f.to == b_id + && matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "B's write must land, or A has nothing to be stale against: {frames:?}" + ); + route(&frames, (a_id, &mut a), (b_id, &mut b)); + } + + for key in ["Backspace"; 5] { + press_a(&mut host, &mut a, &mut b, key); + } + for key in ["Z", "e", "d"] { + press_a(&mut host, &mut a, &mut b, key); + } + let answers = press_a(&mut host, &mut a, &mut b, "Enter"); + (host, answers) +} + +/// improvise-vndi for a rename: a document swap closes the rename editor, so +/// no rename crosses it. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_document_swap_closes_an_open_rename_editor() { + let (host, _) = rename_north(Race::SwapDocument); + assert_eq!( + region(&host, "Zed"), + None, + "the loaded document was renamed" + ); + assert!( + region(&host, "North").is_some(), + "North must survive the swap" + ); +} + +fn region(host: &Host, item: &str) -> Option { + host.app() + .model_state + .workbook + .model + .get_cell(&CellKey::new(vec![ + ("Region".into(), item.into()), + ("_Measure".into(), "Revenue".into()), + ])) + .cloned() +} + +/// improvise-fdk1: the rename moves the record, claimed at the version the +/// editor opened on. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_browser_renames_a_records_coordinate_and_the_host_moves_the_record() { + let (host, answers) = rename_north(Race::Quiet); + assert!( + answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the rename must be accepted: {answers:?}" + ); + assert_eq!(region(&host, "Zed"), Some(CellValue::Number(1200.0))); + assert_eq!(region(&host, "North"), None, "the record moved, not copied"); +} + +/// improvise-fdk1 / improvise-0j0f: another client wrote the record while the +/// editor was open, so the rename is stale. Evidence file. +#[test] +fn a_coordinate_rename_another_client_moved_is_refused() { + let (host, answers) = rename_north(Race::WriteRevenue); + assert!( + answers.iter().any(|f| matches!( + f.msg, + improvise_protocol::ServerFrame::Rejected(improvise_protocol::Rejection::RecordMoved( + _ + )) + )), + "a rename composed against a record someone else wrote must be refused: {answers:?}" + ); + assert_eq!( + region(&host, "North"), + Some(CellValue::Number(999.0)), + "the other client's write must survive" + ); + assert_eq!(region(&host, "Zed"), None, "nothing was renamed"); +} + +/// Three records, so a rename that re-sorts the rows moves another record under +/// the cursor. +fn three_records_host() -> Host { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + for (item, value) in [("North", 1.0), ("South", 2.0), ("West", 3.0)] { + wb.add_item("Region", item).unwrap(); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), item.into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(value), + ); + } + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + let mut app = App::new(wb, None); + app.rebuild_layout(); + Host::new(app) +} + +/// improvise-fdk1: the editor's target is captured once the host has answered +/// this client's own rename, not from the cache it rolled forward. Evidence file. +#[test] +fn a_second_rename_after_a_reorder_renames_the_row_the_host_shows() { + let mut host = three_records_host(); + let mut a = Client::new(); + a.set_viewport(10, 10); + let (id, snapshot) = host.connect(10, 10); + deliver(&mut a, &[snapshot]); + press(&mut host, id, &mut a, "T"); + tile_to(&mut host, id, &mut a, "_Index", "r"); + tile_to(&mut host, id, &mut a, "_Dim", "c"); + press(&mut host, id, &mut a, "Escape"); + assert!(a.state().grid.records, "setup: records mode"); + let coordinate = (0..a.state().grid.extent.cols) + .find(|&c| { + format!("{:?}", a.state().grid.cell(0, c).and_then(|x| x.id.clone())) + .starts_with("Some(Coordinate") + }) + .expect("a coordinate column"); + while a.state().view.cursor().selected.1 < coordinate { + press(&mut host, id, &mut a, "l"); + } + let text_at = |a: &Client, row: usize| { + a.state() + .grid + .cell(row, coordinate) + .map(|c| c.text.clone()) + .unwrap_or_default() + }; + assert_eq!(text_at(&a, 1), "South", "setup: South is row 1"); + assert_eq!(text_at(&a, 2), "West", "setup: West is row 2"); + press(&mut host, id, &mut a, "j"); + + // South -> Zed re-sorts the rows; the commit re-enters editing one row + // down, on West as this client's cache stood before the host answered. + press(&mut host, id, &mut a, "i"); + for key in ["Backspace"; 5] { + press(&mut host, id, &mut a, key); + } + for key in ["Z", "e", "d", "Enter"] { + press(&mut host, id, &mut a, key); + } + assert!( + a.state().view.mode.is_editing(), + "the commit re-enters editing" + ); + let row = a.state().view.cursor().selected.0; + let shown = text_at(&a, row); + assert_ne!( + shown, "West", + "the answered grid must put another record under the cursor than the \ + stale re-entry did, or nothing separates the two captures" + ); + + // Since improvise-fy9o/p6v6 the reopened editor is seeded from the answered cell. + assert_eq!( + a.state().view.buffers.get("edit"), + Some(&shown), + "the reopened editor must hold the answered cell, not the stale one" + ); + let seeded = shown.chars().count(); + for key in std::iter::repeat_n("Backspace", seeded).chain(["A", "l", "p", "h", "a", "Enter"]) { + press(&mut host, id, &mut a, key); + } + // Zed is reachable since improvise-wdmp: the asker keeps its own placement. + let values = [("North", 1.0), ("Zed", 2.0), ("West", 3.0)]; + let (_, moved) = values + .iter() + .find(|(name, _)| *name == shown) + .unwrap_or_else(|| panic!("the cursor shows {shown:?}")); + assert_eq!( + region(&host, "Alpha"), + Some(CellValue::Number(*moved)), + "the second rename must move {shown}, the record the client showed" + ); + assert_eq!(region(&host, &shown), None); + assert_eq!( + region(&host, "West").is_some(), + shown != "West", + "West untouched" + ); +} + +// ── the keyboard editor's target (improvise-8xq0) ──────────────────────── + +/// What client B does while A's keyboard editor is open on North/Cost. +#[derive(PartialEq)] +enum Meanwhile { + HideNorth, + WriteNorth, + HideBoth, + HideRevenue, +} + +/// The commands B sends for `meanwhile`, in order. +fn meanwhile_commands( + host: &Host, + a: &Client, + meanwhile: &Meanwhile, +) -> Vec { + use improvise_protocol::ModelCommand; + let hide = |item: &str| ModelCommand::HideItem { + view: host.app().view_name().to_string(), + category: "Region".into(), + item: item.into(), + }; + match meanwhile { + Meanwhile::HideNorth => vec![hide("North")], + Meanwhile::HideBoth => vec![hide("North"), hide("South")], + Meanwhile::HideRevenue => vec![ModelCommand::HideItem { + view: host.app().view_name().to_string(), + category: "_Measure".into(), + item: "Revenue".into(), + }], + Meanwhile::WriteNorth => vec![ModelCommand::SetCell { + cell: a.record_cell_at(0, 0).expect("North/Revenue is stored"), + value: CellValue::Number(999.0), + }], + } +} + +/// Client A presses `i` on North/Cost (0,1) and types `42`; B does `meanwhile` +/// before A presses Enter. Evidence file. +fn commit_north_cost(meanwhile: Meanwhile) -> (Host, Vec) { + use improvise_command::ui::reduce::caught_up; + + let mut host = host(); + let (mut a, mut b) = (Client::new(), Client::new()); + let (a_id, a_snap) = host.connect(10, 10); + let (b_id, b_snap) = host.connect(10, 10); + a.set_viewport(10, 10); + b.set_viewport(10, 10); + deliver(&mut a, &[a_snap]); + deliver(&mut b, &[b_snap]); + for key in ["l", "i", "4", "2"] { + press(&mut host, a_id, &mut a, key); + } + assert_eq!( + a.state().view.cursor().selected, + (0, 1), + "setup: on North/Cost" + ); + assert!( + a.state().view.mode.is_editing(), + "setup: the editor is open" + ); + + for cmd in meanwhile_commands(&host, &a, &meanwhile) { + let relayed = host.handle(b_id, ClientFrame::command(caught_up(host.app(), cmd))); + route(&relayed, (a_id, &mut a), (b_id, &mut b)); + } + assert!(a.state().view.mode.is_editing(), "setup: still editing"); + if meanwhile == Meanwhile::HideBoth { + assert!( + a.state().grid.cell(0, 0).is_none(), + "setup: no cell is left under the cursor" + ); + } + if meanwhile == Meanwhile::HideNorth { + assert_eq!( + a.state().grid.row_frame.leaf_labels().first().cloned(), + Some("South".to_string()), + "setup: the rows renumbered under the open editor" + ); + } + + let frame = a + .key(&BrowserKey::new("Enter")) + .expect("a commit composes a frame"); + let json = serde_json::to_string(&frame).expect("a client frame serializes"); + let answers = send(&mut host, a_id, &json); + (host, answers) +} + +fn cell(host: &Host, region: &str, measure: &str) -> Option { + host.app() + .model_state + .workbook + .model + .get_cell(&CellKey::new(vec![ + ("Region".into(), region.into()), + ("_Measure".into(), measure.into()), + ])) + .cloned() +} + +/// improvise-8xq0: B hid the row under A's open editor, and Enter wrote the +/// cell under the cursor at Enter. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_writes_the_cell_it_opened_on_after_the_rows_renumber() { + let (host, answers) = commit_north_cost(Meanwhile::HideNorth); + assert!( + answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the write must be accepted: {answers:?}" + ); + assert_eq!(cell(&host, "South", "Revenue"), None, "South was written"); + assert_eq!(cell(&host, "South", "Cost"), None, "South was written"); + assert_eq!( + cell(&host, "North", "Cost"), + Some(CellValue::Number(42.0)), + "the edit must land on the cell the editor opened on" + ); +} + +/// improvise-8xq0: the editor an advance reopens commits to the cell it landed +/// on, not the one the last commit captured. Evidence file. +#[test] +fn a_second_keyboard_commit_writes_the_cell_the_advance_landed_on() { + let mut host = host(); + let mut a = Client::new(); + a.set_viewport(10, 10); + let (id, snapshot) = host.connect(10, 10); + deliver(&mut a, &[snapshot]); + for key in ["i", "5", "Enter"] { + press(&mut host, id, &mut a, key); + } + assert_eq!(a.state().view.cursor().selected, (1, 0), "setup: advanced"); + assert!(a.state().view.mode.is_editing(), "setup: reopened"); + for key in ["7", "Enter"] { + press(&mut host, id, &mut a, key); + } + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(12005.0)) + ); + assert_eq!( + cell(&host, "South", "Revenue"), + Some(CellValue::Number(7.0)), + "the second commit must write the cell the advance landed on" + ); +} + +/// improvise-xz2z, 0j0f's rule on the keyboard path: the basis is the one read +/// when the editor opened. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_is_refused_when_its_record_was_written_while_open() { + let (host, answers) = commit_north_cost(Meanwhile::WriteNorth); + assert!( + answers.iter().any(|f| matches!( + f.msg, + improvise_protocol::ServerFrame::Rejected(improvise_protocol::Rejection::RecordMoved( + _ + )) + )), + "an edit composed against a record someone else wrote must be refused: {answers:?}" + ); + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(999.0)) + ); + assert_eq!(cell(&host, "North", "Cost"), None, "nothing was written"); +} + +/// A's Enter on North/Cost CROSSES B's `meanwhile`: B's relays reach A only +/// after A has sent. Then A presses Enter again. Evidence file. +fn cross_north_cost(meanwhile: Meanwhile) -> (Host, Vec, Client) { + cross_north(&["l", "i", "4", "2"], meanwhile) +} + +/// `cross_north_cost`, with the keys that open the editor and type into it. +fn cross_north(keys: &[&str], meanwhile: Meanwhile) -> (Host, Vec, Client) { + use improvise_command::ui::reduce::caught_up; + + let mut host = host(); + let (mut a, mut b) = (Client::new(), Client::new()); + let (a_id, a_snap) = host.connect(10, 10); + let (b_id, b_snap) = host.connect(10, 10); + a.set_viewport(10, 10); + b.set_viewport(10, 10); + deliver(&mut a, &[a_snap]); + deliver(&mut b, &[b_snap]); + for key in keys { + press(&mut host, a_id, &mut a, key); + } + let mut held = Vec::new(); + for cmd in meanwhile_commands(&host, &a, &meanwhile) { + held.extend(host.handle(b_id, ClientFrame::command(caught_up(host.app(), cmd)))); + } + let first = a + .key(&BrowserKey::new("Enter")) + .expect("a commit composes a frame"); + let json = serde_json::to_string(&first).expect("a client frame serializes"); + let answered = send(&mut host, a_id, &json); + assert!( + answered + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Rejected(_))), + "setup: the crossing Enter is refused: {answered:?}" + ); + route(&held, (a_id, &mut a), (b_id, &mut b)); + route(&answered, (a_id, &mut a), (b_id, &mut b)); + assert!( + a.state().view.mode.is_editing(), + "setup: the refusal reopens the editor" + ); + + let retry = a.key(&BrowserKey::new("Enter")); + let answers = match retry { + Some(frame) => { + let json = serde_json::to_string(&frame).expect("a client frame serializes"); + send(&mut host, a_id, &json) + } + None => Vec::new(), + }; + (host, answers, a) +} + +/// improvise-8phw: the crossing refusal reopened the editor over South, and the +/// retry wrote 42 into South/Cost. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_retried_after_a_crossing_hide_writes_no_other_cell() { + let (host, answers, a) = cross_north_cost(Meanwhile::HideNorth); + assert_eq!(cell(&host, "South", "Cost"), None, "South/Cost was written"); + assert_eq!( + cell(&host, "South", "Revenue"), + None, + "South/Revenue was written" + ); + assert!( + !answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the retry must send no write: {answers:?}" + ); + assert!( + a.state().view.mode.is_editing(), + "the retry declines and leaves the editor open" + ); + assert!( + a.state().view.status_msg.contains("has changed"), + "and says so, rather than going silently upstream: {:?}", + a.state().view.status_msg + ); +} + +/// improvise-8phw, the measure half: the crossing hide leaves the SAME record +/// under the cursor in another column. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_retried_after_a_crossing_column_hide_writes_no_other_measure() { + let (host, answers, _) = cross_north(&["i", "4", "2"], Meanwhile::HideRevenue); + assert_eq!(cell(&host, "North", "Cost"), None, "North/Cost was written"); + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(1200.0)), + "the opened cell is hidden, so nothing may land on it either" + ); + assert!( + !answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the retry must send no write: {answers:?}" + ); +} + +/// improvise-8phw's control: a crossing WRITE to the record is refused, and the +/// retry is the one-keystroke force onto the same cell (gotcha 43). +#[test] +fn a_keyboard_edit_retried_after_a_crossing_write_forces_the_same_cell() { + let (host, answers, _) = cross_north_cost(Meanwhile::WriteNorth); + assert!( + answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the retry is a force: {answers:?}" + ); + assert_eq!(cell(&host, "North", "Cost"), Some(CellValue::Number(42.0))); + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(999.0)) + ); +} + +/// Codex on PR #274: with no cell left under the cursor, Enter went upstream +/// instead of writing the cell the editor opened on. Evidence file. +#[test] +fn a_keyboard_edit_whose_rows_all_vanished_writes_the_cell_it_opened_on() { + let (host, answers) = commit_north_cost(Meanwhile::HideBoth); + assert!( + answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the write must be sent and accepted: {answers:?}" + ); + assert_eq!(cell(&host, "North", "Cost"), Some(CellValue::Number(42.0))); +} + +// ── races on a coordinate rename (improvise-20ek, improvise-milr) ─────── + +type Sid = improvise_command::ui::project::SessionId; + +/// A client driven into records mode through the tile bar, its cursor on the +/// coordinate column of row 0. Returns the session, the client and the column. +fn records_client_at_coordinate(host: &mut Host) -> (Sid, Client, usize) { + let mut a = Client::new(); + a.set_viewport(10, 10); + let (id, snapshot) = host.connect(10, 10); + deliver(&mut a, &[snapshot]); + press(host, id, &mut a, "T"); + tile_to(host, id, &mut a, "_Index", "r"); + tile_to(host, id, &mut a, "_Dim", "c"); + press(host, id, &mut a, "Escape"); + assert!(a.state().grid.records, "setup: records mode"); + let coordinate = (0..a.state().grid.extent.cols) + .find(|&c| { + format!("{:?}", a.state().grid.cell(0, c).and_then(|x| x.id.clone())) + .starts_with("Some(Coordinate") + }) + .expect("a coordinate column"); + while a.state().view.cursor().selected.1 < coordinate { + press(host, id, &mut a, "l"); + } + (id, a, coordinate) +} + +fn retype(host: &mut Host, id: Sid, a: &mut Client, text: &str) { + for _ in 0..8 { + press(host, id, a, "Backspace"); + } + for ch in text.chars() { + press(host, id, a, &ch.to_string()); + } +} + +/// Another session's frame, applied, with what it owes `a` delivered to `a`. +fn from_another_client(host: &mut Host, a_id: Sid, a: &mut Client, cmd: ModelCommandArg) { + let (b_id, _) = host.connect(10, 10); + let frame = improvise_command::ui::reduce::caught_up(host.app(), cmd); + let relayed = host.handle(b_id, ClientFrame::command(frame)); + assert!( + relayed + .iter() + .any(|f| f.to == b_id && matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the other client's write must land: {relayed:?}" + ); + let for_a: Vec = relayed.into_iter().filter(|f| f.to == a_id).collect(); + deliver(a, &for_a); +} + +type ModelCommandArg = improvise_protocol::ModelCommand; + +/// improvise-20ek: the record an editor opened on is gone, and a retry must not +/// rename whatever moved under the cursor. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_retry_after_record_gone_renames_no_other_record() { + use improvise_protocol::{CellId, ModelCommand, Rejection, ServerFrame}; + let mut host = three_records_host(); + let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host); + assert_eq!(a.state().grid.cell(0, coordinate).unwrap().text, "North"); + let Some(CellId::Coordinate { record: north, .. }) = + a.state().grid.cell(0, coordinate).unwrap().id.clone() + else { + panic!("setup: a coordinate id") + }; + press(&mut host, a_id, &mut a, "i"); + retype(&mut host, a_id, &mut a, "Zed"); + + from_another_client( + &mut host, + a_id, + &mut a, + ModelCommand::RenameRecordCoordinate { + record: north, + category: "Region".into(), + item: "Yak".into(), + }, + ); + + let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes"); + let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap()); + deliver(&mut a, &answer); + assert!( + answer + .iter() + .any(|f| matches!(f.msg, ServerFrame::Rejected(Rejection::RecordGone(_)))), + "the first Enter is RecordGone: {answer:?}" + ); + let row = a.state().view.cursor().selected.0; + assert!( + a.state().view.mode.is_editing() + && a.state().grid.cell(row, coordinate).unwrap().text != "Yak", + "setup: the editor is back, over a record other than the one renamed away" + ); + + press(&mut host, a_id, &mut a, "Enter"); + assert_eq!( + region(&host, "Zed"), + None, + "the retry renamed a record never opened" + ); + assert_eq!(region(&host, "South"), Some(CellValue::Number(2.0))); + assert_eq!(region(&host, "West"), Some(CellValue::Number(3.0))); + assert_eq!(region(&host, "Yak"), Some(CellValue::Number(1.0))); +} + +/// improvise-milr: the rename's destination is a record too, and a write to it +/// this client never saw must refuse the rename. Evidence file. +#[test] +fn a_rename_onto_a_record_another_client_just_wrote_is_refused() { + use improvise_protocol::{ModelCommand, Rejection, ServerFrame}; + let mut host = three_records_host(); + let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host); + let revenue = (0..a.state().grid.extent.cols) + .find(|&c| { + a.record_cell_at(1, c) + .is_some_and(|x| x.measure.as_deref() == Some("Revenue")) + }) + .expect("South's revenue"); + let south = a.record_cell_at(1, revenue).unwrap(); + assert_eq!(a.state().grid.cell(1, coordinate).unwrap().text, "South"); + press(&mut host, a_id, &mut a, "i"); + retype(&mut host, a_id, &mut a, "South"); + + from_another_client( + &mut host, + a_id, + &mut a, + ModelCommand::SetCell { + cell: south.clone(), + value: CellValue::Number(999.0), + }, + ); + + let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes"); + let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap()); + assert!( + answer.iter().any(|f| matches!( + f.msg, + ServerFrame::Rejected(Rejection::RecordMoved(h)) if h == south.record + )), + "the destination moved under this client: {answer:?}" + ); + assert_eq!(region(&host, "South"), Some(CellValue::Number(999.0))); + assert_eq!(region(&host, "North"), Some(CellValue::Number(1.0))); +} + +/// The control for the test above: a destination this client HAS seen at its +/// current version is overwritten, which is improvise-kndq's policy, unchanged. +#[test] +fn a_rename_onto_a_record_this_client_has_seen_still_overwrites_it() { + let mut host = three_records_host(); + let (a_id, mut a, _) = records_client_at_coordinate(&mut host); + press(&mut host, a_id, &mut a, "i"); + retype(&mut host, a_id, &mut a, "South"); + let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes"); + let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap()); + assert!( + answer + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "{answer:?}" + ); + assert_eq!( + region(&host, "South"), + Some(CellValue::Number(1.0)), + "North's value" + ); + assert_eq!(region(&host, "North"), None); +} + +/// Another session's frame, applied, with what it owes `a` HELD rather than +/// delivered: `a` has not heard it yet (improvise-9ww8). +fn held_from_another_client(host: &mut Host, a_id: Sid, cmd: ModelCommandArg) -> Vec { + let (b_id, _) = host.connect(10, 10); + let frame = improvise_command::ui::reduce::caught_up(host.app(), cmd); + let relayed = host.handle(b_id, ClientFrame::command(frame)); + assert!( + relayed + .iter() + .any(|f| f.to == b_id && matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the other client's write must land: {relayed:?}" + ); + relayed.into_iter().filter(|f| f.to == a_id).collect() +} + +/// A's rename crosses B's rename of North in flight: A composes Enter before +/// hearing B, then hears B's relays and its own refusal, in that order. +fn crossing_rename(row: usize) -> Host { + use improvise_protocol::{CellId, ModelCommand, Rejection, ServerFrame}; + let mut host = three_records_host(); + let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host); + let Some(CellId::Coordinate { record: north, .. }) = + a.state().grid.cell(0, coordinate).unwrap().id.clone() + else { + panic!("setup: North's coordinate id") + }; + for _ in 0..row { + press(&mut host, a_id, &mut a, "j"); + } + press(&mut host, a_id, &mut a, "i"); + retype(&mut host, a_id, &mut a, "Zed"); + + let held = held_from_another_client( + &mut host, + a_id, + ModelCommand::RenameRecordCoordinate { + record: north, + category: "Region".into(), + item: "Yak".into(), + }, + ); + let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes"); + let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap()); + deliver(&mut a, &held); + deliver(&mut a, &answer); + assert!( + answer + .iter() + .any(|f| matches!(f.msg, ServerFrame::Rejected(Rejection::LayoutMoved))), + "setup: the crossing is refused on the layout tier, before any record: {answer:?}" + ); + assert!( + a.state().view.mode.is_editing(), + "setup: the editor is back" + ); + + press(&mut host, a_id, &mut a, "Enter"); + host +} + +/// improvise-9ww8, the source gone: B renamed the very record A opened on. +/// Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_retry_after_a_crossing_rename_of_its_record_renames_no_other_record() { + let host = crossing_rename(0); + assert_eq!( + region(&host, "Zed"), + None, + "the retry renamed a record never opened" + ); + assert_eq!(region(&host, "Yak"), Some(CellValue::Number(1.0))); + assert_eq!(region(&host, "South"), Some(CellValue::Number(2.0))); + assert_eq!(region(&host, "West"), Some(CellValue::Number(3.0))); +} + +/// improvise-9ww8, the source alive: B's rename only moves A's record off the +/// row under the cursor. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_retry_after_a_crossing_reorder_renames_no_other_record() { + let host = crossing_rename(1); + assert_eq!( + region(&host, "West"), + Some(CellValue::Number(3.0)), + "the retry renamed West, the record that moved under the cursor" + ); + assert_ne!(region(&host, "Zed"), Some(CellValue::Number(3.0))); + assert_eq!(region(&host, "Yak"), Some(CellValue::Number(1.0))); +} + +/// The control for the two crossings above: `RecordMoved` on the SAME record is +/// still a one-keystroke force. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_retry_after_a_crossing_write_to_its_own_record_is_a_force() { + use improvise_protocol::{ModelCommand, Rejection, ServerFrame}; + let mut host = three_records_host(); + let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host); + assert_eq!(a.state().grid.cell(0, coordinate).unwrap().text, "North"); + let revenue = (0..a.state().grid.extent.cols) + .find(|&c| { + a.record_cell_at(0, c) + .is_some_and(|x| x.measure.as_deref() == Some("Revenue")) + }) + .expect("North's revenue"); + let north = a.record_cell_at(0, revenue).unwrap(); + press(&mut host, a_id, &mut a, "i"); + retype(&mut host, a_id, &mut a, "Zed"); + + let held = held_from_another_client( + &mut host, + a_id, + ModelCommand::SetCell { + cell: north.clone(), + value: CellValue::Number(9.0), + }, + ); + let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes"); + let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap()); + deliver(&mut a, &held); + deliver(&mut a, &answer); + assert!( + answer.iter().any(|f| matches!( + f.msg, + ServerFrame::Rejected(Rejection::RecordMoved(h)) if h == north.record + )), + "setup: the crossing write moves the record A is renaming: {answer:?}" + ); + + press(&mut host, a_id, &mut a, "Enter"); + assert_eq!( + region(&host, "Zed"), + Some(CellValue::Number(9.0)), + "the retry must rename North, carrying B's write" + ); + assert_eq!(region(&host, "North"), None); +} + +/// improvise-joed on the rename path: a second rename typed before the host +/// answers the first waits, and keeps its text. Evidence file. +#[test] +fn a_rename_that_waits_for_the_host_keeps_the_typed_text() { + let mut host = three_records_host(); + let mut a = Client::new(); + a.set_viewport(10, 10); + let (id, snapshot) = host.connect(10, 10); + deliver(&mut a, &[snapshot]); + press(&mut host, id, &mut a, "T"); + tile_to(&mut host, id, &mut a, "_Index", "r"); + tile_to(&mut host, id, &mut a, "_Dim", "c"); + press(&mut host, id, &mut a, "Escape"); + assert!(a.state().grid.records, "setup: records mode"); + let coordinate = (0..a.state().grid.extent.cols) + .find(|&c| { + format!("{:?}", a.state().grid.cell(0, c).and_then(|x| x.id.clone())) + .starts_with("Some(Coordinate") + }) + .expect("a coordinate column"); + while a.state().view.cursor().selected.1 < coordinate { + press(&mut host, id, &mut a, "l"); + } + press(&mut host, id, &mut a, "j"); + press(&mut host, id, &mut a, "i"); + for key in ["Backspace"; 5] { + press(&mut host, id, &mut a, key); + } + for key in ["Z", "e", "d"] { + press(&mut host, id, &mut a, key); + } + // The first rename is composed and folded as sent, and never answered. + let first = a + .key(&BrowserKey::new("Enter")) + .expect("the rename composes"); + assert!( + matches!(&first, ClientFrame::Commands(c) if c.iter().any(|c| matches!(c, improvise_protocol::Command::Model { .. }))), + "setup: the first Enter renames" + ); + assert!( + a.state().view.mode.is_editing(), + "setup: the commit reopened" + ); + for key in ["Backspace", "Backspace", "Q"] { + a.key(&BrowserKey::new(key)); + } + let typed = a.state().view.buffers.get("edit").cloned(); + assert!( + typed.as_deref().is_some_and(|t| !t.is_empty()), + "setup: there is typed text to lose: {typed:?}" + ); + + let second = a.key(&BrowserKey::new("Enter")); + assert!( + a.state().view.status_msg.starts_with("still waiting"), + "setup: this is the waiting decline: {:?}", + a.state().view.status_msg + ); + assert!(a.state().view.mode.is_editing(), "the editor stays open"); + assert_eq!( + a.state().view.buffers.get("edit").cloned(), + typed, + "the text the user typed must survive the decline: {second:?}" + ); +} + +/// improvise-joed on #251's `Gone` decline: the retry declined, and the welded +/// `clear-buffer` still emptied the editor. Evidence file. +#[test] +fn a_retry_whose_cell_is_gone_keeps_the_typed_text() { + let (_, _, a) = cross_north_cost(Meanwhile::HideNorth); + assert!( + a.state().view.status_msg.contains("has changed"), + "setup: this is the Gone decline: {:?}", + a.state().view.status_msg + ); + assert_eq!( + a.state().view.buffers.get("edit").map(String::as_str), + Some("42"), + "the text the user typed must survive the decline" + ); +} + +/// `host()` with `Profit = Revenue - Cost`, so a grid with Region on None +/// shows aggregates and a derived column, and no stored cell. +fn host_with_profit() -> Host { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Region", "South").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.model.add_formula( + improvise_command::formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(), + ); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(1200.0), + ); + let mut app = App::new(wb, None); + app.rebuild_layout(); + Host::new(app) +} + +/// improvise-3h9w: B's reframe left A's cursor on an aggregate, and Enter was +/// refused for THAT cell instead of writing the captured one. Evidence file. +#[test] +fn a_captured_edit_commits_when_the_cursor_now_sits_on_an_aggregate() { + use improvise_command::ui::reduce::caught_up; + use improvise_protocol::ModelCommand; + + let mut host = host_with_profit(); + let (mut a, mut b) = (Client::new(), Client::new()); + let (a_id, a_snap) = host.connect(10, 10); + let (b_id, b_snap) = host.connect(10, 10); + a.set_viewport(10, 10); + b.set_viewport(10, 10); + deliver(&mut a, &[a_snap]); + deliver(&mut b, &[b_snap]); + for key in ["l", "i", "4", "2"] { + press(&mut host, a_id, &mut a, key); + } + let region_off = ModelCommand::SetAxis { + view: host.app().view_name().to_string(), + category: "Region".into(), + axis: Axis::None, + }; + let relayed = host.handle( + b_id, + ClientFrame::command(caught_up(host.app(), region_off)), + ); + route(&relayed, (a_id, &mut a), (b_id, &mut b)); + assert!(a.state().view.mode.is_editing(), "setup: still editing"); + let (row, col) = a.state().view.cursor().selected; + assert!( + matches!( + a.state().grid.cell(row, col).and_then(|c| c.id.clone()), + Some(improvise_protocol::CellId::Aggregate(_)) + ), + "setup: the cursor now sits on an aggregate" + ); + + press(&mut host, a_id, &mut a, "Enter"); + assert_eq!( + cell(&host, "North", "Cost"), + Some(CellValue::Number(42.0)), + "the captured cell must be written: status {:?}", + a.state().view.status_msg + ); +} diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index e1bd75f8..ebfa8d64 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1728,13 +1728,15 @@ fn a_records_commit_is_declined_off_the_grid_and_not_off_the_mode() { // The control: the same mode over a REGULAR grid must still commit, or the // decline above is "this client never writes" and proves nothing. let plain = furnished_host(); - let mut ordinary = paged_client(&plain); - ordinary.view.mode = AppMode::editing(); + // Seeded as `records_client` seeds, basis included, over a regular grid; + // opened through the fold, which captures the cell the commit writes. + let mut ordinary = records_client(&plain); + ordinary.view.cursor_mut().selected = stored_cell_at(&ordinary); + ordinary.fold(&improvise_protocol::Command::ChangeMode(AppMode::editing())); ordinary .view .buffers .insert("edit".into(), "42".to_string()); - ordinary.view.cursor_mut().selected = stored_cell_at(&ordinary); assert!(!ordinary.grid.records, "the control must be a regular grid"); assert!( composes_a_write(&ordinary, enter), @@ -2277,6 +2279,262 @@ fn fold_host() -> App { app } +/// Measures on Row (`Profit` both stored and a formula's), months on Column, +/// so a `Down` from the bottom row wraps to the top of the next column. +fn wrapping_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Month").unwrap(); + for month in ["Jan", "Feb"] { + wb.add_item("Month", month).unwrap(); + } + for measure in ["Revenue", "Profit", "Cost"] { + wb.add_item("_Measure", measure).unwrap(); + } + wb.model.add_formula( + improvise_command::formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(), + ); + wb.active_view_mut().set_axis("_Measure", Axis::Row); + wb.active_view_mut().set_axis("Month", Axis::Column); + for month in ["Jan", "Feb"] { + for (measure, n) in [("Revenue", 10.0), ("Cost", 4.0)] { + let key = improvise_command::model::cell::CellKey::new(vec![ + ("Month".to_string(), month.to_string()), + ("_Measure".to_string(), measure.to_string()), + ]); + wb.model + .set_cell(key, improvise_protocol::CellValue::Number(n)); + } + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + app +} + +/// improvise-y5eb: the settle must not read the cache through offsets it has not +/// caught up with. See `context/test-evidence/clients.md`. +#[test] +fn a_wrapping_advance_settles_on_the_cell_it_lands_on_not_a_stale_one() { + let mut tui = wrapping_host(); + tui.cursor_mut().selected = (2, 0); + tui.cursor_mut().row_offset = 1; + for k in ["i", "Enter"] { + reduce_key(&mut tui, BrowserKey::new(k).to_key_event().unwrap()); + } + assert_eq!( + tui.view_state.cursor().selected, + (0, 1), + "setup: Enter wrapped" + ); + assert!( + tui.view_state.mode.is_editing(), + "setup: Revenue/Feb is editable" + ); + + let mut host = improvise_command::ui::host::Host::new(wrapping_host()); + let (session, welcome) = host.connect(2, 6); + let mut client = ClientState { + window: (2, 6), + ..ClientState::default() + }; + deliver(vec![welcome], session, &mut client); + let placed = vec![ + improvise_protocol::Command::SetSelected { row: 2, col: 0 }, + improvise_protocol::Command::SetRowOffset(1), + ]; + client.fold_outgoing(&placed); + deliver( + host.handle(session, ClientFrame::Commands(placed)), + session, + &mut client, + ); + for k in ["i", "Enter"] { + let event = BrowserKey::new(k).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{k}: the client must answer this key locally"); + }; + client.fold_outgoing(&cmds); + deliver( + host.handle(session, ClientFrame::Commands(cmds)), + session, + &mut client, + ); + } + assert_eq!(client.view.cursor().selected, (0, 1)); + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", tui.view_state.mode), + "the client settled on a cell not under its cursor: {:?}", + client.view.status_msg + ); +} + +/// improvise-ik6l: Enter on a typed-into editor over a cell `i` declines says +/// so on both ends. See `context/test-evidence/clients.md`. +#[test] +fn enter_on_an_editor_over_a_declined_cell_declines_out_loud_on_both_ends() { + type Case = (fn() -> App, (usize, usize), &'static str); + let cases: [Case; 4] = [ + (fold_host, (2, 1), "Enter"), + (yank_host, (1, 3), "Enter"), + (fold_host, (2, 1), "Tab"), + (yank_host, (1, 3), "Tab"), + ]; + for (make, at, key) in cases { + let mut app = make(); + app.cursor_mut().selected = at; + app.view_state.mode = AppMode::editing(); + app.view_state.buffers.insert("edit".into(), "7".into()); + app.view_state.opened_editor = Some(improvise_protocol::OpenedEditor { + seed: String::new(), + touched: true, + awaiting: false, + }); + let before = document_text(&app); + let mut client = client_ready_to_edit(&app, (6, 6)); + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{at:?}: {key} went upstream, where nothing answers it"); + }; + for cmd in &cmds { + client.fold(cmd); + } + reduce_key(&mut app, event); + assert_eq!(document_text(&app), before, "setup: the TUI wrote nothing"); + assert!( + cmds.iter() + .all(|c| !matches!(c, improvise_protocol::Command::Model { .. })), + "{at:?}: the browser composed a write" + ); + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", app.view_state.mode), + "{at:?}: the two ends disagree about the mode" + ); + assert_eq!(client.view.buffers, app.view_state.buffers, "{at:?}"); + assert!( + !app.view_state.status_msg.is_empty() && !client.view.status_msg.is_empty(), + "{at:?}: a silent decline: TUI {:?}, browser {:?}", + app.view_state.status_msg, + client.view.status_msg + ); + } +} + +/// improvise-5m9z's other half: what the user typed before the host answered +/// is not thrown away. See `context/test-evidence/clients.md`. +#[test] +fn typing_before_the_host_answers_keeps_the_editor_past_the_window() { + let mut host = improvise_command::ui::host::Host::new(fold_host()); + let (session, welcome) = host.connect(2, 6); + let mut client = ClientState { + window: (2, 6), + ..ClientState::default() + }; + deliver(vec![welcome], session, &mut client); + let placed = vec![improvise_protocol::Command::SetSelected { row: 1, col: 1 }]; + client.fold_outgoing(&placed); + deliver( + host.handle(session, ClientFrame::Commands(placed)), + session, + &mut client, + ); + let mut held = Vec::new(); + for k in ["i", "Enter", "7"] { + let event = BrowserKey::new(k).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{k}: the client must answer this key locally"); + }; + client.fold_outgoing(&cmds); + held.push(host.handle(session, ClientFrame::Commands(cmds))); + } + assert_eq!(client.view.cursor().selected, (2, 1), "setup: on the fold"); + for reply in held { + deliver(reply, session, &mut client); + } + assert!( + client.view.mode.is_editing(), + "the typed 7 was thrown away: {:?}", + client.view.mode + ); + assert_eq!( + client.view.buffers.get("edit").map(String::as_str), + Some("7") + ); +} + +/// improvise-5m9z: an advance past the client's window lands on a cell it holds +/// no id for, and must settle as the TUI does. See `context/test-evidence/clients.md`. +#[test] +fn advancing_the_editor_past_the_window_settles_where_the_tui_does() { + let expanded = || { + let mut app = fold_host(); + let view = app.model_state.workbook.active_view_mut(); + view.toggle_group_collapse("Month", "Q2"); + app.rebuild_layout(); + app + }; + type Case = ( + fn() -> App, + (usize, usize), + (usize, usize), + &'static str, + bool, + ); + let cases: [Case; 3] = [ + (fold_host, (2, 6), (1, 1), "Enter", false), + (yank_host, (6, 3), (1, 2), "Tab", false), + (expanded, (2, 6), (1, 1), "Enter", true), + ]; + for (make, window, at, key, editing) in cases { + let mut tui = make(); + tui.cursor_mut().selected = at; + for k in ["i", key] { + reduce_key(&mut tui, BrowserKey::new(k).to_key_event().unwrap()); + } + assert_eq!(tui.view_state.mode.is_editing(), editing, "setup: the TUI"); + + let mut host = improvise_command::ui::host::Host::new(make()); + let (session, welcome) = host.connect(window.0, window.1); + let mut client = ClientState { + window, + ..ClientState::default() + }; + deliver(vec![welcome], session, &mut client); + let placed = vec![improvise_protocol::Command::SetSelected { + row: at.0, + col: at.1, + }]; + client.fold_outgoing(&placed); + deliver( + host.handle(session, ClientFrame::Commands(placed)), + session, + &mut client, + ); + for k in ["i", key] { + let event = BrowserKey::new(k).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{k}: the client must answer this key locally"); + }; + client.fold_outgoing(&cmds); + deliver( + host.handle(session, ClientFrame::Commands(cmds)), + session, + &mut client, + ); + } + assert_eq!( + client.view.cursor().selected, + tui.view_state.cursor().selected, + "{key}: the advance must land past the window on both ends" + ); + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", tui.view_state.mode), + "{key} from {at:?} in a {window:?} window: the client and the TUI disagree" + ); + } +} + /// improvise-enz6: a commit that advances onto a cell `i` declines re-opens no /// editor there, on either end. See `context/test-evidence/clients.md`. #[test] @@ -2639,6 +2897,161 @@ fn every_projected_edit_text_commits_back_to_its_value() { } } +// A coordinate rename (improvise-fdk1). + +/// Two records, North renamed past South so the sorted records order moves. +/// Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_locally_committed_coordinate_rename_agrees_with_the_host() { + let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE); + let two_records = || { + let mut app = furnished_host(); + let south = north_revenue().with("Region", "South"); + app.model_state + .workbook + .model + .set_cell(south, improvise_protocol::CellValue::Number(900.0)); + app.rebuild_layout(); + reduce_key(&mut app, r); + app + }; + let (mut app, mut twin) = (two_records(), two_records()); + let region = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == "Region") + .expect("a Region column"); + let north = 0; + assert_eq!( + app.layout.record_row(north).unwrap().coords.get("Region"), + Some("North"), + "the fixture renames North, at row 0" + ); + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (north, region); + } + let mut client = records_client(&app); + + both_ends(&mut app, &mut twin, &mut client, "i"); + assert_eq!( + client.view.buffers.get("edit").map(String::as_str), + Some("North"), + "the editor opens on the coordinate it renames" + ); + for key in [ + "Backspace", + "Backspace", + "Backspace", + "Backspace", + "Backspace", + "Z", + "e", + "d", + ] { + both_ends(&mut app, &mut twin, &mut client, key); + } + both_ends(&mut app, &mut twin, &mut client, "Enter"); + + let text = document_text(&app); + assert!( + text.contains("Region=Zed") && !text.contains("Region=North"), + "the rename must move North's cells to Zed: {text}" + ); + assert!( + text.contains("Region=South"), + "South is a bystander and must survive: {text}" + ); +} + +/// `i` then `Enter` on a coordinate: both ends open one editor and write +/// nothing. Evidence: `context/test-evidence/clients.md`. +#[test] +fn an_untouched_coordinate_enter_writes_nothing_on_either_end() { + let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE); + let records = || { + let mut app = furnished_host(); + app.rebuild_layout(); + reduce_key(&mut app, r); + app + }; + let (mut app, mut twin) = (records(), records()); + let region = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == "Region") + .expect("a Region column"); + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (0, region); + } + let mut client = records_client(&app); + let before = document_text(&app); + + both_ends(&mut app, &mut twin, &mut client, "i"); + assert_eq!( + client.view.opened_editor, app.view_state.opened_editor, + "the two ends captured different editors" + ); + assert_eq!( + client.view.opened_editor, + Some(improvise_protocol::OpenedEditor::on("North".into())), + "the coordinate editor recorded no opening" + ); + let enter = BrowserKey::new("Enter").to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, enter) else { + panic!("Enter: the client must answer this key locally"); + }; + assert!( + !cmds + .iter() + .any(|c| matches!(c, improvise_protocol::Command::Model { .. })), + "an untouched coordinate Enter composed a write: {cmds:?}" + ); + assert!( + cmds.contains(&improvise_protocol::Command::ChangeMode(AppMode::Normal)), + "Enter did not take the commit path, so this proves nothing: {cmds:?}" + ); + both_ends(&mut app, &mut twin, &mut client, "Enter"); + assert_eq!( + document_text(&app), + before, + "an untouched Enter changed the document" + ); +} + +/// An empty coordinate is refused on the host's key path, which says why and +/// advances; the browser must say the same. Evidence: `context/test-evidence/clients.md`. +#[test] +fn an_empty_coordinate_rename_is_refused_the_way_the_host_refuses_it() { + let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE); + let records = || { + let mut app = furnished_host(); + reduce_key(&mut app, r); + app + }; + let (mut app, mut twin) = (records(), records()); + let region = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == "Region") + .expect("a Region column"); + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (0, region); + } + let mut client = records_client(&app); + let before = document_text(&app); + + both_ends(&mut app, &mut twin, &mut client, "i"); + for key in ["Backspace"; 5] { + both_ends(&mut app, &mut twin, &mut client, key); + } + both_ends(&mut app, &mut twin, &mut client, "Enter"); + + assert_eq!( + document_text(&app), + before, + "an empty rename writes nothing" + ); + assert!( + app.view_state.status_msg.contains("cannot be empty"), + "the host must have refused, or this proves nothing: {:?}", + app.view_state.status_msg + ); +} + /// North/South/West = 1/2/3 on one measure, with `prune_empty` on: emptying a /// row's only value takes the row away and renumbers the rest. fn pruned_host() -> App { @@ -3367,3 +3780,285 @@ fn a_move_sent_before_a_pruning_commit_is_answered_writes_where_the_tui_writes() "the browser wrote a different record from the one the TUI wrote" ); } + +/// One scroll-then-edit case: the window, where the cursor starts, and the keys. +type Pipelined = ( + &'static str, + (usize, usize), + ((usize, usize), usize, usize), + &'static [&'static str], +); + +/// When the host's replies reach the client. +#[derive(Debug, Clone, Copy, PartialEq)] +enum Replies { + /// Each before the next key. + Delivered, + /// All held to the end, so every key reads the first window. + Held, + /// Held until the last two keys, which then run delivered. + HeldThenTyped, +} + +/// `wrapping_host`'s cells that differ from how it was built, as `Month/Measure`. +fn written_cells(app: &App) -> Vec { + let model = &app.model_state.workbook.model; + let mut written = Vec::new(); + for month in ["Jan", "Feb"] { + for (measure, built) in [ + ("Revenue", Some(10.0)), + ("Profit", None), + ("Cost", Some(4.0)), + ] { + let key = improvise_command::model::cell::CellKey::new(vec![ + ("Month".to_string(), month.to_string()), + ("_Measure".to_string(), measure.to_string()), + ]); + let now = model.get_cell(&key).cloned(); + if now != built.map(improvise_protocol::CellValue::Number) { + written.push(format!("{month}/{measure}")); + } + } + } + written +} + +/// One key on the client, its frame sent to the host; the reply comes back for +/// the caller to deliver or hold. An upstream key reaches a host that answers nothing. +fn sent_or_upstream( + host: &mut improvise_command::ui::host::Host, + session: improvise_command::ui::project::SessionId, + client: &mut ClientState, + key: &str, +) -> Vec { + match resolve(client, BrowserKey::new(key).to_key_event().unwrap()) { + KeyOutcome::Local(cmds) => { + client.fold_outgoing(&cmds); + host.handle(session, ClientFrame::Commands(cmds)) + } + _ => Vec::new(), + } +} + +/// A host over `make()` and a client on it, the cursor placed and answered. +fn placed_client( + make: fn() -> App, + window: (usize, usize), + (at, row_offset, col_offset): ((usize, usize), usize, usize), +) -> ( + improvise_command::ui::host::Host, + improvise_command::ui::project::SessionId, + ClientState, +) { + let mut host = improvise_command::ui::host::Host::new(make()); + let (session, welcome) = host.connect(window.0, window.1); + let mut client = ClientState { + window, + ..ClientState::default() + }; + deliver(vec![welcome], session, &mut client); + let placed = vec![ + improvise_protocol::Command::SetSelected { + row: at.0, + col: at.1, + }, + improvise_protocol::Command::SetRowOffset(row_offset), + improvise_protocol::Command::SetColOffset(col_offset), + ]; + client.fold_outgoing(&placed); + deliver( + host.handle(session, ClientFrame::Commands(placed)), + session, + &mut client, + ); + (host, session, client) +} + +/// improvise-7mro: the offset-aliasing matrix, with the host's replies delivered, +/// held, and held until the typing. See `context/test-evidence/clients.md`. +#[test] +fn keys_sent_before_a_scroll_is_answered_agree_with_the_tui() { + let cases: [Pipelined; 7] = [ + ( + "wrap", + (2, 6), + ((2, 0), 1, 0), + &["i", "Enter", "8", "Enter"], + ), + ( + "horizontal wrap", + (2, 1), + ((2, 0), 1, 0), + &["i", "Enter", "8", "Enter"], + ), + ("G", (2, 6), ((0, 0), 0, 0), &["G", "i", "7", "Enter"]), + ("$", (2, 1), ((0, 0), 0, 0), &["$", "i", "7", "Enter"]), + ("gg", (2, 6), ((2, 0), 1, 0), &["g", "g", "i", "7", "Enter"]), + ("$ x", (2, 1), ((0, 0), 0, 0), &["$", "x"]), + ("gg p", (2, 6), ((2, 0), 1, 0), &["y", "y", "g", "g", "p"]), + ]; + for (name, window, start, keys) in cases { + let mut tui = wrapping_host(); + tui.cursor_mut().selected = start.0; + for &k in keys { + reduce_key(&mut tui, BrowserKey::new(k).to_key_event().unwrap()); + } + assert_eq!( + written_cells(&tui).len(), + 1, + "setup: {name}: the TUI wrote one cell" + ); + for replies in [Replies::Delivered, Replies::Held, Replies::HeldThenTyped] { + let what = format!("{name} ({keys:?}, replies {replies:?})"); + let (mut host, session, mut client) = placed_client(wrapping_host, window, start); + let typed_from = match replies { + Replies::Delivered => 0, + Replies::Held => keys.len(), + Replies::HeldThenTyped => keys.len().saturating_sub(2), + }; + let mut held = Vec::new(); + for (i, &k) in keys.iter().enumerate() { + if i == typed_from { + deliver(std::mem::take(&mut held), session, &mut client); + } + held.extend(sent_or_upstream(&mut host, session, &mut client, k)); + if i >= typed_from { + deliver(std::mem::take(&mut held), session, &mut client); + } + } + if replies == Replies::Held { + assert_eq!( + written_cells(host.app()), + Vec::::new(), + "{what}: a write was composed before its target was projected" + ); + assert!( + client.view.mode.is_editing() == keys.contains(&"i") + && !client.view.status_msg.is_empty(), + "{what}: the last key must wait out loud, any editor open: {:?}, {:?}", + client.view.mode, + client.view.status_msg + ); + deliver(std::mem::take(&mut held), session, &mut client); + let retry = keys.last().expect("a key"); + let reply = sent_or_upstream(&mut host, session, &mut client, retry); + deliver(reply, session, &mut client); + } + assert_eq!( + written_cells(host.app()), + written_cells(&tui), + "{what}: the browser wrote a different cell from the TUI" + ); + if replies != Replies::Held { + assert_eq!(document_text(host.app()), document_text(&tui), "{what}"); + assert_eq!( + client.view.buffers.get("edit"), + tui.view_state.buffers.get("edit"), + "{what}: the editor's buffer" + ); + } + assert_eq!( + client.view.cursor().selected, + tui.view_state.cursor().selected, + "{what}: the cursor" + ); + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", tui.view_state.mode), + "{what}: the mode; browser status {:?}", + client.view.status_msg + ); + } + } +} + +/// Four regions by two measures, every cell stored and no two alike, so a cell +/// read at the wrong row or column is a different value. +fn four_by_two_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + for region in ["R0", "R1", "R2", "R3"] { + wb.add_item("Region", region).unwrap(); + } + for measure in ["A", "B"] { + wb.add_item("_Measure", measure).unwrap(); + } + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + for (r, region) in ["R0", "R1", "R2", "R3"].into_iter().enumerate() { + for (base, measure) in [(10.0, "A"), (20.0, "B")] { + let key = improvise_command::model::cell::CellKey::new(vec![ + ("Region".to_string(), region.to_string()), + ("_Measure".to_string(), measure.to_string()), + ]); + wb.model + .set_cell(key, improvise_protocol::CellValue::Number(base + r as f64)); + } + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + app +} + +/// improvise-7mro: a cell inside the held window, read while the client's offsets +/// differ from the frame's origin. See `context/test-evidence/clients.md`. +#[test] +fn a_held_cell_is_read_at_its_position_and_not_its_cache_index() { + for keys in [ + &["k", "k", "j", "i", "7", "Enter"][..], + &["k", "k", "j", "i", "Tab"][..], + ] { + let mut tui = four_by_two_host(); + tui.cursor_mut().selected = (3, 0); + for &k in keys { + reduce_key(&mut tui, BrowserKey::new(k).to_key_event().unwrap()); + } + let (mut host, session, mut client) = + placed_client(four_by_two_host, (2, 6), ((3, 0), 2, 0)); + let mut held = Vec::new(); + for (i, &k) in keys.iter().enumerate() { + if i == 3 { + let cursor = client.view.cursor(); + assert_eq!( + ( + cursor.selected, + cursor.row_offset, + client.grid.row_frame.first + ), + ((2, 0), 1, 2), + "setup: inside the held window, the offsets not its origin" + ); + } + held.extend(sent_or_upstream(&mut host, session, &mut client, k)); + } + assert_eq!(document_text(host.app()), document_text(&tui), "{keys:?}"); + assert_eq!( + client.view.cursor().selected, + tui.view_state.cursor().selected + ); + // An editor reopened after a write awaits the host's seed (p6v6): read it + // only once the held replies arrive; an unawaited one is read now as well. + let awaiting = client + .view + .opened_editor + .as_ref() + .is_some_and(|o| o.awaiting); + if !awaiting { + assert_editor_agrees(&client, &tui, keys, "held"); + } + deliver(held, session, &mut client); + assert_editor_agrees(&client, &tui, keys, "delivered"); + } +} + +fn assert_editor_agrees(client: &ClientState, tui: &App, keys: &[&str], when: &str) { + assert_eq!( + client.view.buffers.get("edit"), + tui.view_state.buffers.get("edit"), + "{keys:?}, {when}" + ); + assert_eq!( + client.view.opened_editor, tui.view_state.opened_editor, + "{keys:?}, {when}: the editor opened on another cell's seed" + ); +}