From b945a7f881222a43ddcbefc5212f50771a9d6e6e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Thu, 24 Sep 2026 18:25:40 -0700 Subject: [PATCH 1/2] test: the TUI cursor stays drawn beside a side panel and on the tile bar (improvise-gtn7.1) Both fail on gh/main: the grid at column 4 with any side panel open, the tile bar at width 42. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- src/draw.rs | 78 ++++++++++++++++++++++++++++++++++++++++++++++ src/ui/tile_bar.rs | 32 +++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/src/draw.rs b/src/draw.rs index 78110a7f..aea8452b 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -1120,3 +1120,81 @@ mod title_bar_tests { ); } } + +#[cfg(test)] +mod content_tests { + use super::{draw, press}; + use crate::command::key::{KeyCode, KeyEvent, KeyModifiers}; + use crate::model::cell::{CellKey, CellValue}; + use crate::ui::app::App; + use crate::view::Axis; + use crate::workbook::Workbook; + use ratatui::{Terminal, backend::TestBackend, style::Color}; + + const COLS: usize = 12; + + /// One row, twelve columns, and a value per column that names it. + fn wide_grid() -> Workbook { + let mut wb = Workbook::new("T"); + wb.add_category("Row").unwrap(); + wb.add_category("Col").unwrap(); + wb.add_item("Row", "R1").unwrap(); + for i in 0..COLS { + let col = format!("Column_{i:02}"); + wb.add_item("Col", &col).unwrap(); + let key = CellKey::new(vec![("Row".into(), "R1".into()), ("Col".into(), col)]); + wb.model.set_cell(key, CellValue::Number((101 + i) as f64)); + } + wb.active_view_mut().set_axis("Row", Axis::Row); + wb.active_view_mut().set_axis("Col", Axis::Column); + wb + } + + /// The text the whole frame draws in the selected cell's style. + fn selected_text(app: &App) -> String { + let (w, h) = (app.term_width, app.term_height); + let mut terminal = Terminal::new(TestBackend::new(w, h)).expect("test backend"); + terminal.draw(|f| draw(f, app)).expect("draw"); + terminal + .backend() + .buffer() + .content() + .iter() + .filter(|c| c.bg == Color::Cyan && c.fg == Color::Black) + .map(|c| c.symbol()) + .collect() + } + + /// improvise-gtn7.1: the cursor went behind a side panel — + /// `context/test-evidence/tui.md`. + #[test] + fn moving_right_keeps_the_selected_cell_drawn_beside_any_side_panel() { + let panels: [(&str, fn(&mut App)); 4] = [ + ("no", |_| {}), + ("formula", |a| a.view_state.formula_panel_open = true), + ("category", |a| a.view_state.category_panel_open = true), + ("view", |a| a.view_state.view_panel_open = true), + ]; + for (panel, open) in panels { + let mut app = App::new(wide_grid(), None); + (app.term_width, app.term_height) = (80, 24); + // The category panel is open at launch; close all three first. + let vs = &mut app.view_state; + (vs.formula_panel_open, vs.category_panel_open, vs.view_panel_open) = + (false, false, false); + open(&mut app); + for step in 1..COLS { + press(&mut app, KeyEvent::new(KeyCode::Right, KeyModifiers::NONE)).unwrap(); + let col = app.cursor().selected.1; + assert_eq!(col, step, "setup: {panel} panel, the cursor moved"); + let drawn = selected_text(&app); + assert!( + drawn.contains(&(101 + col).to_string()), + "{panel} panel: column {col} is selected (offset {}) but not \ + drawn; the selection drew {drawn:?}", + app.cursor().col_offset + ); + } + } + } +} diff --git a/src/ui/tile_bar.rs b/src/ui/tile_bar.rs index 744c7458..ece68cb3 100644 --- a/src/ui/tile_bar.rs +++ b/src/ui/tile_bar.rs @@ -231,6 +231,38 @@ mod tests { } } + /// improvise-gtn7.1: the tile cursor could scroll off the bar — + /// `context/test-evidence/tui.md`. + #[test] + fn the_selected_tile_is_drawn_at_every_width_it_fits_in() { + let mut wb = Workbook::new("Test"); + for i in 0..8 { + wb.add_category(&format!("Category_{i}")).unwrap(); + } + let app = App::new(wb, None); + let cache = project::tile_bar_of(&app); + let names: Vec<&str> = cache.tiles.iter().map(|t| t.category.as_str()).collect(); + assert!(names.len() > 8, "setup: more tiles than any tested width holds"); + for width in 40u16..=200 { + for (sel, name) in names.iter().enumerate() { + let area = Rect::new(0, 0, width, 1); + let mut buf = Buffer::empty(area); + TileBar::new(&cache, &AppMode::TileSelect, sel).render(area, &mut buf); + let highlighted: String = buf + .content() + .iter() + .filter(|c| c.bg == Color::Cyan) + .map(|c| c.symbol()) + .collect(); + assert!( + highlighted.contains(&format!("[{name} ")), + "width {width}: tile {sel} ({name}) is selected but not drawn: {:?}", + buf_text(&buf) + ); + } + } + } + /// The tiles themselves are what the bar is for, and deleting the hint must /// not delete them. improvise-rr8n. #[test] From 241cc5f3843e91f6be83d79c445b8311fcd92d93 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Thu, 24 Sep 2026 18:32:47 -0700 Subject: [PATCH 2/2] fix(tui): the grid and the tile bar scroll by the width they are drawn in (improvise-gtn7.1) Grid: App::cmd_context counted visible_cols from term_width while draw_content gave the grid term_width minus the 32-column side panel, so with any panel open the cursor was scrolled past the last drawn column. viewport::grid_width, reached through App::grid_width, is now the one answer: draw_content splits on it and the keypress path counts on it. Tile bar: the scroll measured from the end of the prefix while the draw began one column later, two once scrolled (the left arrow), so the selected tile could be dropped. tiles_left(start) is now where both the scroll (first_tile) and the draw begin. The grid defect dates from b5418f2e; it became the launch state when 50e4a372 opened the category panel by default, which is why it read as a regression. Mixed-width columns still lose the cursor with no panel open: improvise-gtn7.2. Reasoning: context/code/tui.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tui.md | 39 ++++++++++++ context/test-evidence/tui.md | 67 +++++++++++++++++++++ crates/improvise-command/src/ui/app.rs | 17 +++++- crates/improvise-command/src/ui/viewport.rs | 19 +++++- src/draw.rs | 45 ++++++-------- src/ui/tile_bar.rs | 58 ++++++++---------- 6 files changed, 181 insertions(+), 64 deletions(-) diff --git a/context/code/tui.md b/context/code/tui.md index 964b9901..15e0b7dd 100644 --- a/context/code/tui.md +++ b/context/code/tui.md @@ -220,6 +220,45 @@ 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 grid's width is one answer, and a scroll is measured where it is drawn + +Two parts of the TUI scroll horizontally, and both lost their cursor for the +same reason: **the code that chose the scroll offset measured a different width +from the code that drew it** (improvise-gtn7.1). + +- **The grid.** `App::cmd_context` counts `visible_cols` for the keypress path, + and `GridWidget` counts again from the `Rect` it is handed. The keypress path + counted from `term_width`, while `draw_content` gave the grid `term_width` + minus the 32-column side panel. So with any of the three panels open the + cursor was scrolled to a column the widget never drew: it went "behind the + panel". Now `viewport::grid_width(term_width, side_panel_open)` is the one + answer, reached through `App::grid_width`. `draw_content` splits the content + row on it, and `cmd_context` counts on it. `App::side_panel_open` is the one + answer to "is a panel open", so a panel that did not count toward the width + would not be drawn either, and the two sides still agree. +- **The tile bar.** `TileBar` chose its first tile against the width after + the prefix, but drew tiles one column later, and two columns later once + scrolled, to leave room for `◀`. When the selected tile fitted within those + one or two columns, the scroll stopped too early and the draw loop dropped the + tile. `tiles_left(start)` is now where both the scroll (`first_tile`) and the + draw begin. + +**Why it looked like a regression.** The grid defect dates from `b5418f2e` +(2026-04-06), when `compute_visible_cols` replaced a rough estimate and took +`term_width`. It was always wrong with a panel open. But until `50e4a372` +(2026-09-20) no panel was open at launch. That commit opened the category panel +by default, in the shared `ViewState::default()`, and from then on every TUI +session started in the broken configuration. The tile bar's offset has been +wrong since `4686f470` (2026-04-09), when it first scrolled. + +**What is still wrong: improvise-gtn7.2.** `visible_cols` is a *count*, taken at +the *old* offset, and `viewport_effects` applies it at the *new* one. Across +columns of mixed width no single count is right, so the cursor can still leave +the grid with no panel open. The fix is to measure the new offset against the +widths, as `first_tile` does. It is not in this change because +`resolve.rs::scrolled_to` mirrors `viewport_effects` over a window counted in +columns, and `lockstep.rs` holds the two in agreement. + ### The caches are built here, per frame `project::formulas_of`, `category_tree_of`, `view_list_of`, `tile_bar_of`, diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 580959dd..0086044d 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -1261,3 +1261,70 @@ 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. + +### moving_right_keeps_the_selected_cell_drawn_beside_any_side_panel + +improvise-gtn7.1, the grid half. The user's report was that "the cursor can go +behind the category screen". The keypress path counted `visible_cols` from +`term_width`, while `draw_content` gave the grid `term_width - 32` whenever a +side panel was open. So `viewport_effects` scrolled only after the cursor had +passed a column the widget never drew. The reasoning, and why it read as a +regression, is in `context/code/tui.md`. + +**The test is a boundary test, on purpose.** The defect was a disagreement +between two measurements, one in `improvise-command` and one in the frame +layout. A unit test of either would pass against the other. So the test renders +the real `draw` into an 80×24 `TestBackend`, presses `Right` eleven times across +twelve columns through `press`, and after each press requires the selected +cell's text to be on screen. Each column holds a distinct value (`101 + i`), and +the selection is read off the cells drawn in the selection style (black on +cyan). So "the cursor is somewhere" and "this column is drawn" cannot be +confused. + +It runs four times: no panel, and each of the three panels alone. The no-panel +run is the control, and it passes on the unfixed code. + +**The fixture was nearly vacuous.** The first version's "no panel" run failed +too, because `ViewState::default()` opens the category panel at launch +(`50e4a372`). So the control was a fourth copy of the category case. All three +panels are now closed before one is opened. That launch default is also why a +defect from April read as a regression in September. + +Red at `ae5cf408`: `formula panel: column 4 is selected (offset 0) but not +drawn`. Seven 10-cell columns fit beside a 4-cell row header in 78 cells, but +only four fit in 46. + +Rivals, each injected and restored: +- **`cmd_context` counts on `self.term_width` again:** red, `formula panel: + column 4`. +- **`draw_content` splits 8 columns narrower than `grid_width`**, a second + derivation that disagrees with the first: red, `no panel: column 6`. +- **`side_panel_open` counts only the category panel:** green, and correctly. + `draw_content` asks the same function whether to draw a panel, so it draws no + formula panel, the grid gets the full width, and the two sides still agree. + This is the benefit of one derivation: a wrong answer there is wrong + everywhere at once, not a disagreement. + +Not covered: columns of mixed width with no panel open still lose the cursor, +for a different reason (improvise-gtn7.2). + +### the_selected_tile_is_drawn_at_every_width_it_fits_in + +improvise-gtn7.1, the tile half: "the cursor goes off screen completely when +navigating the tiles". `TileBar::render` chose its first tile against +`area.width - prefix`, but drew from `prefix + 1`, and from `prefix + 2` once +scrolled (for the `◀`). When the selected tile fitted within the one or two +columns the scroll had over-counted, the scroll stopped early and the draw loop +`break`s before it, so the tile vanished. Red at `ae5cf408`: `width 42: tile 3 +(Category_0) is selected but not drawn: " Tiles: ◀ [_Measure Pag] ▶"`. + +The test enumerates rather than picks. It uses eight regular categories, which +with the three virtual ones gives eleven tiles. It checks every width from 40 to +200 and every tile as the selection, and requires the selected tile's label on +a cyan cell. The failing band is one or two columns wide at each scroll +position, so a hand-picked width would very likely have missed it. + +Rivals, injected into `first_tile` and restored: +- **Measure from the end of the prefix**, as the old loop did: red at width 42. +- **Forget only the `◀` column** (`prefix + 1`): red at width 43. So the test + sees an off-by-one, not only the two-column case. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 104b15ad..df08149c 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -23,7 +23,9 @@ use crate::model::cell::{CellKey, CellValue, DataStore}; use crate::persistence; use crate::ui::effect::{EffectKind, ModelChange, ViewTarget}; use crate::ui::project::Dirty; -use crate::ui::viewport::{compute_col_widths, compute_row_header_width, compute_visible_cols}; +use crate::ui::viewport::{ + compute_col_widths, compute_row_header_width, compute_visible_cols, grid_width, +}; use crate::view::{GridLayout, View}; use crate::workbook::Workbook; use improvise_core::model::record::{AggregateHash, RecordHash}; @@ -556,6 +558,17 @@ impl App { app } + /// Whether any of the three side panels has taken its column. + pub fn side_panel_open(&self) -> bool { + let vs = &self.view_state; + vs.formula_panel_open || vs.category_panel_open || vs.view_panel_open + } + + /// The terminal columns the grid gets (improvise-gtn7.1). + pub fn grid_width(&self) -> u16 { + grid_width(self.term_width, self.side_panel_open()) + } + /// This client's cursor in the active view. Absent means the origin, so /// a view visited for the first time opens at (0, 0). pub fn cursor(&self) -> ViewCursor { @@ -961,7 +974,7 @@ impl App { compute_visible_cols( &col_widths, row_header_width, - self.term_width, + self.grid_width(), cursor.col_offset, ) }, diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index 256de097..32905e8e 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -161,15 +161,30 @@ pub fn compute_row_header_width(layout: &GridLayout) -> u16 { .sum() } +/// The TUI's side-panel column, and the least the grid keeps beside it. +pub const SIDE_PANEL_WIDTH: u16 = 32; +pub const MIN_GRID_WIDTH: u16 = 40; + +/// The grid's width once any side panel has its column: the one answer the +/// keypress path and the frame layout share (`context/code/tui.md`). +pub fn grid_width(term_width: u16, side_panel_open: bool) -> u16 { + match side_panel_open { + true => term_width + .saturating_sub(SIDE_PANEL_WIDTH) + .max(MIN_GRID_WIDTH.min(term_width)), + false => term_width, + } +} + /// Count how many columns fit starting from `col_offset` given the available width. pub fn compute_visible_cols( col_widths: &[u16], row_header_width: u16, - term_width: u16, + grid_width: u16, col_offset: usize, ) -> usize { // Account for grid border (2 chars) - let data_area_width = term_width + let data_area_width = grid_width .saturating_sub(2) .saturating_sub(row_header_width); let mut acc = 0u16; diff --git a/src/draw.rs b/src/draw.rs index aea8452b..65a8eb52 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -323,21 +323,18 @@ fn draw_title(f: &mut Frame, area: Rect, app: &App) { } fn draw_content(f: &mut Frame, area: Rect, app: &App) { - let side_open = app.view_state.formula_panel_open - || app.view_state.category_panel_open - || app.view_state.view_panel_open; - - let grid_area; - if side_open { - let side_w = 32u16; - let chunks = Layout::default() - .direction(Direction::Horizontal) - .constraints([Constraint::Min(40), Constraint::Length(side_w)]) - .split(area); - - grid_area = chunks[0]; - - let side = chunks[1]; + // Split on `App::grid_width`, which the keypress path scrolls by too. + let grid_w = app.grid_width().min(area.width); + let grid_area = Rect { + width: grid_w, + ..area + }; + if app.side_panel_open() { + let side = Rect { + x: area.x + grid_w, + width: area.width - grid_w, + ..area + }; let panel_count = [ app.view_state.formula_panel_open, app.view_state.category_panel_open, @@ -388,8 +385,6 @@ fn draw_content(f: &mut Frame, area: Rect, app: &App) { a, ); } - } else { - grid_area = area; } // The viewport covers the WHOLE grid, so a cache position is an absolute @@ -1169,20 +1164,14 @@ mod content_tests { /// `context/test-evidence/tui.md`. #[test] fn moving_right_keeps_the_selected_cell_drawn_beside_any_side_panel() { - let panels: [(&str, fn(&mut App)); 4] = [ - ("no", |_| {}), - ("formula", |a| a.view_state.formula_panel_open = true), - ("category", |a| a.view_state.category_panel_open = true), - ("view", |a| a.view_state.view_panel_open = true), - ]; - for (panel, open) in panels { + for panel in ["no", "formula", "category", "view"] { let mut app = App::new(wide_grid(), None); (app.term_width, app.term_height) = (80, 24); - // The category panel is open at launch; close all three first. + // The category panel is open at launch, so each run sets all three. let vs = &mut app.view_state; - (vs.formula_panel_open, vs.category_panel_open, vs.view_panel_open) = - (false, false, false); - open(&mut app); + vs.formula_panel_open = panel == "formula"; + vs.category_panel_open = panel == "category"; + vs.view_panel_open = panel == "view"; for step in 1..COLS { press(&mut app, KeyEvent::new(KeyCode::Right, KeyModifiers::NONE)).unwrap(); let col = app.cursor().selected.1; diff --git a/src/ui/tile_bar.rs b/src/ui/tile_bar.rs index ece68cb3..60fac265 100644 --- a/src/ui/tile_bar.rs +++ b/src/ui/tile_bar.rs @@ -10,6 +10,26 @@ use crate::protocol::TileBarCache; use crate::ui::app::AppMode; use crate::view::Axis; +const PREFIX: &str = " Tiles: "; + +/// Where the first drawn tile begins: past the prefix and a gap, and past +/// the `◀` once the bar has scrolled. +fn tiles_left(start: usize) -> u16 { + PREFIX.width() as u16 + 1 + u16::from(start > 0) +} + +/// Scroll by whole tiles: the least start from which tile `sel` is drawn +/// whole, measured from where [`tiles_left`] draws (improvise-gtn7.1). +fn first_tile(widths: &[u16], sel: usize, width: u16) -> usize { + let sel = sel.min(widths.len().saturating_sub(1)); + (0..sel) + .find(|&start| { + let used: u32 = widths[start..=sel].iter().map(|&w| u32::from(w)).sum(); + used <= u32::from(width.saturating_sub(tiles_left(start))) + }) + .unwrap_or(sel) +} + pub struct TileBar<'a> { pub tiles: &'a TileBarCache, pub mode: &'a AppMode, @@ -45,9 +65,8 @@ impl<'a> Widget for TileBar<'a> { None }; - let prefix = " Tiles: "; - let prefix_w = prefix.width() as u16; - buf.set_string(area.x, area.y, prefix, Style::default().fg(Color::Gray)); + let prefix_w = PREFIX.width() as u16; + buf.set_string(area.x, area.y, PREFIX, Style::default().fg(Color::Gray)); let tiles = &self.tiles.tiles; @@ -61,34 +80,10 @@ impl<'a> Widget for TileBar<'a> { .collect(); let widths: Vec = labels.iter().map(|l| l.width() as u16).collect(); - // Available space for tiles (after prefix) - let avail = area.width.saturating_sub(prefix_w); - - // Scroll by whole tiles: the smallest start index at which the - // selected tile still fits in `avail`. - let sel = selected_cat_idx.unwrap_or(0); - let mut start = 0; - loop { - // Check if selected tile is visible when starting from `start` - let mut used: u16 = 0; - let mut sel_visible = false; - for (i, w) in widths.iter().enumerate().take(labels.len()).skip(start) { - if used + *w > avail { - break; - } - used += *w; - if i == sel { - sel_visible = true; - } - } - if sel_visible || start >= sel { - break; - } - start += 1; - } + let start = first_tile(&widths, selected_cat_idx.unwrap_or(0), area.width); // Draw an overflow indicator if we scrolled past the beginning - let mut x = area.x + prefix_w + 1; + let mut x = area.x + tiles_left(start); if start > 0 { buf.set_string( area.x + prefix_w, @@ -96,7 +91,6 @@ impl<'a> Widget for TileBar<'a> { "◀", Style::default().fg(Color::DarkGray), ); - x += 1; } // Render tiles from `start` @@ -237,12 +231,12 @@ mod tests { fn the_selected_tile_is_drawn_at_every_width_it_fits_in() { let mut wb = Workbook::new("Test"); for i in 0..8 { - wb.add_category(&format!("Category_{i}")).unwrap(); + wb.add_category(format!("Category_{i}")).unwrap(); } let app = App::new(wb, None); let cache = project::tile_bar_of(&app); let names: Vec<&str> = cache.tiles.iter().map(|t| t.category.as_str()).collect(); - assert!(names.len() > 8, "setup: more tiles than any tested width holds"); + assert!(names.len() > 8, "setup: the narrow widths must scroll"); for width in 40u16..=200 { for (sel, name) in names.iter().enumerate() { let area = Rect::new(0, 0, width, 1);