Index: trunk/src/org/openstreetmap/josm/actions/SplitWayAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/SplitWayAction.java (revision 15942) +++ trunk/src/org/openstreetmap/josm/actions/SplitWayAction.java (revision 15943) @@ -15,4 +15,5 @@ import java.util.Iterator; import java.util.List; +import java.util.Optional; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; @@ -220,5 +221,5 @@ list.setSelectedValue(wayToKeep, true); } else { - setHighlightedWaySegments(Collections.emptyList()); + setHighlightedWaySegments(Collections.emptyList()); DISPLAY_COUNT.decrementAndGet(); } @@ -287,10 +288,20 @@ final MapFrame map = MainApplication.getMap(); final boolean isMapModeDraw = map != null && map.mapMode == map.mapModeDraw; - final SplitWayCommand result = SplitWayCommand.doSplitWay(way, wayToKeep, newWays, !isMapModeDraw ? newSelection : null); - UndoRedoHandler.getInstance().add(result); - List newSel = result.getNewSelection(); - if (newSel != null && !newSel.isEmpty()) { - way.getDataSet().setSelected(newSel); - } + + Optional splitWayCommand = SplitWayCommand.doSplitWay( + way, + wayToKeep, + newWays, + !isMapModeDraw ? newSelection : null, + SplitWayCommand.WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD + ); + + splitWayCommand.ifPresent(result -> { + UndoRedoHandler.getInstance().add(result); + List newSel = result.getNewSelection(); + if (newSel != null && !newSel.isEmpty()) { + way.getDataSet().setSelected(newSel); + } + }); } Index: trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java =================================================================== --- trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 15942) +++ trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 15943) @@ -2,4 +2,8 @@ package org.openstreetmap.josm.command; +import static org.openstreetmap.josm.command.SplitWayCommand.MissingMemberStrategy.GO_AHEAD_WITHOUT_DOWNLOADS; +import static org.openstreetmap.josm.command.SplitWayCommand.MissingMemberStrategy.GO_AHEAD_WITH_DOWNLOADS; +import static org.openstreetmap.josm.command.SplitWayCommand.MissingMemberStrategy.USER_ABORTED; +import static org.openstreetmap.josm.command.SplitWayCommand.WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD; import static org.openstreetmap.josm.tools.I18n.tr; import static org.openstreetmap.josm.tools.I18n.trn; @@ -9,4 +13,5 @@ import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; @@ -20,4 +25,7 @@ import java.util.function.Consumer; +import javax.swing.JOptionPane; + +import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.DefaultNameFormatter; import org.openstreetmap.josm.data.osm.Node; @@ -27,8 +35,14 @@ import org.openstreetmap.josm.data.osm.RelationMember; import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil; +import org.openstreetmap.josm.gui.ExceptionDialogUtil; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.progress.NullProgressMonitor; +import org.openstreetmap.josm.gui.widgets.JMultilineLabel; +import org.openstreetmap.josm.io.MultiFetchServerObjectReader; +import org.openstreetmap.josm.io.OsmTransferException; import org.openstreetmap.josm.spi.preferences.Config; import org.openstreetmap.josm.tools.CheckParameterUtil; import org.openstreetmap.josm.tools.Logging; - /** * Splits a way into multiple ways (all identical except for their node list). @@ -42,4 +56,5 @@ private static volatile Consumer warningNotifier = Logging::warn; + private static final String DOWNLOAD_MISSING_PREF_KEY = "split_way_download_missing_members"; private static final class RelationInformation { @@ -60,4 +75,5 @@ private final Way originalWay; private final List newWays; + /** Map<Restriction type, type to treat it as> */ private static final Map relationSpecialTypes = new HashMap<>(); @@ -247,20 +263,53 @@ /** - * Splits the way {@code way} into chunks of {@code wayChunks} and replies - * the result of this process in an instance of {@link SplitWayCommand}. - * The {@link SplitWayCommand.Strategy} is used to determine which - * way chunk should reuse the old id and its history. - * - * Note that changes are not applied to the data yet. You have to - * submit the command first, i.e. {@code UndoRedoHandler.getInstance().add(result)}. - * - * @param way the way to split. Must not be null. - * @param wayChunks the list of way chunks into the way is split. Must not be null. - * @param selection The list of currently selected primitives + * Splits the way {@code way} into chunks of {@code wayChunks} and replies the result of this process in an instance + * of {@link SplitWayCommand}. The {@link SplitWayCommand.Strategy} is used to determine which way chunk should + * reuse the old id and its history. + *

+ * If the split way is part of relations, and the order of the new parts in these relations cannot be determined due + * to missing relation members, the user will be asked to consent to downloading these missing members. + *

+ * Note that changes are not applied to the data yet. You have to submit the command first, i.e. {@code + * UndoRedoHandler.getInstance().add(result)}. + * + * @param way the way to split. Must not be null. + * @param wayChunks the list of way chunks into the way is split. Must not be null. + * @param selection The list of currently selected primitives * @param splitStrategy The strategy used to determine which way chunk should reuse the old id and its history * @return the result from the split operation */ - public static SplitWayCommand splitWay(Way way, List> wayChunks, - Collection selection, Strategy splitStrategy) { + public static SplitWayCommand splitWay(Way way, + List> wayChunks, + Collection selection, + Strategy splitStrategy) { + + // This method could be refactored to use an Optional in the future, but would need to be deprecated first + // to phase out use by plugins. + return splitWay(way, wayChunks, selection, splitStrategy, ASK_USER_FOR_CONSENT_TO_DOWNLOAD).orElse(null); + } + + /** + * Splits the way {@code way} into chunks of {@code wayChunks} and replies the result of this process in an instance + * of {@link SplitWayCommand}. The {@link SplitWayCommand.Strategy} is used to determine which way chunk should + * reuse the old id and its history. + *

+ * Note that changes are not applied to the data yet. You have to submit the command first, i.e. {@code + * UndoRedoHandler.getInstance().add(result)}. + * + * @param way the way to split. Must not be null. + * @param wayChunks the list of way chunks into the way is split. Must not be null. + * @param selection The list of currently selected primitives + * @param splitStrategy The strategy used to determine which way chunk should reuse the old id and its + * history + * @param whenRelationOrderUncertain What to do when the split way is part of relations, and the order of the new + * parts in the relation cannot be determined without downloading missing relation + * members. + * @return The result from the split operation, may be an empty {@link Optional} if the operation is aborted. + */ + public static Optional splitWay(Way way, + List> wayChunks, + Collection selection, + Strategy splitStrategy, + WhenRelationOrderUncertain whenRelationOrderUncertain) { // build a list of commands, and also a new selection list final List newSelection = new ArrayList<>(selection.size() + wayChunks.size()); @@ -273,5 +322,7 @@ final Way wayToKeep = splitStrategy.determineWayToKeep(newWays); - return wayToKeep != null ? doSplitWay(way, wayToKeep, newWays, newSelection) : null; + return wayToKeep != null + ? doSplitWay(way, wayToKeep, newWays, newSelection, whenRelationOrderUncertain) + : Optional.empty(); } @@ -284,9 +335,110 @@ * @param newWays potential new ways * @param newSelection new selection list to update (optional: can be null) + * @param whenRelationOrderUncertain Action to perform when the order of the new parts in relations the way is + * member of could not be reliably determined. See + * {@link WhenRelationOrderUncertain}. * @return the {@code SplitWayCommand} */ - public static SplitWayCommand doSplitWay(Way way, Way wayToKeep, List newWays, List newSelection) { - - Collection commandList = new ArrayList<>(newWays.size()); + public static Optional doSplitWay(Way way, + Way wayToKeep, + List newWays, + List newSelection, + WhenRelationOrderUncertain whenRelationOrderUncertain) { + if (whenRelationOrderUncertain == null) whenRelationOrderUncertain = ASK_USER_FOR_CONSENT_TO_DOWNLOAD; + + final int indexOfWayToKeep = newWays.indexOf(wayToKeep); + newWays.remove(wayToKeep); + + // Figure out the order of relation members (if any). + Analysis analysis = analyseSplit(way, wayToKeep, newWays, indexOfWayToKeep); + + // If there are relations that cannot be split properly without downloading more members, + // present the user with an option to do so, or to abort the split. + List relationsNeedingMoreMembers = new ArrayList<>(); + Set incompleteMembers = new HashSet<>(); + for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) { + if (!relationAnalysis.needsMoreMembers()) continue; + + Relation relation = relationAnalysis.getRelation(); + int position = relationAnalysis.getPosition(); + int membersCount = relation.getMembersCount(); + + // Mark the neighbouring members for downloading if these are ways too. + relationsNeedingMoreMembers.add(relation); + RelationMember rmPrev = position == 0 + ? relation.getMember(membersCount - 1) + : relation.getMember(position - 1); + RelationMember rmNext = position == membersCount - 1 + ? relation.getMember(0) + : relation.getMember(position + 1); + + if (rmPrev != null && rmPrev.isWay()) { + incompleteMembers.add(rmPrev.getWay()); + } + if (rmNext != null && rmNext.isWay()) { + incompleteMembers.add(rmNext.getWay()); + } + } + + MissingMemberStrategy missingMemberStrategy; + if (relationsNeedingMoreMembers.isEmpty()) { + // The split can be performed without any extra downloads. + missingMemberStrategy = GO_AHEAD_WITHOUT_DOWNLOADS; + } else { + switch (whenRelationOrderUncertain) { + case ASK_USER_FOR_CONSENT_TO_DOWNLOAD: + // If the analysis shows that for some relations missing members should be downloaded, offer the user the + // chance to consent to this. + + // Only ask the user about downloading missing members when they haven't consented to this before. + if (ConditionalOptionPaneUtil.getDialogReturnValue(DOWNLOAD_MISSING_PREF_KEY) == Integer.MAX_VALUE) { + // User has previously told us downloading missing relation members is fine. + missingMemberStrategy = GO_AHEAD_WITH_DOWNLOADS; + } else { + // Ask the user. + missingMemberStrategy = offerToDownloadMissingMembersIfNeeded(analysis, relationsNeedingMoreMembers); + } + break; + case SPLIT_ANYWAY: + missingMemberStrategy = GO_AHEAD_WITHOUT_DOWNLOADS; + break; + case DOWNLOAD_MISSING_MEMBERS: + missingMemberStrategy = GO_AHEAD_WITH_DOWNLOADS; + break; + case ABORT: + default: + missingMemberStrategy = USER_ABORTED; + break; + } + } + + switch (missingMemberStrategy) { + case GO_AHEAD_WITH_DOWNLOADS: + try { + downloadMissingMembers(incompleteMembers); + } catch (OsmTransferException e) { + ExceptionDialogUtil.explainException(e); + return Optional.empty(); + } + // If missing relation members were downloaded, perform the analysis again to find the relation + // member order for all relations. + analysis = analyseSplit(way, wayToKeep, newWays, indexOfWayToKeep); + return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep)); + case GO_AHEAD_WITHOUT_DOWNLOADS: + // Proceed with the split with the information we have. + // This can mean that there are no missing members we want, or that the user chooses to continue + // the split without downloading them. + return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep)); + case USER_ABORTED: + default: + return Optional.empty(); + } + } + + static Analysis analyseSplit(Way way, + Way wayToKeep, + List newWays, + int indexOfWayToKeep) { + Collection commandList = new ArrayList<>(); Collection nowarnroles = Config.getPref().getList("way.split.roles.nowarn", Arrays.asList("outer", "inner", "forward", "backward", "north", "south", "east", "west")); @@ -296,20 +448,12 @@ changedWay.setNodes(wayToKeep.getNodes()); commandList.add(new ChangeCommand(way, changedWay)); - if (/*!isMapModeDraw &&*/ newSelection != null && !newSelection.contains(way)) { - newSelection.add(way); - } - final int indexOfWayToKeep = newWays.indexOf(wayToKeep); - newWays.remove(wayToKeep); - - if (/*!isMapModeDraw &&*/ newSelection != null) { - newSelection.addAll(newWays); - } + for (Way wayToAdd : newWays) { commandList.add(new AddCommand(way.getDataSet(), wayToAdd)); } - boolean warnmerole = false; - boolean warnme = false; - // now copy all relations to new way also + List relationAnalyses = new ArrayList<>(); + EnumSet warnings = EnumSet.noneOf(WarningType.class); + int numberOfRelations = 0; for (Relation r : OsmPrimitive.getParentRelations(Collections.singleton(way))) { @@ -317,20 +461,27 @@ continue; } + + numberOfRelations++; + Relation c = null; String type = Optional.ofNullable(r.get("type")).orElse(""); + // Known types of ordered relations. + boolean isOrderedRelation = "route".equals(type) || "multipolygon".equals(type) || "boundary".equals(type); int ic = 0; int ir = 0; List relationMembers = r.getMembers(); - for (RelationMember rm: relationMembers) { + for (RelationMember rm : relationMembers) { if (rm.isWay() && rm.getMember() == way) { boolean insert = true; if (relationSpecialTypes.containsKey(type) && "restriction".equals(relationSpecialTypes.get(type))) { RelationInformation rValue = treatAsRestriction(r, rm, c, newWays, way, changedWay); - warnme = rValue.warnme; + if (rValue.warnme) warnings.add(WarningType.GENERIC); insert = rValue.insert; c = rValue.relation; - } else if (!("route".equals(type)) && !("multipolygon".equals(type))) { - warnme = true; + } else if (!isOrderedRelation) { + // Warn the user when relations that are not a route or multipolygon are modified as a result + // of splitting up the way, because we can't tell if this might break anything. + warnings.add(WarningType.GENERIC); } if (c == null) { @@ -340,53 +491,71 @@ if (insert) { if (rm.hasRole() && !nowarnroles.contains(rm.getRole())) { - warnmerole = true; + warnings.add(WarningType.ROLE); } - Boolean backwards = null; - int k = 1; - while (ir - k >= 0 || ir + k < relationMembers.size()) { - if ((ir - k >= 0) && relationMembers.get(ir - k).isWay()) { - Way w = relationMembers.get(ir - k).getWay(); - if ((w.lastNode() == way.firstNode()) || w.firstNode() == way.firstNode()) { - backwards = Boolean.FALSE; - } else if ((w.firstNode() == way.lastNode()) || w.lastNode() == way.lastNode()) { - backwards = Boolean.TRUE; + // Attempt to determine the direction the ways in the relation are ordered. + Direction direction = Direction.UNKNOWN; + if (isOrderedRelation) { + if (way.lastNode() == way.firstNode()) { + // Self-closing way. + direction = 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). + if (ir - 1 >= 0 && relationMembers.get(ir - 1).isWay()) { + Way w = relationMembers.get(ir - 1).getWay(); + if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { + direction = Direction.FORWARDS; + } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { + direction = Direction.BACKWARDS; + } } - break; + if (ir + 1 < relationMembers.size() && relationMembers.get(ir + 1).isWay()) { + Way w = relationMembers.get(ir + 1).getWay(); + if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { + direction = Direction.BACKWARDS; + } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { + direction = Direction.FORWARDS; + } + } } - if ((ir + k < relationMembers.size()) && relationMembers.get(ir + k).isWay()) { - Way w = relationMembers.get(ir + k).getWay(); - if ((w.lastNode() == way.firstNode()) || w.firstNode() == way.firstNode()) { - backwards = Boolean.TRUE; - } else if ((w.firstNode() == way.lastNode()) || w.lastNode() == way.lastNode()) { - backwards = Boolean.FALSE; + } else { + int k = 1; + while (ir - k >= 0 || ir + k < relationMembers.size()) { + if (ir - k >= 0 && relationMembers.get(ir - k).isWay()) { + Way w = relationMembers.get(ir - k).getWay(); + if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { + direction = Direction.FORWARDS; + } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { + direction = Direction.BACKWARDS; + } + break; } - break; - } - k++; - } - - int j = ic; - final List waysToAddBefore = newWays.subList(0, indexOfWayToKeep); - for (Way wayToAdd : waysToAddBefore) { - RelationMember em = new RelationMember(rm.getRole(), wayToAdd); - j++; - if (Boolean.TRUE.equals(backwards)) { - c.addMember(ic + 1, em); - } else { - c.addMember(j - 1, em); + if (ir + k < relationMembers.size() && relationMembers.get(ir + k).isWay()) { + Way w = relationMembers.get(ir + k).getWay(); + if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) { + direction = Direction.BACKWARDS; + } else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) { + direction = Direction.FORWARDS; + } + break; + } + k++; } } - final List waysToAddAfter = newWays.subList(indexOfWayToKeep, newWays.size()); - for (Way wayToAdd : waysToAddAfter) { - RelationMember em = new RelationMember(rm.getRole(), wayToAdd); - j++; - if (Boolean.TRUE.equals(backwards)) { - c.addMember(ic, em); - } else { - c.addMember(j, em); - } - } - ic = j; + + // We don't have enough information to determine the order of the new ways in this relation. + // This may cause relations to be saved with the two new way sections in reverse order. + // + // This often breaks routes. + // + // The user should be asked to download more members, or to abort the split operation. + boolean needsMoreMembers = isOrderedRelation + && direction == Direction.UNKNOWN + && relationMembers.size() > 1 + && r.hasIncompleteMembers(); + + relationAnalyses.add(new RelationAnalysis(c, rm, direction, needsMoreMembers, ic)); + ic += indexOfWayToKeep; } } @@ -399,8 +568,171 @@ } } - if (warnmerole) { + + return new Analysis(relationAnalyses, commandList, warnings, numberOfRelations); + } + + static class Analysis { + List relationAnalyses; + Collection commands; + EnumSet warningTypes; + private final int numberOfRelations; + + Analysis(List relationAnalyses, + Collection commandList, + EnumSet warnings, + int numberOfRelations) { + this.relationAnalyses = relationAnalyses; + commands = commandList; + warningTypes = warnings; + this.numberOfRelations = numberOfRelations; + } + + List getRelationAnalyses() { + return relationAnalyses; + } + + Collection getCommands() { + return commands; + } + + EnumSet getWarningTypes() { + return warningTypes; + } + + public int getNumberOfRelations() { + return numberOfRelations; + } + } + + static MissingMemberStrategy offerToDownloadMissingMembersIfNeeded(Analysis analysis, + List relationsNeedingMoreMembers) { + String[] options = { + tr("Yes, download the missing members"), + tr("No, abort the split operation"), + tr("No, perform the split without downloading") + }; + + String msgMemberOfRelations = trn( + "This way is part of a relation.", + "This way is part of {0} relations.", + analysis.getNumberOfRelations(), + analysis.getNumberOfRelations() + ); + + String msgReferToRelations; + if (analysis.getNumberOfRelations() == 1) { + msgReferToRelations = tr("this relation"); + } else if (analysis.getNumberOfRelations() == relationsNeedingMoreMembers.size()) { + msgReferToRelations = tr("these relations"); + } else { + msgReferToRelations = trn( + "one relation", + "{0} relations", + relationsNeedingMoreMembers.size(), + relationsNeedingMoreMembers.size() + ); + } + + String msgRelationsMissingData = tr( + "For {0} the correct order of the new way parts could not be determined. " + + "To fix this, some missing relation members should be downloaded first.", + msgReferToRelations + ); + + JMultilineLabel msg = new JMultilineLabel(msgMemberOfRelations + " " + msgRelationsMissingData); + msg.setMaxWidth(600); + + int ret = JOptionPane.showOptionDialog( + MainApplication.getMainFrame(), + msg, + tr("Download missing relation members?"), + JOptionPane.OK_CANCEL_OPTION, + JOptionPane.QUESTION_MESSAGE, + null, + options, + options[0] + ); + + switch (ret) { + case JOptionPane.OK_OPTION: + // Ask the user if they want to do this automatically from now on. We only ask this for the download + // action, because automatically cancelling is confusing (the user can't tell why this happened), and + // automatically performing the split without downloading missing members despite needing them is + // likely to break a lot of routes. The user also can't tell the difference between a split that needs + // no downloads at all, and this special case where downloading missing relation members will prevent + // broken relations. + ConditionalOptionPaneUtil.showMessageDialog( + DOWNLOAD_MISSING_PREF_KEY, + MainApplication.getMainFrame(), + tr("Missing relation members will be downloaded. Should this be done automatically from now on?"), + tr("Downloading missing relation members"), + JOptionPane.INFORMATION_MESSAGE + ); + return GO_AHEAD_WITH_DOWNLOADS; + case JOptionPane.CANCEL_OPTION: + return GO_AHEAD_WITHOUT_DOWNLOADS; + default: + return USER_ABORTED; + } + } + + static void downloadMissingMembers(Set incompleteMembers) throws OsmTransferException { + // Download the missing members. + MultiFetchServerObjectReader reader = MultiFetchServerObjectReader.create(); + reader.append(incompleteMembers); + + DataSet ds = reader.parseOsm(NullProgressMonitor.INSTANCE); + MainApplication.getLayerManager().getEditLayer().mergeFrom(ds); + } + + static SplitWayCommand splitBasedOnAnalyses(Way way, + List newWays, + List newSelection, + Analysis analysis, + int indexOfWayToKeep) { + if (newSelection != null && !newSelection.contains(way)) { + newSelection.add(way); + } + + if (newSelection != null) { + newSelection.addAll(newWays); + } + + // Perform the split. + for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) { + RelationMember rm = relationAnalysis.getRelationMember(); + Relation relation = relationAnalysis.getRelation(); + Direction direction = relationAnalysis.getDirection(); + int position = relationAnalysis.getPosition(); + + int j = position; + final List waysToAddBefore = newWays.subList(0, indexOfWayToKeep); + for (Way wayToAdd : waysToAddBefore) { + RelationMember em = new RelationMember(rm.getRole(), wayToAdd); + j++; + if (direction == Direction.BACKWARDS) { + relation.addMember(position + 1, em); + } else { + relation.addMember(j - 1, em); + } + } + final List waysToAddAfter = newWays.subList(indexOfWayToKeep, newWays.size()); + for (Way wayToAdd : waysToAddAfter) { + RelationMember em = new RelationMember(rm.getRole(), wayToAdd); + j++; + if (direction == Direction.BACKWARDS) { + relation.addMember(position, em); + } else { + relation.addMember(j, em); + } + } + } + + EnumSet warnings = analysis.getWarningTypes(); + + if (warnings.contains(WarningType.ROLE)) { warningNotifier.accept( tr("A role based relation membership was copied to all new ways.
You should verify this and correct it when necessary.")); - } else if (warnme) { + } else if (warnings.contains(WarningType.GENERIC)) { warningNotifier.accept( tr("A relation membership was copied to all new ways.
You should verify this and correct it when necessary.")); @@ -411,5 +743,5 @@ trn("Split way {0} into {1} part", "Split way {0} into {1} parts", newWays.size() + 1, way.getDisplayName(DefaultNameFormatter.getInstance()), newWays.size() + 1), - commandList, + analysis.getCommands(), newSelection, way, @@ -524,3 +856,89 @@ return relationSpecialTypes; } + + /** + * What to do when the split way is part of relations, and the order of the new parts in the relation cannot be + * determined without downloading missing relation members. + */ + public enum WhenRelationOrderUncertain { + /** + * Ask the user to consent to downloading the missing members. The user can abort the operation or choose to + * proceed without downloading anything. + */ + ASK_USER_FOR_CONSENT_TO_DOWNLOAD, + /** + * If there are relation members missing, and these are needed to determine the order of the new parts in + * that relation, abort the split operation. + */ + ABORT, + /** + * If there are relation members missing, and these are needed to determine the order of the new parts in + * that relation, continue with the split operation anyway, without downloading anything. Caution: use this + * option with care. + */ + SPLIT_ANYWAY, + /** + * If there are relation members missing, and these are needed to determine the order of the new parts in + * that relation, automatically download these without prompting the user. + */ + DOWNLOAD_MISSING_MEMBERS + } + + static class RelationAnalysis { + private final Relation relation; + private final RelationMember relationMember; + private final Direction direction; + private final boolean needsMoreMembers; + private final int position; + + RelationAnalysis(Relation relation, + RelationMember relationMember, + Direction direction, + boolean needsMoreMembers, + int position) { + this.relation = relation; + this.relationMember = relationMember; + this.direction = direction; + this.needsMoreMembers = needsMoreMembers; + this.position = position; + } + + RelationMember getRelationMember() { + return relationMember; + } + + Direction getDirection() { + return direction; + } + + boolean needsMoreMembers() { + return needsMoreMembers; + } + + Relation getRelation() { + return relation; + } + + int getPosition() { + return position; + } + } + + enum Direction { + FORWARDS, + BACKWARDS, + UNKNOWN, + IRRELEVANT + } + + enum WarningType { + GENERIC, + ROLE + } + + enum MissingMemberStrategy { + GO_AHEAD_WITH_DOWNLOADS, + GO_AHEAD_WITHOUT_DOWNLOADS, + USER_ABORTED + } } Index: trunk/test/data/regress/17400/data.osm =================================================================== --- trunk/test/data/regress/17400/data.osm (revision 15943) +++ trunk/test/data/regress/17400/data.osm (revision 15943) @@ -0,0 +1,3050 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/regress/18596/data.osm =================================================================== --- trunk/test/data/regress/18596/data.osm (revision 15943) +++ trunk/test/data/regress/18596/data.osm (revision 15943) @@ -0,0 +1,37 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 15942) +++ trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 15943) @@ -3,13 +3,18 @@ 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,7 +23,10 @@ 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,5 +43,5 @@ @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,12 +121,12 @@ final Strategy strategy = wayChunks -> { - final Iterator it = wayChunks.iterator(); - for (int i = 0; i < indexOfWayToKeep; i++) { - it.next(); - } - return it.next(); - }; + final Iterator 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(), strategy); + w2, SplitWayCommand.buildSplitChunks(w2, Arrays.asList(n3, n4, n5)), new ArrayList<>(), strategy); UndoRedoHandler.getInstance().add(result); @@ -140,6 +148,188 @@ } + @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 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 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()); + } + } }