-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Feat: Highlight the cursor-word #6197
base: master
Are you sure you want to change the base?
Conversation
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 |
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
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, |
oh wow, than it's a lot easier than i thought. |
Add full lsp context highlighting Improve
15a26c9
to
5ccce75
Compare
bc0aca7
to
684e6bc
Compare
@pascalkuthe If you have some spare time, I would love a review :) |
helix-term/src/ui/editor.rs
Outdated
let is_word_char = |c: Option<char>| -> bool { | ||
if let Some(c) = c { | ||
c.is_ascii_alphanumeric() || c == '-' || c == '_' | ||
} else { | ||
false | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
helix-term/src/ui/editor.rs
Outdated
@@ -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 == '_' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should use helix_core::chars::char_is_word
here to be consistent across the editor
helix-term/src/ui/editor.rs
Outdated
|
||
let text = doc.text().slice(..); | ||
let cursor = doc.selection(view.id).primary().cursor(text); | ||
let cursor_char = text.byte_to_char(cursor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the byte conversion is not needed here almost everything in helix (including cursor(text)
) returns char offsets
helix-term/src/ui/editor.rs
Outdated
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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
helix-term/src/ui/editor.rs
Outdated
for line in line_range { | ||
let current_line = text.line(line); | ||
let current_line = match current_line.as_str() { | ||
Some(l) => l, | ||
None => continue, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
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(){ | |
helix-term/src/ui/editor.rs
Outdated
if *i != 0 { | ||
!is_word_char(current_line.chars().nth(i - 1)) | ||
} else { | ||
true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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)
helix-term/src/ui/editor.rs
Outdated
) -> 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
/// Contains all the cursor word highlights | ||
pub cursor_highlights: Arc<Vec<std::ops::Range<usize>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This Feature closes #3436
(Early) Feature preview:
![image](https://user-images.githubusercontent.com/13474089/222987436-88f198b2-b7ca-46d6-8337-9eb89c6de90f.png)
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.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