diff --git a/context/code/tui.md b/context/code/tui.md index e595cdc3..a3589e3e 100644 --- a/context/code/tui.md +++ b/context/code/tui.md @@ -434,12 +434,12 @@ first correcting key: one Backspace and the user was fixing text they could no longer see the complaint about. The same happened to any refusal that leaves the user in a minibuffer. -**The rule: a key that leaves the user in the same text-entry mode, and says -nothing of its own, puts the stale message back.** `press` remembers the mode -and the message, clears the message as before, dispatches the key, and then -restores the message only if all three of these hold: -- the mode before the key had a minibuffer; -- the mode after the key is the same mode; +**The rule: a key that leaves the user in the same editor, and says nothing of +its own, puts the stale message back.** `press` remembers the editor session and +the message, clears the message as before, dispatches the key, and then restores +the message only if all three of these hold: +- a text-entry mode was open before the key; +- the same editor session is open after it (below); - the key wrote no message of its own. So: @@ -462,10 +462,48 @@ So: same text would be indistinguishable from a key that wrote none. Clearing first and restoring only an empty slot has no such ambiguity. -The mode test is equality of the whole `AppMode`. An edit that stayed in "a" -minibuffer but a different one does not count as staying. No shipped key does -that, but it would be leaving one prompt for another, and the message belonged -to the first. +### What "the same editor" means (improvise-sx7h) + +The first version of this rule compared modes: an equal `AppMode` before and +after the key meant the same editor. That is false for the two cell editors. +`Editing` and `RecordsEditing` commit with `CommitAndAdvance`, which drops to +Normal, writes the cell, moves the cursor and re-opens the editor through +`EnterEditAtCursor`, which sets an **equal** `AppMode`. So after an `Enter` or +`Tab` that committed and moved on, the mode looked unchanged, and the stale +message was restored beside a different cell's editor. It came back again on +every later commit until `Esc`. The reviewer's repro was `R`, `o`, `5`, `Tab`, +`7`: "Added new record row" survived the `Tab`, and so did `o`'s refusal +"That record row is already here…" after the `Tab` that filled the row in. +`FormulaEdit` could not show it, because its successful commit leaves the mode +and writes a message of its own. + +An editor session is one *opening* of a minibuffer. The TUI loop cannot see +openings, so `EditorSession` compares what an opening sets: +- **the whole `AppMode`**, not only its variant: an `ItemAdd` for `Month` is + not the `ItemAdd` for `Region` it replaced; +- **the cursor cell**, which is a cell editor's target, so a commit that + advances is a new editor. The prompt modes do not move the cursor while the + user types, so for them this is inert; +- **the editor's seed** (`opened_editor.seed`). `EnterEditAtCursor` re-seeds + from the cell. So a commit that re-opens on the **same** cell, such as `Tab` + in the last column, is a new editor too, because the seed is now the + committed text. + +What it cannot see: a commit that re-opens on the same cell with the same seed. +That means the user committed the text the cell already held, which writes +nothing. Restoring a message across a commit that changed nothing is harmless. + +**Rejected: an editor generation counter.** It would be exact, but an editor is +opened by more than one effect (`EnterEditAtCursor`, and `ChangeMode` from +`enter-mode`). A counter bumped in each is a hand-kept list of the places a mode +is written, which is the shape gotcha 24 retired. The three fields are set by +the opening itself, so they need no list. + +**Not included: the active view.** The cursor is per view, so in principle the +view belongs with the cell. But no key a minibuffer binds can switch the view +and stay in the minibuffer: a `:` line that switches view settles back to +Normal. So nothing could test the field, and an untestable field is a claim no +test can check. **The browser is not covered by this.** There, `status_msg` is a toast that hides after `TOAST_MS` (six seconds), and the minibuffer element draws only the diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index b8b5b422..016a651c 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -969,3 +969,37 @@ Rivals, each injected into `press` and restored: - **Restore whatever the mode after the key** fails step 4, because the error outlives `Esc`. It does not fail step 3, because a successful commit writes its own message. That is why step 4 exists. + +### a_message_does_not_follow_the_editor_onto_the_next_cell + +improvise-sx7h, a regression #257 introduced against #233. Equal modes stood for +"the same editor", but `Editing`/`RecordsEditing` commit by re-opening an equal +mode on the next cell. The test drives `press` through four scenarios: +1. **Records.** `R`, then `o` ("Added new record row", in `RecordsEditing`), + then `5`. The message stands while typing in the same editor. Then `Tab`: the + editor re-opens on the next cell, and the message must be gone. +2. **Records.** `R`, `o`, `Esc`, then `o` again, which is refused with "That + record row is already here…". Then `5`, with the refusal still drawn. Then + `Tab`, which fills the row in, and the refusal must be gone. +3. **A plain grid.** `i` on `(0, 0)`, with a note set while editing. Then `7`, + with the note kept. Then `Enter`: the editor re-opens a row down, and the + note must be gone. +4. **The last column.** `i`, a note, `7`, `Tab`. The cursor cannot advance, so + the editor re-opens on the **same** cell, and the note must be gone. + +Scenarios 1 and 2 are the reviewer's repro. + +Rivals, each injected into `EditorSession::of` and restored: +- **mode only**, which is #257's rule as pushed at 2b1c5bf7, fails at + scenario 1; +- **no cursor cell** fails at scenario 1, because the new record's cells both + seed `""`, so the seed alone cannot tell them apart; +- **no seed** fails at scenario 4, the only place the cell does not change. + +### an_editor_on_another_category_is_another_editor + +improvise-sx7h's pinning test, asked for by the review. The mode is compared +whole, not by variant. Two `ItemAdd` sessions for different categories must be +different editors, and the same category must be the same. No key reaches that +transition, so it is asserted on `EditorSession::of` directly. The rival that +compares `std::mem::discriminant` fails it, and passes every other test. diff --git a/src/draw.rs b/src/draw.rs index 05409e93..05708b4a 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -100,19 +100,43 @@ pub fn run_tui( } /// One keypress in the TUI. A message lasts until the next key (improvise-cq3d), -/// except while a minibuffer stays open under it (su4u). `context/code/tui.md`. +/// except while one editor stays open under it (su4u, sx7h). `context/code/tui.md`. fn press(app: &mut App, key: key::KeyEvent) -> Result<()> { - let mode = app.view_state.mode.clone(); + let before = EditorSession::of(app); let stale = app.view_state.status_msg.clone(); app.apply_effects(vec![Box::new(crate::ui::effect::SetStatus(String::new()))]); app.handle_key(key)?; - let still_editing = mode.minibuffer().is_some() && app.view_state.mode == mode; + let still_editing = before.is_some() && before == EditorSession::of(app); if still_editing && app.view_state.status_msg.is_empty() && !stale.is_empty() { app.apply_effects(vec![Box::new(crate::ui::effect::SetStatus(stale))]); } Ok(()) } +/// What one opening of a minibuffer sets, which is all the TUI loop can see of +/// it: equal before and after a key means the same editor (improvise-sx7h). +#[derive(Debug, PartialEq)] +struct EditorSession { + mode: AppMode, + cell: (usize, usize), + seed: Option, +} + +impl EditorSession { + fn of(app: &App) -> Option { + app.view_state.mode.minibuffer()?; + Some(Self { + mode: app.view_state.mode.clone(), + cell: app.cursor().selected, + seed: app + .view_state + .opened_editor + .as_ref() + .map(|o| o.seed.clone()), + }) + } +} + // ── Key event conversion (TUI boundary) ────────────────────────────────────── // The only place crossterm's key types and `command::key` may both appear. @@ -580,7 +604,7 @@ fn draw_welcome(f: &mut Frame, area: Rect) { #[cfg(test)] mod bottom_bar_tests { - use super::{draw_bottom_bar, minibuffer_line, press}; + use super::{EditorSession, draw_bottom_bar, minibuffer_line, press}; use crate::command::key::{KeyCode, KeyEvent, KeyModifiers}; use crate::ui::app::{App, AppMode}; use crate::workbook::Workbook; @@ -698,6 +722,133 @@ mod bottom_bar_tests { ); } + /// Region{North, South}, two measures, one cell: the reviewer's records fixture + /// for improvise-sx7h, plus a second row for the plain grid's Enter. + fn one_record() -> App { + use crate::model::cell::{CellKey, CellValue}; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Region", "South").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(1.0), + ); + App::new(wb, None) + } + + /// A commit that re-opens the editor on another cell starts another editor, so + /// a message from the last one must not follow it (improvise-sx7h). Evidence file. + #[test] + fn a_message_does_not_follow_the_editor_onto_the_next_cell() { + let key = |code| KeyEvent::new(code, KeyModifiers::NONE); + let ch = |app: &mut App, c: char| press(app, key(KeyCode::Char(c))).unwrap(); + + // Records: `o` says it added a row and opens an editor on it. + let mut app = one_record(); + ch(&mut app, 'R'); + ch(&mut app, 'o'); + let added = app.view_state.status_msg.clone(); + assert!(!added.is_empty(), "o must say what it did"); + assert!(matches!( + app.view_state.mode, + AppMode::RecordsEditing { .. } + )); + ch(&mut app, '5'); + assert!( + bottom_row(&app, 240).contains(&added), + "typing in the same editor keeps the message" + ); + let cell = app.cursor().selected; + press(&mut app, key(KeyCode::Tab)).unwrap(); + assert!(matches!( + app.view_state.mode, + AppMode::RecordsEditing { .. } + )); + assert_ne!(app.cursor().selected, cell, "Tab moved the editor"); + let row = bottom_row(&app, 240); + assert!( + !row.contains(&added), + "the message followed the editor to the next cell: {row:?}" + ); + + // Records: a second `o` on the blank row is refused, and filling it in ends it. + let mut app = one_record(); + ch(&mut app, 'R'); + ch(&mut app, 'o'); + press(&mut app, key(KeyCode::Esc)).unwrap(); + ch(&mut app, 'o'); + let refusal = app.view_state.status_msg.clone(); + assert!(refusal.contains("already here"), "{refusal:?}"); + ch(&mut app, '5'); + assert!(bottom_row(&app, 240).contains("already here")); + press(&mut app, key(KeyCode::Tab)).unwrap(); + let row = bottom_row(&app, 240); + assert!( + !row.contains("already here"), + "the refusal outlived the Tab that answered it: {row:?}" + ); + + // A plain grid: Enter commits and re-opens the editor one row down. + let mut app = one_record(); + app.cursor_mut().selected = (0, 0); + ch(&mut app, 'i'); + assert!(matches!(app.view_state.mode, AppMode::Editing { .. })); + app.view_state.status_msg = "a note about this cell".into(); + ch(&mut app, '7'); + assert!(bottom_row(&app, 240).contains("a note about this cell")); + let cell = app.cursor().selected; + press(&mut app, key(KeyCode::Enter)).unwrap(); + assert!(matches!(app.view_state.mode, AppMode::Editing { .. })); + assert_ne!(app.cursor().selected, cell, "Enter moved the editor"); + let row = bottom_row(&app, 240); + assert!( + !row.contains("a note about this cell"), + "the note followed the editor down a row: {row:?}" + ); + + // The last column: Tab commits and re-opens the editor on the SAME cell, so + // only the new seed says it is another editor. + let mut app = one_record(); + let last = app.layout.col_count() - 1; + app.cursor_mut().selected = (0, last); + ch(&mut app, 'i'); + app.view_state.status_msg = "a note about this cell".into(); + ch(&mut app, '7'); + press(&mut app, key(KeyCode::Tab)).unwrap(); + assert!(matches!(app.view_state.mode, AppMode::Editing { .. })); + assert_eq!(app.cursor().selected, (0, last), "Tab could not advance"); + let row = bottom_row(&app, 240); + assert!( + !row.contains("a note about this cell"), + "the note outlived the commit on the same cell: {row:?}" + ); + } + + /// Equal modes are the test, not equal variants: an editor adding to another + /// category is another editor (improvise-sx7h). Evidence file. + #[test] + fn an_editor_on_another_category_is_another_editor() { + let mut app = App::new(Workbook::new("T"), None); + let item_add = |category: &str| AppMode::item_add(category.into()); + app.view_state.mode = item_add("Region"); + let region = EditorSession::of(&app); + assert!(region.is_some(), "ItemAdd is a text-entry mode"); + app.view_state.mode = item_add("Month"); + assert_ne!(region, EditorSession::of(&app)); + app.view_state.mode = item_add("Region"); + assert_eq!( + region, + EditorSession::of(&app), + "and the same one is the same" + ); + } + /// The wizard overlay draws correct per-step hints; the status bar stacked a /// second static table under it (improvise-rr8n) — `context/test-evidence/tui.md`. #[test]