diff --git a/context/code/protocol.md b/context/code/protocol.md index 7b797818..fdbf9e8e 100644 --- a/context/code/protocol.md +++ b/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` +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 diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e60db1ab..e73939f4 100644 --- a/context/test-evidence/clients.md +++ b/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. diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 7d33657b..441af17e 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/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(), } } diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index dbcad0d1..57f0d0c5 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/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, + /// 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, } 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], 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)>), diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index e01a44ee..3a86f346 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/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"); diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index db9abcef..a3e0115c 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/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(®ion_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" + ); +}