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

Change cursor shape on mode change #1154

Merged
merged 8 commits into from
Jan 23, 2022

Conversation

sudormrfbin
Copy link
Member

@sudormrfbin sudormrfbin commented Nov 24, 2021

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:

[editor.cursor-shape]
normal = "block"
insert = "bar"
select = "underline"

helix-cursor-shape

Fixes helix-editor#323. Due to terminal limitations we can only
change the shape of the primary cursor.
Comment on lines 262 to 266
spans.push((cursor_scope, cursor_start..range.head));
if i != primary_idx {
spans.push((cursor_scope, cursor_start..range.head));
}
Copy link
Member Author

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 ?

Copy link
Contributor

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?

Copy link
Member Author

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

@dannasessha
Copy link
Contributor

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.

@sudormrfbin
Copy link
Member Author

A new theme scope for ui.cursor.{active,inactive} would work, and we can already change the statusline color based on active window (we can also allow changing the background maybe ?). But that's better tackled in a separate PR.

@pickfire
Copy link
Contributor

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.

@pickfire
Copy link
Contributor

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.

@sudormrfbin
Copy link
Member Author

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

@archseer
Copy link
Member

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 👍🏻

@sudormrfbin
Copy link
Member Author

All modes have a block cursor by default now, but since we're using the terminal cursor for the primary cursor, ui.cursor.primary no longer has any effect #1154 (comment).

@archseer
Copy link
Member

archseer commented Dec 5, 2021

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

@pickfire
Copy link
Contributor

pickfire commented Dec 7, 2021

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.

helix-view/src/editor.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@pickfire pickfire left a 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.

helix-view/src/editor.rs Outdated Show resolved Hide resolved
@archseer
Copy link
Member

Finally had a chance to test this, there's a problem with regex_prompt type inputs: the terminal cursor is rendered a the bottom of the screen in the input prompt, so it's missing on the selection.

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.

Comment on lines +260 to +264
// 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.
Copy link
Member Author

@sudormrfbin sudormrfbin Dec 23, 2021

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

Copy link
Member

@archseer archseer left a 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

@sudormrfbin
Copy link
Member Author

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.

@dylrich
Copy link
Contributor

dylrich commented Jan 11, 2022

A gentle bump since this one seems ready to go! I am very much looking forward to this feature.

@Omnikar
Copy link
Contributor

Omnikar commented Jan 23, 2022

I'm also looking forward to this feature. 👍

@archseer archseer merged commit e2d2f19 into helix-editor:master Jan 23, 2022
@sudormrfbin sudormrfbin deleted the cursor-shape-new branch March 9, 2022 18:49
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.

Add option to change cursor shape on mode change
7 participants