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

Adapt Helm to use flx for better fuzzy matching #3327

Closed
tuhdo opened this issue Oct 10, 2015 · 37 comments
Closed

Adapt Helm to use flx for better fuzzy matching #3327

tuhdo opened this issue Oct 10, 2015 · 37 comments

Comments

@tuhdo
Copy link
Contributor

tuhdo commented Oct 10, 2015

I already created this thread on Reddit. If you already use ido, you know that flx vastly improve Emacs experience.

@bmag
Copy link
Contributor

bmag commented Oct 10, 2015

Using flx in Helm will be awesome, but are you sure this works as expected? When using smex, if I type "em" then eldoc-mode is the first result, but when using helm-M-x, typing "em" gives evil-mode as the first result and eldoc-mode is only the second. When looking at "Emacs Command History" candidates before I type any input, I see that eldoc-mode is much higher (more recent) than evil-mode, so why don't I get eldoc-mode as the first result?

@tuhdo
Copy link
Contributor Author

tuhdo commented Oct 10, 2015

Yes I'm sure it works, and works as you described. But you must not execute evil-mode, so it does not appear in command history and is prioritised higher. A better way is to compared between before and after this tweak is added. Before adding, typing hpa gives candidates like w[h]at-[pa]ge, c[h]eck-[pa]rens....; after adding, typing hpa gives [h]elm-[p]rojectile-[a]g (characters in [] are highlighted)..

@d12frosted
Copy link
Contributor

Wow. Nice thing. Any chance that it will be part of Spacemacs?

@ReneFroger
Copy link

Any update on this? I found Helm somewhat faster after enabling flx.

@tuhdo
Copy link
Contributor Author

tuhdo commented Oct 15, 2015

We can easily update now. @PythonNut added a package for it: https://github.com/PythonNut/helm-flx

@d12frosted
Copy link
Contributor

Great to hear.

TheBB added a commit to TheBB/spacemacs that referenced this issue Oct 15, 2015
@TheBB TheBB mentioned this issue Oct 15, 2015
@TheBB
Copy link
Contributor

TheBB commented Oct 15, 2015

#3414

@CestDiego
Copy link
Contributor

are we doing this? <3 I see nobody objected this.

@TheBB
Copy link
Contributor

TheBB commented Oct 18, 2015

The PR is up there at least.

@herbertjones
Copy link
Contributor

Unfortunately it appears that helm-flx has a poor runtime growth rate. As the input buffer size grows larger than 20, there is a visible slowdown. To test, SPC b b, then type a single character about 20 times. Continue inserting characters, watching the delay increase. Each time a character is entered, the helm mini buffer takes approximately twice as much time as the previous press to update the display which indicates a growth rate of 2^n.

See incident #3500 where this problem appears in Helm Find Files.

@tuhdo
Copy link
Contributor Author

tuhdo commented Oct 22, 2015

I do not have this issue, even for 50k candidates, on both OS X/Linux. I am also using latest develop. Maybe you should try on stock Spacemacs to see if the problem occurs?

@herbertjones
Copy link
Contributor

@tuhdo You repeated a single character into the input buffer 25 times and got 50k candidates? I don't think you are talking about the same input as I am. I am talking about the length of the input field. Of course, for helm mini it isn't normal to take 25+ keystrokes before finding what you are looking for. However, in Helm Find Files, attempting to recurse down a deep directory hierarchy is normal, as is trying to use Helm Find Files to create a new file, perhaps one with a name longer than 25 characters. Unfortunately in that situation this plugin doubles the amount it takes to react for each character in the filename or directory path entered, eventually timing out around 25 characters. This makes helm break before entering the filename, so that Helm Find Files can't be used to create new files that have long filenames, or are in a deep directory hierarchy.

I have completely wiped my .spacemacs and .emacs.d directory and reproduced the issue. The issue is more apparent on OSX, as it locks up the GUI, unlike on Linux.

System Info

  • OS: darwin
  • Emacs: 24.5.1
  • Spacemacs: 0.105.0
  • Spacemacs branch: develop (rev. 47ce170)
  • Distribution: spacemacs
  • Layers:
(emacs-lisp git)

@TheBB
Copy link
Contributor

TheBB commented Oct 22, 2015

Surely helm-find-files shouldn't be testing against the whole path, but rather the last part (the after the last forward slash, that is)?

I can reproduce the issue though. Basically what I did:

cd ~
mkdir yolloyollo && cd yolloyollo
# Repeat about 20 times

Then go to helm-find-files and try to navigate to the end of ~/yolloyollo/.../. Good luck. I can make it about four or five levels in before it gives up.

@CestDiego
Copy link
Contributor

if you are gonna type the whole path, why use fuzzy search? for full path I use C-x C-f

@TheBB
Copy link
Contributor

TheBB commented Oct 22, 2015

I can't even C-l my way to the end. No typing necessary.

@PythonNut
Copy link
Contributor

@TheBB @herbertjones

See lewang/flx#75.

@tuhdo
Copy link
Contributor Author

tuhdo commented Oct 23, 2015

