Skip to content

Commit

Permalink
Fix DECCRA when copying from a double-width line (#15026)
Browse files Browse the repository at this point in the history
When a `DECCRA` operation is copying content that spans a double width
line, it's possible that some range of the bounding rectangle will be
off-screen, and that range is not supposed to be copied. However, the
code checking for off-screen positions was using incorrect coordinates,
so we would mistakenly copy content that shouldn't be copied, and drop
content that should have been copied. This PR fixes that.

## References and Relevant Issues

This was a regression introduced in PR #14650 when fixing an issue with
horizontal scrolling of DBCS characters.

## Validation Steps Performed

I manually verified this fixes the test case in #15019, and I've also
added a unit test that replicates that case.

Closes #15019
  • Loading branch information
j4james authored Mar 22, 2023
1 parent 2acdc9d commit 7a2e4f8
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 1 deletion.
52 changes: 52 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ class ScreenBufferTests
TEST_METHOD(TestDeferredMainBufferResize);

TEST_METHOD(RectangularAreaOperations);
TEST_METHOD(CopyDoubleWidthRectangularArea);

TEST_METHOD(DelayedWrapReset);
};
Expand Down Expand Up @@ -7548,6 +7549,57 @@ void ScreenBufferTests::RectangularAreaOperations()
VERIFY_IS_TRUE(_ValidateLinesContain(targetArea.right, targetArea.top, targetArea.bottom, bufferChar, bufferAttr));
}

void ScreenBufferTests::CopyDoubleWidthRectangularArea()
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
auto& stateMachine = si.GetStateMachine();
auto& textBuffer = si.GetTextBuffer();
WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);

// Fill the entire buffer with Zs. Blue on Green and Underlined.
const auto bufferChar = L'Z';
const auto bufferHeight = si.GetBufferSize().Height();
auto bufferAttr = TextAttribute{ FOREGROUND_BLUE | BACKGROUND_GREEN };
bufferAttr.SetUnderlined(true);
_FillLines(0, bufferHeight, bufferChar, bufferAttr);

// Fill the first three lines with Cs. Green on Red and Intense.
const auto copyChar = L'C';
auto copyAttr = TextAttribute{ FOREGROUND_GREEN | BACKGROUND_RED };
copyAttr.SetIntense(true);
_FillLines(0, 3, copyChar, copyAttr);

// Set the active attributes to Red on Blue;
auto activeAttr = TextAttribute{ FOREGROUND_RED | BACKGROUND_BLUE };
si.SetAttributes(activeAttr);

// Make the second line (offset 1) double width.
textBuffer.GetCursor().SetPosition({ 0, 1 });
textBuffer.SetCurrentLineRendition(LineRendition::DoubleWidth);

// Copy a segment of the top three lines with DECCRA.
stateMachine.ProcessString(L"\033[1;31;3;50;1;4;31;1$v");

Log::Comment(L"The first 30 columns of the target area should remain unchanged");
const auto thirtyBufferChars = std::wstring(30, bufferChar);
VERIFY_IS_TRUE(_ValidateLinesContain(3, 6, thirtyBufferChars, bufferAttr));

Log::Comment(L"The next 20 columns should contain the copied content");
const auto twentyCopyChars = std::wstring(20, copyChar);
VERIFY_IS_TRUE(_ValidateLineContains({ 30, 3 }, twentyCopyChars, copyAttr));
VERIFY_IS_TRUE(_ValidateLineContains({ 30, 5 }, twentyCopyChars, copyAttr));

Log::Comment(L"But the second target row ends after 10, because of the double-width source");
const auto tenCopyChars = std::wstring(10, copyChar);
VERIFY_IS_TRUE(_ValidateLineContains({ 30, 4 }, tenCopyChars, copyAttr));

Log::Comment(L"The subsequent columns in each row should remain unchanged");
VERIFY_IS_TRUE(_ValidateLineContains({ 50, 3 }, bufferChar, bufferAttr));
VERIFY_IS_TRUE(_ValidateLineContains({ 40, 4 }, bufferChar, bufferAttr));
VERIFY_IS_TRUE(_ValidateLineContains({ 50, 5 }, bufferChar, bufferAttr));
}

void ScreenBufferTests::DelayedWrapReset()
{
BEGIN_TEST_METHOD_PROPERTIES()
Expand Down
3 changes: 2 additions & 1 deletion src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1153,11 +1153,12 @@ bool AdaptDispatch::CopyRectangularArea(const VTInt top, const VTInt left, const
do
{
const auto current = next;
const auto currentSrcPos = srcPos;
srcView.WalkInBounds(srcPos, walkDirection);
next = OutputCell(*textBuffer.GetCellDataAt(srcPos));
// If the source position is offscreen (which can occur on double
// width lines), then we shouldn't copy anything to the destination.
if (srcPos.x < textBuffer.GetLineWidth(srcPos.y))
if (currentSrcPos.x < textBuffer.GetLineWidth(currentSrcPos.y))
{
textBuffer.WriteLine(OutputCellIterator({ &current, 1 }), dstPos);
}
Expand Down

0 comments on commit 7a2e4f8

Please sign in to comment.