Prevent horizontally scrolling wide chars erasing themselves#14650
Prevent horizontally scrolling wide chars erasing themselves#146502 commits merged intomicrosoft:mainfrom
Conversation
ghost
added
Area-Output
carlos-zamora
left a comment
carlos-zamora
left a comment
There was a problem hiding this comment.
Straightforward and tested. Thanks for doing this!
| source.WalkInBounds(sourcePos, walkDirection); | ||
| next = OutputCell(*screenInfo.GetCellDataAt(sourcePos)); |
There was a problem hiding this comment.
Is it possible for this to result in source running out of space (somehow?) before target does? I guess we're doing it in the previous code as well, but maybe one fewer time.
There was a problem hiding this comment.
If you look at the WalkInBounds implementation, it just doesn't update the position once it has reached the end of the range, so all we'll be doing is reading the final position twice.
terminal/src/types/viewport.cpp
Lines 405 to 417 in 0eff8c0
for some reason this is hilarious to me. Like, of course it does, classic wide char code 🙃 |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
|
🎉 Handy links: |
ghost
mentioned this pull request
When a `DECCRA` operation is copying content that spans a double width line, it's possible that some range of the bounding rectangle will be off-screen, and that range is not supposed to be copied. However, the code checking for off-screen positions was using incorrect coordinates, so we would mistakenly copy content that shouldn't be copied, and drop content that should have been copied. This PR fixes that. ## References and Relevant Issues This was a regression introduced in PR #14650 when fixing an issue with horizontal scrolling of DBCS characters. ## Validation Steps Performed I manually verified this fixes the test case in #15019, and I've also added a unit test that replicates that case. Closes #15019
5 participants
When the buffer contains wide characters that occupy more than one cell,
and those cells are scrolled horizontally by exactly one column, that
operation can result in the wide characters being completely erased.
This PR attempts to fix that bug, although it's not an ideal long term
solution.
Although not really to blame, it was PR #13626 that exposed this issue.
The root of the problem is that scrolling operations copy cells one by
one, but wide characters are written to the buffer two cells at a time.
So when you move a wide character one position to the left or right, it
can overwrite itself before it's finished copying, and the end result is
the whole character gets erased.
I've attempt to solve this by getting the affected operations to read
two cells in advance before they start writing, so there's no risk of
losing the source data before it's fully output. This may not work in
the long term, with characters wider than two cells, but it should at
least be good enough for now.
I've also changed the
TextBuffer::Writecall to aWriteLinecall toimprove the handling of a wide character on the end of the line, where
moving it right by one column would place it half off screen. It should
just be dropped, but with the
Writemethod, it would end up pushedonto the following line.
Validation Steps Performed
I've manually confirmed this fixes all the test cases described in
#14626, and also added some unit tests that replicate those scenarios.
Closes #14626