Undo/Redo while drawing line/area should keep the user in drawing mode

(closes #3530)

modules/behavior/draw_way.js

@@ -4,71 +4,54 @@ import { t } from '../util/locale';
 import {
     actionAddEntity,
     actionAddMidpoint,
-    actionAddVertex,
-    actionMoveNode
-} from '../actions/index';
+    actionMoveNode,
+    actionNoop
+} from '../actions';
 
-import {
-    modeBrowse,
-    modeSelect
-} from '../modes/index';
-
-import {
-    osmNode,
-    osmWay
-} from '../osm/index';
-
-import {
-    geoChooseEdge,
-    geoEdgeEqual
-} from '../geo/index';
-
-import {
-    behaviorDraw
-} from './draw';
-
-import {
-    utilEntitySelector,
-    utilFunctor
-} from '../util/index';
+import { behaviorDraw } from './draw';
+import { geoChooseEdge, geoEdgeEqual } from '../geo';
+import { modeBrowse, modeSelect } from '../modes';
+import { osmNode, osmWay } from '../osm';
+import { utilEntitySelector } from '../util';
 
 
-export function behaviorDrawWay(context, wayId, index, mode, baseGraph) {
+export function behaviorDrawWay(context, wayId, index, mode, startGraph) {
 
-    var way = context.entity(wayId),
+    var origWay = context.entity(wayId),
         isArea = context.geometry(wayId) === 'area',
-        finished = false,
-        annotation = t((way.isDegenerate() ?
+        tempEdits = 0,
+        annotation = t((origWay.isDegenerate() ?
             'operations.start.annotation.' :
             'operations.continue.annotation.') + context.geometry(wayId)),
         draw = behaviorDraw(context),
-        startIndex, start, end, segment;
+        startIndex,
+        start,
+        end,
+        segment;
 
 
+    // initialize the temporary drawing entities
     if (!isArea) {
-        startIndex = typeof index === 'undefined' ? way.nodes.length - 1 : 0;
-        start = osmNode({ id: 'nStart', loc: context.entity(way.nodes[startIndex]).loc });
+        startIndex = typeof index === 'undefined' ? origWay.nodes.length - 1 : 0;
+        start = osmNode({ id: 'nStart', loc: context.entity(origWay.nodes[startIndex]).loc });
         end = osmNode({ id: 'nEnd', loc: context.map().mouseCoordinates() });
         segment = osmWay({ id: 'wTemp',
             nodes: typeof index === 'undefined' ? [start.id, end.id] : [end.id, start.id],
-            tags: _.clone(way.tags)
+            tags: _.clone(origWay.tags)
         });
     } else {
         end = osmNode({ loc: context.map().mouseCoordinates() });
     }
 
+    // Push an annotated state for undo to return back to.
+    // We must make sure to remove this edit later.
+    context.perform(actionNoop(), annotation);
+    tempEdits++;
 
-    var fn = context[way.isDegenerate() ? 'replace' : 'perform'];
-    if (isArea) {
-        fn(actionAddEntity(end),
-            actionAddVertex(wayId, end.id)
-        );
-    } else {
-        fn(actionAddEntity(start),
-            actionAddEntity(end),
-            actionAddEntity(segment)
-        );
-    }
+    // Add the temporary drawing entities to the graph.
+    // We must make sure to remove this edit later.
+    context.perform(AddDrawEntities());
+    tempEdits++;
 
 
     function move(datum) {
@@ -77,7 +60,7 @@ export function behaviorDrawWay(context, wayId, index, mode, baseGraph) {
         if (datum.type === 'node' && datum.id !== end.id) {
             loc = datum.loc;
 
-        } else if (datum.type === 'way') { // && (segment || datum.id !== segment.id)) {
+        } else if (datum.type === 'way') {
             var dims = context.map().dimensions(),
                 mouse = context.mouse(),
                 pad = 5,
@@ -94,12 +77,21 @@ export function behaviorDrawWay(context, wayId, index, mode, baseGraph) {
         }
 
         context.replace(actionMoveNode(end.id, loc));
+        end = context.entity(end.id);
     }
 
 
     function undone() {
-        finished = true;
-        context.enter(modeBrowse(context));
+        // Undo popped the history back to the initial annotated no-op edit.
+        // Remove initial no-op edit and whatever edit happened immediately before it.
+        context.pop(2);
+        tempEdits = 0;
+
+        if (context.hasEntity(wayId)) {
+            context.enter(mode);
+        } else {
+            context.enter(modeBrowse(context));
+        }
     }
 
 
@@ -133,8 +125,8 @@ export function behaviorDrawWay(context, wayId, index, mode, baseGraph) {
 
 
     drawWay.off = function(surface) {
-        if (!finished)
-            context.pop();
+        context.pop(tempEdits);
+        tempEdits = 0;
 
         context.map()
             .on('drawn.draw', null);
@@ -148,15 +140,40 @@ export function behaviorDrawWay(context, wayId, index, mode, baseGraph) {
     };
 
 
-    function ReplaceTemporaryNode(newNode) {
+    function AddDrawEntities() {
         return function(graph) {
             if (isArea) {
+                // For area drawing, there is no need for a temporary node.
+                // `end` gets inserted into the way as the penultimate node.
                 return graph
-                    .replace(way.addNode(newNode.id))
+                    .replace(end)
+                    .replace(origWay.addNode(end.id));
+            } else {
+                // For line drawing, add a temporary start, end, and segment to the graph.
+                // This allows us to class the new segment as `active`, but still
+                // connect it back to parts of the way that have already been drawn.
+                return graph
+                    .replace(start)
+                    .replace(end)
+                    .replace(segment);
+            }
+        };
+    }
+
+
+    function ReplaceDrawEntities(newNode) {
+        return function(graph) {
+            if (isArea) {
+                // For area drawing, we didn't create a temporary node.
+                // `newNode` gets inserted into the _original_ way as the penultimate node.
+                return graph
+                    .replace(origWay.addNode(newNode.id))
                     .remove(end);
             } else {
+                // For line drawing, add the `newNode` to the way at specified index,
+                // and remove the temporary start, end, and segment.
                 return graph
-                    .replace(graph.entity(wayId).addNode(newNode.id, index))
+                    .replace(origWay.addNode(newNode.id, index))
                     .remove(end)
                     .remove(segment)
                     .remove(start);
@@ -168,54 +185,60 @@ export function behaviorDrawWay(context, wayId, index, mode, baseGraph) {
     // Accept the current position of the temporary node and continue drawing.
     drawWay.add = function(loc) {
         // prevent duplicate nodes
-        var last = context.hasEntity(way.nodes[way.nodes.length - (isArea ? 2 : 1)]);
+        var last = context.hasEntity(origWay.nodes[origWay.nodes.length - (isArea ? 2 : 1)]);
         if (last && last.loc[0] === loc[0] && last.loc[1] === loc[1]) return;
 
+        context.pop(tempEdits);
+
         if (isArea) {
-            context.replace(
-                actionMoveNode(end.id, loc),
+            context.perform(
+                AddDrawEntities(),
                 annotation
             );
         } else {
             var newNode = osmNode({loc: loc});
-            context.replace(
+            context.perform(
                 actionAddEntity(newNode),
-                ReplaceTemporaryNode(newNode),
+                ReplaceDrawEntities(newNode),
                 annotation
             );
         }
 
-        finished = true;
+        tempEdits = 0;
         context.enter(mode);
     };
 
 
     // Connect the way to an existing way.
     drawWay.addWay = function(loc, edge) {
-
         if (isArea) {
+            context.pop(tempEdits);
+
             context.perform(
+                AddDrawEntities(),
                 actionAddMidpoint({ loc: loc, edge: edge}, end),
                 annotation
             );
         } else {
             var previousEdge = startIndex ?
-                [way.nodes[startIndex], way.nodes[startIndex - 1]] :
-                [way.nodes[0], way.nodes[1]];
+                [origWay.nodes[startIndex], origWay.nodes[startIndex - 1]] :
+                [origWay.nodes[0], origWay.nodes[1]];
 
             // Avoid creating duplicate segments
             if (geoEdgeEqual(edge, previousEdge))
                 return;
 
+            context.pop(tempEdits);
+
             var newNode = osmNode({ loc: loc });
             context.perform(
                 actionAddMidpoint({ loc: loc, edge: edge}, newNode),
-                ReplaceTemporaryNode(newNode),
+                ReplaceDrawEntities(newNode),
                 annotation
             );
         }
 
-        finished = true;
+        tempEdits = 0;
         context.enter(mode);
     };
 
@@ -223,47 +246,54 @@ export function behaviorDrawWay(context, wayId, index, mode, baseGraph) {
     // Connect the way to an existing node and continue drawing.
     drawWay.addNode = function(node) {
         // Avoid creating duplicate segments
-        if (way.areAdjacent(node.id, way.nodes[way.nodes.length - 1])) return;
+        if (origWay.areAdjacent(node.id, origWay.nodes[origWay.nodes.length - 1])) return;
+
+        context.pop(tempEdits);
 
         context.perform(
-            ReplaceTemporaryNode(node),
+            ReplaceDrawEntities(node),
             annotation
         );
 
-        finished = true;
+        tempEdits = 0;
         context.enter(mode);
     };
 
 
-    // Finish the draw operation, removing the temporary node. If the way has enough
-    // nodes to be valid, it's selected. Otherwise, return to browse mode.
+    // Finish the draw operation, removing the temporary edits.
+    // If the way has enough nodes to be valid, it's selected.
+    // Otherwise, delete everything and return to browse mode.
     drawWay.finish = function() {
-        context.pop();
-        finished = true;
+        context.pop(tempEdits);
+        tempEdits = 0;
+
+        var way = context.hasEntity(wayId);
+        if (!way || origWay.isDegenerate()) {
+            drawWay.cancel();
+            return;
+        }
 
         window.setTimeout(function() {
             context.map().dblclickEnable(true);
         }, 1000);
 
-        if (context.hasEntity(wayId)) {
-            context.enter(modeSelect(context, [wayId]).newFeature(true));
-        } else {
-            context.enter(modeBrowse(context));
-        }
+        context.enter(modeSelect(context, [wayId]).newFeature(true));
     };
 
 
-    // Cancel the draw operation and return to browse, deleting everything drawn.
+    // Cancel the draw operation, delete everything, and return to browse mode.
     drawWay.cancel = function() {
-        context.perform(
-            utilFunctor(baseGraph),
-            t('operations.cancel_draw.annotation'));
+        context.pop(tempEdits);
+        tempEdits = 0;
+
+        while (context.graph() !== startGraph) {
+            context.pop();
+        }
 
         window.setTimeout(function() {
             context.map().dblclickEnable(true);
         }, 1000);
 
-        finished = true;
         context.enter(modeBrowse(context));
     };
 

modules/modes/add_area.js

@@ -35,7 +35,7 @@ export function modeAddArea(context) {
 
 
     function start(loc) {
-        var graph = context.graph(),
+        var startGraph = context.graph(),
             node = osmNode({ loc: loc }),
             way = osmWay({ tags: defaultTags });
 
@@ -46,12 +46,12 @@ export function modeAddArea(context) {
             actionClose(way.id)
         );
 
-        context.enter(modeDrawArea(context, way.id, graph));
+        context.enter(modeDrawArea(context, way.id, startGraph));
     }
 
 
     function startFromWay(loc, edge) {
-        var graph = context.graph(),
+        var startGraph = context.graph(),
             node = osmNode({ loc: loc }),
             way = osmWay({ tags: defaultTags });
 
@@ -63,12 +63,12 @@ export function modeAddArea(context) {
             actionAddMidpoint({ loc: loc, edge: edge }, node)
         );
 
-        context.enter(modeDrawArea(context, way.id, graph));
+        context.enter(modeDrawArea(context, way.id, startGraph));
     }
 
 
     function startFromNode(node) {
-        var graph = context.graph(),
+        var startGraph = context.graph(),
             way = osmWay({ tags: defaultTags });
 
         context.perform(
@@ -77,7 +77,7 @@ export function modeAddArea(context) {
             actionClose(way.id)
         );
 
-        context.enter(modeDrawArea(context, way.id, graph));
+        context.enter(modeDrawArea(context, way.id, startGraph));
     }
 
 

modules/modes/add_line.js

@@ -27,7 +27,7 @@ export function modeAddLine(context) {
 
 
     function start(loc) {
-        var baseGraph = context.graph(),
+        var startGraph = context.graph(),
             node = osmNode({ loc: loc }),
             way = osmWay();
 
@@ -37,12 +37,12 @@ export function modeAddLine(context) {
             actionAddVertex(way.id, node.id)
         );
 
-        context.enter(modeDrawLine(context, way.id, baseGraph));
+        context.enter(modeDrawLine(context, way.id, startGraph));
     }
 
 
     function startFromWay(loc, edge) {
-        var baseGraph = context.graph(),
+        var startGraph = context.graph(),
             node = osmNode({ loc: loc }),
             way = osmWay();
 
@@ -53,12 +53,12 @@ export function modeAddLine(context) {
             actionAddMidpoint({ loc: loc, edge: edge }, node)
         );
 
-        context.enter(modeDrawLine(context, way.id, baseGraph));
+        context.enter(modeDrawLine(context, way.id, startGraph));
     }
 
 
     function startFromNode(node) {
-        var baseGraph = context.graph(),
+        var startGraph = context.graph(),
             way = osmWay();
 
         context.perform(
@@ -66,7 +66,7 @@ export function modeAddLine(context) {
             actionAddVertex(way.id, node.id)
         );
 
-        context.enter(modeDrawLine(context, way.id, baseGraph));
+        context.enter(modeDrawLine(context, way.id, startGraph));
     }
 
 

modules/modes/draw_area.js

@@ -1,7 +1,7 @@
 import { t } from '../util/locale';
 import { behaviorDrawWay } from '../behavior/index';
 
-export function modeDrawArea(context, wayId, baseGraph) {
+export function modeDrawArea(context, wayId, startGraph) {
     var mode = {
         button: 'area',
         id: 'draw-area'
@@ -13,7 +13,7 @@ export function modeDrawArea(context, wayId, baseGraph) {
     mode.enter = function() {
         var way = context.entity(wayId);
 
-        behavior = behaviorDrawWay(context, wayId, undefined, mode, baseGraph)
+        behavior = behaviorDrawWay(context, wayId, undefined, mode, startGraph)
             .tail(t('modes.draw_area.tail'));
 
         var addNode = behavior.addNode;

modules/modes/draw_line.js

@@ -1,7 +1,7 @@
 import { t } from '../util/locale';
 import { behaviorDrawWay } from '../behavior/index';
 
-export function modeDrawLine(context, wayId, baseGraph, affix) {
+export function modeDrawLine(context, wayId, startGraph, affix) {
     var mode = {
         button: 'line',
         id: 'draw-line'
@@ -15,7 +15,7 @@ export function modeDrawLine(context, wayId, baseGraph, affix) {
             index = (affix === 'prefix') ? 0 : undefined,
             headId = (affix === 'prefix') ? way.first() : way.last();
 
-        behavior = behaviorDrawWay(context, wayId, index, mode, baseGraph)
+        behavior = behaviorDrawWay(context, wayId, index, mode, startGraph)
             .tail(t('modes.draw_line.tail'));
 
         var addNode = behavior.addNode;