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

ShouldColorize::from_env() does not look the TERM environment variable #108

Open
fhars opened this issue Apr 4, 2022 · 5 comments
Open
Labels
feature-request Feature Request - Let's have some new things!

Comments

@fhars
Copy link

fhars commented Apr 4, 2022

ShouldColorize::from_env() completly ignores wether it runs in an environment that is capable of displaying colors or not and always generates color control codes.

Steps to reproduce:

  1. Write a test program that uses colored.
  2. Start the program with TERM=dumb ./my-test-program

Expected behavior:

  1. The output in uncolored.

Observed behavior:

  1. The output is colored.
@fhars
Copy link
Author

fhars commented Apr 4, 2022

This patch fixes it for me, no idea what it does on windows: 0001-query-terminfo-color-capabilities.txt

(And how stupid is it that github does not allow to attach patches to issues?)

@Horgix
Copy link
Collaborator

Horgix commented Apr 4, 2022

(And how stupid is it that github does not allow to attach patches to issues?)

To be fair, if you embed it inside a code block with the patch syntax it correctly displays it:

From dfc8e146672fcae0a1d0a918dfb831d92392ebbb Mon Sep 17 00:00:00 2001
From: Florian Hars <florian@hars.de>
Date: Mon, 4 Apr 2022 16:43:51 +0000
Subject: [PATCH] query terminfo color capabilities

---
 Cargo.toml     |  3 ++-
 src/control.rs | 11 +++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 886b29e..6e8d81f 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "colored"
 description = "The most simple way to add colors in your terminal"
-version = "2.0.0"
+version = "2.0.1"
 authors = ["Thomas Wickham <mackwic@gmail.com>"]
 license = "MPL-2.0"
 homepage = "https://github.com/mackwic/colored"
@@ -16,6 +16,7 @@ no-color = []
 [dependencies]
 atty = "0.2"
 lazy_static = "1"
+terminfo = "0.7"
 
 [target.'cfg(windows)'.dependencies.winapi]
 version = "0.3"
diff --git a/src/control.rs b/src/control.rs
index 7ad6e62..0aae563 100644
--- a/src/control.rs
+++ b/src/control.rs
@@ -104,8 +104,15 @@ impl ShouldColorize {
     /// followed by `CLICOLOR` combined with tty check.
     pub fn from_env() -> Self {
         ShouldColorize {
-            clicolor: ShouldColorize::normalize_env(env::var("CLICOLOR")).unwrap_or_else(|| true)
-                && atty::is(atty::Stream::Stdout),
+            clicolor: (ShouldColorize::normalize_env(env::var("CLICOLOR")).unwrap_or(true))
+                && atty::is(atty::Stream::Stdout)
+                && (if let Ok(db) = terminfo::Database::from_env() {
+                    db.get::<terminfo::capability::MaxColors>()
+                        .map(|mc| 2 < mc.into())
+                        .unwrap_or(false)
+                } else {
+                    true
+                }),
             clicolor_force: ShouldColorize::resolve_clicolor_force(
                 env::var("NO_COLOR"),
                 env::var("CLICOLOR_FORCE"),
-- 
2.25.1

As for the purpose itself, I'm not the best to answer it - so far I always made sure to define the "standard" NO_COLOR environment variable in restrained-output environments when using this crate

@fhars
Copy link
Author

fhars commented Apr 4, 2022

Emacs compilation mode doesn't, it allocates a pty and sets TERM=dumb, which has been the standard way to do this since 1978.

fhars added a commit to fhars/probe-run that referenced this issue Apr 6, 2022
The current version of the colored crate ignores the capabilities of
the terminal (see colored-rs/colored#108),
which makes the outupt of probe-run unparseable in e.g. emacs
compilation mode. As a temporary workaround, this patch overrides
colorization if probe-run is called from a terminal without color
support.
fhars added a commit to fhars/probe-run that referenced this issue Apr 6, 2022
The current version of the colored crate ignores the capabilities of
the terminal (see colored-rs/colored#108),
which makes the outupt of probe-run unparseable in e.g. emacs
compilation mode. As a temporary workaround, this patch overrides
colorization if probe-run is called from a terminal without color
support.
bors bot added a commit to knurling-rs/probe-run that referenced this issue May 9, 2022
320: Disable terminal colorization if `TERM=dumb` is set r=jonas-schievink a=Urhengulas

This PR fixes the problem `@fhars` described in #318:
> The current version of the colored crate ignores the capabilities of the terminal (see colored-rs/colored#108), which makes the outupt of probe-run unparseable in e.g. emacs compilation mode.

It also supersedes #318 as a fix to the problem. The reason for not going with #318 is that it adds the `terminfo` crate as a dependency which seems unmaintained and with many outdated dependencies.

Co-authored-by: Urhengulas <johann.hemmann@code.berlin>
@mackwic mackwic changed the title ShouldColorize::from_env() ignores the environment ShouldColorize::from_env() does not look the TERM environment variable Jun 30, 2022
@mackwic mackwic added the feature-request Feature Request - Let's have some new things! label Jun 30, 2022
@mackwic
Copy link
Collaborator

mackwic commented Jun 30, 2022

Hi @fhars thank you for the feature request.

It seems that you're not familiar with Github, keep in mind that everyone here is a benevolent contributor, please be nice and respectful of the others.

@kalvdans
Copy link

I've made a merge request from @fhars patch where we can discuss the code or extra requirements further, see #129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Feature Request - Let's have some new things!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants