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

Implement Hard Reset for Terminal #4909

Merged
4 commits merged into from
Mar 16, 2020
Merged

Implement Hard Reset for Terminal #4909

4 commits merged into from
Mar 16, 2020

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

This actually implements \033c
(RIS) 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

PR Checklist

Validation Steps Performed

Actually tested printf \033c in the Terminal this time

## 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 added Area-VT Virtual Terminal sequence support Product-Terminal The new Windows Terminal. labels Mar 13, 2020
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.

Will this impact other non-RIS escape sequences?

Comment on lines +417 to +424
// if (success)
// {
// success = SetOriginMode(false); // Absolute cursor addressing.
// }
// if (success)
// {
// success = SetAutoWrapMode(true); // Wrap at end of line.
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, i accept the reason for these comments

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Do we want to test SoftReset too? Or is it good enough that a HardReset includes a SoftReset?

src/cascadia/TerminalCore/TerminalDispatch.cpp Outdated Show resolved Hide resolved
Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 16, 2020
@ghost
Copy link

ghost commented Mar 16, 2020

Hello @zadjii-msft!

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 3dc0672 into master Mar 16, 2020
@ghost ghost deleted the dev/migrie/b/2715-part-2-RIS branch March 16, 2020 15:32
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RIS doesn't propagate through ConPTY to clear connected terminal
3 participants