Changeset 17981 in josm

Timestamp:

2021-07-10T20:52:12+02:00 (3 years ago)

Author:

Don-vip

Message:

fix #21041 - Tagging preset validation: clone properly primitives children for nominal execution of validation tests

Location:

trunk/src/org/openstreetmap/josm

Files:

• command/DeleteCommand.java (modified) (2 diffs)
• data/osm/DataSet.java (modified) (3 diffs)
• data/osm/DataSetMerger.java (modified) (3 diffs)
• data/osm/IPrimitive.java (modified) (1 diff)
• data/osm/Relation.java (modified) (1 diff)
• data/osm/Way.java (modified) (3 diffs)
• data/validation/tests/MapCSSTagCheckerAsserts.java (modified) (3 diffs)
• gui/tagging/presets/TaggingPresetValidation.java (modified) (4 diffs)

trunk/src/org/openstreetmap/josm/command/DeleteCommand.java

@@ -24 +24 @@
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.DefaultNameFormatter;
+import org.openstreetmap.josm.data.osm.IPrimitive;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
@@ -193 +194 @@
                     throw new IllegalArgumentException(osm + " is already deleted");
                 clonedPrimitives.put(osm, osm.save());
-
-                if (osm instanceof Way) {
-                    ((Way) osm).setNodes(null);
-                } else if (osm instanceof Relation) {
-                    ((Relation) osm).setMembers(null);
-                }
+                IPrimitive.resetPrimitiveChildren(osm);
             }
 

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

@@ -203 +203 @@
         copyFrom.getReadLock().lock();
         try {
-            Map<OsmPrimitive, OsmPrimitive> primMap = new HashMap<>();
-            for (Node n : copyFrom.getNodes()) {
-                Node newNode = new Node(n);
-                primMap.put(n, newNode);
-                addPrimitive(newNode);
-            }
-            for (Way w : copyFrom.getWays()) {
-                Way newWay = new Way(w, false, false);
-                primMap.put(w, newWay);
-                List<Node> newNodes = w.getNodes().stream()
-                        .map(n -> (Node) primMap.get(n))
-                        .collect(Collectors.toList());
-                newWay.setNodes(newNodes);
-                addPrimitive(newWay);
-            }
-            // Because relations can have other relations as members we first clone all relations
-            // and then get the cloned members
-            Collection<Relation> relations = copyFrom.getRelations();
-            for (Relation r : relations) {
-                Relation newRelation = new Relation(r, false, false);
-                primMap.put(r, newRelation);
-                addPrimitive(newRelation);
-            }
-            for (Relation r : relations) {
-                ((Relation) primMap.get(r)).setMembers(r.getMembers().stream()
-                        .map(rm -> new RelationMember(rm.getRole(), primMap.get(rm.getMember())))
-                        .collect(Collectors.toList()));
-            }
+            clonePrimitives(copyFrom.getNodes(), copyFrom.getWays(), copyFrom.getRelations());
             DataSourceAddedEvent addedEvent = new DataSourceAddedEvent(this,
                     new LinkedHashSet<>(dataSources), copyFrom.dataSources.stream());
@@ -261 +234 @@
 
     /**
+     * Clones the specified primitives into this data set.
+     * @param nodes nodes to clone
+     * @param ways ways to clone
+     * @param relations relations to clone
+     * @since 17981
+     */
+    public void clonePrimitives(Iterable<Node> nodes, Iterable<Way> ways, Iterable<Relation> relations) {
+        Map<OsmPrimitive, OsmPrimitive> primMap = new HashMap<>();
+        for (Node n : nodes) {
+            Node newNode = new Node(n);
+            primMap.put(n, newNode);
+            addPrimitive(newNode);
+        }
+        for (Way w : ways) {
+            Way newWay = new Way(w, false, false);
+            primMap.put(w, newWay);
+            List<Node> newNodes = w.getNodes().stream()
+                    .map(n -> (Node) primMap.get(n))
+                    .collect(Collectors.toList());
+            newWay.setNodes(newNodes);
+            addPrimitive(newWay);
+        }
+        // Because relations can have other relations as members we first clone all relations
+        // and then get the cloned members
+        for (Relation r : relations) {
+            Relation newRelation = new Relation(r, false, false);
+            primMap.put(r, newRelation);
+            addPrimitive(newRelation);
+        }
+        for (Relation r : relations) {
+            ((Relation) primMap.get(r)).setMembers(r.getMembers().stream()
+                    .map(rm -> new RelationMember(rm.getRole(), primMap.get(rm.getMember())))
+                    .collect(Collectors.toList()));
+        }
+    }
+
+    /**
      * Adds a new data source.
      * @param source data source to add
@@ -505 +515 @@
             firePrimitivesAdded(Collections.singletonList(primitive), false);
         });
+    }
+
+    /**
+     * Adds recursively a primitive, and all its children, to the dataset.
+     *
+     * @param primitive the primitive.
+     * @throws IllegalStateException if the dataset is read-only
+     * @since 17981
+     */
+    public void addPrimitiveRecursive(OsmPrimitive primitive) {
+        if (primitive instanceof Way) {
+            ((Way) primitive).getNodes().forEach(n -> addPrimitiveRecursive(n));
+        } else if (primitive instanceof Relation) {
+            ((Relation) primitive).getMembers().forEach(m -> addPrimitiveRecursive(m.getMember()));
+        }
+        addPrimitive(primitive);
     }
 

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

@@ -187 +187 @@
                 List<OsmPrimitive> referrers = target.getReferrers();
                 if (referrers.isEmpty()) {
-                    resetPrimitive(target);
+                    IPrimitive.resetPrimitiveChildren(target);
                     target.mergeFrom(source);
                     target.setDeleted(true);
@@ -212 +212 @@
             // This can be because of cross-referenced relations.
             for (OsmPrimitive osm: objectsToDelete) {
-                resetPrimitive(osm);
+                IPrimitive.resetPrimitiveChildren(osm);
             }
             for (OsmPrimitive osm: objectsToDelete) {
@@ -218 +218 @@
                 osm.mergeFrom(sourceDataSet.getPrimitiveById(osm.getPrimitiveId()));
             }
-        }
-    }
-
-    private static void resetPrimitive(OsmPrimitive osm) {
-        if (osm instanceof Way) {
-            ((Way) osm).setNodes(null);
-        } else if (osm instanceof Relation) {
-            ((Relation) osm).setMembers(null);
         }
     }

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

@@ -533 +533 @@
                 || getInterestingTags().equals(other.getInterestingTags());
     }
+
+    /**
+     * Resets primitive children, if applicable.
+     * @param p primitive
+     * @since 17981
+     */
+    static void resetPrimitiveChildren(IPrimitive p) {
+        if (p instanceof IWay<?>) {
+            ((IWay<?>) p).setNodes(null);
+        } else if (p instanceof IRelation<?>) {
+            ((IRelation<?>) p).setMembers(null);
+        }
+    }
 }

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

@@ -399 +399 @@
 
     /**
-     * Replies the set of  {@link OsmPrimitive}s referred to by at least one
-     * member of this relation
+     * Replies the set of {@link OsmPrimitive}s referred to by at least one member of this relation.
      *
-     * @return the set of  {@link OsmPrimitive}s referred to by at least one
-     * member of this relation
+     * @return the set of {@link OsmPrimitive}s referred to by at least one member of this relation
      * @see #getMemberPrimitivesList()
      */

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

@@ -197 +197 @@
     /**
      * Constructs a new {@code Way} from an existing {@code Way}.
-     * This  adds links from all way nodes to the clone. See #19885 for possible memory leaks.
+     * This adds links from all way nodes to the clone. See #19885 for possible memory leaks.
      * @param original The original {@code Way} to be identically cloned. Must not be null
      * @param clearMetadata If {@code true}, clears the OSM id and other metadata as defined by {@link #clearOsmMetadata}.
@@ -214 +214 @@
     /**
      * Constructs a new {@code Way} from an existing {@code Way}.
-     * This  adds links from all way nodes to the clone. See #19885 for possible memory leaks.
+     * This adds links from all way nodes to the clone. See #19885 for possible memory leaks.
      * @param original The original {@code Way} to be identically cloned. Must not be null
      * @param clearMetadata If {@code true}, clears the OSM id and other metadata as defined by {@link #clearOsmMetadata}.
@@ -226 +226 @@
     /**
      * Constructs a new {@code Way} from an existing {@code Way} (including its id).
-     * This  adds links from all way nodes to the clone. See #19885 for possible memory leaks.
+     * This adds links from all way nodes to the clone. See #19885 for possible memory leaks.
      * @param original The original {@code Way} to be identically cloned. Must not be null
      * @since 86

trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerAsserts.java

@@ -18 +18 @@
 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.Way;
 import org.openstreetmap.josm.data.validation.TestError;
 import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory;
@@ -63 +61 @@
             checksToRun.add(Collections.singleton(check));
             // Add primitive to dataset to avoid DataIntegrityProblemException when evaluating selectors
-            addPrimitive(ds, p);
+            ds.addPrimitiveRecursive(p);
             final Collection<TestError> pErrors = MapCSSTagChecker.getErrorsForPrimitive(p, true, checksToRun);
             Logging.debug("- Errors: {0}", pErrors);
@@ -88 +86 @@
         previousChecks.clear();
         previousChecks.trimToSize();
-    }
-
-    private static void addPrimitive(DataSet ds, OsmPrimitive p) {
-        if (p instanceof Way) {
-            ((Way) p).getNodes().forEach(n -> addPrimitive(ds, n));
-        } else if (p instanceof Relation) {
-            ((Relation) p).getMembers().forEach(m -> addPrimitive(ds, m.getMember()));
-        }
-        ds.addPrimitive(p);
     }
 

trunk/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetValidation.java

@@ -2 +2 @@
 package org.openstreetmap.josm.gui.tagging.presets;
 
+import static java.util.Collections.singleton;
+import static org.openstreetmap.josm.tools.I18n.tr;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+import javax.swing.JLabel;
+
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.data.osm.DataSet;
-import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.FilterModel;
+import org.openstreetmap.josm.data.osm.INode;
+import org.openstreetmap.josm.data.osm.IRelation;
+import org.openstreetmap.josm.data.osm.IWay;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
-import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.Tag;
-import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
 import org.openstreetmap.josm.data.validation.OsmValidator;
@@ -17 +28 @@
 import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.tools.Logging;
+import org.openstreetmap.josm.tools.SubclassFilteredCollection;
 import org.openstreetmap.josm.tools.Utils;
-
-import javax.swing.JLabel;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.List;
-
-import static org.openstreetmap.josm.tools.I18n.tr;
 
 /**
@@ -74 +78 @@
 
     static OsmPrimitive applyChangedTags(OsmPrimitive original, List<Tag> changedTags) {
-        OsmPrimitive primitive = clone(original);
-        new DataSet(primitive);
-        Command command = TaggingPreset.createCommand(Collections.singleton(primitive), changedTags);
+        DataSet ds = new DataSet();
+        Collection<OsmPrimitive> primitives = FilterModel.getAffectedPrimitives(singleton(original));
+        ds.clonePrimitives(
+                new SubclassFilteredCollection<>(primitives, INode.class::isInstance),
+                new SubclassFilteredCollection<>(primitives, IWay.class::isInstance),
+                new SubclassFilteredCollection<>(primitives, IRelation.class::isInstance));
+        OsmPrimitive primitive = ds.getPrimitiveById(original.getOsmPrimitiveId());
+        Command command = TaggingPreset.createCommand(singleton(primitive), changedTags);
         if (command != null) {
             command.executeCommand();
@@ -82 +91 @@
         return primitive;
     }
-
-    static OsmPrimitive clone(OsmPrimitive original) {
-        if (original instanceof Node) {
-            return new Node(((Node) original));
-        } else if (original instanceof Way) {
-            return new Way(((Way) original), false, false);
-        } else if (original instanceof Relation) {
-            return new Relation(((Relation) original), false, false);
-        } else {
-            throw new IllegalStateException();
-        }
-    }
 }