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

Merge pull request #193 from fiddlerwoaroof/fix/view-history

Browse Source 
improvise-lrrx/8nd9: view history does not survive a document swap, and a refused step leaves no frame
This commit is contained in:
Ed Langley
2026-09-22 22:37:55 -07:00
committed by GitHub
parent d1c4884399 c1932094d0
commit a2f3c8d3e5
4 changed files with 281 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/command.md
+34 -6
View File
@@ -793,12 +793,40 @@ is gone rather than joined by a second one. A guard spelled here as
copy of `switch_to_view`'s own condition, free to disagree with it the day that
condition grows a clause.
Not fixed, and reachable only by construction: `ViewBack` and `ViewForward` call
`switch_to_view` just as unguardedly, pushing the current view onto the opposite
stack before the switch they cannot be sure of. They are safe **because
`ViewState::forget_view` purges both stacks when a view is deleted**, so a popped
frame always names a live view — a guard living one level away from the hazard,
which is the shape that keeps returning here.
`ViewBack` and `ViewForward` have the same shape now (improvise-8nd9). They used to
push the current view onto the opposite stack **before** a switch they could not be
sure of, and were safe only because `ViewState::forget_view` purges both stacks when
a view is deleted — a guard living one level away from the hazard. improvise-lrrx
proved the guard incomplete: `App::install_workbook` (`LoadModel`,
`AcceptWizardModel`) cleared `cursors` and not the history, so `<` after a load
walked the **outgoing** document's frames, landing on a view of the same name in the
incoming one that this client had never visited, or on a refused switch that still
left a phantom frame behind. Both halves are fixed, so neither relies on the other:
`install_workbook` clears both stacks beside `cursors`, for the same reason, and the
two steps go through `stepped_to`, which switches, observes whether the document
**accepted** the view (`App::switch_to_view` now returns that), and only then
restores the frame's mode and hands back the frame just left. A frame the document
refuses is consumed and leaves nothing.
The observation is *accepted*, not *moved*, and that is the one place the sibling
shapes differ. `SwitchView` asks "did `active_view` change" because asking for the
view you are on is not a move. A history frame is a **place**, meaning a view and a
mode, so a live frame naming the view you are on still restores its mode. Such
frames exist: `forget_view` collapses `[A, B, A]` to `[A, A]`. The client's
`reduce_view::stepped` also steps onto them unconditionally. A "moved" test
would therefore drop a frame the client keeps, and the host and the client
would disagree. The first version of this fix made exactly that mistake.
`colon_view_back_respects_whether_history_exists` caught it, because its
fixture is such a frame. `reduce_view`'s `stepped` is unchanged, because the
client holds no `Workbook` to observe against. That leaves the host and the
client able to disagree wherever a client's stack holds a frame the document
would refuse. An earlier version of this paragraph said such a frame could not
exist, because `ForgetView` purges deleted views and a load arrives as a fresh
`Snapshot`. **That is false.** `project.rs::fresh_view` starts from
`ViewState::default()` and folds `SwitchView(active)`, so every connected or
rebased client gets a phantom `{Default, Normal}` back-frame, and `Default`
need not exist in the document. That is improvise-lxg0, and until it is fixed
the divergence is live rather than theoretical.
### `ReturnToNormalUnlessMoved` — observation, not prediction (improvise-toc)
context/test-evidence/command.md
+51
View File
@@ -5150,6 +5150,57 @@ the list. Its seed is exact (`"007"`), but `typed()` classifies the committed
text as `Number(7.0)`. Probed here: both tests fail on it in both modes. That
is the classifier, not the seed, and it is improvise-ixxx.
### loading_a_document_forgets_the_outgoing_documents_view_history
improvise-lrrx: `App::install_workbook` cleared `cursors` but not
`view_back_stack`/`view_forward_stack`, so `<` after `LoadModel` walked the
outgoing document's views inside the incoming one. The history is two deep
(`Default`, `X`, then on `Z`), and the incoming document **has** a view named `X`
but opens on `Y`. Without that, a stale frame is a refused switch and looks the
same as "nothing happened" (the ticket's own fixture warning). Against the unfixed
code, `<` landed on `X`, a view this client never visited in the new document.
Rival injected: the two `clear()` lines removed from `install_workbook`, with the
improvise-8nd9 fix still in place. This test and its wizard twin failed at the
`view-back walked the outgoing document's history` assertion.
### accepting_an_import_forgets_the_outgoing_documents_view_history
improvise-lrrx, through the second door into `install_workbook`. A
wizard-built workbook has only `Default`, so the test creates an `X` in it after
accepting. Without that, the stale `X` frame would be refused and could not be
told apart from a cleared history. Same rival and same failure as the load test.
### a_step_back_the_document_refuses_leaves_no_frame_behind
improvise-8nd9: `ViewBack` pushed the current view onto the forward stack before
the guarded `switch_to_view`, so a frame naming a view the document lacks left a
phantom behind, and `,` would then "go forward" to where it already was. The
dead frame is pushed by hand, standing for any path that forgets
`forget_view`, with a live frame under it and the frame's mode set to `Help` so
that a restored mode is observable. Two rivals. First, the original code, which
pushes first and restores the mode unconditionally: it fails at `nothing moved,
so no mode`. Second, a push-first version that restores the mode only on a move,
so the mode assertion cannot hide the stack assertion: it fails at `a refused
step left X on the forward stack`. The last two assertions check that the live
frame beneath still works, so a fix that drops the whole stack cannot pass.
### a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds
improvise-8nd9. This test holds the fix to the client's `reduce_view::stepped`.
A live frame naming the view you are on (a `forget_view` collapse leaves one)
is still a step. It restores its mode and leaves a forward frame, exactly as
the client folds it. Rival injected: `stepped_to` observing whether
`active_view` *changed* rather than whether the document *accepted* the view.
That was the first version of this fix. This test failed at the
mode assertion, as did
`command::cmd::text_buffer::tests::colon_view_back_respects_whether_history_exists`.
That test's fixture is such a frame, and it is what caught the first version.
### a_step_forward_the_document_refuses_leaves_no_frame_behind
The `ViewForward` half of the test above, with the stacks' roles swapped. It fails
against the unfixed code at `nothing moved, so no mode`.
### yy_then_p_in_records_mode_copies_between_real_records
improvise-6p8q. At `gh/main` `9e78051` it failed at the yank: `left: None,
crates/improvise-command/src/ui/app.rs
+9 -4
View File
@@ -444,6 +444,9 @@ impl App {
pub fn install_workbook(&mut self, workbook: Workbook) {
self.view_state.active_view = workbook.active_view.clone();
self.view_state.cursors.clear();
// The history names the outgoing document's views too (improvise-lrrx).
self.view_state.view_back_stack.clear();
self.view_state.view_forward_stack.clear();
self.model_state.workbook = workbook;
// Every client's mirror is scoped to the outgoing document too, and an
// effect cannot reach the session list. The generation is how it hears.
@@ -581,12 +584,14 @@ impl App {
.flat_map(move |r| (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c)))
}
/// Point **this client's** slice at `name`, if the document has such a view.
/// The document's own default stays put (improvise-jytm, gotcha 29).
pub fn switch_to_view(&mut self, name: &str) {
if self.model_state.workbook.views.contains_key(name) {
/// Point **this client's** slice at `name`, if the document has such a view,
/// and say whether it did. The document's default stays put (gotcha 29).
pub fn switch_to_view(&mut self, name: &str) -> bool {
let accepted = self.model_state.workbook.views.contains_key(name);
if accepted {
self.view_state.active_view = name.to_string();
}
accepted
}
/// What `name` names in this document, falling back to the default for a name
crates/improvise-command/src/ui/effect.rs
+187 -16
View File
@@ -388,19 +388,30 @@ impl Effect for SwitchView {
}
}
/// Switch to `frame`'s view and restore its mode, returning the frame just left
/// — or `None`, touching nothing, if the document refused the view.
fn stepped_to(app: &mut App, frame: ViewFrame) -> Option<ViewFrame> {
let left = ViewFrame {
view_name: app.view_state.active_view.clone(),
mode: app.view_state.mode.clone(),
};
app.switch_to_view(&frame.view_name).then(|| {
app.view_state.mode = frame.mode;
left
})
}
/// Go back in view history (pop back stack, push current to forward stack).
#[derive(Debug)]
pub struct ViewBack;
impl Effect for ViewBack {
fn apply(&self, app: &mut App) {
if let Some(frame) = app.view_state.view_back_stack.pop() {
let current = app.view_state.active_view.clone();
app.view_state.view_forward_stack.push(ViewFrame {
view_name: current,
mode: app.view_state.mode.clone(),
});
app.switch_to_view(&frame.view_name);
app.view_state.mode = frame.mode;
// Observed, as `SwitchView` is: a frame the document refuses is dropped
// and leaves nothing behind (improvise-8nd9, `context/code/command.md`).
if let Some(frame) = app.view_state.view_back_stack.pop()
&& let Some(left) = stepped_to(app, frame)
{
app.view_state.view_forward_stack.push(left);
}
}
fn kind(&self) -> EffectKind {
@@ -413,14 +424,11 @@ impl Effect for ViewBack {
pub struct ViewForward;
impl Effect for ViewForward {
fn apply(&self, app: &mut App) {
if let Some(frame) = app.view_state.view_forward_stack.pop() {
let current = app.view_state.active_view.clone();
app.view_state.view_back_stack.push(ViewFrame {
view_name: current,
mode: app.view_state.mode.clone(),
});
app.switch_to_view(&frame.view_name);
app.view_state.mode = frame.mode;
// The mirror of `ViewBack`, observed the same way (improvise-8nd9).
if let Some(frame) = app.view_state.view_forward_stack.pop()
&& let Some(left) = stepped_to(app, frame)
{
app.view_state.view_back_stack.push(left);
}
}
fn kind(&self) -> EffectKind {
@@ -1925,6 +1933,169 @@ mod tests {
assert_eq!(app.view_state.active_view, before);
}
/// A history two deep through `X` and `Z`: back is `[Default, X]`.
fn app_with_history(storage: Box<dyn crate::storage::Storage>) -> App {
let mut wb = Workbook::new("Outgoing");
wb.create_view("X");
wb.create_view("Z");
let mut app = App::with_storage(wb, None, storage);
for name in ["X", "Z"] {
SwitchView(name.to_string()).apply(&mut app);
}
assert_eq!(app.view_state.view_back_stack.len(), 2, "setup: two deep");
app
}
/// Asserts `<` and `,` leave `view` put and the history empty.
fn assert_history_went_with_the_document(app: &mut App, view: &str) {
assert_eq!(app.view_state.active_view, view, "setup: opened on {view}");
for (name, step) in [
("view-back", Box::new(ViewBack) as Box<dyn Effect>),
("view-forward", Box::new(ViewForward)),
] {
step.apply(app);
assert_eq!(
app.view_state.active_view, view,
"{name} walked the outgoing document's history in the incoming one"
);
}
assert!(app.view_state.view_back_stack.is_empty());
assert!(app.view_state.view_forward_stack.is_empty());
}
/// improvise-lrrx. Evidence: `context/test-evidence/command.md`.
#[test]
fn loading_a_document_forgets_the_outgoing_documents_view_history() {
use crate::storage::{MemoryFs, Storage};
let fs = MemoryFs::new();
// The incoming document HAS a view named `X`, and opens somewhere else.
let mut incoming = Workbook::new("Incoming");
incoming.create_view("X");
incoming.create_view("Y");
incoming.active_view = "Y".to_string();
let text = crate::persistence::format_md(&incoming);
fs.write(std::path::Path::new("b.improv"), text.as_bytes())
.unwrap();
let mut app = app_with_history(Box::new(fs));
LoadModel("b.improv".into()).apply(&mut app);
assert_eq!(
app.model_state.workbook.model.name, "Incoming",
"setup: loaded"
);
assert_history_went_with_the_document(&mut app, "Y");
}
/// improvise-lrrx: the wizard is the other door into `install_workbook`.
#[test]
fn accepting_an_import_forgets_the_outgoing_documents_view_history() {
let mut app = app_with_history(Box::new(crate::storage::MemoryFs::new()));
let mut wizard = crate::import::wizard::ImportWizard::new(serde_json::json!([
{"region": "East", "revenue": 100.0},
{"region": "West", "revenue": 200.0},
]));
wizard.advance();
wizard.advance();
wizard.build();
assert!(wizard.built_summary().is_some(), "setup: a built workbook");
app.wizard = Some(wizard);
AcceptWizardModel.apply(&mut app);
// The new document gains an `X` of its own, so a stale frame could land.
app.model_state.workbook.create_view("X");
let opened = app.view_state.active_view.clone();
assert_ne!(opened, "X", "setup: the import must not open on X");
assert_history_went_with_the_document(&mut app, &opened);
}
/// improvise-8nd9: a frame naming a view the document lacks is refused, and
/// a refused step leaves no frame behind. See `context/test-evidence/command.md`.
#[test]
fn a_step_back_the_document_refuses_leaves_no_frame_behind() {
let mut app = test_app();
app.model_state.workbook.create_view("X");
SwitchView("X".to_string()).apply(&mut app);
// A path that forgot `forget_view`: the history names a dead view.
app.view_state.view_back_stack.push(ViewFrame {
view_name: "Gone".to_string(),
mode: AppMode::Help,
});
ViewBack.apply(&mut app);
assert_eq!(
app.view_state.active_view, "X",
"setup: the step is refused"
);
assert_eq!(
app.view_state.mode,
AppMode::Normal,
"nothing moved, so no mode"
);
assert!(
app.view_state.view_forward_stack.is_empty(),
"a refused step left X on the forward stack: `,` would go nowhere"
);
ViewBack.apply(&mut app);
assert_eq!(
app.view_state.active_view, "Default",
"the live frame survives"
);
assert_eq!(app.view_state.view_forward_stack.len(), 1);
}
/// improvise-8nd9: a live frame naming the view you are on is still a step, as
/// it is in the client's `reduce_view`. See `context/test-evidence/command.md`.
#[test]
fn a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds() {
let mut app = test_app();
app.model_state.workbook.create_view("X");
SwitchView("X".to_string()).apply(&mut app);
app.view_state.view_back_stack.push(ViewFrame {
view_name: "X".to_string(),
mode: AppMode::Help,
});
let mut mirror = app.view_state.clone();
ViewBack.apply(&mut app);
improvise_protocol::reduce_view(&mut mirror, &improvise_protocol::Command::ViewBack);
assert_eq!(
app.view_state.mode,
AppMode::Help,
"the frame's mode is restored"
);
assert_eq!(app.view_state.view_back_stack, mirror.view_back_stack);
assert_eq!(app.view_state.view_forward_stack, mirror.view_forward_stack);
assert_eq!(app.view_state.mode, mirror.mode);
}
/// improvise-8nd9: the forward half of the step above.
#[test]
fn a_step_forward_the_document_refuses_leaves_no_frame_behind() {
let mut app = test_app();
app.model_state.workbook.create_view("X");
SwitchView("X".to_string()).apply(&mut app);
ViewBack.apply(&mut app);
app.view_state.view_forward_stack.push(ViewFrame {
view_name: "Gone".to_string(),
mode: AppMode::Help,
});
ViewForward.apply(&mut app);
assert_eq!(app.view_state.active_view, "Default", "setup: refused");
assert_eq!(
app.view_state.mode,
AppMode::Normal,
"nothing moved, so no mode"
);
assert!(
app.view_state.view_back_stack.is_empty(),
"a refused step left Default on the back stack: `<` would go nowhere"
);
ViewForward.apply(&mut app);
assert_eq!(app.view_state.active_view, "X", "the live frame survives");
assert_eq!(app.view_state.view_back_stack.len(), 1);
}
#[test]
fn create_and_delete_view() {
let mut app = test_app();