From dafaf70aca288d8ee8e789ec376128b81b25b6af Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 5 Jan 2025 15:30:03 +0100 Subject: [PATCH 1/5] Clean up: fix misleading comment --- view.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/view.go b/view.go index 5a331b4..7736f9f 100644 --- a/view.go +++ b/view.go @@ -690,7 +690,7 @@ func (v *View) makeWriteable(x, y int) { v.lines = append(v.lines, nil) } } - // cell `x` must not be index-able (that's why `<`) + // cell `x` need not be index-able (that's why `<`) // append should be used by `lines[y]` user if he wants to write beyond `x` for len(v.lines[y]) < x { if cap(v.lines[y]) > len(v.lines[y]) { From 2168f2aab95401bcf1aeab27cc5c6104b548e457 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 5 Jan 2025 15:18:25 +0100 Subject: [PATCH 2/5] Add test for writeRunes The test shows a bug with the handling of \n (but not with \r, it works fine there): if there's exactly one existing character in the current line after the write position, we overwrite it with a null character. We'll fix this in the next commit. --- view_test.go | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/view_test.go b/view_test.go index 7677aa7..91e5424 100644 --- a/view_test.go +++ b/view_test.go @@ -10,6 +10,86 @@ import ( "github.com/stretchr/testify/assert" ) +func TestWriteRunes(t *testing.T) { + tests := []struct { + existingLines []string + stringToWrite string + expectedLines []string + }{ + { + []string{}, + "", + []string{""}, + }, + { + []string{}, + "1\n", + []string{"1\x00", ""}, + }, + { + []string{"a"}, + "1\n", + []string{"1\x00", ""}, + }, + { + []string{"a\x00"}, + "1\n", + []string{"1\x00", ""}, + }, + { + []string{"ab"}, + "1\n", + /* EXPECTED: + []string{"1b", ""}, + ACTUAL: */ + []string{"1\x00", ""}, + }, + { + []string{"abc"}, + "1\n", + []string{"1bc", ""}, + }, + { + []string{}, + "1\r", + []string{"1\x00"}, + }, + { + []string{"a"}, + "1\r", + []string{"1\x00"}, + }, + { + []string{"a\x00"}, + "1\r", + []string{"1\x00"}, + }, + { + []string{"ab"}, + "1\r", + []string{"1b"}, + }, + { + []string{"abc"}, + "1\r", + []string{"1bc"}, + }, + } + + for _, test := range tests { + v := NewView("name", 0, 0, 10, 10, OutputNormal) + for _, l := range test.existingLines { + v.lines = append(v.lines, stringToCells(l)) + } + v.writeRunes([]rune(test.stringToWrite)) + var resultingLines []string + for _, l := range v.lines { + resultingLines = append(resultingLines, cellsToString(l)) + } + assert.Equal(t, test.expectedLines, resultingLines) + } +} + func TestUpdatedCursorAndOrigin(t *testing.T) { tests := []struct { prevOrigin int From f2c8c241f45f3224deead68070955056f79e8067 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 5 Jan 2025 15:19:31 +0100 Subject: [PATCH 3/5] Fix off-by-one error This fixes the bug described in the previous commit. This is the only behavior change in this branch, the rest of the commits are refactorings to simplify the code. --- view.go | 2 +- view_test.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/view.go b/view.go index 7736f9f..823c7a0 100644 --- a/view.go +++ b/view.go @@ -766,7 +766,7 @@ func (v *View) writeRunes(p []rune) { switch r { case '\n': v.autoRenderHyperlinksInCurrentLine() - if c, ok := v.readCell(v.wx+1, v.wy); !ok || c.chr == 0 { + if c, ok := v.readCell(v.wx, v.wy); !ok || c.chr == 0 { v.writeCells(v.wx, v.wy, []cell{{ chr: 0, fgColor: 0, diff --git a/view_test.go b/view_test.go index 91e5424..f346a28 100644 --- a/view_test.go +++ b/view_test.go @@ -39,10 +39,7 @@ func TestWriteRunes(t *testing.T) { { []string{"ab"}, "1\n", - /* EXPECTED: []string{"1b", ""}, - ACTUAL: */ - []string{"1\x00", ""}, }, { []string{"abc"}, From 1e9abd05cbc05a99770ca54f9fc2ca1a49e87a57 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 5 Jan 2025 15:22:22 +0100 Subject: [PATCH 4/5] Extract helper function This has become possible with the previous commit, which made the code identical between \r and \n. --- view.go | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/view.go b/view.go index 823c7a0..5bbec58 100644 --- a/view.go +++ b/view.go @@ -762,31 +762,28 @@ func (v *View) writeRunes(p []rune) { // Fill with empty cells, if writing outside current view buffer v.makeWriteable(v.wx, v.wy) + finishLine := func() { + v.autoRenderHyperlinksInCurrentLine() + if c, ok := v.readCell(v.wx, v.wy); !ok || c.chr == 0 { + v.writeCells(v.wx, v.wy, []cell{{ + chr: 0, + fgColor: 0, + bgColor: 0, + }}) + } + } + for _, r := range p { switch r { case '\n': - v.autoRenderHyperlinksInCurrentLine() - if c, ok := v.readCell(v.wx, v.wy); !ok || c.chr == 0 { - v.writeCells(v.wx, v.wy, []cell{{ - chr: 0, - fgColor: 0, - bgColor: 0, - }}) - } + finishLine() v.wx = 0 v.wy++ if v.wy >= len(v.lines) { v.lines = append(v.lines, nil) } case '\r': - v.autoRenderHyperlinksInCurrentLine() - if c, ok := v.readCell(v.wx, v.wy); !ok || c.chr == 0 { - v.writeCells(v.wx, v.wy, []cell{{ - chr: 0, - fgColor: 0, - bgColor: 0, - }}) - } + finishLine() v.wx = 0 default: truncateLine, cells := v.parseInput(r, v.wx, v.wy) From deb0e10021deba75f9bc75bc069114475f1a659c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 5 Jan 2025 15:29:47 +0100 Subject: [PATCH 5/5] Simplify code The readCell function was introduced in e659668de4 only for this purpose. We don't need it because: - the extra check for c.chr == 0 is not needed; if there is already a null cell at the line end, we needn't write it again. There is no difference between a null cell and a printable cell in this regard - the only check that's left is for the coordinates; we know that y is valid, and x is not less than 0, so the only check we need is for x against the length of the line. --- view.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/view.go b/view.go index 5bbec58..a83b617 100644 --- a/view.go +++ b/view.go @@ -726,14 +726,6 @@ func (v *View) writeCells(x, y int, cells []cell) { v.lines[y] = line[:newLen] } -// readCell gets cell at specified location (x, y) -func (v *View) readCell(x, y int) (cell, bool) { - if y < 0 || y >= len(v.lines) || x < 0 || x >= len(v.lines[y]) { - return cell{}, false - } - return v.lines[y][x], true -} - // Write appends a byte slice into the view's internal buffer. Because // View implements the io.Writer interface, it can be passed as parameter // of functions like fmt.Fprintf, fmt.Fprintln, io.Copy, etc. Clear must @@ -764,7 +756,7 @@ func (v *View) writeRunes(p []rune) { finishLine := func() { v.autoRenderHyperlinksInCurrentLine() - if c, ok := v.readCell(v.wx, v.wy); !ok || c.chr == 0 { + if v.wx >= len(v.lines[v.wy]) { v.writeCells(v.wx, v.wy, []cell{{ chr: 0, fgColor: 0,