Support SCP control (SELECT CHARACTER PATH) #112

MoSal · 2024-03-02T08:20:30Z

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.

chrisduerr

Why are you interested in adding this protocol?

kchibisov · 2024-03-02T08:37:11Z

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 · 2024-03-02T09:07:43Z

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 · 2024-03-02T09:12:35Z

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.

chrisduerr

Sorry for the delay, I've kinda been avoiding this PR because I'm not a huge fan of the escape.

Considering that this is part of ECMA-48 and rather simple, I'm willing to include it in vte for downstream despite not being interested in it myself. However this would assume it actually gets used, since I'd rather not add stuff just because it exists.

The "terminal-wg" stuff can be largely ignored I think. Some of its stuff falls under valid implementation-dependent behavior for ECMA-48 anyway.

src/ansi.rs

chrisduerr · 2024-03-06T01:37:30Z

src/ansi.rs

+                // SCP control
+                //
+                // Changes from ECMA-48 as proposed by the BiDi draft proposal:
+                //  * Recognize value 0 for Ps1


What purpose does this have for applications? Having a right-to-left "default" seems unfeasible to me. So effectively this would just be another way to use 1.

Note that character path actually has three meanings depending on the mode:

Explicit mode (non-default)

Think of it as prepending LRM + LRO for LTR, or RLM + RLO for RTL.

Explicit RTL mode would turn

hello

to

olleh

Default indeed does not make much since here. And LTR would always be the default.

Implicit mode (default)

In this mode, character path only determines paragraph overall direction. The BiDi algorithm is not overridden like in explicit mode. So no LRO or RLO is prepended.

Implicit RTL mode would turn

hello

to

hello

Note that this being the default mode means that BiDi-unaware applications would be used in that mode.

More on that later.

Auto (implicit) mode

In this mode, character path is least relevant, and is only used when no specific paragraph direction can be inferred from paragraph's text (e.g. paragraph is all spaces).

In this mode. Nothing is prepended if a paragraph direction can be inferred.

So, in Auto RTL mode. This would not change:

hello

This is not the default because it can be very annoying in some cases, with some lines
starting from the right, and some from the lift.

Now about the default character path.

The draft has this to say:

I find it crucial to have a means of saying “return to the terminal emulator’s default direction”. Some terminals might only implement LTR as the default, some might take it from the locale or user settings.

...

Apps mustn’t rely on the default being LTR, as it’s not necessarily the case. If an app wishes to have LTR or RTL mode, it should emit SCP 1 or SCP 2 on startup, and SCP 0 to restore the default on exit. In particular, if an app switches to explicit mode, it should definitely specify LTR or RTL too.

In my WIP implementation for cosmic-term, I allow users (via a key binding) to quickly override the implicit direction, or enable auto direction, when character path is set to default. User overrides can be very useful in some cases.

If LTR is the default, then there is no way to distinguish between BiDi-unaware apps (or Bidi-aware ones that deliberately don't set a paragraph direction), and apps that are BiDi-aware and setting LTR on purpose.

chrisduerr · 2024-03-06T01:38:46Z

src/ansi.rs

+                //
+                // Changes from ECMA-48 as proposed by the BiDi draft proposal:
+                //  * Recognize value 0 for Ps1
+                //  * Allow passing no params or Ps1 alone


Is this actually used by applications? I don't really mind it but I don't see an advantage in applications supporting a newer "spec" that is much more likely to cause only partial support.

src/ansi.rs

MoSal · 2024-03-25T15:05:37Z

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 · 2024-03-25T19:18:57Z

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 · 2024-03-25T19:40:29Z

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 · 2024-03-25T20:10:23Z

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 · 2024-03-25T23:47:23Z

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 · 2024-03-26T21:57:17Z

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 · 2024-03-26T22:23:30Z

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

MoSal · 2024-03-27T04:27:59Z

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 · 2024-03-27T06:39:53Z

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

right, because the escape is fairly rare.

https://terminal-wg.pages.freedesktop.org/bidi Implementation is hidden behind a `bidi_draft` crate feature, and depends on alacritty/vte#112. Public API is limited to: * `Cell::bidi_mode()` method which returns a `BidiMode` enum value. * `Cell::bidi_box_mirroring()` method which returns a boolean. * `Term` boolean field `bidi_disable_arrow_key_swapping`. Cell `BidiFlags` is only used internally since relations/interactions between individual flags are not straight forward, and could lead to erroneous behavior if it's all left for API consumers to figure out. `BidiDir` named fields exist in all `BidiMode` variants. This is done deliberately (instead of e.g. a `BidiMode` struct with a `BidiDir` field) to signify the different purpose `BidiDir` serves in each mode. Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>

chrisduerr

This new code is clean enough to where I am willing to merge it as-is regardless of alacritty_terminal support as long as the remaining comments are addressed.

src/ansi.rs

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. Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>

https://terminal-wg.pages.freedesktop.org/bidi Implementation is hidden behind a `bidi_draft` crate feature, and depends on alacritty/vte#112. Public API is limited to: * `Cell::bidi_mode()` method which returns a `BidiMode` enum value. * `Cell::bidi_box_mirroring()` method which returns a boolean. * `Term` boolean field `bidi_disable_arrow_key_swapping`. Cell `BidiFlags` is only used internally since relations/interactions between individual flags are not straight forward, and could lead to erroneous behavior if it's all left for API consumers to figure out. `BidiDir` named fields exist in all `BidiMode` variants. This is done deliberately (instead of e.g. a `BidiMode` struct with a `BidiDir` field) to signify the different purpose `BidiDir` serves in each mode. Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>

MoSal · 2024-04-01T01:37:57Z

I think all review concerns are addressed.

https://terminal-wg.pages.freedesktop.org/bidi Implementation is hidden behind a `bidi_draft` crate feature, and depends on alacritty/vte#112. Public API is limited to: * `Cell::bidi_mode()` method which returns a `BidiMode` enum value. * `Cell::bidi_box_mirroring()` method which returns a boolean. * `Term` boolean field `bidi_disable_arrow_key_swapping`. Cell `BidiFlags` is only used internally since relations/interactions between individual flags are not straight forward, and could lead to erroneous behavior if it's all left for API consumers to figure out. `BidiDir` named fields exist in all `BidiMode` variants. This is done deliberately (instead of e.g. a `BidiMode` struct with a `BidiDir` field) to signify the different purpose `BidiDir` serves in each mode. Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>

chrisduerr requested changes Mar 2, 2024

View reviewed changes

chrisduerr requested changes Mar 6, 2024

View reviewed changes

MoSal force-pushed the scp branch from 86f2c72 to 3958f53 Compare March 25, 2024 15:05

MoSal mentioned this pull request Mar 27, 2024

Support terminal-wg's BiDi draft proposal in alacritty_terminal alacritty/alacritty#7872

Draft

chrisduerr requested changes Mar 30, 2024

View reviewed changes

src/ansi.rs Outdated Show resolved Hide resolved

src/ansi.rs Outdated Show resolved Hide resolved

src/ansi.rs Outdated Show resolved Hide resolved

MoSal force-pushed the scp branch from 3958f53 to 6b2aa22 Compare April 1, 2024 01:31

MoSal force-pushed the scp branch from 6b2aa22 to 96de2f4 Compare April 1, 2024 01:34

chrisduerr approved these changes Apr 1, 2024

View reviewed changes

chrisduerr merged commit a971e86 into alacritty:master Apr 1, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SCP control (SELECT CHARACTER PATH) #112

Support SCP control (SELECT CHARACTER PATH) #112

MoSal commented Mar 2, 2024

chrisduerr left a comment

kchibisov commented Mar 2, 2024

MoSal commented Mar 2, 2024

MoSal commented Mar 2, 2024

chrisduerr left a comment

chrisduerr Mar 6, 2024

MoSal Mar 25, 2024

chrisduerr Mar 6, 2024

MoSal commented Mar 25, 2024

chrisduerr commented Mar 25, 2024

MoSal commented Mar 25, 2024

kchibisov commented Mar 25, 2024

chrisduerr commented Mar 25, 2024

MoSal commented Mar 26, 2024

kchibisov commented Mar 26, 2024 •

edited

MoSal commented Mar 27, 2024

kchibisov commented Mar 27, 2024

chrisduerr left a comment

MoSal commented Apr 1, 2024

Support SCP control (SELECT CHARACTER PATH) #112

Support SCP control (SELECT CHARACTER PATH) #112

Conversation

MoSal commented Mar 2, 2024

chrisduerr left a comment

Choose a reason for hiding this comment

kchibisov commented Mar 2, 2024

MoSal commented Mar 2, 2024

MoSal commented Mar 2, 2024

chrisduerr left a comment

Choose a reason for hiding this comment

chrisduerr Mar 6, 2024

Choose a reason for hiding this comment

MoSal Mar 25, 2024

Choose a reason for hiding this comment

Explicit mode (non-default)

Implicit mode (default)

Auto (implicit) mode

chrisduerr Mar 6, 2024

Choose a reason for hiding this comment

MoSal commented Mar 25, 2024

chrisduerr commented Mar 25, 2024

MoSal commented Mar 25, 2024

kchibisov commented Mar 25, 2024

chrisduerr commented Mar 25, 2024

MoSal commented Mar 26, 2024

kchibisov commented Mar 26, 2024 • edited

MoSal commented Mar 27, 2024

kchibisov commented Mar 27, 2024

chrisduerr left a comment

Choose a reason for hiding this comment

MoSal commented Apr 1, 2024

kchibisov commented Mar 26, 2024 •

edited