From 7b62e00361cddd8843cb149b276529b930a61c90 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 08:47:43 +0100 Subject: [PATCH] Mark changed lines with a coloured bar, as herdr-reviewr does A bar in the colour of the line, green or red, marks an inserted or a deleted line in place of + or -. The bar takes the same two cells, so the columns, the clicks, and the wrap do not change. The setting diff_signs = true gives the signs back. The review text for the agent keeps + and -. Fixes #3 --- README.md | 1 + diff/diff.go | 6 +++ diff/diff_test.go | 89 ++++++++++++++++++++++++++++++++++- internal/VENDORED.md | 2 + internal/diffview/diffview.go | 19 ++++++-- internal/diffview/terminal.go | 7 +++ tui/config.go | 2 + tui/render.go | 1 + tui/tui_test.go | 36 ++++++++++++-- 9 files changed, 155 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index afab372..fdf0161 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,7 @@ Other settings: - `markdown_view` - `file_list` - `file_list_width` +- `diff_signs`: `true` marks changed lines with `+` and `-`, in place of a coloured bar - `review_prefix` - `url_opener` - `focus_agent_after_send` diff --git a/diff/diff.go b/diff/diff.go index a243075..e44af78 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -64,6 +64,9 @@ type Options struct { // Wrap continues a line that is wider than Width on the next lines, // instead of cutting it. Then Offset does not apply. Wrap bool + // Signs marks a changed line with + or -. Without Signs, a bar in the + // colour of the line marks it. + Signs bool // Theme sets the colours. Theme Theme } @@ -96,6 +99,9 @@ func Render(path, old, new string, o Options) *Diff { if o.Split { dv = dv.Split() } + if o.Signs { + dv = dv.Signs() + } if o.Wrap { dv = dv.Wrap() } diff --git a/diff/diff_test.go b/diff/diff_test.go index 83fa0ad..03470ec 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -108,7 +108,7 @@ func TestRenderSameContentIsEmpty(t *testing.T) { func TestRenderTabsAndOffset(t *testing.T) { d := diff.Render("x.go", "", "\tx := 1\n", diff.Options{Width: 80}) - if got := ansi.Strip(d.Lines[1]); !strings.Contains(got, "+ x := 1") { + if got := ansi.Strip(d.Lines[1]); !strings.Contains(got, "▌ x := 1") { t.Errorf("tab: line = %q", got) } d = diff.Render("x.go", "", "\tx := 1\n", diff.Options{Width: 80, Offset: 6}) @@ -118,6 +118,93 @@ func TestRenderTabsAndOffset(t *testing.T) { } } +// TestMarks checks the mark before the code of each line, in the two +// layouts. By default a bar marks a changed line, as in herdr-reviewr. +// [diff.Options.Signs] gives + and -. The mark takes two cells in both +// cases, so the columns do not move. +func TestMarks(t *testing.T) { + const old, new = "a\nb\nc\n", "a\nB\nc\n" + theme := diff.Theme{ + Background: color.Black, + Green: color.RGBA{0, 0xff, 0, 0xff}, + Red: color.RGBA{0xff, 0, 0, 0xff}, + } + tests := []struct { + name string + o diff.Options + lines []string // the screen lines under the hunk header, without colours + }{ + {"unified bar", diff.Options{Width: 40}, []string{ + " 1 1 a", " 2 ▌ b", " 2 ▌ B", " 3 3 c", + }}, + {"unified signs", diff.Options{Width: 40, Signs: true}, []string{ + " 1 1 a", " 2 - b", " 2 + B", " 3 3 c", + }}, + {"split bar", diff.Options{Width: 40, Split: true}, []string{ + " 1 a 1 a", + " 2 ▌ b 2 ▌ B", + " 3 c 3 c", + }}, + {"split signs", diff.Options{Width: 40, Split: true, Signs: true}, []string{ + " 1 a 1 a", + " 2 - b 2 + B", + " 3 c 3 c", + }}, + } + for _, tt := range tests { + tt.o.Theme = theme + d := diff.Render("x.txt", old, new, tt.o) + if len(d.Lines) != 1+len(tt.lines) { + t.Fatalf("%s: %d lines, want %d", tt.name, len(d.Lines), 1+len(tt.lines)) + } + for i, want := range tt.lines { + got := strings.TrimRight(ansi.Strip(d.Lines[i+1]), " ") + if got != want { + t.Errorf("%s: line %d = %q, want %q", tt.name, i+1, got, want) + } + } + // The mark has the colour of the line: ANSI red for a delete, and + // ANSI green for an insert. + for _, c := range []struct { + line int + fg string + }{{2, "31"}, {3, "32"}} { + if tt.o.Split && c.line == 3 { + continue // the split layout has the insert on the same row + } + if got := markColour(d.Lines[c.line]); got != c.fg { + t.Errorf("%s: the mark of line %d has colour %q, want %q", tt.name, c.line, got, c.fg) + } + } + } + // With a scroll to the right, the bar keeps its colour and shows an + // ellipsis after it. + d := diff.Render("x.txt", "", "abcdefgh\n", diff.Options{Width: 40, Offset: 2}) + if got := ansi.Strip(d.Lines[1]); !strings.Contains(got, "▌…cdefgh") { + t.Errorf("offset: line = %q, want ▌…cdefgh", got) + } + // The review text of a comment uses + and - for both kinds of mark. + for _, signs := range []bool{false, true} { + d := diff.Render("x.txt", old, new, diff.Options{Width: 40, Signs: signs}) + got := snippet(d.Rows[2:5], old, new) + if want := []string{"-b", "+B", " c"}; !slices.Equal(got, want) { + t.Errorf("signs %v: snippet = %q, want %q", signs, got, want) + } + } +} + +// markColour returns the foreground of the SGR sequence just before the +// first bar, + or - in line, for example "31". +func markColour(line string) string { + i := strings.IndexAny(line, "▌+-") + if i < 0 { + return "" + } + sgr := line[strings.LastIndex(line[:i], "\x1b[")+2 : i] + fg, _, _ := strings.Cut(strings.TrimSuffix(sgr, "m"), ";") + return fg +} + func TestSideAt(t *testing.T) { // The unified layout has " 1 " old numbers, " 1 " new numbers, and the // code. diff --git a/internal/VENDORED.md b/internal/VENDORED.md index 8de5730..d6064ee 100644 --- a/internal/VENDORED.md +++ b/internal/VENDORED.md @@ -31,5 +31,7 @@ `Height`, `YOffset`, `InfiniteYScroll` (and the height cut and the scroll code); `MatchLexer`, `LineStyle` and the `Style` fields are unexported; `RowFile` removed + - a coloured bar `▌` marks a changed line, in place of crush's `+` and `-`; `Signs` gives + the signs back - the syntax cache key tells "no background" from black, so a black tint does not leak onto equal lines diff --git a/internal/diffview/diffview.go b/internal/diffview/diffview.go index 5ecc547..1f5a5d9 100644 --- a/internal/diffview/diffview.go +++ b/internal/diffview/diffview.go @@ -23,9 +23,12 @@ const ( ellipsis = "…" // shows where a line is cut ) -// The symbols before the code of a line. +// The symbols before the code of a line. By default, a bar in the colour +// of the line marks an inserted or a deleted line. [View.Signs] changes the +// bar to a sign. const ( symNone = " " // an equal line, a missing line, or a continued line + symBar = "▌ " symInsert = "+ " symDelete = "- " ) @@ -52,6 +55,7 @@ type View struct { style Style tabWidth int wrap bool // wrap long lines instead of cutting them + signs bool // mark a changed line with + or -, not with a bar isComputed bool err error @@ -424,9 +428,15 @@ func fillTo(lines []string, n int, fill func() string) []string { func (v *View) kindStyle(k udiff.OpKind) (lineStyle, string) { switch k { case udiff.Insert: - return v.style.insertLine, symInsert + if v.signs { + return v.style.insertLine, symInsert + } + return v.style.insertLine, symBar case udiff.Delete: - return v.style.deleteLine, symDelete + if v.signs { + return v.style.deleteLine, symDelete + } + return v.style.deleteLine, symBar } return v.style.equalLine, symNone } @@ -467,7 +477,8 @@ func (v *View) codeLines( code = ansi.GraphemeWidth.Cut(code, v.xOffset, len(code)) parts[0] = ansi.Truncate(code, width, ellipsis) if v.xOffset > 0 && strings.TrimSpace(parts[0]) != "" { - sym = sym[:1] + ellipsis + sym, _ = strings.CutSuffix(sym, " ") + sym += ellipsis } } full := lipgloss.NewStyle().MaxWidth(width + leadingSymbolsSize) diff --git a/internal/diffview/terminal.go b/internal/diffview/terminal.go index ac07fc1..c497f90 100644 --- a/internal/diffview/terminal.go +++ b/internal/diffview/terminal.go @@ -7,6 +7,13 @@ import ( "charm.land/lipgloss/v2" ) +// Signs marks an inserted line with + and a deleted line with -, in place +// of a bar. +func (v *View) Signs() *View { + v.signs = true + return v +} + // Wrap makes long lines continue on the next screen lines, instead of // cutting them. The x offset does not apply. func (v *View) Wrap() *View { diff --git a/tui/config.go b/tui/config.go index 791db45..3e68dc1 100644 --- a/tui/config.go +++ b/tui/config.go @@ -72,6 +72,8 @@ type config struct { FileList position `json:"file_list"` // FileListWidth is in columns, minListWidth or more. FileListWidth int `json:"file_list_width"` + // DiffSigns marks a changed line with + or -, in place of a bar. + DiffSigns bool `json:"diff_signs"` // FocusAgent tells if the focus moves to the agent after a send. Nil // means true. FocusAgent *bool `json:"focus_agent_after_send"` diff --git a/tui/render.go b/tui/render.go index bbfb99b..fc94b68 100644 --- a/tui/render.go +++ b/tui/render.go @@ -79,6 +79,7 @@ func (m *model) diffOptions(xoff int) diff.Options { Offset: xoff, Wrap: m.wrap, Split: m.split, + Signs: m.cfg.DiffSigns, Theme: diff.Theme{ Background: m.bg, Foreground: m.fg, diff --git a/tui/tui_test.go b/tui/tui_test.go index 78f1e1d..138225f 100644 --- a/tui/tui_test.go +++ b/tui/tui_test.go @@ -484,21 +484,51 @@ func TestParseConfig(t *testing.T) { "editor": "code -g {file}:{line}", "gitlab_host": "git.corp.example", "focus_agent_after_send": false, - "file_list": "left" + "file_list": "left", + "diff_signs": true }` c, err := parseConfig(good) if err != nil { t.Fatalf("good config: %v", err) } - if c.focusAgent() || c.FileList != positionLeft || c.Layout != LayoutUnified { + if c.focusAgent() || c.FileList != positionLeft || c.Layout != LayoutUnified || !c.DiffSigns { t.Errorf("good config = %+v, want no focus, the list on the left, "+ - "and the default layout", c) + "the default layout, and signs", c) } if c, err := parseConfig(""); err != nil || c.Layout != defaultConfig().Layout { t.Errorf(`parseConfig("") = %+v, %v; want the defaults`, c, err) } } +// TestDiffSigns checks that a bar marks a changed line by default, and +// that the setting diff_signs changes the bar to + and -. +func TestDiffSigns(t *testing.T) { + line := func(m model, code string) string { + for l := range strings.Lines(view(m)) { + if strings.Contains(l, code) { + return l + } + } + return "" + } + m, _ := newTestModel(t) + if l := line(m, "x := 2"); !strings.Contains(l, "▌ x := 2") { + t.Errorf("by default, the inserted line is %q, want a bar", l) + } + cfg, err := parseConfig(`{"diff_signs": true}`) + if err != nil { + t.Fatal(err) + } + m.cfg = cfg + m.rerender() + if l := line(m, "x := 2"); !strings.Contains(l, "+ x := 2") { + t.Errorf("with diff_signs, the inserted line is %q, want +", l) + } + if l := line(m, "return 1"); !strings.Contains(l, "- return 1") { + t.Errorf("with diff_signs, the deleted line is %q, want -", l) + } +} + // TestConfigFlags checks that the flags win over the settings, and that // the settings set the keys and the file list. func TestConfigFlags(t *testing.T) {