From c7422e7dcf5e95938aea59a4a636ad4fc5cdff34 Mon Sep 17 00:00:00 2001 From: Taylor Smock Date: Thu, 14 May 2020 10:08:14 -0600 Subject: [PATCH] Better client-side conflation Instead of performing missing conflation in a parallel code path, we now add the appropriate tags prior to the rest of the conflation taking place. Signed-off-by: Taylor Smock --- .../conflation/AbstractConflationCommand.java | 19 +---- .../cleanup/MissingConnectionTags.java | 58 +++++++++++---- .../commands/CreateConnectionsCommand.java | 6 +- .../backend/MapWithAIMoveActionTest.java | 30 +++----- .../conflation/ConnectedCommandTest.java | 5 ++ .../conflation/DuplicateCommandTest.java | 4 +- .../CreateConnectionsCommandTest.java | 5 ++ .../commands/MapWithAIAddComandTest.java | 17 +++-- .../cleanup/MissingConnectionTagsTest.java | 32 +++++--- .../MissingConnectionTagsMocker.java | 74 +++++++++++++++++++ 10 files changed, 179 insertions(+), 71 deletions(-) create mode 100644 test/unit/org/openstreetmap/josm/plugins/mapwithai/testutils/MissingConnectionTagsMocker.java diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/AbstractConflationCommand.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/AbstractConflationCommand.java index e692b7e..5f019db 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/AbstractConflationCommand.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/AbstractConflationCommand.java @@ -27,7 +27,6 @@ import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.gui.progress.NullProgressMonitor; import org.openstreetmap.josm.gui.progress.ProgressMonitor; import org.openstreetmap.josm.gui.progress.swing.PleaseWaitProgressMonitor; -import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin; import org.openstreetmap.josm.tools.Pair; public abstract class AbstractConflationCommand extends Command { @@ -83,22 +82,10 @@ public abstract class AbstractConflationCommand extends Command { final OsmPrimitive[] primitiveConnections = new OsmPrimitive[connections.length]; for (int i = 0; i < connections.length; i++) { final String member = connections[i]; - final char firstChar = member.charAt(0); - OsmPrimitiveType type = null; - if (firstChar == 'w') { - type = OsmPrimitiveType.WAY; - } else if (firstChar == 'n') { - type = OsmPrimitiveType.NODE; - } else if (firstChar == 'r') { - type = OsmPrimitiveType.RELATION; - } else { - throw new IllegalArgumentException( - tr("{0}: We don't know how to handle {1} types", MapWithAIPlugin.NAME, firstChar)); - } - final long id = Long.parseLong(member.substring(1)); - primitiveConnections[i] = dataSet.getPrimitiveById(id, type); + SimplePrimitiveId primitiveId = SimplePrimitiveId.fromString(member); + primitiveConnections[i] = dataSet.getPrimitiveById(primitiveId); if (primitiveConnections[i] == null) { - missingPrimitives.put(i, new Pair<>(id, type)); + missingPrimitives.put(i, new Pair<>(primitiveId.getUniqueId(), primitiveId.getType())); } } obtainMissingPrimitives(dataSet, primitiveConnections, missingPrimitives); diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/cleanup/MissingConnectionTags.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/cleanup/MissingConnectionTags.java index 347e85b..68f16dc 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/cleanup/MissingConnectionTags.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/cleanup/MissingConnectionTags.java @@ -8,14 +8,16 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashSet; +import java.util.List; import java.util.Objects; import java.util.function.Supplier; import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.swing.JOptionPane; import org.openstreetmap.josm.actions.AutoScaleAction; -import org.openstreetmap.josm.command.ChangeCommand; +import org.openstreetmap.josm.command.ChangePropertyCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.command.SequenceCommand; import org.openstreetmap.josm.data.osm.BBox; @@ -100,11 +102,15 @@ public class MissingConnectionTags extends AbstractConflationCommand { MainApplication.getLayerManager().setActiveLayer(current); } GuiHelper.runInEDT(() -> getAffectedDataSet().setSelected(selection)); - commands.forEach(Command::undoCommand); - return commands.isEmpty() ? null : new SequenceCommand(tr("Perform missing conflation steps"), commands); + if (commands.size() == 1) { + return commands.iterator().next(); + } else if (!commands.isEmpty()) { + return new SequenceCommand(tr("Perform missing conflation steps"), commands); + } + return null; } - private void fixErrors(String prefKey, Collection commands, Collection issues) { + protected void fixErrors(String prefKey, Collection commands, Collection issues) { for (TestError issue : issues) { issue.getHighlighted(); if (!issue.isFixable() || issue.getPrimitives().parallelStream().anyMatch(IPrimitive::isDeleted)) { @@ -147,7 +153,26 @@ public class MissingConnectionTags extends AbstractConflationCommand { way.getDataSet().searchNodes(searchBBox).stream().filter(MissingConnectionTags::noConflationKey) .forEach(duplicateNodeTest::visit); duplicateNodeTest.endTest(); - issues.addAll(duplicateNodeTest.getErrors().stream().distinct().collect(Collectors.toList())); + if (duplicateNodeTest.getErrors().isEmpty()) { + continue; + } + List dupeNodes = duplicateNodeTest.getErrors().stream() + .filter(e -> e.getPrimitives().contains(node)).flatMap(e -> e.getPrimitives().stream()) + .distinct().filter(p -> !p.isDeleted() && !p.equals(node)).collect(Collectors.toList()); + if (dupeNodes.isEmpty()) { + continue; + } + List dupes = duplicateNodeTest.getErrors().stream() + .filter(e -> e.getPrimitives().contains(node)).flatMap(e -> e.getPrimitives().stream()) + .distinct().filter(p -> !p.isDeleted() && !p.equals(node)).map(OsmPrimitive::getPrimitiveId) + .map(Object::toString).collect(Collectors.toList()); + + TestError initial = duplicateNodeTest.getErrors().get(0); + List prims = new ArrayList<>(dupeNodes); + prims.add(node); + issues.add(TestError.builder(initial.getTester(), initial.getSeverity(), initial.getCode()) + .message(initial.getMessage()).primitives(prims) + .fix(() -> new ChangePropertyCommand(node, "dupe", String.join(",", dupes))).build()); duplicateNodeTest.clear(); } } @@ -175,7 +200,7 @@ public class MissingConnectionTags extends AbstractConflationCommand { continue; } TestError.Builder fixError = TestError.builder(error.getTester(), error.getSeverity(), error.getCode()) - .primitives(error.getPrimitives()).fix(createIntersectionCommandSupplier(error, way)) + .primitives(error.getPrimitives()).fix(createIntersectionCommandSupplier(error, way, precision)) .message(error.getMessage()); seenFix.addAll(error.getPrimitives()); issues.add(fixError.build()); @@ -185,7 +210,7 @@ public class MissingConnectionTags extends AbstractConflationCommand { return issues; } - private Supplier createIntersectionCommandSupplier(TestError error, Way way) { + private static Supplier createIntersectionCommandSupplier(TestError error, Way way, double precision) { Collection nodes = Geometry.addIntersections(error.getPrimitives().stream().filter(Way.class::isInstance) .map(Way.class::cast).filter(w -> w.hasKey(HIGHWAY)).collect(Collectors.toList()), false, new ArrayList<>()); @@ -226,12 +251,10 @@ public class MissingConnectionTags extends AbstractConflationCommand { private static Command createAddNodeCommand(Way way, Node node, double precision) { if (Geometry.getDistance(node, way) < precision) { WaySegment seg = Geometry.getClosestWaySegment(way, node); - int index1 = way.getNodes().indexOf(seg.getFirstNode()); - int index2 = way.getNodes().indexOf(seg.getSecondNode()); - if (index2 - index1 == 1) { - Way newWay = new Way(way); - newWay.addNode(index2, node); - return new ChangeCommand(way, newWay); + if (seg != null) { + return new ChangePropertyCommand(node, "conn", + String.join(",", Stream.of(way, seg.getFirstNode(), seg.getSecondNode()) + .map(p -> p.getPrimitiveId().toString()).collect(Collectors.toList()))); } } return null; @@ -247,10 +270,15 @@ public class MissingConnectionTags extends AbstractConflationCommand { // precision is in meters double precision = p == 0 ? 1 : p; + Collection primsAndChildren = new ArrayList<>(possiblyAffectedPrimitives); + possiblyAffectedPrimitives.stream().filter(Way.class::isInstance).map(Way.class::cast).map(Way::getNodes) + .forEach(primsAndChildren::addAll); Collection issues = new ArrayList<>(); for (TestError issue : unconnectedWays.getErrors()) { - if (issue.isFixable()) { - issues.add(issue); + if (issue.isFixable() || issue.getPrimitives().stream().noneMatch(t -> primsAndChildren.contains(t))) { + if (issue.isFixable() && issue.getPrimitives().stream().anyMatch(t -> primsAndChildren.contains(t))) { + issues.add(issue); + } continue; } Way way = Utils.filteredCollection(new ArrayList<>(issue.getPrimitives()), Way.class).stream().findAny() 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 9534df0..302caf2 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 @@ -39,11 +39,11 @@ public class CreateConnectionsCommand extends Command { private Command undoCommands; private static final LinkedHashSet> CONFLATION_COMMANDS = new LinkedHashSet<>(); static { + CONFLATION_COMMANDS.add(MissingConnectionTags.class); CONFLATION_COMMANDS.add(ConnectedCommand.class); CONFLATION_COMMANDS.add(DuplicateCommand.class); CONFLATION_COMMANDS.add(MergeAddressBuildings.class); CONFLATION_COMMANDS.add(MergeBuildingAddress.class); - CONFLATION_COMMANDS.add(MissingConnectionTags.class); CONFLATION_COMMANDS.add(OverNodedWays.class); } @@ -89,8 +89,6 @@ public class CreateConnectionsCommand extends Command { public static List createConnections(DataSet dataSet, Collection collection) { final List permanent = new ArrayList<>(); final List undoable = new ArrayList<>(); - final Collection realPrimitives = collection.stream().map(dataSet::getPrimitiveById) - .filter(Objects::nonNull).collect(Collectors.toList()); List> runCommands = new ArrayList<>(); for (final Class abstractCommandClass : getConflationCommands()) { final AbstractConflationCommand abstractCommand; @@ -105,6 +103,8 @@ public class CreateConnectionsCommand extends Command { if (runCommands.parallelStream().anyMatch(c -> abstractCommand.conflictedCommands().contains(c))) { continue; } + final Collection realPrimitives = collection.stream().map(dataSet::getPrimitiveById) + .filter(Objects::nonNull).collect(Collectors.toList()); final Collection tPrimitives = new TreeSet<>(); abstractCommand.getInterestedTypes() .forEach(clazz -> tPrimitives.addAll(Utils.filteredCollection(realPrimitives, clazz))); 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 92fbcd2..67c949d 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveActionTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveActionTest.java @@ -7,12 +7,6 @@ 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.util.Arrays; -import java.util.HashMap; -import java.util.Map; - -import javax.swing.JOptionPane; - import org.awaitility.Awaitility; import org.awaitility.Durations; import org.junit.Before; @@ -31,12 +25,9 @@ import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.gui.util.GuiHelper; import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.ConnectedCommand; import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.DuplicateCommand; +import org.openstreetmap.josm.plugins.mapwithai.testutils.MissingConnectionTagsMocker; import org.openstreetmap.josm.testutils.JOSMTestRules; -import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker; import org.openstreetmap.josm.testutils.mockers.WindowMocker; -import org.openstreetmap.josm.tools.I18n; - -import com.google.common.collect.ImmutableMap; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import mockit.Mock; @@ -65,6 +56,9 @@ public class MapWithAIMoveActionTest { way2.getNodes().forEach(node -> osmData.addPrimitive(node)); osmData.addPrimitive(way2); mapWithAIData.addPrimitive(way1); + way2.setOsmId(1, 1); + way2.firstNode().setOsmId(1, 1); + way2.lastNode().setOsmId(2, 1); osmLayer = new OsmDataLayer(osmData, "osm", null); final MapWithAILayer mapWithAILayer = new MapWithAILayer(mapWithAIData, "MapWithAI", null); @@ -75,14 +69,16 @@ public class MapWithAIMoveActionTest { @Test public void testMoveAction() { - new JOptionPaneSimpleMocker(ImmutableMap.of("Sequence: Merge 2 nodes", JOptionPane.NO_OPTION)); + new MissingConnectionTagsMocker(); mapWithAIData.addSelected(way1); moveAction.actionPerformed(null); assertEquals(osmLayer, MainApplication.getLayerManager().getActiveLayer(), "Current layer should be the OMS layer"); assertNotNull(osmLayer.getDataSet().getPrimitiveById(way1), "way1 should have been added to the OSM layer"); - UndoRedoHandler.getInstance().undo(); + while (UndoRedoHandler.getInstance().hasUndoCommands()) { + UndoRedoHandler.getInstance().undo(); + } assertNull(osmLayer.getDataSet().getPrimitiveById(way1), "way1 should have been removed from the OSM layer"); } @@ -93,6 +89,7 @@ public class MapWithAIMoveActionTest { @Test public void testConflationDupeKeyRemoval() { + new MissingConnectionTagsMocker(); mapWithAIData.unlock(); way1.lastNode().put(DuplicateCommand.KEY, "n" + Long.toString(way2.lastNode().getUniqueId())); mapWithAIData.lock(); @@ -117,6 +114,7 @@ public class MapWithAIMoveActionTest { @Test public void testConflationConnKeyRemoval() { + new MissingConnectionTagsMocker(); mapWithAIData.unlock(); way1.lastNode().put(ConnectedCommand.KEY, "w" + Long.toString(way2.getUniqueId()) + ",n" + Long.toString(way2.lastNode().getUniqueId()) + ",n" + Long.toString(way2.firstNode().getUniqueId())); @@ -150,13 +148,7 @@ public class MapWithAIMoveActionTest { public void testMaxAddNotification() { TestUtils.assumeWorkingJMockit(); new WindowMocker(); - Map map = new HashMap<>(); - for (String text : Arrays.asList(I18n.marktr("Sequence: Merge {0} nodes"), I18n.marktr("Delete {0} nodes"))) { - for (int i = 0; i < 10; i++) { - map.computeIfAbsent(I18n.tr(text, i), (t) -> JOptionPane.NO_OPTION); - } - } - new JOptionPaneSimpleMocker(map); + new MissingConnectionTagsMocker(); NotificationMocker notification = new NotificationMocker(); DataSet ds = MapWithAIDataUtils.getLayer(true).getDataSet(); diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/ConnectedCommandTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/ConnectedCommandTest.java index b5f4806..1fdcd4f 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/ConnectedCommandTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/ConnectedCommandTest.java @@ -40,6 +40,11 @@ public class ConnectedCommandTest { assertThrows(NullPointerException.class, () -> command.getCommand(Collections.singletonList(toAdd))); + // SimplePrimitiveId doesn't like negative ids + way.setOsmId(1, 1); + way.firstNode().setOsmId(1, 1); + way.lastNode().setOsmId(2, 1); + toAdd.put(command.getKey(), "w" + way.getUniqueId() + ",n" + way.firstNode().getUniqueId() + ",n" + way.lastNode().getUniqueId()); diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/DuplicateCommandTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/DuplicateCommandTest.java index dc6965a..3ec4ad6 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/DuplicateCommandTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/DuplicateCommandTest.java @@ -38,6 +38,8 @@ public class DuplicateCommandTest { assertNull(dupe.getCommand(Collections.singleton(dupe1))); + // SimplePrimitiveId doesn't understand negative ids + dupe2.setOsmId(2, 1); dupe1.put(dupe.getKey(), "n" + dupe2.getUniqueId()); Command command = dupe.getCommand(Collections.singleton(dupe1)); @@ -50,7 +52,7 @@ public class DuplicateCommandTest { assertTrue(dupe1.hasKey(dupe.getKey())); assertFalse(dupe2.hasKey(dupe.getKey())); - Command deleteDupe2 = DeleteCommand.delete(Collections.singleton(dupe2)); + Command deleteDupe2 = DeleteCommand.delete(Collections.singleton(dupe2), true, true); deleteDupe2.executeCommand(); command = dupe.getCommand(Collections.singleton(dupe1)); command.executeCommand(); diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/CreateConnectionsCommandTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/CreateConnectionsCommandTest.java index c0f91de..30c25c6 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/CreateConnectionsCommandTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/CreateConnectionsCommandTest.java @@ -65,6 +65,11 @@ public class CreateConnectionsCommandTest { createConnections.undoCommand(); assertFalse(dataSet.isModified(), "DataSet shouldn't be modified yet"); + // SimplePrimitiveId doesn't like negative ids + way.setOsmId(1, 1); + way.firstNode().setOsmId(1, 1); + way.lastNode().setOsmId(2, 1); + node3.put(ConnectedCommand.KEY, "w" + way.getUniqueId() + ",n" + node1.getUniqueId() + ",n" + node2.getUniqueId()); createConnections = new CreateConnectionsCommand(dataSet, Collections.singleton(node3.save())); 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 78e20cc..e6fe6dd 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java @@ -16,8 +16,6 @@ import java.util.Collections; import java.util.List; import java.util.concurrent.Future; -import javax.swing.JOptionPane; - import org.awaitility.Awaitility; import org.awaitility.Durations; import org.junit.Before; @@ -40,15 +38,13 @@ 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.plugins.mapwithai.testutils.MapWithAITestRules; +import org.openstreetmap.josm.plugins.mapwithai.testutils.MissingConnectionTagsMocker; import org.openstreetmap.josm.plugins.mapwithai.testutils.PleaseWaitDialogMocker; import org.openstreetmap.josm.plugins.mapwithai.testutils.SwingUtilitiesMocker; import org.openstreetmap.josm.testutils.JOSMTestRules; -import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker; import org.openstreetmap.josm.testutils.mockers.WindowMocker; import org.openstreetmap.josm.tools.Logging; -import com.google.common.collect.ImmutableMap; - import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; public class MapWithAIAddComandTest { @@ -111,6 +107,11 @@ public class MapWithAIAddComandTest { new Node(new LatLon(0, 0.15))); final Way way2 = TestUtils.newWay(HIGHWAY_RESIDENTIAL, new Node(new LatLon(0, 0.05)), new Node(new LatLon(0.05, 0.2))); + // SimplePrimitiveId doesn't understand negative ids + way1.setOsmId(1, 1); + way1.firstNode().setOsmId(1, 1); + way1.lastNode().setOsmId(2, 1); + way2.firstNode().put("conn", "w".concat(Long.toString(way1.getUniqueId())).concat(",n") .concat(Long.toString(way1.firstNode().getUniqueId())).concat(",n") @@ -137,7 +138,7 @@ public class MapWithAIAddComandTest { @Test public void testCreateConnectionsUndo() { - new JOptionPaneSimpleMocker(ImmutableMap.of("Sequence: Merge 2 nodes", JOptionPane.NO_OPTION)); + new MissingConnectionTagsMocker(); final DataSet osmData = new DataSet(); final DataSet mapWithAIData = new DataSet(); @@ -150,6 +151,7 @@ public class MapWithAIAddComandTest { osmData.addPrimitive(way2); mapWithAIData.addPrimitive(way1); mapWithAIData.setSelected(way1); + way2.lastNode().setOsmId(1, 1); MapWithAIAddCommand command = new MapWithAIAddCommand(mapWithAIData, osmData, mapWithAIData.getSelected()); command.executeCommand(); @@ -270,6 +272,9 @@ public class MapWithAIAddComandTest { 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))); + original.setOsmId(1, 1); + original.firstNode().setOsmId(1, 1); + original.lastNode().setOsmId(2, 1); String connectedValue = "w" + Long.toString(original.getUniqueId()) + ",n" + Long.toString(original.firstNode().getUniqueId()) + ",n" + Long.toString(original.lastNode().getUniqueId()); diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/conflation/cleanup/MissingConnectionTagsTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/conflation/cleanup/MissingConnectionTagsTest.java index 63f8023..15fcc80 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/conflation/cleanup/MissingConnectionTagsTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/conflation/cleanup/MissingConnectionTagsTest.java @@ -2,11 +2,13 @@ package org.openstreetmap.josm.plugins.mapwithai.commands.conflation.cleanup; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertSame; -import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import javax.swing.JOptionPane; @@ -19,16 +21,15 @@ 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; +import org.openstreetmap.josm.data.osm.SimplePrimitiveId; import org.openstreetmap.josm.data.osm.Way; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.cleanup.MissingConnectionTags; +import org.openstreetmap.josm.plugins.mapwithai.testutils.MissingConnectionTagsMocker; import org.openstreetmap.josm.testutils.JOSMTestRules; -import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker; import org.openstreetmap.josm.testutils.mockers.WindowMocker; -import com.google.common.collect.ImmutableMap; - import mockit.integration.TestRunnerDecorator; public class MissingConnectionTagsTest { @@ -36,7 +37,6 @@ public class MissingConnectionTagsTest { public JOSMTestRules josmTestRules = new JOSMTestRules().projection().main(); private DataSet ds; private MissingConnectionTags missing; - private JOptionPaneSimpleMocker joptionMocker; @Before public void setUp() { @@ -66,7 +66,9 @@ public class MissingConnectionTagsTest { @Test public void testDupeNode() { new WindowMocker(); - joptionMocker = new JOptionPaneSimpleMocker(ImmutableMap.of("Sequence: Merge 2 nodes", JOptionPane.OK_OPTION)); + Map actions = new HashMap<>(); + actions.put("Set dupe=node 1 for node 'node'", JOptionPane.YES_OPTION); + new MissingConnectionTagsMocker(actions); Node node11 = new Node(LatLon.ZERO); Node node21 = new Node(LatLon.ZERO); Node node12 = new Node(LatLon.NORTH_POLE); @@ -77,14 +79,22 @@ public class MissingConnectionTagsTest { way.getNodes().forEach(ds::addPrimitive); ds.addPrimitive(way); } - assertNotSame(way1.firstNode(), way2.firstNode()); + way1.firstNode().setOsmId(1, 1); + assertFalse(way2.firstNode().hasKey("dupe")); Command command = missing.getCommand(Collections.singleton(way2)); + // The dupe key has to appear after making the command, since the command + // was immediately run. + assertTrue(way2.firstNode().hasKey("dupe")); + // The change command will always replace its "undo" after every execution + command.undoCommand(); + assertFalse(way2.firstNode().hasKey("dupe")); for (int i = 0; i < 10; i++) { - assertNotSame(way1.firstNode(), way2.firstNode()); command.executeCommand(); - assertSame(way1.firstNode(), way2.firstNode()); + assertTrue(way2.firstNode().hasKey("dupe")); + assertEquals(way1.firstNode().getOsmPrimitiveId(), + SimplePrimitiveId.fromString(way2.firstNode().get("dupe"))); command.undoCommand(); - assertNotSame(way1.firstNode(), way2.firstNode()); + assertFalse(way2.firstNode().hasKey("dupe")); } } diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/testutils/MissingConnectionTagsMocker.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/testutils/MissingConnectionTagsMocker.java new file mode 100644 index 0000000..e4f8df4 --- /dev/null +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/testutils/MissingConnectionTagsMocker.java @@ -0,0 +1,74 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.plugins.mapwithai.testutils; + +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; + +import javax.swing.JOptionPane; + +import org.openstreetmap.josm.command.Command; +import org.openstreetmap.josm.data.validation.TestError; +import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.cleanup.MissingConnectionTags; +import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker; + +import mockit.Invocation; +import mockit.Mock; +import mockit.MockUp; + +/** + * This mocks the fixErrors to avoid creating a ConditionalOptionPane dialog The + * default is {@link JOptionPane#NO_OPTION} + * + * @author Taylor Smock + * + */ +public class MissingConnectionTagsMocker extends MockUp { + private final JOptionPaneSimpleMocker joptionPaneMocker; + private int defaultOption = JOptionPane.NO_OPTION; + private final Map map; + + /** + * Only use the default option for joptionpanes + */ + public MissingConnectionTagsMocker() { + this(new HashMap<>()); + } + + /** + * Initialize with a map of results + * + * @param map See {@link JOptionPaneSimpleMocker#JOptionPaneSimpleMocker(Map)} + */ + public MissingConnectionTagsMocker(Map map) { + joptionPaneMocker = new JOptionPaneSimpleMocker(map); + this.map = map; + } + + @Mock + protected void fixErrors(Invocation inv, String prefKey, Collection commands, + Collection issues) { + issues.stream().filter(TestError::isFixable).map(t -> t.getFix().getDescriptionText()) + .forEach(m -> map.putIfAbsent(m, defaultOption)); + inv.proceed(prefKey, commands, issues); + } + + /** + * Set the default option + * + * @param jOptionPaneOption Use one of the {@link JOptionPane#getOptions()} + * integers + */ + public void setDefaultOption(int jOptionPaneOption) { + defaultOption = jOptionPaneOption; + } + + /** + * Get the mocker being used for the option pane + * + * @return The JOptionPaneSimpleMocker + */ + public JOptionPaneSimpleMocker getJOptionPaneSimpleMocker() { + return joptionPaneMocker; + } +}