From 2ec09724e3d3f20088f8eb5a31dedd307b293f2f Mon Sep 17 00:00:00 2001 From: Taylor Smock Date: Tue, 15 Oct 2019 13:50:39 -0600 Subject: [PATCH] Modify locks on MapWithAILayer such that we don't have to lock/unlock the dataset. Also implement destroyable in the main plugin (for future restartless updates?). Signed-off-by: Taylor Smock --- .gitlab-ci.yml | 2 +- build.gradle | 2 +- .../plugins/mapwithai/MapWithAIPlugin.java | 23 +++++++- .../mapwithai/backend/MapWithAIDataUtils.java | 14 ++--- .../mapwithai/backend/MapWithAILayer.java | 52 +++++++++++++++++-- .../commands/MapWithAIAddCommand.java | 22 ++++---- .../commands/MapWithAIAddComandTest.java | 2 - 7 files changed, 89 insertions(+), 28 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4106e37..94dd139 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -202,7 +202,7 @@ publish to / GitLab.com packages: name: GitLab.com / JOSM-MapWithAI packages url: https://gitlab.com/gokaart/JOSM_MapWithAI/-/packages script: - - ./gradlew publishPluginPublicationToGitlabRepository + - ./gradlew publishPluginPublicationToGitlabRepository publishAllPublicationsToGitlabRepository releaseToGitlab dependencies: - build only: diff --git a/build.gradle b/build.gradle index d40684e..7c7adcd 100644 --- a/build.gradle +++ b/build.gradle @@ -41,7 +41,7 @@ tasks.withType(JavaCompile).configureEach { def versions = [ awaitility: "4.0.1", errorprone: "2.3.3", - jacoco: "0.8.2", + jacoco: "0.8.5", jmockit: "1.46", junit: "5.5.2", pmd: "6.6.0", diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/MapWithAIPlugin.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/MapWithAIPlugin.java index b5b9e3e..67dc76f 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/MapWithAIPlugin.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/MapWithAIPlugin.java @@ -3,10 +3,13 @@ package org.openstreetmap.josm.plugins.mapwithai; import java.awt.Component; import java.lang.reflect.InvocationTargetException; +import java.util.Arrays; import java.util.LinkedHashMap; import java.util.Map; import java.util.Map.Entry; +import java.util.stream.Collectors; +import javax.swing.Action; import javax.swing.JMenu; import javax.swing.JMenuItem; @@ -20,11 +23,13 @@ import org.openstreetmap.josm.plugins.PluginInformation; import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAIAction; import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAIArbitraryAction; import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAIDataUtils; +import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAILayer; import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAIMoveAction; import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAIRemoteControl; +import org.openstreetmap.josm.tools.Destroyable; import org.openstreetmap.josm.tools.Logging; -public final class MapWithAIPlugin extends Plugin { +public final class MapWithAIPlugin extends Plugin implements Destroyable { /** The name of the plugin */ public static final String NAME = "MapWithAI"; private static String versionInfo; @@ -84,4 +89,20 @@ public final class MapWithAIPlugin extends Plugin { private static void setVersionInfo(String newVersionInfo) { versionInfo = newVersionInfo; } + + @Override + public void destroy() { + final JMenu dataMenu = MainApplication.getMenu().dataMenu; + final Map actions = Arrays.asList(dataMenu.getComponents()).stream() + .filter(component -> component instanceof JMenuItem).map(component -> (JMenuItem) component) + .collect(Collectors.toMap(JMenuItem::getAction, component -> component)); + for (final Entry action : actions.entrySet()) { + if (MENU_ENTRIES.containsKey(action.getKey().getClass())) { + dataMenu.remove(action.getValue()); + } + } + + MainApplication.getLayerManager().getLayersOfType(MapWithAILayer.class).stream() + .forEach(layer -> MainApplication.getLayerManager().removeLayer(layer)); + } } 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 f0422f2..d3a8980 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 @@ -14,6 +14,7 @@ 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; import org.openstreetmap.josm.data.Bounds; @@ -437,13 +438,12 @@ public final class MapWithAIDataUtils { if (mapWithAIBounds.parallelStream().filter(bbox::bounds).count() == 0) { pool.execute(() -> { final DataSet newData = getData(bbox); - synchronized (LAYER_LOCK) { - layer.unlock(); - try { - layer.mergeFrom(newData); - } finally { - layer.lock(); - } + Lock lock = layer.getLock(); + lock.lock(); + try { + layer.mergeFrom(newData); + } finally { + lock.unlock(); } }); } diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayer.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayer.java index d3c82f9..93fa302 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayer.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayer.java @@ -4,9 +4,16 @@ package org.openstreetmap.josm.plugins.mapwithai.backend; import static org.openstreetmap.josm.tools.I18n.tr; import java.io.File; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.stream.Collectors; +import javax.swing.Action; import javax.swing.JLabel; import javax.swing.JPanel; +import javax.swing.SwingConstants; import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.UploadPolicy; @@ -21,6 +28,7 @@ public class MapWithAILayer extends OsmDataLayer { private Integer maximumAddition = null; private String url = null; private Boolean switchLayers = null; + private final Lock lock; /** * Create a new MapWithAI layer @@ -31,11 +39,11 @@ public class MapWithAILayer extends OsmDataLayer { */ public MapWithAILayer(DataSet data, String name, File associatedFile) { super(data, name, associatedFile); - this.lock(); data.setUploadPolicy(UploadPolicy.BLOCKED); + lock = new MapLock(); } - // @Override only JOSM > 15323 + // @Override TODO remove comment on 2020-01-01 public String getChangesetSourceTag() { return "MapWithAI"; } @@ -70,17 +78,51 @@ public class MapWithAILayer extends OsmDataLayer { if (p instanceof JPanel) { final JPanel panel = (JPanel) p; if (maximumAddition != null) { - panel.add(new JLabel(tr("Maximum Additions: {0}", maximumAddition), JLabel.HORIZONTAL), + panel.add(new JLabel(tr("Maximum Additions: {0}", maximumAddition), SwingConstants.HORIZONTAL), GBC.eop().insets(15, 0, 0, 0)); } if (url != null) { - panel.add(new JLabel(tr("URL: {0}", url), JLabel.HORIZONTAL), GBC.eop().insets(15, 0, 0, 0)); + panel.add(new JLabel(tr("URL: {0}", url), SwingConstants.HORIZONTAL), GBC.eop().insets(15, 0, 0, 0)); } if (switchLayers != null) { - panel.add(new JLabel(tr("Switch Layers: {0}", switchLayers), JLabel.HORIZONTAL), + panel.add(new JLabel(tr("Switch Layers: {0}", switchLayers), SwingConstants.HORIZONTAL), GBC.eop().insets(15, 0, 0, 0)); } } return p; } + + @Override + public Action[] getMenuEntries() { + List actions = Arrays.asList(super.getMenuEntries()).stream() + .filter(action -> !(action instanceof LayerSaveAction) && !(action instanceof LayerSaveAsAction)) + .collect(Collectors.toList()); + return actions.toArray(new Action[0]); + } + + public Lock getLock() { + return lock; + } + + private class MapLock extends ReentrantLock { + private static final long serialVersionUID = 5441350396443132682L; + private boolean dataSetLocked; + + @Override + public void lock() { + super.lock(); + dataSetLocked = getDataSet().isLocked(); + if (dataSetLocked) { + getDataSet().unlock(); + } + } + + @Override + public void unlock() { + super.unlock(); + if (dataSetLocked) { + getDataSet().lock(); + } + } + } } diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddCommand.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddCommand.java index f02db65..3f260e8 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddCommand.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddCommand.java @@ -7,6 +7,7 @@ import java.awt.EventQueue; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.concurrent.locks.Lock; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.command.SequenceCommand; @@ -23,6 +24,7 @@ public class MapWithAIAddCommand extends Command implements Runnable { DataSet mapWithAI; Collection primitives; Command command = null; + Lock lock = null; /** * Add primitives from MapWithAI to the OSM data layer @@ -33,7 +35,7 @@ public class MapWithAIAddCommand extends Command implements Runnable { */ public MapWithAIAddCommand(MapWithAILayer mapWithAILayer, OsmDataLayer editLayer, Collection selection) { this(mapWithAILayer.getDataSet(), editLayer.getDataSet(), selection); - + lock = mapWithAILayer.getLock(); } /** @@ -68,10 +70,9 @@ public class MapWithAIAddCommand extends Command implements Runnable { throw new IllegalArgumentException(); } synchronized (this) { - final boolean locked = mapWithAI.isLocked(); try { - if (locked) { - mapWithAI.unlock(); + if (lock != null) { + lock.lock(); } final Command movePrimitivesCommand = new MovePrimitiveDataSetCommand(editable, mapWithAI, primitives); final List allPrimitives = new ArrayList<>(); @@ -82,8 +83,8 @@ public class MapWithAIAddCommand extends Command implements Runnable { } command.executeCommand(); } finally { - if (locked) { - mapWithAI.lock(); + if (lock != null) { + lock.unlock(); } } } @@ -104,10 +105,9 @@ public class MapWithAIAddCommand extends Command implements Runnable { @Override public void undoCommand() { - final boolean locked = mapWithAI.isLocked(); try { - if (locked) { - mapWithAI.unlock(); + if (lock != null) { + lock.lock(); } synchronized (this) { if (command != null) { @@ -115,8 +115,8 @@ public class MapWithAIAddCommand extends Command implements Runnable { } } } finally { - if (locked) { - mapWithAI.lock(); + if (lock != null) { + lock.unlock(); } } } diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java index b03da61..f452bc7 100644 --- a/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java +++ b/test/unit/org/openstreetmap/josm/plugins/mapwithai/commands/MapWithAIAddComandTest.java @@ -15,7 +15,6 @@ import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.Tag; import org.openstreetmap.josm.data.osm.Way; -import org.openstreetmap.josm.plugins.mapwithai.commands.MapWithAIAddCommand; import org.openstreetmap.josm.testutils.JOSMTestRules; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -46,7 +45,6 @@ public class MapWithAIAddComandTest { ds1.addPrimitive(way); } } - ds1.lock(); MapWithAIAddCommand command = new MapWithAIAddCommand(ds1, ds2, Arrays.asList(way1, way2)); command.executeCommand(); Assert.assertNotNull(ds2.getPrimitiveById(way1));