Merge branch 'master' into dependency-injection

app/build.gradle

@@ -83,7 +83,6 @@ android {
 
     defaultConfig {
         applicationId 'fr.free.nrw.commons'
-
         versionCode 76
         versionName '2.6.1'
         setProperty("archivesBaseName", "app-commons-v$versionName-" + getBranchName())
@@ -104,6 +103,7 @@ android {
             proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.txt'
         }
         debug {
+            applicationIdSuffix ".debug"
             testCoverageEnabled true
             versionNameSuffix "-debug-" + getBranchName() + "~" + getBuildVersion()
         }

app/src/main/AndroidManifest.xml

@@ -16,6 +16,9 @@
     <uses-permission android:name="com.google.android.apps.photos.permission.GOOGLE_PHOTOS" />
     <uses-permission android:name="android.permission.READ_LOGS"/>
 
+    <!-- Needed only if your app targets Android 5.0 (API level 21) or higher. -->
+    <uses-feature android:name="android.hardware.location.gps" />
+
     <application
         android:name=".CommonsApplication"
         android:icon="@drawable/ic_launcher"

app/src/main/java/fr/free/nrw/commons/location/LatLng.java

@@ -1,5 +1,8 @@
 package fr.free.nrw.commons.location;
 
+import android.location.Location;
+import android.support.annotation.NonNull;
+
 public class LatLng {
 
     private final double latitude;
@@ -22,6 +25,10 @@ public class LatLng {
         this.accuracy = accuracy;
     }
 
+    public static LatLng from(@NonNull Location location) {
+        return new LatLng(location.getLatitude(), location.getLongitude(), location.getAccuracy());
+    }
+
     public int hashCode() {
         boolean var1 = true;
         byte var2 = 1;

app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java

@@ -1,11 +1,15 @@
 package fr.free.nrw.commons.location;
 
+import android.Manifest;
+import android.app.Activity;
 import android.content.Context;
-import android.location.Criteria;
+import android.content.pm.PackageManager;
 import android.location.Location;
 import android.location.LocationListener;
 import android.location.LocationManager;
 import android.os.Bundle;
+import android.support.v4.app.ActivityCompat;
+import android.support.v4.content.ContextCompat;
 
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -16,58 +20,135 @@ import javax.inject.Singleton;
 import timber.log.Timber;
 
 public class LocationServiceManager implements LocationListener {
+    public static final int LOCATION_REQUEST = 1;
 
-    private String provider;
+    private static final long MIN_LOCATION_UPDATE_REQUEST_TIME_IN_MILLIS = 2 * 60 * 1000;
+    private static final long MIN_LOCATION_UPDATE_REQUEST_DISTANCE_IN_METERS = 10;
+
+    private Context context;
     private LocationManager locationManager;
-    private LatLng lastLocation;
+    private Location lastLocation;
-    private Float latestLocationAccuracy;
     private final List<LocationUpdateListener> locationListeners = new CopyOnWriteArrayList<>();
+    private boolean isLocationManagerRegistered = false;
 
     public LocationServiceManager(Context context) {
+        this.context = context;
         this.locationManager = (LocationManager) context.getSystemService(Context.LOCATION_SERVICE);
-        provider = locationManager.getBestProvider(new Criteria(), true);
     }
 
     public boolean isProviderEnabled() {
         return locationManager.isProviderEnabled(LocationManager.GPS_PROVIDER);
     }
 
-    public LatLng getLastLocation() {
+    public boolean isLocationPermissionGranted() {
-        return lastLocation;
+        return ContextCompat.checkSelfPermission(context,
+                Manifest.permission.ACCESS_FINE_LOCATION) == PackageManager.PERMISSION_GRANTED;
     }
 
-    /**
+    public void requestPermissions(Activity activity) {
-     * Returns the accuracy of the location. The measurement is
+        if (activity.isFinishing()) {
-     * given as a radius in meter of 68 % confidence.
+            return;
-     *
+        }
-     * @return Float
+        ActivityCompat.requestPermissions(activity,
-     */
+                new String[]{Manifest.permission.ACCESS_FINE_LOCATION},
-    public Float getLatestLocationAccuracy() {
+                LOCATION_REQUEST);
-        return latestLocationAccuracy;
+    }
+
+    public boolean isPermissionExplanationRequired(Activity activity) {
+        if (activity.isFinishing()) {
+            return false;
+        }
+        return ActivityCompat.shouldShowRequestPermissionRationale(activity,
+                Manifest.permission.ACCESS_FINE_LOCATION);
+    }
+
+    public LatLng getLastLocation() {
+        if (lastLocation == null) {
+            return null;
+        }
+        return LatLng.from(lastLocation);
     }
 
     /** Registers a LocationManager to listen for current location.
      */
     public void registerLocationManager() {
+        if (!isLocationManagerRegistered)
+            isLocationManagerRegistered = requestLocationUpdatesFromProvider(LocationManager.NETWORK_PROVIDER)
+                    && requestLocationUpdatesFromProvider(LocationManager.GPS_PROVIDER);
+    }
+
+    private boolean requestLocationUpdatesFromProvider(String locationProvider) {
         try {
-            locationManager.requestLocationUpdates(provider, 400, 1, this);
+            locationManager.requestLocationUpdates(locationProvider,
-            Location location = locationManager.getLastKnownLocation(provider);
+                    MIN_LOCATION_UPDATE_REQUEST_TIME_IN_MILLIS,
-            //Location works, just need to 'send' GPS coords
+                    MIN_LOCATION_UPDATE_REQUEST_DISTANCE_IN_METERS,
-            // via emulator extended controls if testing on emulator
+                    this);
-            Timber.d("Checking for location...");
+            return true;
-            if (location != null) {
-                this.onLocationChanged(location);
-            }
         } catch (IllegalArgumentException e) {
             Timber.e(e, "Illegal argument exception");
+            return false;
         } catch (SecurityException e) {
             Timber.e(e, "Security exception");
+            return false;
         }
     }
 
+    protected boolean isBetterLocation(Location location, Location currentBestLocation) {
+        if (currentBestLocation == null) {
+            // A new location is always better than no location
+            return true;
+        }
+
+        // Check whether the new location fix is newer or older
+        long timeDelta = location.getTime() - currentBestLocation.getTime();
+        boolean isSignificantlyNewer = timeDelta > MIN_LOCATION_UPDATE_REQUEST_TIME_IN_MILLIS;
+        boolean isSignificantlyOlder = timeDelta < -MIN_LOCATION_UPDATE_REQUEST_TIME_IN_MILLIS;
+        boolean isNewer = timeDelta > 0;
+
+        // If it's been more than two minutes since the current location, use the new location
+        // because the user has likely moved
+        if (isSignificantlyNewer) {
+            return true;
+            // If the new location is more than two minutes older, it must be worse
+        } else if (isSignificantlyOlder) {
+            return false;
+        }
+
+        // Check whether the new location fix is more or less accurate
+        int accuracyDelta = (int) (location.getAccuracy() - currentBestLocation.getAccuracy());
+        boolean isLessAccurate = accuracyDelta > 0;
+        boolean isMoreAccurate = accuracyDelta < 0;
+        boolean isSignificantlyLessAccurate = accuracyDelta > 200;
+
+        // Check if the old and new location are from the same provider
+        boolean isFromSameProvider = isSameProvider(location.getProvider(),
+                currentBestLocation.getProvider());
+
+        // Determine location quality using a combination of timeliness and accuracy
+        if (isMoreAccurate) {
+            return true;
+        } else if (isNewer && !isLessAccurate) {
+            return true;
+        } else if (isNewer && !isSignificantlyLessAccurate && isFromSameProvider) {
+            return true;
+        }
+        return false;
+    }
+
+    /**
+     * Checks whether two providers are the same
+     */
+    private boolean isSameProvider(String provider1, String provider2) {
+        if (provider1 == null) {
+            return provider2 == null;
+        }
+        return provider1.equals(provider2);
+    }
+
     /** Unregisters location manager.
      */
     public void unregisterLocationManager() {
+        isLocationManagerRegistered = false;
         try {
             locationManager.removeUpdates(this);
         } catch (SecurityException e) {
@@ -87,15 +168,11 @@ public class LocationServiceManager implements LocationListener {
 
     @Override
     public void onLocationChanged(Location location) {
-        double currentLatitude = location.getLatitude();
+        if (isBetterLocation(location, lastLocation)) {
-        double currentLongitude = location.getLongitude();
+            lastLocation = location;
-        latestLocationAccuracy = location.getAccuracy();
+            for (LocationUpdateListener listener : locationListeners) {
-        Timber.d("Latitude: %f Longitude: %f Accuracy %f",
+                listener.onLocationChanged(LatLng.from(lastLocation));
-                currentLatitude, currentLongitude, latestLocationAccuracy);
+            }
-        lastLocation = new LatLng(currentLatitude, currentLongitude, latestLocationAccuracy);
-
-        for (LocationUpdateListener listener : locationListeners) {
-            listener.onLocationChanged(lastLocation);
         }
     }
 

app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java

@@ -1,6 +1,5 @@
 package fr.free.nrw.commons.nearby;
 
-import android.Manifest;
 import android.content.Context;
 import android.content.Intent;
 import android.content.SharedPreferences;
@@ -10,10 +9,8 @@ import android.os.Build;
 import android.os.Bundle;
 import android.preference.PreferenceManager;
 import android.support.annotation.NonNull;
-import android.support.v4.app.ActivityCompat;
 import android.support.v4.app.Fragment;
 import android.support.v4.app.FragmentTransaction;
-import android.support.v4.content.ContextCompat;
 import android.support.v7.app.AlertDialog;
 import android.view.Menu;
 import android.view.MenuInflater;
@@ -44,6 +41,8 @@ import io.reactivex.disposables.Disposable;
 import io.reactivex.schedulers.Schedulers;
 import timber.log.Timber;
 
+import static fr.free.nrw.commons.location.LocationServiceManager.LOCATION_REQUEST;
+
 
 public class NearbyActivity extends NavigationBaseActivity implements LocationUpdateListener {
 
@@ -71,7 +70,6 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
         sharedPreferences = PreferenceManager.getDefaultSharedPreferences(getApplicationContext());
         setContentView(R.layout.activity_nearby);
         ButterKnife.bind(this);
-        checkLocationPermission();
         bundle = new Bundle();
         initDrawer();
         initViewState();
@@ -103,7 +101,7 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
         // Handle item selection
         switch (item.getItemId()) {
             case R.id.action_refresh:
-                lockNearbyView = false;
+                lockNearbyView(false);
                 refreshView(true);
                 return true;
             case R.id.action_toggle_view:
@@ -116,52 +114,9 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
         }
     }
 
-    private void checkLocationPermission() {
+    private void requestLocationPermissions() {
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
+        if (!isFinishing()) {
-            if (ContextCompat.checkSelfPermission(this,
+            locationManager.requestPermissions(this);
-                    Manifest.permission.ACCESS_FINE_LOCATION) == PackageManager.PERMISSION_GRANTED) {
-                refreshView(false);
-            } else {
-                if (ContextCompat.checkSelfPermission(this,
-                        Manifest.permission.ACCESS_FINE_LOCATION)
-                        != PackageManager.PERMISSION_GRANTED) {
-
-                    // Should we show an explanation?
-                    if (ActivityCompat.shouldShowRequestPermissionRationale(this,
-                            Manifest.permission.ACCESS_FINE_LOCATION)) {
-
-                        // Show an explanation to the user *asynchronously* -- don't block
-                        // this thread waiting for the user's response! After the user
-                        // sees the explanation, try again to request the permission.
-
-                        new AlertDialog.Builder(this)
-                                .setMessage(getString(R.string.location_permission_rationale))
-                                .setPositiveButton("OK", (dialog, which) -> {
-                                    ActivityCompat.requestPermissions(NearbyActivity.this,
-                                            new String[]{Manifest.permission.ACCESS_FINE_LOCATION},
-                                            LOCATION_REQUEST);
-                                    dialog.dismiss();
-                                })
-                                .setNegativeButton("Cancel", null)
-                                .create()
-                                .show();
-
-                    } else {
-
-                        // No explanation needed, we can request the permission.
-
-                        ActivityCompat.requestPermissions(this,
-                                new String[]{Manifest.permission.ACCESS_FINE_LOCATION},
-                                LOCATION_REQUEST);
-
-                        // MY_PERMISSIONS_REQUEST_READ_CONTACTS is an
-                        // app-defined int constant. The callback method gets the
-                        // result of the request.
-                    }
-                }
-            }
-        } else {
-            refreshView(false);
         }
     }
 
@@ -186,7 +141,7 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
                 .setCancelable(false)
                 .setPositiveButton(R.string.give_permission, (dialog, which) -> {
                     //will ask for the location permission again
-                    checkLocationPermission();
+                    checkGps();
                 })
                 .setNegativeButton(R.string.cancel, (dialog, which) -> {
                     //dismiss dialog and finish activity
@@ -210,11 +165,48 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
                                 Timber.d("Loaded settings page");
                                 startActivityForResult(callGPSSettingIntent, 1);
                             })
-                    .setNegativeButton(R.string.menu_cancel_upload, (dialog, id) -> dialog.cancel())
+                    .setNegativeButton(R.string.menu_cancel_upload, (dialog, id) -> {
+                        showLocationPermissionDeniedErrorDialog();
+                        dialog.cancel();
+                    })
                     .create()
                     .show();
         } else {
             Timber.d("GPS is enabled");
+            checkLocationPermission();
+        }
+    }
+
+    private void checkLocationPermission() {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
+            if (locationManager.isLocationPermissionGranted()) {
+                refreshView(false);
+            } else {
+                // Should we show an explanation?
+                if (locationManager.isPermissionExplanationRequired(this)) {
+                    // Show an explanation to the user *asynchronously* -- don't block
+                    // this thread waiting for the user's response! After the user
+                    // sees the explanation, try again to request the permission.
+                    new AlertDialog.Builder(this)
+                            .setMessage(getString(R.string.location_permission_rationale_nearby))
+                            .setPositiveButton("OK", (dialog, which) -> {
+                                requestLocationPermissions();
+                                dialog.dismiss();
+                            })
+                            .setNegativeButton("Cancel", (dialog, id) -> {
+                                showLocationPermissionDeniedErrorDialog();
+                                dialog.cancel();
+                            })
+                            .create()
+                            .show();
+
+                } else {
+                    // No explanation needed, we can request the permission.
+                    requestLocationPermissions();
+                }
+            }
+        } else {
+            refreshView(false);
         }
     }
 
@@ -239,7 +231,6 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
     @Override
     protected void onStart() {
         super.onStart();
-        locationManager.registerLocationManager();
         locationManager.addLocationListener(this);
     }
 
@@ -263,13 +254,18 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
         super.onResume();
         lockNearbyView = false;
         checkGps();
-        refreshView(false);
     }
 
+    /**
+     * This method should be the single point to load/refresh nearby places
+     *
+     * @param isHardRefresh
+     */
     private void refreshView(boolean isHardRefresh) {
         if (lockNearbyView) {
             return;
         }
+        locationManager.registerLocationManager();
         LatLng lastLocation = locationManager.getLastLocation();
         if (curLatLang != null && curLatLang.equals(lastLocation)) { //refresh view only if location has changed
             if (isHardRefresh) {
@@ -309,7 +305,7 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
         bundle.putString("PlaceList", gsonPlaceList);
         bundle.putString("CurLatLng", gsonCurLatLng);
 
-        lockNearbyView = true;
+        lockNearbyView(true);
         // Begin the transaction
         if (viewMode.isMap()) {
             setMapFragment();
@@ -320,6 +316,18 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp
         hideProgressBar();
     }
 
+    private void lockNearbyView(boolean lock) {
+        if (lock) {
+            lockNearbyView = true;
+            locationManager.unregisterLocationManager();
+            locationManager.removeLocationListener(this);
+        } else {
+            lockNearbyView = false;
+            locationManager.registerLocationManager();
+            locationManager.addLocationListener(this);
+        }
+    }
+
     private void hideProgressBar() {
         if (progressBar != null) {
             progressBar.setVisibility(View.GONE);

app/src/main/res/values/strings.xml

@@ -213,4 +213,5 @@ Tap this message (or hit back) to skip this step.</string>
 
   <string name="nearby_location_has_not_changed">Location has not changed.</string>
   <string name="nearby_location_not_available">Location not available.</string>
+  <string name="location_permission_rationale_nearby">Permission required to display a list of nearby places</string>
 </resources>

app/src/test/java/fr/free/nrw/commons/nearby/NearbyActivityTest.java

@@ -1,5 +1,6 @@
 package fr.free.nrw.commons.nearby;
 
+import android.app.Activity;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;