avoid cnorm on certain terminals #10769

cjbayliss · 2024-05-15T22:40:07Z

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 · 2024-05-16T00:08:58Z

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 · 2024-05-16T01:26:23Z

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 · 2024-05-16T08:22:00Z

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

pascalkuthe · 2024-05-16T08:30:27Z

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

cjbayliss · 2024-05-16T23:26:57Z

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 · 2024-05-17T00:20:39Z

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 · 2024-05-17T00:31:43Z

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 · 2024-05-17T00:33:17Z

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

pascalkuthe · 2024-05-17T00:58:47Z

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 · 2024-05-17T01:45:11Z

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 · 2024-05-17T03:13:41Z

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

pascalkuthe · 2024-05-20T04:46:34Z

helix-tui/src/backend/crossterm.rs

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


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.

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?

pascalkuthe · 2024-05-20T04:47:38Z

helix-tui/src/backend/crossterm.rs

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


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

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

cjbayliss · 2024-05-20T06:02:58Z

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. 😄

cjbayliss · 2024-05-20T06:10:49Z

helix-tui/src/backend/crossterm.rs

+    };
+
+    format!(
+        "{}{:?}{}",


"{:?}" 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 · 2024-05-20T06:29:03Z

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 · 2024-05-20T23:44:14Z

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 · 2024-05-22T03:45:13Z

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.

archseer · 2024-05-24T05:10:39Z

helix-tui/src/backend/crossterm.rs

+    let Some(termini::Value::Utf8String(se_str)) = terminfo.extended_cap("Se") else {
+        return "".to_string();
+    };


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

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.

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

oh, I see, thank you!!

cjbayliss · 2024-05-24T07:16:54Z

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.

pascalkuthe · 2024-05-24T09:24:31Z

helix-tui/src/backend/crossterm.rs

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


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")

pascalkuthe · 2024-05-24T09:26:03Z

helix-tui/src/backend/crossterm.rs

+    reset_str.push_str(
+        terminfo
+            .utf8_string_cap(termini::StringCapability::CursorNormal)
+            .unwrap(),


Suggested change

.unwrap(),

.unwrap_or(""),

cjbayliss · 2024-05-24T19:17:58Z

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

cjbayliss · 2024-05-27T01:48:29Z

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

gabydd · 2024-05-27T03:49:58Z

yes, please appease clippy 👍

using a terminfo's cnorm doesn't reset the cursor for many terminals, see issue: helix-editor#10089

cjbayliss mentioned this pull request May 15, 2024

revert: Use terminfo to reset terminal cursor style #10762

Closed

archseer added this to the 24.05 milestone May 16, 2024

cjbayliss force-pushed the reset-cursor-2nd branch from d667907 to 24d1eac Compare May 16, 2024 23:25

cjbayliss force-pushed the reset-cursor-2nd branch from 24d1eac to 7724d34 Compare May 17, 2024 01:45

cjbayliss force-pushed the reset-cursor-2nd branch from 7724d34 to 03d70b1 Compare May 17, 2024 03:13

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

pascalkuthe reviewed May 20, 2024

View reviewed changes

cjbayliss force-pushed the reset-cursor-2nd branch from 03d70b1 to 5c201c5 Compare May 20, 2024 05:57

cjbayliss commented May 20, 2024

View reviewed changes

cjbayliss force-pushed the reset-cursor-2nd branch from 5c201c5 to b4adff2 Compare May 20, 2024 06:26

cjbayliss force-pushed the reset-cursor-2nd branch from b4adff2 to a15ab09 Compare May 22, 2024 03:45

archseer reviewed May 24, 2024

View reviewed changes

cjbayliss force-pushed the reset-cursor-2nd branch from a15ab09 to 24c979a Compare May 24, 2024 07:13

pascalkuthe reviewed May 24, 2024

View reviewed changes

cjbayliss force-pushed the reset-cursor-2nd branch from 24c979a to 26ffc66 Compare May 24, 2024 19:17

archseer previously approved these changes May 27, 2024

View reviewed changes

archseer requested a review from pascalkuthe May 27, 2024 01:22

avoid cnorm on certain terminals

1a59986

using a terminfo's cnorm doesn't reset the cursor for many terminals, see issue: helix-editor#10089

cjbayliss dismissed archseer’s stale review via 1a59986 May 27, 2024 03:57

cjbayliss force-pushed the reset-cursor-2nd branch from 26ffc66 to 1a59986 Compare May 27, 2024 03:57

archseer approved these changes May 28, 2024

View reviewed changes

pascalkuthe approved these changes May 28, 2024

View reviewed changes

pascalkuthe merged commit 1796735 into helix-editor:master May 28, 2024
6 checks passed

pascalkuthe mentioned this pull request Jun 4, 2024

Resetting terminal cursor shape after exiting helix seems broken in 24.03 #10089

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid cnorm on certain terminals #10769

avoid cnorm on certain terminals #10769

cjbayliss commented May 15, 2024 •

edited

pascalkuthe commented May 16, 2024

cjbayliss commented May 16, 2024

pascalkuthe commented May 16, 2024 •

edited

pascalkuthe commented May 16, 2024

cjbayliss commented May 16, 2024 •

edited

cjbayliss commented May 17, 2024

the-mikedavis commented May 17, 2024

cjbayliss commented May 17, 2024

pascalkuthe commented May 17, 2024

cjbayliss commented May 17, 2024

cjbayliss commented May 17, 2024

pascalkuthe May 20, 2024

cjbayliss May 20, 2024

pascalkuthe May 20, 2024

cjbayliss May 20, 2024

cjbayliss commented May 20, 2024

cjbayliss May 20, 2024

cjbayliss commented May 20, 2024

cjbayliss commented May 20, 2024

cjbayliss commented May 22, 2024

archseer May 24, 2024

cjbayliss May 24, 2024

archseer May 24, 2024

cjbayliss May 24, 2024

cjbayliss commented May 24, 2024

pascalkuthe May 24, 2024

pascalkuthe May 24, 2024

cjbayliss commented May 24, 2024

cjbayliss commented May 27, 2024

gabydd commented May 27, 2024

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

avoid cnorm on certain terminals #10769

avoid cnorm on certain terminals #10769

Conversation

cjbayliss commented May 15, 2024 • edited

pascalkuthe commented May 16, 2024

cjbayliss commented May 16, 2024

pascalkuthe commented May 16, 2024 • edited

pascalkuthe commented May 16, 2024

cjbayliss commented May 16, 2024 • edited

cjbayliss commented May 17, 2024

the-mikedavis commented May 17, 2024

cjbayliss commented May 17, 2024

pascalkuthe commented May 17, 2024

cjbayliss commented May 17, 2024

cjbayliss commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjbayliss commented May 20, 2024

Choose a reason for hiding this comment

cjbayliss commented May 20, 2024

cjbayliss commented May 20, 2024

cjbayliss commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjbayliss commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjbayliss commented May 24, 2024

cjbayliss commented May 27, 2024

gabydd commented May 27, 2024

cjbayliss commented May 15, 2024 •

edited

pascalkuthe commented May 16, 2024 •

edited

cjbayliss commented May 16, 2024 •

edited