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

One cell at the right end of the screen is ignored #357

Closed
ghost opened this issue Jan 20, 2019 · 4 comments
Closed

One cell at the right end of the screen is ignored #357

ghost opened this issue Jan 20, 2019 · 4 comments
Assignees
Labels
Area-VT Virtual Terminal sequence support Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Milestone

Comments

@ghost
Copy link

ghost commented Jan 20, 2019

One cell at the right end of the screen is ignored by ConPTY (18317). (17763) does not.
I tried it with ConPTY sandwiched by Vim, but it does not look like this in winpty.
It uses VMware12.
It seems that backspace is shifted to the left by one cell when you hit repeatedly across lines.

windows-10-insider---vmware-workstation-2019-01-20-13-49-46

@ghost
Copy link
Author

ghost commented Jan 20, 2019

I felt like Vim's problem. I will review the code as well.

@ghost
Copy link
Author

ghost commented Jan 22, 2019

When erasing the character at the right end of the screen, a blank character is output at the right end of the screen, but it seems that there is not enough sequence (0x0D 0x0A) to bring the cursor to the next line.

Therefore, the cursor does not reach the cell at the right end of the screen in the immediately following sequence (\b).

@miniksa miniksa added Product-Conpty For console issues specifically related to conpty Area-VT Virtual Terminal sequence support labels Jan 22, 2019
@zadjii-msft zadjii-msft self-assigned this Jan 22, 2019
@zadjii-msft zadjii-msft added the Work-Item It's being tracked by an actual work item internally. (to be removed soon) label Jan 22, 2019
@zadjii-msft zadjii-msft added this to the 19H1 milestone Jan 22, 2019
@zadjii-msft
Copy link
Member

Okay, so to make this clear, his is something that's regressed since RS5 (17763).
Looks like even in wsl interop this repros. I've filed MSFT:20266233 to make sure this gets fixed.

image

@DHowett-MSFT DHowett-MSFT added the Resolution-Fix-Available It's available in an Insiders build or a release label Feb 15, 2019
@DHowett-MSFT
Copy link
Contributor

Hey, I forgot to tag this one! It's fixed as of insider build 18834.

ghost pushed a commit that referenced this issue Feb 11, 2020
…an EOL wrap (#4403)

## Summary of the Pull Request

This is a fix that technically was caused by #357, though we didn't have the Terminal at the time, so I only fixed conhost then. When a client app prints the very last column in the buffer, the cursor is often not _actually_ moved to the next row quite yet. The cursor usually just "floats" on the last character of the row, until something happens. This could be a printable character, which will print it on the next line, or a newline, which will move the cursor to the next line manually, or it could be a backspace, which might take the cursor back a character. 

Conhost and gnome-terminal behave slightly differently here, and wt behaves differently all together. Heck, conhost behaves differently depending on what output mode you're in. 

The scenario in question is typing a full row of text, then hitting backspace to erase the last char of the row.

What we were emitting before in this case was definitely wrong - we'd emit a space at that last row, but then not increment our internal tracker of where the cursor is, so the cursor in conpty and the terminal would be misaligned. The easy fix for this is to make sure to always update the `_lastText` member appropriately. This is the `RightExclusive` change.

The second part of this change is to not be so tricksy immediately following a "delayed eol wrap". When we have just printed the last char like that, always use the VT sequence CUP the next time the cursor moves. Depending on the terminal emulator and it's flags, performing a BS in this state might not bring the cursor to the correct position. 

## References

#405, #780, #357

## PR Checklist
* [x] Closes #1245
* [x] I work here
* [ ] Tests added/passed 
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

With the impending #405 PR I have, this still works, but the sequences that are emitted change, so I didn't write a test for this currently.

## Validation Steps Performed

Tried the scenario for both #357 and #1245 in inception, `gnome-temrinal` and `wt` all, and they all display the cursor correctly.
ghost pushed a commit that referenced this issue Apr 9, 2020
Now that the Terminal is doing a better job of actually marking which
lines were and were not wrapped, we're not always copying lines as
"wrapped" when they should be. We're more correctly marking lines as not
wrapped, when previously we'd leave them marked wrapped.

The real problem is here in the `ScrollFrame` method - we'd manually
newline the cursor to make the terminal's viewport shift down to a new
line. If we had to scroll the viewport for a _wrapped_ line, this would
cause the Terminal to mark that line as broken, because conpty would
emit an extra `\n` that didn't actually exist.

This more correctly implements `ScrollFrame`. Now, well move where we
"thought" the cursor was, so when we get to the next `PaintBufferLine`,
if the cursor needs to newline for the next line, it'll newline, but if
we're in the middle of a wrapped line, we'll just keep printing the
wrapped line.

A couple follow up bugs were found to be caused by the same bad logic.
See #5039 and #5161 for more details on the investigations there.

## References

* #4741 RwR, which probably made this worse
* #5122, which I branched off of 
* #1245, #357 - a pair of other conpty wrapped lines bugs
* #5228 - A followup issue for this PR

## PR Checklist
* [x] Closes #5113
* [x] Closes #5180 (by fixing DECRST 25)
* [x] Closes #5039
* [x] Closes #5161 (by ensuring we only `removeSpaces` on the actual
  bottom line)
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

* Checked the cases from #1245, #357 to validate that they still work
* Added more and more tests for these scenarios, and then I added MORE
  tests
* The entire team played with this in selfhost builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Projects
None yet
Development

No branches or pull requests

3 participants