Changeset 8551 in josm

Timestamp:

2015-07-01T20:55:07+02:00 (10 years ago)

Author:

bastiK

Message:

applied #11631 new enhancement - Synchronized access to MapView#layers (patch by michael2402)

File:

• trunk/src/org/openstreetmap/josm/gui/MapView.java (modified) (29 diffs)

trunk/src/org/openstreetmap/josm/gui/MapView.java

@@ -28 +28 @@
 import java.util.LinkedList;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import javax.swing.AbstractButton;
@@ -105 +107 @@
     }
 
+    /**
+     * An interface that needs to be implemented in order to listen for changes to the active edit layer.
+     */
     public interface EditLayerChangeListener {
+
+        /**
+         * Called after the active edit layer was changed.
+         * @param oldLayer The old edit layer
+         * @param newLayer The current (new) edit layer
+         */
         void editLayerChanged(OsmDataLayer oldLayer, OsmDataLayer newLayer);
     }
@@ -180 +191 @@
     }
 
-    protected static void fireActiveLayerChanged(Layer oldLayer, Layer newLayer) {
+    /**
+     * Calls the {@link LayerChangeListener#activeLayerChange(Layer, Layer)} method of all listeners.
+     *
+     * @param oldLayer The old layer
+     * @param newLayer The new active layer.
+     */
+    protected void fireActiveLayerChanged(Layer oldLayer, Layer newLayer) {
+        checkLayerLockNotHeld();
         for (LayerChangeListener l : layerChangeListeners) {
             l.activeLayerChange(oldLayer, newLayer);
@@ -186 +204 @@
     }
 
-    protected static void fireLayerAdded(Layer newLayer) {
+    protected void fireLayerAdded(Layer newLayer) {
+        checkLayerLockNotHeld();
         for (MapView.LayerChangeListener l : MapView.layerChangeListeners) {
             l.layerAdded(newLayer);
@@ -192 +211 @@
     }
 
-    protected static void fireLayerRemoved(Layer layer) {
+    protected void fireLayerRemoved(Layer layer) {
+        checkLayerLockNotHeld();
         for (MapView.LayerChangeListener l : MapView.layerChangeListeners) {
             l.layerRemoved(layer);
@@ -198 +218 @@
     }
 
-    protected static void fireEditLayerChanged(OsmDataLayer oldLayer, OsmDataLayer newLayer) {
+    protected void fireEditLayerChanged(OsmDataLayer oldLayer, OsmDataLayer newLayer) {
+        checkLayerLockNotHeld();
         for (EditLayerChangeListener l : editLayerChangeListeners) {
             l.editLayerChanged(oldLayer, newLayer);
@@ -205 +226 @@
 
     /**
-     * A list of all layers currently loaded.
+     * This is a simple invariant check that tests if the {@link #layerLock} is not write locked.
+     * This should be the case whenever a layer listener is invoked.
+     */
+    private void checkLayerLockNotHeld() {
+        if (layerLock.isWriteLockedByCurrentThread()) {
+            Main.warn("layerLock is write-held while a listener was called.");
+        }
+    }
+
+    /**
+     * A list of all layers currently loaded. Locked by {@link #layerLock}.
      */
     private final transient List<Layer> layers = new ArrayList<>();
+
+    /**
+     * This lock manages concurrent access to {@link #layers},
+     * {@link #editLayer} and {@link #activeLayer}.
+     * <p>
+     * The read lock is always held while those fields are read or while layer change listeners are fired.
+     */
+    private final ReentrantReadWriteLock layerLock = new ReentrantReadWriteLock();
+
     /**
      * The play head marker: there is only one of these so it isn't in any specific layer
@@ -214 +254 @@
 
     /**
-     * The layer from the layers list that is currently active.
+     * The layer from the layers list that is currently active. Locked by {@link #layerLock}.
      */
     private transient Layer activeLayer;
 
+    /**
+     * The edit layer is the current active data layer. Locked by {@link #layerLock}.
+     */
     private transient OsmDataLayer editLayer;
 
@@ -308 +351 @@
     /**
      * Adds a GPX layer. A GPX layer is added below the lowest data layer.
+     * <p>
+     * Does not call {@link #fireLayerAdded(Layer)}.
      *
      * @param layer the GPX layer
      */
     protected void addGpxLayer(GpxLayer layer) {
-        if (layers.isEmpty()) {
-            layers.add(layer);
-            return;
-        }
-        for (int i = layers.size()-1; i >= 0; i--) {
-            if (layers.get(i) instanceof OsmDataLayer) {
-                if (i == layers.size()-1) {
-                    layers.add(layer);
-                } else {
-                    layers.add(i+1, layer);
+        layerLock.writeLock().lock();
+        try {
+            if (layers.isEmpty()) {
+                layers.add(layer);
+                return;
+            }
+            for (int i = layers.size()-1; i >= 0; i--) {
+                if (layers.get(i) instanceof OsmDataLayer) {
+                    if (i == layers.size()-1) {
+                        layers.add(layer);
+                    } else {
+                        layers.add(i+1, layer);
+                    }
+                    return;
                 }
-                return;
-            }
-        }
-        layers.add(0, layer);
+            }
+            layers.add(0, layer);
+        } finally {
+            layerLock.writeLock().unlock();
+        }
     }
 
@@ -335 +385 @@
      */
     public void addLayer(Layer layer) {
-        if (layer instanceof MarkerLayer && playHeadMarker == null) {
-            playHeadMarker = PlayHeadMarker.create();
-        }
-
-        if (layer instanceof GpxLayer) {
-            addGpxLayer((GpxLayer) layer);
-        } else if (layers.isEmpty()) {
-            layers.add(layer);
-        } else if (layer.isBackgroundLayer()) {
-            int i = 0;
-            for (; i < layers.size(); i++) {
-                if (layers.get(i).isBackgroundLayer()) {
-                    break;
+        boolean isOsmDataLayer = layer instanceof OsmDataLayer;
+        layerLock.writeLock().lock();
+        layerLock.readLock().lock();
+        boolean fireSetActiveLayer = false;
+        Layer oldActiveLayer = activeLayer;
+        try {
+            try {
+                if (layer instanceof MarkerLayer && playHeadMarker == null) {
+                    playHeadMarker = PlayHeadMarker.create();
                 }
-            }
-            layers.add(i, layer);
-        } else {
-            layers.add(0, layer);
-        }
-        fireLayerAdded(layer);
-        boolean isOsmDataLayer = layer instanceof OsmDataLayer;
-        if (isOsmDataLayer) {
-            ((OsmDataLayer) layer).addLayerStateChangeListener(this);
-        }
-        boolean callSetActiveLayer = isOsmDataLayer || activeLayer == null;
-        if (callSetActiveLayer) {
-            // autoselect the new layer
-            setActiveLayer(layer); // also repaints this MapView
-        }
-        layer.addPropertyChangeListener(this);
-        Main.addProjectionChangeListener(layer);
-        AudioPlayer.reset();
-        if (!callSetActiveLayer) {
+
+                if (layer instanceof GpxLayer) {
+                    addGpxLayer((GpxLayer) layer);
+                } else if (layers.isEmpty()) {
+                    layers.add(layer);
+                } else if (layer.isBackgroundLayer()) {
+                    int i = 0;
+                    for (; i < layers.size(); i++) {
+                        if (layers.get(i).isBackgroundLayer()) {
+                            break;
+                        }
+                    }
+                    layers.add(i, layer);
+                } else {
+                    layers.add(0, layer);
+                }
+
+                if (isOsmDataLayer || oldActiveLayer == null) {
+                    // autoselect the new layer
+                    fireSetActiveLayer = setActiveLayer(layer, true);
+                }
+            } finally {
+                layerLock.writeLock().unlock();
+            }
+
+            fireLayerAdded(layer);
+            if (isOsmDataLayer) {
+                ((OsmDataLayer) layer).addLayerStateChangeListener(this);
+            }
+            if (fireSetActiveLayer) {
+                onActiveLayerChanged(oldActiveLayer);
+            }
+            layer.addPropertyChangeListener(this);
+            Main.addProjectionChangeListener(layer);
+            AudioPlayer.reset();
+        } finally {
+            layerLock.readLock().unlock();
+        }
+        if (!fireSetActiveLayer) {
             repaint();
         }
@@ -374 +440 @@
     @Override
     protected DataSet getCurrentDataSet() {
-        if (editLayer != null)
-            return editLayer.data;
-        else
-            return null;
-    }
-
-    /**
-     * Replies true if the active layer is drawable.
-     *
-     * @return true if the active layer is drawable, false otherwise
+        layerLock.readLock().lock();
+        try {
+            if (editLayer != null)
+                return editLayer.data;
+            else
+                return null;
+        } finally {
+            layerLock.readLock().unlock();
+        }
+    }
+
+    /**
+     * Replies true if the active data layer (edit layer) is drawable.
+     *
+     * @return true if the active data layer (edit layer) is drawable, false otherwise
      */
     public boolean isActiveLayerDrawable() {
-        return editLayer != null;
-    }
-
-    /**
-     * Replies true if the active layer is visible.
-     *
-     * @return true if the active layer is visible, false otherwise
+        layerLock.readLock().lock();
+        try {
+            return editLayer != null;
+        } finally {
+            layerLock.readLock().unlock();
+        }
+    }
+
+    /**
+     * Replies true if the active data layer (edit layer) is visible.
+     *
+     * @return true if the active data layer (edit layer) is visible, false otherwise
      */
     public boolean isActiveLayerVisible() {
-        return isActiveLayerDrawable() && editLayer.isVisible();
+        layerLock.readLock().lock();
+        try {
+            return isActiveLayerDrawable() && editLayer.isVisible();
+        } finally {
+            layerLock.readLock().unlock();
+        }
     }
 
@@ -431 +512 @@
      */
     public void removeLayer(Layer layer) {
-        List<Layer> layersList = new ArrayList<>(layers);
-
-        if (!layersList.remove(layer))
-            return;
-
-        setEditLayer(layersList);
-
-        if (layer == activeLayer) {
-            setActiveLayer(determineNextActiveLayer(layersList), false);
-        }
-
-        if (layer instanceof OsmDataLayer) {
-            ((OsmDataLayer) layer).removeLayerPropertyChangeListener(this);
-        }
-
-        layers.remove(layer);
-        Main.removeProjectionChangeListener(layer);
-        fireLayerRemoved(layer);
-        layer.removePropertyChangeListener(this);
-        layer.destroy();
-        AudioPlayer.reset();
+        boolean fireEditLayerChanged;
+        boolean fireSetActiveLayer = false;
+        layerLock.writeLock().lock();
+        layerLock.readLock().lock();
+
+        OsmDataLayer oldEditLayer = editLayer;
+        Layer oldActiveLayer = activeLayer;
+
+        try {
+            try {
+                List<Layer> layersList = new ArrayList<>(layers);
+
+                if (!layersList.remove(layer))
+                    return;
+
+                fireEditLayerChanged = setEditLayer(layersList);
+
+                if (layer == activeLayer) {
+                    fireSetActiveLayer = setActiveLayer(determineNextActiveLayer(layersList), false);
+                }
+
+                if (layer instanceof OsmDataLayer) {
+                    ((OsmDataLayer) layer).removeLayerPropertyChangeListener(this);
+                }
+
+                layers.remove(layer);
+                Main.removeProjectionChangeListener(layer);
+
+            } finally {
+                layerLock.writeLock().unlock();
+            }
+            if (fireEditLayerChanged) {
+               onEditLayerChanged(oldEditLayer);
+            }
+            if (fireSetActiveLayer) {
+                onActiveLayerChanged(oldActiveLayer);
+            }
+            fireLayerRemoved(layer);
+            layer.removePropertyChangeListener(this);
+            layer.destroy();
+            AudioPlayer.reset();
+        } finally {
+            layerLock.readLock().unlock();
+        }
         repaint();
+    }
+
+    private void onEditLayerChanged(OsmDataLayer oldEditLayer) {
+        fireEditLayerChanged(oldEditLayer, editLayer);
+        refreshTitle();
     }
 
@@ -476 +585 @@
      */
     public void moveLayer(Layer layer, int pos) {
-        int curLayerPos = layers.indexOf(layer);
-        if (curLayerPos == -1)
-            throw new IllegalArgumentException(tr("Layer not in list."));
-        if (pos == curLayerPos)
-            return; // already in place.
-        layers.remove(curLayerPos);
-        if (pos >= layers.size()) {
-            layers.add(layer);
-        } else {
-            layers.add(pos, layer);
-        }
-        setEditLayer(layers);
-        AudioPlayer.reset();
+        layerLock.writeLock().lock();
+        layerLock.readLock().lock();
+        boolean fireEditLayerChanged;
+        OsmDataLayer oldEditLayer = editLayer;
+        try {
+            try {
+                int curLayerPos = layers.indexOf(layer);
+                if (curLayerPos == -1)
+                    throw new IllegalArgumentException(tr("Layer not in list."));
+                if (pos == curLayerPos)
+                    return; // already in place.
+                layers.remove(curLayerPos);
+                if (pos >= layers.size()) {
+                    layers.add(layer);
+                } else {
+                    layers.add(pos, layer);
+                }
+                fireEditLayerChanged = setEditLayer(layers);
+            } finally {
+                layerLock.writeLock().unlock();
+            }
+            if (fireEditLayerChanged) {
+                onEditLayerChanged(editLayer);
+            }
+            AudioPlayer.reset();
+        } finally {
+            layerLock.readLock().unlock();
+        }
         repaint();
     }
 
+    /**
+     * Gets the index of the layer in the layer list.
+     * @param layer The layer to search for.
+     * @return The index in the list.
+     * @throws IllegalArgumentException if that layer does not belong to this view.
+     */
     public int getLayerPos(Layer layer) {
-        int curLayerPos = layers.indexOf(layer);
+        int curLayerPos;
+        layerLock.readLock().lock();
+        try {
+            curLayerPos = layers.indexOf(layer);
+        } finally {
+            layerLock.readLock().unlock();
+        }
         if (curLayerPos == -1)
             throw new IllegalArgumentException(tr("Layer not in list."));
@@ -506 +642 @@
      * first, layer with the highest Z-Order last.
      */
-    protected List<Layer> getVisibleLayersInZOrder() {
-        List<Layer> ret = new ArrayList<>();
-        for (Layer l: layers) {
-            if (l.isVisible()) {
-                ret.add(l);
-            }
-        }
-        // sort according to position in the list of layers, with one exception:
-        // an active data layer always becomes a higher Z-Order than all other data layers
-        Collections.sort(
-                ret,
-                new Comparator<Layer>() {
-                    @Override
-                    public int compare(Layer l1, Layer l2) {
-                        if (l1 instanceof OsmDataLayer && l2 instanceof OsmDataLayer) {
-                            if (l1 == getActiveLayer()) return -1;
-                            if (l2 == getActiveLayer()) return 1;
-                            return Integer.compare(layers.indexOf(l1), layers.indexOf(l2));
-                        } else
-                            return Integer.compare(layers.indexOf(l1), layers.indexOf(l2));
+    public List<Layer> getVisibleLayersInZOrder() {
+        layerLock.readLock().lock();
+        try {
+            List<Layer> ret = new ArrayList<>();
+            for (Layer l: layers) {
+                if (l.isVisible()) {
+                    ret.add(l);
+                }
+            }
+            // sort according to position in the list of layers, with one exception:
+            // an active data layer always becomes a higher Z-Order than all other data layers
+            Collections.sort(
+                    ret,
+                    new Comparator<Layer>() {
+                        @Override
+                        public int compare(Layer l1, Layer l2) {
+                            if (l1 instanceof OsmDataLayer && l2 instanceof OsmDataLayer) {
+                                if (l1 == getActiveLayer()) return -1;
+                                if (l2 == getActiveLayer()) return 1;
+                                return Integer.compare(layers.indexOf(l1), layers.indexOf(l2));
+                            } else
+                                return Integer.compare(layers.indexOf(l1), layers.indexOf(l2));
+                        }
                     }
-                }
-        );
-        Collections.reverse(ret);
-        return ret;
+            );
+            Collections.reverse(ret);
+            return ret;            
+        } finally {
+            layerLock.readLock().unlock();
+        }
     }
 
@@ -704 +845 @@
      */
     public Collection<Layer> getAllLayers() {
-        return Collections.unmodifiableCollection(new ArrayList<>(layers));
+        layerLock.readLock().lock();
+        try {
+            return Collections.unmodifiableCollection(new ArrayList<>(layers));
+        } finally {
+            layerLock.readLock().unlock();
+        }
     }
 
@@ -711 +857 @@
      */
     public List<Layer> getAllLayersAsList() {
-        return Collections.unmodifiableList(new ArrayList<>(layers));
+        layerLock.readLock().lock();
+        try {
+            return Collections.unmodifiableList(new ArrayList<>(layers));
+        } finally {
+            layerLock.readLock().unlock();
+        }
     }
 
@@ -722 +873 @@
      * </pre>
      *
+     * @param ofType The layer type.
      * @return an unmodifiable list of layers of a certain type.
      */
@@ -729 +881 @@
 
     /**
-     * Replies the number of layers managed by this mav view
-     *
-     * @return the number of layers managed by this mav view
+     * Replies the number of layers managed by this map view
+     *
+     * @return the number of layers managed by this map view
      */
     public int getNumLayers() {
-        return layers.size();
+        layerLock.readLock().lock();
+        try {
+            return layers.size();
+        } finally {
+            layerLock.readLock().unlock();
+        }
     }
 
@@ -746 +903 @@
     }
 
-    private void setEditLayer(List<Layer> layersList) {
-        OsmDataLayer newEditLayer = layersList.contains(editLayer) ? editLayer : null;
-        OsmDataLayer oldEditLayer = editLayer;
-
+    /**
+     * Sets the active edit layer.
+     * <p>
+     * You must own a write {@link #layerLock} when calling this method.
+     * @param layersList A list to select that layer from.
+     * @return <code>true</code> if the edit layer was really changed and the listeners should be informed.
+     */
+    private boolean setEditLayer(List<Layer> layersList) {
+        final OsmDataLayer newEditLayer = findNewEditLayer(layersList);
+
+        // Set new edit layer
+        if (newEditLayer != editLayer) {
+            if (newEditLayer == null) {
+                // Note: Unsafe to call while layer write lock is held.
+                getCurrentDataSet().setSelected();
+            }
+
+            editLayer = newEditLayer;
+            return true;
+        } else {
+            return false;
+        }
+
+    }
+
+    private OsmDataLayer findNewEditLayer(List<Layer> layersList) {
+        OsmDataLayer newEditLayer = layersList.contains(editLayer)?editLayer:null;
         // Find new edit layer
         if (activeLayer != editLayer || !layersList.contains(editLayer)) {
@@ -763 +943 @@
             }
         }
-
-        // Set new edit layer
-        if (newEditLayer != editLayer) {
-            if (newEditLayer == null) {
-                getCurrentDataSet().setSelected();
-            }
-
-            editLayer = newEditLayer;
-            fireEditLayerChanged(oldEditLayer, newEditLayer);
-            refreshTitle();
-        }
-
+        return newEditLayer;
     }
 
@@ -785 +954 @@
      */
     public void setActiveLayer(Layer layer) {
-        setActiveLayer(layer, true);
-    }
-
-    private void setActiveLayer(Layer layer, boolean setEditLayer) {
+        layerLock.writeLock().lock();
+        layerLock.readLock().lock();
+        Layer oldActiveLayer = activeLayer;
+        try {
+            boolean fireSetActiveLayer;
+            try {
+                fireSetActiveLayer = setActiveLayer(layer, true);
+            } finally {
+                layerLock.writeLock().unlock();
+            }
+            if (fireSetActiveLayer) {
+                onActiveLayerChanged(oldActiveLayer);
+            }
+        } finally {
+            layerLock.readLock().unlock();
+        }
+        repaint();
+    }
+
+    /**
+     * Sets the active layer. Propagates this change to all map buttons.
+     * @param layer The layer to be active.
+     * @param setEditLayer if this is <code>true</code>, the edit layer is also set.
+     * @return
+     */
+    private boolean setActiveLayer(final Layer layer, boolean setEditLayer) {
         if (layer != null && !layers.contains(layer))
             throw new IllegalArgumentException(tr("Layer ''{0}'' must be in list of layers", layer.toString()));
 
         if (layer == activeLayer)
-            return;
+            return false;
 
         Layer old = activeLayer;
@@ -800 +991 @@
             setEditLayer(layers);
         }
-        fireActiveLayerChanged(old, layer);
+
+        return true;
+    }
+
+    /**
+     * Replies the currently active layer
+     *
+     * @return the currently active layer (may be null)
+     */
+    public Layer getActiveLayer() {
+        layerLock.readLock().lock();
+        try {
+            return activeLayer;
+        } finally {
+            layerLock.readLock().unlock();
+        }
+    }
+
+    private void onActiveLayerChanged(final Layer old) {
+        fireActiveLayerChanged(old, activeLayer);
 
         /* This only makes the buttons look disabled. Disabling the actions as well requires
@@ -807 +1017 @@
          * order. This way it works as visual clue for new users */
         for (final AbstractButton b: Main.map.allMapModeButtons) {
-            MapMode mode = (MapMode) b.getAction();
-            if (mode.layerIsSupported(layer)) {
+            MapMode mode = (MapMode)b.getAction();
+            final boolean activeLayerSupported = mode.layerIsSupported(activeLayer);
+            if (activeLayerSupported) {
                 Main.registerActionShortcut(mode, mode.getShortcut()); //fix #6876
-                GuiHelper.runInEDTAndWait(new Runnable() {
-                    @Override public void run() {
-                        b.setEnabled(true);
-                    }
-                });
             } else {
                 Main.unregisterShortcut(mode.getShortcut());
-                GuiHelper.runInEDTAndWait(new Runnable() {
-                    @Override public void run() {
-                        b.setEnabled(false);
-                    }
-                });
-            }
+            }
+            GuiHelper.runInEDTAndWait(new Runnable() {
+                @Override public void run() {
+                    b.setEnabled(activeLayerSupported);
+                }
+            });
         }
         AudioPlayer.reset();
@@ -829 +1035 @@
 
     /**
-     * Replies the currently active layer
-     *
-     * @return the currently active layer (may be null)
-     */
-    public Layer getActiveLayer() {
-        return activeLayer;
-    }
-
-    /**
      * Replies the current edit layer, if any
      *
@@ -843 +1040 @@
      */
     public OsmDataLayer getEditLayer() {
-        return editLayer;
+        layerLock.readLock().lock();
+        try {
+            return editLayer;
+        } finally {
+            layerLock.readLock().unlock();
+        }
     }
 
@@ -853 +1055 @@
      */
     public boolean hasLayer(Layer layer) {
-        return layers.contains(layer);
+        layerLock.readLock().lock();
+        try {
+            return layers.contains(layer);
+        } finally {
+            layerLock.readLock().unlock();
+        }
     }
 
@@ -884 +1091 @@
     }
 
+    /**
+     * Sets the title of the JOSM main window, adding a star if there are dirty layers.
+     * @see Main#parent
+     */
     protected void refreshTitle() {
         if (Main.parent != null) {
-            boolean dirty = editLayer != null &&
-                    (editLayer.requiresSaveToFile() || (editLayer.requiresUploadToServer() && !editLayer.isUploadDiscouraged()));
-            ((JFrame) Main.parent).setTitle((dirty ? "* " : "") + tr("Java OpenStreetMap Editor"));
-            ((JFrame) Main.parent).getRootPane().putClientProperty("Window.documentModified", dirty);
+            layerLock.readLock().lock();
+            try {
+                boolean dirty = editLayer != null &&
+                        (editLayer.requiresSaveToFile() || (editLayer.requiresUploadToServer() && !editLayer.isUploadDiscouraged()));
+                ((JFrame) Main.parent).setTitle((dirty ? "* " : "") + tr("Java OpenStreetMap Editor"));
+                ((JFrame) Main.parent).getRootPane().putClientProperty("Window.documentModified", dirty);
+            } finally {
+                layerLock.readLock().unlock();
+            }
         }
     }
@@ -914 +1130 @@
             mapMover.destroy();
         }
-        activeLayer = null;
-        changedLayer = null;
-        editLayer = null;
-        layers.clear();
+        layerLock.writeLock().lock();
+        try {
+            activeLayer = null;
+            changedLayer = null;
+            editLayer = null;
+            layers.clear();
+        } finally {
+            layerLock.writeLock().unlock();
+        }
         nonChangedLayers.clear();
         temporaryLayers.clear();
@@ -931 +1152 @@
     /**
      * Get a string representation of all layers suitable for the {@code source} changeset tag.
+     * @return A String of sources separated by ';'
      */
     public String getLayerInformationForSourceTag() {