[android] keep strong callback reference and use application context during cache path change

4 files changed, 71 insertions, 51 deletions

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 cc4988b549..1301c103f4 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
@@ -20,7 +20,6 @@ import com.mapbox.mapboxsdk.utils.FileUtils;
 import com.mapbox.mapboxsdk.utils.ThreadUtils;
 
 import java.io.File;
-import java.lang.ref.WeakReference;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 
@@ -69,14 +68,14 @@ public class FileSource {
      *
      * @param path the path of the current resources cache database
      */
-    void onSuccess(String path);
+    void onSuccess(@NonNull String path);
 
     /**
      * Receives an error message if setting the path was not successful
      *
      * @param message the error message
      */
-    void onError(String message);
+    void onError(@NonNull String message);
 
   }
 
@@ -265,63 +264,63 @@ public class FileSource {
   /**
    * Changes the path of the resources cache database.
    * Note that the external storage setting needs to be activated in the manifest.
+   * <p>
+   * The callback reference is <b>strongly kept</b> throughout the process,
+   * so it needs to be wrapped in a weak reference or released on the client side if necessary.
    *
    * @param context  the context of the path
    * @param path     the new database path
    * @param callback the callback to obtain the result
+   * @deprecated Use {@link #setResourcesCachePath(String, ResourcesCachePathChangeCallback)}
    */
-  public static void setResourcesCachePath(@NonNull Context context,
+  @Deprecated
+  public static void setResourcesCachePath(@NonNull final Context context,
                                            @NonNull final String path,
-                                           @NonNull ResourcesCachePathChangeCallback callback) {
-    final String fileSourceActivatedMessage = "Cannot set path, file source is activated."
-      + " Make sure that the map or a resources download is not running.";
-    if (getInstance(context).isActivated()) {
+                                           @NonNull final ResourcesCachePathChangeCallback callback) {
+    setResourcesCachePath(path, callback);
+  }
+
+  /**
+   * Changes the path of the resources cache database.
+   * Note that the external storage setting needs to be activated in the manifest.
+   * <p>
+   * The callback reference is <b>strongly kept</b> throughout the process,
+   * so it needs to be wrapped in a weak reference or released on the client side if necessary.
+   *
+   * @param path     the new database path
+   * @param callback the callback to obtain the result
+   */
+  public static void setResourcesCachePath(@NonNull final String path,
+                                           @NonNull final ResourcesCachePathChangeCallback callback) {
+    final Context applicationContext = Mapbox.getApplicationContext();
+    final FileSource fileSource = FileSource.getInstance(applicationContext);
+
+    if (fileSource.isActivated()) {
+      String fileSourceActivatedMessage = "Cannot set path, file source is activated."
+        + " Make sure that the map or a resources download is not running.";
       Logger.w(TAG, fileSourceActivatedMessage);
       callback.onError(fileSourceActivatedMessage);
-    } else if (path.equals(resourcesCachePath)) {
+    } else if (path.equals(getResourcesCachePath(applicationContext))) {
       // no need to change the path
       callback.onSuccess(path);
     } else {
-      final WeakReference<Context> contextWeakReference = new WeakReference<>(context);
-      final WeakReference<ResourcesCachePathChangeCallback> callbackWeakReference = new WeakReference<>(callback);
       new FileUtils.CheckFileWritePermissionTask(new FileUtils.OnCheckFileWritePermissionListener() {
         @Override
         public void onWritePermissionGranted() {
-          final Context context = contextWeakReference.get();
-          final ResourcesCachePathChangeCallback callback = callbackWeakReference.get();
-
-          if (callback == null) {
-            Logger.w(TAG, "Lost callback reference.");
-            return;
-          } else if (context == null) {
-            String lostContextMessage = "Lost context reference.";
-            Logger.w(TAG, lostContextMessage);
-            callback.onError(lostContextMessage);
-            return;
-          }
-
-          // verify fileSource's activation again after the async task returns
-          if (getInstance(context).isActivated()) {
-            Logger.w(TAG, fileSourceActivatedMessage);
-            callback.onError(fileSourceActivatedMessage);
-          } else {
-            final SharedPreferences.Editor editor =
-              context.getSharedPreferences(MapboxConstants.MAPBOX_SHARED_PREFERENCES, Context.MODE_PRIVATE).edit();
-            editor.putString(MAPBOX_SHARED_PREFERENCE_RESOURCES_CACHE_PATH, path);
-            editor.apply();
-            setResourcesCachePath(context, path);
-            callback.onSuccess(path);
-          }
+          final SharedPreferences.Editor editor =
+            applicationContext.getSharedPreferences(MapboxConstants.MAPBOX_SHARED_PREFERENCES,
+              Context.MODE_PRIVATE).edit();
+          editor.putString(MAPBOX_SHARED_PREFERENCE_RESOURCES_CACHE_PATH, path);
+          editor.apply();
+          setResourcesCachePath(applicationContext, path);
+          callback.onSuccess(path);
         }
 
         @Override
         public void onError() {
-          final ResourcesCachePathChangeCallback callback = callbackWeakReference.get();
-          if (callback != null) {
-            String message = "Path is not writable: " + path;
-            Logger.e(TAG, message);
-            callback.onError(message);
-          }
+          String message = "Path is not writable: " + path;
+          Logger.e(TAG, message);
+          callback.onError(message);
         }
       }).execute(new File(path));
     }
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceMapTest.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceMapTest.kt
index 0d4823a93d..5e3489d755 100644
--- a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceMapTest.kt
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/storage/FileSourceMapTest.kt
@@ -36,12 +36,12 @@ open class FileSourceMapTest {
   fun changeResourcesPathWhileMapVisible() {
     val latch = CountDownLatch(1)
     rule.activity.runOnUiThread {
-      FileSource.setResourcesCachePath(rule.activity, fileSourceTestUtils.testPath, object : FileSource.ResourcesCachePathChangeCallback {
-        override fun onSuccess(path: String?) {
+      FileSource.setResourcesCachePath(fileSourceTestUtils.testPath, object : FileSource.ResourcesCachePathChangeCallback {
+        override fun onSuccess(path: String) {
           Assert.fail("Requested resources change while the map is running should fail")
         }
 
-        override fun onError(message: String?) {
+        override fun onError(message: String) {
           Assert.assertEquals("Cannot set path, file source is activated."
             + " Make sure that the map or a resources download is not running.", message)
           latch.countDown()
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 040b288aa2..c69321581a 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
@@ -33,14 +33,13 @@ class FileSourceTestUtils(private val activity: Activity) {
     val latch = CountDownLatch(1)
     activity.runOnUiThread {
       FileSource.setResourcesCachePath(
-        activity,
         path,
         object : FileSource.ResourcesCachePathChangeCallback {
-          override fun onSuccess(path: String?) {
+          override fun onSuccess(path: String) {
             latch.countDown()
           }
 
-          override fun onError(message: String?) {
+          override fun onError(message: String) {
             Assert.fail("Resource path change failed - path: $path, message: $message")
           }
         })
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/ChangeResourcesCachePathActivity.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/ChangeResourcesCachePathActivity.kt
index 75ea9f6970..74b5bde17d 100644
--- a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/ChangeResourcesCachePathActivity.kt
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/ChangeResourcesCachePathActivity.kt
@@ -32,6 +32,8 @@ class ChangeResourcesCachePathActivity : AppCompatActivity(),
 
   private lateinit var offlineManager: OfflineManager
 
+  private val callback = PathChangeCallback(this)
+
   override fun onCreate(savedInstanceState: Bundle?) {
     super.onCreate(savedInstanceState)
     setContentView(R.layout.activity_change_resources_cache_path)
@@ -50,18 +52,23 @@ class ChangeResourcesCachePathActivity : AppCompatActivity(),
     Toast.makeText(this, "Current path: $path", Toast.LENGTH_LONG).show()
   }
 
+  override fun onDestroy() {
+    super.onDestroy()
+    callback.onActivityDestroy()
+  }
+
   override fun onItemClick(parent: AdapterView<*>?, view: View?, position: Int, id: Long) {
     listView.onItemClickListener = null
     val path: String = adapter.getItem(position) as String
-    FileSource.setResourcesCachePath(this, path, this)
+    FileSource.setResourcesCachePath(path, callback)
   }
 
-  override fun onError(message: String?) {
+  override fun onError(message: String) {
     listView.onItemClickListener = this
     Toast.makeText(this, "Error: $message", Toast.LENGTH_LONG).show()
   }
 
-  override fun onSuccess(path: String?) {
+  override fun onSuccess(path: String) {
     listView.onItemClickListener = this
     Toast.makeText(this, "New path: $path", Toast.LENGTH_LONG).show()
 
@@ -118,6 +125,21 @@ class ChangeResourcesCachePathActivity : AppCompatActivity(),
     return paths
   }
 
+  private class PathChangeCallback(private var activity: ChangeResourcesCachePathActivity?) : FileSource.ResourcesCachePathChangeCallback {
+
+    override fun onSuccess(path: String) {
+      activity?.onSuccess(path)
+    }
+
+    override fun onError(message: String) {
+      activity?.onError(message)
+    }
+
+    fun onActivityDestroy() {
+      activity = null
+    }
+  }
+
   class PathAdapter(private val context: Context, private val paths: List<String>) : BaseAdapter() {
 
     override fun getItem(position: Int): Any {