[1831] Improve the internal data structure of the layout algorithm

Bug: eclipse-sirius#1831 Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>
frouene · May 17, 2023 · 50f4926 · 50f4926
1 parent 6d29c4e
commit 50f4926
Show file tree

Hide file tree

Showing 6 changed files with 112 additions and 53 deletions.
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
@@ -75,6 +75,7 @@ Navigate to the parent with the left arrow if a node is collapsed
 - https://github.com/eclipse-sirius/sirius-components/issues/1621[#1621] [project] Migrate the onboard area to Material-UI
 - https://github.com/eclipse-sirius/sirius-components/issues/1852[#1852] [layout] Single position event takes the diagram element id and the double position event takes the source id and target id
 - https://github.com/eclipse-sirius/sirius-components/issues/1971[#1971] [layout] Add the first new layout engine integration test
+- https://github.com/eclipse-sirius/sirius-components/issues/1831[#1831] [layout] Improve the data structure of the layout algorithm
 
 == v2023.4.0
 

diff --git a/...main/java/org/eclipse/sirius/components/diagrams/layout/experimental/DiagramBoxModel.java b/...main/java/org/eclipse/sirius/components/diagrams/layout/experimental/DiagramBoxModel.java
@@ -0,0 +1,27 @@
+/*******************************************************************************
+ * Copyright (c) 2023 Obeo.
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     Obeo - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.sirius.components.diagrams.layout.experimental;
+
+import java.util.Map;
+import java.util.Objects;
+
+/**
+ * Internal data structure of the layout algorithm used to hold the box model.
+ *
+ * @author sbegaudeau
+ */
+public record DiagramBoxModel(Map<String, NodeBoxModel> nodeBoxModels) {
+    public DiagramBoxModel {
+        Objects.requireNonNull(nodeBoxModels);
+    }
+}
diff --git a/...se/sirius/components/diagrams/layout/experimental/DiagramLayoutConfigurationProvider.java b/...se/sirius/components/diagrams/layout/experimental/DiagramLayoutConfigurationProvider.java
@@ -37,7 +37,6 @@
 import org.eclipse.sirius.components.diagrams.layout.api.experimental.NodeLayoutConfiguration;
 import org.eclipse.sirius.components.diagrams.layout.api.experimental.NodeLayoutStrategy;
 import org.eclipse.sirius.components.diagrams.layout.api.experimental.Offsets;
-import org.eclipse.sirius.components.diagrams.layout.incremental.provider.ImageSizeProvider;
 import org.eclipse.sirius.components.diagrams.layoutdata.DiagramLayoutData;
 import org.eclipse.sirius.components.diagrams.layoutdata.Size;
 import org.springframework.stereotype.Service;
