Skip to content
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

Enable 24-bit mode for selected terminals #2495

Closed
wants to merge 3 commits into from

Conversation

faho
Copy link
Member

@faho faho commented Oct 20, 2015

Unfortunately, there's no standard way to detect support (importantly,
terminfo doesn't encode it), but there's a variety of terminals that
support it that we can detect.

It's better than letting this functionality go to waste.

(Note that I'm the wrong one to see if this is working as I'm mildly color-blind)

Unfortunately, there's no standard way to detect support (importantly,
terminfo doesn't encode it), but there's a variety of terminals that
support it that we can detect.

It's better than letting this functionality go to waste.
It seems Konsole can be compiled without dbus support.
@ridiculousfish
Copy link
Member

I think the problem with iTerm is that the nightlies support it but the official release does not.

@faho
Copy link
Member Author

faho commented Oct 20, 2015

How about the general approach? @zanchey thinks it's ugly, I think it's not too ugly (though I can't claim it's beautiful).

Should we then remove the iTerm line for now or wait with merging this until the next release (if one is imminent)?

@pickfire
Copy link
Contributor

@faho, I think that is a bit ugly too. I think the terminal that doesn't support is lesser than those that support true colors. I think you have missed some of the terminal that support true colors. Look at:

https://gist.github.com/XVilka/8346728#now-supporting-truecolor

@faho
Copy link
Member Author

faho commented Oct 21, 2015

I think the terminal that doesn't support is lesser than those that support true colors.

The problem is I don't know how to detect those (it's likely all of them claim to be "xterm" via $TERM) - and if I did, I'd have to detect all of them, no false-negatives (i.e. no enabling truecolor when the terminal doesn't support it since that leaves ugly escapes). So I just don't see how a blacklist is going to work (plus I've been waiting for the next season for ages now).

@ridiculousfish
Copy link
Member

I wonder if it would be OK to emit the old-style color escapes and then the 24-bit ones. This might handle old versions of the affected terminals. I verified st 0.3 and iTerm2 2.0 (which don't support 24 bit) swallow the 24-bit escapes, without spewing anything to console.

@pickfire
Copy link
Contributor

I don't think there will be someone will be using an antique version of st which is 3 years old now.

@krader1961
Copy link
Contributor

I agree with @faho that a blacklist approach is not practical. Anyone using an ancient (i.e., more than a year old) version of a software terminal (as opposed to a hardware terminal) is on their own and should not be accommodated unless doing so is trivial (e.g., via an env var provided by the terminal). In other words we should only whitelist terminals which have had support for 24-bit escapes if that software has been widely available for at least a year or can be cheaply distinguished from older non 24-bit supporting versions.

My main concern regards the complexity of the if statement. It seems to me like this logic is only going to become more complicated. I prefer that it be moved into a separate file which is sourced and tests each whitelisted terminal independently of the others and short-circuits the testing on the first match. That allows for easier extension to other terminals such as those identified in https://gist.github.com/XVilka/8346728#now-supporting-truecolor as well as making it easier to provide more detailed comments regarding each test.

@faho
Copy link
Member Author

faho commented Dec 9, 2015

To keep everyone in the loop:

From the oft-cited list at https://gist.github.com/XVilka/8346728#now-supporting-truecolor, I can't test the windows terms, and qterminal defaults to setting TERM=xterm (WHYYYYYYYY?). All others are included here.

My main concern regards the complexity of the if statement. It seems to me like this logic is only going to become more complicated. I prefer that it be moved into a separate file which is sourced and tests each whitelisted terminal independently of the others and short-circuits the testing on the first match.

I strongly doubt this is going to need some really heavy logic - at most it's a string of or'd checks for supporting terms, with each check being as complicated as "TERM = val -a TERMVER -ge XXYYZZ" at most.

In other words, the VTE-check is probably the most complicated single check we'll need.

If that is the case, I don't see what moving it to a separate file can help.

(Though looking back, I'd probably write the COLORTERM check with contains)

@krader1961
Copy link
Contributor

I'm skeptical this won't grow into a hard to understand block but we can cross that bridge when we reach it. The Travis CI failure is unrelated to this PR so I don't see any reason not to merge it as is.

@faho
Copy link
Member Author

faho commented Dec 9, 2015

@ridiculousfish, @zanchey: Any objections?

I'm not sure how long you want to wait until the next release, I think this requires some wider testing.

@ridiculousfish
Copy link
Member

I just confirmed that this breaks color on the official iTerm2 release (which is from July 2014). We can't merge this as-is.

@gnachman we would like to enable 24-bit color on iTerm2. Do you have guidance or ideas on how the shell can detect whether iTerm2 supports it? Maybe you can add something to the nightly - perhaps define the COLORTERM env variable appropriately, or return it from some appropriate ioctl?

@gnachman
Copy link

Yeesh, I really need to get 3.0 out of beta.

You won't be able to detect it over an ssh connection, but locally at least you can check the format of $ITERM_SESSION_ID. In 2.1.x and earlier it will look like w0t1p1. In 2.9 and later it will look like w0t1p1:E1DA525D-9B0A-4263-835D-479AB3435F31. Checking for the presence of a colon should be sufficient.

@ridiculousfish
Copy link
Member

Nifty, thanks

@ridiculousfish
Copy link
Member

ping @faho to incorporate gnachman's guidance, then I think it's worth merging it and seeing if anyone squawks.

@krader1961
Copy link
Contributor

Argh! I use iTerm2 v2.1.3 and thought I had confirmed it supported 24bit colors under fish. Turns out I had a typo which invalidated the results. The current GA release does not support 24bit colors (nothing bad happens but you don't get any coloration). I built and launched iTerm2 from the current GitHub repository and can confirm that a) 24bit colors do work with the latest beta build, and b) $ITERM_SESSION_ID includes the extra colon-separated data mentioned by @gnachman. Guess I'll be running from a locally built binary for the foreseeable future.

@ridiculousfish
Copy link
Member

@krader1961 most people who want to live on the iTerm2 bleeding edge use the nightlies: https://iterm2.com/downloads/nightly/#/section/home . They have a prompt-to-update feature so you stay current. That's what I use.

This has changed since the last release, just like 24bit support. So if
we check one, we get the other.
@faho
Copy link
Member Author

faho commented Dec 10, 2015

Merged as b73bf53. Let's see if we get any issues. Thanks, everyone!

@faho faho closed this Dec 10, 2015
@faho faho added this to the next-2.x milestone Dec 10, 2015
@faho faho added enhancement release notes Something that is or should be mentioned in the release notes labels Dec 10, 2015
@faho faho deleted the enable-truecolor branch December 10, 2015 11:49
@ridiculousfish
Copy link
Member

Thanks for taking this on faho!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement release notes Something that is or should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants