Repository: microsoft/terminal (language: cpp)

Issue:
ICH no longer works correctly on lines containing wide characters
### Windows Terminal version

Commit 547349af77df16d0eed1c73ba3041c84f7b063da

### Windows build number

10.0.19044.2364

### Other Software

_No response_

### Steps to reproduce

1. Create a build from the latest commit.
2. Start Windows Terminal and open a WSL bash shell.
3. Execute the following command: `printf "\u3053\u3093\u306B\u3061\u306F World\r\e[@\n"`

### Expected Behavior

This should write out the text `こんにちは World`, and then insert a space at the start of the line via an `ICH` sequence. This is what it looks like in Preview version 1.16.2641.0:

![image](https://user-images.githubusercontent.com/4181424/210441754-ddf715d8-b0cf-402e-a725-a859152815b5.png)

### Actual Behavior

When using the latest commit, the Japanese characters are erased by the `ICH` sequence, so all that's left is `World`.

![image](https://user-images.githubusercontent.com/4181424/210441788-30f386eb-2593-4fce-afc1-05b4c4894315.png)

So this looks to me like a regression, although I'm not sure when exactly it stopped working.

And note that this fails in OpenConsole as well. It's just easier to confirm that it's a recent regression in Windows Terminals because I know 1.16 was working OK.


Discussion / hints:
As far as I can tell, the regression came from the new `ROW` code in a01500f051bc896465ac4bd37811142bc2347fb8.

The other thing I've noticed is that it only appears to be a problem when inserting a single space. An `ICH` with a parameter of 2 or more seems to work fine. I wonder if the problem is that it's trying to copy a 2-cell character on top of itself when moving one cell to the right, and in the process it destroys the source data before it can finish writing it to the target.
Thanks for digging into this @j4james! Leonard is out for a couple more weeks -- is this an area you'd be comfortable poking at in advance of the 1.17 release?
The buffer code is a bit of a black box to me, so I don't want to promise anything, but I can take a look and see if there's an easy fix. I suspect this is out of my league though.
First off, a few more test cases.

1. Deleting a character with `DCH`. This hits the same code path as `ICH` (the [`_ScrollRectHorizontally`](https://github.com/microsoft/terminal/blob/4d27a05318f4e86b0918497ba6f980968725a1d2/src/terminal/adapter/adaptDispatch.cpp#L469-L474) method in `AdaptDispatch`), but this shows it fails when scrolling in both directions. 

        printf " \u3053\u3093\u306B\u3061\u306F World\r\e[P\n"

2. Copying the screen one column to the right with `DECCRA`. This goes through the [`CopyRectangularArea`](https://github.com/microsoft/terminal/blob/4d27a05318f4e86b0918497ba6f980968725a1d2/src/terminal/adapter/adaptDispatch.cpp#L1020-L1030) method of `AdaptDispatch`, but it's a similar pattern of code to `_ScrollRectHorizontally`.

        printf "\u3053\u3093\u306B\u3061\u306F World\n\e[;;;;;;2\$v"

3. Scrolling the screen one column to the right with the `ScrollConsoleScreenBuffer` API. This goes through conhost's [`_CopyRectangle`](https://github.com/microsoft/terminal/blob/165d3edde9ca850f25f7c1e740b5661461fcf175/src/host/output.cpp#L105-L111) function, which again is using a similar pattern of code.

       SetConsoleOutputCP(65001);
       std::cout << "こんにちは World\n";
       HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE);
       SMALL_RECT scrollRect = { 0, 0, 32767, 32767 };
       CHAR_INFO fill = {L' '};
       ScrollConsoleScreenBuffer(h, &scrollRect, &scrollRect, {1,0}, &fill);

In all these cases, what we're doing is iterating over a range of cells in the buffer, and copying them from one location to another like this:

```cpp
do
{
    const auto data = OutputCell(*textBuffer.GetCellDataAt(sourcePos));
    textBuffer.Write(OutputCellIterator({ &data, 1 }), targetPos);
    source.WalkInBounds(sourcePos, walkDirection);
} while (target.WalkInBounds(targetPos, walkDirection));
```

But when the source and target are only one cell apart, and the characters we're manipulating are occupying more than one cell, we can end up erasing a character before we've finished copying it.

Let's say we've got a DBCS character in columns 1 and 2, which we're copying one column to the right, so we start by writing the trailing byte to column 3. However, because this is a two-cell character, the code immediately replaces both columns 2 and 3. But this leaves column 1 in a broken state (it's now half a DBCS character), so that gets erased as well.

Now we come to copying what would have been the leading byte from column 1 to column 2, but it's column 1 is now a blank. And once we write a blank into column 2, that leaves column 3 in a broken state, so again that needs to be erased, and everything is blank. At least I that's my understanding of what's going on.

I suspect the erasure of half-DBCS characters is a new thing that was added in the `ROW` update, and that's possibly why we didn't have this issue before. But I don't want to change that - I'm fairly certain that is the right thing to do. We just need to update the various scrolling operations to take that into account, and I think I've got a simple way we can achieve that.

All we need to do is read two cells from the source, before writing to the target, so the code snippet from above would change to something like this:

```cpp
auto next = OutputCell(*textBuffer.GetCellDataAt(sourcePos));
do
{
    const auto current = next;
    source.WalkInBounds(sourcePos, walkDirection);
    next = OutputCell(*textBuffer.GetCellDataAt(sourcePos));
    textBuffer.WriteLine(OutputCellIterator({ &current, 1 }), targetPos);
} while (target.WalkInBounds(targetPos, walkDirection));
```

Now I realise this is not a long term fix - this will assumedly not work once we have characters that can span more than two cells. But it does at least gets things working again without requiring massive changes to the code. And once Leonard is back he can come up with a proper solution.

Fix patch:
diff --git a/src/host/output.cpp b/src/host/output.cpp
index bec18730d6e..79e3c8dea72 100644
--- a/src/host/output.cpp
+++ b/src/host/output.cpp
@@ -102,12 +102,16 @@ static void _CopyRectangle(SCREEN_INFORMATION& screenInfo,
         auto sourcePos = source.GetWalkOrigin(walkDirection);
         auto targetPos = target.GetWalkOrigin(walkDirection);
 
+        // Note that we read two cells from the source before we start writing
+        // to the target, so a two-cell DBCS character can't accidentally delete
+        // itself when moving one cell horizontally.
+        auto next = OutputCell(*screenInfo.GetCellDataAt(sourcePos));
         do
         {
-            const auto data = OutputCell(*screenInfo.GetCellDataAt(sourcePos));
-            screenInfo.Write(OutputCellIterator({ &data, 1 }), targetPos);
-
+            const auto current = next;
             source.WalkInBounds(sourcePos, walkDirection);
+            next = OutputCell(*screenInfo.GetCellDataAt(sourcePos));
+            screenInfo.GetTextBuffer().WriteLine(OutputCellIterator({ &current, 1 }), targetPos);
         } while (target.WalkInBounds(targetPos, walkDirection));
     }
 }
diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp
index 0c1d848d3ad..db6e305b841 100644
--- a/src/host/ut_host/ScreenBufferTests.cpp
+++ b/src/host/ut_host/ScreenBufferTests.cpp
@@ -12,6 +12,7 @@
 #include "input.h"
 #include "getset.h"
 #include "_stream.h" // For WriteCharsLegacy
+#include "output.h" // For ScrollRegion
 
 #include "../interactivity/inc/ServiceLocator.hpp"
 #include "../../inc/conattrs.hpp"
@@ -183,6 +184,7 @@ class ScreenBufferTests
     TEST_METHOD(ScrollOperations);
     TEST_METHOD(InsertChars);
     TEST_METHOD(DeleteChars);
+    TEST_METHOD(ScrollingWideCharsHorizontally);
 
     TEST_METHOD(EraseScrollbackTests);
     TEST_METHOD(EraseTests);
@@ -4088,6 +4090,54 @@ void ScreenBufferTests::DeleteChars()
                    L"A whole line of spaces was inserted from the right, erasing the line.");
 }
 
