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

Do not use light version of colors by default #21433

Merged
merged 1 commit into from
Apr 19, 2017
Merged

Conversation

musm
Copy link
Contributor

@musm musm commented Apr 18, 2017

Ref #11250 (comment)

It looks like all the underscore light_ colors ended up breaking the julia repl color setup for me on 0.6 on, e.g., the bash subsystem for windows.

@ararslan ararslan requested a review from KristofferC April 18, 2017 18:12
Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

What I was talking about in the issue was only related to the circles in the julia logo. For the normal text we have to think of something different than this because the standard red is too dark on most terminals.

We should figure out why the light colors don't work and maybe we can detect those scenarios and only swap in those cases.

@JeffBezanson
Copy link
Member

Is using bold colors sufficient to make them readable? @musm can we see a screenshot after this change?

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

This PR simply restores the colors back to what it was in 0.5 .

Comparison of 0.6 and 0.5.1. standard terminal colors
image

@KristofferC
Copy link
Member

No, because they used to be bold and bold + standard color = light color in most terminals.

@TotalVerb
Copy link
Contributor

TotalVerb commented Apr 18, 2017

Changing away from bold seems to be a conscious decision taken in #11250. Can't we use 256-color when it's available?

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

not sure what "most" means, but it's clearly not the case here and I suspect once 0.6 is released I won't be the only one posting an issue regarding this.

@KristofferC
Copy link
Member

We could, but AFAIK (I am not an expert on this) it is quite hard to get the terminal to tell you exactly what it supports.

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

Sorry I was mistaken on what it was on 0.5, but clearly that was much better despite the contentious decision regarding the bold choice (which I didn't have a problem, since it rendered the colors properly) than having them show up as in the above screenshots.

@KristofferC
Copy link
Member

We've had this for months and this is the first one to report an issue about it. Is something special with your setup?

@JeffBezanson
Copy link
Member

I'm missing something here --- don't we still use bold for the prompt and error messages? In which case it seems asking for light_ versions of colors is just redundant?

@JeffBezanson
Copy link
Member

Ah, it seems the ERROR: text would be ok, but the message too dark?

@KristofferC
Copy link
Member

I would argue standard red is a bit too dark for what is comfortable:

image

@JeffBezanson
Copy link
Member

I guess the only other option is to bold the entire message, instead of just the prefix.

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

I really don't think these should be light_. Either bold or not bold, but I don't think it makes sense to have changed them to light variants as that screws up the colors, at least in my case.

Light_red is pink on my terminal;
and light_green shows up as a dark gray for some reason..... so that julia> prompt doesn't even show up properly

The julia 0.5 colors worked fine (not sure if I observe bold in the terminal).

My argument is that light_ are rather specialized colors and may not show up properly on all terminals, Where as you can pretty much be guaranteed red, green, etc. is not going to cause problems on any terminal that can show colors.

@KristofferC
Copy link
Member

KristofferC commented Apr 18, 2017

You can get back the old color with i.e. ENV["JULIA_ERROR_COLOR"] = :red. Regarding the prompt, we could change that back to :green. I ask again:

We've had this for months and this is the first one to report an issue about it. Is something special with your setup?

@KristofferC
Copy link
Member

Are you for example using the solarized theme?

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

We've had this for months and this is the first one to report an issue about it. Is something special with your setup?

0.6 hasn't been released yet, so I suspect that is a contributing factor as to why. Nothing special I'm just running things on the linux subsystem for windows.

Are you for example using the solarized theme?

no
but if I do use the solarized theme here is how 0.6 looks:
image

@KristofferC
Copy link
Member

Can you do Pkg.install("Crayons") and run Crayons.test_system_colors()?

@JeffBezanson
Copy link
Member

Worth pointing out that everything in this PR except the error color change should be uncontroversial, since it only involves bold text.

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

Im posting the result under a variety of color scheme to try and illustrate my point
image
image
image
image

Most colors make arbitrary decision on what constitutes as a light variant, e.g. light_red, which is why I think it should also be changed

@TotalVerb
Copy link
Contributor

Yeah, I agree. It's unfortunate but light_x seems to be wildly inconsistent across terminals, so we should avoid using it.

@JeffBezanson
Copy link
Member

The second theme in that list should be the default --- is it not?

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

The second theme in that list should be the default --- is it not?

it is the default theme of the terminal

@KristofferC In your case I think it would be more desirable to tune the color of the red in your terminal, than to keep the current configuration, because it is very inconsistent as to what constitutes as 'light' in different terminal setups. I feel like the default, should be something that works decently for most people, even if it is not ideal for some.

@KristofferC
Copy link
Member

KristofferC commented Apr 18, 2017

I tried myself, and there was no problems with the default colorscheme on Bash subsystem for windows:

image

I feel like the default, should be something that works decently for most people

Doing your change suggested here would make it worse for 99.9% of people (everyone except you that have tried 0.6 so far?).

Yeah, I agree. It's unfortunate but light_x seems to be wildly inconsistent across terminals, so we should avoid using it.

Not really. It is inconsistent in that some (non default) colorschemes happen to set e.g. light_red to mean orange or gray or some other sillyness. If someone sets a colorscheme that sets their colors to be completely non standard, I don't think this is an issue here but with the color scheme.

@TotalVerb
Copy link
Contributor

TotalVerb commented Apr 18, 2017

The problem is that this is a regression from 0.5. People will not expect the prompt to become invisible upon upgrading to 0.6.

I agree that color schemes being nonstandard is the fault of the color scheme; but those color schemes are popular and we should pick colors that aren't horrible on any machine or color scheme.

@KristofferC
Copy link
Member

They won't, unless they actively made the choice to define light_red color to mean invisible color.

@JeffBezanson
Copy link
Member

The prompt should be fine since it's still bold. I believe we can safely remove use of light_ colors with bold.

@KristofferC
Copy link
Member

I believe we can safely remove use of light_ colors with bold.

Yes

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

So is this then ok?

@KristofferC
Copy link
Member

No, the default_error_color is still being changed.

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

Don't' we want to print the whole error msg in bold, since we have agreed that using light is troublesome

@KristofferC
Copy link
Member

Who have agreed with that?

As a reference, we also had problems when writing stuff in bold: #8096. It is impossible to support broken setups because they are broken in different ways.

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

I was referring to :

I believe we can safely remove use of light_ colors with bold.

and earlier

Yeah, I agree. It's unfortunate but light_x seems to be wildly inconsistent across terminals, so we should avoid using it.

@JeffBezanson
Copy link
Member

Where we currently use bold and light we might as well use just bold. That leaves light_red for errors, which we do not have an acceptable replacement for. The only thing I think we could do is put the whole error (prefix plus message) in bold, but for consistency that would also have to apply to warn and info, which would require a wider debate.

@musm
Copy link
Contributor Author

musm commented Apr 18, 2017

I have made the requested change. I still maintain that the usage of light should be avoided for the reasons stated earlier.

@KristofferC KristofferC merged commit ceccddf into JuliaLang:master Apr 19, 2017
@musm musm deleted the color branch April 19, 2017 13:02
rofinn pushed a commit to invenia/julia that referenced this pull request Apr 21, 2017
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.

4 participants