DF description/missing color is unreadable on dark themes

[fbruetting]

DF description/missing color is unreadable on dark themes

[bkamins]

In summary: we are OK to change this given we know what color to pick. The restriction is that:

1. It has to be one color (as we cannot detect the theme the user uses).
2. The intention is to dim missing a bit (it should not cry-out, rather it should be visually a bit less standing out, as this is the point of having a different color for it)

[bkamins]

Side note: the same issue applies to the column type subheader.

[bkamins]

By the way - can you please confirm how the following renders under the color schemes we discuss? Thank you!

I am asking, because maybe we can use the gray that uses standard Julia in this printout.

[nalimilan]

I agree that Solarized seems broken. Given that it also affects Julia (and even more seriously than DataFrames), I'd prefer to find a more general solution there, e.g. which would allow overriding the colors that correspond to each named color. This should ideally be used by Crayons and we wouldn't have anything to do here.

[bkamins]

Thank you for the comment. Do you have a recommendation then?

CC @ronisbr

[fbruetting]

Thank for the quick reply!

I think one of the colours should be chosen, as neither background/foreground nor black/dark grey/light gray/white seem to work well for both light an dark themes.

This leaves red, green, orange, blue, magenta and cyan as options. The alternate colours originally were intended as brighter tones (light red, light green, yellow, light blue, light purple and teal), but some themes – for whatever reason – abuse them for completely different colours. Solarized Dark for example uses these places in the colour array to store some dark tones. I don’t know why this is the case, but it seems these cannot be used reliably in every case. So if you chose for example yellow, then in some themes that text gets flashy, while in Solarized Dark it gets very dark.

Julia seems to mostly use cyan as accent colour, so maybe this is a good choice? Otherwise orange and blue also seem to be a good fit. I’d reserve red, green and magenta for signaling purposes.

This webpage might be helpful.

Here are some theme examples:

[ronisbr]

Hi!

Fixing this issue would be extremely simple since we just need to change the color here:

https://github.com/JuliaData/DataFrames.jl/blob/8c32d537b1d5a2ae9b1cdd72928def18aeae0bf9/src/abstractdataframe/prettytables.jl#L28C48-L28C79

Personally, I have no idea what would be the best option.

[fbruetting]

The best option, I guess, would be to select light gray for dark themes and dark gray for light themes. But that seems complicated. 😶️

[ronisbr]

