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.

Sign up for GitHub

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

Jump to bottom

Screen content does not update in tmux #5104

Closed
kasper93 opened this issue Mar 24, 2020 · 6 comments · Fixed by #5122
Closed

Screen content does not update in tmux #5104

kasper93 opened this issue Mar 24, 2020 · 6 comments · Fixed by #5122
Assignees
@miniksa
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 Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.0

Comments

@kasper93
Copy link
Contributor

Environment

Windows 10.0.18363.0
tmux over ssh in cygwin

Steps to reproduce

  1. Attach to tmux session
  2. Fill the screen for example with echo test multiple times
  3. Keep printing test, you should alternate with some different text to see if console is scrolled properly.

Expected behavior

Whole screen buffer is redrawn

Actual behavior

Only first line in terminal is redrawn, the rest of the screen space is static and doesn't update while it should. If the output is one line (i.e without new line). If I do echo 'test\n' whole screen is properly redrawn.

Bisect result

This is regression from this commit ca33d89 before that everything was working fine.

// CC @miniksa
@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 24, 2020
@kasper93 kasper93 changed the title Screen contetnt does not update in tmux Screen content does not update in tmux Mar 24, 2020
@miniksa miniksa self-assigned this Mar 24, 2020
@zadjii-msft zadjii-msft 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-Terminal The new Windows Terminal. Product-Conpty For console issues specifically related to conpty and removed Product-Terminal The new Windows Terminal. labels Mar 24, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 24, 2020
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Mar 24, 2020
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 24, 2020
@miniksa
Copy link
Member

miniksa commented Mar 25, 2020

OK I've managed to reproduce this. I think it's a bug that's been there a while and was just hidden by the fact that the previous invalidation-mechanism would coalesce both invalid regions into repainting the entire display. I need to figure out how to dispatch the appropriate content scrolling from the PTY to the Terminal because the entire point of the til::bitmap change was to not re-emit massive amounts of data that are already on the other side of the fence.

@miniksa miniksa mentioned this issue Mar 25, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Mar 25, 2020
@skyline75489
Copy link
Collaborator

I've encountered a similar issue when using Vim in WSL. The majority of content is not being invalidated while scrolling up and down using keyboard.

图片

The actual content should be line 80 - line 107. But only the first and last line is correctly displayed.

@DHowett-MSFT
Copy link
Contributor

@skyline75489 while you've got a local build, would you mind trying out #5122? It's the speculative fix 😄

@skyline75489
Copy link
Collaborator

@DHowett-MSFT It's working! Vim is back to normal with the help of #5122.

@kasper93
Copy link
Contributor Author

I can also confirm, working fine with the fix.

@ghost ghost closed this as completed in #5122 Mar 27, 2020
@ghost ghost removed the In-PR This issue has a related PR label Mar 27, 2020
ghost pushed a commit that referenced this issue Mar 27, 2020
@miniksa
Correct scrolling invalidation region for tmux in pty w/ bitmap (#5122)
ef80f66 
Correct scrolling invalidation region for tmux in pty w/ bitmap

Add tracing for circling and scrolling operations. Fix improper
invalidation within AdjustCursorPosition routine in the subsection about
scrolling down at the bottom with a set of margins enabled.

## References
- Introduced with #5024 

## Detailed Description of the Pull Request / Additional comments
- This occurs when there is a scroll region restriction applied and a
  newline operation is performed to attempt to spin the contents of just
  the scroll region. This is a frequent behavior of tmux.
- Right now, the Terminal doesn't support any sort of "scroll content"
  operation, so what happens here generally speaking is that the PTY in
  the ConHost will repaint everything when this happens.
- The PTY when doing `AdjustCursorPosition` with a scroll region
  restriction would do the following things:

1. Slide literally everything in the direction it needed to go to take
   advantage of rotating the circular buffer. (This would force a
   repaint in PTY as the PTY always forces repaint when the buffer
   circles.)
2. Copy the lines that weren't supposed to move back to where they were
   supposed to go.
3. Backfill the "revealed" region that encompasses what was supposed to
   be the newline.

- The invalidations for the three operations above were:

1. Invalidate the number of rows of the delta at the top of the buffer
   (this part was wrong)
2. Invalidate the lines that got copied back into position (probably
   unnecessary, but OK)
3. Invalidate the revealed/filled-with-spaces line (this is good).

- When we were using a simple single rectangle for invalidation, the
  union of the top row of the buffer from 1 and the bottom row of the
  buffer from 2 (and 3 was irrelevant as it was already unioned it)
  resulted in repainting the entire buffer and all was good.

- When we switched to a bitmap, it dutifully only repainted the top line
  and the bottom two lines as the middle ones weren't a consequence of
  intersect.

- The logic was wrong. We shouldn't be invalidating rows-from-the-top
  for the amount of the delta. The 1 part should be invalidating
  everything BUT the lines that were invalidated in parts 2 and 3.
  (Arguably part 2 shouldn't be happening at all, but I'm not optimizing
  for that right now.)

- So this solves it by restoring an entire screen repaint for this sort
  of slide data operation by giving the correct number of invalidated
  lines to the bitmap.

## Validation Steps Performed
- Manual validation with the steps described in #5104
- Automatic test `ConptyRoundtripTests::ScrollWithMargins`.

Closes #5104
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Mar 27, 2020
@ghost
Copy link

ghost commented Apr 22, 2020

🎉This issue was addressed in #5122, 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
Assignees

@miniksa miniksa

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 Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.0
Development

Successfully merging a pull request may close this issue.

Correct scrolling invalidation region for tmux in pty w/ bitmap microsoft/terminal
5 participants
@kasper93 @skyline75489 @DHowett-MSFT @miniksa @zadjii-msft