-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
completions: Highlight fuzzy matching #2190
completions: Highlight fuzzy matching #2190
Conversation
.fuzzy_indices(text, pattern) | ||
.map(|(score, fuzzy_indices)| (index, score, fuzzy_indices)) |
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 very slow and expensive for large datasets. We should do what we do in the file picker: use fuzzy_match
to score, then use fuzzy_indices
inside the render method to only compute the indices for elements on screen.
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.
In this case the render
function relies on Table::render_table
function. I can do that, but it would require passing more information to the latter (the highlighted style and the prompt), making it less general. Is it okay?
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.
Sorry for the delay, https://github.com/helix-editor/helix/pull/3053/files#diff-b6c51d6e6645b52604abc7b9cd90c0200ab42d52be2f7e623929376971e784c6R641-R715 has a similar implementation where it patches the spans while rendering. We should be able to reuse the code once that PR is merged
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 see, reusing the code would definitely be the best. Unfortunately that PR hasn't seen much progress in the last couple of months; if you prefer I can pick it up and continue where it was left.
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.
@danyspin97 #3053 has been merged! That should help this PR be able to move further.
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.
@LeoniePhiline Thanks for the heads up :) I have also updated #1905 so this PR should be close to being merged.
closing this one out as stale. Thank you for contributing! |
Requires #1905 .
I had to create a vector for every completion to add the spans, but I didn't notice any slowdown (even with
idle-timeout = 0
). The loop part is only slightly longer for non consecutive matches, but in this case the matching rows are just few ones, so there is no performance impact.Preview: