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.

Sign up for GitHub

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

Jump to bottom

Use terminfo to reset terminal cursor style #8591

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Use terminfo to reset terminal cursor style #8591

merged 1 commit into from
Oct 26, 2023

Conversation

rmehri01
Copy link
Contributor

Uses the cnorm capability from terminfo if available to reset the terminal cursor style. I tested this with xterm and it seems to fix the issue with making the cursor blink like in #3741.

Closes #2684

the-mikedavis
the-mikedavis reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Just some minor style nits

helix-tui/src/backend/crossterm.rs Outdated Show resolved Hide resolved
helix-tui/src/backend/crossterm.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2023
the-mikedavis
the-mikedavis previously approved these changes Oct 26, 2023
View reviewed changes
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2023
pascalkuthe
pascalkuthe reviewed Oct 26, 2023
View reviewed changes
helix-tui/src/backend/crossterm.rs
reset_cursor_command: t
.utf8_string_cap(termini::StringCapability::CursorNormal)
.unwrap_or("\x1B[0 q")
.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.

On unic thus seems fine but does this work on windows?

Copy link
Contributor Author

@rmehri01 rmehri01 Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah good catch, I just tried it out and it doesn't work on windows since the reset command will be set to an empty string instead.

the-mikedavis
the-mikedavis reviewed Oct 26, 2023
View reviewed changes
helix-tui/src/backend/crossterm.rs Outdated
@@ -31,11 +31,22 @@ fn vte_version() -> Option<usize> {
std::env::var("VTE_VERSION").ok()?.parse().ok()
}

#[derive(Clone, Debug)]
struct ResetCursorCommand(String);
Copy link
Member

Choose a reason for hiding this comment

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

The newtype wrapper for the string here seems unnecessary to me. It's fine just to default with unwrap_or/unwrap_or_else rather than unwrap_or_default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this since we also want the default "\x1B[0 q" to be used when terminfo isn't available, for example on Windows, or else it just defaults to an empty string. Or should I just explicitly make the struct in the other case instead of keeping the Capabilities::default()?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Let's define impl Default for Capabilities instead so Windows can keep calling Capabilities::default

the-mikedavis
the-mikedavis previously approved these changes Oct 26, 2023
View reviewed changes
pascalkuthe
pascalkuthe requested changes Oct 26, 2023
View reviewed changes
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

Small style nit: I dislike using default instead of false. Apart from that this LGTM

helix-tui/src/backend/crossterm.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis merged commit 553ffbc into helix-editor:master Oct 26, 2023
6 checks passed
@rmehri01 rmehri01 deleted the reset_cursor_style branch October 26, 2023 23:37
@kladd
Copy link

kladd commented Nov 7, 2023

The terminfos of the terminals on my machine have cnorm as \E[?12l\E25h, "Stop blinking cursor (AT&T 610)" and "Show cursor (DECTCEM), VT220", which has been my experience since this change.

It seems like there's some adoption of DECSCUSR 0/ \E[0 q resetting the user default cursor though. Alacritty and the default terminal on macOS seem to—and Windows I guess microsoft/terminal#7379 (haven't tried).

kladd added a commit to kladd/helix that referenced this pull request Nov 16, 2023
@kladd
Revert "Use terminfo to reset terminal cursor style (helix-editor#8591)"
2df38ac 
This reverts commit 553ffbc.
danillos pushed a commit to danillos/helix that referenced this pull request Nov 21, 2023
@rmehri01 @danillos
Use terminfo to reset terminal cursor style (helix-editor#8591)
ff293c6
kladd added a commit to kladd/helix that referenced this pull request Jan 14, 2024
@kladd
Revert "Use terminfo to reset terminal cursor style (helix-editor#8591)"
76761be 
This reverts commit 553ffbc.
kladd added a commit to kladd/helix that referenced this pull request Jan 14, 2024
@kladd
Revert "Use terminfo to reset terminal cursor style (helix-editor#8591)"
5504cc8 
This reverts commit 553ffbc.
kladd added a commit to kladd/helix that referenced this pull request Jan 27, 2024
@kladd
Revert "Use terminfo to reset terminal cursor style (helix-editor#8591)"
34a4b38 
This reverts commit 553ffbc.
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
@rmehri01 @dgkf
Use terminfo to reset terminal cursor style (helix-editor#8591)
fa89572
@archseer archseer mentioned this pull request Apr 1, 2024
Open
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
@rmehri01 @mtoohey31
Use terminfo to reset terminal cursor style (helix-editor#8591)
ce7f650
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
@rmehri01 @Vulpesx
Use terminfo to reset terminal cursor style (helix-editor#8591)
45b0027
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
@rmehri01
Use terminfo to reset terminal cursor style (helix-editor#8591)
b1edc66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalkuthe pascalkuthe pascalkuthe approved these changes

@the-mikedavis the-mikedavis the-mikedavis approved these changes

Assignees
No one assigned
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helix changes terminal cursor style
4 participants
@rmehri01 @kladd @the-mikedavis @pascalkuthe