Index: trunk/src/org/openstreetmap/josm/actions/AlignInCircleAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/AlignInCircleAction.java (revision 17385) +++ trunk/src/org/openstreetmap/josm/actions/AlignInCircleAction.java (revision 17386) @@ -28,6 +28,10 @@ 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,13 +59,39 @@ /** - * 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 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,12 +99,33 @@ * 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. - * + *

* Case 1: One unclosed way. * --> 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. - * + *

* Case 2: One or more ways are selected and can be joined into a polygon * --> allow action, and align selected ways nodes @@ -85,14 +136,15 @@ * In all cases, selected nodes are fix, nodes with more than one referrers are fix * (first referrer is the selected way) - * + *

* Case 3: Only nodes are selected * --> Align these nodes, all are fix - */ - @Override - public void actionPerformed(ActionEvent e) { - if (!isEnabled()) - return; - - Collection 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 sel = ds.getSelected(); List nodes = new LinkedList<>(); // fixNodes: All nodes for which the angle relative to center should not be modified @@ -113,4 +165,8 @@ // 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,5 +177,5 @@ 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,10 +208,5 @@ 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,12 +215,12 @@ // 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,9 +228,5 @@ 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,7 +242,5 @@ } - if (!actionAllowed(nodes)) return; - - Collection cmds = new LinkedList<>(); + List cmds = new LinkedList<>(); // Move each node to that distance from the center. @@ -217,5 +262,5 @@ 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,11 +276,12 @@ 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,61 +299,17 @@ * @param ways List of ways to be joined * @return Nodes anticlockwise ordered - */ - private static List collectNodesAnticlockwise(List ways) { - List 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 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 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 collectNodesAnticlockwise(List ways) throws InvalidSelection { + Collection rings = Multipolygon.joinWays(ways); + if (rings.size() != 1) + throw new InvalidSelection(); + List 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 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,9 +326,11 @@ /** - * 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 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,35 +342,6 @@ } } - // 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; + } + } Index: trunk/src/org/openstreetmap/josm/data/validation/tests/SelfIntersectingWay.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/SelfIntersectingWay.java (revision 17385) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/SelfIntersectingWay.java (revision 17386) @@ -12,4 +12,5 @@ import org.openstreetmap.josm.data.validation.Test; import org.openstreetmap.josm.data.validation.TestError; +import org.openstreetmap.josm.data.validation.tests.CrossingWays.SelfCrossing; /** @@ -62,3 +63,16 @@ } } + + /** + * 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(); + } } Index: trunk/test/data/alignCircleAfter1.osm =================================================================== --- trunk/test/data/alignCircleAfter1.osm (revision 17386) +++ trunk/test/data/alignCircleAfter1.osm (revision 17386) @@ -0,0 +1,171 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/alignCircleAfter2.osm =================================================================== --- trunk/test/data/alignCircleAfter2.osm (revision 17386) +++ trunk/test/data/alignCircleAfter2.osm (revision 17386) @@ -0,0 +1,171 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/alignCircleBefore.osm =================================================================== --- trunk/test/data/alignCircleBefore.osm (revision 17386) +++ trunk/test/data/alignCircleBefore.osm (revision 17386) @@ -0,0 +1,171 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/alignCircleTwoWaysAfter.osm =================================================================== --- trunk/test/data/alignCircleTwoWaysAfter.osm (revision 17386) +++ trunk/test/data/alignCircleTwoWaysAfter.osm (revision 17386) @@ -0,0 +1,181 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/data/alignCircleTwoWaysBefore.osm =================================================================== --- trunk/test/data/alignCircleTwoWaysBefore.osm (revision 17386) +++ trunk/test/data/alignCircleTwoWaysBefore.osm (revision 17386) @@ -0,0 +1,181 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java (revision 17386) +++ trunk/test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java (revision 17386) @@ -0,0 +1,152 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.actions; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.Set; +import java.util.stream.Collectors; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.command.Command; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.Node; +import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.io.OsmReader; +import org.openstreetmap.josm.testutils.JOSMTestRules; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + +/** + * Unit tests for class {@link AlignInLineAction}. + */ +final class AlignInCircleActionTest { + + /** + * Setup test. + */ + @RegisterExtension + @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") + public JOSMTestRules test = new JOSMTestRules().projection(); + + + /** + * Test case: way with several nodes selected + * @throws Exception if an error occurs + */ + @Test + void testWaySelected() throws Exception { + DataSet ds = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleBefore.osm")), null); + DataSet ds2 = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleAfter1.osm")), null); + + Way roundabout = null; + for (Way w : ds.getWays()) { + if ("roundabout".equals(w.get("junction"))) { + roundabout = w; + break; + } + } + assertNotNull(roundabout); + if (roundabout != null) { + ds.setSelected(roundabout); + Command c = AlignInCircleAction.buildCommand(ds); + c.executeCommand(); + Way expected = (Way) ds2.getPrimitiveById(roundabout); + assertNotNull(expected); + assertEquals(expected, roundabout); + assertEquals(expected.getNodesCount(), roundabout.getNodesCount()); + for (Node n1 : roundabout.getNodes()) { + Node n2 = (Node) ds2.getPrimitiveById(n1); + assertEquals(n1.lat(), n2.lat(), 1e-5); + assertEquals(n1.lon(), n2.lon(), 1e-5); + } + } + } + + /** + * Non-regression test for Bug #20041. + * Don't create move commands when no node is visibly moved + * @throws Exception if an error occurs + */ + @Test + void testTicket20041() throws Exception { + DataSet ds = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleAfter1.osm")), null); + + Way roundabout = null; + for (Way w : ds.getWays()) { + if ("roundabout".equals(w.get("junction"))) { + roundabout = w; + break; + } + } + assertNotNull(roundabout); + if (roundabout != null) { + ds.setSelected(roundabout); + assertThrows(AlignInCircleAction.InvalidSelection.class, () -> AlignInCircleAction.buildCommand(ds)); + } + } + + /** + * Test case: way with several nodes selected + * @throws Exception if an error occurs + */ + @Test + void testNodesSelected() throws Exception { + DataSet ds = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleBefore.osm")), null); + DataSet ds2 = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleAfter2.osm")), null); + + Way circularWay = null; + for (Way w : ds.getWays()) { + if ("roundabout".equals(w.get("junction"))) { + circularWay = w; + break; + } + } + assertNotNull(circularWay); + if (circularWay != null) { + ds.setSelected(circularWay.getNodes()); + Command c = AlignInCircleAction.buildCommand(ds); + assertNotNull(c); + c.executeCommand(); + Way expected = (Way) ds2.getPrimitiveById(circularWay); + assertNotNull(expected); + assertEquals(expected, circularWay); + assertEquals(expected.getNodesCount(), circularWay.getNodesCount()); + for (Node n1 : circularWay.getNodes()) { + Node n2 = (Node) ds2.getPrimitiveById(n1); + assertEquals(n1.lat(), n2.lat(), 1e-5); + assertEquals(n1.lon(), n2.lon(), 1e-5); + } + } + } + + /** + * Test case: original roundabout was split, two ways selected, they build a closed ring + * @throws Exception if an error occurs + */ + @Test + void testOpenWaysSelected() throws Exception { + DataSet ds = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleTwoWaysBefore.osm")), null); + DataSet ds2 = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleTwoWaysAfter.osm")), null); + + Set junctions = ds.getWays().stream().filter(w -> "roundabout".equals(w.get("junction"))).collect(Collectors.toSet()); + assertEquals(2, junctions.size()); + ds.setSelected(junctions); + Command c = AlignInCircleAction.buildCommand(ds); + assertNotNull(c); + c.executeCommand(); + for (Way way : junctions) { + for (Node n1 : way.getNodes()) { + Node n2 = (Node) ds2.getPrimitiveById(n1); + assertEquals(n1.lat(), n2.lat(), 1e-5); + assertEquals(n1.lon(), n2.lon(), 1e-5); + } + } + } + +}