From 48dedb15ed2c54eb93baebfa07c9ac4e446f56c8 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 30 Mar 2014 22:49:19 -0400 Subject: [PATCH 1/4] Prevent a closed way from disconnecting itself.. --- js/id/actions/disconnect.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/js/id/actions/disconnect.js b/js/id/actions/disconnect.js index b9220fa69..a3afe8b4d 100644 --- a/js/id/actions/disconnect.js +++ b/js/id/actions/disconnect.js @@ -20,9 +20,10 @@ iD.actions.Disconnect = function(nodeId, newNodeId) { replacements = action.replacements(graph); replacements.forEach(function(replacement) { - var newNode = iD.Node({id: newNodeId, loc: node.loc, tags: node.tags}); + var way = graph.entity(replacement.wayID), + 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)); + graph = graph.replace(way.replaceNode(way.nodes[replacement.index], newNode.id)); }); return graph; @@ -31,21 +32,22 @@ iD.actions.Disconnect = function(nodeId, newNodeId) { action.replacements = 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) { + way.nodes.forEach(function(waynode, index) { if (waynode === nodeId) { - candidates.push({wayID: parent.id, index: index}); + candidates.push({wayID: way.id, index: index}); } }); }); + candidates = _.uniq(candidates, function(item) { return item.wayID; } ); return keeping ? candidates : candidates.slice(1); }; From 5afcbf94002670ea7443fa90c874da5c9b0e7eda Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 30 Mar 2014 23:06:39 -0400 Subject: [PATCH 2/4] Update tests to not allow closed way to disconnect itself. --- test/spec/actions/disconnect.js | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/test/spec/actions/disconnect.js b/test/spec/actions/disconnect.js index d91296c0a..bf5af5ac7 100644 --- a/test/spec/actions/disconnect.js +++ b/test/spec/actions/disconnect.js @@ -6,7 +6,7 @@ describe("iD.actions.Disconnect", function () { 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 'not_connected' for a node appearing twice in the same way", function () { // a ---- b // | | // d ---- c @@ -17,7 +17,7 @@ describe("iD.actions.Disconnect", function () { iD.Node({id: 'd'}), iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'd', 'a']}) ]); - expect(iD.actions.Disconnect('a').disabled(graph)).not.to.be.ok; + expect(iD.actions.Disconnect('a').disabled(graph)).to.equal('not_connected'); }); it("returns falsy for a node shared by two or more ways", function () { @@ -41,7 +41,7 @@ describe("iD.actions.Disconnect", function () { iD.Node({id: 'a'}), iD.Node({id: 'b'}), iD.Node({id: 'c'}), - iD.Node({id: 'c'}), + iD.Node({id: 'd'}), iD.Node({id: '*'}), iD.Way({id: '-', nodes: ['a', '*', 'b']}), iD.Way({id: '|', nodes: ['*', 'd']}) @@ -109,25 +109,6 @@ describe("iD.actions.Disconnect", function () { expect(graph.entity('|').nodes).to.eql(['d', 'b']); }); - it("replaces later occurrences in a self-intersecting way", function() { - // Situtation: - // a ---- b - // \_ | - // \__ c - // Disconnect at a - // - // Expected result: - // a ---- b ---- c ---- d - var graph = iD.Graph([ - iD.Node({id: 'a'}), - iD.Node({id: 'b'}), - iD.Node({id: 'c'}), - iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'a']}) - ]); - graph = iD.actions.Disconnect('a', 'd')(graph); - expect(graph.entity('w').nodes).to.eql(['a', 'b', 'c', 'd']); - }); - it("disconnects a way with multiple intersection points", function() { // Situtation: // a = b - c From a3a125187dd8d5854d1e020db9fd25e778eb9f26 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 1 Apr 2014 17:22:48 -0400 Subject: [PATCH 3/4] preserve disconnect for lines, improve disconnect for shared nodes in areas.. --- js/id/actions/disconnect.js | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/js/id/actions/disconnect.js b/js/id/actions/disconnect.js index a3afe8b4d..069e5919a 100644 --- a/js/id/actions/disconnect.js +++ b/js/id/actions/disconnect.js @@ -17,19 +17,26 @@ iD.actions.Disconnect = function(nodeId, newNodeId) { var action = function(graph) { var node = graph.entity(nodeId), - replacements = action.replacements(graph); + connections = action.connections(graph); - replacements.forEach(function(replacement) { - var way = graph.entity(replacement.wayID), + connections.forEach(function(connection) { + var way = graph.entity(connection.wayID), newNode = iD.Node({id: newNodeId, loc: node.loc, tags: node.tags}); + graph = graph.replace(newNode); - graph = graph.replace(way.replaceNode(way.nodes[replacement.index], newNode.id)); + 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, parentWays = graph.parentWays(graph.entity(nodeId)); @@ -39,21 +46,23 @@ iD.actions.Disconnect = function(nodeId, newNodeId) { keeping = true; return; } - - way.nodes.forEach(function(waynode, index) { - if (waynode === nodeId) { - candidates.push({wayID: way.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}); + } + }); + } }); - candidates = _.uniq(candidates, function(item) { return item.wayID; } ); 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'; }; From fcdf7bbee2c8638a866711e130dac41b41b350e8 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 1 Apr 2014 17:26:25 -0400 Subject: [PATCH 4/4] Improve test cases for disconnect action to cover shared area nodes.. --- test/spec/actions/disconnect.js | 183 +++++++++++++++++++++++++++++--- 1 file changed, 169 insertions(+), 14 deletions(-) diff --git a/test/spec/actions/disconnect.js b/test/spec/actions/disconnect.js index bf5af5ac7..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 'not_connected' 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 @@ -17,9 +16,39 @@ describe("iD.actions.Disconnect", function () { iD.Node({id: 'd'}), iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'd', 'a']}) ]); + 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: 'd'}), - iD.Node({id: '*'}), - iD.Way({id: '-', nodes: ['a', '*', 'b']}), - iD.Way({id: '|', nodes: ['*', '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'}), @@ -109,14 +141,44 @@ describe("iD.actions.Disconnect", function () { expect(graph.entity('|').nodes).to.eql(['d', 'b']); }); + it("replaces later occurrences in a self-intersecting way", function() { + // Situtation: + // a --- b + // \ / + // \ / + // c + // Disconnect at a + // + // Expected result: + // a --- b + // | + // d --- c + // + var graph = iD.Graph([ + iD.Node({id: 'a'}), + iD.Node({id: 'b'}), + iD.Node({id: 'c'}), + iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'a']}) + ]); + graph = iD.actions.Disconnect('a', 'd')(graph); + expect(graph.entity('w').nodes).to.eql(['a', 'b', 'c', 'd']); + }); + 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'}), @@ -133,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],