-
-
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
Highlight matching text in file picker suggestions #1635
Conversation
The file limit is removed if the folder contains |
That doesn't matter, either way the set of entries is too big to compute upfront. |
Oh, I was just trying to clarify that detail. |
@@ -301,7 +301,7 @@ impl Buffer { | |||
y: u16, | |||
string: S, | |||
width: usize, | |||
style: Style, | |||
style: impl Fn(usize) -> Style, // Map a grapheme's string offset to a style |
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 a significant hotpath and we'll also get some code bloat from monomorphization. Why not just do this in render()
by calling set_string
multiple times? I'm only comfortable with this change if the base case (fixed style) gets reasonably inlined by rust.
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.
Why not just do this in
render()
by callingset_string
multiple times?
Because then I'd have to implement truncating the string (and adding an ellipsis if necessary) manually.
Code bloat is limited, since set_string_truncated
is only called three times. To ensure the hotpath doesn't get slower, I'd actually prefer replacing the usage in set_stringn
(which always sets ellipsis
and truncate_start
to false
) with a specialized function, so set_string_truncated
is no longer a hotpath. This would be much simpler and might even be more efficient than the current implementation.
@archseer what do you think?
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.
ping @archseer
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'll finish my review later this weekend
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 don't quite like the duplication but it's fine for now 👍🏻
@@ -301,7 +301,7 @@ impl Buffer { | |||
y: u16, | |||
string: S, | |||
width: usize, | |||
style: Style, | |||
style: impl Fn(usize) -> Style, // Map a grapheme's string offset to a style |
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'll finish my review later this weekend
@archseer sorry for the delay. I updated the comments. CI failure is because of clippy warnings introduced in the new Rust release. Should I fix them here? |
Pushed b935fac, can you rebase? |
Thanks for working on this! |
In the file picker, highlight characters that match the entered pattern using fuzzy-search.
Implementation note: The file picker can suggest
up to 8192thousands of entries. Calculating the offsets of the highlighted characters for all entries on every key stroke would be inefficient, so the offsets are calculated in therender
function only for the currently visible entriesand cached.