Skip to content
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

Merged
merged 4 commits into from
Mar 1, 2022

Conversation

Aloso
Copy link
Contributor

@Aloso Aloso commented Feb 8, 2022

In the file picker, highlight characters that match the entered pattern using fuzzy-search.

highlight-search

Implementation note: The file picker can suggest up to 8192 thousands 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 the render function only for the currently visible entries and cached.

@kirawi
Copy link
Member

kirawi commented Feb 8, 2022

Implementation note: The file picker can suggest up to 8192 entries. Calculating the offsets of the highlighted characters for all entries on every key stroke would be inefficient, so the offsets are calculated in the render function only for the currently visible entries and cached.

The file limit is removed if the folder contains .git

@archseer
Copy link
Member

archseer commented Feb 9, 2022

The file limit is removed if the folder contains .git

That doesn't matter, either way the set of entries is too big to compute upfront.

@kirawi
Copy link
Member

kirawi commented Feb 9, 2022

Oh, I was just trying to clarify that detail.

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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.

Copy link
Contributor Author

@Aloso Aloso Feb 9, 2022

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 calling set_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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @archseer

Copy link
Member

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

Copy link
Member

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 👍🏻

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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

@Aloso
Copy link
Contributor Author

Aloso commented Feb 25, 2022

@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?

@archseer
Copy link
Member

Pushed b935fac, can you rebase?

@archseer archseer merged commit 59c691d into helix-editor:master Mar 1, 2022
@archseer
Copy link
Member

archseer commented Mar 1, 2022

Thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants