dandavison / delta

A syntax-highlighting pager for git, diff, grep, and blame output
https://dandavison.github.io/delta/
MIT License
21.36k stars 360 forks source link

Detect Dark/Light Mode from Terminal #1615

Closed bash closed 3 months ago

bash commented 5 months ago

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:

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.

dandavison commented 5 months ago

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 commented 5 months ago

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 https://github.com/sharkdp/bat/issues/1746) so that both projects benefit and reviewing expertise / problem ironing-out is shared? cc @eth-p @sharkdp.

bash commented 5 months ago

Thank you @dandavison for testing my changes :)

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

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:

dandavison commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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). https://github.com/sharkdp/bat/issues/1746 is their issue that your work would seem to solve.

bash commented 4 months ago

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 :)

[^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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

dandavison commented 3 months ago

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 commented 3 months ago

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

dandavison commented 3 months ago

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

ccoVeille commented 3 months ago

thank you everyone