From 6ec5e4f8cdb98227db11e5989376c31832ca2048 Mon Sep 17 00:00:00 2001 From: Tobrun Date: Fri, 9 Jun 2017 12:05:54 +0200 Subject: Cherry picks to release branch (#9230) * [ios][macos] test remove source in use * [android] test remove source in use * [core] check source usage before remove * [core] ensure layer::accept works with non-void return values on gcc * [android] - remove upgrade runtime exceptions (#9191) --- include/mbgl/style/layer.hpp | 7 +++++ .../java/com/mapbox/mapboxsdk/maps/MapView.java | 18 ++---------- .../mapboxsdk/testapp/style/RuntimeStyleTests.java | 18 ++++++++++++ platform/darwin/test/MGLStyleTests.mm | 16 +++++++++++ src/mbgl/style/style.cpp | 23 ++++++++++++++++ test/style/style.test.cpp | 32 ++++++++++++++++++++++ 6 files changed, 98 insertions(+), 16 deletions(-) diff --git a/include/mbgl/style/layer.hpp b/include/mbgl/style/layer.hpp index 56f2c48fa7..016b3a1c8b 100644 --- a/include/mbgl/style/layer.hpp +++ b/include/mbgl/style/layer.hpp @@ -5,8 +5,10 @@ #include #include +#include #include #include +#include namespace mbgl { namespace style { @@ -96,6 +98,11 @@ public: case LayerType::FillExtrusion: return visitor(*as()); } + + + // Not reachable, but placate GCC. + assert(false); + throw new std::runtime_error("unknown layer type"); } const std::string& getID() const; diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java index d00da4c155..9a1f2fc515 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java @@ -67,8 +67,6 @@ public class MapView extends FrameLayout { private MapboxMap mapboxMap; private MapCallback mapCallback; - private boolean onStartCalled; - private boolean onStopCalled; private MapGestureDetector mapGestureDetector; private MapKeyListener mapKeyListener; @@ -233,7 +231,6 @@ public class MapView extends FrameLayout { */ @UiThread public void onStart() { - onStartCalled = true; mapboxMap.onStart(); ConnectivityReceiver.instance(getContext()).activate(); } @@ -243,11 +240,7 @@ public class MapView extends FrameLayout { */ @UiThread public void onResume() { - if (!onStartCalled) { - // TODO: 26/10/16, can be removed after 5.0.0 release - throw new IllegalStateException("MapView#onStart() was not called. " - + "You must call this method from the parent's {@link Activity#onStart()} or {@link Fragment#onStart()}."); - } + // replaced by onStart in v5.0.0 } /** @@ -255,7 +248,7 @@ public class MapView extends FrameLayout { */ @UiThread public void onPause() { - // replaced by onStop in v5.0.0, keep around for future development + // replaced by onStop in v5.0.0 } /** @@ -263,7 +256,6 @@ public class MapView extends FrameLayout { */ @UiThread public void onStop() { - onStopCalled = true; mapboxMap.onStop(); ConnectivityReceiver.instance(getContext()).deactivate(); } @@ -273,12 +265,6 @@ public class MapView extends FrameLayout { */ @UiThread public void onDestroy() { - if (!onStopCalled) { - // TODO: 26/10/16, can be removed after 5.0.0 release - throw new IllegalStateException("MapView#onStop() was not called. " - + "You must call this method from the parent's {@link Activity#onStop()} or {@link Fragment#onStop()}."); - } - destroyed = true; nativeMapView.terminateContext(); nativeMapView.terminateDisplay(); diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java index a1c46903bf..a37615aadc 100644 --- a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java @@ -12,6 +12,7 @@ import com.mapbox.mapboxsdk.style.layers.CannotAddLayerException; import com.mapbox.mapboxsdk.style.layers.CircleLayer; import com.mapbox.mapboxsdk.style.layers.FillLayer; import com.mapbox.mapboxsdk.style.layers.Layer; +import com.mapbox.mapboxsdk.style.layers.LineLayer; import com.mapbox.mapboxsdk.style.layers.Property; import com.mapbox.mapboxsdk.style.layers.PropertyFactory; import com.mapbox.mapboxsdk.style.sources.CannotAddSourceException; @@ -223,6 +224,23 @@ public class RuntimeStyleTests extends BaseActivityTest { assertEquals("http://mapbox.com/my-file.json", source.getUrl()); } + @Test + public void testRemoveSourceInUse() { + validateTestSetup(); + + onView(withId(R.id.mapView)).perform(new BaseViewAction() { + + @Override + public void perform(UiController uiController, View view) { + mapboxMap.addSource(new VectorSource("my-source", "mapbox://mapbox.mapbox-terrain-v2")); + mapboxMap.addLayer(new LineLayer("my-layer", "my-source")); + mapboxMap.removeSource("my-source"); + assertNotNull(mapboxMap.getSource("my-source")); + } + + }); + } + /** * https://github.com/mapbox/mapbox-gl-native/issues/7973 */ diff --git a/platform/darwin/test/MGLStyleTests.mm b/platform/darwin/test/MGLStyleTests.mm index f80d5776f0..d93483ea6e 100644 --- a/platform/darwin/test/MGLStyleTests.mm +++ b/platform/darwin/test/MGLStyleTests.mm @@ -240,6 +240,22 @@ XCTAssertTrue([[self.style sourceWithIdentifier:shapeSource.identifier] isMemberOfClass:[MGLVectorSource class]]); } +- (void)testRemovingSourceInUse { + // Add a raster source + MGLRasterSource *rasterSource = [[MGLRasterSource alloc] initWithIdentifier:@"some-identifier" tileURLTemplates:@[] options:nil]; + [self.style addSource:rasterSource]; + + // Add a layer using it + MGLFillStyleLayer *fillLayer = [[MGLFillStyleLayer alloc] initWithIdentifier:@"fillLayer" source:rasterSource]; + [self.style addLayer:fillLayer]; + + // Attempt to remove the raster source + [self.style removeSource:rasterSource]; + + // Ensure it is still there + XCTAssertTrue([[self.style sourceWithIdentifier:rasterSource.identifier] isMemberOfClass:[MGLRasterSource class]]); +} + - (void)testLayers { NSArray *initialLayers = self.style.layers; if ([initialLayers.firstObject.identifier isEqualToString:@"com.mapbox.annotations.points"]) { diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index f083ca47b8..568b575dfa 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -196,7 +196,30 @@ void Style::addSource(std::unique_ptr source) { sources.emplace_back(std::move(source)); } +struct SourceIdUsageEvaluator { + const std::string& sourceId; + + bool operator()(BackgroundLayer&) { return false; } + bool operator()(CustomLayer&) { return false; } + + template + bool operator()(LayerType& layer) { + return layer.getSourceID() == sourceId; + } +}; + std::unique_ptr Style::removeSource(const std::string& id) { + // Check if source is in use + SourceIdUsageEvaluator sourceIdEvaluator {id}; + auto layerIt = std::find_if(layers.begin(), layers.end(), [&](const auto& layer) { + return layer->accept(sourceIdEvaluator); + }); + + if (layerIt != layers.end()) { + Log::Warning(Event::General, "Source '%s' is in use, cannot remove", id.c_str()); + return nullptr; + } + auto it = std::find_if(sources.begin(), sources.end(), [&](const auto& source) { return source->getID() == id; }); diff --git a/test/style/style.test.cpp b/test/style/style.test.cpp index b529abad4a..841c7b291b 100644 --- a/test/style/style.test.cpp +++ b/test/style/style.test.cpp @@ -1,10 +1,12 @@ #include #include +#include #include #include #include #include +#include #include #include #include @@ -67,3 +69,33 @@ TEST(Style, DuplicateSource) { // Expected } } + +TEST(Style, RemoveSourceInUse) { + util::RunLoop loop; + + auto log = new FixtureLogObserver(); + Log::setObserver(std::unique_ptr(log)); + + ThreadPool threadPool{ 1 }; + StubFileSource fileSource; + Style style { threadPool, fileSource, 1.0 }; + + style.setJSON(util::read_file("test/fixtures/resources/style-unused-sources.json")); + + style.addSource(std::make_unique("sourceId", "mapbox://mapbox.mapbox-terrain-v2")); + style.addLayer(std::make_unique("layerId", "sourceId")); + + // Should not remove the source + auto removed = style.removeSource("sourceId"); + ASSERT_EQ(nullptr, removed); + ASSERT_NE(nullptr, style.getSource("sourceId")); + + const FixtureLogObserver::LogMessage logMessage { + EventSeverity::Warning, + Event::General, + int64_t(-1), + "Source 'sourceId' is in use, cannot remove", + }; + + EXPECT_EQ(log->count(logMessage), 1u); +} -- cgit v1.2.1