[josm-dev] JOSM Jumbo patch v8 (JOSM and validator enhancements)

[Dave Hansen]

On Wed, 2008-01-30 at 13:17 +0100, Gabriel Ebner wrote:
> On Tue, Jan 29, 2008 at 12:46:36PM -0800, Dave Hansen wrote:
> > My basic motivation here is to have the validator detect and fix all of
> > the things that I've been doing manually, especially with the TIGER
> > data.
> 
> Thank you for your efforts.
> 
> > I'd love to get this merged into JOSM, but JOSM development is pretty
> > slow these days, and it turns out not to be too much trouble to keep my
> > patches merged up to date with the latest JOSM. 
> 
> I think the following things are pretty much uncontroversial and I'll merge
> them later today if nobody objects.  These features are great but they were
> sadly drowned in the debate over the reverse lookup changes.

Thanks for considering them individually!

> > * remove unused imports
> > * add Main.debug() function for messages
> 
> TODO: Add a --debug flag for that.

OK.  I did this in the latest version.

> > * teach Way/Node merging about TIGER tags (this allows good merging at
> >   TIGER county borders)
> > * add retries to uploads
> > * give better progress in upload dialog
> > * add Bounds::contains() function
> 
> >    * MotorwayLinksMustTouchMotorways, don't let motorway_links exist
> >      unless touching a motorway (lots of these in TIGER)
> >    * MotorwayIntersections: motorways may only touch motorway_links
> >      and other motorways, except at their endpoints (fixable)
> >    * DuplicateSegment: for ways that re-use the same node pair
> 
> TODO: Only highlight the duplicated way segments instead of the complete way.
> This should be just a matter of passing a List containing the WaySegment to
> the TestError constructor.

One issue here is that the current WaySegment isn't an OsmPrimitive
derivative and can't be handed to a TestError.  Should we change that?

> > * Rewrote coastlines checks:
> >    * check for "forks" in coastline
> >    * ensure coastline is ordered (these are fixable now)
> >    * check for "ends" of coastline inside the current layer
> > * Make OverLapping ways fixable
> > * Take different types of ways into account in CrossingWays
> > * consider TIGER tags in UntaggedNodes
> 
> =====
> 
> > * Basically rewrote DuplicatedNodes to allow multiple nodes at the same
> >   spot if they are different types (highway, power, railway, etc...)
> 
> DuplicateNodes is missing from plugins-10.diff

That's odd.  I see it in there:

--- validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java     (revision 6103)
+++ validator/src/org/openstreetmap/josm/plugins/validator/tests/DuplicateNode.java     (working copy)
@@ -14,6 +14,7 @@
 import org.openstreetmap.josm.plugins.validator.Test;
 import org.openstreetmap.josm.plugins.validator.TestError;
 import org.openstreetmap.josm.plugins.validator.util.Bag;
+import org.openstreetmap.josm.tools.*;
 /**
  * Tests if there are duplicate nodes
  *
@@ -22,7 +23,10 @@
 public class DuplicateNode extends Test
 {
        /** Bag of all nodes */
-       Bag<LatLon, OsmPrimitive> nodes;
...


> >    * Empty ways (ways with 0 nodes) (fixable)
> 
> We already check that in UntaggedWay (along with one-node ways and unnamed
> ways).

Ahhh.  OK.  I'll kill my version.

> == Granular commands ==
> 
> > --- src/org/openstreetmap/josm/command/RemoveNodeInWayCommand.java	(revision 0)
> > +	@Override public void undoCommand() {
> > +		way.addNodeNr(location, node);
> > +	    way.modified = false;
> > +    }
> 
> > --- src/org/openstreetmap/josm/command/AddNodeToWayCommand.java	(revision 0)
> > +	@Override public void undoCommand() {
> > +	    super.undoCommand();
> > +		Node removed = way.removeNode(location);
> > +		if (removed != node)
> > +			Main.debug("removed wrong node");
> > +	    way.modified = false;
> > +    }
> 
> > --- src/org/openstreetmap/josm/command/ReplaceNodeInWayCommand.java	(revision 0)
> > +	@Override public void undoCommand() {
> > +		way.replaceNode(newNode, node, replaced_at);
> > +		//stem.out.println("undo ReplaceNode at: " + replaced_at);
> > +	    way.modified = false;
> > +    }
> 
> > --- src/org/openstreetmap/josm/command/ReverseWayCommand.java	(revision 0)
> > +	@Override public void undoCommand() {
> > +		way.reverseNodes();
> > +	    way.modified = false;
> > +    }
> 
> Setting the modified flag to false will break uploads after undo.  If the user
> modifies a way, then reverses it, and then hits undo, then the way will not be
> uploaded.

