From 38b50a347e3e3baeafa2541651c1f63240bf69e9 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 17 Feb 2015 22:09:56 -0500 Subject: [PATCH] WIP: Fix tests, simplify mergeChildNodes --- js/id/actions/merge_remote_changes.js | 64 ++++++++++++----------- test/spec/actions/merge_remote_changes.js | 48 ++++++++--------- 2 files changed, 56 insertions(+), 56 deletions(-) diff --git a/js/id/actions/merge_remote_changes.js b/js/id/actions/merge_remote_changes.js index 55a897fc2..2fb9d4fe4 100644 --- a/js/id/actions/merge_remote_changes.js +++ b/js/id/actions/merge_remote_changes.js @@ -6,47 +6,35 @@ iD.actions.MergeRemoteChanges = function(id, remoteGraph, formatUser) { return _.isFunction(formatUser) ? formatUser(d) : d; } - function mergeChildren(remote, target, graph) { - var children = graph.childNodes(target), - updateNodes = [], - removeNodes = [], - i; + function mergeChildNodes(target, children, replacements) { + if (!target) return; - for (i = 0; i < children.length; i++) { - var lnode = children[i], - rnode = remoteGraph.hasEntity(lnode.id); + for (var i = 0; i < children.length; i++) { + var localNode = children[i], + remoteNode = remoteGraph.hasEntity(localNode.id); + + if (!remoteNode) continue; if (option === 'force_remote') { - if (rnode) { - updateNodes.push(rnode); - } else { - removeNodes.push(lnode); - } + replacements.push(remoteNode); } else { - var tversion = (rnode && rnode.version) || (+lnode.version + 1), - tnode = iD.Entity(lnode, { version: tversion }); - - tnode = mergeLocation(rnode, tnode); - if (tnode) { - updateNodes.push(tnode); + var targetNode = iD.Entity(localNode, { version: remoteNode.version }); + targetNode = mergeLocation(remoteNode, targetNode); + if (targetNode) { + replacements.push(targetNode); } else { - return graph; // child location conflict + return; // fail merge } } } - for (i = 0; i < updateNodes.length; i++) { - graph = graph.replace(updateNodes[i]); - } - for (i = 0; i < removeNodes.length; i++) { - graph = iD.actions.DeleteNode(removeNodes[i].id)(graph); - } - - return graph; + return target; } function mergeLocation(remote, 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); @@ -65,6 +53,8 @@ iD.actions.MergeRemoteChanges = function(id, remoteGraph, formatUser) { function mergeNodes(base, remote, target) { + if (!target) return; + if (option === 'force_local' || _.isEqual(target.nodes, remote.nodes)) { return target; } @@ -102,6 +92,8 @@ iD.actions.MergeRemoteChanges = function(id, remoteGraph, formatUser) { function mergeMembers(remote, target) { + if (!target) return; + if (option === 'force_local' || _.isEqual(target.members, remote.members)) { return target; } @@ -150,24 +142,34 @@ iD.actions.MergeRemoteChanges = function(id, remoteGraph, formatUser) { return fail ? undefined : changed ? target.update({tags: tags}) : target; } + var action = function(graph) { var base = graph.base().entities[id], local = graph.entity(id), remote = remoteGraph.entity(id), - target = iD.Entity(local, { version: remote.version }); + target = iD.Entity(local, { version: remote.version }), + replacements = []; if (target.type === 'node') { target = mergeLocation(remote, target); } else if (target.type === 'way') { graph.rebase(remoteGraph.childNodes(remote), [graph], false); - graph = mergeChildren(remote, target, graph); + target = mergeChildNodes(target, graph.childNodes(local), replacements); target = mergeNodes(base, remote, target); } else if (target.type === 'relation') { target = mergeMembers(remote, target); } target = mergeTags(base, remote, target); - return target ? graph.replace(target) : graph; + + if (target) { + graph = graph.replace(target); + for (var i = 0; i < replacements.length; i++) { + graph = graph.replace(replacements[i]); + } + } + + return graph; }; action.withOption = function(opt) { diff --git a/test/spec/actions/merge_remote_changes.js b/test/spec/actions/merge_remote_changes.js index d6c193b7d..b90a1976c 100644 --- a/test/spec/actions/merge_remote_changes.js +++ b/test/spec/actions/merge_remote_changes.js @@ -68,11 +68,10 @@ describe("iD.actions.MergeRemoteChanges", function () { "merge_remote_changes": { "annotation": "Merged remote changes from server.", "conflict": { - "general": "Conflicting edits were made to {type} {id} {name}", - "location": "Location was changed both locally and remotely.", - "nodelist": "Nodes were changed both locally and remotely.", - "memberlist": "Relation members were changed both locally and remotely.", - "tags": "Tag \"{tag}\" was changed to \"{local}\" locally and \"{remote}\" remotely." + "location": "This object was moved by both you and {user}.", + "nodelist": "Nodes were changed by both you and {user}.", + "memberlist": "Relation members were changed by both you and {user}.", + "tags": "You changed the {tag} tag to \"{local}\" and {user} changed it to \"{remote}\"." } } } @@ -101,7 +100,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Node({id: 'a', loc: remoteLoc, version: '2', tags: remoteTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('a', graph, altGraph); + action = iD.actions.MergeRemoteChanges('a', altGraph); graph = action(graph); @@ -117,7 +116,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Node({id: 'a', loc: remoteLoc, version: '2', tags: remoteTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('a', graph, altGraph); + action = iD.actions.MergeRemoteChanges('a', altGraph); graph = action(graph); @@ -133,7 +132,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Node({id: 'a', loc: remoteLoc, version: '2', tags: remoteTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('a', graph, altGraph); + action = iD.actions.MergeRemoteChanges('a', altGraph); graph = action(graph); @@ -152,7 +151,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('w1', graph, altGraph); + action = iD.actions.MergeRemoteChanges('w1', altGraph); graph = action(graph); @@ -168,7 +167,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('w1', graph, altGraph); + action = iD.actions.MergeRemoteChanges('w1', altGraph); graph = action(graph); @@ -185,7 +184,7 @@ describe("iD.actions.MergeRemoteChanges", function () { 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); + action = iD.actions.MergeRemoteChanges('w1', altGraph); graph = action(graph); @@ -205,7 +204,7 @@ describe("iD.actions.MergeRemoteChanges", function () { 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); + action = iD.actions.MergeRemoteChanges('w1', altGraph); graph = action(graph); @@ -223,7 +222,7 @@ describe("iD.actions.MergeRemoteChanges", function () { 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); + action = iD.actions.MergeRemoteChanges('w1', altGraph); graph = action(graph); @@ -243,7 +242,7 @@ describe("iD.actions.MergeRemoteChanges", function () { 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); + action = iD.actions.MergeRemoteChanges('w1', altGraph); graph = action(graph); @@ -262,7 +261,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Relation({id: 'r', members: remoteMembers, version: '2', tags: remoteRelTags}), graph = makeGraph([local]), altGraph = makeGraph([s1, s2, s3, s4, w4]); - action = iD.actions.MergeRemoteChanges('r', graph, altGraph); + action = iD.actions.MergeRemoteChanges('r', altGraph); graph = action(graph); @@ -277,7 +276,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Relation({id: 'r', members: relMembers, version: '2', tags: remoteRelTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]); - action = iD.actions.MergeRemoteChanges('r', graph, altGraph); + action = iD.actions.MergeRemoteChanges('r', altGraph); graph = action(graph); @@ -292,7 +291,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Relation({id: 'r', members: relMembers, version: '2', tags: remoteRelTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]); - action = iD.actions.MergeRemoteChanges('r', graph, altGraph); + action = iD.actions.MergeRemoteChanges('r', altGraph); graph = action(graph); @@ -309,7 +308,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Node({id: 'a', loc: remoteLoc, version: '2'}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('a', graph, altGraph); + action = iD.actions.MergeRemoteChanges('a', altGraph); graph = action(graph); @@ -329,7 +328,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Node({id: 'a', loc: remoteLoc, version: '2', tags: remoteTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('a', graph, altGraph).withOption('force_local'); + action = iD.actions.MergeRemoteChanges('a', altGraph).withOption('force_local'); graph = action(graph); @@ -347,7 +346,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Node({id: 'a', loc: remoteLoc, version: '2', tags: remoteTags}), graph = makeGraph([local]), altGraph = makeGraph([remote]), - action = iD.actions.MergeRemoteChanges('a', graph, altGraph).withOption('force_remote'); + action = iD.actions.MergeRemoteChanges('a', altGraph).withOption('force_remote'); graph = action(graph); @@ -367,12 +366,11 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), graph = makeGraph([local, r1]), altGraph = makeGraph([remote, s3]), - action = iD.actions.MergeRemoteChanges('w1', graph, altGraph).withOption('force_local'); + action = iD.actions.MergeRemoteChanges('w1', altGraph).withOption('force_local'); graph = action(graph); expect(graph.entity('w1').version).to.eql('2'); - expect(graph.hasEntity('s3')).to.be.undefined; expect(graph.entity('w1').nodes).to.eql(localNodes); expect(graph.entity('w1').tags).to.eql(localTags); }); @@ -386,7 +384,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), graph = makeGraph([local, r1]), altGraph = makeGraph([remote, s3]), - action = iD.actions.MergeRemoteChanges('w1', graph, altGraph).withOption('force_remote'); + action = iD.actions.MergeRemoteChanges('w1', altGraph).withOption('force_remote'); graph = action(graph); @@ -407,7 +405,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Relation({id: 'r', members: remoteMembers, version: '2', tags: remoteRelTags}), graph = makeGraph([local, r1, r2, r3, r4, w3]), altGraph = makeGraph([remote, s1, s2, s3, s4, w4]), - action = iD.actions.MergeRemoteChanges('r', graph, altGraph).withOption('force_local'); + action = iD.actions.MergeRemoteChanges('r', altGraph).withOption('force_local'); graph = action(graph); @@ -425,7 +423,7 @@ describe("iD.actions.MergeRemoteChanges", function () { remote = iD.Relation({id: 'r', members: remoteMembers, version: '2', tags: remoteRelTags}), graph = makeGraph([local, r1, r2, r3, r4, w3]), altGraph = makeGraph([remote, s1, s2, s3, s4, w4]), - action = iD.actions.MergeRemoteChanges('r', graph, altGraph).withOption('force_remote'); + action = iD.actions.MergeRemoteChanges('r', altGraph).withOption('force_remote'); graph = action(graph);