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

DF description/missing color is unreadable on dark themes #3400

Open
fbruetting opened this issue Nov 8, 2023 · 44 comments
Open

DF description/missing color is unreadable on dark themes #3400

fbruetting opened this issue Nov 8, 2023 · 44 comments
Labels
Milestone

Comments

@fbruetting
Copy link

fbruetting commented Nov 8, 2023

In the picture below you see a DataFrame in combination with following themes:

  • Solarized Dark
  • Solarized Dark (with color “alternate black” manually changed to grey)
  • Solarized Light

Bildschirmfoto vom 2023-11-08 17-54-58

Solarized Dark is one of the most popular dark themes, because of its very good readability as its contrast being neither too low nor too high. Without modifications, missings and column types are not readable at all.

I found out that this is due to DataFrames using the “alternate black” color – which on dark themes usually is equal to the background color, or at least very near. So it’s not a problem at all on light themes, but all dark themes have problems with this (not just Solarized). See the following examples with the themes:

  • Adwaita Dark
  • Builder Dark (GNOME Builder)
  • Classic Dark
  • Cobalt
  • Kate Dark
  • Peninsula Dark
  • Solarized Dark
  • Oblivion
  • Arctic Dark

I haven’t encountered these issues anywhere else, neither in my IDE (GNOME Builder), nor in general Julia, so I guess the problem’s root is this package’s color selection. I guess it’s necessary to chose another color here, as “black” just works well one bright themes.

Bildschirmfoto vom 2023-11-08 17-59-02

Bildschirmfoto vom 2023-11-08 17-59-05

Bildschirmfoto vom 2023-11-08 17-59-07

Bildschirmfoto vom 2023-11-08 17-59-09

Bildschirmfoto vom 2023-11-08 17-59-11

Bildschirmfoto vom 2023-11-08 17-59-12

Bildschirmfoto vom 2023-11-08 17-59-14

Bildschirmfoto vom 2023-11-08 17-59-16

Bildschirmfoto vom 2023-11-08 17-59-18

@bkamins bkamins added the display label Nov 8, 2023
@bkamins bkamins added this to the patch milestone Nov 8, 2023
@bkamins
Copy link
Member

bkamins commented Nov 8, 2023

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

CC @ronisbr

@fbruetting
Copy link
Author

fbruetting commented Nov 8, 2023

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:

Bildschirmfoto vom 2023-11-08 20-00-04
Bildschirmfoto vom 2023-11-08 20-00-19
Bildschirmfoto vom 2023-11-08 20-00-27
Bildschirmfoto vom 2023-11-08 20-00-31
Bildschirmfoto vom 2023-11-08 20-00-36
Bildschirmfoto vom 2023-11-08 20-00-43
Bildschirmfoto vom 2023-11-08 20-00-48

@ronisbr
Copy link
Member

ronisbr commented Nov 8, 2023

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
Copy link
Author

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

@bkamins
Copy link
Member

bkamins commented Nov 8, 2023

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)

@ronisbr
Copy link
Member

ronisbr commented Nov 8, 2023

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

@ronisbr
Copy link
Member

ronisbr commented Nov 8, 2023

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

@fbruetting
Copy link
Author

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
Copy link
Member

ronisbr commented Nov 8, 2023

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
Copy link
Member

bkamins commented Nov 8, 2023

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

@fbruetting
Copy link
Author

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
Copy link
Member

ronisbr commented Nov 8, 2023

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

@ronisbr
Copy link
Member

ronisbr commented Nov 8, 2023

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:

IMG_4384

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

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

@fbruetting
Copy link
Author

fbruetting commented Nov 8, 2023

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

@ronisbr
Copy link
Member

ronisbr commented Nov 8, 2023

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

@fbruetting
Copy link
Author

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
Copy link
Member

ronisbr commented Nov 9, 2023

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
Copy link
Member

bkamins commented Nov 9, 2023

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
Copy link

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

ronisbr commented Nov 12, 2023

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

@fbruetting
Copy link
Author

fbruetting commented Nov 13, 2023

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
Copy link
Member

ronisbr commented Nov 13, 2023

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:

Captura de Tela 2023-11-13 às 09 33 28

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
Copy link
Author

fbruetting commented Nov 13, 2023

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.

Bildschirmfoto vom 2023-11-13 13-47-47Bildschirmfoto vom 2023-11-13 13-48-18

@ronisbr
Copy link
Member

ronisbr commented Nov 13, 2023

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

Captura de Tela 2023-11-13 às 10 12 41

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

@bkamins
Copy link
Member

bkamins commented Nov 13, 2023

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

@fbruetting
Copy link
Author

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

l02 - Solarized Light l03 - Linux l07 - Tango

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

d01 - Solarized Dark d04 - Material d05 - Monokai Dark d06 - Orchis d08 - Yaru d09 - Base16 Twilight dark

@bkamins
Copy link
Member

bkamins commented Nov 13, 2023

By the way - can you please confirm how the following renders under the color schemes we discuss? Thank you!
image
I am asking, because maybe we can use the gray that uses standard Julia in this printout.

@fbruetting
Copy link
Author

fbruetting commented Nov 13, 2023

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

Same order as above:

Bildschirmfoto vom 2023-11-13 16-49-02
Bildschirmfoto vom 2023-11-13 16-49-09
Bildschirmfoto vom 2023-11-13 16-49-14
Bildschirmfoto vom 2023-11-13 16-49-21
Bildschirmfoto vom 2023-11-13 16-49-26
Bildschirmfoto vom 2023-11-13 16-49-33
Bildschirmfoto vom 2023-11-13 16-49-39
Bildschirmfoto vom 2023-11-13 16-49-49
Bildschirmfoto vom 2023-11-13 16-49-54

@nalimilan
Copy link
Member

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
Copy link
Author

fbruetting commented Nov 13, 2023

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
Copy link
Member

ronisbr commented Nov 13, 2023

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
Copy link
Author

fbruetting commented Nov 13, 2023

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?

@nalimilan
Copy link
Member

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.

@ronisbr
Copy link
Member

ronisbr commented Nov 13, 2023

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
Copy link
Author

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
Copy link
Member

ronisbr commented Nov 13, 2023

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
Copy link
Member

ronisbr commented Nov 13, 2023

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
Copy link
Author

fbruetting commented Nov 13, 2023

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
Copy link
Member

ronisbr commented Nov 13, 2023

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
Copy link
Author

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
Copy link
Member

ronisbr commented Nov 13, 2023

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants