Changeset 5132 in josm

Timestamp:

Mar 29, 2012 11:22:07 PM (14 months ago)

Author:

simon04

Message:

fix #7513 - Warn non-experts when combining ways with conflicting tags or ways being part of relations

Location:

trunk/src/org/openstreetmap/josm

Files:

• actions/CombineWayAction.java (modified) (5 diffs)
• actions/JoinAreasAction.java (modified) (3 diffs)
• actions/MergeNodesAction.java (modified) (4 diffs)
• data/osm/OsmPrimitive.java (modified) (1 diff)
• gui/DefaultNameFormatter.java (modified) (2 diffs)
• gui/conflict/tags/CombinePrimitiveResolverDialog.java (modified) (8 diffs)
• gui/conflict/tags/TagConflictResolver.java (modified) (2 diffs)
• tools/Utils.java (modified) (4 diffs)

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

@@ -2 +2 @@
 package org.openstreetmap.josm.actions;
 
-import static org.openstreetmap.josm.gui.conflict.tags.TagConflictResolutionUtil.combineTigerTags;
-import static org.openstreetmap.josm.gui.conflict.tags.TagConflictResolutionUtil.completeTagCollectionForEditing;
-import static org.openstreetmap.josm.gui.conflict.tags.TagConflictResolutionUtil.normalizeTagCollectionBeforeEditing;
 import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
 import static org.openstreetmap.josm.tools.I18n.tr;
@@ -34 +31 @@
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
-import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.TagCollection;
 import org.openstreetmap.josm.data.osm.Way;
@@ -93 +89 @@
         }
         return targetWay;
-    }
-
-    /**
-     * Replies the set of referring relations
-     *
-     * @return the set of referring relations
-     */
-    public static Set<Relation> getParentRelations(Collection<Way> ways) {
-        HashSet<Relation> ret = new HashSet<Relation>();
-        for (Way w: ways) {
-            ret.addAll(OsmPrimitive.getFilteredList(w.getReferrers(), Relation.class));
-        }
-        return ret;
     }
 
@@ -194 +177 @@
         modifiedTargetWay.setNodes(path);
 
-        TagCollection completeWayTags = new TagCollection(wayTags);
-        combineTigerTags(completeWayTags);
-        normalizeTagCollectionBeforeEditing(completeWayTags, ways);
-        TagCollection tagsToEdit = new TagCollection(completeWayTags);
-        completeTagCollectionForEditing(tagsToEdit);
-
-        CombinePrimitiveResolverDialog dialog = CombinePrimitiveResolverDialog.getInstance();
-        dialog.getTagConflictResolverModel().populate(tagsToEdit, completeWayTags.getKeysWithMultipleValues());
-        dialog.setTargetPrimitive(targetWay);
-        Set<Relation> parentRelations = getParentRelations(ways);
-        dialog.getRelationMemberConflictResolverModel().populate(
-                parentRelations,
-                ways
-        );
-        dialog.prepareDefaultDecisions();
-
-        // resolve tag conflicts if necessary
-        //
-        if (!completeWayTags.isApplicableToPrimitive() || !parentRelations.isEmpty()) {
-            dialog.setVisible(true);
-            if (dialog.isCanceled())
-                throw new UserCancelException();
-        }
+        List<Command> resolution = CombinePrimitiveResolverDialog.launchIfNecessary(wayTags, ways, Collections.singleton(targetWay));
 
         LinkedList<Command> cmds = new LinkedList<Command>();
@@ -223 +184 @@
 
         cmds.add(new ChangeCommand(targetWay, modifiedTargetWay));
-        cmds.addAll(dialog.buildResolutionCommands());
+        cmds.addAll(resolution);
         cmds.add(new DeleteCommand(deletedWays));
         final SequenceCommand sequenceCommand = new SequenceCommand(tr("Combine {0} ways", ways.size()), cmds);

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

@@ -43 +43 @@
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.conflict.tags.CombinePrimitiveResolverDialog;
-import org.openstreetmap.josm.gui.conflict.tags.TagConflictResolutionUtil;
 import org.openstreetmap.josm.tools.Geometry;
 import org.openstreetmap.josm.tools.Pair;
@@ -538 +537 @@
         }
 
-        if (ways.size() < 2)
+        if (ways.size() < 2) {
             return true;
-
-        //mostly copied from CombineWayAction.java.
+        }
+
         TagCollection wayTags = TagCollection.unionOfAllPrimitives(ways);
-        TagCollection completeWayTags = new TagCollection(wayTags);
-        TagConflictResolutionUtil.combineTigerTags(completeWayTags);
-        TagConflictResolutionUtil.normalizeTagCollectionBeforeEditing(completeWayTags, ways);
-        TagCollection tagsToEdit = new TagCollection(completeWayTags);
-        TagConflictResolutionUtil.completeTagCollectionForEditing(tagsToEdit);
-
-        CombinePrimitiveResolverDialog dialog = CombinePrimitiveResolverDialog.getInstance();
-        dialog.getTagConflictResolverModel().populate(tagsToEdit, completeWayTags.getKeysWithMultipleValues());
-        dialog.setTargetPrimitive(ways.get(0));
-        Collection<Relation> parentRelations = CombineWayAction.getParentRelations(ways);
-        parentRelations = filterOwnMultipolygonRelations(parentRelations, polygons);
-        dialog.getRelationMemberConflictResolverModel().populate(
-                parentRelations,
-                ways
-        );
-        dialog.prepareDefaultDecisions();
-
-        // resolve tag conflicts if necessary
-        //
-        if (!completeWayTags.isApplicableToPrimitive() || !parentRelations.isEmpty()) {
-            dialog.setVisible(true);
-            if (dialog.isCanceled())
-                return false;
-        }
-
-        for (Way way : ways) {
-            dialog.setTargetPrimitive(way);
-            cmds.addAll(dialog.buildResolutionCommands());
-        }
-
-        commitCommands(marktr("Fix tag conflicts"));
-        return true;
+        try {
+            cmds.addAll(CombinePrimitiveResolverDialog.launchIfNecessary(wayTags, ways, ways));
+            commitCommands(marktr("Fix tag conflicts"));
+            return true;
+        } catch (UserCancelException ex) {
+            return false;
+        }
     }
 
@@ -1209 +1183 @@
         Set<Way> processedInnerWays = new LinkedHashSet<Way>();
 
-        for (Relation r : CombineWayAction.getParentRelations(selectedWays)) {
+        for (Relation r : OsmPrimitive.getParentRelations(selectedWays)) {
             if (r.isDeleted() || !r.isMultipolygon()) {
                 continue;

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

@@ -9 +9 @@
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedList;
@@ -22 +23 @@
 import org.openstreetmap.josm.command.DeleteCommand;
 import org.openstreetmap.josm.command.SequenceCommand;
+import org.openstreetmap.josm.corrector.UserCancelException;
 import org.openstreetmap.josm.data.coor.EastNorth;
 import org.openstreetmap.josm.data.coor.LatLon;
@@ -33 +35 @@
 import org.openstreetmap.josm.gui.HelpAwareOptionPane.ButtonSpec;
 import org.openstreetmap.josm.gui.conflict.tags.CombinePrimitiveResolverDialog;
-import org.openstreetmap.josm.gui.conflict.tags.TagConflictResolutionUtil;
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.tools.CheckParameterUtil;
@@ -256 +257 @@
         CheckParameterUtil.ensureParameterNotNull(layer, "layer");
         CheckParameterUtil.ensureParameterNotNull(targetNode, "targetNode");
-        if (nodes == null)
+        if (nodes == null) {
             return null;
+        }
 
         Set<RelationToChildReference> relationToNodeReferences = RelationToChildReference.getRelationToChildReferences(nodes);
 
-        // build the tag collection
-        //
-        TagCollection nodeTags = TagCollection.unionOfAllPrimitives(nodes);
-        TagConflictResolutionUtil.combineTigerTags(nodeTags);
-        TagConflictResolutionUtil.normalizeTagCollectionBeforeEditing(nodeTags, nodes);
-        TagCollection nodeTagsToEdit = new TagCollection(nodeTags);
-        TagConflictResolutionUtil.completeTagCollectionForEditing(nodeTagsToEdit);
-
-        // launch a conflict resolution dialog, if necessary
-        //
-        CombinePrimitiveResolverDialog dialog = CombinePrimitiveResolverDialog.getInstance();
-        dialog.getTagConflictResolverModel().populate(nodeTagsToEdit, nodeTags.getKeysWithMultipleValues());
-        dialog.getRelationMemberConflictResolverModel().populate(relationToNodeReferences);
-        dialog.setTargetPrimitive(targetNode);
-        dialog.prepareDefaultDecisions();
-        // conflict resolution is necessary if there are conflicts in the merged tags
-        // or if at least one of the merged nodes is referred to by a relation
-        //
-        if (! nodeTags.isApplicableToPrimitive() || relationToNodeReferences.size() > 1) {
-            dialog.setVisible(true);
-            if (dialog.isCanceled())
+        try {
+            TagCollection nodeTags = TagCollection.unionOfAllPrimitives(nodes);
+            List<Command> resultion = CombinePrimitiveResolverDialog.launchIfNecessary(nodeTags, nodes, Collections.singleton(targetNode));
+            LinkedList<Command> cmds = new LinkedList<Command>();
+
+            // the nodes we will have to delete
+            //
+            Collection<Node> nodesToDelete = new HashSet<Node>(nodes);
+            nodesToDelete.remove(targetNode);
+
+            // fix the ways referring to at least one of the merged nodes
+            //
+            Collection<Way> waysToDelete = new HashSet<Way>();
+            List<Command> wayFixCommands = fixParentWays(
+                    nodesToDelete,
+                    targetNode);
+            if (wayFixCommands == null) {
                 return null;
-        }
-        LinkedList<Command> cmds = new LinkedList<Command>();
-
-        // the nodes we will have to delete
-        //
-        Collection<Node> nodesToDelete = new HashSet<Node>(nodes);
-        nodesToDelete.remove(targetNode);
-
-        // fix the ways referring to at least one of the merged nodes
-        //
-        Collection<Way> waysToDelete= new HashSet<Way>();
-        List<Command> wayFixCommands = fixParentWays(
-                nodesToDelete,
-                targetNode
-        );
-        if (wayFixCommands == null)
+            }
+            cmds.addAll(wayFixCommands);
+
+            // build the commands
+            //
+            if (targetNode != targetLocationNode) {
+                Node newTargetNode = new Node(targetNode);
+                newTargetNode.setCoor(targetLocationNode.getCoor());
+                cmds.add(new ChangeCommand(targetNode, newTargetNode));
+            }
+            cmds.addAll(resultion);
+            if (!nodesToDelete.isEmpty()) {
+                cmds.add(new DeleteCommand(nodesToDelete));
+            }
+            if (!waysToDelete.isEmpty()) {
+                cmds.add(new DeleteCommand(waysToDelete));
+            }
+            Command cmd = new SequenceCommand(tr("Merge {0} nodes", nodes.size()), cmds);
+            return cmd;
+        } catch (UserCancelException ex) {
             return null;
-        cmds.addAll(wayFixCommands);
-
-        // build the commands
-        //
-        if (targetNode != targetLocationNode) {
-            Node newTargetNode = new Node(targetNode);
-            newTargetNode.setCoor(targetLocationNode.getCoor());
-            cmds.add(new ChangeCommand(targetNode, newTargetNode));
-        }
-        cmds.addAll(dialog.buildResolutionCommands());
-        if (!nodesToDelete.isEmpty()) {
-            cmds.add(new DeleteCommand(nodesToDelete));
-        }
-        if (!waysToDelete.isEmpty()) {
-            cmds.add(new DeleteCommand(waysToDelete));
-        }
-        Command cmd = new SequenceCommand(tr("Merge {0} nodes", nodes.size()), cmds);
-        return cmd;
+        }
     }
 

trunk/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java

@@ -1135 +1135 @@
     }
 
+    /**
+     * Replies the set of referring relations
+     *
+     * @return the set of referring relations
+     */
+    public static Set<Relation> getParentRelations(Collection<? extends OsmPrimitive> primitives) {
+        HashSet<Relation> ret = new HashSet<Relation>();
+        for (OsmPrimitive w : primitives) {
+            ret.addAll(OsmPrimitive.getFilteredList(w.getReferrers(), Relation.class));
+        }
+        return ret;
+    }
 }

trunk/src/org/openstreetmap/josm/gui/DefaultNameFormatter.java

@@ -37 +37 @@
 import org.openstreetmap.josm.tools.I18n;
 import org.openstreetmap.josm.tools.TaggingPresetNameTemplateList;
