Changeset 15064 in josm


Ignore:
Timestamp:
2019-05-09T10:13:10+02:00 (7 months 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:
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java

    r15027 r15064  
    1313import java.text.MessageFormat;
    1414import java.util.ArrayList;
    15 import java.util.Arrays;
    1615import java.util.Collection;
    1716import java.util.Collections;
     
    4039import org.openstreetmap.josm.data.osm.DataSet;
    4140import org.openstreetmap.josm.data.osm.INode;
     41import org.openstreetmap.josm.data.osm.IPrimitive;
    4242import org.openstreetmap.josm.data.osm.IRelation;
    4343import org.openstreetmap.josm.data.osm.IWay;
     
    7070import org.openstreetmap.josm.gui.mappaint.mapcss.Selector;
    7171import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.AbstractSelector;
     72import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.ChildOrParentSelector;
     73import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.ChildOrParentSelectorType;
    7274import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.GeneralSelector;
    7375import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.OptimizedGeneralSelector;
     
    734736         * @return an instance of {@link TestError}, or returns null if the primitive does not give rise to an error.
    735737         */
    736         TestError getErrorForPrimitive(OsmPrimitive p) {
     738        List<TestError> getErrorsForPrimitive(OsmPrimitive p) {
    737739            final Environment env = new Environment(p);
    738             return getErrorForPrimitive(p, whichSelectorMatchesEnvironment(env), env, null);
    739         }
    740 
    741         TestError getErrorForPrimitive(OsmPrimitive p, Selector matchingSelector, Environment env, Test tester) {
     740            return getErrorsForPrimitive(p, whichSelectorMatchesEnvironment(env), env, null);
     741        }
     742
     743        private List<TestError> getErrorsForPrimitive(OsmPrimitive p, Selector matchingSelector, Environment env, Test tester) {
     744            List<TestError> res = new ArrayList<>();
    742745            if (matchingSelector != null && !errors.isEmpty()) {
    743746                final Command fix = fixPrimitive(p);
     
    745748                final String description1 = group == null ? description : group;
    746749                final String description2 = group == null ? null : description;
    747                 final List<OsmPrimitive> primitives;
     750                TestError.Builder errorBuilder = TestError.builder(tester, getSeverity(), 3000)
     751                        .messageWithManuallyTranslatedDescription(description1, description2, matchingSelector.toString());
     752                if (fix != null) {
     753                    errorBuilder = errorBuilder.fix(() -> fix);
     754                }
    748755                if (env.child instanceof OsmPrimitive) {
    749                     primitives = Arrays.asList(p, (OsmPrimitive) env.child);
     756                    res.add(errorBuilder.primitives(p, (OsmPrimitive) env.child).build());
     757                } else if (env.children != null) {
     758                    for (IPrimitive c : env.children) {
     759                        if (c instanceof OsmPrimitive) {
     760                            errorBuilder = TestError.builder(tester, getSeverity(), 3000)
     761                                    .messageWithManuallyTranslatedDescription(description1, description2,
     762                                            matchingSelector.toString());
     763                            if (fix != null) {
     764                                errorBuilder = errorBuilder.fix(() -> fix);
     765                            }
     766                            res.add(errorBuilder.primitives(p, (OsmPrimitive) c).build());
     767                        }
     768                    }
    750769                } else {
    751                     primitives = Collections.singletonList(p);
    752                 }
    753                 final TestError.Builder error = TestError.builder(tester, getSeverity(), 3000)
    754                         .messageWithManuallyTranslatedDescription(description1, description2, matchingSelector.toString())
    755                         .primitives(primitives);
    756                 if (fix != null) {
    757                     return error.fix(() -> fix).build();
    758                 } else {
    759                     return error.build();
    760                 }
    761             } else {
    762                 return null;
    763             }
     770                    res.add(errorBuilder.primitives(p).build());
     771                }
     772            }
     773            return res;
    764774        }
    765775
     
    855865            MapCSSRule r = candidates.next();
    856866            env.clearSelectorMatchingInformation();
     867            if (partialSelection && r.selector instanceof Selector.ChildOrParentSelector) {
     868                ChildOrParentSelector sel = (Selector.ChildOrParentSelector) r.selector;
     869                if (sel.type == ChildOrParentSelectorType.ELEMENT_OF && p.getDataSet() != null) {
     870                    List<OsmPrimitive> toCheck = new ArrayList<>();
     871                    toCheck.addAll(p.getDataSet().searchWays(p.getBBox()));
     872                    toCheck.addAll(p.getDataSet().searchRelations(p.getBBox()));
     873                    toCheck.removeIf(OsmPrimitive::isSelected);
     874                    if (!toCheck.isEmpty()) {
     875                        Set<Set<TagCheck>> checksCol = Collections.singleton(Collections.singleton(indexData.getCheck(r)));
     876                        for (OsmPrimitive p2 : toCheck) {
     877                            for (TestError e : getErrorsForPrimitive(p2, includeOtherSeverity, checksCol)) {
     878                                if (e.getPrimitives().contains(p)) {
     879                                    addIfNotSimilar(e, res);
     880                                }
     881                            }
     882                        }
     883                    }
     884                }
     885            }
    857886            if (r.selector.matches(env)) { // as side effect env.parent will be set (if s is a child selector)
    858887                TagCheck check = indexData.getCheck(r);
     
    864893                    r.declaration.execute(env);
    865894                    if (!check.errors.isEmpty()) {
    866                         final TestError error = check.getErrorForPrimitive(p, r.selector, env, new MapCSSTagCheckerAndRule(check.rule));
    867                         if (error != null) {
    868                             res.add(error);
     895                        for (TestError e: check.getErrorsForPrimitive(p, r.selector, env, new MapCSSTagCheckerAndRule(check.rule))) {
     896                            addIfNotSimilar(e, res);
    869897                        }
    870898                    }
    871 
    872899                }
    873900            }
    874901        }
    875902        return res;
     903    }
     904
     905    /**
     906     * See #12627
     907     * Add error to given list if list doesn't already contain a similar error.
     908     * Similar means same code and description and same combination of primitives and same combination of highlighted objects,
     909     * but maybe with different orders.
     910     * @param toAdd the error to add
     911     * @param errors the list of errors
     912     */
     913    private static void addIfNotSimilar(TestError toAdd, List<TestError> errors) {
     914        boolean isDup = false;
     915        if (toAdd.getPrimitives().size() >= 2) {
     916            for (TestError e : errors) {
     917                if (e.getCode() == toAdd.getCode() && e.getMessage().equals(toAdd.getMessage())
     918                        && e.getPrimitives().size() == toAdd.getPrimitives().size()
     919                        && e.getPrimitives().containsAll(toAdd.getPrimitives())
     920                        && e.getHighlighted().size() == toAdd.getHighlighted().size()
     921                        && e.getHighlighted().containsAll(toAdd.getHighlighted())) {
     922                    isDup = true;
     923                    break;
     924                }
     925            }
     926        }
     927        if (!isDup)
     928            errors.add(toAdd);
    876929    }
    877930
     
    891944                    check.rule.declaration.execute(env);
    892945                    if (!ignoreError && !check.errors.isEmpty()) {
    893                         final TestError error = check.getErrorForPrimitive(p, selector, env, new MapCSSTagCheckerAndRule(check.rule));
    894                         if (error != null) {
    895                             r.add(error);
    896                         }
     946                        r.addAll(check.getErrorsForPrimitive(p, selector, env, new MapCSSTagCheckerAndRule(check.rule)));
    897947                    }
    898948                }
     
    909959    @Override
    910960    public void check(OsmPrimitive p) {
    911         errors.addAll(getErrorsForPrimitive(p, ValidatorPrefHelper.PREF_OTHER.get()));
     961        for (TestError e : getErrorsForPrimitive(p, ValidatorPrefHelper.PREF_OTHER.get())) {
     962            addIfNotSimilar(e, errors);
     963        }
    912964    }
    913965
  • trunk/src/org/openstreetmap/josm/gui/mappaint/Environment.java

    r14214 r15064  
    11// License: GPL. For details, see LICENSE file.
    22package org.openstreetmap.josm.gui.mappaint;
     3
     4import java.util.LinkedHashSet;
     5import java.util.Set;
    36
    47import org.openstreetmap.josm.data.osm.IPrimitive;
     
    5962     */
    6063    public Integer count;
     64
     65    /**
     66     * Set of matched children filled by ContainsFinder and CrossingFinder, null if nothing matched
     67     */
     68    public Set<IPrimitive> children;
    6169
    6270    /**
     
    109117        this.count = other.count;
    110118        this.context = other.getContext();
     119        this.children = other.children == null ? null : new LinkedHashSet<>(other.children);
    111120    }
    112121
     
    274283        index = null;
    275284        count = null;
     285        children = null;
    276286    }
    277287
  • trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java

    r14654 r15064  
    77import java.util.Collection;
    88import java.util.Collections;
     9import java.util.LinkedHashSet;
    910import java.util.List;
    1011import java.util.NoSuchElementException;
     
    258259                return !e.osm.equals(p) && p.isUsable();
    259260            }
     261
     262            protected void addToChildren(Environment e, IPrimitive p) {
     263                if (e.children == null) {
     264                    e.children = new LinkedHashSet<>();
     265                }
     266                e.children.add(p);
     267            }
    260268        }
    261269
     
    299307            @Override
    300308            public void visit(IWay<?> w) {
    301                 if (e.child == null && Objects.equals(layer, OsmUtils.getLayer(w))
     309                if (Objects.equals(layer, OsmUtils.getLayer(w))
    302310                    && left.matches(new Environment(w).withParent(e.osm))
    303311                    && e.osm instanceof IWay && Geometry.PolygonIntersection.CROSSING.equals(
    304312                            Geometry.polygonIntersection(w.getNodes(), ((IWay<?>) e.osm).getNodes()))) {
    305                     e.child = w;
     313                    addToChildren(e, w);
    306314                }
    307315            }
     
    316324            @Override
    317325            public void visit(INode n) {
    318                 if (e.child == null && left.matches(new Environment(n).withParent(e.osm))
     326                if (left.matches(new Environment(n).withParent(e.osm))
    319327                    && ((e.osm instanceof IWay && Geometry.nodeInsidePolygon(n, ((IWay<?>) e.osm).getNodes()))
    320328                            || (e.osm instanceof Relation && (
    321329                                    (Relation) e.osm).isMultipolygon() && Geometry.isNodeInsideMultiPolygon(n, (Relation) e.osm, null)))) {
    322                     e.child = n;
     330                    addToChildren(e, n);
    323331                }
    324332            }
     
    326334            @Override
    327335            public void visit(IWay<?> w) {
    328                 if (e.child == null && left.matches(new Environment(w).withParent(e.osm))
     336                if (left.matches(new Environment(w).withParent(e.osm))
    329337                    && ((e.osm instanceof IWay && Geometry.PolygonIntersection.FIRST_INSIDE_SECOND.equals(
    330338                            Geometry.polygonIntersection(w.getNodes(), ((IWay<?>) e.osm).getNodes())))
     
    332340                                    (Relation) e.osm).isMultipolygon()
    333341                                    && Geometry.isPolygonInsideMultiPolygon(w.getNodes(), (Relation) e.osm, null)))) {
    334                     e.child = w;
     342                    addToChildren(e, w);
    335343                }
    336344            }
     
    388396                }
    389397
    390                 return e.child != null;
     398                return e.children != null;
    391399
    392400            } else if (ChildOrParentSelectorType.CROSSING == type && e.osm instanceof IWay) {
    393401                e.parent = e.osm;
    394                 final CrossingFinder crossingFinder = new CrossingFinder(e);
    395402                if (right instanceof OptimizedGeneralSelector
    396403                        && ((OptimizedGeneralSelector) right).matchesBase(OsmPrimitiveType.WAY)) {
     404                    final CrossingFinder crossingFinder = new CrossingFinder(e);
    397405                    crossingFinder.visit(e.osm.getDataSet().searchWays(e.osm.getBBox()));
    398406                }
    399                 return e.child != null;
     407                return e.children != null;
    400408            } else if (ChildOrParentSelectorType.SIBLING == type) {
    401409                if (e.osm instanceof INode) {
  • trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java

    r14801 r15064  
    1616import java.util.Set;
    1717
    18 import org.junit.Ignore;
    1918import org.junit.Rule;
    2019import org.junit.Test;
     
    8786        new DataSet(n1, n2);
    8887        assertTrue(check.test(n1));
    89         assertEquals("deprecated", check.getErrorForPrimitive(n1).getMessage());
    90         assertEquals("natural=marsh is deprecated", check.getErrorForPrimitive(n1).getDescription());
    91         assertEquals(Severity.WARNING, check.getErrorForPrimitive(n1).getSeverity());
     88
     89        final Collection<TestError> errors = check.getErrorsForPrimitive(n1);
     90        assertEquals(1, errors.size());
     91        TestError err = errors.iterator().next();
     92        assertEquals("deprecated", err.getMessage());
     93        assertEquals("natural=marsh is deprecated", err.getDescription());
     94        assertEquals(Severity.WARNING, err.getSeverity());
    9295        assertEquals("Sequence: Fix of natural=marsh is deprecated", check.fixPrimitive(n1).getDescriptionText());
    9396        assertEquals("{natural=}", ((ChangePropertyCommand) check.fixPrimitive(n1).getChildren().iterator().next()).getTags().toString());
     
    240243     */
    241244    @Test
    242     @Ignore("not fixed yet")
    243245    public void testTicket14287() throws Exception {
    244246        final MapCSSTagChecker test = buildTagChecker(
     
    300302     */
    301303    @Test
    302     @Ignore("not fixed yet")
    303304    public void testTicket12627() throws Exception {
    304305        doTestNaturalWood(12627, "overlapping.osm", 1, 1);
     
    310311     */
    311312    @Test
    312     @Ignore("not fixed yet")
    313313    public void testTicket14289() throws Exception {
    314314        doTestNaturalWood(14289, "example2.osm", 3, 3);
Note: See TracChangeset for help on using the changeset viewer.