From 5870ba98b426a63451e493514383b4124c612f13 Mon Sep 17 00:00:00 2001 From: Rodrigo Damazio Date: Mon, 21 Mar 2011 01:54:39 -0700 Subject: [PATCH] Review suggestions. --- .../android/apps/mytracks/ChartActivity.java | 18 +- .../android/apps/mytracks/MyTracks.java | 20 +- .../android/apps/mytracks/MyTracksMap.java | 42 +-- .../android/apps/mytracks/StatsActivity.java | 14 +- .../android/apps/mytracks/TrackDataHub.java | 248 ++++++++++-------- .../content/MyTracksProviderUtils.java | 18 ++ 6 files changed, 188 insertions(+), 172 deletions(-) diff --git a/MyTracks/src/com/google/android/apps/mytracks/ChartActivity.java b/MyTracks/src/com/google/android/apps/mytracks/ChartActivity.java index e617c0a64..7cc44f2c4 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/ChartActivity.java +++ b/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 = new Location(location); + lastLocation = location; if (result != null) { result[0] = timeOrDistance; diff --git a/MyTracks/src/com/google/android/apps/mytracks/MyTracks.java b/MyTracks/src/com/google/android/apps/mytracks/MyTracks.java index 4c6e9b31e..a896ec767 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/MyTracks.java +++ b/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); } diff --git a/MyTracks/src/com/google/android/apps/mytracks/MyTracksMap.java b/MyTracks/src/com/google/android/apps/mytracks/MyTracksMap.java index 8a85f7b42..43614d3af 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/MyTracksMap.java +++ b/MyTracks/src/com/google/android/apps/mytracks/MyTracksMap.java @@ -155,12 +155,21 @@ public class MyTracksMap extends MapActivity } @Override - protected void onStop() { - Log.d(TAG, "MyTracksMap.onStop"); - - dataHub.unregisterTrackDataListener(this); - - super.onStop(); + protected void onRestoreInstanceState(Bundle bundle) { + Log.d(TAG, "MyTracksMap.onRestoreInstanceState"); + if (bundle != null) { + super.onRestoreInstanceState(bundle); + keepMyLocationVisible = + 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) { - Log.d(TAG, "MyTracksMap.onRestoreInstanceState"); - if (bundle != null) { - super.onRestoreInstanceState(bundle); - keepMyLocationVisible = - 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; - } - } + protected void onStop() { + Log.d(TAG, "MyTracksMap.onStop"); + + dataHub.unregisterTrackDataListener(this); + + super.onStop(); } // Utility functions: diff --git a/MyTracks/src/com/google/android/apps/mytracks/StatsActivity.java b/MyTracks/src/com/google/android/apps/mytracks/StatsActivity.java index a9fa70935..3ddab3bbd 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/StatsActivity.java +++ b/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); diff --git a/MyTracks/src/com/google/android/apps/mytracks/TrackDataHub.java b/MyTracks/src/com/google/android/apps/mytracks/TrackDataHub.java index bb9d0165d..2e1fd9d45 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/TrackDataHub.java +++ b/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 ContentObserver waypointObserver; - private ContentObserver trackObserver; - private LocationListener locationListener; - private OnSharedPreferenceChangeListener preferenceListener; - private SensorEventListener compassListener; + private final ContentObserver pointObserver; + private final ContentObserver waypointObserver; + private final ContentObserver trackObserver; + private final LocationListener locationListener; + private final OnSharedPreferenceChangeListener preferenceListener; + private final SensorEventListener compassListener; // External listeners (to pass data to activities) private final Set 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 Handler listenerHandler; + private final HandlerThread listenerHandlerThread; + 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() { - @Override - public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, - String key) { - notifyPreferenceChanged(key); - } - }; + preferenceListener = new HubSharedPreferenceListener(); 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, - new Runnable() { - @Override - 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()); - } + pointObserver = new TrackContentObserver(contentHandler, new PointObserverCallback()); + waypointObserver = new TrackContentObserver(contentHandler, new WaypointObserverCallback()); + trackObserver = new TrackContentObserver(contentHandler, new TrackObserverCallback()); - @Override - public void onAccuracyChanged(Sensor sensor, int accuracy) { - // 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); - } - }; + compassListener = new CompassListener(); + locationListener = new CurrentLocationListener(); } /** @@ -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() { - 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]; - } - }); + currentSelectedTrackId, minPointId, false, + locationFactory); 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: - // - Has the mod for the sampling frequency (includes first point). - // - 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(); - } - } + notifyNewPoint(location, locationId, lastStoredLocationId, + localNumLoadedPoints, pointSamplingFrequency, listeners); 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) { diff --git a/MyTracksLib/src/com/google/android/apps/mytracks/content/MyTracksProviderUtils.java b/MyTracksLib/src/com/google/android/apps/mytracks/content/MyTracksProviderUtils.java index a62d557d7..a9506f1fc 100644 --- a/MyTracksLib/src/com/google/android/apps/mytracks/content/MyTracksProviderUtils.java +++ b/MyTracksLib/src/com/google/android/apps/mytracks/content/MyTracksProviderUtils.java @@ -343,6 +343,24 @@ public interface MyTracksProviderUtils { return new Location("gps"); } }; + + /** + * 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