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.

Sign up for GitHub

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

Experimental client-side command executions #596

Merged
merged 5 commits into from
Mar 23, 2018
Merged

Experimental client-side command executions #596

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f735011
Experimental execute client-side command protocol extension
kdvolder Mar 20, 2018
3f74781
Rename `workspace/executeCommand`
kdvolder Mar 20, 2018
66aa25d
Also rename Java methods to executeClientCommand
kdvolder Mar 20, 2018
666012b
Add tests for executeClientCommand
kdvolder Mar 23, 2018
3909f03
Fix copyright headers and small test tweak
kdvolder Mar 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JavaClientConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@
import java.util.List;

import org.eclipse.jdt.ls.core.internal.handlers.LogHandler;
import org.eclipse.jdt.ls.core.internal.lsp.ExecuteCommandProposedClient;
import org.eclipse.lsp4j.ApplyWorkspaceEditParams;
import org.eclipse.lsp4j.ApplyWorkspaceEditResponse;
import org.eclipse.lsp4j.Command;
import org.eclipse.lsp4j.ExecuteCommandParams;
import org.eclipse.lsp4j.MessageActionItem;
import org.eclipse.lsp4j.MessageParams;
import org.eclipse.lsp4j.MessageType;
Expand All @@ -27,9 +29,12 @@
import org.eclipse.lsp4j.jsonrpc.services.JsonNotification;
import org.eclipse.lsp4j.services.LanguageClient;

import com.google.common.collect.ImmutableList;

public class JavaClientConnection {

public interface JavaLanguageClient extends LanguageClient {
public interface JavaLanguageClient extends LanguageClient, ExecuteCommandProposedClient {

/**
* The show message notification is sent from a server to a client to ask
* the client to display a particular message in the user interface.
Expand All @@ -56,6 +61,10 @@ public JavaClientConnection(JavaLanguageClient client) {
logHandler.install(this);
}

public Object executeClientCommand(String id, Object... params) {
return this.client.executeClientCommand(new ExecuteCommandParams(id, ImmutableList.copyOf(params))).join();
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably add a timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That raises the question... how long should the timeout be? Or should we change this api and return the CompletableFuture? Then the caller can decide how long they want to wait... or if they want to continue with an async composition instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... returning CompletableFuture seems a bit 'out of style' with the rest of the api in the same class. I called join because I as trying to follow similar pattern as other stuff in the same class. E.g. like here:

https://github.com/kdvolder/eclipse.jdt.ls/blob/66aa25d44cea30d2fdf90b30058508772b990b48/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JavaClientConnection.java#L107

}

/**
* Sends the logMessage message back to the client as a notification
* @param msg The message to send back to the client
Expand Down
23 changes: 23 additions & 0 deletions ...se.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/lsp/ExecuteCommandProposedClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*******************************************************************************
* Copyright (c) 2018 Red Hat Inc. and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat Inc. - initial API and implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

That'd be Pivotal :-)

*******************************************************************************/
package org.eclipse.jdt.ls.core.internal.lsp;

import java.util.concurrent.CompletableFuture;

import org.eclipse.lsp4j.ExecuteCommandParams;
import org.eclipse.lsp4j.jsonrpc.services.JsonRequest;

public interface ExecuteCommandProposedClient {

@JsonRequest("workspace/executeClientCommand")
CompletableFuture<Object> executeClientCommand(ExecuteCommandParams params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to wait for the client to respond?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is a good idea yes. One possible and useful scenario is an error results because the command was deregistered. If command is a kind of 'callback' as in our case, then this may be a good way to trigger cleanup of the callback table.

I've experimented with a more 'proper' cleanup. But as this tends to happen around the same time as server is in the process of shutting down... its not very reliable (messages no longer able to send etc.).

Anyhow... callers wanting to catch errors and responding to them seems to be a good enough reason. And I could also imagine that the command could return useful information (not in our use case, but if it used in a more 'request -> response fashion, rather than a callback it could be).


}