Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent flickering in nushell due to FTCS marks #14677

Merged
merged 10 commits into from
May 11, 2023
5 changes: 4 additions & 1 deletion src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
19 changes: 9 additions & 10 deletions src/renderer/vt/XtermEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 4 additions & 0 deletions src/renderer/vt/invalidate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 15 additions & 1 deletion src/renderer/vt/paint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I don't think the [[unlikely]] is worth it here. Its implementation is super dumb in MSVC and causes any and all affected unlikely code to be moved to the end of the function, even if it's literally just a single mov [rcx+1234], 0 instruction like here. I think it's still useful, but it seems like its usage is mostly restricted to a rather "nuanced" application, especially after observing the disassembly output.

{
_noFlushOnEnd = false;
}
else
{
RETURN_IF_FAILED(_Flush());
}

return S_OK;
}
Expand Down
1 change: 1 addition & 0 deletions src/renderer/vt/vtrenderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down