Changeset 15943 in josm for trunk


Ignore:
Timestamp:
2020-02-26T20:09:08+01:00 (4 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
Files:
4 added
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/actions/SplitWayAction.java

    r15728 r15943  
    1515import java.util.Iterator;
    1616import java.util.List;
     17import java.util.Optional;
    1718import java.util.concurrent.atomic.AtomicInteger;
    1819import java.util.stream.Collectors;
    … …  
    220221                list.setSelectedValue(wayToKeep, true);
    221222            } else {
    222                 setHighlightedWaySegments(Collections.<WaySegment>emptyList());
     223                setHighlightedWaySegments(Collections.emptyList());
    223224                DISPLAY_COUNT.decrementAndGet();
    224225            }
    … …  
    287288        final MapFrame map = MainApplication.getMap();
    288289        final boolean isMapModeDraw = map != null && map.mapMode == map.mapModeDraw;
    289         final SplitWayCommand result = SplitWayCommand.doSplitWay(way, wayToKeep, newWays, !isMapModeDraw ? newSelection : null);
    290         UndoRedoHandler.getInstance().add(result);
    291         List<? extends PrimitiveId> newSel = result.getNewSelection();
    292         if (newSel != null && !newSel.isEmpty()) {
    293             way.getDataSet().setSelected(newSel);
    294         }
     290
     291        Optional<SplitWayCommand> splitWayCommand = SplitWayCommand.doSplitWay(
     292                way,
     293                wayToKeep,
     294                newWays,
     295                !isMapModeDraw ? newSelection : null,
     296                SplitWayCommand.WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD
     297        );
     298
     299        splitWayCommand.ifPresent(result -> {
     300            UndoRedoHandler.getInstance().add(result);
     301            List<? extends PrimitiveId> newSel = result.getNewSelection();
     302            if (newSel != null && !newSel.isEmpty()) {
     303                way.getDataSet().setSelected(newSel);
     304            }
     305        });
    295306    }
    296307

  • trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java

    r15418 r15943  
    22package org.openstreetmap.josm.command;
    33
     4import static org.openstreetmap.josm.command.SplitWayCommand.MissingMemberStrategy.GO_AHEAD_WITHOUT_DOWNLOADS;
     5import static org.openstreetmap.josm.command.SplitWayCommand.MissingMemberStrategy.GO_AHEAD_WITH_DOWNLOADS;
     6import static org.openstreetmap.josm.command.SplitWayCommand.MissingMemberStrategy.USER_ABORTED;
     7import static org.openstreetmap.josm.command.SplitWayCommand.WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD;
    48import static org.openstreetmap.josm.tools.I18n.tr;
    59import static org.openstreetmap.josm.tools.I18n.trn;
    … …  
    913import java.util.Collection;
    1014import java.util.Collections;
     15import java.util.EnumSet;
    1116import java.util.HashMap;
    1217import java.util.HashSet;
    … …  
    2025import java.util.function.Consumer;
    2126
     27import javax.swing.JOptionPane;
     28
     29import org.openstreetmap.josm.data.osm.DataSet;
    2230import org.openstreetmap.josm.data.osm.DefaultNameFormatter;
    2331import org.openstreetmap.josm.data.osm.Node;
    … …  
    2735import org.openstreetmap.josm.data.osm.RelationMember;
    2836import org.openstreetmap.josm.data.osm.Way;
     37import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil;
     38import org.openstreetmap.josm.gui.ExceptionDialogUtil;
     39import org.openstreetmap.josm.gui.MainApplication;
     40import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
     41import org.openstreetmap.josm.gui.widgets.JMultilineLabel;
     42import org.openstreetmap.josm.io.MultiFetchServerObjectReader;
     43import org.openstreetmap.josm.io.OsmTransferException;
    2944import org.openstreetmap.josm.spi.preferences.Config;
    3045import org.openstreetmap.josm.tools.CheckParameterUtil;
    3146import org.openstreetmap.josm.tools.Logging;
    32 
    3347/**
    3448 * Splits a way into multiple ways (all identical except for their node list).
    … …  
    4256
    4357    private static volatile Consumer<String> warningNotifier = Logging::warn;
     58    private static final String DOWNLOAD_MISSING_PREF_KEY = "split_way_download_missing_members";
    4459
    4560    private static final class RelationInformation {
    … …  
    6075    private final Way originalWay;
    6176    private final List<Way> newWays;
     77
    6278    /** Map&lt;Restriction type, type to treat it as&gt; */
    6379    private static final Map<String, String> relationSpecialTypes = new HashMap<>();
    … …  
    247263
    248264    /**
    249      * Splits the way {@code way} into chunks of {@code wayChunks} and replies
    250      * the result of this process in an instance of {@link SplitWayCommand}.
    251      * The {@link SplitWayCommand.Strategy} is used to determine which
    252      * way chunk should reuse the old id and its history.
    253      *
    254      * Note that changes are not applied to the data yet. You have to
    255      * submit the command first, i.e. {@code UndoRedoHandler.getInstance().add(result)}.
    256      *
    257      * @param way the way to split. Must not be null.
    258      * @param wayChunks the list of way chunks into the way is split. Must not be null.
    259      * @param selection The list of currently selected primitives
     265     * Splits the way {@code way} into chunks of {@code wayChunks} and replies the result of this process in an instance
     266     * of {@link SplitWayCommand}. The {@link SplitWayCommand.Strategy} is used to determine which way chunk should
     267     * reuse the old id and its history.
     268     * <p>
     269     * If the split way is part of relations, and the order of the new parts in these relations cannot be determined due
     270     * to missing relation members, the user will be asked to consent to downloading these missing members.
     271     * <p>
     272     * Note that changes are not applied to the data yet. You have to submit the command first, i.e. {@code
     273     * UndoRedoHandler.getInstance().add(result)}.
     274     *
     275     * @param way           the way to split. Must not be null.
     276     * @param wayChunks     the list of way chunks into the way is split. Must not be null.
     277     * @param selection     The list of currently selected primitives
    260278     * @param splitStrategy The strategy used to determine which way chunk should reuse the old id and its history
    261279     * @return the result from the split operation
    262280     */
    263     public static SplitWayCommand splitWay(Way way, List<List<Node>> wayChunks,
    264             Collection<? extends OsmPrimitive> selection, Strategy splitStrategy) {
     281    public static SplitWayCommand splitWay(Way way,
     282                                           List<List<Node>> wayChunks,
     283                                           Collection<? extends OsmPrimitive> selection,
     284                                           Strategy splitStrategy) {
     285
     286        // This method could be refactored to use an Optional in the future, but would need to be deprecated first
     287        // to phase out use by plugins.
     288        return splitWay(way, wayChunks, selection, splitStrategy, ASK_USER_FOR_CONSENT_TO_DOWNLOAD).orElse(null);
     289    }
     290
     291    /**
     292     * Splits the way {@code way} into chunks of {@code wayChunks} and replies the result of this process in an instance
     293     * of {@link SplitWayCommand}. The {@link SplitWayCommand.Strategy} is used to determine which way chunk should
     294     * reuse the old id and its history.
     295     * <p>
     296     * Note that changes are not applied to the data yet. You have to submit the command first, i.e. {@code
     297     * UndoRedoHandler.getInstance().add(result)}.
     298     *
     299     * @param way                        the way to split. Must not be null.
     300     * @param wayChunks                  the list of way chunks into the way is split. Must not be null.
     301     * @param selection                  The list of currently selected primitives
     302     * @param splitStrategy              The strategy used to determine which way chunk should reuse the old id and its
     303     *                                   history
     304     * @param whenRelationOrderUncertain What to do when the split way is part of relations, and the order of the new
     305     *                                   parts in the relation cannot be determined without downloading missing relation
     306     *                                   members.
     307     * @return The result from the split operation, may be an empty {@link Optional} if the operation is aborted.
     308     */
     309    public static Optional<SplitWayCommand> splitWay(Way way,
     310                                                     List<List<Node>> wayChunks,
     311                                                     Collection<? extends OsmPrimitive> selection,
     312                                                     Strategy splitStrategy,
     313                                                     WhenRelationOrderUncertain whenRelationOrderUncertain) {
    265314        // build a list of commands, and also a new selection list
    266315        final List<OsmPrimitive> newSelection = new ArrayList<>(selection.size() + wayChunks.size());
    … …  
    273322        final Way wayToKeep = splitStrategy.determineWayToKeep(newWays);
    274323
    275         return wayToKeep != null ? doSplitWay(way, wayToKeep, newWays, newSelection) : null;
     324        return wayToKeep != null
     325                ? doSplitWay(way, wayToKeep, newWays, newSelection, whenRelationOrderUncertain)
     326                : Optional.empty();
    276327    }
    277328
    … …  
    284335     * @param newWays potential new ways
    285336     * @param newSelection new selection list to update (optional: can be null)
     337     * @param whenRelationOrderUncertain Action to perform when the order of the new parts in relations the way is
     338     *                                   member of could not be reliably determined. See
     339     *                                   {@link WhenRelationOrderUncertain}.
    286340     * @return the {@code SplitWayCommand}
    287341     */
    288     public static SplitWayCommand doSplitWay(Way way, Way wayToKeep, List<Way> newWays, List<OsmPrimitive> newSelection) {
    289 
    290         Collection<Command> commandList = new ArrayList<>(newWays.size());
     342    public static Optional<SplitWayCommand> doSplitWay(Way way,
     343                                                       Way wayToKeep,
     344                                                       List<Way> newWays,
     345                                                       List<OsmPrimitive> newSelection,
     346                                                       WhenRelationOrderUncertain whenRelationOrderUncertain) {
     347        if (whenRelationOrderUncertain == null) whenRelationOrderUncertain = ASK_USER_FOR_CONSENT_TO_DOWNLOAD;
     348
     349        final int indexOfWayToKeep = newWays.indexOf(wayToKeep);
     350        newWays.remove(wayToKeep);
     351
     352        // Figure out the order of relation members (if any).
     353        Analysis analysis = analyseSplit(way, wayToKeep, newWays, indexOfWayToKeep);
     354
     355        // If there are relations that cannot be split properly without downloading more members,
     356        // present the user with an option to do so, or to abort the split.
     357        List<Relation> relationsNeedingMoreMembers = new ArrayList<>();
     358        Set<OsmPrimitive> incompleteMembers = new HashSet<>();
     359        for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) {
     360            if (!relationAnalysis.needsMoreMembers()) continue;
     361
     362            Relation relation = relationAnalysis.getRelation();
     363            int position = relationAnalysis.getPosition();
     364            int membersCount = relation.getMembersCount();
     365
     366            // Mark the neighbouring members for downloading if these are ways too.
     367            relationsNeedingMoreMembers.add(relation);
     368            RelationMember rmPrev = position == 0
     369                    ? relation.getMember(membersCount - 1)
     370                    : relation.getMember(position - 1);
     371            RelationMember rmNext = position == membersCount - 1
     372                    ? relation.getMember(0)
     373                    : relation.getMember(position + 1);
     374
     375            if (rmPrev != null && rmPrev.isWay()) {
     376                incompleteMembers.add(rmPrev.getWay());
     377            }
     378            if (rmNext != null && rmNext.isWay()) {
     379                incompleteMembers.add(rmNext.getWay());
     380            }
     381        }
     382
     383        MissingMemberStrategy missingMemberStrategy;
     384        if (relationsNeedingMoreMembers.isEmpty()) {
     385            // The split can be performed without any extra downloads.
     386            missingMemberStrategy = GO_AHEAD_WITHOUT_DOWNLOADS;
     387        } else {
     388            switch (whenRelationOrderUncertain) {
     389                case ASK_USER_FOR_CONSENT_TO_DOWNLOAD:
     390                    // If the analysis shows that for some relations missing members should be downloaded, offer the user the
     391                    // chance to consent to this.
     392
     393                    // Only ask the user about downloading missing members when they haven't consented to this before.
     394                    if (ConditionalOptionPaneUtil.getDialogReturnValue(DOWNLOAD_MISSING_PREF_KEY) == Integer.MAX_VALUE) {
     395                        // User has previously told us downloading missing relation members is fine.
     396                        missingMemberStrategy = GO_AHEAD_WITH_DOWNLOADS;
     397                    } else {
     398                        // Ask the user.
     399                        missingMemberStrategy = offerToDownloadMissingMembersIfNeeded(analysis, relationsNeedingMoreMembers);
     400                    }
     401                    break;
     402                case SPLIT_ANYWAY:
     403                    missingMemberStrategy = GO_AHEAD_WITHOUT_DOWNLOADS;
     404                    break;
     405                case DOWNLOAD_MISSING_MEMBERS:
     406                    missingMemberStrategy = GO_AHEAD_WITH_DOWNLOADS;
     407                    break;
     408                case ABORT:
     409                default:
     410                    missingMemberStrategy = USER_ABORTED;
     411                    break;
     412            }
     413        }
     414
     415        switch (missingMemberStrategy) {
     416            case GO_AHEAD_WITH_DOWNLOADS:
     417                try {
     418                    downloadMissingMembers(incompleteMembers);
     419                } catch (OsmTransferException e) {
     420                    ExceptionDialogUtil.explainException(e);
     421                    return Optional.empty();
     422                }
     423                // If missing relation members were downloaded, perform the analysis again to find the relation
     424                // member order for all relations.
     425                analysis = analyseSplit(way, wayToKeep, newWays, indexOfWayToKeep);
     426                return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
     427            case GO_AHEAD_WITHOUT_DOWNLOADS:
     428                // Proceed with the split with the information we have.
     429                // This can mean that there are no missing members we want, or that the user chooses to continue
     430                // the split without downloading them.
     431                return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
     432            case USER_ABORTED:
     433            default:
     434                return Optional.empty();
     435        }
     436    }
     437
     438    static Analysis analyseSplit(Way way,
     439                                 Way wayToKeep,
     440                                 List<Way> newWays,
     441                                 int indexOfWayToKeep) {
     442        Collection<Command> commandList = new ArrayList<>();
    291443        Collection<String> nowarnroles = Config.getPref().getList("way.split.roles.nowarn",
    292444                Arrays.asList("outer", "inner", "forward", "backward", "north", "south", "east", "west"));
    … …  
    296448        changedWay.setNodes(wayToKeep.getNodes());
    297449        commandList.add(new ChangeCommand(way, changedWay));
    298         if (/*!isMapModeDraw &&*/ newSelection != null && !newSelection.contains(way)) {
    299             newSelection.add(way);
    300         }
    301         final int indexOfWayToKeep = newWays.indexOf(wayToKeep);
    302         newWays.remove(wayToKeep);
    303 
    304         if (/*!isMapModeDraw &&*/ newSelection != null) {
    305             newSelection.addAll(newWays);
    306         }
     450
    307451        for (Way wayToAdd : newWays) {
    308452            commandList.add(new AddCommand(way.getDataSet(), wayToAdd));
    309453        }
    310454
    311         boolean warnmerole = false;
    312         boolean warnme = false;
    313         // now copy all relations to new way also
     455        List<RelationAnalysis> relationAnalyses = new ArrayList<>();
     456        EnumSet<WarningType> warnings = EnumSet.noneOf(WarningType.class);
     457        int numberOfRelations = 0;
    314458
    315459        for (Relation r : OsmPrimitive.getParentRelations(Collections.singleton(way))) {
    … …  
    317461                continue;
    318462            }
     463
     464            numberOfRelations++;
     465
    319466            Relation c = null;
    320467            String type = Optional.ofNullable(r.get("type")).orElse("");
     468            // Known types of ordered relations.
     469            boolean isOrderedRelation = "route".equals(type) || "multipolygon".equals(type) || "boundary".equals(type);
    321470
    322471            int ic = 0;
    323472            int ir = 0;
    324473            List<RelationMember> relationMembers = r.getMembers();
    325             for (RelationMember rm: relationMembers) {
     474            for (RelationMember rm : relationMembers) {
    326475                if (rm.isWay() && rm.getMember() == way) {
    327476                    boolean insert = true;
    328477                    if (relationSpecialTypes.containsKey(type) && "restriction".equals(relationSpecialTypes.get(type))) {
    329478                        RelationInformation rValue = treatAsRestriction(r, rm, c, newWays, way, changedWay);
    330                         warnme = rValue.warnme;
     479                        if (rValue.warnme) warnings.add(WarningType.GENERIC);
    331480                        insert = rValue.insert;
    332481                        c = rValue.relation;
    333                     } else if (!("route".equals(type)) && !("multipolygon".equals(type))) {
    334                         warnme = true;
     482                    } else if (!isOrderedRelation) {
     483                        // Warn the user when relations that are not a route or multipolygon are modified as a result
     484                        // of splitting up the way, because we can't tell if this might break anything.
     485                        warnings.add(WarningType.GENERIC);
    335486                    }
    336487                    if (c == null) {
    … …  
    340491                    if (insert) {
    341492                        if (rm.hasRole() && !nowarnroles.contains(rm.getRole())) {
    342                             warnmerole = true;
     493                            warnings.add(WarningType.ROLE);
    343494                        }
    344495
    345                         Boolean backwards = null;
    346                         int k = 1;
    347                         while (ir - k >= 0 || ir + k < relationMembers.size()) {
    348                             if ((ir - k >= 0) && relationMembers.get(ir - k).isWay()) {
    349                                 Way w = relationMembers.get(ir - k).getWay();
    350                                 if ((w.lastNode() == way.firstNode()) || w.firstNode() == way.firstNode()) {
    351                                     backwards = Boolean.FALSE;
    352                                 } else if ((w.firstNode() == way.lastNode()) || w.lastNode() == way.lastNode()) {
    353                                     backwards = Boolean.TRUE;
     496                        // Attempt to determine the direction the ways in the relation are ordered.
     497                        Direction direction = Direction.UNKNOWN;
     498                        if (isOrderedRelation) {
     499                            if (way.lastNode() == way.firstNode()) {
     500                                // Self-closing way.
     501                                direction = Direction.IRRELEVANT;
     502                            } else {
     503                                // For ordered relations, looking beyond the nearest neighbour members is not required,
     504                                // and can even cause the wrong direction to be guessed (with closed loops).
     505                                if (ir - 1 >= 0 && relationMembers.get(ir - 1).isWay()) {
     506                                    Way w = relationMembers.get(ir - 1).getWay();
     507                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
     508                                        direction = Direction.FORWARDS;
     509                                    } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
     510                                        direction = Direction.BACKWARDS;
     511                                    }
    354512                                }
    355                                 break;
     513                                if (ir + 1 < relationMembers.size() && relationMembers.get(ir + 1).isWay()) {
     514                                    Way w = relationMembers.get(ir + 1).getWay();
     515                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
     516                                        direction = Direction.BACKWARDS;
     517                                    } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
     518                                        direction = Direction.FORWARDS;
     519                                    }
     520                                }
    356521                            }
    357                             if ((ir + k < relationMembers.size()) && relationMembers.get(ir + k).isWay()) {
    358                                 Way w = relationMembers.get(ir + k).getWay();
    359                                 if ((w.lastNode() == way.firstNode()) || w.firstNode() == way.firstNode()) {
    360                                     backwards = Boolean.TRUE;
    361                                 } else if ((w.firstNode() == way.lastNode()) || w.lastNode() == way.lastNode()) {
    362                                     backwards = Boolean.FALSE;
     522                        } else {
     523                            int k = 1;
     524                            while (ir - k >= 0 || ir + k < relationMembers.size()) {
     525                                if (ir - k >= 0 && relationMembers.get(ir - k).isWay()) {
     526                                    Way w = relationMembers.get(ir - k).getWay();
     527                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
     528                                        direction = Direction.FORWARDS;
     529                                    } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
     530                                        direction = Direction.BACKWARDS;
     531                                    }
     532                                    break;
    363533                                }
    364                                 break;
    365                             }
    366                             k++;
    367                         }
    368 
    369                         int j = ic;
    370                         final List<Way> waysToAddBefore = newWays.subList(0, indexOfWayToKeep);
    371                         for (Way wayToAdd : waysToAddBefore) {
    372                             RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
    373                             j++;
    374                             if (Boolean.TRUE.equals(backwards)) {
    375                                 c.addMember(ic + 1, em);
    376                             } else {
    377                                 c.addMember(j - 1, em);
     534                                if (ir + k < relationMembers.size() && relationMembers.get(ir + k).isWay()) {
     535                                    Way w = relationMembers.get(ir + k).getWay();
     536                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
     537                                        direction = Direction.BACKWARDS;
     538                                    } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
     539                                        direction = Direction.FORWARDS;
     540                                    }
     541                                    break;
     542                                }
     543                                k++;
    378544                            }
    379545                        }
    380                         final List<Way> waysToAddAfter = newWays.subList(indexOfWayToKeep, newWays.size());
    381                         for (Way wayToAdd : waysToAddAfter) {
    382                             RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
    383                             j++;
    384                             if (Boolean.TRUE.equals(backwards)) {
    385                                 c.addMember(ic, em);
    386                             } else {
    387                                 c.addMember(j, em);
    388                             }
    389                         }
    390                         ic = j;
     546
     547                        // We don't have enough information to determine the order of the new ways in this relation.
     548                        // This may cause relations to be saved with the two new way sections in reverse order.
     549                        //
     550                        // This often breaks routes.
     551                        //
     552                        // The user should be asked to download more members, or to abort the split operation.
     553                        boolean needsMoreMembers = isOrderedRelation
     554                                && direction == Direction.UNKNOWN
     555                                && relationMembers.size() > 1
     556                                && r.hasIncompleteMembers();
     557
     558                        relationAnalyses.add(new RelationAnalysis(c, rm, direction, needsMoreMembers, ic));
     559                        ic += indexOfWayToKeep;
    391560                    }
    392561                }
    … …  
    399568            }
    400569        }
    401         if (warnmerole) {
     570
     571        return new Analysis(relationAnalyses, commandList, warnings, numberOfRelations);
     572    }
     573
     574    static class Analysis {
     575        List<RelationAnalysis> relationAnalyses;
     576        Collection<Command> commands;
     577        EnumSet<WarningType> warningTypes;
     578        private final int numberOfRelations;
     579
     580        Analysis(List<RelationAnalysis> relationAnalyses,
     581                 Collection<Command> commandList,
     582                 EnumSet<WarningType> warnings,
     583                 int numberOfRelations) {
     584            this.relationAnalyses = relationAnalyses;
     585            commands = commandList;
     586            warningTypes = warnings;
     587            this.numberOfRelations = numberOfRelations;
     588        }
     589
     590        List<RelationAnalysis> getRelationAnalyses() {
     591            return relationAnalyses;
     592        }
     593
     594        Collection<Command> getCommands() {
     595            return commands;
     596        }
     597
     598        EnumSet<WarningType> getWarningTypes() {
     599            return warningTypes;
     600        }
     601
     602        public int getNumberOfRelations() {
     603            return numberOfRelations;
     604        }
     605    }
     606
     607    static MissingMemberStrategy offerToDownloadMissingMembersIfNeeded(Analysis analysis,
     608                                                                       List<Relation> relationsNeedingMoreMembers) {
     609        String[] options = {
     610                tr("Yes, download the missing members"),
     611                tr("No, abort the split operation"),
     612                tr("No, perform the split without downloading")
     613        };
     614
     615        String msgMemberOfRelations = trn(
     616                "This way is part of a relation.",
     617                "This way is part of {0} relations.",
     618                analysis.getNumberOfRelations(),
     619                analysis.getNumberOfRelations()
     620        );
     621
     622        String msgReferToRelations;
     623        if (analysis.getNumberOfRelations() == 1) {
     624            msgReferToRelations = tr("this relation");
     625        } else if (analysis.getNumberOfRelations() == relationsNeedingMoreMembers.size()) {
     626            msgReferToRelations = tr("these relations");
     627        } else {
     628            msgReferToRelations = trn(
     629                    "one relation",
     630                    "{0} relations",
     631                    relationsNeedingMoreMembers.size(),
     632                    relationsNeedingMoreMembers.size()
     633            );
     634        }
     635
     636        String msgRelationsMissingData = tr(
     637                "For {0} the correct order of the new way parts could not be determined. " +
     638                        "To fix this, some missing relation members should be downloaded first.",
     639                msgReferToRelations
     640        );
     641
     642        JMultilineLabel msg = new JMultilineLabel(msgMemberOfRelations + " " + msgRelationsMissingData);
     643        msg.setMaxWidth(600);
     644
     645        int ret = JOptionPane.showOptionDialog(
     646                MainApplication.getMainFrame(),
     647                msg,
     648                tr("Download missing relation members?"),
     649                JOptionPane.OK_CANCEL_OPTION,
     650                JOptionPane.QUESTION_MESSAGE,
     651                null,
     652                options,
     653                options[0]
     654        );
     655
     656        switch (ret) {
     657            case JOptionPane.OK_OPTION:
     658                // Ask the user if they want to do this automatically from now on. We only ask this for the download
     659                // action, because automatically cancelling is confusing (the user can't tell why this happened), and
     660                // automatically performing the split without downloading missing members despite needing them is
     661                // likely to break a lot of routes. The user also can't tell the difference between a split that needs
     662                // no downloads at all, and this special case where downloading missing relation members will prevent
     663                // broken relations.
     664                ConditionalOptionPaneUtil.showMessageDialog(
     665                        DOWNLOAD_MISSING_PREF_KEY,
     666                        MainApplication.getMainFrame(),
     667                        tr("Missing relation members will be downloaded. Should this be done automatically from now on?"),
     668                        tr("Downloading missing relation members"),
     669                        JOptionPane.INFORMATION_MESSAGE
     670                );
     671                return GO_AHEAD_WITH_DOWNLOADS;
     672            case JOptionPane.CANCEL_OPTION:
     673                return GO_AHEAD_WITHOUT_DOWNLOADS;
     674            default:
     675                return USER_ABORTED;
     676        }
     677    }
     678
     679    static void downloadMissingMembers(Set<OsmPrimitive> incompleteMembers) throws OsmTransferException {
     680        // Download the missing members.
     681        MultiFetchServerObjectReader reader = MultiFetchServerObjectReader.create();
     682        reader.append(incompleteMembers);
     683
     684        DataSet ds = reader.parseOsm(NullProgressMonitor.INSTANCE);
     685        MainApplication.getLayerManager().getEditLayer().mergeFrom(ds);
     686    }
     687
     688    static SplitWayCommand splitBasedOnAnalyses(Way way,
     689                                                List<Way> newWays,
     690                                                List<OsmPrimitive> newSelection,
     691                                                Analysis analysis,
     692                                                int indexOfWayToKeep) {
     693        if (newSelection != null && !newSelection.contains(way)) {
     694            newSelection.add(way);
     695        }
     696
     697        if (newSelection != null) {
     698            newSelection.addAll(newWays);
     699        }
     700
     701        // Perform the split.
     702        for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) {
     703            RelationMember rm = relationAnalysis.getRelationMember();
     704            Relation relation = relationAnalysis.getRelation();
     705            Direction direction = relationAnalysis.getDirection();
     706            int position = relationAnalysis.getPosition();
     707
     708            int j = position;
     709            final List<Way> waysToAddBefore = newWays.subList(0, indexOfWayToKeep);
     710            for (Way wayToAdd : waysToAddBefore) {
     711                RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
     712                j++;
     713                if (direction == Direction.BACKWARDS) {
     714                    relation.addMember(position + 1, em);
     715                } else {
     716                    relation.addMember(j - 1, em);
     717                }
     718            }
     719            final List<Way> waysToAddAfter = newWays.subList(indexOfWayToKeep, newWays.size());
     720            for (Way wayToAdd : waysToAddAfter) {
     721                RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
     722                j++;
     723                if (direction == Direction.BACKWARDS) {
     724                    relation.addMember(position, em);
     725                } else {
     726                    relation.addMember(j, em);
     727                }
     728            }
     729        }
     730
     731        EnumSet<WarningType> warnings = analysis.getWarningTypes();
     732
     733        if (warnings.contains(WarningType.ROLE)) {
    402734            warningNotifier.accept(
    403735                    tr("A role based relation membership was copied to all new ways.<br>You should verify this and correct it when necessary."));
    404         } else if (warnme) {
     736        } else if (warnings.contains(WarningType.GENERIC)) {
    405737            warningNotifier.accept(
    406738                    tr("A relation membership was copied to all new ways.<br>You should verify this and correct it when necessary."));
    … …  
    411743                    trn("Split way {0} into {1} part", "Split way {0} into {1} parts", newWays.size() + 1,
    412744                            way.getDisplayName(DefaultNameFormatter.getInstance()), newWays.size() + 1),
    413                     commandList,
     745                    analysis.getCommands(),
    414746                    newSelection,
    415747                    way,
    … …  
    524856        return relationSpecialTypes;
    525857    }
     858
     859    /**
     860     * What to do when the split way is part of relations, and the order of the new parts in the relation cannot be
     861     * determined without downloading missing relation members.
     862     */
     863    public enum WhenRelationOrderUncertain {
     864        /**
     865         * Ask the user to consent to downloading the missing members. The user can abort the operation or choose to
     866         * proceed without downloading anything.
     867         */
     868        ASK_USER_FOR_CONSENT_TO_DOWNLOAD,
     869        /**
     870         * If there are relation members missing, and these are needed to determine the order of the new parts in
     871         * that relation, abort the split operation.
     872         */
     873        ABORT,
     874        /**
     875         * If there are relation members missing, and these are needed to determine the order of the new parts in
     876         * that relation, continue with the split operation anyway, without downloading anything. Caution: use this
     877         * option with care.
     878         */
     879        SPLIT_ANYWAY,
     880        /**
     881         * If there are relation members missing, and these are needed to determine the order of the new parts in
     882         * that relation, automatically download these without prompting the user.
     883         */
     884        DOWNLOAD_MISSING_MEMBERS
     885    }
     886
     887    static class RelationAnalysis {
     888        private final Relation relation;
     889        private final RelationMember relationMember;
     890        private final Direction direction;
     891        private final boolean needsMoreMembers;
     892        private final int position;
     893
     894        RelationAnalysis(Relation relation,
     895                                RelationMember relationMember,
     896                                Direction direction,
     897                                boolean needsMoreMembers,
     898                                int position) {
     899            this.relation = relation;
     900            this.relationMember = relationMember;
     901            this.direction = direction;
     902            this.needsMoreMembers = needsMoreMembers;
     903            this.position = position;
     904        }
     905
     906        RelationMember getRelationMember() {
     907            return relationMember;
     908        }
     909
     910        Direction getDirection() {
     911            return direction;
     912        }
     913
     914        boolean needsMoreMembers() {
     915            return needsMoreMembers;
     916        }
     917
     918        Relation getRelation() {
     919            return relation;
     920        }
     921
     922        int getPosition() {
     923            return position;
     924        }
     925    }
     926
     927    enum Direction {
     928        FORWARDS,
     929        BACKWARDS,
     930        UNKNOWN,
     931        IRRELEVANT
     932    }
     933
     934    enum WarningType {
     935        GENERIC,
     936        ROLE
     937    }
     938
     939    enum MissingMemberStrategy {
     940        GO_AHEAD_WITH_DOWNLOADS,
     941        GO_AHEAD_WITHOUT_DOWNLOADS,
     942        USER_ABORTED
     943    }
    526944}

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

    r15362 r15943  
    33
    44import static org.junit.Assert.assertEquals;
     5import static org.junit.Assert.assertFalse;
    56import static org.junit.Assert.assertTrue;
    67
     8import java.io.IOException;
     9import java.io.InputStream;
    710import java.util.ArrayList;
    811import java.util.Arrays;
    912import java.util.Collections;
    1013import java.util.Iterator;
     14import java.util.Optional;
    1115
    1216import org.junit.Rule;
    1317import org.junit.Test;
     18import org.openstreetmap.josm.TestUtils;
    1419import org.openstreetmap.josm.command.SplitWayCommand.Strategy;
    1520import org.openstreetmap.josm.data.UndoRedoHandler;
    … …  
    1823import org.openstreetmap.josm.data.osm.Node;
    1924import org.openstreetmap.josm.data.osm.OsmPrimitive;
     25import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
    2026import org.openstreetmap.josm.data.osm.Relation;
    2127import org.openstreetmap.josm.data.osm.RelationMember;
    2228import org.openstreetmap.josm.data.osm.Way;
     29import org.openstreetmap.josm.io.IllegalDataException;
     30import org.openstreetmap.josm.io.OsmReader;
    2331import org.openstreetmap.josm.testutils.JOSMTestRules;
    2432
    … …  
    3543    @Rule
    3644    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
    37     public JOSMTestRules test = new JOSMTestRules().main().projection();
     45    public JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
    3846
    3947    /**
    … …  
    113121
    114122        final Strategy strategy = wayChunks -> {
    115                 final Iterator<Way> it = wayChunks.iterator();
    116                 for (int i = 0; i < indexOfWayToKeep; i++) {
    117                     it.next();
    118                 }
    119                 return it.next();
    120             };
     123            final Iterator<Way> it = wayChunks.iterator();
     124            for (int i = 0; i < indexOfWayToKeep; i++) {
     125                it.next();
     126            }
     127            return it.next();
     128        };
    121129        final SplitWayCommand result = SplitWayCommand.splitWay(
    122                 w2, SplitWayCommand.buildSplitChunks(w2, Arrays.asList(n3, n4, n5)), new ArrayList<OsmPrimitive>(), strategy);
     130                w2, SplitWayCommand.buildSplitChunks(w2, Arrays.asList(n3, n4, n5)), new ArrayList<>(), strategy);
    123131        UndoRedoHandler.getInstance().add(result);
    124132
    … …  
    140148    }
    141149
     150    @Test
     151    public void testOneMemberOrderedRelationShowsWarningTest() {
     152        final DataSet dataSet = new DataSet();
     153
     154        // Positive IDs to mark that these ways are incomplete (i.e., no nodes loaded).
     155        final Way w1 = new Way(1);
     156        final Way w3 = new Way(3);
     157
     158        // The way we are going to split is complete of course.
     159        final Node n1 = new Node(new LatLon(1, 0));
     160        final Node n2 = new Node(new LatLon(2, 0));
     161        final Node n3 = new Node(new LatLon(3, 0));
     162        final Way w2 = new Way();
     163
     164        final Relation route = new Relation();
     165        for (OsmPrimitive p : Arrays.asList(n1, n2, n3, w1, w2, w3, route)) {
     166            dataSet.addPrimitive(p);
     167        }
     168        w2.setNodes(Arrays.asList(n1, n2, n3));
     169
     170        route.put("type", "route");
     171        route.addMember(new RelationMember("", w1));
     172        route.addMember(new RelationMember("", w2));
     173        route.addMember(new RelationMember("", w3));
     174        dataSet.setSelected(Arrays.asList(w2, n2));
     175
     176        // This split cannot be safely performed without downloading extra relation members.
     177        // Here we ask the split method to abort if it needs more information.
     178        final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
     179                w2,
     180                SplitWayCommand.buildSplitChunks(w2, Collections.singletonList(n2)),
     181                new ArrayList<>(),
     182                Strategy.keepLongestChunk(),
     183                SplitWayCommand.WhenRelationOrderUncertain.ABORT
     184        );
     185
     186        assertFalse(result.isPresent());
     187    }
     188
     189    @Test
     190    public void testDoIncompleteMembersOrderedRelationCorrectOrderTest() {
     191        for (int i = 0; i < 2; i++) {
     192            // All these permutations should result in a split that keeps the new parts in order.
     193            doIncompleteMembersOrderedRelationCorrectOrderTest(false, false, i);
     194            doIncompleteMembersOrderedRelationCorrectOrderTest(true, false, i);
     195            doIncompleteMembersOrderedRelationCorrectOrderTest(true, true, i);
     196            doIncompleteMembersOrderedRelationCorrectOrderTest(false, true, i);
     197        }
     198    }
     199
     200    private void doIncompleteMembersOrderedRelationCorrectOrderTest(final boolean reverseWayOne,
     201                                                                    final boolean reverseWayTwo,
     202                                                                    final int indexOfWayToKeep) {
     203        final DataSet dataSet = new DataSet();
     204
     205        // Positive IDs to mark that these ways are incomplete (i.e., no nodes loaded).
     206        final Way w1 = new Way(1);
     207        final Way w4 = new Way(3);
     208
     209        // The ways we are going to split are complete of course.
     210        final Node n1 = new Node(new LatLon(1, 0));
     211        final Node n2 = new Node(new LatLon(2, 0));
     212        final Node n3 = new Node(new LatLon(3, 0));
     213        final Node n4 = new Node(new LatLon(4, 0));
     214        final Node n5 = new Node(new LatLon(5, 0));
     215        final Way w2 = new Way();
     216        final Way w3 = new Way();
     217
     218        final Relation route = new Relation();
     219        for (OsmPrimitive p : Arrays.asList(n1, n2, n3, n4, n5, w1, w2, w3, w4, route)) {
     220            dataSet.addPrimitive(p);
     221        }
     222        w2.setNodes(reverseWayOne ? Arrays.asList(n3, n2, n1) : Arrays.asList(n1, n2, n3));
     223        w3.setNodes(reverseWayTwo ? Arrays.asList(n5, n4, n3) : Arrays.asList(n3, n4, n5));
     224
     225        route.put("type", "route");
     226        route.addMember(new RelationMember("", w1));
     227        route.addMember(new RelationMember("", w2));
     228        route.addMember(new RelationMember("", w3));
     229        route.addMember(new RelationMember("", w4));
     230
     231        Way splitWay = indexOfWayToKeep == 0 ? w2 : w3;
     232        Node splitNode = indexOfWayToKeep == 0 ? n2 : n4;
     233
     234        dataSet.setSelected(Arrays.asList(splitWay, splitNode));
     235
     236        final SplitWayCommand result = SplitWayCommand.splitWay(
     237                splitWay, SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)), new ArrayList<>());
     238        UndoRedoHandler.getInstance().add(result);
     239
     240        assertEquals(5, route.getMembersCount());
     241        assertConnectedAtEnds(route.getMember(1).getWay(), route.getMember(2).getWay());
     242        assertConnectedAtEnds(route.getMember(2).getWay(), route.getMember(3).getWay());
     243    }
     244
    142245    static void assertFirstLastNodeIs(Way way, Node node) {
    143246        assertTrue("First/last node of " + way + " should be " + node, node.equals(way.firstNode()) || node.equals(way.lastNode()));
    144247    }
     248
     249    static void assertConnectedAtEnds(Way one, Way two) {
     250        Node first1 = one.firstNode();
     251        Node last1 = one.lastNode();
     252        Node first2 = two.firstNode();
     253        Node last2 = two.lastNode();
     254
     255        assertTrue("Ways expected to be connected at their ends.",
     256                first1 == first2 || first1 == last2 || last1 == first2 || last1 == last2);
     257    }
     258
     259    /**
     260     * Non-regression test for patch #18596 (Fix relation ordering after split-way)
     261     * @throws IOException if any I/O error occurs
     262     * @throws IllegalDataException if OSM parsing fails
     263     */
     264    @Test
     265    public void testTicket18596() throws IOException, IllegalDataException {
     266        try (InputStream is = TestUtils.getRegressionDataStream(18596, "data.osm")) {
     267            DataSet ds = OsmReader.parseDataSet(is, null);
     268
     269            Way splitWay = (Way) ds.getPrimitiveById(5, OsmPrimitiveType.WAY);
     270            Node splitNode = (Node) ds.getPrimitiveById(100002, OsmPrimitiveType.NODE);
     271
     272            final SplitWayCommand result = SplitWayCommand.splitWay(
     273                    splitWay,
     274                    SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)),
     275                    new ArrayList<>()
     276            );
     277
     278            UndoRedoHandler.getInstance().add(result);
     279
     280            Relation relation = (Relation) ds.getPrimitiveById(8888, OsmPrimitiveType.RELATION);
     281
     282            assertEquals(relation.getMembersCount(), 8);
     283
     284            // Before the patch introduced in #18596, these asserts would fail. The two parts of
     285            // way '5' would be in the wrong order, breaking the boundary relation in this test.
     286            assertConnectedAtEnds(relation.getMember(4).getWay(), relation.getMember(5).getWay());
     287            assertConnectedAtEnds(relation.getMember(5).getWay(), relation.getMember(6).getWay());
     288        }
     289    }
     290
     291    /**
     292     * Non-regression test for issue #17400 ( Warn when splitting way in not fully downloaded region)
     293     *
     294     * Bus route 190 gets broken when the split occurs, because the two new way parts are inserted in the relation in
     295     * the wrong order.
     296     *
     297     * @throws IOException if any I/O error occurs
     298     * @throws IllegalDataException if OSM parsing fails
     299     */
     300    @Test
     301    public void testTicket17400() throws IOException, IllegalDataException {
     302        try (InputStream is = TestUtils.getRegressionDataStream(17400, "data.osm")) {
     303            DataSet ds = OsmReader.parseDataSet(is, null);
     304
     305            Way splitWay = (Way) ds.getPrimitiveById(253731928, OsmPrimitiveType.WAY);
     306            Node splitNode = (Node) ds.getPrimitiveById(29830834, OsmPrimitiveType.NODE);
     307
     308            final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
     309                    splitWay,
     310                    SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)),
     311                    new ArrayList<>(),
     312                    Strategy.keepLongestChunk(),
     313                    // This split requires no additional downloads.
     314                    SplitWayCommand.WhenRelationOrderUncertain.ABORT
     315            );
     316
     317            assertTrue(result.isPresent());
     318
     319            UndoRedoHandler.getInstance().add(result.get());
     320
     321            // 190 Hormersdorf-Thalheim-Stollberg.
     322            Relation relation = (Relation) ds.getPrimitiveById(2873422, OsmPrimitiveType.RELATION);
     323
     324            // One more than the original 161.
     325            assertEquals(relation.getMembersCount(), 162);
     326
     327            // Before the patch introduced in #18596, these asserts would fail. The new parts of
     328            // the Hauptstraße would be in the wrong order, breaking the bus route relation.
     329            // These parts should be connected, in their relation sequence: 74---75---76.
     330            // Before #18596 this would have been a broken connection: 74---75-x-76.
     331            assertConnectedAtEnds(relation.getMember(74).getWay(), relation.getMember(75).getWay());
     332            assertConnectedAtEnds(relation.getMember(75).getWay(), relation.getMember(76).getWay());
     333        }
     334    }
    145335}
Note: See TracChangeset for help on using the changeset viewer.