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

A view-scoped ModelCommand names its view (improvise-lltt)

Browse Source 
Nine ModelCommand variants configure a view and named it implicitly: the
effect read App::view_mut(), the HOST's slice. Since improvise-jytm made
ViewState::active_view authoritative per client, a client on another view
moved the host's view instead of its own -- its grid did not change and
every other client's did.

Each of the nine now carries `view: String`; model_effects maps it to
effect::ViewTarget::Named, and the nine effects carry a ViewTarget rather
than reading an ambient one. There is no operandless form left:
App::view_mut() is deleted and replaced by view_mut_of(&ViewTarget), so a
future geometry effect has to say which view it writes. The host's key
path passes ViewTarget::Active, which is right -- the TUI is the host and
has no session to ask. A Named view the document has not got is a no-op
rather than resolve_view's fallback onto the default.

Second half, which the ticket did not predict: naming the view made a
reframe of a view the host is NOT on reachable, and adopt_cursor_epoch
could not see one -- cursor_epoch is bumped only by the host's own
reset_cursor, so those clients were reset by nobody and told nothing
(gotcha 24's strictly-worse-than-nothing state). App::last_reframed_elsewhere
is the second trigger; its opening frames are taken lazily inside
view_mut_of, the one door such a write goes through, so the held-key path
still hashes one view and improvise-mcq4's declined cost is not paid.

Evidence and the three injected faults: context/test-evidence/command.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-21 10:23:27 -07:00
parent ce895ff560
commit 1dd6950ed3
19 changed files with 671 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files
benches/reshape_observation.rs
+5 -2
View File
@@ -8,7 +8,7 @@ use improvise::model::cell::{CellKey, CellValue};
use improvise::view::Axis;
use improvise::workbook::Workbook;
use improvise_command::ui::app::App;
use improvise_command::ui::effect::{CycleAxis, Effect, SetCell};
use improvise_command::ui::effect::{CycleAxis, Effect, SetCell, ViewTarget};
/// The fixture `grid_projection` uses, so the two numbers are comparable.
fn dense_app(regions: usize, products: usize, measures: usize) -> App {
@@ -108,7 +108,10 @@ fn geometry_at(
let (rows, cols) = (app.layout.row_count(), app.layout.col_count());
let mut best = Duration::MAX;
for i in 0..iters.max(3) {
let effects: Vec<Box<dyn Effect>> = vec![Box::new(CycleAxis("Product".to_string()))];
let effects: Vec<Box<dyn Effect>> = vec![Box::new(CycleAxis(
ViewTarget::Active,
"Product".to_string(),
))];
let start = Instant::now();
std::hint::black_box(app.apply_effects(effects));
best = best.min(start.elapsed());
context/code/command.md
+16 -2
View File
@@ -956,13 +956,27 @@ way, and the effect that got here cannot reach the session list. Bumping
`document_generation` is how the reset reaches them: `ClientSession::project`
compares it and reseeds.
### `adopt_cursor_epoch` — the epoch is the trigger, the view is the audience
### `adopt_cursor_epoch` — the epoch is one trigger of two, the view is the audience
`reset_cursor` resets the host's own slice; every connected client owes the same
invalidation, and `cursor_epoch` is how the reset reaches them. **The trigger stays
the epoch moving** — which effects renumber the grid is a per-effect decision, so a
observed** — which effects renumber the grid is a per-effect decision, so a
table of variants here would be a second description of it (gotcha 24).
**The epoch is the host's own cursor, so it is blind to a view the host is not on**,
and since improvise-lltt a view-configuration command names its view, so such a
write is reachable. The second trigger is `App::last_reframed_elsewhere` — the
non-active views this batch was observed to reframe — and it is a *precise* set
where `last_reframed_views` is deliberately a union with the reshaped set. Two
consequences. A batch that reframes only elsewhere relays against that precise set,
because the union would invalidate a client whose view merely *reshaped* (a page
selection that prunes nothing), which `paging_with_nothing_pruned_invalidates_no_cursor`
forbids. And the opening frames it diffs against are taken **lazily, at
`App::view_mut_of`** — the one door a non-active view can be written through — so
the held-key path still hashes one view and pays nothing for the wider question
(the premise improvise-mcq4 narrowed that arm on was `view_mut()` writing only the
active view, and that premise is gone).
**The audience is not everybody**, and has not been since improvise-jytm made
"which view a client is on" per-client: a reshape of view A threw away the cursor of
a client sitting on view B, which nothing reshaped. `cursor_invalidated` asks
context/test-evidence/command.md
+74
View File
@@ -3496,3 +3496,77 @@ The fix is not local and none is chosen: `grid_in` takes `&App` and
`recompute_formulas` needs `&mut Model`, so `grid_in` cannot recompute for the
view it is projecting. Candidates are on the ticket.
### a_view_named_command_lands_on_the_view_it_names
improvise-lltt, the oldest open P1 here, opened by improvise-jytm making
`ViewState::active_view` authoritative per client.
Nine `ModelCommand` variants configure a view — `SetAxis`, `CycleAxis`,
`TransposeAxes`, `SetPageSelection`, `HideItem`, `ShowItem`, `ToggleGroup`,
`TogglePruneEmpty`, `SetNumberFormat` — and each named its view *implicitly*:
`model_effects` built an effect that wrote `App::view_mut()`, which is the
**host's** slice. A client that had switched to `Second` therefore moved
`Default`: its own grid did not change, and every other client's did.
The fix is the protocol change the ticket asks for. Each of the nine carries a
`view: String`, `model_effects` maps it to `ViewTarget::Named`, and the nine
effects carry a `ViewTarget` rather than reading an ambient one. There is no
operandless form left: `App::view_mut()` is **deleted**, replaced by
`view_mut_of(&ViewTarget)`, so a future geometry effect has to say which view it
writes. The host's own key path passes `ViewTarget::Active`, which is right
because the TUI *is* the host and has no session to ask.
**Fault injected**: `model_effects`' `SetAxis` arm rebuilt with
`ViewTarget::Active` (the pre-fix behaviour, with `view` bound and discarded so
it still compiles). The test fails on `the asker's own view never moved` —
`Second`'s `Channel` stayed on `Row` while `Default`'s moved to `Page`.
The fixture must seat the asker somewhere the host is not, and the two views must
disagree about the category being moved: `Channel` is `Axis::None` in `Default`
and `Axis::Row` in `Second`, and the command asks for `Axis::Page`, so both
assertions separate the fixed code from the broken code in both directions.
### a_reframe_of_a_view_the_host_is_not_on_invalidates_its_clients
improvise-lltt's second half, and the part the ticket did not predict. Naming the
view made a reframe of a view the host is not on reachable for the first time,
and `Sessions::adopt_cursor_epoch` could not see one: it returned early unless
`App::cursor_epoch` had moved, and the epoch is bumped only by `reset_cursor`,
which `apply_effects` calls only for the **host's own** view. So the clients on
the reframed view were reset by nobody and told nothing — gotcha 24's
strictly-worse-than-nothing state, permanently stale rather than stale until the
next scroll.
`App::last_reframed_elsewhere` is the second trigger. Its opening frames are
taken lazily inside `App::view_mut_of`, which is the one door such a write goes
through, so the held-key path still hashes exactly one view: the wide-observation
cost improvise-mcq4 declined is not paid.
**Fault injected**: the gate restored to `if !host_moved { return Vec::new(); }`.
The test fails on `a client on the reframed view was never told`.
Two negative arms, and they are not decoration. A third client left on `Default`
must keep its offset, or "invalidate everyone" passes; and
`paging_with_nothing_pruned_invalidates_no_cursor` is what forbids relaying
against `last_reframed_views`, which is a union with the *reshaped* set and so
fires for a page selection that pruned nothing. That test failed during
development and is why the elsewhere set is precise.
### a_command_naming_a_view_the_document_has_not_got_writes_nothing
`App::resolve_view` falls back to the document's default for a name the document
has not got, which is right for *reading* — a slice naming a deleted view must
not panic in `rebuild_layout` — and wrong for **writing**, where it would move a
view nobody asked about. `view_mut_of` therefore looks the name up directly and
returns `None`.
**Fault injected**: `view_mut_of`'s `Named` arm routed through
`self.resolve_view(name).0`. The test fails on `a name the document has not got
fell back onto a view that has one`.
The window this closes is narrow — `Sessions::adopt_deleted_views` already moves
a mirror off a deleted view and tells it — but it is a race, not an
impossibility, and the silent alternative is a write to the wrong document state.
Not closed here: the client hears nothing. A `Rejection` variant for it is filed
rather than smuggled in, since `Rejection` is wire vocabulary.
crates/improvise-command/src/command/cmd/cell.rs
+2 -2
View File
@@ -1,5 +1,5 @@
use crate::model::Aggregation;
use crate::ui::effect::{self, Effect};
use crate::ui::effect::{self, Effect, ViewTarget};
use super::core::{Cmd, CmdContext};
@@ -178,7 +178,7 @@ impl Cmd for TransposeAxes {
"transpose"
}
fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
vec![Box::new(effect::TransposeAxes)]
vec![Box::new(effect::TransposeAxes(ViewTarget::Active))]
}
}
crates/improvise-command/src/command/cmd/effect_cmds.rs
+10 -2
View File
@@ -1,5 +1,5 @@
use crate::model::cell::CellValue;
use crate::ui::effect::{self, Effect};
use crate::ui::effect::{self, Effect, ViewTarget};
use crate::view::Axis;
use super::core::{Cmd, CmdContext, require_args};
@@ -283,6 +283,7 @@ effect_cmd!(
_ => return vec![], // parse step already validated
};
vec![Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: args[0].clone(),
axis,
})]
@@ -295,6 +296,7 @@ effect_cmd!(
|args: &[String]| require_args("set-page", args, 2),
|args: &Vec<String>, _ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
vec![Box::new(effect::SetPageSelection {
view: ViewTarget::Active,
category: args[0].clone(),
item: args[1].clone(),
})]
@@ -307,6 +309,7 @@ effect_cmd!(
|args: &[String]| require_args("toggle-group", args, 2),
|args: &Vec<String>, _ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
vec![Box::new(effect::ToggleGroup {
view: ViewTarget::Active,
category: args[0].clone(),
group: args[1].clone(),
})]
@@ -319,6 +322,7 @@ effect_cmd!(
|args: &[String]| require_args("hide-item", args, 2),
|args: &Vec<String>, _ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
vec![Box::new(effect::HideItem {
view: ViewTarget::Active,
category: args[0].clone(),
item: args[1].clone(),
})]
@@ -331,6 +335,7 @@ effect_cmd!(
|args: &[String]| require_args("show-item", args, 2),
|args: &Vec<String>, _ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
vec![Box::new(effect::ShowItem {
view: ViewTarget::Active,
category: args[0].clone(),
item: args[1].clone(),
})]
@@ -351,7 +356,10 @@ effect_cmd!(
"set-format",
|args: &[String]| require_args("set-format", args, 1),
|args: &Vec<String>, _ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
vec![Box::new(effect::SetNumberFormat(args.join(" ")))]
vec![Box::new(effect::SetNumberFormat(
ViewTarget::Active,
args.join(" "),
))]
}
);
crates/improvise-command/src/command/cmd/grid.rs
+14 -4
View File
@@ -1,7 +1,7 @@
use crate::model::category::{CategoryKind, DIM_CATEGORY, INDEX_CATEGORY, MEASURE_CATEGORY};
use crate::model::cell::CellValue;
use crate::ui::app::AppMode;
use crate::ui::effect::{self, Effect};
use crate::ui::effect::{self, Effect, ViewTarget};
use crate::view::AxisEntry;
use super::core::{Cmd, CmdContext};
@@ -223,11 +223,11 @@ mod tests {
let dbg = effects_debug(&effects);
assert!(
dbg.contains(r#"SetAxis { category: "Region", axis: Filter }"#),
dbg.contains(r#"SetAxis { view: Active, category: "Region", axis: Filter }"#),
"the drilled coordinate should be fixed on Filter, got: {dbg}"
);
assert!(
!dbg.contains(r#"SetAxis { category: "Region", axis: Page }"#),
!dbg.contains(r#"SetAxis { view: Active, category: "Region", axis: Page }"#),
"and must not also be offered as a page dimension, got: {dbg}"
);
}
@@ -258,6 +258,7 @@ impl Cmd for ToggleGroupAtCursor {
return vec![];
};
vec![Box::new(effect::ToggleGroup {
view: ViewTarget::Active,
category: cat,
group,
})]
@@ -293,6 +294,7 @@ impl Cmd for HideSelectedRowItem {
};
let item_name = items[0].clone();
vec![Box::new(effect::HideItem {
view: ViewTarget::Active,
category: cat_name,
item: item_name,
})]
@@ -390,10 +392,12 @@ impl Cmd for DrillIntoCell {
// Records mode: _Index on Row, _Dim on Column
effects.push(Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: INDEX_CATEGORY.to_string(),
axis: crate::view::Axis::Row,
}));
effects.push(Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: DIM_CATEGORY.to_string(),
axis: crate::view::Axis::Column,
}));
@@ -404,10 +408,12 @@ impl Cmd for DrillIntoCell {
self.key.0.iter().map(|(c, _)| c.clone()).collect();
for (cat, item) in &self.key.0 {
effects.push(Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: cat.clone(),
axis: crate::view::Axis::Filter,
}));
effects.push(Box::new(effect::SetPageSelection {
view: ViewTarget::Active,
category: cat.clone(),
item: item.clone(),
}));
@@ -419,6 +425,7 @@ impl Cmd for DrillIntoCell {
continue;
}
effects.push(Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: cat.clone(),
axis: crate::view::Axis::None,
}));
@@ -439,7 +446,7 @@ impl Cmd for TogglePruneEmpty {
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let currently_on = ctx.view.prune_empty;
vec![
Box::new(effect::TogglePruneEmpty),
Box::new(effect::TogglePruneEmpty(ViewTarget::Active)),
effect::set_status(if currently_on {
"Showing all rows/columns"
} else {
@@ -481,16 +488,19 @@ impl Cmd for ToggleRecordsMode {
// _Index on Row, _Dim on Column, everything else -> None
effects.push(Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: INDEX_CATEGORY.to_string(),
axis: crate::view::Axis::Row,
}));
effects.push(Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: DIM_CATEGORY.to_string(),
axis: crate::view::Axis::Column,
}));
for name in ctx.model.categories.keys() {
if !CategoryKind::of_name(name).is_index_or_dim() {
effects.push(Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: name.clone(),
axis: crate::view::Axis::None,
}));
crates/improvise-command/src/command/cmd/navigation.rs
+3 -1
View File
@@ -1,4 +1,4 @@
use crate::ui::effect::{self, Effect};
use crate::ui::effect::{self, Effect, ViewTarget};
use super::core::{Cmd, CmdContext};
@@ -198,6 +198,7 @@ impl Cmd for PageNext {
.enumerate()
.map(|(i, (cat, items, _))| {
Box::new(effect::SetPageSelection {
view: ViewTarget::Active,
category: cat.clone(),
item: items[indices[i]].clone(),
}) as Box<dyn Effect>
@@ -235,6 +236,7 @@ impl Cmd for PagePrev {
.enumerate()
.map(|(i, (cat, items, _))| {
Box::new(effect::SetPageSelection {
view: ViewTarget::Active,
category: cat.clone(),
item: items[indices[i]].clone(),
}) as Box<dyn Effect>
crates/improvise-command/src/command/cmd/panel.rs
+4 -2
View File
@@ -1,5 +1,5 @@
use crate::ui::app::AppMode;
use crate::ui::effect::{self, Effect, Panel};
use crate::ui::effect::{self, Effect, Panel, ViewTarget};
use super::core::{Cmd, CmdContext};
@@ -509,7 +509,7 @@ impl Cmd for CycleAxisAtCursor {
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
if let Some(cat_name) = ctx.cat_at_cursor() {
vec![Box::new(effect::CycleAxis(cat_name))]
vec![Box::new(effect::CycleAxis(ViewTarget::Active, cat_name))]
} else {
vec![effect::set_status(
"Move cursor to a category header to change axis".to_string(),
@@ -569,10 +569,12 @@ impl Cmd for FilterToItem {
}) => {
vec![
Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: cat_name.clone(),
axis: crate::view::Axis::Page,
}),
Box::new(effect::SetPageSelection {
view: ViewTarget::Active,
category: cat_name.clone(),
item: item_name.clone(),
}),
crates/improvise-command/src/command/cmd/tile.rs
+3 -2
View File
@@ -1,4 +1,4 @@
use crate::ui::effect::{self, Effect};
use crate::ui::effect::{self, Effect, ViewTarget};
use crate::view::Axis;
use super::core::{Cmd, CmdContext};
@@ -124,10 +124,11 @@ impl Cmd for TileAxisOp {
};
let axis_effect: Box<dyn Effect> = match self.axis {
Some(axis) => Box::new(effect::SetAxis {
view: ViewTarget::Active,
category: name.to_string(),
axis,
}),
None => Box::new(effect::CycleAxis(name.to_string())),
None => Box::new(effect::CycleAxis(ViewTarget::Active, name.to_string())),
};
let status = format!("{} → {}", name, new_axis.plain_label());
vec![axis_effect, effect::set_status(status)]
crates/improvise-command/src/ui/app.rs
+73 -10
View File
@@ -21,7 +21,7 @@ use crate::format::parse_number_format;
use crate::import::wizard::ImportWizard;
use crate::model::cell::{CellKey, CellValue, DataStore};
use crate::persistence;
use crate::ui::effect::{EffectKind, ModelChange};
use crate::ui::effect::{EffectKind, ModelChange, ViewTarget};
use crate::ui::project::Dirty;
use crate::ui::viewport::{compute_col_widths, compute_row_header_width, compute_visible_cols};
use crate::view::{GridLayout, View};
@@ -328,6 +328,12 @@ pub struct App {
/// The views the **last** batch was observed to REFRAME: different rows or
/// columns, in a different order. What invalidates a cursor (improvise-ni6k).
last_reframed_views: std::collections::BTreeSet<String>,
/// The frame each **non-active** view had before this batch wrote it, captured
/// at the one door such a write goes through (improvise-lltt).
written_view_frames: std::collections::BTreeMap<String, u64>,
/// The non-active views the last batch reframed. The host's own cursor cannot
/// witness these, so `cursor_epoch` alone is blind to them.
last_reframed_elsewhere: std::collections::BTreeSet<String>,
/// **Config**: assembled at startup by `KeymapSet::default_keymaps()`. Read
/// every keypress, never mutated during a session.
keymap_set: KeymapSet,
@@ -382,6 +388,8 @@ impl App {
cursor_epoch: 0,
last_reshaped_views: std::collections::BTreeSet::new(),
last_reframed_views: std::collections::BTreeSet::new(),
written_view_frames: std::collections::BTreeMap::new(),
last_reframed_elsewhere: std::collections::BTreeSet::new(),
keymap_set: KeymapSet::default_keymaps(),
storage,
}
@@ -534,11 +542,53 @@ impl App {
self.resolve_view(&self.view_state.active_view).1
}
/// The same view, to mutate. View configuration is document state shared by
/// every client on that view, so this is where a geometry effect writes.
pub fn view_mut(&mut self) -> &mut View {
let name = self.view_name().to_string();
&mut self.model_state.workbook.views[&name]
/// The view a geometry effect names, to mutate. There is **no operandless
/// form**: the ambiguity improvise-lltt closed was exactly that one.
pub fn view_mut_of(&mut self, target: &ViewTarget) -> Option<&mut View> {
// A `Named` that the document has not got is a no-op, never a fallback:
// `resolve_view`'s default would move a view nobody asked about.
let name = match target {
ViewTarget::Active => self.view_name().to_string(),
ViewTarget::Named(name) => name.clone(),
};
self.remember_elsewhere(&name);
self.model_state.workbook.views.get_mut(&name)
}
/// Take the opening frame of a view this batch is about to write and the host
/// is not on — the lazy half of `apply_effects`' observation (improvise-lltt).
fn remember_elsewhere(&mut self, name: &str) {
let unseen = name != self.view_name()
&& !self.written_view_frames.contains_key(name)
&& self.model_state.workbook.views.contains_key(name);
if unseen {
let workbook = &self.model_state.workbook;
let frame = view_frame(&workbook.model, &workbook.views[name]);
self.written_view_frames.insert(name.to_string(), frame);
}
}
/// Which non-active views the last batch reframed. A **precise** set, unlike
/// [`last_reframed_views`](Self::last_reframed_views) — `context/code/command.md`.
pub fn last_reframed_elsewhere(&self) -> &std::collections::BTreeSet<String> {
&self.last_reframed_elsewhere
}
/// Diff every frame `remember_elsewhere` took. The audience for a view the
/// host is not on, which no cursor of the host's can witness.
fn observe_written_view_frames(&mut self) {
let before = std::mem::take(&mut self.written_view_frames);
let workbook = &self.model_state.workbook;
self.last_reframed_elsewhere = before
.into_iter()
.filter(|(name, was)| match workbook.views.get(name) {
Some(view) => view_frame(&workbook.model, view) != *was,
None => true,
})
.map(|(name, _)| name)
.collect();
let moved = self.last_reframed_elsewhere.clone();
self.last_reframed_views.extend(moved);
}
/// Send the active view's cursor to the origin after a shape change. Clients
@@ -751,6 +801,8 @@ impl App {
let mut frame_basis = moves_geometry.then(|| self.opening_view_frame_keyed());
let mut frames_basis = writes_cells.then(|| view_frames(&self.model_state.workbook));
self.last_reframed_views.clear();
self.last_reframed_elsewhere.clear();
self.written_view_frames.clear();
let mut dirty = Dirty::NOTHING;
for effect in effects {
let kind = effect.kind();
@@ -830,6 +882,7 @@ impl App {
if let Some(before) = records_before {
self.bump_changed_records(&before);
}
self.observe_written_view_frames();
self.observe_reshape(shapes_before);
// The audience's half, and over-reporting is the safe direction: this set
// is read only behind the epoch gate — `context/code/command.md`.
@@ -1268,6 +1321,7 @@ mod tests {
fn flip_to_q2(app: &mut App) {
app.apply_effects(vec![Box::new(super::super::effect::SetPageSelection {
view: ViewTarget::Active,
category: "Quarter".to_string(),
item: "Q2".to_string(),
})]);
@@ -1338,6 +1392,7 @@ mod tests {
// A batch that reframes nothing. With the guard removed, the opening
// basis is the FROZEN layout and the in-loop hash is the LIVE one.
app.apply_effects(vec![Box::new(super::super::effect::SetNumberFormat(
ViewTarget::Active,
",.3f".to_string(),
))]);
@@ -1370,7 +1425,10 @@ mod tests {
// changes nothing. Comparing across the switch is a near-certain mismatch.
app.apply_effects(vec![
Box::new(super::super::effect::SwitchView("Second".to_string())),
Box::new(super::super::effect::SetNumberFormat(",.2f".to_string())),
Box::new(super::super::effect::SetNumberFormat(
ViewTarget::Active,
",.2f".to_string(),
)),
]);
assert_eq!(
@@ -1413,7 +1471,7 @@ mod tests {
app.apply_effects(vec![
Box::new(super::super::effect::SwitchView("Second".to_string())),
Box::new(super::super::effect::TransposeAxes),
Box::new(super::super::effect::TransposeAxes(ViewTarget::Active)),
]);
assert_eq!(
@@ -1491,6 +1549,7 @@ mod tests {
let epoch = app.cursor_epoch();
app.apply_effects(vec![Box::new(super::super::effect::SetPageSelection {
view: ViewTarget::Active,
category: "Quarter".to_string(),
item: "Q1".to_string(),
})]);
@@ -3666,6 +3725,7 @@ mod basis_tiers {
assert_eq!(app.view().axis_of("Month"), Axis::Column);
app.apply_effects(vec![Box::new(SetAxis {
view: ViewTarget::Active,
category: "Month".into(),
axis: Axis::Row,
})]);
@@ -3694,8 +3754,10 @@ mod basis_tiers {
// Both arms, because the CellData half never left the loop: a test over
// the geometry arm alone cannot say whether the ordering rule is general.
let geometry: Vec<Box<dyn Effect>> =
vec![Box::new(SetNumberFormat(",.2f".into())), Box::new(Save)];
let geometry: Vec<Box<dyn Effect>> = vec![
Box::new(SetNumberFormat(ViewTarget::Active, ",.2f".into())),
Box::new(Save),
];
let data: Vec<Box<dyn Effect>> = vec![
Box::new(SetCell(cell("North", "Revenue"), CellValue::Number(99.0))),
Box::new(Save),
@@ -3909,6 +3971,7 @@ mod basis_tiers {
Box::new(AbortChain),
Box::new(SetCell(cell("South", "Revenue"), CellValue::Number(88.0))),
Box::new(SetAxis {
view: ViewTarget::Active,
category: "Month".into(),
axis: Axis::Column,
}),
crates/improvise-command/src/ui/effect.rs
+99 -37
View File
@@ -36,6 +36,14 @@ pub enum ModelChange {
WholeWorkbook,
}
/// Which view a geometry effect writes. The host's own key path means "the one
/// this app is on"; a wire command names one — `context/code/command.md`.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ViewTarget {
Active,
Named(String),
}
/// A discrete state change produced by a command.
/// Effects know how to apply themselves to the App.
pub trait Effect: Debug {
@@ -241,11 +249,12 @@ impl Effect for EnterEditAtCursor {
}
#[derive(Debug)]
pub struct TogglePruneEmpty;
pub struct TogglePruneEmpty(pub ViewTarget);
impl Effect for TogglePruneEmpty {
fn apply(&self, app: &mut App) {
let v = app.view_mut();
v.prune_empty = !v.prune_empty;
if let Some(v) = app.view_mut_of(&self.0) {
v.prune_empty = !v.prune_empty;
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -382,12 +391,15 @@ impl Effect for ViewForward {
#[derive(Debug)]
pub struct SetAxis {
pub view: ViewTarget,
pub category: String,
pub axis: Axis,
}
impl Effect for SetAxis {
fn apply(&self, app: &mut App) {
app.view_mut().set_axis(&self.category, self.axis);
if let Some(v) = app.view_mut_of(&self.view) {
v.set_axis(&self.category, self.axis);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -396,13 +408,15 @@ impl Effect for SetAxis {
#[derive(Debug)]
pub struct SetPageSelection {
pub view: ViewTarget,
pub category: String,
pub item: String,
}
impl Effect for SetPageSelection {
fn apply(&self, app: &mut App) {
app.view_mut()
.set_page_selection(&self.category, &self.item);
if let Some(v) = app.view_mut_of(&self.view) {
v.set_page_selection(&self.category, &self.item);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -411,13 +425,15 @@ impl Effect for SetPageSelection {
#[derive(Debug)]
pub struct ToggleGroup {
pub view: ViewTarget,
pub category: String,
pub group: String,
}
impl Effect for ToggleGroup {
fn apply(&self, app: &mut App) {
app.view_mut()
.toggle_group_collapse(&self.category, &self.group);
if let Some(v) = app.view_mut_of(&self.view) {
v.toggle_group_collapse(&self.category, &self.group);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -426,12 +442,15 @@ impl Effect for ToggleGroup {
#[derive(Debug)]
pub struct HideItem {
pub view: ViewTarget,
pub category: String,
pub item: String,
}
impl Effect for HideItem {
fn apply(&self, app: &mut App) {
app.view_mut().hide_item(&self.category, &self.item);
if let Some(v) = app.view_mut_of(&self.view) {
v.hide_item(&self.category, &self.item);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -440,12 +459,15 @@ impl Effect for HideItem {
#[derive(Debug)]
pub struct ShowItem {
pub view: ViewTarget,
pub category: String,
pub item: String,
}
impl Effect for ShowItem {
fn apply(&self, app: &mut App) {
app.view_mut().show_item(&self.category, &self.item);
if let Some(v) = app.view_mut_of(&self.view) {
v.show_item(&self.category, &self.item);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -453,10 +475,12 @@ impl Effect for ShowItem {
}
#[derive(Debug)]
pub struct TransposeAxes;
pub struct TransposeAxes(pub ViewTarget);
impl Effect for TransposeAxes {
fn apply(&self, app: &mut App) {
app.view_mut().transpose_axes();
if let Some(v) = app.view_mut_of(&self.0) {
v.transpose_axes();
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -464,10 +488,13 @@ impl Effect for TransposeAxes {
}
#[derive(Debug)]
pub struct CycleAxis(pub String);
pub struct CycleAxis(pub ViewTarget, pub String);
impl Effect for CycleAxis {
fn apply(&self, app: &mut App) {
app.view_mut().cycle_axis(&self.0);
let category = self.1.clone();
if let Some(v) = app.view_mut_of(&self.0) {
v.cycle_axis(&category);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -475,10 +502,13 @@ impl Effect for CycleAxis {
}
#[derive(Debug)]
pub struct SetNumberFormat(pub String);
pub struct SetNumberFormat(pub ViewTarget, pub String);
impl Effect for SetNumberFormat {
fn apply(&self, app: &mut App) {
app.view_mut().number_format = self.0.clone();
let format = self.1.clone();
if let Some(v) = app.view_mut_of(&self.0) {
v.number_format = format;
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -1362,15 +1392,19 @@ mod tests {
),
(
Box::new(SetAxis {
view: ViewTarget::Active,
category: "Type".into(),
axis: Axis::Page,
}),
geom,
),
(Box::new(TransposeAxes), geom),
(Box::new(TransposeAxes(ViewTarget::Active)), geom),
(Box::new(CreateView("Other".into())), geom),
(Box::new(SetNumberFormat(",.3f".into())), geom),
(Box::new(TogglePruneEmpty), geom),
(
Box::new(SetNumberFormat(ViewTarget::Active, ",.3f".into())),
geom,
),
(Box::new(TogglePruneEmpty(ViewTarget::Active)), geom),
];
for (effect, expected) in cases {
assert_eq!(
@@ -1420,29 +1454,34 @@ mod tests {
Box::new(CreateView("Second".into())),
Box::new(DeleteView("Default".into())),
Box::new(SetAxis {
view: ViewTarget::Active,
category: "Type".into(),
axis: Axis::Page,
}),
Box::new(SetPageSelection {
view: ViewTarget::Active,
category: "Type".into(),
item: "Clothing".into(),
}),
Box::new(ToggleGroup {
view: ViewTarget::Active,
category: "Type".into(),
group: "Wearables".into(),
}),
Box::new(HideItem {
view: ViewTarget::Active,
category: "Type".into(),
item: "Food".into(),
}),
Box::new(ShowItem {
view: ViewTarget::Active,
category: "Type".into(),
item: "Clothing".into(),
}),
Box::new(TransposeAxes),
Box::new(CycleAxis("Type".into())),
Box::new(SetNumberFormat(",.3f".into())),
Box::new(TogglePruneEmpty),
Box::new(TransposeAxes(ViewTarget::Active)),
Box::new(CycleAxis(ViewTarget::Active, "Type".into())),
Box::new(SetNumberFormat(ViewTarget::Active, ",.3f".into())),
Box::new(TogglePruneEmpty(ViewTarget::Active)),
];
// A COUNT, not a totality check: nothing forces a fourteenth here, and a
// probe variant proved it — improvise-5ubs, `context/code/command.md`.
@@ -1708,11 +1747,12 @@ mod tests {
// break, so it must still see the axis move.
app.apply_effects(vec![
Box::new(SetAxis {
view: ViewTarget::Active,
category: "Type".into(),
axis: Axis::Page,
}),
Box::new(AbortChain),
Box::new(TogglePruneEmpty),
Box::new(TogglePruneEmpty(ViewTarget::Active)),
]);
assert!(app.last_batch_aborted(), "the fixture must actually abort");
assert_eq!(
@@ -1731,6 +1771,7 @@ mod tests {
app.apply_effects(vec![
Box::new(AbortChain),
Box::new(SetAxis {
view: ViewTarget::Active,
category: "Type".into(),
axis: Axis::Page,
}),
@@ -1846,6 +1887,7 @@ mod tests {
fn set_axis_effect() {
let mut app = test_app();
SetAxis {
view: ViewTarget::Active,
category: "Type".to_string(),
axis: Axis::Page,
}
@@ -1875,7 +1917,7 @@ mod tests {
.into_iter()
.map(String::from)
.collect();
TransposeAxes.apply(&mut app);
TransposeAxes(ViewTarget::Active).apply(&mut app);
let row_after: Vec<String> = app
.model_state
.workbook
@@ -2481,12 +2523,14 @@ mod tests {
),
("SetAxis", nothing, || {
Box::new(SetAxis {
view: ViewTarget::Active,
category: "Type".into(),
axis: Axis::Column,
})
}),
("SetPageSelection", nothing, || {
Box::new(SetPageSelection {
view: ViewTarget::Active,
category: "Type".into(),
item: "Food".into(),
})
@@ -2501,6 +2545,7 @@ mod tests {
},
|| {
Box::new(ToggleGroup {
view: ViewTarget::Active,
category: "Type".into(),
group: "Baked".into(),
})
@@ -2508,6 +2553,7 @@ mod tests {
),
("HideItem", nothing, || {
Box::new(HideItem {
view: ViewTarget::Active,
category: "Type".into(),
item: "Food".into(),
})
@@ -2515,10 +2561,13 @@ mod tests {
(
"ShowItem",
|app| {
app.view_mut().hide_item("Type", "Food");
app.view_mut_of(&ViewTarget::Active)
.unwrap()
.hide_item("Type", "Food");
},
|| {
Box::new(ShowItem {
view: ViewTarget::Active,
category: "Type".into(),
item: "Food".into(),
})
@@ -2527,16 +2576,24 @@ mod tests {
(
"TransposeAxes",
|app| {
app.view_mut().set_axis("Type", Axis::Row);
app.view_mut().set_axis("Month", Axis::Column);
app.view_mut_of(&ViewTarget::Active)
.unwrap()
.set_axis("Type", Axis::Row);
app.view_mut_of(&ViewTarget::Active)
.unwrap()
.set_axis("Month", Axis::Column);
},
|| Box::new(TransposeAxes),
|| Box::new(TransposeAxes(ViewTarget::Active)),
),
("CycleAxis", nothing, || Box::new(CycleAxis("Type".into()))),
("SetNumberFormat", nothing, || {
Box::new(SetNumberFormat(",.4f".into()))
("CycleAxis", nothing, || {
Box::new(CycleAxis(ViewTarget::Active, "Type".into()))
}),
("SetNumberFormat", nothing, || {
Box::new(SetNumberFormat(ViewTarget::Active, ",.4f".into()))
}),
("TogglePruneEmpty", nothing, || {
Box::new(TogglePruneEmpty(ViewTarget::Active))
}),
("TogglePruneEmpty", nothing, || Box::new(TogglePruneEmpty)),
];
for (label, prepare, make) in cases {
@@ -2704,9 +2761,9 @@ mod tests {
fn toggle_prune_empty_effect() {
let mut app = test_app();
let before = app.model_state.workbook.active_view().prune_empty;
TogglePruneEmpty.apply(&mut app);
TogglePruneEmpty(ViewTarget::Active).apply(&mut app);
assert_ne!(app.model_state.workbook.active_view().prune_empty, before);
TogglePruneEmpty.apply(&mut app);
TogglePruneEmpty(ViewTarget::Active).apply(&mut app);
assert_eq!(app.model_state.workbook.active_view().prune_empty, before);
}
@@ -2748,7 +2805,7 @@ mod tests {
#[test]
fn set_number_format_effect() {
let mut app = test_app();
SetNumberFormat(",.2f".to_string()).apply(&mut app);
SetNumberFormat(ViewTarget::Active, ",.2f".to_string()).apply(&mut app);
assert_eq!(app.model_state.workbook.active_view().number_format, ",.2f");
}
@@ -2758,6 +2815,7 @@ mod tests {
fn set_page_selection_effect() {
let mut app = test_app();
SetPageSelection {
view: ViewTarget::Active,
category: "Type".to_string(),
item: "Food".to_string(),
}
@@ -2777,6 +2835,7 @@ mod tests {
fn hide_and_show_item_effects() {
let mut app = test_app();
HideItem {
view: ViewTarget::Active,
category: "Type".to_string(),
item: "Food".to_string(),
}
@@ -2789,6 +2848,7 @@ mod tests {
);
ShowItem {
view: ViewTarget::Active,
category: "Type".to_string(),
item: "Food".to_string(),
}
@@ -2807,6 +2867,7 @@ mod tests {
fn toggle_group_effect() {
let mut app = test_app();
ToggleGroup {
view: ViewTarget::Active,
category: "Type".to_string(),
group: "MyGroup".to_string(),
}
@@ -2818,6 +2879,7 @@ mod tests {
.is_group_collapsed("Type", "MyGroup")
);
ToggleGroup {
view: ViewTarget::Active,
category: "Type".to_string(),
group: "MyGroup".to_string(),
}
@@ -2836,7 +2898,7 @@ mod tests {
fn cycle_axis_effect() {
let mut app = test_app();
let before = app.model_state.workbook.active_view().axis_of("Type");
CycleAxis("Type".to_string()).apply(&mut app);
CycleAxis(ViewTarget::Active, "Type".to_string()).apply(&mut app);
let after = app.model_state.workbook.active_view().axis_of("Type");
assert_ne!(before, after);
}
crates/improvise-command/src/ui/host.rs
+2 -1
View File
@@ -890,7 +890,8 @@ mod tests {
let (asker, _) = host.connect(10, 10);
let (bystander, _) = host.connect(10, 10);
let frames = host.model(asker, ModelCommand::TransposeAxes);
let view = host.app().view_name().to_string();
let frames = host.model(asker, ModelCommand::TransposeAxes { view });
let reset = ServerFrame::Commands(vec![Command::ResetCursor]);
for (who, id) in [("asker", asker), ("bystander", bystander)] {
crates/improvise-command/src/ui/project.rs
+186 -17
View File
@@ -972,12 +972,19 @@ impl Sessions {
/// If the batch renumbered the grid, invalidate the mirrors that were looking
/// at it **and** say so on the wire; neither half works alone (improvise-gvnb).
fn adopt_cursor_epoch(&mut self, app: &App, before: u64) -> Vec<Broadcast> {
if app.cursor_epoch() == before {
// The epoch is the HOST's cursor, so it is blind to a view the host is not
// on — which a view-named command reframes (improvise-lltt).
let host_moved = app.cursor_epoch() != before;
let elsewhere = app.last_reframed_elsewhere();
if !host_moved && elsewhere.is_empty() {
return Vec::new();
}
// The REFRAME set, not the reshape set: a cursor is invalidated by the
// rows moving, which is a different question from the basis's (ni6k).
let reshaped = app.last_reframed_views();
let reshaped = match host_moved {
true => app.last_reframed_views(),
false => elsewhere,
};
self.sessions
.iter_mut()
.filter(|(_, session)| cursor_invalidated(reshaped, &session.view.active_view))
@@ -1053,7 +1060,7 @@ fn fresh_view(app: &App) -> ViewState {
mod tests {
use super::*;
use crate::model::cell::{CellKey, CellValue};
use crate::ui::effect::{AddFormula, Effect, RemoveCategory, SetAxis, SetCell};
use crate::ui::effect::{AddFormula, Effect, RemoveCategory, SetAxis, SetCell, ViewTarget};
use crate::workbook::Workbook;
use improvise_protocol::{
BasisCache, CategoryTreeCache, DocumentCache, FormulaCache, TileBarCache, ViewListCache,
@@ -1495,6 +1502,7 @@ mod tests {
target_category: "_Measure".to_string(),
})],
vec![Box::new(SetAxis {
view: ViewTarget::Active,
category: "Region".to_string(),
axis: Axis::Column,
})],
@@ -2153,7 +2161,9 @@ mod tests {
sessions.apply(&mut app, asker, &Command::SetRowOffset(1));
sessions.apply(&mut app, bystander, &Command::SetRowOffset(2));
let applied = sessions.apply_caught_up(&mut app, asker, ModelCommand::TransposeAxes);
let view = app.view_name().to_string();
let applied =
sessions.apply_caught_up(&mut app, asker, ModelCommand::TransposeAxes { view });
assert_eq!(
sessions
@@ -2542,26 +2552,30 @@ mod tests {
/// The commands that renumber the grid. A **shared** table: the cursor audience
/// and the two shape counters must answer for the same cases (improvise-pdvm).
fn reshaping_commands() -> Vec<improvise_protocol::ModelCommand> {
fn reshaping_commands(view: &str) -> Vec<improvise_protocol::ModelCommand> {
use improvise_protocol::ModelCommand;
vec![
ModelCommand::SetAxis {
view: view.into(),
category: "Region".into(),
axis: Axis::Column,
},
ModelCommand::HideItem {
view: view.into(),
category: "Region".into(),
item: "North".into(),
},
ModelCommand::ShowItem {
view: view.into(),
category: "Product".into(),
item: "Gadget".into(),
},
ModelCommand::ToggleGroup {
view: view.into(),
category: "Product".into(),
group: "Hardware".into(),
},
ModelCommand::TogglePruneEmpty,
ModelCommand::TogglePruneEmpty { view: view.into() },
ModelCommand::AddCategory("Zone".into()),
ModelCommand::RemoveCategory("Region".into()),
ModelCommand::AddItem {
@@ -2594,7 +2608,8 @@ mod tests {
/// the case is only evidence if the grid really moved (improvise-9mo9).
#[test]
fn every_reshaping_model_command_invalidates_every_cursor() {
for cmd in reshaping_commands() {
let named = app_with_a_reshapeable_grid();
for cmd in reshaping_commands(named.view_name()) {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
let before = grid_shape(&app);
@@ -2634,18 +2649,21 @@ mod tests {
/// The commands the document ACCEPTS while changing nothing. Shared with the
/// counter test for the reason `reshaping_commands` is (improvise-pdvm).
fn no_op_commands() -> Vec<improvise_protocol::ModelCommand> {
fn no_op_commands(view: &str) -> Vec<improvise_protocol::ModelCommand> {
use improvise_protocol::ModelCommand;
vec![
ModelCommand::SetAxis {
view: view.into(),
category: "Region".into(),
axis: Axis::Row,
},
ModelCommand::HideItem {
view: view.into(),
category: "Product".into(),
item: "Gadget".into(),
},
ModelCommand::ShowItem {
view: view.into(),
category: "Product".into(),
item: "Widget".into(),
},
@@ -2666,18 +2684,22 @@ mod tests {
// Review's cases: a name the grid never drew, and a `_Measure` item
// that exists only as a formula target (gotcha 7).
ModelCommand::SetAxis {
view: view.into(),
category: "NoSuchCategory".into(),
axis: Axis::Row,
},
ModelCommand::HideItem {
view: view.into(),
category: "Region".into(),
item: "NoSuchItem".into(),
},
ModelCommand::ShowItem {
view: view.into(),
category: "Region".into(),
item: "Ghost".into(),
},
ModelCommand::ToggleGroup {
view: view.into(),
category: "Product".into(),
group: "NoSuchGroup".into(),
},
@@ -2692,7 +2714,8 @@ mod tests {
/// nothing reshaped nothing either (found in review of improvise-9mo9).
#[test]
fn an_accepted_no_op_invalidates_no_cursor() {
for cmd in no_op_commands() {
let named = app_with_a_reshapeable_grid();
for cmd in no_op_commands(named.view_name()) {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
let before = grid_shape(&app);
@@ -2775,10 +2798,12 @@ mod tests {
let (bystander, _) = sessions.connect(&app, 100, 100);
sessions.apply(&mut app, bystander, &Command::SetRowOffset(1));
let view = app.view_name().to_string();
let applied = sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::SetPageSelection {
view: view.clone(),
category: "Quarter".into(),
item: "Q2".into(),
},
@@ -2824,10 +2849,12 @@ mod tests {
let (bystander, _) = sessions.connect(&app, 100, 100);
sessions.apply(&mut app, bystander, &Command::SetRowOffset(1));
let view = app.view_name().to_string();
let applied = sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::SetPageSelection {
view: view.clone(),
category: "Quarter".into(),
item: "Q2".into(),
},
@@ -3036,6 +3063,132 @@ mod tests {
app
}
/// improvise-lltt: the command names its view, so it lands on the asker's view
/// and not on the host's. See `context/test-evidence/command.md`.
#[test]
fn a_view_named_command_lands_on_the_view_it_names() {
use improvise_protocol::ModelCommand;
let mut app = app_with_two_reshapeable_views();
let mut sessions = Sessions::default();
let (asker, _) = sessions.connect(&app, 100, 100);
sessions.apply(&mut app, asker, &Command::SwitchView("Second".into()));
assert_eq!(
app.view_name(),
"Default",
"the host stays on its own view: that split is what makes this ambiguous"
);
sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::SetAxis {
view: "Second".into(),
category: "Channel".into(),
axis: Axis::Page,
},
);
let views = &app.model_state.workbook.views;
assert_eq!(
views["Second"].axis_of("Channel"),
Axis::Page,
"the asker's own view never moved"
);
assert_eq!(
views["Default"].axis_of("Channel"),
Axis::None,
"the axis moved on the view the asker is NOT on"
);
}
/// improvise-lltt: naming a view the document has not got writes nothing —
/// `resolve_view`'s fallback would move the default instead.
#[test]
fn a_command_naming_a_view_the_document_has_not_got_writes_nothing() {
use improvise_protocol::ModelCommand;
let mut app = app_with_two_reshapeable_views();
let before = format!("{:?}", app.model_state.workbook.views);
let mut sessions = Sessions::default();
let (asker, _) = sessions.connect(&app, 100, 100);
sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::SetAxis {
view: "NoSuchView".into(),
category: "Channel".into(),
axis: Axis::Page,
},
);
assert_eq!(
format!("{:?}", app.model_state.workbook.views),
before,
"a name the document has not got fell back onto a view that has one"
);
}
/// improvise-lltt: `cursor_epoch` is the host's own cursor, so it cannot
/// witness a reframe elsewhere. See `context/test-evidence/command.md`.
#[test]
fn a_reframe_of_a_view_the_host_is_not_on_invalidates_its_clients() {
use improvise_protocol::ModelCommand;
let mut app = app_with_two_reshapeable_views();
let mut sessions = Sessions::default();
let (asker, _) = sessions.connect(&app, 100, 100);
let (bystander, _) = sessions.connect(&app, 100, 100);
let (at_home, _) = sessions.connect(&app, 100, 100);
for who in [asker, bystander] {
sessions.apply(&mut app, who, &Command::SwitchView("Second".into()));
}
sessions.apply(&mut app, bystander, &Command::SetRowOffset(1));
sessions.apply(&mut app, at_home, &Command::SetRowOffset(2));
let applied = sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::HideItem {
view: "Second".into(),
category: "Channel".into(),
item: "Direct".into(),
},
);
let broadcast = diff(applied).1;
assert!(
broadcast.contains(&Broadcast::To(bystander, Command::ResetCursor)),
"a client on the reframed view was never told: {broadcast:?}"
);
assert_eq!(
sessions
.get_mut(bystander)
.unwrap()
.view()
.cursor()
.row_offset,
0,
"the mirror kept an offset into a grid that no longer has those rows"
);
// The negative arm, or "invalidate everyone" passes this test.
assert!(
!broadcast.contains(&Broadcast::To(at_home, Command::ResetCursor)),
"a client on `Default` was invalidated by a reframe of `Second`"
);
assert_eq!(
sessions
.get_mut(at_home)
.unwrap()
.view()
.cursor()
.row_offset,
2,
"a client on `Default` lost its place to a reframe of `Second`"
);
}
/// improvise-nynk: which view a client is on is that client's, so a reshape of
/// one view is not addressed to a client sitting on another.
#[test]
@@ -3056,12 +3209,13 @@ mod tests {
"the bystander never left the view the reshape is aimed at"
);
// Aimed at the host's own view, which is `Default`: `HideItem` writes
// `view_mut()`, so `Second`'s shape is untouched by construction.
// Aimed at `Default` BY NAME since improvise-lltt, so `Second`'s shape is
// untouched — the operand, not an ambient one.
let applied = sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::HideItem {
view: "Default".into(),
category: "Region".into(),
item: "North".into(),
},
@@ -3120,7 +3274,8 @@ mod tests {
/// -- "has the grid's shape moved under you" -- so they answer over one table.
#[test]
fn both_shape_counters_move_for_every_reshaping_command() {
for cmd in reshaping_commands() {
let named = app_with_a_reshapeable_grid();
for cmd in reshaping_commands(named.view_name()) {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
app.model_state.dirty = false;
@@ -3165,10 +3320,14 @@ mod tests {
// number format is written to the file while changing no row or column.
let config_only = vec![
ModelCommand::HideItem {
view: "Default".into(),
category: "Region".into(),
item: "NoSuchItem".into(),
},
ModelCommand::SetNumberFormat(",.4f".into()),
ModelCommand::SetNumberFormat {
view: "Default".into(),
format: ",.4f".into(),
},
];
for cmd in config_only {
@@ -3204,7 +3363,8 @@ mod tests {
/// moves neither counter and does not raise `dirty` (improvise-75ld).
#[test]
fn neither_shape_counter_moves_for_an_accepted_no_op() {
for cmd in no_op_commands() {
let named = app_with_a_reshapeable_grid();
for cmd in no_op_commands(named.view_name()) {
let label = format!("{cmd:?}");
let mut app = app_with_a_reshapeable_grid();
app.model_state.dirty = false;
@@ -3243,10 +3403,14 @@ mod tests {
sessions.apply(&mut app, bystander, &Command::SetRowOffset(2));
let view = app.view_name().to_string();
let applied = sessions.apply_caught_up(
&mut app,
asker,
ModelCommand::SetNumberFormat(",.2f".into()),
ModelCommand::SetNumberFormat {
view,
format: ",.2f".into(),
},
);
assert_eq!(
@@ -3280,7 +3444,9 @@ mod tests {
host_dispatch(&mut client, &applied, bystander);
assert_eq!(client.cursor().row_offset, 2);
let applied = sessions.apply_caught_up(&mut app, asker, ModelCommand::TransposeAxes);
let view = app.view_name().to_string();
let applied =
sessions.apply_caught_up(&mut app, asker, ModelCommand::TransposeAxes { view });
host_dispatch(&mut client, &applied, bystander);
assert_eq!(
@@ -3620,6 +3786,7 @@ mod tests {
vec![
Box::new(crate::ui::effect::AbortChain),
Box::new(SetAxis {
view: ViewTarget::Active,
category: "Region".to_string(),
axis: Axis::Column,
}),
@@ -4405,7 +4572,7 @@ mod cell_identity {
mod basis_check {
use super::*;
use crate::model::cell::{CellKey, CellValue};
use crate::ui::effect::{RemoveItem, SetAxis, SetCell};
use crate::ui::effect::{RemoveItem, SetAxis, SetCell, ViewTarget};
use crate::workbook::Workbook;
use improvise_core::view::Axis;
use improvise_protocol::{Basis, ModelCommand};
@@ -4508,6 +4675,7 @@ mod basis_check {
// A reshapes the grid. No record B named is written by this.
app.apply_effects(vec![Box::new(SetAxis {
view: ViewTarget::Active,
category: "Region".to_string(),
axis: Axis::Column,
})]);
@@ -4531,6 +4699,7 @@ mod basis_check {
let (id, _) = sessions.connect(&app, 100, 100);
app.apply_effects(vec![Box::new(SetAxis {
view: ViewTarget::Active,
category: "Region".to_string(),
axis: Axis::Column,
})]);
crates/improvise-command/src/ui/reduce.rs
+94 -23
View File
@@ -7,7 +7,7 @@ use improvise_protocol::{Basis, Command, ModelCommand, Rejection};
use crate::command::key::KeyEvent;
use crate::ui::app::App;
use crate::ui::effect::{self, Effect};
use crate::ui::effect::{self, Effect, ViewTarget};
use crate::ui::project::{Dirty, document_projections};
/// A wire [`Command`] whose basis the authoritative state has answered for
@@ -166,30 +166,65 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec<Box<dyn Effect>> {
ModelCommand::AddCategory(name) => Box::new(effect::AddCategory(name.clone())),
ModelCommand::RemoveCategory(name) => Box::new(effect::RemoveCategory(name.clone())),
ModelCommand::SetAxis { category, axis } => Box::new(effect::SetAxis {
ModelCommand::SetAxis {
view,
category,
axis,
} => Box::new(effect::SetAxis {
view: ViewTarget::Named(view.clone()),
category: category.clone(),
axis: *axis,
}),
ModelCommand::CycleAxis(category) => Box::new(effect::CycleAxis(category.clone())),
ModelCommand::TransposeAxes => Box::new(effect::TransposeAxes),
ModelCommand::SetPageSelection { category, item } => Box::new(effect::SetPageSelection {
ModelCommand::CycleAxis { view, category } => Box::new(effect::CycleAxis(
ViewTarget::Named(view.clone()),
category.clone(),
)),
ModelCommand::TransposeAxes { view } => {
Box::new(effect::TransposeAxes(ViewTarget::Named(view.clone())))
}
ModelCommand::SetPageSelection {
view,
category,
item,
} => Box::new(effect::SetPageSelection {
view: ViewTarget::Named(view.clone()),
category: category.clone(),
item: item.clone(),
}),
ModelCommand::HideItem { category, item } => Box::new(effect::HideItem {
ModelCommand::HideItem {
view,
category,
item,
} => Box::new(effect::HideItem {
view: ViewTarget::Named(view.clone()),
category: category.clone(),
item: item.clone(),
}),
ModelCommand::ShowItem { category, item } => Box::new(effect::ShowItem {
ModelCommand::ShowItem {
view,
category,
item,
} => Box::new(effect::ShowItem {
view: ViewTarget::Named(view.clone()),
category: category.clone(),
item: item.clone(),
}),
ModelCommand::ToggleGroup { category, group } => Box::new(effect::ToggleGroup {
ModelCommand::ToggleGroup {
view,
category,
group,
} => Box::new(effect::ToggleGroup {
view: ViewTarget::Named(view.clone()),
category: category.clone(),
group: group.clone(),
}),
ModelCommand::TogglePruneEmpty => Box::new(effect::TogglePruneEmpty),
ModelCommand::SetNumberFormat(fmt) => Box::new(effect::SetNumberFormat(fmt.clone())),
ModelCommand::TogglePruneEmpty { view } => {
Box::new(effect::TogglePruneEmpty(ViewTarget::Named(view.clone())))
}
ModelCommand::SetNumberFormat { view, format } => Box::new(effect::SetNumberFormat(
ViewTarget::Named(view.clone()),
format.clone(),
)),
ModelCommand::CreateView(name) => Box::new(effect::CreateView(name.clone())),
ModelCommand::DeleteView(name) => Box::new(effect::DeleteView(name.clone())),
@@ -527,29 +562,44 @@ mod tests {
ModelCommand::AddCategory("Product".into()),
ModelCommand::RemoveCategory("Region".into()),
ModelCommand::SetAxis {
view: "Default".into(),
category: "Region".into(),
axis: Axis::Row,
},
ModelCommand::CycleAxis("Region".into()),
ModelCommand::TransposeAxes,
ModelCommand::CycleAxis {
view: "Default".into(),
category: "Region".into(),
},
ModelCommand::TransposeAxes {
view: "Default".into(),
},
ModelCommand::SetPageSelection {
view: "Default".into(),
category: "Region".into(),
item: "North".into(),
},
ModelCommand::HideItem {
view: "Default".into(),
category: "Region".into(),
item: "North".into(),
},
ModelCommand::ShowItem {
view: "Default".into(),
category: "Region".into(),
item: "North".into(),
},
ModelCommand::ToggleGroup {
view: "Default".into(),
category: "Region".into(),
group: "Coastal".into(),
},
ModelCommand::TogglePruneEmpty,
ModelCommand::SetNumberFormat(",.2f".into()),
ModelCommand::TogglePruneEmpty {
view: "Default".into(),
},
ModelCommand::SetNumberFormat {
view: "Default".into(),
format: ",.2f".into(),
},
ModelCommand::CreateView("Third".into()),
ModelCommand::DeleteView("Second".into()),
ModelCommand::LoadModel(PathBuf::from("doc.improv")),
@@ -576,14 +626,14 @@ mod tests {
ModelCommand::AddCategory(_) => "AddCategory",
ModelCommand::RemoveCategory(_) => "RemoveCategory",
ModelCommand::SetAxis { .. } => "SetAxis",
ModelCommand::CycleAxis(_) => "CycleAxis",
ModelCommand::TransposeAxes => "TransposeAxes",
ModelCommand::CycleAxis { .. } => "CycleAxis",
ModelCommand::TransposeAxes { .. } => "TransposeAxes",
ModelCommand::SetPageSelection { .. } => "SetPageSelection",
ModelCommand::HideItem { .. } => "HideItem",
ModelCommand::ShowItem { .. } => "ShowItem",
ModelCommand::ToggleGroup { .. } => "ToggleGroup",
ModelCommand::TogglePruneEmpty => "TogglePruneEmpty",
ModelCommand::SetNumberFormat(_) => "SetNumberFormat",
ModelCommand::TogglePruneEmpty { .. } => "TogglePruneEmpty",
ModelCommand::SetNumberFormat { .. } => "SetNumberFormat",
ModelCommand::CreateView(_) => "CreateView",
ModelCommand::DeleteView(_) => "DeleteView",
ModelCommand::LoadModel(_) => "LoadModel",
@@ -670,13 +720,17 @@ mod tests {
/// effect of each `Debug` shape — nothing, `(`, and ` {` after the name.
#[test]
fn the_effect_name_reader_reads_all_three_debug_shapes() {
assert_eq!(effect_type_name(&effect::TransposeAxes), "TransposeAxes");
assert_eq!(
effect_type_name(&effect::TransposeAxes(ViewTarget::Active)),
"TransposeAxes"
);
assert_eq!(
effect_type_name(&effect::AddCategory("Region".into())),
"AddCategory"
);
assert_eq!(
effect_type_name(&effect::HideItem {
view: ViewTarget::Active,
category: "Region".into(),
item: "North".into(),
}),
@@ -748,12 +802,22 @@ mod tests {
assert_eq!(
by_key(char_key('t'), plain),
by_command(ModelCommand::TransposeAxes, plain),
by_command(
ModelCommand::TransposeAxes {
view: "Default".into(),
},
plain
),
"`t` and Model(TransposeAxes)"
);
assert_eq!(
by_key(char_key('P'), plain),
by_command(ModelCommand::TogglePruneEmpty, plain),
by_command(
ModelCommand::TogglePruneEmpty {
view: "Default".into(),
},
plain
),
"`P` and Model(TogglePruneEmpty)"
);
@@ -787,7 +851,12 @@ mod tests {
// The layout moves under the client's feet, which is the tier
// `Sessions::apply` answers with `Rejection::LayoutMoved`.
let bump = caught_up(&app, ModelCommand::TransposeAxes);
let bump = caught_up(
&app,
ModelCommand::TransposeAxes {
view: app.view_name().to_string(),
},
);
apply_checked(&mut app, &bump);
let refused = Checked::against(&app, &wire).err();
@@ -833,7 +902,9 @@ mod tests {
value: CellValue::Number(1.0),
},
ModelCommand::AddCategory("Product".into()),
ModelCommand::TransposeAxes,
ModelCommand::TransposeAxes {
view: "Default".into(),
},
] {
let mut app = app();
assert!(!app.model_state.dirty, "the fixture starts clean");
crates/improvise-protocol/src/reduce.rs
+35 -12
View File
@@ -67,8 +67,8 @@ pub enum Command {
Project(Projection),
}
/// A mutation of the shared document: the 26 `EffectKind::Model(..)` effects
/// minus two, leaving **24**. Derived, not invented — see §2.
/// A mutation of the shared document: 24 variants, derived not invented (§2). A
/// view-configuration variant **names its view** (improvise-lltt).
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub enum ModelCommand {
// ── ModelChange::CellData ───────────────────────────────────────
@@ -106,29 +106,44 @@ pub enum ModelCommand {
AddCategory(String),
RemoveCategory(String),
SetAxis {
view: String,
category: String,
axis: Axis,
},
CycleAxis(String),
TransposeAxes,
CycleAxis {
view: String,
category: String,
},
TransposeAxes {
view: String,
},
SetPageSelection {
view: String,
category: String,
item: String,
},
HideItem {
view: String,
category: String,
item: String,
},
ShowItem {
view: String,
category: String,
item: String,
},
ToggleGroup {
view: String,
category: String,
group: String,
},
TogglePruneEmpty,
SetNumberFormat(String),
TogglePruneEmpty {
view: String,
},
SetNumberFormat {
view: String,
format: String,
},
CreateView(String),
DeleteView(String),
@@ -158,14 +173,14 @@ pub fn records_named(cmd: &ModelCommand) -> Vec<RecordHash> {
| ModelCommand::AddCategory(_)
| ModelCommand::RemoveCategory(_)
| ModelCommand::SetAxis { .. }
| ModelCommand::CycleAxis(_)
| ModelCommand::TransposeAxes
| ModelCommand::CycleAxis { .. }
| ModelCommand::TransposeAxes { .. }
| ModelCommand::SetPageSelection { .. }
| ModelCommand::HideItem { .. }
| ModelCommand::ShowItem { .. }
| ModelCommand::ToggleGroup { .. }
| ModelCommand::TogglePruneEmpty
| ModelCommand::SetNumberFormat(_)
| ModelCommand::TogglePruneEmpty { .. }
| ModelCommand::SetNumberFormat { .. }
| ModelCommand::CreateView(_)
| ModelCommand::DeleteView(_)
| ModelCommand::LoadModel(_)
@@ -369,7 +384,12 @@ mod tests {
&mut view,
&modelled(ModelCommand::AddCategory("Product".into())),
);
reduce_view(&mut view, &modelled(ModelCommand::TransposeAxes));
reduce_view(
&mut view,
&modelled(ModelCommand::TransposeAxes {
view: "Default".into(),
}),
);
assert_eq!(before, format!("{view:?}"));
}
@@ -435,6 +455,7 @@ mod tests {
value: CellValue::Text("pending".into()),
}),
modelled(ModelCommand::SetAxis {
view: "Default".into(),
category: "Region".into(),
axis: Axis::Filter,
}),
@@ -449,7 +470,9 @@ mod tests {
model_name: Some("Rows".into()),
array_path: None,
}),
modelled(ModelCommand::TransposeAxes),
modelled(ModelCommand::TransposeAxes {
view: "Default".into(),
}),
];
for cmd in &stream {
let json = serde_json::to_string(cmd).expect("serializes");
crates/improvise-protocol/src/resolve.rs
+28 -6
View File
@@ -330,7 +330,9 @@ fn composed(
// this client's to send; both are the host's — clients.md.
"transpose" => Some(vec![Command::Model {
basis: state.basis.claim([])?,
cmd: ModelCommand::TransposeAxes,
cmd: ModelCommand::TransposeAxes {
view: state.view.active_view.clone(),
},
}]),
// ── editing ──────────────────────────────────────────────────
@@ -342,7 +344,13 @@ fn composed(
// ── the category panel ───────────────────────────────────────
"cycle-axis-at-cursor" => Some(match cat_at_cursor(state) {
Some(cat) => vec![model(state, ModelCommand::CycleAxis(cat))?],
Some(cat) => vec![model(
state,
ModelCommand::CycleAxis {
view: state.view.active_view.clone(),
category: cat,
},
)?],
None => vec![Command::SetStatus(
"Move cursor to a category header to change axis".to_string(),
)],
@@ -653,6 +661,7 @@ fn page_cycle(state: &ClientState, way: Odometer) -> Option<Vec<Command>> {
Some(Command::Model {
basis: state.basis.claim([])?,
cmd: ModelCommand::SetPageSelection {
view: state.view.active_view.clone(),
category: (*cat).to_string(),
item: items[indices[i]].clone(),
},
@@ -908,6 +917,7 @@ fn filter_to_item_at(state: &ClientState, index: usize) -> Option<Vec<Command>>
model(
state,
ModelCommand::SetAxis {
view: state.view.active_view.clone(),
category: cat_name.clone(),
axis: Axis::Page,
},
@@ -915,6 +925,7 @@ fn filter_to_item_at(state: &ClientState, index: usize) -> Option<Vec<Command>>
model(
state,
ModelCommand::SetPageSelection {
view: state.view.active_view.clone(),
category: cat_name.clone(),
item: item_name.clone(),
},
@@ -1002,10 +1013,14 @@ fn tile_axis_at(state: &ClientState, index: usize, axis: Option<Axis>) -> Option
let new_axis = axis.unwrap_or_else(|| tile.axis.next());
let cmd = match axis {
Some(axis) => ModelCommand::SetAxis {
view: state.view.active_view.clone(),
category: tile.category.clone(),
axis,
},
None => ModelCommand::CycleAxis(tile.category.clone()),
None => ModelCommand::CycleAxis {
view: state.view.active_view.clone(),
category: tile.category.clone(),
},
};
Some(vec![
model(state, cmd)?,
@@ -1321,7 +1336,10 @@ mod tests {
vec![
Command::Model {
basis: state.basis.claim([]).unwrap(),
cmd: ModelCommand::CycleAxis("Product".into()),
cmd: ModelCommand::CycleAxis {
view: state.view.active_view.clone(),
category: "Product".into(),
},
},
// `Column.next()` is `Page` — the *new* axis, as `Enter` says.
Command::SetStatus("Product → Page".into()),
@@ -1404,6 +1422,7 @@ mod tests {
Command::Model {
basis: basis.clone(),
cmd: ModelCommand::SetAxis {
view: state.view.active_view.clone(),
category: "Region".into(),
axis: Axis::Page
},
@@ -1411,6 +1430,7 @@ mod tests {
Command::Model {
basis,
cmd: ModelCommand::SetPageSelection {
view: state.view.active_view.clone(),
category: "Region".into(),
item: "North".into()
},
@@ -2101,7 +2121,9 @@ mod tests {
resolve(&client, ev(KeyCode::Char('t'))),
KeyOutcome::Local(vec![Command::Model {
basis: client.basis.claim([]).unwrap(),
cmd: ModelCommand::TransposeAxes,
cmd: ModelCommand::TransposeAxes {
view: client.view.active_view.clone(),
},
}])
);
}
@@ -2316,7 +2338,7 @@ mod tests {
.into_iter()
.map(|c| match c {
Command::Model {
cmd: ModelCommand::SetPageSelection { category, item },
cmd: ModelCommand::SetPageSelection { category, item, .. },
..
} => (category, item),
other => panic!("expected a page selection, got {other:?}"),
crates/improvise-wasm-client/tests/lockstep.rs
+1
View File
@@ -473,6 +473,7 @@ fn the_fixtures_basis_tracks_a_host_whose_layout_has_moved() {
let bump = caught_up(
&app,
improvise_protocol::ModelCommand::HideItem {
view: app.view_name().to_string(),
category: "Region".into(),
item: "South".into(),
},
crates/improvise-worker-server/src/worker.rs
+8 -4
View File
@@ -358,9 +358,12 @@ mod tests {
// The frame's basis is the FIRST command's, answered once, so that is
// where the staleness must be injected.
let Command::Model { mut basis, cmd } =
reduce::caught_up(worker.app(), ModelCommand::TransposeAxes)
else {
let Command::Model { mut basis, cmd } = reduce::caught_up(
worker.app(),
ModelCommand::TransposeAxes {
view: worker.app().view_name().to_string(),
},
) else {
unreachable!("caught_up builds a Command::Model")
};
basis.document_generation += 1;
@@ -425,7 +428,8 @@ mod tests {
// Dirty the document, so the automatic trigger genuinely would fire
// if it were consulted. That write fails and is counted.
let wire = reduce::caught_up(worker.app(), ModelCommand::TransposeAxes);
let view = worker.app().view_name().to_string();
let wire = reduce::caught_up(worker.app(), ModelCommand::TransposeAxes { view });
worker.on_frame(ClientFrame::command(wire));
assert!(
worker.app().model_state.dirty,
crates/improvise-ws-server/src/registry.rs
+14 -2
View File
@@ -178,6 +178,16 @@ impl Registry {
self.handle(who, ClientFrame::command(wire))
}
/// The view this client's document is on, so a view-scoped command can name
/// one the document really has (improvise-lltt).
#[cfg(test)]
fn view_of(&self, who: &ClientId) -> String {
match self.documents.get(&who.document) {
Some(host) => host.app().view_name().to_string(),
None => String::new(),
}
}
/// One frame from one client, and everything that owes as a result —
/// within that client's document. An unopen document produces nothing.
pub fn handle(&mut self, who: &ClientId, msg: ClientFrame) -> Vec<Routed> {
@@ -453,7 +463,8 @@ mod tests {
let (same_doc, _) = registry.connect(Some("alpha"), 10, 10).unwrap();
let (other_doc, _) = registry.connect(Some("beta"), 10, 10).unwrap();
let frames = registry.model(&asker, ModelCommand::TransposeAxes);
let view = registry.view_of(&asker);
let frames = registry.model(&asker, ModelCommand::TransposeAxes { view });
let broadcast = ServerFrame::Commands(vec![Command::ResetCursor]);
assert!(
@@ -472,7 +483,8 @@ mod tests {
fn the_other_document_would_also_have_a_cursor_to_invalidate() {
let mut registry = registry();
let (elsewhere, _) = registry.connect(Some("beta"), 10, 10).unwrap();
let frames = registry.model(&elsewhere, ModelCommand::TransposeAxes);
let view = registry.view_of(&elsewhere);
let frames = registry.model(&elsewhere, ModelCommand::TransposeAxes { view });
assert!(
to(&frames, &elsewhere).contains(&&ServerFrame::Commands(vec![Command::ResetCursor])),
"the isolation test would pass for the wrong reason"