Changeset 15852 in josm

Timestamp:

2020-02-15T11:15:05+01:00 (5 years ago)

Author:

GerdP

Message:

fix #10511: Joining complex areas produces exception

• detect case where a single closed way contains the same segment twice or more and fix the "inside" flag
• enable and correct unit test (it "destroyed" preferences.xml)
• init cmdsCount to prevent corrupted undo/redo stacks after a "Join areas internal error."
• make sure that a single command is placed on the undo/redo stack in case of successfull execution. The old code sometimes created two, the latter one "Move tags from ways to relations". I found no good reason for that.
• Try to clean up undo/redo stack if user cancelled operation in 2nd popup asking about relations. Still a dirty hack but a less dangerous one.

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/JoinAreasAction.java (modified) (9 diffs)
• test/unit/org/openstreetmap/josm/actions/JoinAreasActionTest.java (modified) (4 diffs)

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

@@ -183 +183 @@
                     Objects.equals(way, that.way);
         }
+
+        @Override
+        public String toString() {
+            return "w" + way.getUniqueId() + " " + way.getNodesCount() + " nodes";
+        }
     }
 
@@ -254 +259 @@
          */
         WayTraverser(Collection<WayInPolygon> ways) {
-            availableWays = new HashSet<>(ways);
+            availableWays = new LinkedHashSet<>(ways);
             lastWay = null;
         }
@@ -482 +487 @@
      */
     public void join(Collection<Way> ways) {
+        cmdsCount = 0;
         addedRelations.clear();
 
@@ -548 +554 @@
                 commitCommands(tr("Move tags from ways to relations"));
 
+                makeCommitsOneAction(marktr("Joined overlapping areas"));
+
                 if (result.polygons != null && ds != null) {
                     List<Way> allWays = new ArrayList<>();
@@ -569 +577 @@
             if (addUndoRedo) {
                 UndoRedoHandler.getInstance().undo();
-                UndoRedoHandler.getInstance().getRedoCommands().clear();
+                // add no-change commands to the stack to remove the half-done commands
+                Way w = ways.iterator().next();
+                cmds.add(new ChangeCommand(w, w));
+                cmds.add(new ChangeCommand(w, w));
+                commitCommands(tr("Reverting changes"));
+                UndoRedoHandler.getInstance().undo();
             }
         }
@@ -597 +610 @@
      * @return new area formed.
      * @throws UserCancelException if user cancels the operation
-     */
-    public JoinAreasResult joinAreas(List<Multipolygon> areas) throws UserCancelException {
+     * @since xxx : visibility changed from public to private
+     */
+    private JoinAreasResult joinAreas(List<Multipolygon> areas) throws UserCancelException {
 
         // see #11026 - Because <ways> is a dynamic filtered (on ways) of a filtered (on selected objects) collection,
@@ -705 +719 @@
             }
         }
-
-        makeCommitsOneAction(marktr("Joined overlapping areas"));
 
         if (warnAboutRelations) {
@@ -1008 +1020 @@
         }
 
+        revertDuplicateTwoNodeWays(result);
+
         return result;
+    }
+
+    /**
+     * Correct possible error in markWayInsideSide result when splitting a self-intersecting way.
+     * If we have two ways with the same two nodes and the same direction there must be a self intersection.
+     * Change the direction flag for the latter of the two ways. The result is that difference between the number
+     * of ways with insideToTheRight = {@code true} and those with insideToTheRight = {@code false}
+     * differs by 0 or 1, not more.
+     * <p>See #10511
+     * @param parts the parts of a single closed way
+     */
+    private static void revertDuplicateTwoNodeWays(List<WayInPolygon> parts) {
+        for (int i = 0; i < parts.size(); i++) {
+            WayInPolygon w1 = parts.get(i);
+            if (w1.way.getNodesCount() != 2)
+                continue;
+            for (int j = i + 1; j < parts.size(); j++) {
+                WayInPolygon w2 = parts.get(j);
+                if (w2.way.getNodesCount() == 2 && w1.insideToTheRight == w2.insideToTheRight
+                        && w1.way.firstNode() == w2.way.firstNode() && w1.way.lastNode() == w2.way.lastNode()) {
+                    w2.insideToTheRight = !w2.insideToTheRight;
+                }
+            }
+        }
     }
 
@@ -1162 +1200 @@
                 cleanMultigonWays.add(way);
         }
-
         WayTraverser traverser = new WayTraverser(cleanMultigonWays);
         List<AssembledPolygon> result = new ArrayList<>();

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

@@ -15 +15 @@
 import java.util.Set;
 
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -50 +49 @@
     @Rule
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().main().projection();
+    public JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
 
     /**
@@ -58 +57 @@
      */
     @Test
-    @Ignore("disable this test, needs further working") // XXX
     public void testTicket10511() throws IOException, IllegalDataException {
         try (InputStream is = TestUtils.getRegressionDataStream(10511, "10511_mini.osm")) {
@@ -66 +64 @@
             try {
                 new JoinAreasAction(false).join(ds.getWays());
+                Collection<IPrimitive> found = SearchAction.searchAndReturn("type:way", SearchMode.replace);
+                assertEquals(1, found.size());
             } finally {
                 // Ensure we clean the place before leaving, even if test fails.