diff --git a/git-workon-review/src/align.rs b/git-workon-review/src/align.rs index 931bd105..f57f6250 100644 --- a/git-workon-review/src/align.rs +++ b/git-workon-review/src/align.rs @@ -21,6 +21,20 @@ //! (`Patch::line_in_hunk`'s `old_lineno`/`new_lineno`), not something this module can violate, //! so the pairing code below `expect()`s the lineno for the side each kind is documented to //! carry. +//! +//! ## Progressive gap expansion (CS8) +//! +//! [`collapse_gaps`]'s collapsed [`DisplayRow::Gap`]/[`InlineRow::Gap`] markers each carry a +//! `key` — the hidden run's start index in the pre-collapse [`AlignedRow`] space — so a caller +//! can ask for MORE of that specific run to be revealed without losing track of it as it widens. +//! [`collapse_gaps_with_expansions`] takes a `key -> `[`GapExpansion`]` map and re-collapses each +//! run against its entry (if any): `before`/`after` grow the kept window at that edge, `full` +//! reveals the whole run. `collapse_gaps` itself is the empty-map case. State ownership (which +//! gaps are expanded, and by how much) lives OUTSIDE this module, in +//! [`crate::app::FileView::expansions`] — this module stays pure, taking the map as input rather +//! than mutating anything. + +use std::collections::HashMap; use crate::model::{Hunk, HunkLine, LineKind}; @@ -183,16 +197,37 @@ pub fn align_file(hunks: &[Hunk], old_line_count: usize, new_line_count: usize) /// /// Unchanged stretches longer than `2 * CONTEXT_LINES` collapse to a single [`DisplayRow::Gap`] /// so the view doesn't scroll through pages of untouched code. Gap rows are layout-agnostic — -/// they span both panes in SBS. +/// they span both panes in SBS. `key` identifies the collapsed run so a caller can request it be +/// progressively revealed — see [`GapExpansion`] and [`collapse_gaps_with_expansions`]. #[derive(Debug, Clone, Copy)] pub enum DisplayRow { Row(AlignedRow), - Gap { skipped: usize }, + Gap { key: usize, skipped: usize }, } /// Number of context lines kept around hunk content on each side of a gap. pub const CONTEXT_LINES: usize = 3; +/// How far a single collapsed gap has been expanded (CS8). Accumulates across repeated `Enter` +/// presses: `before`/`after` each independently widen how many rows are revealed at that edge of +/// the gap, and `full` — once set — reveals the whole run regardless of `before`/`after`. +/// +/// Keyed in the caller's map by the SAME `key` [`DisplayRow::Gap`]/[`InlineRow::Gap`] carry: the +/// hidden run's start index in the pre-collapse [`AlignedRow`] space. That space never changes +/// shape as a gap widens (only how much of it stays hidden changes), so the key stays valid +/// across repeated expansion requests for the same gap. +#[derive(Debug, Clone, Copy, Default)] +pub struct GapExpansion { + /// Extra rows revealed at the gap's leading edge (extends the visible context below the + /// preceding hunk downward, growing the row range kept immediately after `run_start`). + pub before: usize, + /// Extra rows revealed at the gap's trailing edge (extends the visible context above the + /// following hunk upward, growing the row range kept immediately before `run_end`). + pub after: usize, + /// Reveal every row in the run, ignoring `before`/`after`. + pub full: bool, +} + /// Collapse long unchanged stretches in `rows` into [`DisplayRow::Gap`] markers, keeping /// [`CONTEXT_LINES`] rows of context immediately around hunk content (Del/Add/Filler rows). /// @@ -200,12 +235,33 @@ pub const CONTEXT_LINES: usize = 3; /// (enough to keep `CONTEXT_LINES` on both sides of the gap); shorter stretches, including ones /// between two hunks that are close together, are left as-is (no gap row — the hunks /// effectively merge under one continuous context run). +/// +/// Thin wrapper over [`collapse_gaps_with_expansions`] with no expansions applied. pub fn collapse_gaps(rows: &[AlignedRow]) -> Vec { - collapse_gaps_with(rows, CONTEXT_LINES) + collapse_gaps_with_expansions(rows, &HashMap::new()) } -/// Same as [`collapse_gaps`] but with an explicit context-line count, for testing. +/// Same as [`collapse_gaps`], but a gap whose key has an entry in `expansions` reveals extra rows +/// at its edges (or its whole run) instead of collapsing to the base [`CONTEXT_LINES`] window — +/// see [`GapExpansion`]. +pub fn collapse_gaps_with_expansions( + rows: &[AlignedRow], + expansions: &HashMap, +) -> Vec { + collapse_gaps_inner(rows, CONTEXT_LINES, expansions) +} + +/// Same as [`collapse_gaps_with_expansions`] but with an explicit context-line count, for testing. +#[cfg(test)] fn collapse_gaps_with(rows: &[AlignedRow], context: usize) -> Vec { + collapse_gaps_inner(rows, context, &HashMap::new()) +} + +fn collapse_gaps_inner( + rows: &[AlignedRow], + context: usize, + expansions: &HashMap, +) -> Vec { let is_context = |row: &AlignedRow| { matches!( (row.old_kind, row.new_kind), @@ -243,13 +299,31 @@ fn collapse_gaps_with(rows: &[AlignedRow], context: usize) -> Vec { for row in &rows[run_start..run_end] { out.push(DisplayRow::Row(*row)); } + i = run_end; + continue; + } + + // This run collapses to a gap (before any expansion is applied) — the key is stable + // across future expansion requests, so compute it once here. + let key = run_start; + let expansion = expansions.get(&key).copied().unwrap_or_default(); + + let effective_before = (keep_before + expansion.before).min(run_len); + let effective_after = (keep_after + expansion.after).min(run_len - effective_before); + + if expansion.full || effective_before + effective_after >= run_len { + // The expansion consumes the whole run (or was asked to): no gap left worth + // collapsing, emit every row. + for row in &rows[run_start..run_end] { + out.push(DisplayRow::Row(*row)); + } } else { - for row in &rows[run_start..run_start + keep_before] { + for row in &rows[run_start..run_start + effective_before] { out.push(DisplayRow::Row(*row)); } - let skipped = run_len - keep_before - keep_after; - out.push(DisplayRow::Gap { skipped }); - for row in &rows[run_end - keep_after..run_end] { + let skipped = run_len - effective_before - effective_after; + out.push(DisplayRow::Gap { key, skipped }); + for row in &rows[run_end - effective_after..run_end] { out.push(DisplayRow::Row(*row)); } } @@ -259,6 +333,59 @@ fn collapse_gaps_with(rows: &[AlignedRow], context: usize) -> Vec { out } +/// The currently-hidden [`AlignedRow`] sub-range `[start, end)` for the gap keyed `key`, given +/// its current `expansion` (if any) — used by [`crate::app::FileView::scope_expand_gap`] (CS9) to +/// measure how much of a gap's hidden run a candidate tree-sitter scope range would additionally +/// uncover. `None` when `key` no longer denotes an actual gap: not a context-run start, the run is +/// too short to have collapsed in the first place, or `expansion` already reveals the whole run. +/// +/// Mirrors the run-measuring steps in [`collapse_gaps_inner`] (same `keep_before`/`keep_after`/ +/// `effective_before`/`effective_after` derivation) rather than sharing code with it, because that +/// function additionally needs `run_end` and the row slices themselves to emit `DisplayRow`s, +/// while this one only needs the hidden index range for a `key` a caller already has — keep both +/// in sync if the collapse rule ever changes. +pub(crate) fn gap_hidden_range( + rows: &[AlignedRow], + key: usize, + expansions: &HashMap, +) -> Option<(usize, usize)> { + let is_context = |row: &AlignedRow| { + matches!( + (row.old_kind, row.new_kind), + (CellKind::Context, CellKind::Context) + ) + }; + + let run_start = key; + if run_start >= rows.len() || !is_context(&rows[run_start]) { + return None; + } + let mut run_end = run_start; + while run_end < rows.len() && is_context(&rows[run_end]) { + run_end += 1; + } + let run_len = run_end - run_start; + + let keep_before = if run_start == 0 { 0 } else { CONTEXT_LINES }; + let keep_after = if run_end == rows.len() { + 0 + } else { + CONTEXT_LINES + }; + if (keep_before == 0 && keep_after == 0) || run_len <= keep_before + keep_after { + return None; + } + + let expansion = expansions.get(&key).copied().unwrap_or_default(); + let effective_before = (keep_before + expansion.before).min(run_len); + let effective_after = (keep_after + expansion.after).min(run_len - effective_before); + if expansion.full || effective_before + effective_after >= run_len { + return None; + } + + Some((run_start + effective_before, run_end - effective_after)) +} + /// One row of the inline (unified, single-column) display. /// /// Built by [`inline_rows`] from the SAME gap-collapsed [`DisplayRow`] vector [`collapse_gaps`] @@ -290,6 +417,7 @@ pub enum InlineRow { paired_old: Option, }, Gap { + key: usize, skipped: usize, }, } @@ -348,9 +476,12 @@ pub fn inline_rows(display: &[DisplayRow]) -> Vec { for row in display { match row { - DisplayRow::Gap { skipped } => { + DisplayRow::Gap { key, skipped } => { flush(&mut run, &mut out); - out.push(InlineRow::Gap { skipped: *skipped }); + out.push(InlineRow::Gap { + key: *key, + skipped: *skipped, + }); } DisplayRow::Row(r) if r.old_kind == CellKind::Context && r.new_kind == CellKind::Context => @@ -540,7 +671,7 @@ mod tests { assert!(matches!(row, DisplayRow::Row(r) if r.old_kind == CellKind::Context)); } match display[4] { - DisplayRow::Gap { skipped } => assert_eq!(skipped, 4), + DisplayRow::Gap { skipped, .. } => assert_eq!(skipped, 4), other => panic!("expected gap row, got {other:?}"), } for row in &display[5..8] { @@ -608,7 +739,7 @@ mod tests { // gap, 3 ctx, change assert_eq!(display.len(), 5); match display[0] { - DisplayRow::Gap { skipped } => assert_eq!(skipped, 7), + DisplayRow::Gap { skipped, .. } => assert_eq!(skipped, 7), other => panic!("expected gap row, got {other:?}"), } for row in &display[1..4] { @@ -635,7 +766,7 @@ mod tests { assert!(matches!(row, DisplayRow::Row(_))); } match display[4] { - DisplayRow::Gap { skipped } => assert_eq!(skipped, 7), + DisplayRow::Gap { skipped, .. } => assert_eq!(skipped, 7), other => panic!("expected gap row, got {other:?}"), } } @@ -739,8 +870,243 @@ mod tests { assert!( inline .iter() - .any(|r| matches!(r, InlineRow::Gap { skipped: 4 })), + .any(|r| matches!(r, InlineRow::Gap { skipped: 4, .. })), "expected the gap row to survive the inline conversion unchanged: {inline:?}" ); } + + // ── CS8: progressive gap expansion ────────────────────────────────────── + + /// One change row, a run of `run_len` context rows, one more change row — the shape every + /// CS8 expansion test collapses. With `context = 3` the base hidden count is + /// `run_len - 2 * 3`. + fn change_then_context_run_then_change(run_len: usize) -> Vec { + let mut rows = vec![change_row( + Row::Line(1), + Row::Line(1), + CellKind::Del, + CellKind::Add, + )]; + rows.extend((2..=run_len + 1).map(context_row)); + rows.push(change_row( + Row::Line(run_len + 2), + Row::Line(run_len + 2), + CellKind::Del, + CellKind::Add, + )); + rows + } + + #[test] + fn collapse_gaps_matches_collapse_gaps_with_expansions_over_an_empty_map() { + // `collapse_gaps` is a thin wrapper — pin that it's byte-for-byte the same output as + // calling the expansion-aware entry point with nothing to expand (the pre-CS8 behavior + // every other test in this module already exercises via `collapse_gaps_with`). + let rows = change_then_context_run_then_change(16); + let via_collapse_gaps = collapse_gaps(&rows); + let via_expansions = collapse_gaps_with_expansions(&rows, &HashMap::new()); + assert_eq!(via_collapse_gaps.len(), via_expansions.len()); + for (a, b) in via_collapse_gaps.iter().zip(via_expansions.iter()) { + match (a, b) { + ( + DisplayRow::Gap { + key: ka, + skipped: sa, + }, + DisplayRow::Gap { + key: kb, + skipped: sb, + }, + ) => { + assert_eq!(ka, kb); + assert_eq!(sa, sb); + } + (DisplayRow::Row(ra), DisplayRow::Row(rb)) => { + assert_eq!(ra.old, rb.old); + assert_eq!(ra.new, rb.new); + } + _ => panic!("row kind mismatch: {a:?} vs {b:?}"), + } + } + } + + /// The single [`DisplayRow::Gap`]'s `(key, skipped)` in `display` — the CS8 expansion tests' + /// index-free lookup (the gap's display position depends on how much kept context precedes + /// it, which is exactly what these tests vary). + fn only_gap(display: &[DisplayRow]) -> (usize, usize) { + display + .iter() + .find_map(|r| match r { + DisplayRow::Gap { key, skipped } => Some((*key, *skipped)), + _ => None, + }) + .expect("expected a gap row") + } + + #[test] + fn partial_expansion_reveals_rows_at_both_edges_and_shrinks_skipped() { + // run_len = 16 -> base hidden (K) = 16 - 3 - 3 = 10. + let rows = change_then_context_run_then_change(16); + let (key, base_skipped) = only_gap(&collapse_gaps(&rows)); + assert_eq!(base_skipped, 10, "base hidden count (K)"); + + let mut expansions = HashMap::new(); + expansions.insert( + key, + GapExpansion { + before: 3, + after: 2, + full: false, + }, + ); + let display = collapse_gaps_with_expansions(&rows, &expansions); + + // change, 3 base + 3 revealed before = 6 kept-before rows, gap, 3 base + 2 revealed + // after = 5 kept-after rows, change. + assert_eq!(display.len(), 1 + 6 + 1 + 5 + 1); + for row in &display[1..7] { + assert!(matches!(row, DisplayRow::Row(_))); + } + match display[7] { + DisplayRow::Gap { + key: gap_key, + skipped, + } => { + assert_eq!( + gap_key, key, + "the gap's key must not change across expansion" + ); + assert_eq!(skipped, 5, "K - 5 == 10 - (3 + 2)"); + } + other => panic!("expected a gap row, got {other:?}"), + } + for row in &display[8..13] { + assert!(matches!(row, DisplayRow::Row(_))); + } + assert!(matches!(display[13], DisplayRow::Row(_))); + } + + #[test] + fn widening_an_expansion_accumulates_and_shrinks_skipped_further() { + let rows = change_then_context_run_then_change(20); // K = 20 - 6 = 14 + let (key, _) = only_gap(&collapse_gaps(&rows)); + + // First press: reveal 5 more rows at the leading edge. + let mut expansions = HashMap::new(); + expansions.insert( + key, + GapExpansion { + before: 5, + after: 0, + full: false, + }, + ); + let after_first = collapse_gaps_with_expansions(&rows, &expansions); + let skipped_after_first = match after_first + .iter() + .find(|r| matches!(r, DisplayRow::Gap { .. })) + { + Some(DisplayRow::Gap { skipped, .. }) => *skipped, + _ => panic!("expected a surviving gap row after the first press"), + }; + assert_eq!(skipped_after_first, 14 - 5); + + // Second press accumulates on top of the first (mirrors `FileView::expand_gap`'s + // `entry.before += more_before`), rather than replacing it. + expansions.get_mut(&key).unwrap().before += 5; + let after_second = collapse_gaps_with_expansions(&rows, &expansions); + let skipped_after_second = match after_second + .iter() + .find(|r| matches!(r, DisplayRow::Gap { .. })) + { + Some(DisplayRow::Gap { skipped, .. }) => *skipped, + _ => panic!("expected a surviving gap row after the second press"), + }; + assert_eq!(skipped_after_second, 14 - 10); + assert!(skipped_after_second < skipped_after_first); + } + + #[test] + fn full_expansion_removes_the_gap_row_entirely() { + let rows = change_then_context_run_then_change(16); + let (key, _) = only_gap(&collapse_gaps(&rows)); + let mut expansions = HashMap::new(); + expansions.insert( + key, + GapExpansion { + before: 0, + after: 0, + full: true, + }, + ); + let display = collapse_gaps_with_expansions(&rows, &expansions); + assert!( + display.iter().all(|r| matches!(r, DisplayRow::Row(_))), + "a full expansion must emit every row, no Gap: {display:?}" + ); + assert_eq!(display.len(), rows.len()); + } + + #[test] + fn expansion_consuming_the_whole_run_removes_the_gap_row_without_full() { + // K = 10; before + after (6 + 4 = 10) exactly covers the hidden run without `full`. + let rows = change_then_context_run_then_change(16); + let (key, _) = only_gap(&collapse_gaps(&rows)); + let mut expansions = HashMap::new(); + expansions.insert( + key, + GapExpansion { + before: 6, + after: 4, + full: false, + }, + ); + let display = collapse_gaps_with_expansions(&rows, &expansions); + assert!( + display.iter().all(|r| matches!(r, DisplayRow::Row(_))), + "before + after covering the whole run must emit every row, no Gap: {display:?}" + ); + assert_eq!(display.len(), rows.len()); + } + + #[test] + fn inline_mirror_stays_consistent_with_the_same_expansions_map() { + let rows = change_then_context_run_then_change(16); + let (key, _) = only_gap(&collapse_gaps(&rows)); + let mut expansions = HashMap::new(); + expansions.insert( + key, + GapExpansion { + before: 3, + after: 2, + full: false, + }, + ); + let display = collapse_gaps_with_expansions(&rows, &expansions); + let inline = inline_rows(&display); + + // The SBS gap and the inline gap must carry the same key and skipped count — inline + // reuses the same gap-collapsed `display` vector rather than re-deriving gaps itself. + let sbs_gap = display + .iter() + .find_map(|r| match r { + DisplayRow::Gap { key, skipped } => Some((*key, *skipped)), + _ => None, + }) + .expect("expected a surviving SBS gap"); + let inline_gap = inline + .iter() + .find_map(|r| match r { + InlineRow::Gap { key, skipped } => Some((*key, *skipped)), + _ => None, + }) + .expect("expected a surviving inline gap"); + assert_eq!(sbs_gap, inline_gap); + + // Context rows revealed at the leading edge (old=2..=4, new=2..=4 in this fixture) show + // up as `InlineRow::Context` entries before the inline gap. + assert!(inline + .iter() + .any(|r| matches!(r, InlineRow::Context { old: 4, new: 4 }))); + } } diff --git a/git-workon-review/src/app.rs b/git-workon-review/src/app.rs index 1bbe99d8..49febfe1 100644 --- a/git-workon-review/src/app.rs +++ b/git-workon-review/src/app.rs @@ -16,17 +16,23 @@ use git2::Repository; use workon::{Changeset, ChangesetSpan}; use crate::acquire::{ChangesetDiff, WorktreeDiffs}; -use crate::align::{align_file, collapse_gaps, inline_rows, CellKind, DisplayRow, InlineRow, Row}; +use crate::align::{ + align_file, collapse_gaps_with_expansions, gap_hidden_range, inline_rows, AlignedRow, CellKind, + DisplayRow, GapExpansion, InlineRow, Row, +}; use crate::apply::{Git2Applier, StageVerb}; use crate::config::RawViewConfig; -use crate::highlight::{FgSpan, TsHighlighter}; +use crate::highlight::{lang_key_for_ext, FgSpan, TsHighlighter}; +use crate::icons::OutlineIcons; use crate::model::{DiffModel, FileChange, FileStatus, Hunk, LineKind}; use crate::ops; -use crate::outline::{self, OutlineChangeset, OutlineFile, OutlineItem, OutlineMode}; +use crate::outline::{self, OutlineChangeset, OutlineFile, OutlineItem, OutlineMode, OutlineOrder}; use crate::queue::{OpOutcome, StagingOp, StagingQueue}; use crate::refresh::{IndexSignature, RefreshCoordinator}; +use crate::scope::enclosing_scope_lines; use crate::source::{resolve_source, Source}; use crate::stage_op::{FileStagingOp, LineSelectionOp}; +use crate::summary; use crate::synthesis::LineSelection; use crate::wordiff::{word_diff_spans, Span}; @@ -50,6 +56,20 @@ const SCROLLOFF: usize = 2; /// staged/unstaged split zoom). #[derive(Debug)] pub struct FileView { + /// The pre-collapse row list [`Self::display`]/[`Self::inline`] derive from — retained (CS8) + /// so a gap can be re-collapsed with a wider [`GapExpansion`] window without re-diffing the + /// file. `AlignedRow` is small/`Copy`, so cloning the whole vector per expansion is cheap + /// relative to re-running `align_file`. + aligned: Vec, + /// Per-gap expansion requests, keyed by the hidden run's start index in [`Self::aligned`] + /// (the same key [`DisplayRow::Gap`]/[`InlineRow::Gap`] carry). Reset to empty on every + /// [`Self::load`] — expansions are NOT preserved across a refresh; the view rebuilds from + /// scratch and every gap re-collapses to its base window. See [`Self::expand_gap`]. + expansions: HashMap, + /// The file's hunks, retained (CS8) alongside [`Self::aligned`] so [`Self::rebuild_rows`] can + /// recompute [`Self::display_hunk`]/[`Self::inline_hunk`] after an expansion without needing + /// the original [`FileChange`] back. + hunks: Vec, old_text: String, new_text: String, old_lines: Vec, @@ -134,9 +154,45 @@ impl FileView { let old_lines: Vec = old_text.lines().map(str::to_string).collect(); let new_lines: Vec = new_text.lines().map(str::to_string).collect(); - let aligned = align_file(&file.hunks, old_lines.len(), new_lines.len()); - let display = collapse_gaps(&aligned.rows); - let first_hunk_row = display + let aligned = align_file(&file.hunks, old_lines.len(), new_lines.len()).rows; + let old_hl = ts.highlight_file(old_source_path, &old_text); + let new_hl = ts.highlight_file(&file.path, &new_text); + + let mut view = Self { + aligned, + expansions: HashMap::new(), + hunks: file.hunks.clone(), + old_text, + new_text, + old_lines, + new_lines, + display: Vec::new(), + first_hunk_row: 0, + first_inline_hunk_row: 0, + old_hl, + new_hl, + word_spans: HashMap::new(), + inline: Vec::new(), + inline_word_spans: HashMap::new(), + display_hunk: Vec::new(), + inline_hunk: Vec::new(), + }; + view.rebuild_rows(); + view + } + + /// Recompute [`Self::display`]/[`Self::inline`] (and everything derived from them) from + /// [`Self::aligned`] + [`Self::expansions`] — called once at [`Self::load`] and again after + /// every [`Self::expand_gap`]. Row-keyed word-span caches are cleared: an expansion changes + /// which display/inline index a given content row lands at, so a cached span keyed by the OLD + /// index would silently mismatch the row it renders under. The highlight caches + /// ([`Self::old_hl`]/[`Self::new_hl`]) are source-line-indexed (one entry per line of the full + /// old/new text), not row-indexed, so an expansion — which only changes how many already-hl'd + /// lines are VISIBLE — never invalidates them. + fn rebuild_rows(&mut self) { + self.display = collapse_gaps_with_expansions(&self.aligned, &self.expansions); + self.first_hunk_row = self + .display .iter() .position(|row| { matches!( @@ -146,45 +202,100 @@ impl FileView { }) .unwrap_or(0); - let old_hl = ts.highlight_file(old_source_path, &old_text); - let new_hl = ts.highlight_file(&file.path, &new_text); - let inline = inline_rows(&display); - let first_inline_hunk_row = inline + self.inline = inline_rows(&self.display); + self.first_inline_hunk_row = self + .inline .iter() .position(is_inline_hunk_content_row) .unwrap_or(0); - let display_hunk = display + self.display_hunk = self + .display .iter() .map(|row| { let (old, new) = display_row_linenos(row); - hunk_for_linenos(&file.hunks, old, new) + hunk_for_linenos(&self.hunks, old, new) }) .collect(); - let inline_hunk = inline + self.inline_hunk = self + .inline .iter() .map(|row| { let (old, new) = inline_row_linenos(row); - hunk_for_linenos(&file.hunks, old, new) + hunk_for_linenos(&self.hunks, old, new) }) .collect(); - Self { - old_text, - new_text, - old_lines, - new_lines, - display, - first_hunk_row, - first_inline_hunk_row, - old_hl, - new_hl, - word_spans: HashMap::new(), - inline, - inline_word_spans: HashMap::new(), - display_hunk, - inline_hunk, + self.word_spans.clear(); + self.inline_word_spans.clear(); + } + + /// Accumulate an expansion request for the gap keyed `key` (CS8's progressive reveal) and + /// rebuild the derived rows. `more_before`/`more_after` ADD to whatever was already revealed + /// at that edge (repeated `Enter` presses widen further); `full` is sticky — once set for this + /// gap it stays set. A `key` with no matching gap in the current `display` is harmless: the + /// entry simply sits unused in the map until a gap with that key exists again (it never will, + /// since keys are stable pre-collapse indices — this is just defensive, not reachable from + /// [`App::expand_gap_at_cursor`], which validates the cursor row first). + pub fn expand_gap(&mut self, key: usize, more_before: usize, more_after: usize, full: bool) { + let entry = self.expansions.entry(key).or_default(); + entry.before += more_before; + entry.after += more_after; + entry.full |= full; + self.rebuild_rows(); + } + + /// CS9's scope-reveal: widen the gap keyed `key` to uncover a tree-sitter scope range + /// `[scope_start, scope_end]` (1-based, inclusive — as returned by + /// [`crate::scope::enclosing_scope_lines`]) that encloses the gap's anchor line, in + /// `anchor_prefers_new`'s frame (new-side lineno when `true`, old-side when `false` — see + /// [`App::expand_gap_at_cursor`]'s anchor selection). Only the gap's TRAILING edge (`after`) + /// is ever widened: the anchor sits at the gap's following edge and `scope_start` is what + /// climbs upward from it toward the gap; `scope_end` falls among rows already visible after + /// the gap by construction (the anchor line is inside the scope), so the leading edge never + /// has anything new to reveal here. + /// + /// Returns `true` when this widened the gap (grew `after`, or revealed the whole run because + /// the scope covers it entirely); `false` when the scope added nothing new — either the gap + /// is already fully revealed/not a gap at all, or `scope_start` doesn't reach far enough + /// upward to uncover any currently-hidden row. The caller's signal to fall back to the flat + /// +10 reveal, so repeated presses always widen. + pub fn scope_expand_gap( + &mut self, + key: usize, + scope_start: usize, + anchor_prefers_new: bool, + ) -> bool { + let Some((hidden_start, hidden_end)) = + gap_hidden_range(&self.aligned, key, &self.expansions) + else { + return false; + }; + // Non-empty by construction: `gap_hidden_range` returns `None` (never an empty range) + // once an expansion covers the whole run — see its `effective_before + effective_after + // >= run_len` arm. + let hidden = &self.aligned[hidden_start..hidden_end]; + + let lineno_of = + |row: &AlignedRow| row_lineno(if anchor_prefers_new { row.new } else { row.old }); + // Context rows always carry a lineno on both sides (see the module doc's lineno + // invariant), and linenos increase monotonically through a run, so counting from the + // trailing edge backward while the scope still covers each row is safe. + let count = hidden + .iter() + .rev() + .take_while(|row| lineno_of(row).is_some_and(|n| n >= scope_start)) + .count(); + + if count == 0 { + return false; } + if count >= hidden.len() { + self.expand_gap(key, 0, 0, true); + } else { + self.expand_gap(key, 0, count, false); + } + true } /// The hunk (index into the file's `hunks`) whose span covers display row `row`, or `None` @@ -552,6 +663,29 @@ fn parse_outline_mode(raw: &str) -> Option { } } +/// Parse `workon.review.outline.order` (CS3) into an [`OutlineOrder`]. Canonical strings mirror +/// the variant names, kebab-cased: `head-first`, `base-first`. `None` on anything else — +/// [`App::apply_view_config`] falls back to [`OutlineOrder::default`] and warns. +fn parse_outline_order(raw: &str) -> Option { + match raw { + "head-first" => Some(OutlineOrder::HeadFirst), + "base-first" => Some(OutlineOrder::BaseFirst), + _ => None, + } +} + +/// Parse `workon.review.outline.icons` (CS5) into an [`OutlineIcons`]. Canonical strings mirror +/// the variant names, kebab-cased: `nerd`, `none`. `None` on anything else — +/// [`App::apply_view_config`] falls back to [`OutlineIcons::default`] (also `none` — CS5's +/// no-auto-detection default) and warns. +fn parse_outline_icons(raw: &str) -> Option { + match raw { + "nerd" => Some(OutlineIcons::Nerd), + "none" => Some(OutlineIcons::None), + _ => None, + } +} + /// Parse `workon.review.diff.layout` (CS7) into a [`Layout`]. Canonical strings mirror the /// variant names: `sbs`, `inline`. `None` on anything else — [`App::apply_view_config`] falls /// back to [`Layout::default`] and warns. @@ -576,6 +710,75 @@ fn parse_diff_zoom(raw: &str) -> Option { } } +/// CS4: which outline row a Header/Dir cursor selection resolves to — [`App::summary_target`]'s +/// return type, and the input [`App::summary_for`] consumes to build the renderable summary. +/// `render.rs`'s `render_summary` never matches on this directly — it only calls +/// `App::summary_for`/renders the [`Summary`] that comes back. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SummaryTarget { + /// The cursor rests on an [`OutlineItem::Header`] row — `cs_idx` is that row's true index + /// into [`App::changesets`]. + Changeset(usize), + /// The cursor rests on an [`OutlineItem::Dir`] row — `path` is that row's full path, `cs_idx` + /// its `cs_idx` (`Some` in [`OutlineMode::StackTree`], `None` in the cross-stack + /// [`OutlineMode::Tree`] — see that field's doc comment on [`OutlineItem::Dir`]). + Dir { cs_idx: Option, path: String }, +} + +/// CS4: the renderable summary [`App::summary_for`] builds for a [`SummaryTarget`] — a thin +/// wrapper so `render.rs` has one return type to match on regardless of which kind of row was +/// selected. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Summary { + Changeset(summary::ChangesetSummary), + Dir(summary::DirSummary), +} + +/// CS7: a stable identity for an outline File/Dir row, captured BEFORE a staging/discard op's +/// `coordinated_refresh` rebuilds [`App::outline_items`]'s row list, so the row can be re-found +/// (or gracefully lost, e.g. a fully-discarded file) afterward — see +/// [`App::restore_outline_position`]. `cs_idx`/`path` mirror the row's own fields, EXCEPT a +/// [`OutlineItem::File`]'s `path` here is always the FULL path (from the underlying +/// [`FileChange`]), never the Tree/StackTree leaf-only segment the row itself may display — two +/// rows in different directories can share a leaf name, so the leaf alone isn't a stable key. +/// [`OutlineItem::Dir`]'s own `path` field is already full regardless of mode, so it's reused +/// as-is. No [`OutlineItem::Header`] variant: a header row is never a staging/discard target (see +/// [`App::outline_row_targets`]). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum OutlineRowIdentity { + File { cs_idx: usize, path: String }, + Dir { cs_idx: Option, path: String }, +} + +impl OutlineRowIdentity { + /// Whether outline row `item` is the same row this identity was captured from. A + /// [`OutlineItem::File`]'s displayed `path` may be leaf-only (Tree/StackTree) — that's + /// resolved through [`App::outline_row_targets`]'s `(cs_idx, file_idx)` lookup instead of + /// comparing against the row's own `path` field. + fn matches_file(&self, item_cs_idx: usize, full_path: &str) -> bool { + matches!( + self, + OutlineRowIdentity::File { cs_idx, path } + if *cs_idx == item_cs_idx && path == full_path + ) + } + + /// Whether outline row `item` is the same row this identity was captured from. + fn matches_dir(&self, item: &OutlineItem) -> bool { + match (self, item) { + ( + OutlineRowIdentity::Dir { cs_idx, path }, + OutlineItem::Dir { + cs_idx: item_cs_idx, + path: item_path, + .. + }, + ) => cs_idx == item_cs_idx && path == item_path, + _ => false, + } + } +} + /// The outline side pane's own state (locked fork 3): whether it's showing, whether IT (rather /// than the diff) currently has keyboard focus, its own cursor (an index into /// [`App::outline_items`]'s row list — a wholly separate coordinate space from [`App::cursor`]), @@ -590,6 +793,17 @@ pub struct OutlineState { /// The outline pane's column width — `workon.review.outline.width` (CS7), defaulting to /// [`DEFAULT_OUTLINE_WIDTH`]. Read by `render.rs` in place of the old fixed const. pub width: u16, + /// Top-of-viewport row index into [`App::outline_items`]'s row list, derived from `cursor` + /// via the same scrolloff discipline as [`App::scroll`] (see [`App::derive_outline_scroll`]) — + /// never written directly. + pub scroll: usize, + /// Which end of the stack the stack-shaped modes display first — `workon.review.outline.order` + /// (CS3), defaulting to [`OutlineOrder::HeadFirst`]. Read by [`App::outline_items`]. + pub order: OutlineOrder, + /// CS5: opt-in nerd-font file/dir icons — `workon.review.outline.icons`, defaulting to + /// [`OutlineIcons::None`] (no auto-detection story exists — a terminal can't report the + /// user's font). Read by `render::build_outline_line`. + pub icons: OutlineIcons, } /// Which of a split's two panes has focus — the top pane renders the unstaged role, the bottom the @@ -611,6 +825,68 @@ struct PaneState { scroll: usize, } +/// CS6: a staging op's pre-op position, captured by [`App::capture_position`] before +/// `coordinated_refresh` and restored by [`App::restore_position`] after — so a staging op keeps +/// the reviewer's place instead of `reset_panes`' first-hunk reseat (that reseat still runs for +/// every MANUAL nav: file/changeset switches, zoom cycles). `path` + `role` say WHERE (the same +/// file, the pane the reviewer was in); `old_lineno`/`new_lineno` say WHAT (the acted-on row's +/// position in `role`'s own coordinate frame — the two sides a role's rows are diffed against, +/// per [`FileView::load`]'s table). Deliberately NO pre-op zoom snapshot: [`App::restore_position`] +/// re-derives the POST-op [`EffectiveZoom`] from live state, since the op itself is exactly what +/// invalidates a pre-op snapshot. +struct PositionMemento { + path: String, + role: Role, + old_lineno: Option, + new_lineno: Option, +} + +/// The target role's display row (active layout) whose lineno IN `new_frame`'s coordinate frame +/// (`true` = new side, `false` = old side — the frame the memento's target lineno was captured +/// in) is the first `>= target`. Rows with no lineno on that side — gaps, and the unpaired +/// Del/Add rows whose only lineno lives on the OTHER side — are skipped rather than compared: +/// old-side and new-side numbering diverge as soon as a file has any insertion or deletion above +/// the row, so mixing frames in one monotonic scan would let e.g. a deletion hunk's old-side +/// numbers (which run ahead of the surrounding new-side numbers) capture the cursor first. +/// +/// Falls back to the LAST row carrying a lineno in that frame when `target` is past the view's +/// end (staging the acted-on hunk can shrink the file out from under the old lineno). `None` +/// only when NO row carries a lineno in that frame (e.g. anchoring old-frame in an added-only +/// file) — the caller keeps `reset_panes`' first-hunk position then. +fn find_nearest_row( + view: &FileView, + layout: Layout, + target: usize, + new_frame: bool, +) -> Option { + let in_frame = |old: Option, new: Option| if new_frame { new } else { old }; + let linenos: Vec<(usize, usize)> = match layout { + Layout::Sbs => view + .display + .iter() + .enumerate() + .filter_map(|(i, row)| { + let (old, new) = display_row_linenos(row); + in_frame(old, new).map(|n| (i, n)) + }) + .collect(), + Layout::Inline => view + .inline + .iter() + .enumerate() + .filter_map(|(i, row)| { + let (old, new) = inline_row_linenos(row); + in_frame(old, new).map(|n| (i, n)) + }) + .collect(), + }; + linenos + .iter() + .find(|(_, n)| *n >= target) + .or_else(|| linenos.last()) + .map(|(i, _)| *i) +} + /// Slide `prev_scroll` the minimum amount to keep `cursor` within `[SCROLLOFF, pane_height - 1 - /// SCROLLOFF]` of the viewport, then clamp to `[0, rows - pane_height]` (edge wins over margin). /// The pure core of [`App::derive_scroll`], factored out so a split's unfocused pane can derive its @@ -815,6 +1091,9 @@ pub struct App { /// [`Self::pane_height`] — [`Self::derive_alt_scroll`] derives the unfocused pane's scroll /// against THIS, not the focused pane's height. pub(crate) alt_height: usize, + /// Content height of the outline pane, written by the renderer each frame — same discipline + /// as [`Self::pane_height`]. Read by [`Self::derive_outline_scroll`]. + pub outline_height: usize, /// Label for the old side of the diff, shown next to a rename's `old_path` in the header. /// M4 only reviews the uncommitted (`HEAD` ↔ worktree) diffs, so this is always `"HEAD"` /// today; M5's committed-changeset zoom will want the changeset's actual base rev. @@ -942,6 +1221,19 @@ pub enum PendingOp { file_idx: usize, selections: Vec<(usize, LineSelection)>, }, + /// CS7: discard every file in `files` — `(changeset name, file path)` pairs — from the + /// worktree: an outline File row's single target, or a Dir row's every file under its path. + /// Stored by NAME + PATH rather than raw `(cs_idx, file_idx)` indices because the confirm + /// modal doesn't stop the tick beat: an external index change (e.g. `git add` from another + /// terminal) can run a full refresh between `d` and `y`, rebuilding the per-changeset file + /// lists and shifting positions — [`App::resolve_confirm`] re-resolves each pair against the + /// LIVE changesets at answer time (silently skipping any that vanished) so a stale index can + /// never discard the wrong file. `identity` is the acted-on outline row's + /// [`OutlineRowIdentity`], captured at request-time for the post-op outline cursor restore. + DiscardOutlineFiles { + files: Vec<(String, String)>, + identity: OutlineRowIdentity, + }, } /// A pending destructive op plus the scope-stating prompt shown on the footer until answered. @@ -1008,12 +1300,18 @@ impl App { // "decided without interview" default — preserves the M4 full-width look for a lone // uncommitted changeset), unfocused (the diff keeps initial keyboard focus so the user // can start reading immediately), Stack mode (shows the structure M5 exists to surface). + // Under the pure open/closed toggle (`o`) this is now a consistent split: `o` controls + // visibility, `h`/[`App::focus_outline`] controls focus — so seeding open+unfocused here + // doesn't fight the toggle the way it did under the old three-state cycle. let outline = OutlineState { open: changesets.len() > 1, focused: false, cursor: 0, mode: OutlineMode::default(), width: DEFAULT_OUTLINE_WIDTH, + scroll: 0, + order: OutlineOrder::default(), + icons: OutlineIcons::default(), }; let mut refresh_coordinator = RefreshCoordinator::new(); // Seed the coordinator with the index signature as it stands right after this initial @@ -1038,6 +1336,7 @@ impl App { pane_height: 20, alt: PaneState::default(), alt_height: 20, + outline_height: 20, base_label, highlighter: TsHighlighter::new(), layout: Layout::default(), @@ -1589,6 +1888,12 @@ impl App { /// run on file open and zoom change. The two role coordinate spaces disagree, so carrying a /// raw cursor index across a role/zoom switch would be meaningless; jumping to the role's own /// first hunk (the same position a fresh file open lands on) is always valid and predictable. + /// + /// This is also what `coordinated_refresh` leaves behind after a staging op (via + /// `open_current`), since a refresh is itself a file "open" of the post-op state — CS6's + /// `App::restore_position` runs immediately after, overwriting this first-hunk reseat with + /// the reviewer's pre-op position when it can. Every OTHER caller (manual file/changeset + /// nav, zoom cycles) has no such follow-up, so first-hunk-on-open is still what they see. fn reset_panes(&mut self) { // Any file open / zoom change reshapes the coordinate space an active selection is keyed // in, so drop it (see [`Self::selection_anchor`]). @@ -2071,14 +2376,14 @@ impl App { // ── Outline side pane (CS3) ───────────────────────────────────────────────── - /// Snapshot every reviewed changeset into [`OutlineChangeset`]/[`OutlineFile`] and build the - /// current [`OutlineMode`]'s row list — the outline cursor's index space, and the source of - /// truth `render.rs` draws from. Rebuilt fresh on every call (cheap: a small stack times a - /// handful of files each, no caching, same posture as [`Self::effective_zoom_for`]) rather - /// than cached on `App`, so it's never stale across a mode toggle, a nav, or a refresh. - pub fn outline_items(&self) -> Vec { - let snapshot: Vec = self - .changesets + // ── Summary panel (CS4) ───────────────────────────────────────────────────── + + /// Snapshot every reviewed changeset into [`OutlineChangeset`]/[`OutlineFile`] — the input + /// [`Self::outline_items`] feeds `outline::build_items`, and CS4's [`Self::summary_for`] + /// feeds `outline::latest_by_path` for a [`OutlineMode::Tree`] directory's cross-stack + /// aggregate. Rebuilt fresh on every call, same posture as [`Self::outline_items`] itself. + fn outline_snapshot(&self) -> Vec { + self.changesets .iter() .map(|v| OutlineChangeset { label: v.cs.title.clone().unwrap_or_else(|| v.cs.name.clone()), @@ -2093,11 +2398,95 @@ impl App { .map(|(idx, f)| OutlineFile { path: f.path.clone(), status: v.staged_status(idx), + change: f.status, }) .collect(), }) - .collect(); - outline::build_items(&snapshot, self.outline.mode) + .collect() + } + + /// Build the current [`OutlineMode`]'s row list — the outline cursor's index space, and the + /// source of truth `render.rs` draws from. Rebuilt fresh on every call (cheap: a small stack + /// times a handful of files each, no caching, same posture as [`Self::effective_zoom_for`]) + /// rather than cached on `App`, so it's never stale across a mode toggle, a nav, or a + /// refresh. + pub fn outline_items(&self) -> Vec { + let snapshot = self.outline_snapshot(); + outline::build_items(&snapshot, self.outline.mode, self.outline.order) + } + + /// CS4: the outline row a Header/Dir cursor selection resolves to — `None` when the outline + /// isn't in a state where the diff area shows a summary instead of a file's diff (closed, + /// merely open-but-unfocused, or the cursor is on a File row). `render_body` branches on this + /// before any of its usual diff-body gates (pending/failed/binary/deferred-load). + pub fn summary_target(&self) -> Option { + if !self.outline.open || !self.outline.focused { + return None; + } + let items = self.outline_items(); + match items.get(self.outline.cursor)? { + OutlineItem::Header { cs_idx, .. } => Some(SummaryTarget::Changeset(*cs_idx)), + OutlineItem::Dir { path, cs_idx, .. } => Some(SummaryTarget::Dir { + cs_idx: *cs_idx, + path: path.clone(), + }), + OutlineItem::File { .. } => None, + } + } + + /// Build the renderable summary for `target` (see [`Self::summary_target`]) — + /// `render::render_summary`'s data source. + pub fn summary_for(&self, target: SummaryTarget) -> Summary { + match target { + SummaryTarget::Changeset(cs_idx) => { + let view = &self.changesets[cs_idx]; + let label = view + .cs + .title + .clone() + .unwrap_or_else(|| view.cs.name.clone()); + let failure_message = view.failure_message().map(|s| s.to_string()); + Summary::Changeset(summary::changeset_summary( + label, + view.cs.current, + view.cs.needs_restack, + view.is_pending(), + view.is_failed(), + failure_message, + view.files(), + )) + } + SummaryTarget::Dir { + cs_idx: Some(cs_idx), + path, + } => { + // StackTree mode: the dir row's trie belongs to exactly one changeset, so scope + // the aggregate to that changeset's own files (mirrors `build_stack_tree`'s "no + // cross-changeset dedup" rule). + let view = &self.changesets[cs_idx]; + Summary::Dir(summary::dir_summary( + path, + &view.files().iter().collect::>(), + )) + } + SummaryTarget::Dir { cs_idx: None, path } => { + // Tree mode: the dir row's trie spans the whole stack with no single owning + // changeset — aggregate over the same last-write-wins de-duped path set the Tree + // outline itself displays, reusing `outline::latest_by_path` rather than + // re-deriving the dedup rule here. `latest_by_path` returns a `HashMap`, whose + // iteration order is unspecified — sort by path so the panel's file list reads in + // the same alpha order the Tree outline itself paints (`emit`'s own sort). + let snapshot = self.outline_snapshot(); + let latest = outline::latest_by_path(&snapshot); + let mut entries: Vec<(&String, &outline::FileOccurrence)> = latest.iter().collect(); + entries.sort_by(|a, b| a.0.cmp(b.0)); + let files: Vec<&FileChange> = entries + .into_iter() + .filter_map(|(_, occ)| self.changesets[occ.cs_idx].files().get(occ.file_idx)) + .collect(); + Summary::Dir(summary::dir_summary(path, &files)) + } + } } pub fn outline_open(&self) -> bool { @@ -2112,6 +2501,12 @@ impl App { self.outline.cursor } + /// Top-of-viewport row index into [`Self::outline_items`]'s row list — see + /// [`Self::derive_outline_scroll`]. + pub fn outline_scroll(&self) -> usize { + self.outline.scroll + } + /// The outline pane's column width — `workon.review.outline.width` (CS7), or /// [`DEFAULT_OUTLINE_WIDTH`] if never set. Read by `render.rs` in place of the old fixed /// const. @@ -2123,37 +2518,57 @@ impl App { self.outline.mode } - /// `o`: a three-state cycle — closed -> open+focused -> open+unfocused (focus back on the - /// diff, pane stays visible) -> closed. Opening always grabs focus (per the locked design); - /// the middle -> closed transition ("o while the outline is open but the diff has focus - /// closes it") isn't explicitly specified in the plan but is the natural completion of the - /// cycle, kept simple rather than adding a separate "close" key. + /// Which end of the stack the outline displays first — `workon.review.outline.order` (CS3), + /// or [`OutlineOrder::default`] if never set. + pub fn outline_order(&self) -> OutlineOrder { + self.outline.order + } + + /// CS5: whether the outline renders nerd-font icons — `workon.review.outline.icons`, or + /// [`OutlineIcons::default`] (`None`) if never set. + pub fn outline_icons(&self) -> OutlineIcons { + self.outline.icons + } + + /// `o`: a pure show/hide toggle — closed -> open+focused (+[`Self::sync_outline_to_current`]), + /// open (regardless of focus) -> closed+diff-focused. Focus itself is now a separate concern + /// handled by [`Self::focus_outline`]/[`Self::focus_diff`] (`h`/`l`) — `o` only ever changes + /// visibility. The opening arm IS `focus_outline`'s closed-case behavior, so it delegates + /// there rather than restating it. pub fn toggle_outline(&mut self) { if !self.outline.open { - self.outline.open = true; - self.outline.focused = true; - self.sync_outline_to_current(); - } else if self.outline.focused { - self.outline.focused = false; + self.focus_outline(); } else { self.outline.open = false; + self.outline.focused = false; + } + } + + /// `h`/Esc-cascade target: focus the outline, opening it first if it's closed. Syncing the + /// cursor to the current diff position only happens on the closed -> open transition — if the + /// outline is already open, re-focusing it (e.g. `h` after a manual `j`/`k` outline move + /// followed by `l`) must not stomp a manually positioned cursor. + pub fn focus_outline(&mut self) { + if !self.outline.open { + self.outline.open = true; + self.sync_outline_to_current(); } + self.outline.focused = true; + } + + /// `l`/Enter: return focus to the diff. The outline stays open — this only ever changes + /// focus, never visibility (that's `o`/[`Self::toggle_outline`]'s job). + pub fn focus_diff(&mut self) { + self.outline.focused = false; } /// `?`: toggle the help overlay (CS3). A plain flip — the overlay always renders whatever /// view currently has keyboard focus (see `render::render_help_overlay`), so there is no - /// extra state to reposition here, unlike [`Self::toggle_outline`]'s three-state cycle. + /// extra state to reposition here, unlike [`Self::toggle_outline`]. pub fn toggle_help(&mut self) { self.help_visible = !self.help_visible; } - /// Return focus to the diff without closing the outline (`Esc` while the outline has focus — - /// `tui::update` routes it here instead of quitting, per the locked design's "Esc must still - /// not quit when the outline has focus"). - pub fn outline_unfocus(&mut self) { - self.outline.focused = false; - } - /// `i` while the outline has focus: cycle [`OutlineMode`], then reposition the cursor onto /// the row matching the current diff position in the NEW mode's row list (the row layout /// just changed shape, so the raw index would otherwise point at an unrelated row). @@ -2179,6 +2594,22 @@ impl App { self.outline.mode = mode; } + /// Set the outline stack order directly — the config-startup (CS3) counterpart there is no + /// interactive key for today. Same non-resync posture as [`Self::set_outline_mode`]: called + /// before the first [`Self::open_current`], so no [`Self::sync_outline_to_current`] call is + /// needed here either. + pub fn set_outline_order(&mut self, order: OutlineOrder) { + self.outline.order = order; + } + + /// Set the outline icons setting directly — the config-startup (CS5) counterpart; there is + /// no interactive key for this (icons are a static config choice, not something to toggle + /// mid-session). Same non-resync posture as [`Self::set_outline_mode`]/ + /// [`Self::set_outline_order`]. + pub fn set_outline_icons(&mut self, icons: OutlineIcons) { + self.outline.icons = icons; + } + /// Move the outline's own cursor by `delta` rows (`j`/`k` while the outline has focus), /// clamped into the current row list. Landing on a FILE row jumps the diff there /// immediately (outline -> diff, per the locked design); a HEADER/DIR row itself never @@ -2226,6 +2657,42 @@ impl App { idx += step; } } + self.derive_outline_scroll(items.len()); + } + + /// `g`/`G` while the outline has focus: jump the cursor straight to row `idx` (clamped into + /// the current row list), landing on it in one step — unlike [`Self::outline_move_by`], there + /// is NO burst back-scan here: a jump to a HEADER/DIR row simply doesn't move the diff (`g` + /// typically lands on the stack's first header), and a jump to a FILE row jumps the diff + /// straight there (`G` typically lands on the last file). Used by [`Self::outline_top`]/ + /// [`Self::outline_bottom`]. + fn outline_move_to(&mut self, idx: usize) { + let items = self.outline_items(); + if items.is_empty() { + self.outline.cursor = 0; + self.derive_outline_scroll(0); + return; + } + let idx = idx.min(items.len() - 1); + self.outline.cursor = idx; + if let OutlineItem::File { + cs_idx, file_idx, .. + } = &items[idx] + { + self.switch_changeset(*cs_idx, *file_idx); + } + self.derive_outline_scroll(items.len()); + } + + /// `g` while the outline has focus: jump the cursor to the first row. + pub fn outline_top(&mut self) { + self.outline_move_to(0); + } + + /// `G` while the outline has focus: jump the cursor to the last row. + pub fn outline_bottom(&mut self) { + let last = self.outline_items().len().saturating_sub(1); + self.outline_move_to(last); } /// `Enter` while the outline has focus: jump the diff to the row under the outline cursor (a @@ -2254,6 +2721,242 @@ impl App { self.outline.focused = false; } + // ── Outline staging (CS7) ─────────────────────────────────────────────────── + + /// Whether the changeset at `cs_idx` is a committed range rather than the uncommitted + /// worktree layer — the per-index counterpart to [`Self::is_committed`] (which only reads the + /// ACTIVE changeset). CS7's outline verbs need this because the acted-on row's changeset is + /// whichever one the outline cursor rests on, not necessarily the diff's current changeset. + fn is_committed_at(&self, cs_idx: usize) -> bool { + self.changesets.get(cs_idx).is_some_and(|view| { + matches!( + view.cs.span, + ChangesetSpan::Committed { .. } | ChangesetSpan::CommittedRoot { .. } + ) + }) + } + + /// Resolve the outline row at `idx` to its [`OutlineRowIdentity`] plus the `(cs_idx, + /// file_idx)` pairs an outline stage/discard verb applies to — `None` for a + /// [`OutlineItem::Header`] row (never a staging target) or an out-of-range `idx`. + /// + /// A [`OutlineItem::File`] row resolves to its own single target. A [`OutlineItem::Dir`] row + /// resolves to every file under its `path` (segment-boundary match, [`summary::path_is_under`] + /// — the same rule the summary panel's [`summary::dir_summary`] uses): scoped to that row's own + /// changeset in [`OutlineMode::StackTree`] (`cs_idx: Some`), or to the cross-stack + /// last-write-wins de-duped set [`outline::latest_by_path`] returns in [`OutlineMode::Tree`] + /// (`cs_idx: None`) — mirrors [`Self::summary_for`]'s own Dir-row branching. + fn outline_row_targets(&self, idx: usize) -> Option<(OutlineRowIdentity, Vec<(usize, usize)>)> { + let items = self.outline_items(); + match items.get(idx)? { + OutlineItem::Header { .. } => None, + OutlineItem::File { + cs_idx, file_idx, .. + } => { + let path = self + .changesets + .get(*cs_idx)? + .files() + .get(*file_idx)? + .path + .clone(); + Some(( + OutlineRowIdentity::File { + cs_idx: *cs_idx, + path, + }, + vec![(*cs_idx, *file_idx)], + )) + } + OutlineItem::Dir { path, cs_idx, .. } => { + let identity = OutlineRowIdentity::Dir { + cs_idx: *cs_idx, + path: path.clone(), + }; + let targets = match cs_idx { + Some(cs_idx) => self + .changesets + .get(*cs_idx)? + .files() + .iter() + .enumerate() + .filter(|(_, f)| summary::path_is_under(&f.path, path)) + .map(|(file_idx, _)| (*cs_idx, file_idx)) + .collect(), + None => { + let snapshot = self.outline_snapshot(); + let latest = outline::latest_by_path(&snapshot); + latest + .iter() + .filter(|(p, _)| summary::path_is_under(p, path)) + .map(|(_, occ)| (occ.cs_idx, occ.file_idx)) + .collect() + } + }; + Some((identity, targets)) + } + } + } + + /// Per-file verb selection by [`outline::StagedStatus`] — mirrors [`Self::verb_for_role`]'s + /// toggle direction (unstaged stages, staged unstages), but keyed off the FILE's own status + /// rather than a pane role, since a Dir row's files can each carry a different status. + /// [`outline::StagedStatus::None`] shouldn't normally occur on the uncommitted changeset's own + /// file (a changed file always has SOME status) — treated as a Stage attempt so the op surfaces + /// whatever git reports rather than silently refusing. + fn outline_target_verb(&self, cs_idx: usize, file_idx: usize) -> StageVerb { + match self.changesets[cs_idx].staged_status(file_idx) { + outline::StagedStatus::Staged => StageVerb::Unstage, + outline::StagedStatus::Unstaged + | outline::StagedStatus::Partial + | outline::StagedStatus::None => StageVerb::Stage, + } + } + + /// Footer refusal for an outline stage/discard verb — parallels [`Self::notify_combined_refusal`] + /// but for the two CS7-specific refusal reasons: `committed` (the row's changeset — or, for a + /// Dir row, at least one file under it — is a committed range, not the uncommitted worktree + /// layer) or not (the cursor sits on a [`OutlineItem::Header`] row, which is never a target). + fn notify_outline_refusal(&mut self, verb: &str, committed: bool) { + if committed { + self.notify( + format!("changeset is already committed — nothing to {verb}"), + Severity::Error, + ); + } else { + self.notify( + format!("select a file or directory to {verb}"), + Severity::Error, + ); + } + } + + /// The shared resolve-and-gate preamble of the outline staging verbs (`s`/`d`): resolve the + /// row under the outline cursor to its identity + targets, refusing (with `verb` naming the + /// action in the notice) on a Header row or when any target belongs to a committed changeset, + /// and bailing silently on an empty target list. One helper so the two verbs' gates can't + /// drift apart. + fn outline_verb_targets( + &mut self, + verb: &str, + ) -> Option<(OutlineRowIdentity, Vec<(usize, usize)>)> { + let Some((identity, targets)) = self.outline_row_targets(self.outline.cursor) else { + self.notify_outline_refusal(verb, false); + return None; + }; + if targets + .iter() + .any(|&(cs_idx, _)| self.is_committed_at(cs_idx)) + { + self.notify_outline_refusal(verb, true); + return None; + } + if targets.is_empty() { + return None; + } + Some((identity, targets)) + } + + /// `s` while the outline has focus: stage or unstage the file/directory under the cursor. A + /// [`OutlineItem::File`] row stages or unstages per its own [`Self::outline_target_verb`]; a + /// [`OutlineItem::Dir`] row applies the same per-file verb selection to every file under it + /// (each file stages or unstages independently — a mixed-status directory is not an all-stage + /// or all-unstage op). Refuses on a [`OutlineItem::Header`] row or when any target belongs to + /// a committed changeset (see [`Self::notify_outline_refusal`]). + pub fn outline_stage(&mut self) { + let Some((identity, targets)) = self.outline_verb_targets("stage") else { + return; + }; + let ops: Vec> = targets + .iter() + .filter_map(|&(cs_idx, file_idx)| { + let file = self.changesets.get(cs_idx)?.files().get(file_idx)?.clone(); + let verb = self.outline_target_verb(cs_idx, file_idx); + Some(Box::new(FileStagingOp::file(file, verb)) as Box) + }) + .collect(); + self.outline_run_ops(ops, identity); + } + + /// `d` while the outline has focus: request confirmation to discard the file/directory under + /// the cursor from the worktree — a [`OutlineItem::File`] row discards just that file; a + /// [`OutlineItem::Dir`] row discards every file under it, and the confirm prompt names the + /// scope. Same refusal gates as [`Self::outline_stage`]. The discard itself runs when the user + /// answers `y` (see [`Self::resolve_confirm`]'s [`PendingOp::DiscardOutlineFiles`] arm). + pub fn outline_discard(&mut self) { + let Some((identity, targets)) = self.outline_verb_targets("discard") else { + return; + }; + let prompt = match &identity { + OutlineRowIdentity::File { path, .. } => { + format!("Discard all changes to `{path}`? (y/n)") + } + OutlineRowIdentity::Dir { path, .. } => format!( + "Discard changes to {} files under {path}/? (y/n)", + targets.len() + ), + }; + let files: Vec<(String, String)> = targets + .iter() + .filter_map(|&(cs_idx, file_idx)| { + let view = self.changesets.get(cs_idx)?; + let path = view.files().get(file_idx)?.path.clone(); + Some((view.cs.name.clone(), path)) + }) + .collect(); + self.request_confirm(prompt, PendingOp::DiscardOutlineFiles { files, identity }); + } + + /// The outline-facing counterpart to [`Self::run_op`]: drain `ops` through [`Self::run_ops`], + /// then restore the OUTLINE cursor to (or nearest to) `identity`'s row rather + /// than a diff-pane position (CS6's [`PositionMemento`]/[`Self::restore_position`] only make + /// sense when the diff pane, not the outline, was the focused surface the op started from). + /// [`Self::coordinated_refresh`] (inside `run_ops`) itself calls `sync_outline_to_current`, + /// which can leave the outline cursor on a wholly unrelated row (wherever the DIFF's current + /// file happens to be) — this runs after that and overwrites it with the acted-on row's own + /// position, or the nearest surviving row if it's gone (e.g. a fully-discarded file). + fn outline_run_ops(&mut self, ops: Vec>, identity: OutlineRowIdentity) { + let pre_op_cursor = self.outline.cursor; + // Restore after BOTH outcomes: `run_ops` refreshes (and thereby yanks the outline cursor + // via `sync_outline_to_current`) even on a partial failure, and the acted-on row is where + // the user is looking either way. + let _ = self.run_ops(ops); + self.restore_outline_position(&identity, pre_op_cursor); + } + + /// Re-find `identity`'s row in the freshly rebuilt [`Self::outline_items`] and reseat + /// [`OutlineState::cursor`] there; clamps into bounds instead when the row is gone (a fully + /// discarded file drops out of the combined diff — and with it its row — entirely). Does not + /// touch [`OutlineState::focused`] — an outline-initiated op + /// only ever runs while the outline already has focus, and nothing here changes that. + fn restore_outline_position(&mut self, identity: &OutlineRowIdentity, pre_op_cursor: usize) { + let items = self.outline_items(); + let found = items.iter().position(|item| match item { + OutlineItem::File { + cs_idx, file_idx, .. + } => { + let full_path = self + .changesets + .get(*cs_idx) + .and_then(|v| v.files().get(*file_idx)) + .map(|f| f.path.as_str()); + full_path.is_some_and(|p| identity.matches_file(*cs_idx, p)) + } + OutlineItem::Dir { .. } => identity.matches_dir(item), + OutlineItem::Header { .. } => false, + }); + match found { + Some(idx) => self.outline.cursor = idx, + // Row gone (the NORMAL outcome of a successful discard — the file left the combined + // diff and took its row with it): stay near where the user was ACTING, not wherever + // the refresh's `sync_outline_to_current` just parked the cursor (the diff's current + // file, unrelated to the acted-on row). `pre_op_cursor` is the acted-on row's own + // pre-op position; clamping it lands on the nearest surviving neighbor. + None => self.outline.cursor = pre_op_cursor.min(items.len().saturating_sub(1)), + } + self.derive_outline_scroll(items.len()); + } + /// Reposition (never rebuild/refocus) the outline cursor onto the row matching the CURRENT /// diff changeset+file, or clamp it into bounds if no such row exists (e.g. Flat mode /// deduped the current file's changeset out of the list). The sync-follow discipline's echo @@ -2273,6 +2976,7 @@ impl App { let items = self.outline_items(); if items.is_empty() { self.outline.cursor = 0; + self.derive_outline_scroll(0); return; } if let Some(idx) = items.iter().position(|it| { @@ -2286,6 +2990,7 @@ impl App { } else { self.outline.cursor = self.outline.cursor.min(items.len() - 1); } + self.derive_outline_scroll(items.len()); } /// Row count of file `idx`'s `role` view in the active layout's space (0 if absent/unloaded). @@ -2342,6 +3047,22 @@ impl App { derive_scroll_value(self.alt.cursor, self.alt.scroll, rows, self.alt_height); } + /// Re-derive the outline pane's `scroll` from its `cursor` — the outline's counterpart to + /// [`Self::derive_scroll`], reusing the same [`derive_scroll_value`] core against + /// [`Self::outline_height`]. Called after every outline-cursor mutation (mirroring how every + /// diff-cursor mutator ends with `derive_scroll`); the renderer also re-derives each frame, + /// which covers resizes. Takes the outline row count from the caller — every call site has + /// just built (or is about to paint from) [`Self::outline_items`], and rebuilding the whole + /// snapshot here again just for `.len()` would double the work on every keypress and frame. + pub(crate) fn derive_outline_scroll(&mut self, rows: usize) { + self.outline.scroll = derive_scroll_value( + self.outline.cursor, + self.outline.scroll, + rows, + self.outline_height, + ); + } + /// The `(scroll, cursor)` a split pane renders with: the focused pane contributes its own /// `scroll` and `Some(cursor)` (so the cursor highlight draws there); the unfocused pane /// contributes its stashed scroll and `None` (no highlight). Combined resolves to the focused @@ -2425,6 +3146,78 @@ impl App { } } + /// Reveal more of the collapsed gap under the cursor (`Enter`), or the WHOLE gap (`E`, when + /// `full`) — CS8's progressive unfold, extended by CS9 with a two-tier `Enter`: A silent + /// no-op when the cursor isn't on a `Gap` row (or there's no loaded view): unlike a staging + /// refusal this isn't a mode error worth interrupting the user over, same precedent as + /// [`Self::next_hunk_row`] finding no later hunk. + /// + /// - `full` (`E`): unchanged from CS8 — always the flat full-run reveal via + /// [`FileView::expand_gap`], regardless of grammar. + /// - `!full` (`Enter`, CS9): FIRST tries a tree-sitter scope-reveal — + /// [`gap_scope_start`] resolves the gap's anchor (the following row's new-side lineno, + /// preferring new like CS6's [`Self::restore_position`], old-side for delete-only files) + /// to the smallest enclosing [`crate::scope`] node, and [`FileView::scope_expand_gap`] + /// widens the gap's trailing edge to uncover it. Falls back to the flat +10/+10 reveal + /// (same as CS8) when: the file's extension has no bundled grammar, no allowlisted + /// ancestor encloses the anchor, or the scope reveals nothing new (already fully visible) + /// — so repeated `Enter` presses always widen the gap, uniformly. + /// + /// `self.cursor`'s INDEX is left untouched either way. Rows revealed at the gap's leading + /// edge insert immediately before the gap's own row (shifting the gap marker — and + /// everything after it — down), so after [`FileView::rebuild_rows`] the row now sitting at + /// the old index is the first newly revealed line rather than the gap marker itself: the + /// cursor visually lands on the start of the revealed region without this method needing to + /// compute a new index. The scope-reveal path only ever widens the TRAILING edge (see + /// [`FileView::scope_expand_gap`]'s doc for why), so this holds there too. + pub fn expand_gap_at_cursor(&mut self, full: bool) { + let cursor = self.cursor; + let layout = self.layout; + // Read out before taking `current_view()`'s exclusive borrow — `gap_scope_start` only + // needs the path strings, not the file, so cloning two short `String`s here avoids a + // `self.cur()`/`self.current_view()` borrow conflict for the whole rest of the method. + let anchor_paths = self.cur().diff.files.get(self.current).map(|f| { + let new_path = f.path.clone(); + let old_path = f.old_path.clone().unwrap_or_else(|| f.path.clone()); + (new_path, old_path) + }); + let Some(view) = self.current_view() else { + return; + }; + let key = match layout { + Layout::Sbs => match view.display.get(cursor) { + Some(DisplayRow::Gap { key, .. }) => *key, + _ => return, + }, + Layout::Inline => match view.inline.get(cursor) { + Some(InlineRow::Gap { key, .. }) => *key, + _ => return, + }, + }; + + let scope_revealed = !full + && anchor_paths + .as_ref() + .and_then(|(new_path, old_path)| { + gap_scope_start(view, layout, cursor, new_path, old_path) + }) + .is_some_and(|(scope_start, anchor_prefers_new)| { + view.scope_expand_gap(key, scope_start, anchor_prefers_new) + }); + + if !scope_revealed { + view.expand_gap(key, 10, 10, full); + } + // The expansion just reshaped the focused pane's row space — whichever tier did it — + // so cancel any active selection rather than translating it, per `selection_anchor`'s + // invariant (same rule as layout toggles, zoom changes, file switches, and split-focus + // swaps). Only reached when a gap actually expanded; the non-gap no-op above leaves a + // selection alone. + self.cancel_selection(); + self.derive_scroll(); + self.clamp_cursor(); + } + /// Toggle between side-by-side and inline layouts (`L`). Deliberately does not try to /// re-derive an exactly equivalent `cursor` position for the new layout — the two layouts' /// row vectors track the same underlying content in a different shape, and translating @@ -2510,6 +3303,28 @@ impl App { }; self.set_outline_mode(mode); + let order = match &raw.outline_order { + Some(o) => parse_outline_order(o).unwrap_or_else(|| { + warnings.push(format!( + "workon.review.outline.order = '{o}' unrecognized; using default" + )); + OutlineOrder::default() + }), + None => OutlineOrder::default(), + }; + self.set_outline_order(order); + + let icons = match &raw.outline_icons { + Some(i) => parse_outline_icons(i).unwrap_or_else(|| { + warnings.push(format!( + "workon.review.outline.icons = '{i}' unrecognized; using default" + )); + OutlineIcons::default() + }), + None => OutlineIcons::default(), + }; + self.set_outline_icons(icons); + let layout = match &raw.diff_layout { Some(l) => parse_diff_layout(l).unwrap_or_else(|| { warnings.push(format!( @@ -2762,21 +3577,62 @@ impl App { }; self.run_op(LineSelectionOp::new(file, selections, StageVerb::Discard)); } + PendingOp::DiscardOutlineFiles { files, identity } => { + // Re-resolve each (changeset name, path) pair against the LIVE changesets — an + // intervening tick refresh may have shifted every index since `d` was pressed + // (see the variant's doc); a pair that no longer resolves is silently skipped + // (its file already left the diff, so there's nothing left to discard). + let ops: Vec> = files + .iter() + .filter_map(|(cs_name, path)| { + let view = self.changesets.iter().find(|v| v.cs.name == *cs_name)?; + let file = view.files().iter().find(|f| f.path == *path)?.clone(); + Some(Box::new(FileStagingOp::file(file, StageVerb::Discard)) + as Box) + }) + .collect(); + self.outline_run_ops(ops, identity); + } } } /// Enqueue `op`, drain the queue on the same beat, then act on the outcome: a failure or panic - /// surfaces on the footer and skips the refresh (the index is now in whatever partial state - /// the failed op left it in — the user resolves with `r`); a `Completed` drain refreshes, - /// rebuilding the views + attribution from the new index (locked decision #5). + /// surfaces on the footer (and the views still refresh — see [`Self::run_ops`] for why); a + /// `Completed` drain refreshes, rebuilding the views + attribution from the new index (locked + /// decision #5), then restores the reviewer's pre-op DIFF position (CS6) — a staging op is + /// the ONE nav path that does not reset to the role's first hunk; every manual nav still + /// does, via `reset_panes` unchanged. /// - /// Generic over any [`StagingOp`] — a hunk/file op ([`FileStagingOp`]) or a (possibly - /// multi-hunk) line selection ([`LineSelectionOp`], which applies as ONE merged patch rather - /// than enqueueing one op per hunk — see that type's docs for why splitting is wrong). Either - /// way exactly one op is ever in flight, so the queue's trap-4 live-index staleness doesn't - /// apply — the queue is here for its lock-retry and panic isolation. + /// A thin diff-facing wrapper over [`Self::run_ops`] (one op, one memento) — the diff pane's + /// staging verbs (`s`/`S`/`d`/`D`) are the only callers, so the shared drain/refresh core + /// lives on `run_ops` and this just supplies the diff-position memento CS7's outline verbs + /// don't want (see [`Self::outline_run_ops`], which restores the OUTLINE cursor instead). fn run_op(&mut self, op: impl StagingOp + 'static) { - self.queue.enqueue(op); + let memento = self.capture_position(); + if self.run_ops(vec![Box::new(op)]).is_ok() { + if let Some(memento) = memento { + self.restore_position(memento); + } + } + } + + /// Enqueue every op in `ops`, drain the queue on the same beat, then run a + /// [`Self::coordinated_refresh`] REGARDLESS of outcome — the drain never stops on a failure, + /// so a partial multi-op batch has already mutated the index/worktree and the views must + /// re-read that reality even while a failure notice shows. Returns `Err` after notifying the + /// first failure/panic, `Ok(())` otherwise. Callers own what happens next (a diff-position or + /// outline-cursor restore, or nothing) — this only owns the queue mechanics. + /// + /// Generic over any [`StagingOp`] — a hunk/file op ([`FileStagingOp`]), a (possibly + /// multi-hunk) line selection ([`LineSelectionOp`], which applies as ONE merged patch rather + /// than enqueueing one op per hunk — see that type's docs for why splitting is wrong), or + /// (CS7) several independent whole-file ops from an outline Dir row. The queue's trap-4 + /// live-index staleness doesn't apply here: every op resolves its own direction from the live + /// index inside `run` (see `queue.rs`'s module doc), so draining several back-to-back is safe. + fn run_ops(&mut self, ops: Vec>) -> Result<(), ()> { + for op in ops { + self.queue.enqueue(op); + } // Distinct fields (`queue` mutable, `repo`/`applier` shared) — the borrow checker permits // the disjoint borrows in one call, so the queue needn't be taken out and put back. let outcomes = self.queue.drain(&self.repo, &self.applier); @@ -2785,10 +3641,107 @@ impl App { OpOutcome::Panicked(_) => Some("staging operation panicked".to_string()), OpOutcome::Completed(_) => None, }); + // Refresh in BOTH arms: the queue's drain never stops on a failure (`pump` runs every + // queued op regardless), so in a multi-op batch a single failure still leaves up to N-1 + // other ops applied to the index/worktree — the views must re-read that reality even + // while the failure notice shows. (For a single-op batch the refresh is a harmless + // re-read of unchanged state.) + self.coordinated_refresh(); match failure { - Some(message) => self.notify(message, Severity::Error), - None => self.coordinated_refresh(), + Some(message) => { + self.notify(message, Severity::Error); + Err(()) + } + None => Ok(()), + } + } + + /// Snapshot the focused pane's file/role/position ahead of a staging op, for + /// [`Self::restore_position`] to reseat after the op's `coordinated_refresh` (CS6). `None` + /// when there's no current file, the current view is the combined role (never a staging + /// target — [`Self::staging_role`]), or the focused role's view isn't loaded; restore is then + /// a no-op and today's `reset_panes` first-hunk behavior stands. + fn capture_position(&self) -> Option { + let path = self.files().get(self.current)?.path.clone(); + let role = self.staging_role()?; + let view = self.role_view_ref(self.current, role)?; + // Reuse the same row -> lineno extraction `FileView::load` builds its hunk maps from + // (a Gap row yields (None, None), which restore treats as nothing-to-search-for). + let (old_lineno, new_lineno) = match self.layout { + Layout::Sbs => view + .display + .get(self.cursor) + .map(display_row_linenos) + .unwrap_or((None, None)), + Layout::Inline => view + .inline + .get(self.cursor) + .map(inline_row_linenos) + .unwrap_or((None, None)), + }; + Some(PositionMemento { + path, + role, + old_lineno, + new_lineno, + }) + } + + /// Reseat the focused pane to a pre-staging-op position after `coordinated_refresh` rebuilds + /// the views (CS6) — the staging-path counterpart to `reset_panes`' first-hunk reseat, which + /// this deliberately leaves untouched for every manual nav (file/changeset switch, zoom + /// cycle). Falls back to whatever `reset_panes` already produced (today's first-hunk + /// behavior) when the acted-on file's path is gone (fully discarded) or its memento carried + /// no lineno at all (the cursor sat on a `Gap` row pre-op — nothing to search for). + fn restore_position(&mut self, m: PositionMemento) { + if self.files().get(self.current).map(|f| f.path.as_str()) != Some(m.path.as_str()) { + return; + } + // Force the load `reset_panes` may have deferred so the view below actually exists. + self.complete_pending_open(); + + // Target role: a still-`Split` file keeps both panes, so stay on the memento's own role + // (locked decision: same file, same pane, unless that pane's role is now gone). A + // collapsed-to-`Single` file has exactly one surviving role — THAT is the target + // regardless of which pane the op started in, which is what lands "fully staging a file + // in Split" in the staged pane of the same file. + let target_role = match self.effective_zoom_for(self.current) { + EffectiveZoom::Split => m.role, + EffectiveZoom::Single(role) => role, + }; + if matches!(self.effective_zoom_for(self.current), EffectiveZoom::Split) + && self.split_focus_role() != target_role + { + // Never assign `split_focus` directly — this swaps the cursor/scroll/pane-height + // stashes along with it. + self.toggle_split_focus(); } + + let Some(view) = self.role_view_ref(self.current, target_role) else { + return; + }; + + // The memento's linenos were captured in `m.role`'s own frame (new = worktree for + // Unstaged/Combined, new = index for Staged — see `FileView::load`'s table). Preferring + // new over old is correct BOTH when the role is unchanged (the common case: same pane, + // same frame) AND on the one role change that can happen here — unstaged -> staged after + // fully staging a file in Split. In that case the staged view's new side (index) now + // holds exactly what the unstaged view's new side (worktree) held a moment ago, because + // staging made index == worktree for this file; so new -> new is still the right + // mapping. Whichever side supplies the target, the SEARCH stays in that same frame — + // `find_nearest_row` never falls back across sides (see its doc for why mixing frames + // mis-lands the cursor). + let (target_lineno, new_frame) = match (m.new_lineno, m.old_lineno) { + (Some(n), _) => (n, true), + (None, Some(o)) => (o, false), + (None, None) => return, + }; + let Some(cursor) = find_nearest_row(view, self.layout, target_lineno, new_frame) else { + return; + }; + self.cursor = cursor; + self.clamp_cursor(); + self.derive_scroll(); } /// Start a line selection anchored at the current cursor (`v`). Refuses (a notice, no anchor @@ -3302,6 +4255,43 @@ fn display_row_linenos(row: &DisplayRow) -> (Option, Option) { } } +/// CS9's tree-sitter scope-reveal inputs for the gap at `gap_cursor`: the anchor line and which +/// side it's in (`true` = new, `false` = old), resolved from the row immediately FOLLOWING the +/// gap in `layout`'s row vector — the plan's rationale: the next hunk is what you're reading +/// toward, so its enclosing scope is what's worth revealing. Prefers the new-side lineno when +/// present, falling back to old (CS6's [`App::restore_position`] convention) for the rows a +/// delete-only file's `Filler` new side never populates. +/// +/// Returns `None` when: there's no row after the gap (a trailing gap with nothing beyond it to +/// anchor on), the anchor path's extension has no bundled grammar, or +/// [`enclosing_scope_lines`] finds no enclosing scope for the anchor line — every case the +/// caller treats identically, falling back to the flat +10/+10 reveal. +fn gap_scope_start( + view: &FileView, + layout: Layout, + gap_cursor: usize, + new_path: &str, + old_path: &str, +) -> Option<(usize, bool)> { + let (old, new) = match layout { + Layout::Sbs => display_row_linenos(view.display.get(gap_cursor + 1)?), + Layout::Inline => inline_row_linenos(view.inline.get(gap_cursor + 1)?), + }; + + let (anchor_line, anchor_prefers_new, text, lang_path) = match new { + Some(n) => (n, true, view.new_text(), new_path), + None => (old?, false, view.old_text(), old_path), + }; + + let ext = Path::new(lang_path) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); + let lang_key = lang_key_for_ext(ext)?; + let (scope_start, _scope_end) = enclosing_scope_lines(lang_key, text, anchor_line)?; + Some((scope_start, anchor_prefers_new)) +} + /// Inline-coordinate analog of [`display_row_linenos`]. fn inline_row_linenos(row: &InlineRow) -> (Option, Option) { match *row { @@ -3384,12 +4374,14 @@ mod tests { use super::test_support::app_from_fixture; use super::{ build_file_views, find_next_hunk_row, find_prev_hunk_row, App, ChangesetView, DiffState, - EffectiveZoom, Layout, LoadedViews, Role, Severity, Zoom, DEFAULT_OUTLINE_WIDTH, + EffectiveZoom, Layout, LoadedViews, Role, Severity, Summary, SummaryTarget, Zoom, + DEFAULT_OUTLINE_WIDTH, }; use crate::align::{AlignedRow, CellKind, DisplayRow, InlineRow, Row}; use crate::config::ReviewConfig; + use crate::icons::OutlineIcons; use crate::model::FileStatus; - use crate::outline::{OutlineItem, OutlineMode, StagedStatus}; + use crate::outline::{OutlineItem, OutlineMode, OutlineOrder, StagedStatus}; #[test] fn combined_files_arrive_path_sorted() { @@ -3933,7 +4925,7 @@ mod tests { } fn gap_row(skipped: usize) -> DisplayRow { - DisplayRow::Gap { skipped } + DisplayRow::Gap { key: 0, skipped } } #[test] @@ -5601,6 +6593,252 @@ mod tests { repo.assert(predicate::repo::has_untracked_file("new.txt")); } + // ---- CS6: staging preserves diff position ---------------------------------------------- + + /// Three single-line edits well-separated (>6 lines of pure context apart, git's own + /// hunk-splitting threshold) so each is its own hunk AND the context between any two + /// collapses to a [`DisplayRow::Gap`] — exercising both the mid-file-hunk and the + /// lands-in-a-gap restore paths. + fn three_hunk_fixture() -> Fixture { + let head: String = (1..=24).map(|n| format!("L{n}\n")).collect(); + let worktree: String = (1..=24) + .map(|n| { + if n == 2 || n == 12 || n == 22 { + format!("L{n}X\n") + } else { + format!("L{n}\n") + } + }) + .collect(); + FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("f.txt", &head, &worktree) + .build() + .unwrap() + } + + /// The row-native lineno `App::restore_position` would target for `row` — new side, + /// falling back to old — used by these tests to check where the cursor actually landed + /// without re-deriving the production search itself. + fn row_lineno(row: &DisplayRow) -> Option { + match row { + DisplayRow::Row(r) => match r.new { + Row::Line(n) => Some(n), + Row::Filler => match r.old { + Row::Line(n) => Some(n), + Row::Filler => None, + }, + }, + DisplayRow::Gap { .. } => None, + } + } + + #[test] + fn stage_hunk_on_a_middle_hunk_lands_the_cursor_near_it_not_at_the_first_hunk() { + let fixture = three_hunk_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); // Single(Unstaged): no staged half exists yet. + let first_hunk_row = app.cursor; + + app.next_hunk_row(); // hunk 1 (line 2) -> hunk 2 (line 12) + let hunk2_row = app.cursor; + assert_ne!( + hunk2_row, first_hunk_row, + "test setup: must have moved off hunk 1" + ); + + app.stage_hunk(); // stages ONLY hunk 2 -> the file now has both sub-diffs again + + assert!(app.notice.is_none(), "stage must succeed: {:?}", app.notice); + assert_eq!( + app.effective_zoom_for(app.current), + EffectiveZoom::Split, + "hunks 1/3 stayed unstaged, hunk 2 is now staged — both halves exist" + ); + assert_eq!( + app.split_focus_role(), + Role::Unstaged, + "the memento's own role (Unstaged) survives, so it stays the target" + ); + assert_ne!( + app.cursor, first_hunk_row, + "must NOT reset to the first hunk (today's manual-nav-only behavior)" + ); + + let view = app.role_view_ref(app.current, Role::Unstaged).unwrap(); + let lineno = row_lineno(&view.display[app.cursor]) + .expect("restore must not land the cursor back on a Gap row"); + assert!( + lineno > 2 && lineno < 22, + "expected the cursor between hunk 1 (line 2) and hunk 3 (line 22) — near hunk 2's \ + old position (line 12) — got line {lineno}" + ); + } + + #[test] + fn fully_staging_a_file_in_split_lands_the_cursor_in_the_staged_pane_at_the_same_lines() { + let fixture = partial_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); // Split; focused pane defaults to Unstaged, on gamma's hunk (line 3) + assert_eq!(app.effective_zoom_for(app.current), EffectiveZoom::Split); + assert_eq!(app.split_focus_role(), Role::Unstaged); + + app.stage_hunk(); // stages the only unstaged hunk -> the file is now fully staged + + assert!(app.notice.is_none(), "stage must succeed: {:?}", app.notice); + assert_eq!( + app.effective_zoom_for(app.current), + EffectiveZoom::Single(Role::Staged), + "no unstaged half survives a full stage" + ); + + let view = app.role_view_ref(app.current, Role::Staged).unwrap(); + let staged_first_hunk_row = match app.layout { + Layout::Sbs => view.first_hunk_row, + Layout::Inline => view.first_inline_hunk_row, + }; + assert_ne!( + app.cursor, staged_first_hunk_row, + "must land on gamma's own row, not beta's (the staged view's first hunk)" + ); + let lineno = row_lineno(&view.display[app.cursor]).expect("gamma's row has a lineno"); + assert_eq!( + lineno, 3, + "gamma is line 3 in both HEAD and the fully-staged index" + ); + } + + #[test] + fn unstaging_in_the_staged_pane_keeps_focus_there_when_it_survives() { + let head: String = (1..=14).map(|n| format!("L{n}\n")).collect(); + // Index stages two well-separated edits (lines 2 and 10); the worktree matches the + // index except for one MORE edit (line 14) that was never staged. + let index: String = (1..=14) + .map(|n| { + if n == 2 || n == 10 { + format!("L{n}X\n") + } else { + format!("L{n}\n") + } + }) + .collect(); + let worktree: String = (1..=14) + .map(|n| { + if n == 2 || n == 10 || n == 14 { + format!("L{n}X\n") + } else { + format!("L{n}\n") + } + }) + .collect(); + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .partially_staged_file("f.txt", &head, &index, &worktree) + .build() + .unwrap(); + + let mut app = app_from_fixture(&fixture); + app.open_current(); // Split; focused pane defaults to Unstaged (line 14's hunk) + app.toggle_split_focus(); // -> Staged pane, cursor on hunk 1 (line 2) + let first_hunk_row = app.cursor; + app.next_hunk_row(); // -> hunk 2 (line 10) + assert_ne!( + app.cursor, first_hunk_row, + "test setup: must have moved off hunk 1" + ); + + app.stage_hunk(); // staged pane -> unstage direction: reverts line 10's index entry + + assert!( + app.notice.is_none(), + "unstage must succeed: {:?}", + app.notice + ); + assert_eq!( + app.effective_zoom_for(app.current), + EffectiveZoom::Split, + "line 2 stays staged and line 10/14 are both unstaged now — both halves survive" + ); + assert_eq!( + app.split_focus_role(), + Role::Staged, + "the memento's own role (Staged) survives, so focus stays there" + ); + + let view = app.role_view_ref(app.current, Role::Staged).unwrap(); + let staged_first_hunk_row = match app.layout { + Layout::Sbs => view.first_hunk_row, + Layout::Inline => view.first_inline_hunk_row, + }; + assert_ne!( + app.cursor, staged_first_hunk_row, + "must NOT reset to the (now sole) first hunk at line 2" + ); + } + + #[test] + fn discarding_the_only_file_in_the_changeset_falls_back_gracefully_without_panicking() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .untracked_file("only.txt", "hello\nworld\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + assert_eq!(app.files().len(), 1); + + app.discard_file(); + assert!(app.pending_confirm.is_some()); + app.resolve_confirm(true); // runs the discard through run_op -> restore_position + + assert!( + app.notice.is_none(), + "discard must succeed: {:?}", + app.notice + ); + assert!( + app.files().is_empty(), + "the untracked file's only diff vanishes once discarded" + ); + assert_eq!( + app.cursor, 0, + "the path check bails out; reset_panes' fallback stands" + ); + } + + #[test] + fn staging_with_the_cursor_on_a_gap_row_falls_back_without_panicking() { + let fixture = three_hunk_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = { + let view = app.role_view_ref(app.current, Role::Unstaged).unwrap(); + view.display + .iter() + .position(|r| matches!(r, DisplayRow::Gap { .. })) + .expect("three well-separated hunks must collapse a gap between them") + }; + app.cursor = gap_row; + + app.stage_file(); // whole-file op: ignores the cursor for WHAT it stages + + assert!(app.notice.is_none(), "stage must succeed: {:?}", app.notice); + assert_eq!( + app.effective_zoom_for(app.current), + EffectiveZoom::Single(Role::Staged), + "no unstaged half survives a whole-file stage" + ); + // The pre-op cursor sat on a Gap row, so the memento carried no lineno — restore is a + // no-op and today's `reset_panes` first-hunk reseat stands. + let view = app.role_view_ref(app.current, Role::Staged).unwrap(); + let expected = match app.layout { + Layout::Sbs => view.first_hunk_row, + Layout::Inline => view.first_inline_hunk_row, + }; + assert_eq!(app.cursor, expected); + } + // ---- M4 staging: discard confirm flow -------------------------------------------------- #[test] @@ -6726,7 +7964,7 @@ mod tests { } #[test] - fn toggle_outline_cycles_closed_open_focused_open_unfocused_closed() { + fn toggle_outline_is_a_pure_show_hide_toggle() { let mut app = two_committed_changesets_two_and_one_files(); // Force a known starting state regardless of the default. while app.outline_open() { @@ -6737,19 +7975,88 @@ mod tests { app.toggle_outline(); assert!( app.outline_open() && app.outline_focused(), - "opening focuses" + "o from closed opens AND focuses" ); app.toggle_outline(); assert!( - app.outline_open() && !app.outline_focused(), - "toggling while focused returns focus to the diff without closing" + !app.outline_open() && !app.outline_focused(), + "o from open+focused closes — the toggle only ever tracks visibility" ); + // Re-open, then unfocus without going through `toggle_outline` (mirrors the startup + // seed: open, but diff-focused) — `o` from THAT state must still close, not cycle + // through a middle focused-then-unfocused state. + app.toggle_outline(); + app.focus_diff(); + assert!(app.outline_open() && !app.outline_focused()); + app.toggle_outline(); assert!( - !app.outline_open(), - "toggling again while open-but-unfocused closes the pane" + !app.outline_open() && !app.outline_focused(), + "o from open+unfocused closes the pane" + ); + } + + #[test] + fn focus_outline_opens_when_closed_and_syncs_the_cursor() { + let mut app = two_committed_changesets_two_and_one_files(); + while app.outline_open() { + app.toggle_outline(); + } + assert!(!app.outline_open()); + // Move the diff onto the second changeset before focusing, so a sync is observable. + app.next_changeset(); + let current_cs = app.current_cs(); + + app.focus_outline(); + + assert!(app.outline_open() && app.outline_focused()); + let items = app.outline_items(); + assert!( + matches!( + items[app.outline_cursor()], + crate::outline::OutlineItem::File { cs_idx, .. } if cs_idx == current_cs + ), + "opening via focus_outline syncs the cursor to the current diff position" + ); + } + + #[test] + fn focus_outline_on_an_already_open_outline_does_not_move_the_cursor() { + let mut app = two_committed_changesets_two_and_one_files(); + while app.outline_open() { + app.toggle_outline(); + } + app.toggle_outline(); // open + focus, synced + app.outline_move_by(-1); // manually reposition the outline cursor + app.focus_diff(); + let cursor_before = app.outline_cursor(); + + app.focus_outline(); + + assert!(app.outline_focused()); + assert_eq!( + app.outline_cursor(), + cursor_before, + "re-focusing an already-open outline must not stomp a manually positioned cursor" + ); + } + + #[test] + fn focus_diff_unfocuses_without_closing_the_outline() { + let mut app = two_committed_changesets_two_and_one_files(); + while app.outline_open() { + app.toggle_outline(); + } + app.toggle_outline(); // open + focus + assert!(app.outline_open() && app.outline_focused()); + + app.focus_diff(); + + assert!( + app.outline_open() && !app.outline_focused(), + "focus_diff unfocuses but leaves the outline open" ); } @@ -6839,6 +8146,9 @@ mod tests { let owned = Repository::open(repo.workdir().unwrap()).unwrap(); let mut app = App::from_changesets(owned, vec![view_a, view_b]); app.outline.mode = OutlineMode::Stack; + // CS3: pin BaseFirst explicitly — this test asserts per-header marker content, not + // display order, so it doesn't need to track the new HeadFirst default. + app.outline.order = OutlineOrder::BaseFirst; let items = app.outline_items(); assert_eq!( @@ -6944,6 +8254,10 @@ mod tests { let repo = Repository::open(fixture.repo().unwrap().workdir().unwrap()).unwrap(); let mut app = App::from_changesets(repo, vec![view_pending, view_failed]); app.outline.mode = OutlineMode::Stack; + // CS3: pin BaseFirst explicitly — this test asserts the exact header vec, which is + // incidental to base -> head storage order here, not what's under test (the + // loading/failed markers). + app.outline.order = OutlineOrder::BaseFirst; let items = app.outline_items(); assert_eq!( @@ -7098,6 +8412,10 @@ mod tests { let owned = Repository::open(repo.workdir().unwrap()).unwrap(); let mut app = App::from_changesets(owned, vec![view_a, view_b]); app.outline.mode = OutlineMode::Stack; + // CS3: pin BaseFirst explicitly — the regression this test guards needs cs-a BEFORE + // cs-b in the row list (an earlier row's insertion shifting a later row's index); the + // new HeadFirst default would put cs-b (head) first instead, inverting the scenario. + app.outline.order = OutlineOrder::BaseFirst; assert_eq!( app.current_cs(), 1, @@ -7155,6 +8473,7 @@ mod tests { file_idx: 0, path: "c1.txt".to_string(), status: StagedStatus::None, + change: FileStatus::Added, guides: Vec::new(), }, "a committed changeset's file must carry no staged-ness status" @@ -7170,10 +8489,39 @@ mod tests { ); } + /// CS5: `outline_snapshot`'s `change` field is lifted from the owning `FileChange::status`, + /// a wholly separate axis from `status` (staged-ness — see `outline::OutlineFile::change`'s + /// doc comment). `c1.txt` is a new file introduced by the committed changeset's head commit + /// (`Added`); `u1.txt` is an untracked worktree file (`Untracked`) — distinct FileStatus + /// values, confirming this isn't just always defaulting to one variant. + #[test] + fn outline_snapshot_lifts_change_status_from_the_file_model_independent_of_staged_status() { + let mut app = committed_and_uncommitted_stack(); + app.outline.mode = OutlineMode::Stack; + let items = app.outline_items(); + + let change_for = |path: &str| { + items + .iter() + .find_map(|it| match it { + OutlineItem::File { + path: p, change, .. + } if p == path => Some(*change), + _ => None, + }) + .unwrap_or_else(|| panic!("{path}'s file row present")) + }; + assert_eq!(change_for("c1.txt"), FileStatus::Added); + assert_eq!(change_for("u1.txt"), FileStatus::Untracked); + } + #[test] fn outline_move_by_on_a_file_row_jumps_the_diff() { let mut app = two_committed_changesets_two_and_one_files(); app.outline.mode = OutlineMode::Flat; + // CS3: pin BaseFirst explicitly — this test exercises `outline_move_by`'s row-crossing + // mechanics via hardcoded Flat-mode indices, not display order. + app.outline.order = OutlineOrder::BaseFirst; app.outline.cursor = 0; assert_eq!(app.current_cs(), 0); assert_eq!(app.current, 0); @@ -7193,6 +8541,10 @@ mod tests { fn outline_move_by_on_a_header_row_does_not_jump_the_diff() { let mut app = two_committed_changesets_two_and_one_files(); app.outline.mode = OutlineMode::Stack; + // CS3: pin BaseFirst explicitly — this test's hardcoded row indices assume base -> head + // order (header, a1, a2, header, b1); the new HeadFirst default is a display-order + // concern orthogonal to what's under test here (whether a header move jumps the diff). + app.outline.order = OutlineOrder::BaseFirst; // Header rows sit at indices 0 (cs-a) and 3 (cs-b) in Stack mode (header, a1, a2, // header, b1). Park the diff on a2, cursor on its row. app.outline.cursor = 2; @@ -7218,11 +8570,16 @@ mod tests { // header row (the LAST file crossed, exactly where unit presses leave it). let mut coalesced = two_committed_changesets_two_and_one_files(); coalesced.outline.mode = OutlineMode::Stack; + // CS3: pin BaseFirst explicitly — the burst-vs-sequential equivalence under test doesn't + // depend on which end of the stack displays first, and the inline comments below assume + // base -> head row order. + coalesced.outline.order = OutlineOrder::BaseFirst; coalesced.outline.cursor = 0; coalesced.outline_move_by(3); // header -> a1 -> a2 -> cs-b header let mut sequential = two_committed_changesets_two_and_one_files(); sequential.outline.mode = OutlineMode::Stack; + sequential.outline.order = OutlineOrder::BaseFirst; sequential.outline.cursor = 0; for _ in 0..3 { sequential.outline_move_by(1); @@ -7246,6 +8603,9 @@ mod tests { fn outline_confirm_on_a_header_row_jumps_to_its_first_file_and_returns_focus() { let mut app = two_committed_changesets_two_and_one_files(); app.outline.mode = OutlineMode::Stack; + // CS3: pin BaseFirst explicitly — cursor 3 is hardcoded to cs-b's header under base -> + // head row order; the confirm mechanic under test is order-agnostic. + app.outline.order = OutlineOrder::BaseFirst; app.outline.open = true; app.outline.focused = true; app.outline.cursor = 3; // cs-b's header row @@ -7283,6 +8643,7 @@ mod tests { file_idx: 0, path: "b1.txt".to_string(), status: StagedStatus::None, + change: FileStatus::Added, guides: Vec::new(), }, "the outline cursor must follow the diff's new position" @@ -7310,6 +8671,7 @@ mod tests { file_idx: 0, path: "b1.txt".to_string(), status: StagedStatus::None, + change: FileStatus::Added, guides: vec![true], }, "the outline cursor must follow the diff's new position, landing on b1.txt's row \ @@ -7409,57 +8771,257 @@ mod tests { // contract `render::render` reads (`outline_open`), so a regression there is caught at // the state layer too. let mut app = two_committed_changesets_two_and_one_files(); - // Default state is open+unfocused (locked design), so a single `o` here hits the - // "open, diff has focus" branch of the cycle, which closes the pane. + // Default state is open+unfocused (locked design); the pure toggle closes it regardless + // of focus. assert!(app.outline_open() && !app.outline_focused()); app.toggle_outline(); assert!(!app.outline_open()); } - // ── CS7: view-config (`apply_view_config`) ───────────────────────────────── - - #[test] - fn unset_view_config_keeps_current_defaults() { - let fixture = FixtureBuilder::new().build().unwrap(); - let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); - let mut app = app_from_fixture(&fixture); - - let warnings = app.apply_view_config(&config); - - assert!(warnings.is_empty()); - assert_eq!(app.outline_width(), DEFAULT_OUTLINE_WIDTH); - assert_eq!(app.outline_mode(), OutlineMode::default()); - assert_eq!(app.layout, Layout::default()); - assert_eq!(app.zoom, Zoom::default()); - } + // ── CS2: outline scrolloff viewport + g/G jumps ───────────────────────────── - #[test] - fn outline_width_overrides_default_when_set() { + /// Four committed changesets of three files each — Stack mode (the default) yields 16 rows + /// (header + 3 files, ×4), long enough to exercise [`App::derive_outline_scroll`]'s margin + /// behavior against a small `outline_height`, unlike the 5-row + /// [`two_committed_changesets_two_and_one_files`] fixture used elsewhere in this module. + fn four_committed_changesets_three_files_each() -> App { let fixture = FixtureBuilder::new() - .config("workon.review.outline.width", "40") + .config("core.autocrlf", "false") .build() .unwrap(); - let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); - let mut app = app_from_fixture(&fixture); - - let warnings = app.apply_view_config(&config); + let mut base = fixture + .commit("main") + .file("root.txt", "r\n") + .create("root") + .unwrap(); + let mut changesets = Vec::new(); + for cs_num in 0..4 { + let head = fixture + .commit("main") + .file(&format!("cs{cs_num}_a.txt"), "a\n") + .file(&format!("cs{cs_num}_b.txt"), "b\n") + .file(&format!("cs{cs_num}_c.txt"), "c\n") + .create(&format!("cs{cs_num}")) + .unwrap(); + changesets.push(Changeset { + name: format!("cs-{cs_num}"), + span: ChangesetSpan::Committed { base, head }, + title: None, + current: cs_num == 0, + needs_restack: false, + }); + base = head; + } + let repo = fixture.repo().unwrap(); + let views = changesets + .into_iter() + .map(|cs| { + let diff = crate::acquire::diff_changeset(repo, &cs).unwrap(); + ChangesetView::from_changeset_diff(cs, diff) + }) + .collect(); - assert!(warnings.is_empty()); - assert_eq!(app.outline_width(), 40); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, views); + app.open_current(); + app.outline.mode = OutlineMode::Stack; + assert_eq!(app.outline_items().len(), 16, "4 x (1 header + 3 files)"); + app } #[test] - fn outline_width_out_of_range_falls_back_to_default_with_warning() { - let fixture = FixtureBuilder::new() - .config("workon.review.outline.width", "9999") - .build() - .unwrap(); - let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); - let mut app = app_from_fixture(&fixture); - - let warnings = app.apply_view_config(&config); - - assert_eq!(app.outline_width(), DEFAULT_OUTLINE_WIDTH); + fn outline_move_by_keeps_cursor_within_the_scrolloff_margin() { + let mut app = four_committed_changesets_three_files_each(); + app.outline_height = 5; // bottom_margin = 5 - 1 - SCROLLOFF(2) = 2 + app.outline.cursor = 0; + app.derive_outline_scroll(app.outline_items().len()); + assert_eq!(app.outline_scroll(), 0); + + // Walk down one row at a time; the scroll must follow to keep the cursor within + // `[scroll, scroll + bottom_margin]`, never snapping straight to the cursor. + for _ in 0..8 { + app.outline_move_by(1); + let scroll = app.outline_scroll(); + let cursor = app.outline_cursor(); + assert!( + cursor >= scroll && cursor <= scroll + 2, + "cursor {cursor} must stay within the scrolloff-margined viewport at scroll {scroll}" + ); + } + assert!( + app.outline_scroll() > 0, + "scrolling down must have moved the viewport" + ); + + // Walking back up must scroll up minimally, not snap to zero. + let scroll_at_bottom = app.outline_scroll(); + app.outline_move_by(-1); + assert!( + app.outline_scroll() <= scroll_at_bottom, + "moving up must not increase scroll" + ); + assert!( + app.outline_scroll() > 0, + "a single step up from deep in the list must not snap scroll to zero" + ); + } + + #[test] + fn outline_scroll_clamps_at_both_ends() { + let mut app = four_committed_changesets_three_files_each(); + app.outline_height = 5; + + app.outline.cursor = 0; + app.derive_outline_scroll(app.outline_items().len()); + assert_eq!( + app.outline_scroll(), + 0, + "top row 0 must be visible at start" + ); + + let last = app.outline_items().len() - 1; + app.outline.cursor = last; + app.derive_outline_scroll(app.outline_items().len()); + let scroll = app.outline_scroll(); + assert!( + last >= scroll && last < scroll + app.outline_height, + "the last row must be visible once the cursor reaches it" + ); + assert!( + scroll <= app.outline_items().len().saturating_sub(app.outline_height), + "scroll must never run past the point where the last row leaves the viewport" + ); + } + + #[test] + fn outline_top_lands_cursor_zero_and_does_not_jump_a_header() { + // CS3: the outline's default order is now HeadFirst, so Stack mode's row 0 is cs-b's + // (the head changeset's) header, not cs-a's — see + // `stack_mode_head_first_shows_last_changesets_header_first_with_true_cs_idx` in + // outline.rs for the row-order pin. `outline_top`'s own contract (row 0, no diff jump) + // is order-agnostic, so only the "which header" framing below changes. + let mut app = two_committed_changesets_two_and_one_files(); + app.outline.mode = OutlineMode::Stack; + app.outline_height = 3; + app.next_changeset(); // move the diff off its start so a stray jump would be observable + let (cs_before, file_before) = (app.current_cs(), app.current); + + app.outline.cursor = 4; // a2.txt's row under head-first order (cs-a's last file) + app.outline_top(); + + assert_eq!(app.outline_cursor(), 0, "g lands on row 0"); + assert!( + matches!(app.outline_items()[0], OutlineItem::Header { .. }), + "row 0 in Stack mode is a header (cs-b's, the head changeset, under head-first order)" + ); + assert_eq!( + (app.current_cs(), app.current), + (cs_before, file_before), + "landing on a Header must not jump the diff" + ); + } + + #[test] + fn outline_bottom_lands_on_the_last_row_and_jumps_a_file() { + // CS3: under the new HeadFirst default, Stack mode's row order is cs-b's header/file(s) + // first, then cs-a's — so the LAST row is cs-a's last file (a2.txt, cs_idx 0, file_idx + // 1), not cs-b's only file as it was under the old base-first order. + let mut app = two_committed_changesets_two_and_one_files(); + app.outline.mode = OutlineMode::Stack; + app.outline_height = 3; + assert_eq!(app.current_cs(), 0, "starts on cs-a"); + + app.outline_bottom(); + + let last = app.outline_items().len() - 1; + assert_eq!(app.outline_cursor(), last, "G lands on the last row"); + assert!( + matches!(app.outline_items()[last], OutlineItem::File { .. }), + "the last row in Stack mode under head-first order is cs-a's last file, a2.txt" + ); + assert_eq!( + (app.current_cs(), app.current), + (0, 1), + "landing on a File must switch the diff there" + ); + } + + #[test] + fn outline_cycle_mode_and_sync_leave_scroll_consistent() { + let mut app = four_committed_changesets_three_files_each(); + app.outline_height = 4; + // Push the cursor (and scroll) deep into Stack mode's row list first. + for _ in 0..10 { + app.outline_move_by(1); + } + assert!( + app.outline_scroll() > 0, + "precondition: scrolled away from the top" + ); + + app.outline_cycle_mode(); // -> Tree + let cursor = app.outline_cursor(); + let scroll = app.outline_scroll(); + assert!( + cursor >= scroll && cursor < scroll + app.outline_height, + "outline_cycle_mode must leave the cursor visible within the new mode's scroll" + ); + + app.next_changeset(); // diff-initiated nav -> sync_outline_to_current + let cursor = app.outline_cursor(); + let scroll = app.outline_scroll(); + assert!( + cursor >= scroll && cursor < scroll + app.outline_height, + "sync_outline_to_current must leave the cursor visible within scroll" + ); + } + + // ── CS7: view-config (`apply_view_config`) ───────────────────────────────── + + #[test] + fn unset_view_config_keeps_current_defaults() { + let fixture = FixtureBuilder::new().build().unwrap(); + let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); + let mut app = app_from_fixture(&fixture); + + let warnings = app.apply_view_config(&config); + + assert!(warnings.is_empty()); + assert_eq!(app.outline_width(), DEFAULT_OUTLINE_WIDTH); + assert_eq!(app.outline_mode(), OutlineMode::default()); + assert_eq!(app.outline_order(), OutlineOrder::default()); + assert_eq!(app.outline_icons(), OutlineIcons::default()); + assert_eq!(app.layout, Layout::default()); + assert_eq!(app.zoom, Zoom::default()); + } + + #[test] + fn outline_width_overrides_default_when_set() { + let fixture = FixtureBuilder::new() + .config("workon.review.outline.width", "40") + .build() + .unwrap(); + let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); + let mut app = app_from_fixture(&fixture); + + let warnings = app.apply_view_config(&config); + + assert!(warnings.is_empty()); + assert_eq!(app.outline_width(), 40); + } + + #[test] + fn outline_width_out_of_range_falls_back_to_default_with_warning() { + let fixture = FixtureBuilder::new() + .config("workon.review.outline.width", "9999") + .build() + .unwrap(); + let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); + let mut app = app_from_fixture(&fixture); + + let warnings = app.apply_view_config(&config); + + assert_eq!(app.outline_width(), DEFAULT_OUTLINE_WIDTH); assert_eq!(warnings.len(), 1); assert!(warnings[0].contains("outline.width")); } @@ -7495,6 +9057,68 @@ mod tests { assert!(warnings[0].contains("outline.mode")); } + #[test] + fn outline_order_overrides_default_when_set() { + let fixture = FixtureBuilder::new() + .config("workon.review.outline.order", "base-first") + .build() + .unwrap(); + let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); + let mut app = app_from_fixture(&fixture); + + let warnings = app.apply_view_config(&config); + + assert!(warnings.is_empty()); + assert_eq!(app.outline_order(), OutlineOrder::BaseFirst); + } + + #[test] + fn outline_order_invalid_falls_back_to_default_with_warning() { + let fixture = FixtureBuilder::new() + .config("workon.review.outline.order", "bogus") + .build() + .unwrap(); + let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); + let mut app = app_from_fixture(&fixture); + + let warnings = app.apply_view_config(&config); + + assert_eq!(app.outline_order(), OutlineOrder::default()); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("outline.order")); + } + + #[test] + fn outline_icons_overrides_default_when_set() { + let fixture = FixtureBuilder::new() + .config("workon.review.outline.icons", "nerd") + .build() + .unwrap(); + let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); + let mut app = app_from_fixture(&fixture); + + let warnings = app.apply_view_config(&config); + + assert!(warnings.is_empty()); + assert_eq!(app.outline_icons(), OutlineIcons::Nerd); + } + + #[test] + fn outline_icons_invalid_falls_back_to_default_with_warning() { + let fixture = FixtureBuilder::new() + .config("workon.review.outline.icons", "bogus") + .build() + .unwrap(); + let config = ReviewConfig::new(fixture.repo().unwrap()).view_config(); + let mut app = app_from_fixture(&fixture); + + let warnings = app.apply_view_config(&config); + + assert_eq!(app.outline_icons(), OutlineIcons::default()); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("outline.icons")); + } + #[test] fn diff_layout_overrides_default_when_set() { let fixture = FixtureBuilder::new() @@ -7556,4 +9180,881 @@ mod tests { assert_eq!(warnings.len(), 1); assert!(warnings[0].contains("diff.zoom")); } + + // ── CS4: summary panel ─────────────────────────────────────────────────────── + + /// Force the outline open+focused with `mode` and `cursor`, matching the state + /// `summary_target` requires — the individual state-transition tests below build off this + /// instead of repeating the three-field setup. Pins `order` to `BaseFirst` so a fixture's + /// base -> head file/changeset indices line up with display order (the default `HeadFirst` + /// reverses the header row sequence — irrelevant to what's under test here, see CS3). + fn open_focused_outline(app: &mut App, mode: OutlineMode, cursor: usize) { + app.outline.open = true; + app.outline.focused = true; + app.outline.mode = mode; + app.outline.cursor = cursor; + app.outline.order = OutlineOrder::BaseFirst; + } + + #[test] + fn summary_target_is_none_when_the_outline_is_closed() { + let mut app = two_committed_changesets_two_and_one_files(); + app.outline.open = false; + app.outline.focused = false; + assert_eq!(app.summary_target(), None); + } + + #[test] + fn summary_target_is_none_when_the_outline_is_open_but_unfocused() { + let mut app = two_committed_changesets_two_and_one_files(); + app.outline.open = true; + app.outline.focused = false; + app.outline.mode = OutlineMode::Stack; + app.outline.cursor = 0; // a Header row + assert_eq!( + app.summary_target(), + None, + "an unfocused open outline must never override the diff area (locked design)" + ); + } + + #[test] + fn summary_target_is_none_when_the_cursor_is_on_a_file_row() { + let mut app = two_committed_changesets_two_and_one_files(); + open_focused_outline(&mut app, OutlineMode::Stack, 1); // cs-a's first file row + let items = app.outline_items(); + assert!(matches!(items[1], OutlineItem::File { .. })); + assert_eq!(app.summary_target(), None); + } + + #[test] + fn summary_target_is_some_changeset_on_a_header_row() { + let mut app = two_committed_changesets_two_and_one_files(); + open_focused_outline(&mut app, OutlineMode::Stack, 0); // cs-a's header row + let items = app.outline_items(); + assert!(matches!(items[0], OutlineItem::Header { cs_idx: 0, .. })); + assert_eq!(app.summary_target(), Some(SummaryTarget::Changeset(0))); + } + + #[test] + fn summary_target_is_some_dir_with_cs_idx_none_in_tree_mode() { + let mut app = single_changeset_with_nested_paths(); + let items = { + app.outline.mode = OutlineMode::Tree; + app.outline_items() + }; + let dir_idx = items + .iter() + .position(|it| matches!(it, OutlineItem::Dir { name, .. } if name == "src")) + .expect("src/ dir row present in Tree mode"); + open_focused_outline(&mut app, OutlineMode::Tree, dir_idx); + assert_eq!( + app.summary_target(), + Some(SummaryTarget::Dir { + cs_idx: None, + path: "src".to_string(), + }), + "Tree mode's single cross-stack trie has no owning changeset" + ); + } + + #[test] + fn summary_target_is_some_dir_with_cs_idx_some_in_stack_tree_mode() { + let mut app = single_changeset_with_nested_paths(); + let items = { + app.outline.mode = OutlineMode::StackTree; + app.outline_items() + }; + let dir_idx = items + .iter() + .position(|it| matches!(it, OutlineItem::Dir { name, .. } if name == "src")) + .expect("src/ dir row present in StackTree mode"); + open_focused_outline(&mut app, OutlineMode::StackTree, dir_idx); + assert_eq!( + app.summary_target(), + Some(SummaryTarget::Dir { + cs_idx: Some(0), + path: "src".to_string(), + }), + "StackTree mode's dir row belongs to the single changeset in this fixture" + ); + } + + #[test] + fn summary_target_returns_none_again_after_focus_diff() { + let mut app = two_committed_changesets_two_and_one_files(); + open_focused_outline(&mut app, OutlineMode::Stack, 0); + assert!(app.summary_target().is_some()); + app.focus_diff(); + assert_eq!( + app.summary_target(), + None, + "losing outline focus must immediately fall back to the diff body" + ); + } + + #[test] + fn summary_for_changeset_reflects_the_changesets_flags_and_files() { + let mut app = two_committed_changesets_two_and_one_files(); + open_focused_outline(&mut app, OutlineMode::Stack, 0); + let target = app.summary_target().unwrap(); + let Summary::Changeset(summary) = app.summary_for(target) else { + panic!("expected a Changeset summary for a Header target"); + }; + assert!(summary.current, "cs-a is the current changeset"); + assert!(!summary.needs_restack); + assert!(!summary.loading); + assert!(!summary.failed); + assert_eq!(summary.files.len(), 2, "cs-a touches a1.txt and a2.txt"); + assert!(summary.total_adds + summary.total_dels > 0); + } + + #[test] + fn summary_for_dir_in_tree_mode_aggregates_the_deduped_cross_stack_set() { + let mut app = single_changeset_with_nested_paths(); + app.outline.mode = OutlineMode::Tree; + let items = app.outline_items(); + let dir_idx = items + .iter() + .position(|it| matches!(it, OutlineItem::Dir { name, .. } if name == "src")) + .unwrap(); + open_focused_outline(&mut app, OutlineMode::Tree, dir_idx); + let target = app.summary_target().unwrap(); + let Summary::Dir(summary) = app.summary_for(target) else { + panic!("expected a Dir summary for a Dir target"); + }; + assert_eq!(summary.path, "src"); + let paths: Vec<&str> = summary.files.iter().map(|r| r.path.as_str()).collect(); + assert_eq!(paths, vec!["src/a.txt", "src/b.txt"]); + } + + // ── CS7: stage/unstage/discard from outline rows ───────────────────────────── + + /// Find the [`OutlineItem::File`] row index whose full path is `path` (in the CURRENT outline + /// mode/order) — the CS7 tests' stand-in for "click the row named X", since a row's raw index + /// shifts with mode/order and none of these tests want to hardcode it. + fn outline_file_row(app: &App, path: &str) -> usize { + app.outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::File { path: p, .. } if p == path)) + .unwrap_or_else(|| panic!("no outline File row for {path:?}")) + } + + #[test] + fn outline_stage_on_an_unstaged_file_row_stages_it_and_keeps_the_cursor_there() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + let idx = outline_file_row(&app, "a.txt"); + open_focused_outline(&mut app, OutlineMode::Stack, idx); + + app.outline_stage(); + + assert!(app.notice.is_none(), "stage must succeed: {:?}", app.notice); + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::has_staged_file("a.txt")); + assert!( + app.outline_focused(), + "outline must keep focus across the op" + ); + match &app.outline_items()[app.outline_cursor()] { + OutlineItem::File { path, status, .. } => { + assert_eq!(path, "a.txt"); + assert_eq!( + *status, + StagedStatus::Staged, + "row now shows the staged glyph" + ); + } + other => panic!("expected the cursor to stay on a.txt's File row, got {other:?}"), + } + } + + #[test] + fn outline_stage_on_a_staged_file_row_unstages_it() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .staged_file("new.txt", "hello\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + let idx = outline_file_row(&app, "new.txt"); + open_focused_outline(&mut app, OutlineMode::Stack, idx); + + app.outline_stage(); + + assert!( + app.notice.is_none(), + "unstage must succeed: {:?}", + app.notice + ); + let repo = fixture.repo().unwrap(); + // An Added file has no HEAD entry, so unstaging it lands as untracked — same outcome + // `stage_file_in_staged_pane_unstages_whole_file` pins for the diff-pane path. + repo.assert(predicate::repo::has_untracked_file("new.txt")); + } + + #[test] + fn outline_stage_on_a_dir_row_stages_every_unstaged_file_under_it() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("src/a.txt", "a\n", "a\nCHANGED\n") + .unstaged_file("src/b.txt", "b\n", "b\nCHANGED\n") + .unstaged_file("top.txt", "t\n", "t\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.outline.mode = OutlineMode::StackTree; + let dir_idx = app + .outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::Dir { path, .. } if path == "src")) + .expect("src/ dir row present in StackTree mode"); + open_focused_outline(&mut app, OutlineMode::StackTree, dir_idx); + + app.outline_stage(); + + assert!( + app.notice.is_none(), + "dir stage must succeed: {:?}", + app.notice + ); + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::has_staged_file("src/a.txt")); + repo.assert(predicate::repo::has_staged_file("src/b.txt")); + // The file outside `src/` must be left alone. + repo.assert(predicate::repo::has_unstaged_file("top.txt")); + } + + #[test] + fn outline_stage_on_a_dir_row_applies_each_files_own_verb_under_mixed_status() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("src/a.txt", "a\n", "a\nCHANGED\n") + .staged_file("src/b.txt", "b\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.outline.mode = OutlineMode::StackTree; + let dir_idx = app + .outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::Dir { path, .. } if path == "src")) + .expect("src/ dir row present in StackTree mode"); + open_focused_outline(&mut app, OutlineMode::StackTree, dir_idx); + + app.outline_stage(); + + assert!( + app.notice.is_none(), + "mixed-status dir stage must succeed: {:?}", + app.notice + ); + let repo = fixture.repo().unwrap(); + // The unstaged file stages... + repo.assert(predicate::repo::has_staged_file("src/a.txt")); + // ...and the already-staged (Added, no HEAD entry) file unstages to untracked — each + // file's own verb, not a single direction applied to the whole directory. + repo.assert(predicate::repo::has_untracked_file("src/b.txt")); + } + + #[test] + fn outline_stage_on_the_header_row_refuses_without_touching_the_index() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + // Index 0 in Stack mode is always the changeset Header row. + open_focused_outline(&mut app, OutlineMode::Stack, 0); + assert!(matches!(app.outline_items()[0], OutlineItem::Header { .. })); + + app.outline_stage(); + + let notice = app + .notice + .as_ref() + .expect("staging a Header row must refuse"); + assert_eq!(notice.severity, Severity::Error); + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::has_unstaged_file("a.txt")); + } + + #[test] + fn outline_stage_on_a_committed_changesets_file_row_refuses_with_committed_wording() { + let mut app = committed_and_uncommitted_stack(); + // `BaseFirst` order + Stack mode: Header(committed) 0, File(committed/c1.txt) 1, + // Header(uncommitted) 2, File(uncommitted/u1.txt) 3. + open_focused_outline(&mut app, OutlineMode::Stack, 1); + assert!(matches!( + &app.outline_items()[1], + OutlineItem::File { cs_idx, path, .. } if *cs_idx == 0 && path == "c1.txt" + )); + + app.outline_stage(); + + let notice = app + .notice + .as_ref() + .expect("staging a committed changeset's row must refuse"); + assert_eq!(notice.severity, Severity::Error); + assert!( + notice.text.contains("already committed"), + "got: {:?}", + notice.text + ); + } + + #[test] + fn outline_discard_on_a_file_row_requests_confirm_then_y_reverts_the_worktree() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "ONE\ntwo\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + let idx = outline_file_row(&app, "a.txt"); + open_focused_outline(&mut app, OutlineMode::Stack, idx); + + app.outline_discard(); + + let confirm = app + .pending_confirm + .as_ref() + .expect("discard must request a confirm"); + assert!( + confirm.prompt.contains("a.txt"), + "got: {:?}", + confirm.prompt + ); + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::workdir_file_equals("a.txt", "ONE\ntwo\n")); + + app.resolve_confirm(true); + + assert!(app.pending_confirm.is_none(), "y must clear the confirm"); + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::workdir_file_equals("a.txt", "one\ntwo\n")); + } + + #[test] + fn outline_discard_survives_an_intervening_refresh_that_shifts_file_indices() { + // The confirm modal doesn't stop the tick beat: an external index change can trigger a + // full refresh between `d` and `y`, shifting every (cs_idx, file_idx). The pending op + // stores (changeset name, path) pairs and re-resolves at answer time, so the discard + // must still hit the file it was requested on — not whatever now sits at its old index. + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("b.txt", "one\ntwo\n", "ONE\ntwo\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + let idx = outline_file_row(&app, "b.txt"); + open_focused_outline(&mut app, OutlineMode::Stack, idx); + + app.outline_discard(); + assert!(app.pending_confirm.is_some()); + + // A new modified file that sorts BEFORE b.txt enters the diff while the confirm is up, + // then a refresh rebuilds the file lists — b.txt's file_idx shifts by one. + let repo = fixture.repo().unwrap(); + let workdir = repo.workdir().unwrap(); + std::fs::write(workdir.join("a.txt"), "NEW\n").unwrap(); + let mut index = repo.index().unwrap(); + index.add_path(std::path::Path::new("a.txt")).unwrap(); + index.write().unwrap(); + std::fs::write(workdir.join("a.txt"), "NEW\nCHANGED\n").unwrap(); + app.refresh(); + assert!( + app.pending_confirm.is_some(), + "the refresh must not consume the pending confirm" + ); + + app.resolve_confirm(true); + + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::workdir_file_equals("b.txt", "one\ntwo\n")); + repo.assert(predicate::repo::workdir_file_equals( + "a.txt", + "NEW\nCHANGED\n", + )); + } + + #[test] + fn outline_discard_confirm_n_cancels_and_leaves_the_worktree_unchanged() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "ONE\ntwo\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + let idx = outline_file_row(&app, "a.txt"); + open_focused_outline(&mut app, OutlineMode::Stack, idx); + + app.outline_discard(); + app.resolve_confirm(false); + + assert!(app.pending_confirm.is_none(), "n must clear the confirm"); + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::workdir_file_equals("a.txt", "ONE\ntwo\n")); + } + + #[test] + fn outline_discard_on_a_dir_row_names_the_scope_then_y_discards_every_file_under_it() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("src/a.txt", "a\n", "A\n") + .unstaged_file("src/b.txt", "b\n", "B\n") + .unstaged_file("top.txt", "t\n", "T\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.outline.mode = OutlineMode::StackTree; + let dir_idx = app + .outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::Dir { path, .. } if path == "src")) + .expect("src/ dir row present in StackTree mode"); + open_focused_outline(&mut app, OutlineMode::StackTree, dir_idx); + + app.outline_discard(); + + let confirm = app + .pending_confirm + .as_ref() + .expect("dir discard must request a confirm"); + assert!( + confirm.prompt.contains('2') && confirm.prompt.contains("src"), + "prompt must name the file count and the scoped path, got: {:?}", + confirm.prompt + ); + + app.resolve_confirm(true); + + let repo = fixture.repo().unwrap(); + repo.assert(predicate::repo::workdir_file_equals("src/a.txt", "a\n")); + repo.assert(predicate::repo::workdir_file_equals("src/b.txt", "b\n")); + // The file outside `src/` must be left untouched. + repo.assert(predicate::repo::workdir_file_equals("top.txt", "T\n")); + } + + #[test] + fn outline_stage_in_a_multi_file_outline_keeps_the_cursor_on_the_acted_on_row_not_the_diffs_current_file( + ) { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "a\n", "a\nCHANGED\n") + .unstaged_file("b.txt", "b\n", "b\nCHANGED\n") + .unstaged_file("c.txt", "c\n", "c\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + assert_eq!( + app.files()[app.current].path, + "a.txt", + "the diff opens on the first file, a.txt — never touched by this test" + ); + let idx = outline_file_row(&app, "b.txt"); + open_focused_outline(&mut app, OutlineMode::Stack, idx); + + app.outline_stage(); + + assert!(app.notice.is_none(), "stage must succeed: {:?}", app.notice); + match &app.outline_items()[app.outline_cursor()] { + OutlineItem::File { path, status, .. } => { + assert_eq!( + path, "b.txt", + "the cursor must stay on the acted-on row, not drift to the diff's own \ + current file (a.txt, via sync_outline_to_current inside coordinated_refresh)" + ); + assert_eq!(*status, StagedStatus::Staged); + } + other => panic!("expected the cursor on b.txt's File row, got {other:?}"), + } + } + + // ── CS8: progressive gap expansion ────────────────────────────────────── + + /// A single-file fixture with two hunks separated by a wide (40-line) unchanged run — wide + /// enough that even a full 10/10 [`App::expand_gap_at_cursor`] press still leaves a + /// surviving [`DisplayRow::Gap`] (`40 - 2*3 - 2*10 = 14` rows still hidden), unlike + /// [`two_hunk_fixture`]'s much narrower gap. + fn two_hunks_with_a_wide_gap_fixture() -> Fixture { + let mut committed = String::from("OLD_HUNK_A\n"); + let mut modified = String::from("NEW_HUNK_A\n"); + for i in 1..=40 { + committed.push_str(&format!("ctx{i}\n")); + modified.push_str(&format!("ctx{i}\n")); + } + committed.push_str("OLD_HUNK_B\n"); + modified.push_str("NEW_HUNK_B\n"); + + FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("f.txt", &committed, &modified) + .build() + .unwrap() + } + + /// The display-row index of the current file's ONLY gap row — the fixture shape every CS8 + /// expansion test below relies on. + fn only_gap_row(app: &App) -> usize { + app.current_view_ref() + .expect("loaded view") + .display + .iter() + .position(|r| matches!(r, DisplayRow::Gap { .. })) + .expect("expected exactly one gap row") + } + + #[test] + fn expand_gap_cancels_an_active_selection_but_a_non_gap_press_leaves_it_alone() { + // An expansion reshapes the focused pane's row space, so `selection_anchor`'s invariant + // (cancel, never translate) applies — a selection made before the expand would silently + // cover different lines after it. The non-gap no-op path must NOT cancel: nothing + // reshaped. + let fixture = two_hunks_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + app.start_selection(); + assert!(app.selection_anchor.is_some(), "selection must start"); + app.expand_gap_at_cursor(false); // cursor sits on the first hunk, not a gap: no-op + assert!( + app.selection_anchor.is_some(), + "a no-op press on a non-gap row must leave the selection alone" + ); + + app.cursor = only_gap_row(&app); + app.expand_gap_at_cursor(false); + assert!( + app.selection_anchor.is_none(), + "an actual expansion reshapes the row space and must cancel the selection" + ); + } + + #[test] + fn expand_gap_at_cursor_on_a_gap_row_reveals_more_rows() { + let fixture = two_hunks_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = only_gap_row(&app); + let before_len = app.current_view_ref().unwrap().display.len(); + app.cursor = gap_row; + + app.expand_gap_at_cursor(false); + + let view = app.current_view_ref().unwrap(); + assert!( + view.display.len() > before_len, + "expanding must reveal more rows: {before_len} -> {}", + view.display.len() + ); + assert!( + app.cursor < view.display.len(), + "cursor must stay in bounds" + ); + assert!( + matches!(view.display[app.cursor], DisplayRow::Row(_)), + "the cursor's old index (the gap's leading edge) must now hold a revealed row, not \ + the gap marker: {:?}", + view.display[app.cursor] + ); + // The gap is wide enough (40 hidden rows) that a single 10/10 press doesn't consume it. + assert!( + view.display + .iter() + .any(|r| matches!(r, DisplayRow::Gap { .. })), + "a partial expansion of this fixture must still leave a gap row" + ); + } + + #[test] + fn expand_gap_at_cursor_on_a_non_gap_row_is_a_no_op() { + let fixture = two_hunks_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); // cursor lands on hunk A's row, not the gap + + let before_len = app.current_view_ref().unwrap().display.len(); + let before_cursor = app.cursor; + assert!( + !matches!( + app.current_view_ref().unwrap().display[before_cursor], + DisplayRow::Gap { .. } + ), + "precondition: cursor starts on hunk A, not the gap" + ); + + app.expand_gap_at_cursor(false); + + assert_eq!(app.cursor, before_cursor, "no-op must not move the cursor"); + assert_eq!( + app.current_view_ref().unwrap().display.len(), + before_len, + "no-op must not change the row count" + ); + assert!(app.notice.is_none(), "a no-op must not raise a notice"); + } + + #[test] + fn stage_hunk_after_expanding_a_gap_stages_the_intended_hunk() { + let fixture = two_hunks_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = only_gap_row(&app); + app.cursor = gap_row; + app.expand_gap_at_cursor(false); + + // Move to hunk B (the LATER hunk) through the freshly rebuilt `display`/`display_hunk` — + // this is the coordinate-space desync CS8 must not introduce: `display_hunk` is + // recomputed by `rebuild_rows` from the SAME `aligned`/`hunks` every time, so the row + // under the cursor must still resolve to the right hunk index after an expansion. + app.next_hunk_row(); + app.stage_hunk(); + + assert!(app.notice.is_none(), "stage must succeed: {:?}", app.notice); + let repo = fixture.repo().unwrap(); + let mut expected_index = String::from("OLD_HUNK_A\n"); + let mut expected_workdir = String::from("NEW_HUNK_A\n"); + for i in 1..=40 { + expected_index.push_str(&format!("ctx{i}\n")); + expected_workdir.push_str(&format!("ctx{i}\n")); + } + expected_index.push_str("NEW_HUNK_B\n"); + expected_workdir.push_str("NEW_HUNK_B\n"); + // The index picks up ONLY hunk B; hunk A must stay unstaged. + repo.assert(predicate::repo::index_blob_equals( + "f.txt", + expected_index.as_str(), + )); + repo.assert(predicate::repo::workdir_file_equals( + "f.txt", + expected_workdir.as_str(), + )); + } + + #[test] + fn expanding_a_gap_clears_the_row_keyed_word_span_cache() { + let fixture = two_hunks_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); // cursor on hunk A's row — a word-diff pair + + let hunk_a_row = app.cursor; + app.current_view().unwrap().word_spans_for_row(hunk_a_row); + assert!( + !app.current_view_ref().unwrap().word_spans.is_empty(), + "precondition: the cache must be populated before expanding" + ); + + let gap_row = only_gap_row(&app); + app.cursor = gap_row; + app.expand_gap_at_cursor(false); + + assert!( + app.current_view_ref().unwrap().word_spans.is_empty(), + "rebuild_rows must clear the row-keyed word-span cache — a stale entry would \ + mismatch the row it renders under post-expansion" + ); + // The cache is still USABLE post-clear, not just permanently empty — re-populating it + // must not panic and must produce a non-empty span for the still-word-diffable row. + let (old_spans, new_spans) = app.current_view().unwrap().word_spans_for_row(hunk_a_row); + assert!( + !old_spans.is_empty() || !new_spans.is_empty(), + "hunk A is still a word-diff pair after the rebuild" + ); + } + + #[test] + fn refresh_resets_a_files_gap_expansions() { + let fixture = two_hunks_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = only_gap_row(&app); + app.cursor = gap_row; + app.expand_gap_at_cursor(false); + let expanded_len = app.current_view_ref().unwrap().display.len(); + + app.refresh(); // ends with its own `open_current`, same as every other refresh path + + let view = app.current_view_ref().expect("view survives refresh"); + assert!( + view.display.len() < expanded_len, + "a fresh view must re-collapse to the base gap window, not carry over the prior \ + expansion: expanded {expanded_len}, post-refresh {}", + view.display.len() + ); + assert!( + view.display + .iter() + .any(|r| matches!(r, DisplayRow::Gap { .. })), + "the gap must be back in its base (still-collapsed) form" + ); + } + + // ── CS9: reveal gaps to the enclosing tree-sitter scope ───────────────── + + /// A `.rs` fixture where both edits sit inside the SAME long function, with a 40-line + /// unchanged run between them wide enough that even a +10/+10 press would still leave a + /// gap (mirrors [`two_hunks_with_a_wide_gap_fixture`]'s width) — but because the whole + /// hidden run lies inside `long_function`'s body, a scope-reveal press should uncover it + /// ENTIRELY (the function encloses the whole gap), unlike +10/+10. + fn function_with_a_wide_internal_gap_fixture() -> Fixture { + let mut committed = String::from("fn long_function() {\n let a = OLD_A;\n"); + let mut modified = String::from("fn long_function() {\n let a = NEW_A;\n"); + for i in 1..=40 { + committed.push_str(&format!(" ctx{i}();\n")); + modified.push_str(&format!(" ctx{i}();\n")); + } + committed.push_str(" let b = OLD_B;\n}\n"); + modified.push_str(" let b = NEW_B;\n}\n"); + + FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("f.rs", &committed, &modified) + .build() + .unwrap() + } + + /// A `.rs` fixture where both edits sit at the TOP LEVEL (no enclosing function/impl/etc — + /// only comment lines separate them), so [`crate::scope::enclosing_scope_lines`] finds no + /// allowlisted ancestor around the anchor and a press must fall back to +10/+10 exactly like + /// a grammar-less file. + fn top_level_edits_with_a_wide_gap_fixture() -> Fixture { + let mut committed = String::from("static A: i32 = OLD_A;\n"); + let mut modified = String::from("static A: i32 = NEW_A;\n"); + for i in 1..=40 { + committed.push_str(&format!("// ctx{i}\n")); + modified.push_str(&format!("// ctx{i}\n")); + } + committed.push_str("static B: i32 = OLD_B;\n"); + modified.push_str("static B: i32 = NEW_B;\n"); + + FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("f.rs", &committed, &modified) + .build() + .unwrap() + } + + /// The `skipped` count of the current file's only [`DisplayRow::Gap`], found by scanning + /// `display` (NOT via `app.cursor` — expanding the gap's leading edge shifts the gap marker + /// to a later index, same as [`only_gap_row`] re-finds it after an expansion in the CS8 + /// tests above). Panics if there isn't exactly one gap row. + fn gap_skipped(app: &App) -> usize { + let row = only_gap_row(app); + match app.current_view_ref().expect("loaded view").display[row] { + DisplayRow::Gap { skipped, .. } => skipped, + other => panic!("expected a Gap row, got {other:?}"), + } + } + + #[test] + fn scope_reveal_uncovers_the_whole_gap_when_the_enclosing_function_covers_it() { + let fixture = function_with_a_wide_internal_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = only_gap_row(&app); + app.cursor = gap_row; + + app.expand_gap_at_cursor(false); + + let view = app.current_view_ref().unwrap(); + assert!( + !view + .display + .iter() + .any(|r| matches!(r, DisplayRow::Gap { .. })), + "the enclosing function covers the ENTIRE hidden run, so a single scope-reveal press \ + must consume the gap completely — unlike a flat +10/+10 press, which would still \ + leave one on this fixture's 40-row gap: {:?}", + view.display + ); + } + + #[test] + fn a_grammarless_file_falls_back_to_the_flat_plus_ten_reveal() { + // Reuse CS8's `.txt` fixture (no bundled grammar for that extension) — the scope-reveal + // path must find no lang key and fall straight through to +10/+10, same as before CS9. + let fixture = two_hunks_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = only_gap_row(&app); + app.cursor = gap_row; + let skipped_before = gap_skipped(&app); + + app.expand_gap_at_cursor(false); + + let skipped_after = gap_skipped(&app); + assert_eq!( + skipped_before - skipped_after, + 20, + "no grammar for .txt: exactly the flat 10-before/10-after reveal, same as CS8" + ); + } + + #[test] + fn a_scope_with_nothing_new_falls_back_to_the_flat_plus_ten_reveal() { + // Both edits are top-level `static`s with no enclosing function/impl/etc — the anchor + // line has no allowlisted ancestor, so scope-reveal finds nothing and must fall back to + // +10/+10 exactly like the grammarless case, even though this file DOES have a grammar. + let fixture = top_level_edits_with_a_wide_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = only_gap_row(&app); + app.cursor = gap_row; + let skipped_before = gap_skipped(&app); + + app.expand_gap_at_cursor(false); + + let skipped_after = gap_skipped(&app); + assert_eq!( + skipped_before - skipped_after, + 20, + "no enclosing scope at the top level: falls back to the flat 10-before/10-after reveal" + ); + } + + #[test] + fn full_expand_ignores_scope_reveal_regardless_of_grammar() { + // `E` (full=true) must stay pure CS8 behavior even on a file with a grammar and a scope + // that would otherwise apply — scope-reveal is an `Enter`-only (CS9) refinement. + let fixture = function_with_a_wide_internal_gap_fixture(); + let mut app = app_from_fixture(&fixture); + app.open_current(); + + let gap_row = only_gap_row(&app); + app.cursor = gap_row; + + app.expand_gap_at_cursor(true); + + let view = app.current_view_ref().unwrap(); + assert!( + !view + .display + .iter() + .any(|r| matches!(r, DisplayRow::Gap { .. })), + "E must fully expand the gap: {:?}", + view.display + ); + } } diff --git a/git-workon-review/src/config.rs b/git-workon-review/src/config.rs index b7361c3e..47c00522 100644 --- a/git-workon-review/src/config.rs +++ b/git-workon-review/src/config.rs @@ -31,11 +31,21 @@ //! [workon "review.outline"] //! width = 32 //! mode = tree +//! order = base-first ; head-first | base-first (default: head-first) +//! icons = nerd ; nerd | none (default: none) //! //! [workon "review.diff"] //! layout = split //! zoom = combined //! ``` +//! +//! ## `outline.icons` (CS5) +//! +//! Opt-in nerd-font file/dir icons in the outline pane. There is deliberately NO auto-detection +//! — a terminal cannot report whether the user's font is patched with the nerd-font glyphs, so +//! guessing would silently render tofu/mojibake for anyone without one. Default is `none` +//! (today's plain text); set `icons = nerd` explicitly once your terminal font supports it. See +//! [`crate::icons`] for the glyph table. use git2::Repository; @@ -101,6 +111,8 @@ pub struct RawBinding { pub struct RawViewConfig { pub outline_width: Option, pub outline_mode: Option, + pub outline_order: Option, + pub outline_icons: Option, pub diff_layout: Option, pub diff_zoom: Option, } @@ -203,6 +215,17 @@ impl<'repo> ReviewConfig<'repo> { self.get_view_string(View::Outline, "mode") } + /// Get `workon.review.outline.order`, raw. `None` if unset. + pub fn outline_order(&self) -> Result, git2::Error> { + self.get_view_string(View::Outline, "order") + } + + /// Get `workon.review.outline.icons`, raw. `None` if unset — callers apply the current + /// default ([`crate::icons::OutlineIcons::None`], CS5: no auto-detection story exists). + pub fn outline_icons(&self) -> Result, git2::Error> { + self.get_view_string(View::Outline, "icons") + } + /// Get `workon.review.diff.layout`, raw. `None` if unset. pub fn diff_layout(&self) -> Result, git2::Error> { self.get_view_string(View::Diff, "layout") @@ -224,6 +247,8 @@ impl<'repo> ReviewConfig<'repo> { RawViewConfig { outline_width: self.outline_width().ok().flatten(), outline_mode: self.outline_mode().ok().flatten(), + outline_order: self.outline_order().ok().flatten(), + outline_icons: self.outline_icons().ok().flatten(), diff_layout: self.diff_layout().ok().flatten(), diff_zoom: self.diff_zoom().ok().flatten(), } @@ -407,6 +432,8 @@ mod tests { let fixture = FixtureBuilder::new() .config("workon.review.outline.width", "40") .config("workon.review.outline.mode", "tree") + .config("workon.review.outline.order", "base-first") + .config("workon.review.outline.icons", "nerd") .config("workon.review.diff.layout", "split") .config("workon.review.diff.zoom", "staged") .build() @@ -419,6 +446,14 @@ mod tests { config.outline_mode().expect("mode"), Some("tree".to_string()) ); + assert_eq!( + config.outline_order().expect("order"), + Some("base-first".to_string()) + ); + assert_eq!( + config.outline_icons().expect("icons"), + Some("nerd".to_string()) + ); assert_eq!( config.diff_layout().expect("layout"), Some("split".to_string()) @@ -437,6 +472,8 @@ mod tests { assert_eq!(config.outline_width().expect("width"), None); assert_eq!(config.outline_mode().expect("mode"), None); + assert_eq!(config.outline_order().expect("order"), None); + assert_eq!(config.outline_icons().expect("icons"), None); assert_eq!(config.diff_layout().expect("layout"), None); assert_eq!(config.diff_zoom().expect("zoom"), None); } diff --git a/git-workon-review/src/highlight.rs b/git-workon-review/src/highlight.rs index ba6067e6..365c4fe0 100644 --- a/git-workon-review/src/highlight.rs +++ b/git-workon-review/src/highlight.rs @@ -67,7 +67,10 @@ pub fn capture_index(name: &str) -> Option { HIGHLIGHT_NAMES.iter().position(|n| *n == name) } -fn lang_key_for_ext(ext: &str) -> Option<&'static str> { +/// Maps a file extension to the [`build_config`]/[`language_for_key`] key for its grammar, or +/// `None` when no bundled grammar covers it. `pub(crate)` so [`crate::app`] can resolve a gap's +/// anchor file to a scope-lookup language (CS9) without duplicating this table. +pub(crate) fn lang_key_for_ext(ext: &str) -> Option<&'static str> { match ext { "rs" => Some("rust"), "lua" => Some("lua"), @@ -81,6 +84,26 @@ fn lang_key_for_ext(ext: &str) -> Option<&'static str> { } } +/// The raw `tree_sitter::Language` for a [`lang_key_for_ext`] key, with no highlight query +/// configuration attached — [`build_config`] below wraps the same grammar constructors together +/// with a language's highlight/injection/locals queries for `TsHighlighter`; [`crate::scope`] +/// needs only the grammar (it parses to walk node kinds, not to highlight), so it shares this +/// smaller constructor instead of duplicating the `LANGUAGE.into()` calls. +pub(crate) fn language_for_key(key: &str) -> Option { + let language = match key { + "rust" => tree_sitter_rust::LANGUAGE.into(), + "lua" => tree_sitter_lua::LANGUAGE.into(), + "json" => tree_sitter_json::LANGUAGE.into(), + "toml" => tree_sitter_toml_ng::LANGUAGE.into(), + "javascript" => tree_sitter_javascript::LANGUAGE.into(), + "typescript" => tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into(), + "tsx" => tree_sitter_typescript::LANGUAGE_TSX.into(), + "markdown" => tree_sitter_md::LANGUAGE.into(), + _ => return None, + }; + Some(language) +} + fn build_config(key: &'static str) -> Option { let result = match key { "rust" => HighlightConfiguration::new( diff --git a/git-workon-review/src/icons.rs b/git-workon-review/src/icons.rs new file mode 100644 index 00000000..cd3ba7df --- /dev/null +++ b/git-workon-review/src/icons.rs @@ -0,0 +1,91 @@ +//! CS5's opt-in nerd-font file-type icon table — a pure module, no [`crate::app::App`]/ +//! [`crate::outline`] dependency, mirroring [`crate::summary`]'s pure-module posture. +//! +//! A terminal cannot report which font (patched with the nerd-font private-use glyphs or not) +//! the user has configured, so there is NO auto-detection here or anywhere else in the crate — +//! icons are strictly opt-in via `workon.review.outline.icons = nerd` (see `config.rs`'s schema +//! doc block and `App::apply_view_config`). With the config left at its default (`none`), +//! nothing in this module is ever called from `render.rs`. + +/// Which of the outline's icon strategies is active — `workon.review.outline.icons` +/// (`nerd`/`none`), read once at startup by `App::apply_view_config` (CS5 mirrors CS3's +/// `OutlineOrder` plumbing exactly: `RawViewConfig` field -> `ReviewConfig` getter -> +/// `parse_outline_icons` -> warn-and-fallback in `apply_view_config` -> `OutlineState` field). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum OutlineIcons { + /// No icon glyph — today's plain `[glyph][letter] path` row (CS5's unconditional part only). + #[default] + None, + /// A nerd-font private-use glyph per file extension (falling back to + /// [`DEFAULT_ICON`]/[`DIR_ICON`]), inserted before the path/name. + Nerd, +} + +/// The directory-row icon (nerd-font `nf-fa-folder`, U+F07B) — used for every +/// [`crate::outline::OutlineItem::Dir`] row when [`OutlineIcons::Nerd`] is active. +pub const DIR_ICON: char = '\u{f07b}'; // nf-fa-folder + +/// The fallback file icon (nerd-font `nf-fa-file`, U+F15B) for any extension not in +/// [`icon_for_path`]'s table (including extensionless files). +pub const DEFAULT_ICON: char = '\u{f15b}'; // nf-fa-file + +/// Look up the nerd-font glyph for `path`'s extension — small, deliberately-curated table +/// covering the languages this crate's own `highlight.rs` already bundles grammars for +/// (`lang_key_for_ext`), plus a couple of common project files. Every codepoint below is in the +/// nerd-font private-use area (`seti`/`devicons`/`fa` icon sets); unrecognized extensions and +/// extensionless files fall back to [`DEFAULT_ICON`]. +pub fn icon_for_path(path: &str) -> char { + // `Cargo.lock`/other `*.lock` files: match on the file NAME first, since "lock" isn't a + // meaningful extension-based language distinction the way the rest of the table is. + let name = path.rsplit('/').next().unwrap_or(path); + if name.ends_with(".lock") { + return '\u{f023}'; // nf-fa-lock + } + let ext = match name.rsplit_once('.') { + Some((_, ext)) => ext, + None => return DEFAULT_ICON, + }; + match ext { + "rs" => '\u{e7a8}', // seti-rust + "lua" => '\u{e620}', // seti-lua + "js" | "mjs" | "cjs" => '\u{e74e}', // seti-javascript + "jsx" | "tsx" => '\u{e7ba}', // seti-react + "ts" | "mts" | "cts" => '\u{e628}', // seti-typescript + "json" => '\u{e60b}', // seti-json + "toml" => '\u{e6b2}', // seti-config (toml has no dedicated seti glyph) + "md" | "markdown" => '\u{e73e}', // seti-markdown + _ => DEFAULT_ICON, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn known_extensions_map_to_their_glyphs() { + assert_eq!(icon_for_path("src/main.rs"), '\u{e7a8}'); + assert_eq!(icon_for_path("scripts/init.lua"), '\u{e620}'); + assert_eq!(icon_for_path("index.js"), '\u{e74e}'); + assert_eq!(icon_for_path("app.mjs"), '\u{e74e}'); + assert_eq!(icon_for_path("component.tsx"), '\u{e7ba}'); + assert_eq!(icon_for_path("component.jsx"), '\u{e7ba}'); + assert_eq!(icon_for_path("types.ts"), '\u{e628}'); + assert_eq!(icon_for_path("package.json"), '\u{e60b}'); + assert_eq!(icon_for_path("Cargo.toml"), '\u{e6b2}'); + assert_eq!(icon_for_path("README.md"), '\u{e73e}'); + } + + #[test] + fn lock_files_match_on_name_not_extension() { + assert_eq!(icon_for_path("Cargo.lock"), '\u{f023}'); + assert_eq!(icon_for_path("nested/dir/yarn.lock"), '\u{f023}'); + } + + #[test] + fn unknown_and_extensionless_paths_fall_back_to_the_default_icon() { + assert_eq!(icon_for_path("Makefile"), DEFAULT_ICON); + assert_eq!(icon_for_path("script.sh"), DEFAULT_ICON); + assert_eq!(icon_for_path("noextension"), DEFAULT_ICON); + } +} diff --git a/git-workon-review/src/keymap.rs b/git-workon-review/src/keymap.rs index 1250927b..10d1dcd3 100644 --- a/git-workon-review/src/keymap.rs +++ b/git-workon-review/src/keymap.rs @@ -17,9 +17,10 @@ //! action names and same-view key collisions are collected as [`Keymap::warnings`]. //! //! **Not handled here** (stays hardcoded in `tui.rs`): the confirm modal (`y`/`n`/`Esc`) and the -//! whole `Esc`-precedence cascade (confirm > outline-unfocus > selection-cancel > quit). Per -//! ADR-034 those are conventional and safety-sensitive; they are never routed through the -//! registry, so `Esc` is not a registry token. +//! whole `Esc`-precedence cascade (confirm > help > selection-cancel > outline-focused-quit > +//! focus-outline > quit — see `tui::update`'s doc comment). Per ADR-034 those are conventional +//! and safety-sensitive; they are never routed through the registry, so `Esc` is not a registry +//! token. use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; @@ -61,11 +62,20 @@ pub enum Command { PrevHunk, NextChangeset, PrevChangeset, + ExpandGap, + ExpandGapAll, + // Diff view. + FocusOutline, // Outline view. OutlineDown, OutlineUp, OutlineConfirm, OutlineCycleMode, + FocusDiff, + OutlineTop, + OutlineBottom, + OutlineStage, + OutlineDiscard, } /// One row of the action registry: a [`Command`] with its stable config identity (`view` + @@ -101,7 +111,7 @@ pub static REGISTRY: &[Registered] = &[ view: View::Global, name: "toggle-outline", default_keys: "o", - description: "Toggle the outline pane / focus", + description: "Show or hide the outline pane", }, Registered { command: Command::ToggleHelp, @@ -258,6 +268,27 @@ pub static REGISTRY: &[Registered] = &[ default_keys: "[c", description: "Go to the previous changeset", }, + Registered { + command: Command::FocusOutline, + view: View::Diff, + name: "focus-outline", + default_keys: "h left", + description: "Focus the outline", + }, + Registered { + command: Command::ExpandGap, + view: View::Diff, + name: "expand-gap", + default_keys: "enter", + description: "Reveal more of the collapsed gap under the cursor", + }, + Registered { + command: Command::ExpandGapAll, + view: View::Diff, + name: "expand-gap-all", + default_keys: "E", + description: "Reveal the whole collapsed gap under the cursor", + }, // ── Outline view ───────────────────────────────────────────────────────── Registered { command: Command::OutlineDown, @@ -287,6 +318,41 @@ pub static REGISTRY: &[Registered] = &[ default_keys: "i", description: "Cycle the outline mode", }, + Registered { + command: Command::FocusDiff, + view: View::Outline, + name: "focus-diff", + default_keys: "l right", + description: "Focus the diff view", + }, + Registered { + command: Command::OutlineTop, + view: View::Outline, + name: "scroll-top", + default_keys: "g", + description: "Jump to the top of the outline", + }, + Registered { + command: Command::OutlineBottom, + view: View::Outline, + name: "scroll-bottom", + default_keys: "G", + description: "Jump to the bottom of the outline", + }, + Registered { + command: Command::OutlineStage, + view: View::Outline, + name: "stage", + default_keys: "s", + description: "Stage or unstage the file/directory under the cursor", + }, + Registered { + command: Command::OutlineDiscard, + view: View::Outline, + name: "discard", + default_keys: "d", + description: "Discard the file/directory under the cursor", + }, ]; /// One matchable key press: a [`KeyCode`] plus whether Ctrl/Alt are required. **Shift is diff --git a/git-workon-review/src/lib.rs b/git-workon-review/src/lib.rs index cff5f735..48851b30 100644 --- a/git-workon-review/src/lib.rs +++ b/git-workon-review/src/lib.rs @@ -21,6 +21,7 @@ pub mod config; pub mod error; pub mod file_ops; pub mod highlight; +pub mod icons; pub mod keymap; pub mod model; pub mod ops; @@ -29,8 +30,10 @@ pub mod probe_cache; pub mod queue; pub mod refresh; pub mod render; +pub mod scope; pub mod source; pub mod stage_op; +pub mod summary; pub mod synthesis; pub mod terminal_query; pub mod theme; diff --git a/git-workon-review/src/model.rs b/git-workon-review/src/model.rs index 701d5021..06f6560e 100644 --- a/git-workon-review/src/model.rs +++ b/git-workon-review/src/model.rs @@ -101,6 +101,26 @@ pub enum FileStatus { Unmerged, } +impl FileStatus { + /// The single-character letter the outline's file rows render for this status (CS5): + /// `M`/`A`/`D`/`R`/`C`/`?`/`U`, mirroring `git status --short`'s XY letters where they exist + /// (`?` for untracked, `U` for unmerged/conflicted — git's own convention, not this crate's + /// invention). No mapping like this existed elsewhere in the crate before CS5 (checked the + /// winbar/header, which only special-cases `Renamed`/`Copied` for the `old -> new` label, + /// never prints a letter) — this is the canonical one going forward. + pub fn letter(self) -> char { + match self { + FileStatus::Modified => 'M', + FileStatus::Added => 'A', + FileStatus::Deleted => 'D', + FileStatus::Renamed => 'R', + FileStatus::Copied => 'C', + FileStatus::Untracked => '?', + FileStatus::Unmerged => 'U', + } + } +} + impl From for FileStatus { fn from(delta: git2::Delta) -> Self { match delta { diff --git a/git-workon-review/src/outline.rs b/git-workon-review/src/outline.rs index f4328087..4f7206ef 100644 --- a/git-workon-review/src/outline.rs +++ b/git-workon-review/src/outline.rs @@ -3,12 +3,20 @@ //! renders and the outline cursor indexes — no [`crate::app::App`]/[`crate::app::ChangesetView`] //! dependency, mirroring how [`crate::attribute`] stays a pure module consumed by `app`/`render`. //! -//! CS3 shipped two of the four modes ([`OutlineMode::Flat`]/[`OutlineMode::Stack`]); CS4 (this -//! revision) adds the two path-trie modes ([`OutlineMode::Tree`]/[`OutlineMode::StackTree`]) via -//! the private [`TrieNode`] builder below. +//! CS3 shipped two of the four modes ([`OutlineMode::Flat`]/[`OutlineMode::Stack`]); CS4 added +//! the two path-trie modes ([`OutlineMode::Tree`]/[`OutlineMode::StackTree`]) via the private +//! [`TrieNode`] builder below. CS5 adds each file row's [`crate::model::FileStatus`] (the `M`/ +//! `A`/`D`/... change-status letter — see [`OutlineFile::change`]/[`OutlineItem::File::change`]'s +//! doc comments for why that's a wholly separate field from [`StagedStatus`], which tracks +//! index/worktree staged-ness, not the underlying change kind). Pulling in +//! `crate::model::FileStatus` keeps this module's pure-data posture intact: `model.rs` is itself +//! a pure data module (no `App`/`ChangesetView` dependency), so importing its plain enum doesn't +//! reintroduce the `App` coupling this module was factored out to avoid. use std::collections::HashMap; +use crate::model::FileStatus; + /// Which of the outline's row-building strategies is active — cycled by `i` (only while the /// outline pane has focus; see `App::outline_cycle_mode`). #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -43,6 +51,35 @@ impl OutlineMode { } } +/// Which end of the stack the outline's stack-shaped modes ([`OutlineMode::Stack`]/ +/// [`OutlineMode::StackTree`]) display first — CS3 dogfooding feedback #2. Purely a display +/// order: [`OutlineItem`]'s `cs_idx`/`file_idx` always stay TRUE indices into `App::changesets` +/// regardless of which way the rows are painted (see [`build_items`]'s doc comment). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum OutlineOrder { + /// The most recently created (head) changeset's header renders first — the CS3 default. + #[default] + HeadFirst, + /// The stack's base changeset renders first, matching `App::changesets`' own base -> head + /// storage order (today's pre-CS3 behavior). + BaseFirst, +} + +/// Enumerate `changesets` in `order`'s display scan — the shared preamble of every stack-shaped +/// builder below. Indices are always TRUE base -> head indices into the slice regardless of scan +/// direction (enumerate happens BEFORE any reversal), which is the invariant `cs_idx`/`file_idx` +/// consumers like `App::switch_changeset` rely on. +fn scan_order( + changesets: &[OutlineChangeset], + order: OutlineOrder, +) -> Vec<(usize, &OutlineChangeset)> { + let mut entries: Vec<(usize, &OutlineChangeset)> = changesets.iter().enumerate().collect(); + if order == OutlineOrder::HeadFirst { + entries.reverse(); + } + entries +} + /// A file's staged-ness for the outline's status column — a minimal indicator (locked CS3 /// scope: NOT the prototype's X/Y two-column git-status matrix). Only meaningful for the /// uncommitted changeset's files; a committed changeset's files always resolve to `None` @@ -94,7 +131,15 @@ impl StagedStatus { #[derive(Debug, Clone)] pub struct OutlineFile { pub path: String, + /// Index/worktree staged-ness — [`StagedStatus::None`] for a committed changeset's files. + /// NOT the same axis as [`Self::change`]: a file can be `Staged` (this field) while its + /// underlying change is `Deleted` (that one) — they answer different questions ("is it + /// staged" vs. "what kind of change is it") and must stay two distinct fields. pub status: StagedStatus, + /// CS5: the underlying change kind (Modified/Added/Deleted/...), lifted from the owning + /// [`crate::model::FileChange::status`] — drives the outline's `M`/`A`/`D`/`R`/`C`/`?`/`U` + /// letter (`render::build_outline_line`), independent of [`Self::status`] above. + pub change: FileStatus, } /// One changeset's outline-relevant data — a snapshot, not a borrow, so this module never needs @@ -144,11 +189,24 @@ pub enum OutlineItem { failed: bool, }, /// A directory row — only emitted in [`OutlineMode::Tree`]/[`OutlineMode::StackTree`]. Not a - /// jump target: it carries no `cs_idx`/`file_idx`, so `App::outline_move_by` no-ops on it - /// (same as [`Self::Header`]) and `App::outline_confirm` also no-ops on it (CS4 decision — - /// there's no expand/collapse state to toggle, so Enter on a directory row does nothing but - /// still returns focus to the diff, matching every other confirm outcome). - Dir { name: String, guides: Vec }, + /// jump target: it carries no `file_idx`, so `App::outline_move_by` no-ops on it (same as + /// [`Self::Header`]) and `App::outline_confirm` also no-ops on it (CS4 decision — there's no + /// expand/collapse state to toggle, so Enter on a directory row does nothing but still + /// returns focus to the diff, matching every other confirm outcome). + Dir { + name: String, + /// The FULL path from the trie root (e.g. `"src/cmd"`), unlike `name` which is just the + /// leaf segment — CS4's summary panel needs the whole path to filter files under this + /// directory (see `crate::summary::dir_summary`). + path: String, + /// `Some(cs_idx)` when this row's trie is per-changeset ([`OutlineMode::StackTree`] — + /// the same true index its owning [`Self::Header`] carries); `None` in the cross-stack + /// [`OutlineMode::Tree`], whose single trie spans every changeset (so a dir row there has + /// no single owning changeset to scope a summary to — CS4's `App::summary_for` instead + /// aggregates over [`latest_by_path`]'s de-duped set for that case). + cs_idx: Option, + guides: Vec, + }, /// A file row — the target of every outline->diff jump. `path` is the FULL path in /// [`OutlineMode::Flat`]/[`OutlineMode::Stack`] (unchanged from CS3), but is just the leaf /// segment in [`OutlineMode::Tree`]/[`OutlineMode::StackTree`] — the ancestor directory rows @@ -158,6 +216,9 @@ pub enum OutlineItem { file_idx: usize, path: String, status: StagedStatus, + /// CS5: the change kind (Modified/Added/Deleted/...) — see [`OutlineFile::change`]'s doc + /// comment on why this is distinct from `status` above. + change: FileStatus, guides: Vec, }, } @@ -175,23 +236,33 @@ impl OutlineItem { } } -/// Build the outline's row list for `mode` from every reviewed changeset, in the same base -> -/// head order `App::changesets` holds them. -pub fn build_items(changesets: &[OutlineChangeset], mode: OutlineMode) -> Vec { +/// Build the outline's row list for `mode` from every reviewed changeset. `order` controls which +/// end of the stack displays first for the stack-shaped modes (see [`OutlineOrder`]); `cs_idx`/ +/// `file_idx` on every emitted [`OutlineItem`] are always TRUE indices into `App::changesets` +/// (that array's own base -> head storage order never changes) regardless of `order` — only the +/// ROW SEQUENCE the outline paints flips. [`build_tree`]'s de-dupe is order-independent (see its +/// own doc comment), so `order` is accepted but unused there. +pub fn build_items( + changesets: &[OutlineChangeset], + mode: OutlineMode, + order: OutlineOrder, +) -> Vec { match mode { - OutlineMode::Flat => build_flat(changesets), - OutlineMode::Stack => build_stack(changesets), + OutlineMode::Flat => build_flat(changesets, order), + OutlineMode::Stack => build_stack(changesets, order), OutlineMode::Tree => build_tree(changesets), - OutlineMode::StackTree => build_stack_tree(changesets), + OutlineMode::StackTree => build_stack_tree(changesets, order), } } /// [`OutlineMode::Stack`]: a header per changeset, then its files in order — no de-duplication, /// every changeset's own copy of a path (if touched more than once across the stack) gets its -/// own row under its own header. -fn build_stack(changesets: &[OutlineChangeset]) -> Vec { +/// own row under its own header. `order` picks which end of the stack paints first; `cs_idx`/ +/// `file_idx` are computed from the ORIGINAL (base -> head) enumeration before any reversal, so +/// they stay true indices into `App::changesets` either way. +fn build_stack(changesets: &[OutlineChangeset], order: OutlineOrder) -> Vec { let mut items = Vec::new(); - for (cs_idx, cs) in changesets.iter().enumerate() { + for (cs_idx, cs) in scan_order(changesets, order) { items.push(OutlineItem::Header { cs_idx, label: cs.label.clone(), @@ -206,6 +277,7 @@ fn build_stack(changesets: &[OutlineChangeset]) -> Vec { file_idx, path: file.path.clone(), status: file.status, + change: file.change, guides: Vec::new(), }); } @@ -213,32 +285,36 @@ fn build_stack(changesets: &[OutlineChangeset]) -> Vec { items } -/// [`OutlineMode::Flat`]: every changed path once, in FIRST-appearance order (a stable, readable -/// order that doesn't reshuffle just because a later changeset re-touches an earlier path), but -/// pointing at its LAST (newest / closest-to-head) occurrence — "last-write-wins" per the locked -/// design: a path touched by both an earlier committed changeset and the uncommitted layer -/// should jump to (and show the staged-ness of) the uncommitted layer's copy, not the stale -/// committed one. -fn build_flat(changesets: &[OutlineChangeset]) -> Vec { - let mut order: Vec = Vec::new(); - let mut latest: HashMap = HashMap::new(); - for (cs_idx, cs) in changesets.iter().enumerate() { - for (file_idx, file) in cs.files.iter().enumerate() { - if !latest.contains_key(&file.path) { - order.push(file.path.clone()); +/// [`OutlineMode::Flat`]: every changed path once, in FIRST-appearance order UNDER `order`'s +/// display scan (a stable, readable order that doesn't reshuffle just because a later-scanned +/// changeset re-touches an earlier path), but pointing at its closest-to-head occurrence — +/// "last-write-wins" per the locked design: a path touched by both an earlier committed +/// changeset and the uncommitted layer should jump to (and show the staged-ness of) the +/// uncommitted layer's copy, not the stale committed one. This head-wins target resolution is +/// independent of `order` — [`latest_by_path`] always scans base -> head regardless of which way +/// the row list is displayed, so the resolution below reuses it rather than re-deriving from the +/// (possibly reversed) `order` scan used for display order. +fn build_flat(changesets: &[OutlineChangeset], order: OutlineOrder) -> Vec { + let latest = latest_by_path(changesets); + let mut order_list: Vec = Vec::new(); + let mut seen: std::collections::HashSet<&str> = std::collections::HashSet::new(); + for (_, cs) in scan_order(changesets, order) { + for file in &cs.files { + if seen.insert(file.path.as_str()) { + order_list.push(file.path.clone()); } - latest.insert(file.path.clone(), (cs_idx, file_idx, file.status)); } } - order + order_list .into_iter() .map(|path| { - let (cs_idx, file_idx, status) = latest[&path]; + let occ = latest[&path]; OutlineItem::File { - cs_idx, - file_idx, + cs_idx: occ.cs_idx, + file_idx: occ.file_idx, path, - status, + status: occ.status, + change: occ.change, guides: Vec::new(), } }) @@ -248,31 +324,54 @@ fn build_flat(changesets: &[OutlineChangeset]) -> Vec { /// De-dupe every changed path across the stack to its LAST occurrence (mirrors /// [`build_flat`]'s last-write-wins rule), independent of iteration/insertion order — the trie /// builders below re-sort by path segment anyway, so no stable-order bookkeeping is needed here. -fn latest_by_path( - changesets: &[OutlineChangeset], -) -> HashMap { +/// +/// `pub(crate)`: CS4's `App::summary_for` reuses this directly to aggregate a +/// [`OutlineMode::Tree`] directory's files (`cs_idx: None` on that mode's [`OutlineItem::Dir`] +/// rows) over the same last-write-wins de-duped set the Tree outline itself displays, rather than +/// re-deriving the dedup logic in `app.rs`. +pub(crate) fn latest_by_path(changesets: &[OutlineChangeset]) -> HashMap { let mut latest = HashMap::new(); for (cs_idx, cs) in changesets.iter().enumerate() { for (file_idx, file) in cs.files.iter().enumerate() { - latest.insert(file.path.clone(), (cs_idx, file_idx, file.status)); + latest.insert( + file.path.clone(), + FileOccurrence { + cs_idx, + file_idx, + status: file.status, + change: file.change, + }, + ); } } latest } +/// The file a de-duped path (or a trie leaf) resolves to: its true `(cs_idx, file_idx)` address +/// into `App::changesets` plus the two per-file status axes the outline renders — staged-ness +/// ([`StagedStatus`], CS3) and change kind ([`FileStatus`], CS5). Named because the bare 4-tuple +/// it replaced had to be re-explained (and type-annotated) at every use site. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct FileOccurrence { + pub cs_idx: usize, + pub file_idx: usize, + pub status: StagedStatus, + pub change: FileStatus, +} + /// A node in the path trie the tree modes build. A node with `file.is_some()` is a leaf (a /// changed file at that exact path); otherwise it's a pure directory node. Git paths never /// collide a file and a directory at the same path, so a node is never both. #[derive(Debug, Default)] struct TrieNode { - file: Option<(usize, usize, StagedStatus)>, - /// Insertion order is irrelevant — [`emit`] re-sorts children (dirs-after-files, alpha + file: Option, + /// Insertion order is irrelevant — [`emit`] re-sorts children (dirs-before-files, alpha /// within group) every time it flattens a node. children: Vec<(String, TrieNode)>, } impl TrieNode { - fn insert(&mut self, segments: &[&str], cs_idx: usize, file_idx: usize, status: StagedStatus) { + fn insert(&mut self, segments: &[&str], occ: FileOccurrence) { let (head, rest) = segments .split_first() .expect("insert is never called with an empty segment list"); @@ -286,20 +385,25 @@ impl TrieNode { let child = &mut self.children[idx].1; if rest.is_empty() { // Last-write-wins: a later insert of the same full path overwrites the leaf data. - child.file = Some((cs_idx, file_idx, status)); + child.file = Some(occ); } else { - child.insert(rest, cs_idx, file_idx, status); + child.insert(rest, occ); } } } -/// Flatten `node`'s children into `items`, depth-first, in "dirs after files at each level, -/// alpha within group" order (matches the `~/.config/nvim/lua/app/review/ui/outline.lua` -/// prototype's `_build_path_tree`/`_emit_tree_node`: files read before directories at a given -/// level, so a directory's own contents don't visually separate its sibling files from the -/// directory listing above them). `ancestors_last` is the growing guide vector — see -/// [`OutlineItem`]'s doc comment for how rendering consumes it. -fn emit(node: &TrieNode, ancestors_last: &[bool], items: &mut Vec) { +/// Flatten `node`'s children into `items`, depth-first, in "dirs before files at each level, +/// alpha within group" order (CS3 dogfooding feedback #7: directories read before files at a +/// given level, matching the conventional file-tree convention of grouping folders above +/// loose files). `ancestors_last` is the growing guide vector — see [`OutlineItem`]'s doc +/// comment for how rendering consumes it. +fn emit( + node: &TrieNode, + ancestors_last: &[bool], + path_prefix: &str, + dir_cs_idx: Option, + items: &mut Vec, +) { let mut files: Vec<&(String, TrieNode)> = node .children .iter() @@ -312,54 +416,66 @@ fn emit(node: &TrieNode, ancestors_last: &[bool], items: &mut Vec) .collect(); files.sort_by(|a, b| a.0.cmp(&b.0)); dirs.sort_by(|a, b| a.0.cmp(&b.0)); - let ordered: Vec<&(String, TrieNode)> = files.into_iter().chain(dirs).collect(); + let ordered: Vec<&(String, TrieNode)> = dirs.into_iter().chain(files).collect(); let n = ordered.len(); for (i, (name, child)) in ordered.into_iter().enumerate() { let is_last = i == n - 1; let mut guides = ancestors_last.to_vec(); guides.push(is_last); match child.file { - Some((cs_idx, file_idx, status)) => { + Some(occ) => { items.push(OutlineItem::File { - cs_idx, - file_idx, + cs_idx: occ.cs_idx, + file_idx: occ.file_idx, path: name.clone(), - status, + status: occ.status, + change: occ.change, guides, }); } None => { + let full_path = if path_prefix.is_empty() { + name.clone() + } else { + format!("{path_prefix}/{name}") + }; items.push(OutlineItem::Dir { name: name.clone(), + path: full_path.clone(), + cs_idx: dir_cs_idx, guides: guides.clone(), }); - emit(child, &guides, items); + emit(child, &guides, &full_path, dir_cs_idx, items); } } } } /// [`OutlineMode::Tree`]: [`build_flat`]'s de-duped path set, rendered as a single directory -/// trie spanning the whole stack (no changeset headers). +/// trie spanning the whole stack (no changeset headers). Alpha-sorted by path segment at every +/// level ([`emit`]), not by stack position, and [`latest_by_path`]'s de-dupe always resolves to +/// the closest-to-head occurrence regardless of scan order — so [`OutlineOrder`] has nothing to +/// affect here, and unlike the stack-shaped builders this one takes no `order` parameter. fn build_tree(changesets: &[OutlineChangeset]) -> Vec { let latest = latest_by_path(changesets); let mut root = TrieNode::default(); - for (path, (cs_idx, file_idx, status)) in &latest { + for (path, occ) in &latest { let segments: Vec<&str> = path.split('/').collect(); - root.insert(&segments, *cs_idx, *file_idx, *status); + root.insert(&segments, *occ); } let mut items = Vec::new(); - emit(&root, &[], &mut items); + emit(&root, &[], "", None, &mut items); items } /// [`OutlineMode::StackTree`]: [`build_stack`]'s per-changeset header grouping, but each /// changeset's own files are flattened into their own nested trie (no cross-changeset dedup — /// each changeset trie is built from just that changeset's files, matching `build_stack`'s "every -/// changeset's own copy gets its own row" rule). -fn build_stack_tree(changesets: &[OutlineChangeset]) -> Vec { +/// changeset's own copy gets its own row" rule). `order` picks which end of the stack paints +/// first, same as [`build_stack`]; `cs_idx`/`file_idx` stay true indices regardless. +fn build_stack_tree(changesets: &[OutlineChangeset], order: OutlineOrder) -> Vec { let mut items = Vec::new(); - for (cs_idx, cs) in changesets.iter().enumerate() { + for (cs_idx, cs) in scan_order(changesets, order) { items.push(OutlineItem::Header { cs_idx, label: cs.label.clone(), @@ -371,9 +487,17 @@ fn build_stack_tree(changesets: &[OutlineChangeset]) -> Vec { let mut root = TrieNode::default(); for (file_idx, file) in cs.files.iter().enumerate() { let segments: Vec<&str> = file.path.split('/').collect(); - root.insert(&segments, cs_idx, file_idx, file.status); + root.insert( + &segments, + FileOccurrence { + cs_idx, + file_idx, + status: file.status, + change: file.change, + }, + ); } - emit(&root, &[], &mut items); + emit(&root, &[], "", Some(cs_idx), &mut items); } items } @@ -382,11 +506,32 @@ fn build_stack_tree(changesets: &[OutlineChangeset]) -> Vec { mod tests { use super::*; + /// `change` defaults to [`FileStatus::Modified`] for every file — the ordinary case, and + /// irrelevant to the order/dedup/depth semantics these tests exercise. Tests that care about + /// a SPECIFIC change status (dedup target resolution) use [`cs_with_change`] instead. fn cs( label: &str, current: bool, needs_restack: bool, files: &[(&str, StagedStatus)], + ) -> OutlineChangeset { + cs_with_change( + label, + current, + needs_restack, + &files + .iter() + .map(|(p, s)| (*p, *s, FileStatus::Modified)) + .collect::>(), + ) + } + + /// [`cs`] variant that lets a test pin each file's [`FileStatus`] explicitly (CS5). + fn cs_with_change( + label: &str, + current: bool, + needs_restack: bool, + files: &[(&str, StagedStatus, FileStatus)], ) -> OutlineChangeset { OutlineChangeset { label: label.to_string(), @@ -396,9 +541,10 @@ mod tests { failed: false, files: files .iter() - .map(|(p, s)| OutlineFile { + .map(|(p, s, c)| OutlineFile { path: p.to_string(), status: *s, + change: *c, }) .collect(), } @@ -423,7 +569,10 @@ mod tests { cs("cs-a", false, false, &[("a1.txt", StagedStatus::None)]), cs("cs-b", true, true, &[("b1.txt", StagedStatus::None)]), ]; - let items = build_items(&changesets, OutlineMode::Stack); + // BaseFirst pins the base -> head structural rule (header-then-files per changeset) + // independent of display order; head-first order coverage lives in the dedicated + // `stack_mode_*_order` tests below. + let items = build_items(&changesets, OutlineMode::Stack, OutlineOrder::BaseFirst); assert_eq!( items, vec![ @@ -440,6 +589,7 @@ mod tests { file_idx: 0, path: "a1.txt".to_string(), status: StagedStatus::None, + change: FileStatus::Modified, guides: Vec::new(), }, OutlineItem::Header { @@ -455,6 +605,7 @@ mod tests { file_idx: 0, path: "b1.txt".to_string(), status: StagedStatus::None, + change: FileStatus::Modified, guides: Vec::new(), }, ] @@ -469,7 +620,7 @@ mod tests { cs_slot("cs-pending", true, false), cs_slot("cs-failed", false, true), ]; - let items = build_items(&changesets, OutlineMode::Stack); + let items = build_items(&changesets, OutlineMode::Stack, OutlineOrder::BaseFirst); assert_eq!( items, vec![ @@ -506,7 +657,7 @@ mod tests { ("a2.txt", StagedStatus::None), ], )]; - let items = build_items(&changesets, OutlineMode::Flat); + let items = build_items(&changesets, OutlineMode::Flat, OutlineOrder::HeadFirst); assert!(items .iter() .all(|it| matches!(it, OutlineItem::File { .. }))); @@ -524,7 +675,7 @@ mod tests { &[("shared.txt", StagedStatus::Unstaged)], ), ]; - let items = build_items(&changesets, OutlineMode::Flat); + let items = build_items(&changesets, OutlineMode::Flat, OutlineOrder::HeadFirst); assert_eq!(items.len(), 1, "the shared path must appear exactly once"); assert_eq!( items[0], @@ -533,6 +684,7 @@ mod tests { file_idx: 0, path: "shared.txt".to_string(), status: StagedStatus::Unstaged, + change: FileStatus::Modified, guides: Vec::new(), }, "must point at cs-b (the LATER/newer changeset), not cs-a" @@ -553,7 +705,10 @@ mod tests { ), cs("cs-b", true, false, &[("shared.txt", StagedStatus::Staged)]), ]; - let items = build_items(&changesets, OutlineMode::Flat); + // BaseFirst scans cs-a before cs-b, so "first appearance" here means base -> head scan + // order; the head-first display-order variant lives in + // `flat_mode_head_first_scans_head_to_base_but_keeps_head_wins_target` below. + let items = build_items(&changesets, OutlineMode::Flat, OutlineOrder::BaseFirst); let paths: Vec<&str> = items .iter() .map(|it| match it { @@ -568,6 +723,54 @@ mod tests { ); } + /// CS3: [`OutlineOrder::HeadFirst`] flips [`build_flat`]'s DISPLAY scan (first-appearance + /// order now reads head -> base), but [`latest_by_path`]'s "closest-to-head wins" TARGET + /// resolution never changes — a path touched by two changesets must resolve to the head-most + /// one under BOTH orders. + #[test] + fn flat_mode_head_first_scans_head_to_base_but_keeps_head_wins_target() { + let changesets = vec![ + cs( + "cs-a", + false, + false, + &[ + ("first.txt", StagedStatus::None), + ("shared.txt", StagedStatus::None), + ], + ), + cs("cs-b", true, false, &[("shared.txt", StagedStatus::Staged)]), + ]; + let items = build_items(&changesets, OutlineMode::Flat, OutlineOrder::HeadFirst); + let paths: Vec<&str> = items + .iter() + .map(|it| match it { + OutlineItem::File { path, .. } => path.as_str(), + OutlineItem::Header { .. } | OutlineItem::Dir { .. } => unreachable!(), + }) + .collect(); + assert_eq!( + paths, + vec!["shared.txt", "first.txt"], + "head-first display scans cs-b (head) before cs-a (base), so shared.txt is seen \ + first" + ); + assert_eq!( + items + .iter() + .find(|it| matches!(it, OutlineItem::File { path, .. } if path == "shared.txt")), + Some(&OutlineItem::File { + cs_idx: 1, + file_idx: 0, + path: "shared.txt".to_string(), + status: StagedStatus::Staged, + change: FileStatus::Modified, + guides: Vec::new(), + }), + "target resolution stays head-wins (cs-b) regardless of display order" + ); + } + #[test] fn staged_status_from_flags_covers_the_truth_table() { assert_eq!(StagedStatus::from_flags(false, false), StagedStatus::None); @@ -589,7 +792,7 @@ mod tests { /// Deep-path fixture used by the tree-mode tests: a top-level file, a top-level directory /// with both its own file and a nested subdirectory of two more files — enough to exercise - /// depth > 1 and the dirs-after-files/alpha-within-group ordering at every level. + /// depth > 1 and the dirs-before-files/alpha-within-group ordering at every level. fn deep_path_changeset(label: &str, current: bool, needs_restack: bool) -> OutlineChangeset { cs( label, @@ -605,56 +808,64 @@ mod tests { } #[test] - fn tree_mode_builds_dirs_after_files_alpha_within_group_with_correct_depth_and_guides() { + fn tree_mode_builds_dirs_before_files_alpha_within_group_with_correct_depth_and_guides() { let changesets = vec![deep_path_changeset("cs-a", true, false)]; - let items = build_items(&changesets, OutlineMode::Tree); + let items = build_items(&changesets, OutlineMode::Tree, OutlineOrder::HeadFirst); assert_eq!( items, vec![ - OutlineItem::File { - cs_idx: 0, - file_idx: 0, - path: "top.rs".to_string(), - status: StagedStatus::None, - guides: vec![false], - }, OutlineItem::Dir { name: "src".to_string(), - guides: vec![true], - }, - OutlineItem::File { - cs_idx: 0, - file_idx: 3, - path: "d.rs".to_string(), - status: StagedStatus::None, - guides: vec![true, false], + path: "src".to_string(), + cs_idx: None, + guides: vec![false], }, OutlineItem::Dir { name: "a".to_string(), - guides: vec![true, true], + path: "src/a".to_string(), + cs_idx: None, + guides: vec![false, false], }, OutlineItem::File { cs_idx: 0, file_idx: 1, path: "b.rs".to_string(), status: StagedStatus::None, - guides: vec![true, true, false], + change: FileStatus::Modified, + guides: vec![false, false, false], }, OutlineItem::File { cs_idx: 0, file_idx: 2, path: "c.rs".to_string(), status: StagedStatus::None, - guides: vec![true, true, true], + change: FileStatus::Modified, + guides: vec![false, false, true], + }, + OutlineItem::File { + cs_idx: 0, + file_idx: 3, + path: "d.rs".to_string(), + status: StagedStatus::None, + change: FileStatus::Modified, + guides: vec![false, true], + }, + OutlineItem::File { + cs_idx: 0, + file_idx: 0, + path: "top.rs".to_string(), + status: StagedStatus::None, + change: FileStatus::Modified, + guides: vec![true], }, ], - "root: top.rs (file) then src/ (dir); under src/: d.rs (file) then a/ (dir); \ - under src/a/: b.rs then c.rs — files-before-dirs, alpha within each group" + "root: src/ (dir) then top.rs (file); under src/: a/ (dir) then d.rs (file); \ + under src/a/: b.rs then c.rs — dirs-before-files, alpha within each group" ); - assert_eq!(items[0].depth(), 0, "top.rs is a root-level row"); - assert_eq!(items[1].depth(), 0, "src/ is a root-level row"); - assert_eq!(items[2].depth(), 1, "src/d.rs is one level deep"); - assert_eq!(items[4].depth(), 2, "src/a/b.rs is two levels deep"); + assert_eq!(items[0].depth(), 0, "src/ is a root-level row"); + assert_eq!(items[1].depth(), 1, "src/a/ is one level deep"); + assert_eq!(items[2].depth(), 2, "src/a/b.rs is two levels deep"); + assert_eq!(items[5].depth(), 0, "top.rs is a root-level row"); } #[test] @@ -663,7 +874,7 @@ mod tests { cs("cs-a", false, false, &[("shared.txt", StagedStatus::None)]), cs("cs-b", true, false, &[("shared.txt", StagedStatus::Staged)]), ]; - let items = build_items(&changesets, OutlineMode::Tree); + let items = build_items(&changesets, OutlineMode::Tree, OutlineOrder::HeadFirst); assert_eq!( items, vec![OutlineItem::File { @@ -671,6 +882,7 @@ mod tests { file_idx: 0, path: "shared.txt".to_string(), status: StagedStatus::Staged, + change: FileStatus::Modified, guides: vec![true], }], "the shared path must appear exactly once, pointing at the newer changeset" @@ -683,7 +895,7 @@ mod tests { cs("cs-a", false, false, &[("x/y.txt", StagedStatus::None)]), cs("cs-b", true, true, &[("z.txt", StagedStatus::Unstaged)]), ]; - let items = build_items(&changesets, OutlineMode::StackTree); + let items = build_items(&changesets, OutlineMode::StackTree, OutlineOrder::BaseFirst); assert_eq!( items, vec![ @@ -697,6 +909,8 @@ mod tests { }, OutlineItem::Dir { name: "x".to_string(), + path: "x".to_string(), + cs_idx: Some(0), guides: vec![true], }, OutlineItem::File { @@ -704,6 +918,7 @@ mod tests { file_idx: 0, path: "y.txt".to_string(), status: StagedStatus::None, + change: FileStatus::Modified, guides: vec![true, true], }, OutlineItem::Header { @@ -719,6 +934,7 @@ mod tests { file_idx: 0, path: "z.txt".to_string(), status: StagedStatus::Unstaged, + change: FileStatus::Modified, guides: vec![true], }, ], @@ -726,4 +942,95 @@ mod tests { with no cross-changeset dedup" ); } + + /// CS3: [`OutlineOrder::HeadFirst`] (the new default) shows the LAST changeset's ([`cs-c`], + /// index 2 — the true, base-> head `App::changesets` index) header FIRST, while its `cs_idx` + /// still equals its true index into `changesets` (2), never a display-order index (0). + #[test] + fn stack_mode_head_first_shows_last_changesets_header_first_with_true_cs_idx() { + let changesets = vec![ + cs("cs-a", false, false, &[("a1.txt", StagedStatus::None)]), + cs("cs-b", false, false, &[("b1.txt", StagedStatus::None)]), + cs("cs-c", true, false, &[("c1.txt", StagedStatus::None)]), + ]; + let items = build_items(&changesets, OutlineMode::Stack, OutlineOrder::HeadFirst); + assert_eq!( + items[0], + OutlineItem::Header { + cs_idx: 2, + label: "cs-c".to_string(), + current: true, + needs_restack: false, + loading: false, + failed: false, + }, + "head-first: the LAST (head) changeset's header renders first, carrying its TRUE \ + index (2) into `changesets`, not a display-order index" + ); + let labels: Vec<&str> = items + .iter() + .filter_map(|it| match it { + OutlineItem::Header { label, .. } => Some(label.as_str()), + _ => None, + }) + .collect(); + assert_eq!( + labels, + vec!["cs-c", "cs-b", "cs-a"], + "head-first header order is exactly the reverse of `changesets`' base -> head order" + ); + } + + /// CS3: [`OutlineOrder::BaseFirst`] restores the pre-CS3 base -> head header order. + #[test] + fn stack_mode_base_first_restores_base_to_head_header_order() { + let changesets = vec![ + cs("cs-a", false, false, &[("a1.txt", StagedStatus::None)]), + cs("cs-b", true, false, &[("b1.txt", StagedStatus::None)]), + ]; + let items = build_items(&changesets, OutlineMode::Stack, OutlineOrder::BaseFirst); + let labels: Vec<&str> = items + .iter() + .filter_map(|it| match it { + OutlineItem::Header { label, .. } => Some(label.as_str()), + _ => None, + }) + .collect(); + assert_eq!(labels, vec!["cs-a", "cs-b"]); + } + + /// [`OutlineMode::StackTree`] analog of + /// `stack_mode_head_first_shows_last_changesets_header_first_with_true_cs_idx`. + #[test] + fn stack_tree_mode_head_first_shows_last_changesets_header_first_with_true_cs_idx() { + let changesets = vec![ + cs("cs-a", false, false, &[("x/y.txt", StagedStatus::None)]), + cs("cs-b", true, false, &[("z.txt", StagedStatus::None)]), + ]; + let items = build_items(&changesets, OutlineMode::StackTree, OutlineOrder::HeadFirst); + assert_eq!( + items[0], + OutlineItem::Header { + cs_idx: 1, + label: "cs-b".to_string(), + current: true, + needs_restack: false, + loading: false, + failed: false, + }, + "head-first: cs-b's header renders first, carrying its true index (1)" + ); + assert_eq!( + items[1], + OutlineItem::File { + cs_idx: 1, + file_idx: 0, + path: "z.txt".to_string(), + status: StagedStatus::None, + change: FileStatus::Modified, + guides: vec![true], + }, + "cs-b's own file follows immediately under its head-first header" + ); + } } diff --git a/git-workon-review/src/queue.rs b/git-workon-review/src/queue.rs index fbed8bfb..282997cd 100644 --- a/git-workon-review/src/queue.rs +++ b/git-workon-review/src/queue.rs @@ -68,6 +68,16 @@ pub trait StagingOp: Send { fn run(&mut self, ctx: &OpContext<'_>) -> Result<(), ApplyError>; } +/// Lets an already-boxed trait object be re-enqueued through [`StagingQueue::enqueue`] (which +/// takes `impl StagingOp + 'static` and boxes internally) without unboxing first — CS7's +/// `App::run_ops` collects a `Vec>` of heterogeneous per-file ops (one +/// [`crate::stage_op::FileStagingOp`] per outline target) and enqueues them one at a time. +impl StagingOp for Box { + fn run(&mut self, ctx: &OpContext<'_>) -> Result<(), ApplyError> { + (**self).run(ctx) + } +} + /// The result of running one queued op. #[derive(Debug)] pub enum OpOutcome { diff --git a/git-workon-review/src/render.rs b/git-workon-review/src/render.rs index a2cd0845..d536cd7e 100644 --- a/git-workon-review/src/render.rs +++ b/git-workon-review/src/render.rs @@ -13,13 +13,17 @@ use ratatui::widgets::{Block, Borders, Clear, Paragraph}; use ratatui::Frame; use crate::align::{CellKind, DisplayRow, InlineRow, Row}; -use crate::app::{App, EffectiveZoom, FileView, Layout as AppLayout, Notice, Role, Severity}; +use crate::app::{ + App, EffectiveZoom, FileView, Layout as AppLayout, Notice, Role, Severity, Summary, +}; use crate::attribute::Attribution; use crate::config::View; use crate::highlight::FgSpan; +use crate::icons::OutlineIcons; use crate::keymap::{footer_hint, help_sections, Keymap}; use crate::model::FileStatus; use crate::outline::OutlineItem; +use crate::summary::{ChangesetSummary, DirSummary, SummaryFileRow}; use crate::theme::Palette; use crate::wordiff::Span as WordSpan; @@ -472,20 +476,20 @@ fn render_help_overlay(frame: &mut Frame, app: &App, keymap: &Keymap, area: Rect /// the path. The cursor row (the outline's OWN cursor — a separate coordinate space from the /// diff's [`App::cursor`]) gets the theme's cursor tint while the outline has focus, or the dimmer /// [`Palette::outline_cursor_unfocused_bg`] while it's merely open (so the remembered position stays -/// legible even after focus returns to the diff). -fn render_outline(frame: &mut Frame, app: &App, area: Rect, theme: &Palette) { +/// legible even after focus returns to the diff). `&mut App` (CS2, precedent: [`render_body`] +/// writing [`App::pane_height`]) — writes [`App::outline_height`] and re-derives +/// [`App::derive_outline_scroll`] before painting from `app.outline.scroll`, giving the outline +/// the same stateful scrolloff-margined viewport the diff panes already have, instead of the old +/// transient bottom-anchor scroll computed fresh each frame. +fn render_outline(frame: &mut Frame, app: &mut App, area: Rect, theme: &Palette) { + app.outline_height = area.height as usize; let items = app.outline_items(); + app.derive_outline_scroll(items.len()); + let cursor = app.outline_cursor(); let focused = app.outline_focused(); - - let visible_h = area.height as usize; - let scroll = if visible_h == 0 { - 0 - } else if cursor >= visible_h { - cursor + 1 - visible_h - } else { - 0 - }; + let scroll = app.outline_scroll(); + let icons = app.outline_icons(); let buf = frame.buffer_mut(); for row in 0..area.height { @@ -495,7 +499,7 @@ fn render_outline(frame: &mut Frame, app: &App, area: Rect, theme: &Palette) { continue; }; let is_cursor = item_idx == cursor; - let line = build_outline_line(item, theme); + let line = build_outline_line(item, theme, icons); let line = if is_cursor && focused { apply_cursor_row(line, area.width, theme) } else if is_cursor { @@ -527,9 +531,26 @@ fn tree_prefix(guides: &[bool]) -> String { s } +/// The [`FileStatus`] change-letter's foreground color (CS5): a create-like status (Added/ +/// Untracked) reuses the theme's `add_strong` tint, a destroy-like status (Deleted) reuses +/// `del_strong`, and everything else (Modified/Renamed/Copied/Unmerged — a change to EXISTING +/// content, not a create/destroy) gets the theme's neutral `foreground`. No new [`Palette`] +/// fields — this is deliberately just a remap of tints CS4's summary rows already use. +fn change_letter_color(change: FileStatus, theme: &Palette) -> Color { + match change { + FileStatus::Added | FileStatus::Untracked => theme.add_strong, + FileStatus::Deleted => theme.del_strong, + FileStatus::Modified | FileStatus::Renamed | FileStatus::Copied | FileStatus::Unmerged => { + theme.foreground + } + } +} + /// Build one outline row's rendered [`Line`] — see [`render_outline`]'s doc comment for the -/// marker rules. -fn build_outline_line(item: &OutlineItem, theme: &Palette) -> Line<'static> { +/// marker rules. `icons` (CS5, `workon.review.outline.icons`) is [`OutlineIcons::None`] by +/// default, which reproduces the pre-CS5 row text exactly (no icon glyph, no extra space); only +/// [`OutlineIcons::Nerd`] inserts an icon before the name/path. +fn build_outline_line(item: &OutlineItem, theme: &Palette, icons: OutlineIcons) -> Line<'static> { match item { OutlineItem::Header { label, @@ -562,8 +583,12 @@ fn build_outline_line(item: &OutlineItem, theme: &Palette) -> Line<'static> { } Line::from(spans) } - OutlineItem::Dir { name, guides } => { - let text = format!("{}{name}/", tree_prefix(guides)); + OutlineItem::Dir { name, guides, .. } => { + let icon = match icons { + OutlineIcons::Nerd => format!("{} ", crate::icons::DIR_ICON), + OutlineIcons::None => String::new(), + }; + let text = format!("{}{icon}{name}/", tree_prefix(guides)); Line::from(TSpan::styled( text, Style::default() @@ -574,10 +599,12 @@ fn build_outline_line(item: &OutlineItem, theme: &Palette) -> Line<'static> { OutlineItem::File { path, status, + change, guides, .. } => { let glyph = status.glyph(); + let letter = change.letter(); // Empty `guides` (Flat/Stack modes) keeps the original two-space indent; a // non-empty `guides` (Tree/StackTree modes) draws tree connectors instead — see // `OutlineItem`'s doc comment for why emptiness is the mode signal. @@ -586,8 +613,24 @@ fn build_outline_line(item: &OutlineItem, theme: &Palette) -> Line<'static> { } else { tree_prefix(guides) }; - let text = format!("{prefix}{glyph} {path}"); - Line::from(TSpan::styled(text, Style::default().fg(theme.foreground))) + let icon = match icons { + OutlineIcons::Nerd => format!("{} ", crate::icons::icon_for_path(path)), + OutlineIcons::None => String::new(), + }; + Line::from(vec![ + TSpan::styled( + format!("{prefix}{glyph}"), + Style::default().fg(theme.foreground), + ), + TSpan::styled( + letter.to_string(), + Style::default().fg(change_letter_color(*change, theme)), + ), + TSpan::styled( + format!(" {icon}{path}"), + Style::default().fg(theme.foreground), + ), + ]) } } } @@ -710,9 +753,9 @@ fn render_footer(frame: &mut Frame, app: &App, area: Rect, keymap: &Keymap, them } } -/// Write a gap row's `··· N unchanged lines ···` marker across the FULL body width (both panes -/// and the divider column) — unlike a per-pane content row, a gap hides the same span on both -/// sides, so it isn't "about" one side or the other. +/// Write a gap row's `··· N unchanged lines (Enter to expand) ···` marker across the FULL body +/// width (both panes and the divider column) — unlike a per-pane content row, a gap hides the +/// same span on both sides, so it isn't "about" one side or the other. fn render_gap_row( buf: &mut Buffer, area: Rect, @@ -722,7 +765,7 @@ fn render_gap_row( is_selected: bool, theme: &Palette, ) { - let msg = format!("··· {skipped} unchanged lines ···"); + let msg = format!("··· {skipped} unchanged lines (Enter to expand) ···"); let line = Line::from(TSpan::styled(msg, Style::default().fg(theme.dim))); // Cursor wins over selection on the same row. let line = if is_cursor { @@ -774,7 +817,186 @@ fn render_loading_placeholder( ); } +/// Push a `"path +N -M"` file row's spans onto `lines`: the path in the theme foreground, the +/// add/del counts tinted with the theme's own diff-add/diff-del colors (the strong variants — the +/// same tint a hunk's `+`/`-` gutter itself uses, see [`Palette::add_strong`]/ +/// [`Palette::del_strong`]) so the panel's diffstat reads consistently with the diff body it's +/// standing in for. +fn push_summary_file_row(lines: &mut Vec>, row: &SummaryFileRow, theme: &Palette) { + lines.push(Line::from(vec![ + TSpan::styled(row.path.clone(), Style::default().fg(theme.foreground)), + TSpan::raw(" "), + TSpan::styled( + format!("+{}", row.adds), + Style::default().fg(theme.add_strong), + ), + TSpan::raw(" "), + TSpan::styled( + format!("-{}", row.dels), + Style::default().fg(theme.del_strong), + ), + ])); +} + +/// Append `rows`' file lines to `lines`, truncated to leave room for `budget` more rows within the +/// panel's height — the last line becomes `"… and N more"` (dim) when the list overflows instead +/// of silently clipping. +fn push_summary_file_rows( + lines: &mut Vec>, + rows: &[SummaryFileRow], + budget: usize, + theme: &Palette, +) { + if rows.len() <= budget { + for row in rows { + push_summary_file_row(lines, row, theme); + } + return; + } + // Reserve the last visible row for the "… and N more" marker. + let shown = budget.saturating_sub(1); + for row in &rows[..shown] { + push_summary_file_row(lines, row, theme); + } + let remaining = rows.len() - shown; + lines.push(Line::from(TSpan::styled( + format!("\u{2026} and {remaining} more"), + Style::default().fg(theme.dim), + ))); +} + +/// Append the shared summary body — spacer, height-budgeted per-file rows, and the +/// `"{N} files +A -D"` totals line — used verbatim by both [`changeset_summary_lines`] and +/// [`dir_summary_lines`], which differ only in their title line and early-return states. +fn push_summary_body( + lines: &mut Vec>, + files: &[SummaryFileRow], + total_adds: usize, + total_dels: usize, + height: usize, + theme: &Palette, +) { + lines.push(Line::from("")); + let footer_budget = 1; // the totals line always shows + let file_budget = height.saturating_sub(lines.len() + footer_budget); + push_summary_file_rows(lines, files, file_budget, theme); + lines.push(Line::from(vec![ + TSpan::styled( + format!("{} files", files.len()), + Style::default().fg(theme.foreground), + ), + TSpan::raw(" "), + TSpan::styled( + format!("+{total_adds}"), + Style::default().fg(theme.add_strong), + ), + TSpan::raw(" "), + TSpan::styled( + format!("-{total_dels}"), + Style::default().fg(theme.del_strong), + ), + ])); +} + +/// Build a [`ChangesetSummary`]'s lines: title line (carrying the same current/needs-restack/ +/// failed markers `build_outline_line`'s Header arm draws), a loading/failed line OR the per-file +/// list + totals line. +fn changeset_summary_lines( + summary: &ChangesetSummary, + height: usize, + theme: &Palette, +) -> Vec> { + let mut lines = Vec::new(); + + let mut title_spans = vec![TSpan::styled( + if summary.current { "\u{25CF} " } else { " " }, + Style::default().fg(FG_CURRENT), + )]; + title_spans.push(TSpan::styled( + summary.label.clone(), + Style::default() + .fg(theme.foreground) + .add_modifier(Modifier::BOLD), + )); + if summary.needs_restack { + title_spans.push(TSpan::styled(" \u{26A0}", Style::default().fg(FG_WARN))); + } + lines.push(Line::from(title_spans)); + + if summary.failed { + let msg = summary + .failure_message + .as_deref() + .unwrap_or("(no error message)"); + lines.push(Line::from(TSpan::styled( + format!("\u{2717} {msg}"), + Style::default().fg(FG_ERROR), + ))); + return lines; + } + if summary.loading { + lines.push(Line::from(TSpan::styled( + "Loading\u{2026}", + Style::default().fg(theme.dim), + ))); + return lines; + } + + push_summary_body( + &mut lines, + &summary.files, + summary.total_adds, + summary.total_dels, + height, + theme, + ); + lines +} + +/// Build a [`DirSummary`]'s lines: a bold path title, a blank line, the per-file list, and the +/// totals line — no current/restack/loading/failed markers (a directory carries none of those). +fn dir_summary_lines(summary: &DirSummary, height: usize, theme: &Palette) -> Vec> { + let mut lines = vec![Line::from(TSpan::styled( + format!("{}/", summary.path), + Style::default() + .fg(theme.foreground) + .add_modifier(Modifier::BOLD), + ))]; + push_summary_body( + &mut lines, + &summary.files, + summary.total_adds, + summary.total_dels, + height, + theme, + ); + lines +} + +/// CS4's summary panel: renders in place of the diff body while the outline is open and focused +/// with its cursor on a Header/Dir row (see [`App::summary_target`]) — a title line, a blank +/// line, per-file `"path +N -M"` rows (truncated to the pane height), and a totals line. A +/// loading/failed Header shows its own inline state instead of a file list (see +/// [`changeset_summary_lines`]). +fn render_summary(frame: &mut Frame, summary: &Summary, area: Rect, theme: &Palette) { + let height = area.height as usize; + let lines = match summary { + Summary::Changeset(cs) => changeset_summary_lines(cs, height, theme), + Summary::Dir(dir) => dir_summary_lines(dir, height, theme), + }; + frame.render_widget(Paragraph::new(lines), area); +} + fn render_body(frame: &mut Frame, app: &mut App, area: Rect, theme: &Palette) { + // CS4: the outline is open AND focused, and its cursor rests on a Header/Dir row — show that + // row's summary instead of a file's diff. Checked before every other body gate below (an + // unfocused open outline, or the cursor on a File row, falls straight through to the usual + // diff-body rendering; `summary_target` returns `None` in both cases). + if let Some(target) = app.summary_target() { + let summary = app.summary_for(target); + render_summary(frame, &summary, area, theme); + return; + } // ADR-037: the active changeset's diff hasn't been acquired (or failed to acquire) yet — // both cases have an empty `files()` list, so they must be checked BEFORE the "(no changes)" // fallback below, which would otherwise misreport a Pending/Failed changeset as an @@ -1031,7 +1253,7 @@ fn render_pane_sbs( let is_cursor = cursor == Some(row_idx); let is_selected = selection.is_some_and(|(lo, hi)| row_idx >= lo && row_idx <= hi); match &view.display[row_idx] { - DisplayRow::Gap { skipped } => { + DisplayRow::Gap { skipped, .. } => { render_gap_row( frame.buffer_mut(), area, @@ -1234,7 +1456,7 @@ fn render_pane_inline( let is_cursor = cursor == Some(row_idx); let is_selected = selection.is_some_and(|(lo, hi)| row_idx >= lo && row_idx <= hi); match &view.inline[row_idx] { - InlineRow::Gap { skipped } => { + InlineRow::Gap { skipped, .. } => { render_gap_row( frame.buffer_mut(), area, @@ -1292,6 +1514,7 @@ mod tests { use crate::app::test_support::app_from_fixture; use crate::app::App; use crate::keymap::Keymap; + use crate::outline::OutlineItem; use crate::theme::Palette; /// Render one frame against the default (unrebound) keymap and the dark theme — the vast @@ -2558,26 +2781,269 @@ mod tests { let buf = render_once(&mut app, OUTLINE_TEST_WIDTH, 20); let content: Vec = (0..buf.area.height).map(|y| outline_row(&buf, y)).collect(); - // Row order per the dirs-after-files/alpha-within-group rule, one outline row per - // buffer row starting at y=1 (y=0 is the winbar): `top.txt` (file, root, NOT the root's - // last child — `src/` follows), `src/` (dir, root, IS the root's last child), then - // `a.txt` nested one level under `src/` (the only — hence last — child of `src/`). + // Row order per the CS3 dirs-before-files/alpha-within-group rule, one outline row per + // buffer row starting at y=1 (y=0 is the winbar): `src/` (dir, root, NOT the root's last + // child — `top.txt` follows), `a.txt` nested one level under `src/` (the only — hence + // last — child of `src/`), then `top.txt` (file, root, IS the root's last child). assert!( - content[1].contains('\u{251C}') && content[1].contains("top.txt"), - "expected row 1 to be top.txt with a non-last '├─' guide, got:\n{}", + content[1].contains('\u{251C}') && content[1].contains("src/"), + "expected row 1 to be the src/ directory row with a non-last '├─' guide, got:\n{}", content.join("\n") ); assert!( - content[2].contains('\u{2514}') && content[2].contains("src/"), - "expected row 2 to be the src/ directory row with a last-child '└─' guide, got:\n{}", + content[2].contains('\u{2514}') && content[2].contains("a.txt"), + "expected row 2 to be src/a.txt, indented under src/ with its own last-child '└─' \ + guide, got:\n{}", content.join("\n") ); assert!( - content[3].contains('\u{2514}') && content[3].contains("a.txt"), - "expected row 3 to be src/a.txt, indented under src/ with its own last-child '└─' \ - guide, got:\n{}", + content[3].contains('\u{2514}') && content[3].contains("top.txt"), + "expected row 3 to be top.txt with a last-child '└─' guide, got:\n{}", + content.join("\n") + ); + } + + // ── CS5: file status letter + opt-in nerd-font icons ─────────────────────────── + + #[test] + fn outline_file_row_shows_the_modified_change_letter_in_its_own_color() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.rs", "one\n", "one\nCHANGED\n") + .build() + .unwrap(); + let mut app = app_from_fixture(&fixture); + // A lone changeset defaults the outline closed — force it open so this render test can + // inspect its rows (same pattern as `outline_tree_mode_renders_directory_rows_with_tree_guides`). + if !app.outline_open() { + app.toggle_outline(); + } + + let buf = render_once(&mut app, OUTLINE_TEST_WIDTH, 20); + let content: Vec = (0..buf.area.height).map(|y| outline_row(&buf, y)).collect(); + // Skip y=0: the full-width winbar also names the file ("[1/1] a.rs"), so an unskipped + // search would match it instead of the outline's own row below it. + let row = content + .iter() + .enumerate() + .skip(1) + .find(|(_, r)| r.contains("a.rs")) + .map(|(i, _)| i) + .expect("a.rs's file row present"); + assert!( + content[row].contains('M'), + "expected the Modified change letter 'M' in a.rs's row, got: {:?}", + content[row] + ); + + let letter_x = content[row].find('M').unwrap() as u16; + assert_eq!( + buf.cell((letter_x, row as u16)).unwrap().style().fg, + Some(Palette::dark().foreground), + "Modified is a change-to-existing-content status, so its letter must carry the \ + theme's neutral foreground, not an add/del tint" + ); + } + + #[test] + fn outline_icons_nerd_renders_the_rust_file_icon_and_the_dir_icon() { + use git2::Repository; + use workon::{Changeset, ChangesetSpan}; + + use crate::app::ChangesetView; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let root = fixture + .commit("main") + .file("root.txt", "r\n") + .create("root") + .unwrap(); + let head = fixture + .commit("main") + .file("src/main.rs", "fn main() {}\n") + .create("head") + .unwrap(); + let repo = fixture.repo().unwrap(); + let cs = Changeset { + name: "cs".to_string(), + span: ChangesetSpan::Committed { base: root, head }, + title: None, + current: true, + needs_restack: false, + }; + let view = ChangesetView::from_changeset_diff( + cs.clone(), + crate::acquire::diff_changeset(repo, &cs).unwrap(), + ); + let owned = Repository::open(repo.workdir().unwrap()).unwrap(); + let mut app = App::from_changesets(owned, vec![view]); + app.open_current(); + if !app.outline_open() { + app.toggle_outline(); + } + app.outline_cycle_mode(); // Stack -> Tree, so `src/` renders as its own Dir row + assert_eq!(app.outline_mode(), crate::outline::OutlineMode::Tree); + app.set_outline_icons(crate::icons::OutlineIcons::Nerd); + + let buf = render_once(&mut app, OUTLINE_TEST_WIDTH, 20); + let content: Vec = (0..buf.area.height).map(|y| outline_row(&buf, y)).collect(); + + // Skip y=0 in both searches: the full-width winbar names the path ("[1/1] src/main.rs"), + // so an unskipped search would match it instead of the outline's own rows below it. + let dir_row = content + .iter() + .skip(1) + .find(|r| r.contains("src/")) + .expect("src/ dir row present"); + assert!( + dir_row.contains(crate::icons::DIR_ICON), + "expected the dir icon before src/, got: {dir_row:?}" + ); + let file_row = content + .iter() + .skip(1) + .find(|r| r.contains("main.rs")) + .expect("main.rs file row present"); + assert!( + file_row.contains(crate::icons::icon_for_path("main.rs")), + "expected the rust file icon before main.rs, got: {file_row:?}" + ); + } + + #[test] + fn outline_icons_none_renders_neither_icon() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = changeset_with_nested_paths(&fixture); + if !app.outline_open() { + app.toggle_outline(); + } + app.outline_cycle_mode(); // Stack -> Tree + assert_eq!(app.outline_mode(), crate::outline::OutlineMode::Tree); + assert_eq!( + app.outline_icons(), + crate::icons::OutlineIcons::None, + "sanity: icons default to None" + ); + + let buf = render_once(&mut app, OUTLINE_TEST_WIDTH, 20); + let content: Vec = (0..buf.area.height).map(|y| outline_row(&buf, y)).collect(); + + assert!( + !content.iter().any(|r| r.contains(crate::icons::DIR_ICON)), + "icons=none must never render the dir icon, got:\n{}", content.join("\n") ); + assert!( + !content + .iter() + .any(|r| r.contains(crate::icons::DEFAULT_ICON)), + "icons=none must never render the default file icon, got:\n{}", + content.join("\n") + ); + } + + // ── CS4: summary panel ─────────────────────────────────────────────────────── + + /// The body area's columns, for a render at [`OUTLINE_TEST_WIDTH`] (outline `0..35`, divider + /// `35`, body `36..`) — mirrors [`outline_row`]'s slice but for the OTHER side of the pane. + fn body_text(buf: &Buffer) -> String { + (0..buf.area.height) + .map(|y| { + (36..buf.area.width) + .map(|x| cell_text(buf, x, y)) + .collect::() + }) + .collect::>() + .join("\n") + } + + #[test] + fn focused_header_selection_renders_the_summary_panel_instead_of_the_diff() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + assert!(app.outline_open(), "a two-changeset stack default-opens"); + // Default is open+unfocused; two toggles (close, reopen) focuses it — same idiom + // `outline_cursor_row_carries_cursor_background_when_focused` uses. Construction's + // `sync_outline_to_current` already parked the cursor on cs-b's (the `current` + // changeset's) own File row, not a Header — move it onto cs-b's Header explicitly. + app.toggle_outline(); + app.toggle_outline(); + assert!(app.outline_open() && app.outline_focused()); + let header_idx = app + .outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::Header { cs_idx: 1, .. })) + .expect("cs-b's header row present in Stack mode") as i64; + // A Header row never jumps the diff on `outline_move_by` (only a File row does — see its + // doc comment), so a single relative move onto it is side-effect-free. + let delta = header_idx - app.outline_cursor() as i64; + app.outline_move_by(delta); + assert!(matches!( + app.outline_items()[app.outline_cursor()], + OutlineItem::Header { cs_idx: 1, .. } + )); + app.focus_outline(); // outline_move_by doesn't touch focus; ensure it's still focused + + let buf = render_once(&mut app, OUTLINE_TEST_WIDTH, 20); + let body = body_text(&buf); + assert!( + body.contains("cs-b"), + "expected the summary panel's title (cs-b's label — it has no title, so falls back \ + to its name), got:\n{body}" + ); + assert!( + body.contains("+1") && body.contains("-0"), + "expected a '+N -M' diffstat fragment for cs-b's single added file, got:\n{body}" + ); + assert!( + body.contains("1 files"), + "expected the summary panel's totals line, got:\n{body}" + ); + } + + #[test] + fn unfocused_open_outline_on_a_header_row_still_renders_the_normal_diff_body() { + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + // Default state: open, UNFOCUSED — must NOT show the summary panel (locked design: only + // a FOCUSED outline overrides the diff area) even with the cursor moved onto a Header + // row (construction's `sync_outline_to_current` parks it on cs-b's File row by default). + assert!(app.outline_open() && !app.outline_focused()); + let header_idx = app + .outline_items() + .iter() + .position(|it| matches!(it, OutlineItem::Header { cs_idx: 1, .. })) + .expect("cs-b's header row present in Stack mode") as i64; + let delta = header_idx - app.outline_cursor() as i64; + app.outline_move_by(delta); + assert!(matches!( + app.outline_items()[app.outline_cursor()], + OutlineItem::Header { cs_idx: 1, .. } + )); + assert!( + !app.outline_focused(), + "outline_move_by must not itself grant focus" + ); + + let buf = render_once(&mut app, OUTLINE_TEST_WIDTH, 20); + let body = body_text(&buf); + assert!( + !body.contains("1 files"), + "an unfocused open outline must never override the diff body with the summary \ + panel's totals line, got:\n{body}" + ); } // ── theming fix: canvas paint ──────────────────────────────────────────────── diff --git a/git-workon-review/src/scope.rs b/git-workon-review/src/scope.rs new file mode 100644 index 00000000..6e01076f --- /dev/null +++ b/git-workon-review/src/scope.rs @@ -0,0 +1,181 @@ +//! Enclosing tree-sitter "scope" lookup for CS9's reveal-to-scope gap expansion. +//! +//! Pure module: given a language key (the same key +//! [`crate::highlight::lang_key_for_ext`] resolves a file extension to) and a file's full text, +//! [`enclosing_scope_lines`] finds the smallest allowlisted structural node (function/impl/ +//! class/...) containing a given line, so [`crate::app::App::expand_gap_at_cursor`] can reveal +//! exactly that much of a collapsed gap instead of a flat +10 rows. +//! +//! ## Allowlist philosophy +//! +//! Only "scope" node KINDS are allowlisted per language — deliberately narrow (a function body, +//! an impl/class block, ...) so a press reads as "show me the surrounding definition," not "show +//! me every nested block/expression the cursor happens to sit inside." Languages without a +//! reasonable definition of "scope" for this purpose (json/toml — data, not code with nested +//! definitions) get an empty allowlist, which makes [`enclosing_scope_lines`] always return +//! `None`: the caller falls back to the flat reveal uniformly, no special-casing needed at the +//! call site. +//! +//! ## Line/coordinate conventions +//! +//! The public API is 1-based, matching [`crate::align::Row::Line`] and the rest of the +//! diff-alignment code. tree-sitter's own [`Point::row`] is 0-based; this module converts at its +//! boundary (in, and back out) and nowhere else. The returned range is inclusive on both ends. +//! +//! ## No caching +//! +//! Parsing happens on demand, once per `Enter` press on a gap — bounded by [`MAX_SCOPE_LINES`] +//! (mirrors [`crate::highlight::MAX_HIGHLIGHT_LINES`]'s cap philosophy). That's cheap enough not +//! to be worth a parse-tree cache keyed on file identity + edit generation. + +use tree_sitter::{Parser, Point}; + +use crate::highlight::language_for_key; + +/// Files with more lines than this skip scope lookup entirely (the caller falls back to the flat +/// +N reveal) — same cap philosophy as [`crate::highlight::MAX_HIGHLIGHT_LINES`]. +pub const MAX_SCOPE_LINES: usize = 20_000; + +/// Per-language allowlist of "scope" node kinds, matched by exact string against +/// [`tree_sitter::Node::kind`]. Node kind names are grammar-specific facts verified against the +/// bundled grammars by this module's tests — do not extend without a test parsing a real snippet. +fn scope_kinds(lang_key: &str) -> &'static [&'static str] { + match lang_key { + "rust" => &[ + "function_item", + "impl_item", + "trait_item", + "mod_item", + "struct_item", + "enum_item", + ], + "javascript" => &[ + "function_declaration", + "function_expression", + "method_definition", + "class_declaration", + "arrow_function", + ], + "typescript" | "tsx" => &[ + "function_declaration", + "function_expression", + "method_definition", + "class_declaration", + "arrow_function", + "interface_declaration", + "enum_declaration", + "module_declaration", + ], + "lua" => &["function_declaration", "function_definition"], + // json/toml/markdown: no structural "scope" concept worth revealing to — always fall + // back to the flat reveal. + _ => &[], + } +} + +/// The smallest allowlisted ancestor node (see [`scope_kinds`]) enclosing 1-based `line`, as an +/// inclusive 1-based `(start_line, end_line)` range — or `None` when: `lang_key` has no (or an +/// empty) allowlist, `text` exceeds [`MAX_SCOPE_LINES`], the grammar fails to build, or no +/// allowlisted ancestor contains `line` (e.g. a top-level `use` statement outside any item). +pub fn enclosing_scope_lines(lang_key: &str, text: &str, line: usize) -> Option<(usize, usize)> { + let kinds = scope_kinds(lang_key); + if kinds.is_empty() { + return None; + } + if text.lines().count() > MAX_SCOPE_LINES { + return None; + } + + let language = language_for_key(lang_key)?; + let mut parser = Parser::new(); + parser.set_language(&language).ok()?; + let tree = parser.parse(text, None)?; + + let point = Point { + row: line.saturating_sub(1), + column: 0, + }; + let mut node = tree + .root_node() + .named_descendant_for_point_range(point, point)?; + loop { + if kinds.contains(&node.kind()) { + return Some((node.start_position().row + 1, node.end_position().row + 1)); + } + node = node.parent()?; + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn rust_line_inside_a_function_body_returns_the_function_range() { + let src = "fn outer() {\n let x = 1;\n let y = 2;\n}\n"; + // Line 2 (`let x = 1;`) is inside `fn outer`, which spans lines 1-4. + let range = enclosing_scope_lines("rust", src, 2); + assert_eq!(range, Some((1, 4))); + } + + #[test] + fn rust_nested_function_returns_the_inner_function_not_the_outer() { + let src = "fn outer() {\n fn inner() {\n let z = 1;\n }\n}\n"; + // Line 3 is inside `inner`, which spans lines 2-4 — the smallest (deepest) allowlisted + // ancestor, not `outer` (lines 1-5). + let range = enclosing_scope_lines("rust", src, 3); + assert_eq!(range, Some((2, 4))); + } + + #[test] + fn rust_top_level_use_line_returns_none() { + let src = "use std::fmt;\n\nfn main() {}\n"; + // Line 1 is a top-level `use` — no allowlisted ancestor contains it. + assert_eq!(enclosing_scope_lines("rust", src, 1), None); + } + + #[test] + fn rust_impl_block_with_two_functions_asking_between_them_returns_the_impl() { + let src = "struct S;\n\nimpl S {\n fn a(&self) {\n let _ = 1;\n }\n\n fn b(&self) {\n let _ = 2;\n }\n}\n"; + // Line 5 is inside `fn a`'s body — smallest allowlisted ancestor is the fn. + assert_eq!(enclosing_scope_lines("rust", src, 5), Some((4, 6))); + // Line 7 is the blank line between the two fns, still inside the impl block but outside + // both fn bodies — smallest allowlisted ancestor is the impl. + assert_eq!(enclosing_scope_lines("rust", src, 7), Some((3, 11))); + } + + #[test] + fn typescript_line_in_a_method_returns_the_method_range() { + let src = "class C {\n method() {\n const x = 1;\n }\n}\n"; + let range = enclosing_scope_lines("typescript", src, 3); + assert_eq!(range, Some((2, 4))); + } + + #[test] + fn typescript_line_in_an_interface_returns_the_interface_range() { + let src = "interface Foo {\n bar: string;\n baz: number;\n}\n"; + let range = enclosing_scope_lines("typescript", src, 2); + assert_eq!(range, Some((1, 4))); + } + + #[test] + fn lua_line_in_a_function_returns_its_range() { + let src = "function greet()\n local msg = \"hi\"\n print(msg)\nend\n"; + let range = enclosing_scope_lines("lua", src, 2); + assert_eq!(range, Some((1, 4))); + } + + #[test] + fn json_always_returns_none() { + let src = "{\n \"a\": 1,\n \"b\": {\n \"c\": 2\n }\n}\n"; + assert_eq!(enclosing_scope_lines("json", src, 4), None); + } + + #[test] + fn oversized_text_returns_none() { + // Synthesize a cheap file with more lines than MAX_SCOPE_LINES; content doesn't matter, + // only line count. + let src = "fn f() {}\n".repeat(MAX_SCOPE_LINES + 1); + assert_eq!(enclosing_scope_lines("rust", &src, 1), None); + } +} diff --git a/git-workon-review/src/summary.rs b/git-workon-review/src/summary.rs new file mode 100644 index 00000000..b6eab27d --- /dev/null +++ b/git-workon-review/src/summary.rs @@ -0,0 +1,297 @@ +//! CS4's summary panel: pure builders for the renderable data `render.rs`'s `render_summary` +//! paints when the outline is OPEN AND FOCUSED and its cursor rests on a +//! [`crate::outline::OutlineItem::Header`]/[`crate::outline::OutlineItem::Dir`] row instead of a +//! file — mirrors [`crate::outline`]'s pure-module posture (no [`crate::app::App`]/git2 +//! dependency): everything here is built from `&[FileChange]`-shaped inputs plus a handful of +//! primitives `App::summary_for` supplies. +//! +//! ## What a changeset summary can show +//! +//! `workon::Changeset` (see `git-workon-lib/src/changeset.rs`) exposes only `name`/`title` for a +//! changeset today — no commit body/message. [`changeset_summary`] therefore renders the +//! label (title, falling back to name — the same rule the winbar/outline header already use) +//! plus the diffstat; there is no commit-message row. Surfacing the commit body would need a +//! `repo.find_commit` lookup keyed off the changeset's head OID — left as a follow-up, not part +//! of this changeset's scope. + +use crate::model::{FileChange, LineKind}; + +/// Count added/deleted LINES across `change`'s hunks — `(adds, dels)`. A binary file (no hunks) +/// counts as `(0, 0)`; `LineKind::Context` lines never count toward either total. +pub fn file_diffstat(change: &FileChange) -> (usize, usize) { + let mut adds = 0usize; + let mut dels = 0usize; + for hunk in &change.hunks { + for line in &hunk.lines { + match line.kind { + LineKind::Addition => adds += 1, + LineKind::Deletion => dels += 1, + LineKind::Context => {} + } + } + } + (adds, dels) +} + +/// One file's row in a summary panel's per-file list — just enough to render `"path +N -M"`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SummaryFileRow { + pub path: String, + pub adds: usize, + pub dels: usize, +} + +/// Build the per-file row list plus its `(total_adds, total_dels)` from `files` — shared by +/// [`changeset_summary`] and [`dir_summary`], the only difference between the two being which +/// files the caller has already filtered down to. Borrows only — a summary reads `path` and the +/// hunk line kinds, so no caller should ever need to clone a `FileChange` (with its full hunk +/// content bytes) just to build one. +fn file_rows<'a>( + files: impl IntoIterator, +) -> (Vec, usize, usize) { + let rows: Vec = files + .into_iter() + .map(|f| { + let (adds, dels) = file_diffstat(f); + SummaryFileRow { + path: f.path.clone(), + adds, + dels, + } + }) + .collect(); + let total_adds = rows.iter().map(|r| r.adds).sum(); + let total_dels = rows.iter().map(|r| r.dels).sum(); + (rows, total_adds, total_dels) +} + +/// Renderable summary for a Header-row outline selection: the changeset's own flags/label (the +/// same fields [`crate::outline::OutlineChangeset`] carries) plus a per-file diffstat breakdown. +/// `loading`/`failed` mirror ADR-037's slot state — when either is set, `files` is always empty +/// (a `Pending`/`Failed` [`crate::app::ChangesetView`] never has a real file list), so +/// `render_summary` shows the loading/failure line in place of the file rows rather than an +/// empty list. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ChangesetSummary { + pub label: String, + pub current: bool, + pub needs_restack: bool, + pub loading: bool, + pub failed: bool, + /// The acquisition failure message (ADR-037), `Some` only when `failed`. + pub failure_message: Option, + pub files: Vec, + pub total_adds: usize, + pub total_dels: usize, +} + +/// Build a [`ChangesetSummary`] from a changeset's outline-relevant fields plus its file list. +#[allow(clippy::too_many_arguments)] +pub fn changeset_summary( + label: String, + current: bool, + needs_restack: bool, + loading: bool, + failed: bool, + failure_message: Option, + files: &[FileChange], +) -> ChangesetSummary { + let (files, total_adds, total_dels) = file_rows(files); + ChangesetSummary { + label, + current, + needs_restack, + loading, + failed, + failure_message, + files, + total_adds, + total_dels, + } +} + +/// Renderable summary for a Dir-row outline selection: the aggregate diffstat for every file +/// under `path`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DirSummary { + pub path: String, + pub files: Vec, + pub total_adds: usize, + pub total_dels: usize, +} + +/// Segment-boundary match: `file_path` is "under" `dir_path` only when `dir_path` is a full path +/// SEGMENT prefix of `file_path` — `"src"` matches `"src/a.rs"` but must NOT match `"src2/b.rs"` +/// (a raw [`str::starts_with`] would wrongly match the latter). +/// +/// `pub(crate)`: CS7's `App::outline_row_targets` reuses this to resolve a Dir row's files +/// (`s`/`d` in the outline), the same segment-boundary rule [`dir_summary`] already relies on — +/// rather than re-deriving it in `app.rs`. +pub(crate) fn path_is_under(file_path: &str, dir_path: &str) -> bool { + file_path + .strip_prefix(dir_path) + .and_then(|rest| rest.strip_prefix('/')) + .is_some() +} + +/// Build a [`DirSummary`] for `path`, filtering `files` (already scoped by the caller to +/// whichever changeset(s) the selected [`crate::outline::OutlineItem::Dir`] row's `cs_idx` +/// covers — see `App::summary_for`) down to the ones under `path`. Takes refs — see +/// [`file_rows`]'s doc for why no `FileChange` is ever cloned here. +pub fn dir_summary(path: String, files: &[&FileChange]) -> DirSummary { + let scoped = files + .iter() + .copied() + .filter(|f| path_is_under(&f.path, &path)); + let (files, total_adds, total_dels) = file_rows(scoped); + DirSummary { + path, + files, + total_adds, + total_dels, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::model::{FileStatus, Hunk, HunkLine}; + + fn hunk_line(kind: LineKind) -> HunkLine { + HunkLine { + kind, + content: b"x\n".to_vec(), + old_lnum: None, + new_lnum: None, + missing_newline: false, + } + } + + fn file(path: &str, adds: usize, dels: usize, contexts: usize) -> FileChange { + let mut lines = Vec::new(); + for _ in 0..adds { + lines.push(hunk_line(LineKind::Addition)); + } + for _ in 0..dels { + lines.push(hunk_line(LineKind::Deletion)); + } + for _ in 0..contexts { + lines.push(hunk_line(LineKind::Context)); + } + FileChange { + path: path.to_string(), + old_path: None, + status: FileStatus::Modified, + is_binary: false, + old_mode: 0o100644, + new_mode: 0o100644, + hunks: vec![Hunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 1, + header: Vec::new(), + lines, + }], + } + } + + #[test] + fn file_diffstat_counts_adds_and_dels_but_not_context() { + let f = file("a.rs", 3, 2, 5); + assert_eq!(file_diffstat(&f), (3, 2)); + } + + #[test] + fn file_diffstat_is_zero_for_a_binary_file_with_no_hunks() { + let f = FileChange { + path: "bin.png".to_string(), + old_path: None, + status: FileStatus::Modified, + is_binary: true, + old_mode: 0o100644, + new_mode: 0o100644, + hunks: Vec::new(), + }; + assert_eq!(file_diffstat(&f), (0, 0)); + } + + #[test] + fn changeset_summary_totals_every_files_diffstat() { + let files = vec![file("a.rs", 2, 1, 0), file("b.rs", 0, 3, 0)]; + let summary = changeset_summary( + "My Title".to_string(), + true, + false, + false, + false, + None, + &files, + ); + assert_eq!(summary.label, "My Title"); + assert!(summary.current); + assert!(!summary.needs_restack); + assert_eq!(summary.files.len(), 2); + assert_eq!(summary.total_adds, 2); + assert_eq!(summary.total_dels, 4); + } + + #[test] + fn changeset_summary_loading_carries_no_files() { + let summary = changeset_summary( + "Pending CS".to_string(), + false, + false, + true, + false, + None, + &[], + ); + assert!(summary.loading); + assert!(summary.files.is_empty()); + assert_eq!(summary.total_adds, 0); + } + + #[test] + fn changeset_summary_failed_carries_the_message() { + let summary = changeset_summary( + "Failed CS".to_string(), + false, + false, + false, + true, + Some("boom".to_string()), + &[], + ); + assert!(summary.failed); + assert_eq!(summary.failure_message.as_deref(), Some("boom")); + } + + #[test] + fn dir_summary_filters_by_segment_boundary_not_raw_prefix() { + let files = [ + file("src/a.rs", 1, 0, 0), + file("src/b.rs", 0, 1, 0), + file("src2/b.rs", 5, 5, 0), + file("top.rs", 1, 1, 0), + ]; + let summary = dir_summary("src".to_string(), &files.iter().collect::>()); + let paths: Vec<&str> = summary.files.iter().map(|r| r.path.as_str()).collect(); + assert_eq!( + paths, + vec!["src/a.rs", "src/b.rs"], + "must match src/* but NOT src2/* (segment-boundary, not raw string prefix)" + ); + assert_eq!(summary.total_adds, 1); + assert_eq!(summary.total_dels, 1); + } + + #[test] + fn dir_summary_matches_nested_paths_under_the_dir() { + let files = [file("src/a/b.rs", 2, 0, 0), file("src/c.rs", 0, 2, 0)]; + let summary = dir_summary("src".to_string(), &files.iter().collect::>()); + assert_eq!(summary.files.len(), 2); + assert_eq!(summary.total_adds, 2); + assert_eq!(summary.total_dels, 2); + } +} diff --git a/git-workon-review/src/tui.rs b/git-workon-review/src/tui.rs index c893b286..5d194a2c 100644 --- a/git-workon-review/src/tui.rs +++ b/git-workon-review/src/tui.rs @@ -413,11 +413,18 @@ enum Action { DiscardHunk, DiscardFile, StartSelection, + ExpandGap, + ExpandGapAll, ToggleOutline, OutlineMoveBy(i64), OutlineConfirm, OutlineCycleMode, - OutlineUnfocus, + FocusOutline, + FocusDiff, + OutlineTop, + OutlineBottom, + OutlineStage, + OutlineDiscard, None, } @@ -446,6 +453,8 @@ fn command_to_action(command: Command, pane_height: usize) -> Action { Command::DiscardHunk => Action::DiscardHunk, Command::DiscardFile => Action::DiscardFile, Command::StartSelection => Action::StartSelection, + Command::ExpandGap => Action::ExpandGap, + Command::ExpandGapAll => Action::ExpandGapAll, Command::NextFile => Action::NextFile, Command::PrevFile => Action::PrevFile, Command::NextHunk => Action::NextHunk, @@ -456,13 +465,19 @@ fn command_to_action(command: Command, pane_height: usize) -> Action { Command::OutlineUp => Action::OutlineMoveBy(-1), Command::OutlineConfirm => Action::OutlineConfirm, Command::OutlineCycleMode => Action::OutlineCycleMode, + Command::FocusOutline => Action::FocusOutline, + Command::FocusDiff => Action::FocusDiff, + Command::OutlineTop => Action::OutlineTop, + Command::OutlineBottom => Action::OutlineBottom, + Command::OutlineStage => Action::OutlineStage, + Command::OutlineDiscard => Action::OutlineDiscard, } } /// Map one key press to an [`Action`] through the resolved [`Keymap`], given `pending` (the /// in-flight multi-key sequence buffer — generalized from the old `]`/`[` bracket chord to ANY /// bound sequence), the current pane height (for the half-page deltas), and whether the outline -/// pane currently has focus. +/// pane currently has focus/is open. /// /// Dispatch order: /// 1. The keymap ([`Keymap::advance`]) consumes the key. A bound sequence fires its command; a @@ -470,8 +485,9 @@ fn command_to_action(command: Command, pane_height: usize) -> Action { /// unrecognized suffix mid-sequence drops the buffer without re-processing (the old /// bracket-drop behavior, now general). /// 2. `Esc` stays HARDCODED (ADR-034: the whole `Esc`-precedence cascade is never routed through -/// the registry). Reached only as a fresh, otherwise-unbound key: it unfocuses the outline when -/// the outline has focus, else quits — the terminal leaf of the cascade `update` enforces. +/// the registry). Reached only as a fresh, otherwise-unbound key, it walks outward: the outline +/// having focus quits (same terminal leaf as `q`); otherwise, with the outline open, it focuses +/// the outline (home-base model: `h`/`FocusOutline`'s effect); otherwise it quits. /// /// `outline_focused` selects the keymap's outline vs diff context; the global bindings (`q`/`o`) /// are active in both, so `o` toggles and `q` quits from either pane. @@ -481,6 +497,7 @@ fn map_key( key: KeyEvent, pane_height: usize, outline_focused: bool, + outline_open: bool, ) -> Action { match keymap.advance(outline_focused, pending, key) { Dispatch::Command(command) => command_to_action(command, pane_height), @@ -488,7 +505,9 @@ fn map_key( Dispatch::Unmatched { mid_sequence } => { if !mid_sequence && key.code == KeyCode::Esc { if outline_focused { - Action::OutlineUnfocus + Action::Quit + } else if outline_open { + Action::FocusOutline } else { Action::Quit } @@ -524,6 +543,8 @@ fn action_needs_loaded_view(action: Action) -> bool { | Action::DiscardFile | Action::StartSelection | Action::ToggleSplitFocus + | Action::ExpandGap + | Action::ExpandGapAll ) } @@ -559,11 +580,18 @@ fn apply_action(app: &mut App, action: Action) -> bool { Action::DiscardHunk => app.discard_hunk(), Action::DiscardFile => app.discard_file(), Action::StartSelection => app.start_selection(), + Action::ExpandGap => app.expand_gap_at_cursor(false), + Action::ExpandGapAll => app.expand_gap_at_cursor(true), Action::ToggleOutline => app.toggle_outline(), Action::OutlineMoveBy(delta) => app.outline_move_by(delta), Action::OutlineConfirm => app.outline_confirm(), Action::OutlineCycleMode => app.outline_cycle_mode(), - Action::OutlineUnfocus => app.outline_unfocus(), + Action::FocusOutline => app.focus_outline(), + Action::FocusDiff => app.focus_diff(), + Action::OutlineTop => app.outline_top(), + Action::OutlineBottom => app.outline_bottom(), + Action::OutlineStage => app.outline_stage(), + Action::OutlineDiscard => app.outline_discard(), Action::None => {} } false @@ -579,11 +607,11 @@ enum KeyOutcome { } /// Resolve one `Key` event to a [`KeyOutcome`], given the caller has already ruled out the two -/// modal cases (a pending discard confirm, the help overlay) — this is cases 3-5 of `update`'s +/// modal cases (a pending discard confirm, the help overlay) — this is cases 3-6 of `update`'s /// documented Esc-precedence cascade, extracted so [`update`] and [`update_batch`] share the exact /// same resolution instead of duplicating it. /// -/// Clears any showing footer notice as a side effect, exactly like `update`'s cases 3-5 do (the +/// Clears any showing footer notice as a side effect, exactly like `update`'s cases 3-6 do (the /// confirm/help modals deliberately do not — that stays in their own arms, not here). fn resolve_key( app: &mut App, @@ -603,6 +631,7 @@ fn resolve_key( key, app.pane_height, app.outline_focused(), + app.outline_open(), )) } @@ -615,9 +644,10 @@ fn resolve_key( /// message and performs its normal action. `Resize`/`Tick` do NOT clear it: a redraw or timer /// tick isn't the user acting on the message. /// -/// Esc precedence (highest first): a pending discard confirm > the help overlay being open > the -/// outline having focus > an active line selection > the normal key map (where Esc quits). -/// Concretely: +/// Esc precedence (highest first): a pending discard confirm > the help overlay being open > an +/// active line selection (diff-focused) > the outline having focus > the diff having focus with +/// the outline open > the normal key map (where Esc quits). Concretely — the home-base model: +/// the outline is where Esc always eventually lands you before it quits. /// /// 1. A pending discard confirm captures the keyboard FIRST (before the notice clear and the /// normal key map): `y` accepts, `n`/`Esc` cancels, and every other key is swallowed — a modal @@ -627,17 +657,20 @@ fn resolve_key( /// reacts). Ranked just below the confirm modal — in practice the two are never up /// together, since opening help doesn't run through a confirm, but the confirm winning keeps /// a destructive prompt from ever being silently dismissed by a stray overlay key. -/// 3. Otherwise, while the outline pane has focus, Esc returns focus to the diff (via the normal -/// map's `outline_focused` branch — see [`map_key`]) rather than quitting or falling into the -/// selection-cancel case below (locked design: "Esc must still not quit when the outline has -/// focus"). The selection-Esc arm below is guarded to defer to this case. -/// 4. Otherwise, with an active line selection, Esc CANCELS the selection instead of quitting (`q` -/// still quits). Other keys fall through to the normal map — `j`/`k` extend the selection, -/// `s`/`d` act on it. -/// 5. Otherwise the normal map applies, where Esc (like `q`) quits. +/// 3. Otherwise, with an active line selection AND the diff focused, Esc CANCELS the selection +/// instead of moving focus or quitting (`q` still quits). This arm is guarded to defer to case +/// 4 when the outline has focus (a selection can only be active while looking at the diff, but +/// the guard keeps the precedence explicit). Other keys fall through to the normal map — +/// `j`/`k` extend the selection, `s`/`d` act on it. +/// 4. Otherwise, while the outline pane has focus, Esc QUITS — same terminal leaf as `q`. The +/// outline is home base; there's nowhere further out to walk to. +/// 5. Otherwise, with the diff focused and the outline OPEN, Esc walks outward one step: it +/// focuses the outline (same effect as `h`/[`App::focus_outline`]) rather than quitting. +/// 6. Otherwise (diff focused, outline closed) the normal map applies, where Esc (like `q`) quits +/// — there's no outline to walk out to. /// -/// A `Key` event clears any showing footer notice before applying its own action (cases 3-5); the -/// confirm and help modals (cases 1-2) deliberately do not. Cases 3-5 are delegated to +/// A `Key` event clears any showing footer notice before applying its own action (cases 3-6); the +/// confirm and help modals (cases 1-2) deliberately do not. Cases 3-6 are delegated to /// [`resolve_key`], shared with [`update_batch`]. fn update(app: &mut App, keymap: &Keymap, pending: &mut Vec, event: AppEvent) -> bool { match event { @@ -1224,11 +1257,11 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('q')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('q')), 20, false, false), Action::Quit ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Esc), 20, false), + map_key(&km, &mut pending, key(KeyCode::Esc), 20, false, false), Action::Quit ); } @@ -1238,19 +1271,19 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('j')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('j')), 20, false, false), Action::MoveCursorBy(1) ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Down), 20, false), + map_key(&km, &mut pending, key(KeyCode::Down), 20, false, false), Action::MoveCursorBy(1) ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('k')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('k')), 20, false, false), Action::MoveCursorBy(-1) ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Up), 20, false), + map_key(&km, &mut pending, key(KeyCode::Up), 20, false, false), Action::MoveCursorBy(-1) ); } @@ -1260,16 +1293,16 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, ctrl_key('d'), 21, false), + map_key(&km, &mut pending, ctrl_key('d'), 21, false, false), Action::MoveCursorBy(10) ); assert_eq!( - map_key(&km, &mut pending, ctrl_key('u'), 21, false), + map_key(&km, &mut pending, ctrl_key('u'), 21, false, false), Action::MoveCursorBy(-10) ); // A pane height of 1 still scrolls by at least one line. assert_eq!( - map_key(&km, &mut pending, ctrl_key('d'), 1, false), + map_key(&km, &mut pending, ctrl_key('d'), 1, false, false), Action::MoveCursorBy(1) ); } @@ -1279,21 +1312,79 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('g')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('g')), 20, false, false), + Action::ScrollTop + ); + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Char('G')), 20, false, false), + Action::ScrollBottom + ); + } + + #[test] + fn g_and_shift_g_map_to_outline_top_and_bottom_when_outline_focused() { + // CS2: `g`/`G` are bound per-view (`scroll-top`/`scroll-bottom` in both View::Diff and + // View::Outline), so the SAME key must resolve to a different Action depending on which + // pane has focus — outline-focused maps to the outline jump, not the diff scroll. + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Char('g')), 20, true, true), + Action::OutlineTop + ); + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Char('G')), 20, true, true), + Action::OutlineBottom + ); + // Diff-focused (`outline_focused = false`) still maps to the diff's own scroll actions, + // even with the outline open — see `g_and_shift_g_map_to_top_and_bottom` above. + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Char('g')), 20, false, true), Action::ScrollTop ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('G')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('G')), 20, false, true), Action::ScrollBottom ); } + #[test] + fn enter_and_shift_e_map_to_expand_gap_in_diff_context() { + // CS8: `enter`/`E` are bound in View::Diff only (`expand-gap`/`expand-gap-all`) — Enter + // stays `OutlineConfirm` when the outline has focus (see the next test). + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Enter), 20, false, false), + Action::ExpandGap + ); + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Char('E')), 20, false, false), + Action::ExpandGapAll + ); + // Still diff-scoped even with the outline open, as long as it isn't focused. + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Enter), 20, false, true), + Action::ExpandGap + ); + } + + #[test] + fn enter_still_maps_to_outline_confirm_when_outline_focused() { + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + assert_eq!( + map_key(&km, &mut pending, key(KeyCode::Enter), 20, true, true), + Action::OutlineConfirm + ); + } + #[test] fn shift_l_maps_to_toggle_layout() { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('L')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('L')), 20, false, false), Action::ToggleLayout ); } @@ -1303,11 +1394,11 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('z')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('z')), 20, false, false), Action::CycleZoom ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('w')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('w')), 20, false, false), Action::ToggleSplitFocus ); } @@ -1317,7 +1408,7 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('r')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('r')), 20, false, false), Action::Refresh ); } @@ -1327,11 +1418,11 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Tab), 20, false), + map_key(&km, &mut pending, key(KeyCode::Tab), 20, false, false), Action::NextFile ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::BackTab), 20, false), + map_key(&km, &mut pending, key(KeyCode::BackTab), 20, false, false), Action::PrevFile ); } @@ -1341,23 +1432,23 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char(']')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char(']')), 20, false, false), Action::None ); // The buffer holds the in-flight chord prefix (generalized from the old `Option`). assert_eq!(pending, vec![KeyPress::from_event(key(KeyCode::Char(']')))]); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('f')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('f')), 20, false, false), Action::NextFile ); assert!(pending.is_empty()); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('[')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('[')), 20, false, false), Action::None ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('f')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('f')), 20, false, false), Action::PrevFile ); } @@ -1366,15 +1457,15 @@ mod tests { fn bracket_h_maps_to_hunk_nav() { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); - map_key(&km, &mut pending, key(KeyCode::Char(']')), 20, false); + map_key(&km, &mut pending, key(KeyCode::Char(']')), 20, false, false); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('h')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('h')), 20, false, false), Action::NextHunk ); - map_key(&km, &mut pending, key(KeyCode::Char('[')), 20, false); + map_key(&km, &mut pending, key(KeyCode::Char('[')), 20, false, false); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('h')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('h')), 20, false, false), Action::PrevHunk ); } @@ -1383,9 +1474,9 @@ mod tests { fn unrecognized_bracket_suffix_drops_pending_without_side_effect() { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); - map_key(&km, &mut pending, key(KeyCode::Char(']')), 20, false); + map_key(&km, &mut pending, key(KeyCode::Char(']')), 20, false, false); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('x')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('x')), 20, false, false), Action::None ); assert!( @@ -1603,24 +1694,24 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('s')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('s')), 20, false, false), Action::StageHunk ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('S')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('S')), 20, false, false), Action::StageFile ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('d')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('d')), 20, false, false), Action::DiscardHunk ); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('D')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('D')), 20, false, false), Action::DiscardFile ); // Ctrl-d keeps its half-page meaning — the plain-`d` staging arm must not shadow it. assert_eq!( - map_key(&km, &mut pending, ctrl_key('d'), 20, false), + map_key(&km, &mut pending, ctrl_key('d'), 20, false, false), Action::MoveCursorBy(10) ); } @@ -1630,7 +1721,7 @@ mod tests { let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); assert_eq!( - map_key(&km, &mut pending, key(KeyCode::Char('v')), 20, false), + map_key(&km, &mut pending, key(KeyCode::Char('v')), 20, false, false), Action::StartSelection ); } @@ -1692,6 +1783,48 @@ mod tests { ); } + #[test] + fn esc_cancels_a_selection_before_focusing_the_outline() { + // Even with the outline open (so a bare Esc would otherwise walk out to it), an active + // selection still wins — the outline-focus move is a lower-precedence fallback, not an + // alternative to selection-cancel. + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .unstaged_file("a.txt", "one\ntwo\n", "one\nCHANGED\n") + .build() + .unwrap(); + // Selection needs a stageable (uncommitted) change; a single changeset seeds the + // outline closed, so open it and hand focus back to the diff. + let mut app = app_from_fixture(&fixture); + app.open_current(); + app.toggle_outline(); + app.focus_diff(); + assert!(app.outline_open() && !app.outline_focused()); + app.start_selection(); + assert!(app.selection_anchor.is_some()); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + + let quit = update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Esc)), + ); + + assert!(!quit, "Esc must not quit while a selection is active"); + assert!( + app.selection_anchor.is_none(), + "Esc cancels the active selection first" + ); + assert!( + !app.outline_focused(), + "the outline-focus move only happens on a LATER Esc, once the selection is gone" + ); + } + #[test] fn pending_confirm_captures_y_and_n_and_ignores_other_keys() { use git_workon_fixture::prelude::*; @@ -1804,7 +1937,7 @@ mod tests { } #[test] - fn o_key_toggles_the_outline_through_its_full_cycle() { + fn o_key_is_a_pure_show_hide_toggle() { use git_workon_fixture::prelude::*; let fixture = FixtureBuilder::new() @@ -1823,7 +1956,10 @@ mod tests { &mut pending, AppEvent::Key(key(KeyCode::Char('o'))), ); - assert!(!app.outline_open(), "o from open+unfocused closes the pane"); + assert!( + !app.outline_open() && !app.outline_focused(), + "o from open+unfocused closes the pane" + ); update( &mut app, @@ -1843,8 +1979,8 @@ mod tests { AppEvent::Key(key(KeyCode::Char('o'))), ); assert!( - app.outline_open() && !app.outline_focused(), - "o from open+focused returns focus to the diff without closing" + !app.outline_open() && !app.outline_focused(), + "o from open+focused closes the pane — the toggle only ever tracks visibility" ); } @@ -1890,7 +2026,83 @@ mod tests { } #[test] - fn esc_does_not_quit_while_the_outline_has_focus() { + fn h_from_the_diff_opens_and_focuses_a_closed_outline() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + app.toggle_outline(); // close + assert!(!app.outline_open() && !app.outline_focused()); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + + update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Char('h'))), + ); + + assert!( + app.outline_open() && app.outline_focused(), + "h from the diff with the outline closed opens AND focuses it" + ); + let items = app.outline_items(); + assert!( + matches!( + items[app.outline_cursor()], + workon_review::outline::OutlineItem::File { cs_idx, file_idx, .. } + if cs_idx == app.current_cs() && file_idx == app.current + ), + "opening via h syncs the outline cursor to the current diff position" + ); + } + + #[test] + fn h_from_the_diff_with_the_outline_already_open_focuses_without_moving_the_cursor() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + app.toggle_outline(); // close + app.toggle_outline(); // open + focus, synced + app.outline_move_by(-1); // manually reposition + // Return focus to the diff without going through `o` (mirrors `l`), so the outline + // stays open but the diff has keyboard focus. + update( + &mut app, + &Keymap::defaults(), + &mut Vec::new(), + AppEvent::Key(key(KeyCode::Char('l'))), + ); + assert!(app.outline_open() && !app.outline_focused()); + let cursor_before = app.outline_cursor(); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + + update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Char('h'))), + ); + + assert!(app.outline_focused(), "h focuses the already-open outline"); + assert_eq!( + app.outline_cursor(), + cursor_before, + "h on an already-open outline must not stomp a manually positioned cursor" + ); + } + + #[test] + fn l_from_the_outline_focuses_the_diff_and_leaves_the_outline_open() { use git_workon_fixture::prelude::*; let fixture = FixtureBuilder::new() @@ -1900,6 +2112,35 @@ mod tests { let mut app = two_committed_changesets_app(&fixture); app.toggle_outline(); // close app.toggle_outline(); // open + focus + assert!(app.outline_open() && app.outline_focused()); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + + update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Char('l'))), + ); + + assert!( + app.outline_open() && !app.outline_focused(), + "l focuses the diff but leaves the outline open" + ); + } + + #[test] + fn esc_quits_while_the_outline_has_focus() { + // Home-base model: the outline has nowhere further out to walk to, so Esc there is the + // terminal leaf of the cascade — same as `q`. + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + app.focus_outline(); assert!(app.outline_focused()); let km = Keymap::defaults(); let mut pending: Vec = Vec::new(); @@ -1911,14 +2152,65 @@ mod tests { AppEvent::Key(key(KeyCode::Esc)), ); - assert!(!quit, "Esc must not quit while the outline has focus"); + assert!(quit, "Esc while the outline has focus quits, like q"); + } + + #[test] + fn esc_focuses_the_outline_from_the_diff_when_open() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + // Default: open, unfocused (diff has focus). + assert!(app.outline_open() && !app.outline_focused()); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + + let quit = update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Esc)), + ); + assert!( - !app.outline_focused(), - "Esc while the outline has focus returns focus to the diff" + !quit, + "Esc must not quit when it can walk out to the outline instead" ); assert!( - app.outline_open(), - "Esc must not also close the pane, only unfocus it" + app.outline_focused(), + "Esc from the diff with the outline open focuses the outline" + ); + assert!(app.outline_open(), "Esc must not close the pane"); + } + + #[test] + fn esc_quits_from_the_diff_when_the_outline_is_closed() { + use git_workon_fixture::prelude::*; + + let fixture = FixtureBuilder::new() + .config("core.autocrlf", "false") + .build() + .unwrap(); + let mut app = two_committed_changesets_app(&fixture); + app.toggle_outline(); // close + assert!(!app.outline_open() && !app.outline_focused()); + let km = Keymap::defaults(); + let mut pending: Vec = Vec::new(); + + let quit = update( + &mut app, + &km, + &mut pending, + AppEvent::Key(key(KeyCode::Esc)), + ); + + assert!( + quit, + "Esc from the diff with the outline closed has nowhere to walk to, so it quits" ); } @@ -1931,6 +2223,10 @@ mod tests { .build() .unwrap(); let mut app = two_committed_changesets_app(&fixture); + // CS3: pin BaseFirst explicitly — this test exercises Enter's header-jump + focus + // return, which is orthogonal to display order, but the `-3` row offset below assumes + // the base->head row layout. + app.set_outline_order(workon_review::outline::OutlineOrder::BaseFirst); app.toggle_outline(); // close app.toggle_outline(); // open + focus, cursor synced onto cs-b's file row assert!(app.outline_focused());