-
Notifications
You must be signed in to change notification settings - Fork 393
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
Take advantage of CodeActionLiteral client capability #2962
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
"src/java/Foo.java", | ||
"public class Foo {\n"+ | ||
" void foo() {\n"+ | ||
"String s = \"some str\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested kak-lsp on this and it does work. Thanks so much!
There is an interesting issue which I'll try to fix soon in kak-lsp
:
jdtls
sends a code action containing both an edit
and a command
.
LSP says
/**
- If a code action provides an edit and a command, first the edit is
- executed and then the command.
*/
Which would be wrong here because the edit
and command
do the same thing, but they are not idempotent.
However jdtls
does not include java.apply.workspaceEdit
in the methods it supports in workspace/executeCommand
(executeCommandProvider
).
I guess kak-lsp
should recognize this and not send java.apply.workspaceEdit
, thus avoiding the error.
Maybe I'll add a warning to the logs. I don't think LSP specifies what should happen in this case.
Here is the code action sent by jdtls
:
{
"jsonrpc": "2.0",
"id": 11,
"result": [
{
"title": "Insert missing quote",
"kind": "quickfix",
"diagnostics": [
{
"range": {
"start": {
"line": 5,
"character": 15
},
"end": {
"line": 5,
"character": 24
}
},
"severity": 1,
"code": "1610612995",
"source": "Java",
"message": "String literal is not properly closed by a double-quote"
}
],
"edit": {
"changes": {},
"documentChanges": [
{
"textDocument": {
"version": null,
"uri": "file:///home/johannes/git/kak-lsp/t/java/test.java"
},
"edits": [
{
"range": {
"start": {
"line": 5,
"character": 24
},
"end": {
"line": 5,
"character": 24
}
},
"newText": "\""
}
]
}
]
},
"command": {
"title": "Insert missing quote",
"command": "java.apply.workspaceEdit",
"arguments": [
{
"changes": {},
"documentChanges": [
{
"textDocument": {
"version": null,
"uri": "file:///home/johannes/git/kak-lsp/t/java/test.java"
},
"edits": [
{
"range": {
"start": {
"line": 5,
"character": 24
},
"end": {
"line": 5,
"character": 24
}
},
"newText": "\""
}
]
}
]
}
]
}
},
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a bunch of other places where jdtls
requires executeCommand
of java.apply.workspaceEdit
(for example the "add JavaDoc" code action.
When I add a hack to kak-lsp to support this command (via workspace/applyEdit
) that means that the code action from the earlier comment will add two quotes instead of just one.
Can we convert those other places to also use codeAction.edit
instead of only the java.apply.workspaceEdit
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to make "Generate toString()" work with this patch but not yet the "Add Javadoc" one.
diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/ClientPreferences.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/ClientPreferences.java
index 5bfd515a..66852dd0 100644
--- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/ClientPreferences.java
+++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/ClientPreferences.java
@@ -482,6 +482,12 @@ public class ClientPreferences {
&& capabilities.getWorkspace().getWorkspaceEdit().getChangeAnnotationSupport() != null;
}
+ public boolean isWorkspaceEditSupport() {
+ return v3supported
+ && capabilities.getWorkspace() != null
+ && capabilities.getWorkspace().getWorkspaceEdit() != null;
+ }
+
public boolean skipProjectConfiguration() {
return Boolean.parseBoolean(extendedClientCapabilities.getOrDefault("skipProjectConfiguration", "false").toString());
}
diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java
index c4b10686..ddbb4ba9 100644
--- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java
+++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java
@@ -704,8 +704,12 @@ public class SourceAssistProcessor {
if (preferenceManager.getClientPreferences().isSupportedCodeActionKind(kind)) {
CodeAction codeAction = new CodeAction(name);
codeAction.setKind(kind);
- codeAction.setCommand(command);
- codeAction.setData(new CodeActionData(null, priority));
+ if (preferenceManager.getClientPreferences().isWorkspaceEditSupport()) {
+ codeAction.setEdit(edit);
+ } else {
+ codeAction.setCommand(command);
+ codeAction.setData(new CodeActionData(null, priority));
+ }
codeAction.setDiagnostics(context.getDiagnostics());
return Optional.of(Either.forRight(codeAction));
} else {
@@ -301,7 +301,12 @@ private Optional<Either<Command, CodeAction>> getCodeActionFromProposal(String u | |||
codeAction.setData(new CodeActionData(proposal, -proposal.getRelevance())); | |||
} else { | |||
codeAction.setCommand(command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if edit != null
this line means that we still include the nonstandard command.
By kak-lsp default shows an error in this case (see my other comment).
Can we delete this line so it's not sent in this case?
Hi, is this PR still active? It would be great to get this working... |
Unless I'm missing something obvious, I think I agree with this change, and it looks completely safe to me. There might be a few places where we need to fix it up. First of all, this change wouldn't affect vscode-java client at all. In SourceAssistProcessor and CodeActionHandler, this change attempts to change behaviour when The change sets the workspace edit of a code action where |
c93f930
to
9515f73
Compare
If client advertises `CodeActionLiteralSupport`, using that for `java.apply.workspaceEdit` would allow clients to use a generic algorithm, instead of being forced to provide a special case for jdt.ls. Fixes eclipse-jdtls#376 Signed-off-by: Boris Staletic <boris.staletic@gmail.com> Signed-off-by: Anton Romanov <theli.ua@gmail.com> Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
9515f73
to
b06c7c6
Compare
If client advertises
CodeActionLiteralSupport
, using that forjava.apply.workspaceEdit
would allow clients to use a generic algorithm, instead of being forced to provide a special case for jdt.ls.Fixes #376
This is essentially a rebase of 1278
Superseeds #2688
Since the original reason for revert is now addressed by #1657