Changeset 17386 in josm

Timestamp:

2020-12-02T16:43:36+01:00 (4 years ago)

Author:

GerdP

Message:

fix #20041: Align nodes in Circle creates Command which changes nothing

• only create move command if node is visibly moved
• reject a self-intersecting way
• reject old nodes outside of download area (the original code only shows an info and continues)
• add some unit tests to improve coverage

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/AlignInCircleAction.java (modified) (15 diffs)
• src/org/openstreetmap/josm/data/validation/tests/SelfIntersectingWay.java (modified) (2 diffs)
• test/data/alignCircleAfter1.osm (added)
• test/data/alignCircleAfter2.osm (added)
• test/data/alignCircleBefore.osm (added)
• test/data/alignCircleTwoWaysAfter.osm (added)
• test/data/alignCircleTwoWaysBefore.osm (added)
• test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java (added)

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

@@ -28 +28 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon;
+import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon.JoinedWay;
+import org.openstreetmap.josm.data.validation.tests.SelfIntersectingWay;
 import org.openstreetmap.josm.gui.Notification;
 import org.openstreetmap.josm.tools.Geometry;
+import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Shortcut;
 
@@ -55 +59 @@
 
     /**
-     * Create a {@link MoveCommand} to move a node to a PolarCoor.
+     * InvalidSelection exception has to be raised when action can't be performed
+     */
+    public static class InvalidSelection extends Exception {
+
+        /**
+         * Create an InvalidSelection exception with default message
+         */
+        InvalidSelection() {
+            super(tr("Selection could not be used to align in circle."));
+        }
+
+        /**
+         * Create an InvalidSelection exception with specific message
+         * @param msg Message that will be displayed to the user
+         */
+        InvalidSelection(String msg) {
+            super(msg);
+        }
+    }
+
+    /**
+     * Add a {@link MoveCommand} to move a node to a PolarCoor if there is a significant move.
      * @param n Node to move
      * @param coor polar coordinate where to move the node
-     * @return new MoveCommand
-     * @since 13107
-     */
-    public static MoveCommand createMoveCommand(Node n, PolarCoor coor) {
+     * @param cmds list of commands
+     * @since 17386
+     */
+    public static void addMoveCommandIfNeeded(Node n, PolarCoor coor, List<Command> cmds) {
         EastNorth en = coor.toEastNorth();
-        return new MoveCommand(n, en.east() - n.getEastNorth().east(), en.north() - n.getEastNorth().north());
+        double deltaEast = en.east() - n.getEastNorth().east();
+        double deltaNorth = en.north() - n.getEastNorth().north();
+
+        if (Math.abs(deltaEast) > 5e-6 || Math.abs(deltaNorth) > 5e-6) {
+            cmds.add(new MoveCommand(n, deltaEast, deltaNorth));
+        }
     }
 
@@ -69 +99 @@
      * Perform AlignInCircle action.
      *
+     */
+    @Override
+    public void actionPerformed(ActionEvent e) {
+        if (!isEnabled())
+            return;
+
+        try {
+            Command cmd = buildCommand(getLayerManager().getEditDataSet());
+            UndoRedoHandler.getInstance().add(cmd);
+        } catch (InvalidSelection except) {
+            Logging.debug(except);
+            new Notification(except.getMessage())
+                .setIcon(JOptionPane.INFORMATION_MESSAGE)
+                .setDuration(Notification.TIME_SHORT)
+                .show();
+        }
+
+    }
+
+    /**
+     * Builds "align in circle" command depending on the selected objects.
      * A fixed node is a node for which it is forbidden to change the angle relative to center of the circle.
      * All other nodes are uniformly distributed.
-     *
+     * <p>
      * Case 1: One unclosed way.
      * --&gt; allow action, and align selected way nodes
      * If nodes contained by this way are selected, there are fix.
      * If nodes outside from the way are selected there are ignored.
-     *
+     * <p>
      * Case 2: One or more ways are selected and can be joined into a polygon
      * --&gt; allow action, and align selected ways nodes
@@ -85 +136 @@
      * In all cases, selected nodes are fix, nodes with more than one referrers are fix
      * (first referrer is the selected way)
-     *
+     * <p>
      * Case 3: Only nodes are selected
      * --&gt; Align these nodes, all are fix
-     */
-    @Override
-    public void actionPerformed(ActionEvent e) {
-        if (!isEnabled())
-            return;
-
-        Collection<OsmPrimitive> sel = getLayerManager().getEditDataSet().getSelected();
+     * @param ds data set in which the command operates
+     * @return the resulting command to execute to perform action
+     * @throws InvalidSelection if selection cannot be used
+     * @since 17386
+     *
+     */
+    public static Command buildCommand(DataSet ds) throws InvalidSelection {
+        Collection<OsmPrimitive> sel = ds.getSelected();
         List<Node> nodes = new LinkedList<>();
         // fixNodes: All nodes for which the angle relative to center should not be modified
@@ -113 +165 @@
             // Case 1
             Way w = ways.get(0);
+            if (SelfIntersectingWay.isSelfIntersecting(w)) {
+                throw new InvalidSelection(tr("Self-intersecting way"));
+            }
+
             fixNodes.add(w.firstNode());
             fixNodes.add(w.lastNode());
@@ -121 +177 @@
             closedWay.addNode(w.firstNode());
             nodes = collectNodesAnticlockwise(Collections.singletonList(closedWay));
-            closedWay.setNodes(null);
+            closedWay.setNodes(null); // see #19885
         } else if (!ways.isEmpty() && checkWaysArePolygon(ways)) {
             // Case 2
@@ -152 +208 @@
             nodes = collectNodesAnticlockwise(ways);
             if (nodes.size() < 4) {
-                new Notification(
-                        tr("Not enough nodes in selected ways."))
-                .setIcon(JOptionPane.INFORMATION_MESSAGE)
-                .setDuration(Notification.TIME_SHORT)
-                .show();
-                return;
+                throw new InvalidSelection(tr("Not enough nodes in selected ways."));
             }
         } else if (ways.isEmpty() && nodes.size() > 3) {
@@ -164 +215 @@
             // No need to reorder nodes since all are fix
         } else {
-            // Invalid action
-            new Notification(
-                    tr("Please select at least four nodes."))
-                    .setIcon(JOptionPane.INFORMATION_MESSAGE)
-                    .setDuration(Notification.TIME_SHORT)
-                    .show();
-            return;
-        }
+            if (ways.isEmpty() && nodes.size() <= 3)
+                throw new InvalidSelection(tr("Please select at least four nodes."));
+            throw new InvalidSelection();
+        }
+
+        // Check if one or more nodes are outside of download area
+        if (nodes.stream().anyMatch(Node::isOutsideDownloadArea))
+            throw new InvalidSelection(tr("One or more nodes involved in this action is outside of the downloaded area."));
 
         if (center == null) {
@@ -177 +228 @@
             center = Geometry.getCenter(nodes);
             if (center == null) {
-                new Notification(tr("Cannot determine center of selected nodes."))
-                    .setIcon(JOptionPane.INFORMATION_MESSAGE)
-                    .setDuration(Notification.TIME_SHORT)
-                    .show();
-                return;
+                throw new InvalidSelection(tr("Cannot determine center of selected nodes."));
             }
         }
@@ -195 +242 @@
         }
 
-        if (!actionAllowed(nodes)) return;
-
-        Collection<Command> cmds = new LinkedList<>();
+        List<Command> cmds = new LinkedList<>();
 
         // Move each node to that distance from the center.
@@ -217 +262 @@
             Node first = nodes.get(i % nodeCount);
             PolarCoor pcFirst = new PolarCoor(radius, PolarCoor.computeAngle(first.getEastNorth(), center), center);
-            cmds.add(createMoveCommand(first, pcFirst));
+            addMoveCommandIfNeeded(first, pcFirst, cmds);
             if (j > i + 1) {
                 double delta;
@@ -231 +276 @@
                 for (int k = i+1; k < j; k++) {
                     PolarCoor p = new PolarCoor(radius, pcFirst.angle + (k-i)*delta, center);
-                    cmds.add(createMoveCommand(nodes.get(k % nodeCount), p));
+                    addMoveCommandIfNeeded(nodes.get(k % nodeCount), p, cmds);
                 }
             }
             i = j; // Update start point for next iteration
         }
-
-        UndoRedoHandler.getInstance().add(new SequenceCommand(tr("Align Nodes in Circle"), cmds));
+        if (cmds.isEmpty())
+            throw new InvalidSelection(tr("nothing changed"));
+        return new SequenceCommand(tr("Align Nodes in Circle"), cmds);
     }
 
@@ -253 +299 @@
      * @param ways List of ways to be joined
      * @return Nodes anticlockwise ordered
