Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/RelContextDialog.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/RelContextDialog.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/RelContextDialog.java (revision 36217) @@ -18,6 +18,4 @@ import java.awt.event.MouseEvent; import java.awt.event.MouseListener; -import java.beans.PropertyChangeEvent; -import java.beans.PropertyChangeListener; import java.io.BufferedReader; import java.io.InputStream; @@ -51,6 +49,4 @@ import javax.swing.ListSelectionModel; import javax.swing.SwingUtilities; -import javax.swing.event.ListSelectionEvent; -import javax.swing.event.ListSelectionListener; import javax.swing.table.DefaultTableCellRenderer; import javax.swing.table.DefaultTableModel; @@ -111,5 +107,5 @@ private final DefaultTableModel relationsData; - private final ChosenRelation chosenRelation; + private final transient ChosenRelation chosenRelation; private final JPanel chosenRelationPanel; private final ChosenRelationPopupMenu popupMenu; @@ -413,4 +409,5 @@ @Override public void destroy() { + chosenRelation.removeChosenRelationListener(this); enterRoleAction.destroy(); findRelationAction.destroy(); @@ -487,4 +484,5 @@ Object answer = optionPane.getValue(); + dlg.dispose(); if (answer == null || answer == JOptionPane.UNINITIALIZED_VALUE || (answer instanceof Integer && (Integer) answer != JOptionPane.OK_OPTION)) @@ -545,12 +543,9 @@ for (int i = 0; i < r.getMembersCount(); i++) { RelationMember m = r.getMember(i); - if (selected.contains(m.getMember())) { - if (!role.equals(m.getRole())) { - commands.add(new ChangeRelationMemberRoleCommand(r, i, role)); - } + if (selected.contains(m.getMember()) && !role.equals(m.getRole())) { + commands.add(new ChangeRelationMemberRoleCommand(r, i, role)); } } if (!commands.isEmpty()) { - // UndoRedoHandler.getInstance().add(new ChangeCommand(chosenRelation.get(), r)); UndoRedoHandler.getInstance().add(new SequenceCommand(tr("Change relation member roles to {0}", role), commands)); } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/AddRemoveMemberAction.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/AddRemoveMemberAction.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/AddRemoveMemberAction.java (revision 36217) @@ -10,5 +10,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; @@ -82,6 +82,7 @@ if (!r.getMemberPrimitives().equals(rel.get().getMemberPrimitives())) { - UndoRedoHandler.getInstance().add(new ChangeCommand(rel.get(), r)); + UndoRedoHandler.getInstance().add(new ChangeMembersCommand(rel.get(), r.getMembers())); } + r.setMembers(null); // See #19885 } @@ -96,5 +97,5 @@ if (firstNode != null && !firstNode.equals(lastNode)) { for (int i = 0; i < r.getMembersCount(); i++) { - if (r.getMember(i).getType().equals(OsmPrimitiveType.WAY)) { + if (r.getMember(i).getType() == OsmPrimitiveType.WAY) { Way rw = (Way) r.getMember(i).getMember(); Node firstNodeR = rw.firstNode(); @@ -133,9 +134,7 @@ protected void updateIcon() { - // todo: change icon based on selection final int state; // 0=unknown, 1=add, 2=remove, 3=both DataSet ds = getLayerManager().getEditDataSet(); - if (ds == null || ds.getSelected() == null - || ds.getSelected().isEmpty() || rel == null || rel.get() == null) { + if (ds == null || ds.getSelected().isEmpty() || rel == null || rel.get() == null) { state = 0; } else { @@ -156,14 +155,11 @@ } } - GuiHelper.runInEDT(new Runnable() { - @Override - public void run() { - if (state == 0) { - putValue(LARGE_ICON_KEY, ImageProvider.get("relcontext", "addremove")); - } else { - String iconName = state == 1 ? "add" : state == 2 ? "remove" : "addremove"; - putValue(NAME, null); - putValue(LARGE_ICON_KEY, ImageProvider.get("relcontext", iconName)); - } + GuiHelper.runInEDT(() -> { + if (state == 0) { + putValue(LARGE_ICON_KEY, ImageProvider.get("relcontext", "addremove")); + } else { + String iconName = state == 1 ? "add" : state == 2 ? "remove" : "addremove"; + putValue(NAME, null); + putValue(LARGE_ICON_KEY, ImageProvider.get("relcontext", iconName)); } }); Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ClearChosenRelationAction.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ClearChosenRelationAction.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ClearChosenRelationAction.java (revision 36217) @@ -16,5 +16,5 @@ public class ClearChosenRelationAction extends AbstractAction implements ChosenRelationListener { - private final ChosenRelation rel; + private final transient ChosenRelation rel; public ClearChosenRelationAction(ChosenRelation rel) { Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/CreateMultipolygonAction.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/CreateMultipolygonAction.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/CreateMultipolygonAction.java (revision 36217) @@ -7,13 +7,14 @@ import java.awt.GridBagLayout; import java.awt.event.ActionEvent; -import java.awt.event.ActionListener; import java.awt.event.KeyEvent; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import java.util.TreeSet; @@ -28,5 +29,4 @@ import org.openstreetmap.josm.actions.JosmAction; import org.openstreetmap.josm.command.AddCommand; -import org.openstreetmap.josm.command.ChangeCommand; import org.openstreetmap.josm.command.ChangePropertyCommand; import org.openstreetmap.josm.command.Command; @@ -40,4 +40,5 @@ import org.openstreetmap.josm.data.osm.Relation; import org.openstreetmap.josm.data.osm.RelationMember; +import org.openstreetmap.josm.data.osm.TagMap; import org.openstreetmap.josm.data.osm.Way; import org.openstreetmap.josm.gui.MainApplication; @@ -56,5 +57,5 @@ public class CreateMultipolygonAction extends JosmAction { private static final String PREF_MULTIPOLY = "reltoolbox.multipolygon."; - protected ChosenRelation chRel; + protected transient ChosenRelation chRel; public CreateMultipolygonAction(ChosenRelation chRel) { @@ -72,17 +73,17 @@ public static boolean getDefaultPropertyValue(String property) { switch (property) { - case "boundary": - case "alltags": - case "allowsplit": - return false; - case "boundaryways": - case "tags": - case "single": - return true; + case "boundary": + case "alltags": + case "allowsplit": + return false; + case "boundaryways": + case "tags": + case "single": + return true; } throw new IllegalArgumentException(property); } - private boolean getPref(String property) { + private static boolean getPref(String property) { return Config.getPref().getBoolean(PREF_MULTIPOLY + property, getDefaultPropertyValue(property)); } @@ -97,5 +98,5 @@ if (getPref("allowsplit") || selectedWays.size() == 1) { if (SplittingMultipolygons.canProcess(selectedWays)) { - rels = SplittingMultipolygons.process(ds.getSelectedWays()); + rels = SplittingMultipolygons.process(selectedWays); } } else { @@ -201,7 +202,7 @@ for (OsmPrimitive p : getLayerManager().getEditDataSet().getSelected()) { String role = null; - if (p.getType().equals(OsmPrimitiveType.RELATION)) { + if (p.getType() == OsmPrimitiveType.RELATION) { role = "subarea"; - } else if (p.getType().equals(OsmPrimitiveType.NODE)) { + } else if (p.getType() == OsmPrimitiveType.NODE) { Node n = (Node) p; if (!n.isIncomplete()) { @@ -319,5 +320,5 @@ if (isBoundary || !getPref("alltags")) { for (RelationMember m : relation.getMembers()) { - if (m.hasRole() && m.getRole().equals("outer") && m.isWay()) { + if (m.hasRole() && "outer".equals(m.getRole()) && m.isWay()) { for (String key : values.keySet()) { if (!m.getWay().hasKey(key) && !relation.hasKey(key)) { @@ -337,5 +338,5 @@ } - if (values.containsKey("natural") && values.get("natural").equals("coastline")) { + if ("coastline".equals(values.get("natural"))) { values.remove("natural"); } @@ -356,7 +357,8 @@ boolean moveTags = getPref("tags"); - for (String key : values.keySet()) { + for (Entry entry : values.entrySet()) { + String key = entry.getKey(); List affectedWays = new ArrayList<>(); - String value = values.get(key); + String value = entry.getValue(); for (Way way : innerWays) { @@ -375,5 +377,5 @@ } - if (affectedWays.size() > 0) { + if (!affectedWays.isEmpty()) { commands.add(new ChangePropertyCommand(affectedWays, key, null)); } @@ -386,18 +388,20 @@ } boolean fixed = false; - Relation r2 = new Relation(relation); - for (String key : values.keySet()) { - if (!r2.hasKey(key) && !key.equals("area") - && (!isBoundary || key.equals("admin_level") || key.equals("name"))) { - if (relation.isNew()) { - relation.put(key, values.get(key)); + TagMap tags = relation.getKeys(); + for (Entry e: values.entrySet()) { + final String key = e.getKey(); + final String val = e.getValue(); + if (!tags.containsKey(key) && !"area".equals(key) + && (!isBoundary || "admin_level".equals(key) || "name".equals(key))) { + if (relation.getDataSet() == null) { + relation.put(key, val); } else { - r2.put(key, values.get(key)); + tags.put(key, val); } fixed = true; } } - if (fixed && !relation.isNew()) { - commands.add(new ChangeCommand(relation, r2)); + if (fixed && relation.getDataSet() != null) { + commands.add(new ChangePropertyCommand(Collections.singleton(relation), tags)); } } @@ -444,10 +448,7 @@ dlg.setModalityType(ModalityType.DOCUMENT_MODAL); - name.addActionListener(new ActionListener() { - @Override - public void actionPerformed(ActionEvent e) { - dlg.setVisible(false); - optionPane.setValue(JOptionPane.OK_OPTION); - } + name.addActionListener(e -> { + dlg.setVisible(false); + optionPane.setValue(JOptionPane.OK_OPTION); }); @@ -455,16 +456,17 @@ Object answer = optionPane.getValue(); + dlg.dispose(); if (answer == null || answer == JOptionPane.UNINITIALIZED_VALUE || (answer instanceof Integer && (Integer) answer != JOptionPane.OK_OPTION)) return false; - String admin_level = admin.getText().trim(); - String new_name = name.getText().trim(); - if (admin_level.equals("10") || (admin_level.length() == 1 && Character.isDigit(admin_level.charAt(0)))) { - rel.put("admin_level", admin_level); - Config.getPref().put(PREF_MULTIPOLY + "lastadmin", admin_level); - } - if (new_name.length() > 0) { - rel.put("name", new_name); + String adminLevel = admin.getText().trim(); + String newName = name.getText().trim(); + if ("10".equals(adminLevel) || (adminLevel.length() == 1 && Character.isDigit(adminLevel.charAt(0)))) { + rel.put("admin_level", adminLevel); + Config.getPref().put(PREF_MULTIPOLY + "lastadmin", adminLevel); + } + if (!newName.isEmpty()) { + rel.put("name", newName); } return true; Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/CreateRelationAction.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/CreateRelationAction.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/CreateRelationAction.java (revision 36217) @@ -127,4 +127,5 @@ Object answer = optionPane.getValue(); + dlg.dispose(); if (answer == null || answer == JOptionPane.UNINITIALIZED_VALUE || (answer instanceof Integer && (Integer) answer != JOptionPane.OK_OPTION)) Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/FindRelationAction.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/FindRelationAction.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/FindRelationAction.java (revision 36217) @@ -123,4 +123,6 @@ Object answer = optionPane.getValue(); + dlg.dispose(); + if (answer == null || answer == JOptionPane.UNINITIALIZED_VALUE || (answer instanceof Integer && (Integer) answer != JOptionPane.OK_OPTION)) Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ReconstructPolygonAction.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ReconstructPolygonAction.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ReconstructPolygonAction.java (revision 36217) @@ -8,4 +8,5 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -19,4 +20,5 @@ import org.openstreetmap.josm.command.AddCommand; import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.command.DeleteCommand; @@ -43,13 +45,17 @@ */ public class ReconstructPolygonAction extends JosmAction implements ChosenRelationListener { - private final ChosenRelation rel; + private final transient ChosenRelation rel; private static final List IRRELEVANT_KEYS = Arrays.asList("source", "created_by", "note"); + /** + * Reconstruct one or more polygons from multipolygon relation. + * @param rel the multipolygon relation + */ public ReconstructPolygonAction(ChosenRelation rel) { super(tr("Reconstruct polygon"), "dialogs/filter", tr("Reconstruct polygon from multipolygon relation"), Shortcut.registerShortcut("reltoolbox:reconstructpoly", tr("Relation Toolbox: {0}", tr("Reconstruct polygon from multipolygon relation")), - KeyEvent.CHAR_UNDEFINED, Shortcut.NONE), false); + KeyEvent.CHAR_UNDEFINED, Shortcut.NONE), false, false); this.rel = rel; rel.addChosenRelationListener(this); @@ -72,5 +78,5 @@ if (wont) { JOptionPane.showMessageDialog(MainApplication.getMainFrame(), - tr("Multipolygon must consist only of ways with one referring relation"), + tr("Multipolygon must consist only of ways"), tr("Reconstruct polygon"), JOptionPane.ERROR_MESSAGE); return; @@ -85,5 +91,4 @@ rel.clear(); - List newSelection = new ArrayList<>(); List commands = new ArrayList<>(); Command relationDeleteCommand = DeleteCommand.delete(Collections.singleton(r), true, true); @@ -108,27 +113,23 @@ // this ring has inner rings, so we leave a multipolygon in // place and don't reconstruct the rings. + List members = new ArrayList<>(); Relation n; + for (Way w : p.ways) { + members.add(new RelationMember("outer", w)); + } + for (JoinedPolygon i : myInnerWays) { + for (Way w : i.ways) { + members.add(new RelationMember("inner", w)); + } + } if (relationReused) { n = new Relation(); n.setKeys(r.getKeys()); - } else { - n = new Relation(r); - n.setMembers(null); - } - for (Way w : p.ways) { - n.addMember(new RelationMember("outer", w)); - } - for (JoinedPolygon i : myInnerWays) { - for (Way w : i.ways) { - n.addMember(new RelationMember("inner", w)); - } - } - if (relationReused) { + n.setMembers(members); commands.add(new AddCommand(ds, n)); } else { relationReused = true; - commands.add(new ChangeCommand(r, n)); - } - newSelection.add(n); + commands.add(new ChangeMembersCommand(r, members)); + } continue; } @@ -186,5 +187,4 @@ result.addNode(result.firstNode()); result.setKeys(tags); - newSelection.add(candidateWay == null ? result : candidateWay); commands.add(candidateWay == null ? new AddCommand(ds, result) : new ChangeCommand(candidateWay, result)); } @@ -195,7 +195,8 @@ commands.add(0, relationDeleteCommand); } - UndoRedoHandler.getInstance().add(new SequenceCommand(tr("Reconstruct polygons from relation {0}", r.getDisplayName(DefaultNameFormatter.getInstance())), commands)); + Collection newSelection = UndoRedoHandler.getInstance().getLastCommand().getParticipatingPrimitives(); + newSelection.removeIf(p -> p.isDeleted()); ds.setSelected(newSelection); } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ReconstructRouteAction.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ReconstructRouteAction.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ReconstructRouteAction.java (revision 36217) @@ -13,6 +13,5 @@ import javax.swing.AbstractAction; -import org.openstreetmap.josm.command.ChangeCommand; -import org.openstreetmap.josm.command.Command; +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.data.UndoRedoHandler; import org.openstreetmap.josm.data.coor.EastNorth; @@ -35,6 +34,10 @@ */ public class ReconstructRouteAction extends AbstractAction implements ChosenRelationListener { - private final ChosenRelation rel; - + private final transient ChosenRelation rel; + + /** + * Reconstruct route relation to scheme of public_transport. + * @param rel chosen relation + */ public ReconstructRouteAction(ChosenRelation rel) { super(tr("Reconstruct route")); @@ -49,6 +52,5 @@ public void actionPerformed(ActionEvent e) { Relation r = rel.get(); - Relation recRel = new Relation(r); - recRel.removeMembersFor(recRel.getMemberPrimitives()); + List recMembers = new ArrayList<>(); Map stopMembers = new LinkedHashMap<>(); @@ -118,33 +120,31 @@ nIndex += increment) { Node refNode = w.getNode(nIndex); - if (PublicTransportHelper.isNodeStop(refNode)) { - if (stopMembers.containsKey(refNode)) { - recRel.addMember(stopMembers.get(refNode)); - stopMembers.remove(refNode); - String stopName = PublicTransportHelper.getNameViaStoparea(refNode); - if (stopName == null) { - stopName = ""; + if (!(PublicTransportHelper.isNodeStop(refNode) && stopMembers.containsKey(refNode))) + continue; + recMembers.add(stopMembers.get(refNode)); + stopMembers.remove(refNode); + String stopName = PublicTransportHelper.getNameViaStoparea(refNode); + if (stopName == null) { + stopName = ""; + } + boolean existsPlatform = platformMembers.containsKey(stopName); + if (!existsPlatform) { + stopName = ""; // find of the nameless + } + if (existsPlatform || platformMembers.containsKey(stopName)) { + List lMember = platformMembers.get(stopName); + if (lMember.size() == 1) { + recMembers.add(lMember.get(0)); + lMember.remove(0); + } else { + // choose closest + RelationMember candidat = getClosestPlatform(lMember, refNode); + if (candidat != null) { + recMembers.add(candidat); + lMember.remove(candidat); } - boolean existsPlatform = platformMembers.containsKey(stopName); - if (!existsPlatform) { - stopName = ""; // find of the nameless - } - if (existsPlatform || platformMembers.containsKey(stopName)) { - List lMember = platformMembers.get(stopName); - if (lMember.size() == 1) { - recRel.addMember(lMember.get(0)); - lMember.remove(0); - } else { - // choose closest - RelationMember candidat = getClosestPlatform(lMember, refNode); - if (candidat != null) { - recRel.addMember(candidat); - lMember.remove(candidat); - } - } - if (lMember.isEmpty()) { - platformMembers.remove(stopName); - } - } + } + if (lMember.isEmpty()) { + platformMembers.remove(stopName); } } @@ -153,5 +153,5 @@ for (RelationMember stop : stopMembers.values()) { - recRel.addMember(stop); + recMembers.add(stop); String stopName = PublicTransportHelper.getNameViaStoparea(stop); boolean existsPlatform = platformMembers.containsKey(stopName); @@ -162,5 +162,5 @@ List lMember = platformMembers.get(stopName); if (lMember.size() == 1) { - recRel.addMember(lMember.get(0)); + recMembers.add(lMember.get(0)); lMember.remove(0); } else { @@ -168,5 +168,5 @@ RelationMember candidat = getClosestPlatform(lMember, stop.getNode()); if (candidat != null) { - recRel.addMember(candidat); + recMembers.add(candidat); lMember.remove(candidat); } @@ -180,20 +180,20 @@ for (List lPlatforms : platformMembers.values()) { for (RelationMember platform : lPlatforms) { - recRel.addMember(platform); + recMembers.add(platform); } } for (RelationMember route : routeMembers) { - recRel.addMember(route); + recMembers.add(route); } for (RelationMember wtf : wtfMembers) { - recRel.addMember(wtf); - } - Command command = new ChangeCommand(r, recRel); - UndoRedoHandler.getInstance().add(command); + recMembers.add(wtf); + } + UndoRedoHandler.getInstance().add(new ChangeMembersCommand(r, recMembers)); } private static final double maxSqrDistBetweenStopAndPlatform = 2000; // ~ 26m - private RelationMember getClosestPlatform(List members, Node stop) { + + private static RelationMember getClosestPlatform(List members, Node stop) { if (stop == null || members.isEmpty()) return null; double maxDist = maxSqrDistBetweenStopAndPlatform; @@ -229,5 +229,5 @@ } - private boolean isSuitableRelation(Relation newRelation) { + private static boolean isSuitableRelation(Relation newRelation) { return !(newRelation == null || !"route".equals(newRelation.get("type")) || newRelation.getMembersCount() == 0); } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/SplittingMultipolygons.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/SplittingMultipolygons.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/SplittingMultipolygons.java (revision 36217) @@ -12,4 +12,5 @@ import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Objects; @@ -351,5 +352,5 @@ if (p instanceof Way && !p.equals(ring)) { for (OsmPrimitive r : p.getReferrers()) { - if (r instanceof Relation && r.hasKey("type") && r.get("type").equals("multipolygon")) { + if (r instanceof Relation && "multipolygon".equals(r.get("type"))) { if (touchingWays.containsKey(p)) { touchingWays.put((Way) p, Boolean.TRUE); @@ -365,7 +366,7 @@ List otherWays = new ArrayList<>(); - for (Way w : touchingWays.keySet()) { - if (touchingWays.get(w)) { - otherWays.add(new TheRing(w)); + for (Entry e : touchingWays.entrySet()) { + if (Boolean.TRUE.equals(e.getValue())) { + otherWays.add(new TheRing(e.getKey())); } } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/TheRing.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/TheRing.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/TheRing.java (revision 36217) @@ -15,4 +15,7 @@ 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; @@ -330,4 +333,5 @@ public List getCommands(boolean createMultipolygon, Map relationChangeMap) { Way sourceCopy = new Way(source); + Map tagsToRemove = new HashMap<>(); if (createMultipolygon) { Collection linearTags = Config.getPref().getList(PREF_MULTIPOLY + "lineartags", @@ -335,13 +339,12 @@ relation = new Relation(); relation.put("type", "multipolygon"); - for (String key : sourceCopy.keySet()) { - if (linearTags.contains(key)) { + for (String key : source.keySet()) { + if (linearTags.contains(key) + || ("natural".equals(key) && "coastline".equals(source.get("natural")))) continue; - } - if ("natural".equals(key) && "coastline".equals(sourceCopy.get("natural"))) { - continue; - } - relation.put(key, sourceCopy.get(key)); + + relation.put(key, source.get(key)); sourceCopy.remove(key); + tagsToRemove.put(key, null); } } @@ -354,7 +357,6 @@ Relation rel; if (relationChangeMap != null) { - if (relationChangeMap.containsKey(p)) { - rel = relationChangeMap.get(p); - } else { + rel = relationChangeMap.get(p); + if (rel == null) { rel = new Relation((Relation) p); relationChangeMap.put((Relation) p, rel); @@ -362,5 +364,5 @@ } else { rel = new Relation((Relation) p); - relationCommands.add(new ChangeCommand(p, rel)); + relationCommands.add(new ChangeCommand(p, rel)); // should not happen } for (int i = 0; i < rel.getMembersCount(); i++) { @@ -384,5 +386,9 @@ if (createMultipolygon || !seg.getWayNodes().equals(source.getNodes())) { sourceCopy.setNodes(seg.getWayNodes()); - commands.add(new ChangeCommand(source, sourceCopy)); + if (!tagsToRemove.isEmpty()) { + commands.add(new ChangePropertyCommand(Collections.singleton(source), tagsToRemove)); + } + if (!sourceCopy.getNodes().equals(source.getNodes())) + commands.add(new ChangeNodesCommand(source, sourceCopy.getNodes())); } foundOwnWay = true; @@ -398,4 +404,5 @@ } } + sourceCopy.setNodes(null); // see #19885 if (!foundOwnWay) { final Command deleteCommand = DeleteCommand.delete(Collections.singleton(source)); @@ -413,5 +420,12 @@ public static void updateCommandsWithRelations(List commands, Map relationCache) { for (Map.Entry entry : relationCache.entrySet()) { - commands.add(new ChangeCommand(entry.getKey(), entry.getValue())); + Relation oldRel = entry.getKey(); + Relation newRel = entry.getValue(); + if (oldRel.getKeys().equals(newRel.getKeys())) { + commands.add(new ChangeMembersCommand(oldRel, newRel.getMembers())); + newRel.setMembers(null); // see #19885 + } else { + commands.add(new ChangeCommand(oldRel, newRel)); // should not happen + } } } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/AssociatedStreetFixer.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/AssociatedStreetFixer.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/AssociatedStreetFixer.java (revision 36217) @@ -5,9 +5,11 @@ import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -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.SequenceCommand; @@ -23,4 +25,9 @@ public class AssociatedStreetFixer extends RelationFixer { + private static final String ADDR_HOUSENUMBER = "addr:housenumber"; + private static final String BUILDING = "building"; + private static final String HOUSE = "house"; + private static final String STREET = "street"; + public AssociatedStreetFixer() { super("associatedStreet"); @@ -30,13 +37,13 @@ public boolean isRelationGood(Relation rel) { for (RelationMember m : rel.getMembers()) { - if (m.getType().equals(OsmPrimitiveType.NODE) && !"house".equals(m.getRole())) { + if (m.getType() == OsmPrimitiveType.NODE && !HOUSE.equals(m.getRole())) { setWarningMessage(tr("Node without ''house'' role found")); return false; } - if (m.getType().equals(OsmPrimitiveType.WAY) && !("house".equals(m.getRole()) || "street".equals(m.getRole()))) { + if (m.getType() == OsmPrimitiveType.WAY && !(HOUSE.equals(m.getRole()) || STREET.equals(m.getRole()))) { setWarningMessage(tr("Way without ''house'' or ''street'' role found")); return false; } - if (m.getType().equals(OsmPrimitiveType.RELATION) && !"house".equals(m.getRole())) { + if (m.getType() == OsmPrimitiveType.RELATION && !HOUSE.equals(m.getRole())) { setWarningMessage(tr("Relation without ''house'' role found")); return false; @@ -54,5 +61,5 @@ } for (RelationMember m : rel.getMembers()) { - if ("street".equals(m.getRole()) && !streetName.equals(m.getWay().get("name"))) { + if (STREET.equals(m.getRole()) && !streetName.equals(m.getWay().get("name"))) { String anotherName = m.getWay().get("name"); if (anotherName != null && !anotherName.isEmpty()) { @@ -72,33 +79,32 @@ // name - check which name is most used in street members and add to relation // copy this name to the other street members (???) - Relation rel = new Relation(source); + List members = source.getMembers(); boolean fixed = false; - for (int i = 0; i < rel.getMembersCount(); i++) { - RelationMember m = rel.getMember(i); - + for (int i = 0; i < members.size(); i++) { + RelationMember m = members.get(i); if (m.isNode()) { Node node = m.getNode(); - if (!"house".equals(m.getRole()) && - (node.hasKey("building") || node.hasKey("addr:housenumber"))) { + if (!HOUSE.equals(m.getRole()) && + (node.hasKey(BUILDING) || node.hasKey(ADDR_HOUSENUMBER))) { fixed = true; - rel.setMember(i, new RelationMember("house", node)); + members.set(i, new RelationMember(HOUSE, node)); } } else if (m.isWay()) { Way way = m.getWay(); - if (!"street".equals(m.getRole()) && way.hasKey("highway")) { + if (!STREET.equals(m.getRole()) && way.hasKey("highway")) { fixed = true; - rel.setMember(i, new RelationMember("street", way)); - } else if (!"house".equals(m.getRole()) && - (way.hasKey("building") || way.hasKey("addr:housenumber"))) { + members.set(i, new RelationMember(STREET, way)); + } else if (!HOUSE.equals(m.getRole()) && + (way.hasKey(BUILDING) || way.hasKey(ADDR_HOUSENUMBER))) { fixed = true; - rel.setMember(i, new RelationMember("house", way)); + members.set(i, new RelationMember(HOUSE, way)); } } else if (m.isRelation()) { Relation relation = m.getRelation(); - if (!"house".equals(m.getRole()) && - (relation.hasKey("building") || relation.hasKey("addr:housenumber") || "multipolygon".equals(relation.get("type")))) { + if (!HOUSE.equals(m.getRole()) && + (relation.hasKey(BUILDING) || relation.hasKey(ADDR_HOUSENUMBER) || "multipolygon".equals(relation.get("type")))) { fixed = true; - rel.setMember(i, new RelationMember("house", relation)); + members.set(i, new RelationMember(HOUSE, relation)); } } @@ -107,13 +113,11 @@ // fill relation name Map streetNames = new HashMap<>(); - for (RelationMember m : rel.getMembers()) { - if ("street".equals(m.getRole()) && m.isWay()) { + for (RelationMember m : members) { + if (STREET.equals(m.getRole()) && m.isWay()) { String name = m.getWay().get("name"); if (name == null || name.isEmpty()) { continue; } - Integer count = streetNames.get(name); - streetNames.put(name, count != null ? count + 1 : 1); } @@ -128,37 +132,21 @@ } - if (!rel.hasKey("name") && !commonName.isEmpty()) { - fixed = true; - rel.put("name", commonName); - } else { - commonName = ""; // set empty common name - if we already have name on relation, do not overwrite it + Map nameTag = new HashMap<>(); + if (!source.hasKey("name") && !commonName.isEmpty()) { + nameTag.put("name", commonName); } List commandList = new ArrayList<>(); + final DataSet ds = Utils.firstNonNull(source.getDataSet(), MainApplication.getLayerManager().getEditDataSet()); if (fixed) { - final DataSet ds = Utils.firstNonNull(source.getDataSet(), MainApplication.getLayerManager().getEditDataSet()); - commandList.add(new ChangeCommand(ds, source, rel)); + commandList.add(new ChangeMembersCommand(ds, source, members)); + } + if (!nameTag.isEmpty()) { + commandList.add(new ChangePropertyCommand(ds, Collections.singleton(source), nameTag)); } - /*if (!commonName.isEmpty()) - // fill common name to streets - for (RelationMember m : rel.getMembers()) - if ("street".equals(m.getRole()) && m.isWay()) { - String name = m.getWay().get("name"); - if (commonName.equals(name)) continue; - - // TODO: ask user if he really wants to overwrite street name?? - - Way oldWay = m.getWay(); - Way newWay = new Way(oldWay); - newWay.put("name", commonName); - - commandList.add(new ChangeCommand(MainApplication.getLayerManager().getEditDataSet(), oldWay, newWay)); - } - */ // return results - if (commandList.isEmpty()) { + if (commandList.isEmpty()) return null; - } return SequenceCommand.wrapIfNeeded(tr("fix associatedStreet relation"), commandList); } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/BoundaryFixer.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/BoundaryFixer.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/BoundaryFixer.java (revision 36217) @@ -4,5 +4,8 @@ import static org.openstreetmap.josm.tools.I18n.tr; -import org.openstreetmap.josm.command.ChangeCommand; +import java.util.ArrayList; +import java.util.List; + +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.data.osm.DataSet; @@ -37,13 +40,13 @@ public boolean isRelationGood(Relation rel) { for (RelationMember m : rel.getMembers()) { - if (m.getType().equals(OsmPrimitiveType.RELATION) && !"subarea".equals(m.getRole())) { + if (m.getType() == OsmPrimitiveType.RELATION && !"subarea".equals(m.getRole())) { setWarningMessage(tr("Relation without ''subarea'' role found")); return false; } - if (m.getType().equals(OsmPrimitiveType.NODE) && !("label".equals(m.getRole()) || "admin_centre".equals(m.getRole()))) { + if (m.getType() == OsmPrimitiveType.NODE && !("label".equals(m.getRole()) || "admin_centre".equals(m.getRole()))) { setWarningMessage(tr("Node without ''label'' or ''admin_centre'' role found")); return false; } - if (m.getType().equals(OsmPrimitiveType.WAY) && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) { + if (m.getType() == OsmPrimitiveType.WAY && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) { setWarningMessage(tr("Way without ''inner'' or ''outer'' role found")); return false; @@ -56,53 +59,50 @@ @Override public Command fixRelation(Relation rel) { - Relation r = rel; - Relation rr = fixMultipolygonRoles(r); - boolean fixed = false; - if (rr != null) { - fixed = true; - r = rr; + List members = fixMultipolygonRoles(rel.getMembers()); + if (members.isEmpty()) { + members = rel.getMembers(); } - rr = fixBoundaryRoles(r); - if (rr != null) { - fixed = true; - r = rr; - } - if (fixed) { + members = fixBoundaryRoles(members); + if (!members.equals(rel.getMembers())) { final DataSet ds = Utils.firstNonNull(rel.getDataSet(), MainApplication.getLayerManager().getEditDataSet()); - return new ChangeCommand(ds, rel, r); + return new ChangeMembersCommand(ds, rel, members); } return null; } - private Relation fixBoundaryRoles(Relation source) { - Relation r = new Relation(source); - boolean fixed = false; - for (int i = 0; i < r.getMembersCount(); i++) { - RelationMember m = r.getMember(i); + /** + * Possibly change roles of non-way members. + * @param origMembers original list of relation members + * @return either the original and unmodified list or a new one with at least one new item + */ + private static List fixBoundaryRoles(List origMembers) { + List members = origMembers; + for (int i = 0; i < members.size(); i++) { + RelationMember m = members.get(i); String role = null; if (m.isRelation()) { role = "subarea"; - } else if (m.isNode()) { + } else if (m.isNode() && !m.getMember().isIncomplete()) { Node n = (Node) m.getMember(); - if (!n.isIncomplete()) { - if (n.hasKey("place")) { - String place = n.get("place"); - if (place.equals("state") || place.equals("country") || - place.equals("county") || place.equals("region")) { - role = "label"; - } else { - role = "admin_centre"; - } + if (n.hasKey("place")) { + String place = n.get("place"); + if ("state".equals(place) || "country".equals(place) || + "county".equals(place) || "region".equals(place)) { + role = "label"; } else { - role = "label"; + role = "admin_centre"; } + } else { + role = "label"; } } if (role != null && !role.equals(m.getRole())) { - r.setMember(i, new RelationMember(role, m.getMember())); - fixed = true; + if (members == origMembers) { + members = new ArrayList<>(origMembers); // don't modify original list + } + members.set(i, new RelationMember(role, m.getMember())); } } - return fixed ? r : null; + return members; } } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/MultipolygonFixer.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/MultipolygonFixer.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/MultipolygonFixer.java (revision 36217) @@ -5,9 +5,11 @@ import java.util.ArrayList; -import java.util.Collection; +import java.util.Collections; import java.util.HashSet; +import java.util.List; import java.util.Set; +import java.util.stream.Collectors; -import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.data.osm.DataSet; @@ -29,5 +31,5 @@ } - protected MultipolygonFixer(String...types) { + protected MultipolygonFixer(String... types) { super(types); } @@ -36,5 +38,5 @@ public boolean isRelationGood(Relation rel) { for (RelationMember m : rel.getMembers()) { - if (m.getType().equals(OsmPrimitiveType.WAY) && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) { + if (m.getType() == OsmPrimitiveType.WAY && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) { setWarningMessage(tr("Way without ''inner'' or ''outer'' role found")); return false; @@ -47,8 +49,8 @@ @Override public Command fixRelation(Relation rel) { - Relation rr = fixMultipolygonRoles(rel); - if (rr != null) { + List members = fixMultipolygonRoles(rel.getMembers()); + if (!members.equals(rel.getMembers())) { final DataSet ds = Utils.firstNonNull(rel.getDataSet(), MainApplication.getLayerManager().getEditDataSet()); - return new ChangeCommand(ds, rel, rr); + return new ChangeMembersCommand(ds, rel, members); } return null; @@ -56,15 +58,20 @@ /** - * Basically, created multipolygon from scratch, and if successful, replace roles with new ones. + * Basically, create member list of a multipolygon from scratch, and if + * successful, replace roles with new ones. + * + * @param origMembers original list of relation members + * @return either the original and unmodified list or a new one with at least + * one new item or the empty list if the way members don't build a + * correct multipolygon */ - protected Relation fixMultipolygonRoles(Relation source) { - Collection ways = new ArrayList<>(source.getMemberPrimitives(Way.class)); + protected List fixMultipolygonRoles(final List origMembers) { + List ways = origMembers.stream().filter(m -> m.isWay()).map(m -> m.getWay()).collect(Collectors.toList()); + MultipolygonBuilder mpc = new MultipolygonBuilder(); String error = mpc.makeFromWays(ways); if (error != null) - return null; + return Collections.emptyList(); - Relation r = new Relation(source); - boolean fixed = false; Set outerWays = new HashSet<>(); for (MultipolygonBuilder.JoinedPolygon poly : mpc.outerWays) { @@ -75,6 +82,7 @@ innerWays.addAll(poly.ways); } - for (int i = 0; i < r.getMembersCount(); i++) { - RelationMember m = r.getMember(i); + List members = origMembers; + for (int i = 0; i < members.size(); i++) { + RelationMember m = members.get(i); if (m.isWay()) { final Way way = m.getWay(); @@ -86,10 +94,12 @@ } if (role != null && !role.equals(m.getRole())) { - r.setMember(i, new RelationMember(role, way)); - fixed = true; + if (members == origMembers) { + members = new ArrayList<>(origMembers); // don't modify original list + } + members.set(i, new RelationMember(role, way)); } } } - return fixed ? r : null; + return members; } } Index: applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/PublicTransportFixer.java =================================================================== --- applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/PublicTransportFixer.java (revision 36216) +++ applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/PublicTransportFixer.java (revision 36217) @@ -4,5 +4,8 @@ import static org.openstreetmap.josm.tools.I18n.tr; -import org.openstreetmap.josm.command.ChangeCommand; +import java.util.ArrayList; +import java.util.List; + +import org.openstreetmap.josm.command.ChangeMembersCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.data.osm.DataSet; @@ -11,6 +14,6 @@ import org.openstreetmap.josm.data.osm.RelationMember; import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.tools.Utils; -import org.openstreetmap.josm.tools.Utils; import relcontext.actions.PublicTransportHelper; @@ -26,17 +29,13 @@ } - /*protected PublicTransportFixer(String...types) { - super(types); - }*/ - @Override public boolean isRelationGood(Relation rel) { for (RelationMember m : rel.getMembers()) { - if (m.getType().equals(OsmPrimitiveType.NODE) + if (m.getType() == OsmPrimitiveType.NODE && !(m.getRole().startsWith(PublicTransportHelper.STOP) || m.getRole().startsWith(PublicTransportHelper.PLATFORM))) { setWarningMessage(tr("Node without ''stop'' or ''platform'' role found")); return false; } - if (m.getType().equals(OsmPrimitiveType.WAY) + if (m.getType() == OsmPrimitiveType.WAY && PublicTransportHelper.isWayPlatform(m) && !m.getRole().startsWith(PublicTransportHelper.PLATFORM)) { @@ -49,38 +48,33 @@ } - /*@Override - public boolean isFixerApplicable(Relation rel) { - return true; - }*/ - @Override public Command fixRelation(Relation rel) { - Relation r = rel; - Relation rr = fixStopPlatformRole(r); - boolean fixed = false; - if (rr != null) { - fixed = true; - r = rr; - } - if (fixed) { + List members = fixStopPlatformRole(rel.getMembers()); + if (!members.equals(rel.getMembers())) { final DataSet ds = Utils.firstNonNull(rel.getDataSet(), MainApplication.getLayerManager().getEditDataSet()); - return new ChangeCommand(ds, rel, r); + return new ChangeMembersCommand(ds, rel, rel.getMembers()); } return null; } - private Relation fixStopPlatformRole(Relation source) { - Relation r = new Relation(source); - boolean fixed = false; - for (int i = 0; i < r.getMembersCount(); i++) { - RelationMember m = r.getMember(i); + /** + * Fix roles of members. + * @param origMembers original list of relation members + * @return either the original and unmodified list or a new one with at least one new item + */ + private static List fixStopPlatformRole(List origMembers) { + List members = origMembers; + for (int i = 0; i < members.size(); i++) { + RelationMember m = members.get(i); String role = PublicTransportHelper.getRoleByMember(m); if (role != null && !m.getRole().startsWith(role)) { - r.setMember(i, new RelationMember(role, m.getMember())); - fixed = true; + if (members == origMembers) { + members = new ArrayList<>(origMembers); + } + members.set(i, new RelationMember(role, m.getMember())); } } - return fixed ? r : null; + return members; } }