diff --git a/modules/actions/connect.js b/modules/actions/connect.js index 0746317ad..635d70812 100644 --- a/modules/actions/connect.js +++ b/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++) { diff --git a/test/spec/actions/connect.js b/test/spec/actions/connect.js index 61b7a5dc6..2d9ea9a85 100644 --- a/test/spec/actions/connect.js +++ b/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'); }); diff --git a/test/spec/actions/merge_nodes.js b/test/spec/actions/merge_nodes.js index 8d6c5460a..b437bebf0 100644 --- a/test/spec/actions/merge_nodes.js +++ b/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 // | |