From ff20cb491dd532d1b33624178710f36a53344589 Mon Sep 17 00:00:00 2001 From: Taylor Smock Date: Mon, 23 Dec 2019 15:42:06 -0700 Subject: [PATCH] Remove a custom delete command, and fix the related test issues Signed-off-by: Taylor Smock --- .../commands/CreateConnectionsCommand.java | 16 +++- .../commands/DeletePrimitivesCommand.java | 90 ------------------- .../commands/MovePrimitiveDataSetCommand.java | 23 ++++- .../backend/MapWithAIMoveActionTest.java | 2 +- .../commands/DeletePrimitivesCommandTest.java | 75 ---------------- .../commands/MapWithAIAddComandTest.java | 63 +++++++++++-- .../MovePrimitiveDataSetCommandTest.java | 13 +-- 7 files changed, 100 insertions(+), 182 deletions(-) delete mode 100644 src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommand.java delete mode 100644 test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommandTest.java diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/CreateConnectionsCommand.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/CreateConnectionsCommand.java index b2d4631..f13674e 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/CreateConnectionsCommand.java +++ b/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 primitives; private Command command; - private static final List> CONFLATION_COMMANDS = new ArrayList<>(); + private static final LinkedHashSet> 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> getConflationCommands() { - return Collections.unmodifiableList(CONFLATION_COMMANDS); + public static Set> getConflationCommands() { + 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 command) { + return CONFLATION_COMMANDS.remove(command); } } diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommand.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommand.java deleted file mode 100644 index ef80070..0000000 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommand.java +++ /dev/null @@ -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 selection; - private final DataSet from; - private final Collection removed; - private final Collection 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 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 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 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 modified, Collection deleted, - Collection added) { - for (final Command command : commands) { - command.fillModifiedData(modified, deleted, added); - } - deleted.addAll(removed); - } - -} diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MovePrimitiveDataSetCommand.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MovePrimitiveDataSetCommand.java index 34ac7fa..e747406 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MovePrimitiveDataSetCommand.java +++ b/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 removeKeyCommand = new ArrayList<>(); + Set 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); diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveActionTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveActionTest.java index eca3199..0330598 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveActionTest.java +++ b/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"); diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommandTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommandTest.java deleted file mode 100644 index e8012e7..0000000 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/DeletePrimitivesCommandTest.java +++ /dev/null @@ -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"); - } - -} diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java index 5196480..df6960c 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java +++ b/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); + } + } diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MovePrimitiveDataSetCommandTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MovePrimitiveDataSetCommandTest.java index 8e2d078..f133170 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MovePrimitiveDataSetCommandTest.java +++ b/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()); } }