ED2 inserts loads of blank lines in scrollback

[j4james]

Windows Terminal version

1.19.3172.0

Windows build number

10.0.19045.3930

Other Software

No response

Steps to reproduce

1. Open a WSL bash shell in Windows Terminal.

2. Execute the following set of commands:

   cd /bin
   ls -la
   printf "\e[3J"
   ls -la
   printf "\e[3J"
   printf "\e[2J"

3. Scroll up to view the scrollback buffer.

Expected Behavior

You should see the last lines of the /bin directory listing and the two printf commands.

Actual Behavior

The buffer immediately above the visible viewport is blank, and you have to scroll back for pages to find the previous content.

Reproducing this might be a bit tricky, because if I replace the ls -la with something like seq 1000, the bug isn't triggered. There must be something about the directory listing that's a factor, and hopefully it's not specific to my directory content. And in case it makes any difference, my screen size is 80x24.

It's also worth noting that I can reproduce this in a recent version of OpenConsole, but not in my inbox ConHost (Windows 10), so it looks like it might be a regression.

[lhecker]

It seems that this issue occurs as soon as a line wrap occurs (in my case when the window is <98 columns wide).

[j4james]

FYI, it looks like this broke in commit 7474839 (PR #15701), although I'm not sure why.

Edit: After that commit, the GetLastNonSpaceCharacter call in AdaptDispatch::_EraseAll started returning the wrong coordinates, which explains the results we're seeing. However, I didn't see any significant changes to the function in that commit, so I'm assuming the issue is deeper than that.

[lhecker]

I found that the culprit is the change to ROW::MeasureRight which now uses the _wrapForced member to calculate the row width. That's problematic, because AdeptDispatch::_EraseScrollback uses _FillRect which does not reset the _wrapForced member. That row is then considered non-empty which causes the large number of empty lines.

I'm not sure if I'm seeing this correctly but I believe that's a preexisting bug in _EraseScrollback that simply got uncovered now, right?

I'm working on a change now that replaces the _FillRect call with some code that scrolls the viewport to the absolute start of the TextBuffer so that we can MEM_DECOMMIT the remaining backing buffer efficiently.

[j4james]

That's problematic, because AdeptDispatch::_EraseScrollback uses _FillRect which does not reset the _wrapForced member.

I think that issue may have been introduced by PR #15541. The original _FillRect implementation used a TextBuffer::WriteLine call with the wrap parameter set to false, so that would previously have reset _wrapForced.

I'm working on a change now that replaces the _FillRect call with some code that scrolls the viewport to the absolute start of the TextBuffer so that we can MEM_DECOMMIT the remaining backing buffer efficiently.

That sounds like a good idea, but I think that still leaves a bug in the _EraseAll implementation, which possibly has existed from the start. That call to GetLastNonSpaceCharacter should really be limited to the current VT page. Otherwise if there happened to be content in the "scroll forward" area (which I think could happen if you're mixing legacy console API calls with VT output), then an ED2 sequence could still end up panning too far down.

[lhecker]

Should _FillRect set the wrap flag again? Considering delayed EOL wraps it shouldn't do that I think?

I'm not entirely sure I understand why _EraseAll calls GetLastNonSpaceCharacter so I may be unqualified to fix it. I would've assumed it would simply move the current _api.GetViewport() up by one viewport.height...

[lhecker]

Ah, I think I can see now why it does it that way. Gnome Terminal scrolls just enough to move the current content into the scrollback (i.e. 3 lines of 3 lines are visible). What I find somewhat curious is that a second \e[2J will however scroll the buffer by an entire viewport height. Is that a bug in Gnome Terminal?

I believe the proper way to fix _EraseAll is to stop treating _virtualBottom as the literal bottom of the viewport, but rather as the last line that was ever written. The bottom of the viewport would then be std::max(_virtualBottom, viewport.height). This would also simplify some stuff in TextBuffer so it may be worthwhile to store the value there instead of in SCREEN_INFORMATION. I think this is better than GetLastNonSpaceCharacter because I get the feeling that the reliance on "whitespace = empty" throughout various parts of the code base makes our behavior appear unpredictable/inconsistent at times.

For now I'll submit just the _EraseScrollback as this will fix the _wrapForced bug and make the above work. But I'm curious what you think about my previous comment regarding _FillRect: Should it affect the _wrapForced flag again? Or are there other parts of AdaptDispatch that may be broken?

[j4james]

What I find somewhat curious is that a second \e[2J will however scroll the buffer by an entire viewport height. Is that a bug in Gnome Terminal?

I think it might be a bug, yeah. It's mentioned in VTE's issue 1471. Personally I think I'd prefer if \e[2J didn't scroll anything at all, which is the way XTerm works. That was something that was also discussed in that VTE issue, and we've previously considered it here as well (#7894).

I believe the proper way to fix _EraseAll is to stop treating _virtualBottom as the literal bottom of the viewport, but rather as the last line that was ever written. The bottom of the viewport would then be std::max(_virtualBottom, viewport.height).

You may be right, but all I can remember of _virtualBottom was that it was a nightmare to work with. I think there are dozens of special cases you have to deal with, and it's easy to break something unknowingly. I wouldn't want to touch it.

[j4james]

regarding _FillRect: Should it affect the _wrapForced flag again?

Forgot to respond to this, but I'm not really sure what the correct behavior should be. My first thought was that any operation that writes to the buffer should reset the flag on the lines it touches, because in a pure wrapping scenario that should never happen. Once you've wrapped a line, you shouldn't really be going back to change it. And if you did go back, like a shell prompt where you've backspaced onto the previous line, then I think that probably should "unwrap" it.

I suspect it's more complicated than that, though, and maybe we need to look at what other terminals are doing. But for now I'd be inclined to just put it back to what it was (i.e. resetting the flag when FillRect wraps a line). If nobody was complaining about that behavior before, it's probably good enough for most use cases.

[j4james]

It just occurred to me that there's another complication with _wrapForced being reset. It's all very well for us to decide that certain operations should reset that flag, we also need a way to propagate those changes over conpty. I'm not sure how we would do that, but it would need to be something that works on third-party terminals as well.

[lhecker]

I've been thinking some more about _FillRect and I'd prefer not changing _wrapForced. I thought _FillRect's purpose was something akin to a sequence of CUP/etc. followed by plain text, and such an operation wouldn't change the wrap flag since wrapping is delayed by default, right? If _FillRect was different, it would mean that we'd need 2 different behaviors for writing text, even after I've removed the remaining uses of OutputCellIterator, which I'd like to avoid if possible. But it's possible that I misunderstood the _FillRect's intended behavior, so please let me know if I did!

This is only tangentially related, but in the future I'd prefer if we were to store \n directly inside the buffer's text without any trailing white-space padding in each line (it would only be added lazily). This would allow us to use other buffer algorithms in the future, for instance a plain gap buffer with in-band metadata (e.g. VT instead of separately stored, RLE compressed structs). This would allow us to more easily implement "infinite" scrollback, since the backing buffer could be allocated using a plain circular buffer. The tangent is that such a goal would require an text writing behavior that is as constrained as possible so that we'd hopefully only have 1 (or very very few) text insertion routine(s), as I'm sure that even then it would already be quite a complex algorithm.

[DHowett]

e.g. VT instead of separately stored, RLE compressed structs

I still must object in the strongest possible but still civil terms to this. It's been a year, and I am still not convinced that we should store text in wire format. 🙂

[lhecker]

What I brought up doesn't work anyways unless I build it and I haven't, so in a way it's just an... anecdote? Or maybe, rather an example. When I said "it would allow us" I meant it quite literally (i.e. as a hypothetical) and that's important to me, because...

My point was basically that building on top of an existing architecture to introduce new behavior may prevent us from efficiently switching the underlying architecture if we ever chose to. Not just regarding the text buffer but any other part of any project. Both conhost and WT are fairly tightly entangled between different components. Making it so that _FillRect can be written on top of plain cursor positioning + text writes will simplify our code base, as it is one less place that needs special casing and/or needs to reach into TextBuffer internals.

To give more examples, the rendering interface is also problematic (as I mentioned before), as the InvalidateFoo() calls are spread throughout the entire code base. Forgetting to call the an invalidation function means that something breaks in subtle ways that are hard to trace (in particular when VtEngine is used). Calling it too often (as is the case right now) means sacrificing huge amounts of performance (the invalidation calls make up ~half of all CPU time in ConPTY). They're also difficult to review and to use. For the TextBuffer in particular we have the public GetRowByOffset function for instance which means that dozens of files inspect the underlying ROW struct which makes it very difficult to change the underlying attribute storage, remove the OutputCell iterator functions, or to do many other things.
To counter this, I've been working towards unifying the various OutputCellIterator writers to a single "write plain text with this attribute" function for this reason. Even something like the CHAR_INFO APIs would be replaced with a loop of TextBuffer::Write calls with plain text and attributes, if it's feasible to do so. It will centralize our efforts into a single function that needs to be reviewed, which will reduce bugs (less lines ~= less bugs), simplify maintenance and improve performance.