Issue #389: self message management

- simple fixes based on PR review - fix transformation messages to self messages when relevant during move of an execution - Add a similar behavior between moving the end of a sync message starting an execution to another lifeline and the move of the execution itself. Change-Id: I19c9b868f016401fa71a9c5f65c82093fe9ddd8b Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
eclipsesource · Jan 4, 2019 · a5b424c · a5b424c
1 parent 26342bd
commit a5b424c
Show file tree

Hide file tree

Showing 10 changed files with 75 additions and 32 deletions.
diff --git a/.../eclipse/papyrus/uml/diagram/sequence/figure/anchors/ExecutionSpecificationEndAnchor.java b/.../eclipse/papyrus/uml/diagram/sequence/figure/anchors/ExecutionSpecificationEndAnchor.java
@@ -40,7 +40,7 @@ public void setConnectionSide(int side) {
 	public Point getReferencePoint() {
 		Rectangle body = getOwner().getBounds().getCopy();
 		getOwner().translateToAbsolute(body);
-		return body.getBottomLeft();
+		return body.getBottom();
 	}
 
 	@Override

diff --git a/...eclipse/papyrus/uml/diagram/sequence/figure/anchors/ExecutionSpecificationSideAnchor.java b/...eclipse/papyrus/uml/diagram/sequence/figure/anchors/ExecutionSpecificationSideAnchor.java
@@ -30,7 +30,7 @@ public class ExecutionSpecificationSideAnchor extends AbstractConnectionAnchor i
 
 	public ExecutionSpecificationSideAnchor(IFigure figure, int height) {
 		super(figure);
-		this.side = RIGHT;
+		this.side = RIGHT; // by default, anchors are located on the right.
 		this.height = height;
 	}
 

diff --git a/...org/eclipse/papyrus/uml/diagram/sequence/runtime/internal/edit/parts/MessageEditPart.java b/...org/eclipse/papyrus/uml/diagram/sequence/runtime/internal/edit/parts/MessageEditPart.java
@@ -151,6 +151,9 @@ private void refreshLifeline(MLifeline lifeline) {
 	}
 
 	private void refreshEditPartOfShape(Shape shape) {
+		if(getViewer() == null) {
+			return;
+		}
 		Object lifelineEditPart = getViewer().getEditPartRegistry().get(shape);
 		if (lifelineEditPart instanceof EditPart) {
 			EditPart editPart = (EditPart)lifelineEditPart;
@@ -245,7 +248,11 @@ protected void refreshRouterChange() {
 
 	@Override
 	protected void handlePropertyChangeEvent(PropertyChangeEvent event) {
-
+		// Avoid calling super, as it only handles the refresh of the routing property. The super method
+		// refreshes the router based on the routing style of the notation model. In the case of feedback, the
+		// router may be different (getting from a self message to a normal message for example). So refresh
+		// should be avoided in this case, and connection router shall be updated on notation model change
+		// only. The feedback router is handled in the feedback related methods.
 	}
 
 	@Override

diff --git a/...gram/sequence/runtime/internal/edit/policies/AbstractSequenceGraphicalNodeEditPolicy.java b/...gram/sequence/runtime/internal/edit/policies/AbstractSequenceGraphicalNodeEditPolicy.java
@@ -14,7 +14,6 @@
 
 import static java.lang.Math.abs;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.internal.edit.policies.PrivateRequestUtils.getUpdatedSourceLocation;
-import static org.eclipse.papyrus.uml.diagram.sequence.runtime.internal.edit.policies.PrivateRequestUtils.getUpdatedTargetLocation;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.internal.edit.policies.PrivateRequestUtils.isAllowSemanticReordering;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.internal.edit.policies.PrivateRequestUtils.isForce;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.internal.util.CommandUtil.injectViewInto;
@@ -42,6 +41,7 @@
 import org.eclipse.draw2d.geometry.Point;
 import org.eclipse.emf.common.command.CommandWrapper;
 import org.eclipse.emf.ecore.EClass;
+import org.eclipse.emf.ecore.EObject;
 import org.eclipse.emf.transaction.TransactionalEditingDomain;
 import org.eclipse.gef.EditPart;
 import org.eclipse.gef.LayerConstants;
@@ -479,15 +479,17 @@ protected Command getReconnectSourceCommand(ReconnectRequest request) {
 		OptionalInt yPosition = OptionalInt.of(y);
 
 		org.eclipse.emf.common.command.Command result = null;
+
 		// Check for semantic re-ordering if we're not connecting to an execution
 		if (!getExecutionFinish(lifeline.get(), request.getLocation()).isPresent()
-				// Or if we're moving both ends and the other is not connecting to an execution
-				&& !(isForce(request) && message.getReceiver()
-						.flatMap(rcvr -> getExecutionStart(rcvr, getUpdatedTargetLocation(request)))
-						.isPresent())) {
+		// Or if we're moving both ends and the other is not connecting to an execution
+		/*
+		 * && !(isForce(request) && message.getReceiver() .flatMap(rcvr -> getExecutionStart(rcvr,
+		 * getUpdatedTargetLocation(request))) .isPresent())
+		 */) {
+			// should ensure the execution start is nudged
 			result = getNudgeObstacleCommand(request, message.getSend().get(), y);
 		}
-
 		// If the message didn't have a send end, we wouldn't be reconnecting it
 		MMessageEnd sourceEnd = message.getSend().get();
 		org.eclipse.emf.common.command.Command createFinish = null;
@@ -573,6 +575,16 @@ protected Command getReconnectTargetCommand(ReconnectRequest request) {
 				&& (lifeline.get().getElement() != message.getSender().get().getElement())) {
 			// Doesn't matter where the mouse pointer is
 			y = flatMapToInt(message.getSend(), MElement::getTop).orElse(y);
+		} else if (isForce(request)) {
+			Optional<EObject> sourceLL = message.getSender().map(MElement::getElement);
+			Optional<EObject> targetLL = message.getReceiver().map(MElement::getElement);
+			if (sourceLL.isPresent() && sourceLL.equals(targetLL)) {
+				// this is a self message, target position should be the new source position + the current
+				// height of message
+				int bottom = message.getBottom().getAsInt();
+				int top = message.getTop().getAsInt();
+				y = y + 0;
+			}
 		}
 		OptionalInt yPosition = OptionalInt.of(y);
 

diff --git a/...pyrus/uml/diagram/sequence/runtime/internal/edit/policies/LifelineBodyDropEditPolicy.java b/...pyrus/uml/diagram/sequence/runtime/internal/edit/policies/LifelineBodyDropEditPolicy.java
@@ -79,21 +79,24 @@ protected Command getDropExecutionCommand(MExecution execution, DropObjectsReque
 			return UnexecutableCommand.INSTANCE;
 		}
 
-		Optional<Point> changeBounds = Optional.ofNullable(getChangeBoundsRequest(request))
-				.map(ChangeBoundsRequest::getMoveDelta);
-		OptionalInt deltaY = mapToInt(changeBounds, Point::y);
-		OptionalInt top = flatMap(execution.getTop(), t -> map(deltaY, y -> t + y));
-		OptionalInt bottom = flatMap(execution.getBottom(), b -> map(deltaY, y -> b + y));
-
 		// check if there is a change of lifeline, in this case, opt for the start event cover change
 		// this allows for nice handling of self messages.
+		// note that this forbids the execution to change lifeline and move along the new lifeline
 		if (lifeline.isPresent() && !lifeline.get().equals(execution.getOwner())) {
 
 			Optional<MOccurrence<? extends Element>> start = execution.getStart();
 			if (start.isPresent()) {
-				return lifeline.map(ll -> wrap(start.get().setCovered(ll, top))).orElse(null);
+				return lifeline.map(ll -> wrap(start.get().setCovered(ll, execution.getTop()))).orElse(null);
 			}
 		}
+
+		// if not, let the execution move along the line.
+		Optional<Point> changeBounds = Optional.ofNullable(getChangeBoundsRequest(request))
+				.map(ChangeBoundsRequest::getMoveDelta);
+		OptionalInt deltaY = mapToInt(changeBounds, Point::y);
+		OptionalInt top = flatMap(execution.getTop(), t -> map(deltaY, y -> t + y));
+		OptionalInt bottom = flatMap(execution.getBottom(), b -> map(deltaY, y -> b + y));
+
 		// default behavior, use the set owner API.
 		return lifeline.map(ll -> wrap(execution.setOwner(ll, top, bottom))).orElse(null);
 	}

diff --git a/...el/src/org/eclipse/papyrus/uml/interaction/internal/model/commands/SetCoveredCommand.java b/...el/src/org/eclipse/papyrus/uml/interaction/internal/model/commands/SetCoveredCommand.java
@@ -224,11 +224,11 @@ private boolean isTargetEnd() {
 	}
 
 	protected boolean isBecomingSelfMessage() {
-		return isChangingLifeline() && willBeSelfMessage() && !wasSelfMessage();
+		return willBeSelfMessage() && !wasSelfMessage();
 	}
 
 	protected boolean isBecomingNonSelfMessage() {
-		return isChangingPosition() && wasSelfMessage() && !willBeSelfMessage();
+		return wasSelfMessage() && !willBeSelfMessage();
 	}
 
 	protected boolean wasSelfMessage() {
@@ -335,13 +335,15 @@ private void collectMessageDependencies(MMessageEnd end, boolean isDisconnecting
 				commandSink.accept(defer(
 						() -> reconnectSource(connector, newAttachedShape.get(), newYPosition).orElse(null)));
 				Optional<MMessageEnd> otherEnd = end.getOtherEnd();
-				otherEnd.flatMap(oEnd -> handleSelfMessageChange(oEnd, newYPosition)).ifPresent(commandSink);
+				// otherEnd.flatMap(oEnd -> handleSelfMessageChange(oEnd,
+				// newYPosition)).ifPresent(commandSink);
 				otherEnd.flatMap(this::handleOppositeSendOrReplyMessage).ifPresent(commandSink);
 			} else if (end.isReceive()) {
 				commandSink.accept(defer(
 						() -> reconnectTarget(connector, newAttachedShape.get(), newYPosition).orElse(null)));
 				Optional<MMessageEnd> otherEnd = end.getOtherEnd();
-				otherEnd.flatMap(oEnd -> handleSelfMessageChange(oEnd, newYPosition)).ifPresent(commandSink);
+				// otherEnd.flatMap(oEnd -> handleSelfMessageChange(oEnd,
+				// newYPosition)).ifPresent(commandSink);
 				otherEnd.flatMap(this::handleOppositeSendOrReplyMessage).ifPresent(commandSink);
 			} // else don't know what to do with it
 		}
@@ -373,7 +375,7 @@ private void collectMessageDependencies(MMessageEnd end, boolean isDisconnecting
 					// Track this end
 					final OptionalInt otherNewY;
 					if (end.isStart()) {
-						if (wasSelfMessage() && willBeSelfMessage()) {
+						if (!wasSelfMessage() && willBeSelfMessage()) {
 							otherNewY = OptionalInt.of(PendingVerticalExtentData
 									.getPendingTop(end.getExecution().get()).getAsInt()
 									- layoutHelper().getConstraints().getMinimumHeight(ViewTypes.MESSAGE));
@@ -415,7 +417,7 @@ private void collectMessageDependencies(MMessageEnd end, boolean isDisconnecting
 				// only a simple occurrence on the execution. Move using delta
 				if (otherEnd.isReceive()) {
 					// add an additional room for the self message
-					if (isBecomingSelfMessage() && !wasSelfMessage()) {
+					if (isBecomingSelfMessage()) {
 						// ensure the position has a least the space for self messages, but it could be bigger
 						int minLayout = yPosition.getAsInt()
 								+ layoutHelper().getConstraints().getMinimumHeight(ViewTypes.MESSAGE);
@@ -544,7 +546,6 @@ Optional<Command> handleSelfMessageChange(MMessageEnd otherEnd, int newYPosition
 			Command bend;
 			if (otherEnd.isSend()) {
 				// We're reconnecting the source end, but we need to maintain the gap
-				// int newYPosition = currentYPosition - height;
 				Shape newAttachShape = executionShapeAt(lifeline, currentYPosition).orElse(lifelineBody);
 				bend = reconnectSource(connector, newAttachShape, currentYPosition)
 						.orElse(IdentityCommand.INSTANCE);

diff --git a/...odel/src/org/eclipse/papyrus/uml/interaction/internal/model/commands/SetOwnerCommand.java b/...odel/src/org/eclipse/papyrus/uml/interaction/internal/model/commands/SetOwnerCommand.java
@@ -245,6 +245,22 @@ protected Optional<Command> dependencies(MExecution execution, MLifeline lifelin
 			// that *will be* spanned do not; they only must be reattached per the step below
 			result = execution.getOccurrences().stream()
 					.map(occ -> occ.setCovered(lifeline, topMapping.apply(occ.getTop()))).reduce(chaining());
+
+			if (isStartingSelfMessage(execution)) {
+				// specific case of a self message starting the execution => it should also be moved of delta
+				final Optional<Command> moveInitial = execution.getStart().map(MMessageEnd.class::cast)
+						.flatMap(MMessageEnd::getOtherEnd)
+						.map(occ -> occ.setCovered(lifeline, topMapping.apply(occ.getTop())));
+
+				if (moveInitial.isPresent()) {
+					if (result.isPresent()) {
+						result.get().chain(moveInitial.get());
+					} else {
+						result = moveInitial;
+					}
+				}
+			}
+
 		} else if (isChangingPosition()) {
 			if (!isChangingOwner()) {
 				// We are reshaping it. Refresh nested executions, if necessary

diff --git a/...src/org/eclipse/papyrus/uml/interaction/internal/model/spi/impl/DefaultDiagramHelper.java b/...src/org/eclipse/papyrus/uml/interaction/internal/model/spi/impl/DefaultDiagramHelper.java
@@ -469,7 +469,7 @@ public Command reconnectSource(Connector connector, Shape newSource, int yPositi
 
 		// should force?
 		if (newRouting != connector.getRouting()) {
-			result.chain(SetCommand.create(editingDomain, connector,
+			result = result.chain(SetCommand.create(editingDomain, connector,
 					NotationPackage.Literals.ROUTING_STYLE__ROUTING, newRouting));
 		}
 
@@ -515,7 +515,7 @@ public Command reconnectTarget(Connector connector, Shape newTarget, int yPositi
 		}
 		// should force?
 		if (newRouting != connector.getRouting()) {
-			result.chain(SetCommand.create(editingDomain, connector,
+			result = result.chain(SetCommand.create(editingDomain, connector,
 					NotationPackage.Literals.ROUTING_STYLE__ROUTING, newRouting));
 		}
 

diff --git a/...ml/diagram/sequence/runtime/internal/edit/policies/tests/ExecutionWithSelfMoveUITest.java b/...ml/diagram/sequence/runtime/internal/edit/policies/tests/ExecutionWithSelfMoveUITest.java
@@ -110,23 +110,22 @@ public void moveDownExecution() {
 
 	@Test
 	public void moveUpExecution() {
-		int delta = -50;
+		int delta = -30;
 
 		Point grabAt = execution1Rect.getCenter();
 		Point dropAt = execution1Rect.getCenter().translate(0, delta);
 		editor.moveSelection(grabAt, dropAt);
 
 		// check result
 		Rectangle exec1EPNew = execution1Rect.getCopy().translate(new Point(0, delta));
-		EditPart exec1EP = findEditPart("execution-self::Interaction1::Execution1", false);
-		assertThat(getBounds(exec1EP), equalTo(exec1EPNew));
 		PointList newRequestPoints = requestPoints.getCopy();
 		newRequestPoints.translate(0, delta);
 		assertThat(requestEP, runs(exec1EPNew.getCenter().x(), exec1EPNew.getTop().y() - 20,
 				exec1EPNew.getTopRight().x(), exec1EPNew.getTopRight().y()));
 
-		PointList newReplyPoints = replyPoints.getCopy();
-		newReplyPoints.translate(0, delta);
+		EditPart exec1EP = findEditPart("execution-self::Interaction1::Execution1", false);
+		assertThat(getBounds(exec1EP), equalTo(exec1EPNew));
+
 		assertThat(replyEP, runs(exec1EPNew.getBottomRight().x(), exec1EPNew.getBottomRight().y(),
 				exec1EPNew.getCenter().x(), exec1EPNew.getBottom().y() + 20));
 	}

diff --git a/...yrus/uml/diagram/sequence/runtime/internal/edit/policies/tests/MessageSnappingUITest.java b/...yrus/uml/diagram/sequence/runtime/internal/edit/policies/tests/MessageSnappingUITest.java
@@ -14,6 +14,7 @@
 
 import static org.eclipse.papyrus.uml.diagram.sequence.figure.magnets.IMagnetManager.MODIFIER_NO_SNAPPING;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.tests.matchers.GEFMatchers.isPoint;
+import static org.eclipse.papyrus.uml.diagram.sequence.runtime.tests.matchers.GEFMatchers.EditParts.isBounded;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.tests.matchers.GEFMatchers.EditParts.runs;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.tests.rules.EditorFixture.at;
 import static org.eclipse.papyrus.uml.diagram.sequence.runtime.tests.rules.EditorFixture.sized;
@@ -184,17 +185,20 @@ public void createReplyMessage() {
 
 	@Test
 	public void moveSyncCallMessage() {
+		int execTop = getTop(execEP);
+		int size = getBottom(execEP) - execTop;
 		EditPart messageEP = createConnection(SequenceElementTypes.Sync_Message_Edge, at(LL1_BODY_X, 120),
 				at(LL2_BODY_X, 120));
 		Point midMessage = getMessageGrabPoint(messageEP);
 
 		// The receiving end snaps to the exec start and the sending end matches
-		editor.with(modifiers,
+		editor.with(modifiers.and(editor.allowSemanticReordering()),
 				() -> editor.moveSelection(midMessage, at(midMessage.x(), withinMagnet(EXEC_START))));
 		Point newMessageMidpoint = getMessageGrabPoint(messageEP);
 		assumeThat("Message not moved", newMessageMidpoint, not(isPoint(midMessage.x(), midMessage.y(), 5)));
 
-		int execTop = getTop(execEP);
+		// check exec position
+		assertThat(execEP, isBounded(left(LL2_BODY_X), execTop, EXEC_WIDTH, size));
 		assertThat(messageEP, withModifiers(runs(LL1_BODY_X, execTop, left(LL2_BODY_X), execTop, 1)));
 	}
 
@@ -205,6 +209,7 @@ public void moveReplyMessage() {
 		Point midMessage = getMessageGrabPoint(messageEP);
 
 		// The sending end snaps to the exec start and the receiving end matches
+		// only if the semantic reordering is activated. Otherwise, it will only push down the exec spec
 		editor.with(modifiers,
 				() -> editor.moveSelection(midMessage, at(midMessage.x(), withinMagnet(EXEC_FINISH))));
 		int execBottom = getBottom(execEP);