Changeset 17429 in josm


Ignore:
Timestamp:
2020-12-30T11:52:34+01:00 (3 weeks ago)
Author:
GerdP
Message:

fix #20325: Update Multipolygon removes tags instead of moving them to relation

  • rewrite handling of update multipolygon cases
  • let removeTagsFromWaysIfNeeded() check if getDataset() returns null instead of checking isNew(). I assume it was always meant to work like this. JoinAreasAction works fine with that and I hope no plugin relies on the old behaviour.
  • add regression unit test and more unit tests to improve coverage
Location:
trunk
Files:
7 added
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java

    r17408 r17429  
    341341        boolean changedMembers = rr.a != rr.b;
    342342        final Relation existingRelation = rr.a;
    343         final Relation relation = changedMembers ? rr.b : new Relation(rr.a);
     343        final Relation relation = changedMembers ? rr.b : rr.a;
    344344
    345345        final List<Command> list = removeTagsFromWaysIfNeeded(relation);
     
    349349            commandName = getName(false);
    350350        } else {
    351             boolean changedKeys = !relation.getKeys().equals(existingRelation.getKeys());
    352             if (changedKeys && changedMembers)
    353                 list.add(new ChangeCommand(existingRelation, relation));
    354             else if (changedMembers) {
    355                 list.add(new ChangeMembersCommand(existingRelation, new ArrayList<>(relation.getMembers())));
    356             } else if (changedKeys) {
    357                 list.add(ChangePropertyCommand.build(existingRelation, relation));
     351            if (changedMembers) {
     352                if (!relation.getKeys().equals(existingRelation.getKeys())) {
     353                    list.add(new ChangeCommand(existingRelation, relation));
     354                } else {
     355                    list.add(new ChangeMembersCommand(existingRelation, new ArrayList<>(relation.getMembers())));
     356                }
    358357            }
    359358            if (list.isEmpty()) {
    360                 if (!changedMembers) {
    361                     MultipolygonTest mpTest = new MultipolygonTest();
    362                     mpTest.visit(existingRelation);
    363                     if (!mpTest.getErrors().isEmpty()) {
    364                         showErrors(mpTest.getErrors());
    365                         return null;
    366                     }
     359                MultipolygonTest mpTest = new MultipolygonTest();
     360                mpTest.visit(existingRelation);
     361                if (!mpTest.getErrors().isEmpty()) {
     362                    showErrors(mpTest.getErrors());
     363                    return null;
    367364                }
    368365
     
    404401     * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary.
    405402     * Function was extended in reltoolbox plugin by Zverikk and copied back to the core
    406      * @param relation the multipolygon style relation to process. If it is new, the tags might be
     403     * @param relation the multipolygon style relation to process. If it not linked to a dataset, the tags might be
    407404     * modified, else the list of commands will contain a command to modify its tags
    408405     * @return a list of commands to execute
     
    488485        if (moveTags && !values.isEmpty()) {
    489486            // add those tag values to the relation
    490             boolean fixed = false;
    491487            Map<String, String> tagsToAdd = new HashMap<>();
    492488            for (Entry<String, String> entry : values.entrySet()) {
    493489                String key = entry.getKey();
    494490                if (!relation.hasKey(key)) {
    495                     if (relation.isNew())
     491                    if (relation.getDataSet() == null)
    496492                        relation.put(key, entry.getValue());
    497493                    else
    498494                        tagsToAdd.put(key, entry.getValue());
    499                     fixed = true;
    500                 }
    501             }
    502             if (fixed && !relation.isNew()) {
    503                 DataSet ds = relation.getDataSet();
    504                 if (ds == null) {
    505                     ds = MainApplication.getLayerManager().getEditDataSet();
    506                 }
    507                 commands.add(new ChangePropertyCommand(ds, Collections.singleton(relation), tagsToAdd));
     495                }
     496            }
     497            if (!tagsToAdd.isEmpty()) {
     498                commands.add(new ChangePropertyCommand(Collections.singleton(relation), tagsToAdd));
    508499            }
    509500        }
  • trunk/test/unit/org/openstreetmap/josm/actions/CreateMultipolygonActionTest.java

    r17408 r17429  
    55import static org.junit.jupiter.api.Assertions.assertFalse;
    66import static org.junit.jupiter.api.Assertions.assertNotNull;
     7import static org.junit.jupiter.api.Assertions.assertNull;
    78import static org.junit.jupiter.api.Assertions.assertTrue;
    89
     
    4546    @RegisterExtension
    4647    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
    47     public JOSMTestRules test = new JOSMTestRules().projection().main().preferences();
     48    public JOSMTestRules test = new JOSMTestRules().projection().main().preferences().mapStyles();
    4849
    4950    private static Map<String, String> getRefToRoleMap(Relation relation) {
     
    214215        assertEquals(1, ds.getWays().stream().filter(w -> w.hasTag("building", "yes")).count());
    215216    }
     217
     218    /**
     219     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
     220     * @throws Exception if an error occurs
     221     */
     222    @Test
     223    void testTicket20325() throws Exception {
     224        DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "data.osm"), null);
     225        assertEquals(1, ds.getRelations().size());
     226        Relation mp = ds.getRelations().iterator().next();
     227        assertFalse(ds.getRelations().iterator().next().hasTag("landuse", "farmland"));
     228        assertEquals(1, ds.getWays().stream().filter(w -> w.hasTag("landuse", "farmland")).count());
     229        Pair<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
     230        assertNotNull(cmd);
     231        cmd.a.executeCommand();
     232        assertEquals(1, ds.getRelations().size());
     233        assertTrue(ds.getRelations().iterator().next().hasTag("landuse", "farmland"));
     234        assertEquals(0, ds.getWays().stream().filter(w -> w.hasTag("landuse", "farmland")).count());
     235        cmd.a.undoCommand();
     236        assertEquals(1, ds.getRelations().size());
     237        assertFalse(ds.getRelations().iterator().next().hasTag("landuse", "farmland"));
     238        assertEquals(1, ds.getWays().stream().filter(w -> w.hasTag("landuse", "farmland")).count());
     239    }
     240
     241    /**
     242     * Coverage test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
     243     * New relation, no update needed, no command should be produced.
     244     * @throws Exception if an error occurs
     245     */
     246    @Test
     247    void testTicket20325New() throws Exception {
     248        DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "no-change-new.osm"), null);
     249        assertEquals(1, ds.getRelations().size());
     250        Relation mp = ds.getRelations().iterator().next();
     251        Pair<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
     252        assertNull(cmd);
     253    }
     254
     255    /**
     256     * Coverage test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
     257     * Old relation, no update needed, no command should be produced.
     258     * @throws Exception if an error occurs
     259     */
     260    @Test
     261    void testTicket20325Old() throws Exception {
     262        DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "no-change-old.osm"), null);
     263        assertEquals(1, ds.getRelations().size());
     264        Relation mp = ds.getRelations().iterator().next();
     265        Pair<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
     266        assertNull(cmd);
     267    }
     268
     269    /**
     270     * Coverage test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
     271     * Relation cannot be updated but produces warnings. Doesn't test that a popup was shown.
     272     * @throws Exception if an error occurs
     273     */
     274    @Test
     275    void testTicket20325Invalid() throws Exception {
     276        DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "invalid-new-upldate.osm"), null);
     277        assertEquals(1, ds.getRelations().size());
     278        Relation mp = ds.getRelations().iterator().next();
     279        Pair<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
     280        assertNull(cmd);
     281    }
     282
     283    /**
     284     * Coverage test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
     285     * Relation needs no updates but produces warnings. Doesn't test that a popup was shown.
     286     * @throws Exception if an error occurs
     287     */
     288    @Test
     289    void testTicket20325NoUpdateWarning() throws Exception {
     290        DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "update-no-command-warning.osm"), null);
     291        assertEquals(1, ds.getRelations().size());
     292        Relation mp = ds.getRelations().iterator().next();
     293        Pair<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
     294        assertNull(cmd);
     295    }
     296
    216297}
Note: See TracChangeset for help on using the changeset viewer.