Nested Executions initial implementation

[rschnekenbu]

[WIP] Currently updating with new features implemented these last weeks (message reorient for example is not taken perfectly into account + anchoring for automatically created messages like reply message)

[cdamus]

Looks great, @rschnekenbu! I see only a few problems in the diagram behaviour:

• most severe is that when reorienting the request message that starts the top execution of a stack to be received by another lifeline, all of the nested executions are moved with it (this "just works" as I had expected :-)) but the nesting is lost. All of the executions end up piled directly on top of one another
  
• if I create a synchronous message with execution and reply in which the request is drawn from right to left and then add a nested execution, that execution is on the right side. I think it should be on the left
  
• if I draw a message from right to left out of an execution that is nested on the right side, the message is anchored to the right side of the nested execution, not the left
  
• drawing a synchronous message to an execution specification doesn't create a nested execution. The editor prompts for the kind of execution to create, but then it doesn't create an
  execution nor does it create the reply message (obviously, because there's no execution). I expected to get a nested execution with reply
  
• I think it's already known, but I'll mention it here for completeness' sake, that when moving the lifeline that owns the executions so that messages incoming and outgoing those executions change orientation, they remain attached to the same side of the executions (except for the request/reply messages, which were fixed by improvements to the start/end anchors).

One new test fails on Mac (see the in-line comments) and there's a test that was deleted that should be restored (again, see the in-line comments).

Also, there are a lot of formatting changes in the EMF.Edit item providers. Is that expected? It looks like the code was formatted (with JDT's "clean up") after re-generating.

[cdamus]

Nice! Unfortunately, this test fails on my Mac:

Expected: location point that x that 276 ± 1 and y that 170 ± 1
     but: location y was <165>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at org.eclipse.papyrus.uml.diagram.sequence.runtime.internal.edit.policies.tests.ExecutionSpecificationCreationEditPolicyUITest.createNestedExecution(ExecutionSpecificationCreationEditPolicyUITest.java:64)

On the plus side, all of the other tests still pass (with no new skips). 😉

[rschnekenbu]

Hmmm, interesting! Again an issue with different platforms. I updated the test so it does get the initial Y value and compare the value after execution with the initial one.
Funny fact is that this model is also used in other classes, like ExecutionSnappingUITest. In all other classes, it is not used directly, only with offset to set the message connection point, or not used at all ;)
I would still be very happy to understand this difference. The position of the execution is set on the lifeline body, at least in notation. The height of the header is set also, so as its y position. The interaction iself may have a different layout with its label on top, given the font? This is the only element that does not have a size pushed in notation.

[cdamus]

Now, on my system, the tests are green. So, that's good. However, there's now an assumption failure that skips the ExecutionMoveUITest::moveSyncMessageEnd test. It seems that the sync message is no longer located in the diagram where the test expects it to be.

[rschnekenbu]

I solved all the conversation items, I left the one about test failure on mac platform, because I am interested in understanding the diff here. For the other ones, commit 348a8d6 and e65b707 solves all of them.

For the diagram behaviour:

• reorienting the message starting the nested execution moves now nicely the views. A reparentView was done while setting the owner of the executions, and so all moved elements views were added to the lifeline body figure. In the case of the nested execution, this is useless, as its parent is the nesting execution one. A check is now performed so it does not reparent views if not needed. A functional test was also added to check that layout is good on retargeting.
• For the nesting of executions, according to some live discussions with Antonio, it seems better to always cascade in the same direction, in case of big executions spanning across several computer screens. In this case, it would be easier to detect which one is the nesting one, and which are the nested ones if they are always cascading on the same side. So I propose to always keep the nesting on the right currently. We may update later if the users think this is not practical.
• for the side of the anchors, this is a bigger issue on executions in general. This will be handled in issue Messages attached to executions should swap sides when lifeline order is changed #376. I added a comment in that issue to ensure it is tested in the nested execution cases. I already started to implement some tests for that issue locally (currently failing of course!). This should be my next task.
• Good catch for the issue of creating an execution on connecting a sync message! I will check what is not going there, probably some edit policy missing on the execution similar to the one on lifeline body.
• For the test failures, I had fixed the inline comments. That should be fine now.

Finally, for the code formatting: looking at the history, it seems we have some trouble to keep consistency in formatting. I suspect we use different versions of EMF to generate code and / or do not apply the same actions after code generation. I did a format source code after generation / organize import and then a clean up using project profile (what I usually do when generating Papyrus diagram code). Even if I revert the edit plugin code and generate the code again, I get a lot of differences compared to the master branch. I am using EMF 2.14 for code generation.

Except for the automatic creation of the action execution when attaching a sync message to an execution, I addressed all the topics from your review.

Thank you again for that very valuable review!

[cdamus]

• reorienting the message starting the nested execution moves now nicely the views. A reparentView was done while setting the owner of the executions, and so all moved elements views were added to the lifeline body figure. In the case of the nested execution, this is useless, as its parent is the nesting execution one. A check is now performed so it does not reparent views if not needed. A functional test was also added to check that layout is good on retargeting.

Ah, that makes sense. Good.

• For the nesting of executions, according to some live discussions with Antonio, it seems better to always cascade in the same direction, in case of big executions spanning across several computer screens. In this case, it would be easier to detect which one is the nesting one, and which are the nested ones if they are always cascading on the same side. So I propose to always keep the nesting on the right currently. We may update later if the users think this is not practical.

Okay, I can understand that motivation. Certainly, the worst would be to have nesting switch orientations within a stack, so this is a good compromise.

• for the side of the anchors, this is a bigger issue on executions in general. This will be handled in issue Messages attached to executions should swap sides when lifeline order is changed #376. I added a comment in that issue to ensure it is tested in the nested execution cases. I already started to implement some tests for that issue locally (currently failing of course!). This should be my next task.

Cool. TDD FTW 😉

• Good catch for the issue of creating an execution on connecting a sync message! I will check what is not going there, probably some edit policy missing on the execution similar to the one on lifeline body.

Yes, that sounds likely. Another good motivation for moving more of this logic out of the edit-policies and into the logical model where it can be shared by the edit-policies.

Finally, for the code formatting: looking at the history, it seems we have some trouble to keep consistency in formatting. I suspect we use different versions of EMF to generate code and / or do not apply the same actions after code generation. I did a format source code after generation / organize import and then a clean up using project profile (what I usually do when generating Papyrus diagram code). Even if I revert the edit plugin code and generate the code again, I get a lot of differences compared to the master branch. I am using EMF 2.14 for code generation.

Yeah, I'm using Eclipse for Committers 2018-09, so that's EMF 2.15. Those templates can have some changes, hopefully only improvements, because the genmodel is still targeting EMF 2.14 run-time. However, our project configurations for JDT formatting and clean-up should take care of purely formatting issues (though the generated code may actually be semantically different).

Probably we should standardize on the development environment, perhaps via an Oomph setup model for this project.

[cdamus]

Thanks for the changes, @rschnekenbu! I would just ask for one more change to update a test that skips on Mac but can readily be fixed to work on all platforms.

[cdamus]

🎉

[rschnekenbu]

The test failure should be fixed now, thanks to commit 4ac2cc3

[cdamus]

Yup! Bingo. 👍

[rschnekenbu]

Solving conflicts by merging the master branch. This commit should be hopefully the last one on this PR!