Changeset 18895 in josm for trunk/src

Timestamp:

2023-11-07T14:21:38+01:00 (12 months ago)

Author:

taylor.smock

Message:

Fix #23057: data layers should be selected next, not non-data layers (patch by gaben, modified)

Modifications are as follows:

• Fix test pollution uncovered by change in non-obvious determinate order of tests
• Fix code so that MainLayerManagerTest.testRemoveLayerUnsetsActiveLayer passes

Location:

trunk/src/org/openstreetmap/josm/gui

Files:

• dialogs/layer/DeleteLayerAction.java (modified) (3 diffs)
• layer/Layer.java (modified) (11 diffs)
• layer/MainLayerManager.java (modified) (4 diffs)

trunk/src/org/openstreetmap/josm/gui/dialogs/layer/DeleteLayerAction.java

@@ -6 +6 @@
 import java.awt.Component;
 import java.awt.event.ActionEvent;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 
 import javax.swing.AbstractAction;
@@ -48 +46 @@
         if (!SaveLayersDialog.saveUnsavedModifications(selectedLayers, SaveLayersDialog.Reason.DELETE))
             return;
-        final Map<Integer, Layer> layerMap = model.selectedIndices().filter(i -> model.getLayer(i) != null)
-                .collect(HashMap::new, (map, value) -> map.put(value, model.getLayer(value)), HashMap::putAll);
         for (Layer l: selectedLayers) {
             if (model.getLayerManager().containsLayer(l)) {
@@ -55 +51 @@
                 // this is why we need to check if every layer is still in the list of selected layers.
                 model.getLayerManager().removeLayer(l);
-            }
-        }
-        // Set the next active layer to the next visible layer
-        if (layerMap.size() == 1) {
-            final int selected = Math.min(layerMap.keySet().iterator().next(), model.getRowCount() - 1);
-            int currentLayerIndex = selected;
-            Layer layer = model.getLayer(currentLayerIndex);
-            // If the user has the last layer selected, we need to wrap around.
-            boolean reversed = false;
-            while (layer != null && !layer.isVisible() && currentLayerIndex < model.getRowCount() && currentLayerIndex >= 0) {
-                if (reversed) {
-                    currentLayerIndex--;
-                } else {
-                    currentLayerIndex++;
-                }
-                if (currentLayerIndex == model.getRowCount()) {
-                    reversed = true;
-                    currentLayerIndex = selected;
-                }
-                layer = model.getLayer(currentLayerIndex);
-            }
-            if (layer != null) {
-                model.getLayerManager().setActiveLayer(layer);
-                // Reset the selection
-                model.getSelectionModel().setSelectionInterval(selected, selected);
             }
         }

trunk/src/org/openstreetmap/josm/gui/layer/Layer.java

