Changeset 15064 in josm

Timestamp:

2019-05-09T10:13:10+02:00 (5 years ago)

Author:

GerdP

Message:

fix #12627,#14287,#14289,#17695

• let CrossingFinder and ContainsFinder find all objects instead of stopping at first match
• if objects are selected, make sure that ContainsFinder is called for enclosing objects which are not in the selection
• enable corresponding unit tests

Draw back: MapCSSTagChecker is a bit slower.

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (modified) (9 diffs)
• src/org/openstreetmap/josm/gui/mappaint/Environment.java (modified) (4 diffs)
• src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java (modified) (7 diffs)
• test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (modified) (5 diffs)

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

@@ -13 +13 @@
 import java.text.MessageFormat;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -40 +39 @@
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.INode;
+import org.openstreetmap.josm.data.osm.IPrimitive;
 import org.openstreetmap.josm.data.osm.IRelation;
 import org.openstreetmap.josm.data.osm.IWay;
@@ -70 +70 @@
 import org.openstreetmap.josm.gui.mappaint.mapcss.Selector;
 import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.AbstractSelector;
+import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.ChildOrParentSelector;
+import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.ChildOrParentSelectorType;
 import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.GeneralSelector;
 import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.OptimizedGeneralSelector;
@@ -734 +736 @@
          * @return an instance of {@link TestError}, or returns null if the primitive does not give rise to an error.
          */
-        TestError getErrorForPrimitive(OsmPrimitive p) {
+        List<TestError> getErrorsForPrimitive(OsmPrimitive p) {
             final Environment env = new Environment(p);
-            return getErrorForPrimitive(p, whichSelectorMatchesEnvironment(env), env, null);
-        }
-
-        TestError getErrorForPrimitive(OsmPrimitive p, Selector matchingSelector, Environment env, Test tester) {
+            return getErrorsForPrimitive(p, whichSelectorMatchesEnvironment(env), env, null);
+        }
+
+        private List<TestError> getErrorsForPrimitive(OsmPrimitive p, Selector matchingSelector, Environment env, Test tester) {
+            List<TestError> res = new ArrayList<>();
             if (matchingSelector != null && !errors.isEmpty()) {
                 final Command fix = fixPrimitive(p);
@@ -745 +748 @@
                 final String description1 = group == null ? description : group;
                 final String description2 = group == null ? null : description;
-                final List<OsmPrimitive> primitives;
+                TestError.Builder errorBuilder = TestError.builder(tester, getSeverity(), 3000)
+                        .messageWithManuallyTranslatedDescription(description1, description2, matchingSelector.toString());
+                if (fix != null) {
+                    errorBuilder = errorBuilder.fix(() -> fix);
+                }
                 if (env.child instanceof OsmPrimitive) {
-                    primitives = Arrays.asList(p, (OsmPrimitive) env.child);
+                    res.add(errorBuilder.primitives(p, (OsmPrimitive) env.child).build());
+                } else if (env.children != null) {
+                    for (IPrimitive c : env.children) {
+                        if (c instanceof OsmPrimitive) {
+                            errorBuilder = TestError.builder(tester, getSeverity(), 3000)
+                                    .messageWithManuallyTranslatedDescription(description1, description2,
+                                            matchingSelector.toString());
+                            if (fix != null) {
+                                errorBuilder = errorBuilder.fix(() -> fix);
+                            }
+                            res.add(errorBuilder.primitives(p, (OsmPrimitive) c).build());
+                        }
+                    }
                 } else {
-                    primitives = Collections.singletonList(p);
-                }
-                final TestError.Builder error = TestError.builder(tester, getSeverity(), 3000)
-                        .messageWithManuallyTranslatedDescription(description1, description2, matchingSelector.toString())
-                        .primitives(primitives);
-                if (fix != null) {
-                    return error.fix(() -> fix).build();
-                } else {
-                    return error.build();
-                }
-            } else {
-                return null;
-            }
+                    res.add(errorBuilder.primitives(p).build());
+                }
+            }
+            return res;
         }
 
@@ -855 +865 @@
             MapCSSRule r = candidates.next();
             env.clearSelectorMatchingInformation();
