Changeset 14437 in josm for trunk/src

Timestamp:

2018-11-20T17:01:47+01:00 (6 years ago)

Author:

GerdP

Message:

fix #17010 - Validator creates multiple errors/warnings for same problem

The fix reduces the noise.

• Don't check roles again in RelationChecker when MultipolygonTest is enabled
• remove special code that just searches for an outer way
• correct checkMembersAndRoles, it did not find wrong roles for way members

Location:

trunk/src/org/openstreetmap/josm/data/validation/tests

Files:

• MultipolygonTest.java (modified) (7 diffs)
• RelationChecker.java (modified) (4 diffs)

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

@@ -54 +54 @@
     /** 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;
@@ -133 +131 @@
     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);
-            }
-        }
-    }
-
-    /**
-     * 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());
+                // see #17010: don't report problems twice
+                tmpErrors.removeIf(e -> e.getCode() == WRONG_MEMBER_ROLE);
+            }
+            errors.addAll(tmpErrors);
+        }
     }
 
@@ -528 +511 @@
             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,
@@ -705 +688 @@
      * </ul>
      * @param r relation
-     */
-    private void checkMembersAndRoles(Relation r) {
+     * @param tmpErrors list that will contain found errors
+     * @return true if ways with roles other than inner, outer or empty where found
+     */
+    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());
@@ -717 +705 @@
             } 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()))
@@ -724 +712 @@
             }
         }
+        return hasUnexpectedWayRole;
     }
 
@@ -749 +738 @@
         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) {

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

@@ -19 +19 @@
 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;
@@ -61 +63 @@
      */
     public static final String ROLE_VERIF_PROBLEM_MSG = tr("Role verification problem");
+    private boolean ignoreMultiPolygons;
 
     /**
@@ -100 +103 @@
 
     @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")
@@ -115 +141 @@
         }
 
-        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);
         }