Index: src/org/openstreetmap/josm/actions/ValidateAction.java =================================================================== --- src/org/openstreetmap/josm/actions/ValidateAction.java (revision 18957) +++ src/org/openstreetmap/josm/actions/ValidateAction.java (working copy) @@ -12,7 +12,6 @@ import org.openstreetmap.josm.data.validation.OsmValidator; import org.openstreetmap.josm.data.validation.Test; import org.openstreetmap.josm.data.validation.ValidationTask; -import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.MapFrame; import org.openstreetmap.josm.tools.Shortcut; @@ -70,8 +69,6 @@ selection = getLayerManager().getActiveDataSet().allNonDeletedPrimitives(); lastSelection = null; } else { - AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor(); - selection = v.visit(selection); lastSelection = selection; } } else { Index: src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java =================================================================== --- src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java (revision 18957) +++ src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java (working copy) @@ -6,6 +6,7 @@ import java.awt.Dimension; import java.awt.GridBagLayout; import java.util.Collection; +import java.util.HashSet; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; @@ -17,7 +18,6 @@ import org.openstreetmap.josm.data.validation.OsmValidator; import org.openstreetmap.josm.data.validation.TestError; import org.openstreetmap.josm.data.validation.ValidationTask; -import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor; import org.openstreetmap.josm.gui.ExtendedDialog; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.dialogs.validator.ValidatorTreePanel; @@ -45,9 +45,9 @@ @Override public boolean checkUpload(APIDataSet apiDataSet) { AtomicBoolean returnCode = new AtomicBoolean(); - AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor(); - v.visit(apiDataSet.getPrimitivesToAdd()); - Collection visited = v.visit(apiDataSet.getPrimitivesToUpdate()); + Collection toCheck = new HashSet<>(); + toCheck.addAll(apiDataSet.getPrimitivesToAdd()); + toCheck.addAll(apiDataSet.getPrimitivesToUpdate()); OsmValidator.initializeTests(); new ValidationTask(errors -> { if (errors.stream().allMatch(TestError::isIgnored)) { @@ -58,7 +58,7 @@ // of the progress monitor. GuiHelper.runInEDTAndWait(() -> returnCode.set(displayErrorScreen(errors))); } - }, null, OsmValidator.getEnabledTests(true), visited, null, true).run(); + }, null, OsmValidator.getEnabledTests(true), toCheck, null, true).run(); return returnCode.get(); } Index: src/org/openstreetmap/josm/data/validation/Test.java =================================================================== --- src/org/openstreetmap/josm/data/validation/Test.java (revision 18957) +++ src/org/openstreetmap/josm/data/validation/Test.java (working copy) @@ -6,8 +6,10 @@ import java.awt.GridBagConstraints; import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -383,4 +385,22 @@ public Object getSource() { return "Java: " + this.getClass().getName(); } + + /** + * Filter the list of errors, remove all which do not concern the given list of primitives + * @param given the list of primitives + * @since xxx + */ + public void removeIrrelevantErrors(Collection given) { + if (errors == null || errors.isEmpty()) + return; + // filter errors for those which are needed, don't show errors for objects which were not in the selection + final Set relevant; + if (given instanceof Set) { + relevant = (Set) given; + } else { + relevant = new HashSet<>(given); + } + errors.removeIf(e -> !e.isConcerned(relevant)); + } } Index: src/org/openstreetmap/josm/data/validation/TestError.java =================================================================== --- src/org/openstreetmap/josm/data/validation/TestError.java (revision 18957) +++ src/org/openstreetmap/josm/data/validation/TestError.java (working copy) @@ -12,6 +12,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.TreeSet; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -62,6 +63,8 @@ private final int uniqueCode; /** If this error is selected */ private boolean selected; + /** If all relevant primitives are known*/ + private boolean incompletePrimitives; /** Supplying a command to fix the error */ private final Supplier fixingCommand; @@ -80,6 +83,7 @@ private Collection primitives; private Collection highlighted; private Supplier fixingCommand; + private boolean incompletePrimitives; Builder(Test tester, Severity severity, int code) { this.tester = tester; @@ -218,6 +222,16 @@ } /** + * Sets a flag that the list of primitives may be incomplete. See #23397 + * + * @return {@code this} + */ + public Builder imcompletePrimitives() { + this.incompletePrimitives = true; + return this; + } + + /** * Sets a supplier to obtain a command to fix the error. * * @param fixingCommand the fix supplier. Can be null @@ -276,6 +290,7 @@ this.code = builder.code; this.uniqueCode = builder.uniqueCode; this.fixingCommand = builder.fixingCommand; + this.incompletePrimitives = builder.incompletePrimitives; } /** @@ -666,4 +681,20 @@ ", code=" + code + ", message=" + message + ']'; } + /** + * Check if any of the primitives in this error occurs in the given set of primitives. + * @param given the set of primitives + * @return true if any of the primitives in this error occurs in the given set of primitives, else false + * @since xxx + */ + public boolean isConcerned(Set given) { + if (incompletePrimitives) + return true; + for (OsmPrimitive p : getPrimitives()) { + if (given.contains(p)) { + return true; + } + } + return false; + } } Index: src/org/openstreetmap/josm/data/validation/ValidationTask.java =================================================================== --- src/org/openstreetmap/josm/data/validation/ValidationTask.java (revision 18957) +++ src/org/openstreetmap/josm/data/validation/ValidationTask.java (working copy) @@ -6,14 +6,19 @@ import java.awt.GraphicsEnvironment; import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.function.BiConsumer; import java.util.function.Consumer; import javax.swing.JOptionPane; +import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.OsmPrimitive; +import org.openstreetmap.josm.data.osm.Way; import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; +import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.MapFrame; import org.openstreetmap.josm.gui.Notification; @@ -30,7 +35,7 @@ public class ValidationTask extends PleaseWaitRunnable { private final Consumer> onFinish; private Collection tests; - private final Collection validatedPrimitives; + private final Collection initialPrimitives; private final Collection formerValidatedPrimitives; private final boolean beforeUpload; private boolean canceled; @@ -71,12 +76,41 @@ progressMonitor != null ? progressMonitor : new PleaseWaitProgressMonitor(tr("Validating")), false /*don't ignore exceptions */); this.onFinish = onFinish; - this.validatedPrimitives = validatedPrimitives; + this.initialPrimitives = validatedPrimitives; this.formerValidatedPrimitives = formerValidatedPrimitives; this.tests = tests; this.beforeUpload = beforeUpload; } + /** + * Find objects parent objects of given objects which should be checked for changed geometry. + * @param primitives the given objects + * @return a collection of objects with + */ + private static Set getPotentiallyModified(Collection primitives) { + Set addedWays = new HashSet<>(); + Set addedRelations = new HashSet<>(); + for (OsmPrimitive p : primitives) { + if (!(p instanceof Node && p.isModified() && !p.isNew())) + continue; + for (OsmPrimitive parent : p.getReferrers()) { + if (parent.isDeleted() || parent.isModified() || parent.isNew()) + continue; + if (parent instanceof Way) + addedWays.add(parent); + else + addedRelations.add(parent); + } + } + // allow to find invalid multipolygon relations caused by moved nodes + OsmPrimitive.getParentRelations(addedWays).stream().filter(r -> r.isMultipolygon()) + .forEach(addedRelations::add); + HashSet extendedSet = new HashSet<>(); + extendedSet.addAll(addedWays); + extendedSet.addAll(addedRelations); + return extendedSet; + + } protected ValidationTask(ProgressMonitor progressMonitor, Collection tests, Collection validatedPrimitives, @@ -122,8 +156,22 @@ protected void realRun() { if (Utils.isEmpty(tests)) return; + int testCounter = 0; + final boolean isPartial = this.beforeUpload || formerValidatedPrimitives != null; + Set filter = null; + Collection validatedPrimitives = initialPrimitives; + if (isPartial) { + Set other = getPotentiallyModified(initialPrimitives); + HashSet extendedSet = new HashSet<>(); + AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor(); + extendedSet.addAll(v.visit(initialPrimitives)); + extendedSet.addAll(other); + validatedPrimitives = extendedSet; + filter = new HashSet<>(initialPrimitives); + filter.addAll(other); + } getProgressMonitor().setTicksCount(tests.size() * validatedPrimitives.size()); - int testCounter = 0; + for (Test test : tests) { if (canceled) return; @@ -131,10 +179,15 @@ getProgressMonitor().setCustomText(tr("Test {0}/{1}: Starting {2}", testCounter, tests.size(), test.getName())); test.setBeforeUpload(this.beforeUpload); // Pre-upload checks only run on a partial selection. - test.setPartialSelection(this.beforeUpload || formerValidatedPrimitives != null); + test.setPartialSelection(isPartial); test.startTest(getProgressMonitor().createSubTaskMonitor(validatedPrimitives.size(), false)); test.visit(validatedPrimitives); test.endTest(); + if (isPartial) { + // #23397: remove errors for objects which were not in the initial list of primitives + test.removeIrrelevantErrors(filter); + } + errors.addAll(test.getErrors()); if (this.testConsumer != null) { this.testConsumer.accept(this, test); Index: src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (revision 18957) +++ src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (working copy) @@ -6,6 +6,7 @@ import java.awt.geom.Point2D; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -16,6 +17,8 @@ import org.openstreetmap.josm.data.coor.EastNorth; import org.openstreetmap.josm.data.coor.ILatLon; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.OsmDataManager; import org.openstreetmap.josm.data.osm.OsmPrimitive; import org.openstreetmap.josm.data.osm.OsmUtils; import org.openstreetmap.josm.data.osm.Relation; @@ -81,6 +84,7 @@ private final Map> cellSegments = new HashMap<>(1000); /** The already detected ways in error */ private final Map, List> seenWays = new HashMap<>(50); + private final Set waysToTest = new HashSet<>(); protected final int code; @@ -305,9 +309,28 @@ @Override public void endTest() { - super.endTest(); + final Collection selection; + if (this instanceof SelfCrossing || !partialSelection) { + selection = waysToTest; + } else { + selection = new HashSet<>(); + DataSet ds = OsmDataManager.getInstance().getActiveDataSet(); + if (ds != null) { + for (Way w: waysToTest) { + selection.addAll(ds.searchWays(w.getBBox())); + } + } + } + for (Way w : selection) { + if (!w.isDeleted() && isPrimitiveUsable(w)) { + testWay(w); + } + } + // free storage cellSegments.clear(); seenWays.clear(); + waysToTest.clear(); + super.endTest(); } static boolean isCoastline(OsmPrimitive w) { @@ -344,6 +367,10 @@ @Override public void visit(Way w) { + waysToTest.add(w); + } + + private void testWay(Way w) { boolean findSelfCrossingOnly = this instanceof SelfCrossing; if (findSelfCrossingOnly) { // free memory, we are not interested in previous ways @@ -482,6 +509,7 @@ CheckParameterUtil.ensureParameterNotNull(way, "way"); SelfCrossing test = new SelfCrossing(); test.visit(way); + test.endTest(); return !test.getErrors().isEmpty(); } } Index: src/org/openstreetmap/josm/data/validation/tests/DuplicateWay.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/DuplicateWay.java (revision 18957) +++ src/org/openstreetmap/josm/data/validation/tests/DuplicateWay.java (working copy) @@ -7,6 +7,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -103,6 +104,7 @@ /** Set of known hashcodes for list of coordinates **/ private Set knownHashCodes; + private List waysToCheck; /** * Constructor @@ -115,6 +117,7 @@ @Override public void startTest(ProgressMonitor monitor) { super.startTest(monitor); + waysToCheck = new ArrayList<>(); ways = new MultiMap<>(1000); waysNoTags = new MultiMap<>(1000); knownHashCodes = new HashSet<>(1000); @@ -122,7 +125,22 @@ @Override public void endTest() { - super.endTest(); + if (partialSelection && !waysToCheck.isEmpty()) { + // make sure that we have the error candidates even if not selected + Set extended = new LinkedHashSet<>(waysToCheck); + for (Way w : waysToCheck) { + // select a node, anyone can be used but a middle node is less likely to have many parent ways + final Node n = w.getNode(w.getNodesCount()/2); + // check the ways which might be in the same position + for (Way other : n.getParentWays()) { + if (other != w && !other.isDeleted() && other.isUsable() + && other.getNodesCount() == w.getNodesCount()) + extended.add(other); + } + } + extended.forEach(this::checkWay); + } + for (Set duplicated : ways.values()) { if (duplicated.size() > 1) { TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_WAY) @@ -165,6 +183,8 @@ ways = null; waysNoTags = null; knownHashCodes = null; + waysToCheck = null; + super.endTest(); } /** @@ -181,6 +201,13 @@ public void visit(Way w) { if (!w.isUsable()) return; + if (partialSelection) + waysToCheck.add(w); + else + checkWay(w); + } + + private void checkWay(Way w) { List wLat = getOrderedNodes(w); // If this way has not direction-dependant keys, make sure the list is ordered the same for all ways (fix #8015) if (!w.hasDirectionKeys()) { Index: src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerRule.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerRule.java (revision 18957) +++ src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerRule.java (working copy) @@ -33,6 +33,7 @@ import org.openstreetmap.josm.gui.mappaint.Keyword; import org.openstreetmap.josm.gui.mappaint.MultiCascade; import org.openstreetmap.josm.gui.mappaint.mapcss.Condition; +import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.ClassCondition; import org.openstreetmap.josm.gui.mappaint.mapcss.Expression; import org.openstreetmap.josm.gui.mappaint.mapcss.Instruction; import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSRule; @@ -377,6 +378,20 @@ res.add(errorBuilder.primitives(p, (OsmPrimitive) c).build()); } } + } else if (env.parent != null) { + boolean imcompletePrimitives = false; + if (matchingSelector instanceof Selector.ChildOrParentSelector) { + Selector right = ((Selector.ChildOrParentSelector) matchingSelector).right; + if (right.getConditions().stream().anyMatch(ClassCondition.class::isInstance)) { + // see #23397 + // TODO: find a way to collect all and only those parent objects which triggered this error + imcompletePrimitives = true; + } + } + if (imcompletePrimitives) + res.add(errorBuilder.primitives(p).highlight(p).imcompletePrimitives().build()); + else + res.add(errorBuilder.primitives(p, (OsmPrimitive) env.parent).highlight(p).build()); } else { res.add(errorBuilder.primitives(p).build()); } Index: src/org/openstreetmap/josm/data/validation/util/AggregatePrimitivesVisitor.java =================================================================== --- src/org/openstreetmap/josm/data/validation/util/AggregatePrimitivesVisitor.java (revision 18957) +++ src/org/openstreetmap/josm/data/validation/util/AggregatePrimitivesVisitor.java (working copy) @@ -14,7 +14,6 @@ /** * A visitor that aggregates all primitives it visits. *

- * The primitives are sorted according to their type: first nodes, then ways. * * @author frsantos */ @@ -24,7 +23,7 @@ /** * Visits a collection of primitives - * @param data The collection of primitives + * @param data The collection of primitives in no specific order. * @return The aggregated primitives */ public Collection visit(Collection data) {