Potlatch 2/Developer Documentation/undo

From OpenStreetMap Wiki
Jump to navigation Jump to search

The undo system in potlatch2 is both simple and straightforward, and also a confusing complicated nightmare. Here is some documentation that may or may not help.

The basics

We want potlatch2 to be as user-friendly as possible, and that means that if anyone does something unintentionally, they should be able to press undo and everything sorts itself out. In practice this means that any changes to any Entity must go through the Undo system.

You will want to read this in conjunction with the class reference documentation. Generate them yourself with "ant docs" or read them online at http://random.dev.openstreetmap.org/potlatch2/docs/

Actions

CompositeUndoableActions (in white) are one or more undoable actions - but in ultimately are a series of UndoableEntityActions (in green)

An action is a bunch of changes that correspond to one press of the undo button. That could be changing the tag on a way, or pressing the quadrilaterawhatsit button, moving a node, and so on.

There are two types of UndoableAction

A Composite action doesn't actually make any changes itself - it just figures out what list of changes are going to be needed to meet the overall goal. In the end, all the branches of an action are ones that directly affect an Entity.

There are lots of actions already defined for you. By now we probably have every entity action that's required, and there's a dozen or so useful composite actions too. You can read which ones are available by following the links to subclasses of the different types of action, or have a look in net/systemeD/halcyon/connection/actions in the source code.

How do you actually do things then?

When you have either an entity or a composite action and you want it to happen, what do you do? Well, you need to send the action to MainUndoStack.getGlobalStack.addAction(action). This does two things

  • adds it to the record (stack) of what actions have been done (so they can be undone, if needs be)
  • calls the doAction() method on your action. On a composite action, the doAction in turn calls doAction on all the actions in the list.

How do you actually undo things then?

There's very few occasions where your code needs to undo something that has happened previously - it's usually the user who presses the undo button. So instead you just need to make sure your actions will undo themselves cleanly.

When you're writing an action, it depends on which type:

  • For an UndoableEntityAction - there's no getting away from it, you have to write the nitty-gritty of getting from B back to A
  • For a CompositeUndoableAction - most times you don't need to worry - by default action.undoAction simply runs through the list of actions, backwards, undoing each in turn. This cascades all the way down to the entity actions, which will have a functioning undoAction method.

Pause here and think things over

Trust me, this is a good place to stop reading for a bit, and get your head around the above.

Spotting things in the code

There's a few ways of representing things scattered round the code, and sometimes this can be confusing. Here's a few worked examples.

CreateAndAddNode

Here's a simple example from DrawWay.as - during a click we want to create a new node and add it to the way. There's two different entities affected (the new node, and the existing way needs changing) so it's a composite action. Because this happens in a few places there's a helper function

public function createAndAddNode(event:MouseEvent, performAction:Function):Node
        var undo:CompositeUndoableAction = new CompositeUndoableAction("Add node");  // here the new composite action is created, and given a name. These show up in debugging
	
        var lat:Number = controller.map.coord2lat(event.localY); // (figure out where the node actually is)
        var lon:Number = controller.map.coord2lon(event.localX);  
        var node:Node = controller.connection.createNode({}, lat, lon, undo.push);  // ask the Connection to create a node. Rather than creating it there and then, 
                                                                                                                                            // It'll pass back the Action object to whereever you ask it to. In this case we want to 
                                                                                                                                            // add the action to our composite so we ask it to pass the action to undo.push
        appendNode(node, undo.push); // we also need to add the changes to the way - appendNode just figures out which end. Again, the action ends up being passed to undo.push
			
        performAction(undo);  // we now take our action (undo) and send it back to whereever it was requested to go.
        return node;
}

The code that calls this function looks like this

  // ... just had a mouseEvent to add a new node
  node = createAndAddNode(event, MainUndoStack.getGlobalStack().addAction); // call the function above, and ask it to add the action to the undo stack (which remember also triggers it to happen)
  controller.map.setHighlight(node, { selectedway: true }); // do some more gui stuff that doesn't involve changing the entities themselves
  controller.map.setPurgable([node], false); // do some more stuff that doesn't change the node itself
  // ... etc

Obscure coding?

At first (heck, even after a while) the coding can seem confusing, since it involves a lot of passing around functions and things written in slightly different ways.

MainUndoStack.getGlobalStack().addAction(undo);                                       // when you've got an action called 'undo', and now you want to get it done
someFunctionCall(entity, ..., MainUndoStack.getGlobalStack().addAction); // when you're reacting to a user-input, and want the resulting action to be done immediately, just as
                                                                                                                               // soon as someFunctionCall has figured out *what* needs doing
                                                                                                                              // Doing this twice in the same function would result in two presses of the undo button to revert it.
someFunctionCall(entity, ..., undo.push);                                                        // like the above, but rather than doing it immediately you add it to your composite action called undo. 
                                                                                                                             // You'll call MainUndoStack.addAction(undo) when you're ready
performAction(undo); // you've been given a function (and called it performAction), but to be honest you've no idea whether that function is either 
                                      // a) adding to a different composite via push or b) going to MainUndoStack.getGlobalStack().addAction - and franky you don't care in this function.
                                      // So now that you have your action, everything is done, send it back up the chain.

Variable and function naming

There's two conventions that don't help much in understanding, but they are widely used

  • Calling CompositeUndoableActions 'undo' - You need to remember that undo is just an Action that you are adding things to, it's not actually undoing anything.
  • MainUndoStack.getGlobalStack().addAction - not only does it add the action to the stack, it calls doAction too. So maybe addActionAndDoActionAndThisActionWillBeWhatGetsUndoneNextTimeSomeonePressesUndo() ?

A note about delayed actions

When you are writing a complex composite action - e.g. during your coding of "override doAction()" - it's worth bearing in mind that no entities are being changed at that point. If you were trying to add a node to a way and write something like

trace(way.length);
way.insertNode(7, myNode, push);
trace(way.length);
...
super.doAction();

... then the two trace statements are going to show you the same result. This is because you've not actually added the node between them, you've just added the AddNodeToWay action onto the list of things to do later. Although usually irrelevant, this becomes important when iterating over things, and sometimes tricks like iterating backwards over the list become necessary. You can see the ultimate in juggling while working out relations handling during SplitWayAction since you need to insert the new way perhaps multiple times into a given relation, but you need to work out all the insert indexes up-front before any actual inserting is done.