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

support word and line completion #4816

Closed
wants to merge 2 commits into from
Closed

support word and line completion #4816

wants to merge 2 commits into from

Conversation

QiBaobin
Copy link
Contributor

Screen Shot 2022-11-19 at 20 09 36

Screen Shot 2022-11-19 at 20 08 17

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 19, 2022
@the-mikedavis
Copy link
Member

It looks like this works by shelling out to rg? You shouldn't need an additional tool installed to do this: word completion should be fully built-in

@QiBaobin
Copy link
Contributor Author

QiBaobin commented Nov 20, 2022

It looks like this works by shelling out to rg? You shouldn't need an additional tool installed to do this: word completion should be fully built-in

@the-mikedavis Good point, fixed.

Comment on lines +4003 to +4008
let matches = grep_impl(
&query,
current_content,
if search_root { Some(config) } else { None },
match_whold_line,
)
Copy link
Member

Choose a reason for hiding this comment

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

I think we either want to only use open buffers or current buffer. Scanning the entire tree can easily be very costly and tons of disk I/O.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's very useful for editing source code without lsp, and completing lines is also very useful for writing similar logic in other files even there is a lsp.

We don't scan the file tree for C-x or idle completions, only do it when manually trigger by C-n and C-l, but also limit the match count to a few number.

Comment on lines +3966 to +3972
completion_grep(
cx,
format!(r"\b{}\w+", prefix,),
start_offset,
search_root,
false,
);
Copy link
Member

Choose a reason for hiding this comment

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

Using a regex to extract words is a lot less efficient than just scanning the rope. Here's another approach that does that #3328

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we use this to search external files too just like global_search, we don't load the content into a Rope, right? If it makes sense to change the logic to extract words from the matched lines, I can do the change there.

let all_matches: Vec<FileResult> =
UnboundedReceiverStream::new(all_matches_rx).collect().await;
let all_matches: Vec<FileResult> = UnboundedReceiverStream::new(all_matches_rx)
.map(|(path, _, line_num)| FileResult::new(&path, line_num as usize - 1))
Copy link
Member

Choose a reason for hiding this comment

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

Seems more wasteful to do this upfront rather than calculating it for matches only (after search_path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you guiding me how to write a common function or sink that accepts a transform lambda? I am stuck there, so I send all three fields to the channel.

@ahmed1jilali
Copy link

I really need word completion, does any one know when it's going to happen, thanks

@estin estin mentioned this pull request Feb 27, 2023
@QiBaobin QiBaobin closed this by deleting the head repository Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants