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

VT SOS, APC & PM should be ignored until supported #7032

Closed
skyline75489 opened this issue Jul 23, 2020 · 2 comments · Fixed by #7340
Closed

VT SOS, APC & PM should be ignored until supported #7032

skyline75489 opened this issue Jul 23, 2020 · 2 comments · Fixed by #7340
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Console Backlog

Comments

@skyline75489
Copy link
Collaborator

skyline75489 commented Jul 23, 2020 •
edited
Loading

Environment

Windows build number: 10.0.19041.388
Windows Terminal version (if applicable): 1.1.2021

Any other software? WSL

Steps to reproduce

Run the following bash command:

printf '\e_123123\e\\'

printf '\e^123123\e\\'

Expected behavior

Nothing is shown.

Actual behavior

The data string is printed:

>printf '\e_123123\e\\'
123123%

Detail

Coming from #120 and excellent comments of @j4james on #6328 , it occurs to me that we should ignore APC and PM the same way as DCS.

Feel free to assign this issue to me as it's very likely to be the next thing in my plan.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 23, 2020
@skyline75489 skyline75489 added the Area-VT Virtual Terminal sequence support label Jul 23, 2020
@skyline75489 skyline75489 changed the title VT APC & PM should be ignored unless supported VT APC & PM should be ignored until supported Jul 23, 2020
@skyline75489
Copy link
Collaborator Author

skyline75489 commented Jul 23, 2020 •
edited
Loading

From the official doc of xterm here, SOS, APC & PM are recognized but no actual functions are implemented. So I guess we could do the same by simply recognizing the sequence and call it a day. Doing this will align the behavior of WT with other modern terminals.

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase labels Jul 23, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 23, 2020
@zadjii-msft zadjii-msft added this to the Console Backlog milestone Jul 23, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 24, 2020
@skyline75489 skyline75489 changed the title VT APC & PM should be ignored until supported VT SOS, APC & PM should be ignored until supported Jul 24, 2020
This was referenced Aug 17, 2020
Merged
Merged
@ghost ghost added the In-PR This issue has a related PR label Aug 19, 2020
@ghost ghost closed this as completed in #7340 Sep 16, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 16, 2020
ghost pushed a commit that referenced this issue Sep 16, 2020
@skyline75489
Preprocess and convert C1 controls to their 7 bit equivalent (#7340)
f91b53d 
C1 control characters are now first converted to their 7 bit equivalent.
This allows us to unify the logic of C1 and C0 escape handling. This
also adds support for SOS/PM/APC string.

* Unify the logic for C1 and C0 escape handling by converting C1 to C0
  beforehand. This adds support for various C1 characters, including
  IND(8/4), NEL(8/5), HTS(8/8), RI(8/13), SS2(8/14), SS3(8/15),
  OSC(9/13), etc. 
* Add support for SOS/PM/APC escape sequences. Fixes #7032
* Use "Variable Length String" logic to unify the string termination
  handling of OSC, DCS and SOS/PM/APC. This fixes an issue where OSC
  action is successfully dispatched even when terminated with non-ST
  character. Introduced by #6328, the DCS PassThrough is spared from
  this issue. This PR puts them together and add test cases for them.

References:
https://vt100.net/docs/vt510-rm/chapter4.html
https://vt100.net/emu/dec_ansi_parser

Closes #7032
Closes #7317
@ghost
Copy link

ghost commented Sep 22, 2020

🎉This issue was addressed in #7340, which has now been successfully released as Windows Terminal Preview v1.4.2652.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Console Backlog
Development

Successfully merging a pull request may close this issue.

Preprocess and convert C1 controls to their 7 bit equivalent skyline75489/terminal
3 participants
@DHowett @skyline75489 @zadjii-msft