-     */
-    private static List<Node> collectNodesAnticlockwise(List<Way> ways) {
-        List<Node> nodes = new ArrayList<>();
-        Node firstNode = ways.get(0).firstNode();
-        Node lastNode = null;
-        Way lastWay = null;
-        while (firstNode != lastNode) {
-            if (lastNode == null) lastNode = firstNode;
-            for (Way way: ways) {
-                if (way == lastWay) continue;
-                if (way.firstNode() == lastNode) {
-                    List<Node> wayNodes = way.getNodes();
-                    for (int i = 0; i < wayNodes.size() - 1; i++) {
-                        nodes.add(wayNodes.get(i));
-                    }
-                    lastNode = way.lastNode();
-                    lastWay = way;
-                    break;
-                }
-                if (way.lastNode() == lastNode) {
-                    List<Node> wayNodes = way.getNodes();
-                    for (int i = wayNodes.size() - 1; i > 0; i--) {
-                        nodes.add(wayNodes.get(i));
-                    }
-                    lastNode = way.firstNode();
-                    lastWay = way;
-                    break;
-                }
-            }
-        }
-        // Check if nodes are in anticlockwise order
-        int nc = nodes.size();
-        double area = 0;
-        for (int i = 0; i < nc; i++) {
-            EastNorth p1 = nodes.get(i).getEastNorth();
-            EastNorth p2 = nodes.get((i+1) % nc).getEastNorth();
-            area += p1.east()*p2.north() - p2.east()*p1.north();
-        }
-        if (area < 0)
+     * @throws InvalidSelection if selection cannot be used
+     */
+    private static List<Node> collectNodesAnticlockwise(List<Way> ways) throws InvalidSelection {
+        Collection<JoinedWay> rings = Multipolygon.joinWays(ways);
+        if (rings.size() != 1)
+            throw new InvalidSelection();
+        List<Node> nodes = new ArrayList<>(rings.iterator().next().getNodes());
+        if (nodes.get(0) != nodes.get(nodes.size()-1))
+            throw new InvalidSelection();
+        if (Geometry.isClockwise(nodes))
             Collections.reverse(nodes);
+        nodes.remove(nodes.size() - 1);
         return nodes;
-    }
-
-    /**
-     * Check if one or more nodes are outside of download area
-     * @param nodes Nodes to check
-     * @return true if action can be done
-     */
-    private static boolean actionAllowed(Collection<Node> nodes) {
-        boolean outside = nodes.stream().anyMatch(Node::isOutsideDownloadArea);
-        if (outside)
-            new Notification(
-                    tr("One or more nodes involved in this action is outside of the downloaded area."))
-                    .setIcon(JOptionPane.WARNING_MESSAGE)
-                    .setDuration(Notification.TIME_SHORT)
-                    .show();
-        return true;
     }
 
@@ -324 +326 @@
 
     /**
-     * Determines if ways can be joined into a polygon.
+     * Determines if ways can be joined into a single polygon.
      * @param ways The ways collection to check
-     * @return true if all ways can be joined into a polygon
+     * @return true if all ways can be joined into a single polygon
      */
     private static boolean checkWaysArePolygon(Collection<Way> ways) {
+        if (Multipolygon.joinWays(ways).size() != 1)
+            return false;
         // For each way, nodes strictly between first and last should't be reference by an other way
         for (Way way: ways) {
@@ -338 +342 @@
             }
         }
-        // Test if ways can be joined
-        Way currentWay = null;
-        Node startNode = null, endNode = null;
-        int used = 0;
-        while (true) {
-            Way nextWay = null;
-            for (Way w: ways) {
-                if (w.isClosed()) return ways.size() == 1;
-                if (w == currentWay) continue;
-                if (currentWay == null) {
-                    nextWay = w;
-                    startNode = w.firstNode();
-                    endNode = w.lastNode();
-                    break;
-                }
-                if (w.firstNode() == endNode) {
-                    nextWay = w;
-                    endNode = w.lastNode();
-                    break;
-                }
-                if (w.lastNode() == endNode) {
-                    nextWay = w;
-                    endNode = w.firstNode();
-                    break;
-                }
-            }
-            if (nextWay == null) return false;
-            used += 1;
-            currentWay = nextWay;
-            if (endNode == startNode) return used == ways.size();
-        }
-    }
+        return true;
+    }
+
 }

trunk/src/org/openstreetmap/josm/data/validation/tests/SelfIntersectingWay.java

@@ -12 +12 @@
 import org.openstreetmap.josm.data.validation.Test;
 import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.data.validation.tests.CrossingWays.SelfCrossing;
 
 /**
@@ -62 +63 @@
         }
     }
+
+    /**
+     * Check if the given way is self-intersecting
+     * @param way the way to check
+     * @return {@code true} if way contains some nodes more than once
+     * @see SelfCrossing
+     * @since 17386
+     */
+    public static boolean isSelfIntersecting(Way way) {
+        SelfIntersectingWay test = new SelfIntersectingWay();
+        test.visit(way);
+        return !test.errors.isEmpty();
+    }
 }