-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Change cursor shape on mode change #1154
Conversation
Fixes helix-editor#323. Due to terminal limitations we can only change the shape of the primary cursor.
db238d6
to
7961355
Compare
helix-term/src/ui/editor.rs
Outdated
spans.push((cursor_scope, cursor_start..range.head)); | ||
if i != primary_idx { | ||
spans.push((cursor_scope, cursor_start..range.head)); | ||
} |
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.
Previously the terminal cursor was hidden and we drew our own primary cursor. Now we use the terminal cursor as the primary cursor and hence ui.cursor.primary
is no longer used to style it. Is there any way to use it to style the 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.
Can you please add a comment for these?
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 was looking for a way to style the terminal cursor so that we can still use ui.cursor.primary
; it's possible to do so and vim allows it: #323 (comment).
There was also talk (chat) about having some difference in appearance between a cursor in an active window, and a cursor in an inactive window, in addition to the connecting idea of having an active window look different from an inactive one. |
A new theme scope for |
I did the same previously and I find it quite ugly so I didn't submit this patch. Maybe we should use a less distracting color for the other cursors or even just hide the cursor like what vim to to avoid distraction? Especially when the other cursors look like block cursor and seemed to be distracting me. |
I still find the default cursor ugly and should be changed, maybe like vim hidden or different theme. Not gonna approve this now due to stylistic reasons. |
@pickfire Hiding the cursors will cause confusion and make it hard to see the edit position (all the cursors may not be aligned at the same column and could be at different places). We could simply have all cursors be block by default and make shape changing opt in. |
Yeah, let's make this behavior opt-in. Looks good otherwise 👍🏻 |
All modes have a block cursor by default now, but since we're using the terminal cursor for the primary cursor, |
Hmm, I liked that behavior :/ It could be replicated on some terminals where the cursor is just an invert of fg/bg, but that's not consistent across all implementations |
opt-in is a good idea, I rather it opt-in since people may not find it look good, at least for me, I think even hiding the cursor (or use a different color) might make it look nicer than reusing the original 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.
Other than the comment I think it should be good since it's opt-in.
Finally had a chance to test this, there's a problem with Could we retain the current behavior by only doing it if the cursor is block shaped? That way it's still stylable and we avoid issues like ^. From what I can tell kakoune's behavior matches our behavior on master: the cursors are all manually drawn. |
// Bar and underline cursors are drawn by the terminal | ||
// BUG: If the editor area loses focus while having a bar or | ||
// underline cursor (eg. when a regex prompt has focus) then | ||
// the primary cursor will be invisible. This doesn't happen | ||
// with block cursors since we manually draw *all* cursors. |
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.
All block cursors are manually drawn now, but we have this edge case with other cursors since there's no way of knowing if the editor are has the focus currently. I think it's okay for the most part since putting a bar cursor for normal mode seems unlikely (but it's a bug nonetheless).
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.
Forgot about this PR 😅
It looks good now 👍🏻 can you rebase to fix conflicts and undo the tree-sitter-scala
submodule change? Then we should be good to merge
Done ! TBH I forgot about the PR too :) The merge commit has some additional changes of implementing Serialize for some types since #798 requires it. |
A gentle bump since this one seems ready to go! I am very much looking forward to this feature. |
I'm also looking forward to this feature. 👍 |
Fixes #323. Due to terminal limitations we can only change the shape of the primary cursor, but it's better than nothing.
We can keep the previous behavior of block cursors for everything by default if needed since only the primary cursor can be changed.Now by default all modes have a block cursor. Configure it like so: