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

Ensure that delayed EOL wrap is reset when necessary #14936

Merged
merged 8 commits into from
Mar 4, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/buffer/out/cursor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,9 @@ void Cursor::CopyProperties(const Cursor& OtherCursor) noexcept
_cursorType = OtherCursor._cursorType;
}

void Cursor::DelayEOLWrap(const til::point coordDelayedAt) noexcept
void Cursor::DelayEOLWrap() noexcept
{
_coordDelayedAt = coordDelayedAt;
_coordDelayedAt = _cPosition;
_fDelayedEolWrap = true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/cursor.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class Cursor final

void CopyProperties(const Cursor& OtherCursor) noexcept;

void DelayEOLWrap(const til::point coordDelayedAt) noexcept;
void DelayEOLWrap() noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

I love this. Removing parameters that should be known by internal state. 😄

void ResetDelayEOLWrap() noexcept;
til::point GetDelayedAtPosition() const noexcept;
bool IsDelayedEOLWrap() const noexcept;
Expand Down
5 changes: 5 additions & 0 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,11 @@ void TextBuffer::SetCurrentLineRendition(const LineRendition lineRendition)
}
TriggerRedraw(Viewport::FromDimensions({ 0, rowIndex }, { GetSize().Width(), 1 }));
}
// There is some variation in how this was handled by the different DEC
// terminals, but the STD 070 reference (on page D-13) makes it clear that
// the delayed wrap (aka the Last Column Flag) was expected to be reset when
// line rendition controls were executed.
GetCursor().ResetDelayEOLWrap();
j4james marked this conversation as resolved.
Show resolved Hide resolved
}

void TextBuffer::ResetLineRenditionRange(const til::CoordType startRow, const til::CoordType endRow) noexcept
Expand Down
119 changes: 112 additions & 7 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,8 @@ class ScreenBufferTests
TEST_METHOD(TestDeferredMainBufferResize);

TEST_METHOD(RectangularAreaOperations);

TEST_METHOD(DelayedWrapReset);
};

void ScreenBufferTests::SingleAlternateBufferCreationTest()
Expand Down Expand Up @@ -6266,48 +6268,55 @@ void ScreenBufferTests::CursorSaveRestore()
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, til::point(0, 0), true));

Log::Comment(L"Restore after save.");
// Set the cursor position, attributes, and character set.
// Set the cursor position, delayed wrap, attributes, and character set.
cursor.SetPosition({ 20, 10 });
cursor.DelayEOLWrap();
si.SetAttributes(colorAttrs);
stateMachine.ProcessString(selectGraphicsChars);
// Save state.
stateMachine.ProcessString(saveCursor);
// Reset the cursor position, attributes, and character set.
// Reset the cursor position, delayed wrap, attributes, and character set.
cursor.SetPosition({ 0, 0 });
si.SetAttributes(defaultAttrs);
stateMachine.ProcessString(selectAsciiChars);
// Restore state.
stateMachine.ProcessString(restoreCursor);
// Verify initial position, colors, and graphic character set.
// Verify initial position, delayed wrap, colors, and graphic character set.
VERIFY_ARE_EQUAL(til::point(20, 10), cursor.GetPosition());
VERIFY_IS_TRUE(cursor.IsDelayedEOLWrap());
cursor.ResetDelayEOLWrap();
VERIFY_ARE_EQUAL(colorAttrs, si.GetAttributes());
stateMachine.ProcessString(asciiText);
VERIFY_IS_TRUE(_ValidateLineContains({ 20, 10 }, graphicText, colorAttrs));

