atom / fuzzy-finder

Find and open files quickly
MIT License
275 stars 138 forks source link

Regression in highlight with alternate scoring. #296

Closed jeancroy closed 7 years ago

jeancroy commented 7 years ago

Since about atom 1.1, the alternate scoring option is provided that decide both how to rank and highlight the result.

On current version, alternate scoring option only decide ranking, and it seems highlight is always rendered using classic fuzzaldrin. I think it's important to have both together as I consider highlight an explanation of current ranking.

It's very probable the behavior broke when switching the underlying component to remove jquery. It's hard to detect because there's no test for it and one must be in knowledge of the algorithms to spot the differences.


Here's a recent example, typical of classic fuzzaldrin.

One can see that the word member is split because classic fuzzaldrin select first occurence of m then first occurence of e following that m then first m then ber

img

Here's a reference highlight with fuzz-aldrin-plus outside of atom. One can see that the algorithm try to group matches together instead of processing greedy left to right.

img

Now that algorithm isn't perfect and here we have an issue with -payment being before plain edit. But the overall highlight seems more intuitive.

The reason I write this is that 'scattered letters' was by far the main complains with classic fuzzaldrin and complains stated to reappears.

50Wliu commented 7 years ago

https://github.com/atom/fuzzy-finder/blob/e69a9e23cca40127f93b60e30ab7491df21dd9d3/lib/fuzzy-finder-view.js#L314

jeancroy commented 7 years ago

Ok the problem is a bit complicated because there's code in place to use the proper matcher.

https://github.com/atom/fuzzy-finder/blob/4cecf236ba85c11342df35abbf7611194f448343/lib/fuzzy-finder-view.js#L59-L61

However I can replicate

v1.15

screenshot_1

v1.17 (1.18beta looks the same)

screenshot_3

Both have alternate scoring turned on. Maybe something broke the propagation of settings in 1.17 ?

50Wliu commented 7 years ago

Looks like a regression with the jQuery removal then. Thanks for catching this.

/cc @as-cii