Skip to content

Commit

Permalink
Filter out control characters that don't do anything (#15075)
Browse files Browse the repository at this point in the history
On a real VT terminal, most of the control characters that don't do
anything are supposed to be filtered out, and not written to the buffer.
Up to to now, though, we've only been filtering out `NUL`. This PR
extends our control processing to filter the remaining characters that
aren't supposed to be displayed.

We introduced filtering for the `NUL` control in PR #3015.

The are two special cases worth mentioning.

1. The `SUB` control's main purpose is to the cancel a control sequence
that is in progress, but it also needs to output an error character (a
reverse question mark) to the display.

2. The `DEL` control is typically filtered out, but when a 96-character
set is designated, it can sometimes be mapped to a printable glyph that
needs to be displayed.

## Validation Steps Performed

I've manually tested that all the controls that are meant to be filtered
out are no longer being displayed.

I've also extended the existing `NUL` unit test to cover the full set of
controls characters that are supposed to be filtered.

Closes #10786
  • Loading branch information
j4james authored Apr 5, 2023
1 parent 06526ca commit aea0477
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 19 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ ansicpg
ANSISYS
ANSISYSRC
ANSISYSSC
answerback
antialiasing
ANull
anycpu
Expand Down
37 changes: 23 additions & 14 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class ScreenBufferTests

TEST_METHOD(EraseAllTests);

TEST_METHOD(OutputNULTest);
TEST_METHOD(InactiveControlCharactersTest);

TEST_METHOD(VtResize);
TEST_METHOD(VtResizeComprehensive);
Expand Down Expand Up @@ -1014,8 +1014,19 @@ void ScreenBufferTests::EraseAllTests()
viewport.BottomInclusive()));
}

void ScreenBufferTests::OutputNULTest()
void ScreenBufferTests::InactiveControlCharactersTest()
{
// These are the control characters that don't write anything to the
// output buffer, and are expected not to move the cursor position.

BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:ordinal", L"{0, 1, 2, 3, 4, 5, 6, 7, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 28, 29, 30, 31}")
END_TEST_METHOD_PROPERTIES()

unsigned ordinal;
VERIFY_SUCCEEDED(TestData::TryGetValue(L"ordinal", ordinal));
const auto ch = static_cast<wchar_t>(ordinal);

auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& stateMachine = si.GetStateMachine();
Expand All @@ -1024,31 +1035,29 @@ void ScreenBufferTests::OutputNULTest()
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Writing a single NUL"));
stateMachine.ProcessString({ L"\0", 1 });
Log::Comment(L"Writing a single control character");
const auto singleChar = std::wstring(1, ch);
stateMachine.ProcessString(singleChar);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Writing many NULs"));
stateMachine.ProcessString({ L"\0\0\0\0\0\0\0\0", 8 });
Log::Comment(L"Writing many control characters");
const auto manyChars = std::wstring(8, ch);
stateMachine.ProcessString(manyChars);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Testing a single NUL followed by real text"));
stateMachine.ProcessString({ L"\0foo", 4 });
Log::Comment(L"Testing a single control character followed by real text");
stateMachine.ProcessString(singleChar + L"foo");
VERIFY_ARE_EQUAL(3, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

stateMachine.ProcessString(L"\n");
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(1, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Writing NULs in between other strings"));
stateMachine.ProcessString({ L"\0foo\0bar\0", 9 });
Log::Comment(L"Writing controls in between other strings");
stateMachine.ProcessString(singleChar + L"foo" + singleChar + L"bar" + singleChar);
VERIFY_ARE_EQUAL(6, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(1, cursor.GetPosition().y);
}
Expand Down
25 changes: 20 additions & 5 deletions src/terminal/parser/OutputStateMachineEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)
{
switch (wch)
{
case AsciiChars::NUL:
// microsoft/terminal#1825 - VT applications expect to be able to write NUL
// and have _nothing_ happen. Filter the NULs here, so they don't fill the
// buffer with empty spaces.
case AsciiChars::ENQ:
// GH#11946: At some point we may want to add support for the VT
// answerback feature, which requires responding to an ENQ control
// with a user-defined reply, but until then we just ignore it.
break;
case AsciiChars::BEL:
_dispatch->WarningBell();
Expand Down Expand Up @@ -79,9 +79,24 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)
case AsciiChars::SO:
_dispatch->LockingShift(1);
break;
default:
case AsciiChars::SUB:
// The SUB control is used to cancel a control sequence in the same
// way as CAN, but unlike CAN it also displays an error character,
// typically a reverse question mark.
_dispatch->Print(L'\u2E2E');
break;
case AsciiChars::DEL:
// The DEL control can sometimes be translated into a printable glyph
// if a 96-character set is designated, so we need to pass it through
// to the Print method. If not translated, it will be filtered out
// there.
_dispatch->Print(wch);
break;
default:
// GH#1825, GH#10786: VT applications expect to be able to write other
// control characters and have _nothing_ happen. We filter out these
// characters here, so they don't fill the buffer.
break;
}

_ClearLastChar();
Expand Down

0 comments on commit aea0477

Please sign in to comment.