Index: src/relcontext/RelContextDialog.java =================================================================== --- src/relcontext/RelContextDialog.java (revision 36216) +++ src/relcontext/RelContextDialog.java (working copy) @@ -17,8 +17,6 @@ import java.awt.event.MouseAdapter; 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; import java.io.InputStreamReader; @@ -50,8 +48,6 @@ import javax.swing.JTable; 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; import javax.swing.table.TableColumnModel; @@ -110,7 +106,7 @@ public static final String PREF_PREFIX = "reltoolbox"; private final DefaultTableModel relationsData; - private final ChosenRelation chosenRelation; + private final transient ChosenRelation chosenRelation; private final JPanel chosenRelationPanel; private final ChosenRelationPopupMenu popupMenu; private final MultipolygonSettingsPopup multiPopupMenu; @@ -412,6 +408,7 @@ @Override public void destroy() { + chosenRelation.removeChosenRelationListener(this); enterRoleAction.destroy(); findRelationAction.destroy(); createMultipolygonAction.destroy(); @@ -486,6 +483,7 @@ dlg.setVisible(true); Object answer = optionPane.getValue(); + dlg.dispose(); if (answer == null || answer == JOptionPane.UNINITIALIZED_VALUE || (answer instanceof Integer && (Integer) answer != JOptionPane.OK_OPTION)) return null; @@ -544,14 +542,11 @@ List commands = new ArrayList<>(); 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: src/relcontext/actions/AddRemoveMemberAction.java =================================================================== --- src/relcontext/actions/AddRemoveMemberAction.java (revision 36216) +++ src/relcontext/actions/AddRemoveMemberAction.java (working copy) @@ -9,7 +9,7 @@ import java.util.Collection; 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; import org.openstreetmap.josm.data.osm.DataSet; @@ -81,8 +81,9 @@ } 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 } /** @@ -95,7 +96,7 @@ 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(); Node lastNodeR = rw.lastNode(); @@ -132,11 +133,9 @@ } 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 { Collection toAdd = new ArrayList<>(ds.getSelected()); @@ -155,16 +154,13 @@ } } } - 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: src/relcontext/actions/ClearChosenRelationAction.java =================================================================== --- src/relcontext/actions/ClearChosenRelationAction.java (revision 36216) +++ src/relcontext/actions/ClearChosenRelationAction.java (working copy) @@ -15,7 +15,7 @@ import relcontext.ChosenRelationListener; public class ClearChosenRelationAction extends AbstractAction implements ChosenRelationListener { - private final ChosenRelation rel; + private final transient ChosenRelation rel; public ClearChosenRelationAction(ChosenRelation rel) { super(); Index: src/relcontext/actions/CreateMultipolygonAction.java =================================================================== --- src/relcontext/actions/CreateMultipolygonAction.java (revision 36216) +++ src/relcontext/actions/CreateMultipolygonAction.java (working copy) @@ -6,15 +6,16 @@ import java.awt.Dialog.ModalityType; 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; @@ -27,7 +28,6 @@ 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; import org.openstreetmap.josm.command.SequenceCommand; @@ -39,6 +39,7 @@ import org.openstreetmap.josm.data.osm.OsmPrimitiveType; 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; import org.openstreetmap.josm.spi.preferences.Config; @@ -55,7 +56,7 @@ */ public class CreateMultipolygonAction extends JosmAction { private static final String PREF_MULTIPOLY = "reltoolbox.multipolygon."; - protected ChosenRelation chRel; + protected transient ChosenRelation chRel; public CreateMultipolygonAction(ChosenRelation chRel) { super("Multi", "data/multipolygon", tr("Create a multipolygon from selected objects"), @@ -71,19 +72,19 @@ 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)); } @@ -96,7 +97,7 @@ List rels = null; if (getPref("allowsplit") || selectedWays.size() == 1) { if (SplittingMultipolygons.canProcess(selectedWays)) { - rels = SplittingMultipolygons.process(ds.getSelectedWays()); + rels = SplittingMultipolygons.process(selectedWays); } } else { if (TheRing.areAllOfThoseRings(selectedWays)) { @@ -200,9 +201,9 @@ private void addBoundaryMembers(Relation rel) { 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()) { if (n.hasKey("place")) { @@ -318,7 +319,7 @@ boolean isBoundary = getPref("boundary"); 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)) { conflictingKeys.add(key); @@ -336,7 +337,7 @@ values.remove(linearTag); } - if (values.containsKey("natural") && values.get("natural").equals("coastline")) { + if ("coastline".equals(values.get("natural"))) { values.remove("natural"); } @@ -355,9 +356,10 @@ List commands = new ArrayList<>(); 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) { if (way.hasKey(key) && (isBoundary || value.equals(way.get(key)))) { @@ -374,7 +376,7 @@ } } - if (affectedWays.size() > 0) { + if (!affectedWays.isEmpty()) { commands.add(new ChangePropertyCommand(affectedWays, key, null)); } } @@ -385,20 +387,22 @@ values.put("name", name); } 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)); } } @@ -443,29 +447,27 @@ final JDialog dlg = optionPane.createDialog(MainApplication.getMainFrame(), tr("Create a new relation")); 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); }); dlg.setVisible(true); 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); + 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 (new_name.length() > 0) { - rel.put("name", new_name); + if (!newName.isEmpty()) { + rel.put("name", newName); } return true; } Index: src/relcontext/actions/CreateRelationAction.java =================================================================== --- src/relcontext/actions/CreateRelationAction.java (revision 36216) +++ src/relcontext/actions/CreateRelationAction.java (working copy) @@ -126,6 +126,7 @@ dlg.setVisible(true); Object answer = optionPane.getValue(); + dlg.dispose(); if (answer == null || answer == JOptionPane.UNINITIALIZED_VALUE || (answer instanceof Integer && (Integer) answer != JOptionPane.OK_OPTION)) return null; Index: src/relcontext/actions/FindRelationAction.java =================================================================== --- src/relcontext/actions/FindRelationAction.java (revision 36216) +++ src/relcontext/actions/FindRelationAction.java (working copy) @@ -122,6 +122,8 @@ dlg.setVisible(true); Object answer = optionPane.getValue(); + dlg.dispose(); + if (answer == null || answer == JOptionPane.UNINITIALIZED_VALUE || (answer instanceof Integer && (Integer) answer != JOptionPane.OK_OPTION)) return; Index: src/relcontext/actions/ReconstructPolygonAction.java =================================================================== --- src/relcontext/actions/ReconstructPolygonAction.java (revision 36216) +++ src/relcontext/actions/ReconstructPolygonAction.java (working copy) @@ -7,6 +7,7 @@ import java.awt.event.KeyEvent; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -18,6 +19,7 @@ import org.openstreetmap.josm.actions.JosmAction; 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; import org.openstreetmap.josm.command.SequenceCommand; @@ -42,15 +44,19 @@ * @author Zverik */ 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); setEnabled(isSuitableRelation(rel.get())); @@ -71,7 +77,7 @@ } 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; } @@ -84,7 +90,6 @@ } rel.clear(); - List newSelection = new ArrayList<>(); List commands = new ArrayList<>(); Command relationDeleteCommand = DeleteCommand.delete(Collections.singleton(r), true, true); if (relationDeleteCommand == null) @@ -107,29 +112,25 @@ if (!myInnerWays.isEmpty()) { // this ring has inner rings, so we leave a multipolygon in // place and don't reconstruct the rings. + List members = new ArrayList<>(); Relation n; - 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)); + members.add(new RelationMember("outer", w)); } for (JoinedPolygon i : myInnerWays) { for (Way w : i.ways) { - n.addMember(new RelationMember("inner", w)); + members.add(new RelationMember("inner", w)); } } if (relationReused) { + n = new Relation(); + n.setKeys(r.getKeys()); + n.setMembers(members); commands.add(new AddCommand(ds, n)); } else { relationReused = true; - commands.add(new ChangeCommand(r, n)); + commands.add(new ChangeMembersCommand(r, members)); } - newSelection.add(n); continue; } @@ -185,7 +186,6 @@ result.setNodes(p.nodes); 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)); } @@ -194,9 +194,10 @@ // The relation needs to be deleted first, so that undo/redo continue to work properly 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: src/relcontext/actions/ReconstructRouteAction.java =================================================================== --- src/relcontext/actions/ReconstructRouteAction.java (revision 36216) +++ src/relcontext/actions/ReconstructRouteAction.java (working copy) @@ -12,8 +12,7 @@ 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; import org.openstreetmap.josm.data.osm.Node; @@ -34,8 +33,12 @@ * @author freeExec */ 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")); putValue(SMALL_ICON, ImageProvider.get("dialogs", "filter")); @@ -48,8 +51,7 @@ @Override 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<>(); Map> platformMembers = new LinkedHashMap<>(); @@ -117,42 +119,40 @@ nIndex != wayNodeEndIndex; 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); + } } } } for (RelationMember stop : stopMembers.values()) { - recRel.addMember(stop); + recMembers.add(stop); String stopName = PublicTransportHelper.getNameViaStoparea(stop); boolean existsPlatform = platformMembers.containsKey(stopName); if (!existsPlatform) { @@ -161,13 +161,13 @@ if (existsPlatform || platformMembers.containsKey(stopName)) { List lMember = platformMembers.get(stopName); if (lMember.size() == 1) { - recRel.addMember(lMember.get(0)); + recMembers.add(lMember.get(0)); lMember.remove(0); } else { // choose closest RelationMember candidat = getClosestPlatform(lMember, stop.getNode()); if (candidat != null) { - recRel.addMember(candidat); + recMembers.add(candidat); lMember.remove(candidat); } } @@ -179,22 +179,22 @@ 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); + recMembers.add(wtf); } - Command command = new ChangeCommand(r, recRel); - UndoRedoHandler.getInstance().add(command); + 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; RelationMember result = null; @@ -228,7 +228,7 @@ setEnabled(isSuitableRelation(newRelation)); } - private boolean isSuitableRelation(Relation newRelation) { + private static boolean isSuitableRelation(Relation newRelation) { return !(newRelation == null || !"route".equals(newRelation.get("type")) || newRelation.getMembersCount() == 0); } } Index: src/relcontext/actions/SplittingMultipolygons.java =================================================================== --- src/relcontext/actions/SplittingMultipolygons.java (revision 36216) +++ src/relcontext/actions/SplittingMultipolygons.java (working copy) @@ -11,6 +11,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Objects; import org.openstreetmap.josm.command.AddCommand; @@ -350,7 +351,7 @@ for (OsmPrimitive p : n.getReferrers()) { 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); } else { @@ -364,9 +365,9 @@ } 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: src/relcontext/actions/TheRing.java =================================================================== --- src/relcontext/actions/TheRing.java (revision 36216) +++ src/relcontext/actions/TheRing.java (working copy) @@ -14,6 +14,9 @@ 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; import org.openstreetmap.josm.data.osm.DataSet; @@ -329,20 +332,20 @@ */ 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", CreateMultipolygonAction.DEFAULT_LINEAR_TAGS); 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); } } @@ -353,15 +356,14 @@ if (p instanceof Relation) { 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); } } 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++) { if (rel.getMember(i).getMember().equals(source)) { @@ -383,7 +385,11 @@ if (w.equals(source)) { 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; } else { @@ -397,6 +403,7 @@ relation.addMember(new RelationMember("outer", w)); } } + sourceCopy.setNodes(null); // see #19885 if (!foundOwnWay) { final Command deleteCommand = DeleteCommand.delete(Collections.singleton(source)); if (deleteCommand != null) { @@ -412,7 +419,14 @@ 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: src/relcontext/relationfix/AssociatedStreetFixer.java =================================================================== --- src/relcontext/relationfix/AssociatedStreetFixer.java (revision 36216) +++ src/relcontext/relationfix/AssociatedStreetFixer.java (working copy) @@ -4,11 +4,13 @@ import static org.openstreetmap.josm.tools.I18n.tr; 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; import org.openstreetmap.josm.data.osm.DataSet; @@ -22,6 +24,11 @@ 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"); } @@ -29,15 +36,15 @@ @Override 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; } @@ -53,7 +60,7 @@ streetName = ""; } 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()) { setWarningMessage(tr("Relation has streets with different names")); @@ -71,35 +78,34 @@ // any way/point/relation with addr:housenumber=* or building=* or type=multipolygon -> house // 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)); } } } @@ -106,15 +112,13 @@ // 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); } } @@ -127,39 +131,23 @@ } } - 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: src/relcontext/relationfix/BoundaryFixer.java =================================================================== --- src/relcontext/relationfix/BoundaryFixer.java (revision 36216) +++ src/relcontext/relationfix/BoundaryFixer.java (working copy) @@ -3,7 +3,10 @@ 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; import org.openstreetmap.josm.data.osm.Node; @@ -36,15 +39,15 @@ @Override 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; } @@ -55,54 +58,51 @@ @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: src/relcontext/relationfix/MultipolygonFixer.java =================================================================== --- src/relcontext/relationfix/MultipolygonFixer.java (revision 36216) +++ src/relcontext/relationfix/MultipolygonFixer.java (working copy) @@ -4,11 +4,13 @@ import static org.openstreetmap.josm.tools.I18n.tr; 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; import org.openstreetmap.josm.data.osm.MultipolygonBuilder; @@ -28,7 +30,7 @@ super("multipolygon"); } - protected MultipolygonFixer(String...types) { + protected MultipolygonFixer(String... types) { super(types); } @@ -35,7 +37,7 @@ @Override 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; } @@ -46,26 +48,31 @@ @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; } /** - * 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) { outerWays.addAll(poly.ways); @@ -74,8 +81,9 @@ for (MultipolygonBuilder.JoinedPolygon poly : mpc.innerWays) { 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(); String role = null; @@ -85,11 +93,13 @@ role = "inner"; } 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: src/relcontext/relationfix/PublicTransportFixer.java =================================================================== --- src/relcontext/relationfix/PublicTransportFixer.java (revision 36216) +++ src/relcontext/relationfix/PublicTransportFixer.java (working copy) @@ -3,7 +3,10 @@ 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; import org.openstreetmap.josm.data.osm.OsmPrimitiveType; @@ -10,8 +13,8 @@ import org.openstreetmap.josm.data.osm.Relation; 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; /** @@ -25,19 +28,15 @@ super("route", "public_transport"); } - /*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)) { setWarningMessage(tr("Way platform without ''platform'' role found") + " r" + m.getUniqueId()); @@ -48,39 +47,34 @@ return true; } - /*@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; } }