Hum, there is no universal way to check if the theme is dark or light AFAIK :(

[ronisbr]

I agree 100% with @bkamins 's proposal here.

[fbruetting]

In summary: we are OK to change this given we know what color to pick. The restriction is that:

1. It has to be one color (as we cannot detect the theme the user uses).

2. The intention is to dim `missing` a bit (it should not cry-out, rather it should be visually a bit less standing out, as this is the point of having a different color for it)

I searched for a bit, but it doesn’t seem possible to tick both boxes. Maybe then hardcoding a medium gray, which is suitable for both theme variants?

[ronisbr]

I searched for a bit, but it doesn’t seem possible to tick both boxes. Maybe then hardcoding a medium gray, which is suitable for both theme variants?

Do you mean, setting a RGB color?

[bkamins]

I think it would make sense to hardcode RGB medium gray (I guess it is possible - right?)

[fbruetting]

In the shell it sure is, I just don’t know how Julia interfaces with that. Shells use strange color codes like \033[31m, as you can see in my link above.

[ronisbr]

The problem is for terminals that do not support 24-bit colors. I need to test to check what will happen.

[ronisbr]

In the shell it sure is, I just don’t know how Julia interfaces with that. Shells use strange color codes like \033[31m, as you can see in my link above.

This code says the terminal must change the foreground color to red. In PrettyTables, we use Crayons.jl that is a conversor between a color and those escape sequences.

Maybe we can assume everyone will be using at least a terminal that supports 256 colors. In this case, we have access to a wider range of colors. IMHO, assume 24-bit color is too much restrictive.

In the case of 256 colors, there are those options:

(from https://www.lihaoyi.com/post/BuildyourownCommandLinewithANSIescapecodes.html)

Thus, we can select a color between 232 and 255.

[fbruetting]

You can also substitute :dark_gray by (127,127,127) in your referenced code. I just tested this.

[ronisbr]

But AFAIK it is converted to a 24-bit color code, which should not work in terminals that do not support it.

[fbruetting]

While we’re at this: Is it possible to specify this color as an environment variable or by a function call? I guess that would be nice for circumventing corner cases due to hardcoded values, for example in the rare case anyone has a grey background.

[ronisbr]

Yes, it should be fine to check if a ENV variable is defined and use this value instead. Maybe it will add an overhead, but should not be noticeable. What do you think @bkamins ? Any suggestions about the variable name?

[bkamins]

In the past we have had a policy not to introduce behavior depending on global state.

@nalimilan - in this case do you think it would be acceptable?

[giordano]

Isn't this the same problem as JuliaLang/julia#38730 caused by the fact the solarized theme uses same colours for background and foreground?

[fbruetting]

Isn't this the same problem as JuliaLang/julia#38730 caused by the fact the solarized theme uses same colours for background and foreground?

Yup, seems so. Thanks for the tip, now I see that my stack traces contain more information than what is visible. 😅️

[nalimilan]

In the past we have had a policy not to introduce behavior depending on global state.

@nalimilan - in this case do you think it would be acceptable?

I would find it kind of weird to have a mechanism to set the color used when printing a particular object (missing). Isn't there a more general theming system that we could use?

Anyway, regarding this particular issue, requiring users to set an environment variable seems suboptimal. Most of them won't even know they can do that. Is there a way to check whether light_grey gives the same color as the background, and switch to dark_grey in that case?

[ronisbr]

Unfortunately no. There is no universal way to check what is the color the terminal will use given a ANSI escape sequence.

[fbruetting]

In the past we have had a policy not to introduce behavior depending on global state.
@nalimilan - in this case do you think it would be acceptable?

I would find it kind of weird to have a mechanism to set the color used when printing a particular object (missing). Isn't there a more general theming system that we could use?

Anyway, regarding this particular issue, requiring users to set an environment variable seems suboptimal. Most of them won't even know they can do that. Is there a way to check whether light_grey gives the same color as the background, and switch to dark_grey in that case?

My proposal was to use medium gray by default and only if that collides with themes (which then are neither light nor dark but rather something in the middle, which I assue to be pretty rare) users are required to manually chose a suitable color for de-highlighting. The problem is, that terminals just define highlighting-colors but no de-highlighting-colors.

[ronisbr]

My proposal was to use medium gray by default and only if that collides with themes (which then are neither light nor dark but rather something in the middle, which I assue to be pretty rare) users are required to manually chose a suitable color for de-highlighting. The problem is, that terminals just define highlighting-colors but no de-highlighting-colors.

Notice that this is only possible if we assume a terminal capable of outputting 256 colors since the default 8 colors only contains bright black as gray. IMHO, I think the number of people using terminals that cannot output 256 colors should be minimal. Thus, it should be fine to select one of those colors:

Assuming 24-bit and selecting the RGB values is way more restrictive.

@fbruetting can you please test in your terminal what would be a good gray selection for those themes? To do so, open a terminal with your theme paste: echo "\e[38;5;239mmissing", changing 239 to the color you are testing.

[fbruetting]

Of course, here it is for Solarized Dark and Light: (btw: echo -e was necessary)

I think 243 provides the best readability for both variants.

[ronisbr]

In the "default" themes, we will have (before on top, after on bottom):

IMHO, the highlight will lose its purpose because it almost indistinguishable from the normal text (which usually is light gray instead of pure white).

[fbruetting]

Not having a good contrast is much less of a problem than not seeing those values at all, so I’d still prefer hardcoding medium gray.

Here’s a comparison of common themes for the colors 240-247 (all of these are shipped with Tilix):

Solarized Light / Linux / Tango

Solarized Dark / Material / Monokai Dark / Orchis / Yaru / Base16 Twilight dark

[fbruetting]

That would lead to invisible text in Solarized Dark, like in the Julia-issue referenced above.

Same order as above:

[nalimilan]

What if we took a relatively dark grey like 238, which is not super readable in Solarized Dark but is at least discernible and better than the current situation on that theme? Anyway it's not hard to find out that these entries correspond to missing values, what matters is that you notice the cell isn't completely empty.

[fbruetting]

Relevant Solarized bug: altercation/solarized#220

When I read this, there are MASSIVE complications and Solarized is to blame because of intentionally violating the specification. It might be better to let everything be as it is and let Solarized be broken, because when you’re fixing the colors for Solarized, all other themes suffer, because DF then hardcodes values in an environment where all other colors are variable.

Best solution would be to indroduce a config setting, which lets Solarized users alter that color if there’s need, then every other theme would work perfectly and there won’t be compromises at all – just a manual fix for an intentional spec violation. Seems the correct approach to me. Solarized just needs to fix the palette, or needs a fork, obviously.

[ronisbr]

Best solution would be to indroduce a config setting, which lets Solarized users alter that color if there’s need, then every other theme would work perfectly and there won’t be compromises at all – just a manual fix for an intentional spec violation. Seems the correct approach to me.

I totally agree. I had problems with solarized themes in the past for a similar reason. Thus, the option is to define a ENV variable that will change the colors in subheaders and in those highlighters.

[fbruetting]

Actually, it would be best if Julia would implement that grey-color ENV/setting (if that’s not already possible) and DF to then refer to that Julia-grey-color-setting. Then we’d have a Julia-global solution.

@nalimilan ? 😄

P.S.: Even better would be if Julia could be able to modify that color and pass it on to all packages, then no package has to implement such a reference, but I don’t know if this would even be possible. Should I open up an issue in Julia for that?

[ronisbr]

I agree that Solarized seems broken. Given that it also affects Julia (and even more seriously than DataFrames), I'd prefer to find a more general solution there, e.g. which would allow overriding the colors that correspond to each named color. This should ideally be used by Crayons and we wouldn't have anything to do here.

I think this is not the correct approach. Crayons.jl uses the escape sequences given the ANSI definitions. It should not modify the colors. The correct way to replace a color is to set the desired one in your terminal, by changing its theme. Of course, each application (DataFrames in this case) can provide a mechanism to tune the colors it uses if necessary. Since there is no definition of a "theme" in DataFrames because we only use few colors by default, I think an ENV to address this specific problem would not be bad.

[fbruetting]

It’s not always possible to modify the color palette. In GNOME Builder you for example can just select a theme but not modify it.

I think we should shrink O(1 + n_pkgs) to O(1) and have a global fix in Crayons then, I created an issue there:

[ronisbr]

I think we should shrink O(1 + n_pkgs) to O(1) and have a global fix in Crayons then, I created an issue there:

No, it will not be global. There are packages, like Term.jl, (and Julia itself) that just output the escape sequences without using Crayons.jl at all. It will not have the intended behavior of a global fix. IMHO, if a theme is setting something that violates the ANSI specification (making a color equal to background when it just should not happen), the theme must be fixed.

Even inside PrettyTables.jl this will not be global. We have a text cell type called AnsiTextCell that allows to have decorated strings inside the tables cells. It does not use Crayons.jl, it parses the escape sequences as per ANSI specification. In this case, we will fix the missing in DataFrames.jl but those other cases will be completely inconsistent.

Allowing Crayons.jl (or anything) to manually change the color can lead to a lot of problems. Example: Package A uses :red to highlight an error. For some reason Package B overloads :red to be black. Package A will print invisible error messages. Hence, it would not be a simple task. It needs to be containerized so that each package has its own theme. This kind of thing requires changing Crayons.jl API or using global variables. In summary: it can be done, but it will not be simple, and I think it is really the wrong move due to an error in a theme.

[ronisbr]

Btw, I was reading about the fixes in all the backlinks to that issue in solarized theme. Almost all involve changing the theme color in the application.

[fbruetting]

Hm, I understood nalimilan so as that the core color definition happens in Crayons and everything else refers to that.

You’re correct that the theme needs a fix, but reality is that this wasn’t fixed in 10 years and yet it’s usage went through the roof – so you can be sure that that won’t happen. There needs a fork to be established, which is a process of at least a decade, so we can’t do anything here, therefore the ugly ENV fix idea.

For fixing the applications it’s the question of which color to put there. Guess that’s arbitrary then. But I also see your point in the logic chain “application ships a broken theme” ⇒ application needs to fix it.

[ronisbr]

Yes, this is precisely my point! However, I also agree that solarized is indeed very popular. Hence, IMHO, the optimal approach (given the circumstances) would be an ENV in DataFrames that at the end is selecting the color for specific types of cells.

[fbruetting]

But that is O(1 + n_pgks) and fixes neither Julia (like stacktraces) nor the same issues in other packages. We should strive for a Julia-global fix.

[ronisbr]

We should strive for a Julia-global fix.

In this case, I do not agree because the only global fix will be fixing the theme, as many others are doing.

If we change the color definition in Crayons.jl, we fix this issue in DataFrames.jl, but the stack traces will continue to be unreadable.

IMHO, changing the color inside the application is like type-piracy. The terminal you are using is responsible to render the dark gray color. The dark gray color is defined by your theme. Hence, we can fix Julia stack traces manually (by changing the color), we can fix Crayons.jl, but any other package that prints raw escape sequences will be wrong.

[nalimilan]

Looking at JuliaLang/julia#38730 I realize that Julia already has a workaround via e.g. Base.text_colors[:light_black] = "\e[38;5;243m". We could use that value instead of :dark_grey (Julia doesn't have that color for some reason), given that by default Base.text_colors[:light_black] == "\e[90m", i.e. it defaults to the value set by the theme.

I'd argue that the best place to do this would be even more upstream, i.e. in Crayons and similar packages, to ensure consistency. But anyway since this has to be set manually it won't affect users that don't want it so it won't make things worse than the current situation.