[josm-dev] Broken out jumbo patch

[Frederik Ramm]

Hi,

Patches applied:

01  applied  make-ChangePropertyCommand-take-single-object
02  changed  Give OsmDataLayer a containsPoint() function 
03  applied  clean up OsmPrimitive.equals()
04  not yet  way changes
05  applied  make commands able to fail
06  changed  DeleteCommand convenience constructor
07  applied  Command.getOrig
08  applied  RemoveRelationMemberCommand

rest: not yet

I'm still not 100% sure whether all your problems with the commands
are really due to bad design or just a misunderstanding.

You say:

> 1. ChangeCommand is bad.  Applying two to the same object causes bad
>    things to happen, and this happens *all* *the* *time*.

[...]

> 	way1 = new Way(way);
> 	way1.nodes.add(node2);
> 	c1 = new ChangeCommand(way, way1);
> 
> 	way2 = new Way(way);
> 	way2.nodes.add(node3);
> 	c2 = new ChangeCommand(way, way2);
> 
> 	c1.execute();
> 	c2.execute();

This is obviously not the way it is intended to be used. Either you
have to do this:

way1 = new Way(way);
way1.nodes.add(node2);
way1.nodes.add(node3);
c1 = new ChangeCommand(way, way1);
c1.execute();

or you have to do this:

way1 = new Way(way);
way1.nodes.add(node2);
c1 = new ChangeCommand(way, way1);
c1.execute();

// now re-read way from the DataSet - you will get the modified
// version that was "way1" above
 
way2 = new Way(way);
way2.nodes.add(node3);
c2 = new ChangeCommand(way, way2);
c2.execute();

I fail to see the use case for your example above. In what scenario
would I be forced to build up a stack of ChangeCommands which (a) act
on the same object but (b) somehow don't know that and thus cannot be
consolidated into one?

Let me guess: The validator iterating over a large amount of objects
and "fixing" things, and in the process touching the same way twice
for different reasons, and still wanting to put everything in one big
SequenceCommand in the end?

In that case. would it be too much to ask of the validator to simply
build a stack of before/after pairs of changed objects, like so...

HashSet<OsmPrimitive,OsmPrimitive> changed_objects = new ...

for (o : objects_that_need_to_be_changed) {
   OsmPrimitive originalObject = o;
   if (changed_objects.containsKey(o)) {
       originalObject = changed_objects.get(o);
   }
   OsmPrimitive newObject = originalObject.clone();
   changed_objects.put(o, newObject);

   // make changes to newObject
}

s = new SequenceCommand();
for (o : changed_objects.keySet()) {
   c = new ChangeCommand(o, changed_objects.get(o));
   s.add(c);
}

I don't say that your way of doing it is false but you seem to have
done an awful lot of work overthrowing the whole ChangeCommand stuff
when a simple mechanism as sketched above could have been used - or am
I overlooking something bigger?

> 2. Having smart commands like "replace this node in this way at this
>    place" is safe, verifiable, and easy to debug.

Yes but it also means that whenever you want to, say, replace two
nodes in that way, you have to extend that class - unless you still
allow using the generic ChangeCommand, which will then result in even
more difficulties for newcomers to JOSM because they will wonder why
some places use "InsertNodeIntoWay" and some just "ChangeCommand".

I'm also wary of the slippery slope. Some of your commands contain
internal SequenceCommands, so what you have provided here might only
be the beginning, and we'll soon see an
"InserNewNodeAtIntersectionOfTwoWaysAndConnectThemCommand". I know the
slippery slope argument is invalid most of times but could you
perhaps try and provide an idea about which kinds of operations
should, in your opinion, get their own commands and which should still
be done "manually" by putting together a SequenceCommand?

Bye
Frederik

-- 
Frederik Ramm ## eMail frederik at remote.org ## N49°00'09" E008°23'33"