+            if (partialSelection && r.selector instanceof Selector.ChildOrParentSelector) {
+                ChildOrParentSelector sel = (Selector.ChildOrParentSelector) r.selector;
+                if (sel.type == ChildOrParentSelectorType.ELEMENT_OF && p.getDataSet() != null) {
+                    List<OsmPrimitive> toCheck = new ArrayList<>();
+                    toCheck.addAll(p.getDataSet().searchWays(p.getBBox()));
+                    toCheck.addAll(p.getDataSet().searchRelations(p.getBBox()));
+                    toCheck.removeIf(OsmPrimitive::isSelected);
+                    if (!toCheck.isEmpty()) {
+                        Set<Set<TagCheck>> checksCol = Collections.singleton(Collections.singleton(indexData.getCheck(r)));
+                        for (OsmPrimitive p2 : toCheck) {
+                            for (TestError e : getErrorsForPrimitive(p2, includeOtherSeverity, checksCol)) {
+                                if (e.getPrimitives().contains(p)) {
+                                    addIfNotSimilar(e, res);
+                                }
+                            }
+                        }
+                    }
+                }
+            }
             if (r.selector.matches(env)) { // as side effect env.parent will be set (if s is a child selector)
                 TagCheck check = indexData.getCheck(r);
@@ -864 +893 @@
                     r.declaration.execute(env);
                     if (!check.errors.isEmpty()) {
-                        final TestError error = check.getErrorForPrimitive(p, r.selector, env, new MapCSSTagCheckerAndRule(check.rule));
-                        if (error != null) {
-                            res.add(error);
+                        for (TestError e: check.getErrorsForPrimitive(p, r.selector, env, new MapCSSTagCheckerAndRule(check.rule))) {
+                            addIfNotSimilar(e, res);
                         }
                     }
-
                 }
             }
         }
         return res;
+    }
+
+    /**
+     * See #12627
+     * Add error to given list if list doesn't already contain a similar error.
+     * Similar means same code and description and same combination of primitives and same combination of highlighted objects,
+     * but maybe with different orders.
+     * @param toAdd the error to add
+     * @param errors the list of errors
+     */
+    private static void addIfNotSimilar(TestError toAdd, List<TestError> errors) {
+        boolean isDup = false;
+        if (toAdd.getPrimitives().size() >= 2) {
+            for (TestError e : errors) {
+                if (e.getCode() == toAdd.getCode() && e.getMessage().equals(toAdd.getMessage())
+                        && e.getPrimitives().size() == toAdd.getPrimitives().size()
+                        && e.getPrimitives().containsAll(toAdd.getPrimitives())
+                        && e.getHighlighted().size() == toAdd.getHighlighted().size()
+                        && e.getHighlighted().containsAll(toAdd.getHighlighted())) {
+                    isDup = true;
+                    break;
+                }
+            }
+        }
+        if (!isDup)
+            errors.add(toAdd);
     }
 
@@ -891 +944 @@
                     check.rule.declaration.execute(env);
                     if (!ignoreError && !check.errors.isEmpty()) {
-                        final TestError error = check.getErrorForPrimitive(p, selector, env, new MapCSSTagCheckerAndRule(check.rule));
-                        if (error != null) {
-                            r.add(error);
-                        }
+                        r.addAll(check.getErrorsForPrimitive(p, selector, env, new MapCSSTagCheckerAndRule(check.rule)));
                     }
                 }
@@ -909 +959 @@
     @Override
     public void check(OsmPrimitive p) {
-        errors.addAll(getErrorsForPrimitive(p, ValidatorPrefHelper.PREF_OTHER.get()));
+        for (TestError e : getErrorsForPrimitive(p, ValidatorPrefHelper.PREF_OTHER.get())) {
+            addIfNotSimilar(e, errors);
+        }
     }
 

trunk/src/org/openstreetmap/josm/gui/mappaint/Environment.java

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.gui.mappaint;
+
+import java.util.LinkedHashSet;
+import java.util.Set;
 
 import org.openstreetmap.josm.data.osm.IPrimitive;
@@ -59 +62 @@
      */
     public Integer count;
+
+    /**
+     * Set of matched children filled by ContainsFinder and CrossingFinder, null if nothing matched
+     */
+    public Set<IPrimitive> children;
 
     /**
@@ -109 +117 @@
         this.count = other.count;
         this.context = other.getContext();
+        this.children = other.children == null ? null : new LinkedHashSet<>(other.children);
     }
 
@@ -274 +283 @@
         index = null;
         count = null;
+        children = null;
     }
 

trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java

@@ -7 +7 @@
 import java.util.Collection;
 import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.NoSuchElementException;
@@ -258 +259 @@
                 return !e.osm.equals(p) && p.isUsable();
             }
+
+            protected void addToChildren(Environment e, IPrimitive p) {
+                if (e.children == null) {
+                    e.children = new LinkedHashSet<>();
+                }
+                e.children.add(p);
+            }
         }
 
@@ -299 +307 @@
             @Override
             public void visit(IWay<?> w) {
-                if (e.child == null && Objects.equals(layer, OsmUtils.getLayer(w))
+                if (Objects.equals(layer, OsmUtils.getLayer(w))
                     && left.matches(new Environment(w).withParent(e.osm))
                     && e.osm instanceof IWay && Geometry.PolygonIntersection.CROSSING.equals(
                             Geometry.polygonIntersection(w.getNodes(), ((IWay<?>) e.osm).getNodes()))) {
-                    e.child = w;
+                    addToChildren(e, w);
                 }
             }
@@ -316 +324 @@
             @Override
             public void visit(INode n) {
-                if (e.child == null && left.matches(new Environment(n).withParent(e.osm))
+                if (left.matches(new Environment(n).withParent(e.osm))
                     && ((e.osm instanceof IWay && Geometry.nodeInsidePolygon(n, ((IWay<?>) e.osm).getNodes()))
                             || (e.osm instanceof Relation && (
                                     (Relation) e.osm).isMultipolygon() && Geometry.isNodeInsideMultiPolygon(n, (Relation) e.osm, null)))) {
-                    e.child = n;
+                    addToChildren(e, n);
                 }
             }
@@ -326 +334 @@
             @Override
             public void visit(IWay<?> w) {
-                if (e.child == null && left.matches(new Environment(w).withParent(e.osm))
+                if (left.matches(new Environment(w).withParent(e.osm))
                     && ((e.osm instanceof IWay && Geometry.PolygonIntersection.FIRST_INSIDE_SECOND.equals(
                             Geometry.polygonIntersection(w.getNodes(), ((IWay<?>) e.osm).getNodes())))
@@ -332 +340 @@
                                     (Relation) e.osm).isMultipolygon()
                                     && Geometry.isPolygonInsideMultiPolygon(w.getNodes(), (Relation) e.osm, null)))) {
-                    e.child = w;
+                    addToChildren(e, w);
                 }
             }
@@ -388 +396 @@
                 }
 
-                return e.child != null;
+                return e.children != null;
 
             } else if (ChildOrParentSelectorType.CROSSING == type && e.osm instanceof IWay) {
                 e.parent = e.osm;
-                final CrossingFinder crossingFinder = new CrossingFinder(e);
                 if (right instanceof OptimizedGeneralSelector
                         && ((OptimizedGeneralSelector) right).matchesBase(OsmPrimitiveType.WAY)) {
+                    final CrossingFinder crossingFinder = new CrossingFinder(e);
                     crossingFinder.visit(e.osm.getDataSet().searchWays(e.osm.getBBox()));
                 }
-                return e.child != null;
+                return e.children != null;
             } else if (ChildOrParentSelectorType.SIBLING == type) {
                 if (e.osm instanceof INode) {

trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java

@@ -16 +16 @@
 import java.util.Set;
 
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -87 +86 @@
         new DataSet(n1, n2);
         assertTrue(check.test(n1));
-        assertEquals("deprecated", check.getErrorForPrimitive(n1).getMessage());
-        assertEquals("natural=marsh is deprecated", check.getErrorForPrimitive(n1).getDescription());
-        assertEquals(Severity.WARNING, check.getErrorForPrimitive(n1).getSeverity());
+
+        final Collection<TestError> errors = check.getErrorsForPrimitive(n1);
+        assertEquals(1, errors.size());
+        TestError err = errors.iterator().next();
+        assertEquals("deprecated", err.getMessage());
+        assertEquals("natural=marsh is deprecated", err.getDescription());
+        assertEquals(Severity.WARNING, err.getSeverity());
         assertEquals("Sequence: Fix of natural=marsh is deprecated", check.fixPrimitive(n1).getDescriptionText());
         assertEquals("{natural=}", ((ChangePropertyCommand) check.fixPrimitive(n1).getChildren().iterator().next()).getTags().toString());
@@ -240 +243 @@
      */
     @Test
-    @Ignore("not fixed yet")
     public void testTicket14287() throws Exception {
         final MapCSSTagChecker test = buildTagChecker(
@@ -300 +302 @@
      */
     @Test
-    @Ignore("not fixed yet")
     public void testTicket12627() throws Exception {
         doTestNaturalWood(12627, "overlapping.osm", 1, 1);
@@ -310 +311 @@
      */
     @Test
-    @Ignore("not fixed yet")
     public void testTicket14289() throws Exception {
         doTestNaturalWood(14289, "example2.osm", 3, 3);