Index: trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java =================================================================== --- trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 18538) +++ trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 18539) @@ -488,5 +488,5 @@ if (way.lastNode() == way.firstNode()) { // Self-closing way. - direction = Direction.IRRELEVANT; + direction = direction.merge(Direction.IRRELEVANT); } else { // For ordered relations, looking beyond the nearest neighbour members is not required, @@ -498,7 +498,7 @@ else { if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { - direction = Direction.FORWARDS; + direction = direction.merge(Direction.FORWARDS); } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { - direction = Direction.BACKWARDS; + direction = direction.merge(Direction.BACKWARDS); } } @@ -510,7 +510,7 @@ else { if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { - direction = Direction.BACKWARDS; + direction = direction.merge(Direction.BACKWARDS); } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { - direction = Direction.FORWARDS; + direction = direction.merge(Direction.FORWARDS); } } @@ -529,7 +529,7 @@ Way w = r.getMember(ir - k).getWay(); if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { - direction = Direction.FORWARDS; + direction = direction.merge(Direction.FORWARDS); } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { - direction = Direction.BACKWARDS; + direction = direction.merge(Direction.BACKWARDS); } break; @@ -538,7 +538,7 @@ Way w = r.getMember(ir + k).getWay(); if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { - direction = Direction.BACKWARDS; + direction = direction.merge(Direction.BACKWARDS); } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { - direction = Direction.FORWARDS; + direction = direction.merge(Direction.FORWARDS); } break; @@ -952,5 +952,28 @@ BACKWARDS, UNKNOWN, - IRRELEVANT + IRRELEVANT; + + /** + * Merge directions (this helps avoid overriding {@link #FORWARDS} with {@link #BACKWARDS}). + * @param other The other direction to merge. {@link #UNKNOWN} will be overridden. + * @return The merged direction + */ + Direction merge(Direction other) { + if (this == other) { + return this; + } + if (this == IRRELEVANT || other == IRRELEVANT || + (this == FORWARDS && other == BACKWARDS) || + (other == FORWARDS && this == BACKWARDS)) { + return IRRELEVANT; + } + if (this == UNKNOWN) { + return other; + } + if (other == UNKNOWN) { + return this; + } + return UNKNOWN; + } } Index: trunk/src/org/openstreetmap/josm/data/osm/DataSet.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/DataSet.java (revision 18538) +++ trunk/src/org/openstreetmap/josm/data/osm/DataSet.java (revision 18539) @@ -531,9 +531,14 @@ */ public void addPrimitiveRecursive(OsmPrimitive primitive) { + Stream children; if (primitive instanceof Way) { - ((Way) primitive).getNodes().forEach(n -> addPrimitiveRecursive(n)); + children = ((Way) primitive).getNodes().stream().map(OsmPrimitive.class::cast); } else if (primitive instanceof Relation) { - ((Relation) primitive).getMembers().forEach(m -> addPrimitiveRecursive(m.getMember())); - } + children = ((Relation) primitive).getMembers().stream().map(RelationMember::getMember); + } else { + children = Stream.empty(); + } + // Relations may have the same member multiple times. + children.filter(p -> !Objects.equals(this, p.getDataSet())).forEach(this::addPrimitiveRecursive); addPrimitive(primitive); } Index: trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 18538) +++ trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 18539) @@ -2,4 +2,5 @@ package org.openstreetmap.josm.command; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -12,8 +13,14 @@ import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Optional; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.command.SplitWayCommand.Strategy; @@ -27,4 +34,6 @@ import org.openstreetmap.josm.data.osm.RelationMember; import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType; +import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator; import org.openstreetmap.josm.io.IllegalDataException; import org.openstreetmap.josm.io.OsmReader; @@ -43,5 +52,5 @@ @RegisterExtension @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") - public JOSMTestRules test = new JOSMTestRules().main().projection().preferences(); + static JOSMTestRules test = new JOSMTestRules().main().projection().preferences(); /** @@ -77,20 +86,19 @@ } + static Stream testRouteRelation() { + Stream.Builder builder = Stream.builder(); + for (int i = 0; i < 4; i++) { + builder.add(Arguments.of(false, i)); + builder.add(Arguments.of(true, i)); + } + return builder.build(); + } + /** * Unit tests of route relations. */ - @Test - void testRouteRelation() { - doTestRouteRelation(false, 0); - doTestRouteRelation(false, 1); - doTestRouteRelation(false, 2); - doTestRouteRelation(false, 3); - doTestRouteRelation(true, 0); - doTestRouteRelation(true, 1); - doTestRouteRelation(true, 2); - doTestRouteRelation(true, 3); - } - - void doTestRouteRelation(final boolean wayIsReversed, final int indexOfWayToKeep) { + @ParameterizedTest + @MethodSource + void testRouteRelation(final boolean wayIsReversed, final int indexOfWayToKeep) { final DataSet dataSet = new DataSet(); final Node n1 = new Node(new LatLon(1, 0)); @@ -187,16 +195,19 @@ } - @Test - void testDoIncompleteMembersOrderedRelationCorrectOrderTest() { + static Stream testIncompleteMembersOrderedRelationCorrectOrderTest() { + Stream.Builder builder = Stream.builder(); for (int i = 0; i < 2; i++) { // All these permutations should result in a split that keeps the new parts in order. - doIncompleteMembersOrderedRelationCorrectOrderTest(false, false, i); - doIncompleteMembersOrderedRelationCorrectOrderTest(true, false, i); - doIncompleteMembersOrderedRelationCorrectOrderTest(true, true, i); - doIncompleteMembersOrderedRelationCorrectOrderTest(false, true, i); - } - } - - private void doIncompleteMembersOrderedRelationCorrectOrderTest(final boolean reverseWayOne, + builder.add(Arguments.of(false, false, i)); + builder.add(Arguments.of(true, false, i)); + builder.add(Arguments.of(true, true, i)); + builder.add(Arguments.of(false, true, i)); + } + return builder.build(); + } + + @ParameterizedTest + @MethodSource + void testIncompleteMembersOrderedRelationCorrectOrderTest(final boolean reverseWayOne, final boolean reverseWayTwo, final int indexOfWayToKeep) { @@ -434,3 +445,56 @@ } + /** + * Test case: smart ordering in routes + * See #21856 + */ + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testTicket21856(boolean reverse) { + Way way1 = TestUtils.newWay("highway=residential", TestUtils.newNode(""), TestUtils.newNode("")); + way1.setOsmId(23_968_090, 1); + way1.lastNode().setOsmId(6_823_898_683L, 1); + Way way2 = TestUtils.newWay("highway=residential", way1.lastNode(), TestUtils.newNode("")); + way2.setOsmId(728_199_307, 1); + way2.lastNode().setOsmId(6_823_898_684L, 1); + Node splitNode = TestUtils.newNode(""); + splitNode.setOsmId(6_823_906_290L, 1); + Way splitWay = TestUtils.newWay("highway=service", way2.firstNode(), splitNode, TestUtils.newNode(""), way2.lastNode()); + // The behavior should be the same regardless of the direction of the way + if (reverse) { + List nodes = new ArrayList<>(splitWay.getNodes()); + Collections.reverse(nodes); + splitWay.setNodes(nodes); + } + splitWay.setOsmId(728_199_306, 1); + Relation route = TestUtils.newRelation("type=route route=bus", new RelationMember("", way1), new RelationMember("", splitWay), + new RelationMember("", way2), new RelationMember("", way1)); + DataSet dataSet = new DataSet(); + dataSet.addPrimitiveRecursive(route); + dataSet.setSelected(splitNode); + // Sanity check (preconditions -- the route should be well-formed already) + WayConnectionTypeCalculator connectionTypeCalculator = new WayConnectionTypeCalculator(); + List links = connectionTypeCalculator.updateLinks(route, route.getMembers()); + assertAll("All links should be connected (forward)", + links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext))); + assertAll("All links should be connected (backward)", + links.subList(1, links.size() - 1).stream().map(link -> () -> assertTrue(link.linkPrev))); + final Optional result = SplitWayCommand.splitWay( + splitWay, + SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)), + new ArrayList<>(), + Strategy.keepLongestChunk(), + // This split requires additional downloads but problem occured before the download + SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY + ); + assertTrue(result.isPresent()); + result.get().executeCommand(); + // Actual check + connectionTypeCalculator = new WayConnectionTypeCalculator(); + links = connectionTypeCalculator.updateLinks(route, route.getMembers()); + assertAll("All links should be connected (forward)", + links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext))); + assertAll("All links should be connected (backward)", + links.subList(1, links.size() - 1).stream().map(link -> () -> assertTrue(link.linkPrev))); + } }