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

Properly support 2J sequence for clearing the screen #4252

Closed
zadjii-msft opened this issue Jan 15, 2020 · 2 comments
Closed

Properly support 2J sequence for clearing the screen #4252

zadjii-msft opened this issue Jan 15, 2020 · 2 comments
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty

Comments

@zadjii-msft
Copy link
Member

This was taken from MSFT:21096414, which was found here in the code:

if (!_resized && dirtyView == _lastViewport)
{
// TODO: MSFT:21096414 - This is never actually hit. We set
// _resized=true on every frame (see VtEngine::UpdateViewport).
// Unfortunately, not always setting _resized is not a good enough
// solution, see that work item for a description why.
RETURN_IF_FAILED(_ClearScreen());
_clearedAllThisFrame = true;
}
}

Apparently this bug never made the hop from ADO to Github. Full text below:

Currently, we only ever send one CSI 2 J from conpty - when we first start up.
Otherwise, the other call to ClearScreen is behind a conditional with !resized - see XtermEngine::StartPaint:

auto dirtyRect = GetDirtyRectInChars();
const auto dirtyView = Viewport::FromInclusive(dirtyRect);
if (!_resized && dirtyView == _lastViewport)
{
    RETURN_IF_FAILED(_ClearScreen());
    _clearedAllThisFrame = true;
}

Unfortunately, !resized will never be true - in state.cpp, in VtEngine::UpdateViewport, we always set _resized to true, and we always call UpdateViewport at the start of as frame.

What that means currently is that the terminal will never get cleared when the conpty is cleared.
Removing the _resized check, or changing the assignment of _resized to only when the dimensions change is NOT correct however.

There's no way to tell the difference between a frame that was all invalid was because the screen was cleared or because it contains all new text.

For example, if you make the simple fix described above, and run help or ping in vtpipeterm, then scroll into history, you might see the output duplicated. This would be because the pty emitted an entire screen of output, then on the next frame, determined everything was invalid, then cleared the terminal screen, then re-emitted the same screen of text.

So we're might be doing something funky in the full screen writing case, where we on the next frame still think everything is invalid. Maybe the scroll delta is causing us to have invalid text up near the top of the pty, causing us to think the screen should be cleared after a full screen of text.

Alternatively, as a half measure, we could have the adapter handle 2J, then in pty mode return false, causing the state machine to write the 2J to the renderer as well. That's probably not the right solution - we might want a specific call for the adapter to tell the renderer that it's requested an EraseAll, so the renderer can update it's state accordingly. Alternatively, the IRenderTarget could have a TriggerClearAll that does specifically that, though that would be relatively unused by the other engines, so I'm not a fan of that.
This half measure isn't great, because it wouldn't work for cmd.exe's cls - it would only work for things emitting a 2J (wsl)

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) labels Jan 15, 2020
@zadjii-msft zadjii-msft added this to the Console Backlog milestone Jan 15, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 15, 2020
@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 Jan 16, 2020
ghost pushed a commit that referenced this issue May 5, 2020
The Erase All VT sequence (`^[[2J`) is supposed to erase the entire
contents of the viewport. The way it usually does this is by shifting
the entirety of the viewport contents into scrollback, and starting the
new viewport below it. 

Currently, conpty doesn't propagate that state change correctly. When
conpty gets a 2J, it simply erases the content of the connected
terminal's viewport, by writing over it with spaces. Conpty didn't
really have a good way of communicating "your viewport should move", it
only knew "the buffer is now full of spaces".

This would lead to bugs like #2832, where pressing <kbd>ctrl+L</kbd> in
`bash` would delete the current contents of the viewport, instead of
moving the viewport down.

This PR makes sure that when conpty sees a 2J, it passes that through
directly to the connected terminal application as well. Fortunately, 2J
was already implemented in the Windows Terminal, so this actually fixes
the behavior of <kbd>ctrl+L</kbd>/`clear` in WSL in the Terminal.

## References