@@ -36 +36 @@
 /**
  * A layer encapsulates the gui component of one dataset and its representation.
- *
+ * <p>
  * Some layers may display data directly imported from OSM server. Other only
  * display background images. Some can be edited, some not. Some are static and
  * other changes dynamically (auto-updated).
- *
+ * <p>
  * Layers can be visible or not. Most actions the user can do applies only on
  * selected layers. The available actions depend on the selected layers too.
- *
+ * <p>
  * All layers are managed by the MapView. They are displayed in a list to the
  * right of the screen.
@@ -175 +175 @@
     /**
      * Initialization code, that depends on Main.map.mapView.
-     *
+     * <p>
      * It is always called in the event dispatching thread.
      * Note that Main.map is null as long as no layer has been added, so do
      * not execute code in the constructor, that assumes Main.map.mapView is
      * not null.
-     *
+     * <p>
      * If you need to execute code when this layer is added to the map view, use
      * {@link #attachToMapView(org.openstreetmap.josm.gui.layer.MapViewPaintable.MapViewEvent)}
@@ -270 +270 @@
      * menu component than JMenuItem or when it supports multiple layers. Actions that support multiple layers should also
      * have correct equals implementation.
-     *
+     * <p>
      * Use {@link SeparatorLayerAction#INSTANCE} instead of new JSeparator
      * @return menu actions for this layer
@@ -278 +278 @@
     /**
      * Called, when the layer is removed from the mapview and is going to be destroyed.
-     *
-     * This is because the Layer constructor can not add itself safely as listener
+     * <p>
+     * This is because the Layer constructor cannot add itself safely as a listener
      * to the layerlist dialog, because there may be no such dialog yet (loaded
      * via command line parameter).
@@ -303 +303 @@
     /**
      * Sets the associated file for this layer.
-     *
+     * <p>
      * The associated file might be the one that the user opened.
      * @param file The file, may be <code>null</code>
@@ -354 +354 @@
 
     /**
-     * Replies true if this layer was renamed by user
-     *
-     * @return true if this layer was renamed by user
+     * Replies true if user renamed this layer
+     *
+     * @return true if user renamed this layer
      */
     public boolean isRenamed() {
@@ -484 +484 @@
 
     /**
-     * fires a property change for the property {@link #FILTER_STATE_PROP}.
+     * Fires a property change for the property {@link #FILTER_STATE_PROP}.
      */
     protected void fireFilterStateChanged() {
@@ -491 +491 @@
 
     /**
-     * allows to check whether a projection is supported or not
+     * Allows to check whether a projection is supported or not.
      * @param proj projection
-     *
      * @return True if projection is supported for this layer
      */
@@ -610 +609 @@
 
     /**
-     * Replies the savable state of this layer (i.e if it can be saved through a "File-&gt;Save" dialog).
+     * Replies the savable state of this layer (i.e., if it can be saved through a "File-&gt;Save" dialog).
      * @return true if this layer can be saved to a file
      * @since 5459
@@ -628 +627 @@
 
     /**
-     * Creates a new "Save" dialog for this layer and makes it visible.<br>
-     * When the user has chosen a file, checks the file extension, and confirms overwrite if needed.
+     * Creates a new "Save" dialog for this layer and makes it visible.
+     * <p>
+     * When the user has chosen a file, checks the file extension, and confirms overwriting if needed.
      * @return The output {@code File}
      * @see SaveActionBase#createAndOpenSaveFileChooser
@@ -675 +675 @@
     @Override
     public String toString() {
-        return getClass().getSimpleName() + " [name=" + name + ", associatedFile=" + associatedFile + ']';
+        return getClass().getSimpleName() + " [name=" + name + ", associatedFile=" + associatedFile + ", visible=" + visible + ']';
     }
 }

trunk/src/org/openstreetmap/josm/gui/layer/MainLayerManager.java

@@ -7 +7 @@
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.ListIterator;
@@ -358 +359 @@
 
     /**
-     * Determines the next active data layer according to the following
-     * rules:
-     * <ul>
-     *   <li>if there is at least one {@link OsmDataLayer} the first one
-     *     becomes active</li>
-     *   <li>otherwise, the top most layer of any type becomes active</li>
-     * </ul>
+     * Determines the next active data layer.
+     * <p>
+     * The layer becomes active, which has the next highest index (closer to bottom) relative to {@code except} parameter
+     * in the following order:
+     * <ol>
+     *     <li>{@link OsmDataLayer} and visible, or if there is none</li>
+     *     <li>{@link OsmDataLayer} and hidden, or if there is none</li>
+     *     <li>any type</li>
+     * </ol>
      *
      * @param except A layer to ignore.
@@ -371 +374 @@
     private Layer suggestNextActiveLayer(Layer except) {
         List<Layer> layersList = new ArrayList<>(getLayers());
-        layersList.remove(except);
-        // First look for data layer
-        for (Layer layer : layersList) {
+
+        // construct a new list with decreasing priority
+        int indexOfExcept = layersList.indexOf(except);
+        List<Layer> remainingLayers = new ArrayList<>(layersList.subList(indexOfExcept, layersList.size()));
+        List<Layer> previousLayers = new ArrayList<>(layersList.subList(0, indexOfExcept));
+        Collections.reverse(previousLayers);
+        remainingLayers.addAll(previousLayers);
+        remainingLayers.remove(except);
+
+        // First look for visible data layer
+        for (Layer layer : remainingLayers) {
+            if (layer instanceof OsmDataLayer && layer.isVisible()) {
+                return layer;
+            }
+        }
+
+        // Then any data layer
+        for (Layer layer : remainingLayers) {
             if (layer instanceof OsmDataLayer) {
                 return layer;
@@ -380 +398 @@
 
         // Then any layer
-        if (!layersList.isEmpty())
-            return layersList.get(0);
+        for (Layer layer : layersList) {
+            if (layer != except) {
+                return layer;
+            }
+        }
 
         // and then give up