From 0eb676d3cd5e90431db400fff4882e6cb7a1bb41 Mon Sep 17 00:00:00 2001
From: David Piggott <david@piggott.me.uk>
Date: Mon, 14 Feb 2011 10:53:37 +0000
Subject: [PATCH] Improve style and usability of ImportAllTracks

Fix style issues pointed out in code review. Move acquireWakeLock method into
MyTracksUtils and call that from ImportAllTracks and ExportAllTracks. Replace
important Toast messages with dialogs.
---
 MyTracks/res/layout/mytracks_list.xml         |  20 +-
 MyTracks/res/values/strings.xml               |  16 +-
 .../apps/mytracks/ExportAllTracks.java        |  41 +---
 .../apps/mytracks/ImportAllTracks.java        | 215 ++++++++----------
 .../android/apps/mytracks/MyTracksList.java   |  14 +-
 .../apps/mytracks/util/MyTracksUtils.java     |  39 ++++
 6 files changed, 162 insertions(+), 183 deletions(-)

diff --git a/MyTracks/res/layout/mytracks_list.xml b/MyTracks/res/layout/mytracks_list.xml
index f0bf579a3..720984dd8 100644
--- a/MyTracks/res/layout/mytracks_list.xml
+++ b/MyTracks/res/layout/mytracks_list.xml
@@ -46,6 +46,16 @@ limitations under the License.
     android:background="@drawable/list_bar"
     android:layout_weight="0"
     android:padding="3dip">
+    <TextView
+      android:layout_width="wrap_content"
+      android:layout_height="wrap_content"
+      android:layout_weight="1"/>
+    <Button android:id="@+id/tracklist_btn_import_all"
+      android:layout_width="wrap_content"
+      android:layout_height="48dip"
+      android:layout_gravity="left|top"
+      android:text="@string/tracklist_btn_import_all"
+      android:textSize="14dip"/>
     <Button android:id="@+id/tracklist_btn_export_all"
       android:layout_width="wrap_content"
       android:layout_height="48dip"
@@ -58,15 +68,5 @@ limitations under the License.
       android:layout_gravity="left|top"
       android:text="@string/tracklist_btn_delete_all"
       android:textSize="14dip"/>
-    <TextView
-      android:layout_width="wrap_content"
-      android:layout_height="wrap_content"
-      android:layout_weight="1"/>
-    <Button android:id="@+id/tracklist_btn_import_all"
-      android:layout_width="wrap_content"
-      android:layout_height="48dip"
-      android:layout_gravity="left|top"
-      android:text="@string/tracklist_btn_import_all"
-      android:textSize="14dip"/>
    </LinearLayout>
 </LinearLayout>
diff --git a/MyTracks/res/values/strings.xml b/MyTracks/res/values/strings.xml
index ff61b6946..9761b42a8 100644
--- a/MyTracks/res/values/strings.xml
+++ b/MyTracks/res/values/strings.xml
@@ -140,9 +140,19 @@ limitations under the License.
   <string name="tracklist_share_tcx_file">A TCX file</string>
   <string name="tracklist_clear_map">Clear map</string>
   <!-- Import All -->
-  <string name="tracklist_btn_import_all">Import from SD</string>
-  <string name="import_done">Import finished</string>
-  <string name="import_empty">No files found in /sdcard/MyTracks/gpx/</string>
+  <string name="tracklist_btn_import_all">Import all</string>
+  <string name="import_done">Import finished, imported</string>
+  <!-- Format:
+  $1 : Successful imports
+  $2 : Attempted imports
+  $3 : MyTracks directory
+  -->
+  <string name="import_done">Imported %1$d of %2$d GPX files found in /sdcard/%3$s/gpx/</string>
+  <!-- Format:
+  $1 : MyTracks directory
+  -->
+  <string name="import_empty">No files found in /sdcard/%1$s/gpx/</string>
+  <string name="import_error">Import error for file</string>
 
   <!-- Waypoints list -->
   <string name="waypointslist_btn_delete_all">Delete all markers</string>
