This is an automated email from the ASF dual-hosted git repository.
mattcasters pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 1491b86223 Align auto layout positions with the canvas grid. fixes
#7569 (#8747)
1491b86223 is described below
commit 1491b86223f39e0748ee5debdaab24b3d3cea4ac
Author: Bart Maertens <[email protected]>
AuthorDate: Mon Oct 5 21:13:14 2026 +0200
Align auto layout positions with the canvas grid. fixes #7569 (#8747)
Auto layout now snaps node coordinates, spacing, the selection-anchor
translation and moved notes to the same configurable grid the GUI uses for
manual moves (CanvasGridSize, default 16). Margins and spacing never round
down to zero, so the first position always sits at least one full grid cell
inside the canvas instead of on the grid origin.
---
.../apache/hop/core/layout/LayeredGraphLayout.java | 64 +++++++--
.../hop/pipeline/PipelineMetaLayoutTest.java | 151 ++++++++++++++++++++-
.../hop/workflow/WorkflowMetaLayoutTest.java | 72 ++++++++++
.../main/java/org/apache/hop/ui/core/PropsUi.java | 4 +-
4 files changed, 270 insertions(+), 21 deletions(-)
diff --git
a/engine/src/main/java/org/apache/hop/core/layout/LayeredGraphLayout.java
b/engine/src/main/java/org/apache/hop/core/layout/LayeredGraphLayout.java
index b6b8dec1d8..de716e6970 100644
--- a/engine/src/main/java/org/apache/hop/core/layout/LayeredGraphLayout.java
+++ b/engine/src/main/java/org/apache/hop/core/layout/LayeredGraphLayout.java
@@ -59,8 +59,12 @@ public final class LayeredGraphLayout {
public static final int MARGIN_X = 50;
public static final int MARGIN_Y = 50;
- /** Hop snaps icon positions to a 16px grid. Keep spacing a multiple of
this. */
- private static final int GRID = 16;
+ /**
+ * The grid size the computed positions are snapped to. Hop snaps icon
positions to the canvas
+ * grid during manual moves, so auto-layout must use the same lattice. A
grid size of 1 or less
+ * disables snapping.
+ */
+ public static final int DEFAULT_GRID_SIZE = 16;
/** The direction in which the graph flows from roots towards leaves. */
public enum Direction {
@@ -77,6 +81,7 @@ public final class LayeredGraphLayout {
private int nodeSpacing = DEFAULT_Y_SPACING;
private int crossingIterations = DEFAULT_ITERATIONS;
private boolean moveNotes = true;
+ private int gridSize = DEFAULT_GRID_SIZE;
public Direction getDirection() {
return direction;
@@ -129,6 +134,20 @@ public final class LayeredGraphLayout {
this.moveNotes = moveNotes;
return this;
}
+
+ /**
+ * The grid size the computed positions are snapped to. Use the same value
as the canvas grid
+ * the GUI snaps manual moves to, so auto-layout and manual alignment
agree. A value of 1 or
+ * less disables snapping.
+ */
+ public int getGridSize() {
+ return gridSize;
+ }
+
+ public Options setGridSize(int gridSize) {
+ this.gridSize = gridSize;
+ return this;
+ }
}
/**
@@ -194,13 +213,16 @@ public final class LayeredGraphLayout {
final Point[] computed = new Point[n];
layout(n, edges, options, (node, x, y) -> computed[node] = new Point(x,
y));
- // Translate so the arranged block lands where the nodes used to be.
+ // Translate so the arranged block lands where the nodes used to be. Snap
the translation to
+ // the grid so anchored results stay aligned with it, and keep the block
at least one grid
+ // cell inside the canvas: never on the grid origin (0,0) or off-canvas to
the top or left.
int dx = 0;
int dy = 0;
if (anchorToOriginal) {
Point computedMin = topLeftOfPoints(computed);
- dx = origin.x - computedMin.x;
- dy = origin.y - computedMin.y;
+ int gridSize = options.getGridSize();
+ dx = Math.max(snap(origin.x - computedMin.x, gridSize), gridSize -
computedMin.x);
+ dy = Math.max(snap(origin.y - computedMin.y, gridSize), gridSize -
computedMin.y);
}
// Capture node positions before/after so notes can follow the node
they're closest to.
@@ -232,9 +254,10 @@ public final class LayeredGraphLayout {
}
int nearest = nearestNode(p.x, p.y, beforeX, beforeY, threshold);
if (nearest >= 0) {
+ int gridSize = options.getGridSize();
note.setLocation(
- p.x + (afterX[nearest] - beforeX[nearest]),
- p.y + (afterY[nearest] - beforeY[nearest]));
+ snap(p.x + (afterX[nearest] - beforeX[nearest]), gridSize),
+ snap(p.y + (afterY[nearest] - beforeY[nearest]), gridSize));
}
}
}
@@ -455,8 +478,15 @@ public final class LayeredGraphLayout {
}
// Spacing along the flow direction (between layers) and perpendicular
(between nodes).
- int flowSpace = snap(options.getLayerSpacing());
- int crossSpace = snap(options.getNodeSpacing());
+ // Everything is snapped to the grid but never rounds down to zero: with a
grid larger than
+ // the margins, the first position would otherwise collapse onto the grid
origin (0,0) and
+ // consecutive layers would overlap. The first position always sits at
least one full grid
+ // cell inside the canvas.
+ int grid = Math.max(1, options.getGridSize());
+ int flowSpace = Math.max(snap(options.getLayerSpacing(), grid), grid);
+ int crossSpace = Math.max(snap(options.getNodeSpacing(), grid), grid);
+ int marginX = Math.max(snap(MARGIN_X, grid), grid);
+ int marginY = Math.max(snap(MARGIN_Y, grid), grid);
Direction direction = options.getDirection();
boolean horizontal = direction == Direction.LEFT_RIGHT || direction ==
Direction.RIGHT_LEFT;
boolean reversed = direction == Direction.RIGHT_LEFT || direction ==
Direction.BOTTOM_TOP;
@@ -493,9 +523,9 @@ public final class LayeredGraphLayout {
int along = flowIndex * flowSpace;
int cross = crossIndex * crossSpace;
- int x = horizontal ? MARGIN_X + along : MARGIN_X + cross;
- int y = horizontal ? MARGIN_Y + cross : MARGIN_Y + along;
- sink.setPosition(node, snap(x), snap(y));
+ int x = horizontal ? marginX + along : marginX + cross;
+ int y = horizontal ? marginY + cross : marginY + along;
+ sink.setPosition(node, snap(x, grid), snap(y, grid));
}
}
}
@@ -574,7 +604,13 @@ public final class LayeredGraphLayout {
return (((long) u) << 32) | (v & 0xffffffffL);
}
- private static int snap(int value) {
- return Math.round((float) value / GRID) * GRID;
+ /**
+ * Snaps {@code value} to the nearest multiple of {@code grid}; values of 1
or less pass through.
+ */
+ private static int snap(int value, int grid) {
+ if (grid <= 1) {
+ return value;
+ }
+ return Math.round((float) value / grid) * grid;
}
}
diff --git
a/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaLayoutTest.java
b/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaLayoutTest.java
index 39b6a286a2..fb7c9546ec 100644
--- a/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaLayoutTest.java
+++ b/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaLayoutTest.java
@@ -42,6 +42,11 @@ public class PipelineMetaLayoutTest {
meta.addPipelineHop(new PipelineHopMeta(from, to));
}
+ private void assertOnGrid(Point p, int gridSize) {
+ assertEquals(0, Math.floorMod(p.x, gridSize), "x not on grid: " + p.x);
+ assertEquals(0, Math.floorMod(p.y, gridSize), "y not on grid: " + p.y);
+ }
+
@Test
public void testLayoutNoOverlapAndLeftToRight() {
PipelineMeta meta = new PipelineMeta();
@@ -107,11 +112,12 @@ public class PipelineMetaLayoutTest {
PipelineMetaLayout.layout(meta, new LayeredGraphLayout.Options());
- // The near note moved by the same delta as transform 'a'.
+ // The near note follows transform 'a' and, like a manual move, lands on
the grid.
aDx = a.getLocation().x - aDx;
aDy = a.getLocation().y - aDy;
- assertEquals(110 + aDx, nearNote.getLocation().x);
- assertEquals(110 + aDy, nearNote.getLocation().y);
+ assertOnGrid(nearNote.getLocation(), 16);
+ assertTrue(Math.abs(nearNote.getLocation().x - (110 + aDx)) <= 8);
+ assertTrue(Math.abs(nearNote.getLocation().y - (110 + aDy)) <= 8);
// The far note was left untouched.
assertEquals(9000, farNote.getLocation().x);
@@ -183,16 +189,149 @@ public class PipelineMetaLayoutTest {
assertEquals(77, other.getLocation().x);
assertEquals(88, other.getLocation().y);
- // The arranged block is anchored to the top-left of where the subset was
(minX=1000, minY=50).
+ // The arranged block is anchored near the top-left of where the subset
was (minX=1000,
+ // minY=50), snapped to the grid so later manual moves stay aligned with
it.
int minX = Math.min(a.getLocation().x, b.getLocation().x);
int minY = Math.min(a.getLocation().y, b.getLocation().y);
- assertEquals(1000, minX);
- assertEquals(50, minY);
+ assertOnGrid(a.getLocation(), 16);
+ assertOnGrid(b.getLocation(), 16);
+ assertTrue(Math.abs(minX - 1000) <= 8);
+ assertTrue(Math.abs(minY - 50) <= 8);
// And it still reads left-to-right.
assertTrue(b.getLocation().x > a.getLocation().x, "subset not
left-to-right");
}
+ @Test
+ public void testPositionsAlignToDefaultGrid() {
+ PipelineMeta meta = new PipelineMeta();
+ TransformMeta a = transform("a");
+ TransformMeta b = transform("b");
+ TransformMeta c = transform("c");
+ a.setLocation(101, 203); // deliberately off-grid origins
+ b.setLocation(302, 51);
+ c.setLocation(17, 19);
+ meta.addTransform(a);
+ meta.addTransform(b);
+ meta.addTransform(c);
+ hop(meta, a, b);
+ hop(meta, b, c);
+
+ PipelineMetaLayout.layout(meta);
+
+ for (int i = 0; i < meta.nrTransforms(); i++) {
+ assertOnGrid(meta.getTransform(i).getLocation(), 16);
+ }
+ }
+
+ @Test
+ public void testPositionsAlignToCustomGridSize() {
+ PipelineMeta meta = new PipelineMeta();
+ TransformMeta a = transform("a");
+ TransformMeta b = transform("b");
+ TransformMeta c = transform("c");
+ meta.addTransform(a);
+ meta.addTransform(b);
+ meta.addTransform(c);
+ hop(meta, a, b);
+ hop(meta, b, c);
+
+ PipelineMetaLayout.layout(
+ meta,
+ new
LayeredGraphLayout.Options().setLayerSpacing(151).setNodeSpacing(97).setGridSize(10));
+
+ for (int i = 0; i < meta.nrTransforms(); i++) {
+ assertOnGrid(meta.getTransform(i).getLocation(), 10);
+ }
+ }
+
+ @Test
+ public void testGridSizeOneDisablesSnapping() {
+ PipelineMeta meta = new PipelineMeta();
+ TransformMeta a = transform("a");
+ TransformMeta b = transform("b");
+ TransformMeta c = transform("c");
+ meta.addTransform(a);
+ meta.addTransform(b);
+ meta.addTransform(c);
+ hop(meta, a, b);
+ hop(meta, b, c);
+
+ PipelineMetaLayout.layout(
+ meta, new
LayeredGraphLayout.Options().setLayerSpacing(151).setGridSize(1));
+
+ // Raw margins and spacing: nothing is rounded to a grid.
+ assertEquals(50, a.getLocation().x);
+ assertEquals(50, a.getLocation().y);
+ assertEquals(201, b.getLocation().x);
+ assertEquals(50, b.getLocation().y);
+ assertEquals(352, c.getLocation().x);
+ assertEquals(50, c.getLocation().y);
+ }
+
+ @Test
+ public void testSubsetAnchoredLayoutStaysOnGrid() {
+ PipelineMeta meta = new PipelineMeta();
+ TransformMeta a = transform("a");
+ TransformMeta b = transform("b");
+ TransformMeta other = transform("other");
+ a.setLocation(1001, 2003); // off-grid
+ b.setLocation(3011, 61);
+ other.setLocation(77, 88);
+ meta.addTransform(a);
+ meta.addTransform(b);
+ meta.addTransform(other);
+ hop(meta, a, b);
+
+ PipelineMetaLayout.layout(meta, new LayeredGraphLayout.Options(),
Arrays.asList(a, b));
+
+ assertOnGrid(a.getLocation(), 16);
+ assertOnGrid(b.getLocation(), 16);
+
+ // The unselected transform must not have moved.
+ assertEquals(77, other.getLocation().x);
+ assertEquals(88, other.getLocation().y);
+ }
+
+ @Test
+ public void testLargeGridStartsOneCellInsideCanvas() {
+ PipelineMeta meta = new PipelineMeta();
+ TransformMeta a = transform("a");
+ TransformMeta b = transform("b");
+ meta.addTransform(a);
+ meta.addTransform(b);
+ hop(meta, a, b);
+
+ // A grid larger than the margins used to round the first position down to
(0,0).
+ PipelineMetaLayout.layout(meta, new
LayeredGraphLayout.Options().setGridSize(128));
+
+ assertEquals(128, a.getLocation().x); // first position: grid cell 1:1,
not the origin
+ assertEquals(128, a.getLocation().y);
+ assertOnGrid(b.getLocation(), 128);
+ assertTrue(b.getLocation().x > a.getLocation().x);
+ }
+
+ @Test
+ public void testSubsetNearCornerStaysOneCellInsideCanvas() {
+ PipelineMeta meta = new PipelineMeta();
+ TransformMeta a = transform("a");
+ TransformMeta b = transform("b");
+ a.setLocation(5, 5);
+ b.setLocation(200, 10);
+ meta.addTransform(a);
+ meta.addTransform(b);
+ hop(meta, a, b);
+
+ PipelineMetaLayout.layout(meta, new LayeredGraphLayout.Options(),
Arrays.asList(a, b));
+
+ // The anchored block never lands on the grid origin or off-canvas.
+ assertOnGrid(a.getLocation(), 16);
+ assertOnGrid(b.getLocation(), 16);
+ assertTrue(a.getLocation().x >= 16);
+ assertTrue(a.getLocation().y >= 16);
+ assertTrue(b.getLocation().x > a.getLocation().x);
+ }
+
@Test
public void testEmptyPipelineDoesNotThrow() {
PipelineMetaLayout.layout(new PipelineMeta());
diff --git
a/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaLayoutTest.java
b/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaLayoutTest.java
index a92d7196b7..76ec81134b 100644
--- a/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaLayoutTest.java
+++ b/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaLayoutTest.java
@@ -17,11 +17,14 @@
package org.apache.hop.workflow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
import org.apache.hop.core.gui.Point;
+import org.apache.hop.core.layout.LayeredGraphLayout;
import org.apache.hop.workflow.action.ActionMeta;
import org.apache.hop.workflow.actions.dummy.ActionDummy;
import org.junit.jupiter.api.Test;
@@ -40,6 +43,11 @@ public class WorkflowMetaLayoutTest {
meta.addWorkflowHop(new WorkflowHopMeta(from, to));
}
+ private void assertOnGrid(Point p, int gridSize) {
+ assertEquals(0, Math.floorMod(p.x, gridSize), "x not on grid: " + p.x);
+ assertEquals(0, Math.floorMod(p.y, gridSize), "y not on grid: " + p.y);
+ }
+
@Test
public void testLayoutNoOverlapAndLeftToRight() {
WorkflowMeta meta = new WorkflowMeta();
@@ -83,6 +91,70 @@ public class WorkflowMetaLayoutTest {
}
}
+ @Test
+ public void testPositionsAlignToDefaultGrid() {
+ WorkflowMeta meta = new WorkflowMeta();
+ ActionMeta a = action("a");
+ ActionMeta b = action("b");
+ ActionMeta c = action("c");
+ a.setLocation(101, 203); // deliberately off-grid origins
+ b.setLocation(302, 51);
+ c.setLocation(17, 19);
+ meta.addAction(a);
+ meta.addAction(b);
+ meta.addAction(c);
+ hop(meta, a, b);
+ hop(meta, b, c);
+
+ WorkflowMetaLayout.layout(meta);
+
+ for (int i = 0; i < meta.nrActions(); i++) {
+ assertOnGrid(meta.getAction(i).getLocation(), 16);
+ }
+ }
+
+ @Test
+ public void testSubsetAnchoredLayoutStaysOnGrid() {
+ WorkflowMeta meta = new WorkflowMeta();
+ ActionMeta a = action("a");
+ ActionMeta b = action("b");
+ ActionMeta other = action("other");
+ a.setLocation(1001, 2003); // off-grid
+ b.setLocation(3011, 61);
+ other.setLocation(77, 88);
+ meta.addAction(a);
+ meta.addAction(b);
+ meta.addAction(other);
+ hop(meta, a, b);
+
+ WorkflowMetaLayout.layout(meta, new LayeredGraphLayout.Options(),
Arrays.asList(a, b));
+
+ assertOnGrid(a.getLocation(), 16);
+ assertOnGrid(b.getLocation(), 16);
+
+ // The unselected action must not have moved.
+ assertEquals(77, other.getLocation().x);
+ assertEquals(88, other.getLocation().y);
+ }
+
+ @Test
+ public void testLargeGridStartsOneCellInsideCanvas() {
+ WorkflowMeta meta = new WorkflowMeta();
+ ActionMeta a = action("a");
+ ActionMeta b = action("b");
+ meta.addAction(a);
+ meta.addAction(b);
+ hop(meta, a, b);
+
+ // A grid larger than the margins used to round the first position down to
(0,0).
+ WorkflowMetaLayout.layout(meta, new
LayeredGraphLayout.Options().setGridSize(128));
+
+ assertEquals(128, a.getLocation().x); // first position: grid cell 1:1,
not the origin
+ assertEquals(128, a.getLocation().y);
+ assertOnGrid(b.getLocation(), 128);
+ assertTrue(b.getLocation().x > a.getLocation().x);
+ }
+
@Test
public void testEmptyWorkflowDoesNotThrow() {
WorkflowMetaLayout.layout(new WorkflowMeta());
diff --git a/ui/src/main/java/org/apache/hop/ui/core/PropsUi.java
b/ui/src/main/java/org/apache/hop/ui/core/PropsUi.java
index 4a5c6b9de0..18b19c69b9 100644
--- a/ui/src/main/java/org/apache/hop/ui/core/PropsUi.java
+++ b/ui/src/main/java/org/apache/hop/ui/core/PropsUi.java
@@ -1389,7 +1389,9 @@ public class PropsUi extends Props {
.setLayerSpacing(getAutoLayoutLayerSpacing())
.setNodeSpacing(getAutoLayoutNodeSpacing())
.setCrossingIterations(getAutoLayoutCrossingIterations())
- .setMoveNotes(isAutoLayoutMoveNotes());
+ .setMoveNotes(isAutoLayoutMoveNotes())
+ // Auto-layout must snap to the same grid as manual moves to keep
items aligned.
+ .setGridSize(getCanvasGridSize());
}
/**
