[josm-dev] Jumbo Patch

[Gabriel Ebner]

On Sun, Dec 16, 2007 at 09:33:14PM +0000, Gervase Markham wrote:
> Gabriel Ebner wrote:
> > You'll end up having to change the callers anyhow, as they assume a certain
> > behavior of how primitives work.
> 
> Then that's your mistake; those assumptions should be documented, or 
> unnecessary :-)

*My* mistake?  I wasn't even around yet when these decisions were made. :-)

But you're right.  There should be a JOSM internals manual somewhere, but I
guess it'll share the fate of the javadocs:  Someone will change the code but
not the documentation.  In fact I'd take the comments and even the javadocs as
nothing more than a starting point, we've got some pretty scary and misleading
stuff in JOSM:

    /**
     * Split a way into two or more parts, starting at a selected node.
     * 
     * FIXME: what do the following "arguments" refer to?
     * @param way the way to split
     * @param nodes the node(s) to split the way at; must be part of the way.
     */
    private void splitWay() {

> Right. So the correct answer is to make adding nodes to a Way and to the 
> undo/redo list an atomic operation, if the two must be coupled together.
> 
> Ignoring for the moment the fact that undoRedo is a "global", it might 
> go something like this:
> 
> Way wnew = new Way(w);
> wnew.replaceNodes(newNodes);
> 
> class Way {
>    ...
> 
>    function replaceNodes(nodeList newNodes) {
>      Way oldway = this.clone();
>      this.nodes = newNodes;
>      Main.main.undoRedo.add(new ChangeCommand(oldway, this));
>      ...
>    }
> }

I didn't tell you the whole story.  If you want to have two modifications as a
single undo item, you've got to put them into a SequenceCommand:

  Command a = ...;
  Command b = ...;
  Main.main.undoRedo.add(new SequenceCommand(a, b));

The catch is that if you change a way twice, the second change will overwrite
the first one.  (So you've got to remember what you've changed -- and that's
the main reason we don't have helper functions like replaceNodes() already.)

> [ concurrent modification & shared mutable state ]

User code isn't supposed to modify the primitives from the data set anyhow.
Unfortunately we are mutation to generate partially different primitives:

  // Create a clone of the way with a node appended.
  Way wnew = new Way(w);
  w.nodes.add(n);

That's it.  From then on, user code shouldn't (and does not) modify the Way
anymore.  It will only be modified by *Command after that.

So we actually have quasi readonly objects; in that sense we already have de
facto private data with accessors.

  Gabriel.