Service refactoring: move location management to the LocationHandle.

Tests refactoring: location and service tests re-worked.

src/main/java/de/dennisguse/opentracks/services/TrackRecordingService.java

@@ -24,11 +24,7 @@ import android.content.Intent;
 import android.content.SharedPreferences;
 import android.content.SharedPreferences.OnSharedPreferenceChangeListener;
 import android.database.sqlite.SQLiteException;
-import android.location.Location;
-import android.location.LocationListener;
-import android.location.LocationManager;
 import android.net.Uri;
-import android.os.Bundle;
 import android.os.IBinder;
 import android.os.PowerManager.WakeLock;
 import android.util.Log;
@@ -37,9 +33,6 @@ import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 import androidx.core.app.TaskStackBuilder;
 
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-
 import de.dennisguse.opentracks.R;
 import de.dennisguse.opentracks.TrackListActivity;
 import de.dennisguse.opentracks.TrackRecordingActivity;
@@ -50,6 +43,7 @@ import de.dennisguse.opentracks.content.provider.ContentProviderUtils;
 import de.dennisguse.opentracks.content.provider.CustomContentProvider;
 import de.dennisguse.opentracks.content.provider.TrackPointIterator;
 import de.dennisguse.opentracks.content.sensor.SensorDataSet;
+import de.dennisguse.opentracks.services.handlers.HandlerServer;
 import de.dennisguse.opentracks.services.sensors.BluetoothRemoteSensorManager;
 import de.dennisguse.opentracks.services.tasks.AnnouncementPeriodicTaskFactory;
 import de.dennisguse.opentracks.services.tasks.PeriodicTaskExecutor;
@@ -62,7 +56,6 @@ import de.dennisguse.opentracks.util.SystemUtils;
 import de.dennisguse.opentracks.util.TrackIconUtils;
 import de.dennisguse.opentracks.util.TrackNameUtils;
 import de.dennisguse.opentracks.util.TrackPointUtils;
-import de.dennisguse.opentracks.util.UnitConversions;
 
 /**
  * A background service that registers a location listener and records track points.
@@ -70,25 +63,19 @@ import de.dennisguse.opentracks.util.UnitConversions;
  *
  * @author Leif Hendrik Wilden
  */
-public class TrackRecordingService extends Service {
+public class TrackRecordingService extends Service implements HandlerServer.HandlerServerInterface {
 
     private static final String TAG = TrackRecordingService.class.getSimpleName();
 
     // The following variables are set in onCreate:
-    @Deprecated //TODO Should not be necessary
-    private ExecutorService locationExecutorService; // Enforces order of location changes.
     private ContentProviderUtils contentProviderUtils;
-    private LocationManager locationManager;
     private PeriodicTaskExecutor voiceExecutor;
     private TrackRecordingServiceNotificationManager notificationManager;
-    private LocationListenerPolicy locationListenerPolicy;
 
     private long recordingTrackId;
     private boolean recordingTrackPaused;
     private int recordingDistanceInterval;
     private int maxRecordingDistance;
-    private int recordingGpsAccuracy;
-    private long currentRecordingInterval;
 
     private final OnSharedPreferenceChangeListener sharedPreferenceChangeListener = new OnSharedPreferenceChangeListener() {
         @Override
@@ -110,27 +97,14 @@ public class TrackRecordingService extends Service {
             if (PreferencesUtils.isKey(context, R.string.voice_frequency_key, key)) {
                 voiceExecutor.setTaskFrequency(PreferencesUtils.getVoiceFrequency(context));
             }
-            if (PreferencesUtils.isKey(context, R.string.min_recording_interval_key, key)) {
-                int minRecordingInterval = PreferencesUtils.getMinRecordingInterval(context);
-                if (minRecordingInterval == PreferencesUtils.getMinRecordingIntervalAdaptBatteryLife(context)) {
-                    // Choose battery life over moving time accuracy.
-                    locationListenerPolicy = new AdaptiveLocationListenerPolicy(30 * UnitConversions.ONE_SECOND_MS, 5 * UnitConversions.ONE_MINUTE_MS, 5);
-                } else if (minRecordingInterval == PreferencesUtils.getMinRecordingIntervalAdaptAccuracy(context)) {
-                    // Get all the updates.
-                    locationListenerPolicy = new AdaptiveLocationListenerPolicy(UnitConversions.ONE_SECOND_MS, 30 * UnitConversions.ONE_SECOND_MS, 0);
-                } else {
-                    locationListenerPolicy = new AbsoluteLocationListenerPolicy(minRecordingInterval * UnitConversions.ONE_SECOND_MS);
-                }
-            }
             if (PreferencesUtils.isKey(context, R.string.recording_distance_interval_key, key)) {
                 recordingDistanceInterval = PreferencesUtils.getRecordingDistanceInterval(context);
             }
             if (PreferencesUtils.isKey(context, R.string.max_recording_distance_key, key)) {
                 maxRecordingDistance = PreferencesUtils.getMaxRecordingDistance(context);
             }
-            if (PreferencesUtils.isKey(context, R.string.recording_gps_accuracy_key, key)) {
-                recordingGpsAccuracy = PreferencesUtils.getRecordingGPSAccuracy(context);
-            }
+
+            handlerServer.onSharedPreferenceChanged(context, preferences, key);
         }
     };
 
@@ -143,38 +117,16 @@ public class TrackRecordingService extends Service {
     private boolean isIdle;
 
     private TrackRecordingServiceBinder binder = new TrackRecordingServiceBinder(this);
-    private final LocationListener locationListener = new LocationListener() {
 
-        @Override
-        public void onLocationChanged(final Location location) {
-            if (locationExecutorService == null || locationExecutorService.isShutdown() || locationExecutorService.isTerminated()) {
-                return;
-            }
-            locationExecutorService.submit(() -> onLocationChangedAsync(location));
-        }
-
-        @Override
-        public void onStatusChanged(String provider, int status, Bundle extras) {
-            Log.w(TAG, "LocationListener.onStatusChanged(): is not implemented.");
-        }
-
-        @Override
-        public void onProviderEnabled(String provider) {
-            Log.w(TAG, "LocationListener.onProviderEnabled(): is not implemented.");
-        }
-
-        @Override
-        public void onProviderDisabled(String provider) {
-            Log.w(TAG, "LocationListener.onProviderDisabled(): is not implemented.");
-        }
-    };
+    private HandlerServer handlerServer;
 
     @Override
     public void onCreate() {
         super.onCreate();
-        locationExecutorService = Executors.newSingleThreadExecutor();
+
+        handlerServer = new HandlerServer(this);
+
         contentProviderUtils = new ContentProviderUtils(this);
-        locationManager = (LocationManager) getSystemService(Context.LOCATION_SERVICE);
         voiceExecutor = new PeriodicTaskExecutor(this, new AnnouncementPeriodicTaskFactory());
 
         notificationManager = new TrackRecordingServiceNotificationManager(this);
@@ -200,6 +152,8 @@ public class TrackRecordingService extends Service {
 
     @Override
     public void onDestroy() {
+        handlerServer.stop(this);
+
         if (remoteSensorManager != null) {
             remoteSensorManager.stop();
             remoteSensorManager = null;
@@ -208,9 +162,6 @@ public class TrackRecordingService extends Service {
         // Reverse order from onCreate
         showNotification(false); //TODO Why?
 
-        unregisterLocationListener();
-        locationManager = null;
-
         PreferencesUtils.unregister(this, sharedPreferenceChangeListener);
 
         try {
@@ -227,8 +178,6 @@ public class TrackRecordingService extends Service {
         // This should be the next to last operation
         wakeLock = SystemUtils.releaseWakeLock(wakeLock);
 
-        // Shutdown the executorService last to avoid sending events to a dead executor.
-        locationExecutorService.shutdown();
         super.onDestroy();
     }
 
@@ -428,7 +377,7 @@ public class TrackRecordingService extends Service {
 
     private void startGps() {
         wakeLock = SystemUtils.acquireWakeLock(this, wakeLock);
-        registerLocationListener();
+        handlerServer.start(this);
         showNotification(true);
     }
 
@@ -498,6 +447,8 @@ public class TrackRecordingService extends Service {
         }
         lastTrackPoint = null;
 
+        handlerServer.stop(this);
+
         stopGps(trackStopped);
     }
 
@@ -509,7 +460,7 @@ public class TrackRecordingService extends Service {
     void stopGps(boolean shutdown) {
         if (!isRecording()) return;
 
-        unregisterLocationListener();
+        handlerServer.stop(this);
         showNotification(false);
         wakeLock = SystemUtils.releaseWakeLock(wakeLock);
         if (shutdown) {
@@ -548,46 +499,27 @@ public class TrackRecordingService extends Service {
         PreferencesUtils.setBoolean(this, R.string.recording_track_paused_key, recordingTrackPaused);
     }
 
-    void onLocationChangedAsync(Location location) {
+    @Override
+    public void newTrackPoint(TrackPoint trackPoint, int recordingGpsAccuracy) {
         if (!isRecording() || isPaused()) {
-            Log.w(TAG, "Ignore onLocationChangedAsync. Not recording or paused.");
+            Log.w(TAG, "Ignore newTrackPoint. Not recording or paused.");
             return;
         }
 
         Track track = contentProviderUtils.getTrack(recordingTrackId);
         if (track == null) {
-            Log.w(TAG, "Ignore onLocationChangedAsync. No track.");
+            Log.w(TAG, "Ignore newTrackPoint. No track.");
             return;
         }
 
-        if (!LocationUtils.isValidLocation(location)) {
-            Log.w(TAG, "Ignore onLocationChangedAsync. location is invalid.");
-            return;
-        }
-
-        TrackPoint trackPoint = new TrackPoint(location);
         fillWithSensorDataSet(trackPoint);
 
         notificationManager.updateTrackPoint(this, trackPoint, recordingGpsAccuracy);
 
-        if (!TrackPointUtils.fulfillsAccuracy(trackPoint, recordingGpsAccuracy)) {
-            Log.d(TAG, "Ignore onLocationChangedAsync. Poor accuracy.");
-            return;
-        }
-
         TrackPointUtils.fixTime(trackPoint);
 
         //TODO Figure out how to avoid loading the lastValidTrackPoint from the database
         TrackPoint lastValidTrackPoint = getLastValidTrackPointInCurrentSegment(track.getId());
-        long idleTime = 0L;
-        if (TrackPointUtils.after(trackPoint, lastValidTrackPoint)) {
-            idleTime = trackPoint.getTime() - lastValidTrackPoint.getTime();
-        }
-
-        locationListenerPolicy.updateIdleTime(idleTime);
-        if (currentRecordingInterval != locationListenerPolicy.getDesiredPollingInterval()) {
-            registerLocationListener();
-        }
 
         //Storing trackPoint
 
@@ -598,7 +530,7 @@ public class TrackRecordingService extends Service {
             return;
         }
 
-        if (!LocationUtils.isValidLocation(lastValidTrackPoint.getLocation())) {
+        if (lastValidTrackPoint == null || !LocationUtils.isValidLocation(lastValidTrackPoint.getLocation())) {
             // For some reason the previous first trackPoint was not stored, but currentSegmentHasLocation set true.
             // Should not happen. The current segment should have a location. Just insert the current location.
             insertTrackPoint(track, trackPoint);
@@ -714,28 +646,6 @@ public class TrackRecordingService extends Service {
         }
     }
 
-    private void registerLocationListener() {
-        if (locationManager == null) {
-            Log.e(TAG, "locationManager is null.");
-            return;
-        }
-        try {
-            long interval = locationListenerPolicy.getDesiredPollingInterval();
-            locationManager.requestLocationUpdates(LocationManager.GPS_PROVIDER, interval, locationListenerPolicy.getMinDistance_m(), locationListener);
-            currentRecordingInterval = interval;
-        } catch (SecurityException e) {
-            Log.e(TAG, "Could not register location listener; permissions not granted.", e);
-        }
-    }
-
-    private void unregisterLocationListener() {
-        if (locationManager == null) {
-            Log.e(TAG, "locationManager is null.");
-            return;
-        }
-        locationManager.removeUpdates(locationListener);
-    }
-
     private void showNotification(boolean isGpsStarted) {
         if (isRecording()) {
             Intent intent = IntentUtils.newIntent(this, TrackRecordingActivity.class)
@@ -765,19 +675,6 @@ public class TrackRecordingService extends Service {
         }
     }
 
-    /**
-     * Disables processing of location updates from {@link android.location.LocationManager}.
-     */
-    @VisibleForTesting
-    public void enableLocationExecutor(boolean enable) {
-        if (enable) {
-            locationExecutorService = Executors.newSingleThreadExecutor();
-        } else {
-            locationExecutorService.shutdownNow();
-            locationExecutorService = null;
-        }
-    }
-
     @VisibleForTesting
     public void setRemoteSensorManager(BluetoothRemoteSensorManager remoteSensorManager) {
         this.remoteSensorManager = remoteSensorManager;

src/main/java/de/dennisguse/opentracks/services/TrackRecordingServiceBinder.java

@@ -1,9 +1,8 @@
 package de.dennisguse.opentracks.services;
 
-import android.location.Location;
-
 import androidx.annotation.VisibleForTesting;
 
+import de.dennisguse.opentracks.content.data.TrackPoint;
 import de.dennisguse.opentracks.content.sensor.SensorDataSet;
 import de.dennisguse.opentracks.services.sensors.BluetoothRemoteSensorManager;
 
@@ -82,23 +81,11 @@ class TrackRecordingServiceBinder extends android.os.Binder implements TrackReco
         return trackRecordingService.insertWaypoint(name, category, description, photoUrl);
     }
 
-    @VisibleForTesting
-    @Override
-    public void insertLocation(Location location) {
-        trackRecordingService.onLocationChangedAsync(location);
-    }
-
     @Override
     public SensorDataSet getSensorData() {
         return trackRecordingService.getSensorDataSet();
     }
 
-    @VisibleForTesting
-    @Override
-    public void enableLocationExecutor(boolean enable) {
-        trackRecordingService.enableLocationExecutor(enable);
-    }
-
     @VisibleForTesting
     @Override
     public void setRemoteSensorManager(BluetoothRemoteSensorManager remoteSensorManager) {
@@ -112,4 +99,10 @@ class TrackRecordingServiceBinder extends android.os.Binder implements TrackReco
     void detachFromService() {
         trackRecordingService = null;
     }
+
+    @VisibleForTesting
+    @Override
+    public void newTrackPoint(TrackPoint trackPoint, int recordingGpsAccuracy) {
+        trackRecordingService.newTrackPoint(trackPoint, recordingGpsAccuracy);
+    }
 }

src/main/java/de/dennisguse/opentracks/services/TrackRecordingServiceInterface.java

@@ -15,10 +15,9 @@
  */
 package de.dennisguse.opentracks.services;
 
-import android.location.Location;
-
 import androidx.annotation.VisibleForTesting;
 
+import de.dennisguse.opentracks.content.data.TrackPoint;
 import de.dennisguse.opentracks.content.sensor.SensorDataSet;
 import de.dennisguse.opentracks.services.sensors.BluetoothRemoteSensorManager;
 
@@ -99,23 +98,16 @@ public interface TrackRecordingServiceInterface {
      */
     SensorDataSet getSensorData();
 
-    /**
-     * Inserts a location in the current recording track.
-     * <p>
-     * When recording a track, GPS locations are automatically inserted.
-     * This is used for inserting special track points or for testing.
-     *
-     * @param location the location to be inserted
-     */
-    @VisibleForTesting
-    void insertLocation(Location location);
-
-    /**
-     * Disables processing of location updates from {@link android.location.LocationManager}.
-     */
-    @VisibleForTesting
-    void enableLocationExecutor(boolean enable);
-
     @VisibleForTesting
     void setRemoteSensorManager(BluetoothRemoteSensorManager remoteSensorManager);
+
+    /**
+     * Inserts a track point in the current recording track.
+     * This is used for inserting special track points or for testing.
+     *
+     * @param trackPoint           the track point object to be inserted.
+     * @param recordingGpsAccuracy recording GPS accuracy.
+     */
+    @VisibleForTesting
+    void newTrackPoint(TrackPoint trackPoint, int recordingGpsAccuracy);
 }

src/main/java/de/dennisguse/opentracks/services/handlers/AbsoluteLocationListenerPolicy.java

@@ -14,7 +14,7 @@
  * the License.
  */
 
-package de.dennisguse.opentracks.services;
+package de.dennisguse.opentracks.services.handlers;
 
 /**
  * This is a simple location listener policy that will always dictate the same polling interval.

src/main/java/de/dennisguse/opentracks/services/handlers/AdaptiveLocationListenerPolicy.java

@@ -14,7 +14,7 @@
  * the License.
  */
 
-package de.dennisguse.opentracks.services;
+package de.dennisguse.opentracks.services.handlers;
 
 /**
  * A {@link LocationListenerPolicy} that will change based on how long the user has been stationary.

src/main/java/de/dennisguse/opentracks/services/handlers/HandlerServer.java

@@ -0,0 +1,47 @@
+package de.dennisguse.opentracks.services.handlers;
+
+import android.content.Context;
+import android.content.SharedPreferences;
+
+import de.dennisguse.opentracks.content.data.TrackPoint;
+
+public class HandlerServer {
+    private String TAG = HandlerServer.class.getSimpleName();
+
+    private LocationHandler locationHandler;
+    private HandlerServerInterface service;
+
+    public HandlerServer(HandlerServerInterface service) {
+        this.locationHandler = new LocationHandler(this);
+        this.service = service;
+    }
+
+    public void start(Context context) {
+        locationHandler.onStart(context);
+        locationHandler.onSharedPreferenceChanged(context, null, null);
+    }
+
+    public void stop(Context context) {
+        locationHandler.onStop(context);
+    }
+
+    public void onSharedPreferenceChanged(Context context, SharedPreferences preferences, String key) {
+        locationHandler.onSharedPreferenceChanged(context, preferences, key);
+    }
+
+    public void sendTrackPoint(TrackPoint trackPoint, int recordingGpsAccuracy) {
+        service.newTrackPoint(trackPoint, recordingGpsAccuracy);
+    }
+
+    public interface HandlerServerInterface {
+        void newTrackPoint(TrackPoint trackPoint, int gpsAccuracy);
+    }
+
+    public interface Handler {
+        void onStart(Context context);
+
+        void onStop(Context context);
+
+        void onSharedPreferenceChanged(Context context, SharedPreferences preferences, String key);
+    }
+}

src/main/java/de/dennisguse/opentracks/services/handlers/LocationHandler.java

@@ -0,0 +1,143 @@
+package de.dennisguse.opentracks.services.handlers;
+
+import android.content.Context;
+import android.content.SharedPreferences;
+import android.location.Location;
+import android.location.LocationListener;
+import android.location.LocationManager;
+import android.os.Bundle;
+import android.util.Log;
+
+import androidx.annotation.NonNull;
+
+import de.dennisguse.opentracks.R;
+import de.dennisguse.opentracks.content.data.TrackPoint;
+import de.dennisguse.opentracks.util.LocationUtils;
+import de.dennisguse.opentracks.util.PreferencesUtils;
+import de.dennisguse.opentracks.util.TrackPointUtils;
+import de.dennisguse.opentracks.util.UnitConversions;
+
+class LocationHandler implements HandlerServer.Handler, LocationListener {
+
+    private String TAG = LocationHandler.class.getSimpleName();
+
+    private LocationManager locationManager;
+    private HandlerServer handlerServer;
+    private LocationListenerPolicy locationListenerPolicy;
+    private long currentRecordingInterval;
+    private int recordingGpsAccuracy;
+    private TrackPoint lastValidTrackPoint;
+
+    public LocationHandler(HandlerServer handlerServer) {
+        this.handlerServer = handlerServer;
+    }
+
+    @Override
+    public void onStart(Context context) {
+        locationManager = (LocationManager) context.getSystemService(Context.LOCATION_SERVICE);
+        registerLocationListener();
+    }
+
+    @Override
+    public void onStop(Context context) {
+        locationManager = null;
+        unregisterLocationListener();
+    }
+
+    @Override
+    public void onSharedPreferenceChanged(Context context, SharedPreferences preferences, String key) {
+        if (PreferencesUtils.isKey(context, R.string.min_recording_interval_key, key)) {
+            int minRecordingInterval = PreferencesUtils.getMinRecordingInterval(context);
+            if (minRecordingInterval == PreferencesUtils.getMinRecordingIntervalAdaptBatteryLife(context)) {
+                // Choose battery life over moving time accuracy.
+                locationListenerPolicy = new AdaptiveLocationListenerPolicy(30 * UnitConversions.ONE_SECOND_MS, 5 * UnitConversions.ONE_MINUTE_MS, 5);
+            } else if (minRecordingInterval == PreferencesUtils.getMinRecordingIntervalAdaptAccuracy(context)) {
+                // Get all the updates.
+                locationListenerPolicy = new AdaptiveLocationListenerPolicy(UnitConversions.ONE_SECOND_MS, 30 * UnitConversions.ONE_SECOND_MS, 0);
+            } else {
+                locationListenerPolicy = new AbsoluteLocationListenerPolicy(minRecordingInterval * UnitConversions.ONE_SECOND_MS);
+            }
+
+            if (locationManager != null) {
+                registerLocationListener();
+            }
+        }
+        if (PreferencesUtils.isKey(context, R.string.recording_gps_accuracy_key, key)) {
+            recordingGpsAccuracy = PreferencesUtils.getRecordingGPSAccuracy(context);
+        }
+    }
+
+    @Override
+    public void onLocationChanged(@NonNull Location location) {
+        // TODO do we still need to process the location processing in an asynchronous manner? Let's go to check it out.
+        computeLocation(location);
+    }
+
+    @Override
+    public void onStatusChanged(String provider, int status, Bundle extras) {
+
+    }
+
+    @Override
+    public void onProviderEnabled(@NonNull String provider) {
+    }
+
+    @Override
+    public void onProviderDisabled(@NonNull String provider) {
+    }
+
+    /**
+     * Checks if location is valid and builds a track point that will be send through HandlerServer.
+     *
+     * @param location {@link Location} object.
+     */
+    private void computeLocation(Location location) {
+        if (!LocationUtils.isValidLocation(location)) {
+            Log.w(TAG, "Ignore newTrackPoint. location is invalid.");
+            return;
+        }
+
+        TrackPoint trackPoint = new TrackPoint(location);
+
+        if (!TrackPointUtils.fulfillsAccuracy(trackPoint, recordingGpsAccuracy)) {
+            Log.d(TAG, "Ignore newTrackPoint. Poor accuracy.");
+            return;
+        }
+
+        long idleTime = 0L;
+        if (TrackPointUtils.after(trackPoint, lastValidTrackPoint)) {
+            idleTime = trackPoint.getTime() - lastValidTrackPoint.getTime();
+        }
+
+        locationListenerPolicy.updateIdleTime(idleTime);
+        if (currentRecordingInterval != locationListenerPolicy.getDesiredPollingInterval()) {
+            registerLocationListener();
+        }
+
+        lastValidTrackPoint = trackPoint;
+        handlerServer.sendTrackPoint(trackPoint, recordingGpsAccuracy);
+    }
+
+    private void registerLocationListener() {
+        if (locationManager == null) {
+            Log.e(TAG, "locationManager is null.");
+            return;
+        }
+        try {
+            long interval = locationListenerPolicy.getDesiredPollingInterval();
+            currentRecordingInterval = interval;
+            locationManager.requestLocationUpdates(LocationManager.GPS_PROVIDER, interval, locationListenerPolicy.getMinDistance_m(), this);
+        } catch (SecurityException e) {
+            Log.e(TAG, "Could not register location listener; permissions not granted.", e);
+        }
+    }
+
+    private void unregisterLocationListener() {
+        if (locationManager == null) {
+            Log.e(TAG, "locationManager is null.");
+            return;
+        }
+        locationManager.removeUpdates(this);
+        locationManager = null;
+    }
+}

src/main/java/de/dennisguse/opentracks/services/handlers/LocationListenerPolicy.java

@@ -14,7 +14,7 @@
  * the License.
  */
 
-package de.dennisguse.opentracks.services;
+package de.dennisguse.opentracks.services.handlers;
 
 /**
  * This is an interface for classes that will manage the location listener policy.