From 29d608970bb0573bee1abfa2b0c41c7cce9b6b8e Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 23 Jan 2013 10:03:05 -0500 Subject: [PATCH] UnjoinNode action (fixes #442) --- js/id/actions/unjoin_node.js | 40 +++++++++++++++++ js/id/modes/select.js | 5 +++ js/id/ui/inspector.js | 11 ++++- test/index.html | 2 + test/index_packaged.html | 1 + test/spec/actions/unjoin_node.js | 77 ++++++++++++++++++++++++++++++++ 6 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 js/id/actions/unjoin_node.js create mode 100644 test/spec/actions/unjoin_node.js diff --git a/js/id/actions/unjoin_node.js b/js/id/actions/unjoin_node.js new file mode 100644 index 000000000..4bc7a5ca7 --- /dev/null +++ b/js/id/actions/unjoin_node.js @@ -0,0 +1,40 @@ +// Unjoin the ways at the given node. +// +// For testing convenience, accepts an ID to assign to the (first) new node. +// Normally, this will be undefined and the way will automatically +// be assigned a new ID. +// +// Reference: +// https://github.com/openstreetmap/potlatch2/blob/master/net/systemeD/halcyon/connection/actions/UnjoinNodeAction.as +// https://github.com/openstreetmap/josm/blob/mirror/src/org/openstreetmap/josm/actions/UnGlueAction.java +// +iD.actions.UnjoinNode = function(nodeId, newNodeId) { + var action = function(graph) { + if (!action.permitted(graph)) + return graph; + + var node = graph.entity(nodeId); + + graph.parentWays(node).forEach(function(parent, i) { + if (i === 0) + return; + + var index = parent.nodes.indexOf(nodeId), + newNode = iD.Node({id: newNodeId, loc: node.loc, tags: node.tags}), + nodes = parent.nodes.slice(); + + nodes.splice(index, 1, newNode.id); + + graph = graph.replace(newNode); + graph = graph.replace(parent.update({nodes: nodes})); + }); + + return graph; + }; + + action.permitted = function(graph) { + return graph.parentWays(graph.entity(nodeId)).length >= 2; + }; + + return action; +}; diff --git a/js/id/modes/select.js b/js/id/modes/select.js index 3a3750d36..2b92cfedd 100644 --- a/js/id/modes/select.js +++ b/js/id/modes/select.js @@ -85,6 +85,11 @@ iD.modes.Select = function(entity, initial) { iD.actions.SplitWay(d.id), 'split a way'); + }).on('unjoin', function(d) { + mode.history.perform( + iD.actions.UnjoinNode(d.id), + 'unjoined ways'); + }).on('remove', function() { remove(); diff --git a/js/id/ui/inspector.js b/js/id/ui/inspector.js index be9db53f0..058f318df 100644 --- a/js/id/ui/inspector.js +++ b/js/id/ui/inspector.js @@ -1,6 +1,6 @@ iD.ui.inspector = function() { var event = d3.dispatch('changeTags', 'reverseWay', - 'update', 'remove', 'close', 'splitWay'), + 'update', 'remove', 'close', 'splitWay', 'unjoin'), taginfo = iD.taginfo(), initial = false, tagList; @@ -58,8 +58,10 @@ iD.ui.inspector = function() { function drawButtons(selection) { var entity = selection.datum(); + var inspectorButtonWrap = selection.append('div') .attr('class','button-wrap joined fl'); + var inspectorButton1 = inspectorButtonWrap.append('button') .attr('class', 'apply col6 action') .on('click', apply); @@ -80,17 +82,24 @@ iD.ui.inspector = function() { .attr('href', 'http://www.openstreetmap.org/browse/' + entity.type + '/' + entity.osmId()) .attr('target', '_blank') .text('View on OSM'); + if (entity.type === 'way') { minorButtons.append('a') .attr('href', '#') .text('Reverse Direction') .on('click', function() { event.reverseWay(entity); }); } + if (entity.geometry() === 'vertex') { minorButtons.append('a') .attr('href', '#') .text('Split Way') .on('click', function() { event.splitWay(entity); }); + + minorButtons.append('a') + .attr('href', '#') + .text('Unjoin') + .on('click', function() { event.unjoin(entity); }); } } diff --git a/test/index.html b/test/index.html index 4099f2786..6df3944d9 100644 --- a/test/index.html +++ b/test/index.html @@ -79,6 +79,7 @@ + @@ -140,6 +141,7 @@ + diff --git a/test/index_packaged.html b/test/index_packaged.html index 8def4c0da..1432e6686 100644 --- a/test/index_packaged.html +++ b/test/index_packaged.html @@ -44,6 +44,7 @@ + diff --git a/test/spec/actions/unjoin_node.js b/test/spec/actions/unjoin_node.js new file mode 100644 index 000000000..247239f5c --- /dev/null +++ b/test/spec/actions/unjoin_node.js @@ -0,0 +1,77 @@ +describe("iD.actions.UnjoinNode", function () { + describe("#permitted", function () { + it("returns false for a node shared by less than two ways", function () { + var graph = iD.Graph({'a': iD.Node()}); + + expect(iD.actions.UnjoinNode('a').permitted(graph)).to.equal(false); + }); + + it("returns true for a node shared by two or more ways", function () { + // a ---- b ---- c + // | + // d + var graph = iD.Graph({ + 'a': iD.Node({id: 'a'}), + 'b': iD.Node({id: 'b'}), + 'c': iD.Node({id: 'c'}), + 'd': iD.Node({id: 'd'}), + '-': iD.Way({id: '-', nodes: ['a', 'b', 'c']}), + '|': iD.Way({id: '|', nodes: ['d', 'b']}) + }); + + expect(iD.actions.UnjoinNode('b').permitted(graph)).to.equal(true); + }); + }); + + it("replaces the node with a new node in all but the first way", function () { + // Situation: + // a ---- b ---- c + // | + // d + // Split at b. + // + // Expected result: + // a ---- b ---- c + // + // e + // | + // d + // + var graph = iD.Graph({ + 'a': iD.Node({id: 'a'}), + 'b': iD.Node({id: 'b'}), + 'c': iD.Node({id: 'c'}), + 'd': iD.Node({id: 'd'}), + '-': iD.Way({id: '-', nodes: ['a', 'b', 'c']}), + '|': iD.Way({id: '|', nodes: ['d', 'b']}) + }); + + graph = iD.actions.UnjoinNode('b', 'e')(graph); + + expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); + expect(graph.entity('|').nodes).to.eql(['d', 'e']); + }); + + it("copies location and tags to the new nodes", function () { + var tags = {highway: 'traffic_signals'}, + loc = [1, 2], + graph = iD.Graph({ + 'a': iD.Node({id: 'a'}), + 'b': iD.Node({id: 'b', loc: loc, tags: tags}), + 'c': iD.Node({id: 'c'}), + 'd': iD.Node({id: 'd'}), + '-': iD.Way({id: '-', nodes: ['a', 'b', 'c']}), + '|': iD.Way({id: '|', nodes: ['d', 'b']}) + }); + + graph = iD.actions.UnjoinNode('b', 'e')(graph); + + // Immutable loc => should be shared by identity. + expect(graph.entity('b').loc).to.equal(loc); + expect(graph.entity('e').loc).to.equal(loc); + + // Immutable tags => should be shared by identity. + expect(graph.entity('b').tags).to.equal(tags); + expect(graph.entity('e').tags).to.equal(tags); + }); +});