diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index f235177c..2d97e428 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -5,7 +5,7 @@ use crate::ui::effect::{self, Effect}; use crate::view::RecordCol; use improvise_protocol::{OpenedEditor, untouched}; -use super::core::{Cmd, CmdContext, read_buffer}; +use super::core::{Cmd, CmdContext, read_buffer, write_buffer}; use super::grid::AddRecordRow; use super::navigation::{CursorState, EnterAdvance, viewport_effects}; @@ -805,6 +805,72 @@ mod tests { "the reopened editor did not capture the cell it landed on" ); } + + /// Every cell of the 2×2 fixture, by its key. + fn every_cell(app: &crate::ui::app::App) -> Vec> { + use crate::model::cell::CellKey; + let mut cells = Vec::new(); + for region in ["North", "West"] { + for measure in ["Cost", "Revenue"] { + let key = CellKey::new(vec![ + ("Region".into(), region.into()), + ("_Measure".into(), measure.into()), + ]); + cells.push(app.model_state.workbook.model.get_cell(&key).cloned()); + } + } + cells + } + + /// BUG (improvise-fy9o) — see `context/test-evidence/command.md`. + #[test] + fn enter_enter_leaves_every_cell_unchanged() { + use improvise_protocol::key::KeyCode; + for (records, start) in [(false, (1, 0)), (true, (1, 2))] { + let mut app = grid_holding(CellValue::Number(4.0), records); + if records { + app.view_state.mode = AppMode::RecordsNormal; + } + app.cursor_mut().selected = start; + let before = every_cell(&app); + press(&mut app, KeyCode::Char('i')); + press(&mut app, KeyCode::Enter); + let reopened = app.view_state.buffers.get("edit").cloned(); + press(&mut app, KeyCode::Enter); + assert_eq!( + every_cell(&app), + before, + "records={records}: i, Enter, Enter changed a cell (the reopened editor held {reopened:?})" + ); + } + } + + /// The reopened editor shows the cell it landed on, so typing extends it. + /// BUG (improvise-fy9o) — see `context/test-evidence/command.md`. + #[test] + fn typing_after_the_reopen_still_writes() { + use crate::model::cell::CellKey; + use improvise_protocol::key::KeyCode; + let mut app = grid_holding(CellValue::Number(4.0), false); + app.cursor_mut().selected = (1, 0); + press(&mut app, KeyCode::Char('i')); + press(&mut app, KeyCode::Enter); + assert_eq!( + app.view_state.buffers.get("edit").map(String::as_str), + Some("2"), + "the editor reopened on (North, Revenue) is not seeded with its value" + ); + press(&mut app, KeyCode::Char('5')); + press(&mut app, KeyCode::Enter); + let landed = CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "Revenue".into()), + ]); + assert_eq!( + app.model_state.workbook.model.get_cell(&landed), + Some(&CellValue::Number(25.0)) + ); + } } // ── Commit commands (mode-specific buffer consumers) ──────────────────────── @@ -951,6 +1017,11 @@ impl Cmd for CommitAndAdvance { // Drop to normal **before** the commit, which can abort: tidy-up goes where // an abort cannot reach it (improvise-2zq, gotcha 21). effects.push(effect::change_mode(exit_mode_for(&self.edit_mode))); + // This command owns its buffer's clear, at the head, so the re-entry + // below re-seeds after it rather than being blanked (improvise-fy9o). + if let Some(mb) = self.edit_mode.minibuffer() { + effects.push(write_buffer(&mb.buffer_key, String::new())); + } // An untouched editor writes nothing; everything around the write still runs. let writes = !untouched(self.opened.as_ref(), &self.value); if writes { diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index d98b693c..300e4ef8 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -690,6 +690,22 @@ mod tests { } /// The buffer `append-char` is bound to in `mode`'s keymap. + /// The effects `name` builds in `mode` with `buffer` holding text, as Debug. + fn clears_itself(mode: &AppMode, name: &str, args: &[String], buffer: &str) -> String { + use crate::command::cmd::test_helpers::*; + let wb = two_cat_model(); + let layout = make_layout(&wb); + let reg = make_registry(); + let bufs: HashMap = [(buffer.to_string(), "typed".to_string())].into(); + let mut ctx = make_ctx(&wb, &layout, ®); + ctx.mode = mode; + ctx.buffers = &bufs; + let cmd = reg + .interactive(name, args, &ctx) + .unwrap_or_else(|e| panic!("{name} {args:?} does not build: {e}")); + effects_debug(&cmd.execute(&ctx)) + } + fn append_target(ks: &KeymapSet, mode: &AppMode) -> String { let key = ModeKey::from_app_mode(mode, false).expect("a text-entry mode key"); let km = ks.maps().get(&key).expect("every mode has a keymap"); @@ -764,15 +780,22 @@ mod tests { let km = ks.maps().get(&mode).unwrap(); for key in exits { - let Some(Binding::Sequence(steps)) = km.lookup(*key, none) else { - panic!("{mode:?}: {key:?} should be a commit/exit sequence"); - }; - assert!( - steps - .iter() - .any(|(n, a)| *n == "clear-buffer" && a.as_slice() == [buffer.to_string()]), - "{mode:?}: {key:?} must clear \"{buffer}\", got {steps:?}" - ); + match km.lookup(*key, none) { + Some(Binding::Sequence(steps)) => assert!( + steps.iter().any(|(n, a)| *n == "clear-buffer" + && a.as_slice() == [buffer.to_string()]), + "{mode:?}: {key:?} must clear \"{buffer}\", got {steps:?}" + ), + // A reopening commit owns its clear (improvise-fy9o): ask the command. + Some(Binding::Cmd { name, args }) => { + let dbg = clears_itself(&app_mode, name, args, &buffer); + assert!( + dbg.contains(&format!("SetBuffer {{ name: \"{buffer}\", value: \"\" }}")), + "{mode:?}: {key:?} runs {name} alone, which must clear \"{buffer}\": {dbg}" + ); + } + other => panic!("{mode:?}: {key:?} should commit or exit, got {other:?}"), + } } } } diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 6b7aef43..aefd9027 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -347,6 +347,13 @@ impl TextEntry { self } + /// Bind `key` to a commit that clears the buffer **itself**, first, because + /// it reopens an editor a trailing clear would blank (improvise-fy9o). + pub fn commit_reopening(mut self, key: KeyCode, cmd: &'static str, args: Vec) -> Self { + self.km.bind_args(key, KeyModifiers::NONE, cmd, args); + self + } + /// Replace a spine binding — used only by CommandMode, whose Backspace /// exits the mode on an empty buffer instead of popping a character. pub fn rebind(mut self, key: KeyCode, cmd: &'static str) -> Self { @@ -851,8 +858,8 @@ impl Keymaps { // edit-mode as an argument, so the command stays mode-agnostic. ── let ed = Arc::new( TextEntry::new(&AppMode::editing(), "normal", global.clone()) - .commit(KeyCode::Enter, "commit-cell-edit", vec!["editing".into()]) - .commit( + .commit_reopening(KeyCode::Enter, "commit-cell-edit", vec!["editing".into()]) + .commit_reopening( KeyCode::Tab, "commit-and-advance-right", vec!["editing".into()], @@ -867,12 +874,12 @@ impl Keymaps { ModeKey::RecordsEditing, Arc::new( TextEntry::new(&AppMode::records_editing(), "records-normal", ed) - .commit( + .commit_reopening( KeyCode::Enter, "commit-cell-edit", vec!["records-editing".into()], ) - .commit( + .commit_reopening( KeyCode::Tab, "commit-and-advance-right", vec!["records-editing".into()], diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 0d964ec7..fb6d4bff 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -896,10 +896,19 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option Option { + edit_mode + .minibuffer() + .map(|mb| write_buffer(&mb.buffer_key, String::new())) +} + /// The write a commit makes: none when the buffer is the seed the editor /// captured as it opened, and only the write is skipped (improvise-ixxx). fn written(state: &ClientState, id: &CellId, text: &str) -> Option> { @@ -924,6 +933,7 @@ fn create_column( // `exit_mode_for`: the tidy-up goes FIRST, where an aborting // `AddCategory` cannot reach it (gotcha 21). let mut commands = vec![Command::ChangeMode(AppMode::Normal)]; + commands.extend(cleared(&edit_mode)); let name = text.trim(); if !name.is_empty() { commands.push(model( @@ -3355,18 +3365,28 @@ mod tests { } ); assert_eq!(commands[1], Command::ChangeMode(AppMode::Normal)); - assert_eq!(commands[2], Command::SetSelected { row: 1, col: 0 }); - assert!( - commands.contains(&Command::ChangeMode(AppMode::editing())), - "the advance re-enters editing on the cell it landed on: {commands:?}" - ); + // The commit owns its clear, at the head (improvise-fy9o). assert_eq!( - commands.last(), - Some(&Command::SetBuffer { + commands[2], + Command::SetBuffer { name: "edit".into(), value: String::new(), - }), - "the keymap welds `clear-buffer` to every commit: {commands:?}" + } + ); + assert_eq!(commands[3], Command::SetSelected { row: 1, col: 0 }); + assert_eq!( + commands.last(), + Some(&Command::ChangeMode(AppMode::editing())), + "the advance re-enters editing on the cell it landed on: {commands:?}" + ); + let last_write = commands.iter().rev().find_map(|c| match c { + Command::SetBuffer { name, value } if name == "edit" => Some(value.as_str()), + _ => None, + }); + assert_eq!( + last_write, + Some("900"), + "the reopened editor must show the cell it landed on, not blank: {commands:?}" ); } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 3dad0c9a..cb1b9a34 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1893,6 +1893,71 @@ fn an_untouched_browser_edit_of_a_misread_value_writes_nothing() { } } +/// BUG (improvise-fy9o) — see `context/test-evidence/clients.md`. +#[test] +fn enter_enter_leaves_every_cell_unchanged_on_both_ends() { + let every_cell = |app: &App| -> Vec> { + [("North", "Cost"), ("North", "Revenue"), ("South", "Cost"), ("South", "Revenue")] + .iter() + .map(|(r, m)| app.model_state.workbook.model.get_cell(®ion_measure(r, m)).cloned()) + .collect() + }; + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (1, 0); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (1, 0); + let before = every_cell(&client_host); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "Enter", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + assert_eq!( + client.view.buffers, key_host.view_state.buffers, + "{key}: client and host disagree about the buffers" + ); + } + assert_eq!(every_cell(&client_host), before, "the browser's Enter, Enter wrote"); + assert_eq!(every_cell(&key_host), before, "the host's Enter, Enter wrote"); +} + +/// BUG (improvise-fy9o) — see `context/test-evidence/clients.md`. +#[test] +fn typing_into_the_reopened_editor_extends_the_cell_on_both_ends() { + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (1, 0); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (1, 0); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "Enter", "5", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + } + for (end, app) in [("browser", &client_host), ("host", &key_host)] { + assert_eq!( + app.model_state + .workbook + .model + .get_cell(®ion_measure("North", "Revenue")), + Some(&improvise_protocol::CellValue::Number(25.0)), + "{end}: the digit typed into the reopened editor did not extend its seed" + ); + } +} + /// `i` then `Enter` on (South, Revenue) holding `value`, through the resolver /// and a real host: the seed is the stored value's, and the cell is unchanged. fn untouched_browser_edit(value: improvise_protocol::CellValue) {