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

fix(crossterm): When no terminfo check if WezTerm for undercurl #13224

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

Conversation

RoloEdits
Copy link
Contributor

On windows TermInfo::from_env (I believe) always fails, so this currently can only be activated by the config option, even if the terminal supports it. We could just leave it at that (and close this PR), or add a list of names of terminals that would support it on platforms like windows (which this PR does with WezTerm) so that it works out of the box. true-color support I beleive is specialized to always return true on windows, so there is precedent for things like this:

// helix-term/src/lib.rs
#[cfg(windows)]
fn true_color() -> bool {
    true
}

@pascalkuthe
Copy link
Member

Not a fan of starting to hardcode specific terminals in helix

@RoloEdits
Copy link
Contributor Author

RoloEdits commented Mar 29, 2025

I would tend to agree. The scope around the change I made looks like:

    pub fn from_env_or_default(config: &EditorConfig) -> Self {
        match termini::TermInfo::from_env() {
            Err(_) => Capabilities {
                has_extended_underlines: config.undercurl
                    || matches!(term_program().as_deref(), Some("WezTerm")), // <-- what I added
                ..Capabilities::default()
            },
            Ok(t) => Capabilities {
                // Smulx, VTE: https://unix.stackexchange.com/a/696253/246284
                // Su (used by kitty): https://sw.kovidgoyal.net/kitty/underlines
                // WezTerm supports underlines but a lot of distros don't properly install its terminfo
                has_extended_underlines: config.undercurl
                    || t.extended_cap("Smulx").is_some()
                    || t.extended_cap("Su").is_some()
                    || vte_version() >= Some(5102)
                    || matches!(term_program().as_deref(), Some("WezTerm")),
                reset_cursor_command: reset_cursor_approach(t),
            },
        }
    }

Should I then change this PR to remove the other WezTerm at the bottom?

I did notice in my search that its also hardcoded here (#3588):

    // helix-view/src/editor.rs
    if env_var_is_set("WEZTERM_UNIX_SOCKET") && exists("wezterm") {
        return Some(TerminalConfig {
            command: "wezterm".to_string(),
            args: vec!["cli".to_string(), "split-pane".to_string()],
        });
    }

Should there be work done to try to clean up these kinds of hard coded things, or just accept them as "dept" and we can close this PR?

@nik-rev
Copy link
Contributor

nik-rev commented Mar 31, 2025

I would tend to agree. The scope around the change I made looks like:

    pub fn from_env_or_default(config: &EditorConfig) -> Self {
        match termini::TermInfo::from_env() {
            Err(_) => Capabilities {
                has_extended_underlines: config.undercurl
                    || matches!(term_program().as_deref(), Some("WezTerm")), // <-- what I added
                ..Capabilities::default()
            },
            Ok(t) => Capabilities {
                // Smulx, VTE: https://unix.stackexchange.com/a/696253/246284
                // Su (used by kitty): https://sw.kovidgoyal.net/kitty/underlines
                // WezTerm supports underlines but a lot of distros don't properly install its terminfo
                has_extended_underlines: config.undercurl
                    || t.extended_cap("Smulx").is_some()
                    || t.extended_cap("Su").is_some()
                    || vte_version() >= Some(5102)
                    || matches!(term_program().as_deref(), Some("WezTerm")),
                reset_cursor_command: reset_cursor_approach(t),
            },
        }
    }

Should I then change this PR to remove the other WezTerm at the bottom?

I did notice in my search that its also hardcoded here (#3588):

    // helix-view/src/editor.rs
    if env_var_is_set("WEZTERM_UNIX_SOCKET") && exists("wezterm") {
        return Some(TerminalConfig {
            command: "wezterm".to_string(),
            args: vec!["cli".to_string(), "split-pane".to_string()],
        });
    }

Should there be work done to try to clean up these kinds of hard coded things, or just accept them as "dept" and we can close this PR?

perhaps it'd be more suitable to find out a way how to hard code this (and similar) in the helix-editor/crossterm fork, instead of here?

@RoloEdits
Copy link
Contributor Author

We will know more of what to do here once progress is made there. Specially if querying the terminal itself for capabilities can reach parity between platforms.

@the-mikedavis
Copy link
Member

For better detection of true color and extended underlines I'd like to take advantage of a VT request/response sequence: the DECRQSS/DECRPSS query for SGR. Some terminals like Kitty and Foot already support it but not WezTerm or Alacritty yet. I sent a patch for WezTerm wezterm/wezterm#6856 and Alacritty has an open issue for it (alacritty/alacritty#7184). See https://github.com/termstandard/colors?tab=readme-ov-file#querying-the-terminal. The same can be done for extended underlines by including an SGR sequence to set the underline color and checking the response SGR for that color. This will take a bit of work on the Helix side as well but I am already planning changes around that - I will add a comment about that to #13223.

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.

4 participants