From 489e2805e86ca9fed0e704664652822f3a1e1ecf Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 15 Apr 2026 23:42:44 -0700 Subject: [PATCH] feat(ui): implement AbortChain and CleanEmptyRecords effects Implement AbortChain and CleanEmptyRecords effects to allow short-circuiting effect batches and purging cells with empty coordinates. Update the App struct to support aborting effects during the application of an effect batch. Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf) --- src/ui/app.rs | 95 ++++++++++++++++++++++++++++++- src/ui/effect.rs | 145 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 225 insertions(+), 15 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index a21593d..ec53bcd 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -229,6 +229,12 @@ pub struct App { /// Current grid layout, derived from model + view + drill_state. /// Rebuilt via `rebuild_layout()` after state changes. pub layout: GridLayout, + /// When set to true by an effect during `apply_effects`, the remaining + /// effects in the batch are skipped. The flag is reset at the start of + /// every `apply_effects` call. Use via the `AbortChain` effect — this is + /// the mechanism by which e.g. "advance at bottom-right" short-circuits + /// the trailing `EnterEditAtCursor` in a `CommitAndAdvance` chain. + pub abort_effects: bool, keymap_set: KeymapSet, } @@ -272,6 +278,7 @@ impl App { buffers: HashMap::new(), transient_keymap: None, layout, + abort_effects: false, keymap_set: KeymapSet::default_keymaps(), } } @@ -338,7 +345,8 @@ impl App { visible_rows: (self.term_height as usize).saturating_sub(8), visible_cols: { let (fmt_comma, fmt_decimals) = parse_number_format(&view.number_format); - let col_widths = compute_col_widths(&self.workbook.model, layout, fmt_comma, fmt_decimals); + let col_widths = + compute_col_widths(&self.workbook.model, layout, fmt_comma, fmt_decimals); let row_header_width = compute_row_header_width(layout); compute_visible_cols( &col_widths, @@ -353,8 +361,16 @@ impl App { } pub fn apply_effects(&mut self, effects: Vec>) { + self.abort_effects = false; for effect in effects { effect.apply(self); + if self.abort_effects { + // AbortChain (or another abort-setting effect) requested + // that the rest of this batch be skipped. Reset the flag so + // the next dispatch starts clean. + self.abort_effects = false; + break; + } } self.rebuild_layout(); } @@ -909,6 +925,73 @@ mod tests { app } + /// improvise-3zq (bug #2): `AddRecordRow` creates a cell with an empty + /// `CellKey` when no Page-axis categories supply coords — that cell + /// serialises as ` = 0` in .improv and re-appears on every records + /// toggle. Leaving records mode must clean up any such meaningless + /// records (inverse of the `SortData` that runs on entry). + #[test] + fn leaving_records_mode_cleans_empty_key_cells() { + use crate::model::cell::{CellKey, CellValue}; + let mut app = records_model_with_two_rows(); + // Simulate Tab-at-bottom-right having produced an empty-key cell. + app.workbook + .model + .set_cell(CellKey::new(vec![]), CellValue::Number(0.0)); + assert!( + app.workbook + .model + .data + .iter_cells() + .any(|(k, _)| k.0.is_empty()), + "setup: empty-key cell should be present" + ); + // Leave records mode via R. + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + assert!( + !app.layout.is_records_mode(), + "setup: should have left records mode" + ); + assert!( + !app.workbook + .model + .data + .iter_cells() + .any(|(k, _)| k.0.is_empty()), + "empty-key records should be cleaned when leaving records mode" + ); + } + + /// improvise-3zq (bug #1): Enter on the bottom-right cell of records + /// view should commit and leave edit mode. Previously `CommitAndAdvance` + /// pushed an `EnterEditAtCursor` effect unconditionally, so the cursor + /// stayed put and we re-entered editing on the same cell. + #[test] + fn enter_at_bottom_right_of_records_view_exits_editing() { + let mut app = records_model_with_two_rows(); + let last_row = app.layout.row_count() - 1; + let last_col = app.layout.col_count() - 1; + app.workbook.active_view_mut().selected = (last_row, last_col); + + app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE)) + .unwrap(); + assert!(app.mode.is_editing(), "setup: should be editing"); + + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + assert!( + !app.mode.is_editing(), + "Enter at bottom-right should exit editing, got {:?}", + app.mode + ); + assert!( + matches!(app.mode, AppMode::RecordsNormal), + "should return to RecordsNormal, got {:?}", + app.mode + ); + } + /// improvise-hmu: TAB on the bottom-right cell of records view should /// insert a new record below and move to the first cell of the new row /// in edit mode. @@ -963,7 +1046,8 @@ mod tests { ("Month".to_string(), "Jan".to_string()), ("Region".to_string(), "East".to_string()), ]); - wb.model.set_cell(record_key.clone(), CellValue::Number(1.0)); + wb.model + .set_cell(record_key.clone(), CellValue::Number(1.0)); let mut app = App::new(wb, None); app.handle_key(KeyEvent::new(KeyCode::Char('>'), KeyModifiers::NONE)) @@ -1034,7 +1118,12 @@ mod tests { .unwrap(); assert!( - !app.workbook.model.category("Region").unwrap().items.contains_key(""), + !app.workbook + .model + .category("Region") + .unwrap() + .items + .contains_key(""), "records-mode edits should not create empty category items" ); } diff --git a/src/ui/effect.rs b/src/ui/effect.rs index 16ae172..bdf7285 100644 --- a/src/ui/effect.rs +++ b/src/ui/effect.rs @@ -548,7 +548,7 @@ pub struct Save; impl Effect for Save { fn apply(&self, app: &mut App) { if let Some(ref path) = app.file_path { - match crate::persistence::save(&app.workbook,path) { + match crate::persistence::save(&app.workbook, path) { Ok(()) => { app.dirty = false; app.status_msg = format!("Saved to {}", path.display()); @@ -567,7 +567,7 @@ impl Effect for Save { pub struct SaveAs(pub PathBuf); impl Effect for SaveAs { fn apply(&self, app: &mut App) { - match crate::persistence::save(&app.workbook,&self.0) { + match crate::persistence::save(&app.workbook, &self.0) { Ok(()) => { app.file_path = Some(self.0.clone()); app.dirty = false; @@ -927,6 +927,44 @@ impl Effect for SetPanelCursor { } } +// ── Chain control ──────────────────────────────────────────────────────────── + +/// Signals `App::apply_effects` to skip the remaining effects in the batch. +/// The flag is reset at the start of every `apply_effects` call, so each +/// dispatch starts clean. Use this when a sequence's premise no longer +/// holds (e.g. "advance to next cell" at bottom-right) and later effects +/// (e.g. "re-enter editing there") should be short-circuited. +#[derive(Debug)] +pub struct AbortChain; +impl Effect for AbortChain { + fn apply(&self, app: &mut App) { + app.abort_effects = true; + } +} + +// ── Records hygiene ────────────────────────────────────────────────────────── + +/// Remove cells whose `CellKey` has no coordinates — these are meaningless +/// records that can only be produced by `AddRecordRow` when no page +/// filters are set. Pushed by `ToggleRecordsMode` when leaving records +/// mode, as the inverse of the `SortData` that runs on entry. +#[derive(Debug)] +pub struct CleanEmptyRecords; +impl Effect for CleanEmptyRecords { + fn apply(&self, app: &mut App) { + let empties: Vec = app + .workbook + .model + .data + .iter_cells() + .filter_map(|(k, _)| if k.0.is_empty() { Some(k) } else { None }) + .collect(); + for key in empties { + app.workbook.model.clear_cell(&key); + } + } +} + // ── Convenience constructors ───────────────────────────────────────────────── pub fn mark_dirty() -> Box { @@ -987,8 +1025,8 @@ pub fn help_page_set(page: usize) -> Box { #[cfg(test)] mod tests { use super::*; - use crate::workbook::Workbook; use crate::model::cell::{CellKey, CellValue}; + use crate::workbook::Workbook; fn test_app() -> App { let mut wb = Workbook::new("Test"); @@ -1048,7 +1086,10 @@ mod tests { ("Month".into(), "Jan".into()), ]); SetCell(key.clone(), CellValue::Number(42.0)).apply(&mut app); - assert_eq!(app.workbook.model.get_cell(&key), Some(&CellValue::Number(42.0))); + assert_eq!( + app.workbook.model.get_cell(&key), + Some(&CellValue::Number(42.0)) + ); ClearCell(key.clone()).apply(&mut app); assert_eq!(app.workbook.model.get_cell(&key), None); @@ -1073,7 +1114,8 @@ mod tests { let mut app = test_app(); // "Margin" does not exist as an item in "Type" before adding the formula assert!( - !app.workbook.model + !app.workbook + .model .category("Type") .unwrap() .ordered_item_names() @@ -1118,7 +1160,8 @@ mod tests { ); // Should NOT be in the category's own items assert!( - !app.workbook.model + !app.workbook + .model .category("_Measure") .unwrap() .ordered_item_names() @@ -1293,6 +1336,69 @@ mod tests { assert_eq!(app.mode, AppMode::Help); } + /// `AbortChain` must cause subsequent effects in the same + /// `apply_effects` batch to be skipped, and the flag must reset so the + /// next dispatch starts clean. + #[test] + fn abort_chain_short_circuits_apply_effects() { + let mut app = test_app(); + app.status_msg = String::new(); + let effects: Vec> = vec![ + Box::new(SetStatus("before".into())), + Box::new(AbortChain), + Box::new(SetStatus("after".into())), + ]; + app.apply_effects(effects); + assert_eq!( + app.status_msg, "before", + "effects after AbortChain must not apply" + ); + assert!( + !app.abort_effects, + "abort flag should reset at end of apply_effects" + ); + // A subsequent batch must not be affected by the prior abort. + app.apply_effects(vec![Box::new(SetStatus("next-batch".into()))]); + assert_eq!(app.status_msg, "next-batch"); + } + + /// `CleanEmptyRecords` removes cells whose `CellKey` has no + /// coordinates, and leaves all other cells untouched. + #[test] + fn clean_empty_records_removes_only_empty_key_cells() { + let mut app = test_app(); + // An empty-key cell (the bug: produced by AddRecordRow when no page + // filters are set). + app.workbook + .model + .set_cell(CellKey::new(vec![]), CellValue::Number(0.0)); + // Plus a well-formed cell that must survive. + let valid = CellKey::new(vec![ + ("Type".to_string(), "Food".to_string()), + ("Month".to_string(), "Jan".to_string()), + ]); + app.workbook + .model + .set_cell(valid.clone(), CellValue::Number(42.0)); + assert_eq!(app.workbook.model.data.iter_cells().count(), 2); + + CleanEmptyRecords.apply(&mut app); + + assert!( + !app.workbook + .model + .data + .iter_cells() + .any(|(k, _)| k.0.is_empty()), + "empty-key cell should be gone" + ); + assert_eq!( + app.workbook.model.get_cell(&valid), + Some(&CellValue::Number(42.0)), + "valid cell must survive" + ); + } + /// `EnterEditAtCursor` must use its `target_mode` field, *not* whatever /// `app.mode` happens to be when applied. Previous implementation /// branched on `app.mode.is_records()` — the parameterized version @@ -1492,7 +1598,9 @@ mod tests { ("Month".into(), "Jan".into()), ]); // Set original cell - app.workbook.model.set_cell(key.clone(), CellValue::Number(42.0)); + app.workbook + .model + .set_cell(key.clone(), CellValue::Number(42.0)); let records = vec![(key.clone(), CellValue::Number(42.0))]; StartDrill(records).apply(&mut app); @@ -1508,7 +1616,10 @@ mod tests { ApplyAndClearDrill.apply(&mut app); assert!(app.drill_state.is_none()); assert!(app.dirty); - assert_eq!(app.workbook.model.get_cell(&key), Some(&CellValue::Number(99.0))); + assert_eq!( + app.workbook.model.get_cell(&key), + Some(&CellValue::Number(99.0)) + ); } #[test] @@ -1518,7 +1629,9 @@ mod tests { ("Type".into(), "Food".into()), ("Month".into(), "Jan".into()), ]); - app.workbook.model.set_cell(key.clone(), CellValue::Number(42.0)); + app.workbook + .model + .set_cell(key.clone(), CellValue::Number(42.0)); let records = vec![(key.clone(), CellValue::Number(42.0))]; StartDrill(records).apply(&mut app); @@ -1540,7 +1653,10 @@ mod tests { ("Type".into(), "Drink".into()), ("Month".into(), "Jan".into()), ]); - assert_eq!(app.workbook.model.get_cell(&new_key), Some(&CellValue::Number(42.0))); + assert_eq!( + app.workbook.model.get_cell(&new_key), + Some(&CellValue::Number(42.0)) + ); // "Drink" should have been added as an item let items: Vec<&str> = app .workbook @@ -1560,7 +1676,9 @@ mod tests { ("Type".into(), "Food".into()), ("Month".into(), "Jan".into()), ]); - app.workbook.model.set_cell(key.clone(), CellValue::Number(42.0)); + app.workbook + .model + .set_cell(key.clone(), CellValue::Number(42.0)); let records = vec![(key.clone(), CellValue::Number(42.0))]; StartDrill(records).apply(&mut app); @@ -1640,7 +1758,10 @@ mod tests { item: "Food".to_string(), } .apply(&mut app); - assert_eq!(app.workbook.active_view().page_selection("Type"), Some("Food")); + assert_eq!( + app.workbook.active_view().page_selection("Type"), + Some("Food") + ); } // ── Hide/show items ─────────────────────────────────────────────────