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

Fix for PHPCodeHints invocation failure scenarios #14692

Merged
merged 1 commit into from
Apr 10, 2019
Merged

Fix for PHPCodeHints invocation failure scenarios #14692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/extensions/default/PhpTooling/CodeHintsProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,15 @@ define(function (require, exports, module) {

self.query = context.token.string.slice(0, context.pos.ch - context.token.start);
if (msgObj) {
var res = msgObj.items || [];
var res = msgObj.items || [],
trimmedQuery = self.query.trim(),
hasIgnoreCharacters = self.ignoreQuery.includes(implicitChar) || self.ignoreQuery.includes(trimmedQuery),
isExplicitInvokation = implicitChar === null;

// There is a bug in Php Language Server, Php Language Server does not provide superGlobals
// Variables as completion. so these variables are being explicity put in response objects
// below code should be removed if php server fix this bug.
if(self.query) {
if((isExplicitInvokation || trimmedQuery) && !hasIgnoreCharacters) {
for(var key in phpSuperGlobalVariables) {
res.push({
label: key,
Expand All @@ -115,7 +119,12 @@ define(function (require, exports, module) {
}
}

var filteredHints = filterWithQueryAndMatcher(res, self.query);
var filteredHints = [];
if (hasIgnoreCharacters || (isExplicitInvokation && !trimmedQuery)) {
filteredHints = filterWithQueryAndMatcher(res, "");
} else {
filteredHints = filterWithQueryAndMatcher(res, self.query);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to filter hints. isn't server itself sending filtered hints based on the query??

Copy link
Collaborator Author

@shubhsnov shubhsnov Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the purpose is to create stringRanges, and matchGoodness, then why this function removes some of hints from hintlist retured from server??

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@niteskum niteskum Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

}

StringMatch.basicMatchSort(filteredHints);
filteredHints.forEach(function (element) {
Expand Down
6 changes: 4 additions & 2 deletions src/languageTools/DefaultProviders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ define(function (require, exports, module) {
function CodeHintsProvider(client) {
this.client = client;
this.query = "";
this.ignoreQuery = ["-", "->", ">", ":", "::", "(", "()", ")", "[", "[]", "]", "{", "{}", "}"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you have added this list to the defaultprovider, the ignorequery list will be different for different languages. why should this be here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@swmitra swmitra Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

function formatTypeDataForToken($hintObj, token) {
Expand Down Expand Up @@ -160,11 +161,12 @@ define(function (require, exports, module) {
token = $hint.data("token"),
txt = null,
query = this.query,
shouldIgnoreQuery = this.ignoreQuery.includes(query),
inclusion = shouldIgnoreQuery ? "" : query,
start = {
line: cursor.line,
ch: cursor.ch - query.length
ch: cursor.ch - inclusion.length
},

end = {
line: cursor.line,
ch: cursor.ch
Expand Down