Changeset 10006 in josm for trunk/src/org

Timestamp:

2016-03-18T00:41:42+01:00 (9 years ago)

Author:

Don-vip

Message:

fix #12639 - NPE in History Window for primitives with more than 100 versions

Location:

trunk/src/org/openstreetmap/josm

Files:

• data/osm/history/HistoryOsmPrimitive.java (modified) (1 diff)
• gui/history/HistoryLoadTask.java (modified) (12 diffs)
• io/ChangesetQuery.java (modified) (3 diffs)
• io/OsmServerChangesetReader.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/data/osm/history/HistoryOsmPrimitive.java

@@ -180 +180 @@
     }
 
+    /**
+     * Returns the changeset for this history primitive.
+     * @return the changeset for this history primitive
+     */
     public Changeset getChangeset() {
         return changeset;
     }
 
+    /**
+     * Sets the changeset for this history primitive.
+     * @param changeset the changeset for this history primitive
+     */
     public void setChangeset(Changeset changeset) {
         this.changeset = changeset;

trunk/src/org/openstreetmap/josm/gui/history/HistoryLoadTask.java

@@ -7 +7 @@
 import java.awt.Component;
 import java.io.IOException;
-import java.text.MessageFormat;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 import org.openstreetmap.josm.data.osm.Changeset;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
-import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 import org.openstreetmap.josm.data.osm.PrimitiveId;
-import org.openstreetmap.josm.data.osm.SimplePrimitiveId;
 import org.openstreetmap.josm.data.osm.history.History;
 import org.openstreetmap.josm.data.osm.history.HistoryDataSet;
@@ -22 +21 @@
 import org.openstreetmap.josm.gui.ExceptionDialogUtil;
 import org.openstreetmap.josm.gui.PleaseWaitRunnable;
+import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.io.ChangesetQuery;
 import org.openstreetmap.josm.io.OsmServerChangesetReader;
@@ -30 +30 @@
 
 /**
- * Loads the object history of an collection of objects from the
- * server.
+ * Loads the object history of a collection of objects from the server.
  *
  * It provides a fluent API for configuration.
@@ -38 +37 @@
  *
  * <pre>
- *   HistoryLoadTask task  = new HistoryLoadTask()
- *      .add(1, OsmPrimitiveType.NODE)
- *      .add(1233, OsmPrimitiveType.WAY)
- *      .add(37234, OsmPrimitveType.RELATION)
+ *   HistoryLoadTask task = new HistoryLoadTask()
+ *      .add(node)
+ *      .add(way)
+ *      .add(relation)
  *      .add(aHistoryItem);
  *
  *   Main.worker.execute(task);
- *
  * </pre>
  */
@@ -52 +50 @@
     private boolean canceled;
     private Exception lastException;
-    private final Set<PrimitiveId> toLoad;
+    private final Set<PrimitiveId> toLoad = new HashSet<>();
     private HistoryDataSet loadedData;
     private OsmServerHistoryReader reader;
@@ -61 +59 @@
     public HistoryLoadTask() {
         super(tr("Load history"), true);
-        toLoad = new HashSet<>();
     }
 
@@ -75 +72 @@
         super(parent, tr("Load history"), true);
         CheckParameterUtil.ensureParameterNotNull(parent, "parent");
-        toLoad = new HashSet<>();
-    }
-
-    /**
-     * Adds an object whose history is to be loaded.
-     *
-     * @param id the object id
-     * @param type the object type
-     * @return this task
-     */
-    public HistoryLoadTask add(long id, OsmPrimitiveType type) {
-        if (id <= 0)
-            throw new IllegalArgumentException(MessageFormat.format("Parameter ''{0}'' > 0 expected. Got {1}.", "id", id));
-        CheckParameterUtil.ensureParameterNotNull(type, "type");
-        SimplePrimitiveId pid = new SimplePrimitiveId(id, type);
-        toLoad.add(pid);
-        return this;
     }
 
@@ -115 +95 @@
     public HistoryLoadTask add(HistoryOsmPrimitive primitive) {
         CheckParameterUtil.ensureParameterNotNull(primitive, "primitive");
-        toLoad.add(primitive.getPrimitiveId());
-        return this;
+        return add(primitive.getPrimitiveId());
     }
 
@@ -128 +107 @@
     public HistoryLoadTask add(History history) {
         CheckParameterUtil.ensureParameterNotNull(history, "history");
-        toLoad.add(history.getPrimitiveId());
-        return this;
+        return add(history.getPrimitiveId());
     }
 
@@ -142 +120 @@
     public HistoryLoadTask add(OsmPrimitive primitive) {
         CheckParameterUtil.ensureValidPrimitiveId(primitive, "primitive");
-        toLoad.add(primitive.getPrimitiveId());
-        return this;
+        return add(primitive.getPrimitiveId());
     }
 
@@ -194 +171 @@
                     break;
                 }
-                String msg = "";
-                switch(pid.getType()) {
-                case NODE: msg = marktr("Loading history for node {0}"); break;
-                case WAY: msg = marktr("Loading history for way {0}"); break;
-                case RELATION: msg = marktr("Loading history for relation {0}"); break;
-                }
-                progressMonitor.indeterminateSubTask(tr(msg,
-                        Long.toString(pid.getUniqueId())));
+                String msg = getLoadingMessage(pid);
+                progressMonitor.indeterminateSubTask(tr(msg, Long.toString(pid.getUniqueId())));
                 reader = null;
-                HistoryDataSet ds = null;
+                HistoryDataSet ds;
                 try {
                     reader = new OsmServerHistoryReader(pid.getType(), pid.getUniqueId());
-                    ds = reader.parseHistory(progressMonitor.createSubTaskMonitor(1, false));
-                    // load corresponding changesets (mostly for changeset comment)
-                    for (final Changeset i : new OsmServerChangesetReader().queryChangesets(
-                            new ChangesetQuery().forChangesetIds(ds.getChangesetIds()), progressMonitor.createSubTaskMonitor(1, false))) {
-                        ds.putChangeset(i);
-                    }
+                    ds = loadHistory(reader, progressMonitor);
                 } catch (OsmTransferException e) {
                     if (canceled)
@@ -225 +191 @@
     }
 
+    protected static HistoryDataSet loadHistory(OsmServerHistoryReader reader, ProgressMonitor progressMonitor) throws OsmTransferException {
+        HistoryDataSet ds = reader.parseHistory(progressMonitor.createSubTaskMonitor(1, false));
+        // load corresponding changesets (mostly for changeset comment)
+        OsmServerChangesetReader changesetReader = new OsmServerChangesetReader();
+        List<Long> changesetIds = new ArrayList<>(ds.getChangesetIds());
+
+        // query changesets 100 by 100 (OSM API limit)
+        int n = ChangesetQuery.MAX_CHANGESETS_NUMBER;
+        for (int i = 0; i < changesetIds.size(); i += n) {
+            for (Changeset c : changesetReader.queryChangesets(
+                    new ChangesetQuery().forChangesetIds(changesetIds.subList(i, Math.min(i + n, changesetIds.size()))),
+                    progressMonitor.createSubTaskMonitor(1, false))) {
+                ds.putChangeset(c);
+            }
+        }
+
+        return ds;
+    }
+
+    protected static String getLoadingMessage(PrimitiveId pid) {
+        switch (pid.getType()) {
+        case NODE:
+            return marktr("Loading history for node {0}");
+        case WAY:
+            return marktr("Loading history for way {0}");
+        case RELATION:
+            return marktr("Loading history for relation {0}");
+        default:
+            return "";
+        }
+    }
+
+    /**
+     * Determines if this task has ben canceled.
+     * @return {@code true} if this task has ben canceled
+     */
     public boolean isCanceled() {
         return canceled;
     }
 
+    /**
+     * Returns the last exception that occured during loading, if any.
+     * @return the last exception that occured during loading, or {@code null}
+     */
     public Exception getLastException() {
         return lastException;

trunk/src/org/openstreetmap/josm/io/ChangesetQuery.java

@@ -16 +16 @@
 import java.util.Map.Entry;
 
+import org.openstreetmap.josm.Main;
 import org.openstreetmap.josm.data.Bounds;
 import org.openstreetmap.josm.data.coor.LatLon;
@@ -23 +24 @@
 
 public class ChangesetQuery {
+
+    /**
+     * Maximum number of changesets returned by the OSM API call "/changesets?"
+     */
+    public static int MAX_CHANGESETS_NUMBER = 100;
 
     /**
@@ -234 +240 @@
     public ChangesetQuery forChangesetIds(Collection<Long> changesetIds) {
         CheckParameterUtil.ensureParameterNotNull(changesetIds, "changesetIds");
+        if (changesetIds.size() > MAX_CHANGESETS_NUMBER) {
+            Main.warn("Changeset query built with more than " + MAX_CHANGESETS_NUMBER + " changeset ids (" + changesetIds.size() + ")");
+        }
         this.changesetIds = changesetIds;
         return this;

trunk/src/org/openstreetmap/josm/io/OsmServerChangesetReader.java

@@ -73 +73 @@
             try (InputStream in = getInputStream(sb.toString(), monitor.createSubTaskMonitor(1, true))) {
                 if (in == null)
-                    return null;
+                    return Collections.emptyList();
                 monitor.indeterminateSubTask(tr("Downloading changesets ..."));
                 result = OsmChangesetParser.parse(in, monitor.createSubTaskMonitor(1, true));