From c503b9f96c4e16a232a67307af43a5b89d6a0352 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 3 Mar 2015 23:43:37 -0500 Subject: [PATCH] fill remoteGraph with loadMultiple, finally do proper undeletion --- data/core.yaml | 2 +- dist/locales/en.json | 2 +- js/id/actions/merge_remote_changes.js | 55 +++++----- js/id/modes/save.js | 122 ++++++++++------------ test/spec/actions/merge_remote_changes.js | 2 +- 5 files changed, 88 insertions(+), 95 deletions(-) diff --git a/data/core.yaml b/data/core.yaml index 95fd8bc3b..fbbd813e5 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -334,7 +334,7 @@ en: your changes or the other user's changes. merge_remote_changes: conflict: - deleted: 'This object has been deleted.' + deleted: 'This object has been deleted by {user}.' 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}.' diff --git a/dist/locales/en.json b/dist/locales/en.json index 12ed1b56f..4eab3d39c 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -407,7 +407,7 @@ }, "merge_remote_changes": { "conflict": { - "deleted": "This object has been deleted.", + "deleted": "This object has been deleted by {user}.", "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}.", diff --git a/js/id/actions/merge_remote_changes.js b/js/id/actions/merge_remote_changes.js index 21f5f22cd..2888c57b5 100644 --- a/js/id/actions/merge_remote_changes.js +++ b/js/id/actions/merge_remote_changes.js @@ -63,11 +63,11 @@ iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph, formatUser } - function mergeChildren(target, children, updates, graph) { - function isUsed(node, target) { + function mergeChildren(targetWay, children, updates, graph) { + function isUsed(node, targetWay) { var parentWays = _.pluck(graph.parentWays(node), 'id'); return node.hasInterestingTags() || - _.without(parentWays, target.id).length > 0 || + _.without(parentWays, targetWay.id).length > 0 || graph.parentRelations(node).length > 0; } @@ -78,35 +78,41 @@ iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph, formatUser node = graph.hasEntity(id); // remove unused childNodes.. - if (target.nodes.indexOf(id) === -1) { - if (node && !isUsed(node, target)) { + if (targetWay.nodes.indexOf(id) === -1) { + if (node && !isUsed(node, targetWay)) { updates.removeIds.push(id); } continue; } // restore used childNodes.. - var localNode = localGraph.hasEntity(id), - remoteNode = remoteGraph.hasEntity(id), - targetNode; + var local = localGraph.hasEntity(id), + remote = remoteGraph.hasEntity(id), + target; - if (remoteNode && option === 'force_remote') { - updates.replacements.push(remoteNode); + if (!remote) continue; - } else if (localNode && option === 'force_local') { - targetNode = iD.Entity(localNode, - { version: (remoteNode ? remoteNode.version : +localNode.version + 1) }); - updates.replacements.push(targetNode); + if (option === 'force_remote' && remote.visible) { + updates.replacements.push(remote); + } + if (option === 'force_local' && local) { + target = iD.Entity(local, { version: remote.version }); + updates.replacements.push(target); + } + if (option === 'safe' && local && remote) { + target = iD.Entity(local, { version: remote.version }); + if (remote.visible) { + target = mergeLocation(remote, target); + } else { + conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) })); + } - } else if (localNode && remoteNode && option === 'safe') { - targetNode = iD.Entity(localNode, { version: remoteNode.version }); - targetNode = mergeLocation(remoteNode, targetNode); if (conflicts.length !== ccount) break; - updates.replacements.push(targetNode); + updates.replacements.push(target); } } - return target; + return targetWay; } @@ -191,16 +197,15 @@ iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph, formatUser var updates = { replacements: [], removeIds: [] }, base = graph.base().entities[id], local = localGraph.entity(id), - remote = remoteGraph.hasEntity(id), - target; + remote = remoteGraph.entity(id), + target = iD.Entity(local, { version: remote.version }); // delete/undelete - if (!remote) { + if (!remote.visible) { if (option === 'force_remote') { return iD.actions.DeleteMultiple([id])(graph); } else if (option === 'force_local') { - target = iD.Entity(local, { version: +local.version + 1 }); if (target.type === 'way') { target = mergeChildren(target, _.uniq(local.nodes), updates, graph); graph = updateChildren(updates, graph); @@ -208,14 +213,12 @@ iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph, formatUser return graph.replace(target); } else { - conflicts.push(t('merge_remote_changes.conflict.deleted')); + conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) })); return graph; // do nothing } } // merge - target = iD.Entity(local, { version: remote.version }); - if (target.type === 'node') { target = mergeLocation(remote, target); diff --git a/js/id/modes/save.js b/js/id/modes/save.js index f1ec772a5..2c8ede84a 100644 --- a/js/id/modes/save.js +++ b/js/id/modes/save.js @@ -8,6 +8,14 @@ iD.modes.Save = function(context) { } function save(e, tryAgain) { + function withChildNodes(ids) { + return _.uniq(_.reduce(toCheck, function(result, id) { + var e = context.entity(id); + if (e.type === 'way') result.push.apply(result, e.nodes); + return result; + }, _.clone(ids))); + } + var loading = iD.ui.Loading(context).message(t('save.uploading')).blocking(true), history = context.history(), origChanges = history.changes(iD.actions.DiscardTags(history.difference())), @@ -15,6 +23,7 @@ iD.modes.Save = function(context) { remoteGraph = iD.Graph(history.base(), true), modified = _.filter(history.difference().summary(), {changeType: 'modified'}), toCheck = _.pluck(_.pluck(modified, 'entity'), 'id'), + toLoad = withChildNodes(toCheck), conflicts = [], errors = []; @@ -22,89 +31,70 @@ iD.modes.Save = function(context) { context.container().call(loading); if (toCheck.length) { - _.each(toCheck, loadAndCheck); + context.connection().loadMultiple(toLoad, loaded); } else { finalize(); } // Reload modified entities into an alternate graph and check for conflicts.. - function loadAndCheck(id) { - context.connection().loadEntity(id, function(err, result) { - toCheck = _.without(toCheck, id); + function loaded(err, result) { + if (errors.length) return; - if (err) { - if (err.status === 410) { // Status: Gone (contains no responseText) - if (!tryAgain) { - remoteGraph.remove(remoteGraph.hasEntity(id)); - addDeleteConflict(id); - } - } else { - errors.push({ - id: id, - msg: err.responseText, - details: [ t('save.status_code', { code: err.status }) ] - }); - } + if (err) { + errors.push({ + msg: err.responseText, + details: [ t('save.status_code', { code: err.status }) ] + }); + showErrors(); - } else { - _.each(result.data, function(entity) { remoteGraph.replace(entity); }); - checkConflicts(id); + } else { + _.each(result.data, function(entity) { + remoteGraph.replace(entity); + toLoad = _.without(toLoad, entity.id); + }); + + if (!toLoad.length) { + checkConflicts(); } - - if (!toCheck.length) { - finalize(); - } - }); + } } - function addDeleteConflict(id) { - var local = localGraph.entity(id), - action = iD.actions.MergeRemoteChanges, - forceLocal = action(id, localGraph, remoteGraph).withOption('force_local'), - forceRemote = action(id, localGraph, remoteGraph).withOption('force_remote'); + function checkConflicts() { + _.each(toCheck, function(id) { + var local = localGraph.entity(id), + remote = remoteGraph.entity(id); - conflicts.push({ - id: id, - name: entityName(local), - details: [ t('merge_remote_changes.conflict.deleted') ], - chosen: 1, - choices: [ - choice(id, t('save.conflict.restore'), forceLocal), - choice(id, t('save.conflict.delete'), forceRemote) - ], + if (compareVersions(local, remote)) return; + + var action = iD.actions.MergeRemoteChanges, + merge = action(id, localGraph, remoteGraph, formatUser), + diff = history.replace(merge); + + if (diff.length()) return; // merged safely + + var forceLocal = action(id, localGraph, remoteGraph).withOption('force_local'), + forceRemote = action(id, localGraph, remoteGraph).withOption('force_remote'), + keepMine = t('save.conflict.' + (remote.visible ? 'keep_local' : 'restore')), + keepTheirs = t('save.conflict.' + (remote.visible ? 'keep_remote' : 'delete')); + + conflicts.push({ + id: id, + name: entityName(local), + details: merge.conflicts(), + chosen: 1, + choices: [ + choice(id, keepMine, forceLocal), + choice(id, keepTheirs, forceRemote) + ] + }); }); + + finalize(); } - function checkConflicts(id) { - var local = localGraph.entity(id), - remote = remoteGraph.entity(id); - - if (compareVersions(local, remote)) return; - - var action = iD.actions.MergeRemoteChanges, - merge = action(id, localGraph, remoteGraph, formatUser), - diff = history.replace(merge); - - if (diff.length()) return; // merged safely - - var forceLocal = action(id, localGraph, remoteGraph).withOption('force_local'), - forceRemote = action(id, localGraph, remoteGraph).withOption('force_remote'); - - conflicts.push({ - id: id, - name: entityName(local), - details: merge.conflicts(), - chosen: 1, - choices: [ - choice(id, t('save.conflict.keep_local'), forceLocal), - choice(id, t('save.conflict.keep_remote'), forceRemote) - ] - }); - } - function compareVersions(local, remote) { if (local.version !== remote.version) return false; diff --git a/test/spec/actions/merge_remote_changes.js b/test/spec/actions/merge_remote_changes.js index 97e07e5cf..9c15289ee 100644 --- a/test/spec/actions/merge_remote_changes.js +++ b/test/spec/actions/merge_remote_changes.js @@ -68,7 +68,7 @@ describe("iD.actions.MergeRemoteChanges", function () { 'merge_remote_changes': { "annotation": "Merged remote changes from server.", "conflict": { - "deleted": "This object has been deleted.", + "deleted": "This object has been deleted by {user}.", "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}.",