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

refactor: extract GridLayout as single source for view→grid mapping

Browse Source 
Three copies of cross_product existed (grid.rs, app.rs, persistence/mod.rs)
with slightly different signatures. Extracted into GridLayout in
src/view/layout.rs, which is now the single canonical mapping from a View
to a 2-D grid: row/col counts, labels, and cell_key(row, col) → CellKey.

All consumers updated to use GridLayout::new(model, view):
- grid.rs: render_grid, total-row computation, page bar
- persistence/mod.rs: export_csv
- app.rs: move_selection, jump_to_last_row/col, scroll_rows,
  search_navigate, selected_cell_key

Also includes two app.rs UI bug fixes that were discovered while
refactoring:
- Ctrl+Arrow tile movement was unreachable (shadowed by plain arrow arms);
  moved before plain arrow handlers
- RemoveFormula dispatch now passes target_category (required by the
  formula management fix in the previous commit)

GridLayout has 6 unit tests covering counts, label formatting, cell_key
correctness, out-of-bounds, page coord inclusion, and evaluate round-trip.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Ed L
2026-03-24 00:12:00 -07:00
parent 9ef0a72894
commit d99d22820e
5 changed files with 282 additions and 309 deletions
Download Patch File Download Diff File Expand all files Collapse all files

138
src/ui/app.rs
View File

