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.

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

Remove dependency on IsGlyphFullWidth for IRM/DECSWL #16903

Merged
merged 6 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
aaaaabbb
aabbcc
ABANDONFONT
abbcc
Expand Down Expand Up @@ -92,6 +93,8 @@ backported
backstory
barbaz
Bazz
bbb
bbccb
BBDM
bbwe
bcount
Expand Down
9 changes: 2 additions & 7 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,6 @@ void ROW::_init() noexcept
#pragma warning(push)
}

void ROW::TransferAttributes(const til::small_rle<TextAttribute, uint16_t, 1>& attr, til::CoordType newWidth)
{
_attr = attr;
_attr.resize_trailing_extent(gsl::narrow<uint16_t>(newWidth));
}

void ROW::CopyFrom(const ROW& source)
{
_lineRendition = source._lineRendition;
Expand All @@ -365,7 +359,8 @@ void ROW::CopyFrom(const ROW& source)
};
CopyTextFrom(state);

TransferAttributes(source.Attributes(), _columnCount);
_attr = source.Attributes();
_attr.resize_trailing_extent(_columnCount);
}

// Returns the previous possible cursor position, preceding the given column.
Expand Down
1 change: 0 additions & 1 deletion src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ class ROW final
til::CoordType GetReadableColumnCount() const noexcept;

void Reset(const TextAttribute& attr) noexcept;
void TransferAttributes(const til::small_rle<TextAttribute, uint16_t, 1>& attr, til::CoordType newWidth);
void CopyFrom(const ROW& source);

