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

Add support for using multiple language servers to be registered for the same files #4609 #5442

Merged
merged 20 commits into from
Jul 10, 2017

Conversation

tsmaeder
Copy link
Contributor

@tsmaeder tsmaeder commented Jun 22, 2017

What does this PR do?

This PR allows starting multiple language servers per file. The results for various calls are merged according to https://code.visualstudio.com/docs/extensionAPI/vscode-api.
Opening an editor blocks until all necessary editors have started. The merged server capabilities for all the applicable servers are computed whenever an editor is opened. One copy of a language server is started per project.

This PR is based on #5156, which is not merged yet.

What issues does this PR fix or reference?

#4609

Changelog

Added support for using multiple language servers to be registered for the same files.

Release Notes

The support of the Language Server Protocol is getting better and better in Eclipse Che. Two main aspects have been improved: the ability to register a language server for a specific filename and the ability to support multiple language servers for a single file.

Until now, it was only possible to register a language server for a file extension like *.xml, but now it is possible to register a language server for a specific filename like pom.xml. The improvements also allow for multiple extensions/or and filenames to be mapped to a single mime type and language id. The language registration and language server launcher registration have also been separated. You can learn more into the following documentation.

This version also allows multiple language servers to be used for a single file. When opening a file in an editor, the language servers registered for that file are initialized and started if needed. The results coming from the various language servers are computed together.

You can see a sample demo here in a python file, where Python language server and another demo language server are both registered and working together.

27958182-1fe7b30e-6322-11e7-9ecc-fef98ff2c04d

Those fundamental improvements on how language servers are supported in Che are going to provide more flexibility for contributors and more capabilities for users. As an example, it is now possible to provide support for a pom.xml file, where a Maven language server and an XML language server are both registered and operating together.

Docs PR

eclipse-che/che-docs#256

@codenvy-ci
Copy link

Can one of the admins verify this patch?

@tsmaeder tsmaeder changed the title WIP: 4609 multiple ls 4609 multiple ls Jun 27, 2017
@@ -70,6 +71,7 @@ protected void addAnnotation(final Annotation annotation, final Position positio
try {
addPosition(position);
} catch (BadLocationException ignore) {
Log.error(getClass(), "BadLocation for "+annotation);

Choose a reason for hiding this comment

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

If I'm not mistaken this line is not properly formatted

@slemeur slemeur added the kind/enhancement A feature request - must adhere to the feature request template. label Jun 27, 2017
@@ -12,7 +12,7 @@

import com.google.gwt.user.client.rpc.AsyncCallback;
import com.google.gwt.user.client.ui.Widget;

import org.eclipse.che.api.languageserver.shared.dto.DtoClientImpls.CodeActionParamsDto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you use here implementations instead of interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lsp4j does not use interfaces

@@ -22,18 +22,18 @@
* @param diagnostic
* Diagnostic - The discovered diagnostic.
*/
void acceptDiagnostic(Diagnostic diagnostic);
void acceptDiagnostic(String diagnosticsCollection, Diagnostic diagnostic);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add description what diagnosticsCollection parameter mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, not clear

@slemeur
Copy link
Contributor

slemeur commented Jun 27, 2017

Nice @tsmaeder !

This is really good, we need to write a nice release note paragraph for that enhancement!
I think that we should record a small demo for that. Would you be able to record an animated gif? It would perfectly illustrate the use case and the new capability.

I'll provide the release note summary.

Thanks

@slemeur slemeur self-requested a review June 27, 2017 16:44
@benoitf benoitf changed the title 4609 multiple ls Added support for using multiple language servers to be registered for the same files #4609 Jun 29, 2017
@benoitf benoitf changed the title Added support for using multiple language servers to be registered for the same files #4609 Add support for using multiple language servers to be registered for the same files #4609 Jun 29, 2017
@slemeur
Copy link
Contributor

slemeur commented Jul 3, 2017

Hi @tsmaeder. Could you record a small animated gif that we will include in the release note to demo the new capability? (Don't forget to zoom-in on your browser so that it will be visible to the reader);

Thanks in advance

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. team/californication labels Jul 4, 2017
@tsmaeder
Copy link
Contributor Author

tsmaeder commented Jul 5, 2017

@benoitf this change should not be visible to the user until we start USING multiple language servers/file. Does it make sense to put it in the release notes?

@benoitf
Copy link
Contributor

benoitf commented Jul 5, 2017

@tsmaeder are you sure the question is for me or it's for @slemeur asking for a gif demo ?

@slemeur
Copy link
Contributor

slemeur commented Jul 5, 2017

@tsmaeder : This issue + #5156 are adding new capability to the system - and they are pretty excellent ! I'll handle the release note section but its goal will not be on the end-user benefits, it will focus on explaining the new capability added to the system and what it will allow to do in the future - this could be interesting for any Che contributors.

When we will leverage the capability, like pom.xml file for example, we will focus the explanations on the what kind of features the user will get.

The animated gif might not be necessary, but as you did a demo during the sprint demo, I thought it could nicely illustrate the explanations.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Jul 7, 2017

demo

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Jul 7, 2017

Longer demo video available here: https://youtu.be/-G3WlGskIvs

Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
@slemeur
Copy link
Contributor

slemeur commented Jul 7, 2017

Thanks a lot @tsmaeder !

Copy link
Contributor

@slemeur slemeur left a comment

Choose a reason for hiding this comment

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

👍

Signed-off-by: Thomas Mäder <tmader@redhat.com>
@benoitf
Copy link
Contributor

benoitf commented Jul 10, 2017

ci-build

@tsmaeder tsmaeder merged commit 9f88048 into eclipse-che:master Jul 10, 2017
@tsmaeder tsmaeder removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 10, 2017
@tsmaeder tsmaeder added this to the 5.15.0 milestone Jul 10, 2017
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
…the same files eclipse-che#4609 (eclipse-che#5442)

Add support for using multiple language servers for the same files.

Signed-off-by: Thomas Mäder <tmader@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants