Return end position of candidate in match function

[ptrv]

The function should return the position of the end of the candidate that
matches prefix. This fixes font-locking in company popup.

[ptrv]

@abingham What about this?

[abingham]

Sorry, I thought I had posted a comment to this earlier and was waiting for a response. It looks like I forgot!

My basic question was what the observable effect of this was. I remember that when I wrote the code originally, I couldn't really tell what the return value of this function did. I ended up interpreting the match documentation in company.el as asking for an absolute position, but your change seems to be returning an offset relative to the start of the match.

I've merged your change, but I'm still curious: what observable effect can I see that shows what this value does?

[ptrv]

Basically the popup had wrong font-locking. Since the function returned the absolute position in the buffer, everything was highlighted. The function should instead only highlight the the prefix.

Notice selection on 6th entry:

Wrong (with using (point)):

Correct (with using (length prefix)):

[abingham]

OK, thanks!

[dgutov]

Currently match is used to highlight the "longest common substring" in all candidates, or the generalization of that notion, when non-prefix matching is used (the part of the candidate until the last matching character).

Since the backend doesn't seem to return this info to you, and you do use non-prefix matching, you should probably avoid responding to match. Then the default logic will be used (i.e. try-completion).

[abingham]

@dgutov Thanks for the explanation. If what you're saying is true, you should consider updating the company-backends documentation. Right now it says "Backends that use [non-prefix matching] must disable cache (return t to no-cache') and should also respond tomatch'." This makes it sound like a match response is effectively mandatory for non-prefix matching. A better explanation of what match does might be helpful, too.

@ptrv Can you verify that not responding to match does the right thing?

[ptrv]

@abingham Not responding to match highlights the longest common substring which is correct

[abingham]

OK, then I guess we should just remove that. I'll do that in a bit.

[ptrv]

Wait, I thought you meant when no-cache is nil. When no-cache is t, displaying the longest common substring does not work because its a fuzzy matching and often there is no common substring

[ptrv]

With using (length prefix) the whole candidate is highlighted which is ok IMHO

[abingham]

Ah, sorry for the misunderstanding. Just so I'm clear: everything worked fine before I committed c234050, right? If so, I'll undo that change.

[ptrv]

Yes, it was fine

[dgutov]

This is wrong.

1. Why would the whole candidate be highlighted? With this implementation, (length prefix) numbers of characters will be highlighted at the beginning of each candidate, which doesn't really fit prefix or non-prefix matching.
2. If the current set of candidates are proper prefix matches (either because of the setting, or just by luck), the longest common substring might well be longer than the prefix.

[ptrv]

In the case of company-ycmd with the prefix argument we get the whole candidate (without function args) that's why the whole candidate is highlighted without the function args. With removing the function we get the number of characters of the prefix highlighted, which does not make much sense IMHO because ycmd returns candidates based on fuzzy matching.

[ptrv]

Not responding to the match function leads to this:

[ptrv]

Using (length prefix):

[dgutov]

Ah, so the name of the argument should be different (not prefix, but candidate). Sorry for misunderstanding.

Not responding to the match function leads to this:

I wonder how that's possible. For instance, if I comment out the match case in company-capf, and the current input results in non-prefix completion (when using that backend), no characters are highlighted, instead of what you have on the screenshot.

Guess I'll have to install ycmd to check.

[ptrv]

Yes, we should definitely rename the argument

[ptrv]

@abingham What do you think about the two versions of highlighting?

[abingham]

Between the two options you just posted, the second one using (length prefix) is obviously better. The first one is meaningless.

To be honest, the highlighting is almost a non-issue for me personally. I don't want it to behave nonsensically, and I'd probably be happy not highlight anything at all. I just want the completion candidates. Knowing how my input matched the candidates doesn't help me, though I'm sensitive to the fact that it might help some users.

I guess a sort of ultimate solution would:

1. highlight just the matching segments of a fuzzy match
2. highlight just matching prefix in a prefix match

AKAIK we can't accomplish (1) for a few reasons:

1. ycmd doesn't tell us how it matched, so...
2. ...we'd have to duplicate it's logic. Even doing that...
3. ...company doesn't provide a way to highlight just the matching segments.

If the argument passed to the match function is the candidate and not the prefix, then I'm also not entirely sure how we'd accomplish (2).

So...hmmm...I'm mostly just thinking out loud now. Let me know if I've got anything wrong!

The bottom line is that I'm satisfied with any reasonable solution; the first option listed above doesn't meet that requirement. If it helps, I'm happy with the current solution.

[dgutov]

we'd have to duplicate it's logic

That would be bad. Better not highlight at all in non-prefix case, I think.

company doesn't provide a way to highlight just the matching segments

I can add that capability, no problem. So far, though, simply highlighting the part of the string up until the end of the match worked well enough.

If the argument passed to the match function is the candidate and not the prefix, then I'm also not entirely sure how we'd accomplish (2)

Several ways: use company-prefix, which saves the current prefix. Or I can pass an additional argument to match. But see above.

If it helps, I'm happy with the current solution.

Like mentioned, I find it surprising that omitting the match handler doesn't do TRT.

[dgutov]

omitting the match handler doesn't do TRT

Should be fixed now, it was my bug, and it also affected the behavior of company-complete-common. I couldn't reproduce it previously only because company-capf by default doesn't show non-prefix completions when it can find prefix ones. Sorry about that.

When the next Company version is released, you should be able to go back to ignoring match.

[ptrv]

Great, thanks for your work!
On Nov 23, 2014 4:46 AM, "Dmitry Gutov" notifications@github.com wrote:

omitting the match handler doesn't do TRT

Should be fixed now, it was my bug, and it had some ugly consequences in
the behavior of company-complete-common. I couldn't reproduce it
previously only because company-capf by default doesn't show non-prefix
completions when it can find prefix ones.

When the next Company version is released, you should be able to go back
to ignoring match.

—
Reply to this email directly or view it on GitHub
#92 (comment).

[ptrv]

I can confirm, that with ignoring match, the longest common substring is correctly highlighted and when show result from a fuzzy match there is not highlighting, which is also a correct behavior.

Commit c234050 can be brought back

[dgutov]

@abingham

If what you're saying is true, you should consider updating the company-backends documentation. Right now it says "Backends that use [non-prefix matching] must disable cache (return t to no-cache') and should also respond tomatch'." This makes it sound like a match response is effectively mandatory for non-prefix matching. A better explanation of what match does might be helpful, too.

Also done, I think. But if the description of match is still unclear, please suggest an improvement.

[abingham]

The description of match and its relationship to non-prefix matching is
much clearer now. Thanks!

On Mon Nov 24 2014 at 4:28:23 AM Dmitry Gutov notifications@github.com
wrote:

@abingham https://github.com/abingham

If what you're saying is true, you should consider updating the
company-backends documentation. Right now it says "Backends that use
[non-prefix matching] must disable cache (return t to no-cache') and should
also respond tomatch'." This makes it sound like a match response is
effectively mandatory for non-prefix matching. A better explanation of what
match does might be helpful, too.

Also done, I think. But if the description of match is still unclear,
please suggest an improvement.

—
Reply to this email directly or view it on GitHub
#92 (comment).