Skip to content

Commit

Permalink
Fix Copy to Clipboard to preserve visual structure of block selection (…
Browse files Browse the repository at this point in the history 
…#8579)

There are two issue with copy to clipboard when block is selected:
* We don't add new lines for lines that were wrapped
* We remove trailing whitespaces which is not intuitive in block selection.

Fixed the copy logic to always add newlines and not to remove
whitespaces when block is selected.

Even if shift is pressed!

## Detailed Description of the Pull Request / Additional comments
* Added optional parameter to `TextBuffer::GetText` 
that allows to apply formatting (includeCRLF / trimming) 
to lines that were wrapped
* Changed `Terminal::RetrieveSelectedTextFromBuffer` 
to apply the following parameters when block is selected:
  * includeCRLF = true
  * trimTrailingWhitespaces = false
  * apply the formatting above to all rows, including the ones 
that were wrapped 

## Validation Steps Performed
* Manual tests for both block and standard selection
* Copy with both right-click and command
* Added UT

Closes #6740
  • Loading branch information
@Don-Vito
Don-Vito committed Dec 14, 2020
1 parent cae0f9a commit a1f42e8
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 40 deletions.
13 changes: 7 additions & 6 deletions src/buffer/out/textBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1512,12 +1512,14 @@ void TextBuffer::_ExpandTextRow(SMALL_RECT& textRow) const
// - trimTrailingWhitespace - remove the trailing whitespace at the end of each line
// - textRects - the rectangular regions from which the data will be extracted from the buffer (i.e.: selection rects)
// - GetAttributeColors - function used to map TextAttribute to RGB COLORREFs. If null, only extract the text.
// - formatWrappedRows - if set we will apply formatting (CRLF inclusion and whitespace trimming) on wrapped rows
// Return Value:
// - The text, background color, and foreground color data of the selected region of the text buffer.
const TextBuffer::TextAndColor TextBuffer::GetText(const bool includeCRLF,
const bool trimTrailingWhitespace,
const std::vector<SMALL_RECT>& selectionRects,
std::function<std::pair<COLORREF, COLORREF>(const TextAttribute&)> GetAttributeColors) const
std::function<std::pair<COLORREF, COLORREF>(const TextAttribute&)> GetAttributeColors,
const bool formatWrappedRows) const
{
TextAndColor data;
const bool copyTextColor = GetAttributeColors != nullptr;
Expand Down Expand Up @@ -1579,12 +1581,12 @@ const TextBuffer::TextAndColor TextBuffer::GetText(const bool includeCRLF,
it++;
}

const bool forcedWrap = GetRowByOffset(iRow).GetCharRow().WasWrapForced();
// We apply formatting to rows if the row was NOT wrapped or formatting of wrapped rows is allowed
const bool shouldFormatRow = formatWrappedRows || !GetRowByOffset(iRow).GetCharRow().WasWrapForced();

if (trimTrailingWhitespace)
{
// if the row was NOT wrapped...
if (!forcedWrap)
if (shouldFormatRow)
{
// remove the spaces at the end (aka trim the trailing whitespace)
while (!selectionText.empty() && selectionText.back() == UNICODE_SPACE)
Expand All @@ -1603,8 +1605,7 @@ const TextBuffer::TextAndColor TextBuffer::GetText(const bool includeCRLF,
// a.k.a if we're earlier than the bottom, then apply CR/LF.
if (includeCRLF && i < selectionRects.size() - 1)
{
// if the row was NOT wrapped...
if (!forcedWrap)
if (shouldFormatRow)
{
// then we can assume a CR/LF is proper
selectionText.push_back(UNICODE_CARRIAGERETURN);
Expand Down
5 changes: 3 additions & 2 deletions src/buffer/out/textBuffer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,11 @@ class TextBuffer final
std::vector<std::vector<COLORREF>> BkAttr;
};

const TextAndColor GetText(const bool lineSelection,
const TextAndColor GetText(const bool includeCRLF,
const bool trimTrailingWhitespace,
const std::vector<SMALL_RECT>& textRects,
std::function<std::pair<COLORREF, COLORREF>(const TextAttribute&)> GetAttributeColors = nullptr) const;
std::function<std::pair<COLORREF, COLORREF>(const TextAttribute&)> GetAttributeColors = nullptr,
const bool formatWrappedRows = false) const;

static std::string GenHTML(const TextAndColor& rows,
const int fontHeightPoints,
Expand Down
11 changes: 8 additions & 3 deletions src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,15 @@ const TextBuffer::TextAndColor Terminal::RetrieveSelectedTextFromBuffer(bool sin

const auto GetAttributeColors = std::bind(&Terminal::GetAttributeColors, this, std::placeholders::_1);

return _buffer->GetText(!singleLine,
!singleLine,
// GH#6740: Block selection should preserve the text block as is:
// - No trailing white-spaces should be removed.
// - CRLFs need to be added - so the lines structure is preserved
// - We should apply formatting above to wrapped rows as well (newline should be added).
return _buffer->GetText(!singleLine || _blockSelection,
!singleLine && !_blockSelection,
selectionRects,
GetAttributeColors);
GetAttributeColors,
_blockSelection);
}

// Method Description:
Expand Down
109 changes: 80 additions & 29 deletions src/host/ut_host/TextBufferTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2471,56 +2471,107 @@ void TextBufferTests::GetText()
// |_____|

// simulate a selection from origin to {4,5}
const auto textRects = _buffer->GetTextRects({ 0, 0 }, { 4, 5 });
const auto textRects = _buffer->GetTextRects({ 0, 0 }, { 4, 5 }, blockSelection);

std::wstring result = L"";
const auto textData = _buffer->GetText(includeCRLF, trimTrailingWhitespace, textRects).text;

const auto formatWrappedRows = blockSelection;
const auto textData = _buffer->GetText(includeCRLF, trimTrailingWhitespace, textRects, nullptr, formatWrappedRows).text;
for (auto& text : textData)
{
result += text;
}

std::wstring expectedText = L"";
if (includeCRLF)
if (formatWrappedRows)
{
if (trimTrailingWhitespace)
if (includeCRLF)
{
Log::Comment(L"Standard Copy to Clipboard");
expectedText += L"12345";
expectedText += L"67\r\n";
expectedText += L" 345\r\n";
expectedText += L"123 \r\n";
if (trimTrailingWhitespace)
{
Log::Comment(L"UNDEFINED");
expectedText += L"12345\r\n";
expectedText += L"67\r\n";
expectedText += L" 345\r\n";
expectedText += L"123\r\n";
expectedText += L"\r\n";
}
else
{
Log::Comment(L"Copy block selection to Clipboard");
expectedText += L"12345\r\n";
expectedText += L"67 \r\n";
expectedText += L" 345\r\n";
expectedText += L"123 \r\n";
expectedText += L" \r\n";
expectedText += L" ";
}
}
else
{
Log::Comment(L"UI Automation");
expectedText += L"12345";
expectedText += L"67 \r\n";
expectedText += L" 345\r\n";
expectedText += L"123 ";
expectedText += L" \r\n";
expectedText += L" ";
if (trimTrailingWhitespace)
{
Log::Comment(L"UNDEFINED");
expectedText += L"12345";
expectedText += L"67";
expectedText += L" 345";
expectedText += L"123";
}
else
{
Log::Comment(L"UNDEFINED");
expectedText += L"12345";
expectedText += L"67 ";
expectedText += L" 345";
expectedText += L"123 ";
expectedText += L" ";
expectedText += L" ";
}
}
}
else
{
if (trimTrailingWhitespace)
if (includeCRLF)
{
Log::Comment(L"UNDEFINED");
expectedText += L"12345";
expectedText += L"67";
expectedText += L" 345";
expectedText += L"123 ";
if (trimTrailingWhitespace)
{
Log::Comment(L"Standard Copy to Clipboard");
expectedText += L"12345";
expectedText += L"67\r\n";
expectedText += L" 345\r\n";
expectedText += L"123 \r\n";
}
else
{
Log::Comment(L"UI Automation");
expectedText += L"12345";
expectedText += L"67 \r\n";
expectedText += L" 345\r\n";
expectedText += L"123 ";
expectedText += L" \r\n";
expectedText += L" ";
}
}
else
{
Log::Comment(L"Shift+Copy to Clipboard");
expectedText += L"12345";
expectedText += L"67 ";
expectedText += L" 345";
expectedText += L"123 ";
expectedText += L" ";
expectedText += L" ";
if (trimTrailingWhitespace)
{
Log::Comment(L"UNDEFINED");
expectedText += L"12345";
expectedText += L"67";
expectedText += L" 345";
expectedText += L"123 ";
}
else
{
Log::Comment(L"Shift+Copy to Clipboard");
expectedText += L"12345";
expectedText += L"67 ";
expectedText += L" 345";
expectedText += L"123 ";
expectedText += L" ";
expectedText += L" ";
}
}
}

Expand Down

0 comments on commit a1f42e8

Please sign in to comment.