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

Adaptive Theme Selection Based on the Terminal Color Scheme #12098

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luca-schlecker
Copy link
Contributor

@luca-schlecker luca-schlecker commented Nov 20, 2024

Fixes #8899, #10281

Overview

Introduce theme = { light = "<name>", dark = "<name>" } syntax inside config.toml to allow adaptive theme selection based on the terminal color scheme.

Theme selection is based on terminal-colorsaurus which has also been used in delta and bat.

Considerations

  • If the theme detection fails, dark mode is chosen by default.
  • The new syntax using an untagged enum ensures existing configs remain valid.
  • Theme validation (ensuring the theme name exists) is only done for the detected theme. This would defer errors if a missing theme is specified but reduces code changes.
  • terminal-colorsaurus' code is only called in case an adaptive theme has been specified, guaranteeing practically zero overhead for static themes.

@cmoog
Copy link
Contributor

cmoog commented Nov 25, 2024

There seems to be a small bug when this runs via :config-reload

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Nov 26, 2024
@luca-schlecker
Copy link
Contributor Author

This bug turns out to be a bit trickier. I've spent a few hours and couldn't come up with a clean solution. Crossterm and Colorsaurus are interfering with each other here because Colorsaurus, instead of querying the operating system, uses ANSI escape codes to get the terminal background color and determines the active color scheme that way. This approach has some benefits like allowing theme detection through an ssh session.

I believe the interference is happening as soon as the future from the async function input_stream.next() (Application::event_loop_until_idle) is still pending whilst trying to detect the color scheme.

I've pondered a bit and could think of the following workarounds:

  • Use dark-light to query the operating system instead (thus no terminal i/o required, but less flexible).
  • Use a Semaphore to prevent input_stream.next() from being called when trying to detect the color scheme. For this to be effective the Semaphore would have to be locked before this tokio::select!, increasing latency of config reloads as the select-call would have to finish first.
  • Tie theme detection directly to input events (Application::event_loop_until_idle):
Some(event) = input_stream.next() => {
    // <-- theme detection works here without interference
    self.handle_terminal_events(event).await;
    // <-- also here
}
  • Only determine the terminal color scheme once at startup and don't update it on config reload.

Let me know what your thoughts on this are.

@MaxVerevkin
Copy link

  • Only determine the terminal color scheme once at startup and don't update it on config reload

That would be very limiting and render this feature useless for some. People that want to specify two color schemes are likely to switch between light/dark regularly.

Ideally, the change of theme should be detected automatically, some how. I don't know if there is a standard way to do it without polling.

@luca-schlecker
Copy link
Contributor Author

luca-schlecker commented Nov 27, 2024

  • Only determine the terminal color scheme once at startup and don't update it on config reload

That would be very limiting and render this feature useless for some. People that want to specify two color schemes are likely to switch between light/dark regularly.

I fully agree with you, but I would leave this decision to the maintainers as they are the ones having to maintain that code in the long run.

Ideally, the change of theme should be detected automatically, some how. I don't know if there is a standard way to do it without polling.

On Windows, there's the WM_THEMECHANGED Notification using WinProc.
On MacOS there's AppleInterfaceThemeChangedNotification and NSDistributedNotificationCenter seen on this stackoverflow and also implemented in electron with this PR: electron/electron#14755.
On Linux there seems to be org.freedesktop.appearance.color-scheme mentioned on the gnome block.

The dark-light crate currently supports asynchronous Linux theme changes but still polls the MacOS and Windows endpoints. Maybe additional support for the aforementioned notification endpoints could be contributed. Theme detection would then only be based on the operating system instead of the terminal color scheme though and wouldn't work through an SSH session for example.

Maybe allow the user to switch between different implementations?:

theme = "solarized_dark" # static
theme = { source = "os", light = "solarized_light", dark = "solarized_dark" } # Based on the OS Theme
theme = { source = "terminal", light = "solarized_light", dark = "solarized_dark" } # Based on the Terminal color scheme

This would increase implementation complexity though.

@bash
Copy link

bash commented Dec 3, 2024

One possible alternative approach would be to send out the OSC query on startup and on config reload. Then process the response in handle_terminal_events. This requires more work on your side–not as nice as calling terminal_colorsaurus directly, but fixes the interference.

I'd be open to exposing more low-level functions in colorsaurus as needed (i.e. xparsecolor).


Unsolicited info dump regarding change detection

Detecting changes directly from the OS is really annoying on macOS (requires an initialized NSApplication) and Windows (requires a running event loop). I have some of those caveats documented here: https://docs.rs/mundy/latest/mundy/struct.Preferences.html#method.subscribe

There's currently no way that I'm aware of to be notified by the terminal about theme changes.
The only exception here is iTerm sending SIGWINCH on theme change. I started a discussion about this in VTE's issue tracker, but it hasn't seen any movement–mainly because I haven't had time and energy for this.

@Rudxain
Copy link
Contributor

Rudxain commented Dec 11, 2024

dark-light crate currently supports asynchronous Linux theme changes

Nit-pick: It does on main but not stable (1.1.1)

@bash
Copy link

bash commented Dec 11, 2024

I threw together a prototype based on the idea proposed in my previous comment: #12238

It requires changes to terminal-colorsaurus (of which I am the owner and can publish as needed) and crossterm (no clue if they would accept my change).

@luca-schlecker feel free to adopt anything from my prototype PR.

@kirawi
Copy link
Member

kirawi commented Dec 13, 2024

Just to add, I believe dark-light was rejected for being too heavy a dependency. The maintainers can clarify if I'm wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically select light/dark theme variant
6 participants