From b5856e24155b81d3350a543dab9d8f1f5adb83b2 Mon Sep 17 00:00:00 2001 From: Quincy Morgan Date: Wed, 11 Mar 2020 17:48:09 -0700 Subject: [PATCH] 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 | 289 ++++++++++++++++++----------------- modules/modes/add_area.js | 6 +- modules/modes/add_line.js | 6 +- modules/modes/draw_area.js | 4 +- modules/modes/draw_line.js | 4 +- 5 files changed, 158 insertions(+), 151 deletions(-) diff --git a/modules/behavior/draw_way.js b/modules/behavior/draw_way.js index 0c465042d..421eb0ef8 100644 --- a/modules/behavior/draw_way.js +++ b/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(); diff --git a/modules/modes/add_area.js b/modules/modes/add_area.js index ca6da2ee1..b20e78e2c 100644 --- a/modules/modes/add_area.js +++ b/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)); } diff --git a/modules/modes/add_line.js b/modules/modes/add_line.js index 2f44ad8cd..d0dc748d2 100644 --- a/modules/modes/add_line.js +++ b/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)); } diff --git a/modules/modes/draw_area.js b/modules/modes/draw_area.js index d9602ff6a..15bf9cb9f 100644 --- a/modules/modes/draw_area.js +++ b/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() diff --git a/modules/modes/draw_line.js b/modules/modes/draw_line.js index 356fe6a96..40d5840ff 100644 --- a/modules/modes/draw_line.js +++ b/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()