Index: trunk/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java (revision 17358) @@ -26,5 +26,5 @@ import org.openstreetmap.josm.actions.relation.DownloadSelectedIncompleteMembersAction; import org.openstreetmap.josm.command.AddCommand; -import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.command.ChangePropertyCommand; import org.openstreetmap.josm.command.Command; @@ -116,4 +116,8 @@ // to avoid EDT violations SwingUtilities.invokeLater(() -> { + if (multipolygonRelation != null) { + // rather ugly: update generated a ChangeMembersCommand with a copy of the member list, so clear the list now + commandAndRelation.b.setMembers(null); // #see 19885 + } UndoRedoHandler.getInstance().add(command); final Relation relation = (Relation) MainApplication.getLayerManager().getEditDataSet() @@ -229,5 +233,6 @@ * @param selectedWays selected ways * @param selectedMultipolygonRelation selected multipolygon relation - * @return pair of old and new multipolygon relation if a difference was found, else the pair contains the old relation twice + * @return null if ways don't build a valid multipolygon, pair of old and new multipolygon relation if a difference was found, + * else the pair contains the old relation twice */ public static Pair updateMultipolygonRelation(Collection selectedWays, Relation selectedMultipolygonRelation) { @@ -244,4 +249,5 @@ } showErrors(mpTest.getErrors()); + calculated.setMembers(null); // see #19885 return null; //could not make multipolygon. } @@ -280,4 +286,5 @@ } foundDiff |= merged.addAll(calculated.getMembers()); + calculated.setMembers(null); // see #19885 if (!foundDiff) { return Pair.create(old, old); // unchanged @@ -343,5 +350,5 @@ } else { if (!unchanged) { - list.add(new ChangeCommand(existingRelation, relation)); + list.add(new ChangeMembersCommand(existingRelation, new ArrayList<>(relation.getMembers()))); } if (list.isEmpty()) { @@ -390,7 +397,8 @@ /** - * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary + * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary. * Function was extended in reltoolbox plugin by Zverikk and copied back to the core - * @param relation the multipolygon style relation to process + * @param relation the multipolygon style relation to process. If it is new, the tags might be + * modified, else the list of commands will contain a command to modify its tags * @return a list of commands to execute */ @@ -472,15 +480,16 @@ } - if (moveTags) { + values.remove("area"); + if (moveTags && !values.isEmpty()) { // add those tag values to the relation boolean fixed = false; - Relation r2 = new Relation(relation); + Map tagsToAdd = new HashMap<>(); for (Entry entry : values.entrySet()) { String key = entry.getKey(); - if (!r2.hasKey(key) && !"area".equals(key)) { + if (!relation.hasKey(key)) { if (relation.isNew()) relation.put(key, entry.getValue()); else - r2.put(key, entry.getValue()); + tagsToAdd.put(key, entry.getValue()); fixed = true; } @@ -491,7 +500,5 @@ ds = MainApplication.getLayerManager().getEditDataSet(); } - commands.add(new ChangeCommand(ds, relation, r2)); - } else { - r2.setMembers(null); // see #19885 + commands.add(new ChangePropertyCommand(ds, Collections.singleton(relation), tagsToAdd)); } } Index: trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java (revision 17358) @@ -26,6 +26,7 @@ import org.openstreetmap.josm.actions.ReverseWayAction.ReverseWayResult; import org.openstreetmap.josm.command.AddCommand; -import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.command.ChangeNodesCommand; +import org.openstreetmap.josm.command.ChangePropertyCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.command.DeleteCommand; @@ -1634,10 +1635,8 @@ } - Relation newRel = new Relation(r); - List members = newRel.getMembers(); + List members = r.getMembers(); members.remove(rm); - newRel.setMembers(members); - - cmds.add(new ChangeCommand(r, newRel)); + + cmds.add(new ChangeMembersCommand(r, members)); RelationRole saverel = new RelationRole(r, rm.getRole()); if (!result.contains(saverel)) { @@ -1674,11 +1673,9 @@ } // Add it back! - Relation newRel = new Relation(r.rel); - newRel.addMember(new RelationMember(r.role, outer)); - cmds.add(new ChangeCommand(r.rel, newRel)); - } - - Relation newRel; - RelationRole soleOuter; + List modifiedMembers = new ArrayList<>(r.rel.getMembers()); + modifiedMembers.add(new RelationMember(r.role, outer)); + cmds.add(new ChangeMembersCommand(r.rel, modifiedMembers)); + } + switch (multiouters.size()) { case 0: @@ -1686,12 +1683,12 @@ case 1: // Found only one to be part of a multipolygon relation, so just add it back as well - soleOuter = multiouters.get(0); - newRel = new Relation(soleOuter.rel); - newRel.addMember(new RelationMember(soleOuter.role, outer)); - cmds.add(new ChangeCommand(ds, soleOuter.rel, newRel)); + RelationRole soleOuter = multiouters.get(0); + List modifiedMembers = new ArrayList<>(soleOuter.rel.getMembers()); + modifiedMembers.add(new RelationMember(soleOuter.role, outer)); + cmds.add(new ChangeMembersCommand(ds, soleOuter.rel, modifiedMembers)); return; default: // Create a new relation with all previous members and (Way)outer as outer. - newRel = new Relation(); + Relation newRel = new Relation(); for (RelationRole r : multiouters) { // Add members @@ -1718,9 +1715,9 @@ */ private void stripTags(Collection ways) { - for (Way w : ways) { - final Way wayWithoutTags = new Way(w); - wayWithoutTags.removeAll(); - cmds.add(new ChangeCommand(w, wayWithoutTags)); - } + Map tagsToRemove = new HashMap<>(); + ways.stream().flatMap(w -> w.keySet().stream()).forEach(k -> tagsToRemove.put(k, null)); + if (tagsToRemove.isEmpty()) + return; + cmds.add(new ChangePropertyCommand(new ArrayList<>(ways), tagsToRemove)); /* I18N: current action printed in status display */ commitCommands(marktr("Remove tags from inner ways")); Index: trunk/src/org/openstreetmap/josm/actions/ReverseWayAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/ReverseWayAction.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/actions/ReverseWayAction.java (revision 17358) @@ -18,5 +18,5 @@ import org.openstreetmap.josm.actions.corrector.ReverseWayNoTagCorrector; import org.openstreetmap.josm.actions.corrector.ReverseWayTagCorrector; -import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangeNodesCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.command.SequenceCommand; @@ -41,5 +41,4 @@ */ public static class ReverseWayResult { - private final Way newWay; private final Collection tagCorrectionCommands; private final Command reverseCommand; @@ -47,20 +46,10 @@ /** * Create a new {@link ReverseWayResult} - * @param newWay The new way primitive * @param tagCorrectionCommands The commands to correct the tags * @param reverseCommand The command to reverse the way */ - public ReverseWayResult(Way newWay, Collection tagCorrectionCommands, Command reverseCommand) { - this.newWay = newWay; + public ReverseWayResult(Collection tagCorrectionCommands, Command reverseCommand) { this.tagCorrectionCommands = tagCorrectionCommands; this.reverseCommand = reverseCommand; - } - - /** - * Gets the new way object - * @return The new, reversed way - */ - public Way getNewWay() { - return newWay; } @@ -150,14 +139,12 @@ public static ReverseWayResult reverseWay(Way w) throws UserCancelException { ReverseWayNoTagCorrector.checkAndConfirmReverseWay(w); - Way wnew = new Way(w); - List nodesCopy = wnew.getNodes(); + List nodesCopy = w.getNodes(); Collections.reverse(nodesCopy); - wnew.setNodes(nodesCopy); Collection corrCmds = Collections.emptyList(); if (Config.getPref().getBoolean("tag-correction.reverse-way", true)) { - corrCmds = new ReverseWayTagCorrector().execute(w, wnew); + corrCmds = new ReverseWayTagCorrector().execute(w, w); } - return new ReverseWayResult(wnew, corrCmds, new ChangeCommand(w, wnew)); + return new ReverseWayResult(corrCmds, new ChangeNodesCommand(w, new ArrayList<>(nodesCopy))); } Index: trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java =================================================================== --- trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java (revision 17358) @@ -8,7 +8,9 @@ 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,4 +23,5 @@ 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,5 +76,5 @@ * @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,5 +89,5 @@ * * @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,3 +290,34 @@ 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 changedTags = new HashMap<>(); + // find tags which have to be changed or removed + for (Entry 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 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 tag = changedTags.entrySet().iterator().next(); + return new ChangePropertyCommand(Collections.singleton(source), tag.getKey(), tag.getValue()); + } + return new ChangePropertyCommand(Collections.singleton(source), new HashMap<>(changedTags)); + } } Index: trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java =================================================================== --- trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 17358) @@ -17,4 +17,5 @@ import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; @@ -90,5 +91,5 @@ * @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 commandList, @@ -402,18 +403,27 @@ } 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,5 +475,5 @@ } if (c == null) { - c = new Relation(r); + c = new Relation(r); // #19885: will be removed later } @@ -552,8 +562,4 @@ } } - - if (c != null) { - commandList.add(new ChangeCommand(r.getDataSet(), r, c)); - } } changedWay.setNodes(null); // see #19885 @@ -575,4 +581,14 @@ 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,4 +704,5 @@ } + Set modifiedRelations = new LinkedHashSet<>(); // Perform the split. for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) { @@ -729,4 +746,11 @@ } } + 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 } Index: trunk/src/org/openstreetmap/josm/data/osm/MultipolygonBuilder.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/MultipolygonBuilder.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/data/osm/MultipolygonBuilder.java (revision 17358) @@ -118,13 +118,17 @@ MultipolygonTest mpTest = new MultipolygonTest(); Relation calculated = mpTest.makeFromWays(ways); - if (!mpTest.getErrors().isEmpty()) { - return mpTest.getErrors().iterator().next().getMessage(); - } - Pair, List> outerInner = joinWays(calculated); - this.outerWays.clear(); - this.innerWays.clear(); - this.outerWays.addAll(outerInner.a); - this.innerWays.addAll(outerInner.b); - return null; + try { + if (!mpTest.getErrors().isEmpty()) { + return mpTest.getErrors().iterator().next().getMessage(); + } + Pair, List> outerInner = joinWays(calculated); + this.outerWays.clear(); + this.innerWays.clear(); + this.outerWays.addAll(outerInner.a); + this.innerWays.addAll(outerInner.b); + return null; + } finally { + calculated.setMembers(null); // see #19885 + } } Index: trunk/src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java (revision 17358) @@ -1154,10 +1154,9 @@ return; - Relation copy = new Relation(cur); + List members = cur.getMembers(); for (OsmPrimitive primitive: OsmDataManager.getInstance().getInProgressSelection()) { - copy.removeMembersFor(primitive); - } - UndoRedoHandler.getInstance().add(new ChangeMembersCommand(cur, copy.getMembers())); - copy.setMembers(null); // see #19885 + members.removeIf(rm -> rm.getMember() == primitive); + } + UndoRedoHandler.getInstance().add(new ChangeMembersCommand(cur, members)); tagTable.clearSelection(); Index: trunk/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java (revision 17358) @@ -49,5 +49,5 @@ import org.openstreetmap.josm.actions.JosmAction; -import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.data.UndoRedoHandler; @@ -939,25 +939,24 @@ final Collection presets = TaggingPresets.getMatchingPresets( EnumSet.of(TaggingPresetType.forPrimitive(orig)), orig.getKeys(), false); - Relation relation = new Relation(orig); + Relation target = new Relation(orig); boolean modified = false; for (OsmPrimitive p : primitivesToAdd) { if (p instanceof Relation) { - List loop = RelationChecker.checkAddMember(relation, (Relation) p); + List loop = RelationChecker.checkAddMember(target, (Relation) p); if (!loop.isEmpty() && loop.get(0).equals(loop.get(loop.size() - 1))) { warnOfCircularReferences(p, loop); continue; } - } else if (MemberTableModel.hasMembersReferringTo(relation.getMembers(), Collections.singleton(p)) + } else if (MemberTableModel.hasMembersReferringTo(target.getMembers(), Collections.singleton(p)) && !confirmAddingPrimitive(p)) { continue; } final Set roles = findSuggestedRoles(presets, p); - relation.addMember(new RelationMember(roles.size() == 1 ? roles.iterator().next() : "", p)); + target.addMember(new RelationMember(roles.size() == 1 ? roles.iterator().next() : "", p)); modified = true; } - if (!modified) { - relation.setMembers(null); // see #19885 - } - return modified ? new ChangeCommand(orig, relation) : null; + List members = new ArrayList<>(target.getMembers()); + target.setMembers(null); // see #19885 + return modified ? new ChangeMembersCommand(orig, members) : null; } catch (AddAbortException ign) { Logging.trace(ign); Index: trunk/src/org/openstreetmap/josm/gui/dialogs/relation/RelationEditor.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/dialogs/relation/RelationEditor.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/gui/dialogs/relation/RelationEditor.java (revision 17358) @@ -14,4 +14,5 @@ import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.tools.CheckParameterUtil; +import org.openstreetmap.josm.tools.Logging; /** @@ -71,5 +72,6 @@ * * @param layer the data layer the relation is a member of - * @param r the relation to be edited + * @param r the relation to be edited. If the relation doesn't belong to a {@code DataSet} + * callers MUST NOT use the relation after calling this method. * @param selectedMembers a collection of relation members which shall be selected when the editor is first launched * @return an instance of RelationEditor suitable for editing that kind of relation @@ -80,4 +82,12 @@ else { RelationEditor editor = new GenericRelationEditor(layer, r, selectedMembers); + if (r != null && r.getDataSet() == null) { + // see #19885: We have to assume that the relation was only created as container for tags and members + // the editor has created its own copy by now. + // Since the members point to the container we unlink them here. + Logging.debug("Member list is reset for relation {0}", r.getUniqueId()); + r.setMembers(null); + } + RelationDialogManager.getRelationDialogManager().positionOnScreen(editor); RelationDialogManager.getRelationDialogManager().register(layer, r, editor); Index: trunk/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java (revision 17357) +++ trunk/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java (revision 17358) @@ -5,4 +5,6 @@ import java.awt.Component; +import java.util.ArrayList; +import java.util.List; import javax.swing.JOptionPane; @@ -11,4 +13,7 @@ import org.openstreetmap.josm.command.AddCommand; import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangeMembersCommand; +import org.openstreetmap.josm.command.ChangePropertyCommand; +import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.command.conflict.ConflictAddCommand; import org.openstreetmap.josm.data.UndoRedoHandler; @@ -86,7 +91,15 @@ tagEditorModel.applyToPrimitive(editedRelation); getMemberTableModel().applyToRelation(editedRelation); - if (!editedRelation.hasEqualSemanticAttributes(originRelation, false)) { + List cmds = new ArrayList<>(); + if (originRelation.getKeys().equals(editedRelation.getKeys())) { + cmds.add(new ChangeMembersCommand(originRelation, editedRelation.getMembers())); + } + Command cmdProps = ChangePropertyCommand.build(originRelation, editedRelation); + if (cmdProps != null) + cmds.add(cmdProps); + if (cmds.size() >= 2) { UndoRedoHandler.getInstance().add(new ChangeCommand(originRelation, editedRelation)); - } else { + } else if (!cmds.isEmpty()) { + UndoRedoHandler.getInstance().add(cmds.get(0)); editedRelation.setMembers(null); // see #19885 }