diff --git a/build.gradle b/build.gradle index acfaa6b..a98e8d0 100644 --- a/build.gradle +++ b/build.gradle @@ -17,7 +17,6 @@ dependencies { packIntoJar "org.apache.commons:commons-lang3:3.7" implementation "javax.validation:validation-api:2.0.1.Final" - testImplementation "junit:junit:4.+" testImplementation "org.codehaus.groovy:groovy-all:2.4.13" testImplementation "org.hamcrest:hamcrest-library:1.3" diff --git a/src/main/java/org/openstreetmap/josm/plugins/contourmerge/ContourMergeModel.java b/src/main/java/org/openstreetmap/josm/plugins/contourmerge/ContourMergeModel.java index e78e0dc..4a346f8 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/contourmerge/ContourMergeModel.java +++ b/src/main/java/org/openstreetmap/josm/plugins/contourmerge/ContourMergeModel.java @@ -11,6 +11,7 @@ import org.openstreetmap.josm.gui.layer.OsmDataLayer; import javax.validation.constraints.NotNull; +import javax.validation.constraints.Null; import java.awt.*; import java.util.*; import java.util.List; @@ -459,16 +460,14 @@ protected Stream buildNodeDeleteCommands( // of a merge operation final WaySlice first = sources.get(0); - final Set ways = sources.stream().map(slice -> slice.getWay()) + final Set ways = sources.stream().map(WaySlice::getWay) .collect(Collectors.toSet()); - return first.getNodes().stream() .map(n -> { // true, if the node n is only referenced by source ways // in the merge operation final boolean hasNoParents = n.getReferrers().stream() - .filter(p -> !ways.contains(p)) - .count() == 0; + .allMatch(ways::contains); if (hasNoParents && !n.isTagged()) { return Optional.of(new DeleteCommand(n)); @@ -476,7 +475,7 @@ protected Stream buildNodeDeleteCommands( return Optional.empty(); }) .filter(Optional::isPresent) - .map(o -> o.get()); + .map(Optional::get); } /** @@ -489,15 +488,21 @@ protected Stream buildNodeDeleteCommands( public Command buildContourAlignCommand() { final WaySlice dragSource = getDragSource(); final WaySlice dropTarget = getDropTarget(); + return buildContourAlignCommand(dragSource, dropTarget); + } + + public @Null Command buildContourAlignCommand( + @Null final WaySlice dragSource, + @Null final WaySlice dropTarget) { if (dragSource == null || dropTarget == null) return null; final List waySlices = dragSource.findAllEquivalentWaySlices() - .collect(Collectors.toList()); + .collect(Collectors.toList()); - List cmds = Stream.concat( - buildSourceChangeCommands(waySlices, dropTarget), - buildNodeDeleteCommands(waySlices) + final List cmds = Stream.concat( + buildSourceChangeCommands(waySlices, dropTarget), + buildNodeDeleteCommands(waySlices) ).collect(Collectors.toList()); return new SequenceCommand(tr("Merging Contour"), cmds); diff --git a/src/main/java/org/openstreetmap/josm/plugins/contourmerge/WaySlice.java b/src/main/java/org/openstreetmap/josm/plugins/contourmerge/WaySlice.java index b9713d8..790dcee 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/contourmerge/WaySlice.java +++ b/src/main/java/org/openstreetmap/josm/plugins/contourmerge/WaySlice.java @@ -1,16 +1,14 @@ package org.openstreetmap.josm.plugins.contourmerge; -import java.util.*; -import java.util.function.Function; -import java.util.stream.Stream; - -import javax.validation.constraints.NotNull; - +import lombok.EqualsAndHashCode; import org.apache.commons.lang3.Validate; import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.Way; -import lombok.EqualsAndHashCode; +import javax.validation.constraints.NotNull; +import java.util.*; +import java.util.function.Function; +import java.util.stream.Stream; /** *

A WaySlice is a sub sequence of a ways sequence of @@ -273,38 +271,73 @@ public WaySlice getOpositeSlice(){ * @param newNodes the new nodes. Ignored if null. * @return the cloned way with the new nodes */ - public Way replaceNodes(final List newNodes) { - final Way nw = new Way(w); - if (newNodes == null || newNodes.isEmpty()) return nw; + public Way replaceNodes(final List newNodes) { + final Way newWay = new Way(w); + final List updatedNodeList = new ArrayList<>(); if (!w.isClosed()) { - List oldNodes = new ArrayList<>(w.getNodes()); - oldNodes.subList(start, end+1).clear(); - oldNodes.addAll(start, newNodes); - nw.setNodes(oldNodes); + updatedNodeList.addAll(w.getNodes()); + updatedNodeList.subList(start, end + 1).clear(); + updatedNodeList.addAll(start, newNodes); + newWay.setNodes(updatedNodeList); } else { - final List updatedNodeList = new ArrayList<>(w.getNodes()); + // this is a slice of a closed way + updatedNodeList.addAll(w.getNodes()); if (inDirection) { + // because the slice is 'in direction' either of the start + // or the end node, neither of them, but not both, are + // included in the slice. + // first, we normalize the current situation if (start == 0) { - updatedNodeList.remove(updatedNodeList.size()-1); - } - updatedNodeList.subList(start,end+1).clear(); - updatedNodeList.addAll(start, newNodes); - if (start == 0) { - updatedNodeList.add(newNodes.get(0)); + // jn -- n ........ n -- n -- ...-- jn + // <---- slice --> + // <-- cut&replace --> cut + // + // (jn - shared join node; n - arbitrary node) + + // cut ... + updatedNodeList.subList(0, end+1).clear(); + updatedNodeList.remove(updatedNodeList.size() - 1); + // ... and replace + updatedNodeList.addAll(0, newNodes); + } else if (end == w.getNodesCount() - 1) { + // jn -- n .... n -- ....... -- jn + // <---- slice --> + // cut <-- cut&replace --> + // + // (jn - shared join node; n - arbitrary node) + + // cut ... + updatedNodeList.subList(start, end + 1).clear(); + updatedNodeList.remove(0); + // ... and replace + updatedNodeList.addAll(newNodes); + } else { + // jn -- n -- n .... n -- -- jn + // <---- slice --> + // keep <-- cut&replace --> keep + // + // (jn - shared join node; n - arbitrary node) + + // cut ... + updatedNodeList.subList(start, end + 1).clear(); + // ... and replace + updatedNodeList.addAll(start, newNodes); } - nw.setNodes(updatedNodeList); + // make sure the way is closed + updatedNodeList.add(updatedNodeList.get(0)); + newWay.setNodes(updatedNodeList); } else { int upper = updatedNodeList.size()-1; - updatedNodeList.subList(end, upper+1).clear(); - updatedNodeList.subList(0,start+1).clear(); + updatedNodeList.subList(end, upper + 1).clear(); + updatedNodeList.subList(0,start + 1).clear(); updatedNodeList.addAll(0, newNodes); // make sure the new way is closed updatedNodeList.add(newNodes.get(0)); - nw.setNodes(updatedNodeList); + newWay.setNodes(updatedNodeList); } } - return nw; + return newWay; } /** diff --git a/src/test/groovy/org/openstreetmap/josm/plugins/contourmerge/AllUnitTests.groovy b/src/test/groovy/org/openstreetmap/josm/plugins/contourmerge/AllUnitTests.groovy index a8636f3..f6bba32 100644 --- a/src/test/groovy/org/openstreetmap/josm/plugins/contourmerge/AllUnitTests.groovy +++ b/src/test/groovy/org/openstreetmap/josm/plugins/contourmerge/AllUnitTests.groovy @@ -5,6 +5,8 @@ import org.junit.runners.Suite @RunWith(Suite.class) @Suite.SuiteClasses([ ContourMergeModelTest.class, - WaySliceTest.class + WaySliceTest.class, + DataIntegrityProblemTest01.class, + DataIntegrityProblemTest02.class ]) class AllUnitTests {} diff --git a/src/test/groovy/org/openstreetmap/josm/plugins/contourmerge/DataIntegrityProblemTest.groovy b/src/test/groovy/org/openstreetmap/josm/plugins/contourmerge/DataIntegrityProblemTest.groovy new file mode 100644 index 0000000..e955c36 --- /dev/null +++ b/src/test/groovy/org/openstreetmap/josm/plugins/contourmerge/DataIntegrityProblemTest.groovy @@ -0,0 +1,272 @@ +package org.openstreetmap.josm.plugins.contourmerge + +import org.junit.Before +import org.junit.BeforeClass +import org.junit.Test +import org.openstreetmap.josm.data.Preferences +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.data.projection.ProjectionRegistry +import org.openstreetmap.josm.data.projection.Projections +import org.openstreetmap.josm.gui.layer.OsmDataLayer +import org.openstreetmap.josm.spi.preferences.Config + +import static org.hamcrest.MatcherAssert.assertThat +import static org.hamcrest.Matchers.equalTo +import static org.junit.Assert.* + +class DataIntegrityProblemTest01 { + + DataSet dataSet + OsmDataLayer layer + ContourMergeModel mergeModel + + Way closedWay + Way openWay + + WaySlice closedWaySlice + List closedWayNodes + WaySlice openWaySlice + + @BeforeClass + static void initJosmConfig() { + Config.setPreferencesInstance(new Preferences()) + ProjectionRegistry.setProjection( + Projections.getProjectionByCode("EPSG:3857")) + } + + def buildNode(Map args) { + def node = new Node(args.id) + node.setCoor(new LatLon(args.lat, args.lon)) + return node + } + + def createDataSet() { + // create a dataset + // add two ways + // - a closed way with four nodes + // - an open node with 6 nodes, which shares two nodes with + // the other way + dataSet = new DataSet() + + //def joinNode = new Node(1, new LatLon(0,0)) + def joinNode = buildNode(id: 0, lat: 0, lon: 0) + closedWayNodes = [ + joinNode, + buildNode(id: 1, lat: 0, lon: -1), + buildNode(id: 2, lat: -1, lon: -1), + buildNode(id: 3, lat: 0, lon: -1), + joinNode + ] + + closedWayNodes[0..3].each {node -> + dataSet.addPrimitive(node) + } + closedWay = new Way() + closedWay.setNodes(closedWayNodes) + dataSet.addPrimitive(closedWay) + + def openWayNodes = [ + buildNode(id: 10, lat: 1, lon: -3), + buildNode(id: 11, lat: 1, lon: -2), + buildNode(id: 12, lat: 1, lon: -1), + buildNode(id: 13, lat: 1, lon: 0), + buildNode(id: 14, lat: 1, lon: 1), + buildNode(id: 15, lat: 1, lon: 2), + ] + openWayNodes.each {node -> + dataSet.addPrimitive(node) + } + openWay = new Way() + openWay.setNodes(openWayNodes) + dataSet.addPrimitive(openWay) + } + + def prepareMergeModel() { + layer = new OsmDataLayer(dataSet, "test name", null /* no file */) + mergeModel = new ContourMergeModel(layer) + } + + + @Before + void prepareTestData() { + createDataSet() + prepareMergeModel() + } + + @Test + void "test case 01"() { + closedWaySlice = new WaySlice(closedWay, 3, 4, true /* in direction */) + openWaySlice = new WaySlice(openWay, 2, 3, true /* in direction */) + + def source = closedWaySlice + def target = openWaySlice + def mergeCommand = mergeModel.buildContourAlignCommand( + source, + target + ) + mergeCommand.executeCommand() + + assertTrue("closedWay should still be closed", closedWay.isClosed()) + assertTrue("openWay should still be open", !openWay.isClosed()) + assertTrue("the original join node of the closed way should be deleted", + closedWayNodes[0].isDeleted()) + + openWay.getNodes().eachWithIndex{ node, i -> + assertFalse("the ${i}-th node of the open way should not be deleted", + openWay.getNode(i).isDeleted()) + } + def openWayNodeIds = openWay.getNodes().collect {Node node -> + node.getId()} + assertThat(openWayNodeIds, equalTo([10l, 11l, 12l, 13l, 14l, 15l])) + + def closedWayIds = closedWay.getNodes().collect {Node node -> + node.getId()} + assertThat(closedWayIds, equalTo([1l, 2l, 12l, 13l, 1l], )) + + closedWay.getNodes().eachWithIndex{ node, i -> + assertFalse("the ${i}-th node of the closed way should not be deleted", + closedWay.getNode(i).isDeleted()) + } + + def numDeletedNodes = dataSet.getNodes().grep { + node -> node.isDeleted() + }.size() + + assertEquals("should have 2 deleted nodes, got ${numDeletedNodes}", + 2, numDeletedNodes, ) + } +} + + +class DataIntegrityProblemTest02 { + + DataSet dataSet + OsmDataLayer layer + ContourMergeModel mergeModel + + Way closedWay + Way openWay + + WaySlice closedWaySlice + List closedWayNodes + WaySlice openWaySlice + + @BeforeClass + static void initJosmConfig() { + Config.setPreferencesInstance(new Preferences()) + ProjectionRegistry.setProjection( + Projections.getProjectionByCode("EPSG:3857")) + } + + def buildNode(Map args) { + def node = new Node(args.id) + node.setCoor(new LatLon(args.lat, args.lon)) + return node + } + + def createDataSet() { + // create a dataset + // add two ways + // - a closed way with four nodes + // - an open node with 6 nodes, which shares two nodes with + // the other way + dataSet = new DataSet() + + //def joinNode = new Node(1, new LatLon(0,0)) + def joinNode = buildNode(id: 0, lat: 0, lon: 0) + closedWayNodes = [ + joinNode, + buildNode(id: 1, lat: 1, lon: -1), + buildNode(id: 2, lat: 1, lon: -2), + buildNode(id: 3, lat: 0, lon: -3), + buildNode(id: 4, lat: -1, lon: -2), + buildNode(id: 5, lat: -1, lon: -1), + joinNode + ] + + closedWayNodes[0..5].each {node -> + dataSet.addPrimitive(node) + } + closedWay = new Way() + closedWay.setNodes(closedWayNodes) + dataSet.addPrimitive(closedWay) + + def openWayNodes = [ + buildNode(id: 10, lat: 1, lon: -3), + buildNode(id: 11, lat: 1, lon: -2), + buildNode(id: 12, lat: 1, lon: -1), + buildNode(id: 13, lat: 1, lon: 0), + buildNode(id: 14, lat: 1, lon: 1), + buildNode(id: 15, lat: 1, lon: 2), + ] + openWayNodes.each {node -> + dataSet.addPrimitive(node) + } + openWay = new Way() + openWay.setNodes(openWayNodes) + dataSet.addPrimitive(openWay) + } + + def prepareMergeModel() { + layer = new OsmDataLayer(dataSet, "test name", null /* no file */) + mergeModel = new ContourMergeModel(layer) + } + + + @Before + void prepareTestData() { + createDataSet() + prepareMergeModel() + } + + @Test + void "test case 02"() { + closedWaySlice = new WaySlice(closedWay, 1, 5, false /* in direction */) + openWaySlice = new WaySlice(openWay, 2, 3, true /* in direction */) + + def source = closedWaySlice + def target = openWaySlice + def mergeCommand = mergeModel.buildContourAlignCommand( + source, + target + ) + mergeCommand.executeCommand() + + assertTrue("closedWay should still be closed", closedWay.isClosed()) + assertTrue("openWay should still be open", !openWay.isClosed()) + assertTrue("the original join node of the closed way should be deleted", + closedWayNodes[0].isDeleted()) + + openWay.getNodes().eachWithIndex{ node, i -> + assertFalse("the ${i}-th node of the open way should not be deleted", + openWay.getNode(i).isDeleted()) + } + def openWayNodeIds = openWay.getNodes().collect {Node node -> + node.getId()} + println("openWayNodeIds: ${openWayNodeIds}") + //assertThat(openWayNodeIds, equalTo([10l, 11l, 12l, 13l, 14l, 15l])) + + def closedWayIds = closedWay.getNodes().collect {Node node -> + node.getId()} + //assertThat(closedWayIds, equalTo([1l, 2l, 12l, 13l, 1l], )) + println("closedWayIds: ${closedWayIds}") + + closedWay.getNodes().eachWithIndex{ node, i -> + assertFalse("the ${i}-th node of the closed way should not be deleted", + closedWay.getNode(i).isDeleted()) + } + + def numDeletedNodes = dataSet.getNodes().grep { + node -> node.isDeleted() + }.size() + + assertEquals("should have 3 deleted nodes, got ${numDeletedNodes}", + 3, numDeletedNodes, ) + } +} + + + diff --git a/test/data/issue-20/README.md b/test/data/issue-20/README.md new file mode 100644 index 0000000..4fe4c01 --- /dev/null +++ b/test/data/issue-20/README.md @@ -0,0 +1,2 @@ +Sample data sets, used to analyse the defects reported in issue +[#20: DataIntegrityProblemException: Deleted node referenced](https://github.com/Gubaer/josm-contourmerge-plugin/issues/20) \ No newline at end of file diff --git a/test/data/issue-20/test-set-01.osm b/test/data/issue-20/test-set-01.osm new file mode 100644 index 0000000..1dbc938 --- /dev/null +++ b/test/data/issue-20/test-set-01.osm @@ -0,0 +1,54 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/data/issue-20/test-set-02.osm b/test/data/issue-20/test-set-02.osm new file mode 100644 index 0000000..dac706e --- /dev/null +++ b/test/data/issue-20/test-set-02.osm @@ -0,0 +1,40 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/data/issue-20/test-set-03.osm b/test/data/issue-20/test-set-03.osm new file mode 100644 index 0000000..5baa649 --- /dev/null +++ b/test/data/issue-20/test-set-03.osm @@ -0,0 +1,28 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/data/issue-20/test-set-04.osm b/test/data/issue-20/test-set-04.osm new file mode 100644 index 0000000..458cced --- /dev/null +++ b/test/data/issue-20/test-set-04.osm @@ -0,0 +1,32 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +