Merge pull request #178 from fiddlerwoaroof/fix/dsxq-yank-value-proj

Browser yy yanks the projected value, not the edit text re-parsed (improvise-dsxq)

context/code/protocol.md

@@ -552,8 +552,39 @@ client over the same grid, and they disagreed by a comma.
 
 `numeric` and `error` are decided by the server for the same reason: it is
 the side that knows which it was, and re-deriving client-side is how the two
-ends drift apart. `CellDisplay` is deliberately not a `CellValue` — by the
-time a cell reaches a client the number formatting has already happened.
+ends drift apart. `CellDisplay` is deliberately not *drawn from* a `CellValue`
+— by the time a cell reaches a client the number formatting has already
+happened, and `text` is what a renderer paints.
+
+### `CellDisplay::value` — what `yank` copies (improvise-dsxq)
+
+The value the host evaluated at this position, the same `Option<CellValue>`
+that `text` was formatted from: `evaluate_aggregated` over the view's
+`none_cats` in a pivot grid, `records_value` in a records grid. It exists
+because the browser's `yy` has to copy what the TUI's `YankCell` copies, and
+neither string on the cell can be turned back into it:
+
+- `edit_text` is `CellValue::to_string()`, which writes a non-whole number
+  `{:.4}` — `1.23456789` becomes `1.2346`.
+- Re-parsing any text with `typed()` turns number-like text into a number —
+  `Text("007")` becomes `Number(7.0)`.
+- In a pivot grid `edit_text` is the *stored* cell (`get_cell`), so an
+  aggregate or a formula cell has none and yanked nothing at all.
+
+So the value is **projected**, not re-derived (gotcha 47): a client that
+wants the value asks for the value. It is kept separate from `edit_text`
+rather than replacing it, because the two answer different questions — what
+an editor starts with is a *string*, deliberately the one the TUI's buffer
+holds, and changing it would change what the TUI's own editor shows.
+
+It cost `Eq`: `CellValue` holds an `f64`, so `CellDisplay`, `GridCache` and
+`GridUpdate` are `PartialEq` only. Nothing hashed or `Eq`-bounded them.
+
+In a records grid the TUI does **not** yank this value: `YankCell` and
+`PasteCell` resolve the synthetic `(_Index, _Dim)` key, so the TUI yanks
+`None` and pastes a junk cell into the document (improvise-6p8q). The browser
+is right there and the TUI is wrong, which is why the lockstep yank test stays
+on a pivot grid.
 
 ### `AxisFrame` — the grid's frame, and why it is *one* type
 

context/test-evidence/clients.md

@@ -4148,6 +4148,12 @@ so the paste is evidence about the yank rather than about a hand-seeded field.
 
 **Fault injected:** the `"yank"` arm of `composed` renamed. Fails on the yank.
 
+**Superseded in part by improvise-dsxq** (entry at the end of this file). The
+fixture's `1200` is the one kind of value on which the edit text and the value
+agree, so this test could not see that the arm yanked the edit text re-parsed.
+The arm now yanks `CellDisplay::value`, and the test sets that field on its
+hand-built cell. The parity claim lives in the lockstep tests, not here.
+
 ### `a_second_key_that_matches_nothing_still_spends_the_prefix`
 
 `App::dispatch_key` `take()`s the prefix before looking the second key up, so
