You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2020/10/21 05:56:39 UTC
[GitHub] [netbeans] JaroslavTulach commented on a change in pull request #2478: StatusDisplayer messages remoted to vscode.
JaroslavTulach commented on a change in pull request #2478:
URL: https://github.com/apache/netbeans/pull/2478#discussion_r509000593
##########
File path: java/java.lsp.server/vscode/src/extension.ts
##########
@@ -197,6 +199,9 @@ export function activate(context: ExtensionContext) {
client.start();
client.onReady().then((value) => {
commands.executeCommand('setContext', 'nbJavaLSReady', true);
+ client.onNotification(StatusMessageRequest.type, (mp) => {
Review comment:
`client.onNotification(StatusMessageRequest.type, showStatusBarMessage)` would be shorter. Shall this registration really be done in `onReady` callback, shall not it be done sooner without any callback?
##########
File path: java/java.lsp.server/vscode/src/extension.ts
##########
@@ -240,6 +245,29 @@ export function activate(context: ExtensionContext) {
}
+function showStatusBarMessage(params : ShowStatusMessageParams) {
+ let decorated : string = params.message;
+ let timeout : number = 10000;
+
+ if (params.type) {
Review comment:
Maybe you don't need the `if` check. `switch (undefined)` goes to default.
##########
File path: java/java.lsp.server/vscode/src/extension.ts
##########
@@ -240,6 +245,29 @@ export function activate(context: ExtensionContext) {
}
+function showStatusBarMessage(params : ShowStatusMessageParams) {
+ let decorated : string = params.message;
+ let timeout : number = 10000;
Review comment:
Shoudn't the timeout be read from `params.timeout`, if present?
##########
File path: java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/NbCodeLanguageClient.java
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import org.eclipse.lsp4j.MessageParams;
+import org.eclipse.lsp4j.jsonrpc.services.JsonNotification;
+import org.eclipse.lsp4j.jsonrpc.validation.NonNull;
+import org.eclipse.lsp4j.services.LanguageClient;
+
+/**
+ * An extension to the standard LanguageClient that adds several messages missing
+ * from the official LSP protocol.s
+ * @author sdedic
+ */
+public interface NbCodeLanguageClient extends LanguageClient {
+
+ /**
+ * Shows a message in the status bar. Log- and Info-type messages are shown "as is".
+ * The other message types can be decorated by an icon according to {@link MessageParams#getType}.
+ * The message will be hidden after specified number of milliseconds; 0 means the client
+ * controls when the message is hidden.
+ *
+ * @param params message type and text.
+ */
+ @JsonNotification("window/showStatusBarMessage")
+ public void showStatusBarMessage(@NonNull ShowStatusMessageParams params);
Review comment:
Not urgent, but shouldn't there be some client/server capabilities handshake to find out if this message is supported on both sides? If this message isn't supported, then send some standard message like `logMessage` or `showMessage`? @jlahoda tends to repeat that VSCode isn't the only client for this server.
##########
File path: java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java
##########
@@ -77,14 +85,56 @@ public static void launchServer(InputStream in, OutputStream out) {
Exceptions.printStackTrace(ex);
}
}
+
+ private static Launcher<NbCodeLanguageClient> createLauncher(LanguageServerImpl server, InputStream in, OutputStream out) {
+ return new LSPLauncher.Builder<NbCodeLanguageClient>()
+ .setLocalService(server)
+ .setRemoteInterface(NbCodeLanguageClient.class)
+ .setInput(in)
+ .setOutput(out)
+ .wrapMessages(new ConsumeWithLookup(server.getSessionLookup())::attachLookup)
Review comment:
`wrapMessages`! That's the trick. Cute.
##########
File path: java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ShowStatusMessageParams.java
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import org.eclipse.lsp4j.MessageParams;
+import org.eclipse.lsp4j.MessageType;
+import org.eclipse.xtext.xbase.lib.Pure;
+
+/**
+ *
+ * @author sdedic
+ */
+public class ShowStatusMessageParams extends MessageParams {
+ private int timeout = 0;
+
+ public ShowStatusMessageParams(MessageType type, String message) {
+ super(type, message);
+ }
+
+ public ShowStatusMessageParams(MessageType type, String message, int timeout) {
+ super(type, message);
+ this.timeout = timeout;
+ }
+
+ @Pure
+ public int getTimeout() {
Review comment:
The TypeScript definition says `timeout` is optional. Shouldn't this method return `Integer` and `null` if the timeout isn't set?
##########
File path: java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java
##########
@@ -185,6 +236,15 @@ public WorkspaceService getWorkspaceService() {
@Override
public void connect(LanguageClient client) {
this.client = client;
Review comment:
Wouldn't it be simpler if the type of `client` field was changed to `NbCodeLanguageClient`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists