Changeset 17429 in josm

Timestamp:

2020-12-30T11:52:34+01:00 (3 years 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:

• src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java (modified) (4 diffs)
• test/data/regress/20325 (added)
• test/data/regress/20325/data.osm (added)
• test/data/regress/20325/invalid-new-upldate.osm (added)
• test/data/regress/20325/no-change-new.osm (added)
• test/data/regress/20325/no-change-old.osm (added)
• test/data/regress/20325/update-no-command-warning.osm (added)
• test/data/upd-multipolygon-cases (added)
• test/unit/org/openstreetmap/josm/actions/CreateMultipolygonActionTest.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java

@@ -341 +341 @@
         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<Command> list = removeTagsFromWaysIfNeeded(relation);
@@ -349 +349 @@
             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;
                 }
 
@@ -404 +401 @@
      * 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
@@ -488 +485 @@
         if (moveTags && !values.isEmpty()) {
             // add those tag values to the relation
-            boolean fixed = false;
             Map<String, String> tagsToAdd = new HashMap<>();
             for (Entry<String, String> 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));
             }
         }

trunk/test/unit/org/openstreetmap/josm/actions/CreateMultipolygonActionTest.java

@@ -5 +5 @@
 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;
 
@@ -45 +46 @@
     @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<String, String> getRefToRoleMap(Relation relation) {
@@ -214 +215 @@
         assertEquals(1, ds.getWays().stream().filter(w -> w.hasTag("building", "yes")).count());
     }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
+     * @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<SequenceCommand, Relation> 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 <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
+     * 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<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
+        assertNull(cmd);
+    }
+
+    /**
+     * Coverage test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
+     * 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<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
+        assertNull(cmd);
+    }
+
+    /**
+     * Coverage test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
+     * 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<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
+        assertNull(cmd);
+    }
+
+    /**
+     * Coverage test for <a href="https://josm.openstreetmap.de/ticket/20325">Bug #20325</a>.
+     * 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<SequenceCommand, Relation> cmd = CreateMultipolygonAction.createMultipolygonCommand(ds.getWays(), mp);
+        assertNull(cmd);
+    }
+
 }