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

ConPTY should less aggressively reprint the screen #919

Closed
Tyriar opened this issue May 21, 2019 · 7 comments
Closed

ConPTY should less aggressively reprint the screen #919

Tyriar opened this issue May 21, 2019 · 7 comments
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conpty For console issues specifically related to conpty

Comments

@Tyriar
Copy link
Member

Tyriar commented May 21, 2019

Tasks in VS Code listens to terminal output to detect diagnostics (errors/warnings) and when watch tasks start and end. It also writes to the terminal itself some status messages such as:

> Executing task: echo %FOO% <

Because of the way ConPTY reprints the screen frequently, this causes problems with this though as it will replace the terminal-side messages like the "Executing task" message and will also report diagnostics multiple times (which I think we de-dupe but could potentially cause issues).

It would be better if ConPTY kept track of when only new output was printed at the cursor and only reprinted when absolutely necessary like on a resize. This would bring it much closer in line to how winpty and nix ptys work.

VS Code issue: microsoft/vscode#74063

@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 May 21, 2019
@Tyriar
Copy link
Member Author

Tyriar commented May 21, 2019

This should also improve performance as it would avoid clearing out all the rows which happens a lot right now.

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues labels May 21, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 21, 2019
@zadjii-msft
Copy link
Member

Do we have a particular repro case where the screen is repainted when we don't expect it to be?

For the record, conpty already only repaints the regions it believes have changed in the course of a frame. Most of the code is in this directory - with invalidate.cpp and paint.cpp probably being the most interesting. The Renderer in the sibling directory base acts as the controller for painting only the regions the VtEngine (which powers conpty) thinks are invalid.

There was some bug in either RS5 or in-between RS5 and 19H1 where we'd incorrectly repaint the screen whenever the active attributes were changed (which is pretty constantly). I believe that's fixed now, so this shouldn't be as bad as before, but it could always be better. Concrete scenarios would certainly make this easier to drill into.

In the linked scenario, it almost seems like the task is spawning a new conpty instance to run the task, and then writing that output to the parent, is that correct? If that's the case then maybe the child task needs to inherit the cursor position of the parent terminal (using dwFlags= PSEUDOCONSOLE_INHERIT_CURSOR

@Tyriar
Copy link
Member Author

Tyriar commented May 21, 2019

@zadjii-msft the flow in microsoft/vscode#74063 is roughly:

  1. Instantiate xterm.js
  2. Write "> Executing task: echo %FOO% <" to xterm.js
  3. Launch the process via conpty and hook it up to xterm.js
  4. Conpty reprints the screen right after process launch

@alexr00 what's your Windows 10 build number?

@zadjii-msft
Copy link
Member

Yea that sounds exactly like the kind of situation where INHERIT_CURSOR is needed.

Since conpty has to maintain the state of a console buffer, by default it assumes that the buffer is empty. However, with INHERIT_CURSOR, conpty asks the terminal on startup where the cursor currently is. When the terminal replies, conpty uses that as the initial cursor position, and won't render any text above that point. It'll use that y-coordinate as a "virtual top" which is inaccessible. As the text scrolls, we'll move that virtualtop up, until the entire buffer is addressable again.

I believe WSL and ssh.exe both use this flag, to make console sessions more continuous. Otherwise, as you've noted, we'll start by clearing the screen, which is only really desirable if you know you're starting from a fresh terminal window.

@Tyriar
Copy link
Member Author

Tyriar commented May 21, 2019

What's the problem you're trying to fix by not assuming a cursor position of 1,1 when conpty starts?

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 21, 2019

We are assuming a cursor position of 1,1 when ConPTY starts. PSEUDOCONSOLE_INHERIT_CURSOR overrides that behaviour.

The reason we by default assume a cursor position of 1,1 is that Win32 console applications are more likely than Unix VT applications to try to read back the screen buffer contents. They can figure out where the cursor is without asking for a cursor report, and they can request that regions of console screen buffer memory be copied back into their processes. They can also write from their own buffers directly into the console buffer at an absolute (not viewport-relative like VT!) location.

To support these APIs from a pseudoconsole, we had to do two things.

First and foremost, we can't lie to a Win32 console application about where the cursor is, or where any output has been drawn. That would force us to pile up hacks to adjust the location of every buffer- or cursor position-related call. So, we assume the cursor is at 1,1 unless we have explicit knowledge from the hosting terminal as to where the cursor should be.

Because there is no "API" for requesting the cursor position apart from requesting DSR and waiting for a response, INHERIT_CURSOR requires that the connecting terminal provide DSR before it'll continue. Again, this is required for API consistency.

The second thing we do is to ensure that the win32 console and its VT representation are in sync (that is: a console application reading a buffer out of the console won't see blank space where the terminal says there's text, and vice-versa): we clear the screen. This only applies when we home the cursor to 1,1 on startup.

TL;DR: We home the cursor to 1,1 and clear the screen for the widest compatibility with Win32 console applications, and both of those behaviors are overridable because using INHERIT_CURSOR disables the screen clear and the cursor homing.

Double TL;DR: The premise for your question didn't make sense, so I decided it would be best to just explain everything.

@Tyriar
Copy link
Member Author

Tyriar commented May 21, 2019

Thanks it sounds like we should just default to PSEUDOCONSOLE_INHERIT_CURSOR and the repainting issue sounds like a bug that's already fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

No branches or pull requests

3 participants