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

Default to ANSI colors even if truecolor is available? #176

Closed
Benjamin-L opened this issue May 18, 2022 · 3 comments · Fixed by #177
Closed

Default to ANSI colors even if truecolor is available? #176

Benjamin-L opened this issue May 18, 2022 · 3 comments · Fixed by #177
Labels
breaking A semver-major breaking change enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Benjamin-L
Copy link
Contributor

Currently the default color scheme from GraphicalTheme::default will prefer truecolor to ansi colors, if available. Many users have terminals with a specific color scheme configured for the ANSI colors, and CLI tools that synthesize their own color scheme with arbitrary RGB values tend to look very out of place. This is particularly bad with a non-black background, which can result in text with unreadably low contrast. My personal preference would be for truecolor to only be used by default in situations where the exact color value matters rather than the semantic label of the color. For example, truecolor-by-default makes sense for a CLI image viewer or a tool to visualize color hex codes. Error diagnostics fall pretty cleanly into the "exact color values don't matter but semantics do" group.

This is, however, a subjective thing, and no hard feelings if the miette maintainers don't agree and want to keep the current behavior. I've set up FORCECOLOR=1 on my machine, so things look fine for me either way.

@zkat
Copy link
Owner

zkat commented May 19, 2022

You're not the first person to bring this up, but for some reason I'm having trouble finding the last discussion about this. But in the end, I think you're right. The downside is just that screenshots won't look as pretty by default but that's more marketing than anything else haha.

Unfortunately, it's a breaking change, but I guess integers are cheap, yeah? :)

If you're interested, I'd accept a PR that changes the default to be ansi-based colors, as long as a bit of work is put into making sure the default colors "feel" ok on at least light and dark with default color settings.

@zkat zkat added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers breaking A semver-major breaking change labels May 19, 2022
@Benjamin-L
Copy link
Contributor Author

I should have time to put together a PR this weekend :)

@zkat zkat closed this as completed in #177 Jun 24, 2022
zkat pushed a commit that referenced this issue Jun 24, 2022
Fixes: #176

Change the default ansi color theme to use colors that are more similar
to the colors from the default RGB theme. In particular, don't use red
for any of the span labels, since that color is also used for errors.

BREAKING CHANGES:
* the default theme now prefers ANSI colors, even if RGB is supported
* `MietteHandlerOpts::ansi_colors` is removed
* `MietteHandlerOpts::rgb_color` now takes an enum that controls the
  color format used when color support is enabled, and has no effect
  otherwise.
@sunshowers
Copy link
Contributor

Just wanted to say thanks for doing this @Benjamin-L! I did indeed raise this point in #150 as well but never had the time to follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A semver-major breaking change enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants