Ticket #16809: v1-0002-SlippyMapBBoxChooser-redesign-getAllTileSources-t.patch

src/org/openstreetmap/josm/gui/bbox/SlippyMapBBoxChooser.java

@@ -16 +16 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
 
 import javax.swing.ButtonModel;
 import javax.swing.JOptionPane;
@@ -78 +80 @@
      * TMS TileSource provider for the slippymap chooser
      */
     public static class TMSTileSourceProvider implements TileSourceProvider {
-        private static final Set<String> existingSlippyMapUrls = new HashSet<>();
-        static {
-            // Urls that already exist in the slippymap chooser and shouldn't be copied from TMS layer list
-            existingSlippyMapUrls.add("https://{switch:a,b,c}.tile.openstreetmap.org/{zoom}/{x}/{y}.png");      // Mapnik
-        }
 
         @Override
         public List<TileSource> getTileSources() {
             if (!TMSLayer.PROP_ADD_TO_SLIPPYMAP_CHOOSER.get()) return Collections.<TileSource>emptyList();
             List<TileSource> sources = new ArrayList<>();
             for (ImageryInfo info : ImageryLayerInfo.instance.getLayers()) {
-                if (existingSlippyMapUrls.contains(info.getUrl())) {
-                    continue;
-                }
                 try {
                     TileSource source = TMSLayer.getTileSourceStatic(info);
                     if (source != null) {
@@ -177 +171 @@
         }
         setMaxTilesInMemory(Config.getPref().getInt("slippy_map_chooser.max_tiles", 1000));
 
-        List<TileSource> tileSources = getAllTileSources();
+        List<TileSource> tileSources = new ArrayList<>(getAllTileSources().values());
 
         this.showDownloadAreaButtonModel = new JToggleButton.ToggleButtonModel();
         this.showDownloadAreaButtonModel.setSelected(PROP_SHOWDLAREA.get());
@@ -210 +204 @@
         new SlippyMapControler(this, this);
     }
 
-    private static List<TileSource> getAllTileSources() {
-        List<TileSource> tileSources = new ArrayList<>();
-        for (TileSourceProvider provider: providers) {
-            tileSources.addAll(provider.getTileSources());
-        }
-        return tileSources;
+    private static LinkedHashMap<String, TileSource> getAllTileSources() {
+        // using a LinkedHashMap of <id, TileSource> to retain ordering but provide deduplication
+        return providers.stream().flatMap(
+            provider -> provider.getTileSources().stream()
+        ).collect(Collectors.toMap(
+            TileSource::getId,
+            ts -> ts,
+            (old, _new) -> old,
+            () -> new LinkedHashMap<String, TileSource>()
+        ));
     }
 
     /**
@@ -358 +356 @@
         this.tileController.setTileCache(new MemoryTileCache());
         this.setTileSource(tileSource);
         PROP_MAPSTYLE.put(tileSource.getName()); // TODO Is name really unique?
-        if (this.iSourceButton.getCurrentSource() != tileSource) { // prevent infinite recursion
-            this.iSourceButton.setCurrentMap(tileSource);
-        }
+
+        // we need to refresh the tile sources in case the deselected source should no longer be present
+        // (and only remained there because its removal was deferred while the source was still the
+        // selected one). this should also have the effect of propagating the new selection to the
+        // iSourceButton & menu: it attempts to re-select the current source when rebuilding its menu.
+        this.refreshTileSources();
     }
 
     @Override
@@ -416 +417 @@
      * @since 6364
      */
     public final void refreshTileSources() {
-        iSourceButton.setSources(getAllTileSources());
+        final LinkedHashMap<String, TileSource> newTileSources = getAllTileSources();
+        final TileSource currentTileSource = this.getTileController().getTileSource();
+
+        // re-add the currently active TileSource to prevent inconsistent display of menu
+        newTileSources.putIfAbsent(currentTileSource.getId(), currentTileSource);
+
+        this.iSourceButton.setSources(new ArrayList<>(newTileSources.values()));
     }
 }

test/unit/org/openstreetmap/josm/gui/dialogs/MinimapDialogTest.java

@@ -30 +30 @@
 import org.openstreetmap.josm.data.Bounds;
 import org.openstreetmap.josm.data.DataSource;
 import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.imagery.ImageryLayerInfo;
 import org.openstreetmap.josm.data.projection.ProjectionRegistry;
 import org.openstreetmap.josm.data.projection.Projections;
 import org.openstreetmap.josm.gui.MainApplication;
@@ -257 +258 @@
     }
 
     /**
+     * Tests that the currently selected source being removed from ImageryLayerInfo will remain present and
+     * selected in the source menu even after the tile sources have been refreshed.
+     * @throws Exception if any error occurs
+     */
+    @Test
+    public void testRemovedSourceStillSelected() throws Exception {
+        // relevant prefs starting out empty, should choose the first source and have shown download area enabled
+        // (not that there's a data layer for it to use)
+
+        this.setUpMiniMap();
+
+        this.clickSourceMenuItemByLabel("Green Tiles");
+
+        ImageryLayerInfo.instance.remove(
+            ImageryLayerInfo.instance.getLayers().stream().filter(i -> i.getName().equals("Green Tiles")).findAny().get()
+        );
+
+        this.assertSingleSelectedSourceLabel("Green Tiles");
+
+        this.slippyMap.refreshTileSources();
+
+        this.assertSingleSelectedSourceLabel("Green Tiles");
+
+        // call paint to trigger new tile fetch
+        this.paintSlippyMap();
+
+        Awaitility.await().atMost(1000, MILLISECONDS).until(this.slippyMapTasksFinished);
+
+        this.paintSlippyMap();
+
+        assertEquals(0xff00ff00, paintedSlippyMap.getRGB(0, 0));
+    }
+
+    /**
      * Tests minimap obeys a saved "mapstyle" preference on startup.
      * @throws Exception if any error occurs
      */