Skip to content

Commit

Permalink
make filling chars (and, thus, erase line/char) unset wrap (#2831)
Browse files Browse the repository at this point in the history
EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with
chars, we were setting the wrap flag. We need to specifically not do this on
ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to
the end of the line.

Originally, we had a boolean `setWrap` that would mean...
- **true**: if writing to the end of the row, SET the wrap value to true
- **false**: if writing to the end of the row, DON'T CHANGE the wrap value

Now we're making this bool a std::optional to allow for a ternary state. This
allows for us to handle the following cases completely. Refer to the table
below:

,- current wrap value
|     ,- are we filling the last cell in the row?
|     |     ,- new wrap value
|     |     |     ,- comments
|--   |--   |--   |
| 0   | 0   | 0   |
| 0   | 1   | 0   |
| 0   | 1   | 1   | THIS CASE WAS HANDLED CORRECTLY
| 1   | 0   | 0   | THIS CASE WAS UNHANDLED
| 1   | 0   | 1   |
| 1   | 1   | 1   |

To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have
~setWrap~ `wrap` mean the following:
- **true**: if writing to the end of the row, SET the wrap value to TRUE
- **false**: if writing to the end of the row, SET the wrap value to FALSE
- **nullopt**: leave the wrap value as it is

Closes #1126
  • Loading branch information
carlos-zamora authored and DHowett committed Oct 3, 2019
1 parent 9461df9 commit f081a95
Show file tree
Hide file tree
Showing 9 changed files with 228 additions and 17 deletions.
15 changes: 10 additions & 5 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,11 @@ const UnicodeStorage& ROW::GetUnicodeStorage() const noexcept
// Arguments:
// - it - custom console iterator to use for seeking input data. bool() false when it becomes invalid while seeking.
// - index - column in row to start writing at
// - setWrap - set the wrap flags if we hit the end of the row while writing and there's still more data in the iterator.
// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data in the iterator.
// - limitRight - right inclusive column ID for the last write in this row. (optional, will just write to the end of row if nullopt)
// Return Value:
// - iterator to first cell that was not written to this row.
OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, const bool setWrap, std::optional<size_t> limitRight)
OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, const std::optional<bool> wrap, std::optional<size_t> limitRight)
{
THROW_HR_IF(E_INVALIDARG, index >= _charRow.size());
THROW_HR_IF(E_INVALIDARG, limitRight.value_or(0) >= _charRow.size());
Expand Down Expand Up @@ -202,10 +202,15 @@ OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, co
++it;
}

// If we're asked to set the wrap status and we just filled the last column with some text, set wrap status on the row.
if (setWrap && fillingLastColumn)
// If we're asked to (un)set the wrap status and we just filled the last column with some text...
// NOTE:
// - wrap = std::nullopt --> don't change the wrap value
// - wrap = true --> we're filling cells as a steam, consider this a wrap
// - wrap = false --> we're filling cells as a block, unwrap
if (wrap.has_value() && fillingLastColumn)
{
_charRow.SetWrapForced(true);
// set wrap status on the row to parameter's value.
_charRow.SetWrapForced(wrap.value());
}
}
else
Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class ROW final
UnicodeStorage& GetUnicodeStorage() noexcept;
const UnicodeStorage& GetUnicodeStorage() const noexcept;

OutputCellIterator WriteCells(OutputCellIterator it, const size_t index, const bool setWrap, std::optional<size_t> limitRight = std::nullopt);
OutputCellIterator WriteCells(OutputCellIterator it, const size_t index, const std::optional<bool> wrap = std::nullopt, std::optional<size_t> limitRight = std::nullopt);

friend bool operator==(const ROW& a, const ROW& b) noexcept;

Expand Down
13 changes: 8 additions & 5 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -318,10 +318,12 @@ OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt)
// Arguments:
// - givenIt - Iterator representing output cell data to write
// - target - the row/column to start writing the text to
// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data
// Return Value:
// - The final position of the iterator
OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt,
const COORD target)
const COORD target,
const std::optional<bool> wrap)
{
// Make mutable copy so we can walk.
auto it = givenIt;
Expand All @@ -336,7 +338,8 @@ OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt,
while (it && size.IsInBounds(lineTarget))
{
// Attempt to write as much data as possible onto this line.
it = WriteLine(it, lineTarget, true);
// NOTE: if wrap = true/false, we want to set the line's wrap to true/false (respectively) if we reach the end of the line
it = WriteLine(it, lineTarget, wrap);

// Move to the next line down.
lineTarget.X = 0;
Expand All @@ -351,13 +354,13 @@ OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt,
// Arguments:
// - givenIt - The iterator that will dereference into cell data to insert
// - target - Coordinate targeted within output buffer
// - setWrap - Whether we should try to set the wrap flag if we write up to the end of the line and have more data
// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data in the iterator.
// - limitRight - Optionally restrict the right boundary for writing (e.g. stop writing earlier than the end of line)
// Return Value:
// - The iterator, but advanced to where we stopped writing. Use to find input consumed length or cells written length.
OutputCellIterator TextBuffer::WriteLine(const OutputCellIterator givenIt,
const COORD target,
const bool setWrap,
const std::optional<bool> wrap,
std::optional<size_t> limitRight)
{
// If we're not in bounds, exit early.
Expand All @@ -368,7 +371,7 @@ OutputCellIterator TextBuffer::WriteLine(const OutputCellIterator givenIt,

// Get the row and write the cells
ROW& row = GetRowByOffset(target.Y);
const auto newIt = row.WriteCells(givenIt, target.X, setWrap, limitRight);
const auto newIt = row.WriteCells(givenIt, target.X, wrap, limitRight);

// Take the cell distance written and notify that it needs to be repainted.
const auto written = newIt.GetCellDistance(givenIt);
Expand Down
5 changes: 3 additions & 2 deletions src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,12 @@ class TextBuffer final
OutputCellIterator Write(const OutputCellIterator givenIt);

OutputCellIterator Write(const OutputCellIterator givenIt,
const COORD target);
const COORD target,
const std::optional<bool> wrap = true);

OutputCellIterator WriteLine(const OutputCellIterator givenIt,
const COORD target,
const bool setWrap = false,
const std::optional<bool> setWrap = std::nullopt,
const std::optional<size_t> limitRight = std::nullopt);

bool InsertCharacter(const wchar_t wch, const DbcsAttribute dbcsAttribute, const TextAttribute attr);
Expand Down
5 changes: 4 additions & 1 deletion src/host/_output.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,10 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
try
{
const OutputCellIterator it(character, lengthToWrite);
const auto done = screenInfo.Write(it, startingCoordinate);

// when writing to the buffer, specifically unset wrap if we get to the last column.
// a fill operation should UNSET wrap in that scenario. See GH #1126 for more details.
const auto done = screenInfo.Write(it, startingCoordinate, false);
cellsModified = done.GetInputDistance(it);

// Notify accessibility
Expand Down
121 changes: 121 additions & 0 deletions src/host/ft_host/API_FillOutputTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,125 @@ class FillOutputTests
&charsWritten));
VERIFY_ARE_EQUAL(1u, charsWritten);
}

TEST_METHOD(UnsetWrap)
{
// WARNING: If this test suddenly decides to start failing,
// this is because the wrap registry key is not set.
// TODO GH #2859: Get/Set Registry Key for Wrap

HANDLE hConsole = GetStdOutputHandle();
DWORD charsWritten = 0;

CONSOLE_SCREEN_BUFFER_INFOEX sbiex = { 0 };
sbiex.cbSize = sizeof(sbiex);
VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleScreenBufferInfoEx(hConsole, &sbiex));

const auto consoleWidth = sbiex.dwSize.X;

std::wstring input(consoleWidth + 2, L'a');
std::wstring filled(consoleWidth, L'b');

// Write until a wrap occurs
VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleW(hConsole,
input.data(),
gsl::narrow_cast<DWORD>(input.size()),
&charsWritten,
nullptr));

// Verify wrap occurred
std::unique_ptr<wchar_t[]> bufferText = std::make_unique<wchar_t[]>(consoleWidth);
DWORD readSize = 0;
VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole,
bufferText.get(),
consoleWidth,
{ 0, 0 },
&readSize));

WEX::Common::String expected(input.c_str(), readSize);
WEX::Common::String actual(bufferText.get(), readSize);
VERIFY_ARE_EQUAL(expected, actual);

bufferText = std::make_unique<wchar_t[]>(2);
VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole,
bufferText.get(),
2,
{ 0, 1 },
&readSize));

VERIFY_ARE_EQUAL(2u, readSize);
expected = WEX::Common::String(input.c_str(), readSize);
actual = WEX::Common::String(bufferText.get(), readSize);
VERIFY_ARE_EQUAL(expected, actual);

// Fill Console Line with 'b's
VERIFY_WIN32_BOOL_SUCCEEDED(FillConsoleOutputCharacterW(hConsole,
L'b',
consoleWidth,
{ 2, 0 },
&charsWritten));

// Verify first line is full of 'a's then 'b's
bufferText = std::make_unique<wchar_t[]>(consoleWidth);
VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole,
bufferText.get(),
consoleWidth,
{ 0, 0 },
&readSize));

expected = WEX::Common::String(input.c_str(), 2);
actual = WEX::Common::String(bufferText.get(), 2);
VERIFY_ARE_EQUAL(expected, actual);

expected = WEX::Common::String(filled.c_str(), consoleWidth - 2);
actual = WEX::Common::String(&bufferText[2], readSize - 2);
VERIFY_ARE_EQUAL(expected, actual);

// Verify second line is still has 'a's that wrapped over
bufferText = std::make_unique<wchar_t[]>(2);
VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole,
bufferText.get(),
static_cast<SHORT>(2),
{ 0, 0 },
&readSize));

VERIFY_ARE_EQUAL(2u, readSize);
expected = WEX::Common::String(input.c_str(), 2);
actual = WEX::Common::String(bufferText.get(), readSize);
VERIFY_ARE_EQUAL(expected, actual);

// Resize to be smaller by 2
sbiex.srWindow.Right -= 2;
sbiex.dwSize.X -= 2;
VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleScreenBufferInfoEx(hConsole, &sbiex));

// Verify first line is full of 'a's then 'b's
bufferText = std::make_unique<wchar_t[]>(consoleWidth - 2);
VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole,
bufferText.get(),
consoleWidth - static_cast<SHORT>(2),
{ 0, 0 },
&readSize));

expected = WEX::Common::String(input.c_str(), 2);
actual = WEX::Common::String(bufferText.get(), 2);
VERIFY_ARE_EQUAL(expected, actual);

expected = WEX::Common::String(filled.c_str(), consoleWidth - 4);
actual = WEX::Common::String(&bufferText[2], readSize - 2);
VERIFY_ARE_EQUAL(expected, actual);

// Verify second line is still has 'a's ('b's didn't wrap over)
bufferText = std::make_unique<wchar_t[]>(static_cast<SHORT>(2));
VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole,
bufferText.get(),
static_cast<SHORT>(2),
{ 0, 0 },
&readSize));

VERIFY_ARE_EQUAL(2u, readSize);
expected = WEX::Common::String(input.c_str(), 2);
actual = WEX::Common::String(bufferText.get(), readSize);
VERIFY_ARE_EQUAL(expected, actual);
}
};
7 changes: 5 additions & 2 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2599,14 +2599,17 @@ OutputCellIterator SCREEN_INFORMATION::Write(const OutputCellIterator it)
// Arguments:
// - it - Iterator representing output cell data to write.
// - target - The position to start writing at
// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data
// Return Value:
// - the iterator at its final position
// Note:
// - will throw exception on error.
OutputCellIterator SCREEN_INFORMATION::Write(const OutputCellIterator it,
const COORD target)
const COORD target,
const std::optional<bool> wrap)
{
return _textBuffer->Write(it, target);
// NOTE: if wrap = true/false, we want to set the line's wrap to true/false (respectively) if we reach the end of the line
return _textBuffer->Write(it, target, wrap);
}

// Routine Description:
Expand Down
3 changes: 2 additions & 1 deletion src/host/screenInfo.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
OutputCellIterator Write(const OutputCellIterator it);

OutputCellIterator Write(const OutputCellIterator it,
const COORD target);
const COORD target,
const std::optional<bool> wrap = true);

OutputCellIterator WriteRect(const OutputCellIterator it,
const Microsoft::Console::Types::Viewport viewport);
Expand Down
74 changes: 74 additions & 0 deletions src/host/ut_host/TextBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ class TextBufferTests

TEST_METHOD(TestWrapFlag);

TEST_METHOD(TestWrapThroughWriteLine);

TEST_METHOD(TestDoubleBytePadFlag);

void DoBoundaryTest(PWCHAR const pwszInputString,
Expand Down Expand Up @@ -197,6 +199,78 @@ void TextBufferTests::TestWrapFlag()
VERIFY_IS_FALSE(Row.GetCharRow().WasWrapForced());
}

void TextBufferTests::TestWrapThroughWriteLine()
{
TextBuffer& textBuffer = GetTbi();

auto VerifyWrap = [&](bool expected) {
ROW& Row = textBuffer._GetFirstRow();

if (expected)
{
VERIFY_IS_TRUE(Row.GetCharRow().WasWrapForced());
}
else
{
VERIFY_IS_FALSE(Row.GetCharRow().WasWrapForced());
}
};

// Construct string for testing
const auto width = textBuffer.GetSize().Width();
std::wstring chars = L"";
for (auto i = 0; i < width; i++)
{
chars.append(L"a");
}
const auto lineOfText = std::move(chars);

Log::Comment(L"Case 1 : Implicit wrap (false)");
{
TextAttribute expectedAttr(FOREGROUND_RED);
OutputCellIterator it(lineOfText, expectedAttr);

textBuffer.WriteLine(it, { 0, 0 });
VerifyWrap(false);
}

Log::Comment(L"Case 2 : wrap = true");
{
TextAttribute expectedAttr(FOREGROUND_RED);
OutputCellIterator it(lineOfText, expectedAttr);

textBuffer.WriteLine(it, { 0, 0 }, true);
VerifyWrap(true);
}

Log::Comment(L"Case 3: wrap = nullopt (remain as TRUE)");
{
TextAttribute expectedAttr(FOREGROUND_RED);
OutputCellIterator it(lineOfText, expectedAttr);

textBuffer.WriteLine(it, { 0, 0 }, std::nullopt);
VerifyWrap(true);
}

Log::Comment(L"Case 4: wrap = false");
{
TextAttribute expectedAttr(FOREGROUND_RED);
OutputCellIterator it(lineOfText, expectedAttr);

textBuffer.WriteLine(it, { 0, 0 }, false);
VerifyWrap(false);
}

Log::Comment(L"Case 5: wrap = nullopt (remain as false)");
{
TextAttribute expectedAttr(FOREGROUND_RED);
OutputCellIterator it(lineOfText, expectedAttr);

textBuffer.WriteLine(it, { 0, 0 }, std::nullopt);
VerifyWrap(false);
}
}

void TextBufferTests::TestDoubleBytePadFlag()
{
TextBuffer& textBuffer = GetTbi();
Expand Down

0 comments on commit f081a95

Please sign in to comment.