OK.  I'll fix this up.  I do wonder, however, if there's a better
interface we could use for this.  Something like this:

class OsmPrimitive {
	ArrayList<Modification> modificationList = new ...;
	Modification setModified()
	{
		Modification mod = new Modification();
		modificationList.add(mod)
		return mod;
	}
	void unModify(Modification mod)
	{
		if (modificationList.indexOf(mod) !=
		    modificationList.size()-1)
			error();
		modificationList.remove(mod);
	}
	void isModified()
	{
		return (modificationList.size()==0)
	}

Which would make sure that people are keeping a stack of state of the
object, and unmodifying it in the right order.  Even better, it seems
like we should have a nice set of access functions to the objects so
that it can track this state itself, internally.

One thing that I've done for now in Command is save copies of the
original objects.  Right now all that I'm using this for is to restore
the state to its old value, but it could be used for more.

> > --- src/org/openstreetmap/josm/command/ConditionalDeleteCommand.java	(revision 0)
> > +	@Override public void fillModifiedData(Collection<OsmPrimitive> modified, Collection<OsmPrimitive> deleted, Collection<OsmPrimitive> added) {}
> 
> You'll need to implement either this or undoCommand, else undo won't work.

OK, fixed this.

> == Reverse lookup ==
> 
> >   * uses preference: reverse-lookup=always/never/cache
> >    * 'always' will create the cache from the start, and keep it up to
> >      date always.
> >    * 'never' will always use the slow back-reference collector
> >    * 'cache' will use the slow one the first time, then the cache after
> >      that. (this is the default)
> 
> Good idea.
> 
> However, I still think that this your proposal is (partly) the wrong way to
> go.  IMHO there should be one ReverseLookup instance per DataSet, and the
> bookkeeping should be done in the Commands.  This would fix pretty much all of
> the problems I currently see:
> 
> 1) Now that ReverseLookup is static, it accumulates a lot of dead entries.
> E.g. load an osm file with reverse-lookup=always and delete the layer
> afterwards; ReverseLookup will still contain backreferences for all the nodes.
> This can possibly be fixed by a) pruning the cache after layer manipulations,
> or b) keeping the cache local to the data set (so that it can be garbage
> collected).

That's fine, as long as we limit changing to objects to happening inside
of commands.  I'll look into this.  Having the cache local to the
dataset seems the most clean and efficient way of doing it to me.

One minor thing is how to get the layer data itself into the commands.
Should we make Command.executeCommand() take a layer on which to act?
Can an OsmPrimitive appear in more than one layer at a time?

I'll send out another patch once I work through this command stuff.

> 2) Deleting a way doesn't remove it from the cache.  E.g. add two nodes and a
> way, then remove the way; ReverseLookup.waysUsingNode(<one of the two nodes>)
> will still contain the way.

That's odd.  here's Way.delete():

	@Override public void delete(boolean deleted) {
		super.delete(deleted);
		for (Node n : nodes) {
			if (deleted)
				n.removeFrom(this);
			else
				n.addTo(this);
		}
	}

Seems that if you call way.delete(), all of the nodes will get taken out
of the reverse lookup.


> 3) ChangeCommand will produce entries to ways that are not in the data set.
> E.g. change the tags of a way; ReverseLookup.waysUsingNode() will contain both
> the way that is in the data set and the way that is stored in
> ChangeCommand.newOsm.

I think ChangeCommand is bogus.  We need more inspection when we modify
objects than just bulk-replacing their data members.  I've just
completed removing its last use in my tree.  

> == Fallible commands ==
> 
> > * change Commands to be able to return success/failure, which will
> > * allow SequenceCommand() to detect errors, and roll back all operations
> >   which were performed before that one errored out
> 
> Is there a use case for this?  I have been searching the list archives and
> couldn't find any.  (The changes are fine, I'd just like to know what problem
> they were meant to solve.)

It was originally to fix ChangeCommand being stupid to detect when
multiple ChangeCommands conflicted and back them out nicely.  Right now,
I just consider it proper error handling.  This is especially important
with respect to SequenceCommand since any inconsistency could cause it
to do some really unexpected things.

> AFAICT only RemoveNodeInWayCommand and ReplaceNodeInWayCommand can actually
> fail.  And I suppose that failure means that there's a bug in the calling
> code.  If that's the case, wouldn't it be better to fail loudly instead of
> silently rolling back the current command?

Yeah, that's a good idea.  Should we introduce a Main.error() for stuff
like this?

-- Dave