From 1616fc70ee9df89a4786f52db81375b4efce8693 Mon Sep 17 00:00:00 2001 From: Steve Bennett Date: Sun, 13 May 2012 15:59:42 +1000 Subject: [PATCH] Fix bugs related to single-node ways being generated in a few places. I also merged the very similar RemoveNodeFromWayAction and RemoveNodeByIndexAction into a rewritten RemoveNodeFromWayAction. There's lots of fiddliness in this stuff to do with the undo model, which necessitates writing actions "in the future", and also various signals that can get bounced back and forth: removing a node might necessitate removing the way, which in turn calls the node removal code. There's also the trickiness that sometimes single-node ways should exist, especially during user interaction like drawing a way, or replacing a node with another one during a merge. I preserved the behaviour that RemoveNodeFromWayAction also cleans out repeated nodes first. This shouldn't really be there, but at least it's a separate code block, and can be removed at some future point. There's also a bit of bonus source code commenting. --- net/systemeD/halcyon/connection/Way.as | 8 +- .../connection/actions/DeleteWayAction.as | 6 +- .../actions/RemoveNodeByIndexAction.as | 52 ------ .../actions/RemoveNodeFromWayAction.as | 159 +++++++++++++----- .../connection/actions/ReplaceNodeAction.as | 2 +- .../connection/actions/UnjoinNodeAction.as | 2 +- net/systemeD/potlatch2/controller/DrawWay.as | 10 +- .../potlatch2/controller/SelectedWay.as | 7 + .../potlatch2/controller/SelectedWayNode.as | 16 +- 9 files changed, 156 insertions(+), 106 deletions(-) delete mode 100644 net/systemeD/halcyon/connection/actions/RemoveNodeByIndexAction.as diff --git a/net/systemeD/halcyon/connection/Way.as b/net/systemeD/halcyon/connection/Way.as index 0cca3122..9bf6de85 100644 --- a/net/systemeD/halcyon/connection/Way.as +++ b/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(); } diff --git a/net/systemeD/halcyon/connection/actions/DeleteWayAction.as b/net/systemeD/halcyon/connection/actions/DeleteWayAction.as index 59d91a30..ce14e1f9 100644 --- a/net/systemeD/halcyon/connection/actions/DeleteWayAction.as +++ b/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? diff --git a/net/systemeD/halcyon/connection/actions/RemoveNodeByIndexAction.as b/net/systemeD/halcyon/connection/actions/RemoveNodeByIndexAction.as deleted file mode 100644 index 9f2303da..00000000 --- a/net/systemeD/halcyon/connection/actions/RemoveNodeByIndexAction.as +++ /dev/null @@ -1,52 +0,0 @@ -package net.systemeD.halcyon.connection.actions { - - import net.systemeD.halcyon.connection.*; - - public class RemoveNodeByIndexAction extends UndoableEntityAction { - - private var way:Way; - private var nodeList:Array; - private var index:uint; - private var removed:Array; - private var fireEvent:Boolean; - - public function RemoveNodeByIndexAction(way:Way, nodeList:Array, index:uint, fireEvent:Boolean=true) { - super(way, "Remove node " + nodeList[index].id + " from position " + index); - this.way = way; - this.nodeList = nodeList; - this.index = index; - this.fireEvent = fireEvent; - } - - public override function doAction():uint { - var preceding:Node=(index>1) ? nodeList[index-1] : null; - var node:Node=nodeList[index]; - removed=[]; - - while (nodeList[index]==node || (nodeList[index]==preceding && preceding!=null)) { - var removedNode:Node=nodeList.splice(index, 1)[0]; - removed.push(removedNode); - if (nodeList.indexOf(removedNode)==-1) { removedNode.removeParent(way); } - if (fireEvent) { - entity.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_REMOVED, removedNode, way, index)); - } - } - way.deleted = nodeList.length == 0; - markDirty(); - return SUCCESS; - } - - public override function undoAction():uint { - for (var i:int=removed.length-1; i>=0; i--) { - nodeList.splice(index, 0, removed[i]); - removed[i].addParent(way); - if (fireEvent) { - entity.dispatchEvent(new WayNodeEvent(Connection.WAY_NODE_ADDED, removed[i], way, index)); - } - } - way.deleted = nodeList.length == 0; - markClean(); - return SUCCESS; - } - } -} \ No newline at end of file diff --git a/net/systemeD/halcyon/connection/actions/RemoveNodeFromWayAction.as b/net/systemeD/halcyon/connection/actions/RemoveNodeFromWayAction.as index cd3007b6..cd11c9d6 100644 --- a/net/systemeD/halcyon/connection/actions/RemoveNodeFromWayAction.as +++ b/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; } - } + } } diff --git a/net/systemeD/halcyon/connection/actions/ReplaceNodeAction.as b/net/systemeD/halcyon/connection/actions/ReplaceNodeAction.as index 80445e97..5451d813 100644 --- a/net/systemeD/halcyon/connection/actions/ReplaceNodeAction.as +++ b/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