Index: applications/editors/josm/plugins/validator/build.xml =================================================================== --- applications/editors/josm/plugins/validator/build.xml (revision 19225) +++ applications/editors/josm/plugins/validator/build.xml (revision 19226) @@ -26,6 +26,6 @@ ** update before publishing --> - - + + Index: applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/ValidateUploadHook.java =================================================================== --- applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/ValidateUploadHook.java (revision 19225) +++ applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/ValidateUploadHook.java (revision 19226) @@ -15,5 +15,4 @@ import org.openstreetmap.josm.actions.upload.UploadHook; import org.openstreetmap.josm.data.APIDataSet; -import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.OsmPrimitive; import org.openstreetmap.josm.plugins.validator.util.AgregatePrimitivesVisitor; Index: applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/ValidatorDialog.java =================================================================== --- applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/ValidatorDialog.java (revision 19225) +++ applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/ValidatorDialog.java (revision 19226) @@ -43,5 +43,4 @@ import org.openstreetmap.josm.gui.progress.ProgressMonitor; import org.openstreetmap.josm.io.OsmTransferException; -import org.openstreetmap.josm.plugins.validator.tests.DuplicateNode; import org.openstreetmap.josm.tools.Shortcut; import org.xml.sax.SAXException; Index: applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java =================================================================== --- applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java (revision 19225) +++ applications/editors/josm/plugins/validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java (revision 19226) @@ -4,9 +4,13 @@ import java.awt.geom.Area; +import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import javax.swing.JOptionPane; @@ -33,6 +37,11 @@ protected static int DUPLICATE_NODE = 1; - /** Bag of all nodes */ - Bag nodes; + /** The map of potential duplicates. + * + * If there is exactly one node for a given pos, the map includes a pair . + * If there are multiple nodes for a given pos, the map includes a pair + * + */ + Map potentialDuplicates; /** @@ -48,42 +57,53 @@ public void startTest(ProgressMonitor monitor) { super.startTest(monitor); - nodes = new Bag(1000); - } - + potentialDuplicates = new HashMap(); + } + + @Override - public void endTest() { - for (List duplicated : nodes.values()) { - if (duplicated.size() > 1) { - boolean sameTags = true; - Map keys0 = duplicated.get(0).getKeys(); - keys0.remove("created_by"); - for (int i = 0; i < duplicated.size(); i++) { - Map keysI = duplicated.get(i).getKeys(); - keysI.remove("created_by"); - if (!keys0.equals(keysI)) - sameTags = false; - } - if (!sameTags) { - TestError testError = new TestError(this, Severity.WARNING, - tr("Nodes at same position"), DUPLICATE_NODE, - duplicated); - errors.add(testError); - } else { - TestError testError = new TestError(this, Severity.ERROR, - tr("Duplicated nodes"), DUPLICATE_NODE, duplicated); - errors.add(testError); - } - } + public void endTest() { + for (Entry entry: potentialDuplicates.entrySet()) { + Object v = entry.getValue(); + if (v instanceof Node) { + // just one node at this position. Nothing to report as + // error + continue; + } + + // multiple nodes at the same position -> report errors + // + NodesByEqualTagsMap map = (NodesByEqualTagsMap)v; + errors.addAll(map.buildTestErrors(this)); } super.endTest(); - nodes = null; + potentialDuplicates = null; } - @Override - public void visit(Node n) - { - if(n.isUsable()) - nodes.add(n.getCoor(), n); - } + @Override + public void visit(Node n) { + if (n.isUsable()) { + LatLon rounded = n.getCoor().getRoundedToOsmPrecision(); + if (potentialDuplicates.get(rounded) == null) { + // in most cases there is just one node at a given position. We + // avoid to create an extra object and add remember the node + // itself at this position + potentialDuplicates.put(rounded, n); + } else if (potentialDuplicates.get(rounded) instanceof Node) { + // we have an additional node at the same position. Create an extra + // object to keep track of the nodes at this position. + // + Node n1 = (Node)potentialDuplicates.get(rounded); + NodesByEqualTagsMap map = new NodesByEqualTagsMap(); + map.add(n1); + map.add(n); + potentialDuplicates.put(rounded, map); + } else if (potentialDuplicates.get(rounded) instanceof NodesByEqualTagsMap) { + // we have multiple nodes at the same position. + // + NodesByEqualTagsMap map = (NodesByEqualTagsMap)potentialDuplicates.get(rounded); + map.add(n); + } + } + } /** @@ -103,9 +123,8 @@ } - @Override - public boolean isFixable(TestError testError) - { - return (testError.getTester() instanceof DuplicateNode); - } + @Override + public boolean isFixable(TestError testError) { + return (testError.getTester() instanceof DuplicateNode); + } /** @@ -138,3 +157,61 @@ return true; } + + static private class NodesByEqualTagsMap { + /** + * a bag of primitives with the same position. The bag key is represented + * by the tag set of the primitive. This allows for easily find nodes at + * the same position with the same tag sets later. + */ + private Bag, OsmPrimitive> bag; + + public NodesByEqualTagsMap() { + bag = new Bag, OsmPrimitive>(); + } + + public void add(Node n) { + bag.add(n.getKeys(), n); + } + + public List buildTestErrors(Test parentTest) { + List errors = new ArrayList(); + // check whether we have multiple nodes at the same position with + // the same tag set + // + for (Iterator> it = bag.keySet().iterator(); it.hasNext(); ) { + Map tagSet = it.next(); + if (bag.get(tagSet).size() > 1) { + errors.add(new TestError( + parentTest, + Severity.ERROR, + tr("Duplicated nodes"), + DUPLICATE_NODE, + bag.get(tagSet) + )); + it.remove(); + } + + } + + // check whether we have multiple nodes at the same position with + // differing tag sets + // + if (!bag.isEmpty()) { + List duplicates = new ArrayList(); + for (List l: bag.values()) { + duplicates.addAll(l); + } + if (duplicates.size() > 1) { + errors.add(new TestError( + parentTest, + Severity.WARNING, + tr("Nodes at same position"), + DUPLICATE_NODE, + duplicates + )); + } + } + return errors; + } + } } Index: applications/editors/josm/plugins/validator/test/org/openstreetmap/josm/plugins/validator/tests/DuplicateNodeTest.groovy =================================================================== --- applications/editors/josm/plugins/validator/test/org/openstreetmap/josm/plugins/validator/tests/DuplicateNodeTest.groovy (revision 19226) +++ applications/editors/josm/plugins/validator/test/org/openstreetmap/josm/plugins/validator/tests/DuplicateNodeTest.groovy (revision 19226) @@ -0,0 +1,150 @@ +package org.openstreetmap.josm.plugins.validator.tests; +import java.util.Collections; + +import org.junit.Test; +import org.junit.experimental.theories.PotentialAssignment; +import org.openstreetmap.josm.data.osm.Node; +import org.openstreetmap.josm.data.coor.LatLon + +import org.openstreetmap.josm.gui.progress.NullProgressMonitor; +import org.openstreetmap.josm.plugins.validator.tests.DuplicateNode; +import org.openstreetmap.josm.plugins.validator.TestError; + +class DuplicateNodeTest { + + @Test + public void emptyNodeCollection() { + DuplicateNode t = new DuplicateNode() + + t.startTest NullProgressMonitor.INSTANCE + t.visit Collections.emptyList() + t.endTest() + + assert t.@errors.isEmpty() + } + + @Test + public void twoDuplicateNodes() { + DuplicateNode t = new DuplicateNode() + + def n1 = new Node(new LatLon(1.0,1.0)) + def n2 = new Node(new LatLon(1.0, 1.0)) + + def nodes = [n1,n2] + + t.startTest NullProgressMonitor.INSTANCE + t.visit nodes + assert t.@potentialDuplicates.size() == 1 + t.endTest() + + assert t.@errors.size() == 1 + TestError te = t.@errors.get(0) + assert te.getPrimitives().contains(n1) + assert te.getPrimitives().contains(n2) + } + + @Test + public void twoDuplicateNodes_ReducedPrecision() { + DuplicateNode t = new DuplicateNode() + + def n1 = new Node(new LatLon(1.0,1.0)) + def n2 = new Node(new LatLon(1.00000001, 1.00000001)) + + def nodes = [n1,n2] + + t.startTest NullProgressMonitor.INSTANCE + t.visit nodes + t.endTest() + + assert t.@errors.size() == 1 + TestError te = t.@errors.get(0) + assert te.getPrimitives().contains(n1) + assert te.getPrimitives().contains(n2) + } + + @Test + public void twoDuplicateNodes_DifferentTagSets() { + DuplicateNode t = new DuplicateNode() + + def n1 = new Node(new LatLon(1.0,1.0)) + n1.put "aaaa", "aaaa" + + def n2 = new Node(new LatLon(1.0, 1.0)) + n2.put "bbbb", "bbbb" + + def nodes = [n1,n2] + + t.startTest NullProgressMonitor.INSTANCE + t.visit nodes + t.endTest() + + assert t.@errors.size() == 1 + assert t.@errors.get(0).getMessage() == "Nodes at same position" + } + + @Test + public void fourDuplicateNodes_TwoDifferentTagSets() { + DuplicateNode t = new DuplicateNode() + + def n1 = new Node(new LatLon(1.0,1.0)) + n1.put "aaaa", "aaaa" + + def n2 = new Node(new LatLon(1.0, 1.0)) + n2.put "bbbb", "bbbb" + + def n3 = new Node(new LatLon(1.0,1.0)) + n3.put "aaaa", "aaaa" + + def n4 = new Node(new LatLon(1.0, 1.0)) + n4.put "bbbb", "bbbb" + + def nodes = [n1,n2,n3,n4] + + t.startTest NullProgressMonitor.INSTANCE + t.visit nodes + t.endTest() + + assert t.@errors.size() == 2 + assert t.@errors.get(0).getMessage() == "Duplicated nodes" + assert t.@errors.get(0).getPrimitives().size() == 2 + assert t.@errors.get(1).getMessage() == "Duplicated nodes" + assert t.@errors.get(1).getPrimitives().size() == 2 + } + + @Test + public void SixDuplicateNodes_FourDifferentTagSets() { + DuplicateNode t = new DuplicateNode() + + def n1 = new Node(new LatLon(1.0,1.0)) + n1.put "aaaa", "aaaa" + + def n2 = new Node(new LatLon(1.0, 1.0)) + n2.put "bbbb", "bbbb" + + def n3 = new Node(new LatLon(1.0,1.0)) + n3.put "aaaa", "aaaa" + + def n4 = new Node(new LatLon(1.0, 1.0)) + n4.put "bbbb", "bbbb" + + def n5 = new Node(new LatLon(1.0, 1.0)) + n5.put "55555", "5555" + + def n6 = new Node(new LatLon(1.0, 1.0)) + n6.put "6666", "6666" + + def nodes = [n1,n2,n3,n4,n5,n6] + + t.startTest NullProgressMonitor.INSTANCE + t.visit nodes + t.endTest() + + assert t.@errors.size() == 3 + assert t.@errors.get(0).getMessage() == "Duplicated nodes" + assert t.@errors.get(0).getPrimitives().size() == 2 + assert t.@errors.get(1).getMessage() == "Duplicated nodes" + assert t.@errors.get(1).getPrimitives().size() == 2 + assert t.@errors.get(2).getMessage() == "Nodes at same position" + assert t.@errors.get(2).getPrimitives().size() == 2 + } +}