Ignore:
Timestamp:
2024-03-03T10:25:25+01:00 (13 months ago)
Author:
GerdP
Message:

fix #23521: fix some memory leaks

  • dispose dialogs
  • either avoid to create clones of ways or relations or use setNodes(null) / setMembers(null)
  • replaces most ChangeCommand instances by better specialized alternatives
  • add some comments
  • fix some checkstyle / sonar issues
Location:
applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/AssociatedStreetFixer.java

    r36103 r36217  
    55
    66import java.util.ArrayList;
     7import java.util.Collections;
    78import java.util.HashMap;
    89import java.util.List;
    910import java.util.Map;
    1011
    11 import org.openstreetmap.josm.command.ChangeCommand;
     12import org.openstreetmap.josm.command.ChangeMembersCommand;
     13import org.openstreetmap.josm.command.ChangePropertyCommand;
    1214import org.openstreetmap.josm.command.Command;
    1315import org.openstreetmap.josm.command.SequenceCommand;
    … …  
    2325public class AssociatedStreetFixer extends RelationFixer {
    2426
     27    private static final String ADDR_HOUSENUMBER = "addr:housenumber";
     28    private static final String BUILDING = "building";
     29    private static final String HOUSE = "house";
     30    private static final String STREET = "street";
     31
    2532    public AssociatedStreetFixer() {
    2633        super("associatedStreet");
    … …  
    3037    public boolean isRelationGood(Relation rel) {
    3138        for (RelationMember m : rel.getMembers()) {
    32             if (m.getType().equals(OsmPrimitiveType.NODE) && !"house".equals(m.getRole())) {
     39            if (m.getType() == OsmPrimitiveType.NODE && !HOUSE.equals(m.getRole())) {
    3340                setWarningMessage(tr("Node without ''house'' role found"));
    3441                return false;
    3542            }
    36             if (m.getType().equals(OsmPrimitiveType.WAY) && !("house".equals(m.getRole()) || "street".equals(m.getRole()))) {
     43            if (m.getType() == OsmPrimitiveType.WAY && !(HOUSE.equals(m.getRole()) || STREET.equals(m.getRole()))) {
    3744                setWarningMessage(tr("Way without ''house'' or ''street'' role found"));
    3845                return false;
    3946            }
    40             if (m.getType().equals(OsmPrimitiveType.RELATION) && !"house".equals(m.getRole())) {
     47            if (m.getType() == OsmPrimitiveType.RELATION && !HOUSE.equals(m.getRole())) {
    4148                setWarningMessage(tr("Relation without ''house'' role found"));
    4249                return false;
    … …  
    5461        }
    5562        for (RelationMember m : rel.getMembers()) {
    56             if ("street".equals(m.getRole()) && !streetName.equals(m.getWay().get("name"))) {
     63            if (STREET.equals(m.getRole()) && !streetName.equals(m.getWay().get("name"))) {
    5764                String anotherName = m.getWay().get("name");
    5865                if (anotherName != null && !anotherName.isEmpty()) {
    … …  
    7279        // name - check which name is most used in street members and add to relation
    7380        // copy this name to the other street members (???)
    74         Relation rel = new Relation(source);
     81        List<RelationMember> members = source.getMembers();
    7582        boolean fixed = false;
    7683
    77         for (int i = 0; i < rel.getMembersCount(); i++) {
    78             RelationMember m = rel.getMember(i);
    79 
     84        for (int i = 0; i < members.size(); i++) {
     85            RelationMember m = members.get(i);
    8086            if (m.isNode()) {
    8187                Node node = m.getNode();
    82                 if (!"house".equals(m.getRole()) &&
    83                         (node.hasKey("building") || node.hasKey("addr:housenumber"))) {
     88                if (!HOUSE.equals(m.getRole()) &&
     89                        (node.hasKey(BUILDING) || node.hasKey(ADDR_HOUSENUMBER))) {
    8490                    fixed = true;
    85                     rel.setMember(i, new RelationMember("house", node));
     91                    members.set(i, new RelationMember(HOUSE, node));
    8692                }
    8793            } else if (m.isWay()) {
    8894                Way way = m.getWay();
    89                 if (!"street".equals(m.getRole()) && way.hasKey("highway")) {
     95                if (!STREET.equals(m.getRole()) && way.hasKey("highway")) {
    9096                    fixed = true;
    91                     rel.setMember(i, new RelationMember("street", way));
    92                 } else if (!"house".equals(m.getRole()) &&
    93                         (way.hasKey("building") || way.hasKey("addr:housenumber"))) {
     97                    members.set(i, new RelationMember(STREET, way));
     98                } else if (!HOUSE.equals(m.getRole()) &&
     99                        (way.hasKey(BUILDING) || way.hasKey(ADDR_HOUSENUMBER))) {
    94100                    fixed = true;
    95                     rel.setMember(i, new RelationMember("house", way));
     101                    members.set(i, new RelationMember(HOUSE, way));
    96102                }
    97103            } else if (m.isRelation()) {
    98104                Relation relation = m.getRelation();
    99                 if (!"house".equals(m.getRole()) &&
    100                         (relation.hasKey("building") || relation.hasKey("addr:housenumber") || "multipolygon".equals(relation.get("type")))) {
     105                if (!HOUSE.equals(m.getRole()) &&
     106                        (relation.hasKey(BUILDING) || relation.hasKey(ADDR_HOUSENUMBER) || "multipolygon".equals(relation.get("type")))) {
    101107                    fixed = true;
    102                     rel.setMember(i, new RelationMember("house", relation));
     108                    members.set(i, new RelationMember(HOUSE, relation));
    103109                }
    104110            }
    … …  
    107113        // fill relation name
    108114        Map<String, Integer> streetNames = new HashMap<>();
    109         for (RelationMember m : rel.getMembers()) {
    110             if ("street".equals(m.getRole()) && m.isWay()) {
     115        for (RelationMember m : members) {
     116            if (STREET.equals(m.getRole()) && m.isWay()) {
    111117                String name = m.getWay().get("name");
    112118                if (name == null || name.isEmpty()) {
    113119                    continue;
    114120                }
    115 
    116121                Integer count = streetNames.get(name);
    117 
    118122                streetNames.put(name, count != null ? count + 1 : 1);
    119123            }
    … …  
    128132        }
    129133
    130         if (!rel.hasKey("name") && !commonName.isEmpty()) {
    131             fixed = true;
    132             rel.put("name", commonName);
    133         } else {
    134             commonName = ""; // set empty common name - if we already have name on relation, do not overwrite it
     134        Map<String, String> nameTag = new HashMap<>();
     135        if (!source.hasKey("name") && !commonName.isEmpty()) {
     136            nameTag.put("name", commonName);
    135137        }
    136138
    137139        List<Command> commandList = new ArrayList<>();
     140        final DataSet ds = Utils.firstNonNull(source.getDataSet(), MainApplication.getLayerManager().getEditDataSet());
    138141        if (fixed) {
    139             final DataSet ds = Utils.firstNonNull(source.getDataSet(), MainApplication.getLayerManager().getEditDataSet());
    140             commandList.add(new ChangeCommand(ds, source, rel));
     142            commandList.add(new ChangeMembersCommand(ds, source, members));
     143        }
     144        if (!nameTag.isEmpty()) {
     145            commandList.add(new ChangePropertyCommand(ds, Collections.singleton(source), nameTag));
    141146        }
    142147
    143         /*if (!commonName.isEmpty())
    144         // fill common name to streets
    145         for (RelationMember m : rel.getMembers())
    146             if ("street".equals(m.getRole()) && m.isWay()) {
    147                 String name = m.getWay().get("name");
    148                 if (commonName.equals(name)) continue;
    149 
    150                 // TODO: ask user if he really wants to overwrite street name??
    151 
    152                 Way oldWay = m.getWay();
    153                 Way newWay = new Way(oldWay);
    154                 newWay.put("name", commonName);
    155 
    156                 commandList.add(new ChangeCommand(MainApplication.getLayerManager().getEditDataSet(), oldWay, newWay));
    157             }
    158          */
    159148        // return results
    160         if (commandList.isEmpty()) {
     149        if (commandList.isEmpty())
    161150            return null;
    162         }
    163151        return SequenceCommand.wrapIfNeeded(tr("fix associatedStreet relation"), commandList);
    164152    }

  • applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/BoundaryFixer.java

    r36103 r36217  
    44import static org.openstreetmap.josm.tools.I18n.tr;
    55
    6 import org.openstreetmap.josm.command.ChangeCommand;
     6import java.util.ArrayList;
     7import java.util.List;
     8
     9import org.openstreetmap.josm.command.ChangeMembersCommand;
    710import org.openstreetmap.josm.command.Command;
    811import org.openstreetmap.josm.data.osm.DataSet;
    … …  
    3740    public boolean isRelationGood(Relation rel) {
    3841        for (RelationMember m : rel.getMembers()) {
    39             if (m.getType().equals(OsmPrimitiveType.RELATION) && !"subarea".equals(m.getRole())) {
     42            if (m.getType() == OsmPrimitiveType.RELATION && !"subarea".equals(m.getRole())) {
    4043                setWarningMessage(tr("Relation without ''subarea'' role found"));
    4144                return false;
    4245            }
    43             if (m.getType().equals(OsmPrimitiveType.NODE) && !("label".equals(m.getRole()) || "admin_centre".equals(m.getRole()))) {
     46            if (m.getType() == OsmPrimitiveType.NODE && !("label".equals(m.getRole()) || "admin_centre".equals(m.getRole()))) {
    4447                setWarningMessage(tr("Node without ''label'' or ''admin_centre'' role found"));
    4548                return false;
    4649            }
    47             if (m.getType().equals(OsmPrimitiveType.WAY) && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) {
     50            if (m.getType() == OsmPrimitiveType.WAY && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) {
    4851                setWarningMessage(tr("Way without ''inner'' or ''outer'' role found"));
    4952                return false;
    … …  
    5659    @Override
    5760    public Command fixRelation(Relation rel) {
    58         Relation r = rel;
    59         Relation rr = fixMultipolygonRoles(r);
    60         boolean fixed = false;
    61         if (rr != null) {
    62             fixed = true;
    63             r = rr;
     61        List<RelationMember> members = fixMultipolygonRoles(rel.getMembers());
     62        if (members.isEmpty()) {
     63            members = rel.getMembers();
    6464        }
    65         rr = fixBoundaryRoles(r);
    66         if (rr != null) {
    67             fixed = true;
    68             r = rr;
    69         }
    70         if (fixed) {
     65        members = fixBoundaryRoles(members);
     66        if (!members.equals(rel.getMembers())) {
    7167            final DataSet ds = Utils.firstNonNull(rel.getDataSet(), MainApplication.getLayerManager().getEditDataSet());
    72             return new ChangeCommand(ds, rel, r);
     68            return new ChangeMembersCommand(ds, rel, members);
    7369        }
    7470        return null;
    7571    }
    7672
    77     private Relation fixBoundaryRoles(Relation source) {
    78         Relation r = new Relation(source);
    79         boolean fixed = false;
    80         for (int i = 0; i < r.getMembersCount(); i++) {
    81             RelationMember m = r.getMember(i);
     73    /**
     74     * Possibly change roles of non-way members.
     75     * @param origMembers original list of relation members
     76     * @return either the original and unmodified list or a new one with at least one new item
     77     */
     78    private static List<RelationMember> fixBoundaryRoles(List<RelationMember> origMembers) {
     79        List<RelationMember> members = origMembers;
     80        for (int i = 0; i < members.size(); i++) {
     81            RelationMember m = members.get(i);
    8282            String role = null;
    8383            if (m.isRelation()) {
    8484                role = "subarea";
    85             } else if (m.isNode()) {
     85            } else if (m.isNode() && !m.getMember().isIncomplete()) {
    8686                Node n = (Node) m.getMember();
    87                 if (!n.isIncomplete()) {
    88                     if (n.hasKey("place")) {
    89                         String place = n.get("place");
    90                         if (place.equals("state") || place.equals("country") ||
    91                                 place.equals("county") || place.equals("region")) {
    92                             role = "label";
    93                         } else {
    94                             role = "admin_centre";
    95                         }
     87                if (n.hasKey("place")) {
     88                    String place = n.get("place");
     89                    if ("state".equals(place) || "country".equals(place) ||
     90                            "county".equals(place) || "region".equals(place)) {
     91                        role = "label";
    9692                    } else {
    97                         role = "label";
     93                        role = "admin_centre";
    9894                    }
     95                } else {
     96                    role = "label";
    9997                }
    10098            }
    10199            if (role != null && !role.equals(m.getRole())) {
    102                 r.setMember(i, new RelationMember(role, m.getMember()));
    103                 fixed = true;
     100                if (members == origMembers) {
     101                    members = new ArrayList<>(origMembers); // don't modify original list
     102                }
     103                members.set(i, new RelationMember(role, m.getMember()));
    104104            }
    105105        }
    106         return fixed ? r : null;
     106        return members;
    107107    }
    108108}

  • applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/MultipolygonFixer.java

    r36103 r36217  
    55
    66import java.util.ArrayList;
    7 import java.util.Collection;
     7import java.util.Collections;
    88import java.util.HashSet;
     9import java.util.List;
    910import java.util.Set;
     11import java.util.stream.Collectors;
    1012
    11 import org.openstreetmap.josm.command.ChangeCommand;
     13import org.openstreetmap.josm.command.ChangeMembersCommand;
    1214import org.openstreetmap.josm.command.Command;
    1315import org.openstreetmap.josm.data.osm.DataSet;
    … …  
    2931    }
    3032
    31     protected MultipolygonFixer(String...types) {
     33    protected MultipolygonFixer(String... types) {
    3234        super(types);
    3335    }
    … …  
    3638    public boolean isRelationGood(Relation rel) {
    3739        for (RelationMember m : rel.getMembers()) {
    38             if (m.getType().equals(OsmPrimitiveType.WAY) && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) {
     40            if (m.getType() == OsmPrimitiveType.WAY && !("outer".equals(m.getRole()) || "inner".equals(m.getRole()))) {
    3941                setWarningMessage(tr("Way without ''inner'' or ''outer'' role found"));
    4042                return false;
    … …  
    4749    @Override
    4850    public Command fixRelation(Relation rel) {
    49         Relation rr = fixMultipolygonRoles(rel);
    50         if (rr != null) {
     51        List<RelationMember> members = fixMultipolygonRoles(rel.getMembers());
     52        if (!members.equals(rel.getMembers())) {
    5153            final DataSet ds = Utils.firstNonNull(rel.getDataSet(), MainApplication.getLayerManager().getEditDataSet());
    52             return new ChangeCommand(ds, rel, rr);
     54            return new ChangeMembersCommand(ds, rel, members);
    5355        }
    5456        return null;
    … …  
    5658
    5759    /**
    58      * Basically, created multipolygon from scratch, and if successful, replace roles with new ones.
     60     * Basically, create member list of a multipolygon from scratch, and if
     61     * successful, replace roles with new ones.
     62     *
     63     * @param origMembers original list of relation members
     64     * @return either the original and unmodified list or a new one with at least
     65     *         one new item or the empty list if the way members don't build a
     66     *         correct multipolygon
    5967     */
    60     protected Relation fixMultipolygonRoles(Relation source) {
    61         Collection<Way> ways = new ArrayList<>(source.getMemberPrimitives(Way.class));
     68    protected List<RelationMember> fixMultipolygonRoles(final List<RelationMember> origMembers) {
     69        List<Way> ways = origMembers.stream().filter(m -> m.isWay()).map(m -> m.getWay()).collect(Collectors.toList());
     70
    6271        MultipolygonBuilder mpc = new MultipolygonBuilder();
    6372        String error = mpc.makeFromWays(ways);
    6473        if (error != null)
    65             return null;
     74            return Collections.emptyList();
    6675
    67         Relation r = new Relation(source);
    68         boolean fixed = false;
    6976        Set<Way> outerWays = new HashSet<>();
    7077        for (MultipolygonBuilder.JoinedPolygon poly : mpc.outerWays) {
    … …  
    7582            innerWays.addAll(poly.ways);
    7683        }
    77         for (int i = 0; i < r.getMembersCount(); i++) {
    78             RelationMember m = r.getMember(i);
     84        List<RelationMember> members = origMembers;
     85        for (int i = 0; i < members.size(); i++) {
     86            RelationMember m = members.get(i);
    7987            if (m.isWay()) {
    8088                final Way way = m.getWay();
    … …  
    8694                }
    8795                if (role != null && !role.equals(m.getRole())) {
    88                     r.setMember(i, new RelationMember(role, way));
    89                     fixed = true;
     96                    if (members == origMembers) {
     97                        members = new ArrayList<>(origMembers); // don't modify original list
     98                    }
     99                    members.set(i, new RelationMember(role, way));
    90100                }
    91101            }
    92102        }
    93         return fixed ? r : null;
     103        return members;
    94104    }
    95105}

  • applications/editors/josm/plugins/reltoolbox/src/relcontext/relationfix/PublicTransportFixer.java

    r36103 r36217  
    44import static org.openstreetmap.josm.tools.I18n.tr;
    55
    6 import org.openstreetmap.josm.command.ChangeCommand;
     6import java.util.ArrayList;
     7import java.util.List;
     8
     9import org.openstreetmap.josm.command.ChangeMembersCommand;
    710import org.openstreetmap.josm.command.Command;
    811import org.openstreetmap.josm.data.osm.DataSet;
    … …  
    1114import org.openstreetmap.josm.data.osm.RelationMember;
    1215import org.openstreetmap.josm.gui.MainApplication;
     16import org.openstreetmap.josm.tools.Utils;
    1317
    14 import org.openstreetmap.josm.tools.Utils;
    1518import relcontext.actions.PublicTransportHelper;
    1619
    … …  
    2629    }
    2730
    28     /*protected PublicTransportFixer(String...types) {
    29         super(types);
    30     }*/
    31 
    3231    @Override
    3332    public boolean isRelationGood(Relation rel) {
    3433        for (RelationMember m : rel.getMembers()) {
    35             if (m.getType().equals(OsmPrimitiveType.NODE)
     34            if (m.getType() == OsmPrimitiveType.NODE
    3635                    && !(m.getRole().startsWith(PublicTransportHelper.STOP) || m.getRole().startsWith(PublicTransportHelper.PLATFORM))) {
    3736                setWarningMessage(tr("Node without ''stop'' or ''platform'' role found"));
    3837                return false;
    3938            }
    40             if (m.getType().equals(OsmPrimitiveType.WAY)
     39            if (m.getType() == OsmPrimitiveType.WAY
    4140                    && PublicTransportHelper.isWayPlatform(m)
    4241                    && !m.getRole().startsWith(PublicTransportHelper.PLATFORM)) {
    … …  
    4948    }
    5049
    51     /*@Override
    52     public boolean isFixerApplicable(Relation rel) {
    53         return true;
    54     }*/
    55 
    5650    @Override
    5751    public Command fixRelation(Relation rel) {
    58         Relation r = rel;
    59         Relation rr = fixStopPlatformRole(r);
    60         boolean fixed = false;
    61         if (rr != null) {
    62             fixed = true;
    63             r = rr;
    64         }
    65         if (fixed) {
     52        List<RelationMember> members = fixStopPlatformRole(rel.getMembers());
     53        if (!members.equals(rel.getMembers())) {
    6654            final DataSet ds = Utils.firstNonNull(rel.getDataSet(), MainApplication.getLayerManager().getEditDataSet());
    67             return new ChangeCommand(ds, rel, r);
     55            return new ChangeMembersCommand(ds, rel, rel.getMembers());
    6856        }
    6957        return null;
    7058    }
    7159
    72     private Relation fixStopPlatformRole(Relation source) {
    73         Relation r = new Relation(source);
    74         boolean fixed = false;
    75         for (int i = 0; i < r.getMembersCount(); i++) {
    76             RelationMember m = r.getMember(i);
     60    /**
     61     * Fix roles of members.
     62     * @param origMembers original list of relation members
     63     * @return either the original and unmodified list or a new one with at least one new item
     64     */
     65    private static List<RelationMember> fixStopPlatformRole(List<RelationMember> origMembers) {
     66        List<RelationMember> members = origMembers;
     67        for (int i = 0; i < members.size(); i++) {
     68            RelationMember m = members.get(i);
    7769            String role = PublicTransportHelper.getRoleByMember(m);
    7870
    7971            if (role != null && !m.getRole().startsWith(role)) {
    80                 r.setMember(i, new RelationMember(role, m.getMember()));
    81                 fixed = true;
     72                if (members == origMembers) {
     73                    members = new ArrayList<>(origMembers);
     74                }
     75                members.set(i, new RelationMember(role, m.getMember()));
    8276            }
    8377        }
    84         return fixed ? r : null;
     78        return members;
    8579    }
    8680}
Note: See TracChangeset for help on using the changeset viewer.