Index: src/org/openstreetmap/josm/actions/SplitWayAction.java =================================================================== --- src/org/openstreetmap/josm/actions/SplitWayAction.java (revision 16223) +++ src/org/openstreetmap/josm/actions/SplitWayAction.java (working copy) @@ -25,6 +25,7 @@ import javax.swing.JPanel; import javax.swing.ListSelectionModel; +import org.openstreetmap.josm.command.MissingDataHelper; import org.openstreetmap.josm.command.SplitWayCommand; import org.openstreetmap.josm.data.UndoRedoHandler; import org.openstreetmap.josm.data.osm.DataSet; @@ -293,7 +294,7 @@ wayToKeep, newWays, !isMapModeDraw ? newSelection : null, - SplitWayCommand.WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD + MissingDataHelper.WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD ); splitWayCommand.ifPresent(result -> { Index: src/org/openstreetmap/josm/command/MissingDataHelper.java =================================================================== --- src/org/openstreetmap/josm/command/MissingDataHelper.java (nonexistent) +++ src/org/openstreetmap/josm/command/MissingDataHelper.java (working copy) @@ -0,0 +1,133 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.command; + +import static org.openstreetmap.josm.tools.I18n.tr; + +import javax.swing.JOptionPane; + +import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.widgets.JMultilineLabel; + +/** + * Methods for actions which may corrupt relations. They should make sure that possible membership in relations is known + * and that relations are not corrupted by the modification of the member. + * @since xxx + */ +public class MissingDataHelper { + + private MissingDataHelper() { + // Hide default constructor for utils classes + } + + /** + * What to do when the action might modify a member of a relation and either the parents + * of the object are not known for certain or the action might need to add members in a specific order and that + * order cannot be determined with the available information. + * This can be used for unit tests. + */ + public enum WhenRelationOrderUncertain { + /** + * Ask the user to consent to downloading the missing information. The user can abort the operation or choose to + * proceed without downloading anything. + */ + ASK_USER_FOR_CONSENT_TO_DOWNLOAD, + /** + * If needed information is missing, abort the action. + */ + ABORT, + /** + * Perform the action even when needed information is missing, without downloading anything and risk to corrupt relations. + */ + PERFORM_ANYWAY, + /** + * If needed information is missing, automatically download these without prompting the user. + */ + DOWNLOAD_MISSING + } + + /** + * What to do if needed data is missing. + */ + public enum MissingDataStrategy { + /** continue, download needed data */ + GO_AHEAD_WITH_DOWNLOADS, + /** continue, don't download needed data and risk to break relations */ + GO_AHEAD_WITHOUT_DOWNLOADS, + /** aboort action */ + USER_ABORTED + } + + /** + * Check given preference key and optionally show dialog to ask user for confirmation regarding the download of missing data. + * @param whenRelationOrderUncertain What to do when the action might corrupt relations and additional data is needed + * @param preferenceKey the preference key which is used to store the user decision in case the answer is "Yes, download missing data" + * @return the strategy to use + */ + public static MissingDataStrategy getUserDecision(WhenRelationOrderUncertain whenRelationOrderUncertain, String preferenceKey) { + switch (whenRelationOrderUncertain) { + case ASK_USER_FOR_CONSENT_TO_DOWNLOAD: + // Only ask the user about downloading missing data when they haven't consented to this before. + if (ConditionalOptionPaneUtil.getDialogReturnValue(preferenceKey) == Integer.MAX_VALUE) { + // User has previously told us downloading missing relation members is fine. + return MissingDataStrategy.GO_AHEAD_WITH_DOWNLOADS; + } + // Ask the user. + return offerToDownloadMissingDataIfNeeded(preferenceKey); + case PERFORM_ANYWAY: + return MissingDataStrategy.GO_AHEAD_WITHOUT_DOWNLOADS; + case DOWNLOAD_MISSING: + return MissingDataStrategy.GO_AHEAD_WITH_DOWNLOADS; + case ABORT: + default: + return MissingDataStrategy.USER_ABORTED; + } + } + + static MissingDataStrategy offerToDownloadMissingDataIfNeeded(String preferenceKey) { + + JMultilineLabel msg = new JMultilineLabel(tr("Relations might be corrupted by splitting the way. " + + "Download missing information from Server?")); + + String[] options = { + tr("Yes, download missing data"), + tr("No, abort the split operation"), + tr("No, perform the split without downloading") + }; + + msg.setMaxWidth(600); + + int ret = JOptionPane.showOptionDialog( + MainApplication.getMainFrame(), + msg, + tr("Download missing information about relations?"), + 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 parent relations is + // likely to break relations. + ConditionalOptionPaneUtil.showMessageDialog( + preferenceKey, + MainApplication.getMainFrame(), + tr("Missing information about relations will be downloaded. " + + "Should this be done automatically from now on?"), + tr("Download missing information about relations"), + JOptionPane.INFORMATION_MESSAGE + ); + return MissingDataStrategy.GO_AHEAD_WITH_DOWNLOADS; + case JOptionPane.CANCEL_OPTION: + return MissingDataStrategy.GO_AHEAD_WITHOUT_DOWNLOADS; + default: + return MissingDataStrategy.USER_ABORTED; + } + } + +} Index: src/org/openstreetmap/josm/command/SplitWayCommand.java =================================================================== --- src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 16223) +++ src/org/openstreetmap/josm/command/SplitWayCommand.java (working copy) @@ -1,10 +1,6 @@ // License: GPL. For details, see LICENSE file. 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; @@ -24,8 +20,8 @@ import java.util.Set; import java.util.function.Consumer; -import javax.swing.JOptionPane; - +import org.openstreetmap.josm.command.MissingDataHelper.MissingDataStrategy; +import org.openstreetmap.josm.command.MissingDataHelper.WhenRelationOrderUncertain; import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.DefaultNameFormatter; import org.openstreetmap.josm.data.osm.Node; @@ -34,12 +30,11 @@ import org.openstreetmap.josm.data.osm.Relation; import org.openstreetmap.josm.data.osm.RelationMember; import org.openstreetmap.josm.data.osm.Way; -import org.openstreetmap.josm.gui.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.OsmServerBackreferenceReader; import org.openstreetmap.josm.io.OsmTransferException; import org.openstreetmap.josm.spi.preferences.Config; import org.openstreetmap.josm.tools.CheckParameterUtil; @@ -55,7 +50,7 @@ public class SplitWayCommand extends SequenceCommand { private static volatile Consumer warningNotifier = Logging::warn; - private static final String DOWNLOAD_MISSING_PREF_KEY = "split_way_download_missing_members"; + private static final String DOWNLOAD_MISSING_PREF_KEY = "split_way_download_missing_data"; private static final class RelationInformation { boolean warnme; @@ -285,7 +280,8 @@ // 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); + return splitWay(way, wayChunks, selection, splitStrategy, + WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD).orElse(null); } /** @@ -344,11 +340,34 @@ List newWays, List newSelection, WhenRelationOrderUncertain whenRelationOrderUncertain) { - if (whenRelationOrderUncertain == null) whenRelationOrderUncertain = ASK_USER_FOR_CONSENT_TO_DOWNLOAD; + if (whenRelationOrderUncertain == null) whenRelationOrderUncertain = WhenRelationOrderUncertain.ASK_USER_FOR_CONSENT_TO_DOWNLOAD; final int indexOfWayToKeep = newWays.indexOf(wayToKeep); newWays.remove(wayToKeep); + MissingDataStrategy missingDataStrategy = null; + if (!way.isNew() && (way.getDataSet().getDataSourceBounds().isEmpty() + || way.getNodes().stream().allMatch(Node::isOutsideDownloadArea))) { + missingDataStrategy = MissingDataHelper.getUserDecision(whenRelationOrderUncertain, DOWNLOAD_MISSING_PREF_KEY); + switch (missingDataStrategy) { + case GO_AHEAD_WITH_DOWNLOADS: + try { + downloadParents(way); + } catch (OsmTransferException e) { + ExceptionDialogUtil.explainException(e); + return Optional.empty(); + } + break; + case GO_AHEAD_WITHOUT_DOWNLOADS: + // Proceed with the split with the information we have. + // This can mean that we break relations. + break; + case USER_ABORTED: + default: + return Optional.empty(); + } + } + // Figure out the order of relation members (if any). Analysis analysis = analyseSplit(way, wayToKeep, newWays, indexOfWayToKeep); @@ -380,59 +399,28 @@ } } - MissingMemberStrategy missingMemberStrategy; if (relationsNeedingMoreMembers.isEmpty()) { // The split can be performed without any extra downloads. - missingMemberStrategy = GO_AHEAD_WITHOUT_DOWNLOADS; + missingDataStrategy = MissingDataStrategy.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; + if (missingDataStrategy == null) { + missingDataStrategy = MissingDataHelper.getUserDecision(whenRelationOrderUncertain, DOWNLOAD_MISSING_PREF_KEY); } } - - 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: + if (missingDataStrategy == MissingDataStrategy.USER_ABORTED) + return Optional.empty(); + if (missingDataStrategy == MissingDataStrategy.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)); } static Analysis analyseSplit(Way way, @@ -472,7 +460,7 @@ int ir = 0; List relationMembers = r.getMembers(); for (RelationMember rm : relationMembers) { - if (rm.isWay() && rm.getMember() == way) { + if (rm.getMember() == way) { boolean insert = true; if (relationSpecialTypes.containsKey(type) && "restriction".equals(relationSpecialTypes.get(type))) { RelationInformation rValue = treatAsRestriction(r, rm, c, newWays, way, changedWay); @@ -541,6 +529,22 @@ // won't help in any case. direction = Direction.IRRELEVANT; } + if (direction == Direction.UNKNOWN && (!way.firstNode().isOutsideDownloadArea() + || way.lastNode().isOutsideDownloadArea())) { + // check if any other complete way in the relation is connected to the way + // if so, the order of the relation is broken + for (int i = 0; i < r.getMembersCount(); i++) { + if (i >= ir - 1 && i <= ir + 1) + continue; + RelationMember rmTest = r.getMember(i); + if (rmTest.isWay() && !rmTest.getMember().isIncomplete() && + (way.isFirstLastNode(rmTest.getWay().firstNode()) + || way.isFirstLastNode(rm.getWay().lastNode()))) { + direction = Direction.IRRELEVANT; + break; + } + } + } } } else { int k = 1; @@ -627,78 +631,6 @@ } } - 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(); @@ -708,6 +640,13 @@ MainApplication.getLayerManager().getEditLayer().mergeFrom(ds); } + private static void downloadParents(Way way) throws OsmTransferException { + // Download possible parent relations + OsmServerBackreferenceReader reader = new OsmServerBackreferenceReader(way); + DataSet ds = reader.parseOsm(NullProgressMonitor.INSTANCE); + MainApplication.getLayerManager().getEditLayer().mergeFrom(ds); + } + static SplitWayCommand splitBasedOnAnalyses(Way way, List newWays, List newSelection, @@ -879,34 +818,6 @@ 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; @@ -959,9 +870,4 @@ ROLE } - enum MissingMemberStrategy { - GO_AHEAD_WITH_DOWNLOADS, - GO_AHEAD_WITHOUT_DOWNLOADS, - USER_ABORTED - } } Index: test/data/regress/18596/data.osm =================================================================== --- test/data/regress/18596/data.osm (revision 16223) +++ test/data/regress/18596/data.osm (working copy) @@ -1,5 +1,6 @@ + Index: test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java =================================================================== --- test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 16223) +++ test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (working copy) @@ -17,6 +17,8 @@ import org.junit.Test; import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.command.SplitWayCommand.Strategy; +import org.openstreetmap.josm.data.Bounds; +import org.openstreetmap.josm.data.DataSource; import org.openstreetmap.josm.data.UndoRedoHandler; import org.openstreetmap.josm.data.coor.LatLon; import org.openstreetmap.josm.data.osm.DataSet; @@ -150,6 +152,7 @@ @Test public void testOneMemberOrderedRelationShowsWarningTest() { final DataSet dataSet = new DataSet(); + dataSet.addDataSource(new DataSource(new Bounds(-180, -90, 180, 90), "Test")); // Positive IDs to mark that these ways are incomplete (i.e., no nodes loaded). final Way w1 = new Way(1); @@ -180,7 +183,7 @@ SplitWayCommand.buildSplitChunks(w2, Collections.singletonList(n2)), new ArrayList<>(), Strategy.keepLongestChunk(), - SplitWayCommand.WhenRelationOrderUncertain.ABORT + MissingDataHelper.WhenRelationOrderUncertain.ABORT ); assertFalse(result.isPresent()); @@ -311,7 +314,7 @@ new ArrayList<>(), Strategy.keepLongestChunk(), // This split requires no additional downloads. - SplitWayCommand.WhenRelationOrderUncertain.ABORT + MissingDataHelper.WhenRelationOrderUncertain.ABORT ); assertTrue(result.isPresent()); @@ -359,7 +362,7 @@ new ArrayList<>(), Strategy.keepLongestChunk(), // This split requires no additional downloads. If any are needed, this command will fail. - SplitWayCommand.WhenRelationOrderUncertain.ABORT + MissingDataHelper.WhenRelationOrderUncertain.ABORT ); // Should not result in aborting the split.