Changeset 8871 in josm

Timestamp:

2015-10-14T22:23:55+02:00 (9 years ago)

Author:

simon04

Message:

fix #10018 fix #11945 - Improve relation member conflict resolution when combining ways

Keep/delete decisions are made if every member has the same role and the
members are in consecutive order within the relation.

Moves CombinePrimitiveResolverDialog#prepareDefaultRelationDecisions
to RelationMemberConflictResolverModel#prepareDefaultRelationDecisions.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/conflict/tags/CombinePrimitiveResolverDialog.java (modified) (3 diffs)
• src/org/openstreetmap/josm/gui/conflict/tags/RelationMemberConflictDecision.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/conflict/tags/RelationMemberConflictResolverModel.java (modified) (7 diffs)
• test/unit/org/openstreetmap/josm/gui/conflict/tags/RelationMemberConflictResolverModelTest.java (added)

trunk/src/org/openstreetmap/josm/gui/conflict/tags/CombinePrimitiveResolverDialog.java

@@ -18 +18 @@
 import java.beans.PropertyChangeListener;
 import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
@@ -51 +48 @@
 import org.openstreetmap.josm.tools.CheckParameterUtil;
 import org.openstreetmap.josm.tools.ImageProvider;
-import org.openstreetmap.josm.tools.MultiMap;
-import org.openstreetmap.josm.tools.Predicates;
 import org.openstreetmap.josm.tools.Utils;
 import org.openstreetmap.josm.tools.Utils.Function;
@@ -298 +293 @@
     }
 
-    protected void prepareDefaultTagDecisions() {
+    /**
+     * Prepares the default decisions for populated tag and relation membership conflicts.
+     */
+    public void prepareDefaultDecisions() {
         getTagConflictResolverModel().prepareDefaultTagDecisions();
-    }
-
-    protected void prepareDefaultRelationDecisions() {
-        final RelationMemberConflictResolverModel model = getRelationMemberConflictResolverModel();
-        final Map<Relation, Integer> numberOfKeepResolutions = new HashMap<>();
-        final MultiMap<OsmPrimitive, Relation> resolvedRelationsPerPrimitive = new MultiMap<>();
-
-        for (int i = 0; i < model.getNumDecisions(); i++) {
-            final RelationMemberConflictDecision decision = model.getDecision(i);
-            final Relation r = decision.getRelation();
-            final OsmPrimitive p = decision.getOriginalPrimitive();
-            if (!numberOfKeepResolutions.containsKey(r)) {
-                decision.decide(RelationMemberConflictDecisionType.KEEP);
-                numberOfKeepResolutions.put(r, 1);
-                resolvedRelationsPerPrimitive.put(p, r);
-                continue;
-            }
-
-            final Integer keepResolutions = numberOfKeepResolutions.get(r);
-            final Collection<Relation> resolvedRelations = Utils.firstNonNull(
-                    resolvedRelationsPerPrimitive.get(p), Collections.<Relation>emptyList());
-            if (keepResolutions <= Utils.filter(resolvedRelations, Predicates.equalTo(r)).size()) {
-                // old relation contains one primitive more often than the current resolution => keep the current member
-                decision.decide(RelationMemberConflictDecisionType.KEEP);
-                numberOfKeepResolutions.put(r, keepResolutions + 1);
-                resolvedRelationsPerPrimitive.put(p, r);
-            } else {
-                decision.decide(RelationMemberConflictDecisionType.REMOVE);
-                resolvedRelationsPerPrimitive.put(p, r);
-            }
-        }
-        model.refresh();
-    }
-
-    /**
-     * Prepares the default decisions for populated tag and relation membership conflicts.
-     */
-    public void prepareDefaultDecisions() {
-        prepareDefaultTagDecisions();
-        prepareDefaultRelationDecisions();
+        getRelationMemberConflictResolverModel().prepareDefaultRelationDecisions();
     }
 

trunk/src/org/openstreetmap/josm/gui/conflict/tags/RelationMemberConflictDecision.java

@@ -115 +115 @@
         return true;
     }
+
+    @Override
+    public String toString() {
+        return originalPrimitive.getPrimitiveId() + " at index " + pos + " with role " + role + " in " + relation.getUniqueId() + " => " + decision;
+    }
 }

trunk/src/org/openstreetmap/josm/gui/conflict/tags/RelationMemberConflictResolverModel.java

@@ -6 +6 @@
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
+import java.util.TreeSet;
 
 import javax.swing.table.DefaultTableModel;
@@ -20 +25 @@
 import org.openstreetmap.josm.data.osm.RelationToChildReference;
 import org.openstreetmap.josm.gui.util.GuiHelper;
+import org.openstreetmap.josm.tools.Predicate;
+import org.openstreetmap.josm.tools.Utils;
 
 /**
@@ -34 +41 @@
     /** the collection of relations for which we manage conflicts */
     protected transient Collection<Relation> relations;
+    /** the collection of primitives for which we manage conflicts */
+    protected transient Collection<? extends OsmPrimitive> primitives;
     /** the number of conflicts */
     private int numConflicts;
@@ -151 +160 @@
     public void populate(Collection<Relation> relations, Collection<? extends OsmPrimitive> memberPrimitives) {
         decisions.clear();
-        relations = relations == null ? new LinkedList<Relation>() : relations;
+        relations = relations == null ? Collections.<Relation>emptyList() : relations;
         memberPrimitives = memberPrimitives == null ? new LinkedList<OsmPrimitive>() : memberPrimitives;
         for (Relation r : relations) {
@@ -159 +168 @@
         }
         this.relations = relations;
+        this.primitives = memberPrimitives;
         refresh();
     }
@@ -172 +182 @@
         decisions.clear();
         this.relations = new HashSet<>(references.size());
+        final Collection<OsmPrimitive> primitives = new HashSet<>();
         for (RelationToChildReference reference: references) {
             decisions.add(new RelationMemberConflictDecision(reference.getParent(), reference.getPosition()));
             relations.add(reference.getParent());
-        }
+            primitives.add(reference.getChild());
+        }
+        this.primitives = primitives;
         refresh();
+    }
+
+    /**
+     * Prepare the default decisions for the current model.
+     *
+     * Keep/delete decisions are made if every member has the same role and the members are in consecutive order within the relation.
+     * For multiple occurrences those conditions are tested stepwise for each occurrence.
+     */
+    public void prepareDefaultRelationDecisions() {
+
+        for (final Relation relation : relations) {
+            final Map<OsmPrimitive, List<RelationMemberConflictDecision>> decisionsByPrimitive = new LinkedHashMap<>(primitives.size(), 1);
+            for (final RelationMemberConflictDecision decision : decisions) {
+                if (decision.getRelation() == relation) {
+                    final OsmPrimitive primitive = decision.getOriginalPrimitive();
+                    if (!decisionsByPrimitive.containsKey(primitive)) {
+                        decisionsByPrimitive.put(primitive, new ArrayList<RelationMemberConflictDecision>());
+                    }
+                    decisionsByPrimitive.get(primitive).add(decision);
+                }
+            }
+
+            //noinspection StatementWithEmptyBody
+            if (!decisionsByPrimitive.keySet().containsAll(primitives)) {
+                // some primitives are not part of the relation, leave undecided
+            } else {
+                final Collection<Iterator<RelationMemberConflictDecision>> iterators = new ArrayList<>(primitives.size());
+                for (final Collection<RelationMemberConflictDecision> i : decisionsByPrimitive.values()) {
+                    iterators.add(i.iterator());
+                }
+                while (Utils.forAll(iterators, new Predicate<Iterator<RelationMemberConflictDecision>>() {
+                    @Override
+                    public boolean evaluate(Iterator<RelationMemberConflictDecision> it) {
+                        return it.hasNext();
+                    }
+                })) {
+                    final List<RelationMemberConflictDecision> decisions = new ArrayList<>();
+                    final Collection<String> roles = new HashSet<>();
+                    final Collection<Integer> indices = new TreeSet<>();
+                    for (Iterator<RelationMemberConflictDecision> it : iterators) {
+                        final RelationMemberConflictDecision decision = it.next();
+                        decisions.add(decision);
+                        roles.add(decision.getRole());
+                        indices.add(decision.getPos());
+                    }
+                    if (roles.size() != 1) {
+                        // roles to not patch, leave undecided
+                        continue;
+                    } else if (!isCollectionOfConsecutiveNumbers(indices)) {
+                        // not consecutive members in relation, leave undecided
+                        continue;
+                    }
+                    decisions.get(0).decide(RelationMemberConflictDecisionType.KEEP);
+                    for (RelationMemberConflictDecision decision : decisions.subList(1, decisions.size())) {
+                        decision.decide(RelationMemberConflictDecisionType.REMOVE);
+                    }
+                }
+            }
+        }
+
+        refresh();
+    }
+
+    static boolean isCollectionOfConsecutiveNumbers(Collection<Integer> numbers) {
+        if (numbers.isEmpty()) {
+            return true;
+        }
+        final Iterator<Integer> it = numbers.iterator();
+        Integer previousValue = it.next();
+        while (it.hasNext()) {
+            final Integer i = it.next();
+            if (previousValue + 1 != i) {
+                return false;
+            }
+            previousValue = i;
+        }
+        return true;
     }
 
@@ -195 +285 @@
      */
     public int getNumDecisions() {
-        return decisions == null ? 0 : decisions.size();
+        return decisions == null /* accessed via super constructor */ ? 0 : decisions.size();
     }