Ticket #21856: 21856.patch

File 21856.patch, 12.2 KB (added by taylor.smock, 4 years ago)

  • 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 b  
    487487                        if (isOrderedRelation) {
    488488                            if (way.lastNode() == way.firstNode()) {
    489489                                // Self-closing way.
    490                                 direction = Direction.IRRELEVANT;
     490                                direction = direction.merge(Direction.IRRELEVANT);
    491491                            } else {
    492492                                // For ordered relations, looking beyond the nearest neighbour members is not required,
    493493                                // and can even cause the wrong direction to be guessed (with closed loops).
    … …  
    497497                                        missingWays.add(w);
    498498                                    else {
    499499                                        if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    500                                             direction = Direction.FORWARDS;
     500                                            direction = direction.merge(Direction.FORWARDS);
    501501                                        } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
    502                                             direction = Direction.BACKWARDS;
     502                                            direction = direction.merge(Direction.BACKWARDS);
    503503                                        }
    504504                                    }
    505505                                }
    … …  
    509509                                        missingWays.add(w);
    510510                                    else {
    511511                                        if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    512                                             direction = Direction.BACKWARDS;
     512                                            direction = direction.merge(Direction.BACKWARDS);
    513513                                        } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
    514                                             direction = Direction.FORWARDS;
     514                                            direction = direction.merge(Direction.FORWARDS);
    515515                                        }
    516516                                    }
    517517                                }
    … …  
    528528                                if (ir - k >= 0 && r.getMember(ir - k).isWay()) {
    529529                                    Way w = r.getMember(ir - k).getWay();
    530530                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    531                                         direction = Direction.FORWARDS;
     531                                        direction = direction.merge(Direction.FORWARDS);
    532532                                    } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
    533                                         direction = Direction.BACKWARDS;
     533                                        direction = direction.merge(Direction.BACKWARDS);
    534534                                    }
    535535                                    break;
    536536                                }
    537537                                if (ir + k < r.getMembersCount() && r.getMember(ir + k).isWay()) {
    538538                                    Way w = r.getMember(ir + k).getWay();
    539539                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    540                                         direction = Direction.BACKWARDS;
     540                                        direction = direction.merge(Direction.BACKWARDS);
    541541                                    } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
    542                                         direction = Direction.FORWARDS;
     542                                        direction = direction.merge(Direction.FORWARDS);
    543543                                    }
    544544                                    break;
    545545                                }
    … …  
    951951        FORWARDS,
    952952        BACKWARDS,
    953953        UNKNOWN,
    954         IRRELEVANT
     954        IRRELEVANT;
     955
     956        /**
     957         * Merge directions (this helps avoid overriding {@link #FORWARDS} with {@link #BACKWARDS}).
     958         * @param other The other direction to merge. {@link #UNKNOWN} will be overridden.
     959         * @return The merged direction
     960         */
     961        Direction merge(Direction other) {
     962            if (this == other) {
     963                return this;
     964            }
     965            if (this == IRRELEVANT || other == IRRELEVANT ||
     966                    (this == FORWARDS && other == BACKWARDS) ||
     967                    (other == FORWARDS && this == BACKWARDS)) {
     968                return IRRELEVANT;
     969            }
     970            if (this == UNKNOWN) {
     971                return other;
     972            }
     973            if (other == UNKNOWN) {
     974                return this;
     975            }
     976            return UNKNOWN;
     977        }
    955978    }
    956979
    957980    enum WarningType {

  • 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 b  
    530530     * @since 17981
    531531     */
    532532    public void addPrimitiveRecursive(OsmPrimitive primitive) {
     533        Stream<OsmPrimitive> children;
    533534        if (primitive instanceof Way) {
    534             ((Way) primitive).getNodes().forEach(n -> addPrimitiveRecursive(n));
     535            children = ((Way) primitive).getNodes().stream().map(OsmPrimitive.class::cast);
    535536        } else if (primitive instanceof Relation) {
    536             ((Relation) primitive).getMembers().forEach(m -> addPrimitiveRecursive(m.getMember()));
     537            children = ((Relation) primitive).getMembers().stream().map(RelationMember::getMember);
     538        } else {
     539            children = Stream.empty();
    537540        }
     541        // Relations may have the same member multiple times.
     542        children.filter(p -> !Objects.equals(this, p.getDataSet())).forEach(this::addPrimitiveRecursive);
    538543        addPrimitive(primitive);
    539544    }
    540545

  • 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 b  
    11// License: GPL. For details, see LICENSE file.
    22package org.openstreetmap.josm.command;
    33
     4import static org.junit.jupiter.api.Assertions.assertAll;
    45import static org.junit.jupiter.api.Assertions.assertEquals;
    56import static org.junit.jupiter.api.Assertions.assertFalse;
    67import static org.junit.jupiter.api.Assertions.assertTrue;
    … …  
    1112import java.util.Arrays;
    1213import java.util.Collections;
    1314import java.util.Iterator;
     15import java.util.List;
    1416import java.util.Optional;
    1517
    1618import org.junit.jupiter.api.Test;
    1719import org.junit.jupiter.api.extension.RegisterExtension;
     20import org.junit.jupiter.params.ParameterizedTest;
     21import org.junit.jupiter.params.provider.ValueSource;
    1822import org.openstreetmap.josm.TestUtils;
    1923import org.openstreetmap.josm.command.SplitWayCommand.Strategy;
    2024import org.openstreetmap.josm.data.UndoRedoHandler;
    … …  
    2630import org.openstreetmap.josm.data.osm.Relation;
    2731import org.openstreetmap.josm.data.osm.RelationMember;
    2832import org.openstreetmap.josm.data.osm.Way;
     33import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType;
     34import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator;
    2935import org.openstreetmap.josm.io.IllegalDataException;
    3036import org.openstreetmap.josm.io.OsmReader;
    3137import org.openstreetmap.josm.testutils.JOSMTestRules;
    3238
    3339import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
     40import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
    3441
    3542/**
    3643 * Unit tests for class {@link SplitWayCommand}.
    … …  
    4249     */
    4350    @RegisterExtension
    4451    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
    45     public JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
     52    static JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
    4653
    4754    /**
    4855     * Unit test of {@link SplitWayCommand#findVias}.
    … …  
    433440        }
    434441    }
    435442
     443    /**
     444     * Test case: smart ordering in routes
     445     * See #21856
     446     */
     447    @ParameterizedTest
     448    @ValueSource(booleans = {false, true})
     449    void testTicket21856(boolean reverse) {
     450        Way way1 = TestUtils.newWay("highway=residential", TestUtils.newNode(""), TestUtils.newNode(""));
     451        way1.setOsmId(23_968_090, 1);
     452        way1.lastNode().setOsmId(6_823_898_683L, 1);
     453        Way way2 = TestUtils.newWay("highway=residential", way1.lastNode(), TestUtils.newNode(""));
     454        way2.setOsmId(728_199_307, 1);
     455        way2.lastNode().setOsmId(6_823_898_684L, 1);
     456        Node splitNode = TestUtils.newNode("");
     457        splitNode.setOsmId(6_823_906_290L, 1);
     458        Way splitWay = TestUtils.newWay("highway=service", way2.firstNode(), splitNode, TestUtils.newNode(""), way2.lastNode());
     459        // The behavior should be the same regardless of the direction of the way
     460        if (reverse) {
     461            List<Node> nodes = new ArrayList<>(splitWay.getNodes());
     462            Collections.reverse(nodes);
     463            splitWay.setNodes(nodes);
     464        }
     465        splitWay.setOsmId(728_199_306, 1);
     466        Relation route = TestUtils.newRelation("type=route route=bus", new RelationMember("", way1), new RelationMember("", splitWay),
     467                new RelationMember("", way2), new RelationMember("", way1));
     468        DataSet dataSet = new DataSet();
     469        dataSet.addPrimitiveRecursive(route);
     470        dataSet.setSelected(splitNode);
     471        // Sanity check (preconditions -- the route should be well-formed already)
     472        WayConnectionTypeCalculator connectionTypeCalculator = new WayConnectionTypeCalculator();
     473        List<WayConnectionType> links = connectionTypeCalculator.updateLinks(route, route.getMembers());
     474        assertAll("All links should be connected (forward)", links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext)));
     475        assertAll("All links should be connected (backward)", links.subList(1, links.size() - 1).stream().map(link -> () -> assertTrue(link.linkPrev)));
     476        final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
     477                splitWay,
     478                SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)),
     479                new ArrayList<>(),
     480                Strategy.keepLongestChunk(),
     481                // This split requires additional downloads but problem occured before the download
     482                SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY
     483        );
     484        assertTrue(result.isPresent());
     485        result.get().executeCommand();
     486        // Actual check
     487        connectionTypeCalculator = new WayConnectionTypeCalculator();
     488        links = connectionTypeCalculator.updateLinks(route, route.getMembers());
     489        assertAll("All links should be connected (forward)", links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext)));
     490        assertAll("All links should be connected (backward)", links.subList(1, links.size() - 1).stream().map(link -> () -> assertTrue(link.linkPrev)));
     491    }
    436492}