Merge pull request #8839 from tpetillon/history_preservation

Object history preservation improvements

modules/actions/add_member.js

@@ -32,7 +32,7 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) {
     // Add a way member into the relation "wherever it makes sense".
     // In this situation we were not supplied a memberIndex.
     function addWayMember(relation, graph) {
-        var groups, tempWay, item, i, j, k;
+        var groups, tempWay, insertPairIsReversed, item, i, j, k;
 
         // remove PTv2 stops and platforms before doing anything.
         var PTv2members = [];
@@ -65,6 +65,14 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) {
             groups = utilArrayGroupBy(tempRelation.members, 'type');
             groups.way = groups.way || [];
 
+            // Insert pair is reversed if the inserted way comes before the original one.
+            // (Except when they form a loop.)
+            var originalWay = graph.entity(insertPair.originalID);
+            var insertedWay = graph.entity(insertPair.insertedID);
+            insertPairIsReversed = originalWay.nodes.length > 0 && insertedWay.nodes.length > 0 &&
+                insertedWay.nodes[insertedWay.nodes.length - 1] === originalWay.nodes[0] &&
+                originalWay.nodes[originalWay.nodes.length - 1] !== insertedWay.nodes[0];
+
         } else {
             // Add the member anywhere, one time. Just push and let `osmJoinWays` decide where to put it.
             groups = utilArrayGroupBy(relation.members, 'type');
@@ -96,16 +104,17 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) {
 
                 // If this is a paired item, generate members in correct order and role
                 if (tempWay && item.id === tempWay.id) {
-                    if (nodes[0].id === insertPair.nodes[0]) {
-                        item.pair = [
-                            { id: insertPair.originalID, type: 'way', role: item.role },
-                            { id: insertPair.insertedID, type: 'way', role: item.role }
-                        ];
-                    } else {
+                    var reverse = nodes[0].id !== insertPair.nodes[0] ^ insertPairIsReversed;
+                    if (reverse) {
                         item.pair = [
                             { id: insertPair.insertedID, type: 'way', role: item.role },
                             { id: insertPair.originalID, type: 'way', role: item.role }
                         ];
+                    } else {
+                        item.pair = [
+                            { id: insertPair.originalID, type: 'way', role: item.role },
+                            { id: insertPair.insertedID, type: 'way', role: item.role }
+                        ];
                     }
                 }
 

modules/actions/connect.js

@@ -1,12 +1,13 @@
 import { actionDeleteNode } from './delete_node';
 import { actionDeleteWay } from './delete_way';
-import { utilArrayUniq } from '../util';
+import { osmEntity } from '../osm';
+import { utilArrayUniq, utilOldestID } from '../util';
 
 
 // Connect the ways at the given nodes.
 //
 // First choose a node to be the survivor, with preference given
-// to an existing (not new) node.
+// to the oldest existing (not new) and "interesting" node.
 //
 // Tags and relation memberships of of non-surviving nodes are merged
 // to the survivor.
@@ -24,11 +25,21 @@ export function actionConnect(nodeIDs) {
         var parents;
         var i, j;
 
-        // Choose a survivor node, prefer an existing (not new) node - #4974
+        // Select the node with the ID passed as parameter if it is in the list,
+        // otherwise select the node with the oldest ID as the survivor, or the
+        // last one if there are only new nodes.
+        nodeIDs.reverse();
+
+        var interestingIDs = [];
         for (i = 0; i < nodeIDs.length; i++) {
-            survivor = graph.entity(nodeIDs[i]);
-            if (survivor.version) break;  // found one
+            node = graph.entity(nodeIDs[i]);
+            if (node.hasInterestingTags()) {
+                if (!node.isNew()) {
+                    interestingIDs.push(node.id);
+                }
+            }
         }
+        survivor = graph.entity(utilOldestID(interestingIDs.length > 0 ? interestingIDs : nodeIDs));
 
         // Replace all non-surviving nodes with the survivor and merge tags.
         for (i = 0; i < nodeIDs.length; i++) {
@@ -71,11 +82,8 @@ export function actionConnect(nodeIDs) {
         var relations, relation, role;
         var i, j, k;
 
-        // Choose a survivor node, prefer an existing (not new) node - #4974
-        for (i = 0; i < nodeIDs.length; i++) {
-            survivor = graph.entity(nodeIDs[i]);
-            if (survivor.version) break;  // found one
-        }
+        // Select the node with the oldest ID as the survivor.
+        survivor = graph.entity(utilOldestID(nodeIDs));
 
         // 1. disable if the nodes being connected have conflicting relation roles
         for (i = 0; i < nodeIDs.length; i++) {

modules/actions/join.js

@@ -3,7 +3,7 @@ import { actionDeleteWay } from './delete_way';
 import { osmIsInterestingTag } from '../osm/tags';
 import { osmJoinWays } from '../osm/multipolygon';
 import { geoPathIntersections } from '../geo';
-import { utilArrayGroupBy, utilArrayIdentical, utilArrayIntersection } from '../util';
+import { utilArrayGroupBy, utilArrayIdentical, utilArrayIntersection, utilOldestID } from '../util';
 
 
 // Join ways at the end node they share.
@@ -28,6 +28,11 @@ export function actionJoin(ids) {
     var action = function(graph) {
         var ways = ids.map(graph.entity, graph);
 
+        // Prefer to keep an existing way.
+        // if there are multiple existing ways, keep the oldest one
+        // the oldest way is determined by the ID of the way.
+        var survivorID = utilOldestID(ways.map(way => way.id));
+
         // if any of the ways are sided (e.g. coastline, cliff, kerb)
         // sort them first so they establish the overall order - #6033
         ways.sort(function(a, b) {
@@ -38,17 +43,6 @@ export function actionJoin(ids) {
                 : 0;
         });
 
-        // Prefer to keep an existing way.
-        // if there are multiple existing ways, keep the oldest one
-        // the oldest way is determined by the ID of the way
-        const survivorID = (
-            ways
-                .filter((way) => !way.isNew())
-                .sort((a, b) => +a.osmId() - +b.osmId())[0] || ways[0]
-        ).id;
-
-
-
         var sequences = osmJoinWays(ways, graph);
         var joined = sequences[0];
 

modules/actions/merge.js

@@ -1,5 +1,6 @@
+import { osmEntity } from '../osm';
 import { osmTagSuggestingArea } from '../osm/tags';
-import { utilArrayGroupBy, utilArrayUniq } from '../util';
+import { utilArrayGroupBy, utilArrayUniq, utilCompareIDs } from '../util';
 
 
 export function actionMerge(ids) {
@@ -29,21 +30,65 @@ export function actionMerge(ids) {
             var nodes = utilArrayUniq(graph.childNodes(target));
             var removeNode = point;
 
-            for (var i = 0; i < nodes.length; i++) {
-                var node = nodes[i];
-                if (graph.parentWays(node).length > 1 ||
-                    graph.parentRelations(node).length ||
-                    node.hasInterestingTags()) {
-                    continue;
+            if (!point.isNew()) {
+                // Try to preserve the original point if it already has
+                // an ID in the database.
+
+                var inserted = false;
+
+                var canBeReplaced = function(node) {
+                    return !(graph.parentWays(node).length > 1 ||
+                        graph.parentRelations(node).length);
+                };
+
+                var replaceNode = function(node) {
+                    graph = graph.replace(point.update({ tags: node.tags, loc: node.loc }));
+                    target = target.replaceNode(node.id, point.id);
+                    graph = graph.replace(target);
+                    removeNode = node;
+                    inserted = true;
+                };
+
+                var i;
+                var node;
+
+                // First, try to replace a new child node on the target way.
+                for (i = 0; i < nodes.length; i++) {
+                    node = nodes[i];
+                    if (canBeReplaced(node) && node.isNew()) {
+                        replaceNode(node);
+                        break;
+                    }
                 }
 
-                // Found an uninteresting child node on the target way.
-                // Move orig point into its place to preserve point's history. #3683
-                graph = graph.replace(point.update({ tags: {}, loc: node.loc }));
-                target = target.replaceNode(node.id, point.id);
-                graph = graph.replace(target);
-                removeNode = node;
-                break;
+                if (!inserted && point.hasInterestingTags()) {
+                    // No new child node found, try to find an existing, but
+                    // uninteresting child node instead.
+                    for (i = 0; i < nodes.length; i++) {
+                        node = nodes[i];
+                        if (canBeReplaced(node) &&
+                            !node.hasInterestingTags()) {
+                            replaceNode(node);
+                            break;
+                        }
+                    }
+
+                    if (!inserted) {
+                        // Still not inserted, try to find an existing, interesting,
+                        // but more recent child node.
+                        for (i = 0; i < nodes.length; i++) {
+                            node = nodes[i];
+                            if (canBeReplaced(node) &&
+                                utilCompareIDs(point.id, node.id) < 0) {
+                                replaceNode(node);
+                                break;
+                            }
+                        }
+                    }
+
+                    // If the point still hasn't been inserted, we give up.
+                    // There are more interesting or older nodes on the way.
+                }
             }
 
             graph = graph.remove(removeNode);

modules/actions/merge_polygon.js

@@ -1,6 +1,6 @@
 import { geoPolygonContainsPolygon } from '../geo';
 import { osmJoinWays, osmRelation } from '../osm';
-import { utilArrayGroupBy, utilArrayIntersection, utilObjectOmit } from '../util';
+import { utilArrayGroupBy, utilArrayIntersection, utilObjectOmit, utilOldestID } from '../util';
 
 
 export function actionMergePolygon(ids, newRelationId) {
@@ -85,13 +85,21 @@ export function actionMergePolygon(ids, newRelationId) {
             outer = !outer;
         }
 
-        // Move all tags to one relation
-        var relation = entities.multipolygon[0] ||
-            osmRelation({ id: newRelationId, tags: { type: 'multipolygon' }});
+        // Move all tags to one relation.
+        // Keep the oldest multipolygon alive if it exists.
+        var relation;
+        if (entities.multipolygon.length > 0) {
+            var oldestID = utilOldestID(entities.multipolygon.map((entity) => entity.id));
+            relation = entities.multipolygon.find((entity) => entity.id === oldestID);
+        } else {
+            relation = osmRelation({ id: newRelationId, tags: { type: 'multipolygon' }});
+        }
 
-        entities.multipolygon.slice(1).forEach(function(m) {
-            relation = relation.mergeTags(m.tags);
-            graph = graph.remove(m);
+        entities.multipolygon.forEach(function(m) {
+            if (m.id !== relation.id) {
+                relation = relation.mergeTags(m.tags);
+                graph = graph.remove(m);
+            }
         });
 
         entities.closedWay.forEach(function(way) {

modules/osm/entity.js

@@ -36,7 +36,11 @@ osmEntity.id.fromOSM = function(type, id) {
 
 
 osmEntity.id.toOSM = function(id) {
-    return id.slice(1);
+    var match = id.match(/^[cnwr](-?\d+)$/);
+    if (match) {
+        return match[1];
+    }
+    return '';
 };
 
 
@@ -129,7 +133,8 @@ osmEntity.prototype = {
 
 
     isNew: function() {
-        return this.osmId() < 0;
+        var osmId = osmEntity.id.toOSM(this.id);
+        return osmId.length === 0 || osmId[0] === '-';
     },
 
 

modules/util/index.js

@@ -35,6 +35,8 @@ export { utilHighlightEntities } from './util';
 export { utilKeybinding } from './keybinding';
 export { utilNoAuto } from './util';
 export { utilObjectOmit } from './object';
+export { utilCompareIDs } from './util';
+export { utilOldestID } from './util';
 export { utilPrefixCSSProperty } from './util';
 export { utilPrefixDOMProperty } from './util';
 export { utilQsString } from './util';

modules/util/util.js

@@ -581,3 +581,51 @@ export function utilUnicodeCharsCount(str) {
 export function utilUnicodeCharsTruncated(str, limit) {
     return Array.from(str).slice(0, limit).join('');
 }
+
+function toNumericID(id) {
+    var match = id.match(/^[cnwr](-?\d+)$/);
+    if (match) {
+        return parseInt(match[1], 10);
+    }
+    return NaN;
+}
+
+function compareNumericIDs(left, right) {
+    if (isNaN(left) && isNaN(right)) return -1;
+    if (isNaN(left)) return 1;
+    if (isNaN(right)) return -1;
+    if (Math.sign(left) !== Math.sign(right)) return -Math.sign(left);
+    if (Math.sign(left) < 0) return Math.sign(right - left);
+    return Math.sign(left - right);
+}
+
+// Returns -1 if the first parameter ID is older than the second,
+// 1 if the second parameter is older, 0 if they are the same.
+// If both IDs are test IDs, the function returns -1.
+export function utilCompareIDs(left, right) {
+    return compareNumericIDs(toNumericID(left), toNumericID(right));
+}
+
+// Returns the chronologically oldest ID in the list.
+// Database IDs (with positive numbers) before editor ones (with negative numbers).
+// Among each category, the closest number to 0 is the oldest.
+// Test IDs (any string that does not conform to OSM's ID scheme) are taken last.
+export function utilOldestID(ids) {
+    if (ids.length === 0) {
+        return undefined;
+    }
+
+    var oldestIDIndex = 0;
+    var oldestID = toNumericID(ids[0]);
+
+    for (var i = 1; i < ids.length; i++) {
+        var num = toNumericID(ids[i]);
+
+        if (compareNumericIDs(oldestID, num) === 1) {
+            oldestIDIndex = i;
+            oldestID = num;
+        }
+    }
+
+    return ids[oldestIDIndex];
+}

test/spec/actions/add_member.js

@@ -148,6 +148,35 @@ describe('iD.actionAddMember', function() {
             expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']);
         });
 
+        it('inserts the member multiple times if insertPair provided (middle) (reversed pair)', function() {
+            // Before:  a  ..  b ===> c ~~~> d <~~~ c <=== b  ..  a
+            // After:   a ---> b ===> c ~~~> d <~~~ c <=== b <--- a
+            var graph = iD.coreGraph([
+                iD.osmNode({id: 'a', loc: [0, 0]}),
+                iD.osmNode({id: 'b', loc: [0, 0]}),
+                iD.osmNode({id: 'c', loc: [0, 0]}),
+                iD.osmNode({id: 'd', loc: [0, 0]}),
+                iD.osmWay({id: '-', nodes: ['a', 'b']}),
+                iD.osmWay({id: '=', nodes: ['b', 'c']}),
+                iD.osmWay({id: '~', nodes: ['c', 'd']}),
+                iD.osmRelation({id: 'r', members: [
+                    {id: '=', type: 'way'},
+                    {id: '~', type: 'way'},
+                    {id: '~', type: 'way'},
+                    {id: '=', type: 'way'}
+                ]})
+            ]);
+
+            var member = { id: '=', type: 'way' };
+            var insertPair = {
+                originalID: '=',
+                insertedID: '-',
+                nodes: ['a','b','c']
+            };
+            graph = iD.actionAddMember('r', member, undefined, insertPair)(graph);
+            expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']);
+        });
+
         it('inserts the member multiple times if insertPair provided (beginning/end)', function() {
             // Before:         b <=== c ~~~> d <~~~ c ===> b
             // After:   a <--- b <=== c ~~~> d <~~~ c ===> b ---> a
@@ -177,6 +206,35 @@ describe('iD.actionAddMember', function() {
             expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']);
         });
 
+        it('inserts the member multiple times if insertPair provided (beginning/end) (reversed pair)', function() {
+            // Before:  a <--- b  ..  c ~~~> d <~~~ c  ..  b ---> a
+            // After:   a <--- b <=== c ~~~> d <~~~ c ===> b ---> a
+            var graph = iD.coreGraph([
+                iD.osmNode({id: 'a', loc: [0, 0]}),
+                iD.osmNode({id: 'b', loc: [0, 0]}),
+                iD.osmNode({id: 'c', loc: [0, 0]}),
+                iD.osmNode({id: 'd', loc: [0, 0]}),
+                iD.osmWay({id: '-', nodes: ['b', 'a']}),
+                iD.osmWay({id: '=', nodes: ['c', 'b']}),
+                iD.osmWay({id: '~', nodes: ['c', 'd']}),
+                iD.osmRelation({id: 'r', members: [
+                    {id: '-', type: 'way'},
+                    {id: '~', type: 'way'},
+                    {id: '~', type: 'way'},
+                    {id: '-', type: 'way'}
+                ]})
+            ]);
+
+            var member = { id: '-', type: 'way' };
+            var insertPair = {
+                originalID: '-',
+                insertedID: '=',
+                nodes: ['c','b','a']
+            };
+            graph = iD.actionAddMember('r', member, undefined, insertPair)(graph);
+            expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']);
+        });
+
         it('keeps stops and platforms ordered before node, way, relation (for PTv2 routes)', function() {
             var graph = iD.coreGraph([
                 iD.osmNode({id: 'a', loc: [0, 0]}),

test/spec/actions/connect.js

@@ -1,28 +1,85 @@
 describe('iD.actionConnect', function() {
-    it('chooses the first non-new node as the survivor', function() {
+    it('merges tags', function() {
         var graph = iD.coreGraph([
-            iD.osmNode({id: 'a'}),
-            iD.osmNode({id: 'b', version: '1'}),
-            iD.osmNode({id: 'c', version: '1'})
+            iD.osmNode({id: 'a', tags: { highway: 'traffic_signals' }}),
+            iD.osmNode({id: 'b', tags: { crossing: 'marked' }}),
         ]);
 
-        graph = iD.actionConnect(['a', 'b', 'c'])(graph);
+        graph = iD.actionConnect(['a', 'b'])(graph);
         expect(graph.hasEntity('a')).not.to.be.ok;
-        expect(graph.hasEntity('b')).to.be.ok;
-        expect(graph.hasEntity('c')).not.to.be.ok;
+
+        var survivor = graph.hasEntity('b');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+        expect(survivor.tags).to.eql({ highway: 'traffic_signals', crossing: 'marked' }, 'merge all tags');
+    });
+
+    it('chooses the oldest node as the survivor', function() {
+        var graph = iD.coreGraph([
+            iD.osmNode({id: 'n3'}),
+            iD.osmNode({id: 'n-1'}),
+            iD.osmNode({id: 'n2'}),
+            iD.osmNode({id: 'n4'})
+        ]);
+
+        graph = iD.actionConnect(['n3', 'n-1', 'n2', 'n4'])(graph);
+        expect(graph.hasEntity('n3')).not.to.be.ok;
+        expect(graph.hasEntity('n-1')).not.to.be.ok;
+        expect(graph.hasEntity('n2')).to.be.ok;
+        expect(graph.hasEntity('n4')).not.to.be.ok;
+    });
+
+    it('chooses the oldest interesting node as the survivor', function() {
+        var graph = iD.coreGraph([
+            iD.osmNode({id: 'n3'}),
+            iD.osmNode({id: 'n1'}),
+            iD.osmNode({id: 'n2', tags: { highway: 'traffic_signals' }}),
+            iD.osmNode({id: 'n4', tags: { crossing: 'marked' }})
+        ]);
+
+        graph = iD.actionConnect(['n3', 'n1', 'n2', 'n4'])(graph);
+
+        expect(graph.hasEntity('n3')).not.to.be.ok;
+        expect(graph.hasEntity('n1')).not.to.be.ok;
+        expect(graph.hasEntity('n4')).not.to.be.ok;
+
+        var survivor = graph.hasEntity('n2');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+        expect(survivor.tags).to.eql({ highway: 'traffic_signals', crossing: 'marked' }, 'merge all tags');
+    });
+
+    it('chooses an existing node as the survivor', function() {
+        var graph = iD.coreGraph([
+            iD.osmNode({id: 'n3'}),
+            iD.osmNode({id: 'n-1'}),
+            iD.osmNode({id: 'n-2', tags: { highway: 'traffic_signals' }}),
+            iD.osmNode({id: 'n-4', tags: { crossing: 'marked' }})
+        ]);
+
+        graph = iD.actionConnect(['n3', 'n-1', 'n-2', 'n-4'])(graph);
+
+        expect(graph.hasEntity('n-1')).not.to.be.ok;
+        expect(graph.hasEntity('n-2')).not.to.be.ok;
+        expect(graph.hasEntity('n-4')).not.to.be.ok;
+
+        var survivor = graph.hasEntity('n3');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+        expect(survivor.tags).to.eql({ highway: 'traffic_signals', crossing: 'marked' }, 'merge all tags');
     });
 
     it('chooses the last node as the survivor when all are new', function() {
         var graph = iD.coreGraph([
-            iD.osmNode({id: 'a'}),
-            iD.osmNode({id: 'b'}),
+            iD.osmNode({id: 'a', tags: { highway: 'traffic_signals' }}),
+            iD.osmNode({id: 'b', tags: { crossing: 'marked' }}),
             iD.osmNode({id: 'c'})
         ]);
 
         graph = iD.actionConnect(['a', 'b', 'c'])(graph);
         expect(graph.hasEntity('a')).not.to.be.ok;
         expect(graph.hasEntity('b')).not.to.be.ok;
-        expect(graph.hasEntity('c')).to.be.ok;
+
+        var survivor = graph.hasEntity('c');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+        expect(survivor.tags).to.eql({ highway: 'traffic_signals', crossing: 'marked' }, 'merge all tags');
     });
 
 

test/spec/actions/join.js

@@ -402,7 +402,7 @@ describe('iD.actionJoin', function () {
         expect(graph.entity('-').tags).to.eql({'lanes:backward': 2});
     });
 
-    it('prefers to keep existing ways', function () {
+    it('keeps the way already in the database', function () {
         // a --> b ==> c ++> d
         // --- is new, === is existing, +++ is new
         // Expected result:
@@ -447,6 +447,60 @@ describe('iD.actionJoin', function () {
         expect(graph.hasEntity('w-1')).to.be.undefined;
     });
 
+    it('keeps the oldest id - oldest first', function () {
+        var graph = iD.coreGraph([
+            iD.osmNode({id: 'a', loc: [0,0]}),
+            iD.osmNode({id: 'b', loc: [2,0]}),
+            iD.osmNode({id: 'c', loc: [4,0]}),
+            iD.osmNode({id: 'd', loc: [6,0]}),
+            iD.osmWay({id: 'w1', nodes: ['a', 'b']}),
+            iD.osmWay({id: 'w2', nodes: ['b', 'c']}),
+            iD.osmWay({id: 'w3', nodes: ['c', 'd']})
+        ]);
+
+        graph = iD.actionJoin(['w1', 'w2', 'w3'])(graph);
+
+        expect(graph.entity('w1').nodes).to.eql(['a', 'b', 'c', 'd']);
+        expect(graph.hasEntity('w2')).to.be.undefined;
+        expect(graph.hasEntity('w3')).to.be.undefined;
+    });
+
+    it('keeps the oldest id - oldest last', function () {
+        var graph = iD.coreGraph([
+            iD.osmNode({id: 'a', loc: [0,0]}),
+            iD.osmNode({id: 'b', loc: [2,0]}),
+            iD.osmNode({id: 'c', loc: [4,0]}),
+            iD.osmNode({id: 'd', loc: [6,0]}),
+            iD.osmWay({id: 'w3', nodes: ['a', 'b']}),
+            iD.osmWay({id: 'w2', nodes: ['b', 'c']}),
+            iD.osmWay({id: 'w1', nodes: ['c', 'd']})
+        ]);
+
+        graph = iD.actionJoin(['w3', 'w2', 'w1'])(graph);
+
+        expect(graph.entity('w1').nodes).to.eql(['a', 'b', 'c', 'd']);
+        expect(graph.hasEntity('w2')).to.be.undefined;
+        expect(graph.hasEntity('w3')).to.be.undefined;
+    });
+
+    it('keeps the oldest id - oldest middle', function () {
+        var graph = iD.coreGraph([
+            iD.osmNode({id: 'a', loc: [0,0]}),
+            iD.osmNode({id: 'b', loc: [2,0]}),
+            iD.osmNode({id: 'c', loc: [4,0]}),
+            iD.osmNode({id: 'd', loc: [6,0]}),
+            iD.osmWay({id: 'w2', nodes: ['a', 'b']}),
+            iD.osmWay({id: 'w1', nodes: ['b', 'c']}),
+            iD.osmWay({id: 'w3', nodes: ['c', 'd']})
+        ]);
+
+        graph = iD.actionJoin(['w2', 'w1', 'w3'])(graph);
+
+        expect(graph.entity('w1').nodes).to.eql(['a', 'b', 'c', 'd']);
+        expect(graph.hasEntity('w2')).to.be.undefined;
+        expect(graph.hasEntity('w3')).to.be.undefined;
+    });
+
     it('merges tags', function () {
         var graph = iD.coreGraph([
             iD.osmNode({id: 'a', loc: [0,0]}),
@@ -489,7 +543,7 @@ describe('iD.actionJoin', function () {
         //            v v v
         //
         //  Expected result:
-        // a =====> b =====> c
+        // a -----> b -----> c
         //   v v v    v v v
         //
         var graph = iD.coreGraph([
@@ -500,8 +554,8 @@ describe('iD.actionJoin', function () {
             iD.osmWay({id: '=', nodes: ['b', 'c'], tags: { natural: 'cliff' }})
         ]);
         graph = iD.actionJoin(['-', '='])(graph);
-        expect(graph.entity('=').nodes).to.eql(['a', 'b', 'c']);
-        expect(graph.entity('=').tags).to.eql({ natural: 'cliff' });
+        expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']);
+        expect(graph.entity('-').tags).to.eql({ natural: 'cliff' });
     });
 
     it('preserves sidedness of start segment, contra-directional lines', function () {
@@ -529,7 +583,7 @@ describe('iD.actionJoin', function () {
         //             v v v
         //
         //  Expected result:
-        // a <===== b <===== c
+        // a <----- b <----- c
         //    v v v    v v v
         //
         var graph = iD.coreGraph([
@@ -540,8 +594,8 @@ describe('iD.actionJoin', function () {
             iD.osmWay({id: '=', nodes: ['c', 'b'], tags: { natural: 'cliff' }})
         ]);
         graph = iD.actionJoin(['-', '='])(graph);
-        expect(graph.entity('=').nodes).to.eql(['c', 'b', 'a']);
-        expect(graph.entity('=').tags).to.eql({ natural: 'cliff' });
+        expect(graph.entity('-').nodes).to.eql(['c', 'b', 'a']);
+        expect(graph.entity('-').tags).to.eql({ natural: 'cliff' });
     });
 
 

test/spec/actions/merge.js

@@ -37,22 +37,115 @@ describe('iD.actionMerge', function () {
         expect(graph.entity('r').members).to.eql([{id: 'w', role: 'r', type: 'way'}]);
     });
 
-    it('preserves original point if possible', function () {
+    it('preserves existing point id when possible', function () {
         var graph = iD.coreGraph([
-                iD.osmNode({id: 'a', loc: [1, 0], tags: {a: 'a'}}),
-                iD.osmNode({id: 'p', loc: [0, 0], tags: {p: 'p'}}),
-                iD.osmNode({id: 'q', loc: [0, 1]}),
-                iD.osmWay({id: 'w', nodes: ['p', 'q'], tags: {w: 'w'}})
+                iD.osmNode({id: 'n1', loc: [1, 0], tags: {n1: 'n1'}}),
+                iD.osmNode({id: 'a', loc: [0, 0], tags: {a: 'a'}}),
+                iD.osmNode({id: 'b', loc: [0, 1]}),
+                iD.osmWay({id: 'w', nodes: ['a', 'b'], tags: {w: 'w'}})
             ]),
-            action = iD.actionMerge(['a', 'w']);
+            action = iD.actionMerge(['n1', 'w']);
 
         graph = action(graph);
-        expect(graph.hasEntity('a')).to.be.ok;
-        expect(graph.hasEntity('p')).to.be.ok;
-        expect(graph.hasEntity('q')).to.be.undefined;
+        expect(graph.hasEntity('n1')).to.be.ok;
+        expect(graph.hasEntity('a')).to.be.undefined;
+        expect(graph.hasEntity('b')).to.be.ok;
+        expect(graph.entity('w').tags).to.eql({n1: 'n1', w: 'w'});
+        expect(graph.entity('w').nodes).to.eql(['n1', 'b']);
+        expect(graph.entity('n1').loc[0]).to.eql(0);
+        expect(graph.entity('n1').loc[1]).to.eql(0);
+    });
+
+    it('preserves existing point ids when possible', function () {
+        var graph = iD.coreGraph([
+                iD.osmNode({id: 'n1', loc: [1, 0], tags: {n1: 'n1'}}),
+                iD.osmNode({id: 'n2', loc: [2, 0], tags: {n2: 'n2'}}),
+                iD.osmNode({id: 'a', loc: [0, 1]}),
+                iD.osmNode({id: 'b', loc: [0, 2], tags: {b: 'b'}}),
+                iD.osmNode({id: 'c', loc: [0, 3]}),
+                iD.osmWay({id: 'w', nodes: ['a', 'b', 'c'], tags: {w: 'w'}})
+            ]),
+            action = iD.actionMerge(['n1', 'n2', 'w']);
+
+        graph = action(graph);
+        expect(graph.hasEntity('n1')).to.be.ok;
+        expect(graph.hasEntity('n2')).to.be.ok;
+        expect(graph.hasEntity('a')).to.be.undefined;
+        expect(graph.hasEntity('b')).to.be.undefined;
+        expect(graph.hasEntity('c')).to.be.ok;
+        expect(graph.entity('n2').tags).to.eql({b: 'b'});
+        expect(graph.entity('w').tags).to.eql({n1: 'n1', n2: 'n2', w: 'w'});
+        expect(graph.entity('w').nodes).to.eql(['n1', 'n2', 'c']);
+        expect(graph.entity('n1').loc[0]).to.eql(0);
+        expect(graph.entity('n1').loc[1]).to.eql(1);
+        expect(graph.entity('n2').loc[0]).to.eql(0);
+        expect(graph.entity('n2').loc[1]).to.eql(2);
+    });
+
+    it('preserves existing node ids when possible', function () {
+        var graph = iD.coreGraph([
+                iD.osmNode({id: 'a', loc: [1, 0], tags: {a: 'a'}}),
+                iD.osmNode({id: 'b', loc: [2, 0]}),
+                iD.osmNode({id: 'n1', loc: [0, 1]}),
+                iD.osmNode({id: 'n2', loc: [0, 2], tags: {n2: 'n2'}}),
+                iD.osmWay({id: 'w', nodes: ['n1', 'n2'], tags: {w: 'w'}})
+            ]),
+            action = iD.actionMerge(['a', 'b', 'w']);
+
+        graph = action(graph);
+        expect(graph.hasEntity('a')).to.be.undefined;
+        expect(graph.hasEntity('b')).to.be.undefined;
+        expect(graph.hasEntity('n1')).to.be.ok;
+        expect(graph.hasEntity('n2')).to.be.ok;
         expect(graph.entity('w').tags).to.eql({a: 'a', w: 'w'});
-        expect(graph.entity('w').nodes).to.eql(['p', 'a']);
-        expect(graph.entity('a').loc[0]).to.eql(0);
-        expect(graph.entity('a').loc[1]).to.eql(1);
+        expect(graph.entity('w').nodes).to.eql(['n1', 'n2']);
+        expect(graph.entity('n1').loc[0]).to.eql(0);
+        expect(graph.entity('n1').loc[1]).to.eql(1);
+        expect(graph.entity('n2').loc[0]).to.eql(0);
+        expect(graph.entity('n2').loc[1]).to.eql(2);
+    });
+
+    it('preserves interesting existing node ids when possible', function () {
+        var graph = iD.coreGraph([
+                iD.osmNode({id: 'n1', loc: [1, 0], tags: {n1: 'n1'}}),
+                iD.osmNode({id: 'n2', loc: [0, 1], tags: {n2: 'n2'}}),
+                iD.osmNode({id: 'n3', loc: [0, 2]}),
+                iD.osmWay({id: 'w', nodes: ['n2', 'n3'], tags: {w: 'w'}})
+            ]),
+            action = iD.actionMerge(['n1', 'w']);
+
+        graph = action(graph);
+        expect(graph.hasEntity('n1')).to.be.ok;
+        expect(graph.hasEntity('n2')).to.be.ok;
+        expect(graph.hasEntity('n3')).to.be.undefined;
+        expect(graph.entity('w').tags).to.eql({n1: 'n1', w: 'w'});
+        expect(graph.entity('w').nodes).to.eql(['n2', 'n1']);
+        expect(graph.entity('n1').loc[0]).to.eql(0);
+        expect(graph.entity('n1').loc[1]).to.eql(2);
+    });
+
+    it('preserves oldest interesting existing node ids', function () {
+        var graph = iD.coreGraph([
+                iD.osmNode({id: 'n3', loc: [1, 0], tags: {n3: 'n3'}}),
+                iD.osmNode({id: 'n6', loc: [2, 0], tags: {n6: 'n6'}}),
+                iD.osmNode({id: 'n2', loc: [0, 1], tags: {n2: 'n2'}}),
+                iD.osmNode({id: 'n5', loc: [0, 2], tags: {n5: 'n5'}}),
+                iD.osmNode({id: 'n1', loc: [0, 3], tags: {n1: 'n1'}}),
+                iD.osmNode({id: 'n4', loc: [0, 4], tags: {n4: 'n4'}}),
+                iD.osmWay({id: 'w', nodes: ['n2', 'n5', 'n1', 'n4'], tags: {w: 'w'}})
+            ]),
+            action = iD.actionMerge(['n3', 'n6', 'w']);
+
+        graph = action(graph);
+        expect(graph.hasEntity('n1')).to.be.ok;
+        expect(graph.hasEntity('n2')).to.be.ok;
+        expect(graph.hasEntity('n3')).to.be.ok;
+        expect(graph.hasEntity('n4')).to.be.ok;
+        expect(graph.hasEntity('n5')).to.be.undefined;
+        expect(graph.hasEntity('n6')).to.be.undefined;
+        expect(graph.entity('w').tags).to.eql({n3: 'n3', n6: 'n6', w: 'w'});
+        expect(graph.entity('w').nodes).to.eql(['n2', 'n3', 'n1', 'n4']);
+        expect(graph.entity('n3').loc[0]).to.eql(0);
+        expect(graph.entity('n3').loc[1]).to.eql(2);
     });
 });

test/spec/actions/merge_nodes.js

@@ -72,9 +72,79 @@ describe('iD.actionMergeNodes', function () {
     });
 
 
+    it('keeps the id of the interesting node', function() {
+        var graph = iD.coreGraph([
+            iD.osmNode({ id: 'n1', loc: [0, 0] }),
+            iD.osmNode({ id: 'n2', loc: [4, 4], tags: { highway: 'traffic_signals' }})
+        ]);
+
+        graph = iD.actionMergeNodes(['n1', 'n2'])(graph);
+
+        expect(graph.hasEntity('n1')).to.be.undefined;
+
+        var survivor = graph.hasEntity('n2');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+        expect(survivor.tags).to.eql({ highway: 'traffic_signals' }, 'merge all tags');
+        expect(survivor.loc).to.eql([4, 4], 'use loc of interesting node');
+    });
+
+
+    it('keeps the id of the existing node', function() {
+        var graph = iD.coreGraph([
+            iD.osmNode({ id: 'n1', loc: [0, 0] }),
+            iD.osmNode({ id: 'b', loc: [4, 4], tags: { highway: 'traffic_signals' }})
+        ]);
+
+        graph = iD.actionMergeNodes(['n1', 'b'])(graph);
+
+        expect(graph.hasEntity('b')).to.be.undefined;
+
+        var survivor = graph.hasEntity('n1');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+        expect(survivor.tags).to.eql({ highway: 'traffic_signals' }, 'merge all tags');
+        expect(survivor.loc).to.eql([4, 4], 'use loc of interesting node');
+    });
+
+
+    it('keeps the id of the oldest node', function() {
+        var graph = iD.coreGraph([
+            iD.osmNode({ id: 'n2', loc: [0, 0] }),
+            iD.osmNode({ id: 'n1', loc: [2, 2] }),
+            iD.osmNode({ id: 'n3', loc: [4, 4] })
+        ]);
+
+        graph = iD.actionMergeNodes(['n2', 'n1', 'n3'])(graph);
+
+        expect(graph.hasEntity('n2')).to.be.undefined;
+        expect(graph.hasEntity('n3')).to.be.undefined;
+
+        var survivor = graph.hasEntity('n1');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+    });
+
+
+    it('keeps the id of the oldest interesting node', function() {
+        var graph = iD.coreGraph([
+            iD.osmNode({ id: 'n3', loc: [0, 0] }),
+            iD.osmNode({ id: 'n1', loc: [2, 2] }),
+            iD.osmNode({ id: 'n2', loc: [4, 4], tags: { highway: 'traffic_signals' }}),
+            iD.osmNode({ id: 'n4', loc: [8, 8], tags: { crossing: 'marked' }})
+        ]);
+
+        graph = iD.actionMergeNodes(['n2', 'n1', 'n3', 'n4'])(graph);
+
+        expect(graph.hasEntity('n1')).to.be.undefined;
+        expect(graph.hasEntity('n3')).to.be.undefined;
+        expect(graph.hasEntity('n4')).to.be.undefined;
+
+        var survivor = graph.hasEntity('n2');
+        expect(survivor).to.be.an.instanceof(iD.osmNode);
+    });
+
+
     it('merges two nodes along a single way', function() {
         //
-        //  scenerio:         merge b,c:
+        //  scenario:         merge b,c:
         //
         //  a -- b -- c       a ---- c
         //
@@ -98,7 +168,7 @@ describe('iD.actionMergeNodes', function () {
 
     it('merges two nodes from two ways', function() {
         //
-        //  scenerio:        merge b,d:
+        //  scenario:        merge b,d:
         //
         //  a -- b -- c      a -_   _- c
         //                        d
@@ -129,7 +199,7 @@ describe('iD.actionMergeNodes', function () {
 
     it('merges three nodes from three ways', function () {
         //
-        //  scenerio:        merge b,d:
+        //  scenario:        merge b,d,e:
         //
         //        c                c
         //        |                |

test/spec/actions/merge_polygon.js

@@ -68,15 +68,15 @@ describe('iD.actionMergePolygon', function () {
         expect(r.members.length).to.equal(3);
     });
 
-    it('creates a multipolygon from two multipolygon relations', function() {
-        graph = iD.actionMergePolygon(['w0', 'w1'], 'r')(graph);
-        graph = iD.actionMergePolygon(['w2', 'w5'], 'r2')(graph);
-        graph = iD.actionMergePolygon(['r', 'r2'])(graph);
+    it('creates a multipolygon from two multipolygon relations and keeps the oldest alive', function() {
+        graph = iD.actionMergePolygon(['w0', 'w1'], 'r2')(graph);
+        graph = iD.actionMergePolygon(['w2', 'w5'], 'r1')(graph);
+        graph = iD.actionMergePolygon(['r2', 'r1'])(graph);
 
         // Delete other relation
         expect(graph.hasEntity('r2')).to.equal(undefined);
 
-        var r = graph.entity('r');
+        var r = graph.entity('r1');
         expect(find(r, 'w0').role).to.equal('outer');
         expect(find(r, 'w1').role).to.equal('inner');
         expect(find(r, 'w2').role).to.equal('outer');

test/spec/osm/entity.js

@@ -32,6 +32,11 @@ describe('iD.osmEntity', function () {
         describe('.toOSM', function () {
             it('reverses fromOSM', function () {
                 expect(iD.osmEntity.id.toOSM(iD.osmEntity.id.fromOSM('node', '1'))).to.equal('1');
+                expect(iD.osmEntity.id.toOSM(iD.osmEntity.id.fromOSM('node', '-1'))).to.equal('-1');
+            });
+
+            it('returns the empty string for other strings', function () {
+                expect(iD.osmEntity.id.toOSM('a')).to.equal('');
             });
         });
     });

test/spec/spec_helpers.js

@@ -131,6 +131,14 @@ if (typeof ArrayBuffer.isView === 'undefined') {
     ArrayBuffer.isView = function() { return false; };
 }
 
+// Polyfill for `Math.sign()` in PhantomJS
+// From https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/sign#Polyfill
+if (!Math.sign) {
+    Math.sign = function(x) {
+        return ((x > 0) - (x < 0)) || +x;
+    };
+}
+
 // Add support for sinon-stubbing `fetch` API
 // (sinon fakeServer works only on `XMLHttpRequest`)
 // see https://github.com/sinonjs/nise/issues/7

test/spec/util/util.js

@@ -226,6 +226,38 @@ describe('iD.util', function() {
         });
     });
 
+    describe('utilCompareIDs', function() {
+        it('sorts existing IDs numerically in ascending order', function() {
+            expect(iD.utilCompareIDs('w100', 'w200')).to.eql(-1);
+            expect(iD.utilCompareIDs('w100', 'w50')).to.eql(1);
+            expect(iD.utilCompareIDs('w100', 'w100')).to.eql(0);
+        });
+        it('sorts new IDs numerically in descending order', function() {
+            expect(iD.utilCompareIDs('w-100', 'w-200')).to.eql(-1);
+            expect(iD.utilCompareIDs('w-100', 'w-50')).to.eql(1);
+            expect(iD.utilCompareIDs('w-100', 'w-100')).to.eql(0);
+        });
+        it('sorts existing IDs before new IDs', function() {
+            expect(iD.utilCompareIDs('w-1', 'w1')).to.eql(1);
+            expect(iD.utilCompareIDs('w1', 'w-1')).to.eql(-1);
+            expect(iD.utilCompareIDs('w-100', 'w1')).to.eql(1);
+            expect(iD.utilCompareIDs('w100', 'w-1')).to.eql(-1);
+            expect(iD.utilCompareIDs('w-1', 'w100')).to.eql(1);
+            expect(iD.utilCompareIDs('w1', 'w-100')).to.eql(-1);
+        });
+        it('sorts existing and new IDs before anything else', function() {
+            expect(iD.utilCompareIDs('w1', 'asdf')).to.eql(-1);
+            expect(iD.utilCompareIDs('asdf', 'w1')).to.eql(1);
+            expect(iD.utilCompareIDs('w-1', 'asdf')).to.eql(-1);
+            expect(iD.utilCompareIDs('asdf', 'w-1')).to.eql(1);
+        });
+        it('returns -1 for other strings', function() {
+            expect(iD.utilCompareIDs('aaa', 'b')).to.eql(-1);
+            expect(iD.utilCompareIDs('b', 'aaa')).to.eql(-1);
+            expect(iD.utilCompareIDs('a', 'a')).to.eql(-1);
+        });
+    });
+
     describe('utilDisplayName', function() {
         it('returns the name if tagged with a name', function() {
             expect(iD.utilDisplayName({tags: {name: 'East Coast Greenway'}})).to.eql('East Coast Greenway');
@@ -252,4 +284,25 @@ describe('iD.util', function() {
             expect(iD.utilDisplayName({tags: {network: 'BART', ref: 'Yellow', from: 'Antioch', to: 'Millbrae', via: 'Pittsburg/Bay Point;San Francisco International Airport', route: 'subway'}})).to.eql('BART Yellow from Antioch to Millbrae via Pittsburg/Bay Point;San Francisco International Airport');
         });
     });
+
+    describe('utilOldestID', function() {
+        it('returns the oldest database ID', function() {
+            expect(iD.utilOldestID(['w3', 'w1', 'w2'])).to.eql('w1');
+        });
+        it('returns the oldest editor ID', function() {
+            expect(iD.utilOldestID(['w-3', 'w-2', 'w-1'])).to.eql('w-1');
+        });
+        it('returns the oldest IDs among database and editor IDs', function() {
+            expect(iD.utilOldestID(['w-1', 'w1', 'w-2'])).to.eql('w1');
+        });
+        it('returns the oldest database ID', function() {
+            expect(iD.utilOldestID(['w100', 'w-1', 'a', 'w-300', 'w2'])).to.eql('w2');
+        });
+        it('returns the oldest editor ID if no database IDs', function() {
+            expect(iD.utilOldestID(['w-100', 'w-1', 'a', 'w-300', 'w-2'])).to.eql('w-1');
+        });
+        it('returns the first ID in the list otherwise', function() {
+            expect(iD.utilOldestID(['z', 'a', 'A', 'Z'])).to.eql('z');
+        });
+    });
 });