fix(protocol): a formula-owned cell projects CellId::Derived, not Record
improvise-ir7l. project::cell_id decided Record vs Aggregate purely on whether a regular category sat on Axis::None; it never asked whether the coordinate named a formula target. So a formula cell was a Record, render.rs::editable said true, and a write to it landed, was stored, and was then shadowed forever by formula_cache — not a revert, a permanent silent shadow, on the demo's front page. Rather than a second "but not really" flag beside CellId (the two-answers shape ADR 002 exists to forbid), a formula-owned cell now projects a different kind of identity. Derived carries the formula's target and no surrogate, so the write is unconstructible rather than refused. The predicate is Model::formula_for(&CellKey), one home, which already existed three times open-coded inside model/types.rs. The TUI lands in the same change: EditOrDrill calls the same predicate and refuses, or the two front ends diverge. Two orderings are load-bearing and tested: Derived is asked before the record branch (the coordinate is stored), and after the aggregate branch (gotcha 10 — an aggregating formula cell is still drillable). Two tests were passing while their names were false for the Profit column and are renamed and two-sided now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -258,6 +258,35 @@ client's own token is the lookup key for its own version, with no `CellKey`
|
|||||||
rebuilt on the hot path. `measure` is absent for a record carrying no
|
rebuilt on the hot path. `measure` is absent for a record carrying no
|
||||||
`_Measure` coordinate at all — the shape `add-record-row` seeds.
|
`_Measure` coordinate at all — the shape `add-record-row` seeds.
|
||||||
|
|
||||||
|
### `CellId::Derived` — a formula owns the cell (improvise-ir7l)
|
||||||
|
|
||||||
|
A formula target is a real stored coordinate, so the record branch claimed it
|
||||||
|
and every client offered an editor over it. The write *landed* and was then
|
||||||
|
shadowed forever, because `evaluate_aggregated` reads `formula_cache` before
|
||||||
|
raw data — not a revert, a permanent silent shadow.
|
||||||
|
|
||||||
|
The fix keeps ADR 002's discipline rather than adding a second answer beside
|
||||||
|
it: a formula-owned cell projects a **different kind of identity**, so the
|
||||||
|
write is unconstructible instead of flagged. `Derived` carries the formula's
|
||||||
|
target and **no surrogate**, so there is nothing a client could spend on a
|
||||||
|
`SetCell`; that is the same reason `Aggregate` carries an `AggregateHash` and
|
||||||
|
not a `RecordHash`.
|
||||||
|
|
||||||
|
Two orderings matter in `project::cell_id`. Derived is asked **before** the
|
||||||
|
record branch, since the coordinate is stored and the record branch would
|
||||||
|
otherwise win. It is asked **after** the aggregate branch, because a formula
|
||||||
|
cell that also aggregates is still drillable (gotcha 10) — `Aggregate` and
|
||||||
|
`Derived` are not alternatives for the same cell, and reversing the two turns
|
||||||
|
a drillable cell into a dead one.
|
||||||
|
|
||||||
|
The predicate itself is `Model::formula_for(&CellKey)` in `improvise-core`,
|
||||||
|
the one home of "is this cell derived". It already existed three times inside
|
||||||
|
`model/types.rs` as an open-coded loop; the TUI's `EditOrDrill` and the
|
||||||
|
producer now call the same function rather than each spelling the rule. The
|
||||||
|
TUI had to change in the same breath: it re-derives from the `Model` it holds,
|
||||||
|
which is legitimate (it is the authority), but it must give the *same* answer
|
||||||
|
or the two front ends diverge — design-principles §5.
|
||||||
|
|
||||||
### `CellDisplay::id` — `None` is not "aggregate"
|
### `CellDisplay::id` — `None` is not "aggregate"
|
||||||
|
|
||||||
That is `Some(CellId::Aggregate(..))`, and the distinction is the whole point
|
That is `Some(CellId::Aggregate(..))`, and the distinction is the whole point
|
||||||
|
|||||||
@@ -665,3 +665,18 @@ persistence criterion.
|
|||||||
- **`a_delete_is_visible_at_once_and_lands_on_commit`** — both halves, because
|
- **`a_delete_is_visible_at_once_and_lands_on_commit`** — both halves, because
|
||||||
either one alone would pass against a backend that got the timing exactly
|
either one alone would pass against a backend that got the timing exactly
|
||||||
backwards.
|
backwards.
|
||||||
|
|
||||||
|
### the_shipped_demo_seeds_a_worker_whose_raw_cells_are_settable
|
||||||
|
|
||||||
|
improvise-ir7l renamed this from `..._whose_every_cell_is_settable` and
|
||||||
|
narrowed what it claims. The old loop called `MainThread::cell` — which panics
|
||||||
|
on anything but a stored cell — over *every* column including `Profit`, which
|
||||||
|
`web/demo.improv` defines as `Revenue - Cost`. It passed only because a formula
|
||||||
|
target projected `CellId::Record`, which is the bug the ticket names: a test
|
||||||
|
whose name was false for a third of the grid it walked.
|
||||||
|
|
||||||
|
It now walks the two raw columns and asserts separately that the `Profit`
|
||||||
|
column is `CellId::Derived`, so it fails if either half moves. That second
|
||||||
|
assertion is what makes the narrowing honest rather than a retreat — dropping
|
||||||
|
the column would have left the page free to offer it again with nothing
|
||||||
|
noticing.
|
||||||
|
|||||||
@@ -2166,3 +2166,33 @@ step machine; every other printable key edits the name (gotcha 22).
|
|||||||
On the confirmation step Enter installs the built workbook and Esc steps back — it
|
On the confirmation step Enter installs the built workbook and Esc steps back — it
|
||||||
does not throw the import away, and no key closes the wizard outright
|
does not throw the import away, and no key closes the wizard outright
|
||||||
(improvise-907, from improvise-c6j).
|
(improvise-907, from improvise-c6j).
|
||||||
|
|
||||||
|
### a_formula_owned_cell_is_projected_as_derived_and_its_siblings_are_not
|
||||||
|
|
||||||
|
improvise-ir7l. `project::cell_id` decided `Record` vs `Aggregate` purely on
|
||||||
|
whether a regular category sat on `Axis::None`; it never asked whether the
|
||||||
|
`_Measure` coordinate named a formula target. So a formula cell was a `Record`,
|
||||||
|
`render.rs::editable` said true, and a write to it was accepted, stored and
|
||||||
|
then shadowed forever by `formula_cache`.
|
||||||
|
|
||||||
|
The fault injected: replacing the `derived` lookup with `None`, which is the
|
||||||
|
pre-fix code exactly. The test then reports `Record(RecordCell { measure:
|
||||||
|
Some("Profit") })` where it wants `Derived { target: "Profit" }`. The siblings
|
||||||
|
are asserted in the same test so a producer that answered `Derived` for *every*
|
||||||
|
cell could not pass.
|
||||||
|
|
||||||
|
### an_aggregating_formula_cell_is_still_projected_as_an_aggregate
|
||||||
|
|
||||||
|
The negative arm of the same change, and the one a careless fix breaks: gotcha
|
||||||
|
10 says drilling into a formula cell is supported, so `Aggregate` must still
|
||||||
|
win where the cell aggregates. Proved discriminating by returning `derived`
|
||||||
|
before the aggregate branch — the cell then reads `Derived` and becomes
|
||||||
|
undrillable, with nothing else in the suite noticing.
|
||||||
|
|
||||||
|
### edit_or_drill_refuses_a_formula_owned_cell
|
||||||
|
|
||||||
|
The TUI half. `EditOrDrill` never asked the question either, so the terminal
|
||||||
|
offered the same editor over the same shadowed cell. Landing only the
|
||||||
|
projection half would have made the two front ends disagree, which is the
|
||||||
|
defect design-principles §5 is about. Fault injected by deleting the guard: the
|
||||||
|
command returns `[SetBuffer{edit}, ChangeMode(Editing)]` again.
|
||||||
|
|||||||
@@ -329,7 +329,7 @@ close.
|
|||||||
|
|
||||||
## `tests/web_demo_model.rs`
|
## `tests/web_demo_model.rs`
|
||||||
|
|
||||||
### every_visible_cell_in_the_web_demo_is_settable
|
### every_web_demo_cell_is_settable_except_the_column_a_formula_owns
|
||||||
|
|
||||||
improvise-bck. The demo document the standalone page opens must be one a visitor
|
improvise-bck. The demo document the standalone page opens must be one a visitor
|
||||||
can *type into*, and the bug is not hypothetical: `examples/demo.improv` leaves
|
can *type into*, and the bug is not hypothetical: `examples/demo.improv` leaves
|
||||||
@@ -348,6 +348,17 @@ and could agree with itself while disagreeing with the client.
|
|||||||
It was proved discriminating by pointing it at `examples/demo.improv`, where it
|
It was proved discriminating by pointing it at `examples/demo.improv`, where it
|
||||||
names the aggregate at (0,0).
|
names the aggregate at (0,0).
|
||||||
|
|
||||||
|
**improvise-ir7l renamed it, because the old name was false.** The demo's
|
||||||
|
third column is `Profit = Revenue - Cost`, and a formula target is a stored
|
||||||
|
coordinate, so it projected `Record` and the page offered an editor over it —
|
||||||
|
the test passed while its name was wrong for a third of the landing-page grid,
|
||||||
|
which is exactly the shape this repo keeps finding. The assertion is now
|
||||||
|
two-sided: the two raw columns must be `Record` and the `Profit` column must
|
||||||
|
be `CellId::Derived`, so it fails if either half moves. Proved discriminating
|
||||||
|
in both directions by deleting the `derived` branch of `project::cell_id`
|
||||||
|
(Profit reads back as a `Record`) and, separately, by pointing it at
|
||||||
|
`examples/demo.improv` again (the aggregate at (0,0) still fails it).
|
||||||
|
|
||||||
### the_web_demo_shows_a_formula_beside_its_inputs
|
### the_web_demo_shows_a_formula_beside_its_inputs
|
||||||
|
|
||||||
The payoff a visitor should see without pressing a key: a formula column beside
|
The payoff a visitor should see without pressing a key: a formula column beside
|
||||||
|
|||||||
@@ -188,6 +188,31 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// improvise-ir7l: the cell under the cursor is a formula target, so there is
|
||||||
|
/// nothing to type into. `context/test-evidence/command.md`.
|
||||||
|
#[test]
|
||||||
|
fn edit_or_drill_refuses_a_formula_owned_cell() {
|
||||||
|
let mut m = two_cat_model();
|
||||||
|
m.active_view_mut().set_axis("Type", crate::view::Axis::Row);
|
||||||
|
m.active_view_mut()
|
||||||
|
.set_axis("_Measure", crate::view::Axis::Column);
|
||||||
|
m.model
|
||||||
|
.add_formula(crate::formula::parse_formula("Profit = 1", "_Measure").expect("parses"));
|
||||||
|
let layout = make_layout(&m);
|
||||||
|
let reg = make_registry();
|
||||||
|
let ctx = make_ctx(&m, &layout, ®);
|
||||||
|
let dbg = effects_debug(
|
||||||
|
&EditOrDrill {
|
||||||
|
edit_mode: AppMode::editing(),
|
||||||
|
}
|
||||||
|
.execute(&ctx),
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
dbg.contains("computed by a formula") && !dbg.contains("Editing"),
|
||||||
|
"a formula cell must refuse rather than open an editor, got: {dbg}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn enter_search_mode_sets_flag_and_clears_query() {
|
fn enter_search_mode_sets_flag_and_clears_query() {
|
||||||
let m = two_cat_model();
|
let m = two_cat_model();
|
||||||
@@ -313,6 +338,18 @@ impl Cmd for EditOrDrill {
|
|||||||
};
|
};
|
||||||
return DrillIntoCell { key }.execute(ctx);
|
return DrillIntoCell { key }.execute(ctx);
|
||||||
}
|
}
|
||||||
|
// The same question `cell_id` asks, off the same predicate: a formula owns
|
||||||
|
// the cell, so there is nothing to type into (improvise-ir7l).
|
||||||
|
if let Some(formula) = ctx
|
||||||
|
.cell_key()
|
||||||
|
.as_ref()
|
||||||
|
.and_then(|k| ctx.model.formula_for(k))
|
||||||
|
{
|
||||||
|
return vec![effect::set_status(format!(
|
||||||
|
"{} is computed by a formula — edit the formula, not the cell",
|
||||||
|
formula.target
|
||||||
|
))];
|
||||||
|
}
|
||||||
vec![
|
vec![
|
||||||
Box::new(effect::SetBuffer {
|
Box::new(effect::SetBuffer {
|
||||||
name: "edit".to_string(),
|
name: "edit".to_string(),
|
||||||
|
|||||||
@@ -308,11 +308,18 @@ fn cell_id(app: &App, row: usize, col: usize) -> Option<CellId> {
|
|||||||
.category(cat)
|
.category(cat)
|
||||||
.is_some_and(|category| category.kind.is_regular())
|
.is_some_and(|category| category.kind.is_regular())
|
||||||
});
|
});
|
||||||
|
// A derived cell is asked about before a stored one: a formula target is a
|
||||||
|
// real coordinate, so the record branch would otherwise claim it (ir7l).
|
||||||
|
let derived = model.formula_for(&key).map(|f| CellId::Derived {
|
||||||
|
target: f.target.clone(),
|
||||||
|
});
|
||||||
Some(match aggregated {
|
Some(match aggregated {
|
||||||
true => CellId::Aggregate(AggregateHash::of(&key)),
|
true => CellId::Aggregate(AggregateHash::of(&key)),
|
||||||
false => CellId::Record(RecordCell {
|
false => derived.unwrap_or_else(|| {
|
||||||
measure: key.get(MEASURE_CATEGORY).map(String::from),
|
CellId::Record(RecordCell {
|
||||||
record: RecordHash::of(&key),
|
measure: key.get(MEASURE_CATEGORY).map(String::from),
|
||||||
|
record: RecordHash::of(&key),
|
||||||
|
})
|
||||||
}),
|
}),
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
@@ -673,7 +680,7 @@ impl ClientSession {
|
|||||||
.flatten()
|
.flatten()
|
||||||
.filter_map(|cell| match cell.id.as_ref()? {
|
.filter_map(|cell| match cell.id.as_ref()? {
|
||||||
CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))),
|
CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))),
|
||||||
CellId::Aggregate(_) => None,
|
CellId::Aggregate(_) | CellId::Derived { .. } => None,
|
||||||
})
|
})
|
||||||
.collect();
|
.collect();
|
||||||
let desired = basis::published(app.document_generation(), app.layout_version(), records);
|
let desired = basis::published(app.document_generation(), app.layout_version(), records);
|
||||||
@@ -964,7 +971,7 @@ mod tests {
|
|||||||
.flatten()
|
.flatten()
|
||||||
.filter_map(|cell| match cell.id.as_ref()? {
|
.filter_map(|cell| match cell.id.as_ref()? {
|
||||||
CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))),
|
CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))),
|
||||||
CellId::Aggregate(_) => None,
|
CellId::Aggregate(_) | CellId::Derived { .. } => None,
|
||||||
})
|
})
|
||||||
.collect(),
|
.collect(),
|
||||||
));
|
));
|
||||||
@@ -2201,6 +2208,12 @@ mod cell_identity {
|
|||||||
/// Region on Row, `_Measure` on Column, nothing regular on None. Three rows and
|
/// Region on Row, `_Measure` on Column, nothing regular on None. Three rows and
|
||||||
/// two measures, so "the record" is not "the row" or "the cell".
|
/// two measures, so "the record" is not "the row" or "the cell".
|
||||||
fn raw_app() -> App {
|
fn raw_app() -> App {
|
||||||
|
let mut app = App::new(raw_workbook(), None);
|
||||||
|
app.rebuild_layout();
|
||||||
|
app
|
||||||
|
}
|
||||||
|
|
||||||
|
fn raw_workbook() -> Workbook {
|
||||||
let mut wb = Workbook::new("Raw");
|
let mut wb = Workbook::new("Raw");
|
||||||
wb.add_category("Region").unwrap();
|
wb.add_category("Region").unwrap();
|
||||||
for item in ["North", "South", "East"] {
|
for item in ["North", "South", "East"] {
|
||||||
@@ -2221,9 +2234,7 @@ mod cell_identity {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
let mut app = App::new(wb, None);
|
wb
|
||||||
app.rebuild_layout();
|
|
||||||
app
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The same with a second *regular* category left on None, so every visible cell
|
/// The same with a second *regular* category left on None, so every visible cell
|
||||||
@@ -2259,6 +2270,33 @@ mod cell_identity {
|
|||||||
app
|
app
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// `raw_app` plus `Profit = Revenue - Cost`, so column 2 is formula-owned and
|
||||||
|
/// columns 0 and 1 are the raw cells that feed it.
|
||||||
|
fn formula_app() -> App {
|
||||||
|
let mut wb = raw_workbook();
|
||||||
|
wb.model.add_formula(
|
||||||
|
improvise_formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(),
|
||||||
|
);
|
||||||
|
let mut app = App::new(wb, None);
|
||||||
|
app.rebuild_layout();
|
||||||
|
app
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The same document with a second regular category left on None, so the
|
||||||
|
/// formula column aggregates as well as being derived.
|
||||||
|
fn aggregating_formula_app() -> App {
|
||||||
|
let mut wb = raw_workbook();
|
||||||
|
wb.add_category("Product").unwrap();
|
||||||
|
wb.add_item("Product", "Shirts").unwrap();
|
||||||
|
wb.active_view_mut().set_axis("Product", Axis::None);
|
||||||
|
wb.model.add_formula(
|
||||||
|
improvise_formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(),
|
||||||
|
);
|
||||||
|
let mut app = App::new(wb, None);
|
||||||
|
app.rebuild_layout();
|
||||||
|
app
|
||||||
|
}
|
||||||
|
|
||||||
fn id_at(app: &App, row: usize, col: usize) -> CellId {
|
fn id_at(app: &App, row: usize, col: usize) -> CellId {
|
||||||
grid_of(app, WHOLE)
|
grid_of(app, WHOLE)
|
||||||
.cell(row, col)
|
.cell(row, col)
|
||||||
@@ -2271,7 +2309,7 @@ mod cell_identity {
|
|||||||
fn record_of_id(id: CellId) -> RecordCell {
|
fn record_of_id(id: CellId) -> RecordCell {
|
||||||
match id {
|
match id {
|
||||||
CellId::Record(cell) => cell,
|
CellId::Record(cell) => cell,
|
||||||
CellId::Aggregate(hash) => panic!("expected a stored cell, got an aggregate {hash}"),
|
other => panic!("expected a stored cell, got {other:?}"),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -2282,6 +2320,31 @@ mod cell_identity {
|
|||||||
assert!(matches!(id_at(&raw_app(), 0, 0), CellId::Record(_)));
|
assert!(matches!(id_at(&raw_app(), 0, 0), CellId::Record(_)));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A formula target is a real coordinate, so the record branch claimed it and
|
||||||
|
/// the browser offered an editor. `context/test-evidence/command.md`.
|
||||||
|
#[test]
|
||||||
|
fn a_formula_owned_cell_is_projected_as_derived_and_its_siblings_are_not() {
|
||||||
|
let app = formula_app();
|
||||||
|
assert_eq!(
|
||||||
|
id_at(&app, 0, 2),
|
||||||
|
CellId::Derived {
|
||||||
|
target: "Profit".to_string()
|
||||||
|
}
|
||||||
|
);
|
||||||
|
assert!(matches!(id_at(&app, 0, 0), CellId::Record(_)));
|
||||||
|
assert!(matches!(id_at(&app, 0, 1), CellId::Record(_)));
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Gotcha 10: a formula cell that *aggregates* is still drillable, so the
|
||||||
|
/// derived answer must not displace the aggregate one.
|
||||||
|
#[test]
|
||||||
|
fn an_aggregating_formula_cell_is_still_projected_as_an_aggregate() {
|
||||||
|
assert!(matches!(
|
||||||
|
id_at(&aggregating_formula_app(), 0, 2),
|
||||||
|
CellId::Aggregate(_)
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_cell_that_sums_over_a_none_axis_category_is_projected_as_an_aggregate() {
|
fn a_cell_that_sums_over_a_none_axis_category_is_projected_as_an_aggregate() {
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
@@ -2642,7 +2705,7 @@ mod basis_check {
|
|||||||
.expect("a stored cell is projected with an identity")
|
.expect("a stored cell is projected with an identity")
|
||||||
{
|
{
|
||||||
CellId::Record(cell) => cell,
|
CellId::Record(cell) => cell,
|
||||||
CellId::Aggregate(h) => panic!("the fixture must project stored cells, got {h}"),
|
other => panic!("the fixture must project stored cells, got {other:?}"),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -247,6 +247,14 @@ impl Model {
|
|||||||
&self.formulas
|
&self.formulas
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The formula that owns `key`, if one does. **The one home** of "is this
|
||||||
|
/// cell derived" — every other answer to it is a second derivation.
|
||||||
|
pub fn formula_for(&self, key: &CellKey) -> Option<&Formula> {
|
||||||
|
self.formulas
|
||||||
|
.iter()
|
||||||
|
.find(|f| key.get(&f.target_category) == Some(f.target.as_str()))
|
||||||
|
}
|
||||||
|
|
||||||
/// Names of all categories, including the virtual ones.
|
/// Names of all categories, including the virtual ones.
|
||||||
pub fn category_names(&self) -> Vec<&str> {
|
pub fn category_names(&self) -> Vec<&str> {
|
||||||
self.categories.keys().map(|s| s.as_str()).collect()
|
self.categories.keys().map(|s| s.as_str()).collect()
|
||||||
@@ -312,14 +320,10 @@ impl Model {
|
|||||||
if depth == 0 {
|
if depth == 0 {
|
||||||
return Some(CellValue::Error("circular".into()));
|
return Some(CellValue::Error("circular".into()));
|
||||||
}
|
}
|
||||||
for formula in &self.formulas {
|
match self.formula_for(key) {
|
||||||
if let Some(item_val) = key.get(&formula.target_category)
|
Some(formula) => self.eval_formula_depth(formula, key, depth - 1),
|
||||||
&& item_val == formula.target
|
None => self.data.get(key).cloned(),
|
||||||
{
|
|
||||||
return self.eval_formula_depth(formula, key, depth - 1);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
self.data.get(key).cloned()
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The aggregation to apply to a cell's raw values, from the measure
|
/// The aggregation to apply to a cell's raw values, from the measure
|
||||||
@@ -426,14 +430,8 @@ impl Model {
|
|||||||
/// One formula cell for the fixed-point pass: raw aggregation for a
|
/// One formula cell for the fixed-point pass: raw aggregation for a
|
||||||
/// non-formula ref, the cache for a formula ref.
|
/// non-formula ref, the cache for a formula ref.
|
||||||
fn evaluate_formula_cell(&self, key: &CellKey, none_cats: &[String]) -> Option<CellValue> {
|
fn evaluate_formula_cell(&self, key: &CellKey, none_cats: &[String]) -> Option<CellValue> {
|
||||||
for formula in &self.formulas {
|
self.formula_for(key)
|
||||||
if let Some(item_val) = key.get(&formula.target_category)
|
.and_then(|formula| self.eval_formula_with_cache(formula, key, none_cats))
|
||||||
&& item_val == formula.target
|
|
||||||
{
|
|
||||||
return self.eval_formula_with_cache(formula, key, none_cats);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
None
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The cached evaluator. Its twin is `eval_formula_depth`; a rule stated
|
/// The cached evaluator. Its twin is `eval_formula_depth`; a rule stated
|
||||||
|
|||||||
@@ -47,6 +47,9 @@ pub enum CellId {
|
|||||||
Record(RecordCell),
|
Record(RecordCell),
|
||||||
/// An aggregate over stored cells. `Drill` takes this and nothing else.
|
/// An aggregate over stored cells. `Drill` takes this and nothing else.
|
||||||
Aggregate(AggregateHash),
|
Aggregate(AggregateHash),
|
||||||
|
/// A cell a formula owns. It carries no surrogate, so a write for it is
|
||||||
|
/// unconstructible rather than refused — see `context/code/protocol.md` §3.
|
||||||
|
Derived { target: String },
|
||||||
}
|
}
|
||||||
|
|
||||||
/// One cell as the client draws it. Deliberately not a `CellValue` — the
|
/// One cell as the client draws it. Deliberately not a `CellValue` — the
|
||||||
|
|||||||
@@ -462,6 +462,9 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option<Vec<Command>> {
|
|||||||
CellId::Aggregate(_) => Some(vec![Command::SetStatus(
|
CellId::Aggregate(_) => Some(vec![Command::SetStatus(
|
||||||
"drilling into an aggregate needs the host — not in the browser yet".to_string(),
|
"drilling into an aggregate needs the host — not in the browser yet".to_string(),
|
||||||
)]),
|
)]),
|
||||||
|
CellId::Derived { target } => Some(vec![Command::SetStatus(format!(
|
||||||
|
"{target} is computed by a formula — edit the formula, not the cell"
|
||||||
|
))]),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -526,7 +529,7 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option<V
|
|||||||
fn record_at_cursor(state: &ClientState) -> Option<improvise_protocol::RecordHash> {
|
fn record_at_cursor(state: &ClientState) -> Option<improvise_protocol::RecordHash> {
|
||||||
match cell_at_cursor(state)?.id.as_ref()? {
|
match cell_at_cursor(state)?.id.as_ref()? {
|
||||||
CellId::Record(cell) => Some(cell.record),
|
CellId::Record(cell) => Some(cell.record),
|
||||||
CellId::Aggregate(_) => None,
|
CellId::Aggregate(_) | CellId::Derived { .. } => None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1149,6 +1152,23 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// `i` on a formula-owned cell names the formula, so the user learns where
|
||||||
|
/// the value comes from rather than typing into a shadowed cell (ir7l).
|
||||||
|
#[test]
|
||||||
|
fn i_on_a_derived_cell_names_the_formula() {
|
||||||
|
let client = client_on(CellId::Derived {
|
||||||
|
target: "Profit".into(),
|
||||||
|
});
|
||||||
|
let KeyOutcome::Local(commands) = resolve(client.state(), ev(KeyCode::Char('i'))) else {
|
||||||
|
panic!("a derived cell must not be a silent no-op");
|
||||||
|
};
|
||||||
|
assert!(
|
||||||
|
matches!(&commands[..], [Command::SetStatus(msg)] if msg.contains("Profit")
|
||||||
|
&& msg.contains("formula")),
|
||||||
|
"expected a status naming the formula target, got {commands:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/// Enter in `Editing` is `[commit-cell-edit editing, clear-buffer edit]`
|
/// Enter in `Editing` is `[commit-cell-edit editing, clear-buffer edit]`
|
||||||
/// and every part shows up here — including the odd-looking trailing clear.
|
/// and every part shows up here — including the odd-looking trailing clear.
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -127,11 +127,11 @@ impl Client {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// The address of the stored cell at a viewport position, or `None` on the
|
/// The address of the stored cell at a viewport position, or `None` on the
|
||||||
/// three shapes that are not one. The last is not a check (ADR 002).
|
/// four shapes that are not one. The last is not a check (ADR 002).
|
||||||
pub fn record_cell_at(&self, row: usize, col: usize) -> Option<RecordCell> {
|
pub fn record_cell_at(&self, row: usize, col: usize) -> Option<RecordCell> {
|
||||||
match self.state.grid.cell(row, col)?.id.clone()? {
|
match self.state.grid.cell(row, col)?.id.clone()? {
|
||||||
CellId::Record(cell) => Some(cell),
|
CellId::Record(cell) => Some(cell),
|
||||||
CellId::Aggregate(_) => None,
|
CellId::Aggregate(_) | CellId::Derived { .. } => None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -305,8 +305,8 @@ mod tests {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A 2x2 grid whose four cells cover the three id shapes plus a padded
|
/// A 2x3 grid whose cells cover the four id shapes plus a padded slot, so
|
||||||
/// slot, so one fixture answers every editability question.
|
/// one fixture answers every editability question.
|
||||||
fn group(cat: &str, name: &str, collapsed: bool) -> FrameEntry {
|
fn group(cat: &str, name: &str, collapsed: bool) -> FrameEntry {
|
||||||
FrameEntry::Group(improvise_protocol::FrameGroup {
|
FrameEntry::Group(improvise_protocol::FrameGroup {
|
||||||
cat: cat.into(),
|
cat: cat.into(),
|
||||||
@@ -351,13 +351,21 @@ mod tests {
|
|||||||
view,
|
view,
|
||||||
seed: vec![
|
seed: vec![
|
||||||
Projection::Grid(GridUpdate::RowFrame(AxisFrame::flat(["North", "South"]))),
|
Projection::Grid(GridUpdate::RowFrame(AxisFrame::flat(["North", "South"]))),
|
||||||
Projection::Grid(GridUpdate::ColFrame(AxisFrame::flat(["Revenue", "Total"]))),
|
Projection::Grid(GridUpdate::ColFrame(AxisFrame::flat([
|
||||||
|
"Revenue", "Total", "Profit",
|
||||||
|
]))),
|
||||||
Projection::Grid(GridUpdate::Cells(vec![
|
Projection::Grid(GridUpdate::Cells(vec![
|
||||||
((0, 0), CellDisplay::number("1200").with_id(record_id())),
|
((0, 0), CellDisplay::number("1200").with_id(record_id())),
|
||||||
(
|
(
|
||||||
(0, 1),
|
(0, 1),
|
||||||
CellDisplay::number("1900").with_id(CellId::Aggregate(aggregate())),
|
CellDisplay::number("1900").with_id(CellId::Aggregate(aggregate())),
|
||||||
),
|
),
|
||||||
|
(
|
||||||
|
(0, 2),
|
||||||
|
CellDisplay::number("700").with_id(CellId::Derived {
|
||||||
|
target: "Profit".into(),
|
||||||
|
}),
|
||||||
|
),
|
||||||
((1, 0), CellDisplay::text("pending")),
|
((1, 0), CellDisplay::text("pending")),
|
||||||
])),
|
])),
|
||||||
],
|
],
|
||||||
@@ -365,7 +373,7 @@ mod tests {
|
|||||||
client
|
client
|
||||||
}
|
}
|
||||||
|
|
||||||
/// ADR 002's permission, as the page sees it: three different facts, and
|
/// ADR 002's permission, as the page sees it: four different facts, and
|
||||||
/// only the first is a cell a user may type into.
|
/// only the first is a cell a user may type into.
|
||||||
#[test]
|
#[test]
|
||||||
fn only_a_record_cell_renders_as_editable() {
|
fn only_a_record_cell_renders_as_editable() {
|
||||||
@@ -378,6 +386,10 @@ mod tests {
|
|||||||
!model.rows[0].cells()[1].editable,
|
!model.rows[0].cells()[1].editable,
|
||||||
"an aggregate is a drill target, and Drill is not on the wire yet"
|
"an aggregate is a drill target, and Drill is not on the wire yet"
|
||||||
);
|
);
|
||||||
|
assert!(
|
||||||
|
!model.rows[0].cells()[2].editable,
|
||||||
|
"a formula owns the cell, so a write to it would be shadowed on recompute"
|
||||||
|
);
|
||||||
assert!(
|
assert!(
|
||||||
!model.rows[1].cells()[0].editable,
|
!model.rows[1].cells()[0].editable,
|
||||||
"id: None is not 'aggregate' — it is 'no projected cell', and not writable either"
|
"id: None is not 'aggregate' — it is 'no projected cell', and not writable either"
|
||||||
@@ -396,7 +408,7 @@ mod tests {
|
|||||||
assert!(model.rows[0].cells()[0].numeric);
|
assert!(model.rows[0].cells()[0].numeric);
|
||||||
assert!(!model.rows[1].cells()[0].numeric);
|
assert!(!model.rows[1].cells()[0].numeric);
|
||||||
assert_eq!(model.rows[0].cells()[0].text, "1200");
|
assert_eq!(model.rows[0].cells()[0].text, "1200");
|
||||||
assert_eq!(model.col_levels[0], vec!["Revenue", "Total"]);
|
assert_eq!(model.col_levels[0], vec!["Revenue", "Total", "Profit"]);
|
||||||
assert_eq!(model.rows[1].labels(), ["South"]);
|
assert_eq!(model.rows[1].labels(), ["South"]);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -189,7 +189,7 @@ mod tests {
|
|||||||
fn addressed(grid: &GridCache, row: usize, col: usize) -> RecordCell {
|
fn addressed(grid: &GridCache, row: usize, col: usize) -> RecordCell {
|
||||||
match grid.cells[row][col].id.as_ref().expect("a projected cell") {
|
match grid.cells[row][col].id.as_ref().expect("a projected cell") {
|
||||||
CellId::Record(rc) => rc.clone(),
|
CellId::Record(rc) => rc.clone(),
|
||||||
CellId::Aggregate(_) => panic!("row {row} col {col} is an aggregate, not writable"),
|
other => panic!("row {row} col {col} is not writable: {other:?}"),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -120,7 +120,7 @@ impl MainThread {
|
|||||||
fn cell(&self, row: usize, col: usize) -> RecordCell {
|
fn cell(&self, row: usize, col: usize) -> RecordCell {
|
||||||
match self.grid.cells[row][col].id.as_ref().expect("a cell") {
|
match self.grid.cells[row][col].id.as_ref().expect("a cell") {
|
||||||
CellId::Record(rc) => rc.clone(),
|
CellId::Record(rc) => rc.clone(),
|
||||||
CellId::Aggregate(_) => panic!("({row},{col}) is an aggregate"),
|
other => panic!("({row},{col}) is not a stored cell: {other:?}"),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -300,7 +300,7 @@ fn the_vfs_offers_every_document_the_origin_holds() {
|
|||||||
/// The document the static site actually ships, driven the way the page drives
|
/// The document the static site actually ships, driven the way the page drives
|
||||||
/// it. See `context/test-evidence/clients.md`.
|
/// it. See `context/test-evidence/clients.md`.
|
||||||
#[test]
|
#[test]
|
||||||
fn the_shipped_demo_seeds_a_worker_whose_every_cell_is_settable() {
|
fn the_shipped_demo_seeds_a_worker_whose_raw_cells_are_settable() {
|
||||||
let seed = std::fs::read(Path::new(env!("CARGO_MANIFEST_DIR")).join("../../web/demo.improv"))
|
let seed = std::fs::read(Path::new(env!("CARGO_MANIFEST_DIR")).join("../../web/demo.improv"))
|
||||||
.expect("web/demo.improv is where the site build expects it");
|
.expect("web/demo.improv is where the site build expects it");
|
||||||
|
|
||||||
@@ -318,13 +318,22 @@ fn the_shipped_demo_seeds_a_worker_whose_every_cell_is_settable() {
|
|||||||
);
|
);
|
||||||
assert_eq!(main.text_at(0, 2), "12,200", "Profit is live on arrival");
|
assert_eq!(main.text_at(0, 2), "12,200", "Profit is live on arrival");
|
||||||
|
|
||||||
// `cell` panics on an aggregate, which is exactly the failure a visitor
|
// `cell` panics on anything but a stored cell, which is exactly the failure a
|
||||||
// meets as a cell that does not answer a click (ADR 002).
|
// visitor meets as a cell that does not answer a click (ADR 002).
|
||||||
for row in 0..main.grid.row_frame.len() {
|
for row in 0..main.grid.row_frame.len() {
|
||||||
for col in 0..main.grid.col_frame.len() {
|
for col in 0..2 {
|
||||||
main.cell(row, col);
|
main.cell(row, col);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// The third column is `Profit`, which a formula owns: the page must not
|
||||||
|
// offer it, so it carries no surrogate to offer (improvise-ir7l).
|
||||||
|
for row in 0..main.grid.row_frame.len() {
|
||||||
|
assert!(
|
||||||
|
matches!(main.grid.cells[row][2].id, Some(CellId::Derived { .. })),
|
||||||
|
"row {row}'s Profit cell must be Derived, got {:?}",
|
||||||
|
main.grid.cells[row][2].id
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// And the payoff the page exists to show: an edit to an input moves the
|
// And the payoff the page exists to show: an edit to an input moves the
|
||||||
// formula beside it, through the projection stream.
|
// formula beside it, through the projection stream.
|
||||||
|
|||||||
+20
-12
@@ -11,10 +11,10 @@ fn demo_app() -> App {
|
|||||||
App::new(workbook, Some(path))
|
App::new(workbook, Some(path))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Every cell the page paints names a stored record, so every cell is one the
|
/// Every cell the page paints is either one the visitor may change or one a
|
||||||
/// visitor may click and change.
|
/// formula owns — never an aggregate. `context/test-evidence/tui.md`.
|
||||||
#[test]
|
#[test]
|
||||||
fn every_visible_cell_in_the_web_demo_is_settable() {
|
fn every_web_demo_cell_is_settable_except_the_column_a_formula_owns() {
|
||||||
let app = demo_app();
|
let app = demo_app();
|
||||||
let (rows, cols) = (app.layout.row_count(), app.layout.col_count());
|
let (rows, cols) = (app.layout.row_count(), app.layout.col_count());
|
||||||
assert!(rows > 0 && cols > 0, "the demo grid must not be empty");
|
assert!(rows > 0 && cols > 0, "the demo grid must not be empty");
|
||||||
@@ -32,15 +32,23 @@ fn every_visible_cell_in_the_web_demo_is_settable() {
|
|||||||
for r in 0..rows {
|
for r in 0..rows {
|
||||||
for c in 0..cols {
|
for c in 0..cols {
|
||||||
let cell = grid.cell(r, c).expect("a projected cell at every position");
|
let cell = grid.cell(r, c).expect("a projected cell at every position");
|
||||||
match &cell.id {
|
let expected_derived = grid
|
||||||
Some(CellId::Record(_)) => {}
|
.col_frame
|
||||||
other => panic!(
|
.item(c)
|
||||||
"cell ({r},{c}) — row {:?}, column {:?} — is {other:?}, not a Record, so the \
|
.is_some_and(|t| t.as_slice() == ["Profit"]);
|
||||||
page will show it as un-editable",
|
let ok = match (&cell.id, expected_derived) {
|
||||||
grid.row_frame.item(r),
|
(Some(CellId::Record(_)), false) => true,
|
||||||
grid.col_frame.item(c),
|
(Some(CellId::Derived { target }), true) => target == "Profit",
|
||||||
),
|
_ => false,
|
||||||
}
|
};
|
||||||
|
assert!(
|
||||||
|
ok,
|
||||||
|
"cell ({r},{c}) — row {:?}, column {:?} — is {:?}; the two raw columns must be \
|
||||||
|
Records the page offers, and Profit must be Derived so it does not",
|
||||||
|
grid.row_frame.item(r),
|
||||||
|
grid.col_frame.item(c),
|
||||||
|
cell.id,
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user