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

Refine terminal analysis for color console output #1752

Merged
merged 1 commit into from
Aug 26, 2017

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Aug 24, 2017

For Unix/Linux, check the TERM env variable to be sure the particular
terminal supports color output. Inspired by Google Benchmark.

// be capable of the color codes. List copied from the Apache-licensed
// Google Benchmark: https://github.com/google/benchmark/blob/master/src/colorprint.cc
const char* const supported_terminals[] = {
"cygwin", "linux", "rxvt-unicode", "rxvt-unicode-256color",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps asking tput on Linux would be better than using a hard coded list? It has all the plumbing to dig through the system's termcap database stuff

will@Tower:$ tput colors
8
will@Tower:
$ TERM=vt100 tput colors
-1
will@Tower:~$ TERM=xterm-256color tput colors
256

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a bad idea, I guess I can try that.

Realistically, is anybody using a terminal that doesn't support the basic color text usage that our Term class needs?

The current (pre-this-PR) code doesn't check at all, it just assumes all terminals will work. Nobody has complained, though to be fair I think the only place we use it is in tests, not in any critical path.

I just happened to be browsing through the code for Google test and benchmark projects (for other reasons) and ran across this and borrowed it. I figured it was a step up from a blanket assumption, and there was no harm, and if it's good enough for Google, it probably is good enough for us.

So the question is just whether it's worth the trouble, and whether there are any cases where tput will not reliably be found? (I guess if not able to run tput, it can fall back on this test.)

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 25, 2017

Upon thinking about the comment from @wrosecrans a bit more, here's where I'm at:

The current proposal of this PR is certainly a step forward. It could be better, but in a practical sense it's likely to be good enough for our anticipated use cases (confirming evidence: it's good enough for Google Test & Google Benchmark).

The tput idea is good; I'm not criticizing its merits. But I'd need to implement it, test it (ugh), and also get side-tracked into writing an interface for wrapping popen in a portable way in order to capture the output of the tput command. Which feels like a bit of an unwelcome rabbit hole to descend, given the current pile of more important things on my plate.

So I'm inclined to commit this modest improvement as-is, with the addition of a comment suggesting that room for future improvement could include tput or some other more thorough and robust assessment of terminal capabilities, should we find ourselves in a future that necessitates more heavy reliance on terminal formatting as an important feature and a set of users with terminal windows that can't be counted on to support the set of terminal features we use.

@wrosecrans
Copy link
Contributor

As far as I can tell, the worst case scenario is slightly less colorful text for for a handful of users with very unusual terminal configurations... Please don't let my nit-picking discourage you from adding a feature. If I'm too lazy to implement the suggestion, it probably shouldn't carry much weight, anyway. :)

I tend to reflexively complain about any hard coded list of things regardless of whether it's actually a problem. It honestly only seemed important to me because I have been experimenting with odd terminal configurations recently to see which ones support "sixel" graphics in the terminal. (And I figured out how to use tput to make the world's most annoying blinking red shell prompt on a few production servers that people shouldn't be logging into directly.)

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 26, 2017

I do appreciate the suggestion, I think your instinct is right, I just don't have time now to fully investigate. I am adding a comment in the code to explain this. Since it's not on a critical path, and the worst that can happen is that some terminal fails to print in color, I'm going to commit this as-is for now.

For Unix/Linux, check the TERM env variable to be sure the particular
terminal supports color output. Inspired by Google Benchmark.
@lgritz lgritz merged commit 6c69c89 into AcademySoftwareFoundation:master Aug 26, 2017
@lgritz lgritz deleted the lg-termcolor branch August 26, 2017 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants