More straightforward newline handling

fixtures/pikachu.sh.rendered

@@ -43,4 +43,3 @@
                           <span class="term-fgx144">0</span><span class="term-fgx138">1</span><span class="term-fgx144">10</span><span class="term-fgx143">1</span><span class="term-fgx100">1</span><span class="term-fgx58">1</span>                            <span class="term-fgx94">0</span><span class="term-fgx222">11</span><span class="term-fgx179">1</span><span class="term-fgx228">0</span><span class="term-fgx143">0</span><span class="term-fgx180">0</span>
                                                                <span class="term-fgx59">0</span><span class="term-fgx102">1</span><span class="term-fgx144">0</span><span class="term-fgx187">0</span><span class="term-fgx101">0</span>
 &nbsp;
-&nbsp;

output.go

@@ -77,10 +77,12 @@ func (b *outputBuffer) appendChar(char rune) {
 	}
 }
 
-// lineToHTML joins parts of a line together and renders them in HTML. It
-// ignores the newline field (i.e. assumes all parts are !newline except the
-// last part). The output string will have a terminating \n.
+// lineToHTML joins parts of a line together and renders them in HTML.
 func lineToHTML(parts []screenLine) string {
+	if len(parts) == 0 {
+		return ""
+	}
+
 	var buf outputBuffer
 
 	// Combine metadata - last metadata wins.
@@ -175,10 +177,12 @@ func lineToHTML(parts []screenLine) string {
 	closeFrom(0)
 
 	out := strings.TrimRight(buf.String(), " \t")
-	if out == "" {
-		return " \n"
+	if parts[len(parts)-1].newline {
+		if out == "" {
+			return " \n"
+		}
+		out += "\n"
 	}
-	out += "\n"
 	return out
 }
 

output_test.go

@@ -18,12 +18,12 @@ func TestScreenLineAsHTML_Interleaving(t *testing.T) {
 		{
 			name:  "a span /a /span",
 			input: "five \x1b]8;;http://example.com\x1b\\six \x1b[35mseven \x1b]8;;\x1b\\eight\x1b[0m",
-			want:  `five <a href="http://example.com">six <span class="term-fg35">seven </span></a><span class="term-fg35">eight</span>` + "\n",
+			want:  `five <a href="http://example.com">six <span class="term-fg35">seven </span></a><span class="term-fg35">eight</span>`,
 		},
 		{
 			name:  "span a /span /a",
 			input: "five \x1b[35msix \x1b]8;;http://example.com\x1b\\seven \x1b[0meight\x1b]8;;\x1b\\",
-			want:  `five <span class="term-fg35">six <a href="http://example.com">seven </a></span><a href="http://example.com">eight</a>` + "\n",
+			want:  `five <span class="term-fg35">six <a href="http://example.com">seven </a></span><a href="http://example.com">eight</a>`,
 		},
 	}
 
@@ -42,6 +42,13 @@ func TestScreenLineAsHTML_Interleaving(t *testing.T) {
 			if diff := cmp.Diff(got, test.want); diff != "" {
 				t.Errorf("lineToHTML(s.screen[:1]) diff (-got +want):\n%s", diff)
 			}
+
+			// Test that it respects newlines
+			s.screen[0].newline = true
+			got = lineToHTML(s.screen[:1])
+			if diff := cmp.Diff(got, test.want + "\n"); diff != "" {
+				t.Errorf("lineToHTML(s.screen[:1]) diff (-got +want):\n%s", diff)
+			}
 		})
 	}
 }

screen.go

@@ -44,10 +44,6 @@ type Screen struct {
 	// It defaults to 160 columns * 100 lines.
 	cols, lines int
 
-	// When multiple screen lines are scrolled out at once, their storage can be
-	// recycled later on.
-	nodeRecycling [][]node
-
 	// Optional callback. If not nil, as each line is scrolled out of the top of
 	// the buffer, this func is called with the HTML.
 	// The line will always have a `\n` suffix.
@@ -220,28 +216,25 @@ func (s *Screen) currentLineForWriting() *screenLine {
 		// be false.
 		s.currentLine().newline = false
 		s.y++
+	} else if len(s.screen) > 0 && s.currentLine() == nil {
+		// Since we will be adding new lines and we are not continuing from the
+		// previous line, ensure the last line ends in a newline.
+		s.screen[len(s.screen)-1].newline = true
 	}
+	// This is a pointer to the most recent line we added during this method.
+	var addedLine *screenLine
 	// Ensure there are enough lines on screen to start writing here.
 	for s.currentLine() == nil {
+		if addedLine != nil {
+			// we are adding a line below a line we just added, so that line needs a
+			// newline.
+			addedLine.newline = true
+		}
 		// If maxLines is not in use, or adding a new line would not make it
 		// larger than maxLines, then just allocate a new line.
 		if s.maxLines <= 0 || len(s.screen)+1 <= s.maxLines {
-			var nodes []node
-			if len(s.nodeRecycling) > 0 {
-				// Pop one off the end of nodeRecycling
-				r1 := len(s.nodeRecycling) - 1
-				nodes = s.nodeRecycling[r1]
-				s.nodeRecycling = s.nodeRecycling[:r1]
-			}
-			if nodes == nil {
-				// No slices available for recycling, make a new one.
-				nodes = make([]node, 0, s.cols)
-			}
-			newLine := screenLine{
-				nodes:   nodes,
-				newline: true,
-			}
-			s.screen = append(s.screen, newLine)
+			s.screen = append(s.screen, screenLine{nodes: make([]node, 0, s.cols)})
+			addedLine = &s.screen[len(s.screen)-1]
 			if s.y >= s.lines {
 				// Because the "window" is always the last s.lines of s.screen
 				// (or all of them, if there are fewer lines than s.lines)
@@ -254,52 +247,22 @@ func (s *Screen) currentLineForWriting() *screenLine {
 
 		// maxLines is in effect, and adding a new line would make the screen
 		// larger than maxLines.
-		// Pass the whole line being scrolled out to ScrollOutFunc if available,
-		// otherwise just scroll out 1 line to nowhere.
-		scrollOutTo := 1
+		// Scroll out one line.
 		if s.ScrollOutFunc != nil {
-			// Whole lines need to be passed to the callback. Find the end of
-			// the line (the screen line with newline = true).
-			// The majority of the time this will just be the first screen line.
-			// If it's all one enormous line, stop at the top of the screen.
-			// (so, allow scrollout to eat all of the "scrollback" but none of
-			// the "visible screen". We're talking a line that's 160*200
-			// chars long for the top of the screen to be reached that way.)
-			scrollOutTo = s.top()
-			if s.top() == 0 {
-				// We still need to scroll out a line, even if there are no lines above
-				// the top of the window. Get the next line.
-				scrollOutTo = len(s.screen)
-			}
-			for i, l := range s.screen[:scrollOutTo] {
-				if l.newline {
-					scrollOutTo = i + 1
-					break
-				}
-			}
-			s.ScrollOutFunc(lineToHTML(s.screen[:scrollOutTo]))
-		}
-		for i := range scrollOutTo {
-			s.nodeRecycling = append(s.nodeRecycling, s.screen[i].nodes[:0])
-		}
-		s.LinesScrolledOut += scrollOutTo
-
-		var nodes []node
-		if r1 := len(s.nodeRecycling) - 1; r1 >= 0 {
-			// Make a new line on the bottom using a recycled node slice. There's
-			// usually at least one we just added.
-			nodes = s.nodeRecycling[r1]
-			s.nodeRecycling = s.nodeRecycling[:r1]
-		} else {
-			// No nodes to recycle, make a new node slice. This happens when we scroll
-			// out a line that consisted of no screenlines.
-			nodes = make([]node, 0, s.cols)
+			s.ScrollOutFunc(lineToHTML(s.screen[:1]))
 		}
-		newLine := screenLine{
-			nodes:   nodes,
-			newline: true,
-		}
-		s.screen = append(s.screen[scrollOutTo:], newLine)
+
+		// Remove the scrolled out line and add a new line.
+		s.screen = append(
+			s.screen,
+			screenLine{
+				// recycle the nodes from the line we are removing
+				nodes: s.screen[0].nodes[:0:s.cols],
+			},
+		)[1:]
+
+		addedLine = &s.screen[len(s.screen)-1]
+		s.LinesScrolledOut++
 
 		// Since the buffer added 1 line, s.y moves upwards.
 		s.y--
@@ -504,6 +467,10 @@ func (s *Screen) applyEscape(code rune, instructions []string) {
 				s.screen[i].clearAll()
 			}
 		}
+		if len(s.screen) > 0 {
+			// The final line should not have a newline.
+			s.screen[len(s.screen)-1].newline = false
+		}
 
 	case 'K': // Erase in Line: erases part of the line.
 		switch inst(0) {
@@ -515,6 +482,10 @@ func (s *Screen) applyEscape(code rune, instructions []string) {
 
 		case "2":
 			s.currentLine().clearAll()
+			if len(s.screen) > 0 {
+				// The final line should not have a newline.
+				s.screen[len(s.screen)-1].newline = false
+			}
 		}
 
 	case 'M':
@@ -546,8 +517,14 @@ func (s *Screen) AsHTML() string {
 		screen = screen[lineEnd:]
 	}
 
-	// For backwards compatibility the final newline is trimmed.
-	return strings.TrimSuffix(sb.String(), "\n")
+	// For backwards compatibility the final newline is trimmed. If the final line
+	// consists of only a non-breaking space, trim that, too.
+	render := sb.String()
+	var ok bool
+	if render, ok = strings.CutSuffix(render, "\n"); ok {
+	  render = strings.TrimSuffix(render, "&nbsp;")
+	}
+	return render
 }
 
 // AsPlainText renders the screen without any ANSI style etc.
@@ -566,13 +543,16 @@ func (s *Screen) newLine() {
 	// give us the next line if the cursor was placed past the end of the line.
 	s.x = 0
 
-	// Ensure the previous line, if it already exists, gets a \n in the render.
-	// This could happen if we got CSI A (cursor up), and then \n onto a line
-	// that had previously been wrapped from the previous line.
-	if line := s.currentLine(); line != nil {
-		line.newline = true
-	}
+	// Ensure the current line gets a \n in the render.
+	// This could be necessary if the cursor was moved to a line that wraps to the
+	// next line.
+	s.currentLineForWriting().newline = true
 	s.y++
+	// newlines are real characters being printed, not just moving the cursor.
+	// Getting the current line will force the Screen to add any missing lines
+	// to the slice of screenlines, which ensures that they get rendered and 
+	// scrolls out any content that should be.
+	_ = s.currentLineForWriting()
 }
 
 func (s *Screen) revNewLine() {

terminal_test.go

@@ -51,6 +51,11 @@ var rendererTestCases = []struct {
 		input: "hello\n",
 		want:  "hello",
 	},
+	{
+		name:  "handles trailing double newline",
+		input: "hello\n\n",
+		want:  "hello\n",
+	},
 	{
 		name:  "closes colors that get opened",
 		input: "he\033[32mllo",
@@ -175,7 +180,7 @@ var rendererTestCases = []struct {
 	{
 		name:  "allows clearing lines below the current line",
 		input: "foo\nbar\x1b[A\x1b[Jbaz",
-		want:  "foobaz\n ",
+		want:  "foobaz",
 	},
 	{
 		name:  "doesn't freak out about clearing lines below when there aren't any",
@@ -490,8 +495,7 @@ func TestStreamingRendererAgainstCases(t *testing.T) {
 }
 
 func TestStreamingRendererAgainstFixtures(t *testing.T) {
-	// Streaming vs non-streaming differs in adding a \n on extraordinarily
-	// huge lines, but huge-line is an important test for streaming mode.
+	// huge-line is an important test for streaming mode.
 	for _, base := range append(TestFiles, "huge-line.sh") {
 		t.Run(fmt.Sprintf("for fixture %q", base), func(t *testing.T) {
 			raw := loadFixture(t, base, "raw")