Ticket #18670: 18670.2.patch

src/org/openstreetmap/josm/actions/UnGlueAction.java

@@ -108 +108 @@
                 }
                 // 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)) {
+                if (!selfCrossing) {
+                    if (selection.size() == 1 && selectedNode.isTagged()) {
                         unglueOneNodeAtMostOneWay(e);
                     } else {
                         errorTime = Notification.TIME_SHORT;
                         errMsg = tr("This node is not glued to anything else.");
                     }
+                }
             } else {
                 // and then do the work.
                 unglueWays();
@@ -245 +246 @@
     }
 
     /**
-     * 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<? extends OsmPrimitive> 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.
      *
@@ -291 +270 @@
         for (OsmPrimitive p : selection) {
             if (p instanceof Node) {
                 selectedNode = (Node) p;
-                if (size == 1 || selectedWay != null)
-                    return size == 1 || selectedWay.containsNode(selectedNode);
+                if (size == 1 || (selectedWay != null && selectedWay.containsNode(selectedNode)))
+                    return true;
             } else if (p instanceof Way) {
                 selectedWay = (Way) p;
                 if (size == 2 && selectedNode != null)
@@ -359 +338 @@
      * @param w parent way
      * @param cmds List of commands that will contain the new "add node" command
      * @param newNodes List of nodes that will contain the new node
-     * @return new way The modified way. Change command mus be handled by the caller
+     * @return new way The modified way. Change command must be handled by the caller
      */
     private static Way modifyWay(Node originalNode, Way w, List<Command> cmds, List<Node> newNodes) {
         // clone the node for the way
@@ -460 +439 @@
             if (wayWithSelectedNode == null) {
                 parentWays.removeFirst();
             }
+            Set<Way> 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(parentWays);
+            notifyWayPartOfRelation(warnParents);
         } else {
-            cmds.add(new ChangeCommand(selectedWay, modifyWay(selectedNode, selectedWay, cmds, newNodes)));
-            notifyWayPartOfRelation(Collections.singleton(selectedWay));
+            Way modWay = modifyWay(selectedNode, selectedWay, cmds, newNodes);
+            addCheckedChangeNodesCmd(cmds, selectedWay, modWay.getNodes());
         }
 
         if (dialog != null) {
@@ -527 +509 @@
             // selectedNode doesn't need unglue
             return false;
         }
-        cmds.add(new ChangeNodesCommand(way, newNodes));
-        notifyWayPartOfRelation(Collections.singleton(way));
+        addCheckedChangeNodesCmd(cmds, way, newNodes);
         try {
             final PropertiesMembershipChoiceDialog dialog = PropertiesMembershipChoiceDialog.showIfNecessary(
                     Collections.singleton(selectedNode), false);
@@ -568 +549 @@
             }
             allNewNodes.addAll(newNodes);
         }
-        cmds.add(new ChangeCommand(selectedWay, tmpWay)); // only one changeCommand for a way, else garbage will happen
-        notifyWayPartOfRelation(Collections.singleton(selectedWay));
+        // 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",
@@ -577 +558 @@
         getLayerManager().getEditDataSet().setSelected(allNewNodes);
     }
 
+    private void addCheckedChangeNodesCmd(List<Command> cmds, Way w, List<Node> nodes) {
+        boolean relationCheck = w.firstNode() != nodes.get(0) || w.lastNode() != nodes.get(nodes.size() - 1);
+        cmds.add(new ChangeNodesCommand(w, nodes));
+        if (relationCheck) {
+            notifyWayPartOfRelation(Collections.singleton(w));
+        }
+    }
+
     @Override
     protected void updateEnabledState() {
         updateEnabledStateOnCurrentSelection();

test/unit/org/openstreetmap/josm/actions/UnGlueActionTest.java

@@ -30 +30 @@
      */
     @Rule
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().main();
+    public JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
 
     /**
      * Setup test.