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

refactor(ui): move workbook/file_path/dirty into ModelState (improvise-x2c)

Browse Source 
Step 2 of vb4. Populates ModelState with the document slice and routes
every read/write site (effects, draw, main, app methods, tests) through
app.model_state.X. App no longer owns workbook, file_path, or dirty
directly. Effect::apply signatures still take &mut App; narrowing happens
in step 5 (improvise-drg).

A structural test (app_model_state_owns_workbook_file_path_and_dirty)
locks in the field layout. ModelState now has a manual Default impl
that creates an "Untitled" Workbook so the existing constructibility
test keeps working.

623 tests pass workspace-wide (+1 new). cargo clippy --workspace --tests
clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-04-28 22:06:33 -07:00
parent f11d79f700
commit 917b928759
4 changed files with 228 additions and 186 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ui/app.rs
+92 -69
View File
@@ -187,10 +187,23 @@ impl AppMode {
/// Document state slice: the workbook and its IO bookkeeping. Distinct from
/// `Workbook` itself (which is pure document semantics in `improvise-core`)
/// because `file_path` and `dirty` are persistence-layer concerns. Filled in
/// by improvise-x2c (vb4 step 2).
#[derive(Debug, Default)]
pub struct ModelState {}
/// because `file_path` and `dirty` are persistence-layer concerns.
#[derive(Debug)]
pub struct ModelState {
pub workbook: Workbook,
pub file_path: Option<PathBuf>,
pub dirty: bool,
}
impl Default for ModelState {
fn default() -> Self {
Self {
workbook: Workbook::new("Untitled"),
file_path: None,
dirty: false,
}
}
}
/// UI session-state slice: mode, cursors, panels, buffers, navigation stacks,
/// and other per-session state that does not persist to disk. Filled in by
@@ -199,8 +212,7 @@ pub struct ModelState {}
pub struct ViewState {}
pub struct App {
pub workbook: Workbook,
pub file_path: Option<PathBuf>,
pub model_state: ModelState,
pub mode: AppMode,
pub status_msg: String,
pub wizard: Option<ImportWizard>,
@@ -213,7 +225,6 @@ pub struct App {
pub cat_panel_cursor: usize,
pub view_panel_cursor: usize,
pub formula_cursor: usize,
pub dirty: bool,
/// Yanked cell value for `p` paste
pub yanked: Option<CellValue>,
/// Tile select cursor (which category index is highlighted)
@@ -264,8 +275,11 @@ impl App {
GridLayout::with_frozen_records(&workbook.model, view, None)
};
Self {
workbook,
file_path,
model_state: ModelState {
workbook,
file_path,
dirty: false,
},
mode: AppMode::Normal,
status_msg: String::new(),
wizard: None,
@@ -278,7 +292,6 @@ impl App {
cat_panel_cursor: 0,
view_panel_cursor: 0,
formula_cursor: 0,
dirty: false,
yanked: None,
tile_cat_idx: 0,
view_back_stack: Vec::new(),
@@ -299,20 +312,20 @@ impl App {
/// Rebuild the grid layout from current workbook, active view, and drill
/// state. Note: `with_frozen_records` already handles pruning internally.
pub fn rebuild_layout(&mut self) {
let none_cats = self.workbook.active_view().none_cats();
self.workbook.model.recompute_formulas(&none_cats);
let view = self.workbook.active_view();
let none_cats = self.model_state.workbook.active_view().none_cats();
self.model_state.workbook.model.recompute_formulas(&none_cats);
let view = self.model_state.workbook.active_view();
let frozen = self.drill_state.as_ref().map(|s| Rc::clone(&s.records));
self.layout = GridLayout::with_frozen_records(&self.workbook.model, view, frozen);
self.layout = GridLayout::with_frozen_records(&self.model_state.workbook.model, view, frozen);
}
pub fn cmd_context(&self, key: KeyCode, _mods: KeyModifiers) -> CmdContext<'_> {
let view = self.workbook.active_view();
let view = self.model_state.workbook.active_view();
let layout = &self.layout;
let (sel_row, sel_col) = view.selected;
CmdContext {
model: &self.workbook.model,
workbook: &self.workbook,
model: &self.model_state.workbook.model,
workbook: &self.model_state.workbook,
view,
layout,
registry: self.keymap_set.registry(),
@@ -322,7 +335,7 @@ impl App {
col_offset: view.col_offset,
search_query: &self.search_query,
yanked: &self.yanked,
dirty: self.dirty,
dirty: self.model_state.dirty,
search_mode: self.search_mode,
formula_panel_open: self.formula_panel_open,
category_panel_open: self.category_panel_open,
@@ -345,7 +358,7 @@ impl App {
.or_else(|| layout.resolve_display(k))
.unwrap_or_default()
} else {
self.workbook
self.model_state.workbook
.model
.get_cell(k)
.map(|v| v.to_string())
@@ -359,7 +372,7 @@ impl App {
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);
compute_col_widths(&self.model_state.workbook.model, layout, fmt_comma, fmt_decimals);
let row_header_width = compute_row_header_width(layout);
compute_visible_cols(
&col_widths,
@@ -392,7 +405,7 @@ impl App {
/// Virtual categories (_Index, _Dim, _Measure) are always present and don't count.
pub fn is_empty_model(&self) -> bool {
use crate::model::category::CategoryKind;
self.workbook.model.categories.values().all(|c| {
self.model_state.workbook.model.categories.values().all(|c| {
matches!(
c.kind,
CategoryKind::VirtualIndex
@@ -431,12 +444,12 @@ impl App {
}
pub fn autosave_if_needed(&mut self) {
if self.dirty
if self.model_state.dirty
&& self.last_autosave.elapsed() > Duration::from_secs(30)
&& let Some(path) = &self.file_path.clone()
&& let Some(path) = &self.model_state.file_path.clone()
{
let ap = persistence::autosave_path(path);
let _ = persistence::save(&self.workbook, &ap);
let _ = persistence::save(&self.model_state.workbook, &ap);
self.last_autosave = Instant::now();
}
}
@@ -494,6 +507,16 @@ mod tests {
let _: ViewState = ViewState::default();
}
/// improvise-x2c: ModelState owns the document slice — workbook,
/// file_path, and dirty. App accesses them through model_state.
#[test]
fn app_model_state_owns_workbook_file_path_and_dirty() {
let app = App::new(Workbook::new("T"), Some(PathBuf::from("/tmp/x")));
let _: &Workbook = &app.model_state.workbook;
let _: &Option<PathBuf> = &app.model_state.file_path;
let _: bool = app.model_state.dirty;
}
fn two_col_model() -> App {
let mut wb = Workbook::new("T");
wb.add_category("Row").unwrap(); // → Row axis
@@ -515,7 +538,7 @@ mod tests {
fn enter_advance_cmd(app: &App) -> crate::command::cmd::navigation::EnterAdvance {
use crate::command::cmd::navigation::CursorState;
let view = app.workbook.active_view();
let view = app.model_state.workbook.active_view();
let cursor = CursorState {
row: view.selected.0,
col: view.selected.1,
@@ -532,29 +555,29 @@ mod tests {
#[test]
fn enter_advance_moves_down_within_column() {
let mut app = two_col_model();
app.workbook.active_view_mut().selected = (0, 0);
app.model_state.workbook.active_view_mut().selected = (0, 0);
let cmd = enter_advance_cmd(&app);
run_cmd(&mut app, &cmd);
assert_eq!(app.workbook.active_view().selected, (1, 0));
assert_eq!(app.model_state.workbook.active_view().selected, (1, 0));
}
#[test]
fn enter_advance_wraps_to_top_of_next_column() {
let mut app = two_col_model();
// row_max = 2 (A,B,C), col 0 → should wrap to (0, 1)
app.workbook.active_view_mut().selected = (2, 0);
app.model_state.workbook.active_view_mut().selected = (2, 0);
let cmd = enter_advance_cmd(&app);
run_cmd(&mut app, &cmd);
assert_eq!(app.workbook.active_view().selected, (0, 1));
assert_eq!(app.model_state.workbook.active_view().selected, (0, 1));
}
#[test]
fn enter_advance_stays_at_bottom_right() {
let mut app = two_col_model();
app.workbook.active_view_mut().selected = (2, 1);
app.model_state.workbook.active_view_mut().selected = (2, 1);
let cmd = enter_advance_cmd(&app);
run_cmd(&mut app, &cmd);
assert_eq!(app.workbook.active_view().selected, (2, 1));
assert_eq!(app.model_state.workbook.active_view().selected, (2, 1));
}
#[test]
@@ -633,34 +656,34 @@ mod tests {
}
assert_eq!(
app.workbook.active_view().selected.1,
app.model_state.workbook.active_view().selected.1,
3,
"cursor should be at column 3"
);
assert!(
app.workbook.active_view().col_offset > 0,
app.model_state.workbook.active_view().col_offset > 0,
"col_offset should scroll when cursor moves past visible area (only ~2 cols fit \
in 80-char terminal with 26-char-wide columns), but col_offset is {}",
app.workbook.active_view().col_offset
app.model_state.workbook.active_view().col_offset
);
}
#[test]
fn home_jumps_to_first_col() {
let mut app = two_col_model();
app.workbook.active_view_mut().selected = (1, 1);
app.model_state.workbook.active_view_mut().selected = (1, 1);
app.handle_key(KeyEvent::new(KeyCode::Home, KeyModifiers::NONE))
.unwrap();
assert_eq!(app.workbook.active_view().selected, (1, 0));
assert_eq!(app.model_state.workbook.active_view().selected, (1, 0));
}
#[test]
fn end_jumps_to_last_col() {
let mut app = two_col_model();
app.workbook.active_view_mut().selected = (1, 0);
app.model_state.workbook.active_view_mut().selected = (1, 0);
app.handle_key(KeyEvent::new(KeyCode::End, KeyModifiers::NONE))
.unwrap();
assert_eq!(app.workbook.active_view().selected, (1, 1));
assert_eq!(app.model_state.workbook.active_view().selected, (1, 1));
}
#[test]
@@ -668,40 +691,40 @@ mod tests {
let mut app = two_col_model();
// Add enough rows
for i in 0..30 {
app.workbook
app.model_state.workbook
.model
.category_mut("Row")
.unwrap()
.add_item(format!("R{i}"));
}
app.term_height = 28; // ~20 visible rows → delta = 15
app.workbook.active_view_mut().selected = (0, 0);
app.model_state.workbook.active_view_mut().selected = (0, 0);
app.handle_key(KeyEvent::new(KeyCode::PageDown, KeyModifiers::NONE))
.unwrap();
assert_eq!(app.workbook.active_view().selected.1, 0, "column preserved");
assert_eq!(app.model_state.workbook.active_view().selected.1, 0, "column preserved");
assert!(
app.workbook.active_view().selected.0 > 0,
app.model_state.workbook.active_view().selected.0 > 0,
"row should advance on PageDown"
);
// 3/4 of ~20 = 15
assert_eq!(app.workbook.active_view().selected.0, 15);
assert_eq!(app.model_state.workbook.active_view().selected.0, 15);
}
#[test]
fn page_up_scrolls_backward() {
let mut app = two_col_model();
for i in 0..30 {
app.workbook
app.model_state.workbook
.model
.category_mut("Row")
.unwrap()
.add_item(format!("R{i}"));
}
app.term_height = 28;
app.workbook.active_view_mut().selected = (20, 0);
app.model_state.workbook.active_view_mut().selected = (20, 0);
app.handle_key(KeyEvent::new(KeyCode::PageUp, KeyModifiers::NONE))
.unwrap();
assert_eq!(app.workbook.active_view().selected.0, 5);
assert_eq!(app.model_state.workbook.active_view().selected.0, 5);
}
#[test]
@@ -709,22 +732,22 @@ mod tests {
let mut app = two_col_model();
// Total rows: A, B, C + R0..R9 = 13 rows. Last row = 12.
for i in 0..10 {
app.workbook
app.model_state.workbook
.model
.category_mut("Row")
.unwrap()
.add_item(format!("R{i}"));
}
app.term_height = 13; // ~5 visible rows
app.workbook.active_view_mut().selected = (0, 0);
app.model_state.workbook.active_view_mut().selected = (0, 0);
// G jumps to last row (row 12)
app.handle_key(KeyEvent::new(KeyCode::Char('G'), KeyModifiers::NONE))
.unwrap();
let last = app.workbook.active_view().selected.0;
let last = app.model_state.workbook.active_view().selected.0;
assert_eq!(last, 12, "should be at last row");
// With only ~5 visible rows and 13 rows, offset should scroll.
// Bug: hardcoded 20 means `12 >= 0 + 20` is false → no scroll.
let offset = app.workbook.active_view().row_offset;
let offset = app.model_state.workbook.active_view().row_offset;
assert!(
offset > 0,
"row_offset should scroll when last row is beyond visible area, but is {offset}"
@@ -735,34 +758,34 @@ mod tests {
fn ctrl_d_scrolls_viewport_with_small_terminal() {
let mut app = two_col_model();
for i in 0..30 {
app.workbook
app.model_state.workbook
.model
.category_mut("Row")
.unwrap()
.add_item(format!("R{i}"));
}
app.term_height = 13; // ~5 visible rows
app.workbook.active_view_mut().selected = (0, 0);
app.model_state.workbook.active_view_mut().selected = (0, 0);
// Ctrl+d scrolls by 5 rows
app.handle_key(KeyEvent::new(KeyCode::Char('d'), KeyModifiers::CONTROL))
.unwrap();
assert_eq!(app.workbook.active_view().selected.0, 5);
assert_eq!(app.model_state.workbook.active_view().selected.0, 5);
// Press Ctrl+d again — now at row 10 with only 5 visible rows,
// row_offset should have scrolled (not stay at 0 due to hardcoded 20)
app.handle_key(KeyEvent::new(KeyCode::Char('d'), KeyModifiers::CONTROL))
.unwrap();
assert_eq!(app.workbook.active_view().selected.0, 10);
assert_eq!(app.model_state.workbook.active_view().selected.0, 10);
assert!(
app.workbook.active_view().row_offset > 0,
app.model_state.workbook.active_view().row_offset > 0,
"row_offset should scroll with small terminal, but is {}",
app.workbook.active_view().row_offset
app.model_state.workbook.active_view().row_offset
);
}
#[test]
fn tab_in_edit_mode_commits_and_moves_right() {
let mut app = two_col_model();
app.workbook.active_view_mut().selected = (0, 0);
app.model_state.workbook.active_view_mut().selected = (0, 0);
// Enter edit mode
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
@@ -780,7 +803,7 @@ mod tests {
app.mode
);
assert_eq!(
app.workbook.active_view().selected.1,
app.model_state.workbook.active_view().selected.1,
1,
"should have moved to column 1"
);
@@ -885,7 +908,7 @@ mod tests {
.unwrap();
assert_eq!(
app.workbook.model.get_cell(&CellKey::new(vec![(
app.model_state.workbook.model.get_cell(&CellKey::new(vec![(
"_Measure".to_string(),
"Rev".to_string(),
)])),
@@ -957,11 +980,11 @@ mod tests {
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
app.model_state.workbook
.model
.set_cell(CellKey::new(vec![]), CellValue::Number(0.0));
assert!(
app.workbook
app.model_state.workbook
.model
.data
.iter_cells()
@@ -976,7 +999,7 @@ mod tests {
"setup: should have left records mode"
);
assert!(
!app.workbook
!app.model_state.workbook
.model
.data
.iter_cells()
@@ -994,7 +1017,7 @@ mod tests {
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.model_state.workbook.active_view_mut().selected = (last_row, last_col);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
@@ -1025,7 +1048,7 @@ mod tests {
let last_row = initial_rows - 1;
let last_col = app.layout.col_count() - 1;
app.workbook.active_view_mut().selected = (last_row, last_col);
app.model_state.workbook.active_view_mut().selected = (last_row, last_col);
// Enter edit mode on the bottom-right cell
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
@@ -1042,7 +1065,7 @@ mod tests {
"TAB on bottom-right should insert a record below"
);
assert_eq!(
app.workbook.active_view().selected,
app.model_state.workbook.active_view().selected,
(initial_rows, 0),
"TAB should move to first cell of the new row"
);
@@ -1078,7 +1101,7 @@ mod tests {
let value_col = (0..app.layout.col_count())
.find(|&col| app.layout.col_label(col) == "Value")
.expect("drill view should include a Value column");
app.workbook.active_view_mut().selected = (0, value_col);
app.model_state.workbook.active_view_mut().selected = (0, value_col);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
app.handle_key(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE))
@@ -1089,7 +1112,7 @@ mod tests {
.unwrap();
assert_eq!(
app.workbook.model.get_cell(&record_key),
app.model_state.workbook.model.get_cell(&record_key),
Some(&CellValue::Number(1.0)),
"drill edit should remain staged until leaving the drill view"
);
@@ -1107,7 +1130,7 @@ mod tests {
.unwrap();
assert_eq!(
app.workbook.model.get_cell(&record_key),
app.model_state.workbook.model.get_cell(&record_key),
Some(&CellValue::Number(9.0)),
"leaving drill view should apply the staged edit"
);
@@ -1140,7 +1163,7 @@ mod tests {
.unwrap();
assert!(
!app.workbook
!app.model_state.workbook
.model
.category("Region")
.unwrap()