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

refactor: break Model↔View cycle, introduce Workbook wrapper

Browse Source 
Model is now pure data (categories, cells, formulas, measure_agg) with
no references to view/. The Workbook struct owns the Model together
with views and the active view name, and is responsible for cross-slice
operations (add/remove category → notify views, view management).

- New: src/workbook.rs with Workbook wrapper and cross-slice helpers
  (add_category, add_label_category, remove_category, create_view,
  switch_view, delete_view, normalize_view_state).
- Model: strip view state and view-touching methods. recompute_formulas
  remains on Model as a primitive; the view-derived none_cats list is
  gathered at each call site (App::rebuild_layout, persistence::load)
  so the view dependency is explicit, not hidden behind a wrapper.
- View: add View::none_cats() helper.
- CmdContext: add workbook and view fields so commands can reach both
  slices without threading Model + View through every call.
- App: rename `model` field to `workbook`.
- Persistence (save/load/format_md/parse_md/export_csv): take/return
  Workbook so the on-disk format carries model + views together.
- Widgets (GridWidget, TileBar, CategoryContent, ViewContent): take
  explicit &Model + &View instead of routing through Model.

Tests updated throughout to reflect the new shape. View-management
tests that previously lived on Model continue to cover the same
behaviour via a build_workbook() helper in model/types.rs. All 573
tests pass; clippy is clean.

This is Phase A of improvise-36h. Phase B will mechanically extract
crates/improvise-core/ containing model/, view/, format.rs, workbook.rs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-04-15 21:08:11 -07:00
parent f02d905aac
commit 3fbf56ec8b
26 changed files with 1271 additions and 972 deletions
Download Patch File Download Diff File Expand all files Collapse all files

283
src/model/types.rs
View File

@ -7,18 +7,24 @@ use serde::{Deserialize, Serialize};
use super::category::{Category, CategoryId};
use super::cell::{CellKey, CellValue, DataStore};
use crate::formula::{AggFunc, Formula};
use crate::view::View;
const MAX_CATEGORIES: usize = 12;
/// Pure-data document model: categories, cells, and formulas.
///
/// `Model` intentionally does **not** know about views. The view axes and
/// per-view state live in [`crate::workbook::Workbook`], which wraps a
/// `Model` with the view ensemble. Cross-slice operations — adding a
/// category and registering it on every view, for example — are therefore
/// methods on `Workbook`, not `Model`. This breaks the former `Model ↔ View`
/// cycle so the `model/` and `view/` modules can be lifted into a shared
/// `improvise-core` crate without pulling view code into pure data types.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Model {
pub name: String,
pub categories: IndexMap<String, Category>,
pub data: DataStore,
formulas: Vec<Formula>,
pub views: IndexMap<String, View>,
pub active_view: String,
next_category_id: CategoryId,
/// Per-measure aggregation function (measure item name → agg func).
/// Used when collapsing categories on `Axis::None`. Defaults to SUM.
@ -33,11 +39,8 @@ impl Model {
pub fn new(name: impl Into<String>) -> Self {
use crate::model::category::CategoryKind;
let name = name.into();
let default_view = View::new("Default");
let mut views = IndexMap::new();
views.insert("Default".to_string(), default_view);
let mut categories = IndexMap::new();
// Virtual categories — always present, default to Axis::None
// Virtual categories — always present.
categories.insert(
"_Index".to_string(),
Category::new(0, "_Index").with_kind(CategoryKind::VirtualIndex),
@ -50,30 +53,20 @@ impl Model {
"_Measure".to_string(),
Category::new(2, "_Measure").with_kind(CategoryKind::VirtualMeasure),
);
let mut m = Self {
Self {
name,
categories,
data: DataStore::new(),
formulas: Vec::new(),
views,
active_view: "Default".to_string(),
next_category_id: 3,
measure_agg: HashMap::new(),
formula_cache: HashMap::new(),
};
// Add virtuals to existing views (default view).
// Start in records mode; on_category_added will reclaim Row/Column
// for the first two regular categories.
for view in m.views.values_mut() {
view.on_category_added("_Index");
view.on_category_added("_Dim");
view.on_category_added("_Measure");
view.set_axis("_Index", crate::view::Axis::Row);
view.set_axis("_Dim", crate::view::Axis::Column);
}
m
}
/// Add a pivot category. Enforces the `MAX_CATEGORIES` limit for regular
/// categories. The caller (typically [`crate::workbook::Workbook`]) is
/// responsible for registering the new category on every view.
pub fn add_category(&mut self, name: impl Into<String>) -> Result<CategoryId> {
let name = name.into();
// Only regular pivot categories count against the limit.
@ -92,19 +85,15 @@ impl Model {
self.next_category_id += 1;
self.categories
.insert(name.clone(), Category::new(id, name.clone()));
// Add to all views
for view in self.views.values_mut() {
view.on_category_added(&name);
}
Ok(id)
}
/// Add a Label-kind category: stored alongside regular categories so
/// records views can display it, but default to `Axis::None` and
/// excluded from the pivot-category count limit.
/// records views can display it, but excluded from the pivot-category
/// count limit. The caller is responsible for setting the view axis
/// (typically to `Axis::None`).
pub fn add_label_category(&mut self, name: impl Into<String>) -> Result<CategoryId> {
use crate::model::category::CategoryKind;
use crate::view::Axis;
let name = name.into();
if self.categories.contains_key(&name) {
return Ok(self.categories[&name].id);
@ -113,23 +102,17 @@ impl Model {
self.next_category_id += 1;
let cat = Category::new(id, name.clone()).with_kind(CategoryKind::Label);
self.categories.insert(name.clone(), cat);
for view in self.views.values_mut() {
view.on_category_added(&name);
view.set_axis(&name, Axis::None);
}
Ok(id)
}
/// Remove a category and all cells that reference it.
/// Remove a category and all cells that reference it. The caller is
/// responsible for removing the category from any views that referenced
/// it.
pub fn remove_category(&mut self, name: &str) {
if !self.categories.contains_key(name) {
return;
}
self.categories.shift_remove(name);
// Remove from all views
for view in self.views.values_mut() {
view.on_category_removed(name);
}
// Remove cells that have a coord in this category
let to_remove: Vec<CellKey> = self
.data
@ -208,59 +191,6 @@ impl Model {
&self.formulas
}
pub fn active_view(&self) -> &View {
self.views
.get(&self.active_view)
.expect("active_view always names an existing view")
}
pub fn active_view_mut(&mut self) -> &mut View {
self.views
.get_mut(&self.active_view)
.expect("active_view always names an existing view")
}
pub fn create_view(&mut self, name: impl Into<String>) -> &mut View {
let name = name.into();
let mut view = View::new(name.clone());
// Copy category assignments from default if any
for cat_name in self.categories.keys() {
view.on_category_added(cat_name);
}
self.views.insert(name.clone(), view);
self.views.get_mut(&name).unwrap()
}
pub fn switch_view(&mut self, name: &str) -> Result<()> {
if self.views.contains_key(name) {
self.active_view = name.to_string();
Ok(())
} else {
Err(anyhow!("View '{name}' not found"))
}
}
pub fn delete_view(&mut self, name: &str) -> Result<()> {
if self.views.len() <= 1 {
return Err(anyhow!("Cannot delete the last view"));
}
self.views.shift_remove(name);
if self.active_view == name {
self.active_view = self.views.keys().next().unwrap().clone();
}
Ok(())
}
/// Reset all view scroll offsets to zero.
/// Call this after loading or replacing a model so stale offsets don't
/// cause the grid to render an empty area.
pub fn normalize_view_state(&mut self) {
for view in self.views.values_mut() {
view.row_offset = 0;
view.col_offset = 0;
}
}
/// Return all category names
/// Names of all categories (including virtual ones).
pub fn category_names(&self) -> Vec<&str> {
@ -848,7 +778,6 @@ impl Model {
mod model_tests {
use super::Model;
use crate::model::cell::{CellKey, CellValue};
use crate::view::Axis;
fn coord(pairs: &[(&str, &str)]) -> CellKey {
CellKey::new(
@ -859,13 +788,6 @@ mod model_tests {
)
}
#[test]
fn new_model_has_default_view() {
let m = Model::new("Test");
// active_view() panics if missing; this test just ensures it doesn't panic
let _ = m.active_view();
}
#[test]
fn add_category_creates_entry() {
let mut m = Model::new("Test");
@ -892,14 +814,6 @@ mod model_tests {
assert!(m.add_category("TooMany").is_err());
}
#[test]
fn add_category_notifies_existing_views() {
let mut m = Model::new("Test");
m.add_category("Region").unwrap();
// axis_of panics for unknown categories; not panicking here confirms it was registered
let _ = m.active_view().axis_of("Region");
}
#[test]
fn set_and_get_cell_roundtrip() {
let mut m = Model::new("Test");
@ -981,80 +895,6 @@ mod model_tests {
0,
"all cells with Region coord should be removed"
);
// Views should no longer know about Region
// (axis_of would panic for unknown category, so check categories_on)
let v = m.active_view();
assert!(v.categories_on(crate::view::Axis::Row).is_empty());
}
#[test]
fn create_view_copies_category_structure() {
let mut m = Model::new("Test");
m.add_category("Region").unwrap();
m.add_category("Product").unwrap();
m.create_view("Secondary");
let v = m.views.get("Secondary").unwrap();
// axis_of panics for unknown categories; not panicking confirms categories were registered
let _ = v.axis_of("Region");
let _ = v.axis_of("Product");
}
#[test]
fn switch_view_changes_active_view() {
let mut m = Model::new("Test");
m.create_view("Other");
m.switch_view("Other").unwrap();
assert_eq!(m.active_view, "Other");
}
#[test]
fn switch_view_unknown_returns_error() {
let mut m = Model::new("Test");
assert!(m.switch_view("NoSuchView").is_err());
}
#[test]
fn delete_view_removes_it() {
let mut m = Model::new("Test");
m.create_view("Extra");
m.delete_view("Extra").unwrap();
assert!(!m.views.contains_key("Extra"));
}
#[test]
fn delete_last_view_returns_error() {
let mut m = Model::new("Test");
assert!(m.delete_view("Default").is_err());
}
#[test]
fn delete_active_view_switches_to_another() {
let mut m = Model::new("Test");
m.create_view("Other");
m.switch_view("Other").unwrap();
m.delete_view("Other").unwrap();
assert_ne!(m.active_view, "Other");
}
#[test]
fn first_category_goes_to_row_second_to_column_rest_to_page() {
let mut m = Model::new("Test");
m.add_category("Region").unwrap();
m.add_category("Product").unwrap();
m.add_category("Time").unwrap();
let v = m.active_view();
assert_eq!(v.axis_of("Region"), Axis::Row);
assert_eq!(v.axis_of("Product"), Axis::Column);
assert_eq!(v.axis_of("Time"), Axis::Page);
}
#[test]
fn data_is_shared_across_views() {
let mut m = Model::new("Test");
m.create_view("Second");
let k = coord(&[("Region", "East")]);
m.set_cell(k.clone(), CellValue::Number(77.0));
assert_eq!(m.get_cell(&k), Some(&CellValue::Number(77.0)));
}
#[test]
@ -1702,6 +1542,7 @@ mod five_category {
use crate::formula::parse_formula;
use crate::model::cell::{CellKey, CellValue};
use crate::view::Axis;
use crate::workbook::Workbook;
const DATA: &[(&str, &str, &str, &str, f64, f64)] = &[
("East", "Shirts", "Online", "Q1", 1_000.0, 600.0),
@ -1772,6 +1613,52 @@ mod five_category {
m
}
/// Build a Workbook whose model matches `build_model()`. Used by the
/// view-management tests in this module: view state lives on Workbook,
/// not Model, so those tests need the wrapper.
fn build_workbook() -> Workbook {
let mut wb = Workbook::new("Sales");
for cat in ["Region", "Product", "Channel", "Time", "_Measure"] {
wb.add_category(cat).unwrap();
}
for cat in ["Region", "Product", "Channel", "Time"] {
let items: &[&str] = match cat {
"Region" => &["East", "West"],
"Product" => &["Shirts", "Pants"],
"Channel" => &["Online", "Retail"],
"Time" => &["Q1", "Q2"],
_ => &[],
};
if let Some(c) = wb.model.category_mut(cat) {
for &item in items {
c.add_item(item);
}
}
}
if let Some(c) = wb.model.category_mut("_Measure") {
for &item in &["Revenue", "Cost", "Profit", "Margin", "Total"] {
c.add_item(item);
}
}
for &(region, product, channel, time, rev, cost) in DATA {
wb.model.set_cell(
coord(region, product, channel, time, "Revenue"),
CellValue::Number(rev),
);
wb.model.set_cell(
coord(region, product, channel, time, "Cost"),
CellValue::Number(cost),
);
}
wb.model
.add_formula(parse_formula("Profit = Revenue - Cost", "_Measure").unwrap());
wb.model
.add_formula(parse_formula("Margin = Profit / Revenue", "_Measure").unwrap());
wb.model
.add_formula(parse_formula("Total = SUM(Revenue)", "_Measure").unwrap());
wb
}
fn approx(a: f64, b: f64) -> bool {
(a - b).abs() < 1e-9
}
@ -1938,8 +1825,8 @@ mod five_category {
#[test]
fn default_view_first_two_on_axes_rest_on_page() {
let m = build_model();
let v = m.active_view();
let wb = build_workbook();
let v = wb.active_view();
assert_eq!(v.axis_of("Region"), Axis::Row);
assert_eq!(v.axis_of("Product"), Axis::Column);
assert_eq!(v.axis_of("Channel"), Axis::Page);
@ -1949,9 +1836,9 @@ mod five_category {
#[test]
fn rearranging_axes_does_not_affect_data() {
let mut m = build_model();
let mut wb = build_workbook();
{
let v = m.active_view_mut();
let v = wb.active_view_mut();
v.set_axis("Region", Axis::Page);
v.set_axis("Product", Axis::Page);
v.set_axis("Channel", Axis::Row);
@ -1959,44 +1846,48 @@ mod five_category {
v.set_axis("_Measure", Axis::Page);
}
assert_eq!(
m.get_cell(&coord("East", "Shirts", "Online", "Q1", "Revenue")),
wb.model
.get_cell(&coord("East", "Shirts", "Online", "Q1", "Revenue")),
Some(&CellValue::Number(1_000.0))
);
}
#[test]
fn two_views_have_independent_axis_assignments() {
let mut m = build_model();
m.create_view("Pivot");
let mut wb = build_workbook();
wb.create_view("Pivot");
{
let v = m.views.get_mut("Pivot").unwrap();
let v = wb.views.get_mut("Pivot").unwrap();
v.set_axis("Time", Axis::Row);
v.set_axis("Channel", Axis::Column);
v.set_axis("Region", Axis::Page);
v.set_axis("Product", Axis::Page);
v.set_axis("_Measure", Axis::Page);
}
assert_eq!(m.views.get("Default").unwrap().axis_of("Region"), Axis::Row);
assert_eq!(m.views.get("Pivot").unwrap().axis_of("Time"), Axis::Row);
assert_eq!(
m.views.get("Pivot").unwrap().axis_of("Channel"),
wb.views.get("Default").unwrap().axis_of("Region"),
Axis::Row
);
assert_eq!(wb.views.get("Pivot").unwrap().axis_of("Time"), Axis::Row);
assert_eq!(
wb.views.get("Pivot").unwrap().axis_of("Channel"),
Axis::Column
);
}
#[test]
fn page_selections_are_per_view() {
let mut m = build_model();
m.create_view("West only");
if let Some(v) = m.views.get_mut("West only") {
let mut wb = build_workbook();
wb.create_view("West only");
if let Some(v) = wb.views.get_mut("West only") {
v.set_page_selection("Region", "West");
}
assert_eq!(
m.views.get("Default").unwrap().page_selection("Region"),
wb.views.get("Default").unwrap().page_selection("Region"),
None
);
assert_eq!(
m.views.get("West only").unwrap().page_selection("Region"),
wb.views.get("West only").unwrap().page_selection("Region"),
Some("West")
);
}