[Potlatch-dev] Introducing Magic Roundabout tool

[Andy Allan]

On Thu, Feb 24, 2011 at 10:47 PM, Steve Bennett <stevagewp at gmail.com> wrote:

> Whoops, sorry about that. All fixed now.

Cheers.

> I've discovered some slight flakiness, in that occasionally it
> produces a bad result, and if you try to undo, you get an exception.
> I'll need to understand a bit more how to use the undo system.

Regarding the Undo system - while we all hail randomjunk as the only
one who Truly Understands how it works, I've been toiling away as an
apprentice for a while now[1] and pretty much have got to grips with
it. I've taken a few hours this morning to put together a wiki page
with some documentation, that as it says at the top, may or may not
help.

http://wiki.openstreetmap.org/wiki/Potlatch_2/Developer_Documentation/undo

I'd suggest reading that - if by magic it's comprehensible, the next
bit of this email might make more sense.

As for your code, the two^Wthree^Wfour things I can spot straight away are:

1) In DrawWay.as.magicRoundabout() you're adding two different things
to the undo stack - the first and last lines are adding a composite
action called "undo" that doesn't actually do anything. The middle
line asks MagicRoundabout to return its resulting action to the
MainUndoStack. So two ways of achieving similar goals would be:

new MagicRoundabout(currentNode(), elastic.length,
MainUndoStack.getGlobalStack().addAction);

OR

var undo:CompositeUndoableAction = new CompositeUndoableAction("Magic
roundabout");
new MagicRoundabout(currentNode(), elastic.length, undo.push);
MainUndoStack.getGlobalStack().addAction(undo);

... but since the second one would be creating a composite action, and
pushing only one action onto its list, it's a bit redundant.

2) MagicRoundabout.as#30 calls performAction multiple times - instead
you could create a compositeaction, add the new way and all the
splitways to it, and send it back in one go.

3) Assuming you do 2), then when magicroundabout finishes adding
SplitWayActions it'll need to add a MakeJunctions action, to the list
too. But MakeJunctions accesses the undostack directly (line 40) so it
breaks the undo chain. MakeJunction should be rewritten to take a
performAction function and instead of doing things itself, pass the
actions back to the caller.

4) MakeJunction calls MainUndoStack...addAction() in a loop - that'll
add a separate undo step for every junction. Since only one keypress
causes MakeJunction, it should be undoable in one button press.
MakeJunction should have its own composite action and push things onto
it when it loops round like this, then passes just one action back to
the caller.

Oh, and finally, I suspect some of these composite actions should be
in net/systemeD/connection/actions - especially MakeJunction since
you've now shown with MagicRoundabout that it's a useful too for other
actions to build on top of.

Right, well, if this all turns out to be gobbldy-gook don't worry too
much - I'm happy to rewrite them when I get a chance. If you want to
give it a go yourself I'm 100% happy to explain further or answer
questions.

Cheers,
Andy


[1] Like, years, man