Make UTextFromTextBuffer newline aware #17120
Conversation
| } | ||
|
|
||
| return text; | ||
| const auto req = CopyRequest::FromConfig(*this, start, end, true, false, false, false); |
There was a problem hiding this comment.
FWIW I believe we should remove the constructor from CopyRequest and instead sanitize it in each individual function that accepts it. This would allow the callers to use designated initializers like so:
CopyRequest req{
.beg = ...,
.end = ...,
.singleLine = true,
};This would turn CopyRequest into a true request struct IMO. It would be similar to sanitizing a web request on the server to protect against malicious clients.
|
|
||
| return text; | ||
| const auto req = CopyRequest::FromConfig(*this, start, end, true, false, false, false); | ||
| return GetPlainText(req); |
There was a problem hiding this comment.
are we sure this is functionally equivalent? can you test the known multi-cell-character URL bugs?
There was a problem hiding this comment.
GetPlainText is only used when pressing Ctrl+Enter on a URL (in mark mode) if it's detected via pattern, and the pattern only supports ASCII characters.
There was a problem hiding this comment.
Clever. Effectively, you're letting our utext adapter fall back to a temporary buffer that is filled with the content of one row plus a newline character.
My concern is, most of the text in a terminal is like this (not forced wrap). Is it possible to have a fast path for when nobody is trying to read the end of the line?
|
I don't think it's worth special-casing it at this time. The copy is very fast and not a major bottleneck. I tried storing the |
| size_t capacity; | ||
| wchar_t data[1]; | ||
|
|
||
| static RefcountBuffer* EnsureCapacityForOverwrite(RefcountBuffer* buffer, size_t capacity) |
There was a problem hiding this comment.
nit: This might warrant a doc comment. This is the first thing in the review, and it's not immediately clear what it's trying to achieve
check if buffer exists, and is at least size capacity. If it isn't, make a new one at least the size of capacity.
| } | ||
|
|
||
| const auto oldCapacity = buffer ? buffer->capacity << 1 : 0; | ||
| const auto newCapacity = std::max(capacity + 128, oldCapacity); |
There was a problem hiding this comment.
if we had to make a new buffer, make the new buffer's size the bigger of (2x the old buffer's size OR capacity+128). I guess to add some padding, in case the caller only needed 80 right now, but might want 200 with this same buffer
| constexpr RefcountBuffer*& accessBuffer(UText* ut) noexcept | ||
| { | ||
| static_assert(sizeof(ut->q) == sizeof(RefcountBuffer*)); | ||
| return *std::bit_cast<RefcountBuffer**>(&ut->q); |
There was a problem hiding this comment.
oh so this is some spooky bit casting magic, got it
This PR achieves two things: * When encountering rows with newlines (`WasForceWrapped` = `false`) we'll now copy the contents out of the row and append a `\n`. To make `utext_clone` cheap, it adds a reference counted buffer. * Text extraction in `Terminal::GetHyperlinkAtBufferPosition` was fixed by using a higher level `TextBuffer::GetPlainText` instead of iterating through each cell. Closes #16676 Closes #17065 ## Validation Steps Performed * In pwsh execute the following: ``"`e[999C`e[22Dhttps://example.com/foo`nbar"`` * Hovering over the URL only underlines `.../foo` and not `bar` ✅ * The tooltip ends in `.../foo` and not `.../fo` ✅ (cherry picked from commit 5b8eadb) Service-Card-Id: 92509615 Service-Version: 1.20
This PR achieves two things:
WasForceWrapped=false)we'll now copy the contents out of the row and append a
\n.To make
utext_clonecheap, it adds a reference counted buffer.Terminal::GetHyperlinkAtBufferPositionwas fixed by using a higher level
TextBuffer::GetPlainTextinstead of iterating through each cell.
Closes #16676
Closes #17065
Validation Steps Performed
"`e[999C`e[22Dhttps://example.com/foo`nbar".../fooand notbar✅.../fooand not.../fo✅