* #4252 - right now this isn't the _most_ optimal scenario, we're
  literally just printing a 2J, then we'll perform "erase line" `height`
  times. The erase line operations are all redundant at this point - the
  entire viewport is blank, but conpty doesn't really know that.
  Fortunately, #4252 was already filed for me to come through and
  optimize this path.

## PR Checklist
* [x] Closes #2832
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed
* ran tests
* compared <kbd>ctrl+L</kbd> with its behavior in conhost
* compared `clear` with its behavior in conhost
DHowett-MSFT pushed a commit that referenced this issue May 5, 2020
The Erase All VT sequence (`^[[2J`) is supposed to erase the entire
contents of the viewport. The way it usually does this is by shifting
the entirety of the viewport contents into scrollback, and starting the
new viewport below it. 

Currently, conpty doesn't propagate that state change correctly. When
conpty gets a 2J, it simply erases the content of the connected
terminal's viewport, by writing over it with spaces. Conpty didn't
really have a good way of communicating "your viewport should move", it
only knew "the buffer is now full of spaces".

This would lead to bugs like #2832, where pressing <kbd>ctrl+L</kbd> in
`bash` would delete the current contents of the viewport, instead of
moving the viewport down.

This PR makes sure that when conpty sees a 2J, it passes that through
directly to the connected terminal application as well. Fortunately, 2J
was already implemented in the Windows Terminal, so this actually fixes
the behavior of <kbd>ctrl+L</kbd>/`clear` in WSL in the Terminal.

## References

* #4252 - right now this isn't the _most_ optimal scenario, we're
  literally just printing a 2J, then we'll perform "erase line" `height`
  times. The erase line operations are all redundant at this point - the
  entire viewport is blank, but conpty doesn't really know that.
  Fortunately, #4252 was already filed for me to come through and
  optimize this path.

## PR Checklist
* [x] Closes #2832
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed
* ran tests
* compared <kbd>ctrl+L</kbd> with its behavior in conhost
* compared `clear` with its behavior in conhost
DHowett-MSFT pushed a commit that referenced this issue May 5, 2020
The Erase All VT sequence (`^[[2J`) is supposed to erase the entire
contents of the viewport. The way it usually does this is by shifting
the entirety of the viewport contents into scrollback, and starting the
new viewport below it.

Currently, conpty doesn't propagate that state change correctly. When
conpty gets a 2J, it simply erases the content of the connected
terminal's viewport, by writing over it with spaces. Conpty didn't
really have a good way of communicating "your viewport should move", it
only knew "the buffer is now full of spaces".

This would lead to bugs like #2832, where pressing <kbd>ctrl+L</kbd> in
`bash` would delete the current contents of the viewport, instead of
moving the viewport down.

This PR makes sure that when conpty sees a 2J, it passes that through
directly to the connected terminal application as well. Fortunately, 2J
was already implemented in the Windows Terminal, so this actually fixes
the behavior of <kbd>ctrl+L</kbd>/`clear` in WSL in the Terminal.

## References

* #4252 - right now this isn't the _most_ optimal scenario, we're
  literally just printing a 2J, then we'll perform "erase line" `height`
  times. The erase line operations are all redundant at this point - the
  entire viewport is blank, but conpty doesn't really know that.
  Fortunately, #4252 was already filed for me to come through and
  optimize this path.

## PR Checklist
* [x] Closes #2832
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed
* ran tests
* compared <kbd>ctrl+L</kbd> with its behavior in conhost
* compared `clear` with its behavior in conhost

(cherry picked from commit 9fe624f)
@DHowett
Copy link
Member

DHowett commented Aug 13, 2020

I believe we did implement the half measure. Mike, can this be closed?

@zadjii-msft
Copy link
Member Author

Oh yea this is done, probably as a part of all the #4200 work.

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Aug 13, 2020
@zadjii-msft zadjii-msft removed their assignment Aug 13, 2020
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-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

No branches or pull requests

3 participants