Changeset 14966 in josm for trunk/src/org/openstreetmap

Timestamp:

2019-04-07T07:54:49+02:00 (6 years ago)

Author:

GerdP

Message:

fix #17561 Confusing error message for turn restriction
fix #17567 rephrase warning for role location_hint in restriction relation

• improve validator messages for restriction relations
• suppress duplicate warnings for wrong roles from RelationChecker when TurnrestrictionTest is enabled
• add unit test for TurnrestrictionTest similar to the one for MultipolygonTest

Location:

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

Files:

• RelationChecker.java (modified) (4 diffs)
• TurnrestrictionTest.java (modified) (15 diffs)

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

@@ -64 +64 @@
     public static final String ROLE_VERIF_PROBLEM_MSG = tr("Role verification problem");
     private boolean ignoreMultiPolygons;
+    private boolean ignoreTurnRestrictions;
 
     /**
@@ -109 +110 @@
             if (t instanceof MultipolygonTest) {
                 ignoreMultiPolygons = true;
-                break;
+            }
+            if (t instanceof TurnrestrictionTest) {
+                ignoreTurnRestrictions = true;
             }
         }
@@ -125 +128 @@
         if (ignoreMultiPolygons && n.isMultipolygon()) {
             // see #17010: don't report same problem twice
+            return;
+        }
+        if (ignoreTurnRestrictions && n.hasTag("type","restriction")) {
+            // see #17561: don't report same problem twice
             return;
         }
@@ -321 +328 @@
                 if (!key.isEmpty()) {
                     errors.add(TestError.builder(this, Severity.WARNING, ROLE_UNKNOWN)
-                            .message(ROLE_VERIF_PROBLEM_MSG, marktr("Role ''{0}'' unknown in templates ''{1}''"), key, templates)
+                            .message(ROLE_VERIF_PROBLEM_MSG, marktr("Role ''{0}'' is not in templates ''{1}''"), key, templates)
                             .primitives(n)
                             .build());

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

@@ -5 +5 @@
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
@@ -38 +39 @@
     protected static final int SUPERFLUOUS = 1815;
     protected static final int FROM_EQUALS_TO = 1816;
+    protected static final int UNKNOWN_RESTRICTION = 1817;
+    protected static final int TO_CLOSED_WAY = 1818;
+    protected static final int FROM_CLOSED_WAY = 1819;
+
+    private static final List<String> SUPPORTED_RESTRICTIONS = Arrays.asList(
+            "no_right_turn", "no_left_turn", "no_u_turn", "no_straight_on",
+            "only_right_turn", "only_left_turn", "only_straight_on",
+            "no_entry", "no_exit"
+        );
 
     /**
@@ -50 +60 @@
         if (!r.hasTag("type", "restriction"))
             return;
+
+        if (!r.hasTag("restriction", SUPPORTED_RESTRICTIONS)) {
+            errors.add(TestError.builder(this, Severity.ERROR, UNKNOWN_RESTRICTION)
+                    .message(tr("Unknown restriction"))
+                    .primitives(r)
+                    .build());
+            return;
+        }
 
         Way fromWay = null;
@@ -98 +116 @@
                 default:
                     errors.add(TestError.builder(this, Severity.WARNING, UNKNOWN_ROLE)
-                            .message(tr("Unknown role"))
+                            .message(tr("Unknown role in restriction"))
                             .primitives(l)
                             .highlight(m.getMember())
@@ -105 +123 @@
             } else if (m.isNode()) {
                 Node n = m.getNode();
-                if ("via".equals(m.getRole())) {
+                switch (m.getRole()) {
+                case "via":
                     if (!via.isEmpty()) {
                         if (via.get(0) instanceof Node) {
@@ -115 +134 @@
                         via.add(n);
                     }
-                } else {
+                    break;
+                case "location_hint":
                     errors.add(TestError.builder(this, Severity.WARNING, UNKNOWN_ROLE)
-                            .message(tr("Unknown role"))
+                            .message(tr("Role location_hint in not in templates"))
+                            .primitives(l)
+                            .highlight(m.getMember())
+                            .build());
+                    break;
+                default:
+                    errors.add(TestError.builder(this, Severity.WARNING, UNKNOWN_ROLE)
+                            .message(tr("Unknown role in restriction"))
                             .primitives(l)
                             .highlight(m.getMember())
@@ -135 +162 @@
                     .primitives(r)
                     .build());
+            return;
         }
         if (moreto) {
@@ -141 +169 @@
                     .primitives(r)
                     .build());
+            return;
         }
         if (morevia) {
@@ -147 +176 @@
                     .primitives(r)
                     .build());
+            return;
         }
         if (mixvia) {
@@ -153 +183 @@
                     .primitives(r)
                     .build());
+            return;
         }
 
@@ -161 +192 @@
                     .build());
             return;
-        }
+        } else if (fromWay.isClosed()) {
+            errors.add(TestError.builder(this, Severity.ERROR, FROM_CLOSED_WAY)
+                    .message(tr("\"from\" way is a closed way"))
+                    .primitives(r)
+                    .highlight(fromWay)
+                    .build());
+            return;
+        }
+
         if (toWay == null) {
             errors.add(TestError.builder(this, Severity.ERROR, NO_TO)
                     .message(tr("No \"to\" way found"))
                     .primitives(r)
+                    .build());
+            return;
+        } else if (toWay.isClosed()) {
+            errors.add(TestError.builder(this, Severity.ERROR, TO_CLOSED_WAY)
+                    .message(tr("\"to\" way is a closed way"))
+                    .primitives(r)
+                    .highlight(toWay)
                     .build());
             return;
@@ -186 +232 @@
         if (via.get(0) instanceof Node) {
             final Node viaNode = (Node) via.get(0);
-            final Way viaPseudoWay = new Way();
-            viaPseudoWay.addNode(viaNode);
-            checkIfConnected(fromWay, viaPseudoWay,
-                    tr("The \"from\" way does not start or end at a \"via\" node."), FROM_VIA_NODE);
-            if (toWay.isOneway() != 0 && viaNode.equals(toWay.lastNode(true))) {
+            if (isFullOneway(toWay) && viaNode.equals(toWay.lastNode(true))) {
                 errors.add(TestError.builder(this, Severity.WARNING, SUPERFLUOUS)
                         .message(tr("Superfluous turnrestriction as \"to\" way is oneway"))
                         .primitives(r)
-                        .build());
-                return;
-            }
-            checkIfConnected(viaPseudoWay, toWay,
+                        .highlight(toWay)
+                        .build());
+                return;
+            }
+            if (isFullOneway(fromWay) && viaNode.equals(fromWay.firstNode(true))) {
+                errors.add(TestError.builder(this, Severity.WARNING, SUPERFLUOUS)
+                        .message(tr("Superfluous turnrestriction as \"from\" way is oneway"))
+                        .primitives(r)
+                        .highlight(fromWay)
+                        .build());
+                return;
+            }
+            final Way viaPseudoWay = new Way();
+            viaPseudoWay.addNode(viaNode);
+            checkIfConnected(r, fromWay, viaPseudoWay,
+                    tr("The \"from\" way does not start or end at a \"via\" node."), FROM_VIA_NODE);
+            checkIfConnected(r, viaPseudoWay, toWay,
                     tr("The \"to\" way does not start or end at a \"via\" node."), TO_VIA_NODE);
         } else {
+            if (isFullOneway(toWay) && ((Way) via.get(via.size() - 1)).isFirstLastNode(toWay.lastNode(true))) {
+                errors.add(TestError.builder(this, Severity.WARNING, SUPERFLUOUS)
+                        .message(tr("Superfluous turnrestriction as \"to\" way is oneway"))
+                        .primitives(r)
+                        .highlight(toWay)
+                        .build());
+                return;
+            }
+            if (isFullOneway(fromWay) && ((Way) via.get(0)).isFirstLastNode(fromWay.firstNode(true))) {
+                errors.add(TestError.builder(this, Severity.WARNING, SUPERFLUOUS)
+                        .message(tr("Superfluous turnrestriction as \"from\" way is oneway"))
+                        .primitives(r)
+                        .highlight(fromWay)
+                        .build());
+                return;
+            }
             // check if consecutive ways are connected: from/via[0], via[i-1]/via[i], via[last]/to
-            checkIfConnected(fromWay, (Way) via.get(0),
+            checkIfConnected(r, fromWay, (Way) via.get(0),
                     tr("The \"from\" and the first \"via\" way are not connected."), FROM_VIA_WAY);
             if (via.size() > 1) {
@@ -207 +278 @@
                     Way previous = (Way) via.get(i - 1);
                     Way current = (Way) via.get(i);
-                    checkIfConnected(previous, current,
+                    checkIfConnected(r, previous, current,
                             tr("The \"via\" ways are not connected."), UNCONNECTED_VIA);
                 }
             }
-            if (toWay.isOneway() != 0 && ((Way) via.get(via.size() - 1)).isFirstLastNode(toWay.lastNode(true))) {
-                errors.add(TestError.builder(this, Severity.WARNING, SUPERFLUOUS)
-                        .message(tr("Superfluous turnrestriction as \"to\" way is oneway"))
-                        .primitives(r)
-                        .build());
-                return;
-            }
-            checkIfConnected((Way) via.get(via.size() - 1), toWay,
+            checkIfConnected(r, (Way) via.get(via.size() - 1), toWay,
                     tr("The last \"via\" and the \"to\" way are not connected."), TO_VIA_WAY);
         }
@@ -227 +291 @@
     }
 
-    private void checkIfConnected(Way previous, Way current, String msg, int code) {
+    private void checkIfConnected(Relation r, Way previous, Way current, String msg, int code) {
         boolean c;
         if (isFullOneway(previous) && isFullOneway(current)) {
@@ -243 +307 @@
         }
         if (!c) {
+            List<OsmPrimitive> hilite = new ArrayList<>();
+            if (previous.getNodesCount() == 1 && previous.isNew())
+                hilite.add(previous.firstNode());
+            else
+                hilite.add(previous);
+            if (current.getNodesCount() == 1 && current.isNew())
+                hilite.add(current.firstNode());
+            else
+                hilite.add(current);
+            List<OsmPrimitive> primitives = new ArrayList<>();
+            primitives.add(r);
+            primitives.addAll(hilite);
             errors.add(TestError.builder(this, Severity.ERROR, code)
                     .message(msg)
-                    .primitives(previous, current)
+                    .primitives(primitives)
+                    .highlight(hilite)
                     .build());
         }