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

Choose Theme Based on The Terminal's Color Scheme #2896

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

bash
Copy link

@bash bash commented Mar 15, 2024

Resolves #1746

This PR is an alternative to #2797. The most notable difference is that this PR uses my own library terminal-colorsaurus instead of termbg.
Among other things, this solves (or at least softens) concerns around timeout / latency.

For additional background, see also my PR to delta: dandavison/delta#1615

Usage

As suggested in #1746, there are two new options with accompanying environment variables:

  • --theme-dark / BAT_THEME_DARK
  • --theme-light / BAT_THEME_LIGHT

By default, bat detects if the terminal is dark or light and uses --theme-<mode> or falls back to the default dark or light theme respectively.

If the detection fails, the terminal is treated as having a dark background.

The --theme option and BAT_THEME env var always overrides these new options for backwards compatibility and to have an easy way to disable color detection. As a consequence --theme=default always uses the default dark theme.

I also decided to add an option for controlling the color detection --detect-color-scheme. I don't think this is strictly necessary and I'm leaning on removing this. The only value that is not achievable through a combination of other options is --detect-color-scheme=always.

Detection can be controlled via --color-scheme:

  • auto - detect from terminal
  • auto:always - always detect from terminal, even if output is redirected
  • dark
  • light
  • system - detect from OS (macOS only)

--theme / BAT_THEME takes some special values to control detection:

  • auto (default) - detect from terminal
  • auto:always - always detect from terminal, even if output is redirected
  • auto:system - detect from OS (macOS only)
  • dark - Always use the theme configured in --theme-dark
  • light - Always use the theme configured in --theme-light

I thought it might be useful to include dark and light as special values as a user may have configured dark and light themes via the env var / config but wants to override.

Latency and Terminal Support

terminal-colorsaurus includes Measurements for a couple of terminals, both terminals that support OSC 10/OSC 11 and some that don't.

For terminals that don't support OSC 10/OSC 11 querying for the color scheme usually takes well below 100 µs.

How does this work?

Colorsaurus sends two escape sequences: OSC 11 (the actual color querying sequence) followed by DA1 (which is supported by almost all terminals out there). Since terminals answer in the same order as the sequences were received we know that if we receive the answer to DA1 then the terminal does not support OSC 11 and can bail out early and avoid a long timeout.

For terminals that support OSC 10/OSC 11 there's a wide range of latency: Some terminals take less than 100 µs while others usually take around 30 ms.

Windows

Windows is not supported. Calling terminal-colorsaurus on windows is a no-op, so there's no added latency there.

macOS

I have completely removed the current detection via macOS system preferences as terminal-colorsaurus is correctly able to detect whether the terminal is dark or light. I saw dalance/termbg#8 referenced somewhere in the discussion, but I am unable to reproduce the issue. I am running macOS Ventura 13.6.4.

The color scheme is detected from the terminal by default, but can be detected from the OS instead by passing --color-scheme=system --theme=auto:system.

Open Questions

* [ ] Should I keep --detect-color-scheme or remove it?

  • Is the additional startup cost acceptable?

@patbl
Copy link

patbl commented Mar 18, 2024

As suggested in #1746, there are two new options with accompanying environment variables:

  • --theme-dark / BAT_THEME_DARK
  • --theme-light / DELTA_THEME_LIGHT

I think there was a typo (the second one should probably be BAT_THEME_LIGHT).

@bash
Copy link
Author

bash commented Mar 18, 2024

@patbl Yep that's a typo, thanks :)

Lucky me, it's only in the PR description and not in the actual PR changes.

@sersorrel
Copy link

This seems to work well! I do need to specify --detect-color-scheme=always, though – I use bat via lesspipe, and without that flag (actually implemented locally as a patch to change the default, since lesspipe doesn't provide any way to send custom arguments to bat...) the detection doesn't run.

What is the downside of not always doing colour scheme detection? The code briefly mentions a race condition with pagers; am I just lucky to have not hit that myself? What would be the symptoms of this happening?

@bash
Copy link
Author

bash commented Apr 9, 2024

@sersorrel That's great to hear! I'm not familiar with lesspipe, how exactly do you combine bat with lesspipe? Do you pipe bat's output to less?

What is the downside of not always doing colour scheme detection? The code briefly mentions a race condition with pagers.

Yes, exactly :) You can observe the race condition when piping bat's output to less. In that case both processes are started (mostly) simultaneously and try to access the terminal at the same time (both color detection and less read from the terminal and enable / disable "raw mode").