+import org.openstreetmap.josm.tools.Utils;
+import org.openstreetmap.josm.tools.Utils.Function;
 
 /**
@@ -686 +688 @@
 
     public String formatAsHtmlUnorderedList(Collection<? extends OsmPrimitive> primitives) {
-        StringBuilder sb = new StringBuilder(1024);
-        sb.append("<ul>");
-        for (OsmPrimitive i : primitives) {
-            sb.append("<li>").append(i.getDisplayName(this)).append("</li>");
-        }
-        sb.append("</ul>");
-        return sb.toString();
+        return Utils.joinAsHtmlUnorderedList(Utils.transform(primitives, new Function<OsmPrimitive, String>() {
+
+            @Override
+            public String apply(OsmPrimitive x) {
+                return x.getDisplayName(DefaultNameFormatter.this);
+            }
+        }));
     }
 

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

@@ -4 +4 @@
 import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
 import static org.openstreetmap.josm.tools.I18n.tr;
+import static org.openstreetmap.josm.tools.I18n.trn;
 
 import java.awt.BorderLayout;
@@ -16 +17 @@
 import java.beans.PropertyChangeEvent;
 import java.beans.PropertyChangeListener;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.LinkedList;
@@ -30 +32 @@
 
 import org.openstreetmap.josm.Main;
+import org.openstreetmap.josm.actions.ExpertToggleAction;
 import org.openstreetmap.josm.command.ChangePropertyCommand;
 import org.openstreetmap.josm.command.Command;
+import org.openstreetmap.josm.corrector.UserCancelException;
+import org.openstreetmap.josm.data.osm.NameFormatter;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
@@ -37 +42 @@
 import org.openstreetmap.josm.data.osm.TagCollection;
 import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil;
 import org.openstreetmap.josm.gui.DefaultNameFormatter;
 import org.openstreetmap.josm.gui.SideButton;
@@ -42 +48 @@
 import org.openstreetmap.josm.gui.help.HelpUtil;
 import org.openstreetmap.josm.tools.ImageProvider;
+import org.openstreetmap.josm.tools.Utils;
+import org.openstreetmap.josm.tools.Utils.Function;
 import org.openstreetmap.josm.tools.WindowGeometry;
 
@@ -47 +55 @@
  * This dialog helps to resolve conflicts occurring when ways are combined or
  * nodes are merged.
+ *
+ * Usage: {@link #launchIfNecessary} followed by {@link #buildResolutionCommands}.
+ *
+ * Prior to {@link #launchIfNecessary}, the following usage sequence was needed:
  *
  * There is a singleton instance of this dialog which can be retrieved using
@@ -82 +94 @@
      *
      * @return the unique instance of the dialog
+     * @deprecated use {@link #launchIfNecessary} instead.
      */
+    @Deprecated
     public static CombinePrimitiveResolverDialog getInstance() {
         if (instance == null) {
@@ -421 +435 @@
         }
     }
+
+    public static List<Command> launchIfNecessary(
+            final TagCollection tagsOfPrimitives,
+            final Collection<? extends OsmPrimitive> primitives,
+            final Collection<? extends OsmPrimitive> targetPrimitives) throws UserCancelException {
+
+        final TagCollection completeWayTags = new TagCollection(tagsOfPrimitives);
+        TagConflictResolutionUtil.combineTigerTags(completeWayTags);
+        TagConflictResolutionUtil.normalizeTagCollectionBeforeEditing(completeWayTags, primitives);
+        final TagCollection tagsToEdit = new TagCollection(completeWayTags);
+        TagConflictResolutionUtil.completeTagCollectionForEditing(tagsToEdit);
+
+        final CombinePrimitiveResolverDialog dialog = CombinePrimitiveResolverDialog.getInstance();
+
+        dialog.getTagConflictResolverModel().populate(tagsToEdit, completeWayTags.getKeysWithMultipleValues());
+
+        final Set<Relation> parentRelations = OsmPrimitive.getParentRelations(primitives);
+        dialog.getRelationMemberConflictResolverModel().populate(parentRelations, primitives);
+        dialog.prepareDefaultDecisions();
+
+        // show information dialog to non-experts
+        if (!completeWayTags.isApplicableToPrimitive() && !ExpertToggleAction.isExpert()) {
+            String conflicts = Utils.joinAsHtmlUnorderedList(Utils.transform(completeWayTags.getKeysWithMultipleValues(), new Function<String, String>() {
+
+                @Override
+                public String apply(String key) {
+                    return tr("{0} ({1})", key, Utils.join(tr(", "), Utils.transform(completeWayTags.getValues(key), new Function<String, String>() {
+
+                        @Override
+                        public String apply(String x) {
+                            return x == null || x.isEmpty() ? tr("<i>missing</i>") : x;
+                        }
+                    })));
+                }
+            }));
+            String msg = tr("You are about to combine {0} objects, "
+                    + "but the following tags are used conflictingly:<br/>{1}"
+                    + "If these objects are combined, the resulting object may have unwanted tags.<br/>"
+                    + "If you want to continue, you are shown a dialog to fix the conflicting tags.<br/><br/>"
+                    + "Do you want to continue?",
+                    primitives.size(), conflicts);
+            if (!ConditionalOptionPaneUtil.showConfirmationDialog(
+                    "combine_tags",
+                    Main.parent,
+                    "<html>" + msg + "</html>",
+                    tr("Combine confirmation"),
+                    JOptionPane.YES_NO_OPTION,
+                    JOptionPane.QUESTION_MESSAGE,
+                    JOptionPane.YES_OPTION)) {
+                throw new UserCancelException();
+            }
+        }
+
+        if (!parentRelations.isEmpty() && !ExpertToggleAction.isExpert()) {
+            String msg = trn("You are about to combine {1} objects, "
+                    + "which are part of {0} relation:<br/>{2}"
+                    + "Combining these objects may break this relation. If you are unsure, please cancel this operation.<br/>"
+                    + "If you want to continue, you are shown a dialog to decide how to adapt the relation.<br/><br/>"
+                    + "Do you want to continue?",
+                    "You are about to combine {1} objects, "
+                    + "which are part of {0} relations:<br/>{2}"
+                    + "Combining these objects may break these relations. If you are unsure, please cancel this operation.<br/>"
+                    + "If you want to continue, you are shown a dialog to decide how to adapt the relations.<br/><br/>"
+                    + "Do you want to continue?",
+                    parentRelations.size(), parentRelations.size(), primitives.size(),
+                    DefaultNameFormatter.getInstance().formatAsHtmlUnorderedList(parentRelations));
+            if (!ConditionalOptionPaneUtil.showConfirmationDialog(
+                    "combine_tags",
+                    Main.parent,
+                    "<html>" + msg + "</html>",
+                    tr("Combine confirmation"),
+                    JOptionPane.YES_NO_OPTION,
+                    JOptionPane.QUESTION_MESSAGE,
+                    JOptionPane.YES_OPTION)) {
+                throw new UserCancelException();
+            }
+        }
+
+        // resolve tag conflicts if necessary
+        if (!completeWayTags.isApplicableToPrimitive() || !parentRelations.isEmpty()) {
+            dialog.setVisible(true);
+            if (dialog.isCanceled()) {
+                throw new UserCancelException();
+            }
+        }
+        List<Command> cmds = new LinkedList<Command>();
+        for (OsmPrimitive i : targetPrimitives) {
+            dialog.setTargetPrimitive(i);
+            cmds.addAll(dialog.buildResolutionCommands());
+        }
+        return cmds;
+    }
 }

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

@@ -28 +28 @@
     /** the model for the tag conflict resolver */
     private TagConflictResolverModel model;
-    /** selects wheter only tags with conflicts are displayed */
+    /** selects whether only tags with conflicts are displayed */
     private JCheckBox cbShowTagsWithConflictsOnly;
     private JCheckBox cbShowTagsWithMultiValuesOnly;
@@ -41 +41 @@
         gc.weightx = 1.0;
         gc.anchor = GridBagConstraints.LINE_START;
+        gc.gridwidth = 2;
         pnl.add(new JLabel(tr("<html>Please select the values to keep for the following tags.</html>")), gc);
 
+        gc.gridwidth = 1;
         gc.gridy = 1;
         gc.fill = GridBagConstraints.HORIZONTAL;

trunk/src/org/openstreetmap/josm/tools/Utils.java

@@ -23 +23 @@
 import java.util.Iterator;
 import java.util.List;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
 
 /**
@@ -166 +167 @@
     }
 
+    public static String joinAsHtmlUnorderedList(Collection<?> values) {
+        StringBuilder sb = new StringBuilder(1024);
+        sb.append("<ul>");
+        for (Object i : values) {
+            sb.append("<li>").append(i).append("</li>");
+        }
+        sb.append("</ul>");
+        return sb.toString();
+    }
+
     /**
      * convert Color to String
@@ -423 +434 @@
      * @return the transformed unmodifiable collection
      */
-    public static <A, B> Collection<B> transform(final Collection<A> c, final Function<A, B> f) {
+    public static <A, B> Collection<B> transform(final Collection<? extends A> c, final Function<A, B> f) {
         return new Collection<B>() {
 
@@ -460 +471 @@
                 return new Iterator<B>() {
 
-                    private Iterator<A> it = c.iterator();
+                    private Iterator<? extends A> it = c.iterator();
 
                     @Override