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

Make sure conpty is flushed before clearing scrollback #13324

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jun 18, 2022

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

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.

@ghost ghost added Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty labels Jun 18, 2022
@j4james j4james marked this pull request as ready for review June 18, 2022 23:16
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some top tier debugging, thanks!

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH DANG! Excellent find, thank you!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 20, 2022
@DHowett DHowett merged commit 08c2f35 into microsoft:main Jun 20, 2022
@ghost
Copy link

ghost commented Jun 20, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett pushed a commit that referenced this pull request Jun 30, 2022
## 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
@ghost
Copy link

ghost commented Jul 6, 2022

🎉Windows Terminal v1.14.186 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 6, 2022

🎉Windows Terminal Preview v1.15.186 has been released which incorporates this pull request.:tada:

Handy links:

@jrd-lewis
Copy link

jrd-lewis commented Jul 6, 2022

🎉Windows Terminal v1.14.186 has been released which incorporates this pull request.🎉

Handy links:

I just installed the latest stable Windows Terminal Version: 1.14.1862.0 directly from the releases page; and it looks that despite being emailed this morning that this PR that was implemented in today's releases addressed my issue from 2020, it did not. I'm still experiencing the same thing I did back then: #4461

@j4james
Copy link
Collaborator Author

j4james commented Jul 7, 2022

@jrd-lewis I think closing #4461 as a duplicate of #3126 was probably a mistake. Your problem was regarding the command history not being cleared, while #3126 (and this PR) has to do with clearing the scroll-back buffer.

That said, the PowerShell history isn't actually something that is controlled by the terminal, so there's nothing we could do about that anyway. And I suspect it may be working as intended, since you're clearing the PSConsoleReadLine history which I believe is separate from the actual PowerShell history.

But I think the best place to get further info would be in the PowerShell/PSReadLine repository. Just doing a quick scan, I found PowerShell/PSReadLine/issues/1247, which may be relevant. I'm sorry you weren't directed there earlier.

@DHowett
Copy link
Member

DHowett commented Jul 8, 2022

Thanks for this, @j4james. You're right -- I shouldn't have closed that as a duplicate. I completely overlooked the fact that @jrd-lewis was talking about command history. Even though he had said, "press the up arrow." 🤦🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Projects
None yet
5 participants