Review suggestions

MyTracks/src/com/google/android/apps/mytracks/ChartActivity.java

@@ -107,7 +107,6 @@ public class ChartActivity extends Activity implements TrackDataListener {
   protected void onCreate(Bundle savedInstanceState) {
     Log.w(TAG, "ChartActivity.onCreate");
     super.onCreate(savedInstanceState);
-    dataHub = TrackDataHub.getInstance(this);
 
     // The volume we want to control is the Text-To-Speech volume
     int volumeStream =
@@ -143,6 +142,7 @@ public class ChartActivity extends Activity implements TrackDataListener {
   protected void onResume() {
     super.onResume();
 
+    dataHub = TrackDataHub.getStartedInstance();
     dataHub.registerTrackDataListener(this, EnumSet.of(
         ListenerDataType.SELECTED_TRACK_CHANGED,
         ListenerDataType.POINT_UPDATES,
@@ -154,6 +154,7 @@ public class ChartActivity extends Activity implements TrackDataListener {
   @Override
   protected void onPause() {
     dataHub.unregisterTrackDataListener(this);
+    dataHub = null;
 
     super.onPause();
   }
@@ -444,7 +445,7 @@ public class ChartActivity extends Activity implements TrackDataListener {
     this.metricUnits = metric;
 
     chartView.setMetricUnits(metric);
-    
+
     return true;  // Reload data
   }
 

MyTracks/src/com/google/android/apps/mytracks/MapActivity.java

@@ -1,12 +1,12 @@
 /*
  * Copyright 2008 Google Inc.
- * 
+ *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not
  * use this file except in compliance with the License. You may obtain a copy of
  * the License at
- * 
+ *
  * http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
  * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
@@ -132,8 +132,6 @@ public class MapActivity extends com.google.android.maps.MapActivity
         new StatusAnnouncerFactory(ApiFeatures.getInstance()).getVolumeStream();
     setVolumeControlStream(volumeStream);
 
-    dataHub = TrackDataHub.getInstance(this);
-
     // We don't need a window title bar:
     requestWindowFeature(Window.FEATURE_NO_TITLE);
 
@@ -180,9 +178,10 @@ public class MapActivity extends com.google.android.maps.MapActivity
 
   @Override
   protected void onResume() {
-    Log.d(TAG, "MapActivity.onStart");
+    Log.d(TAG, "MapActivity.onResume");
     super.onResume();
 
+    dataHub = TrackDataHub.getStartedInstance();
     dataHub.registerTrackDataListener(this, EnumSet.of(
         ListenerDataType.SELECTED_TRACK_CHANGED,
         ListenerDataType.POINT_UPDATES,
@@ -203,9 +202,10 @@ public class MapActivity extends com.google.android.maps.MapActivity
 
   @Override
   protected void onPause() {
-    Log.d(TAG, "MapActivity.onStop");
+    Log.d(TAG, "MapActivity.onPause");
 
     dataHub.unregisterTrackDataListener(this);
+    dataHub = null;
 
     super.onPause();
   }
@@ -338,9 +338,9 @@ public class MapActivity extends com.google.android.maps.MapActivity
 
         if (trackSelected) {
           busyPane.setVisibility(View.VISIBLE);
-  
+
           zoomMapToBoundaries(track);
-  
+
           mapOverlay.setShowEndMarker(!isRecording);
           busyPane.setVisibility(View.GONE);
         }

MyTracks/src/com/google/android/apps/mytracks/MyTracks.java

@@ -23,7 +23,7 @@ import com.google.android.apps.mytracks.content.TracksColumns;
 import com.google.android.apps.mytracks.content.WaypointCreationRequest;
 import com.google.android.apps.mytracks.io.file.TempFileCleaner;
 import com.google.android.apps.mytracks.services.ITrackRecordingService;
-import com.google.android.apps.mytracks.services.ServiceStateHelper;
+import com.google.android.apps.mytracks.services.ServiceUtils;
 import com.google.android.apps.mytracks.services.TrackRecordingServiceBinder;
 import com.google.android.apps.mytracks.services.tasks.StatusAnnouncerFactory;
 import com.google.android.apps.mytracks.util.ApiFeatures;
@@ -127,7 +127,7 @@ public class MyTracks extends TabActivity implements OnTouchListener {
 
     providerUtils = MyTracksProviderUtils.Factory.get(this);
     preferences = getSharedPreferences(Constants.SETTINGS_NAME, 0);
-    dataHub = TrackDataHub.getInstance(this);
+    dataHub = TrackDataHub.newInstance(this);
     menuManager = new MenuManager(this);
     serviceBinder = TrackRecordingServiceBinder.getInstance(this);
 
@@ -209,7 +209,7 @@ public class MyTracks extends TabActivity implements OnTouchListener {
     dataHub.start();
 
     // Ensure that service is running if we're supposed to be recording
-    if (ServiceStateHelper.isRecording(this, preferences)) {
+    if (ServiceUtils.isRecording(this, preferences)) {
       serviceBinder.startService();
     }
 
@@ -238,7 +238,7 @@ public class MyTracks extends TabActivity implements OnTouchListener {
   @Override
   public boolean onPrepareOptionsMenu(Menu menu) {
     menuManager.onPrepareOptionsMenu(menu, providerUtils.getLastTrack() != null,
-        ServiceStateHelper.isRecording(this, preferences),
+        ServiceUtils.isRecording(this, preferences),
         dataHub.isATrackSelected());
     return super.onPrepareOptionsMenu(menu);
   }
@@ -257,7 +257,7 @@ public class MyTracks extends TabActivity implements OnTouchListener {
 
   @Override
   public boolean onTrackballEvent(MotionEvent event) {
-    if (ServiceStateHelper.isRecording(this, preferences)) {
+    if (ServiceUtils.isRecording(this, preferences)) {
       if (event.getAction() == MotionEvent.ACTION_DOWN) {
         try {
           insertWaypoint(WaypointCreationRequest.DEFAULT_STATISTICS);
@@ -475,8 +475,4 @@ public class MyTracks extends TabActivity implements OnTouchListener {
   long getSelectedTrackId() {
     return dataHub.getSelectedTrackId();
   }
-
-  public TrackDataHub getDataHub() {
-    return dataHub;
-  }
 }

MyTracks/src/com/google/android/apps/mytracks/StatsActivity.java

@@ -22,7 +22,7 @@ import com.google.android.apps.mytracks.content.TrackDataHub;
 import com.google.android.apps.mytracks.content.TrackDataHub.ListenerDataType;
 import com.google.android.apps.mytracks.content.TrackDataListener;
 import com.google.android.apps.mytracks.content.Waypoint;
-import com.google.android.apps.mytracks.services.ServiceStateHelper;
+import com.google.android.apps.mytracks.services.ServiceUtils;
 import com.google.android.apps.mytracks.services.tasks.StatusAnnouncerFactory;
 import com.google.android.apps.mytracks.util.ApiFeatures;
 import com.google.android.maps.mytracks.R;
@@ -90,7 +90,7 @@ public class StatsActivity extends Activity implements TrackDataListener {
     @Override
     public void run() {
       Log.i(TAG, "Started UI update thread");
-      while (ServiceStateHelper.isRecording(StatsActivity.this, preferences)) {
+      while (ServiceUtils.isRecording(StatsActivity.this, preferences)) {
         runOnUiThread(updateResults);
         try {
           Thread.sleep(1000L);
@@ -109,7 +109,6 @@ public class StatsActivity extends Activity implements TrackDataListener {
     super.onCreate(savedInstanceState);
 
     preferences = getSharedPreferences(Constants.SETTINGS_NAME, 0);
-    dataHub = TrackDataHub.getInstance(this);
     utils = new StatsUtilities(this);
 
     // The volume we want to control is the Text-To-Speech volume
@@ -136,18 +135,20 @@ public class StatsActivity extends Activity implements TrackDataListener {
 
   @Override
   protected void onResume() {
+    super.onResume();
+
+    dataHub = TrackDataHub.getStartedInstance();
     dataHub.registerTrackDataListener(this, EnumSet.of(
         ListenerDataType.SELECTED_TRACK_CHANGED,
         ListenerDataType.TRACK_UPDATES,
         ListenerDataType.LOCATION_UPDATES,
         ListenerDataType.DISPLAY_PREFERENCES));
-
-    super.onResume();
   }
 
   @Override
   protected void onPause() {
     dataHub.unregisterTrackDataListener(this);
+    dataHub = null;
 
     if (thread != null) {
       thread.interrupt();
@@ -175,7 +176,7 @@ public class StatsActivity extends Activity implements TrackDataListener {
 
     utils.setReportSpeed(displaySpeed);
     updateLabels();
-  
+
     return true;  // Reload data
   }
 

MyTracks/src/com/google/android/apps/mytracks/TrackList.java

@@ -18,7 +18,7 @@ package com.google.android.apps.mytracks;
 import com.google.android.apps.mytracks.content.TracksColumns;
 import com.google.android.apps.mytracks.io.file.SaveActivity;
 import com.google.android.apps.mytracks.io.sendtogoogle.SendActivity;
-import com.google.android.apps.mytracks.services.ServiceStateHelper;
+import com.google.android.apps.mytracks.services.ServiceUtils;
 import com.google.android.apps.mytracks.util.StringUtils;
 import com.google.android.apps.mytracks.util.UnitConversions;
 import com.google.android.maps.mytracks.R;
@@ -79,7 +79,7 @@ public class TrackList extends ListActivity
               R.string.tracklist_show_track);
           menu.add(0, Constants.MENU_EDIT, 0,
               R.string.tracklist_edit_track);
-          if (!ServiceStateHelper.isRecording(TrackList.this, null)
+          if (!ServiceUtils.isRecording(TrackList.this, null)
               || trackId != recordingTrackId) {
             menu.add(0, Constants.MENU_SEND_TO_GOOGLE, 0,
                 R.string.tracklist_send_to_google);
@@ -219,7 +219,7 @@ public class TrackList extends ListActivity
 
     View deleteAll = findViewById(R.id.tracklist_btn_delete_all);
     View exportAll = findViewById(R.id.tracklist_btn_export_all);
-    boolean notRecording = !ServiceStateHelper.isRecording(this, preferences);
+    boolean notRecording = !ServiceUtils.isRecording(this, preferences);
     deleteAll.setOnClickListener(this);
     deleteAll.setEnabled(notRecording);
     exportAll.setOnClickListener(this);

MyTracks/src/com/google/android/apps/mytracks/content/TrackDataHub.java

@@ -1,12 +1,12 @@
 /*
  * Copyright 2011 Google Inc.
- * 
+ *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not
  * use this file except in compliance with the License. You may obtain a copy of
  * the License at
- * 
+ *
  * http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
  * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
@@ -166,9 +166,6 @@ public class TrackDataHub {
   /** Condensed listener for system data listener events. */
   private final DataSourceListener dataSourceListener = new HubDataSourceListener();
 
-  /** Whether we've been started. */
-  private boolean started;
-
   // Cached preference values
   private int minRequiredAccuracy;
   private boolean useMetricUnits;
@@ -193,23 +190,21 @@ public class TrackDataHub {
   private int lastSamplingFrequency;
   private DoubleBufferedLocationFactory locationFactory;
 
-  private static TrackDataHub instance;
-  
-  public synchronized static TrackDataHub getInstance(Context context) {
-    if (instance != null) {
-      return instance;
-    }
+  private static TrackDataHub startedInstance;
 
+  /**
+   * Builds a new {@link TrackDataHub} instance.
+   */
+  public synchronized static TrackDataHub newInstance(Context context) {
     // Ensure our singleton is never bound to an activity, to avoid memory leaks.
     context = context.getApplicationContext();
 
     SharedPreferences preferences = context.getSharedPreferences(Constants.SETTINGS_NAME, 0);
     MyTracksProviderUtils providerUtils = MyTracksProviderUtils.Factory.get(context);
-    instance = new TrackDataHub(context,
+    return new TrackDataHub(context,
         new TrackDataListeners(),
         preferences, providerUtils,
         TARGET_DISPLAYED_TRACK_POINTS);
-    return instance;
   }
 
   /**
@@ -240,11 +235,11 @@ public class TrackDataHub {
    */
   public void start() {
     Log.i(TAG, "TrackDataHub.start");
-    if (started) {
+    if (startedInstance != null) {
       Log.w(TAG, "Already started, ignoring");
       return;
     }
-    started = true;
+    startedInstance = this;
 
     listenerHandlerThread = new HandlerThread("trackDataContentThread");
     listenerHandlerThread.start();
@@ -266,13 +261,27 @@ public class TrackDataHub {
     return new DataSourcesWrapperImpl(context, preferences);
   }
 
+  /**
+   * If there's an instance for which {@link start} has been called, returns it.
+   *
+   * @return the started instance
+   * @throws IllegalStateException if there isn't a started instance
+   */
+  public static TrackDataHub getStartedInstance() {
+    if (startedInstance == null) {
+      throw new IllegalStateException("Data hub not started");
+    }
+
+    return startedInstance;
+  }
+
   /**
    * Stops listening to data sources and reporting the data to external
    * listeners.
    */
   public void stop() {
     Log.i(TAG, "TrackDataHub.stop");
-    if (!started) {
+    if (!isStarted()) {
       Log.w(TAG, "Not started, ignoring");
       return;
     }
@@ -281,7 +290,7 @@ public class TrackDataHub {
     dataSourceManager.unregisterAllListeners();
     listenerHandlerThread.getLooper().quit();
 
-    started = false;
+    startedInstance = null;
 
     dataSources = null;
     dataSourceManager = null;
@@ -289,12 +298,16 @@ public class TrackDataHub {
     listenerHandler = null;
   }
 
+  private boolean isStarted() {
+    return startedInstance != null;
+  }
+
   @Override
   protected void finalize() throws Throwable {
-    if (started || listenerHandlerThread.isAlive()) {
+    if (isStarted() || listenerHandlerThread.isAlive()) {
       Log.e(TAG, "Forgot to stop() TrackDataHub");
     }
- 
+
     super.finalize();
   }
 
@@ -346,7 +359,7 @@ public class TrackDataHub {
    * is not available or doesn't have a fix.
    */
   public void forceUpdateLocation() {
-    if (!started) {
+    if (!isStarted()) {
       Log.w(TAG, "Not started, not forcing location update");
       return;
     }
@@ -361,7 +374,7 @@ public class TrackDataHub {
 
   /** Returns the ID of the currently-selected track. */
   public long getSelectedTrackId() {
-    if (!started) {
+    if (!isStarted()) {
       loadSharedPreferences();
     }
     return selectedTrackId;
@@ -374,7 +387,7 @@ public class TrackDataHub {
 
   /** Returns whether the selected track is still being recorded. */
   public boolean isRecordingSelected() {
-    if (!started) {
+    if (!isStarted()) {
       loadSharedPreferences();
     }
     long recordingTrackId = preferences.getLong(RECORDING_TRACK_KEY, -1);
@@ -430,7 +443,7 @@ public class TrackDataHub {
 
       // Don't load any data or start internal listeners if start() hasn't been
       // called. When it is called, we'll do both things.
-      if (!started) return;
+      if (!isStarted()) return;
 
       reloadDataForListener(registration);
 
@@ -444,7 +457,7 @@ public class TrackDataHub {
 
       // Don't load any data or start internal listeners if start() hasn't been
       // called. When it is called, we'll do both things.
-      if (!started) return;
+      if (!isStarted()) return;
 
       dataSourceManager.updateAllListeners(getNeededListenerTypes());
     }
@@ -463,11 +476,11 @@ public class TrackDataHub {
 
   /**
    * Reloads all track data received so far into the specified listeners.
-   * 
+   *
    * Assumes it's called from a block that synchronizes on {@link #listeners}.
    */
   private void reloadDataForListener(final ListenerRegistration registration) {
-    if (!started) {
+    if (!isStarted()) {
       Log.w(TAG, "Not started, not reloading");
       return;
     }
@@ -559,7 +572,7 @@ public class TrackDataHub {
    * Reloads all track data received so far into the specified listeners.
    */
   private void loadDataForAllListeners() {
-    if (!started) {
+    if (!isStarted()) {
       Log.w(TAG, "Not started, not reloading");
       return;
     }
@@ -626,13 +639,13 @@ public class TrackDataHub {
 
   /** Called when the speed/pace reporting preference changes. */
   private void notifySpeedReportingChanged() {
-    if (!started) return;
+    if (!isStarted()) return;
 
     runInListenerThread(new Runnable() {
       @Override
       public void run() {
         Set<TrackDataListener> displayListeners =
-            getListenersFor(ListenerDataType.DISPLAY_PREFERENCES);        
+            getListenersFor(ListenerDataType.DISPLAY_PREFERENCES);
 
         for (TrackDataListener listener : displayListeners) {
           // TODO: Do the reloading just once for all interested listeners
@@ -648,12 +661,12 @@ public class TrackDataHub {
 
   /** Called when the metric units setting changes. */
   private void notifyUnitsChanged() {
-    if (!started) return;
+    if (!isStarted()) return;
 
     runInListenerThread(new Runnable() {
       @Override
       public void run() {
-        Set<TrackDataListener> displayListeners = getListenersFor(ListenerDataType.DISPLAY_PREFERENCES);        
+        Set<TrackDataListener> displayListeners = getListenersFor(ListenerDataType.DISPLAY_PREFERENCES);
 
         for (TrackDataListener listener : displayListeners) {
           if (listener.onUnitsChanged(useMetricUnits)) {
@@ -868,7 +881,7 @@ public class TrackDataHub {
               if (!LocationUtils.isValidLocation(waypoint.getLocation())) {
                 continue;
               }
-    
+
               for (TrackDataListener listener : listeners) {
                 listener.onNewWaypoint(waypoint);
               }
@@ -879,7 +892,7 @@ public class TrackDataHub {
             cursor.close();
           }
         }
-    
+
         for (TrackDataListener listener : listeners) {
           listener.onNewWaypointsDone();
         }

MyTracks/src/com/google/android/apps/mytracks/services/ServiceUtils.java

@@ -30,7 +30,7 @@ import android.util.Log;
  *
  * @author Rodrigo Damazio
  */
-public class ServiceStateHelper {
+public class ServiceUtils {
 
   public static boolean isRecording(Context ctx, SharedPreferences preferences) {
     TrackRecordingServiceBinder serviceBinder = TrackRecordingServiceBinder.getInstance(ctx);
@@ -51,5 +51,5 @@ public class ServiceStateHelper {
     return preferences.getLong(ctx.getString(R.string.recording_track_key), -1) > 0;
   }
 
-  private ServiceStateHelper() {}
+  private ServiceUtils() {}
 }

MyTracks/src/com/google/android/apps/mytracks/services/TrackRecordingServiceBinder.java

@@ -30,7 +30,7 @@ import android.util.Log;
 import java.util.WeakHashMap;
 
 /**
- * A helper for managing the binding to the track recording service.
+ * A manager for the binding to the track recording service.
  * This uses reference counting so multiple callers can share a binding.
  *
  * @author Rodrigo Damazio
@@ -70,7 +70,7 @@ public class TrackRecordingServiceBinder {
     }
   };
 
-  /** Pointer to the service, if bound. */
+  /** Reference to the service, if bound. */
   private ITrackRecordingService trackRecordingService;
 
   /** Count of bindings to the service. */
@@ -99,7 +99,7 @@ public class TrackRecordingServiceBinder {
   /**
    * Binds to the service, and calls the given callback afterwards.
    * Calls to this method should be balanced with calls to {@link #unbindService}.
-   * 
+   *
    * @param onBindCallback the callback for when the service is connected
    */
   public void bindService(Runnable onBindCallback) {
@@ -118,7 +118,7 @@ public class TrackRecordingServiceBinder {
 
       if (bindCount == 1) {
         Intent intent = new Intent(applicationContext, TrackRecordingService.class);
-        int flags = SystemUtils.isRelease(applicationContext) ? 0 : Context.BIND_DEBUG_UNBIND; 
+        int flags = SystemUtils.isRelease(applicationContext) ? 0 : Context.BIND_DEBUG_UNBIND;
         applicationContext.bindService(intent, serviceConnection, flags);
       }
     }
@@ -186,8 +186,7 @@ public class TrackRecordingServiceBinder {
    */
   public synchronized static TrackRecordingServiceBinder getInstance(Context context) {
     if (instance == null) {
-      Context applicationContext = context.getApplicationContext();
-      instance = new TrackRecordingServiceBinder(applicationContext);
+      instance = new TrackRecordingServiceBinder(context.getApplicationContext());
     }
     return instance;
   }

MyTracksTest/src/com/google/android/apps/mytracks/MyTracksTest.java

@@ -15,7 +15,7 @@
  */
 package com.google.android.apps.mytracks;
 
-import com.google.android.apps.mytracks.services.ServiceStateHelper;
+import com.google.android.apps.mytracks.services.ServiceUtils;
 import com.google.android.maps.mytracks.R;
 
 import android.app.Activity;
@@ -250,6 +250,6 @@ public class MyTracksTest extends ActivityInstrumentationTestCase2<MyTracks>{
   }
 
   private boolean isRecording() {
-    return ServiceStateHelper.isRecording(getActivity(), getSharedPreferences());
+    return ServiceUtils.isRecording(getActivity(), getSharedPreferences());
   }
 }