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 <taylor.smock@kaart.com>

.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:

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",

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<Action, Component> 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, Component> 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));
+    }
 }

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) {
+                    Lock lock = layer.getLock();
-                        layer.unlock();
+                    lock.lock();
                     try {
                         layer.mergeFrom(newData);
                     } finally {
-                            layer.lock();
+                        lock.unlock();
-                        }
                     }
                 });
             }

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<Action> 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();
+            }
+        }
+    }
 }

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<OsmPrimitive> 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<OsmPrimitive> 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) {
+                if (lock != null) {
-                    mapWithAI.unlock();
+                    lock.lock();
                 }
                 final Command movePrimitivesCommand = new MovePrimitiveDataSetCommand(editable, mapWithAI, primitives);
                 final List<OsmPrimitive> allPrimitives = new ArrayList<>();
@@ -82,8 +83,8 @@ public class MapWithAIAddCommand extends Command implements Runnable {
                 }
                 command.executeCommand();
             } finally {
-                if (locked) {
+                if (lock != null) {
-                    mapWithAI.lock();
+                    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) {
+            if (lock != null) {
-                mapWithAI.unlock();
+                lock.lock();
             }
             synchronized (this) {
                 if (command != null) {
@@ -115,8 +115,8 @@ public class MapWithAIAddCommand extends Command implements Runnable {
                 }
             }
         } finally {
-            if (locked) {
+            if (lock != null) {
-                mapWithAI.lock();
+                lock.unlock();
             }
         }
     }

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));