Changeset 15943 in josm for trunk/test

Timestamp:

2020-02-26T20:09:08+01:00 (5 years ago)

Author:

GerdP

Message:

see #18596 Fix relation ordering after split-way
Patch by JeroenHoek, slightly modified

• download needed relation members if wanted
• improve member ordering of route relations

TODO:

• download parents if not known
• fix also ordering of members in multipolygon relations

Location:

trunk/test

Files:

• data/regress/17400 (added)
• data/regress/17400/data.osm (added)
• data/regress/18596 (added)
• data/regress/18596/data.osm (added)
• unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (modified) (5 diffs)

trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java

@@ -3 +3 @@
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
+import java.io.IOException;
+import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Iterator;
+import java.util.Optional;
 
 import org.junit.Rule;
 import org.junit.Test;
+import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.command.SplitWayCommand.Strategy;
 import org.openstreetmap.josm.data.UndoRedoHandler;
@@ -18 +23 @@
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 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.io.IllegalDataException;
+import org.openstreetmap.josm.io.OsmReader;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
@@ -35 +43 @@
     @Rule
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().main().projection();
+    public JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
 
     /**
@@ -113 +121 @@
 
         final Strategy strategy = wayChunks -> {
-                final Iterator<Way> it = wayChunks.iterator();
-                for (int i = 0; i < indexOfWayToKeep; i++) {
-                    it.next();
-                }
-                return it.next();
-            };
+            final Iterator<Way> it = wayChunks.iterator();
+            for (int i = 0; i < indexOfWayToKeep; i++) {
+                it.next();
+            }
+            return it.next();
+        };
         final SplitWayCommand result = SplitWayCommand.splitWay(
-                w2, SplitWayCommand.buildSplitChunks(w2, Arrays.asList(n3, n4, n5)), new ArrayList<OsmPrimitive>(), strategy);
+                w2, SplitWayCommand.buildSplitChunks(w2, Arrays.asList(n3, n4, n5)), new ArrayList<>(), strategy);
         UndoRedoHandler.getInstance().add(result);
 
@@ -140 +148 @@
     }
 
+    @Test
+    public void testOneMemberOrderedRelationShowsWarningTest() {
+        final DataSet dataSet = new DataSet();
+
+        // Positive IDs to mark that these ways are incomplete (i.e., no nodes loaded).
+        final Way w1 = new Way(1);
+        final Way w3 = new Way(3);
+
+        // The way we are going to split is complete of course.
+        final Node n1 = new Node(new LatLon(1, 0));
+        final Node n2 = new Node(new LatLon(2, 0));
+        final Node n3 = new Node(new LatLon(3, 0));
+        final Way w2 = new Way();
+
+        final Relation route = new Relation();
+        for (OsmPrimitive p : Arrays.asList(n1, n2, n3, w1, w2, w3, route)) {
+            dataSet.addPrimitive(p);
+        }
+        w2.setNodes(Arrays.asList(n1, n2, n3));
+
+        route.put("type", "route");
+        route.addMember(new RelationMember("", w1));
+        route.addMember(new RelationMember("", w2));
+        route.addMember(new RelationMember("", w3));
+        dataSet.setSelected(Arrays.asList(w2, n2));
+
+        // This split cannot be safely performed without downloading extra relation members.
+        // Here we ask the split method to abort if it needs more information.
+        final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
+                w2,
+                SplitWayCommand.buildSplitChunks(w2, Collections.singletonList(n2)),
+                new ArrayList<>(),
+                Strategy.keepLongestChunk(),
+                SplitWayCommand.WhenRelationOrderUncertain.ABORT
+        );
+
+        assertFalse(result.isPresent());
+    }
+
+    @Test
+    public void testDoIncompleteMembersOrderedRelationCorrectOrderTest() {
+        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,
+                                                                    final boolean reverseWayTwo,
+                                                                    final int indexOfWayToKeep) {
+        final DataSet dataSet = new DataSet();
+
+        // Positive IDs to mark that these ways are incomplete (i.e., no nodes loaded).
+        final Way w1 = new Way(1);
+        final Way w4 = new Way(3);
+
+        // The ways we are going to split are complete of course.
+        final Node n1 = new Node(new LatLon(1, 0));
+        final Node n2 = new Node(new LatLon(2, 0));
+        final Node n3 = new Node(new LatLon(3, 0));
+        final Node n4 = new Node(new LatLon(4, 0));
+        final Node n5 = new Node(new LatLon(5, 0));
+        final Way w2 = new Way();
+        final Way w3 = new Way();
+
+        final Relation route = new Relation();
+        for (OsmPrimitive p : Arrays.asList(n1, n2, n3, n4, n5, w1, w2, w3, w4, route)) {
+            dataSet.addPrimitive(p);
+        }
+        w2.setNodes(reverseWayOne ? Arrays.asList(n3, n2, n1) : Arrays.asList(n1, n2, n3));
+        w3.setNodes(reverseWayTwo ? Arrays.asList(n5, n4, n3) : Arrays.asList(n3, n4, n5));
+
+        route.put("type", "route");
+        route.addMember(new RelationMember("", w1));
+        route.addMember(new RelationMember("", w2));
+        route.addMember(new RelationMember("", w3));
+        route.addMember(new RelationMember("", w4));
+
+        Way splitWay = indexOfWayToKeep == 0 ? w2 : w3;
+        Node splitNode = indexOfWayToKeep == 0 ? n2 : n4;
+
+        dataSet.setSelected(Arrays.asList(splitWay, splitNode));
+
+        final SplitWayCommand result = SplitWayCommand.splitWay(
+                splitWay, SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)), new ArrayList<>());
+        UndoRedoHandler.getInstance().add(result);
+
+        assertEquals(5, route.getMembersCount());
+        assertConnectedAtEnds(route.getMember(1).getWay(), route.getMember(2).getWay());
+        assertConnectedAtEnds(route.getMember(2).getWay(), route.getMember(3).getWay());
+    }
+
     static void assertFirstLastNodeIs(Way way, Node node) {
         assertTrue("First/last node of " + way + " should be " + node, node.equals(way.firstNode()) || node.equals(way.lastNode()));
     }
+
+    static void assertConnectedAtEnds(Way one, Way two) {
+        Node first1 = one.firstNode();
+        Node last1 = one.lastNode();
+        Node first2 = two.firstNode();
+        Node last2 = two.lastNode();
+
+        assertTrue("Ways expected to be connected at their ends.",
+                first1 == first2 || first1 == last2 || last1 == first2 || last1 == last2);
+    }
+
+    /**
+     * Non-regression test for patch #18596 (Fix relation ordering after split-way)
+     * @throws IOException if any I/O error occurs
+     * @throws IllegalDataException if OSM parsing fails
+     */
+    @Test
+    public void testTicket18596() throws IOException, IllegalDataException {
+        try (InputStream is = TestUtils.getRegressionDataStream(18596, "data.osm")) {
+            DataSet ds = OsmReader.parseDataSet(is, null);
+
+            Way splitWay = (Way) ds.getPrimitiveById(5, OsmPrimitiveType.WAY);
+            Node splitNode = (Node) ds.getPrimitiveById(100002, OsmPrimitiveType.NODE);
+
+            final SplitWayCommand result = SplitWayCommand.splitWay(
+                    splitWay,
+                    SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)),
+                    new ArrayList<>()
+            );
+
+            UndoRedoHandler.getInstance().add(result);
+
+            Relation relation = (Relation) ds.getPrimitiveById(8888, OsmPrimitiveType.RELATION);
+
+            assertEquals(relation.getMembersCount(), 8);
+
+            // Before the patch introduced in #18596, these asserts would fail. The two parts of
+            // way '5' would be in the wrong order, breaking the boundary relation in this test.
+            assertConnectedAtEnds(relation.getMember(4).getWay(), relation.getMember(5).getWay());
+            assertConnectedAtEnds(relation.getMember(5).getWay(), relation.getMember(6).getWay());
+        }
+    }
+
+    /**
+     * Non-regression test for issue #17400 ( Warn when splitting way in not fully downloaded region)
+     *
+     * Bus route 190 gets broken when the split occurs, because the two new way parts are inserted in the relation in
+     * the wrong order.
+     *
+     * @throws IOException if any I/O error occurs
+     * @throws IllegalDataException if OSM parsing fails
+     */
+    @Test
+    public void testTicket17400() throws IOException, IllegalDataException {
+        try (InputStream is = TestUtils.getRegressionDataStream(17400, "data.osm")) {
+            DataSet ds = OsmReader.parseDataSet(is, null);
+
+            Way splitWay = (Way) ds.getPrimitiveById(253731928, OsmPrimitiveType.WAY);
+            Node splitNode = (Node) ds.getPrimitiveById(29830834, OsmPrimitiveType.NODE);
+
+            final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
+                    splitWay,
+                    SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)),
+                    new ArrayList<>(),
+                    Strategy.keepLongestChunk(),
+                    // This split requires no additional downloads.
+                    SplitWayCommand.WhenRelationOrderUncertain.ABORT
+            );
+
+            assertTrue(result.isPresent());
+
+            UndoRedoHandler.getInstance().add(result.get());
+
+            // 190 Hormersdorf-Thalheim-Stollberg.
+            Relation relation = (Relation) ds.getPrimitiveById(2873422, OsmPrimitiveType.RELATION);
+
+            // One more than the original 161.
+            assertEquals(relation.getMembersCount(), 162);
+
+            // Before the patch introduced in #18596, these asserts would fail. The new parts of
+            // the Hauptstraße would be in the wrong order, breaking the bus route relation.
+            // These parts should be connected, in their relation sequence: 74---75---76.
+            // Before #18596 this would have been a broken connection: 74---75-x-76.
+            assertConnectedAtEnds(relation.getMember(74).getWay(), relation.getMember(75).getWay());
+            assertConnectedAtEnds(relation.getMember(75).getWay(), relation.getMember(76).getWay());
+        }
+    }
 }