Fix duplicate ways command to work properly with undo/redo

Signed-off-by: Taylor Smock <taylor.smock@kaart.com>

src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MergeDuplicateWaysAction.java

@@ -16,7 +16,6 @@ import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
 import org.openstreetmap.josm.plugins.mapwithai.commands.MergeDuplicateWays;
-import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Shortcut;
 
 /**
@@ -51,9 +50,8 @@ public class MergeDuplicateWaysAction extends JosmAction {
             if (command != null) {
                 UndoRedoHandler.getInstance().add(command);
                 i++;
-                Logging.error(Integer.toString(i));
             }
-        } while (command != null && i < 10);
+        } while (command != null && i < 1);
     }
 
 }

src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MergeDuplicateWays.java

@@ -35,6 +35,7 @@ public class MergeDuplicateWays extends Command {
     private final Way way2;
 
     private final List<Command> commands;
+    private Command command;
 
     public MergeDuplicateWays(DataSet data) {
         this(data, null, null);
@@ -61,7 +62,7 @@ public class MergeDuplicateWays extends Command {
 
     @Override
     public boolean executeCommand() {
-        if (commands.isEmpty()) {
+        if (commands.isEmpty() || command == null) {
             if (way1 == null && way2 == null) {
                 filterDataSet(getAffectedDataSet(), commands);
             } else if (way1 != null && way2 == null) {
@@ -69,31 +70,38 @@ public class MergeDuplicateWays extends Command {
             } else if (way1 == null) {
                 checkForDuplicateWays(way2, commands);
             } else {
-                final Command command = checkForDuplicateWays(way1, way2);
+                final Command tCommand = checkForDuplicateWays(way1, way2);
-                if (command != null) {
+                if (tCommand != null) {
-                    commands.add(command);
+                    commands.add(tCommand);
-                    command.executeCommand();
+                    tCommand.executeCommand();
                 }
             }
-        } else {
+            List<Command> realCommands = commands.stream().filter(Objects::nonNull).distinct()
-            for (final Command command : commands) {
+                    .collect(Collectors.toList());
-                command.executeCommand();
+            commands.clear();
+            commands.addAll(realCommands);
+            if (!commands.isEmpty() && commands.size() != 1) {
+                command = new SequenceCommand(getDescriptionText(), commands);
+            } else if (commands.size() == 1) {
+                command = commands.get(0);
             }
+        } else {
+            command.executeCommand();
         }
         return true;
     }
 
     @Override
     public void undoCommand() {
-        for (final Command tCommand : commands) {
+        if (command != null) {
-            tCommand.undoCommand();
+            command.undoCommand();
         }
     }
 
     public static void filterDataSet(DataSet dataSet, List<Command> commands) {
         final List<Way> ways = new ArrayList<>(
                 dataSet.getWays().parallelStream().filter(prim -> !prim.isIncomplete() && !prim.isDeleted())
-                        .collect(Collectors.toList()));
+                .collect(Collectors.toList()));
         for (int i = 0; i < ways.size(); i++) {
             final Way way1 = ways.get(i);
             final Collection<Way> nearbyWays = dataSet.searchWays(way1.getBBox()).parallelStream()
@@ -208,8 +216,8 @@ public class MergeDuplicateWays extends Command {
             before.forEach(node -> newWay.addNode(0, node));
             after.forEach(newWay::addNode);
             if (newWay.getNodesCount() > 0) {
-                commands.add(new DeleteCommand(way2));
                 commands.add(new ChangeCommand(way1, newWay));
+                commands.add(DeleteCommand.delete(Collections.singleton(way2), true, true));
             }
             if (commands.contains(null)) {
                 commands = commands.stream().filter(Objects::nonNull).collect(Collectors.toList());

test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MergeDuplicateWaysTest.java

@@ -2,6 +2,12 @@
 package org.openstreetmap.josm.plugins.mapwithai.backend;
 
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.LinkedHashSet;
@@ -10,10 +16,10 @@ import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
-import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
 import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
@@ -39,9 +45,9 @@ public class MergeDuplicateWaysTest {
     public void testRemoveCommonTags() {
         DataSet ds1 = new DataSet(TestUtils.newNode("orig_id=2222 highway=secondary"));
         GetDataRunnable.removeCommonTags(ds1);
-        Assert.assertEquals(1, ds1.allPrimitives().stream().mapToInt(prim -> prim.getKeys().size()).sum());
+        assertEquals(1, ds1.allPrimitives().stream().mapToInt(prim -> prim.getKeys().size()).sum());
         GetDataRunnable.removeCommonTags(ds1);
-        Assert.assertEquals(1, ds1.allPrimitives().stream().mapToInt(prim -> prim.getKeys().size()).sum());
+        assertEquals(1, ds1.allPrimitives().stream().mapToInt(prim -> prim.getKeys().size()).sum());
     }
 
     /**
@@ -59,19 +65,19 @@ public class MergeDuplicateWaysTest {
         ds1.addPrimitive(way2);
 
         new MergeDuplicateWays(ds1).executeCommand();
-        Assert.assertFalse(way1.isDeleted());
+        assertFalse(way1.isDeleted());
-        Assert.assertFalse(way2.isDeleted());
+        assertFalse(way2.isDeleted());
-        way1.getNodes().forEach(node -> Assert.assertFalse(way2.containsNode(node)));
+        way1.getNodes().forEach(node -> assertFalse(way2.containsNode(node)));
-        way2.getNodes().forEach(node -> Assert.assertFalse(way1.containsNode(node)));
+        way2.getNodes().forEach(node -> assertFalse(way1.containsNode(node)));
 
         Node tNode = new Node(new LatLon(1, 1));
         ds1.addPrimitive(tNode);
         way1.addNode(1, tNode);
 
         new MergeDuplicateWays(ds1).executeCommand();
-        Assert.assertNotSame(way1.isDeleted(), way2.isDeleted());
+        assertNotSame(way1.isDeleted(), way2.isDeleted());
         Way tWay = way1.isDeleted() ? way2 : way1;
-        Assert.assertEquals(4, tWay.getNodesCount());
+        assertEquals(4, tWay.getNodesCount());
     }
 
     /**
@@ -79,6 +85,8 @@ public class MergeDuplicateWaysTest {
      */
     @Test
     public void testMergeWays() {
+        int undoRedoTries = 10;
+
         Way way1 = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(1, 1)));
         Way way2 = TestUtils.newWay("", new Node(new LatLon(1, 1)), new Node(new LatLon(1, 2)));
         Set<Pair<Pair<Integer, Node>, Pair<Integer, Node>>> set = new LinkedHashSet<>();
@@ -90,23 +98,32 @@ public class MergeDuplicateWaysTest {
         ds.addPrimitive(way1);
 
         // Test with one node in common
-        Assert.assertNull(MergeDuplicateWays.mergeWays(way1, way2, set));
+        assertNull(MergeDuplicateWays.mergeWays(way1, way2, set));
-        Assert.assertFalse(way2.isDeleted());
+        assertFalse(way2.isDeleted());
-        Assert.assertFalse(way1.isDeleted());
+        assertFalse(way1.isDeleted());
-        Assert.assertEquals(2, way1.getNodesCount());
+        assertEquals(2, way1.getNodesCount());
 
+        // Test with two nodes in common
         Node tNode = new Node(new LatLon(0, 0));
         ds.addPrimitive(tNode);
         way2.addNode(0, tNode);
         set.clear(); // we can't use the last pair added
         set.add(new Pair<>(new Pair<>(0, way1.firstNode()), new Pair<>(0, way2.firstNode())));
         set.add(new Pair<>(new Pair<>(1, way1.lastNode()), new Pair<>(1, way2.getNode(1))));
-        MergeDuplicateWays.mergeWays(way1, way2, set).executeCommand();
+        Command command = MergeDuplicateWays.mergeWays(way1, way2, set);
-
+        for (int i = 0; i < undoRedoTries; i++) {
-        Assert.assertTrue(way2.isDeleted());
+            command.executeCommand();
-        Assert.assertFalse(way1.isDeleted());
+            assertTrue(way2.isDeleted());
-        Assert.assertEquals(3, way1.getNodesCount());
+            assertFalse(way1.isDeleted());
+            assertEquals(3, way1.getNodesCount());
+            command.undoCommand();
+            assertFalse(way2.isDeleted());
+            assertFalse(way1.isDeleted());
+            assertEquals(2, way1.getNodesCount());
+        }
+        command.executeCommand();
 
+        // Test with a reversed way
         way1 = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(1, 1)));
         way2 = TestUtils.newWay("", new Node(new LatLon(1, 1)), new Node(new LatLon(1, 2)));
         way2.addNode(0, new Node(new LatLon(0, 0)));
