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

[Gabriel Ebner]

On Fri, Feb 01, 2008 at 03:21:18PM -0800, Dave Hansen wrote:
> > 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?

For the time, I have made the TestError constructor take an extra List<?>,
which you can use to override what is highlighted.  That way, you can not only
highlight just way segments, but also just the interesting parts of an error,
e.g. when a motorway intersects a residential road, you can highlight just the
intersection.

> > > * 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:

Oops.  Sorry, I had been looking for DuplicateNode*s*.

> > 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 is the just same as using a modification counter (which takes much less
space):

  private int mods = 0;

  public void setUnmodified() { mods = 0; }
  public boolean isModified() { return mods != 0; }
  public void modify() { mods++; }
  public void unmodify() { mods--; }

  // Although it'd be more JOSM-like if we'd just skip the accessors and
  // simply write osm.mods++;  Sed de gustibus non est disputandum...

Hmmm, this would probably break if we were to allow undo of changes that have
already been uploaded.  But even so, it seems worth a try since it would allow
us not to keep any backup in most commands.  (if you're still thinking about
reducing memory)

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

It does have some sort of appeal to have "versioned" objects that you can
modify without worrying about commands; but this means that you lose the
ability to group changes or to even know what the last change was.

If you just want to simplify some of the commands, you could do a superclass
that keeps track of the modified flag.

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

In fact, that's what ChangeCommand does right now.  But we don't actually need
to keep a copy a copy of everything, we'd just need to store the modified bit.

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

Yes, OsmPrimitives are only modified using Commands.  Except for merges and
loading from files that is, but I think we won't get around rebuilding the
index in these cases anyway.

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

We actually already need the layer parameter, for AddCommand:

  private final OsmPrimitive osm;

  public AddCommand(OsmPrimitive osm) {
    this.osm = osm;
    this.ds = Main.main.editLayer().data;
  }

Note that it takes the current data layer at the time of *creating* the
command.  JOSM currently only has a single undo list for all layers, so a
command could actually be undone while another layer is active.

So we probably need the constructor to take the OsmLayer parameter.

> Can an OsmPrimitive appear in more than one layer at a time?

No.  (Well, it does during a merge, but no commands are executed at that
time.)

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

Sorry once again.  I already sent out a mail about that.

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

You can still bulk-replace an object, you just have to create commands for all
its data fields.  :-)

But what you cannot do anymore is to inadvertently modify status bits like
selected, deleted, modified.  Which is a good idea since there actually was a
bug because some code was modifying the selection status via commands.

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

In this case is might be worthwhile to actually check whether the index is
valid in ReplaceNodeInWayCommand, AddNodeToWayCommand, ...

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

I'm afraid that just popping up a dialog with the sole message being "Cannot
find node in ReplaceNodeInWay" is going to be pretty useless.  Ideally, a
stack trace like you'd get for an uncaught exception would be great.  However
I don't see a way to get such a stack trace in Java without actually throwing
an exception.  (Or maybe how about really throwing an exception, making
SequenceCommand do a rollback if an exception occurs, and letting
the BugReportExceptionHandler handle all our popup needs?)

  Gabriel.