fix: review round — the Ok-but-no-op resets, the measure regression, and a
loop-of-one test F1 (HIGH). Gating the reset on Ok is not the same as gating it on "something changed". Six accepted commands changed nothing and reset every cursor anyway: SetAxis to the axis already held, HideItem on an already-hidden item, ShowItem on a never-hidden one, AddItem of an existing item (Category::add_item dedupes and returns Ok), and RemoveItem/RemoveCategory of something absent. Each now asks the state before mutating. The case that made it urgent needs no second client: TileSelect binds `r` to set-axis-for-tile row, which emits SetAxis with no comparison against the current axis, so `r` on a Row category sent the user's own cursor to the origin. F2 (REGRESSION this PR introduced). Naming a measure in the trailing `+` column at the bottom-right of a records grid sent the cursor home: AddItem now resets, inserts_a_column matches only NewCategory, and EnterAdvance at the bottom-right returns AbortChain with no SetSelected. The restore moves to BEFORE the advance, where an aborting advance cannot skip it -- gotcha 21's rule applied to the cursor rather than the mode -- which also subsumes the +cat arm's copy. F3. a_refused_model_command_invalidates_no_cursor named a class and tested one member; injecting reset_cursor into AddCategory's Err arm left it green. Now a table over all three refusable effects, and that injection fails it by name. F5. The benchmark could not reproduce its own figures: it took the mean over a few iterations, and review measured the same share at 8% and at 131% -- a part slower than the whole containing it, printed flat. It also passed (false, 2) where grid_of passes the view's (true, 0), so it timed a call that never happens. Now min-of-N with the view's own format, and a share above 100% prints UNSTABLE. Figures are stable across runs at ratio 1.2-1.5x, col_widths 45-49%. F6. The drg counts omitted the 8 simple_effect! invocations and misclassified AddItem at base through a regex that cannot see `app` newline `.model_state` -- doctrine(derive_then_verify) catching the person citing it. Correct: 57 effects, narrow set 32 -> 25, not 25 -> 17 of 49. The direction holds and is still the argument; the headline was overstated by about half again. Corrected in context/code/command.md and on the bead, not quietly restated. F9. a_model_command_that_leaves_the_active_view_alone_broadcasts_nothing asserts a ResetCursor broadcast, so the name read as a lie: ..._broadcasts_no_switch. F4 filed as improvise-pdvm: AddItem/RemoveItem declare CellData so layout_version does not bump while cursor_epoch does, and this PR's own reshape assertion is the first evidence for it. Related to improvise-75ld, same question other direction. Faults injected for every new guard: SetAxis's moved check, AddItem's fresh check, AddCategory's Err arm, and the pre-fix advance ordering ((0,0) vs (0,3)). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -197,6 +197,79 @@ mod tests {
|
||||
assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}");
|
||||
}
|
||||
|
||||
/// Naming a new measure in the trailing `+` column at the bottom-right sent the
|
||||
/// cursor home: `AddItem` resets, and the advance aborts before restoring it.
|
||||
#[test]
|
||||
fn naming_a_measure_at_the_bottom_right_leaves_the_cursor_put() {
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::ui::app::App;
|
||||
use crate::view::Axis;
|
||||
use crate::workbook::Workbook;
|
||||
|
||||
let mut wb = Workbook::new("Records");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_item("Region", "East").unwrap();
|
||||
wb.add_item("_Measure", "Revenue").unwrap();
|
||||
wb.model.set_cell(
|
||||
CellKey::new(vec![
|
||||
("Region".into(), "East".into()),
|
||||
("_Measure".into(), "Revenue".into()),
|
||||
]),
|
||||
CellValue::Number(100.0),
|
||||
);
|
||||
{
|
||||
let v = wb.active_view_mut();
|
||||
v.set_axis("_Index", Axis::Row);
|
||||
v.set_axis("_Dim", Axis::Column);
|
||||
v.set_axis("Region", Axis::None);
|
||||
v.set_axis("_Measure", Axis::None);
|
||||
}
|
||||
let mut app = App::new(wb, None);
|
||||
app.rebuild_layout();
|
||||
assert!(
|
||||
app.layout.is_records_mode(),
|
||||
"the fixture is not records mode"
|
||||
);
|
||||
|
||||
let bottom = app.layout.row_count() - 1;
|
||||
let right = app.layout.col_count() - 1;
|
||||
assert!(
|
||||
matches!(app.layout.record_col(right), Some(RecordCol::NewMeasure)),
|
||||
"the last column is not the new-measure column, so this proves nothing"
|
||||
);
|
||||
app.cursor_mut().selected = (bottom, right);
|
||||
|
||||
let effects = {
|
||||
let reg = make_registry();
|
||||
let mut ctx = make_ctx(&app.model_state.workbook, &app.layout, ®);
|
||||
ctx.selected = (bottom, right);
|
||||
let key = ctx.cell_key().expect("a cell under the cursor");
|
||||
CommitAndAdvance {
|
||||
key,
|
||||
value: "Cost".to_string(),
|
||||
advance: super::AdvanceDir::Down,
|
||||
cursor: super::CursorState::from_ctx(&ctx),
|
||||
edit_mode: AppMode::records_editing(),
|
||||
}
|
||||
.execute(&ctx)
|
||||
};
|
||||
app.apply_effects(effects);
|
||||
|
||||
assert!(
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.measure_item_names()
|
||||
.contains(&"Cost".to_string()),
|
||||
"the measure was not created, so the cursor question is moot"
|
||||
);
|
||||
assert_eq!(
|
||||
app.cursor().selected,
|
||||
(bottom, right),
|
||||
"naming a measure at the bottom-right sent the cursor home"
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-lae: the save prompt commits to `SaveAs`, not `ExportCsv` (wrong
|
||||
/// format) and not `Save` (there is no path yet).
|
||||
#[test]
|
||||
@@ -425,14 +498,16 @@ impl Cmd for CommitAndAdvance {
|
||||
// an abort cannot reach it (improvise-2zq, gotcha 21).
|
||||
effects.push(effect::change_mode(exit_mode_for(&self.edit_mode)));
|
||||
commit_cell_value(ctx, &self.key, &self.value, &mut effects);
|
||||
// The commit may reshape the grid and so reset the cursor. Restore it before
|
||||
// the advance, where an aborting advance cannot skip it (gotcha 21).
|
||||
effects.push(Box::new(effect::SetSelected(
|
||||
self.cursor.row,
|
||||
self.cursor.col,
|
||||
)));
|
||||
if inserts_a_column(ctx, &self.key) {
|
||||
// Staying put lands on the inserted column (improvise-2zq), and `AddCategory`
|
||||
// resets the cursor, so this restores it rather than relying on nobody moving it.
|
||||
// Staying put is what lands on the inserted column (improvise-2zq); the
|
||||
// restore above is what makes "staying put" something we do, not inherit.
|
||||
effects.push(Box::new(effect::RebuildLayout));
|
||||
effects.push(Box::new(effect::SetSelected(
|
||||
self.cursor.row,
|
||||
self.cursor.col,
|
||||
)));
|
||||
return effects;
|
||||
}
|
||||
match self.advance {
|
||||
|
||||
Reference in New Issue
Block a user