diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index 10a836094c6..2a1e32baedf 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -18,6 +18,9 @@ template class Thread; class ResourceTransform; +// TODO: the callback should include a potential error info when https://github.com/mapbox/mapbox-gl-native/issues/14759 is resolved +using PathChangeCallback = std::function; + class DefaultFileSource : public FileSource { public: /* @@ -47,7 +50,7 @@ class DefaultFileSource : public FileSource { void setResourceTransform(optional>&&); - void setResourceCachePath(const std::string&); + void setResourceCachePath(const std::string&, optional>&&); std::unique_ptr request(const Resource&, Callback) override; diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/storage/FileSource.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/storage/FileSource.java index 1301c103f41..b3b7b61831a 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/storage/FileSource.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/storage/FileSource.java @@ -312,8 +312,7 @@ public void onWritePermissionGranted() { Context.MODE_PRIVATE).edit(); editor.putString(MAPBOX_SHARED_PREFERENCE_RESOURCES_CACHE_PATH, path); editor.apply(); - setResourcesCachePath(applicationContext, path); - callback.onSuccess(path); + internalSetResourcesCachePath(applicationContext, path, callback); } @Override @@ -326,11 +325,26 @@ public void onError() { } } - private static void setResourcesCachePath(@NonNull Context context, @NonNull String path) { - resourcesCachePathLoaderLock.lock(); - resourcesCachePath = path; - resourcesCachePathLoaderLock.unlock(); - getInstance(context).setResourceCachePath(path); + private static void internalSetResourcesCachePath(@NonNull Context context, @NonNull String path, + @NonNull final ResourcesCachePathChangeCallback callback) { + final FileSource fileSource = getInstance(context); + fileSource.setResourceCachePath(path, new ResourcesCachePathChangeCallback() { + @Override + public void onSuccess(@NonNull String path) { + fileSource.deactivate(); + resourcesCachePathLoaderLock.lock(); + resourcesCachePath = path; + resourcesCachePathLoaderLock.unlock(); + callback.onSuccess(path); + } + + @Override + public void onError(@NonNull String message) { + fileSource.deactivate(); + callback.onError(message); + } + }); + fileSource.activate(); } private static boolean isPathWritable(String path) { @@ -388,7 +402,7 @@ private FileSource(String cachePath, AssetManager assetManager) { public native void setResourceTransform(final ResourceTransformCallback callback); @Keep - private native void setResourceCachePath(String path); + private native void setResourceCachePath(String path, ResourcesCachePathChangeCallback callback); @Keep private native void initialize(String accessToken, String cachePath, AssetManager assetManager); diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceStandaloneTest.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceStandaloneTest.kt index 91b3922f472..08a15b0c54a 100644 --- a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceStandaloneTest.kt +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceStandaloneTest.kt @@ -1,6 +1,5 @@ package com.mapbox.mapboxsdk.testapp.storage -import android.os.Handler import android.support.test.annotation.UiThreadTest import android.support.test.rule.ActivityTestRule import android.support.test.runner.AndroidJUnit4 @@ -10,6 +9,7 @@ import org.junit.* import org.junit.rules.TestName import org.junit.runner.RunWith import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit @RunWith(AndroidJUnit4::class) class FileSourceStandaloneTest { @@ -55,19 +55,57 @@ class FileSourceStandaloneTest { fileSourceTestUtils.changePath(fileSourceTestUtils.testPath) Assert.assertEquals(fileSourceTestUtils.testPath, FileSource.getResourcesCachePath(rule.activity)) - // workaround for https://github.com/mapbox/mapbox-gl-native/issues/14334 - val latch = CountDownLatch(1) + fileSourceTestUtils.changePath(fileSourceTestUtils.originalPath) + Assert.assertEquals(fileSourceTestUtils.originalPath, FileSource.getResourcesCachePath(rule.activity)) + } + + @Test + fun overridePathChangeCallTest() { + val firstLatch = CountDownLatch(1) + val secondLatch = CountDownLatch(1) rule.activity.runOnUiThread { - fileSource.activate() - Handler().postDelayed({ + FileSource.setResourcesCachePath( + fileSourceTestUtils.testPath, + object : FileSource.ResourcesCachePathChangeCallback { + override fun onSuccess(path: String) { + Assert.assertEquals(fileSourceTestUtils.testPath, path) + firstLatch.countDown() + } + + override fun onError(message: String) { + Assert.fail("First attempt should succeed.") + } + }) + + FileSource.setResourcesCachePath( + fileSourceTestUtils.testPath2, + object : FileSource.ResourcesCachePathChangeCallback { + override fun onSuccess(path: String) { + Assert.fail("Second attempt should fail because first one is in progress.") + } + + override fun onError(message: String) { + Assert.assertEquals("Another resources cache path change is in progress", message) + secondLatch.countDown() + } + }) + } + + if (!secondLatch.await(5, TimeUnit.SECONDS)) { + rule.runOnUiThread { + // if we fail to call a callback, the file source is not going to be deactivated fileSource.deactivate() - latch.countDown() - }, 2000) + } + Assert.fail("Second attempt should fail.") } - latch.await() - fileSourceTestUtils.changePath(fileSourceTestUtils.originalPath) - Assert.assertEquals(fileSourceTestUtils.originalPath, FileSource.getResourcesCachePath(rule.activity)) + if (!firstLatch.await(5, TimeUnit.SECONDS)) { + rule.runOnUiThread { + // if we fail to call a callback, the file source is not going to be deactivated + fileSource.deactivate() + } + Assert.fail("First attempt should succeed.") + } } @After diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceTestUtils.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceTestUtils.kt index c69321581a0..c79d3b2752e 100644 --- a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceTestUtils.kt +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceTestUtils.kt @@ -10,10 +10,15 @@ import java.util.concurrent.CountDownLatch class FileSourceTestUtils(private val activity: Activity) { val originalPath = FileSource.getResourcesCachePath(activity) val testPath = "$originalPath/test" + val testPath2 = "$originalPath/test2" + + private val paths = listOf(testPath, testPath2) fun setup() { - val testFile = File(testPath) - testFile.mkdirs() + for (path in paths) { + val testFile = File(path) + testFile.mkdirs() + } } @WorkerThread @@ -22,25 +27,29 @@ class FileSourceTestUtils(private val activity: Activity) { if (currentPath != originalPath) { changePath(originalPath) } - val testFile = File(testPath) - if (testFile.exists()) { - testFile.deleteRecursively() + + for (path in paths) { + val testFile = File(path) + if (testFile.exists()) { + testFile.deleteRecursively() + } } } @WorkerThread - fun changePath(path: String) { + fun changePath(requestedPath: String) { val latch = CountDownLatch(1) activity.runOnUiThread { FileSource.setResourcesCachePath( - path, + requestedPath, object : FileSource.ResourcesCachePathChangeCallback { override fun onSuccess(path: String) { + Assert.assertEquals(requestedPath, path) latch.countDown() } override fun onError(message: String) { - Assert.fail("Resource path change failed - path: $path, message: $message") + Assert.fail("Resource path change failed - path: $requestedPath, message: $message") } }) } diff --git a/platform/android/src/file_source.cpp b/platform/android/src/file_source.cpp index 41081cd0fb7..ba5f18dbc40 100644 --- a/platform/android/src/file_source.cpp +++ b/platform/android/src/file_source.cpp @@ -77,10 +77,25 @@ void FileSource::setResourceTransform(jni::JNIEnv& env, const jni::Object& _callback) { + if (pathChangeCallback) { + FileSource::ResourcesCachePathChangeCallback::onError(env, _callback, jni::Make(env, "Another resources cache path change is in progress")); + return; + } + std::string newPath = jni::Make(env, path); mapbox::sqlite::setTempPath(newPath); - fileSource->setResourceCachePath(newPath + DATABASE_FILE); + + auto global = jni::NewGlobal(env, _callback); + pathChangeCallback = std::make_unique>(*Scheduler::GetCurrent(), + [this, callback = std::make_shared(std::move(global)), newPath] { + android::UniqueEnv _env = android::AttachEnv(); + FileSource::ResourcesCachePathChangeCallback::onSuccess(*_env, *callback, jni::Make(*_env, newPath)); + pathChangeCallback.reset(); + }); + + fileSource->setResourceCachePath(newPath + DATABASE_FILE, pathChangeCallback->self()); } void FileSource::resume(jni::JNIEnv&) { @@ -123,11 +138,32 @@ mbgl::ResourceOptions FileSource::getSharedResourceOptions(jni::JNIEnv& env, con return fileSource->resourceOptions.clone(); } +// FileSource::ResourcesCachePathChangeCallback // + +void FileSource::ResourcesCachePathChangeCallback::onSuccess(jni::JNIEnv& env, + const jni::Object& callback, + const jni::String& path) { + static auto& javaClass = jni::Class::Singleton(env); + static auto method = javaClass.GetMethod(env, "onSuccess"); + + callback.Call(env, method, path); +} + +void FileSource::ResourcesCachePathChangeCallback::onError(jni::JNIEnv& env, + const jni::Object& callback, + const jni::String& message) { + static auto& javaClass = jni::Class::Singleton(env); + static auto method = javaClass.GetMethod(env, "onError"); + + callback.Call(env, method, message); +} + void FileSource::registerNative(jni::JNIEnv& env) { - // Ensure the class for ResourceTransformCallback is cached. If it's requested for the + // Ensure the classes are cached. If they're requested for the // first time on a background thread, Android's class loader heuristics will fail. // https://developer.android.com/training/articles/perf-jni#faq_FindClass jni::Class::Singleton(env); + jni::Class::Singleton(env); static auto& javaClass = jni::Class::Singleton(env); diff --git a/platform/android/src/file_source.hpp b/platform/android/src/file_source.hpp index 3001a5e0f09..f3ad33eb317 100644 --- a/platform/android/src/file_source.hpp +++ b/platform/android/src/file_source.hpp @@ -11,6 +11,7 @@ namespace mbgl { template class Actor; class ResourceTransform; +using mbgl::PathChangeCallback; namespace android { @@ -28,6 +29,18 @@ class FileSource { static std::string onURL(jni::JNIEnv&, const jni::Object&, int, std::string); }; + struct ResourcesCachePathChangeCallback { + static constexpr auto Name() { return "com/mapbox/mapboxsdk/storage/FileSource$ResourcesCachePathChangeCallback";} + + static void onSuccess(jni::JNIEnv&, + const jni::Object&, + const jni::String&); + + static void onError(jni::JNIEnv&, + const jni::Object&, + const jni::String&); + }; + FileSource(jni::JNIEnv&, const jni::String&, const jni::String&, const jni::Object&); ~FileSource(); @@ -40,7 +53,7 @@ class FileSource { void setResourceTransform(jni::JNIEnv&, const jni::Object&); - void setResourceCachePath(jni::JNIEnv&, const jni::String&); + void setResourceCachePath(jni::JNIEnv&, const jni::String&, const jni::Object&); void resume(jni::JNIEnv&); @@ -59,6 +72,7 @@ class FileSource { optional activationCounter; mbgl::ResourceOptions resourceOptions; std::unique_ptr> resourceTransform; + std::unique_ptr> pathChangeCallback; std::shared_ptr fileSource; }; diff --git a/platform/default/src/mbgl/storage/default_file_source.cpp b/platform/default/src/mbgl/storage/default_file_source.cpp index 2768f9d1139..eabb7b45c39 100644 --- a/platform/default/src/mbgl/storage/default_file_source.cpp +++ b/platform/default/src/mbgl/storage/default_file_source.cpp @@ -46,8 +46,11 @@ class DefaultFileSource::Impl { onlineFileSource.setResourceTransform(std::move(transform)); } - void setResourceCachePath(const std::string& path) { + void setResourceCachePath(const std::string& path, optional>&& callback) { offlineDatabase->changePath(path); + if (callback) { + callback->invoke(&PathChangeCallback::operator()); + } } void listRegions(std::function)> callback) { @@ -252,8 +255,8 @@ void DefaultFileSource::setResourceTransform(optionalactor().invoke(&Impl::setResourceTransform, std::move(transform)); } -void DefaultFileSource::setResourceCachePath(const std::string& path) { - impl->actor().invoke(&Impl::setResourceCachePath, path); +void DefaultFileSource::setResourceCachePath(const std::string& path, optional>&& callback) { + impl->actor().invoke(&Impl::setResourceCachePath, path, std::move(callback)); } std::unique_ptr DefaultFileSource::request(const Resource& resource, Callback callback) { diff --git a/test/storage/default_file_source.test.cpp b/test/storage/default_file_source.test.cpp index e1c5df68959..64b23e315c4 100644 --- a/test/storage/default_file_source.test.cpp +++ b/test/storage/default_file_source.test.cpp @@ -572,6 +572,18 @@ TEST(DefaultFileSource, TEST_REQUIRES_SERVER(SetResourceTransform)) { loop.run(); } +TEST(DefaultFileSource, SetResourceCachePath) { + util::RunLoop loop; + DefaultFileSource fs(":memory:", "."); + + Actor callback(loop, [&]() -> void { + loop.stop(); + }); + + fs.setResourceCachePath("./new_offline.db", callback.self()); + loop.run(); +} + // Test that a stale cache file that has must-revalidate set will trigger a response. TEST(DefaultFileSource, TEST_REQUIRES_SERVER(RespondToStaleMustRevalidate)) { util::RunLoop loop;