fix incorrect node size calculation in topdown fallback case (#1152)

* fix incorrect node size calculation in topdown fallback case

* Extend topdown test for node dimension calculation

Check that calculated node dimensions correctly include padding in the
fallback case of topdown layout where no approximator is set.

* organize imports

Author: Eddykasp

plugins/org.eclipse.elk.core/src/org/eclipse/elk/core/RecursiveGraphLayoutEngine.java

@@ -235,6 +235,7 @@ protected List<ElkEdge> layoutRecursively(final ElkNode layoutNode, final TestCo
                             // provider if yes its size needs to be pre-computed before computing the layout
                             LayoutAlgorithmData localAlgorithmData = 
                                     childNode.getProperty(CoreOptions.RESOLVED_ALGORITHM);
+                            ElkPadding padding = childNode.getProperty(CoreOptions.PADDING);
                             if (childNode.getChildren().size() > 0
                                     && localAlgorithmData.getInstancePool().fetch() instanceof ITopdownLayoutProvider) {
                                 // topdownlayout providers should not be used on hierarchical nodes
@@ -254,18 +255,17 @@ protected List<ElkEdge> layoutRecursively(final ElkNode layoutNode, final TestCo
                                 ITopdownSizeApproximator approximator = 
                                         childNode.getProperty(CoreOptions.TOPDOWN_SIZE_APPROXIMATOR);
                                 KVector size = approximator.getSize(childNode);
-                                ElkPadding padding = childNode.getProperty(CoreOptions.PADDING);
                                 childNode.setDimensions(Math.max(childNode.getWidth(), size.x + padding.left + padding.right),
                                         Math.max(childNode.getHeight(), size.y + padding.top + padding.bottom));
                             } else {
                                 // If no approximator is set, use the set sizes for atomic nodes and use the properties
                                 // that have been set for nodes containing further children
                                 if (childNode.getChildren().size() != 0) {
-                                    childNode.setDimensions(
-                                            childNode.getProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_WIDTH),
+                                    KVector size = new KVector(childNode.getProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_WIDTH), 
                                             childNode.getProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_WIDTH) /
-                                            childNode.getProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_ASPECT_RATIO)
-                                    );
+                                            childNode.getProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_ASPECT_RATIO));
+                                    childNode.setDimensions(Math.max(childNode.getWidth(), size.x + padding.left + padding.right),
+                                            Math.max(childNode.getHeight(), size.y + padding.top + padding.bottom));
                                 }
                             }
                         }

test/org.eclipse.elk.alg.topdown.test/src/org/eclipse/elk/alg/topdown/test/TopdownLayoutTest.java

@@ -248,4 +248,49 @@ public void testScaleCapBounded() {
         // topdown scale factor of toplevel node
         assertEquals(3.0, toplevel.getProperty(CoreOptions.TOPDOWN_SCALE_FACTOR), 0.00001);
     }
+    
+    /**
+     * Tests that paddings are correctly considered when computing the size of nodes with further children.
+     */
+    @Test
+    public void testChildDimensionCalculation() {
+        PlainJavaInitialization.initializePlainJavaLayout();
+        ElkNode graph = ElkGraphUtil.createGraph();
+        graph.setProperty(CoreOptions.TOPDOWN_LAYOUT, true);
+        graph.setProperty(CoreOptions.TOPDOWN_NODE_TYPE, TopdownNodeTypes.ROOT_NODE);
+        
+        ElkNode toplevel = ElkGraphUtil.createNode(graph);
+        toplevel.setProperty(CoreOptions.TOPDOWN_LAYOUT, true);
+        toplevel.setProperty(CoreOptions.TOPDOWN_NODE_TYPE, TopdownNodeTypes.HIERARCHICAL_NODE);
+        toplevel.setProperty(CoreOptions.NODE_SIZE_FIXED_GRAPH_SIZE, true);
+        toplevel.setProperty(CoreOptions.ALGORITHM, "org.eclipse.elk.layered");
+        toplevel.setProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_WIDTH, 20.0);
+        toplevel.setProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_ASPECT_RATIO, 1.0);
+        ElkPadding padding = new ElkPadding(10);
+        toplevel.setProperty(CoreOptions.PADDING, padding);
+        
+        ElkNode child1 = ElkGraphUtil.createNode(toplevel);
+        child1.setProperty(CoreOptions.TOPDOWN_LAYOUT, true);
+        child1.setProperty(CoreOptions.TOPDOWN_NODE_TYPE, TopdownNodeTypes.HIERARCHICAL_NODE);
+        child1.setProperty(CoreOptions.NODE_SIZE_FIXED_GRAPH_SIZE, true);
+        child1.setX(0);
+        child1.setY(0);
+        child1.setProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_WIDTH, 20.0);
+        child1.setProperty(CoreOptions.TOPDOWN_HIERARCHICAL_NODE_ASPECT_RATIO, 1.0);
+        
+        
+        // prepare layout engine
+        LayoutConfigurator config = new LayoutConfigurator();
+        ElkUtil.applyVisitors(graph, config, new LayoutAlgorithmResolver());
+        // call layout with layout engine
+        try {
+            new RecursiveGraphLayoutEngine().layout(graph, new BasicProgressMonitor());
+        } catch (UnsupportedGraphException exception) {
+            fail(exception.toString());
+        }
+               
+        // child dimensions computed in fallback case depend on the paddings and the "topdown size"
+        assertEquals(20.0 + padding.getHorizontal(), toplevel.getWidth(), 0.00001);
+        assertEquals(20.0 + padding.getVertical(), toplevel.getHeight(), 0.00001);
+    }
 }