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

feat: Add local search in buffer #13053

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

oxcrow
Copy link
Contributor

@oxcrow oxcrow commented Mar 8, 2025

Hi everyone,

I added a feature to do local search in the currently opened document buffer to improve user experience.

This feature is similar to global_search but is only limited to one document.

The command is activated by space+l and seems to work well.

Kindly review and allow this feature to be merged.

Thank you
~oxcrow

Screenshot From 2025-03-08 12-31-58

Reasoning

  • The global_search method is quite nice since it works in a fuzzy finding method and provides a table of matches that we can easily preview to find what we want.
  • A similar feature is needed for searching through the currently opened buffer (i.e only one document). We could use global_search however it produces too many false positive results from other files, which makes it difficult to use.
  • The standard local search method using / does not feel very good to use.
  • Other editors such as Emacs/Neovim provide plugins for local search such as Swiper, which feel better to use https://github.com/abo-abo/swiper
  • This feature is implemented to provide a similar feature as Swiper.

Implementation

  • I took the code of global_search and modified it little bit.
  • Specifically I modified the code to only search through the current document.
  • The code works but it should be improved by someone more experienced (if necessary).

Grep search through a local buffer similar to `global_search`.

The method works but it should be improved by someone more experienced.
@nik-rev
Copy link
Contributor

nik-rev commented Mar 8, 2025

Showing the path to the file in the global search picker where there can be lots of files makes sense. But here it doesn't really make sense, because you're searching in the same file - so all picker items will show the same path

Maybe instead of showing the path, show the line number and the string contents of that line in the 2nd column

As per review comments of helix maintainers,
- The filename and directory path was hidden for local_search
- The : colon separator was also hidden since it is not required
@oxcrow
Copy link
Contributor Author

oxcrow commented Mar 9, 2025

@nik-rev Thank you for your feedback.

I was able to implement and push the changes you requested.

Here is the result.

fix

On another note, I also found a bug that needs to be fixed. The local_search method seems to also search in all opened buffers. As shown in this image the search result contains results from two files README.md and test.txt.

I will try to fix it.

bug

@nik-rev
Copy link
Contributor

nik-rev commented Mar 9, 2025

Nice! So you have a "line number" column. What about having a 2nd column for "text on that line"? Since there is quite a lot of whitespace

@oxcrow
Copy link
Contributor Author

oxcrow commented Mar 9, 2025

I agree that having the "text on the current line" being displayed with the search result will be helpful.

Since I'm new to hacking on helix I do not know how to extract the data from the buffer right now.

Once I know how to extract the data I can display it by modifying the contents of columns.

I will try to look into this and implement this.

Any help regarding which file/struct/variable contains the buffer text data so I can extract (clone) it will be helpful.

Thank you.

@nik-rev
Copy link
Contributor

nik-rev commented Mar 9, 2025

I agree that having the "text on the current line" being displayed with the search result will be helpful.

Since I'm new to hacking on helix I do not know how to extract the data from the buffer right now.

Once I know how to extract the data I can display it by modifying the contents of columns.

I will try to look into this and implement this.

Any help regarding which file/struct/variable contains the buffer text data so I can extract (clone) it will be helpful.

Thank you.

You have access to a cx: &mut Context. doc!(cx.editor) should give you the currently focused Document which has methods like Document::text() which returns a Rope data structure representing the text in the current file

Display placeholder line content with local_search result.
Since columns expect a fn and not a closure it proved challenging
to extract data from cx.editor without borrowing cx within the scope.

For now a placeholder line content is placed until we fix this.
@oxcrow
Copy link
Contributor Author

oxcrow commented Mar 13, 2025

Issue: I have not been able to extract data from cx.editor.text().

Since columns expects a fn and not a closure, I was not able to borrow/access cx.editor from inside columns.

I was able to pad the line number with whitespaces to align the results, and add a dummy / placeholder text (that needs to be fixed).

Now how can I access the text data within columns?

I'm thinking of storing the text data inside FileResult when we run the search.

This will most likely work.

#[derive(Debug)]
struct FileResult {                                            
    path: PathBuf, 
    line_num: usize,
    line_content: String, // << Just store the text data here ?
}          

result

Store and display line content in local_search result.

TODO: Fix the awful formatting of the displayed line content.
@oxcrow
Copy link
Contributor Author

oxcrow commented Mar 13, 2025

Success: The line content can now be displayed.

The formatting looks awful so I will try to fix it by only showing the first N non-whitespace characters of the lines.

result

Using a maximum limit of 80 characters per line allows the results to
be displayed correctly on a wide monitor. Unfortunately on small
monitors the issue still persists.

Reduce the padding length from 12 to 8.
@oxcrow
Copy link
Contributor Author

oxcrow commented Mar 13, 2025

Success: The results are now formatted correctly.

result

Separate the line number and the line content rendering logic.
Use "line" as column header instead of "path".
@Axlefublr
Copy link
Contributor

omg I've been wanting this for a while!!

This fixes a bug where results were being returned from all
document buffers opened in the editor.

Now only one document is searched.

The current document.
@oxcrow
Copy link
Contributor Author

oxcrow commented Mar 16, 2025

Success: The bug has been fixed.

Now only the current open document buffer is searched.

The other document buffers are ignored.

Thus even though multiple buffers are open searching "x" only returns results from the current document buffer.

@nik-rev Is there anything else that needs to be added/modified?

Most things seem right to me. One thing that might need to be fixed, is how we format the results on smaller monitors. Since I allowed the max result length to be 80, the results might wrap around on smaller screens.

I added a variable for this in the code called local_search_result_line_length, which if necessary, could be exposed to the user so they can configure it.

However I do not know how.

Thanks
~oxcrow

result

@nik-rev
Copy link
Contributor

nik-rev commented Mar 16, 2025

Success: The bug has been fixed.

Now only the current open document buffer is searched.

The other document buffers are ignored.

Thus even though multiple buffers are open searching "x" only returns results from the current document buffer.

@nik-rev Is there anything else that needs to be added/modified?

Most things seem right to me. One thing that might need to be fixed, is how we format the results on smaller monitors. Since I allowed the max result length to be 80, the results might wrap around on smaller screens.

I added a variable for this in the code called local_search_result_line_length, which if necessary, could be exposed to the user so they can configure it.

However I do not know how.

Thanks ~oxcrow

result

Looks good to me so far. I'll use it in my own fork of Helix from now on to give it a proper test drive

@nik-rev Is there anything else that needs to be added/modified?

FYI I'm not a maintainer, so you'll need someone who is to review this too

@oxcrow
Copy link
Contributor Author

oxcrow commented Mar 21, 2025

@nik-rev The current local searcher uses grep but I'm not too happy with it.

It would be netter to use a fuzzy searcher so that it would be easier for users to find complex things easily.

Do you know how may I replace the grep searcher with a fuzzy searcher?

The fuzzy file finder seems to work in the way that I want to replicate here. So maybe I will start by looking in there first.

@ivanrg99
Copy link

I would love to see this merged.

@oxcrow you could take a look at nucleo, made by the helix team and already in the dependencies.

I can see it being used in the Picker

I will revisit this when I have some more free time. Would love to help in any way possible

@ivanrg99
Copy link

I got it working, but it now loads all the lines directly into the picker, instead of starting the picker empty. Would that be ok?

image

image

If that is the case, should I create a PR on your fork @oxcrow ? I'm just starting to contribute back to open source so I don't know the approach

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.

4 participants