EsriSourceReader: Lazily load replacement tags

Signed-off-by: Taylor Smock <tsmock@fb.com>

src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/GetDataRunnable.java

@@ -248,7 +248,7 @@ public class GetDataRunnable extends RecursiveTask<DataSet> {
 
     private static boolean checkIfPrimitiveDuplicatesPrimitiveInDataSet(OsmPrimitive primitive, DataSet ds) {
         final List<OsmPrimitive> possibleDuplicates = searchDataSet(ds, primitive);
-        return possibleDuplicates.parallelStream().filter(prim -> !prim.isDeleted())
+        return possibleDuplicates.stream().filter(prim -> !prim.isDeleted())
                 .anyMatch(prim -> checkIfProbableDuplicate(prim, primitive));
     }
 

src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java

@@ -4,11 +4,14 @@ package org.openstreetmap.josm.plugins.mapwithai.backend;
 import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
 import static org.openstreetmap.josm.tools.I18n.tr;
 
+import javax.swing.JOptionPane;
+
 import java.awt.geom.Area;
 import java.net.SocketTimeoutException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.TreeSet;
@@ -16,8 +19,6 @@ import java.util.concurrent.ForkJoinPool;
 import java.util.concurrent.locks.Lock;
 import java.util.stream.Collectors;
 
-import javax.swing.JOptionPane;
-
 import org.openstreetmap.josm.data.Bounds;
 import org.openstreetmap.josm.data.UndoRedoHandler;
 import org.openstreetmap.josm.data.coor.LatLon;
@@ -48,7 +49,7 @@ import org.openstreetmap.josm.tools.Utils;
  */
 public final class MapWithAIDataUtils {
     /** The maximum dimensions for MapWithAI data (in kilometers) */
-    public static final int MAXIMUM_SIDE_DIMENSIONS = 10_000; // RapiD is about 1km, max is 10km, but 10km causes
+    public static final int MAXIMUM_SIDE_DIMENSIONS = 10_000; // RapiD is about 1 km, max is 10 km, but 10 km causes
     // timeouts
     private static final int TOO_MANY_BBOXES = 4;
     private static ForkJoinPool forkJoinPool;
@@ -100,7 +101,7 @@ public final class MapWithAIDataUtils {
      * @return A DataSet with data inside the bounds
      */
     public static DataSet getData(Bounds bounds) {
-        return getData(Arrays.asList(bounds), MAXIMUM_SIDE_DIMENSIONS);
+        return getData(Collections.singletonList(bounds), MAXIMUM_SIDE_DIMENSIONS);
     }
 
     /**
@@ -112,7 +113,7 @@ public final class MapWithAIDataUtils {
      * @return A DataSet with data inside the bounds
      */
     public static DataSet getData(Bounds bounds, int maximumDimensions) {
-        return getData(Arrays.asList(bounds), maximumDimensions);
+        return getData(Collections.singletonList(bounds), maximumDimensions);
     }
 
     /**
@@ -287,7 +288,7 @@ public final class MapWithAIDataUtils {
      */
     public static boolean getMapWithAIData(MapWithAILayer layer) {
         final List<OsmDataLayer> osmLayers = MainApplication.getLayerManager().getLayersOfType(OsmDataLayer.class)
-                .stream().filter(obj -> !MapWithAILayer.class.isInstance(obj)).collect(Collectors.toList());
+                .stream().filter(obj -> !(obj instanceof MapWithAILayer)).collect(Collectors.toList());
         boolean gotData = false;
         for (final OsmDataLayer osmLayer : osmLayers) {
             if (!osmLayer.isLocked() && getMapWithAIData(layer, osmLayer)) {
@@ -343,7 +344,7 @@ public final class MapWithAIDataUtils {
                 } finally {
                     lock.unlock();
                 }
-                toDownload.stream().forEach(layer::onPostDownloadFromServer);
+                toDownload.forEach(layer::onPostDownloadFromServer);
             });
         }
         return !toDownload.isEmpty();
@@ -359,12 +360,12 @@ public final class MapWithAIDataUtils {
         // Lat is y, Lon is x
         final LatLon bottomLeft = bounds.getMin();
         final LatLon topRight = bounds.getMax();
-        final double minx = bottomLeft.getX();
+        final double minX = bottomLeft.getX();
-        final double maxx = topRight.getX();
+        final double maxX = topRight.getX();
-        final double miny = bottomLeft.getY();
+        final double minY = bottomLeft.getY();
-        final double maxy = topRight.getY();
+        final double maxY = topRight.getY();
-        final LatLon bottomRight = new LatLon(miny, maxx);
+        final LatLon bottomRight = new LatLon(minY, maxX);
-        final LatLon topLeft = new LatLon(maxy, minx);
+        final LatLon topLeft = new LatLon(maxY, minX);
         return Math.max(bottomLeft.greatCircleDistance(bottomRight), topLeft.greatCircleDistance(topRight));
     }
 
@@ -380,11 +381,10 @@ public final class MapWithAIDataUtils {
         final List<Bounds> returnBounds = new ArrayList<>();
         final double width = getWidth(bound);
         final double height = getHeight(bound);
-        final Double widthDivisions = width / maximumDimensions;
+        final double widthDivisions = width / maximumDimensions;
-        final Double heightDivisions = height / maximumDimensions;
+        final double heightDivisions = height / maximumDimensions;
-        final int widthSplits = widthDivisions.intValue() + ((widthDivisions - widthDivisions.intValue()) > 0 ? 1 : 0);
+        final int widthSplits = (int) widthDivisions + ((widthDivisions - Math.floor(widthDivisions)) > 0 ? 1 : 0);
-        final int heightSplits = heightDivisions.intValue()
+        final int heightSplits = (int) heightDivisions + ((heightDivisions - Math.floor(heightDivisions)) > 0 ? 1 : 0);
-                + ((heightDivisions - heightDivisions.intValue()) > 0 ? 1 : 0);
 
         final double newMinWidths = Math.abs(bound.getMaxLon() - bound.getMinLon()) / widthSplits;
         final double newMinHeights = Math.abs(bound.getMaxLat() - bound.getMinLat()) / heightSplits;

src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/mapwithai/MapWithAIInfo.java

@@ -3,6 +3,11 @@ package org.openstreetmap.josm.plugins.mapwithai.data.mapwithai;
 
 import static org.openstreetmap.josm.tools.I18n.tr;
 
+import javax.json.Json;
+import javax.json.JsonArray;
+import javax.json.JsonObject;
+import javax.json.stream.JsonParser;
+
 import java.io.Serializable;
 import java.io.StringReader;
 import java.util.ArrayList;
@@ -12,14 +17,10 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import javax.json.Json;
-import javax.json.JsonArray;
-import javax.json.JsonObject;
-import javax.json.stream.JsonParser;
-
 import org.openstreetmap.josm.data.StructUtils.StructEntry;
 import org.openstreetmap.josm.data.imagery.ImageryInfo;
 import org.openstreetmap.josm.data.imagery.ImageryInfo.ImageryBounds;
@@ -35,6 +36,7 @@ public class MapWithAIInfo extends
 
     private List<MapWithAICategory> categories;
     private JsonArray parameters;
+    private Supplier<Map<String, String>> replacementTagsSupplier;
     private Map<String, String> replacementTags;
     private boolean conflate;
     private String conflationUrl;
@@ -97,8 +99,8 @@ public class MapWithAIInfo extends
             if (i.conflationParameters != null) {
                 conflationParameters = i.conflationParameters.toString();
             }
-            if (i.replacementTags != null) {
+            if (i.getReplacementTags() != null) {
-                replacementTags = i.replacementTags;
+                this.replacementTags = i.getReplacementTags();
             }
             source = i.source;
             conflate = i.conflate;
@@ -439,6 +441,16 @@ public class MapWithAIInfo extends
         this.replacementTags = replacementTags;
     }
 
+    /**
+     * Set the required replacement tags (as a supplier -- used only to avoid making
+     * unnecessary requests)
+     *
+     * @param replacementTagsSupplier The supplier to get the tags to replace
+     */
+    public void setReplacementTags(final Supplier<Map<String, String>> replacementTagsSupplier) {
+        this.replacementTagsSupplier = replacementTagsSupplier;
+    }
+
     /**
      * Get the requested tags to replace. These should be run before user requested
      * replacements.
@@ -446,7 +458,15 @@ public class MapWithAIInfo extends
      * @return The required replacement tags
      */
     public Map<String, String> getReplacementTags() {
-        return replacementTags;
+        if (this.replacementTags == null && this.replacementTagsSupplier != null) {
+            synchronized (this) {
+                if (this.replacementTagsSupplier != null) {
+                    this.replacementTags = this.replacementTagsSupplier.get();
+                }
+                this.replacementTagsSupplier = null;
+            }
+        }
+        return this.replacementTags;
     }
 
     /**

src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/mapwithai/MapWithAILayerInfo.java

@@ -59,11 +59,11 @@ public class MapWithAILayerInfo {
     private final Map<String, MapWithAIInfo> layerIds = new HashMap<>();
     private final ListenerList<LayerChangeListener> listeners = ListenerList.create();
     /** List of all available default layers */
-    static final List<MapWithAIInfo> defaultLayers = new ArrayList<>();
+    static final List<MapWithAIInfo> defaultLayers = Collections.synchronizedList(new ArrayList<>());
     /** List of all available default layers (including mirrors) */
-    static final List<MapWithAIInfo> allDefaultLayers = new ArrayList<>();
+    static final List<MapWithAIInfo> allDefaultLayers = Collections.synchronizedList(new ArrayList<>());
     /** List of all layer ids of available default layers (including mirrors) */
-    static final Map<String, MapWithAIInfo> defaultLayerIds = new HashMap<>();
+    static final Map<String, MapWithAIInfo> defaultLayerIds = Collections.synchronizedMap(new HashMap<>());
 
     /** The prefix for configuration of the MapWithAI sources */
     public static final String CONFIG_PREFIX = "mapwithai.sources.";
@@ -345,16 +345,20 @@ public class MapWithAILayerInfo {
 
         protected void finish() {
             defaultLayers.clear();
-            allDefaultLayers.clear();
+            synchronized (allDefaultLayers) {
-            defaultLayers.addAll(newLayers);
+                allDefaultLayers.clear();
-            this.updateEsriLayers(newLayers);
+                defaultLayers.addAll(newLayers);
-            allDefaultLayers.sort(new MapWithAIInfo.MapWithAIInfoCategoryComparator());
+                this.updateEsriLayers(newLayers);
-            allDefaultLayers.sort(Comparator.comparing(TileSourceInfo::getName));
+                allDefaultLayers.sort(new MapWithAIInfo.MapWithAIInfoCategoryComparator());
-            allDefaultLayers.sort(Comparator.comparing(info -> info.getCategory().getDescription()));
+                allDefaultLayers.sort(Comparator.comparing(TileSourceInfo::getName));
-            allDefaultLayers.sort(Comparator
+                allDefaultLayers.sort(Comparator.comparing(info -> info.getCategory().getDescription()));
-                    .comparingInt(info -> -info.getAdditionalCategories().indexOf(MapWithAICategory.FEATURED)));
+                allDefaultLayers.sort(Comparator
-            defaultLayerIds.clear();
+                        .comparingInt(info -> -info.getAdditionalCategories().indexOf(MapWithAICategory.FEATURED)));
-            buildIdMap(allDefaultLayers, defaultLayerIds);
+                defaultLayerIds.clear();
+                synchronized (defaultLayerIds) {
+                    buildIdMap(allDefaultLayers, defaultLayerIds);
+                }
+            }
             updateEntriesFromDefaults(!loadError);
             buildIdMap(layers, layerIds);
             if (!loadError && !defaultLayerIds.isEmpty()) {

src/main/java/org/openstreetmap/josm/plugins/mapwithai/io/mapwithai/ESRISourceReader.java

@@ -36,6 +36,7 @@ import org.apache.commons.jcs3.engine.behavior.IElementAttributes;
 import org.openstreetmap.josm.data.cache.JCSCacheManager;
 import org.openstreetmap.josm.data.imagery.ImageryInfo.ImageryBounds;
 import org.openstreetmap.josm.data.preferences.LongProperty;
+import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.io.CachedFile;
 import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAICategory;
 import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIInfo;
@@ -150,7 +151,7 @@ public class ESRISourceReader {
         MapWithAIInfo newInfo = new MapWithAIInfo(source);
         newInfo.setId(feature.getString("id"));
         if (feature.getString("type", "").equals("Feature Service")) {
-            newInfo.setUrl(featureService(newInfo, feature.getString("url")));
+            MainApplication.worker.execute(() -> newInfo.setUrl(featureService(newInfo, feature.getString("url"))));
         } else {
             newInfo.setUrl(feature.getString("url"));
         }
@@ -278,7 +279,7 @@ public class ESRISourceReader {
                     .asJsonObject();
             if (layer.containsKey("id")) {
                 String partialUrl = (url.endsWith("/") ? url : url + "/") + layer.getInt("id");
-                mapwithaiInfo.setReplacementTags(getReplacementTags(partialUrl));
+                mapwithaiInfo.setReplacementTags(() -> getReplacementTags(partialUrl));
 
                 return partialUrl;
             }

test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/DownloadListenerTest.java

@@ -9,7 +9,6 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.Field;
 
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.awaitility.Awaitility;
 import org.awaitility.Durations;
 import org.junit.jupiter.api.Test;
@@ -22,21 +21,25 @@ import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAITestRules;
 import org.openstreetmap.josm.plugins.mapwithai.testutils.annotations.MapWithAISources;
 import org.openstreetmap.josm.plugins.mapwithai.testutils.annotations.NoExceptions;
+import org.openstreetmap.josm.plugins.mapwithai.testutils.annotations.Wiremock;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * Test class for {@link DownloadListener}
  *
  * @author Taylor Smock
  */
-@NoExceptions
 @BasicPreferences
+@NoExceptions
+@Wiremock
 @MapWithAISources
 class DownloadListenerTest {
     @RegisterExtension
     @SuppressFBWarnings("URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    JOSMTestRules rule = new MapWithAITestRules().projection();
+    JOSMTestRules rule = new MapWithAITestRules().projection().territories();
 
     @Test
     void testDataSourceChange() {
@@ -62,9 +65,11 @@ class DownloadListenerTest {
         continuousDownload.actionPerformed(null);
 
         // Test when MapWithAI layer is continuous downloading
+        assertTrue(layer.getDataSet().isEmpty());
         ds.addDataSource(new DataSource(bounds, "Test bounds 2"));
 
-        assertTrue(layer.getDataSet().isEmpty());
+        Awaitility.await().atMost(Durations.FIVE_SECONDS)
+                .until(() -> MapWithAIDataUtils.getForkJoinPool().isQuiescent());
         Awaitility.await().atMost(Durations.FIVE_SECONDS).until(() -> !layer.getDataSet().isEmpty());
         assertFalse(layer.getDataSet().isEmpty());
 

test/unit/org/openstreetmap/josm/plugins/mapwithai/data/mapwithai/MapWithAIInfoTest.java

@@ -3,20 +3,21 @@ package org.openstreetmap.josm.plugins.mapwithai.data.mapwithai;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import javax.json.Json;
+import javax.json.JsonArray;
+import javax.management.ReflectionException;
+
 import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
 import java.text.MessageFormat;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Random;
 import java.util.stream.Stream;
 
-import javax.json.Json;
-import javax.json.JsonArray;
-import javax.management.ReflectionException;
-
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
@@ -60,6 +61,8 @@ class MapWithAIInfoTest {
 
     @Test
     void testCloneInitializer() throws ReflectionException, IllegalArgumentException, IllegalAccessException {
+        final List<String> ignoredFields = Collections
+                .singletonList("replacementTagsSupplier" /* The supplier shouldn't be copied */);
         MapWithAIInfo orig = new MapWithAIInfo("Test info");
         // Use random to ensure that I do not accidentally introduce a dependency
         Random random = new Random();
@@ -90,8 +93,8 @@ class MapWithAIInfoTest {
             } else if (f.getType().isAssignableFrom(JsonArray.class)) {
                 JsonArray array = Json.createArrayBuilder().addNull().addNull().build();
                 f.set(orig, array);
-            } else {
+            } else if (!ignoredFields.contains(f.getName())) {
-                throw new IllegalArgumentException("Account for " + type.getSimpleName());
+                throw new IllegalArgumentException("Account for " + type.getSimpleName() + " " + f.getName());
             }
         }
 

test/unit/org/openstreetmap/josm/plugins/mapwithai/io/mapwithai/ESRISourceReaderTest.java

@@ -8,12 +8,15 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.concurrent.Future;
 
-import com.github.tomakehurst.wiremock.WireMockServer;
+import org.awaitility.Awaitility;
+import org.awaitility.Durations;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIInfo;
 import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIType;
 import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAITestRules;
@@ -21,6 +24,8 @@ import org.openstreetmap.josm.plugins.mapwithai.testutils.annotations.Wiremock;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 import org.openstreetmap.josm.testutils.annotations.BasicWiremock;
 
+import com.github.tomakehurst.wiremock.WireMockServer;
+
 @Wiremock
 class ESRISourceReaderTest {
 
@@ -56,6 +61,9 @@ class ESRISourceReaderTest {
             info.setUrl(url);
             final ESRISourceReader reader = new ESRISourceReader(info);
             Collection<MapWithAIInfo> layers = reader.parse();
+            Future<?> workerQueue = MainApplication.worker.submit(() -> {
+                /* Sync threads */});
+            Awaitility.await().atMost(Durations.FIVE_SECONDS).until(workerQueue::isDone);
             assertFalse(layers.isEmpty(), "There should be a MapWithAI layer");
             assertTrue(layers.stream().noneMatch(i -> info.getUrl().equals(i.getUrl())),
                     "The ESRI server should be expanded to feature servers");