Ticket #19956: 19956.patch

src/org/openstreetmap/josm/data/validation/tests/DuplicateRelation.java

@@ -28 +28 @@
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.Test;
 import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.tools.MultiMap;
 
@@ -201 +202 @@
 
     @Override
     public void endTest() {
-        super.endTest();
         for (Set<OsmPrimitive> duplicated : relations.values()) {
             if (duplicated.size() > 1) {
                 TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_RELATION)
@@ -222 +222 @@
             }
         }
         relationsNoKeys = null;
+        super.endTest();
     }
 
     @Override
@@ -245 +246 @@
      */
     @Override
     public Command fixError(TestError testError) {
-        if (testError.getCode() == SAME_RELATION) return null;
-        Set<Relation> relFix = testError.primitives(Relation.class)
-                .filter(r -> !r.isDeleted())
+        if (!isFixable(testError))
+            return null;
+        Set<Relation> toFix = testError.primitives(Relation.class)
+                .filter(this::isPrimitiveUsable)
                 .collect(Collectors.toSet());
 
-        if (relFix.size() < 2)
+        if (toFix.size() < 2)
             return null;
 
+        // see #19956 check again
+        Test test2 = new DuplicateRelation();
+        test2.startTest(NullProgressMonitor.INSTANCE);
+        toFix.forEach(test2::visit);
+        test2.endTest();
+        boolean errorStillExists = false;
+        for (TestError e : test2.getErrors()) {
+            if (e.getCode() == testError.getCode() && e.getPrimitives().size() >= 2) {
+                // refresh primitives, might be fewer than before
+                toFix = e.primitives(Relation.class).collect(Collectors.toSet());
+                errorStillExists = true;
+                break;
+            }
+        }
+        if (!errorStillExists)
+            return null;
+
         long idToKeep = 0;
-        Relation relationToKeep = relFix.iterator().next();
+        Relation relationToKeep = toFix.iterator().next();
         // Find the relation that is member of one or more relations. (If any)
         Relation relationWithRelations = null;
         Collection<Relation> relRef = null;
-        for (Relation w : relFix) {
-            Collection<Relation> rel = w.referrers(Relation.class).collect(Collectors.toList());
+        for (Relation r : toFix) {
+            Collection<Relation> rel = r.referrers(Relation.class).collect(Collectors.toList());
             if (!rel.isEmpty()) {
                 if (relationWithRelations != null)
                     throw new AssertionError("Cannot fix duplicate relations: More than one relation is member of another relation.");
-                relationWithRelations = w;
+                relationWithRelations = r;
                 relRef = rel;
             }
             // Only one relation will be kept - the one with lowest positive ID, if such exist
             // or one "at random" if no such exists. Rest of the relations will be deleted
-            if (!w.isNew() && (idToKeep == 0 || w.getId() < idToKeep)) {
-                idToKeep = w.getId();
-                relationToKeep = w;
+            if (!r.isNew() && (idToKeep == 0 || r.getId() < idToKeep)) {
+                idToKeep = r.getId();
+                relationToKeep = r;
             }
         }
 
@@ -291 +310 @@
         }
 
         // Delete all relations in the list
-        relFix.remove(relationToKeep);
-        commands.add(new DeleteCommand(relFix));
+        toFix.remove(relationToKeep);
+        commands.add(new DeleteCommand(toFix));
         return new SequenceCommand(tr("Delete duplicate relations"), commands);
     }
 

src/org/openstreetmap/josm/data/validation/tests/DuplicateWay.java

@@ -29 +29 @@
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.Test;
 import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.tools.MultiMap;
 
@@ -122 +123 @@
 
     @Override
     public void endTest() {
-        super.endTest();
         for (Set<OsmPrimitive> duplicated : ways.values()) {
             if (duplicated.size() > 1) {
                 TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_WAY)
@@ -165 +165 @@
         ways = null;
         waysNoTags = null;
         knownHashCodes = null;
+        super.endTest();
     }
 
     /**
@@ -249 +250 @@
      */
     @Override
     public Command fixError(TestError testError) {
-        Set<Way> wayz = testError.primitives(Way.class)
-                .filter(w -> !w.isDeleted())
+        if (!isFixable(testError))
+            return null;
+
+        Set<Way> toFix = testError.primitives(Way.class)
+                .filter(this::isPrimitiveUsable)
                 .collect(Collectors.toSet());
 
-        if (wayz.size() < 2)
+        if (toFix.size() < 2)
             return null;
 
+        // see #19956 check again
+        Test test2 = new DuplicateWay();
+        test2.startTest(NullProgressMonitor.INSTANCE);
+        toFix.forEach(test2::visit);
+        test2.endTest();
+        boolean errorStillExists = false;
+        for (TestError e : test2.getErrors()) {
+            if (e.getCode() == testError.getCode() && e.getPrimitives().size() >= 2) {
+                // refresh primitives, might be fewer than before
+                toFix = e.primitives(Way.class).collect(Collectors.toSet());
+                errorStillExists = true;
+                break;
+            }
+        }
+        if (!errorStillExists)
+            return null;
+
         long idToKeep = 0;
-        Way wayToKeep = wayz.iterator().next();
+        Way wayToKeep = toFix.iterator().next();
         // Find the way that is member of one or more relations. (If any)
         Way wayWithRelations = null;
         List<Relation> relations = null;
-        for (Way w : wayz) {
+        for (Way w : toFix) {
             List<Relation> rel = w.referrers(Relation.class).collect(Collectors.toList());
             if (!rel.isEmpty()) {
                 if (wayWithRelations != null)
@@ -295 +316 @@
 
         // Delete all ways in the list
         // Note: nodes are not deleted, these can be detected and deleted at next pass
-        wayz.remove(wayToKeep);
-        commands.add(new DeleteCommand(wayz));
+        toFix.remove(wayToKeep);
+        commands.add(new DeleteCommand(toFix));
         return new SequenceCommand(tr("Delete duplicate ways"), commands);
     }