Index: src/org/openstreetmap/josm/command/SplitWayCommand.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/command/SplitWayCommand.java b/src/org/openstreetmap/josm/command/SplitWayCommand.java --- a/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 18538) +++ b/src/org/openstreetmap/josm/command/SplitWayCommand.java (date 1660586532730) @@ -487,7 +487,7 @@ if (isOrderedRelation) { 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, // and can even cause the wrong direction to be guessed (with closed loops). @@ -497,9 +497,9 @@ missingWays.add(w); 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); } } } @@ -509,9 +509,9 @@ missingWays.add(w); 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); } } } @@ -528,18 +528,18 @@ if (ir - k >= 0 && r.getMember(ir - k).isWay()) { 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; } if (ir + k < r.getMembersCount() && r.getMember(ir + k).isWay()) { 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; } @@ -951,7 +951,30 @@ FORWARDS, 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; + } } enum WarningType { Index: src/org/openstreetmap/josm/data/osm/DataSet.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/data/osm/DataSet.java b/src/org/openstreetmap/josm/data/osm/DataSet.java --- a/src/org/openstreetmap/josm/data/osm/DataSet.java (revision 18538) +++ b/src/org/openstreetmap/josm/data/osm/DataSet.java (date 1660579475456) @@ -530,11 +530,16 @@ * @since 17981 */ 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: test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java b/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java --- a/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 18538) +++ b/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (date 1660586468864) @@ -1,6 +1,7 @@ // License: GPL. For details, see LICENSE file. 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; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -11,10 +12,13 @@ import java.util.Arrays; import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Optional; 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.ValueSource; import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.command.SplitWayCommand.Strategy; import org.openstreetmap.josm.data.UndoRedoHandler; @@ -26,11 +30,14 @@ import org.openstreetmap.josm.data.osm.Relation; 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; import org.openstreetmap.josm.testutils.JOSMTestRules; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import org.openstreetmap.josm.testutils.annotations.BasicPreferences; /** * Unit tests for class {@link SplitWayCommand}. @@ -42,7 +49,7 @@ */ @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(); /** * Unit test of {@link SplitWayCommand#findVias}. @@ -433,4 +440,53 @@ } } + /** + * 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))); + } }