211: Revise TypeHints and server side feedback for creation actions

[CamilleLetavernier]

• Add a new parameter to EdgeTypeHint, 'dynamic', indicating that new
  edges need to check with the server before allowing creation
• Make source/target element type ids in EdgeTypeHint optional
  If not defined, all potential element types are considered to be valid sources/targets
• Add RequestEdgeCheckAction and EdgeCheckResultAction response to implement
  the dynamic check
• Add a optional EdegeCreationchecker component that can be implemented by adopters to provide dynamic typehints
• Adapt workflow example to use dynamic edge hints for weighted edges
• Move typehints related components into feature subpackage
• Move progress actions into progress feature subpackage

Co-authored-by: Camille Letavernier <[email protected] >

[tortmayr]

Thanks @CamilleLetavernier.
I only had time to do a preliminary code review for now, but I will also test the changes later today

[CamilleLetavernier]

Thanks,
I've pushed a commit to address the issues. I'm surprised that the code still worked despite the inconsistent Action Type; but I guess it's because it's a Request/Response? We don't actually have an ActionHandler for the response, so I think the type is ignored on the client side.
Looking at ResponseAction on the server, I think I should be using ResponseAction.repond() instead of simply returning the response action?

[tortmayr]

Looking at ResponseAction on the server, I think I should be using ResponseAction.repond() instead of simply returning the response action?

No, that's fine. The ActionDistpacher has some default behavior in place to automatically call ResponseAction.response for the return actions of ActionHandlers.

glsp-server/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/internal/actions/DefaultActionDispatcher.java

Line 193 in a48db75

final List<Action> responses = actionHandler.execute(action).stream()

[tortmayr]

Thanks for addressing the issues @CamilleLetavernier. In general the change looks good to me.

However, currently it's not really possible to test this change without further modifications.
Maybe we could provide a simple custom RequestTargetTypeHintsActionHandler for the workflow example? For instance we could make the type hints for weighted edges dynamic and check on the server side whether the target is valid (I.e. source is a decision node, target is a task or fork/join node)

Also I'm not the biggest fan of having default no-op RequestTargetTypeHintsActionHandler that needs to be rebind by adopters. Maybe we could use a generic action handler that delegates to an optional injected service? Similar to how we handle e.g. RequestModelActions, ReuestMarkersAction, LayoutOperation etc.

We could introduce a EdgeTargetValidator service that can be bound by adopters, and the handler for the RequestCheckEdgeTargetAction simply delegates to the validator (if bound)
@planger WDYT?

[planger]

We could introduce a EdgeTargetValidator service that can be bound by adopters, and the handler for the RequestCheckEdgeTargetAction simply delegates to the validator (if bound)
@planger WDYT?

Yes that would be better. Having to rebind a no-op is very implicit, being able to overwrite an empty method in the abstract class is more explicit. Our credo was always to have abstract methods for the mandatory implementations an adopter need to deliver, and the they can press Ctrl+space in their module and see what other optional implementations they can provide.

In any case, I would try to avoid the term "validator" and rather use "check" or so, to avoid confusing this concept with the model validation.

[tortmayr]

@martin-fleck-at I have taken over this draft from Camille and have finalized the PR. Could you please have a look at it?

[martin-fleck-at]

Change looks good to me on a high level. I just have some minor concerns regarding documentation and default values. Thank you!

Btw, is there a plan to also adapt this in the Node server?

[tortmayr]

Btw, is there a plan to also adapt this in the Node server?
The PR for the node server will be openend once the client change is merged (because we need the protocol changes there)

[martin-fleck-at]

LGTM!