No completion for FQN constructors #2152

snjeza · 2022-07-06T20:47:10Z

Signed-off-by: Snjezana Peco [email protected]

rgrunber · 2022-07-07T20:57:11Z

It definitely works. I'm just not sure if we should be accepting this without also fixing the duplicate constructor issue in JDT Core.

rgrunber · 2022-07-08T18:40:54Z

Ok, so this is basically the same principle as redhat-developer/vscode-java#2538 ? The language server is doing the right thing and returning all the constructor invocations that match java.util.Arr from the context of List<String> list = new java.util.Arr|, but the client ignores them because (I'm assuming) java.util.Arr can never match a completion item label like ArrayList(int initialCapacity). So this PR modifies the label to ArrayList(int capacity) - java.util.ArrayList to ensure the match can happen in the client.

~~Is it safe to say we have no control over this "filtering logic" that happens after we return the set of constructors ?~~
~~Do other clients even suffer from this ?~~
~~I assume it would be possible to do this change on the client side (eg. vscode-java) by using completion provider ? Maybe adding to the completion item data to contain the package name ?~~

Update : I noticed the constructors don't define a filterText in their completion items. Can this workaround be simplified just by supplying a proper filterText ?

/**
* A string that should be used when filtering a set of
* completion items. When `falsy` the label is used as the
* filter text for this item.
*/
filterText?: string;

Update 2 : item.setFilterText(typeInfo.toString()); seems to be working for me.

snjeza · 2022-07-08T19:27:34Z

Update 2 : item.setFilterText(typeInfo.toString()); seems to be working for me.

@rgrunber I have updated the PR. Please review it.

rgrunber · 2022-07-08T20:11:48Z

