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

[autocomplete] prioritize the user for which the nickname STARTS with the text typed #2231

Closed
bernard-ng opened this issue Sep 16, 2020 · 14 comments

Comments

@bernard-ng
Copy link
Contributor

Describe the bug
If you start typing the at-character ("@") followed by letters, the Converse UI will automatically try to auto-complete with the nickname of an occupant of the room.
For example, when I type "@ ber" I get this:

converse-bug

One thing that would be an improvement, is if Converse would prioritize the user for which the nickname STARTS with the text that I typed
You see in the example above that both Bernard's name as well as Viktor's match
If I would have pressed 'tab' after typing "@ ber" then it would have auto-completed with Viktor's name, instead of Bernard's
I think 99% of the times, a user intends to select the user who's nickname starts with whatever is typed.

To Reproduce
type a text starting with "@"

Expected behavior
prioritize the user for which the nickname STARTS with the text that is typed

Environment (please complete the following information):

  • Desktop
  • Browser [Chrome]
  • Converse.js version [6.0.1]
@guusdk
Copy link
Member

guusdk commented Sep 16, 2020

@Zash noted that the autocomplete behavior can be controlled with the muc_mention_autocomplete_filter configuration option. The default for this is contains but starts_with can also be used.

However, it would be preferable to prioritize nicknames that start with the provided input, when the contains option is used. That currently is not the case.

@bernard-ng
Copy link
Contributor Author

By analyzing the code, here are the different filters available:

export const FILTER_CONTAINS = function (text, input) {
return RegExp(helpers.regExpEscape(input.trim()), "i").test(text);
};
export const FILTER_STARTSWITH = function (text, input) {
return RegExp("^" + helpers.regExpEscape(input.trim()), "i").test(text);
};

What do you think of a solution that adds a third filter, "starts_or_contains" ?

@guusdk
Copy link
Member

guusdk commented Sep 16, 2020

What do you think of a solution that adds a third filter, "starts_or_contains" ?

I do not think that's the right approach. starts_or_contains would have the same results (something that 'starts' with a particular substring also 'contains' that substring).

Instead, I think you should modify the order of results that are returned with FILTER_CONTAINS.

@jcbrand
Copy link
Member

jcbrand commented Sep 16, 2020

The AutoComplete class has a sort configuration attribute (see here) and sorts with a function called SORT_BY_LENGTH.

You'll need to update AutoComplete to accept a function for the sort config parameter (and not just a boolean as currently), and then you need to update the MUC code that uses AutoComplete to pass in your own sort function that sorts according to whether the matches start with the string or not.

@bernard-ng
Copy link
Contributor Author

The sort function only takes two parameters, the current and the next element, but I'm supposed to reorganize the results according to the text typed by the user.

An idea on how I can have the text typed in a sort function without changing the signature of the function so that it remains compatible ?

@jcbrand
Copy link
Member

jcbrand commented Sep 17, 2020

The things being compared here are instances of the Suggestion class.

That class already has a value attribute which is the suggested match. What's still needed is the query string (the string being auto-completed). If you have that, then you have all the information necessary.

I've made a change to do that here: a72ad8a

So now you have the text typed by the user as the query attribute on the Suggestion instance.

@bernard-ng
Copy link
Contributor Author

Here is what I’ve done #2234, it works as expected on my machine but it seems to break some unit tests, how can I fix that ?

@jcbrand
Copy link
Member

jcbrand commented Sep 18, 2020

You'll have to identify the failing tests and then run them individually to figure out why they're failing and then fix the tests or the underlying issue in the code.

I've written some documentation on how to run the tests:

https://github.com/conversejs/converse.js/blob/master/docs/source/testing.rst

@bernard-ng
Copy link
Contributor Author

tell me if I'm wrong, the auto-completion for contacts uses the "contains" filter but it looks like the results are not sorted,
what made the test can not pass, I saw the contact list available in the mock :

converse.js/spec/mock.js

Lines 541 to 557 in fff9eea

mock.current_contacts_map = {
'Mercutio': ['Colleagues', 'friends & acquaintences'],
'Juliet Capulet': ['friends & acquaintences'],
'Lady Montague': ['Colleagues', 'Family'],
'Lord Montague': ['Family'],
'Friar Laurence': ['friends & acquaintences'],
'Tybalt': ['friends & acquaintences'],
'Lady Capulet': ['ænemies'],
'Benviolo': ['friends & acquaintences'],
'Balthasar': ['Colleagues'],
'Peter': ['Colleagues'],
'Abram': ['Colleagues'],
'Sampson': ['Colleagues'],
'Gregory': ['friends & acquaintences'],
'Potpan': [],
'Friar John': []
};

Tybalt, comes before Balthasar.

how do I activate the filter, when it comes to contacts?
my little change fixes the test but not totally, I can't activate the sorting function I created.

@guusdk
Copy link
Member

guusdk commented Sep 23, 2020

Can you please rephrase your question? We've disucced it in the chatroom, but couldn't understand what you're trying to say.

@bernard-ng
Copy link
Contributor Author

bernard-ng commented Sep 23, 2020

is it the same Autocomplete class located in the src/converse-autocomplete.js file that makes the following two examples work ?

au-completion in a chatroom
chatroom

au-completion for contact
Sans titre (1)

@jcbrand
Copy link
Member

jcbrand commented Sep 24, 2020

@bernard-ng Yes, AFAIK they both use the <converse-autocomplete> component, and that component makes a new instance of AutoComplete every time it's created.

See here:

this.auto_complete = new AutoComplete(this.firstElementChild, {

@bernard-ng
Copy link
Contributor Author

alright in this case I don't have anything more to do on my side, is there anything to modify before merging ?

@guusdk
Copy link
Member

guusdk commented Sep 24, 2020

Like I suggested in #2234 (comment) please remove all of the unneeded commits that are in the PR, to clean things up.

@jcbrand jcbrand closed this as completed in efd4e50 Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants