Merge pull request #1797 from OpenTracksApp/refactoring#1708

Refactoring TrackRecordingServiceConnection

build.gradle

@@ -133,6 +133,7 @@ dependencies {
     implementation 'androidx.gridlayout:gridlayout:1.0.0'
     implementation 'com.google.android.material:material:1.11.0'
     implementation 'androidx.constraintlayout:constraintlayout:2.1.4'
+    implementation 'androidx.core:core:1.12.0'
     implementation 'androidx.core:core-splashscreen:1.0.1'
     implementation 'androidx.mediarouter:mediarouter:1.6.0'
 

src/main/java/de/dennisguse/opentracks/TrackEditActivity.java

@@ -28,7 +28,6 @@ import de.dennisguse.opentracks.data.models.ActivityType;
 import de.dennisguse.opentracks.data.models.Track;
 import de.dennisguse.opentracks.databinding.TrackEditBinding;
 import de.dennisguse.opentracks.fragments.ChooseActivityTypeDialogFragment;
-import de.dennisguse.opentracks.services.TrackRecordingServiceConnection;
 import de.dennisguse.opentracks.util.TrackUtils;
 
 /**
@@ -44,7 +43,6 @@ public class TrackEditActivity extends AbstractActivity implements ChooseActivit
 
     private static final String ICON_VALUE_KEY = "icon_value_key";
 
-    private TrackRecordingServiceConnection trackRecordingServiceConnection;
     private ContentProviderUtils contentProviderUtils;
     private Track track;
     private ActivityType activityType;
@@ -55,7 +53,6 @@ public class TrackEditActivity extends AbstractActivity implements ChooseActivit
     protected void onCreate(Bundle bundle) {
         super.onCreate(bundle);
 
-        trackRecordingServiceConnection = new TrackRecordingServiceConnection();
         Track.Id trackId = getIntent().getParcelableExtra(EXTRA_TRACK_ID);
         if (trackId == null) {
             Log.e(TAG, "invalid trackId");
@@ -114,18 +111,6 @@ public class TrackEditActivity extends AbstractActivity implements ChooseActivit
         setSupportActionBar(viewBinding.bottomAppBarLayout.bottomAppBar);
     }
 
-    @Override
-    protected void onStart() {
-        super.onStart();
-        trackRecordingServiceConnection.startConnection(this);
-    }
-
-    @Override
-    protected void onStop() {
-        super.onStop();
-        trackRecordingServiceConnection.unbind(this);
-    }
-
     @Override
     public void onSaveInstanceState(@NonNull Bundle outState) {
         super.onSaveInstanceState(outState);

src/main/java/de/dennisguse/opentracks/TrackListActivity.java

@@ -157,14 +157,9 @@ public class TrackListActivity extends AbstractTrackDeleteActivity implements Co
                 startActivity(new Intent(Settings.ACTION_LOCATION_SOURCE_SETTINGS));
             } else {
                 if (gpsStatusValue.isGpsStarted()) {
-                    recordingStatusConnection.unbindAndStop(this);
+                    recordingStatusConnection.stopService(this);
-                    recordingStatusConnection.startConnection(this); //TODO We need to stay listening!
                 } else {
-                    new TrackRecordingServiceConnection((service, connection) -> {
+                    TrackRecordingServiceConnection.execute(this, (service, connection) -> service.tryStartSensors());
-                        service.tryStartSensors();
-
-                        connection.unbind(this);
-                    }).startAndBindWithCallback(this);
                 }
             }
         });
@@ -183,15 +178,13 @@ public class TrackListActivity extends AbstractTrackDeleteActivity implements Co
             // Not Recording -> Recording
             Log.i(TAG, "Starting recording");
             updateGpsMenuItem(false, true);
-            new TrackRecordingServiceConnection((service, connection) -> {
+            TrackRecordingServiceConnection.execute(this, (service, connection) -> {
                 Track.Id trackId = service.startNewTrack();
 
                 Intent newIntent = IntentUtils.newIntent(TrackListActivity.this, TrackRecordingActivity.class);
                 newIntent.putExtra(TrackRecordingActivity.EXTRA_TRACK_ID, trackId);
                 startActivity(newIntent);
-
+            });
-                connection.unbind(this);
-            }).startAndBind(this, true);
         });
         viewBinding.trackListFabAction.setOnLongClickListener((view) -> {
             if (!recordingStatus.isRecording()) {
@@ -220,7 +213,7 @@ public class TrackListActivity extends AbstractTrackDeleteActivity implements Co
         super.onStart();
 
         PreferencesUtils.registerOnSharedPreferenceChangeListener(sharedPreferenceChangeListener);
-        recordingStatusConnection.startConnection(this);
+        recordingStatusConnection.bind(this);
     }
 
     @Override

src/main/java/de/dennisguse/opentracks/TrackRecordedActivity.java

@@ -201,7 +201,7 @@ public class TrackRecordedActivity extends AbstractTrackDeleteActivity implement
         }
 
         if (item.getItemId() == R.id.track_detail_resume_track) {
-            new TrackRecordingServiceConnection((service, connection) -> {
+            TrackRecordingServiceConnection.execute(this, (service, connection) -> {
                 service.resumeTrack(trackId);
 
                 Intent newIntent = IntentUtils.newIntent(TrackRecordedActivity.this, TrackRecordingActivity.class)
@@ -209,9 +209,8 @@ public class TrackRecordedActivity extends AbstractTrackDeleteActivity implement
                 startActivity(newIntent);
                 overridePendingTransition(android.R.anim.fade_in, android.R.anim.fade_out);
 
-                connection.unbind(this);
                 finish();
-            }).startAndBind(this, true);
+            });
             return true;
         }
 

src/main/java/de/dennisguse/opentracks/TrackRecordingActivity.java

@@ -207,7 +207,7 @@ public class TrackRecordingActivity extends AbstractActivity implements ChooseAc
 
         PreferencesUtils.registerOnSharedPreferenceChangeListener(sharedPreferenceChangeListener);
 
-        trackRecordingServiceConnection.startConnection(this);
+        trackRecordingServiceConnection.bind(this);
         trackDataHub.start();
     }
 
@@ -224,7 +224,7 @@ public class TrackRecordingActivity extends AbstractActivity implements ChooseAc
             trackDataHub.setRecordingStatus(recordingStatus);
         }
 
-        trackRecordingServiceConnection.startAndBindWithCallback(this);
+        trackRecordingServiceConnection.bind(this);
     }
 
     @Override

src/main/java/de/dennisguse/opentracks/TrackStoppedActivity.java

@@ -136,7 +136,7 @@ public class TrackStoppedActivity extends AbstractTrackDeleteActivity implements
     }
 
     private void resumeTrackAndFinish() {
-        new TrackRecordingServiceConnection((service, connection) -> {
+        TrackRecordingServiceConnection.execute(this, (service, connection) -> {
             service.resumeTrack(trackId);
 
             Intent newIntent = IntentUtils.newIntent(TrackStoppedActivity.this, TrackRecordingActivity.class)
@@ -144,9 +144,8 @@ public class TrackStoppedActivity extends AbstractTrackDeleteActivity implements
             startActivity(newIntent);
             overridePendingTransition(android.R.anim.fade_in, android.R.anim.fade_out);
 
-            connection.unbind(this);
             finish();
-        }).startAndBind(this, true);
+        });
     }
 
     @Override

src/main/java/de/dennisguse/opentracks/fragments/StatisticsRecordingFragment.java

@@ -88,7 +88,7 @@ public class StatisticsRecordingFragment extends Fragment {
 
         PreferencesUtils.registerOnSharedPreferenceChangeListener(sharedPreferenceChangeListener);
 
-        trackRecordingServiceConnection.startConnection(getContext());
+        trackRecordingServiceConnection.bind(getContext());
     }
 
     @Override

src/main/java/de/dennisguse/opentracks/publicapi/AbstractAPIActivity.java

@@ -37,8 +37,7 @@ public abstract class AbstractAPIActivity extends AppCompatActivity {
 
         if (PreferencesUtils.isPublicAPIenabled()) {
             Log.i(TAG, "Received and trying to execute requested action.");
-            new TrackRecordingServiceConnection(serviceConnectedCallback)
+            TrackRecordingServiceConnection.execute(this, serviceConnectedCallback);
-                    .startAndBind(this, isStartServiceForeground());
         } else {
             Toast.makeText(this, getString(R.string.settings_public_api_disabled_toast), Toast.LENGTH_LONG).show();
             Log.w(TAG, "Public API is disabled; ignoring request.");
@@ -46,10 +45,6 @@ public abstract class AbstractAPIActivity extends AppCompatActivity {
         }
     }
 
-    protected boolean isStartServiceForeground() {
-        return false;
-    }
-
     protected abstract void execute(TrackRecordingService service);
 
     protected abstract boolean isPostExecuteStopService();

src/main/java/de/dennisguse/opentracks/publicapi/StartRecording.java

@@ -62,9 +62,4 @@ public class StartRecording extends AbstractAPIActivity {
     protected boolean isPostExecuteStopService() {
         return false;
     }
-
-    @Override
-    protected boolean isStartServiceForeground() {
-        return true;
-    }
 }

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

@@ -30,6 +30,7 @@ import android.widget.Toast;
 
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
+import androidx.core.app.ServiceCompat;
 import androidx.lifecycle.LiveData;
 import androidx.lifecycle.MutableLiveData;
 
@@ -111,7 +112,7 @@ public class TrackRecordingService extends Service implements TrackPointCreator.
         recordingDataObservable = new MutableLiveData<>(NOT_RECORDING);
 
         trackPointCreator = new TrackPointCreator(this);
-        trackRecordingManager = new TrackRecordingManager(this, trackPointCreator, this , handler);
+        trackRecordingManager = new TrackRecordingManager(this, trackPointCreator, this, handler);
 
         voiceAnnouncementManager = new VoiceAnnouncementManager(this);
         notificationManager = new TrackRecordingServiceNotificationManager(this);
@@ -213,19 +214,15 @@ public class TrackRecordingService extends Service implements TrackPointCreator.
         Log.i(TAG, "startSensors");
         wakeLock = SystemUtils.acquireWakeLock(this, wakeLock);
         trackPointCreator.start(this, handler);
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
 
-            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) {
-                if (!PermissionRequester.RECORDING.hasPermission(this)) {
+            if (!PermissionRequester.RECORDING.hasPermission(this)) {
-                    Toast.makeText(this, R.string.permission_recording_failed, Toast.LENGTH_LONG).show();
+                Toast.makeText(this, R.string.permission_recording_failed, Toast.LENGTH_LONG).show();
-                    return;
+                return;
-                }
             }
-
-            startForeground(TrackRecordingServiceNotificationManager.NOTIFICATION_ID, notificationManager.setGPSonlyStarted(this), ServiceInfo.FOREGROUND_SERVICE_TYPE_LOCATION + ServiceInfo.FOREGROUND_SERVICE_TYPE_CONNECTED_DEVICE);
-        } else {
-            startForeground(TrackRecordingServiceNotificationManager.NOTIFICATION_ID, notificationManager.setGPSonlyStarted(this));
         }
+
+        ServiceCompat.startForeground(this, TrackRecordingServiceNotificationManager.NOTIFICATION_ID, notificationManager.setGPSonlyStarted(this), ServiceInfo.FOREGROUND_SERVICE_TYPE_LOCATION + ServiceInfo.FOREGROUND_SERVICE_TYPE_CONNECTED_DEVICE);
     }
 
     public void endCurrentTrack() {

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

@@ -42,7 +42,7 @@ import de.dennisguse.opentracks.data.models.Marker;
  *
  * @author Rodrigo Damazio
  */
-public class TrackRecordingServiceConnection implements ServiceConnection, DeathRecipient {
+public class TrackRecordingServiceConnection {
 
     private static final String TAG = TrackRecordingServiceConnection.class.getSimpleName();
 
@@ -50,9 +50,29 @@ public class TrackRecordingServiceConnection implements ServiceConnection, Death
 
     private TrackRecordingService trackRecordingService;
 
-    public TrackRecordingServiceConnection() {
+    private final ServiceConnection serviceConnection = new ServiceConnection() {
-        callback = null;
+        @Override
-    }
+        public void onServiceConnected(ComponentName className, IBinder service) {
+            Log.i(TAG, "Connected to the service: " + service);
+            try {
+                service.linkToDeath(deathRecipient, 0);
+            } catch (RemoteException e) {
+                Log.e(TAG, "Failed to bind a death recipient.", e);
+            }
+            setTrackRecordingService(((TrackRecordingService.Binder) service).getService());
+        }
+
+        @Override
+        public void onServiceDisconnected(ComponentName className) {
+            Log.i(TAG, "Disconnected from the service.");
+            setTrackRecordingService(null);
+        }
+    };
+
+    private final DeathRecipient deathRecipient = () -> {
+        Log.d(TAG, "Service died.");
+        setTrackRecordingService(null);
+    };
 
     public TrackRecordingServiceConnection(@NonNull Callback callback) {
         this.callback = callback;
@@ -60,58 +80,13 @@ public class TrackRecordingServiceConnection implements ServiceConnection, Death
 
     public void bind(@NonNull Context context) {
         if (trackRecordingService != null) {
-            return;
-        }
-        context.bindService(new Intent(context, TrackRecordingService.class), this, 0);
-    }
-
-    /**
-     * Starts and binds the service.
-     *
-     * @param foreground is the service expected to call `startForeground()`?
-     */
-    public void startAndBind(Context context, boolean foreground) {
-        if (trackRecordingService != null) {
-            // Service is already started and bound.
-            return;
-        }
-
-        Log.i(TAG, "Starting the service.");
-        if (foreground) {
-            ContextCompat.startForegroundService(context, new Intent(context, TrackRecordingService.class));
-        } else {
-            context.startService(new Intent(context, TrackRecordingService.class));
-        }
-
-        startConnection(context);
-    }
-
-    //TODO There should be a better way to implement this.
-
-    /**
-     * Triggers the onConnected() callback even if already connected.
-     */
-    //TODO Check if this is actually needed as it is used to re-connect from Activities in onResume by using a LiveData; might be obsolete. If not, there should be a better way to implement this.
-    @Deprecated
-    public void startAndBindWithCallback(Context context) {
-        if (trackRecordingService == null) {
-            startAndBind(context, false);
-            return;
-        }
-        if (callback != null) {
             callback.onConnected(trackRecordingService, this);
-        }
-    }
-
-    public void startConnection(@NonNull Context context) {
-        if (trackRecordingService != null) {
-            // Service is already started and bound.
             return;
         }
 
         Log.i(TAG, "Binding the service.");
         int flags = BuildConfig.DEBUG ? Context.BIND_DEBUG_UNBIND : 0;
-        context.bindService(new Intent(context, TrackRecordingService.class), this, flags);
+        context.bindService(new Intent(context, TrackRecordingService.class), serviceConnection, flags);
     }
 
     /**
@@ -120,72 +95,45 @@ public class TrackRecordingServiceConnection implements ServiceConnection, Death
     //TODO This is often called for one-shot operations and should be refactored as unbinding is required.
     public void unbind(Context context) {
         try {
-            context.unbindService(this);
+            context.unbindService(serviceConnection);
         } catch (IllegalArgumentException e) {
             // Means not bound to the service. OK to ignore.
         }
         setTrackRecordingService(null);
     }
 
-    public void unbindAndStop(Context context) {
+    public void stopService(Context context) {
-        unbind(context);
         context.stopService(new Intent(context, TrackRecordingService.class));
     }
 
-    @Nullable
+    public void unbindAndStop(Context context) {
-    public TrackRecordingService getServiceIfBound() {
+        unbind(context);
-        return trackRecordingService;
+        stopService(context);
     }
 
     private void setTrackRecordingService(TrackRecordingService value) {
         trackRecordingService = value;
-        if (callback != null) {
+        if (value != null) {
-            if (value != null) {
+            callback.onConnected(value, this);
-                callback.onConnected(value, this);
-            } else {
-                callback.onDisconnected();
-            }
         }
     }
 
-    @Override
+    //TODO Move to some other place; not needed here.
-    public void onServiceConnected(ComponentName className, IBinder service) {
-        Log.i(TAG, "Connected to the service: " + service);
-        try {
-            service.linkToDeath(this, 0);
-        } catch (RemoteException e) {
-            Log.e(TAG, "Failed to bind a death recipient.", e);
-        }
-        setTrackRecordingService(((TrackRecordingService.Binder) service).getService());
-    }
-
-    @Override
-    public void onServiceDisconnected(ComponentName className) {
-        Log.i(TAG, "Disconnected from the service.");
-        setTrackRecordingService(null);
-    }
-
-    @Override
-    public void binderDied() {
-        Log.d(TAG, "Service died.");
-        setTrackRecordingService(null);
-    }
-
     @Nullable
     public Marker.Id addMarker(Context context, String name, String category, String description, String photoUrl) {
-        TrackRecordingService trackRecordingService = getServiceIfBound();
         if (trackRecordingService == null) {
             Log.d(TAG, "Unable to add marker, no track recording service");
-        } else {
+            return null;
-            try {
+        }
-                Marker.Id marker = trackRecordingService.insertMarker(name, category, description, photoUrl);
+
-                if (marker != null) {
+        try {
-                    Toast.makeText(context, R.string.marker_add_success, Toast.LENGTH_SHORT).show();
+            Marker.Id marker = trackRecordingService.insertMarker(name, category, description, photoUrl);
-                    return marker;
+            if (marker != null) {
-                }
+                Toast.makeText(context, R.string.marker_add_success, Toast.LENGTH_SHORT).show();
-            } catch (IllegalStateException e) {
+                return marker;
-                Log.e(TAG, "Unable to add marker.", e);
             }
+        } catch (IllegalStateException e) {
+            Log.e(TAG, "Unable to add marker.", e);
         }
 
         Toast.makeText(context, R.string.marker_add_error, Toast.LENGTH_LONG).show();
@@ -193,7 +141,6 @@ public class TrackRecordingServiceConnection implements ServiceConnection, Death
     }
 
     public void stopRecording(@NonNull Context context) {
-        TrackRecordingService trackRecordingService = getServiceIfBound();
         if (trackRecordingService == null) {
             Log.e(TAG, "TrackRecordingService not connected.");
         } else {
@@ -203,9 +150,17 @@ public class TrackRecordingServiceConnection implements ServiceConnection, Death
     }
 
     public interface Callback {
-        void onConnected(TrackRecordingService service, TrackRecordingServiceConnection connection);
+        void onConnected(TrackRecordingService service, TrackRecordingServiceConnection self);
+    }
 
-        default void onDisconnected() {
+    public static void execute(Context context, Callback callback) {
-        }
+        Callback withUnbind = (service, connection) -> {
+            callback.onConnected(service, connection);
+            connection.unbind(context);
+        };
+        new TrackRecordingServiceConnection(withUnbind)
+                .bind(context);
+
+        ContextCompat.startForegroundService(context, new Intent(context, TrackRecordingService.class));
     }
 }

src/main/java/de/dennisguse/opentracks/ui/markers/MarkerEditViewModel.java

@@ -32,7 +32,7 @@ public class MarkerEditViewModel extends AndroidViewModel {
     private MutableLiveData<Marker> markerData;
     private boolean isNewMarker;
     private Uri photoOriginalUri;
-    private final TrackRecordingServiceConnection trackRecordingServiceConnection = new TrackRecordingServiceConnection();
+    private final TrackRecordingServiceConnection trackRecordingServiceConnection = new TrackRecordingServiceConnection((service, connection) -> {});
 
     public MarkerEditViewModel(@NonNull Application application) {
         super(application);
@@ -41,7 +41,7 @@ public class MarkerEditViewModel extends AndroidViewModel {
     public LiveData<Marker> getMarkerData(@NonNull Track.Id trackId, @Nullable Marker.Id markerId) {
         if (markerData == null) {
             markerData = new MutableLiveData<>();
-            trackRecordingServiceConnection.startConnection(getApplication());
+            trackRecordingServiceConnection.bind(getApplication());
             loadData(trackId, markerId);
         }
         return markerData;

src/main/java/de/dennisguse/opentracks/ui/markers/MarkerListActivity.java

@@ -119,7 +119,7 @@ public class MarkerListActivity extends AbstractActivity {
     @Override
     protected void onStart() {
         super.onStart();
-        trackRecordingServiceConnection.startConnection(this);
+        trackRecordingServiceConnection.bind(this);
     }
 
     @Override