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

easylogging++: Generalize TERM rule for color-supporting terminals #9772

Closed
wants to merge 1 commit into from

Conversation

laanwj
Copy link
Contributor

@laanwj laanwj commented Feb 5, 2025

With newer versions of tmux, such as version 3.4 packaged by Ubuntu 24.04, tmux-256color the default TERM variable value.

Instead of adding it specifically, extend the rule to look for -color or -256color in TERM, as these can all be assumed to support color.

@laanwj laanwj force-pushed the 2025-02-tmux-256color branch from 53aa757 to e0dbf91 Compare February 5, 2025 21:44
@laanwj laanwj changed the title easylogging++: Add tmux-256color to color-supporting terminals easylogging++: Generalize TERM rule for color-supporting terminals Feb 5, 2025
With newer versions of tmux, such as version 3.4 packaged by Ubuntu
24.04, `tmux-256color` is the default TERM variable value.

Instead of adding it specifically, extend the rule to look for
`-color` or `-256color` in TERM, as these can all be assumed to support
color.
@laanwj laanwj force-pushed the 2025-02-tmux-256color branch from e0dbf91 to d588a99 Compare February 5, 2025 21:48
@laanwj
Copy link
Contributor Author

laanwj commented Feb 5, 2025

Okay, implemented the string search instead.

Another alternative would be to use a blacklist instead of a whitelist. All modern terminals can at least parse basic ANSI 16-color sequences, although some might ignore them for aesthetic reasons.

Exceptions would be TERM empty or non-existent (eg as on Windows), TERM=dumb (no escapes support at all), can't think of any other.

|| term == "screen" || term == "linux" || term == "cygwin"
|| term == "screen-256color" || term == "screen.xterm-256color";
return term == "xterm" || term == "screen" || term == "linux" || term == "cygwin"
|| term.find("-color") != std::string::npos
Copy link

Choose a reason for hiding this comment

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

I'm OK with this choice, though I was expecting just a check for the ending (sadly, ends_with requires c++20, but an equivalent is trivial using compare).

Copy link
Contributor Author

@laanwj laanwj Feb 6, 2025

Choose a reason for hiding this comment

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

Do you think that really matters for the heuristic if it's at the end? What if it's tmux-color-graphics or whatever.

Copy link

Choose a reason for hiding this comment

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

Possible variants of -(256)?color are why I'm OK with the choice. However, when inferring a rule from a pattern, I think it helps to do so as narrowly as possible: This code has existed for a long time, and, in all that time, -color-x has never arisen, whereas -color and -256color have. So, the narrow choice clarifies to the reader (including the compiler, and therefore the machine which runs the code): I know exactly where this should occur in the string. Furthermore, much like browser version strings which "look like" (i.e. match widely used patterns for) prior browsers, it seems positioning color as a suffix has a clear intent behind it, so if someone wanted to add another component to the string, they'd do it in the middle. If I'm wrong, and a color terminal adds -color in the middle, then it'd probably better to deal with that case with real data available, so as to once again suitably narrow the pattern. In fact, the main worry I have about looking for -color anywhere in the string mirrors your question: What if the word color appears mid-string and it isn't a color terminal, because color is part of a larger word? For example, tmux-colorless. Then, you'd have two problems, because someone responding to that bug report would have to alter the algorithm, and not break anything else, which, based on just the code, without knowledge of the history, would involve either an explicit exclusion of the new value, or some other complication of the rule. I considered this scenario a low risk, but it illustrates the broader principle.

@laanwj
Copy link
Contributor Author

laanwj commented Feb 6, 2025

i think i don't care enough about this to keep it open, i thought it'd be useful but it's turning into too much arguing.

@laanwj laanwj closed this Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants