Index: /trunk/src/org/openstreetmap/josm/gui/MapView.java =================================================================== --- /trunk/src/org/openstreetmap/josm/gui/MapView.java (revision 8550) +++ /trunk/src/org/openstreetmap/josm/gui/MapView.java (revision 8551) @@ -28,5 +28,7 @@ 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,5 +107,14 @@ } + /** + * 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,5 +191,12 @@ } - 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,5 +204,6 @@ } - protected static void fireLayerAdded(Layer newLayer) { + protected void fireLayerAdded(Layer newLayer) { + checkLayerLockNotHeld(); for (MapView.LayerChangeListener l : MapView.layerChangeListeners) { l.layerAdded(newLayer); @@ -192,5 +211,6 @@ } - protected static void fireLayerRemoved(Layer layer) { + protected void fireLayerRemoved(Layer layer) { + checkLayerLockNotHeld(); for (MapView.LayerChangeListener l : MapView.layerChangeListeners) { l.layerRemoved(layer); @@ -198,5 +218,6 @@ } - 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,7 +226,26 @@ /** - * 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 layers = new ArrayList<>(); + + /** + * This lock manages concurrent access to {@link #layers}, + * {@link #editLayer} and {@link #activeLayer}. + *

+ * 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,8 +254,11 @@ /** - * 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,23 +351,30 @@ /** * Adds a GPX layer. A GPX layer is added below the lowest data layer. + *

+ * 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,37 +385,53 @@ */ 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,26 +440,41 @@ @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,26 +512,54 @@ */ public void removeLayer(Layer layer) { - List 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 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,22 +585,49 @@ */ 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,29 +642,34 @@ * first, layer with the highest Z-Order last. */ - protected List getVisibleLayersInZOrder() { - List 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() { - @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 getVisibleLayersInZOrder() { + layerLock.readLock().lock(); + try { + List 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() { + @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,5 +845,10 @@ */ public Collection getAllLayers() { - return Collections.unmodifiableCollection(new ArrayList<>(layers)); + layerLock.readLock().lock(); + try { + return Collections.unmodifiableCollection(new ArrayList<>(layers)); + } finally { + layerLock.readLock().unlock(); + } } @@ -711,5 +857,10 @@ */ public List getAllLayersAsList() { - return Collections.unmodifiableList(new ArrayList<>(layers)); + layerLock.readLock().lock(); + try { + return Collections.unmodifiableList(new ArrayList<>(layers)); + } finally { + layerLock.readLock().unlock(); + } } @@ -722,4 +873,5 @@ * * + * @param ofType The layer type. * @return an unmodifiable list of layers of a certain type. */ @@ -729,10 +881,15 @@ /** - * 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,8 +903,31 @@ } - private void setEditLayer(List layersList) { - OsmDataLayer newEditLayer = layersList.contains(editLayer) ? editLayer : null; - OsmDataLayer oldEditLayer = editLayer; - + /** + * Sets the active edit layer. + *

+ * You must own a write {@link #layerLock} when calling this method. + * @param layersList A list to select that layer from. + * @return true if the edit layer was really changed and the listeners should be informed. + */ + private boolean setEditLayer(List 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 layersList) { + OsmDataLayer newEditLayer = layersList.contains(editLayer)?editLayer:null; // Find new edit layer if (activeLayer != editLayer || !layersList.contains(editLayer)) { @@ -763,16 +943,5 @@ } } - - // Set new edit layer - if (newEditLayer != editLayer) { - if (newEditLayer == null) { - getCurrentDataSet().setSelected(); - } - - editLayer = newEditLayer; - fireEditLayerChanged(oldEditLayer, newEditLayer); - refreshTitle(); - } - + return newEditLayer; } @@ -785,13 +954,35 @@ */ 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 true, 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,5 +991,24 @@ 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,20 +1017,16 @@ * 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,13 +1035,4 @@ /** - * 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,5 +1040,10 @@ */ public OsmDataLayer getEditLayer() { - return editLayer; + layerLock.readLock().lock(); + try { + return editLayer; + } finally { + layerLock.readLock().unlock(); + } } @@ -853,5 +1055,10 @@ */ public boolean hasLayer(Layer layer) { - return layers.contains(layer); + layerLock.readLock().lock(); + try { + return layers.contains(layer); + } finally { + layerLock.readLock().unlock(); + } } @@ -884,10 +1091,19 @@ } + /** + * 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,8 +1130,13 @@ 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,4 +1152,5 @@ /** * Get a string representation of all layers suitable for the {@code source} changeset tag. + * @return A String of sources separated by ';' */ public String getLayerInformationForSourceTag() {