diff --git a/MyTracks/src/com/google/android/apps/mytracks/ExportAllTracks.java b/MyTracks/src/com/google/android/apps/mytracks/ExportAllTracks.java
index 1eb86831a..86c5bc9cf 100644
--- a/MyTracks/src/com/google/android/apps/mytracks/ExportAllTracks.java
+++ b/MyTracks/src/com/google/android/apps/mytracks/ExportAllTracks.java
@@ -20,18 +20,17 @@ import com.google.android.apps.mytracks.content.TracksColumns;
 import com.google.android.apps.mytracks.io.TrackWriter;
 import com.google.android.apps.mytracks.io.TrackWriterFactory;
 import com.google.android.apps.mytracks.io.TrackWriterFactory.TrackFileFormat;
+import com.google.android.apps.mytracks.util.MyTracksUtils;
 import com.google.android.maps.mytracks.R;
 
 import android.app.Activity;
 import android.app.AlertDialog;
 import android.app.ProgressDialog;
-import android.content.Context;
 import android.content.DialogInterface;
 import android.content.SharedPreferences;
 import android.database.Cursor;
 import android.os.Handler;
 import android.os.HandlerThread;
-import android.os.PowerManager;
 import android.os.PowerManager.WakeLock;
 import android.util.Log;
 import android.widget.Toast;
@@ -121,7 +120,7 @@ public class ExportAllTracks {
     	  prefs.getLong(activity.getString(R.string.recording_track_key), -1);
     }
     if (recordingTrackId != -1) {
-      acquireWakeLock();
+      wakeLock = MyTracksUtils.acquireWakeLock(activity, wakeLock);
     }
 
     // Now we can safely export everything.
@@ -211,40 +210,4 @@ public class ExportAllTracks {
       }
     }
   }
-
-  /**
-   * Tries to acquire a partial wake lock if not already acquired. Logs errors
-   * and gives up trying in case the wake lock cannot be acquired.
-   */
-  private void acquireWakeLock() {
-    Log.i(MyTracksConstants.TAG, "ExportAllTracks: Aquiring wake lock.");
-    try {
-      PowerManager pm =
-          (PowerManager) activity.getSystemService(Context.POWER_SERVICE);
-      if (pm == null) {
-        Log.e(MyTracksConstants.TAG,
-            "ExportAllTracks: Power manager not found!");
-        return;
-      }
-      if (wakeLock == null) {
-        wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK,
-            MyTracksConstants.TAG);
-        if (wakeLock == null) {
-          Log.e(MyTracksConstants.TAG,
-              "ExportAllTracks: Could not create wake lock (null).");
-          return;
-        }
-      }
-      if (!wakeLock.isHeld()) {
-        wakeLock.acquire();
-        if (!wakeLock.isHeld()) {
-          Log.e(MyTracksConstants.TAG,
-              "ExportAllTracks: Could not acquire wake lock.");
-        }
-      }
-    } catch (RuntimeException e) {
-      Log.e(MyTracksConstants.TAG,
-          "ExportAllTracks: Caught unexpected exception: " + e.getMessage(), e);
-    }
-  }
 }
diff --git a/MyTracks/src/com/google/android/apps/mytracks/ImportAllTracks.java b/MyTracks/src/com/google/android/apps/mytracks/ImportAllTracks.java
index 72cdaa3a6..be62d7041 100644
--- a/MyTracks/src/com/google/android/apps/mytracks/ImportAllTracks.java
+++ b/MyTracks/src/com/google/android/apps/mytracks/ImportAllTracks.java
@@ -15,32 +15,26 @@
  */
 package com.google.android.apps.mytracks;
 
+import com.google.android.apps.mytracks.content.MyTracksProviderUtils;
+import com.google.android.apps.mytracks.io.GpxImporter;
+import com.google.android.apps.mytracks.util.MyTracksUtils;
+import com.google.android.maps.mytracks.R;
+
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileNotFoundException;
-import java.io.IOException;
 import java.util.Iterator;
 import java.util.LinkedList;
 
-import javax.xml.parsers.ParserConfigurationException;
-
-import org.xml.sax.SAXException;
-
 import android.app.Activity;
+import android.app.AlertDialog;
 import android.app.ProgressDialog;
-import android.content.Context;
 import android.content.SharedPreferences;
 import android.os.Handler;
 import android.os.HandlerThread;
-import android.os.PowerManager;
 import android.os.PowerManager.WakeLock;
 import android.util.Log;
 import android.widget.Toast;
 
-import com.google.android.apps.mytracks.content.MyTracksProviderUtils;
-import com.google.android.apps.mytracks.io.GpxImporter;
-import com.google.android.maps.mytracks.R;
-
 /**
  * A class that will import all GPX tracks in /sdcard/MyTracks/gpx/
  *
@@ -51,7 +45,9 @@ public class ImportAllTracks {
   private final Activity activity;
   private WakeLock wakeLock;
   private ProgressDialog progress;
-
+  private int gpxFileCandidateCount;
+  private int importSuccessCount;
+  
   public ImportAllTracks(Activity activity) {
     this.activity = activity;
     Log.i(MyTracksConstants.TAG, "ImportAllTracks: Starting");
@@ -83,21 +79,32 @@ public class ImportAllTracks {
     	  prefs.getLong(activity.getString(R.string.recording_track_key), -1);
     }
     if (recordingTrackId != -1) {
-      acquireWakeLock();
+      wakeLock = MyTracksUtils.acquireWakeLock(activity, wakeLock);
     }
 
     // Now we can safely import everything.
     importAll();
 
-    // Release the wake lock if we recorded one.
+    // Release the wake lock if we acquired one.
     // TODO check what happens if we started recording after getting this lock.
     if (wakeLock != null && wakeLock.isHeld()) {
       wakeLock.release();
       Log.i(MyTracksConstants.TAG, "ImportAllTracks: Releasing wake lock.");
     }
-    Log.i(MyTracksConstants.TAG, "ImportAllTracks: Done");
-    Toast.makeText(activity, R.string.import_done, Toast.LENGTH_SHORT).show();
-  }
+    
+		Log.i(MyTracksConstants.TAG, "ImportAllTracks: Done");
+		AlertDialog.Builder builder = new AlertDialog.Builder(activity);
+		if (gpxFileCandidateCount == 0) {
+			builder.setMessage(activity.getString(R.string.import_empty,
+					MyTracksConstants.SDCARD_TOP_DIR));
+		} else {
+			builder.setMessage(activity.getString(R.string.import_done,
+					importSuccessCount, gpxFileCandidateCount,
+					MyTracksConstants.SDCARD_TOP_DIR));
+		}
+		builder.setPositiveButton(R.string.ok, null);
+		builder.show();
+	}
 
   private void makeProgressDialog(final int trackCount) {
     String importMsg = activity.getString(R.string.tracklist_btn_import_all);
@@ -114,112 +121,72 @@ public class ImportAllTracks {
    * Actually import the tracks.
    * This should be called after the wake locks have been acquired.
    */
