From 3e45afbc5e6b8413f20b47ec96b81cc6b42e2857 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 7 Dec 2012 10:32:44 -0500 Subject: [PATCH] Move history annotation to History#perform/replace Actions are too low-level for annotations. --- js/id/actions/add_node.js | 2 +- js/id/actions/add_way.js | 2 +- js/id/actions/add_way_node.js | 2 +- js/id/actions/change_entity_tags.js | 2 +- js/id/actions/delete_node.js | 2 +- js/id/actions/delete_way.js | 2 +- js/id/actions/move.js | 2 +- js/id/actions/remove_relation_member.js | 2 +- js/id/actions/remove_way_node.js | 2 +- js/id/actions/reverse_way.js | 2 +- js/id/graph/graph.js | 16 +++---- js/id/graph/history.js | 53 +++++++++++---------- js/id/modes/add_area.js | 7 ++- js/id/modes/add_place.js | 6 ++- js/id/modes/add_road.js | 11 +++-- js/id/modes/drag_features.js | 31 +++++++------ js/id/modes/draw_area.js | 10 +++- js/id/modes/draw_road.js | 13 ++++-- js/id/modes/select.js | 22 ++++++--- test/spec/graph/graph.js | 5 -- test/spec/graph/history.js | 62 +++++++++++-------------- 21 files changed, 142 insertions(+), 114 deletions(-) diff --git a/js/id/actions/add_node.js b/js/id/actions/add_node.js index f8916e50b..669a81d49 100644 --- a/js/id/actions/add_node.js +++ b/js/id/actions/add_node.js @@ -1,6 +1,6 @@ // https://github.com/openstreetmap/josm/blob/mirror/src/org/openstreetmap/josm/command/AddCommand.java iD.actions.AddNode = function(node) { return function(graph) { - return graph.replace(node, 'added a place'); + return graph.replace(node); }; }; diff --git a/js/id/actions/add_way.js b/js/id/actions/add_way.js index 4b53a8ea3..2be062d3a 100644 --- a/js/id/actions/add_way.js +++ b/js/id/actions/add_way.js @@ -1,5 +1,5 @@ iD.actions.AddWay = function(way) { return function(graph) { - return graph.replace(way, 'started a road'); + return graph.replace(way); }; }; diff --git a/js/id/actions/add_way_node.js b/js/id/actions/add_way_node.js index f3c4521aa..69361b8dc 100644 --- a/js/id/actions/add_way_node.js +++ b/js/id/actions/add_way_node.js @@ -5,6 +5,6 @@ iD.actions.AddWayNode = function(wayId, nodeId, index) { node = graph.entity(nodeId), nodes = way.nodes.slice(); nodes.splice(index || nodes.length, 0, nodeId); - return graph.replace(way.update({nodes: nodes}), 'added to a road'); + return graph.replace(way.update({nodes: nodes})); }; }; diff --git a/js/id/actions/change_entity_tags.js b/js/id/actions/change_entity_tags.js index 25cf5911c..96901c2eb 100644 --- a/js/id/actions/change_entity_tags.js +++ b/js/id/actions/change_entity_tags.js @@ -1,6 +1,6 @@ iD.actions.ChangeEntityTags = function(entityId, tags) { return function(graph) { var entity = graph.entity(entityId); - return graph.replace(entity.update({tags: tags}), 'changed tags'); + return graph.replace(entity.update({tags: tags})); }; }; diff --git a/js/id/actions/delete_node.js b/js/id/actions/delete_node.js index fb858d75f..e8b6d34da 100644 --- a/js/id/actions/delete_node.js +++ b/js/id/actions/delete_node.js @@ -13,6 +13,6 @@ iD.actions.DeleteNode = function(nodeId) { graph = iD.actions.RemoveRelationMember(parent.id, nodeId)(graph); }); - return graph.remove(node, 'removed a node'); + return graph.remove(node); }; }; diff --git a/js/id/actions/delete_way.js b/js/id/actions/delete_way.js index a16309e56..120ac17f5 100644 --- a/js/id/actions/delete_way.js +++ b/js/id/actions/delete_way.js @@ -26,6 +26,6 @@ iD.actions.DeleteWay = function(wayId) { } }); - return graph.remove(way, 'removed a way'); + return graph.remove(way); }; }; diff --git a/js/id/actions/move.js b/js/id/actions/move.js index 42c82db56..21870c8d4 100644 --- a/js/id/actions/move.js +++ b/js/id/actions/move.js @@ -3,6 +3,6 @@ iD.actions.Move = function(entityId, loc) { return function(graph) { var entity = graph.entity(entityId); - return graph.replace(entity.update({loc: loc}), 'moved an element'); + return graph.replace(entity.update({loc: loc})); }; }; diff --git a/js/id/actions/remove_relation_member.js b/js/id/actions/remove_relation_member.js index f4061d177..f89bbe803 100644 --- a/js/id/actions/remove_relation_member.js +++ b/js/id/actions/remove_relation_member.js @@ -2,6 +2,6 @@ iD.actions.RemoveRelationMember = function(relationId, memberId) { return function(graph) { var relation = graph.entity(relationId), members = _.without(relation.members, memberId); - return graph.replace(relation.update({members: members}), 'removed from a relation'); + return graph.replace(relation.update({members: members})); }; }; diff --git a/js/id/actions/remove_way_node.js b/js/id/actions/remove_way_node.js index 073f7f42d..8c589a20c 100644 --- a/js/id/actions/remove_way_node.js +++ b/js/id/actions/remove_way_node.js @@ -12,6 +12,6 @@ iD.actions.RemoveWayNode = function(wayId, nodeId) { } else { nodes = _.without(way.nodes, nodeId); } - return graph.replace(way.update({nodes: nodes}), 'removed from a road'); + return graph.replace(way.update({nodes: nodes})); }; }; diff --git a/js/id/actions/reverse_way.js b/js/id/actions/reverse_way.js index 5b93f5578..facc75edf 100644 --- a/js/id/actions/reverse_way.js +++ b/js/id/actions/reverse_way.js @@ -3,6 +3,6 @@ iD.actions.ReverseWay = function(wayId) { return function(graph) { var way = graph.entity(wayId), nodes = way.nodes.slice().reverse(); - return graph.replace(way.update({nodes: nodes}), 'changed way direction'); + return graph.replace(way.update({nodes: nodes})); }; }; diff --git a/js/id/graph/graph.js b/js/id/graph/graph.js index fe878de2c..0626a492f 100644 --- a/js/id/graph/graph.js +++ b/js/id/graph/graph.js @@ -1,5 +1,5 @@ -iD.Graph = function(entities, annotation) { - if (!(this instanceof iD.Graph)) return new iD.Graph(entities, annotation); +iD.Graph = function(entities) { + if (!(this instanceof iD.Graph)) return new iD.Graph(entities); if (_.isArray(entities)) { this.entities = {}; @@ -10,8 +10,6 @@ iD.Graph = function(entities, annotation) { this.entities = entities || {}; } - this.annotation = annotation; - if (iD.debug) { Object.freeze(this); Object.freeze(this.entities); @@ -40,19 +38,19 @@ iD.Graph.prototype = { merge: function(graph) { var entities = _.clone(this.entities); _.defaults(entities, graph.entities); - return iD.Graph(entities, this.annotation); + return iD.Graph(entities); }, - replace: function(entity, annotation) { + replace: function(entity) { var entities = _.clone(this.entities); entities[entity.id] = entity; - return iD.Graph(entities, annotation); + return iD.Graph(entities); }, - remove: function(entity, annotation) { + remove: function(entity) { var entities = _.clone(this.entities); delete entities[entity.id]; - return iD.Graph(entities, annotation); + return iD.Graph(entities); }, // get all objects that intersect an extent. diff --git a/js/id/graph/history.js b/js/id/graph/history.js index df444840b..eec93f3e8 100644 --- a/js/id/graph/history.js +++ b/js/id/graph/history.js @@ -2,6 +2,23 @@ iD.History = function() { var stack, index, dispatch = d3.dispatch('change'); + function perform(actions) { + actions = Array.prototype.slice.call(actions) + + var annotation; + + if (_.isString(_.last(actions))) { + annotation = actions.pop(); + } + + var graph = stack[index].graph; + for (var i = 0; i < actions.length; i++) { + graph = actions[i](graph); + } + + return {graph: graph, annotation: annotation}; + } + function maybeChange() { if (stack[index].annotation) { dispatch.change(); @@ -10,38 +27,26 @@ iD.History = function() { var history = { graph: function () { - return stack[index]; + return stack[index].graph; }, merge: function (graph) { for (var i = 0; i < stack.length; i++) { - stack[i] = stack[i].merge(graph); + stack[i].graph = stack[i].graph.merge(graph); } }, perform: function () { stack = stack.slice(0, index + 1); - - var graph = this.graph(); - for (var i = 0; i < arguments.length; i++) { - graph = arguments[i](graph); - } - - stack.push(graph); + stack.push(perform(arguments)); index++; - maybeChange(); + dispatch.change(); }, replace: function () { // assert(index == stack.length - 1) - - var graph = this.graph(); - for (var i = 0; i < arguments.length; i++) { - graph = arguments[i](graph); - } - - stack[index] = graph; - maybeChange(); + stack[index] = perform(arguments); + dispatch.change(); }, undo: function () { @@ -78,19 +83,19 @@ iD.History = function() { // generate reports of changes for changesets to use modify: function () { - return stack[index].modifications(); + return stack[index].graph.modifications(); }, create: function () { - return stack[index].creations(); + return stack[index].graph.creations(); }, 'delete': function () { return _.difference( - _.pluck(stack[0].entities, 'id'), - _.pluck(stack[index].entities, 'id') + _.pluck(stack[0].graph.entities, 'id'), + _.pluck(stack[index].graph.entities, 'id') ).map(function (id) { - return stack[0].fetch(id); + return stack[0].graph.fetch(id); }); }, @@ -103,7 +108,7 @@ iD.History = function() { }, reset: function () { - stack = [iD.Graph()]; + stack = [{graph: iD.Graph()}]; index = 0; dispatch.change(); } diff --git a/js/id/modes/add_area.js b/js/id/modes/add_area.js index d9ecdbcce..cdee57082 100644 --- a/js/id/modes/add_area.js +++ b/js/id/modes/add_area.js @@ -22,14 +22,17 @@ iD.modes.AddArea = function() { // start from an existing node history.perform( iD.actions.AddWay(way), - iD.actions.AddWayNode(way.id, datum.id)); + iD.actions.AddWayNode(way.id, datum.id), + 'started an area'); + } else { // start from a new node var node = iD.Node({loc: map.mouseCoordinates()}); history.perform( iD.actions.AddWay(way), iD.actions.AddNode(node), - iD.actions.AddWayNode(way.id, node.id)); + iD.actions.AddWayNode(way.id, node.id), + 'started an area'); } controller.enter(iD.modes.DrawArea(way.id)); diff --git a/js/id/modes/add_place.js b/js/id/modes/add_place.js index 56e54959d..82b423368 100644 --- a/js/id/modes/add_place.js +++ b/js/id/modes/add_place.js @@ -14,7 +14,11 @@ iD.modes.AddPlace = function() { map.surface.on('click.addplace', function() { var node = iD.Node({loc: map.mouseCoordinates(), _poi: true}); - history.perform(iD.actions.AddNode(node)); + + history.perform( + iD.actions.AddNode(node), + 'added a place'); + controller.enter(iD.modes.Select(node)); }); diff --git a/js/id/modes/add_road.js b/js/id/modes/add_road.js index 879f930c8..5f05c3f77 100644 --- a/js/id/modes/add_road.js +++ b/js/id/modes/add_road.js @@ -31,8 +31,10 @@ iD.modes.AddRoad = function() { } else { history.perform( iD.actions.AddWay(way), - iD.actions.AddWayNode(way.id, datum.id)); + iD.actions.AddWayNode(way.id, datum.id), + 'started a road'); } + } else if (datum.type === 'way') { // begin a new way starting from an existing way var node = iD.Node({loc: map.mouseCoordinates()}), @@ -41,7 +43,9 @@ iD.modes.AddRoad = function() { history.perform( iD.actions.AddWay(way), iD.actions.AddWayNode(datum.id, node, index), - iD.actions.AddWayNode(way.id, node.id)); + iD.actions.AddWayNode(way.id, node.id), + 'started a road'); + } else { // begin a new way var node = iD.Node({loc: map.mouseCoordinates()}); @@ -49,7 +53,8 @@ iD.modes.AddRoad = function() { history.perform( iD.actions.AddWay(way), iD.actions.AddNode(node), - iD.actions.AddWayNode(way.id, node.id)); + iD.actions.AddWayNode(way.id, node.id), + 'started a road'); } controller.enter(iD.modes.DrawRoad(way.id, direction)); diff --git a/js/id/modes/drag_features.js b/js/id/modes/drag_features.js index 82fb7e59a..7a38883ed 100644 --- a/js/id/modes/drag_features.js +++ b/js/id/modes/drag_features.js @@ -1,5 +1,5 @@ iD.modes._dragFeatures = function(mode) { - var dragging, incarnated; + var dragging; var dragbehavior = d3.behavior.drag() .origin(function(entity) { @@ -8,26 +8,31 @@ iD.modes._dragFeatures = function(mode) { }) .on('drag', function(entity) { d3.event.sourceEvent.stopPropagation(); + + var loc = mode.map.projection.invert([d3.event.x, d3.event.y]); + if (!dragging) { if (entity.accuracy) { - var node = iD.Node({ loc: entity.loc }); + dragging = iD.Node({loc: loc}); mode.history.perform( - iD.actions.AddNode(node), - iD.actions.AddWayNode(entity.way, node.id, entity.index)); - incarnated = node.id; + iD.actions.AddNode(dragging), + iD.actions.AddWayNode(entity.way, dragging.id, entity.index)); + } else { + dragging = entity; + mode.history.perform( + iD.actions.Move(dragging.id, loc)); } - dragging = iD.util.trueObj([entity.id].concat( - _.pluck(mode.history.graph().parentWays(entity.id), 'id'))); - mode.history.perform(iD.actions.Noop()); } - if (incarnated) entity = mode.history.graph().entity(incarnated); - var to = mode.map.projection.invert([d3.event.x, d3.event.y]); - mode.history.replace(iD.actions.Move(entity.id, to)); + + mode.history.replace(iD.actions.Move(dragging.id, loc)); }) - .on('dragend', function () { + .on('dragend', function (entity) { if (!dragging) return; dragging = undefined; - incarnated = undefined; + + mode.history.replace( + iD.actions.Noop(), + entity.accuracy ? 'added a node to a way' : 'moved a node'); }); mode.map.surface diff --git a/js/id/modes/draw_area.js b/js/id/modes/draw_area.js index 2b952a897..c52000538 100644 --- a/js/id/modes/draw_area.js +++ b/js/id/modes/draw_area.js @@ -31,7 +31,8 @@ iD.modes.DrawArea = function(wayId) { if (datum.id === tailId) { history.replace( iD.actions.DeleteNode(node.id), - iD.actions.AddWayNode(way.id, tailId)); + iD.actions.AddWayNode(way.id, tailId), + 'added to an area'); controller.enter(iD.modes.Select(way)); @@ -39,11 +40,16 @@ iD.modes.DrawArea = function(wayId) { // connect the way to an existing node history.replace( iD.actions.DeleteNode(node.id), - iD.actions.AddWayNode(way.id, datum.id)); + iD.actions.AddWayNode(way.id, datum.id), + 'added to an area'); controller.enter(iD.modes.DrawArea(wayId)); } else { + history.replace( + iD.actions.Noop(), + 'added to an area'); + controller.enter(iD.modes.DrawArea(wayId)); } }); diff --git a/js/id/modes/draw_road.js b/js/id/modes/draw_road.js index 769c4412c..50f605660 100644 --- a/js/id/modes/draw_road.js +++ b/js/id/modes/draw_road.js @@ -34,7 +34,8 @@ iD.modes.DrawRoad = function(wayId, direction) { // connect the way in a loop history.replace( iD.actions.DeleteNode(node.id), - iD.actions.AddWayNode(wayId, tailId, index)); + iD.actions.AddWayNode(wayId, tailId, index), + 'added to a road'); controller.enter(iD.modes.Select(way)); @@ -42,7 +43,8 @@ iD.modes.DrawRoad = function(wayId, direction) { // connect the way to an existing node history.replace( iD.actions.DeleteNode(node.id), - iD.actions.AddWayNode(wayId, datum.id, index)); + iD.actions.AddWayNode(wayId, datum.id, index), + 'added to a road'); controller.enter(iD.modes.DrawRoad(wayId, direction)); @@ -51,11 +53,16 @@ iD.modes.DrawRoad = function(wayId, direction) { var connectedIndex = iD.modes.chooseIndex(datum, d3.mouse(map.surface.node()), map); history.replace( - iD.actions.AddWayNode(datum.id, node.id, connectedIndex)); + iD.actions.AddWayNode(datum.id, node.id, connectedIndex), + 'added to a road'); controller.enter(iD.modes.DrawRoad(wayId, direction)); } else { + history.replace( + iD.actions.Noop(), + 'added to a road'); + controller.enter(iD.modes.DrawRoad(wayId, direction)); } }); diff --git a/js/id/modes/select.js b/js/id/modes/select.js index 10e086eb4..2f917e77f 100644 --- a/js/id/modes/select.js +++ b/js/id/modes/select.js @@ -15,8 +15,7 @@ iD.modes.Select = function (entity) { d3.event.sourceEvent.stopPropagation(); if (!dragging) { - dragging = iD.util.trueObj([entity.id].concat( - _.pluck(mode.history.graph().parentWays(entity.id), 'id'))); + dragging = true; mode.history.perform(iD.actions.Noop()); } @@ -37,10 +36,14 @@ iD.modes.Select = function (entity) { function remove() { switch (entity.type) { case 'way': - mode.history.perform(iD.actions.DeleteWay(entity.id)); + mode.history.perform( + iD.actions.DeleteWay(entity.id), + 'deleted a way'); break; case 'node': - mode.history.perform(iD.actions.DeleteNode(entity.id)); + mode.history.perform( + iD.actions.DeleteNode(entity.id), + 'deleted a node'); } mode.controller.exit(); @@ -59,11 +62,18 @@ iD.modes.Select = function (entity) { .call(inspector); inspector.on('changeTags', function(d, tags) { - mode.history.perform(iD.actions.ChangeEntityTags(d.id, tags)); + mode.history.perform( + iD.actions.ChangeEntityTags(d.id, tags), + 'changed tags'); + }).on('changeWayDirection', function(d) { - mode.history.perform(iD.actions.ReverseWay(d.id)); + mode.history.perform( + iD.actions.ReverseWay(d.id), + 'reversed a way'); + }).on('remove', function() { remove(); + }).on('close', function() { mode.controller.exit(); }); diff --git a/test/spec/graph/graph.js b/test/spec/graph/graph.js index b0f71cd91..248056c0d 100644 --- a/test/spec/graph/graph.js +++ b/test/spec/graph/graph.js @@ -11,11 +11,6 @@ describe('iD.Graph', function() { expect(graph.entity(entity.id)).to.equal(entity); }); - it('can be constructed with an annotation', function() { - var graph = iD.Graph({}, 'first graph'); - expect(graph.annotation).to.equal('first graph'); - }); - if (iD.debug) { it("is frozen", function () { expect(Object.isFrozen(iD.Graph())).to.be.true; diff --git a/test/spec/graph/history.js b/test/spec/graph/history.js index 49f4fe397..efabcc32a 100644 --- a/test/spec/graph/history.js +++ b/test/spec/graph/history.js @@ -1,7 +1,6 @@ -describe("History", function () { +describe("iD.History", function () { var history, spy, - graph = iD.Graph([], "action"), - action = function() { return graph; }; + action = function() { return iD.Graph(); }; beforeEach(function () { history = iD.History(); @@ -16,13 +15,14 @@ describe("History", function () { describe("#perform", function () { it("updates the graph", function () { - history.perform(action); + var graph = iD.Graph(); + history.perform(d3.functor(graph)); expect(history.graph()).to.equal(graph); }); - it("pushes the undo stack", function () { - history.perform(action); - expect(history.undoAnnotation()).to.equal("action"); + it("pushes an undo annotation", function () { + history.perform(action, "annotation"); + expect(history.undoAnnotation()).to.equal("annotation"); }); it("emits a change event", function () { @@ -31,32 +31,27 @@ describe("History", function () { expect(spy).to.have.been.called; }); - it("does not emit a change event when performing a noop", function () { - history.on('change', spy); - history.perform(iD.actions.Noop); - expect(spy).not.to.have.been.called; - }); - it("performs multiple actions", function () { - var action1 = sinon.stub().returns(graph), - action2 = sinon.stub().returns(graph); - history.perform(action1, action2); + var action1 = sinon.stub().returns(iD.Graph()), + action2 = sinon.stub().returns(iD.Graph()); + history.perform(action1, action2, "annotation"); expect(action1).to.have.been.called; expect(action2).to.have.been.called; + expect(history.undoAnnotation()).to.equal("annotation"); }); }); describe("#replace", function () { it("updates the graph", function () { - history.replace(action); + var graph = iD.Graph(); + history.replace(d3.functor(graph)); expect(history.graph()).to.equal(graph); }); - it("replaces the undo stack", function () { - history.perform(action); - history.replace(action); - history.undo(); - expect(history.undoAnnotation()).to.be.undefined; + it("replaces the undo annotation", function () { + history.perform(action, "annotation1"); + history.replace(action, "annotation2"); + expect(history.undoAnnotation()).to.equal("annotation2"); }); it("emits a change event", function () { @@ -65,32 +60,27 @@ describe("History", function () { expect(spy).to.have.been.called; }); - it("does not emit a change event when performing a noop", function () { - history.on('change', spy); - history.replace(iD.actions.Noop); - expect(spy).not.to.have.been.called; - }); - it("performs multiple actions", function () { - var action1 = sinon.stub().returns(graph), - action2 = sinon.stub().returns(graph); - history.replace(action1, action2); + var action1 = sinon.stub().returns(iD.Graph()), + action2 = sinon.stub().returns(iD.Graph()); + history.replace(action1, action2, "annotation"); expect(action1).to.have.been.called; expect(action2).to.have.been.called; + expect(history.undoAnnotation()).to.equal("annotation"); }); }); describe("#undo", function () { it("pops the undo stack", function () { - history.perform(action); + history.perform(action, "annotation"); history.undo(); expect(history.undoAnnotation()).to.be.undefined; }); it("pushes the redo stack", function () { - history.perform(action); + history.perform(action, "annotation"); history.undo(); - expect(history.redoAnnotation()).to.equal("action"); + expect(history.redoAnnotation()).to.equal("annotation"); }); it("emits a change event", function () { @@ -113,8 +103,8 @@ describe("History", function () { describe("#reset", function () { it("clears the version stack", function () { - history.perform(action); - history.perform(action); + history.perform(action, "annotation"); + history.perform(action, "annotation"); history.undo(); history.reset(); expect(history.undoAnnotation()).to.be.undefined;