Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue #343 Restrict message end movement #331

Merged
merged 13 commits into from
Sep 12, 2018
Merged

Fix issue #343 Restrict message end movement #331

merged 13 commits into from
Sep 12, 2018

Conversation

cdamus
Copy link
Collaborator

@cdamus cdamus commented Jul 30, 2018

Note that this pull request is based on pull request #330 for issue #73, which is not yet merged.

Implement the requirement to not allow movement of a message end to result in the other end being moved also to maintain semantic integrity. Thus, only the end grabbed by the user can be moved and
then only if the resulting message (the other end always staying put) would be valid.

@planger planger mentioned this pull request Aug 10, 2018
Closed
4 tasks
@cdamus
Issue eclipsesource#73 Execution start/finish on message ends
48c8ae1 
Update alignment constraints and feed-back of message
creation to enforce horizontality of non-asynchronous
messages by recalculating the sending end instead of the
receiving end because the receiving end is generally
more significant.

Implement a magnet mechanism to snap connections to
the ends of an execution specification.  Upon creation of
a message connecting to either end of an execution
specification, replace the occurrence at that end by the
message end.

Register a palette provider that injects magnet manager
support into the selection tool and connection creation
tools to support user override of the magnet snapping
behaviour when drawing and moving connections, using
the same keyboard modifier as for suppression of the
snapping of shapes.

Update a create message test to account for messages
anchored on the beginning of an execution specification
correctly remaining attached when the execution moves.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>

# Conflicts:
#	plugins/org.eclipse.papyrus.uml.interaction.model/src/org/eclipse/papyrus/uml/interaction/internal/model/commands/InsertMessageCommand.java
#	plugins/org.eclipse.papyrus.uml.interaction.model/src/org/eclipse/papyrus/uml/interaction/internal/model/spi/impl/DefaultDiagramHelper.java
#	plugins/org.eclipse.papyrus.uml.interaction.model/src/org/eclipse/papyrus/uml/interaction/model/spi/DiagramHelper.java
@cdamus
Fix issue eclipsesource#73: Semantic ordering of fragments
1c125a9 
Fix the semantic ordering of fragments on creation of
a message, inserting new message ends more correctly
relative to the existing interaction fragments in the
interaction’s ordered fragments list.  This removes the
need for work-arounds in the tests that post facto
adjusted semantic ordering.
@cdamus
Issue eclipsesource#73: Empiricism in the tests
1f60803 
Measure the actual top and bottom of the execution specification
as actually rendered differently on different platforms, instead of
assuming that it ends up exactly where requested via the API.
@cdamus
Issue eclipsesource#73: Empirical message grabbing
e35e2f5 
Get the actual location at which to grab the message to move
it in message moving tests, to account for platform variability.
@cdamus
Fix issue eclipsesource#343 Restrict message end movement
f39f2eb 
Implement the requirement to not allow movement of a message end
to result in the other end being moved also to maintain semantic
integrity.  Thus, only the end grabbed by the user can be moved and
then only if the resulting message (the other end always staying put)
would be valid.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>

# Conflicts:
#	plugins/org.eclipse.papyrus.uml.diagram.sequence.runtime/src/org/eclipse/papyrus/uml/diagram/sequence/runtime/internal/edit/policies/AbstractSequenceGraphicalNodeEditPolicy.java
planger
planger requested changes Sep 10, 2018
View reviewed changes
Copy link
Collaborator

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! This mostly works very well. I just encountered two bugs while testing:

  1. Moving the send message end of a sync message connected to the start of an execution downwards leads to a down-sloped sync message pointing to the middle of the execution.
    Initial scenario:
    image
    Now select the message, grab the handle on the send event message end and move it downwards and drop the message end. Then I get this situation:
    image

  2. Strictly speaking, this is not a message end move problem, but a message move problem in general (so feel free to address this in a separate issue). Given the same initial scenario as above, when grabbing the message in the middle and moving it upwards above the start of the lifeline, we end up with this scenario:
    image

Thanks!

@planger
Copy link
Collaborator

planger commented Sep 10, 2018

One more thing: I was able to drop the receive end of the message on the lifeline header or even above too by dragging it towards or beyond the y-posiiton of the lifeline header.
image
Thanks!

@cdamus
Copy link
Collaborator Author

cdamus commented Sep 10, 2018

The connection to the lifeline probably is happening because of the rebase, which brings in message anchors on the lifeline head that were implemented in parallel for create messages. So, I'll need to do some work to disable the move to the lifeline head.

No immediate thoughts on the other problems. I'll look into them.

@cdamus cdamus mentioned this pull request Sep 10, 2018
Closed
@cdamus
Issue eclipsesource#343 No connection to LL heads
0fbbe3b 
Do not permit connection to the lifeline head.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
@cdamus
Issue eclipsesource#343 Msg end ordering for executions
5bc6937 
When drawing a message replaces an execution occurrence
specification that starts or finishes an execution specification,
the ordering of the message ends would end up reversed
in the interaction fragments (the message being received
before it is sent) and also (in the case of starting the execution
specification) after the execution specification, itself.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
@cdamus
Issue eclipsesource#343 Message anchoring on move
eaa5ad7 
Fix some mistakes in the anchoring of message ends
to lifelines and execution specifications when moving
messages such that a message end moves from an
execution specification to a lifeline or vice-versa.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
@cdamus
Copy link
Collaborator Author

cdamus commented Sep 10, 2018

I've fixed a few more of the problems found in @planger's testing (thanks). There is still an issue in the visual feed-back during message move remaining stuck at the start or finish of an execution specification. A naïve fix that lets execution specifications have left- or right-side anchors with negative values or values greater than the length of the execution fixes that problem, but doesn't really make sense and breaks a JUnit test for create messages. So, I need to work on letting the FeedbackHelper change not just anchor identity but also the connected node figure at each end, which determines the kind of anchor.

planger
planger previously approved these changes Sep 11, 2018
View reviewed changes
@planger
Copy link
Collaborator

planger commented Sep 11, 2018 •
edited by cdamus
Loading

@cdamus Awesome, thanks! Code looks good so far. So I think we can approve all changes up until now.

There are just a few more remaining issues. Here is the summary of the results of re-testing with these changes merged into current master:

  • The issue of dragging a message end connected to an execution to a lifeline header is fixed
  • Adding an execution to Lifeline2 (see Nudging doesn't work reliably #343 (comment)) now doesn't break the semantic ordering anymore
  • The semantic order breaks if we add an execution as a first position of Lifeline3
    image
    image
  • The semantic order breaks (imho) when we add an execution to Lifeline1. I would have expected that the execution is added before the 3s and 3r.
    image
    image
  • Message ends can still be dragged to the interaction: drag them to the interaction label
  • Messages can still be dragged to the interaction: grab them in the middle, send event is moved according to the feedback, and drop them at the interaction label; the message ends up as a reflexiv connection in the lower right corner of the interaction (actually we could keep that as a funny easter-egg ;))
  • As you have mentioned, the feedback is not ideal, but imho we can keep this open and handle in the separate issue.

@planger planger mentioned this pull request Sep 12, 2018
Closed
planger and others added 3 commits September 12, 2018 08:57
@planger @cdamus
Add tests for semantic ordering after creation on top
03af9e0 
Adds tests for issues eclipsesource#343 and eclipsesource#347. To enable more efficient adding of
testing, this change also refactors the tests a bit and extracts common
methods into dedicated model fixture.

Fix wrong name of receive of message 2

Improve ModelEditFixture.isSemanticallyBefore()

Signed-off-by: Philip Langer <planger@eclipsesource.com>
@cdamus
Issue eclipsesource#343 code review comments
a6d4e7a 
Address comments from code review of tests contribution
for issue eclipsesource#343.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
@cdamus
Merge branch '343-add-tests' into 343-restrict-msg-ends
ca64b47
@cdamus cdamus dismissed planger’s stale review September 12, 2018 13:06

Intend to follow up additional issues in this PR.

@cdamus
Issue eclipsesource#343: Insertion problems
66011de 
Fix the semantic insertion order of interaction fragments
in the case of creating an execution specification.

Includes fixing a test that didn’t account for tear-down of
the logical model when the model is changed.  Also adding
the Papyrus di resources to git for ease of reference to the
notation in a Papyrus run-time workbench.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
@cdamus
Copy link
Collaborator Author

cdamus commented Sep 12, 2018

@planger I have pushed a new commit that fixes the execution-specification insertion problems (I have checked them off in your comment). Thus only the "Easter egg" and feedback issues remain, which as discussed off-line are of lower priority and can be deferred.

planger
planger approved these changes Sep 12, 2018
View reviewed changes
Copy link
Collaborator

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks a lot! Works fine and the tests pass now. (y) I'll create new gh issues for the two remaining problems. I'll do some more in depth testing in the next days, but this shouldn't hold up merging this branch imho.

@planger planger merged commit 771e6b3 into eclipsesource:master Sep 12, 2018
This was referenced Sep 12, 2018
Closed
Closed
@planger
Copy link
Collaborator

planger commented Sep 12, 2018

I've opened #349 and #350 for the remaining issues mentioned in #331 (comment).

@cdamus cdamus deleted the 343-restrict-msg-ends branch September 12, 2018 20:42
@planger planger mentioned this pull request Nov 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@planger planger planger approved these changes

@rschnekenbu rschnekenbu Awaiting requested review from rschnekenbu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cdamus @planger