Changeset 15459 in josm

Timestamp:

2019-10-16T17:23:02+02:00 (5 years ago)

Author:

GerdP

Message:

fix #17837 and #18223, add unit tests

TODO:

• Will havily blow up size of preferences.xml when user ignores lots of "single elements" instead of the group.
• Maybe ignore entries in preferences with many (how many?) elements, as those probably were caused by error #18223 and are meaningless

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/validation/OsmValidator.java (modified) (9 diffs)
• test/unit/org/openstreetmap/josm/data/validation/OsmValidatorTest.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/data/validation/OsmValidator.java

@@ -260 +260 @@
 
     /**
-     *  Make sure that we don't keep single entries for a "group ignore" or
-     *  multiple different entries for the single entries that are in the same group.
-     */
-    private static void cleanupIgnoredErrors() {
+     *  Make sure that we don't keep single entries for a "group ignore".
+     */
+    protected static void cleanupIgnoredErrors() {
         if (ignoredErrors.size() > 1) {
             List<String> toRemove = new ArrayList<>();
 
             Iterator<Entry<String, String>> iter = ignoredErrors.entrySet().iterator();
-            Entry<String, String> last = iter.next();
+            String lastKey = iter.next().getKey();
             while (iter.hasNext()) {
-                Entry<String, String> entry = iter.next();
-                if (entry.getKey().startsWith(last.getKey())) {
-                    toRemove.add(entry.getKey());
+                String currKey = iter.next().getKey();
+                if (currKey.startsWith(lastKey) && sameCode(currKey, lastKey)) {
+                    toRemove.add(currKey);
                 } else {
-                    last = entry;
+                    lastKey = currKey;
                 }
             }
@@ -285 +284 @@
             ignoredErrors.putAll(tmap);
         }
+    }
+
+    private static boolean sameCode(String key1, String key2) {
+        return extractCodeFromIgnoreKey(key1).equals(extractCodeFromIgnoreKey(key2));
+    }
+
+    /**
+     * Extract the leading digits building the code for the error key.
+     * @param key the error key
+     * @return the leading digits
+     */
+    private static String extractCodeFromIgnoreKey(String key) {
+        int lenCode = 0;
+
+        for (int i = 0; i < key.length(); i++) {
+            if (key.charAt(i) >= '0' && key.charAt(i) <= '9') {
+                lenCode++;
+            } else {
+                break;
+            }
+        }
+        return key.substring(0, lenCode);
     }
 
@@ -314 +335 @@
         for (Entry<String, String> e: ignoredErrors.entrySet()) {
             String key = e.getKey();
-            String value = e.getValue();
-            ArrayList<String> ignoredWayList = new ArrayList<>();
+            // key starts with a code, it maybe followed by a string (eg. a MapCSS rule) and
+            // optionally with a list of one or more OSM element IDs
+            String description = e.getValue();
+
+            ArrayList<String> ignoredElementList = new ArrayList<>();
             String[] osmobjects = elemId1Pattern.split(key);
             for (int i = 1; i < osmobjects.length; i++) {
@@ -324 +348 @@
                     if (index < key.lastIndexOf(']')) continue;
                     char type = key.charAt(index - 1);
-                    ignoredWayList.add(type + osmid);
-                }
-            }
-            for (String osmignore : ignoredWayList) {
+                    ignoredElementList.add(type + osmid);
+                }
+            }
+            for (String osmignore : ignoredElementList) {
                 key = key.replace(':' + osmignore, "");
             }
@@ -334 +358 @@
             DefaultMutableTreeNode branch;
 
-            if (value != null && !value.isEmpty()) {
-                trunk = inTree(root, value);
+            if (description != null && !description.isEmpty()) {
+                trunk = inTree(root, description);
                 branch = inTree(trunk, key);
                 trunk.add(branch);
@@ -342 +366 @@
                 branch = trunk;
             }
-            ignoredWayList.forEach(osmignore -> branch.add(new DefaultMutableTreeNode(osmignore)));
-
+            if (!ignoredElementList.isEmpty()) {
+                String item;
+                if (ignoredElementList.size() == 1) {
+                    item = ignoredElementList.iterator().next();
+                } else {
+                    // combination of two or more objects, keep them together
+                    item = ignoredElementList.toString(); // [ID1, ID2, ..., IDn]
+                }
+                branch.add(new DefaultMutableTreeNode(item));
+            }
             root.add(trunk);
         }
@@ -378 +410 @@
         HashMap<String, String> rHashMap = new HashMap<>();
 
-        String osmids = node.getUserObject().toString();
-        String description = "";
-
-        if (!model.getRoot().equals(node)) {
-            description = ((DefaultMutableTreeNode) node.getParent()).getUserObject().toString();
-        } else {
-            description = node.getUserObject().toString();
-        }
-        if (tr("Ignore list").equals(description)) description = "";
-        if (!osmids.matches("^[0-9]+(_.*|$)")) {
-            description = osmids;
-            osmids = "";
-        }
-
-
-        StringBuilder sb = new StringBuilder();
         for (int i = 0; i < model.getChildCount(node); i++) {
             DefaultMutableTreeNode child = (DefaultMutableTreeNode) model.getChild(node, i);
             if (model.getChildCount(child) == 0) {
-                String ignoreName = child.getUserObject().toString();
-                if (ignoreName.matches("^(r|w|n)_.*")) {
-                    sb.append(':').append(child.getUserObject().toString());
-                } else if (ignoreName.matches("^[0-9]+(_.*|)$")) {
-                    rHashMap.put(ignoreName, description);
+                // create an entry for the error list
+                String key = node.getUserObject().toString();
+                String description;
+
+                if (!model.getRoot().equals(node)) {
+                    description = ((DefaultMutableTreeNode) node.getParent()).getUserObject().toString();
+                } else {
+                    description = key; // we get here when reading old file ignorederrors
+                }
+                if (tr("Ignore list").equals(description))
+                    description = "";
+                if (!key.matches("^[0-9]+(_.*|$)")) {
+                    description = key;
+                    key = "";
+                }
+
+                String item = child.getUserObject().toString();
+                String entry = null;
+                if (item.matches("^\\[(r|w|n)_.*")) {
+                    // list of elements (produced with list.toString() method)
+                    entry = key + ":" + item.substring(1, item.lastIndexOf(']')).replace(", ", ":");
+                } else if (item.matches("^(r|w|n)_.*")) {
+                    // single element
+                    entry = key + ":" + item;
+                } else if (item.matches("^[0-9]+(_.*|)$")) {
+                    // no element ids
+                    entry = item;
+                }
+                if (entry != null) {
+                    rHashMap.put(entry, description);
+                } else {
+                    Logging.warn("ignored unexpected item in validator ignore list management dialog:'" + item + "'");
                 }
             } else {
@@ -407 +450 @@
             }
         }
-        osmids += sb.toString();
-        if (!osmids.isEmpty() && osmids.indexOf(':') != 0) rHashMap.put(osmids, description);
         return rHashMap;
     }
@@ -616 +657 @@
                                 ))));
     }
