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

File picker does not match all files #52

Closed
hovsater opened this issue Jun 2, 2021 · 10 comments
Closed

File picker does not match all files #52

hovsater opened this issue Jun 2, 2021 · 10 comments
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug

Comments

@hovsater
Copy link
Contributor

hovsater commented Jun 2, 2021

In the root of my current project I have a Gemfile. In a sub directory relative to the root I also have a Gemfile. Opening the file picker and searching for Gemfile only result in sub-directory/Gemfile. The root Gemfile isn't matched. In fact, a lot of the root level files aren't matched. Rakefile and Procfile are not matched while Dockerfile and Guardfile are.

In a smaller project, these files are found just fine.

Looks like this might be the culprit

const MAX: usize = 2048;
Picker::new(
files.take(MAX).collect(),

If the project is large enough, this might exclude files exceeding MAX. Since the picker isn't updating the set of files upon filtering, some files will always be excluded.

My Rust knowledge is pretty much non-existent, but would it make sense to pass the iterator directly to the picker and let the Picker limit the iterator after matching?

matches.extend(
self.options
.iter()
.enumerate()
.filter_map(|(index, option)| {
// TODO: maybe using format_fn isn't the best idea here
let text = (format_fn)(option);
// TODO: using fuzzy_indices could give us the char idx for match highlighting
matcher
.fuzzy_match(&text, pattern)
.map(|score| (index, score))
}),
);

So if self.options now is the files iterator containing all results, we could limit the results by adding .take(MAX) at the end and end up with something like:

 self.options
    .enumerate()
    .filter_map(|(index, option)| {
        let text = (format_fn)(option);
        matcher.fuzzy_match(&text, pattern).map(|score| (index, score))
    })
    .take(MAX),
@archseer
Copy link
Member

archseer commented Jun 6, 2021

The problem I see with passing an iterator through: you can only consume it once then we'd need to construct a new one which would re-query the file system.

As a quick fix we could significantly raise the limit? I just primarily wanted to avoid someone using this on something like / and crashing.

@ghost
Copy link

ghost commented Jun 6, 2021

There is many ways to solve this. A compromise solution could be to search enough to fill the screen (like the first 20 files) and keep the iterator for future search. The previous results can be cached.

The drawback is when the user deletes what he was searching a new iterator needs to be created.

@archseer
Copy link
Member

archseer commented Jun 7, 2021

Since we use skim's matching algorithms it might be worth figuring out how skim does this for it's interactive mode.

@ghost
Copy link

ghost commented Jun 7, 2021

I downloaded skim and launch the command from $HOME, $ sk.

$ ps aux | grep -E '^USER|sk$' 
USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
begin    3129735 90.6  2.9 1509812 230360 pts/7  Sl+  11:08   9:46 sk

@hovsater
Copy link
Contributor Author

hovsater commented Jun 7, 2021

I tried on macOS as well.

~ $ sk
~ $ ps -O %mem  -p 57760
  PID %MEM   TT  STAT      TIME COMMAND
57760  0.2 s002  S+     0:47.79 sk

Translates to roughly 267MB on my machine.

@pickfire
Copy link
Contributor

pickfire commented Jun 7, 2021

I looked a bit into skim and I think it does a lot more. Like it does rayon to parallelize search. I wonder if @lotabout have any suggestions how we can integrate it here since we have to display it on a certain rectangle, we may also need to have the ability to do extra filtering later on.

@archseer
Copy link
Member

archseer commented Jun 7, 2021

Oh I don't think we need to embed skim, my thought was since we use the fuzzy-matcher crate which was built for skim there might be something we can learn from the implementation.

@sudormrfbin
Copy link
Member

Seems to be reasonably solved by #171 and #187 ?

@kirawi kirawi added A-helix-term Area: Helix term improvements C-bug Category: This is a bug labels Aug 19, 2021
@workingj
Copy link
Contributor

seems to me that this could be closed...

@dead10ck
Copy link
Member

Agreed, please reopen if you still encounter this problem.

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 C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

7 participants