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

Detect Dark/Light Mode from Terminal #1615

Merged
merged 7 commits into from Mar 12, 2024

Conversation

bash
Copy link
Contributor

@bash bash commented Jan 23, 2024

Use the terminal-colorsaurus crate to detect light/dark mode if neither --light nor --dark is specified.

I was unsatisfied with existing crates (such as termbg) and so I went down the rabbit hole of making my own crate for this. It uses OSC 10 / OSC 11 on Unix and winapi calls on Windows. Edit: I have since removed Windows support for the reasons outlined here

My crate has a few advantages over existing crates that do this:

  • Compares the perceived lightness of background and foreground color (which means it correctly detects the theme in low constrast cases).
  • Works if stdin, stdout and stderr are redirected.
  • Safely restores the terminal from raw mode even if the library errors.
  • No dependency on an async runtime.

To avoid race conditions when piped into a pager as mentioned in a comment in this previous attempt color detection is disabled when stdout is attached to a pipe.

@bash bash changed the title Try To Detect Dark/Light Mode from Terminal Detect Dark/Light Mode from Terminal Jan 23, 2024
@dandavison
Copy link
Owner

Hi @bash, this looks really interesting!

I don't think I quite have it working in my set up. It is correctly detecting my light-background terminal (i.e. I have removed all env vars and git config settings that would influence it, and on your branch it is correctly using GitHub theme for a light background whereas on master it's using the dark theme despite my terminal being light). However, when I comment out my alacritty color settings so that my terminal has alacritty's default dark background, it continues to use the GitHub theme. Sorry if I've messed up my testing which is possible.

Do you have any concerns about added startup latency? (Do you have any timings?) This is the main/only thing I want us to be sure about regarding this work. Obviously delta ideally wants to always have imperceptible start up time, even on not so high-end machines.

As a tangential comment / for follow up, I think that delta does a few disk I/O things synchronously at start up. I wonder whether there would be latency wins in doing more things concurrently.

I haven't followed the technical details of terminal color detection closely. I did look into it at one point and couldn't quite see / was confused about how delta could do it given that it was accepting stdin from git and outputting stdout to less. In practice, does your comment

Only query the terminal if we're not piped to a pager

point to any meaningful limitations? To start with the basic/dumb question, I believe it still works even when delta's output is going to less, right? Can you give me a little bit of hand-holding regarding how this is working at a technical level? Is the point that you're writing to / reading from the tty before delta has started redirecting its output to the pager?

@dandavison
Copy link
Owner

dandavison commented Feb 3, 2024

If it's the case that terminal-colorsaurus is solving limitations of existing crates (e.g. termbg) such that color detection for delta/bat etc is feasible now when it was not before, would you consider opening a PR against bat also (would fix sharkdp/bat#1746) so that both projects benefit and reviewing expertise / problem ironing-out is shared? cc @eth-p @sharkdp.

@bash
Copy link
Contributor Author

bash commented Feb 8, 2024

Thank you @dandavison for testing my changes :)

Since opening this PR, I did some more testing and decided to change a few things:

  • I completely removed Windows support from terminal-colorsaurus as the state of detecting the terminal's color on Windows is quite broken.
  • Similar to how there's often a --color=auto/never/always flag I thought it was a good idea to include a similar flag for the dark/light detection for cases when the heuristic is wrong.
  • I noticed during my daily driving that my heuristic (output not redirected) is wrong in the case of git add --patch. I decided to piggy-back on the --color-only flag that is usually set in that case. (Is this a good idea?)

Prompted by your comment, I decided to collect some measurements regarding terminal latency.
As you can see, there is a wide range with some terminals below 100 µs and some that usually take up to 30 ms.

Can you give me a little bit of hand-holding regarding how this is working at a technical level?

I will happily try to do that. Please note that I have learned a lot of these things over the past few weeks by reading man pages and forum answers, so take everything I say with a grain of salt :)

Querying the terminal for its colors involves communicating with the controlling terminal. This is a bidirectional stream that can be acquired by opening /dev/tty.
Running a command from a shell without redirecting anything means stdin, stdout and stderr all point to this terminal.

I think this is where the confusion might come from: In the case where all standard streams point to the terminal, one can write to stdout and read the terminal's response from stdin.
However, that's not the goal: We want to read/write to the terminal. In this case stdin/stdout just happen to point to the terminal.

Where does the race with the pager come from then? Usually, delta is the one to start the pager after processing its settings / querying the terminal for its colors. In that case we don't get a race.

The race occurs when the user manually pipes delta's output to a pager, e.g. by running git diff | delta | less.
In this case, both delta and less run simultaneously and there are two ways to cause a race:

  • less reads from the terminal to detect if the user has pressed q to exit. So any response that the terminal tries to send to delta might get partially consumed by less instead!
  • Both less and terminal-colorsaurus enable / disable raw mode on the terminal. terminal-colorsaurus might disable raw mode while less is still running and depending on it being enabled.

@dandavison
Copy link
Owner

Hi @bash, thanks a lot for the explanation (and sorry for the high-latency conversation). I'm still finding this:

I don't think I quite have it working in my set up. It is correctly detecting my light-background terminal (i.e. I have removed all env vars and git config settings that would influence it, and on your branch it is correctly using GitHub theme for a light background whereas on master it's using the dark theme despite my terminal being light). However, when I comment out my alacritty color settings so that my terminal has alacritty's default dark background, it continues to use the GitHub theme.

(Would you mind rebasing / merging main?)

@bash
Copy link
Contributor Author

bash commented Mar 7, 2024

Hi @dandavison

Sure thing, I have rebased the PR :)

Regarding your setup: Are you using tmux by any chance? In that case tmux is the one to answer the color query. tmux in turn queries the terminal but iirc caches the result. So if the terminal changes its colors during runtime tmux will unfortunately not know about that.

@dandavison
Copy link
Owner

Ah-ha, that's right, I'm using tmux. Do you think they might be amenable to a PR allowing that caching to be optionally disabled? tmux isn't released often but I assume lots of people (like me) install it from source -- we have to for hyperlink support currently -- so having a patch to enable your terminal color detection would very useful. Also, I think the bat project would benefit from your work just as much as delta (and their architecture w.r.t. the interaction between the Rust app and the pager etc is essentially identical to delta's because delta copied what they do). sharkdp/bat#1746 is their issue that your work would seem to solve.

src/options/theme.rs Outdated Show resolved Hide resolved
src/cli.rs Show resolved Hide resolved
@bash
Copy link
Contributor Author

bash commented Mar 8, 2024

Do you think they might be amenable to a PR allowing that caching to be optionally disabled?

I think they already react to color changes, but the only terminal that I know of with a mechanism to notify processes of color changes is iTerm 2 1. Here's the tmux issue for reference.

Also, I think the bat project would benefit from your work just as much as delta [...]

Thank you for the encouragement, I think I'll cook up a PR for bat then :)

Footnotes

  1. A few weeks ago I submitted a patch to vte (the terminal library that GNOME's terminal(s) uses) that adds support for the same mechanism that iTerm 2 uses. They didn't seem happy about the specific mechanism used and I haven't had time yet to consider the alternatives. I have no idea what the stance of other terminal implementors on this.

@dandavison
Copy link
Owner

dandavison commented Mar 12, 2024

Hi @bash, I see in your crate the code comment says

Windows is [not supported][windows_unsupported].

and above you wrote

It uses OSC 10 / OSC 11 on Unix and winapi calls on Windows.

How will this PR behave for people using delta on Windows?

@bash
Copy link
Contributor Author

bash commented Mar 12, 2024

Oh, that's a mistake 👀 I decided to remove windows support at some point because the winapi calls unfortunately report incorrect colors for most users (details here).

I have updated the original PR description with a remark.

Colorsaurus returns Err(Unsupported) on Windows which means that delta will fall back to the default (dark mode).

@bash
Copy link
Contributor Author

bash commented Mar 12, 2024

@dandavison I see that the tests are failing on Windows, was this already the case before or did I break them?

@dandavison dandavison merged commit 2f76c56 into dandavison:main Mar 12, 2024
11 of 12 checks passed
@dandavison
Copy link
Owner

I've merged this, thanks a lot @bash for the work! (including all the clear explanations and code comments).

(The Windows test failure isn't your fault; there's a race condition in some tests that access env vars but are running concurrently that hasn't been fixed yet)

@aykevl
Copy link

aykevl commented Mar 12, 2024

Did a quick check in Konsole and it appears to be working for me.

@dandavison
Copy link
Owner

Nice, thanks @aykevl -- it's really useful to have some extra confirmation on different setups for this sort of feature.

@ccoVeille
Copy link

thank you everyone

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.

None yet

4 participants