On Thu, Feb 24, 2011 at 10:47 PM, Steve Bennett <[email protected]> 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 _______________________________________________ Potlatch-dev mailing list [email protected] http://lists.openstreetmap.org/listinfo/potlatch-dev
