Merge remote-tracking branch 'gh/main' into feat/cgqt-import-collision-step

# Conflicts:
#	context/test-evidence/tui.md
This commit is contained in:
Edward Langley
2026-09-24 18:43:40 -07:00
6 changed files with 278 additions and 51 deletions
+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 everything else and let the status bar carry them correctly instead of being
switched off. 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 ### The caches are built here, per frame
`project::formulas_of`, `category_tree_of`, `view_list_of`, `tile_bar_of`, `project::formulas_of`, `category_tree_of`, `view_list_of`, `tile_bar_of`,
+67
View File
@@ -1262,6 +1262,73 @@ 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 transition, so it is asserted on `EditorSession::of` directly. The rival that
compares `std::mem::discriminant` fails it, and passes every other test. 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.
### wrapping_keeps_every_word_within_the_width ### wrapping_keeps_every_word_within_the_width
improvise-cgqt: the Confirm step drew each warning with `truncate`, so the improvise-cgqt: the Confirm step drew each warning with `truncate`, so the
+15 -2
View File
@@ -23,7 +23,9 @@ use crate::model::cell::{CellKey, CellValue, DataStore};
use crate::persistence; use crate::persistence;
use crate::ui::effect::{EffectKind, ModelChange, ViewTarget}; use crate::ui::effect::{EffectKind, ModelChange, ViewTarget};
use crate::ui::project::Dirty; 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::view::{GridLayout, View};
use crate::workbook::Workbook; use crate::workbook::Workbook;
use improvise_core::model::record::{AggregateHash, RecordHash}; use improvise_core::model::record::{AggregateHash, RecordHash};
@@ -556,6 +558,17 @@ impl App {
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 /// This client's cursor in the active view. Absent means the origin, so
/// a view visited for the first time opens at (0, 0). /// a view visited for the first time opens at (0, 0).
pub fn cursor(&self) -> ViewCursor { pub fn cursor(&self) -> ViewCursor {
@@ -961,7 +974,7 @@ impl App {
compute_visible_cols( compute_visible_cols(
&col_widths, &col_widths,
row_header_width, row_header_width,
self.term_width, self.grid_width(),
cursor.col_offset, cursor.col_offset,
) )
}, },
+17 -2
View File
@@ -161,15 +161,30 @@ pub fn compute_row_header_width(layout: &GridLayout) -> u16 {
.sum() .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. /// Count how many columns fit starting from `col_offset` given the available width.
pub fn compute_visible_cols( pub fn compute_visible_cols(
col_widths: &[u16], col_widths: &[u16],
row_header_width: u16, row_header_width: u16,
term_width: u16, grid_width: u16,
col_offset: usize, col_offset: usize,
) -> usize { ) -> usize {
// Account for grid border (2 chars) // Account for grid border (2 chars)
let data_area_width = term_width let data_area_width = grid_width
.saturating_sub(2) .saturating_sub(2)
.saturating_sub(row_header_width); .saturating_sub(row_header_width);
let mut acc = 0u16; let mut acc = 0u16;
+84 -17
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) { fn draw_content(f: &mut Frame, area: Rect, app: &App) {
let side_open = app.view_state.formula_panel_open // Split on `App::grid_width`, which the keypress path scrolls by too.
|| app.view_state.category_panel_open let grid_w = app.grid_width().min(area.width);
|| app.view_state.view_panel_open; let grid_area = Rect {
width: grid_w,
let grid_area; ..area
if side_open { };
let side_w = 32u16; if app.side_panel_open() {
let chunks = Layout::default() let side = Rect {
.direction(Direction::Horizontal) x: area.x + grid_w,
.constraints([Constraint::Min(40), Constraint::Length(side_w)]) width: area.width - grid_w,
.split(area); ..area
};
grid_area = chunks[0];
let side = chunks[1];
let panel_count = [ let panel_count = [
app.view_state.formula_panel_open, app.view_state.formula_panel_open,
app.view_state.category_panel_open, app.view_state.category_panel_open,
@@ -388,8 +385,6 @@ fn draw_content(f: &mut Frame, area: Rect, app: &App) {
a, a,
); );
} }
} else {
grid_area = area;
} }
// The viewport covers the WHOLE grid, so a cache position is an absolute // The viewport covers the WHOLE grid, so a cache position is an absolute
@@ -1120,3 +1115,75 @@ 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() {
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, so each run sets all three.
let vs = &mut app.view_state;
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;
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
);
}
}
}
}
+56 -30
View File
@@ -10,6 +10,26 @@ use crate::protocol::TileBarCache;
use crate::ui::app::AppMode; use crate::ui::app::AppMode;
use crate::view::Axis; 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 struct TileBar<'a> {
pub tiles: &'a TileBarCache, pub tiles: &'a TileBarCache,
pub mode: &'a AppMode, pub mode: &'a AppMode,
@@ -45,9 +65,8 @@ impl<'a> Widget for TileBar<'a> {
None None
}; };
let prefix = " Tiles: "; let prefix_w = PREFIX.width() as u16;
let prefix_w = prefix.width() as u16; buf.set_string(area.x, area.y, PREFIX, Style::default().fg(Color::Gray));
buf.set_string(area.x, area.y, prefix, Style::default().fg(Color::Gray));
let tiles = &self.tiles.tiles; let tiles = &self.tiles.tiles;
@@ -61,34 +80,10 @@ impl<'a> Widget for TileBar<'a> {
.collect(); .collect();
let widths: Vec<u16> = labels.iter().map(|l| l.width() as u16).collect(); let widths: Vec<u16> = labels.iter().map(|l| l.width() as u16).collect();
// Available space for tiles (after prefix) let start = first_tile(&widths, selected_cat_idx.unwrap_or(0), area.width);
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;
}
// Draw an overflow indicator if we scrolled past the beginning // 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 { if start > 0 {
buf.set_string( buf.set_string(
area.x + prefix_w, area.x + prefix_w,
@@ -96,7 +91,6 @@ impl<'a> Widget for TileBar<'a> {
"◀", "◀",
Style::default().fg(Color::DarkGray), Style::default().fg(Color::DarkGray),
); );
x += 1;
} }
// Render tiles from `start` // Render tiles from `start`
@@ -231,6 +225,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: 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);
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 /// The tiles themselves are what the bar is for, and deleting the hint must
/// not delete them. improvise-rr8n. /// not delete them. improvise-rr8n.
#[test] #[test]