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

Support colons as a VT parameter separator #4321

Closed
atwok opened this issue Jan 21, 2020 · 19 comments · Fixed by #15648
Closed

Support colons as a VT parameter separator #4321

atwok opened this issue Jan 21, 2020 · 19 comments · Fixed by #15648
Assignees
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Milestone

Comments

@atwok
Copy link

atwok commented Jan 21, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.19041.21]
Windows Terminal version (if applicable): 0.8.10091.0

Any other software?  WSL, Ubuntu

Steps to reproduce

I have TERM set to xterm-24bit using the standard termconfig instructions that are online. Running emacs in terminal mode (emacs -nw) and then using M-x list-display-colors

Behavior (windows terminal on the left, wsltty on the right)

image

In both cases, emacs knows about all 500+ colors available, but wsltty shows them and windows terminal doesn't. Also this has been like this for a couple months at least, but now it's bugging me enough to ask for a fix. :)

@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 Jan 21, 2020
@zadjii-msft
Copy link
Member

Does this repro in a legacy console window? (i.e. not wsltty, but conhost.exe running wsl directly)?

Could you link to the

standard termconfig instructions

for xterm-24bit?

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 21, 2020
@j4james
Copy link
Collaborator

j4james commented Jan 21, 2020

Yeah, the terminfo definition is likely to be the issue here. I'm guessing it's using the colon separator syntax for 24-bit colors which mintty supports but we don't. If that's the case you can just replaces the colons with semicolons in the terminfo file and it should be fine.

@j4james
Copy link
Collaborator

j4james commented Jan 21, 2020

Actually, I've just found the instructions in the Emacs FAQ for setting up the 24bit terminfo (see here). They create two terminfo entries: one using the colon syntax, and one with the semicolon syntax. If those are the instructions you followed, then you can just set your TERM to xterm-24bits (with an s) and it should use the semicolon syntax.

@atwok
Copy link
Author

atwok commented Jan 21, 2020

Oh geez. You are correct. I switched to my already defined xterm-24bits and that worked.

So yeah, this is not a bug. Maybe a request to indicate if terminfo isn't being parsed correctly due to colons?

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jan 21, 2020
@DHowett-MSFT
Copy link
Contributor

I'm not sure we can generally know that. Alas!

@j4james
Copy link
Collaborator

j4james commented Jan 21, 2020

It's probably worth adding support for the ITU colon syntax to the backlog, though. It may not be as widely used as the semicolon syntax, but it's still worth supporting if possible.

@zadjii-msft
Copy link
Member

zadjii-msft commented Jan 21, 2020

It's probably worth adding support for the ITU colon syntax to the backlog, though. It may not be as widely used as the semicolon syntax, but it's still worth supporting if possible.

Let's re-purpose this issue as that thread then 😄

Relevant:
https://gitlab.com/gnachman/iterm2/issues/6377

@zadjii-msft zadjii-msft changed the title 24 bit color in emacs terminal mode stays in black and white Support colons as a VT parameter separator Jan 21, 2020
@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. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Jan 21, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 21, 2020
@zadjii-msft zadjii-msft added this to the Console Backlog milestone Jan 21, 2020
@egmontkob
Copy link

See XVilka's gist, in particular:

  • This comment about why it's important to get the parser right, and why lazy programmer approaches (e.g. treating : as a synonym for ;) are truly harmful;
  • A few comments starting here (Dec 2-5, 2018) how the spec is unclear about the first separator being : or ; (note though that implementations tend to go with : as that clearly makes more sense);

and of course the spec ITU T.416 itself, too.

@egmontkob
Copy link

[On a side note... Even after years of developing a terminal, I still keep getting uncertain whether nowadays' most popular terminal description is called xterm-256color or xterm-256colors. Emacs's decision of introducing something that is seemingly a singular vs. plural difference to denote the separator (that final s in xterm-24bits presumably standing for semicolon) was... let's just say probably not a great idea :)]

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 23, 2020
@DHowett-MSFT
Copy link
Contributor

Pulling the triage tag on this. This seems totally reasonable, and I appreciate your insight on how we shouldn't just treat : and ; as equivalent, @egmontkob 😄

Thanks!

@DHowett
Copy link
Member

DHowett commented Aug 8, 2020

I'm working on this. First step is #7220, and the additional delta over switching the VT engine to it is +/- 26/2 lines (not accounting for tests). 😄

@TBBle
Copy link

TBBle commented Sep 21, 2020

Since I don't see it mentioned anywhere here, ansi-vte52.sh briefly demonstrates the ITU-T T.416 forms of SGR 38 and SGR 48, including an incorrect form of the truecolour specification (where the colourspace ID was missed) apparently originating from the KDE konsole application, although "pretty much everyone" independently made the same mistake.

@mixmastamyk
Copy link

This is an important feature, hopefully it can get some attention. As long as popular terminals don't support the correct syntax it incentivizes everyone to continue building with and relying on the broken syntax.

To be honest, it probably should have gone in before 1.0 was declared to minimize the damage. Windows has an oversized influence and so a greater responsibility, just like Spider-man. ;-)

jazzdelightsme added a commit to jazzdelightsme/PowerShell that referenced this issue Mar 4, 2021
The ControlSequenceLength function calculates the length of a VT control
sequence--used by LengthInBufferCells. SGR control sequences might have
parameters, separated by semicolons... or possibly colons. See:

microsoft/terminal#4321

This change updates ControlSequenceLength to treat colons similarly to
semicolons.
jazzdelightsme added a commit to jazzdelightsme/PowerShell that referenced this issue Mar 4, 2021
The ControlSequenceLength function calculates the length of a VT control
sequence--used by LengthInBufferCells. SGR control sequences might have
parameters, separated by semicolons... or possibly colons. See:

microsoft/terminal#4321

This change updates ControlSequenceLength to treat colons similarly to
semicolons.
@229c9cf0
Copy link

I'd love to see this happen soon-ish. I have a bunch of in-house tools that have to support both fancy (256 / truecolor) and limited (16-color only) terminals, and the lack of support for the : separator means Windows Terminal doesn't get full colors.

I can safely use the \e[...;38:2:R:G:B;...m form and the truecolor part will be ignored as a single unknown control sequence by the old 16-color terminal. But if I were to use ; as a separator, it will instead interpret it as 5 separate control sequences, and then ignore those that it doesn't recognize and interpret the others, and then I end up e.g. with blinking inverted text and stuff like that.

I can't just detect the terminal type, as large parts of the output are static text files that have been pre-generated. (And I can't justify re-architecting that just for Windows Terminal.) So I'm stuck with only using : as a separator, and effectively only getting 16 colors on Windows.

@j4james
Copy link
Collaborator

j4james commented Sep 12, 2021

I can safely use the \e[...;38:2:R:G:B;...m form and the truecolor part will be ignored as a single unknown control sequence by the old 16-color terminal.

@229c9cf0 Just FYI, there are terminals out there that will simply echo the sequence parameters to the screen when they encounter a colon in an escape sequence, so it's not really that safe. Also, the "correct" syntax for the colon format is actually 38:2::R:G:B. Some terminals support both colon variants, some only support one or the other, but everyone seems to support the semicolon syntax (assuming they support true color at all).

And I can't justify re-architecting that just for Windows Terminal

I don't know what your target audience is, but in case you aren't aware, Windows Terminal is by no means the only terminal that doesn't support the colon syntax. You can get a general idea of the level of support at this website: https://gist.github.com/XVilka/8346728. But note that it's not always correct, so it's best to confirm for yourself. For example, I'm pretty sure that Konsole doesn't support colons, while more recent versions of Alacritty actually do support colons now.

@TBBle
Copy link

TBBle commented Sep 13, 2021

You can get a general idea of the level of support at this website: https://gist.github.com/XVilka/8346728. But note that it's not always correct, so it's best to confirm for yourself.

Updates for that Gist now go to https://github.com/termstandard/colors, and they also take PRs to fix out of date or wrong info.

@zadjii-msft zadjii-msft modified the milestones: Console Backlog, Backlog Jan 4, 2022
danschwarz referenced this issue in AnonymouX47/term-image May 30, 2023
- Fix: Use the colon-separated SGR FG/BG direct color control sequences
  instead of the semi-colon-separated variation by KDE Konsole.
@KalleOlaviNiemitalo
Copy link

Perhaps this could be implemented by adding a bool isSubparameter member to VTParameter, populating that in StateMachine::_ActionParam(const wchar_t wch), and making AdaptDispatch::_ApplyGraphicsOptions skip any subparameters not handled by AdaptDispatch::_ApplyGraphicsOption.

@tusharsnx
Copy link
Contributor

Since this is going to be closed after #15648, I've opened a new issue #15706 to track ITU T.416 (inspired) color sequence.

@DHowett
Copy link
Member

DHowett commented Jul 14, 2023

Thanks so much for doing all this work! I'm really excited to dig into your PR.

DHowett pushed a commit that referenced this issue Jul 18, 2023
Adds support for colon `:` separated sub parameters in parser.
Technically, after this PR, nothing should change except, now sub
parameters are parsed, stored safely and we don't invalidate the whole
sequence when a `:` is received within a parameter substring.

In this PR:
- If sub parameters are detected with a parameter, but the usage is
unrecognised, we simply *skip* the parameter in `adaptDispatch`.
- A separate store for sub parameters is used to avoid too many changes
to the codebase.
- We currently allow up to `6` sub parameters for each parameter, extra
sub parameters are *ignored*.
- Introduced `VTSubParameters` for easy access to underlying sub
parameters.

> **Info**: We don't use sub parameters for any feature yet, this is
just the core implementation to support newer usecases.

## Validation Steps Performed
- [x] Use of sub parameters must not have any effect on the output.
- [x] Skip parameters with unexpected set of sub parameters.
- [x] Skip sequences with unexpected set of sub parameters.

References #4321
References #7228
References #15599
References xtermjs/xterm.js#2751
Closes #4321
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jul 18, 2023
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 Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.