From 6b18066dd695c5b60f998204bf26b0a5aa55edc5 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 26 Apr 2015 01:07:48 -0400 Subject: [PATCH 1/7] Graph#revert (see #537) --- js/id/core/graph.js | 10 ++++++ test/spec/core/graph.js | 69 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/js/id/core/graph.js b/js/id/core/graph.js index 772206122..20d114bce 100644 --- a/js/id/core/graph.js +++ b/js/id/core/graph.js @@ -251,6 +251,16 @@ iD.Graph.prototype = { }); }, + revert: function(entity) { + if (this.entities[entity.id] === this.base().entities[entity.id]) + return this; + + return this.update(function() { + this._updateCalculated(entity, this.base().entities[entity.id]); + delete this.entities[entity.id]; + }); + }, + update: function() { var graph = this.frozen ? iD.Graph(this, true) : this; diff --git a/test/spec/core/graph.js b/test/spec/core/graph.js index d5238270d..65c4c76cb 100644 --- a/test/spec/core/graph.js +++ b/test/spec/core/graph.js @@ -334,28 +334,83 @@ describe('iD.Graph', function() { expect(graph.replace(w1).parentWays(node)).to.eql([w1]); }); - it("adds parentRels", function () { + it("adds parentRelations", function () { var node = iD.Node({id: 'n' }), - r1 = iD.Relation({id: 'w', members: [{id: 'n'}]}), - graph = iD.Graph([node]); + r1 = iD.Relation({id: 'r', members: [{id: 'n'}]}), + graph = iD.Graph([node]); expect(graph.replace(r1).parentRelations(node)).to.eql([r1]); }); it("removes parentRelations", function () { var node = iD.Node({id: 'n' }), - r1 = iD.Relation({id: 'w', members: [{id: 'n'}]}), - graph = iD.Graph([node, r1]); + r1 = iD.Relation({id: 'r', members: [{id: 'n'}]}), + graph = iD.Graph([node, r1]); expect(graph.remove(r1).parentRelations(node)).to.eql([]); }); it("doesn't add duplicate parentRelations", function () { var node = iD.Node({id: 'n' }), - r1 = iD.Relation({id: 'w', members: [{id: 'n'}]}), - graph = iD.Graph([node, r1]); + r1 = iD.Relation({id: 'r', members: [{id: 'n'}]}), + graph = iD.Graph([node, r1]); expect(graph.replace(r1).parentRelations(node)).to.eql([r1]); }); }); + describe("#revert", function () { + it("is a no-op if the entity is identical to the base entity", function () { + var n1 = iD.Node({id: 'n' }), + graph = iD.Graph([n1]); + expect(graph.revert(n1)).to.equal(graph); + }); + + it("returns a new graph", function () { + var n1 = iD.Node({id: 'n' }), + n2 = n1.update({}), + graph = iD.Graph([n1]).replace(n2); + expect(graph.revert(n2)).not.to.equal(graph); + }); + + it("doesn't modify the receiver", function () { + var n1 = iD.Node({id: 'n' }), + n2 = n1.update({}), + graph = iD.Graph([n1]).replace(n2); + graph.revert(n2); + expect(graph.entity(n2.id)).to.equal(n2); + }); + + it("reverts the entity to the base version", function () { + var n1 = iD.Node({id: 'n' }), + n2 = n1.update({}), + graph = iD.Graph([n1]).replace(n2); + + expect(graph.entity('n')).to.equal(n2); + graph = graph.revert(n2); + expect(graph.entity('n')).to.equal(n1); + }); + + it("reverts parentWays", function () { + var n1 = iD.Node({id: 'n' }), + w1 = iD.Way({id: 'w', nodes: ['n']}), + w2 = w1.removeNode('n'), + graph = iD.Graph([n1, w1]).replace(w2); + + expect(graph.parentWays(graph.entity('n'))).to.eql([]); + graph = graph.revert(w2); + expect(graph.parentWays(graph.entity('n'))).to.eql([w1]); + }); + + it("reverts parentRelations", function () { + var n1 = iD.Node({id: 'n' }), + r1 = iD.Relation({id: 'r', members: [{id: 'n'}]}), + r2 = r1.removeMembersWithID('n'), + graph = iD.Graph([n1, r1]).replace(r2); + + expect(graph.parentRelations(graph.entity('n'))).to.eql([]); + graph = graph.revert(r2); + expect(graph.parentRelations(graph.entity('n'))).to.eql([r1]); + }); + }); + describe("#update", function () { it("returns a new graph if self is frozen", function () { var graph = iD.Graph(); From c10b83f28f0e2a032d947977e8c698fb5ff1030b Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 26 Apr 2015 01:08:31 -0400 Subject: [PATCH 2/7] iD.actions.Revert (see #537) --- index.html | 1 + js/id/actions/revert.js | 5 +++++ test/index.html | 2 ++ test/index_packaged.html | 1 + test/spec/actions/revert.js | 11 +++++++++++ 5 files changed, 20 insertions(+) create mode 100644 js/id/actions/revert.js create mode 100644 test/spec/actions/revert.js diff --git a/index.html b/index.html index fd5e3aeba..348fabb32 100644 --- a/index.html +++ b/index.html @@ -169,6 +169,7 @@ + diff --git a/js/id/actions/revert.js b/js/id/actions/revert.js new file mode 100644 index 000000000..21da5bf7e --- /dev/null +++ b/js/id/actions/revert.js @@ -0,0 +1,5 @@ +iD.actions.Revert = function(entity) { + return function(graph) { + return graph.revert(entity); + }; +}; diff --git a/test/index.html b/test/index.html index ae6159606..d72a3840b 100644 --- a/test/index.html +++ b/test/index.html @@ -147,6 +147,7 @@ + @@ -250,6 +251,7 @@ + diff --git a/test/index_packaged.html b/test/index_packaged.html index fe590c099..e3ea2f00c 100644 --- a/test/index_packaged.html +++ b/test/index_packaged.html @@ -54,6 +54,7 @@ + diff --git a/test/spec/actions/revert.js b/test/spec/actions/revert.js new file mode 100644 index 000000000..cdfa19da1 --- /dev/null +++ b/test/spec/actions/revert.js @@ -0,0 +1,11 @@ +describe('iD.actions.Revert', function() { + it('reverts an entity', function() { + var n1 = iD.Node({id: 'n' }), + n2 = n1.update({}), + graph = iD.Graph([n1]).replace(n2); + + expect(graph.entity('n')).to.equal(n2); + graph = iD.actions.Revert(n2)(graph) + expect(graph.entity('n')).to.equal(n1); + }); +}); From 00c0641f06a8f875164a0fef57afba2e737b0ea5 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 27 Apr 2015 10:16:47 -0400 Subject: [PATCH 3/7] Make view extent include both local and remote versions of the change --- js/id/renderer/map.js | 13 +++++++++---- js/id/ui/conflicts.js | 16 +++++++++++++--- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/js/id/renderer/map.js b/js/id/renderer/map.js index f7cf27792..ed6dd2841 100644 --- a/js/id/renderer/map.js +++ b/js/id/renderer/map.js @@ -409,10 +409,15 @@ iD.Map = function(context) { } }; - map.trimmedExtent = function() { - var headerY = 60, footerY = 30, pad = 10; - return new iD.geo.Extent(projection.invert([pad, dimensions[1] - footerY - pad]), - projection.invert([dimensions[0] - pad, headerY + pad])); + map.trimmedExtent = function(_) { + if (!arguments.length) { + var headerY = 60, footerY = 30, pad = 10; + return new iD.geo.Extent(projection.invert([pad, dimensions[1] - footerY - pad]), + projection.invert([dimensions[0] - pad, headerY + pad])); + } else { + var extent = iD.geo.Extent(_); + map.centerZoom(extent.center(), map.trimmedExtentZoom(extent)); + } }; function calcZoom(extent, dim) { diff --git a/js/id/ui/conflicts.js b/js/id/ui/conflicts.js index 321dd9a0b..01ef1e76c 100644 --- a/js/id/ui/conflicts.js +++ b/js/id/ui/conflicts.js @@ -198,17 +198,27 @@ iD.ui.Conflicts = function(context) { .selectAll('input') .property('checked', function(d) { return d === datum; }); + var extent = iD.geo.Extent(), + entity; + + entity = context.graph().hasEntity(datum.id); + if (entity) extent._extend(entity.extent(context.graph())); + datum.action(); - zoomToEntity(datum.id); + + entity = context.graph().hasEntity(datum.id); + if (entity) extent._extend(entity.extent(context.graph())); + + zoomToEntity(datum.id, extent); } - function zoomToEntity(id) { + function zoomToEntity(id, extent) { context.surface().selectAll('.hover') .classed('hover', false); var entity = context.graph().hasEntity(id); if (entity) { - context.map().zoomTo(entity); + context.map().trimmedExtent(extent); context.surface().selectAll( iD.util.entityOrMemberSelector([entity.id], context.graph())) .classed('hover', true); From a565b72f7da030bbe5ca798a38f1926ec626223b Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 27 Apr 2015 10:18:07 -0400 Subject: [PATCH 4/7] Before saving, revert entities where user chose "keep theirs" --- js/id/modes/save.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/js/id/modes/save.js b/js/id/modes/save.js index b66037604..83c471c9b 100644 --- a/js/id/modes/save.js +++ b/js/id/modes/save.js @@ -175,6 +175,20 @@ iD.modes.Save = function(context) { selection.remove(); }) .on('save', function() { + for (var i = 0; i < conflicts.length; i++) { + if (conflicts[i].chosen === 1) { // user chose "keep theirs" + var entity = context.entity(conflicts[i].id); + if (entity.type === 'way') { + var children = _.uniq(entity.nodes); + for (var j = 0; j < children.length; j++) { + var child = context.entity(children[j]); + history.replace(iD.actions.Revert(child)); + } + } + history.replace(iD.actions.Revert(entity)); + } + } + selection.remove(); save(e, true); }) From cecdc012ee15e3c46b81fa4fbd28ad9b0a4c43e9 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 27 Apr 2015 11:37:43 -0400 Subject: [PATCH 5/7] Don't commit empty changesets.. (closes #1483) instead, silently flush context and return to browse mode --- js/id/modes/save.js | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/js/id/modes/save.js b/js/id/modes/save.js index 83c471c9b..ce8df98a2 100644 --- a/js/id/modes/save.js +++ b/js/id/modes/save.js @@ -134,23 +134,30 @@ iD.modes.Save = function(context) { } else if (errors.length) { showErrors(); } else { - context.connection().putChangeset( - history.changes(iD.actions.DiscardTags(history.difference())), - e.comment, - history.imageryUsed(), - function(err, changeset_id) { - if (err) { - errors.push({ - msg: err.responseText, - details: [ t('save.status_code', { code: err.status }) ] - }); - showErrors(); - } else { - loading.close(); - context.flush(); - success(e, changeset_id); - } - }); + var changes = history.changes(iD.actions.DiscardTags(history.difference())); + if (changes.modified.length || changes.created.length || changes.deleted.length) { + context.connection().putChangeset( + changes, + e.comment, + history.imageryUsed(), + function(err, changeset_id) { + if (err) { + errors.push({ + msg: err.responseText, + details: [ t('save.status_code', { code: err.status }) ] + }); + showErrors(); + } else { + loading.close(); + context.flush(); + success(e, changeset_id); + } + }); + } else { // changes were insignificant or reverted by user + loading.close(); + context.flush(); + cancel(); + } } } From b087e78528e9e10775be01679d5c961dc1b541d6 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 27 Apr 2015 11:46:11 -0400 Subject: [PATCH 6/7] Allow zoomToEntity to be called w/o Extent In case user clicks on Entity description - line 102 --- js/id/ui/conflicts.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/js/id/ui/conflicts.js b/js/id/ui/conflicts.js index 01ef1e76c..8bf1011fa 100644 --- a/js/id/ui/conflicts.js +++ b/js/id/ui/conflicts.js @@ -218,7 +218,11 @@ iD.ui.Conflicts = function(context) { var entity = context.graph().hasEntity(id); if (entity) { - context.map().trimmedExtent(extent); + if (extent) { + context.map().trimmedExtent(extent); + } else { + context.map().zoomTo(entity); + } context.surface().selectAll( iD.util.entityOrMemberSelector([entity.id], context.graph())) .classed('hover', true); From 587d1451be0bef77b628d2ba949beb1480e420fe Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 27 Apr 2015 12:02:59 -0400 Subject: [PATCH 7/7] More tests for Graph#revert (reverting new things should remove them) --- test/spec/core/graph.js | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/test/spec/core/graph.js b/test/spec/core/graph.js index 65c4c76cb..8ed01880e 100644 --- a/test/spec/core/graph.js +++ b/test/spec/core/graph.js @@ -378,7 +378,7 @@ describe('iD.Graph', function() { expect(graph.entity(n2.id)).to.equal(n2); }); - it("reverts the entity to the base version", function () { + it("reverts an updated entity to the base version", function () { var n1 = iD.Node({id: 'n' }), n2 = n1.update({}), graph = iD.Graph([n1]).replace(n2); @@ -388,7 +388,16 @@ describe('iD.Graph', function() { expect(graph.entity('n')).to.equal(n1); }); - it("reverts parentWays", function () { + it("removes a new entity", function () { + var n1 = iD.Node({id: 'n' }), + graph = iD.Graph().replace(n1); + + expect(graph.entity('n')).to.equal(n1); + graph = graph.revert(n1); + expect(graph.hasEntity('n')).to.be.undefined; + }); + + it("reverts updated parentWays", function () { var n1 = iD.Node({id: 'n' }), w1 = iD.Way({id: 'w', nodes: ['n']}), w2 = w1.removeNode('n'), @@ -399,7 +408,7 @@ describe('iD.Graph', function() { expect(graph.parentWays(graph.entity('n'))).to.eql([w1]); }); - it("reverts parentRelations", function () { + it("reverts updated parentRelations", function () { var n1 = iD.Node({id: 'n' }), r1 = iD.Relation({id: 'r', members: [{id: 'n'}]}), r2 = r1.removeMembersWithID('n'), @@ -409,6 +418,26 @@ describe('iD.Graph', function() { graph = graph.revert(r2); expect(graph.parentRelations(graph.entity('n'))).to.eql([r1]); }); + + it("removes new parentWays", function () { + var n1 = iD.Node({id: 'n' }), + w1 = iD.Way({id: 'w', nodes: ['n']}), + graph = iD.Graph().replace(n1).replace(w1); + + expect(graph.parentWays(graph.entity('n'))).to.eql([w1]); + graph = graph.revert(w1); + expect(graph.parentWays(graph.entity('n'))).to.eql([]); + }); + + it("removes new parentRelations", function () { + var n1 = iD.Node({id: 'n' }), + r1 = iD.Relation({id: 'r', members: [{id: 'n'}]}), + graph = iD.Graph().replace(n1).replace(r1); + + expect(graph.parentRelations(graph.entity('n'))).to.eql([r1]); + graph = graph.revert(r1); + expect(graph.parentRelations(graph.entity('n'))).to.eql([]); + }); }); describe("#update", function () {