[571925] Use options Priority and Node Placement Strategy

This commit adds a bigger "Priority" [1] on edges used as source or
target to give preferential treatment for this edge (more flat than
edges connected to it). This option must be used in correlation with
"NETWORK_SIMPLEX" as "Node Placement Strategy" [2]. Otherwise, the
"Priority" is ignored.
As conclusion, the result is better with this commit.

[1] https://www.eclipse.org/elk/reference/options/org-eclipse-elk-priority_org-eclipse-elk-layered.html
[2] https://www.eclipse.org/elk/reference/options/org-eclipse-elk-layered-nodePlacement-strategy.html

Bug: 571925
Change-Id: I8864fff9661570e15cfb838c4a8537d40bc9947b
Signed-off-by: Laurent Redor <laurent.redor@obeo.fr>

5 files changed, 87 insertions, 24 deletions

diff --git a/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/ElkDiagramLayoutConnector.java b/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/ElkDiagramLayoutConnector.java
index 649ec13376..9a8a9491e5 100644
--- a/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/ElkDiagramLayoutConnector.java
+++ b/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/ElkDiagramLayoutConnector.java
@@ -212,6 +212,12 @@ public class ElkDiagramLayoutConnector implements IDiagramLayoutConnector {
     public static final IProperty<Boolean> NODE_SIZE_FIXED_SIZE = new Property<Boolean>("org.eclipse.sirius.diagram.elk.fixedNodeSize", false, null, null);
 
     /**
+     * Value used for split edge for property {@link LayeredOptions#PRIORITY}. It allows to give the priority for
+     * "straight aspect" to split edges (compared to edges using split edge as source or target).
+     */
+    private static final int PRIORITY_SPLIT_EDGE_VALUE = 10;
+
+    /**
      * Value used for split edge for property {@link LayeredOptions#PRIORITY_STRAIGHTNESS}. It allows to give the
      * priority for "straight aspect" to split edges (compared to edges using split edge as source or target).
      */
@@ -1641,8 +1647,9 @@ public class ElkDiagramLayoutConnector implements IDiagramLayoutConnector {
             // Use the existing edge as the first part of the split edge (from source to intermediate node).
             // To force that this edge is as straight as possible (better layout result), use the layered option
             // "LayeredOptions.PRIORITY_STRAIGHTNESS". 10 is used arbitrary (more than 1 by default).
-            // TODO : This is OK only if Layered layout is used. See if there is equivalent option for other kind of
-            // layout.
+            // TODO : Tested with Layered layout and nodePlacement.strategy==NETWORK_SIMPLEX. See later if there are
+            // equivalent options for other kind of layout.
+            edgeToSplit.setProperty(CoreOptions.PRIORITY, PRIORITY_SPLIT_EDGE_VALUE);
             edgeToSplit.setProperty(LayeredOptions.PRIORITY_STRAIGHTNESS, PRIORITY_STRAIGHTNESS_SPLIT_EDGE_VALUE);
             edgeToSplit.getTargets().add(intermediateNode);
             // TODO : There is maybe something specific to handle edge's labels (coordinates are maybe wrong after this
diff --git a/plugins/org.eclipse.sirius.doc/doc/Release_Notes.html b/plugins/org.eclipse.sirius.doc/doc/Release_Notes.html
index edf91f32a7..d9de02fe26 100644
--- a/plugins/org.eclipse.sirius.doc/doc/Release_Notes.html
+++ b/plugins/org.eclipse.sirius.doc/doc/Release_Notes.html
@@ -181,8 +181,8 @@
 		</ul>
 		<p>order, despite this problem, to have a satisfactory layout result. This new behavior has been tested with 
 			<a href="https://www.eclipse.org/elk/reference/algorithms/org-eclipse-elk-layered.html">ELK Layered</a> algorithm with option 
-			<a href="https://www.eclipse.org/elk/reference/options/org-eclipse-elk-layered-layering-strategy.html">
-				<code>Node Layering Strategy</code>
+			<a href="https://www.eclipse.org/elk/reference/options/org-eclipse-elk-layered-nodePlacement-strategy.html">
+				<code>Node Placement Strategy</code>
 			</a> set to 
 			<code>NETWORK_SIMPLEX</code>. There is no guarantee with other kind of layouts. All cases are not supported. Further additional developments are needed to support more. There are probably still some constructions not correctly handled but it is a first step. As example, the edges with label(s) have not been tested. Here is a list of supported/tested cases:
 		</p>
diff --git a/plugins/org.eclipse.sirius.doc/doc/Release_Notes.textile b/plugins/org.eclipse.sirius.doc/doc/Release_Notes.textile
index 7d2ba42401..59d8df5337 100644
--- a/plugins/org.eclipse.sirius.doc/doc/Release_Notes.textile
+++ b/plugins/org.eclipse.sirius.doc/doc/Release_Notes.textile
@@ -16,7 +16,7 @@ h3. Specifier-Visible Changes
 
 * <span class="label label-success">Added</span> A new @CellEditor@ tool is available for feature column mapping of edition table. It allows to define a specific @org.eclipse.jface.viewers.CellEditor@ to edit a cell (see "documentation":specifier/tables/Tables.html#column_tools for more details).
 * <span class="label label-info">Modified</span> ELK integration now handles "egde on edge" case. As reminder, Sirius allows to have an edge as source or target of another edge. This kind of construction is not allowed by ELK. Sirius 6.5.0 introduces a specific transformation from Sirius graph to EKL graph (and reciprocally) in
-order, despite this problem, to have a satisfactory layout result. This new behavior has been tested with "ELK Layered":https://www.eclipse.org/elk/reference/algorithms/org-eclipse-elk-layered.html algorithm with option "@Node Layering Strategy@":https://www.eclipse.org/elk/reference/options/org-eclipse-elk-layered-layering-strategy.html set to @NETWORK_SIMPLEX@. There is no guarantee with other kind of layouts. All cases are not supported. Further additional developments are needed to support more. There are probably still some constructions not correctly handled but it is a first step. As example, the edges with label(s) have not been tested. Here is a list of supported/tested cases:
+order, despite this problem, to have a satisfactory layout result. This new behavior has been tested with "ELK Layered":https://www.eclipse.org/elk/reference/algorithms/org-eclipse-elk-layered.html algorithm with option "@Node Placement Strategy@":https://www.eclipse.org/elk/reference/options/org-eclipse-elk-layered-nodePlacement-strategy.html set to @NETWORK_SIMPLEX@. There is no guarantee with other kind of layouts. All cases are not supported. Further additional developments are needed to support more. There are probably still some constructions not correctly handled but it is a first step. As example, the edges with label(s) have not been tested. Here is a list of supported/tested cases:
 ** An edge having another edge as source,
 ** An edge having another edge as target,
 ** An edge that is the source of several edges,
diff --git a/plugins/org.eclipse.sirius.tests.junit/data/unit/layout/withELK/My.odesign b/plugins/org.eclipse.sirius.tests.junit/data/unit/layout/withELK/My.odesign
index e8d5290720..8458d53e0e 100644
--- a/plugins/org.eclipse.sirius.tests.junit/data/unit/layout/withELK/My.odesign
+++ b/plugins/org.eclipse.sirius.tests.junit/data/unit/layout/withELK/My.odesign
@@ -725,6 +725,16 @@
         <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers" value="50.0">
           <targets>PARENT</targets>
         </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeEdgeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeNodeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:EnumLayoutOption" id="org.eclipse.elk.layered.nodePlacement.strategy">
+          <targets>PARENT</targets>
+          <value name="NETWORK_SIMPLEX"/>
+        </layoutOptions>
       </layout>
       <defaultLayer name="Default">
         <edgeMappings name="SuperTypesAndRef" sourceMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_subClasses']/@defaultLayer/@containerMappings[name='Classes']" targetMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_subClasses']/@defaultLayer/@edgeMappings[name='References']" targetFinderExpression="aql:self.eReferences->select(ref | self.eSuperTypes->includes(ref.eType))">
@@ -800,6 +810,16 @@
         <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers" value="50.0">
           <targets>PARENT</targets>
         </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeEdgeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeNodeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:EnumLayoutOption" id="org.eclipse.elk.layered.nodePlacement.strategy">
+          <targets>PARENT</targets>
+          <value name="NETWORK_SIMPLEX"/>
+        </layoutOptions>
       </layout>
       <defaultLayer name="Default">
         <edgeMappings name="SuperTypesAndRef" sourceMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_subClassesReverse']/@defaultLayer/@containerMappings[name='Classes']" targetMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_subClassesReverse']/@defaultLayer/@edgeMappings[name='References']" targetFinderExpression="aql:self.eReferences->select(ref | self.eSuperTypes->includes(ref.eType))">
@@ -875,6 +895,16 @@
         <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers" value="50.0">
           <targets>PARENT</targets>
         </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeEdgeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeNodeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:EnumLayoutOption" id="org.eclipse.elk.layered.nodePlacement.strategy">
+          <targets>PARENT</targets>
+          <value name="NETWORK_SIMPLEX"/>
+        </layoutOptions>
       </layout>
       <defaultLayer name="Default">
         <edgeMappings name="References" sourceMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackage']/@defaultLayer/@containerMappings[name='Classes']" targetMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackage']/@defaultLayer/@containerMappings[name='Classes']" targetFinderExpression="feature:eType" sourceFinderExpression="feature:eContainer" domainClass="ecore::EReference" useDomainElement="true">
@@ -942,6 +972,16 @@
         <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers" value="50.0">
           <targets>PARENT</targets>
         </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeEdgeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeNodeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:EnumLayoutOption" id="org.eclipse.elk.layered.nodePlacement.strategy">
+          <targets>PARENT</targets>
+          <value name="NETWORK_SIMPLEX"/>
+        </layoutOptions>
       </layout>
       <defaultLayer name="Default">
         <edgeMappings name="References" sourceMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackageWithOpAtRoot']/@defaultLayer/@containerMappings[name='Classes']" targetMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackageWithOpAtRoot']/@defaultLayer/@containerMappings[name='Classes']" targetFinderExpression="feature:eType" sourceFinderExpression="feature:eContainer" domainClass="ecore::EReference" useDomainElement="true">
@@ -1009,6 +1049,16 @@
         <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers" value="50.0">
           <targets>PARENT</targets>
         </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeEdgeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeNodeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:EnumLayoutOption" id="org.eclipse.elk.layered.nodePlacement.strategy">
+          <targets>PARENT</targets>
+          <value name="NETWORK_SIMPLEX"/>
+        </layoutOptions>
       </layout>
       <defaultLayer name="Default">
         <edgeMappings name="References" sourceMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackageReverse']/@defaultLayer/@containerMappings[name='Classes']" targetMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackageReverse']/@defaultLayer/@containerMappings[name='Classes']" targetFinderExpression="feature:eType" sourceFinderExpression="feature:eContainer" domainClass="ecore::EReference" useDomainElement="true">
