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

[vim] fix fat cursor for non-monospace fonts #6765

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

krydos
Copy link
Contributor

@krydos krydos commented Aug 14, 2021

This pull request should fix #3256

(non monospace font and header size is different from other portion of a text)
image
image

I don't think it's good idea to check if vim mode is enabled directly in cursorCoords but I don't really see the better way.

Could somebody help me with better solution?

Thanks

@marijnh
Copy link
Member

marijnh commented Aug 15, 2021

Making a core method work differently when the vim mode is enabled in order to be able to use it in a way it isn't intended to is way too dodgy. Maybe see if you can use charCoords instead.

@krydos
Copy link
Contributor Author

krydos commented Aug 15, 2021

@marijnh, do you mean I should update drawSelectionCursor to call charCoords somewhere next to cursorCoords? If so, that makes sense and feels better. I'll try.

@krydos
Copy link
Contributor Author

krydos commented Aug 16, 2021

Ok, new change feels much better. We don't touch measurement functions at all now. drawSelectionCursor function is the only affected by the change and the change itself is related to how the cursor is drawn.

@marijnh thanks for the advice

@marijnh
Copy link
Member

marijnh commented Aug 17, 2021

The core library shouldn't know anything about the vim mode, so testing for it like this doesn't seem like a good idea.

There seems to already be logic in keymap/vim.js that sets a bookmark with a cm-animate-fat-cursor class. But somehow it is only firing in visual mode. I don't know the code well enough to figure out why that is, but it seems that an approach like that (having the vim mode use markText to display its visual cursor) would be the way to do this.

@krydos
Copy link
Contributor Author

krydos commented Aug 17, 2021

@marijnh you're right about cm-animate-fat-cursor, but that part in visual mode doesn't update the cursor width. It just draws one more "fake cursor" and hides the actual cursor.
The actual cursor isn't well-shaped even in visual mode.

I agree that check for vim mode in code library doesn't look well.

Vim mode assigns cm-fat-cursor class to the wrapper element so we can check that I think.

Not sure how better it is but at least we don't directly check for vim mode. It also means that any addon/keymap that wants to use fat cursor will work fine after this change is merged.

@marijnh
Copy link
Member

marijnh commented Aug 18, 2021

Hm, that's still a bit of a hack, but I guess it is preferable to the alternatives. Let's try it this way.

@marijnh marijnh merged commit 2b9b089 into codemirror:master Aug 18, 2021
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.

[Vim] Cursor width too small when use keycap Vim
2 participants