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

Lazily resolve completion text edit according to the preference #2600

Merged
merged 2 commits into from
Apr 23, 2023

Conversation

jdneo
Copy link
Contributor

@jdneo jdneo commented Apr 18, 2023

  • JDT.LS will only resolve the text edit for the snippets only when
    the preference 'java.completion.lazyResolveTextEdit.enabled' is set
    to true.
  • If not, the 'textEdit' of the snippets will be set during
    'textDocument/completion'.

requires redhat-developer/vscode-java#3072

fix #2584

- JDT.LS will only resolve the text edit for the snippets only when
  the preference 'java.completion.lazyResolveTextEdit.enabled' is set
  to true.
- If not, the 'textEdit' of the snippets will be set during
  'textDocument/completion'.

Signed-off-by: Sheng Chen <sheche@microsoft.com>
@mickaelistria
Copy link
Contributor

In LSP4E (which may not be perfect), I see the issue the other way round with this patch: the LS return {"jsonrpc":"2.0","id":"51","result":{"label":"syserr","kind":15,"detail":"print to standard err","documentation":{"kind":"markdown","value":"```java\nSystem.err.println();\n```"},"insertTextFormat":2,"textEdit":{"range":{"start":{"line":12,"character":2},"end":{"line":12,"character":2}},"newText":"System.err.println(${0});"},"command":{"title":"","command":"java.completion.onDidSelect","arguments":["3","1"]}}} to resolve request, and this result in the situation shown in this video
Screencast from 2023-04-18 11-04-18.webm

@jdneo
Copy link
Contributor Author

jdneo commented Apr 18, 2023

So is the text edit lazy resolve turned on or off in you case?

I cannot understand why the range of the textedit in your attached response is zero.

@mickaelistria
Copy link
Contributor

Here are the full capabilities, the interesting part is "resolveSupport":{"properties":["documentation","detail","additionalTextEdits"]}

{"jsonrpc":"2.0","id":"1","method":"initialize","params":{"processId":813393,"rootPath":"/home/mistria/sandbox/eclipse-workspace/blah/","rootUri":"file:///home/mistria/sandbox/eclipse-workspace/blah/","initializationOptions":{"extendedClientCapabilities":{"classFileContentsSupport":true,"skipProjectConfiguration":true,"excludedMarkerTypes":["org.eclipse.lsp4e.diagnostic"],"skipTextEventPropagation":true},"settings":{"java":{"implementationsCodeLens":{"enabled":false},"project":{"resourceFilters":""},"referencesCodeLens":{"enabled":false}}}},"capabilities":{"workspace":{"applyEdit":true,"workspaceEdit":{"documentChanges":true,"resourceOperations":["create","delete","rename"],"failureHandling":"undo"},"symbol":{"dynamicRegistration":true},"executeCommand":{"dynamicRegistration":true},"workspaceFolders":true,"configuration":true},"textDocument":{"synchronization":{"willSave":true,"willSaveWaitUntil":true,"didSave":true},"completion":{"completionItem":{"snippetSupport":true,"documentationFormat":["markdown","plaintext"],"resolveSupport":{"properties":["documentation","detail","additionalTextEdits"]},"insertTextModeSupport":{"valueSet":[1,2]}}},"hover":{"contentFormat":["markdown","plaintext"]},"signatureHelp":{},"references":{},"documentHighlight":{},"documentSymbol":{"symbolKind":{"valueSet":[18,17,5,14,9,10,22,24,8,1,12,11,20,6,2,3,21,16,19,25,4,7,15,23,26,13]},"hierarchicalDocumentSymbolSupport":true},"formatting":{"dynamicRegistration":true},"rangeFormatting":{},"definition":{"linkSupport":true},"typeDefinition":{"linkSupport":true},"codeAction":{"codeActionLiteralSupport":{"codeActionKind":{"valueSet":["quickfix","refactor","refactor.extract","refactor.inline","refactor.rewrite","source","source.organizeImports"]}},"dataSupport":true,"resolveSupport":{"properties":["edit"]},"dynamicRegistration":true},"codeLens":{},"documentLink":{},"colorProvider":{},"rename":{"prepareSupport":true},"foldingRange":{},"selectionRange":{},"inlayHint":{}},"window":{"workDoneProgress":true,"showMessage":{},"showDocument":{"support":true}}},"clientInfo":{"name":"Eclipse Platform","version":"0.16.1.qualifier"},"trace":"off","workspaceFolders":[{"uri":"file:///home/mistria/sandbox/eclipse-workspace/blah/","name":"blah"},{"uri":"file:///home/mistria/fosdem/workspace-demo/hello/","name":"hello"},{"uri":"file:///home/mistria/runtime-Test-EclipesWithJDTLS/languageServers-log/","name":"languageServers-log"},{"uri":"file:///home/mistria/git/lsp4e/org.eclipse.lsp4e.debug/","name":"org.eclipse.lsp4e.debug"},{"uri":"file:///home/mistria/git/lsp4e/org.eclipse.lsp4e.test/","name":"org.eclipse.lsp4e.test"}]}}

I cannot understand why the range of the textedit in your attached response is zero.

We call resolve relatively early, when selecting the completion item (at this stage s is already typed and the relevant didChange was already sent to the LS, maybe that text edit is somehow ignored?); then we resolve it and retrieve the text edit, we keep typing and then we apply it (LSP4E takes care of updating its internal copy of the text edit as we modify the code. Should we re-resolve before applying to ensure we get the best result from the server ?

@jdneo jdneo changed the title Resolve text edit according to the 'resolveSupport' Lazily resolve completion text edit according to the preference Apr 18, 2023
@jdneo
Copy link
Contributor Author

jdneo commented Apr 18, 2023

Oh, sorry I forgot to update the title and description.

According to the suggestion: redhat-developer/vscode-java#3072 (comment), this PR has changed to use a preference to control the behavior.

By default the setting is false at the server side. I assume in your case, the text edit is not lazy resolved, so it will calculated in the request textDocument/completion. And the request completionItem/resolve will not update the textEdit.

Not quite familiar with the Eclipse side implementation. In VS Code, following things will happen during completion:

  1. User types s.
  2. didChange notification send to server, server updates the compilation unit's buffer.
  3. textDocument/completion sends to server, server returned the completion items.
  4. completionItem/resolve sends to server, if text edit lazy resolving is turned off, server will not update it.
  5. User keeps typing ys.... In this case, VS Code will do the text filtering (correct the range according to what user has typed) at the client side according to the content of textEdit and apply it to the document.

See here, the range of the text edit is determined by token start and token end. Why it becomes zero? 🤔

@mickaelistria
Copy link
Contributor

Thank you for the details, I'll try to investigate it more on eclipseide-jdtls end and will come back later with some more details. In the meantime, the new behavior we see is not really a regression, it's a different bug of the same serverity; so it would be fine if this PR gets merged.

Signed-off-by: Sheng Chen <sheche@microsoft.com>
Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@testforstephen testforstephen merged commit 96e1185 into eclipse-jdtls:master Apr 23, 2023
@jdneo jdneo deleted the cs/textEditResolveFlag branch April 24, 2023 02:26
@mickaelistria
Copy link
Contributor

The remaining -and unrelated- issue I see with completion of snippets in eclipseide-jdtls is now tracked in #2627

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.

Use a flag to control whether the textEdit of completion will be calculated in resolve request
3 participants