Implement cursorline #2170

TobTobXX · 2022-04-19T09:35:07Z

Highlight all lines where cursers are on, imitating vim's cursorline option.

Todo:

implement it
config option
documentation
update (a few) themes

Improvements:

Use ui.cursorline[.primary | .secondary]
Discuss whether to highlight secondary cursors.
TBD: Option to disable on inactive window
TBD: Syncing?

TobTobXX · 2022-04-20T01:08:42Z

Ok, almost everything is done now. Only the discussion point, about ui.highlight. I don't think updating themes is within the scope of this PR, but maybe another key would have better support?

Also, this is my first contribution btw, so apply extra scrutiny when reviewing, even though I applied diligence to keep conventions.

Callum-Irving · 2022-04-30T18:57:29Z

Another thing that would be useful to add is the ability to only show the cursorline in some modes. In Neovim I have it set up to only show the cursorline in insert mode which helps me quickly see what mode I'm in without looking at the status line.

TobTobXX · 2022-05-01T00:20:21Z

Tangential, but not orthagonal: The ui.highlight was actually first used (I think) for highlighting the current line when using the DAP. As this may conflict, we may need conditional logic anyway. Any maintainer has input on this?

the-mikedavis · 2022-05-01T14:51:42Z

Re-using ui.virtual.ruler could be an interesting option here since cursorline is pretty much a horizontal ruler that can move. Could also introduce a new ui. scope for this, something like ui.cursorline with fine-grained sub-scopes like

# based on modes:
ui.cursorline.insert
ui.cursorline.normal
# or highlight the primary cursor line differently
ui.cursorline.primary

Although we haven't used different theming based on modes before - doing it with fine-grained scope names might not be the best option (considering crossing the .primary suffix with .{insert,normal,select} - that's a lot of scopes).

TobTobXX · 2022-05-02T07:48:52Z

Oh, yeah, using ui.virtual.ruler makes a lot more sense. I don't think having different colors depending on mode makes a lot of sense. I don't know what @Callum-Irving was thinking of, but I guess you'd usually only want to be able to set the cursorline to on/off depending on mode, but not to different colors.

ChrHorn · 2022-05-03T11:20:42Z

I'm currently experimenting with themes a bit. For me a separate setting like ui.cursorline would be nice.
Most Vim/VSCode themes I looked at use different colors for cursor line and ruler.

Regarding the mode switching, I think a more universal solution might be better. You could also change other UI elements, like the status line color. But this is probably out of scope for this PR.

archseer · 2022-05-04T02:19:14Z

Although we haven't used different theming based on modes before