+
+    /**
+     * For unit tests
+     */
+    protected static void clearIgnoredErrors() {
+        ignoredErrors.clear();
+    }
 }

trunk/test/unit/org/openstreetmap/josm/data/validation/OsmValidatorTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.data.validation;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -19 +23 @@
     @Rule
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules();
+    public JOSMTestRules test = new JOSMTestRules().projection();
 
+    /**
+     * Setup test.
+     * @throws Exception if an error occurs
+     */
+    @Before
+    public void setUp() throws Exception {
+        OsmValidator.clearIgnoredErrors();
+    }
     /**
      * Tests that {@code OsmValidator} satisfies utility class criterias.
@@ -29 +41 @@
         UtilityClassTestUtil.assertUtilityClassWellDefined(OsmValidator.class);
     }
+
+    /**
+     * Test that {@link OsmValidator#cleanupIgnoredErrors()} really removes entry with element IDs when group is ignored
+     */
+    @Test
+    public void testCleanupIgnoredErrors1() {
+        OsmValidator.addIgnoredError("1351:n_2449148994:w_236955234", "Way end node near other way");
+        OsmValidator.addIgnoredError("1351:n_6871910559:w_733713588", "Way end node near other way");
+        OsmValidator.addIgnoredError("1351");
+        OsmValidator.cleanupIgnoredErrors();
+        assertTrue(OsmValidator.hasIgnoredError("1351"));
+        assertFalse(OsmValidator.hasIgnoredError("1351:n_6871910559:w_733713588"));
+        assertFalse(OsmValidator.hasIgnoredError("1351:n_2449148994:w_236955234"));
+    }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/17837">Bug #17837</a>.
+     * {@link OsmValidator#cleanupIgnoredErrors()} must not remove entry 1201 when 120 is before it.
+     */
+    @Test
+    public void testCleanupIgnoredErrorsTicket17837() {
+        OsmValidator.addIgnoredError("120");
+        OsmValidator.addIgnoredError("3000");
+        OsmValidator.addIgnoredError("1201"); // starts with 120, but has different code
+        OsmValidator.cleanupIgnoredErrors();
+        assertTrue(OsmValidator.hasIgnoredError("1201"));
+    }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/18223">Bug #18223</a>.
+     * {@link OsmValidator#cleanupIgnoredErrors()} must not combine primitives.
+     */
+    @Test
+    public void testCleanupIgnoredErrorsTicket18223() {
+        OsmValidator.addIgnoredError("1351:n_2449148994:w_236955234", "Way end node near other way");
+        OsmValidator.addIgnoredError("1351:n_6871910559:w_733713588", "Way end node near other way");
+        OsmValidator.cleanupIgnoredErrors();
+        assertFalse(OsmValidator.hasIgnoredError("1351"));
+        assertTrue(OsmValidator.hasIgnoredError("1351:n_2449148994:w_236955234"));
+        assertTrue(OsmValidator.hasIgnoredError("1351:n_6871910559:w_733713588"));
+    }
+
 }