diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index d0da7b1..a4f680f 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -53,6 +53,53 @@ pub fn parse_hunk_header(header: &str) -> Option<(usize, usize)> { Some((old_start, new_start)) } +/// Build the line-number gutter string for one diff line. +/// +/// Both columns are right-aligned to `w` and separated by a single space, so +/// the old and new numbers never collide regardless of magnitude. A trailing +/// space separates the gutter from the line content. Additions blank the old +/// column; deletions blank the new column; hunk headers are blank across the +/// full `2 * w + 2` gutter width. +fn format_gutter(kind: &DiffLineKind, old_line: usize, new_line: usize, w: usize) -> String { + match kind { + DiffLineKind::Addition => format!("{:>w$} {:>w$} ", "", new_line, w = w), + DiffLineKind::Deletion => format!("{:>w$} {:>w$} ", old_line, "", w = w), + DiffLineKind::Context => format!("{:>w$} {:>w$} ", old_line, new_line, w = w), + DiffLineKind::HunkHeader => " ".repeat(2 * w + 2), + } +} + +/// Per-column digit width for a file's diff gutter: the digit count of the +/// largest line number that will be displayed across both the old and new +/// columns. Floored at 1 so empty/degenerate diffs still render a stable +/// gutter. Sizing per-file keeps narrow files compact while guaranteeing +/// 5+ digit line numbers never collide. +fn gutter_width(diff: &FileDiff) -> usize { + let mut max_line = 0usize; + for hunk in &diff.hunks { + let (mut old_line, mut new_line) = parse_hunk_header(&hunk.header).unwrap_or((1, 1)); + for diff_line in &hunk.lines { + match diff_line.kind { + DiffLineKind::Addition => { + max_line = max_line.max(new_line); + new_line += 1; + } + DiffLineKind::Deletion => { + max_line = max_line.max(old_line); + old_line += 1; + } + DiffLineKind::Context => { + max_line = max_line.max(old_line).max(new_line); + old_line += 1; + new_line += 1; + } + DiffLineKind::HunkHeader => {} + } + } + } + max_line.to_string().len().max(1) +} + /// Render the diff overlay onto `frame`. /// /// The overlay is drawn as a centred, bordered panel on top of whatever is @@ -85,13 +132,19 @@ pub fn render_diff_overlay( let inner = block.inner(overlay_rect); frame.render_widget(block, overlay_rect); + // Size the line-number gutter to this file's largest line number. + let w = gutter_width(diff); + // Build diff lines with line numbers and colours let mut lines: Vec = Vec::new(); for hunk in &diff.hunks { // Hunk header line lines.push(Line::from(vec![ - Span::styled(" ", Style::default().fg(colors::DIFF_LINE_NUMBER)), + Span::styled( + format_gutter(&DiffLineKind::HunkHeader, 0, 0, w), + Style::default().fg(colors::DIFF_LINE_NUMBER), + ), Span::styled( &hunk.header, Style::default() @@ -103,27 +156,22 @@ pub fn render_diff_overlay( let (mut old_line, mut new_line) = parse_hunk_header(&hunk.header).unwrap_or((1, 1)); for diff_line in &hunk.lines { - let (gutter, style) = match diff_line.kind { + let gutter = format_gutter(&diff_line.kind, old_line, new_line, w); + let style = match diff_line.kind { DiffLineKind::Addition => { - let g = format!("{:>4}{:>4} ", " ", new_line); new_line += 1; - (g, Style::default().fg(colors::DIFF_ADD_FG)) + Style::default().fg(colors::DIFF_ADD_FG) } DiffLineKind::Deletion => { - let g = format!("{:>4}{:>4} ", old_line, " "); old_line += 1; - (g, Style::default().fg(colors::DIFF_DEL_FG)) + Style::default().fg(colors::DIFF_DEL_FG) } DiffLineKind::Context => { - let g = format!("{:>4}{:>4} ", old_line, new_line); old_line += 1; new_line += 1; - (g, Style::default().fg(colors::DIFF_CONTEXT)) + Style::default().fg(colors::DIFF_CONTEXT) } - DiffLineKind::HunkHeader => ( - " ".to_string(), - Style::default().fg(colors::DIFF_HUNK_HEADER), - ), + DiffLineKind::HunkHeader => Style::default().fg(colors::DIFF_HUNK_HEADER), }; let prefix = match diff_line.kind { @@ -202,4 +250,99 @@ mod tests { let area = Rect::new(0, 0, 100, 100); assert_eq!(inner_height(area), 83); } + + #[test] + fn test_format_gutter_context_separates_columns() { + // width 2, both columns present + let g = format_gutter(&DiffLineKind::Context, 12, 34, 2); + assert_eq!(g, "12 34 "); + } + + #[test] + fn test_format_gutter_addition_blanks_old_column() { + let g = format_gutter(&DiffLineKind::Addition, 12, 34, 2); + assert_eq!(g, " 34 "); + } + + #[test] + fn test_format_gutter_deletion_blanks_new_column() { + let g = format_gutter(&DiffLineKind::Deletion, 12, 34, 2); + assert_eq!(g, "12 "); + } + + #[test] + fn test_format_gutter_five_digits_stay_separated() { + // The bug: 5-digit numbers used to collide. With width 5 they must + // be separated by at least one space. + let g = format_gutter(&DiffLineKind::Context, 12345, 12346, 5); + assert_eq!(g, "12345 12346 "); + assert!(g.contains("12345 12346")); + } + + #[test] + fn test_format_gutter_hunk_header_is_blank_of_matching_width() { + // Blank gutter must span 2*w + 2 columns to align with number rows. + let g = format_gutter(&DiffLineKind::HunkHeader, 0, 0, 4); + assert_eq!(g, " ".repeat(4 * 2 + 2)); + assert_eq!(g.len(), 10); + } + + fn diff_with(hunks: Vec<(&str, Vec<(DiffLineKind, &str)>)>) -> FileDiff { + use crate::git::{DiffHunk, DiffLine}; + FileDiff { + hunks: hunks + .into_iter() + .map(|(header, lines)| DiffHunk { + header: header.to_string(), + lines: lines + .into_iter() + .map(|(kind, content)| DiffLine { + kind, + content: content.to_string(), + }) + .collect(), + }) + .collect(), + } + } + + #[test] + fn test_gutter_width_empty_diff_is_one() { + let diff = FileDiff::default(); + assert_eq!(gutter_width(&diff), 1); + } + + #[test] + fn test_gutter_width_small_numbers() { + // Lines run 1..=3 on each side -> 1 digit. + let diff = diff_with(vec![( + "@@ -1,3 +1,3 @@", + vec![ + (DiffLineKind::Context, "a"), + (DiffLineKind::Context, "b"), + (DiffLineKind::Context, "c"), + ], + )]); + assert_eq!(gutter_width(&diff), 1); + } + + #[test] + fn test_gutter_width_tracks_largest_across_hunks() { + // Second hunk starts at 9998 with 3 context lines -> reaches 10000 (5 digits). + let diff = diff_with(vec![ + ( + "@@ -1,2 +1,2 @@", + vec![(DiffLineKind::Context, "a"), (DiffLineKind::Context, "b")], + ), + ( + "@@ -9998,3 +9998,3 @@", + vec![ + (DiffLineKind::Context, "x"), + (DiffLineKind::Context, "y"), + (DiffLineKind::Context, "z"), + ], + ), + ]); + assert_eq!(gutter_width(&diff), 5); + } } diff --git a/src/ui/help_overlay.rs b/src/ui/help_overlay.rs index 00853e2..5fbae19 100644 --- a/src/ui/help_overlay.rs +++ b/src/ui/help_overlay.rs @@ -1,24 +1,48 @@ //! Full-screen help overlay listing built-in keybindings. //! -//! Shown when the user presses `?`. Displays a centred panel at ~85% of -//! the terminal area with a two-column list: key on the left, description -//! on the right. Dismissible with `?`, `Esc`, `q`, or `Space`. +//! Shown when the user presses `?`. Displays a centred panel sized to fit its +//! content, with a two-column list: key on the left, description on the right. +//! Dismissible with `?`, `Esc`, `q`, or `Space`. use ratatui::{ layout::Rect, style::{Modifier, Style}, text::{Line, Span}, - widgets::{Block, BorderType, Borders, Clear, Paragraph, Wrap}, + widgets::{Block, BorderType, Borders, Clear, Padding, Paragraph, Wrap}, Frame, }; use crate::ui::colors; +use crate::ui::fit; -/// Return a centred `Rect` that occupies `percent_x`% width and `percent_y`% -/// height of `area`. -fn centered_rect(percent_x: u16, percent_y: u16, area: Rect) -> Rect { - let w = area.width * percent_x / 100; - let h = area.height * percent_y / 100; +/// Horizontal padding (columns) inside the help overlay border, each side. +const HELP_H_PAD: u16 = 2; + +/// Size the help overlay to its content: just large enough to show every help +/// line, plus borders and `HELP_H_PAD` columns of breathing room on each side. +/// Centred in `area` and never larger than `area` itself. Width is also floored +/// to the title width so the " Keybindings " title never clips. +fn help_overlay_rect(area: Rect) -> Rect { + let lines = build_help_lines(); + let content_h = lines.len() as u16; + let title_w = fit::display_width(" Keybindings ") as u16; + let content_w = lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|s| fit::display_width(s.content.as_ref())) + .sum::() as u16 + }) + .max() + .unwrap_or(0) + .max(title_w); + + let desired_w = content_w + 2 * HELP_H_PAD + 2; // both-side padding + borders + let desired_h = content_h + 2; // top + bottom borders + + let w = desired_w.min(area.width); + let h = desired_h.min(area.height); let x = area.x + (area.width.saturating_sub(w)) / 2; let y = area.y + (area.height.saturating_sub(h)) / 2; Rect::new(x, y, w, h) @@ -77,13 +101,25 @@ pub fn build_help_lines() -> Vec> { let mut lines: Vec> = Vec::new(); lines.push(Line::from("")); + // Width of the widest key across every section, plus a 2-column gap, so + // all descriptions line up regardless of key length. Measured by terminal + // display width (not char count) to stay correct with arrow glyphs. + let key_w = entries + .iter() + .flat_map(|(_, rows)| rows.iter()) + .map(|(key, _)| fit::display_width(key)) + .max() + .unwrap_or(0) + + 2; + for (section, rows) in entries { lines.push(Line::from(vec![ Span::raw(" "), Span::styled(section.to_string(), header_style), ])); for (key, desc) in *rows { - let key_padded = format!("{:<14}", key); + let pad = key_w.saturating_sub(fit::display_width(key)); + let key_padded = format!("{key}{}", " ".repeat(pad)); lines.push(Line::from(vec![ Span::raw(" "), Span::styled(key_padded, key_style), @@ -99,7 +135,7 @@ pub fn build_help_lines() -> Vec> { /// Render the help overlay onto `frame`. pub fn render_help_overlay(frame: &mut Frame) { let area = frame.area(); - let overlay_rect = centered_rect(85, 85, area); + let overlay_rect = help_overlay_rect(area); frame.render_widget(Clear, overlay_rect); @@ -108,7 +144,8 @@ pub fn render_help_overlay(frame: &mut Frame) { .border_type(BorderType::Rounded) .border_style(Style::default().fg(colors::BORDER_FOCUSED)) .title(" Keybindings ") - .title_style(Style::default().fg(colors::HEADER_TEXT)); + .title_style(Style::default().fg(colors::HEADER_TEXT)) + .padding(Padding::horizontal(HELP_H_PAD)); let inner = block.inner(overlay_rect); frame.render_widget(block, overlay_rect); @@ -121,6 +158,7 @@ pub fn render_help_overlay(frame: &mut Frame) { #[cfg(test)] mod tests { use super::*; + use crate::ui::fit::display_width; fn line_text(line: &Line<'_>) -> String { line.spans @@ -133,6 +171,43 @@ mod tests { lines.iter().map(line_text).collect::>().join("\n") } + /// Display width of a row's indent + padded-key (i.e. the column where the + /// description begins). Only meaningful for key/desc rows (3 spans). + fn desc_column(line: &Line<'_>) -> Option { + if line.spans.len() != 3 { + return None; + } + let prefix: String = line.spans[..2].iter().map(|s| s.content.as_ref()).collect(); + Some(display_width(&prefix)) + } + + #[test] + fn test_help_descriptions_share_one_column() { + let lines = build_help_lines(); + let columns: Vec = lines.iter().filter_map(desc_column).collect(); + assert!(columns.len() > 5, "expected several key/desc rows"); + let first = columns[0]; + assert!( + columns.iter().all(|&c| c == first), + "all descriptions must start at the same column, got {columns:?}" + ); + } + + #[test] + fn test_help_longest_key_keeps_gap() { + // "gg / G / Ctrl+u/d/f/b" is the widest key; it must still leave a >=2 + // space gap before its description rather than overflowing. + let lines = build_help_lines(); + let row = lines + .iter() + .find(|l| line_text(l).contains("gg / G / Ctrl+u/d/f/b")) + .expect("missing longest-key row"); + assert_eq!(row.spans.len(), 3); + let key_span = row.spans[1].content.as_ref(); + let gap = display_width(key_span) - display_width("gg / G / Ctrl+u/d/f/b"); + assert!(gap >= 2, "expected >=2 space gap, got {gap}"); + } + #[test] fn test_help_lines_contain_all_sections() { let lines = build_help_lines(); @@ -174,4 +249,58 @@ mod tests { text.starts_with(" m") && text.contains("Cycle view mode") })); } + + #[test] + fn test_help_rect_fits_content_not_full_box() { + let area = Rect::new(0, 0, 200, 100); + let rect = help_overlay_rect(area); + let lines = build_help_lines(); + // Height is exactly the content plus top+bottom borders. + assert_eq!(rect.height, lines.len() as u16 + 2); + // Much smaller than the old fixed 85% box in both dimensions. + assert!(rect.height < area.height * 85 / 100); + assert!(rect.width < area.width); + } + + #[test] + fn test_help_rect_centered_in_area() { + let area = Rect::new(0, 0, 200, 100); + let rect = help_overlay_rect(area); + assert_eq!(rect.x, area.x + (area.width - rect.width) / 2); + assert_eq!(rect.y, area.y + (area.height - rect.height) / 2); + } + + #[test] + fn test_help_rect_clamped_to_small_area() { + let area = Rect::new(0, 0, 20, 8); + let rect = help_overlay_rect(area); + assert!( + rect.width <= area.width, + "width {} exceeds area", + rect.width + ); + assert!( + rect.height <= area.height, + "height {} exceeds area", + rect.height + ); + } + + #[test] + fn test_help_rect_width_accounts_for_content_and_padding() { + let area = Rect::new(0, 0, 200, 100); + let rect = help_overlay_rect(area); + let content_w = build_help_lines() + .iter() + .map(|l| { + l.spans + .iter() + .map(|s| fit::display_width(s.content.as_ref())) + .sum::() as u16 + }) + .max() + .unwrap_or(0); + // borders (2) + horizontal padding on both sides + the widest line. + assert_eq!(rect.width, content_w + 2 * HELP_H_PAD + 2); + } }