Log::Comment(L"Restore again without save.");
// Reset the cursor position, attributes, and character set.
// Reset the cursor position, delayed wrap, attributes, and character set.
cursor.SetPosition({ 0, 0 });
si.SetAttributes(defaultAttrs);
stateMachine.ProcessString(selectAsciiChars);
// Restore state.
stateMachine.ProcessString(restoreCursor);
// Verify initial saved position, colors, and graphic character set.
// Verify initial saved position, delayed wrap, colors, and graphic character set.
VERIFY_ARE_EQUAL(til::point(20, 10), cursor.GetPosition());
VERIFY_IS_TRUE(cursor.IsDelayedEOLWrap());
cursor.ResetDelayEOLWrap();
VERIFY_ARE_EQUAL(colorAttrs, si.GetAttributes());
stateMachine.ProcessString(asciiText);
VERIFY_IS_TRUE(_ValidateLineContains({ 20, 10 }, graphicText, colorAttrs));

Log::Comment(L"Restore after reset.");
// Soft reset.
stateMachine.ProcessString(L"\x1b[!p");
// Set the cursor position, attributes, and character set.
// Set the cursor position, delayed wrap, attributes, and character set.
cursor.SetPosition({ 20, 10 });
cursor.DelayEOLWrap();
si.SetAttributes(colorAttrs);
stateMachine.ProcessString(selectGraphicsChars);
// Restore state.
stateMachine.ProcessString(restoreCursor);
// Verify home position, default attributes, and ascii character set.
// Verify home position, no delayed wrap, default attributes, and ascii character set.
VERIFY_ARE_EQUAL(til::point(0, 0), cursor.GetPosition());
VERIFY_IS_FALSE(cursor.IsDelayedEOLWrap());
VERIFY_ARE_EQUAL(defaultAttrs, si.GetAttributes());
stateMachine.ProcessString(asciiText);
VERIFY_IS_TRUE(_ValidateLineContains(til::point(0, 0), asciiText, defaultAttrs));
Expand Down Expand Up @@ -7538,3 +7547,99 @@ void ScreenBufferTests::RectangularAreaOperations()
VERIFY_IS_TRUE(_ValidateLinesContain(targetArea.top, targetArea.bottom, bufferChars, bufferAttr));
VERIFY_IS_TRUE(_ValidateLinesContain(targetArea.right, targetArea.top, targetArea.bottom, bufferChar, bufferAttr));
}

void ScreenBufferTests::DelayedWrapReset()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
TEST_METHOD_PROPERTY(L"Data:op", L"{0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34}")
Copy link
Member

Choose a reason for hiding this comment

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

I feel like a loop over ops might be easier without compromising on the tests too much. I mean, IsolationLevel = Method means that this won't reset the console state after every round anyways, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I know this is horrible, and I did consider using a loop, but the IsolationLevel does actually give us a clean state for each op, and that's necessary in these tests to prevent them interfering with each other. Otherwise we'd have to do a bunch of manual clean up which would be a bit messy.

The other advantage of this approach is that you get a separate test case for each op, so say there's a failure in three of the ops, you'll get three separate errors, and you know exactly what's broken. With a loop, the first failure would prevent the rest of the ops from running, which isn't as nice.

Copy link
Member

Choose a reason for hiding this comment

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

I for one am a big fan of the "gross set of test properties that result in separate test runs". Pretty sure I do that all over RoundtripTests 😝

Copy link
Member

@DHowett DHowett Mar 1, 2023

Choose a reason for hiding this comment

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

I rather like this approach as well. I have it on my list to take the Reflow tests (which use a custom TAEF COM class to offer table-based testing where each test case gets its own name!) and make them "generic" so that other tests can rely on them instead of this, but at the end of the day it is still a way to make a hundred individual test cases in TAEF's eyes.

I have significant beef with the kind of test Leonard is suggesting:

TEST_METHOD(TestEverythingEverywhereAllAtOnce) {
    for(auto i = 0; i < 100; ++i) {
        VERIFY_FALSE(foo, "foo failed lol");
    }
}

which iteration failed? good luck figuring it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, some form of table-based testing would have been nice here. Ideally my ops array could have been declared as the property source, and each op would then trigger an individual test run. Similar to a Scenario Outline in a Gherkin test, where you have a table of examples.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #14950 with a nod to this test 😄

END_TEST_METHOD_PROPERTIES();

int opIndex;
VERIFY_SUCCEEDED(TestData::TryGetValue(L"op", opIndex));

auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
auto& stateMachine = si.GetStateMachine();
const auto width = si.GetTextBuffer().GetSize().Width();
const auto halfWidth = width / 2;
WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
WI_SetFlag(si.OutputMode, DISABLE_NEWLINE_AUTO_RETURN);
stateMachine.ProcessString(L"\033[?40h"); // Make sure DECCOLM is allowed

const auto startRow = 5;
const auto startCol = width - 1;

// The operations below are all those that the DEC STD 070 reference has
// documented as needing to reset the Last Column Flag (see page D-13). The
// only controls that we haven't included are HT and SUB, because most DEC
// terminals did *not* trigger a reset after executing those sequences, and
// most modern terminals also seem to have agreed that that is the correct
// approach to take.
const struct
{
std::wstring_view name;
std::wstring_view sequence;
til::point expectedPos = {};
bool absolutePos = false;
} ops[] = {
{ L"DECSTBM", L"\033[1;10r", { 0, 0 }, true },
{ L"DECSWL", L"\033#5" },
{ L"DECDWL", L"\033#6", { halfWidth - 1, startRow }, true },
{ L"DECDHL (top)", L"\033#3", { halfWidth - 1, startRow }, true },
{ L"DECDHL (bottom)", L"\033#4", { halfWidth - 1, startRow }, true },
{ L"DECCOLM set", L"\033[?3h", { 0, 0 }, true },
{ L"DECOM set", L"\033[?6h", { 0, 0 }, true },
{ L"DECCOLM set", L"\033[?3l", { 0, 0 }, true },
{ L"DECOM reset", L"\033[?6l", { 0, 0 }, true },
{ L"DECAWM reset", L"\033[?7l" },
{ L"CUU", L"\033[A", { 0, -1 } },
{ L"CUD", L"\033[B", { 0, 1 } },
{ L"CUF", L"\033[C" },
{ L"CUB", L"\033[D", { -1, 0 } },
{ L"CUP", L"\033[3;7H", { 6, 2 }, true },
{ L"HVP", L"\033[3;7f", { 6, 2 }, true },
{ L"BS", L"\b", { -1, 0 } },
{ L"LF", L"\n", { 0, 1 } },
{ L"VT", L"\v", { 0, 1 } },
{ L"FF", L"\f", { 0, 1 } },
{ L"CR", L"\r", { 0, startRow }, true },
{ L"IND", L"\033D", { 0, 1 } },
{ L"RI", L"\033M", { 0, -1 } },
{ L"NEL", L"\033E", { 0, startRow + 1 }, true },
{ L"ECH", L"\033[X" },
{ L"DCH", L"\033[P" },
{ L"ICH", L"\033[@" },
{ L"EL", L"\033[K" },
{ L"DECSEL", L"\033[?K" },
{ L"DL", L"\033[M", { 0, startRow }, true },
{ L"IL", L"\033[L", { 0, startRow }, true },
{ L"ED", L"\033[J" },
{ L"ED (all)", L"\033[2J" },
{ L"ED (scrollback)", L"\033[3J" },
{ L"DECSED", L"\033[?J" },
};
const auto& op = gsl::at(ops, opIndex);

Log::Comment(L"Writing a character at the end of the line should set delayed EOL wrap");
const auto startPos = til::point{ startCol, startRow };
si.GetTextBuffer().GetCursor().SetPosition(startPos);
stateMachine.ProcessCharacter(L'X');
{
auto& cursor = si.GetTextBuffer().GetCursor();
VERIFY_IS_TRUE(cursor.IsDelayedEOLWrap());
VERIFY_ARE_EQUAL(startPos, cursor.GetPosition());
}

Log::Comment(NoThrowString().Format(
L"Executing a %s control should reset delayed EOL wrap",
op.name.data()));
const auto expectedPos = op.absolutePos ? op.expectedPos : startPos + op.expectedPos;
stateMachine.ProcessString(op.sequence);
{
auto& cursor = si.GetTextBuffer().GetCursor();
const auto actualPos = cursor.GetPosition() - si.GetViewport().Origin();
VERIFY_IS_FALSE(cursor.IsDelayedEOLWrap());
VERIFY_ARE_EQUAL(expectedPos, actualPos);
}
}
6 changes: 5 additions & 1 deletion src/renderer/vt/paint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,11 @@ using namespace Microsoft::Console::Types;
{
RETURN_IF_FAILED(_EraseCharacter(numSpaces));
}
else
// If we're past the end of the row (i.e. in the "delayed EOL wrap"
// state), then there is no need to erase the rest of line. In fact
// if we did output an EL sequence at this point, it could reset the
// "delayed EOL wrap" state, breaking subsequent output.
else if (_lastText.x <= _lastViewport.RightInclusive())
{
RETURN_IF_FAILED(_EraseLine());
}
Expand Down
45 changes: 44 additions & 1 deletion src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
cursor.SetPosition(cursorPosition);
if (wrapAtEOL)
{
cursor.DelayEOLWrap(cursorPosition);
cursor.DelayEOLWrap();
}
}
else
Expand Down Expand Up @@ -446,6 +446,7 @@ bool AdaptDispatch::CursorSaveState()
auto& savedCursorState = _savedCursorState.at(_usingAltBuffer);
savedCursorState.Column = cursorPosition.x + 1;
savedCursorState.Row = cursorPosition.y + 1;
savedCursorState.IsDelayedEOLWrap = textBuffer.GetCursor().IsDelayedEOLWrap();
savedCursorState.IsOriginModeRelative = _modes.test(Mode::Origin);
savedCursorState.Attributes = attributes;
savedCursorState.TermOutput = _termOutput;
Expand Down Expand Up @@ -484,6 +485,12 @@ bool AdaptDispatch::CursorRestoreState()
_modes.reset(Mode::Origin);
CursorPosition(row, col);

// If the delayed wrap flag was set when the cursor was saved, we need to restore than now.
DHowett marked this conversation as resolved.
Show resolved Hide resolved
if (savedCursorState.IsDelayedEOLWrap)
{
_api.GetTextBuffer().GetCursor().DelayEOLWrap();
}

// Once the cursor position is restored, we can then restore the actual origin mode.
_modes.set(Mode::Origin, savedCursorState.IsOriginModeRelative);

Expand Down Expand Up @@ -600,6 +607,8 @@ void AdaptDispatch::_InsertDeleteCharacterHelper(const VTInt delta)
const auto startCol = textBuffer.GetCursor().GetPosition().x;
const auto endCol = textBuffer.GetLineWidth(row);
_ScrollRectHorizontally(textBuffer, { startCol, row, endCol, row + 1 }, delta);
// The ICH and DCH controls are expected to reset the delayed wrap flag.
textBuffer.GetCursor().ResetDelayEOLWrap();
}

// Routine Description:
Expand Down Expand Up @@ -668,6 +677,9 @@ bool AdaptDispatch::EraseCharacters(const VTInt numChars)
const auto startCol = textBuffer.GetCursor().GetPosition().x;
const auto endCol = std::min<VTInt>(startCol + numChars, textBuffer.GetLineWidth(row));

// The ECH control is expected to reset the delayed wrap flag.
textBuffer.GetCursor().ResetDelayEOLWrap();

auto eraseAttributes = textBuffer.GetCurrentAttributes();
eraseAttributes.SetStandardErase();
_FillRect(textBuffer, { startCol, row, endCol, row + 1 }, L' ', eraseAttributes);
Expand Down Expand Up @@ -721,6 +733,11 @@ bool AdaptDispatch::EraseInDisplay(const DispatchTypes::EraseType eraseType)
const auto row = textBuffer.GetCursor().GetPosition().y;
const auto col = textBuffer.GetCursor().GetPosition().x;

