Fixes #21: Align action processor with client-side action dispatcher #66
Conversation
- Action processor should invoke ALL registered action handlers - Each response by an action handler should be processed - Introduce client action handler that sends actions to client Note: This change does not adapt the operation handlers which execute operations and do not return any actions as of now. Fixes eclipse-glsp/glsp#21 Signed-off-by: Martin Fleck <[email protected]>
martin-fleck-at
force-pushed
the
issue-21
branch
from
cdef27d to
655323d
Compare
There was a problem hiding this comment.
Thanks! Code looks good for the most part, I only noticed a few minor issues.
I really like the approach of using named injection to identify client actions 👍
Maybe it would be a good idea to also adapt the workflow example and create a multi-handler scenario to demonstrate/test this new feature
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/action/DefaultActionProcessor.java
Outdated
Show resolved
Hide resolved
| @@ -88,7 +97,22 @@ private MultiBindingDefaults() {} | |||
| TriggerEdgeCreationAction.class, | |||
| UpdateModelAction.class); | |||
|
|
|||
| public static final List<Class<? extends Action>> DEFAULT_CLIENT_ACTIONS = Lists.newArrayList( | |||
| SetClipboardDataAction.class, | |||
There was a problem hiding this comment.
It looks like this list is not complete. There are a couple of other client actions (ike. Center, FitToScreen, Select, Expand, ServerStatusAction etc.)
We should also double check the ActionHandlers e.g. the SetEditModeActionHandler implicitly registers the SetEditModeAction which has to be registered as client action as well.
There was a problem hiding this comment.
Very good point! As discussed, I registered the necessary actions as client actions and also removed up some unused actions, cf also eclipse-glsp/glsp-client#74.
I also added a logging example for the server-side action dispatching, cf. eclipse-glsp/glsp-examples#60
There was a problem hiding this comment.
Looks good to me. Just a quick question: There are a couple of actions that are declared in both DEFAULT_ACTIONS and DEFAULT_CLIENT_ACTIONS. Is this really necessary?
There was a problem hiding this comment.
Very good observation! I tested this and you are right, the DIActionRegistry already captures all handled actions through the action and operation handler bindings. As a result, the default action binding was removed altogether. I pushed a new commit to reflect that. Thank you!
- Fix wrong logger - Add missing client actions - Add client-side actions that could be sent from server -- NavigateToTarget action - Remove actions that are neither used on the server nor on the client -- CollapseExpandAction -- CollapseExpandAllAction -- OpenAction -- RequestExportSvgAction Signed-off-by: Martin Fleck <[email protected]>
- DIActionRegistry already considers handled actions and operations -- Remove default action binding altogether -- Only handled actions are part of the action registry - Add missing trigger actions and SetBounds action to client action set Signed-off-by: Martin Fleck <[email protected]>
… dispatcher (eclipse-glsp#66) * eclipse-glsp#21: Align action processor more with client-side action dispatcher - Action processor should invoke ALL registered action handlers - Each response by an action handler should be processed - Introduce client action handler that sends actions to client Note: This change does not adapt the operation handlers which execute operations and do not return any actions as of now. Fixes eclipse-glsp/glsp#21 Signed-off-by: Martin Fleck <[email protected]> * eclipse-glsp#21: Incorporate feedback - Fix wrong logger - Add missing client actions - Add client-side actions that could be sent from server -- NavigateToTarget action - Remove actions that are neither used on the server nor on the client -- CollapseExpandAction -- CollapseExpandAllAction -- OpenAction -- RequestExportSvgAction Signed-off-by: Martin Fleck <[email protected]> * eclipse-glsp#21: Avoid registering actions that are automatically registered anyway - DIActionRegistry already considers handled actions and operations -- Remove default action binding altogether -- Only handled actions are part of the action registry - Add missing trigger actions and SetBounds action to client action set Signed-off-by: Martin Fleck <[email protected]>
Note: This change does not adapt the operation handlers which execute operations and do not return any actions as of now.
Signed-off-by: Martin Fleck [email protected]