@@ -4261,3 +4267,62 @@ with no `SetBuffer`, then presses the keys: Enter must compose
 - 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`.
+
+## improvise-dsxq — the browser's `yy` copies the value, not a string
+
+PR #161 (improvise-lut7) made the browser's `yank` arm compose
+`SetYanked(typed(edit_text))`. The TUI's `YankCell` yanks
+`evaluate_aggregated(key)`, the value itself. Those differ in three ways, and
+#161's only fixture, `1200`, hits none of them:
+
+- `edit_text` is `CellValue::to_string()`, which rounds a non-whole number to
+  four places.
+- `typed()` turns number-like text into a number.
+- In a pivot grid `edit_text` is the stored cell only, so an aggregate or a
+  formula cell has none and the browser yanked `None`.
+
+The fix projects the evaluated value as `CellDisplay::value`, and the arm yanks
+that (`context/code/protocol.md` §3). Reproduced at `40b8913` by both tests
+below before the fix: the browser yanked `Number(1.2346)` for `1.23456789`, and
+`None` for the aggregate.
+
+### `yy_then_p_copies_the_value_the_host_copies`
+
+Three cases, one per disagreement, each on a real `App` and a `ClientState`
+seeded by `grid_of`. The host presses `y y p` through `reduce_key`; a twin
+host applies exactly what the client composed. Each case asserts the host
+yanked the expected value (so a fixture that yanks nothing cannot pass), that
+the client yanked the same, that the host's paste landed, and that the two
+documents match. Sources sit at row 2 or 3 and targets at row 1, never row 0,
+and the first column is an unused `Units` measure, so no cursor is at index 0.
+
+- `1.23456789`, a non-whole number.
+- `Text("007")`, number-like text.
+- `Profit = Revenue - Cost` over `10.123456` and `0.5`, a formula's value,
+  pasted into a stored `Revenue` cell.
+
+**Faults injected, each run:**
+- The pre-fix arm, `typed(edit_text)`. Fails on the first case with
+  `Number(1.2346)`.
+- The arm yanking `typed(&value.to_string())`, the projected value re-rendered.
+  Fails on the first case with `Number(1.2346)`.
+- The producer coercing number-like `Text` through `typed()`. The first case
+  passes, and the second fails with `Number(7.0)`.
+- The producer projecting the raw `get_cell` instead of the evaluated value.
+  The first two cases pass, and the third fails with `None`.
+
+### `yy_over_an_aggregate_copies_the_sum_the_host_copies`
+
+`Product` on the None axis with two stored records, `1.23456789` and `1.0`, so
+the cell at row 1 is an `Aggregate` with no stored value. Yank only, since an
+all-aggregate grid has no record cell to paste into. The host must yank
+`Number(2.23456789)`, which rules out a fixture that is not an aggregate.
+
+**Faults injected, each run:** the pre-fix arm (fails with `None`), the
+re-rendered value (fails with `Number(2.2346)`), and the raw `get_cell`
+producer (fails with `None`).
+
+Records mode is deliberately not covered. There the TUI yanks `None` and its
+`p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key
+(improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep
+case there would assert the bug.

crates/improvise-command/src/ui/project.rs

@@ -282,6 +282,7 @@ fn cell_display(
             numeric: matches!(value, Some(CellValue::Number(_))),
             error: matches!(value, Some(CellValue::Error(_))),
             id: cell_id(app, layout, row, col),
+            value: value.clone(),
         };
     }
     let key = layout.cell_key(row, col);
@@ -293,6 +294,7 @@ fn cell_display(
         // The *unformatted* value the TUI seeds its edit buffer from. Projecting it
         // beats a client un-formatting `text` (gotcha 47).
         edit_text: key.map(|key| app.display_value_in(layout, &key)),
+        value: value.clone(),
     }
 }
 

crates/improvise-protocol/src/render.rs

@@ -4,6 +4,7 @@
 use serde::{Deserialize, Serialize};
 use unicode_width::UnicodeWidthStr;
 
+use improvise_core::model::cell::CellValue;
 use improvise_core::model::record::{AggregateHash, RecordHash};
 use improvise_core::view::Axis;
 use improvise_core::view::layout::PageAxis;
@@ -64,9 +65,9 @@ pub enum CreateKind {
     Category,
 }
 
-/// One cell as the client draws it. Deliberately not a `CellValue` — the
+/// One cell as the client draws it. Drawn from `text`, never from `value` — the
 /// formatting has already happened, and re-deriving it is how ends drift.
-#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
 pub struct CellDisplay {
     pub text: String,
     /// Right-align numerics, left-align text — decided by the server, which
@@ -80,6 +81,9 @@ pub struct CellDisplay {
     /// What an editor should start with — the value as the user last
     /// **typed** it, not as it is drawn. The difference corrupts data (§3).
     pub edit_text: Option<String>,
+    /// The value the host evaluated here, unrounded and untyped-by-text: what
+    /// `yank` copies. Neither `text` nor `edit_text` can be re-parsed into it (§3).
+    pub value: Option<CellValue>,
 }
 
 impl CellDisplay {
@@ -98,6 +102,7 @@ impl CellDisplay {
             numeric: true,
             error: false,
             id: None,
+            value: None,
         }
     }
 
@@ -110,6 +115,7 @@ impl CellDisplay {
             error: true,
             id: None,
             edit_text: None,
+            value: None,
         }
     }
 
