From 8641bfc357da4857825c2a42b586a155233f4f66 Mon Sep 17 00:00:00 2001 From: Taylor Smock Date: Tue, 15 Oct 2019 15:56:08 -0600 Subject: [PATCH] Improve threading and popups Signed-off-by: Taylor Smock --- .classpath | 1 + .../mapwithai/backend/MapWithAIAction.java | 12 +- .../backend/MapWithAIAvailability.java | 20 ++-- .../mapwithai/backend/MapWithAIDataUtils.java | 110 ++++++++++-------- .../mapwithai/backend/MapWithAILayerTest.java | 5 + .../backend/MapWithAIRemoteControlTest.java | 6 + 6 files changed, 95 insertions(+), 59 deletions(-) diff --git a/.classpath b/.classpath index 710897b..b335141 100644 --- a/.classpath +++ b/.classpath @@ -18,5 +18,6 @@ + diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAction.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAction.java index ca8f1af..befe04f 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAction.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAction.java @@ -5,9 +5,11 @@ import static org.openstreetmap.josm.tools.I18n.tr; import java.awt.event.ActionEvent; import java.awt.event.KeyEvent; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import java.util.TreeMap; import java.util.stream.Collectors; @@ -15,7 +17,10 @@ import javax.swing.JOptionPane; import org.openstreetmap.josm.actions.JosmAction; import org.openstreetmap.josm.data.Bounds; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.Notification; +import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin; import org.openstreetmap.josm.tools.Shortcut; @@ -48,7 +53,12 @@ public class MapWithAIAction extends JosmAction { final MapWithAILayer layer = MapWithAIDataUtils.getLayer(false); if (layer != null) { final Notification notification = new Notification(); - final List bounds = layer.getDataSet().getDataSourceBounds(); + final List bounds = new ArrayList<>(layer.getDataSet().getDataSourceBounds()); + if (bounds.isEmpty()) { + MainApplication.getLayerManager().getLayersOfType(OsmDataLayer.class).stream() + .map(OsmDataLayer::getDataSet).filter(Objects::nonNull).map(DataSet::getDataSourceBounds) + .forEach(bounds::addAll); + } final StringBuilder message = new StringBuilder(); message.append(MapWithAIPlugin.NAME); message.append(": "); diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAvailability.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAvailability.java index 8beee71..f8620ea 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAvailability.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIAvailability.java @@ -49,17 +49,15 @@ public final class MapWithAIAvailability { private MapWithAIAvailability() { try (CachedFile cachedRapidReleases = new CachedFile(rapidReleases); JsonParser parser = Json.createParser(cachedRapidReleases.getContentReader())) { - final JsonParser.Event event = parser.next(); - if (JsonParser.Event.START_OBJECT.equals(event)) { - final Stream> entries = parser.getObjectStream(); - final Optional> objects = entries - .filter(entry -> "objects".equals(entry.getKey())).findFirst(); - if (objects.isPresent()) { - final JsonObject value = objects.get().getValue().asJsonObject(); - final JsonObject centroid = value.getJsonObject("rapid_releases_1011_centroid"); - final JsonArray countries = centroid.getJsonArray("geometries"); - parseForCountries(countries); - } + parser.next(); + final Stream> entries = parser.getObjectStream(); + final Optional> objects = entries + .filter(entry -> "objects".equals(entry.getKey())).findFirst(); + if (objects.isPresent()) { + final JsonObject value = objects.get().getValue().asJsonObject(); + final JsonObject centroid = value.getJsonObject("rapid_releases_1011_centroid"); + final JsonArray countries = centroid.getJsonArray("geometries"); + parseForCountries(countries); } } catch (IOException e) { Logging.debug(e); diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java index d3a8980..7f9c923 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java @@ -3,17 +3,19 @@ package org.openstreetmap.josm.plugins.mapwithai.backend; import static org.openstreetmap.josm.tools.I18n.tr; +import java.awt.EventQueue; import java.io.IOException; import java.io.InputStream; +import java.lang.reflect.InvocationTargetException; import java.net.URL; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.List; +import java.util.Objects; import java.util.TreeSet; import java.util.concurrent.ForkJoinPool; import java.util.concurrent.RecursiveTask; -import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.Lock; import java.util.stream.Collectors; @@ -76,6 +78,7 @@ public final class MapWithAIDataUtils { return layer; } + /** * Get a dataset from the API servers using a bbox * @@ -83,54 +86,74 @@ public final class MapWithAIDataUtils { * @return A DataSet with data inside the bbox */ public static DataSet getData(BBox bbox) { - final DataSet dataSet = new DataSet(); - if (bbox.isValid()) { - final PleaseWaitProgressMonitor monitor = new PleaseWaitProgressMonitor(); - monitor.setCancelable(Boolean.FALSE); - monitor.beginTask(tr("Downloading {0} data", MapWithAIPlugin.NAME)); - monitor.indeterminateSubTask(null); - new ForkJoinPool().invoke(new GetDataRunnable(bbox, dataSet)); // TODO use an application level pool - monitor.finishTask(); - monitor.close(); - } + return getData(Arrays.asList(bbox)); + } + + /** + * Get a dataset from the API servers using a list bboxes + * + * @param bbox The bboxes from which to get data + * @return A DataSet with data inside the bboxes + */ + public static DataSet getData(List bbox) { + final DataSet dataSet = new DataSet(); + final List realBBoxes = bbox.stream().filter(BBox::isValid).collect(Collectors.toList()); + final PleaseWaitProgressMonitor monitor = new PleaseWaitProgressMonitor(); + try { + EventQueue.invokeAndWait(() -> { + monitor.setCancelable(Boolean.FALSE); + monitor.beginTask(tr("Downloading {0} data", MapWithAIPlugin.NAME)); + monitor.indeterminateSubTask(null); + }); + } catch (InvocationTargetException e) { + Logging.debug(e); + } catch (InterruptedException e) { + Logging.debug(e); + Thread.currentThread().interrupt(); + } + ForkJoinPool.commonPool().invoke(new GetDataRunnable(realBBoxes, dataSet, monitor)); - /* Microsoft buildings don't have a source, so we add one */ - MapWithAIDataUtils.addSourceTags(dataSet, "building", "Microsoft"); return dataSet; } private static class GetDataRunnable extends RecursiveTask { private static final long serialVersionUID = 258423685658089715L; - private final transient BBox bbox; + private final transient List bbox; private final transient DataSet dataSet; + private final transient PleaseWaitProgressMonitor monitor; - public GetDataRunnable(BBox bbox, DataSet dataSet) { + public GetDataRunnable(BBox bbox, DataSet dataSet, PleaseWaitProgressMonitor monitor) { + this(Arrays.asList(bbox), dataSet, monitor); + } + + public GetDataRunnable(List bbox, DataSet dataSet, PleaseWaitProgressMonitor monitor) { this.bbox = bbox; this.dataSet = dataSet; + this.monitor = monitor; } @Override public DataSet compute() { List bboxes = reduceBBoxSize(bbox); if (bboxes.size() == 1) { - final DataSet temporaryDataSet = getDataReal(getBbox()); + final DataSet temporaryDataSet = getDataReal(bboxes.get(0)); synchronized (MapWithAIDataUtils.GetDataRunnable.class) { dataSet.mergeFrom(temporaryDataSet); } } else { Collection tasks = bboxes.parallelStream() - .map(tBbox -> new GetDataRunnable(tBbox, getRawResult())).collect(Collectors.toList()); + .map(tBbox -> new GetDataRunnable(tBbox, dataSet, null)).collect(Collectors.toList()); tasks.forEach(GetDataRunnable::fork); tasks.forEach(GetDataRunnable::join); } - return dataSet; - } + if (Objects.nonNull(monitor)) { + monitor.finishTask(); + monitor.close(); + } - /** - * @return The {@code BBox} associated with this object - */ - public BBox getBbox() { - return bbox; + /* Microsoft buildings don't have a source, so we add one */ + MapWithAIDataUtils.addSourceTags(dataSet, "building", "Microsoft"); + return dataSet; } private static DataSet getDataReal(BBox bbox) { @@ -371,6 +394,12 @@ public final class MapWithAIDataUtils { } } + public static List reduceBBoxSize(List bboxes) { + List returnBBoxes = new ArrayList<>(); + bboxes.forEach(bbox -> returnBBoxes.addAll(reduceBBoxSize(bbox))); + return returnBBoxes; + } + public static List reduceBBoxSize(BBox bbox) { final List returnBounds = new ArrayList<>(); final double width = getWidth(bbox); @@ -432,30 +461,17 @@ public final class MapWithAIDataUtils { final List editSetBBoxes = bboxes.stream() .filter(bbox -> mapWithAIBounds.stream().noneMatch(tBBox -> tBBox.bounds(bbox))) .collect(Collectors.toList()); - final ForkJoinPool pool = new ForkJoinPool(); - for (final BBox bbox : editSetBBoxes) { - // TODO remove bounds that are already downloaded - if (mapWithAIBounds.parallelStream().filter(bbox::bounds).count() == 0) { - pool.execute(() -> { - final DataSet newData = getData(bbox); - Lock lock = layer.getLock(); - lock.lock(); - try { - layer.mergeFrom(newData); - } finally { - lock.unlock(); - } - }); + ForkJoinPool.commonPool().execute(() -> { + layer.getDataSet().clear(); + final DataSet newData = getData(editSetBBoxes); + Lock lock = layer.getLock(); + lock.lock(); + try { + layer.mergeFrom(newData); + } finally { + lock.unlock(); } - } - pool.shutdown(); - try { - pool.awaitTermination(10, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Logging.debug(e); - Thread.currentThread().interrupt(); - } - + }); } /** diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayerTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayerTest.java index 0b0b691..a5476a0 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayerTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayerTest.java @@ -2,11 +2,13 @@ package org.openstreetmap.josm.plugins.mapwithai.backend; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.awaitility.Awaitility.await; import static org.openstreetmap.josm.tools.I18n.tr; import java.awt.Component; import java.util.Arrays; import java.util.List; +import java.util.concurrent.TimeUnit; import javax.swing.JLabel; import javax.swing.JPanel; @@ -121,11 +123,14 @@ public class MapWithAILayerTest { osm.unlock(); MapWithAIDataUtils.getMapWithAIData(mapWithAILayer); + await().atMost(10, TimeUnit.SECONDS).until(() -> !mapWithAILayer.getDataSet().getDataSourceBounds().isEmpty()); Assert.assertFalse(mapWithAILayer.getDataSet().getDataSourceBounds().isEmpty()); Assert.assertEquals(1, mapWithAILayer.getDataSet().getDataSourceBounds().parallelStream().distinct().count()); osm.getDataSet().addDataSource(new DataSource(new Bounds(-0.001, -0.001, 0, 0), "random test")); MapWithAIDataUtils.getMapWithAIData(mapWithAILayer); + await().atMost(10, TimeUnit.SECONDS).until( + () -> mapWithAILayer.getDataSet().getDataSourceBounds().parallelStream().distinct().count() == 2); Assert.assertEquals(2, mapWithAILayer.getDataSet().getDataSourceBounds().parallelStream().distinct().count()); MapWithAIDataUtils.getMapWithAIData(mapWithAILayer); diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIRemoteControlTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIRemoteControlTest.java index 337aa2a..203445a 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIRemoteControlTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIRemoteControlTest.java @@ -2,6 +2,9 @@ package org.openstreetmap.josm.plugins.mapwithai.backend; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.awaitility.Awaitility.await; + +import java.util.concurrent.TimeUnit; import org.junit.Assert; import org.junit.Before; @@ -124,6 +127,9 @@ public class MapWithAIRemoteControlTest { newHandler("http://127.0.0.1:8111/mapwithai?bbox={bbox}".replace("{bbox}", temp.toStringCSV(","))).handle(); Assert.assertFalse(MainApplication.getLayerManager().getLayersOfType(MapWithAILayer.class).isEmpty()); + await().atMost(10, TimeUnit.SECONDS) + .until(() -> !MapWithAIDataUtils.getLayer(false).getDataSet().getDataSourceBounds().isEmpty()); + final BBox added = MapWithAIDataUtils.getLayer(false).getDataSet().getDataSourceBounds().iterator().next().toBBox(); Assert.assertTrue(temp.bounds(added));