-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
4402361
to
ed39a08
Compare
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 @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; |
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.
We should decide whether we want to import Inject
from google.inject or javax.inject and double-check all imports to ensure consistency
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.
In most places we use google.inject. So I used this one now consistently.
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/SelectAllAction.java
Outdated
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/ServerMessageAction.java
Outdated
Show resolved
Hide resolved
...ipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ModelSubmissionHandler.java
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/gmodel/GModelDiagramModule.java
Outdated
Show resolved
Hide resolved
binding.add(CutOperationHandler.class); | ||
binding.add(DeleteOperationHandler.class); | ||
binding.add(LayoutOperationHandler.class); |
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.
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
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.
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 <[email protected]>
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 |
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. LGTM!
* Adaptations for eclipse-glsp/glsp-server#165 * TPD update, see eclipse-glsp/glsp-server#163
* Adaptations for eclipse-glsp/glsp-server#165 * TPD update, see eclipse-glsp/glsp-server#163
* Adaptations for eclipse-glsp/glsp-server#165 * TPD update, see eclipse-glsp/glsp-server#163
* Adaptations for eclipse-glsp/glsp-server#165 * TPD update, see eclipse-glsp/glsp-server#163
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 prefixGModel
in the class name.