Detect potential missing connection tags, and offer to add the appropriate connection

Signed-off-by: Taylor Smock <taylor.smock@kaart.com>
pull/1/head
Taylor Smock 2020-05-12 10:32:48 -06:00
rodzic bc2a7fa195
commit c3143a056a
Nie znaleziono w bazie danych klucza dla tego podpisu
ID klucza GPG: 625F6A74A3E4311A
8 zmienionych plików z 367 dodań i 6 usunięć

Wyświetl plik

@ -31,7 +31,7 @@ import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
import org.openstreetmap.josm.tools.Pair;
public abstract class AbstractConflationCommand extends Command {
Collection<OsmPrimitive> possiblyAffectedPrimitives;
protected Collection<OsmPrimitive> possiblyAffectedPrimitives;
public AbstractConflationCommand(DataSet data) {
super(data);

Wyświetl plik

@ -19,6 +19,7 @@ import org.openstreetmap.josm.data.osm.Node;
import org.openstreetmap.josm.data.osm.OsmPrimitive;
import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
import org.openstreetmap.josm.tools.Logging;
import org.openstreetmap.josm.tools.Utils;
public class DuplicateCommand extends AbstractConflationCommand {
public static final String KEY = "dupe";
@ -90,8 +91,8 @@ public class DuplicateCommand extends AbstractConflationCommand {
@Override
public Command getRealCommand() {
final List<Command> commands = new ArrayList<>();
for (Node tNode : possiblyAffectedPrimitives.stream().filter(Node.class::isInstance).map(Node.class::cast)
.distinct().filter(node -> node.hasKey(KEY)).collect(Collectors.toList())) {
for (Node tNode : Utils.filteredCollection(possiblyAffectedPrimitives, Node.class).stream().distinct()
.filter(node -> node.hasKey(KEY)).collect(Collectors.toList())) {
List<Command> tCommands = duplicateNode(getAffectedDataSet(), tNode);
// We have to execute the command to avoid duplicating the command later. Undo
// occurs later, so that the state doesn't actually change.

Wyświetl plik

@ -0,0 +1,234 @@
// License: GPL. For details, see LICENSE file.
package org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.cleanup;
import static org.openstreetmap.josm.tools.I18n.tr;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Objects;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import javax.swing.JOptionPane;
import org.openstreetmap.josm.actions.AutoScaleAction;
import org.openstreetmap.josm.command.ChangeCommand;
import org.openstreetmap.josm.command.Command;
import org.openstreetmap.josm.command.SequenceCommand;
import org.openstreetmap.josm.data.osm.BBox;
import org.openstreetmap.josm.data.osm.DataSet;
import org.openstreetmap.josm.data.osm.IPrimitive;
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.data.osm.WaySegment;
import org.openstreetmap.josm.data.validation.TestError;
import org.openstreetmap.josm.data.validation.tests.CrossingWays;
import org.openstreetmap.josm.data.validation.tests.DuplicateNode;
import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil;
import org.openstreetmap.josm.gui.MainApplication;
import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
import org.openstreetmap.josm.gui.util.GuiHelper;
import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.AbstractConflationCommand;
import org.openstreetmap.josm.plugins.mapwithai.commands.CreateConnectionsCommand;
import org.openstreetmap.josm.spi.preferences.Config;
import org.openstreetmap.josm.tools.Geometry;
import org.openstreetmap.josm.tools.Logging;
import org.openstreetmap.josm.tools.Utils;
/**
* This checks for missing dupe tags, and asks the user if it is a duplicate
*
* @author Taylor Smock
*/
public class MissingConnectionTags extends AbstractConflationCommand {
private double precision = Config.getPref().getDouble("validator.duplicatenodes.precision", 0.);
public MissingConnectionTags(DataSet data) {
super(data);
}
@Override
public String getDescriptionText() {
return tr("Deduplicate nodes and connect ways");
}
@Override
public Collection<Class<? extends OsmPrimitive>> getInterestedTypes() {
return Collections.singleton(Way.class);
}
@Override
public String getKey() {
// For now, we assume that only highways are at issue.
return "highway";
}
@Override
public Command getRealCommand() {
// precision is in meters
precision = precision == 0 ? 1 : precision;
String prefKey = "mapwithai.conflation.missingconflationtags";
Collection<OsmPrimitive> selection = getAffectedDataSet().getAllSelected();
ConditionalOptionPaneUtil.startBulkOperation(prefKey);
Collection<Command> commands = new ArrayList<>();
fixErrors(prefKey, commands, findDuplicateNodes(possiblyAffectedPrimitives));
fixErrors(prefKey, commands, findCrossingWaysAtNodes(possiblyAffectedPrimitives));
ConditionalOptionPaneUtil.endBulkOperation(prefKey);
GuiHelper.runInEDT(() -> getAffectedDataSet().setSelected(selection));
commands.forEach(Command::undoCommand);
return commands.isEmpty() ? null : new SequenceCommand(tr("Perform missing conflation steps"), commands);
}
private void fixErrors(String prefKey, Collection<Command> commands, Collection<TestError> issues) {
for (TestError issue : issues) {
issue.getHighlighted();
if (!issue.isFixable() || issue.getPrimitives().parallelStream().anyMatch(IPrimitive::isDeleted)) {
continue;
}
GuiHelper.runInEDT(() -> getAffectedDataSet().setSelected(issue.getPrimitives()));
Collection<? extends OsmPrimitive> primitives = issue.getPrimitives();
if (primitives.stream().noneMatch(Node.class::isInstance)) {
AutoScaleAction.zoomTo(issue.getPrimitives());
} else {
AutoScaleAction.zoomTo(issue.getPrimitives().stream().filter(Node.class::isInstance)
.map(Node.class::cast).collect(Collectors.toList()));
}
String message = issue.getFix().getDescriptionText();
boolean fixIt = ConditionalOptionPaneUtil.showConfirmationDialog(prefKey, MainApplication.getMainFrame(),
message, tr("Possible missing conflation key"), JOptionPane.YES_NO_CANCEL_OPTION,
JOptionPane.QUESTION_MESSAGE, JOptionPane.YES_OPTION);
if (fixIt) {
Command command = issue.getFix();
command.executeCommand();
commands.add(command);
}
}
}
/**
* Find nodes that may be missing a dupe tag
*
* @param possiblyAffectedPrimitives The primitives that may be affected
* @return The issues
*/
protected static Collection<TestError> findDuplicateNodes(Collection<OsmPrimitive> possiblyAffectedPrimitives) {
Collection<TestError> issues = new ArrayList<>();
DuplicateNode duplicateNodeTest = new DuplicateNode();
for (Way way : Utils.filteredCollection(possiblyAffectedPrimitives, Way.class)) {
for (Node node : way.getNodes()) {
duplicateNodeTest.startTest(NullProgressMonitor.INSTANCE);
BBox searchBBox = node.getBBox();
searchBBox.addPrimitive(node, 0.001);
way.getDataSet().searchNodes(searchBBox).stream().filter(MissingConnectionTags::noConflationKey)
.forEach(duplicateNodeTest::visit);
duplicateNodeTest.endTest();
issues.addAll(duplicateNodeTest.getErrors().stream().distinct().collect(Collectors.toList()));
duplicateNodeTest.clear();
}
}
return issues;
}
/**
* Find nodes that may be missing a conn tag
*
* @param possiblyAffectedPrimitives The primitives that may be affected
* @return The issues found
*/
protected Collection<TestError> findCrossingWaysAtNodes(Collection<OsmPrimitive> possiblyAffectedPrimitives) {
Collection<TestError> issues = new ArrayList<>();
CrossingWays.Ways crossingWays = new CrossingWays.Ways();
for (Way way : Utils.filteredCollection(possiblyAffectedPrimitives, Way.class)) {
Collection<OsmPrimitive> seenFix = new HashSet<>();
crossingWays.startTest(NullProgressMonitor.INSTANCE);
way.getDataSet().searchWays(way.getBBox()).forEach(crossingWays::visit);
crossingWays.endTest();
for (TestError error : crossingWays.getErrors()) {
if (seenFix.containsAll(error.getPrimitives())) {
continue;
}
TestError.Builder fixError = TestError.builder(error.getTester(), error.getSeverity(), error.getCode())
.primitives(error.getPrimitives()).fix(createIntersectionCommandSupplier(error, way))
.message(error.getMessage());
seenFix.addAll(error.getPrimitives());
issues.add(fixError.build());
}
crossingWays.clear();
}
return issues;
}
private Supplier<Command> createIntersectionCommandSupplier(TestError error, Way way) {
Collection<Node> 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<>());
if (nodes.stream().anyMatch(n -> way.getNodes().stream().filter(MissingConnectionTags::noConflationKey)
.anyMatch(wn -> n.getCoor().greatCircleDistance(wn.getCoor()) < precision))) {
return () -> createIntersectionCommand(way,
way.getNodes().stream().filter(MissingConnectionTags::noConflationKey)
.filter(n1 -> nodes.stream().anyMatch(n2 -> Geometry.getDistance(n1, n2) < precision))
.collect(Collectors.toList()),
precision);
}
return null;
}
private static Command createIntersectionCommand(Way way, Collection<Node> intersectionNodes, double precision) {
Collection<Command> commands = new ArrayList<>();
if (intersectionNodes.stream().anyMatch(way::containsNode)) {
Collection<Way> searchWays = way.getDataSet().searchWays(way.getBBox()).parallelStream()
.collect(Collectors.toList());
searchWays.remove(way);
for (Way potential : searchWays) {
for (Node node : way.getNodes()) {
if (Geometry.getDistance(node, potential) < precision) {
WaySegment seg = Geometry.getClosestWaySegment(potential, node);
int index1 = potential.getNodes().indexOf(seg.getFirstNode());
int index2 = potential.getNodes().indexOf(seg.getSecondNode());
if (index2 - index1 == 1) {
Way newWay = new Way(potential);
newWay.addNode(index2, node);
commands.add(new ChangeCommand(potential, newWay));
}
}
}
}
}
if (commands.size() == 1) {
return commands.iterator().next();
} else if (!commands.isEmpty()) {
return new SequenceCommand(tr("Create intersections"), commands);
}
return null;
}
private static boolean noConflationKey(OsmPrimitive prim) {
return CreateConnectionsCommand.getConflationCommands().stream().map(c -> {
try {
return c.getConstructor(DataSet.class).newInstance(prim.getDataSet());
} catch (InstantiationException | IllegalAccessException | IllegalArgumentException
| InvocationTargetException | NoSuchMethodException | SecurityException e) {
Logging.error(e);
}
return null;
}).filter(Objects::nonNull).noneMatch(c -> prim.hasKey(c.getKey()));
}
@Override
public boolean allowUndo() {
return false;
}
@Override
public boolean keyShouldNotExistInOSM() {
return false;
}
}

Wyświetl plik

@ -28,6 +28,7 @@ import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.Conn
import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.DuplicateCommand;
import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.MergeAddressBuildings;
import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.MergeBuildingAddress;
import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.cleanup.MissingConnectionTags;
import org.openstreetmap.josm.tools.Logging;
import org.openstreetmap.josm.tools.Utils;
@ -41,6 +42,7 @@ public class CreateConnectionsCommand extends Command {
CONFLATION_COMMANDS.add(DuplicateCommand.class);
CONFLATION_COMMANDS.add(MergeAddressBuildings.class);
CONFLATION_COMMANDS.add(MergeBuildingAddress.class);
CONFLATION_COMMANDS.add(MissingConnectionTags.class);
}
public CreateConnectionsCommand(DataSet data, Collection<PrimitiveData> primitives) {
@ -58,7 +60,7 @@ public class CreateConnectionsCommand extends Command {
if (command != null) {
command.executeCommand();
}
if (undoCommands != null) {
if (undoCommands != null && !UndoRedoHandler.getInstance().getUndoCommands().contains(undoCommands)) {
GuiHelper.runInEDTAndWait(() -> UndoRedoHandler.getInstance().add(undoCommands));
}
return true;

Wyświetl plik

@ -7,6 +7,8 @@ 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 javax.swing.JOptionPane;
import org.awaitility.Awaitility;
import org.awaitility.Durations;
import org.junit.Before;
@ -26,8 +28,11 @@ 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.testutils.JOSMTestRules;
import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker;
import org.openstreetmap.josm.testutils.mockers.WindowMocker;
import com.google.common.collect.ImmutableMap;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import mockit.Mock;
import mockit.MockUp;
@ -65,6 +70,8 @@ public class MapWithAIMoveActionTest {
@Test
public void testMoveAction() {
new JOptionPaneSimpleMocker(ImmutableMap.of("Sequence: Merge 2 nodes", JOptionPane.NO_OPTION));
mapWithAIData.addSelected(way1);
moveAction.actionPerformed(null);
assertEquals(osmLayer, MainApplication.getLayerManager().getActiveLayer(),

Wyświetl plik

@ -16,8 +16,11 @@ import java.util.List;
import java.util.concurrent.Future;
import java.util.concurrent.FutureTask;
import javax.swing.JOptionPane;
import org.awaitility.Awaitility;
import org.awaitility.Durations;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.jupiter.api.Assertions;
@ -32,14 +35,18 @@ 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.MainApplication;
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.SwingUtilitiesMocker;
import org.openstreetmap.josm.testutils.JOSMTestRules;
import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker;
import org.openstreetmap.josm.tools.Logging;
import com.google.common.collect.ImmutableMap;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class MapWithAIAddComandTest {
@ -47,7 +54,13 @@ public class MapWithAIAddComandTest {
@Rule
@SuppressFBWarnings("URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
public JOSMTestRules test = new MapWithAITestRules().wiremock().projection().assertionsInEDT();
public JOSMTestRules test = new MapWithAITestRules().wiremock().projection().assertionsInEDT().main();
@Before
public void setUp() {
// Required to avoid an NPE with AutoZoomHandler
MainApplication.getLayerManager().addLayer(new OsmDataLayer(new DataSet(), "Temp", null));
}
@Test
public void testMoveCollection() {
@ -121,6 +134,8 @@ public class MapWithAIAddComandTest {
@Test
public void testCreateConnectionsUndo() {
new JOptionPaneSimpleMocker(ImmutableMap.of("Sequence: Merge 2 nodes", JOptionPane.NO_OPTION));
final DataSet osmData = new DataSet();
final DataSet mapWithAIData = new DataSet();
final Way way1 = TestUtils.newWay(HIGHWAY_RESIDENTIAL, new Node(new LatLon(0, 0)),

Wyświetl plik

@ -0,0 +1,91 @@
// License: GPL. For details, see LICENSE file.
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 java.util.Arrays;
import java.util.Collections;
import javax.swing.JOptionPane;
import org.junit.After;
import org.junit.Before;
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;
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.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 {
@Rule
public JOSMTestRules josmTestRules = new JOSMTestRules().projection().main();
private DataSet ds;
private MissingConnectionTags missing;
private JOptionPaneSimpleMocker joptionMocker;
@Before
public void setUp() {
TestRunnerDecorator.cleanUpAllMocks();
ds = new DataSet();
// Required to avoid an NPE in AutoScaleAction
MainApplication.getLayerManager().addLayer(new OsmDataLayer(ds, "Test Layer", null));
missing = new MissingConnectionTags(ds);
}
@After
public void tearDown() {
TestRunnerDecorator.cleanUpAllMocks();
}
@Test
public void testUndoable() {
/*
* If we allow undo, issues occur due to dataset issues. If removing this test,
* add an broader test to check for what happens when the move command is used
* to move data, and then data is modified with this command, and then undo/redo
* happens.
*/
assertFalse(missing.allowUndo());
}
@Test
public void testDupeNode() {
new WindowMocker();
joptionMocker = new JOptionPaneSimpleMocker(ImmutableMap.of("Sequence: Merge 2 nodes", JOptionPane.OK_OPTION));
Node node11 = new Node(LatLon.ZERO);
Node node21 = new Node(LatLon.ZERO);
Node node12 = new Node(LatLon.NORTH_POLE);
Node node22 = new Node(LatLon.SOUTH_POLE);
Way way1 = TestUtils.newWay("highway=residential", node11, node12);
Way way2 = TestUtils.newWay("highway=residential", node21, node22);
for (Way way : Arrays.asList(way1, way2)) {
way.getNodes().forEach(ds::addPrimitive);
ds.addPrimitive(way);
}
assertNotSame(way1.firstNode(), way2.firstNode());
Command command = missing.getCommand(Collections.singleton(way2));
for (int i = 0; i < 10; i++) {
assertNotSame(way1.firstNode(), way2.firstNode());
command.executeCommand();
assertSame(way1.firstNode(), way2.firstNode());
command.undoCommand();
assertNotSame(way1.firstNode(), way2.firstNode());
}
}
}

Wyświetl plik

@ -3,6 +3,7 @@ package org.openstreetmap.josm.plugins.mapwithai.testutils;
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options;
import java.lang.Thread.UncaughtExceptionHandler;
import java.util.logging.Level;
import org.junit.runners.model.InitializationError;
@ -21,12 +22,15 @@ import org.openstreetmap.josm.tools.Logging;
import com.github.tomakehurst.wiremock.WireMockServer;
import mockit.integration.TestRunnerDecorator;
public class MapWithAITestRules extends JOSMTestRules {
private boolean sources;
private boolean wiremock;
private WireMockServer wireMock;
private boolean workerExceptions = true;
private UncaughtExceptionHandler currentExceptionHandler;
public MapWithAITestRules() {
super();
@ -58,6 +62,7 @@ public class MapWithAITestRules extends JOSMTestRules {
*/
@Override
protected void before() throws InitializationError, ReflectiveOperationException {
TestRunnerDecorator.cleanUpAllMocks();
super.before();
Logging.getLogger().setFilter(record -> record.getLevel().intValue() >= Level.WARNING.intValue()
|| record.getSourceClassName().startsWith("org.openstreetmap.josm.plugins.mapwithai"));
@ -80,6 +85,7 @@ public class MapWithAITestRules extends JOSMTestRules {
}
}
if (workerExceptions) {
currentExceptionHandler = Thread.getDefaultUncaughtExceptionHandler();
Thread.setDefaultUncaughtExceptionHandler((t, e) -> {
Logging.error(t.getClass().getSimpleName());
Logging.error(e);
@ -88,13 +94,18 @@ public class MapWithAITestRules extends JOSMTestRules {
}
@Override
protected void after() {
protected void after() throws ReflectiveOperationException {
super.after();
if (wiremock) {
wireMock.stop();
resetMapWithAILayerInfo();
DataAvailability.setReleaseUrl(DataAvailability.DEFAULT_SERVER_URL);
Config.getPref().put("osm-server.url", null);
}
if (workerExceptions) {
Thread.setDefaultUncaughtExceptionHandler(currentExceptionHandler);
}
TestRunnerDecorator.cleanUpAllMocks();
}
private static void setupMapWithAILayerInfo(WireMockServer wireMockServer) {