Feat: Highlight the cursor-word #6197

SoraTenshi · 2023-03-05T21:41:41Z

This Feature closes #3436

(Early) Feature preview:

I know that <Space>-h exists, however i prefer it the way, that it automatically highlights all occurrences of that word that is under the cursor.

Base functionality
Use LSP
Configurable style

Also another question: would it be preferred if the style is either underline / bold? It depends heavily on the implementation and i am not sure if my current implementation is good.

Inspired by: https://github.com/xiyaowong/nvim-cursorword

pascalkuthe · 2023-03-05T21:57:56Z

You seem to perform a simple text based matching. While this is a good fallback, I think we should use the LS if possible since the same textual word might not always correspond to the same identifier. Specifically you can probably use the text_document_document_highlight request already used by <space>-h. Since you get a list of text ranges from that API it would be more appropriate to create a highlight iterator and merge it before the syntax highlights with syntax::merge.

SoraTenshi · 2023-03-05T22:14:37Z

You seem to perform a simple text based matching. While this is a good fallback, I think we should use the LS if possible since the same textual word might not always correspond to the same identifier. Specifically you can probably use the text_document_document_highlight request already used by <space>-h. Since you get a list of text ranges from that API it would be more appropriate to create a highlight iterator and merge it before the syntax highlights with syntax::merge.

You're correct. I would however still "finalize" the raw, non-lsp version of this feature and then probably prioritize based on the lsp.
Also; from what i know about <Space>-h, i would also implement boundary checks for the highlighting (Space-h behaviour is, that it select the variables)

pascalkuthe · 2023-03-05T22:35:00Z

You're correct. I would however still "finalize" the raw, non-lsp version of this feature and then probably prioritize based on the lsp.

Sure I think having a plaintext version is still nice. That being said I think that doing this as a line annotation is probably not quite the right way to go (although I am really happy you like the API :P). Really long words can be split across multiple lines when soft-wrapping so a line doesn't always end with a word (which can lead to wrong highlights in multiple ways). It's also slightly slower than it could be (details are a bit complex but unimportant).

The ideal way to implement this would be using a regex \b{word}\b but sadly right now regex requires us to convert a String which negates the performance benefits. Although that will changes soon (super excited for that) but not yet... Instead you probably want to use text.slice(text.line_to_char(start_line)..text.line_to_char(start_line + height).chars() and manually start matching the search string at every search boundary, keep the found matches in a Vec and then use syntax::merge just like you would for the LSP

Also; from what i know about -h, i would also implement boundary checks for the highlighting (Space-h behaviour is, that it select the variables)

Note quite sure what you mean with this

SoraTenshi · 2023-03-05T22:39:40Z

Also; from what i know about -h, i would also implement boundary checks for the highlighting (Space-h behaviour is, that it select the variables)

Note quite sure what you mean with this

Space-h selects all word occurrences, that may also not be within the viewport.

Also, thanks for the idea. i will take a look at that!

pascalkuthe · 2023-03-05T22:42:31Z

Also; from what i know about -h, i would also implement boundary checks for the highlighting (Space-h behaviour is, that it select the variables)

Note quite sure what you mean with this

Space-h selects all word occurrences, that may also not be within the viewport.

Also, thanks for the idea. i will take a look at that!

That's true but not really an issue, syntax:merge will skip anything that's out of bound automatically

SoraTenshi · 2023-03-05T22:46:28Z

Also; from what i know about -h, i would also implement boundary checks for the highlighting (Space-h behaviour is, that it select the variables)

Note quite sure what you mean with this

Space-h selects all word occurrences, that may also not be within the viewport.
Also, thanks for the idea. i will take a look at that!

That's true but not really an issue, syntax:merge will skip anything that's out of bound automatically

oh wow, than it's a lot easier than i thought.
Thanks! Will continue working on it :)

Add full lsp context highlighting Improve

whitespace

SoraTenshi · 2023-04-04T11:36:44Z

@pascalkuthe If you have some spare time, I would love a review :)
Anyone else is of course free to review it as well! :)

pascalkuthe · 2023-04-12T20:24:11Z

helix-term/src/ui/editor.rs

+ let is_word_char = |c: Option<char>| -> bool {
+ if let Some(c) = c {
+ c.is_ascii_alphanumeric() || c == '-' || c == '_'
+ } else {
+ false
+ }
+ };


