tui: align diff display by always showing sign char and keeping fixed gutter (#2353)

diff lines without a sign char were misaligned.
This commit is contained in:
Jeremy Rose
2025-08-15 12:32:45 -04:00
committed by GitHub
parent 265fd89e31
commit 45d6c74682
4 changed files with 30 additions and 15 deletions

View File

@@ -11,3 +11,19 @@ In the codex-rs folder where the rust code lives:
Before finalizing a change to `codex-rs`, run `just fmt` (in `codex-rs` directory) to format the code and `just fix` (in `codex-rs` directory) to fix any linter issues in the code. Additionally, run the tests: Before finalizing a change to `codex-rs`, run `just fmt` (in `codex-rs` directory) to format the code and `just fix` (in `codex-rs` directory) to fix any linter issues in the code. Additionally, run the tests:
1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`. 1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`.
2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`. 2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`.
## Snapshot tests
This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to validate rendered output. When UI or text output changes intentionally, update the snapshots as follows:
- Run tests to generate any updated snapshots:
- `cargo test -p codex-tui`
- Check whats pending:
- `cargo insta pending-snapshots -p codex-tui`
- Review changes by reading the generated `*.snap.new` files directly in the repo, or preview a specific file:
- `cargo insta show -p codex-tui path/to/file.snap.new`
- Only if you intend to accept all new snapshots in this crate, run:
- `cargo insta accept -p codex-tui`
If you dont have the tool:
- `cargo install cargo-insta`

View File

@@ -269,9 +269,9 @@ fn push_wrapped_diff_line(
let mut remaining_text: &str = text; let mut remaining_text: &str = text;
// Reserve a fixed number of spaces after the line number so that content starts // Reserve a fixed number of spaces after the line number so that content starts
// at a consistent column. The sign ("+"/"-") is rendered as part of the content // at a consistent column. Content includes a 1-character diff sign prefix
// and colored with the same foreground as the edited text, not as a separate // ("+"/"-" for inserts/deletes, or a space for context lines) so alignment
// dimmed column. // stays consistent across all diff lines.
let gap_after_ln = SPACES_AFTER_LINE_NUMBER.saturating_sub(ln_str.len()); let gap_after_ln = SPACES_AFTER_LINE_NUMBER.saturating_sub(ln_str.len());
let first_prefix_cols = indent.len() + ln_str.len() + gap_after_ln; let first_prefix_cols = indent.len() + ln_str.len() + gap_after_ln;
let cont_prefix_cols = indent.len() + ln_str.len() + gap_after_ln; let cont_prefix_cols = indent.len() + ln_str.len() + gap_after_ln;
@@ -306,14 +306,10 @@ fn push_wrapped_diff_line(
spans.push(RtSpan::raw(indent)); spans.push(RtSpan::raw(indent));
spans.push(RtSpan::styled(ln_str.clone(), style_dim())); spans.push(RtSpan::styled(ln_str.clone(), style_dim()));
spans.push(RtSpan::raw(" ".repeat(gap_after_ln))); spans.push(RtSpan::raw(" ".repeat(gap_after_ln)));
// Always include a sign character at the start of the displayed chunk
// Prefix the content with the sign if it is an insertion or deletion, and color // ('+' for insert, '-' for delete, ' ' for context) so gutters align.
// the sign and content with the same foreground as the edited text. let sign_char = sign_opt.unwrap_or(' ');
let display_chunk = match sign_opt { let display_chunk = format!("{sign_char}{chunk}");
Some(sign_char) => format!("{sign_char}{chunk}"),
None => chunk.to_string(),
};
let content_span = match line_style { let content_span = match line_style {
Some(style) => RtSpan::styled(display_chunk, style), Some(style) => RtSpan::styled(display_chunk, style),
None => RtSpan::raw(display_chunk), None => RtSpan::raw(display_chunk),
@@ -326,8 +322,9 @@ fn push_wrapped_diff_line(
lines.push(line); lines.push(line);
first = false; first = false;
} else { } else {
// Continuation lines keep a space for the sign column so content aligns
let hang_prefix = format!( let hang_prefix = format!(
"{indent}{}{}", "{indent}{}{} ",
" ".repeat(ln_str.len()), " ".repeat(ln_str.len()),
" ".repeat(gap_after_ln) " ".repeat(gap_after_ln)
); );

View File

@@ -1,13 +1,14 @@
--- ---
source: tui/src/diff_render.rs source: tui/src/diff_render.rs
assertion_line: 380
expression: terminal.backend() expression: terminal.backend()
--- ---
"proposed patch to 1 file (+1 -1) " "proposed patch to 1 file (+1 -1) "
" └ src/lib.rs → src/lib_new.rs (+1 -1) " " └ src/lib.rs → src/lib_new.rs (+1 -1) "
" 1 line one " " 1 line one "
" 2 -line two " " 2 -line two "
" 2 +line two changed " " 2 +line two changed "
" 3 line three " " 3 line three "
" " " "
" " " "
" " " "

View File

@@ -1,9 +1,10 @@
--- ---
source: tui/src/diff_render.rs source: tui/src/diff_render.rs
assertion_line: 380
expression: terminal.backend() expression: terminal.backend()
--- ---
" 1 +this is a very long line that should wrap across multiple terminal col " " 1 +this is a very long line that should wrap across multiple terminal col "
" umns and continue " " umns and continue "
" " " "
" " " "
" " " "