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

Try to detect the current theme based on the terminal background color #2797

Closed
wants to merge 1 commit into from

Conversation

aykevl
Copy link

@aykevl aykevl commented Dec 8, 2023

This uses the termbg crate, as discussed here: #1746
Other than that, it uses the same logic as on macOS.

This behavior should probably be extended to macOS too, for cases like a terminal with a light background while the system theme is dark (or vice versa). But I didn't want to accidentally break anything on macOS.


This is a continuation of #2631 which can't be reopened.

This uses the termbg crate, as discussed here:
sharkdp#1746
Other than that, it uses the same logic as on macOS.

This behavior should probably be extended to macOS too, for cases like a
terminal with a light background while the system theme is dark (or vice
versa). But I didn't want to accidentally break anything on macOS.
@@ -93,10 +97,23 @@ impl HighlightingAssets {
pub fn default_theme() -> &'static str {
#[cfg(not(target_os = "macos"))]
{
Self::default_dark_theme()
// Try to detect the color scheme from the terminal.
let timeout = std::time::Duration::from_millis(100);
Copy link
Owner

Choose a reason for hiding this comment

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

That is a rather long timeout. How much time does it actually take to determine the preferred theme?

Copy link
Author

Choose a reason for hiding this comment

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

No idea, I just copied the example.

I expect it will be very fast in most cases, but there's also SSH where a roundtrip can easily take 100ms over a long distance.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok. I'm not going to accept this change without comprehensive benchmarks. Let's put it on hold until someone looks into it.

Copy link
Author

@aykevl aykevl Jan 22, 2024

Choose a reason for hiding this comment

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

I added the following quick patch on top of this PR to measure the time:

diff --git a/src/assets.rs b/src/assets.rs
index 4bc73d2..abe5710 100644
--- a/src/assets.rs
+++ b/src/assets.rs
@@ -98,8 +98,9 @@ impl HighlightingAssets {
         #[cfg(not(target_os = "macos"))]
         {
             // Try to detect the color scheme from the terminal.
+            let start = std::time::Instant::now();
             let timeout = std::time::Duration::from_millis(100);
-            if let Ok(theme) = termbg::theme(timeout) {
+            let theme = if let Ok(theme) = termbg::theme(timeout) {
                 // We found a color scheme for this terminal, so use it.
                 match theme {
                     termbg::Theme::Dark => Self::default_dark_theme(),
@@ -108,7 +109,10 @@ impl HighlightingAssets {
             } else {
                 // No color scheme found, default to the dark theme.
                 Self::default_dark_theme()
-            }
+            };
+            let duration = start.elapsed();
+            println!("Time elapsed in termbg::theme is: {:?}", duration);
+            theme
         }
         #[cfg(target_os = "macos")]
         {

...and the result is that it does take quite some time. The time is highly variable, sometimes taking less than 1ms and sometimes taking up to 20ms (or even 40ms) but usually it's around 5ms. (This is in Konsole on Asahi Linux).

That's more than I expected. Feel free to close this PR if you think that's too much.
An alternative would be to look only at the COLORFGBG environment variable, which should be fast but doesn't change when switching between dark/light mode.

(Personally I've just switched to the ansi theme, which doesn't look as good as some of the others but at least consistently works).

Copy link
Author

Choose a reason for hiding this comment

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

@sharkdp what do you think of these numbers?

Copy link
Collaborator

@eth-p eth-p Mar 12, 2024

Choose a reason for hiding this comment

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

In the worst case where bat can't detect the theme, 100ms is the equivalent of making bat 2-3x as slow to start up. Cutting it down to 50ms is a lot better, but it has the risk of both not working over SSH, and still significantly increasing the startup time.

I can't speak for the others, but I don't think that's reasonable as a default. When calling bat repeatedly in a shell script (e.g. bat + ripgrep), that latency adds up really quickly.

I would prefer if it were opt-in. Make two new --dark-theme and --light-theme options that are mutually exclusive with --theme. The logic could work like this:

  1. If --theme or BAT_THEME is provided, use that theme.
  2. If --dark-theme and --light-theme are both provided, detect the terminal background color, and use the appropriate theme.
  3. Otherwise, use the current logic of defaulting to the dark theme.

This would allow terminal background color detection to remain disabled by default for minimal impact on startup time. For those who have a need to switch the theme based on the terminal background (e.g. using multiple profiles, or a terminal that changes background based on the time of day), they would have the ability for bat to adapt to it if they wish.

Copy link

@bash bash Mar 15, 2024

Choose a reason for hiding this comment

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

I would like to chime in here as I submitted a PR similar to this one to delta.

The worst case that you're talking about here is indeed an issue because the timeout conflates two unrelated cases:

  1. The terminal does not support OSC 11
  2. There's a lot of latency (e.g. because we're connected via SSH)

Because of this and a bunch of other reasons I wrote an alternative to termbg called terminal-colorsaurus.

How does colorsaurus solve this issue?
It 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.

Colorsaurus includes benchmark results for various terminals—including some that don't support OSC 11.
Using the technique described above colorsaurus only adds < 100 µs latency for such terminals.

Copy link
Author

Choose a reason for hiding this comment

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

@bash thanks! Right now I'm using --theme=ansi because it's good enough for me and still looks good (in the terminal history) after a light/dark theme switch. But feel free to reuse whatever I wrote so we finally get proper dark mode detection.

Your trick of sending another escape code afterwards is really neat!

Copy link

Choose a reason for hiding this comment

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

Your trick of sending another escape code afterwards is really neat!

@aykevl Thanks :) I have to admit thought, that I did not come up with this trick myself. I found it while digging through this discussion in the Terminal WG.

@sharkdp sharkdp added the help wanted Extra attention is needed label Jan 21, 2024
@aykevl aykevl closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants