wezterm icon indicating copy to clipboard operation
wezterm copied to clipboard

termwiz: support NO_COLOR environment variable

Open orhun opened this issue 1 year ago • 1 comments

Hey!

I'm one of the maintainers of @ratatui-org and we had one issue where we discussed supporting the NO_COLOR environment variable: https://github.com/ratatui-org/ratatui/issues/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! :bear:

orhun avatar Feb 15 '24 17:02 orhun

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:

  • https://docs.rs/termwiz/latest/termwiz/caps/struct.Capabilities.html#method.color_level reports the supported color level
  • https://docs.rs/termwiz/latest/termwiz/caps/struct.ProbeHints.html#method.color_level is a way for an application to override this
  • the default values for ProbeHints are taken from the environment:

https://github.com/wez/wezterm/blob/22f9f8d288521508bd4cdc924d9f209d78b36483/termwiz/src/caps/mod.rs#L122-L126

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:

https://github.com/wez/wezterm/blob/22f9f8d288521508bd4cdc924d9f209d78b36483/termwiz/src/render/terminfo.rs#L52

https://github.com/wez/wezterm/blob/22f9f8d288521508bd4cdc924d9f209d78b36483/termwiz/src/render/windows.rs#L72-L88

  • 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?

wez avatar Feb 15 '24 19:02 wez

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 avatar Feb 25 '24 13:02 orhun

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 avatar May 03 '24 20:05 orhun

LGTM, thanks!

wez avatar May 04 '24 23:05 wez