Index: trunk/src/org/openstreetmap/josm/actions/JoinNodeWayAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/JoinNodeWayAction.java (revision 15427) +++ trunk/src/org/openstreetmap/josm/actions/JoinNodeWayAction.java (revision 15428) @@ -12,10 +12,11 @@ import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.SortedSet; -import java.util.TreeSet; + +import javax.swing.JOptionPane; import org.openstreetmap.josm.command.ChangeCommand; @@ -33,4 +34,5 @@ import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.MapView; +import org.openstreetmap.josm.gui.Notification; import org.openstreetmap.josm.tools.Geometry; import org.openstreetmap.josm.tools.MultiMap; @@ -98,5 +100,5 @@ for (Node node : selectedNodes) { List wss = mapView.getNearestWaySegments(mapView.getPoint(node), OsmPrimitive::isSelectable); - MultiMap insertPoints = new MultiMap<>(); + Set seenWays = new HashSet<>(); for (WaySegment ws : wss) { // Maybe cleaner to pass a "isSelected" predicate to getNearestWaySegments, but this is less invasive. @@ -104,26 +106,20 @@ continue; } - - if (!ws.getFirstNode().equals(node) && !ws.getSecondNode().equals(node)) { - insertPoints.put(ws.way, ws.lowerIndex); + // only use the closest WaySegment of each way and ignore those that already contain the node + if (!ws.getFirstNode().equals(node) && !ws.getSecondNode().equals(node) + && !seenWays.contains(ws.way)) { + MultiMap innerMap = data.get(ws.way); + if (innerMap == null) { + innerMap = new MultiMap<>(); + data.put(ws.way, innerMap); + } + innerMap.put(ws.lowerIndex, node); + seenWays.add(ws.way); } } - for (Map.Entry> entry : insertPoints.entrySet()) { - final Way w = entry.getKey(); - final Set insertPointsForWay = entry.getValue(); - for (int i : pruneSuccs(insertPointsForWay)) { - MultiMap innerMap; - if (!data.containsKey(w)) { - innerMap = new MultiMap<>(); - } else { - innerMap = data.get(w); - } - innerMap.put(i, node); - data.put(w, innerMap); - } - } } // Execute phase: traverse the structure "data" and finally put the nodes into place + Map movedNodes = new HashMap<>(); for (Map.Entry> entry : data.entrySet()) { final Way w = entry.getKey(); @@ -143,10 +139,18 @@ w.getNode(segmentIndex+1).getEastNorth(), node.getEastNorth()); - MoveCommand c = new MoveCommand( - node, ProjectionRegistry.getProjection().eastNorth2latlon(newPosition)); - // Avoid moving a given node several times at the same position in case of overlapping ways - if (!cmds.contains(c)) { - cmds.add(c); + EastNorth prevMove = movedNodes.get(node); + if (prevMove != null) { + if (!prevMove.equalsEpsilon(newPosition, 1e-4)) { + new Notification(tr("Multiple target ways, no common point found. Nothing was changed.")) + .setIcon(JOptionPane.INFORMATION_MESSAGE) + .show(); + return; + } + continue; } + MoveCommand c = new MoveCommand(node, + ProjectionRegistry.getProjection().eastNorth2latlon(newPosition)); + cmds.add(c); + movedNodes.put(node, newPosition); } } @@ -166,14 +170,4 @@ } - private static SortedSet pruneSuccs(Collection is) { - SortedSet is2 = new TreeSet<>(); - for (int i : is) { - if (!is2.contains(i - 1) && !is2.contains(i + 1)) { - is2.add(i); - } - } - return is2; - } - /** * Sorts collinear nodes by their distance to a common reference node. Index: trunk/test/data/regress/11508/11508_example.osm =================================================================== --- trunk/test/data/regress/11508/11508_example.osm (revision 15428) +++ trunk/test/data/regress/11508/11508_example.osm (revision 15428) @@ -0,0 +1,32 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/regress/18189/data.osm =================================================================== --- trunk/test/data/regress/18189/data.osm (revision 15428) +++ trunk/test/data/regress/18189/data.osm (revision 15428) @@ -0,0 +1,42 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/regress/18189/moveontocrossing.osm =================================================================== --- trunk/test/data/regress/18189/moveontocrossing.osm (revision 15428) +++ trunk/test/data/regress/18189/moveontocrossing.osm (revision 15428) @@ -0,0 +1,23 @@ + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/regress/18189/moveontoway.osm =================================================================== --- trunk/test/data/regress/18189/moveontoway.osm (revision 15428) +++ trunk/test/data/regress/18189/moveontoway.osm (revision 15428) @@ -0,0 +1,16 @@ + + + + + + + + + + + + + + + + Index: trunk/test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java (revision 15428) +++ trunk/test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java (revision 15428) @@ -0,0 +1,181 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.actions; + +import static org.junit.Assert.assertTrue; + +import java.awt.Rectangle; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.Rule; +import org.junit.Test; +import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.data.coor.EastNorth; +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.data.osm.visitor.BoundingXYVisitor; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.layer.Layer; +import org.openstreetmap.josm.gui.layer.OsmDataLayer; +import org.openstreetmap.josm.io.OsmReader; +import org.openstreetmap.josm.testutils.JOSMTestRules; +import org.openstreetmap.josm.tools.Geometry; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + +/** + * Unit tests for class {@link JoinNodeWayAction}. + */ +public final class JoinNodeWayActionTest { + /** + * Setup test. + */ + @Rule + @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") + public JOSMTestRules test = new JOSMTestRules().projection().main().preferences(); + + private void setupMapView(DataSet ds) { + // setup a reasonable screen size + MainApplication.getMap().mapView.setBounds(new Rectangle(1920, 1080)); + if (ds.getDataSourceBoundingBox() != null) { + MainApplication.getMap().mapView.zoomTo(ds.getDataSourceBoundingBox()); + } else { + BoundingXYVisitor v = new BoundingXYVisitor(); + for (Layer l : MainApplication.getLayerManager().getLayers()) { + l.visitBoundingBox(v); + } + MainApplication.getMap().mapView.zoomTo(v); + } + } + + /** + * Test case: Move node onto two almost overlapping ways + * see #18189 moveontoway.osm + * @throws Exception if an error occurs + */ + @Test + public void testTicket18189() throws Exception { + DataSet dataSet = new DataSet(); + OsmDataLayer layer = new OsmDataLayer(dataSet, OsmDataLayer.createNewName(), null); + MainApplication.getLayerManager().addLayer(layer); + try { + Node n1 = new Node(new LatLon(59.92881498658, 30.30104052971)); + Node n2 = new Node(new LatLon(59.92881459851, 30.30104056556)); + Node n3 = new Node(new LatLon(59.92881498658, 30.3010405297)); + Node n4 = new Node(new LatLon(59.92881459851, 30.30104056556)); + Node n5 = new Node(new LatLon(59.92881483122, 30.30104056465)); + + dataSet.addPrimitive(n1); + dataSet.addPrimitive(n2); + dataSet.addPrimitive(n3); + dataSet.addPrimitive(n4); + dataSet.addPrimitive(n5); + + Way w1 = new Way(); + w1.setNodes(Arrays.asList(n1, n2)); + dataSet.addPrimitive(w1); + Way w2 = new Way(); + w2.setNodes(Arrays.asList(n3, n4)); + dataSet.addPrimitive(w2); + + dataSet.addSelected(n5); + EastNorth expected = Geometry.closestPointToSegment(n1.getEastNorth(), n2.getEastNorth(), n5.getEastNorth()); + + setupMapView(dataSet); + JoinNodeWayAction action = JoinNodeWayAction.createMoveNodeOntoWayAction(); + action.setEnabled(true); + action.actionPerformed(null); + // Make sure the node was only moved once + assertTrue("Node n5 wasn't added to way w1.", w1.containsNode(n5)); + assertTrue("Node n5 wasn't added to way w2.", w2.containsNode(n5)); + assertTrue("Node was moved to an unexpected position", n5.getEastNorth().equalsEpsilon(expected, 1e-7)); + } finally { + MainApplication.getLayerManager().removeLayer(layer); + } + } + + /** + * Non-regression test for Bug #11508. + * @throws Exception if an error occurs + */ + @Test + public void testTicket11508() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(11508, "11508_example.osm"), null); + Layer layer = new OsmDataLayer(ds, OsmDataLayer.createNewName(), null); + MainApplication.getLayerManager().addLayer(layer); + try { + List nodesToMove = ds.getNodes().stream().filter(n -> n.hasTag("name", "select me and press N")) + .collect(Collectors.toList()); + assertTrue(nodesToMove.size() == 1); + Node toMove = nodesToMove.iterator().next(); + Node expected = new Node(new LatLon(47.56331849690742, 8.800789259499311)); + ds.setSelected(toMove); + setupMapView(ds); + JoinNodeWayAction action = JoinNodeWayAction.createMoveNodeOntoWayAction(); + action.setEnabled(true); + action.actionPerformed(null); + + assertTrue("Node was moved to an unexpected position", toMove.getEastNorth().equalsEpsilon(expected.getEastNorth(), 1e-7)); + assertTrue("Node was not added to expected number of ways", toMove.getParentWays().size() == 2); + } finally { + MainApplication.getLayerManager().removeLayer(layer); + } + } + + /** + * Check that nothing is changed if ways are too far. + * @throws Exception if an error occurs + */ + @Test + public void testTicket18189Crossing() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(18189, "moveontocrossing.osm"), null); + Layer layer = new OsmDataLayer(ds, OsmDataLayer.createNewName(), null); + MainApplication.getLayerManager().addLayer(layer); + try { + setupMapView(ds); + JoinNodeWayAction action = JoinNodeWayAction.createMoveNodeOntoWayAction(); + action.setEnabled(true); + List nodesToMove = ds.getNodes().stream().filter(n -> n.hasTag("name", "select me and press N")) + .collect(Collectors.toList()); + assertTrue(nodesToMove.size() == 1); + Node toMove = nodesToMove.iterator().next(); + ds.setSelected(toMove); + action.actionPerformed(null); + assertTrue(toMove.getParentWays().isEmpty()); + } finally { + MainApplication.getLayerManager().removeLayer(layer); + } + } + + /** + * Check that nothing is changed if ways are too far. + * @throws Exception if an error occurs + */ + @Test + public void testTicket18189ThreeWays() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(18189, "data.osm"), null); + Layer layer = new OsmDataLayer(ds, OsmDataLayer.createNewName(), null); + MainApplication.getLayerManager().addLayer(layer); + try { + setupMapView(ds); + JoinNodeWayAction action = JoinNodeWayAction.createMoveNodeOntoWayAction(); + action.setEnabled(true); + List nodesToMove = ds.getNodes().stream().filter(n -> n.hasTag("name", "select me and press N")) + .collect(Collectors.toList()); + assertTrue(nodesToMove.size() == 1); + Node toMove = nodesToMove.iterator().next(); + Node expected = new Node(new LatLon(-21.088998104148224, -50.38629102179512)); + ds.setSelected(toMove); + action.actionPerformed(null); + assertTrue("Node was moved to an unexpected position", toMove.getEastNorth().equalsEpsilon(expected.getEastNorth(), 1e-7)); + assertTrue("Node was not added to expected number of ways", toMove.getParentWays().size() == 4); + + } finally { + MainApplication.getLayerManager().removeLayer(layer); + } + } + +}