Changeset 18963 in josm

Timestamp:

2024-01-30T14:33:14+01:00 (12 months ago)

Author:

taylor.smock

Message:

Fix #23444: NPE in org.openstreetmap.josm.gui.MapViewState$MapViewEastNorthPoint.<init>

This occurs when an incomplete way is loaded into JOSM (by an .osm file, for
example). This is fixed by checking to see if the way is usable (not incomplete,
not deleted, and more importantly for this ticket, no nodes are incomplete).

This additionally fixes some pmd issues and sonarlint issues.

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyHelper.java (modified) (6 diffs)
• src/org/openstreetmap/josm/data/validation/ValidationTask.java (modified) (1 diff)
• src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyActionTest.java (modified) (2 diffs)

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

@@ -44 +44 @@
 
         if (node != null) {
-            Optional<Way> candidate = node.referrers(Way.class).findFirst();
+            Optional<Way> candidate = node.referrers(Way.class).filter(Way::isUsable).findFirst();
             if (candidate.isPresent()) {
                 return candidate.get();
@@ -70 +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();
@@ -87 +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;
@@ -107 +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.
@@ -126 +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;
@@ -144 +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,

trunk/src/org/openstreetmap/josm/data/validation/ValidationTask.java

@@ -112 +112 @@
 
     }
+
     protected ValidationTask(ProgressMonitor progressMonitor,
             Collection<Test> tests,

trunk/src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java

@@ -354 +354 @@
         return selection;
     }
+
     static boolean isCoastline(OsmPrimitive w) {
         return w.hasTag("natural", "water", "coastline") || w.hasTag(LANDUSE, "reservoir");

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

@@ -2 +2 @@
 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}.
@@ -45 +98 @@
     @Test
     void testEnumState() {
-        TestUtils.superficialEnumCodeCoverage(State.class);
+        assertDoesNotThrow(() -> TestUtils.superficialEnumCodeCoverage(State.class));
+    }
+
+    @Test
+    void testNonRegression23444() {
+        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();
+        }
     }
 }