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

Bug: resizing doesn't work properly in Windows Terminal #1465

Closed
Shorotshishir opened this issue Jun 23, 2019 · 16 comments · Fixed by #4741
Closed

Bug: resizing doesn't work properly in Windows Terminal #1465

Shorotshishir opened this issue Jun 23, 2019 · 16 comments · Fixed by #4741
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. Needs-Tag-Fix Doesn't match tag requirements 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

Comments

@Shorotshishir
Copy link

Environment

Windows build number: [Version 10.0.18362.175]
Windows Terminal version (0.2.1715.0):

Steps to reproduce

open any terminal (PowerShell/cmd/wsl). type any command (e.g. dir, ver, uname) and enter. resize the app window to the minimum and gradually enlarge the window.

Expected behavior

information should stay the same. shouldn't break and should render all the characters like below

powershell0
wsl0
cmd0

Actual behavior

breaks apart and doesn't render properly

powershell1
wsl1
cmd1

@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 Jun 23, 2019
@Shorotshishir Shorotshishir changed the title Bug Report Bug Report: text breaks apart while resizing app window Jun 23, 2019
@JushBJJ
Copy link

JushBJJ commented Jun 23, 2019

Kinda like #1433 but this time, its with all command lines.

@HBelusca
Copy link
Contributor

HBelusca commented Jun 24, 2019

Here is a screen movie to illustrate the described problems above (note how the scrollbar modifies):
2019-06-24_14-16-55

Auxiliary observations:

  • VtPipeTerm.exe misbehaves as well when resizing either of the background host window, or the front window.
  • OpenConsole.exe does not have this problem, but acts very slowly when trying to resize the window (why?)

@DHowett-MSFT
Copy link
Contributor

OpenConsole in debug or in release?

If vtpipeterm is hitting it too, that's definitely a conpty issue.

@HBelusca
Copy link
Contributor

