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

GLSP-1254 Fix client-server action forwarding #58

Merged
merged 4 commits into from
Feb 19, 2024
Merged

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented Feb 18, 2024

What it does

  • Deprecate the concept of marking actions as locally dispatched by appending the __localDispatch property. This concept is a remainder of the 1.x API implementation and no longer necessary in 2.x. In addition, it conflicted with the default behavior of marking actions received from the server.

  • Remove ExtensionAction bindings of RequestClipboardDataAction and SetClipboardDataAction in vscodeCopyPasteModule. These bindings where added on accident. As a consequence, related actions dispatched in the webview client where also forwarded back to the host extension where they were not handled.

Fixes eclipse-glsp/glsp#1254

How to test

Test with different integration kinds. Diagram rendering should now work again.

Follow-ups

eclipse-glsp/glsp#1259

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)
  • [API] The VsCodeGLSPModelSource and ExtensionAction are now deprecated.
    - VsCodeGLSPModelSource: Rebinding to a custom model source is no longer necessary. Use the default GLSPModelSource instead.
    - ExtensionAction: The concept of marking actions as locally dispatched ExtensionActions is no longer necessary and usage is discouraged.

@tortmayr tortmayr force-pushed the tortmayr/issues/1245 branch from 4a2928b to a269aa7 Compare February 18, 2024 09:55
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you very much for the update, Tobias! Indeed this PR fixes the rendering of the model so we definitely want to get that in. I just noticed another issue of which I'm not sure if it is related: I can no longer save a model. It seems that the SaveAction is already marked as coming from the server, so some thing may have gotten lost here, could you please have another looks? If you think it is more work, I can also approve this and we can log a separate issue.

- Deprecate the concept of marking actions as locally dispatched by appending the `__localDispatch` property. This concept is a remainder of the 1.x API implementation and no longer necessary in 2.x. In addition, it conflicted with the default behavior of marking actions received from the server.

- Remove `ExtensionAction` bindings of `RequestClipboardDataAction` and `SetClipboardDataAction` in `vscodeCopyPasteModule`. These bindings where added on accident. As a consequence, related actions dispatched in the webview client where also forwarded back to the host extension where they were not handled.

Fixes eclipse-glsp/glsp#1254
@tortmayr tortmayr force-pushed the tortmayr/issues/1245 branch from 9c77a95 to f12b430 Compare February 18, 2024 23:09
@tortmayr
Copy link
Contributor Author

tortmayr commented Feb 18, 2024

Good catch.
The problem here is that all actions dispatched from the host extension are actually sent to the client.
However, certain actions like (Save, Undo,Redo and RequestModel) should actually be sent to the server.
Previously this worked due to the faulty (non) marking of server-received actions. So for instance the client received a SaveModelAction and also dispatched it to the server.
Howedver, this is an unnecessary roundtrip. I introduced a sendActionToServer´ method in the GLSVscodeConnector` to directly send the actions in questions to the server without the roundtrip to the client.

Note that this is just a quick fix. We should create a follow-up to improve this behavior and use the information provided by client and server (i.e. handled action kinds to automatically derive the intended target when dispatching an action in the host extension. I have created a follow-up Ticket for that (eclipse-glsp/glsp#1259)

@tortmayr tortmayr force-pushed the tortmayr/issues/1245 branch from f12b430 to f4d99ce Compare February 18, 2024 23:19
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update Tobias! I re-tested the change and now everything works much better for the workflow example.

I approve this PR for now but is the risk of adopters not breaking still rather high since we only handle known server actions for now but if there is any customization that will not work with the PR. I saw that you created a follow-up ticket (eclipse-glsp/glsp#1259) to address the client/server action gap. So if there is a high risk of breakage, that issue should be definitely treated as high priority and fixed before making any further releases, I believe.

…t extension

Introduce `dispatchAction` method in `GLSPVSCodeConnector`. 
This method uses the handled action kinds provided by server/client to determine wether the action should be forwarded to client and/or server.

Deprecate `sendActionToActiveClient` and `sendActionToClient` methods (and use `dispatchAction` internally).

Fixes eclipse-glsp/glsp#1259
@tortmayr
Copy link
Contributor Author

@martin-fleck-at I have applied the changes for the follow-up PR directly in this PR. Hopefully, we can avoid any breakage with this.

I also have introduced a migration label that allows us to mark PRs that need to be mentioned/covered in the migration guide.

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Changes look great to me for the most part, thank you Tobias! I'm only worried about the dependencies but maybe they are fine as well, please double check.

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

LGTM!

@tortmayr tortmayr merged commit 11ab9a1 into master Feb 19, 2024
3 checks passed
@tortmayr tortmayr deleted the tortmayr/issues/1245 branch February 19, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Action forwarding not working properly in VS Code integration
2 participants