Implement a pair of shims for `cls`, `Clear-Host` in conpty mode #5627

zadjii-msft · 2020-04-28T20:02:33Z

Summary of the Pull Request

This PR implements a pair of shims for cmd and powershell, so that their cls and Clear-Host functions will clear the entire terminal buffer (like they do in conhost), instead of just the viewport. With the conpty viewport and buffer being the same size, there's effectively no way to know if an application is calling these API's in this way with the intention of clearing the buffer or the viewport. We absolutely have to guess.

Each of these shims checks to see if the way that the API is being called exactly matches the way cmd or powershell would call these APIs. If it does, we manually write a ^[[3J to the connected terminal, to get he Terminal to clear it's own scrollback.

~~⚠️ If another application were trying to clear the viewport with an exactly similar API call, this would also cause the terminal scrollback to get cleared ⚠️~~

Should these shims be restricted to when the process that's calling them is actually cmd.exe or powershell.exe? Can I even do this? I think we've done such a good job of isolating the client process information from the rest of the host code that I can't figure out how to do this.
- YES, this can be done, and I did it.
TODO: While I'm here, should I have DoSrvPrivateEraseAll (the implementation for ^[[2J, in getset.cpp) also manually trigger a EraseAll in the terminal in conpty mode?

PR Checklist

Closes Clear-Host does not clear the previous content (Powershell) #3126
Actually closes cls in PowerShell and cmd.exe doesn't fully clear the Terminal scroll-back history #1305 too, which is really the same thing, but probably deserves a callout
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Validation Steps Performed

ran tests
checked cls in the Terminal
checked Clear-Host in the Terminal
Checked running powershell clear-host from cmd.exe

…e to actually play with it

zadjii-msft · 2020-04-28T20:11:41Z

@msftbot make sure @miniksa signs off on this

ghost · 2020-04-28T20:11:46Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett-MSFT · 2020-04-28T20:12:26Z

wowww

pingzing · 2020-04-29T21:35:58Z

This should probably check for pwsh.exe in addition to powershell.exe, yeah?

zadjii-msft · 2020-04-29T21:39:35Z

This should probably check for pwsh.exe in addition to powershell.exe, yeah?

I was under the impression that the updated pwsh.exe would be using ^[[3J, but I just tried it and it doesn't look like that's the case... Hmm. I'm thinking the forward-thinking call would be to just get pwsh updated (since that's actually receiving updates, compared to powershell.exe), but adding pwsh.exe to this list would be trivial.

@ team thoughts?

DHowett-MSFT · 2020-04-29T22:13:15Z

If pwsh begins using 3J it’ll just skip the shim, which I’m fine with. We should keep the shim because of the long tail of folks on 7.0

DHowett-MSFT

Looks great to me; gotta rvw the tests

src/server/ConsoleShimPolicy.cpp

src/inc/til/size.h

miniksa · 2020-04-29T22:46:15Z

This should probably check for pwsh.exe in addition to powershell.exe, yeah?

I was under the impression that the updated pwsh.exe would be using ^[[3J, but I just tried it and it doesn't look like that's the case... Hmm. I'm thinking the forward-thinking call would be to just get pwsh updated (since that's actually receiving updates, compared to powershell.exe), but adding pwsh.exe to this list would be trivial.

@ team thoughts?

Do it for pwsh.exe too, is my thought.

DHowett-MSFT · 2020-04-30T02:16:49Z

This feels right. Selfhost 0.11.1204.

src/host/_output.cpp

…ls-shim # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

…soft/terminal into dev/migrie/b/3126-cls-shim

…ls-shim

… the same time

ghost · 2020-05-05T22:27:01Z

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

## Summary of the Pull Request When you execute a `cls` in the cmd shell, or `Clear-Host` in PowerShell, we have a pair of shims that attempt to detect those operations and forward an `ED3` sequence to conpty to clear the scrollback. If there was a linefeed at the bottom of the viewport immediately prior to those functions being called, that event might still be pending, and only forwarded to conpty after the `ED3`. The result then is a line pushed into the scrollback that shouldn't be there. This PR tries to avoid that situation by forcing the renderer to flush before the `ED3` sequence is sent. ## References The `cls` and `Clear-Host` shims were originally added in PR #5627. ## PR Checklist * [x] Closes #5770 * [x] Closes #13320 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Validation Steps Performed I've manually tested in PowerShell with `echo Hello; Clear-Host` (this is the only way I could reliably reproduce the original problem), and in the cmd shell with `cls`. Both cases are now working as expected.

## Summary of the Pull Request When you execute a `cls` in the cmd shell, or `Clear-Host` in PowerShell, we have a pair of shims that attempt to detect those operations and forward an `ED3` sequence to conpty to clear the scrollback. If there was a linefeed at the bottom of the viewport immediately prior to those functions being called, that event might still be pending, and only forwarded to conpty after the `ED3`. The result then is a line pushed into the scrollback that shouldn't be there. This PR tries to avoid that situation by forcing the renderer to flush before the `ED3` sequence is sent. ## References The `cls` and `Clear-Host` shims were originally added in PR #5627. ## PR Checklist * [x] Closes #5770 * [x] Closes #13320 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Validation Steps Performed I've manually tested in PowerShell with `echo Hello; Clear-Host` (this is the only way I could reliably reproduce the original problem), and in the cmd shell with `cls`. Both cases are now working as expected. (cherry picked from commit 08c2f35) Service-Card-Id: 83388911 Service-Version: 1.14

zadjii-msft added 4 commits April 28, 2020 11:05

This seems like it works as a test

ff14bad

Just from the test passing, I believe this will ship cmd.exe. Now tim…

54a5bd2

…e to actually play with it

Add the powershell case too

9583e04

create the shim for powershell and Clear-Host as well

841eff6

zadjii-msft added Product-Conpty For console issues specifically related to conpty Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. labels Apr 28, 2020

zadjii-msft requested review from DHowett-MSFT and miniksa April 28, 2020 20:02

some doc comments I missed

a4151c3

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 28, 2020

metathinker mentioned this pull request Apr 29, 2020

cls in PowerShell and cmd.exe doesn't fully clear the Terminal scroll-back history #1305

Closed

Make this only work for cmd.exe and powershell.exe

f74d272

zadjii-msft marked this pull request as ready for review April 29, 2020 21:13

DHowett-MSFT reviewed Apr 29, 2020

View reviewed changes

src/server/ConsoleShimPolicy.cpp Outdated Show resolved Hide resolved

miniksa reviewed Apr 29, 2020

View reviewed changes

src/server/ConsoleShimPolicy.cpp Outdated Show resolved Hide resolved

src/inc/til/size.h Show resolved Hide resolved

DHowett-MSFT approved these changes Apr 30, 2020

View reviewed changes

src/host/_output.cpp Outdated Show resolved Hide resolved

zadjii-msft added 8 commits April 30, 2020 08:29

pr comments

ed062ff

Merge remote-tracking branch 'origin/master' into dev/migrie/b/3126-c…

2f61d55

…ls-shim # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

whoops

2b953b4

yikes this is some real ded code

8707c18

Merge branch 'dev/migrie/b/3126-cls-shim' of https://github.com/micro…

5e83280

…soft/terminal into dev/migrie/b/3126-cls-shim

This is more dead code

1c65edb

Merge remote-tracking branch 'origin/master' into dev/migrie/b/3126-c…

e1355fc

…ls-shim

This is my own fault for adding so many tests in the same file all at…

65d754b

… the same time

DHowett-MSFT approved these changes Apr 30, 2020

View reviewed changes

miniksa approved these changes Apr 30, 2020

View reviewed changes

ghost merged commit 7612044 into master Apr 30, 2020

ghost deleted the dev/migrie/b/3126-cls-shim branch April 30, 2020 21:53

ghost mentioned this pull request May 5, 2020

Clear-Host does not clear the previous content (Powershell) #3126

Closed

zadjii-msft mentioned this pull request May 6, 2020

up-arrow glitch on Win Powershell 5.1 #5753

Closed

PrzemyslawKlys mentioned this pull request May 6, 2020

Clear-Host sometimes leaves a line at the top of the screen #5770

Closed

DHowett mentioned this pull request Jan 11, 2021

Consider allowing users to disable E3 scrollback clear behavior of CSI J #8736

Closed

j4james mentioned this pull request Jun 18, 2022

Make sure conpty is flushed before clearing scrollback #13324

Merged

7 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a pair of shims for `cls`, `Clear-Host` in conpty mode #5627

Implement a pair of shims for `cls`, `Clear-Host` in conpty mode #5627

zadjii-msft commented Apr 28, 2020 •

edited

zadjii-msft commented Apr 28, 2020

ghost commented Apr 28, 2020

DHowett-MSFT commented Apr 28, 2020

pingzing commented Apr 29, 2020

zadjii-msft commented Apr 29, 2020

DHowett-MSFT commented Apr 29, 2020

DHowett-MSFT left a comment

miniksa commented Apr 29, 2020

DHowett-MSFT commented Apr 30, 2020

ghost commented May 5, 2020

Implement a pair of shims for cls, Clear-Host in conpty mode #5627

Implement a pair of shims for cls, Clear-Host in conpty mode #5627

Conversation

zadjii-msft commented Apr 28, 2020 • edited

Summary of the Pull Request

PR Checklist

Validation Steps Performed

zadjii-msft commented Apr 28, 2020

ghost commented Apr 28, 2020

DHowett-MSFT commented Apr 28, 2020

pingzing commented Apr 29, 2020

zadjii-msft commented Apr 29, 2020

DHowett-MSFT commented Apr 29, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

miniksa commented Apr 29, 2020

DHowett-MSFT commented Apr 30, 2020

ghost commented May 5, 2020

Implement a pair of shims for `cls`, `Clear-Host` in conpty mode #5627

Implement a pair of shims for `cls`, `Clear-Host` in conpty mode #5627

zadjii-msft commented Apr 28, 2020 •

edited