@@ -416,7 +422,7 @@ fn shows(items: &[&Vec<String>], idx: usize, first_rendered: usize, level: usize
 
 /// The grid widget's copy of the visible cells (improvise-n10), indexed by
 /// position within the **viewport** and never by `CellKey`.
-#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
 pub struct GridCache {
     /// The row axis's frame: levels, label tuples and groups. **Not** a list
     /// of strings — see [`AxisFrame`].
@@ -563,7 +569,7 @@ enum LabelAxis {
 
 /// What can change about the grid: cell values, the frames that name them,
 /// the widths, the totals, the page bar and the extent.
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 pub enum GridUpdate {
     /// Display values for cells, by viewport position.
     Cells(Vec<((usize, usize), CellDisplay)>),

crates/improvise-protocol/src/resolve.rs

@@ -530,12 +530,10 @@ fn composed(
             state,
             ModelCommand::ClearCell(record_cell_at_cursor(state)?),
         )?]),
-        // `YankCell::execute`: the value, then the status. `edit_text` is
-        // what the host evaluated, which is what the TUI yanks.
+        // `YankCell::execute`: the value, then the status. The projected
+        // value, never text re-parsed: improvise-dsxq.
         "yank" => Some(vec![
-            Command::SetYanked(typed(
-                cell_at_cursor(state)?.edit_text.as_deref().unwrap_or(""),
-            )),
+            Command::SetYanked(cell_at_cursor(state)?.value.clone()),
             Command::SetStatus("Yanked".to_string()),
         ]),
         "paste" => Some(match state.view.yanked.clone() {
@@ -3198,6 +3196,7 @@ mod tests {
     #[test]
     fn yy_yanks_the_cell_under_the_cursor_and_p_pastes_what_it_yanked() {
         let mut state = client_on(record_cell());
+        state.grid.cells[0][0].value = Some(CellValue::Number(1200.0));
         arm(&mut state, ev(KeyCode::Char('y')));
         let KeyOutcome::Local(yank) = resolve(&state, ev(KeyCode::Char('y'))) else {
             panic!("`yy` is bound to yank");

crates/improvise-wasm-client/tests/lockstep.rs

@@ -1581,3 +1581,177 @@ fn opens_an_editor(client: &ClientState, key: KeyEvent) -> bool {
         _ => false,
     }
 }
+
+// Yank and paste (improvise-dsxq).
+
+fn region_measure(region: &str, measure: &str) -> improvise_command::model::cell::CellKey {
+    improvise_command::model::cell::CellKey::new(vec![
+        ("Region".to_string(), region.to_string()),
+        ("_Measure".to_string(), measure.to_string()),
+    ])
+}
+
+/// Four rows and four measures, the last a formula, holding values on which the
+/// edit text and the value disagree. Evidence file.
+fn yank_host() -> App {
+    use improvise_protocol::CellValue::{Number, Text};
+    let mut wb = Workbook::new("Test");
+    wb.add_category("Region").unwrap();
+    for item in ["West", "North", "South", "East"] {
+        wb.add_item("Region", item).unwrap();
+    }
+    for item in ["Units", "Revenue", "Cost"] {
+        wb.add_item("_Measure", item).unwrap();
+    }
+    wb.model.add_formula(
+        improvise_command::formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(),
+    );
+    wb.active_view_mut().set_axis("Region", Axis::Row);
+    wb.active_view_mut().set_axis("_Measure", Axis::Column);
+    for (region, measure, value) in [
+        ("North", "Revenue", Number(1.0)),
+        ("North", "Cost", Number(2.0)),
+        ("South", "Revenue", Number(1.23456789)),
+        ("South", "Cost", Text("007".into())),
+        ("East", "Revenue", Number(10.123456)),
+        ("East", "Cost", Number(0.5)),
+    ] {
+        wb.model.set_cell(region_measure(region, measure), value);
+    }
+    let mut app = App::new(wb, None);
+    app.term_height = 4 + 8;
+    app.rebuild_layout();
+    app
+}
+
+/// One unmodified character on both ends: the host through its keymap, a twin
+/// through exactly what the client composed.
+fn press(app: &mut App, twin: &mut App, client: &mut ClientState, key: char) {
+    let event = KeyEvent::new(KeyCode::Char(key), KeyModifiers::NONE);
+    let KeyOutcome::Local(cmds) = resolve(client, event) else {
+        panic!("{key}: the client must answer this key locally");
+    };
+    for cmd in &cmds {
+        client.fold(cmd);
+    }
+    apply_on(twin, &cmds);
+    reduce_key(app, event);
+}
+
+fn seat(app: &mut App, client: &mut ClientState, at: (usize, usize)) {
+    app.cursor_mut().selected = at;
+    client.view.cursor_mut().selected = at;
+}
+
+/// `yy` then `p` copies the value the TUI copies, not its edit text re-parsed.
+/// Never an integer: the one kind on which the two agree. Evidence file.
+#[test]
+fn yy_then_p_copies_the_value_the_host_copies() {
+    use improvise_protocol::CellValue::{Number, Text};
+    let cases = [
+        (
+            "a non-whole number",
+            (2, 1),
+            (1, 1),
+            "Revenue",
+            Number(1.23456789),
+        ),
+        (
+            "number-like text",
+            (2, 2),
+            (1, 2),
+            "Cost",
+            Text("007".into()),
+        ),
+        (
+            "a formula's value",
+            (3, 3),
+            (1, 1),
+            "Revenue",
+            Number(10.123456 - 0.5),
+        ),
+    ];
+    for (what, from, to, measure, expected) in cases {
+        let mut app = yank_host();
+        let mut twin = yank_host();
+        let mut client = client_ready_to_edit(&app, (4, 4));
+
+        seat(&mut app, &mut client, from);
+        press(&mut app, &mut twin, &mut client, 'y');
+        press(&mut app, &mut twin, &mut client, 'y');
+        assert_eq!(
+            app.view_state.yanked,
+            Some(expected.clone()),
+            "{what}: the host must yank the value itself, or the fixture proves nothing"
+        );
+        assert_eq!(
+            client.view.yanked, app.view_state.yanked,
+            "{what}: client and host yanked different values"
+        );
+
+        seat(&mut app, &mut client, to);
+        press(&mut app, &mut twin, &mut client, 'p');
+        assert_eq!(
+            app.model_state
+                .workbook
+                .model
+                .get_cell(&region_measure("North", measure)),
+            Some(&expected),
+            "{what}: the host's paste must have landed"
+        );
+        assert_eq!(
+            document_text(&twin),
+            document_text(&app),
+            "{what}: the client's paste wrote a different document"
+        );
+    }
+}
+
+/// A true aggregate: `Product` on no axis, so the cell sums two records and has
+/// no stored value. Yank only: an all-aggregate grid has nowhere to paste.
+#[test]
+fn yy_over_an_aggregate_copies_the_sum_the_host_copies() {
+    let mut wb = Workbook::new("Test");
+    wb.add_category("Region").unwrap();
+    wb.add_item("Region", "North").unwrap();
+    wb.add_item("Region", "South").unwrap();
+    wb.add_category("Product").unwrap();
+    wb.add_item("Product", "Widget").unwrap();
+    wb.add_item("Product", "Gadget").unwrap();
+    wb.add_item("_Measure", "Revenue").unwrap();
+    wb.active_view_mut().set_axis("Region", Axis::Row);
+    wb.active_view_mut().set_axis("_Measure", Axis::Column);
+    wb.active_view_mut().set_axis("Product", Axis::None);
+    for (product, value) in [("Widget", 1.23456789), ("Gadget", 1.0)] {
+        wb.model.set_cell(
+            region_measure("South", "Revenue").with("Product", product),
+            improvise_protocol::CellValue::Number(value),
+        );
+    }
+    let mut app = App::new(wb, None);
+    app.term_height = 2 + 8;
+    app.rebuild_layout();
+    let mut client = client_of(&app, (2, 1));
+
+    app.cursor_mut().selected = (1, 0);
+    client.view.cursor_mut().selected = (1, 0);
+    for _ in 0..2 {
+        let event = KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE);
+        let KeyOutcome::Local(cmds) = resolve(&client, event) else {
+            panic!("y must resolve locally over an aggregate too");
+        };
+        for cmd in &cmds {
+            client.fold(cmd);
+        }
+        reduce_key(&mut app, event);
+    }
+    assert_eq!(
+        app.view_state.yanked,
+        Some(improvise_protocol::CellValue::Number(1.23456789 + 1.0)),
+        "the host yanks the sum, or the fixture is not an aggregate"
+    );
+    assert_eq!(
+        client.view.yanked, app.view_state.yanked,
+        "client and host yanked different values over an aggregate"
+    );
+}