Changeset 18922 in josm


Ignore:
Timestamp:
2023-12-20T20:03:45+01:00 (5 months ago)
Author:
taylor.smock
Message:

Fix #23302: Create a preference for address duplicate detection to include buildings and POIs (patch by zyphlar, modified)

Modifications are as follows:

  • Add test case
Location:
trunk
Files:
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/data/validation/tests/Addresses.java

    r18871 r18922  
    2020import java.util.stream.Stream;
    2121
     22import javax.swing.JCheckBox;
     23import javax.swing.JPanel;
     24
    2225import org.openstreetmap.josm.command.Command;
    2326import org.openstreetmap.josm.command.DeleteCommand;
    … …  
    3134import org.openstreetmap.josm.data.osm.TagMap;
    3235import org.openstreetmap.josm.data.osm.Way;
     36import org.openstreetmap.josm.data.preferences.BooleanProperty;
    3337import org.openstreetmap.josm.data.preferences.DoubleProperty;
     38import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
    3439import org.openstreetmap.josm.data.validation.Severity;
    3540import org.openstreetmap.josm.data.validation.Test;
    3641import org.openstreetmap.josm.data.validation.TestError;
     42import org.openstreetmap.josm.gui.progress.ProgressMonitor;
     43import org.openstreetmap.josm.tools.GBC;
    3744import org.openstreetmap.josm.tools.Geometry;
    3845import org.openstreetmap.josm.tools.Logging;
    … …  
    7178    protected static final String ADDR_POSTCODE      = "addr:postcode";
    7279    protected static final String ASSOCIATED_STREET  = "associatedStreet";
     80    protected static final String NAME_TAG           = "name";
     81    private static final String HOUSE                = "house";
     82    private static final String STREET               = "street";
    7383    // CHECKSTYLE.ON: SingleSpaceSeparator
     84
     85    private static final BooleanProperty PREF_INCLUDE_BLDG_POI =
     86            new BooleanProperty(ValidatorPrefHelper.PREFIX + "." + OpeningHourTest.class.getSimpleName() + "." + "includebuildingpois", false);
     87    private final JCheckBox checkboxIncludeBldgPOI = new JCheckBox(
     88            /* I18n: Label text for checkbox choosing to validate addresses for all types of objects, not just plain addresses */
     89            tr("Include POIs like amenities, offices, and buildings in duplicate address detection"));
     90    private boolean includeBldgAndPOI;
    7491
    7592    private Map<String, Collection<OsmPrimitive>> knownAddresses;
    … …  
    8097     */
    8198    public Addresses() {
     99        /* I18n: Label text for checkbox choosing to validate addresses */
    82100        super(tr("Addresses"), tr("Checks for errors in addresses and associatedStreet relations."));
    83101    }
    … …  
    142160     */
    143161    private void collectAddress(OsmPrimitive p) {
    144         if (!isPOI(p)) {
     162        if (includeBldgAndPOI || !isPOI(p)) {
    145163            for (String simplifiedAddress : getSimplifiedAddresses(p)) {
    146164                if (!ignoredAddresses.contains(simplifiedAddress)) {
    … …  
    155173        ignoredAddresses = new HashSet<>();
    156174        for (OsmPrimitive p : primitive.getDataSet().allNonDeletedPrimitives()) {
    157             if (p instanceof Node && p.hasKey(ADDR_UNIT, ADDR_FLATS)) {
     175            if ((includeBldgAndPOI || p instanceof Node) && p.hasKey(ADDR_UNIT, ADDR_FLATS)) {
    158176                for (OsmPrimitive r : p.getReferrers()) {
    159177                    if (hasAddress(r)) {
    … …  
    177195
    178196    @Override
     197    public void startTest(ProgressMonitor progressMonitor) {
     198        super.startTest(progressMonitor);
     199        this.includeBldgAndPOI = PREF_INCLUDE_BLDG_POI.get();
     200    }
     201
     202    @Override
    179203    public void endTest() {
    180204        knownAddresses = null;
    … …  
    187211            initAddressMap(p);
    188212        }
    189         if (!isPOI(p) && hasAddress(p)) {
     213        if ((includeBldgAndPOI || !isPOI(p)) && hasAddress(p)) {
    190214            List<TestError> result = new ArrayList<>();
    191215            for (String simplifiedAddress : getSimplifiedAddresses(p)) {
    … …  
    199223                        String city1 = p.get(ADDR_CITY);
    200224                        String city2 = p2.get(ADDR_CITY);
     225                        String name1 = p.get(NAME_TAG);
     226                        String name2 = p2.get(NAME_TAG);
    201227                        double distance = getDistance(p, p2);
    202228                        if (city1 != null && city2 != null) {
    … …  
    236262                            }
    237263                        }
     264                        if (severityLevel == Severity.WARNING && !Objects.equals(name1, name2)) {
     265                            // since multiple objects can exist at one address, a different name tag isn't very concerning
     266                            severityLevel = Severity.OTHER;
     267                        }
    238268                        result.add(TestError.builder(this, severityLevel, DUPLICATE_HOUSE_NUMBER)
    239269                                .message(tr("Duplicate house numbers"), marktr("''{0}'' ({1}m)"), simplifiedAddress, (int) distance)
    … …  
    302332                String role = m.getRole();
    303333                OsmPrimitive p = m.getMember();
    304                 if ("house".equals(role)) {
     334                if (HOUSE.equals(role)) {
    305335                    houses.add(p);
    306336                    String number = p.get(ADDR_HOUSE_NUMBER);
    … …  
    316346                        wrongStreetNames.add(p);
    317347                    }
    318                 } else if ("street".equals(role)) {
     348                } else if (STREET.equals(role)) {
    319349                    if (p instanceof Way) {
    320350                        street.add((Way) p);
    … …  
    457487            if ("".equals(role)) {
    458488                if (m.isWay() && m.getMember().hasKey("highway")) {
    459                     role = "street";
     489                    role = STREET;
    460490                } else if (m.getMember().hasTag("building"))
    461                     role = "house";
     491                    role = HOUSE;
    462492            }
    463493            switch (role) {
    464             case "house":
     494            case HOUSE:
    465495            case "addr:houselink":
    466496            case "address":
    … …  
    472502                }
    473503                break;
    474             case "street":
     504            case STREET:
    475505                if (!m.getMember().hasTag("name") && r.hasTag("name"))
    476506                    return;
    … …  
    523553    }
    524554
     555    @Override
     556    public void addGui(JPanel testPanel) {
     557        super.addGui(testPanel);
     558        checkboxIncludeBldgPOI.setSelected(PREF_INCLUDE_BLDG_POI.get());
     559        testPanel.add(checkboxIncludeBldgPOI, GBC.eol().insets(20, 0, 0, 0));
     560    }
     561
     562    @Override
     563    public boolean ok() {
     564        super.ok();
     565        PREF_INCLUDE_BLDG_POI.put(checkboxIncludeBldgPOI.isSelected());
     566        includeBldgAndPOI = PREF_INCLUDE_BLDG_POI.get();
     567        return false;
     568    }
     569
    525570}

  • trunk/test/unit/org/openstreetmap/josm/data/validation/tests/AddressesTest.java

    r18853 r18922  
    33
    44import static org.junit.jupiter.api.Assertions.assertEquals;
     5import static org.junit.jupiter.api.Assertions.assertInstanceOf;
    56import static org.junit.jupiter.api.Assertions.assertNotNull;
    67import static org.junit.jupiter.api.Assertions.assertNull;
     8import static org.junit.jupiter.api.Assertions.assertTrue;
    79import static org.openstreetmap.josm.data.coor.LatLon.NORTH_POLE;
    810import static org.openstreetmap.josm.data.coor.LatLon.SOUTH_POLE;
    … …  
    1012
    1113import java.util.List;
     14
     15import javax.swing.JCheckBox;
     16import javax.swing.JPanel;
    1217
    1318import org.junit.jupiter.api.Test;
    … …  
    1722import org.openstreetmap.josm.data.osm.Node;
    1823import org.openstreetmap.josm.data.osm.RelationMember;
     24import org.openstreetmap.josm.data.osm.Way;
    1925import org.openstreetmap.josm.data.validation.Severity;
    2026import org.openstreetmap.josm.data.validation.TestError;
     27import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
    2128
    2229/**
    … …  
    120127        doTestDuplicateHouseNumber(num1, ZERO, num4, ZERO, null);
    121128    }
     129
     130    /**
     131     * See #23302
     132     */
     133    @Test
     134    void testCheckForDuplicatePOIBuildingAddresses() {
     135        final Addresses test = new Addresses();
     136        final JPanel panel = new JPanel();
     137        final Node poi = TestUtils.newNode("addr:housenumber=1 addr:street=Foo");
     138        final Way building = TestUtils.newWay("addr:housenumber=1 addr:street=Foo building=yes",
     139                TestUtils.newNode(""), TestUtils.newNode(""), TestUtils.newNode(""));
     140        final DataSet ds = new DataSet();
     141        // Ensure that we are checking for building-poi duplicates
     142        test.addGui(panel);
     143        JCheckBox checkboxIncludeBldgPOI = assertInstanceOf(JCheckBox.class, panel.getComponent(panel.getComponentCount() - 1));
     144        checkboxIncludeBldgPOI.setSelected(true);
     145        test.ok();
     146        // Set up the dataset
     147        ds.addPrimitive(poi);
     148        ds.addPrimitiveRecursive(building);
     149        building.addNode(building.firstNode());
     150
     151        // Duplicate addresses with no additional information should always have warnings
     152        test.startTest(NullProgressMonitor.INSTANCE);
     153        test.visit(ds.allPrimitives());
     154        test.endTest();
     155        assertEquals(1, test.getErrors().size());
     156
     157        // Do the first test checking for building-poi duplicates
     158        poi.put("name", "FooBar");
     159        test.startTest(NullProgressMonitor.INSTANCE);
     160        test.visit(ds.allPrimitives());
     161        test.endTest();
     162        assertEquals(1, test.getErrors().size());
     163        assertEquals(Severity.OTHER, test.getErrors().get(0).getSeverity());
     164
     165        // Now check if they have the same name
     166        building.put("name", "FooBar");
     167        test.startTest(NullProgressMonitor.INSTANCE);
     168        test.visit(ds.allPrimitives());
     169        test.endTest();
     170        assertEquals(1, test.getErrors().size());
     171        assertEquals(Severity.WARNING, test.getErrors().get(0).getSeverity());
     172
     173        // Now check if they have a different name
     174        building.put("name", "FooBar2");
     175        test.startTest(NullProgressMonitor.INSTANCE);
     176        test.visit(ds.allPrimitives());
     177        test.endTest();
     178        assertEquals(1, test.getErrors().size());
     179        assertEquals(Severity.OTHER, test.getErrors().get(0).getSeverity());
     180
     181        // Now ensure that it doesn't get errors when disabled
     182        checkboxIncludeBldgPOI.setSelected(false);
     183        test.ok();
     184        test.startTest(NullProgressMonitor.INSTANCE);
     185        test.visit(ds.allPrimitives());
     186        test.endTest();
     187        assertTrue(test.getErrors().isEmpty());
     188    }
    122189}
Note: See TracChangeset for help on using the changeset viewer.