[josm-dev] Jumbo Patch
Gabriel Ebner
ge at gabrielebner.at
Sat Dec 15 09:56:56 GMT 2007
(Commenting on the plugin changes.)
On Fri, Dec 14, 2007 at 04:45:56PM -0800, Dave Hansen wrote:
> Index: plugins/validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java
> ===================================================================
> --- plugins/validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java (revision 6054)
> +++ plugins/validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java (working copy)
> @@ -23,6 +23,7 @@
> {
> /** Bag of all nodes */
> Bag<LatLon, OsmPrimitive> nodes;
> + Bag<LatLon, Way> ways;
>
> /**
> * Constructor
> @@ -38,15 +39,90 @@
> public void startTest()
> {
> nodes = new Bag<LatLon, OsmPrimitive>(1000);
> + ways = new Bag<LatLon, Way>(1000);
> }
>
> + static Node getNodeAt(Way w, LatLon l)
> + {
> + //stem.out.println("getNodeAt() begin");
> + for (Node n : w.allNodes()) {
> + //stem.out.println(" getNodeAt(" + l + ") node: " + n + " equal: " + (n.coor.equals(l)) );
> + if (n.coor.equals(l)) {
> + //stem.out.println("getNodeAt() returning: " + n);
> + return n;
> + }
> + }
> + //stem.out.println("getNodeAt() never found anything");
> + return null;
> + }
> + static String getType(OsmPrimitive p)
> + {
> + Collection<String> types = new LinkedList<String>();
> + types.add("highway");
> + types.add("railway");
> + types.add("powerline");
> + for (String type : types) {
> + if (p.get(type) == null)
> + continue;
> + // Motorways are allowed to intersect other
> + // roads, but not be connected.
> + if (type.equals("highway") && p.get("highway").equals("motorway"))
> + return p.get("highway");
> + return type;
> + }
> + return null;
> + }
> @Override
> public void endTest()
> {
> - for(List<OsmPrimitive> duplicated : nodes.values() )
> + for(LatLon key : ways.keySet() )
> {
> - if( duplicated.size() > 1)
> - {
> + //stem.out.println("there are " + ways.get(key).size() + " ways and " +
> + // nodes.get(key).size() + " nodes at " + key);
> + // this means that each of the nodes was found
> + // in _some_ way, meaning there are multiple ways
> + // at these coordinates. We'll deal with merging
> + // (or not) those ways here.
> + if (ways.get(key).size() >= nodes.get(key).size())
> + nodes.remove(key);
> + ArrayList<OsmPrimitive> duplicated = new ArrayList<OsmPrimitive>();
> + for (Way w1 : ways.get(key)) {
> + for (Way w2 : ways.get(key)) {
> + if (w1 == w2) {
> + //stem.out.println("same way");
> + continue;
> + }
> + if (getType(w1) != getType(w2)) {
> + //stem.out.println("different types: '" + getType(w1) + "'/'" + getType(w2) + "'");
> + continue;
> + }
> + Node n1 = getNodeAt(w1, key);
> + Node n2 = getNodeAt(w2, key);
> + if (n1 == null || n1.deleted) {
> + //stem.out.println("n1 deleted or null: " + n1);
> + continue;
> + }
> + if (n2 == null || n2.deleted) {
> + //stem.out.println("n2 deleted or null: " + n2);
> + continue;
> + }
> + if (n1 == n2) {
> + //stem.out.println("same node");
> + continue;
> + }
> + if (!duplicated.contains(n1))
> + duplicated.add(n1);
> + if (!duplicated.contains(n2))
> + duplicated.add(n2);
> + }
> + }
> + if (duplicated.size() <= 1)
> + continue;
> + TestError testError = new TestError(this, Severity.ERROR, tr("Duplicated nodes"), duplicated);
> + errors.add( testError );
> + }
> + for(List<OsmPrimitive> duplicated : nodes.values() ) {
> + if( duplicated.size() > 1) {
> TestError testError = new TestError(this, Severity.ERROR, tr("Duplicated nodes"), duplicated);
> errors.add( testError );
> }
The detection code is maybe *seven* lines now (excluding empty lines, braces
and signatures).
As we're speeding up way-on-node lookups one way or another anyhow, couldn't
we just filter out the unwanted errors? I.e. something like this:
@Override
public void endTest()
{
for(List<OsmPrimitive> duplicated : nodes.values() )
{
if( duplicated.size() > 1)
{
boolean wantedError = false;
Set<String> types = new HashSet<String>();
for (OsmPrimitive p : duplicated) {
Set<String> nodeTypes = new HashSet<String>();
for (Way w : waysForNode((Node) p)) {
String type = getType(w);
if (types.contains(type)) {
wantedError = true;
}
nodeTypes.add(getType(w));
}
types.addAll(nodeTypes);
}
if (!wantedError) continue;
TestError testError = new TestError(this, Severity.ERROR, tr("Duplicated nodes"), duplicated);
errors.add( testError );
}
}
nodes = null;
}
Aside from that, I think we'd better limit that to tiger ways. Potlatch seems
to sometimes create duplicate nodes, and that would not be detected then.
> @@ -98,43 +183,32 @@
> nodes.remove(target);
>
> // Merge all properties
> - Node newtarget = new Node(target);
> - for (final OsmPrimitive o : nodes)
> - {
> + Collection<Command> cmds = new LinkedList<Command>();
[...]
> return new SequenceCommand(tr("Merge Nodes"), cmds);
> }
MergeNodes.mergeNodes(nodes, target) ?
> +/**
> + * Checks for similar named ways, symptom of a possible typo. It uses the
> + * Levenshtein distance to check for similarity
> + *
> + * @author frsantos
> + */
I know there are some extreme cases of copy-and-paste documentation in JOSM
itself, but please don't duplicate that. :-)
> + public EmptyWays()
> + {
> + super(tr("Empty Ways."),
> + tr("This test checks for ways that have no segments in them."));
> + }
s/segments/nodes/
> + @Override
> + public void visit(Way w)
> + {
> + if( w.deleted )
You need to add "|| w.incomplete" here; incomplete ways also have 0 nodes, and
generate warm, fuzzy 412 errors when uploading.
> Index: plugins/validator/src/org/openstreetmap/josm/plugins/validator/OSMValidatorPlugin.java
> ===================================================================
> --- plugins/validator/src/org/openstreetmap/josm/plugins/validator/OSMValidatorPlugin.java (revision 6054)
> +++ plugins/validator/src/org/openstreetmap/josm/plugins/validator/OSMValidatorPlugin.java (working copy)
> @@ -39,6 +39,7 @@
> Map<Layer, List<TestError>> layerErrors = new HashMap<Layer, List<TestError>>();
>
> /**
> + * 1;3B
Typo?
> Index: plugins/validator/src/org/openstreetmap/josm/plugins/validator/TestError.java
> ===================================================================
> --- plugins/validator/src/org/openstreetmap/josm/plugins/validator/TestError.java (revision 6054)
> +++ plugins/validator/src/org/openstreetmap/josm/plugins/validator/TestError.java (working copy)
> @@ -20,7 +20,7 @@
> /** The error message */
> private String message;
> /** The affected primitives */
> - private List<OsmPrimitive> primitives;
> + private List<OsmPrimitive> __primitives;
Is there a reason to prefix this with a double underscore?
> @@ -42,12 +42,22 @@
> * @param message The error message
> * @param primitives The affected primitives
> */
> + void assignPrimitives(List<OsmPrimitive> newprimitives, int nr)
> + {
> + for (OsmPrimitive op : newprimitives) {
> + if (op == null) {
> + System.out.println("TestError " + nr + " op null");
> + System.out.println(op.toString());
> + }
> + }
> + this.__primitives = newprimitives;
> + }
1) The javadoc is really bad from a code documentation POV, but that doesn't
mean you should just insert another method directly under it (javadoc comments
apply to the method immediately following it.)
2) Doesn't the second line always print "null"?
Gabriel.
More information about the josm-dev
mailing list