From 79404c47d9bdca0611f8d5680dacdf6f760f48c9 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 1 Apr 2013 15:46:13 -0700 Subject: [PATCH] Multiselect control over disconnection (#1220) --- js/id/actions/disconnect.js | 58 ++++++++++++++++++--------------- js/id/operations/disconnect.js | 11 ++++--- test/spec/actions/disconnect.js | 53 +++++++++++++++++++++++++++--- 3 files changed, 87 insertions(+), 35 deletions(-) diff --git a/js/id/actions/disconnect.js b/js/id/actions/disconnect.js index b8f48ea2d..67336d950 100644 --- a/js/id/actions/disconnect.js +++ b/js/id/actions/disconnect.js @@ -1,5 +1,7 @@ // Disconect the ways at the given node. // +// Optionally, disconnect only the given ways. +// // 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. @@ -10,40 +12,44 @@ // 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.Disconnect = function(nodeId, newNodeId) { +iD.actions.Disconnect = function(nodeId, wayIds, newNodeId) { var action = function(graph) { - var node = graph.entity(nodeId); + var node = graph.entity(nodeId), + replacements = action.replacements(graph); - graph.parentWays(node).forEach(function(parent, i) { - - var keep = i === 0; - - parent.nodes.forEach(function(waynode, index) { - if (waynode === nodeId) { - - if (!keep) { - var newNode = iD.Node({id: newNodeId, loc: node.loc, tags: node.tags}); - - graph = graph.replace(newNode); - graph = graph.replace(parent.updateNode(newNode.id, index)); - } - - // Only keep the first occurrence in first way - keep = false; - } - }); + replacements.forEach(function(replacement) { + var newNode = iD.Node({id: newNodeId, loc: node.loc, tags: node.tags}); + graph = graph.replace(newNode); + graph = graph.replace(replacement.way.updateNode(newNode.id, replacement.index)); }); return graph; }; + action.replacements = function(graph) { + var candidates = [], + keeping = false, + parents = graph.parentWays(graph.entity(nodeId)); + + parents.forEach(function(parent) { + if (wayIds && wayIds.length && wayIds.indexOf(parent.id) === -1) { + keeping = true; + return; + } + + parent.nodes.forEach(function(waynode, index) { + if (waynode === nodeId) { + candidates.push({way: parent, index: index}); + } + }); + }); + + return keeping ? candidates : candidates.slice(1); + }; + action.disabled = function(graph) { - var parentWays = graph.parentWays(graph.entity(nodeId)); - if (parentWays.length >= 2) - return; - if (parentWays.length === 0) - return 'not_connected'; - if (parentWays[0].nodes.filter(function(d) { return d === nodeId; }).length < 2) + var replacements = action.replacements(graph); + if (replacements.length === 0 || (wayIds && wayIds.length && wayIds.length !== replacements.length)) return 'not_connected'; }; diff --git a/js/id/operations/disconnect.js b/js/id/operations/disconnect.js index ecebac45c..9ec207561 100644 --- a/js/id/operations/disconnect.js +++ b/js/id/operations/disconnect.js @@ -1,14 +1,17 @@ iD.operations.Disconnect = function(selection, context) { - var entityId = selection[0], - action = iD.actions.Disconnect(entityId); + var vertices = _.filter(selection, function vertex(entityId) { + return context.geometry(entityId) === 'vertex' + }); + + var entityId = vertices[0], + action = iD.actions.Disconnect(entityId, _.without(selection, entityId)); var operation = function() { context.perform(action, t('operations.disconnect.annotation')); }; operation.available = function() { - return selection.length === 1 && - context.geometry(entityId) === 'vertex'; + return vertices.length === 1; }; operation.disabled = function() { diff --git a/test/spec/actions/disconnect.js b/test/spec/actions/disconnect.js index 7048983a4..925b969c2 100644 --- a/test/spec/actions/disconnect.js +++ b/test/spec/actions/disconnect.js @@ -35,6 +35,20 @@ describe("iD.actions.Disconnect", function () { expect(iD.actions.Disconnect('b').disabled(graph)).not.to.be.ok; }); + + it("returns falsy for an intersection of two ways with parent way specified", function () { + var graph = iD.Graph({ + 'a': iD.Node({id: 'a'}), + 'b': iD.Node({id: 'b'}), + 'c': iD.Node({id: 'c'}), + 'd': iD.Node({id: 'c'}), + '*': iD.Node({id: '*'}), + '-': iD.Way({id: '-', nodes: ['a', '*', 'b']}), + '|': iD.Way({id: '|', nodes: ['*', 'd']}) + }); + + expect(iD.actions.Disconnect('*', ['|']).disabled(graph)).not.to.be.ok; + }); }); it("replaces the node with a new node in all but the first way", function () { @@ -42,7 +56,7 @@ describe("iD.actions.Disconnect", function () { // a ---- b ---- c // | // d - // Split at b. + // Disconnect at b. // // Expected result: // a ---- b ---- c @@ -60,18 +74,47 @@ describe("iD.actions.Disconnect", function () { '|': iD.Way({id: '|', nodes: ['d', 'b']}) }); - graph = iD.actions.Disconnect('b', 'e')(graph); + graph = iD.actions.Disconnect('b', undefined, 'e')(graph); expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); expect(graph.entity('|').nodes).to.eql(['d', 'e']); }); + it("replaces the node with a new node in the specified ways", function () { + // Situation: + // a ---- b ==== c + // | + // d + // Disconnect - at b. + // + // Expected result: + // a ---- e 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']}), + '=': iD.Way({id: '=', nodes: ['b', 'c']}), + '|': iD.Way({id: '|', nodes: ['d', 'b']}) + }); + + graph = iD.actions.Disconnect('b', ['-'], 'e')(graph); + + expect(graph.entity('-').nodes).to.eql(['a', 'e']); + expect(graph.entity('=').nodes).to.eql(['b', 'c']); + expect(graph.entity('|').nodes).to.eql(['d', 'b']); + }); + it("replaces later occurrences in a self-intersecting way", function() { // Situtation: // a ---- b // \_ | // \__ c - // Split at a + // Disconnect at a // // Expected result: // a ---- b ---- c ---- d @@ -81,7 +124,7 @@ describe("iD.actions.Disconnect", function () { 'c': iD.Node({id: 'c'}), 'w': iD.Way({id: 'w', nodes: ['a', 'b', 'c', 'a']}) }); - graph = iD.actions.Disconnect('a', 'd')(graph); + graph = iD.actions.Disconnect('a', undefined, 'd')(graph); expect(graph.entity('w').nodes).to.eql(['a', 'b', 'c', 'd']); }); @@ -97,7 +140,7 @@ describe("iD.actions.Disconnect", function () { '|': iD.Way({id: '|', nodes: ['d', 'b']}) }); - graph = iD.actions.Disconnect('b', 'e')(graph); + graph = iD.actions.Disconnect('b', undefined, 'e')(graph); // Immutable loc => should be shared by identity. expect(graph.entity('b').loc).to.equal(loc);