Ticket #18566: 18566-alpha.patch

src/org/openstreetmap/josm/gui/io/AbstractPrimitiveTask.java

@@ -20 +20 @@
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.gui.progress.swing.PleaseWaitProgressMonitor;
 import org.openstreetmap.josm.gui.util.GuiHelper;
+import org.openstreetmap.josm.io.MultiFetchOverpassObjectReader;
 import org.openstreetmap.josm.io.MultiFetchServerObjectReader;
 import org.openstreetmap.josm.io.OsmServerObjectReader;
 import org.openstreetmap.josm.io.OsmTransferException;
@@ -41 +42 @@
     protected OsmServerObjectReader objectReader;
 
     private boolean zoom;
-    private boolean downloadRelations;
     private boolean fullRelation;
 
     protected AbstractPrimitiveTask(String title, OsmDataLayer layer) {
@@ -70 +70 @@
     }
 
     /**
-     * Sets whether .
-     * @param downloadRelations {@code true} if
+     * Sets whether all members of the relation should be downloaded completely.
+     * @param downloadRelations ignored since xxx
      * @param fullRelation {@code true} if a full download is required,
      *                     i.e., a download including the immediate children of a relation.
      * @return {@code this}
      */
     public final AbstractPrimitiveTask setDownloadRelations(boolean downloadRelations, boolean fullRelation) {
-        this.downloadRelations = downloadRelations;
         this.fullRelation = fullRelation;
         return this;
     }
@@ -103 +102 @@
                 multiObjectReader = MultiFetchServerObjectReader.create();
             }
             initMultiFetchReader(multiObjectReader);
+            if (multiObjectReader instanceof MultiFetchOverpassObjectReader) {
+                ((MultiFetchOverpassObjectReader) multiObjectReader).setRecurseDownRelations(fullRelation);
+            }
             theirDataSet = multiObjectReader.parseOsm(progressMonitor.createSubTaskMonitor(ProgressMonitor.ALL_TICKS, false));
             missingPrimitives = multiObjectReader.getMissingPrimitives();
             synchronized (this) {
@@ -110 +112 @@
             }
             new DataSetMerger(ds, theirDataSet).merge();
 
-            if (downloadRelations) {
+            if (fullRelation) {
                 loadIncompleteRelationMembers();
             }
 

src/org/openstreetmap/josm/gui/io/UpdatePrimitivesTask.java

@@ -5 +5 @@
 
 import java.util.Collection;
 import java.util.Collections;
+import java.util.stream.Collectors;
 
-import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
-import org.openstreetmap.josm.data.osm.Relation;
-import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.io.MultiFetchServerObjectReader;
 
@@ -32 +30 @@
     public UpdatePrimitivesTask(OsmDataLayer layer, Collection<? extends OsmPrimitive> toUpdate) {
         super(tr("Update objects"), layer);
         this.toUpdate = toUpdate != null ? toUpdate : Collections.<OsmPrimitive>emptyList();
+        this.setDownloadRelations(false, false);
     }
 
-    protected void initMultiFetchReaderWithNodes(MultiFetchServerObjectReader reader) {
-        getProgressMonitor().indeterminateSubTask(tr("Initializing nodes to update ..."));
-        for (OsmPrimitive primitive : toUpdate) {
-            if (primitive instanceof Node && !primitive.isNew()) {
-                reader.append(primitive);
-            }
-        }
-    }
-
-    protected void initMultiFetchReaderWithWays(MultiFetchServerObjectReader reader) {
-        getProgressMonitor().indeterminateSubTask(tr("Initializing ways to update ..."));
-        for (OsmPrimitive primitive : toUpdate) {
-            if (primitive instanceof Way && !primitive.isNew()) {
-                // this also adds way nodes
-                reader.append(primitive);
-            }
-        }
-    }
-
-    protected void initMultiFetchReaderWithRelations(MultiFetchServerObjectReader reader) {
-        getProgressMonitor().indeterminateSubTask(tr("Initializing relations to update ..."));
-        for (OsmPrimitive primitive : toUpdate) {
-            if (primitive instanceof Relation && !primitive.isNew()) {
-                // this also adds relation members
-                reader.append(primitive);
-            }
-        }
-    }
-
     @Override
     protected void initMultiFetchReader(MultiFetchServerObjectReader reader) {
-        initMultiFetchReaderWithNodes(reader);
-        initMultiFetchReaderWithWays(reader);
-        initMultiFetchReaderWithRelations(reader);
+        reader.append(toUpdate.stream().filter(p -> !p.isNew()).collect(Collectors.toList()));
     }
 }

src/org/openstreetmap/josm/io/MultiFetchOverpassObjectReader.java

@@ -5 +5 @@
 import java.util.stream.Collectors;
 
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
+import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Utils;
 
 /**
@@ -12 +13 @@
  *
  * @since 9241
  */
