Index: trunk/src/org/openstreetmap/josm/actions/relation/DownloadMembersAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/relation/DownloadMembersAction.java (revision 17340) +++ trunk/src/org/openstreetmap/josm/actions/relation/DownloadMembersAction.java (revision 17341) @@ -7,12 +7,12 @@ 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; /** @@ -35,12 +35,15 @@ 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: trunk/src/org/openstreetmap/josm/data/osm/DataSetMerger.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/DataSetMerger.java (revision 17340) +++ trunk/src/org/openstreetmap/josm/data/osm/DataSetMerger.java (revision 17341) @@ -302,20 +302,9 @@ return true; - if (target.isIncomplete() && !source.isIncomplete()) { - // target is incomplete, source completes it - // => merge source into target - // - target.mergeFrom(source); - objectsWithChildrenToMerge.add(source.getPrimitiveId()); - } else if (!target.isIncomplete() && source.isIncomplete()) { - // target is complete and source is incomplete - // => keep target, it has more information already - // - } else if (target.isIncomplete() && source.isIncomplete()) { - // target and source are incomplete. Doesn't matter which one to - // take. We take target. - // - } else if (!target.isModified() && !source.isModified() && target.isVisible() != source.isVisible() - && target.getVersion() == source.getVersion()) { + boolean mergeFromSource = false; + boolean haveSameVersion = target.getVersion() == source.getVersion(); + + if (haveSameVersion && !target.isModified() && !source.isModified() + && target.isVisible() != source.isVisible()) { // Same version, but different "visible" attribute and neither of them are modified. // It indicates a serious problem in datasets. @@ -324,5 +313,23 @@ 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()) { + } + + if (!target.isModified() && source.isDeleted()) { + // target not modified and source is deleted + // So mark it to be deleted. See #20091 + // + objectsToDelete.add(target); + } else if (source.isIncomplete()) { + // source is incomplete. Nothing to do. + // + } else if (target.isIncomplete()) { + // target is incomplete, source completes it + // => merge source into target + // + mergeFromSource = true; + } else if (target.isDeleted() && source.isDeleted() && !haveSameVersion) { + // both deleted. Source is newer. Take source. See #19783 + mergeFromSource = true; + } else if (target.isDeleted() && !source.isDeleted() && haveSameVersion) { // same version, but target is deleted. Assume target takes precedence // otherwise too many conflicts when refreshing from the server @@ -340,25 +347,13 @@ } } - } 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()); - } else if (!target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) { - // both not modified. Merge nevertheless. + mergeFromSource = true; + } else if (!target.isModified() && !source.isModified()) { + // both not modified. Merge nevertheless, even if versions are the same // This helps when updating "empty" relations, see #4295 - target.mergeFrom(source); - objectsWithChildrenToMerge.add(source.getPrimitiveId()); - } 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()); - } else if (target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) { + mergeFromSource = true; + } else if (target.isModified() && !source.isModified() && haveSameVersion) { // target is same as source but target is modified // => keep target and reset modified flag if target and source are semantically equal @@ -368,5 +363,5 @@ } else if (source.isDeleted() != target.isDeleted()) { // target is modified and deleted state differs. - // this have to be resolved manually. + // this has to be resolved manually. // addConflict(target, source); @@ -381,4 +376,7 @@ // attributes should already be equal if we get here. // + mergeFromSource = true; + } + if (mergeFromSource) { target.mergeFrom(source); objectsWithChildrenToMerge.add(source.getPrimitiveId()); Index: trunk/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java (revision 17340) +++ trunk/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java (revision 17341) @@ -7,4 +7,5 @@ import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -22,8 +23,10 @@ import org.openstreetmap.josm.data.projection.ProjectionRegistry; import org.openstreetmap.josm.data.projection.Projections; +import org.openstreetmap.josm.gui.progress.NullProgressMonitor; import org.openstreetmap.josm.testutils.JOSMTestRules; import org.openstreetmap.josm.tools.date.DateUtils; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + /** * Unit tests for class {@link DataSetMerger}. @@ -974,4 +977,205 @@ } + /** + * merge two equal objects with different visibility, + * => make sure that DataIntegrityProblemException is thrown. + */ + @Test + void testSameVersionAndDifferentVisibility() { + + // -- source dataset + + // a complete node + Node n1 = new Node(1, 2); + n1.setCoor(new LatLon(1, 1)); + n1.setVisible(true); + assertFalse(n1.isModified()); + their.addPrimitive(n1); + + // --- target dataset + // an complete node + Node n1b = new Node(1, 2); + n1b.setCoor(new LatLon(1, 1)); + n1b.setVisible(false); + assertFalse(n1b.isModified()); + my.addPrimitive(n1b); + + //-- merge it + DataSetMerger visitor = new DataSetMerger(my, their); + assertThrows(DataIntegrityProblemException.class, () -> visitor.merge()); + } + + /** + * node without referrers deleted in source + */ + @Test + void testDeletedSingleNode() { + + // -- source dataset + + // a complete node + Node n1 = new Node(1, 1); + n1.setCoor(new LatLon(1, 1)); + n1.setVisible(true); + assertFalse(n1.isModified()); + their.addPrimitive(n1); + + //-- merge it to create identical copies + DataSetMerger visitor = new DataSetMerger(my, their); + visitor.merge(); + + n1.setDeleted(true); + + visitor = new DataSetMerger(my, their); + visitor.merge(); + OsmPrimitive n1b = my.getPrimitiveById(n1); + assertTrue(n1b.isDeleted()); + } + + /** + * node without referrers deleted in source + */ + @Test + void testDeletedSingleNodeWithMonitor() { + + // -- source dataset + + // a complete node + Node n1 = new Node(1, 1); + n1.setCoor(new LatLon(1, 1)); + n1.setVisible(true); + assertFalse(n1.isModified()); + their.addPrimitive(n1); + + //-- merge it to create identical copies + DataSetMerger visitor = new DataSetMerger(my, their); + visitor.merge(NullProgressMonitor.INSTANCE); + + n1.setDeleted(true); + + visitor = new DataSetMerger(my, their); + visitor.merge(NullProgressMonitor.INSTANCE); + OsmPrimitive n1b = my.getPrimitiveById(n1); + assertTrue(n1b.isDeleted()); + } + + /** + * Way without referrers deleted in source + */ + @Test + void testDeletedWayNoReferrers () { + + // -- source dataset + + // a complete way with two nodes + Node n1 = new Node(1, 1); + n1.setCoor(new LatLon(1, 1)); + Node n2 = new Node(2, 1); + n2.setCoor(new LatLon(1, 1)); + n1.setVisible(true); + n2.setVisible(true); + their.addPrimitive(n1); + their.addPrimitive(n2); + Way w1 = new Way(1,1); + their.addPrimitive(w1); + w1.addNode(n1); + w1.addNode(n2); + w1.setVisible(true); + w1.setModified(false); + assertFalse(n1.isModified()); + assertFalse(n2.isModified()); + assertFalse(w1.isModified()); + + //-- merge it to create identical copies + DataSetMerger visitor = new DataSetMerger(my, their); + visitor.merge(); + + w1.setDeleted(true); + + visitor = new DataSetMerger(my, their); + visitor.merge(); + OsmPrimitive w1b = my.getPrimitiveById(w1); + assertTrue(w1b.isDeleted()); + } + + /** + * Dependency loop in relations,, both deleted in source + * => make sure that DataIntegrityProblemException is thrown. + */ + @Test + void testDeletedRelationLoop() { + + // -- source dataset + Relation r1 = new Relation(1, 1); + Relation r2 = new Relation(2, 1); + their.addPrimitive(r1); + their.addPrimitive(r2); + // create dependency loop + r1.addMember(new RelationMember("", r2)); + r2.addMember(new RelationMember("", r1)); + + //-- merge it to create identical copies + DataSetMerger visitor = new DataSetMerger(my, their); + visitor.merge(); + + r1.setMembers(null); + r2.setMembers(null); + r1.setDeleted(true); + r2.setDeleted(true); + visitor = new DataSetMerger(my, their); + visitor.merge(); + + OsmPrimitive r1b = my.getPrimitiveById(r1); + OsmPrimitive r2b = my.getPrimitiveById(r2); + assertTrue(r1b.isDeleted()); + assertTrue(r2b.isDeleted()); + } + + /** + * Way is deleted in my but member of relation in their + */ + @Test + void testDeletedWayStillMemberOfRelation() { + + // -- source dataset + Node n1 = new Node(1, 1); + n1.setCoor(new LatLon(1, 1)); + Node n2 = new Node(2, 1); + n2.setCoor(new LatLon(1, 1)); + n1.setVisible(true); + n2.setVisible(true); + their.addPrimitive(n1); + their.addPrimitive(n2); + Way w1 = new Way(1,1); + their.addPrimitive(w1); + w1.addNode(n1); + w1.addNode(n2); + w1.setVisible(true); + w1.setModified(false); + assertFalse(n1.isModified()); + assertFalse(n2.isModified()); + assertFalse(w1.isModified()); + + //-- merge it to create identical copies + DataSetMerger visitor = new DataSetMerger(my, their); + visitor.merge(); + + // let relation use the way that is in my dataset + Relation r1 = new Relation(1, 1); + their.addPrimitive(r1); + r1.addMember(new RelationMember("", w1)); + + Way myWay = (Way) my.getPrimitiveById(w1); + myWay.setNodes(null); + myWay.setDeleted(true); + + visitor = new DataSetMerger(my, their); + visitor.merge(); + assertFalse(visitor.getConflicts().isEmpty()); + assertFalse(myWay.isDeleted()); + assertTrue(myWay.isEmpty()); + my.clear(); // prevent error from consistency test + } + private void doTestTicket7481(DataSet source, DataSet target) { // Local node A @@ -1100,3 +1304,70 @@ 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()); + } + }