From b7fafaa42949cbe27a36ab99b21d3c75819dbd0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Paczos?= Date: Tue, 7 May 2019 13:19:53 +0200 Subject: [android] use strong references to the offline merge callbacks --- .../mapbox/mapboxsdk/offline/OfflineManager.java | 121 +++++++++------------ .../offline/MergeOfflineRegionsActivity.kt | 26 ++++- 2 files changed, 75 insertions(+), 72 deletions(-) diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java index 8684d7c6f1..535107c529 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java @@ -2,11 +2,12 @@ package com.mapbox.mapboxsdk.offline; import android.annotation.SuppressLint; import android.content.Context; -import android.os.AsyncTask; import android.os.Handler; import android.os.Looper; +import android.support.annotation.AnyThread; import android.support.annotation.Keep; import android.support.annotation.NonNull; +import android.support.annotation.UiThread; import com.mapbox.mapboxsdk.LibraryLoader; import com.mapbox.mapboxsdk.Mapbox; @@ -21,7 +22,6 @@ import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; -import java.lang.ref.WeakReference; import java.nio.channels.FileChannel; /** @@ -32,6 +32,7 @@ import java.nio.channels.FileChannel; * * @see Offline Maps Information/ */ +@UiThread public class OfflineManager { private static final String TAG = "Mbgl - OfflineManager"; @@ -156,6 +157,7 @@ public class OfflineManager { return instance; } + @AnyThread private Handler getHandler() { if (handler == null) { handler = new Handler(Looper.getMainLooper()); @@ -205,6 +207,8 @@ public class OfflineManager { * Merge offline regions from a secondary database into the main offline database. *

* When the merge is completed, or fails, the {@link MergeOfflineRegionsCallback} will be invoked on the main thread. + * The callback reference is strongly kept throughout the process, + * so it needs to be wrapped in a weak reference or released on the client side if necessary. *

* The secondary database may need to be upgraded to the latest schema. * This is done in-place and requires write-access to the provided path. @@ -225,73 +229,50 @@ public class OfflineManager { */ public void mergeOfflineRegions(@NonNull String path, @NonNull final MergeOfflineRegionsCallback callback) { final File src = new File(path); - new FileUtils.CheckFileReadPermissionTask(new FileUtils.OnCheckFileReadPermissionListener() { + new Thread(new Runnable() { @Override - public void onReadPermissionGranted() { - new FileUtils.CheckFileWritePermissionTask(new FileUtils.OnCheckFileWritePermissionListener() { - @Override - public void onWritePermissionGranted() { - // path writable, merge and update schema in place if necessary - mergeOfflineDatabaseFiles(src, callback, false); - } - - @Override - public void onError() { - // path not writable, copy the the file to temp directory, then merge and update schema on a copy if - // necessary - File dst = new File(FileSource.getInternalCachePath(context), src.getName()); - new CopyTempDatabaseFileTask(OfflineManager.this, callback).execute(src, dst); + public void run() { + String errorMessage = null; + if (src.canWrite()) { + getHandler().post(new Runnable() { + @Override + public void run() { + // path writable, merge and update schema in place if necessary + mergeOfflineDatabaseFiles(src, callback, false); + } + }); + } else if (src.canRead()) { + // path not writable, copy the the file to temp directory + final File dst = new File(FileSource.getInternalCachePath(context), src.getName()); + try { + copyTempDatabaseFile(src, dst); + getHandler().post(new Runnable() { + @Override + public void run() { + // merge and update schema using the copy + OfflineManager.this.mergeOfflineDatabaseFiles(dst, callback, true); + } + }); + } catch (IOException ex) { + ex.printStackTrace(); + errorMessage = ex.getMessage(); } - }).execute(src); - } - - @Override - public void onError() { - // path not readable, abort - callback.onError("Secondary database needs to be located in a readable path."); - } - }).execute(src); - } - - private static final class CopyTempDatabaseFileTask extends AsyncTask { - @NonNull - private final WeakReference offlineManagerWeakReference; - @NonNull - private final WeakReference callbackWeakReference; - - CopyTempDatabaseFileTask(OfflineManager offlineManager, MergeOfflineRegionsCallback callback) { - this.offlineManagerWeakReference = new WeakReference<>(offlineManager); - this.callbackWeakReference = new WeakReference<>(callback); - } - - @Override - protected Object doInBackground(Object... objects) { - File src = (File) objects[0]; - File dst = (File) objects[1]; - - try { - copyTempDatabaseFile(src, dst); - return dst; - } catch (IOException ex) { - return ex.getMessage(); - } - } + } else { + // path not readable, abort + errorMessage = "Secondary database needs to be located in a readable path."; + } - @Override - protected void onPostExecute(Object object) { - MergeOfflineRegionsCallback callback = callbackWeakReference.get(); - if (callback != null) { - OfflineManager offlineManager = offlineManagerWeakReference.get(); - if (object instanceof File && offlineManager != null) { - // successfully copied the file, perform merge - File dst = (File) object; - offlineManager.mergeOfflineDatabaseFiles(dst, callback, true); - } else if (object instanceof String) { - // error occurred - callback.onError((String) object); + if (errorMessage != null) { + final String finalErrorMessage = errorMessage; + getHandler().post(new Runnable() { + @Override + public void run() { + callback.onError(finalErrorMessage); + } + }); } } - } + }).start(); } private static void copyTempDatabaseFile(@NonNull File sourceFile, File destFile) throws IOException { @@ -324,13 +305,13 @@ public class OfflineManager { mergeOfflineRegions(fileSource, file.getAbsolutePath(), new MergeOfflineRegionsCallback() { @Override public void onMerge(final OfflineRegion[] offlineRegions) { + if (isTemporaryFile) { + file.delete(); + } getHandler().post(new Runnable() { @Override public void run() { fileSource.deactivate(); - if (isTemporaryFile) { - file.delete(); - } callback.onMerge(offlineRegions); } }); @@ -338,13 +319,13 @@ public class OfflineManager { @Override public void onError(final String error) { + if (isTemporaryFile) { + file.delete(); + } getHandler().post(new Runnable() { @Override public void run() { fileSource.deactivate(); - if (isTemporaryFile) { - file.delete(); - } callback.onError(error); } }); diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt index e9321d5e27..12169dbe8f 100644 --- a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt @@ -24,7 +24,7 @@ class MergeOfflineRegionsActivity : AppCompatActivity() { override fun onFileCopiedFromAssets() { Toast.makeText( this@MergeOfflineRegionsActivity, - String.format("OnFileCOpied."), + String.format("OnFileCopied."), Toast.LENGTH_LONG).show() mergeDb() } @@ -57,6 +57,27 @@ class MergeOfflineRegionsActivity : AppCompatActivity() { } } + /** + * Since we expect from the results of the offline merge callback to interact with the hosting activity, + * we need to ensure that we are not interacting with a destroyed activity. + */ + private class MergeCallback(private var activityCallback: OfflineManager.MergeOfflineRegionsCallback?) : OfflineManager.MergeOfflineRegionsCallback { + + override fun onMerge(offlineRegions: Array?) { + activityCallback?.onMerge(offlineRegions) + } + + override fun onError(error: String?) { + activityCallback?.onError(error) + } + + fun onActivityDestroy() { + activityCallback = null + } + } + + private val mergeCallback = MergeCallback(onRegionMergedListener) + override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) setContentView(R.layout.activity_merge_offline_regions) @@ -81,7 +102,7 @@ class MergeOfflineRegionsActivity : AppCompatActivity() { private fun mergeDb() { OfflineManager.getInstance(this).mergeOfflineRegions( - FileSource.getResourcesCachePath(this) + "/" + TEST_DB_FILE_NAME, onRegionMergedListener + FileSource.getResourcesCachePath(this) + "/" + TEST_DB_FILE_NAME, mergeCallback ) } @@ -112,6 +133,7 @@ class MergeOfflineRegionsActivity : AppCompatActivity() { override fun onDestroy() { super.onDestroy() + mergeCallback.onActivityDestroy() mapView.onDestroy() // restoring connectivity state -- cgit v1.2.1