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

Alternate screen shouldn't write to scrollback or normal screen #3492

Closed
egmontkob opened this issue Nov 8, 2019 · 2 comments · Fixed by #12561
Closed

Alternate screen shouldn't write to scrollback or normal screen #3492

egmontkob opened this issue Nov 8, 2019 · 2 comments · Fixed by #12561
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, 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 Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@egmontkob
Copy link

Environment

Windows build number: 10.0.18362.0
Windows Terminal version (if applicable): 0.6.2951.0

Epic

Scrollback handling should be just like in VTE because that's the best (ahem ahem) 🤣

Steps to reproduce 1

  • ssh to a Linux box, and execute these:

    seq -f 'normal %.f' 100
    echo -ne '\e[?1049h' # switch to alt screen
    seq -f 'alt %.f' 100
    echo -ne '\e[?1049l' # switch back to normal screen
    
  • scroll back and examine the contents

Expected behavior 1

Outputs of these two seqs shouldn't interleave

Actual behavior 1

Output consists of normal 1 to normal 74 (exact number depends on the window height), followed by alt 1 to alt 74, and finally normal 75 to normal 100. (alt 75 onwards are gone.)

Steps to reproduce 2

  • ssh to a Linux box.
  • produce a few pages of output
  • start a fullscreen app such as mc
  • resize the window in various ways (maybe even back and forth to smaller and then bigger)
  • quit mc
  • examine the output, including the scrollback

Expected behavior 2

This should look okay (see below).

Actual behavior 2

The contents seem quite garbled. Most of the time you'll see remains of mc's user interface in the scrollback buffer; sometimes even on the normal screen (this latter is harder to reproduce, and perhaps only happens with solid background colors).


VTE rocks

The way this works amazingly in gnome-terminal is:

The alternate screen is pretty much only used by fullscreen apps that control the entire UI and repaint on window resize. Therefore it's unrelated to the normal screen and the scrollback. Data never travels between the (normal screen + scrollback) combo and the alternate screen. The scrollback buffer belongs to the normal screen only. Everything scrolled out from the alternate screen is lost for good.

On resize, the (normal screen + scrollback) is resized as a single unit, for the resizing operation it doesn't matter where the boundary is.

The (normal screen + scrollback) combo is resized the same way, regardless of whether it's the active one or the alternate screen is shown. Stress-test this: produce output on the normal screen, switch to the alternate one (e.g. mc), resize, quit mc. The normal screen's contents will be updated to the new width just as if you haven't switched to the alternate screen.

There's plenty of further tiny details to this story, e.g. how to vertically position the contents, some of which are documented in VTE's doc/rewrap.txt.

Of course there's no specification anywhere that all this has to behave like this. It's just what I believe makes the most sense. Obviously my opinion is quite biased since this was my first big contribution to VTE. :) Luckily I only had to do the rewrapping; the separation of the (normal screen + scrollback) vs. alternate screen was already as desired.

@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 8, 2019
@DHowett-MSFT
Copy link
Contributor

This is actually one of the unfortunate circumstances of the pseudoconsole API.

conhost implements the alt buffer reasonably well (we think): it turns off scrollback, reflow, and so on.
When it remotes the contents of the screen as VT over pseudoconsole we're not also sending the alt buffer state down the wire (#381).

@zadjii-msft zadjii-msft added Product-Conpty For console issues specifically related to conpty 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. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 11, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 11, 2019
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Nov 11, 2019
@zadjii-msft zadjii-msft added the Priority-1 A description (P1) label Jan 9, 2020
@zadjii-msft zadjii-msft mentioned this issue Nov 29, 2020
@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H1 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
@ghost ghost added the In-PR This issue has a related PR label Feb 23, 2022
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Apr 12, 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.
@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 12, 2022
@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #12561, which has now been successfully released as Windows Terminal Preview v1.14.143.:tada:

Handy links:

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.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, 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 Product-Terminal The new Windows Terminal. 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.

5 participants