Index: /trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java =================================================================== --- /trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java (revision 15882) +++ /trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java (revision 15883) @@ -480,4 +480,12 @@ public void actionPerformed(ActionEvent e) { join(getLayerManager().getEditDataSet().getSelectedWays()); + clearFields(); + } + + private void clearFields() { + ds = null; + cmdsCount = 0; + cmds.clear(); + addedRelations.clear(); } @@ -488,6 +496,5 @@ */ public void join(Collection ways) { - cmdsCount = 0; - addedRelations.clear(); + clearFields(); if (ways.isEmpty()) { @@ -675,6 +682,21 @@ List preparedWays = new ArrayList<>(); + // maps oldest way referring to start of each part + Map oldestWayMap = new HashMap<>(); + for (Way way : outerStartingWays) { List splitWays = splitWayOnNodes(way, nodes); + + // see #9599 + if (!way.isNew() && splitWays.size() > 1) { + for (Way part : splitWays) { + Node n = part.firstNode(); + Way old = oldestWayMap.get(n); + if (old == null || old.getUniqueId() > way.getUniqueId()) { + oldestWayMap.put(n, way); + } + } + } + preparedWays.addAll(markWayInsideSide(splitWays, false)); } @@ -721,4 +743,25 @@ commitCommands(marktr("Delete relations")); + + // see #9599: result should contain original way(s) where possible + if (discardedWays.stream().anyMatch(w -> !w.isNew())) { + for (int i = 0; i < polygons.size(); i++) { + Multipolygon mp = polygons.get(i); + for (int k = 0; k < mp.getInnerWays().size(); k++) { + Way inner = mp.getInnerWays().get(k); + Way older = keepOlder(inner, oldestWayMap, discardedWays); + if (inner != older) { + mp.getInnerWays().set(k, older); + } + } + Way older = keepOlder(mp.outerWay, oldestWayMap, discardedWays); + if (older != mp.outerWay) { + Multipolygon mpNew = new Multipolygon(older); + mpNew.innerWays.addAll(mp.getInnerWays()); + polygons.set(i, mpNew); + } + } + commitCommands(marktr("Keep older versions")); + } // Delete the discarded inner ways @@ -740,4 +783,31 @@ return new JoinAreasResult(true, polygons); + } + + /** + * Create copy of given way using an older id so that we don't create a new way instead of a modified old one. + * @param way the way to check + * @param oldestWayMap nodes from old ways + * @param discardedWays collection of ways which will be deleted (modified) + * @return a copy of the way with an older id or the way itself + */ + private Way keepOlder(Way way, Map oldestWayMap, List discardedWays) { + Way oldest = null; + for (Node n : way.getNodes()) { + Way orig = oldestWayMap .get(n); + if (orig != null && (oldest == null || oldest.getUniqueId() > orig.getUniqueId()) + && discardedWays.contains(orig)) { + oldest = orig; + } + } + if (oldest != null) { + discardedWays.remove(oldest); + discardedWays.add(way); + Way copy = new Way(oldest); + copy.setNodes(way.getNodes()); + cmds.add(new ChangeCommand(oldest, copy)); + return copy; + } + return way; } @@ -1056,5 +1126,5 @@ * @param way way to split * @param nodes split points - * @return list of split ways (or original ways if no splitting is done). + * @return list of split ways (or original way if no splitting is done). */ private List splitWayOnNodes(Way way, Set nodes) { @@ -1373,7 +1443,12 @@ //TODO: ReverseWay and Combine way are really slow and we use them a lot here. This slows down large joins. List actionWays = new ArrayList<>(ways.size()); - + int oldestPos = 0; + Way oldest = ways.get(0).way; for (WayInPolygon way : ways) { actionWays.add(way.way); + if (oldest.isNew() || !way.way.isNew() && oldest.getUniqueId() > way.way.getUniqueId()) { + oldest = way.way; + oldestPos = actionWays.size() - 1; + } if (!way.insideToTheRight) { @@ -1383,4 +1458,7 @@ } } + + // see #9599: Help CombineWayAction to use the oldest way + Collections.rotate(actionWays, actionWays.size() - oldestPos); Pair result = CombineWayAction.combineWaysWorker(actionWays); Index: /trunk/test/data/regress/9599/ex5.osm =================================================================== --- /trunk/test/data/regress/9599/ex5.osm (revision 15883) +++ /trunk/test/data/regress/9599/ex5.osm (revision 15883) @@ -0,0 +1,32 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: /trunk/test/data/regress/9599/three_old.osm =================================================================== --- /trunk/test/data/regress/9599/three_old.osm (revision 15883) +++ /trunk/test/data/regress/9599/three_old.osm (revision 15883) @@ -0,0 +1,45 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: /trunk/test/unit/org/openstreetmap/josm/actions/JoinAreasActionTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/actions/JoinAreasActionTest.java (revision 15882) +++ /trunk/test/unit/org/openstreetmap/josm/actions/JoinAreasActionTest.java (revision 15883) @@ -52,4 +52,50 @@ /** + * Non-regression test for bug #9599. + * @throws IOException if any I/O error occurs + * @throws IllegalDataException if OSM parsing fails + */ + @Test + public void testTicket9599() throws IOException, IllegalDataException { + try (InputStream is = TestUtils.getRegressionDataStream(9599, "ex5.osm")) { + DataSet ds = OsmReader.parseDataSet(is, null); + Layer layer = new OsmDataLayer(ds, null, null); + MainApplication.getLayerManager().addLayer(layer); + try { + new JoinAreasAction(false).join(ds.getWays()); + Collection found = SearchAction.searchAndReturn("type:way", SearchMode.replace); + assertEquals(1, found.size()); + assertEquals(257786939, found.iterator().next().getUniqueId()); + } finally { + // Ensure we clean the place before leaving, even if test fails. + MainApplication.getLayerManager().removeLayer(layer); + } + } + } + + /** + * Non-regression test for bug #9599. + * @throws IOException if any I/O error occurs + * @throws IllegalDataException if OSM parsing fails + */ + @Test + public void testTicket9599Simple() throws IOException, IllegalDataException { + try (InputStream is = TestUtils.getRegressionDataStream(9599, "three_old.osm")) { + DataSet ds = OsmReader.parseDataSet(is, null); + Layer layer = new OsmDataLayer(ds, null, null); + MainApplication.getLayerManager().addLayer(layer); + try { + new JoinAreasAction(false).join(ds.getWays()); + Collection found = SearchAction.searchAndReturn("type:way", SearchMode.replace); + assertEquals(1, found.size()); + assertEquals(31567319, found.iterator().next().getUniqueId()); + } finally { + // Ensure we clean the place before leaving, even if test fails. + MainApplication.getLayerManager().removeLayer(layer); + } + } + } + + /** * Non-regression test for bug #10511. * @throws IOException if any I/O error occurs