Fix wrapped lines in less in Git for Windows
#5771
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e test. That's all we need, but man that's just weird
…definitely not the one that is actually reported in the bug. I'm gonna start fresh.
ghost
added
Area-Server
DHowett-MSFT
approved these changes
May 7, 2020
ghost
requested review from
|
small changes like this freak me out, so I'mma hope @miniksa can take a look at it |
miniksa
requested changes
May 8, 2020
ghost
added
the
Needs-Author-Feedback
…it_log_--pretty=oneline
ghost
removed
the
Needs-Author-Feedback
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! 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 (
|
DHowett-MSFT
pushed a commit
that referenced
this pull request
May 8, 2020
## Summary of the Pull Request
This PR resolves an issue with the Git for Windows (MSYS) version of `less`. It _doesn't_ use VT processing for emitting text tothe buffer, so when it hits `WriteCharsLegacy`, `WC_DELAY_EOL_WRAP` is NOT set.
When this happens, `less` is writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting the
```c++
Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
```
call in `_stream.cpp:560`.
The cursor is _currently_ at `{40, 29}`, the _start_ of the run of text that wrapped. We're trying to adjust it to `{0, 30}`, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got to `IncrementCircularBuffer` first, so we can move the cursor there.
When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at `{40, 29}` here, so unfortunately, the `cursorIsInDeferredWrap` check in `XtermEngine::PaintCursor` is `false`. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is `{40, 29}`.
If we're painting the frame because we circled the buffer, then the cursor might still be in the position it was before the text was written to the buffer to cause the buffer to circle. In that case, then we DON'T want to paint the cursor here either, because it'll cause us to manually break this line. That's okay though, the frame will be painted again, after the circling is complete.
## PR Checklist
* [x] Closes #5691
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
I suppose that's the detailed description above
## Validation Steps Performed
* ran tests
* checked that the bug was actually fixed in the Terminal
(cherry picked from commit 3847271)
zadjii-msft
added a commit
that referenced
this pull request
May 12, 2020
This reverts commit 3847271.
zadjii-msft
added a commit
that referenced
this pull request
May 12, 2020
This reverts commit bffca4e.
This was referenced May 12, 2020
DHowett-MSFT
pushed a commit
that referenced
this pull request
May 12, 2020
This PR reverts a relatively minor change that was made incorrectly to ConPTY in #5771. In that PR, I authored two tests. One of them actually caught the bug that was supposed to be fixed by #5771. The other test was simply authored during the investigation. I believed at the time that the test revealed a bug in conpty that was fixed by _removing_ this block of code. However, an investigation itno #5839 revealed that this code was actually fairly critical. So, I'm also _skipping_ this buggy test for now. I'm also adding a specific test case to this bug. The problem in the bugged case of `WrapNewLineAtBottom` is that `WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer, which is causing the cursor to automatically move to the next line in the buffer. This is because `WriteCharsLegacy` isn't being called with the `WC_DELAY_EOL_WRAP` flag. So, in that test case, * The client emits a wrapped line to conpty * conpty fills the bottom line with that text, then dutifully increments the buffer to make space for the cursor on a _new_ bottom line. * Conpty reprints the last `~` of the wrapped line * Then it gets to the next line, which is being painted _before_ the client emits the rest of the line of text to fill that row. * Conpty thinks this row is empty, (it is) and manually breaks the row. However, the test expects this row to be emitted as wrapped. The problem comes from the torn state in the middle of these frames - the original line probably _should_ remain wrapped, but this is a sufficiently rare case that the fix is being punted into the next release. It's possible that improving how we handle line wrapping might also fix this case - currently we're only marking a row as wrapped when we print the last cell of a row, but we should probably mark it as wrapped instead when we print the first char of the _following_ row. That work is being tracked in #5800 ### The real bug in this PR The problem in the `DeleteWrappedWord` test is that the first line is still being marked as wrapped. So when we get to painting the line below it, we'll see that there are no characters to be printed (only spaces), we emit a `^[20X^[20C`, but the cursor is still at the end of the first line. Because it's there, we don't actually clear the text we want to clear. So DeleteWrappedWord, #5839 needs the `_wrappedRow = std::nullopt;` statement here. ## References * I guess just look at #5800, I put everything in there. ## Validation Steps Performed * Tested manually that this was fixed for the Terminal * ran tests Closes #5839
DHowett-MSFT
pushed a commit
that referenced
this pull request
May 12, 2020
This PR reverts a relatively minor change that was made incorrectly to ConPTY in #5771. In that PR, I authored two tests. One of them actually caught the bug that was supposed to be fixed by #5771. The other test was simply authored during the investigation. I believed at the time that the test revealed a bug in conpty that was fixed by _removing_ this block of code. However, an investigation itno #5839 revealed that this code was actually fairly critical. So, I'm also _skipping_ this buggy test for now. I'm also adding a specific test case to this bug. The problem in the bugged case of `WrapNewLineAtBottom` is that `WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer, which is causing the cursor to automatically move to the next line in the buffer. This is because `WriteCharsLegacy` isn't being called with the `WC_DELAY_EOL_WRAP` flag. So, in that test case, * The client emits a wrapped line to conpty * conpty fills the bottom line with that text, then dutifully increments the buffer to make space for the cursor on a _new_ bottom line. * Conpty reprints the last `~` of the wrapped line * Then it gets to the next line, which is being painted _before_ the client emits the rest of the line of text to fill that row. * Conpty thinks this row is empty, (it is) and manually breaks the row. However, the test expects this row to be emitted as wrapped. The problem comes from the torn state in the middle of these frames - the original line probably _should_ remain wrapped, but this is a sufficiently rare case that the fix is being punted into the next release. It's possible that improving how we handle line wrapping might also fix this case - currently we're only marking a row as wrapped when we print the last cell of a row, but we should probably mark it as wrapped instead when we print the first char of the _following_ row. That work is being tracked in #5800 ### The real bug in this PR The problem in the `DeleteWrappedWord` test is that the first line is still being marked as wrapped. So when we get to painting the line below it, we'll see that there are no characters to be printed (only spaces), we emit a `^[20X^[20C`, but the cursor is still at the end of the first line. Because it's there, we don't actually clear the text we want to clear. So DeleteWrappedWord, #5839 needs the `_wrappedRow = std::nullopt;` statement here. ## References * I guess just look at #5800, I put everything in there. ## Validation Steps Performed * Tested manually that this was fixed for the Terminal * ran tests Closes #5839 (cherry picked from commit b4c33dd)
|
🎉 Handy links: |
ghost
mentioned this pull request
jelster
pushed a commit
to jelster/terminal
that referenced
this pull request
May 28, 2020
## Summary of the Pull Request
This PR resolves an issue with the Git for Windows (MSYS) version of `less`. It _doesn't_ use VT processing for emitting text tothe buffer, so when it hits `WriteCharsLegacy`, `WC_DELAY_EOL_WRAP` is NOT set.
When this happens, `less` is writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting the
```c++
Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
```
call in `_stream.cpp:560`.
The cursor is _currently_ at `{40, 29}`, the _start_ of the run of text that wrapped. We're trying to adjust it to `{0, 30}`, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got to `IncrementCircularBuffer` first, so we can move the cursor there.
When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at `{40, 29}` here, so unfortunately, the `cursorIsInDeferredWrap` check in `XtermEngine::PaintCursor` is `false`. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is `{40, 29}`.
If we're painting the frame because we circled the buffer, then the cursor might still be in the position it was before the text was written to the buffer to cause the buffer to circle. In that case, then we DON'T want to paint the cursor here either, because it'll cause us to manually break this line. That's okay though, the frame will be painted again, after the circling is complete.
## PR Checklist
* [x] Closes microsoft#5691
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
I suppose that's the detailed description above
## Validation Steps Performed
* ran tests
* checked that the bug was actually fixed in the Terminal
jelster
pushed a commit
to jelster/terminal
that referenced
this pull request
May 28, 2020
This PR reverts a relatively minor change that was made incorrectly to ConPTY in microsoft#5771. In that PR, I authored two tests. One of them actually caught the bug that was supposed to be fixed by microsoft#5771. The other test was simply authored during the investigation. I believed at the time that the test revealed a bug in conpty that was fixed by _removing_ this block of code. However, an investigation itno microsoft#5839 revealed that this code was actually fairly critical. So, I'm also _skipping_ this buggy test for now. I'm also adding a specific test case to this bug. The problem in the bugged case of `WrapNewLineAtBottom` is that `WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer, which is causing the cursor to automatically move to the next line in the buffer. This is because `WriteCharsLegacy` isn't being called with the `WC_DELAY_EOL_WRAP` flag. So, in that test case, * The client emits a wrapped line to conpty * conpty fills the bottom line with that text, then dutifully increments the buffer to make space for the cursor on a _new_ bottom line. * Conpty reprints the last `~` of the wrapped line * Then it gets to the next line, which is being painted _before_ the client emits the rest of the line of text to fill that row. * Conpty thinks this row is empty, (it is) and manually breaks the row. However, the test expects this row to be emitted as wrapped. The problem comes from the torn state in the middle of these frames - the original line probably _should_ remain wrapped, but this is a sufficiently rare case that the fix is being punted into the next release. It's possible that improving how we handle line wrapping might also fix this case - currently we're only marking a row as wrapped when we print the last cell of a row, but we should probably mark it as wrapped instead when we print the first char of the _following_ row. That work is being tracked in microsoft#5800 ### The real bug in this PR The problem in the `DeleteWrappedWord` test is that the first line is still being marked as wrapped. So when we get to painting the line below it, we'll see that there are no characters to be printed (only spaces), we emit a `^[20X^[20C`, but the cursor is still at the end of the first line. Because it's there, we don't actually clear the text we want to clear. So DeleteWrappedWord, microsoft#5839 needs the `_wrappedRow = std::nullopt;` statement here. ## References * I guess just look at microsoft#5800, I put everything in there. ## Validation Steps Performed * Tested manually that this was fixed for the Terminal * ran tests Closes microsoft#5839
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
This PR resolves an issue with the Git for Windows (MSYS) version of
less. It doesn't use VT processing for emitting text tothe buffer, so when it hitsWriteCharsLegacy,WC_DELAY_EOL_WRAPis NOT set.When this happens,
lessis writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting thecall in
_stream.cpp:560.The cursor is currently at
{40, 29}, the start of the run of text that wrapped. We're trying to adjust it to{0, 30}, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got toIncrementCircularBufferfirst, so we can move the cursor there.When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at
{40, 29}here, so unfortunately, thecursorIsInDeferredWrapcheck inXtermEngine::PaintCursorisfalse. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is{40, 29}.If we're painting the frame because we circled the buffer, then the cursor might still be in the position it was before the text was written to the buffer to cause the buffer to circle. In that case, then we DON'T want to paint the cursor here either, because it'll cause us to manually break this line. That's okay though, the frame will be painted again, after the circling is complete.
PR Checklist
Detailed Description of the Pull Request / Additional comments
I suppose that's the detailed description above
Validation Steps Performed