Prevent drag/connect which could damage relations

(closes #4921)

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.

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",

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;
 }

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)

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;

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');
+        });
+    });
+
 });