From fc0a15e6c47f5a4ced2a1965df7ae59138ceb32b Mon Sep 17 00:00:00 2001 From: tyr Date: Thu, 20 Feb 2014 15:27:29 +0100 Subject: [PATCH 1/3] fix duplicate objects after restoring data from localStorage This makes sure that the originals of changed entities get merged into the base of the stack after restoring the data from JSON. This is necessary, because the stack will only have elements for the current viewport after a restart and previously *modified* objects will now be falsely detected as *created* ones. Also removed some ineffective code. --- js/id/core/history.js | 23 +++++++++++++++++++---- test/spec/core/history.js | 4 ++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/js/id/core/history.js b/js/id/core/history.js index e4f76aae8..7325dbbb0 100644 --- a/js/id/core/history.js +++ b/js/id/core/history.js @@ -199,9 +199,19 @@ iD.History = function(context) { return x; }); + // make sure that the originals of changed entities get merged into + // the base of the stack after restoring the data from JSON. + var base = stack[0], + baseEntities = {}; + _.forEach(allEntities, function(entity) { + if (entity.id in base.graph.entities && !base.graph.entities.hasOwnProperty(entity.id)) + baseEntities[entity.id] = base.graph.entities[entity.id]; + }); + return JSON.stringify({ - version: 2, + version: 3, entities: _.values(allEntities), + baseEntities: _.values(baseEntities), stack: s, nextIDs: iD.Entity.id.next, index: index @@ -214,13 +224,20 @@ iD.History = function(context) { iD.Entity.id.next = h.nextIDs; index = h.index; - if (h.version === 2) { + if (h.version === 2 || h.version === 3) { var allEntities = {}; h.entities.forEach(function(entity) { allEntities[iD.Entity.key(entity)] = iD.Entity(entity); }); + if (h.version === 3) { + // this merges originals for changed entities into the base of + // the stack even if the current stack doesn't have them (for + // example when iD has been restarted in a different region) + stack[0].graph.rebase(h.baseEntities, _.pluck(stack, 'graph')); + } + stack = h.stack.map(function(d) { var entities = {}, entity; @@ -292,8 +309,6 @@ iD.History = function(context) { var json = context.storage(getKey('saved_history')); if (json) history.fromJSON(json); - - context.storage(getKey('saved_history', null)); }, _getKey: getKey diff --git a/test/spec/core/history.js b/test/spec/core/history.js index 0df9fc5d8..6c7a21496 100644 --- a/test/spec/core/history.js +++ b/test/spec/core/history.js @@ -229,12 +229,12 @@ describe("iD.History", function () { }); describe("#toJSON", function() { - it("generates v2 JSON", function() { + it("generates v3 JSON", function() { var node = iD.Node({id: 'n-1'}); history.merge([iD.Node({id: 'n1'})]); history.perform(iD.actions.AddEntity(node)); var json = JSON.parse(history.toJSON()); - expect(json.version).to.eql(2); + expect(json.version).to.eql(3); expect(json.entities).to.eql([node]); }); }); From 86c4bc910549b48761a1037d7948be88664fa987 Mon Sep 17 00:00:00 2001 From: tyr Date: Thu, 20 Feb 2014 18:42:54 +0100 Subject: [PATCH 2/3] add tests for "restoring from v3 JSON" (core/history.js) --- test/spec/core/history.js | 67 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/test/spec/core/history.js b/test/spec/core/history.js index 6c7a21496..bbffe247e 100644 --- a/test/spec/core/history.js +++ b/test/spec/core/history.js @@ -350,5 +350,72 @@ describe("iD.History", function () { expect(history.imageryUsed()).to.eql(["Bing"]); expect(iD.Entity.id.next).to.eql({node: -1, way: -2, relation: -3}); }); + + it("restores from v3 JSON (creation)", function() { + var json = { + "version": 3, + "entities": [ + {"loc": [1, 2], "id": "n-1"} + ], + "baseEntities": [], + "stack": [ + {}, + {"modified": ["n-1v0"], "imageryUsed": ["Bing"], "annotation": "Added a point."} + ], + "nextIDs": {"node": -2, "way": -1, "relation": -1}, + "index": 1 + }; + history.fromJSON(JSON.stringify(json)); + expect(history.graph().entity('n-1')).to.eql(iD.Node({id: 'n-1', loc: [1, 2]})); + expect(history.undoAnnotation()).to.eql("Added a point."); + expect(history.imageryUsed()).to.eql(["Bing"]); + expect(iD.Entity.id.next).to.eql({node: -2, way: -1, relation: -1}); + expect(history.difference().created().length).to.eql(1); + }); + + it("restores from v3 JSON (modification)", function() { + var json = { + "version": 3, + "entities": [ + {"loc": [1, 2], "id": "n-1"}, + {"loc": [2, 3], "id": "n-1", "v": 1} + ], + "baseEntities": [{"loc": [1, 2], "id": "n-1"}], + "stack": [ + {}, + {"modified": ["n-1v0"], "imageryUsed": ["Bing"], "annotation": "Added a point."}, + {"modified": ["n-1v1"], "imageryUsed": ["Bing"], "annotation": "Moved a point."} + ], + "nextIDs": {"node": -2, "way": -1, "relation": -1}, + "index": 2 + }; + history.fromJSON(JSON.stringify(json)); + expect(history.graph().entity('n-1')).to.eql(iD.Node({id: 'n-1', loc: [2, 3], v: 1})); + expect(history.undoAnnotation()).to.eql("Moved a point."); + expect(history.imageryUsed()).to.eql(["Bing"]); + expect(iD.Entity.id.next).to.eql({node: -2, way: -1, relation: -1}); + expect(history.difference().modified().length).to.eql(1); + }); + + it("restores from v3 JSON (deletion)", function() { + var json = { + "version": 3, + "entities": [], + "baseEntities": [], + "stack": [ + {}, + {"deleted": ["n1"], "imageryUsed": ["Bing"], "annotation": "Deleted a point."} + ], + "nextIDs": {"node": -1, "way": -2, "relation": -3}, + "index": 1 + }; + history.fromJSON(JSON.stringify(json)); + history.merge([iD.Node({id: 'n1'})]); + expect(history.graph().hasEntity('n1')).to.be.undefined; + expect(history.undoAnnotation()).to.eql("Deleted a point."); + expect(history.imageryUsed()).to.eql(["Bing"]); + expect(iD.Entity.id.next).to.eql({node: -1, way: -2, relation: -3}); + expect(history.difference().deleted().length).to.eql(1); + }); }); }); From efd3223e0ca74d2fb1be258ccb8a81dbd59614e9 Mon Sep 17 00:00:00 2001 From: tyr Date: Sat, 22 Feb 2014 14:09:17 +0100 Subject: [PATCH 3/3] extend history loading fix to deletions Deleted objects need to be kept in the base of the history stack, too. This also improves the respective unit tests. --- js/id/core/history.js | 21 ++++++++++----------- test/spec/core/history.js | 17 +++++++++++------ 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/js/id/core/history.js b/js/id/core/history.js index 7325dbbb0..f71365d37 100644 --- a/js/id/core/history.js +++ b/js/id/core/history.js @@ -174,7 +174,9 @@ iD.History = function(context) { toJSON: function() { if (stack.length <= 1) return; - var allEntities = {}; + var allEntities = {}, + baseEntities = {}, + base = stack[0]; var s = stack.map(function(i) { var modified = [], deleted = []; @@ -187,6 +189,10 @@ iD.History = function(context) { } else { deleted.push(id); } + // make sure that the originals of changed or deleted entities get merged + // into the base of the stack after restoring the data from JSON. + if (id in base.graph.entities && !base.graph.entities.hasOwnProperty(id)) + baseEntities[id] = base.graph.entities[id]; }); var x = {}; @@ -199,15 +205,6 @@ iD.History = function(context) { return x; }); - // make sure that the originals of changed entities get merged into - // the base of the stack after restoring the data from JSON. - var base = stack[0], - baseEntities = {}; - _.forEach(allEntities, function(entity) { - if (entity.id in base.graph.entities && !base.graph.entities.hasOwnProperty(entity.id)) - baseEntities[entity.id] = base.graph.entities[entity.id]; - }); - return JSON.stringify({ version: 3, entities: _.values(allEntities), @@ -235,7 +232,9 @@ iD.History = function(context) { // this merges originals for changed entities into the base of // the stack even if the current stack doesn't have them (for // example when iD has been restarted in a different region) - stack[0].graph.rebase(h.baseEntities, _.pluck(stack, 'graph')); + var baseEntities = h.baseEntities.map(iD.Entity); + stack[0].graph.rebase(baseEntities, _.pluck(stack, 'graph')); + tree.rebase(baseEntities); } stack = h.stack.map(function(d) { diff --git a/test/spec/core/history.js b/test/spec/core/history.js index bbffe247e..064e286ec 100644 --- a/test/spec/core/history.js +++ b/test/spec/core/history.js @@ -230,12 +230,18 @@ describe("iD.History", function () { describe("#toJSON", function() { it("generates v3 JSON", function() { - var node = iD.Node({id: 'n-1'}); - history.merge([iD.Node({id: 'n1'})]); - history.perform(iD.actions.AddEntity(node)); + var node_1 = iD.Node({id: 'n-1'}), + node1 = iD.Node({id: 'n1'}), + node2 = iD.Node({id: 'n2'}), + node3 = iD.Node({id: 'n3'}); + history.merge([node1, node2, node3]); + history.perform(iD.actions.AddEntity(node_1)); // addition + history.perform(iD.actions.ChangeTags('n2', {k: 'v'})); // modification + history.perform(iD.actions.DeleteNode('n3')); // deletion var json = JSON.parse(history.toJSON()); expect(json.version).to.eql(3); - expect(json.entities).to.eql([node]); + expect(json.entities).to.eql([node_1, node2.update({tags: {k: 'v'}})]); + expect(json.baseEntities).to.eql([node2, node3]); }); }); @@ -401,7 +407,7 @@ describe("iD.History", function () { var json = { "version": 3, "entities": [], - "baseEntities": [], + "baseEntities": [{"loc": [1, 2], "id": "n1"}], "stack": [ {}, {"deleted": ["n1"], "imageryUsed": ["Bing"], "annotation": "Deleted a point."} @@ -410,7 +416,6 @@ describe("iD.History", function () { "index": 1 }; history.fromJSON(JSON.stringify(json)); - history.merge([iD.Node({id: 'n1'})]); expect(history.graph().hasEntity('n1')).to.be.undefined; expect(history.undoAnnotation()).to.eql("Deleted a point."); expect(history.imageryUsed()).to.eql(["Bing"]);