-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update crossterm, use more terminal features #13223
Conversation
Historically we haven't wanted to add light/dark mode since it required platform-specific dependencies. The Contour terminal [proposed] a VT extension to query for the current mode and subscribe to future changes, however, which can be supported squarely in Crossterm with no new deps. Currently at least Contour, Kitty and Ghostty support this on the terminal emulator side and Neovim on the client side. This patch allows configuring multiple themes: # this is still accepted: # theme = "ayu_dark" theme = { dark = "ayu_dark", light = "ayu_light" } On startup we query the terminal for the current mode and choose that theme. We then receive events from crossterm whenever the terminal changes theme. [proposed]: https://github.com/contour-terminal/contour/blob/master/docs/vt-extensions/color-palette-update-notifications.md
`cossterm::terminal::terminal_features` queries for all VT extension features at once and gives back a small struct with the current state of each feature. We can use this struct to determine which features the terminal supports. This patch changes to prefer this feature over individual queries like `supports_keyboard_enhancement` and `query_terminal_theme_mode`. This also updates our handling of the Kitty keyboard protocol. Once we enable the protocol we ask the terminal for the currently enabled flags. If we see that the terminal hasn't enabled the requested flags then we turn off the feature. This is intended for Zellij which doesn't support the `REPORT_ALTERNATE_KEYS` flag. When `DISAMBIGUATE_ESCAPE_CODES` is enabled but not `REPORT_ALTERNATE_KEYS` we get key combinations like `A-S-9` instead of `A-(` (which we also get without the enhanced keyboard protocol). Disabling the protocol is simpler in this case than normalizing based on the keyboard.
This takes advantage of the synchronized output commands from crossterm to tell the terminal emulator when to pause and resume drawing. This should feel snappier and have fewer visual artifacts like tearing when we update the screen with many changes (scrolling or changing theme for example).
@@ -47,6 +47,7 @@ unicode-segmentation = "1.2" | |||
ropey = { version = "1.6.1", default-features = false, features = ["simd"] } | |||
foldhash = "0.1" | |||
parking_lot = "0.12" | |||
crossterm = { package = "helix-crossterm", version = "0.1.0-beta.1" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merging this we should publish 0.1.0
@@ -13,6 +13,7 @@ use crate::{ | |||
}, | |||
}; | |||
|
|||
use arc_swap::access::DynAccess; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think importing this as is can ruin some type inference for some instances of DynGuard
(let
bindings would then need a fully specified type which, can be long nested types for this kind of use case)? Sort of a precautionary measure, but I would just fully qualify the two instances of the use here.
edit: Yeah, just cherry-picked this and did see this when I am load
ing from my icons branch.
The light/dark switching is not working in Ghostty 1.1.3 on NixOS. Here are the logs:
|
It might be worth adding an override for some of these features if there is some platform/terminal issues. We do that for [editor]
synchronized-output = true
# ... |
WezTerm doesn't yet support the theme extension: wezterm/wezterm#6454. The kitty keyboard protocol is supported but it has to be enabled in config: https://wezterm.org/config/lua/config/enable_kitty_keyboard.html None of the features can be detected on Windows though since the escape sequence parsing is compiled out ( |
There seems to be an issue with how Ghostty handles the request to figure out the current theme mode: $ printf '\e[?996n\e[c'
[?62;22c[?997;2n It's queueing the response for the query so it comes after any other requests. That makes it hard to query for - we'd have to set a timeout to detect terminals that don't support the feature and that would affect time-to-first-render. I'll open an issue with Ghostty. Edit: looks like this was discussed recently: ghostty-org/ghostty#6738. I'll mark this as a draft until we can figure out how to handle this. |
I think overrides definitely need to be added. I added it in my build branch (for the synchronized output) and it seems to work as expected.
I'll try to take a look at the fork you made and poke around. See if I can't get something together, at least for the features that helix uses. I was thinking we could use the |
Something I came across for the kitty protocol is something I also came across for yazi which it handled in sxyazi/yazi#2474 helix will need to do something similar, otherwise if you enable the helix config, lets say a
Also another issue (perhaps unrelated idk) is that when clicking on links in helix, say a url, scrolling changes to be (at least it seems) arrow inputs. So rather than scroll the page and have the cursor follow along, scrolling then becomes cursor actions (up and down). |
For this I believe we just need to tweak the impl of I wonder if we actually need to care about the winapi stuff anymore. As I understand it, cmd.exe got support for escape sequences in Windows 10 and I believe the Rust compiler hasn't supported anything older than Windows 10 since Feb 2024 |
I took a much longer look into this and the problem is that crossterm doesn't "speak VT" on Windows even though it is possible now with ConPTY. It handles key events from the Console API the old way: https://github.com/crossterm-rs/crossterm/blob/36d95b26a26e64b0f8c12edfe11f410a6d56a812/src/event/sys/windows/parse.rs#L204-L296. It also needs to enable VT inputs and attempt to handle key events as possible VT escape sequences like Termwiz does. But all VT parsing code is compiled only on Unix currently. It's not an easy change to make in crossterm. It's also not easy to switch over to Termwiz sadly. I tried reviving @archseer's work in 7a51085 as the-mikedavis@0a515da but Termwiz has an API which is fairly different from crossterm. Both libraries also depend on the deprecated Neither crate is quite what we want so I've got a new crate that only "speaks VT" in progress which merges the nicer parts of crossterm and Termwiz with a lower-level interface that should be flexible enough to do all of the manipulation and querying we want. |
I've got the new branch for that (☝️) up in #13307 so I will close in favor of that. That PR doesn't include the theme mode stuff (#13281) but I will add it in a follow-up PR. Enabling the theme mode updates is a matter of |
This is a rollup of a few crossterm related changes. It also switches us to use https://github.com/helix-editor/crossterm instead of upstream crossterm - the idea is to maintain a soft fork where we can experiment with features before sending them upstream.