feat(records): a +cat column, so a schema can be built without leaving the grid

improvise-2zq step 2, on the user's ruling. Step 1 made this coherent by
stopping `on_category_added` from ejecting the user; this adds the column.

Typing a name into `+cat` creates that category, and because
`GridLayout::records_columns` filters on `kind.is_regular()` and never
looks at items or at an axis, the coordinate column appears immediately
and is typeable on the next keystroke.

**The two questions the ruling left open, and the answers.**

*Where the column goes.* Not a second trailing column, and not one `+`
disambiguated by a prefix. A prefix would put a sigil into the name space
— and a category and a measure may legitimately share a name, which is
the exact ambiguity `_Dim` already carries a column *index* rather than a
label to avoid (gotcha 1). A key was rejected too: the name still has to
be typed somewhere, and a minibuffer lands in the records-exit class bug
this PR characterizes but does not fix.

So each `+` sits adjacent to what it adds to — `[coords…, +cat,
measures…, +]`. Position tells the two apart instead of a label, which is
the clutter the ticket was worried about. It costs a mid-grid column
index, and the consequence worth naming is that a **blank** records grid
now opens with the cursor on `+cat` rather than `+`: a record needs
coordinates before it needs measures, so that order is defensible, but it
does change what the first keystroke in an empty grid does.

*Whether committing also sets the coordinate on the current row.* It does
not. It is symmetric with the measure `+`, which creates an empty column
and writes no value; and setting it would move **every** measure cell in
the row onto the new coordinate (gotcha 1) — a data migration across N
cells attached to the same Enter that made a schema change, in an app
with no undo. The follow-up edit is one column away.

**The refusal is visible, which took a change of order.** `MAX_CATEGORIES`
is reachable here rather than theoretical. `AddCategory` now aborts the
batch on refusal — same reason a failed save does (gotcha 21): what
follows assumes the category exists, and the trailing `EnterEditAtCursor`
would re-open the minibuffer *over* the message, since `draw_bottom_bar`
renders the minibuffer instead of the status line. And
`CommitAndAdvance` now emits its mode change **before** the commit rather
than after, so an abort cannot eat it. That is the third time this PR has
applied one rule — tidy-up goes where an abort cannot reach it — and the
comment at that site had already described the pattern for the *advance*
failing; the commit failing was simply not considered. The limit itself
is still checked in exactly one place, `Model::add_category`.

Evidence. Three new tests drive real keys:
`the_new_category_column_creates_a_category_and_its_column` (the column
is usable immediately), `..._writes_no_coordinate` (the deliberate
answer to question 2, asserted over every row), and
`..._refuses_past_the_limit_where_the_user_can_see_it`, which asserts the
mode has **no minibuffer** — proved discriminating by swapping the commit
back in front of the mode change and watching it report
"Maximum of 12 categories reached" from inside `RecordsEditing`, where
nothing would have drawn it.

Four existing characterization tests were updated knowingly, not
incidentally: the shared records fixture's column list (which accounted
for 8 of the 9 initial failures on its own), the core layout and
persistence smoke expectations, and the blank-model bootstrap, which now
has to step to the `+` column and so records the cursor-position
consequence above.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-19 23:10:33 -07:00
parent 8173b87723
commit 6a929c1d21
7 changed files with 288 additions and 36 deletions
@@ -343,6 +343,7 @@ fn commit_plain_records_edit(
commit_regular_cell_value(&row.cell_key(measure), value, effects);
}
RecordCol::NewMeasure => add_measure(value, effects),
RecordCol::NewCategory => add_category(value, effects),
}
}
@@ -361,6 +362,29 @@ fn add_measure(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
effects.push(effect::mark_dirty());
}
/// Typing a name into the `+cat` column creates that category, which gives
/// the row a coordinate column to type into (improvise-2zq).
///
/// It creates the column and **does not set the coordinate on the current
/// row**, which is the deliberate answer to the ticket's third question.
/// Two reasons. It is symmetric with the measure `+`, which creates an empty
/// column and writes no value. And setting it would move *every* measure
/// cell in the row onto the new coordinate (gotcha 1) — a data migration
/// across N cells attached to the same Enter that made a schema change, in
/// an app with no undo. The follow-up edit is one `l` away in the column
/// that just appeared.
///
/// Like `add_measure` this is a schema change, so it applies immediately
/// even inside a drill rather than being staged.
fn add_category(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
let name = value.trim();
if name.is_empty() {
return;
}
effects.push(Box::new(effect::AddCategory(name.to_string())));
effects.push(effect::mark_dirty());
}
fn commit_cell_value(
ctx: &CmdContext,
key: &CellKey,
@@ -372,8 +396,10 @@ fn commit_cell_value(
return;
};
match (&col, ctx.has_drill_state) {
// A new measure is a schema change, never a staged cell edit.
// A new measure or category is a schema change, never a staged
// cell edit: there is no cell to roll back with the drill.
(RecordCol::NewMeasure, _) => add_measure(value, effects),
(RecordCol::NewCategory, _) => add_category(value, effects),
(_, true) => effects.push(stage_drill_edit(record_idx, col, value)),
(_, false) => commit_plain_records_edit(ctx, record_idx, &col, value, effects),
}
@@ -426,14 +452,22 @@ impl Cmd for CommitAndAdvance {
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
commit_cell_value(ctx, &self.key, &self.value, &mut effects);
// Pre-emptively drop to the normal counterpart of edit_mode. If the
// advance succeeds, the trailing `EnterEditAtCursor` below will lift
// us back into editing on the new cell. If the advance aborts
// (e.g. already at bottom-right on Enter), `EnterEditAtCursor` is
// skipped and we land in normal mode — which is the desired
// "Enter at bottom-right commits and exits" behavior.
//
// It goes **before** the commit, not after (improvise-2zq). The
// commit can abort the batch itself — `AddCategory` does so when the
// document is at `MAX_CATEGORIES` — and an abort before this ran
// left the app in the editing mode, where `draw_bottom_bar` renders
// the minibuffer instead of the status line and the refusal the
// effect had just written was never drawn. Tidy-up goes where an
// abort cannot reach it; same rule as `ExecuteCommand`'s.
effects.push(effect::change_mode(exit_mode_for(&self.edit_mode)));
commit_cell_value(ctx, &self.key, &self.value, &mut effects);
match self.advance {
AdvanceDir::Down => {
let adv = EnterAdvance {
+189 -13
View File
@@ -1391,6 +1391,155 @@ mod tests {
);
}
/// improvise-2zq step 2: the `+cat` column creates a category and its
/// column, without leaving the grid.
///
/// Driven through real keys. The assertion that earns its place is the
/// last one: the new column is **usable immediately**, which is the whole
/// point — `records_columns` filters on `kind.is_regular()` and never
/// requires items, so a category with no items is a column the moment it
/// exists.
#[test]
fn the_new_category_column_creates_a_category_and_its_column() {
let mut app = records_model_with_two_rows();
let plus_cat = (0..app.layout.col_count())
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
.expect("the records grid offers a +cat column");
app.cursor_mut().selected = (0, plus_cat);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
for c in "Product".chars() {
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
.unwrap();
}
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
.unwrap();
app.rebuild_layout();
assert!(
app.model_state.workbook.model.category("Product").is_some(),
"the category is created"
);
assert!(
app.layout.is_records_mode(),
"and creating it does not eject the user (step 1)"
);
let cols: Vec<String> = (0..app.layout.col_count())
.map(|i| app.layout.col_label(i))
.collect();
assert_eq!(
cols,
vec![
"Region",
"Product",
crate::view::NEW_CATEGORY_COL,
"meas1",
"meas2",
crate::view::NEW_MEASURE_COL
],
"the new coordinate column appears immediately, before the \
still-available +cat, got {cols:?}"
);
}
/// Committing the `+cat` name creates the **column** and does not write
/// the coordinate on the row the cursor was on.
///
/// The deliberate answer to the ticket's third question, and the reason
/// is gotcha 1: setting a coordinate moves *every* measure cell in the
/// row onto it. That is a data migration across N cells, and attaching it
/// to the same Enter that made a schema change — in an app with no undo —
/// is a bigger single keystroke than the measure `+` asks for. The
/// follow-up edit is one column away.
#[test]
fn the_new_category_column_creates_the_column_but_writes_no_coordinate() {
let mut app = records_model_with_two_rows();
let plus_cat = (0..app.layout.col_count())
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
.expect("+cat column");
app.cursor_mut().selected = (0, plus_cat);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
for c in "Product".chars() {
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
.unwrap();
}
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
.unwrap();
app.rebuild_layout();
let product_col = (0..app.layout.col_count())
.find(|i| app.layout.col_label(*i) == "Product")
.expect("the new column exists");
for row in 0..app.layout.row_count() {
assert_eq!(
app.layout.records_display(row, product_col).as_deref(),
Some(""),
"row {row} must have no coordinate for a category just created"
);
}
}
/// `MAX_CATEGORIES` is a reachable refusal on this path, and the refusal
/// has to be **visible**.
///
/// `draw_bottom_bar` renders the minibuffer instead of the status line,
/// so a message written while an editor is open is never drawn. The
/// commit path therefore drops out of editing *before* running the add,
/// and `AddCategory` aborts the batch on refusal so the trailing
/// `EnterEditAtCursor` cannot re-open the editor over the message.
///
/// The limit itself is checked in exactly one place — `Model::add_category`
/// — and nothing here restates it; this fills the document to the limit
/// by asking for categories until it refuses.
#[test]
fn the_new_category_column_refuses_past_the_limit_where_the_user_can_see_it() {
let mut app = records_model_with_two_rows();
let mut n = 0;
while app
.model_state
.workbook
.add_category(format!("Filler{n}"))
.is_ok()
{
n += 1;
assert!(n < 100, "add_category must refuse eventually");
}
app.rebuild_layout();
app.view_state.status_msg.clear();
let plus_cat = (0..app.layout.col_count())
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
.expect("+cat column");
app.cursor_mut().selected = (0, plus_cat);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
for c in "TooMany".chars() {
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
.unwrap();
}
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
.unwrap();
assert!(
app.model_state.workbook.model.category("TooMany").is_none(),
"the category is refused"
);
assert!(
!app.view_state.status_msg.is_empty(),
"the refusal is reported"
);
assert!(
app.view_state.mode.minibuffer().is_none(),
"and reported where it is drawn: a mode with a minibuffer hides \
the status line, so the user would see nothing at all. mode is \
{:?}, message {:?}",
app.view_state.mode,
app.view_state.status_msg
);
}
/// improvise-2zq step 1: naming a category in records mode used to eject
/// the user from the grid they were standing in.
///
@@ -1545,10 +1694,16 @@ mod tests {
);
}
/// A blank model has no measures, so wide format shows only the trailing
/// new-measure column. Typing a name there creates the measure, which
/// gives it a real column to type values into — records mode stays
/// self-sufficient for onboarding (improvise-c5v).
/// A blank model has no categories and no measures, so wide format shows
/// exactly the two creation columns. Typing a name into the new-measure
/// one creates the measure, which gives it a real column to type values
/// into — records mode stays self-sufficient for onboarding
/// (improvise-c5v).
///
/// improvise-2zq widened that: a blank grid now offers `+cat` beside `+`,
/// so both halves of a schema can be bootstrapped without leaving the
/// grid. This test still pins the measure half; the category half is
/// `the_new_category_column_creates_a_category_and_its_column`.
#[test]
fn blank_model_bootstraps_its_first_measure_from_the_new_measure_column() {
use crate::model::cell::CellKey;
@@ -1561,8 +1716,8 @@ mod tests {
.collect();
assert_eq!(
cols,
vec![crate::view::NEW_MEASURE_COL],
"a blank model offers only the new-measure column"
vec![crate::view::NEW_CATEGORY_COL, crate::view::NEW_MEASURE_COL],
"a blank model offers exactly the two creation columns"
);
// `o` seeds a row and opens an edit on it.
@@ -1570,7 +1725,17 @@ mod tests {
.unwrap();
assert_eq!(app.layout.row_count(), 1);
// Naming a measure in the + column creates it.
// The cursor lands at column 0, which in a blank grid is now `+cat`
// rather than `+` (improvise-2zq) — a record needs coordinates
// before it needs measures, so that order is deliberate. Move to the
// new-measure column to exercise the measure half.
// `o` left us editing column 0, so leave that edit before moving.
app.handle_key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE))
.unwrap();
let new_measure_col = app.layout.col_count() - 1;
app.cursor_mut().selected = (0, new_measure_col);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
for c in "Rev".chars() {
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
.unwrap();
@@ -1583,12 +1748,17 @@ mod tests {
.collect();
assert_eq!(
cols,
vec!["Rev", crate::view::NEW_MEASURE_COL],
"the named measure becomes a real column"
vec![
crate::view::NEW_CATEGORY_COL,
"Rev",
crate::view::NEW_MEASURE_COL
],
"the named measure becomes a real column, between the two \
creation columns"
);
// Typing into that column writes the value.
let rev_col = 0;
let rev_col = 1;
app.cursor_mut().selected = (0, rev_col);
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
.unwrap();
@@ -1650,9 +1820,15 @@ mod tests {
.collect();
assert_eq!(
cols,
vec!["Region", "meas1", "meas2", crate::view::NEW_MEASURE_COL],
"setup: category column, one column per measure, then the \
new-measure column"
vec![
"Region",
crate::view::NEW_CATEGORY_COL,
"meas1",
"meas2",
crate::view::NEW_MEASURE_COL
],
"setup: category columns, the new-category column, one column \
per measure, then the new-measure column (improvise-2zq)"
);
app
}
+15 -2
View File
@@ -95,8 +95,20 @@ macro_rules! simple_effect {
pub struct AddCategory(pub String);
impl Effect for AddCategory {
fn apply(&self, app: &mut App) {
// A refusal aborts the batch, for the same reason a failed save does
// (gotcha 21): what follows assumes the category exists. From the
// `+cat` column (improvise-2zq) the rest of the batch would advance
// the cursor and re-open an editor on a column that was never
// created — and re-opening the minibuffer is also what would *hide*
// this message, since `draw_bottom_bar` renders the minibuffer
// instead of the status line.
//
// `MAX_CATEGORIES` is a reachable refusal on that path rather than a
// theoretical one, and the limit is checked in exactly one place —
// `Model::add_category` — so nothing here restates it.
if let Err(e) = app.model_state.workbook.add_category(&self.0) {
app.view_state.status_msg = e.to_string();
app.abort_effects = true;
}
}
fn kind(&self) -> EffectKind {
@@ -658,8 +670,9 @@ impl Effect for ApplyAndClearDrill {
apply_coordinate_rename(app, row, category, new_value);
}
// Never staged — `commit_cell_value` applies a new measure
// immediately, since it is a schema change and not a cell.
RecordCol::NewMeasure => {}
// or category immediately, since a schema change is not a
// cell edit and has nothing to roll back with the drill.
RecordCol::NewMeasure | RecordCol::NewCategory => {}
}
}
app.model_state.dirty = true;
+6 -4
View File
@@ -315,10 +315,12 @@ fn cell_id(app: &App, row: usize, col: usize) -> Option<CellId> {
RecordCol::Measure(measure) => Some(measure.clone()),
// Not cells to write into. Editing a category column renames a
// coordinate across every measure cell in the row (gotcha 1) and
// the `+` column creates a measure; both are different intents
// from `SetCell`, so neither gets an id it could be composed
// with.
RecordCol::Category(_) | RecordCol::NewMeasure => return None,
// the `+`/`+cat` columns create a measure or a category; all are
// different intents from `SetCell`, so none gets an id it could
// be composed with.
RecordCol::Category(_) | RecordCol::NewMeasure | RecordCol::NewCategory => {
return None;
}
};
return Some(CellId::Record(RecordCell {
record: RecordHash::of(record),
+34 -10
View File
@@ -10,6 +10,14 @@ use crate::view::{Axis, View};
/// measure column (improvise-c5v).
pub const NEW_MEASURE_COL: &str = "+";
/// The label of the column that creates a new **category** (improvise-2zq).
///
/// It sits at the end of the category block rather than trailing the whole
/// grid, so each `+` is adjacent to what it adds to: `+cat` after the
/// coordinates, `+` after the measures. Two trailing columns would have made
/// the user read a label to tell them apart; position says it instead.
pub const NEW_CATEGORY_COL: &str = "+cat";
/// Extract (record_index, column_index) from a synthetic records-mode
/// CellKey. Returns None for normal pivot-mode keys.
///
@@ -84,6 +92,9 @@ pub enum RecordCol {
Measure(String),
/// The trailing column that creates a new measure.
NewMeasure,
/// The column that creates a new category (improvise-2zq). Sits after
/// the coordinates, where the column it creates will appear.
NewCategory,
}
/// One entry on a grid axis: either a visual group header or a data-item tuple.
@@ -320,6 +331,7 @@ impl GridLayout {
})
.map(|c| RecordCol::Category(c.to_string()))
.collect();
cols.push(RecordCol::NewCategory);
cols.extend(
model
.category(MEASURE_CATEGORY)
@@ -336,6 +348,7 @@ impl GridLayout {
match col {
RecordCol::Category(name) | RecordCol::Measure(name) => name.clone(),
RecordCol::NewMeasure => NEW_MEASURE_COL.to_string(),
RecordCol::NewCategory => NEW_CATEGORY_COL.to_string(),
}
}
@@ -367,7 +380,7 @@ impl GridLayout {
),
// Always blank: it is where a new measure name is typed, never
// a place a value is stored.
RecordCol::NewMeasure => Some(String::new()),
RecordCol::NewMeasure | RecordCol::NewCategory => Some(String::new()),
}
}
@@ -605,8 +618,8 @@ fn cross_product(model: &Model, view: &View, cats: &[String]) -> Vec<AxisEntry>
#[cfg(test)]
mod tests {
use super::{
AxisEntry, GridLayout, NEW_MEASURE_COL, RecordCol, data_items, prune_axis_items,
synthetic_record_info,
AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, data_items,
prune_axis_items, synthetic_record_info,
};
use crate::model::cell::{CellKey, CellValue};
use crate::view::Axis;
@@ -835,22 +848,33 @@ mod tests {
.collect();
assert_eq!(
cols,
vec!["Region", "Revenue", "Cost", NEW_MEASURE_COL],
"categories, then one column per measure, then the new-measure column"
vec![
"Region",
NEW_CATEGORY_COL,
"Revenue",
"Cost",
NEW_MEASURE_COL
],
"categories, the new-category column, one column per measure, \
then the new-measure column — each `+` adjacent to what it adds \
to (improvise-2zq)"
);
assert_eq!(
layout.record_col(0),
Some(&RecordCol::Category("Region".to_string()))
);
assert_eq!(layout.record_col(1), Some(&RecordCol::NewCategory));
assert_eq!(
layout.record_col(1),
layout.record_col(2),
Some(&RecordCol::Measure("Revenue".to_string()))
);
assert_eq!(layout.record_col(3), Some(&RecordCol::NewMeasure));
assert_eq!(layout.record_col(4), Some(&RecordCol::NewMeasure));
// Both measures of the one row are readable side by side.
assert_eq!(layout.records_display(0, 1).as_deref(), Some("100"));
assert_eq!(layout.records_display(0, 2).as_deref(), Some("50"));
// Both measures of the one row are readable side by side, and the
// `+cat` column between them and the coordinate is always blank.
assert_eq!(layout.records_display(0, 1).as_deref(), Some(""));
assert_eq!(layout.records_display(0, 2).as_deref(), Some("100"));
assert_eq!(layout.records_display(0, 3).as_deref(), Some("50"));
}
/// On initial entry into records mode, rows should be in a deterministic
+2 -1
View File
@@ -4,6 +4,7 @@ pub mod types;
pub use axis::{Axis, AxisColor};
pub use layout::{
AxisEntry, GridLayout, NEW_MEASURE_COL, RecordCol, RecordRow, synthetic_record_info,
AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow,
synthetic_record_info,
};
pub use types::{DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View};
+6 -4
View File
@@ -39,8 +39,10 @@ fn imported_model_shows_one_row_per_record_with_measure_columns() {
.collect();
assert_eq!(
cols,
vec!["product", "region", "cost", "revenue", "+"],
"categories, then one column per measure, then the new-measure column"
vec!["product", "region", "+cat", "cost", "revenue", "+"],
"categories, the new-category column, one column per measure, then \
the new-measure column — each `+` sits next to what it adds to \
(improvise-2zq)"
);
assert_eq!(layout.row_count(), 2, "two source records, not four cells");
@@ -57,8 +59,8 @@ fn imported_model_shows_one_row_per_record_with_measure_columns() {
assert_eq!(
rows,
vec![
vec!["Pants", "West", "80", "200", ""],
vec!["Shirts", "East", "40", "100", ""],
vec!["Pants", "West", "", "80", "200", ""],
vec!["Shirts", "East", "", "40", "100", ""],
],
"got {rows:?}"
);