Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support documentSelector in registerCapability #566

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

angelozerr
Copy link
Contributor

feat: Support documentSelector in registerCapability

Fixes #517

@angelozerr
Copy link
Contributor Author

To test this PR:

  • use IntelliJ Quarkus from the master
  • open an application.properties and click on a property key. You should see some highlight (and LSP console textDocument/documentHighlight)
  • open a Java file and click somewhere, you should not see some LSP textDocument/documentHighlight request

@angelozerr
Copy link
Contributor Author

@fbricon if you have time to review it that's nice, otherwise I will merge it tomorrow (after writting some tests).

@angelozerr angelozerr force-pushed the documentSelector branch 2 times, most recently from a232f5d to 9975813 Compare October 7, 2024 20:12
@mrglavas
Copy link
Contributor

mrglavas commented Oct 7, 2024

I was looking at this PR earlier but didn't immediately make the connection to redhat-developer/intellij-quarkus#1394. I have the same question about whether this introduces behavioral changes and whether it can be done in a way that doesn't impact existing adopters.

I'm also curious if LSPIJUtils is intended to be part of the API. I noticed a public method is being deleted in this PR. We use other methods from this class in Liberty Tools.

@angelozerr
Copy link
Contributor Author

I was looking at this PR earlier but didn't immediately make the connection to redhat-developer/intellij-quarkus#1394. I have the same question about whether this introduces behavioral changes and whether it can be done in a way that doesn't impact existing adopters.

I gave you an answer, but it is a bug from intellij quarkus and liberty tools.

Developping an hack just to keep the highlight for property in microprofile config. Properties is very annoyig.

I'm also curious if LSPIJUtils is intended to be part of the API. I noticed a public method is being deleted in this PR. We use other methods from this class in Liberty Tools.

Yes I know but this class is very big and I am trying to clean up.

The methods that I have removed is just to create LSP hover params that adaptors should never used

@angelozerr
Copy link
Contributor Author

I was looking at this PR earlier but didn't immediately make the connection to redhat-developer/intellij-quarkus#1394. I have the same question about whether this introduces behavioral changes and whether it can be done in a way that doesn't impact existing adopters.

After some discussion with @fbricon I manage this case. When languageId is not defined in the mapping and if there is a documentSelector which defines a language, we consider that documentSelector returns true.

It means that it should work with your Liberty Tools without doing some changes. But I suggest that you do this change to avoid sending LSP textDocument/documentHighlight when you click on a java file editor.

@angelozerr angelozerr merged commit 5f2d875 into redhat-developer:main Oct 8, 2024
6 checks passed
@angelozerr
Copy link
Contributor Author

angelozerr commented Oct 8, 2024

@mrglavas I merged the PR and creates a nighty build, please test it to check your highlight continue to work.

Thanks.

@mrglavas
Copy link
Contributor

mrglavas commented Oct 8, 2024

@mrglavas I merged the PR and creates a nighty build, please test it to check your highlight continue to work.

@angelozerr Thanks for addressing this issue. Does the latest nightly build 0.7.0-20241008-125202 contain this PR?

@mrglavas
Copy link
Contributor

mrglavas commented Oct 8, 2024

It means that it should work with your Liberty Tools without doing some changes. But I suggest that you do this change to avoid sending LSP textDocument/documentHighlight when you click on a java file editor.

I plan on opening an issue in our project to synch up with the change made in Quarkus Tools.

@angelozerr
Copy link
Contributor Author

@angelozerr Thanks for addressing this issue. Does the latest nightly build 0.7.0-20241008-125202 contain this PR?

Yes!

@angelozerr
Copy link
Contributor Author

I plan on opening an issue in our project to synch up with the change made in Quarkus Tools.

Without the change that I did in IJ Quarkus, it should work too, so please test if highlight in microprofile-config.properties is working for you.

@mrglavas
Copy link
Contributor

mrglavas commented Oct 8, 2024

It seems to be working the same as before. I captured some of the trace and see the request and response for textDocument/documentHighlight.

[Trace - 09:40:40] Sending request 'textDocument/documentHighlight - (11)'.
Params: {
  "textDocument": {
    "uri": "file:///C:/libertyToolsGit19/jax-rs-app/src/main/resources/META-INF/microprofile-config.properties"
  },
  "position": {
    "line": 0,
    "character": 10
  }
}

[Trace - 09:40:40] Received response 'textDocument/documentHighlight - (11)' in 8ms.
Result: [
  {
    "range": {
      "start": {
        "line": 0,
        "character": 0
      },
      "end": {
        "line": 0,
        "character": 22
      }
    },
    "kind": 3
  }
]

@angelozerr
Copy link
Contributor Author

It seems to be working the same as before. I captured some of the trace and see the request and response for textDocument/documentHighlight.

Nice! The thing which should change is how the textDocument/documentHighlight is registered. It is registered with registerCapability:

You should see that in your LSP console:

[Trace - 15:57:57] Received response 'initialize - (1)' in 1110ms.
Result: {
  "capabilities": {
    "textDocumentSync": 2,
    "hoverProvider": false,
    "definitionProvider": false,
    "documentSymbolProvider": false,
    "workspaceSymbolProvider": true,
    "documentFormattingProvider": false,
    "documentRangeFormattingProvider": false,
    "inlayHintProvider": false
  }
}

all static capability should set to false.

And after that you should see that:

[Trace - 14:49:03] Received request 'client/registerCapability - (16)'
Params: {
  "registrations": [
    {
      "id": "c3e43cc7-5043-4648-80e4-77b82b71215c",
      "method": "textDocument/documentHighlight",
      "registerOptions": {
        "documentSelector": [
          {
            "language": "microprofile-properties"
          }
        ]
      }
    }
  ]
}

Right?

@mrglavas
Copy link
Contributor

mrglavas commented Oct 8, 2024

Yes, this looks the same. Thanks.

[Trace - 09:37:41] Received response 'initialize - (1)' in 1441ms.
Result: {
  "capabilities": {
    "textDocumentSync": 2,
    "hoverProvider": false,
    "definitionProvider": false,
    "documentSymbolProvider": false,
    "workspaceSymbolProvider": true,
    "documentFormattingProvider": false,
    "documentRangeFormattingProvider": false,
    "inlayHintProvider": false
  }
}

[Trace - 09:37:42] Received request 'client/registerCapability - (9)'
Params: {
  "registrations": [
    {
      "id": "cb64e8f3-8fda-4fa0-ae7e-78d75a90c4dc",
      "method": "textDocument/documentHighlight",
      "registerOptions": {
        "documentSelector": [
          {
            "language": "microprofile-properties"
          }
        ]
      }
    }
  ]
}

@angelozerr
Copy link
Contributor Author

And if you do the same thing inside Java file, you should see some [Trace - 09:40:40] Sending request 'textDocument/documentHighlight - (11)'.

which returns nothing.

If you do the same fix than I did for IJ Quarkus with languageId, this trace should disappear only for Java files.

Right?

@mrglavas
Copy link
Contributor

mrglavas commented Oct 8, 2024

And if you do the same thing inside Java file, you should see some [Trace - 09:40:40] Sending request 'textDocument/documentHighlight - (11)'.

which returns nothing.

If you do the same fix than I did for IJ Quarkus with languageId, this trace should disappear only for Java files.

Right?

Yes, that's exactly what I'm seeing. Thanks.

[Trace - 09:40:55] Sending request 'textDocument/documentHighlight - (16)'.
Params: {
  "textDocument": {
    "uri": "file:///C:/libertyToolsGit19/jax-rs-app/src/main/java/mrglavas/CircleResource.java"
  },
  "position": {
    "line": 64,
    "character": 27
  }
}

[Trace - 09:40:55] Received response 'textDocument/documentHighlight - (16)' in 4ms.
No response returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support documentSelector in registerCapability
2 participants