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

Automatically detect whether the terminal background is a light color #28739

Closed
wants to merge 4 commits into from

Conversation

eschnett
Copy link
Contributor

This detects (or tries to) whether the terminal background is a light color. The REPL then chooses a different default color scheme that is easier to read, avoiding e.g. yellow.

This only affects the default colors; any user-chosen colors remain the same.

Closes #28690.

@eschnett eschnett added the REPL Julia's REPL (Read Eval Print Loop) label Aug 18, 2018
@KristofferC
Copy link
Sponsor Member

KristofferC commented Aug 18, 2018

What is the user has changed the way the default colours are shown in the terminal to already fix this issue? Then we will still swap to these non-standard colors.

If the defaults in some terminals make colors unreadable, maybe it is the defaults that need to be changed?

# (We could/should use more mechanisms here)
# Blindly guess an OS-dependent default
if Sys.isapple()
return true
Copy link
Member

Choose a reason for hiding this comment

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

This is a bad assumption. iTerm2 defaults to black. However, iTerm2 does have color query escape codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To my knowledge, iTerm2 sets the COLORFGBG environment variable that we check above, thus this default isn't reached.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I don't think we should ever default this to true solely based on OS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the default based on OS; all OS now use the same default

@eschnett
Copy link
Contributor Author

@KristofferC If the user sets colors, then Julia won't use its default colors. This change only modifies the defaults that Julia uses if there is no customization.

I don't think it is possible to choose colors that work with every background. Light colors are unreadable on light backgrounds, and dark colors are unreadable on dark backgrounds. Plus, Julia wants to use several different colors to improve the output.

@StefanKarpinski
Copy link
Sponsor Member

This seems like a good direction to make the default experience better. But does this light/dark background detection really work? It seems kind of implausible to me that it would actually work.

@eschnett
Copy link
Contributor Author

Appveyor reports an all-green status, but the Github status here on this page doesn't update.

@eschnett eschnett force-pushed the eschnett/light-background branch 2 times, most recently from 1266cc5 to 9815076 Compare November 28, 2018 22:28
Introduce a distinction between light and dark text colors. Use different default colors in both cases.

The text color scheme can be set via a command line option (--color=[light | dark]), or Julia can try to detect the terminal's background color. In this case, it roughly follows vim's approach:
1. Look for COLORFGBG environment variable
2. Use an ANSI escape sequence to query the terminal for the background color
3. Make an educated guess from the terminal name

Tested with iTerm2, Terminal, xterm on MacOS, Linux.
@eschnett
Copy link
Contributor Author

The patch now uses roughly the same mechanism as vim. Querying the terminal via ANSI escape sequences work for iTerm2 and xterm (and probably their derivatives). There are also command line options to override things (--color=[auto | on | off | light | dark]). Ready for review.

@StefanKarpinski
Copy link
Sponsor Member

Bump, who should review?

@eschnett
Copy link
Contributor Author

eschnett commented Jan 3, 2019

I now find that many Julia packages set colours directly (e.g. by using :yellow), and are not going through either environment variables or a colour scheme (as the REPL does). It seems that it would be a huge task to change this, as it would involve changing many packages as well (Pkg, profiler, ..., ?), and maybe even setting up usability coding guidelines. (Or maybe Julia should set the terminal background colour as well, to ensure that everybody is using the same background colour and thus has readable colours.)

A work-around would be to conditionally change the definition of :yellow to be :orange instead, i.e. to change the meaning of colours behind package writers' back. That goes in the same direction as @KristofferC's comment, except that he changes the terminal settings.

In summary, I'm disappointed that my pull request didn't actually solve the problem it was supposed to solve. I don't know whether it's actually useful in practice. If so, then it's only a first step towards making Julia output readable on a light background.

end
end

# Use OSC 11 escape sequence
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

this implementation seems overcomplicated. I think if you leave this out, we can merge the rest, then work on this more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leave out which part – looking at the COLORFGBG variable?

if term.term_type == "linux" || # Linux console
term.term_type == "screen.linux" || # Linux console with screen
startswith(term.term_type, "cygwin") || # Cygwin shell
startswith(term.term_type, "putty") # Putty progrem
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I know this list comes from VIM, but it seems likely ancient. I think most terminals I use report as "xterm" now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a list of terminals which default to using a dark background. The terminals which default to a light background (xterm and friends) are not in this list.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 27, 2020

I think we've somewhat essentially decided not to go with this PR. I think the --color=light / --color=dark options still have merit however, so I'll keep the corresponding issue open and mention that there

@vtjnash vtjnash closed this Oct 27, 2020
@StefanKarpinski
Copy link
Sponsor Member

Probably better as an environment variable since you presumably don't want to have to set it on each invocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default REPL output is difficult to read on light background
5 participants