Ticket #17768: 17768-v0.patch

src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java

@@ -2 +2 @@
 package org.openstreetmap.josm.actions;
 
 import static org.openstreetmap.josm.tools.I18n.tr;
+import static org.openstreetmap.josm.tools.I18n.trn;
 
 import java.awt.event.ActionEvent;
 import java.awt.event.KeyEvent;
@@ -11 +12 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -28 +31 @@
 import org.openstreetmap.josm.command.SequenceCommand;
 import org.openstreetmap.josm.data.UndoRedoHandler;
 import org.openstreetmap.josm.data.osm.DataSet;
-import org.openstreetmap.josm.data.osm.MultipolygonBuilder;
-import org.openstreetmap.josm.data.osm.MultipolygonBuilder.JoinedPolygon;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.OsmUtils;
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.RelationMember;
 import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon;
+import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.data.validation.tests.MultipolygonTest;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.Notification;
 import org.openstreetmap.josm.gui.dialogs.relation.DownloadRelationMemberTask;
@@ -212 +216 @@
         Set<Way> ways = new HashSet<>(selectedWays);
         ways.addAll(selectedMultipolygonRelation.getMemberPrimitives(Way.class));
 
-        final MultipolygonBuilder polygon = analyzeWays(ways, true);
-        if (polygon == null) {
-            return null; //could not make multipolygon.
-        } else {
-            return Pair.create(selectedMultipolygonRelation, createRelation(polygon, selectedMultipolygonRelation));
+        // even if no way was added the inner/outer roles might be different
+        MultipolygonTest mpTest = new MultipolygonTest();
+        Pair<Multipolygon, Relation> tested = mpTest.makeFromWays(ways);
+        if (mpTest.getErrors().isEmpty()) {
+            return mergeRelationsMembers(selectedMultipolygonRelation, tested.b);
         }
+        showErrors(mpTest.getErrors());
+        return null; //could not make multipolygon.
     }
 
     /**
+     * Merge members of multipolygon relation. Maintains the order of the old relation. May change roles,
+     * removes duplicate and non-way members and adds new members found in {@code calculated}.
+     * @param old old multipolygon relation
+     * @param calculated calculated multipolygon relation
+     * @return pair of old and new multipolygon relation
+     */
+    private static Pair<Relation, Relation> mergeRelationsMembers(Relation old, Relation calculated) {
+        Set<RelationMember> merged = new LinkedHashSet<>();
+        boolean foundDiff = false;
+        int nonWayMember = 0;
+        // maintain order of members in updated relation
+        for (RelationMember oldMem :old.getMembers()) {
+            if (oldMem.isNode() || oldMem.isRelation()) {
+                nonWayMember++;
+                continue;
+            }
+            for (RelationMember newMem : calculated.getMembers()) {
+                if (newMem.getMember().equals(oldMem.getMember())) {
+                    if (!newMem.getRole().equals(oldMem.getRole())) {
+                        foundDiff = true;
+                    }
+                    foundDiff |= !merged.add(newMem); // detect duplicate members in old relation
+                    break;
+                }
+            }
+        }
+        if (nonWayMember > 0) {
+            foundDiff = true;
+            String msg = trn("Non-Way member removed from multipolygon", "Non-Way members removed from multipolygon", nonWayMember);
+            GuiHelper.runInEDT(() -> new Notification(msg).setIcon(JOptionPane.WARNING_MESSAGE).show());
+        }
+        foundDiff |= merged.addAll(calculated.getMembers());
+        if (!foundDiff) {
+            return Pair.create(old, old); // unchanged
+        }
+        Relation toModify = new Relation(old);
+        toModify.setMembers(new ArrayList<>(merged));
+        return Pair.create(old, toModify);
+    }
+
+    /**
      * Returns a {@link Pair} null and the newly created/modified multipolygon {@link Relation}.
      * @param selectedWays selected ways
      * @param showNotif if {@code true}, shows a notification if an error occurs
@@ -227 +274 @@
      * @return pair of null and new multipolygon relation
      */
     public static Pair<Relation, Relation> createMultipolygonRelation(Collection<Way> selectedWays, boolean showNotif) {
+        MultipolygonTest mpTest = new MultipolygonTest();
+        Pair<Multipolygon, Relation> tested = mpTest.makeFromWays(selectedWays);
+        tested.b.setMembers(RelationSorter.sortMembersByConnectivity(tested.b.getMembers()));
+        if (mpTest.getErrors().isEmpty())
+            return Pair.create(null, tested.b);
+        if (showNotif) {
+            showErrors(mpTest.getErrors());
+        }
+        return null; //could not make multipolygon.
+    }
 
-        final MultipolygonBuilder polygon = analyzeWays(selectedWays, showNotif);
-        if (polygon == null) {
-            return null; //could not make multipolygon.
-        } else {
-            return Pair.create(null, createRelation(polygon, null));
+    private static void showErrors(List<TestError> errors) {
+        if (!errors.isEmpty()) {
+            StringBuilder sb = new StringBuilder();
+            Set<String> errorMessages = new LinkedHashSet<>();
+            errors.forEach(e-> errorMessages.add(e.getMessage()));
+            Iterator<String> iter = errorMessages.iterator();
+            while (iter.hasNext()) {
+                sb.append(iter.next());
+                if (iter.hasNext())
+                    sb.append('\n');
+            }
+            GuiHelper.runInEDT(() -> new Notification(sb.toString()).setIcon(JOptionPane.INFORMATION_MESSAGE).show());
         }
     }
 
@@ -260 +324 @@
             list.add(new AddCommand(selectedWays.iterator().next().getDataSet(), relation));
             commandName = getName(false);
         } else {
-            list.add(new ChangeCommand(existingRelation, relation));
+            if (existingRelation != relation) {
+                list.add(new ChangeCommand(existingRelation, relation));
+            }
+            if (list.isEmpty()) {
+                GuiHelper.runInEDT(() -> new Notification(tr("Nothing changed")).setDuration(Notification.TIME_SHORT).setIcon(JOptionPane.INFORMATION_MESSAGE).show());
+                return null;
+            }
             commandName = getName(true);
         }
         return Pair.create(new SequenceCommand(commandName, list), relation);
@@ -289 +359 @@
         }
     }
 
-    /**
-     * This method analyzes ways and creates multipolygon.
-     * @param selectedWays list of selected ways
-     * @param showNotif if {@code true}, shows a notification if an error occurs
-     * @return <code>null</code>, if there was a problem with the ways.
-     */
-    private static MultipolygonBuilder analyzeWays(Collection<Way> selectedWays, boolean showNotif) {
-
-        MultipolygonBuilder pol = new MultipolygonBuilder();
-        final String error = pol.makeFromWays(selectedWays);
-
-        if (error != null) {
-            if (showNotif) {
-                GuiHelper.runInEDT(() ->
-                        new Notification(error)
-                        .setIcon(JOptionPane.INFORMATION_MESSAGE)
-                        .show());
-            }
-            return null;
-        } else {
-            return pol;
-        }
-    }
-
-    /**
-     * Builds a relation from polygon ways.
-     * @param pol data storage class containing polygon information
-     * @param clone relation to clone, can be null
-     * @return multipolygon relation
-     */
-    private static Relation createRelation(MultipolygonBuilder pol, Relation clone) {
-        // Create new relation
-        Relation rel = clone != null ? new Relation(clone) : new Relation();
-        rel.put("type", "multipolygon");
-        // Add ways to it
-        for (JoinedPolygon jway:pol.outerWays) {
-            addMembers(jway, rel, "outer");
-        }
-
-        for (JoinedPolygon jway:pol.innerWays) {
-            addMembers(jway, rel, "inner");
-        }
-
-        if (clone == null) {
-            rel.setMembers(RelationSorter.sortMembersByConnectivity(rel.getMembers()));
-        }
-
-        return rel;
-    }
-
-    private static void addMembers(JoinedPolygon polygon, Relation rel, String role) {
-        final int count = rel.getMembersCount();
-        final Set<Way> ways = new HashSet<>(polygon.ways);
-        for (int i = 0; i < count; i++) {
-            final RelationMember m = rel.getMember(i);
-            if (ways.contains(m.getMember()) && !role.equals(m.getRole())) {
-                rel.setMember(i, new RelationMember(role, m.getMember()));
-            }
-        }
-        ways.removeAll(rel.getMemberPrimitivesList());
-        for (final Way way : ways) {
-            rel.addMember(new RelationMember(role, way));
-        }
-    }
-
     private static final List<String> DEFAULT_LINEAR_TAGS = Arrays.asList("barrier", "fence_type", "source");
 
     /**

src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java

@@ -38 +38 @@
 import org.openstreetmap.josm.tools.Geometry;
 import org.openstreetmap.josm.tools.Geometry.PolygonIntersection;
 import org.openstreetmap.josm.tools.Logging;
+import org.openstreetmap.josm.tools.Pair;
 
 /**
  * Checks if multipolygons are valid
@@ -78 +79 @@
     private static final int FOUND_INSIDE = 1;
     private static final int FOUND_OUTSIDE = 2;
 
+    /** set when used to build a multipolygon relation */
+    private Relation createdRelation;
+
     /**
      * Constructs a new {@code MultipolygonTest}.
      */
@@ -463 +467 @@
         if (list == null || list.isEmpty()) {
             return;
         }
-
+        if (r == createdRelation) {
+            List<RelationMember> modMembers = new ArrayList<>();
+            for (PolygonLevel pol : list) {
+                final String calculatedRole = (pol.level % 2 == 0) ? "outer" : "inner";
+                for (long wayId : pol.outerWay.getWayIds()) {
+                    RelationMember member = wayMap.get(wayId);
+                    modMembers.add(new RelationMember(calculatedRole, member.getMember()));
+                }
+            }
+            r.setMembers(modMembers);
+            return;
+        }
         for (PolygonLevel pol : list) {
-            String calculatedRole = (pol.level % 2 == 0) ? "outer" : "inner";
+            final String calculatedRole = (pol.level % 2 == 0) ? "outer" : "inner";
             for (long wayId : pol.outerWay.getWayIds()) {
                 RelationMember member = wayMap.get(wayId);
                 if (!calculatedRole.equals(member.getRole())) {
@@ -893 +908 @@
             return null;
         }
     }
+
+    /**
+     * Create a multipolygon relation from the given ways and test it.
+     * @param ways the collection of ways
+     * @return a pair of a {@link Multipolygon} instance and the relation.
+     */
+    public Pair<Multipolygon, Relation> makeFromWays(Collection<Way> ways) {
+        Relation r = new Relation();
+        createdRelation = r;
+        r.put("type", "multipolygon");
+        for (Way w : ways) {
+            r.addMember(new RelationMember("", w));
+        }
+        errors.clear();
+        Multipolygon polygon = null;
+        boolean hasRepeatedMembers = checkRepeatedWayMembers(r);
+        if (!hasRepeatedMembers) {
+            polygon = new Multipolygon(r);
+            // don't check style consistency here
+            checkGeometryAndRoles(r, polygon);
+        }
+        createdRelation = null;
+        errors.removeIf(e->e.getSeverity() == Severity.OTHER);
+        return new Pair<>(polygon, r);
+    }
+
 }