diff --git a/js/id/actions/disconnect.js b/js/id/actions/disconnect.js index b9220fa69..069e5919a 100644 --- a/js/id/actions/disconnect.js +++ b/js/id/actions/disconnect.js @@ -17,41 +17,52 @@ iD.actions.Disconnect = function(nodeId, newNodeId) { var action = function(graph) { var node = graph.entity(nodeId), - replacements = action.replacements(graph); + connections = action.connections(graph); + + connections.forEach(function(connection) { + var way = graph.entity(connection.wayID), + newNode = iD.Node({id: newNodeId, loc: node.loc, tags: node.tags}); - replacements.forEach(function(replacement) { - var newNode = iD.Node({id: newNodeId, loc: node.loc, tags: node.tags}); graph = graph.replace(newNode); - graph = graph.replace(graph.entity(replacement.wayID).updateNode(newNode.id, replacement.index)); + if (connection.index === 0 && way.isArea()) { + // replace shared node with shared node.. + graph = graph.replace(way.replaceNode(way.nodes[0], newNode.id)); + } else { + // replace shared node with multiple new nodes.. + graph = graph.replace(way.updateNode(newNode.id, connection.index)); + } }); return graph; }; - action.replacements = function(graph) { + action.connections = function(graph) { var candidates = [], keeping = false, - parents = graph.parentWays(graph.entity(nodeId)); + parentWays = graph.parentWays(graph.entity(nodeId)); - parents.forEach(function(parent) { - if (wayIds && wayIds.indexOf(parent.id) === -1) { + parentWays.forEach(function(way) { + if (wayIds && wayIds.indexOf(way.id) === -1) { keeping = true; return; } - - parent.nodes.forEach(function(waynode, index) { - if (waynode === nodeId) { - candidates.push({wayID: parent.id, index: index}); - } - }); + if (way.isArea() && (way.nodes[0] === nodeId)) { + candidates.push({wayID: way.id, index: 0}); + } else { + way.nodes.forEach(function(waynode, index) { + if (waynode === nodeId) { + candidates.push({wayID: way.id, index: index}); + } + }); + } }); return keeping ? candidates : candidates.slice(1); }; action.disabled = function(graph) { - var replacements = action.replacements(graph); - if (replacements.length === 0 || (wayIds && wayIds.length !== replacements.length)) + var connections = action.connections(graph); + if (connections.length === 0 || (wayIds && wayIds.length !== connections.length)) return 'not_connected'; }; diff --git a/test/spec/actions/disconnect.js b/test/spec/actions/disconnect.js index d91296c0a..f8851e4cc 100644 --- a/test/spec/actions/disconnect.js +++ b/test/spec/actions/disconnect.js @@ -2,11 +2,10 @@ describe("iD.actions.Disconnect", function () { describe("#disabled", function () { it("returns 'not_connected' for a node shared by less than two ways", function () { var graph = iD.Graph([iD.Node({id: 'a'})]); - expect(iD.actions.Disconnect('a').disabled(graph)).to.equal('not_connected'); }); - it("returns falsy for a node appearing twice in the same way", function () { + it("returns falsy for the closing node in a closed line", function () { // a ---- b // | | // d ---- c @@ -20,6 +19,36 @@ describe("iD.actions.Disconnect", function () { expect(iD.actions.Disconnect('a').disabled(graph)).not.to.be.ok; }); + it("returns not_connected for the closing node in a closed area", function () { + // a ---- b + // | | + // d ---- c + var graph = iD.Graph([ + iD.Node({id: 'a'}), + iD.Node({id: 'b'}), + iD.Node({id: 'c'}), + iD.Node({id: 'd'}), + iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'd', 'a'], tags: {area: 'yes'}}) + ]); + expect(iD.actions.Disconnect('a').disabled(graph)).to.equal('not_connected'); + }); + + it("returns falsy for a shared non-closing node in an area", function () { + // a --- b --- c + // | | + // e --- d + var graph = iD.Graph([ + iD.Node({id: 'a'}), + iD.Node({id: 'b'}), + iD.Node({id: 'c'}), + iD.Node({id: 'd'}), + iD.Node({id: 'e'}), + iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'd', 'e', 'b', 'a'], tags: {area: 'yes'}}) + ]); + + expect(iD.actions.Disconnect('b').disabled(graph)).not.to.be.ok; + }); + it("returns falsy for a node shared by two or more ways", function () { // a ---- b ---- c // | @@ -37,18 +66,21 @@ describe("iD.actions.Disconnect", function () { }); it("returns falsy for an intersection of two ways with parent way specified", function () { + // a ---- b ---- c + // | + // d var graph = iD.Graph([ iD.Node({id: 'a'}), iD.Node({id: 'b'}), iD.Node({id: 'c'}), - iD.Node({id: 'c'}), - iD.Node({id: '*'}), - iD.Way({id: '-', nodes: ['a', '*', 'b']}), - iD.Way({id: '|', nodes: ['*', 'd']}) + iD.Node({id: 'd'}), + iD.Way({id: '-', nodes: ['a', 'b', 'c']}), + iD.Way({id: '|', nodes: ['d', 'b']}) ]); - expect(iD.actions.Disconnect('*', ['|']).disabled(graph)).not.to.be.ok; + expect(iD.actions.Disconnect('b', ['|']).disabled(graph)).not.to.be.ok; }); + }); it("replaces the node with a new node in all but the first way", function () { @@ -88,9 +120,9 @@ describe("iD.actions.Disconnect", function () { // Disconnect - at b. // // Expected result: - // a ---- e b ==== c - // | - // d + // a ---- e b ==== c + // | + // d // var graph = iD.Graph([ iD.Node({id: 'a'}), @@ -111,13 +143,17 @@ describe("iD.actions.Disconnect", function () { it("replaces later occurrences in a self-intersecting way", function() { // Situtation: - // a ---- b - // \_ | - // \__ c - // Disconnect at a + // a --- b + // \ / + // \ / + // c + // Disconnect at a // // Expected result: - // a ---- b ---- c ---- d + // a --- b + // | + // d --- c + // var graph = iD.Graph([ iD.Node({id: 'a'}), iD.Node({id: 'b'}), @@ -130,12 +166,19 @@ describe("iD.actions.Disconnect", function () { it("disconnects a way with multiple intersection points", function() { // Situtation: - // a = b - c - // | | - // e - d - // Where b starts/ends -. + // a == b -- c + // | | + // e -- d + // 2 ways a-b and b-c-d-e-b + // // Disconnect at b - + // + // Expected Result: + // a == b * -- c + // | | + // e -- d + // 2 ways a-b and *-c-d-e-* + // var graph = iD.Graph([ iD.Node({id: 'a'}), iD.Node({id: 'b'}), @@ -152,6 +195,99 @@ describe("iD.actions.Disconnect", function () { expect(graph.entity('w2').nodes).to.eql(['*', 'c', 'd', 'e', '*']); }); + it("disconnects a shared non-closing node in an area", function() { + // Situtation: + // a -- b -- c + // | | + // e -- d + // + // Disconnect at b + // + // Expected Result: + // a -- b -- c + // | | + // * -- e -- d + // + var graph = iD.Graph([ + iD.Node({id: 'a'}), + iD.Node({id: 'b'}), + iD.Node({id: 'c'}), + iD.Node({id: 'd'}), + iD.Node({id: 'e'}), + iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'd', 'e', 'b', 'a'], tags: {area: 'yes'}}) + ]); + + graph = iD.actions.Disconnect('b', '*')(graph); + + expect(graph.entity('w').nodes).to.eql(['a', 'b', 'c', 'd', 'e', '*', 'a']); + }); + + it("disconnects the closing node of an area without breaking the area", function() { + // Situtation: + // a --- b --- d + // \ / \ / + // \ / \ / + // c e + // 2 areas: a-b-c-a and b-d-e-b + // + // Disconnect at b + // + // Expected Result: + // a --- b * --- d + // \ / \ / + // \ / \ / + // c e + // 2 areas: a-b-c-a and *-d-e-* + + var graph = iD.Graph([ + iD.Node({id: 'a'}), + iD.Node({id: 'b'}), + iD.Node({id: 'c'}), + iD.Node({id: 'd'}), + iD.Node({id: 'e'}), + iD.Way({id: 'w1', nodes: ['a', 'b', 'c', 'a'], tags: {area: 'yes'}}), + iD.Way({id: 'w2', nodes: ['b', 'd', 'e', 'b'], tags: {area: 'yes'}}) + ]); + + graph = iD.actions.Disconnect('b', '*')(graph); + + expect(graph.entity('w1').nodes).to.eql(['a', 'b', 'c', 'a']); + expect(graph.entity('w2').nodes).to.eql(['*', 'd', 'e', '*']); + }); + + it("disconnects multiple closing nodes of multiple areas without breaking the areas", function() { + // Situtation: + // a --- b --- d + // \ / \ / + // \ / \ / + // c e + // 2 areas: b-c-a-b and b-d-e-b + // + // Disconnect at b + // + // Expected Result: + // a --- b * --- d + // \ / \ / + // \ / \ / + // c e + // 2 areas: b-c-a-b and *-d-e-* + + var graph = iD.Graph([ + iD.Node({id: 'a'}), + iD.Node({id: 'b'}), + iD.Node({id: 'c'}), + iD.Node({id: 'd'}), + iD.Node({id: 'e'}), + iD.Way({id: 'w1', nodes: ['b', 'c', 'a', 'b'], tags: {area: 'yes'}}), + iD.Way({id: 'w2', nodes: ['b', 'd', 'e', 'b'], tags: {area: 'yes'}}) + ]); + + graph = iD.actions.Disconnect('b', '*')(graph); + + expect(graph.entity('w1').nodes).to.eql(['b', 'c', 'a', 'b']); + expect(graph.entity('w2').nodes).to.eql(['*', 'd', 'e', '*']); + }); + it("copies location and tags to the new nodes", function () { var tags = {highway: 'traffic_signals'}, loc = [1, 2],