@ -7,7 +7,7 @@ use crate::model::Model;
use crate::model::cell::{CellKey, CellValue};
use crate::import::wizard::{ImportWizard, WizardStep};
use crate::persistence;
use crate::view::Axis;
use crate::view::{Axis, GridLayout};
use crate::command::{self, Command};
#[derive(Debug, Clone, PartialEq)]
@ -176,6 +176,17 @@ impl App {
}
}
// ── Tile movement (Ctrl+Arrow) — must come before plain arrows ──
(KeyCode::Left, KeyModifiers::CONTROL)
| (KeyCode::Right, KeyModifiers::CONTROL)
| (KeyCode::Up, KeyModifiers::CONTROL)
| (KeyCode::Down, KeyModifiers::CONTROL) => {
let count = self.model.category_names().len();
if count > 0 {
self.mode = AppMode::TileSelect { cat_idx: 0 };
}
}
// ── Navigation ─────────────────────────────────────────────────
(KeyCode::Up, _) | (KeyCode::Char('k'), KeyModifiers::NONE) => {
self.move_selection(-1, 0);
@ -279,17 +290,6 @@ impl App {
self.mode = AppMode::TileSelect { cat_idx: 0 };
}
}
// Legacy Ctrl+Arrow still works
(KeyCode::Left, KeyModifiers::CONTROL)
| (KeyCode::Right, KeyModifiers::CONTROL)
| (KeyCode::Up, KeyModifiers::CONTROL)
| (KeyCode::Down, KeyModifiers::CONTROL) => {
let count = self.model.category_names().len();
if count > 0 {
self.mode = AppMode::TileSelect { cat_idx: 0 };
}
}
// ── Page axis ──────────────────────────────────────────────────
(KeyCode::Char('['), _) => { self.page_prev(); }
(KeyCode::Char(']'), _) => { self.page_next(); }
@ -606,8 +606,10 @@ impl App {
}
KeyCode::Char('d') | KeyCode::Delete => {
if self.formula_cursor < self.model.formulas.len() {
let target = self.model.formulas[self.formula_cursor].target.clone();
command::dispatch(&mut self.model, &Command::RemoveFormula { target });
let f = &self.model.formulas[self.formula_cursor];
let target = f.target.clone();
let target_category = f.target_category.clone();
command::dispatch(&mut self.model, &Command::RemoveFormula { target, target_category });
if self.formula_cursor > 0 { self.formula_cursor -= 1; }
self.dirty = true;
}
@ -915,14 +917,10 @@ impl App {
// ── Motion helpers ───────────────────────────────────────────────────────
fn move_selection(&mut self, dr: i32, dc: i32) {
// Use cross-product counts so multi-category axes navigate correctly.
let (row_max, col_max) = {
let view = match self.model.active_view() { Some(v) => v, None => return };
let row_cats: Vec<String> = view.categories_on(Axis::Row).into_iter().map(String::from).collect();
let col_cats: Vec<String> = view.categories_on(Axis::Column).into_iter().map(String::from).collect();
let rm = cross_product_strs(&row_cats, &self.model, view).len().saturating_sub(1);
let cm = cross_product_strs(&col_cats, &self.model, view).len().saturating_sub(1);
(rm, cm)
let layout = GridLayout::new(&self.model, view);
(layout.row_count().saturating_sub(1), layout.col_count().saturating_sub(1))
};
if let Some(view) = self.model.active_view_mut() {
@ -941,8 +939,7 @@ impl App {
fn jump_to_last_row(&mut self) {
let count = {
let view = match self.model.active_view() { Some(v) => v, None => return };
let row_cats: Vec<String> = view.categories_on(Axis::Row).into_iter().map(String::from).collect();
cross_product_strs(&row_cats, &self.model, view).len().saturating_sub(1)
GridLayout::new(&self.model, view).row_count().saturating_sub(1)
};
if let Some(view) = self.model.active_view_mut() {
view.selected.0 = count;
@ -953,8 +950,7 @@ impl App {
fn jump_to_last_col(&mut self) {
let count = {
let view = match self.model.active_view() { Some(v) => v, None => return };
let col_cats: Vec<String> = view.categories_on(Axis::Column).into_iter().map(String::from).collect();
cross_product_strs(&col_cats, &self.model, view).len().saturating_sub(1)
GridLayout::new(&self.model, view).col_count().saturating_sub(1)
};
if let Some(view) = self.model.active_view_mut() {
view.selected.1 = count;
@ -965,8 +961,7 @@ impl App {
fn scroll_rows(&mut self, delta: i32) {
let row_max = {
let view = match self.model.active_view() { Some(v) => v, None => return };
let row_cats: Vec<String> = view.categories_on(Axis::Row).into_iter().map(String::from).collect();
cross_product_strs(&row_cats, &self.model, view).len().saturating_sub(1)
GridLayout::new(&self.model, view).row_count().saturating_sub(1)
};
if let Some(view) = self.model.active_view_mut() {
let nr = (view.selected.0 as i32 + delta).clamp(0, row_max as i32) as usize;
@ -986,45 +981,21 @@ impl App {
Some(v) => v,
None => return,
};
let row_cats: Vec<String> = view.categories_on(Axis::Row).into_iter().map(String::from).collect();
let col_cats: Vec<String> = view.categories_on(Axis::Column).into_iter().map(String::from).collect();
let page_cats: Vec<String> = view.categories_on(Axis::Page).into_iter().map(String::from).collect();
let layout = GridLayout::new(&self.model, view);
let (cur_row, cur_col) = view.selected;
// Build cross-product for rows and cols (inline, avoids circular dep with grid.rs)
let row_items: Vec<Vec<String>> = cross_product_strs(&row_cats, &self.model, view);
let col_items: Vec<Vec<String>> = cross_product_strs(&col_cats, &self.model, view);
let page_coords: Vec<(String, String)> = page_cats.iter().map(|cat| {
let items: Vec<String> = self.model.category(cat)
.map(|c| c.ordered_item_names().into_iter().map(String::from).collect())
.unwrap_or_default();
let sel = view.page_selection(cat)
.map(String::from)
.or_else(|| items.first().cloned())
.unwrap_or_default();
(cat.clone(), sel)
}).collect();
// Enumerate all (row_idx, col_idx) grid positions
let total_rows = row_items.len().max(1);
let total_cols = col_items.len().max(1);
let total_rows = layout.row_count().max(1);
let total_cols = layout.col_count().max(1);
let total = total_rows * total_cols;
let cur_flat = cur_row * total_cols + cur_col;
let matches: Vec<usize> = (0..total).filter(|&flat| {
let ri = flat / total_cols;
let ci = flat % total_cols;
let row_item = row_items.get(ri).cloned().unwrap_or_default();
let col_item = col_items.get(ci).cloned().unwrap_or_default();
let mut coords = page_coords.clone();
for (cat, item) in row_cats.iter().zip(row_item.iter()) {
coords.push((cat.clone(), item.clone()));
}
for (cat, item) in col_cats.iter().zip(col_item.iter()) {
coords.push((cat.clone(), item.clone()));
}
let key = CellKey::new(coords);
let key = match layout.cell_key(ri, ci) {
Some(k) => k,
None => return false,
};
let val = self.model.evaluate(&key);
let s = match &val {
CellValue::Number(n) => format!("{n}"),
@ -1130,36 +1101,8 @@ impl App {
pub fn selected_cell_key(&self) -> Option<CellKey> {
let view = self.model.active_view()?;
let row_cats: Vec<String> = view.categories_on(Axis::Row).into_iter().map(String::from).collect();
let col_cats: Vec<String> = view.categories_on(Axis::Column).into_iter().map(String::from).collect();
let page_cats: Vec<String> = view.categories_on(Axis::Page).into_iter().map(String::from).collect();
let (sel_row, sel_col) = view.selected;
let mut coords = vec![];
for cat_name in &page_cats {
let items = self.model.category(cat_name)
.map(|c| c.ordered_item_names().into_iter().map(String::from).collect::<Vec<_>>())
.unwrap_or_default();
let sel = view.page_selection(cat_name)
.map(String::from)
.or_else(|| items.first().cloned())?;
coords.push((cat_name.clone(), sel));
}
// Use cross-product indexing so multi-category axes resolve correctly.
let row_items = cross_product_strs(&row_cats, &self.model, view);
let row_item = row_items.get(sel_row)?;
for (cat, item) in row_cats.iter().zip(row_item.iter()) {
coords.push((cat.clone(), item.clone()));
}
let col_items = cross_product_strs(&col_cats, &self.model, view);
let col_item = col_items.get(sel_col)?;
for (cat, item) in col_cats.iter().zip(col_item.iter()) {
coords.push((cat.clone(), item.clone()));
}
Some(CellKey::new(coords))
GridLayout::new(&self.model, view).cell_key(sel_row, sel_col)
}
// ── Persistence ──────────────────────────────────────────────────────────
@ -1209,26 +1152,3 @@ impl App {
}
}
/// Compute the Cartesian product of items from `cats` in the active view,
/// filtering hidden items. Returns `vec![vec![]]` when `cats` is empty.
fn cross_product_strs(cats: &[String], model: &crate::model::Model, view: &crate::view::View) -> Vec<Vec<String>> {
if cats.is_empty() {
return vec![vec![]];
}
let mut result: Vec<Vec<String>> = vec![vec![]];
for cat_name in cats {
let items: Vec<String> = model.category(cat_name)
.map(|c| c.ordered_item_names().into_iter()
.filter(|item| !view.is_hidden(cat_name, item))
.map(String::from).collect())
.unwrap_or_default();
result = result.into_iter().flat_map(|prefix| {
items.iter().map(move |item| {
let mut row = prefix.clone();
row.push(item.clone());
row
})
}).collect();
}
result
}