Move AdaptDispatch::_FillRect into TextBuffer by lhecker · Pull Request #15541 · microsoft/terminal

lhecker · 2023-06-12T16:26:07Z

This commit makes 2 changes:

Expose dirty-range information from ROW::CopyTextFrom
This will allow us to call TriggerRedraw, which is an aspect
I haven't previously considered as something this API needs.
Add a FillRect API to TextBuffer and refactor AdeptDispatch
to use that API. Even if we determine that the new text APIs are
unfit (for instance too difficult to use), this will make it simpler
to write efficient implementations right inside TextBuffer.

Since the new FillRect API lacks bounds checks the way WriteLine
has them, it breaks AdaptDispatch::_EraseAll which failed to adjust
the bottom parameter after scrolling the contents. This would result
in more rows being erased than intended.

Validation Steps Performed

chcp 65001
Launch pwsh
"`e[29483`$x" fills the viewport with cats ✅
ResizeTraditional still doesn't work any worse than it used to ✅

DHowett · 2023-06-12T17:12:12Z

@msftbot make sure @j4james signs off 😄

src/terminal/parser/stateMachine.hpp

j4james

Very happy to have full Unicode support in DECFRA, but I'm just concerned about the possible loss of accessibility in ConHost, and the need for clamping in the REP escape sequence.

src/terminal/adapter/adaptDispatch.cpp

src/terminal/parser/stateMachine.hpp

DHowett · 2023-06-13T16:22:59Z

I see your rename, but I am a bit worried that the title buries the lede a bit ;)

lhecker · 2023-06-13T16:30:27Z

Do you feel like the TextBuffer change is more significant? Would you like me to extract it out?

DHowett · 2023-06-13T16:33:56Z

I'd suggest something like, "Lift AdaptDispatch::_FillRect into TextBuffer, refactor". Note Adapt (not Adept!)

lhecker · 2023-06-13T16:35:49Z

I've shortened it a bit, so it better fits the column limit.

j4james · 2023-06-13T20:59:40Z

"`e[29483`$x" fills the viewport with cats ✅

Technically true, but disappointing. 😿

j4james

I've only really reviewed the AdaptDispatch code, but I've also built it locally and done some manual testing, and it looks good to me.

carlos-zamora · 2023-06-13T20:58:25Z

src/terminal/adapter/adaptDispatch.cpp

        }
        _api.NotifyBufferRotation(delta);
        newViewportTop -= delta;
+        newViewportBottom -= delta;


Huh, we didn't do this before, but this seems right. Any idea if this fixes some bug or something?

Ah yes I forgot to mention this. Thanks for pointing it out. It's not really a bug per se, but the old code relied on TextBuffer::WriteLine discarding any writes that are out of bounds. The new TextBuffer::FillRect on the other hand will not check if the given rectangle is in the bounds of the buffer and so if you give it an address like 310 in a 300-row buffer it'll fill row 10. This change fixes the issue.

lhecker · 2023-06-13T22:11:23Z

Technically true, but disappointing. 😿

Yeah. 😢 But we got the tech now to do it at least. 😅 I didn't feel comfortable making the change to the MAX_PARAMETER_VALUE just yet, because I wouldn't consider myself knowledgeable in that area. One option I've considered is that we only do a std::min() bounds check when we return the value stored in a VTParameter so that we can choose to extract the unclamped value in specific cases like this one. But I discarded the idea for the same reason. I'd be happy to pick it up if we want to pursue it again.

#15541 changed `AdaptDispatch::_FillRect` which caused it to not affect the `ROW::_wrapForced` flag anymore. This change in behavior was not noticeable as `TextBuffer::GetLastNonSpaceCharacter` had a bug where rows of only whitespace text would always be treated as empty. This would then affect `AdaptDispatch::_EraseAll` to accidentally correctly guess the last row with text despite the `_FillRect` change. #15701 then fixed `GetLastNonSpaceCharacter` indirectly by fixing `ROW::MeasureRight` which now made the previous change apparent. `_EraseAll` would now guess the last row of text incorrectly, because it would find the rows that `_FillRect` cleared but still had `_wrapForced` set to `true`. This PR fixes the issue by replacing the `_FillRect` usage to clear rows with direct calls to `ROW::Reset()`. In the future this could be extended by also `MEM_DECOMMIT`ing the now unused underlying memory. Closes #16603 ## Validation Steps Performed * Enter WSL and resize the window to <40 columns * Execute ```sh cd /bin ls -la printf "\e[3J" ls -la printf "\e[3J" printf "\e[2J" ``` * Only one viewport-height-many lines of whitespace exist between the current prompt line and the previous scrollback contents ✅

#15541 changed `AdaptDispatch::_FillRect` which caused it to not affect the `ROW::_wrapForced` flag anymore. This change in behavior was not noticeable as `TextBuffer::GetLastNonSpaceCharacter` had a bug where rows of only whitespace text would always be treated as empty. This would then affect `AdaptDispatch::_EraseAll` to accidentally correctly guess the last row with text despite the `_FillRect` change. #15701 then fixed `GetLastNonSpaceCharacter` indirectly by fixing `ROW::MeasureRight` which now made the previous change apparent. `_EraseAll` would now guess the last row of text incorrectly, because it would find the rows that `_FillRect` cleared but still had `_wrapForced` set to `true`. This PR fixes the issue by replacing the `_FillRect` usage to clear rows with direct calls to `ROW::Reset()`. In the future this could be extended by also `MEM_DECOMMIT`ing the now unused underlying memory. Closes #16603 ## Validation Steps Performed * Enter WSL and resize the window to <40 columns * Execute ```sh cd /bin ls -la printf "\e[3J" ls -la printf "\e[3J" printf "\e[2J" ``` * Only one viewport-height-many lines of whitespace exist between the current prompt line and the previous scrollback contents ✅ (cherry picked from commit 5f71cf3) Service-Card-Id: 91707937 Service-Version: 1.19

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Product-Terminal The new Windows Terminal. labels Jun 12, 2023

lhecker force-pushed the dev/lhecker/15390-unicode-fill branch from e4f75bb to fbbc632 Compare June 12, 2023 16:31

lhecker mentioned this pull request Jun 12, 2023

Initialize rows lazily #15524

Merged

lhecker commented Jun 12, 2023

View reviewed changes

src/terminal/parser/stateMachine.hpp Outdated Show resolved Hide resolved

j4james reviewed Jun 12, 2023

View reviewed changes

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved

src/terminal/adapter/adaptDispatch.cpp Show resolved Hide resolved

src/terminal/parser/stateMachine.hpp Outdated Show resolved Hide resolved

Remove IsGlyphFullWidth usage from AdeptDispatch

04e7f59

lhecker changed the title ~~Improve Unicode correctness of AdeptDispatch's rect fill~~ Remove IsGlyphFullWidth usage from AdeptDispatch Jun 13, 2023

lhecker force-pushed the dev/lhecker/15390-unicode-fill branch from 55e8118 to 04e7f59 Compare June 13, 2023 15:06

lhecker changed the title ~~Remove IsGlyphFullWidth usage from AdeptDispatch~~ Move AdaptDispatch::_FillRect into TextBuffer Jun 13, 2023

j4james approved these changes Jun 13, 2023

View reviewed changes

carlos-zamora approved these changes Jun 13, 2023

View reviewed changes

DHowett approved these changes Jun 14, 2023

View reviewed changes

DHowett merged commit c183d12 into main Jun 14, 2023

DHowett deleted the dev/lhecker/15390-unicode-fill branch June 14, 2023 19:34

lhecker mentioned this pull request Jul 24, 2023

REP corrupts some Unicode characters (emoji) #15751

Closed

j4james mentioned this pull request Jan 27, 2024

ED2 inserts loads of blank lines in scrollback #16603

Closed

lhecker mentioned this pull request Jan 27, 2024

Reset _wrapForced when erasing scrollback #16610

Merged

Conversation

lhecker commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

DHowett commented Jun 12, 2023

Uh oh!

Uh oh!

j4james left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett commented Jun 13, 2023

Uh oh!

lhecker commented Jun 13, 2023

Uh oh!

DHowett commented Jun 13, 2023

Uh oh!

lhecker commented Jun 13, 2023

Uh oh!

j4james commented Jun 13, 2023

Uh oh!

j4james left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

lhecker Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

lhecker commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lhecker commented Jun 12, 2023 •

edited

Loading

lhecker commented Jun 13, 2023 •

edited

Loading