Here's how I witness the race condition:
Run bat --detect-color-scheme always .editorconfig | less.
For me less sometimes won't recognize q for exiting.

@sersorrel
Copy link

sersorrel commented Apr 11, 2024

Honestly, I can't figure out exactly how lesspipe.sh invokes bat... The general mechanism is that less examines $LESSOPEN and pipes the file to it before displaying whatever it prints (whether for syntax highlighting or for e.g. displaying a list of files inside a .zip). lesspipe.sh is a generic script that, when invoked as $LESSOPEN, will look at the filename and apply syntax highlighting or whatever other filtering as appropriate.

I can't reproduce the race condition with that command, interestingly.

@bash
Copy link
Author

bash commented Apr 16, 2024

From a quick (and not thorough) peek at less' source code I think there's indeed no race condition in that case. Less waits for $LESSOPEN to complete before it starts to do its own input processing.

@bash
Copy link
Author

bash commented May 18, 2024

I have been thinking about the --detect-color-scheme flag lately, maybe it'd be more useful as a more general --color-scheme flag instead. It would take the following values:

  • dark - replaces --detect-color-scheme=never
  • light - replaces --detect-color-scheme=never
  • auto - same as --detect-color-scheme=auto
  • auto:always - same as --detect-color-scheme=always
  • system - detect from OS instead (on macOS)

The main advantage is that to disable auto-detection one can choose between dark or light instead of being stuck on dark.

It also opens the door to more keywords such as system that would use the current mechanism on macOS instead of detecting from the terminal.

Thoughts?

@bash
Copy link
Author

bash commented Jul 18, 2024

I finally got around to rebasing this and generalizing --detect-color-scheme to --color-scheme (which also means that detecting dark/light from the OS is back).

Another, maybe better idea to consider is to remove the separate flag completely and overload the meaning of --theme instead:

  • --theme=auto
  • --theme=auto:always
  • --theme=auto:system

I quite like this for a few reasons:

  • No extra option
  • If --theme is set it still always wins against other options
  • Can be set via env var (BAT_THEME)
  • If the additional startup delay is not acceptable as a default we could then go the route of only doing detection when explicitly opted in via --theme=auto or BAT_THEME=auto.

@sersorrel
Copy link

This unfortunately needs another rebase – I think it's just imports and the changelog that conflict.

Overloading --theme (enabling --theme-light and --theme-dark only when --theme=auto, whether that's the default or not) makes sense to me, I think.

Copy link
Collaborator

@keith-hall keith-hall left a comment

Choose a reason for hiding this comment

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

Looks like this will bring a lot of benefits, thanks for the hard work and patience for a code review. It looks good to me, so I'm giving my approval. I especially like the detailed justifications, and the comprehensive test cases. Probably we'll wait to see what another maintainer thinks before merging but I would personally like to see this in the next release 🙂

@bash
Copy link
Author

bash commented Aug 17, 2024

Thank you so much for the review and the kind feedback 😊 Getting this into the next release would be wonderful :)

Do you have any preference on the CLI arguments (separate --detect-color-scheme option vs overloaded --theme?

@keith-hall
Copy link
Collaborator

Overloading --theme could be easier from a maintenance/support perspective as there is only one argument to worry about and thus less to explain to users. So probably that would get my vote 🙂

@bash
Copy link
Author

bash commented Sep 7, 2024

I'm trying to integrate the changes from this PR downstream in delta (no PR yet, issue: #1678).

I made some small changes to accomodate this. Namely:

  • Include the color scheme with the theme (delta needs the color scheme to adjust other colors).
  • Flatten the preference enum (this is not just an improvement for delta, my original enums were just too complicated to use :/)
  • Added a display impl for the ThemePreference enum (delta needs to be able to convert the value back to a string).
  • Expose the BAT_THEME_* env vars as constants (if this is undesired I'd happily revert).

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.

Support for different themes based on Terminal color scheme
4 participants