[josm-dev] Jumbo Patch

[Dave Hansen]

On Sat, 2007-12-15 at 10:56 +0100, Gabriel Ebner wrote:
> (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)) );
... snipped lots of code...
> > +		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).

Yeah, but it's a worthless seven lines of code for the TIGER data.  We
have probably hundreds of thousands of points in the same location which
are perfectly valid, and were requested to be created like that.
They're certainly not errors.  But, it takes knowledge of the ways that
use these points to figure this out.  That's where all the code comes
from.

> 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.

My intent was only to ignore those points where there were significantly
different types of ways using the points.  Although TIGER is the best
example of this, I don't think it's limited to TIGER by any stretch of
the imagination.

> Potlatch seems
> to sometimes create duplicate nodes, and that would not be detected then.

Could you explain a bit more?  I'm not quite understanding...

> > @@ -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.  :-)

Heh.  Fixed.

> > +	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.

OK.  Got it.

> > 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?

Yup.  Fixed.

> > 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"?

I think I added this just for debugging because I thought someone was
adding null primitives and I wanted to check them.  I'll kill it.

-- Dave