From ddc5e324f6cfa7fdcc25bfc679b0f25c70ad97b2 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 18:28:44 -0500 Subject: [PATCH 01/12] Extract iD.Difference iD.Difference represents the difference between two graphs. It knows how to calculate the set of entities that were created, modified, or deleted, and also contains the logic for recursively extending a difference to the complete set of entities that will require a redraw, taking into account child and parent relationships. Additionally, all history mutators now return a difference. --- index.html | 1 + js/id/graph/difference.js | 113 +++++++++++++++++++ js/id/graph/graph.js | 60 ---------- js/id/graph/history.js | 44 ++++---- js/id/renderer/map.js | 33 +----- test/index.html | 2 + test/index_packaged.html | 1 + test/spec/graph/difference.js | 202 ++++++++++++++++++++++++++++++++++ test/spec/graph/graph.js | 82 -------------- test/spec/graph/history.js | 41 +++++-- 10 files changed, 375 insertions(+), 204 deletions(-) create mode 100644 js/id/graph/difference.js create mode 100644 test/spec/graph/difference.js diff --git a/index.html b/index.html index d8be52dcd..f9b627c16 100644 --- a/index.html +++ b/index.html @@ -119,6 +119,7 @@ + diff --git a/js/id/graph/difference.js b/js/id/graph/difference.js new file mode 100644 index 000000000..8bea5664c --- /dev/null +++ b/js/id/graph/difference.js @@ -0,0 +1,113 @@ +/* + iD.Difference represents the difference between two graphs. + It knows how to calculate the set of entities that were + created, modified, or deleted, and also contains the logic + for recursively extending a difference to the complete set + of entities that will require a redraw, taking into account + child and parent relationships. + */ +iD.Difference = function (base, head) { + var changes = {}, length = 0; + + _.each(head.entities, function(h, id) { + var b = base.entities[id]; + if (h !== b) { + changes[id] = {base: b, head: h}; + length++; + } + }); + + _.each(base.entities, function(b, id) { + var h = head.entities[id]; + if (!changes[id] && h !== b) { + changes[id] = {base: b, head: h}; + length++; + } + }); + + var difference = {}; + + difference.length = function () { + return length; + }; + + difference.changes = function() { + return changes; + }; + + difference.modified = function() { + var result = []; + _.each(changes, function(change) { + if (change.base && change.head) result.push(change.head); + }); + return result; + }; + + difference.created = function() { + var result = []; + _.each(changes, function(change) { + if (!change.base && change.head) result.push(change.head); + }); + return result; + }; + + difference.deleted = function() { + var result = []; + _.each(changes, function(change) { + if (change.base && !change.head) result.push(change.base); + }); + return result; + }; + + difference.complete = function(extent) { + var result = {}, id, change; + + function addParents(parents) { + for (var i = 0; i < parents.length; i++) { + var parent = parents[i]; + + if (parent.id in result) + continue; + + result[parent.id] = parent; + addParents(head.parentRelations(parent)); + } + } + + for (id in changes) { + change = changes[id]; + + var h = change.head, + b = change.base, + entity = h || b; + + if (extent && !entity.intersects(extent, h ? head : base)) + continue; + + result[id] = h; + + if (entity.type === 'way') { + var nh = h ? h.nodes : [], + nb = b ? b.nodes : [], + diff; + + diff = _.difference(nh, nb); + for (var i = 0; i < diff.length; i++) { + result[diff[i]] = head.entity(diff[i]); + } + + diff = _.difference(nb, nh); + for (var i = 0; i < diff.length; i++) { + result[diff[i]] = head.entity(diff[i]); + } + } + + addParents(head.parentWays(entity)); + addParents(head.parentRelations(entity)); + } + + return result; + }; + + return difference; +}; diff --git a/js/id/graph/graph.js b/js/id/graph/graph.js index a19bfe094..2692aeba6 100644 --- a/js/id/graph/graph.js +++ b/js/id/graph/graph.js @@ -232,65 +232,5 @@ iD.Graph.prototype = { } } return items; - }, - - difference: function (graph) { - - function diff(a, b) { - var result = [], - keys = Object.keys(a.entities), - entity, oldentity, id, i; - - for (i = 0; i < keys.length; i++) { - id = keys[i]; - entity = a.entities[id]; - oldentity = b.entities[id]; - if (entity !== oldentity) { - - // maybe adding affected children better belongs in renderer/map.js? - if (entity && entity.type === 'way' && - oldentity && oldentity.type === 'way') { - result = result - .concat(_.difference(entity.nodes, oldentity.nodes)) - .concat(_.difference(oldentity.nodes, entity.nodes)); - - } else if (entity && entity.type === 'way') { - result = result.concat(entity.nodes); - - } else if (oldentity && oldentity.type === 'way') { - result = result.concat(oldentity.nodes); - } - - result.push(id); - } - } - return result; - } - - return _.unique(diff(this, graph).concat(diff(graph, this)).sort()); - }, - - modified: function() { - var result = [], base = this.base().entities; - _.each(this.entities, function(entity, id) { - if (entity && base[id]) result.push(id); - }); - return result; - }, - - created: function() { - var result = [], base = this.base().entities; - _.each(this.entities, function(entity, id) { - if (entity && !base[id]) result.push(id); - }); - return result; - }, - - deleted: function() { - var result = [], base = this.base().entities; - _.each(this.entities, function(entity, id) { - if (!entity && base[id]) result.push(id); - }); - return result; } }; diff --git a/js/id/graph/history.js b/js/id/graph/history.js index 42e1b1604..cbadbf87f 100644 --- a/js/id/graph/history.js +++ b/js/id/graph/history.js @@ -21,7 +21,9 @@ iD.History = function() { } function change(previous) { - dispatch.change(history.graph().difference(previous)); + var difference = iD.Difference(previous, history.graph()); + dispatch.change(difference); + return difference; } var history = { @@ -42,7 +44,7 @@ iD.History = function() { stack.push(perform(arguments)); index++; - change(previous); + return change(previous); }, replace: function () { @@ -51,7 +53,7 @@ iD.History = function() { // assert(index == stack.length - 1) stack[index] = perform(arguments); - change(previous); + return change(previous); }, pop: function () { @@ -60,7 +62,7 @@ iD.History = function() { if (index > 0) { index--; stack.pop(); - change(previous); + return change(previous); } }, @@ -80,7 +82,7 @@ iD.History = function() { } dispatch.undone(); - change(previous); + return change(previous); }, redo: function () { @@ -92,7 +94,7 @@ iD.History = function() { } dispatch.redone(); - change(previous); + return change(previous); }, undoAnnotation: function () { @@ -111,31 +113,27 @@ iD.History = function() { } }, - changes: function () { - var initial = stack[0].graph, - current = stack[index].graph; + difference: function () { + var base = stack[0].graph, + head = stack[index].graph; + return iD.Difference(base, head); + }, + changes: function () { + var difference = history.difference(); return { - modified: current.modified().map(function (id) { - return current.entity(id); - }), - created: current.created().map(function (id) { - return current.entity(id); - }), - deleted: current.deleted().map(function (id) { - return initial.entity(id); - }) - }; + modified: difference.modified(), + created: difference.created(), + deleted: difference.deleted() + } }, hasChanges: function() { - return !!this.numChanges(); + return this.difference().length() > 0; }, numChanges: function() { - return d3.sum(d3.values(this.changes()).map(function(c) { - return c.length; - })); + return this.difference().length(); }, imagery_used: function(source) { diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index 1f22a7c81..839aa18a3 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -64,44 +64,19 @@ iD.Map = function(context) { extent = map.extent(), graph = context.graph(); - function addParents(parents) { - for (var i = 0; i < parents.length; i++) { - var parent = parents[i]; - if (only[parent.id] === undefined) { - only[parent.id] = parent; - addParents(graph.parentRelations(parent)); - } - } - } - if (!difference) { all = graph.intersects(extent); filter = d3.functor(true); } else { - var only = {}; - - for (var j = 0; j < difference.length; j++) { - var id = difference[j], - entity = graph.entity(id); - - // Even if the entity is false (deleted), it needs to be - // removed from the surface - only[id] = entity; - - if (entity && entity.intersects(extent, graph)) { - addParents(graph.parentWays(only[id])); - addParents(graph.parentRelations(only[id])); - } - } - - all = _.compact(_.values(only)); + var complete = difference.complete(extent); + all = _.compact(_.values(complete)); filter = function(d) { if (d.type === 'midpoint') { for (var i = 0; i < d.ways.length; i++) { - if (d.ways[i].id in only) return true; + if (d.ways[i].id in complete) return true; } } else { - return d.id in only; + return d.id in complete; } }; } diff --git a/test/index.html b/test/index.html index da3cd475c..b6d925923 100644 --- a/test/index.html +++ b/test/index.html @@ -115,6 +115,7 @@ + @@ -164,6 +165,7 @@ + diff --git a/test/index_packaged.html b/test/index_packaged.html index 948ed0ae1..ef91e2823 100644 --- a/test/index_packaged.html +++ b/test/index_packaged.html @@ -55,6 +55,7 @@ + diff --git a/test/spec/graph/difference.js b/test/spec/graph/difference.js new file mode 100644 index 000000000..f12a77254 --- /dev/null +++ b/test/spec/graph/difference.js @@ -0,0 +1,202 @@ +describe("iD.Difference", function () { + describe("#changes", function () { + it("includes created entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph(), + head = base.replace(node), + diff = iD.Difference(base, head); + expect(diff.changes()).to.eql({n: {base: undefined, head: node}}); + }); + + it("includes undone created entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph(), + head = base.replace(node), + diff = iD.Difference(head, base); + expect(diff.changes()).to.eql({n: {base: node, head: undefined}}); + }); + + it("includes modified entities", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.update(), + base = iD.Graph([n1]), + head = base.replace(n2), + diff = iD.Difference(base, head); + expect(diff.changes()).to.eql({n: {base: n1, head: n2}}); + }); + + it("includes undone modified entities", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.update(), + base = iD.Graph([n1]), + head = base.replace(n2), + diff = iD.Difference(head, base); + expect(diff.changes()).to.eql({n: {base: n2, head: n1}}); + }); + + it("includes deleted entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph([node]), + head = base.remove(node), + diff = iD.Difference(base, head); + expect(diff.changes()).to.eql({n: {base: node, head: undefined}}); + }); + + it("includes undone deleted entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph([node]), + head = base.remove(node), + diff = iD.Difference(head, base); + expect(diff.changes()).to.eql({n: {base: undefined, head: node}}); + }); + + it("doesn't include created entities that were subsequently deleted", function () { + var node = iD.Node(), + base = iD.Graph(), + head = base.replace(node).remove(node), + diff = iD.Difference(base, head); + expect(diff.changes()).to.eql({}); + }); + }); + + describe("#created", function () { + it("returns an array of created entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph(), + head = base.replace(node), + diff = iD.Difference(base, head); + expect(diff.created()).to.eql([node]); + }); + }); + + describe("#modified", function () { + it("returns an array of modified entities", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.move([1, 2]), + base = iD.Graph([n1]), + head = base.replace(n2), + diff = iD.Difference(base, head); + expect(diff.modified()).to.eql([n2]); + }); + }); + + describe("#deleted", function () { + it("returns an array of deleted entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph([node]), + head = base.remove(node), + diff = iD.Difference(base, head); + expect(diff.deleted()).to.eql([node]); + }); + }); + + describe("#complete", function () { + it("includes created entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph(), + head = base.replace(node), + diff = iD.Difference(base, head); + expect(diff.complete()['n']).to.equal(node); + }); + + it("includes modified entities", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.move([1, 2]), + base = iD.Graph([n1]), + head = base.replace(n2), + diff = iD.Difference(base, head); + expect(diff.complete()['n']).to.equal(n2); + }); + + it("includes deleted entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph([node]), + head = base.remove(node), + diff = iD.Difference(base, head); + expect(diff.complete()).to.eql({n: undefined}); + }); + + it("includes nodes added to a way", function () { + var n1 = iD.Node({id: 'n1'}), + n2 = iD.Node({id: 'n2'}), + w1 = iD.Way({id: 'w', nodes: ['n1']}), + w2 = w1.addNode('n2'), + base = iD.Graph([n1, n2, w1]), + head = base.replace(w2), + diff = iD.Difference(base, head); + + expect(diff.complete()['n2']).to.equal(n2); + }); + + it("includes nodes removed from a way", function () { + var n1 = iD.Node({id: 'n1'}), + n2 = iD.Node({id: 'n2'}), + w1 = iD.Way({id: 'w', nodes: ['n1', 'n2']}), + w2 = w1.removeNode('n2'), + base = iD.Graph([n1, n2, w1]), + head = base.replace(w2), + diff = iD.Difference(base, head); + + expect(diff.complete()['n2']).to.equal(n2); + }); + + it("includes parent ways of modified nodes", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.move([1, 2]), + way = iD.Way({id: 'w', nodes: ['n']}), + base = iD.Graph([n1, way]), + head = base.replace(n2), + diff = iD.Difference(base, head); + + expect(diff.complete()['w']).to.equal(way); + }); + + it("includes parent relations of modified entities", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.move([1, 2]), + rel = iD.Relation({id: 'r', members: [{id: 'n'}]}), + base = iD.Graph([n1, rel]), + head = base.replace(n2), + diff = iD.Difference(base, head); + + expect(diff.complete()['r']).to.equal(rel); + }); + + it("includes parent relations of modified entities, recursively", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.move([1, 2]), + rel1 = iD.Relation({id: 'r1', members: [{id: 'n'}]}), + rel2 = iD.Relation({id: 'r2', members: [{id: 'r1'}]}), + base = iD.Graph([n1, rel1, rel2]), + head = base.replace(n2), + diff = iD.Difference(base, head); + + expect(diff.complete()['r2']).to.equal(rel2); + }); + + it("includes parent relations of parent ways of modified nodes", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.move([1, 2]), + way = iD.Way({id: 'w', nodes: ['n']}), + rel = iD.Relation({id: 'r', members: [{id: 'w'}]}), + base = iD.Graph([n1, way, rel]), + head = base.replace(n2), + diff = iD.Difference(base, head); + + expect(diff.complete()['r']).to.equal(rel); + }); + + it("copes with recursive relations", function () { + var node = iD.Node({id: 'n'}), + rel1 = iD.Relation({id: 'r1', members: [{id: 'n'}, {id: 'r2'}]}), + rel2 = iD.Relation({id: 'r2', members: [{id: 'r1'}]}), + base = iD.Graph([node, rel1, rel2]), + head = base.replace(node.move([1, 2])), + diff = iD.Difference(base, head); + + expect(diff.complete()).to.be.ok; + }); + + it("limits changes to those within a given extent"); + }); +}); diff --git a/test/spec/graph/graph.js b/test/spec/graph/graph.js index 05bcbbe89..a357ff360 100644 --- a/test/spec/graph/graph.js +++ b/test/spec/graph/graph.js @@ -333,86 +333,4 @@ describe('iD.Graph', function() { expect(graph.childNodes(way)).to.eql([node]); }); }); - - describe("#difference", function () { - it("returns an Array of ids of changed entities", function () { - var initial = iD.Node({id: "n1"}), - updated = initial.update({}), - created = iD.Node(), - deleted = iD.Node({id: 'n2'}), - graph1 = iD.Graph([initial, deleted]), - graph2 = graph1.replace(updated).replace(created).remove(deleted); - expect(graph2.difference(graph1)).to.eql([created.id, updated.id, deleted.id]); - }); - - - it("includes created entities, and reverse", function () { - var node = iD.Node(), - graph1 = iD.Graph(), - graph2 = graph1.replace(node); - expect(graph2.difference(graph1)).to.eql([node.id]); - expect(graph1.difference(graph2)).to.eql([node.id]); - }); - - it("includes entities changed from base, and reverse", function () { - var node = iD.Node(), - graph1 = iD.Graph(node), - graph2 = graph1.replace(node.update()); - expect(graph2.difference(graph1)).to.eql([node.id]); - expect(graph1.difference(graph2)).to.eql([node.id]); - }); - - it("includes already changed entities that were updated, and reverse", function () { - var node = iD.Node(), - graph1 = iD.Graph().replace(node), - graph2 = graph1.replace(node.update()); - expect(graph2.difference(graph1)).to.eql([node.id]); - expect(graph1.difference(graph2)).to.eql([node.id]); - }); - - it("includes affected child nodes", function () { - var n = iD.Node({id: 'n'}), - n2 = iD.Node({id: 'n2'}), - w1 = iD.Way({id: 'w1', nodes: ['n']}), - w1_ = iD.Way({id: 'w1', nodes: ['n', 'n2']}), - graph1 = iD.Graph([n, n2, w1]), - graph2 = graph1.replace(w1_); - expect(graph2.difference(graph1)).to.eql(['n2', 'w1']); - expect(graph1.difference(graph2)).to.eql(['n2', 'w1']); - }); - - }); - - describe("#modified", function () { - it("returns an Array of ids of modified entities", function () { - var node = iD.Node({id: 'n1'}), - node_ = iD.Node({id: 'n1'}), - graph = iD.Graph([node]).replace(node_); - expect(graph.modified()).to.eql([node.id]); - }); - }); - - describe("#created", function () { - it("returns an Array of ids of created entities", function () { - var node1 = iD.Node({id: 'n-1'}), - node2 = iD.Node({id: 'n2'}), - graph = iD.Graph([node2]).replace(node1); - expect(graph.created()).to.eql([node1.id]); - }); - }); - - describe("#deleted", function () { - it("returns an Array of ids of deleted entities", function () { - var node1 = iD.Node({id: "n1"}), - node2 = iD.Node(), - graph = iD.Graph([node1, node2]).remove(node1); - expect(graph.deleted()).to.eql([node1.id]); - }); - - it("doesn't include created entities that were subsequently deleted", function () { - var node = iD.Node(), - graph = iD.Graph().replace(node).remove(node); - expect(graph.deleted()).to.eql([]); - }); - }); }); diff --git a/test/spec/graph/history.js b/test/spec/graph/history.js index dc685b446..3f28f437e 100644 --- a/test/spec/graph/history.js +++ b/test/spec/graph/history.js @@ -14,6 +14,10 @@ describe("iD.History", function () { }); describe("#perform", function () { + it("returns a difference", function () { + expect(history.perform(action).changes()).to.eql({}); + }); + it("updates the graph", function () { var node = iD.Node(); history.perform(function (graph) { return graph.replace(node); }); @@ -27,8 +31,8 @@ describe("iD.History", function () { it("emits a change event", function () { history.on('change', spy); - history.perform(action); - expect(spy).to.have.been.calledWith([]); + var difference = history.perform(action); + expect(spy).to.have.been.calledWith(difference); }); it("performs multiple actions", function () { @@ -42,6 +46,10 @@ describe("iD.History", function () { }); describe("#replace", function () { + it("returns a difference", function () { + expect(history.replace(action).changes()).to.eql({}); + }); + it("updates the graph", function () { var node = iD.Node(); history.replace(function (graph) { return graph.replace(node); }); @@ -56,8 +64,8 @@ describe("iD.History", function () { it("emits a change event", function () { history.on('change', spy); - history.replace(action); - expect(spy).to.have.been.calledWith([]); + var difference = history.replace(action); + expect(spy).to.have.been.calledWith(difference); }); it("performs multiple actions", function () { @@ -71,6 +79,11 @@ describe("iD.History", function () { }); describe("#pop", function () { + it("returns a difference", function () { + history.perform(action, "annotation"); + expect(history.pop().changes()).to.eql({}); + }); + it("updates the graph", function () { history.perform(action, "annotation"); history.pop(); @@ -86,12 +99,16 @@ describe("iD.History", function () { it("emits a change event", function () { history.perform(action); history.on('change', spy); - history.pop(); - expect(spy).to.have.been.calledWith([]); + var difference = history.pop(); + expect(spy).to.have.been.calledWith(difference); }); }); describe("#undo", function () { + it("returns a difference", function () { + expect(history.undo().changes()).to.eql({}); + }); + it("pops the undo stack", function () { history.perform(action, "annotation"); history.undo(); @@ -121,12 +138,16 @@ describe("iD.History", function () { it("emits a change event", function () { history.perform(action); history.on('change', spy); - history.undo(); - expect(spy).to.have.been.calledWith([]); + var difference = history.undo(); + expect(spy).to.have.been.calledWith(difference); }); }); describe("#redo", function () { + it("returns a difference", function () { + expect(history.redo().changes()).to.eql({}); + }); + it("emits an redone event", function () { history.perform(action); history.undo(); @@ -139,8 +160,8 @@ describe("iD.History", function () { history.perform(action); history.undo(); history.on('change', spy); - history.redo(); - expect(spy).to.have.been.calledWith([]); + var difference = history.redo(); + expect(spy).to.have.been.calledWith(difference); }); }); From 11d723819dc222e946074b70a5980c59c9890c1d Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 19:13:53 -0500 Subject: [PATCH 02/12] Difference#extantIDs --- js/id/graph/difference.js | 8 ++++++++ test/spec/graph/difference.js | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/js/id/graph/difference.js b/js/id/graph/difference.js index 8bea5664c..e2159271a 100644 --- a/js/id/graph/difference.js +++ b/js/id/graph/difference.js @@ -35,6 +35,14 @@ iD.Difference = function (base, head) { return changes; }; + difference.extantIDs = function() { + var result = []; + _.each(changes, function(change, id) { + if (change.head) result.push(id); + }); + return result; + }; + difference.modified = function() { var result = []; _.each(changes, function(change) { diff --git a/test/spec/graph/difference.js b/test/spec/graph/difference.js index f12a77254..c9647bbea 100644 --- a/test/spec/graph/difference.js +++ b/test/spec/graph/difference.js @@ -59,6 +59,33 @@ describe("iD.Difference", function () { }); }); + describe("#extantIDs", function () { + it("includes the ids of created entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph(), + head = base.replace(node), + diff = iD.Difference(base, head); + expect(diff.extantIDs()).to.eql(['n']); + }); + + it("includes the ids of modified entities", function () { + var n1 = iD.Node({id: 'n'}), + n2 = n1.move([1, 2]), + base = iD.Graph([n1]), + head = base.replace(n2), + diff = iD.Difference(base, head); + expect(diff.extantIDs()).to.eql(['n']); + }); + + it("omits the ids of deleted entities", function () { + var node = iD.Node({id: 'n'}), + base = iD.Graph([node]), + head = base.remove(node), + diff = iD.Difference(base, head); + expect(diff.extantIDs()).to.eql([]); + }); + }); + describe("#created", function () { it("returns an array of created entities", function () { var node = iD.Node({id: 'n'}), From 1de05b518dcec864ca39429a3e0a394b31958763 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 19:15:28 -0500 Subject: [PATCH 03/12] Select result when splitting or joining ways Fixes #601. Fixes #603. --- js/id/operations/merge.js | 6 +++--- js/id/operations/split.js | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/js/id/operations/merge.js b/js/id/operations/merge.js index 08517e9b2..d02222368 100644 --- a/js/id/operations/merge.js +++ b/js/id/operations/merge.js @@ -2,9 +2,9 @@ iD.operations.Merge = function(selection, context) { var action = iD.actions.Join(selection[0], selection[1]); var operation = function() { - context.perform( - action, - t('operations.merge.annotation', {n: selection.length})); + var annotation = t('operations.merge.annotation', {n: selection.length}), + difference = context.perform(action, annotation); + context.enter(iD.modes.Select(context, difference.extantIDs())); }; operation.available = function() { diff --git a/js/id/operations/split.js b/js/id/operations/split.js index 07d7d56ff..f49f4b9ed 100644 --- a/js/id/operations/split.js +++ b/js/id/operations/split.js @@ -3,7 +3,9 @@ iD.operations.Split = function(selection, context) { action = iD.actions.Split(entityId); var operation = function() { - context.perform(action, t('operations.split.annotation')); + var annotation = t('operations.split.annotation'), + difference = context.perform(action, annotation); + context.enter(iD.modes.Select(context, difference.extantIDs())); }; operation.available = function() { From fc00f154a913788b5d16fed1a917a4c3fce80491 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 19:44:47 -0500 Subject: [PATCH 04/12] Dispatch a change event on merge --- js/id/graph/history.js | 2 ++ js/id/renderer/map.js | 1 - test/spec/graph/history.js | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/js/id/graph/history.js b/js/id/graph/history.js index cbadbf87f..3cc1ace01 100644 --- a/js/id/graph/history.js +++ b/js/id/graph/history.js @@ -35,6 +35,8 @@ iD.History = function() { for (var i = 0; i < stack.length; i++) { stack[i].graph.rebase(entities); } + + dispatch.change(); }, perform: function () { diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index 839aa18a3..c3b528da3 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -102,7 +102,6 @@ iD.Map = function(context) { function connectionLoad(err, result) { context.history().merge(result); - redraw(Object.keys(result)); } function zoomPan() { diff --git a/test/spec/graph/history.js b/test/spec/graph/history.js index 3f28f437e..0ecbed5d3 100644 --- a/test/spec/graph/history.js +++ b/test/spec/graph/history.js @@ -13,6 +13,20 @@ describe("iD.History", function () { }); }); + describe("#merge", function () { + it("merges the entities into all graph versions", function () { + var n = iD.Node({id: 'n'}); + history.merge({n: n}); + expect(history.graph().entity('n')).to.equal(n); + }); + + it("emits a change event", function () { + history.on('change', spy); + history.merge({}); + expect(spy).to.have.been.called; + }); + }); + describe("#perform", function () { it("returns a difference", function () { expect(history.perform(action).changes()).to.eql({}); From ec602a7db7ef139baa1614bcb5c9c8cb0ec43f54 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 19:46:59 -0500 Subject: [PATCH 05/12] Hook up connection and history in context --- js/id/id.js | 4 ++++ js/id/renderer/map.js | 7 ------- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/js/id/id.js b/js/id/id.js index 358610525..56d3e481a 100644 --- a/js/id/id.js +++ b/js/id/id.js @@ -16,6 +16,10 @@ window.iD = function () { // the connection requires .storage() to be available on calling. var connection = iD.Connection(context); + connection.on('load.context', function (err, result) { + history.merge(result); + }); + /* Straight accessors. Avoid using these if you can. */ context.ui = function() { return ui; }; context.connection = function() { return connection; }; diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index c3b528da3..b2561f21f 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -26,9 +26,6 @@ iD.Map = function(context) { surface, tilegroup; function map(selection) { - context.connection() - .on('load.tile', connectionLoad); - context.history() .on('change.map', redraw); @@ -100,10 +97,6 @@ iD.Map = function(context) { surface.selectAll('.layer *').remove(); } - function connectionLoad(err, result) { - context.history().merge(result); - } - function zoomPan() { if (d3.event && d3.event.sourceEvent.type === 'dblclick') { if (!dblclickEnabled) { From 80a5a083b0affb576065facd7260073835ff4d0d Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 19:55:52 -0500 Subject: [PATCH 06/12] Remove unused --- js/id/graph/entity.js | 11 +--------- test/spec/graph/entity.js | 42 ------------------------------------- test/spec/graph/node.js | 9 -------- test/spec/graph/relation.js | 9 -------- test/spec/graph/way.js | 9 -------- 5 files changed, 1 insertion(+), 79 deletions(-) diff --git a/js/id/graph/entity.js b/js/id/graph/entity.js index ae612c268..bce9675ee 100644 --- a/js/id/graph/entity.js +++ b/js/id/graph/entity.js @@ -43,7 +43,6 @@ iD.Entity.prototype = { if (!this.id && this.type) { this.id = iD.Entity.id(this.type); - this._updated = true; } if (iD.debug) { @@ -63,7 +62,7 @@ iD.Entity.prototype = { }, update: function(attrs) { - return iD.Entity(this, attrs, {_updated: true}); + return iD.Entity(this, attrs); }, mergeTags: function(tags) { @@ -80,14 +79,6 @@ iD.Entity.prototype = { return this.update({tags: merged}); }, - created: function() { - return this._updated && this.osmId().charAt(0) === '-'; - }, - - modified: function() { - return this._updated && this.osmId().charAt(0) !== '-'; - }, - intersects: function(extent, resolver) { return this.extent(resolver).intersects(extent); }, diff --git a/test/spec/graph/entity.js b/test/spec/graph/entity.js index 088ab5516..f2ac85f17 100644 --- a/test/spec/graph/entity.js +++ b/test/spec/graph/entity.js @@ -52,12 +52,6 @@ describe('iD.Entity', function () { expect(e.id).to.equal('w1'); }); - it("tags the entity as updated", function () { - var tags = {foo: 'bar'}, - e = iD.Entity().update({tags: tags}); - expect(e._updated).to.to.be.true; - }); - it("doesn't modify the input", function () { var attrs = {tags: {foo: 'bar'}}, e = iD.Entity().update(attrs); @@ -104,42 +98,6 @@ describe('iD.Entity', function () { }); }); - describe("#created", function () { - it("returns falsy by default", function () { - expect(iD.Entity({id: 'w1234'}).created()).not.to.be.ok; - }); - - it("returns falsy for an unmodified Entity", function () { - expect(iD.Entity({id: 'w1234'}).created()).not.to.be.ok; - }); - - it("returns falsy for a modified Entity with positive ID", function () { - expect(iD.Entity({id: 'w1234'}).update({}).created()).not.to.be.ok; - }); - - it("returns truthy for a modified Entity with negative ID", function () { - expect(iD.Entity({id: 'w-1234'}).update({}).created()).to.be.ok; - }); - }); - - describe("#modified", function () { - it("returns falsy by default", function () { - expect(iD.Entity({id: 'w1234'}).modified()).not.to.be.ok; - }); - - it("returns falsy for an unmodified Entity", function () { - expect(iD.Entity({id: 'w1234'}).modified()).not.to.be.ok; - }); - - it("returns truthy for a modified Entity with positive ID", function () { - expect(iD.Entity({id: 'w1234'}).update({}).modified()).to.be.ok; - }); - - it("returns falsy for a modified Entity with negative ID", function () { - expect(iD.Entity({id: 'w-1234'}).update({}).modified()).not.to.be.ok; - }); - }); - describe("#intersects", function () { it("returns true for a way with a node within the given extent", function () { var node = iD.Node({loc: [0, 0]}), diff --git a/test/spec/graph/node.js b/test/spec/graph/node.js index ad149261e..71a2c5e95 100644 --- a/test/spec/graph/node.js +++ b/test/spec/graph/node.js @@ -4,15 +4,6 @@ describe('iD.Node', function () { expect(iD.Node().type).to.equal("node"); }); - it("returns a created Entity if no ID is specified", function () { - expect(iD.Node().created()).to.be.ok; - }); - - it("returns an unmodified Entity if ID is specified", function () { - expect(iD.Node({id: 'n1234'}).created()).not.to.be.ok; - expect(iD.Node({id: 'n1234'}).modified()).not.to.be.ok; - }); - it("defaults tags to an empty object", function () { expect(iD.Node().tags).to.eql({}); }); diff --git a/test/spec/graph/relation.js b/test/spec/graph/relation.js index 8edf9c1e7..6e00dcacb 100644 --- a/test/spec/graph/relation.js +++ b/test/spec/graph/relation.js @@ -10,15 +10,6 @@ describe('iD.Relation', function () { expect(iD.Relation().type).to.equal("relation"); }); - it("returns a created Entity if no ID is specified", function () { - expect(iD.Relation().created()).to.be.ok; - }); - - it("returns an unmodified Entity if ID is specified", function () { - expect(iD.Relation({id: 'r1234'}).created()).not.to.be.ok; - expect(iD.Relation({id: 'r1234'}).modified()).not.to.be.ok; - }); - it("defaults members to an empty array", function () { expect(iD.Relation().members).to.eql([]); }); diff --git a/test/spec/graph/way.js b/test/spec/graph/way.js index ec41edec6..6b6b1543f 100644 --- a/test/spec/graph/way.js +++ b/test/spec/graph/way.js @@ -10,15 +10,6 @@ describe('iD.Way', function() { expect(iD.Way().type).to.equal("way"); }); - it("returns a created Entity if no ID is specified", function () { - expect(iD.Way().created()).to.be.ok; - }); - - it("returns an unmodified Entity if ID is specified", function () { - expect(iD.Way({id: 'w1234'}).created()).not.to.be.ok; - expect(iD.Way({id: 'w1234'}).modified()).not.to.be.ok; - }); - it("defaults nodes to an empty array", function () { expect(iD.Way().nodes).to.eql([]); }); From b08722fd3fb3cbfdf19af48926d4b3185cbbcdf2 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 19:59:05 -0500 Subject: [PATCH 07/12] Pass just the projection to Circularize --- js/id/actions/circularize.js | 6 +++--- js/id/operations/circularize.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/js/id/actions/circularize.js b/js/id/actions/circularize.js index edbd0d8b3..d844134eb 100644 --- a/js/id/actions/circularize.js +++ b/js/id/actions/circularize.js @@ -1,4 +1,4 @@ -iD.actions.Circularize = function(wayId, map) { +iD.actions.Circularize = function(wayId, projection) { var action = function(graph) { var way = graph.entity(wayId), @@ -6,7 +6,7 @@ iD.actions.Circularize = function(wayId, map) { tags = {}, key, role; var points = nodes.map(function(n) { - return map.projection(n.loc); + return projection(n.loc); }), centroid = d3.geom.polygon(points).centroid(), radius = d3.median(points, function(p) { @@ -15,7 +15,7 @@ iD.actions.Circularize = function(wayId, map) { circular_nodes = []; for (var i = 0; i < 12; i++) { - circular_nodes.push(iD.Node({ loc: map.projection.invert([ + circular_nodes.push(iD.Node({ loc: projection.invert([ centroid[0] + Math.cos((i / 12) * Math.PI * 2) * radius, centroid[1] + Math.sin((i / 12) * Math.PI * 2) * radius]) })); diff --git a/js/id/operations/circularize.js b/js/id/operations/circularize.js index 419a6e9eb..2680da4ec 100644 --- a/js/id/operations/circularize.js +++ b/js/id/operations/circularize.js @@ -1,6 +1,6 @@ iD.operations.Circularize = function(selection, context) { var entityId = selection[0], - action = iD.actions.Circularize(entityId, context.map()); + action = iD.actions.Circularize(entityId, context.projection); var operation = function() { var annotation = t('operations.circularize.annotation.' + context.geometry(entityId)); From 9e878b1cf927817c4cf7e619bcf5722278bd72e8 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 20:00:05 -0500 Subject: [PATCH 08/12] Remove redundant conditional --- js/id/validate.js | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/js/id/validate.js b/js/id/validate.js index aa7d8771b..6329b626f 100644 --- a/js/id/validate.js +++ b/js/id/validate.js @@ -15,31 +15,29 @@ iD.validate = function(changes, graph) { if (tags.building && tags.building === 'yes') return 'building=yes'; } - if (changes.created.length) { - for (var i = 0; i < changes.created.length; i++) { - change = changes.created[i]; + for (var i = 0; i < changes.created.length; i++) { + change = changes.created[i]; - if (change.geometry(graph) === 'point' && _.isEmpty(change.tags)) { - warnings.push({ - message: t('validations.untagged_point'), - entity: change - }); - } + if (change.geometry(graph) === 'point' && _.isEmpty(change.tags)) { + warnings.push({ + message: t('validations.untagged_point'), + entity: change + }); + } - if (change.geometry(graph) === 'line' && _.isEmpty(change.tags)) { - warnings.push({ message: t('validations.untagged_line'), entity: change }); - } + if (change.geometry(graph) === 'line' && _.isEmpty(change.tags)) { + warnings.push({ message: t('validations.untagged_line'), entity: change }); + } - if (change.geometry(graph) === 'area' && _.isEmpty(change.tags)) { - warnings.push({ message: t('validations.untagged_area'), entity: change }); - } + if (change.geometry(graph) === 'area' && _.isEmpty(change.tags)) { + warnings.push({ message: t('validations.untagged_area'), entity: change }); + } - if (change.geometry(graph) === 'line' && tagSuggestsArea(change)) { - warnings.push({ - message: t('validations.tag_suggests_area', {tag: tagSuggestsArea(change)}), - entity: change - }); - } + if (change.geometry(graph) === 'line' && tagSuggestsArea(change)) { + warnings.push({ + message: t('validations.tag_suggests_area', {tag: tagSuggestsArea(change)}), + entity: change + }); } } From 0b3e0fb3db7556438da8d560d9fab7dffd829e7f Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 20:29:01 -0500 Subject: [PATCH 09/12] Use iD.actions.DeleteNode when removing nodes They need to be removed from any parent relations. Also, make sure to uniq child nodes, otherwise the start/end node over-contributes to the centroid calculation. This action needs tests. --- js/id/actions/circularize.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/js/id/actions/circularize.js b/js/id/actions/circularize.js index d844134eb..2c72047d9 100644 --- a/js/id/actions/circularize.js +++ b/js/id/actions/circularize.js @@ -2,7 +2,7 @@ iD.actions.Circularize = function(wayId, projection) { var action = function(graph) { var way = graph.entity(wayId), - nodes = graph.childNodes(way), + nodes = _.uniq(graph.childNodes(way)), tags = {}, key, role; var points = nodes.map(function(n) { @@ -36,8 +36,6 @@ iD.actions.Circularize = function(wayId, projection) { circular_nodes.splice(closest, 1, nodes[i]); if (closest === 0) circular_nodes.splice(circular_nodes.length - 1, 1, nodes[i]); else if (closest === circular_nodes.length - 1) circular_nodes.splice(0, 1, nodes[i]); - } else { - graph = graph.remove(nodes[i]); } } @@ -45,9 +43,16 @@ iD.actions.Circularize = function(wayId, projection) { graph = graph.replace(circular_nodes[i]); } - return graph.replace(way.update({ - nodes: _.pluck(circular_nodes, 'id') - })); + var ids = _.pluck(circular_nodes, 'id'), + difference = _.difference(_.uniq(way.nodes), ids); + + graph = graph.replace(way.update({nodes: ids})); + + for (i = 0; i < difference.length; i++) { + graph = iD.actions.DeleteNode(difference[i])(graph); + } + + return graph; }; action.enabled = function(graph) { From 33f5db4d96b317be8c24d1b2db52405d2063d584 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 21:17:43 -0500 Subject: [PATCH 10/12] Fix hash with selected id --- js/id/behavior/hash.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/id/behavior/hash.js b/js/id/behavior/hash.js index 9056db393..c8762fb13 100644 --- a/js/id/behavior/hash.js +++ b/js/id/behavior/hash.js @@ -45,7 +45,7 @@ iD.behavior.Hash = function(context) { context.map().on('drawn.hash', function() { if (!context.entity(id)) return; selectoff(); - context.enter(iD.modes.Select([id])); + context.enter(iD.modes.Select(context, [id])); }); context.on('enter.hash', function() { From 441be74539c1b78d95b143109add18ab48504fba Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 Feb 2013 21:26:29 -0500 Subject: [PATCH 11/12] Simplify --- js/id/actions/circularize.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/js/id/actions/circularize.js b/js/id/actions/circularize.js index 2c72047d9..532dcdb45 100644 --- a/js/id/actions/circularize.js +++ b/js/id/actions/circularize.js @@ -2,8 +2,7 @@ iD.actions.Circularize = function(wayId, projection) { var action = function(graph) { var way = graph.entity(wayId), - nodes = _.uniq(graph.childNodes(way)), - tags = {}, key, role; + nodes = _.uniq(graph.childNodes(way)); var points = nodes.map(function(n) { return projection(n.loc); @@ -21,8 +20,6 @@ iD.actions.Circularize = function(wayId, projection) { })); } - circular_nodes.push(circular_nodes[0]); - for (i = 0; i < nodes.length; i++) { if (graph.parentWays(nodes[i]).length > 1) { var closest, closest_dist = Infinity, dist; @@ -34,8 +31,6 @@ iD.actions.Circularize = function(wayId, projection) { } } circular_nodes.splice(closest, 1, nodes[i]); - if (closest === 0) circular_nodes.splice(circular_nodes.length - 1, 1, nodes[i]); - else if (closest === circular_nodes.length - 1) circular_nodes.splice(0, 1, nodes[i]); } } @@ -46,6 +41,8 @@ iD.actions.Circularize = function(wayId, projection) { var ids = _.pluck(circular_nodes, 'id'), difference = _.difference(_.uniq(way.nodes), ids); + ids.push(ids[0]); + graph = graph.replace(way.update({nodes: ids})); for (i = 0; i < difference.length; i++) { From 13b0b540a7cbc15d0c67f067f72a3f3f16092e62 Mon Sep 17 00:00:00 2001 From: Ansis Brammanis Date: Sat, 2 Feb 2013 23:47:29 -0500 Subject: [PATCH 12/12] Don't snap to midpoints, snap to their parent way --- js/id/behavior/draw_way.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/js/id/behavior/draw_way.js b/js/id/behavior/draw_way.js index 9efcd5bb9..f724613da 100644 --- a/js/id/behavior/draw_way.js +++ b/js/id/behavior/draw_way.js @@ -16,10 +16,13 @@ iD.behavior.DrawWay = function(context, wayId, index, mode, baseGraph) { function move(datum) { var loc = context.map().mouseCoordinates(); - if (datum.type === 'node' || datum.type === 'midpoint') { + if (datum.type === 'node') { loc = datum.loc; - } else if (datum.type === 'way') { - loc = iD.geo.chooseIndex(datum, d3.mouse(context.surface().node()), context).loc; + } else if (datum.type === 'midpoint' || datum.type === 'way') { + var way = datum.type === 'way' ? + datum : + baseGraph.entity(datum.ways[0].id); + loc = iD.geo.chooseIndex(way, d3.mouse(context.surface().node()), context).loc; } context.replace(iD.actions.MoveNode(nodeId, loc));