diff --git a/Cargo.lock b/Cargo.lock index b80ea5bd..961d6d68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -897,6 +897,7 @@ dependencies = [ "pest_derive", "pest_meta", "proptest", + "serde", ] [[package]] diff --git a/context/code/clients.md b/context/code/clients.md index 321b5881..7d7a1ea3 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -996,20 +996,39 @@ folded when the resolver runs — the trap that reads as "the cursor lands on `Plan` and the document switches to `Default`", silent and wrong in the half a user does not watch. -The second clause of rule 1 is what makes the formula panel look inconsistent -and is in fact the rule holding. `ModeKey::ViewPanel` binds `Enter` to -`switch-view-at-cursor`; `ModeKey::FormulaPanel` binds `a`/`n`/`o` and -`d`/`Delete` and **no `Enter` at all**. So a view-row click switches and a -formula-row click only navigates. Opening the editor on a formula click would -be an activation *invented for the pointer that the keyboard does not have* — -a second derivation of what a formula row means, which is the disease §5 names. -**The guard for this must be aimed at the keymap, not at the click**: asserting -that the click emits no `FormulaEdit` only restates the author's reading, and a -reviewer demonstrated the gap by adding `Enter -> enter-mode formula-edit` to -`FormulaPanel` and watching the whole workspace stay green while the click and -the keyboard disagreed. `the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does` -asserts the binding's absence and names `click_formula_row` as what must change -if it ever appears. +The second clause of rule 1 is why a formula-row click opens the editor. Until +improvise-km71, `ModeKey::FormulaPanel` bound `a`/`n`/`o` and `d`/`Delete` and +**no `Enter` at all**, so a formula-row click only navigated, and opening the +editor on a click would have been an activation *invented for the pointer that +the keyboard did not have*. km71 bound `Enter` (and `e`) to +`edit-formula-at-cursor`, so the rule now yields the editor on the clicked row, +and `click_formula_row` ends with `edit_formula_at(index)` exactly as +`click_view_row` ends with `switch_view_at(index)`. **The guard is aimed at what +`Enter` resolves to, not at the keymap and not at the click**: +`a_formula_row_click_ends_with_what_enter_does_on_that_row` resolves `Enter` at +cursor 1 and asserts the click on row 1, made with the cursor at 0, *ends with* +those commands. The earlier guard asserted only that `Enter` was unbound, which +tied the two by a comment; this one fails if either side changes without the +other, and it separates "reads the clicked index" from "reads the cursor", +because the two point at different rows. The superseded test was +`the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`. + +Editing needs the client to name *which* formula, and `FormulaCache` used to +carry only the raw text, from which neither the target nor its category is +recoverable without reimplementing `parse_formula` on the client — the gap +`decision(improvise-e24v)` recorded for `delete-formula-at-cursor`. km71 closed +it the gotcha 47 way, by **enriching the projection**: `FormulaCache::formulas` +is `Vec`. The id is opaque on purpose +(`context/code/core.md` §14). `delete-formula-at-cursor` is still declined in +the browser; it now needs only its resolver. + +One divergence is pinned rather than closed: a rename. The host's +`ReplaceFormula` rewrites the status to say what was renamed and which formulas +still name the old target; the client cannot know the old target (the id is +opaque) and folds the optimistic "Formula updated". +`renaming_a_formula_from_the_panel_lands_the_same_document_as_the_host` asserts +both sides of that, as `a_formula_the_client_cannot_parse_diverges_on_the_status_line` +does for a parse error. **The browser assertion that PR #60 dropped stays dropped, and improvise-qf57 is the decision rather than the oversight.** `#grid input` `toHaveCount(0)` was @@ -1026,6 +1045,9 @@ running the spec: `#grid input` `toHaveCount(0)` **passes**, and the run fails on the `#mode` assertion below it. Restoring it would add an assertion that cannot fail — the same defect this file already records one line down, where an explanatory `assert!` was removed for never being able to be the failing one. +Since improvise-km71 the activation is real rather than invented, so the spec's +`#mode` assertion reads `"FORMULA"` and it also asserts that `#minibuffer` holds +the clicked formula's text; the argument about `#grid input` is unchanged. **Rule 2 — a click is all of its intents or none of them** (improvise-hkr8). Each `click_*` returns `Option>` and `click` does the single diff --git a/context/code/command.md b/context/code/command.md index f64aeb55..c5c5e16f 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2280,9 +2280,31 @@ would be sent and every client would stay stale *permanently* rather than until next scroll: a partial fix strictly worse than none. Broadcasting only would leave the server projecting each client a window it is no longer looking at. -The audience is `Broadcast::ToAll`, and the asker is not an exception. Its mirror -was reset here like everyone else's, and it folded nothing optimistically, because -a `Command::Model` is not something a client can apply to itself. +The audience is every session on a reframed view, each addressed with +`Broadcast::To` (improvise-nynk), and the asker is not an exception: a transpose +invalidates its own position too. What an earlier version of this paragraph got +wrong is the claim that the asker "folded nothing optimistically". A +`Command::Model` is not something a client can apply to itself, but the **rest of +its frame** is, and a browser frame routinely places the cursor *after* the +mutation. The `+cat` commit is `[ChangeMode, AddCategory, SetSelected]`. The +mirror folds that in order: reset, then placed, which is exactly the order the TUI +applies the same batch in. The client folded all of it **before** hearing the +reset, so the relayed `ResetCursor` arrived last and threw the placement away. +The client sat at the origin while its mirror, and every projection built from +it, sat at the placement (improvise-wdmp). + +So the host answers the asker in the order the mirror saw. Once the asker has been +told `ResetCursor`, `Host::mutating_frame` relays back to it every **later** +cursor-placing command of the same frame (`SetSelected`, `SetRowOffset`, +`SetColOffset`), ahead of that command's projections. All three are absolute, so +folding them twice is idempotent. Excluding the asker from the reset, which the +ticket proposed, was rejected. For a frame that places nothing after the reframe +it is gotcha 24's own "strictly worse than nothing": the mirror is reset and the +client never hears it. `a_frame_that_reframes_and_then_places_the_cursor_leaves_the_asker_placed` +runs both shapes. One residue is stated rather than handled: a frame that +reframes and then *switches views* before placing would reset the view the +client ends on rather than the one it reframed. No resolver composes that frame +today. What is *not* broadcast is a position: another client's transpose must not move where I am looking — that is UI and it is mine (improvise-g0f). It may only declare @@ -3548,6 +3570,56 @@ unlisted command that diverges fails the test, and so does a listed one that does not diverge. A stale entry therefore cannot linger, and a new registration is covered the day it lands. +## Editing a formula: the capture is on the mode, read in `execute` (improvise-km71) + +`edit-formula-at-cursor` (Enter or `e` in the formula panel) opens +`AppMode::FormulaEdit` with `slot: FormulaSlot::Existing { id, was }` and seeds +the `formula` buffer with the formula's `raw`. That slot is the capture: which +formula, and what it read when the editor opened. Nothing at commit time looks +at the cursor again. + +**Where `commit-buffer formula` reads the slot matters, and the registry is the +wrong place.** The obvious move was to resolve it in `parse_commit_kind`, the +way `Item` takes its category from `AppMode::ItemAdd`. That does not work, and +it fails silently: the interactive arm is built **from `parse`** +(`context/code/cli.md` §8), and `parse` accepts `formula` under +`AppMode::Normal`, so the mode-reading fallback is never reached. `Item` only +works that way because `parse` *refuses* `item`. The first build of km71 did +exactly this, and every edit became an add. `add_formula` upserts by target, so +an edit that kept its target still landed in place, and only the rename tests +and the concurrency tests showed it. Making the interactive arm read the mode +first would break improvise-yf7p's guarantee that an accepted argument list is +never overridden by context. So `CommitBuffer::execute` asks +`CommitKind::in_mode(ctx.mode)`. That is a read of context in `execute`, which +is the answer yf7p gave to `toggle-panel-visibility`. A script line stays an +add, and cannot name an edit, because the slot does not exist outside the +editor. + +**`effect::ReplaceFormula` refuses with `refuse_and_stop`, not `refuse`.** The +Enter binding is `[commit-buffer formula, clear-buffer formula]`, and +`CommitBuffer` puts the change back to `FormulaPanel` *after* the mutation. A +refusal that stops the batch therefore leaves the editor open with the user's +text and the reason in the minibuffer line (`draw_bottom_bar` draws the status +beside the minibuffer). That covers all four refusals: the formula is gone, it +was rewritten since the editor opened (`was`), the new text does not parse, or +the new target belongs to another formula, or the rename is of a formula +outside `_Measure` (improvise-kb1d, below). `AddFormula`'s parse error stops the +batch the same way since improvise-v4hg (#213), so a bad *new* formula also +leaves the editor open with its text; adding and editing now agree. + +**A target change outside `_Measure` is refused (improvise-kb1d).** A regular +category's formula target is a real item of that category, not a derived one. +`replace_formula` added the new target item and left the old one, so renaming +`Total = East + West [Region]` to `Sum` left `Total` as an empty item and +appended `Sum` at the end. Renaming the item in place was the alternative and +was not taken: an item's name is its identity in cells, views, hidden lists and +groups, so that rename is the whole of improvise-sboz, not a small fix. Editing +such a formula without changing its target still works. + +On success the command's optimistic "Formula updated" stands, except on a +rename, where the effect rewrites the status to name the old and new targets +and any formula that still mentions the old one (`context/code/core.md` §18). + ## A wire write reaches only a cell the host projects as a Record (improvise-ciw8) `App::resolve_record` answers "which record does this surrogate name". Its diff --git a/context/code/core.md b/context/code/core.md index 42a3e59c..88f3c81a 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -1292,3 +1292,57 @@ What this does **not** fix: a text cell whose text reads as a number. `"007"` seeds `"007"`, and `typed()` classifies that as `Number(7.0)`. The seed is exact, but the classifier has no way to know the cell was text. That is improvise-ixxx. + +## 18. Which formula an edit means (improvise-km71) + +A `Formula` has no stored id. It lives in `Model::formulas: Vec` and is +identified, in practice and by `add_formula`'s upsert, by `(target, +target_category)`. **`FormulaId` is that pair behind one opaque type**: its +fields are private, `Formula::id()` is its only constructor, and nothing +outside `improvise-formula` can read a name out of it. That is deliberate. The +stable-identity work (improvise-sboz, ADR 005) will make a formula's identity +something other than its names, and every signature that already says +`FormulaId` — `Model::formula`, `Model::replace_formula`, +`ModelCommand::ReplaceFormula`, `FormulaSlot::Existing`, `FormulaEntry` — then +changes by changing this one struct. A name pair spread across those signatures +would have made the swap a sweep. + +**The index is not a candidate.** A position is what `formula_cursor` is, and +the editor must not recompute its target from the cursor at commit +(improvise-ixxx/2wu3's lesson): another client deleting an earlier formula +shifts every index after it, and the commit would land on a neighbour. It would +also put a position on the wire, which `every_model_command_field_is_position_free` +makes a compile error. + +**An edit is its own operation, never remove-then-add and never +`add_formula`.** `add_formula` upserts on `(target, target_category)`: fed an +edit that changes the target, it appends a second formula and leaves the old +one; fed an edit onto another formula's target, it silently replaces *that* +formula. `Model::replace_formula(id, formula)` writes into the slot `id` names, +so the position (and with it `_Measure`'s item order, since +`measure_item_names` appends formula targets in formula order) is kept, and it +**refuses** when the new target belongs to a different formula. + +**A target change is a rename of the same formula.** The id travels with the +edit, so `Profit = …` edited to `Gross = …` is the formula that was `Profit`, +in the same place. That is the least surprising reading of "edit this row", and +it is what ADR 005 proposes. What a rename cannot carry under name identity is +everything else that names the old target: other formulas' references, a +view's hidden list, a `measure_agg` entry. Those cannot follow without rewriting +`raw` text the parser does not print back, so the edit says so out loud: +`ReplaceFormula` reports which formulas still mention the old name +(`Formula::mentions`, over the expression and its `WHERE`), and their cells +show the `ref` error rather than a silent wrong number. Under a stable id those +references are what sboz makes follow. **This holds for `_Measure` targets only.** +Outside `_Measure` a target is a real item, and a rename is refused until sboz +(improvise-kb1d, `context/code/command.md`). A rename also leaves stored cells under +the old `_Measure` name in `## Data`, invisible because the formula shadowed them. + +**Concurrency is checked twice.** `FormulaSlot::Existing { id, was }` is +captured when the editor opens: the id must still resolve, and the formula must +still read `was`. Either failing means another client removed, renamed or +rewrote it meanwhile, and the edit is refused with the batch stopped, so the +editor stays open over the user's text (gotcha 21). `was` is not redundant +with a stable id: a concurrent *replace* keeps the id and changes the text, +which only `was` can see. + diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 4db85da2..426347fb 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -488,11 +488,14 @@ The serde derives that existed only for that path went with it: `Workbook`, `DataStore`'s manual impls and the formula AST. So did indexmap's `serde` feature in `improvise-core`, `improvise-io` and the root crate, and `improvise-formula`'s serde dependency. Each was removed and the whole workspace rebuilt, so the -compiler named every other consumer. What remains is the wire: -`Axis` (whose `rename_all = "lowercase"` is its wire spelling), `PageAxis`, and -`CategoryKind`, which went back on when `TileEntry` began carrying it -(improvise-bqm0; `context/code/command.md`). It returned as a wire type, not as -a document one: nothing loads a `Category` through serde. +compiler named every other consumer. Two remained, and both are the wire: +`Axis` (whose `rename_all = "lowercase"` is its wire spelling) and `PageAxis`. +A third arrived with improvise-km71 by the same rule: `FormulaId`, which crosses +the wire in `ModelCommand::ReplaceFormula`, `FormulaSlot` and `FormulaEntry`. +So `improvise-formula` depends on serde again, for that one type. No derive went +back on the formula AST. A fourth, `CategoryKind`, went back on when `TileEntry` +began carrying it (improvise-bqm0; `context/code/command.md`). It returned as a +wire type, not as a document one: nothing loads a `Category` through serde. `CellValue` was never a candidate, since it is carried on the wire too. JSON *import* (`StartImportWizard`, `ImportJsonHeadless`) is a separate feature and is untouched. diff --git a/context/code/protocol.md b/context/code/protocol.md index 97cdab54..a05c8999 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -84,6 +84,20 @@ naming it would produce a mode with nothing to add to. `Quit` and `start-import-wizard` — which do work beyond changing the mode, so `enter-mode` would be a way to reach half of each. +### `AppMode::FormulaEdit` carries what it will commit to (improvise-km71) + +`FormulaEdit { minibuf, slot }` is one mode for two editors. `slot` is +`FormulaSlot::New` for `a`/`n`/`o` (`enter-mode formula-edit`, which +`from_name` still builds) or `FormulaSlot::Existing { id, was }` for +`edit-formula-at-cursor`. It is one variant and not a second `AppMode` because +the keys, the buffer and the panel are identical: a second variant would add a +`ModeKey`, a keymap and a hint for no difference in what the keys do. The slot +is `ItemAdd`'s `category` again, a capture made when the mode is entered. Like +`ItemAdd` it is not nameable from `from_name`, since a script line has no +formula to open. It must be portable and snapshot-safe (§1 above). `FormulaId` +is plain serde data, and a fresh client that inherits an open edit inherits +exactly the claim the editor made. The commit re-checks that claim. + ### `Panel` `Panel::ALL`/`name()`/`key()` drive the toggle keys for Normal *and* all @@ -164,9 +178,12 @@ 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 29 model effects, minus three: `SwitchView`, which `Command` -already names and routes, and `ApplyAndClearDrill`/`AcceptWizardModel`, which -are **not addressable** (improvise-3x3u). That leaves **26**. A variant with +The count is 27 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 no data mirrors a nullary effect whose operand is document state the authoritative `App` already holds. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index fc1bcacc..41acdf04 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1485,39 +1485,29 @@ view, or with a click on the row already under the cursor, the bug is invisible — the same reason `a_click_focuses_the_panel_moves_its_cursor_and_activates_the_row` clicks Product rather than `_Index`. -### a_formula_row_click_navigates_and_activates_nothing +### a_formula_row_click_navigates_and_opens_the_editor_on_that_row -The negative arm, and it is a statement about the *rule* rather than about the -formula panel. The rule is "go there, then do what `Enter` does there"; -`ModeKey::FormulaPanel`'s keymap binds `a`/`n`/`o` to `enter-mode formula-edit` -and `d`/`Delete` to delete, and **no `Enter` at all**. So the rule yields -navigation and nothing else, and that is the rule applying rather than the fix -stopping half way. +Formerly `a_formula_row_click_navigates_and_activates_nothing`. That was the +negative arm of the rule "go there, then do what `Enter` does there", true while +`ModeKey::FormulaPanel` bound no `Enter`. improvise-km71 bound `Enter` (and +`e`) to `edit-formula-at-cursor`, so the same rule now yields the editor on the +clicked row. The test asserts the exact command vector. The clicked row is +`Margin` at index 1 and the cursor stays at 0, so a click that read the cursor +would open `Profit`. -This test asserts the *exact* command vector, which is the whole of what it can -say. It once carried a second, explanatory `assert!` that no `FormulaEdit` mode -change appears — **which could never have been the failing assertion**, because -the `assert_eq!` above it pins the same three commands and aborts first. A -reviewer demonstrated it by injecting a `ChangeMode(FormulaEdit)` and watching -the panic land on the `assert_eq!` with the explanatory message never printed. -It was a comment wearing an `assert!`, and the argument now lives in -`context/code/clients.md` where a comment run cannot hold it. +The old version once carried an explanatory `assert!` that no `FormulaEdit` +mode change appeared, which could never have been the failing assertion, +because the `assert_eq!` above it pinned the same commands and aborted first. +That lesson stands: an `assert!` after an exact `assert_eq!` on the same vector +is a comment. -### the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does - -The guard that the removed assertion was pretending to be. The asymmetry — a -view-row click activates, a formula-row click does not — is a fact about the -**keymap**, and nothing tied the click to it. The reviewer proved the gap by -adding `Enter -> enter-mode formula-edit` to `ModeKey::FormulaPanel` and running -the whole workspace: **1381 passing, nothing failed**, while `click_formula_row` -still activated nothing. The click and the keyboard then disagree about what a -formula row does, silently. - -Injecting that same binding now fails this test and only this test. The failure -message names `click_formula_row` as what must change, because the person who -adds an `Enter` binding is not the person reading `resolve.rs`'s click arms. The -positive half (`ViewPanel` *does* bind `Enter`) is there so the test cannot pass -by asserting that no panel binds anything. +The guard that tied the click to the keymap was +`the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`. A reviewer +showed that nothing else tied them: adding an `Enter` binding to `FormulaPanel` +left 1381 tests passing while `click_formula_row` still activated nothing. That +test is replaced by `a_formula_row_click_ends_with_what_enter_does_on_that_row`, +below, which compares the click with what `Enter` actually resolves to rather +than asserting a binding's absence. #### The view and formula row clicks in the browser (improvise-iioi) @@ -4695,6 +4685,29 @@ laundering invisible (the edge) is also the one that makes a wrong-direction jump visible, so moving off the edge fixed one blind spot and opened the other, and each needs its own question. +### a_formula_row_click_ends_with_what_enter_does_on_that_row + +improvise-km71, replacing `the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`. +The old guard asserted that `Enter` was unbound, which tied the click to the +keyboard only by the message in its `assert!`. This one resolves `Enter` at +cursor 1 and asserts that the click on row 1, made with the cursor at 0, ends +with the same commands. Rival: `click_formula_row` activates at +`state.view.formula_cursor` instead of the clicked index. This test and +`a_formula_row_click_navigates_and_opens_the_editor_on_that_row` both fail. The +cursor and the index differ in the fixture, which is what lets them fail. + +### renaming_a_formula_from_the_panel_lands_the_same_document_as_the_host + +improvise-km71. `editing_a_formula_from_the_panel_agrees_with_the_host` alone +cannot separate a browser that composes `ReplaceFormula` from one that composes +`AddFormula`: the upsert lands a same-target edit in place on the twin too. +Rival: `CommitKind::of` ignores the slot. Both lockstep tests fail. The edit +test fails only on the status line ("Formula added" against "Formula +updated"). This test fails on the **document**, which is the property. It also +pins a real divergence: on a rename the host says what was renamed and the +client cannot, because `FormulaId` is opaque and it does not know the old +target. + **improvise-w7qd, and the floor was the hole.** As first written the test covered `move-selection` and a four-entry jump table, behind `pressed >= 14`. Two things passed the whole workspace, both found in re-review of #180 at diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 448455eb..70cb9478 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5655,6 +5655,46 @@ across two independently built ones. If a future refactor made the fixture share one map, or share one `RandomState`, the second assertion fails. Without this test the law would then pass everything and look healthy. +### changing_the_target_renames_the_formula_in_its_own_place + +improvise-km71. This is the test that separates "edit" from "add". The rival +was the first build: `commit-buffer formula` was resolved in `parse_commit_kind`, +where the registry's parse-first rule never reaches the mode, so every edit +became an `AddFormula`. `committing_an_edit_replaces_the_formula_in_place` +**passed** under that rival, because `add_formula` upserts by +`(target, target_category)` and an edit that keeps its target lands in the same +slot. Injected on purpose (the commit emits `AddFormula` for the replace kind): +this test, the rename-status test, the collision test, the parse-error test and +both concurrency-refusal tests fail, and the in-place test passes. A rename is the input the upsert cannot fake. The fixture has three +formulas with the cursor on index 1, so "the one under the cursor" is not also +"the first" or "the last". + +### an_edit_lands_on_its_formula_after_an_earlier_one_is_deleted_meanwhile + +improvise-km71. The capture is made when the editor opens and never re-read +from the cursor. Rival: `ReplaceFormula` resolves its formula from +`formula_cursor` at commit. The fixture deletes `Profit` (index 0) while the +editor is open on `Tax` (index 1), so cursor 1 now names `Margin`. The rival +rewrote `Margin`, and this test fails, along with +`an_edit_whose_formula_was_deleted_meanwhile_refuses_out_loud` and +`an_edit_whose_formula_was_changed_meanwhile_refuses_out_loud`. The other nine +formula-edit tests pass under it, because none of them moves the list. + +### an_edit_whose_formula_was_changed_meanwhile_refuses_out_loud + +improvise-km71. Another client rewrote `Tax`'s text but kept its target, so the +id still resolves. Only the `was` check can see this. Rival: the `was` filter +deleted. This test is the only one that fails. That is the case for keeping +`was` after improvise-sboz gives formulas a stable id: a concurrent replace +keeps the id. + +### a_rename_onto_another_formulas_target_refuses_and_keeps_both + +improvise-km71. Rival: `Model::replace_formula`'s collision check disabled. +This test and `replace_formula_refuses_to_displace_another_formula` fail. +Without the check, renaming `Tax` to `Margin` leaves two formulas with one +identity, and `formula(&id)` then finds the first. + ### a_fresh_session_has_no_history_to_step_back_through improvise-lxg0. `project.rs::fresh_view` built a new session's slice by folding @@ -5930,6 +5970,16 @@ host's `Normal`, and nothing else different. `install_workbook` needs no change of its own, because both of its callers (`LoadModel`, `AcceptWizardModel`) run inside `apply_effects`, which settles after the batch. +### renaming_a_formula_outside_measure_refuses_and_keeps_the_editor + +improvise-kb1d, found by the pre-merge review of #214 and by ChatGPT Codex (P1). +The fixture has a Region-targeted formula at index 1, between two `_Measure` +formulas, because every other km71 test used `_Measure`, whose items are +derived from the formula list and cannot show a stranded item. Rival: the guard +in `Model::replace_formula` disabled. This test fails on the formula list, and +`replace_formula_refuses_to_rename_a_target_outside_measure` in core fails too. + + ### `an_untouched_pivot_edit_of_a_misread_value_writes_nothing` ### `an_untouched_records_edit_of_a_misread_value_writes_nothing` @@ -6077,3 +6127,27 @@ four sites. It reverts each `effect::refuse(` to `effect::set_status(` alone and runs `cargo test --workspace --no-fail-fast`. Four of four fail this test, and nothing else fails. None of the four needs an abort: each is the only effect its command emits, and `p`/`i` are single bindings, not sequences. + +### 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, +for example the records `+cat` commit `[ChangeMode, AddCategory, SetSelected]`, is +folded by the client all at once and before any answer arrives. The host's mirror +folds the same frame in order. `adopt_cursor_epoch` resets the mirror at the +mutation, and the later `SetSelected` places it. The relayed `ResetCursor` +reached the client **after** its own placement and undid it. So the client sat at +the origin while its mirror, and every projection built from it, sat at the +placement. That is a split, not merely the jump the ticket described. The test +drives a real `Host` and a real `ClientState` through `fold_outgoing`, then folds +every frame addressed to the asker. It seats the cursor off the origin first, so +a reset that never arrived is visible. It runs two shapes: with a placement +after the reframe (expect `(1, 0)`), and without one (expect `(0, 0)`, gvnb's +reset). In both, the client's cursor must equal its mirror's. At the base the +placed case failed: the client was at `(0, 0)` and the mirror at `(1, 0)`. + +Two rivals. First, the echo disabled, which is the pre-fix behaviour: the placed +case fails. Second, the ticket's suggested fix, which drops the asker's +`ResetCursor` from the relay: the unplaced case fails, because the mirror was +reset and the client never heard. That rival also fails +`a_geometry_command_relays_the_cursor_reset_to_the_asker_too` and the +ws-server's `the_other_document_would_also_have_a_cursor_to_invalidate`. diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index ce85543d..ad72ce04 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -1,5 +1,7 @@ +use crate::formula::FormulaId; use crate::model::category::MEASURE_CATEGORY; use crate::model::cell::CellKey; +use crate::protocol::FormulaSlot; use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect}; use crate::view::RecordCol; @@ -1042,6 +1044,12 @@ impl Cmd for CommitAndAdvance { #[derive(Debug, Clone, PartialEq, Eq)] pub enum CommitKind { Formula, + /// Edit the formula the editor was opened on. Never parsed: `execute` reads + /// it off the mode's slot, the capture made at open (improvise-km71). + ReplaceFormula { + id: FormulaId, + was: String, + }, Category, /// Add an item to this category, resolved from [`AppMode::ItemAdd`] at /// construction so `execute` stays pure over the kind and its buffer. @@ -1065,10 +1073,28 @@ pub fn parse_commit_kind(arg: &str, mode: &AppMode) -> Result CommitKind { + match (self, mode) { + ( + Self::Formula, + AppMode::FormulaEdit { + slot: FormulaSlot::Existing { id, was }, + .. + }, + ) => Self::ReplaceFormula { + id: id.clone(), + was: was.clone(), + }, + (kind, _) => kind.clone(), + } + } + /// The minibuffer this kind consumes. pub fn buffer(&self) -> &'static str { match self { - Self::Formula => "formula", + Self::Formula | Self::ReplaceFormula { .. } => "formula", Self::Category => "category", Self::Item { .. } => "item", } @@ -1078,7 +1104,7 @@ impl CommitKind { /// minibuffer. fn on_empty(&self) -> AppMode { match self { - Self::Formula => AppMode::FormulaPanel, + Self::Formula | Self::ReplaceFormula { .. } => AppMode::FormulaPanel, Self::Category | Self::Item { .. } => AppMode::CategoryPanel, } } @@ -1087,7 +1113,7 @@ impl CommitKind { /// categories and items are entered in batches, formulas are not. fn on_commit(&self) -> Option { match self { - Self::Formula => Some(AppMode::FormulaPanel), + Self::Formula | Self::ReplaceFormula { .. } => Some(AppMode::FormulaPanel), Self::Category | Self::Item { .. } => None, } } @@ -1095,6 +1121,7 @@ impl CommitKind { fn status(&self, name: &str) -> String { match self { Self::Formula => "Formula added".to_string(), + Self::ReplaceFormula { .. } => "Formula updated".to_string(), Self::Category => format!("Added category \"{name}\""), Self::Item { .. } => format!("Added \"{name}\""), } @@ -1108,6 +1135,11 @@ impl CommitKind { raw: name.to_string(), target_category: "_Measure".to_string(), }), + Self::ReplaceFormula { id, was } => Box::new(effect::ReplaceFormula { + id: id.clone(), + was: was.clone(), + raw: name.to_string(), + }), Self::Category => Box::new(effect::AddCategory(name.to_string())), Self::Item { category } => Box::new(effect::AddItem { category: category.clone(), @@ -1128,18 +1160,19 @@ impl Cmd for CommitBuffer { "commit-buffer" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let raw = read_buffer(ctx, self.kind.buffer()); + let kind = self.kind.in_mode(ctx.mode); + let raw = read_buffer(ctx, kind.buffer()); let name = raw.trim(); if name.is_empty() { - return vec![effect::change_mode(self.kind.on_empty())]; + return vec![effect::change_mode(kind.on_empty())]; } let mut effects: Vec> = vec![ - effect::set_status(self.kind.status(name)), + effect::set_status(kind.status(name)), // The add goes last: it is the only party that knows whether it worked, // so the optimistic status must not clobber its error (improvise-6t3). - self.kind.add_effect(name), + kind.add_effect(name), ]; - effects.extend(self.kind.on_commit().map(effect::change_mode)); + effects.extend(kind.on_commit().map(effect::change_mode)); effects } } diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 6793c910..4cc3dc70 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -4,7 +4,7 @@ use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect, Panel, ViewTarget}; use crate::view::Axis; -use super::core::{Cmd, CmdContext}; +use super::core::{Cmd, CmdContext, write_buffer}; #[cfg(test)] mod tests { @@ -685,6 +685,25 @@ impl Cmd for DeleteFormulaAtCursor { } } +/// Open the formula editor on the formula under the cursor, seeded with its +/// text and naming it from here on — the commit never re-reads the cursor. +#[derive(Debug)] +pub struct EditFormulaAtCursor; +impl Cmd for EditFormulaAtCursor { + fn name(&self) -> &'static str { + "edit-formula-at-cursor" + } + fn execute(&self, ctx: &CmdContext) -> Vec> { + match ctx.model.formulas().get(ctx.formula_cursor) { + Some(f) => vec![ + effect::change_mode(AppMode::formula_replace(f.id(), f.raw.clone())), + write_buffer("formula", f.raw.clone()), + ], + None => vec![], + } + } +} + // ── Category panel commands ───────────────────────────────────────────────── /// Cycle the axis assignment of the category at the cursor. diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 176bd8ae..e394c118 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -382,6 +382,7 @@ pub fn default_registry() -> CmdRegistry { None, ); r.register_nullary(|| Box::new(DeleteFormulaAtCursor)); + r.register_nullary(|| Box::new(EditFormulaAtCursor)); r.register_nullary(|| Box::new(AddRecordRow)); r.register_nullary(|| Box::new(TogglePruneEmpty)); r.register_nullary(|| Box::new(ToggleRecordsMode)); diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 6cb8d7cc..b323249e 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -4864,3 +4864,261 @@ mod basis_tiers { } } } + +/// Editing a formula from the panel (improvise-km71), driven through +/// `handle_key` at a NONZERO cursor over three formulas. +#[cfg(test)] +mod formula_edit_tests { + use super::*; + use crate::formula::parse_formula; + + const PROFIT: &str = "Profit = Revenue - Cost"; + const TAX: &str = "Tax = Revenue * 0.1"; + const MARGIN: &str = "Margin = Profit / Revenue"; + + fn app_with_three_formulas() -> App { + let mut wb = Workbook::new("T"); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + for raw in [PROFIT, TAX, MARGIN] { + wb.model + .add_formula(parse_formula(raw, "_Measure").unwrap()); + } + let mut app = App::new(wb, None); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + app + } + + fn press(app: &mut App, code: KeyCode) { + app.handle_key(KeyEvent::new(code, KeyModifiers::NONE)) + .unwrap(); + } + + /// Clear the minibuffer by keys and type `text`, as a user would. + fn retype(app: &mut App, text: &str) { + let len = app.view_state.buffers.get("formula").map_or(0, String::len); + for _ in 0..len { + press(app, KeyCode::Backspace); + } + for c in text.chars() { + press(app, KeyCode::Char(c)); + } + } + + fn raws(app: &App) -> Vec { + app.model_state + .workbook + .model + .formulas() + .iter() + .map(|f| f.raw.clone()) + .collect() + } + + fn buffer(app: &App) -> Option<&str> { + app.view_state.buffers.get("formula").map(String::as_str) + } + + fn open_editor(app: &mut App) { + press(app, KeyCode::Enter); + assert!( + matches!(app.view_state.mode, AppMode::FormulaEdit { .. }), + "Enter on a formula must open the editor, got {:?}", + app.view_state.mode + ); + } + + fn in_editor(app: &App) -> bool { + matches!(app.view_state.mode, AppMode::FormulaEdit { .. }) + } + + #[test] + fn enter_opens_the_editor_seeded_with_the_formula_under_the_cursor() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + assert_eq!(buffer(&app), Some(TAX), "cursor 1 is Tax, not the first"); + } + + #[test] + fn e_opens_the_same_editor() { + let mut app = app_with_three_formulas(); + press(&mut app, KeyCode::Char('e')); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some(TAX)); + } + + #[test] + fn committing_an_edit_replaces_the_formula_in_place() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Tax = Revenue * 0.2", MARGIN]); + assert_eq!(app.view_state.mode, AppMode::FormulaPanel); + } + + /// The rival "add instead of replace" passes the test above, because + /// `add_formula` replaces by target. A rename is where it cannot hide. + #[test] + fn changing_the_target_renames_the_formula_in_its_own_place() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Levy = Revenue * 0.1"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Levy = Revenue * 0.1", MARGIN]); + let measures = app.model_state.workbook.model.measure_item_names(); + assert!( + !measures.contains(&"Tax".to_string()), + "Tax was a formula target only, so it goes: {measures:?}" + ); + } + + #[test] + fn a_rename_says_which_formulas_still_name_the_old_target() { + let mut app = app_with_three_formulas(); + app.view_state.formula_cursor = 0; + open_editor(&mut app); + retype(&mut app, "Gross = Revenue - Cost"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app)[0], "Gross = Revenue - Cost"); + let status = &app.view_state.status_msg; + assert!( + status.contains("Profit") && status.contains("Margin"), + "Margin still refers to Profit, and the user must be told: {status:?}" + ); + } + + #[test] + fn a_rename_onto_another_formulas_target_refuses_and_keeps_both() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Margin = Revenue * 0.1"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, TAX, MARGIN]); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert!(app.view_state.status_msg.contains("Margin")); + } + + #[test] + fn a_parse_error_keeps_the_editor_the_text_and_the_formula() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = = "); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, TAX, MARGIN]); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some("Tax = = ")); + assert!( + app.view_state.status_msg.contains("Formula error"), + "{:?}", + app.view_state.status_msg + ); + } + + /// The editor names its formula when it OPENS. Deleting an earlier one + /// moves the edited formula to index 0 and puts `Margin` under cursor 1. + #[test] + fn an_edit_lands_on_its_formula_after_an_earlier_one_is_deleted_meanwhile() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + let model = &mut app.model_state.workbook.model; + model.remove_formula("Profit", "_Measure"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec!["Tax = Revenue * 0.2", MARGIN]); + } + + #[test] + fn an_edit_whose_formula_was_deleted_meanwhile_refuses_out_loud() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + let model = &mut app.model_state.workbook.model; + model.remove_formula("Tax", "_Measure"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, MARGIN], "nothing re-added"); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some("Tax = Revenue * 0.2")); + assert!(app.last_batch_refused()); + } + + #[test] + fn an_edit_whose_formula_was_changed_meanwhile_refuses_out_loud() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + let theirs = parse_formula("Tax = Revenue * 0.3", "_Measure").unwrap(); + app.model_state.workbook.model.add_formula(theirs); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Tax = Revenue * 0.3", MARGIN]); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert!(app.last_batch_refused()); + } + + #[test] + fn esc_leaves_the_formula_untouched() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = 0"); + press(&mut app, KeyCode::Esc); + assert_eq!(raws(&app), vec![PROFIT, TAX, MARGIN]); + assert_eq!(app.view_state.mode, AppMode::FormulaPanel); + } + + /// `n` still adds: the replace slot must not leak into a fresh editor. + #[test] + fn a_new_formula_after_an_edit_is_added_not_replaced() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + press(&mut app, KeyCode::Esc); + press(&mut app, KeyCode::Char('n')); + assert_eq!(buffer(&app), Some(""), "a new formula starts empty"); + retype(&mut app, "Levy = Revenue * 0.3"); + press(&mut app, KeyCode::Enter); + assert_eq!( + raws(&app), + vec![PROFIT, TAX, MARGIN, "Levy = Revenue * 0.3"] + ); + } + + /// improvise-kb1d: a Region-targeted formula at index 1. Renaming it would + /// strand `Total` as an empty item, so the edit refuses and stays open. + #[test] + fn renaming_a_formula_outside_measure_refuses_and_keeps_the_editor() { + let mut wb = Workbook::new("T"); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("Region", "West").unwrap(); + for (raw, cat) in [ + (PROFIT, "_Measure"), + ("Total = East + West", "Region"), + (MARGIN, "_Measure"), + ] { + wb.model.add_formula(parse_formula(raw, cat).unwrap()); + } + let mut app = App::new(wb, None); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + let regions = |app: &App| { + let region = app.model_state.workbook.model.category("Region").unwrap(); + region + .ordered_item_names() + .iter() + .map(|s| s.to_string()) + .collect::>() + }; + let before = regions(&app); + open_editor(&mut app); + retype(&mut app, "Sum = East + West"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Total = East + West", MARGIN]); + assert_eq!(regions(&app), before, "no ghost item, nothing appended"); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some("Sum = East + West")); + } +} diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 04ef04a4..e8fe85d3 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -2,6 +2,8 @@ use std::collections::BTreeMap; use std::fmt::Debug; use std::path::PathBuf; +use crate::formula::FormulaId; +use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; use crate::view::{Axis, RecordCol, RecordRow}; @@ -246,6 +248,61 @@ impl Effect for RemoveFormula { } } +/// Edit the formula `id` names in place. Every refusal stops the batch, so the +/// editor stays open over the user's text — `context/code/command.md`. +#[derive(Debug)] +pub struct ReplaceFormula { + pub id: FormulaId, + pub was: String, + pub raw: String, +} +impl Effect for ReplaceFormula { + fn apply(&self, app: &mut App) { + match self.replace(&mut app.model_state.workbook.model) { + Ok(Some(status)) => app.view_state.status_msg = status, + Ok(None) => {} + Err(why) => refuse_and_stop(app, why), + } + } + fn kind(&self) -> EffectKind { + EffectKind::Model(ModelChange::CellData) + } +} + +impl ReplaceFormula { + /// The replacement, and what a rename leaves naming the old target. + fn replace(&self, model: &mut Model) -> Result, String> { + let old = model + .formula(&self.id) + .filter(|f| f.raw == self.was) + .ok_or_else(|| format!("\"{}\" was changed or removed meanwhile", self.was))?; + let old_target = old.target.clone(); + let new = crate::formula::parse_formula(&self.raw, &old.target_category) + .map_err(|e| format!("Formula error: {e}"))?; + let new_target = new.target.clone(); + model + .replace_formula(&self.id, new) + .map_err(|e| e.to_string())?; + Ok((new_target != old_target).then(|| renamed(model, &old_target, &new_target))) + } +} + +fn renamed(model: &Model, old: &str, new: &str) -> String { + let still: Vec<&str> = model + .formulas() + .iter() + .filter(|f| f.mentions(old)) + .map(|f| f.target.as_str()) + .collect(); + match still.is_empty() { + true => format!("Renamed {old} to {new}"), + false => format!( + "Renamed {old} to {new}; {old} is still used by {}", + still.join(", ") + ), + } +} + /// Refresh the derived grid layout mid-batch: `apply_effects` rebuilds only once /// after the batch, so an effect that *reads* it needs this first (improvise-ehz). #[derive(Debug)] diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 86cd8769..0028ace2 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -162,6 +162,7 @@ impl Host { let undo = self.app.snapshot(); let mirrors = self.sessions.clone(); let mut out = Vec::new(); + let mut asker_reset = false; for cmd in batch { if let Some(reason) = reduce::broken_chain(&self.app, cmd.command()) { return self.undo_frame(id, undo, mirrors, reason); @@ -169,7 +170,23 @@ impl Host { // Only a mutation runs `apply_effects`, which resets both flags — so // after anything else they are STALE. A refusal need not abort (cz8d). let mutation = matches!(cmd.command(), Command::Model { .. }); - out.extend(self.command(id, cmd)); + let command = cmd.command(); + let mut answered = self.command(id, cmd); + // The asker folded this placement BEFORE hearing its reset, so it is owed + // again after it (improvise-wdmp, `context/code/command.md`). + if asker_reset && places_cursor(command) { + answered.insert( + 0, + Addressed { + to: id, + msg: ServerFrame::Commands(vec![command.clone()]), + }, + ); + } + asker_reset |= answered + .iter() + .any(|a| a.to == id && is_cursor_reset(&a.msg)); + out.extend(answered); let failed = self.app.last_batch_refused() || self.app.last_batch_aborted(); if mutation && failed { return self.undo_frame(id, undo, mirrors, Rejection::MutationRefused); @@ -266,6 +283,19 @@ impl Host { } } +/// A command that writes this client's cursor, and so survives a reset only if +/// it is folded after it. +fn places_cursor(cmd: &Command) -> bool { + matches!( + cmd, + Command::SetSelected { .. } | Command::SetRowOffset(_) | Command::SetColOffset(_) + ) +} + +fn is_cursor_reset(msg: &ServerFrame) -> bool { + matches!(msg, ServerFrame::Commands(cmds) if cmds.contains(&Command::ResetCursor)) +} + /// One file per way `ImportJsonHeadless` can fail, plus one that imports; the /// `absent.*` paths are left unwritten. Shared by the frame and key-path tests. #[cfg(test)] @@ -1706,6 +1736,71 @@ mod tests { } } + /// improvise-wdmp: the asker's own frame placed its cursor after the reframe, + /// and the relayed reset undid it. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_frame_that_reframes_and_then_places_the_cursor_leaves_the_asker_placed() { + use improvise_protocol::{AppMode, ClientState}; + + // With a placement after the reframe, and without one: the asker ends where + // the TUI would, and its client agrees with its mirror either way. + for (placed, expected) in [(true, (1, 0)), (false, (0, 0))] { + let mut host = host(); + let (asker, snapshot) = host.connect(10, 10); + let (bystander, _) = host.connect(10, 10); + let mut client = ClientState::default(); + match snapshot.msg { + ServerFrame::Snapshot(s) => client.bootstrap(*s), + other => panic!("setup: connect answers a Snapshot, got {other:?}"), + } + let send = |host: &mut Host, client: &mut ClientState, frame: Vec| { + client.fold_outgoing(&frame); + let answer = host.handle(asker, ClientFrame::Commands(frame)); + for msg in to(&answer, asker) { + match msg { + ServerFrame::Commands(cmds) => cmds.iter().for_each(|c| client.fold(c)), + ServerFrame::Accepted => client.accepted(), + other => panic!("the frame must be accepted: {other:?}"), + } + } + answer + }; + // Off the origin first, so a reset that never arrived is visible. + send( + &mut host, + &mut client, + vec![Command::SetSelected { row: 1, col: 0 }], + ); + + // The `+cat` commit's shape: tidy-up, the schema change, then (maybe) + // the cursor put back where the user was (resolve.rs::create_column). + let add = ModelCommand::AddCategory("Product".into()); + let mut frame = vec![ + Command::ChangeMode(AppMode::Normal), + crate::ui::reduce::caught_up(host.app(), add), + ]; + if placed { + frame.push(Command::SetSelected { row: 1, col: 0 }); + } + let answer = send(&mut host, &mut client, frame); + assert!( + to(&answer, bystander).contains(&ServerFrame::Commands(vec![Command::ResetCursor])), + "setup: the frame must really reframe, or nothing here is at stake" + ); + + let mirror = host.sessions.get(asker).unwrap().view().cursor(); + assert_eq!( + mirror.selected, expected, + "setup, placed={placed}: the mirror" + ); + assert_eq!( + client.view.cursor(), + mirror, + "placed={placed}: the asker's client and its mirror disagree" + ); + } + } + /// A refused batch is answered to the **asker and nobody else** (ADR 001): /// nothing happened, so there is nothing to tell the others. #[test] diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index f9be4e45..b289a648 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -15,9 +15,9 @@ use improvise_io::import::analyzer::{DateComponent, FieldKind}; use improvise_io::import::wizard::{ImportWizard, WizardStep}; use improvise_protocol::{ AxisFrame, Basis, BasisCache, CategoryRow, CategoryTreeCache, CategoryTreeUpdate, CellDisplay, - CellId, Command, CreateKind, DocumentCache, FormulaCache, FrameEntry, FrameGroup, GridCache, - GridExtent, GridUpdate, Projection, RecordCell, Rejection, Snapshot, TileBarCache, TileEntry, - ViewListCache, ViewState, ViewSummary, WidgetCache, WizardCache, WizardDateField, + CellId, Command, CreateKind, DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, + GridCache, GridExtent, GridUpdate, Projection, RecordCell, Rejection, Snapshot, TileBarCache, + TileEntry, ViewListCache, ViewState, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind, WizardProposal, WizardStepKind, basis, reduce_view, }; @@ -375,14 +375,15 @@ pub fn category_rows(model: &Model, view: &View) -> Vec { .collect() } -/// The raw text of every formula, which is all the formula panel draws. -pub fn formula_texts(app: &App) -> Vec { +/// Every formula's text, which the panel draws, and its identity, which an +/// edit names it by (improvise-km71). +pub fn formula_entries(app: &App) -> Vec { app.model_state .workbook .model .formulas() .iter() - .map(|f| f.raw.clone()) + .map(FormulaEntry::from) .collect() } @@ -441,10 +442,10 @@ pub fn category_tree_in(app: &App, view: &View) -> CategoryTreeCache { } } -/// The formula panel's cache: the raw text of every formula. +/// The formula panel's cache: every formula's text and identity. pub fn formulas_of(app: &App) -> FormulaCache { FormulaCache { - formulas: formula_texts(app), + formulas: formula_entries(app), } } @@ -1828,8 +1829,13 @@ mod tests { client.fold(&projections); } assert_eq!( - client.formulas.formulas, - vec!["Total = Revenue".to_string()], + client + .formulas + .formulas + .iter() + .map(|f| f.raw.as_str()) + .collect::>(), + vec!["Total = Revenue"], "the client must be holding the formula before it can lose it" ); @@ -1843,7 +1849,7 @@ mod tests { ); assert_eq!( client.formulas.formulas, - Vec::::new(), + Vec::::new(), "the client is still showing a formula the document no longer has" ); } @@ -1868,8 +1874,13 @@ mod tests { let mut client = ClientCaches::default(); client.fold(&snapshot.seed); assert_eq!( - client.formulas.formulas, - vec!["Total = Revenue".to_string()], + client + .formulas + .formulas + .iter() + .map(|f| f.raw.as_str()) + .collect::>(), + vec!["Total = Revenue"], "the client must be holding the formula before it can lose it" ); @@ -1892,7 +1903,7 @@ mod tests { ); assert_eq!( client.formulas.formulas, - Vec::::new(), + Vec::::new(), "the client is still showing a formula the document no longer has" ); } diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index dcc8f070..37d7e5db 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -180,6 +180,11 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec> { target: target.clone(), target_category: target_category.clone(), }), + ModelCommand::ReplaceFormula { id, was, raw } => Box::new(effect::ReplaceFormula { + id: id.clone(), + was: was.clone(), + raw: raw.clone(), + }), ModelCommand::CleanEmptyRecords => Box::new(effect::CleanEmptyRecords), ModelCommand::SortData => Box::new(effect::SortData), @@ -687,6 +692,13 @@ pub(crate) mod tests { target: "Profit".into(), target_category: "_Measure".into(), }, + ModelCommand::ReplaceFormula { + id: crate::formula::parse_formula("Profit = Revenue", "_Measure") + .unwrap() + .id(), + was: "Profit = Revenue".into(), + raw: "Profit = Revenue - Cost".into(), + }, ModelCommand::CleanEmptyRecords, ModelCommand::SortData, ModelCommand::AddCategory("Product".into()), @@ -751,6 +763,7 @@ pub(crate) mod tests { ModelCommand::RemoveItem { .. } => "RemoveItem", ModelCommand::AddFormula { .. } => "AddFormula", ModelCommand::RemoveFormula { .. } => "RemoveFormula", + ModelCommand::ReplaceFormula { .. } => "ReplaceFormula", ModelCommand::CleanEmptyRecords => "CleanEmptyRecords", ModelCommand::SortData => "SortData", ModelCommand::AddCategory(_) => "AddCategory", diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 8ae92d91..a27460e8 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -5,7 +5,7 @@ use indexmap::{IndexMap, IndexSet}; use super::category::{Category, CategoryId, CategoryKind, MEASURE_CATEGORY}; use super::cell::{CellKey, CellValue, DataStore}; -use crate::formula::{AggFunc, Expr, Filter, Formula}; +use crate::formula::{AggFunc, Expr, Filter, Formula, FormulaId}; const MAX_CATEGORIES: usize = 12; @@ -496,6 +496,38 @@ impl Model { &self.formulas } + pub fn formula(&self, id: &FormulaId) -> Option<&Formula> { + self.formulas.iter().find(|f| f.id() == *id) + } + + /// Put `formula` where `id` is, keeping its position; refuses rather than + /// let a new target displace another formula — `context/code/core.md` §18. + pub fn replace_formula(&mut self, id: &FormulaId, formula: Formula) -> Result<()> { + let new_id = formula.id(); + let renames = new_id != *id; + // A regular category's target is a real item; renaming it is sboz's (kb1d). + if renames && !CategoryKind::of_name(&formula.target_category).is_measure() { + return Err(anyhow!( + "Renaming a formula outside _Measure is not supported yet; delete and re-add it" + )); + } + if renames && self.formula(&new_id).is_some() { + return Err(anyhow!("A formula for {} already exists", formula.target)); + } + let pos = self + .formulas + .iter() + .position(|f| f.id() == *id) + .ok_or_else(|| anyhow!("That formula no longer exists"))?; + if !CategoryKind::of_name(&formula.target_category).is_measure() + && let Some(cat) = self.categories.get_mut(&formula.target_category) + { + cat.add_item(&formula.target); + } + self.formulas[pos] = formula; + Ok(()) + } + /// The formula that owns `key`, if one does. **The one home** of "is this /// cell derived" — every other answer to it is a second derivation. pub fn formula_for(&self, key: &CellKey) -> Option<&Formula> { @@ -2137,6 +2169,88 @@ mod formula_tests { assert_eq!(m.evaluate(&k), None); } + fn three_formulas() -> Model { + let mut m = revenue_cost_model(); + for raw in [ + "Profit = Revenue - Cost", + "Tax = Revenue * 0.1", + "Margin = Profit / Revenue", + ] { + m.add_formula(parse_formula(raw, "_Measure").unwrap()); + } + m + } + + fn targets(m: &Model) -> Vec<&str> { + m.formulas.iter().map(|f| f.target.as_str()).collect() + } + + #[test] + fn replace_formula_renames_in_place_and_evaluates_under_the_new_name() { + let mut m = three_formulas(); + let id = m.formulas[1].id(); + m.replace_formula( + &id, + parse_formula("Levy = Revenue * 0.5", "_Measure").unwrap(), + ) + .unwrap(); + assert_eq!(targets(&m), vec!["Profit", "Levy", "Margin"]); + let levy = coord(&[("_Measure", "Levy"), ("Region", "East")]); + assert_eq!(m.evaluate(&levy).and_then(|v| v.as_f64()), Some(500.0)); + } + + #[test] + fn replace_formula_refuses_to_displace_another_formula() { + let mut m = three_formulas(); + let id = m.formulas[1].id(); + let onto_margin = parse_formula("Margin = Revenue", "_Measure").unwrap(); + assert!(m.replace_formula(&id, onto_margin).is_err()); + assert_eq!(targets(&m), vec!["Profit", "Tax", "Margin"]); + } + + #[test] + fn replace_formula_refuses_an_id_that_names_nothing() { + let mut m = three_formulas(); + let id = m.formulas[1].id(); + m.remove_formula("Tax", "_Measure"); + let tax = parse_formula("Tax = Revenue", "_Measure").unwrap(); + assert!(m.replace_formula(&id, tax).is_err()); + assert_eq!(targets(&m), vec!["Profit", "Margin"]); + } + + /// improvise-kb1d: a Region-targeted formula at index 1. A rename would + /// leave `Total` as an empty item and append `Sum`, so it is refused. + #[test] + fn replace_formula_refuses_to_rename_a_target_outside_measure() { + let mut m = three_formulas(); + m.formulas + .insert(1, parse_formula("Total = East + West", "Region").unwrap()); + let shape = |m: &Model| { + let region = m.category("Region").unwrap().ordered_item_names(); + let owned: Vec = region.iter().map(|s| s.to_string()).collect(); + ( + targets(m).iter().map(|s| s.to_string()).collect::>(), + owned, + ) + }; + let before = shape(&m); + let id = m.formulas[1].id(); + let sum = parse_formula("Sum = East + West", "Region").unwrap(); + assert!(m.replace_formula(&id, sum).is_err()); + assert_eq!(shape(&m), before, "nothing moved, nothing added"); + let edit = parse_formula("Total = East - West", "Region").unwrap(); + m.replace_formula(&id, edit).unwrap(); + assert_eq!(targets(&m), vec!["Profit", "Total", "Tax", "Margin"]); + } + + #[test] + fn a_formula_mentions_what_its_expression_or_its_where_names() { + let margin = parse_formula("Margin = Profit / Revenue", "_Measure").unwrap(); + assert!(margin.mentions("Profit") && !margin.mentions("Tax")); + let scoped = parse_formula("East = SUM(Revenue) WHERE Region = \"East\"", "_Measure"); + assert!(scoped.unwrap().mentions("Region")); + } + #[test] fn sum_aggregation_across_region() { let mut m = revenue_cost_model(); diff --git a/crates/improvise-formula/Cargo.toml b/crates/improvise-formula/Cargo.toml index d7459be8..00bf407c 100644 --- a/crates/improvise-formula/Cargo.toml +++ b/crates/improvise-formula/Cargo.toml @@ -10,6 +10,8 @@ repository = "https://github.com/fiddlerwoaroof/improvise" anyhow = "1" pest = "2.9.2" pest_derive = "2.9.2" +# For FormulaId alone, which crosses the wire (improvise-km71); no AST derive. +serde = { version = "1", features = ["derive"] } [dev-dependencies] pest_meta = "2.9.2" diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 57641035..ee1305b5 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -84,4 +84,47 @@ impl Formula { self.format = format; self } + + /// Whether this formula's expression or its `WHERE` names `name`. + pub fn mentions(&self, name: &str) -> bool { + self.expr.mentions(name) || self.filter.as_ref().is_some_and(|f| f.names(name)) + } + + /// Which formula this is. The one constructor of a [`FormulaId`]. + pub fn id(&self) -> FormulaId { + FormulaId { + target: self.target.clone(), + target_category: self.target_category.clone(), + } + } +} + +/// Which formula an edit means, opaque so its content can change in one place: +/// today a target and its category — `context/code/core.md` §18. +#[derive(Debug, Clone, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] +pub struct FormulaId { + target: String, + target_category: String, +} + +impl Expr { + /// Whether this expression names `name` anywhere, as a reference or as + /// either half of a coordinate filter. + pub fn mentions(&self, name: &str) -> bool { + match self { + Expr::Number(_) => false, + Expr::Ref(r) => r == name, + Expr::BinOp(_, l, r) => l.mentions(name) || r.mentions(name), + Expr::UnaryMinus(e) => e.mentions(name), + Expr::Agg(_, e, f) => e.mentions(name) || f.as_ref().is_some_and(|f| f.names(name)), + Expr::If(c, t, e) => c.mentions(name) || t.mentions(name) || e.mentions(name), + Expr::Is(f) => f.names(name), + } + } +} + +impl Filter { + fn names(&self, name: &str) -> bool { + self.category == name || self.item == name + } } diff --git a/crates/improvise-formula/src/lib.rs b/crates/improvise-formula/src/lib.rs index 1ebc5bc8..741811f9 100644 --- a/crates/improvise-formula/src/lib.rs +++ b/crates/improvise-formula/src/lib.rs @@ -2,6 +2,6 @@ pub mod ast; pub mod parser; pub mod quote; -pub use ast::{AggFunc, BinOp, Expr, Filter, Formula}; +pub use ast::{AggFunc, BinOp, Expr, Filter, Formula, FormulaId}; pub use parser::parse_formula; pub use quote::{is_bare_name, quote_name}; diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 117f4fde..19cc648b 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -101,6 +101,7 @@ pub(crate) fn advertised(mode: &AppMode) -> Vec<(&'static str, Option vec![ ("enter-mode", arg("formula-edit"), "new"), + ("edit-formula-at-cursor", None, "edit"), ("delete-formula-at-cursor", None, "delete"), ("move-panel-cursor", None, "nav"), ("enter-mode", arg("normal"), "back"), @@ -720,6 +721,12 @@ fn panels(keys: KeysRow) -> Vec { Some(&["formula-edit"]), "New formula", ), + keys( + &ModeKey::FormulaPanel, + "edit-formula-at-cursor", + None, + "Edit selected formula", + ), keys( &ModeKey::FormulaPanel, "delete-formula-at-cursor", diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 6b7aef43..3231cd18 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -766,6 +766,8 @@ impl Keymaps { "enter-mode", vec!["formula-edit".into()], ) + .bind(KeyCode::Enter, "edit-formula-at-cursor") + .bind(KeyCode::Char('e'), "edit-formula-at-cursor") .delete("delete-formula-at-cursor") .finish(), ), diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 147a0bb5..5f270582 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -30,10 +30,10 @@ pub use keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey}; pub use reduce::{Command, HelpPage, ModelCommand, records_named, reduce_view}; pub use render::{ AxisFrame, CategoryRow, CategoryTreeCache, CategoryTreeUpdate, CellDisplay, CellId, CreateKind, - DocumentCache, FormulaCache, FrameEntry, FrameGroup, GridCache, GridExtent, GridUpdate, - MAX_ROW_HEADER_W, MIN_ROW_HEADER_W, Projection, RecordCell, Snapshot, TileBarCache, TileEntry, - ViewListCache, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind, - WizardProposal, WizardStepKind, header_level_widths, opening_tile, + DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, GridCache, GridExtent, + GridUpdate, MAX_ROW_HEADER_W, MIN_ROW_HEADER_W, Projection, RecordCell, Snapshot, TileBarCache, + TileEntry, ViewListCache, ViewSummary, WidgetCache, WizardCache, WizardDateField, + WizardFieldKind, WizardProposal, WizardStepKind, header_level_widths, opening_tile, }; pub use resolve::{ Click, KeyOutcome, OpenedEditor, Request, click, edited, read_buffer, resolve, typed, @@ -51,6 +51,9 @@ pub use improvise_core::view::layout::PageAxis; // without depending on `improvise-core` directly. pub use improvise_core::model::cell::CellValue; +// On the wire in `ModelCommand::ReplaceFormula` and in `AppMode::FormulaEdit`. +pub use improvise_core::formula::FormulaId; + /// Number of pages in the Help screen. Lives here with the `help_page` state /// it bounds, so help-page effects stay terminal-independent. pub const HELP_PAGE_COUNT: usize = 5; @@ -142,6 +145,7 @@ pub enum AppMode { }, FormulaEdit { minibuf: MinibufferConfig, + slot: FormulaSlot, }, FormulaPanel, CategoryPanel, @@ -179,6 +183,18 @@ pub enum AppMode { }, } +/// What a formula editor commits to, captured when it OPENS and never +/// recomputed at commit — `context/code/protocol.md` §1 (improvise-km71). +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub enum FormulaSlot { + New, + /// Replace the formula `id` names, provided it still reads `was`. + Existing { + id: FormulaId, + was: String, + }, +} + /// `Normal` and `RecordsNormal` are one mode over two layouts, so the mode an /// end holds must answer for the grid it holds — `context/code/protocol.md`. pub fn settled_mode(mode: &AppMode, records: bool) -> AppMode { @@ -336,12 +352,22 @@ impl AppMode { } pub fn formula_edit() -> Self { + Self::formula_editor("formula: ", FormulaSlot::New) + } + + /// The editor opened on an existing formula, which it names from here on. + pub fn formula_replace(id: FormulaId, was: String) -> Self { + Self::formula_editor("edit formula: ", FormulaSlot::Existing { id, was }) + } + + fn formula_editor(prompt: &str, slot: FormulaSlot) -> Self { Self::FormulaEdit { minibuf: MinibufferConfig { buffer_key: "formula".into(), - prompt: "formula: ".into(), + prompt: prompt.into(), color: MinibufColor::Cyan, }, + slot, } } diff --git a/crates/improvise-protocol/src/reduce.rs b/crates/improvise-protocol/src/reduce.rs index 50c75e90..2e744b67 100644 --- a/crates/improvise-protocol/src/reduce.rs +++ b/crates/improvise-protocol/src/reduce.rs @@ -11,7 +11,7 @@ use improvise_core::view::Axis; use crate::basis::Basis; use crate::render::{Projection, RecordCell}; use crate::resolve::OpenedEditor; -use crate::{AppMode, HELP_PAGE_COUNT, KeyCode, KeyModifiers, Panel, ViewState}; +use crate::{AppMode, FormulaId, HELP_PAGE_COUNT, KeyCode, KeyModifiers, Panel, ViewState}; use improvise_core::model::record::RecordHash; /// One message on the wire, in either direction. @@ -79,7 +79,7 @@ pub enum Command { Project(Projection), } -/// A mutation of the shared document: 24 variants, derived not invented (§2). A +/// A mutation of the shared document: 25 variants, derived not invented (§2). A /// view-configuration variant **names its view** (improvise-lltt). #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum ModelCommand { @@ -111,6 +111,13 @@ pub enum ModelCommand { target: String, target_category: String, }, + /// Edit the formula `id` names in place, if it still reads `was` — an edit + /// of that formula, not a remove-then-add (improvise-km71). + ReplaceFormula { + id: FormulaId, + was: String, + raw: String, + }, CleanEmptyRecords, SortData, @@ -180,6 +187,7 @@ pub fn records_named(cmd: &ModelCommand) -> Vec { | ModelCommand::RemoveItem { .. } | ModelCommand::AddFormula { .. } | ModelCommand::RemoveFormula { .. } + | ModelCommand::ReplaceFormula { .. } | ModelCommand::CleanEmptyRecords | ModelCommand::SortData | ModelCommand::AddCategory(_) @@ -593,6 +601,9 @@ mod tests { fn surrogate(_: &RecordHash) -> usize { 1 } + fn formula(_: &FormulaId) -> usize { + 1 + } fn record(RecordCell { record, measure }: &RecordCell) -> usize { surrogate(record) + optional_name(measure) } @@ -627,6 +638,7 @@ mod tests { target, target_category, } => name(target) + name(target_category), + ModelCommand::ReplaceFormula { id, was, raw } => formula(id) + name(was) + name(raw), ModelCommand::CleanEmptyRecords | ModelCommand::SortData => 0, ModelCommand::AddCategory(category) | ModelCommand::RemoveCategory(category) diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index a6f25c10..2306463f 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -10,7 +10,8 @@ use improvise_core::model::record::{AggregateHash, RecordHash}; use improvise_core::view::Axis; use improvise_core::view::layout::PageAxis; -use crate::ViewState; +use crate::{FormulaId, ViewState}; +use improvise_core::formula::Formula; /// A widget's local copy of what it draws. **Total and deterministic**: an /// update naming something absent is dropped, never a panic. @@ -729,17 +730,42 @@ impl WidgetCache for CategoryTreeCache { } } -/// The formula panel's copy of the formula list (improvise-pca). One -/// mutation: add and delete are the only effects, and both send whole. +/// The formula panel's copy of the formula list (improvise-pca). Every +/// formula effect sends it whole. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct FormulaCache { - pub formulas: Vec, + pub formulas: Vec, +} + +/// One formula as a client holds it: its text, and the identity an edit names +/// it by, which the text alone cannot give a client (improvise-km71). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct FormulaEntry { + pub id: FormulaId, + pub raw: String, +} + +impl From<&Formula> for FormulaEntry { + fn from(f: &Formula) -> Self { + Self { + id: f.id(), + raw: f.raw.clone(), + } + } +} + +/// A `_Measure` formula's entry, identified exactly as the host identifies it. +#[cfg(any(test, feature = "test-support"))] +impl From<&str> for FormulaEntry { + fn from(raw: &str) -> Self { + Self::from(&improvise_core::formula::parse_formula(raw, "_Measure").expect(raw)) + } } impl WidgetCache for FormulaCache { - type Update = Vec; + type Update = Vec; - fn apply(&mut self, update: &Vec) { + fn apply(&mut self, update: &Vec) { self.formulas = update.clone(); } } @@ -893,8 +919,8 @@ pub struct WizardCache { pub enum Projection { Grid(GridUpdate), CategoryTree(CategoryTreeUpdate), - /// The raw text of every formula. - Formulas(Vec), + /// Every formula: its text and its identity. + Formulas(Vec), ViewList(Vec), TileBar(Vec), /// Name, dirty marker and where the host keeps it (improvise-z754). @@ -1546,10 +1572,10 @@ mod tests { #[test] fn the_list_caches_hold_the_last_update() { let formulas = FormulaCache::folded(&[ - vec!["Profit = Revenue - Cost".to_string()], + vec!["Profit = Revenue - Cost".into()], vec![ - "Profit = Revenue - Cost".to_string(), - "Tax = Revenue * 0.1".to_string(), + "Profit = Revenue - Cost".into(), + "Tax = Revenue * 0.1".into(), ], ]); assert_eq!(formulas.formulas.len(), 2); @@ -1907,7 +1933,7 @@ mod tests { assert_eq!(booted, (grid, formulas)); assert_eq!(booted.0.cell(0, 0).unwrap().text, "5"); - assert_eq!(booted.1.formulas, vec!["Tax = Revenue * 0.1"]); + assert_eq!(booted.1.formulas, vec!["Tax = Revenue * 0.1".into()]); } /// Two subscribers, each folding only what is addressed to it — the diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index cadf4105..cac800ee 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -9,8 +9,8 @@ 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, GridExtent, HelpPage, KeyCode, KeyEvent, - ModelCommand, Panel, RecordCell, ViewState, records_named, + AppMode, CellId, CellValue, Command, CreateKind, FormulaId, FormulaSlot, GridExtent, HelpPage, + KeyCode, KeyEvent, ModelCommand, Panel, RecordCell, ViewState, records_named, }; /// The virtual category a new measure is filed under, spelled as @@ -187,8 +187,8 @@ fn click_view_row(state: &ClientState, index: usize) -> Option> { Some(commands) } -/// The formula panel binds **no** `Enter`, so the rule yields navigation and -/// nothing else. That is the rule applying, not an omission — clients.md. +/// Focus the panel, move its cursor, then open the editor on the row — what +/// `F` then `Enter` do. `edit-formula-at-cursor` is the activation (km71). fn click_formula_row(state: &ClientState, index: usize) -> Option> { state.formulas.formulas.get(index)?; let mut commands = focus(state, Panel::Formula); @@ -196,9 +196,26 @@ fn click_formula_row(state: &ClientState, index: usize) -> Option> panel: Panel::Formula, cursor: index, }); + // Off `index`, never the cursor: `SetPanelCursor` is not folded yet. + commands.extend(edit_formula_at(state, index)); Some(commands) } +/// `EditFormulaAtCursor::execute` at an arbitrary index: open the editor on +/// that formula, naming it, and seed the buffer with its text. +fn edit_formula_at(state: &ClientState, index: usize) -> Vec { + match state.formulas.formulas.get(index) { + Some(entry) => vec![ + Command::ChangeMode(AppMode::formula_replace( + entry.id.clone(), + entry.raw.clone(), + )), + write_buffer("formula", entry.raw.clone()), + ], + None => vec![], + } +} + /// Focus the panel, move its cursor, then activate — the three things the /// keyboard needs three keystrokes for, and `filter-to-item` is the activation. fn click_category_row(state: &ClientState, index: usize) -> Option> { @@ -456,6 +473,9 @@ fn composed( "Move cursor to a category header to change axis".to_string(), )], }), + // ── the formula panel ──────────────────────────────────────── + "edit-formula-at-cursor" => Some(edit_formula_at(state, state.view.formula_cursor)), + "open-item-add-at-cursor" => Some(match cat_at_cursor(state) { Some(cat) => vec![Command::ChangeMode(AppMode::item_add(cat))], None => vec![Command::SetStatus( @@ -1333,10 +1353,11 @@ fn parse_axis(s: &str) -> Option { } } -/// A mirror of `commit.rs`'s `CommitKind`, resolved against the mode exactly -/// as `parse_commit_kind` does — only `Item` needs the mode. +/// A mirror of `commit.rs`'s `CommitKind`, resolved against the mode as +/// `parse_commit_kind` then `CommitKind::in_mode` do; there is no registry here. enum CommitKind { Formula, + ReplaceFormula { id: FormulaId, was: String }, Category, Item { category: String }, } @@ -1344,6 +1365,16 @@ enum CommitKind { impl CommitKind { fn of(arg: &str, mode: &AppMode) -> Option { match (arg, mode) { + ( + "formula", + AppMode::FormulaEdit { + slot: FormulaSlot::Existing { id, was }, + .. + }, + ) => Some(Self::ReplaceFormula { + id: id.clone(), + was: was.clone(), + }), ("formula", _) => Some(Self::Formula), ("category", _) => Some(Self::Category), ("item", AppMode::ItemAdd { category, .. }) => Some(Self::Item { @@ -1355,7 +1386,7 @@ impl CommitKind { fn buffer(&self) -> &'static str { match self { - Self::Formula => "formula", + Self::Formula | Self::ReplaceFormula { .. } => "formula", Self::Category => "category", Self::Item { .. } => "item", } @@ -1363,14 +1394,14 @@ impl CommitKind { fn on_empty(&self) -> AppMode { match self { - Self::Formula => AppMode::FormulaPanel, + Self::Formula | Self::ReplaceFormula { .. } => AppMode::FormulaPanel, Self::Category | Self::Item { .. } => AppMode::CategoryPanel, } } fn on_commit(&self) -> Option { match self { - Self::Formula => Some(AppMode::FormulaPanel), + Self::Formula | Self::ReplaceFormula { .. } => Some(AppMode::FormulaPanel), Self::Category | Self::Item { .. } => None, } } @@ -1378,6 +1409,7 @@ impl CommitKind { fn status(&self, name: &str) -> String { match self { Self::Formula => "Formula added".to_string(), + Self::ReplaceFormula { .. } => "Formula updated".to_string(), Self::Category => format!("Added category \"{name}\""), Self::Item { .. } => format!("Added \"{name}\""), } @@ -1389,6 +1421,11 @@ impl CommitKind { raw: name.to_string(), target_category: "_Measure".to_string(), }, + Self::ReplaceFormula { id, was } => ModelCommand::ReplaceFormula { + id: id.clone(), + was: was.clone(), + raw: name.to_string(), + }, Self::Category => ModelCommand::AddCategory(name.to_string()), Self::Item { category } => ModelCommand::AddItem { category: category.clone(), @@ -1944,11 +1981,12 @@ mod tests { ); } - /// The formula panel binds no `Enter`, so the rule yields navigation and - /// nothing else. A mode change here would be an invented activation. + /// The formula panel's `Enter` edits (improvise-km71), so a click opens the + /// editor on the row CLICKED — `Margin`, where the cursor is not. #[test] - fn a_formula_row_click_navigates_and_activates_nothing() { + fn a_formula_row_click_navigates_and_opens_the_editor_on_that_row() { let state = with_panels(); + let margin = state.formulas.formulas[1].clone(); let commands = click(&state, Click::FormulaRow(1)); assert_eq!( commands, @@ -1962,28 +2000,30 @@ mod tests { panel: Panel::Formula, cursor: 1 }, + Command::ChangeMode(AppMode::formula_replace(margin.id, margin.raw.clone())), + Command::SetBuffer { + name: "formula".into(), + value: margin.raw, + }, ] ); } - /// The asymmetry is a fact about the keymap, so it is asserted against - /// the keymap — not against a reading of it. See `context/code/clients.md`. + /// A click mirrors `Enter`, so the tie is asserted against what `Enter` + /// resolves to, not against a reading of it. See `context/code/clients.md`. #[test] - fn the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does() { - let keymaps = Keymaps::shared(); + fn a_formula_row_click_ends_with_what_enter_does_on_that_row() { + let mut state = with_panels(); + state.view.mode = AppMode::FormulaPanel; + state.view.formula_cursor = 1; + let KeyOutcome::Local(enter) = resolve(&state, ev(KeyCode::Enter)) else { + panic!("Enter in the formula panel resolves here"); + }; + assert!(!enter.is_empty(), "Enter on a formula must do something"); + let clicked = click(&with_panels(), Click::FormulaRow(1)); assert!( - keymaps - .lookup(&ModeKey::FormulaPanel, KeyCode::Enter, KeyModifiers::NONE) - .is_none(), - "if FormulaPanel gains an Enter binding, click_formula_row must \ - activate too — the click mirrors Enter, and this is the only \ - thing tying the two together" - ); - assert!( - keymaps - .lookup(&ModeKey::ViewPanel, KeyCode::Enter, KeyModifiers::NONE) - .is_some(), - "and the view panel's click activates because its Enter does" + clicked.ends_with(&enter), + "the click's activation must be Enter's: {clicked:?} vs {enter:?}" ); } @@ -2195,7 +2235,7 @@ mod tests { "Net = Profit", ] .into_iter() - .map(str::to_string) + .map(crate::FormulaEntry::from) .collect(); state.views.views = ["Default", "Second", "Third"] .into_iter() diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index e02f7cf9..21ea8364 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -134,8 +134,8 @@ impl Client { self.clicked(Click::ViewRow(index)) } - /// The user clicked row `index` of the formula panel. Navigation only — - /// that panel binds no `Enter` (improvise-iioi). + /// The user clicked row `index` of the formula panel: focus it, move there, + /// then open the editor on that formula, as `Enter` does (improvise-km71). #[wasm_bindgen] pub fn click_formula(&mut self, index: usize) -> Result, JsError> { self.clicked(Click::FormulaRow(index)) diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index e9c420da..65119556 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -377,8 +377,8 @@ fn formula_rows(state: &ClientState) -> Option> { .formulas .iter() .enumerate() - .map(|(i, text)| RenderFormulaRow { - text: text.clone(), + .map(|(i, entry)| RenderFormulaRow { + text: entry.raw.clone(), selected: i == state.view.formula_cursor, }) .collect() diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 3ef54548..ae9ca177 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -2334,3 +2334,96 @@ fn every_projected_edit_text_commits_back_to_its_value() { } } } + +const FORMULAS: [&str; 3] = [ + "Profit = Revenue * 2", + "Tax = Revenue * 0.1", + "Margin = Profit / Revenue", +]; + +fn add_formulas(app: &mut App) { + for raw in FORMULAS { + let f = improvise_command::formula::parse_formula(raw, "_Measure").unwrap(); + app.model_state.workbook.model.add_formula(f); + } + app.rebuild_layout(); +} + +fn raws(app: &App) -> Vec { + let formulas = app.model_state.workbook.model.formulas(); + formulas.iter().map(|f| f.raw.clone()).collect() +} + +/// The formula panel's edit (improvise-km71), opened at a NONZERO cursor over +/// three formulas, typed into and committed, with both ends agreeing throughout. +#[test] +fn editing_a_formula_from_the_panel_agrees_with_the_host() { + let mut app = furnished_host(); + add_formulas(&mut app); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + let mut twin = twin_of(&app); + add_formulas(&mut twin); + let mut client = furnished_client(&app, (2, 1)); + client.formulas = improvise_command::ui::project::formulas_of(&app); + + both_ends(&mut app, &mut twin, &mut client, "Enter"); + assert_eq!( + client.view.buffers.get("formula"), + app.view_state.buffers.get("formula"), + "the two ends seeded the editor differently" + ); + assert_eq!( + app.view_state.buffers.get("formula").map(String::as_str), + Some(FORMULAS[1]), + "Enter must seed the formula under cursor 1" + ); + for key in ["Backspace", "2", "Enter"] { + both_ends(&mut app, &mut twin, &mut client, key); + } + assert_eq!( + raws(&app), + vec![FORMULAS[0], "Tax = Revenue * 0.2", FORMULAS[2]], + "the commit must replace the formula it opened on, in place" + ); +} + +/// A rename through the browser: the one input that separates "replace" from +/// "add", since `add_formula` upserts by target. The status diverges on purpose. +#[test] +fn renaming_a_formula_from_the_panel_lands_the_same_document_as_the_host() { + let mut app = furnished_host(); + add_formulas(&mut app); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + let mut twin = twin_of(&app); + add_formulas(&mut twin); + let mut client = furnished_client(&app, (2, 1)); + client.formulas = improvise_command::ui::project::formulas_of(&app); + both_ends(&mut app, &mut twin, &mut client, "Enter"); + for view in [&mut app.view_state, &mut twin.view_state, &mut client.view] { + view.buffers + .insert("formula".into(), "Levy = Revenue * 0.1".into()); + } + + let event = BrowserKey::new("Enter").to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("Enter in the formula editor resolves here"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut twin, &cmds); + reduce_key(&mut app, event); + + assert_eq!(document_text(&twin), document_text(&app)); + assert_eq!( + raws(&app), + vec![FORMULAS[0], "Levy = Revenue * 0.1", FORMULAS[2]] + ); + assert_eq!(client.view.mode, app.view_state.mode); + assert_eq!(client.view.status_msg, "Formula updated"); + assert_eq!(app.view_state.status_msg, "Renamed Tax to Levy"); +} diff --git a/harness/opfs-contract/Cargo.lock b/harness/opfs-contract/Cargo.lock index 385b3e29..579594c6 100644 --- a/harness/opfs-contract/Cargo.lock +++ b/harness/opfs-contract/Cargo.lock @@ -196,6 +196,7 @@ dependencies = [ "anyhow", "pest", "pest_derive", + "serde", ] [[package]] diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index 6e23d4ed..34f7fbec 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -251,15 +251,16 @@ test.describe("a pointer event reaches the client and its answer is painted", () await expect(page.locator("#views li.active")).toContainText("Season"); await expect(page.locator("#views li.selected")).toContainText("Season"); - // A formula row: navigation only, because FormulaPanel binds no Enter and - // a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md. - // `#grid input` toHaveCount(0) belongs here and is deliberately absent: - // it cannot fail, measured by injection (improvise-qf57, clients.md). + // A formula row: FormulaPanel's Enter edits the formula (improvise-km71) + // and a click mirrors Enter, so the click opens the editor on that row. + // `#grid input` toHaveCount(0) is deliberately absent (improvise-qf57). await page.keyboard.press("Shift+F"); await expect(page.locator("#formulas li").first()).toBeVisible(); await page.keyboard.press("Escape"); + const formula = await page.locator("#formulas li").first().innerText(); await page.locator("#formulas li").first().click(); - await expect(page.locator("#mode")).toHaveText("FORMULAS"); + await expect(page.locator("#mode")).toHaveText("FORMULA"); + await expect(page.locator("#minibuffer")).toContainText(formula.trim()); await expect(page.locator("#formulas li.selected")).toHaveCount(1); }); diff --git a/src/ui/formula_panel.rs b/src/ui/formula_panel.rs index 247eaa0e..e83f07f9 100644 --- a/src/ui/formula_panel.rs +++ b/src/ui/formula_panel.rs @@ -30,7 +30,7 @@ impl PanelContent for FormulaContent<'_> { } fn title(&self) -> &str { - " Formulas [n]ew [d]elete " + " Formulas [n]ew [e]dit [d]elete " } fn item_count(&self) -> usize { @@ -42,7 +42,7 @@ impl PanelContent for FormulaContent<'_> { } fn render_item(&self, index: usize, is_selected: bool, inner: Rect, buf: &mut Buffer) { - let raw = &self.formulas.formulas[index]; + let raw = &self.formulas.formulas[index].raw; let style = if is_selected { Style::default() .fg(Color::Black) @@ -79,6 +79,8 @@ impl PanelContent for FormulaContent<'_> { #[cfg(test)] mod tests { use super::*; + use crate::formula::parse_formula; + use crate::protocol::FormulaEntry; use crate::ui::app::App; use crate::ui::panel::Panel; use crate::workbook::Workbook; @@ -101,7 +103,10 @@ mod tests { /// improvise-8l0c — `context/test-evidence/tui.md`. fn render_formulas(raws: &[&str], width: u16) -> Buffer { let cache = FormulaCache { - formulas: raws.iter().map(|s| (*s).to_string()).collect(), + formulas: raws + .iter() + .map(|s| FormulaEntry::from(&parse_formula(s, "_Measure").unwrap())) + .collect(), }; let area = Rect::new(0, 0, width, 5); let mut buf = Buffer::empty(area); @@ -151,7 +156,12 @@ mod tests { .map(|f| f.raw.clone()) .collect(); assert_eq!(was.len(), 2, "fixture must carry formulas"); - assert_eq!(was, project::formulas_of(&app).formulas); + let cached: Vec = project::formulas_of(&app) + .formulas + .into_iter() + .map(|f| f.raw) + .collect(); + assert_eq!(was, cached); } /// Pins the drawn rows, so a change to how the cache is read shows up.