Changeset 17358 in josm for trunk/src/org/openstreetmap/josm/command

Timestamp:

2020-11-25T11:50:22+01:00 (4 years ago)

Author:

GerdP

Message:

see #19885: memory leak with "temporary" objects in validator and actions

• (hopefully) fix memory leaks in complex actions
• handle complex cases with presets and RelationEditor

I hope these changes don't break plugins which extend or overwrite RelationEditor

Location:

trunk/src/org/openstreetmap/josm/command

Files:

• ChangePropertyCommand.java (modified) (5 diffs)
• SplitWayCommand.java (modified) (8 diffs)

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

@@ -8 +8 @@
 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.Map.Entry;
 import java.util.NoSuchElementException;
 import java.util.Objects;
@@ -21 +23 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
+import org.openstreetmap.josm.data.osm.Tagged;
 import org.openstreetmap.josm.tools.I18n;
 import org.openstreetmap.josm.tools.ImageProvider;
@@ -73 +76 @@
      * @param ds The target data set. Must not be {@code null}
      * @param objects the objects to modify. Must not be empty
-     * @param tags the tags to set
+     * @param tags the tags to set. Caller must make sure that the tas are not changed once the command was executed.
      * @since 12726
      */
@@ -86 +89 @@
      *
      * @param objects the objects to modify. Must not be empty, and objects must belong to a data set
-     * @param tags the tags to set
+     * @param tags the tags to set. Caller must make sure that the tas are not changed once the command was executed.
      * @throws NullPointerException if objects is null or contain null item
      * @throws NoSuchElementException if objects is empty
@@ -287 +290 @@
                 Objects.equals(tags, that.tags);
     }
+
+    /**
+     * Calculate the {@link ChangePropertyCommand} that is needed to change the tags in source to be equal to those in target.
+     * @param source the source primitive
+     * @param target the target primitive
+     * @return null if no changes are needed, else a {@link ChangePropertyCommand}
+     * @since 17357
+     */
+    public static Command build(OsmPrimitive source, Tagged target) {
+        Map<String, String> changedTags = new HashMap<>();
+        // find tags which have to be changed or removed
+        for (Entry<String, String> tag : source.getKeys().entrySet()) {
+            String key = tag.getKey();
+            String val = target.get(key);
+            if (!tag.getValue().equals(val))
+                changedTags.put(key, val); // null or a different value
+        }
+        // find tags which exist only in target, they have to be added
+        for (Entry<String, String> tag : target.getKeys().entrySet()) {
+            String key = tag.getKey();
+            if (!source.hasTag(key))
+                changedTags.put(key, tag.getValue());
+        }
+        if (changedTags.isEmpty())
+            return null;
+        if (changedTags.size() == 1) {
+            Entry<String, String> tag = changedTags.entrySet().iterator().next();
+            return new ChangePropertyCommand(Collections.singleton(source), tag.getKey(), tag.getValue());
+        }
+        return new ChangePropertyCommand(Collections.singleton(source), new HashMap<>(changedTags));
+    }
 }

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

@@ -17 +17 @@
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -90 +91 @@
      * @param newSelection The new list of selected primitives ids (which is saved for later retrieval with {@link #getNewSelection})
      * @param originalWay The original way being split (which is saved for later retrieval with {@link #getOriginalWay})
-     * @param newWays The resulting new ways (which is saved for later retrieval with {@link #getOriginalWay})
+     * @param newWays The resulting new ways (which is saved for later retrieval with {@link #getNewWays})
      */
     public SplitWayCommand(String name, Collection<Command> commandList,
@@ -402 +403 @@
                 } catch (OsmTransferException e) {
                     ExceptionDialogUtil.explainException(e);
+                    analysis.cleanup();
                     return Optional.empty();
                 }
                 // If missing relation members were downloaded, perform the analysis again to find the relation
                 // member order for all relations.
+                analysis.cleanup();
                 analysis = analyseSplit(way, wayToKeep, newWays);
-                return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
+                break;
             case GO_AHEAD_WITHOUT_DOWNLOADS:
                 // Proceed with the split with the information we have.
                 // This can mean that there are no missing members we want, or that the user chooses to continue
                 // the split without downloading them.
-                return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
+                break;
             case USER_ABORTED:
             default:
                 return Optional.empty();
+        }
+        try {
+            return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
+        } finally {
+            // see #19885
+            wayToKeep.setNodes(null);
+            analysis.cleanup();
         }
     }
@@ -465 +475 @@
                     }
                     if (c == null) {
-                        c = new Relation(r);
+                        c = new Relation(r); // #19885: will be removed later
                     }
 
@@ -552 +562 @@
                 }
             }
-
-            if (c != null) {
-                commandList.add(new ChangeCommand(r.getDataSet(), r, c));
-            }
         }
         changedWay.setNodes(null); // see #19885
@@ -575 +581 @@
             warningTypes = warnings;
             this.numberOfRelations = numberOfRelations;
+        }
+
+        /**
+         * Unlink temporary copies of relations. See #19885
+         */
+        void cleanup() {
+            for (RelationAnalysis ra : relationAnalyses) {
+                if (ra.relation.getDataSet() == null)
+                    ra.relation.setMembers(null);
+            }
         }
 
@@ -688 +704 @@
         }
 
+        Set<Relation> modifiedRelations = new LinkedHashSet<>();
         // Perform the split.
         for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) {
@@ -729 +746 @@
                 }
             }
+            modifiedRelations.add(relation);
+        }
+        for (Relation r : modifiedRelations) {
+            DataSet ds = way.getDataSet();
+            Relation orig = (Relation) ds.getPrimitiveById(r);
+            analysis.getCommands().add(new ChangeMembersCommand(orig, new ArrayList<>(r.getMembers())));
+            r.setMembers(null); // see #19885
         }