From b09c712fc5cbd3225a653cf9662fa3429360609e Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 9 Apr 2018 14:20:40 -0400 Subject: [PATCH] When connecting nodes, prefer to keep an existing (not new) node (closes #4974, closes #4674) --- modules/actions/connect.js | 21 +++-- test/spec/actions/connect.js | 144 ++++++++++++++++++----------------- 2 files changed, 90 insertions(+), 75 deletions(-) diff --git a/modules/actions/connect.js b/modules/actions/connect.js index 1eb0bc5cc..df633715b 100644 --- a/modules/actions/connect.js +++ b/modules/actions/connect.js @@ -3,8 +3,8 @@ import { actionDeleteNode } from './delete_node'; // Connect the ways at the given nodes. // -// The last node will survive. All other nodes will be replaced with -// the surviving node in parent ways, and then removed. +// First choose a node to be the survivor, with preference given +// to an existing (not new) node. // // Tags and relation memberships of of non-surviving nodes are merged // to the survivor. @@ -17,11 +17,20 @@ import { actionDeleteNode } from './delete_node'; // export function actionConnect(nodeIds) { return function(graph) { - var last = nodeIds[nodeIds.length - 1]; - var survivor = graph.entity(last); + var survivor; + var node; + var i; - for (var i = 0; i < nodeIds.length - 1; i++) { - var node = graph.entity(nodeIds[i]); + // 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 + } + + // Replace all non-surviving nodes with the survivor and merge tags. + for (i = 0; i < nodeIds.length; i++) { + node = graph.entity(nodeIds[i]); + if (node.id === survivor.id) continue; /* eslint-disable no-loop-func */ graph.parentWays(node).forEach(function(parent) { diff --git a/test/spec/actions/connect.js b/test/spec/actions/connect.js index 066e4a73a..a9316c2c2 100644 --- a/test/spec/actions/connect.js +++ b/test/spec/actions/connect.js @@ -1,18 +1,31 @@ describe('iD.actionConnect', function() { - it('removes all but the final node', function() { - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Node({id: 'c'}) - ]); + it('chooses the first non-new node as the survivor', function() { + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b', version: '1'}), + iD.osmNode({id: 'c', version: '1'}) + ]); graph = iD.actionConnect(['a', 'b', 'c'])(graph); - - expect(graph.hasEntity('a')).to.be.undefined; - expect(graph.hasEntity('b')).to.be.undefined; - expect(graph.entity('c')).not.to.be.undefined; + expect(graph.hasEntity('a')).not.to.be.ok; + expect(graph.hasEntity('b')).to.be.ok; + expect(graph.hasEntity('c')).not.to.be.ok; }); + 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: '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; + }); + + it('replaces non-surviving nodes in parent ways', function() { // a --- b --- c // @@ -28,18 +41,17 @@ describe('iD.actionConnect', function() { // | // d // - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Node({id: 'c'}), - iD.Node({id: 'd'}), - iD.Node({id: 'e'}), - iD.Way({id: '-', nodes: ['a', 'b', 'c']}), - iD.Way({id: '|', nodes: ['d', 'e']}) - ]); + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'd'}), + iD.osmNode({id: 'e'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmWay({id: '|', nodes: ['d', 'e']}) + ]); graph = iD.actionConnect(['e', 'b'])(graph); - expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); expect(graph.entity('|').nodes).to.eql(['d', 'b']); }); @@ -53,18 +65,17 @@ describe('iD.actionConnect', function() { // // Connect [a, d]. // - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Node({id: 'c'}), - iD.Node({id: 'd'}), - iD.Node({id: 'e'}), - iD.Way({id: '-', nodes: ['a', 'b', 'c', 'a']}), - iD.Way({id: '=', nodes: ['d', 'e']}) - ]); + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'd'}), + iD.osmNode({id: 'e'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'a']}), + iD.osmWay({id: '=', nodes: ['d', 'e']}) + ]); graph = iD.actionConnect(['a', 'd'])(graph); - expect(graph.entity('-').nodes).to.eql(['d', 'b', 'c', 'd']); }); @@ -77,15 +88,14 @@ describe('iD.actionConnect', function() { // // a --- c // - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Node({id: 'c'}), - iD.Way({id: '-', nodes: ['a', 'b', 'c']}) - ]); + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}) + ]); graph = iD.actionConnect(['b', 'c'])(graph); - expect(graph.entity('-').nodes).to.eql(['a', 'c']); expect(graph.hasEntity('b')).to.be.undefined; }); @@ -103,17 +113,16 @@ describe('iD.actionConnect', function() { // | // d // - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Node({id: 'c'}), - iD.Node({id: 'c'}), - iD.Way({id: '-', nodes: ['a', 'b', 'c']}), - iD.Way({id: '|', nodes: ['b', 'd']}) - ]); + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'c'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmWay({id: '|', nodes: ['b', 'd']}) + ]); graph = iD.actionConnect(['b', 'c'])(graph); - expect(graph.entity('-').nodes).to.eql(['a', 'c']); expect(graph.entity('|').nodes).to.eql(['c', 'd']); expect(graph.hasEntity('b')).to.be.undefined; @@ -124,44 +133,41 @@ describe('iD.actionConnect', function() { // // Connect [a, b] // - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Way({id: '-', nodes: ['a', 'b']}) - ]); + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmWay({id: '-', nodes: ['a', 'b']}) + ]); graph = iD.actionConnect(['a', 'b'])(graph); - expect(graph.hasEntity('a')).to.be.undefined; expect(graph.hasEntity('-')).to.be.undefined; }); it('merges tags to the surviving node', function() { - var graph = iD.Graph([ - iD.Node({id: 'a', tags: {a: 'a'}}), - iD.Node({id: 'b', tags: {b: 'b'}}), - iD.Node({id: 'c', tags: {c: 'c'}}) - ]); + var graph = iD.coreGraph([ + iD.osmNode({id: 'a', tags: {a: 'a'}}), + iD.osmNode({id: 'b', tags: {b: 'b'}}), + iD.osmNode({id: 'c', tags: {c: 'c'}}) + ]); graph = iD.actionConnect(['a', 'b', 'c'])(graph); - expect(graph.entity('c').tags).to.eql({a: 'a', b: 'b', c: 'c'}); }); it('merges memberships to the surviving node', function() { - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Node({id: 'c'}), - iD.Node({id: 'c'}), - iD.Way({id: '-', nodes: ['a', 'b']}), - iD.Way({id: '=', nodes: ['c', 'd']}), - iD.Relation({id: 'r1', members: [{id: 'b', role: 'r1', type: 'node'}]}), - iD.Relation({id: 'r2', members: [{id: 'b', role: 'r2', type: 'node'}, {id: 'c', role: 'r2', type: 'node'}]}) - ]); + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'c'}), + iD.osmWay({id: '-', nodes: ['a', 'b']}), + iD.osmWay({id: '=', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r1', members: [{id: 'b', role: 'r1', type: 'node'}]}), + iD.osmRelation({id: 'r2', members: [{id: 'b', role: 'r2', type: 'node'}, {id: 'c', role: 'r2', type: 'node'}]}) + ]); graph = iD.actionConnect(['b', 'c'])(graph); - expect(graph.entity('r1').members).to.eql([{id: 'c', role: 'r1', type: 'node'}]); expect(graph.entity('r2').members).to.eql([{id: 'c', role: 'r2', type: 'node'}]); });