From 349a673c0e9382878c54ae070d8cfb370f6e3625 Mon Sep 17 00:00:00 2001 From: Martin Fleck Date: Tue, 25 Aug 2026 15:59:18 +0200 Subject: [PATCH 1/2] GLSP-1727: Keep applying computed bounds when an entry cannot be applied A single unresolvable element in a ComputedBoundsAction aborted the whole batch: the client had already measured every element, but one stale id threw and the server dropped the remaining bounds, alignments and routes along with the model update the client was waiting for. Mirrors the same fix in glsp-server-node. - resolve elements through the index Optional instead of getOrThrow, so an unknown id yields an empty result - report a route with fewer than two points as not applicable rather than as an error, a client may have nothing to report for an unmeasured edge yet - log a skipped entry, routes at debug level because an edge the client has not finished routing yet is expected - split the apply step into applyElementBounds, applyAlignments and applyRoutes - leave applyRoutingPoints strict, an unknown id in a ChangeRoutingPointsOperation is a real error Adjusting the apply step previously meant overriding executeAction, which also takes over the revision check and the model lock. ComputedBoundsActionHandler now dispatches through overridable per-kind methods, so an adopter can replace one kind and rebind the handler, as the workflow example does for other handlers. LayoutUtil.applyRoute now returns Optional; noted in the changelog under potentially breaking changes. Relates to eclipse-glsp/glsp#1727 --- CHANGELOG.md | 5 + .../model/ComputedBoundsActionHandler.java | 56 ++++++- .../eclipse/glsp/server/utils/LayoutUtil.java | 101 ++++++++--- .../ComputedBoundsActionHandlerTest.java | 142 ++++++++++++++++ .../glsp/server/utils/LayoutUtilTest.java | 158 ++++++++++++++++++ 5 files changed, 437 insertions(+), 25 deletions(-) create mode 100644 tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java create mode 100644 tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index cda394d5..74bada85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ ### Potentially Breaking Changes +- [layout] Keep applying computed bounds when an individual entry cannot be applied [#295](https://github.com/eclipse-glsp/glsp-server/pull/295) + - `LayoutUtil.applyRoute` now returns `Optional` instead of `GEdge` + - `LayoutUtil.applyBounds`, `applyAlignment` and `applyRoute` no longer throw for an element the index cannot resolve, they report it as not applied. `applyRoutingPoints` stays strict. + - `ComputedBoundsActionHandler` applies the computed bounds through the new overridable `applyBounds`, `applyElementBounds`, `applyAlignments` and `applyRoutes` methods, so adjusting one kind no longer means taking over `executeAction` and its model lock + ## [v2.7.0 - 01/06/2026](https://github.com/eclipse-glsp/glsp-server/releases/tag/v2.7.0) ### Changes diff --git a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java index d4be507d..b5a157cc 100644 --- a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java +++ b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (c) 2019-2023 EclipseSource and others. + * Copyright (c) 2019-2026 EclipseSource and others. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -17,10 +17,14 @@ import java.util.List; +import org.eclipse.glsp.graph.GModelIndex; import org.eclipse.glsp.graph.GModelRoot; import org.eclipse.glsp.server.actions.AbstractActionHandler; import org.eclipse.glsp.server.actions.Action; import org.eclipse.glsp.server.model.GModelState; +import org.eclipse.glsp.server.types.ElementAndAlignment; +import org.eclipse.glsp.server.types.ElementAndBounds; +import org.eclipse.glsp.server.types.ElementAndRoutingPoints; import org.eclipse.glsp.server.utils.LayoutUtil; import com.google.inject.Inject; @@ -44,11 +48,59 @@ public List executeAction(final ComputedBoundsAction action) { GModelRoot model = modelState.getRoot(); if (model != null && action.getRevision().isPresent() && action.getRevision().get().doubleValue() == model.getRevision()) { - LayoutUtil.applyBounds(model, action, modelState); + applyBounds(model, action); return submissionHandler.submitModelDirectly(); } } return none(); } + /** + * Applies everything the client computed for the given model. + * + *