@@ -1079,6 +1129,16 @@
         <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeEdgeBetweenLayers" value="20.0">
           <targets>PARENT</targets>
         </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeEdgeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:DoubleLayoutOption" id="org.eclipse.elk.layered.spacing.edgeNodeBetweenLayers" value="20.0">
+          <targets>PARENT</targets>
+        </layoutOptions>
+        <layoutOptions xsi:type="description_1:EnumLayoutOption" id="org.eclipse.elk.layered.nodePlacement.strategy">
+          <targets>PARENT</targets>
+          <value name="NETWORK_SIMPLEX"/>
+        </layoutOptions>
       </layout>
       <defaultLayer name="Default">
         <edgeMappings name="References" sourceMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackageWithOpAtRootReverse']/@defaultLayer/@containerMappings[name='Classes']" targetMapping="//@ownedViewpoints[name='ElkTestViewpoint']/@ownedRepresentations[name='DiagramEdgeOnEdge_withPackageWithOpAtRootReverse']/@defaultLayer/@containerMappings[name='Classes']" targetFinderExpression="feature:eType" sourceFinderExpression="feature:eContainer" domainClass="ecore::EReference" useDomainElement="true">
diff --git a/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java b/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java
index a406ecc248..a13d641965 100644
--- a/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java
+++ b/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java
@@ -1598,13 +1598,9 @@ public class SimpleELKLayoutTest extends SiriusDiagramTestCase {
         Connection sourceConnection = sourceEdgeEditPart.getConnectionFigure();
         checkEdgeWithEdgeAsSource("op1", sourceConnection, 3);
         checkEdgeWithEdgeAsSource("op3", sourceConnection, 3);
-        // TODO : I failed to get a layout setting that force edges used as source or target to be straight. This is the
-        // case here.
-        // I asked on gitter to see if someone has an idea: https://gitter.im/eclipse/elk?at=6059b3cd88edaa1eb8d56f8d
-        // In this context, we adapt the test (no check about horizontality and different number of expected points.
-        sourceEdgeEditPart = checkEdge("C3", "C4", false);
+        sourceEdgeEditPart = checkEdge("C3", "C4", true);
         sourceConnection = sourceEdgeEditPart.getConnectionFigure();
-        checkEdgeWithEdgeAsSource("op2", sourceConnection, 2);
+        checkEdgeWithEdgeAsSource("op2", sourceConnection, 3);
     }
 
     /**
@@ -1619,9 +1615,9 @@ public class SimpleELKLayoutTest extends SiriusDiagramTestCase {
      */
     public void testArrangeAll_edgeOnEdge_DifferentLevel1_EdgeAsTarget() {
         testArrangeAllResult_ForPackageResetOrigin("diagramEdgeOnEdge_withPackage for levels1EdgeOnEdge", true, false);
-        DEdgeEditPart targetEdgeEditPart = checkEdge("C1", "C2", false);
+        DEdgeEditPart targetEdgeEditPart = checkEdge("C1", "C2", true);
         Connection targetConnection = targetEdgeEditPart.getConnectionFigure();
-        checkEdgeWithEdgeAsTarget("op1", targetConnection, 2);
+        checkEdgeWithEdgeAsTarget("op1", targetConnection, 3);
         checkEdgeWithEdgeAsTarget("op2", targetConnection, 5);
         checkEdgeWithEdgeAsTarget("op3", targetConnection, 5);
         checkEdgeWithEdgeAsTarget("op4", targetConnection, 5);
@@ -1663,10 +1659,10 @@ public class SimpleELKLayoutTest extends SiriusDiagramTestCase {
         testArrangeAllResult_ForPackageResetOrigin("diagramEdgeOnEdge_withPackageWithOpAtRoot for levels2EdgeOnEdge");
         DEdgeEditPart targetEdgeEditPart = checkEdge("C2", "C1", true);
         Connection targetConnection = targetEdgeEditPart.getConnectionFigure();
-        checkEdgeWithEdgeAsTarget("op1", targetConnection, 3);
+        checkEdgeWithEdgeAsTarget("op1", targetConnection, 5);
         checkEdgeWithEdgeAsTarget("op2", targetConnection, 5);
         checkEdgeWithEdgeAsTarget("op3", targetConnection, 5);
-        checkEdgeWithEdgeAsTarget("op4", targetConnection, 5);
+        checkEdgeWithEdgeAsTarget("op4", targetConnection, 3);
         checkEdgeWithEdgeAsTarget("op5", targetConnection, 5);
     }
 
@@ -1682,11 +1678,11 @@ public class SimpleELKLayoutTest extends SiriusDiagramTestCase {
      */
     public void testArrangeAll_edgeOnEdge_DifferentLevel1_EdgeAsSource() {
         testArrangeAllResult_ForPackageResetOrigin("diagramEdgeOnEdge_withPackageReverse for levels1EdgeOnEdge");
-        DEdgeEditPart targetEdgeEditPart = checkEdge("C1", "C2", false);
+        DEdgeEditPart targetEdgeEditPart = checkEdge("C1", "C2", true);
         Connection targetConnection = targetEdgeEditPart.getConnectionFigure();
         checkEdgeWithEdgeAsSource("op1", targetConnection, 3);
         checkEdgeWithEdgeAsSource("op2", targetConnection, 3);
-        checkEdgeWithEdgeAsSource("op3", targetConnection, 2);
+        checkEdgeWithEdgeAsSource("op3", targetConnection, 3);
         checkEdgeWithEdgeAsSource("op4", targetConnection, 3);
         checkEdgeWithEdgeAsSource("op5", targetConnection, 3);
     }
@@ -1703,13 +1699,13 @@ public class SimpleELKLayoutTest extends SiriusDiagramTestCase {
      */
     public void testArrangeAll_edgeOnEdge_DifferentLevel2_EdgeAsSource() {
         testArrangeAllResult_ForPackageResetOrigin("diagramEdgeOnEdge_withPackageWithOpAtRootReverse for levels1EdgeOnEdge");
-        DEdgeEditPart targetEdgeEditPart = checkEdge("C1", "C2", false);
+        DEdgeEditPart targetEdgeEditPart = checkEdge("C1", "C2", true);
         Connection targetConnection = targetEdgeEditPart.getConnectionFigure();
         checkEdgeWithEdgeAsSource("op1", targetConnection, 3);
-        checkEdgeWithEdgeAsSource("op2", targetConnection, 2);
-        checkEdgeWithEdgeAsSource("op3", targetConnection, 4);
-        checkEdgeWithEdgeAsSource("op4", targetConnection, 4);
-        checkEdgeWithEdgeAsSource("op5", targetConnection, 4);
+        checkEdgeWithEdgeAsSource("op2", targetConnection, 3);
+        checkEdgeWithEdgeAsSource("op3", targetConnection, 3);
+        checkEdgeWithEdgeAsSource("op4", targetConnection, 3);
+        checkEdgeWithEdgeAsSource("op5", targetConnection, 3);
     }
 
     /**
@@ -1726,9 +1722,9 @@ public class SimpleELKLayoutTest extends SiriusDiagramTestCase {
         testArrangeAllResult_ForPackageResetOrigin("diagramEdgeOnEdge_withPackageWithOpAtRootReverse for levels2EdgeOnEdge");
         DEdgeEditPart targetEdgeEditPart = checkEdge("C2", "C1", true);
         Connection targetConnection = targetEdgeEditPart.getConnectionFigure();
-        checkEdgeWithEdgeAsSource("op1", targetConnection, 5);
+        checkEdgeWithEdgeAsSource("op1", targetConnection, 3);
         checkEdgeWithEdgeAsSource("op2", targetConnection, 5);
-        checkEdgeWithEdgeAsSource("op3", targetConnection, 3);
+        checkEdgeWithEdgeAsSource("op3", targetConnection, 5);
         checkEdgeWithEdgeAsSource("op4", targetConnection, 5);
         checkEdgeWithEdgeAsSource("op5", targetConnection, 5);
     }