New api - port hover

[ahmedneilhussain]

Based on code originally submitted in #333 - this ports LSPHover to use the new API. Note that there is no timeout code in the revised logic, because there is no intermediate future exposed [the call to getLanguageServer is now rolled up into internal logic], but this should be fine as the consuming code in getHoverRegion() incorporates the timeout.

The API already excludes any nulls from the returned list, so that post-processing step is no longer needed here either.

Have tested it manually using the Corrosion plugin as well.

[mickaelistria]

What about the timeout?

[ahmedneilhussain]

What about the timeout?

I mentioned this above - I think this is covered by the calling code that has a timeout for when it ultimately tries to dereference the hover.

https://github.com/eclipse/lsp4e/blob/5a75b544c083ab6b89ef11851dc07d2fcbe556ed/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/hover/LSPTextHover.java#L177

Is there a need to have an intermediate stage timeout?

[rubenporras]

Suggested change

	.withFilter(capabilities -> LSPEclipseUtils.hasCapability(capabilities.getHoverProvider()))
	.withCapabilities(ServerCapabilities::getHoverProvider)

[rubenporras]

Now that I look only at this small consumer, It looks like it makes to have a method withCapabilities that would allow to write code as simple as in my suggested replacement. Would that be possible?

[rubenporras]

Yes, something like

	public E withCapability(final @NonNull Function<ServerCapabilities, Either<Boolean, ? extends Object>> serverCapabilities) {
		this.filter = f -> LSPEclipseUtils.hasCapability(serverCapabilities.apply(f));
		return (E)this;
	}

in LanguageServers should work.

[ahmedneilhussain]

No objection from me. Anything that pushes the boilerplate into common code and makes the calling code more focused on the problem domain is nice. Shall I go ahead and commit your suggestion?

[rubenporras]

Sure, I am happy if you just add it as part of this small PR, if it gets bigger, we could merge it standalone

[ahmedneilhussain]

Implemented

[angelozerr]

Please keep in mind that it should really nice to support cancel checker. For instance with LemMinx Maven, hover can take some time because compute is complex. When user hover something, it should cancel all previous CompletableFuture server.getTextDocumentService().hover(...) and restart the process.

This cancel should be done before this.request = LanguageServers.forDocument(document)

I spend some times to try to support this cancel by trying to use CompletableFuture API (without success). The only solution that I found is to collect in a list all call of server.getTextDocumentService().hover(..) and loop for the list to cancel it.

See my draft with documentLink but it is the same thing for hover #238

If you find a better idea with my LSPRequest wrapper, you are welcome!

[rubenporras]

Yes, I agree, supporting a cancel checker is a must, at least for a mature product. Since we are introducing now a new API, I think it makes sense to see it can accommodate the case.

[rubenporras]

Hi @ahmedneilhussain , I think this PR would not prevent handling cancelation as done in #238. One can still wrap the request into something that can be cancelled once a newer request is initiated, right? If so we can merge this one without preventing cancellation handling, and then I would merge this PR independent of the cancellation support which does not exist yet.

In can also refactor it later to add some withCapability method to LanguageServers

[rubenporras]

why do we need to change "HoverContents2" with null? I do not understand this part.

[ahmedneilhussain]

I put it as part of the first commit (which got rid of the superfluous call to shutdown the dispatch threads) as it's maintenance not really part of this main change. I realised the test is wrong, or rather incomplete - testCollectAllExcludesNulls() should be making sure that any null response from a language server gets filtered out of the returned list. The test (owing to some cut-and-paste errors and a brain fade on my part) wasn't ensuring that properly. If you look at the mock it can never generate 'HoverContent2' it is supposed to be generating null on the second call, and the test should explicitly be testing for the null getting filtered out.

I just realised this in the course of the main change - I had made some errors in earlier drafts of this work ensuring that the null semantics were preserved when code was rewritten to the new API, so I checked this carefully and spotted the flawed test.

Can split this fix out if you think it is still unclear - I kept it as a separate commit to emphasise it was an unrelated change.

[rubenporras]

I am done with the review, I would have approved it if I would understand why we need to change the test, but I do not. Besides, something with the PR is off, because the Legal Agreement check has failed. Could you fix that and other comments (if appropriate)?

[ahmedneilhussain]

Thanks @rubenporras it seems that my personal email (which is not associated with my account and license declaration) gets used when I commit your suggestion from within the PR. I will rebase to correct the error and also implement your other suggestion.