Add support for colon separated sub-parameters

[tusharsnx]

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

• Use of sub parameters must not have any effect on the output.
• Skip parameters with unexpected set of sub parameters.
• Skip sequences with unexpected set of sub parameters.

References #4321
References #7228
References #15599
References xtermjs/xterm.js#2751
Closes #4321

[zadjii-msft]

as a general FYI, @j4james is pretty much the owner of this whole section of the codebase at this point, so I'd appreciate their S/O

[zadjii-msft]

I'm generally cool with this, though I'll hold my ✅ to make sure that @j4james gets eyes on this.

[j4james]

I have some issues with the standards compliance of this implementation.

Per STD 070, section 3.5.3.1, "The bit combination 3/10 is reserved for future standardization. If 3/10 is received within a parameter string, the entire sequence up to and including the final character shall be ignored." If we accept the ODA documentation as the "future standardization", that only defines 3/10 usage in the SGR sequence.¹ Outside of that the original rules still apply.

So something like \e[10:20H should still be a no-op. It should not be interpreted as \e[10H.

It's arguable that colons could now be accepted in controls with selective parameters, but even then, any parameter substring that we don't yet support is the equivalent of an unimplemented parameter. And section 3.5.1.3 says "unimplemented selective parameter values of control sequences shall be ignored as if that parameter were not received."

So something like \e[?5:10h should also be no-op. It should not be interpreted as \e[?5h.

The only thing up for debate is whether something like \e[?10:10;5h should be entirely ignored, or we could just ignore the initial parameter substring. But I'm leaning towards ignoring the entire thing until parameter substrings have defined usage in that particular control sequence.

Then within SGR itself, any parameter substrings that we don't yet recognise are still the equivalent of unimplemented parameters, so they should still be ignored as mentioned above.

So something like \e[31:42m should be a no-op. It should not be interpreted as \e[31m. Even something like \e[4:3m should be a no-op, until we actually support extended underline sequences.

Footnotes

1. Although if we're supporting parameter substrings in SGR, we probably also need to support them in the equivalent attribute parameters of DECCARA and DECRARA. But that may introduce another level of complexity, because a colon in any of the numeric parameters should still cause the sequence to be ignored - it should only be permitted within the selective attribute parameters. ↩

[KalleOlaviNiemitalo]

@j4james, what is ODA?

[j4james]

@j4james, what is ODA?

Sorry, I should have been clearer about that. By ODA, I meant the Open Document Architecture format, ITU-T recommendation T.416 or ISO/IEC 8613-6. That's where modern terminals got the idea for the colon-based 256-color and RGB extensions, although it's not actually a terminal standard as such.

[tusharsnx]

@j4james I don't want to pretend that I understood everything what you said there, because I'm too new to all the inner workings of terminal. But I guess what you mean is, incase we don't know if a parameter accepts sub parameters or not, then we should better enter into CsiIgnore state, instead of simply parsing it further. So it's more like a opt-in to accepts sub parameter, isn't it?
I like this idea, this ensures future proofing, since if suddenly there's a new sequence(that uses sub parameter) and we don't how it works, it will be safely discarded within PTY layer (just like how it is now).

Let me see how we can achieve it on this PR. Should that address all your concerns?

[j4james]

So it's more like a opt-in to accepts sub parameter, isn't it?

@tusharsnn That's exactly right, yes. But note that the CsiIgnore state is currently handled at the StateMachine level, but you don't know what the actual control sequence is going to be used for until you've parsed the final character, and then moved on to the OutputStateMachineEngine.

My initial idea was you could have a check at the start of the ActionCsiDispatch method in OutputStateMachineEngine, and that could immediately abort if the parameters contained any sub parameters, while the id was anything other than SGR. You'd also need a similar check in ActionDcsDispatch, except that should always abort if there are any sub parameters.

What I'm not sure about is how we'd handle DECCARA and DECRARA, because only some of their parameters are allowed to have sub parameters. I think that's going to end up being a bit messy.

[tusharsnx]

What I'm not sure about is how we'd handle DECCARA and DECRARA, because only some of their parameters are allowed to have sub parameters. I think that's going to end up being a bit messy.

@j4james Can you share a reference, so that I can have a look on which parameters support sub parameters.

[j4james]

if(_acceptsSubParam(_parameters.back()))

But you can't tell whether a given parameter accepts sub parameters at this point. For example, if the parameter is a 4, it could indicate the underline attribute of an SGR sequence, but it could also be the row number 4 from a CUP sequence, or the insert-replace mode number in an SM or RM sequence, or a thousand other things.

Can you share a reference, so that I can have a look on which parameters support sub parameters.

See https://vt100.net/docs/vt510-rm/DECCARA.html and https://vt100.net/docs/vt510-rm/DECRARA.html.

The first four parameters are numeric parameters, which can't have sub parameters. The remaining parameters are rendition attributes, which are selective parameters, and should theoretically support the same values that we allow in an SGR sequence.

[tusharsnx]

@j4james

But you can't tell whether a given parameter accepts sub parameters at this point.

yeah, you're right. I realised that after I made the reply. right now, I think we should guard against both parameter and sequence to never allow parameter when we don't accept them.

So, something like \x1b[38:2::3m should rightfully enter CsiIgnore right after we parse a : after 38. This should avoid the need to check if a parameter has a sub parameter(s) when it isn't suppose to have one for every parameter in SGR .

I've updated the validation steps to test for two new scenarios.

[KalleOlaviNiemitalo]

I don't think the state machine should have any idea of what 38 means or whether it should have subparameters in SGR.

[tusharsnx]

I wanna work a little bit on ergonomics for accessing sub parameters. As it is, to get sub parameter at index 0 from the std::span of sub parameters, we would need to do til::at(subParams, idx). It'll be better if we can reduce it to subParams.at(idx) like how we do for parameters.

I'm thinking of introducing VTSubParameters class for this purpose, and later update VTParameters to hold this instead of plain std::span<const VTParameter>.

[j4james]

I've tried all the edge cases I can think of and it all looks good to me. Just a couple of nits, but you're welcome to ignore those. I appreciate you putting up with all my little technical complaints.

[zadjii-msft]

Looks great to me. Thanks for working together on this on - really excited to see what comes next 😉

I do agree about the TestOption = 12345, thing - can we yank that if it's unused?

[lhecker]

I've tested this PR and it works great. Thank you for your hard work! I have just one question before I approve it:
Why don't we just store the subparameter range right inside the VTParameter struct? That way we don't need this extra vector and the parameter slicing would be a bit simpler.

[tusharsnx]

VTParameter is base for both the _parameter and _subParameter. Storing subparameter range within VTParameter might make sense for _parameter but its should be illegal for _subParameter. But it's totally possible to implement that with an extra class based on VTParameter.

[lhecker]

I personally think it's fine to store the range for both and leave it unused for sub-parameters, because I suspect that it'll not cause serious bugs. For instance, incorrectly asking a sub-param for its sub-params in turn (= illegal) could just say that there are no sub-params available (+ abort() in debug builds). I'm not sure if that's better than your current approach, however. It was an idea that I had when I saw the VTParameters::subspan function and wondered if storing the range right inside VTParameters would make it simpler.

For me it's a ✅ anyways and I'll leave it up to you what you personally like best. We can still tune this code however we want in the future after all. 🙂

I'll ask @DHowett to give this just a quick look, maybe he got anything else in mind.

[tusharsnx]

We can still tune this code however we want in the future after all. 🙂

That's exactly what I feel like right now.

There's a lot we can improve here, but I'm gonna hold myself until I actually implement something that uses sub parameters. (Spoiler alert, ITU's colour sequence support incoming! 😅 )

[DHowett]

This is amazing, thank you. @j4james, thank you for the review and everything -- it looks like it came together so well thanks to the two of you.

Y'all, this is so cool.

[tusharsnx]

It was a great experience working on Windows Terminal for the first time ❤️, feels like an achievement to me. @j4james @zadjii-msft @DHowett @lhecker @carlos-zamora Thank you all for reviewing the PR.