(I should have added that I was testing with master a5f31f7 .

OpenConsole in debug or in release?

OK the slowness was just in debug mode; I recompiled and re-tested in release mode and it's back to the usual speed ("fast").

If vtpipeterm is hitting it too, that's definitely a conpty issue.

For this my interpretation of what's going on may be erroneous because I'm also looking at the background host window, while the front window only allows resizing but not scrolling. See the animation below.
2019-06-24_19-36-51

@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 Jun 27, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 27, 2019
@DHowett-MSFT DHowett-MSFT changed the title Bug Report: text breaks apart while resizing app window Bug: resizing doesn't work properly in Windows Terminal Jun 27, 2019
@parkovski
Copy link

parkovski commented Jun 29, 2019

I think I found one resize bug. A ConPTY sends two WINDOW_BUFFER_SIZE_EVENT messages when the window is restored from maximize, but the first one has the wrong size (not sure of the internals of this, but it appears in conhost/tmux/pwsh, alacritty/pwsh, and Windows Terminal).

Run this program as conevents -es in conhost and a different terminal to see the difference; maximize and restore the window. For example, restoring the WinTerm window reports 119 x 46 in the WINDOW_BUFFER_SIZE_EVENT message, but GetConsoleScreenBufferInfo returns the correct value 119 x 32.

@zadjii-msft
Copy link
Member

I'm moving the conpty maximize thing to its own separate issue, #1765.

This is definitely a conpty bug, but I'm not really sure the right way to fix it. If I had to guess, when we repaint the buffer due to a resize, we're painting more or less lines than the terminal expects, which is causing the old viewport contents to slide into the scrollback.

This is pretty bad and I really want to find some time to fix it.

@mcpiroman
Copy link
Contributor

mcpiroman commented Jul 4, 2019

My best bet to fix or at least simplify this is to not emit new resized buffer from conhost, but instead run the resize on terminall's buffer too. This way both buffers should be the same and additionally terminal will have access to context of the resize, which would e.g. enable us to track selection transform (#1368), and maybe to fix the bugs above too.

It should be at least as (in)efficient as it is now (especially over network), because despite doubled work, conhost wouldn't need to transfer the buffer back to terminal, which seems comparatively costly.

It could eventually be futher optimizied so thet when user drags window the resize runs only on terminal's buffer and is reported to conhost only when the user releases LMB (but this would reqiure to freeze sth like the conhost does when scrolling?).

And since you want to support existing 3-party terminals - they handle resize themselfs. So while the present solution should work, it will be unusual to them and may cause some troubles such as we have now (even if we resolve them on terminal side).

And finally, in passthrough mode terminal would have to resize the buffer itself.

@zadjii-msft
Copy link
Member

Currently, the terminal does also resize it's buffer at the same time as when it requests a resize with the conpty. We're using an older resize algorithim right now in the Terminal, that only does a "traditional resize". The terminal doesn't try to reflow text in its buffer at all (like conhost does). The result of this is that when there's text outside the viewport in the terminal, the rows of text are going to get clipped to the width of the terminal. If you resize down to 5 chars wide, every row is going to only have 5 chars in it.

Right now, when the console (conpty) resizes, it might reflow text, to try and maintain line wrapping. If conpty didn't re-emit the state of its buffer after the resize, then a terminal that didn't support resize with reflow would be in a torn state with the pseudoconsole.

What we should be doing is reusing conhost's ResizeWithReflow in the terminal. It's a method that's a little tightly tied to conhost itself currently, which is why we haven't had a chance to pull it out quite yet.

Now, there might actually be a better way to handle resizes that aren't so chatty. There's probably a class of resize operations that don't need to force a repaint. We might even want to experiment with having conpty disable resize with reflow entirely, though that might be a strictly worse experience (as text that's cut off from the conpty will be lost forever, and when it's repainted, the terminal will lose it too).

Part of the oddity of all this is due to the fact that conpty has to maintain its own buffer state - passthrough mode might not have so much difficulty with this scenario.

We could almost certainly have a mode for the terminal where it doesn't resize until the drag handle is released. That deserves its own issue (#2140)

@mcpiroman
Copy link
Contributor

Couldn't we just ResizeWithOverflow on both conhost and terminal, without any rendering and repainting in the process (VT, not graphical rendering ofc)? If the input (buffer) is the same and we run the same operation, then output should also be the same.

@danielskowronski
Copy link

danielskowronski commented Oct 7, 2019

If I set "historySize" : 90000, resizing will be broken vs "historySize" : 9000, it will work fine. Version: 0.5.2762.0

OMG, it really works! version: 0.5.2681.0

@zadjii-msft
Copy link
Member

Another repro from another thread:

Steps to reproduce

  1. SSH to a linux. I tired centos7.
  2. Maximize the windows terminal window.
  3. Enter a command which print something to the terminal. for example "ls".
  4. Restore down the window.
  5. keep pressing enter to create new lines.

Expected behavior

Nothing happens.

Actual behavior

Previous message is shown again.

bugreportjpg

@espresso3389
Copy link

If I set "historySize" : 90000, resizing will be broken vs "historySize" : 9000, it will work fine. Version: 0.5.2762.0

It seems that the historySize limit seems 32K - possible_screen_rows. So if your terminal can have 120 lines at most, historySize should be 32648 or less. So practically, the value around 32600 can be good enough; 9999 is too small for me anyway.

@zadjii-msft zadjii-msft self-assigned this Jan 16, 2020
@ghost ghost added the In-PR This issue has a related PR label Feb 28, 2020
DHowett-MSFT pushed a commit that referenced this issue Mar 13, 2020
This PR adds support for "Resize with Reflow" to the Terminal. In
conhost, `ResizeWithReflow` is the function that's responsible for
reflowing wrapped lines of text as the buffer gets resized. Now that
#4415 has merged, we can also implement this in the Terminal. Now, when
the Terminal is resized, it will reflow the lines of it's buffer in the
same way that conhost does. This means, the terminal will no longer chop
off the ends of lines as the buffer is too small to represent them. 

As a happy side effect of this PR, it also fixed #3490. This was a bug
that plagued me during the investigation into this functionality. The
original #3490 PR, #4354, tried to fix this bug with some heavy conpty
changes. Turns out, that only made things worse, and far more
complicated. When I really got to thinking about it, I realized "conhost
can handle this right, why can't the Terminal?". Turns out, by adding
resize with reflow, I was also able to fix this at the same time.
Conhost does a little bit of math after reflowing to attempt to keep the
viewport in the same relative place after a reflow. By re-using that
logic in the Terminal, I was able to fix #3490.

I also included that big ole test from #3490, because everyone likes
adding 60 test cases in a PR.

## References
* #4200 - this scenario
* #405/#4415 - conpty emits wrapped lines, which was needed for this PR
* #4403 - delayed EOL wrapping via conpty, which was also needed for
  this
* #4354 - we don't speak of this PR anymore

## PR Checklist
* [x] Closes #1465
* [x] Closes #3490
* [x] Closes #4771
* [x] Tests added/passed

## EDIT: Changes to this PR on 5 March 2020

I learned more since my original version of this PR. I wrote that in
January, and despite my notes that say it was totally working, it
_really_ wasn't.

Part of the hard problem, as mentioned in #3490, is that the Terminal
might request a resize to (W, H-1), and while conpty is preparing that
frame, or before the terminal has received that frame, the Terminal
resizes to (W, H-2). Now, there aren't enough lines in the terminal
buffer to catch all the lines that conpty is about to emit. When that
happens, lines get duplicated in the buffer. From a UX perspective, this
certainly looks a lot worse than a couple lost lines. It looks like
utter chaos.

So I've introduced a new mode to conpty to try and counteract this
behavior. This behavior I'm calling "quirky resize". The **TL;DR** of
quirky resize mode is that conpty won't emit the entire buffer on a
resize, and will trust that the terminal is prepared to reflow it's
buffer on it's own.

This will enable the quirky resize behavior for applications that are
prepared for it. The "quirky resize" is "don't `InvalidateAll` when the
terminal resizes". This is added as a quirk as to not regress other
terminal applications that aren't prepared for this behavior
(gnome-terminal, conhost in particular). For those kinds of terminals,
when the buffer is resized, it's just going to lose lines. That's what
currently happens for them.  

When the quirk is enabled, conpty won't repaint the entire buffer. This
gets around the "duplicated lines" issue that requesting multiple
resizes in a row can cause. However, for these terminals that are
unprepared, the conpty cursor might end up in the wrong position after a
quirky resize.

The case in point is maximizing the terminal. For maximizing
(height->50) from a buffer that's 30 lines tall, with the cursor on
y=30, this is what happens: 

  * With the quirk disabled, conpty reprints the entire buffer. This is
    60 lines that get printed. This ends up blowing away about 20 lines
    of scrollback history, as the terminal app would have tried to keep
    the text pinned to the bottom of the window. The term. app moved the
    viewport up 20 lines, and then the 50 lines of conpty output (30
    lines of text, and 20 blank lines at the bottom) overwrote the lines
    from the scrollback. This is bad, but not immediately obvious, and
    is **what currently happens**. 


  * With the quirk enabled, conpty doesn't emit any lines, but the
    actual content of the window is still only in the top 30 lines.
    However, the terminal app has still moved 20 lines down from the
    scrollback back into the viewport. So the terminal's cursor is at
    y=50 now, but conpty's is at 30. This means that the terminal and
    conpty are out of sync, and there's not a good way of re-syncing
    these. It's very possible (trivial in `powershell`) that the new
    output will jump up to y=30 override the existing output in the
    terminal buffer. 

The Windows Terminal is already prepared for this quirky behavior, so it
doesn't keep the output at the bottom of the window. It shifts it's
viewport down to match what conpty things the buffer looks like.

What happens when we have passthrough mode and WT is like "I would like
quirky resize"? I guess things will just work fine, cause there won't be
a buffer behind the passthrough app that the terminal cares about. Sure,
in the passthrough case the Terminal could _not_ quirky resize, but the
quirky resize won't be wrong.
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements 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 Mar 13, 2020
@lo-w
Copy link

lo-w commented Mar 16, 2020

@jjoos thanks, it works, the max num seems 32 723,
"historySize": 32723,

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. Needs-Tag-Fix Doesn't match tag requirements 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
Development

Successfully merging a pull request may close this issue.