Description: 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 Author: GerdP Origin: https://josm.openstreetmap.de/changeset/17429/josm Bug: https://josm.openstreetmap.de/ticket/20325 --- a/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java +++ b/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java @@ -340,7 +340,7 @@ public class CreateMultipolygonAction ex } boolean changedMembers = rr.a != rr.b; final Relation existingRelation = rr.a; - final Relation relation = changedMembers ? rr.b : new Relation(rr.a); + final Relation relation = changedMembers ? rr.b : rr.a; final List list = removeTagsFromWaysIfNeeded(relation); final String commandName; @@ -348,22 +348,19 @@ public class CreateMultipolygonAction ex list.add(new AddCommand(selectedWays.iterator().next().getDataSet(), relation)); commandName = getName(false); } else { - boolean changedKeys = !relation.getKeys().equals(existingRelation.getKeys()); - if (changedKeys && changedMembers) - list.add(new ChangeCommand(existingRelation, relation)); - else if (changedMembers) { - list.add(new ChangeMembersCommand(existingRelation, new ArrayList<>(relation.getMembers()))); - } else if (changedKeys) { - list.add(ChangePropertyCommand.build(existingRelation, relation)); + if (changedMembers) { + if (!relation.getKeys().equals(existingRelation.getKeys())) { + list.add(new ChangeCommand(existingRelation, relation)); + } else { + list.add(new ChangeMembersCommand(existingRelation, new ArrayList<>(relation.getMembers()))); + } } if (list.isEmpty()) { - if (!changedMembers) { - MultipolygonTest mpTest = new MultipolygonTest(); - mpTest.visit(existingRelation); - if (!mpTest.getErrors().isEmpty()) { - showErrors(mpTest.getErrors()); - return null; - } + MultipolygonTest mpTest = new MultipolygonTest(); + mpTest.visit(existingRelation); + if (!mpTest.getErrors().isEmpty()) { + showErrors(mpTest.getErrors()); + return null; } GuiHelper.runInEDT(() -> new Notification(tr("Nothing changed")).setDuration(Notification.TIME_SHORT) @@ -403,7 +400,7 @@ public class CreateMultipolygonAction ex /** * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary. * Function was extended in reltoolbox plugin by Zverikk and copied back to the core - * @param relation the multipolygon style relation to process. If it is new, the tags might be + * @param relation the multipolygon style relation to process. If it not linked to a dataset, the tags might be * modified, else the list of commands will contain a command to modify its tags * @return a list of commands to execute */ @@ -487,24 +484,18 @@ public class CreateMultipolygonAction ex values.remove("area"); if (moveTags && !values.isEmpty()) { // add those tag values to the relation - boolean fixed = false; Map tagsToAdd = new HashMap<>(); for (Entry entry : values.entrySet()) { String key = entry.getKey(); if (!relation.hasKey(key)) { - if (relation.isNew()) + if (relation.getDataSet() == null) relation.put(key, entry.getValue()); else tagsToAdd.put(key, entry.getValue()); - fixed = true; } } - if (fixed && !relation.isNew()) { - DataSet ds = relation.getDataSet(); - if (ds == null) { - ds = MainApplication.getLayerManager().getEditDataSet(); - } - commands.add(new ChangePropertyCommand(ds, Collections.singleton(relation), tagsToAdd)); + if (!tagsToAdd.isEmpty()) { + commands.add(new ChangePropertyCommand(Collections.singleton(relation), tagsToAdd)); } } --- /dev/null +++ b/test/data/regress/20325/data.osm @@ -0,0 +1,407 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + --- /dev/null +++ b/test/data/regress/20325/invalid-new-upldate.osm @@ -0,0 +1,23 @@ + + + + + + + + + + + + + + + + + + + + + + + --- /dev/null +++ b/test/data/regress/20325/no-change-new.osm @@ -0,0 +1,31 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + --- /dev/null +++ b/test/data/regress/20325/no-change-old.osm @@ -0,0 +1,96 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + --- /dev/null +++ b/test/data/regress/20325/update-no-command-warning.osm @@ -0,0 +1,16 @@ + + + + + + + + + + + + + + + + --- a/test/unit/org/openstreetmap/josm/actions/CreateMultipolygonActionTest.java +++ b/test/unit/org/openstreetmap/josm/actions/CreateMultipolygonActionTest.java @@ -4,6 +4,7 @@ package org.openstreetmap.josm.actions; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.file.Files; @@ -44,7 +45,7 @@ class CreateMultipolygonActionTest { */ @RegisterExtension @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") - public JOSMTestRules test = new JOSMTestRules().projection().main().preferences(); + public JOSMTestRules test = new JOSMTestRules().projection().main().preferences().mapStyles(); private static Map getRefToRoleMap(Relation relation) { Map refToRole = new TreeMap<>(); @@ -213,4 +214,84 @@ class CreateMultipolygonActionTest { assertFalse(ds.getRelations().iterator().next().hasTag("building", "yes")); assertEquals(1, ds.getWays().stream().filter(w -> w.hasTag("building", "yes")).count()); } + + /** + * Non-regression test for Bug #20325. + * @throws Exception if an error occurs + */ + @Test + void testTicket20325() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "data.osm"), null); + assertEquals(1, ds.getRelations().size()); + Relation mp = ds.getRelations().iterator().next(); + assertFalse(ds.getRelations().iterator().next().hasTag("landuse", "farmland")); + assertEquals(1, ds.getWays().stream().filter(w -> w.hasTag("landuse", "farmland")).count()); + Pair cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp); + assertNotNull(cmd); + cmd.a.executeCommand(); + assertEquals(1, ds.getRelations().size()); + assertTrue(ds.getRelations().iterator().next().hasTag("landuse", "farmland")); + assertEquals(0, ds.getWays().stream().filter(w -> w.hasTag("landuse", "farmland")).count()); + cmd.a.undoCommand(); + assertEquals(1, ds.getRelations().size()); + assertFalse(ds.getRelations().iterator().next().hasTag("landuse", "farmland")); + assertEquals(1, ds.getWays().stream().filter(w -> w.hasTag("landuse", "farmland")).count()); + } + + /** + * Coverage test for Bug #20325. + * New relation, no update needed, no command should be produced. + * @throws Exception if an error occurs + */ + @Test + void testTicket20325New() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "no-change-new.osm"), null); + assertEquals(1, ds.getRelations().size()); + Relation mp = ds.getRelations().iterator().next(); + Pair cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp); + assertNull(cmd); + } + + /** + * Coverage test for Bug #20325. + * Old relation, no update needed, no command should be produced. + * @throws Exception if an error occurs + */ + @Test + void testTicket20325Old() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "no-change-old.osm"), null); + assertEquals(1, ds.getRelations().size()); + Relation mp = ds.getRelations().iterator().next(); + Pair cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp); + assertNull(cmd); + } + + /** + * Coverage test for Bug #20325. + * Relation cannot be updated but produces warnings. Doesn't test that a popup was shown. + * @throws Exception if an error occurs + */ + @Test + void testTicket20325Invalid() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "invalid-new-upldate.osm"), null); + assertEquals(1, ds.getRelations().size()); + Relation mp = ds.getRelations().iterator().next(); + Pair cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp); + assertNull(cmd); + } + + /** + * Coverage test for Bug #20325. + * Relation needs no updates but produces warnings. Doesn't test that a popup was shown. + * @throws Exception if an error occurs + */ + @Test + void testTicket20325NoUpdateWarning() throws Exception { + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(20325, "update-no-command-warning.osm"), null); + assertEquals(1, ds.getRelations().size()); + Relation mp = ds.getRelations().iterator().next(); + Pair cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp); + assertNull(cmd); + } + }