From 0ea69548ea6389d8c3020939aed4fd2dd6bddfdf Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 5 Mar 2019 17:46:54 -0500 Subject: [PATCH 1/3] Add history.pauseChangeDispatch / history.resumeChangeDispatch To avoid dispatching change events at improper times --- modules/core/history.js | 25 ++++++++++++++++---- test/spec/core/history.js | 49 +++++++++++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 17 deletions(-) diff --git a/modules/core/history.js b/modules/core/history.js index 97f730579..fb7f9fa8c 100644 --- a/modules/core/history.js +++ b/modules/core/history.js @@ -36,6 +36,7 @@ export function coreHistory(context) { var duration = 150; var _imageryUsed = []; var _checkpoints = {}; + var _pausedGraph; var _stack; var _index; var _tree; @@ -104,11 +105,13 @@ export function coreHistory(context) { // determine difference and dispatch a change event function change(previous, isAnnotated) { var difference = coreDifference(previous, history.graph()); - dispatch.call('change', this, difference); - if (isAnnotated) { - // actions like dragging a node can fire lots of changes, - // so use 'annotatedChange' to listen for grouped undo/redo changes - dispatch.call('annotatedChange', this, difference); + if (!_pausedGraph) { + dispatch.call('change', this, difference); + if (isAnnotated) { + // actions like dragging a node can fire lots of changes, + // so use 'annotatedChange' to listen for grouped undo/redo changes + dispatch.call('annotatedChange', this, difference); + } } return difference; } @@ -244,6 +247,18 @@ export function coreHistory(context) { }, + pauseChangeDispatch: function() { + _pausedGraph = _stack[_index].graph; + }, + + + resumeChangeDispatch: function() { + var previous = _pausedGraph; + _pausedGraph = null; + return change(previous, true); + }, + + undoAnnotation: function() { var i = _index; while (i >= 0) { diff --git a/test/spec/core/history.js b/test/spec/core/history.js index 3c1533d3b..c9872a66f 100644 --- a/test/spec/core/history.js +++ b/test/spec/core/history.js @@ -55,8 +55,8 @@ describe('iD.History', function () { }); it('performs multiple actions', function () { - var action1 = sinon.stub().returns(iD.coreGraph()), - action2 = sinon.stub().returns(iD.coreGraph()); + var action1 = sinon.stub().returns(iD.coreGraph()); + var action2 = sinon.stub().returns(iD.coreGraph()); history.perform(action1, action2, 'annotation'); expect(action1).to.have.been.called; expect(action2).to.have.been.called; @@ -99,8 +99,8 @@ describe('iD.History', function () { }); it('performs multiple actions', function () { - var action1 = sinon.stub().returns(iD.coreGraph()), - action2 = sinon.stub().returns(iD.coreGraph()); + var action1 = sinon.stub().returns(iD.coreGraph()); + var action2 = sinon.stub().returns(iD.coreGraph()); history.replace(action1, action2, 'annotation'); expect(action1).to.have.been.called; expect(action2).to.have.been.called; @@ -193,8 +193,8 @@ describe('iD.History', function () { }); it('performs multiple actions', function () { - var action1 = sinon.stub().returns(iD.coreGraph()), - action2 = sinon.stub().returns(iD.coreGraph()); + var action1 = sinon.stub().returns(iD.coreGraph()); + var action2 = sinon.stub().returns(iD.coreGraph()); history.perform(action, 'annotation'); history.overwrite(action1, action2, 'annotation2'); expect(action1).to.have.been.called; @@ -266,6 +266,31 @@ describe('iD.History', function () { }); }); + describe('#pauseChangeDispatch / #resumeChangeDispatch', function() { + it('prevents change events from getting dispatched', function() { + history.perform(action, 'base'); + history.on('change', spy); + + history.pauseChangeDispatch(); + + history.perform(action, 'perform'); + expect(spy).to.have.been.notCalled; + history.replace(action, 'replace'); + expect(spy).to.have.been.notCalled; + history.overwrite(action, 'replace'); + expect(spy).to.have.been.notCalled; + history.undo(); + expect(spy).to.have.been.notCalled; + history.redo(); + expect(spy).to.have.been.notCalled; + history.pop(); + expect(spy).to.have.been.notCalled; + + var difference = history.resumeChangeDispatch(); + expect(spy).to.have.been.calledOnceWith(difference); + }); + }); + describe('#changes', function () { it('includes created entities', function () { var node = iD.osmNode(); @@ -274,8 +299,8 @@ describe('iD.History', function () { }); it('includes modified entities', function () { - var node1 = iD.osmNode({id: 'n1'}), - node2 = node1.update({ tags: { yes: 'no' } }); + var node1 = iD.osmNode({id: 'n1'}); + var node2 = node1.update({ tags: { yes: 'no' } }); history.merge([node1]); history.perform(function (graph) { return graph.replace(node2); }); expect(history.changes().modified).to.eql([node2]); @@ -357,10 +382,10 @@ describe('iD.History', function () { }); it('generates v3 JSON', function() { - var node_1 = iD.osmNode({id: 'n-1'}), - node1 = iD.osmNode({id: 'n1'}), - node2 = iD.osmNode({id: 'n2'}), - node3 = iD.osmNode({id: 'n3'}); + var node_1 = iD.osmNode({id: 'n-1'}); + var node1 = iD.osmNode({id: 'n1'}); + var node2 = iD.osmNode({id: 'n2'}); + var node3 = iD.osmNode({id: 'n3'}); history.merge([node1, node2, node3]); history.perform(iD.actionAddEntity(node_1)); // addition history.perform(iD.actionChangeTags('n2', {k: 'v'})); // modification From 6da2ba7b9ddc634a1af5621ffefa886dd6d3b6e8 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Wed, 6 Mar 2019 12:00:16 -0500 Subject: [PATCH 2/3] Make sure pause/resumeChangeDispatch can be called multiple times --- modules/core/context.js | 2 + modules/core/history.js | 12 ++-- test/spec/core/history.js | 143 ++++++++++++++++++++++---------------- 3 files changed, 95 insertions(+), 62 deletions(-) diff --git a/modules/core/context.js b/modules/core/context.js index 2efa2c4f1..9e730711a 100644 --- a/modules/core/context.js +++ b/modules/core/context.js @@ -488,6 +488,8 @@ export function coreContext() { context.graph = history.graph; context.changes = history.changes; context.intersects = history.intersects; + context.pauseChangeDispatch = history.pauseChangeDispatch; + context.resumeChangeDispatch = history.resumeChangeDispatch; validator = coreValidator(context); diff --git a/modules/core/history.js b/modules/core/history.js index fb7f9fa8c..ea4c7cdac 100644 --- a/modules/core/history.js +++ b/modules/core/history.js @@ -248,14 +248,18 @@ export function coreHistory(context) { pauseChangeDispatch: function() { - _pausedGraph = _stack[_index].graph; + if (!_pausedGraph) { + _pausedGraph = _stack[_index].graph; + } }, resumeChangeDispatch: function() { - var previous = _pausedGraph; - _pausedGraph = null; - return change(previous, true); + if (_pausedGraph) { + var previous = _pausedGraph; + _pausedGraph = null; + return change(previous, true); + } }, diff --git a/test/spec/core/history.js b/test/spec/core/history.js index c9872a66f..4fbd67ec3 100644 --- a/test/spec/core/history.js +++ b/test/spec/core/history.js @@ -1,6 +1,11 @@ -describe('iD.History', function () { - var context, history, spy, - action = function() { return iD.coreGraph(); }; +describe('iD.coreHistory', function () { + var context, history, spy; + var actionNoop = function(g) { return g; }; + var actionAddNode = function (nodeID) { + return function(g) { + return g.replace(iD.osmNode({ id: nodeID })); + }; + }; beforeEach(function () { context = iD.coreContext(); @@ -33,7 +38,7 @@ describe('iD.History', function () { describe('#perform', function () { it('returns a difference', function () { - expect(history.perform(action).changes()).to.eql({}); + expect(history.perform(actionNoop).changes()).to.eql({}); }); it('updates the graph', function () { @@ -43,13 +48,13 @@ describe('iD.History', function () { }); it('pushes an undo annotation', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); expect(history.undoAnnotation()).to.equal('annotation'); }); it('emits a change event', function () { history.on('change', spy); - var difference = history.perform(action); + var difference = history.perform(actionNoop); expect(spy).to.have.been.calledWith(difference); expect(spy.callCount).to.eql(1); }); @@ -77,7 +82,7 @@ describe('iD.History', function () { describe('#replace', function () { it('returns a difference', function () { - expect(history.replace(action).changes()).to.eql({}); + expect(history.replace(actionNoop).changes()).to.eql({}); }); it('updates the graph', function () { @@ -87,14 +92,14 @@ describe('iD.History', function () { }); it('replaces the undo annotation', function () { - history.perform(action, 'annotation1'); - history.replace(action, 'annotation2'); + history.perform(actionNoop, 'annotation1'); + history.replace(actionNoop, 'annotation2'); expect(history.undoAnnotation()).to.equal('annotation2'); }); it('emits a change event', function () { history.on('change', spy); - var difference = history.replace(action); + var difference = history.replace(actionNoop); expect(spy).to.have.been.calledWith(difference); }); @@ -110,49 +115,49 @@ describe('iD.History', function () { describe('#pop', function () { it('returns a difference', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); expect(history.pop().changes()).to.eql({}); }); it('updates the graph', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); history.pop(); expect(history.undoAnnotation()).to.be.undefined; }); it('does not push the redo stack', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); history.pop(); expect(history.redoAnnotation()).to.be.undefined; }); it('emits a change event', function () { - history.perform(action); + history.perform(actionNoop); history.on('change', spy); var difference = history.pop(); expect(spy).to.have.been.calledWith(difference); }); it('pops n times', function () { - history.perform(action, 'annotation1'); - history.perform(action, 'annotation2'); - history.perform(action, 'annotation3'); + history.perform(actionNoop, 'annotation1'); + history.perform(actionNoop, 'annotation2'); + history.perform(actionNoop, 'annotation3'); history.pop(2); expect(history.undoAnnotation()).to.equal('annotation1'); }); it('pops 0 times', function () { - history.perform(action, 'annotation1'); - history.perform(action, 'annotation2'); - history.perform(action, 'annotation3'); + history.perform(actionNoop, 'annotation1'); + history.perform(actionNoop, 'annotation2'); + history.perform(actionNoop, 'annotation3'); history.pop(0); expect(history.undoAnnotation()).to.equal('annotation3'); }); it('pops 1 time if argument is invalid', function () { - history.perform(action, 'annotation1'); - history.perform(action, 'annotation2'); - history.perform(action, 'annotation3'); + history.perform(actionNoop, 'annotation1'); + history.perform(actionNoop, 'annotation2'); + history.perform(actionNoop, 'annotation3'); history.pop('foo'); expect(history.undoAnnotation()).to.equal('annotation2'); history.pop(-1); @@ -162,40 +167,40 @@ describe('iD.History', function () { describe('#overwrite', function () { it('returns a difference', function () { - history.perform(action, 'annotation'); - expect(history.overwrite(action).changes()).to.eql({}); + history.perform(actionNoop, 'annotation'); + expect(history.overwrite(actionNoop).changes()).to.eql({}); }); it('updates the graph', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); var node = iD.osmNode(); history.overwrite(function (graph) { return graph.replace(node); }); expect(history.graph().entity(node.id)).to.equal(node); }); it('replaces the undo annotation', function () { - history.perform(action, 'annotation1'); - history.overwrite(action, 'annotation2'); + history.perform(actionNoop, 'annotation1'); + history.overwrite(actionNoop, 'annotation2'); expect(history.undoAnnotation()).to.equal('annotation2'); }); it('does not push the redo stack', function () { - history.perform(action, 'annotation'); - history.overwrite(action, 'annotation2'); + history.perform(actionNoop, 'annotation'); + history.overwrite(actionNoop, 'annotation2'); expect(history.redoAnnotation()).to.be.undefined; }); it('emits a change event', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); history.on('change', spy); - var difference = history.overwrite(action, 'annotation2'); + var difference = history.overwrite(actionNoop, 'annotation2'); expect(spy).to.have.been.calledWith(difference); }); it('performs multiple actions', function () { var action1 = sinon.stub().returns(iD.coreGraph()); var action2 = sinon.stub().returns(iD.coreGraph()); - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); history.overwrite(action1, action2, 'annotation2'); expect(action1).to.have.been.called; expect(action2).to.have.been.called; @@ -209,26 +214,26 @@ describe('iD.History', function () { }); it('pops the undo stack', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); history.undo(); expect(history.undoAnnotation()).to.be.undefined; }); it('pushes the redo stack', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); history.undo(); expect(history.redoAnnotation()).to.equal('annotation'); }); it('emits an undone event', function () { - history.perform(action); + history.perform(actionNoop); history.on('undone', spy); history.undo(); expect(spy).to.have.been.called; }); it('emits a change event', function () { - history.perform(action); + history.perform(actionNoop); history.on('change', spy); var difference = history.undo(); expect(spy).to.have.been.calledWith(difference); @@ -241,7 +246,7 @@ describe('iD.History', function () { }); it('does redo into an annotated state', function () { - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); history.on('redone', spy); history.undo(); history.redo(); @@ -250,7 +255,7 @@ describe('iD.History', function () { }); it('does not redo into a non-annotated state', function () { - history.perform(action); + history.perform(actionNoop); history.on('redone', spy); history.undo(); history.redo(); @@ -258,7 +263,7 @@ describe('iD.History', function () { }); it('emits a change event', function () { - history.perform(action); + history.perform(actionNoop); history.undo(); history.on('change', spy); var difference = history.redo(); @@ -268,16 +273,16 @@ describe('iD.History', function () { describe('#pauseChangeDispatch / #resumeChangeDispatch', function() { it('prevents change events from getting dispatched', function() { - history.perform(action, 'base'); + history.perform(actionNoop, 'base'); history.on('change', spy); history.pauseChangeDispatch(); - history.perform(action, 'perform'); + history.perform(actionNoop, 'perform'); expect(spy).to.have.been.notCalled; - history.replace(action, 'replace'); + history.replace(actionNoop, 'replace'); expect(spy).to.have.been.notCalled; - history.overwrite(action, 'replace'); + history.overwrite(actionNoop, 'replace'); expect(spy).to.have.been.notCalled; history.undo(); expect(spy).to.have.been.notCalled; @@ -286,8 +291,31 @@ describe('iD.History', function () { history.pop(); expect(spy).to.have.been.notCalled; - var difference = history.resumeChangeDispatch(); - expect(spy).to.have.been.calledOnceWith(difference); + var diff = history.resumeChangeDispatch(); + expect(spy).to.have.been.calledOnceWith(diff); + }); + + it('does nothing if resume called before pause', function() { + history.perform(actionNoop, 'base'); + history.on('change', spy); + + history.resumeChangeDispatch(); + expect(spy).to.have.been.notCalled; + }); + + it('uses earliest difference if pause called multiple times', function() { + history.perform(actionNoop, 'base'); + history.on('change', spy); + + history.pauseChangeDispatch(); + history.perform(actionAddNode('a'), 'perform'); + + history.pauseChangeDispatch(); + history.perform(actionAddNode('b'), 'perform'); + + var diff = history.resumeChangeDispatch(); + expect(spy).to.have.been.calledOnceWith(diff); + expect(diff.changes()).to.include.keys(['a', 'b']); }); }); @@ -328,8 +356,8 @@ describe('iD.History', function () { describe('#reset', function () { it('clears the version stack', function () { - history.perform(action, 'annotation'); - history.perform(action, 'annotation'); + history.perform(actionNoop, 'annotation'); + history.perform(actionNoop, 'annotation'); history.undo(); history.reset(); expect(history.undoAnnotation()).to.be.undefined; @@ -345,16 +373,16 @@ describe('iD.History', function () { describe('#checkpoint', function () { it('saves and resets to checkpoints', function () { - history.perform(action, 'annotation1'); - history.perform(action, 'annotation2'); - history.perform(action, 'annotation3'); + history.perform(actionNoop, 'annotation1'); + history.perform(actionNoop, 'annotation2'); + history.perform(actionNoop, 'annotation3'); history.checkpoint('check1'); - history.perform(action, 'annotation4'); - history.perform(action, 'annotation5'); + history.perform(actionNoop, 'annotation4'); + history.perform(actionNoop, 'annotation5'); history.checkpoint('check2'); - history.perform(action, 'annotation6'); - history.perform(action, 'annotation7'); - history.perform(action, 'annotation8'); + history.perform(actionNoop, 'annotation6'); + history.perform(actionNoop, 'annotation7'); + history.perform(actionNoop, 'annotation8'); history.reset('check1'); expect(history.undoAnnotation()).to.equal('annotation3'); @@ -375,8 +403,7 @@ describe('iD.History', function () { describe('#toJSON', function() { it('doesn\'t generate unsaveable changes', function() { - var node_1 = iD.osmNode({id: 'n-1'}); - history.perform(iD.actionAddEntity(node_1)); + history.perform(actionAddNode('n-1')); history.perform(iD.actionDeleteNode('n-1')); expect(history.toJSON()).to.be.not.ok; }); From e216d5be3e5dfea27ca96c51febf95381fa1d45a Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Wed, 6 Mar 2019 15:37:12 -0500 Subject: [PATCH 3/3] Avoid dispatching extra change events, don't mutate childNodes (closes #5996, related #5941) --- modules/behavior/draw_way.js | 40 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/modules/behavior/draw_way.js b/modules/behavior/draw_way.js index f017d03e5..db3a80a79 100644 --- a/modules/behavior/draw_way.js +++ b/modules/behavior/draw_way.js @@ -1,3 +1,5 @@ +import _clone from 'lodash-es/clone'; + import { event as d3_event, select as d3_select @@ -30,6 +32,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin // Push an annotated state for undo to return back to. // We must make sure to remove this edit later. + context.pauseChangeDispatch(); context.perform(actionNoop(), annotation); _tempEdits++; @@ -37,6 +40,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin // We must make sure to remove this edit later. context.perform(_actionAddDrawNode()); _tempEdits++; + context.resumeChangeDispatch(); function keydown() { @@ -122,7 +126,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin for (var i = 0; i < parents.length; i++) { var parent = parents[i]; - var nodes = graph.childNodes(parent); + var nodes = _clone(graph.childNodes(parent)); if (origWay.isClosed()) { // Check if Area if (finishDraw) { @@ -149,17 +153,20 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin 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 - resetContextGraphToStartGraph(); + resetToStartGraph(); + context.resumeChangeDispatch(); context.enter(modeSelect(context, [wayID])); } else { // Remove whatever segment was drawn previously and continue drawing context.pop(1); + context.resumeChangeDispatch(); context.enter(mode); } } @@ -171,6 +178,13 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin } + function resetToStartGraph() { + while (context.graph() !== startGraph) { + context.pop(); + } + } + + var drawWay = function(surface) { behavior .on('move', move) @@ -197,19 +211,16 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin .on('undone.draw', undone); }; - function resetContextGraphToStartGraph() { - while (context.graph() !== startGraph) { - context.pop(); - } - } 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) { + context.pauseChangeDispatch(); context.pop(_tempEdits); - resetContextGraphToStartGraph(); + resetToStartGraph(); + context.resumeChangeDispatch(); } context.map() @@ -257,6 +268,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin return; // can't click here } + context.pauseChangeDispatch(); context.pop(_tempEdits); _tempEdits = 0; @@ -265,6 +277,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin annotation ); + context.resumeChangeDispatch(); checkGeometry(false); // finishDraw = false context.enter(mode); }; @@ -276,6 +289,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin return; // can't click here } + context.pauseChangeDispatch(); context.pop(_tempEdits); _tempEdits = 0; @@ -285,6 +299,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin annotation ); + context.resumeChangeDispatch(); checkGeometry(false); // finishDraw = false context.enter(mode); }; @@ -296,6 +311,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin return; // can't click here } + context.pauseChangeDispatch(); context.pop(_tempEdits); _tempEdits = 0; @@ -304,6 +320,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin annotation ); + context.resumeChangeDispatch(); checkGeometry(false); // finishDraw = false context.enter(mode); }; @@ -318,6 +335,7 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin return; // can't click here } + context.pauseChangeDispatch(); context.pop(_tempEdits); _tempEdits = 0; @@ -327,6 +345,8 @@ export function behaviorDrawWay(context, wayID, index, mode, startGraph, baselin return; } + context.resumeChangeDispatch(); + window.setTimeout(function() { context.map().dblclickEnable(true); }, 1000); @@ -340,10 +360,12 @@ 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; - resetContextGraphToStartGraph(); + resetToStartGraph(); + context.resumeChangeDispatch(); window.setTimeout(function() { context.map().dblclickEnable(true);