Changeset 18960 in josm


Ignore:
Timestamp:
2024-01-30T09:04:11+01:00 (8 months ago)
Author:
GerdP
Message:

fix #23397: Improve the results of partial validations

  • pass also parent ways and relations of uploaded/selected objects to the testers, child objects are already added, this allows to find e.g. overlapping polygons problems with tags in members of relations
  • let CrossingWays find a problem if at least one of the crossing ways is in the partial selection.
  • let DuplicatWays find a problem if at least one of the duplicated ways is in the partial selection
  • add code to filter the detected issues so that those issues which are clearly not related to the original list of objects are removed. A few issues from mapcss tests may remain.
  • add new preference validator.partial.add.parents to disable the addition of parent objects, default is enabled
  • add new preference validator.partial.removeIrrelevant to disable the filtering of irrelevant errors, default is enabled
  • duplicated code to call AggregatePrimitivesVisitor was moved to ValidationTask
Location:
trunk/src/org/openstreetmap/josm
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/actions/ValidateAction.java

    r17616 r18960  
    1313import org.openstreetmap.josm.data.validation.Test;
    1414import org.openstreetmap.josm.data.validation.ValidationTask;
    15 import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor;
    1615import org.openstreetmap.josm.gui.MainApplication;
    1716import org.openstreetmap.josm.gui.MapFrame;
    … …  
    7170                lastSelection = null;
    7271            } else {
    73                 AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor();
    74                 selection = v.visit(selection);
    7572                lastSelection = selection;
    7673            }

  • trunk/src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java

    r18776 r18960  
    77import java.awt.GridBagLayout;
    88import java.util.Collection;
     9import java.util.HashSet;
    910import java.util.List;
    1011import java.util.concurrent.atomic.AtomicBoolean;
    … …  
    1819import org.openstreetmap.josm.data.validation.TestError;
    1920import org.openstreetmap.josm.data.validation.ValidationTask;
    20 import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor;
    2121import org.openstreetmap.josm.gui.ExtendedDialog;
    2222import org.openstreetmap.josm.gui.MainApplication;
    … …  
    4646    public boolean checkUpload(APIDataSet apiDataSet) {
    4747        AtomicBoolean returnCode = new AtomicBoolean();
    48         AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor();
    49         v.visit(apiDataSet.getPrimitivesToAdd());
    50         Collection<OsmPrimitive> visited = v.visit(apiDataSet.getPrimitivesToUpdate());
     48        Collection<OsmPrimitive> toCheck = new HashSet<>();
     49        toCheck.addAll(apiDataSet.getPrimitivesToAdd());
     50        toCheck.addAll(apiDataSet.getPrimitivesToUpdate());
    5151        OsmValidator.initializeTests();
    5252        new ValidationTask(errors -> {
    … …  
    5959                GuiHelper.runInEDTAndWait(() -> returnCode.set(displayErrorScreen(errors)));
    6060            }
    61         }, null, OsmValidator.getEnabledTests(true), visited, null, true).run();
     61        }, null, OsmValidator.getEnabledTests(true), toCheck, null, true).run();
    6262
    6363        return returnCode.get();

  • trunk/src/org/openstreetmap/josm/data/preferences/sources/ValidatorPrefHelper.java

    r16296 r18960  
    5858
    5959    /**
     60     * See #23397
     61     * The preferences key for the addition of parent objects for modified objects
     62     */
     63    public static final BooleanProperty PREF_ADD_PARENTS = new BooleanProperty(PREFIX + ".partial.add.parents", true);
     64
     65    /**
     66     * See #23397
     67     * The preferences key for the deletion of results which do not belong to the selection
     68     * or the parents of modified objects.
     69     *
     70     */
     71    public static final BooleanProperty PREF_REMOVE_IRRELEVANT = new BooleanProperty(PREFIX + ".partial.removeIrrelevant", true);
     72
     73    /**
    6074     * Constructs a new {@code PresetPrefHelper}.
    6175     */

  • trunk/src/org/openstreetmap/josm/data/validation/Test.java

    r17732 r18960  
    77import java.util.ArrayList;
    88import java.util.Collection;
     9import java.util.HashSet;
    910import java.util.List;
    1011import java.util.Optional;
     12import java.util.Set;
    1113import java.util.function.Predicate;
    1214import java.util.stream.Collectors;
    … …  
    384386        return "Java: " + this.getClass().getName();
    385387    }
     388
     389    /**
     390     * Filter the list of errors, remove all which do not concern the given list of primitives
     391     * @param given the list of primitives
     392     * @since 18960
     393     */
     394    public void removeIrrelevantErrors(Collection<? extends OsmPrimitive> given) {
     395        if (errors == null || errors.isEmpty())
     396            return;
     397        // filter errors for those which are needed, don't show errors for objects which were not in the selection
     398        final Set<? extends OsmPrimitive> relevant;
     399        if (given instanceof Set) {
     400            relevant = (Set<? extends OsmPrimitive>) given;
     401        } else {
     402            relevant = new HashSet<>(given);
     403        }
     404        errors.removeIf(e -> !e.isConcerned(relevant));
     405    }
    386406}

  • trunk/src/org/openstreetmap/josm/data/validation/TestError.java

    r18637 r18960  
    1313import java.util.Locale;
    1414import java.util.Map;
     15import java.util.Set;
    1516import java.util.TreeSet;
    1617import java.util.function.Supplier;
    … …  
    6364    /** If this error is selected */
    6465    private boolean selected;
     66    /** If all relevant primitives are known*/
     67    private boolean incompletePrimitives;
    6568    /** Supplying a command to fix the error */
    6669    private final Supplier<Command> fixingCommand;
    … …  
    8184        private Collection<?> highlighted;
    8285        private Supplier<Command> fixingCommand;
     86        private boolean incompletePrimitives;
    8387
    8488        Builder(Test tester, Severity severity, int code) {
    … …  
    215219            CheckParameterUtil.ensureParameterNotNull(highlighted, "highlighted");
    216220            this.highlighted = Collections.singleton(highlighted);
     221            return this;
     222        }
     223
     224        /**
     225         * Sets a flag that the list of primitives may be incomplete. See #23397
     226         *
     227         * @return {@code this}
     228         */
     229        public Builder imcompletePrimitives() {
     230            this.incompletePrimitives = true;
    217231            return this;
    218232        }
    … …  
    277291        this.uniqueCode = builder.uniqueCode;
    278292        this.fixingCommand = builder.fixingCommand;
     293        this.incompletePrimitives = builder.incompletePrimitives;
    279294    }
    280295
    … …  
    667682    }
    668683
     684    /**
     685     * Check if any of the primitives in this error occurs in the given set of primitives.
     686     * @param given the set of primitives
     687     * @return true if any of the primitives in this error occurs in the given set of primitives, else false
     688     * @since 18960
     689     */
     690    public boolean isConcerned(Set<? extends OsmPrimitive> given) {
     691        if (incompletePrimitives)
     692            return true;
     693        for (OsmPrimitive p : getPrimitives()) {
     694            if (given.contains(p)) {
     695                return true;
     696            }
     697        }
     698        return false;
     699    }
    669700}

  • trunk/src/org/openstreetmap/josm/data/validation/ValidationTask.java

    r18850 r18960  
    77import java.util.ArrayList;
    88import java.util.Collection;
     9import java.util.Collections;
     10import java.util.HashSet;
    911import java.util.List;
     12import java.util.Set;
    1013import java.util.function.BiConsumer;
    1114import java.util.function.Consumer;
    … …  
    1417
    1518import org.openstreetmap.josm.data.osm.OsmPrimitive;
     19import org.openstreetmap.josm.data.osm.Way;
    1620import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
     21import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor;
    1722import org.openstreetmap.josm.gui.MainApplication;
    1823import org.openstreetmap.josm.gui.MapFrame;
    … …  
    3136    private final Consumer<List<TestError>> onFinish;
    3237    private Collection<Test> tests;
    33     private final Collection<OsmPrimitive> validatedPrimitives;
     38    private final Collection<OsmPrimitive> initialPrimitives;
    3439    private final Collection<OsmPrimitive> formerValidatedPrimitives;
    3540    private final boolean beforeUpload;
    … …  
    7277                false /*don't ignore exceptions */);
    7378        this.onFinish = onFinish;
    74         this.validatedPrimitives = validatedPrimitives;
     79        this.initialPrimitives = validatedPrimitives;
    7580        this.formerValidatedPrimitives = formerValidatedPrimitives;
    7681        this.tests = tests;
    … …  
    7883    }
    7984
     85    /**
     86     * Find objects parent objects of given objects which should be checked for geometry problems
     87     * or mismatches between child tags and parent tags.
     88     * @param primitives the given objects
     89     * @return the collection of relevant parent objects
     90     */
     91    private static Set<OsmPrimitive> getRelevantParents(Collection<OsmPrimitive> primitives) {
     92        Set<OsmPrimitive> addedWays = new HashSet<>();
     93        Set<OsmPrimitive> addedRelations = new HashSet<>();
     94        for (OsmPrimitive p : primitives) {
     95            for (OsmPrimitive parent : p.getReferrers()) {
     96                if (parent.isDeleted())
     97                    continue;
     98                if (parent instanceof Way)
     99                    addedWays.add(parent);
     100                else
     101                    addedRelations.add(parent);
     102            }
     103        }
     104
     105        // allow to find invalid multipolygon relations caused by moved nodes
     106        OsmPrimitive.getParentRelations(addedWays).stream().filter(r -> r.isMultipolygon() && !r.isDeleted())
     107                .forEach(addedRelations::add);
     108        HashSet<OsmPrimitive> extendedSet = new HashSet<>();
     109        extendedSet.addAll(addedWays);
     110        extendedSet.addAll(addedRelations);
     111        return extendedSet;
     112
     113    }
    80114    protected ValidationTask(ProgressMonitor progressMonitor,
    81115            Collection<Test> tests,
    … …  
    123157        if (Utils.isEmpty(tests))
    124158            return;
     159        int testCounter = 0;
     160        final boolean isPartial = this.beforeUpload || formerValidatedPrimitives != null;
     161        Set<OsmPrimitive> filter = null;
     162        Collection<OsmPrimitive> validatedPrimitives = initialPrimitives;
     163        if (isPartial) {
     164            Set<OsmPrimitive> other = Collections.emptySet();
     165            if (Boolean.TRUE.equals(ValidatorPrefHelper.PREF_ADD_PARENTS.get())) {
     166                other = getRelevantParents(initialPrimitives);
     167            }
     168            HashSet<OsmPrimitive> extendedSet = new HashSet<>();
     169            AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor();
     170            extendedSet.addAll(v.visit(initialPrimitives));
     171            extendedSet.addAll(other);
     172            validatedPrimitives = extendedSet;
     173            filter = new HashSet<>(initialPrimitives);
     174            filter.addAll(other);
     175        }
    125176        getProgressMonitor().setTicksCount(tests.size() * validatedPrimitives.size());
    126         int testCounter = 0;
     177
    127178        for (Test test : tests) {
    128179            if (canceled)
    … …  
    132183            test.setBeforeUpload(this.beforeUpload);
    133184            // Pre-upload checks only run on a partial selection.
    134             test.setPartialSelection(this.beforeUpload || formerValidatedPrimitives != null);
     185            test.setPartialSelection(isPartial);
    135186            test.startTest(getProgressMonitor().createSubTaskMonitor(validatedPrimitives.size(), false));
    136187            test.visit(validatedPrimitives);
    137188            test.endTest();
     189            if (isPartial && Boolean.TRUE.equals(ValidatorPrefHelper.PREF_REMOVE_IRRELEVANT.get())) {
     190                // #23397: remove errors for objects which were not in the initial list of primitives
     191                test.removeIrrelevantErrors(filter);
     192            }
     193
    138194            errors.addAll(test.getErrors());
    139195            if (this.testConsumer != null) {

  • trunk/src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java

    r18870 r18960  
    77import java.util.ArrayList;
    88import java.util.Arrays;
     9import java.util.Collection;
    910import java.util.HashMap;
    1011import java.util.HashSet;
    … …  
    1718import org.openstreetmap.josm.data.coor.EastNorth;
    1819import org.openstreetmap.josm.data.coor.ILatLon;
     20import org.openstreetmap.josm.data.osm.DataSet;
     21import org.openstreetmap.josm.data.osm.OsmDataManager;
    1922import org.openstreetmap.josm.data.osm.OsmPrimitive;
    2023import org.openstreetmap.josm.data.osm.OsmUtils;
    … …  
    8285    /** The already detected ways in error */
    8386    private final Map<List<Way>, List<WaySegment>> seenWays = new HashMap<>(50);
     87    private final Set<Way> waysToTest = new HashSet<>();
    8488
    8589    protected final int code;
    … …  
    306310    @Override
    307311    public void endTest() {
    308         super.endTest();
     312        final Collection<Way> selection;
     313        if (this instanceof SelfCrossing || !partialSelection) {
     314            selection = waysToTest;
     315        } else {
     316            selection = new HashSet<>();
     317            DataSet ds = OsmDataManager.getInstance().getActiveDataSet();
     318            if (ds != null) {
     319                for (Way w: waysToTest) {
     320                    selection.addAll(ds.searchWays(w.getBBox()));
     321                }
     322            }
     323        }
     324        for (Way w : selection) {
     325            if (!w.isDeleted() && isPrimitiveUsable(w)) {
     326                testWay(w);
     327            }
     328        }
     329        // free storage
    309330        cellSegments.clear();
    310331        seenWays.clear();
     332        waysToTest.clear();
     333        if (partialSelection)
     334            removeIrrelevantErrors(waysToTest);
     335        super.endTest();
    311336    }
    312337
    … …  
    345370    @Override
    346371    public void visit(Way w) {
     372        waysToTest.add(w);
     373    }
     374
     375    private void testWay(Way w) {
    347376        boolean findSelfCrossingOnly = this instanceof SelfCrossing;
    348377        if (findSelfCrossingOnly) {
    … …  
    483512        SelfCrossing test = new SelfCrossing();
    484513        test.visit(way);
     514        test.endTest();
    485515        return !test.getErrors().isEmpty();
    486516    }

  • trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateWay.java

    r17243 r18960  
    88import java.util.Collections;
    99import java.util.HashSet;
     10import java.util.LinkedHashSet;
    1011import java.util.LinkedList;
    1112import java.util.List;
    … …  
    104105    /** Set of known hashcodes for list of coordinates **/
    105106    private Set<Integer> knownHashCodes;
     107    private List<Way> waysToCheck;
    106108
    107109    /**
    … …  
    116118    public void startTest(ProgressMonitor monitor) {
    117119        super.startTest(monitor);
     120        waysToCheck = new ArrayList<>();
    118121        ways = new MultiMap<>(1000);
    119122        waysNoTags = new MultiMap<>(1000);
    … …  
    123126    @Override
    124127    public void endTest() {
    125         super.endTest();
     128        if (partialSelection && !waysToCheck.isEmpty()) {
     129            // make sure that we have the error candidates even if not selected
     130            Set<Way> extended = new LinkedHashSet<>(waysToCheck);
     131            for (Way w : waysToCheck) {
     132                // select a node, anyone can be used but a middle node is less likely to have many parent ways
     133                final Node n = w.getNode(w.getNodesCount()/2);
     134                // check the ways which might be in the same position
     135                for (Way other : n.getParentWays()) {
     136                    if (other != w && !other.isDeleted() && other.isUsable()
     137                            && other.getNodesCount() == w.getNodesCount())
     138                        extended.add(other);
     139                }
     140            }
     141            extended.forEach(this::checkWay);
     142        }
     143
    126144        for (Set<OsmPrimitive> duplicated : ways.values()) {
    127145            if (duplicated.size() > 1) {
    … …  
    166184        waysNoTags = null;
    167185        knownHashCodes = null;
     186        waysToCheck = null;
     187        super.endTest();
    168188    }
    169189
    … …  
    182202        if (!w.isUsable())
    183203            return;
     204        if (partialSelection)
     205            waysToCheck.add(w);
     206        else
     207            checkWay(w);
     208    }
     209
     210    private void checkWay(Way w) {
    184211        List<LatLon> wLat = getOrderedNodes(w);
    185212        // If this way has not direction-dependant keys, make sure the list is ordered the same for all ways (fix #8015)

  • trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerRule.java

    r18762 r18960  
    3434import org.openstreetmap.josm.gui.mappaint.MultiCascade;
    3535import org.openstreetmap.josm.gui.mappaint.mapcss.Condition;
     36import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.ClassCondition;
    3637import org.openstreetmap.josm.gui.mappaint.mapcss.Expression;
    3738import org.openstreetmap.josm.gui.mappaint.mapcss.Instruction;
    … …  
    378379                    }
    379380                }
     381            } else if (env.parent != null) {
     382                boolean imcompletePrimitives = false;
     383                if (matchingSelector instanceof Selector.ChildOrParentSelector) {
     384                    Selector right = ((Selector.ChildOrParentSelector) matchingSelector).right;
     385                    if (right.getConditions().stream().anyMatch(ClassCondition.class::isInstance)) {
     386                        // see #23397
     387                        // TODO: find a way to collect all and only those parent objects which triggered this error
     388                        imcompletePrimitives = true;
     389                    }
     390                }
     391                if (imcompletePrimitives)
     392                    res.add(errorBuilder.primitives(p).highlight(p).imcompletePrimitives().build());
     393                else
     394                    res.add(errorBuilder.primitives(p, (OsmPrimitive) env.parent).highlight(p).build());
    380395            } else {
    381396                res.add(errorBuilder.primitives(p).build());

  • trunk/src/org/openstreetmap/josm/data/validation/util/AggregatePrimitivesVisitor.java

    r12809 r18960  
    1515 * A visitor that aggregates all primitives it visits.
    1616 * <p>
    17  * The primitives are sorted according to their type: first nodes, then ways.
    1817 *
    1918 * @author frsantos
    … …  
    2524    /**
    2625     * Visits a collection of primitives
    27      * @param data The collection of primitives
     26     * @param data The collection of primitives in no specific order.
    2827     * @return The aggregated primitives
    2928     */
Note: See TracChangeset for help on using the changeset viewer.