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

Scrolling down in VIM scrolls up duplicated vim status lines #5161

Closed
0xd4d opened this issue Mar 28, 2020 · 9 comments · Fixed by #5181
Closed

Scrolling down in VIM scrolls up duplicated vim status lines #5161

0xd4d opened this issue Mar 28, 2020 · 9 comments · Fixed by #5181
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@0xd4d
Copy link

0xd4d commented Mar 28, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.18363.720]   
Windows Terminal version (if applicable): 0.10.781.0

vim for Windows (official git build): VIM - Vi IMproved 8.2 (2019 Dec 12, compiled Jan 16 2020 08:26:10)

Steps to reproduce

  • Start vim: vim (the one that comes with git version 2.26.0.windows.1)
  • Press i
  • Press ENTER until it starts scrolling the screen
  • (to exit vim): ESC and then type :q! and then ENTER 😄

Expected behavior

The status line stays where it's supposed to be, at the bottom of the screen.

Actual behavior

Every time a the screen is scrolled down, the last line (white status line) is scrolled up, and a new white status line is printed at the bottom. This results in two white lines, then 3, etc...

This does not reproduce if I use the old cmd.exe (started by eg. Explorer, not wt.exe)

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Mar 28, 2020
@avysk
Copy link

avysk commented Mar 29, 2020

I can reproduce this with vim which comes with git; if I use vim-tux build from chocolatey, it works normally.

@0xd4d
Copy link
Author

0xd4d commented Mar 31, 2020

Does vim-tux have a status line with a white background? If it's the same color as the background, you won't see it.

@avysk
Copy link

avysk commented Mar 31, 2020

Does vim-tux have a status line with a white background?

Yes.

image

EDIT: I can be wrong. I have number and rnu set; setting them in git vim manually prevents the problem from reproducing. I'll try if I can reproduce this in vim-tux without those settings.

EDIT2 I cannot reproduce problem In vim-tux just by commenting out set number and set rnu in my config.

EDIT3 I have tried to remove my vim settings completely and run vim-tux as vim -u "C:\Program Files\Git\etc\vimrc" -- the problem is not reproducible.

@avysk
Copy link

avysk commented Mar 31, 2020

...and here's git vim (notice that it's broken in some other way as well):
image

@DHowett-MSFT
Copy link
Contributor

Well would you look at that, it reproduces immediately.

The behavior is slightly different on 0.11-pre: the line occasionally flickers, or the cursor jumps below the line, but it doesn't get copied onto the screen repeatedly. It's probably impacted by #5181.

Idly, I wonder if this bug will go away when git-for-windows updates to cygwin >3.1, which changes how VT is parsed.

@DHowett-MSFT DHowett-MSFT added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 3, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 3, 2020
@DHowett-MSFT
Copy link
Contributor

@zadjii-msft this is one to keep an eye on: Git Vim isn't using vt, but this seems like another deferred newline issue. It looks like it's fixed with #5181, but I only have your not-latest changes and we need to make sure it doesn't regress.

Simple repro: run \program files\git\bin\bash -li then vim. press i and hit enter until it goes off the bottom of the screen.

0.10 pty tap on top, 0.11 on bottom

                                       BAD LF HERE, no other diff
                                       V
␛[m␛[?25l␛[30;1H␛[?25h␛[?25l␛[30m␛[107m␊␛[m␛[28;1H␛[30m␛[107m␍␊[No␣Name][+]␣[unix]␣(15:59␣31/12/1969)␣␣␣␣␣␣␣␣␣␣␣␣30,1␣Bot␛[97m␛[49m--␣INSERT␣--␛[K␛[28;1H␛[?25h
␛[m␛[?25l␛[30;1H␛[?25h␛[?25l␛[30m␛[107m␛[m␛[28;1H␛[30m␛[107m␍␊[No␣Name][+]␣[unix]␣(15:59␣31/12/1969)␣␣␣␣␣␣␣␣␣␣␣␣30,1␣Bot␛[97m␛[49m--␣INSERT␣--␛[K␛[28;1H␛[?25h

weird thing is, it prints the lf before it goes and strikes the status line again...

@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.x milestone Apr 3, 2020
@DHowett-MSFT DHowett-MSFT self-assigned this Apr 3, 2020
@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 3, 2020

Oh shoot this isn't fixed in my latest changes, but I'll try and make a repro case for this.


EDIT: I think this is because we're newlining to add a new row, then moving the cursor to the line the old status bar is on, then we're not clearing it, because the the bottom line is new (_newBottomLine=true), and then we're moving down to the new location of the status bar, printing that, then printing the INSERT line.


EDIT2: Okay I've got what I think is a patch for this, but I'm not sure that it doesn't make things horribly worse:

Change the init of removeSpaces in VtEngine::_PaintUtf8BufferLine to the following:

    const bool removeSpaces = (useEraseChar || (_clearedAllThisFrame) || (_newBottomLine && coord.Y == _lastViewport.BottomInclusive()));

and the problem goes away. Now I just need to write a test for this, and validate the other tests don't regress too.

@PistachioCake
Copy link

I'm so glad I found this here, I've been trying to solve this for weeks and am new to the whole open-source thing 😅

To add: I could not reproduce this issue using Ubuntu WSL and its associated vim (version 8.0.1453), but it does occur in git bash with its associated vim (version 8.1.2292). It does not occur when either are run as individual processes, not through Windows Terminal.

zadjii-msft added a commit that referenced this issue Apr 3, 2020
@ghost ghost added the In-PR This issue has a related PR label Apr 6, 2020
@ghost ghost closed this as completed in #5181 Apr 9, 2020
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
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Apr 9, 2020
@ghost
Copy link

ghost commented Apr 22, 2020

🎉This issue was addressed in #5181, which has now been successfully released as Windows Terminal Preview v0.11.1121.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants