diff --git a/data/core.yaml b/data/core.yaml index 059d8975d..d1d0b3290 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -115,6 +115,7 @@ en: vertex: Connected a way to another. line: Connected a way to a line. area: Connected a way to an area. + relation: These features can't be connected because they have conflicting relation roles. disconnect: title: Disconnect description: Disconnect these lines/areas from each other. diff --git a/dist/locales/en.json b/dist/locales/en.json index 90bb1a55d..f537bbf01 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -150,7 +150,8 @@ "vertex": "Connected a way to another.", "line": "Connected a way to a line.", "area": "Connected a way to an area." - } + }, + "relation": "These features can't be connected because they have conflicting relation roles." }, "disconnect": { "title": "Disconnect", diff --git a/modules/actions/connect.js b/modules/actions/connect.js index df633715b..70800024d 100644 --- a/modules/actions/connect.js +++ b/modules/actions/connect.js @@ -16,10 +16,11 @@ import { actionDeleteNode } from './delete_node'; // https://github.com/openstreetmap/josm/blob/mirror/src/org/openstreetmap/josm/actions/MergeNodesAction.java // export function actionConnect(nodeIds) { - return function(graph) { + var action = function(graph) { var survivor; var node; - var i; + var parents; + var i, j; // Choose a survivor node, prefer an existing (not new) node - #4974 for (i = 0; i < nodeIds.length; i++) { @@ -32,17 +33,17 @@ export function actionConnect(nodeIds) { node = graph.entity(nodeIds[i]); if (node.id === survivor.id) continue; - /* eslint-disable no-loop-func */ - graph.parentWays(node).forEach(function(parent) { - if (!parent.areAdjacent(node.id, survivor.id)) { - graph = graph.replace(parent.replaceNode(node.id, survivor.id)); + parents = graph.parentWays(node); + for (j = 0; j < parents.length; j++) { + if (!parents[j].areAdjacent(node.id, survivor.id)) { + graph = graph.replace(parents[j].replaceNode(node.id, survivor.id)); } - }); + } - graph.parentRelations(node).forEach(function(parent) { - graph = graph.replace(parent.replaceMember(node, survivor)); - }); - /* eslint-enable no-loop-func */ + parents = graph.parentRelations(node); + for (j = 0; j < parents.length; j++) { + graph = graph.replace(parents[j].replaceMember(node, survivor)); + } survivor = survivor.mergeTags(node.tags); graph = actionDeleteNode(node.id)(graph); @@ -52,4 +53,33 @@ export function actionConnect(nodeIds) { return graph; }; + + + action.disabled = function(graph) { + var seen = {}; + + for (var i = 0; i < nodeIds.length; i++) { + var node = graph.entity(nodeIds[i]); + + var toCheck = [node].concat(graph.parentWays(node)); + for (var j = 0; j < toCheck.length; j++) { + var entity = toCheck[j]; + + var relations = graph.parentRelations(entity); + for (var k = 0; k < relations.length; k++) { + var relation = relations[k]; + var role = relation.memberById(entity.id).role || ''; + + if (seen[relation.id] !== undefined && seen[relation.id] !== role) { + return 'relation'; + } else { + seen[relation.id] = role; + } + } + } + } + }; + + + return action; } diff --git a/modules/actions/disconnect.js b/modules/actions/disconnect.js index 82274234f..62a2162b0 100644 --- a/modules/actions/disconnect.js +++ b/modules/actions/disconnect.js @@ -20,12 +20,12 @@ export function actionDisconnect(nodeId, newNodeId) { var action = function(graph) { - var node = graph.entity(nodeId), - connections = action.connections(graph); + var node = graph.entity(nodeId); + var connections = action.connections(graph); connections.forEach(function(connection) { - var way = graph.entity(connection.wayID), - newNode = osmNode({id: newNodeId, loc: node.loc, tags: node.tags}); + var way = graph.entity(connection.wayID); + var newNode = osmNode({id: newNodeId, loc: node.loc, tags: node.tags}); graph = graph.replace(newNode); if (connection.index === 0 && way.isArea()) { @@ -45,9 +45,9 @@ export function actionDisconnect(nodeId, newNodeId) { action.connections = function(graph) { - var candidates = [], - keeping = false, - parentWays = graph.parentWays(graph.entity(nodeId)); + var candidates = []; + var keeping = false; + var parentWays = graph.parentWays(graph.entity(nodeId)); parentWays.forEach(function(way) { if (wayIds && wayIds.indexOf(way.id) === -1) { @@ -74,9 +74,9 @@ export function actionDisconnect(nodeId, newNodeId) { if (connections.length === 0 || (wayIds && wayIds.length !== connections.length)) return 'not_connected'; - var parentWays = graph.parentWays(graph.entity(nodeId)), - seenRelationIds = {}, - sharedRelation; + var parentWays = graph.parentWays(graph.entity(nodeId)); + var seenRelationIds = {}; + var sharedRelation; parentWays.forEach(function(way) { if (wayIds && wayIds.indexOf(way.id) === -1) diff --git a/modules/modes/drag_node.js b/modules/modes/drag_node.js index a824c4f81..04e875014 100644 --- a/modules/modes/drag_node.js +++ b/modules/modes/drag_node.js @@ -175,7 +175,8 @@ export function modeDragNode(context) { // - `behavior/draw.js` `click()` // - `behavior/draw_way.js` `move()` var d = datum(); - var targetLoc = d && d.properties && d.properties.entity && d.properties.entity.loc; + var target = d && d.properties && d.properties.entity; + var targetLoc = target && target.loc; var targetNodes = d && d.properties && d.properties.nodes; if (targetLoc) { // snap to node/vertex - a point target with `.loc` @@ -194,10 +195,23 @@ export function modeDragNode(context) { moveAnnotation(entity) ); + // Below here: validations + var isInvalid = false; + + // Check if this connection to `target` could cause relations to break.. + if (target) { + isInvalid = isRelationConflict([entity.id, target.id], context.graph()); + if (isInvalid && !context.surface().classed('nope')) { + uiFlash().text(t('operations.connect.relation'))(); + } + } + + // Check if this drag causes the geometry to break.. + if (!isInvalid) { + isInvalid = isInvalidGeometry(entity, context.graph()); + } - // check if this movement causes the geometry to break var nopeDisabled = context.surface().classed('nope-disabled'); - var isInvalid = isInvalidGeometry(entity, context.graph()); if (nopeDisabled) { context.surface() .classed('nope', false) @@ -212,6 +226,38 @@ export function modeDragNode(context) { } + // See also actionConnect.disabled() + // (similar, but this code can also check node-way) + function isRelationConflict(ids, graph) { + var seen = {}; + + for (var i = 0; i < ids.length; i++) { + var ent1 = graph.entity(ids[i]); + + var toCheck = [ent1]; + if (ent1.type === 'node') { + toCheck = toCheck.concat(graph.parentWays(ent1)); + } + for (var j = 0; j < toCheck.length; j++) { + var ent2 = toCheck[j]; + + var relations = graph.parentRelations(ent2); + for (var k = 0; k < relations.length; k++) { + var relation = relations[k]; + var role = relation.memberById(ent2.id).role || ''; + + if (seen[relation.id] !== undefined && seen[relation.id] !== role) { + return 'relation'; + } else { + seen[relation.id] = role; + } + } + } + } + return false; + } + + function isInvalidGeometry(entity, graph) { var parents = graph.parentWays(entity); var i, j, k; diff --git a/test/spec/actions/connect.js b/test/spec/actions/connect.js index a9316c2c2..59bad0ad1 100644 --- a/test/spec/actions/connect.js +++ b/test/spec/actions/connect.js @@ -171,4 +171,101 @@ describe('iD.actionConnect', function() { 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'}]); }); + + + describe('#disabled', function () { + it('returns falsy when connecting members of the same relation and same roles (different ways)', function () { + // + // "Route 1" "Route 1" + // 'forward' 'forward' + // 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: ['a', 'b']}), + iD.osmWay({id: '=', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r1', tags: { name: 'Route 1'}, members: [ + { id: '-', type: 'way', role: 'forward' }, + { id: '=', type: 'way', role: 'forward' } + ]}) + ]); + + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.be.not.ok; + }); + + it('returns falsy when connecting members of the different relation and different roles (different ways)', function () { + // + // "Route 1" "Route 2" + // 'forward' 'backward' + // 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: ['a', 'b']}), + iD.osmWay({id: '=', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r1', tags: { name: 'Route 1'}, members: [ + { id: '-', type: 'way', role: 'forward' } + ]}), + iD.osmRelation({id: 'r2', tags: { name: 'Route 2'}, members: [ + { id: '=', type: 'way', role: 'backward' } + ]}) + ]); + + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.be.not.ok; + }); + + it('returns \'relation\' when connecting members of the same relation but different roles (different ways)', function () { + // + // "Route 1" "Route 1" + // 'forward' 'backward' + // 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: ['a', 'b']}), + iD.osmWay({id: '=', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r1', tags: { name: 'Route 1'}, members: [ + { id: '-', type: 'way', role: 'forward' }, + { id: '=', type: 'way', role: 'backward' } + ]}) + ]); + + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.eql('relation'); + }); + + it('returns \'relation\' when connecting members of the same relation but different roles (joined way)', function () { + // + // from via to + // 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: ['a', 'b']}), + iD.osmWay({id: '=', nodes: ['b', 'c']}), + iD.osmWay({id: '~', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r', members: [ + { id: '-', type: 'way', role: 'from' }, + { id: '=', type: 'way', role: 'via' }, + { id: '~', type: 'way', role: 'to' } + ]}) + ]); + + expect(iD.actionConnect(['a', 'b']).disabled(graph)).to.eql('relation'); + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.eql('relation'); + expect(iD.actionConnect(['c', 'd']).disabled(graph)).to.eql('relation'); + }); + }); + });