til::CoordType NavigateToPrevious(til::CoordType column) const noexcept;
Expand Down
37 changes: 32 additions & 5 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -562,14 +562,44 @@ til::point TextBuffer::NavigateCursor(til::point position, til::CoordType distan

// This function is intended for writing regular "lines" of text as it'll set the wrap flag on the given row.
// You can continue calling the function on the same row as long as state.columnEnd < state.columnLimit.
void TextBuffer::Write(til::CoordType row, const TextAttribute& attributes, RowWriteState& state)
void TextBuffer::Replace(til::CoordType row, const TextAttribute& attributes, RowWriteState& state)
{
auto& r = GetMutableRowByOffset(row);
r.ReplaceText(state);
r.ReplaceAttributes(state.columnBegin, state.columnEnd, attributes);
TriggerRedraw(Viewport::FromExclusive({ state.columnBeginDirty, row, state.columnEndDirty, row + 1 }));
}

void TextBuffer::Insert(til::CoordType row, const TextAttribute& attributes, RowWriteState& state)
{
auto& r = GetMutableRowByOffset(row);
auto& scratch = GetScratchpadRow();

scratch.CopyFrom(r);

r.ReplaceText(state);
r.ReplaceAttributes(state.columnBegin, state.columnEnd, attributes);

// Restore trailing text from our backup in scratch.
RowWriteState restoreState{
.text = scratch.GetText(state.columnBegin, state.columnLimit),
.columnBegin = state.columnEnd,
.columnLimit = state.columnLimit,
};
r.ReplaceText(restoreState);

// Restore trailing attributes as well.
if (const auto copyAmount = restoreState.columnEnd - restoreState.columnBegin; copyAmount > 0)
{
auto& rowAttr = r.Attributes();
const auto& scratchAttr = scratch.Attributes();
const auto restoreAttr = scratchAttr.slice(gsl::narrow<uint16_t>(state.columnBegin), gsl::narrow<uint16_t>(state.columnBegin + copyAmount));
rowAttr.replace(gsl::narrow<uint16_t>(restoreState.columnBegin), gsl::narrow<uint16_t>(restoreState.columnEnd), restoreAttr);
}

TriggerRedraw(Viewport::FromExclusive({ state.columnBeginDirty, row, restoreState.columnEndDirty, row + 1 }));
}

// Fills an area of the buffer with a given fill character(s) and attributes.
void TextBuffer::FillRect(const til::rect& rect, const std::wstring_view& fill, const TextAttribute& attributes)
{
Expand Down Expand Up @@ -1083,11 +1113,8 @@ void TextBuffer::SetCurrentLineRendition(const LineRendition lineRendition, cons
// And if it's no longer single width, the right half of the row should be erased.
if (lineRendition != LineRendition::SingleWidth)
{
const auto fillChar = L' ';
const auto fillOffset = GetLineWidth(rowIndex);
const auto fillLength = gsl::narrow<size_t>(GetSize().Width() - fillOffset);
const OutputCellIterator fillData{ fillChar, fillAttributes, fillLength };
row.WriteCells(fillData, fillOffset, false);
FillRect({ fillOffset, rowIndex, til::CoordTypeMax, rowIndex + 1 }, L" ", fillAttributes);
// We also need to make sure the cursor is clamped within the new width.
GetCursor().SetPosition(ClampPositionWithinLine(cursorPosition));
}
Expand Down
3 changes: 2 additions & 1 deletion src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ class TextBuffer final
til::point NavigateCursor(til::point position, til::CoordType distance) const;

// Text insertion functions
void Write(til::CoordType row, const TextAttribute& attributes, RowWriteState& state);
void Replace(til::CoordType row, const TextAttribute& attributes, RowWriteState& state);
void Insert(til::CoordType row, const TextAttribute& attributes, RowWriteState& state);
void FillRect(const til::rect& rect, const std::wstring_view& fill, const TextAttribute& attributes);

OutputCellIterator Write(const OutputCellIterator givenIt);
Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class UTextAdapterTests
RowWriteState state{
.text = L"abc 𝒶𝒷𝒸 abc ネコちゃん",
};
buffer.Write(0, TextAttribute{}, state);
buffer.Replace(0, TextAttribute{}, state);
VERIFY_IS_TRUE(state.text.empty());

static constexpr auto s = [](til::CoordType beg, til::CoordType end) -> til::point_span {
Expand Down
2 changes: 1 addition & 1 deletion src/host/_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ void _writeCharsLegacyUnprocessed(SCREEN_INFORMATION& screenInfo, const std::wst
auto cursorPosition = textBuffer.GetCursor().GetPosition();

state.columnBegin = cursorPosition.x;
textBuffer.Write(cursorPosition.y, textBuffer.GetCurrentAttributes(), state);
textBuffer.Replace(cursorPosition.y, textBuffer.GetCurrentAttributes(), state);
cursorPosition.x = state.columnEnd;

if (wrapAtEOL && state.columnEnd >= state.columnLimit)
Expand Down
12 changes: 6 additions & 6 deletions src/host/readDataCooked.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1192,13 +1192,13 @@ try
buffer.front() = L'┌';
buffer.back() = L'┐';
state.text = buffer;
textBuffer.Write(contentRect.top - 1, attributes, state);
textBuffer.Replace(contentRect.top - 1, attributes, state);

// bottom line └───┘
buffer.front() = L'└';
buffer.back() = L'┘';
state.text = buffer;
textBuffer.Write(contentRect.bottom, attributes, state);
textBuffer.Replace(contentRect.bottom, attributes, state);

// middle lines │ │
buffer.assign(widthSizeT, L' ');
Expand All @@ -1207,7 +1207,7 @@ try
for (til::CoordType y = contentRect.top; y < contentRect.bottom; ++y)
{
state.text = buffer;
textBuffer.Write(y, attributes, state);
textBuffer.Replace(y, attributes, state);
}
}

Expand Down Expand Up @@ -1391,7 +1391,7 @@ void COOKED_READ_DATA::_popupHandleCommandNumberInput(Popup& popup, const wchar_
.columnBegin = popup.contentRect.right - CommandNumberMaxInputLength,
.columnLimit = popup.contentRect.right,
};
_screenInfo.GetTextBuffer().Write(popup.contentRect.top, _screenInfo.GetPopupAttributes(), state);
_screenInfo.GetTextBuffer().Replace(popup.contentRect.top, _screenInfo.GetPopupAttributes(), state);
}
}

Expand Down Expand Up @@ -1474,7 +1474,7 @@ void COOKED_READ_DATA::_popupDrawPrompt(const Popup& popup, const UINT id) const
.columnBegin = popup.contentRect.left,
.columnLimit = popup.contentRect.right,
};
_screenInfo.GetTextBuffer().Write(popup.contentRect.top, _screenInfo.GetPopupAttributes(), state);
_screenInfo.GetTextBuffer().Replace(popup.contentRect.top, _screenInfo.GetPopupAttributes(), state);
}

void COOKED_READ_DATA::_popupDrawCommandList(Popup& popup) const
Expand Down Expand Up @@ -1533,7 +1533,7 @@ void COOKED_READ_DATA::_popupDrawCommandList(Popup& popup) const
buffer.append(width, L' ');

state.text = buffer;
_screenInfo.GetTextBuffer().Write(y, attr, state);
_screenInfo.GetTextBuffer().Replace(y, attr, state);
}

cl.dirtyHeight = height;
Expand Down
91 changes: 86 additions & 5 deletions src/host/ut_host/TextBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ class TextBufferTests

TEST_METHOD(TestBurrito);
TEST_METHOD(TestOverwriteChars);
TEST_METHOD(TestRowReplaceText);
TEST_METHOD(TestReplace);
TEST_METHOD(TestInsert);

TEST_METHOD(TestAppendRTFText);

Expand Down Expand Up @@ -2003,13 +2004,12 @@ void TextBufferTests::TestOverwriteChars()
#undef complex1
}

void TextBufferTests::TestRowReplaceText()
void TextBufferTests::TestReplace()
{
static constexpr til::size bufferSize{ 10, 3 };
static constexpr UINT cursorSize = 12;
const TextAttribute attr{ 0x7f };
TextBuffer buffer{ bufferSize, attr, cursorSize, false, _renderer };
auto& row = buffer.GetMutableRowByOffset(0);

#define complex L"\U0001F41B"

Expand Down Expand Up @@ -2073,17 +2073,98 @@ void TextBufferTests::TestRowReplaceText()
.columnBegin = t.input.columnBegin,
.columnLimit = t.input.columnLimit,
};
row.ReplaceText(actual);
buffer.Replace(0, attr, actual);
VERIFY_ARE_EQUAL(t.expected.text, actual.text);
VERIFY_ARE_EQUAL(t.expected.columnEnd, actual.columnEnd);
VERIFY_ARE_EQUAL(t.expected.columnBeginDirty, actual.columnBeginDirty);
VERIFY_ARE_EQUAL(t.expected.columnEndDirty, actual.columnEndDirty);
VERIFY_ARE_EQUAL(t.expectedRow, row.GetText());
VERIFY_ARE_EQUAL(t.expectedRow, buffer.GetRowByOffset(0).GetText());
}

#undef complex
}

void TextBufferTests::TestInsert()
{
static constexpr til::size bufferSize{ 10, 3 };
static constexpr UINT cursorSize = 12;
static constexpr TextAttribute attr1{ 0x11111111, 0x00000000 };
static constexpr TextAttribute attr2{ 0x22222222, 0x00000000 };
static constexpr TextAttribute attr3{ 0x33333333, 0x00000000 };
TextBuffer buffer{ bufferSize, attr1, cursorSize, false, _renderer };

struct Test
{
const wchar_t* description;
struct
{
std::wstring_view text;
til::CoordType columnBegin = 0;
til::CoordType columnLimit = 0;
TextAttribute attr;
} input;
struct
{
std::wstring_view text;
til::CoordType columnEnd = 0;
til::CoordType columnBeginDirty = 0;
til::CoordType columnEndDirty = 0;
} expected;
std::wstring_view expectedRow;
};

static constexpr std::array tests{
Test{
L"Not enough space -> early exit",
{ L"aaa", 5, 5, attr1 },
{ L"aaa", 5, 5, 5 },
L" ",
},
Test{
L"Too much to fit",
{ L"aaaaabbb", 0, 5, attr1 },
Fixed Show fixed Hide fixed
{ L"bbb", 5, 0, 5 },
Fixed Show fixed Hide fixed
L"aaaaa ",
},
Test{
L"Wide char intersects limit",
{ L"bbbb😄", 0, 5, attr2 },
{ L"😄", 5, 0, 5 },
L"bbbb ",
},
Test{
L"Insert middle",
{ L"cc", 2, 5, attr3 },
{ L"", 4, 2, 4 },
L"bbccb ",
Fixed Show fixed Hide fixed
},
};

for (const auto& t : tests)
{
Log::Comment(t.description);
RowWriteState actual{
.text = t.input.text,
.columnBegin = t.input.columnBegin,
.columnLimit = t.input.columnLimit,
};
buffer.Insert(0, t.input.attr, actual);
VERIFY_ARE_EQUAL(t.expected.text, actual.text);
VERIFY_ARE_EQUAL(t.expected.columnEnd, actual.columnEnd);
VERIFY_ARE_EQUAL(t.expected.columnBeginDirty, actual.columnBeginDirty);
VERIFY_ARE_EQUAL(t.expected.columnEndDirty, actual.columnEndDirty);
VERIFY_ARE_EQUAL(t.expectedRow, buffer.GetRowByOffset(0).GetText());
}

auto& scratch = buffer.GetScratchpadRow();
scratch.ReplaceAttributes(0, 5, attr2);
scratch.ReplaceAttributes(2, 4, attr3);

const auto& expectedAttr = scratch.Attributes();
const auto& actualAttr = buffer.GetRowByOffset(0).Attributes();
VERIFY_ARE_EQUAL(expectedAttr, actualAttr);
}

void TextBufferTests::TestAppendRTFText()
{
{
Expand Down
12 changes: 0 additions & 12 deletions src/inc/til/rle.h
Original file line number Diff line number Diff line change
Expand Up @@ -1042,18 +1042,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
#ifdef __WEX_COMMON_H__
namespace WEX::TestExecution
{
template<typename T, typename S, typename Container>
class VerifyOutputTraits<::til::basic_rle<T, S, Container>>
{
using rle_vector = ::til::basic_rle<T, S, Container>;

public:
static WEX::Common::NoThrowString ToString(const rle_vector& object)
{
return WEX::Common::NoThrowString(object.to_string().c_str());
}
};
Copy link
Member Author

Choose a reason for hiding this comment

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

...yes. 😑
Let me know if you dislike this removal. rle_vector uses strstream like all the other classes and TextAttribute has no string-serializer. I just removed it because it was easier. I could however write a full ToString method here if we want to.

Copy link
Member

Choose a reason for hiding this comment

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

yeah honestly I don't particularly care for all of these


template<typename T, typename S, typename Container>
class VerifyCompareTraits<::til::basic_rle<T, S, Container>, ::til::basic_rle<T, S, Container>>
{
Expand Down
29 changes: 9 additions & 20 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

#include "adaptDispatch.hpp"
#include "../../renderer/base/renderer.hpp"
#include "../../types/inc/GlyphWidth.hpp"
#include "../../types/inc/Viewport.hpp"
#include "../../types/inc/utils.hpp"
#include "../../inc/unicode.hpp"
Expand Down Expand Up @@ -119,26 +118,17 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
}
}

if (_modes.test(Mode::InsertReplace))
{
// If insert-replace mode is enabled, we first measure how many cells
// the string will occupy, and scroll the target area right by that
// amount to make space for the incoming text.
const OutputCellIterator it(state.text, attributes);
auto measureIt = it;
while (measureIt && measureIt.GetCellDistance(it) < state.columnLimit)
{
++measureIt;
}
const auto row = cursorPosition.y;
const auto cellCount = measureIt.GetCellDistance(it);
_ScrollRectHorizontally(textBuffer, { cursorPosition.x, row, state.columnLimit, row + 1 }, cellCount);
}

state.columnBegin = cursorPosition.x;

const auto textPositionBefore = state.text.data();
textBuffer.Write(cursorPosition.y, attributes, state);
if (_modes.test(Mode::InsertReplace))
{
textBuffer.Insert(cursorPosition.y, attributes, state);
}
Comment on lines +124 to +127
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't working correctly when you have horizontal margins set. If you're inserting text while inside the margins, the content that is pushed to the right should only move as far as the right margin. I didn't look at the implementation that closely, but I'm guessing the Insert method is not taking the state.columnLimit into account.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Test case in pwsh:

"`e7`e[?69h`e[1;20s`e8*`e[19b`r`e[4h `e[19b`e[4l`e[s`e8`n"

That should produce what is effectively a blank line, but instead we're seeing 20 * characters offset by 20 columns.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe I fixed this issue. Thanks!

I noticed that after running your test string, TextBuffer::SetCurrentLineRendition won't get called anymore. Is that to be expected? (It does work again after emitting a RIS.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't think about that, but that is actually correct. When the horizontal margin mode is enabled, line renditions are disabled. I should have reset mode ?69 at the end of that test case.

else
{
textBuffer.Replace(cursorPosition.y, attributes, state);
}
const auto textPositionAfter = state.text.data();

// TODO: A row should not be marked as wrapped just because we wrote the last column.
Expand Down Expand Up @@ -190,8 +180,7 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)

// Notify UIA of new text.
// It's important to do this here instead of in TextBuffer, because here you
// have access to the entire line of text, whereas TextBuffer writes it one
// character at a time via the OutputCellIterator.
// have access to the entire line of text.
textBuffer.TriggerNewTextNotification(string);
}

Expand Down
Loading