The same mode is not the same editor: a message does not follow a commit (improvise-sx7h)
#257 restored a stale message whenever the mode after a key equalled the mode before it. Editing and RecordsEditing commit with CommitAndAdvance, which re-opens an equal AppMode on the next cell. So the message followed the editor from cell to cell until Esc. The reviewer's repro was R, o, 5, Tab: "Added new record row", or o's "That record row is already here" refusal, survived the Tab. press now compares an EditorSession, made of the things an opening sets: - the whole AppMode, not its variant; - the cursor cell, which is a cell editor's target; - the editor's seed, which EnterEditAtCursor re-seeds from the cell, so a commit that re-opens on the same cell (Tab in the last column) is new. A generation counter was rejected: editors are opened by more than one effect, and a counter in each is a hand-kept list. The view was left out: no minibuffer key can switch it and stay. Tests: - four press scenarios: the two records repros, Enter in a plain grid, and Tab in the last column; - a direct check that ItemAdd on another category is another editor. Rivals: mode-only (#257 as pushed), no cell, no seed and discriminant each fail a distinct step. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
+48
-10
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
+155
-4
@@ -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<String>,
|
||||
}
|
||||
|
||||
impl EditorSession {
|
||||
fn of(app: &App) -> Option<Self> {
|
||||
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]
|
||||
|
||||
Reference in New Issue
Block a user