From 8ac0ec6d382d3b6e0ba32b24d59c7a33b52a0c8a Mon Sep 17 00:00:00 2001 From: Paul Recker Date: Wed, 1 Jul 2026 16:22:48 +0200 Subject: [PATCH] fix(review): show diff colors on non-truecolor terminals and fix tab indentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diff view emitted 24-bit truecolor for every syntax color and diff background. Terminals that drop truecolor escapes — tmux and macOS Terminal.app unless specially configured — rendered the diff as plain, uncolored text, which reads as "missing syntax highlighting". Detect the terminal color depth and downsample RGB to the 256-color palette when truecolor isn't reliably supported (new src/ui/review/color.rs). Tabs collapsed to a single cell, breaking indentation. Expand them to tab-stop-aligned spaces in the diff view. Rework the in-TUI scope switcher into a clearer menu (uncommitted / branch / branch+worktree) and add a commit picker: review a single commit, or mark two commits to review the range between them. New [review] config: truecolor ("auto" | "always" | "never") and tab_width. Docs updated in docs/review.md. --- Cargo.lock | 2 +- docs/review.md | 27 ++++ src/commands/review.rs | 7 ++ src/config.rs | 25 ++++ src/ui/review/color.rs | 220 ++++++++++++++++++++++++++++++++ src/ui/review/diff_view.rs | 119 +++++++++++++++++- src/ui/review/highlight.rs | 46 ++++--- src/ui/review/mod.rs | 252 ++++++++++++++++++++++++++++++++++--- 8 files changed, 659 insertions(+), 39 deletions(-) create mode 100644 src/ui/review/color.rs diff --git a/Cargo.lock b/Cargo.lock index e3823f4..a220be3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -691,7 +691,7 @@ dependencies = [ [[package]] name = "gx" -version = "0.4.0" +version = "0.5.0" dependencies = [ "clap", "clap_complete", diff --git a/docs/review.md b/docs/review.md index bd31ed4..612f56a 100644 --- a/docs/review.md +++ b/docs/review.md @@ -32,6 +32,22 @@ gx review A..B # an explicit commit range The base defaults to `origin`'s default branch (falling back to `origin/main` then `origin/master`). +### Switching scope inside the TUI + +You don't have to pick the range up front — press `s` while reviewing to change +what you're looking at, without leaving the UI: + +- **Uncommitted changes** — the working tree vs `HEAD`. +- **All changes on this branch** — `base…HEAD` (the default). +- **Branch changes + working tree** — committed branch work plus anything not yet + committed. +- **Pick a commit or range…** — opens a commit picker. `Enter` reviews the + highlighted commit on its own; press `space` to mark one end of a range, move + to the other commit, then `Enter` to review everything between them. + +Each scope keeps its own comments (they're keyed separately), so switching back +and forth never mixes them up. + ## Keys | Key | Action | @@ -49,6 +65,7 @@ then `origin/master`). | `o` | list orphaned comments (see Persistence) | | `F` | **finish**: copy the review to the clipboard | | `X` (twice) | discard the saved review | +| `s` | change scope: uncommitted · branch · +worktree · pick a commit/range | | `v` | toggle split / unified | | `b` | toggle the sidebar | | `?` | help overlay | @@ -84,6 +101,8 @@ Under `[review]` in the gx config (`gx setup` shows the path): | --- | --- | --- | | `appearance` | `auto` | `auto` detects the terminal background (light/dark) and picks a matching theme + diff palette; `light` / `dark` force it | | `theme` | *(auto)* | syntect theme name; empty picks `InspiredGitHub` (light) or `base16-ocean.dark` (dark) from `appearance` | +| `truecolor` | `auto` | `auto` uses 24-bit color only when the terminal reliably supports it, otherwise 256-color; `always` / `never` force it | +| `tab_width` | `4` | columns a tab expands to in the diff | | `side_by_side_min_width` | `120` | below this terminal width, use the unified view | | `default_mode` | `branch` | default range mode | @@ -91,3 +110,11 @@ The diff adapts to your terminal: on a light background it uses a light syntax theme with pale add/remove tints; on a dark background, a dark theme with the darker tints. If auto-detection guesses wrong (some terminals don't answer the query), set `appearance` explicitly. + +**No colors / plain text?** syntect themes are 24-bit RGB, but many terminals — +Apple's Terminal.app, and tmux/screen unless configured for `RGB`/`Tc` — silently +drop truecolor escapes, which shows up as a diff with no syntax highlighting and +no add/remove backgrounds. `truecolor = "auto"` (the default) sidesteps this by +downsampling to the widely-supported 256-color palette in those environments. If +your terminal *does* handle truecolor through a multiplexer, set +`truecolor = "always"` for full-fidelity colors. diff --git a/src/commands/review.rs b/src/commands/review.rs index 88abdc7..de5a5e3 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -29,11 +29,18 @@ pub fn run(target: Option, base: Option) -> Result<()> { cfg.review.theme.clone() }; + // Resolve the terminal color depth: themes are 24-bit RGB, but many + // terminals/multiplexers drop truecolor escapes (which reads as "no syntax + // highlighting"), so we downsample to 256-color unless truecolor is safe. + let color_depth = ui::review::color::detect(&cfg.review.truecolor); + ui::review::run( range, files, &theme, cfg.review.side_by_side_min_width, appearance, + color_depth, + cfg.review.tab_width, ) } diff --git a/src/config.rs b/src/config.rs index 18426e0..eee1bac 100644 --- a/src/config.rs +++ b/src/config.rs @@ -117,6 +117,19 @@ pub struct ReviewConfig { #[serde(default)] pub theme: String, + /// Color depth for the diff view: "auto" downsamples the theme's 24-bit + /// colors to the 256-color palette unless the terminal reliably supports + /// truecolor (many terminals and multiplexers silently drop 24-bit escapes, + /// which shows up as *no* syntax colors); "always" forces truecolor, + /// "never" forces 256-color. + #[serde(default = "default_truecolor")] + pub truecolor: String, + + /// How many columns a tab expands to in the diff view. Tabs are rendered as + /// spaces so indentation lines up (a raw tab collapses to one cell). + #[serde(default = "default_tab_width")] + pub tab_width: u16, + /// Minimum terminal width (columns) for side-by-side; below this the diff /// falls back to a unified single-column view. #[serde(default = "default_side_by_side_min_width")] @@ -132,6 +145,14 @@ fn default_appearance() -> String { "auto".to_string() } +fn default_truecolor() -> String { + "auto".to_string() +} + +fn default_tab_width() -> u16 { + 4 +} + fn default_side_by_side_min_width() -> u16 { 120 } @@ -145,6 +166,8 @@ impl Default for ReviewConfig { ReviewConfig { appearance: default_appearance(), theme: String::new(), + truecolor: default_truecolor(), + tab_width: default_tab_width(), side_by_side_min_width: default_side_by_side_min_width(), default_mode: default_review_mode(), } @@ -310,6 +333,8 @@ mod tests { let review = ReviewConfig::default(); assert_eq!(review.appearance, "auto"); assert_eq!(review.theme, ""); // empty = auto-pick from appearance + assert_eq!(review.truecolor, "auto"); + assert_eq!(review.tab_width, 4); assert_eq!(review.side_by_side_min_width, 120); assert_eq!(review.default_mode, "branch"); assert_eq!(Config::default().review.appearance, "auto"); diff --git a/src/ui/review/color.rs b/src/ui/review/color.rs new file mode 100644 index 0000000..26eff28 --- /dev/null +++ b/src/ui/review/color.rs @@ -0,0 +1,220 @@ +//! Terminal color-depth handling for the diff view. +//! +//! syntect themes and the diff palette are authored in 24-bit RGB. Many +//! terminals — Apple's Terminal.app, and tmux/screen unless explicitly +//! configured for `RGB`/`Tc` — silently *drop* 24-bit (`38;2;…`) escapes, which +//! shows up as a diff with no syntax colors and no add/remove backgrounds (the +//! "missing syntax highlighting" symptom). To stay legible everywhere, we +//! downsample every RGB color to the 256-color palette unless we are confident +//! the terminal handles truecolor. + +use ratatui::style::Color; + +/// The color resolution the diff view will actually emit. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum ColorDepth { + /// 24-bit RGB, emitted verbatim. + TrueColor, + /// The xterm 256-color palette; RGB colors are mapped to the nearest index. + Ansi256, +} + +/// Resolve the configured preference into a concrete depth. +/// +/// `"always"`/`"truecolor"`/`"24bit"` force truecolor; `"never"`/`"256"` force +/// 256-color; anything else auto-detects. +pub fn detect(pref: &str) -> ColorDepth { + match pref { + "always" | "truecolor" | "24bit" => ColorDepth::TrueColor, + "never" | "256" | "ansi256" => ColorDepth::Ansi256, + _ => detect_auto(&Env::from_process()), + } +} + +/// The environment inputs auto-detection reads (extracted so it is testable). +struct Env { + term: String, + colorterm: String, + term_program: String, + in_multiplexer: bool, +} + +impl Env { + fn from_process() -> Self { + let get = |k: &str| std::env::var(k).unwrap_or_default(); + Env { + term: get("TERM"), + colorterm: get("COLORTERM"), + term_program: get("TERM_PROGRAM"), + in_multiplexer: std::env::var_os("TMUX").is_some() + || std::env::var_os("STY").is_some(), + } + } +} + +fn detect_auto(env: &Env) -> ColorDepth { + // Terminals that advertise direct color in TERM (e.g. `xterm-direct`) always + // mean it. + if env.term.contains("direct") { + return ColorDepth::TrueColor; + } + + let advertises = matches!(env.colorterm.as_str(), "truecolor" | "24bit"); + + // Inside tmux/screen, COLORTERM is usually inherited from the outer terminal + // but the multiplexer only forwards 24-bit when specially configured — which + // we cannot detect at runtime. Be conservative: fall back to 256-color + // (which every terminal renders) and let `truecolor = "always"` opt back in. + let in_mux = env.in_multiplexer + || env.term.starts_with("screen") + || env.term.starts_with("tmux") + || matches!(env.term_program.as_str(), "tmux" | "screen"); + + if advertises && !in_mux { + ColorDepth::TrueColor + } else { + ColorDepth::Ansi256 + } +} + +/// Adapt a color to the target depth: RGB colors are mapped to the nearest +/// 256-color index under [`ColorDepth::Ansi256`]; everything else (named ANSI +/// colors, `Reset`, already-indexed) passes through unchanged. +pub fn adapt(color: Color, depth: ColorDepth) -> Color { + match (depth, color) { + (ColorDepth::Ansi256, Color::Rgb(r, g, b)) => Color::Indexed(rgb_to_ansi256(r, g, b)), + _ => color, + } +} + +/// The six RGB levels of the xterm color cube. +const CUBE_STEPS: [u8; 6] = [0x00, 0x5f, 0x87, 0xaf, 0xd7, 0xff]; + +/// Map one channel to its nearest cube index (0..=5). Matches the thresholds +/// tmux and other terminals use. +fn channel_to_cube(v: u8) -> usize { + if v < 48 { + 0 + } else if v < 114 { + 1 + } else { + ((v as usize - 35) / 40).min(5) + } +} + +fn dist(a: (u8, u8, u8), b: (u8, u8, u8)) -> u32 { + let d = |x: u8, y: u8| { + let d = x as i32 - y as i32; + (d * d) as u32 + }; + d(a.0, b.0) + d(a.1, b.1) + d(a.2, b.2) +} + +/// Nearest xterm 256-color index for an RGB triple, choosing between the 6×6×6 +/// color cube (16..=231) and the grayscale ramp (232..=255) by which is closer. +pub fn rgb_to_ansi256(r: u8, g: u8, b: u8) -> u8 { + let (qr, qg, qb) = ( + channel_to_cube(r), + channel_to_cube(g), + channel_to_cube(b), + ); + let cube = (CUBE_STEPS[qr], CUBE_STEPS[qg], CUBE_STEPS[qb]); + let cube_idx = 16 + 36 * qr + 6 * qg + qb; + + // Nearest point on the 24-step grayscale ramp (levels 8, 18, … 238). + let avg = (r as u32 + g as u32 + b as u32) / 3; + let gray_i = if avg <= 8 { + 0 + } else { + (((avg - 8) + 5) / 10).min(23) + } as usize; + let gray_level = (8 + 10 * gray_i) as u8; + let gray = (gray_level, gray_level, gray_level); + let gray_idx = 232 + gray_i; + + let target = (r, g, b); + if dist(gray, target) < dist(cube, target) { + gray_idx as u8 + } else { + cube_idx as u8 + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn env(term: &str, colorterm: &str, term_program: &str, mux: bool) -> Env { + Env { + term: term.into(), + colorterm: colorterm.into(), + term_program: term_program.into(), + in_multiplexer: mux, + } + } + + #[test] + fn tmux_with_inherited_truecolor_falls_back_to_256() { + // The reported case: COLORTERM=truecolor but running under tmux, which + // drops 24-bit unless configured. Must not claim truecolor. + let e = env("tmux-256color", "truecolor", "tmux", true); + assert_eq!(detect_auto(&e), ColorDepth::Ansi256); + } + + #[test] + fn plain_iterm_truecolor_is_truecolor() { + let e = env("xterm-256color", "truecolor", "iTerm.app", false); + assert_eq!(detect_auto(&e), ColorDepth::TrueColor); + } + + #[test] + fn terminal_app_without_colorterm_is_256() { + // macOS Terminal.app: 256-color capable, no 24-bit, no COLORTERM. + let e = env("xterm-256color", "", "Apple_Terminal", false); + assert_eq!(detect_auto(&e), ColorDepth::Ansi256); + } + + #[test] + fn direct_term_is_truecolor_even_in_mux() { + let e = env("xterm-direct", "", "", true); + assert_eq!(detect_auto(&e), ColorDepth::TrueColor); + } + + #[test] + fn explicit_preference_overrides_detection() { + assert_eq!(detect("always"), ColorDepth::TrueColor); + assert_eq!(detect("never"), ColorDepth::Ansi256); + } + + #[test] + fn adapt_only_touches_rgb_under_256() { + assert_eq!( + adapt(Color::Rgb(255, 0, 0), ColorDepth::Ansi256), + Color::Indexed(196) + ); + // Truecolor passes RGB through. + assert_eq!( + adapt(Color::Rgb(255, 0, 0), ColorDepth::TrueColor), + Color::Rgb(255, 0, 0) + ); + // Named / reset colors are never rewritten. + assert_eq!(adapt(Color::Cyan, ColorDepth::Ansi256), Color::Cyan); + assert_eq!(adapt(Color::Reset, ColorDepth::Ansi256), Color::Reset); + } + + #[test] + fn rgb_to_ansi256_maps_known_anchors() { + assert_eq!(rgb_to_ansi256(0, 0, 0), 16); // cube black + assert_eq!(rgb_to_ansi256(255, 0, 0), 196); // pure red + assert_eq!(rgb_to_ansi256(0, 255, 0), 46); // pure green + assert_eq!(rgb_to_ansi256(0, 0, 255), 21); // pure blue + assert_eq!(rgb_to_ansi256(255, 255, 255), 231); // cube white + } + + #[test] + fn rgb_to_ansi256_prefers_grayscale_for_neutral_tones() { + // A mid gray should land on the grayscale ramp (232..=255), not the cube. + let idx = rgb_to_ansi256(128, 128, 128); + assert!((232..=255).contains(&idx), "got {idx}"); + } +} diff --git a/src/ui/review/diff_view.rs b/src/ui/review/diff_view.rs index 184e7d6..4293e56 100644 --- a/src/ui/review/diff_view.rs +++ b/src/ui/review/diff_view.rs @@ -19,6 +19,7 @@ use ratatui::prelude::*; use ratatui::widgets::*; use super::Appearance; +use super::color::{self, ColorDepth}; use super::highlight::{Highlighter, Segment}; /// Colors for the diff view, chosen for the terminal's light or dark @@ -38,10 +39,28 @@ pub struct Palette { } impl Palette { - pub fn for_appearance(appearance: Appearance) -> Self { - match appearance { + pub fn for_appearance(appearance: Appearance, depth: ColorDepth) -> Self { + let base = match appearance { Appearance::Dark => Palette::dark(), Appearance::Light => Palette::light(), + }; + base.adapted(depth) + } + + /// Downsample every RGB field to the terminal's color depth so the diff + /// backgrounds survive on 256-color terminals (named colors pass through). + fn adapted(self, depth: ColorDepth) -> Self { + let a = |c: Color| color::adapt(c, depth); + Palette { + add_bg: a(self.add_bg), + add_emph_bg: a(self.add_emph_bg), + del_bg: a(self.del_bg), + del_emph_bg: a(self.del_emph_bg), + cursor_bg: a(self.cursor_bg), + select_bg: a(self.select_bg), + empty_bg: a(self.empty_bg), + gutter_fg: a(self.gutter_fg), + header_fg: a(self.header_fg), } } @@ -308,6 +327,7 @@ pub fn render( h_scroll: usize, focused: bool, palette: Palette, + tab_width: usize, ) { let path_label = match &rf.diff.old_path { Some(old) => format!("{old} → {}", rf.diff.path), @@ -354,6 +374,7 @@ pub fn render( h_scroll, focused && i == cursor, palette, + tab_width, ) }) .collect() @@ -371,6 +392,7 @@ pub fn render( h_scroll, focused && i == cursor, palette, + tab_width, ) }) .collect() @@ -415,6 +437,7 @@ fn side_line_to_line<'a>( h_scroll: usize, cursor: bool, pal: Palette, + tab_width: usize, ) -> Line<'a> { // First column is a 1-cell comment marker; the rest is the body. let body_width = width.saturating_sub(1); @@ -439,6 +462,7 @@ fn side_line_to_line<'a>( h_scroll, cursor, pal, + tab_width, )); spans.push(Span::styled( " │ ", @@ -453,6 +477,7 @@ fn side_line_to_line<'a>( h_scroll, cursor, pal, + tab_width, )); Line::from(spans) } @@ -469,6 +494,7 @@ fn uni_line_to_line<'a>( h_scroll: usize, cursor: bool, pal: Palette, + tab_width: usize, ) -> Line<'a> { let body_width = width.saturating_sub(1); match line { @@ -506,6 +532,7 @@ fn uni_line_to_line<'a>( h_scroll, text_w, pal, + tab_width, )); Line::from(spans) } @@ -529,6 +556,7 @@ fn cell<'a>( h_scroll: usize, cursor: bool, pal: Palette, + tab_width: usize, ) -> Vec> { let Some(row) = row else { // No line on this side: blank gutter + filler at the empty-side bg. @@ -556,6 +584,7 @@ fn cell<'a>( h_scroll, text_w, pal, + tab_width, )); spans } @@ -613,6 +642,12 @@ fn num_span<'a>(num: Option, gw: usize, bg: Color, pal: Palette) -> Span< /// Build the styled, horizontally-scrolled, width-padded text spans for a line, /// layering syntax color, diff background, and word-level emphasis. `text` is /// the raw line, used as a fallback when no syntax segments are available. +/// +/// Tabs are expanded to `tab_width`-aligned spaces so indentation lines up — a +/// raw `\t` in a ratatui span otherwise renders as a single cell and collapses +/// indentation. Emphasis is keyed off the byte offset in `text`; the syntax +/// segments concatenate to the same bytes, so tracking bytes as we walk either +/// keeps both aligned. #[allow(clippy::too_many_arguments)] fn text_spans<'a>( text: &str, @@ -623,21 +658,33 @@ fn text_spans<'a>( h_scroll: usize, width: usize, pal: Palette, + tab_width: usize, ) -> Vec> { let base_bg = row_bg(kind, cursor, pal); let emph_bg = emph_bg(kind, cursor, pal); + let tab_width = tab_width.max(1); - // Expand to per-character (char, style), tracking byte offset for emphasis. + // Expand to per-cell (char, style), tracking byte offset for emphasis and + // visual column for tab stops. let mut chars: Vec<(char, Style)> = Vec::new(); let mut byte = 0usize; let mut push_char = |ch: char, fg: Option| { let emphasized = in_ranges(byte, emphasis); - let mut style = Style::default().bg(if emphasized { emph_bg } else { base_bg }); + let bg = if emphasized { emph_bg } else { base_bg }; + byte += ch.len_utf8(); + if ch == '\t' { + // Advance to the next tab stop, filling with the line's background. + let fill = tab_width - (chars.len() % tab_width); + for _ in 0..fill { + chars.push((' ', Style::default().bg(bg))); + } + return; + } + let mut style = Style::default().bg(bg); if let Some(c) = fg { style = style.fg(c); } chars.push((ch, style)); - byte += ch.len_utf8(); }; if segments.is_empty() { @@ -834,6 +881,66 @@ mod tests { assert!(!in_ranges(5, &ranges)); } + /// Collect the rendered characters (spans flattened) for assertions. + fn rendered_text(spans: &[Span]) -> String { + spans.iter().flat_map(|s| s.content.chars()).collect() + } + + #[test] + fn tab_expands_to_next_tab_stop() { + // A leading tab (width 4) becomes 4 spaces before the text. + let spans = text_spans( + "\tx", + &[], + &[], + RowKind::Context, + false, + 0, + 40, + Palette::dark(), + 4, + ); + let s = rendered_text(&spans); + assert!(s.starts_with(" x"), "tab should expand to 4 spaces, got {s:?}"); + } + + #[test] + fn tab_aligns_partial_column_to_stop() { + // "ab\tc" with width 4: after "ab" (col 2) a tab fills 2 cells to col 4. + let spans = text_spans( + "ab\tc", + &[], + &[], + RowKind::Context, + false, + 0, + 40, + Palette::dark(), + 4, + ); + let s = rendered_text(&spans); + assert!(s.starts_with("ab c"), "expected alignment to next stop, got {s:?}"); + } + + #[test] + fn tab_expands_within_syntax_segments() { + // Segments (syntax path) carry the tab too; it must still expand. + let segs = vec![(Style::default().fg(Color::Rgb(1, 2, 3)), "\tlet".to_string())]; + let spans = text_spans( + "\tlet", + &segs, + &[], + RowKind::Added, + false, + 0, + 40, + Palette::dark(), + 2, + ); + let s = rendered_text(&spans); + assert!(s.starts_with(" let"), "tab in segment should expand, got {s:?}"); + } + #[test] fn renders_unified_buffer_with_header_and_text() { use ratatui::Terminal; @@ -862,6 +969,7 @@ mod tests { 0, true, Palette::dark(), + 4, ) }) .unwrap(); @@ -900,6 +1008,7 @@ mod tests { 0, true, Palette::dark(), + 4, ) }) .unwrap(); diff --git a/src/ui/review/highlight.rs b/src/ui/review/highlight.rs index c39b482..2eb82ff 100644 --- a/src/ui/review/highlight.rs +++ b/src/ui/review/highlight.rs @@ -9,6 +9,7 @@ //! A size guard keeps a large generated/vendored file (few changes, many lines) //! from stalling the UI: past the guard, lines are returned as plain text. +use super::color::{self, ColorDepth}; use ratatui::style::{Color, Modifier, Style}; use std::sync::OnceLock; use syntect::easy::HighlightLines; @@ -39,12 +40,14 @@ pub type Segment = (Style, String); /// borrows the process-global theme set). pub struct Highlighter { theme: &'static Theme, + depth: ColorDepth, } impl Highlighter { /// Build a highlighter for `theme_name`, falling back to the default theme - /// and then to any available theme if the name is unknown. - pub fn new(theme_name: &str) -> Self { + /// and then to any available theme if the name is unknown. `depth` controls + /// whether the theme's 24-bit colors are downsampled to 256-color. + pub fn new(theme_name: &str, depth: ColorDepth) -> Self { let ts = theme_set(); let theme = ts .themes @@ -52,7 +55,7 @@ impl Highlighter { .or_else(|| ts.themes.get(DEFAULT_THEME)) .or_else(|| ts.themes.values().next()) .expect("syntect ships at least one default theme"); - Highlighter { theme } + Highlighter { theme, depth } } /// Highlight a whole file into line-indexed styled segments; callers index @@ -70,7 +73,7 @@ impl Highlighter { let mut out = Vec::new(); for line in LinesWithEndings::from(content) { match highlighter.highlight_line(line, ps) { - Ok(ranges) => out.push(to_segments(ranges)), + Ok(ranges) => out.push(to_segments(ranges, self.depth)), // Degrade a problem line to plain text rather than dropping it. Err(_) => out.push(vec![(Style::default(), strip_eol(line))]), } @@ -86,10 +89,10 @@ fn plain_lines(content: &str) -> Vec> { .collect() } -fn to_segments(ranges: Vec<(SynStyle, &str)>) -> Vec { +fn to_segments(ranges: Vec<(SynStyle, &str)>, depth: ColorDepth) -> Vec { let mut segs: Vec = ranges .into_iter() - .map(|(style, text)| (to_ratatui_style(style), text.to_string())) + .map(|(style, text)| (to_ratatui_style(style, depth), text.to_string())) .collect(); // The last segment carries the line's trailing newline; drop it. if let Some(last) = segs.last_mut() { @@ -107,12 +110,12 @@ fn strip_eol(line: &str) -> String { line.trim_end_matches(['\n', '\r']).to_string() } -/// Convert a syntect style to a ratatui style: RGB foreground plus font -/// modifiers. This is the bridge that lets us stay on ratatui 0.30 without the -/// `syntect-tui` crate (which pins ratatui 0.29). -pub fn to_ratatui_style(s: SynStyle) -> Style { +/// Convert a syntect style to a ratatui style: foreground color (downsampled to +/// `depth`) plus font modifiers. This is the bridge that lets us stay on ratatui +/// 0.30 without the `syntect-tui` crate (which pins ratatui 0.29). +pub fn to_ratatui_style(s: SynStyle, depth: ColorDepth) -> Style { let fg = s.foreground; - let mut style = Style::default().fg(Color::Rgb(fg.r, fg.g, fg.b)); + let mut style = Style::default().fg(color::adapt(Color::Rgb(fg.r, fg.g, fg.b), depth)); if s.font_style.contains(FontStyle::BOLD) { style = style.add_modifier(Modifier::BOLD); } @@ -163,16 +166,27 @@ mod tests { }, font_style: FontStyle::BOLD | FontStyle::ITALIC, }; - let style = to_ratatui_style(syn); + let style = to_ratatui_style(syn, ColorDepth::TrueColor); assert_eq!(style.fg, Some(Color::Rgb(10, 20, 30))); assert!(style.add_modifier.contains(Modifier::BOLD)); assert!(style.add_modifier.contains(Modifier::ITALIC)); assert!(!style.add_modifier.contains(Modifier::UNDERLINED)); } + #[test] + fn bridge_downsamples_rgb_under_256() { + let syn = SynStyle { + foreground: SynColor { r: 255, g: 0, b: 0, a: 255 }, + background: SynColor { r: 0, g: 0, b: 0, a: 255 }, + font_style: FontStyle::empty(), + }; + let style = to_ratatui_style(syn, ColorDepth::Ansi256); + assert_eq!(style.fg, Some(Color::Indexed(196))); + } + #[test] fn highlights_rust_into_multiple_colors_and_keeps_line_count() { - let h = Highlighter::new("base16-ocean.dark"); + let h = Highlighter::new("base16-ocean.dark", ColorDepth::TrueColor); let content = "fn main() {\n let x = 1;\n}\n"; let lines = h.highlight_file("main.rs", content); @@ -183,7 +197,7 @@ mod tests { #[test] fn unknown_extension_does_not_panic() { - let h = Highlighter::new("base16-ocean.dark"); + let h = Highlighter::new("base16-ocean.dark", ColorDepth::TrueColor); let lines = h.highlight_file("notes.unknownext", "hello world\n"); assert_eq!(lines.len(), 1); } @@ -191,13 +205,13 @@ mod tests { #[test] fn unknown_theme_falls_back() { // Must not panic on a bogus theme name. - let h = Highlighter::new("no-such-theme"); + let h = Highlighter::new("no-such-theme", ColorDepth::TrueColor); assert_eq!(h.highlight_file("a.rs", "fn a(){}\n").len(), 1); } #[test] fn oversized_file_returns_plain_segments() { - let h = Highlighter::new("base16-ocean.dark"); + let h = Highlighter::new("base16-ocean.dark", ColorDepth::TrueColor); let big = "x\n".repeat(MAX_HIGHLIGHT_LINES + 1); let lines = h.highlight_file("big.rs", &big); assert_eq!(lines.len(), MAX_HIGHLIGHT_LINES + 1); diff --git a/src/ui/review/mod.rs b/src/ui/review/mod.rs index 533d08e..fc7650a 100644 --- a/src/ui/review/mod.rs +++ b/src/ui/review/mod.rs @@ -3,16 +3,19 @@ //! highlighting (U3/U4); the file-tree sidebar (U5) and persistence (U7) slot in //! as those units land. +pub mod color; pub mod diff_view; pub mod file_tree; pub mod highlight; +use crate::git::log::{self, LogEntry}; use crate::git::review::blob; use crate::git::review::diff::{self, ChangedFile}; use crate::git::review::range::{self, Endpoint, ReviewRange}; use crate::git::review::state::{self, Comment, ReviewState, Side}; use crate::ui::terminal::with_terminal; use crate::ui::{render_help_bar, status_char, status_color}; +use color::ColorDepth; use crossterm::event::{self, Event, KeyCode, KeyModifiers}; use diff_view::{RenderedFile, ViewMode}; use file_tree::{FileTree, NodeKind}; @@ -24,6 +27,8 @@ use ratatui::widgets::*; use std::time::Duration; const SIDEBAR_WIDTH: u16 = 32; +/// How many recent commits the commit picker lists. +const COMMIT_PICKER_LIMIT: usize = 200; /// Resolved terminal appearance, driving the syntax theme and diff palette. #[derive(Clone, Copy, PartialEq, Eq)] @@ -52,6 +57,7 @@ enum Mode { CommentPopup, OrphanedList, RangeSwitch, + CommitPicker, Filter, Help, } @@ -75,25 +81,39 @@ struct Popup { } /// Launch the review TUI for an already-resolved range and changed-file list. +#[allow(clippy::too_many_arguments)] pub fn run( range: ReviewRange, files: Vec, theme: &str, min_width: u16, appearance: Appearance, + color_depth: ColorDepth, + tab_width: u16, ) -> Result<()> { // `with_terminal` enters the alternate screen / raw mode and restores it // (even on panic, via its guard) before returning; the inner Result carries // the loop's outcome plus an optional message to print after teardown. - let message = - with_terminal(|terminal| run_loop(terminal, range, files, theme, min_width, appearance)) - .into_diagnostic()??; + let message = with_terminal(|terminal| { + run_loop( + terminal, + range, + files, + theme, + min_width, + appearance, + color_depth, + tab_width, + ) + }) + .into_diagnostic()??; if let Some(msg) = message { println!("{msg}"); } Ok(()) } +#[allow(clippy::too_many_arguments)] fn run_loop( terminal: &mut crate::ui::Term, range: ReviewRange, @@ -101,8 +121,10 @@ fn run_loop( theme: &str, min_width: u16, appearance: Appearance, + color_depth: ColorDepth, + tab_width: u16, ) -> Result> { - let mut app = App::new(range, files, theme, min_width, appearance); + let mut app = App::new(range, files, theme, min_width, appearance, color_depth, tab_width); let mut needs_redraw = true; loop { @@ -161,6 +183,7 @@ struct App { h_scroll: usize, view_override: Option, min_width: u16, + tab_width: u16, palette: diff_view::Palette, show_sidebar: bool, mode: Mode, @@ -175,18 +198,26 @@ struct App { pending_bracket: Option, last_diff_height: usize, last_view: ViewMode, + /// Commits offered by the commit picker (lazily loaded on first open). + commits: Vec, + commit_cursor: usize, + /// A commit marked as one end of a range in the picker (press space). + commit_anchor: Option, } impl App { + #[allow(clippy::too_many_arguments)] fn new( range: ReviewRange, files: Vec, theme: &str, min_width: u16, appearance: Appearance, + color_depth: ColorDepth, + tab_width: u16, ) -> Self { let cache = (0..files.len()).map(|_| None).collect(); - let palette = diff_view::Palette::for_appearance(appearance); + let palette = diff_view::Palette::for_appearance(appearance, color_depth); // Key persistence on the clone's shared git dir + the range scope, then // resume any saved review for this (clone, scope). let key = crate::git::worktree::common_git_dir() @@ -200,7 +231,7 @@ impl App { files, selected: 0, cache, - highlighter: Highlighter::new(theme), + highlighter: Highlighter::new(theme, color_depth), review, tree, tree_cursor: 0, @@ -210,6 +241,7 @@ impl App { h_scroll: 0, view_override: None, min_width, + tab_width, palette, show_sidebar: true, mode: Mode::Normal, @@ -224,6 +256,9 @@ impl App { pending_bracket: None, last_diff_height: 1, last_view: ViewMode::SideBySide, + commits: Vec::new(), + commit_cursor: 0, + commit_anchor: None, } } @@ -305,6 +340,10 @@ impl App { self.handle_rangeswitch_key(key); false } + Mode::CommitPicker => { + self.handle_commitpicker_key(key); + false + } Mode::Filter => { self.handle_filter_key(key); false @@ -801,7 +840,8 @@ impl App { } KeyCode::Char('b') => range::resolve_branch(None), KeyCode::Char('u') => range::resolve_uncommitted(), - KeyCode::Char('t') => range::resolve_branch(None).map(|mut r| { + // Accept both `w` (working tree) and the legacy `t`. + KeyCode::Char('w') | KeyCode::Char('t') => range::resolve_branch(None).map(|mut r| { r.to = Endpoint::WorkingTree; r.label = format!("{} +worktree", r.label); // Distinct scope so the +worktree review doesn't share a @@ -809,6 +849,10 @@ impl App { r.scope_id = format!("{}+worktree", r.scope_id); r }), + KeyCode::Char('c') => { + self.open_commit_picker(); + return; + } _ => return, }; match resolved { @@ -820,6 +864,99 @@ impl App { } } + /// Open the commit picker, loading recent commits on first use. + fn open_commit_picker(&mut self) { + if self.commits.is_empty() { + match log::get_log(COMMIT_PICKER_LIMIT) { + Ok(graph) => self.commits = graph.entries, + Err(e) => { + self.status = Some(format!("Couldn't load commits: {e}")); + self.mode = Mode::Normal; + return; + } + } + } + if self.commits.is_empty() { + self.status = Some("No commits to pick from".into()); + self.mode = Mode::Normal; + return; + } + self.commit_cursor = 0; + self.commit_anchor = None; + self.mode = Mode::CommitPicker; + } + + fn handle_commitpicker_key(&mut self, key: event::KeyEvent) { + let len = self.commits.len(); + match (key.code, key.modifiers) { + (KeyCode::Esc, _) | (KeyCode::Char('q'), _) => { + self.mode = Mode::RangeSwitch; + self.commit_anchor = None; + } + (KeyCode::Char('j'), _) | (KeyCode::Down, _) => { + if len > 0 { + self.commit_cursor = (self.commit_cursor + 1).min(len - 1); + } + } + (KeyCode::Char('k'), _) | (KeyCode::Up, _) => { + self.commit_cursor = self.commit_cursor.saturating_sub(1); + } + (KeyCode::Char('d'), KeyModifiers::CONTROL) => { + self.commit_cursor = (self.commit_cursor + 10).min(len.saturating_sub(1)); + } + (KeyCode::Char('u'), KeyModifiers::CONTROL) => { + self.commit_cursor = self.commit_cursor.saturating_sub(10); + } + (KeyCode::Char('g'), _) => self.commit_cursor = 0, + (KeyCode::Char('G'), _) => self.commit_cursor = len.saturating_sub(1), + // Space marks/unmarks a range endpoint. + (KeyCode::Char(' '), _) => { + self.commit_anchor = match self.commit_anchor { + Some(a) if a == self.commit_cursor => None, + _ => Some(self.commit_cursor), + }; + } + (KeyCode::Enter, _) => self.confirm_commit_pick(), + _ => {} + } + } + + /// Resolve the picker's selection into a range and switch to it. With no + /// anchor, review the single selected commit; with an anchor, review the + /// inclusive range between the two picked commits (older..newer). + fn confirm_commit_pick(&mut self) { + let Some(sel) = self.commits.get(self.commit_cursor) else { + return; + }; + let resolved = match self.commit_anchor { + None => range::resolve_commit(&sel.oid.to_string()), + Some(anchor) => { + let Some(other) = self.commits.get(anchor) else { + return; + }; + // The log is newest-first, so the larger index is the older + // commit. Diff from just before the older commit to the newer. + let (older, newer) = if anchor >= self.commit_cursor { + (other, sel) + } else { + (sel, other) + }; + // `older^..newer` includes the older commit itself in the diff. + // Short ids are git-unique and keep the header label readable. + let from = format!("{}^", older.short_id); + range::resolve_explicit_range(&from, &newer.short_id) + } + }; + self.commit_anchor = None; + match resolved { + Ok(range) => self.switch_range(range), + Err(e) => { + self.status = Some(format!("Commit range failed: {e}")); + self.mode = Mode::Normal; + } + } + } + /// Re-resolve to `new_range`, rebuild the file list and diffs, and load the /// (separately-keyed) review for the new scope. The current scope's review /// is saved first. @@ -913,6 +1050,7 @@ impl App { self.h_scroll, self.focus == Focus::Diff, self.palette, + self.tab_width as usize, ); } @@ -923,34 +1061,105 @@ impl App { Mode::CommentPopup => self.draw_popup(f, area), Mode::OrphanedList => self.draw_orphans(f, area), Mode::RangeSwitch => self.draw_rangeswitch(f, area), + Mode::CommitPicker => self.draw_commit_picker(f, area), _ => {} } } fn draw_rangeswitch(&self, f: &mut Frame, area: Rect) { + let key = |k: &str, desc: &str| { + Line::from(vec![ + Span::styled( + format!(" {k} "), + Style::default().fg(Color::Cyan).add_modifier(Modifier::BOLD), + ), + Span::raw(desc.to_string()), + ]) + }; let lines = vec![ Line::from(Span::styled( - "Switch range", + "What do you want to review?", Style::default().fg(Color::Cyan).add_modifier(Modifier::BOLD), )), Line::raw(""), - Line::raw("b branch vs base (committed)"), - Line::raw("t branch vs base + working tree"), - Line::raw("u uncommitted (working tree vs HEAD)"), + key("u", "Uncommitted changes (working tree vs HEAD)"), + key("b", "All changes on this branch (base…HEAD)"), + key("w", "Branch changes + uncommitted working tree"), + key("c", "Pick a commit or commit range…"), Line::raw(""), Line::from(Span::styled( "esc to cancel", Style::default().fg(Color::DarkGray), )), ]; - let popup = centered_rect(50, 45, area); + let popup = centered_rect(60, 50, area); f.render_widget(Clear, popup); let block = Block::default() .borders(Borders::ALL) - .title(format!(" Range — now: {} ", self.range.label)); + .title(format!(" Review scope — now: {} ", self.range.label)); f.render_widget(Paragraph::new(lines).block(block), popup); } + fn draw_commit_picker(&self, f: &mut Frame, area: Rect) { + let popup = centered_rect(80, 80, area); + f.render_widget(Clear, popup); + + let hint = match self.commit_anchor { + Some(_) => " Pick commit — ⏎ review range · space clear mark ", + None => " Pick commit — ⏎ review one · space mark range start ", + }; + let block = Block::default() + .borders(Borders::ALL) + .border_style(Style::default().fg(Color::Cyan)) + .title(hint); + let inner = block.inner(popup); + f.render_widget(block, popup); + + let h = inner.height as usize; + let start = if self.commit_cursor >= h { + self.commit_cursor - h + 1 + } else { + 0 + }; + // Width available for the summary after the fixed-width columns. + let sha_w = 8usize; + let time_w = 14usize; + let summary_w = (inner.width as usize).saturating_sub(sha_w + time_w + 4); + + let lines: Vec = self + .commits + .iter() + .enumerate() + .skip(start) + .take(h) + .map(|(i, c)| { + let on_cursor = i == self.commit_cursor; + let is_anchor = self.commit_anchor == Some(i); + let mark = if is_anchor { "◆ " } else { " " }; + let mut summary = c.summary.clone(); + if summary.chars().count() > summary_w { + summary = summary.chars().take(summary_w.saturating_sub(1)).collect(); + summary.push('…'); + } + let base = if on_cursor { + Style::default().bg(self.palette.select_bg) + } else { + Style::default() + }; + Line::from(vec![ + Span::styled(mark, base.fg(Color::Magenta)), + Span::styled(format!("{:sha_w$}", c.short_id), base.fg(Color::Yellow)), + Span::styled(format!("{summary:summary_w$} "), base), + Span::styled( + format!("{:>time_w$}", c.time_relative), + base.fg(Color::DarkGray), + ), + ]) + }) + .collect(); + f.render_widget(Paragraph::new(lines), inner); + } + fn draw_orphans(&self, f: &mut Frame, area: Rect) { let mut lines = vec![ Line::from(Span::styled( @@ -990,7 +1199,10 @@ impl App { .title(format!(" Review — {} ", self.range.label)); let inner = block.inner(area); f.render_widget(block, area); - let msg = format!("No changes in {}.\n\nPress q to quit.", self.range.label); + let msg = format!( + "No changes in {}.\n\nPress s to change scope (uncommitted · branch · pick a commit),\nor q to quit.", + self.range.label + ); f.render_widget( Paragraph::new(msg) .style(Style::default().fg(Color::DarkGray)) @@ -1103,7 +1315,7 @@ impl App { ("D", "del"), ("]c", "hunk"), ("Tab", "file"), - ("s", "range"), + ("s", "scope"), ("v", "view"), ("F", "finish"), ("X", "reset"), @@ -1119,7 +1331,13 @@ impl App { ("esc", "cancel"), ], Mode::OrphanedList => &[("esc", "close")], - Mode::RangeSwitch => &[("b/t/u", "pick"), ("esc", "cancel")], + Mode::RangeSwitch => &[("u/b/w/c", "pick scope"), ("esc", "cancel")], + Mode::CommitPicker => &[ + ("j/k", "move"), + ("space", "mark range"), + ("⏎", "review"), + ("esc", "back"), + ], Mode::Filter => &[("type", "filter"), ("⏎", "apply"), ("esc", "clear")], Mode::Help => &[("esc", "close")], }; @@ -1178,7 +1396,7 @@ impl App { Line::raw("g / G top / bottom"), Line::raw("]c / [c next / prev hunk (also } / {)"), Line::raw("Tab focus the file sidebar (j/k move, ⏎ open, / filter)"), - Line::raw("s switch range (branch / +worktree / uncommitted)"), + Line::raw("s change scope: uncommitted / branch / +worktree / pick commit"), Line::raw("h / l ← → scroll horizontally"), Line::raw("c comment on the current line"), Line::raw("V start a multi-line selection, then c"),