### Eclipse Workspace Patch 1.0 #P josm diff --git src/org/openstreetmap/josm/data/vector/VectorDataSet.java src/org/openstreetmap/josm/data/vector/VectorDataSet.java index 86d5383..7988be8 100644 --- src/org/openstreetmap/josm/data/vector/VectorDataSet.java +++ src/org/openstreetmap/josm/data/vector/VectorDataSet.java @@ -252,7 +252,7 @@ @Override public Collection getPrimitives(Predicate predicate) { - Collection primitives = tryRead(this.readWriteLock, () -> { + final Collection primitives = tryRead(this.readWriteLock, () -> { final Storage dataStore = this.getBestZoomDataStore().orElse(null); final Stream dataStoreStream = dataStore != null ? dataStore.stream().map(MVTTile::getData) : Stream.empty(); return Stream.concat(dataStoreStream, Stream.of(this.customDataStore)) @@ -363,14 +363,27 @@ public Collection getAllSelected() { return tryRead(this.readWriteLock, () -> { final Storage dataStore = this.getBestZoomDataStore().orElse(null); - Stream dataStoreStream = dataStore != null ? dataStore.stream().map(MVTTile::getData) : Stream.empty(); - return Stream.concat(dataStoreStream, Stream.of(this.customDataStore)).map(VectorDataStore::getPrimitivesMap) - .flatMap(dataMap -> { - // Synchronize on dataMap to avoid concurrent modification errors - synchronized (dataMap) { - return this.currentSelectedPrimitives.stream().map(dataMap::get).filter(Objects::nonNull); - } - }).collect(Collectors.toList()); + final Stream dataStoreStream = dataStore != null + ? dataStore.stream().map(MVTTile::getData) + : Stream.empty(); + + return Stream.concat(dataStoreStream, Stream.ofNullable(this.customDataStore)) + .map(VectorDataStore::getPrimitivesMap) + .filter(Objects::nonNull) + .flatMap(dataMap -> { + final Map lockObject = dataMap; + synchronized (lockObject) { + final List selectedInMap = new ArrayList<>(); + for (PrimitiveId id : this.currentSelectedPrimitives) { + final VectorPrimitive primitive = lockObject.get(id); + if (primitive != null) { + selectedInMap.add(primitive); + } + } + return selectedInMap.stream(); + } + }) + .collect(Collectors.toList()); }).orElseGet(Collections::emptyList); } @@ -490,7 +503,7 @@ private boolean doSelectionChange(final Function, SelectionChangeEvent> command) { synchronized (this.selectionLock) { - SelectionChangeEvent event = + final SelectionChangeEvent event = command.apply(currentSelectedPrimitives.stream().map(this::getPrimitiveById).collect(Collectors.toSet())); if (event.isNop()) { return false; @@ -635,8 +648,8 @@ * @param invisibleLayers The layer to not show */ public void setInvisibleLayers(Collection invisibleLayers) { - String[] currentInvisibleLayers = invisibleLayers.stream().filter(Objects::nonNull).toArray(String[]::new); - List temporaryList = Arrays.asList(currentInvisibleLayers); + final String[] currentInvisibleLayers = invisibleLayers.stream().filter(Objects::nonNull).toArray(String[]::new); + final List temporaryList = Arrays.asList(currentInvisibleLayers); this.dataStoreMap.values().stream().flatMap(Collection::stream).map(MVTTile::getData) .forEach(dataStore -> dataStore.getAllPrimitives().parallelStream() .forEach(primitive -> primitive.setVisible(!temporaryList.contains(primitive.getLayer())))); diff --git test/unit/org/openstreetmap/josm/data/vector/VectorDataSetTest.java test/unit/org/openstreetmap/josm/data/vector/VectorDataSetTest.java index 39f1b65..9b66b6b 100644 --- test/unit/org/openstreetmap/josm/data/vector/VectorDataSetTest.java +++ test/unit/org/openstreetmap/josm/data/vector/VectorDataSetTest.java @@ -6,15 +6,25 @@ import java.nio.file.Paths; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashSet; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; import org.awaitility.Awaitility; import org.awaitility.Durations; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.RepeatedTest; +import org.junit.jupiter.api.Test; import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.data.coor.LatLon; import org.openstreetmap.josm.data.imagery.ImageryInfo; import org.openstreetmap.josm.data.imagery.vectortile.mapbox.MVTTile; import org.openstreetmap.josm.data.imagery.vectortile.mapbox.MapboxVectorCachedTileLoader; @@ -73,8 +83,8 @@ throw new IllegalArgumentException("Tiles come with a {z}, {x}, and {y} component"); } final MapboxVectorTileSource tileSource = layer.getTileSource(); - MapboxVectorCachedTileLoader tileLoader = layer.getTileLoader(); - Collection tilesCollection = new ArrayList<>(); + final MapboxVectorCachedTileLoader tileLoader = layer.getTileLoader(); + final Collection tilesCollection = new ArrayList<>(); for (int i = 0; i < tiles.length / 3; i++) { final MVTTile tile = (MVTTile) layer.createTile(tileSource, tiles[3 * i + 1], tiles[3 * i + 2], tiles[3 * i]); tileLoader.createTileLoaderJob(tile).submit(); @@ -124,4 +134,107 @@ assertEquals(1, dataSet.getWays().size()); assertEquals(0, dataSet.getRelations().size()); } + + + /** + * Test that {@link VectorDataSet#getAllSelected()} returns the expected selected primitives + * and returns an empty collection once the selection is cleared. + */ + @Test + void testGetAllSelected() { + final VectorDataSet dataSet = this.layer.getData(); + assertTrue(dataSet.getAllSelected().isEmpty()); + + dataSet.setZoom(14); + loadTile(this.layer, 14, 3248, 6258); + + // Select two arbitrary nodes from the loaded tile + final List nodesToSelect = dataSet.getNodes().stream() + .limit(2) + .collect(Collectors.toList()); + assertEquals(2, nodesToSelect.size()); + + dataSet.setSelected(nodesToSelect); + + final Collection selected = dataSet.getAllSelected(); + assertEquals(2, selected.size()); + assertTrue(selected.containsAll(nodesToSelect)); + + // Clear selection again + dataSet.clearSelection(); + assertTrue(dataSet.getAllSelected().isEmpty()); + } + + /** + * Test that {@link VectorDataSet#getAllSelected()} properly combines selected primitives + * from both loaded vector tiles and the custom data store. + */ + @Test + void testGetAllSelectedWithCustomDataStore() { + final VectorDataSet dataSet = this.layer.getData(); + dataSet.setZoom(14); + loadTile(this.layer, 14, 3248, 6258); + + // Combine a primitive from the tile and a custom primitive (customDataStore) + final VectorPrimitive tilePrimitive = dataSet.getNodes().iterator().next(); + final VectorNode customNode = new VectorNode("custom-layer"); + customNode.setCoor(new LatLon(0, 0)); + dataSet.addPrimitive(customNode); + + dataSet.setSelected(Arrays.asList(tilePrimitive, customNode)); + + final Collection selected = dataSet.getAllSelected(); + assertEquals(2, selected.size()); + assertTrue(selected.contains(tilePrimitive)); + assertTrue(selected.contains(customNode)); + } + + /** + * Test that {@link VectorDataSet#getAllSelected()} is thread-safe when primitives are + * concurrently added and removed while reading the current selection. + * + * @throws Exception if any thread execution or timeout error occurs + */ + @Test + void testGetAllSelectedThreadSafety() throws Exception { + final VectorDataSet dataSet = this.layer.getData(); + dataSet.setZoom(14); + loadTile(this.layer, 14, 3248, 6258); + + final List nodesToSelect = dataSet.getNodes().stream() + .limit(5) + .collect(Collectors.toList()); + dataSet.setSelected(nodesToSelect); + + final int iterations = 200; + final ExecutorService executor = Executors.newFixedThreadPool(2); + final CountDownLatch startLatch = new CountDownLatch(1); + + // Thread 1: Continuously add and remove primitives (modifies underlying maps) + final Future writer = executor.submit(() -> { + startLatch.await(); + for (int i = 0; i < iterations; i++) { + final VectorNode tempNode = new VectorNode("temp-layer"); + tempNode.setCoor(new LatLon(i * 0.001, i * 0.001)); + dataSet.addPrimitive(tempNode); + dataSet.removePrimitive(tempNode); + } + return null; + }); + + // Thread 2: Concurrently read getAllSelected() + final Future reader = executor.submit(() -> { + startLatch.await(); + for (int i = 0; i < iterations; i++) { + final Collection selected = dataSet.getAllSelected(); + assertEquals(5, selected.size()); + } + return null; + }); + + startLatch.countDown(); + writer.get(5, TimeUnit.SECONDS); + reader.get(5, TimeUnit.SECONDS); + executor.shutdown(); + } }