We do this for selections already and yeah the behavior gets a bit confusing (see #2366)

Do we really want to highlight all lines with cursors though? That might get very noisy when editing a lot of selections. I assumed this would only be implemented for the primary cursor

TobTobXX · 2022-05-06T11:54:57Z

Agree, doing it for the primary cursor only (and not showing a cursorline when there are multiple cursors) makes more sense.

Noted these improvements. But right now, final exams are kinda filling my time. I'll continue working on the PR before the 23rd, you can ping me if I don't.

helix-term/src/ui/editor.rs

TobTobXX · 2022-05-26T00:47:37Z

Ok, this is how it works currently:

There are three new theme keys:

ui.cursorline
ui.cursorline.primary (used for cursorline for primary cursor, falls back to ui.cursorline)
ui.cursorline.secondary (used for cursorline for secondary cursors, falls back to ui.cursorline)
(all of these fall back to the Style::default() (ie. no coloring) when none of the above is specified by the theme.

The intention is that a theme can (but must not) distinguish between primary and secondary cursors. If just ui.cursorline is specified, all cursors highlight with this color. If either of the .primary or .secondary is specified, the primary resp. the secondary cursorlines take the specified appearance.

This all can be toggled by the config option cursorline.

Technically, one of the two .primary and .secondary could be removed, they are redundant. Should we do that? If yes, which one?

TobTobXX · 2022-05-26T01:18:14Z

Now, If I understood @pickfire and @Zoybean correctly, there are these features that could be nice:

(1) Only show cursors for primary window
(2) Sync between cursorlines between windows on the same buffer

(1) could be a config option. Probably the simplest way would be to convert the cursorline from a boolean to an enum of "false, active, true". Although, really needed? Input pls.
(2) IMHO, only syncing the cursorlines is confusing and unintuitive. Cursorlines are meant to represent where the cursor is, which they won't in inactive windows. It'd probably be better to properly implement synced cursors instead of just synicng the styling. 2c. Input pls too.

pickfire · 2022-05-26T05:15:50Z

(1) Only show cursors for primary window

For the primary window yes, but if the same line appear on other windows, show them as well. Since on the same document, the user is editing on the same line. I am thinking enum would be better given that it could have multiple options.

(2) Sync between cursorlines between windows on the same buffer

No, don't sync cursorline, but only show the same cursorline on the same window.

If a cursorline appears on the first window, on all the window opened with the same buffer, show it.

But when user changed to another window on the same buffer, the original cursorline for that buffer is still the same, so now all other buffers will highlight the same cursorline the user is on for all windows.

This way, when user input some stuff on the primary window, the characters won't just magically appear in secondary window, there will also be a cursorline for that secondary window so it users that user is on that line, but in a different color.

tl;dr my suggestion

cursors are not shared between buffers/windows, but for every window with the primary buffer, show the cursors from the primary window, and for every other buffers, show their own cursor for their own window, only the main cursor in main primary window should show in unique color

Zoybean · 2022-05-26T06:10:21Z

These seem to overlap too much for me to cleanly separate them in my head, but here is how I would ideally have mine set up (along with my thought process):

Only the primary window has visible cursors, so only the primary window's cursors should have lines. (I think this maps to 1)
Cursors are not shared between buffers, so cursor lines for one buffer shouldn't appear in another buffer. (I think this maps to 2)
Corollary: only show cursor lines in the primary window. (Maybe this maps to 1)

This seems distinct from pickfire's answers, so I agree that this would make sense to be configurable along these lines

TobTobXX · 2022-05-26T21:29:30Z

Ok, I think I got it. I was confused, because usually, when I open the same buffer in two windows, it's because I'm editing/viewing a large file at two completely different locations. But I can see the use.
I'll try my hand at it.

Other RFCs:

(1) ATM I'm just naïvely adding config options. Right now I am thinking about the config option cursorline with these options:

none (default, no cursorline)
active-only (only show cursorline on focused window)
all (current behaviour)
synced (pickfire's behaviour)

(Note: I do prefer the current behaviour, since when changing the window, I know in advance where the cursor will be. So I would like to also keep this behaviour.)

(2) Also the themeing keys are starting to get out of hand. At least, four are required, any combination of focused / unfocused window and primary / secondary cursor. This puts a relatively high burden on theme porters to support cursorlines IMO. This could be improved by having sensible defaults (ie. inactive colors default to their active counterparts). I'd recommend the following keys:

ui.cursorline (universal fallback)
ui.cursorline.primary (primary cursor, fb: ui.cursorline)
ui.cursorline.secondary (secondary cursors, fb: .primary)
ui.cursorline.primary.unfocused (primary, unfocused, fb: .primary)
ui.cursorline.secondary.unfocused (secondary, unfocused, fb: .secondary)

archseer · 2022-05-31T06:12:46Z

This seems to be getting a bit overengineered: helix is not doom emacs, so it's okay to diverge.

In our case each window has it's own set of cursors. We already only show cursors for the current view/active files, so I imagine it would get confusing if cursorlines of all views were rendered but didn't map to a selection.

Since the intent is to make it easier to find the main cursor, I think it's sufficient to only render the primary selection's cursorline in the active view. Rendering it on all views pulls eye focus and isn't as easy to figure out which view is actually focused.

TobTobXX · 2022-06-01T11:44:41Z

@archseer What about this:

config option cursorline which is a bool.

And these themeing keys:

ui.cursorline (coloring for primary cursors)
ui.cursorline.secondary (secondary cursors, no coloring when not specified)

sbromberger · 2022-06-19T19:41:22Z

It looks like this errors if ui.selection is unset. Why is that?

(Actually, I have ui.selection defined, and it's saying "Invalid theme: requires ui.selection". Digging deeper.)

The error only appears (and the theme is not loaded) when ui.cursorline is set. Theme is here: https://gist.github.com/d2b3515615e0370eaf7c3c49d0105750

the-mikedavis · 2022-06-19T20:38:59Z

There's a syntax error because the =s are missing. The blanket error message for all toml parsing failures for themes at the moment is to point out that ui.selection is missing.

sbromberger · 2022-06-19T20:54:50Z

oh, man, that was dumb of me. Thanks.

sbromberger · 2022-06-19T20:56:50Z

for some reason enabling cursorline is causing noticeable lag in scrolling. I just have a bg style set, and it's significantly slower to scroll than when cursorline is disabled.

pickfire · 2022-06-19T23:51:48Z

for some reason enabling cursorline is causing noticeable lag in scrolling. I just have a bg style set, and it's significantly slower to scroll than when cursorline is disabled.

It feel slower at first, but I compare it a couple of times, it seemed similar. I guess that's because the whole line is moving and you eyes need to process a lot of stuff. Not sure if it's really slower since I didn't time it to the milliseconds with replay.

sbromberger · 2022-06-20T00:12:11Z

It feel slower at first

It's definitely slower, but I'm getting used to it. Certainly having the cursorline makes up for the delay.

helix-term/src/ui/editor.rs

the-mikedavis · 2022-06-20T01:32:46Z

I don't notice any slowdown running in release or debug mode and scrolling quickly with a mouse. It might be worth poking around with cargo-flamegraph if it's noticeable. From a quick glance I see highlight_cursorline itself taking almost no time

the-mikedavis · 2022-06-20T01:39:05Z

For #2170 (comment), let's keep ui.cursorline.primary and ui.cursorline.secondary. Either will fall back to ui.cursorline if undefined, so the least a theme needs to support cursorline is to add a ui.cursorline theme. Most existing themes can use their ui.virtual color.

@archseer for

only render the primary selection's cursorline in the active view

were you thinking of only rendering the primary selection altogether (no secondary selections at all), or only highlighting the primary selection as ui.cursorline.primary on the focused view?

pickfire · 2022-06-22T08:00:33Z

helix-term/src/ui/editor.rs

+            if primary_line == line {
+                surface.set_style(area, primary_style);
+            } else if secondary_lines.contains(&line) {
+                surface.set_style(area, secondary_style);
+            }


Why not just highlight primary_line after secondary_line after the loop so we don't have to check whether or not it was part of secondary line?

We still have to check for each line if secondary_lines contains it, since line is one of all the lines on the screen, only some will be highlighted.

Strictly speaking, not checking and then just overwriting after the loop might be even (veery slightly) slower, since the comparison primary_line == line is basically free, but coloring the secondary_line (which is redundant because it will be overwritten) is less cheap. (I might be wrong though.)

archseer · 2022-06-22T10:02:21Z

I think we want to provide the ability to highlight both primary and secondary selections, but by default I'd only style the primary selection (users can add their own styling for secondary if they want).

helix-term/src/ui/editor.rs

TobTobXX · 2022-06-22T10:24:43Z

@archseer

but by default I'd only style the primary selection

So make ui.cursorline.secondary fall back to nothing? ie. when it is not specified, secondary selections are not colored?

the-mikedavis · 2022-06-22T22:04:23Z

To do it without adding an exception to the fallback behavior, we could add a theme just for ui.cursorline.primary to the default theme(s). ui.cursorline.secondary won't fall back to that highlight.

poliorcetics · 2022-06-27T08:56:42Z

The documentation for ui.cursorline.{primary|secondary} was not added to the book I think

TobTobXX force-pushed the master branch from a297513 to 9e06157 Compare April 20, 2022 00:59

TobTobXX marked this pull request as ready for review April 20, 2022 01:09

pickfire reviewed May 11, 2022

View reviewed changes

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved

the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2022

TobTobXX force-pushed the master branch from 6faad6a to e4ed1ea Compare May 24, 2022 23:27

the-mikedavis removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 27, 2022

the-mikedavis reviewed Jun 20, 2022

View reviewed changes

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved

Implement cursorline

21e894f

TobTobXX force-pushed the master branch from 7e92040 to 21e894f Compare June 20, 2022 08:37

the-mikedavis linked an issue Jun 20, 2022 that may be closed by this pull request

Add "highlight current line" #1761

Closed

pickfire reviewed Jun 22, 2022

View reviewed changes

archseer reviewed Jun 22, 2022

View reviewed changes

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved

Binary search possible lines

df376ba

archseer merged commit 8dc86be into helix-editor:master Jun 27, 2022

the-mikedavis mentioned this pull request Jun 28, 2022

add docs for cursorline scopes #2904

Merged

This was referenced Jul 27, 2022

Accessibility Improvement - Cursor Locator #3218

Closed

Add transparent theme based on base16_default #3216

Merged

gabydd mentioned this pull request May 29, 2023

Cursorline colors #7164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cursorline #2170

Implement cursorline #2170

TobTobXX commented Apr 19, 2022 •

edited

Loading

TobTobXX commented Apr 20, 2022

Callum-Irving commented Apr 30, 2022

TobTobXX commented May 1, 2022

the-mikedavis commented May 1, 2022

TobTobXX commented May 2, 2022

ChrHorn commented May 3, 2022 •

edited

Loading

archseer commented May 4, 2022

TobTobXX commented May 6, 2022

TobTobXX commented May 26, 2022 •

edited

Loading

TobTobXX commented May 26, 2022

pickfire commented May 26, 2022 •

edited

Loading

Zoybean commented May 26, 2022

TobTobXX commented May 26, 2022 •

edited

Loading

archseer commented May 31, 2022

TobTobXX commented Jun 1, 2022

sbromberger commented Jun 19, 2022 •

edited

Loading

the-mikedavis commented Jun 19, 2022

sbromberger commented Jun 19, 2022

sbromberger commented Jun 19, 2022

pickfire commented Jun 19, 2022 •

edited

Loading

sbromberger commented Jun 20, 2022

the-mikedavis commented Jun 20, 2022

the-mikedavis commented Jun 20, 2022

pickfire Jun 22, 2022

TobTobXX Jun 22, 2022

archseer commented Jun 22, 2022

TobTobXX commented Jun 22, 2022

the-mikedavis commented Jun 22, 2022

poliorcetics commented Jun 27, 2022

Implement cursorline #2170

Implement cursorline #2170

Conversation

TobTobXX commented Apr 19, 2022 • edited Loading

Todo:

Improvements:

TobTobXX commented Apr 20, 2022

Callum-Irving commented Apr 30, 2022

TobTobXX commented May 1, 2022

the-mikedavis commented May 1, 2022

TobTobXX commented May 2, 2022

ChrHorn commented May 3, 2022 • edited Loading

archseer commented May 4, 2022

TobTobXX commented May 6, 2022

TobTobXX commented May 26, 2022 • edited Loading

TobTobXX commented May 26, 2022

pickfire commented May 26, 2022 • edited Loading

Zoybean commented May 26, 2022

TobTobXX commented May 26, 2022 • edited Loading

archseer commented May 31, 2022

TobTobXX commented Jun 1, 2022

sbromberger commented Jun 19, 2022 • edited Loading

the-mikedavis commented Jun 19, 2022

sbromberger commented Jun 19, 2022

sbromberger commented Jun 19, 2022

pickfire commented Jun 19, 2022 • edited Loading

sbromberger commented Jun 20, 2022

the-mikedavis commented Jun 20, 2022

the-mikedavis commented Jun 20, 2022

pickfire Jun 22, 2022

Choose a reason for hiding this comment

TobTobXX Jun 22, 2022

Choose a reason for hiding this comment

archseer commented Jun 22, 2022

TobTobXX commented Jun 22, 2022

the-mikedavis commented Jun 22, 2022

poliorcetics commented Jun 27, 2022

TobTobXX commented Apr 19, 2022 •

edited

Loading

ChrHorn commented May 3, 2022 •

edited

Loading

TobTobXX commented May 26, 2022 •

edited

Loading

pickfire commented May 26, 2022 •

edited

Loading

TobTobXX commented May 26, 2022 •

edited

Loading

sbromberger commented Jun 19, 2022 •

edited

Loading

pickfire commented Jun 19, 2022 •

edited

Loading