Repository: microsoft/terminal (language: cpp)

Issue:
Backspace in wrapped cooked read erases char on line above
### Windows Terminal version

Commit ac5b4f5831c125e7defe65eab93d3928b282559f

### Windows build number

10.0.19045.4529

### Other Software

_No response_

### Steps to reproduce

1. Checkout the latest code from main (ac5b4f5831c125e7defe65eab93d3928b282559f or later).
2. Start a cmd shell in OpenConsole.
3. Type some content at the prompt until it wraps onto the second line.
4. Backspace until the cursor is at the start of that second line.


### Expected Behavior

Backspaces on the second line should have no effect on the characters in the first line.


### Actual Behavior

The last character on the first line gets erased when backspacing over the first character in the second line.

This is what it looks like after wrapping.
![image](https://github.com/user-attachments/assets/c31b06fa-52c1-4a74-b4ab-7eda11111eae)

And this is what it looks like after two backspaces. Notice the `r` erased from the first line.
![image](https://github.com/user-attachments/assets/e6e440b0-0460-452e-92f0-cc14cadce133)

The same thing occurs in both OpenConsole and Windows Terminal. I think this was introduced by the new VT cooked read implementation (PR #17445).



Discussion / hints:
I should note that this is just a visual glitch. The actual prompt content is correct. It just looks like a character is missing.
Your issue occurs because `if (pagerPromptEnd.x >= size.width)` must be after `if (pagerPromptEnd.y <= _pagerPromptEnd.y)` and not before it (I think).

I found two more bugs:
* Writing a wide glyph into the last column breaks the cursor position.
  Fixed by moving `dist += state.len;` in `TextBuffer::FitTextIntoColumns` to the end of the loop.
* Having a fairly full prompt (e.g. 80% of the viewport height) and pressing F7 with a rather full history will move the prompt contents upwards. But when dismissing F7 via ESC it'll not restore the prompt to its previous state.

Fix patch:
diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
index 4ee701982eb..553365e4f5e 100644
--- a/src/buffer/out/textBuffer.cpp
+++ b/src/buffer/out/textBuffer.cpp
@@ -445,7 +445,6 @@ size_t TextBuffer::FitTextIntoColumns(const std::wstring_view& chars, til::Coord
     while (dist < len)
     {
         cwd.GraphemeNext(state, chars);
-        dist += state.len;
         col += state.width;
 
         // If we ran out of columns, we need to always return `columnLimit` and not `cols`,
@@ -457,6 +456,8 @@ size_t TextBuffer::FitTextIntoColumns(const std::wstring_view& chars, til::Coord
             columns = columnLimit;
             return dist;
         }
+
+        dist += state.len;
     }
 
     // But if we simply ran out of text we just need to return the actual number of columns.
diff --git a/src/host/readDataCooked.cpp b/src/host/readDataCooked.cpp
index 2fc30f76ec9..f67b10c3302 100644
--- a/src/host/readDataCooked.cpp
+++ b/src/host/readDataCooked.cpp
@@ -940,11 +940,6 @@ void COOKED_READ_DATA::_redisplay()
         }
 
         pagerPromptEnd = { res.column, gsl::narrow_cast<til::CoordType>(lines.size() - 1) };
-        if (pagerPromptEnd.x >= size.width)
-        {
-            pagerPromptEnd.x = 0;
-            pagerPromptEnd.y++;
-        }
 
         // If the content got a little shorter than it was before, we need to erase the tail end.
         // If the last character on a line got removed, we'll skip this code because `remaining`
@@ -960,12 +955,14 @@ void COOKED_READ_DATA::_redisplay()
                 auto& line = lines.back();
 
                 // CSI K may be expensive, so use spaces if we can.
-                if (remaining <= 8)
+                if (remaining <= 16)
                 {
                     line.text.append(remaining, L' ');
+                    line.columns += remaining;
                 }
                 else
                 {
+                    // CSI K doesn't change the cursor position, so we don't modify .columns.
                     line.text.append(L"\x1b[K");
                 }
             }
@@ -1015,12 +1012,34 @@ void COOKED_READ_DATA::_redisplay()
             // If the cursor is at the end of the buffer we must always show it after the last character.
             // Since VT uses delayed EOL wrapping, we must write at least 1 more character to force the
             // potential delayed line wrap at the end of the prompt, on the last line.
-            // This doubles as the code that erases the last character on the last line when backspacing.
-            // That's also why we append 2 spaces, because the last character may have been a ^E control
-            // character visualizer, which sneakily actually consists of 2 characters.
+            // We append an extra line to get the lineCount for scrolling right.
             if (_bufferCursor == _buffer.size())
             {
-                lines.emplace_back(L"  \r", 0, 0, 0);
+                auto& line = lines.emplace_back();
+
+                // This mirrors the `if (pagerPromptEnd.y <= _pagerPromptEnd.y)` above. We need to repeat this here,
+                // because if we append another line then we also need to repeat the "delete to end" logic.
+                // The best way to see this code kick in is if you have a prompt like this:
+                //   +----------+
+                //   |C:\> foo  | <-- end the line in >=1 spaces
+                //   |bar_      | <-- start the line with a word >2 characters
+                //   +----------+
+                // Then put the cursor at the end (where the "_" is) and press Ctrl+Backspace.
+                auto remaining = (_pagerPromptEnd.y - pagerPromptEnd.y) * size.width + _pagerPromptEnd.x - pagerPromptEnd.x;
+
+                // Here we ensure that we force a EOL wrap no matter what. At a minimum this will result in " \r".
+                remaining = std::max(1, remaining);
+
+                // CSI K may be expensive, so use spaces if we can.
+                if (remaining <= 16)
+                {
+                    line.text.append(remaining, L' ');
+                    line.text.push_back(L'\r');
+                }
+                else
+                {
+                    line.text.append(L" \r\x1b[K");
+                }
             }
         }
 
@@ -1073,20 +1092,45 @@ void COOKED_READ_DATA::_redisplay()
 
     // If we have so much text that it doesn't fit into the viewport (origin == {0,0}),
     // then we can scroll the existing contents of the pager and only write what got newly uncovered.
-    if (const auto delta = pagerContentTop - _pagerContentTop; delta != 0 && _pagerHeight == size.height && pagerHeight == size.height)
+    //
+    // The check for origin == {0,0} is important because it ensures that we "own" the entire viewport and
+    // that scrolling our contents doesn't scroll away the user's output that may still be in the viewport.
+    // (Anything below the origin is assumed to belong to us.)
+    if (const auto delta = pagerContentTop - _pagerContentTop; delta != 0 && _originInViewport == til::point{})
     {
         const auto deltaAbs = abs(delta);
         til::CoordType beg = 0;
         til::CoordType end = pagerHeight;
 
-        // If the top changed by more than the viewport height, scrolling doesn't make sense.
-        if (deltaAbs < size.height)
+        // Let's say the viewport is 10 lines tall. Scenarios:
+        // * We had 2 lines (_pagerContentTop == 0, _pagerHeight == 2),
+        //   and now it's 11 lines (pagerContentTop == 1, pagerHeight == 11).
+        //   --> deltaAbs == 1
+        //   --> Scroll ✔️
+        // * We had 2 lines (_pagerContentTop == 0, _pagerHeight == 2),
+        //   and now it's 12 lines (pagerContentTop == 2, pagerHeight == 12).
+        //   --> deltaAbs == 2
+        //   --> Scroll ❌
+        //
+        // The same applies when going from 11/12 lines back to 2. It appears scrolling
+        // makes sense if the delta is smaller than the current or previous pagerHeight.
+        if (deltaAbs < std::min(_pagerHeight, pagerHeight))
         {
             beg = delta >= 0 ? pagerHeight - deltaAbs : 0;
             end = delta >= 0 ? pagerHeight : deltaAbs;
             const auto cmd = delta >= 0 ? L'S' : L'T';
             fmt::format_to(std::back_inserter(output), FMT_COMPILE(L"\x1b[{}{}"), deltaAbs, cmd);
         }
+        else
+        {
+            // We may not be scrolling with VT, because we're scrolling by more rows than the pagerHeight.
+            // Since no one is now clearing the scrolled in rows for us anymore, we need to do it ourselves.
+            auto& lastLine = lines.at(pagerHeight - 1 + pagerContentTop);
+            if (lastLine.columns < size.width)
+            {
+                lastLine.text.append(L"\x1b[K");
+            }
+        }
 
         // Mark each row that has been uncovered by the scroll as dirty.
         for (auto i = beg; i < end; i++)
@@ -1134,7 +1178,10 @@ void COOKED_READ_DATA::_redisplay()
             const auto& line = lines.at(i + pagerContentTop);
 
             // Skip lines that aren't marked as dirty.
-            if (line.dirtyBegOffset >= line.text.size())
+            // We use dirtyBegColumn instead of dirtyBegOffset to test for dirtiness, because a line that has 1 column
+            // of space for layout and was asked to fit a wide glyph will have no text, but still be "dirty".
+            // This ensures that we get the initial starting position of the _appendCUP below right.
+            if (line.dirtyBegColumn >= size.width)
             {
                 continue;
             }
@@ -1212,6 +1259,7 @@ COOKED_READ_DATA::LayoutResult COOKED_READ_DATA::_layoutLine(std::wstring& outpu
         {
             if (column >= columnLimit)
             {
+                column = columnLimit;
                 goto outerLoopExit;
             }
 
@@ -1243,12 +1291,6 @@ COOKED_READ_DATA::LayoutResult COOKED_READ_DATA::_layoutLine(std::wstring& outpu
     }
 
 outerLoopExit:
-    if (it != end && column < columnLimit)
-    {
-        output.append(columnLimit - column, L' ');
-        column = columnLimit;
-    }
-
     return {
         .offset = static_cast<size_t>(it - beg),
         .column = column,
diff --git a/src/host/readDataCooked.hpp b/src/host/readDataCooked.hpp
index 22da5334276..e6f35397125 100644
--- a/src/host/readDataCooked.hpp
+++ b/src/host/readDataCooked.hpp
@@ -168,6 +168,7 @@ class COOKED_READ_DATA final : public ReadData
     til::point _originInViewport;
     // This value is in the pager coordinate space. (0,0) is the first character of the
     // first line, independent on where the prompt actually appears on the screen.
+    // The coordinate is "end exclusive", so the last character is 1 in front of it.
     til::point _pagerPromptEnd;
     // The scroll position of the pager.
     til::CoordType _pagerContentTop = 0;


Test patch: