Skip to content

Commit

Permalink
Dispatch more C0 control characters from the VT state machine (micros…
Browse files Browse the repository at this point in the history
…oft#4171)

This commit moves the handling of the `BEL`, `BS`, `TAB`, and `CR`
controls characters into the state machine (when in VT mode), instead of
forwarding them on to the default string writer, which would otherwise
have to parse them out all over again.

This doesn't cover all the control characters, but `ESC`, `SUB`, and
`CAN` are already an integral part of the `StateMachine` itself; `NUL`
is filtered out by the `OutputStateMachineEngine`; and `LF`, `FF`, and
`VT`  are due to be implemented as part of PR microsoft#3271.

Once all of these controls are handled at the state machine level, we
can strip out all the VT-specific code from the `WriteCharsLegacy`
function, which should simplify it considerably. This would also let us
simplify the `Terminal::_WriteBuffer` implementation, and the planned
replacement stream writer for issue microsoft#780.

On the conhost side, the implementation is handled as follows:

* The `BS` control is dispatched to the existing `CursorBackward`
  method, with a distance of 1.
* The `TAB` control is dispatched to the existing `ForwardTab` method,
  with a tab count of 1.
* The `CR` control required a new dispatch method, but the
  implementation was a simple call to the new `_CursorMovePosition` method
  from PR microsoft#3628.
* The `BEL` control also required a new dispatch method, as well as an
  additional private API in the `ConGetSet` interface. But that's mostly
  boilerplate code - ultimately it just calls the `SendNotifyBeep` method.

On the Windows Terminal side, not all dispatch methods are implemented.

* There is an existing `CursorBackward` implementation, so `BS` works
  OK.
* There isn't a `ForwardTab` implementation, but `TAB` isn't currently
  required by the conpty protocol.
* I had to implement the `CarriageReturn` dispatch method, but that was
  a simple call to `Terminal::SetCursorPosition`.
* The `WarningBell` method I've left unimplemented, because that
  functionality wasn't previously supported anyway, and there's an
  existing issue for that (microsoft#4046).

## Validation Steps Performed

I've added a state machine test to confirm that the updated control
characters are now forwarded to the appropriate dispatch handlers. But
since the actual implementation is mostly relying on existing
functionality, I'm assuming that code is already adequately tested
elsewhere. That said, I have also run various manual tests of my own,
and confirmed that everything still worked as well as before.

References microsoft#3271
References microsoft#780
References microsoft#3628
References microsoft#4046
  • Loading branch information
j4james authored and DHowett committed Jan 17, 2020
1 parent 2fec178 commit 0586955
Show file tree
Hide file tree
Showing 12 changed files with 141 additions and 10 deletions.
8 changes: 8 additions & 0 deletions src/cascadia/TerminalCore/TerminalDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,14 @@ try
}
CATCH_LOG_RETURN_FALSE()

bool TerminalDispatch::CarriageReturn() noexcept
try
{
const auto cursorPos = _terminalApi.GetCursorPosition();
return _terminalApi.SetCursorPosition(0, cursorPos.Y);
}
CATCH_LOG_RETURN_FALSE()

bool TerminalDispatch::SetWindowTitle(std::wstring_view title) noexcept
try
{
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalCore/TerminalDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class TerminalDispatch : public Microsoft::Console::VirtualTerminal::TermDispatc
bool LineFeed(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::LineFeedType lineFeedType) noexcept override;

bool EraseCharacters(const size_t numChars) noexcept override;
bool CarriageReturn() noexcept override;
bool SetWindowTitle(std::wstring_view title) noexcept override;

bool SetColorTableEntry(const size_t tableIndex, const DWORD color) noexcept override;
Expand Down
9 changes: 9 additions & 0 deletions src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,15 @@ bool ConhostInternalGetSet::PrivateLineFeed(const bool withReturn)
return NT_SUCCESS(DoSrvPrivateLineFeed(_io.GetActiveOutputBuffer(), withReturn));
}

// Routine Description:
// - Sends a notify message to play the "SystemHand" sound event.
// Return Value:
// - true if successful. false otherwise.
bool ConhostInternalGetSet::PrivateWarningBell()
{
return _io.GetActiveOutputBuffer().SendNotifyBeep();
}

// Routine Description:
// - Connects the PrivateReverseLineFeed call directly into our Driver Message servicing call inside Conhost.exe
// PrivateReverseLineFeed is an internal-only "API" call that the vt commands can execute,
Expand Down
2 changes: 2 additions & 0 deletions src/host/outputStream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal::

bool PrivateSetScrollingRegion(const SMALL_RECT& scrollMargins) override;

bool PrivateWarningBell() override;

bool PrivateGetLineFeedMode() const override;
bool PrivateLineFeed(const bool withReturn) override;
bool PrivateReverseLineFeed() override;
Expand Down
2 changes: 2 additions & 0 deletions src/terminal/adapter/ITermDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch
virtual bool EnableCursorBlinking(const bool enable) = 0; // ATT610
virtual bool SetOriginMode(const bool relativeMode) = 0; // DECOM
virtual bool SetTopBottomScrollingMargins(const size_t topMargin, const size_t bottomMargin) = 0; // DECSTBM
virtual bool WarningBell() = 0; // BEL
virtual bool CarriageReturn() = 0; // CR
virtual bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) = 0; // IND, NEL
virtual bool ReverseLineFeed() = 0; // RI
virtual bool SetWindowTitle(std::wstring_view title) = 0; // OscWindowTitle
Expand Down
24 changes: 24 additions & 0 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,30 @@ bool AdaptDispatch::SetTopBottomScrollingMargins(const size_t topMargin,
return _DoSetTopBottomScrollingMargins(topMargin, bottomMargin) && CursorPosition(1, 1);
}

// Routine Description:
// - BEL - Rings the warning bell.
// Causes the terminal to emit an audible tone of brief duration.
// Arguments:
// - None
// Return Value:
// - True if handled successfully. False otherwise.
bool AdaptDispatch::WarningBell()
{
return _pConApi->PrivateWarningBell();
}

// Routine Description:
// - CR - Performs a carriage return.
// Moves the cursor to the leftmost column.
// Arguments:
// - None
// Return Value:
// - True if handled successfully. False otherwise.
bool AdaptDispatch::CarriageReturn()
{
return _CursorMovePosition(Offset::Unchanged(), Offset::Absolute(1), true);
}

// Routine Description:
// - IND/NEL - Performs a line feed, possibly preceded by carriage return.
// Moves the cursor down one line, and possibly also to the leftmost column.
Expand Down
2 changes: 2 additions & 0 deletions src/terminal/adapter/adaptDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ namespace Microsoft::Console::VirtualTerminal
bool SetOriginMode(const bool relativeMode) noexcept override; // DECOM
bool SetTopBottomScrollingMargins(const size_t topMargin,
const size_t bottomMargin) override; // DECSTBM
bool WarningBell() override; // BEL
bool CarriageReturn() override; // CR
bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) override; // IND, NEL
bool ReverseLineFeed() override; // RI
bool SetWindowTitle(const std::wstring_view title) override; // OscWindowTitle
Expand Down
1 change: 1 addition & 0 deletions src/terminal/adapter/conGetSet.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ namespace Microsoft::Console::VirtualTerminal
virtual bool PrivateAllowCursorBlinking(const bool enable) = 0;

virtual bool PrivateSetScrollingRegion(const SMALL_RECT& scrollMargins) = 0;
virtual bool PrivateWarningBell() = 0;
virtual bool PrivateGetLineFeedMode() const = 0;
virtual bool PrivateLineFeed(const bool withReturn) = 0;
virtual bool PrivateReverseLineFeed() = 0;
Expand Down
2 changes: 2 additions & 0 deletions src/terminal/adapter/termDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class Microsoft::Console::VirtualTerminal::TermDispatch : public Microsoft::Cons
bool EnableCursorBlinking(const bool /*enable*/) noexcept override { return false; } // ATT610
bool SetOriginMode(const bool /*relativeMode*/) noexcept override { return false; }; // DECOM
bool SetTopBottomScrollingMargins(const size_t /*topMargin*/, const size_t /*bottomMargin*/) noexcept override { return false; } // DECSTBM
bool WarningBell() noexcept override { return false; } // BEL
bool CarriageReturn() noexcept override { return false; } // CR
bool LineFeed(const DispatchTypes::LineFeedType /*lineFeedType*/) noexcept override { return false; } // IND, NEL
bool ReverseLineFeed() noexcept override { return false; } // RI
bool SetWindowTitle(std::wstring_view /*title*/) noexcept override { return false; } // OscWindowTitle
Expand Down
7 changes: 7 additions & 0 deletions src/terminal/adapter/ut_adapter/adapterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,13 @@ class TestGetSet final : public ConGetSet
return _privateSetScrollingRegionResult;
}

bool PrivateWarningBell() override
{
Log::Comment(L"PrivateWarningBell MOCK called...");
// We made it through the adapter, woo! Return true.
return TRUE;
}

