Changeset 36142 in osm for applications/editors/josm/plugins/reltoolbox

Timestamp:

2023-09-18T19:27:57+02:00 (14 months ago)

Author:

taylor.smock

Message:

See #23170: Better behavior around undo/redo, only delete ways with one referrer

Location:

applications/editors/josm/plugins/reltoolbox

Files:

• src/relcontext/actions/ReconstructPolygonAction.java (modified) (3 diffs)
• test/data (added)
• test/data/regress (added)
• test/data/regress/23170 (added)
• test/data/regress/23170/r1585888.osm (added)
• test/unit/relcontext/actions/ReconstructPolygonActionTest.java (modified) (5 diffs)

applications/editors/josm/plugins/reltoolbox/src/relcontext/actions/ReconstructPolygonAction.java

@@ -64 +64 @@
         boolean wont = false;
         for (RelationMember m : r.getMembers()) {
-            if (m.isWay() && m.getWay().getReferrers().size() == 1) {
+            if (m.isWay()) {
                 ways.add(m.getWay());
             } else {
@@ -157 +157 @@
             tags.remove("type");
 
-            // then delete ways that are not relevant (do not take part in other relations of have strange tags)
+            // then delete ways that are not relevant (do not take part in other relations or have strange tags)
             Way candidateWay = null;
             for (Way w : p.ways) {
-                if (w.getReferrers().equals(relations)) {
+                if (w.getReferrers().size() == 1) {
                     // check tags that remain
                     Set<String> keys = new HashSet<>(w.keySet());
@@ -192 +192 @@
         // only delete the relation if it hasn't been re-used
         if (!relationReused) {
-            commands.add(relationDeleteCommand);
+            // The relation needs to be deleted first, so that undo/redo continue to work properly
+            commands.add(0, relationDeleteCommand);
         }
 

applications/editors/josm/plugins/reltoolbox/test/unit/relcontext/actions/ReconstructPolygonActionTest.java

@@ -5 +5 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.openstreetmap.josm.tools.I18n.tr;
 
-import java.awt.Component;
-import java.util.Arrays;
+import java.io.IOException;
 import java.util.Collection;
-import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicReference;
-import java.util.function.Predicate;
 import java.util.stream.Stream;
-
-import javax.swing.JOptionPane;
 
 import org.junit.jupiter.api.AfterEach;
@@ -23 +16 @@
 import org.openstreetmap.josm.actions.DeleteAction;
 import org.openstreetmap.josm.command.DeleteCommand;
+import org.openstreetmap.josm.data.UndoRedoHandler;
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.DatasetConsistencyTest;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.RelationMember;
 import org.openstreetmap.josm.data.osm.RelationToChildReference;
+import org.openstreetmap.josm.data.osm.SimplePrimitiveId;
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
+import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.gui.util.GuiHelper;
+import org.openstreetmap.josm.io.IllegalDataException;
+import org.openstreetmap.josm.io.OsmReader;
 import org.openstreetmap.josm.testutils.annotations.Main;
 import org.openstreetmap.josm.testutils.annotations.Projection;
 
 import junit.framework.AssertionFailedError;
-import mockit.Mock;
-import mockit.MockUp;
 import relcontext.ChosenRelation;
 
@@ -108 +105 @@
         assertEquals(1, keptWay.getNodes().stream().distinct().filter(n -> "3".equals(n.get("name"))).count());
         assertEmpty(DatasetConsistencyTest.runTests(ds));
+        assertDoesNotThrow(() -> UndoRedoHandler.getInstance().undo());
+        assertEmpty(DatasetConsistencyTest.runTests(ds));
     }
 
@@ -115 +114 @@
     @Test
     void testPolygonReconstructComplex() {
-        final AtomicReference<String> shownMessage = new AtomicReference<>();
-        new MockUp<JOptionPane>() {
-            @Mock
-            void showMessageDialog(Component parentComponent,
-                                   Object message, String title, int messageType) {
-                shownMessage.set((String) message);
-            }
-        };
         final Relation otherRelation = TestUtils.newRelation("type=multipolygon landuse=retail",
                 new RelationMember("outer", way1),
@@ -131 +122 @@
         assertDoesNotThrow(() -> GuiHelper.runInEDTAndWait(() -> action.actionPerformed(null)));
         assertEmpty(DatasetConsistencyTest.runTests(ds));
-        assertEquals(tr("Multipolygon must consist only of ways with one referring relation"), shownMessage.get());
+        assertDoesNotThrow(() -> UndoRedoHandler.getInstance().undo());
+        assertEmpty(DatasetConsistencyTest.runTests(ds));
+    }
+
+    /**
+     * Ensure that we bail if a way in the relation to be simplified will be deleted from another relation.
+     */
+    @Test
+    void testPolygonReconstructDuplicate() {
+        final Relation otherRelation = TestUtils.newRelation("type=multipolygon landuse=retail",
+                new RelationMember("outer", way1),
+                new RelationMember("outer", way2),
+                new RelationMember("outer", way3));
+        ds.addPrimitiveRecursive(otherRelation);
+        assertDoesNotThrow(() -> GuiHelper.runInEDTAndWait(() -> action.actionPerformed(null)));
+        assertEmpty(DatasetConsistencyTest.runTests(ds));
+        assertDoesNotThrow(() -> UndoRedoHandler.getInstance().undo());
+        assertEmpty(DatasetConsistencyTest.runTests(ds));
+    }
+
+    @Test
+    void testPolygonReconstructR1585888() throws IOException, IllegalDataException {
+        ds.clear();
+        ds.mergeFrom(OsmReader.parseDataSet(TestUtils.getRegressionDataStream(23170, "r1585888.osm"), NullProgressMonitor.INSTANCE));
+        assertEmpty(DatasetConsistencyTest.runTests(ds));
+
+        ds.setSelected(new SimplePrimitiveId(1585888, OsmPrimitiveType.RELATION));
+        chosenRelation.set((Relation) ds.getPrimitiveById(1585888, OsmPrimitiveType.RELATION));
+        assertDoesNotThrow(() -> GuiHelper.runInEDTAndWait(() -> action.actionPerformed(null)));
+        assertEmpty(DatasetConsistencyTest.runTests(ds));
+
+        final Collection<Way> selectedWays = ds.getSelectedWays();
+        assertEquals(selectedWays.size(), ds.getSelected().size());
+        assertTrue(selectedWays.stream().allMatch(Way::isClosed));
+        assertTrue(selectedWays.stream().mapToInt(Way::getNodesCount).anyMatch(i -> i == 15));
+        assertTrue(selectedWays.stream().mapToInt(Way::getNodesCount).anyMatch(i -> i == 23));
+        assertTrue(selectedWays.stream().mapToInt(Way::getNodesCount).anyMatch(i -> i == 37));
+        assertTrue(selectedWays.stream().allMatch(way -> "residential".equals(way.get("landuse"))));
+        assertDoesNotThrow(() -> UndoRedoHandler.getInstance().undo());
+        assertEmpty(DatasetConsistencyTest.runTests(ds));
     }