JDT completions coming from LS application fixed

[BoykoAlex]

No description provided.

[martinlippert]

I tested this locally and this fix breaks #881 (after applying such a proposal, the linked editing mode doesn't come up). So this would need to change https://github.com/eclipse/lsp4e/blob/master/org.eclipse.lsp4e.jdt/src/org/eclipse/lsp4e/jdt/LSJavaCompletionProposalComputer.java#L87 so that it creates the LSJavaProposalExtension2 in your fix.

But I am wondering why LSJavaProposal can't implement all the necessary extension interfaces, much like LSCompletionProposal (the actual delegate) instead of having different subclasses for each?

[BoykoAlex]

It might wrap org.eclipse.lsp4e.operations.codeactions.CodeActionCompletionProposal which does not implement any completion extension interfaces. The apply method for such proposal needs only the document as a parameter.
Now extension 1 and 2 have an alternative "apply" method that takes a number of different parameters (viewr, doc, offset, trigger chat etc.). Looking at org.eclipse.jface.text.contentassist.CompletionProposalPopup line 997 suggests that we need wrappers for each extension that has diferent apply logic.
The issue is that code action comes as CodeActionCompletionProposal which when wrapped suddenly implements extension 2 and applied differently. We could keep LSJavaProposal only without the extensions and have its apply method with cases like in CompletionProposalPopup line 997, i.e getting doc from the viewer though seems a bit risky as doc comes from the context rather than viewer in the proposal popup logic...

[BoykoAlex]

I've added the missing bit in the latest commit

[BoykoAlex]

@mickaelistria am I okay to merge this once @martinlippert approves?

[martinlippert]

Looks good to me for the moment, all my tests were successful. I am still not convinced that the different wrapper implementations are really necessary here, so would like to review these changes in a future step to dive into the details here a bit. But first, let's merge this to move forward here.

[martinlippert]

@mickaelistria am I okay to merge this once @martinlippert approves?

Based on your previous comment @mickaelistria , I decided to merge this after successful testing on my side. Hope that is fine with you... :-)