-  private void importAll() {
-    try {
-      MyTracksProviderUtils providerUtils =
-          MyTracksProviderUtils.Factory.get(activity);
-
-      LinkedList<File> gpxFiles = new LinkedList<File>();
-      File[] gpxFileCandidates = new File("/sdcard/" + MyTracksConstants.SDCARD_TOP_DIR + "/gpx").listFiles(); 
-      if(gpxFileCandidates == null || gpxFileCandidates.length == 0) {
-    	  Toast.makeText(activity, activity.getString(R.string.import_empty), Toast.LENGTH_LONG).show();
-    	  return;
-      }
-      for (File file : gpxFileCandidates) {
-          if (!file.isDirectory() && file.getName().endsWith(".gpx")) {
-              gpxFiles.add(file);
-          }
-      }
-      
-      final int trackCount = gpxFiles.size();
-      
-      Log.i(MyTracksConstants.TAG,
-          "ImportAllTracks: Importing: " + trackCount + " tracks.");
-      activity.runOnUiThread(new Runnable() {
-        public void run() {
-          makeProgressDialog(trackCount);
-        }
-      });
-
-      Iterator<File> gpxFilesIterator = gpxFiles.iterator();
-      File currentFile;
-      int currentFileNumber = 0;
-      while(gpxFilesIterator.hasNext()) {
-    	currentFile = gpxFilesIterator.next();
-    	final int status = currentFileNumber;
-        activity.runOnUiThread(new Runnable() {
-          public void run() {
-            synchronized (this) {
-              if (progress == null) {
-                return;
-              }
-              progress.setProgress(status);
-            }
-          }
-        });
-
-        Log.i(MyTracksConstants.TAG, "ImportAllTracks: importing: " + currentFile.getName());
-        try {
-			GpxImporter.importGPXFile(new FileInputStream(currentFile), providerUtils);
-		} catch (FileNotFoundException e) {
-			e.printStackTrace();
-            Toast.makeText(activity, "FileNotFoundException", Toast.LENGTH_LONG).show();
-		} catch (ParserConfigurationException e) {
-			e.printStackTrace();
-            Toast.makeText(activity, "ParserConfigurationException", Toast.LENGTH_LONG).show();
-		} catch (SAXException e) {
-			e.printStackTrace();
-            Toast.makeText(activity, "SAXException", Toast.LENGTH_LONG).show();
-		} catch (IOException e) {
-			e.printStackTrace();
-            Toast.makeText(activity, "IOException", Toast.LENGTH_LONG).show();
+	private void importAll() {
+		MyTracksProviderUtils providerUtils = MyTracksProviderUtils.Factory
+				.get(activity);
+		LinkedList<File> gpxFiles = new LinkedList<File>();
+		File[] gpxFileCandidates = new File("/sdcard/"
+				+ MyTracksConstants.SDCARD_TOP_DIR + "/gpx").listFiles();
+		if (gpxFileCandidates != null) {
+			for (File file : gpxFileCandidates) {
+				if (!file.isDirectory() && file.getName().endsWith(".gpx")) {
+					gpxFiles.add(file);
+				}
+			}
 		}
-		
-		currentFileNumber++;
-      }
-    } finally {
-      if (progress != null) {
-        synchronized (this) {
-          progress.dismiss();
-          progress = null;
-        }
-      }
-    }
-  }
+		gpxFileCandidateCount = gpxFiles.size();
 
-  /**
-   * Tries to acquire a partial wake lock if not already acquired. Logs errors
-   * and gives up trying in case the wake lock cannot be acquired.
-   */
-  private void acquireWakeLock() {
-    Log.i(MyTracksConstants.TAG, "ImportAllTracks: Acquiring wake lock.");
-    try {
-      PowerManager pm =
-          (PowerManager) activity.getSystemService(Context.POWER_SERVICE);
-      if (pm == null) {
-        Log.e(MyTracksConstants.TAG,
-            "ImportAllTracks: Power manager not found!");
-        return;
-      }
-      if (wakeLock == null) {
-        wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK,
-            MyTracksConstants.TAG);
-        if (wakeLock == null) {
-          Log.e(MyTracksConstants.TAG,
-              "ImportAllTracks: Could not create wake lock (null).");
-          return;
-        }
-      }
-      if (!wakeLock.isHeld()) {
-        wakeLock.acquire();
-        if (!wakeLock.isHeld()) {
-          Log.e(MyTracksConstants.TAG,
-              "ImportAllTracks: Could not acquire wake lock.");
-        }
-      }
-    } catch (RuntimeException e) {
-      Log.e(MyTracksConstants.TAG,
-          "ImportAllTracks: Caught unexpected exception: " + e.getMessage(), e);
-    }
-  }
+		if (gpxFileCandidateCount != 0) {
+			Log.i(MyTracksConstants.TAG, "ImportAllTracks: Importing: "
+					+ gpxFileCandidateCount + " tracks.");
+			activity.runOnUiThread(new Runnable() {
+				public void run() {
+					makeProgressDialog(gpxFileCandidateCount);
+				}
+			});
+
+			Iterator<File> gpxFilesIterator = gpxFiles.iterator();
+			File currentFile;
+			int currentFileNumber = 0;
+			while (gpxFilesIterator.hasNext()) {
+				currentFile = gpxFilesIterator.next();
+				final int status = currentFileNumber;
+				activity.runOnUiThread(new Runnable() {
+					public void run() {
+						synchronized (this) {
+							if (progress == null) {
+								return;
+							}
+							progress.setProgress(status);
+						}
+					}
+				});
+				if (importFile(currentFile, providerUtils)) {
+					importSuccessCount++;
+				}
+				currentFileNumber++;
+			}
+			if (progress != null) {
+				synchronized (this) {
+					progress.dismiss();
+					progress = null;
+				}
+			}
+		}
+	}
+  
+	private boolean importFile(File gpxFile, MyTracksProviderUtils providerUtils) {
+		Log.i(MyTracksConstants.TAG, "ImportAllTracks: importing: "
+				+ gpxFile.getName());
+		try {
+			GpxImporter.importGPXFile(new FileInputStream(gpxFile), providerUtils);
+			return true;
+		} catch (Exception e) {
+			Log.w("", e);
+			Toast.makeText(activity,
+					activity.getString(R.string.import_error) + " " + gpxFile.getName(),
+					Toast.LENGTH_LONG).show();
+			return false;
+		}
+	}
 }
