Changeset 16300 in josm for trunk

Timestamp:

2020-04-14T10:27:34+02:00 (4 years ago)

Author:

GerdP

Message:

fix #18670, #19042: Unglue ways: False positives about affected relations

• avoid to warn about possibly broken turn restrictions when we can make sure they are not broken
• when no way is selected, try to find one that produces fewer updates

TODO: handle dataset without download areas

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/UnGlueAction.java (modified) (11 diffs)
• test/unit/org/openstreetmap/josm/actions/UnGlueActionTest.java (modified) (1 diff)

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

@@ -109 +109 @@
                 // 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 {
@@ -116 +116 @@
                         errMsg = tr("This node is not glued to anything else.");
                     }
+                }
             } else {
                 // and then do the work.
@@ -246 +247 @@
 
     /**
-     * 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.
@@ -292 +271 @@
             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;
@@ -360 +339 @@
      * @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) {
@@ -446 +425 @@
         List<Node> newNodes = new LinkedList<>();
         if (selectedWay == null) {
-            Way wayWithSelectedNode = null;
-            LinkedList<Way> parentWays = new LinkedList<>();
-            for (OsmPrimitive osm : selectedNode.getReferrers()) {
-                if (osm.isUsable() && osm instanceof Way) {
-                    Way w = (Way) osm;
-                    if (wayWithSelectedNode == null && !w.isFirstLastNode(selectedNode)) {
-                        wayWithSelectedNode = w;
-                    } else {
-                        parentWays.add(w);
-                    }
-                }
-            }
-            if (wayWithSelectedNode == null) {
-                parentWays.removeFirst();
-            }
+            LinkedList<Way> parentWays = selectedNode.referrers(Way.class).filter(Way::isUsable)
+                    .collect(Collectors.toCollection(LinkedList::new));
+            // see #5452 and #18670
+            parentWays.sort((o1, o2) -> {
+                int d = Boolean.compare(!o1.isNew() && !o1.isModified(), !o2.isNew() && !o2.isModified());
+                if (d == 0) {
+                    d = Integer.compare(o2.getReferrers().size(), o1.getReferrers().size()); // reversed
+                }
+                if (d == 0) {
+                    d = Boolean.compare(o1.isFirstLastNode(selectedNode), o2.isFirstLastNode(selectedNode));
+                }
+                return d;
+            });
+            // first way should not be changed, preferring older ways and those with fewer parents
+            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());
         }
 
@@ -528 +511 @@
             return false;
         }
-        cmds.add(new ChangeNodesCommand(way, newNodes));
-        notifyWayPartOfRelation(Collections.singleton(way));
+        addCheckedChangeNodesCmd(cmds, way, newNodes);
         try {
             final PropertiesMembershipChoiceDialog dialog = PropertiesMembershipChoiceDialog.showIfNecessary(
@@ -569 +551 @@
             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(
@@ -576 +558 @@
                         selectedNodes.size(), selectedNodes.size(), selectedNodes.size()+allNewNodes.size()), cmds));
         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));
+        }
     }
 
@@ -612 +602 @@
 
     protected void notifyWayPartOfRelation(final Collection<Way> ways) {
-        final Set<String> affectedRelations = ways.stream()
-                .flatMap(w -> w.getReferrers().stream())
-                .filter(ref -> ref instanceof Relation && ref.isUsable())
-                .map(ref -> ref.getDisplayName(DefaultNameFormatter.getInstance()))
-                .collect(Collectors.toSet());
+        final Set<Node> affectedNodes = (selectedNodes != null) ? selectedNodes : Collections.singleton(selectedNode);
+        final Set<String> 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()));
+        }
         if (affectedRelations.isEmpty()) {
             return;
@@ -622 +630 @@
 
         final int size = affectedRelations.size();
-        final String msg1 = trn("Unglueing affected {0} relation: {1}", "Unglueing affected {0} relations: {1}",
+        final String msg1 = trn("Unglueing possibly affected {0} relation: {1}", "Unglueing possibly affected {0} relations: {1}",
                 size, size, Utils.joinAsHtmlUnorderedList(affectedRelations));
         final String msg2 = trn("Ensure that the relation has not been broken!", "Ensure that the relations have not been broken!",

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

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