-
Notifications
You must be signed in to change notification settings - Fork 29
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
#425#120 Rework Actionhandler/Operationhandler API #135
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.
Thanks, @tortmayr! Looks already very good!
I haven't really made up my mind yet, but we could even go as far as injecting also the action / operation with the concrete type. This could make several abstract classes superfluous. But it would also make the interface of handlers a bit implicit. WDYT?
I'd also be very interested in the opinion of others, @martin-fleck-at, @CamilleLetavernier?
Maybe I missed it, but do we already bind a concrete GModelState class so that handlers can inject their concrete class directly?
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/ActionHandler.java
Outdated
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/ActionHandler.java
Outdated
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/ActionHandler.java
Outdated
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/BasicActionHandler.java
Outdated
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/DefaultActionHandler.java
Outdated
Show resolved
Hide resolved
....eclipse.glsp.server/src/org/eclipse/glsp/server/operations/BasicCreateOperationHandler.java
Outdated
Show resolved
Hide resolved
...ns/org.eclipse.glsp.server/src/org/eclipse/glsp/server/operations/BasicOperationHandler.java
Outdated
Show resolved
Hide resolved
...clipse.glsp.server/src/org/eclipse/glsp/server/operations/DefaultCreateOperationHandler.java
Outdated
Show resolved
Hide resolved
.../org.eclipse.glsp.server/src/org/eclipse/glsp/server/operations/DefaultOperationHandler.java
Outdated
Show resolved
Hide resolved
I'm a bit lost here. I don't see how injecting the action/operation could help here. Can you please eloborate?
Yes we do: glsp-server/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/di/DiagramModule.java Line 169 in 0a51fc6
|
I was wondering whether we could, before we invoke the handler, put the action (to be processed) with the concrete type into the DI container, so that handlers could also use
Yes, but can clients now also inject a subclass of |
I see a couple of problems with that proposal:
To sum up, I'm not sure if the proposal is feasible from a technical perspective (without working around the limitations of Guice) and would also affect alot of the existing API and the current behavior. I personally don't think that the benefits are worth the effort and think that the current approach of providing a TypedInterface for the basic 1-to-1 action handelers is a good compromise.
Yes, with 1d33c76 this is now possible |
With the DI rework #127 its no longer necessary to pass the model state object as part of the execute method. instead action/operation handlers can simply directly inject the model state if the want to use it. Since action & operation handlers are now client session scoped an arbitary state object can be injected and used for execution. (Part of eclipse-glsp/glsp/issues/120) - Remove the modelstate argument from the execute() method of `ActionHandler` and `OperationHandler` - Remove `Handler` super interface because it doesn't provide any additional value - Add documentation for affected API. - Introduce new `DefaultActionHandler` & `DefaultOperationHandler` that serve as replacement for the now deprecated `BasicActionHandler` & `BasicOperationHandler` - Deprecate `BasicOperationHandler` & `BasicActionHandler` & `BasicCreateOperationHandler` - Replace usage of `BasicActionHandler` with `DefaultActionHandler` - Replace usage of `BasicOperationHandler' with `DefaultActionHandler` - Replace usage of `BasicCreateOperationhandler' with `DefaultOperationHandler` & `DefaultCreateOperationHandler` Fixes eclipse-glsp/glsp/issues/425
…tions/ActionHandler.java Co-authored-by: Philip Langer <[email protected]>
Co-authored-by: Philip Langer <[email protected]>
Naming: Default->Abstract
Bind actual implementation class of GModelState to itself as singleton before binding it to GModelState. e.g. by overriding the following method of `GLSPDiagramModule` ```java bindGModelState(){ return MyModelState.class } ``` clients can also directly inject the implementing subclass with ```java @Inject protected MyModelState myModelState; ``` Also fix type in `GLSPServerModule`
4c103dc
to
1d33c76
Compare
@tortmayr This looks now very promising. I really like the way how it is done now. |
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 feedback and the further improvements! Looks great!
I agree that if Guice makes it hard to add a binding to the injector after initial creation of the container, it is probably not worth going that route.
Btw I also successfully tested a custom GModelState class and it works fine! Thanks! |
Oh I just realized while testing that label validation broke, I guess with my change (GModelState is null). Unfortunately I have to work on something else now, but will investigate the issue later today or tomorrow. |
This simplifies the use of custom model state classes because implementations can now inject their concrete custom class instead of the generic GModelState that they'd have to cast again. Also several implementations often don't need the model state making the method parameter superfluous. Signed-off-by: Philip Langer <[email protected]>
4a6af53
to
17535e9
Compare
Fixed with 17535e9 |
@planger Thanks for the update. I was initially planning to do that in a follow-up PR because it's not directly related to this issue but we of course can do it directly with this PR as well. |
* #425#120 Rework Actionhandler/Operationhandler API With the DI rework eclipse-glsp#127 its no longer necessary to pass the model state object as part of the execute method. instead action/operation handlers can simply directly inject the model state if the want to use it. Since action & operation handlers are now client session scoped an arbitary state object can be injected and used for execution. (Part of eclipse-glsp/glsp/issues/120) - Remove the modelstate argument from the execute() method of `ActionHandler` and `OperationHandler` - Remove `Handler` super interface because it doesn't provide any additional value - Add documentation for affected API. - Introduce new `DefaultActionHandler` & `DefaultOperationHandler` that serve as replacement for the now deprecated `BasicActionHandler` & `BasicOperationHandler` - Deprecate `BasicOperationHandler` & `BasicActionHandler` & `BasicCreateOperationHandler` - Replace usage of `BasicActionHandler` with `DefaultActionHandler` - Replace usage of `BasicOperationHandler' with `DefaultActionHandler` - Replace usage of `BasicCreateOperationhandler' with `DefaultOperationHandler` & `DefaultCreateOperationHandler` Fixes eclipse-glsp/glsp/issues/425 * Remove GModelState from interfaces as it should now be injected instead This simplifies the use of custom model state classes because implementations can now inject their concrete custom class instead of the generic GModelState that they'd have to cast again. Also several implementations often don't need the model state making the method parameter superfluous. Fixes eclipse-glsp/glsp/issues/120 Co-authored-by: Philip Langer <[email protected]>
With the DI rework #127 its no longer necessary to pass the model state object as part of the execute method. instead action/operation handlers
can simply directly inject the model state if the want to use it. Since action & operation handlers are now client session scoped an arbitary state object can be injected and used for execution. (Part of eclipse-glsp/glsp/issues/120)
ActionHandler
andOperationHandler
Handler
super interface because it doesn't provide any additional valueDefaultActionHandler
&DefaultOperationHandler
that serve as replacement for the now deprecatedBasicActionHandler
&BasicOperationHandler
BasicOperationHandler
&BasicActionHandler
&BasicCreateOperationHandler
BasicActionHandler
withDefaultActionHandler
BasicOperationHandler' with
DefaultActionHandler`BasicCreateOperationhandler' with
DefaultOperationHandler&
DefaultCreateOperationHandler`Fixes eclipse-glsp/glsp/issues/425