I still can't reproduce according to @TheBB instructions. In this demo, you can see that when I type elm, helm-M-x gave me evil-local-mode, eldoc-mode...., it means helm-flx-mode is activated. Beside, I am using veresion 105.x. Then, I tried to go down the yolo directories as much as I could, but I could not see any slow down on update:

test

However, @PythonNut confirmed this issue and made a fix, so it did exist and I'm glad it got fixed so quickly.

@PythonNut
Copy link
Contributor

Admittedly, I haven't reproduced the bug myself either. I do know that flx has horrible asymptotic complexity over the length of the input.

I assume that you need to invoke a recursive file finder to run into the bug.

@ghost
Copy link

ghost commented Oct 23, 2015

Note that helm-flx won't give good results when the number of matches exceeds helm-candidate-number-limit, because helm will only pass helm-flx the first LIMIT matches it finds, no matter their quality.

emacs-helm/helm#1237
emacs-helm/helm#1238

@PythonNut
Copy link
Contributor

@alkapote thanks. I'm glad to see this being worked on. It's been in the back of my mind as a sort of mathematical bugaboo for a while.

@tuhdo do note that I'd be totally okay with adding advise to helm-flx if we can't merge it into helm proper.

@robbyoconnor
Copy link
Contributor

@PythonNut -- @tuhdo is the a ninja with helm -- he knows what he's saying/doing :D

@cpaulik
Copy link
Contributor

cpaulik commented Oct 24, 2015

Helm-flx seems to break some helm features for me.

Compare the two situations on the screenshot below. On the left is helm and on the right helm+helm-flx.

  • helm-flx removes the colors for directories, symlinks etc.
  • helm-flx makes it harder to distinguish between e.g. a new file tes entered below and marked with ? in original helm
  • helm-flx shows the full path for some reason.
    20151024112603

This is on latest develop with
helm-20151023.1330 and helm-flx-20151023.2240

@TheBB
Copy link
Contributor

TheBB commented Oct 24, 2015

Yeah, that's unfortunate. I see the same, and I had to update packages this morning to reproduce it, so it must be a recent change.

@TheBB
Copy link
Contributor

TheBB commented Oct 24, 2015

@PythonNut I assume it's due to PythonNut/helm-flx@bf4943e.

@TheBB
Copy link
Contributor

TheBB commented Oct 24, 2015

(advice-remove 'helm-ff-sort-candidates 'helm-flx-helm-ff-sort-candidates)

Works around it for now.

@PythonNut
Copy link
Contributor

@cpaulik @TheBB should be fixed now, sorry.

PythonNut -- tuhdo is the a ninja with helm -- he knows what he's saying/doing :D

@robbyoconnor no of course. Everyone knows tuhdo is a ninja. However, the process for getting code merged into helm can be very... interesting.

@TheBB
Copy link
Contributor

TheBB commented Oct 24, 2015

Thanks!

However, the process for getting code merged into helm can be very... interesting.

Oh, I believe you. :-/

@PythonNut
Copy link
Contributor

You guys may be interested in disabling helm-find-files support until the bugs (flx optimizations, namely) are worked out.

@TheBB
Copy link
Contributor

TheBB commented Oct 24, 2015

Appreciate it.

Related commit: e776ada

The update hasn't reached MELPA yet but it'll catch on when it does.

@ghost
Copy link

ghost commented Oct 25, 2015

However, the process for getting code merged into helm can be very... interesting.

I just learned this the hard way when trying to fix the problem from #3327 (comment).

After spending a couple of days addressing his concerns I asked him if there was anything else that needed to be done. His reply was that although he hasn't really had time to look at the code, it still has (unspecified) "errors", that he suspects performance is bad (It's fine, he didn't actually try it), and that he really thinks a better way to fix the problem would be... to implement exactly what I described, explained, and implemented. Sorry, life's too short. I tried...

@PythonNut
Copy link
Contributor

@ghost would you like to push the changes to helm-flx in the form of advice?

@EphramPerdition
Copy link

@PythonNut, thanks, It's a package now (Please Ignore my shifting username):

https://github.com/EphramPerdition/helm-fuzzier

The README recommends it be used together with helm-flx.

I'd like a few folks to try it out and report whether it breaks something for them before pushing it to MELPA.

helm-fuzzier together with helm-flx finally makes Helm do what I mean.

@TheBB
Copy link
Contributor

TheBB commented Oct 26, 2015

@EphramPerdition Thanks, I'm trying it out. So far so good.

@EphramPerdition
Copy link

Great, "works for me" is just as useful as a bug report at this point.

@PythonNut
Copy link
Contributor

@EphramPerdition

The README recommends it be used together with helm-flx.

Is there any case where you would use helm-fuzzier without helm-flx?

@EphramPerdition
Copy link

@PythonNut, you can use it without helm-flx, sure. The results should be much better then vanilla helm, even without helm-flx.

Note that each of these packages touches a different part of helm. helm-fuzzier improves matching and helm-flx improves the scoring. You might conceivably want to swap one and not the other for something you like better.

@PythonNut
Copy link
Contributor

As of lewang/flx#78, flx, should now have substantially improved asymptotic complexity.

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

No branches or pull requests