Index: src/org/openstreetmap/josm/actions/CombineWayAction.java =================================================================== --- src/org/openstreetmap/josm/actions/CombineWayAction.java (revision 15562) +++ src/org/openstreetmap/josm/actions/CombineWayAction.java (working copy) @@ -31,7 +31,12 @@ import org.openstreetmap.josm.data.osm.OsmUtils; import org.openstreetmap.josm.data.osm.TagCollection; import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon; +import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon.JoinedWay; import org.openstreetmap.josm.data.preferences.BooleanProperty; +import org.openstreetmap.josm.data.validation.Test; +import org.openstreetmap.josm.data.validation.tests.OverlappingWays; +import org.openstreetmap.josm.data.validation.tests.SelfIntersectingWay; import org.openstreetmap.josm.gui.ExtendedDialog; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.Notification; @@ -122,8 +127,7 @@ } // try to build a new way which includes all the combined ways - NodeGraph graph = NodeGraph.createNearlyUndirectedGraphFromNodeWays(ways); - List path = graph.buildSpanningPathNoRemove(); + List path = tryJoin(ways); if (path == null) { warnCombiningImpossible(); return null; @@ -138,10 +142,23 @@ List unreversedWays = new LinkedList<>(); for (Way w: ways) { // Treat zero or one-node ways as unreversed as Combine action action is a good way to fix them (see #8971) - if (w.getNodesCount() < 2 || (path.indexOf(w.getNode(0)) + 1) == path.lastIndexOf(w.getNode(1))) { + if (w.getNodesCount() < 2) { unreversedWays.add(w); } else { - reversedWays.add(w); + boolean foundStartSegment = false; + int last = path.lastIndexOf(w.getNode(0)); + + for (int i = path.indexOf(w.getNode(0)); i <= last; i++) { + if (path.get(i) == w.getNode(0) && i + 1 < path.size() && w.getNode(1) == path.get(i + 1)) { + foundStartSegment = true; + break; + } + } + if (foundStartSegment) { + unreversedWays.add(w); + } else { + reversedWays.add(w); + } } } // reverse path if all ways have been reversed @@ -212,6 +229,38 @@ return new Pair<>(targetWay, sequenceCommand); } + static List tryJoin(Collection ways) { + List path = joinWithMultipolygonCode(ways); + if (path == null) { + NodeGraph graph = NodeGraph.createNearlyUndirectedGraphFromNodeWays(ways); + path = graph.buildSpanningPathNoRemove(); + } + return path; + } + + /** + * Use {@link Multipolygon#joinWays(Collection)} to join ways. + * @param ways the ways + * @return List of nodes of the combined ways or null if ways could not be combined to a single way. + * Result may contain overlapping segments. + */ + private static List joinWithMultipolygonCode(Collection ways) { + // sort so that old unclosed ways appear first + LinkedList toJoin = new LinkedList<>(ways); + toJoin.sort((o1, o2) -> { + int d = Boolean.compare(o1.isNew(), o2.isNew()); + if (d == 0) + d = Boolean.compare(o1.isClosed(), o2.isClosed()); + return d; + }); + Collection list = Multipolygon.joinWays(toJoin); + if (list.size() == 1) { + // ways form a single line string + return new ArrayList<>(list.iterator().next().getNodes()); + } + return null; + } + @Override public void actionPerformed(ActionEvent event) { final DataSet ds = getLayerManager().getEditDataSet(); @@ -237,8 +286,24 @@ if (combineResult == null) return; + final Way selectedWay = combineResult.a; UndoRedoHandler.getInstance().add(combineResult.b); + Test test = new OverlappingWays(); + test.startTest(null); + test.visit(combineResult.a); + test.endTest(); + if (test.getErrors().isEmpty()) { + test = new SelfIntersectingWay(); + test.startTest(null); + test.visit(combineResult.a); + test.endTest(); + } + if (!test.getErrors().isEmpty()) { + new Notification(test.getErrors().get(0).getMessage()) + .setIcon(JOptionPane.WARNING_MESSAGE) + .show(); + } if (selectedWay != null) { GuiHelper.runInEDT(() -> ds.setSelected(selectedWay)); } @@ -261,4 +326,5 @@ } setEnabled(numWays >= 2); } + } Index: test/data/regress/18367/nocombine.osm =================================================================== --- test/data/regress/18367/nocombine.osm (nonexistent) +++ test/data/regress/18367/nocombine.osm (working copy) @@ -0,0 +1,22 @@ + + + + + + + + + + + + + + + + + + + + + + Index: test/unit/org/openstreetmap/josm/actions/CombineWayActionTest.java =================================================================== --- test/unit/org/openstreetmap/josm/actions/CombineWayActionTest.java (revision 15562) +++ test/unit/org/openstreetmap/josm/actions/CombineWayActionTest.java (working copy) @@ -2,12 +2,12 @@ package org.openstreetmap.josm.actions; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -71,9 +71,8 @@ public void testTicket18385() throws IOException, IllegalDataException { try (InputStream is = TestUtils.getRegressionDataStream(18385, "data.osm")) { DataSet ds = OsmReader.parseDataSet(is, null); - NodeGraph graph = NodeGraph.createNearlyUndirectedGraphFromNodeWays(ds.getWays()); - List path = graph.buildSpanningPathNoRemove(); - assertNull(path); + List path = CombineWayAction.tryJoin(ds.getWays()); + assertTrue(path != null); } } @@ -90,13 +89,36 @@ assertEquals(2, selection.size()); if (!selection.get(0).isNew()) Collections.reverse(selection); - NodeGraph graph = NodeGraph.createNearlyUndirectedGraphFromNodeWays(selection); - List path = graph.buildSpanningPathNoRemove(); + double expectedLen = getOriginalLength(selection); + List path = CombineWayAction.tryJoin(selection); assertTrue(path != null); + Way combined = new Way(0); + combined.setNodes(path); + assertEquals(expectedLen, combined.getLength(), 0.01); } } /** + * Non-regression test for bug #18367 (Lines cannot be combined when they share an overlapping segment) + * @throws IOException if any I/O error occurs + * @throws IllegalDataException if OSM parsing fails + */ + @Test + public void testTicket18367() throws IOException, IllegalDataException { + try (InputStream is = TestUtils.getRegressionDataStream(18367, "nocombine.osm")) { + DataSet ds = OsmReader.parseDataSet(is, null); + ArrayList selection = new ArrayList<>(ds.getWays()); + assertEquals(2, selection.size()); + double expectedLen = getOriginalLength(selection); + List path = CombineWayAction.tryJoin(selection); + assertTrue(path != null); + Way combined = new Way(0); + combined.setNodes(path); + assertEquals(expectedLen, combined.getLength(), 1e-7); + } + } + + /** * Unit test of methods {@link NodePair#equals} and {@link NodePair#hashCode}. */ @Test @@ -106,4 +128,13 @@ .withPrefabValues(Node.class, new Node(1), new Node(2)) .verify(); } + + private static double getOriginalLength(Collection ways) { + double len = 0; + for (Way w : ways) { + len += w.getLength(); + } + return len; + } + }