Fix a rendering bug with filled lines

Inserting an actual newline at the end of a soft line-broken box no
longer inserts a spurious blank line. This fixes a pernicious
rendering issue.

frame/boxtest_helpers_test.go

@@ -69,6 +69,7 @@ func comparecore(t *testing.T, prefix string, testvector []BoxTester) {
 // computed box found in frame. prefix and name describe the test and i is the
 // box index.
 func expectedboxesequal(t *testing.T, prefix, name string, i int, frame *frameimpl, afterboxes []*frbox) {
+	t.Helper()
 	if got, want := frame.box[i], afterboxes[i]; !reflect.DeepEqual(got, want) {
 		switch {
 		case got == nil && want != nil:
@@ -87,7 +88,8 @@ func expectedboxesequal(t *testing.T, prefix, name string, i int, frame *frameim
 	}
 }
 
-// testcore checks if the frame's box model matches the provided afterboxes, nbox, Use this to implement Verify methods.
+// testcore checks if the frame's box model matches the provided
+// afterboxes, nbox, Use this to implement Verify methods.
 func testcore(t *testing.T, prefix, name string, frame *frameimpl, nbox int, afterboxes []*frbox) {
 	if got, want := len(frame.box), nbox; got != want {
 		t.Errorf("%s-%s: len(frame.box) got %d but want %d\n", prefix, name, got, want)
@@ -96,6 +98,12 @@ func testcore(t *testing.T, prefix, name string, frame *frameimpl, nbox int, aft
 		t.Errorf("%s-%s: ran add but did not succeed in creating boxex", prefix, name)
 	}
 
+	if len(afterboxes) > len(frame.box) {
+		frame.TestLogboxes(t, "mismatch of length between expected, desired")
+		t.Errorf("wrong number of afterboxes. Should have %d boxes, got %d", len(afterboxes), len(frame.box))
+		return
+	}
+
 	// First part of box array must match the provided afterboxes slice.
 	for i := range afterboxes {
 		expectedboxesequal(t, prefix, name, i, frame, afterboxes)
@@ -108,3 +116,17 @@ func testcore(t *testing.T, prefix, name string, frame *frameimpl, nbox int, aft
 		}
 	}
 }
+
+// Logboxes shows the box model to the log for debugging convenience.
+func (f *frameimpl) TestLogboxes(t *testing.T, message string, args ...interface{}) {
+	t.Helper()
+	t.Logf(message, args...)
+	for i, b := range f.box {
+		if b != nil {
+			t.Logf("	box[%d] -> %v\n", i, b)
+		} else {
+			t.Logf("	box[%d] is WRONGLY nil\n", i)
+		}
+	}
+	t.Logf("end: "+message, args...)
+}

frame/draw.go

@@ -237,7 +237,7 @@ func (f *frameimpl) Tick(pt image.Point, ticked bool) {
 }
 
 func (f *frameimpl) _draw(pt image.Point) image.Point {
-	// f.LogBoxes("_draw -- start")
+	// f.Logboxes("_draw -- start")
 	for nb := 0; nb < len(f.box); nb++ {
 		b := f.box[nb]
 		if b == nil {
@@ -246,6 +246,7 @@ func (f *frameimpl) _draw(pt image.Point) image.Point {
 		}
 		pt = f.cklinewrap0(pt, b)
 		if pt.Y == f.rect.Max.Y {
+			f.lastlinefull = true
 			f.nchars -= f.strlen(nb)
 			f.delbox(nb, len(f.box)-1)
 			break
@@ -270,7 +271,7 @@ func (f *frameimpl) _draw(pt image.Point) image.Point {
 			}
 		}
 	}
-	// f.LogBoxes("_draw -- end")
+	// f.Logboxes("_draw -- end")
 	return pt
 }
 

frame/frame.go

@@ -253,6 +253,10 @@ type frbox struct {
 // 	m.reallock.Unlock()
 // }
 
+// TODO(rjk): It might make sense to group frameimpl into context (e.g.
+// fonts, etc.) and the actual boxes. At any rate, it's worth thinking
+// carefully about the data structures and how they should really be put
+// together.
 type frameimpl struct {
 	lk sync.Mutex
 	// lk debugginglock

frame/insert.go

@@ -26,7 +26,7 @@ func (frame *frameimpl) addifnonempty(box *frbox, inby []byte) *frbox {
 
 // bxscan divides inby into single-line, nl and tab boxes. bxscan assumes that
 // it has ownership of inby
-func (f *frameimpl) bxscan(inby []byte, ppt *image.Point) (image.Point, *frameimpl) {
+func (f *frameimpl) bxscan(inby []byte, p, bn int) (image.Point, image.Point, *frameimpl) {
 	frame := &frameimpl{
 		rect:              f.rect,
 		display:           f.display,
@@ -38,7 +38,7 @@ func (f *frameimpl) bxscan(inby []byte, ppt *image.Point) (image.Point, *frameim
 		box:               []*frbox{},
 	}
 
-	// TODO(rjk): This is (conceivably) pointless works?
+	// TODO(rjk): This is probably unnecessary.
 	copy(frame.cols[:], f.cols[:])
 
 	nl := 0
@@ -92,8 +92,19 @@ func (f *frameimpl) bxscan(inby []byte, ppt *image.Point) (image.Point, *frameim
 	}
 	frame.addifnonempty(wipbox, []byte{})
 
-	*ppt = f.cklinewrap0(*ppt, frame.box[0])
-	return frame._draw(*ppt), frame
+	newboxes := frame.box
+
+	// Temporarily create prefixboxes to find the position of (a possibly
+	// infinitely thin) rune immediately at position p.
+	prefixboxes := f.box[0:bn]
+	frame.box = prefixboxes
+	pt0 := frame.ptofcharptb(p, f.rect.Min, 0)
+
+	frame.box = newboxes
+	pt1 := frame._draw(pt0)
+	f.lastlinefull = frame.lastlinefull
+
+	return pt0, pt1, frame
 }
 
 func (f *frameimpl) chop(pt image.Point, p, bn int) {
@@ -143,9 +154,9 @@ func (f *frameimpl) insertimpl(r []rune, p0 int) bool {
 }
 
 func (f *frameimpl) insertbyteimpl(inby []byte, p0 int) bool {
-	// log.Printf("frame.Insert. Start: %q", string(inby))
-	// defer log.Println("frame.Insert end")
-	//	f.Logboxes("at very start of insert")
+	//log.Printf("frame.Insert. Start: %q", string(inby))
+	//defer log.Println("frame.Insert end")
+	//f.Logboxes("at very start of insert")
 	f.validateboxmodel("Frame.Insert Start p0=%d, «%s»", p0, string(inby))
 	defer f.validateboxmodel("Frame.Insert End p0=%d, «%s»", p0, string(inby))
 	f.validateinputs(inby, "Frame.Insert Start")
@@ -166,26 +177,28 @@ func (f *frameimpl) insertbyteimpl(inby []byte, p0 int) bool {
 	}
 
 	//	f.Logboxes("at end of findbox")
+	//	log.Println("n0", n0)
 
 	cn0 := p0
 	nn0 := n0
-	pt0 := f.ptofcharnb(p0, n0)
-	ppt0 := pt0
-	opt0 := pt0
 
-	pt1, nframe := f.bxscan(inby, &ppt0)
+	// ppt0 and ppt1 are start and end of insertion as they will appear when
+	// insertion is complete. pt0 is current location of insertion position.
+	// (p0); pt1 is terminal point (without line wrap) of insertion.
+	pt0, pt1, nframe := f.bxscan(inby, p0, n0)
+
+	// TODO(rjk): Figure out why opt0 needs to exist.
+	opt0 := pt0
+	ppt0 := pt0
 	ppt1 := pt1
 
+	// TODO(rjk): I am not sure what this block does. Or if it is doing the
+	// right thing.
 	if n0 < len(f.box) {
 		pt0 = f.cklinewrap(pt0, f.box[n0])
 		ppt1 = f.cklinewrap0(ppt1, f.box[n0])
 	}
 	f.modified = true
-	/*
-	 * ppt0 and ppt1 are start and end of insertion as they will appear when
-	 * insertion is complete. pt0 is current location of insertion position
-	 * (p0); pt1 is terminal point (without line wrap) of insertion.
-	 */
 
 	// Remove the selection or tick. This will redraw all selected text characters.
 	// TODO(rjk): Do not remove the selection if it's unnecessary to do so.
@@ -340,12 +353,15 @@ func (f *frameimpl) insertbyteimpl(inby []byte, p0 int) bool {
 	f.SelectPaint(ppt0, ppt1, col)
 	nframe.drawtext(ppt0, tcol, col)
 
+	// Skip the rest if nothing is added. This means that f.lastlinefull is valid.
+	if len(nframe.box) == 0 {
+		return f.lastlinefull
+	}
+
 	// Actually add boxes.
 	f.addbox(nn0, len(nframe.box))
 	copy(f.box[nn0:], nframe.box)
 
-	// f.Logboxes("after adding")
-
 	if nn0 > 0 && f.box[nn0-1].Nrune >= 0 && ppt0.X-f.box[nn0-1].Wid >= f.rect.Min.X {
 		nn0--
 		ppt0.X -= f.box[nn0].Wid
@@ -356,7 +372,6 @@ func (f *frameimpl) insertbyteimpl(inby []byte, p0 int) bool {
 		n0++
 	}
 	f.clean(ppt0, nn0, n0+1)
-	//	f.Logboxes("after clean")
 	f.nchars += nframe.nchars
 	if f.sp0 >= p0 {
 		f.sp0 += nframe.nchars

frame/insert_more_test.go

@@ -45,12 +45,23 @@ func TestInsertAligned(t *testing.T) {
 			name:     "splitWrappedLine",
 			fn:       splitWrappedLine,
 			textarea: image.Rect(20, 10, 59, 60),
-			// This inserts an additional blankline for a newline added to the end of
-			// a full text row that doesn't belong there. The contents of the screen
-			// no longer match what we'd expect based on the box model. e.g.
-			// insertForcesWrap below shows that the newline should add a box without
-			// actually drawing anything. Subsequent edits then induce confusion.
-			knowntofail: true,
+			want: []string{
+				"fill (20,10)-(59,20) [0,0],[3,1]",
+				"fill (20,20)-(59,50) [0,1],[3,3]",
+				"fill (20,50)-(33,60) [0,4],[1,1]",
+				`screen-800x600 <- string "a本ポ" atpoint: (20,10) [0,0] fill: black`,
+				`screen-800x600 <- string "ポポポ" atpoint: (20,20) [0,1] fill: black`,
+				`screen-800x600 <- string "ポポh" atpoint: (20,30) [0,2] fill: black`,
+				`screen-800x600 <- string "ell" atpoint: (20,40) [0,3] fill: black`,
+				`screen-800x600 <- string "o" atpoint: (20,50) [0,4] fill: black`,
+				// The previously failing insertion starts here. We didn't have to do
+				// anything in this case. But we still fill blank space at the end of the
+				// line over again. This is (hopefully) harmless.
+				// TODO(rjk): Elide the 0-width draws.
+				"fill (58,10)-(59,20) [-,0],[-,1]",
+				"fill (20,20)-(20,30) [0,1],[0,1]",
+			},
+			knowntofail: false,
 		},
 		{
 			// Insert a single character that forces conversion of non-wrapped to
@@ -92,6 +103,9 @@ func TestInsertAligned(t *testing.T) {
 				`screen-800x600 <- string "2ef" atpoint: (20,30) [0,2] fill: black`,
 				`screen-800x600 <- string "3gh" atpoint: (20,40) [0,3] fill: black`,
 				`screen-800x600 <- string "4ij" atpoint: (20,50) [0,4] fill: black`,
+				"fill (58,50)-(59,60) [-,4],[-,1]",
+				// Doesn't this stick below? it's 0 wide?
+				"fill (20,60)-(20,70) [0,5],[0,1]",
 			},
 		},