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

Add support for multi-param OSC sequences #942

Open
5 of 7 tasks
zadjii-msft opened this issue May 22, 2019 · 9 comments
Open
5 of 7 tasks

Add support for multi-param OSC sequences #942

zadjii-msft opened this issue May 22, 2019 · 9 comments
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented May 22, 2019

@vapier wrote:

i don't think this correctly handles chaining to following OSC sequences.

That's correct. It doesn't handle chaining at all. It also doesn't handle color names.

Since this was my first PR, I was trying not to rewrite, but to just add a little on top of what was there and learn how things work -- I even reused the color parser that was written for OSC 4, though it won't support color names, meaning you can only set RGB values, and not just reuse colors from your base 16 for foreground and background.

The structure of the OSC handler in the State Machine seems designed on the assumption that they are single-valued (meaning one value per escape sequence), which makes chaining basically impossible.

Basically, still to do:

Somewhere along the way someone will need to decide if we're going to do special colors for bold/italic/underline/blink/reverse ... and which window properties should be settable by OSC 3 😉 @oising?

Originally posted by @Jaykul in #891 (comment)

@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 May 22, 2019
@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase labels May 22, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 22, 2019
@oising
Copy link
Collaborator

oising commented May 23, 2019

@zadjii-msft That's a fun one alright. I'm looking for something a bit simpler at first to cut my teeth on, as the last time I wrote C++, MFC was the big shiny thing.

With respect to OSC 3, and window props, if it's the kind of window properties like pixel size, text area size, window titles, icons etc, then I think the right approach is to do what xterm does. The have some flags that this light up a number of CSI sequences for querying and setting window properties (focus, maximize, fullscreen, report text area size in cols/rows, report window size in pixels, etc.) These came from dtterm, and the X guys added some more. Some of them are probably going to be problematic (abuse) to enable out of the box, so there is a blacklist (disallowedWindowOps) and the feature flag itself is allowWindowOps.

Drop the following into your ~/.Xdefaults and fire up xterm and try echo -e "\e[18t" to get the visible textarea in rows/columns, for example.

*VT100.allowWindowOps: true
*VT100.allowTitleOps: true
*VT100.allowFontOps: true

But back to OSC 3, it seems that's a more generalized mechanism for getting/setting arbitrary properties, whereas the above is well-known.

@DHowett-MSFT DHowett-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. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels May 23, 2019
@zadjii-msft zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. and removed Issue-Task It's a feature request, but it doesn't really need a major design. labels Nov 26, 2019
@zadjii-msft zadjii-msft added this to the Console Backlog milestone Nov 26, 2019
@skyline75489
Copy link
Collaborator

So... I was trying to work on the multi-param OSC issue. Funny that I can not find any documentation indicating that there is support for multi-param OSC sequences. Not in http://www.xfree86.org/4.5.0/ctlseqs.html. Not in anywhere google can lead me.

Am I missing something or just I'm bad at google..

@TBBle
Copy link

TBBle commented Sep 10, 2020

The xterm docs you linked mention that OSC 4 takes multiple parameters, and it can take an arbitrarily long list of pairs of parameters as a shorthand for repeating the OSC 4 call:

Any number of c name pairs may be given.

OSC 10 through OSC 17 also can take multiple parameters, but in this case, it's a shorthand for calling each of OSC 10-17 in turn.

One or more parameters is expected for P t . Each succesive parameter changes the next color in the list. The value of P s tells the starting point in the list.

(That xterm doc is hard to read, there's a bunch of missing newlines in the middle of the OSC description text, which makes it hard to find the above text, and also hard to parse it.)

The quoted text in the issue description was specifically referring to these behaviours of OSC 4 and OSC 10-17, but out of context, it appears to be talking about generically chaining different OSC commands, the way CSI commands can be.

mintty's extensive set of OSC commands includes a few multi-param commands such as OSC 8, and OSC 1337, which are both pretty-specific in how they're parsed. OSC I (i.e. icon) seems to use a comma in its parameter as a separator, and OSC 1337 uses : to separate its final parameter.

Another example, hterm documents OSC 4, OSC 52 and OSC 777 as multiparam.

We actually have OSC 52 implemented already via #5823, although a quick glance at the implementation shows no attempt to generically handle multi-parameter OSC sequences.

A challenge for a generic OSC parameter parser is that some OSC commands are allowed to have ; in their final parameter, e.g. OSC 0, or OSC 8.

After looking at the above, it makes sense to me that there's no generic multi-parameter OSC handling, each OSC sequence is specific in its parameter formats. They don't always use ; as a parameter separator, and you can't blindly split by ; to get an arbitrary list of parameters, except in specific cases like OSC 4 and OSC 10-17 which are defined that way.

So I would interpret this ticket not as "Implement a multi-param OSC command handler" but "fix the current OSC sequence handler so that multi-param OSC commands can work", which unblocks actually implementing some of the above sequences.

And looking at the code, and considering the implementation of OSC 52, I'm not sure what the actual problem is. The OSC handler gets the entire OSC string (everything after the first ;, which appears to be a consistent convention) to break up as necessary.

Edit: The dispatcher could probably be nicer, right now it's split into 'parse OSC' and 'dispatch action based on OSC', and has a bunch of local variables to carry between the two. It's not clear why we have two suspiciously similar switch statements one after the other. It's very possible I'm overlooking something, but that function just wants to be refactored. ^_^

More edit: Hmm. There is some implicit assumption in the current dispatcher that the OSC is a number, apparently. I'm not sure how OSC l or OSC I would work with this, for example. It's a shame we already have OSC 1 and OSC 50 meaning something different, because otherwise this would be a use-case for a roman-numeral-supporting atoi.

@skyline75489
Copy link
Collaborator

Thanks @TBBle for the thorough and detailed writeup. I’m sure it will help us towards a better implementation.

@TBBle
Copy link

TBBle commented Sep 13, 2020

I happened across this code, which is where (or least part of where) the assumptions about OSC commands being all-numeric will need to be unwound. Happily, it doesn't embed any assumption about the data after the first ;, so we don't have any issues with the "multi-param" OSC commands.

@skyline75489
Copy link
Collaborator

Another note: For \e]10;green;white\e\\, GNOME Terminal(VTE) actually ignores the second parameter. Only the foreground is working.

Xterm will faithfully make both parameter working on foreground and background respectively .

@j4james
Copy link
Collaborator

j4james commented Sep 13, 2020

Yeah, setting the 10+ range was rarely supported in the terminals I tested - only XTerm and Kitty could set both the OSC 10+ range and the OSC 4 range. A couple more (including VTE) could set the OSC 4 range, but not OSC 10+. And Alacritty could handle OSC 10 but not OSC 4. This is not something I'd want to rely on if I were writing an app.

@DHowett
Copy link
Member

DHowett commented Sep 18, 2020

that function just wants to be refactored. ^_^

yeah we have some debt here. they're all written like that :(

DHowett pushed a commit that referenced this issue Oct 15, 2020
* Correct the behaviour of parsing `rgb:R/G/B`. It should be interpreted
  as `RR/GG/BB` instead of `0R/0G/0B`
* Add support for `rgb:RRR/GGG/BBB` and `rgb:RRRR/GGGG/BBBB`. The
  behaviour of 12 bit variants is to repeat the first digit at the end,
  e.g. `rgb:123/456/789` becomes `rgb:1231/4564/7897`.
* Add support for `#` formats. We are following the rules of
  [XParseColor] by interpreting `#RGB` as `R000G000B000`.
* Add support for XOrg app color names, which are supported by xterm, VTE
  and many other terminal emulators.
* Multi-parameter OSC 4 is now supported.
* The chaining of OSC 10-12 is not yet supported. But the parameter
  validation is relaxed by parsing the parameters as multi-params but
  only use the first one, which means `\e]10;rgb:R/G/B;` and
  `\e]10:rgb:R/G/B;invalid` will execute `OSC 10` with the first color
  correctly. This fixes some of the issues mentioned in #942 but not
  all of them.

[XParseColor]: https://linux.die.net/man/3/xparsecolor

Closes #3715
@skyline75489
Copy link
Collaborator

Pinging for updating the issue description now that #7578 is merged.

ghost pushed a commit that referenced this issue Feb 4, 2021
This adds the support for chaining OSC 10-12, allowing users to set all
of them at once.

**BREAKING CHANGE**
Before this PR, the OSC 10/11/12 command will only be dispatched iff the
first color is valid. This is no longer true. The new implementation
strictly follows xterm's behavior. Each color is treated independently.
For example, `\e]10;invalid;white\e\\` is effectively `\e]11;white\e\\`.

## Validation Steps Performed

Tests added. Manually tested.

Main OSC color tracking issue: #942
OSC 4 & Initial OSC 10-12 PR: #7578

Closes one item in #942
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. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

No branches or pull requests

7 participants