Merge pull request #169 from fiddlerwoaroof/fix/ktlk-editor-one-home

The editor overlay and Enter read one buffer (improvise-ktlk)

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`.

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) {

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

crates/improvise-protocol/src/resolve.rs

@@ -955,11 +955,10 @@ fn record_at_cursor(state: &ClientState) -> Option<crate::RecordHash> {
     }
 }
 
-/// 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<String> {
+    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]

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,