Awesome. Is there any downside to adding the typeInfo as filterText to all constructor invocations (not just when they're qualified with package name) ? It should simplify the logic quite a bit.

snjeza · 2022-07-08T20:29:41Z

Awesome. Is there any downside to adding the typeInfo as filterText to all constructor invocations (not just when they're qualified with package name) ? It should simplify the logic quite a bit.

~~checking...~~
It works fine.

snjeza · 2022-07-08T21:12:15Z

@rgrunber I have updated the PR.

rgrunber · 2022-07-09T01:34:47Z

@testforstephen , let me know if you're ok with with merging this. The only downside is it would expose eclipse-jdt/eclipse.jdt.core#195 until it gets fixed.

testforstephen · 2022-07-11T14:08:54Z

I see some side effects with current change.

The filter text has impact on ranking. Since we use FQN as the filter text for the constructor proposal, it will result in the filter matching less efficient for non-FQN constructors. See the case below, the expected proposal new ArrayList<>() is displayed after anonymous proposal, that's not good. It might be better to restrict the new approach to the FQN constructor only.
Typing () after the method name, the completion list disappeared.
Current:

Expected:

We had discussion on how to use filterText for method proposal before, see Why is java.signatureHelp disabled by default? redhat-developer/vscode-java#2063 (comment). If we only put the method name in the filter text, then it's better to enable signature help by default to assist the parameter names.

rgrunber · 2022-07-11T15:42:38Z

@snjeza initially had a change to only do the filterText for the FQN-constuctor case, but I asked to have it removed for ease of maintenance. It could be restored to do as you wanted.

Do we need to abandon the filterText entirely since it breaks completing within the parameter names ? Do we have to restore the label hack ?

rgrunber

Seems fixed to me. @testforstephen , can you confirm ? The duplicate issue would be fixed after we update the target platform.

...rc/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalDescriptionProvider.java

snjeza · 2022-07-14T01:40:43Z

The duplicate issue would be fixed after we update the target platform.

A related PR - #2158

testforstephen · 2022-07-14T13:11:24Z

...rc/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalDescriptionProvider.java

+						ICompilationUnit unit = JDTUtils.resolveCompilationUnit(uri);
+						if (unit != null) {
+							IDocument document = JsonRpcHelpers.toDocument(unit.getBuffer());
+							String prefix = document.get(requiredProposal.getReplaceStart(), requiredProposal.getReplaceEnd() - requiredProposal.getReplaceStart());
+							if (prefix != null && prefix.indexOf(".") > -1) {


The new commit works well for the two cases I mentioned previously. One additional concern is performance.

The profiling shows the operation JDTUtils.resolveCompliationUnit is not cheap. For all constructor completion proposals, the completion context prefix should be the same, can we calculate it only once and reuse it?

snjeza · 2022-07-14T19:58:45Z

@rgrunber @testforstephen I have updated the PR.

testforstephen · 2022-07-15T05:53:23Z

...rc/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalDescriptionProvider.java

+				Object data = item.getData();
+				if (data instanceof Map) {
+					String uri = ((Map<String, String>) data).get(CompletionResolveHandler.DATA_FIELD_URI);


The latest commit works better.

A minor comment is to remove the unnecessary lines 344 ~ Line 346.

Signed-off-by: Snjezana Peco <[email protected]>

snjeza · 2022-07-15T19:46:50Z

@rgrunber @testforstephen I have updated the PR.

testforstephen

LGTM

snjeza requested review from gorkem, fbricon, rgrunber, jdneo and testforstephen July 6, 2022 20:47

snjeza mentioned this pull request Jul 6, 2022

[content assist] Duplicate constructor completion eclipse-jdt/eclipse.jdt.core#195

Closed

snjeza force-pushed the issue-2147 branch from 995e73a to ed4e04e Compare July 8, 2022 19:26

snjeza force-pushed the issue-2147 branch from ed4e04e to 5ae3aa6 Compare July 8, 2022 21:11

snjeza added the in progress label Jul 11, 2022

snjeza force-pushed the issue-2147 branch from 5ae3aa6 to f82fbff Compare July 11, 2022 18:43

snjeza removed the in progress label Jul 11, 2022

rgrunber reviewed Jul 13, 2022

View reviewed changes

...rc/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalDescriptionProvider.java Outdated Show resolved Hide resolved

snjeza force-pushed the issue-2147 branch from f82fbff to 8123aa1 Compare July 14, 2022 00:27

testforstephen reviewed Jul 14, 2022

View reviewed changes

snjeza force-pushed the issue-2147 branch from 8123aa1 to 671f397 Compare July 14, 2022 19:57

testforstephen reviewed Jul 15, 2022

View reviewed changes

No completion for FQN constructors

f5fde80

Signed-off-by: Snjezana Peco <[email protected]>

snjeza force-pushed the issue-2147 branch from 671f397 to f5fde80 Compare July 15, 2022 19:45

testforstephen approved these changes Jul 18, 2022

View reviewed changes

snjeza merged commit 82e71bb into eclipse-jdtls:master Jul 18, 2022

testforstephen mentioned this pull request Jul 19, 2022

Add a compatible constructor to CompletionProposalDescriptionProvider to avoid breaking Java Debugger #2165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No completion for FQN constructors #2152

No completion for FQN constructors #2152

snjeza commented Jul 6, 2022

rgrunber commented Jul 7, 2022

rgrunber commented Jul 8, 2022 •

edited

Loading

snjeza commented Jul 8, 2022

rgrunber commented Jul 8, 2022

snjeza commented Jul 8, 2022 •

edited

Loading

snjeza commented Jul 8, 2022

rgrunber commented Jul 9, 2022

testforstephen commented Jul 11, 2022

rgrunber commented Jul 11, 2022

rgrunber left a comment

snjeza commented Jul 14, 2022

testforstephen Jul 14, 2022

snjeza Jul 14, 2022

snjeza commented Jul 14, 2022

testforstephen Jul 15, 2022

snjeza Jul 15, 2022

snjeza commented Jul 15, 2022

testforstephen left a comment

No completion for FQN constructors #2152

No completion for FQN constructors #2152

Conversation

snjeza commented Jul 6, 2022

rgrunber commented Jul 7, 2022

rgrunber commented Jul 8, 2022 • edited Loading

snjeza commented Jul 8, 2022

rgrunber commented Jul 8, 2022

snjeza commented Jul 8, 2022 • edited Loading

snjeza commented Jul 8, 2022

rgrunber commented Jul 9, 2022

testforstephen commented Jul 11, 2022

rgrunber commented Jul 11, 2022

rgrunber left a comment

Choose a reason for hiding this comment

snjeza commented Jul 14, 2022

testforstephen Jul 14, 2022

Choose a reason for hiding this comment

snjeza Jul 14, 2022

Choose a reason for hiding this comment

snjeza commented Jul 14, 2022

testforstephen Jul 15, 2022

Choose a reason for hiding this comment

snjeza Jul 15, 2022

Choose a reason for hiding this comment

snjeza commented Jul 15, 2022

testforstephen left a comment

Choose a reason for hiding this comment

rgrunber commented Jul 8, 2022 •

edited

Loading

snjeza commented Jul 8, 2022 •

edited

Loading