From 278bb4a51c4026718e7d68403e345c833f8217d2 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 23 Jan 2013 12:08:25 -0500 Subject: [PATCH] Fix some cases where a degenerate way was created --- js/id/actions/delete_node.js | 7 +++++- js/id/graph/way.js | 4 ++++ js/id/modes/draw_area.js | 37 +++++++++++++++++++++----------- js/id/modes/draw_line.js | 33 +++++++++++++++++++--------- js/id/modes/select.js | 10 +++------ test/spec/actions/delete_node.js | 18 ++++++++++++++++ test/spec/graph/way.js | 25 +++++++++++++++++++++ 7 files changed, 103 insertions(+), 31 deletions(-) diff --git a/js/id/actions/delete_node.js b/js/id/actions/delete_node.js index d600bc5fc..a93129ce3 100644 --- a/js/id/actions/delete_node.js +++ b/js/id/actions/delete_node.js @@ -5,7 +5,12 @@ iD.actions.DeleteNode = function(nodeId) { graph.parentWays(node) .forEach(function(parent) { - graph = graph.replace(parent.removeNode(nodeId)); + parent = parent.removeNode(nodeId); + graph = graph.replace(parent); + + if (parent.isDegenerate()) { + graph = iD.actions.DeleteWay(parent.id)(graph); + } }); graph.parentRelations(node) diff --git a/js/id/graph/way.js b/js/id/graph/way.js index eb3cf4087..0de0e55a1 100644 --- a/js/id/graph/way.js +++ b/js/id/graph/way.js @@ -58,6 +58,10 @@ _.extend(iD.Way.prototype, { !this.tags.barrier); }, + isDegenerate: function() { + return _.uniq(this.nodes).length < (this.isArea() ? 3 : 2); + }, + geometry: function() { return this.isArea() ? 'area' : 'line'; }, diff --git a/js/id/modes/draw_area.js b/js/id/modes/draw_area.js index a90d13c9c..494d1affb 100644 --- a/js/id/modes/draw_area.js +++ b/js/id/modes/draw_area.js @@ -12,9 +12,8 @@ iD.modes.DrawArea = function(wayId) { history = mode.history, controller = mode.controller, way = history.graph().entity(wayId), - headId = (way.nodes.length == 1) ? - way.nodes[0] : - way.nodes[way.nodes.length - 2], + index = way.nodes.length - 1, + headId = way.nodes[index - 1], tailId = way.first(), node = iD.Node({loc: map.mouseCoordinates()}); @@ -24,12 +23,20 @@ iD.modes.DrawArea = function(wayId) { history.perform( iD.actions.AddNode(node), - iD.actions.AddWayNode(way.id, node.id, -1)); + iD.actions.AddWayNode(way.id, node.id, index)); surface.selectAll('.way, .node') .filter(function (d) { return d.id === wayId || d.id === node.id; }) .classed('active', true); + function ReplaceTemporaryNode(replacementId) { + return function(graph) { + graph = graph.replace(graph.entity(wayId).updateNode(replacementId, index)); + graph = graph.remove(node); + return graph; + } + } + function mousemove() { history.replace(iD.actions.MoveNode(node.id, map.mouseCoordinates())); } @@ -55,8 +62,7 @@ iD.modes.DrawArea = function(wayId) { } else if (datum.type === 'node' && datum.id !== node.id) { // connect the way to an existing node history.replace( - iD.actions.DeleteNode(node.id), - iD.actions.AddWayNode(way.id, datum.id, -1), + ReplaceTemporaryNode(datum.id), way.nodes.length > 2 ? 'added to an area' : ''); controller.enter(iD.modes.DrawArea(wayId)); @@ -77,13 +83,11 @@ iD.modes.DrawArea = function(wayId) { iD.actions.DeleteNode(node.id), iD.actions.DeleteNode(headId)); - if (history.graph().fetch(wayId).nodes.length === 2) { - history.replace( - iD.actions.DeleteNode(way.nodes[0]), - iD.actions.DeleteWay(wayId)); - controller.enter(iD.modes.Browse()); - } else { + if (history.graph().entity(wayId)) { controller.enter(iD.modes.DrawArea(wayId)); + } else { + // The way was deleted because it had too few nodes. + controller.enter(iD.modes.Browse()); } } @@ -95,8 +99,15 @@ iD.modes.DrawArea = function(wayId) { function ret() { d3.event.preventDefault(); + history.replace(iD.actions.DeleteNode(node.id)); - controller.enter(iD.modes.Select(way, true)); + + if (history.graph().entity(wayId)) { + controller.enter(iD.modes.Select(way, true)); + } else { + // The way was deleted because it had too few nodes. + controller.enter(iD.modes.Browse()); + } } surface diff --git a/js/id/modes/draw_line.js b/js/id/modes/draw_line.js index 3b3892aa6..b8fabeca5 100644 --- a/js/id/modes/draw_line.js +++ b/js/id/modes/draw_line.js @@ -13,7 +13,7 @@ iD.modes.DrawLine = function(wayId, direction) { controller = mode.controller, way = history.graph().entity(wayId), node = iD.Node({loc: map.mouseCoordinates()}), - index = (direction === 'forward') ? undefined : 0, + index = (direction === 'forward') ? way.nodes.length : 0, headId = (direction === 'forward') ? way.last() : way.first(), tailId = (direction === 'forward') ? way.first() : way.last(); @@ -35,6 +35,14 @@ iD.modes.DrawLine = function(wayId, direction) { .filter(function (d) { return d.id === wayId || d.id === node.id; }) .classed('active', true); + function ReplaceTemporaryNode(replacementId) { + return function(graph) { + graph = graph.replace(graph.entity(wayId).updateNode(replacementId, index)); + graph = graph.remove(node); + return graph; + } + } + function mousemove() { history.replace(iD.actions.MoveNode(node.id, map.mouseCoordinates())); } @@ -46,8 +54,7 @@ iD.modes.DrawLine = function(wayId, direction) { // connect the way in a loop if (way.nodes.length > 2) { history.replace( - iD.actions.DeleteNode(node.id), - iD.actions.AddWayNode(wayId, tailId, index), + ReplaceTemporaryNode(tailId), 'added to a line'); controller.enter(iD.modes.Select(way, true)); @@ -66,8 +73,7 @@ iD.modes.DrawLine = function(wayId, direction) { } else if (datum.type === 'node' && datum.id !== node.id) { // connect the way to an existing node history.replace( - iD.actions.DeleteNode(node.id), - iD.actions.AddWayNode(wayId, datum.id, index), + ReplaceTemporaryNode(datum.id), 'added to a line'); controller.enter(iD.modes.DrawLine(wayId, direction)); @@ -106,11 +112,11 @@ iD.modes.DrawLine = function(wayId, direction) { iD.actions.DeleteNode(node.id), iD.actions.DeleteNode(headId)); - if (history.graph().fetch(wayId).nodes.length === 0) { - history.replace(iD.actions.DeleteWay(wayId)); - controller.enter(iD.modes.Browse()); - } else { + if (history.graph().entity(wayId)) { controller.enter(iD.modes.DrawLine(wayId, direction)); + } else { + // The way was deleted because it had too few nodes. + controller.enter(iD.modes.Browse()); } } @@ -122,8 +128,15 @@ iD.modes.DrawLine = function(wayId, direction) { function ret() { d3.event.preventDefault(); + history.replace(iD.actions.DeleteNode(node.id)); - controller.enter(iD.modes.Select(way, true)); + + if (history.graph().entity(wayId)) { + controller.enter(iD.modes.Select(way, true)); + } else { + // The way was deleted because it had too few nodes. + controller.enter(iD.modes.Browse()); + } } surface diff --git a/js/id/modes/select.js b/js/id/modes/select.js index 2b92cfedd..d80d6fffd 100644 --- a/js/id/modes/select.js +++ b/js/id/modes/select.js @@ -15,13 +15,9 @@ iD.modes.Select = function(entity, initial) { iD.actions.DeleteWay(entity.id), 'deleted a way'); } else if (entity.type === 'node') { - var parents = mode.history.graph().parentWays(entity), - operations = [iD.actions.DeleteNode(entity.id)]; - parents.forEach(function(parent) { - if (_.uniq(parent.nodes).length === 1) operations.push(iD.actions.DeleteWay(parent.id)); - }); - mode.history.perform.apply(mode.history, - operations.concat(['deleted a node'])); + mode.history.perform( + iD.actions.DeleteNode(entity.id), + 'deleted a node'); } mode.controller.exit(); diff --git a/test/spec/actions/delete_node.js b/test/spec/actions/delete_node.js index a73f7e33f..93860117f 100644 --- a/test/spec/actions/delete_node.js +++ b/test/spec/actions/delete_node.js @@ -24,4 +24,22 @@ describe("iD.actions.DeleteNode", function () { graph = action(iD.Graph([node1, node2, relation])); expect(graph.entity(relation.id).members).to.eql([{ id: node2.id }]); }); + + it("deletes parent ways that would otherwise have less than two nodes", function () { + var node1 = iD.Node(), + node2 = iD.Node(), + way = iD.Way({nodes: [node1.id, node2.id]}), + action = iD.actions.DeleteNode(node1.id), + graph = action(iD.Graph([node1, node2, way])); + expect(graph.entity(way.id)).to.be.undefined; + }); + + it("deletes degenerate circular ways", function () { + var node1 = iD.Node(), + node2 = iD.Node(), + way = iD.Way({nodes: [node1.id, node2.id, node1.id]}), + action = iD.actions.DeleteNode(node2.id), + graph = action(iD.Graph([node1, node2, way])); + expect(graph.entity(way.id)).to.be.undefined; + }); }); diff --git a/test/spec/graph/way.js b/test/spec/graph/way.js index 0c70ea692..87d7d0029 100644 --- a/test/spec/graph/way.js +++ b/test/spec/graph/way.js @@ -117,6 +117,31 @@ describe('iD.Way', function() { }); }); + describe("#isDegenerate", function() { + it("returns true for a linear way with zero or one nodes", function () { + expect(iD.Way({nodes: []}).isDegenerate()).to.equal(true); + expect(iD.Way({nodes: ['a']}).isDegenerate()).to.equal(true); + }); + + it("returns true for a circular way with only one unique node", function () { + expect(iD.Way({nodes: ['a', 'a']}).isDegenerate()).to.equal(true); + }); + + it("returns false for a linear way with two or more nodes", function () { + expect(iD.Way({nodes: ['a', 'b']}).isDegenerate()).to.equal(false); + }); + + it("returns true for an area with zero, one, or two unique nodes", function () { + expect(iD.Way({tags: {area: 'yes'}, nodes: []}).isDegenerate()).to.equal(true); + expect(iD.Way({tags: {area: 'yes'}, nodes: ['a', 'a']}).isDegenerate()).to.equal(true); + expect(iD.Way({tags: {area: 'yes'}, nodes: ['a', 'b', 'a']}).isDegenerate()).to.equal(true); + }); + + it("returns false for an area with three or more unique nodes", function () { + expect(iD.Way({tags: {area: 'yes'}, nodes: ['a', 'b', 'c', 'a']}).isDegenerate()).to.equal(false); + }); + }); + describe("#geometry", function() { it("returns 'line' when the way is not an area", function () { expect(iD.Way().geometry()).to.equal('line');