Fix for #3860 (undo bug on create way)

net/systemeD/halcyon/connection/CompositeUndoableAction.as

@@ -105,6 +105,7 @@ package net.systemeD.halcyon.connection {
             var str:String = " {" + actions.join(",") + "}";
             return name + str;
         }
+
     }
 
 }

net/systemeD/halcyon/connection/MainUndoStack.as

@@ -18,6 +18,8 @@ package net.systemeD.halcyon.connection {
 
         private var undoActions:Array = [];
         private var redoActions:Array = [];
+        private var undorequests:int = 0;
+        private var redorequests:int = 0;
 
         /**
          * Performs the action, then puts it on the undo stack.
@@ -88,6 +90,10 @@ package net.systemeD.halcyon.connection {
             redoActions.push(action);
             dispatchEvent(new Event("new_undo_item"));
             dispatchEvent(new Event("new_redo_item"));
+            if (undorequests > 0) {
+            	undorequests --;
+            	undo();
+            }
         }
 
         /**
@@ -104,10 +110,15 @@ package net.systemeD.halcyon.connection {
 			}
 		}
 
-		public function removeLastIfAction(action:Class):void {
+        /**
+        * Remove (without undoing) the most recent action, but only if it's a particular class
+        * @param action The class of the previous action.
+        */
+		public function removeLastIfAction(action:Class):UndoableAction {
 			if (undoActions.length && undoActions[undoActions.length-1] is action) {
-				undoActions.pop();
+				return undoActions.pop() as UndoableAction;
 			}
+			return null;
 		}
 
         [Bindable(event="new_undo_item")]
@@ -134,6 +145,23 @@ package net.systemeD.halcyon.connection {
             undoActions.push(action);
             dispatchEvent(new Event("new_undo_item"));
             dispatchEvent(new Event("new_redo_item"));
+            if (redorequests > 0) {
+                redorequests --;
+                redo();
+            }
+
+        }
+
+        /** The cleanest solution to an ugly problem. Say an undo event X wants to call
+        * another undo event Y: if it calls it directly, they end up swapped
+        * in the undo history. This way, they can get called in the normal order. It's
+        * a way to loosely chain two events together. */ 
+        public function requestUndo():void {
+            undorequests = undorequests + 1;
+        }
+
+        public function requestRedo():void {
+            redorequests = redorequests + 1;
         }
 
     }

net/systemeD/halcyon/connection/actions/AddNodeToWayAction.as

@@ -34,6 +34,7 @@ package net.systemeD.halcyon.connection.actions {
             nodeList.splice(index, 0, node);
             markDirty();
 			way.expandBbox(node);
+
             way.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_ADDED, node, way, index));
 
             return SUCCESS;
@@ -57,14 +58,10 @@ package net.systemeD.halcyon.connection.actions {
 			markClean();
             way.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_REMOVED, removed[0], way, index));
 
-			// delete way if it's now 1-length, and convert the one remaining node to a POI
+			// If it's now 1-length, we want to delete the way and convert the one remaining node to a POI.
+			// We can't do this directly, so request the MainUndoStack to do it.
 			if (autoDelete && way.length==1) {
-				way.setDeletedState(true);
-				way.dispatchEvent(new EntityEvent(Connection.WAY_DELETED, way));
-				firstNode=way.getNode(0);
-				firstNode.removeParent(way);
-				if (!firstNode.hasParentWays) firstNode.connection.registerPOI(firstNode);
-				MainUndoStack.getGlobalStack().removeLastIfAction(BeginWayAction);
+				MainUndoStack.getGlobalStack().requestUndo();
 			}
 			return SUCCESS;
         }

net/systemeD/halcyon/connection/actions/BeginWayAction.as

@@ -2,12 +2,55 @@ package net.systemeD.halcyon.connection.actions {
 
     import net.systemeD.halcyon.connection.*;
 
-    /* This is needed so that the specific type of CUA can be detected when CreatePOIAction is called */
+    /** The action of starting drawing a way. It's a messy action to define because the
+    * user deals with nodes rather than ways, and we don't want an undo step that is
+    * invisible. Going forwards (redo), the redo of the following node is triggered
+    * automatically. Going backwards (undo), the conversion of the last node into a 
+    * POI happens automatically. In some ideal universe, both this step and the
+    * creation of either the preceding or following node would be bundled together
+    * in one CUA, but it turns out to be very hard to implement that way. */
     public class BeginWayAction extends CompositeUndoableAction {
 
-        public function BeginWayAction(){
+        private var firstNode:Node;
+        private var newWay:Way;
+        private var connection:Connection;
+
+        public function BeginWayAction(connection:Connection, firstNode: Node){
           super("Begin Way Action");
+          this.connection = connection;
+          this.firstNode = firstNode;
+        }
+
+        public override function doAction():uint {
+          if (newWay == null) {
+            //newWay = connection.createWay({}, [firstNode], push);
+            // we create the way, then add the node to it. doing it in one step
+            // seemed to cause the undo process to delete the node with the way.
+            // A bug in DeleteWayAction.doAction()?
+            newWay = connection.createWay({}, [], push);
+            newWay.appendNode(firstNode,push);
+          } else {
+            // This is a redo. Way creation is an invisible step, so we request
+            // that the next step (the next node) gets redone as well.
+            MainUndoStack.getGlobalStack().requestRedo();
+          }
+
+          super.doAction();
+          connection.sendEvent(new EntityEvent(Connection.NEW_WAY, newWay), false);
+          connection.unregisterPOI(firstNode);
+          return SUCCESS;
+        }
+
+        public override function undoAction():uint {
+          super.undoAction();
+          connection.registerPOI(firstNode);
+          return SUCCESS;
+        }
+
+        public function getWay():Way {
+          return newWay;
         }
+
 
     }
 }

net/systemeD/halcyon/connection/actions/CreatePOIAction.as

@@ -12,13 +12,18 @@ package net.systemeD.halcyon.connection.actions {
 		private var lon:Number;
 		private var connection:Connection;
 
-        public function CreatePOIAction(connection:Connection, tags:Object, lat:Number, lon:Number) {
+        public function CreatePOIAction(connection:Connection, tags:Object, lat:Number, lon:Number, node: Node=null, nodeCreation: CreateEntityAction=null) {
           super("Create POI");
           this.connection = connection;
           this.tags = tags;
           this.lat = lat;
           this.lon = lon;
+          if (node) {
+            this.newNode = node;
+            push(nodeCreation);
+          }
         }
+
 
         public override function doAction():uint {
           if (newNode == null) {

net/systemeD/potlatch2/controller/DrawWay.as

@@ -200,6 +200,7 @@ package net.systemeD.potlatch2.controller {
 
 		protected function keyExitDrawing():ControllerState {
 			var cs:ControllerState=stopDrawing();
+			// If they've only drawn one node, discard it and the attached way with no remorse. 
 			if (selectedWay.length==1) { 
 				if (MainUndoStack.getGlobalStack().undoIfAction(BeginWayAction)) { 
 					return new NoSelection();

net/systemeD/potlatch2/controller/NoSelection.as

@@ -25,16 +25,25 @@ package net.systemeD.potlatch2.controller {
 
 			if (event.type==MouseEvent.MOUSE_UP && (focus==null || (paint && paint.isBackground)) && map.dragstate!=map.DRAGGING && map.dragstate!=map.SWALLOW_MOUSEUP) {
 				map.dragstate=map.NOT_DRAGGING;
-				// ** FIXME: BeginWayAction ought to be a discrete class
-				var undo:CompositeUndoableAction = new BeginWayAction();
 				var conn:Connection = layer.connection;
-				var startNode:Node = conn.createNode(
-					{}, 
-					controller.map.coord2lat(event.localY),
-					controller.map.coord2lon(event.localX), undo.push);
-				var way:Way = conn.createWay({}, [startNode], undo.push);
-				MainUndoStack.getGlobalStack().addAction(undo);
-				return new DrawWay(way, true, false);
+
+				// User just created a node...
+				var nodeAction:CreatePOIAction = new CreatePOIAction(
+				    conn, 
+				    {}, 
+				    controller.map.coord2lat(event.localY), 
+				    controller.map.coord2lon(event.localX));
+
+				MainUndoStack.getGlobalStack().addAction(nodeAction);
+
+				// And a way. See BeginWayAction doco for why we keep these separate.
+				var wayAction:BeginWayAction = new BeginWayAction(
+				    layer.connection,
+				    nodeAction.getNode());
+
+                MainUndoStack.getGlobalStack().addAction(wayAction);
+
+				return new DrawWay(wayAction.getWay(), true, false);
 			}
 			return this;
 		}