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

termwiz: support NO_COLOR environment variable #5020

Merged
merged 4 commits into from
May 4, 2024

Conversation

orhun
Copy link
Contributor

@orhun orhun commented Feb 15, 2024

Hey!

I'm one of the maintainers of @ratatui-org and we had one issue where we discussed supporting the NO_COLOR environment variable: ratatui/ratatui#884

We support termwiz as one of our backends and I realized it does not support NO_COLOR environment variable which brought me here.

I'm pretty sure you are familiar with it but here is more information about no color: https://no-color.org

One example use case is:

As a color-blind user, I frequently struggle distinguishing colorized output and appreciate the ability to disable it as per no-color.org

In this PR, I give this a quick stab (based on crossterm's implementation) and you can see this taking effect in our termwiz example:

termwiz no color

I'm happy to make changes in the implementation (and also add tests) - just let me know! 🐻

@wez
Copy link
Owner

wez commented Feb 15, 2024

Hi! Thanks for this!

It's a very targeted implementation and it isn't the way I would have thought to do it, which I'll describe below. What I had in mind when I read the summary of this PR is definitely more work than your proposed implementation, but I think it is probably better in the long term for a couple of reasons:

  • Changing the nature of a type conversion based on an environment variable makes it difficult to write unit tests for that type conversion
  • The type conversion is an implicit solution rather than an explicit solution, which means that as functionality evolves in the future it may not consider the color level and may do the wrong thing.

termwiz has a Capabilities concept that is intended to communicate a combination of terminal capabilities and user preferences down to the underlying rendering layer.

In terms of color, there is a color_level concept that is probably a bit under-used today:

impl ProbeHints {
pub fn new_from_env() -> Self {
ProbeHints::default()
.term(var("TERM").ok())
.colorterm(var("COLORTERM").ok())

so I would imagine that the implementation of this feature might be:

  • Add ColorLevel::MonoChrome variant to represent this mode
  • In ProbeHints::new_from_env, if NO_COLOR=1 in the environment, override the color level to MonoChrome
  • Apply the effects of MonoChrome in the renderer implementations, which I think means to generally ignore/skip the actual color changing portions of the attributes:

fn flush_pending_attr<W: RenderTty + Write>(&mut self, out: &mut W) -> Result<()> {

let fg = match attr.foreground() {
ColorAttribute::TrueColorWithDefaultFallback(_) | ColorAttribute::Default => {
FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN
}
ColorAttribute::TrueColorWithPaletteFallback(_, idx)
| ColorAttribute::PaletteIndex(idx) => ansi_colors!(
idx,
White,
FOREGROUND_RED,
FOREGROUND_GREEN,
FOREGROUND_BLUE,
FOREGROUND_INTENSITY
),
};
let bg = match attr.background() {

  • I don't recall if there are other edges to the API surface where it might be important to handle this stuff; so it's possible that there might need to be something of a bit of a game of whackamole to run them all down. This is one area where this approach is more of a painful slog than your proposed solution.

What do you think of this?

@orhun
Copy link
Contributor Author

orhun commented Feb 25, 2024

Hey @wez, thank you for your detailed response (and sorry for the late reply!)

The proposed solution makes sense to me and definitely sounds better for the long term. I will update the implementation based on this and let you know if there are any rough edges.

@orhun
Copy link
Contributor Author

orhun commented May 3, 2024

Hey again @wez I made the suggested changes and it works as expected on Linux. I didn't have the chance to test Windows though.

Let me know!

@orhun orhun force-pushed the termwiz/support_no_color branch from ff1d479 to ceeea5d Compare May 3, 2024 20:11
@wez wez merged commit 1375e79 into wez:main May 4, 2024
4 of 18 checks passed
@wez
Copy link
Owner

wez commented May 4, 2024

LGTM, thanks!

wez added a commit that referenced this pull request May 5, 2024
saep pushed a commit to saep/wezterm that referenced this pull request Jul 14, 2024
* termwiz: support NO_COLOR environment variable

* style: update formatting

* refactor: use capabilities for enabling no-color
saep pushed a commit to saep/wezterm that referenced this pull request Jul 14, 2024
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