+void ScreenBufferTests::ScrollingWideCharsHorizontally()
+{
+    // The point of this test is to make sure wide characters can be
+    // moved horizontally by one cell without erasing themselves.
+
+    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
+    auto& stateMachine = si.GetStateMachine();
+    WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
+
+    const auto viewport = si.GetViewport();
+    const auto testRow = viewport.Top();
+
+    Log::Comment(L"Fill the test row with content containing wide chars");
+    const auto testChars = L"こんにちは World";
+    const auto testAttr = TextAttribute{ FOREGROUND_RED | BACKGROUND_BLUE };
+    _FillLine(testRow, testChars, testAttr);
+
+    Log::Comment(L"Position the cursor at the start of the test row");
+    VERIFY_SUCCEEDED(si.SetCursorPosition({ 0, testRow }, true));
+
+    Log::Comment(L"Insert 1 cell at the start of the test row");
+    stateMachine.ProcessString(L"\033[@");
+    VERIFY_IS_TRUE(_ValidateLineContains({ 1, testRow }, testChars, testAttr));
+
+    Log::Comment(L"Delete 1 cell from the start of the test row");
+    stateMachine.ProcessString(L"\033[P");
+    VERIFY_IS_TRUE(_ValidateLineContains(testRow, testChars, testAttr));
+
+    Log::Comment(L"Copy the test row 1 cell to the right");
+    stateMachine.ProcessString(L"\033[1;1;1;;;1;2$v");
+    VERIFY_IS_TRUE(_ValidateLineContains({ 1, testRow }, testChars, testAttr));
+
+    Log::Comment(L"Copy the test row 1 cell to the left");
+    stateMachine.ProcessString(L"\033[1;2;1;;;1;1$v");
+    VERIFY_IS_TRUE(_ValidateLineContains(testRow, testChars, testAttr));
+
+    Log::Comment(L"Scroll the test row 1 cell to the right");
+    const auto testRect = til::inclusive_rect{ 0, testRow, viewport.Width() - 2, testRow };
+    ScrollRegion(si, testRect, std::nullopt, { 1, testRow }, L' ', testAttr);
+    VERIFY_IS_TRUE(_ValidateLineContains({ 1, testRow }, testChars, testAttr));
+
+    Log::Comment(L"Scroll the test row 1 cell to the left");
+    const auto testRect2 = til::inclusive_rect{ 1, testRow, viewport.Width() - 1, testRow };
+    ScrollRegion(si, testRect2, std::nullopt, { 0, testRow }, L' ', testAttr);
+    VERIFY_IS_TRUE(_ValidateLineContains(testRow, testChars, testAttr));
+}
+
 void ScreenBufferTests::EraseScrollbackTests()
 {
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index 4c9d60df4aa..8bb756ccb72 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -466,11 +466,16 @@ void AdaptDispatch::_ScrollRectHorizontally(TextBuffer& textBuffer, const til::r
         const auto walkDirection = Viewport::DetermineWalkDirection(source, target);
         auto sourcePos = source.GetWalkOrigin(walkDirection);
         auto targetPos = target.GetWalkOrigin(walkDirection);
+        // Note that we read two cells from the source before we start writing
+        // to the target, so a two-cell DBCS character can't accidentally delete
+        // itself when moving one cell horizontally.
+        auto next = OutputCell(*textBuffer.GetCellDataAt(sourcePos));
         do
         {
-            const auto data = OutputCell(*textBuffer.GetCellDataAt(sourcePos));
-            textBuffer.Write(OutputCellIterator({ &data, 1 }), targetPos);
+            const auto current = next;
             source.WalkInBounds(sourcePos, walkDirection);
+            next = OutputCell(*textBuffer.GetCellDataAt(sourcePos));
+            textBuffer.WriteLine(OutputCellIterator({ &current, 1 }), targetPos);
         } while (target.WalkInBounds(targetPos, walkDirection));
     }
 
@@ -1017,16 +1022,21 @@ bool AdaptDispatch::CopyRectangularArea(const VTInt top, const VTInt left, const
         const auto walkDirection = Viewport::DetermineWalkDirection(srcView, dstView);
         auto srcPos = srcView.GetWalkOrigin(walkDirection);
         auto dstPos = dstView.GetWalkOrigin(walkDirection);
+        // Note that we read two cells from the source before we start writing
+        // to the target, so a two-cell DBCS character can't accidentally delete
+        // itself when moving one cell horizontally.
+        auto next = OutputCell(*textBuffer.GetCellDataAt(srcPos));
         do
         {
+            const auto current = next;
+            srcView.WalkInBounds(srcPos, walkDirection);
+            next = OutputCell(*textBuffer.GetCellDataAt(srcPos));
             // If the source position is offscreen (which can occur on double
             // width lines), then we shouldn't copy anything to the destination.
             if (srcPos.x < textBuffer.GetLineWidth(srcPos.y))
             {
-                const auto data = OutputCell(*textBuffer.GetCellDataAt(srcPos));
-                textBuffer.Write(OutputCellIterator({ &data, 1 }), dstPos);
+                textBuffer.WriteLine(OutputCellIterator({ &current, 1 }), dstPos);
             }
-            srcView.WalkInBounds(srcPos, walkDirection);
         } while (dstView.WalkInBounds(dstPos, walkDirection));
         _api.NotifyAccessibilityChange(dstRect);
     }


Test patch: