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

Editor: Themable cursor #1373

Merged
merged 8 commits into from
Feb 27, 2020

Conversation

glennsl
Copy link
Member

@glennsl glennsl commented Feb 26, 2020

A few technical adjustments were needed to bring in the theme colors. The cursor was converted from a component to an immediate mode render function, and the "background", i.e. the text under the cursor, is now rendered on top of the cursor using the background color.

@glennsl glennsl requested a review from bryphe February 26, 2020 21:53
@bryphe
Copy link
Member

bryphe commented Feb 26, 2020

Thanks for fixing this, @glennsl ! Looks great (seems it is properly themed now) - and much more usable, since the character under the cursor is no longer obscured.

@bryphe
Copy link
Member

bryphe commented Feb 27, 2020

One issue I saw - the 'tab' character is not rendered correctly:
image

It'd be preferable to not render anything in that case

let width = float(characterWidth) *. context.charWidth;
Draw.rect(~context, ~x, ~y, ~width, ~height, ~color=foreground);

switch (BufferLine.subExn(~index=offset, ~length=1, bufferLine)) {
Copy link
Member

Choose a reason for hiding this comment

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

For grabbing the character - we actually want ~index=cursorPosition.column instead of offset.

There's actually 3 'metrics' for character position:

  • The byte position of a character. This is the actual byte position in the string.
  • The UTF8-index of a character. UTF-8 characters have a variable length - from 1-4 bytes. So if you have two 4-byte characters - the first character would be at byte index 0, character index 0, and the second would be at byte index 4, character index 1. Most of the BufferLine APIs use the UTF8-index of the character.
  • The width (and offset) of a character. These are like the UTF-8 index, except they account for the visual width and visual offset of a character. For example, a tab character is only one byte, but depending on the settings, it could take up a variable amount of visual space. The width is the width of the individual character, and the offset is the x-position of the character, taking into account all the previous character's widths before it.

If we pass in the offset to subExn - we get this result with a tab character:
bug-cursor-overlay

Note how when the cursor is over the c, it's actually showing s - that's because the offset is 4 (due to the visual space of the tab), but we're asking for the character index at position 4 - which would be the s: |\t|c|o|n|s|.

An extra point of confusion is that the cursorPosition.column is a byte position coming from Vim (`github.com/onivim/reason-libvim/pull/90)....

Lot of opportunity for clean up here... It'd be nice to have something in place like we have for the zero-based index vs the one-based index - a way to leverage the type system to help us manage these different 'views' into the UTF-8 string, and not getting any 'parity mismatches' where we pass something like a byte offset into an API that expects a UTF-8 character index.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, and thanks for the explanation! I find it extra confusing that subExn then takes the position in bytes, but the length is in characters. So I have to mix and match even for a single function call.

Anyway, fixed in 5adf1e8

Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

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

Noted a couple issues with the tab character, but overall looks good. Thank you for your work on this!

@glennsl
Copy link
Member Author

glennsl commented Feb 27, 2020

One issue I saw - the 'tab' character is not rendered correctly:
image

It'd be preferable to not render anything in that case

Ah, missed that because it actually doesn't render anything with the font I use. Fixed in 045be58.

@glennsl glennsl requested a review from bryphe February 27, 2020 10:58
Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

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

Latest changes look great. Huge usability improvement 💯

@glennsl glennsl merged commit 5af4abc into onivim:master Feb 27, 2020
@glennsl glennsl deleted the feature/editor/cursor-legibility branch February 27, 2020 17:25
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.

2 participants