Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix autocomplete #2212

Merged
merged 4 commits into from
Oct 15, 2018
Merged

Fix autocomplete #2212

merged 4 commits into from
Oct 15, 2018

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Oct 11, 2018

This rewrites quite a lot of QueryMatcher.

  • Remove FuzzyMatcher which was a whole file of commented out code that just deferred to QueryMatcher
  • Simplify & remove some cruft from QueryMatcher, eg. most of the KeyMap stuff was completely unused.
  • Don't rely on object iteration order, which fixes a bug where users whose display names were entirely numeric would always appear first...
  • Add options.funcs to QueryMatcher to allow for indexing by things other than keys on the objects
  • Use above to index users by username minus the leading '@'
  • Don't include the '@' in the query when autocomple is triggered by typing '@'.
  • Add unit tests!

Fixes element-hq/element-web#6782

This has been commented out for ages. Just remove it and make things
use QueryMatcher directly rather than looking like they do fuzzy matching
but not.
This rewrites quite a lot of QueryMatcher.
 * Remove FuzzyMatcher which was a whole file of commented out code
   that just deferred to QueryMatcher
 * Simplify & remove some cruft from QueryMatcher, eg. most of the
   KeyMap stuff was completely unused.
 * Don't rely on object iteration order, which fixes a bug where
   users whose display names were entirely numeric would always
   appear first...
 * Add options.funcs to QueryMatcher to allow for indexing by things
   other than keys on the objects
 * Use above to index users by username minus the leading '@'
 * Don't include the '@' in the query when autocomple is triggered
   by typing '@'.

Fixes element-hq/element-web#6782
Which 1) has a fairly complex interface with lots of subtleties and
2) is really trivial to unit test.
@dbkr dbkr requested a review from a team October 11, 2018 20:07
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

other than the question about localparts, I think this is sane.

this.matcher = new FuzzyMatcher([], {
keys: ['name', 'userId'],
this.matcher = new QueryMatcher([], {
keys: ['name'],
Copy link
Member

Choose a reason for hiding this comment

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

do we not want to keep userId here?

Copy link
Member

Choose a reason for hiding this comment

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

or something to match the localpart, at least

Copy link
Member Author

Choose a reason for hiding this comment

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

It's done below with a function to chop off the leading '@'

Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

Just tried it out, works beautifully. Happy this is finally fixed!

@dbkr dbkr merged commit bd35825 into develop Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@-mention doesn't autocomplete display names
3 participants