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

Better fuzzy search #379

Merged
merged 5 commits into from Feb 11, 2016
Merged

Better fuzzy search #379

merged 5 commits into from Feb 11, 2016

Conversation

saveman71
Copy link
Collaborator

So I was motivated to solve #282 in a better way than #361 (not a Levenshtein distance, but the real thing like in sublime-text, as requested in the issue).

Sorry @nmitsou ^_^'

It looks like this for now:
screenshot from 2016-02-01 23-15-52

Comparaison with #361:

  • Highlighting of the matched text
  • Maybe faster
  • Cannot auto-correct typos (clogk -> clock)

This PR also adds support for multi-part highlighting.

Here is how the score is calculated:

  • A positive match is given the base score of 20
  • 30 points are given gradually with the percentage of characters matched
  • 50 points are given gradually with the percentage of word starting characters matched (uppercase, or preceded by a whitespace). This allows queries that contain the beginning of a word to have a much better ranking.
  • => This totals to a score going from 20 to 100
  • The more fragmented the matches are, the less the result is significant (see this line for the actual formula).

Can someone test it and provide some feedback, especially on the ranking of the apps?

/cc @Neamar @nmitsou @emersion

@saveman71
Copy link
Collaborator Author

Note to self: aliases having a relevance of 10, they always are pushed back irrelevant results.

@nmitsou
Copy link
Collaborator

nmitsou commented Feb 4, 2016

Yes I like it. Much better than #361.
Ordering between apps seemed fine to me.
Contacts were also higher ranked than apps which is good

@nmitsou nmitsou mentioned this pull request Feb 4, 2016
@Neamar
Copy link
Owner

Neamar commented Feb 7, 2016

Thanks @saveman71 for this piece of content!
I'll add it to my own KISS and test it for a few days, since I don't want to rush any decisions regarding the ordering ;)

}

// If we are at the beginning of a word, add it to matchedWordStarts
if (Character.isUpperCase(pojo.name.charAt(appPos)) || appPos == 0 || Character.isWhitespace(pojo.name.charAt(appPos - 1)))
Copy link
Owner

Choose a reason for hiding this comment

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

OQ: do we want to enforce a capital letter for a first word?

Copy link
Owner

Choose a reason for hiding this comment

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

OQ: if appPos == 0, it may be worth boosting the app relevance too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OQ: do we want to enforce a capital letter for a first word?

There is no need to, appPos == 0 is there to handle that.

OQ: if appPos == 0, it may be worth boosting the app relevance too?

Well if appPos == 0 is true, we already add 1 to matchedWordStarts. Imo this is not necessary because it will allow something like Barcode Scanner to have the same score if we write Bar or Sca, and I think is a good thing.

Copy link
Owner

Choose a reason for hiding this comment

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

Works for me.

@saveman71
Copy link
Collaborator Author

Done /cc @Neamar

I removed the base score alltogether, should not pose any problem and I can't remember why it was there :(

I'll see with actual usage data if there is something to adjust.

@ntninja ntninja mentioned this pull request Feb 9, 2016
@Neamar
Copy link
Owner

Neamar commented Feb 10, 2016

Been using it for a few days, seems good to me!

Neamar added a commit that referenced this pull request Feb 11, 2016
@Neamar Neamar merged commit 41aaec1 into master Feb 11, 2016
@Neamar Neamar deleted the fuzzysearch branch February 11, 2016 20:39
@nmitsou
Copy link
Collaborator

nmitsou commented Feb 14, 2016

Hi @saveman71 and @Neamar ,
The recent crash on #387 must be related to this commit. Maybe the problem comes when the normalized app name has less characters than the original app name ?

The appPos variable in the for loop (AppProvider:38) ends on pojo.nameNormalized.length but inside the loop the counter has access to the pojo.name variable.

If pojo.name.length != pojo.nameNormalized.length then you get OutOfBounds

@saveman71
Copy link
Collaborator Author

Okay so this happens only if you have (at least) one app with some special characters in it, which I didn't have.
One solution would be to make Pojo.mapPosition() public so we can use it in AppProvider.
Another quick fix would be to stop using pojo.name alltogether, but we loose the ability to match an uppercase letter inside a word (e.g. "appProvider": we can't make a better score with the "P").

What could be done is to release the quickfix, and maybe sort out what can be done to fix the issue?

@nmitsou
Copy link
Collaborator

nmitsou commented Feb 14, 2016

Yes, quickfix seems a good idea !

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

Successfully merging this pull request may close these issues.

None yet

3 participants