Skip to content

Commit

Permalink
When inserting/deleting lines, preserve RGB/256 attributes (#2668)
Browse files Browse the repository at this point in the history 
* This fixes #832 by not mucking with roundtripping attributes. Still needs a test

* Add a test

* Lets just make this test test everything

  @miniksa https://media0.giphy.com/media/d7mMzaGDYkz4ZBziP6/giphy.gif

* Remove dead code
  • Loading branch information
@zadjii-msft
zadjii-msft authored and msftbot[bot] committed Sep 9, 2019
1 parent ce34c73 commit bac69f7
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 6 deletions.
25 changes: 19 additions & 6 deletions src/host/getset.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2055,12 +2055,25 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert)
SMALL_RECT srClip = screenEdges;
srClip.Top = cursorPosition.Y;

LOG_IF_FAILED(ServiceLocator::LocateGlobals().api.ScrollConsoleScreenBufferWImpl(screenInfo,
srScroll,
coordDestination,
srClip,
UNICODE_SPACE,
screenInfo.GetAttributes().GetLegacyAttributes()));
// Here we previously called to ScrollConsoleScreenBufferWImpl to
// perform the scrolling operation. However, that function only accepts
// a WORD for the fill attributes. That means we'd lose 256/RGB fidelity
// for fill attributes. So instead, we'll just call ScrollRegion
// ourselves, with the same params that ScrollConsoleScreenBufferWImpl
// would have.
// See microsoft/terminal#832 for more context.
try
{
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
ScrollRegion(screenInfo,
srScroll,
srClip,
coordDestination,
UNICODE_SPACE,
screenInfo.GetAttributes());
}
CATCH_LOG();
}
}

Expand Down
99 changes: 99 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ class ScreenBufferTests
TEST_METHOD(DeleteLinesInMargins);
TEST_METHOD(ReverseLineFeedInMargins);

TEST_METHOD(InsertDeleteLines256Colors);

TEST_METHOD(SetOriginMode);

TEST_METHOD(HardResetBuffer);
Expand Down Expand Up @@ -3453,6 +3455,103 @@ void ScreenBufferTests::ReverseLineFeedInMargins()
}
}

void ScreenBufferTests::InsertDeleteLines256Colors()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:insert", L"{false, true}")
TEST_METHOD_PROPERTY(L"Data:colorStyle", L"{0, 1, 2}")
END_TEST_METHOD_PROPERTIES();

// colorStyle will be used to control whether we use a color from the 16
// color table, a color from the 256 color table, or a pure RGB color.
const int Use16Color = 0;
const int Use256Color = 1;
const int UseRGBColor = 2;

bool insert;
int colorStyle;
VERIFY_SUCCEEDED(TestData::TryGetValue(L"insert", insert), L"whether to insert(true) or delete(false) lines");
VERIFY_SUCCEEDED(TestData::TryGetValue(L"colorStyle", colorStyle), L"controls whether to use the 16 color table, 256 table, or RGB colors");

// This test is largely taken from repro code from
// https://github.com/microsoft/terminal/issues/832#issuecomment-507447272
Log::Comment(
L"Sets the attributes to a 256/RGB color, then scrolls some lines with"
L" DL. Verifies the rows are cleared with the attributes we'd expect.");

auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& tbi = si.GetTextBuffer();
auto& stateMachine = si.GetStateMachine();
auto& cursor = si.GetTextBuffer().GetCursor();

TextAttribute expectedAttr{ si.GetAttributes() };
std::wstring sgrSeq = L"\x1b[48;5;2m";
if (colorStyle == Use16Color)
{
expectedAttr.SetBackground(gci.GetColorTableEntry(2));
}
else if (colorStyle == Use256Color)
{
expectedAttr.SetBackground(gci.GetColorTableEntry(20));
sgrSeq = L"\x1b[48;5;20m";
}
else if (colorStyle == UseRGBColor)
{
expectedAttr.SetBackground(RGB(1, 2, 3));
sgrSeq = L"\x1b[48;2;1;2;3m";
}

// Set some scrolling margins
stateMachine.ProcessString(L"\x1b[1;3r");

// Set the BG color to the table index 2, as a 256-color sequence
stateMachine.ProcessString(sgrSeq);

VERIFY_ARE_EQUAL(expectedAttr, si.GetAttributes());

// Move to home
stateMachine.ProcessString(L"\x1b[H");

// Insert/Delete 10 lines
stateMachine.ProcessString(insert ? L"\x1b[10L" : L"\x1b[10M");

Log::Comment(NoThrowString().Format(
L"cursor=%s", VerifyOutputTraits<COORD>::ToString(cursor.GetPosition()).GetBuffer()));
Log::Comment(NoThrowString().Format(
L"viewport=%s", VerifyOutputTraits<SMALL_RECT>::ToString(si.GetViewport().ToInclusive()).GetBuffer()));

VERIFY_ARE_EQUAL(0, cursor.GetPosition().X);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y);

stateMachine.ProcessString(L"foo");
Log::Comment(NoThrowString().Format(
L"cursor=%s", VerifyOutputTraits<COORD>::ToString(cursor.GetPosition()).GetBuffer()));
VERIFY_ARE_EQUAL(3, cursor.GetPosition().X);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y);
{
auto iter00 = tbi.GetCellDataAt({ 0, 0 });
auto iter10 = tbi.GetCellDataAt({ 1, 0 });
auto iter20 = tbi.GetCellDataAt({ 2, 0 });
auto iter30 = tbi.GetCellDataAt({ 3, 0 });
auto iter01 = tbi.GetCellDataAt({ 0, 1 });
auto iter02 = tbi.GetCellDataAt({ 0, 2 });
VERIFY_ARE_EQUAL(L"f", iter00->Chars());
VERIFY_ARE_EQUAL(L"o", iter10->Chars());
VERIFY_ARE_EQUAL(L"o", iter20->Chars());
VERIFY_ARE_EQUAL(L"\x20", iter30->Chars());
VERIFY_ARE_EQUAL(L"\x20", iter01->Chars());
VERIFY_ARE_EQUAL(L"\x20", iter02->Chars());

VERIFY_ARE_EQUAL(expectedAttr, iter00->TextAttr());
VERIFY_ARE_EQUAL(expectedAttr, iter10->TextAttr());
VERIFY_ARE_EQUAL(expectedAttr, iter20->TextAttr());
VERIFY_ARE_EQUAL(expectedAttr, iter30->TextAttr());
VERIFY_ARE_EQUAL(expectedAttr, iter01->TextAttr());
VERIFY_ARE_EQUAL(expectedAttr, iter02->TextAttr());
}
}

void ScreenBufferTests::SetOriginMode()
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Expand Down

0 comments on commit bac69f7

Please sign in to comment.