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(); + } }