From d4bafd94e070219d4cbd5ee40dfb747c61fe5125 Mon Sep 17 00:00:00 2001 From: maskara Date: Tue, 7 Nov 2017 01:43:21 +0530 Subject: [PATCH 1/5] Fixes leaks in contribution and nearby activity --- .../free/nrw/commons/CommonsApplication.java | 16 +- .../free/nrw/commons/auth/LoginActivity.java | 4 + .../contributions/ContributionsActivity.java | 27 ++-- .../location/LocationServiceManager.java | 4 + .../nrw/commons/nearby/NearbyActivity.java | 139 +++++++----------- 5 files changed, 84 insertions(+), 106 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java b/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java index 44bf72ff4..bff65b0d2 100644 --- a/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java +++ b/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java @@ -77,6 +77,8 @@ public class CommonsApplication extends Application implements HasActivityInject private DBOpenHelper dbOpenHelper = null; private NearbyPlaces nearbyPlaces = null; + private RefWatcher refWatcher; + /** * This should not be called by ANY application code (other than the magic Android glue) * Use CommonsApplication.getInstance() instead to get the singleton. @@ -128,7 +130,11 @@ public class CommonsApplication extends Application implements HasActivityInject public void onCreate() { super.onCreate(); - setupLeakCanary(); + if (LeakCanary.isInAnalyzerProcess(this)) { + refWatcher = RefWatcher.DISABLED; + return; + } + refWatcher = LeakCanary.install(this); Timber.plant(new Timber.DebugTree()); @@ -153,11 +159,9 @@ public class CommonsApplication extends Application implements HasActivityInject cacheData = new CacheController(); } - protected RefWatcher setupLeakCanary() { - if (LeakCanary.isInAnalyzerProcess(this)) { - return RefWatcher.DISABLED; - } - return LeakCanary.install(this); + public static RefWatcher getRefWatcher(Context context) { + CommonsApplication application = (CommonsApplication) context.getApplicationContext(); + return application.refWatcher; } /** diff --git a/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java b/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java index 9e3423928..6d8e60b3b 100644 --- a/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java @@ -21,6 +21,8 @@ import android.widget.Button; import android.widget.EditText; import android.widget.TextView; +import com.squareup.leakcanary.RefWatcher; + import butterknife.BindView; import butterknife.ButterKnife; import fr.free.nrw.commons.BuildConfig; @@ -110,6 +112,8 @@ public class LoginActivity extends AccountAuthenticatorActivity { twoFactorEdit.removeTextChangedListener(textWatcher); delegate.onDestroy(); super.onDestroy(); + RefWatcher refWatcher = CommonsApplication.getRefWatcher(this); + refWatcher.watch(this); } @Override diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java index 63a6e9c64..1fec12dd7 100644 --- a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java @@ -38,6 +38,7 @@ import fr.free.nrw.commons.settings.Prefs; import fr.free.nrw.commons.upload.UploadService; import io.reactivex.android.schedulers.AndroidSchedulers; import io.reactivex.disposables.CompositeDisposable; +import io.reactivex.disposables.Disposable; import io.reactivex.schedulers.Schedulers; import timber.log.Timber; @@ -156,6 +157,7 @@ public class ContributionsActivity extends AuthenticatedActivity requestAuthToken(); initDrawer(); setTitle(getString(R.string.title_activity_contributions)); + setUploadCount(); } @Override @@ -255,8 +257,6 @@ public class ContributionsActivity extends AuthenticatedActivity ((CursorAdapter) contributionsList.getAdapter()).swapCursor(cursor); } - setUploadCount(); - contributionsList.clearSyncMessage(); notifyAndMigrateDataSetObservers(); } @@ -288,18 +288,17 @@ public class ContributionsActivity extends AuthenticatedActivity @SuppressWarnings("ConstantConditions") private void setUploadCount() { CommonsApplication app = ((CommonsApplication) getApplication()); - compositeDisposable.add( - mediaWikiApi - .getUploadCount(app.getCurrentAccount().name) - .subscribeOn(Schedulers.io()) - .observeOn(AndroidSchedulers.mainThread()) - .subscribe( - uploadCount -> getSupportActionBar().setSubtitle(getResources() - .getQuantityString(R.plurals.contributions_subtitle, - uploadCount, uploadCount)), - t -> Timber.e(t, "Fetching upload count failed") - ) - ); + Disposable uploadCountDisposable = mediaWikiApi + .getUploadCount(app.getCurrentAccount().name) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + uploadCount -> getSupportActionBar().setSubtitle(getResources() + .getQuantityString(R.plurals.contributions_subtitle, + uploadCount, uploadCount)), + t -> Timber.e(t, "Fetching upload count failed") + ); + compositeDisposable.add(uploadCountDisposable); } @Override diff --git a/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java b/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java index e87eaf92e..e1156daa3 100644 --- a/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java +++ b/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java @@ -21,6 +21,10 @@ public class LocationServiceManager implements LocationListener { provider = locationManager.getBestProvider(new Criteria(), true); } + public boolean isProviderEnabled() { + return locationManager.isProviderEnabled(LocationManager.GPS_PROVIDER); + } + public LatLng getLatestLocation() { return latestLocation; } diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java index 94b6b5f19..2d0d16286 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java @@ -5,9 +5,7 @@ import android.content.Context; import android.content.Intent; import android.content.SharedPreferences; import android.content.pm.PackageManager; -import android.location.LocationManager; import android.net.Uri; -import android.os.AsyncTask; import android.os.Build; import android.os.Bundle; import android.preference.PreferenceManager; @@ -37,6 +35,9 @@ import fr.free.nrw.commons.location.LatLng; import fr.free.nrw.commons.location.LocationServiceManager; import fr.free.nrw.commons.theme.NavigationBaseActivity; import fr.free.nrw.commons.utils.UriSerializer; +import io.reactivex.Observable; +import io.reactivex.android.schedulers.AndroidSchedulers; +import io.reactivex.schedulers.Schedulers; import timber.log.Timber; @@ -50,7 +51,6 @@ public class NearbyActivity extends NavigationBaseActivity { private LocationServiceManager locationManager; private LatLng curLatLang; private Bundle bundle; - private NearbyAsyncTask nearbyAsyncTask; private SharedPreferences sharedPreferences; private NearbyActivityMode viewMode; @@ -60,6 +60,7 @@ public class NearbyActivity extends NavigationBaseActivity { sharedPreferences = PreferenceManager.getDefaultSharedPreferences(getApplicationContext()); setContentView(R.layout.activity_nearby); ButterKnife.bind(this); + locationManager = new LocationServiceManager(this); checkLocationPermission(); bundle = new Bundle(); initDrawer(); @@ -105,11 +106,8 @@ public class NearbyActivity extends NavigationBaseActivity { } private void startLookingForNearby() { - locationManager = new LocationServiceManager(this); - locationManager.registerLocationManager(); curLatLang = locationManager.getLatestLocation(); - nearbyAsyncTask = new NearbyAsyncTask(this); - nearbyAsyncTask.execute(); + setupPlaceList(getBaseContext()); } private void checkLocationPermission() { @@ -169,9 +167,6 @@ public class NearbyActivity extends NavigationBaseActivity { startLookingForNearby(); } else { //If permission not granted, go to page that says Nearby Places cannot be displayed - if (nearbyAsyncTask != null) { - nearbyAsyncTask.cancel(true); - } if (progressBar != null) { progressBar.setVisibility(View.GONE); } @@ -200,8 +195,7 @@ public class NearbyActivity extends NavigationBaseActivity { } private void checkGps() { - LocationManager manager = (LocationManager) getSystemService(LOCATION_SERVICE); - if (!manager.isProviderEnabled(LocationManager.GPS_PROVIDER)) { + if (!locationManager.isProviderEnabled()) { Timber.d("GPS is not enabled"); new AlertDialog.Builder(this) .setMessage(R.string.gps_disabled) @@ -231,16 +225,24 @@ public class NearbyActivity extends NavigationBaseActivity { } private void toggleView() { - if (nearbyAsyncTask != null) { - if (nearbyAsyncTask.getStatus() == AsyncTask.Status.FINISHED) { - if (viewMode.isMap()) { - setMapFragment(); - } else { - setListFragment(); - } - } - sharedPreferences.edit().putBoolean(MAP_LAST_USED_PREFERENCE, viewMode.isMap()).apply(); + if (viewMode.isMap()) { + setMapFragment(); + } else { + setListFragment(); } + sharedPreferences.edit().putBoolean(MAP_LAST_USED_PREFERENCE, viewMode.isMap()).apply(); + } + + @Override + protected void onStart() { + super.onStart(); + locationManager.registerLocationManager(); + } + + @Override + protected void onStop() { + super.onStop(); + locationManager.unregisterLocationManager(); } @Override @@ -249,83 +251,48 @@ public class NearbyActivity extends NavigationBaseActivity { checkGps(); } - @Override - protected void onPause() { - super.onPause(); - if (nearbyAsyncTask != null) { - nearbyAsyncTask.cancel(true); - } - } - private void refreshView() { curLatLang = locationManager.getLatestLocation(); progressBar.setVisibility(View.VISIBLE); - nearbyAsyncTask = new NearbyAsyncTask(this); - nearbyAsyncTask.execute(); + setupPlaceList(getBaseContext()); } - @Override - protected void onDestroy() { - super.onDestroy(); - if (locationManager != null) { - locationManager.unregisterLocationManager(); - } + private void setupPlaceList(Context context) { + Observable.fromCallable(() -> NearbyController + .loadAttractionsFromLocation(curLatLang, CommonsApplication.getInstance())) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe((result) -> { + populatePlaces(context, result); + }); } - private class NearbyAsyncTask extends AsyncTask> { + private void populatePlaces(Context context, List placeList) { + Gson gson = new GsonBuilder() + .registerTypeAdapter(Uri.class, new UriSerializer()) + .create(); + String gsonPlaceList = gson.toJson(placeList); + String gsonCurLatLng = gson.toJson(curLatLang); - private final Context mContext; - - private NearbyAsyncTask(Context context) { - mContext = context; + if (placeList.size() == 0) { + int duration = Toast.LENGTH_SHORT; + Toast toast = Toast.makeText(context, R.string.no_nearby, duration); + toast.show(); } - @Override - protected void onProgressUpdate(Integer... values) { - super.onProgressUpdate(values); + bundle.clear(); + bundle.putString("PlaceList", gsonPlaceList); + bundle.putString("CurLatLng", gsonCurLatLng); + + // Begin the transaction + if (viewMode.isMap()) { + setMapFragment(); + } else { + setListFragment(); } - @Override - protected List doInBackground(Void... params) { - return NearbyController - .loadAttractionsFromLocation(curLatLang, CommonsApplication.getInstance() - ); - } - - @Override - protected void onPostExecute(List placeList) { - super.onPostExecute(placeList); - - if (isCancelled()) { - return; - } - - Gson gson = new GsonBuilder() - .registerTypeAdapter(Uri.class, new UriSerializer()) - .create(); - String gsonPlaceList = gson.toJson(placeList); - String gsonCurLatLng = gson.toJson(curLatLang); - - if (placeList.size() == 0) { - int duration = Toast.LENGTH_SHORT; - Toast toast = Toast.makeText(mContext, R.string.no_nearby, duration); - toast.show(); - } - - bundle.clear(); - bundle.putString("PlaceList", gsonPlaceList); - bundle.putString("CurLatLng", gsonCurLatLng); - - // Begin the transaction - if (viewMode.isMap()) { - setMapFragment(); - } else { - setListFragment(); - } - - if (progressBar != null) { - progressBar.setVisibility(View.GONE); - } + if (progressBar != null) { + progressBar.setVisibility(View.GONE); } } From a59bf1bc4b36c41b4b52f13dbe53920775a6d25a Mon Sep 17 00:00:00 2001 From: maskara Date: Tue, 7 Nov 2017 02:01:14 +0530 Subject: [PATCH 2/5] make location manager singleton --- .../commons/location/LocationServiceManager.java | 15 +++++++++++++-- .../free/nrw/commons/nearby/NearbyActivity.java | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java b/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java index e1156daa3..946b8ff7e 100644 --- a/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java +++ b/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java @@ -7,6 +7,7 @@ import android.location.LocationListener; import android.location.LocationManager; import android.os.Bundle; +import fr.free.nrw.commons.CommonsApplication; import timber.log.Timber; public class LocationServiceManager implements LocationListener { @@ -16,11 +17,21 @@ public class LocationServiceManager implements LocationListener { private LatLng latestLocation; private Float latestLocationAccuracy; - public LocationServiceManager(Context context) { - this.locationManager = (LocationManager) context.getSystemService(Context.LOCATION_SERVICE); + private static LocationServiceManager locationServiceManager; + + private LocationServiceManager() { + Context applicationContext = CommonsApplication.getInstance().getApplicationContext(); + this.locationManager = (LocationManager) applicationContext.getSystemService(Context.LOCATION_SERVICE); provider = locationManager.getBestProvider(new Criteria(), true); } + public static LocationServiceManager getInstance() { + if (locationServiceManager == null) { + locationServiceManager = new LocationServiceManager(); + } + return locationServiceManager; + } + public boolean isProviderEnabled() { return locationManager.isProviderEnabled(LocationManager.GPS_PROVIDER); } diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java index 2d0d16286..31a684f56 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java @@ -60,7 +60,7 @@ public class NearbyActivity extends NavigationBaseActivity { sharedPreferences = PreferenceManager.getDefaultSharedPreferences(getApplicationContext()); setContentView(R.layout.activity_nearby); ButterKnife.bind(this); - locationManager = new LocationServiceManager(this); + locationManager = LocationServiceManager.getInstance(); checkLocationPermission(); bundle = new Bundle(); initDrawer(); From 0101dc4820899373fae9df9f1433c98dcef7067c Mon Sep 17 00:00:00 2001 From: maskara Date: Tue, 7 Nov 2017 02:07:32 +0530 Subject: [PATCH 3/5] Cleanup --- app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java b/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java index 6d8e60b3b..9e3423928 100644 --- a/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/auth/LoginActivity.java @@ -21,8 +21,6 @@ import android.widget.Button; import android.widget.EditText; import android.widget.TextView; -import com.squareup.leakcanary.RefWatcher; - import butterknife.BindView; import butterknife.ButterKnife; import fr.free.nrw.commons.BuildConfig; @@ -112,8 +110,6 @@ public class LoginActivity extends AccountAuthenticatorActivity { twoFactorEdit.removeTextChangedListener(textWatcher); delegate.onDestroy(); super.onDestroy(); - RefWatcher refWatcher = CommonsApplication.getRefWatcher(this); - refWatcher.watch(this); } @Override From 6a6ba7d542dec520870ddd46652b33c9cd76cd95 Mon Sep 17 00:00:00 2001 From: maskara Date: Tue, 7 Nov 2017 02:27:10 +0530 Subject: [PATCH 4/5] Fix Leak canary test --- .../java/fr/free/nrw/commons/CommonsApplication.java | 11 ++++++++--- .../fr/free/nrw/commons/TestCommonsApplication.java | 3 ++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java b/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java index bff65b0d2..6340c5eec 100644 --- a/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java +++ b/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java @@ -130,11 +130,9 @@ public class CommonsApplication extends Application implements HasActivityInject public void onCreate() { super.onCreate(); - if (LeakCanary.isInAnalyzerProcess(this)) { - refWatcher = RefWatcher.DISABLED; + if (setupLeakCanary() == RefWatcher.DISABLED) { return; } - refWatcher = LeakCanary.install(this); Timber.plant(new Timber.DebugTree()); @@ -159,6 +157,13 @@ public class CommonsApplication extends Application implements HasActivityInject cacheData = new CacheController(); } + protected RefWatcher setupLeakCanary() { + if (LeakCanary.isInAnalyzerProcess(this)) { + return RefWatcher.DISABLED; + } + return LeakCanary.install(this); + } + public static RefWatcher getRefWatcher(Context context) { CommonsApplication application = (CommonsApplication) context.getApplicationContext(); return application.refWatcher; diff --git a/app/src/test/java/fr/free/nrw/commons/TestCommonsApplication.java b/app/src/test/java/fr/free/nrw/commons/TestCommonsApplication.java index bf877ae95..af4d50a91 100644 --- a/app/src/test/java/fr/free/nrw/commons/TestCommonsApplication.java +++ b/app/src/test/java/fr/free/nrw/commons/TestCommonsApplication.java @@ -4,7 +4,8 @@ import com.squareup.leakcanary.RefWatcher; // This class is automatically discovered by Robolectric public class TestCommonsApplication extends CommonsApplication { - @Override protected RefWatcher setupLeakCanary() { + @Override + protected RefWatcher setupLeakCanary() { // No leakcanary in unit tests. return RefWatcher.DISABLED; } From c97f39fcdf1f7088eb8abcc0948724c440e98f13 Mon Sep 17 00:00:00 2001 From: maskara Date: Sun, 12 Nov 2017 03:17:32 +0530 Subject: [PATCH 5/5] Dispose observable for places --- .../fr/free/nrw/commons/nearby/NearbyActivity.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java index 31a684f56..8efef5ca8 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java @@ -37,6 +37,7 @@ import fr.free.nrw.commons.theme.NavigationBaseActivity; import fr.free.nrw.commons.utils.UriSerializer; import io.reactivex.Observable; import io.reactivex.android.schedulers.AndroidSchedulers; +import io.reactivex.disposables.Disposable; import io.reactivex.schedulers.Schedulers; import timber.log.Timber; @@ -53,6 +54,7 @@ public class NearbyActivity extends NavigationBaseActivity { private Bundle bundle; private SharedPreferences sharedPreferences; private NearbyActivityMode viewMode; + private Disposable placesDisposable; @Override protected void onCreate(Bundle savedInstanceState) { @@ -245,6 +247,12 @@ public class NearbyActivity extends NavigationBaseActivity { locationManager.unregisterLocationManager(); } + @Override + protected void onDestroy() { + super.onDestroy(); + placesDisposable.dispose(); + } + @Override protected void onResume() { super.onResume(); @@ -258,7 +266,7 @@ public class NearbyActivity extends NavigationBaseActivity { } private void setupPlaceList(Context context) { - Observable.fromCallable(() -> NearbyController + placesDisposable = Observable.fromCallable(() -> NearbyController .loadAttractionsFromLocation(curLatLang, CommonsApplication.getInstance())) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread())