From 900fd936231d5343acfbe6de1cc88503abd47f0f Mon Sep 17 00:00:00 2001 From: "taylor.smock" Date: Thu, 19 Sep 2024 13:38:39 +0000 Subject: [PATCH] Fix #20908: IllegalStateException: JOSM expected to find primitive in dataset after undoing a Parallel mode action The test acts (roughly) like a user would, and performs click and drags via a new utility class (`MapModeUtils`) which has various methods for performing mouse actions on the map view. git-svn-id: https://josm.openstreetmap.de/svn/trunk@19227 0c6e7542-c601-0410-84e7-c038aed88b3b --- .../actions/mapmode/ParallelWayAction.java | 6 +- .../mapmode/ParallelWayActionTest.java | 53 +++++++-- .../josm/testutils/MapModeUtils.java | 103 ++++++++++++++++++ .../josm/tools/ExceptionUtilTest.java | 5 +- 4 files changed, 153 insertions(+), 14 deletions(-) create mode 100644 test/unit/org/openstreetmap/josm/testutils/MapModeUtils.java diff --git a/src/org/openstreetmap/josm/actions/mapmode/ParallelWayAction.java b/src/org/openstreetmap/josm/actions/mapmode/ParallelWayAction.java index 1b9ec3986d1..c864d4e7879 100644 --- a/src/org/openstreetmap/josm/actions/mapmode/ParallelWayAction.java +++ b/src/org/openstreetmap/josm/actions/mapmode/ParallelWayAction.java @@ -12,11 +12,13 @@ import java.awt.Point; import java.awt.event.KeyEvent; import java.awt.event.MouseEvent; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.EnumMap; import java.util.EnumSet; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -286,7 +288,9 @@ public void mousePressed(MouseEvent e) { // Since the created way is left selected, we need to unselect again here if (pWays != null && pWays.getWays() != null) { - getLayerManager().getEditDataSet().clearSelection(pWays.getWays()); + final List ways = new ArrayList<>(pWays.getWays()); + ways.removeIf(w -> w.getDataSet() == null); + getLayerManager().getEditDataSet().clearSelection(ways); pWays = null; } diff --git a/test/unit/org/openstreetmap/josm/actions/mapmode/ParallelWayActionTest.java b/test/unit/org/openstreetmap/josm/actions/mapmode/ParallelWayActionTest.java index cfef89a7073..615aec10050 100644 --- a/test/unit/org/openstreetmap/josm/actions/mapmode/ParallelWayActionTest.java +++ b/test/unit/org/openstreetmap/josm/actions/mapmode/ParallelWayActionTest.java @@ -4,16 +4,21 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.actions.mapmode.ParallelWayAction.Mode; import org.openstreetmap.josm.actions.mapmode.ParallelWayAction.Modifier; +import org.openstreetmap.josm.data.UndoRedoHandler; +import org.openstreetmap.josm.data.coor.LatLon; import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.MapFrame; import org.openstreetmap.josm.gui.layer.OsmDataLayer; +import org.openstreetmap.josm.testutils.MapModeUtils; import org.openstreetmap.josm.testutils.annotations.Main; import org.openstreetmap.josm.testutils.annotations.Projection; +import org.openstreetmap.josm.tools.Logging; /** * Unit tests for class {@link ParallelWayAction}. @@ -21,23 +26,47 @@ @Main @Projection class ParallelWayActionTest { + private MapFrame map; + private ParallelWayAction mapMode; + private DataSet dataSet; + + @BeforeEach + void setup() { + OsmDataLayer layer = new OsmDataLayer(new DataSet(), "ParallelWayActionTest", null); + MainApplication.getLayerManager().addLayer(layer); + this.map = MainApplication.getMap(); + this.mapMode = new ParallelWayAction(this.map); + this.dataSet = layer.getDataSet(); + } + /** * Unit test of {@link ParallelWayAction#enterMode} and {@link ParallelWayAction#exitMode}. */ @Test void testMode() { - OsmDataLayer layer = new OsmDataLayer(new DataSet(), "", null); - try { - MainApplication.getLayerManager().addLayer(layer); - MapFrame map = MainApplication.getMap(); - ParallelWayAction mapMode = new ParallelWayAction(map); - MapMode oldMapMode = map.mapMode; - assertTrue(map.selectMapMode(mapMode)); - assertEquals(mapMode, map.mapMode); - assertTrue(map.selectMapMode(oldMapMode)); - } finally { - MainApplication.getLayerManager().removeLayer(layer); - } + final MapMode oldMapMode = this.map.mapMode; + assertTrue(this.map.selectMapMode(mapMode)); + assertEquals(this.mapMode, this.map.mapMode); + assertTrue(this.map.selectMapMode(oldMapMode)); + } + + /** + * Non-regression test for #20908 + */ + @Test + void testNonRegression20908() { + Logging.clearLastErrorAndWarnings(); + this.map.selectMapMode(this.map.mapModeDraw); + MapModeUtils.clickAt(LatLon.ZERO); + MapModeUtils.clickAt(2, new LatLon(0.0001, 0)); + assertEquals(3, this.dataSet.allPrimitives().size()); + this.map.selectMapMode(mapMode); + MapModeUtils.dragFromTo(new LatLon(0.00005, 0), new LatLon(0.00005, 0.0001)); + assertEquals(6, this.dataSet.allPrimitives().size()); + UndoRedoHandler.getInstance().undo(); + assertEquals(3, this.dataSet.allPrimitives().size()); + this.map.mapMode.mousePressed(MapModeUtils.mouseClickAt(new LatLon(0.00005, 0.0001))); + assertTrue(Logging.getLastErrorAndWarnings().isEmpty(), String.join("\n", Logging.getLastErrorAndWarnings())); } /** diff --git a/test/unit/org/openstreetmap/josm/testutils/MapModeUtils.java b/test/unit/org/openstreetmap/josm/testutils/MapModeUtils.java new file mode 100644 index 00000000000..3810ebb4cfa --- /dev/null +++ b/test/unit/org/openstreetmap/josm/testutils/MapModeUtils.java @@ -0,0 +1,103 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.testutils; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.awt.event.MouseEvent; + +import org.awaitility.Awaitility; +import org.awaitility.Durations; +import org.openstreetmap.josm.data.coor.ILatLon; +import org.openstreetmap.josm.data.coor.LatLon; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.util.GuiHelper; + +/** + * Utils for doing stuff in the {@link org.openstreetmap.josm.actions.mapmode.MapMode} + */ +public final class MapModeUtils { + private MapModeUtils() { + // Hide the constructor + } + + /** + * Click at a specified lat/lon + * Note that we use {@link org.openstreetmap.josm.actions.mapmode.MapMode} from {@link org.openstreetmap.josm.gui.MapFrame} + * from {@link MainApplication#getMap()}. + * @param coordinates The coordinates to click at (lat, lon, lat, lon, ...) + */ + public static void clickAt(double... coordinates) { + assertEquals(0, coordinates.length % 2, "coordinates must be a multiple of 2"); + for (int i = 0; i < coordinates.length; i += 2) { + clickAt(new LatLon(coordinates[i], coordinates[i + 1])); + } + } + + /** + * Click at a specified lat/lon + * Note that we use {@link org.openstreetmap.josm.actions.mapmode.MapMode} from {@link org.openstreetmap.josm.gui.MapFrame} + * from {@link MainApplication#getMap()}. + * @param coordinates The coordinates to click at + */ + public static void clickAt(ILatLon... coordinates) { + assertEquals(0, coordinates.length % 2, "coordinates must be a multiple of 2"); + for (ILatLon coordinate : coordinates) { + clickAt(coordinate); + } + } + + /** + * Click at a specified lat/lon + * Note that we use {@link org.openstreetmap.josm.actions.mapmode.MapMode} from {@link org.openstreetmap.josm.gui.MapFrame} + * from {@link MainApplication#getMap()}. + * @param location The location to click at + */ + public static void clickAt(ILatLon location) { + clickAt(1, location); + } + + /** + * Click at a specified lat/lon + * Note that we use {@link org.openstreetmap.josm.actions.mapmode.MapMode} from {@link org.openstreetmap.josm.gui.MapFrame} + * from {@link MainApplication#getMap()}. + * @param location The location to click at + * @param times The number of times to click + */ + public static void clickAt(int times, ILatLon location) { + for (int i = 0; i < times; i++) { + final var click = mouseClickAt(location); + MainApplication.getMap().mapMode.mousePressed(click); + MainApplication.getMap().mapMode.mouseReleased(click); + GuiHelper.runInEDTAndWait(() -> { /* Sync UI thread */ }); + } + } + + /** + * Perform a click-n-drag operation + * @param from The originating point + * @param to The end point + */ + public static void dragFromTo(ILatLon from, ILatLon to) { + MainApplication.getMap().mapMode.mousePressed(mouseClickAt(from)); + // Some actions wait a period of time to avoid accidental dragging. + Awaitility.await().pollDelay(Durations.FIVE_HUNDRED_MILLISECONDS).atLeast(Durations.FIVE_HUNDRED_MILLISECONDS).until(() -> true); + MainApplication.getMap().mapMode.mouseDragged(mouseClickAt(from)); + MainApplication.getMap().mapMode.mouseDragged(mouseClickAt(to)); + MainApplication.getMap().mapMode.mouseReleased(mouseClickAt(to)); + GuiHelper.runInEDTAndWait(() -> { /* Sync UI thread */ }); + } + + /** + * Create the click event + * @param location The location for the click event + * @return The click event + */ + public static MouseEvent mouseClickAt(ILatLon location) { + final var mapView = MainApplication.getMap().mapView; + mapView.zoomTo(mapView.getCenter(), 0.005); + mapView.zoomTo(location); + final var point = mapView.getPoint(location); + return new MouseEvent(MainApplication.getMap(), Long.hashCode(System.currentTimeMillis()), + System.currentTimeMillis(), 0, point.x, point.y, 1, false, MouseEvent.BUTTON1); + } +} diff --git a/test/unit/org/openstreetmap/josm/tools/ExceptionUtilTest.java b/test/unit/org/openstreetmap/josm/tools/ExceptionUtilTest.java index 11b91e61c2e..c5178d38633 100644 --- a/test/unit/org/openstreetmap/josm/tools/ExceptionUtilTest.java +++ b/test/unit/org/openstreetmap/josm/tools/ExceptionUtilTest.java @@ -86,6 +86,7 @@ static Stream testExplainBadRequest() { new OsmApiException(HttpURLConnection.HTTP_BAD_REQUEST, "You requested too many nodes", "")) ); } + /** * Test of {@link ExceptionUtil#explainBadRequest} method. */ @@ -93,7 +94,9 @@ static Stream testExplainBadRequest() { @MethodSource @SuppressWarnings("unchecked") void testExplainBadRequest(Supplier message, Object exception) { - assertEquals(message.get(), ExceptionUtil.explainBadRequest(exception instanceof Supplier ? ((Supplier) exception).get() : (OsmApiException) exception)); + assertEquals(message.get(), ExceptionUtil.explainBadRequest( + exception instanceof Supplier ? ((Supplier) exception).get() : (OsmApiException) exception + )); } /**