Merge pull request #184 from fiddlerwoaroof/fix/6p8q-records-yank-paste

TUI records-mode yy/p resolve the real record; p declines aggregates (improvise-6p8q, l4mp)
This commit is contained in:
Ed Langley
2026-09-22 21:50:15 -07:00
committed by GitHub
5 changed files with 518 additions and 8 deletions
+45
View File
@@ -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`.
+16 -1
View File
@@ -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`.
+66
View File
@@ -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.
@@ -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<CellKey> {
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::<CellKey>::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::<CellKey>::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::<CellKey>::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, &reg);
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<Box<dyn Effect>> {
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<Box<dyn Effect>> {
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<dyn Effect> {
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<dyn Effect> {
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)]
@@ -1233,6 +1233,81 @@ fn page_selections(app: &App) -> Vec<(String, Option<String>)> {
.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) {