diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e130266a..e60db1ab 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4087,17 +4087,18 @@ Demonstrated at `e7de19d`: the first test below reports `Some("")`. ### `an_editing_mode_with_no_edit_buffer_shows_the_cell_rather_than_a_blank` -The fallback is `improvise_protocol::edit_seed`, which is the **existing** home -of "what an editor opened here starts with" — `seed_at` at the cursor, the -function `enter-advance` already seeds from. A second reconciliation of the -scroll offset inside `render.rs` would have been gotcha 42's own trap, so the -one-line fallback cost one `pub` rather than a copy. +**Superseded in part by improvise-ktlk** (entry at the end of this file). As +first landed, the fallback lived in `render.rs`: `editing` fell back to +`edit_seed` while `commit` and `append-char` still read the absent buffer as +`""`, so the overlay showed `1200` and Enter cleared it. The fallback now lives +in the fold (`ClientState::settle_editor`), which seeds the buffer itself, and +`render.rs` reads the same buffer `commit` does. The test's first assertion +changed with it: it used to check that the buffer was still absent after the +fold, and now checks that the fold seeded it. -The test asserts three things in order, and the middle one is why it is not -vacuous: that the buffer really is absent, that the cell really holds `1200`, -and only then what `editing` says. Without the first two a fixture that happened -to carry an `edit` buffer, or a grid whose cell was blank anyway, would pass -against the bug. +The test asserts three things in order: what the fold put in the buffer, that +the cell really holds `1200`, and only then what `editing` says. Without the +second, a grid whose cell was blank anyway would pass against the bug. **Fault injected:** `unwrap_or_default()`, the original expression. Fails with `Some("")`. @@ -4223,3 +4224,40 @@ and converged. **Fault injected:** the same `Binding::Prefix` → `Upstream` reversion. Fails on the second press. + +## improvise-ktlk — the overlay and the commit read one buffer + +PR #162 (improvise-xg1z) changed what the overlay **draws** for an `Editing` +mode that arrived without its `edit` buffer, and not what Enter **commits**. +`render.rs` painted `edit_seed(state)`, while `resolve.rs::commit` and +`append-char` read `read_buffer(&state.view, "edit")`, which is `""` for an +absent key. So the overlay showed the value, Enter composed `ClearCell`, and one +typed character replaced the shown text rather than extending it. #162's tests +only looked at the drawing, and none of them pressed a key. + +The fix gives the fact one home. The fold seeds the missing buffer +(`ClientState::settle_editor`, called after every fold, bootstrap and refusal, +beside `settle_mode`), and `render.rs` reads the buffer through `read_buffer`, +as `commit` does. `edit_seed` is `pub(crate)` again and no renderer calls it. +Seeding happens only when there is a cell under the cursor that has +`edit_text`. With no such cell the buffer stays absent, which reads as `""` +on both paths, and `commit` declines anyway for want of a cell. + +### `an_editor_entered_without_its_buffer_commits_and_extends_the_cell` + +The fixture's display text (`1,200.50`) and edit text (`1200.5`) **differ**. +With `CellDisplay::number("1200")`, as #162 used, a fallback that seeds from the +formatted `text` passes too (gotcha 8). The test folds `ChangeMode(editing)` +with no `SetBuffer`, then presses the keys: Enter must compose +`SetCell(Number(1200.5))`, and after a typed `5`, Enter must compose +`SetCell(Number(1200.55))`. + +**Faults injected, each run:** +- The tree before this fix (render-only fallback). The buffer assertion fails + with `""`. With that assertion removed, the Enter assertion fails on its own + with `None`, because the first command is `ClearCell`. +- `edit_seed` returning the drawn `text` instead of `edit_text`. Fails with + `"1,200.50"`. +- The over-broad seed, "absent **or empty**, refill". This test passes, and + `an_edit_buffer_the_user_has_emptied_is_not_refilled_from_the_cell` fails, + as it did against the same rival in `render.rs`. diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index 8010e920..599f21e4 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -74,6 +74,7 @@ impl ClientState { self.apply_projection(projection); } self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -85,6 +86,7 @@ impl ClientState { other => reduce_view(&mut self.view, other), } self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -124,6 +126,7 @@ impl ClientState { } self.last_rejection = Some(rejection); self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -158,6 +161,21 @@ impl ClientState { self.view.mode = settled_mode(&self.view.mode, self.grid.records); } + /// 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) { + let absent = self + .view + .mode + .minibuffer() + .filter(|_| self.view.mode.is_editing()) + .map(|mb| mb.buffer_key.clone()) + .filter(|key| !self.view.buffers.contains_key(key)); + if let (Some(key), Some(seed)) = (absent, crate::resolve::edit_seed(self)) { + self.view.buffers.insert(key, seed); + } + } + /// 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) { diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 53544924..8abc5ec4 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -34,7 +34,7 @@ pub use render::{ WizardProposal, WizardStepKind, header_level_widths, }; pub use resolve::{ - Click, KeyOutcome, Request, click, edit_seed, read_buffer, resolve, typed, typed_cell_command, + Click, KeyOutcome, Request, click, read_buffer, resolve, typed, typed_cell_command, }; pub use viewmodel::{CatTreeEntry, flatten_cat_tree, view_axis_summary}; // `improvise-core`'s, because the index that resolves one lives in diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index be549bf3..e01a44ee 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -955,11 +955,10 @@ fn record_at_cursor(state: &ClientState) -> Option { } } -/// What an editor opened on the cursor starts with. The one home, so a -/// renderer does not reconcile the scroll offset again (gotcha 42). -pub fn edit_seed(state: &ClientState) -> String { - let (row, col) = state.view.cursor().selected; - seed_at(state, row, col) +/// 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. @@ -3028,6 +3027,55 @@ mod tests { )); } + /// improvise-ktlk: what an editor shows and what Enter commits are one + /// fact. See `context/test-evidence/clients.md`. + #[test] + fn an_editor_entered_without_its_buffer_commits_and_extends_the_cell() { + let mut client = client_on(record_cell()); + let mut cell = CellDisplay::number("1,200.50"); + cell.edit_text = Some("1200.5".into()); + cell.id = Some(record_cell()); + client.fold(&Command::Project(Projection::Grid(GridUpdate::Cells( + vec![((0, 0), cell)], + )))); + client.fold(&Command::ChangeMode(AppMode::editing())); + assert_eq!( + read_buffer(&client.view, "edit"), + "1200.5", + "the buffer is what the overlay paints and what a commit reads" + ); + + let committed = |client: &ClientState| { + let KeyOutcome::Local(commands) = resolve(client, ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + match &commands[0] { + Command::Model { + cmd: ModelCommand::SetCell { value, .. }, + .. + } => Some(value.clone()), + _ => None, + } + }; + assert_eq!( + committed(&client), + Some(CellValue::Number(1200.5)), + "Enter on an editor showing the cell must not clear it" + ); + + let KeyOutcome::Local(typed) = resolve(&client, ev(KeyCode::Char('5'))) else { + panic!("a character in Editing is append-char"); + }; + for command in &typed { + client.fold(command); + } + assert_eq!( + committed(&client), + Some(CellValue::Number(1200.55)), + "a typed character extends what the editor showed, not replaces it" + ); + } + /// At the bottom-right corner `EnterAdvance` returns `AbortChain`, so the /// commit leaves the user in normal mode. See the evidence file. #[test] diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index b73785d0..8025d36b 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -294,16 +294,13 @@ pub fn render_model(state: &ClientState) -> RenderModel { help: help_rows(&state.view), help_page: state.view.help_page, help_titles: help_titles(&state.view), - // An ABSENT buffer is not an empty one: blanking the overlay over a - // real value is one Enter from clearing it (improvise-xg1z). - editing: state.view.mode.is_editing().then(|| { - state - .view - .buffers - .get("edit") - .cloned() - .unwrap_or_else(|| improvise_protocol::edit_seed(state)) - }), + // The buffer Enter commits, never a second derivation of it: the fold + // seeds an absent one (improvise-xg1z, improvise-ktlk). + editing: state + .view + .mode + .is_editing() + .then(|| improvise_protocol::read_buffer(&state.view, "edit")), minibuffer: state.view.mode.minibuffer().map(|mb| RenderMinibuffer { prompt: mb.prompt.clone(), value: improvise_protocol::read_buffer(&state.view, &mb.buffer_key), @@ -912,10 +909,11 @@ mod tests { client .state_mut() .fold(&Command::ChangeMode(improvise_protocol::AppMode::editing())); - assert!( - !client.state().view.buffers.contains_key("edit"), - "the fixture must really be missing the buffer, or this proves \ - nothing about the fallback" + assert_eq!( + client.state().view.buffers.get("edit").map(String::as_str), + Some("1200"), + "the fold seeds the missing buffer, so the overlay and a commit \ + read one thing (improvise-ktlk)" ); assert_eq!( client.model().rows[0].cells()[0].text,