@@ -121,12 +138,21 @@ public class MergeDuplicateWaysTest {
         set.clear();
         set.add(new Pair<>(new Pair<>(0, way1.firstNode()), new Pair<>(2, way2.lastNode())));
         set.add(new Pair<>(new Pair<>(1, way1.lastNode()), new Pair<>(1, way2.getNode(1))));
-        MergeDuplicateWays.mergeWays(way1, way2, set).executeCommand();
 
-        Assert.assertTrue(way2.isDeleted());
+        command = MergeDuplicateWays.mergeWays(way1, way2, set);
-        Assert.assertFalse(way1.isDeleted());
+        for (int i = 0; i < undoRedoTries; i++) {
-        Assert.assertEquals(3, way1.getNodesCount());
+            command.executeCommand();
+            assertTrue(way2.isDeleted());
+            assertFalse(way1.isDeleted());
+            assertEquals(3, way1.getNodesCount());
 
+            command.undoCommand();
+            assertFalse(way2.isDeleted());
+            assertFalse(way1.isDeleted());
+            assertEquals(2, way1.getNodesCount());
+        }
+
+        // Test that nodes on both sides get added
         way1 = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(1, 1)));
         way2 = TestUtils.newWay("", new Node(new LatLon(1, 1)), new Node(new LatLon(1, 2)));
         way2.addNode(0, new Node(new LatLon(0, 0)));
