diff --git a/js/id/actions/merge_remote_changes.js b/js/id/actions/merge_remote_changes.js index a7e867093..d1c94908a 100644 --- a/js/id/actions/merge_remote_changes.js +++ b/js/id/actions/merge_remote_changes.js @@ -6,45 +6,82 @@ iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph) { function mergeLocation(target) { + if (!target) return; + function pointEqual(a, b) { var epsilon = 1e-6; return (Math.abs(a[0] - b[0]) < epsilon) && (Math.abs(a[1] - b[1]) < epsilon); } - if (!pointEqual(remote.loc, local.loc)) { - return (option === 'force_remote') ? target.update({loc: remote.loc}) : undefined; + if (pointEqual(target.loc, remote.loc)) { + return target; } - return target; + if (option === 'force_remote') { + return target.update({loc: remote.loc}); + } + + return; // fail merge } function mergeRemoteChildren(target) { + if (!target) return; + + if (_.isEqual(target.nodes, remote.nodes)) { + return target; + } if (option === 'force_remote') { return target.update({nodes: remote.nodes}); } - // todo, support non-destructive merging - // for now fail on any change.. - if (!_.isEqual(local.nodes, remote.nodes)) { - return; + var o = base.nodes || [], + a = local.nodes || [], + b = remote.nodes || [], + nodes = [], + hunks = Diff3.diff3_merge(a, o, b, true); + + for (var i = 0, imax = hunks.length; i !== imax; i++) { + var hunk = hunks[i]; + if (hunk.ok) { + nodes.push.apply(nodes, hunk.ok); + } + else { + // for all conflicts, we can assume c.a !== c.b + // because `diff3_merge` called with `true` option to exclude false conflicts.. + var c = hunk.conflict; + if (_.isEqual(c.o, c.a)) { // only changed remotely + nodes.push.apply(nodes, c.b); + } + else if (_.isEqual(c.o, c.b)) { // only changed locally + nodes.push.apply(nodes, c.a); + } + else { // changed both locally and remotely + return; // fail merge.. + } + } } - return target; + + return target.update({nodes: nodes}); } function mergeRemoteMembers(target) { + if (!target) return; + + if (_.isEqual(target.members, remote.members)) { + return target; + } if (option === 'force_remote') { return target.update({members: remote.members}); } - // todo, support non-destructive merging - // for now fail on any change.. - if (!_.isEqual(local.members, remote.members)) { - return; - } - return target; + return; // fail merge } function mergeRemoteTags(target) { - if (!target) { return; } + if (!target) return; + + if (_.isEqual(target.tags, remote.tags)) { + return target; + } if (option === 'force_remote') { return target.update({tags: remote.tags}); } @@ -60,9 +97,10 @@ iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph) { for (var i = 0, imax = keys.length; i !== imax; i++) { var k = keys[i]; if (remote.tags[k] !== base.tags[k]) { // tag modified remotely.. - if (local.tags[k] && local.tags[k] !== remote.tags[k]) { - return; - } else { + if (target.tags[k] && target.tags[k] !== remote.tags[k]) { + return; // fail merge.. + } + else { tags[k] = remote.tags[k]; changed = true; } @@ -81,10 +119,12 @@ iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph) { if (target.type === 'node') { target = mergeLocation(target); - } else if (target.type === 'way') { + } + else if (target.type === 'way') { graph.rebase(remoteGraph.childNodes(remote), [graph], false); target = mergeRemoteChildren(target); - } else if (target.type === 'relation') { + } + else if (target.type === 'relation') { target = mergeRemoteMembers(target); } diff --git a/test/spec/actions/merge_remote_changes.js b/test/spec/actions/merge_remote_changes.js index e7d554746..17e6a2722 100644 --- a/test/spec/actions/merge_remote_changes.js +++ b/test/spec/actions/merge_remote_changes.js @@ -148,24 +148,80 @@ describe("iD.actions.MergeRemoteChanges", function () { expect(graph.entity('w1').tags).to.eql({foo: 'foo_local', bar: 'bar_remote', area: 'yes'}); }); - it("doesn't merge ways if nodelist reordered", function () { + it("merges ways if nodelist changed only remotely", function () { var localNodes = ['p1', 'p2', 'p3', 'p4', 'p1'], // didn't change nodes - remoteNodes = ['p1', 'p3', 'p4', 'p2', 'p1'], // reordered nodes + remoteNodes = ['p1', 'r2', 'r3', 'p4', 'p1'], // changed nodes localTags = {foo: 'foo_local', area: 'yes'}, // changed tag foo remoteTags = {foo: 'foo', bar: 'bar_remote', area: 'yes'}, // didn't change tag foo, added tag bar local = iD.Way({id: 'w1', nodes: localNodes, version: '1', v: 2, tags: localTags}), remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), graph = makeGraph([local]), + altGraph = makeGraph([remote, r2, r3]), + action = iD.actions.MergeRemoteChanges('w1', graph, altGraph); + + graph = action(graph); + + expect(graph.entity('w1').version).to.eql('2'); + expect(graph.entity('w1').tags).to.eql({foo: 'foo_local', bar: 'bar_remote', area: 'yes'}); + expect(graph.entity('w1').nodes).to.eql(remoteNodes); + expect(graph.hasEntity('r2')).to.eql(r2); + expect(graph.hasEntity('r3')).to.eql(r3); + }); + + it("merges ways if nodelist changed only locally", function () { + var localNodes = ['p1', 'r2', 'r3', 'p4', 'p1'], // changed nodes + remoteNodes = ['p1', 'p2', 'p3', 'p4', 'p1'], // didn't change nodes + localTags = {foo: 'foo_local', area: 'yes'}, // changed tag foo + remoteTags = {foo: 'foo', bar: 'bar_remote', area: 'yes'}, // didn't change tag foo, added tag bar + local = iD.Way({id: 'w1', nodes: localNodes, version: '1', v: 2, tags: localTags}), + remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), + graph = makeGraph([local, r2, r3]), altGraph = makeGraph([remote]), action = iD.actions.MergeRemoteChanges('w1', graph, altGraph); graph = action(graph); + expect(graph.entity('w1').version).to.eql('2'); + expect(graph.entity('w1').tags).to.eql({foo: 'foo_local', bar: 'bar_remote', area: 'yes'}); + expect(graph.entity('w1').nodes).to.eql(localNodes); + }); + + it("merges ways if nodelist changes don't overlap", function () { + var localNodes = ['p1', 'r1', 'r2', 'p3', 'p4', 'p1'], // changed p2 -> r1, r2 + remoteNodes = ['p1', 'p2', 'p3', 'r3', 'r4', 'p1'], // changed p4 -> r3, r4 + localTags = {foo: 'foo_local', area: 'yes'}, // changed tag foo + remoteTags = {foo: 'foo', bar: 'bar_remote', area: 'yes'}, // didn't change tag foo, added tag bar + local = iD.Way({id: 'w1', nodes: localNodes, version: '1', v: 2, tags: localTags}), + remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), + graph = makeGraph([local, r1, r2]), + altGraph = makeGraph([remote, r3, r4]), + action = iD.actions.MergeRemoteChanges('w1', graph, altGraph); + + graph = action(graph); + + expect(graph.entity('w1').version).to.eql('2'); + expect(graph.entity('w1').tags).to.eql({foo: 'foo_local', bar: 'bar_remote', area: 'yes'}); + expect(graph.entity('w1').nodes).to.eql(['p1', 'r1', 'r2', 'p3', 'r3', 'r4', 'p1']); + expect(graph.hasEntity('r3')).to.eql(r3); + expect(graph.hasEntity('r4')).to.eql(r4); + }); + + it("doesn't merge ways if nodelist changes overlap", function () { + var localNodes = ['p1', 'r1', 'r2', 'p3', 'p4', 'p1'], // changed p2 -> r1, r2 + remoteNodes = ['p1', 'r3', 'r4', 'p3', 'p4', 'p1'], // changed p2 -> r3, r4 + localTags = {foo: 'foo_local', area: 'yes'}, // changed tag foo + remoteTags = {foo: 'foo', bar: 'bar_remote', area: 'yes'}, // didn't change tag foo, added tag bar + local = iD.Way({id: 'w1', nodes: localNodes, version: '1', v: 2, tags: localTags}), + remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), + graph = makeGraph([local, r1, r2]), + altGraph = makeGraph([remote, r3, r4]), + action = iD.actions.MergeRemoteChanges('w1', graph, altGraph); + + graph = action(graph); + expect(graph.entity('w1')).to.eql(local); }); - it("merges ways if nodelist order preserved"); - }); describe("relations", function () {