-
Notifications
You must be signed in to change notification settings - Fork 753
feat(terminal): support native terminal cursor rendering #5348
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
Conversation
src/highlighters.cc
Outdated
| } | ||
| // Check if we should skip drawing the primary cursor (for terminal cursor native mode) | ||
| const bool skip_primary_cursor = TerminalUI::has_instance() && | ||
| TerminalUI::instance().is_cursor_native(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about this, I dont like highlighters knowing anything about TerminalUI, I think we could just let users override the PrimaryCursor face in their config to default,default to get the same outcome (and keep the door open for more effects based of primary cursor face).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops not fixed. I'll look into other impelmentation later 😂
5feef81 to
ae47b93
Compare
mawww
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change can be made much smaller, essentially just reacting inside set_ui_options. We probably can just always emit the right escape sequence in there regardless of if it is redundant or not.
ae47b93 to
ca6a580
Compare
|
Have removed all the unnecessary changes. Thanks for the quick review! |
src/terminal_ui.cc
Outdated
| m_padding_fill = find("terminal_padding_fill").map(to_bool).value_or(false); | ||
|
|
||
| // Emit cursor show sequence | ||
| write(STDOUT_FILENO, "\033[?25h"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not mean to always enable it, I think we still want to have the terminal_native_cursor option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I've reverted that removal commit.
This reverts commit 549a5d2.
mawww
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the comment and update the doc/pages/options.asciidoc file as well to document this new ui_option ?
src/terminal_ui.cc
Outdated
| if (new_cursor_native != m_terminal_cursor_native) | ||
| { | ||
| m_terminal_cursor_native = new_cursor_native; | ||
| // Emit cursor visibility command when the option changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that comment does not add much value, it seems to just describe what the code obviously does.
src/terminal_ui.hh
Outdated
|
|
||
| Codepoint m_padding_char = '~'; | ||
| bool m_padding_fill = false; | ||
| bool m_terminal_cursor_native = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can probably remove the "terminal_" part from the variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it 👍🏼
src/terminal_ui.cc
Outdated
| if (new_cursor_native != m_terminal_cursor_native) | ||
| { | ||
| m_terminal_cursor_native = new_cursor_native; | ||
| write(STDOUT_FILENO, m_terminal_cursor_native ? "\033[?25h" : "\033[?25l"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this means that the PrimaryCursor face will be overridden by the color used by the terminal emulator,
and that cursor shapes (blinking etc) will be visible.
On a related note, I like that Neovim changes the cursor to a vertical line while in insert mode; Maybe Kakoune can achieve this even with its software cursor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrimaryCursor will still be applied, then on top of it the native cursor effect from the terminal, I suspect most users will want to override PrimaryCursor to default,default.
Make the cursor trail animation work in the Kitty terminal. The Ghostty terminal is also adding support for cursor shaders.
Demo
2025-06-24.8.39.24.mp4