Index: trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java (revision 17375) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java (revision 17376) @@ -8,5 +8,4 @@ import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; @@ -18,4 +17,5 @@ import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.openstreetmap.josm.actions.MergeNodesAction; @@ -25,5 +25,4 @@ import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.OsmPrimitive; -import org.openstreetmap.josm.data.osm.OsmPrimitiveType; import org.openstreetmap.josm.data.osm.Storage; import org.openstreetmap.josm.data.osm.Way; @@ -162,5 +161,5 @@ List errors = new ArrayList<>(); - MultiMap, OsmPrimitive> mm = new MultiMap<>(); + MultiMap, Node> mm = new MultiMap<>(); for (Node n: nodes) { mm.put(n.getKeys(), n); @@ -171,5 +170,5 @@ // check whether we have multiple nodes at the same position with the same tag set for (Iterator> it = mm.keySet().iterator(); it.hasNext();) { - Set primitives = mm.get(it.next()); + Set primitives = mm.get(it.next()); if (primitives.size() > 1) { @@ -178,23 +177,16 @@ } - for (OsmPrimitive p : primitives) { - if (p.getType() == OsmPrimitiveType.NODE) { - Node n = (Node) p; - List lp = n.getReferrers(); - for (OsmPrimitive sp: lp) { - if (sp.getType() == OsmPrimitiveType.WAY) { - boolean typed = false; - Way w = (Way) sp; - Map keys = w.getKeys(); - for (Entry e : typeMap.entrySet()) { - if (keys.containsKey(e.getKey())) { - e.setValue(Boolean.TRUE); - typed = true; - } - } - if (!typed) { - typeMap.put("none", Boolean.TRUE); - } + for (Node n : primitives) { + for (Way w: n.getParentWays()) { + boolean typed = false; + Map keys = w.getKeys(); + for (Entry e : typeMap.entrySet()) { + if (keys.containsKey(e.getKey())) { + e.setValue(Boolean.TRUE); + typed = true; } + } + if (!typed) { + typeMap.put("none", Boolean.TRUE); } } @@ -202,56 +194,37 @@ long nbType = typeMap.entrySet().stream().filter(Entry::getValue).count(); - + final TestError.Builder builder; if (nbType > 1) { - errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED) - .message(tr("Mixed type duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get(HIGHWAY)) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY) - .message(tr("Highway duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get(RAILWAY)) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY) - .message(tr("Railway duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get(WATERWAY)) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY) - .message(tr("Waterway duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("boundary")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY) - .message(tr("Boundary duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("power")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER) - .message(tr("Power duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("natural")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL) - .message(tr("Natural duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("building")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING) - .message(tr("Building duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("landuse")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE) - .message(tr("Landuse duplicated nodes")) - .primitives(primitives) - .build()); + builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED) + .message(tr("Mixed type duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get(HIGHWAY))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY) + .message(tr("Highway duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get(RAILWAY))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY) + .message(tr("Railway duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get(WATERWAY))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY) + .message(tr("Waterway duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("boundary"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY) + .message(tr("Boundary duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("power"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER) + .message(tr("Power duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("natural"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL) + .message(tr("Natural duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("building"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING) + .message(tr("Building duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("landuse"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE) + .message(tr("Landuse duplicated nodes")); } else { - errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER) - .message(tr("Other duplicated nodes")) - .primitives(primitives) - .build()); + builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER) + .message(tr("Other duplicated nodes")); } + errors.add(builder.primitives(primitives).build()); it.remove(); } @@ -261,5 +234,5 @@ if (!mm.isEmpty()) { List duplicates = new ArrayList<>(); - for (Set l: mm.values()) { + for (Set l: mm.values()) { duplicates.addAll(l); } @@ -278,23 +251,19 @@ public void visit(Node n) { if (n.isUsable()) { - if (potentialDuplicates.get(n) == null) { + Object old = potentialDuplicates.get(n); + if (old == 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(n); - } else if (potentialDuplicates.get(n) instanceof Node) { + } else if (old 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(n); - List nodes = new ArrayList<>(2); - nodes.add(n1); - nodes.add(n); - potentialDuplicates.put(nodes); - } else if (potentialDuplicates.get(n) instanceof List) { - // we have multiple nodes at the same position. + potentialDuplicates.put(Stream.of((Node) old, n).collect(Collectors.toList())); + } else { + // we have more than two nodes at the same position. // - List nodes = (List) potentialDuplicates.get(n); - nodes.add(n); + ((List) old).add(n); } } @@ -307,4 +276,6 @@ @Override public Command fixError(TestError testError) { + if (!isFixable(testError)) + return null; final Set nodes = testError.primitives(Node.class) // Filter nodes that have already been deleted (see #5764 and #5773) @@ -312,17 +283,11 @@ .collect(Collectors.toCollection(LinkedHashSet::new)); - // Merge only if at least 2 nodes remain - if (nodes.size() >= 2) { - // Use first existing node or first node if all nodes are new - Node target = nodes.stream() - .filter(n -> !n.isNew()) - .findFirst() - .orElseGet(() -> nodes.iterator().next()); - - if (Command.checkOutlyingOrIncompleteOperation(nodes, Collections.singleton(target)) == Command.IS_OK) - return MergeNodesAction.mergeNodes(nodes, target); - } - - return null; // undoRedo handling done in mergeNodes + // Use first existing node or first node if all nodes are new + Node target = nodes.stream() + .filter(n -> !n.isNew()) + .findFirst() + .orElseGet(() -> nodes.iterator().next()); + + return MergeNodesAction.mergeNodes(nodes, target); }