Review suggestions.

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

@@ -89,7 +89,6 @@ public class ChartActivity extends Activity implements TrackDataListener {
   private final Runnable updateChart = new Runnable() {
     @Override
     public void run() {
-      Log.e(TAG, "Gone", new Throwable());
       busyPane.setVisibility(View.GONE);
       zoomControls.setIsZoomInEnabled(chartView.canZoomIn());
       zoomControls.setIsZoomOutEnabled(chartView.canZoomOut());
@@ -135,13 +134,6 @@ public class ChartActivity extends Activity implements TrackDataListener {
     });
   }
 
-  @Override
-  protected void onStop() {
-    dataHub.unregisterTrackDataListener(this);
-
-    super.onStop();
-  }
-
   @Override
   protected void onStart() {
     super.onStart();
@@ -149,6 +141,13 @@ public class ChartActivity extends Activity implements TrackDataListener {
     dataHub.registerTrackDataListener(this);
   }
 
+  @Override
+  protected void onStop() {
+    dataHub.unregisterTrackDataListener(this);
+
+    super.onStop();
+  }
+
   private void zoomIn() {
     chartView.zoomIn();
     zoomControls.setIsZoomInEnabled(chartView.canZoomIn());
@@ -301,8 +300,7 @@ public class ChartActivity extends Activity implements TrackDataListener {
     }
 
     // Keep a copy so the location can be reused.
-    // TODO: Make the Data Manager use double-buffering
+    lastLocation = location;
-    lastLocation = new Location(location);
 
     if (result != null) {
       result[0] = timeOrDistance;

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

@@ -436,6 +436,7 @@ public class MyTracks extends TabActivity implements OnTouchListener,
   public void onActivityResult(int requestCode, int resultCode,
       final Intent results) {
     TrackFileFormat exportFormat = null;
+    final long trackId = results.getLongExtra("trackid", dataHub.getSelectedTrackId());
     switch (requestCode) {
       case Constants.GET_LOGIN: {
         if (resultCode != RESULT_OK || auth == null || !auth.authResult(resultCode, results)) {
@@ -445,7 +446,6 @@ public class MyTracks extends TabActivity implements OnTouchListener,
       }
       case Constants.SHOW_TRACK: {
         if (results != null) {
-          final long trackId = results.getLongExtra("trackid", -1);
           if (trackId >= 0) {
             dataHub.loadTrack(trackId);
 
@@ -474,14 +474,12 @@ public class MyTracks extends TabActivity implements OnTouchListener,
       }
       case Constants.DELETE_TRACK: {
         if (results != null && resultCode == RESULT_OK) {
-          final long trackId = results.getLongExtra("trackid", dataHub.getSelectedTrackId());
           deleteTrack(trackId);
         }
         break;
       }
       case Constants.EDIT_DETAILS: {
         if (results != null && resultCode == RESULT_OK) {
-          final long trackId = results.getLongExtra("trackid", dataHub.getSelectedTrackId());
           Intent intent = new Intent(this, TrackDetails.class);
           intent.putExtra("trackid", trackId);
           startActivity(intent);
@@ -510,17 +508,11 @@ public class MyTracks extends TabActivity implements OnTouchListener,
         // Authenticated with Google My Maps
         if (results != null && resultCode == RESULT_OK) {
           final String mapId;
-          final long trackId;
           if (results.hasExtra("mapid")) {
             mapId = results.getStringExtra("mapid");
           } else {
             mapId = "new";
           }
-          if (results.hasExtra("trackid")) {
-            trackId = results.getLongExtra("trackid", -1);
-          } else {
-            trackId = dataHub.getSelectedTrackId();
-          }
 
           sendToGoogleMaps(trackId, mapId);
         } else {
@@ -531,13 +523,6 @@ public class MyTracks extends TabActivity implements OnTouchListener,
       case Constants.AUTHENTICATE_TO_FUSION_TABLES: {
         // Authenticated with Google Fusion Tables
         if (results != null && resultCode == RESULT_OK) {
-          final long trackId;
-          if (results.hasExtra("trackid")) {
-            trackId = results.getLongExtra("trackid", -1);
-          } else {
-            trackId = dataHub.getSelectedTrackId();
-          }
-
           sendToFusionTables(trackId);
         } else {
           onSendToGoogleDone();
@@ -556,7 +541,6 @@ public class MyTracks extends TabActivity implements OnTouchListener,
       case Constants.AUTHENTICATE_TO_TRIX: {
         // Authenticated with Trix
         if (resultCode == RESULT_OK) {
-          final long trackId = results.getLongExtra("trackid", dataHub.getSelectedTrackId());
           sendToGoogleDocs(trackId);
         } else {
           onSendToGoogleDone();
@@ -576,7 +560,6 @@ public class MyTracks extends TabActivity implements OnTouchListener,
         if (exportFormat == null) { exportFormat = TrackFileFormat.TCX; }
 
         if (results != null && resultCode == Activity.RESULT_OK) {
-          final long trackId = results.getLongExtra("trackid", dataHub.getSelectedTrackId());
           if (trackId >= 0) {
             saveTrack(trackId, exportFormat);
           }
@@ -609,7 +592,6 @@ public class MyTracks extends TabActivity implements OnTouchListener,
         if (exportFormat == null) { exportFormat = TrackFileFormat.TCX; }
 
         if (results != null && resultCode == Activity.RESULT_OK) {
-          final long trackId = results.getLongExtra("trackid", dataHub.getSelectedTrackId());
           if (trackId >= 0) {
             sendTrack(trackId, exportFormat);
           }

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

@@ -155,12 +155,21 @@ public class MyTracksMap extends MapActivity
   }
 
   @Override
-  protected void onStop() {
+  protected void onRestoreInstanceState(Bundle bundle) {
-    Log.d(TAG, "MyTracksMap.onStop");
+    Log.d(TAG, "MyTracksMap.onRestoreInstanceState");
-
+    if (bundle != null) {
-    dataHub.unregisterTrackDataListener(this);
+      super.onRestoreInstanceState(bundle);
-
+      keepMyLocationVisible =
-    super.onStop();
+          bundle.getBoolean(KEY_KEEP_MY_LOCATION_VISIBLE, false);
+      if (bundle.containsKey(KEY_CURRENT_LOCATION)) {
+        currentLocation = (Location) bundle.getParcelable(KEY_CURRENT_LOCATION);
+        if (currentLocation != null) {
+          showCurrentLocation();
+        }
+      } else {
+        currentLocation = null;
+      }
+    }
   }
 
   @Override
@@ -182,21 +191,12 @@ public class MyTracksMap extends MapActivity
   }
 
   @Override
-  protected void onRestoreInstanceState(Bundle bundle) {
+  protected void onStop() {
-    Log.d(TAG, "MyTracksMap.onRestoreInstanceState");
+    Log.d(TAG, "MyTracksMap.onStop");
-    if (bundle != null) {
+
-      super.onRestoreInstanceState(bundle);
+    dataHub.unregisterTrackDataListener(this);
-      keepMyLocationVisible =
+
-          bundle.getBoolean(KEY_KEEP_MY_LOCATION_VISIBLE, false);
+    super.onStop();
-      if (bundle.containsKey(KEY_CURRENT_LOCATION)) {
-        currentLocation = (Location) bundle.getParcelable(KEY_CURRENT_LOCATION);
-        if (currentLocation != null) {
-          showCurrentLocation();
-        }
-      } else {
-        currentLocation = null;
-      }
-    }
   }
 
   // Utility functions:

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

@@ -125,6 +125,13 @@ public class StatsActivity extends Activity implements TrackDataListener {
     }
   }
 
+  @Override
+  protected void onStart() {
+    dataHub.registerTrackDataListener(this);
+
+    super.onStart();
+  }
+
   @Override
   protected void onStop() {
     dataHub.unregisterTrackDataListener(this);
@@ -137,13 +144,6 @@ public class StatsActivity extends Activity implements TrackDataListener {
     super.onStop();
   }
 
-  @Override
-  protected void onStart() {
-    dataHub.registerTrackDataListener(this);
-
-    super.onStart();
-  }
-
   @Override
   public boolean onUnitsChanged(boolean metric) {
     utils.setMetricUnits(metric);

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

@@ -18,9 +18,7 @@ package com.google.android.apps.mytracks;
 import static com.google.android.apps.mytracks.Constants.TAG;
 
 import com.google.android.apps.mytracks.TrackDataListener.ProviderState;
-import com.google.android.apps.mytracks.content.MyTracksLocation;
 import com.google.android.apps.mytracks.content.MyTracksProviderUtils;
-import com.google.android.apps.mytracks.content.MyTracksProviderUtils.LocationFactory;
 import com.google.android.apps.mytracks.content.MyTracksProviderUtils.LocationIterator;
 import com.google.android.apps.mytracks.content.Track;
 import com.google.android.apps.mytracks.content.TrackPointsColumns;
@@ -78,7 +76,7 @@ public class TrackDataHub {
 
   // Application services
   private final Context context;
-  private MyTracksProviderUtils providerUtils;
+  private final MyTracksProviderUtils providerUtils;
 
   // System services
   private final SensorManager sensorManager;
@@ -87,12 +85,12 @@ public class TrackDataHub {
   private final ContentResolver contentResolver;
 
   // Internal listeners (to receive data from the system)
-  private ContentObserver pointObserver;
+  private final ContentObserver pointObserver;
-  private ContentObserver waypointObserver;
+  private final ContentObserver waypointObserver;
-  private ContentObserver trackObserver;
+  private final ContentObserver trackObserver;
-  private LocationListener locationListener;
+  private final LocationListener locationListener;
-  private OnSharedPreferenceChangeListener preferenceListener;
+  private final OnSharedPreferenceChangeListener preferenceListener;
-  private SensorEventListener compassListener;
+  private final SensorEventListener compassListener;
 
   // External listeners (to pass data to activities)
   private final Set<TrackDataListener> registeredListeners =
@@ -100,8 +98,8 @@ public class TrackDataHub {
 
   // Get content notifications on the main thread, send listener callbacks in another.
   // This ensures listener calls are serialized.
-  private HandlerThread listenerHandlerThread;
+  private final HandlerThread listenerHandlerThread;
-  private Handler listenerHandler;
+  private final Handler listenerHandler;
   private boolean started;
 
   // Cached preference values
@@ -125,6 +123,38 @@ public class TrackDataHub {
   private int numLoadedPoints;
   private boolean hasProviderEnabled;
 
+  /** Callback for when the tracks table is updated. */
+  private class TrackObserverCallback implements Runnable {
+    @Override
+    public void run() {
+      notifyTrackUpdated(getRegisteredListenerArray());
+    }
+  }
+
+  /** Callback for when the waypoints table is updated. */
+  private class WaypointObserverCallback implements Runnable {
+    @Override
+    public void run() {
+      notifyWaypointUpdated(getRegisteredListenerArray());
+    }
+  }
+
+  /** Callback for when the points table is updated. */
+  private class PointObserverCallback implements Runnable {
+    @Override
+    public void run() {
+      notifyPointsUpdated(true, getRegisteredListenerArray());
+    }
+  }
+
+  /** Listener for when preferences change. */
+  private class HubSharedPreferenceListener implements OnSharedPreferenceChangeListener {
+    @Override
+    public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key) {
+      notifyPreferenceChanged(key);
+    }
+  }
+
   /**
    * Generic content observer which will call a given {@link Runnable} in the
    * given handler if the content has changed and we're recording the selected
@@ -158,6 +188,55 @@ public class TrackDataHub {
     }
   }
 
+  /** Listener for the current location (independent from track data). */
+  private class CurrentLocationListener implements
+      LocationListener {
+    @Override
+    public void onStatusChanged(String provider, int status, Bundle extras) {
+      if (!LocationManager.GPS_PROVIDER.equals(provider)) return;
+
+      hasProviderEnabled = (status == LocationProvider.AVAILABLE);
+      notifyFixType();
+    }
+
+    @Override
+    public void onProviderEnabled(String provider) {
+      if (!LocationManager.GPS_PROVIDER.equals(provider)) return;
+
+      hasProviderEnabled = true;
+      notifyFixType();
+    }
+
+    @Override
+    public void onProviderDisabled(String provider) {
+      if (!LocationManager.GPS_PROVIDER.equals(provider)) return;
+
+      hasProviderEnabled = false;
+      notifyFixType();
+    }
+
+    @Override
+    public void onLocationChanged(Location location) {
+      notifyLocationChanged(location);
+    }
+  }
+
+  /** Listener for compass readings. */
+  private class CompassListener implements
+      SensorEventListener {
+    @Override
+    public void onSensorChanged(SensorEvent event) {
+      lastSeenMagneticHeading = event.values[0];
+      maybeUpdateDeclination();
+      notifyHeadingChanged(getRegisteredListenerArray());
+    }
+
+    @Override
+    public void onAccuracyChanged(Sensor sensor, int accuracy) {
+      // Do nothing
+    }
+  }
+
   public TrackDataHub(Context ctx, MyTracksProviderUtils providerUtils) {
     this.context = ctx;
     this.providerUtils = providerUtils;
@@ -173,13 +252,7 @@ public class TrackDataHub {
     listenerHandler = new Handler(listenerHandlerThread.getLooper());
 
     sharedPreferences = ctx.getSharedPreferences(MyTracksSettings.SETTINGS_NAME, 0);
-    preferenceListener = new OnSharedPreferenceChangeListener() {
+    preferenceListener = new HubSharedPreferenceListener();
-      @Override
-      public void onSharedPreferenceChanged(SharedPreferences sharedPreferences,
-          String key) {
-        notifyPreferenceChanged(key);
-      }
-    };
 
     sensorManager = (SensorManager) ctx.getSystemService(Context.SENSOR_SERVICE);
     locationManager =
@@ -187,70 +260,12 @@ public class TrackDataHub {
 
     contentResolver = ctx.getContentResolver();
     Handler contentHandler = new Handler();
-    pointObserver = new TrackContentObserver(contentHandler,
+    pointObserver = new TrackContentObserver(contentHandler, new PointObserverCallback());
-        new Runnable() {
+    waypointObserver = new TrackContentObserver(contentHandler, new WaypointObserverCallback());
-          @Override
+    trackObserver = new TrackContentObserver(contentHandler, new TrackObserverCallback());
-          public void run() {
-            notifyPointsUpdated(true, getRegisteredListenerArray());
-          }
-        });
-    waypointObserver = new TrackContentObserver(contentHandler,
-        new Runnable() {
-          @Override
-          public void run() {
-            notifyWaypointUpdated(getRegisteredListenerArray());
-          }
-        });
-    trackObserver = new TrackContentObserver(contentHandler,
-        new Runnable() {
-          @Override
-          public void run() {
-            notifyTrackUpdated(getRegisteredListenerArray());
-          }
-        });
-    compassListener = new SensorEventListener() {
-      @Override
-      public void onSensorChanged(SensorEvent event) {
-        lastSeenMagneticHeading = event.values[0];
-        maybeUpdateDeclination();
-        notifyHeadingChanged(getRegisteredListenerArray());
-      }
 
-      @Override
+    compassListener = new CompassListener();
-      public void onAccuracyChanged(Sensor sensor, int accuracy) {
+    locationListener = new CurrentLocationListener();
-        // Do nothing
-      }
-    };
-    locationListener = new LocationListener() {
-      @Override
-      public void onStatusChanged(String provider, int status, Bundle extras) {
-        if (!LocationManager.GPS_PROVIDER.equals(provider)) return;
-
-        hasProviderEnabled = (status == LocationProvider.AVAILABLE);
-        notifyFixType();
-      }
-
-      @Override
-      public void onProviderEnabled(String provider) {
-        if (!LocationManager.GPS_PROVIDER.equals(provider)) return;
-
-        hasProviderEnabled = true;
-        notifyFixType();
-      }
-
-      @Override
-      public void onProviderDisabled(String provider) {
-        if (!LocationManager.GPS_PROVIDER.equals(provider)) return;
-
-        hasProviderEnabled = false;
-        notifyFixType();
-      }
-
-      @Override
-      public void onLocationChanged(Location location) {
-        notifyLocationChanged(location);
-      }
-    };
   }
 
   /**
@@ -887,21 +902,11 @@ public class TrackDataHub {
     int pointSamplingFrequency = -1;
 
     // Create a double-buffering location provider.
+    MyTracksProviderUtils.DoubleBufferedLocationFactory locationFactory =
+        new MyTracksProviderUtils.DoubleBufferedLocationFactory();
     LocationIterator it = providerUtils.getLocationIterator(
-        currentSelectedTrackId, minPointId, false, new LocationFactory() {
+        currentSelectedTrackId, minPointId, false,
-          private final Location locs[] = new MyTracksLocation[] {
+        locationFactory);
-            new MyTracksLocation("gps"),
-            new MyTracksLocation("gps")
-          };
-
-          private int lastLoc = 0;
-
-          @Override
-          public Location createLocation() {
-            lastLoc = (lastLoc + 1) % locs.length;
-            return locs[lastLoc];
-          }
-        });
 
     while (it.hasNext()) {
       if (currentSelectedTrackId != selectedTrackId) {
@@ -934,26 +939,8 @@ public class TrackDataHub {
             (int) (1 + numTotalPoints / Constants.TARGET_DISPLAYED_TRACK_POINTS);
       }
 
-      // Include a point if it fits one of the following criteria:
+      notifyNewPoint(location, locationId, lastStoredLocationId,
-      // - Has the mod for the sampling frequency (includes first point).
+          localNumLoadedPoints, pointSamplingFrequency, listeners);
-      // - Is the last point and we are not recording this track.
-      boolean isValid = MyTracksUtils.isValidLocation(location);
-      if (isValid &&
-          (localNumLoadedPoints % pointSamplingFrequency == 0 ||
-           (!isRecordingSelected() && locationId == lastStoredLocationId))) {
-        // No need to allocate a new location (we can safely reuse the existing).
-        for (TrackDataListener listener : listeners) {
-          listener.onNewTrackPoint(location);
-        }
-      }
-
-      // Report segment splits separately.
-      if (!isValid) {
-        // TODO: Always send last valid point before and first valid point after a split
-        for (TrackDataListener listener : listeners) {
-          listener.onSegmentSplit();
-        }
-      }
 
       localNumLoadedPoints++;
       localLastSeenLocationId = locationId;
@@ -971,6 +958,37 @@ public class TrackDataHub {
     }
   }
 
+  private void notifyNewPoint(Location location,
+      long locationId,
+      long lastStoredLocationId,
+      int numLoadedPoints,
+      int pointSamplingFrequency,
+      TrackDataListener[] listeners) {
+    boolean isValid = MyTracksUtils.isValidLocation(location);
+    if (isValid) {
+      // Include a point if it fits one of the following criteria:
+      // - Has the mod for the sampling frequency (includes first point).
+      // - Is the last point and we are not recording this track.
+      if (numLoadedPoints % pointSamplingFrequency == 0 ||
+         (!isRecordingSelected() && locationId == lastStoredLocationId)) {
+        // No need to allocate a new location (we can safely reuse the existing).
+        for (TrackDataListener listener : listeners) {
+          listener.onNewTrackPoint(location);
+        }
+      } else {
+        for (TrackDataListener listener : listeners) {
+          listener.onSampledOutTrackPoint(location);
+        }
+      }
+    } else {
+      // Report segment splits separately.
+      // TODO: Always send last valid point before and first valid point after a split
+      for (TrackDataListener listener : listeners) {
+        listener.onSegmentSplit();
+      }
+    }
+  }
+
   /** Returns an array with all the currently-registered listeners. */
   private TrackDataListener[] getRegisteredListenerArray() {
     synchronized (registeredListeners) {

MyTracksLib/src/com/google/android/apps/mytracks/content/MyTracksProviderUtils.java

@@ -344,6 +344,24 @@ public interface MyTracksProviderUtils {
     }
   };
 
+  /**
+   * A location factory which uses two location instances (one for the current location,
+   * and one for the previous), useful when we need to keep the last location.
+   */
+  public class DoubleBufferedLocationFactory implements LocationFactory {
+    private final Location locs[] = new MyTracksLocation[] {
+      new MyTracksLocation("gps"),
+      new MyTracksLocation("gps")
+    };
+    private int lastLoc = 0;
+
+    @Override
+    public Location createLocation() {
+      lastLoc = (lastLoc + 1) % locs.length;
+      return locs[lastLoc];
+    }
+  }
+  
   /**
    * Creates a new read-only iterator over all track points for the given track.  It provides
    * a lightweight way of iterating over long tracks without failing due to the underlying cursor