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

Refactor classes that are only useful for GModel source models #165

Merged
merged 3 commits into from
May 16, 2022

Conversation

planger
Copy link
Member

@planger planger commented May 13, 2022

Move base diagram module and operation handlers that operate directly on GModels (as a model source) to the dedicated package org.eclipse.glsp.server.gmodel and add prefix GModel in the class name.

@planger planger force-pushed the refactor-gmodel branch 2 times, most recently from 4402361 to ed39a08 Compare May 13, 2022 14:42
@planger planger marked this pull request as ready for review May 13, 2022 14:42
@planger planger requested a review from tortmayr May 13, 2022 14:42
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 @planger. Changes look mostly good to me, I only have a few inline comments.

One additional discussion point:

Due to our current naming scheme the handler names tend to become relatively
long (especially implementation specific handlers for the emf/direct gmodel usecases).
I'm wondering whether we should shorten this a little bit by dropping the Action/Operation part. At the moment action and operation names are unique anyways so appending this information adds no additional value.
e.g. GModelApplyLabelEditOperationHandler => GModelApplyLabelEditHandler.
@planger WDYT?

@@ -18,12 +18,17 @@
import java.util.List;
import java.util.Optional;

import javax.inject.Inject;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should decide whether we want to import Inject from google.inject or javax.inject and double-check all imports to ensure consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

In most places we use google.inject. So I used this one now consistently.

binding.add(CutOperationHandler.class);
binding.add(DeleteOperationHandler.class);
binding.add(LayoutOperationHandler.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

With the LayoutOperationHandler being generic and reusable for all implementations shouldn't it be bound as part of the parent module?

Same is probably true for the CutOperationHandler

Copy link
Member Author

Choose a reason for hiding this comment

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

in fact, LayoutOperationHandler was already bound in the superclass anyway. But I now also pushed the cut operation handler up into the super class (and removed both from the subclass).

Move base diagram module and operation handlers that operate directly on
GModels (as a model source) to the dedicated package
`org.eclipse.glsp.server.gmodel` and add prefix `GModel` in the class
name.

Also adds JavaDoc to several classes.

Co-authored-by: Tobias Ortmayr <tortmayr@eclipsesource.com>
@planger
Copy link
Member Author

planger commented May 16, 2022

Due to our current naming scheme the handler names tend to become relatively
long (especially implementation specific handlers for the emf/direct gmodel usecases).
I'm wondering whether we should shorten this a little bit by dropping the Action/Operation part. At the moment action and operation names are unique anyways so appending this information adds no additional value.
e.g. GModelApplyLabelEditOperationHandler => GModelApplyLabelEditHandler.
@planger WDYT?

Thanks for the review! I've addressed all inline comments. Regarding the class name... I'm not so sure. They tend to get long, that's true, but this way they are also more explicit and I don't think it is a problem. But I'd be also fine renaming them and omit the Operation in their name. I didn't do it yet, so if you'd like this to be done, let me know.

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 for the fast update. LGTM!

@planger planger merged commit b6822d5 into master May 16, 2022
@planger planger deleted the refactor-gmodel branch May 16, 2022 08:35
planger added a commit to eclipse-glsp/glsp-eclipse-integration that referenced this pull request May 23, 2022
planger added a commit to eclipse-glsp/glsp-eclipse-integration that referenced this pull request May 23, 2022
planger added a commit to eclipse-glsp/glsp-eclipse-integration that referenced this pull request May 23, 2022
planger added a commit to eclipse-glsp/glsp-eclipse-integration that referenced this pull request May 23, 2022
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.

None yet

2 participants