From d4faf98455feb57dec8f573edef4b760ebe77495 Mon Sep 17 00:00:00 2001
From: Leonard Hecker <lhecker@microsoft.com>
Date: Fri, 3 May 2024 01:26:03 +0200
Subject: [PATCH] Fix remaining buffer serialization bugs (#17182)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It may be more accurate to say: "Fix _known_ remaining buffer
serialization bugs", but I'll try to be positive about my code.

Initially, the buffer is initialized with the default attributes,
but once it begins to scroll, newly scrolled in rows are initialized
with the current attributes. This means we need to set the current
attributes to those of the upcoming row before the row comes up.

This is related to #17074.

## Validation Steps Performed
* Persist and restore a buffer 10 times
* All previous "Restore" status messages look correct ✅
* The escape sequences in the buffer file look correct ✅
---
 src/buffer/out/textBuffer.cpp | 52 +++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 9 deletions(-)

diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
index 2aa9176a8c..f0c893bc60 100644
--- a/src/buffer/out/textBuffer.cpp
+++ b/src/buffer/out/textBuffer.cpp
@@ -2559,6 +2559,7 @@ void TextBuffer::Serialize(const wchar_t* destination) const
     TextColor previousBg;
     TextColor previousUl;
     uint16_t previousHyperlinkId = 0;
+    bool delayedLineBreak = false;
 
     // This iterates through each row. The exit condition is at the end
     // of the for() loop so that we can properly handle file flushing.
@@ -2578,9 +2579,11 @@ void TextBuffer::Serialize(const wchar_t* destination) const
         }
 
         const auto& runs = row.Attributes().runs();
-        auto it = runs.begin();
+        const auto beg = runs.begin();
         const auto end = runs.end();
+        auto it = beg;
         const auto last = end - 1;
+        const auto lastCharX = row.MeasureRight();
         til::CoordType oldX = 0;
 
         for (; it != end; ++it)
@@ -2760,24 +2763,55 @@ void TextBuffer::Serialize(const wchar_t* destination) const
                 }
             }
 
-            auto newX = oldX + it->length;
-            // Trim whitespace with default attributes from the end of each line.
-            if (it == last && it->value == TextAttribute{})
+            // Initially, the buffer is initialized with the default attributes, but once it begins to scroll,
+            // newly scrolled in rows are initialized with the current attributes. This means we need to set
+            // the current attributes to those of the upcoming row before the row comes up. Or inversely:
+            // We let the row come up, let it set its attributes and only then print the newline.
+            if (delayedLineBreak)
             {
-                // This can result in oldX > newX, but that's okay because GetText()
-                // is robust against that and returns an empty string.
-                newX = row.MeasureRight();
+                buffer.append(L"\r\n");
+                delayedLineBreak = false;
+            }
+
+            auto newX = oldX + it->length;
+
+            // Since our text buffer doesn't store the original input text, the information over the amount of trailing
+            // whitespaces was lost. If we don't do anything here then a row that just says "Hello" would be serialized
+            // to "Hello                    ...". If the user restores the buffer dump with a different window size,
+            // this would result in some fairly ugly reflow. This code attempts to at least trim trailing whitespaces.
+            //
+            // As mentioned above for `delayedLineBreak`, rows are initialized with their first attribute, BUT
+            // only if the viewport has begun to scroll. Otherwise, they're initialized with the default attributes.
+            // In other words, we can only skip \x1b[K = Erase in Line, if both the first/last attribute are the default attribute.
+            static constexpr TextAttribute defaultAttr;
+            const auto trimTrailingWhitespaces = it == last && lastCharX < newX;
+            const auto clearToEndOfLine = trimTrailingWhitespaces && beg->value != defaultAttr || beg->value != defaultAttr;
+
+            if (trimTrailingWhitespaces)
+            {
+                newX = lastCharX;
             }
 
             buffer.append(row.GetText(oldX, newX));
+
+            if (clearToEndOfLine)
+            {
+                buffer.append(L"\x1b[K");
+            }
+
             oldX = newX;
         }
 
         const auto moreRowsRemaining = currentRow < lastRowWithText;
+        delayedLineBreak = !row.WasWrapForced();
 
-        if (!row.WasWrapForced() || !moreRowsRemaining)
+        if (!moreRowsRemaining)
         {
-            buffer.append(L"\r\n");
+            if (previousHyperlinkId)
+            {
+                buffer.append(L"\x1b]8;;\x1b\\");
+            }
+            buffer.append(L"\x1b[m\r\n");
         }
 
         if (buffer.size() >= writeThreshold || !moreRowsRemaining)