#472 Refactor action defintion to interfaces

[tortmayr]

Sprotty moved away from class based approach for action definition and solely relies on interfaces now.
To be able to update to the latest sprotty version GLSP needs to to do the same. Otherwise action definitions might clash
and there will be an inconsistent mix of class and interface definitions.
This PR refactors the action definitions in the base protocol & the glsp client to interfaces. Action definitions that are reused from sprotty are marked
with extends sprotty.XYZAction. In addition, to the pure interface definition for each action/operation type a namespace is provided that offers utility functions for type checking and creating new instances.

• Refactor all action definitions to interfaces+namespaces
• Refactor all action constructions calls to use the new Action.create() approach instead of the old new Action() approach
• Add tests for the action utility function (is, create etc.) of all protocol actions.
• Replace all old typeguard functions like isComputedBoundsAction with their new namespace counterpart (SetBoundsAction.is)
• Add isOperation:true boolean property to the operation interface. This serves as discriminator to make actions & operations distinguishable on a
  type level.
• Update to latest sprotty version
• Update to latest glsp-config package to make use of the new no-deprecated eslint rules.
• Add package specific eslint warnings for restricted/discourages imports.

Also:

• Disable hover-popup listener when the Edge creation tool is active
• Update changelog

Contributed on behalf of STMicroelectronics

[planger]

Excellent work! Thank you very much for this comprehensive change!
Great that you also added lots of typedoc and unit tests! 💯

I made a couple of edit suggestions and comments about the typedocs. While most of them are great, there are several which imho don't really add information. I tend to prefer omitting those, because they add noise without helping.

I tested with eclipse-glsp/glsp-theia-integration#110 and observed a few regressions:

• Task editing doesn't work: after selecting type or duration, the input disappears
• Validation throws an error (Missing handler for action { kind: 'applyMarkers',...)
• Resize and move restrictors for overlaps don't work... I'm not sure though if this is a regression or didn't work in the first place
• Open push command (in Theia) doesn't select the correct element (could be unrelated too)

Is there a particular reason why we really copy and not import and re-export all Sprotty actions? I think there was a reason, but I can't remember right now. Now reviewing the code it feels like a lot of maintenance overhead with the risk of getting out of sync?

Again, excellent work on this huge change! Thanks for your persistence!

[tortmayr]

Thank you for your fast review. I'm going to address the feedback right away.

Task editing doesn't work: after selecting type or duration, the input disappears

This problem seems to be already present on the current master

Resize and move restrictors for overlaps don't work... I'm not sure though if this is a regression or didn't work in the first place

This is related to eclipse-glsp/glsp/issues/394. Currently the movement restrictor is not enabled for the workflow example.

Is there a particular reason why we really copy and not import and re-export all Sprotty actions? I think there was a reason, but I can't remember right now. Now reviewing the code it feels like a lot of maintenance overhead with the risk of getting out of sync?

I assume you are referring to the actions in the protocol package?

• With a simple reexport we lose the possibility to apply GLSP-specific JS-DOC to the action definitions
• The sprotty protocol actions use a slightly different approach regarding their namespace functions. They only offer the constant KIND and a create function. And (for whatever reason) opted to keep the guard functions as separate isSomeAction() functions. In GLSP we prefer guard functions as part of the namespace (is()). So we would have to do some kind of module augmentation anyways. In addtion, the isSomeAction guard functions of sprotty are only available for some actions.
• With re declaring we have the possibility to extend the sprotty provided interfaces. This is something we for instance do in the ServerStatusAction where we add an additional timeout property.
• The parameterize of sprottiy's create() functions is not consistent and quite confusing at times. With the namespace re declare we have the possibility to apply our own consistent pattern.
• With re declaring the protocol actions we have a consistent entry place for API documentation. Otherwise the doc would
  be scattered across sprotty-protocol and @eclipse-glsp/protocol. With the current approach we have all protocol action documented in one place which also makes it relatively easy to sync doc changes with the https://github.com/eclipse-glsp/glsp/blob/master/PROTOCOL.md file.

Since we extend from the sprotty interfaces I don't see any risks of getting out of sync. This ensure that we are always compliant to the sprotty definition. I would even argue that is is a better approach in terms of API stability. With a simple reexport we would pick up breaking changes much later (if at all).

[planger]

Ah, right, now I see. Thanks!

[tortmayr]

I have pushed an update that should address all the raised issues.
There are a lot of comments regading JS-DOC so I hope that I didn't miss any. I had a quick check and all comments regarding doc seem to be outdated now (meaning the source code at the location has changed -> I hopefully addressed the comment 😉 )

Task editing doesn't work: after selecting type or duration, the input disappears

Should be fixed now. Was related to a incorrect implementation of LabeledAction.is. With fixed I mean its in the current state
as the master. Apparently something is broken here. At least for me selecting a suggestion has no effect on the actual model. We should look into that in a follow-up.

Validation throws an error (Missing handler for action { kind: 'applyMarkers',...)

Fixed. Was related to an incorrect KIND property of the ApplyMarkers command.

Resize and move restrictors for overlaps don't work... I'm not sure though if this is a regression or didn't work in the first place

As mentioned before, movement restrictors for the Workflow example are currently disabled.

Open push command (in Theia) doesn't select the correct element (could be unrelated too)

Fixed. It seems like we introduced duplicate names when changing the Workflow example styling. I updated all example1.wf files.

[planger]

Excellent, thanks a lot!
I did a few inline suggestions that you should be able to directly apply, mostly to typedoc.
So the only thing left is imho to adjust the text in the other tests. But I already approve for you to spare another review cycle.

Thanks again for your persistence with this large and important change! Now we are consistent and up to date with Sprotty again, and it is simpler for adopters when it comes to importing classes and registering DI implementations! 💯

[tortmayr]

@planger Thank you for the detailled review. I have updated the JS-DOC according to your suggestions and applied the new naming scheme to all test cases. So now we should be ready to merge