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

fix(tui): the grid and the tile bar scroll by the width they are drawn in (improvise-gtn7.1)

Browse Source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Edward Langley
2026-09-24 18:32:47 -07:00
parent b945a7f881
commit 241cc5f384
6 changed files with 181 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/tui.md
+39
View File
@@ -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`,
context/test-evidence/tui.md
+67
View File
@@ -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.
crates/improvise-command/src/ui/app.rs
+15 -2
View File
@@ -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,
)
},
crates/improvise-command/src/ui/viewport.rs
+17 -2
View File
@@ -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;
src/draw.rs
+17 -28
View File
@@ -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;
src/ui/tile_bar.rs
+26 -32
View File
@@ -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<u16> = 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);