Rewrite behaviorDrawWay to only always use one and only one temporary edit

Fix issues involving undoing while drawing ways
Prevent self-intersection when adding nodes to ways on touch devices (close #7423)

modules/behavior/draw_way.js

@@ -17,7 +17,7 @@ import { osmNode } from '../osm/node';
 import { utilRebind } from '../util/rebind';
 import { utilKeybinding } from '../util';
 
-export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselineGraph) {
+export function behaviorDrawWay(context, wayID, index, mode, startGraph) {
 
     var dispatch = d3_dispatch('rejectedSelfIntersection');
 
@@ -32,8 +32,6 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
     behavior.hover().initialNodeID(index ? _origWay.nodes[index] :
         (_origWay.isClosed() ? _origWay.nodes[_origWay.nodes.length - 2] : _origWay.nodes[_origWay.nodes.length - 1]));
 
-    var _tempEdits = 0;
-
     // The osmNode to be placed.
     // This is temporary and just follows the mouse cursor until an "add" event occurs.
     var _drawNode;
@@ -43,20 +41,40 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
         _drawNode = osmNode({ loc: loc });
 
         context.pauseChangeDispatch();
-        // Add the drawing node to the graph.
-        // We must make sure to remove this edit later.
-        context.perform(_actionAddDrawNode(_drawNode));
-        _tempEdits++;
+        context.replace(function actionAddDrawNode(graph) {
+            // add the draw node to the graph and insert it into the way
+            var way = graph.entity(wayID);
+            return graph
+                .replace(_drawNode)
+                .replace(way.addNode(_drawNode.id, index));
+        }, _annotation);
         context.resumeChangeDispatch();
 
         setActiveElements();
     }
 
+    function removeDrawNode() {
+
+        context.pauseChangeDispatch();
+        context.replace(
+            function actionDeleteDrawNode(graph) {
+               var way = graph.entity(wayID);
+               return graph
+                   .replace(way.removeNode(_drawNode.id))
+                   .remove(_drawNode);
+           },
+            _annotation
+        );
+        _drawNode = undefined;
+        context.resumeChangeDispatch();
+    }
+
+    var _didResolveTempEdit = false;
+
     // Push an annotated state for undo to return back to.
-    // We must make sure to remove this edit later.
+    // We must make sure to replace or remove it later.
     context.pauseChangeDispatch();
     context.perform(actionNoop(), _annotation);
-    _tempEdits++;
     context.resumeChangeDispatch();
 
 
@@ -103,7 +121,8 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
 
         context.surface().classed('nope-disabled', d3_event.altKey);
 
-        var targetLoc = datum && datum.properties && datum.properties.entity && allowsVertex(datum.properties.entity) && datum.properties.entity.loc;
+        var targetLoc = datum && datum.properties && datum.properties.entity &&
+            allowsVertex(datum.properties.entity) && datum.properties.entity.loc;
         var targetNodes = datum && datum.properties && datum.properties.nodes;
 
         if (targetLoc) {   // snap to node/vertex - a point target with `.loc`
@@ -116,18 +135,18 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
             }
         }
 
-        context.replace(actionMoveNode(_drawNode.id, loc));
+        context.replace(actionMoveNode(_drawNode.id, loc), _annotation);
         _drawNode = context.entity(_drawNode.id);
-        checkGeometry(false);
+        checkGeometry(true /* includeDrawNode */);
     }
 
 
     // Check whether this edit causes the geometry to break.
     // If so, class the surface with a nope cursor.
-    // `finishDraw` - Only checks the relevant line segments if finishing drawing
-    function checkGeometry(finishDraw) {
+    // `includeDrawNode` - Only check the relevant line segments if finishing drawing
+    function checkGeometry(includeDrawNode) {
         var nopeDisabled = context.surface().classed('nope-disabled');
-        var isInvalid = _drawNode ? isInvalidGeometry(_drawNode, context.graph(), finishDraw) : false;
+        var isInvalid = isInvalidGeometry(includeDrawNode);
 
         if (nopeDisabled) {
             context.surface()
@@ -141,53 +160,67 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
     }
 
 
-    function isInvalidGeometry(entity, graph, finishDraw) {
-        var parents = graph.parentWays(entity);
+    function isInvalidGeometry(includeDrawNode) {
 
-        for (var i = 0; i < parents.length; i++) {
-            var parent = parents[i];
-            var nodes = graph.childNodes(parent).slice();  // shallow copy
+        var testNode = _drawNode;
 
-            if (_origWay.isClosed()) { // Check if Area
-                if (finishDraw) {
-                    if (nodes.length < 3) return false;
-                    nodes.splice(-2, 1);
-                    entity = nodes[nodes.length-2];
-                } else {
-                    nodes.pop();
-                }
-            } else { // Line
-                if (finishDraw) {
-                    nodes.pop();
-                }
+        // we only need to test the single way we're drawing
+        var parentWay = context.graph().entity(wayID);
+        var nodes = context.graph().childNodes(parentWay).slice();  // shallow copy
+
+        if (includeDrawNode) {
+            if (parentWay.isClosed()) {
+                // don't test the last segment for closed ways - #4655
+                // (still test the first segement)
+                nodes.pop();
             }
+        } else { // discount the draw node
 
-            if (geoHasSelfIntersections(nodes, entity.id)) {
-                return true;
+            if (parentWay.isClosed()) {
+                if (nodes.length < 3) return false;
+                if (_drawNode) nodes.splice(-2, 1);
+                testNode = nodes[nodes.length - 2];
+            } else {
+                // there's nothing we need to test if we ignore the draw node on open ways
+                return false;
             }
         }
 
-        return false;
+        return testNode && geoHasSelfIntersections(nodes, testNode.id);
     }
 
 
     function undone() {
-        context.pauseChangeDispatch();
-        // Undo popped the history back to the initial annotated no-op edit.
-        _tempEdits = 0;     // We will deal with the temp edits here
-        context.pop(1);     // Remove initial no-op edit
 
-        if (context.graph() === baselineGraph) {    // We've undone back to the beginning
-            // baselineGraph may be behind startGraph if this way was added rather than continued
-            resetToStartGraph();
-            context.resumeChangeDispatch();
-            context.enter(modeSelect(context, [wayID]));
+        // undoing removed the temp edit
+        _didResolveTempEdit = true;
+
+        context.pauseChangeDispatch();
+
+        var nextMode;
+
+        if (context.graph() === startGraph) { // we've undone back to the beginning
+            nextMode = modeSelect(context, [wayID]);
         } else {
-            // Remove whatever segment was drawn previously and continue drawing
-            context.pop(1);
-            context.resumeChangeDispatch();
-            context.enter(mode);
+            context.history()
+                .on('undone.draw', null);
+            // remove whatever segment was drawn previously
+            context.undo();
+
+            if (context.graph() === startGraph) { // we've undone back to the beginning
+                nextMode = modeSelect(context, [wayID]);
+            } else {
+                // continue drawing
+                nextMode = mode;
+            }
         }
+
+        // clear the redo stack by adding and removing an edit
+        context.perform(actionNoop());
+        context.pop(1);
+
+        context.resumeChangeDispatch();
+        context.enter(nextMode);
     }
 
 
@@ -234,12 +267,13 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
 
 
     drawWay.off = function(surface) {
-        // Drawing was interrupted unexpectedly.
-        // This can happen if the user changes modes,
-        // clicks geolocate button, a hashchange event occurs, etc.
-        if (_tempEdits) {
+
+        if (!_didResolveTempEdit) {
+            // Drawing was interrupted unexpectedly.
+            // This can happen if the user changes modes,
+            // clicks geolocate button, a hashchange event occurs, etc.
+
             context.pauseChangeDispatch();
-            context.pop(_tempEdits);
             resetToStartGraph();
             context.resumeChangeDispatch();
         }
@@ -265,114 +299,92 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
     };
 
 
-    function _actionAddDrawNode(drawNode) {
-        return function(graph) {
-            return graph
-                .replace(drawNode)
-                .replace(_origWay.addNode(drawNode.id, index));
-        };
+    function attemptAdd(d, loc, doAdd) {
+        var didJustAddDrawNode = false;
+        if (_drawNode) {
+            // move the node to the final loc in case move wasn't called
+            // consistently (e.g. on touch devices)
+            context.replace(actionMoveNode(_drawNode.id, loc), _annotation);
+            _drawNode = context.entity(_drawNode.id);
+        } else {
+            createDrawNode(loc);
+            didJustAddDrawNode = true;
+        }
+
+        checkGeometry(true /* includeDrawNode */);
+        if ((d && d.properties && d.properties.nope) || context.surface().classed('nope')) {
+            if (didJustAddDrawNode) {
+                // prevent the temporary draw node from appearing on touch devices
+                removeDrawNode();
+            }
+            dispatch.call('rejectedSelfIntersection', this);
+            return;   // can't click here
+        }
+
+        context.pauseChangeDispatch();
+        doAdd();
+        // we just replaced the temporary edit with the real one
+        _didResolveTempEdit = true;
+        context.resumeChangeDispatch();
+
+        context.enter(mode);
     }
 
 
-    function _actionReplaceDrawNode(drawNode, newNode) {
-        return function(graph) {
-            return graph
-                .replace(_origWay.addNode(newNode.id, index))
-                .remove(drawNode);
-        };
-    }
-
-
-    // Accept the current position of the drawing node and continue drawing.
+    // Accept the current position of the drawing node
     drawWay.add = function(loc, d) {
-        if ((d && d.properties && d.properties.nope) || context.surface().classed('nope')) {
-            dispatch.call('rejectedSelfIntersection', this);
-            return;   // can't click here
-        }
-
-        if (!_drawNode) createDrawNode(loc);
-
-        // always move the node to the final loc in case move wasn't consistently called (e.g. on touch devices)
-        context.replace(actionMoveNode(_drawNode.id, loc));
-        _drawNode = context.entity(_drawNode.id);
-
-        context.pauseChangeDispatch();
-        context.pop(_tempEdits);
-        _tempEdits = 0;
-
-        context.perform(
-            _actionAddDrawNode(_drawNode),
-            _annotation
-        );
-
-        context.resumeChangeDispatch();
-        checkGeometry(false);   // finishDraw = false
-        context.enter(mode);
+        attemptAdd(d, loc, function() {
+            // don't need to do anything extra
+        });
     };
 
 
-    // Connect the way to an existing way.
+    // Connect the way to an existing way
     drawWay.addWay = function(loc, edge, d) {
-        if ((d && d.properties && d.properties.nope) || context.surface().classed('nope')) {
-            dispatch.call('rejectedSelfIntersection', this);
-            return;   // can't click here
-        }
-
-        if (!_drawNode) createDrawNode();
-
-        context.pauseChangeDispatch();
-        context.pop(_tempEdits);
-        _tempEdits = 0;
-
-        context.perform(
-            _actionAddDrawNode(_drawNode),
-            actionAddMidpoint({ loc: loc, edge: edge }, _drawNode),
-            _annotation
-        );
-
-        context.resumeChangeDispatch();
-        checkGeometry(false);   // finishDraw = false
-        context.enter(mode);
+        attemptAdd(d, loc, function() {
+            context.replace(
+                actionAddMidpoint({ loc: loc, edge: edge }, _drawNode),
+                _annotation
+            );
+        });
     };
 
 
-    // Connect the way to an existing node and continue drawing.
+    // Connect the way to an existing node
     drawWay.addNode = function(node, d) {
-        if ((d && d.properties && d.properties.nope) || context.surface().classed('nope')) {
-            dispatch.call('rejectedSelfIntersection', this);
-            return;   // can't click here
-        }
+        attemptAdd(d, node.loc, function() {
+            context.replace(
+                function actionReplaceDrawNode(graph) {
+                    // remove the temporary draw node and insert the existing node
+                    // at the same index
 
-        if (!_drawNode) createDrawNode();
-
-        context.pauseChangeDispatch();
-        context.pop(_tempEdits);
-        _tempEdits = 0;
-
-        context.perform(
-            _actionReplaceDrawNode(_drawNode, node),
-            _annotation
-        );
-
-        context.resumeChangeDispatch();
-        checkGeometry(false);   // finishDraw = false
-        context.enter(mode);
+                    graph = graph
+                        .replace(graph.entity(wayID).removeNode(_drawNode.id))
+                        .remove(_drawNode);
+                    return graph
+                        .replace(graph.entity(wayID).addNode(node.id, index));
+                },
+                _annotation
+            );
+        });
     };
 
 
-    // Finish the draw operation, removing the temporary edits.
+    // Finish the draw operation, removing the temporary edit.
     // If the way has enough nodes to be valid, it's selected.
     // Otherwise, delete everything and return to browse mode.
     drawWay.finish = function() {
-        checkGeometry(true);   // finishDraw = true
+        checkGeometry(false /* includeDrawNode */);
         if (context.surface().classed('nope')) {
             dispatch.call('rejectedSelfIntersection', this);
             return;   // can't click here
         }
 
         context.pauseChangeDispatch();
-        context.pop(_tempEdits);
-        _tempEdits = 0;
+        // remove the temporary edit
+        context.pop(1);
+        _didResolveTempEdit = true;
+        context.resumeChangeDispatch();
 
         var way = context.hasEntity(wayID);
         if (!way || way.isDegenerate()) {
@@ -380,8 +392,6 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
             return;
         }
 
-        context.resumeChangeDispatch();
-
         window.setTimeout(function() {
             context.map().dblclickZoomEnable(true);
         }, 1000);
@@ -394,9 +404,6 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin
     // Cancel the draw operation, delete everything, and return to browse mode.
     drawWay.cancel = function() {
         context.pauseChangeDispatch();
-        context.pop(_tempEdits);
-        _tempEdits = 0;
-
         resetToStartGraph();
         context.resumeChangeDispatch();
 

modules/modes/add_area.js

@@ -40,7 +40,7 @@ export function modeAddArea(context, mode) {
             actionClose(way.id)
         );
 
-        context.enter(modeDrawArea(context, way.id, startGraph, context.graph(), mode.button));
+        context.enter(modeDrawArea(context, way.id, startGraph, mode.button));
     }
 
 
@@ -57,7 +57,7 @@ export function modeAddArea(context, mode) {
             actionAddMidpoint({ loc: loc, edge: edge }, node)
         );
 
-        context.enter(modeDrawArea(context, way.id, startGraph, context.graph(), mode.button));
+        context.enter(modeDrawArea(context, way.id, startGraph, mode.button));
     }
 
 
@@ -71,7 +71,7 @@ export function modeAddArea(context, mode) {
             actionClose(way.id)
         );
 
-        context.enter(modeDrawArea(context, way.id, startGraph, context.graph(), mode.button));
+        context.enter(modeDrawArea(context, way.id, startGraph, mode.button));
     }
 
 

modules/modes/add_line.js

@@ -32,7 +32,7 @@ export function modeAddLine(context, mode) {
             actionAddVertex(way.id, node.id)
         );
 
-        context.enter(modeDrawLine(context, way.id, startGraph, context.graph(), mode.button));
+        context.enter(modeDrawLine(context, way.id, startGraph, mode.button));
     }
 
 
@@ -48,7 +48,7 @@ export function modeAddLine(context, mode) {
             actionAddMidpoint({ loc: loc, edge: edge }, node)
         );
 
-        context.enter(modeDrawLine(context, way.id, startGraph, context.graph(), mode.button));
+        context.enter(modeDrawLine(context, way.id, startGraph, mode.button));
     }
 
 
@@ -61,7 +61,7 @@ export function modeAddLine(context, mode) {
             actionAddVertex(way.id, node.id)
         );
 
-        context.enter(modeDrawLine(context, way.id, startGraph, context.graph(), mode.button));
+        context.enter(modeDrawLine(context, way.id, startGraph, mode.button));
     }
 
 

modules/modes/draw_area.js

@@ -3,7 +3,7 @@ import { behaviorDrawWay } from '../behavior/draw_way';
 import { uiFlash } from '../ui/flash';
 
 
-export function modeDrawArea(context, wayID, startGraph, baselineGraph, button) {
+export function modeDrawArea(context, wayID, startGraph, button) {
     var mode = {
         button: button,
         id: 'draw-area'
@@ -16,7 +16,7 @@ export function modeDrawArea(context, wayID, startGraph, baselineGraph, button)
     mode.enter = function() {
         var way = context.entity(wayID);
 
-        behavior = behaviorDrawWay(context, wayID, undefined, mode, startGraph, baselineGraph)
+        behavior = behaviorDrawWay(context, wayID, undefined, mode, startGraph)
             .tail(t('modes.draw_area.tail'))
             .on('rejectedSelfIntersection.modeDrawArea', function() {
                 uiFlash()

modules/modes/draw_line.js

@@ -3,7 +3,7 @@ import { behaviorDrawWay } from '../behavior/draw_way';
 import { uiFlash } from '../ui/flash';
 
 
-export function modeDrawLine(context, wayID, startGraph, baselineGraph, button, affix, continuing) {
+export function modeDrawLine(context, wayID, startGraph, button, affix, continuing) {
     var mode = {
         button: button,
         id: 'draw-line'
@@ -20,7 +20,7 @@ export function modeDrawLine(context, wayID, startGraph, baselineGraph, button,
         var index = (affix === 'prefix') ? 0 : undefined;
         var headID = (affix === 'prefix') ? way.first() : way.last();
 
-        behavior = behaviorDrawWay(context, wayID, index, mode, startGraph, baselineGraph)
+        behavior = behaviorDrawWay(context, wayID, index, mode, startGraph)
             .tail(t('modes.draw_line.tail'))
             .on('rejectedSelfIntersection.modeDrawLine', function() {
                 uiFlash()