[Potlatch-dev] Introducing Magic Roundabout tool

[Steve Bennett]

On Sat, Feb 26, 2011 at 2:52 AM, Dave Stubbs <osm.list at randomjunk.co.uk> wrote:
> Yeah, what we have is a nested and OO implementation of a simple stack
> with markers. This allows you to do cool stuff like encapsulate
> complex actions without bothering the user with extra undo steps or
> adding lots of boiler plate code around the "RecordAction" stage.

Don't get me wrong, I think the implementation is fantastic. The only
thing I'm commenting on is some of the unintuitive naming and
mechanics.

For example, passing a function to an operation, where the function
represents a storage operation for an undoable action...is not very
intuitive. In practice, exactly one of two things gets passed to those
functions: either MainUndoStack.getInstance().push, or some
CompositeUndoableAction.addAction. It would therefore be possible to
replace declarations like this:

public function createWay(tags:Object, nodes:Array,
performCreate:Function):Way {
            var way:Way = new Way(nextNegative, 0, tags, true, nodes.concat());
            performCreate(new CreateEntityAction(way, setWay));
...

with

public function createWay(tags:Object, nodes:Array, actionStack:
CompositeUndoableAction=null):Way {
            var way:Way = new Way(nextNegative, 0, tags, true, nodes.concat());
            recordAction(actionStack, new CreateEntityAction(way, setWay));
...

where recordAction is:

protected function recordAction(stack: CompositeUndoAbleAction,
action: Action):void {
  if (stack)
    stack.push(action)
  else
    MainUndoStack.getInstance().push(action);
}

Personally, I think passing either a composite action stack, or null,
to an operation is a lot more intuitive than passing a function. And
it would make some simpler:

w = createWay({}, nodes);

Now, the fact that it's creating an action which is being placed on
the main undo stack is totally invisible. Magic behind the scenes, and
all that.

Steve