[josm-dev] Jumbo Patch

[Gervase Markham]

Gabriel Ebner wrote:
> 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. :-)

Sorry; it's an idiom. When I say "That's your mistake", it really sort 
of means "That's the problem, right there". I'll try and be more precise 
in my language.

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

That would be nice, but I'm not even arguing for that. For an app of 
JOSM's size, you should be able to get by with a one-page "here's how 
the source is organized" document, plus JavaDoc on intelligent methods 
which encapsulate function within the objects.

So when I say "the assumptions should be documented", I mean something 
like "each function should document, in its JavaDoc, its assumptions, 
prerequisites etc.".

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() {

Indeed, this is not good.

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

But this logic should be inside the undoRedo system itself - when a new 
change comes in, it should decide whether to combine it with a previous 
change, and so on. So the way to to it is just undoRedo.add() all of 
your commands, but when you call undoRedo.undo(), it doesn't necessarily 
undo just a single command.

Gerv