Index: src/org/openstreetmap/josm/actions/relation/DownloadMembersAction.java =================================================================== --- src/org/openstreetmap/josm/actions/relation/DownloadMembersAction.java (revision 17322) +++ src/org/openstreetmap/josm/actions/relation/DownloadMembersAction.java (working copy) @@ -6,14 +6,14 @@ import java.awt.event.ActionEvent; import java.util.Collection; +import java.util.List; +import java.util.stream.Collectors; import org.openstreetmap.josm.data.osm.IPrimitive; -import org.openstreetmap.josm.data.osm.Relation; +import org.openstreetmap.josm.data.osm.PrimitiveId; import org.openstreetmap.josm.gui.MainApplication; -import org.openstreetmap.josm.gui.dialogs.relation.DownloadRelationTask; +import org.openstreetmap.josm.gui.io.DownloadPrimitivesTask; import org.openstreetmap.josm.tools.ImageProvider; -import org.openstreetmap.josm.tools.SubclassFilteredCollection; -import org.openstreetmap.josm.tools.Utils; /** * The action for downloading members of relations @@ -34,14 +34,17 @@ @Override public void actionPerformed(ActionEvent e) { if (!isEnabled() || relations.isEmpty() || !MainApplication.isDisplayingMapView()) return; - MainApplication.worker.submit(new DownloadRelationTask( - Utils.filteredCollection(relations, Relation.class), MainApplication.getLayerManager().getEditLayer())); + List members = relations.stream() + .flatMap(r -> r.getMemberPrimitivesList().stream().filter(osm -> !osm.isNew()).map(IPrimitive::getOsmPrimitiveId)) + .distinct() + .collect(Collectors.toList()); + + MainApplication.worker.submit(new DownloadPrimitivesTask(MainApplication.getLayerManager().getEditLayer(), members, false)); } @Override public void setPrimitives(Collection primitives) { - // selected non-new relations - this.relations = SubclassFilteredCollection.filter(getRelations(primitives), r -> !r.isNew()); + this.relations = getRelations(primitives); updateEnabledState(); } Index: src/org/openstreetmap/josm/data/osm/DataSetMerger.java =================================================================== --- src/org/openstreetmap/josm/data/osm/DataSetMerger.java (revision 17322) +++ src/org/openstreetmap/josm/data/osm/DataSetMerger.java (working copy) @@ -9,6 +9,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -63,7 +64,7 @@ conflicts = new ConflictCollection(); mergedMap = new HashMap<>(); objectsWithChildrenToMerge = new HashSet<>(); - objectsToDelete = new HashSet<>(); + objectsToDelete = new LinkedHashSet<>(); } /** @@ -297,16 +298,21 @@ // found a corresponding target, remember it mergedMap.put(source.getPrimitiveId(), target.getPrimitiveId()); + boolean mergeFromSource = false; if (target.getVersion() > source.getVersion()) // target.version > source.version => keep target version return true; - if (target.isIncomplete() && !source.isIncomplete()) { + if (!target.isModified() && source.isDeleted()) { + // target not modified. We can assume that source is the most recent version, + // so mark it to be deleted. + // + objectsToDelete.add(target); + } else if (target.isIncomplete() && !source.isIncomplete()) { // target is incomplete, source completes it // => merge source into target // - target.mergeFrom(source); - objectsWithChildrenToMerge.add(source.getPrimitiveId()); + mergeFromSource = true; } else if (!target.isIncomplete() && source.isIncomplete()) { // target is complete and source is incomplete // => keep target, it has more information already @@ -323,6 +329,9 @@ // We shouldn't merge that datasets. throw new DataIntegrityProblemException(tr("Conflict in ''visible'' attribute for object of type {0} with id {1}", target.getType(), target.getId())); + } else if (target.isDeleted() && source.isDeleted() && target.getVersion() < source.getVersion()) { + // both deleted. Source is newer. Take source. See #19783 + mergeFromSource = true; } else if (target.isDeleted() && !source.isDeleted() && target.getVersion() == source.getVersion()) { // same version, but target is deleted. Assume target takes precedence // otherwise too many conflicts when refreshing from the server @@ -339,26 +348,18 @@ break; } } - } else if (!target.isModified() && source.isDeleted()) { - // target not modified. We can assume that source is the most recent version, - // so mark it to be deleted. - // - objectsToDelete.add(target); } else if (!target.isModified() && source.isModified()) { // target not modified. We can assume that source is the most recent version. // clone it into target. - target.mergeFrom(source); - objectsWithChildrenToMerge.add(source.getPrimitiveId()); + mergeFromSource = true; } else if (!target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) { // both not modified. Merge nevertheless. // This helps when updating "empty" relations, see #4295 - target.mergeFrom(source); - objectsWithChildrenToMerge.add(source.getPrimitiveId()); + mergeFromSource = true; } else if (!target.isModified() && !source.isModified() && target.getVersion() < source.getVersion()) { // my not modified but other is newer. clone other onto mine. // - target.mergeFrom(source); - objectsWithChildrenToMerge.add(source.getPrimitiveId()); + mergeFromSource = true; } else if (target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) { // target is same as source but target is modified // => keep target and reset modified flag if target and source are semantically equal @@ -380,6 +381,9 @@ // technical attributes like timestamp or user information. Semantic // attributes should already be equal if we get here. // + mergeFromSource = true; + } + if (mergeFromSource) { target.mergeFrom(source); objectsWithChildrenToMerge.add(source.getPrimitiveId()); } Index: test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java =================================================================== --- test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java (revision 17322) +++ test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java (working copy) @@ -1099,4 +1099,70 @@ assertEquals(new LatLon(1, 1), n.getCoor()); assertTrue(n.isModified()); } + + /** + * Non-regression test for Bug #19783. + */ + @Test + void testTicket19783() { + // Server node: deleted + Node n1 = new Node(1, 2); + n1.setDeleted(true); + n1.setVisible(false); + n1.setModified(false); + assertFalse(n1.isModified()); + their.addPrimitive(n1); + + // Local node: one modification: move + Node n1b = new Node(1, 1); + n1b.setCoor(new LatLon(1, 1)); + n1.setIncomplete(false); + assertFalse(n1b.isModified()); + my.addPrimitive(n1b); + n1b.setDeleted(true); + assertTrue(n1b.isModified()); + + // Merge + DataSetMerger visitor = new DataSetMerger(my, their); + visitor.merge(); + + // Check that modification is gone and version was merged + Node n = (Node) my.getPrimitiveById(1, OsmPrimitiveType.NODE); + assertNotNull(n); + assertFalse(n.isModified()); + assertFalse(n.isVisible()); + assertTrue(n.isDeleted()); + assertEquals(2, n.getVersion()); + } + + /** + * Non-regression test for Bug #20091. + * Relation refers to incomplete way that ways deleted on server. + */ + @Test + void testTicket20091() { + // Server way: deleted + Way w1 = new Way(1, 2); + w1.setDeleted(true); + w1.setVisible(false); + w1.setModified(false); + assertFalse(w1.isModified()); + their.addPrimitive(w1); + + Way w1b = new Way(1); + w1b.setIncomplete(true); + my.addPrimitive(w1b); + Relation r1 = new Relation(1, 1); + r1.addMember(new RelationMember("outer", w1b)); + r1.setModified(true); + my.addPrimitive(r1); + + // Merge + DataSetMerger visitor = new DataSetMerger(my, their); + visitor.merge(); + + assertEquals(1, visitor.getConflicts().size()); + assertTrue(r1.isModified()); + assertEquals(w1b, visitor.getConflicts().iterator().next().getMy()); + } }