From 38958515d8f729777dcd2719c24b709ea86a6339 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 3 Dec 2012 17:26:38 -0500 Subject: [PATCH 1/7] Clean up inspector setup --- js/id/id.js | 3 ++- js/id/renderer/map.js | 12 +++--------- js/id/ui/inspector.js | 1 - 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/js/id/id.js b/js/id/id.js index 8d178901e..a8600c42c 100644 --- a/js/id/id.js +++ b/js/id/id.js @@ -107,7 +107,8 @@ var iD = function(container) { .on('click', function(d) { return d[2](); }); this.append('div') - .attr('class', 'inspector-wrap').style('display', 'none'); + .attr('class', 'inspector-wrap') + .style('display', 'none'); this.append('div') .attr('id', 'about') diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index a6219db8a..cd8c4d908 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -129,9 +129,6 @@ iD.Map = function() { arrow.remove(); map.size(this.size()); - - hideInspector(); - map.surface = surface; } @@ -442,7 +439,8 @@ iD.Map = function() { selection = entity.id; d3.select('.inspector-wrap') .style('display', 'block') - .datum(map.history.graph().fetch(entity.id)).call(inspector); + .datum(map.history.graph().fetch(entity.id)) + .call(inspector); redraw(); } @@ -451,11 +449,7 @@ iD.Map = function() { if (entity) entity = entity[0]; if (!entity || selection === entity.id || (entity.tags && entity.tags.elastic)) return; if (entity.type === 'way') d3.select(d3.event.target).call(waydragbehavior); - selection = entity.id; - d3.select('.inspector-wrap') - .style('display', 'block') - .datum(map.history.graph().fetch(entity.id)).call(inspector); - redraw(); + selectEntity(entity); } function removeEntity(entity) { diff --git a/js/id/ui/inspector.js b/js/id/ui/inspector.js index 7e9ea12bb..96c6185a2 100644 --- a/js/id/ui/inspector.js +++ b/js/id/ui/inspector.js @@ -141,6 +141,5 @@ iD.Inspector = function() { }); } - return d3.rebind(inspector, event, 'on'); }; From 0370b487e3246f5a98a615abdedbb70d34987842 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 3 Dec 2012 17:30:52 -0500 Subject: [PATCH 2/7] Inject history dependency in map --- js/id/actions/modes.js | 36 ++++++++++++++++++------------------ js/id/id.js | 14 ++++++++------ js/id/renderer/map.js | 38 +++++++++++++++++++++----------------- test/spec/Map.js | 5 +++++ 4 files changed, 52 insertions(+), 41 deletions(-) diff --git a/js/id/actions/modes.js b/js/id/actions/modes.js index 7a94e21c7..650b893d5 100644 --- a/js/id/actions/modes.js +++ b/js/id/actions/modes.js @@ -100,7 +100,7 @@ iD.modes.AddRoad = { if (t.data() && t.data()[0] && t.data()[0].type === 'node') { // continue an existing way var id = t.data()[0].id; - var parents = this.map.history.graph().parents(id); + var parents = this.map.history().graph().parents(id); if (parents.length && parents[0].nodes[0] === id) { way = parents[0]; direction = 'backward'; @@ -115,7 +115,7 @@ iD.modes.AddRoad = { var index = iD.modes.chooseIndex(t.data()[0], d3.mouse(surface.node()), this.map); node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); - var connectedWay = this.map.history.graph().entity(t.data()[0].id); + var connectedWay = this.map.history().graph().entity(t.data()[0].id); connectedWay.nodes.splice(index, 0, node.id); this.map.perform(iD.actions.addWayNode(connectedWay, node)); } else { @@ -160,7 +160,7 @@ iD.modes.DrawRoad = function(way_id, direction) { nextnode = iD.modes._node([NaN, NaN]); var nextnode_id = nextnode.id; - var way = this.map.history.graph().entity(way_id), + var way = this.map.history().graph().entity(way_id), firstNode = way.nodes[0], lastNode = _.last(way.nodes); way.nodes[push](nextnode_id); @@ -168,11 +168,11 @@ iD.modes.DrawRoad = function(way_id, direction) { function mousemove() { var ll = this.map.projection.invert(d3.mouse(surface.node())); - var way = this.map.history.graph().entity(way_id); - var node = iD.Entity(this.map.history.graph().entity(nextnode_id), { + var way = this.map.history().graph().entity(way_id); + var node = iD.Entity(this.map.history().graph().entity(nextnode_id), { lon: ll[0], lat: ll[1] }); - this.map.history.replace(iD.actions.addWayNode(way, node)); + this.map.history().replace(iD.actions.addWayNode(way, node)); var only = iD.Util.trueObj([way.id].concat(_.pluck(way.nodes, 'id'))); this.map.redraw(only); } @@ -182,18 +182,18 @@ iD.modes.DrawRoad = function(way_id, direction) { d3.event.stopPropagation(); if (t.data() && t.data()[0] && t.data()[0].type === 'node') { if (t.data()[0].id == firstNode || t.data()[0].id == lastNode) { - var l = this.map.history.graph().entity(way.nodes[pop]()); + var l = this.map.history().graph().entity(way.nodes[pop]()); this.map.perform(iD.actions.removeWayNode(way, l)); // If this is drawing a loop and this is not the drawing // end of the stick, finish the circle if (direction === 'forward' && t.data()[0].id == firstNode) { way.nodes[push](firstNode); this.map.perform(iD.actions.addWayNode(way, - this.map.history.graph().entity(firstNode))); + this.map.history().graph().entity(firstNode))); } else if (direction === 'backward' && t.data()[0].id == lastNode) { way.nodes[push](lastNode); this.map.perform(iD.actions.addWayNode(way, - this.map.history.graph().entity(lastNode))); + this.map.history().graph().entity(lastNode))); } delete way.tags.elastic; this.map.perform(iD.actions.changeTags(way, way.tags)); @@ -208,14 +208,14 @@ iD.modes.DrawRoad = function(way_id, direction) { var index = iD.modes.chooseIndex(t.data()[0], d3.mouse(surface.node()), this.map); node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); - var connectedWay = this.map.history.graph().entity(t.data()[0].id); + var connectedWay = this.map.history().graph().entity(t.data()[0].id); connectedWay.nodes.splice(1, 0, node.id); this.map.perform(iD.actions.addWayNode(connectedWay, node)); } else { node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); } - var old = this.map.history.graph().entity(way.nodes[pop]()); + var old = this.map.history().graph().entity(way.nodes[pop]()); this.map.perform(iD.actions.removeWayNode(way, old)); way.nodes[push](node.id); this.map.perform(iD.actions.addWayNode(way, node)); @@ -304,7 +304,7 @@ iD.modes.DrawArea = function(way_id) { nextnode = iD.modes._node([NaN, NaN]); var surface = this.map.surface, - way = this.map.history.graph().entity(way_id), + way = this.map.history().graph().entity(way_id), firstnode_id = _.first(way.nodes), nextnode_id = nextnode.id; @@ -313,12 +313,12 @@ iD.modes.DrawArea = function(way_id) { function mousemove() { var ll = this.map.projection.invert(d3.mouse(surface.node())); - var way = this.map.history.graph().entity(way_id); - var node = iD.Entity(this.map.history.graph().entity(nextnode_id), { + var way = this.map.history().graph().entity(way_id); + var node = iD.Entity(this.map.history().graph().entity(nextnode_id), { lon: ll[0], lat: ll[1] }); - this.map.history.replace(iD.actions.addWayNode(way, node)); + this.map.history().replace(iD.actions.addWayNode(way, node)); var only = iD.Util.trueObj([way.id].concat(_.pluck(way.nodes, 'id'))); this.map.redraw(only); } @@ -328,11 +328,11 @@ iD.modes.DrawArea = function(way_id) { d3.event.stopPropagation(); if (t.data() && t.data()[0] && t.data()[0].type === 'node') { if (t.data()[0].id == firstnode_id) { - var l = this.map.history.graph().entity(way.nodes.pop()); + var l = this.map.history().graph().entity(way.nodes.pop()); this.map.perform(iD.actions.removeWayNode(way, l)); way.nodes.push(way.nodes[0]); this.map.perform(iD.actions.addWayNode(way, - this.map.history.graph().entity(way.nodes[0]))); + this.map.history().graph().entity(way.nodes[0]))); delete way.tags.elastic; this.map.perform(iD.actions.changeTags(way, way.tags)); // End by clicking on own tail @@ -345,7 +345,7 @@ iD.modes.DrawArea = function(way_id) { node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); } - var old = this.map.history.graph().entity(way.nodes.pop()); + var old = this.map.history().graph().entity(way.nodes.pop()); this.map.perform(iD.actions.removeWayNode(way, old)); way.nodes.push(node.id); this.map.perform(iD.actions.addWayNode(way, node)); diff --git a/js/id/id.js b/js/id/id.js index a8600c42c..148968e6d 100644 --- a/js/id/id.js +++ b/js/id/id.js @@ -1,8 +1,10 @@ var iD = function(container) { var connection = iD.Connection() .url('http://api06.dev.openstreetmap.org'), + history = iD.History(), map = iD.Map() - .connection(connection), + .connection(connection) + .history(history), controller = iD.Controller(map); map.background.source(iD.Background.Bing); @@ -75,9 +77,9 @@ var iD = function(container) { function save(e) { d3.select('.shaded').remove(); var l = iD.loading('uploading changes to openstreetmap'); - connection.putChangeset(map.history.changes(), e.comment, function() { + connection.putChangeset(history.changes(), e.comment, function() { l.remove(); - map.history = iD.History(); + map.history(iD.History()); map.flush().redraw(); }); } @@ -89,7 +91,7 @@ var iD = function(container) { }); var modal = shaded.append('div') .attr('class', 'modal commit-pane') - .datum(map.history.changes()); + .datum(history.changes()); modal.call(iD.commit() .on('cancel', function() { shaded.remove(); @@ -117,8 +119,8 @@ var iD = function(container) { "/ imagery © 2012 Bing, GeoEye, Getmapping, Intermap, Microsoft.

"); map.on('update', function() { - var undo = map.history.undoAnnotation(), - redo = map.history.redoAnnotation(); + var undo = history.undoAnnotation(), + redo = history.redoAnnotation(); bar.select('#undo') .property('disabled', !undo) diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index cd8c4d908..7a720838d 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -1,5 +1,5 @@ iD.Map = function() { - var connection, + var connection, history, dimensions = [], dispatch = d3.dispatch('move', 'update'), inspector = iD.Inspector(), @@ -33,12 +33,12 @@ iD.Map = function() { if (!dragging) { dragging = true; only = iD.Util.trueObj([entity.id].concat( - _.pluck(map.history.graph().parents(entity.id), 'id'))); - map.history.perform(iD.actions.noop()); + _.pluck(history.graph().parents(entity.id), 'id'))); + history.perform(iD.actions.noop()); } var to = projection.invert([d3.event.x, d3.event.y]); - map.history.replace(iD.actions.move(entity, to)); + history.replace(iD.actions.move(entity, to)); redraw(only); }) @@ -132,8 +132,6 @@ iD.Map = function() { map.surface = surface; } - map.history = iD.History(); - function prefixMatch(p) { // via mbostock var i = -1, n = p.length, s = document.body.style; while (++i < n) if (p[i] + 'Transform' in s) return '-' + p[i].toLowerCase() + '-'; @@ -163,7 +161,7 @@ iD.Map = function() { if (surface.style(transformProp) != 'none') return; var all = [], ways = [], areas = [], points = [], waynodes = [], extent = map.extent(), - graph = map.history.graph(); + graph = history.graph(); if (!only) { all = graph.intersects(extent); @@ -404,7 +402,7 @@ iD.Map = function() { if (result instanceof Error) { // TODO: handle } else { - map.history.merge(result); + history.merge(result); drawVector(iD.Util.trueObj(Object.keys(result.entities))); } }); @@ -439,7 +437,7 @@ iD.Map = function() { selection = entity.id; d3.select('.inspector-wrap') .style('display', 'block') - .datum(map.history.graph().fetch(entity.id)) + .datum(history.graph().fetch(entity.id)) .call(inspector); redraw(); } @@ -454,7 +452,7 @@ iD.Map = function() { function removeEntity(entity) { // Remove this node from any ways that is a member of - map.history.graph().parents(entity.id) + history.graph().parents(entity.id) .filter(function(d) { return d.type === 'way'; }) .forEach(function(parent) { parent.nodes = _.without(parent.nodes, entity.id); @@ -464,7 +462,7 @@ iD.Map = function() { } inspector.on('changeTags', function(d, tags) { - var entity = map.history.graph().entity(d.id); + var entity = history.graph().entity(d.id); map.perform(iD.actions.changeTags(entity, tags)); }).on('changeWayDirection', function(d) { map.perform(iD.actions.changeWayDirection(d)); @@ -518,21 +516,21 @@ iD.Map = function() { } map.perform = function(action) { - map.history.perform(action); + history.perform(action); map.update(); redraw(); return map; }; map.undo = function() { - map.history.undo(); + history.undo(); map.update(); redraw(); return map; }; map.redo = function() { - map.history.redo(); + history.redo(); map.update(); redraw(); return map; @@ -611,9 +609,15 @@ iD.Map = function() { }; map.connection = function(_) { - if (!arguments.length) return connection; - connection = _; - return map; + if (!arguments.length) return connection; + connection = _; + return map; + }; + + map.history = function (_) { + if (!arguments.length) return history; + history = _; + return map; }; map.background = background; diff --git a/test/spec/Map.js b/test/spec/Map.js index 63054d7a4..82ff270d5 100644 --- a/test/spec/Map.js +++ b/test/spec/Map.js @@ -66,6 +66,11 @@ describe('Map', function() { beforeEach(function () { spy = sinon.spy(); + map.history({ + perform: function () {}, + undo: function () {}, + redo: function () {} + }); map.on('update', spy); }); From e93c9624d82b351f33254b8561ebe6d5756c8052 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 3 Dec 2012 17:44:24 -0500 Subject: [PATCH 3/7] Convert History to module pattern There is only ever one History, so memory use is not a concern. --- js/id/graph/history.js | 152 ++++++++++++++++++++--------------------- test/index.html | 1 + test/spec/history.js | 41 +++++++++++ 3 files changed, 117 insertions(+), 77 deletions(-) create mode 100644 test/spec/history.js diff --git a/js/id/graph/history.js b/js/id/graph/history.js index 8d2eb586d..501b0a037 100644 --- a/js/id/graph/history.js +++ b/js/id/graph/history.js @@ -1,85 +1,83 @@ iD.History = function() { - if (!(this instanceof iD.History)) return new iD.History(); - this.stack = [iD.Graph()]; - this.index = 0; -}; + var stack = [iD.Graph()], + index = 0; -iD.History.prototype = { + return { + graph: function () { + return stack[index]; + }, - graph: function() { - return this.stack[this.index]; - }, + merge: function (graph) { + for (var i = 0; i < stack.length; i++) { + stack[i] = stack[i].merge(graph); + } + }, - merge: function(graph) { - for (var i = 0; i < this.stack.length; i++) { - this.stack[i] = this.stack[i].merge(graph); - } - }, - - perform: function(action) { - this.stack = this.stack.slice(0, this.index + 1); - this.stack.push(action(this.graph())); - this.index++; - }, - - replace: function(action) { - // assert(this.index == this.stack.length - 1) - this.stack[this.index] = action(this.graph()); - }, - - undo: function() { - while (this.index > 0) { - this.index--; - if (this.stack[this.index].annotation) break; - } - }, - - redo: function() { - while (this.index < this.stack.length - 1) { - this.index++; - if (this.stack[this.index].annotation) break; - } - }, - - undoAnnotation: function() { - var index = this.index; - while (index >= 0) { - if (this.stack[index].annotation) return this.stack[index].annotation; - index--; - } - }, - - redoAnnotation: function() { - var index = this.index + 1; - while (index <= this.stack.length - 1) { - if (this.stack[index].annotation) return this.stack[index].annotation; + perform: function (action) { + stack = stack.slice(0, index + 1); + stack.push(action(this.graph())); index++; + }, + + replace: function (action) { + // assert(index == stack.length - 1) + stack[index] = action(this.graph()); + }, + + undo: function () { + while (index > 0) { + index--; + if (stack[index].annotation) break; + } + }, + + redo: function () { + while (index < stack.length - 1) { + index++; + if (stack[index].annotation) break; + } + }, + + undoAnnotation: function () { + var i = index; + while (i >= 0) { + if (stack[i].annotation) return stack[i].annotation; + i--; + } + }, + + redoAnnotation: function () { + var i = index + 1; + while (i <= stack.length - 1) { + if (stack[i].annotation) return stack[i].annotation; + i++; + } + }, + + // generate reports of changes for changesets to use + modify: function () { + return stack[index].modifications(); + }, + + create: function () { + return stack[index].creations(); + }, + + 'delete': function () { + return _.difference( + _.pluck(stack[0].entities, 'id'), + _.pluck(stack[index].entities, 'id') + ).map(function (id) { + return stack[0].fetch(id); + }); + }, + + changes: function () { + return { + modify: this.modify(), + create: this.create(), + 'delete': this['delete']() + }; } - }, - - // generate reports of changes for changesets to use - modify: function() { - return this.stack[this.index].modifications(); - }, - - create: function() { - return this.stack[this.index].creations(); - }, - - 'delete': function() { - return _.difference( - _.pluck(this.stack[0].entities, 'id'), - _.pluck(this.stack[this.index].entities, 'id') - ).map(function(id) { - return this.stack[0].fetch(id); - }.bind(this)); - }, - - changes: function() { - return { - modify: this.modify(), - create: this.create(), - 'delete': this['delete']() - }; } }; diff --git a/test/index.html b/test/index.html index 493a386ec..f427bbf7c 100644 --- a/test/index.html +++ b/test/index.html @@ -72,6 +72,7 @@ + diff --git a/test/spec/history.js b/test/spec/history.js new file mode 100644 index 000000000..ad2a2585b --- /dev/null +++ b/test/spec/history.js @@ -0,0 +1,41 @@ +describe("History", function () { + var history, spy, + graph = iD.Graph([], "action"), + action = function() { return graph; }; + + beforeEach(function () { + history = iD.History(); + }); + + describe("#graph", function () { + it("returns the current graph", function () { + expect(history.graph()).to.be.an.instanceOf(iD.Graph); + }); + }); + + describe("#perform", function () { + it("updates the graph", function () { + history.perform(action); + expect(history.graph()).to.equal(graph); + }); + + it("pushes the undo stack", function () { + history.perform(action); + expect(history.undoAnnotation()).to.equal("action"); + }); + }); + + describe("#undo", function () { + it("pops the undo stack", function () { + history.perform(action); + history.undo(); + expect(history.undoAnnotation()).to.be.undefined; + }); + + it("pushes the redo stack", function () { + history.perform(action); + history.undo(); + expect(history.redoAnnotation()).to.equal("action"); + }); + }); +}); From 8a8d6fae32ede4324556f02f89f7a98a1c1ceabe Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 3 Dec 2012 18:00:29 -0500 Subject: [PATCH 4/7] Move responsibility for dispatching change event to history --- js/id/graph/history.js | 19 ++++++++++++++++--- js/id/id.js | 2 +- js/id/renderer/map.js | 8 ++------ test/spec/Map.js | 29 ----------------------------- test/spec/history.js | 35 +++++++++++++++++++++++++++++++++++ 5 files changed, 54 insertions(+), 39 deletions(-) diff --git a/js/id/graph/history.js b/js/id/graph/history.js index 501b0a037..41feba27c 100644 --- a/js/id/graph/history.js +++ b/js/id/graph/history.js @@ -1,8 +1,15 @@ iD.History = function() { var stack = [iD.Graph()], - index = 0; + index = 0, + dispatch = d3.dispatch('change'); - return { + function maybeChange() { + if (stack[index].annotation) { + dispatch.change(); + } + } + + var history = { graph: function () { return stack[index]; }, @@ -17,11 +24,13 @@ iD.History = function() { stack = stack.slice(0, index + 1); stack.push(action(this.graph())); index++; + maybeChange(); }, replace: function (action) { // assert(index == stack.length - 1) stack[index] = action(this.graph()); + maybeChange(); }, undo: function () { @@ -29,6 +38,7 @@ iD.History = function() { index--; if (stack[index].annotation) break; } + dispatch.change(); }, redo: function () { @@ -36,6 +46,7 @@ iD.History = function() { index++; if (stack[index].annotation) break; } + dispatch.change(); }, undoAnnotation: function () { @@ -79,5 +90,7 @@ iD.History = function() { 'delete': this['delete']() }; } - } + }; + + return d3.rebind(history, dispatch, 'on'); }; diff --git a/js/id/id.js b/js/id/id.js index 148968e6d..c053504c4 100644 --- a/js/id/id.js +++ b/js/id/id.js @@ -118,7 +118,7 @@ var iD = function(container) { "report a bug " + "/ imagery © 2012 Bing, GeoEye, Getmapping, Intermap, Microsoft.

"); - map.on('update', function() { + history.on('change', function() { var undo = history.undoAnnotation(), redo = history.redoAnnotation(); diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index 7a720838d..611b2db3c 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -1,7 +1,7 @@ iD.Map = function() { var connection, history, dimensions = [], - dispatch = d3.dispatch('move', 'update'), + dispatch = d3.dispatch('move'), inspector = iD.Inspector(), selection = null, translateStart, @@ -45,7 +45,6 @@ iD.Map = function() { .on('dragend', function () { if (dragging) { dragging = false; - map.update(); redraw(); } }), @@ -517,21 +516,18 @@ iD.Map = function() { map.perform = function(action) { history.perform(action); - map.update(); redraw(); return map; }; map.undo = function() { history.undo(); - map.update(); redraw(); return map; }; map.redo = function() { history.redo(); - map.update(); redraw(); return map; }; @@ -626,5 +622,5 @@ iD.Map = function() { map.selectEntity = selectEntity; map.dblclickEnable = dblclickEnable; - return d3.rebind(map, dispatch, 'on', 'move', 'update'); + return d3.rebind(map, dispatch, 'on', 'move'); }; diff --git a/test/spec/Map.js b/test/spec/Map.js index 82ff270d5..ec8d9484d 100644 --- a/test/spec/Map.js +++ b/test/spec/Map.js @@ -61,35 +61,6 @@ describe('Map', function() { }); }); - describe("update", function () { - var spy; - - beforeEach(function () { - spy = sinon.spy(); - map.history({ - perform: function () {}, - undo: function () {}, - redo: function () {} - }); - map.on('update', spy); - }); - - it("is emitted when performing an action", function () { - map.perform(iD.actions.noop); - expect(spy).to.have.been.called; - }); - - it("is emitted when undoing an action", function () { - map.undo(); - expect(spy).to.have.been.called; - }); - - it("is emitted when redoing an action", function () { - map.redo(); - expect(spy).to.have.been.called; - }); - }); - describe("surface", function() { it("is an SVG element", function() { expect(map.surface.node().tagName).to.equal("svg"); diff --git a/test/spec/history.js b/test/spec/history.js index ad2a2585b..821e15390 100644 --- a/test/spec/history.js +++ b/test/spec/history.js @@ -38,4 +38,39 @@ describe("History", function () { expect(history.redoAnnotation()).to.equal("action"); }); }); + + describe("change", function () { + var spy; + + beforeEach(function () { + spy = sinon.spy(); + }); + + it("is not emitted when performing a noop", function () { + history.on('change', spy); + history.perform(iD.actions.noop); + expect(spy).not.to.have.been.called; + }); + + it("is emitted when performing an action", function () { + history.on('change', spy); + history.perform(action); + expect(spy).to.have.been.called; + }); + + it("is emitted when undoing an action", function () { + history.perform(action); + history.on('change', spy); + history.undo(); + expect(spy).to.have.been.called; + }); + + it("is emitted when redoing an action", function () { + history.perform(action); + history.undo(); + history.on('change', spy); + history.redo(); + expect(spy).to.have.been.called; + }); + }); }); From 0a1e0bdfe4af0953398f9777b480d7142e944cb3 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 3 Dec 2012 18:09:05 -0500 Subject: [PATCH 5/7] History#reset --- js/id/graph/history.js | 11 +++++++++-- js/id/id.js | 2 +- test/spec/history.js | 24 ++++++++++++++++++------ 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/js/id/graph/history.js b/js/id/graph/history.js index 41feba27c..11a6111ce 100644 --- a/js/id/graph/history.js +++ b/js/id/graph/history.js @@ -1,6 +1,5 @@ iD.History = function() { - var stack = [iD.Graph()], - index = 0, + var stack, index, dispatch = d3.dispatch('change'); function maybeChange() { @@ -89,8 +88,16 @@ iD.History = function() { create: this.create(), 'delete': this['delete']() }; + }, + + reset: function () { + stack = [iD.Graph()]; + index = 0; + dispatch.change(); } }; + history.reset(); + return d3.rebind(history, dispatch, 'on'); }; diff --git a/js/id/id.js b/js/id/id.js index c053504c4..6a54170b8 100644 --- a/js/id/id.js +++ b/js/id/id.js @@ -79,7 +79,7 @@ var iD = function(container) { var l = iD.loading('uploading changes to openstreetmap'); connection.putChangeset(history.changes(), e.comment, function() { l.remove(); - map.history(iD.History()); + history.reset(); map.flush().redraw(); }); } diff --git a/test/spec/history.js b/test/spec/history.js index 821e15390..f45265b93 100644 --- a/test/spec/history.js +++ b/test/spec/history.js @@ -4,7 +4,8 @@ describe("History", function () { action = function() { return graph; }; beforeEach(function () { - history = iD.History(); + history = iD.History(); + spy = sinon.spy(); }); describe("#graph", function () { @@ -39,13 +40,24 @@ describe("History", function () { }); }); - describe("change", function () { - var spy; - - beforeEach(function () { - spy = sinon.spy(); + describe("#reset", function () { + it("clears the version stack", function () { + history.perform(action); + history.perform(action); + history.undo(); + history.reset(); + expect(history.undoAnnotation()).to.be.undefined; + expect(history.redoAnnotation()).to.be.undefined; }); + it("emits a change event", function () { + history.on('change', spy); + history.reset(); + expect(spy).to.have.been.called; + }); + }); + + describe("change", function () { it("is not emitted when performing a noop", function () { history.on('change', spy); history.perform(iD.actions.noop); From da5239b98de59413e16b0fae70bf9b164853b454 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 3 Dec 2012 18:36:16 -0500 Subject: [PATCH 6/7] Eliminate Map#{perform,undo,redo} Map binds to history changes instead. --- js/id/actions/modes.js | 40 +++++++++++++++---------------- js/id/controller/controller.js | 5 +++- js/id/id.js | 12 +++++----- js/id/renderer/map.js | 43 ++++++++++------------------------ 4 files changed, 42 insertions(+), 58 deletions(-) diff --git a/js/id/actions/modes.js b/js/id/actions/modes.js index 650b893d5..2ab14af37 100644 --- a/js/id/actions/modes.js +++ b/js/id/actions/modes.js @@ -42,7 +42,7 @@ iD.modes.AddPlace = { d3.mouse(surface.node())); var n = iD.modes._node(ll); n._poi = true; - this.map.perform(iD.actions.addNode(n)); + this.history.perform(iD.actions.addNode(n)); this.map.selectEntity(n); this.controller.exit(); this.exit(); @@ -117,16 +117,16 @@ iD.modes.AddRoad = { d3.mouse(surface.node()))); var connectedWay = this.map.history().graph().entity(t.data()[0].id); connectedWay.nodes.splice(index, 0, node.id); - this.map.perform(iD.actions.addWayNode(connectedWay, node)); + this.history.perform(iD.actions.addWayNode(connectedWay, node)); } else { node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); } if (start) { - this.map.perform(iD.actions.startWay(way)); + this.history.perform(iD.actions.startWay(way)); way.nodes.push(node.id); - this.map.perform(iD.actions.addWayNode(way, node)); + this.history.perform(iD.actions.addWayNode(way, node)); } this.controller.enter(iD.modes.DrawRoad(way.id, direction)); } @@ -164,7 +164,7 @@ iD.modes.DrawRoad = function(way_id, direction) { firstNode = way.nodes[0], lastNode = _.last(way.nodes); way.nodes[push](nextnode_id); - this.map.perform(iD.actions.addWayNode(way, nextnode)); + this.history.perform(iD.actions.addWayNode(way, nextnode)); function mousemove() { var ll = this.map.projection.invert(d3.mouse(surface.node())); @@ -183,20 +183,20 @@ iD.modes.DrawRoad = function(way_id, direction) { if (t.data() && t.data()[0] && t.data()[0].type === 'node') { if (t.data()[0].id == firstNode || t.data()[0].id == lastNode) { var l = this.map.history().graph().entity(way.nodes[pop]()); - this.map.perform(iD.actions.removeWayNode(way, l)); + this.history.perform(iD.actions.removeWayNode(way, l)); // If this is drawing a loop and this is not the drawing // end of the stick, finish the circle if (direction === 'forward' && t.data()[0].id == firstNode) { way.nodes[push](firstNode); - this.map.perform(iD.actions.addWayNode(way, + this.history.perform(iD.actions.addWayNode(way, this.map.history().graph().entity(firstNode))); } else if (direction === 'backward' && t.data()[0].id == lastNode) { way.nodes[push](lastNode); - this.map.perform(iD.actions.addWayNode(way, + this.history.perform(iD.actions.addWayNode(way, this.map.history().graph().entity(lastNode))); } delete way.tags.elastic; - this.map.perform(iD.actions.changeTags(way, way.tags)); + this.history.perform(iD.actions.changeTags(way, way.tags)); this.map.selectEntity(way); // End by clicking on own tail return this.controller.exit(); @@ -210,15 +210,15 @@ iD.modes.DrawRoad = function(way_id, direction) { d3.mouse(surface.node()))); var connectedWay = this.map.history().graph().entity(t.data()[0].id); connectedWay.nodes.splice(1, 0, node.id); - this.map.perform(iD.actions.addWayNode(connectedWay, node)); + this.history.perform(iD.actions.addWayNode(connectedWay, node)); } else { node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); } var old = this.map.history().graph().entity(way.nodes[pop]()); - this.map.perform(iD.actions.removeWayNode(way, old)); + this.history.perform(iD.actions.removeWayNode(way, old)); way.nodes[push](node.id); - this.map.perform(iD.actions.addWayNode(way, node)); + this.history.perform(iD.actions.addWayNode(way, node)); way.nodes = way.nodes.slice(); this.controller.enter(iD.modes.DrawRoad(way_id, direction)); } @@ -273,9 +273,9 @@ iD.modes.AddArea = { d3.mouse(surface.node()))); } - this.map.perform(iD.actions.startWay(way)); + this.history.perform(iD.actions.startWay(way)); way.nodes.push(node.id); - this.map.perform(iD.actions.addWayNode(way, node)); + this.history.perform(iD.actions.addWayNode(way, node)); this.map.selectEntity(way); this.controller.enter(iD.modes.DrawArea(way.id)); } @@ -309,7 +309,7 @@ iD.modes.DrawArea = function(way_id) { nextnode_id = nextnode.id; way.nodes.push(nextnode_id); - this.map.perform(iD.actions.addWayNode(way, nextnode)); + this.history.perform(iD.actions.addWayNode(way, nextnode)); function mousemove() { var ll = this.map.projection.invert(d3.mouse(surface.node())); @@ -329,12 +329,12 @@ iD.modes.DrawArea = function(way_id) { if (t.data() && t.data()[0] && t.data()[0].type === 'node') { if (t.data()[0].id == firstnode_id) { var l = this.map.history().graph().entity(way.nodes.pop()); - this.map.perform(iD.actions.removeWayNode(way, l)); + this.history.perform(iD.actions.removeWayNode(way, l)); way.nodes.push(way.nodes[0]); - this.map.perform(iD.actions.addWayNode(way, + this.history.perform(iD.actions.addWayNode(way, this.map.history().graph().entity(way.nodes[0]))); delete way.tags.elastic; - this.map.perform(iD.actions.changeTags(way, way.tags)); + this.history.perform(iD.actions.changeTags(way, way.tags)); // End by clicking on own tail return this.controller.exit(); } else { @@ -346,9 +346,9 @@ iD.modes.DrawArea = function(way_id) { d3.mouse(surface.node()))); } var old = this.map.history().graph().entity(way.nodes.pop()); - this.map.perform(iD.actions.removeWayNode(way, old)); + this.history.perform(iD.actions.removeWayNode(way, old)); way.nodes.push(node.id); - this.map.perform(iD.actions.addWayNode(way, node)); + this.history.perform(iD.actions.addWayNode(way, node)); way.nodes = way.nodes.slice(); this.controller.enter(iD.modes.DrawArea(way_id)); } diff --git a/js/id/controller/controller.js b/js/id/controller/controller.js index c3721b4eb..ac204b753 100644 --- a/js/id/controller/controller.js +++ b/js/id/controller/controller.js @@ -1,16 +1,19 @@ // A controller holds a single action at a time and calls `.enter` and `.exit` // to bind and unbind actions. -iD.Controller = function(map) { +iD.Controller = function(map, history) { var event = d3.dispatch('enter', 'exit'); var controller = { mode: null }; controller.enter = function(mode) { mode.controller = controller; + mode.history = history; mode.map = map; + if (controller.mode) { controller.mode.exit(); event.exit(controller.mode); } + mode.enter(); controller.mode = mode; event.enter(mode); diff --git a/js/id/id.js b/js/id/id.js index 6a54170b8..25d43ecec 100644 --- a/js/id/id.js +++ b/js/id/id.js @@ -5,7 +5,7 @@ var iD = function(container) { map = iD.Map() .connection(connection) .history(history), - controller = iD.Controller(map); + controller = iD.Controller(map, history); map.background.source(iD.Background.Bing); @@ -39,13 +39,13 @@ var iD = function(container) { .attr({ id: 'undo', 'class': 'mini' }) .property('disabled', true) .html('←') - .on('click', map.undo); + .on('click', history.undo); bar.append('button') .attr({ id: 'redo', 'class': 'mini' }) .property('disabled', true) .html('→') - .on('click', map.redo); + .on('click', history.redo); bar.append('input') .attr({ type: 'text', placeholder: 'find a place', id: 'geocode-location' }) @@ -118,7 +118,7 @@ var iD = function(container) { "report a bug " + "/ imagery © 2012 Bing, GeoEye, Getmapping, Intermap, Microsoft.

"); - history.on('change', function() { + history.on('change.buttons', function() { var undo = history.undoAnnotation(), redo = history.redoAnnotation(); @@ -140,11 +140,11 @@ var iD = function(container) { d3.select(document).on('keydown', function() { // cmd-z if (d3.event.which === 90 && d3.event.metaKey) { - map.undo(); + history.undo(); } // cmd-shift-z if (d3.event.which === 90 && d3.event.metaKey && d3.event.shiftKey) { - map.redo(); + history.redo(); } }); diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index 611b2db3c..9c19d6973 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -12,9 +12,8 @@ iD.Map = function() { .scale(projection.scale()) .scaleExtent([1024, 256 * Math.pow(2, 20)]) .on('zoom', zoomPan), - only, dblclickEnabled = true, - dragging = false, + dragging, dragbehavior = d3.behavior.drag() .origin(function(entity) { if (entity.accuracy) { @@ -31,8 +30,7 @@ iD.Map = function() { d3.event.sourceEvent.stopPropagation(); if (!dragging) { - dragging = true; - only = iD.Util.trueObj([entity.id].concat( + dragging = iD.Util.trueObj([entity.id].concat( _.pluck(history.graph().parents(entity.id), 'id'))); history.perform(iD.actions.noop()); } @@ -40,11 +38,11 @@ iD.Map = function() { var to = projection.invert([d3.event.x, d3.event.y]); history.replace(iD.actions.move(entity, to)); - redraw(only); + redraw(); }) .on('dragend', function () { if (dragging) { - dragging = false; + dragging = undefined; redraw(); } }), @@ -455,16 +453,16 @@ iD.Map = function() { .filter(function(d) { return d.type === 'way'; }) .forEach(function(parent) { parent.nodes = _.without(parent.nodes, entity.id); - map.perform(iD.actions.removeWayNode(parent, entity)); + history.perform(iD.actions.removeWayNode(parent, entity)); }); - map.perform(iD.actions.remove(entity)); + history.perform(iD.actions.remove(entity)); } inspector.on('changeTags', function(d, tags) { var entity = history.graph().entity(d.id); - map.perform(iD.actions.changeTags(entity, tags)); + history.perform(iD.actions.changeTags(entity, tags)); }).on('changeWayDirection', function(d) { - map.perform(iD.actions.changeWayDirection(d)); + history.perform(iD.actions.changeWayDirection(d)); }).on('remove', function(d) { removeEntity(d); hideInspector(); @@ -500,38 +498,20 @@ iD.Map = function() { redraw(); } - function redraw(only) { - if (!only) { + function redraw() { + if (!dragging) { dispatch.move(map); tilegroup.call(background); } if (map.zoom() > 16) { download(); - drawVector(only); + drawVector(dragging); } else { hideVector(); } return map; } - map.perform = function(action) { - history.perform(action); - redraw(); - return map; - }; - - map.undo = function() { - history.undo(); - redraw(); - return map; - }; - - map.redo = function() { - history.redo(); - redraw(); - return map; - }; - function dblclickEnable(_) { if (!arguments.length) return dblclickEnabled; dblclickEnabled = _; @@ -613,6 +593,7 @@ iD.Map = function() { map.history = function (_) { if (!arguments.length) return history; history = _; + history.on('change.map', redraw); return map; }; From fadbaf46a674a9eea013facbe534b4500d1098d8 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 3 Dec 2012 18:41:04 -0500 Subject: [PATCH 7/7] Use this.history directly --- js/id/actions/modes.js | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/js/id/actions/modes.js b/js/id/actions/modes.js index 2ab14af37..df318c50d 100644 --- a/js/id/actions/modes.js +++ b/js/id/actions/modes.js @@ -100,7 +100,7 @@ iD.modes.AddRoad = { if (t.data() && t.data()[0] && t.data()[0].type === 'node') { // continue an existing way var id = t.data()[0].id; - var parents = this.map.history().graph().parents(id); + var parents = this.history.graph().parents(id); if (parents.length && parents[0].nodes[0] === id) { way = parents[0]; direction = 'backward'; @@ -115,7 +115,7 @@ iD.modes.AddRoad = { var index = iD.modes.chooseIndex(t.data()[0], d3.mouse(surface.node()), this.map); node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); - var connectedWay = this.map.history().graph().entity(t.data()[0].id); + var connectedWay = this.history.graph().entity(t.data()[0].id); connectedWay.nodes.splice(index, 0, node.id); this.history.perform(iD.actions.addWayNode(connectedWay, node)); } else { @@ -160,7 +160,7 @@ iD.modes.DrawRoad = function(way_id, direction) { nextnode = iD.modes._node([NaN, NaN]); var nextnode_id = nextnode.id; - var way = this.map.history().graph().entity(way_id), + var way = this.history.graph().entity(way_id), firstNode = way.nodes[0], lastNode = _.last(way.nodes); way.nodes[push](nextnode_id); @@ -168,11 +168,11 @@ iD.modes.DrawRoad = function(way_id, direction) { function mousemove() { var ll = this.map.projection.invert(d3.mouse(surface.node())); - var way = this.map.history().graph().entity(way_id); - var node = iD.Entity(this.map.history().graph().entity(nextnode_id), { + var way = this.history.graph().entity(way_id); + var node = iD.Entity(this.history.graph().entity(nextnode_id), { lon: ll[0], lat: ll[1] }); - this.map.history().replace(iD.actions.addWayNode(way, node)); + this.history.replace(iD.actions.addWayNode(way, node)); var only = iD.Util.trueObj([way.id].concat(_.pluck(way.nodes, 'id'))); this.map.redraw(only); } @@ -182,18 +182,18 @@ iD.modes.DrawRoad = function(way_id, direction) { d3.event.stopPropagation(); if (t.data() && t.data()[0] && t.data()[0].type === 'node') { if (t.data()[0].id == firstNode || t.data()[0].id == lastNode) { - var l = this.map.history().graph().entity(way.nodes[pop]()); + var l = this.history.graph().entity(way.nodes[pop]()); this.history.perform(iD.actions.removeWayNode(way, l)); // If this is drawing a loop and this is not the drawing // end of the stick, finish the circle if (direction === 'forward' && t.data()[0].id == firstNode) { way.nodes[push](firstNode); this.history.perform(iD.actions.addWayNode(way, - this.map.history().graph().entity(firstNode))); + this.history.graph().entity(firstNode))); } else if (direction === 'backward' && t.data()[0].id == lastNode) { way.nodes[push](lastNode); this.history.perform(iD.actions.addWayNode(way, - this.map.history().graph().entity(lastNode))); + this.history.graph().entity(lastNode))); } delete way.tags.elastic; this.history.perform(iD.actions.changeTags(way, way.tags)); @@ -208,14 +208,14 @@ iD.modes.DrawRoad = function(way_id, direction) { var index = iD.modes.chooseIndex(t.data()[0], d3.mouse(surface.node()), this.map); node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); - var connectedWay = this.map.history().graph().entity(t.data()[0].id); + var connectedWay = this.history.graph().entity(t.data()[0].id); connectedWay.nodes.splice(1, 0, node.id); this.history.perform(iD.actions.addWayNode(connectedWay, node)); } else { node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); } - var old = this.map.history().graph().entity(way.nodes[pop]()); + var old = this.history.graph().entity(way.nodes[pop]()); this.history.perform(iD.actions.removeWayNode(way, old)); way.nodes[push](node.id); this.history.perform(iD.actions.addWayNode(way, node)); @@ -304,7 +304,7 @@ iD.modes.DrawArea = function(way_id) { nextnode = iD.modes._node([NaN, NaN]); var surface = this.map.surface, - way = this.map.history().graph().entity(way_id), + way = this.history.graph().entity(way_id), firstnode_id = _.first(way.nodes), nextnode_id = nextnode.id; @@ -313,12 +313,12 @@ iD.modes.DrawArea = function(way_id) { function mousemove() { var ll = this.map.projection.invert(d3.mouse(surface.node())); - var way = this.map.history().graph().entity(way_id); - var node = iD.Entity(this.map.history().graph().entity(nextnode_id), { + var way = this.history.graph().entity(way_id); + var node = iD.Entity(this.history.graph().entity(nextnode_id), { lon: ll[0], lat: ll[1] }); - this.map.history().replace(iD.actions.addWayNode(way, node)); + this.history.replace(iD.actions.addWayNode(way, node)); var only = iD.Util.trueObj([way.id].concat(_.pluck(way.nodes, 'id'))); this.map.redraw(only); } @@ -328,11 +328,11 @@ iD.modes.DrawArea = function(way_id) { d3.event.stopPropagation(); if (t.data() && t.data()[0] && t.data()[0].type === 'node') { if (t.data()[0].id == firstnode_id) { - var l = this.map.history().graph().entity(way.nodes.pop()); + var l = this.history.graph().entity(way.nodes.pop()); this.history.perform(iD.actions.removeWayNode(way, l)); way.nodes.push(way.nodes[0]); this.history.perform(iD.actions.addWayNode(way, - this.map.history().graph().entity(way.nodes[0]))); + this.history.graph().entity(way.nodes[0]))); delete way.tags.elastic; this.history.perform(iD.actions.changeTags(way, way.tags)); // End by clicking on own tail @@ -345,7 +345,7 @@ iD.modes.DrawArea = function(way_id) { node = iD.modes._node(this.map.projection.invert( d3.mouse(surface.node()))); } - var old = this.map.history().graph().entity(way.nodes.pop()); + var old = this.history.graph().entity(way.nodes.pop()); this.history.perform(iD.actions.removeWayNode(way, old)); way.nodes.push(node.id); this.history.perform(iD.actions.addWayNode(way, node));