@@ -140,13 +166,20 @@ public class MergeDuplicateWaysTest {
         set.add(new Pair<>(new Pair<>(0, way1.firstNode()), new Pair<>(2, way2.getNode(2))));
         set.add(new Pair<>(new Pair<>(1, way1.lastNode()), new Pair<>(3, way2.getNode(3))));
         List<Node> currentWay2Nodes = way2.getNodes();
-        MergeDuplicateWays.mergeWays(way1, way2, set).executeCommand();
+        command = MergeDuplicateWays.mergeWays(way1, way2, set);
 
-        Assert.assertTrue(way2.isDeleted());
+        for (int i = 0; i < undoRedoTries; i++) {
-        Assert.assertFalse(way1.isDeleted());
+            command.executeCommand();
-        Assert.assertEquals(4, way1.getNodesCount());
+            assertTrue(way2.isDeleted());
-        Assert.assertEquals(currentWay2Nodes.get(0), way1.firstNode());
+            assertFalse(way1.isDeleted());
-        Assert.assertEquals(currentWay2Nodes.get(1), way1.getNode(1));
+            assertEquals(4, way1.getNodesCount());
+            assertEquals(currentWay2Nodes.get(0), way1.firstNode());
+            assertEquals(currentWay2Nodes.get(1), way1.getNode(1));
+            command.undoCommand();
+            assertFalse(way2.isDeleted());
+            assertFalse(way1.isDeleted());
+            assertEquals(2, way1.getNodesCount());
+        }
     }
 
     /**
@@ -162,18 +195,18 @@ public class MergeDuplicateWaysTest {
         set.add(pair1);
         set.add(pair2);
 
-        Assert.assertTrue(MergeDuplicateWays.checkDirection(set));
+        assertTrue(MergeDuplicateWays.checkDirection(set));
         pair1.a.a = pair1.a.a - 1;
-        Assert.assertTrue(MergeDuplicateWays.checkDirection(set));
+        assertTrue(MergeDuplicateWays.checkDirection(set));
         pair1.a.a = pair1.a.a + 3;
-        Assert.assertFalse(MergeDuplicateWays.checkDirection(set));
+        assertFalse(MergeDuplicateWays.checkDirection(set));
         pair1.a.a = pair1.a.a - 2;
 
-        Assert.assertTrue(MergeDuplicateWays.checkDirection(set));
+        assertTrue(MergeDuplicateWays.checkDirection(set));
         pair1.b.a = pair1.b.a - 1;
-        Assert.assertTrue(MergeDuplicateWays.checkDirection(set));
+        assertTrue(MergeDuplicateWays.checkDirection(set));
         pair1.b.a = pair1.b.a + 3;
-        Assert.assertFalse(MergeDuplicateWays.checkDirection(set));
+        assertFalse(MergeDuplicateWays.checkDirection(set));
         pair1.b.a = pair1.b.a - 2;
     }
 
@@ -183,14 +216,14 @@ public class MergeDuplicateWaysTest {
     @Test
     public void testSorted() {
         List<Integer> integerList = Arrays.asList(1, 2, 3, 4, 6, 7, 8, 9, 5);
-        Assert.assertFalse(MergeDuplicateWays.sorted(integerList));
+        assertFalse(MergeDuplicateWays.sorted(integerList));
         integerList = integerList.stream().sorted().collect(Collectors.toList());
-        Assert.assertTrue(MergeDuplicateWays.sorted(integerList));
+        assertTrue(MergeDuplicateWays.sorted(integerList));
         integerList.remove(3);
-        Assert.assertFalse(MergeDuplicateWays.sorted(integerList));
+        assertFalse(MergeDuplicateWays.sorted(integerList));
 
         integerList = Arrays.asList(1);
-        Assert.assertTrue(MergeDuplicateWays.sorted(integerList));
+        assertTrue(MergeDuplicateWays.sorted(integerList));
     }
 
     /**
@@ -202,27 +235,27 @@ public class MergeDuplicateWaysTest {
         Way way2 = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(1, 1)));
 
         Map<Pair<Integer, Node>, Map<Integer, Node>> duplicateNodes = MergeDuplicateWays.getDuplicateNodes(way1, way2);
-        Assert.assertEquals(2, duplicateNodes.size());
+        assertEquals(2, duplicateNodes.size());
-        Assert.assertEquals(2, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
+        assertEquals(2, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
 
         way2.addNode(new Node(new LatLon(0, 0)));
 
         duplicateNodes = MergeDuplicateWays.getDuplicateNodes(way1, way2);
-        Assert.assertEquals(2, duplicateNodes.size());
+        assertEquals(2, duplicateNodes.size());
-        Assert.assertEquals(3, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
+        assertEquals(3, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
 
         way2.addNode(way2.firstNode());
 
         duplicateNodes = MergeDuplicateWays.getDuplicateNodes(way1, way2);
-        Assert.assertEquals(2, duplicateNodes.size());
+        assertEquals(2, duplicateNodes.size());
-        Assert.assertEquals(4, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
+        assertEquals(4, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
 
         way2.setNodes(way2.getNodes().stream().limit(2).collect(Collectors.toList()));
         way2.addNode(new Node(new LatLon(2, 2)));
 
         duplicateNodes = MergeDuplicateWays.getDuplicateNodes(way1, way2);
-        Assert.assertEquals(2, duplicateNodes.size());
+        assertEquals(2, duplicateNodes.size());
-        Assert.assertEquals(2, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
+        assertEquals(2, duplicateNodes.values().stream().flatMap(col -> col.keySet().stream()).count());
     }
 
 }