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

Enhance ANSI colors for progress ui #2368

Merged
merged 1 commit into from
Sep 24, 2021
Merged

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Sep 17, 2021

relates to

Attempt to use ANSI colors suitable for most terms:

xterm

image

windows powershell

image

windows cmd

image

windows terminal

image

tabby

image


We could also use another SGR code for blue like cyan which seems ok across major terms:

image

There is also the dichromatic palette for color blind. I guess there is a lot of discussion around this topic, so feel free to give your opinion. And another topic around NO_COLOR that we can discuss in a follow-up.

cc @thaJeztah

Signed-off-by: CrazyMax [email protected]

@tonistiigi
Copy link
Member

tonistiigi commented Sep 19, 2021

I think the problem with this is that it makes the colors absolute. So while these may look good for you, they might not when user is using a specific theme. aec.BlueF doesn't necessarily mean actual blue but an alternative color that should be visible for the current theme. I don't know if this is detectable in any way(or for example if powershell supports themes at all?).

For NO_COLOR, I wonder if aec is willing to implement this in the library level or provide an easily configurable switch.

@crazy-max
Copy link
Member Author

crazy-max commented Sep 20, 2021

I think the problem with this is that it makes the colors absolute. So while these may look good for you, they might not when user is using a specific theme. aec.BlueF doesn't necessarily mean actual blue but an alternative color that should be visible for the current theme. I don't know if this is detectable in any way(or for example if powershell supports themes at all?).

I'm fine using another SGR code like aec.CyanF (36). Majority of loggers use this color:

For NO_COLOR, I wonder if aec is willing to implement this in the library level or provide an easily configurable switch.

Yes should be done upstream pretty much like go-ansi.

@thaJeztah
Copy link
Member

I think the problem with this is that it makes the colors absolute. So while these may look good for you, they might not when user is using a specific theme

Good point as well. It's still odd that (at least on macOS) the default "dark blue" is very hard to read and the "bright" blue may be a bit too bright 😞

@crazy-max
Copy link
Member Author

Using aec.CyanF (36):

xterm

image

windows powershell

image

windows cmd

image

windows terminal (wsl2)

image

tabby

image

Copy link
Member

@thaJeztah thaJeztah 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'm not the biggest fan of cyan, but it's definitely more readable, and out of the available options (looking at the default colours on macOS), looks like a good choice (unless we want to remove color and go for "grey").

Screenshot 2021-09-20 at 13 10 21

I think there's no "perfect" option here, so we'll have to leave it to the user to customise their colours further.

@tonistiigi
Copy link
Member

Leave it cyan for windows only?

@crazy-max
Copy link
Member Author

crazy-max commented Sep 20, 2021

@tonistiigi

Leave it cyan for windows only?

I think we can keep cyan global? I was also wondering if the screenshot in the README was using cyan because INFO[0000] from logrus is cyan and progress ui looks the same?

@thaJeztah
Copy link
Member

Leave it cyan for windows only?

I think it may be equally bad on macOS (default config that is). I'd have to try on a fresh macOS profile (unless someone has one set up)

@tonistiigi
Copy link
Member

I was also wondering if the screenshot in the README was using cyan because

No, this has never been cyan

@tonistiigi
Copy link
Member

@AkihiroSuda @ktock thoughts?

@ktock
Copy link
Collaborator

ktock commented Sep 21, 2021

SGTM about cyan as it at least solves the problem of invisible in some terminals.
But I'm a little bit worry about if this can be less readable on a white background terminal.

@thaJeztah
Copy link
Member

I don't use a white / light background terminal myself; does "green" or "bright green" work better for those? (there was a ticket in the compose repository that mentions compose used "green" before the switch to using buildkit / buildx); docker/compose#8629

@crazy-max
Copy link
Member Author

does "green" or "bright green" work better for those? (there was a ticket in the compose repository that mentions compose used "green" before the switch to using buildkit / buildx); docker/compose#8629

