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

Preprocess and convert C1 controls to their 7 bit equivalent #7317

Closed
skyline75489 opened this issue Aug 17, 2020 · 6 comments · Fixed by #7340
Closed

Preprocess and convert C1 controls to their 7 bit equivalent #7317

skyline75489 opened this issue Aug 17, 2020 · 6 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. 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

Description of the new feature/enhancement

This is from a discussion in #6328:

@j4james: I was actually wondering whether this was something we could handle as a sort of preprocessing step that converts all of the C1 controls into their two character 7-bit equivalent. That way we can drop all the extra code that's checking for 90 and 9B introducers, and 9C terminators, because those conditions would now be handled as regular 7-bit escapes.
@skyline75489 skyline75489 added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Aug 17, 2020
@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 Aug 17, 2020
@skyline75489 skyline75489 mentioned this issue Aug 17, 2020
Merged
@zadjii-msft zadjii-msft added 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. Product-Conhost For issues in the Console codebase and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Aug 17, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 17, 2020
@skyline75489 skyline75489 changed the title Proprocess and convert C1 controls to their 7 bit equivalent Preprocess and convert C1 controls to their 7 bit equivalent Aug 17, 2020
@zadjii-msft zadjii-msft added this to the Console Backlog milestone Aug 18, 2020
@jdebp
Copy link

jdebp commented Aug 18, 2020

Surely the other way around is better? Technically, per ECMA-48, it's those actual C1 characters that delimit command strings and control strings, and that begin control sequences. ESC Fe sequences are just ways to represent them for 7-bit transports. It has been almost 4 decades since the world went largely 8-bit clean.

@j4james
Copy link
Collaborator

j4james commented Aug 18, 2020

Surely the other way around is better?

Semantically does it make any difference? The only reason I was suggesting converting from the C1 controls to the 7-bit escapes was because I thought that would be easier to handle the way our parser is setup.

It has been almost 4 decades since the world went largely 8-bit clean.

That may be so, but in practice most people are using UTF-8 nowdays, so those C1 controls don't really gain you anything and are very rarely supported (at least in my experience).

@skyline75489 skyline75489 mentioned this issue Aug 19, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Aug 19, 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 Aug 20, 2020
@skyline75489
Copy link
Collaborator Author

If I'm not mistaken, converting ESC Fe to C1 characters would require looking ahead one character beyond ESC. That would also be troublesome. I'd prefer the other way around which is techinically the same.

@jdebp
Copy link

jdebp commented Aug 29, 2020

You're looking to the next character anyway, otherwise you simply aren't doing escape sequences right. And of course converting to the C1 characters means that scanning for (say) ST at the end of a command string or control string is just a single character test.

@skyline75489
Copy link
Collaborator Author

Well that’s not wrong but we are somewhat stuck with the current implementation that’s prioritizing 7bit instead of 8bit. And you can check out my PR #7340 if you’re interested.

@ghost ghost closed this as completed in #7340 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 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
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:

@DHowett DHowett mentioned this issue Oct 8, 2020
Closed
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. 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
5 participants
@DHowett @j4james @skyline75489 @zadjii-msft @jdebp