Ticket #23057: josm_23057.patch

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

@@ -47 +47 @@
             return;
         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)) {
                 // it may happen that this call removes other layers.
@@ -56 +54 @@
                 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);
-            }
-        }
     }
 
     @Override

test/unit/org/openstreetmap/josm/gui/dialogs/layer/DeleteLayerActionTest.java

@@ -10 +10 @@
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.util.Collections;
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Supplier;
@@ -21 +22 @@
 import org.openstreetmap.josm.gui.dialogs.LayerListDialog;
 import org.openstreetmap.josm.gui.layer.Layer;
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
+import org.openstreetmap.josm.gui.layer.geoimage.GeoImageLayer;
 import org.openstreetmap.josm.testutils.annotations.Main;
 import org.openstreetmap.josm.testutils.annotations.Projection;
 
@@ -122 +124 @@
         assertAll(model.getLayers().stream().map(layer -> () -> assertNotSame(toRemove, layer)));
     }
 
+    @Test
+    void testRemoveBottomActiveWithBackgroundLayer() {
+        GeoImageLayer geoImageLayer = new GeoImageLayer(Collections.emptyList(), null, "imageLayer");
+        OsmDataLayer osmDataLayer1 = new OsmDataLayer(new DataSet(), "dataLayer1", null);
+        OsmDataLayer osmDataLayer2 = new OsmDataLayer(new DataSet(), "dataLayer2", null);
+
+        // remove all the layers added in BeforeEach()
+        for (Layer l : MainApplication.getLayerManager().getLayers()) {
+            MainApplication.getLayerManager().removeLayer(l);
+        }
+        MainApplication.getLayerManager().addLayer(geoImageLayer);
+        MainApplication.getLayerManager().addLayer(osmDataLayer1);
+        MainApplication.getLayerManager().addLayer(osmDataLayer2);
+
+        model.getLayerManager().setActiveLayer(osmDataLayer1);
+        model.setSelectedLayer(osmDataLayer1);
+
+        deleteLayerAction.actionPerformed(null);
+
+        assertSame(model.getLayerManager().getActiveLayer(), model.getLayer(0));
+        assertEquals("dataLayer2", Objects.requireNonNull(model.getLayerManager().getActiveLayer().getName()));
+        assertAll(model.getLayers().stream().map(layer -> () -> assertNotSame(osmDataLayer1, layer)));
+    }
+
+    @Test
+    void testRemoveBottomActiveAllHidden() {
+        hideRange(0, 9);
+        final Layer toRemove = model.getLayer(9);
+        assertNotNull(toRemove);
+        assertFalse(toRemove.isVisible());
+        assertEquals(0, model.getLayers().stream().filter(Layer::isVisible).count());
+
+        model.getLayerManager().setActiveLayer(toRemove);
+        model.setSelectedLayer(toRemove);
+        deleteLayerAction.actionPerformed(null);
+
+        assertSame(model.getLayerManager().getActiveLayer(), model.getLayer(8));
+        assertEquals("testActiveLayer1", Objects.requireNonNull(model.getLayer(8)).getName());
+        assertAll(model.getLayers().stream().map(layer -> () -> assertNotSame(toRemove, layer)));
+    }
+
     private void hideRange(int start, int end) {
         model.getSelectionModel().setSelectionInterval(start, end);
         showHideLayerAction.actionPerformed(null);

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

@@ -35 +35 @@
 
 /**
  * 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.
  *
@@ -174 +174 @@
 
     /**
      * 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)}
      */
@@ -269 +269 @@
      * Returns list of actions. Action can implement LayerAction interface when it needs to be represented by other
      * 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
      */
@@ -277 +277 @@
 
     /**
      * 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).
      */
@@ -302 +302 @@
 
     /**
      * 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>
      */
@@ -353 +353 @@
     }
 
     /**
-     * Replies true if this layer was renamed by user
+     * Replies true if user renamed this layer
      *
-     * @return true if this layer was renamed by user
+     * @return true if user renamed this layer
      */
     public boolean isRenamed() {
         return renamed;
@@ -483 +483 @@
     }
 
     /**
-     * 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() {
         propertyChangeSupport.firePropertyChange(FILTER_STATE_PROP, null, null);
@@ -490 +490 @@
     }
 
     /**
-     * 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
      */
     public boolean isProjectionSupported(Projection proj) {
@@ -609 +608 @@
     }
 
     /**
-     * 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
      */
@@ -627 +626 @@
     }
 
     /**
-     * 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
      * @since 5459
@@ -674 +674 @@
 
     @Override
     public String toString() {
-        return getClass().getSimpleName() + " [name=" + name + ", associatedFile=" + associatedFile + ']';
+        return getClass().getSimpleName() + " [name=" + name + ", associatedFile=" + associatedFile + ", visible=" + visible + ']';
     }
 }

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

@@ -6 +6 @@
 import java.awt.GraphicsEnvironment;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.ListIterator;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -357 +358 @@
     }
 
     /**
-     * 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.
      * @return the next active data layer
@@ -370 +373 @@
      */
     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> layersList2 = new ArrayList<>(layersList.subList(indexOfExcept, layersList.size()));
+        List<Layer> layersList3 = new ArrayList<>(layersList.subList(0, indexOfExcept));
+        Collections.reverse(layersList3);
+        layersList2.addAll(layersList3);
+        layersList2.remove(except);
+
+        // First look for visible data layer
+        for (Layer layer : layersList2) {
+            if (layer instanceof OsmDataLayer && layer.isVisible()) {
+                return layer;
+            }
+        }
+
+        // Then any data layer
+        for (Layer layer : layersList2) {
             if (layer instanceof OsmDataLayer) {
                 return layer;
             }
@@ -379 +397 @@
         }
 
         // Then any layer
-        if (!layersList.isEmpty())
-            return layersList.get(0);
+        if (!layersList2.isEmpty())
+            return layersList2.get(0);
 
         // and then give up
         return null;