-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Implement a pair of shims for cls
, Clear-Host
in conpty mode
#5627
Conversation
@msftbot make sure @miniksa signs off on this |
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:
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". |
wowww |
This should probably check for |
I was under the impression that the updated @ team thoughts? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me; gotta rvw the tests
Do it for |
This feels right. Selfhost 0.11.1204. |
…ls-shim # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
…soft/terminal into dev/migrie/b/3126-cls-shim
🎉 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
Summary of the Pull Request
This PR implements a pair of shims for
cmd
andpowershell
, so that theircls
andClear-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
orpowershell
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.cmd.exe
orpowershell.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.DoSrvPrivateEraseAll
(the implementation for^[[2J
, ingetset.cpp
) also manually trigger a EraseAll in the terminal in conpty mode?PR Checklist
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 calloutValidation Steps Performed
cls
in the TerminalClear-Host
in the Terminalpowershell clear-host
fromcmd.exe