Ticket #23444: 23444.patch

src/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyHelper.java

@@ -3 +3 @@
 
 import java.awt.Point;
 import java.util.List;
-import java.util.Optional;
 
 import org.openstreetmap.josm.data.coor.EastNorth;
 import org.openstreetmap.josm.data.osm.Node;
@@ -43 +42 @@
         Node node = mv.getNearestNode(p, OsmPrimitive::isSelectable);
 
         if (node != null) {
-            Optional<Way> candidate = node.referrers(Way.class).findFirst();
-            if (candidate.isPresent()) {
-                return candidate.get();
+            for (Way way : node.getParentWays()) {
+                // We don't want to return ways that are not complete
+                if (way.isUsable()) {
+                    return way;
+                }
             }
         }
 
@@ -69 +70 @@
 
         EastNorth pEN = mv.getEastNorth(p.x, p.y);
 
-        Double bestDistance = Double.MAX_VALUE;
-        Double currentDistance;
+        double bestDistance = Double.MAX_VALUE;
+        double currentDistance;
         List<Pair<Node, Node>> wpps = w.getNodePairs(false);
 
         Node result = null;
 
-        mainLoop:
         for (Node n : w.getNodes()) {
             EastNorth nEN = n.getEastNorth();
 
@@ -86 +86 @@
 
             currentDistance = pEN.distance(nEN);
 
-            if (currentDistance < bestDistance) {
-                // Making sure this candidate is not behind any segment.
-                for (Pair<Node, Node> wpp : wpps) {
-                    if (!wpp.a.equals(n)
-                            && !wpp.b.equals(n)
-                            && Geometry.getSegmentSegmentIntersection(
-                            wpp.a.getEastNorth(), wpp.b.getEastNorth(),
-                            pEN, nEN) != null) {
-                        continue mainLoop;
-                    }
-                }
+            if (currentDistance < bestDistance && ensureCandidateIsNotBehindSegments(wpps, n, pEN, nEN)) {
                 result = n;
                 bestDistance = currentDistance;
             }
@@ -106 +96 @@
     }
 
     /**
+     * Check to see if a candidate node is underneath a way segment
+     *
+     * @param wpps The pairs of nodes to check for crossing way segments
+     * @param n The current node to check
+     * @param pEN The cursor east-north position
+     * @param nEN The node east-north position
+     * @return {@code true} if the candidate node is underneath a way segment
+     */
+    private static boolean ensureCandidateIsNotBehindSegments(Iterable<Pair<Node, Node>> wpps, Node n, EastNorth pEN, EastNorth nEN) {
+        // Making sure this candidate is not behind any segment.
+        for (Pair<Node, Node> wpp : wpps) {
+            if (!wpp.a.equals(n)
+                    && !wpp.b.equals(n)
+                    && Geometry.getSegmentSegmentIntersection(
+                    wpp.a.getEastNorth(), wpp.b.getEastNorth(),
+                    pEN, nEN) != null) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    /**
      * Returns the nearest way segment to cursor. The distance to segment ab is
      * the length of altitude from p to ab (say, c) or the minimum distance from
      * p to a or b if c is out of ab.
-     *
+     * <p>
      * The priority is given to segments where c is in ab. Otherwise, a segment
      * with the largest angle apb is chosen.
      *
@@ -125 +138 @@
 
         EastNorth pEN = mv.getEastNorth(p.x, p.y);
 
-        Double currentDistance;
-        Double currentAngle;
-        Double bestDistance = Double.MAX_VALUE;
-        Double bestAngle = 0.0;
+        double bestDistance = Double.MAX_VALUE;
+        double bestAngle = 0.0;
 
         int candidate = -1;
 
@@ -143 +154 @@
 
             // Finding intersection of the segment with its altitude from p
             EastNorth altitudeIntersection = Geometry.closestPointToSegment(a, b, pEN);
-            currentDistance = pEN.distance(altitudeIntersection);
+            final double currentDistance = pEN.distance(altitudeIntersection);
 
+            final double currentAngle;
             if (!altitudeIntersection.equals(a) && !altitudeIntersection.equals(b)) {
                 // If the segment intersects with the altitude from p,
                 // make an angle too big to let this candidate win any others

test/unit/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyActionTest.java

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.actions.mapmode;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.awt.Graphics2D;
+import java.awt.Point;
+import java.awt.event.MouseEvent;
+import java.awt.image.BufferedImage;
+
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.actions.mapmode.ImproveWayAccuracyAction.State;
+import org.openstreetmap.josm.data.Bounds;
+import org.openstreetmap.josm.data.coor.ILatLon;
+import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.MapFrame;
+import org.openstreetmap.josm.gui.MapView;
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
+import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.testutils.annotations.Main;
 import org.openstreetmap.josm.testutils.annotations.Projection;
+import org.openstreetmap.josm.testutils.mockers.WindowlessMapViewStateMocker;
+import org.openstreetmap.josm.testutils.mockers.WindowlessNavigatableComponentMocker;
 
+import mockit.Mock;
+
 /**
  * Unit tests for class {@link ImproveWayAccuracyAction}.
  */
-@Main
 @Projection
 class ImproveWayAccuracyActionTest {
+    @RegisterExtension
+    Main.MainExtension mainExtension = new Main.MainExtension().setMapViewMocker(SizedWindowlessMapViewStateMocker::new)
+            .setNavigableComponentMocker(SizedWindowlessNavigatableComponentMocker::new);
+
+    private static final int WIDTH = 800;
+    private static final int HEIGHT = 600;
+    private static class SizedWindowlessMapViewStateMocker extends WindowlessMapViewStateMocker {
+        @Mock
+        public int getWidth() {
+            return WIDTH;
+        }
+
+        @Mock
+        public int getHeight() {
+            return HEIGHT;
+        }
+    }
+
+    private static class SizedWindowlessNavigatableComponentMocker extends WindowlessNavigatableComponentMocker {
+        @Mock
+        public int getWidth() {
+            return WIDTH;
+        }
+
+        @Mock
+        public int getHeight() {
+            return HEIGHT;
+        }
+    }
+
+    private static MouseEvent generateEvent(MapView mapView, ILatLon location) {
+        final Point p = mapView.getPoint(location);
+        return new MouseEvent(mapView, 0, 0, 0, p.x, p.y, p.x, p.y, 1, false, MouseEvent.BUTTON1);
+    }
+
     /**
      * Unit test of {@link ImproveWayAccuracyAction#enterMode} and {@link ImproveWayAccuracyAction#exitMode}.
      */
@@ -46 +99 @@
     void testEnumState() {
         TestUtils.superficialEnumCodeCoverage(State.class);
     }
+
+    @Test
+    void testNonRegression23444() {
+        final int width = 800;
+        final int height = 600;
+        final DataSet dataSet = new DataSet();
+        final OsmDataLayer layer = new OsmDataLayer(dataSet, "ImproveWayAccuracyActionT", null);
+        MainApplication.getLayerManager().addLayer(layer);
+        final ImproveWayAccuracyAction mapMode = new ImproveWayAccuracyAction();
+        final MapFrame map = MainApplication.getMap();
+        assertTrue(map.selectMapMode(mapMode));
+        assertEquals(mapMode, map.mapMode);
+        final Way testWay = TestUtils.newWay("", new Node(1, 1), new Node(2, 1),
+                new Node(3), new Node(4, 1), new Node(5, 1));
+        testWay.firstNode().setCoor(new LatLon(0, 0));
+        testWay.lastNode().setCoor(new LatLon(0.001, 0.001));
+        testWay.getNode(1).setCoor(new LatLon(0.0001, 0.0001));
+        testWay.getNode(3).setCoor(new LatLon(0.0009, 0.0009));
+        dataSet.addPrimitiveRecursive(testWay);
+        assertFalse(testWay.getNode(2).isLatLonKnown(), "The second node should not have valid coordinates");
+        final Graphics2D g2d = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB).createGraphics();
+        g2d.setClip(0, 0, WIDTH, HEIGHT);
+        try {
+            // If this fails, something else is wrong
+            assertDoesNotThrow(() -> map.mapView.paint(g2d), "The mapview should be able to handle a null coordinate node");
+            // Ensure that the test way is selected (and use the methods from the action to do so)
+            GuiHelper.runInEDTAndWaitWithException(() -> {
+                map.mapView.zoomTo(new Bounds(0, 0, 0.001, 0.001));
+                // Get the target way selected (note: not selected in dataset -- use mapMode.mapReleased for that)
+                mapMode.mouseMoved(generateEvent(map.mapView, testWay.getNode(1)));
+            });
+            // mouseMoved shouldn't cause the way to get selected
+            assertFalse(dataSet.getAllSelected().contains(testWay));
+
+            // Now check painting (where the problem should occur; the mapMode.paint call should be called as part of the map.mapView.paint call)
+            assertDoesNotThrow(() -> map.mapView.paint(g2d));
+            assertDoesNotThrow(() -> mapMode.paint(g2d, map.mapView, new Bounds(0, 0, 0.001, 0.001)));
+
+            // Finally, check painting during selection
+            GuiHelper.runInEDTAndWaitWithException(() -> {
+                // Set the way as selected
+                mapMode.mouseReleased(generateEvent(map.mapView, new LatLon(0.0001, 0.0001)));
+                // Set the mouse location (unset in mouseReleased call)
+                mapMode.mouseMoved(generateEvent(map.mapView, new LatLon(0.0001, 0.0001)));
+            });
+            // The way shouldn't be selected, since it isn't usable for the improve way tool
+            assertFalse(dataSet.getAllSelected().contains(testWay));
+            // Now check painting again (just in case)
+            assertDoesNotThrow(() -> map.paint(g2d));
+            assertDoesNotThrow(() -> mapMode.paint(g2d, map.mapView, new Bounds(0, 0, 0.001, 0.001)));
+        } finally {
+            g2d.dispose();
+        }
+    }
 }