Organize hover responses in a predictable way for ease of use

[turkeylurkey]

I have a file called microprofile-config.properties and it contains

injected.value2=Injected value2
value=lookup value

com.example.demo.client.Service/mp-rest/url=http://localhost:9081/data/client/service
serviceb.url=http://localhost:9081/data/protected

When I hover over the word "Service" I can get two different results in the IDE. Two different language servers respond and it seems random which result is displayed first. Sometimes an empty page is displayed as page 1 and other content on page 2.


Sometimes the page with all the content is first and the blank one is second.

I think the responses should be sorted in some deterministic way so the user can rely on seeing things a certain way. Even if it is as simple as alphabetical order by name of language server it should be consistent over a reasonable period of time. Obviously as language servers start and stop the order could change but it should be consistent while a certain collection of LS are running.

Also, one language server responded with

[Trace - 16:48:07] Received response 'textDocument/hover - (9)' in 6ms.
Result: {
  "contents": {
    "kind": "plaintext",
    "value": ""
  }
}

Fixed I think we should check value for null or empty string and not even return a message in that case.

[angelozerr]

I think we should check value for null or empty string and not even return a message in that case.

It should work like this by using

lsp4ij/src/main/java/com/redhat/devtools/lsp4ij/features/documentation/LSPDocumentationHelper.java

Line 59 in c521ff9

if (hover == null) {

and there is a test about this usecase

lsp4ij/src/test/java/com/redhat/devtools/lsp4ij/features/documentation/TypeScriptHoverTest.java

Line 115 in c521ff9

public void testNoHoverWithInvalidMarkupContent() {

Are you sure the empty hover comes from an another language server? You mean that your Jakarta LS contribuet to microprofile-config.properties?

Coud youshare your 2 LSP traces withe the two hover.

[aparnamichael]

@angelozerr

These are the traces from the language servers.

Liberty config LS:

[Trace - 15:17:10] Sending request 'textDocument/hover - (13)'.
Params: {
"textDocument": {
"uri": "file:///Users/aparnacmichael/Documents/Devex/Codebase/liberty-tools-intellij/src/test/resources/projects/gradle/sampleGradleMPLSApp/src/main/resources/META-INF/microprofile-config.properties"
},
"position": {
"line": 5,
"character": 9
}
}

[Trace - 15:17:10] Received response 'textDocument/hover - (13)' in 1ms.
Result: {
"contents": {
"kind": "plaintext",
"value": ""
}
}

Microprofile LS:

[Trace - 15:17:10] Sending request 'textDocument/hover - (21)'.
Params: {
"textDocument": {
"uri": "file:///Users/aparnacmichael/Documents/Devex/Codebase/liberty-tools-intellij/src/test/resources/projects/gradle/sampleGradleMPLSApp/src/main/resources/META-INF/microprofile-config.properties"
},
"position": {
"line": 5,
"character": 9
}
}

[Trace - 15:17:10] Received response 'textDocument/hover - (21)' in 2ms.
Result: {
"contents": {
"kind": "markdown",
"value": "io.openliberty.mp.sample.client.Service/mp-rest/url\n\nThe base URL to use for this service, the equivalent of the baseUrl method.\r\nThis property (or */mp-rest/uri) is considered required, however implementations may have other ways to define these URLs/URIs.\n\n * Type: java.lang.String\n * Value: http://localhost:9081/data/client/service"
},
"range": {
"start": {
"line": 5,
"character": 0
},
"end": {
"line": 5,
"character": 51
}
}
}

Video for your reference.

LSPconsole.Rsponse.mov

[angelozerr]

Thanks @aparnamichael for your answer.

I don't know when I will have time to study this issue since now I'm very busy with our IJ Quarkus to create a release as soon as some bug fixes will be done.

[turkeylurkey]

It looks like this line should catch it.

lsp4ij/src/main/java/com/redhat/devtools/lsp4ij/features/documentation/LSPDocumentationHelper.java

Line 76 in c521ff9

if (!isValidContent(value)) {

The test you refer to is for "kind": "markdown", so it is not applicable in this case. Your plaintext test case has a non-empty string.

[angelozerr]

Any PR with test are welcome!

[turkeylurkey]

I think your tests are unit tests and don't hit LSPDocumentationHelper.

Also, we have been using 06/15 and I believe the code from 06/18 fixes the problem with the empty message.

This means we don't have a use case that shows multiple messages for a hover situation but if we did the messages would be in a random order. So the first part of this issue is still in play.

[angelozerr]

If I understand correctly your comment if there are several ls which provide non empty hover, LSP4IJ need to sort them.

[turkeylurkey]

That's right, the user will be more comfortable seeing them in a predictable order.

[angelozerr]

It means that we need to provide an extension point to sort.

I think we should wait for having a concrete usecase to implement that

[turkeylurkey]

That sounds like a good idea but I'd hate for it to become so complex that the basic function is not implemented.

That said, we no longer have a use case so this is not high priority for us.

[angelozerr]

An another simple idea is to sort hover by server name.

That said, we no longer have a use case so this is not high priority for us.

Indeed let's waiting for a concrete usecase to discuss again about that.

[angelozerr]

@turkeylurkey what about sorting hover results by language server display name?