Index: src/org/openstreetmap/josm/actions/UnGlueAction.java =================================================================== --- src/org/openstreetmap/josm/actions/UnGlueAction.java (revision 16352) +++ src/org/openstreetmap/josm/actions/UnGlueAction.java (working copy) @@ -5,19 +5,20 @@ import static org.openstreetmap.josm.tools.I18n.tr; import static org.openstreetmap.josm.tools.I18n.trn; +import java.awt.Point; import java.awt.event.ActionEvent; import java.awt.event.KeyEvent; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.stream.Collectors; import javax.swing.JOptionPane; -import javax.swing.JPanel; import org.openstreetmap.josm.command.AddCommand; import org.openstreetmap.josm.command.ChangeCommand; @@ -41,16 +42,13 @@ import org.openstreetmap.josm.tools.Logging; import org.openstreetmap.josm.tools.Shortcut; import org.openstreetmap.josm.tools.UserCancelException; -import org.openstreetmap.josm.tools.Utils; /** - * Duplicate nodes that are used by multiple ways. + * Duplicate nodes that are used by multiple ways or tagged nodes used by a single way + * or nodes which referenced more than once by a single way. * - * Resulting nodes are identical, up to their position. - * * This is the opposite of the MergeNodesAction. * - * If a single node is selected, it will copy that node and remove all tags from the old one */ public class UnGlueAction extends JosmAction { @@ -75,7 +73,7 @@ @Override public void actionPerformed(ActionEvent e) { try { - unglue(e); + unglue(); } catch (UserCancelException ignore) { Logging.trace(ignore); } finally { @@ -83,36 +81,35 @@ } } - protected void unglue(ActionEvent e) throws UserCancelException { + protected void unglue() throws UserCancelException { Collection selection = getLayerManager().getEditDataSet().getSelected(); String errMsg = null; int errorTime = Notification.TIME_DEFAULT; + if (checkSelectionOneNodeAtMostOneWay(selection)) { checkAndConfirmOutlyingUnglue(); - int count = 0; - for (Way w : selectedNode.getParentWays()) { - if (!w.isUsable() || w.getNodesCount() < 1) { - continue; + List parentWays = selectedNode.getParentWays().stream().filter(Way::isUsable).collect(Collectors.toList()); + + if (parentWays.size() < 2) { + if (!parentWays.isEmpty()) { + // single way + Way way = selectedWay == null ? parentWays.get(0) : selectedWay; + boolean closedOrSelfCrossing = way.getNodes().stream().filter(n -> n == selectedNode).count() > 1; + + final PropertiesMembershipChoiceDialog dialog = PropertiesMembershipChoiceDialog.showIfNecessary( + Collections.singleton(selectedNode), !selectedNode.isTagged()); + if (dialog != null) { + unglueOneNodeAtMostOneWay(way, dialog); + return; + } else if (closedOrSelfCrossing) { + unglueClosedOrSelfCrossingWay(way, dialog); + return; + } } - count++; - } - if (count < 2) { - boolean selfCrossing = false; - if (count == 1) { - // First try unglue self-crossing way - selfCrossing = unglueSelfCrossingWay(); - } - // If there aren't enough ways, maybe the user wanted to unglue the nodes - // (= copy tags to a new node) - if (!selfCrossing) - if (checkForUnglueNode(selection)) { - unglueOneNodeAtMostOneWay(e); - } else { - errorTime = Notification.TIME_SHORT; - errMsg = tr("This node is not glued to anything else."); - } + errorTime = Notification.TIME_SHORT; + errMsg = tr("This node is not glued to anything else."); } else { // and then do the work. unglueWays(); @@ -119,28 +116,18 @@ } } else if (checkSelectionOneWayAnyNodes(selection)) { checkAndConfirmOutlyingUnglue(); - Set tmpNodes = new HashSet<>(); - for (Node n : selectedNodes) { - int count = 0; - for (Way w : n.getParentWays()) { - if (!w.isUsable()) { - continue; - } - count++; - } - if (count >= 2) { - tmpNodes.add(n); - } - } - if (tmpNodes.isEmpty()) { + selectedNodes.removeIf(n -> n.getParentWays().stream().filter(Way::isUsable).count() < 2); + if (selectedNodes.isEmpty()) { if (selection.size() > 1) { errMsg = tr("None of these nodes are glued to anything else."); } else { errMsg = tr("None of this way''s nodes are glued to anything else."); } + } else if (selectedNodes.size() == 1) { + selectedNode = selectedNodes.iterator().next(); + unglueWays(); } else { // and then do the work. - selectedNodes = tmpNodes; unglueOneWayAnyNodes(); } } else { @@ -175,12 +162,12 @@ selectedNodes = null; } - static void update(PropertiesMembershipChoiceDialog dialog, Node existingNode, List newNodes, Collection cmds) { + static void update(PropertiesMembershipChoiceDialog dialog, Node existingNode, List newNodes, List cmds) { updateMemberships(dialog.getMemberships().orElse(null), existingNode, newNodes, cmds); updateProperties(dialog.getTags().orElse(null), existingNode, newNodes, cmds); } - private static void updateProperties(ExistingBothNew tags, Node existingNode, Iterable newNodes, Collection cmds) { + private static void updateProperties(ExistingBothNew tags, Node existingNode, Iterable newNodes, List cmds) { if (ExistingBothNew.NEW == tags) { final Node newSelectedNode = new Node(existingNode); newSelectedNode.removeAll(); @@ -194,81 +181,37 @@ /** * Assumes there is one tagged Node stored in selectedNode that it will try to unglue. - * (i.e. copy node and remove all tags from the old one. Relations will not be removed) - * @param e event that triggered the action + * (i.e. copy node and remove all tags from the old one.) + * @param way way to modify + * @param dialog */ - private void unglueOneNodeAtMostOneWay(ActionEvent e) { - final PropertiesMembershipChoiceDialog dialog; - try { - dialog = PropertiesMembershipChoiceDialog.showIfNecessary(Collections.singleton(selectedNode), true); - } catch (UserCancelException ex) { - Logging.trace(ex); - return; + private void unglueOneNodeAtMostOneWay(Way way, PropertiesMembershipChoiceDialog dialog) { + List cmds = new ArrayList<>(); + List newNodes = new ArrayList<>(); + Way modWay = modifyWay(selectedNode, way, cmds, newNodes); + cmds.add(new ChangeNodesCommand(way, modWay.getNodes())); + if (dialog != null) { + update(dialog, selectedNode, newNodes, cmds); } - final Node unglued = new Node(selectedNode, true); - boolean moveSelectedNode = false; - - List cmds = new LinkedList<>(); - cmds.add(new AddCommand(selectedNode.getDataSet(), unglued)); - if (dialog != null && ExistingBothNew.NEW == dialog.getTags().orElse(null)) { - // unglued node gets the ID and history, thus replace way node with a fresh one - final Way way = selectedNode.getParentWays().get(0); - final List newWayNodes = way.getNodes(); - newWayNodes.replaceAll(n -> selectedNode.equals(n) ? unglued : n); - cmds.add(new ChangeNodesCommand(way, newWayNodes)); - updateMemberships(dialog.getMemberships().map(ExistingBothNew::opposite).orElse(null), - selectedNode, Collections.singletonList(unglued), cmds); - updateProperties(dialog.getTags().map(ExistingBothNew::opposite).orElse(null), - selectedNode, Collections.singletonList(unglued), cmds); - moveSelectedNode = true; - } else if (dialog != null) { - update(dialog, selectedNode, Collections.singletonList(unglued), cmds); - } - - // If this wasn't called from menu, place it where the cursor is/was + // Place the selected node where the cursor is/was MapView mv = MainApplication.getMap().mapView; - if (e.getSource() instanceof JPanel) { - final LatLon latLon = mv.getLatLon(mv.lastMEvent.getX(), mv.lastMEvent.getY()); - if (moveSelectedNode) { - cmds.add(new MoveCommand(selectedNode, latLon)); - } else { - unglued.setCoor(latLon); - } + Point currMousePos = mv.getMousePosition(); + if (currMousePos != null) { + final LatLon latLon = mv.getLatLon(currMousePos.getX(), currMousePos.getY()); + cmds.add(new MoveCommand(selectedNode, latLon)); + } else { + cmds.add(new MoveCommand(selectedNode, 0, 5)); } - UndoRedoHandler.getInstance().add(new SequenceCommand(tr("Unglued Node"), cmds)); - getLayerManager().getEditDataSet().setSelected(moveSelectedNode ? selectedNode : unglued); - mv.repaint(); + getLayerManager().getEditDataSet().setSelected(selectedNode); } /** - * Checks if selection is suitable for ungluing. This is the case when there's a single, - * tagged node selected that's part of at least one way (ungluing an unconnected node does - * not make sense. Due to the call order in actionPerformed, this is only called when the - * node is only part of one or less ways. - * - * @param selection The selection to check against - * @return {@code true} if selection is suitable - */ - private boolean checkForUnglueNode(Collection selection) { - if (selection.size() != 1) - return false; - OsmPrimitive n = (OsmPrimitive) selection.toArray()[0]; - if (!(n instanceof Node)) - return false; - if (((Node) n).getParentWays().isEmpty()) - return false; - - selectedNode = (Node) n; - return selectedNode.isTagged(); - } - - /** * Checks if the selection consists of something we can work with. * Checks only if the number and type of items selected looks good. * - * If this method returns "true", selectedNode and selectedWay will be set. + * If this method returns "true", selectedNode will be set, selectedWay might be set * * Returns true if either one node is selected or one node and one * way are selected and the node is part of the way. @@ -361,17 +304,11 @@ */ private static Way modifyWay(Node originalNode, Way w, List cmds, List newNodes) { // clone the node for the way - Node newNode = new Node(originalNode, true /* clear OSM ID */); + Node newNode = cloneNode(originalNode, cmds); newNodes.add(newNode); - cmds.add(new AddCommand(originalNode.getDataSet(), newNode)); - List nn = new ArrayList<>(); - for (Node pushNode : w.getNodes()) { - if (originalNode == pushNode) { - pushNode = newNode; - } - nn.add(pushNode); - } + List nn = new ArrayList<>(w.getNodes()); + nn.replaceAll(n -> n == originalNode ? newNode : n); Way newWay = new Way(w); newWay.setNodes(nn); @@ -378,6 +315,12 @@ return newWay; } + private static Node cloneNode(Node originalNode, List cmds) { + Node newNode = new Node(originalNode, true /* clear OSM ID */); + cmds.add(new AddCommand(originalNode.getDataSet(), newNode)); + return newNode; + } + /** * put all newNodes into the same relation(s) that originalNode is in * @param memberships where the memberships should be places @@ -385,7 +328,7 @@ * @param cmds List of commands that will contain the new "change relation" commands * @param newNodes List of nodes that contain the new node */ - private static void updateMemberships(ExistingBothNew memberships, Node originalNode, List newNodes, Collection cmds) { + private static void updateMemberships(ExistingBothNew memberships, Node originalNode, List newNodes, List cmds) { if (memberships == null || ExistingBothNew.OLD == memberships) { return; } @@ -421,21 +364,16 @@ * dupe a single node into as many nodes as there are ways using it, OR * * dupe a single node once, and put the copy on the selected way + * @throws UserCancelException if user cancels choice */ - private void unglueWays() { - final PropertiesMembershipChoiceDialog dialog; - try { - dialog = PropertiesMembershipChoiceDialog.showIfNecessary(Collections.singleton(selectedNode), false); - } catch (UserCancelException e) { - Logging.trace(e); - return; - } - - List cmds = new LinkedList<>(); - List newNodes = new LinkedList<>(); + private void unglueWays() throws UserCancelException { + final PropertiesMembershipChoiceDialog dialog = PropertiesMembershipChoiceDialog + .showIfNecessary(Collections.singleton(selectedNode), false); + List cmds = new ArrayList<>(); + List newNodes = new ArrayList<>(); + List parentWays; if (selectedWay == null) { - LinkedList parentWays = selectedNode.referrers(Way.class).filter(Way::isUsable) - .collect(Collectors.toCollection(LinkedList::new)); + parentWays = selectedNode.referrers(Way.class).filter(Way::isUsable).collect(Collectors.toList()); // see #5452 and #18670 parentWays.sort((o1, o2) -> { int d = Boolean.compare(!o1.isNew() && !o1.isModified(), !o2.isNew() && !o2.isModified()); @@ -448,23 +386,21 @@ return d; }); // first way should not be changed, preferring older ways and those with fewer parents - parentWays.removeFirst(); - - Set warnParents = new HashSet<>(); - for (Way w : parentWays) { - if (w.isFirstLastNode(selectedNode)) - warnParents.add(w); - cmds.add(new ChangeCommand(w, modifyWay(selectedNode, w, cmds, newNodes))); - } - notifyWayPartOfRelation(warnParents); + parentWays.remove(0); } else { - Way modWay = modifyWay(selectedNode, selectedWay, cmds, newNodes); - addCheckedChangeNodesCmd(cmds, selectedWay, modWay.getNodes()); + parentWays = Collections.singletonList(selectedWay); } + Set warnParents = new HashSet<>(); + for (Way w : parentWays) { + if (w.isFirstLastNode(selectedNode)) + warnParents.add(w); + cmds.add(new ChangeCommand(w, modifyWay(selectedNode, w, cmds, newNodes))); + } if (dialog != null) { update(dialog, selectedNode, newNodes, cmds); } + notifyWayPartOfRelation(warnParents); execCommands(cmds, newNodes); } @@ -483,98 +419,72 @@ /** * Duplicates a node used several times by the same way. See #9896. + * A closed way will be "opened" when the closing node is unglued. + * @param way way to modify + * @param dialog user dialog, might be null * @return true if action is OK false if there is nothing to do */ - private boolean unglueSelfCrossingWay() { + private boolean unglueClosedOrSelfCrossingWay(Way way, PropertiesMembershipChoiceDialog dialog) { // According to previous check, only one valid way through that node - Way way = null; - for (Way w: selectedNode.getParentWays()) { - if (w.isUsable() && w.getNodesCount() >= 1) { - way = w; - } - } - if (way == null) { - return false; - } - List cmds = new LinkedList<>(); + List cmds = new ArrayList<>(); List oldNodes = way.getNodes(); List newNodes = new ArrayList<>(oldNodes.size()); List addNodes = new ArrayList<>(); - boolean seen = false; + int count = 0; for (Node n: oldNodes) { - if (n == selectedNode) { - if (seen) { - Node newNode = new Node(n, true /* clear OSM ID */); - cmds.add(new AddCommand(selectedNode.getDataSet(), newNode)); - newNodes.add(newNode); - addNodes.add(newNode); - } else { - newNodes.add(n); - seen = true; - } - } else { - newNodes.add(n); + if (n == selectedNode && count++ > 0) { + n = cloneNode(selectedNode, cmds); + addNodes.add(n); } + newNodes.add(n); } if (addNodes.isEmpty()) { // selectedNode doesn't need unglue return false; } + if (dialog != null) { + update(dialog, selectedNode, addNodes, cmds); + } addCheckedChangeNodesCmd(cmds, way, newNodes); - try { - final PropertiesMembershipChoiceDialog dialog = PropertiesMembershipChoiceDialog.showIfNecessary( - Collections.singleton(selectedNode), false); - if (dialog != null) { - update(dialog, selectedNode, addNodes, cmds); - } - execCommands(cmds, addNodes); - return true; - } catch (UserCancelException ignore) { - Logging.trace(ignore); - } - return false; + execCommands(cmds, addNodes); + return true; } /** * dupe all nodes that are selected, and put the copies on the selected way + * @throws UserCancelException * */ - private void unglueOneWayAnyNodes() { - Way tmpWay = selectedWay; + private void unglueOneWayAnyNodes() throws UserCancelException { + final PropertiesMembershipChoiceDialog dialog = + PropertiesMembershipChoiceDialog.showIfNecessary(selectedNodes, false); - final PropertiesMembershipChoiceDialog dialog; - try { - dialog = PropertiesMembershipChoiceDialog.showIfNecessary(selectedNodes, false); - } catch (UserCancelException e) { - Logging.trace(e); - return; - } + Map replaced = new HashMap<>(); + List cmds = new ArrayList<>(); - List cmds = new LinkedList<>(); - List allNewNodes = new LinkedList<>(); - for (Node n : selectedNodes) { - List newNodes = new LinkedList<>(); - tmpWay = modifyWay(n, tmpWay, cmds, newNodes); - if (dialog != null) { - update(dialog, n, newNodes, cmds); - } - allNewNodes.addAll(newNodes); + selectedNodes.forEach(n -> replaced.put(n, cloneNode(n, cmds))); + List modNodes = selectedWay.getNodes().stream().map(n -> replaced.getOrDefault(n, n)) + .collect(Collectors.toList()); + + // only one changeCommand for a way, else garbage will happen + addCheckedChangeNodesCmd(cmds, selectedWay, modNodes); + if (dialog != null) { + replaced.forEach((k,v) -> update(dialog, k, Collections.singletonList(v), cmds)); } - // only one changeCommand for a way, else garbage will happen - addCheckedChangeNodesCmd(cmds, selectedWay, tmpWay.getNodes()); UndoRedoHandler.getInstance().add(new SequenceCommand( trn("Dupe {0} node into {1} nodes", "Dupe {0} nodes into {1} nodes", - selectedNodes.size(), selectedNodes.size(), selectedNodes.size()+allNewNodes.size()), cmds)); - getLayerManager().getEditDataSet().setSelected(allNewNodes); + selectedNodes.size(), selectedNodes.size(), 2 * selectedNodes.size()), cmds)); + getLayerManager().getEditDataSet().setSelected(replaced.values()); } - private void addCheckedChangeNodesCmd(List cmds, Way w, List nodes) { - boolean relationCheck = w.firstNode() != nodes.get(0) || w.lastNode() != nodes.get(nodes.size() - 1); + private boolean addCheckedChangeNodesCmd(List cmds, Way w, List nodes) { + boolean relationCheck = !calcAffectedRelations(Collections.singleton(w)).isEmpty(); cmds.add(new ChangeNodesCommand(w, nodes)); if (relationCheck) { notifyWayPartOfRelation(Collections.singleton(w)); } + return relationCheck; } @Override @@ -611,38 +521,42 @@ } protected void notifyWayPartOfRelation(final Collection ways) { - final Set affectedNodes = (selectedNodes != null) ? selectedNodes : Collections.singleton(selectedNode); - final Set affectedRelations = new HashSet<>(); - for (Relation r: OsmPrimitive.getParentRelations(ways)) { - if (!r.isUsable()) - continue; - // see #18670: suppress notification when well known restriction types are not affected - if (r.hasTag("type", "restriction", "connectivity", "destination_sign") && !r.hasIncompleteMembers()) { - int count = 0; - for (RelationMember rm : r.getMembers()) { - if (rm.isNode() && affectedNodes.contains(rm.getNode())) - count++; - if (rm.isWay() && ways.contains(rm.getWay())) { - count++; - if ("via".equals(rm.getRole())) { - count++; - } - } - } - if (count < 2) - continue; - } - affectedRelations.add(r.getDisplayName(DefaultNameFormatter.getInstance())); - } + Set affectedRelations = calcAffectedRelations(ways); if (affectedRelations.isEmpty()) { return; } - final int size = affectedRelations.size(); final String msg1 = trn("Unglueing possibly affected {0} relation: {1}", "Unglueing possibly affected {0} relations: {1}", - size, size, Utils.joinAsHtmlUnorderedList(affectedRelations)); + size, size, DefaultNameFormatter.getInstance().formatAsHtmlUnorderedList(affectedRelations, 20)); final String msg2 = trn("Ensure that the relation has not been broken!", "Ensure that the relations have not been broken!", size); - new Notification("" + msg1 + msg2).setIcon(JOptionPane.WARNING_MESSAGE).show(); + new Notification(msg1 + msg2).setIcon(JOptionPane.WARNING_MESSAGE).show(); } + + protected Set calcAffectedRelations(final Collection ways) { + final Set affectedNodes = (selectedNodes != null) ? selectedNodes : Collections.singleton(selectedNode); + return OsmPrimitive.getParentRelations(ways) + .stream().filter(r -> isRelationAffected(r, affectedNodes, ways)) + .collect(Collectors.toSet()); + } + + private static boolean isRelationAffected(Relation r, Set affectedNodes, Collection ways) { + if (!r.isUsable()) + return false; + // see #18670: suppress notification when well known restriction types are not affected + if (!r.hasTag("type", "restriction", "connectivity", "destination_sign") || r.hasIncompleteMembers()) + return true; + int count = 0; + for (RelationMember rm : r.getMembers()) { + if (rm.isNode() && affectedNodes.contains(rm.getNode())) + count++; + if (rm.isWay() && ways.contains(rm.getWay())) { + count++; + if ("via".equals(rm.getRole())) { + count++; + } + } + } + return count >= 2; + } }