there is an existing function for this: helix_core::chars::char_is_word that should be reused here. To cover the option you can do c.map_or(false, char_is_word)

pascalkuthe · 2023-04-12T20:27:15Z

helix-term/src/ui/editor.rs

@@ -264,6 +272,121 @@ impl EditorView {
 .for_each(|area| surface.set_style(area, ruler_theme))
 }

+ pub fn cursor_word(doc: &Document, view: &View) -> Option<String> {
+ let is_word = |c: char| -> bool { c.is_ascii_alphanumeric() || c == '-' || c == '_' };


you should use helix_core::chars::char_is_word here to be consistent across the editor

pascalkuthe · 2023-04-12T20:28:21Z

helix-term/src/ui/editor.rs

+
+ let text = doc.text().slice(..);
+ let cursor = doc.selection(view.id).primary().cursor(text);
+ let cursor_char = text.byte_to_char(cursor);


the byte conversion is not needed here almost everything in helix (including cursor(text)) returns char offsets

pascalkuthe · 2023-04-12T20:33:01Z

helix-term/src/ui/editor.rs

+ let start = (0..=cursor_char)
+ .rev()
+ .find(|&i| !is_word(text.char(i)))
+ .map(|i| i + 1)
+ .unwrap_or(0);
+ let end = (cursor_char..text.len_chars())
+ .find(|&i| !is_word(text.char(i)))
+ .unwrap_or_else(|| text.len_chars());


index into the repo repeatetly is pretty inefficient. A rope is a binary tree internally so this is O(Wlog(N)) . By comparison doing text.chars_at(cursor_char).take_while(is_word).count() is just O(log(N)+W) (just finding the position once). You can take a look at set_completion in commands.rs for an example where we calculate the start of the word (start_offset) at the cursor position. The end works similarly

pascalkuthe · 2023-04-12T21:21:37Z

helix-term/src/ui/editor.rs

+ for line in line_range {
+ let current_line = text.line(line);
+ let current_line = match current_line.as_str() {
+ Some(l) => l,
+ None => continue,
+ };


this is ineffecit. First you are iterating a line range and obtaining a line with text.line which is O(logN) so this loop is O(MlogN) with the lines iterator it would be O(logN + M). However you are not actually interested in lines anyway since you are really just obtaining the string. The problem is that as_str will return None at any line that contains a ropey chunk boundary which could weridly discard lots of matches. There is actually aa way to implement this with a small temporary buffer that doesn't discard any matches at all but that complexity is probably not worth it/required here for now. Instead you can just iterate the chunks directly. That way only words that actually contain a chunk boundary are now shown (which is pretty rare):

Suggested change

for line in line_range {

let current_line = text.line(line);

let current_line = match current_line.as_str() {

Some(l) => l,

None => continue,

};

for chunk in text.slice(text.line_to_char(line_range.start)..text.line_to_char(line_range.end)).chunks(){

pascalkuthe · 2023-04-12T21:27:31Z

helix-term/src/ui/editor.rs

+ if *i != 0 {
+ !is_word_char(current_line.chars().nth(i - 1))
+ } else {
+ true
+ }


match_indicies return byte indices so using that as a char offset is incorrect (and slow).
A more efficient way to implement this would be

Suggested change

if *i != 0 {

!is_word_char(current_line.chars().nth(i - 1))

} else {

true

}

char_is_word(chunk[..i].chars().next_back())

(returns false automatically at chunk boundaries as next_back returns None in that case)

pascalkuthe · 2023-04-12T21:28:49Z

helix-term/src/ui/editor.rs

+ ) -> Option<Vec<(usize, std::ops::Range<usize>)>> {
+ let scope_index = theme.find_scope_index("ui.wordmatch")?;
+ let mut result: Vec<(usize, std::ops::Range<usize>)> = Vec::new();
+ let is_lsp_ready = doc.language_server().is_some() && !editor.cursor_highlights.is_empty();


I think there should be some specical handling to indicate when the LSP is ready. It's perfectly valid to for the LSP to return an empty range and if that is really the LSPs newest response than we we should do respect that. An option would probably work well

I am not sure if my updated check works as expected, but i think it's as close to figuring out whether lsp positions should be used or not.

pascalkuthe · 2023-04-12T21:33:49Z

helix-view/src/editor.rs

+
+ /// Contains all the cursor word highlights
+ pub cursor_highlights: Arc<Vec<std::ops::Range<usize>>>,


this needs to be either cleared when edits are applied to the document or (even better) mapped trough the changes applied to the document which would remove the need for the fallback implementation you added entirely. You can probably build on the improvements to change mapping I did in #6447

helix-term/src/commands/lsp.rs

helix-term/src/ui/editor.rs

Fix some leftover ifs

Underline words under cursor

bfd9b58

SoraTenshi added 2 commits March 9, 2023 00:36

Apply suggestions

9806fc5

Start impl lsp

5ccce75

Add full lsp context highlighting Improve

SoraTenshi force-pushed the underline_highlighted branch from 15a26c9 to 5ccce75 Compare March 9, 2023 20:49

SoraTenshi added 3 commits March 9, 2023 21:52

Merge branch 'master' into underline_highlighted

59c6a88

Fix formatting mistake

ca55385

Fix clippy, don't check for lsp on collection

686cd6f

SoraTenshi marked this pull request as ready for review March 9, 2023 21:10

SoraTenshi added 3 commits March 10, 2023 20:36

Re-add lsp check

02ae0fb

Merge branch 'master' into underline_highlighted

8717692

Fix missing }

684e6bc

whitespace

SoraTenshi force-pushed the underline_highlighted branch from bc0aca7 to 684e6bc Compare March 14, 2023 10:12

SoraTenshi mentioned this pull request Apr 12, 2023

Highlight mutable variables #6659

Open

pascalkuthe self-requested a review April 12, 2023 20:22

pascalkuthe requested changes Apr 12, 2023

View reviewed changes

SoraTenshi added 3 commits April 24, 2023 23:49

Work on review comments

fb18d3e

Merge branch 'master' into underline_highlighted

9c382f0

Correct merge problems

ed47f0b

poliorcetics reviewed May 1, 2023

View reviewed changes

SoraTenshi added 4 commits May 2, 2023 21:48

Use new let else syntax

dc3641a

Merge branch 'master' into underline_highlighted

25f8ec7

Some more usage of the new Syntax

70a952c

Fix some leftover ifs

Merge branch 'master' into underline_highlighted

c5c6e99

SoraTenshi marked this pull request as draft May 25, 2023 18:41

Merge branch 'master' into underline_highlighted

5eec7f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Highlight the cursor-word #6197

Feat: Highlight the cursor-word #6197

SoraTenshi commented Mar 5, 2023 •

edited

Loading

pascalkuthe commented Mar 5, 2023

SoraTenshi commented Mar 5, 2023

pascalkuthe commented Mar 5, 2023

SoraTenshi commented Mar 5, 2023

pascalkuthe commented Mar 5, 2023

SoraTenshi commented Mar 5, 2023

SoraTenshi commented Apr 4, 2023

pascalkuthe Apr 12, 2023

SoraTenshi Apr 24, 2023

pascalkuthe Apr 12, 2023

pascalkuthe Apr 12, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023

SoraTenshi Apr 24, 2023

pascalkuthe Apr 12, 2023


		/// Contains all the cursor word highlights
		pub cursor_highlights: Arc<Vec<std::ops::Range<usize>>>,

Feat: Highlight the cursor-word #6197

Are you sure you want to change the base?

Feat: Highlight the cursor-word #6197

Conversation

SoraTenshi commented Mar 5, 2023 • edited Loading

pascalkuthe commented Mar 5, 2023

SoraTenshi commented Mar 5, 2023

pascalkuthe commented Mar 5, 2023

SoraTenshi commented Mar 5, 2023

pascalkuthe commented Mar 5, 2023

SoraTenshi commented Mar 5, 2023

SoraTenshi commented Apr 4, 2023

pascalkuthe Apr 12, 2023

Choose a reason for hiding this comment

SoraTenshi Apr 24, 2023

Choose a reason for hiding this comment

pascalkuthe Apr 12, 2023

Choose a reason for hiding this comment

pascalkuthe Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

pascalkuthe Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

pascalkuthe Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

pascalkuthe Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

pascalkuthe Apr 12, 2023

Choose a reason for hiding this comment

SoraTenshi Apr 24, 2023

Choose a reason for hiding this comment

pascalkuthe Apr 12, 2023

Choose a reason for hiding this comment

SoraTenshi commented Mar 5, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023 •

edited

Loading

pascalkuthe Apr 12, 2023 •

edited

Loading