Skip to content

Remove is_color_escape_seq #8769

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

Merged
merged 1 commit into from
Mar 13, 2022
Merged

Conversation

faho
Copy link
Member

@faho faho commented Mar 8, 2022

This is supposed to detect color escape sequences, to figure out how
long an escape sequence is, for use in width calculations.

However, the typical color sequences are already taken care of by
is_csi_style_escape_seq because they look like a csi sequence starting
with \e[ and ending in m.

In the entire terminfo database shipped with ncurses 6.3, these are
the terminals that have non-csi color sequences:

at-color
atari-color
atari_st-color
d220-dg
d230-dg
d230c-dg
d430-dg
d430-unix
d430-unix-25
d430-unix-s
d430-unix-sr
d430-unix-w
d430c-dg
d430c-unix
d430c-unix-25
d430c-unix-s
d430c-unix-sr
d430c-unix-w
d470-dg
d470c-dg
dg+fixed
dgmode+color
dgmode+color8
dgunix+fixed
emu
fbterm (this has an uncommon ending char of }, but we account for that)
i3164
ibm3164
linux-m1b
linux-m2
minitel1
minitel1b
putty-m1b
putty-m2
st52-color
tt52
tw52
tw52-color
xterm-8bit

Most of these were discontinued in the 90s and their manufacturers no
longer exist (like Data General, which went defunct in 1999). The last one is a special mode for xterm that is fundamentally UTF-8 incompatible because it encodes a CSI as \X9b (a valid UTF-8 continuation byte) - note that this means we already don't support it because it starts escape sequences with a non-escape character.

The linux/putty m1b and m2 entries (also for minitel) don't support
color to begin with and the sequences they have in their terminfo
entries are single control characters anyway, so the calculation would still
add up (because they are 0 wide).

In turn, what we gain from this is much faster width calculations with
unrecognized escapes -
e.g. string length -V \efoo is sped up by a factor of 20.

An alternative would be to skip this if max_colors is > 16 as that is
the most any of these entries can do. The runtime scales linearly with
the number of colors so on those systems it would be reasonably quick anyway.

But given just how outdated these are I believe it is okay to just
remove support outright. I do not believe anyone has ever run fish on
any of these.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • [N/A] Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

This is supposed to detect color escape sequences, to figure out how
long an escape sequence is, for use in width calculations.

However, the typical color sequences are already taken care of by
is_csi_style_escape_seq because they look like a csi sequence starting
with `\e[` and ending in `m`.

In the entire terminfo database shipped with ncurses 6.3, these are
the terminals that have non-csi color sequences:

at-color
atari-color
atari_st-color
d220-dg
d230-dg
d230c-dg
d430-dg
d430-unix
d430-unix-25
d430-unix-s
d430-unix-sr
d430-unix-w
d430c-dg
d430c-unix
d430c-unix-25
d430c-unix-s
d430c-unix-sr
d430c-unix-w
d470-dg
d470c-dg
dg+fixed
dgmode+color
dgmode+color8
dgunix+fixed
emu
fbterm
i3164
ibm3164
linux-m1b
linux-m2
minitel1
minitel1b
putty-m1b
putty-m2
st52-color
tt52
tw52
tw52-color
xterm-8bit

Most of these were discontinued in the 90s and their manufacturers no
longer exist (like Data General, which went defunct in 1999). The last one is a special mode for xterm that is
fundamentally UTF-8 incompatible because it encodes a CSI as \X9b.

The linux/putty m1b and m2 entries (also for minitel) don't support
color to begin with and the sequences they have in their terminfo
entries are control characters anyway, so the calculation would still
add up.

In turn, what we gain from this is much faster width calculations with
unrecognized escapes -
e.g. `string length -V \efoo` is sped up by a factor of 20.

An alternative would be to skip this if max_colors is > 16 as that is
the most any of these entries can do. The runtime scales linearly with
the number of colors so on those systems it would be reasonably quick anyway.

But given just *how* outdated these are I believe it is okay to just
remove support outright. I do not believe anyone has ever run fish on
any of these.
@faho faho added enhancement performance Purely performance-related enhancement without any changes in black box output labels Mar 8, 2022
@faho faho added this to the fish-future milestone Mar 8, 2022
@zanchey
Copy link
Member

zanchey commented Mar 10, 2022

Looks like FbTerm is still around? Though not terribly popular - https://qa.debian.org/popcon.php?package=fbterm

A cursory look shows that it is using CSI-style escapes for seta/setab - https://salsa.debian.org/debian/fbterm/-/blob/master/terminfo/fbterm

@faho
Copy link
Member Author

faho commented Mar 10, 2022

A cursory look shows that it is using CSI-style escapes for seta/setab - https://salsa.debian.org/debian/fbterm/-/blob/master/terminfo/fbterm

Ooooh, apparently I had misread "csi-style" as ending with "m" - which is how I know it.

It ends with anything between "@" and ~ - which includes much more, including A-Z and a-z, and also } like fbterm uses.

So the fbterm sequences would actually still be caught. All the others don't start with a \e[ CSI, so they won't be.

@faho faho modified the milestones: fish-future, fish 3.5.0 Mar 13, 2022
@faho faho merged commit a785919 into fish-shell:master Mar 13, 2022
@faho faho deleted the no-more-special-colors branch March 13, 2022 10:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement performance Purely performance-related enhancement without any changes in black box output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants