fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

feat(ui): implement AbortChain and CleanEmptyRecords effects

Browse Source 
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)
This commit is contained in:
Edward Langley
2026-04-15 23:42:44 -07:00
parent f272a9d459
commit 489e2805e8
2 changed files with 225 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

95
src/ui/app.rs
View File

@ -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<Box<dyn super::effect::Effect>>) {
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"
);
}

145
src/ui/effect.rs
View File

@ -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<CellKey> = 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<dyn Effect> {
@ -987,8 +1025,8 @@ pub fn help_page_set(page: usize) -> Box<dyn Effect> {
#[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<Box<dyn Effect>> = 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 ─────────────────────────────────────────────────