diff --git a/context/code/command.md b/context/code/command.md index 3dd16fd3..b6cefb51 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -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) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index d92223f6..1d9ad37e 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -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, diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 565280a6..4969f2cb 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -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 diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 9bc78b34..18528e69 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -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 { + 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) -> 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), + ("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();