-class MultiFetchOverpassObjectReader extends MultiFetchServerObjectReader {
+public class MultiFetchOverpassObjectReader extends MultiFetchServerObjectReader {
+    private boolean recurseDownRelations = true;
 
     @Override
     protected String buildRequestString(final OsmPrimitiveType type, Set<Long> idPackage) {
-        final String query = idPackage.stream()
-                .map(x -> type.getAPIName() + '(' + x + ");>;")
-                .collect(Collectors.joining("", "(", ");out meta;"));
+        final String query = type.getAPIName() + "(id:"
+                    + idPackage.stream().map(String::valueOf).collect(Collectors.joining(",")) + ");"
+                    + getRecurseOption(type) + "out meta;";
+        Logging.debug("{0} {1}", "Overpass query:", query);
         return "interpreter?data=" + Utils.encodeUrl(query);
     }
 
+    private String getRecurseOption(final OsmPrimitiveType type) {
+        if (type == OsmPrimitiveType.WAY)
+            return ">;";
+        if (type == OsmPrimitiveType.RELATION && recurseDownRelations)
+            return ">>;"; // >>; needed for sub relations
+        return "";
+    }
+
     @Override
     protected String getBaseUrl() {
         return OverpassDownloadReader.OVERPASS_SERVER.get();
@@ -33 +44 @@
         // accomplished using >; in the query string above
         return true;
     }
+
+    /**
+     * Should generated queries recurse down on relations?
+     * @param recurseDownRelations true: yes, recurse down to retrieve the complete relation
+     */
+    public void setRecurseDownRelations(boolean recurseDownRelations) {
+        this.recurseDownRelations = recurseDownRelations;
+    }
+
 }

src/org/openstreetmap/josm/io/MultiFetchServerObjectReader.java

@@ -382 +382 @@
         progressMonitor.beginTask(trn("Downloading {0} object from ''{1}''",
                 "Downloading {0} objects from ''{1}''", n, n, OsmApi.getOsmApi().getBaseUrl()));
         try {
+            boolean ignoreAlreadyDownloaded = outputDataSet != null && outputDataSet.isEmpty();
             missingPrimitives = new HashSet<>();
             if (isCanceled()) return null;
+            fetchPrimitives(relations, OsmPrimitiveType.RELATION, progressMonitor);
+            if (isCanceled()) return null;
             fetchPrimitives(ways, OsmPrimitiveType.WAY, progressMonitor);
+            if (outputDataSet != null && ignoreAlreadyDownloaded) {
+                // avoid to download data that was already downloaded
+                nodes.removeAll(outputDataSet.getNodes().stream().filter(node -> !node.isIncomplete()).map(OsmPrimitive::getUniqueId).collect(Collectors.toList()));
+            }
             if (isCanceled()) return null;
             fetchPrimitives(nodes, OsmPrimitiveType.NODE, progressMonitor);
-            if (isCanceled()) return null;
-            fetchPrimitives(relations, OsmPrimitiveType.RELATION, progressMonitor);
             if (outputDataSet != null) {
                 outputDataSet.deleteInvisible();
             }

test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java

@@ -42 +42 @@
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.spi.preferences.Config;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
-import org.openstreetmap.josm.testutils.JOSMTestRules;
 
 /**
  * Unit tests of {@link MultiFetchServerObjectReader}.

test/unit/org/openstreetmap/josm/io/MultiFetchOverpassObjectReaderTest.java

@@ -6 +6 @@
 import java.util.Arrays;
 import java.util.TreeSet;
 
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.junit.Rule;
 import org.junit.Test;
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
@@ -13 +12 @@
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 import org.openstreetmap.josm.tools.Utils;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * Unit tests of {@link MultiFetchOverpassObjectReader}.
  */
@@ -29 +30 @@
      * Test {@link MultiFetchOverpassObjectReader#buildRequestString}
      */
     @Test
-    public void testBuildRequestString() {
+    public void testBuildRequestWaysString() {
         String requestString = new MultiFetchOverpassObjectReader()
                 .buildRequestString(OsmPrimitiveType.WAY, new TreeSet<>(Arrays.asList(130L, 123L, 126L)));
-        assertEquals("interpreter?data=" + Utils.encodeUrl("(way(123);>;way(126);>;way(130);>;);out meta;"), requestString);
+        assertEquals("interpreter?data=" + Utils.encodeUrl("way(id:123,126,130);>;out meta;"), requestString);
     }
 
+    /**
+     * Test {@link MultiFetchOverpassObjectReader#buildRequestString}
+     */
+    @Test
+    public void testBuildRequestRelationsString() {
+        MultiFetchOverpassObjectReader reader = new MultiFetchOverpassObjectReader();
+        reader.setRecurseDownRelations(true);
+        String requestString = reader.buildRequestString(OsmPrimitiveType.RELATION,
+                new TreeSet<>(Arrays.asList(130L, 123L, 126L)));
+        assertEquals("interpreter?data=" + Utils.encodeUrl("relation(id:123,126,130);>>;out meta;"), requestString);
+        reader.setRecurseDownRelations(false);
+        requestString = reader.buildRequestString(OsmPrimitiveType.RELATION,
+                new TreeSet<>(Arrays.asList(130L, 123L, 126L)));
+        assertEquals("interpreter?data=" + Utils.encodeUrl("relation(id:123,126,130);out meta;"), requestString);
+    }
+
 }