Ticket #17010: 17010-v2.patch

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

@@ -53 +53 @@
     public static final int WRONG_MEMBER_ROLE = 1602;
     /** Multipolygon is not closed */
     public static final int NON_CLOSED_WAY = 1603;
-    /** No outer way for multipolygon */
-    public static final int MISSING_OUTER_WAY = 1604;
     /** Multipolygon inner way is outside */
     public static final int INNER_WAY_OUTSIDE = 1605;
     /** Intersection between multipolygon ways */
@@ -132 +130 @@
     @Override
     public void visit(Relation r) {
         if (r.isMultipolygon() && r.getMembersCount() > 0) {
-            checkMembersAndRoles(r);
-            checkOuterWay(r);
+            List<TestError> tmpErrors = new ArrayList<>(30);
+            boolean hasUnexpectedWayRoles = checkMembersAndRoles(r, tmpErrors);
             boolean hasRepeatedMembers = checkRepeatedWayMembers(r);
             // Rest of checks is only for complete multipolygon
-            if (!hasRepeatedMembers && !r.hasIncompleteMembers()) {
+            if (!hasUnexpectedWayRoles && !hasRepeatedMembers && !r.hasIncompleteMembers()) {
                 Multipolygon polygon = new Multipolygon(r);
                 checkStyleConsistency(r, polygon);
                 checkGeometryAndRoles(r, polygon);
+            } else {
+                // if we are not able to calculate the geometry we report the messages found by simple checks
+                errors.addAll(tmpErrors);
             }
         }
     }
 
     /**
-     * Checks that multipolygon has at least an outer way:<ul>
-     * <li>{@link #MISSING_OUTER_WAY}: No outer way for multipolygon</li>
-     * </ul>
-     * @param r relation
-     */
-    private void checkOuterWay(Relation r) {
-        for (RelationMember m : r.getMembers()) {
-            if (m.isWay() && "outer".equals(m.getRole())) {
-                return;
-            }
-        }
-        errors.add(TestError.builder(this, Severity.WARNING, MISSING_OUTER_WAY)
-                .message(r.isBoundary() ? tr("No outer way for boundary") : tr("No outer way for multipolygon"))
-                .primitives(r)
-                .build());
-    }
-
-    /**
      * Various style-related checks:<ul>
      * <li>{@link #NO_STYLE_POLYGON}: Multipolygon relation should be tagged with area tags and not the outer way</li>
      * <li>{@link #INNER_STYLE_MISMATCH}: With the currently used mappaint style the style for inner way equals the multipolygon style</li>
@@ -527 +510 @@
             String calculatedRole = (pol.level % 2 == 0) ? "outer" : "inner";
             for (long wayId : pol.outerWay.getWayIds()) {
                 RelationMember member = wayMap.get(wayId);
-                if (!member.getRole().equals(calculatedRole)) {
+                if (!calculatedRole.equals(member.getRole())) {
                     errors.add(TestError.builder(this, Severity.ERROR, WRONG_MEMBER_ROLE)
                             .message(RelationChecker.ROLE_VERIF_PROBLEM_MSG,
                                     marktr("Role for ''{0}'' should be ''{1}''"),
@@ -704 +687 @@
      * <li>{@link #WRONG_MEMBER_TYPE}: Non-Way in multipolygon</li>
      * </ul>
      * @param r relation
+     * @param tmpErrors list that will contain found errors
+     * @return true if ways with roles other than inner, outer or empty where found
      */
-    private void checkMembersAndRoles(Relation r) {
+    private boolean checkMembersAndRoles(Relation r, List<TestError> tmpErrors) {
+        boolean hasUnexpectedWayRole = false;
         for (RelationMember rm : r.getMembers()) {
             if (rm.isWay()) {
-                if (!(rm.hasRole("inner", "outer") || !rm.hasRole())) {
-                    errors.add(TestError.builder(this, Severity.WARNING, WRONG_MEMBER_ROLE)
-                            .message(tr("No useful role for multipolygon member"))
+                if (rm.hasRole() && !(rm.hasRole("inner", "outer")))
+                    hasUnexpectedWayRole = true;
+                if (!(rm.hasRole("inner", "outer")) || !rm.hasRole()) {
+                    tmpErrors.add(TestError.builder(this, Severity.ERROR, WRONG_MEMBER_ROLE)
+                            .message(tr("Role for multipolygon way member should be inner or outer"))
                             .primitives(Arrays.asList(r, rm.getMember()))
                             .build());
                 }
             } else {
                 if (!r.isBoundary() || !rm.hasRole("admin_centre", "label", "subarea", "land_area")) {
-                    errors.add(TestError.builder(this, Severity.WARNING, WRONG_MEMBER_TYPE)
+                    tmpErrors.add(TestError.builder(this, Severity.WARNING, WRONG_MEMBER_TYPE)
                             .message(r.isBoundary() ? tr("Non-Way in boundary") : tr("Non-Way in multipolygon"))
                             .primitives(Arrays.asList(r, rm.getMember()))
                             .build());
@@ -723 +711 @@
                 }
             }
         }
+        return hasUnexpectedWayRole;
     }
 
     private static Collection<? extends OsmPrimitive> combineRelAndPrimitives(Relation r, Collection<? extends OsmPrimitive> primitives) {
@@ -748 +737 @@
         boolean hasDups = false;
         Map<OsmPrimitive, List<RelationMember>> seenMemberPrimitives = new HashMap<>();
         for (RelationMember rm : r.getMembers()) {
+            if (!rm.isWay())
+                continue;
             List<RelationMember> list = seenMemberPrimitives.get(rm.getMember());
             if (list == null) {
                 list = new ArrayList<>(2);

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

@@ -18 +18 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.RelationMember;
+import org.openstreetmap.josm.data.validation.OsmValidator;
 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.ProgressMonitor;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetItem;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetType;
@@ -60 +62 @@
      * @since 6731
      */
     public static final String ROLE_VERIF_PROBLEM_MSG = tr("Role verification problem");
+    private boolean ignoreMultiPolygons;
 
     /**
      * Constructor
@@ -99 +102 @@
     }
 
     @Override
+    public void startTest(ProgressMonitor progressMonitor) {
+        super.startTest(progressMonitor);
+
+        for (Test t : OsmValidator.getEnabledTests(false)) {
+            if (t instanceof MultipolygonTest) {
+                ignoreMultiPolygons = true;
+                break;
+            }
+        }
+    }
+
+    @Override
     public void visit(Relation n) {
+        Map<String, RoleInfo> map = buildRoleInfoMap(n);
+        if (map.isEmpty()) {
+            errors.add(TestError.builder(this, Severity.ERROR, RELATION_EMPTY)
+                    .message(tr("Relation is empty"))
+                    .primitives(n)
+                    .build());
+        }
+        if (ignoreMultiPolygons && n.isMultipolygon()) {
+            // see #17010: don't report same problem twice
+            return;
+        }
         Map<Role, String> allroles = buildAllRoles(n);
         if (allroles.isEmpty() && n.hasTag("type", "route")
                 && n.hasTag("route", "train", "subway", "monorail", "tram", "bus", "trolleybus", "aerialway", "ferry")) {
@@ -114 +140 @@
                     .build());
         }
 
-        Map<String, RoleInfo> map = buildRoleInfoMap(n);
-        if (map.isEmpty()) {
-            errors.add(TestError.builder(this, Severity.ERROR, RELATION_EMPTY)
-                    .message(tr("Relation is empty"))
-                    .primitives(n)
-                    .build());
-        } else if (!allroles.isEmpty()) {
+        if (!map.isEmpty() && !allroles.isEmpty()) {
             checkRoles(n, allroles, map);
         }
     }