Index: applications/editors/josm/plugins/reverter/src/reverter/ChangesetReverter.java =================================================================== --- applications/editors/josm/plugins/reverter/src/reverter/ChangesetReverter.java (revision 36317) +++ applications/editors/josm/plugins/reverter/src/reverter/ChangesetReverter.java (revision 36318) @@ -14,4 +14,5 @@ import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; @@ -56,4 +57,5 @@ public class ChangesetReverter { + /** The type of reversion */ public enum RevertType { FULL, @@ -67,14 +69,21 @@ )); + /** The id of the changeset */ public final int changesetId; + /** The changeset to be reverted */ public final Changeset changeset; + /** The reversion type */ public final RevertType revertType; - private final OsmDataLayer layer; // data layer associated with reverter - private final DataSet ds; // DataSet associated with reverter - private final ChangesetDataSet cds; // Current changeset data + /** data layer associated with reverter */ + private final OsmDataLayer layer; + /** DataSet associated with reverter */ + private final DataSet ds; + /** Current changeset data */ + private final ChangesetDataSet cds; /** original DataSet, used if a new layer is requested */ private final DataSet ods; - private DataSet nds; // Dataset that contains new objects downloaded by reverter + /** Dataset that contains new objects downloaded by reverter */ + private DataSet nds; private final HashSet missing = new HashSet<>(); @@ -87,4 +96,9 @@ //// Handling missing objects //////////////////////////////////////// + + /** + * Add a primitive that is not in {@link #ds} to {@link #missing} + * @param id The primitive to add if missing + */ private void addIfMissing(PrimitiveId id) { OsmPrimitive p = ds.getPrimitiveById(id); @@ -94,4 +108,8 @@ } + /** + * Add missing primitives to {@link #missing} + * @param primitives The primitives to iterate through + */ private void addMissingHistoryIds(Iterable primitives) { for (HistoryOsmPrimitive p : primitives) { @@ -100,4 +118,8 @@ } + /** + * Add missing necessary primitives to {@link #missing} + * @param p The primitive to look at + */ private void addMissingId(OsmPrimitive p) { addIfMissing(p); @@ -171,5 +193,11 @@ } - // Build our own lists of created/updated/modified objects for better performance + buildCreatedUpdatedModifiedObjectLists(); + } + + /** + * Build our own lists of created/updated/modified objects for better performance + */ + private void buildCreatedUpdatedModifiedObjectLists() { for (PrimitiveId id : cds.getIds()) { ChangesetDataSetEntry first = cds.getFirstEntry(id); @@ -189,12 +217,21 @@ } + /** + * Add missing primitives that have been created + */ public void checkMissingCreated() { addMissingHistoryIds(created); } + /** + * Add missing primitives that have been updated + */ public void checkMissingUpdated() { addMissingHistoryIds(updated); } + /** + * Add missing primitives that have been deleted + */ public void checkMissingDeleted() { addMissingHistoryIds(deleted); @@ -237,22 +274,5 @@ if (progressMonitor.isCanceled()) return; nds = rdr.parseOsm(progressMonitor.createSubTaskMonitor(1, true)); - ds.update(() -> { - for (OsmPrimitive p : nds.allPrimitives()) { - if (!p.isIncomplete()) { - addMissingId(p); - } else { - if (ds.getPrimitiveById(p.getPrimitiveId()) == null) { - switch (p.getType()) { - case NODE: ds.addPrimitive(new Node(p.getUniqueId())); break; - case CLOSEDWAY: - case WAY: ds.addPrimitive(new Way(p.getUniqueId())); break; - case MULTIPOLYGON: - case RELATION: ds.addPrimitive(new Relation(p.getUniqueId())); break; - default: throw new AssertionError(); - } - } - } - } - }); + ds.update(this::addPartialPrimitives); } finally { progressMonitor.finishTask(); @@ -260,4 +280,31 @@ } + /** + * Add partial primitives if they are incomplete in {@link #nds} + */ + private void addPartialPrimitives() { + for (OsmPrimitive p : nds.allPrimitives()) { + if (!p.isIncomplete()) { + addMissingId(p); + } else { + if (ds.getPrimitiveById(p.getPrimitiveId()) == null) { + switch (p.getType()) { + case NODE: ds.addPrimitive(new Node(p.getUniqueId())); break; + case CLOSEDWAY: + case WAY: ds.addPrimitive(new Way(p.getUniqueId())); break; + case MULTIPOLYGON: + case RELATION: ds.addPrimitive(new Relation(p.getUniqueId())); break; + default: throw new AssertionError(); + } + } + } + } + } + + /** + * Download any missing primitives we have + * @param monitor The monitor to update with our progress + * @throws OsmTransferException If there is an issue transferring data from OSM + */ public void downloadMissingPrimitives(ProgressMonitor monitor) throws OsmTransferException { if (!hasMissingObjects()) return; @@ -308,40 +355,70 @@ switch (current.getType()) { case NODE: - LatLon currentCoor = ((Node) current).getCoor(); - LatLon historyCoor = ((HistoryNode) history).getCoords(); - if (currentCoor == historyCoor || (currentCoor != null && historyCoor != null && currentCoor.equals(historyCoor))) - return true; - // Handle case where a deleted note has been restored to avoid false conflicts (fix #josm8660) - if (currentCoor != null && historyCoor == null) { - LatLon previousCoor = ((Node) nds.getPrimitiveById(history.getPrimitiveId())).getCoor(); - return previousCoor != null && previousCoor.equals(currentCoor); - } - return false; + return hasEqualSemanticAttributesNode(nds, (HistoryNode) history, (Node) current); case CLOSEDWAY: case WAY: - List currentNodes = ((Way) current).getNodes(); - List historyNodes = ((HistoryWay) history).getNodes(); - if (currentNodes.size() != historyNodes.size()) return false; - for (int i = 0; i < currentNodes.size(); i++) { - if (currentNodes.get(i).getId() != historyNodes.get(i)) return false; - } - return true; + return hasEqualSemanticAttributesWay((HistoryWay) history, (Way) current); case MULTIPOLYGON: case RELATION: - List currentMembers = - ((Relation) current).getMembers(); - List historyMembers = ((HistoryRelation) history).getMembers(); - if (currentMembers.size() != historyMembers.size()) return false; - for (int i = 0; i < currentMembers.size(); i++) { - org.openstreetmap.josm.data.osm.RelationMember currentMember = + return hasEqualSemanticAttributesRelation((HistoryRelation) history, (Relation) current); + default: throw new AssertionError(); + } + } + + /** + * Check if two nodes with the same tags are otherwise semantically similar + * @param nds The dataset with downloaded data from OSM (see {@link #nds}) + * @param history The node history + * @param current The current node + * @return {@code true} if they are semantically similar + */ + private static boolean hasEqualSemanticAttributesNode(DataSet nds, HistoryNode history, Node current) { + LatLon currentCoor = current.getCoor(); + LatLon historyCoor = history.getCoords(); + if (Objects.equals(currentCoor, historyCoor)) + return true; + // Handle case where a deleted note has been restored to avoid false conflicts (fix #josm8660) + if (currentCoor != null && historyCoor == null) { + LatLon previousCoor = ((Node) nds.getPrimitiveById(history.getPrimitiveId())).getCoor(); + return previousCoor != null && previousCoor.equals(currentCoor); + } + return false; + } + + /** + * Check if two ways with the same tags are otherwise semantically similar + * @param history The way history + * @param current The current way + * @return {@code true} if they are semantically similar + */ + private static boolean hasEqualSemanticAttributesWay(HistoryWay history, Way current) { + List currentNodes = current.getNodes(); + List historyNodes = history.getNodes(); + if (currentNodes.size() != historyNodes.size()) return false; + for (int i = 0; i < currentNodes.size(); i++) { + if (currentNodes.get(i).getId() != historyNodes.get(i)) return false; + } + return true; + } + + /** + * Check if two relations with the same tags are otherwise semantically similar + * @param history The relation history + * @param current The current relation + * @return {@code true} if they are semantically similar + */ + private static boolean hasEqualSemanticAttributesRelation(HistoryRelation history, Relation current) { + List currentMembers = current.getMembers(); + List historyMembers = history.getMembers(); + if (currentMembers.size() != historyMembers.size()) return false; + for (int i = 0; i < currentMembers.size(); i++) { + org.openstreetmap.josm.data.osm.RelationMember currentMember = currentMembers.get(i); - RelationMemberData historyMember = historyMembers.get(i); - if (!currentMember.getRole().equals(historyMember.getRole())) return false; - if (!currentMember.getMember().getPrimitiveId().equals(new SimplePrimitiveId( - historyMember.getMemberId(), historyMember.getMemberType()))) return false; - } - return true; - default: throw new AssertionError(); - } + RelationMemberData historyMember = historyMembers.get(i); + if (!currentMember.getRole().equals(historyMember.getRole())) return false; + if (!currentMember.getMember().getPrimitiveId().equals(new SimplePrimitiveId( + historyMember.getMemberId(), historyMember.getMemberType()))) return false; + } + return true; } @@ -386,38 +463,77 @@ } - // Check objects versions + checkObjectVersions(cmds, conflicted, toDelete); + checkForDeletedReferrers(cmds, conflicted, toDelete); + return cmds; + } + + /** + * Check objects versions + * @param cmds The command list to add to + * @param conflicted The primitives with conflicts + * @param toDelete The primitives that will be deleted + */ + private void checkObjectVersions(List cmds, HashSet conflicted, HashSet toDelete) { for (Iterator it = cds.iterator(); it.hasNext();) { ChangesetDataSetEntry entry = it.next(); - if (!checkOsmChangeEntry(entry)) continue; - if (entry.getModificationType() == ChangesetModificationType.DELETED - && revertType == RevertType.SELECTION_WITH_UNDELETE) { - // see #22520: missing merge target when object is first created and then - // deleted in the same changeset - ChangesetDataSetEntry first = cds.getFirstEntry(entry.getPrimitive().getPrimitiveId()); - if (first.getModificationType() == ChangesetModificationType.CREATED) - continue; - } - HistoryOsmPrimitive hp = entry.getPrimitive(); - OsmPrimitive dp = ds.getPrimitiveById(hp.getPrimitiveId()); - if (dp == null || dp.isIncomplete()) { - throw new IllegalStateException(addChangesetIdPrefix(tr("Missing merge target for {0}", hp.getPrimitiveId()))); - } - - if (hp.getVersion() != dp.getVersion() - && (hp.isVisible() || dp.isVisible()) && - /* Don't create conflict if changeset object and dataset object - * has same semantic attributes (but different versions) */ - !hasEqualSemanticAttributes(dp, hp) - /* Don't create conflict if the object has to be deleted but has already been deleted */ - && !(toDelete.contains(dp) && dp.isDeleted())) { - cmds.add(new ConflictAddCommand(layer.data, createConflict(dp, - entry.getModificationType() == ChangesetModificationType.CREATED))); - conflicted.add(dp); - } - } - - /* Check referrers for deleted objects: if object is referred by another object that - * isn't going to be deleted or modified, create a conflict. - */ + if (checkOsmChangeEntry(entry) && checkModificationType(cds, entry, revertType)) { + HistoryOsmPrimitive hp = entry.getPrimitive(); + OsmPrimitive dp = ds.getPrimitiveById(hp.getPrimitiveId()); + if (dp == null || dp.isIncomplete()) { + throw new IllegalStateException(addChangesetIdPrefix(tr("Missing merge target for {0}", hp.getPrimitiveId()))); + } + + if (checkObjectVersionsNotSemanticallySame(toDelete, hp, dp)) { + cmds.add(new ConflictAddCommand(layer.data, createConflict(dp, + entry.getModificationType() == ChangesetModificationType.CREATED))); + conflicted.add(dp); + } + } + } + } + + /** + * Check that the modification type is something that we are looking at + * @param cds {@link #cds} (current changeset data) + * @param entry The entry in the changeset data + * @param revertType The type of revert we are doing + * @return {@code true} if the entry is something we are interested in for this revert + */ + private static boolean checkModificationType(ChangesetDataSet cds, ChangesetDataSetEntry entry, RevertType revertType) { + if (entry.getModificationType() == ChangesetModificationType.DELETED + && revertType == RevertType.SELECTION_WITH_UNDELETE) { + // see #22520: missing merge target when object is first created and then + // deleted in the same changeset + ChangesetDataSetEntry first = cds.getFirstEntry(entry.getPrimitive().getPrimitiveId()); + return first.getModificationType() != ChangesetModificationType.CREATED; + } + return true; + } + + /** + * Check that versions are not the same prior to creating a conflict + * @param toDelete The deleted objects + * @param hp The history primitive + * @param dp The current primitive + * @return {@code true} if the objects are not semantically the same + */ + private boolean checkObjectVersionsNotSemanticallySame(HashSet toDelete, HistoryOsmPrimitive hp, OsmPrimitive dp) { + return hp.getVersion() != dp.getVersion() + && (hp.isVisible() || dp.isVisible()) && + /* Don't create conflict if changeset object and dataset object + * has same semantic attributes (but different versions) */ + !hasEqualSemanticAttributes(dp, hp) + /* Don't create conflict if the object has to be deleted but has already been deleted */ + && !(toDelete.contains(dp) && dp.isDeleted()); + } + + /** + * Check referrers for deleted objects: if object is referred by another object that + * isn't going to be deleted or modified, create a conflict. + * @param cmds The command list to add to + * @param conflicted The primitives with conflicts + * @param toDelete The primitives that will be deleted + */ + private void checkForDeletedReferrers(List cmds, HashSet conflicted, HashSet toDelete) { List delSorted = toDelete.stream() .filter(p -> !p.isDeleted()) @@ -429,26 +545,5 @@ for (int i = 0; i < delSorted.size() && !restartNeeded; i++) { OsmPrimitive p = delSorted.get(i); - for (OsmPrimitive referrer : p.getReferrers()) { - if (toDelete.contains(referrer)) continue; // object is going to be deleted - if (nds.getPrimitiveById(referrer) != null) - continue; /* object is going to be modified so it cannot refer to - * objects created in changeset to be reverted - */ - if (conflicted.add(p)) { - cmds.add(new ConflictAddCommand(layer.data, createConflict(p, true))); - if (p instanceof Relation) { - // handle possible special case with nested relations - for (int j = 0; j < i; j++) { - if (delSorted.get(j).getReferrers().contains(p)) { - // we have to create a conflict for a previously processed relation - restartNeeded = true; - break; - } - } - } - } - toDelete.remove(p); - break; - } + restartNeeded = checkForDeletedReferrersPrimitive(cmds, conflicted, toDelete, delSorted, p, i); } } while (restartNeeded); @@ -458,11 +553,56 @@ cmds.add(new DeleteCommand(delSorted)); } - return cmds; - } - + } + + /** + * + * @param cmds The command list to add to + * @param conflicted The primitives with conflicts + * @param toDelete The primitives that will be deleted + * @param delSorted The list of sorted deleted objects we need to check against + * @param p The primitive to check + * @param i The current index in {@code delSorted} + * @return {@code true} if we need to restart processing + */ + private boolean checkForDeletedReferrersPrimitive(List cmds, HashSet conflicted, + HashSet toDelete, List delSorted, + OsmPrimitive p, int i) { + for (OsmPrimitive referrer : p.getReferrers()) { + if (toDelete.contains(referrer) || // object is going to be deleted + nds.getPrimitiveById(referrer) != null) + continue; /* object is going to be modified so it cannot refer to + * objects created in changeset to be reverted + */ + if (conflicted.add(p)) { + cmds.add(new ConflictAddCommand(layer.data, createConflict(p, true))); + if (p instanceof Relation) { + // handle possible special case with nested relations + for (int j = 0; j < i; j++) { + if (delSorted.get(j).getReferrers().contains(p)) { + // we have to create a conflict for a previously processed relation + return true; + } + } + } + } + toDelete.remove(p); + return false; + } + return false; + } + + /** + * Check if there are missing objects + * @return {@code true} if we need to handle missing objects + */ public boolean hasMissingObjects() { return !missing.isEmpty(); } + /** + * Get coordinates for nodes that have no coordinates + * @param progressMonitor The monitor to update our progress with + * @throws OsmTransferException If we have an issue getting data from the API + */ public void fixNodesWithoutCoordinates(ProgressMonitor progressMonitor) throws OsmTransferException { Collection nodes = new ArrayList<>(nds.getNodes()); Index: applications/editors/josm/plugins/reverter/src/reverter/RevertChangesetAction.java =================================================================== --- applications/editors/josm/plugins/reverter/src/reverter/RevertChangesetAction.java (revision 36317) +++ applications/editors/josm/plugins/reverter/src/reverter/RevertChangesetAction.java (revision 36318) @@ -2,4 +2,5 @@ package reverter; +import static org.openstreetmap.josm.tools.I18n.marktr; import static org.openstreetmap.josm.tools.I18n.tr; @@ -14,9 +15,16 @@ import reverter.ChangesetReverter.RevertType; +/** + * An action for reverting changesets + */ public class RevertChangesetAction extends JosmAction { + private static final String REVERT_CHANGESET = marktr("Revert changeset"); + /** + * Create a new action for reverting changesets + */ public RevertChangesetAction() { - super(tr("Revert changeset"), "revert-changeset", tr("Revert changeset"), - Shortcut.registerShortcut("tool:revert", tr("Data: {0}", tr("Revert changeset")), KeyEvent.VK_T, Shortcut.CTRL_SHIFT), + super(tr(REVERT_CHANGESET), "revert-changeset", tr(REVERT_CHANGESET), + Shortcut.registerShortcut("tool:revert", tr("Data: {0}", tr(REVERT_CHANGESET)), KeyEvent.VK_T, Shortcut.CTRL_SHIFT), true, false); } Index: applications/editors/josm/plugins/reverter/src/reverter/RevertChangesetTask.java =================================================================== --- applications/editors/josm/plugins/reverter/src/reverter/RevertChangesetTask.java (revision 36317) +++ applications/editors/josm/plugins/reverter/src/reverter/RevertChangesetTask.java (revision 36318) @@ -9,5 +9,4 @@ import java.util.Collections; import java.util.List; -import java.util.concurrent.Callable; import java.util.stream.Collectors; @@ -30,4 +29,7 @@ import reverter.ChangesetReverter.RevertType; +/** + * The task for reverting changesets + */ public class RevertChangesetTask extends PleaseWaitRunnable { private final Collection changesetIds; @@ -40,16 +42,41 @@ private int numberOfConflicts; + /** + * Create a new task for reverting a changeset with a default progress monitor and no new layer with user confirmation of the download + * @param changesetId The changeset id to revert + * @param revertType The type of revert to do + */ public RevertChangesetTask(int changesetId, RevertType revertType) { this(changesetId, revertType, false); } + /** + * Create a new task for reverting a changeset with a default progress monitor and no new layer + * @param changesetId The changeset id to revert + * @param revertType The type of revert to do + * @param autoConfirmDownload {@code true} if the user has already indicated that they want to download missing data + */ public RevertChangesetTask(int changesetId, RevertType revertType, boolean autoConfirmDownload) { this(changesetId, revertType, autoConfirmDownload, false); } + /** + * Create a new task for reverting a changeset with a default progress monitor + * @param changesetId The changeset id to revert + * @param revertType The type of revert to do + * @param autoConfirmDownload {@code true} if the user has already indicated that they want to download missing data + * @param newLayer {@code true} if the user wants the reversion to be on a new layer + */ public RevertChangesetTask(int changesetId, RevertType revertType, boolean autoConfirmDownload, boolean newLayer) { this(Collections.singleton(changesetId), revertType, autoConfirmDownload, newLayer); } + /** + * Create a new task for reverting a changeset with a default progress monitor + * @param changesetIds The changeset ids to revert + * @param revertType The type of revert to do + * @param autoConfirmDownload {@code true} if the user has already indicated that they want to download missing data + * @param newLayer {@code true} if the user wants the reversion to be on a new layer + */ public RevertChangesetTask(Collection changesetIds, RevertType revertType, boolean autoConfirmDownload, boolean newLayer) { this(null, changesetIds, revertType, autoConfirmDownload, newLayer); @@ -64,5 +91,6 @@ * @param newLayer {@code true} if the user wants the reversion to be on a new layer */ - public RevertChangesetTask(ProgressMonitor progressMonitor, Collection changesetIds, RevertType revertType, boolean autoConfirmDownload, boolean newLayer) { + public RevertChangesetTask(ProgressMonitor progressMonitor, Collection changesetIds, RevertType revertType, + boolean autoConfirmDownload, boolean newLayer) { super(tr("Reverting..."), progressMonitor, false); this.changesetIds = new ArrayList<>(changesetIds); @@ -77,13 +105,8 @@ if (!rev.hasMissingObjects()) return true; if (!downloadConfirmed) { - final Integer selectedOption = GuiHelper.runInEDTAndWaitAndReturn(new Callable() { - @Override - public Integer call() throws Exception { - return JOptionPane.showConfirmDialog(MainApplication.getMainFrame(), - tr("This changeset has objects that are not present in current dataset.\n" + - "It is needed to download them before reverting. Do you want to continue?"), - tr("Confirm"), JOptionPane.YES_NO_OPTION); - } - }); + final Integer selectedOption = GuiHelper.runInEDTAndWaitAndReturn(() -> JOptionPane.showConfirmDialog(MainApplication.getMainFrame(), + tr("This changeset has objects that are not present in current dataset.\n" + + "It is needed to download them before reverting. Do you want to continue?"), + tr("Confirm"), JOptionPane.YES_NO_OPTION)); downloadConfirmed = selectedOption != null && selectedOption == JOptionPane.YES_OPTION; if (!downloadConfirmed) return false; @@ -142,4 +165,5 @@ rev = new ChangesetReverter(changesetId, revertType, newLayer, oldDataSet, progressMonitor.createSubTaskMonitor(0, true)); } catch (final RevertRedactedChangesetException e) { + Logging.debug(e); GuiHelper.runInEDT(() -> new Notification( e.getMessage()+"
"+ @@ -191,5 +215,15 @@ return null; } - GuiHelper.runInEDT(() -> cmds.get(0).getAffectedDataSet().update(() -> { + GuiHelper.runInEDT(() -> executeCommands(cmds)); + final String desc = getRevertDescription(revertType, changesetId); + return new RevertChangesetCommand(desc, cmds); + } + + /** + * Execute a list of commands + * @param cmds The commands to execute + */ + private void executeCommands(List cmds) { + cmds.get(0).getAffectedDataSet().update(() -> { for (Command c : cmds) { if (c instanceof ConflictAddCommand) { @@ -198,12 +232,19 @@ c.executeCommand(); } - })); - final String desc; + }); + } + + /** + * Get the description to show the user for this reversion + * @param revertType The type of revert + * @param changesetId The changeset id + * @return The string to show the user + */ + private static String getRevertDescription(RevertType revertType, int changesetId) { if (revertType == RevertType.FULL) { - desc = tr("Revert changeset {0}", String.valueOf(changesetId)); + return tr("Revert changeset {0}", String.valueOf(changesetId)); } else { - desc = tr("Partially revert changeset {0}", String.valueOf(changesetId)); - } - return new RevertChangesetCommand(desc, cmds); + return tr("Partially revert changeset {0}", String.valueOf(changesetId)); + } }