+ * Override this, or one of the per-kind methods it delegates to, to adjust what is applied. Overriding + * {@link #executeAction(ComputedBoundsAction)} instead would also take over the revision check and the model lock. + *

+ * + * @param root The model root. + * @param action The computed bounds action. + */ + protected void applyBounds(final GModelRoot root, final ComputedBoundsAction action) { + GModelIndex index = modelState.getIndex(); + applyElementBounds(action.getBounds(), index); + applyAlignments(action.getAlignments(), index); + applyRoutes(action.getRoutes(), index); + } + + /** + * Applies the computed bounds of several elements. + * + * @param allBounds The new bounds. + * @param index The model index. + */ + protected void applyElementBounds(final List allBounds, final GModelIndex index) { + LayoutUtil.applyElementBounds(allBounds, index); + } + + /** + * Applies the computed alignments of several elements. + * + * @param alignments The new alignments. + * @param index The model index. + */ + protected void applyAlignments(final List alignments, final GModelIndex index) { + LayoutUtil.applyAlignments(alignments, index); + } + + /** + * Applies the computed routes. + * + * @param routes The new routes. + * @param index The model index. + */ + protected void applyRoutes(final List routes, final GModelIndex index) { + LayoutUtil.applyRoutes(routes, index); + } + } diff --git a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java index 49271eae..b6d2f756 100644 --- a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java +++ b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2019-2022 EclipseSource and others. + * Copyright (c) 2019-2026 EclipseSource and others. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -21,6 +21,8 @@ import java.util.List; import java.util.Optional; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.eclipse.emf.common.util.EList; import org.eclipse.emf.ecore.util.EcoreUtil; import org.eclipse.glsp.graph.GAlignable; @@ -45,11 +47,17 @@ public final class LayoutUtil { + protected static Logger LOGGER = LogManager.getLogger(LayoutUtil.class); + private LayoutUtil() {} /** * Apply the computed bounds from the given {@link ComputedBoundsAction} to the model. * + *

+ * An entry that cannot be applied is skipped and logged rather than treated as an error. + *

+ * * @param root The model root. * @param action The computed bounds action. * @param modelState The model state @@ -57,9 +65,55 @@ private LayoutUtil() {} public static void applyBounds(final GModelRoot root, final ComputedBoundsAction action, final GModelState modelState) { GModelIndex index = modelState.getIndex(); - action.getBounds().forEach(bounds -> applyBounds(bounds, index)); - action.getAlignments().forEach(alignment -> applyAlignment(alignment, index)); - action.getRoutes().forEach(route -> applyRoute(route, index)); + applyElementBounds(action.getBounds(), index); + applyAlignments(action.getAlignments(), index); + applyRoutes(action.getRoutes(), index); + } + + /** + * Applies the computed bounds of several elements. + * + * @param allBounds The new bounds. + * @param index The model index. + */ + public static void applyElementBounds(final List allBounds, final GModelIndex index) { + allBounds.forEach(bounds -> { + if (applyBounds(bounds, index).isEmpty()) { + LOGGER.warn("Skipped computed bounds of element '" + bounds.getElementId() + "'"); + } + }); + } + + /** + * Applies the computed alignments of several elements. + * + * @param alignments The new alignments. + * @param index The model index. + */ + public static void applyAlignments(final List alignments, final GModelIndex index) { + alignments.forEach(alignment -> { + if (applyAlignment(alignment, index).isEmpty()) { + LOGGER.warn("Skipped computed alignment of element '" + alignment.getElementId() + "'"); + } + }); + } + + /** + * Applies the computed routes. + * + *

+ * A skipped route is logged at debug level, an edge the client has not finished routing yet is expected. + *

+ * + * @param routes The new routes. + * @param index The model index. + */ + public static void applyRoutes(final List routes, final GModelIndex index) { + routes.forEach(route -> { + if (applyRoute(route, index).isEmpty()) { + LOGGER.debug("Skipped computed route of element '" + route.getElementId() + "'"); + } + }); } /** @@ -67,13 +121,12 @@ public static void applyBounds(final GModelRoot root, final ComputedBoundsAction * * @param bounds The new bounds. * @param index The model index. - * @return The changed element. + * @return The changed element, or empty if the bounds could not be applied to any element. */ public static Optional applyBounds(final ElementAndBounds bounds, final GModelIndex index) { - GModelElement element = getOrThrow(index.get(bounds.getElementId()), - "Model element not found! ID: " + bounds.getElementId()); - if (element instanceof GBoundsAware) { - GBoundsAware bae = (GBoundsAware) element; + Optional element = index.get(bounds.getElementId()); + if (element.isPresent() && element.get() instanceof GBoundsAware) { + GBoundsAware bae = (GBoundsAware) element.get(); if (bounds.getNewPosition() != null) { bae.setPosition(GraphUtil.copy(bounds.getNewPosition())); } @@ -90,13 +143,12 @@ public static Optional applyBounds(final ElementAndBounds bounds, * * @param alignment The new alignment. * @param index The model index. - * @return The changed element. + * @return The changed element, or empty if the alignment could not be applied to any element. */ public static Optional applyAlignment(final ElementAndAlignment alignment, final GModelIndex index) { - GModelElement element = getOrThrow(index.get(alignment.getElementId()), - "Model element not found! ID: " + alignment.getElementId()); - if (element instanceof GAlignable) { - GAlignable alignable = (GAlignable) element; + Optional element = index.get(alignment.getElementId()); + if (element.isPresent() && element.get() instanceof GAlignable) { + GAlignable alignable = (GAlignable) element.get(); alignable.setAlignment(alignment.getNewAlignment()); return Optional.of(alignable); } @@ -104,22 +156,25 @@ public static Optional applyAlignment(final ElementAndAlignment alig } /** - * Applies the new route to the model. + * Applies the new route to the model. A route needs at least a source and a target point to describe an edge. * * @param route The new route. * @param index The model index. - * @return The changed element. + * @return The changed edge, or empty if the route could not be applied to any edge. */ - public static GEdge applyRoute(final ElementAndRoutingPoints route, final GModelIndex index) { + public static Optional applyRoute(final ElementAndRoutingPoints route, final GModelIndex index) { List routingPoints = route.getNewRoutingPoints(); - if (routingPoints.size() < 2) { - throw new GLSPServerException("Invalid Route!"); + Optional edge = index.findElementByClass(route.getElementId(), GEdge.class); + if (edge.isEmpty() || routingPoints == null || routingPoints.size() < 2) { + return Optional.empty(); } + EList edgeRoutingPoints = edge.get().getRoutingPoints(); + edgeRoutingPoints.clear(); + edgeRoutingPoints.addAll(routingPoints); // first and last point mark the source and target point - GEdge edge = applyRoutingPoints(route, index); - EList edgeRoutingPoints = edge.getRoutingPoints(); - edge.getArgs().put(GArguments.KEY_EDGE_SOURCE_POINT, edgeRoutingPoints.remove(0)); - edge.getArgs().put(GArguments.KEY_EDGE_TARGET_POINT, edgeRoutingPoints.remove(edgeRoutingPoints.size() - 1)); + edge.get().getArgs().put(GArguments.KEY_EDGE_SOURCE_POINT, edgeRoutingPoints.remove(0)); + edge.get().getArgs().put(GArguments.KEY_EDGE_TARGET_POINT, + edgeRoutingPoints.remove(edgeRoutingPoints.size() - 1)); return edge; } diff --git a/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java new file mode 100644 index 00000000..77fc1240 --- /dev/null +++ b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java @@ -0,0 +1,142 @@ +/******************************************************************************** + * Copyright (c) 2026 EclipseSource and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ +package org.eclipse.glsp.server.features.core.model; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.List; + +import org.eclipse.glsp.graph.GEdge; +import org.eclipse.glsp.graph.GGraph; +import org.eclipse.glsp.graph.GModelIndex; +import org.eclipse.glsp.graph.GNode; +import org.eclipse.glsp.graph.GPoint; +import org.eclipse.glsp.graph.GraphFactory; +import org.eclipse.glsp.graph.util.GraphUtil; +import org.eclipse.glsp.server.model.DefaultGModelState; +import org.eclipse.glsp.server.model.GModelState; +import org.eclipse.glsp.server.types.ElementAndAlignment; +import org.eclipse.glsp.server.types.ElementAndBounds; +import org.eclipse.glsp.server.types.ElementAndRoutingPoints; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class ComputedBoundsActionHandlerTest { + + private static final String NODE_ID = "node0"; + private static final String EDGE_ID = "edge0"; + + /** Exposes a fixed index, the injected one is not available outside of Guice. */ + private static class TestGModelState extends DefaultGModelState { + private final GModelIndex testIndex; + + TestGModelState(final GModelIndex testIndex) { + this.testIndex = testIndex; + } + + @Override + public GModelIndex getIndex() { return testIndex; } + } + + /** Records which per-kind methods the apply step reached, and drops the routes. */ + private static class TestHandler extends ComputedBoundsActionHandler { + private final List applied = new ArrayList<>(); + + TestHandler(final GModelState state) { + this.modelState = state; + } + + void apply(final GGraph root, final ComputedBoundsAction action) { + applyBounds(root, action); + } + + @Override + protected void applyElementBounds(final List allBounds, final GModelIndex index) { + applied.add("bounds"); + super.applyElementBounds(allBounds, index); + } + + @Override + protected void applyAlignments(final List alignments, final GModelIndex index) { + applied.add("alignments"); + super.applyAlignments(alignments, index); + } + + @Override + protected void applyRoutes(final List routes, final GModelIndex index) { + applied.add("routes"); + // deliberately not delegating, an adopter may drop what the client reported + } + } + + private GGraph graph; + private GNode node; + private GEdge edge; + private TestHandler handler; + + @BeforeEach + void setUpGraph() { + graph = GraphFactory.eINSTANCE.createGGraph(); + graph.setId("graphId"); + graph.setRevision(1); + + node = GraphFactory.eINSTANCE.createGNode(); + node.setId(NODE_ID); + node.setSize(GraphUtil.dimension(1, 1)); + + GNode target = GraphFactory.eINSTANCE.createGNode(); + target.setId("node1"); + + edge = GraphFactory.eINSTANCE.createGEdge(); + edge.setId(EDGE_ID); + edge.setSourceId(NODE_ID); + edge.setTargetId(target.getId()); + + graph.getChildren().add(node); + graph.getChildren().add(target); + graph.getChildren().add(edge); + + handler = new TestHandler(new TestGModelState(GModelIndex.create(graph))); + } + + private ComputedBoundsAction computedBounds() { + ElementAndBounds bounds = new ElementAndBounds(entry -> { + entry.setElementId(NODE_ID); + entry.setNewSize(GraphUtil.dimension(10, 20)); + }); + // three points, so a routing point survives the source and target being split off + List route = List.of(GraphUtil.point(0, 0), GraphUtil.point(5, 5), GraphUtil.point(10, 10)); + return new ComputedBoundsAction(List.of(bounds), List.of(), + List.of(new ElementAndRoutingPoints(EDGE_ID, route)), graph.getRevision()); + } + + @Test + void appliesEveryKindOfComputedBounds() { + handler.apply(graph, computedBounds()); + + assertEquals(List.of("bounds", "alignments", "routes"), handler.applied); + } + + @Test + void letsAnOverrideReplaceASingleKind() { + handler.apply(graph, computedBounds()); + + assertEquals(10, node.getSize().getWidth()); + assertTrue(edge.getRoutingPoints().isEmpty()); + } +} diff --git a/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java new file mode 100644 index 00000000..0af0affb --- /dev/null +++ b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java @@ -0,0 +1,158 @@ +/******************************************************************************** + * Copyright (c) 2026 EclipseSource and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ +package org.eclipse.glsp.server.utils; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; + +import org.eclipse.glsp.graph.GEdge; +import org.eclipse.glsp.graph.GGraph; +import org.eclipse.glsp.graph.GModelIndex; +import org.eclipse.glsp.graph.GNode; +import org.eclipse.glsp.graph.GPoint; +import org.eclipse.glsp.graph.GraphFactory; +import org.eclipse.glsp.graph.util.GraphUtil; +import org.eclipse.glsp.server.features.core.model.ComputedBoundsAction; +import org.eclipse.glsp.server.model.DefaultGModelState; +import org.eclipse.glsp.server.types.ElementAndAlignment; +import org.eclipse.glsp.server.types.ElementAndBounds; +import org.eclipse.glsp.server.types.ElementAndRoutingPoints; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class LayoutUtilTest { + + private static final String NODE_ID = "node0"; + private static final String EDGE_ID = "edge0"; + private static final String UNKNOWN_ID = "does-not-exist"; + + /** Exposes a fixed index, the injected one is not available outside of Guice. */ + private static class TestGModelState extends DefaultGModelState { + private final GModelIndex testIndex; + + TestGModelState(final GModelIndex testIndex) { + this.testIndex = testIndex; + } + + @Override + public GModelIndex getIndex() { return testIndex; } + } + + private GGraph graph; + private GNode node; + private GModelIndex index; + + @BeforeEach + void setUpGraph() { + graph = GraphFactory.eINSTANCE.createGGraph(); + graph.setId("graphId"); + graph.setRevision(1); + + node = GraphFactory.eINSTANCE.createGNode(); + node.setId(NODE_ID); + node.setSize(GraphUtil.dimension(1, 1)); + + GNode target = GraphFactory.eINSTANCE.createGNode(); + target.setId("node1"); + + GEdge edge = GraphFactory.eINSTANCE.createGEdge(); + edge.setId(EDGE_ID); + edge.setSourceId(NODE_ID); + edge.setTargetId(target.getId()); + + graph.getChildren().add(node); + graph.getChildren().add(target); + graph.getChildren().add(edge); + + index = GModelIndex.create(graph); + } + + private static ElementAndBounds bounds(final String elementId) { + return new ElementAndBounds(bounds -> { + bounds.setElementId(elementId); + bounds.setNewSize(GraphUtil.dimension(10, 20)); + }); + } + + private static ElementAndAlignment alignment(final String elementId) { + return new ElementAndAlignment(alignment -> { + alignment.setElementId(elementId); + alignment.setNewAlignment(GraphUtil.point(1, 1)); + }); + } + + private static ElementAndRoutingPoints route(final String elementId, final GPoint... points) { + return new ElementAndRoutingPoints(elementId, List.of(points)); + } + + private void assertNodeResized() { + assertEquals(10, node.getSize().getWidth()); + assertEquals(20, node.getSize().getHeight()); + } + + @Test + void appliesTheReportedBounds() { + assertTrue(LayoutUtil.applyBounds(bounds(NODE_ID), index).isPresent()); + assertNodeResized(); + } + + @Test + void reportsBoundsOfAnUnresolvableElementAsNotApplied() { + assertTrue(LayoutUtil.applyBounds(bounds(UNKNOWN_ID), index).isEmpty()); + } + + @Test + void reportsAlignmentOfAnUnresolvableElementAsNotApplied() { + assertTrue(LayoutUtil.applyAlignment(alignment(UNKNOWN_ID), index).isEmpty()); + } + + @Test + void appliesTheReportedRoute() { + GPoint source = GraphUtil.point(0, 0); + GPoint target = GraphUtil.point(10, 10); + + assertTrue(LayoutUtil.applyRoute(route(EDGE_ID, source, GraphUtil.point(5, 5), target), index).isPresent()); + } + + @Test + void reportsRouteOfAnUnresolvableEdgeAsNotApplied() { + assertTrue( + LayoutUtil.applyRoute(route(UNKNOWN_ID, GraphUtil.point(0, 0), GraphUtil.point(10, 10)), index).isEmpty()); + } + + @Test + void reportsRouteOfAnElementThatIsNoEdgeAsNotApplied() { + assertTrue( + LayoutUtil.applyRoute(route(NODE_ID, GraphUtil.point(0, 0), GraphUtil.point(10, 10)), index).isEmpty()); + } + + @Test + void reportsRouteWithoutSourceAndTargetPointAsNotApplied() { + assertTrue(LayoutUtil.applyRoute(route(EDGE_ID, GraphUtil.point(0, 0)), index).isEmpty()); + } + + @Test + void keepsApplyingComputedBoundsAfterAnEntryThatCannotBeApplied() { + ComputedBoundsAction action = new ComputedBoundsAction(List.of(bounds(UNKNOWN_ID), bounds(NODE_ID)), + List.of(alignment(UNKNOWN_ID)), List.of(route(EDGE_ID, GraphUtil.point(0, 0))), graph.getRevision()); + + LayoutUtil.applyBounds(graph, action, new TestGModelState(index)); + + assertNodeResized(); + } +} From b1fb4e29622e74fc62bd3f47b790912d367c2a24 Mon Sep 17 00:00:00 2001 From: Martin Fleck Date: Wed, 26 Aug 2026 11:03:50 +0200 Subject: [PATCH 2/2] GLSP-1727: Deprecate the unused applyBounds and simplify its tests - deprecate LayoutUtil.applyBounds(root, action, modelState), it calls the static per-kind methods and therefore skips the overridable ones on ComputedBoundsActionHandler - point the batch test at the live per-kind methods, so the deprecated entry point has no callers left - drop the duplicated TestGModelState from both tests, updateRoot on DefaultGModelState already builds and stores the index - assert the applied route keeps only the intermediate point and puts source and target under their args keys - keep the root parameter on the handler's applyBounds, glsp-server-node has the same signature and an override may want the root Relates to eclipse-glsp/glsp#1727 --- CHANGELOG.md | 1 + .../model/ComputedBoundsActionHandler.java | 2 +- .../eclipse/glsp/server/utils/LayoutUtil.java | 5 ++++ .../ComputedBoundsActionHandlerTest.java | 16 ++--------- .../glsp/server/utils/LayoutUtilTest.java | 28 +++++++++---------- 5 files changed, 23 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74bada85..3e3ea706 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - `LayoutUtil.applyRoute` now returns `Optional` instead of `GEdge` - `LayoutUtil.applyBounds`, `applyAlignment` and `applyRoute` no longer throw for an element the index cannot resolve, they report it as not applied. `applyRoutingPoints` stays strict. - `ComputedBoundsActionHandler` applies the computed bounds through the new overridable `applyBounds`, `applyElementBounds`, `applyAlignments` and `applyRoutes` methods, so adjusting one kind no longer means taking over `executeAction` and its model lock + - `LayoutUtil.applyBounds(GModelRoot, ComputedBoundsAction, GModelState)` is deprecated. It dispatches to the static per-kind methods directly and therefore bypasses the overridable ones of `ComputedBoundsActionHandler`. ## [v2.7.0 - 01/06/2026](https://github.com/eclipse-glsp/glsp-server/releases/tag/v2.7.0) diff --git a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java index b5a157cc..f1827a72 100644 --- a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java +++ b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandler.java @@ -63,7 +63,7 @@ public List executeAction(final ComputedBoundsAction action) { * {@link #executeAction(ComputedBoundsAction)} instead would also take over the revision check and the model lock. *

* - * @param root The model root. + * @param root The model root whose revision the action matched. * @param action The computed bounds action. */ protected void applyBounds(final GModelRoot root, final ComputedBoundsAction action) { diff --git a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java index b6d2f756..0bd486b8 100644 --- a/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java +++ b/plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/utils/LayoutUtil.java @@ -61,7 +61,12 @@ private LayoutUtil() {} * @param root The model root. * @param action The computed bounds action. * @param modelState The model state + * @deprecated Use + * {@link org.eclipse.glsp.server.features.core.model.ComputedBoundsActionHandler#applyBounds(GModelRoot, ComputedBoundsAction)} + * instead. This method dispatches to the static per-kind methods directly and therefore bypasses the + * overridable ones of the handler. */ + @Deprecated public static void applyBounds(final GModelRoot root, final ComputedBoundsAction action, final GModelState modelState) { GModelIndex index = modelState.getIndex(); diff --git a/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java index 77fc1240..12e251e2 100644 --- a/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java +++ b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/features/core/model/ComputedBoundsActionHandlerTest.java @@ -41,18 +41,6 @@ public class ComputedBoundsActionHandlerTest { private static final String NODE_ID = "node0"; private static final String EDGE_ID = "edge0"; - /** Exposes a fixed index, the injected one is not available outside of Guice. */ - private static class TestGModelState extends DefaultGModelState { - private final GModelIndex testIndex; - - TestGModelState(final GModelIndex testIndex) { - this.testIndex = testIndex; - } - - @Override - public GModelIndex getIndex() { return testIndex; } - } - /** Records which per-kind methods the apply step reached, and drops the routes. */ private static class TestHandler extends ComputedBoundsActionHandler { private final List applied = new ArrayList<>(); @@ -111,7 +99,9 @@ void setUpGraph() { graph.getChildren().add(target); graph.getChildren().add(edge); - handler = new TestHandler(new TestGModelState(GModelIndex.create(graph))); + DefaultGModelState modelState = new DefaultGModelState(); + modelState.updateRoot(graph); + handler = new TestHandler(modelState); } private ComputedBoundsAction computedBounds() { diff --git a/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java index 0af0affb..d6eaa15b 100644 --- a/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java +++ b/tests/org.eclipse.glsp.server.test/src/org/eclipse/glsp/server/utils/LayoutUtilTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; +import java.util.Optional; import org.eclipse.glsp.graph.GEdge; import org.eclipse.glsp.graph.GGraph; @@ -26,9 +27,9 @@ import org.eclipse.glsp.graph.GNode; import org.eclipse.glsp.graph.GPoint; import org.eclipse.glsp.graph.GraphFactory; +import org.eclipse.glsp.graph.builder.impl.GArguments; import org.eclipse.glsp.graph.util.GraphUtil; import org.eclipse.glsp.server.features.core.model.ComputedBoundsAction; -import org.eclipse.glsp.server.model.DefaultGModelState; import org.eclipse.glsp.server.types.ElementAndAlignment; import org.eclipse.glsp.server.types.ElementAndBounds; import org.eclipse.glsp.server.types.ElementAndRoutingPoints; @@ -41,18 +42,6 @@ public class LayoutUtilTest { private static final String EDGE_ID = "edge0"; private static final String UNKNOWN_ID = "does-not-exist"; - /** Exposes a fixed index, the injected one is not available outside of Guice. */ - private static class TestGModelState extends DefaultGModelState { - private final GModelIndex testIndex; - - TestGModelState(final GModelIndex testIndex) { - this.testIndex = testIndex; - } - - @Override - public GModelIndex getIndex() { return testIndex; } - } - private GGraph graph; private GNode node; private GModelIndex index; @@ -124,9 +113,16 @@ void reportsAlignmentOfAnUnresolvableElementAsNotApplied() { @Test void appliesTheReportedRoute() { GPoint source = GraphUtil.point(0, 0); + GPoint middle = GraphUtil.point(5, 5); GPoint target = GraphUtil.point(10, 10); - assertTrue(LayoutUtil.applyRoute(route(EDGE_ID, source, GraphUtil.point(5, 5), target), index).isPresent()); + Optional applied = LayoutUtil.applyRoute(route(EDGE_ID, source, middle, target), index); + + assertTrue(applied.isPresent()); + // the source and target point are moved into the args, only the intermediate points remain as routing points + assertEquals(List.of(middle), applied.get().getRoutingPoints()); + assertEquals(source, applied.get().getArgs().get(GArguments.KEY_EDGE_SOURCE_POINT)); + assertEquals(target, applied.get().getArgs().get(GArguments.KEY_EDGE_TARGET_POINT)); } @Test @@ -151,7 +147,9 @@ void keepsApplyingComputedBoundsAfterAnEntryThatCannotBeApplied() { ComputedBoundsAction action = new ComputedBoundsAction(List.of(bounds(UNKNOWN_ID), bounds(NODE_ID)), List.of(alignment(UNKNOWN_ID)), List.of(route(EDGE_ID, GraphUtil.point(0, 0))), graph.getRevision()); - LayoutUtil.applyBounds(graph, action, new TestGModelState(index)); + LayoutUtil.applyElementBounds(action.getBounds(), index); + LayoutUtil.applyAlignments(action.getAlignments(), index); + LayoutUtil.applyRoutes(action.getRoutes(), index); assertNodeResized(); }