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

fix(tui): one key table on screen, and it is the derived one (improvise-rr8n)

Browse Source 
Two hand-written key tables were drawn BESIDE derived ones, in a different
notation, and one of them was false.

The tile bar's non-TileSelect hint read " Ctrl+^v<-> to move tiles" while
Ctrl-Left/Right/Up/Down are bound in Normal to enter-tile-select -- pressing one
ENTERS tile mode and moves nothing. It was the only persistent on-screen
documentation of those keys, and it spelt Ctrl+ where key_label, and therefore
every derived hint and help row, spells Ctrl-.

The fix DELETES both of the tile bar's hints rather than rewriting one, which
departs from the ticket's acceptance criteria and does so on the ticket's own
evidence. rr8n proposed rewriting the string and suppressing the STATUS BAR's
hint in TileSelect. But the derived status line is a strict superset of the tile
bar's literal -- it carries Space, h/l and Tab, which the tile bar's omits, as
the ticket's own finding 2 records -- and it cannot go stale, being read off the
keymap. Suppressing it would have deleted information and kept the table that had
just been shown to rot. In Normal the derived line already says T:tiles, so only
the Ctrl-arrow spelling is lost, and it was never documented correctly.

The wizard goes the other way and there the ticket is right. Its overlay draws
correct per-step hints; the status bar drew one static line under it, wrong on
four of eight steps, including telling the user Esc cancels on Done where Esc
goes BACK. That line is derived too, but from the wrong input -- the wizard's
real keys are resolved per step inside HandleWizardKey. So draw_status suppresses
its hint fallback for that one mode.

The rule, stated in context/code/tui.md: keep whichever table is derived from the
thing it describes.

Five tests. Four faults injected, each caught by a distinct test; the sharpest is
suppressing the hint fallback unconditionally, which passes both tests aimed at
the suppression and is caught only by the counterweight asserting TileSelect
still advertises its keys. Two things that let the false hint survive are
recorded: every_hint_key_resolves_in_its_mode cannot see a literal in a widget,
and the widget's own characterization test renders at 80 columns, where the hint
does not fit and is never drawn.

