kieler / klighd-vscode

Eclipse Public License 2.0
8 stars 6 forks source link

Theia diagrams to VSCode diagrams + CLI rebuild #1

Closed christoph-fricke closed 2 years ago

christoph-fricke commented 3 years ago

This PR highlights the changes between the previous Theia IDE extensions and the new VSCode extension + CLI.

Most notable changes are the application packages, which are completely new. Huge changes to the diagram core include the addition of options module, sidebars, preferences, services and the inclusion of a diagram server as well as the concept of registries. The removal of old the Theia extensions properly makes this code review a bit chaotic, so these are the big changes that should be reviewed the most.

During the rebuild all diagram code moved to this new repository. So this PR is a bit special and should not be merged. Only used for code review.

christoph-fricke commented 3 years ago

Overall very nicely structured and well-documented code. I left some comments and questions here and there, I'd be happy if you take a look at those. I have another more general question: Why did you merge the previous keith-diagram and keith-sprotty packages into one? Seperating this was quite nice as keith-sprotty hat no dependency to Theia (and would therefore not have a dependency to VS Code here) to make it possible to reuse elsewhere. I can see that with the standalone application this would probably not be used, but having that separation with IDE-unspecific code also has its upsides.

I am not sure if I fully understand what you mean. The changes where discussed basically all the time :thinking:. I am not sure where klighd-core (previously keith-sprotty) is supposed to has IDE specific code. All code in the core is platform independent.

Functionality that requires access to the platform is provided via services that are implemented and provided to the core by each platform (VS Code and Standalone). Especially the Connection service is important which is used to remove the server connection logic (how it is connected etc.) from the actual DiagramServer class and thus from the core. Therefore, the DiagramServer, which contains a lot of action handler registration for all the custom actions that should be sent to the server, only has to be implemented once and can be part of the core.

Using the approach suggested by Sprotty would mean that the VS Code extension extends and provides a VsCodeLspDiagramServer and the standalone view extends and provides an WebsocketDiagramServer. Which would also mean that all the action registration would be duplicated, and would be a concern for each platform. The service approach solves the problem by relying on composition instead of inheritance (which is arguably superior). The ability to send actions is therefore composed into the DiagramServer, which is now only concerned about handling actions` instead of inherited.

Otherwise, integrating other keith-diagram features, such as the options UI, into the core allowed for higher code sharing and easier integration for other platforms. My original proposal suggested that it should be done via a layer on top of the Sprotty container, but we both came to the conclusion that it would be beneficial (avoiding an extra layer thus being more flexible) to integrate it into the Sprotty DI container, after I discovered the undocumented UIExtensions feature.

NiklasRentzCAU commented 3 years ago

Overall very nicely structured and well-documented code. I left some comments and questions here and there, I'd be happy if you take a look at those. I have another more general question: Why did you merge the previous keith-diagram and keith-sprotty packages into one? Seperating this was quite nice as keith-sprotty hat no dependency to Theia (and would therefore not have a dependency to VS Code here) to make it possible to reuse elsewhere. I can see that with the standalone application this would probably not be used, but having that separation with IDE-unspecific code also has its upsides.

I am not sure if I fully understand what you mean. The changes where discussed basically all the time . I am not sure where klighd-core (previously keith-sprotty) is supposed to has IDE specific code. All code in the core is platform independent.

Functionality that requires access to the platform is provided via services that are implemented and provided to the core by each platform (VS Code and Standalone). Especially the Connection service is important which is used to remove the server connection logic (how it is connected etc.) from the actual DiagramServer class and thus from the core. Therefore, the DiagramServer, which contains a lot of action handler registration for all the custom actions that should be sent to the server, only has to be implemented once and can be part of the core.

Using the approach suggested by Sprotty would mean that the VS Code extension extends and provides a VsCodeLspDiagramServer and the standalone view extends and provides an WebsocketDiagramServer. Which would also mean that all the action registration would be duplicated, and would be a concern for each platform. The service approach solves the problem by relying on composition instead of inheritance (which is arguably superior). The ability to send actions is therefore composed into the DiagramServer, which is now only concerned about handling actions` instead of inherited.

Otherwise, integrating other keith-diagram features, such as the options UI, into the core allowed for higher code sharing and easier integration for other platforms. My original proposal suggested that it should be done via a layer on top of the Sprotty container, but we both came to the conclusion that it would be beneficial (avoiding an extra layer thus being more flexible) to integrate it into the Sprotty DI container, after I discovered the undocumented UIExtensions feature.

Somehow I misread the klighd-core package as having a VS Code dependency. As that is not the case, my general comment is invalid.

NiklasRentzCAU commented 2 years ago

The code review this PR was used for has been completed. Closing this.