Move terminal claim/restore to helix-tui, cache keyboard enhancement support query #6194
Conversation
This moves the `Application::claim_term` and `helix-term::application::restore_term` functions into the helix-tui crate. How the terminal should be claimed and restored is a TUI concern and is implemented differently through different TUI backends. This cleans out a lot of crossterm and TUI code in Application and makes it easier to modify claim/restore based on information we query from the terminal host. The child commit will take advantage of this to cache the check for whether the host terminal supports the keyboard enhancement protocol. Without this change, caching that information takes much more code which is not easily reusable for anything else. The code to restore the terminal is somewhat duplicated by this patch: we want to restore the terminal in cases of panics. Panic handler hooks must live for `'static` and the Application's terminal does not.
Wether the host terminal supports keyboard enhancement can be cached for the lifetime of a Helix session. Caching this lookup prevents a potential lockup within crossterm's event reading system where the query for the keyboard enhancement support waits on the next keyboard event, which can happen if the crossterm event stream is checked by `tokio::select!` in another thread.
the-mikedavis
added
A-helix-term
|
This resolves #6149 for me with I don't know if there's a good way to measure it, but it feels like there's still a small but noticeable lag when I first open a file, compared to versions before a066815, where opening a file feels instantaneous. Would it make sense to have an option similar to |
|
I'll give
The mechanisms for detecting true-color and undercurl support (see also #6196) are not very robust (they depend on an environment variable and your installed terminfo database respectively) and the query for keyboard enhancement support here is much more reliable (a direct request/response between the application and host terminal) so I don't think a new option to force it should be necessary. |
In my testing this takes around 3-4ms in terminals that support the enhanced keyboard protocol (Kitty, WezTerm) and a few hundred microseconds in terminals that don't (st, Alacritty).
|
I added some logging to the debug line that says whether the protocol is supported or not. I checked around and the check seems pretty fast on terminals that do support the protocol (3-4ms on Kitty, WezTerm) and very fast on terminals that don't (~100μs on st, Alacritty). Let me know if you find longer measurements! I think it might be a nice improvement to run this query and enable keyboard enhancement asynchronously after startup. It isn't safe to do it with the current way queries work in crossterm though - that would need to wait on a change in crossterm that would allow multiple concurrent readers. |
|
Ugh, I was comparing a |
This is #6160 but in a way that is more extendable in the long-term. This will make it easier to add checks for other host terminal information like initial cursor shape, title line, whether the terminal supports synchronized output, etc. without adding a whole bunch of fields to Application or functions to the Terminal trait.
The first commit in this change moves
Application::claim_termandapplication::restore_termtohelix-tuiinto the crossterm backend. I believe this is the more proper place for this code and it makes the second change much easier: we cache whether the terminal supports the enhanced keyboard protocol, resolving #6149 (though it would stil be nice to improve this upstream).