// The ED control is expected to reset the delayed wrap flag.
// The special case variants above ("erase all" and "erase scrollback")
// take care of that themselves when they set the cursor position.
textBuffer.GetCursor().ResetDelayEOLWrap();

auto eraseAttributes = textBuffer.GetCurrentAttributes();
eraseAttributes.SetStandardErase();

Expand Down Expand Up @@ -758,6 +775,9 @@ bool AdaptDispatch::EraseInLine(const DispatchTypes::EraseType eraseType)
const auto row = textBuffer.GetCursor().GetPosition().y;
const auto col = textBuffer.GetCursor().GetPosition().x;

// The EL control is expected to reset the delayed wrap flag.
textBuffer.GetCursor().ResetDelayEOLWrap();

auto eraseAttributes = textBuffer.GetCurrentAttributes();
eraseAttributes.SetStandardErase();
switch (eraseType)
Expand Down Expand Up @@ -822,6 +842,9 @@ bool AdaptDispatch::SelectiveEraseInDisplay(const DispatchTypes::EraseType erase
const auto row = textBuffer.GetCursor().GetPosition().y;
const auto col = textBuffer.GetCursor().GetPosition().x;

// The DECSED control is expected to reset the delayed wrap flag.
textBuffer.GetCursor().ResetDelayEOLWrap();

switch (eraseType)
{
case DispatchTypes::EraseType::FromBeginning:
Expand Down Expand Up @@ -855,6 +878,9 @@ bool AdaptDispatch::SelectiveEraseInLine(const DispatchTypes::EraseType eraseTyp
const auto row = textBuffer.GetCursor().GetPosition().y;
const auto col = textBuffer.GetCursor().GetPosition().x;

// The DECSEL control is expected to reset the delayed wrap flag.
textBuffer.GetCursor().ResetDelayEOLWrap();

switch (eraseType)
{
case DispatchTypes::EraseType::FromBeginning:
Expand Down Expand Up @@ -1606,6 +1632,11 @@ bool AdaptDispatch::_ModeParamsHelper(const DispatchTypes::ModeParams param, con
return true;
case DispatchTypes::ModeParams::DECAWM_AutoWrapMode:
_api.SetAutoWrapMode(enable);
// Resetting DECAWM should also reset the delayed wrap flag.
if (!enable)
{
_api.GetTextBuffer().GetCursor().ResetDelayEOLWrap();
}
return true;
case DispatchTypes::ModeParams::DECARM_AutoRepeatMode:
_terminalInput.SetInputMode(TerminalInput::Mode::AutoRepeat, enable);
Expand Down Expand Up @@ -2097,8 +2128,20 @@ bool AdaptDispatch::ForwardTab(const VTInt numTabs)
}
}

// While the STD 070 reference suggests that horizontal tabs should reset
// the delayed wrap, almost none of the DEC terminals actually worked that
// way, and most modern terminal emulators appear to have taken the same
// approach (i.e. they don't reset). For us this is a bit messy, since all
// cursor movement resets the flag automatically, so we need to save the
// original state here, and potentially reapply it after the move.
const auto delayedWrapOriginallySet = cursor.IsDelayedEOLWrap();
cursor.SetXPosition(column);
_ApplyCursorMovementFlags(cursor);
if (delayedWrapOriginallySet)
{
cursor.DelayEOLWrap();
}

return true;
}

Expand Down
1 change: 1 addition & 0 deletions src/terminal/adapter/adaptDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ namespace Microsoft::Console::VirtualTerminal
{
VTInt Row = 1;
VTInt Column = 1;
bool IsDelayedEOLWrap = false;
bool IsOriginModeRelative = false;
TextAttribute Attributes = {};
TerminalOutput TermOutput = {};
Expand Down