Fix bugs related to single-node ways being generated in a few places (#4378)

net/systemeD/halcyon/connection/Way.as

@@ -126,11 +126,11 @@ package net.systemeD.halcyon.connection {
 		}
 
 		public function removeNode(node:Node, performAction:Function):void {
-			performAction(new RemoveNodeFromWayAction(this, node, nodes));
+            performAction(new RemoveNodeFromWayAction(this, nodes, node));
 		}
 
-        public function removeNodeByIndex(index:uint, performAction:Function, fireEvent:Boolean=true):void {
-            performAction(new RemoveNodeByIndexAction(this, nodes, index, fireEvent));
+        public function removeNodeByIndex(index:uint, performAction:Function, fireEvent:Boolean=true, allowSingleNodeWays: Boolean=false):void {
+            performAction(new RemoveNodeFromWayAction(this, nodes, null, index, fireEvent, allowSingleNodeWays));
         }
 
 		public function sliceNodes(start:int,end:int):Array {
@@ -139,7 +139,7 @@ package net.systemeD.halcyon.connection {
 
         public function deleteNodesFrom(start:int, performAction:Function):void {
             for (var i:int=nodes.length-1; i>=start; i--) {
-              performAction(new RemoveNodeByIndexAction(this, nodes, i));
+              performAction(new RemoveNodeFromWayAction(this, nodes, null, i));
             }
             markDirty();
         }

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

@@ -11,7 +11,7 @@ package net.systemeD.halcyon.connection.actions {
         public function DeleteWayAction(way:Way, setDeleted:Function, nodeList:Array) {
             super(way, "Delete");
             this.setDeleted = setDeleted;
-            this.nodeList = nodeList;
+            this.nodeList = nodeList; // reference to way's actual nodes array. 
         }
 
         public override function doAction():uint {
@@ -24,8 +24,10 @@ package net.systemeD.halcyon.connection.actions {
 			way.suspend();
 			way.removeFromParents(effects.push);
 			oldNodeList = nodeList.slice();
+			// Delete or detach each node
 			while (nodeList.length > 0) {
-				node=nodeList.pop();
+
+				node=nodeList.pop(); // do the actual deletion
 				node.removeParent(way);
 				way.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_REMOVED, node, way, 0));
                 if (!node.hasParents && !node.hasInterestingTags()) { //need to trigger redraw of new POIs?

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

@@ -3,59 +3,140 @@ package net.systemeD.halcyon.connection.actions {
     import net.systemeD.halcyon.connection.*;
 
     public class RemoveNodeFromWayAction extends UndoableEntityAction {
-        private var node:Node;
-        private var nodeList:Array;
-        private var nodeRemovedFrom:Array;
-
-        public function RemoveNodeFromWayAction(way:Way, node:Node, nodeList:Array) {
-            super(way, "Remove node "+node.id+" from ");
-            this.node = node;
+
+        private var way:Way;          // synonym for entity
+        private var nodeList:Array;   // points to raw node list of the way, to actually remove nodes
+        private var index:int;       // index of the node we were asked to remove, or -1
+        private var node:Node;        // the node we were asked to remove
+        private var nodeRemovedFrom:Array;  // list of [node, index] pairs of nodes removed
+        private var fireEvent:Boolean;      // should we send a signal?
+        private var effects:CompositeUndoableAction; // possible side effects if we need to kill the way itself
+        private var allowSingleNodeWays:Boolean; 
+
+        /** Remove a node from a way. Specify either the node itself (all instances removed) or its index. 
+        * For (unverified) historical reasons it also removes any repeated nodes first.
+        * If removing this node leaves a 1-length way behind, the way is destroyed, if allowSingleNodeWays is false.
+        * (Sometimes, like when drawing a way, a single-node way is ok.)
+        * */
+        public function RemoveNodeFromWayAction(way:Way, nodeList:Array, node:Node = null, index:int = -1, fireEvent:Boolean=true, allowSingleNodeWays:Boolean=false) {
+            if (node == null)
+                node = nodeList[index];
+            super(way, "Remove node " + node.id + " from position " + index);
+
+            if (node != null && index != -1 && nodeList[index] != node) {
+                throw new Error("Node and index specified but inconsistent: nodes[" + index + "] = " + nodeList[index].id + "; node.id = " + node.id);
+            }
+
+            this.way = way;
             this.nodeList = nodeList;
+            this.index = index;
+            this.node = node;
+            this.fireEvent = fireEvent;
+            this.allowSingleNodeWays = allowSingleNodeWays;
         }
-            
+
         public override function doAction():uint {
+
+            function removeByIndex(idx: int):void {
+                var removedNode:Node=nodeList.splice(idx, 1)[0];
+                nodeRemovedFrom.push([removedNode, idx]);
+                if (nodeList.indexOf(removedNode) == -1) { removedNode.removeParent(way); }
+                if (fireEvent) {
+                   entity.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_REMOVED, removedNode, way, idx));
+                }
+
+            }
+            if (nodeList.indexOf(node) < 0)
+                return NO_CHANGE;
+
+            way.suspend();
+
             nodeRemovedFrom = [];
-			var i:int, node2:Node;
-			while ((i=nodeList.indexOf(node))>-1) {
-				// remove the node from the way
-				nodeList.splice(i,1);
-				nodeRemovedFrom.push([node,i]);
-				entity.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_REMOVED, node, Way(entity), i));
+            var adjustedindex:int = index;
+
+            // first, remove any repeated nodes, adjusting an offset as necessary.
+            // Ideally, we'd use way.removeRepeatedNodes(), but the undo action model kind of prevents that.
+            for (var i:int = 1; i < nodeList.length; i++) {
+                if (nodeList[i] == nodeList[i-1] && i != (adjustedindex)) {
+                    removeByIndex(i);
+
+                    if (adjustedindex > i) // we deleted an item to the left of the node we're going to remove
+                        adjustedindex --;
+                }
+            }
+
+            if (index > -1) { // handle "remove by index" case
+                removeByIndex(adjustedindex);
+                if (index > 0 && adjustedindex < nodeList.length && nodeList[adjustedindex - 1] == nodeList[adjustedindex]) {
+                    // removing a node created repeated nodes ( ABCBD minus C = ABBD, make it ABD)
+                    removeByIndex(adjustedindex);
+                } 
+            } else { // handle "remove all instances of node" case
+                for (i = 0; i < nodeList.length; i++) {
+                    if (nodeList[i] == node) {
+                        removeByIndex(i);
+                        if (i > 0 && i < nodeList.length && nodeList[i - 1] == nodeList[i]) {
+                            // same test as above
+                            removeByIndex(i);
+                        } 
+                    }
+                }
+            }
+
+            if (nodeList.length == 0)
+                way.deleted = true;
+
+            if (nodeList.length == 1 && !allowSingleNodeWays) {
+            	way.deleted = true;
+                // And if it's length 1, also do something about the remaining node.
+                var orphan:Node = nodeList[0]; 
+                removeByIndex(0); // (Length zero now.)
+                // this code duplicated from DeleteWayAction.
+                if (!orphan.hasParents && !orphan.hasInterestingTags()) {
+                    // the last node wasn't interesting after all, so destroy it.
+                    effects = new CompositeUndoableAction("Way deletion side effects.");
+                    orphan.remove(effects.push);
+                    effects.doAction();
+                } else {
+                    if (!orphan.hasParentWays) 
+                        orphan.connection.registerPOI(node); // it's now a POI
+                    // or it's already part of another way.
+                }
+                way.dispatchEvent(new EntityEvent(Connection.WAY_DELETED, way));    // delete WayUI
 
-				// remove any repeated nodes that have occurred as a result (i.e. removing B from ABA)
-				while (i>0 && nodeList[i-1]==nodeList[i]) {
-					node2=nodeList.splice(i,1)[0];
-					nodeRemovedFrom.push([node2,i]);
-					entity.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_REMOVED, node2, Way(entity), i));
-				}
-			}
-
-			if ( nodeRemovedFrom.length > 0 ) {
-			    node.removeParent(entity);
-			    entity.deleted = nodeList.length == 0;
-			    markDirty();
-			    return SUCCESS;
-			}
-
-            return NO_CHANGE;
-        }
+            }
+            markDirty();
+            way.resume();
 
+            return SUCCESS;
+        }
+
         public override function undoAction():uint {
             node.addParent(entity);
 
-            for (var i:int = nodeRemovedFrom.length - 1; i >= 0; i--) {
-                var removal:Array = nodeRemovedFrom[i];
-                var reinstate:Node = removal[0];
-                var index:int = removal[1];
-                nodeList.splice(index, 0, reinstate);
-                entity.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_ADDED, reinstate, Way(entity), index));
+            // re-add the node(s) that was/were removed earlier, in the correct sequence, and to the right index.
+            while (nodeRemovedFrom.length > 0) {
+                var node_index:Array = nodeRemovedFrom.pop();
+                var reinstate:Node = node_index[0];
+                var idx:int = node_index[1];
+                nodeList.splice(idx, 0, reinstate);
+                reinstate.addParent(way);
+                if (fireEvent) {
+                    entity.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_ADDED, reinstate, Way(entity), idx));
+                }
             }
+            if (effects) effects.undoAction();
 
-            entity.deleted = nodeList.length == 0;
+            // not sure what to do if we restore a 1-length way. Hrm.
+            if (way.deleted && nodeList.length > 1) {
+            	way.deleted = false;
+            	way.connection.dispatchEvent(new EntityEvent(Connection.NEW_WAY, way));
+            	// for symmetry, we ought to register node UIs here, but seems to perform correctly as is.
+            }
             markClean();
 
             return SUCCESS;
         }
