Ticket #23397: 23397-beta2.patch

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

@@ -12 +12 @@
 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 +69 @@
                 selection = getLayerManager().getActiveDataSet().allNonDeletedPrimitives();
                 lastSelection = null;
             } else {
-                AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor();
-                selection = v.visit(selection);
                 lastSelection = selection;
             }
         } else {

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

@@ -6 +6 @@
 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 +18 @@
 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 +45 @@
     @Override
     public boolean checkUpload(APIDataSet apiDataSet) {
         AtomicBoolean returnCode = new AtomicBoolean();
-        AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor();
-        v.visit(apiDataSet.getPrimitivesToAdd());
-        Collection<OsmPrimitive> visited = v.visit(apiDataSet.getPrimitivesToUpdate());
+        Collection<OsmPrimitive> toCheck = new HashSet<>();
+        toCheck.addAll(apiDataSet.getPrimitivesToAdd());
+        toCheck.addAll(apiDataSet.getPrimitivesToUpdate());
         OsmValidator.initializeTests();
         new ValidationTask(errors -> {
             if (errors.stream().allMatch(TestError::isIgnored)) {
@@ -58 +58 @@
                 // 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();
     }

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

@@ -6 +6 @@
 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 +385 @@
     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<? extends OsmPrimitive> 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<? extends OsmPrimitive> relevant;
+        if (given instanceof Set) {
+            relevant = (Set<? extends OsmPrimitive>) given;
+        } else {
+            relevant = new HashSet<>(given);
+        }
+        errors.removeIf(e -> !e.isConcerned(relevant));
+    }
 }

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

@@ -12 +12 @@
 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;
@@ -666 +667 @@
                 ", 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<? extends OsmPrimitive> given) {
+        for (OsmPrimitive p : getPrimitives()) {
+            if (given.contains(p)) {
+                return true;
+            }
+        }
+        return false;
+    }
 }

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

@@ -6 +6 @@
 import java.awt.GraphicsEnvironment;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
@@ -12 +13 @@
 
 import javax.swing.JOptionPane;
 
+import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 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 +33 @@
 public class ValidationTask extends PleaseWaitRunnable {
     private final Consumer<List<TestError>> onFinish;
     private Collection<Test> tests;
-    private final Collection<OsmPrimitive> validatedPrimitives;
+    private final Collection<OsmPrimitive> initialPrimitives;
     private final Collection<OsmPrimitive> formerValidatedPrimitives;
     private final boolean beforeUpload;
     private boolean canceled;
@@ -71 +74 @@
                 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;
     }
 
+    /**
+     * Create extended list
+     * - add child objects because MapCss tests may need them to work properly
+     * - add parent objects of modified nodes as they may mean a geometry change
+     * @param primitives the primitives
+     * @return extended list of primitives
+     */
+    private static Collection<OsmPrimitive> extendList(Collection<OsmPrimitive> primitives) {
+        Collection<OsmPrimitive> extendedList = new HashSet<>(primitives);
+        for (OsmPrimitive p : primitives) {
+            if (p instanceof Node && p.isModified() && !p.isNew()) {
+                for (OsmPrimitive parent : p.getReferrers()) {
+                    if (!parent.isDeleted()) {
+                        extendedList.add(parent);
+                    }
+                }
+            }
+        }
+        AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor();
+        return v.visit(extendedList);
+    }
+
     protected ValidationTask(ProgressMonitor progressMonitor,
             Collection<Test> tests,
             Collection<OsmPrimitive> validatedPrimitives,
@@ -122 +147 @@
     protected void realRun() {
         if (Utils.isEmpty(tests))
             return;
+        int testCounter = 0;
+        final boolean isPartial = this.beforeUpload || formerValidatedPrimitives != null;
+
+        Collection<OsmPrimitive> validatedPrimitives = isPartial ? extendList(initialPrimitives) : initialPrimitives;
         getProgressMonitor().setTicksCount(tests.size() * validatedPrimitives.size());
-        int testCounter = 0;
+
         for (Test test : tests) {
             if (canceled)
                 return;
@@ -131 +160 @@
             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(initialPrimitives);
+            }
             errors.addAll(test.getErrors());
             if (this.testConsumer != null) {
                 this.testConsumer.accept(this, test);

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

@@ -6 +6 @@
 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 +17 @@
 
 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 +84 @@
     private final Map<Point2D, List<WaySegment>> cellSegments = new HashMap<>(1000);
     /** The already detected ways in error */
     private final Map<List<Way>, List<WaySegment>> seenWays = new HashMap<>(50);
+    private final Set<Way> waysToTest = new HashSet<>();
 
     protected final int code;
 
@@ -305 +309 @@
 
     @Override
     public void endTest() {
-        super.endTest();
+        final Collection<Way> 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 +367 @@
 
     @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 +509 @@
         CheckParameterUtil.ensureParameterNotNull(way, "way");
         SelfCrossing test = new SelfCrossing();
         test.visit(way);
+        test.endTest();
         return !test.getErrors().isEmpty();
     }
 }

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

@@ -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 +104 @@
 
     /** Set of known hashcodes for list of coordinates **/
     private Set<Integer> knownHashCodes;
+    private List<Way> waysToCheck;
 
     /**
      * Constructor
@@ -115 +117 @@
     @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 +125 @@
 
     @Override
     public void endTest() {
-        super.endTest();
+        if (partialSelection && !waysToCheck.isEmpty()) {
+            // make sure that we have the error candidates even if not selected
+            Set<Way> 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<OsmPrimitive> duplicated : ways.values()) {
             if (duplicated.size() > 1) {
                 TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_WAY)
@@ -162 +180 @@
                 errors.add(testError);
             }
         }
+
         ways = null;
         waysNoTags = null;
         knownHashCodes = null;
+        waysToCheck = null;
+        super.endTest();
     }
 
     /**
@@ -181 +202 @@
     public void visit(Way w) {
         if (!w.isUsable())
             return;
+        if (partialSelection)
+            waysToCheck.add(w);
+        else
+            checkWay(w);
+    }
+
+    private void checkWay(Way w) {
         List<LatLon> 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()) {

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

@@ -377 +377 @@
                         res.add(errorBuilder.primitives(p, (OsmPrimitive) c).build());
                     }
                 }
+            } else if (env.parent != null) {
+                res.add(errorBuilder.primitives(p, (OsmPrimitive) env.parent).highlight(p).build());
             } else {
                 res.add(errorBuilder.primitives(p).build());
             }

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

@@ -14 +14 @@
 /**
  * A visitor that aggregates all primitives it visits.
  * <p>
- * The primitives are sorted according to their type: first nodes, then ways.
  *
  * @author frsantos
  */
@@ -24 +23 @@
 
     /**
      * 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<OsmPrimitive> visit(Collection<OsmPrimitive> data) {