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

#738: Add route data to ComputedBoundsAction #181

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Conversation

martin-fleck-at
Copy link
Contributor

Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks Martin. Code seems to work fine.
I have tested it by changing the routerKind of the workflow example edge creation handlers to "manhattan".

I have a few inline suggestions and one more general question:

I personally find it a bit odd that an action called "ComputedBoundsAction" also contains the routing points. Also by directly adapting the "ComputedBoundsAction" which we might introduce an inconsistency/incompatibility with the original ComputedBoundsAction from sprotty.

Maybe it would be better to use a dedicated ComputedRoutingPointsOperation and wrap it together with the ComputedBoundsOperation in a "CompoundOperation`?

@martin-fleck-at
Copy link
Contributor Author

@tortmayr Thank you very much for your feedback, I adapted the code accordingly. Regarding a dedicated operation for the routing points, I opted against it since the client-side layout information is currently all part of the ComputedBoundsAction, also the alignment. Furthermore, this action carries a specific meaning as the ComputedBoundsActionHandler will directly submit the model afterwards, so splitting that into two operations would make their handling more difficult.

@martin-fleck-at martin-fleck-at changed the title #738: Extend ComputedBoundsAction with routing points #738: Add route data to ComputedBoundsAction Oct 17, 2022
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks. Code looks good to me an works like a charm.

@martin-fleck-at martin-fleck-at merged commit fbd330b into master Nov 14, 2022
@martin-fleck-at martin-fleck-at deleted the issue-738 branch November 14, 2022 16:20
tortmayr added a commit to eclipse-glsp/glsp-server-node that referenced this pull request Dec 6, 2022
Implement routing points support for the node glsp server and ensure that the API mirrors the latest changes of the java glsp server (eclipse-glsp/glsp-server#181)

Fixes eclipse-glsp/glsp#738
tortmayr added a commit to eclipse-glsp/glsp-server-node that referenced this pull request Dec 6, 2022
Implement routing points support for the node glsp server and ensure that the API mirrors the latest changes of the java glsp server (eclipse-glsp/glsp-server#181)

Fixes eclipse-glsp/glsp#738
tortmayr added a commit to eclipse-glsp/glsp-server-node that referenced this pull request Dec 6, 2022
Implement routing points support for the node glsp server and ensure that the API mirrors the latest changes of the java glsp server (eclipse-glsp/glsp-server#181)

Fixes eclipse-glsp/glsp#738
tortmayr added a commit to eclipse-glsp/glsp-server-node that referenced this pull request Dec 6, 2022
Implement routing points support for the node glsp server and ensure that the API mirrors the latest changes of the java glsp server (eclipse-glsp/glsp-server#181)

Fixes eclipse-glsp/glsp#738
tortmayr added a commit to eclipse-glsp/glsp-server-node that referenced this pull request Dec 7, 2022
Implement routing points support for the node glsp server and ensure that the API mirrors the latest changes of the java glsp server (eclipse-glsp/glsp-server#181)

Fixes eclipse-glsp/glsp#738
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.

2 participants