Support SCP control (SELECT CHARACTER PATH)

[MoSal]

Modern usage of this control function comes from the BiDi draft proposal:

https://terminal-wg.pages.freedesktop.org/bidi/recommendation/escape-sequences.html

The draft slightly extends the definition in ECMA-48, as commented.

[kchibisov]

Also, terminal-wg is long dead, and most proposals from it were not adopted or were altered later by downstream, so adding stuff from it is a bit questionable.

[MoSal]

Why are you interested in adding this protocol?

I plan to add support for the BiDi draft in alacritty_terminal (hidden behind a feature of course), then utilize that to add full support in cosmic-term.

Will use forks if necessary. No problem.

[MoSal]

Also, terminal-wg is long dead, and most proposals from it were not adopted or were altered later by downstream, so adding stuff from it is a bit questionable.

I'm not aware of any modern attempts at standardizing BiDi support in terminals.

The draft is still implemented by VTE-based (the Gnome one) terminals. I've been testing against MATE Terminal to confirm.

Not sure what downstream you're referring to.

[MoSal]

Sorry for the delay

Ditto.

However this would assume it actually gets used, since I'd rather not add stuff just because it exists.

Agreed.

For me personally, if support in alacritty_terminal will not be accepted, then there is no point in including this in vte upstream.

[chrisduerr]

For me personally, if support in alacritty_terminal will not be accepted, then there is no point in including this in vte upstream.

Thanks for pointing this out. The patch doesn't look too bad, but it would have a massive performance impact so there's no way it could be accepted for upstream usage. Effectively just making it something we have to carry around for another terminal emulator. This just looks like a draft so I don't want to be too hasty in outright shooting it down, but I'm struggling to justify it from Alacritty's perspective.

@kchibisov Any thoughts?

[MoSal]

but it would have a massive performance impact so there's no way it could be accepted for upstream usage.

Note that there is zero code change if the feature is not enabled.

[kchibisov]

would have a massive performance impact so there's no way it could be accepted for upstream usage

I think it'll be fine as long as it's in the extra data? Though, we might slightly tune it to reduce the boilerplate to write cow stuff correctly.

The patch doesn't touch the rest of code path, so I don't see how it'll hurt. Also, I'd rather measure perf with our bot to talk about perf penalties.

[chrisduerr]

I think it'll be fine as long as it's in the extra data? Though, we might slightly tune it to reduce the boilerplate to write cow stuff correctly.

Yeah that's probably what I would recommend for making it more feasible.

Note that there is zero code change if the feature is not enabled.

Yes, this is what I meant by "upstream usage", as in actually supporting this in Alacritty. However even for alacritty_terminal I'm not sure I'd want something that actually impacts performance significantly without good reason. While this crate's goals certainly are a little less defined than Alacritty itself, I would still like to retain the principle that using alacritty_terminal should always result in a fast emulator (so it would need to go into extra data anyway).

The patch doesn't touch the rest of code path, so I don't see how it'll hurt. Also, I'd rather measure perf with our bot to talk about perf penalties.

Yeah that's true. It's just something I noticed in the patch and I don't need the bot to know that putting anything in the cell data will impact performance. Just so @MoSal is aware.

[MoSal]

I'm not sure what kind of performance regression should I be looking out for.

I ran vtebench in alacritty, first time built with defaults, then with the bidi_draft feature enabled. And I'm not seeing a clear performance regression.

A test case that would show such a regression would be most helpful.

[kchibisov]

Your system is rather noisy, the thing that regressed, I guess is that the cell size got increased, which we should never do.

[MoSal]

the thing that regressed, I guess is that the cell size got increased, which we should never do.

That, I get.

Unless I misunderstood, what's being suggested is moving bidi_flags to CellExtra like this, right?

[kchibisov]

Unless I misunderstood, what's being suggested is moving bidi_flags to CellExtra like this, right?

right, because the escape is fairly rare.

[MoSal]

I think all review concerns are addressed.