diff --git a/MyTracks/src/com/google/android/apps/mytracks/MyTracksList.java b/MyTracks/src/com/google/android/apps/mytracks/MyTracksList.java
index ad9eefbc8..6fe18df2a 100644
--- a/MyTracks/src/com/google/android/apps/mytracks/MyTracksList.java
+++ b/MyTracks/src/com/google/android/apps/mytracks/MyTracksList.java
@@ -15,6 +15,11 @@
  */
 package com.google.android.apps.mytracks;
 
+import com.google.android.apps.mytracks.content.TracksColumns;
+import com.google.android.apps.mytracks.util.StringUtils;
+import com.google.android.apps.mytracks.util.UnitConversions;
+import com.google.android.maps.mytracks.R;
+
 import android.app.ListActivity;
 import android.content.Intent;
 import android.content.SharedPreferences;
@@ -22,22 +27,17 @@ import android.database.Cursor;
 import android.os.Bundle;
 import android.os.Handler;
 import android.view.ContextMenu;
+import android.view.ContextMenu.ContextMenuInfo;
 import android.view.MenuItem;
 import android.view.SubMenu;
 import android.view.View;
-import android.view.Window;
-import android.view.ContextMenu.ContextMenuInfo;
 import android.view.View.OnCreateContextMenuListener;
+import android.view.Window;
 import android.widget.AdapterView;
 import android.widget.ListView;
 import android.widget.SimpleCursorAdapter;
 import android.widget.TextView;
 
-import com.google.android.apps.mytracks.content.TracksColumns;
-import com.google.android.apps.mytracks.util.StringUtils;
-import com.google.android.apps.mytracks.util.UnitConversions;
-import com.google.android.maps.mytracks.R;
-
 /**
  * A list activity displaying all the recorded tracks. There's a context
  * menu (via long press) displaying various options such as showing, editing,
diff --git a/MyTracks/src/com/google/android/apps/mytracks/util/MyTracksUtils.java b/MyTracks/src/com/google/android/apps/mytracks/util/MyTracksUtils.java
index 4d6da6786..0a7e0d5b0 100644
--- a/MyTracks/src/com/google/android/apps/mytracks/util/MyTracksUtils.java
+++ b/MyTracks/src/com/google/android/apps/mytracks/util/MyTracksUtils.java
@@ -21,12 +21,15 @@ import com.google.android.apps.mytracks.stats.TripStatistics;
 
 import com.google.android.maps.GeoPoint;
 
+import android.app.Activity;
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager;
 import android.content.pm.PackageManager.NameNotFoundException;
 import android.content.pm.Signature;
 import android.location.Location;
+import android.os.PowerManager;
+import android.os.PowerManager.WakeLock;
 import android.util.Log;
 
 import java.util.ArrayList;
@@ -286,6 +289,42 @@ public class MyTracksUtils {
       return "";
     }
   }
+  
+  /**
+   * Tries to acquire a partial wake lock if not already acquired. Logs errors
+   * and gives up trying in case the wake lock cannot be acquired.
+   */
+	public static WakeLock acquireWakeLock(Activity activity, WakeLock wakeLock) {
+		Log.i(MyTracksConstants.TAG, "MyTracksUtils: Acquiring wake lock.");
+		try {
+			PowerManager pm = (PowerManager) activity
+					.getSystemService(Context.POWER_SERVICE);
+			if (pm == null) {
+				Log.e(MyTracksConstants.TAG, "MyTracksUtils: Power manager not found!");
+				return wakeLock;
+			}
+			if (wakeLock == null) {
+				wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK,
+						MyTracksConstants.TAG);
+				if (wakeLock == null) {
+					Log.e(MyTracksConstants.TAG,
+							"MyTracksUtils: Could not create wake lock (null).");
+				}
+				return wakeLock;
+			}
+			if (!wakeLock.isHeld()) {
+				wakeLock.acquire();
+				if (!wakeLock.isHeld()) {
+					Log.e(MyTracksConstants.TAG,
+							"MyTracksUtils: Could not acquire wake lock.");
+				}
+			}
+		} catch (RuntimeException e) {
+			Log.e(MyTracksConstants.TAG,
+					"MyTracksUtils: Caught unexpected exception: " + e.getMessage(), e);
+		}
+		return wakeLock;
+	}
 
   /**
    * This is a utility class w/ only static members.