-    }
+   }
 }
 

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

@@ -22,7 +22,7 @@ package net.systemeD.halcyon.connection.actions {
             for each (var way:Way in node.parentWays) {
               for (var x:uint=0; x<way.length; x++) {
                 if (way.getNode(x) == node) {
-                  way.removeNodeByIndex(x, push);
+                  way.removeNodeByIndex(x, push, true, true); // Must temporarily allow a one-length node
                   way.insertNode(x, replacement, push);
                 }
               }

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

@@ -28,7 +28,7 @@ package net.systemeD.halcyon.connection.actions {
                 var newNode:Node = way.connection.createNode(node.getTagsCopy(), node.lat, node.lon, push);
                 for (var i:int = 0; i < way.length; i++) {
                   if(way.getNode(i) == node) {
-                    way.removeNodeByIndex(i, push);
+                    way.removeNodeByIndex(i, push, true, true);// allow one-length nodes temporarily
                     way.insertNode(i, newNode, push);
                   }
                 }

net/systemeD/potlatch2/controller/DrawWay.as

@@ -12,8 +12,8 @@ package net.systemeD.potlatch2.controller {
 	public class DrawWay extends SelectedWay {
 		private var elastic:Elastic;
 		private var editEnd:Boolean;            // if true, we're drawing from node[n-1], else "backwards" from node[0] 
-		private var leaveNodeSelected:Boolean;
-		private var lastClick:Entity=null;
+		private var leaveNodeSelected:Boolean;  // leave the end node selected when we stop drawing?
+		private var lastClick:Entity=null;      // keep thing clicked on, and when
 		private var lastClickTime:Date;
 		private var hoverEntity:Entity;			// keep track of the currently rolled-over object, because
 												// Flash can fire a mouseDown from the map even if you
@@ -156,7 +156,7 @@ package net.systemeD.potlatch2.controller {
 			                          controller.map.coord2latp(controller.map.mouseY));
 		}
 
-        /* Fix up the elastic after a WayNode event - e.g. triggered by undo */
+        /** Fix up the elastic after a WayNode event - e.g. triggered by undo, adding/removing a node */
         private function fixElastic(event:Event):void {
             if (firstSelected == null) return;
             var node:Node;
@@ -250,11 +250,11 @@ package net.systemeD.potlatch2.controller {
 
 			if (editEnd) {
 				node=Way(firstSelected).getLastNode();
-				Way(firstSelected).removeNodeByIndex(Way(firstSelected).length-1, undo.push);
+				Way(firstSelected).removeNodeByIndex(Way(firstSelected).length-1, undo.push, true, true);
 				newDraw=Way(firstSelected).length-2;
 			} else {
 				node=Way(firstSelected).getNode(0);
-				Way(firstSelected).removeNodeByIndex(0, undo.push);
+				Way(firstSelected).removeNodeByIndex(0, undo.push, true, true);
 				newDraw=0;
 			}
 			// Only actually delete the node if it has no other tags, and is not part of other ways (or part of this way twice)

net/systemeD/potlatch2/controller/SelectedWay.as

@@ -110,6 +110,10 @@ package net.systemeD.potlatch2.controller {
 			selectedWay.remove(MainUndoStack.getGlobalStack().addAction);
 			return new NoSelection();
 		}
+
+		private function updateNodeHighlights(event:WayNodeEvent):void {
+			layer.setHighlightOnNodes(initWay, { selectedway: true });
+		}
 
         /** Officially enter this state by marking the previously nominated way as selected. */
         override public function enterState():void {
@@ -120,6 +124,8 @@ package net.systemeD.potlatch2.controller {
 	            selection = [initWay];
 	            controller.updateSelectionUI();
 	            initWay.addEventListener(Connection.WAY_REORDERED, updateSelectionUI, false, 0, true);
+                initWay.addEventListener(Connection.WAY_NODE_ADDED, updateNodeHighlights,
+                    false, 0, true); // if ways are re-added with undo, they need to show up.
 			}
 			layer.setPurgable(selection,false);
         }
@@ -132,6 +138,7 @@ package net.systemeD.potlatch2.controller {
             copyRelations(firstSelected);
 			layer.setPurgable(selection,true);
             firstSelected.removeEventListener(Connection.WAY_REORDERED, updateSelectionUI);
+            firstSelected.removeEventListener(Connection.WAY_NODE_ADDED, updateNodeHighlights);
             clearSelection(newState);
         }
 

net/systemeD/potlatch2/controller/SelectedWayNode.as

@@ -175,18 +175,30 @@ package net.systemeD.potlatch2.controller {
 			return new SelectedWay(parentWay);
 		}
 
+		/** Remove a node from this way, but don't necessarily delete it - unless it's boring and tagless. */
+		// TODO: add check for relations
+		// BUG: add check for 1-length way
 		public function removeNode():ControllerState {
 			if (firstSelected.numParentWays==1 && parentWay.hasOnceOnly(firstSelected as Node) && !(firstSelected as Node).hasInterestingTags()) {
 				return deleteNode();
 			}
 			parentWay.removeNodeByIndex(selectedIndex, MainUndoStack.getGlobalStack().addAction);
-			return new SelectedWay(parentWay);
+            layer.setHighlight((firstSelected as Node), { selectedway: false });
+			if (!parentWay.deleted)
+			     return new SelectedWay(parentWay);
+			else
+			     return new NoSelection(); // consider selecting the remaining node, if any.
 		}
 
+		/** Delete this node (and hence remove it from this way) */
+        // BUG: add check for 1-length way
 		public function deleteNode():ControllerState {
 			layer.setPurgable(selection,true);
 			firstSelected.remove(MainUndoStack.getGlobalStack().addAction);
-			return new SelectedWay(parentWay);
+            if (!parentWay.deleted)
+                 return new SelectedWay(parentWay);
+            else
+                 return new NoSelection(); // consider selecting the remaining node, if any.
 		}
 
         public function unjoin():ControllerState {