Repository: microsoft/terminal (language: cpp)

Issue:
[1.15+] Flushing FTCS sequences causes flickering in nushell 0.66+
### Windows Terminal version

1.15.2003.0

### Windows build number

Microsoft Windows [Version 10.0.22000.795]

### Other Software

Nushell v0.66+

### Steps to reproduce

Hi, I work on a cross-platform shell called [Nushell](https://github.com/nushell/nushell) and [we've noticed a rendering regression in Windows Terminal Preview v1.15](https://github.com/nushell/nushell/issues/6214).

Nushell's prompt now flickers on every keystroke. The following recording illustrates the problem (but it's a little worse in real life, the recording didn't capture every flicker):

https://user-images.githubusercontent.com/26268125/183734387-10477dd6-220c-4f7d-8b21-f7737736d0ee.mp4

To reproduce this, run any version of Nushell from v0.66 or later and start typing. Nushell can be downloaded from [our GitHub releases](https://github.com/nushell/nushell/releases/tag/0.66.2) or installed with Scoop (`scoop install nushell`) or winget (`winget install nushell`). `nu.exe` is the executable for Nushell.

## Details / Investigation

The flickering does _not_ occur in Windows Terminal v1.14.1963.0.

We bisected Nushell's code and found that the flickering did not occur prior to [this PR](https://github.com/nushell/nushell/pull/6087) which added some scroll mark ANSI escape codes for VS Code. WT v1.15 Preview [added experimental support for scroll marks](https://devblogs.microsoft.com/commandline/windows-terminal-preview-1-15-release/#scroll-marks-experimental); maybe this is a bug in the experimental scroll mark code?

The flickering occurs even with the scroll mark settings (`experimental.autoMarkPrompts` and `experimental.showMarksOnScrollbar`) set to false.

If there's anything else we can do to help with investigation or reproduction, just let me know!


Discussion / hints:
_(I'm supposed to be on vacation RN so apologies for the brevity in my notes)_

Hmmmmmmmmm. I wonder if this isn't playing nicely with the frame flushing. Like, when the prompt gets drawn, what's happening is a 
* Move the cursor to the start of the prompt 
* erase the rest of the line
* mark here as the start of the prompt (FLUSHES THE FRAME)
* draw the prompt
* mark here as the end of the prompt (flushes maybe? maybe not yet)
* draw the right side of the status

The trick is that we're flushing all buffered state to the Terminal when we see a prompt sequence. The problem is that at the time the mark is re-rendered, the prompt line is empty, causing the flickering. 

No clever solutions off the top of the dome. I'll noodle on the drive home. 


<hr>

### Edit 2023/01/10: The above set of steps is wrong. Don't use that as a guide for what's happening.
@zadjii-msft for what it's worth, we're tracking a related issue with WezTerm. I say they're related because disabling the `OSC 133` (and friends) escape sequences seem to correct the problem but, of course, we lose the features that `OSC 133` provides. @wez, the author, has a theory here https://github.com/nushell/nushell/issues/5585#issuecomment-1230417496.

Quoting here
> If I edit the macos capture and remove the OSC 133 A and OSC 133 B sequences, and replay that file on windows, then the output looks correct. Those are the FinalTerm style shell integration escape sequences.
>
> It looks to me as though this is a ConPTY issue where unexpected/unknown OSC sequences confuse it about cursor positioning or some other similar concern that triggers it to rewrite the output stream.
> 
> A workaround would be to skip outputting OSC 133 on Windows or if you know that ConPTY is present.

You can see more of the detailed research here https://github.com/nushell/nushell/issues/5585
Ah, that's probably another related issue. Before a few months ago, conpty didn't recognize those sequences at all, and would emit them immediately, possible before the rest of the "frame" they were emitted with. at the end of the day, it just means that FTCS sequence support on older Windows builds is gonna be _wacky_. 

Now, the Terminal uses an out-of-band version of the ConPTY apis. We do that so we can always use the `main` version of ConPTY in the Terminal, rather than the OS one (which might be months or years out of date). So the issue in the Terminal version of ConPTY is caused by me trying to fix the issue in the OS version of ConPTY (used by wezterm et. al). 

If that all makes sense. 
>  it just means that FTCS sequence support on older Windows builds is gonna be wacky.

It all makes sense except this was on an updated copy of Windows 11. So, I expect old versions to be _wacky_ but hopefully recent Win10 and Win11 will behave better in the near future.
> an updated copy of Windows 11

I mean, FTCS support landed literally 3 months ago - I'm not sure it's even made its way to _any_ official Windows builds yet 😅 
FWIW, wezterm bundles and uses openconsole.exe and conpty.dll built from GitHub.  I just updated it to the most recent tag this morning and the issue reproduces there too.

https://github.com/wez/wezterm/commit/0d85d05442a2ca38264a2cd3a21f57d565a4464f 
Any update on this issue @zadjii-msft?
Alas, no. I haven't noodled on this too much since getting back from leave, and I'm only just getting back into the swing of things after the holidays. 

I've got this marked up to take a look at ~this month, but I seriously doubt I'll have a good solution that fast. 

IIRC we briefly discussed some ideas, that might work _okay_ in this specific scenario, but wouldn't work more broadly for the other sequences that are leveraging the same flushing that we do here (looking at you _alt buffer sequences_). I can't remember the details though 😕 
I would also really appreciate a solution for this. Makes using Windows Terminal a pain right now because of all the flickering.
(huge apologies to folks following this thread, these notes are an unintelligible stream of consciousness)

Refer to #13163

My outline of events in https://github.com/microsoft/terminal/issues/13710#issuecomment-1209754247 might be wrong[1]. Look at the video closer - is that it? The left prompt is always visible, even as command and right prompt are flickering. Hmm.

[This diff](https://github.com/nushell/nushell/pull/6087/files#diff-fd310d9240cc6d04626c92f5f34f10a4f4e21db2b92ccbefb8d82c4672b20ba7) would seem to suggest that the prompt is written as one long string, "`FTCS A` + `prompt` + `FTCS B`", presumably followed by the command, followed by the right prompt. In 1.15, ~only FTCS A would have been implemented~. I don't think that matters 

~In 1.15 we return false out of the adapter on the `FTCS B` which means "I didn't understand that, just write that to the pipe NOW".~ this is also wrong.

ConPty always flushes all FTCS's in 1.15. On "`FTCS A` + `prompt` + `FTCS B`", we do 
```
[
  "FTCS A" -> Flush(),
  prompt + "FTCS B" -> Flush(),
  input command + right prompt -> (regularly scheduled) Paint()
]
```

as three frames. That it shouldn't. That probably should all be one frame. Can we just `FlushNoWrite()` from the adapter? Would that be insane? We'd have to be careful to write to the buffer, not write the frame to the pipe, write the sequence, but then also leave the engine in a state where the regularly scheduled paint frame tick would still cause the actual `Present` to the pipe, but with a quick path for "we had nothing to do but we did have stuff we already did.


I'll keep editing with notes as I go

[1] <sup>it might not be, I've re-edited this comment like 10 times as writing it</sup>

<hr>


This is what I get out the debug tap. Now, where are the frames....

* Frame 1:
```
␍
␛[K
␛[?25l
␛[57C
␛]133;A␛\
```
CR, cursor off, Cursor _Forward_ 17 (?!), mark

* Frame 2:
```
␛[32m
␛[1m
␍
C:\Users\migrie
␛]133;B␛\
␛[2␣q
␛[m
␛[38;5;14m
␛[1m
〉
␛[36m
␛[22m
z
␛[38;5;5m
␛[1m
␛[17C
01/11/2023␣09:43:35␣AM
␛[24;19H
␛[?25h
␛[m
```


This is what actually gets written to conhost
```
[?25l
[24;1H
[J
[38;5;10m
[1m
]133;A\
[1;32m
C:\Users\migrie
]133;B\
[38;5;14m
[1m
〉
[38;5;5m
[1m
7
[24;36H
01/11/2023 10:40:54 AM
8
[0m
[36m
zmh
[0m
7
8
[2 q
[?25h
```

`[2 q` looks out of place....

<hr>

Okay playing with this more locally, and staring at it, I do see the prompt section flickering as I'd expect. 

So, I guess the trick now is that even if we do _not_ immediately flush the frame, a regular frame might come in in the middle of the string. Hmm. 

Bad idea 
- if we have buffered output on StartPaint, but there's nothing to do, don't flush. 
- At the end of a VT string, in pty mode, poke the render engine to let it know it's safe to flush. 


The console should be locked for that whole write, start to finish. `ApiRoutines::WriteConsoleWImpl` takes the lock and holds it till it's all processed. So that must mean we're getting a `Flush` we're not expecting, _during_ the handling of the string. After the `133;A`, before the `B`. Hmm. 
I accidentally discovered one configuration in which nushell does not seem to flicker within Windows Terminal:

- Enabled WSL1 on a Windows 11 22H2 system.
- Installed [ArchWSL](https://github.com/yuk7/ArchWSL) as the Linux distribution (btw :) ).
- Installed (Windows version of) nushell via [scoop](https://scoop.sh).
- Ran `nu.exe` from within an ArchWSL profile in Windows Terminal.
- No flicker.

In contrast, all of the following exhibit the flicker in Windows Terminal (on the same Windows 11 22H2 system):

- Running Linux version of nushell (installed via `pacman`) from within ArchWSL profile in Windows Terminal.
- Running Windows version of nushell from within Powershell profile in Windows Terminal.
- Running Windows version of nushell from within cmd profile in Windows Terminal.

Absolutely no idea what it is about the former configuration that differs from the latter three in terms of flicker-proneness, but posting just in case it may be a clue for anyone who has a better bead on this.
@jxcrw that makes sense. The `nu.exe` in WSL invokes another ConPTY, one that shipped with the OS which farther out of date, and that version would end up probably either
* eating all the FTCS sequences
* passing them through immediately to the Terminal, before the rest of the frame _starts_ rendering, so nothing looks out of place (though, shell integration wouldn't work right)



Fix patch:
diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp
index 029eca42b7b..789020ebfc9 100644
--- a/src/host/screenInfo.cpp
+++ b/src/host/screenInfo.cpp
@@ -2323,7 +2323,10 @@ void SCREEN_INFORMATION::SetTerminalConnection(_In_ VtEngine* const pTtyConnecti
     if (pTtyConnection)
     {
         engine.SetTerminalConnection(pTtyConnection,
-                                     std::bind(&StateMachine::FlushToTerminal, _stateMachine.get()));
+                                     [&stateMachine = *_stateMachine]() -> bool {
+                                         ServiceLocator::LocateGlobals().pRender->NotifyPaintFrame();
+                                         return stateMachine.FlushToTerminal();
+                                     });
     }
     else
     {
diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp
index 33847e6106a..413779b0479 100644
--- a/src/renderer/vt/XtermEngine.cpp
+++ b/src/renderer/vt/XtermEngine.cpp
@@ -526,18 +526,17 @@ CATCH_RETURN();
     RETURN_IF_FAILED(_fUseAsciiOnly ?
                          VtEngine::_WriteTerminalAscii(wstr) :
                          VtEngine::_WriteTerminalUtf8(wstr));
-    // GH#4106, GH#2011 - WriteTerminalW is only ever called by the
+    // GH#4106, GH#2011, GH#13710 - WriteTerminalW is only ever called by the
     // StateMachine, when we've encountered a string we don't understand. When
-    // this happens, we usually don't actually trigger another frame, but we
-    // _do_ want this string to immediately be sent to the terminal. Since we
-    // only flush our buffer on actual frames, this means that strings we've
-    // decided to pass through would have gotten buffered here until the next
-    // actual frame is triggered.
-    //
-    // To fix this, flush here, so this string is sent to the connected terminal
-    // application.
+    // this happens, we will trigger a new frame in the renderer, and
+    // immediately buffer this wstr (representing the sequence we didn't
+    // understand). We won't immediately _Flush to the terminal - that might
+    // cause flickering (where we've buffered some state but not the whole
+    // "frame" as specified by the app). We'll just immediately buffer this
+    // sequence, and flush it when the render thread comes around to paint the
+    // frame normally.
 
-    return _Flush();
+    return S_OK;
 }
 
 // Method Description:
diff --git a/src/renderer/vt/invalidate.cpp b/src/renderer/vt/invalidate.cpp
index d6dd1b9db39..1c7f3fe498f 100644
--- a/src/renderer/vt/invalidate.cpp
+++ b/src/renderer/vt/invalidate.cpp
@@ -120,6 +120,10 @@ CATCH_RETURN();
         _circled = circled;
     }
 
+    // If we flushed for any reason other than circling (i.e, a sequence that we
+    // didn't understand), we don't need to push the buffer out on EndPaint.
+    _noFlushOnEnd = !circled;
+
     _trace.TraceTriggerCircling(*pForcePaint);
 
     return S_OK;
diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp
index 32a0a9949c0..dda813b92fe 100644
--- a/src/renderer/vt/paint.cpp
+++ b/src/renderer/vt/paint.cpp
@@ -94,7 +94,21 @@ using namespace Microsoft::Console::Types;
         RETURN_IF_FAILED(_MoveCursor(_deferredCursorPos));
     }
 
-    RETURN_IF_FAILED(_Flush());
+    // If this frame was triggered because we encountered a VT sequence which
+    // required the buffered state to get printed, we don't want to flush this
+    // frame to the pipe. That might result in us rendering half the output of a
+    // particular frame (as emitted by the client).
+    //
+    // Instead, we'll leave this frame in _buffer, and just keep appending to
+    // it as needed.
+    if (_noFlushOnEnd) [[unlikely]]
+    {
+        _noFlushOnEnd = false;
+    }
+    else
+    {
+        RETURN_IF_FAILED(_Flush());
+    }
 
     return S_OK;
 }
diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp
index 48bf6133efe..7f12d12d740 100644
--- a/src/renderer/vt/vtrenderer.hpp
+++ b/src/renderer/vt/vtrenderer.hpp
@@ -139,6 +139,7 @@ namespace Microsoft::Console::Render
 
         bool _resizeQuirk{ false };
         bool _passthrough{ false };
+        bool _noFlushOnEnd{ false };
         std::optional<TextColor> _newBottomLineBG{ std::nullopt };
 
         [[nodiscard]] HRESULT _WriteFill(const size_t n, const char c) noexcept;


Test patch: