Changeset 17358 in josm for trunk/src/org


Ignore:
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
Files:
10 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java

    r17343 r17358  
    2626import org.openstreetmap.josm.actions.relation.DownloadSelectedIncompleteMembersAction;
    2727import org.openstreetmap.josm.command.AddCommand;
    28 import org.openstreetmap.josm.command.ChangeCommand;
     28import org.openstreetmap.josm.command.ChangeMembersCommand;
    2929import org.openstreetmap.josm.command.ChangePropertyCommand;
    3030import org.openstreetmap.josm.command.Command;
     
    116116            // to avoid EDT violations
    117117            SwingUtilities.invokeLater(() -> {
     118                if (multipolygonRelation != null) {
     119                    // rather ugly: update generated a ChangeMembersCommand with a copy of the member list, so clear the list now
     120                    commandAndRelation.b.setMembers(null); // #see 19885
     121                }
    118122                UndoRedoHandler.getInstance().add(command);
    119123                final Relation relation = (Relation) MainApplication.getLayerManager().getEditDataSet()
     
    229233     * @param selectedWays selected ways
    230234     * @param selectedMultipolygonRelation selected multipolygon relation
    231      * @return pair of old and new multipolygon relation if a difference was found, else the pair contains the old relation twice
     235     * @return null if ways don't build a valid multipolygon, pair of old and new multipolygon relation if a difference was found,
     236     * else the pair contains the old relation twice
    232237     */
    233238    public static Pair<Relation, Relation> updateMultipolygonRelation(Collection<Way> selectedWays, Relation selectedMultipolygonRelation) {
     
    244249        }
    245250        showErrors(mpTest.getErrors());
     251        calculated.setMembers(null); // see #19885
    246252        return null; //could not make multipolygon.
    247253    }
     
    280286        }
    281287        foundDiff |= merged.addAll(calculated.getMembers());
     288        calculated.setMembers(null); // see #19885
    282289        if (!foundDiff) {
    283290            return Pair.create(old, old); // unchanged
     
    343350        } else {
    344351            if (!unchanged) {
    345                 list.add(new ChangeCommand(existingRelation, relation));
     352                list.add(new ChangeMembersCommand(existingRelation, new ArrayList<>(relation.getMembers())));
    346353            }
    347354            if (list.isEmpty()) {
     
    390397
    391398    /**
    392      * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary
     399     * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary.
    393400     * Function was extended in reltoolbox plugin by Zverikk and copied back to the core
    394      * @param relation the multipolygon style relation to process
     401     * @param relation the multipolygon style relation to process. If it is new, the tags might be
     402     * modified, else the list of commands will contain a command to modify its tags
    395403     * @return a list of commands to execute
    396404     */
     
    472480        }
    473481
    474         if (moveTags) {
     482        values.remove("area");
     483        if (moveTags && !values.isEmpty()) {
    475484            // add those tag values to the relation
    476485            boolean fixed = false;
    477             Relation r2 = new Relation(relation);
     486            Map<String, String> tagsToAdd = new HashMap<>();
    478487            for (Entry<String, String> entry : values.entrySet()) {
    479488                String key = entry.getKey();
    480                 if (!r2.hasKey(key) && !"area".equals(key)) {
     489                if (!relation.hasKey(key)) {
    481490                    if (relation.isNew())
    482491                        relation.put(key, entry.getValue());
    483492                    else
    484                         r2.put(key, entry.getValue());
     493                        tagsToAdd.put(key, entry.getValue());
    485494                    fixed = true;
    486495                }
     
    491500                    ds = MainApplication.getLayerManager().getEditDataSet();
    492501                }
    493                 commands.add(new ChangeCommand(ds, relation, r2));
    494             } else {
    495                 r2.setMembers(null); // see #19885
     502                commands.add(new ChangePropertyCommand(ds, Collections.singleton(relation), tagsToAdd));
    496503            }
    497504        }
  • trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java

    r17188 r17358  
    2626import org.openstreetmap.josm.actions.ReverseWayAction.ReverseWayResult;
    2727import org.openstreetmap.josm.command.AddCommand;
    28 import org.openstreetmap.josm.command.ChangeCommand;
     28import org.openstreetmap.josm.command.ChangeMembersCommand;
    2929import org.openstreetmap.josm.command.ChangeNodesCommand;
     30import org.openstreetmap.josm.command.ChangePropertyCommand;
    3031import org.openstreetmap.josm.command.Command;
    3132import org.openstreetmap.josm.command.DeleteCommand;
     
    16341635                }
    16351636
    1636                 Relation newRel = new Relation(r);
    1637                 List<RelationMember> members = newRel.getMembers();
     1637                List<RelationMember> members = r.getMembers();
    16381638                members.remove(rm);
    1639                 newRel.setMembers(members);
    1640 
    1641                 cmds.add(new ChangeCommand(r, newRel));
     1639
     1640                cmds.add(new ChangeMembersCommand(r, members));
    16421641                RelationRole saverel = new RelationRole(r, rm.getRole());
    16431642                if (!result.contains(saverel)) {
     
    16741673            }
    16751674            // Add it back!
    1676             Relation newRel = new Relation(r.rel);
    1677             newRel.addMember(new RelationMember(r.role, outer));
    1678             cmds.add(new ChangeCommand(r.rel, newRel));
    1679         }
    1680 
    1681         Relation newRel;
    1682         RelationRole soleOuter;
     1675            List<RelationMember> modifiedMembers = new ArrayList<>(r.rel.getMembers());
     1676            modifiedMembers.add(new RelationMember(r.role, outer));
     1677            cmds.add(new ChangeMembersCommand(r.rel, modifiedMembers));
     1678        }
     1679
    16831680        switch (multiouters.size()) {
    16841681        case 0:
     
    16861683        case 1:
    16871684            // Found only one to be part of a multipolygon relation, so just add it back as well
    1688             soleOuter = multiouters.get(0);
    1689             newRel = new Relation(soleOuter.rel);
    1690             newRel.addMember(new RelationMember(soleOuter.role, outer));
    1691             cmds.add(new ChangeCommand(ds, soleOuter.rel, newRel));
     1685            RelationRole soleOuter = multiouters.get(0);
     1686            List<RelationMember> modifiedMembers = new ArrayList<>(soleOuter.rel.getMembers());
     1687            modifiedMembers.add(new RelationMember(soleOuter.role, outer));
     1688            cmds.add(new ChangeMembersCommand(ds, soleOuter.rel, modifiedMembers));
    16921689            return;
    16931690        default:
    16941691            // Create a new relation with all previous members and (Way)outer as outer.
    1695             newRel = new Relation();
     1692            Relation newRel = new Relation();
    16961693            for (RelationRole r : multiouters) {
    16971694                // Add members
     
    17181715     */
    17191716    private void stripTags(Collection<Way> ways) {
    1720         for (Way w : ways) {
    1721             final Way wayWithoutTags = new Way(w);
    1722             wayWithoutTags.removeAll();
    1723             cmds.add(new ChangeCommand(w, wayWithoutTags));
    1724         }
     1717        Map<String, String> tagsToRemove = new HashMap<>();
     1718        ways.stream().flatMap(w -> w.keySet().stream()).forEach(k -> tagsToRemove.put(k, null));
     1719        if (tagsToRemove.isEmpty())
     1720            return;
     1721        cmds.add(new ChangePropertyCommand(new ArrayList<>(ways), tagsToRemove));
    17251722        /* I18N: current action printed in status display */
    17261723        commitCommands(marktr("Remove tags from inner ways"));
  • trunk/src/org/openstreetmap/josm/actions/ReverseWayAction.java

    r17333 r17358  
    1818import org.openstreetmap.josm.actions.corrector.ReverseWayNoTagCorrector;
    1919import org.openstreetmap.josm.actions.corrector.ReverseWayTagCorrector;
    20 import org.openstreetmap.josm.command.ChangeCommand;
     20import org.openstreetmap.josm.command.ChangeNodesCommand;
    2121import org.openstreetmap.josm.command.Command;
    2222import org.openstreetmap.josm.command.SequenceCommand;
     
    4141     */
    4242    public static class ReverseWayResult {
    43         private final Way newWay;
    4443        private final Collection<Command> tagCorrectionCommands;
    4544        private final Command reverseCommand;
     
    4746        /**
    4847         * Create a new {@link ReverseWayResult}
    49          * @param newWay The new way primitive
    5048         * @param tagCorrectionCommands The commands to correct the tags
    5149         * @param reverseCommand The command to reverse the way
    5250         */
    53         public ReverseWayResult(Way newWay, Collection<Command> tagCorrectionCommands, Command reverseCommand) {
    54             this.newWay = newWay;
     51        public ReverseWayResult(Collection<Command> tagCorrectionCommands, Command reverseCommand) {
    5552            this.tagCorrectionCommands = tagCorrectionCommands;
    5653            this.reverseCommand = reverseCommand;
    57         }
    58 
    59         /**
    60          * Gets the new way object
    61          * @return The new, reversed way
    62          */
    63         public Way getNewWay() {
    64             return newWay;
    6554        }
    6655
     
    150139    public static ReverseWayResult reverseWay(Way w) throws UserCancelException {
    151140        ReverseWayNoTagCorrector.checkAndConfirmReverseWay(w);
    152         Way wnew = new Way(w);
    153         List<Node> nodesCopy = wnew.getNodes();
     141        List<Node> nodesCopy = w.getNodes();
    154142        Collections.reverse(nodesCopy);
    155         wnew.setNodes(nodesCopy);
    156143
    157144        Collection<Command> corrCmds = Collections.<Command>emptyList();
    158145        if (Config.getPref().getBoolean("tag-correction.reverse-way", true)) {
    159             corrCmds = new ReverseWayTagCorrector().execute(w, wnew);
     146            corrCmds = new ReverseWayTagCorrector().execute(w, w);
    160147        }
    161         return new ReverseWayResult(wnew, corrCmds, new ChangeCommand(w, wnew));
     148        return new ReverseWayResult(corrCmds, new ChangeNodesCommand(w, new ArrayList<>(nodesCopy)));
    162149    }
    163150
  • trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java

    r16800 r17358  
    88import java.util.Collection;
    99import java.util.Collections;
     10import java.util.HashMap;
    1011import java.util.LinkedList;
    1112import java.util.List;
    1213import java.util.Map;
     14import java.util.Map.Entry;
    1315import java.util.NoSuchElementException;
    1416import java.util.Objects;
     
    2123import org.openstreetmap.josm.data.osm.OsmPrimitive;
    2224import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
     25import org.openstreetmap.josm.data.osm.Tagged;
    2326import org.openstreetmap.josm.tools.I18n;
    2427import org.openstreetmap.josm.tools.ImageProvider;
     
    7376     * @param ds The target data set. Must not be {@code null}
    7477     * @param objects the objects to modify. Must not be empty
    75      * @param tags the tags to set
     78     * @param tags the tags to set. Caller must make sure that the tas are not changed once the command was executed.
    7679     * @since 12726
    7780     */
     
    8689     *
    8790     * @param objects the objects to modify. Must not be empty, and objects must belong to a data set
    88      * @param tags the tags to set
     91     * @param tags the tags to set. Caller must make sure that the tas are not changed once the command was executed.
    8992     * @throws NullPointerException if objects is null or contain null item
    9093     * @throws NoSuchElementException if objects is empty
     
    287290                Objects.equals(tags, that.tags);
    288291    }
     292
     293    /**
     294     * Calculate the {@link ChangePropertyCommand} that is needed to change the tags in source to be equal to those in target.
     295     * @param source the source primitive
     296     * @param target the target primitive
     297     * @return null if no changes are needed, else a {@link ChangePropertyCommand}
     298     * @since 17357
     299     */
     300    public static Command build(OsmPrimitive source, Tagged target) {
     301        Map<String, String> changedTags = new HashMap<>();
     302        // find tags which have to be changed or removed
     303        for (Entry<String, String> tag : source.getKeys().entrySet()) {
     304            String key = tag.getKey();
     305            String val = target.get(key);
     306            if (!tag.getValue().equals(val))
     307                changedTags.put(key, val); // null or a different value
     308        }
     309        // find tags which exist only in target, they have to be added
     310        for (Entry<String, String> tag : target.getKeys().entrySet()) {
     311            String key = tag.getKey();
     312            if (!source.hasTag(key))
     313                changedTags.put(key, tag.getValue());
     314        }
     315        if (changedTags.isEmpty())
     316            return null;
     317        if (changedTags.size() == 1) {
     318            Entry<String, String> tag = changedTags.entrySet().iterator().next();
     319            return new ChangePropertyCommand(Collections.singleton(source), tag.getKey(), tag.getValue());
     320        }
     321        return new ChangePropertyCommand(Collections.singleton(source), new HashMap<>(changedTags));
     322    }
    289323}
  • trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java

    r17163 r17358  
    1717import java.util.HashSet;
    1818import java.util.Iterator;
     19import java.util.LinkedHashSet;
    1920import java.util.LinkedList;
    2021import java.util.List;
     
    9091     * @param newSelection The new list of selected primitives ids (which is saved for later retrieval with {@link #getNewSelection})
    9192     * @param originalWay The original way being split (which is saved for later retrieval with {@link #getOriginalWay})
    92      * @param newWays The resulting new ways (which is saved for later retrieval with {@link #getOriginalWay})
     93     * @param newWays The resulting new ways (which is saved for later retrieval with {@link #getNewWays})
    9394     */
    9495    public SplitWayCommand(String name, Collection<Command> commandList,
     
    402403                } catch (OsmTransferException e) {
    403404                    ExceptionDialogUtil.explainException(e);
     405                    analysis.cleanup();
    404406                    return Optional.empty();
    405407                }
    406408                // If missing relation members were downloaded, perform the analysis again to find the relation
    407409                // member order for all relations.
     410                analysis.cleanup();
    408411                analysis = analyseSplit(way, wayToKeep, newWays);
    409                 return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
     412                break;
    410413            case GO_AHEAD_WITHOUT_DOWNLOADS:
    411414                // Proceed with the split with the information we have.
    412415                // This can mean that there are no missing members we want, or that the user chooses to continue
    413416                // the split without downloading them.
    414                 return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
     417                break;
    415418            case USER_ABORTED:
    416419            default:
    417420                return Optional.empty();
     421        }
     422        try {
     423            return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
     424        } finally {
     425            // see #19885
     426            wayToKeep.setNodes(null);
     427            analysis.cleanup();
    418428        }
    419429    }
     
    465475                    }
    466476                    if (c == null) {
    467                         c = new Relation(r);
     477                        c = new Relation(r); // #19885: will be removed later
    468478                    }
    469479
     
    552562                }
    553563            }
    554 
    555             if (c != null) {
    556                 commandList.add(new ChangeCommand(r.getDataSet(), r, c));
    557             }
    558564        }
    559565        changedWay.setNodes(null); // see #19885
     
    575581            warningTypes = warnings;
    576582            this.numberOfRelations = numberOfRelations;
     583        }
     584
     585        /**
     586         * Unlink temporary copies of relations. See #19885
     587         */
     588        void cleanup() {
     589            for (RelationAnalysis ra : relationAnalyses) {
     590                if (ra.relation.getDataSet() == null)
     591                    ra.relation.setMembers(null);
     592            }
    577593        }
    578594
     
    688704        }
    689705
     706        Set<Relation> modifiedRelations = new LinkedHashSet<>();
    690707        // Perform the split.
    691708        for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) {
     
    729746                }
    730747            }
     748            modifiedRelations.add(relation);
     749        }
     750        for (Relation r : modifiedRelations) {
     751            DataSet ds = way.getDataSet();
     752            Relation orig = (Relation) ds.getPrimitiveById(r);
     753            analysis.getCommands().add(new ChangeMembersCommand(orig, new ArrayList<>(r.getMembers())));
     754            r.setMembers(null); // see #19885
    731755        }
    732756
  • trunk/src/org/openstreetmap/josm/data/osm/MultipolygonBuilder.java

    r15390 r17358  
    118118        MultipolygonTest mpTest = new MultipolygonTest();
    119119        Relation calculated = mpTest.makeFromWays(ways);
    120         if (!mpTest.getErrors().isEmpty()) {
    121             return mpTest.getErrors().iterator().next().getMessage();
    122         }
    123         Pair<List<JoinedPolygon>, List<JoinedPolygon>> outerInner = joinWays(calculated);
    124         this.outerWays.clear();
    125         this.innerWays.clear();
    126         this.outerWays.addAll(outerInner.a);
    127         this.innerWays.addAll(outerInner.b);
    128         return null;
     120        try {
     121            if (!mpTest.getErrors().isEmpty()) {
     122                return mpTest.getErrors().iterator().next().getMessage();
     123            }
     124            Pair<List<JoinedPolygon>, List<JoinedPolygon>> outerInner = joinWays(calculated);
     125            this.outerWays.clear();
     126            this.innerWays.clear();
     127            this.outerWays.addAll(outerInner.a);
     128            this.innerWays.addAll(outerInner.b);
     129            return null;
     130        } finally {
     131            calculated.setMembers(null); // see #19885
     132        }
    129133    }
    130134
  • trunk/src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java

    r17215 r17358  
    11541154                return;
    11551155
    1156             Relation copy = new Relation(cur);
     1156            List<RelationMember> members = cur.getMembers();
    11571157            for (OsmPrimitive primitive: OsmDataManager.getInstance().getInProgressSelection()) {
    1158                 copy.removeMembersFor(primitive);
    1159             }
    1160             UndoRedoHandler.getInstance().add(new ChangeMembersCommand(cur, copy.getMembers()));
    1161             copy.setMembers(null); // see #19885
     1158                members.removeIf(rm -> rm.getMember() == primitive);
     1159            }
     1160            UndoRedoHandler.getInstance().add(new ChangeMembersCommand(cur, members));
    11621161
    11631162            tagTable.clearSelection();
  • trunk/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java

    r17172 r17358  
    4949
    5050import org.openstreetmap.josm.actions.JosmAction;
    51 import org.openstreetmap.josm.command.ChangeCommand;
     51import org.openstreetmap.josm.command.ChangeMembersCommand;
    5252import org.openstreetmap.josm.command.Command;
    5353import org.openstreetmap.josm.data.UndoRedoHandler;
     
    939939            final Collection<TaggingPreset> presets = TaggingPresets.getMatchingPresets(
    940940                    EnumSet.of(TaggingPresetType.forPrimitive(orig)), orig.getKeys(), false);
    941             Relation relation = new Relation(orig);
     941            Relation target = new Relation(orig);
    942942            boolean modified = false;
    943943            for (OsmPrimitive p : primitivesToAdd) {
    944944                if (p instanceof Relation) {
    945                     List<Relation> loop = RelationChecker.checkAddMember(relation, (Relation) p);
     945                    List<Relation> loop = RelationChecker.checkAddMember(target, (Relation) p);
    946946                    if (!loop.isEmpty() && loop.get(0).equals(loop.get(loop.size() - 1))) {
    947947                        warnOfCircularReferences(p, loop);
    948948                        continue;
    949949                    }
    950                 } else if (MemberTableModel.hasMembersReferringTo(relation.getMembers(), Collections.singleton(p))
     950                } else if (MemberTableModel.hasMembersReferringTo(target.getMembers(), Collections.singleton(p))
    951951                        && !confirmAddingPrimitive(p)) {
    952952                    continue;
    953953                }
    954954                final Set<String> roles = findSuggestedRoles(presets, p);
    955                 relation.addMember(new RelationMember(roles.size() == 1 ? roles.iterator().next() : "", p));
     955                target.addMember(new RelationMember(roles.size() == 1 ? roles.iterator().next() : "", p));
    956956                modified = true;
    957957            }
    958             if (!modified) {
    959                 relation.setMembers(null); // see #19885
    960             }
    961             return modified ? new ChangeCommand(orig, relation) : null;
     958            List<RelationMember> members = new ArrayList<>(target.getMembers());
     959            target.setMembers(null); // see #19885
     960            return modified ? new ChangeMembersCommand(orig, members) : null;
    962961        } catch (AddAbortException ign) {
    963962            Logging.trace(ign);
  • trunk/src/org/openstreetmap/josm/gui/dialogs/relation/RelationEditor.java

    r17172 r17358  
    1414import org.openstreetmap.josm.gui.layer.OsmDataLayer;
    1515import org.openstreetmap.josm.tools.CheckParameterUtil;
     16import org.openstreetmap.josm.tools.Logging;
    1617
    1718/**
     
    7172     *
    7273     * @param layer the data layer the relation is a member of
    73      * @param r the relation to be edited
     74     * @param r the relation to be edited. If the relation doesn't belong to a {@code DataSet}
     75     * callers MUST NOT use the relation after calling this method.
    7476     * @param selectedMembers a collection of relation members which shall be selected when the editor is first launched
    7577     * @return an instance of RelationEditor suitable for editing that kind of relation
     
    8082        else {
    8183            RelationEditor editor = new GenericRelationEditor(layer, r, selectedMembers);
     84            if (r != null && r.getDataSet() == null) {
     85                // see #19885: We have to assume that the relation was only created as container for tags and members
     86                // the editor has created its own copy by now.
     87                // Since the members point to the container we unlink them here.
     88                Logging.debug("Member list is reset for relation  {0}", r.getUniqueId());
     89                r.setMembers(null);
     90            }
     91
    8292            RelationDialogManager.getRelationDialogManager().positionOnScreen(editor);
    8393            RelationDialogManager.getRelationDialogManager().register(layer, r, editor);
  • trunk/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java

    r17172 r17358  
    55
    66import java.awt.Component;
     7import java.util.ArrayList;
     8import java.util.List;
    79
    810import javax.swing.JOptionPane;
     
    1113import org.openstreetmap.josm.command.AddCommand;
    1214import org.openstreetmap.josm.command.ChangeCommand;
     15import org.openstreetmap.josm.command.ChangeMembersCommand;
     16import org.openstreetmap.josm.command.ChangePropertyCommand;
     17import org.openstreetmap.josm.command.Command;
    1318import org.openstreetmap.josm.command.conflict.ConflictAddCommand;
    1419import org.openstreetmap.josm.data.UndoRedoHandler;
     
    8691        tagEditorModel.applyToPrimitive(editedRelation);
    8792        getMemberTableModel().applyToRelation(editedRelation);
    88         if (!editedRelation.hasEqualSemanticAttributes(originRelation, false)) {
     93        List<Command> cmds = new ArrayList<>();
     94        if (originRelation.getKeys().equals(editedRelation.getKeys())) {
     95            cmds.add(new ChangeMembersCommand(originRelation, editedRelation.getMembers()));
     96        }
     97        Command cmdProps = ChangePropertyCommand.build(originRelation, editedRelation);
     98        if (cmdProps != null)
     99            cmds.add(cmdProps);
     100        if (cmds.size() >= 2) {
    89101            UndoRedoHandler.getInstance().add(new ChangeCommand(originRelation, editedRelation));
    90         } else {
     102        } else if (!cmds.isEmpty()) {
     103            UndoRedoHandler.getInstance().add(cmds.get(0));
    91104            editedRelation.setMembers(null); // see #19885
    92105        }
Note: See TracChangeset for help on using the changeset viewer.