@@ -50,12 +49,6 @@
 @Service
 public class DiagramLayoutConfigurationProvider implements IDiagramLayoutConfigurationProvider {
 
-    private final ImageSizeProvider imageSizeProvider;
-
-    public DiagramLayoutConfigurationProvider(ImageSizeProvider imageSizeProvider) {
-        this.imageSizeProvider = Objects.requireNonNull(imageSizeProvider);
-    }
-
     @Override
     public DiagramLayoutConfiguration getDiagramLayoutConfiguration(Diagram diagram, DiagramLayoutData previousLayoutData, Optional<IDiagramEvent> optionalDiagramEvent) {
         var builder = DiagramLayoutConfiguration.newDiagramLayoutConfiguration(diagram.getId())
@@ -166,19 +159,14 @@ private EdgeLayoutConfiguration convertEdge(DiagramLayoutConfiguration diagramLa
     public LabelLayoutConfiguration convertLabel(Label label) {
         var labelStyle = label.getStyle();
         var fontStyle = new FontStyle(labelStyle.isBold(), labelStyle.isItalic(), labelStyle.isUnderline(), labelStyle.isStrikeThrough());
-        var optionalIconSize = this.imageSizeProvider.getSize(labelStyle.getIconURL());
-        var builder = LabelLayoutConfiguration.newLabelLayoutConfiguration(label.getId())
+        return LabelLayoutConfiguration.newLabelLayoutConfiguration(label.getId())
                 .text(label.getText())
                 .fontSize(labelStyle.getFontSize())
                 .fontStyle(fontStyle)
                 .border(Offsets.empty())
                 .padding(Offsets.of(5.0))
-                .margin(Offsets.of(5.0));
-
-        if (optionalIconSize.isPresent()) {
-            Size iconSize = new Size(optionalIconSize.get().getWidth(), optionalIconSize.get().getHeight());
-            builder.iconLayoutConfiguration(new IconLayoutConfiguration(iconSize, 10.0));
-        }
-        return builder.build();
+                .margin(Offsets.of(5.0))
+                .iconLayoutConfiguration(new IconLayoutConfiguration(new Size(16, 16), 10.0))
+                .build();
     }
 }
diff --git a/.../java/org/eclipse/sirius/components/diagrams/layout/experimental/DiagramLayoutEngine.java b/.../java/org/eclipse/sirius/components/diagrams/layout/experimental/DiagramLayoutEngine.java
@@ -50,15 +50,15 @@ public class DiagramLayoutEngine implements IDiagramLayoutEngine {
 
     @Override
     public DiagramLayoutData layout(DiagramLayoutConfiguration diagramLayoutConfiguration) {
-        Map<String, Rectangle> nodeBounds = new HashMap<>();
+        DiagramBoxModel diagramBoxModel = new DiagramBoxModel(new HashMap<>());
 
         try {
-            this.layoutContents(diagramLayoutConfiguration.id(), diagramLayoutConfiguration, nodeBounds);
+            this.layoutContents(diagramLayoutConfiguration.id(), diagramLayoutConfiguration, diagramBoxModel);
 
             var childNodeIds = diagramLayoutConfiguration.childNodeLayoutConfigurations().stream()
                     .map(NodeLayoutConfiguration::id)
                     .toList();
-            this.arrangeChildren(diagramLayoutConfiguration.id(), childNodeIds, diagramLayoutConfiguration, nodeBounds);
+            this.arrangeChildren(diagramLayoutConfiguration.id(), childNodeIds, diagramLayoutConfiguration, diagramBoxModel);
         } catch (IllegalArgumentException exception) {
             // Several kind of assertions are performed during the layout, in case of bugs in our code,
             // those assertions could throw some exceptions. We will catch them here in order to return
@@ -67,12 +67,12 @@ public DiagramLayoutData layout(DiagramLayoutConfiguration diagramLayoutConfigur
         }
 
         Map<String, NodeLayoutData> newNodeLayoutData = new HashMap<>();
-        nodeBounds.forEach((nodeId, newBounds) -> newNodeLayoutData.put(nodeId, new NodeLayoutData(nodeId, newBounds.topLeft(), newBounds.size())));
+        diagramBoxModel.nodeBoxModels().forEach((nodeId, nodeBoxModel) -> newNodeLayoutData.put(nodeId, new NodeLayoutData(nodeId, nodeBoxModel.bounds().topLeft(), nodeBoxModel.bounds().size())));
 
         return new DiagramLayoutData(newNodeLayoutData, Map.of(), Map.of());
     }
 
-    private void layoutContents(String containerId, DiagramLayoutConfiguration diagramLayoutConfiguration, Map<String, Rectangle> nodeBounds) {
+    private void layoutContents(String containerId, DiagramLayoutConfiguration diagramLayoutConfiguration, DiagramBoxModel diagramBoxModel) {
         // Recursively layout the children's contents
         IParentLayoutConfiguration containerLayoutConfiguration = Optional.<IParentLayoutConfiguration>ofNullable(diagramLayoutConfiguration.nodeLayoutConfigurationsById().get(containerId))
                 .orElse(diagramLayoutConfiguration);
@@ -81,18 +81,21 @@ private void layoutContents(String containerId, DiagramLayoutConfiguration diagr
                 .map(NodeLayoutConfiguration::id)
                 .toList();
 
-        childNodeIds.forEach(childNodeId -> this.layoutContents(childNodeId, diagramLayoutConfiguration, nodeBounds));
+        childNodeIds.forEach(childNodeId -> this.layoutContents(childNodeId, diagramLayoutConfiguration, diagramBoxModel));
 
         // At this point the children have their correct *size* but are not correctly positioned.
         // Arrange them properly as if on an unbounded canvas.
-        this.arrangeChildren(containerId, childNodeIds, diagramLayoutConfiguration, nodeBounds);
+        this.arrangeChildren(containerId, childNodeIds, diagramLayoutConfiguration, diagramBoxModel);
 
         // Shift the children's position if needed so that they stay inside the container's content area.
         var internalOffsets = diagramLayoutConfiguration.optionalNodeLayoutConfiguration(containerId)
                 .map(configuration -> configuration.border().combine(configuration.padding()))
                 .orElse(Offsets.empty());
 
-        Rectangle childrenFootprint = Rectangle.union(childNodeIds.stream().map(nodeBounds::get).toList());
+        Rectangle childrenFootprint = Rectangle.union(childNodeIds.stream()
+                .map(diagramBoxModel.nodeBoxModels()::get)
+                .map(NodeBoxModel::footprint)
+                .toList());
         if (!childNodeIds.isEmpty() && !containerId.equals(diagramLayoutConfiguration.id())) {
             double dx = 0.0;
             double dy = 0.0;
@@ -104,8 +107,17 @@ private void layoutContents(String containerId, DiagramLayoutConfiguration diagr
                 dy = internalOffsets.top() - childrenOrigin.y();
             }
             for (String childNodeId : childNodeIds) {
-                Rectangle childBounds = nodeBounds.get(childNodeId).translate(dx, dy);
-                nodeBounds.put(childNodeId, childBounds);
+                var optionalNodeBoxModel = Optional.ofNullable(diagramBoxModel.nodeBoxModels().get(childNodeId));
+                var optionalNodeLayoutConfiguration = diagramLayoutConfiguration.optionalNodeLayoutConfiguration(childNodeId);
+                if (optionalNodeBoxModel.isPresent() && optionalNodeLayoutConfiguration.isPresent()) {
+                    var nodeBoxModel = optionalNodeBoxModel.get();
+                    var nodeLayoutConfiguration = optionalNodeLayoutConfiguration.get();
+
+                    var newBounds = nodeBoxModel.bounds().translate(dx, dy);
+                    var newFootprint = newBounds.expand(nodeLayoutConfiguration.margin());
+                    var newNodeBoxModel = new NodeBoxModel(nodeBoxModel.nodeId(), newBounds, newFootprint);
+                    diagramBoxModel.nodeBoxModels().put(childNodeId, newNodeBoxModel);
+                }
             }
         }
 
@@ -128,22 +140,31 @@ private void layoutContents(String containerId, DiagramLayoutConfiguration diagr
         Size requestedSize = diagramLayoutConfiguration.optionalResizeEvent(containerId).map(ResizeEvent::newSize).map(s -> new Size(s.getWidth(), s.getHeight())).orElse(previousSize);
         double width = Math.max(contentsSize.width(), Math.max(minSize.width(), requestedSize.width()));
         double height = Math.max(contentsSize.height(), Math.max(minSize.height(), requestedSize.height()));
-        nodeBounds.put(containerId, new Rectangle(0, 0, width, height));
+
+        var optionalNodeLayoutConfiguration = diagramLayoutConfiguration.optionalNodeLayoutConfiguration(containerId);
+        if (optionalNodeLayoutConfiguration.isPresent()) {
+            var nodeLayoutConfiguration = optionalNodeLayoutConfiguration.get();
+
+            var bounds = new Rectangle(0, 0, width, height);
+            var footprint = bounds.expand(nodeLayoutConfiguration.margin());
+            var newNodeBoxModel = new NodeBoxModel(containerId, bounds, footprint);
+            diagramBoxModel.nodeBoxModels().put(containerId, newNodeBoxModel);
+        }
     }
 
     /**
      * Assuming all the children have their proper size, arrange them (only changing their positions) to their final
      * position (relative to their parent).
      */
-    private void arrangeChildren(String parentElementId, List<String> childNodeIds, DiagramLayoutConfiguration diagramLayoutConfiguration, Map<String, Rectangle> layout) {
+    private void arrangeChildren(String parentElementId, List<String> childNodeIds, DiagramLayoutConfiguration diagramLayoutConfiguration, DiagramBoxModel diagramBoxModel) {
         Canvas canvas = new Canvas();
 
         // First, place the node(s) which has been directly interacted with by the end-user if there are any.
         for (String childNodeId : childNodeIds) {
             Optional<MoveEvent> optionalMoveEvent = diagramLayoutConfiguration.optionalMoveEvent(childNodeId);
             if (optionalMoveEvent.isPresent()) {
                 Position newPosition = new Position(optionalMoveEvent.get().newPosition().getX(), optionalMoveEvent.get().newPosition().getY());
-                canvas.setBounds(childNodeId, layout.get(childNodeId).moveTo(newPosition));
+                canvas.setBounds(childNodeId, diagramBoxModel.nodeBoxModels().get(childNodeId).bounds().moveTo(newPosition));
             }
             // A resize can also change the position if it moves the top-left corner.
             Optional<ResizeEvent> optionalResizeEvent = diagramLayoutConfiguration.optionalResizeEvent(childNodeId);
@@ -153,7 +174,7 @@ private void arrangeChildren(String parentElementId, List<String> childNodeIds,
                     double dx = -resizeEvent.positionDelta().getX();
                     double dy = -resizeEvent.positionDelta().getY();
                     Position newPosition = bounds.topLeft().translate(dx, dy);
-                    canvas.setBounds(childNodeId, layout.get(childNodeId).moveTo(newPosition));
+                    canvas.setBounds(childNodeId, diagramBoxModel.nodeBoxModels().get(childNodeId).bounds().moveTo(newPosition));
                 });
             }
         }
@@ -178,14 +199,14 @@ private void arrangeChildren(String parentElementId, List<String> childNodeIds,
                 String newChildId = childrenWithoutPreviousLocation.iterator().next();
                 childrenWithoutPreviousLocation.remove(newChildId);
                 var position = singlePositionEvent.position();
-                canvas.place(newChildId, this.relativePosition(diagramLayoutConfiguration, parentElementId, new Position(position.getX(), position.getY())), layout.get(newChildId).size());
+                canvas.place(newChildId, this.relativePosition(diagramLayoutConfiguration, parentElementId, new Position(position.getX(), position.getY())), diagramBoxModel.nodeBoxModels().get(newChildId).footprint().size());
             });
         }
 
         // Next, try to keep the position for all the other nodes which have previous positions.
         for (String childId : childrenWithPreviousLocation) {
             diagramLayoutConfiguration.optionalPreviousFootprint(childId).ifPresent(bounds -> {
-                canvas.place(childId, bounds.topLeft(), layout.get(childId).size());
+                canvas.place(childId, bounds.topLeft(), diagramBoxModel.nodeBoxModels().get(childId).bounds().size());
             });
         }
 
@@ -195,14 +216,18 @@ private void arrangeChildren(String parentElementId, List<String> childNodeIds,
                 .map(NodeLayoutConfiguration::margin)
                 .orElse(Offsets.empty());
 
-        Function<String, Size> sizeProvider = nodeId -> layout.get(nodeId).size();
+        Function<String, Size> sizeProvider = nodeId -> diagramBoxModel.nodeBoxModels().get(nodeId).bounds().size();
 
         var newNodesLayout = new CanvasLayoutEngine(childrenWithoutPreviousLocation, sizeProvider, marginProvider)
                 .getLeftToRightLayout(canvas.getOccupiedFootprints(marginProvider));
         newNodesLayout.forEach(canvas::setBounds);
 
-        // "Commit" the result into the global layout
-        layout.putAll(canvas.getAllBounds());
+        // "Commit" the result into the global box model
+        canvas.getAllBounds().entrySet().stream()
+                        .map(entry -> {
+                            var nodeBoxModel = diagramBoxModel.nodeBoxModels().get(entry.getKey());
+                            return new NodeBoxModel(nodeBoxModel.nodeId(), nodeBoxModel.bounds(), entry.getValue());
+                        }).forEach(nodeBoxModel -> diagramBoxModel.nodeBoxModels().put(nodeBoxModel.nodeId(), nodeBoxModel));
     }
 
     public Position relativePosition(DiagramLayoutConfiguration diagramLayoutConfiguration, String nodeId, Position absolutePosition) {

diff --git a/...rc/main/java/org/eclipse/sirius/components/diagrams/layout/experimental/NodeBoxModel.java b/...rc/main/java/org/eclipse/sirius/components/diagrams/layout/experimental/NodeBoxModel.java
@@ -0,0 +1,30 @@
+/*******************************************************************************
+ * Copyright (c) 2023 Obeo.
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     Obeo - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.sirius.components.diagrams.layout.experimental;
+
+import java.util.Objects;
+
+import org.eclipse.sirius.components.diagrams.layout.api.experimental.Rectangle;
+
+/**
+ * Internal data structure of the node.
+ *
+ * @author sbegaudeau
+ */
+public record NodeBoxModel(String nodeId, Rectangle bounds, Rectangle footprint) {
+    public NodeBoxModel {
+        Objects.requireNonNull(nodeId);
+        Objects.requireNonNull(bounds);
+        Objects.requireNonNull(footprint);
+    }
+}
diff --git a/...rg/eclipse/sirius/web/sample/tests/integration/diagram/DiagramLayoutIntegrationTests.java b/...rg/eclipse/sirius/web/sample/tests/integration/diagram/DiagramLayoutIntegrationTests.java
@@ -24,34 +24,24 @@
 import org.eclipse.sirius.components.collaborative.diagrams.export.svg.NodeExportService;
 import org.eclipse.sirius.components.diagrams.Diagram;
 import org.eclipse.sirius.components.diagrams.layout.api.experimental.DiagramLayoutConfiguration;
-import org.eclipse.sirius.components.diagrams.layout.api.experimental.IDiagramLayoutConfigurationProvider;
-import org.eclipse.sirius.components.diagrams.layout.api.experimental.IDiagramLayoutEngine;
+import org.eclipse.sirius.components.diagrams.layout.experimental.DiagramLayoutConfigurationProvider;
+import org.eclipse.sirius.components.diagrams.layout.experimental.DiagramLayoutEngine;
 import org.eclipse.sirius.components.diagrams.layoutdata.DiagramLayoutData;
 import org.eclipse.sirius.components.diagrams.tests.TestDiagramBuilder;
-import org.eclipse.sirius.web.sample.tests.integration.AbstractIntegrationTests;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.boot.test.context.SpringBootTest;
 
 /**
  * Integration tests for diagram layout engine.
  *
  * @author gcoutable
  */
-@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
-public class DiagramLayoutIntegrationTests extends AbstractIntegrationTests {
+public class DiagramLayoutIntegrationTests {
 
     private final Logger logger = LoggerFactory.getLogger(DiagramLayoutIntegrationTests.class);
 
-    @Autowired
-    private IDiagramLayoutConfigurationProvider diagramLayoutConfigurationProvider;
-
-    @Autowired
-    private IDiagramLayoutEngine diagramLayoutEngine;
-
     @Test
     @DisplayName("Given a diagram, when the layout is performed, then valid layout data are computed")
     public void givenDiagramWhenLayoutIsPerformedThenValidLayoutDataAreComputed() {
@@ -60,9 +50,9 @@ public void givenDiagramWhenLayoutIsPerformedThenValidLayoutDataAreComputed() {
                 .nodes(List.of(builder.getNode("node")))
                 .build();
 
-        DiagramLayoutConfiguration diagramLayoutConfiguration = this.diagramLayoutConfigurationProvider.getDiagramLayoutConfiguration(diagram, new DiagramLayoutData(Map.of(), Map.of(), Map.of()), Optional.empty());
+        DiagramLayoutConfiguration diagramLayoutConfiguration = new DiagramLayoutConfigurationProvider().getDiagramLayoutConfiguration(diagram, new DiagramLayoutData(Map.of(), Map.of(), Map.of()), Optional.empty());
 
-        DiagramLayoutData diagramLayoutData = this.diagramLayoutEngine.layout(diagramLayoutConfiguration);
+        DiagramLayoutData diagramLayoutData = new DiagramLayoutEngine().layout(diagramLayoutConfiguration);
 
         Diagram layoutedDiagram = Diagram.newDiagram(diagram)
                 .layoutData(diagramLayoutData)
@@ -78,9 +68,7 @@ private DiagramExportService getDiagramExportService() {
         DiagramElementExportService diagramElementExportService = new DiagramElementExportService(imageRegistry);
         EdgeExportService edgeExportService = new EdgeExportService(diagramElementExportService);
         NodeExportService nodeExportService = new NodeExportService(diagramElementExportService);
-        DiagramExportService diagramExportService = new DiagramExportService(nodeExportService, edgeExportService, imageRegistry);
-
-        return diagramExportService;
+        return new DiagramExportService(nodeExportService, edgeExportService, imageRegistry);
     }
 
 }