Evidence: context/test-evidence/tui.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-22 18:44:09 -07:00
parent 6bd6cb5771
commit 915d0fefeb
4 changed files with 244 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/tui.md
+37
View File
@@ -183,6 +183,43 @@ decoration", and which is which is local. Truncation goes through
`ui::text::truncate` (improvise-8l0c), so an overflow is a `…` rather than a
mid-token clip of `?:`.
### One key table on screen, and it is the one derived from the keymap
The TUI stacks the tile bar (row 3) directly above the status bar (row 4), and
the import wizard's overlay sits above the status bar too, so a mode can put
**two** key tables on screen at once. When it does, they disagree — different
notation, different content, and in one case a flat contradiction.
The rule is not "the overlay wins" or "the status bar wins". It is **keep
whichever table is derived from the thing it describes** (improvise-rr8n):
- **Tile select: the status bar wins.** `hint_line(&mode)` is read off the
keymap, so it cannot go stale, and it is a strict superset of what the tile
bar's literal carried — `Space`, `h`/`l` and `Tab` were only ever on the
derived line. The tile bar's own hints are therefore **deleted**, both of
them, and `TileBar::render` now advertises no keys at all. The one it drew in
Normal was false: it named `Ctrl+↑↓←→` as "to move tiles" while those four
keys are bound to `enter-tile-select`, which moves nothing.
- **The import wizard: the overlay wins.** `hint_line(&AppMode::ImportWizard)`
is derived too, but from the *wrong input* — the wizard's real keys are
resolved per step inside `HandleWizardKey`, and one static line off the mode's
keymap is wrong on four of its eight steps, including telling the user `Esc`
cancels on `Done` where `Esc` goes back. So `draw_status` suppresses its hint
fallback for that one mode and lets the overlay's per-step hints stand alone.
Two things worth keeping from how this survived. `every_hint_key_resolves_in_its_mode`
checks `AppMode::hint_text()` against the keymaps and **cannot see a string
literal in a widget**, which is the same blind spot improvise-wk18 names for
`help.rs`. And the widget's own characterization test renders at 80 columns,
where the hint does not fit and is never drawn — so the test that pins this
widget's output was structurally incapable of showing either hint. A fixture
width is part of what a test can see.
The longer-term shape the ticket names is still open: the wizard's per-step keys
belong in `advertised()` keyed by step, which would make them derived like
everything else and let the status bar carry them correctly instead of being
switched off.
### The caches are built here, per frame
`project::formulas_of`, `category_tree_of`, `view_list_of`, `tile_bar_of`,
context/test-evidence/tui.md
+106
View File
@@ -730,3 +730,109 @@ prevent.
Fault injected: the `Keeps::Left` arm rewritten to the body of the `Keeps::Right`
arm. This test fails; the other four pass.
---
## `src/ui/tile_bar.rs` + `src/draw.rs` — one hint table on screen, and it is the derived one
improvise-rr8n. Two hand-written key tables were drawn **beside** derived ones,
in a different notation, and one of them was outright false.
**The tile bar's non-TileSelect hint named keys that do something else.** It read
`" Ctrl+↑↓←→ to move tiles"`, while `Ctrl-Left/Right/Up/Down` are bound in Normal
to `enter-tile-select` — pressing one *enters* tile mode and moves nothing. It
was also the only persistent on-screen documentation of those keys, and it spelt
`Ctrl+` where `keymap::key_label`, and therefore every derived hint and help row,
spells `Ctrl-`.
**Nothing could have caught it.** `every_hint_key_resolves_in_its_mode` checks
`AppMode::hint_text()` against the keymaps; a string literal in a widget is
invisible to it. And the shipped fixture `renders_each_tile_with_its_axis_label`
renders at **80 columns**, where the hint does not fit (`x + hint.width() <=
area.x + area.width` is false) and is never drawn — so the one test that pins
this widget's output could not see either hint at all. That is why the new tests
render at 140.
**The fix deletes rather than rewrites, and that departs from the ticket's AC.**
improvise-rr8n proposed rewriting the string to `" Ctrl-↑↓←→ or T to pick a
tile"` and suppressing the *status bar's* hint in TileSelect. Measured against
the ticket's own second finding, that is backwards:
```
tile bar (hand-written): [Enter] cycle axis [r/c/p] set axis [←→] select [Esc] cancel
status bar (derived): hl←→:select Enter/Space:cycle r:row c:col p:page Esc/Tab:back
```
The derived line is a strict **superset** — it carries `Space`, `h`/`l` and
`Tab`, which the tile bar's omits — and it cannot go stale, being read off the
keymap. Suppressing it would have deleted information and kept the table that
had just been shown to rot. So the tile bar now advertises nothing, and in
Normal the derived line already says `T:tiles`; only the `Ctrl-`arrow spelling
is lost, and it was never documented correctly.
**The wizard goes the other way, and there the ticket is right.** Its overlay
draws correct per-step hints, and the status bar drew one static
`Space:toggle c:cycle Enter:next Esc:cancel` under it — wrong on four of eight
steps, including an outright contradiction on `Done`, where the bar says `Esc`
cancels and `Esc` goes *back*. It is static because it is derived from the
`ImportWizard` keymap, while the wizard's real keys are resolved per step inside
`HandleWizardKey`; so the derivation is sound and its *input* is the wrong thing.
`draw_status` therefore suppresses the hint fallback for that one mode.
The asymmetry is the point: **keep whichever table is derived from the thing it
describes.** For tile select that is the status line; for the wizard it is the
overlay.
### the_tile_bar_advertises_no_keys_of_its_own
Renders the bar in Normal and TileSelect at 140 columns and requires that neither
row names `Ctrl`, `Enter`, `Esc`, `←→` or `move tiles`. An absence assertion, so
it is paired with the two below that require the information to still exist
elsewhere.
Two faults injected, both caught here and by nothing else:
- The original `else` branch restored — fails in Normal on `"Ctrl"`, quoting the
whole false line.
- Only the `else` branch deleted, keeping the TileSelect literal (the narrow
reading of finding 1) — fails in TileSelect on `"Enter"`. This is the rival
that matters, because it is the fix a reader of finding 1 alone would write,
and it leaves two disagreeing dialects stacked on adjacent rows.
Note it would also fail against the ticket's own proposed replacement string,
which still contains `Ctrl`. That is deliberate rather than accidental: the
property asserted is "this widget names no keys", not "this widget's keys are
currently right".
### the_tile_bar_still_draws_its_tiles_at_every_width_it_used_to
The counterweight. Deleting a `set_string` at the end of a render function is
the kind of edit that takes a neighbour with it, so this pins the tiles and the
`Tiles:` prefix at 80 and 140. It also covers the dead `x += 1` the deletion
orphaned — clippy caught that one, but the test is what says the drawing still
lands.
### the_wizard_overlay_is_the_only_key_hint_on_screen
The wizard half. Requires the bottom row in `ImportWizard` to name none of
`Space:toggle`, `c:cycle`, `Esc:cancel`. Fails against the unfixed code quoting
the full row, including the `Esc:cancel` that contradicts the overlay.
### suppressing_the_wizard_hint_leaves_the_rest_of_the_bar
The direction the suppression could overshoot: the mode label `IMPORT` and a real
`status_msg` are not hints and must survive. A fix that blanked the bar in that
mode, or that suppressed the *message* along with the hint, fails here.
### the_status_bar_still_advertises_the_tile_keys
The counterweight to the tile bar deletion, and the test that rejects the
ticket's own AC: Normal must still say `T:tiles`, and TileSelect must still carry
`Enter/Space:cycle`, `r:row` and `Esc/Tab:back`.
Fault injected: suppressing the hint fallback unconditionally
(`true => String::new()`) rather than for `ImportWizard` alone. All three wizard
tests pass — including both of the ones *aimed* at the suppression — and only
this one fails. That is the shape design-principles §7 names: the assertion next
door is satisfied by the wrong fix, and the discriminating input is the mode the
change was not supposed to touch.
src/draw.rs
+58
View File
@@ -454,6 +454,9 @@ fn draw_status(f: &mut Frame, area: Rect, app: &App) {
let msg = match app.view_state.status_msg.is_empty() {
false => app.view_state.status_msg.clone(),
// The wizard overlay draws its own per-step hints; one static table
// under it contradicts them on four of eight steps (improvise-rr8n).
true if matches!(app.view_state.mode, AppMode::ImportWizard) => String::new(),
true => app.hint_text(),
};
@@ -584,6 +587,61 @@ mod bottom_bar_tests {
.collect()
}
/// 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]
fn the_wizard_overlay_is_the_only_key_hint_on_screen() {
let mut app = App::new(Workbook::new("T"), None);
app.view_state.mode = AppMode::ImportWizard;
let row = bottom_row(&app, 100);
for token in ["Space:toggle", "c:cycle", "Esc:cancel"] {
assert!(
!row.contains(token),
"the status bar names {token:?} under an overlay that contradicts \
it on four of eight steps: {row:?}"
);
}
}
/// Suppressing the hint must not blank the bar: the mode label and any
/// status message still belong there. improvise-rr8n.
#[test]
fn suppressing_the_wizard_hint_leaves_the_rest_of_the_bar() {
let mut app = App::new(Workbook::new("T"), None);
app.view_state.mode = AppMode::ImportWizard;
assert!(
bottom_row(&app, 100).contains("IMPORT"),
"the mode label is not a hint and must survive"
);
app.view_state.status_msg = "no array found in that JSON".to_string();
assert!(
bottom_row(&app, 100).contains("no array found in that JSON"),
"a real status message must still reach the bar"
);
}
/// The counterweight to deleting the tile bar's own hint: the tile keys are
/// still advertised, by the derived line. improvise-rr8n.
#[test]
fn the_status_bar_still_advertises_the_tile_keys() {
let mut app = App::new(Workbook::new("T"), None);
assert!(
bottom_row(&app, 200).contains("T:tiles"),
"Normal must still say how to reach tile select"
);
app.view_state.mode = AppMode::TileSelect;
let row = bottom_row(&app, 200);
for token in ["Enter/Space:cycle", "r:row", "Esc/Tab:back"] {
assert!(
row.contains(token),
"TileSelect lost {token:?}, which the tile bar's own hint never \
carried in full: {row:?}"
);
}
}
fn app_with_minibuffer_and_message() -> App {
let mut app = App::new(Workbook::new("T"), None);
app.view_state.mode = AppMode::category_add();
src/ui/tile_bar.rs
+43 -13
View File
@@ -126,21 +126,10 @@ impl<'a> Widget for TileBar<'a> {
// Draw overflow indicator if tiles remain after the visible area
if last_drawn + 1 < labels.len() && x < area.x + area.width {
buf.set_string(x, area.y, "▶", Style::default().fg(Color::DarkGray));
x += 1;
}
// Hint
if matches!(self.mode, AppMode::TileSelect) {
let hint = " [Enter] cycle axis [r/c/p] set axis [←→] select [Esc] cancel";
if x + hint.width() as u16 <= area.x + area.width {
buf.set_string(x, area.y, hint, Style::default().fg(Color::DarkGray));
}
} else {
let hint = " Ctrl+↑↓←→ to move tiles";
if x + hint.width() as u16 <= area.x + area.width {
buf.set_string(x, area.y, hint, Style::default().fg(Color::DarkGray));
}
}
// The bar draws tiles and advertises no keys: the status line's hint is
// derived from the keymap and this one was not (improvise-rr8n).
}
}
@@ -218,4 +207,45 @@ mod tests {
" Tiles: [_Index ·] [_Dim ·] [_Measure Col] [Region Row] [Product Col]"
);
}
/// The tile bar advertised keys that do something else (improvise-rr8n) —
/// `context/test-evidence/tui.md`.
#[test]
fn the_tile_bar_advertises_no_keys_of_its_own() {
let app = app_with_two_categories();
let cache = project::tile_bar_of(&app);
for mode in [AppMode::Normal, AppMode::TileSelect] {
// Wide enough that a hint would fit: the shipped 80-column fixture
// is too narrow to draw one, which is how the false claim survived.
let area = Rect::new(0, 0, 140, 1);
let mut buf = Buffer::empty(area);
TileBar::new(&cache, &mode, 0).render(area, &mut buf);
let text = buf_text(&buf);
for token in ["Ctrl", "Enter", "Esc", "←→", "move tiles"] {
assert!(
!text.contains(token),
"the tile bar names {token:?} in {mode:?}, and nothing checks \
that claim against the keymap: {text:?}"
);
}
}
}
/// The tiles themselves are what the bar is for, and deleting the hint must
/// not delete them. improvise-rr8n.
#[test]
fn the_tile_bar_still_draws_its_tiles_at_every_width_it_used_to() {
let app = app_with_two_categories();
let cache = project::tile_bar_of(&app);
for width in [80u16, 140] {
let area = Rect::new(0, 0, width, 1);
let mut buf = Buffer::empty(area);
TileBar::new(&cache, &AppMode::TileSelect, 0).render(area, &mut buf);
let text = buf_text(&buf);
assert!(
text.contains("[Region Row]") && text.contains("Tiles:"),
"at width {width} the bar lost its tiles: {text:?}"
);
}
}
}