bool PrivateGetLineFeedMode() const override
{
Log::Comment(L"PrivateGetLineFeedMode MOCK called...");
Expand Down
28 changes: 18 additions & 10 deletions src/terminal/parser/OutputStateMachineEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,24 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)
// and have _nothing_ happen. Filter the NULs here, so they don't fill the
// buffer with empty spaces.
break;
case AsciiChars::BEL:
_dispatch->WarningBell();
// microsoft/terminal#2952
// If we're attached to a terminal, let's also pass the BEL through.
if (_pfnFlushToTerminal != nullptr)
{
_pfnFlushToTerminal();
}
break;
case AsciiChars::BS:
_dispatch->CursorBackward(1);
break;
case AsciiChars::TAB:
_dispatch->ForwardTab(1);
break;
case AsciiChars::CR:
_dispatch->CarriageReturn();
break;
case AsciiChars::LF:
case AsciiChars::FF:
case AsciiChars::VT:
Expand All @@ -59,16 +77,6 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)

_ClearLastChar();

if (wch == AsciiChars::BEL)
{
// microsoft/terminal#2952
// If we're attached to a terminal, let's also pass the BEL through.
if (_pfnFlushToTerminal != nullptr)
{
_pfnFlushToTerminal();
}
}

return true;
}

Expand Down
65 changes: 65 additions & 0 deletions src/terminal/parser/ut_parser/OutputEngineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -677,8 +677,12 @@ class StatefulDispatch final : public TermDispatch
_cursorKeysMode{ false },
_cursorBlinking{ true },
_isOriginModeRelative{ false },
_warningBell{ false },
_carriageReturn{ false },
_lineFeed{ false },
_lineFeedType{ (DispatchTypes::LineFeedType)-1 },
_forwardTab{ false },
_numTabs{ 0 },
_isDECCOLMAllowed{ false },
_windowWidth{ 80 },
_options{ s_cMaxOptions, static_cast<DispatchTypes::GraphicsOptions>(s_uiGraphicsCleared) } // fill with cleared option
Expand Down Expand Up @@ -906,13 +910,32 @@ class StatefulDispatch final : public TermDispatch
return true;
}

bool WarningBell() noexcept override
{
_warningBell = true;
return true;
}

bool CarriageReturn() noexcept override
{
_carriageReturn = true;
return true;
}

bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) noexcept override
{
_lineFeed = true;
_lineFeedType = lineFeedType;
return true;
}

bool ForwardTab(const size_t numTabs) noexcept override
{
_forwardTab = true;
_numTabs = numTabs;
return true;
}

bool EnableDECCOLMSupport(const bool fEnabled) noexcept override
{
_isDECCOLMAllowed = fEnabled;
Expand Down Expand Up @@ -959,8 +982,12 @@ class StatefulDispatch final : public TermDispatch
bool _cursorKeysMode;
bool _cursorBlinking;
bool _isOriginModeRelative;
bool _warningBell;
bool _carriageReturn;
bool _lineFeed;
DispatchTypes::LineFeedType _lineFeedType;
bool _forwardTab;
size_t _numTabs;
bool _isDECCOLMAllowed;
size_t _windowWidth;

Expand Down Expand Up @@ -1844,4 +1871,42 @@ class StateMachineExternalTest final

pDispatch->ClearState();
}

TEST_METHOD(TestControlCharacters)
{
auto dispatch = std::make_unique<StatefulDispatch>();
auto pDispatch = dispatch.get();
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));
StateMachine mach(std::move(engine));

Log::Comment(L"BEL (Warning Bell) control character");
mach.ProcessCharacter(AsciiChars::BEL);

VERIFY_IS_TRUE(pDispatch->_warningBell);

pDispatch->ClearState();

Log::Comment(L"BS (Back Space) control character");
mach.ProcessCharacter(AsciiChars::BS);

VERIFY_IS_TRUE(pDispatch->_cursorBackward);
VERIFY_ARE_EQUAL(1u, pDispatch->_cursorDistance);

pDispatch->ClearState();

Log::Comment(L"CR (Carriage Return) control character");
mach.ProcessCharacter(AsciiChars::CR);

VERIFY_IS_TRUE(pDispatch->_carriageReturn);

pDispatch->ClearState();

Log::Comment(L"HT (Horizontal Tab) control character");
mach.ProcessCharacter(AsciiChars::TAB);

VERIFY_IS_TRUE(pDispatch->_forwardTab);
VERIFY_ARE_EQUAL(1u, pDispatch->_numTabs);

pDispatch->ClearState();
}
};

0 comments on commit 0586955

Please sign in to comment.