Skip to content

Commit

Permalink
We've been trying to reach you about your WriteCharsLegacy's extended…
Browse files Browse the repository at this point in the history
… Emoji support (#15567)

This is a complete rewrite of the old `WriteCharsLegacy` function
which is used when VT mode is disabled as well as for all interactive
console input handling on Windows. The previous code was almost
horrifying in some aspects as it first wrote the incoming text into a
local buffer, stripping/replacing any control characters. That's not
particular fast and never was. It's unknown why it was like that.

It also measured the width of each glyph to correctly determine the
cursor position and line wrapping. Presumably this used to work quite
well in the original console code, because it would then just copy
that local buffer into the destination text buffer, but with the
introduction of the broken and extremely slow `OutputCellIterator`
abstraction this would end up measuring all text twice and cause
disagreements between `WriteCharsLegacy`'s idea of the cursor position
and `OutputCellIterator`'s cursor position. Emoji input was basically
entirely broken. This PR fixes it by passing any incoming text
straight to the `TextBuffer` as well as by using its cursor positioning
facilities to correctly implement wrapping and backspace handling.

Backspacing over Emojis and an array of other aspects still don't work
correctly thanks to cmdline.cpp, but it works quite a lot better now.

Related to #8000
Closes #8839
Closes #10808

## Validation Steps Performed
* Printing various Unicode text ✅
* On an fgets() input line
  * Typing text works ✅
  * Inserting text works anywhere ✅
  * Ctrl+X is translated to ^X ✅
  * Null is translated to ^@ ✅
    This was tested by hardcoding the `OutputMode` to 3 instead of 7.
  * Backspace only advances to start of the input ✅
  * Backspace deletes the entire preceding tab ✅
  * Backspace doesn't delete whitespace preceding a tab ✅
  * Backspacing a force-wrapped wide glyph unwraps the line break ✅
  * Backspacing ^X deletes both glyphs ✅
  * Backspacing a force-wrapped tab deletes trailing whitespace ✅
* When executing
  ```cpp
  fputs("foo: ", stdout);
  fgets(buffer, stdin);
  ```
  pressing tab and then backspace does not delete the whitespace
  that follows after the "foo:" string (= `sOriginalXPosition`).
  • Loading branch information
lhecker authored Jun 30, 2023
1 parent 030db09 commit 94e6b91
Show file tree
Hide file tree
Showing 14 changed files with 322 additions and 562 deletions.
17 changes: 17 additions & 0 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,12 @@ LineRendition ROW::GetLineRendition() const noexcept
return _lineRendition;
}

uint16_t ROW::GetLineWidth() const noexcept
{
const auto scale = _lineRendition != LineRendition::SingleWidth ? 1 : 0;
return _columnCount >> scale;
}

// Routine Description:
// - Sets all properties of the ROW to default values
// Arguments:
Expand Down Expand Up @@ -882,6 +888,17 @@ std::wstring_view ROW::GetText() const noexcept
return { _chars.data(), _charSize() };
}

std::wstring_view ROW::GetText(til::CoordType columnBegin, til::CoordType columnEnd) const noexcept
{
const til::CoordType columns = _columnCount;
const auto colBeg = std::max(0, std::min(columns, columnBegin));
const auto colEnd = std::max(colBeg, std::min(columns, columnEnd));
const size_t chBeg = _uncheckedCharOffset(gsl::narrow_cast<size_t>(colBeg));
const size_t chEnd = _uncheckedCharOffset(gsl::narrow_cast<size_t>(colEnd));
#pragma warning(suppress : 26481) // Don't use pointer arithmetic. Use span instead (bounds.1).
return { _chars.data() + chBeg, chEnd - chBeg };
}

DelimiterClass ROW::DelimiterClassAt(til::CoordType column, const std::wstring_view& wordDelimiters) const noexcept
{
const auto col = _clampedColumn(column);
Expand Down
2 changes: 2 additions & 0 deletions src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ class ROW final
bool WasDoubleBytePadded() const noexcept;
void SetLineRendition(const LineRendition lineRendition) noexcept;
LineRendition GetLineRendition() const noexcept;
uint16_t GetLineWidth() const noexcept;

void Reset(const TextAttribute& attr) noexcept;
void TransferAttributes(const til::small_rle<TextAttribute, uint16_t, 1>& attr, til::CoordType newWidth);
Expand Down Expand Up @@ -151,6 +152,7 @@ class ROW final
std::wstring_view GlyphAt(til::CoordType column) const noexcept;
DbcsAttribute DbcsAttrAt(til::CoordType column) const noexcept;
std::wstring_view GetText() const noexcept;
std::wstring_view GetText(til::CoordType columnBegin, til::CoordType columnEnd) const noexcept;
DelimiterClass DelimiterClassAt(til::CoordType column, const std::wstring_view& wordDelimiters) const noexcept;

auto AttrBegin() const noexcept { return _attr.begin(); }
Expand Down
16 changes: 7 additions & 9 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -450,18 +450,11 @@ void TextBuffer::ConsumeGrapheme(std::wstring_view& chars) noexcept

// 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::WriteLine(til::CoordType row, bool wrapAtEOL, const TextAttribute& attributes, RowWriteState& state)
void TextBuffer::Write(til::CoordType row, const TextAttribute& attributes, RowWriteState& state)
{
auto& r = GetRowByOffset(row);

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

if (state.columnEnd >= state.columnLimit)
{
r.SetWrapForced(wrapAtEOL);
}

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

Expand Down Expand Up @@ -946,7 +939,7 @@ const Cursor& TextBuffer::GetCursor() const noexcept
return _cursor;
}

[[nodiscard]] TextAttribute TextBuffer::GetCurrentAttributes() const noexcept
const TextAttribute& TextBuffer::GetCurrentAttributes() const noexcept
{
return _currentAttributes;
}
Expand All @@ -956,6 +949,11 @@ void TextBuffer::SetCurrentAttributes(const TextAttribute& currentAttributes) no
_currentAttributes = currentAttributes;
}

void TextBuffer::SetWrapForced(const til::CoordType y, bool wrap)
{
GetRowByOffset(y).SetWrapForced(wrap);
}

void TextBuffer::SetCurrentLineRendition(const LineRendition lineRendition, const TextAttribute& fillAttributes)
{
const auto cursorPosition = GetCursor().GetPosition();
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 @@ -98,7 +98,7 @@ class TextBuffer final

// Text insertion functions
static void ConsumeGrapheme(std::wstring_view& chars) noexcept;
void WriteLine(til::CoordType row, bool wrapAtEOL, const TextAttribute& attributes, RowWriteState& state);
void Write(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 Expand Up @@ -133,10 +133,11 @@ class TextBuffer final

til::CoordType TotalRowCount() const noexcept;

[[nodiscard]] TextAttribute GetCurrentAttributes() const noexcept;
const TextAttribute& GetCurrentAttributes() const noexcept;

void SetCurrentAttributes(const TextAttribute& currentAttributes) noexcept;

void SetWrapForced(til::CoordType y, bool wrap);
void SetCurrentLineRendition(const LineRendition lineRendition, const TextAttribute& fillAttributes);
void ResetLineRenditionRange(const til::CoordType startRow, const til::CoordType endRow);
LineRendition GetLineRendition(const til::CoordType row) const;
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#include "../host/readDataCooked.hpp"
#include "../host/output.h"
#include "../host/_stream.h" // For WriteCharsLegacy
#include "../host/cmdline.h" // For WC_LIMIT_BACKSPACE
#include "../host/cmdline.h" // For WC_INTERACTIVE
#include "test/CommonState.hpp"

#include "../cascadia/TerminalCore/Terminal.hpp"
Expand Down Expand Up @@ -3422,7 +3422,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottomLikeMSYS()
}
else if (writingMethod == PrintWithWriteCharsLegacy)
{
doWriteCharsLegacy(si, str, WC_LIMIT_BACKSPACE);
doWriteCharsLegacy(si, str, WC_INTERACTIVE);
}
};

Expand Down
4 changes: 2 additions & 2 deletions src/host/CommandNumberPopup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ void CommandNumberPopup::_handleNumber(COOKED_READ_DATA& cookedReadData, const w
&CharsToWrite,
&NumSpaces,
cookedReadData.OriginalCursorPosition().x,
WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS,
WC_INTERACTIVE | WC_KEEP_CURSOR_VISIBLE,
nullptr));
cookedReadData.ScreenInfo().SetAttributes(realAttributes);
try
Expand Down Expand Up @@ -73,7 +73,7 @@ void CommandNumberPopup::_handleBackspace(COOKED_READ_DATA& cookedReadData) noex
&CharsToWrite,
&NumSpaces,
cookedReadData.OriginalCursorPosition().x,
WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS,
WC_INTERACTIVE | WC_KEEP_CURSOR_VISIBLE,
nullptr));
cookedReadData.ScreenInfo().SetAttributes(realAttributes);
_pop();
Expand Down
Loading

0 comments on commit 94e6b91

Please sign in to comment.