diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 3c153204..89414b9c 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -680,15 +680,20 @@ record lands is `Plan::place`: its categories, then its date parts, then its labels, then its measure values, or a reason it is dropped. `collisions()` and `build_model_with_warnings` both call it, so the question the wizard asks and the thing the build does cannot disagree. Neither has a second walk of the -record. A collision is a tuple reached by **two or more records that each -carry a measure value**. A record with no measure makes no cell, so it cannot -collide. The tuple includes label values, as `CellKey` always has. +record. **A collision is a cell that more than one record writes**, meaning a +tuple plus a measure (improvise-9bit). A `Collision` names the tuple and only +the records that share one of its cells. A record with no measure makes no +cell, so it collides with nothing. The tuple includes label values, as +`CellKey` always has. -Two records on one tuple with **disjoint** measures (one has `Revenue`, the -other `Cost`) count as a collision even though summing them loses nothing. The -unit is the record, the user's own words were "rows collide on every -coordinate", and a cell-level rule would make "which records collided" depend -on which measure you look at. +This PR's first version keyed on the tuple alone. It was **superseded in review +(improvise-9bit)**, and the argument it gave is withdrawn. Under that rule, two +records with **disjoint** measures on one tuple (one has `Revenue`, the other +`Cost`) counted as colliding although no cell sums. The wizard then offered +"Sum them into one cell" where nothing would sum, and with the `Refuse` default +the CLI refused a sparse-measure CSV that had always imported cleanly. The +question the user is asked is "these values would be added together", so the +rule has to be the one that decides whether anything is added. The data is structured. A `Collision` holds `coords` plus 1-based `records` positions. `WizardCache::collisions` carries it, and @@ -710,8 +715,10 @@ at `Refuse`, which `validation_error` blocks, so a bare advance could never leave the step. **Separate is an occurrence index, not a row number.** This is the user's -preferred shape. Records on a colliding tuple get `1, 2, 3…` in source order, -and every other record gets `1`. The category holds only as many items as the +preferred shape. The records in a collision get `1, 2, 3…` in source order, +and every other record gets `1`. That includes a record on the same tuple that +shares no cell with the others: it lands on occurrence `1` beside the first +colliding record, and since the two write different measures, nothing sums. The category holds only as many items as the deepest collision, rather than one item per record, and every existing coordinate keeps its meaning. Items are added `1..=n` in numeric order (gotcha 18). The category goes on **`Axis::None`**. Left to diff --git a/context/code/tui.md b/context/code/tui.md index 43f3d864..a792bb5e 100644 --- a/context/code/tui.md +++ b/context/code/tui.md @@ -569,10 +569,14 @@ one-line message therefore draws exactly as before, which is why the pinned frames (`every_step_draws_what_it_drew_before`, `the_formula_step_draws_its_transient_state`) did not move. -**Lists scroll by item, and say what they hide.** `draw_scrolling` draws the -items from `WizardCache::scroll` on, each one wrapped whole. It writes a -`▲ n more above` or `▼ n more below` row instead of cutting an item in half. -`ImportWizard::scroll` clamps to `len - 1`, so the last item can reach the top +**Lists scroll by item, and the header says what is shown.** `Panel::scrolling` +draws the items from `WizardCache::scroll` on, each one wrapped whole. The +list's own header row reads `Colliding records 3–9 of 14 (PgUp/PgDn):` whenever +some items are off screen. A first version put "more above" and "more below" +markers inside the list's rows. When the first item filled the list exactly, +the marker landed on the hint row and was drawn over (Codex, PR #304). The +header row belongs to no item, so no item can cover it, and no item loses a line +to make room for a marker. `ImportWizard::scroll` clamps to `len - 1`, so the last item can reach the top and the list can never scroll away. It is by item, not by line, because the wizard state cannot know the panel width that line-level clamping needs. The residual is honest. A **single** item taller than the whole list area, roughly @@ -580,5 +584,17 @@ residual is honest. A **single** item taller than the whole list area, roughly wizard warning is that long today: a collision tuple is bounded by twelve categories plus labels, but a label value is not bounded. +**Every write is clipped to the panel.** The widget draws through `Panel`, +whose `put` writes only inside the panel's inner rectangle, and its height +arithmetic saturates. Before this change, the duplicates step wrote at an +unbounded row and ratatui panicked at 80×10, and four older steps panicked on +`u16` underflow at heights of 3 to 5 (review B2 on PR #304). The bottom row's +`set_bottom_line` now truncates in cells through `ui::text::truncate`. It had +used the widget's byte-measured `truncate`, so a wrapped line holding a `←` +was cut one character early, with `…`, although it fit (review B1). The local +byte `truncate` still draws field names and labels. It is byte-identical on +ASCII, which keeps every pinned frame, and those rows are labels rather than +messages. + Other TUI surfaces still truncate text a user needs. They are out of this ticket's scope and reported in its hand-back rather than fixed here. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 9c5f1513..044a4dca 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1097,12 +1097,15 @@ loop was therefore re-ordered to `[backward, forward]` under the sort fault and observed failing on the backward history specifically, with `left: ["alpha", …]` (sorted) against `right: ["charlie", …]` (backward). -### a_long_collision_list_is_truncated_with_a_count +### every_summed_cell_is_reported_in_full -The warning is rendered into a one-line status bar. A file with ten thousand -duplicate coordinates would otherwise compose a warning naming every one. Three -cells are named and the rest counted; the empty case returns `None` rather than -a warning that says nothing happened. +This replaces `a_long_collision_list_is_truncated_with_a_count`, which pinned +2cw9's three-then-"and N more" warning. After the Sum choice, the Confirm step +still showed "…; and 28 more". The user's rule is that a message they need is +never truncated, so each summed cell is now its own warning, and the step wraps +and scrolls them. It was red at compile time against the old `Option` +signature, then red against a `Vec` that still held one elided sentence. The +rival **elided after three** fails it. ### build_model_warns_about_records_skipped_for_missing_category @@ -1577,3 +1580,37 @@ Also `choosing_go_back_returns_to_the_step_before`, `back_retraces_the_duplicate option" are different. The rival **chooser ignores the cursor** fails six of them. **suggestion ignores headers** and **name check accepts a taken name** each fail their own test. + +### records_with_disjoint_measures_on_one_tuple_do_not_collide + +improvise-9bit, found by the adversarial review of PR #304 at `92233cff`. +`collisions_among` keyed on the tuple alone, so `{East, revenue}` and +`{East, cost}` were reported as colliding although no cell sums. Under the +`Refuse` default, a sparse-measure CSV that imported cleanly before was refused. +It was red before the fix: `collisions()` returned the East group. The rival +**tuple-only rule** fails it, and fails +`only_the_records_that_share_a_cell_are_listed_and_numbered` too. + +### a_record_with_no_measure_collides_with_nothing + +The rule the reviewer's injection `if false && measures.is_empty()` showed was +unguarded. It asserts records `[1, 3]` with the measureless `#2` excluded, and +that the two measureless West records form no collision at all. It was green +before the fix, since the rule already held, so its evidence is the rival: +**a measureless record counts as writing** fails it. + +### only_the_records_that_share_a_cell_are_listed_and_numbered + +A tuple with one `cost` record and two `revenue` records. Only `#2` and `#3` +are listed. Under Separate, the `cost` record keeps occurrence `1`, beside the +first `revenue` record, and the second `revenue` record gets `2`. The rivals +**tuple-only rule** and **a non-colliding record numbered past 1** each fail it. + +### the_name_check_covers_categories_derived_from_dates + +improvise-4klp, filed by the same review. The name step's check against +date-derived categories was untested. The test extracts `when_Year` and types +that name, which must be refused. It was green on arrival, so its evidence is +the reviewer's rival, **date-derived names dropped from the taken names**, +which fails it. That fault would have let `add_category` reuse the existing +category, and every cell key would then name `when_Year` twice. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 29fd9b04..69cf6582 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -1354,8 +1354,78 @@ Its rival **message drawn on one line** fails it. ### scrolling_reaches_the_last_collision Fourteen colliding tuples, more than fit. The first frame must show the first -tuple and a `more below` marker, and not show the last one. After scrolling, -the last tuple and a `more above` marker must be drawn. The rivals **list -ignores the scroll offset** and **no marker for what lies below** each fail it. +tuple and a header naming the range shown, and not show the last one. After scrolling, +the last tuple must be drawn. Since the Codex fix, the header row names the +range shown (`Colliding records 14–14 of 14`) in place of the in-list markers. +The rivals **list ignores the scroll offset** and **the header claims every item +shown** each fail it. `keeping_separate_at_the_cap_is_drawn_unavailable_with_its_reason` fails under **disabled reason not drawn**. + +### a_multibyte_message_is_drawn_whole_at_every_width + +Review B1 on PR #304. Messages were wrapped in cells and then drawn through +`set_bottom_line`, which truncated with the widget's **byte**-measured +`truncate`. A wrapped line holding a `←` (three bytes) that fit in cells was +still cut, with `…`. At 80 columns the category-cap reason lost `(←)`, and +words were lost at 22 panel widths from 30 to 100. The earlier +`a_message_longer_than_the_panel_is_drawn_whole` was ASCII-only and could not +see it. This test drives the real cap reason and a message of accents, CJK +and emoji across every width from 30 to 100. It asserts no `…` and every word +in order. It was red before the fix. The rival **bottom row measured in bytes +again** fails it. + +Its first run exposed a bug in the test's own helper. `buf_text` read the +padding cell after a double-width character as a space, so `日本語` read as +`日 本 語`. The helper now skips that cell. This changes nothing for the ASCII +pinned frames. + +### wrapping_measures_cells_not_bytes + +Review B1's second half. A byte count is never smaller than a cell count, so +byte-wrapping only ever wraps early, and "no line wider than asked" plus "no +word lost" cannot tell the two apart. The reviewer swapped `.width()` for +`.len()` in `wrap_words` and the lib stayed green. This test takes strings that +fit in cells but not in bytes (`←←← éé` is 6 cells and 14 bytes; `日本 ab` is 7 +cells and 10 bytes) and asserts each stays on **one** line. That rival now +fails it. It was green on arrival, since `wrap` was already right. + +### no_step_panics_on_a_small_terminal + +Review B2 on PR #304. The duplicates step wrote at an unbounded row, and +ratatui panicked with index-outside-buffer at 80×≤10, 60×≤12 and 50×≤14. It +sweeps every step the wizard can reach at every size from 20 to 100 wide and +3 to 20 high: + +- Preview, SelectArrayPath, ReviewProposals and ConfigureDates; +- ResolveDuplicates, both plain and at the cap with its message; +- NameDuplicateCategory; +- DefineFormulas, empty and with formulas and an open editor; +- NameModel, and Done with and without warnings. + +It was red before the fix with 3,513 panics, and not only in the new steps. +NameDuplicateCategory panicked up to 13 high, and ResolveDuplicates at every +height to 20. Preview, SelectArrayPath, DefineFormulas and NameModel panicked +at heights 3 to 5 on `u16` underflow in `inner.height - N`, which predates +this ticket. + +The fix is `Panel`, one clipped writer, plus saturating arithmetic. Its rivals +are **writes not clipped to the panel** and **height arithmetic not +saturating**. The second one **survived** at first, because the sweep's +DefineFormulas step had no formulas, so the loop holding `inner.bottom() - 5` +never ran. A DefineFormulas step with formulas was added, and the rival now +fails. + +The steps run in parallel threads, each over its own prebuilt `WizardCache`. +Run serially the sweep took 3.8s, over the 2-second binary limit, and 0.55s +for the whole lib in parallel on an Apple M4 Max. + +### a_first_item_that_fills_the_list_still_says_more_follow + +The Codex P2 on PR #304 at `92233cff`. When the first visible item's wrapped +height exactly filled the list, the "more below" marker was written on the next +row, the hint row, and the hint drew over it. The fix moves the scroll status +to the list's header row, which no item can cover. This test fills three rows +with a three-line item and a second item after it. It asserts the first item +is drawn whole and the header reads `1–1 of 2`. The rival **the header claims +every item shown** fails it. diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index 2dc31540..ccea6a31 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -435,10 +435,13 @@ impl Plan<'_> { } } +/// A tuple, and each record on it: its 1-based position and the measures it writes. +type OnTuple<'a> = (&'a Vec<(String, String)>, Vec<(usize, Vec<&'a str>)>); + /// The tuples where two records write the same cell (improvise-9bit), with /// only the records that do. Disjoint measures on one tuple sum nothing. fn collisions_among(placements: &[Placement]) -> Vec { - let mut tuples: Vec<(&Vec<(String, String)>, Vec<(usize, Vec<&str>)>)> = vec![]; + let mut tuples: Vec = vec![]; let mut index: HashMap = HashMap::new(); for (at, placement) in placements.iter().enumerate() { let Placement::Placed { diff --git a/src/ui/import_wizard_ui.rs b/src/ui/import_wizard_ui.rs index 4567a9d5..87217533 100644 --- a/src/ui/import_wizard_ui.rs +++ b/src/ui/import_wizard_ui.rs @@ -61,6 +61,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { .title(title); let inner = block.inner(popup_area); block.render(popup_area, buf); + let mut panel = Panel { buf, clip: inner }; let mut y = inner.y; let x = inner.x; @@ -69,9 +70,9 @@ impl<'a> Widget for ImportWizardWidget<'a> { match &self.wizard.step { WizardStepKind::Preview => { let summary = &self.wizard.preview_summary; - buf.set_string(x, y, truncate(summary, w), Style::default()); + panel.put(x, y, truncate(summary, w), Style::default()); y += 2; - buf.set_string( + panel.put( x, y, "Press Enter to continue\u{2026}", @@ -79,7 +80,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); } WizardStepKind::SelectArrayPath => { - buf.set_string( + panel.put( x, y, "Select the path containing records:", @@ -87,7 +88,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); y += 1; for (i, path) in self.wizard.array_paths.iter().enumerate() { - if y >= inner.y + inner.height { + if y >= inner.bottom() { break; } let is_sel = i == self.wizard.cursor; @@ -102,11 +103,11 @@ impl<'a> Widget for ImportWizardWidget<'a> { // The root path is already spelled "(root)" by the // projection, so both ends say it the same way. let label = format!(" {path}"); - buf.set_string(x, y, truncate(&label, w), style); + panel.put(x, y, truncate(&label, w), style); y += 1; } y += 1; - buf.set_string( + panel.put( x, y, "\u{2191}\u{2193} select Enter confirm \u{2190} back", @@ -114,7 +115,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); } WizardStepKind::ReviewProposals => { - buf.set_string( + panel.put( x, y, "Review field proposals (Space toggle, c cycle kind):", @@ -122,7 +123,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); y += 1; let header = format!(" {:<20} {:<22} {:<6}", "Field", "Kind", "Accept"); - buf.set_string( + panel.put( x, y, truncate(&header, w), @@ -133,7 +134,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { y += 1; for (i, proposal) in self.wizard.proposals.iter().enumerate() { - if y >= inner.y + inner.height - 2 { + if y >= inner.bottom().saturating_sub(2) { break; } let is_sel = i == self.wizard.cursor; @@ -163,12 +164,11 @@ impl<'a> Widget for ImportWizardWidget<'a> { Style::default().fg(Color::DarkGray) }; - buf.set_string(x, y, truncate(&row, w), style); + panel.put(x, y, truncate(&row, w), style); y += 1; } - let hint_y = inner.y + inner.height - 1; - set_bottom_line( - buf, + let hint_y = inner.bottom().saturating_sub(1); + panel.bottom_line( x, hint_y, w, @@ -177,7 +177,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); } WizardStepKind::ConfigureDates => { - buf.set_string( + panel.put( x, y, "Select date components to extract (Space toggle):", @@ -187,11 +187,11 @@ impl<'a> Widget for ImportWizardWidget<'a> { let mut item_idx = 0; for proposal in &self.wizard.date_fields { - if y >= inner.y + inner.height - 2 { + if y >= inner.bottom().saturating_sub(2) { break; } let header = format!(" {} (format: {})", proposal.field, proposal.format); - buf.set_string( + panel.put( x, y, truncate(&header, w), @@ -204,7 +204,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { // Components arrive in the wizard's own order, already // paired with their state, so the cursor counts these rows. for (label, enabled) in &proposal.components { - if y >= inner.y + inner.height - 2 { + if y >= inner.bottom().saturating_sub(2) { break; } let enabled = *enabled; @@ -221,14 +221,13 @@ impl<'a> Widget for ImportWizardWidget<'a> { } else { Style::default().fg(Color::DarkGray) }; - buf.set_string(x, y, truncate(&row, w), style); + panel.put(x, y, truncate(&row, w), style); y += 1; item_idx += 1; } } - let hint_y = inner.y + inner.height - 1; - set_bottom_line( - buf, + let hint_y = inner.bottom().saturating_sub(1); + panel.bottom_line( x, hint_y, w, @@ -244,7 +243,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { What should the import do with them?", wiz.collisions.len() ); - y = draw_wrapped(buf, x, y, w, &intro, Style::default().fg(Color::Yellow)); + y = panel.wrapped(x, y, w, &intro, Style::default().fg(Color::Yellow)); y += 1; for (i, choice) in wiz.duplicate_choices.iter().enumerate() { let is_sel = i == wiz.cursor; @@ -261,30 +260,26 @@ impl<'a> Widget for ImportWizardWidget<'a> { (false, Some(_)) => Style::default().fg(Color::DarkGray), (false, None) => Style::default(), }; - y = draw_wrapped(buf, x, y, w, &label, style); + y = panel.wrapped(x, y, w, &label, style); if let Some(reason) = &choice.disabled { let dim = Style::default().fg(Color::DarkGray); - y = draw_wrapped(buf, x + 4, y, w.saturating_sub(4), reason, dim); + y = panel.wrapped(x + 4, y, w.saturating_sub(4), reason, dim); } } y += 1; - buf.set_string(x, y, "Colliding records:", Style::default().fg(Color::Gray)); - y += 1; let rows: Vec = wiz.collisions.iter().map(|c| c.describe()).collect(); - let bottom = inner.y + inner.height - 1; - draw_scrolling( - buf, + let bottom = inner.bottom().saturating_sub(1); + panel.scrolling( x, - y..bottom, + (y, "Colliding records"), + y.saturating_add(1)..bottom, w, &rows, wiz.scroll, " ", Style::default(), ); - set_bottom_line( - buf, - x, + panel.bottom_line(x, bottom, w, "\u{2191}\u{2193} choose Enter: select PgUp/PgDn: scroll \u{2190} back Esc: cancel", @@ -292,7 +287,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); } WizardStepKind::NameDuplicateCategory => { - buf.set_string( + panel.put( x, y, "Name the category that tells duplicate records apart:", @@ -300,10 +295,9 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); y += 1; let input = format!("> {}\u{2588}", self.wizard.duplicate_category); - buf.set_string(x, y, truncate(&input, w), Style::default().fg(Color::Green)); + panel.put(x, y, truncate(&input, w), Style::default().fg(Color::Green)); y += 2; - draw_wrapped( - buf, + panel.wrapped( x, y, w, @@ -312,17 +306,16 @@ impl<'a> Widget for ImportWizardWidget<'a> { grid, so totals look as they would summed.", Style::default().fg(Color::DarkGray), ); - set_bottom_line( - buf, + panel.bottom_line( x, - inner.y + inner.height - 1, + inner.bottom().saturating_sub(1), w, "Enter: next \u{2190} back Esc: cancel", Style::default().fg(Color::DarkGray), ); } WizardStepKind::DefineFormulas => { - buf.set_string( + panel.put( x, y, "Define formulas (optional):", @@ -332,7 +325,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { // Show existing formulas if self.wizard.formulas.is_empty() && !self.wizard.formula_editing { - buf.set_string( + panel.put( x, y, " (no formulas yet)", @@ -341,7 +334,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { y += 1; } for (i, formula) in self.wizard.formulas.iter().enumerate() { - if y >= inner.y + inner.height - 5 { + if y >= inner.bottom().saturating_sub(5) { break; } let is_sel = i == self.wizard.cursor && !self.wizard.formula_editing; @@ -353,14 +346,14 @@ impl<'a> Widget for ImportWizardWidget<'a> { } else { Style::default().fg(Color::Green) }; - buf.set_string(x, y, truncate(&format!(" {}", formula), w), style); + panel.put(x, y, truncate(&format!(" {}", formula), w), style); y += 1; } // Formula input area if self.wizard.formula_editing { y += 1; - buf.set_string( + panel.put( x, y, "Formula (e.g., Profit = Revenue - Cost):", @@ -368,7 +361,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { ); y += 1; let input = format!("> {}\u{2588}", self.wizard.formula_buffer); - buf.set_string(x, y, truncate(&input, w), Style::default().fg(Color::Green)); + panel.put(x, y, truncate(&input, w), Style::default().fg(Color::Green)); y += 1; } @@ -376,13 +369,13 @@ impl<'a> Widget for ImportWizardWidget<'a> { let samples = &self.wizard.sample_formulas; if !samples.is_empty() { y += 1; - buf.set_string(x, y, "Examples:", Style::default().fg(Color::DarkGray)); + panel.put(x, y, "Examples:", Style::default().fg(Color::DarkGray)); y += 1; for sample in samples { - if y >= inner.y + inner.height - 1 { + if y >= inner.bottom().saturating_sub(1) { break; } - buf.set_string( + panel.put( x, y, truncate(&format!(" {}", sample), w), @@ -392,33 +385,26 @@ impl<'a> Widget for ImportWizardWidget<'a> { } } - let hint_y = inner.y + inner.height - 1; + let hint_y = inner.bottom().saturating_sub(1); let hint = if self.wizard.formula_editing { "Enter: add Esc: cancel" } else { "n: new formula d: delete Enter: next \u{2190} back Esc: cancel" }; - set_bottom_line( - buf, - x, - hint_y, - w, - hint, - Style::default().fg(Color::DarkGray), - ); + panel.bottom_line(x, hint_y, w, hint, Style::default().fg(Color::DarkGray)); } WizardStepKind::NameModel => { - buf.set_string(x, y, "Model name:", Style::default().fg(Color::Yellow)); + panel.put(x, y, "Model name:", Style::default().fg(Color::Yellow)); y += 1; let name_str = format!("> {}\u{2588}", self.wizard.model_name); - buf.set_string( + panel.put( x, y, truncate(&name_str, w), Style::default().fg(Color::Green), ); y += 2; - buf.set_string( + panel.put( x, y, "Enter: build \u{2190} back Esc: cancel", @@ -430,32 +416,32 @@ impl<'a> Widget for ImportWizardWidget<'a> { // user is about to accept, warnings included (improvise-907). match &self.wizard.built_summary { Some(summary) => { - buf.set_string( + panel.put( x, y, truncate(summary, w), Style::default().fg(Color::Green), ); y += 2; - let bottom = inner.y + inner.height - 1; - draw_scrolling( - buf, - x, - y..bottom, - w, - &self.wizard.built_warnings, - self.wizard.scroll, - "! ", - Style::default().fg(Color::Yellow), - ); - } - None => { - buf.set_string(x, y, "Nothing to confirm.", Style::default().fg(Color::Red)) + let bottom = inner.bottom().saturating_sub(1); + let warnings = &self.wizard.built_warnings; + if !warnings.is_empty() { + panel.scrolling( + x, + (y, "Warnings"), + y.saturating_add(1)..bottom, + w, + warnings, + self.wizard.scroll, + "! ", + Style::default().fg(Color::Yellow), + ); + } } + None => panel.put(x, y, "Nothing to confirm.", Style::default().fg(Color::Red)), } - let hint_y = inner.y + inner.height - 1; - set_bottom_line( - buf, + let hint_y = inner.bottom().saturating_sub(1); + panel.bottom_line( x, hint_y, w, @@ -469,64 +455,91 @@ impl<'a> Widget for ImportWizardWidget<'a> { // upward from there rather than being cut (improvise-cgqt). if let Some(msg) = &self.wizard.message { let lines = wrap(msg, w); - let bottom = inner.y + inner.height - 1; + let bottom = inner.bottom().saturating_sub(1); let first = bottom.saturating_sub(lines.len() as u16 - 1).max(inner.y); for (row, line) in (first..=bottom).zip(&lines) { - set_bottom_line(buf, x, row, w, line, Style::default().fg(Color::Red)); + panel.bottom_line(x, row, w, line, Style::default().fg(Color::Red)); } } } } -/// Draw `text` wrapped to `w` cells from row `y`; returns the row after it. -fn draw_wrapped(buf: &mut Buffer, x: u16, y: u16, w: usize, text: &str, style: Style) -> u16 { - let lines = wrap(text, w); - for (i, line) in lines.iter().enumerate() { - buf.set_string(x, y + i as u16, line, style); - } - y + lines.len() as u16 +/// The wizard's drawing surface. Every write is clipped to the panel's rows +/// and width, so no step can reach past the buffer (review B2 on #304). +struct Panel<'b> { + buf: &'b mut Buffer, + clip: Rect, } -/// Draw `items` from `scroll` on, each wrapped whole, within `rows`. A marker -/// row says what lies above or below, so nothing is hidden without a word. -#[allow(clippy::too_many_arguments)] -fn draw_scrolling( - buf: &mut Buffer, - x: u16, - rows: std::ops::Range, - w: usize, - items: &[String], - scroll: usize, - prefix: &str, - style: Style, -) { - let dim = Style::default().fg(Color::DarkGray); - let mut y = rows.start; - if scroll > 0 && y < rows.end { - buf.set_string(x, y, format!("\u{25b2} {scroll} more above (PgUp)"), dim); - y += 1; +impl Panel<'_> { + fn put(&mut self, x: u16, y: u16, text: impl AsRef, style: Style) { + let inside = (self.clip.y..self.clip.bottom()).contains(&y) && x < self.clip.right(); + if inside { + let room = usize::from(self.clip.right() - x); + self.buf.set_stringn(x, y, text, room, style); + } } - for (i, item) in items.iter().enumerate().skip(scroll) { - let lines = wrap(&format!("{prefix}{item}"), w); - let remaining = items.len() - i; - // Keep a row for the "more below" marker unless this is the last item. - let room = rows.end.saturating_sub(y) as usize; - let needed = lines.len() + usize::from(remaining > 1); - if needed > room && i > scroll { - buf.set_string(x, y, format!("\u{25bc} {remaining} more below (PgDn)"), dim); - return; + + fn bottom_line(&mut self, x: u16, y: u16, w: usize, text: &str, style: Style) { + if (self.clip.y..self.clip.bottom()).contains(&y) { + set_bottom_line(self.buf, x, y, w, text, style); } - for line in lines.iter().take(room) { - buf.set_string(x, y, line, style); - y += 1; + } + + /// Draw `text` wrapped to `w` cells from row `y`; returns the row after it. + fn wrapped(&mut self, x: u16, y: u16, w: usize, text: &str, style: Style) -> u16 { + let lines = wrap(text, w); + for (i, line) in lines.iter().enumerate() { + self.put(x, y.saturating_add(i as u16), line, style); } + y.saturating_add(lines.len() as u16) + } + + /// Draw `items` from `scroll` on, each wrapped whole, in `rows`. Which of + /// them are shown is said on the `header` row, where no item can cover it. + #[allow(clippy::too_many_arguments)] + fn scrolling( + &mut self, + x: u16, + header: (u16, &str), + rows: std::ops::Range, + w: usize, + items: &[String], + scroll: usize, + prefix: &str, + style: Style, + ) { + let mut y = rows.start; + let mut shown = scroll; + for item in items.iter().skip(scroll) { + let lines = wrap(&format!("{prefix}{item}"), w); + let room = usize::from(rows.end.saturating_sub(y)); + if lines.len() > room && shown > scroll { + break; + } + for line in lines.iter().take(room) { + self.put(x, y, line, style); + y += 1; + } + shown += 1; + } + let (row, title) = header; + let label = match (scroll, shown) { + (0, n) if n == items.len() => format!("{title}:"), + (first, last) => format!( + "{title} {}\u{2013}{last} of {} (PgUp/PgDn):", + first + 1, + items.len() + ), + }; + self.put(x, row, label, Style::default().fg(Color::Gray)); } } /// The ONE writer for the wizard's bottom row, padding in terminal cells /// (improvise-q0vb). Both halves are argued in `context/code/tui.md`. fn set_bottom_line(buf: &mut Buffer, x: u16, y: u16, w: usize, text: &str, style: Style) { - let text = truncate(text, w); + let text = crate::ui::text::truncate(text, w); let pad = " ".repeat(w.saturating_sub(UnicodeWidthStr::width(text.as_str()))); buf.set_string(x, y, format!("{text}{pad}"), style); } @@ -610,11 +623,19 @@ mod tests { buf.content() .chunks(w) .map(|row| { - row.iter() - .map(|c| c.symbol()) - .collect::() - .trim_end() - .to_string() + // A double-width character's second cell is padding, not text. + let mut skip = 0; + let mut text = String::new(); + for cell in row { + match skip { + 0 => { + text.push_str(cell.symbol()); + skip = UnicodeWidthStr::width(cell.symbol()).saturating_sub(1); + } + _ => skip -= 1, + } + } + text.trim_end().to_string() }) .collect::>() .join("\n") @@ -859,12 +880,39 @@ mod tests { !first.contains("Region=Region-13"), "precondition: it overflows\n{first}" ); - assert!(first.contains("more below"), "{first}"); + assert!(first.contains("Colliding records 1\u{2013}"), "{first}"); + assert!(first.contains(" of 14 (PgUp/PgDn):"), "{first}"); w.scroll(100); let last = render(&w); assert!(last.contains("Region=Region-13"), "{last}"); - assert!(last.contains("more above"), "{last}"); + assert!( + last.contains("Colliding records 14\u{2013}14 of 14"), + "{last}" + ); + } + + /// Codex P2 on PR #304: an item that fills the list exactly left the "more + /// below" marker on the hint row, drawn over. `context/test-evidence/tui.md`. + #[test] + fn a_first_item_that_fills_the_list_still_says_more_follow() { + let area = Rect::new(0, 0, 10, 6); + let mut buf = Buffer::empty(area); + let mut panel = Panel { + buf: &mut buf, + clip: area, + }; + let items = ["aaaa bbbb cccc".to_string(), "dddd".to_string()]; + panel.scrolling(0, (0, "L"), 1..4, 4, &items, 0, "", Style::default()); + set_bottom_line(&mut buf, 0, 4, 10, "hint", Style::default()); + let drawn = buf_text(&buf); + let rows: Vec<&str> = drawn.lines().collect(); + assert_eq!( + rows[1..4], + ["aaaa", "bbbb", "cccc"], + "the first item whole:\n{drawn}" + ); + assert!(rows[0].starts_with("L 1\u{2013}1 of 2"), "{drawn}"); } #[test] @@ -892,6 +940,175 @@ mod tests { assert_drawn_whole(&drawn, &reason); } + fn render_at(w: &ImportWizard, width: u16, height: u16) -> String { + let area = Rect::new(0, 0, width, height); + let mut buf = Buffer::empty(area); + let cache = project::wizard_cache(w); + ImportWizardWidget::new(&cache).render(area, &mut buf); + buf_text(&buf) + } + + /// The reported import: twelve categories and a collision. + fn wizard_at_the_cap() -> ImportWizard { + let recs: Vec = (0..3) + .chain([0]) + .map(|r| { + let mut m = serde_json::Map::new(); + for i in 0..12 { + m.insert(format!("c{i:02}"), json!(format!("v{r}"))); + } + m.insert("amount".into(), json!(1.0)); + serde_json::Value::Object(m) + }) + .collect(); + let mut w = ImportWizard::new(serde_json::Value::Array(recs)); + w.advance(); + assert!(matches!(w.step, WizardStep::ResolveDuplicates), "setup"); + w + } + + /// Review B1 on PR #304: the cap reason carries a `←`, three bytes wide, + /// and a byte-measured bottom row cut it. `context/test-evidence/tui.md`. + #[test] + fn a_multibyte_message_is_drawn_whole_at_every_width() { + let mut w = wizard_at_the_cap(); + w.move_cursor(1); + w.choose_duplicate(); + let reason = w + .message + .clone() + .expect("setup: choosing Separate at the cap"); + assert!(reason.contains('\u{2190}'), "setup: {reason}"); + let messages = [ + reason, + "Région 日本語 🎉 naïve café — ← → Skip, Measure, Sum".repeat(3), + ]; + for msg in messages { + w.message = Some(msg.clone()); + for width in 30..=100u16 { + let drawn = render_at(&w, width, 30); + let flat: String = drawn.lines().collect(); + assert!( + !flat.contains('\u{2026}'), + "width {width} truncated:\n{drawn}" + ); + assert_drawn_whole(&drawn, &msg); + } + } + } + + /// Every step the wizard can reach, as the TUI would draw it: each a fresh + /// wizard walked by `advance`, `choose` or `build` in turn. + fn every_step() -> Vec { + #[derive(Clone, Copy)] + enum Op { + Advance, + Separate, + Sum, + Build, + } + let colliding = || { + ImportWizard::new(json!([ + {"Region": "East", "Revenue": 1.0}, {"Region": "East", "Revenue": 2.0} + ])) + }; + let walk = |mut w: ImportWizard, ops: &[Op]| { + for op in ops { + match op { + Op::Advance => w.advance(), + Op::Separate => { + w.move_cursor(1); + w.choose_duplicate(); + } + Op::Sum => w.choose_duplicate(), + Op::Build => w.build(), + } + } + w + }; + use Op::*; + vec![ + ImportWizard::new(json!([])), + ImportWizard::new(json!({ + "sales": [{"Region": "East", "Revenue": 1}], + "costs": [{"Region": "West", "Cost": 2}], + })), + wizard(), + walk(wizard(), &[Advance]), + walk(wizard(), &[Advance, Advance]), + walk(wizard(), &[Advance, Advance, Advance]), + walk(wizard(), &[Advance, Advance, Advance, Build]), + walk(colliding(), &[Advance]), + walk(colliding(), &[Advance, Separate]), + walk(colliding(), &[Advance, Sum, Advance, Build]), + wizard_at_the_cap(), + walk(wizard_at_the_cap(), &[Separate]), + { + let mut w = walk(wizard(), &[Advance, Advance]); + for formula in ["Profit = Revenue - Cost", "Margin = Profit / Revenue"] { + w.start_formula_edit(); + formula.chars().for_each(|c| w.push_formula_char(c)); + w.confirm_formula(); + } + w.start_formula_edit(); + w.push_formula_char('M'); + w + }, + ] + } + + /// Review B2 on PR #304: the duplicates step wrote past the panel and + /// ratatui panicked. Every step, every small size. `context/test-evidence/tui.md`. + #[test] + fn no_step_panics_on_a_small_terminal() { + let steps = every_step(); + let kinds: Vec = steps.iter().map(|w| format!("{:?}", w.step)).collect(); + for kind in [ + "Preview", + "SelectArrayPath", + "ReviewProposals", + "ConfigureDates", + "ResolveDuplicates", + "NameDuplicateCategory", + "DefineFormulas", + "NameModel", + "Done", + ] { + assert!( + kinds.iter().any(|k| k == kind), + "setup: no {kind} in {kinds:?}" + ); + } + let sweep = |cache: &crate::protocol::WizardCache, kind: &str| { + let mut panics = vec![]; + for width in 20..=100u16 { + for height in 3..=20u16 { + let drawn = std::panic::catch_unwind(|| { + let area = Rect::new(0, 0, width, height); + ImportWizardWidget::new(cache).render(area, &mut Buffer::empty(area)); + }); + if drawn.is_err() { + panics.push(format!("{kind} {width}x{height}")); + } + } + } + panics + }; + let caches: Vec<_> = steps.iter().map(project::wizard_cache).collect(); + let panics: Vec = std::thread::scope(|scope| { + let running: Vec<_> = caches + .iter() + .zip(&kinds) + .map(|(cache, kind)| scope.spawn(move || sweep(cache, kind))) + .collect(); + running + .into_iter() + .flat_map(|t| t.join().expect("the sweep itself does not panic")) + .collect() + }); + assert!(panics.is_empty(), "{} panics: {:?}", panics.len(), panics); + } + const PREVIEW_STEP: &str = concat!( "┌ Import Wizard — Preview ─────────────────────────────────────────────────────┐\n", "│Array of 0 records. Sample keys: │\n", diff --git a/src/ui/text.rs b/src/ui/text.rs index 8debcbb4..2f82ebab 100644 --- a/src/ui/text.rs +++ b/src/ui/text.rs @@ -104,6 +104,17 @@ mod tests { } } + /// Review B1 on PR #304: bytes are never fewer than cells, so a byte + /// measure only wraps early. These fit in cells and not in bytes. + #[test] + fn wrapping_measures_cells_not_bytes() { + let s = "\u{2190}\u{2190}\u{2190} \u{e9}\u{e9}"; + assert_eq!(s.len(), 14, "setup: fourteen bytes"); + assert_eq!(wrap(s, 6), [s], "six cells fit on one line of six"); + let cjk = "\u{65e5}\u{672c} ab"; + assert_eq!(wrap(cjk, 7), [cjk], "seven cells, ten bytes"); + } + /// An indented option or list row keeps its indent, on every line. #[test] fn wrapping_keeps_a_leading_indent_on_every_line() {