From e1898e5c569dc34bdb34c30213bcd0f773b20450 Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Fri, 10 Apr 2020 23:08:48 +0200 Subject: [PATCH 1/8] Add the `utilOldestID()` function --- modules/util/index.js | 1 + modules/util/util.js | 50 +++++++++++++++++++++++++++++++++++++++ test/spec/spec_helpers.js | 8 +++++++ test/spec/util/util.js | 21 ++++++++++++++++ 4 files changed, 80 insertions(+) diff --git a/modules/util/index.js b/modules/util/index.js index 45e1266ef..26fb02c79 100644 --- a/modules/util/index.js +++ b/modules/util/index.js @@ -35,6 +35,7 @@ export { utilHighlightEntities } from './util'; export { utilKeybinding } from './keybinding'; export { utilNoAuto } from './util'; export { utilObjectOmit } from './object'; +export { utilOldestID } from './util'; export { utilPrefixCSSProperty } from './util'; export { utilPrefixDOMProperty } from './util'; export { utilQsString } from './util'; diff --git a/modules/util/util.js b/modules/util/util.js index e078647e6..ec3d5d539 100644 --- a/modules/util/util.js +++ b/modules/util/util.js @@ -581,3 +581,53 @@ export function utilUnicodeCharsCount(str) { export function utilUnicodeCharsTruncated(str, limit) { return Array.from(str).slice(0, limit).join(''); } + + +// 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 idPattern = /^[cnwr](-?)(\d+)$/; + var decomposeID = (id) => { + var res = { + id: id, + num: NaN + }; + + var match = id.match(idPattern); + if (match) { + res.num = parseInt(match[2], 10); + res.isNew = !!match[1]; + } + + return res; + }; + + var compareDecomposed = (left, right) => { + if (isNaN(left.num) && isNaN(right.num)) return -1; + if (isNaN(left.num)) return 1; + if (isNaN(right.num)) return -1; + if (left.isNew && !right.isNew) return 1; + if (!left.isNew && right.isNew) return -1; + return Math.sign(left.num - right.num); + }; + + var oldestIDIndex = 0; + var oldestID = decomposeID(ids[0]); + + for (var i = 1; i < ids.length; i++) { + var decomposed = decomposeID(ids[i]); + + if (compareDecomposed(oldestID, decomposed) === 1) { + oldestIDIndex = i; + oldestID = decomposed; + } + } + + return ids[oldestIDIndex]; +} diff --git a/test/spec/spec_helpers.js b/test/spec/spec_helpers.js index f140d0a70..6506928ec 100644 --- a/test/spec/spec_helpers.js +++ b/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 diff --git a/test/spec/util/util.js b/test/spec/util/util.js index 95f8e1c18..9768bfa12 100644 --- a/test/spec/util/util.js +++ b/test/spec/util/util.js @@ -252,4 +252,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'); + }); + }); }); From e9c74362893b51b214945ad83e38ac558b4bfc47 Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Fri, 10 Apr 2020 23:32:39 +0200 Subject: [PATCH 2/8] Use `utilOldestID()` when joining ways --- modules/actions/join.js | 18 ++++------- test/spec/actions/join.js | 68 +++++++++++++++++++++++++++++++++++---- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/modules/actions/join.js b/modules/actions/join.js index 9d85dfe6f..5d20ad40a 100644 --- a/modules/actions/join.js +++ b/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]; diff --git a/test/spec/actions/join.js b/test/spec/actions/join.js index b7974a3ec..f75be5814 100644 --- a/test/spec/actions/join.js +++ b/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' }); }); From 87ca2b09cce5d38124cc42741cb9f1b7524d27d0 Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Sat, 11 Apr 2020 16:26:17 +0200 Subject: [PATCH 3/8] Keep the oldest interesting ID alive when merging nodes --- modules/actions/connect.js | 28 +++++++----- test/spec/actions/connect.js | 77 +++++++++++++++++++++++++++----- test/spec/actions/merge_nodes.js | 76 +++++++++++++++++++++++++++++-- 3 files changed, 158 insertions(+), 23 deletions(-) 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 // | | From 23b3bc27b6d9465b30faa8171bdd07511c3087ba Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Sat, 11 Apr 2020 16:26:17 +0200 Subject: [PATCH 4/8] Keep the oldest ID alive when merging polygons --- modules/actions/merge_polygon.js | 22 +++++++++++++++------- test/spec/actions/merge_polygon.js | 10 +++++----- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/modules/actions/merge_polygon.js b/modules/actions/merge_polygon.js index 8d5829401..0f29cd5b5 100644 --- a/modules/actions/merge_polygon.js +++ b/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) { diff --git a/test/spec/actions/merge_polygon.js b/test/spec/actions/merge_polygon.js index d3f7e0f4f..89814da98 100644 --- a/test/spec/actions/merge_polygon.js +++ b/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'); From fab6dfa3dd848c8e7a635f38f40750be1773c268 Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Sat, 11 Apr 2020 16:26:17 +0200 Subject: [PATCH 5/8] Add the `utilCompareIDs()` function --- modules/util/index.js | 1 + modules/util/util.js | 56 ++++++++++++++++++++---------------------- test/spec/util/util.js | 32 ++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 29 deletions(-) diff --git a/modules/util/index.js b/modules/util/index.js index 26fb02c79..a1fd6c74f 100644 --- a/modules/util/index.js +++ b/modules/util/index.js @@ -35,6 +35,7 @@ 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'; diff --git a/modules/util/util.js b/modules/util/util.js index ec3d5d539..f80128b61 100644 --- a/modules/util/util.js +++ b/modules/util/util.js @@ -582,6 +582,29 @@ 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). @@ -592,40 +615,15 @@ export function utilOldestID(ids) { return undefined; } - var idPattern = /^[cnwr](-?)(\d+)$/; - var decomposeID = (id) => { - var res = { - id: id, - num: NaN - }; - - var match = id.match(idPattern); - if (match) { - res.num = parseInt(match[2], 10); - res.isNew = !!match[1]; - } - - return res; - }; - - var compareDecomposed = (left, right) => { - if (isNaN(left.num) && isNaN(right.num)) return -1; - if (isNaN(left.num)) return 1; - if (isNaN(right.num)) return -1; - if (left.isNew && !right.isNew) return 1; - if (!left.isNew && right.isNew) return -1; - return Math.sign(left.num - right.num); - }; - var oldestIDIndex = 0; - var oldestID = decomposeID(ids[0]); + var oldestID = toNumericID(ids[0]); for (var i = 1; i < ids.length; i++) { - var decomposed = decomposeID(ids[i]); + var num = toNumericID(ids[i]); - if (compareDecomposed(oldestID, decomposed) === 1) { + if (compareNumericIDs(oldestID, num) === 1) { oldestIDIndex = i; - oldestID = decomposed; + oldestID = num; } } diff --git a/test/spec/util/util.js b/test/spec/util/util.js index 9768bfa12..a033afc38 100644 --- a/test/spec/util/util.js +++ b/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'); From 7a1d08f80e9a507bfcd6b547187b5e89bd9bcf60 Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Sat, 11 Apr 2020 16:26:17 +0200 Subject: [PATCH 6/8] Return the empty string when calling `toOSM()` on entities with test IDs --- modules/osm/entity.js | 9 +++++++-- test/spec/osm/entity.js | 5 +++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/modules/osm/entity.js b/modules/osm/entity.js index 7212443de..5636fdc78 100644 --- a/modules/osm/entity.js +++ b/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] === '-'; }, diff --git a/test/spec/osm/entity.js b/test/spec/osm/entity.js index cbc81833d..95f440c9a 100644 --- a/test/spec/osm/entity.js +++ b/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(''); }); }); }); From 21f171863cb4caef002fa85c3fe8db051eef57ca Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Sat, 11 Apr 2020 16:26:17 +0200 Subject: [PATCH 7/8] Keep the oldest IDs alive when merging nodes into a way --- modules/actions/merge.js | 73 ++++++++++++++++++----- test/spec/actions/merge.js | 117 +++++++++++++++++++++++++++++++++---- 2 files changed, 164 insertions(+), 26 deletions(-) diff --git a/modules/actions/merge.js b/modules/actions/merge.js index d44cfdc18..70fc6fca3 100644 --- a/modules/actions/merge.js +++ b/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); diff --git a/test/spec/actions/merge.js b/test/spec/actions/merge.js index 0c80063ea..c0e407c68 100644 --- a/test/spec/actions/merge.js +++ b/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); }); }); From 3ff06f90451312f0a8c4a8e84396148919d11ab9 Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Sat, 11 Apr 2020 16:26:17 +0200 Subject: [PATCH 8/8] Fix relation handling on way split Depending on which way is the longest, the new way is inserted into the relation before the existing one. This case must be explicitly handled for the relation to remain correct. --- modules/actions/add_member.js | 23 +- test/spec/actions/add_member.js | 58 +++ test/spec/actions/split.js | 865 ++++++++++++++++++++++---------- 3 files changed, 675 insertions(+), 271 deletions(-) diff --git a/modules/actions/add_member.js b/modules/actions/add_member.js index d7e8867b2..9be9dc937 100644 --- a/modules/actions/add_member.js +++ b/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 } + ]; } } diff --git a/test/spec/actions/add_member.js b/test/spec/actions/add_member.js index 02564cee8..62f6c69bc 100644 --- a/test/spec/actions/add_member.js +++ b/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]}), diff --git a/test/spec/actions/split.js b/test/spec/actions/split.js index 1e5c4aca0..b58ef10d2 100644 --- a/test/spec/actions/split.js +++ b/test/spec/actions/split.js @@ -164,6 +164,99 @@ describe('iD.actionSplit', function () { expect(graph.entity('=').tags).to.equal(tags); }); + it('gives the previous id to the longest way (first)', function () { + // + // Situation: + // a ---> b ---> c ---> d ---> e ---> f split at 'd' + // + // Expected result: + // a ---> b ---> c ---> d ===> e ===> f + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmNode({ id: 'e', loc: [4, 0] }), + iD.osmNode({ id: 'f', loc: [5, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'd', 'e', 'f'] }) + ]); + + graph = iD.actionSplit('d', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c', 'd']); + expect(graph.entity('=').nodes).to.eql(['d', 'e', 'f']); + }); + + it('gives the previous id to the longest way (second)', function () { + // + // Situation: + // a ---> b ---> c ---> d ---> e ---> f split at 'c' + // + // Expected result: + // a ===> b ===> c ---> d ---> e ---> f + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmNode({ id: 'e', loc: [4, 0] }), + iD.osmNode({ id: 'f', loc: [5, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'd', 'e', 'f'] }) + ]); + + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['c', 'd', 'e', 'f']); + expect(graph.entity('=').nodes).to.eql(['a', 'b', 'c']); + }); + + it('gives the previous id to the first way on same length', function () { + // + // Situation: + // a ---> b ---> c ---> d ---> e split at 'c' + // + // Expected result: + // a ---> b ---> c ===> d ===> e + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmNode({ id: 'e', loc: [4, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'd', 'e'] }) + ]); + + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); + expect(graph.entity('=').nodes).to.eql(['c', 'd', 'e']); + }); + + it('gives the previous id to the longest way even with fewer nodes', function () { + // + // Situation: + // a -----------------> d ---> e ---> f split at 'd' + // + // Expected result: + // a -----------------> d ===> e ===> f + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmNode({ id: 'e', loc: [4, 0] }), + iD.osmNode({ id: 'f', loc: [5, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'd', 'e', 'f'] }) + ]); + + graph = iD.actionSplit('d', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['a', 'd']); + expect(graph.entity('=').nodes).to.eql(['d', 'e', 'f']); + }); + it('splits a way at a T-junction', function () { // // Situation: @@ -442,6 +535,64 @@ describe('iD.actionSplit', function () { expect(members(graph)).to.eql(['~', '=', '-']); }); + it('adds the new way to parent relations (existing way is first)', function () { + // + // Situation: + // a ---> b ---> c ---> d split at 'c' + // Relation: ['-'] + // + // Expected result: + // a ---> b ---> c ===> d + // Relation: ['-', '='] + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'd'] }), + iD.osmRelation({id: 'r', members: [ + { id: '-', type: 'way', role: 'forward' } + ]}) + ]); + + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('r').members).to.eql([ + { id: '-', type: 'way', role: 'forward' }, + { id: '=', type: 'way', role: 'forward' } + ]); + }); + + it('adds the new way to parent relations (existing way is second)', function () { + // + // Situation: + // a ---> b ---> c ---> d split at 'b' + // Relation: ['-'] + // + // Expected result: + // a ===> b ---> c ---> d + // Relation: ['=', '-'] + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'd'] }), + iD.osmRelation({id: 'r', members: [ + { id: '-', type: 'way', role: 'forward' } + ]}) + ]); + + graph = iD.actionSplit('b', ['='])(graph); + + expect(graph.entity('r').members).to.eql([ + { id: '=', type: 'way', role: 'forward' }, + { id: '-', type: 'way', role: 'forward' } + ]); + }); + it('reorders members as node, way, relation (for Public Transport routing)', function () { var graph = iD.coreGraph([ iD.osmNode({ id: 'a', loc: [0, 0] }), @@ -473,122 +624,195 @@ describe('iD.actionSplit', function () { var b = iD.osmNode({ id: 'b', loc: [0, 1] }); var c = iD.osmNode({ id: 'c', loc: [0, 2] }); var d = iD.osmNode({ id: 'd', loc: [0, 3] }); + var e = iD.osmNode({ id: 'e', loc: [0, 4] }); + + // + // Situation: + // a ---> b ---> c ---> d ~~~> e + // Relation: ['-', '~', '~', '-'] + // + var outAndBack1 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '~', nodes: ['d', 'e']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way'}, + {id: '~', type: 'way'}, + {id: '~', type: 'way'}, + {id: '-', type: 'way'} + ]}) + ]); + + // + // Situation: + // a <--- b <--- c <--- d ~~~> e + // Relation: ['-', '~', '~', '-'] + // + var outAndBack2 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['d', 'c', 'b', 'a']}), + iD.osmWay({id: '~', nodes: ['d', 'e']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way'}, + {id: '~', type: 'way'}, + {id: '~', type: 'way'}, + {id: '-', type: 'way'} + ]}) + ]); + + // + // Situation: + // a ---> b ---> c ---> d <~~~ e + // Relation: ['-', '~', '~', '-'] + // + var outAndBack3 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '~', nodes: ['e', 'd']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way'}, + {id: '~', type: 'way'}, + {id: '~', type: 'way'}, + {id: '-', type: 'way'} + ]}) + ]); + + // + // Situation: + // a <--- b <--- c <--- d <~~~ e + // Relation: ['-', '~', '~', '-'] + // + var outAndBack4 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['d', 'c', 'b', 'a']}), + iD.osmWay({id: '~', nodes: ['e', 'd']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way'}, + {id: '~', type: 'way'}, + {id: '~', type: 'way'}, + {id: '-', type: 'way'} + ]}) + ]); + + it('splits out-and-back1 route at c', function () { + // + // Expected result: + // a ---> b ---> c ===> d ~~~> e + // Relation: ['-', '=', '~', '~', '=', '-'] + // + var graph = outAndBack1; + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); + expect(graph.entity('=').nodes).to.eql(['c', 'd']); + expect(graph.entity('~').nodes).to.eql(['d', 'e']); + expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); + }); it('splits out-and-back1 route at b', function () { - // - // Situation: - // a ---> b ---> c ~~~> d split at 'b' - // Relation: ['-', '~', '~', '-'] // // Expected result: - // a ---> b ===> c ~~~> d - // Relation: ['-', '=', '~', '~', '=', '-'] + // a ===> b ---> c ---> d ~~~> e + // Relation: ['=', '-', '~', '~', '-', '='] // - var graph = iD.coreGraph([ - a, b, c, d, - iD.osmWay({id: '-', nodes: ['a', '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 graph = outAndBack1; graph = iD.actionSplit('b', ['='])(graph); - expect(graph.entity('-').nodes).to.eql(['a', 'b']); - expect(graph.entity('=').nodes).to.eql(['b', 'c']); - expect(graph.entity('~').nodes).to.eql(['c', 'd']); - expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); + expect(graph.entity('-').nodes).to.eql(['b', 'c', 'd']); + expect(graph.entity('=').nodes).to.eql(['a', 'b']); + expect(graph.entity('~').nodes).to.eql(['d', 'e']); + expect(members(graph)).to.eql(['=', '-', '~', '~', '-', '=']); }); it('splits out-and-back2 route at b', function () { - // - // Situation: - // a <--- b <--- c ~~~> d split at 'b' - // Relation: ['-', '~', '~', '-'] // // Expected result: - // a <=== b <--- c ~~~> d + // a <=== b <--- c <--- d ~~~> e // Relation: ['=', '-', '~', '~', '-', '='] // - var graph = iD.coreGraph([ - a, b, c, d, - iD.osmWay({id: '-', nodes: ['c', 'b', 'a']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', members: [ - {id: '-', type: 'way'}, - {id: '~', type: 'way'}, - {id: '~', type: 'way'}, - {id: '-', type: 'way'} - ]}) - ]); + var graph = outAndBack2; graph = iD.actionSplit('b', ['='])(graph); - expect(graph.entity('-').nodes).to.eql(['c', 'b']); + expect(graph.entity('-').nodes).to.eql(['d', 'c', 'b']); expect(graph.entity('=').nodes).to.eql(['b', 'a']); - expect(graph.entity('~').nodes).to.eql(['c', 'd']); + expect(graph.entity('~').nodes).to.eql(['d', 'e']); expect(members(graph)).to.eql(['=', '-', '~', '~', '-', '=']); }); + it('splits out-and-back2 route at c', function () { + // + // Expected result: + // a <--- b <--- c <=== d ~~~> e + // Relation: ['-', '=', '~', '~', '=', '-'] + // + var graph = outAndBack2; + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['c', 'b', 'a']); + expect(graph.entity('=').nodes).to.eql(['d', 'c']); + expect(graph.entity('~').nodes).to.eql(['d', 'e']); + expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); + }); + + it('splits out-and-back3 route at c', function () { + // + // Expected result: + // a ---> b ---> c ===> d <~~~ e + // Relation: ['-', '=', '~', '~', '=', '-'] + // + var graph = outAndBack3; + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); + expect(graph.entity('=').nodes).to.eql(['c', 'd']); + expect(graph.entity('~').nodes).to.eql(['e', 'd']); + expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); + }); + it('splits out-and-back3 route at b', function () { - // - // Situation: - // a ---> b ---> c <~~~ d split at 'b' - // Relation: ['-', '~', '~', '-'] // // Expected result: - // a ---> b ===> c <~~~ d - // Relation: ['-', '=', '~', '~', '=', '-'] + // a ===> b ---> c ---> d <~~~ e + // Relation: ['=', '-', '~', '~', '-', '='] // - var graph = iD.coreGraph([ - a, b, c, d, - iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), - iD.osmWay({id: '~', nodes: ['d', 'c']}), - iD.osmRelation({id: 'r', members: [ - {id: '-', type: 'way'}, - {id: '~', type: 'way'}, - {id: '~', type: 'way'}, - {id: '-', type: 'way'} - ]}) - ]); + var graph = outAndBack3; graph = iD.actionSplit('b', ['='])(graph); - expect(graph.entity('-').nodes).to.eql(['a', 'b']); - expect(graph.entity('=').nodes).to.eql(['b', 'c']); - expect(graph.entity('~').nodes).to.eql(['d', 'c']); - expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); + expect(graph.entity('-').nodes).to.eql(['b', 'c', 'd']); + expect(graph.entity('=').nodes).to.eql(['a', 'b']); + expect(graph.entity('~').nodes).to.eql(['e', 'd']); + expect(members(graph)).to.eql(['=', '-', '~', '~', '-', '=']); }); it('splits out-and-back4 route at b', function () { - // - // Situation: - // a <--- b <--- c <~~~ d split at 'b' - // Relation: ['-', '~', '~', '-'] // // Expected result: - // a <=== b <--- c <~~~ d + // a <=== b <--- c <--- d <~~~ e // Relation: ['=', '-', '~', '~', '-', '='] // - var graph = iD.coreGraph([ - a, b, c, d, - iD.osmWay({id: '-', nodes: ['c', 'b', 'a']}), - iD.osmWay({id: '~', nodes: ['d', 'c']}), - iD.osmRelation({id: 'r', members: [ - {id: '-', type: 'way'}, - {id: '~', type: 'way'}, - {id: '~', type: 'way'}, - {id: '-', type: 'way'} - ]}) - ]); + var graph = outAndBack4; graph = iD.actionSplit('b', ['='])(graph); - expect(graph.entity('-').nodes).to.eql(['c', 'b']); + expect(graph.entity('-').nodes).to.eql(['d', 'c', 'b']); expect(graph.entity('=').nodes).to.eql(['b', 'a']); - expect(graph.entity('~').nodes).to.eql(['d', 'c']); + expect(graph.entity('~').nodes).to.eql(['e', 'd']); expect(members(graph)).to.eql(['=', '-', '~', '~', '-', '=']); }); + + it('splits out-and-back4 route at c', function () { + // + // Expected result: + // a <--- b <--- c <=== d <~~~ e + // Relation: ['-', '=', '~', '~', '=', '-'] + // + var graph = outAndBack4; + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('-').nodes).to.eql(['c', 'b', 'a']); + expect(graph.entity('=').nodes).to.eql(['d', 'c']); + expect(graph.entity('~').nodes).to.eql(['e', 'd']); + expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); + }); }); describe('splitting hat routes', function () { @@ -1157,145 +1381,248 @@ describe('iD.actionSplit', function () { ['restriction', 'restriction:bus', 'manoeuvre'].forEach(function (type) { describe('type = ' + type, function () { + var a = iD.osmNode({id: 'a', loc: [0, 0]}); + var b = iD.osmNode({id: 'b', loc: [1, 0]}); + var c = iD.osmNode({id: 'c', loc: [2, 0]}); + var d = iD.osmNode({id: 'd', loc: [3, 0]}); + var e = iD.osmNode({id: 'e', loc: [4, 0]}); + var f = iD.osmNode({id: 'f', loc: [5, 0]}); - it('updates a restriction\'s \'from\' role - via node', function () { - // Situation: - // a ----> b ----> c ~~~~ d - // A restriction from ---- to ~~~~ via node c. - // - // Split at b. + // + // Situation: + // a ----> b ----> c ----> d ~~~~ e + // A restriction from ---- to ~~~~ via node d. + // + var restriction1 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '~', nodes: ['d', 'e']}), + iD.osmRelation({id: 'r', tags: {type: type}, members: [ + {id: '-', role: 'from', type: 'way'}, + {id: '~', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} + ]}) + ]); + + // + // Situation: + // a ----> b ----> c ----> d ~~~~ e + // A restriction from ~~~~ to ---- via node d. + // + var restriction2 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '~', nodes: ['d', 'e']}), + iD.osmRelation({id: 'r', tags: {type: type}, members: [ + {id: '~', role: 'from', type: 'way'}, + {id: '-', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} + ]}) + ]); + + // + // Situation: + // a ----> b ----> c ----> d ~~~~ e + // A restriction from ---- to ---- via node d. + // + var restriction3 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '~', nodes: ['d', 'e']}), + iD.osmRelation({id: 'r', tags: {type: type}, members: [ + {id: '-', role: 'from', type: 'way'}, + {id: '-', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} + ]}) + ]); + + // + // Situation: + // f <~~~~ e + // | + // | + // a ----> b ----> c ----> d + // + // A restriction from ---- to ~~~~ via way | + // + var restriction4 = iD.coreGraph([ + a, b, c, d, e, f, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '|', nodes: ['d', 'e']}), + iD.osmWay({id: '~', nodes: ['e', 'f']}), + iD.osmRelation({id: 'r', tags: {type: type}, members: [ + {id: '-', role: 'from', type: 'way'}, + {id: '~', role: 'to', type: 'way'}, + {id: '|', role: 'via', type: 'way'} + ]}) + ]); + + // + // Situation: + // f <~~~~ e + // | + // | + // a ----> b ----> c ----> d + // + // A restriction from ~~~~ to ---- via way | + // + var restriction5 = iD.coreGraph([ + a, b, c, d, e, f, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '|', nodes: ['d', 'e']}), + iD.osmWay({id: '~', nodes: ['e', 'f']}), + iD.osmRelation({id: 'r', tags: {type: type}, members: [ + {id: '~', role: 'from', type: 'way'}, + {id: '-', role: 'to', type: 'way'}, + {id: '|', role: 'via', type: 'way'} + ]}) + ]); + + // + // Situation: + // e f + // | ‖ + // | ‖ + // a ----> b ----> c ----> d + // + // A restriction from | to ‖ via way ---- + // + var restriction6 = iD.coreGraph([ + a, b, c, d, e, f, + iD.osmWay({id: '-', nodes: ['a', 'b', 'c', 'd']}), + iD.osmWay({id: '|', nodes: ['e', 'a']}), + iD.osmWay({id: '‖', nodes: ['f', 'd']}), + iD.osmRelation({id: 'r', tags: {type: type}, members: [ + {id: '|', role: 'from', type: 'way'}, + {id: '-', role: 'via', type: 'way'}, + {id: '‖', role: 'to', type: 'way'} + ]}) + ]); + + // + // Situation: + // a <---- b <---- c <---- d ~~~~ e + // A restriction from ---- to ~~~~ via d. + // + var restriction7 = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['d', 'c', 'b', 'a']}), + iD.osmWay({id: '~', nodes: ['d', 'e']}), + iD.osmRelation({id: 'r', tags: {type: type}, members: [ + {id: '-', role: 'from', type: 'way'}, + {id: '~', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} + ]}) + ]); + + it('updates a restriction\'s \'from\' role - via node (1c)', function () { // // Expected result: - // a ----> b ====> c ~~~~ d - // A restriction from ==== to ~~~~ via node c. + // a ----> b ----> c ====> d ~~~~ e + // A restriction from ==== to ~~~~ via node d. // - var graph = iD.coreGraph([ - iD.osmNode({id: 'a'}), - iD.osmNode({id: 'b'}), - iD.osmNode({id: 'c'}), - iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', tags: {type: type}, members: [ - {id: '-', role: 'from', type: 'way'}, - {id: '~', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} - ]}) - ]); - - graph = iD.actionSplit('b', ['='])(graph); + var graph = restriction1; + graph = iD.actionSplit('c', ['='])(graph); expect(graph.entity('r').members).to.eql([ {id: '=', role: 'from', type: 'way'}, {id: '~', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} + {id: 'd', role: 'via', type: 'node'} ]); }); - it('updates a restriction\'s \'to\' role - via node', function () { - // Situation: - // a ----> b ----> c ~~~~ d - // A restriction from ~~~~ to ---- via node c. - // - // Split at b. + it('updates a restriction\'s \'from\' role - via node (1b)', function () { // // Expected result: - // a ----> b ====> c ~~~~ d - // A restriction from ~~~~ to ==== via node c. + // a ====> b ----> c ----> d ~~~~ e + // A restriction from ---- to ~~~~ via node d. // - var graph = iD.coreGraph([ - iD.osmNode({id: 'a'}), - iD.osmNode({id: 'b'}), - iD.osmNode({id: 'c'}), - iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', tags: {type: type}, members: [ - {id: '~', role: 'from', type: 'way'}, - {id: '-', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} - ]}) - ]); - + var graph = restriction1; graph = iD.actionSplit('b', ['='])(graph); + expect(graph.entity('r').members).to.eql([ + {id: '-', role: 'from', type: 'way'}, + {id: '~', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} + ]); + }); + + it('updates a restriction\'s \'to\' role - via node (2c)', function () { + // + // Expected result: + // a ----> b ----> c ====> d ~~~~ e + // A restriction from ~~~~ to ==== via node d. + // + var graph = restriction2; + graph = iD.actionSplit('c', ['='])(graph); + expect(graph.entity('r').members).to.eql([ {id: '~', role: 'from', type: 'way'}, {id: '=', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} + {id: 'd', role: 'via', type: 'node'} ]); }); - it('updates both \'to\' and \'from\' roles for via-node u-turn restrictions', function () { - // Situation: - // a ----> b ----> c ~~~~ d - // A restriction from ---- to ---- via node c. - // - // Split at b. + it('updates a restriction\'s \'to\' role - via node (2b)', function () { // // Expected result: - // a ----> b ====> c ~~~~ d - // A restriction from ==== to ==== via node c. + // a ====> b ----> c ----> d ~~~~ e + // A restriction from ~~~~ to ---- via node d. // - var graph = iD.coreGraph([ - iD.osmNode({id: 'a'}), - iD.osmNode({id: 'b'}), - iD.osmNode({id: 'c'}), - iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', tags: {type: type}, members: [ - {id: '-', role: 'from', type: 'way'}, - {id: '-', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} - ]}) - ]); - + var graph = restriction2; graph = iD.actionSplit('b', ['='])(graph); + expect(graph.entity('r').members).to.eql([ + {id: '~', role: 'from', type: 'way'}, + {id: '-', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} + ]); + }); + + it('updates both \'to\' and \'from\' roles for via-node u-turn restrictions (3c)', function () { + // + // Expected result: + // a ----> b ----> c ====> d ~~~~ e + // A restriction from ==== to ==== via node d. + // + var graph = restriction3; + graph = iD.actionSplit('c', ['='])(graph); + expect(graph.entity('r').members).to.eql([ {id: '=', role: 'from', type: 'way'}, {id: '=', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} + {id: 'd', role: 'via', type: 'node'} ]); }); - it('updates a restriction\'s \'from\' role - via way', function () { - // Situation: - // e <~~~~ d - // | - // | - // a ----> b ----> c - // - // A restriction from ---- to ~~~~ via way | - // - // Split at b. + it('updates both \'to\' and \'from\' roles for via-node u-turn restrictions (3b)', function () { // // Expected result: - // e <~~~~ d - // | - // | - // a ----> b ====> c + // a ====> b ----> c ----> d ~~~~ e + // A restriction from ---- to ---- via node d. + // + var graph = restriction3; + graph = iD.actionSplit('b', ['='])(graph); + + expect(graph.entity('r').members).to.eql([ + {id: '-', role: 'from', type: 'way'}, + {id: '-', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} + ]); + }); + + it('updates a restriction\'s \'from\' role - via way (4c)', function () { + // + // Expected result: + // f <~~~~ e + // | + // | + // a ----> b ----> c ====> d // // A restriction from ==== to ~~~~ via way | // - 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: ['c', 'd']}), - iD.osmWay({id: '~', nodes: ['d', 'e']}), - iD.osmRelation({id: 'r', tags: {type: type}, members: [ - {id: '-', role: 'from', type: 'way'}, - {id: '~', role: 'to', type: 'way'}, - {id: '|', role: 'via', type: 'way'} - ]}) - ]); - - graph = iD.actionSplit('b', ['='])(graph); + var graph = restriction4; + graph = iD.actionSplit('c', ['='])(graph); expect(graph.entity('r').members).to.eql([ {id: '=', role: 'from', type: 'way'}, @@ -1304,42 +1631,38 @@ describe('iD.actionSplit', function () { ]); }); - it('updates a restriction\'s \'to\' role - via way', function () { - // Situation: - // e <~~~~ d - // | - // | - // a ----> b ----> c - // - // A restriction from ~~~~ to ---- via way | - // - // Split at b. + it('updates a restriction\'s \'from\' role - via way (4b)', function () { // // Expected result: - // e <~~~~ d - // | - // | - // a ----> b ====> c + // f <~~~~ e + // | + // | + // a ====> b ----> c ----> d + // + // A restriction from ---- to ~~~~ via way | + // + var graph = restriction4; + graph = iD.actionSplit('b', ['='])(graph); + + expect(graph.entity('r').members).to.eql([ + {id: '-', role: 'from', type: 'way'}, + {id: '~', role: 'to', type: 'way'}, + {id: '|', role: 'via', type: 'way'} + ]); + }); + + it('updates a restriction\'s \'to\' role - via way (5c)', function () { + // + // Expected result: + // f <~~~~ e + // | + // | + // a ----> b ----> c ====> d // // A restriction from ~~~~ to ==== via way | // - 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: ['c', 'd']}), - iD.osmWay({id: '~', nodes: ['d', 'e']}), - iD.osmRelation({id: 'r', tags: {type: type}, members: [ - {id: '~', role: 'from', type: 'way'}, - {id: '-', role: 'to', type: 'way'}, - {id: '|', role: 'via', type: 'way'} - ]}) - ]); - - graph = iD.actionSplit('b', ['='])(graph); + var graph = restriction5; + graph = iD.actionSplit('c', ['='])(graph); expect(graph.entity('r').members).to.eql([ {id: '~', role: 'from', type: 'way'}, @@ -1348,43 +1671,38 @@ describe('iD.actionSplit', function () { ]); }); - - it('updates a restriction\'s \'via\' role when splitting via way', function () { - // Situation: - // d e - // | ‖ - // | ‖ - // a ----> b ----> c - // - // A restriction from | to ‖ via way ---- - // - // Split at b. + it('updates a restriction\'s \'to\' role - via way (5b)', function () { // // Expected result: - // d e - // | ‖ - // | ‖ - // a ----> b ====> c + // f <~~~~ e + // | + // | + // a ====> b ----> c ----> d + // + // A restriction from ~~~~ to ---- via way | + // + var graph = restriction5; + graph = iD.actionSplit('b', ['='])(graph); + + expect(graph.entity('r').members).to.eql([ + {id: '~', role: 'from', type: 'way'}, + {id: '-', role: 'to', type: 'way'}, + {id: '|', role: 'via', type: 'way'} + ]); + }); + + it('updates a restriction\'s \'via\' role when splitting via way (6c)', function () { + // + // Expected result: + // e f + // | ‖ + // | ‖ + // a ----> b ----> c ====> d // // A restriction from | to ‖ via ways ----, ==== // - 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', 'a']}), - iD.osmWay({id: '‖', nodes: ['e', 'c']}), - iD.osmRelation({id: 'r', tags: {type: type}, members: [ - {id: '|', role: 'from', type: 'way'}, - {id: '-', role: 'via', type: 'way'}, - {id: '‖', role: 'to', type: 'way'} - ]}) - ]); - - graph = iD.actionSplit('b', ['='])(graph); + var graph = restriction6; + graph = iD.actionSplit('c', ['='])(graph); expect(graph.entity('r').members).to.eql([ {id: '|', role: 'from', type: 'way'}, @@ -1394,37 +1712,56 @@ describe('iD.actionSplit', function () { ]); }); - it('leaves unaffected restrictions unchanged', function () { - // Situation: - // a <---- b <---- c ~~~~ d - // A restriction from ---- to ~~~~ via c. - // - // Split at b. + it('updates a restriction\'s \'via\' role when splitting via way (6b)', function () { // // Expected result: - // a <==== b <---- c ~~~~ d - // A restriction from ---- to ~~~~ via c. + // e f + // | ‖ + // | ‖ + // a ====> b ----> c ----> d // - var graph = iD.coreGraph([ - iD.osmNode({id: 'a'}), - iD.osmNode({id: 'b'}), - iD.osmNode({id: 'c'}), - iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['c', 'b', 'a']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', tags: {type: type}, members: [ - {id: '-', role: 'from', type: 'way'}, - {id: '~', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} - ]}) - ]); + // A restriction from | to ‖ via ways ----, ==== + // + var graph = restriction6; + graph = iD.actionSplit('c', ['='])(graph); + expect(graph.entity('r').members).to.eql([ + {id: '|', role: 'from', type: 'way'}, + {id: '-', role: 'via', type: 'way'}, + {id: '=', role: 'via', type: 'way'}, + {id: '‖', role: 'to', type: 'way'} + ]); + }); + + it('leaves unaffected restrictions unchanged (7b)', function () { + // + // Expected result: + // a <==== b <---- c <---- d ~~~~ e + // A restriction from ---- to ~~~~ via d. + // + var graph = restriction7; graph = iD.actionSplit('b', ['='])(graph); expect(graph.entity('r').members).to.eql([ {id: '-', role: 'from', type: 'way'}, {id: '~', role: 'to', type: 'way'}, - {id: 'c', role: 'via', type: 'node'} + {id: 'd', role: 'via', type: 'node'} + ]); + }); + + it('leaves unaffected restrictions unchanged (7c)', function () { + // + // Expected result: + // a <---- b <---- c <==== d ~~~~ e + // A restriction from ---- to ~~~~ via d. + // + var graph = restriction7; + graph = iD.actionSplit('c', ['='])(graph); + + expect(graph.entity('r').members).to.eql([ + {id: '=', role: 'from', type: 'way'}, + {id: '~', role: 'to', type: 'way'}, + {id: 'd', role: 'via', type: 'node'} ]); }); });