Not sure about green, for me this color indicates a successful/terminated state, not an ongoing one.

@ktock
Copy link
Collaborator

ktock commented Sep 22, 2021

FYI: BuildKit on white background (emacs term mode)

blue

blue

cyan

cyan

seems a bit less readable?

@tonistiigi
Copy link
Member

tonistiigi commented Sep 22, 2021

It seems like some xterms allow getting the background color.

printf "\033]11;?\007" background
printf "\033]10;?\007" foreground

works in iTerm/Terminal for me. Does not work in vscode inline console. No idea about windows + others

update: poc https://gist.github.com/tonistiigi/a0c094d9858912f58b295586e9ec61d1 . blocks on read in vscode atm

@crazy-max
Copy link
Member Author

crazy-max commented Sep 22, 2021

blocks on read in vscode atm

same on xterm-256color, cmd, powershell

@tonistiigi
Copy link
Member

if it doesn't work on windows and powershell doesn't have an alternative then I think trying to detect background is not worth it

@crazy-max
Copy link
Member Author

Found muesli/termenv lib which seems to detect dark background: https://gist.github.com/crazy-max/610dfd4d93a0c8d4c6d539a9f7e08a7a. Could be a candidate to replace aec (also handles NO_COLOR).

@thaJeztah
Copy link
Member

We can shop around a bit to see what good modules exist. My (personal) take on those are that;

  • it possible, we should try to unify a bit in our codebase (there's way too many different approaches used, some self-baked things as well)
  • have a close look at them to make sure they're not overly complex / feature-rich (some of the libraries I found have way, way too many features); also considering that some are using outdated approaches (some of which are now already provided by (e.g. golang.org/x/sys or golang.org/x/term)
  • of course, also looking at what additional dependencies they bring. This one looks to be potentially adding 3 more dependencies https://github.com/muesli/termenv/blob/master/go.mod#L5-L10
  • outweigh pros/cons overall (do we need the functionality, or could we still work around it (if we only need a small bit of the code, perhaps copying some bits is an option)

@tonistiigi
Copy link
Member

@crazy-max looks like for windows it just returns constant true? https://github.com/muesli/termenv/blob/feb87954c13cdde17d5c454b24580fa58df01311/termenv_windows.go#L14 For unix it seems to use same xterm method together with some env detection

@crazy-max
Copy link
Member Author

crazy-max commented Sep 23, 2021

@tonistiigi

looks like for windows it just returns constant true?

Yeah indeed just tested myself. Would need smth like that through Win32API to work for Windows on cmd, powershell (rust impl here). For Windows terminal, maybe in the future: microsoft/terminal#3718

@tonistiigi
Copy link
Member

I think we can start by setting cyan only on windows (seems that it looks better on all defaults there). Then we can see if we can do something smarter for rest and only do cyan on dark background.

@crazy-max
Copy link
Member Author

crazy-max commented Sep 23, 2021

I think we can start by setting cyan only on windows (seems that it looks better on all defaults there). Then we can see if we can do something smarter for rest and only do cyan on dark background.

Done.

package progressui

import "github.com/morikuni/aec"

Copy link
Member

Choose a reason for hiding this comment

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

should these be a const?

Copy link
Member

Choose a reason for hiding this comment

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

arf, commented on the wrong line

@DavidAntliff
Copy link

Nice that it's fixed for Windows.

Then we can see if we can do something smarter for rest and only do cyan on dark background.

Is this tracked somewhere? The dark blue is still practically unreadable on a black terminal in Linux (e.g. rxvt-unicode).

This seems to be a fairly new problem (last few months) - I don't recall it being dark blue last time I ran docker-compose (docker-archive/compose-cli#1947). Is there an environment variable or something I can set so I can actually see the output without losing all colour?

@mcrapts
Copy link

mcrapts commented Jun 4, 2022

Maybe don't use any colors at all? They don't add much anyway, and then it's perfectly readable in all terminals.

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.

6 participants