Code hinting should sort by scope

[dangoor]

Steps to reproduce:

1. Open StringMatch.js
2. Put the cursor in StringMatcher.prototype.match (I used the top of the function)
3. Type "st"

Expected:

• str on top followed by stringMatch and StringMatcher.

Actual:

• str on top, followed by String followed by stringMatch and StringMatcher

Globals and keywords should always be lower than the others.

[dangoor]

I'm going to label this a medium priority, because I think the ordering is very important for code hints (because it determines how much you need to type or how many times you need to hit the arrow key)

[dloverin]

Scope is a secondary sort. The primary sort is matchGoodness. What I'm seeing is "str" and "String" have the same matchGoodness (-279). "stringMatch" has a matchGoodness of -278 and "StringMatcher" is -277. If "String", "stringMatch" and "StringMatcher" all had the same matchGoodness score then "String" would be sorted below them.

Does StringMatcher has a mode where all prefix matches are considered equal?

[dangoor]

No... I think the difference there is the length penalty.

My comment in the review for the initial pull request was that scope could either be the primary sort and matchGoodness secondary, matchGoodness can be boosted based on scope. Boosting matchGoodness is probably better because the user can type something that more clearly points to a global but might also happen to match something closer in scope.

The trick with boosting matchGoodness would be figuring out the right boost. It's always a bit of guesswork because it's trying to be psychic about what the user intends.

One possibility would be something like 25 points for each scope level. So, if there were 4 levels, 75 points for innermost, 50 for module level, 25 for globals, 0 for keywords.

[dloverin]

Boosting matchGoodness seems pretty tricky. I would expect all the prefix matches, including builtin globals, to be listed before matches in the middle of hints.

[dloverin]

I have a fix that in the hinter that makes prefixes matches sort to the top by setting the matchGoodness to negative Number.MAX_VALUE.

[dangoor]

I would expect all the prefix matches, including builtin globals, to be listed before matches in the middle of hints.

OK, I can buy that (and this appears to be the case with my fix in #3411), and my statement in the original description "globals and keywords should always be lower than the others" is not the right solution.

The case that I started with here, though, is one where I have prefix matches on a local variable (str), variables in the current function's closure (stringMatch, StringMatcher) and a global (String). All else being roughly equal, I'd rather see the global appear below those others.

[dangoor]

Changing this to low priority, because I think #3411 is actually the more important one than this one.

[dloverin]

The global will appear below those because my fix sets all of the prefix matches to the same score, overriding any length penalty.

[dloverin]

Sent a pull request with the fix.

[dangoor]

Oh, I see what you're saying. The results also seem good to me just doing this in Session.js in my branch for #3493:

--- a/src/extensions/default/JavaScriptCodeHints/Session.js
+++ b/src/extensions/default/JavaScriptCodeHints/Session.js
@@ -346,6 +346,7 @@ define(function (require, exports, module) {
                         searchResult.builtin = 1;
                     } else {
                         searchResult.builtin = 0;
+                        searchResult.matchGoodness -= 25;
                     }
                 }

[dloverin]

Subtracting 25 seems mysterious and heavily tied to how the matcher scores. It may work in one case but not in another. I think making prefix matches equal and letting the sort do the rest is a more robust solution.

[dangoor]

The solution presented in #3552 does not always guarantee that a prefix match will be the winner. stringMatch gives preference to matches on word boundaries (including camelCase). Here's a contrived example: stringTimeRing. If I match that with "str", the match is stringTimeRing, even though stringTimeRing is also a valid way to look at the match. Heuristics like this are imperfect, but work much better than straight up regexes from what I've seen.

To guarantee that prefix matches come out on top, It would likely be easy for me to add a flag to string match to actually check for a prefix match first and if it finds one to max out matchGoodness.

[dloverin]

I thought the fix for #3493 was going to prefer prefix matches over camelCase matches. If not, I think we really need a flag to check for prefix matches first. Could you also have the prefix match flag set all of the prefix matches to the same matchGoodness score. Then #3552 would not be needed.

[dangoor]

In most cases, #3493 will indeed prefer prefix matches, but it's not guaranteed. I think that's an improvement to the matching in general. I just pushed #3588 which adds a preferPrefixMatch option which checks for a prefix match (case insensitive check), short circuits all other matching behavior when it finds one and also sets the matchGoodness to -Number.MAX_VALUE just as you had.

My current pull request doesn't set that option in the JS code hinting. Perhaps I should just do so, since that was the reason for adding the new option.

[dangoor]

OK, I updated the pull request in #3588 to turn the option on for JS code hinting and it fixes this issue.

[dloverin]

If pull request #3588 is not ready for Sprint 24, pull request #3552 will fix this issue.

[pthiess]

Reviewed, tagged for Sprint 25

[njx]

Fixed by filer. Closing.