From 4bf21cbc8adcc497caaf4e55325d5fd29ea775f9 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 6 Jan 2014 16:23:15 -0800 Subject: [PATCH] Restore deleted nodes that are discovered to have extant parents This bug has existed since 1.0! Fixes #2085 --- js/id/core/graph.js | 48 ++++++++++++++++++++++--------- js/id/core/history.js | 5 +--- test/spec/core/graph.js | 64 ++++++++++++++++++++++++----------------- test/spec/core/tree.js | 20 ++++++------- 4 files changed, 82 insertions(+), 55 deletions(-) diff --git a/js/id/core/graph.js b/js/id/core/graph.js index ba11d7f11..633e75ec0 100644 --- a/js/id/core/graph.js +++ b/js/id/core/graph.js @@ -12,7 +12,7 @@ iD.Graph = function(other, mutable) { this.entities = Object.create({}); this._parentWays = Object.create({}); this._parentRels = Object.create({}); - this.rebase(other || []); + this.rebase(other || [], [this]); } this.transients = {}; @@ -95,22 +95,44 @@ iD.Graph.prototype = { // is used only during the history operation that merges newly downloaded // data into each state. To external consumers, it should appear as if the // graph always contained the newly downloaded data. - rebase: function(entities) { + rebase: function(entities, stack) { var base = this.base(), - i, k, child, id, keys; + i, j, k, id; - // Merging of data only needed if graph is the base graph - if (!this.inherited) { - for (i = 0; i < entities.length; i++) { - var entity = entities[i]; - if (!base.entities[entity.id]) { - base.entities[entity.id] = entity; - this._updateCalculated(undefined, entity, - base.parentWays, base.parentRels); + for (i = 0; i < entities.length; i++) { + var entity = entities[i]; + + if (base.entities[entity.id]) + continue; + + // Merging data into the base graph + base.entities[entity.id] = entity; + this._updateCalculated(undefined, entity, + base.parentWays, base.parentRels); + + // Restore provisionally-deleted nodes that are discovered to have an extant parent + if (entity.type === 'way') { + for (j = 0; j < entity.nodes.length; j++) { + id = entity.nodes[j]; + for (k = 1; k < stack.length; k++) { + var ents = stack[k].entities; + if (ents.hasOwnProperty(id) && ents[id] === undefined) { + delete ents[id]; + } + } } } } + for (i = 0; i < stack.length; i++) { + stack[i]._updateRebased(); + } + }, + + _updateRebased: function() { + var base = this.base(), + i, k, child, id, keys; + keys = Object.keys(this._parentWays); for (i = 0; i < keys.length; i++) { child = keys[i]; @@ -229,9 +251,7 @@ iD.Graph.prototype = { freeze: function() { this.frozen = true; - if (iD.debug) { - Object.freeze(this.entities); - } + // No longer freezing entities here due to in-place updates needed in rebase. return this; }, diff --git a/js/id/core/history.js b/js/id/core/history.js index 73749aadb..5d0cb5fb4 100644 --- a/js/id/core/history.js +++ b/js/id/core/history.js @@ -42,10 +42,7 @@ iD.History = function(context) { }, merge: function(entities, extent) { - for (var i = 0; i < stack.length; i++) { - stack[i].graph.rebase(entities); - } - + stack[0].graph.rebase(entities, _.pluck(stack, 'graph')); tree.rebase(entities); dispatch.change(undefined, extent); diff --git a/test/spec/core/graph.js b/test/spec/core/graph.js index 5fb2bbe6a..e8d6aa83c 100644 --- a/test/spec/core/graph.js +++ b/test/spec/core/graph.js @@ -62,26 +62,24 @@ describe('iD.Graph', function() { it("sets the frozen flag", function () { expect(iD.Graph([], true).freeze().frozen).to.be.true; }); - - if (iD.debug) { - it("freezes entities", function () { - expect(Object.isFrozen(iD.Graph().entities)).to.be.true; - }); - } }); describe("#rebase", function () { it("preserves existing entities", function () { var node = iD.Node({id: 'n'}), graph = iD.Graph([node]); - graph.rebase([]); + + graph.rebase([], [graph]); + expect(graph.entity('n')).to.equal(node); }); it("includes new entities", function () { var node = iD.Node({id: 'n'}), graph = iD.Graph(); - graph.rebase([node]); + + graph.rebase([node], [graph]); + expect(graph.entity('n')).to.equal(node); }); @@ -89,13 +87,17 @@ describe('iD.Graph', function() { var a = iD.Node({id: 'n'}), b = iD.Node({id: 'n'}), graph = iD.Graph([a]); - graph.rebase([b]); + + graph.rebase([b], [graph]); + expect(graph.entity('n')).to.equal(a); }); it("inherits entities from base prototypally", function () { var graph = iD.Graph(); - graph.rebase([iD.Node()]); + + graph.rebase([iD.Node()], [graph]); + expect(graph.entities).not.to.have.ownProperty('n'); }); @@ -105,7 +107,8 @@ describe('iD.Graph', function() { w2 = iD.Way({id: 'w2', nodes: ['n']}), graph = iD.Graph([n, w1]); - graph.rebase([w2]); + graph.rebase([w2], [graph]); + expect(graph.parentWays(n)).to.eql([w1, w2]); expect(graph._parentWays.hasOwnProperty('n')).to.be.false; }); @@ -114,7 +117,9 @@ describe('iD.Graph', function() { var n = iD.Node({id: 'n'}), w1 = iD.Way({id: 'w1', nodes: ['n']}), graph = iD.Graph([n, w1]); - graph.rebase([w1]); + + graph.rebase([w1], [graph]); + expect(graph.parentWays(n)).to.eql([w1]); }); @@ -126,8 +131,7 @@ describe('iD.Graph', function() { graph = iD.Graph([n, w1]), graph2 = graph.replace(w2); - graph.rebase([w3]); - graph2.rebase([w3]); + graph.rebase([w3], [graph, graph2]); expect(graph2.parentWays(n)).to.eql([w1, w2, w3]); }); @@ -140,8 +144,7 @@ describe('iD.Graph', function() { graph = iD.Graph([n1, n2, w1]), graph2 = graph.replace(w2); - graph.rebase([w1]); - graph2.rebase([w1]); + graph.rebase([w1], [graph, graph2]); expect(graph2.parentWays(n2)).to.eql([]); }); @@ -152,19 +155,30 @@ describe('iD.Graph', function() { graph = iD.Graph([n, w1]), graph2 = graph.remove(w1); - graph.rebase([w1]); - graph2.rebase([w1]); + graph.rebase([w1], [graph, graph2]); expect(graph2.parentWays(n)).to.eql([]); }); + it("re-adds a deleted node that is discovered to have another parent", function() { + var n = iD.Node({id: 'n'}), + w1 = iD.Way({id: 'w1', nodes: ['n']}), + w2 = iD.Way({id: 'w2', nodes: ['n']}), + graph = iD.Graph([n, w1]), + graph2 = graph.remove(n); + + graph.rebase([n, w2], [graph, graph2]); + + expect(graph2.entity('n')).to.eql(n); + }); + it("updates parentRelations", function () { var n = iD.Node({id: 'n'}), r1 = iD.Relation({id: 'r1', members: [{id: 'n'}]}), r2 = iD.Relation({id: 'r2', members: [{id: 'n'}]}), graph = iD.Graph([n, r1]); - graph.rebase([r2]); + graph.rebase([r2], [graph]); expect(graph.parentRelations(n)).to.eql([r1, r2]); expect(graph._parentRels.hasOwnProperty('n')).to.be.false; @@ -177,8 +191,7 @@ describe('iD.Graph', function() { graph = iD.Graph([n, r1]), graph2 = graph.replace(r2); - graph.rebase([r1]); - graph2.rebase([r1]); + graph.rebase([r1], [graph, graph2]); expect(graph2.parentRelations(n)).to.eql([]); }); @@ -189,8 +202,7 @@ describe('iD.Graph', function() { graph = iD.Graph([n, r1]), graph2 = graph.remove(r1); - graph.rebase([r1]); - graph2.rebase([r1]); + graph.rebase([r1], [graph, graph2]); expect(graph2.parentRelations(n)).to.eql([]); }); @@ -203,8 +215,8 @@ describe('iD.Graph', function() { graph = iD.Graph([n, r1]), graph2 = graph.replace(r2); - graph.rebase([r3]); - graph2.rebase([r3]); + graph.rebase([r3], [graph, graph2]); + expect(graph2.parentRelations(n)).to.eql([r1, r2, r3]); }); @@ -221,7 +233,7 @@ describe('iD.Graph', function() { } expect(numParents(n)).to.equal(1); - graph.rebase([w2]); + graph.rebase([w2], [graph]); expect(numParents(n)).to.equal(2); }); }); diff --git a/test/spec/core/tree.js b/test/spec/core/tree.js index 50da2017b..35b3385a5 100644 --- a/test/spec/core/tree.js +++ b/test/spec/core/tree.js @@ -5,7 +5,7 @@ describe("iD.Tree", function() { tree = iD.Tree(graph), node = iD.Node({id: 'n', loc: [1, 1]}); - graph.rebase([node]); + graph.rebase([node], [graph]); tree.rebase([node]); expect(tree.intersects(iD.geo.Extent([0, 0], [2, 2]), graph)).to.eql([node]); @@ -17,11 +17,11 @@ describe("iD.Tree", function() { node = iD.Node({id: 'n', loc: [1, 1]}), extent = iD.geo.Extent([0, 0], [2, 2]); - graph.rebase([node]); + graph.rebase([node], [graph]); tree.rebase([node]); expect(tree.intersects(extent, graph)).to.eql([node]); - graph.rebase([node]); + graph.rebase([node], [graph]); tree.rebase([node]); expect(tree.intersects(extent, graph)).to.eql([node]); }); @@ -35,7 +35,7 @@ describe("iD.Tree", function() { expect(tree.intersects(iD.geo.Extent([9, 9], [11, 11]), g)).to.eql([node_]); - graph.rebase({n: node}); + graph.rebase([node], [graph]); tree.rebase([node]); expect(tree.intersects(iD.geo.Extent([0, 0], [2, 2]), g)).to.eql([]); @@ -63,11 +63,11 @@ describe("iD.Tree", function() { relation = iD.Relation({id: 'r', members: [{id: 'n1'}, {id: 'n2'}]}), extent = iD.geo.Extent([0.5, 0.5], [1.5, 1.5]); - graph.rebase([relation, n1]); + graph.rebase([relation, n1], [graph]); tree.rebase([relation, n1]); expect(tree.intersects(extent, graph)).to.eql([]); - graph.rebase([n2]); + graph.rebase([n2], [graph]); tree.rebase([n2]); expect(tree.intersects(extent, graph)).to.eql([n2, relation]); }); @@ -83,8 +83,7 @@ describe("iD.Tree", function() { expect(tree.intersects(extent, graph)).to.eql([]); - base.rebase([node]); - graph.rebase([node]); + base.rebase([node], [base, graph]); tree.rebase([node]); expect(tree.intersects(extent, graph)).to.eql([node, way]); }); @@ -160,7 +159,7 @@ describe("iD.Tree", function() { var graph = base.replace(node).remove(node); expect(tree.intersects(extent, graph)).to.eql([]); - base.rebase([node]); + base.rebase([node], [base]); tree.rebase([node]); expect(tree.intersects(extent, graph)).to.eql([]); }); @@ -176,8 +175,7 @@ describe("iD.Tree", function() { var graph = base.replace(r1).replace(r2); expect(tree.intersects(extent, graph)).to.eql([]); - base.rebase([node]); - graph.rebase([node]); + base.rebase([node], [base, graph]); tree.rebase([node]); expect(tree.intersects(extent, graph)).to.eql([node, r1, r2]); });