Skip to content

Commit

Permalink
When Conpty encounters an unknown string, flush immediately (#4896)
Browse files Browse the repository at this point in the history
When ConPTY encounters a string we don't understand, immediately flush the frame.

## References

This PR supersedes #2665. This solution is much simpler than what was proposed in that PR. 
As mentioned in #2665: "This might have some long-term consequences for #1173."

## PR Checklist
* [x] Closes #2011
* [x] Closes #4106
* [x] I work here
* [x] Tests added/passed
  • Loading branch information
zadjii-msft authored Mar 12, 2020
1 parent 275417c commit ffd8f53
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 11 deletions.
31 changes: 31 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final

TEST_METHOD(PassthroughClearScrollback);

TEST_METHOD(PassthroughCursorShapeImmediately);

TEST_METHOD(TestWrappingALongString);
TEST_METHOD(TestAdvancedWrapping);
TEST_METHOD(TestExactWrappingWithoutSpaces);
Expand Down Expand Up @@ -673,6 +675,35 @@ void ConptyRoundtripTests::MoveCursorAtEOL()
verifyData1(termTb);
}

void ConptyRoundtripTests::PassthroughCursorShapeImmediately()
{
// This is a test for GH#4106, and more indirectly, GH #2011.

Log::Comment(NoThrowString().Format(
L"Change the cursor shape with VT. This should immediately be flushed to the Terminal."));
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());

auto& g = ServiceLocator::LocateGlobals();
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& hostSm = si.GetStateMachine();
auto& hostTb = si.GetTextBuffer();
auto& termTb = *term->_buffer;

_flushFirstFrame();

_logConpty = true;

VERIFY_ARE_NOT_EQUAL(CursorType::VerticalBar, hostTb.GetCursor().GetType());
VERIFY_ARE_NOT_EQUAL(CursorType::VerticalBar, termTb.GetCursor().GetType());

expectedOutput.push_back("\x1b[5 q");
hostSm.ProcessString(L"\x1b[5 q");

VERIFY_ARE_EQUAL(CursorType::VerticalBar, hostTb.GetCursor().GetType());
VERIFY_ARE_EQUAL(CursorType::VerticalBar, termTb.GetCursor().GetType());
}

void ConptyRoundtripTests::PassthroughClearScrollback()
{
Log::Comment(NoThrowString().Format(
Expand Down
14 changes: 13 additions & 1 deletion src/renderer/vt/WinTelnetEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,5 +111,17 @@ WinTelnetEngine::WinTelnetEngine(_In_ wil::unique_hfile hPipe,
// - S_OK or suitable HRESULT error from either conversion or writing pipe.
[[nodiscard]] HRESULT WinTelnetEngine::WriteTerminalW(_In_ const std::wstring_view wstr) noexcept
{
return VtEngine::_WriteTerminalAscii(wstr);
RETURN_IF_FAILED(VtEngine::_WriteTerminalAscii(wstr));
// GH#4106, GH#2011 - 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.

return _Flush();
}
18 changes: 15 additions & 3 deletions src/renderer/vt/XtermEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -477,9 +477,21 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
// - S_OK or suitable HRESULT error from either conversion or writing pipe.
[[nodiscard]] HRESULT XtermEngine::WriteTerminalW(const std::wstring_view wstr) noexcept
{
return _fUseAsciiOnly ?
VtEngine::_WriteTerminalAscii(wstr) :
VtEngine::_WriteTerminalUtf8(wstr);
RETURN_IF_FAILED(_fUseAsciiOnly ?
VtEngine::_WriteTerminalAscii(wstr) :
VtEngine::_WriteTerminalUtf8(wstr));
// GH#4106, GH#2011 - 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.

return _Flush();
}

// Method Description:
Expand Down
16 changes: 9 additions & 7 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1847,13 +1847,6 @@ bool AdaptDispatch::EnableAlternateScroll(const bool enabled)
// True if handled successfully. False otherwise.
bool AdaptDispatch::SetCursorStyle(const DispatchTypes::CursorStyle cursorStyle)
{
bool isPty = false;
_pConApi->IsConsolePty(isPty);
if (isPty)
{
return false;
}

CursorType actualType = CursorType::Legacy;
bool fEnableBlinking = false;

Expand Down Expand Up @@ -1894,6 +1887,15 @@ bool AdaptDispatch::SetCursorStyle(const DispatchTypes::CursorStyle cursorStyle)
success = _pConApi->PrivateAllowCursorBlinking(fEnableBlinking);
}

// If we're a conpty, always return false, so that this cursor state will be
// sent to the connected terminal
bool isPty = false;
_pConApi->IsConsolePty(isPty);
if (isPty)
{
return false;
}

return success;
}

Expand Down

0 comments on commit ffd8f53

Please sign in to comment.