From 3bad09d497fdfec17781166b69d3b761c935623e Mon Sep 17 00:00:00 2001 From: Jon D Date: Fri, 6 Jul 2018 22:20:19 +0100 Subject: [PATCH 1/5] Merge Work-In-Progress 4320 to 4320 --- data/core.yaml | 5 + dist/locales/en.json | 6 + modules/actions/detach_node.js | 27 + modules/actions/index.js | 1 + modules/operations/detach_node.js | 50 ++ modules/operations/index.js | 1 + .../operations/operation-detachNode.svg | 61 ++ test/index.html | 14 +- test/spec/actions/detach_node.js | 540 ++++++++++++++++++ 9 files changed, 700 insertions(+), 5 deletions(-) create mode 100644 modules/actions/detach_node.js create mode 100644 modules/operations/detach_node.js create mode 100644 svg/iD-sprite/operations/operation-detachNode.svg create mode 100644 test/spec/actions/detach_node.js diff --git a/data/core.yaml b/data/core.yaml index 5b544040b..820e0fc0d 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -229,6 +229,11 @@ en: annotation: create: Added a turn restriction delete: Deleted a turn restriction + detachNode: + title: Detach + key: D + description: Detach this node from these lines/areas. + annotation: Detached a node from owning lines/areas. restriction: controls: distance: Distance diff --git a/dist/locales/en.json b/dist/locales/en.json index 3412491a4..8abaa9db5 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -297,6 +297,12 @@ "create": "Added a turn restriction", "delete": "Deleted a turn restriction" } + }, + "detachNode": { + "title": "Detach", + "key": "D", + "description": "Detach this node from these lines/areas.", + "annotation": "Detached a node from owning lines/areas." } }, "restriction": { diff --git a/modules/actions/detach_node.js b/modules/actions/detach_node.js new file mode 100644 index 000000000..d83fd46ed --- /dev/null +++ b/modules/actions/detach_node.js @@ -0,0 +1,27 @@ +import { osmNode } from '../osm'; + +export function actionDetachNode(nodeId) { + return function (graph) { + // Get the point in question + var node = graph.entity(nodeId); + // Get all of the ways it's currently attached to + var parentWays = graph.parentWays(node); + // Create a new node to replace the one we will detach + var replacementNode = osmNode({ loc: node.loc }); + // We need to process each way in turn, updating the graph as we go + return parentWays + .reduce(function (accGraph, parentWay) { + // Make a note of where in the way our target node is inside this way + var originalIndex = parentWay.nodes.indexOf(nodeId); + // Swap out the target node for the replacement + var updatedWay = parentWay + .removeNode(nodeId) // Remove our target node from the parent way + .addNode(replacementNode.id, originalIndex); // Add in the replacement node in its place + // Update the graph with the updated way + return accGraph.replace(updatedWay); + }, + // Seed the reduction with the input graph, updated to include the replacementNode so + // that is accessible to the ways when we add it in to them + graph.replace(replacementNode)); + }; +} diff --git a/modules/actions/index.js b/modules/actions/index.js index 330690db2..4f15456d8 100644 --- a/modules/actions/index.js +++ b/modules/actions/index.js @@ -33,3 +33,4 @@ export { actionSplit } from './split'; export { actionStraighten } from './straighten'; export { actionUnrestrictTurn } from './unrestrict_turn'; export { actionReflect } from './reflect.js'; +export { actionDetachNode } from './detach_node'; \ No newline at end of file diff --git a/modules/operations/detach_node.js b/modules/operations/detach_node.js new file mode 100644 index 000000000..28ca9b600 --- /dev/null +++ b/modules/operations/detach_node.js @@ -0,0 +1,50 @@ +import { actionDetachNode } from '../actions/index'; +import { behaviorOperation } from '../behavior/index'; +import { modeMove } from '../modes/index'; +import { t } from '../util/locale'; + +export function operationDetachNode(selectedIDs, context) { + var selectedNode = selectedIDs[0]; + var operation = function () { + context.perform(actionDetachNode(selectedNode)); + context.enter(modeMove(context, [selectedNode], context.graph)); + }; + var hasTags = function (entity) { + return Object.keys(entity.tags).length > 0; + }; + operation.available = function () { + // Check multiple items aren't selected + if (selectedIDs.length !== 1) { + return false; + } + // Get the entity itself + var graph = context.graph(); + var entity = graph.hasEntity(selectedNode); + if (!entity) { + // This probably isn't possible + return false; + } + // Confirm entity is a node with tags + if (entity.type === 'node' && hasTags(entity)) { + // Confirm that the node is owned by at least 1 parent way + var parentWays = graph.parentWays(entity); + return parentWays && parentWays.length > 0; + } + // Not appropriate + return false; + }; + operation.disabled = function () { + return false; + }; + operation.tooltip = function () { + return t('operations.detachNode.description'); + }; + operation.annotation = function () { + return t('operations.detachNode.annotation'); + }; + operation.id = 'detachNode'; + operation.keys = [t('operations.detachNode.key')]; + operation.title = t('operations.detachNode.title'); + operation.behavior = behaviorOperation(context).which(operation); + return operation; +} diff --git a/modules/operations/index.js b/modules/operations/index.js index 1ad24cd97..8339d8205 100644 --- a/modules/operations/index.js +++ b/modules/operations/index.js @@ -10,3 +10,4 @@ export { operationReverse } from './reverse'; export { operationRotate } from './rotate'; export { operationSplit } from './split'; export { operationStraighten } from './straighten'; +export { operationDetachNode } from './detach_node'; diff --git a/svg/iD-sprite/operations/operation-detachNode.svg b/svg/iD-sprite/operations/operation-detachNode.svg new file mode 100644 index 000000000..fea5ba873 --- /dev/null +++ b/svg/iD-sprite/operations/operation-detachNode.svg @@ -0,0 +1,61 @@ + + + + + + image/svg+xml + + + + + + + + + + diff --git a/test/index.html b/test/index.html index 188b9084c..73fd6cc59 100644 --- a/test/index.html +++ b/test/index.html @@ -1,5 +1,6 @@ + Mocha Tests @@ -7,6 +8,7 @@ +
@@ -17,9 +19,9 @@ @@ -60,6 +62,7 @@ + @@ -138,7 +141,8 @@ - + + \ No newline at end of file diff --git a/test/spec/actions/detach_node.js b/test/spec/actions/detach_node.js new file mode 100644 index 000000000..70739b640 --- /dev/null +++ b/test/spec/actions/detach_node.js @@ -0,0 +1,540 @@ +describe('iD.actionDetachNode', function () { + var tags = { 'name': 'test' }; + function createTargetNode(id, lonlat) { + return iD.Node({ id: id, loc: lonlat, tags: tags }); + } + describe('simple way', function () { + var graph; + beforeEach(function () { + // Set up a simple way + // a-b-c-d + // (0,0)-(1,0)-(2,0)-(3,0) + graph = iD.Graph([ + iD.Node({ id: 'a', loc: [0, 0] }), + iD.Node({ id: 'b', loc: [1, 0] }), + iD.Node({ id: 'c', loc: [2, 0] }), + iD.Node({ id: 'd', loc: [3, 0] }), + iD.Way({ id: 'w', nodes: ['a', 'b', 'c', 'd'] }) + ]); + }); + + describe('target in first position', function () { + beforeEach(function () { + // Swap target into the location & position of A + var targetNode = createTargetNode('a', graph.entity('a').loc); + graph = graph.replace(targetNode); + }); + + it('does not change length of way', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // Confirm that the way still has 4 nodes + var target = assertionGraph.entity('w'); + expect(target.nodes.length).to.eql(4); + }); + + it('does not change order of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // Confirm that the way is ordered correctly + var target = assertionGraph.entity('w'); + // Note that we can't be sure of the id of the replacement node + // so we only assert the nodes we know the ids for + // As we have already confirmed the size of the array we can assume + // that the replacement node is in the correct posisiton by a process of elimination + expect(target.nodes[1]).to.eql('b'); + expect(target.nodes[2]).to.eql('c'); + expect(target.nodes[3]).to.eql('d'); + }); + + it('does not change location of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // Confirm that the nodes have not moved, including the replacement node + var nodes = assertionGraph.entity('w').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([0, 0]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([1, 0]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([2, 0]); + expect(assertionGraph.entity(nodes[3]).loc).to.eql([3, 0]); + }); + + it('does replace target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + var nodes = assertionGraph.entity('w').nodes; + // Confirm that the target is no longer "a" + expect(nodes[0]).not.to.eql('a'); + // and that the tags are not present + expect(assertionGraph.entity(nodes[0]).tags).to.eql({}); + }); + + it('does detach target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // confirm that a still exists + var targetNode = assertionGraph.entity('a'); + expect(targetNode).not.to.eql(undefined); + // .., and that the location is correct + expect(targetNode.loc).to.eql([0, 0]); + // ... and that the tags are intact + expect(targetNode.tags).to.eql(tags); + // ... and that the parentWay is empty + expect(assertionGraph.parentWays(targetNode)).to.eql([]); + }); + }); + + describe('target in second position', function () { + beforeEach(function () { + // Swap target into the location & position of B + var targetNode = createTargetNode('b', graph.entity('b').loc); + graph = graph.replace(targetNode); + }); + + it('does not change length of way', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Confirm that the way still has 4 nodes + var target = assertionGraph.entity('w'); + expect(target.nodes.length).to.eql(4); + }); + + it('does not change order of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Confirm that the way is ordered correctly + var target = assertionGraph.entity('w'); + // Note that we can't be sure of the id of the replacement node + // so we only assert the nodes we know the ids for + // As we have already confirmed the size of the array we can assume + // that the replacement node is in the correct posisiton by a process of elimination + expect(target.nodes[0]).to.eql('a'); + expect(target.nodes[2]).to.eql('c'); + expect(target.nodes[3]).to.eql('d'); + }); + + it('does not change location of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Confirm that the nodes have not moved, including the replacement node + var nodes = assertionGraph.entity('w').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([0, 0]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([1, 0]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([2, 0]); + expect(assertionGraph.entity(nodes[3]).loc).to.eql([3, 0]); + }); + + it('does replace target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + var nodes = assertionGraph.entity('w').nodes; + // Confirm that the target is no longer "a" + expect(nodes[1]).not.to.eql('b'); + // and that the tags are not present + expect(assertionGraph.entity(nodes[1]).tags).to.eql({}); + }); + + it('does detach target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // confirm that a still exists + var targetNode = assertionGraph.entity('b'); + expect(targetNode).not.to.eql(undefined); + // .., and that the location is correct + expect(targetNode.loc).to.eql([1, 0]); + // ... and that the tags are intact + expect(targetNode.tags).to.eql(tags); + // ... and that the parentWay is empty + expect(assertionGraph.parentWays(targetNode)).to.eql([]); + }); + }); + }); + describe('closed way', function () { + var graph; + beforeEach(function () { + // Set up a closed way + // a-b (0,0)-(1,0) + // | | + // d-c (0,1)-(1,1) + graph = iD.Graph([ + iD.Node({ id: 'a', loc: [0, 0] }), + iD.Node({ id: 'b', loc: [1, 0] }), + iD.Node({ id: 'c', loc: [1, 1] }), + iD.Node({ id: 'd', loc: [0, 1] }), + iD.Way({ id: 'w', nodes: ['a', 'b', 'c', 'd', 'a'] }) + ]); + }); + + describe('target in first position', function () { + beforeEach(function () { + // Swap target into the location & position of A + var targetNode = createTargetNode('a', graph.entity('a').loc); + graph = graph.replace(targetNode); + }); + + it('does not change length of way', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // Confirm that the way still has 5 nodes + var target = assertionGraph.entity('w'); + expect(target.nodes.length).to.eql(5); + }); + + it('does not change order of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // Confirm that the way is ordered correctly + var target = assertionGraph.entity('w'); + // Note that we can't be sure of the id of the replacement node + // so we only assert the nodes we know the ids for + // As we have already confirmed the size of the array we can assume + // that the replacement node is in the correct posisiton by a process of elimination + expect(target.nodes[1]).to.eql('b'); + expect(target.nodes[2]).to.eql('c'); + expect(target.nodes[3]).to.eql('d'); + // Need to confirm that the id of the first & last node is the same so that the way remains closed + expect(target.nodes[0]).to.eql(target.nodes[4]); + }); + + it('does not change location of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // Confirm that the nodes have not moved, including the replacement node + var nodes = assertionGraph.entity('w').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([0, 0]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([1, 0]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([1, 1]); + expect(assertionGraph.entity(nodes[3]).loc).to.eql([0, 1]); + // We don't need to assert node[4] location as we've already confirmed that it is the same as node 0 + }); + + it('does replace target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + var nodes = assertionGraph.entity('w').nodes; + // Confirm that the target is no longer "a" + expect(nodes[0]).not.to.eql('a'); + // .. also in the tail position + expect(nodes[4]).not.to.eql('a'); + // and that the tags are not present (already confirmed same node in position 0 & 4, so only need to check tags once) + expect(assertionGraph.entity(nodes[0]).tags).to.eql({}); + }); + + it('does detach target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('a')(graph); + + // confirm that a still exists + var targetNode = assertionGraph.entity('a'); + expect(targetNode).not.to.eql(undefined); + // .., and that the location is correct + expect(targetNode.loc).to.eql([0, 0]); + // ... and that the tags are intact + expect(targetNode.tags).to.eql(tags); + // ... and that the parentWay is empty + expect(assertionGraph.parentWays(targetNode)).to.eql([]); + }); + }); + + describe('target in second position', function () { + beforeEach(function () { + // Swap target into the location & position of B + var targetNode = createTargetNode('b', graph.entity('b').loc); + graph = graph.replace(targetNode); + }); + + it('does not change length of way', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Confirm that the way still has 5 nodes + var target = assertionGraph.entity('w'); + expect(target.nodes.length).to.eql(5); + }); + + it('does not change order of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Confirm that the way is ordered correctly + var target = assertionGraph.entity('w'); + // Note that we can't be sure of the id of the replacement node + // so we only assert the nodes we know the ids for + // As we have already confirmed the size of the array we can assume + // that the replacement node is in the correct posisiton by a process of elimination + expect(target.nodes[0]).to.eql('a'); + expect(target.nodes[2]).to.eql('c'); + expect(target.nodes[3]).to.eql('d'); + expect(target.nodes[4]).to.eql('a'); + }); + + it('does not change location of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Confirm that the nodes have not moved, including the replacement node + var nodes = assertionGraph.entity('w').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([0, 0]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([1, 0]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([1, 1]); + expect(assertionGraph.entity(nodes[3]).loc).to.eql([0, 1]); + // Confirmed already that node[4] is node[0] so no further assertion needed + }); + + it('does replace target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + var nodes = assertionGraph.entity('w').nodes; + // Confirm that the target is no longer "a" + expect(nodes[1]).not.to.eql('b'); + // and that the tags are not present + expect(assertionGraph.entity(nodes[1]).tags).to.eql({}); + }); + + it('does detach target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('b')(graph); + + // confirm that a still exists + var targetNode = assertionGraph.entity('b'); + expect(targetNode).not.to.eql(undefined); + // .., and that the location is correct + expect(targetNode.loc).to.eql([1, 0]); + // ... and that the tags are intact + expect(targetNode.tags).to.eql(tags); + // ... and that the parentWay is empty + expect(assertionGraph.parentWays(targetNode)).to.eql([]); + }); + }); + }); + describe('intersecting simple ways', function () { + var graph; + beforeEach(function () { + // Set up two simple ways + // a-b-c-d (0,0)-(1,0)-(2,0)-(3,0) + // e (2,1) + // f (2,2) + // Node c represents the target + graph = iD.Graph([ + iD.Node({ id: 'a', loc: [0, 0] }), + iD.Node({ id: 'b', loc: [1, 0] }), + iD.Node({ id: 'c', loc: [2, 0], tags: tags }), + iD.Node({ id: 'd', loc: [3, 0] }), + iD.Node({ id: 'e', loc: [2, 1] }), + iD.Node({ id: 'f', loc: [2, 2] }), + iD.Way({ id: 'w', nodes: ['a', 'b', 'c', 'd'] }), + iD.Way({ id: 'x', nodes: ['c', 'e', 'f'] }) + ]); + }); + + it('does not change length of ways', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // Confirm that the way still has 4 nodes + var target = assertionGraph.entity('w'); + expect(target.nodes.length).to.eql(4); + // .. and second way has 3 + target = assertionGraph.entity('x'); + expect(target.nodes.length).to.eql(3); + }); + + it('does not change order of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // Confirm that the way is ordered correctly + var target = assertionGraph.entity('w'); + // Note that we can't be sure of the id of the replacement node + // so we only assert the nodes we know the ids for + // As we have already confirmed the size of the array we can assume + // that the replacement node is in the correct posisiton by a process of elimination + expect(target.nodes[0]).to.eql('a'); + expect(target.nodes[1]).to.eql('b'); + expect(target.nodes[3]).to.eql('d'); + // and second way + target = assertionGraph.entity('x'); + expect(target.nodes[1]).to.eql('e'); + expect(target.nodes[2]).to.eql('f'); + }); + + it('does not change location of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // Confirm that the nodes have not moved, including the replacement node + var nodes = assertionGraph.entity('w').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([0, 0]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([1, 0]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([2, 0]); + expect(assertionGraph.entity(nodes[3]).loc).to.eql([3, 0]); + // and second way + nodes = assertionGraph.entity('x').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([2, 0]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([2, 1]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([2, 2]); + }); + + it('uses same replacement node at intersection', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + // Confirm both ways have the same replacement node + expect(assertionGraph.entity('w').nodes[2]).to.eql(assertionGraph.entity('x').nodes[0]); + }); + + it('does replace target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + var nodes = assertionGraph.entity('w').nodes; + // Confirm that the target is no longer "c" + expect(nodes[2]).not.to.eql('c'); + // and that the tags are not present + expect(assertionGraph.entity(nodes[2]).tags).to.eql({}); + // Confirm that the second way's first node is the same + expect(assertionGraph.entity('x').nodes[0]).to.eql(nodes[2]); + }); + + it('does detach target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // confirm that a still exists + var targetNode = assertionGraph.entity('c'); + expect(targetNode).not.to.eql(undefined); + // .., and that the location is correct + expect(targetNode.loc).to.eql([2, 0]); + // ... and that the tags are intact + expect(targetNode.tags).to.eql(tags); + // ... and that the parentWay is empty + expect(assertionGraph.parentWays(targetNode)).to.eql([]); + }); + }); + describe('intersecting closed way', function () { + var graph; + beforeEach(function () { + // Set up two intersecting closed ways + // a-b (0,0)-(1,0) + // | | + // d-c-e (0,1)-(1,1)-(2,1) + // | | + // g f (0,2) - (1,2) + // C is the target node + graph = iD.Graph([ + iD.Node({ id: 'a', loc: [0, 0] }), + iD.Node({ id: 'b', loc: [1, 0] }), + iD.Node({ id: 'c', loc: [1, 1], tags: tags }), + iD.Node({ id: 'd', loc: [0, 1] }), + iD.Node({ id: 'e', loc: [2, 1] }), + iD.Node({ id: 'f', loc: [1, 2] }), + iD.Node({ id: 'g', loc: [0, 2] }), + iD.Way({ id: 'w', nodes: ['a', 'b', 'c', 'd', 'a'] }), + iD.Way({ id: 'x', nodes: ['c', 'e', 'f', 'g', 'c'] }) + ]); + }); + + it('does not change length of ways', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // Confirm that the way still has 5 nodes + var target = assertionGraph.entity('w'); + expect(target.nodes.length).to.eql(5); + // and the second + target = assertionGraph.entity('x'); + expect(target.nodes.length).to.eql(5); + }); + + it('does not change order of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // Confirm that the way is ordered correctly + var target = assertionGraph.entity('w'); + // Note that we can't be sure of the id of the replacement node + // so we only assert the nodes we know the ids for + // As we have already confirmed the size of the array we can assume + // that the replacement node is in the correct posisiton by a process of elimination + expect(target.nodes[0]).to.eql('a'); + expect(target.nodes[1]).to.eql('b'); + expect(target.nodes[3]).to.eql('d'); + // Need to confirm that the id of the first & last node is the same so that the way remains closed + expect(target.nodes[0]).to.eql(target.nodes[4]); + // and the same for the other way + target = assertionGraph.entity('x'); + expect(target.nodes[1]).to.eql('e'); + expect(target.nodes[2]).to.eql('f'); + expect(target.nodes[3]).to.eql('g'); + expect(target.nodes[0]).to.eql(target.nodes[4]); + }); + + it('does not change location of nodes', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // Confirm that the nodes have not moved, including the replacement node + var nodes = assertionGraph.entity('w').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([0, 0]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([1, 0]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([1, 1]); + expect(assertionGraph.entity(nodes[3]).loc).to.eql([0, 1]); + // We don't need to assert node[4] location as we've already confirmed that it is the same as node 0 + // and the other way + nodes = assertionGraph.entity('x').nodes; + expect(assertionGraph.entity(nodes[0]).loc).to.eql([1, 1]); + expect(assertionGraph.entity(nodes[1]).loc).to.eql([2, 1]); + expect(assertionGraph.entity(nodes[2]).loc).to.eql([1, 2]); + expect(assertionGraph.entity(nodes[3]).loc).to.eql([0, 2]); + }); + + it('uses same replacement node at intersection', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + // Confirm both ways have the same replacement node + expect(assertionGraph.entity('w').nodes[2]).to.eql(assertionGraph.entity('x').nodes[0]); + }); + + it('does replace target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + var nodes = assertionGraph.entity('w').nodes; + // Confirm that the target is no longer "c" + expect(nodes[0]).not.to.eql('c'); + // .. also in the tail position + expect(nodes[4]).not.to.eql('c'); + // and that the tags are not present (already confirmed same node in position 0 & 4, so only need to check tags once) + expect(assertionGraph.entity(nodes[0]).tags).to.eql({}); + // Don't need to check for way 2 since we've already confirmed it is the same node + }); + + it('does detach target node', function () { + // Act + var assertionGraph = iD.actionDetachNode('c')(graph); + + // confirm that a still exists + var targetNode = assertionGraph.entity('c'); + expect(targetNode).not.to.eql(undefined); + // .., and that the location is correct + expect(targetNode.loc).to.eql([1, 1]); + // ... and that the tags are intact + expect(targetNode.tags).to.eql(tags); + // ... and that the parentWay is empty + expect(assertionGraph.parentWays(targetNode)).to.eql([]); + }); + }); +}); \ No newline at end of file From 3b6b4bb7be3fd1d02d8ca881733e7e6cc13ab816 Mon Sep 17 00:00:00 2001 From: Jon D Date: Sat, 7 Jul 2018 10:37:58 +0100 Subject: [PATCH 2/5] Clean up SVG --- .../operations/operation-detachNode.svg | 66 ++----------------- 1 file changed, 7 insertions(+), 59 deletions(-) diff --git a/svg/iD-sprite/operations/operation-detachNode.svg b/svg/iD-sprite/operations/operation-detachNode.svg index fea5ba873..504acc2cb 100644 --- a/svg/iD-sprite/operations/operation-detachNode.svg +++ b/svg/iD-sprite/operations/operation-detachNode.svg @@ -1,61 +1,9 @@ - + + - - - - image/svg+xml - - - - - - - - - + xmlns="http://www.w3.org/2000/svg" version="1.1" x="0" y="0" width="20" height="20" viewBox="0 0 20 20" id="svg838"> + + + + From 3fe6f16526a4421db684b398b2efc63341f1d648 Mon Sep 17 00:00:00 2001 From: Jon D Date: Sat, 7 Jul 2018 10:50:44 +0100 Subject: [PATCH 3/5] Fix keyboard shortcut clash --- data/core.yaml | 2 +- dist/locales/en.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data/core.yaml b/data/core.yaml index 820e0fc0d..20d827514 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -231,7 +231,7 @@ en: delete: Deleted a turn restriction detachNode: title: Detach - key: D + key: T description: Detach this node from these lines/areas. annotation: Detached a node from owning lines/areas. restriction: diff --git a/dist/locales/en.json b/dist/locales/en.json index 8abaa9db5..15dbd00c9 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -300,7 +300,7 @@ }, "detachNode": { "title": "Detach", - "key": "D", + "key": "T", "description": "Detach this node from these lines/areas.", "annotation": "Detached a node from owning lines/areas." } From dd3de593ca85653ab82d0f3162df75320f34e887 Mon Sep 17 00:00:00 2001 From: Jon D Date: Sat, 7 Jul 2018 11:12:41 +0100 Subject: [PATCH 4/5] Add tests for Detach Node operation --- test/index.html | 1 + test/spec/operations/detach_node.js | 85 +++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 test/spec/operations/detach_node.js diff --git a/test/index.html b/test/index.html index 73fd6cc59..f0a1f7189 100644 --- a/test/index.html +++ b/test/index.html @@ -140,6 +140,7 @@ + diff --git a/test/spec/operations/detach_node.js b/test/spec/operations/detach_node.js new file mode 100644 index 000000000..6604ce051 --- /dev/null +++ b/test/spec/operations/detach_node.js @@ -0,0 +1,85 @@ +describe('iD.operationDetachNode', function () { + var fakeContext; + var graph; + var fakeTags = { 'name': 'fake' }; + beforeEach(function () { + // Set up graph + var createFakeNode = function (id, hasTags) { + return hasTags + ? { id: id, type: 'node', tags: fakeTags } + : { id: id, type: 'node' }; + }; + // a - node with tags & parent way + // b - node with tags & 2 parent ways + // c - node with no tags, parent way + // d - node with no tags, 2 parent ways + // e - node with tags, no parent way + // f - node with no tags, no parent way + graph = iD.Graph([ + iD.Node(createFakeNode('a', true)), + iD.Node(createFakeNode('b', true)), + iD.Node(createFakeNode('c', false)), + iD.Node(createFakeNode('d', false)), + iD.Node(createFakeNode('e', true)), + iD.Node(createFakeNode('f', false)), + iD.Way({ id: 'x', nodes: ['a', 'b', 'c', 'd'] }), + iD.Way({ id: 'y', nodes: ['b', 'd'] }) + ]); + + // Set up the fake context + fakeContext = {}; + fakeContext.graph = function () { + return graph; + }; + }); + + it('is not available for no selected ids', function () { + var result = iD.operationDetachNode([], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for two selected ids', function () { + var result = iD.operationDetachNode(['a', 'b'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for unkown selected id', function () { + var result = iD.operationDetachNode(['z'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected way', function () { + var result = iD.operationDetachNode(['x'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with tags, no parent way', function () { + var result = iD.operationDetachNode(['e'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with no tags, no parent way', function () { + var result = iD.operationDetachNode(['f'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with no tags, parent way', function () { + var result = iD.operationDetachNode(['c'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with no tags, two parent ways', function () { + var result = iD.operationDetachNode(['d'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is available for selected node with tags, parent way', function () { + var result = iD.operationDetachNode(['a'], fakeContext).available(); + expect(result).to.eql(true); + }); + + it('is available for selected node with tags, two parent ways', function () { + var result = iD.operationDetachNode(['b'], fakeContext).available(); + expect(result).to.eql(true); + }); +}); From 90bc0b8537b3e36b46921b157c5081b71a983a10 Mon Sep 17 00:00:00 2001 From: Jon D Date: Sun, 22 Jul 2018 19:35:29 +0100 Subject: [PATCH 5/5] Update to prevent detachment of node when either a via or location_hint role in a turn restriction. Update to move any other relation to new node. --- data/core.yaml | 3 +- dist/locales/en.json | 3 +- modules/actions/detach_node.js | 20 +- modules/operations/detach_node.js | 56 ++++- modules/util/index.js | 4 +- test/spec/actions/detach_node.js | 82 +++++++- test/spec/operations/detach_node.js | 303 +++++++++++++++++++++------- 7 files changed, 385 insertions(+), 86 deletions(-) diff --git a/data/core.yaml b/data/core.yaml index 20d827514..9f5c8eb3f 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -233,7 +233,8 @@ en: title: Detach key: T description: Detach this node from these lines/areas. - annotation: Detached a node from owning lines/areas. + annotation: Detached a node from owning lines/areas. + via_restriction: "This can't be detached because it would damage a turn restriction." restriction: controls: distance: Distance diff --git a/dist/locales/en.json b/dist/locales/en.json index 15dbd00c9..4aea68312 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -302,7 +302,8 @@ "title": "Detach", "key": "T", "description": "Detach this node from these lines/areas.", - "annotation": "Detached a node from owning lines/areas." + "annotation": "Detached a node from owning lines/areas.", + "via_restriction": "This can't be detached because it would damage a turn restriction." } }, "restriction": { diff --git a/modules/actions/detach_node.js b/modules/actions/detach_node.js index d83fd46ed..7ff5cd8e3 100644 --- a/modules/actions/detach_node.js +++ b/modules/actions/detach_node.js @@ -9,19 +9,31 @@ export function actionDetachNode(nodeId) { // Create a new node to replace the one we will detach var replacementNode = osmNode({ loc: node.loc }); // We need to process each way in turn, updating the graph as we go - return parentWays + var updatedWaysGraph = parentWays .reduce(function (accGraph, parentWay) { // Make a note of where in the way our target node is inside this way var originalIndex = parentWay.nodes.indexOf(nodeId); // Swap out the target node for the replacement - var updatedWay = parentWay - .removeNode(nodeId) // Remove our target node from the parent way + var updatedWay = parentWay.removeNode(nodeId) // Remove our target node from the parent way .addNode(replacementNode.id, originalIndex); // Add in the replacement node in its place - // Update the graph with the updated way + // Update the graph with the updated way and pass into the next cycle of the reduce operation return accGraph.replace(updatedWay); }, // Seed the reduction with the input graph, updated to include the replacementNode so // that is accessible to the ways when we add it in to them graph.replace(replacementNode)); + // Process any relations too + var parentRels = updatedWaysGraph.parentRelations(node); + return parentRels + .reduce(function (accGraph, parentRel) { + // Move the relationship to the new node + var originalMember = parentRel.memberById(nodeId); + var newMember = { id: replacementNode.id, type: 'node', role: originalMember.role }; + // Remove & replace with the new member + var updatedRel = parentRel.removeMembersWithID(nodeId) + .addMember(newMember, originalMember.index); + // Update graph and pass into the next cycle of the reduce operation + return accGraph.replace(updatedRel); + }, updatedWaysGraph); }; } diff --git a/modules/operations/detach_node.js b/modules/operations/detach_node.js index 28ca9b600..bb1f985cf 100644 --- a/modules/operations/detach_node.js +++ b/modules/operations/detach_node.js @@ -2,6 +2,8 @@ import { actionDetachNode } from '../actions/index'; import { behaviorOperation } from '../behavior/index'; import { modeMove } from '../modes/index'; import { t } from '../util/locale'; +import _flatMap from 'lodash-es/flatMap'; +import _uniq from 'lodash-es/uniq'; export function operationDetachNode(selectedIDs, context) { var selectedNode = selectedIDs[0]; @@ -37,7 +39,10 @@ export function operationDetachNode(selectedIDs, context) { return false; }; operation.tooltip = function () { - return t('operations.detachNode.description'); + var disableReason = operation.disabled(); + return disableReason + ? t('operations.detachNode.' + disableReason) + : t('operations.detachNode.description'); }; operation.annotation = function () { return t('operations.detachNode.annotation'); @@ -46,5 +51,54 @@ export function operationDetachNode(selectedIDs, context) { operation.keys = [t('operations.detachNode.key')]; operation.title = t('operations.detachNode.title'); operation.behavior = behaviorOperation(context).which(operation); + + operation.disabled = function () { + // We should prevent the node being detached if it represents a via/location_hint node of a turn restriction + var graph = context.graph(); + // Get nodes for the Ids (although there should only be one, we can handle multiple here) + var nodes = selectedIDs.map(function (i) { return graph.hasEntity(i); }) + .filter(isNotNullOrUndefined); + // Get all via nodes of restrictions involving the target nodes + var restrictionNodeIds = _flatMap(nodes, function (node) { + // Get the relations that this node belongs to + var relationsFromNode = graph.parentRelations(node); + // Check each relation in turn + return _flatMap(relationsFromNode, function (relation) { + // Check to see if this is a restriction relation, if not return null + if (!relation.isValidRestriction()) { + return null; + } + // We have identified that it is a restriction. + // https://wiki.openstreetmap.org/wiki/Relation:restriction indicates that + // from & to roles are only appropriate for Ways + // The via members can be either nodes or ways. Via-Ways do not prevent us removing a node + // from within them, as it is the way itself which is in the relation with the via role, + // and not the consitutent nodes (so if we switch out a constituent node, the way id + // does not change and therefore the relation will not be affected). Therefore we + // only need to examine the standalone nodes + return relation.members.filter(function (m) { + return (m.role === 'via' || m.role === 'location_hint') && m.type === 'node'; + }).map(function (m) { return m.id; }); + }); + }).filter(isNotNullOrUndefined); + + // Get unique list of ids in restrictionNodeIds to simplify checking + var nodeIds = _uniq(restrictionNodeIds); + + // Now we have a list of via/location_hint nodes, we should prevent detachment if the target node is in this list + var anyInhibits = nodes.filter(function (n) { + return nodeIds.indexOf(n.id) !== -1; + }); + if (anyInhibits.length > 0) { + // The node is a via/location_hint, do not permit + return 'via_restriction'; + } + // We are ok to proceed + return false; + }; return operation; } + +function isNotNullOrUndefined(i) { + return i !== undefined && i !== null; +} \ No newline at end of file diff --git a/modules/util/index.js b/modules/util/index.js index f64856de8..d25494503 100644 --- a/modules/util/index.js +++ b/modules/util/index.js @@ -12,7 +12,7 @@ export { utilFunctor } from './util'; export { utilGetAllNodes } from './util'; export { utilGetPrototypeOf } from './util'; export { utilGetSetValue } from './get_set_value'; -export { utilIdleWorker} from './idle_worker'; +export { utilIdleWorker } from './idle_worker'; export { utilNoAuto } from './util'; export { utilPrefixCSSProperty } from './util'; export { utilPrefixDOMProperty } from './util'; @@ -24,4 +24,4 @@ export { utilStringQs } from './util'; export { utilSuggestNames } from './suggest_names'; export { utilTagText } from './util'; export { utilTriggerEvent } from './trigger_event'; -export { utilWrap } from './util'; +export { utilWrap } from './util'; \ No newline at end of file diff --git a/test/spec/actions/detach_node.js b/test/spec/actions/detach_node.js index 70739b640..16fcc5e3a 100644 --- a/test/spec/actions/detach_node.js +++ b/test/spec/actions/detach_node.js @@ -79,7 +79,7 @@ describe('iD.actionDetachNode', function () { // confirm that a still exists var targetNode = assertionGraph.entity('a'); expect(targetNode).not.to.eql(undefined); - // .., and that the location is correct + // ... and that the location is correct expect(targetNode.loc).to.eql([0, 0]); // ... and that the tags are intact expect(targetNode.tags).to.eql(tags); @@ -149,7 +149,7 @@ describe('iD.actionDetachNode', function () { // confirm that a still exists var targetNode = assertionGraph.entity('b'); expect(targetNode).not.to.eql(undefined); - // .., and that the location is correct + // ... and that the location is correct expect(targetNode.loc).to.eql([1, 0]); // ... and that the tags are intact expect(targetNode.tags).to.eql(tags); @@ -240,7 +240,7 @@ describe('iD.actionDetachNode', function () { // confirm that a still exists var targetNode = assertionGraph.entity('a'); expect(targetNode).not.to.eql(undefined); - // .., and that the location is correct + // ... and that the location is correct expect(targetNode.loc).to.eql([0, 0]); // ... and that the tags are intact expect(targetNode.tags).to.eql(tags); @@ -312,7 +312,7 @@ describe('iD.actionDetachNode', function () { // confirm that a still exists var targetNode = assertionGraph.entity('b'); expect(targetNode).not.to.eql(undefined); - // .., and that the location is correct + // ... and that the location is correct expect(targetNode.loc).to.eql([1, 0]); // ... and that the tags are intact expect(targetNode.tags).to.eql(tags); @@ -416,7 +416,7 @@ describe('iD.actionDetachNode', function () { // confirm that a still exists var targetNode = assertionGraph.entity('c'); expect(targetNode).not.to.eql(undefined); - // .., and that the location is correct + // ... and that the location is correct expect(targetNode.loc).to.eql([2, 0]); // ... and that the tags are intact expect(targetNode.tags).to.eql(tags); @@ -529,7 +529,7 @@ describe('iD.actionDetachNode', function () { // confirm that a still exists var targetNode = assertionGraph.entity('c'); expect(targetNode).not.to.eql(undefined); - // .., and that the location is correct + // ... and that the location is correct expect(targetNode.loc).to.eql([1, 1]); // ... and that the tags are intact expect(targetNode.tags).to.eql(tags); @@ -537,4 +537,74 @@ describe('iD.actionDetachNode', function () { expect(assertionGraph.parentWays(targetNode)).to.eql([]); }); }); + describe('with relation', function () { + var graph; + + beforeEach(function () { + // Set up a simple way + // a-b-c (0,0)-(1,0)-(2,0) + // Node b represents the target + // With a relationship for the way including b + graph = iD.Graph([ + iD.Node({ id: 'a', loc: [0, 0] }), + iD.Node({ id: 'b', loc: [1, 0], tags: tags }), + iD.Node({ id: 'c', loc: [2, 0] }), + iD.Way({ id: 'w', nodes: ['a', 'b', 'c'] }), + iD.Relation({ + id: 'r', + tags: { + type: 'route', + route: 'foot' + }, + members: [ + { id: 'a', type: 'node', role: 'point' }, + { id: 'b', type: 'node', role: 'point' }, + { id: 'c', type: 'node', role: 'point' } + ] + }) + ]); + }); + + it('detached node not a member of relation', function () { + var assertionGraph = iD.actionDetachNode('b')(graph); + + var targetNode = assertionGraph.entity('b'); + // Confirm is not a member of the relation + expect(assertionGraph.parentRelations(targetNode).length).to.eql(0); + }); + + it('new node is a member of relation', function () { + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Find the new node + var targetWay = assertionGraph.entity('w'); + var newNodeId = targetWay.nodes.filter(function (m) { + return m !== 'a' && m !== 'b' && m !== 'c'; + })[0]; + var newNode = assertionGraph.entity(newNodeId); + + // Confirm is a member of the relation + expect(assertionGraph.parentRelations(newNode).length).to.eql(1); + expect(assertionGraph.parentRelations(newNode)[0].id).to.eql('r'); + }); + + it('Relation membership has the same properties', function () { + var assertionGraph = iD.actionDetachNode('b')(graph); + + // Find the new node + var targetWay = assertionGraph.entity('w'); + var newNodeId = targetWay.nodes.filter(function (m) { + return m !== 'a' && m !== 'b' && m !== 'c'; + })[0]; + + // Get the relation + var targetRelation = assertionGraph.entity('r'); + // Find the member + var targetMember = targetRelation.memberById(newNodeId); + + // Confirm membership is the same as original (except for the new id) + expect(targetMember).to.eql({ id: newNodeId, index: 1, type: 'node', role: 'point' }); + }); + + }); }); \ No newline at end of file diff --git a/test/spec/operations/detach_node.js b/test/spec/operations/detach_node.js index 6604ce051..357d390fa 100644 --- a/test/spec/operations/detach_node.js +++ b/test/spec/operations/detach_node.js @@ -1,85 +1,246 @@ describe('iD.operationDetachNode', function () { var fakeContext; var graph; + + // Some common setup functions + // Set up the fake context + fakeContext = {}; + fakeContext.graph = function () { + return graph; + }; var fakeTags = { 'name': 'fake' }; - beforeEach(function () { - // Set up graph - var createFakeNode = function (id, hasTags) { - return hasTags - ? { id: id, type: 'node', tags: fakeTags } - : { id: id, type: 'node' }; - }; - // a - node with tags & parent way - // b - node with tags & 2 parent ways - // c - node with no tags, parent way - // d - node with no tags, 2 parent ways - // e - node with tags, no parent way - // f - node with no tags, no parent way - graph = iD.Graph([ - iD.Node(createFakeNode('a', true)), - iD.Node(createFakeNode('b', true)), - iD.Node(createFakeNode('c', false)), - iD.Node(createFakeNode('d', false)), - iD.Node(createFakeNode('e', true)), - iD.Node(createFakeNode('f', false)), - iD.Way({ id: 'x', nodes: ['a', 'b', 'c', 'd'] }), - iD.Way({ id: 'y', nodes: ['b', 'd'] }) - ]); + // Set up graph + var createFakeNode = function (id, hasTags) { + return hasTags + ? { id: id, type: 'node', tags: fakeTags } + : { id: id, type: 'node' }; + }; - // Set up the fake context - fakeContext = {}; - fakeContext.graph = function () { - return graph; - }; + describe('available', function () { + beforeEach(function () { + // a - node with tags & parent way + // b - node with tags & 2 parent ways + // c - node with no tags, parent way + // d - node with no tags, 2 parent ways + // e - node with tags, no parent way + // f - node with no tags, no parent way + graph = iD.Graph([ + iD.Node(createFakeNode('a', true)), + iD.Node(createFakeNode('b', true)), + iD.Node(createFakeNode('c', false)), + iD.Node(createFakeNode('d', false)), + iD.Node(createFakeNode('e', true)), + iD.Node(createFakeNode('f', false)), + iD.Way({ id: 'x', nodes: ['a', 'b', 'c', 'd'] }), + iD.Way({ id: 'y', nodes: ['b', 'd'] }) + ]); + }); + + it('is not available for no selected ids', function () { + var result = iD.operationDetachNode([], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for two selected ids', function () { + var result = iD.operationDetachNode(['a', 'b'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for unkown selected id', function () { + var result = iD.operationDetachNode(['z'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected way', function () { + var result = iD.operationDetachNode(['x'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with tags, no parent way', function () { + var result = iD.operationDetachNode(['e'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with no tags, no parent way', function () { + var result = iD.operationDetachNode(['f'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with no tags, parent way', function () { + var result = iD.operationDetachNode(['c'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is not available for selected node with no tags, two parent ways', function () { + var result = iD.operationDetachNode(['d'], fakeContext).available(); + expect(result).to.eql(false); + }); + + it('is available for selected node with tags, parent way', function () { + var result = iD.operationDetachNode(['a'], fakeContext).available(); + expect(result).to.eql(true); + }); + + it('is available for selected node with tags, two parent ways', function () { + var result = iD.operationDetachNode(['b'], fakeContext).available(); + expect(result).to.eql(true); + }); }); - it('is not available for no selected ids', function () { - var result = iD.operationDetachNode([], fakeContext).available(); - expect(result).to.eql(false); - }); + describe('disabled', function () { + it('returns enabled for non-related node', function () { + graph = iD.Graph([ + iD.Node(createFakeNode('a', false)), + iD.Node(createFakeNode('b', true)), + iD.Node(createFakeNode('c', false)), + iD.Way({ id: 'x', nodes: ['a', 'b', 'c'] }) + ]); + var result = iD.operationDetachNode(['b'], fakeContext).disabled(); + expect(result).to.eql(false); + }); - it('is not available for two selected ids', function () { - var result = iD.operationDetachNode(['a', 'b'], fakeContext).available(); - expect(result).to.eql(false); - }); + it('returns enabled for non-restriction related node', function () { + graph = iD.Graph([ + iD.Node(createFakeNode('a', false)), + iD.Node(createFakeNode('b', true)), + iD.Node(createFakeNode('c', false)), + iD.Way({ id: 'x', nodes: ['a', 'b', 'c'] }), + iD.Relation({ id: 'r', members: [{ id: 'b', role: 'label' }] }) + ]); + var result = iD.operationDetachNode(['b'], fakeContext).disabled(); + expect(result).to.eql(false); + }); - it('is not available for unkown selected id', function () { - var result = iD.operationDetachNode(['z'], fakeContext).available(); - expect(result).to.eql(false); - }); + it('returns not-enabled for via node in restriction', function () { + // https://wiki.openstreetmap.org/wiki/Relation:restriction indicates that + // from & to roles are only appropriate for Ways + graph = iD.Graph([ + iD.Node(createFakeNode('a', false)), + iD.Node(createFakeNode('b', false)), + iD.Node(createFakeNode('c', false)), + iD.Node(createFakeNode('d', true)), + iD.Node(createFakeNode('e', false)), + iD.Node(createFakeNode('f', false)), + iD.Node(createFakeNode('g', false)), + iD.Way({ id: 'x', nodes: ['a', 'b', 'c'] }), + iD.Way({ id: 'y', nodes: ['e', 'f', 'g'] }), + iD.Relation({ + id: 'r', + tags: { + type: 'restriction', + restriction: 'no_right_turn' + }, + members: [ + { id: 'x', type: 'way', role: 'from' }, + { id: 'd', type: 'node', role: 'via' }, + { id: 'z', type: 'way', role: 'to' } + ] + }) + ]); + var result = iD.operationDetachNode(['d'], fakeContext).disabled(); + expect(result).not.to.eql(false); + }); - it('is not available for selected way', function () { - var result = iD.operationDetachNode(['x'], fakeContext).available(); - expect(result).to.eql(false); - }); + it('returns not-enabled for via node in restriction and other non-restriction relation', function () { + graph = iD.Graph([ + iD.Node(createFakeNode('a', false)), + iD.Node(createFakeNode('b', false)), + iD.Node(createFakeNode('c', false)), + iD.Node(createFakeNode('d', true)), + iD.Node(createFakeNode('e', false)), + iD.Node(createFakeNode('f', false)), + iD.Node(createFakeNode('g', false)), + iD.Way({ id: 'x', nodes: ['a', 'b', 'c'] }), + iD.Way({ id: 'y', nodes: ['e', 'f', 'g'] }), + iD.Relation({ + id: 'r', + tags: { + type: 'restriction', + restriction: 'no_right_turn' + }, + members: [ + { id: 'x', type: 'way', role: 'from' }, + { id: 'd', type: 'node', role: 'via' }, + { id: 'z', type: 'way', role: 'to' } + ] + }), + iD.Relation({ + id: 's', + members: [ + { id: 'x', type: 'way' }, + { id: 'd', type: 'node' }, + ] + }) + ]); + var result = iD.operationDetachNode(['d'], fakeContext).disabled(); + expect(result).not.to.eql(false); + }); - it('is not available for selected node with tags, no parent way', function () { - var result = iD.operationDetachNode(['e'], fakeContext).available(); - expect(result).to.eql(false); - }); + it('returns not-enabled for location_hint node in restriction', function () { + // https://wiki.openstreetmap.org/wiki/Relation:restriction indicates that + // from & to roles are only appropriate for Ways + graph = iD.Graph([ + iD.Node(createFakeNode('a', false)), + iD.Node(createFakeNode('b', false)), + iD.Node(createFakeNode('c', false)), + iD.Node(createFakeNode('d', true)), + iD.Node(createFakeNode('e', false)), + iD.Node(createFakeNode('f', false)), + iD.Node(createFakeNode('g', false)), + iD.Way({ id: 'x', nodes: ['a', 'b'] }), + iD.Way({ id: 'y', nodes: ['e', 'f', 'g'] }), + iD.Relation({ + id: 'r', + tags: { + type: 'restriction', + restriction: 'no_right_turn' + }, + members: [ + { id: 'x', type: 'way', role: 'from' }, + { id: 'c', type: 'node', role: 'via' }, + { id: 'd', type: 'node', role: 'location_hint' }, + { id: 'z', type: 'way', role: 'to' } + ] + }) + ]); + var result = iD.operationDetachNode(['d'], fakeContext).disabled(); + expect(result).not.to.eql(false); + }); - it('is not available for selected node with no tags, no parent way', function () { - var result = iD.operationDetachNode(['f'], fakeContext).available(); - expect(result).to.eql(false); - }); - - it('is not available for selected node with no tags, parent way', function () { - var result = iD.operationDetachNode(['c'], fakeContext).available(); - expect(result).to.eql(false); - }); - - it('is not available for selected node with no tags, two parent ways', function () { - var result = iD.operationDetachNode(['d'], fakeContext).available(); - expect(result).to.eql(false); - }); - - it('is available for selected node with tags, parent way', function () { - var result = iD.operationDetachNode(['a'], fakeContext).available(); - expect(result).to.eql(true); - }); - - it('is available for selected node with tags, two parent ways', function () { - var result = iD.operationDetachNode(['b'], fakeContext).available(); - expect(result).to.eql(true); + it('returns not-enabled for location_hint node in restriction and other non-restriction relation', function () { + graph = iD.Graph([ + iD.Node(createFakeNode('a', false)), + iD.Node(createFakeNode('b', false)), + iD.Node(createFakeNode('c', false)), + iD.Node(createFakeNode('d', true)), + iD.Node(createFakeNode('e', false)), + iD.Node(createFakeNode('f', false)), + iD.Node(createFakeNode('g', false)), + iD.Way({ id: 'x', nodes: ['a', 'b'] }), + iD.Way({ id: 'y', nodes: ['e', 'f', 'g'] }), + iD.Relation({ + id: 'r', + tags: { + type: 'restriction', + restriction: 'no_right_turn' + }, + members: [ + { id: 'x', type: 'way', role: 'from' }, + { id: 'c', type: 'node', role: 'via' }, + { id: 'd', type: 'node', role: 'location_hint' }, + { id: 'z', type: 'way', role: 'to' } + ] + }), + iD.Relation({ + id: 's', + members: [ + { id: 'x', type: 'way' }, + { id: 'd', type: 'node' }, + ] + }) + ]); + var result = iD.operationDetachNode(['d'], fakeContext).disabled(); + expect(result).not.to.eql(false); + }); }); });