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

ED3 in the alt buffer should erase the main scrollback buffer #3686

Open
j4james opened this issue Nov 24, 2019 · 6 comments
Open

ED3 in the alt buffer should erase the main scrollback buffer #3686

j4james opened this issue Nov 24, 2019 · 6 comments
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Milestone

Comments

@j4james
Copy link
Collaborator

j4james commented Nov 24, 2019

Environment

Windows build number: Version 10.0.18362.356
Also tested with a recent build from source: 171f00c

Steps to reproduce

  1. Start a WSL bash shell in conhost.
  2. Do a long directory listing so you get some content in the backscroll buffer.
  3. Execute this command: printf "\e[?1049h\e[3J\e[?1049l"

That command switches to the alternate buffer (private mode 1049), executes the ED escape sequence with parameter 3 (i.e. erase the scrollback buffer), and then switches back to the main buffer.

Expected behavior

The alternate screen buffer doesn't have a scrollback, so the ED3 sequence should erase the main scrollback buffer. Then when you switch back to the main screen, there should be no scrollback any more.

This is how the command is implemented in XTerm, and most of the other terminal emulators I've tested.

Actual behavior

The main scrollback buffer isn't erased.

This is probably somewhat related to issue #3545. I think the alt screen technically has its own scrollback (even though it's zero length), and it's that empty buffer that is being "erased" when ED3 is executed. It might be better if the alt screen shared the same buffer as the main screen.

@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 Nov 24, 2019
@egmontkob
Copy link

It might be better if the alt screen shared the same buffer as the main screen.

I disagree with this bit, in fact, it should be the exact opposite: operations performed on the alternate screen shouldn't be able to place anything in the (normal screen's) scrollback. See #3492.

That being said, I think ED3 could easily wipe out the normal screen's scrollback even when the alternate screen is active, as done in at least xterm and vte, to address this issue.

@j4james
Copy link
Collaborator Author

j4james commented Nov 24, 2019

operations performed on the alternate screen shouldn't be able to place anything in the (normal screen's) scrollback.

I realise that. But I think that could be achieved fairly easily by treating the alternate screen as if the viewport were fully enclosed in scrolling margins, so the content is scrolled out of existence instead of panning the viewport down. By doing things this way, the user can then still manually pan the viewport up to see the scrollback from the main screen, which is what XTerm lets you do.

I don't think the VTE way of doing things is necessarily bad - it certainly seems less buggy than XTerm - but XTerm is the de facto standard for this stuff, and I thought that was what we were trying to emulate.

@egmontkob
Copy link

which is what XTerm lets you do

XTerm is weird here: if you're on the alternate screen, you can scroll back by dragging the scrollbar, but not using the keyboard. If you don't have the scrollbar enabled, you're out of luck.

(On the other hand, it's a great thing that Shift+PageUp and similar keypresses can be sent to the application running on the alternate screen.)

But I think that could be achieved fairly easily by treating the alternate screen as if the viewport were fully enclosed in scrolling margins, so the content is scrolled out of existence instead of panning the viewport down.

This is indeed one possible way of thinking about it, although IMO a problematic one. There are more scenarios to keep in mind. E.g. when enlarging the window, contents shouldn't be brought back from the scrollback to the alternate screen. When rewrapping (which XTerm doesn't do so can't be taken as reference), the scrollback should be rewrapped as a contiguous region with the normal screen (#3491 + #3492), i.e. even at their boundary reflow or not depending on whether there's an explicit newline there. When selecting a word by double-clicking at the upper-left corner, or autodetecting URLs etc., on the normal screen it should include the scrolled-out bits if there was an implicit overflow, on the alternate screen it shouldn't. Etc.

I believe all this logic is much more cumbersome (although certainly possible) to get right if you think of the scrollback as a shared resource, and becomes much easier if you think of the scrollback as belonging to the normal screen only.

That being said, when the alternate screen is active, you could still show the normal screen's scrollback lines if the user scrolls back. It's not done in VTE, but could easily be done. Or it could even show the entire normal screen above the alternate one, followed by its scrollback if you scroll back even further, which would arguably make more sense. (Really, what's the point in seeing all the previous normal screen activity, except for the last ~24 lines, when you scroll back from the alternate screen?) With this approach it's merely a presentation thing to display the normal screen's scrollback, if you wish to.

@j4james
Copy link
Collaborator Author

j4james commented Nov 25, 2019

I believe all this logic is much more cumbersome (although certainly possible) to get right if you think of the scrollback as a shared resource, and becomes much easier if you think of the scrollback as belonging to the normal screen only.

You may well be right. But with what I know of the code base, I'm not certain I could make it work with separate buffers, but I do think I'd have a chance with a single buffer. But really it's up to the person that is assigned the issue. If it's not me, I don't really care.

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase labels Nov 25, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 25, 2019
@zadjii-msft zadjii-msft added this to the 21H1 milestone Nov 25, 2019
@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Nov 25, 2019
@zadjii-msft
Copy link
Member

Seems reasonable to me. Since the alt buffer and main buffer are both just separate SCREEN_INFORMATION objects, it shouldn't be too hard to detect when we're in the alt buffer and perform that erase operation on the main buffer instead.

@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 Nov 25, 2019
@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H1 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
@zadjii-msft
Copy link
Member

printf "\x1b[?1049hThis is the alt buffer" ; sleep 1 ; printf "\x1b[3J" ; sleep 3 ; printf "\x1b[?1049l"
  • xterm shows the length of the scrollbar, then after the 3J you can see it cleared
  • VTE has the scrollback cleared when you get back to the main buffer

just getting a useful repro set up.

@zadjii-msft zadjii-msft self-assigned this Feb 24, 2022
@zadjii-msft zadjii-msft modified the milestones: Terminal v1.14, Backlog Apr 7, 2022
@zadjii-msft zadjii-msft removed their assignment Apr 7, 2022
DHowett pushed a commit that referenced this issue Apr 12, 2022
This PR allows the Terminal to actually use the alt buffer
appropriately. Currently, we just render the alt buffer state into the
main buffer and that is wild. It means things like `vim` will let the
user scroll up to see the previous history (which it shouldn't).

Very first thing this PR does: updates the
`{Trigger|Invalidate}Circling` methods to instead be
`{Trigger|Invalidate}Flush(bool circling)`. We need this so that when an
app requests the alt buffer in conpty, we can immediately flush the
frame before asking the Terminal side to switch to the other buffer. The
`Circling` methods was a great place to do this, but we don't actually
want to set the circled flag in VtRenderer when that happens just for a
flush. 

The Terminal's implementation is a little different than conhost's.
Conhost's implementation grew organically, so I had it straight up
create an entire new screen buffer for the alt buffer. The Terminal
doesn't need all that! All we need to do is have a separate `TextBuffer`
for the alt buffer contents. This makes other parts easier as well - we
don't really need to do anything with the `_mutableViewport` in the alt
buffer, because it's always in the same place. So, we can just leave it
alone and when we come back to the main buffer, there it is. Helper
methods have been updated to account for this. 

* [x] Closes #381
* [x] Closes #3492
* #3686, #3082, #3321, #3493 are all good follow-ups here.
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 Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

No branches or pull requests

4 participants