Remove a custom delete command, and fix the related test issues

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

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

@@ -7,8 +7,10 @@ import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 import java.util.TreeSet;
 import java.util.stream.Collectors;
 
@@ -27,7 +29,7 @@ import org.openstreetmap.josm.tools.Utils;
 public class CreateConnectionsCommand extends Command {
     private final Collection<OsmPrimitive> primitives;
     private Command command;
-    private static final List<Class<? extends AbstractConflationCommand>> CONFLATION_COMMANDS = new ArrayList<>();
+    private static final LinkedHashSet<Class<? extends AbstractConflationCommand>> CONFLATION_COMMANDS = new LinkedHashSet<>();
     static {
         CONFLATION_COMMANDS.add(ConnectedCommand.class);
         CONFLATION_COMMANDS.add(DuplicateCommand.class);
@@ -125,7 +127,15 @@ public class CreateConnectionsCommand extends Command {
     /**
      * @return A set of commands to run when copying data from the MapWithAI layer
      */
-    public static List<Class<? extends AbstractConflationCommand>> getConflationCommands() {
+    public static Set<Class<? extends AbstractConflationCommand>> getConflationCommands() {
-        return Collections.unmodifiableList(CONFLATION_COMMANDS);
+        return Collections.unmodifiableSet(CONFLATION_COMMANDS);
+    }
+
+    /**
+     * @return {@code true} if the conflation command was removed and was present
+     * @see List#remove
+     */
+    public static boolean removeConflationCommand(Class<? extends AbstractConflationCommand> command) {
+        return CONFLATION_COMMANDS.remove(command);
     }
 }

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

@@ -1,90 +0,0 @@
-package org.openstreetmap.josm.plugins.mapwithai.commands;
-
-import static org.openstreetmap.josm.tools.I18n.tr;
-
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-import java.util.stream.Collectors;
-
-import org.openstreetmap.josm.command.Command;
-import org.openstreetmap.josm.data.osm.DataSet;
-import org.openstreetmap.josm.data.osm.OsmPrimitive;
-import org.openstreetmap.josm.data.osm.Way;
-
-public class DeletePrimitivesCommand extends Command {
-    private final Collection<OsmPrimitive> selection;
-    private final DataSet from;
-    private final Collection<OsmPrimitive> removed;
-    private final Collection<Command> commands;
-    private final boolean deleteChildren;
-
-    /**
-     * Delete primitives from a DataSet
-     *
-     * @param from      The {@link DataSet} to remove primitives from
-     * @param selection The primitives to remove
-     */
-    public DeletePrimitivesCommand(DataSet from, Collection<OsmPrimitive> selection) {
-        this(from, selection, false);
-    }
-
-    /**
-     * Delete primitives from a DataSet
-     *
-     * @param from      The {@link DataSet} to remove primitives from
-     * @param selection The primitives to remove
-     * @param removeAll {@code true} if all children should be removed (for ways).
-     */
-    public DeletePrimitivesCommand(DataSet from, Collection<OsmPrimitive> selection, boolean removeAll) {
-        super(from);
-        this.from = from;
-        this.selection = selection;
-        commands = new ArrayList<>();
-        removed = new ArrayList<>();
-        this.deleteChildren = removeAll;
-    }
-
-    @Override
-    public boolean executeCommand() {
-        for (final OsmPrimitive primitive : selection) {
-            primitive.setDeleted(true);
-            removed.add(primitive);
-            if (primitive instanceof Way) {
-                final List<OsmPrimitive> nodes = ((Way) primitive).getNodes().stream()
-                        .filter(node -> (!node.hasKeys() || deleteChildren) && node.getParentWays().isEmpty())
-                        .collect(Collectors.toList());
-                final DeletePrimitivesCommand delNodes = new DeletePrimitivesCommand(from, nodes);
-                delNodes.executeCommand();
-                commands.add(delNodes);
-            }
-        }
-        return true;
-    }
-
-    @Override
-    public void undoCommand() {
-        for (final Command command : commands) {
-            command.undoCommand();
-        }
-        for (final OsmPrimitive primitive : removed) {
-            primitive.setDeleted(false);
-        }
-        removed.clear();
-    }
-
-    @Override
-    public String getDescriptionText() {
-        return tr("Remove primitives from data set");
-    }
-
-    @Override
-    public void fillModifiedData(Collection<OsmPrimitive> modified, Collection<OsmPrimitive> deleted,
-            Collection<OsmPrimitive> added) {
-        for (final Command command : commands) {
-            command.fillModifiedData(modified, deleted, added);
-        }
-        deleted.addAll(removed);
-    }
-
-}

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

@@ -4,13 +4,18 @@ package org.openstreetmap.josm.plugins.mapwithai.commands;
 import static org.openstreetmap.josm.tools.I18n.tr;
 import static org.openstreetmap.josm.tools.I18n.trn;
 
+import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 import org.openstreetmap.josm.command.AddPrimitivesCommand;
+import org.openstreetmap.josm.command.ChangePropertyCommand;
 import org.openstreetmap.josm.command.Command;
+import org.openstreetmap.josm.command.DeleteCommand;
 import org.openstreetmap.josm.command.SequenceCommand;
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
@@ -18,6 +23,7 @@ import org.openstreetmap.josm.data.osm.PrimitiveData;
 import org.openstreetmap.josm.data.osm.visitor.MergeSourceBuildingVisitor;
 import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
 import org.openstreetmap.josm.plugins.mapwithai.backend.GetDataRunnable;
+import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAIDataUtils;
 import org.openstreetmap.josm.tools.Logging;
 
 /**
@@ -68,7 +74,22 @@ public class MovePrimitiveDataSetCommand extends Command {
 
         commands.add(new AddPrimitivesCommand(primitiveAddData,
                 selection.stream().map(OsmPrimitive::save).collect(Collectors.toList()), to));
-        commands.add(new DeletePrimitivesCommand(from, selection, true));
+        List<Command> removeKeyCommand = new ArrayList<>();
+        Set<OsmPrimitive> fullSelection = new HashSet<>();
+        MapWithAIDataUtils.addPrimitivesToCollection(fullSelection, selection);
+        CreateConnectionsCommand.getConflationCommands().forEach(clazz -> {
+            try {
+                removeKeyCommand.add(new ChangePropertyCommand(fullSelection,
+                        clazz.getConstructor(DataSet.class).newInstance(from).getKey(), null));
+            } catch (InstantiationException | IllegalAccessException | IllegalArgumentException
+                    | InvocationTargetException | NoSuchMethodException | SecurityException e) {
+                Logging.error(e);
+            }
+        });
+        SequenceCommand sequence = new SequenceCommand("Temporary Command", removeKeyCommand);
+        sequence.executeCommand(); // This *must* be executed for the delete command to get everything.
+        commands.add(DeleteCommand.delete(selection, true, true));
+        sequence.undoCommand();
 
         return new SequenceCommand(trn("Move {0} OSM Primitive between data sets",
                 "Move {0} OSM Primitives between data sets", selection.size(), selection.size()), commands);

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

@@ -99,7 +99,7 @@ public class MapWithAIMoveActionTest {
         assertFalse(way2.lastNode().hasKey(ConnectedCommand.CONN_KEY), "The conn key should have been removed");
         assertFalse(way2.firstNode().hasKey(ConnectedCommand.CONN_KEY), "The conn key should have been removed");
         assertFalse(way2.getNode(1).hasKey(ConnectedCommand.CONN_KEY), "The conn key should have been removed");
-        assertTrue(way1.lastNode().isDeleted(), "way1 should be deleted when added");
+        assertTrue(way1.isDeleted(), "way1 should be deleted when added");
 
         UndoRedoHandler.getInstance().undo();
         assertFalse(way2.lastNode().hasKey(ConnectedCommand.CONN_KEY), "The conn key shouldn't exist");

test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommandTest.java

@@ -1,75 +0,0 @@
-// License: GPL. For details, see LICENSE file.
-package org.openstreetmap.josm.plugins.mapwithai.commands;
-
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
-import java.util.Collections;
-
-import org.junit.Rule;
-import org.junit.Test;
-import org.openstreetmap.josm.TestUtils;
-import org.openstreetmap.josm.data.coor.LatLon;
-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.testutils.JOSMTestRules;
-
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
-
-public class DeletePrimitivesCommandTest {
-    @Rule
-    @SuppressFBWarnings("URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules();
-
-    @Test
-    public void testDeletePrimitives() {
-        final DataSet ds = new DataSet();
-        final Way way1 = TestUtils.newWay("highway=residential", new Node(new LatLon(0, 0)),
-                new Node(new LatLon(-0.1, 0.1)));
-        way1.getNodes().forEach(node -> ds.addPrimitive(node));
-        ds.addPrimitive(way1);
-
-        DeletePrimitivesCommand delete = new DeletePrimitivesCommand(ds, Collections.singleton(way1));
-        delete.executeCommand();
-        assertTrue(way1.isDeleted(), "The way should be deleted");
-        assertEquals(0, ds.allNonDeletedPrimitives().size(), "There should be no non-deleted primitives");
-
-        delete.undoCommand();
-
-        assertTrue(ds.containsWay(way1), "The DataSet should contain way1");
-        assertEquals(3, ds.allNonDeletedPrimitives().size(), "There should be three non-deleted primitives");
-
-        final Node tNode = new Node(new LatLon(0.1, 0.1));
-        ds.addPrimitive(tNode);
-
-        delete.executeCommand();
-        assertTrue(way1.isDeleted(), "The way should be deleted");
-        assertEquals(1, ds.allNonDeletedPrimitives().size(), "Non-relevant objects should not be affected");
-
-        delete.undoCommand();
-        assertEquals(4, ds.allNonDeletedPrimitives().size(), "The DataSet should be as it was");
-
-        way1.firstNode().put("highway", "stop");
-
-        delete.executeCommand();
-        assertTrue(way1.isDeleted(), "The way should be deleted");
-        assertEquals(2, ds.allNonDeletedPrimitives().size(), "Objects with their own keys should remain");
-
-        delete.undoCommand();
-        assertTrue(way1.firstNode().hasKey("highway"), "Objects should not lose their keys");
-
-        delete = new DeletePrimitivesCommand(ds, Collections.singleton(way1), true);
-
-        delete.executeCommand();
-        assertTrue(way1.isDeleted(), "The way should be deleted");
-        assertEquals(1, ds.allNonDeletedPrimitives().size(),
-                "All nodes in the way not in another way should be removed");
-
-        delete.undoCommand();
-        assertEquals(4, ds.allNonDeletedPrimitives().size(), "The DataSet should be as it was");
-
-        assertTrue(way1.firstNode().hasKey("highway"), "Objects should not lose their keys");
-    }
-
-}

test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java

@@ -9,6 +9,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.io.File;
 import java.util.Arrays;
 import java.util.Collections;
 
@@ -17,6 +18,7 @@ import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.actions.SaveAction;
 import org.openstreetmap.josm.command.MoveCommand;
 import org.openstreetmap.josm.data.UndoRedoHandler;
 import org.openstreetmap.josm.data.coor.LatLon;
@@ -25,6 +27,10 @@ import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 import org.openstreetmap.josm.data.osm.Tag;
 import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.data.validation.tests.SharpAngles;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
+import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
+import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.ConnectedCommand;
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
@@ -75,8 +81,6 @@ public class MapWithAIAddComandTest {
         command.executeCommand();
         for (Way way : Arrays.asList(way1, way2)) {
             assertNotNull(ds2.getPrimitiveById(way), "DataSet should still contain object");
-            assertNotNull(ds2.getPrimitiveById(way.firstNode()), "DataSet should still contain object");
-            assertNotNull(ds2.getPrimitiveById(way.lastNode()), "DataSet should still contain object");
             assertTrue(way.isDeleted(), "The way should be deleted");
         }
 
@@ -84,14 +88,10 @@ public class MapWithAIAddComandTest {
         command = new MapWithAIAddCommand(ds1, ds2, Arrays.asList(way3));
         command.executeCommand();
         assertNotNull(ds2.getPrimitiveById(way3), "DataSet should still contain object");
-        assertNotNull(ds2.getPrimitiveById(way3.firstNode()), "DataSet should still contain object");
-        assertNotNull(ds2.getPrimitiveById(way3.lastNode()), "DataSet should still contain object");
         assertTrue(way3.isDeleted(), "The way should be deleted");
 
         command.undoCommand();
         assertNull(ds2.getPrimitiveById(way3), "DataSet should no longer contain object");
-        assertNull(ds2.getPrimitiveById(way3.firstNode()), "DataSet should no longer contain object");
-        assertNull(ds2.getPrimitiveById(way3.lastNode()), "DataSet should no longer contain object");
         assertFalse(ds1.getPrimitiveById(way3).isDeleted(),
                 "The way should no longer be deleted in its original DataSet");
     }
@@ -174,9 +174,11 @@ public class MapWithAIAddComandTest {
         from.addPrimitive(way1);
         from.addPrimitive(new Node(new LatLon(-0.1, 0.1)));
 
+        final Node way1FirstNode = way1.firstNode();
+
         UndoRedoHandler.getInstance().add(new MapWithAIAddCommand(from, to, Collections.singleton(way1)));
 
-        final Node tNode = (Node) to.getPrimitiveById(way1.firstNode());
+        final Node tNode = (Node) to.getPrimitiveById(way1FirstNode);
 
         UndoRedoHandler.getInstance().add(new MoveCommand(tNode, LatLon.ZERO));
 
@@ -215,4 +217,51 @@ public class MapWithAIAddComandTest {
         assertNotNull(osmData.getPrimitiveById(176220609, OsmPrimitiveType.NODE));
         assertNotNull(osmData.getPrimitiveById(way));
     }
+
+    @Test
+    public void testMultiConnectionsSimultaneous() {
+        SharpAngles test = new SharpAngles();
+        Way way1 = TestUtils.newWay("highway=residential", new Node(new LatLon(3.4186753, 102.0559126)),
+                new Node(new LatLon(3.4185682, 102.0555264)));
+        Way way2 = TestUtils.newWay("highway=residential", new Node(new LatLon(3.4186271, 102.0559502)),
+                new Node(new LatLon(3.4188681, 102.0562935)));
+        Way original = TestUtils.newWay("highway=tertiary", new Node(new LatLon(3.4185368, 102.0560268)),
+                new Node(new LatLon(3.4187717, 102.0558451)));
+        String connectedValue = "w" + Long.toString(original.getUniqueId()) + ",n"
+                + Long.toString(original.firstNode().getUniqueId()) + ",n"
+                + Long.toString(original.lastNode().getUniqueId());
+        way1.firstNode().put(ConnectedCommand.CONN_KEY, connectedValue);
+        way2.firstNode().put(ConnectedCommand.CONN_KEY, connectedValue);
+
+        DataSet ds = new DataSet();
+        DataSet osmData = new DataSet();
+        OsmDataLayer layer = new OsmDataLayer(osmData, "Test Layer", null);
+        for (Way way : Arrays.asList(way1, way2)) {
+            way.getNodes().parallelStream().filter(node -> node.getDataSet() == null).forEach(ds::addPrimitive);
+            if (way.getDataSet() == null) {
+                ds.addPrimitive(way);
+            }
+        }
+        original.getNodes().forEach(osmData::addPrimitive);
+        osmData.addPrimitive(original);
+
+        MapWithAIAddCommand connectionsCommand = new MapWithAIAddCommand(ds, osmData, ds.allPrimitives());
+        connectionsCommand.executeCommand();
+        test.startTest(NullProgressMonitor.INSTANCE);
+        test.visit(ds.allPrimitives());
+        test.endTest();
+        assertTrue(test.getErrors().isEmpty());
+
+        SaveAction.doSave(layer, new File("post_command1.osm"), false);
+        connectionsCommand.undoCommand();
+
+        connectionsCommand = new MapWithAIAddCommand(ds, osmData, ds.allPrimitives());
+        connectionsCommand.executeCommand();
+        test.startTest(NullProgressMonitor.INSTANCE);
+        test.visit(osmData.allPrimitives());
+        test.endTest();
+        assertTrue(test.getErrors().isEmpty());
+        SaveAction.doSave(layer, new File("post_command2.osm"), false);
+    }
+
 }

test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MovePrimitiveDataSetCommandTest.java

@@ -17,7 +17,6 @@ import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Way;
-import org.openstreetmap.josm.plugins.mapwithai.commands.MovePrimitiveDataSetCommand;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@@ -46,7 +45,7 @@ public class MovePrimitiveDataSetCommandTest {
 
         move.executeCommand();
         move.fillModifiedData(modified, deleted, added);
-        Assert.assertEquals(3, deleted.size());
+        Assert.assertEquals(0, deleted.size());
         Assert.assertEquals(0, added.size()); // the JOSM Add command doesn't add to this list
         Assert.assertEquals(0, modified.size());
         Assert.assertEquals(1, from.allNonDeletedPrimitives().size());
@@ -83,7 +82,7 @@ public class MovePrimitiveDataSetCommandTest {
         deleted.clear();
         move.executeCommand();
         move.fillModifiedData(modified, deleted, added);
-        Assert.assertEquals(3, deleted.size());
+        Assert.assertEquals(0, deleted.size());
         Assert.assertEquals(0, added.size()); // the JOSM Add command doesn't add to this list
         Assert.assertEquals(0, modified.size());
         Assert.assertEquals(1, from.allNonDeletedPrimitives().size());
@@ -135,9 +134,11 @@ public class MovePrimitiveDataSetCommandTest {
         from.addPrimitive(way1);
         from.addPrimitive(new Node(new LatLon(-0.1, 0.1)));
 
+        final Node way1Node1 = way1.firstNode();
+
         UndoRedoHandler.getInstance().add(new MovePrimitiveDataSetCommand(to, from, Collections.singleton(way1)));
 
-        final Node tNode = (Node) to.getPrimitiveById(way1.firstNode());
+        final Node tNode = (Node) to.getPrimitiveById(way1Node1);
 
         UndoRedoHandler.getInstance().add(new MoveCommand(tNode, LatLon.ZERO));
 
@@ -158,7 +159,9 @@ public class MovePrimitiveDataSetCommandTest {
 
     @Test
     public void testDescription() {
-        Assert.assertNotNull(new MovePrimitiveDataSetCommand(new DataSet(), new DataSet(), Collections.emptyList())
+        Node tNode = new Node(new LatLon(0, 0));
+        DataSet from = new DataSet(tNode);
+        Assert.assertNotNull(new MovePrimitiveDataSetCommand(new DataSet(), from, Collections.singleton(tNode))
                 .getDescriptionText());
     }
 }