Fix for PHPCodeHints invocation failure scenarios #14692
Conversation
| if (hasIgnoreCharacters || (isExplicitInvokation && !trimmedQuery)) { | ||
| filteredHints = filterWithQueryAndMatcher(res, ""); | ||
| } else { | ||
| filteredHints = filterWithQueryAndMatcher(res, self.query); |
There was a problem hiding this comment.
do we need to filter hints. isn't server itself sending filtered hints based on the query??
There was a problem hiding this comment.
Filtering here is required for creating stringRanges, and matchGoodness, which are then used to highlight the matched text and sort the things accordingly as per the query.
There was a problem hiding this comment.
if the purpose is to create stringRanges, and matchGoodness, then why this function removes some of hints from hintlist retured from server??
There was a problem hiding this comment.
Can we make change here that none of the hints returned from server should be removed?
If we can do this then we will not have to worry about ignoreCharacters list
There was a problem hiding this comment.
I am not sure I understand. The characters in ignoreList are from the query, not the hints. In essence, I am actually doing the same thing you have suggested. The query is being saved on the Brackets side for a session. So when a user types "se" the results are then filtered by calculating the distance of the hint from the subsequence "se". The results are then sorted by the distance. When a user types something like "->" which is not part of the hint but still invokes hints, I am just not using "->" to filter out the hints, rather preserving the order of the hints as they come.
There was a problem hiding this comment.
@shubhsnov
I feel same thing can be done without maintaining an ignore characters list in codeHintsProvider
make change in filterWithQueryAndMatcher function.
Pass a boolean parameter(dontRemoveIfNoMatch) to this function. if this third parameter is true none of the hints should be removed even if hints has no match with query.
call this "filterWithQueryAndMatcher" for server hints with dontRemoveIfNoMatch as true.
and call this function separately for static hints with dontRemoveIfNoMatch as false which is default.
lets think if we can do this?
There was a problem hiding this comment.
We'll need the list to determine if we have to ignore the query length in insertHint. Using the list to maintain consistency across the session.
There was a problem hiding this comment.
@niteskum filterWithQueryAndMatcher implementation shouldn't be changed as there are multiple extensions using this fn. This is more generic implementation which shouldn't be altered for code hint listing use cases.
| @@ -46,6 +46,7 @@ define(function (require, exports, module) { | |||
| function CodeHintsProvider(client) { | |||
| this.client = client; | |||
| this.query = ""; | |||
| this.ignoreQuery = ["-", "->", ">", ":", "::", "(", "()", ")", "[", "[]", "]", "{", "{}", "}"]; | |||
There was a problem hiding this comment.
Since you have added this list to the defaultprovider, the ignorequery list will be different for different languages. why should this be here?
There was a problem hiding this comment.
I could have made it specific to PHP but the CodeHintProvider for PHP was using the insertHint from the DefaultProvider, and we needed to know if any query needed to be ignored there as well.
Secondly, this was something that would affect most languages. For PHP I could have stopped at "->" and ":" but then decided to take a more generic approach by including the braces as well, as these things need to be ignored by most languages.
There was a problem hiding this comment.
IMO, - inclusion in ignore list would create problems. - is a valid token character for a lot of languages. HTML5 attributes, CSS property names and values, where we need filtering. Rest all characters should be safe to ignore from query. However this can be revisited when we try to add more hint providers in future.
Fix for class-based invocation "::"
Fix for object-based invocation "->"
Fix for Explicit invocation "Ctrl-Space"
Fix for insertHints in the above scenarios
ping @niteskum @narayani28 @swmitra for review.