[josm-dev] Jumbo Patch

[Gabriel Ebner]

On Fri, Dec 21, 2007 at 12:06:14PM -0800, Dave Hansen wrote:
> I think ChangeCommand sucks (and by implication, the wnew approach).

ChangeCommand has one nice thing going for it, it's just one single command.
Anything else would require at the very least 4 commands (names are just
examples):

  - ChangeTagsCommand
  - ChangeCoordinatesCommand
  - ReplaceWayNodesCommand
  - ReplaceRelationMembersCommand

And I guess this is too coarse too.  So probably more are needed.  And hitting
undo will wreak havoc if there's just a single bug in these commands.

The commands have historically been a very stable piece of code.
ChangeCommand's last significant change took place in April 2006.

> It's way too coarse, and I'd like to make it basically invalid to
> perform on ways unless it's applied atomically with its creation.

Did I miss anything in the patches?  You still gather all the commands up in a
SequenceCommand and then apply them long after you've created them.  You don't
apply them atomically with their creation, but rather apply more fine-grained
modifications when they're executed.

I had suggested applying the commands immediately upon creation somewhere in
the thread, so Main.ds would actually represent the current state of the
world, but it didn't seem to have caught on.

> Ways just have too much state, and can get too easily made inconsistent.

Groping in the dark for the light switch is still groping in the dark even if
you can only turn the switch one way.  You can still have two commands
canceling each other out when performed in one order and reinforcing
themselves in another order.  You won't get around knowing the current state
of the data set when creating a command.  Creating a way and adding a node to
it works only in one order, not in the other.

> As for leaking references, having any kind of reverse lookup system will
> make them more likely.  But, those are bugs that I am very willing to
> fix.  

I was not talking about more likely.  I was talking about definite.

E.g. you need to remove the back-references when deleting a way, else the
back-references will not only be wrong, they will also prevent the way from
being garbage collected, creating a leak.  The same when undoing the addition
of a way.

Not to mention that this will need all sorts of hairy dispose() calls (or
clearAllNodes() if you prefer) at least in MergeVisitor.  This is one of the
places were changes don't go through the undoRedo list, but are applied
directly.  (And no, making the changes through the undoRedo list is not an
option, as that would set the modified bits and make JOSM upload all the
merged primitives.)

What's wrong with a good old hash table in DataSet that you can remove or
rebuild when necessary?

  Gabriel.