From a41994cb4144e447dba907b8ce07e0443a60b77a Mon Sep 17 00:00:00 2001 From: Rodrigo Damazio Date: Wed, 1 Jun 2011 17:34:44 -0300 Subject: [PATCH] Review suggestions --- .../android/apps/mytracks/ChartActivity.java | 5 +- .../android/apps/mytracks/MapActivity.java | 18 ++--- .../android/apps/mytracks/MyTracks.java | 14 ++-- .../android/apps/mytracks/StatsActivity.java | 13 +-- .../android/apps/mytracks/TrackList.java | 6 +- .../apps/mytracks/content/TrackDataHub.java | 81 +++++++++++-------- ...viceStateHelper.java => ServiceUtils.java} | 4 +- .../services/TrackRecordingServiceBinder.java | 11 ++- .../android/apps/mytracks/MyTracksTest.java | 4 +- 9 files changed, 83 insertions(+), 73 deletions(-) rename MyTracks/src/com/google/android/apps/mytracks/services/{ServiceStateHelper.java => ServiceUtils.java} (96%) diff --git a/MyTracks/src/com/google/android/apps/mytracks/ChartActivity.java b/MyTracks/src/com/google/android/apps/mytracks/ChartActivity.java index 45a92f2a6..2a052bbf1 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/ChartActivity.java +++ b/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 } diff --git a/MyTracks/src/com/google/android/apps/mytracks/MapActivity.java b/MyTracks/src/com/google/android/apps/mytracks/MapActivity.java index 69ccad437..5cdc369eb 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/MapActivity.java +++ b/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); } diff --git a/MyTracks/src/com/google/android/apps/mytracks/MyTracks.java b/MyTracks/src/com/google/android/apps/mytracks/MyTracks.java index 14273d723..e1b1131da 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/MyTracks.java +++ b/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; - } } diff --git a/MyTracks/src/com/google/android/apps/mytracks/StatsActivity.java b/MyTracks/src/com/google/android/apps/mytracks/StatsActivity.java index c4490fa1d..41271a684 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/StatsActivity.java +++ b/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 } diff --git a/MyTracks/src/com/google/android/apps/mytracks/TrackList.java b/MyTracks/src/com/google/android/apps/mytracks/TrackList.java index 2f463ee90..30cfd7ac4 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/TrackList.java +++ b/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); diff --git a/MyTracks/src/com/google/android/apps/mytracks/content/TrackDataHub.java b/MyTracks/src/com/google/android/apps/mytracks/content/TrackDataHub.java index d4ddb5ebe..5093708bf 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/content/TrackDataHub.java +++ b/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 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 displayListeners = getListenersFor(ListenerDataType.DISPLAY_PREFERENCES); + Set 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(); } diff --git a/MyTracks/src/com/google/android/apps/mytracks/services/ServiceStateHelper.java b/MyTracks/src/com/google/android/apps/mytracks/services/ServiceUtils.java similarity index 96% rename from MyTracks/src/com/google/android/apps/mytracks/services/ServiceStateHelper.java rename to MyTracks/src/com/google/android/apps/mytracks/services/ServiceUtils.java index 4051ae94f..111b9f1bc 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/services/ServiceStateHelper.java +++ b/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() {} } diff --git a/MyTracks/src/com/google/android/apps/mytracks/services/TrackRecordingServiceBinder.java b/MyTracks/src/com/google/android/apps/mytracks/services/TrackRecordingServiceBinder.java index d280cf005..e216ee7ff 100644 --- a/MyTracks/src/com/google/android/apps/mytracks/services/TrackRecordingServiceBinder.java +++ b/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; } diff --git a/MyTracksTest/src/com/google/android/apps/mytracks/MyTracksTest.java b/MyTracksTest/src/com/google/android/apps/mytracks/MyTracksTest.java index d3b54a869..9f6973d35 100644 --- a/MyTracksTest/src/com/google/android/apps/mytracks/MyTracksTest.java +++ b/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{ } private boolean isRecording() { - return ServiceStateHelper.isRecording(getActivity(), getSharedPreferences()); + return ServiceUtils.isRecording(getActivity(), getSharedPreferences()); } }