Index: src/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyHelper.java =================================================================== --- src/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyHelper.java (revision 18958) +++ src/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyHelper.java (working copy) @@ -3,7 +3,6 @@ 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,9 +42,11 @@ Node node = mv.getNearestNode(p, OsmPrimitive::isSelectable); if (node != null) { - Optional 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,13 +70,12 @@ EastNorth pEN = mv.getEastNorth(p.x, p.y); - Double bestDistance = Double.MAX_VALUE; - Double currentDistance; + double bestDistance = Double.MAX_VALUE; + double currentDistance; List> wpps = w.getNodePairs(false); Node result = null; - mainLoop: for (Node n : w.getNodes()) { EastNorth nEN = n.getEastNorth(); @@ -86,17 +86,7 @@ currentDistance = pEN.distance(nEN); - if (currentDistance < bestDistance) { - // Making sure this candidate is not behind any segment. - for (Pair 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,10 +96,33 @@ } /** + * 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> wpps, Node n, EastNorth pEN, EastNorth nEN) { + // Making sure this candidate is not behind any segment. + for (Pair 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. - * + *

* The priority is given to segments where c is in ab. Otherwise, a segment * with the largest angle apb is chosen. * @@ -125,10 +138,8 @@ 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,8 +154,9 @@ // 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 Index: test/unit/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyActionTest.java =================================================================== --- test/unit/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyActionTest.java (revision 18958) +++ test/unit/org/openstreetmap/josm/actions/mapmode/ImproveWayAccuracyActionTest.java (working copy) @@ -1,25 +1,78 @@ // 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,4 +99,58 @@ 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(); + } + } }