#233 Add operation arguments to CreateNodeOperationHandler #108
Conversation
tortmayr
force-pushed
the
tortmayr/issues/233
branch
from
b3f46f3 to
12e289f
Compare
...se.glsp.server/src/org/eclipse/glsp/server/operations/gmodel/CreateNodeOperationHandler.java
Show resolved
Hide resolved
| @@ -40,13 +41,13 @@ public void executeOperation(final CreateNodeOperation operation, final GModelSt | |||
| container = Optional.of(modelState.getRoot()); | |||
| } | |||
|
|
|||
| GModelElement element = createNode(getLocation(operation), modelState); | |||
| GModelElement element = createNode(getLocation(operation), operation.getArgs(), modelState); | |||
There was a problem hiding this comment.
As far as I can see the map may be null here. I am not too much of a fan of null collection parameters, especially when point is wrapped in a nice Optional. But it could be that this is consistent with other operation handlers as well, just wanted to mention that in case it is different anywhere else.
There was a problem hiding this comment.
Good point. I updated the CreateNodeOperation so that the args map is initalized with an empty map. This way we can avoid the overhead of wrapping the Map into an Optional and the map should always be defined. Unless somebody explicitly uses the setter to set the map to null, which should hopefully never be the case 😉)
There was a problem hiding this comment.
I like having an empty map before having an optional map much more! However, it seems that CreateEdgeOperation and CreateNodeOperation both initialize the args with null due to re-using the full constructor. So either we do a putAll in the CreateNodeOperation constructor for non-null maps OR the subclasses also need to use an empty map as their default constructor argument.
There was a problem hiding this comment.
Good catch. I checked other occurrences where we use an arbitrary args Map (e.g. EditorContext, RequestModelAction etc.) and we always more or less operate on the assumption that this map is not null. So I updated the constructors of all classes that use an args Map to ensure that they are initialized with an empty map.
There was a problem hiding this comment.
Fantastic, thank you!
tortmayr
force-pushed
the
tortmayr/issues/233
branch
from
12e289f to
eea52b4
Compare
...xample.workflow/src/org/eclipse/glsp/example/workflow/handler/CreateActivityNodeHandler.java
Outdated
Show resolved
Hide resolved
…p/example/workflow/handler/CreateActivityNodeHandler.java
eclipse-glsp#108) * eclipse-glsp#233 Add operation arguments to CreateNodeOperationHandler Resolves eclipse-glsp/glsp/issues/233
Resolves eclipse-glsp/glsp/issues/233