Ticket #23399: 23399.2.patch

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

@@ -56 +56 @@
 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 +339 @@
      * @since 16566 (private)
      */
     private static SequenceCommand buildSimplifyWaysCommand(List<Way> ways, double threshold) {
-        Collection<Command> allCommands = ways.stream()
-                .map(way -> createSimplifyCommand(way, threshold))
-                .filter(Objects::nonNull)
-                .collect(StreamUtils.toUnmodifiableList());
+        List<Command> 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<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()),
-                allCommands);
+                Collections.unmodifiableList(allCommands));
     }
 
     /**
@@ -371 +382 @@
      * @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 +440 @@
         Collection<Command> 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);
     }
@@ -564 +589 @@
 
         @Override
         public void selectionChanged(SelectionChangeEvent event) {
-            updateWayList(event.getSource());
+            if (false) updateWayList(event.getSource());
         }
 
         void updateWayList(DataSet dataSet) {

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

@@ -1 +1 @@
 // 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 static org.junit.jupiter.api.Assumptions.assumeTrue;
 
+import java.awt.GraphicsEnvironment;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Paths;
@@ -24 +29 @@
 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.testutils.mockers.HelpAwareOptionPaneMocker;
 import org.openstreetmap.josm.tools.Utils;
 
 /**
@@ -99 +109 @@
         assertEquals(1, deleteCommands.size());
         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("The selection contains 1 000 ways. Are you sure you want to simplify them all?", "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());
+    }
 }