microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.7k stars 8.33k forks source link

Add support for multi-param OSC sequences #942

Open zadjii-msft opened 5 years ago

zadjii-msft commented 5 years ago

@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 https://github.com/microsoft/terminal/pull/891#issuecomment-494667974

oising commented 5 years ago

@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.

skyline75489 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

TBBle commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

that function just wants to be refactored. ^_^

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

skyline75489 commented 4 years ago

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