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

Improve color detection across platforms #885

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heaths
Copy link

@heaths heaths commented May 4, 2024

Fixes #882 by improving style::available_color_count():

  • Checks ANSI support on Windows.
  • Uses the COLORTERM environment variable and falls back to the TERM environment variable.
  • Supports "xterm-24bit" and "truecolor" values which return u16::MAX.

@heaths heaths force-pushed the issue882 branch 2 times, most recently from 7bbdbd9 to b8ae856 Compare May 4, 2024 16:45
@heaths
Copy link
Author

heaths commented May 4, 2024

Contains fix for #883 to unblock testing. Tested in Windows Terminal with bash (WSL) and pwsh.exe (WT uses conpty) and cmd.exe (which uses conhost).

@heaths heaths marked this pull request as ready for review May 4, 2024 16:47
@heaths heaths requested a review from TimonPost as a code owner May 4, 2024 16:47
@TimonPost
Copy link
Member

TimonPost commented May 5, 2024

can you rebase now main is stable

Copy link
Contributor

@joshka joshka left a comment

Choose a reason for hiding this comment

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

LGTM

I'd suggest adding a small amount of details to the commit message body instead of just linking to the issue as this makes it easier for people to review what's changed in a release.

Something like:

`style::available_color_count()` now:
- checks support_ansi on windows
- uses COLORTERM environment variable with fallback to TERM
- supports 24bit / truecolor (reported as u16::MAX)

Would it be worthwhile to add unit tests for this? Perhaps using https://crates.io/crates/temp-env?

src/style.rs Outdated Show resolved Hide resolved
Fixes crossterm-rs#882 by improving `style::available_color_count()`:
- Checks ANSI support on Windows.
- Uses the COLORTERM environment variable and falls back to the TERM environment variable.
- Supports "xterm-24bit" and "truecolor" values which return `u16::MAX`.
@joshka
Copy link
Contributor

joshka commented May 7, 2024

LGTM

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.

available_color_count is wrong on Windows
3 participants