Decouple sprotty-Theia integration from LSP

eclipse-sprotty / sprotty-theia

Integration of Sprotty diagrams into Theia IDE

https://eclipse.org/sprotty

Eclipse Public License 2.0

24 stars 14 forks source link

Decouple sprotty-Theia integration from LSP #18

Closed tortmayr closed 5 years ago

tortmayr commented 6 years ago

Currently the glue code for using sprotty diagrams in Theia is tightly coupled to the language server protocol. It's not possible to use this integration code with non LSP-based sprotty servers without major modifications.

This change is a proposal to loose the coupling of the sprotty-theia integration and LSP by:

Extracting a generic TheiaSprottyConnector Interface and refactor the existing implementation to LSTheiaSprottyConnector
Extracting a generic abstract TheiaDiagramServer and refactor the existing implementation to LSTheiaDiagramServer

With this changes the glue code can be reused for all types of sprotty servers and communication protocols by implementing customized TheiaSprottyConnector and TheiaDiagramServer classes.

In particular this would enable the GLSP project to reuse this glue code without maintaining a fork.

JanKoehnlein commented 6 years ago

Sounds like a good idea in general. What do you think, @spoenemann? AFAICS, we have three scenarios

LSP based server (e.g. the Xtext examples)
Non-LSP based server (e.g. EclipseSource)
No server at all (npm view) Do you see more?

Is GLSP completely independent of LSP?

Before we start reviewing the code, could you please rebase it to master? The API has changed quite a bit (DiagramManager is provided by TheiaConnector, DiagramManagerProvider is removed).

tortmayr commented 6 years ago

Yes I agree this should be the three core scenarios. Maybe @planger can also give some input on this.

Technically the GLSP framework is completely independent from LSP but of course we have some overlap. (e.g. client-server communication is also JSON-RPC/Websocket based).

The code should already be rebased to the lastest master and the API changes should be included.

spoenemann commented 6 years ago

Sounds good. sprotty-theia should be about integrating Sprotty with Theia, which does not necessarily have to be via LSP.

I have already an example that uses the older theia-sprotty without any server component: https://github.com/TypeFox/npm-dependency-graph

planger commented 6 years ago

Thanks a lot for your fast feedback!

Yes I agree this should be the three core scenarios. Maybe @planger can also give some input on this.

No, not really. Those three main scenarios sound good to me (LSP-based, servers that are not based on LSP, and no server at all).

Is GLSP completely independent of LSP?

Right, GLSP is completely independent. It provides an own server (framework) that doesn't require any LSP-based language server running alongside it. This is actually one of the main goals we are pursuing with GLSP. In many of our use cases, we don't have an LSP-based server that has authority over the model, but rather have stand-alone models (managed by an own component) and different types of editors for different parts of the model (diagrams, forms, trees, etc.; maybe text, but not necessarily).

tortmayr commented 5 years ago

PR is now rebased on the current master. In addition, I tested the change with the xtext-theia-sprotty-example

JanKoehnlein commented 5 years ago

Please rebase to current sprotty-theia. Sorry for the hassle.

tortmayr commented 5 years ago

No worries. PR and test branch are now rebased

JanKoehnlein commented 5 years ago

Great, thanks for the patch!

I moved the LS specific part into the language-server directory, so instead of creating a pull request against your fork I did a new one https://github.com/eclipse/sprotty-theia/pull/22 with an additional commit.