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

avoid cnorm on certain terminals #10769

Merged
merged 1 commit into from
May 28, 2024

Conversation

cjbayliss
Copy link
Contributor

@cjbayliss cjbayliss commented May 15, 2024

Inspired @pascalkuthe's comment on my previous PR to revert the use of cnorm for resetting the cursor, I've tried writing an approach that checks the $TERM_PROGRAM or $TERM to decide whether to use cnorm or \x1B[0 q.

Please note, this is my first time writing rust, so please be picky about what I've done wrong or could do better.

I only have access to tmux and alacritty, and getting other terminals on my system is a little tedious because I need to compile them (gentoo). It would be nice if the folks affected by #10089 could run echo $TERM_PROGRAM and echo $TERM in their terminal. I'll add those terminals to this PR.

@pascalkuthe
Copy link
Member

I looked into what vom does a lot more. Manually detecting every terminal cannot/should not be a solution.

If you look at the sourcecode linked in the issue you will see that nvim doesn't actually use cnorm. Instead they use the Se extended capability when available.

The linked code that use TERM detection handles some edgecases but they seem pretty rare/for the most part affect terminal version that are over a decade old. I am ok with not supporting that. I suspect that nobody will actually ever run helix in auch a terminal (the only exception maybe being the Linux virtual terminal nut I doubt people care about cursor style there).

So I think we should just keep the code as is and use Se instead of cnorm.

@cjbayliss
Copy link
Contributor Author

Is Se a standard terminal capability? on my system, none of the installed terminfo files have Se, only cnorm. I can't find any mention of Se in the man pages for terminfo or termcap. Is there an RFC or something I can read about terminal capabilities?

In tmux (with correct $TERM set) Se does not reset the cursor, it changes the cursor to a block. In alacritty Se does reset the cursor. This means we would still need to manually detect some terminals (at least tmux.)

@pascalkuthe
Copy link
Member

pascalkuthe commented May 16, 2024

I think you just need to use Se first he way I described ans then cnorm (if it is set) afterwards

@pascalkuthe
Copy link
Member

And Se is a extended capability and not part of thebdefault set. In practice terminfo is "whatever emulators supprot". There is no rfc or standardization process. You need to look at the sourcecode of implementations

@archseer archseer added this to the 24.05 milestone May 16, 2024
@cjbayliss
Copy link
Contributor Author

cjbayliss commented May 16, 2024

Alright, thank you. I've changed this to check for terminals that don't correctly use either Se or cnorm, then try Se else try cnorm before finally falling back to \x1B[0 q

@cjbayliss
Copy link
Contributor Author

I've tested this PR with the follow terminals and teminal multiplxer combinations:

alacritty -> works
alacritty+tmux -> works
alacritty+screen -> works
kitty -> works
kitty+tmux -> fails to reset the cursor
kitty+screen -> works
foot -> works
foot+tmux -> works
foot+screen -> works

The only combo I've tried that fails to reset the cursor is kitty+tmux. I tried commenting out the tmux work around, and kitty+tmux still fails to reset the cursor. Without the workaround for tmux, the cursor fails to reset in both alacritty+tmux and foot+tmux. Therefore IMO the workaround for tmux is needed.

@the-mikedavis
Copy link
Member

Kitty's docs and author are big proponents of ditching tmux if you use Kitty IIRC (since Kitty has native multiplexing) so I'm not sure we should add the tmux workaround just for Kitty's sake

@cjbayliss
Copy link
Contributor Author

OK. Just to be clear: the workaround for tmux in this PR is for terminal other than kitty.

@pascalkuthe
Copy link
Member

You didn't implement what I said before. We should always use Se (with the fallback of always emitting the seequenve) and cnorm afterwards. Either cnorm is not set (or doesn't reset the curosr shape) then it doesn't matter. Otherwise it overwrites Se.
Emitting both sequences doesn't hurt and should ensure maximum compatability

@cjbayliss
Copy link
Contributor Author

Understood, I've rewritten it to build up a reset string to return, starting with Se, then adding cnorm, then adding the fallback string if in a terminal that needs that (tmux here, maybe more in the future.)

I'm new to rust, so I used push_str() to build the string, is this an appropriate approach? Also the compiler told me to initialize the reset_str with Default::default(), I'm guessing the compiler knows best?

@cjbayliss
Copy link
Contributor Author

I reread your comment, and realized I did it wrong. Now the reset string:

  • adds Se or fallback to \x1B[0 q
  • also add cnorm all the time in all terminals
  • if a terminal where Se/cnorm don't reset the cursor, add \x1B[0 q

@archseer archseer requested a review from pascalkuthe May 19, 2024 23:55

match term_program().as_deref() {
// some terminals don't reset the cursor with either cnorm or Se
Some("tmux") => {
Copy link
Member

Choose a reason for hiding this comment

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

So tmux sets both cnorm and SE and both don't contain that reset sequence?

I guess it's because it passes that escape sequence trough but doesn't know about it itself?

I would really like to avoid special cases like this. I think what would work is if you always prepend the fallback sequence. So basically format!("{fallback}{Se}{cnorm}") (with this approach Se and cnorm won't need a fallback).

It's a bit of a hack but it should work. Emitting the escape sequence multiple times is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that approach works! although I'm confused as to why, because printf '\033[0 q' && tput Se && tput cnorm doesn't work... maybe the way I've implemented it in rust is wrong?

}
fn vte_version() -> Option<usize> {
std::env::var("VTE_VERSION").ok()?.parse().ok()
}
fn reset_cursor_approach(terminfo: TermInfo, fallback: &str) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn reset_cursor_approach(terminfo: TermInfo, fallback: &str) -> String {
fn reset_cursor_escape_sequence(terminfo: TermInfo) -> String {

Passing the fallback as an argument is a bit qeord/unnecessary just put it into the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry about that, it is a habit of my make all state come into the function through the arguments. I've undone it.

@cjbayliss
Copy link
Contributor Author

I've forced pushed an update.

BTW, I'm happy to make as many changes as requested until this is suitable. So please don't feel like you are requesting too many changes. I love helix (thank you for making it ❤️) and want this issue to get resolved hopfully before a release, but it isn't urgent for me personally because I can easily patch helix on my system. 😄

};

format!(
"{}{:?}{}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"{:?}" is wrong here, that is why the cursor resetting was working in tmux for me. I accidentally left the "{:?}" while trying to figure out the types because I'm a rust noob xD

@cjbayliss
Copy link
Contributor Author

right, so I force pushed yet again, because I didn't do the format!() part right. And because of that, Se never made it to the terminal. Now that Se makes it to the terminal, the cursor does not get reset in tmux. I suspect this might be a tmux bug. I should probably report it.

The question is, do we ignore the tmux issue and go with the cleaner approach that works in many terminals?

@cjbayliss
Copy link
Contributor Author

It seems that Se does not reset the cursor in tmux, but sets the cursor to DECSCUSR 2. I don't think they consider that a bug, see: tmux/tmux#1416

I've tried using the override options suggested there, and none of them work for me even when switching the ,xterm*: part to just ,*: in a tmux minimal config. I hoped to get one of them to work so that we could tell tmux users to just set an override.

@cjbayliss
Copy link
Contributor Author

OK, I've done more testing today. If I set TERM to the terminfo of my parent terminal while inside tmux, everything works fine. I know it isn't considered a great practice to set TERM to something other than your terminal's cap file, but I feel like this is a reasonable workaround.

@pascalkuthe I've forced pushed the version without a workaround for tmux. I agree with you that avoiding special cases is the way to go. After all the testing I've done, I think it is best to to tell users if the {fallback}{Se}{cnorm} approach doesn't work, then they should file a bug with their terminal emulator. Or, if in a terminal multiplexer like tmux, try setting TERM to the terminfo of the parent terminal.

Comment on lines 39 to 43
let Some(termini::Value::Utf8String(se_str)) = terminfo.extended_cap("Se") else {
return "".to_string();
};
Copy link
Member

Choose a reason for hiding this comment

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

If there's no Se capability we don't end up emitting the fallback? I thought the intent was to always use the fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this current implementation always emits the fallback, (Se or an empty string), and cnorm in that order

That let se_str is written that way because I couldn't figure out a cleaner way to access the value in Option<Value<'_>> that .extended_cap("Se") returns.

Copy link
Member

Choose a reason for hiding this comment

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

No, this snippet early returns and an empty string is emitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I see, thank you!!

@cjbayliss
Copy link
Contributor Author

I corrected the early return. Thank you for pointing out my blunder! IDK why, but for some reason I was thinking about let variable = some_function() else { return something_esle() }; like it was a closure.

Comment on lines 41 to 43
if let termini::Value::Utf8String(se_str) = terminfo
.extended_cap("Se")
.unwrap_or(termini::Value::Utf8String(""))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let termini::Value::Utf8String(se_str) = terminfo
.extended_cap("Se")
.unwrap_or(termini::Value::Utf8String(""))
if let Some(termini::Value::Utf8String(se_str)) = terminfo.extended_cap("Se")

reset_str.push_str(
terminfo
.utf8_string_cap(termini::StringCapability::CursorNormal)
.unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.unwrap(),
.unwrap_or(""),

@cjbayliss
Copy link
Contributor Author

Thank you for the review! both changes made, and pushed. 🙂

archseer
archseer previously approved these changes May 27, 2024
@archseer archseer requested a review from pascalkuthe May 27, 2024 01:22
@cjbayliss
Copy link
Contributor Author

Should I push a fix to satisfy clippy? I didn't know that using the return keyward was not considered good practice 🙂

@gabydd
Copy link
Member

gabydd commented May 27, 2024

yes, please appease clippy 👍

using a terminfo's cnorm doesn't reset the cursor for many terminals,
see issue: helix-editor#10089
@pascalkuthe pascalkuthe merged commit 1796735 into helix-editor:master May 28, 2024
6 checks passed
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
using a terminfo's cnorm doesn't reset the cursor for many terminals,
see issue: helix-editor#10089
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
using a terminfo's cnorm doesn't reset the cursor for many terminals,
see issue: helix-editor#10089
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.

5 participants