Skip to content

Commit

Permalink
Bugfix: CLS should clear current active buffer (#2729)
Browse files Browse the repository at this point in the history 
CLS calls two functions: 
- `SetConsoleCursorPositionImpl()`
- `ScrollConsoleScreenBufferWImpl()`
Both of these were not checking which buffer to apply to (main vs active buffer).

Now we get the active buffer and apply the changes to that one.

Also, we forgot to switch out of the alt buffer in the previous test. Added that in.

Closes #1189.
  • Loading branch information
@carlos-zamora @DHowett
carlos-zamora authored and DHowett committed Sep 13, 2019
1 parent b693fd4 commit 3d35e39
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 7 deletions.
19 changes: 12 additions & 7 deletions src/host/getset.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,9 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont

CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

const COORD coordScreenBufferSize = context.GetBufferSize().Dimensions();
auto& buffer = context.GetActiveBuffer();

const COORD coordScreenBufferSize = buffer.GetBufferSize().Dimensions();
// clang-format off
RETURN_HR_IF(E_INVALIDARG, (position.X >= coordScreenBufferSize.X ||
position.Y >= coordScreenBufferSize.Y ||
Expand All @@ -643,15 +645,15 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
// MSFT: 15813316 - Try to use this SetCursorPosition call to inherit the cursor position.
RETURN_IF_FAILED(gci.GetVtIo()->SetCursorPosition(position));

RETURN_IF_NTSTATUS_FAILED(context.SetCursorPosition(position, true));
RETURN_IF_NTSTATUS_FAILED(buffer.SetCursorPosition(position, true));

LOG_IF_FAILED(ConsoleImeResizeCompStrView());

COORD WindowOrigin;
WindowOrigin.X = 0;
WindowOrigin.Y = 0;
{
const SMALL_RECT currentViewport = context.GetViewport().ToInclusive();
const SMALL_RECT currentViewport = buffer.GetViewport().ToInclusive();
if (currentViewport.Left > position.X)
{
WindowOrigin.X = position.X - currentViewport.Left;
Expand All @@ -671,7 +673,7 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
}
}

RETURN_IF_NTSTATUS_FAILED(context.SetViewportOrigin(false, WindowOrigin, true));
RETURN_IF_NTSTATUS_FAILED(buffer.SetViewportOrigin(false, WindowOrigin, true));

return S_OK;
}
Expand Down Expand Up @@ -820,6 +822,8 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

auto& buffer = context.GetActiveBuffer();

TextAttribute useThisAttr(fillAttribute);

// Here we're being a little clever - similar to FillConsoleOutputAttributeImpl
Expand All @@ -835,18 +839,19 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
// this scenario is highly unlikely, and we can reasonably do this
// on their behalf.
// see MSFT:19853701
if (context.InVTMode())

if (buffer.InVTMode())
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
const auto currentAttributes = context.GetAttributes();
const auto currentAttributes = buffer.GetAttributes();
const auto bufferLegacy = gci.GenerateLegacyAttributes(currentAttributes);
if (bufferLegacy == fillAttribute)
{
useThisAttr = currentAttributes;
}
}

ScrollRegion(context, source, clip, target, fillCharacter, useThisAttr);
ScrollRegion(buffer, source, clip, target, fillCharacter, useThisAttr);

return S_OK;
}
Expand Down
92 changes: 92 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ class ScreenBufferTests
TEST_METHOD(HardResetBuffer);

TEST_METHOD(RestoreDownAltBufferWithTerminalScrolling);

TEST_METHOD(ClearAlternateBuffer);
};

void ScreenBufferTests::SingleAlternateBufferCreationTest()
Expand Down Expand Up @@ -4219,6 +4221,8 @@ void ScreenBufferTests::RestoreDownAltBufferWithTerminalScrolling()
VERIFY_ARE_EQUAL(0, altBuffer._viewport.Top());
VERIFY_ARE_EQUAL(altBuffer._viewport.BottomInclusive(), altBuffer._virtualBottom);

auto useMain = wil::scope_exit([&] { altBuffer.UseMainScreenBuffer(); });

const COORD originalSize = originalView.Dimensions();
const COORD doubledSize = { originalSize.X * 2, originalSize.Y * 2 };

Expand Down Expand Up @@ -4255,3 +4259,91 @@ void ScreenBufferTests::RestoreDownAltBufferWithTerminalScrolling()
VERIFY_ARE_EQUAL(altBuffer._viewport.BottomInclusive(), altBuffer._virtualBottom);
}
}

void ScreenBufferTests::ClearAlternateBuffer()
{
// This is a test for microsoft/terminal#1189. Refer to that issue for more
// context

CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& g = ServiceLocator::LocateGlobals();
gci.LockConsole(); // Lock must be taken to manipulate buffer.
auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });

auto& siMain = gci.GetActiveOutputBuffer();
auto WriteText = [&](TextBuffer& tbi) {
// Write text to buffer
auto& stateMachine = siMain.GetStateMachine();
auto& cursor = tbi.GetCursor();
stateMachine.ProcessString(L"foo\nfoo");
VERIFY_ARE_EQUAL(cursor.GetPosition().X, 3);
VERIFY_ARE_EQUAL(cursor.GetPosition().Y, 1);
};

auto VerifyText = [&](TextBuffer& tbi) {
// Verify written text in buffer
{
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({ 1, 1 });
auto iter03 = tbi.GetCellDataAt({ 2, 1 });
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"f", iter01->Chars());
VERIFY_ARE_EQUAL(L"o", iter02->Chars());
VERIFY_ARE_EQUAL(L"o", iter03->Chars());
}
};

WriteText(siMain.GetTextBuffer());
VerifyText(siMain.GetTextBuffer());

Log::Comment(L"Create an alternate buffer");
if (VERIFY_IS_TRUE(NT_SUCCESS(siMain.UseAlternateScreenBuffer())))
{
VERIFY_IS_NOT_NULL(siMain._psiAlternateBuffer);
auto& altBuffer = *siMain._psiAlternateBuffer;
VERIFY_ARE_EQUAL(0, altBuffer._viewport.Top());
VERIFY_ARE_EQUAL(altBuffer._viewport.BottomInclusive(), altBuffer._virtualBottom);

auto useMain = wil::scope_exit([&] { altBuffer.UseMainScreenBuffer(); });

WriteText(altBuffer.GetTextBuffer());
VerifyText(altBuffer.GetTextBuffer());

#pragma region Test ScrollConsoleScreenBufferWImpl()
// Clear text of alt buffer (same params as in CMD)
VERIFY_SUCCEEDED(g.api.ScrollConsoleScreenBufferWImpl(siMain,
{ 0, 0, 120, 9001 },
{ 0, -9001 },
std::nullopt,
L' ',
7));

// Verify text is now gone
VERIFY_ARE_EQUAL(L" ", altBuffer.GetTextBuffer().GetCellDataAt({ 0, 0 })->Chars());
#pragma endregion

#pragma region Test SetConsoleCursorPositionImpl()
// Reset cursor position as we do with CLS command (same params as in CMD)
VERIFY_SUCCEEDED(g.api.SetConsoleCursorPositionImpl(siMain, { 0 }));

// Verify state of alt buffer
auto& altBufferCursor = altBuffer.GetTextBuffer().GetCursor();
VERIFY_ARE_EQUAL(altBufferCursor.GetPosition().X, 0);
VERIFY_ARE_EQUAL(altBufferCursor.GetPosition().Y, 0);
#pragma endregion
}

// Verify state of main buffer is untouched
auto& cursor = siMain.GetTextBuffer().GetCursor();
VERIFY_ARE_EQUAL(cursor.GetPosition().X, 3);
VERIFY_ARE_EQUAL(cursor.GetPosition().Y, 1);

VerifyText(siMain.GetTextBuffer());
}

0 comments on commit 3d35e39

Please sign in to comment.