diff --git a/context/code/command.md b/context/code/command.md index e1bed0a7..ca241086 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3078,3 +3078,48 @@ and every entry, so *any* append reframes and throws the cursor to the origin although nothing was renumbered. Placing the cursor masks that at this call site. Distinguishing "rows appended" from "rows renumbered" is a larger question and does not belong in a P1 data-loss fix. + +## Paste lands on a record or nowhere (improvise-6p8q, improvise-l4mp) + +`YankCell` and `PasteCell` carry a `CellKey` like every cell command, and in +records mode the cursor's key is the **synthetic** `(_Index, _Dim)` pair — a +row index and a column index (gotcha 1). Both commands used it as though it +named a cell. `yy` evaluated it and got `None` on every records cell; `p` wrote +a *new* cell keyed `_Dim=|3|, _Index=|0|`, which `format_md` then persisted into +`## Data`. One keystroke corrupted the saved document, and the browser, which +composes its paste against the projected `CellId::Record`, never had the bug. + +**Yank reads what the grid draws.** A synthetic key is answered by +`GridLayout::records_value`, the one resolution of a records cell +(improvise-tom1), so a coordinate column yanks its item as text and a measure +column its stored value. That is also what the browser yanks, since +improvise-dsxq made it copy `CellDisplay::value`, which the projection fills +from the same call. One consequence is shared by both ends and is not decided +here: a blank — a creating column, or a measure the record lacks — yanks +`Text("")` where an empty pivot cell yanks `None`. Filtering it in the TUI alone +would be a second derivation, so it is left to the producer (improvise-s1lx). + +**Paste writes only where a value is stored.** A synthetic key resolves through +`GridLayout::record_cell_key`; the coordinate columns and both creating columns +store no value, so the paste is declined out loud rather than guessed at. A +coordinate column is deliberately *not* a rename: renaming moves every measure +cell in the row, a different intent from `SetCell`, and the browser does not +offer it either (`project::cell_id` answers `None` there). + +**Inside a drill the paste is declined, not staged.** Gotcha 19 asks the +records branch to follow the commit path, whose drill arm stages the edit — but +`pending_edits` holds *strings* and `CellValue`'s `Display` rounds a number to +four places, so staging a pasted `1.23456789` would commit `1.2346`. Writing +straight through would break the drill's atomicity. Declining is the only arm +that loses nothing; a typed staging map is the real fix if drill paste is +wanted. + +**An aggregate is a key that leaves out a regular category the layout +aggregates over** (improvise-l4mp). That is asked of the *key*, not of the view, +so `paste Region=West _Measure=Revenue Product=Gadget` from a script still +writes where the current view happens to aggregate `Product` away: a key that +names every coordinate is a record whatever the view shows. For the cursor's +key the two questions agree, because a cursor key never carries a None-axis +category. The **collapsed-group fold** half of improvise-l4mp is not covered: +its key carries the group's name as an ordinary coordinate, so no question about +the key alone can see it — it needs `GridLayout::folds_a_group`. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 85766267..3ce396f8 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4389,4 +4389,19 @@ 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. +case there would assert the bug. Since improvise-6p8q's fix it is covered, by +`yy_then_p_in_records_mode_writes_the_record_the_host_writes` below. + +### yy_then_p_in_records_mode_writes_the_record_the_host_writes + +improvise-6p8q in lockstep. The yank test in this file deliberately stayed on a +pivot grid because a records case would have asserted the TUI bug. Two records +(North, South), the yank from North and the paste onto South on row 1, the +`Revenue` column being neither the first column nor the last. Rivals, each run: +restoring the synthetic-key paste fails `both_ends`' document comparison on +`p` (the twin applied the client's `SetCell` to South's record, the host wrote +a `_Dim`/`_Index` cell); restoring the synthetic-key yank fails the yank +assertion. The tail yanks off the `Region` and `+` columns and requires the two +registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on +`+`, which is why that filter was removed rather than kept. The evidence entries +for the host-side tests are in `command.md`. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 511a8461..6d36571e 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5021,3 +5021,69 @@ bracket-matched `#[…]` before reading the name. `an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: a doc comment, two attributes (one with nested brackets), and a wrapped call. With the whitespace skip removed it fails with `[]`. + +### yy_then_p_in_records_mode_copies_between_real_records + +improvise-6p8q. At `gh/main` `9e78051` it failed at the yank: `left: None, +right: Some(Number(700.0))`, because `YankCell` evaluated the synthetic +`(_Index, _Dim)` key. With the yank alone repaired (rival: paste still writes +`SetCell(synthetic key)`), it fails at the paste assertion, and the document +carried `_Dim=|3|, _Index=|0| = …` in `## Data`. The test drives `yy`/`p` +through `App::handle_key` and then saves and reloads, because the persisted +line is the harm. The fixture is chosen so no two answers coincide: two records +by two measures with four distinct values, the yank from row 0 `Cost` and the +paste onto row 1 `Revenue`, so "the first row", "the first measure" and "the +value already there" are each wrong. The persistence assertion is scoped to the +`## Data` section: a view section legitimately writes `_Index: none`, and a +first version of this test failed on that. + +### yy_on_a_records_coordinate_column_yanks_the_item_as_text + +A coordinate column yanks its item as text (`West`, row 1): the yank reads +`GridLayout::records_value`, the same value the browser yanks off +`CellDisplay::value`. Rival: restoring the synthetic-key yank fails it with +`None`. + +### p_on_a_records_column_that_stores_no_value_changes_nothing + +`p` on `Region`, `+cat` and `+` at row 1 leaves the document byte-identical and +says why. At `9e78051` it failed on `Region` with `_Dim=|0|, _Index=|1| = 5.5` +appended to `## Data`. The control — the same paste onto `Revenue` writes — is +what keeps the declines from being "this command never writes". + +### p_inside_a_drill_neither_writes_nor_stages + +At `9e78051` a drilled paste wrote `_Dim=|3|, _Index=|0| = 5.5`. The rival that +resolves the real record but ignores the drill (writing straight through) fails +the document-unchanged assertion at the same line. Why declining rather than +staging: `context/code/command.md`, "Paste lands on a record or nowhere". + +### p_onto_a_none_axis_aggregate_writes_nothing + +improvise-l4mp, None-axis half. At `9e78051` `p` over the West/Revenue aggregate +(Product on None) wrote `Region=West, _Measure=Revenue = 5.5` — a partial key +that is not a record. The cursor is on row 1, not row 0. The control reruns the +same paste with `Product` paged instead of aggregated and requires the write, +so a paste that declines everywhere fails. Rival: removing the aggregate check +fails the first assertion. + +### paste_with_an_explicit_full_key_writes_even_where_the_view_aggregates + +Pins that "aggregate" is asked of the key, not of the view. It passes at +`9e78051` (there was no check at all); its job is the rival where the check +reads only whether the view has a regular category on None, which fails it — +that rival would silently turn every scripted `paste` with full coordinates +into a no-op on an aggregating view. + +### the_aggregate_decline_does_not_send_the_user_where_paste_also_declines + +improvise-48jd, from the #184 review. The aggregate decline said "drill in to +paste into a record", and inside the drill `p` answers `PASTE_INSIDE_DRILL`, so +the advice led to a second refusal. The test follows the advice literally: `p` +on the West aggregate, `>` into its drill, cursor onto the `Revenue` column, `p` +again. It asserts that the drilled paste really is the decline, so the setup +cannot pass by landing on "no cell at cursor". A first version of the test did +exactly that, because West had no record to drill into. It then asserts that +the first message does not mention drilling. At `0ed2e41` it failed with the +old text. The rival is restoring that text. Real drill paste, which means +staging a typed `CellValue`, is still open as improvise-48jd. diff --git a/crates/improvise-command/src/command/cmd/cell.rs b/crates/improvise-command/src/command/cmd/cell.rs index ef92935b..a68c30b8 100644 --- a/crates/improvise-command/src/command/cmd/cell.rs +++ b/crates/improvise-command/src/command/cmd/cell.rs @@ -1,5 +1,7 @@ use crate::model::Aggregation; +use crate::model::cell::{CellKey, CellValue}; use crate::ui::effect::{self, Effect, ViewTarget}; +use crate::view::synthetic_record_info; use super::core::{Cmd, CmdContext}; @@ -91,6 +93,276 @@ mod tests { assert!(effects.is_empty()); } + use crate::command::key::{KeyCode, KeyEvent, KeyModifiers}; + use crate::ui::app::App; + use crate::workbook::Workbook; + + fn press(app: &mut App, c: char) { + app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE)) + .unwrap(); + } + + fn record(region: &str, measure: &str) -> CellKey { + CellKey::new(vec![ + ("Region".into(), region.into()), + ("_Measure".into(), measure.into()), + ]) + } + + /// Two records by two measures, every value distinct, in records mode. + fn records_app() -> App { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("Region", "West").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + for (region, measure, n) in [ + ("East", "Revenue", 1200.0), + ("East", "Cost", 700.0), + ("West", "Revenue", 900.0), + ("West", "Cost", 400.0), + ] { + wb.model + .set_cell(record(region, measure), CellValue::Number(n)); + } + let mut app = App::new(wb, None); + press(&mut app, 'R'); + assert!(app.layout.is_records_mode(), "setup: in records mode"); + app + } + + fn col_of(app: &App, label: &str) -> usize { + (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == label) + .unwrap_or_else(|| panic!("no records column {label}")) + } + + fn synthetic_cells(wb: &Workbook) -> Vec { + wb.model + .data + .iter_cells() + .map(|(k, _)| k) + .filter(|k| k.get("_Index").is_some() || k.get("_Dim").is_some()) + .collect() + } + + #[test] + fn yy_then_p_in_records_mode_copies_between_real_records() { + let mut app = records_app(); + let (east, west) = (0, 1); + assert_eq!( + app.layout.record_row(west).unwrap().coords.get("Region"), + Some("West") + ); + + app.cursor_mut().selected = (east, col_of(&app, "Cost")); + press(&mut app, 'y'); + press(&mut app, 'y'); + assert_eq!( + app.view_state.yanked, + Some(CellValue::Number(700.0)), + "yy must yank the record's stored value, not the synthetic key's" + ); + + app.cursor_mut().selected = (west, col_of(&app, "Revenue")); + press(&mut app, 'p'); + let wb = &app.model_state.workbook; + assert_eq!( + wb.model.get_cell(&record("West", "Revenue")), + Some(&CellValue::Number(700.0)), + "p must write the real record under the cursor" + ); + assert_eq!( + wb.model.get_cell(&record("East", "Cost")), + Some(&CellValue::Number(700.0)) + ); + assert_eq!(synthetic_cells(wb), Vec::::new()); + + let text = crate::persistence::format_md(wb); + let data = text.split("## Data").nth(1).expect("a data section"); + assert!( + !data.contains("_Index") && !data.contains("_Dim"), + "the saved document carries a synthetic-keyed line:\n{text}" + ); + let reloaded = crate::persistence::parse_md(&text).expect("round trip"); + assert_eq!( + reloaded.model.get_cell(&record("West", "Revenue")), + Some(&CellValue::Number(700.0)) + ); + assert_eq!(synthetic_cells(&reloaded), Vec::::new()); + } + + #[test] + fn yy_on_a_records_coordinate_column_yanks_the_item_as_text() { + let mut app = records_app(); + app.cursor_mut().selected = (1, col_of(&app, "Region")); + press(&mut app, 'y'); + press(&mut app, 'y'); + assert_eq!(app.view_state.yanked, Some(CellValue::Text("West".into()))); + } + + #[test] + fn p_on_a_records_column_that_stores_no_value_changes_nothing() { + for label in ["Region", "+cat", "+"] { + let mut app = records_app(); + app.view_state.yanked = Some(CellValue::Number(5.5)); + let before = crate::persistence::format_md(&app.model_state.workbook); + app.cursor_mut().selected = (1, col_of(&app, label)); + press(&mut app, 'p'); + assert_eq!( + crate::persistence::format_md(&app.model_state.workbook), + before, + "p on the {label} column changed the document" + ); + assert!( + !app.view_state.status_msg.is_empty(), + "p on the {label} column declined silently" + ); + } + // The control: the same paste onto a measure column does write. + let mut app = records_app(); + app.view_state.yanked = Some(CellValue::Number(5.5)); + app.cursor_mut().selected = (1, col_of(&app, "Revenue")); + press(&mut app, 'p'); + assert_eq!( + app.model_state + .workbook + .model + .get_cell(&record("West", "Revenue")), + Some(&CellValue::Number(5.5)), + "the control never wrote, so the declines above prove nothing" + ); + } + + #[test] + fn p_inside_a_drill_neither_writes_nor_stages() { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.model + .set_cell(record("East", "Revenue"), CellValue::Number(1.0)); + wb.model + .set_cell(record("East", "Cost"), CellValue::Number(2.0)); + let mut app = App::new(wb, None); + press(&mut app, '>'); + assert!(app.drill_state.is_some(), "setup: drilled"); + let before = crate::persistence::format_md(&app.model_state.workbook); + app.view_state.yanked = Some(CellValue::Number(5.5)); + app.cursor_mut().selected = (0, col_of(&app, "Cost")); + press(&mut app, 'p'); + assert_eq!( + crate::persistence::format_md(&app.model_state.workbook), + before + ); + assert!( + app.drill_state.as_ref().unwrap().pending_edits.is_empty(), + "a paste was staged as text, which loses the value's type" + ); + assert_eq!( + synthetic_cells(&app.model_state.workbook), + Vec::::new() + ); + assert!(!app.view_state.status_msg.is_empty(), "declined silently"); + } + + /// Region on rows, Product aggregated away on None: every cell is an aggregate. + fn aggregated_app(product_axis: crate::view::Axis) -> App { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("Region", "West").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", crate::view::Axis::Row); + wb.active_view_mut() + .set_axis("_Measure", crate::view::Axis::Column); + wb.active_view_mut().set_axis("Product", product_axis); + wb.model.set_cell( + record("East", "Revenue").with("Product", "Widget"), + CellValue::Number(10.0), + ); + let mut app = App::new(wb, None); + app.view_state.yanked = Some(CellValue::Number(5.5)); + app.cursor_mut().selected = (1, 0); + app + } + + #[test] + fn p_onto_a_none_axis_aggregate_writes_nothing() { + let mut app = aggregated_app(crate::view::Axis::None); + let before = crate::persistence::format_md(&app.model_state.workbook); + press(&mut app, 'p'); + assert_eq!( + crate::persistence::format_md(&app.model_state.workbook), + before, + "p wrote a cell at the aggregate's partial key" + ); + assert!(!app.view_state.status_msg.is_empty(), "declined silently"); + + // The control: with Product paged rather than aggregated, p writes. + let mut app = aggregated_app(crate::view::Axis::Page); + press(&mut app, 'p'); + assert_eq!( + app.model_state + .workbook + .model + .get_cell(&record("West", "Revenue").with("Product", "Widget")), + Some(&CellValue::Number(5.5)) + ); + } + + #[test] + fn the_aggregate_decline_does_not_send_the_user_where_paste_also_declines() { + let mut app = aggregated_app(crate::view::Axis::None); + app.model_state.workbook.model.set_cell( + record("West", "Revenue").with("Product", "Gadget"), + CellValue::Number(3.0), + ); + app.rebuild_layout(); + press(&mut app, 'p'); + let advice = app.view_state.status_msg.clone(); + press(&mut app, '>'); + assert!(app.drill_state.is_some(), "setup: the aggregate drills"); + app.cursor_mut().selected = (0, col_of(&app, "Revenue")); + press(&mut app, 'p'); + let in_drill = app.view_state.status_msg.clone(); + assert_eq!( + in_drill, PASTE_INSIDE_DRILL, + "setup: the drilled paste declines" + ); + assert!( + !advice.to_lowercase().contains("drill"), + "{advice:?} sends the user into a drill, where p answers {in_drill:?}" + ); + } + + #[test] + fn paste_with_an_explicit_full_key_writes_even_where_the_view_aggregates() { + let app = aggregated_app(crate::view::Axis::None); + let wb = &app.model_state.workbook; + let layout = make_layout(wb); + let reg = make_registry(); + let yanked = Some(CellValue::Number(5.5)); + let mut ctx = make_ctx(wb, &layout, ®); + ctx.yanked = &yanked; + assert!(!ctx.none_cats().is_empty(), "setup: the view aggregates"); + let key = record("West", "Revenue").with("Product", "Gadget"); + let effects = PasteCell { key: key.clone() }.execute(&ctx); + let mut app = aggregated_app(crate::view::Axis::None); + app.apply_effects(effects); + assert_eq!( + app.model_state.workbook.model.get_cell(&key), + Some(&CellValue::Number(5.5)), + "a key naming every coordinate is a record, whatever the view shows" + ); + } + #[test] fn transpose_produces_transpose() { let m = two_cat_model(); @@ -146,9 +418,13 @@ impl Cmd for YankCell { "yank" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let value = ctx - .model - .evaluate_aggregated(&self.key, Aggregation::of(ctx.none_cats())); + // A records cell is the layout's one resolution, as the browser yanks it (gotcha 1). + let value = match synthetic_record_info(&self.key) { + Some((row, col)) => ctx.layout.records_value(row, col), + None => ctx + .model + .evaluate_aggregated(&self.key, Aggregation::of(ctx.none_cats())), + }; vec![ Box::new(effect::SetYanked(value)), effect::set_status("Yanked"), @@ -166,14 +442,47 @@ impl Cmd for PasteCell { "paste" } fn execute(&self, ctx: &CmdContext) -> Vec> { - if let Some(value) = ctx.yanked.clone() { - vec![Box::new(effect::SetCell(self.key.clone(), value))] - } else { - vec![] + match ctx.yanked.clone() { + Some(value) => vec![paste_into(ctx, &self.key, value)], + None => vec![], } } } +/// Where a paste at `key` lands. The reasoning, and why a drill declines: +/// `context/code/command.md`, "Paste lands on a record or nowhere". +fn paste_into(ctx: &CmdContext, key: &CellKey, value: CellValue) -> Box { + match synthetic_record_info(key) { + Some((row, col)) => paste_into_record(ctx, row, col, value), + None if aggregates(ctx, key) => effect::set_status(PASTE_ONTO_AGGREGATE), + None => Box::new(effect::SetCell(key.clone(), value)), + } +} + +fn paste_into_record( + ctx: &CmdContext, + row: usize, + col: usize, + value: CellValue, +) -> Box { + match (ctx.layout.record_cell_key(row, col), ctx.has_drill_state) { + (Some(_), true) => effect::set_status(PASTE_INSIDE_DRILL), + (Some(real), false) => Box::new(effect::SetCell(real, value)), + (None, _) => effect::set_status(PASTE_ONTO_NO_VALUE), + } +} + +/// Whether `key` leaves out a regular category the layout aggregates over. +fn aggregates(ctx: &CmdContext, key: &CellKey) -> bool { + ctx.none_cats().iter().any(|cat| { + key.get(cat).is_none() && ctx.model.category(cat).is_some_and(|c| c.kind.is_regular()) + }) +} + +const PASTE_ONTO_AGGREGATE: &str = "Cannot paste onto an aggregate — it combines several records, so there is no one cell to write"; +const PASTE_ONTO_NO_VALUE: &str = "Paste lands only on a measure column"; +const PASTE_INSIDE_DRILL: &str = "Paste is not staged inside a drill — edit the cell instead"; + // ── View commands ──────────────────────────────────────────────────────────── #[derive(Debug)] diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index a3e0115c..39985d5c 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1233,6 +1233,81 @@ fn page_selections(app: &App) -> Vec<(String, Option)> { .collect() } +/// improvise-6p8q: the TUI yanked and pasted through the synthetic records key. +/// See `context/test-evidence/clients.md`. +#[test] +fn yy_then_p_in_records_mode_writes_the_record_the_host_writes() { + let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE); + let two_records = || { + let mut app = furnished_host(); + let south = north_revenue().with("Region", "South"); + app.model_state + .workbook + .model + .set_cell(south, improvise_protocol::CellValue::Number(900.0)); + app.rebuild_layout(); + reduce_key(&mut app, r); + app + }; + let (mut app, mut twin) = (two_records(), two_records()); + let revenue = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == "Revenue") + .expect("a Revenue column"); + let south = 1; + assert_eq!( + app.layout.record_row(south).unwrap().coords.get("Region"), + Some("South") + ); + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (0, revenue); + } + let mut client = records_client(&app); + + both_ends(&mut app, &mut twin, &mut client, "y"); + both_ends(&mut app, &mut twin, &mut client, "y"); + assert_eq!( + (client.view.yanked.clone(), app.view_state.yanked.clone()), + ( + Some(improvise_protocol::CellValue::Number(1200.0)), + Some(improvise_protocol::CellValue::Number(1200.0)) + ), + "both ends yank North's stored Revenue" + ); + + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (south, revenue); + } + client.view.cursor_mut().selected = (south, revenue); + both_ends(&mut app, &mut twin, &mut client, "p"); + assert_eq!( + app.model_state + .workbook + .model + .get_cell(&north_revenue().with("Region", "South")), + Some(&improvise_protocol::CellValue::Number(1200.0)), + "the paste must land on South's real record" + ); + + // Off the measure columns the two ends still yank one value. + for label in ["Region", "+"] { + let col = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == label) + .unwrap(); + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (south, col); + } + client.view.cursor_mut().selected = (south, col); + both_ends(&mut app, &mut twin, &mut client, "y"); + both_ends(&mut app, &mut twin, &mut client, "y"); + assert_eq!(client.view.yanked, app.view_state.yanked, "{label}"); + } + assert_eq!( + app.view_state.yanked, + Some(improvise_protocol::CellValue::Text(String::new())), + "the last yank must have run, over the blank creating column" + ); +} + /// Both hosts driven into records mode the way a user gets there, so the /// `_Records` view and its `_Index`/`_Dim` axes are the real ones. fn records_pair() -> (App, App) {