Subject: [PATCH] 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 in 1k way test * Reduced memory allocations in 1k way test (>11GB to <250MB) * Reduced CPU cycles in 1k way test (16s to 2.7s) --- Index: core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java b/core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java --- a/core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java (revision 18934) +++ b/core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java (date 1704976425430) @@ -56,7 +56,6 @@ import org.openstreetmap.josm.tools.GBC; import org.openstreetmap.josm.tools.ImageProvider; import org.openstreetmap.josm.tools.Shortcut; -import org.openstreetmap.josm.tools.StreamUtils; /** * Delete unnecessary nodes from a way @@ -340,15 +339,27 @@ * @since 16566 (private) */ private static SequenceCommand buildSimplifyWaysCommand(List ways, double threshold) { - Collection allCommands = ways.stream() - .map(way -> createSimplifyCommand(way, threshold)) - .filter(Objects::nonNull) - .collect(StreamUtils.toUnmodifiableList()); + List allCommands = new ArrayList<>(ways.size()); + for (Way way : ways) { + Command command = createSimplifyCommand(way, threshold, false); + if (command != null) { + allCommands.add(command); + } + } if (allCommands.isEmpty()) return null; + final List 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()), - allCommands); + Collections.unmodifiableList(allCommands)); } /** @@ -371,6 +382,18 @@ * @since 15419 */ 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; @@ -417,7 +440,9 @@ Collection cmds = new LinkedList<>(); 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); } @@ -571,6 +596,9 @@ final List newWays = dataSet.getSelectedWays().stream() .filter(p -> !p.isIncomplete()) .collect(Collectors.toList()); + if (Objects.deepEquals(newWays, this.wayList)) { + return; + } this.wayList.clear(); this.wayList.addAll(newWays); if (this.consumer != null) { Index: core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java b/core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java --- a/core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java (revision 18934) +++ b/core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java (date 1704976425456) @@ -1,7 +1,10 @@ // License: GPL. For details, see LICENSE file. 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 java.io.IOException; @@ -24,11 +27,15 @@ import org.openstreetmap.josm.data.osm.DataSet; 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.tools.Utils; /** @@ -99,4 +106,32 @@ assertEquals(1, deleteCommands.size()); assertEquals(Collections.singleton(n1), deleteCommands.iterator().next().getParticipatingPrimitives()); } + + /** + * Non-regression test for #23399 + */ + @Test + void testNonRegression23399() { + new ExtendedDialogMocker(Collections.singletonMap("Simplify way", "Simplify")) { + @Override + protected String getString(ExtendedDialog instance) { + return instance.getTitle(); + } + }; + final ArrayList 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()); + } }