-
Notifications
You must be signed in to change notification settings - Fork 8
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
Package API #15
Package API #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thank you Luca! This API rework is a great step forward. In general I think your proposed API is very well designed and I didn't find any bigger issues.
However, I have a couple of in-line comments and suggestions (mostly related to naming and general extensibility).
The new functionality (navigation, svg-export, copy-paste, diagram-menu) works very well and I didn't encounter any issues during testing.
Also feel free to add yourself to the contributors
list of the package.json
files. This is a major contribution to the glsp-vscode-integration (or even the GLSP project in general) and we want to give credit where is due 😉 .
packages/vscode-integration/src/quickstart-components/glsp-server-starter.ts
Outdated
Show resolved
Hide resolved
packages/vscode-integration/src/quickstart-components/glsp-server-adapter.ts
Outdated
Show resolved
Hide resolved
packages/vscode-integration/src/quickstart-components/glsp-server-adapter.ts
Outdated
Show resolved
Hide resolved
Hi, just bumping this thread. Thank you very much for taking the time to review my changes! You can let me know if there is something I should improve! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast update. The PR looks good to me now.
I have one last request: This branch now has a lot of commits. Could you please squash them into one single commit with a descriptive commit message? Thanks
This commit reworks a large part of the VS Code integration package by updating the user-facing API and adding editor features. At the heart of the user-facing API lies the "GlspVscodeConnector". This component acts as a bridge between a diagram client and a graphical language server implementation, both of which the extension authors provide via the connector's options. The connector will propagate any message sent by the server to the client and vice-versa. It intercepts messages of certain types to add VS Code native functionality, for example, it processes the "setMarkers" action sent by the server to render diagnostics within VS Code. The VS Code connector exposes methods to interact with active clients. The "sendActionToActiveClient" method can be used to send various GLSP-Protocol Actions to the currently active client. Extension authors can use this action to interact with the diagram through user-triggered VS Code commands. Extension authors can use interceptor functions in the connector configuration to control how the VS Code integration behaves when it receives messages. The new package API now provides "quickstart components" that reduce the necessary boilerplate code to get started. Quickstart components include a helper class to launch the default GLSP language server, a default editor provider to render webviews, and a helper class to connect a language server to the VS Code integration. Complete Changelog: - Added native editor functionality - Clipboard support - SVG Export - Native menu contributions - Rework API for extension authors - Component to bridge client and server (GlspVscodeConnector) - Interceptors - Method to interact with currently active diagram editor (sendActionToActiveClient) - Exposure of currently selected elements within the diagram editor - Add quickstart components - GLSP server launcher - Default editor provider - Helper class to connect language server to VS Code integration - Add documentation
5ba07b4
to
e255f8b
Compare
I squashed the commits as requested and tried to include a comprehensive message. Thanks once again for taking the time to review this PR! |
@lforst Now that this PR is merged could you please update the related issues (#201-#204)? Please comment on each issue to let us know if the issue is fixed or only partly fixed (and what's still missing). |
@tortmayr Thanks for reminding me! I left comments in the relevant issues and created follow-up issues like discussed. Please let me know if I missed something! |
Disclaimer: This pull request is very large, adds a lot of code, removes a lot of existing code, and might contain opinionated changes. This is why I am very open to discussions as well as feedback and I am ready to implement any suggestions you might have.
Originally my aim with this pull request was to implement a well thought out user-facing API and clean up the existing one. Because I had to consider all the open features which were not implemented yet for the API design, the whole thing went a bit out of control and I decided to implement the rest of the features too, in order to check the features against the API.
The set of changes now entails:
If you are going to review this PR I recommend taking a look at the documentation I added to get a feeling for what this PR is about.