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

anstyle-query's term_supports_ansi_color isn't quite right on Windows #210

Closed
sunshowers opened this issue Aug 30, 2024 · 5 comments
Closed

Comments

@sunshowers
Copy link

Hey!

I was checking out anstyle-query and saw this bit of code:

// If TERM isn't set, then we are in a weird environment that
// probably doesn't support ansi.
None => return false,

I checked both conhost.exe and Windows Terminal and neither of them set TERM. But they do support ANSI colors on all versions of Windows 10 after 1511, so returning false isn't quite right.

I guess this makes sense for anstream, which can fall back to Windows console library calls, but if some other crate decides to use anstyle-query and only supports ANSI colors then they're going to get a misleading answer sadly.

@epage
Copy link
Collaborator

epage commented Aug 30, 2024

From how I use that check, I think we have different expectations for it. I treat term_supports_ansi_color as "does this terminal natively support ANSI color codes" and require people to separately check enable_ansi_colors for "can I change this terminal to support ansi color codes".

#[cfg(feature = "auto")]
let use_wincon = raw.is_terminal()
&& !anstyle_query::windows::enable_ansi_colors().unwrap_or(true)
&& !anstyle_query::term_supports_ansi_color();
#[cfg(not(feature = "auto"))]
let use_wincon = true;
if use_wincon {
Self::wincon(raw).unwrap_or_else(|raw| Self::always_ansi_(raw))
} else {
Self::always_ansi_(raw)
}

@sunshowers
Copy link
Author

Oh I see! That makes sense then. But yeah it definitely didn't match my expectations of how that function works.

@sunshowers
Copy link
Author

I guess it might be worth updating the comment because it's not actually strange that TERM isn't set on Windows — only things like mintty set it.

@epage
Copy link
Collaborator

epage commented Sep 3, 2024

Wow, looks like I butchered termcolors code, applying comments in cases they didn't apply. Made improvements in #212.

@sunshowers
Copy link
Author

All right, I think this looks good. Thanks!

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

No branches or pull requests

2 participants