Changeset 18935 in josm for trunk

Timestamp:

2024-01-15T17:20:18+01:00 (11 months ago)

Author:

taylor.smock

Message:

Fix #23399: Simplify way crashes

This was primarily caused by selection updates occurring while the commands were generated. The fixes could be any of the following:

• Synchronized collections
• New list
• Avoiding selection updates

The last option was taken for the following reasons:

• Avoiding a StackOverflow exception when many ways are being simplified
• Reduced memory allocations (>11GB to <250MB)
• Reduced CPU cycles

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/SimplifyWayAction.java (modified) (3 diffs)
• test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java (modified) (3 diffs)

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

@@ -341 +341 @@
      */
     private static SequenceCommand buildSimplifyWaysCommand(List<Way> ways, double threshold) {
-        Collection<Command> allCommands = ways.stream()
-                .map(way -> createSimplifyCommand(way, threshold))
+        List<Command> allCommands = ways.stream()
+                .map(way -> createSimplifyCommand(way, threshold, false))
                 .filter(Objects::nonNull)
                 .collect(StreamUtils.toUnmodifiableList());
         if (allCommands.isEmpty())
             return null;
+        final List<OsmPrimitive> deletedPrimitives = allCommands.stream()
+                .map(Command::getChildren)
+                .flatMap(Collection::stream)
+                .filter(DeleteCommand.class::isInstance)
+                .map(DeleteCommand.class::cast)
+                .map(DeleteCommand::getParticipatingPrimitives)
+                .flatMap(Collection::stream)
+                .collect(Collectors.toList());
+        allCommands.get(0).getAffectedDataSet().clearSelection(deletedPrimitives);
         return new SequenceCommand(
                 trn("Simplify {0} way", "Simplify {0} ways", allCommands.size(), allCommands.size()),
@@ -372 +381 @@
      */
     public static SequenceCommand createSimplifyCommand(Way w, double threshold) {
+        return createSimplifyCommand(w, threshold, true);
+    }
+
+    /**
+     * Creates the SequenceCommand to simplify a way with a given threshold.
+     *
+     * @param w the way to simplify
+     * @param threshold the max error threshold
+     * @param deselect {@code true} if we want to deselect the deleted nodes
+     * @return The sequence of commands to run
+     */
+    private static SequenceCommand createSimplifyCommand(Way w, double threshold, boolean deselect) {
         int lower = 0;
         int i = 0;
@@ -418 +439 @@
         cmds.add(new ChangeNodesCommand(w, newNodes));
         cmds.add(new DeleteCommand(w.getDataSet(), delNodes));
-        w.getDataSet().clearSelection(delNodes);
+        if (deselect) {
+            w.getDataSet().clearSelection(delNodes);
+        }
         return new SequenceCommand(
                 trn("Simplify Way (remove {0} node)", "Simplify Way (remove {0} nodes)", delNodes.size(), delNodes.size()), cmds);

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

@@ -2 +2 @@
 package org.openstreetmap.josm.actions;
 
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 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.openstreetmap.josm.tools.I18n.tr;
 
 import java.io.IOException;
@@ -25 +29 @@
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.gui.ExtendedDialog;
 import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
+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 org.openstreetmap.josm.testutils.mockers.ExtendedDialogMocker;
+import org.openstreetmap.josm.testutils.mockers.HelpAwareOptionPaneMocker;
 import org.openstreetmap.josm.tools.Utils;
 
@@ -100 +109 @@
         assertEquals(Collections.singleton(n1), deleteCommands.iterator().next().getParticipatingPrimitives());
     }
+
+    /**
+     * Non-regression test for #23399
+     */
+    @Test
+    void testNonRegression23399() {
+        TestUtils.assumeWorkingJMockit();
+        new ExtendedDialogMocker(Collections.singletonMap("Simplify way", "Simplify")) {
+            @Override
+            protected String getString(ExtendedDialog instance) {
+                return instance.getTitle();
+            }
+        };
+        new HelpAwareOptionPaneMocker(Collections.singletonMap(
+                tr("The selection contains {0} ways. Are you sure you want to simplify them all?", 1000), "Yes"));
+        final ArrayList<Way> ways = new ArrayList<>(1000);
+        final DataSet ds = new DataSet();
+        for (int i = 0; i < 1000; i++) {
+            final Way way = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(0, 0.001)),
+                    new Node(new LatLon(0, 0.002)));
+            ways.add(way);
+            ds.addPrimitiveRecursive(way);
+        }
+        MainApplication.getLayerManager().addLayer(new OsmDataLayer(ds, "SimplifyWayActionTest#testNonRegression23399", null));
+        GuiHelper.runInEDTAndWait(() -> ds.setSelected(ds.allPrimitives()));
+        assertEquals(ds.allPrimitives().size(), ds.getAllSelected().size());
+        assertDoesNotThrow(() -> GuiHelper.runInEDTAndWaitWithException(() -> action.actionPerformed(null)));
+        assertAll(ways.stream().map(way -> () -> assertEquals(2, way.getNodesCount())));
+        assertAll(ds.getAllSelected().stream().map(p -> () -> assertFalse(p.isDeleted())));
+        assertEquals(3000, ds.getAllSelected().size());
+    }
 }