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.

Sign up for GitHub

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

Jump to bottom

Make the alt buffer inherit cursor state from the main buffer #10843

Merged
5 commits merged into from
Aug 2, 2021
Merged

Make the alt buffer inherit cursor state from the main buffer #10843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a062838
Inherit main cursor position in the alt buffer.
j4james Aug 1, 2021
b5d9341
Inherit main cursor visibility in the alt buffer.
j4james Aug 1, 2021
0604d8f
Copy alt cursor style and visibility back to main buffer.
j4james Aug 1, 2021
75a76b7
Fix test that relied on alt cursor being set to home.
j4james Aug 1, 2021
044efd4
Add unit tests confirming alt buffer cursor inheritance.
j4james Aug 1, 2021
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
18 changes: 16 additions & 2 deletions src/host/screenInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1904,10 +1904,17 @@ const SCREEN_INFORMATION& SCREEN_INFORMATION::GetMainBuffer() const
ppsiNewScreenBuffer);
if (NT_SUCCESS(Status))
{
// Update the alt buffer's cursor style to match our own.
// Update the alt buffer's cursor style, visibility, and position to match our own.
auto& myCursor = GetTextBuffer().GetCursor();
auto* const createdBuffer = *ppsiNewScreenBuffer;
createdBuffer->GetTextBuffer().GetCursor().SetStyle(myCursor.GetSize(), myCursor.GetColor(), myCursor.GetType());
auto& altCursor = createdBuffer->GetTextBuffer().GetCursor();
altCursor.SetStyle(myCursor.GetSize(), myCursor.GetColor(), myCursor.GetType());
altCursor.SetIsVisible(myCursor.IsVisible());
altCursor.SetBlinkingAllowed(myCursor.IsBlinkingAllowed());
// The new position should match the viewport-relative position of the main buffer.
auto altCursorPos = myCursor.GetPosition();
altCursorPos.Y -= GetVirtualViewport().Top();
altCursor.SetPosition(altCursorPos);

s_InsertScreenBuffer(createdBuffer);

Expand Down Expand Up @@ -1998,6 +2005,13 @@ void SCREEN_INFORMATION::UseMainScreenBuffer()
s_RemoveScreenBuffer(psiAlt); // this will also delete the alt buffer
Copy link
Member

Choose a reason for hiding this comment

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

I suspect the issue @lhecker found stems from the deletion of psiAlt here before we establish a reference to its cursor :)

Copy link
Collaborator Author

@j4james j4james Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yikes. Sorry guys. I don't know why I thought it was a good idea to add that code after the line that clearly says it deletes the alt buffer. Will try and fix ASAP.

Funnily enough I can't actually reproduce the crash with cmatrix or cacafire, but maybe it only crashes in a debug build?

Edit: I didn't eventually get to test a debug build and was able to reproduce the crash. PR fix is #10878.

// deleting the alt buffer will give the GetSet back to its main

// Copy the alt buffer's cursor style and visibility back to the main buffer.
const auto& altCursor = psiAlt->GetTextBuffer().GetCursor();
auto& mainCursor = psiMain->GetTextBuffer().GetCursor();
mainCursor.SetStyle(altCursor.GetSize(), altCursor.GetColor(), altCursor.GetType());
mainCursor.SetIsVisible(altCursor.IsVisible());
mainCursor.SetBlinkingAllowed(altCursor.IsBlinkingAllowed());

// Tell the VT MouseInput handler that we're in the main buffer now
gci.GetActiveInputBuffer()->GetTerminalInput().UseMainScreenBuffer();
}
Expand Down
70 changes: 70 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ class ScreenBufferTests

TEST_METHOD(MultipleAlternateBuffersFromMainCreationTest);

TEST_METHOD(AlternateBufferCursorInheritanceTest);

TEST_METHOD(TestReverseLineFeed);

TEST_METHOD(TestResetClearTabStops);
Expand Down Expand Up @@ -344,6 +346,71 @@ void ScreenBufferTests::MultipleAlternateBuffersFromMainCreationTest()
}
}

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

auto& mainBuffer = gci.GetActiveOutputBuffer();
auto& mainCursor = mainBuffer.GetTextBuffer().GetCursor();

Log::Comment(L"Set the cursor attributes in the main buffer.");
auto mainCursorPos = COORD{ 3, 5 };
auto mainCursorVisible = false;
auto mainCursorSize = 33u;
auto mainCursorColor = RGB(1, 2, 3);
auto mainCursorType = CursorType::DoubleUnderscore;
auto mainCursorBlinking = false;
mainCursor.SetPosition(mainCursorPos);
mainCursor.SetIsVisible(mainCursorVisible);
mainCursor.SetStyle(mainCursorSize, mainCursorColor, mainCursorType);
mainCursor.SetBlinkingAllowed(mainCursorBlinking);

Log::Comment(L"Switch to the alternate buffer.");
VERIFY_SUCCEEDED(mainBuffer.UseAlternateScreenBuffer());
auto& altBuffer = gci.GetActiveOutputBuffer();
auto& altCursor = altBuffer.GetTextBuffer().GetCursor();
auto useMain = wil::scope_exit([&] { altBuffer.UseMainScreenBuffer(); });

Log::Comment(L"Confirm the cursor position is inherited from the main buffer.");
VERIFY_ARE_EQUAL(mainCursorPos, altCursor.GetPosition());
Log::Comment(L"Confirm the cursor visibility is inherited from the main buffer.");
VERIFY_ARE_EQUAL(mainCursorVisible, altCursor.IsVisible());
Log::Comment(L"Confirm the cursor style is inherited from the main buffer.");
VERIFY_ARE_EQUAL(mainCursorSize, altCursor.GetSize());
VERIFY_ARE_EQUAL(mainCursorColor, altCursor.GetColor());
VERIFY_ARE_EQUAL(mainCursorType, altCursor.GetType());
VERIFY_ARE_EQUAL(mainCursorBlinking, altCursor.IsBlinkingAllowed());

Log::Comment(L"Set the cursor attributes in the alt buffer.");
auto altCursorPos = COORD{ 5, 3 };
auto altCursorVisible = true;
auto altCursorSize = 66u;
auto altCursorColor = RGB(3, 2, 1);
auto altCursorType = CursorType::EmptyBox;
auto altCursorBlinking = true;
altCursor.SetPosition(altCursorPos);
altCursor.SetIsVisible(altCursorVisible);
altCursor.SetStyle(altCursorSize, altCursorColor, altCursorType);
altCursor.SetBlinkingAllowed(altCursorBlinking);

Log::Comment(L"Switch back to the main buffer.");
useMain.release();
altBuffer.UseMainScreenBuffer();
VERIFY_ARE_EQUAL(&mainBuffer, &gci.GetActiveOutputBuffer());

Log::Comment(L"Confirm the cursor position is restored to what it was.");
VERIFY_ARE_EQUAL(mainCursorPos, mainCursor.GetPosition());
Log::Comment(L"Confirm the cursor visibility is inherited from the alt buffer.");
VERIFY_ARE_EQUAL(altCursorVisible, mainCursor.IsVisible());
Log::Comment(L"Confirm the cursor style is inherited from the alt buffer.");
VERIFY_ARE_EQUAL(altCursorSize, mainCursor.GetSize());
VERIFY_ARE_EQUAL(altCursorColor, mainCursor.GetColor());
VERIFY_ARE_EQUAL(altCursorType, mainCursor.GetType());
VERIFY_ARE_EQUAL(altCursorBlinking, mainCursor.IsBlinkingAllowed());
}

void ScreenBufferTests::TestReverseLineFeed()
{
CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
Expand Down Expand Up @@ -4948,6 +5015,9 @@ void ScreenBufferTests::ClearAlternateBuffer()

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

// Set the position to home, otherwise it's inherited from the main buffer.
VERIFY_SUCCEEDED(altBuffer.SetCursorPosition({ 0, 0 }, true));

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

Expand Down