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.

Sign up for GitHub

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

Jump to bottom

Passthrough CSI 3 J in Conpty #4433

Merged
4 commits merged into from
Feb 10, 2020
Merged

Passthrough CSI 3 J in Conpty #4433

4 commits merged into from
Feb 10, 2020

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

Conpty doesn't need CSI 3 J, it doesn't have a scrollback. The terminal that's connected should use that. This makes conpty pass it through, like other sequences that conpty has no need for.

References

PR Checklist

DHowett-MSFT
DHowett-MSFT approved these changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Love it.

@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Jan 31, 2020
@ghost ghost requested review from miniksa, carlos-zamora and leonMSFT January 31, 2020 23:54
@j4james
Copy link
Collaborator

j4james commented Feb 2, 2020

I don't want sound like I'm objecting to this PR, because it does at least improve the situation, but "passthrough" as a solution is almost never going to solve the problem completely. In this particular case you're fixing the CSI 3 J escape sequence, but any app that clears the scrollback through the console APIs is still not going to work.

In particular, issue #3126 (which was a closed as a dup of #2715) is not fixed by this PR. So if you close #2715 then you'll need to reopen #3126.

@miniksa
Copy link
Member

miniksa commented Feb 3, 2020

I don't want sound like I'm objecting to this PR, because it does at least improve the situation, but "passthrough" as a solution is almost never going to solve the problem completely. In this particular case you're fixing the CSI 3 J escape sequence, but any app that clears the scrollback through the console APIs is still not going to work.

In particular, issue #3126 (which was a closed as a dup of #2715) is not fixed by this PR. So if you close #2715 then you'll need to reopen #3126.

I reactivated 3126, @j4james. Thanks for the consideration.

miniksa
miniksa reviewed Feb 3, 2020
View reviewed changes
src/terminal/adapter/adaptDispatch.cpp
// terminal application. We've reset our state, but the connected terminal
// might need to do more.
bool isPty = false;
_pConApi->IsConsolePty(isPty);
Copy link
Member

Choose a reason for hiding this comment

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

This really feels like a function that should have been returning the bool...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Big +1. This is one of things I was hoping to clean up when moving to exceptions in place of return values for the error handling (which is still on my long term todo list). But some of these getters aren't even using the return value, so they don't actually need to have the exception handling in place - it should be an easy update.

miniksa
miniksa approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Fine with me. I reactivated the other one. Just one comment that the pattern for looking up if it's a PTY is weird.

@miniksa miniksa added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Second It's a PR that needs another sign-off labels Feb 10, 2020
@ghost
Copy link

ghost commented Feb 10, 2020

Hello @miniksa!

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.

@ghost ghost merged commit 2d6b8bc into master Feb 10, 2020
@ghost ghost deleted the dev/migrie/b/2715 branch February 10, 2020 20:30
@ghost
Copy link

ghost commented Feb 13, 2020

🎉Windows Terminal Preview v0.9.433.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Feb 13, 2020
Closed
zadjii-msft added a commit that referenced this pull request Mar 13, 2020
@zadjii-msft
Implement Hard Reset for Terminal
52124c7 
## Summary of the Pull Request

This _actually_ implements `\033c`
([RIS](https://vt100.net/docs/vt220-rm/chapter4.html)) for the Windows Terminal.
I thought I had done this in #4433, but that PR actually only passthrough'd
`\x1b[3J`. I didn't realize at the time that #2715 was mostly about hard reset,
not erase scrollback.

Not only should conpty pass through RIS, but the Terminal should also be
prepared to actually handle that sequence. So this PR adds that support as well.

## References

* #4433: original PR I thought fixed this.

## PR Checklist
* [x] Closes #2715 for real this time
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

Actually tested `printf \033c` in the Terminal this time
@zadjii-msft zadjii-msft mentioned this pull request Mar 13, 2020
Merged
3 tasks
ghost pushed a commit that referenced this pull request Mar 16, 2020
@zadjii-msft
Implement Hard Reset for Terminal (#4909)
3dc0672 
## Summary of the Pull Request

This _actually_ implements `\033c`
([RIS](https://vt100.net/docs/vt220-rm/chapter4.html)) for the Windows Terminal.
I thought I had done this in #4433, but that PR actually only passthrough'd
`\x1b[3J`. I didn't realize at the time that #2715 was mostly about hard reset,
not erase scrollback.

Not only should conpty pass through RIS, but the Terminal should also be
prepared to actually handle that sequence. So this PR adds that support as well.

## References

* #4433: original PR I thought fixed this.

## PR Checklist
* [x] Closes #2715 for real this time
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

Actually tested `printf \033c` in the Terminal this time
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j4james j4james j4james left review comments

@miniksa miniksa miniksa approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

Assignees
No one assigned
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RIS doesn't propagate through ConPTY to clear connected terminal
4 participants
@zadjii-msft @j4james @miniksa @DHowett-MSFT