Changeset 18895 in josm


Ignore:
Timestamp:
2023-11-07T14:21:38+01:00 (7 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
Files:
8 edited

Legend:

Unmodified
Added
Removed

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

    r18837 r18895  
    66import java.awt.Component;
    77import java.awt.event.ActionEvent;
    8 import java.util.HashMap;
    98import java.util.List;
    10 import java.util.Map;
    119
    1210import javax.swing.AbstractAction;
    … …  
    4846        if (!SaveLayersDialog.saveUnsavedModifications(selectedLayers, SaveLayersDialog.Reason.DELETE))
    4947            return;
    50         final Map<Integer, Layer> layerMap = model.selectedIndices().filter(i -> model.getLayer(i) != null)
    51                 .collect(HashMap::new, (map, value) -> map.put(value, model.getLayer(value)), HashMap::putAll);
    5248        for (Layer l: selectedLayers) {
    5349            if (model.getLayerManager().containsLayer(l)) {
    … …  
    5551                // this is why we need to check if every layer is still in the list of selected layers.
    5652                model.getLayerManager().removeLayer(l);
    57             }
    58         }
    59         // Set the next active layer to the next visible layer
    60         if (layerMap.size() == 1) {
    61             final int selected = Math.min(layerMap.keySet().iterator().next(), model.getRowCount() - 1);
    62             int currentLayerIndex = selected;
    63             Layer layer = model.getLayer(currentLayerIndex);
    64             // If the user has the last layer selected, we need to wrap around.
    65             boolean reversed = false;
    66             while (layer != null && !layer.isVisible() && currentLayerIndex < model.getRowCount() && currentLayerIndex >= 0) {
    67                 if (reversed) {
    68                     currentLayerIndex--;
    69                 } else {
    70                     currentLayerIndex++;
    71                 }
    72                 if (currentLayerIndex == model.getRowCount()) {
    73                     reversed = true;
    74                     currentLayerIndex = selected;
    75                 }
    76                 layer = model.getLayer(currentLayerIndex);
    77             }
    78             if (layer != null) {
    79                 model.getLayerManager().setActiveLayer(layer);
    80                 // Reset the selection
    81                 model.getSelectionModel().setSelectionInterval(selected, selected);
    8253            }
    8354        }

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

    r17626 r18895  
    3636/**
    3737 * A layer encapsulates the gui component of one dataset and its representation.
    38  *
     38 * <p>
    3939 * Some layers may display data directly imported from OSM server. Other only
    4040 * display background images. Some can be edited, some not. Some are static and
    4141 * other changes dynamically (auto-updated).
    42  *
     42 * <p>
    4343 * Layers can be visible or not. Most actions the user can do applies only on
    4444 * selected layers. The available actions depend on the selected layers too.
    45  *
     45 * <p>
    4646 * All layers are managed by the MapView. They are displayed in a list to the
    4747 * right of the screen.
    … …  
    175175    /**
    176176     * Initialization code, that depends on Main.map.mapView.
    177      *
     177     * <p>
    178178     * It is always called in the event dispatching thread.
    179179     * Note that Main.map is null as long as no layer has been added, so do
    180180     * not execute code in the constructor, that assumes Main.map.mapView is
    181181     * not null.
    182      *
     182     * <p>
    183183     * If you need to execute code when this layer is added to the map view, use
    184184     * {@link #attachToMapView(org.openstreetmap.josm.gui.layer.MapViewPaintable.MapViewEvent)}
    … …  
    270270     * menu component than JMenuItem or when it supports multiple layers. Actions that support multiple layers should also
    271271     * have correct equals implementation.
    272      *
     272     * <p>
    273273     * Use {@link SeparatorLayerAction#INSTANCE} instead of new JSeparator
    274274     * @return menu actions for this layer
    … …  
    278278    /**
    279279     * Called, when the layer is removed from the mapview and is going to be destroyed.
    280      *
    281      * This is because the Layer constructor can not add itself safely as listener
     280     * <p>
     281     * This is because the Layer constructor cannot add itself safely as a listener
    282282     * to the layerlist dialog, because there may be no such dialog yet (loaded
    283283     * via command line parameter).
    … …  
    303303    /**
    304304     * Sets the associated file for this layer.
    305      *
     305     * <p>
    306306     * The associated file might be the one that the user opened.
    307307     * @param file The file, may be <code>null</code>
    … …  
    354354
    355355    /**
    356      * Replies true if this layer was renamed by user
    357      *
    358      * @return true if this layer was renamed by user
     356     * Replies true if user renamed this layer
     357     *
     358     * @return true if user renamed this layer
    359359     */
    360360    public boolean isRenamed() {
    … …  
    484484
    485485    /**
    486      * fires a property change for the property {@link #FILTER_STATE_PROP}.
     486     * Fires a property change for the property {@link #FILTER_STATE_PROP}.
    487487     */
    488488    protected void fireFilterStateChanged() {
    … …  
    491491
    492492    /**
    493      * allows to check whether a projection is supported or not
     493     * Allows to check whether a projection is supported or not.
    494494     * @param proj projection
    495      *
    496495     * @return True if projection is supported for this layer
    497496     */
    … …  
    610609
    611610    /**
    612      * Replies the savable state of this layer (i.e if it can be saved through a "File-&gt;Save" dialog).
     611     * Replies the savable state of this layer (i.e., if it can be saved through a "File-&gt;Save" dialog).
    613612     * @return true if this layer can be saved to a file
    614613     * @since 5459
    … …  
    628627
    629628    /**
    630      * Creates a new "Save" dialog for this layer and makes it visible.<br>
    631      * When the user has chosen a file, checks the file extension, and confirms overwrite if needed.
     629     * Creates a new "Save" dialog for this layer and makes it visible.
     630     * <p>
     631     * When the user has chosen a file, checks the file extension, and confirms overwriting if needed.
    632632     * @return The output {@code File}
    633633     * @see SaveActionBase#createAndOpenSaveFileChooser
    … …  
    675675    @Override
    676676    public String toString() {
    677         return getClass().getSimpleName() + " [name=" + name + ", associatedFile=" + associatedFile + ']';
     677        return getClass().getSimpleName() + " [name=" + name + ", associatedFile=" + associatedFile + ", visible=" + visible + ']';
    678678    }
    679679}

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

    r18801 r18895  
    77import java.util.ArrayList;
    88import java.util.Collection;
     9import java.util.Collections;
    910import java.util.List;
    1011import java.util.ListIterator;
    … …  
    358359
    359360    /**
    360      * Determines the next active data layer according to the following
    361      * rules:
    362      * <ul>
    363      *   <li>if there is at least one {@link OsmDataLayer} the first one
    364      *     becomes active</li>
    365      *   <li>otherwise, the top most layer of any type becomes active</li>
    366      * </ul>
     361     * Determines the next active data layer.
     362     * <p>
     363     * The layer becomes active, which has the next highest index (closer to bottom) relative to {@code except} parameter
     364     * in the following order:
     365     * <ol>
     366     *     <li>{@link OsmDataLayer} and visible, or if there is none</li>
     367     *     <li>{@link OsmDataLayer} and hidden, or if there is none</li>
     368     *     <li>any type</li>
     369     * </ol>
    367370     *
    368371     * @param except A layer to ignore.
    … …  
    371374    private Layer suggestNextActiveLayer(Layer except) {
    372375        List<Layer> layersList = new ArrayList<>(getLayers());
    373         layersList.remove(except);
    374         // First look for data layer
    375         for (Layer layer : layersList) {
     376
     377        // construct a new list with decreasing priority
     378        int indexOfExcept = layersList.indexOf(except);
     379        List<Layer> remainingLayers = new ArrayList<>(layersList.subList(indexOfExcept, layersList.size()));
     380        List<Layer> previousLayers = new ArrayList<>(layersList.subList(0, indexOfExcept));
     381        Collections.reverse(previousLayers);
     382        remainingLayers.addAll(previousLayers);
     383        remainingLayers.remove(except);
     384
     385        // First look for visible data layer
     386        for (Layer layer : remainingLayers) {
     387            if (layer instanceof OsmDataLayer && layer.isVisible()) {
     388                return layer;
     389            }
     390        }
     391
     392        // Then any data layer
     393        for (Layer layer : remainingLayers) {
    376394            if (layer instanceof OsmDataLayer) {
    377395                return layer;
    … …  
    380398
    381399        // Then any layer
    382         if (!layersList.isEmpty())
    383             return layersList.get(0);
     400        for (Layer layer : layersList) {
     401            if (layer != except) {
     402                return layer;
     403            }
     404        }
    384405
    385406        // and then give up

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

    r18839 r18895  
    1111import static org.junit.jupiter.api.Assertions.assertTrue;
    1212
     13import java.util.Collections;
    1314import java.util.Objects;
    1415import java.util.concurrent.atomic.AtomicInteger;
    … …  
    2223import org.openstreetmap.josm.gui.layer.Layer;
    2324import org.openstreetmap.josm.gui.layer.OsmDataLayer;
     25import org.openstreetmap.josm.gui.layer.geoimage.GeoImageLayer;
    2426import org.openstreetmap.josm.testutils.annotations.Main;
    2527import org.openstreetmap.josm.testutils.annotations.Projection;
    … …  
    123125    }
    124126
     127    @Test
     128    void testRemoveBottomActiveWithBackgroundLayer() {
     129        GeoImageLayer geoImageLayer = new GeoImageLayer(Collections.emptyList(), null, "imageLayer");
     130        OsmDataLayer osmDataLayer1 = new OsmDataLayer(new DataSet(), "dataLayer1", null);
     131        OsmDataLayer osmDataLayer2 = new OsmDataLayer(new DataSet(), "dataLayer2", null);
     132
     133        // remove all the layers added in BeforeEach()
     134        for (Layer l : MainApplication.getLayerManager().getLayers()) {
     135            MainApplication.getLayerManager().removeLayer(l);
     136        }
     137        MainApplication.getLayerManager().addLayer(geoImageLayer);
     138        MainApplication.getLayerManager().addLayer(osmDataLayer1);
     139        MainApplication.getLayerManager().addLayer(osmDataLayer2);
     140
     141        model.getLayerManager().setActiveLayer(osmDataLayer1);
     142        model.setSelectedLayer(osmDataLayer1);
     143
     144        deleteLayerAction.actionPerformed(null);
     145
     146        assertSame(model.getLayerManager().getActiveLayer(), model.getLayer(0));
     147        assertEquals("dataLayer2", Objects.requireNonNull(model.getLayerManager().getActiveLayer().getName()));
     148        assertAll(model.getLayers().stream().map(layer -> () -> assertNotSame(osmDataLayer1, layer)));
     149    }
     150
     151    @Test
     152    void testRemoveBottomActiveAllHidden() {
     153        hideRange(0, 9);
     154        final Layer toRemove = model.getLayer(9);
     155        assertNotNull(toRemove);
     156        assertFalse(toRemove.isVisible());
     157        assertEquals(0, model.getLayers().stream().filter(Layer::isVisible).count());
     158
     159        model.getLayerManager().setActiveLayer(toRemove);
     160        model.setSelectedLayer(toRemove);
     161        deleteLayerAction.actionPerformed(null);
     162
     163        assertSame(model.getLayerManager().getActiveLayer(), model.getLayer(8));
     164        assertEquals("testActiveLayer1", Objects.requireNonNull(model.getLayer(8)).getName());
     165        assertAll(model.getLayers().stream().map(layer -> () -> assertNotSame(toRemove, layer)));
     166    }
     167
    125168    private void hideRange(int start, int end) {
    126169        model.getSelectionModel().setSelectionInterval(start, end);

  • trunk/test/unit/org/openstreetmap/josm/gui/layer/geoimage/GeoImageLayerTest.java

    r18870 r18895  
    66import java.util.Collections;
    77
     8import org.junit.jupiter.api.AfterEach;
    89import org.junit.jupiter.api.Test;
    910import org.openstreetmap.josm.data.osm.DataSet;
    … …  
    1920@Main
    2021class GeoImageLayerTest {
     22    @AfterEach
     23    void tearDown() {
     24        if (ImageViewerDialog.hasInstance()) {
     25            ImageViewerDialog.getInstance().destroy();
     26        }
     27    }
     28
    2129    /**
    2230     * Test that {@link GeoImageLayer#mergeFrom} throws IAE for invalid arguments

  • trunk/test/unit/org/openstreetmap/josm/gui/layer/geoimage/ImagesLoaderTest.java

    r18870 r18895  
    1616import org.openstreetmap.josm.io.GpxReader;
    1717import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
     18import org.openstreetmap.josm.testutils.annotations.Main;
     19import org.openstreetmap.josm.testutils.annotations.Projection;
    1820
    1921/**
    … …  
    2123 */
    2224@BasicPreferences
     25@Main
     26@Projection
    2327class ImagesLoaderTest {
    2428    /**

  • trunk/test/unit/org/openstreetmap/josm/gui/layer/geoimage/WikimediaCommonsLoaderTest.java

    r18689 r18895  
    1111import java.util.List;
    1212
    13 import org.junit.jupiter.api.BeforeAll;
     13import org.junit.jupiter.api.AfterEach;
    1414import org.junit.jupiter.api.Test;
    1515import org.openstreetmap.josm.data.Bounds;
    1616import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
    17 import org.openstreetmap.josm.tools.Http1Client;
    18 import org.openstreetmap.josm.tools.HttpClient;
     17import org.openstreetmap.josm.testutils.annotations.HTTP;
     18import org.openstreetmap.josm.testutils.annotations.Main;
    1919
    2020import com.github.tomakehurst.wiremock.WireMockServer;
    … …  
    2424 */
    2525@BasicPreferences
     26@HTTP
     27@Main
    2628class WikimediaCommonsLoaderTest {
    27 
    28     @BeforeAll
    29     static void beforeAll() {
    30         HttpClient.setFactory(Http1Client::new);
     29    @AfterEach
     30    void tearDown() {
     31        if (ImageViewerDialog.hasInstance()) {
     32            ImageViewerDialog.getInstance().destroy();
     33        }
    3134    }
    3235

  • trunk/test/unit/org/openstreetmap/josm/gui/layer/markerlayer/ImageMarkerTest.java

    r18870 r18895  
    88import java.net.MalformedURLException;
    99
     10import org.junit.jupiter.api.AfterEach;
    1011import org.junit.jupiter.api.Test;
    1112import org.openstreetmap.josm.TestUtils;
    … …  
    1314import org.openstreetmap.josm.data.gpx.GpxData;
    1415import org.openstreetmap.josm.data.gpx.WayPoint;
     16import org.openstreetmap.josm.gui.layer.geoimage.ImageViewerDialog;
    1517import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
    1618import org.openstreetmap.josm.testutils.annotations.Main;
    … …  
    2224@Main
    2325class ImageMarkerTest {
     26    @AfterEach
     27    void tearDown() {
     28        if (ImageViewerDialog.hasInstance()) {
     29            ImageViewerDialog.getInstance().destroy();
     30        }
     31    }
     32
    2433    /**
    2534     * Unit test of {@link ImageMarker#ImageMarker}.
Note: See TracChangeset for help on using the changeset viewer.