Index: /trunk/src/org/openstreetmap/josm/actions/CreateCircleAction.java =================================================================== --- /trunk/src/org/openstreetmap/josm/actions/CreateCircleAction.java (revision 8302) +++ /trunk/src/org/openstreetmap/josm/actions/CreateCircleAction.java (revision 8303) @@ -10,4 +10,5 @@ import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; import java.util.LinkedList; @@ -28,4 +29,5 @@ import org.openstreetmap.josm.gui.Notification; import org.openstreetmap.josm.tools.Geometry; +import org.openstreetmap.josm.tools.RightAndLefthandTraffic; import org.openstreetmap.josm.tools.Shortcut; @@ -115,10 +117,5 @@ @Override public int compare(PolarNode pc1, PolarNode pc2) { - if(pc1.a < pc2.a) - return -1; - else if(pc1.a == pc2.a) - return 0; - else - return 1; + return Double.compare(pc1.a, pc2.a); } } @@ -137,25 +134,18 @@ Collection sel = getCurrentDataSet().getSelected(); - List nodes = new LinkedList<>(); + List nodes = OsmPrimitive.getFilteredList(sel, Node.class); + List ways = OsmPrimitive.getFilteredList(sel, Way.class); + Way existingWay = null; - - for (OsmPrimitive osm : sel) - if (osm instanceof Node) { - nodes.add((Node)osm); - } // special case if no single nodes are selected and exactly one way is: // then use the way's nodes - if (nodes.isEmpty() && (sel.size() == 1)) { - for (OsmPrimitive osm : sel) - if (osm instanceof Way) { - existingWay = ((Way)osm); - for (Node n : ((Way)osm).getNodes()) - { - if(!nodes.contains(n)) { - nodes.add(n); - } - } + if (nodes.isEmpty() && (ways.size() == 1)) { + existingWay = ways.get(0); + for (Node n : existingWay.getNodes()) { + if(!nodes.contains(n)) { + nodes.add(n); } + } } @@ -210,7 +200,7 @@ // build a way for the circle - List wayToAdd = new ArrayList<>(); + List nodesToAdd = new ArrayList<>(); for(int i = 0; i < nodes.size(); i++) { - wayToAdd.add(angles[i].node); + nodesToAdd.add(angles[i].node); double delta = angles[(i+1) % nodes.size()].a - angles[i].a; if(delta < 0) @@ -226,16 +216,21 @@ } Node n = new Node(ll); - wayToAdd.add(n); + nodesToAdd.add(n); cmds.add(new AddCommand(n)); } } - wayToAdd.add(wayToAdd.get(0)); // close the circle + nodesToAdd.add(nodesToAdd.get(0)); // close the circle + if (existingWay != null && existingWay.getNodesCount() >= 3) { + nodesToAdd = orderNodesByWay(nodesToAdd, existingWay); + } else { + nodesToAdd = orderNodesByTrafficHand(nodesToAdd); + } if (existingWay == null) { Way newWay = new Way(); - newWay.setNodes(wayToAdd); + newWay.setNodes(nodesToAdd); cmds.add(new AddCommand(newWay)); } else { Way newWay = new Way(existingWay); - newWay.setNodes(wayToAdd); + newWay.setNodes(nodesToAdd); cmds.add(new ChangeCommand(existingWay, newWay)); } @@ -243,4 +238,40 @@ Main.main.undoRedo.add(new SequenceCommand(tr("Create Circle"), cmds)); Main.map.repaint(); + } + + /** + * Order nodes according to left/right hand traffic. + * @param nodes Nodes list to be ordered. + * @return Modified nodes list ordered according hand traffic. + */ + private List orderNodesByTrafficHand(List nodes) { + boolean rightHandTraffic = true; + for (Node n: nodes) { + if (!RightAndLefthandTraffic.isRightHandTraffic(n.getCoor())) { + rightHandTraffic = false; + break; + } + } + if (rightHandTraffic == Geometry.isClockwise(nodes)) { + Collections.reverse(nodes); + } + return nodes; + } + + /** + * Order nodes according to way direction. + * @param nodes Nodes list to be ordered. + * @param way Way used to determine direction. + * @return Modified nodes list with same direction as way. + */ + private List orderNodesByWay(List nodes, Way way) { + List wayNodes = way.getNodes(); + if (!way.isClosed()) { + wayNodes.add(wayNodes.get(0)); + } + if (Geometry.isClockwise(wayNodes) != Geometry.isClockwise(nodes)) { + Collections.reverse(nodes); + } + return nodes; } Index: /trunk/src/org/openstreetmap/josm/tools/Geometry.java =================================================================== --- /trunk/src/org/openstreetmap/josm/tools/Geometry.java (revision 8302) +++ /trunk/src/org/openstreetmap/josm/tools/Geometry.java (revision 8303) @@ -688,14 +688,24 @@ */ public static boolean isClockwise(Way w) { - if (!w.isClosed()) { + return isClockwise(w.getNodes()); + } + + /** + * Determines whether path from nodes list is oriented clockwise. + * @see #isClockwise(Way) + * @param nodes Nodes list to be checked. + * @return true if and only if way is oriented clockwise. + * @throws IllegalArgumentException if way is not closed (see {@link Way#isClosed}). + */ + public static boolean isClockwise(List nodes) { + double area2 = 0.; + int nodesCount = nodes.size(); + if (nodesCount < 3 || nodes.get(0) != nodes.get(nodesCount - 1)) { throw new IllegalArgumentException("Way must be closed to check orientation."); } - double area2 = 0.; - int nodesCount = w.getNodesCount(); - for (int node = 1; node <= /*sic! consider last-first as well*/ nodesCount; node++) { - LatLon coorPrev = w.getNode(node - 1).getCoor(); - LatLon coorCurr = w.getNode(node % nodesCount).getCoor(); + LatLon coorPrev = nodes.get(node - 1).getCoor(); + LatLon coorCurr = nodes.get(node % nodesCount).getCoor(); area2 += coorPrev.lon() * coorCurr.lat(); area2 -= coorCurr.lon() * coorPrev.lat(); Index: /trunk/test/unit/org/openstreetmap/josm/actions/CreateCircleActionTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/actions/CreateCircleActionTest.java (revision 8303) +++ /trunk/test/unit/org/openstreetmap/josm/actions/CreateCircleActionTest.java (revision 8303) @@ -0,0 +1,185 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.actions; + +import static org.junit.Assert.assertTrue; + +import java.awt.geom.Area; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.openstreetmap.josm.JOSMFixture; +import org.openstreetmap.josm.Main; +import org.openstreetmap.josm.data.coor.EastNorth; +import org.openstreetmap.josm.data.coor.LatLon; +import org.openstreetmap.josm.data.osm.BBox; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.Node; +import org.openstreetmap.josm.data.osm.OsmPrimitive; +import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.gui.layer.OsmDataLayer; +import org.openstreetmap.josm.tools.GeoPropertyIndex; +import org.openstreetmap.josm.tools.GeoPropertyIndex.GeoProperty; +import org.openstreetmap.josm.tools.Geometry; +import org.openstreetmap.josm.tools.RightAndLefthandTraffic; + +/** + * Unit tests for class CreateCircleAction. + */ +public final class CreateCircleActionTest { + + /** + * Setup test. + */ + @BeforeClass + public static void setUp() { + JOSMFixture.createUnitTestFixture().init(true); + } + + /** + * FIXME: Conveniance method to prevent Selection Change events. + * A more proper way should be to define a TestingDataSet class with limited + * functionalities, but DataSet is declare as final (due to Cloneable interface). + * + * I don't know why, but in other tests there are no problem to add selected primitives + * but in this case there is a problem with an even listener of selection change. + */ + public void addSelected(OsmPrimitive p, DataSet ds) { + try { + Method method = ds.getClass() + .getDeclaredMethod("addSelected", + new Class[] {Collection.class, boolean.class}); + method.setAccessible(true); + method.invoke(ds, Collections.singleton(p), false); + } catch (Exception e) { + e.printStackTrace(); + assertTrue("Can't add OsmPrimitive to dataset", false); + } + } + + /** + * Test case: When Create Circle action is performed with a single way selected, + * circle direction must equals way direction. + * see #7421 + */ + @Test + public void test7421_0() { + DataSet dataSet = new DataSet(); + OsmDataLayer layer = new OsmDataLayer(dataSet, OsmDataLayer.createNewName(), null); + + Node n1 = new Node(new EastNorth(0, 0)); + Node n2 = new Node(new EastNorth(-1, 1)); + Node n3 = new Node(new EastNorth(1, 1)); + dataSet.addPrimitive(n1); + dataSet.addPrimitive(n2); + dataSet.addPrimitive(n3); + + Way w = new Way(); // Way is Clockwize + w.setNodes(Arrays.asList(new Node[] {n1, n2, n3})); + dataSet.addPrimitive(w); + + addSelected(w, dataSet); + + CreateCircleAction action = new CreateCircleAction(); + action.setEnabled(true); + try { + Main.main.addLayer(layer); + action.actionPerformed(null); + } finally { + // Ensure we clean the place before leaving, even if test fails. + Main.map.mapView.removeLayer(layer); + } + + // Expected result: Dataset contain one closed way, clockwise + Collection resultingWays = dataSet.getWays(); + assertTrue(String.format("Expect one way after perform action. %d found", + resultingWays.size()), + resultingWays.size() == 1); + Way resultingWay = resultingWays.iterator().next(); + assertTrue("Resulting way is not closed", + resultingWay.isClosed()); + assertTrue("Found anti-clockwize circle while way was clockwize", + Geometry.isClockwise(resultingWay)); + } + + /** + * Mock left/right hand traffic database with constant traffic hand + */ + private static class ConstantTrafficHand implements GeoProperty { + boolean isLeft; + ConstantTrafficHand(boolean isLeft) { + this.isLeft = isLeft; + } + @Override + public Boolean get(LatLon ll) { + return isLeft; + } + @Override + public Boolean get(BBox box) { + return isLeft; + } + } + + /** + * Test case: When Create Circle action is performed with nodes, resulting + * circle direction depend on traffic hand. Simulate a left hand traffic. + * see #7421 + */ + @Test + public void test7421_1() { + DataSet dataSet = new DataSet(); + OsmDataLayer layer = new OsmDataLayer(dataSet, OsmDataLayer.createNewName(), null); + + Node n1 = new Node(new EastNorth(0, 0)); + Node n2 = new Node(new EastNorth(-1, 1)); + Node n3 = new Node(new EastNorth(1, 1)); + dataSet.addPrimitive(n1); + dataSet.addPrimitive(n2); + dataSet.addPrimitive(n3); + + addSelected(n1, dataSet); + addSelected(n2, dataSet); + addSelected(n3, dataSet); + + // Mock left/right hand traffic database + try { + Field leftHandTrafficPolygons = RightAndLefthandTraffic.class + .getDeclaredField("leftHandTrafficPolygons"); + leftHandTrafficPolygons.setAccessible(true); + leftHandTrafficPolygons.set(null, new ArrayList()); + Field rlCache = RightAndLefthandTraffic.class.getDeclaredField("rlCache"); + rlCache.setAccessible(true); + ConstantTrafficHand trafficHand = new ConstantTrafficHand(true); + rlCache.set(null, new GeoPropertyIndex(trafficHand, 24)); + } catch (Exception e) { + e.printStackTrace(); + assertTrue("Impossible to mock left/right hand database", false); + } + + CreateCircleAction action = new CreateCircleAction(); + action.setEnabled(true); + try { + Main.main.addLayer(layer); + action.actionPerformed(null); + } finally { + // Ensure we clean the place before leaving, even if test fails. + Main.map.mapView.removeLayer(layer); + } + + // Expected result: Dataset contain one closed way, clockwise + Collection resultingWays = dataSet.getWays(); + assertTrue(String.format("Expect one way after perform action. %d found", + resultingWays.size()), + resultingWays.size() == 1); + Way resultingWay = resultingWays.iterator().next(); + assertTrue("Resulting way is not closed", + resultingWay.isClosed()); + assertTrue("Found anti-clockwise way while traffic is left hand.", + Geometry.isClockwise(resultingWay)); + } +}