Repository: microsoft/terminal (language: cpp)

Issue:
VT renderer works incorrectly when cursor is moved between writes
### Windows Terminal version

Latest source

### Windows build number

10.0.19045.4355

### Other Software

No

### Steps to reproduce

The original issue is described here: https://github.com/FarGroup/FarManager/issues/841
According to the comments, it regressed somewhere between 1.18.3181.0 - 1.19.11213.0

MRE: [VtRendererBug.zip](https://github.com/microsoft/terminal/files/15314733/VtRendererBug.zip)
STR: compile, run, follow instructions


### Expected Behavior

The vertical bar moves right without affecting the underlying text.

It works as expected in OpenConsole.

### Actual Behavior

The vertical bar and the underlying text are broken.
The third line is written into the second.

It doesn't work as expected in WT and other terminals that use OpenConsole, e.g. WezTerm.

---

I've managed to trace it down to https://github.com/microsoft/terminal/blob/f62d2d5d2c7b4188daf1d46f48faacf5ee2c717d/src/renderer/vt/XtermEngine.cpp#L281-L282

It looks like specifying the position explicitly fixes the issue:
```DIFF
-                std::string seq = "\r\n";
-                hr = _Write(seq);
+                hr = _CursorPosition(coord);
```
Which could mean that the cursor position in `_lastText` is tracked incorrectly / unreliably.


Discussion / hints:
Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!


### Closed similar issues:
- [Issues with cursor invalidation in GDI (#17150)](https://github.com/microsoft/terminal/issues/17150),  similarity score: 0.75

> Note: You can give me feedback by thumbs upping or thumbs downing this comment.
> MRE: [VtRendererBug.zip](https://github.com/microsoft/terminal/files/15314733/VtRendererBug.zip)
> STR: compile, run, follow instructions

P.S. I *think* the provided example reproduces the same issue.
Just in case if not, STR for the original one:
1. Get Far - https://github.com/FarGroup/FarManager/releases/
2. Run it.
3. In F9 - Options - Interface settings
   uncheck:
     `[ ] Use Virtual Terminal for rendering`
     `[ ] Fullwidth-aware rendering`
   check:
     `[x] ClearType-friendly redraw (can be slow)`
4. Edit any file (F4)
5. In F11 - FarColorer - Configure - Main settings
   check:
     `[x] Cross`
   set `Cross style` to `both`
6. Press `CtrlHome` to move the cursor to the upper left corner and then `Right` a few times.
We've talked internally about reverting #15500 for the 1.21 release. It's the source for all these issues. While the fix you found may fix this particular issue, it's not unlikely that there will be more.

On the current main branch, which targets version 1.22, we're thinking of potentially removing VtEngine in its entirety. This would then also fix the cursor invalidation issue introduced in #15500. Unfortunately, this means that this issue would intentionally exist on main for up to a couple months.
FYI, I did a git bisect with the VtRendererBug test case, and for me the issue first showed up in commit 72b44888b5487a41cf2f030de86daea34b30e9c0. However, I suspect it may be one of those things that's timing dependent, and that commit just exposed a bug that has always existed.
I've just confirmed that reverting commit 72b44888b5487a41cf2f030de86daea34b30e9c0 on the main branch is enough to fix the issue for me (i.e. moving the `WaitUntilCanRender` call back down to the bottom of the loop, before `PaintFrame`). And I've also now tested the cursor movement in Far itself, and the same fix works there too.

Again I'm not sure that's a real solution to the problem, but it might be worth considering as a quick fix, assuming it works for everyone else and not just me.
Thanks James, reverting 72b44888b5487a41cf2f030de86daea34b30e9c0 fixed it for me as well.
As for what's going wrong, this is what I've been able to establish:

1. When you write a line of text that extends to the last column, we set the `_wrapForced` flag, even though the line hasn't actually wrapped (this is partially tracked in #15602, although that's referencing the `AdaptDispatch` class, while this particular case is in one of the console APIs).

2. When the VtEngine outputs that line, and it sees that flag is set, it records the apparent wrapped state in the `_wrappedRow` field, again despite the fact that it hasn't actually wrapped.

3. At this point in time, the cursor is meant to be at the start of line 2, because that's where it was manually positioned, and the `WriteConsoleOutput` API doesn't change that. So at the end of the frame, the VtEngine tries to move the cursor there. However, because of `_wrappedRow` being set to the line above, it thinks there is nothing to do (the `previousLineWrapped` test [here](https://github.com/microsoft/terminal/blob/183a8956f699e1967d4cb4712184d0dbc395a0e5/src/renderer/vt/XtermEngine.cpp#L265-L278)).

4. Even though the `_MoveCursor` method didn't do anything in the previous step, it still updates the `_lastText` field to where it mistakenly thought the cursor was (it thinks it's at the start of line 2, when it's actually at the end of line 1). This is why the subsequent output ends up the wrong location.

Ignoring bug number 1 for the moment, it seems to me that point 3 is also a bug, because the `_wrappedRow` field being set does not actually indicate that the cursor is on the next line, only that it doesn't need to be moved there if you're about to write a character. If you're just painting the cursor at that point, it needs to be moved explicitly.

So another possible way to fix this issue would be to set `_wrappedRow = std::nullopt` in the `VtEngine::PaintCursor` method before calling `_MoveCursor`.

That said, the conpty code is complicated, and I really don't know it very well, so there may be more to it than that. I'm just throwing this out there as potential solution.
Another simple test case that can be run from a WSL bash shell:
```
printf '\033[2J\033[1;999H*'; sleep 1; printf '\033[2H\033[65;1;1;1;999$x'; sleep 1; printf '\033[3HThis should be line 3\n'
```

The text "This should be line 3", should be on line 3 😄, but it usually ends up on line 2.

This one is not fixed by reverting 72b44888b5487a41cf2f030de86daea34b30e9c0, but it is fixed by the `_wrappedRow` patch suggested above.

 
Whichever the fix ends up being, I'm going to defer it until the next servicing wave for 1.21 - thanks for all the great investigations, everyone 😄 

Fix patch:
diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 2dc64562e88..42099a24e56 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -3542,7 +3542,18 @@ void ConptyRoundtripTests::WrapNewLineAtBottomLikeMSYS()
             const auto actualNonSpacesAttrs = defaultAttrs;
             const auto actualSpacesAttrs = rowCircled || isTerminal ? defaultAttrs : conhostDefaultAttrs;
 
-            VERIFY_ARE_EQUAL(isWrapped, tb.GetRowByOffset(row).WasWrapForced());
+            // When using WriteCharsLegacy to emit a wrapped line, with the
+            // frame painted before the second half of the wrapped line, the
+            // cursor needs to be manually moved to the second line, because
+            // that's what is expected of WriteCharsLegacy, and the terminal
+            // would otherwise delay that movement. But this means the line
+            // won't be marked as wrapped, and there's no easy way to fix that.
+            // For now we're just skipping this test.
+            if (!(writingMethod == PrintWithWriteCharsLegacy && paintEachNewline == PaintEveryLine && isWrapped))
+            {
+                VERIFY_ARE_EQUAL(isWrapped, tb.GetRowByOffset(row).WasWrapForced());
+            }
+
             if (isWrapped)
             {
                 TestUtils::VerifyExpectedString(tb, std::wstring(charsInFirstLine, L'~'), { 0, row });
diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp
index 1ebff63cac2..68ea772e4b3 100644
--- a/src/host/ut_host/ConptyOutputTests.cpp
+++ b/src/host/ut_host/ConptyOutputTests.cpp
@@ -122,6 +122,7 @@ class ConptyOutputTests
     TEST_METHOD(InvalidateUntilOneBeforeEnd);
     TEST_METHOD(SetConsoleTitleWithControlChars);
     TEST_METHOD(IncludeBackgroundColorChangesInFirstFrame);
+    TEST_METHOD(MoveCursorAfterWrapForced);
 
 private:
     bool _writeCallback(const char* const pch, const size_t cch);
@@ -428,3 +429,39 @@ void ConptyOutputTests::IncludeBackgroundColorChangesInFirstFrame()
 
     VERIFY_SUCCEEDED(renderer.PaintFrame());
 }
+
+void ConptyOutputTests::MoveCursorAfterWrapForced()
+{
+    auto& g = ServiceLocator::LocateGlobals();
+    auto& renderer = *g.pRender;
+    auto& gci = g.getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer();
+    auto& sm = si.GetStateMachine();
+
+    // We write a character in the rightmost column to trigger the _wrapForced
+    // flag. Technically this is a bug, but it's how things currently work.
+    sm.ProcessString(L"\x1b[1;999H*");
+
+    expectedOutput.push_back("\x1b[2J"); // standard init sequence for the first frame
+    expectedOutput.push_back("\x1b[m"); // standard init sequence for the first frame
+    expectedOutput.push_back("\x1b[1;80H");
+    expectedOutput.push_back("*");
+    expectedOutput.push_back("\x1b[?25h");
+
+    VERIFY_SUCCEEDED(renderer.PaintFrame());
+
+    // Position the cursor on line 2, and fill line 1 with A's.
+    sm.ProcessString(L"\x1b[2H");
+    sm.ProcessString(L"\033[65;1;1;1;999$x");
+
+    expectedOutput.push_back("\x1b[H");
+    expectedOutput.push_back("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
+
+    // The cursor must be explicitly moved to line 2 at the end of the frame.
+    // Although that may technically already be the next output location, we
+    // still need the cursor to be shown in that position when the frame ends.
+    expectedOutput.push_back("\r\n");
+    expectedOutput.push_back("\x1b[?25h");
+
+    VERIFY_SUCCEEDED(renderer.PaintFrame());
+}
diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp
index 1fcb5335887..69985b27eec 100644
--- a/src/renderer/vt/paint.cpp
+++ b/src/renderer/vt/paint.cpp
@@ -224,6 +224,15 @@ using namespace Microsoft::Console::Types;
 {
     _trace.TracePaintCursor(options.coordCursor);
 
+    // GH#17270: If the wrappedRow field is set, and the target cursor position
+    // is at the start of the next row, it's expected that any subsequent output
+    // would already be written to that location, so the _MoveCursor method may
+    // decide it doesn't need to do anything. In this case, though, we're not
+    // writing anything else, so the cursor will end up in the wrong location at
+    // the end of the frame. Clearing the wrappedRow field fixes that.
+    _wrappedRow = std::nullopt;
+    _trace.TraceClearWrapped();
+
     // MSFT:15933349 - Send the terminal the updated cursor information, if it's changed.
     LOG_IF_FAILED(_MoveCursor(options.coordCursor));
 


Test patch: