Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an accidental regression from #5771 #5870

Merged
merged 5 commits into from
May 12, 2020

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented 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

Validation Steps Performed

  • Tested manually that this was fixed for the Terminal
  • ran tests

Closes #5839

@ghost ghost added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty labels May 12, 2020
@zadjii-msft zadjii-msft mentioned this pull request May 12, 2020
31 tasks
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensively documented. Thank you

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp. OK.

@DHowett-MSFT DHowett-MSFT merged commit b4c33dd into master May 12, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/migrie/b/5839-ctrl-bs-regression branch May 12, 2020 22:02
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)
@ghost
Copy link

ghost commented May 13, 2020

🎉Windows Terminal Release Candidate v0.11.1333.0 (1.0rc2) has been released which incorporates this pull request.:tada:

Handy links:

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty
Projects
None yet
3 participants