From aebc69829872f1dc6fe67b746bf2849a4d33ef47 Mon Sep 17 00:00:00 2001 From: Tim Su Date: Fri, 5 Jun 2009 09:35:44 +0000 Subject: [PATCH] Tracked down and fixed a couple bugs with synchronization, handling IllegalStateException, added more error reporting via flurry. Ready for 2.8 release! --- AndroidManifest.xml | 4 +- res/values/strings.xml | 4 +- .../astrid/activities/SyncLoginActivity.java | 39 +++++++++++---- src/com/timsu/astrid/activities/TaskEdit.java | 2 +- .../astrid/activities/TaskListAdapter.java | 6 +-- .../activities/TaskListSubActivity.java | 8 ++-- .../astrid/data/task/TaskController.java | 18 ++++--- .../timsu/astrid/sync/RTMSyncProvider.java | 31 ++++++------ .../astrid/sync/SynchronizationProvider.java | 4 +- src/com/timsu/astrid/sync/Synchronizer.java | 48 ++++++++++++------- 10 files changed, 100 insertions(+), 64 deletions(-) diff --git a/AndroidManifest.xml b/AndroidManifest.xml index 0aee18ba0..d71700338 100644 --- a/AndroidManifest.xml +++ b/AndroidManifest.xml @@ -1,8 +1,8 @@ + android:versionCode="105" + android:versionName="2.8.0"> diff --git a/res/values/strings.xml b/res/values/strings.xml index cc22adc52..dc8c64337 100644 --- a/res/values/strings.xml +++ b/res/values/strings.xml @@ -299,7 +299,9 @@ occur (it is a minor drain on battery). Please Log In to RTM... -Sorry, there was an error verifying your login. Please let us know about this error: +Sorry, there was an error verifying your login. Please try again. +\n\n +Error Message: diff --git a/src/com/timsu/astrid/activities/SyncLoginActivity.java b/src/com/timsu/astrid/activities/SyncLoginActivity.java index 82ceed514..bf78c42ea 100644 --- a/src/com/timsu/astrid/activities/SyncLoginActivity.java +++ b/src/com/timsu/astrid/activities/SyncLoginActivity.java @@ -20,6 +20,7 @@ package com.timsu.astrid.activities; import android.app.Activity; +import android.content.DialogInterface; import android.os.Bundle; import android.os.Handler; import android.view.View; @@ -30,6 +31,7 @@ import android.widget.Button; import android.widget.TextView; import com.timsu.astrid.R; +import com.timsu.astrid.utilities.DialogUtilities; /** * This activity displays a WebView that allows users to log in to the @@ -61,9 +63,9 @@ public class SyncLoginActivity extends Activity { * Verifies whether the user's login attempt was successful. Will be * called off of the UI thread, use the handler to post messages. * - * @return true if activity should be dismissed, false otherwise + * @return error string, or null if sync was successful */ - public boolean verifyLogin(Handler handler); + public String verifyLogin(Handler handler); } private static SyncLoginCallback callback = null; @@ -84,7 +86,7 @@ public class SyncLoginActivity extends Activity { int labelParam = getIntent().getIntExtra(LABEL_TOKEN, 0); TextView label = (TextView)findViewById(R.id.login_label); - WebView webView = (WebView)findViewById(R.id.browser); + final WebView webView = (WebView)findViewById(R.id.browser); Button done = (Button)findViewById(R.id.done); Button cancel = (Button)findViewById(R.id.cancel); @@ -103,11 +105,12 @@ public class SyncLoginActivity extends Activity { webView.getSettings().setSupportZoom(true); webView.loadUrl(urlParam); - final Handler handler = new Handler(); done.setOnClickListener(new OnClickListener() { @Override public void onClick(View v) { - if(callback == null) { + final Handler handler = new Handler(); + + if(callback == null) { finish(); return; } @@ -115,11 +118,27 @@ public class SyncLoginActivity extends Activity { new Thread(new Runnable() { @Override public void run() { - boolean result = callback.verifyLogin(handler); - if(result) { - TaskList.synchronizeNow = true; - } - finish(); + final String result = callback.verifyLogin(handler); + + webView.destroy(); + if(result == null) { + TaskList.synchronizeNow = true; + finish(); + } else { + // display the error + handler.post(new Runnable() { + @Override + public void run() { + DialogUtilities.okDialog(SyncLoginActivity.this, result, + new DialogInterface.OnClickListener() { + public void onClick(DialogInterface arg0, int arg1) { + TaskListSubActivity.shouldRefreshTaskList = true; + finish(); + } + }); + } + }); + } } }).start(); } diff --git a/src/com/timsu/astrid/activities/TaskEdit.java b/src/com/timsu/astrid/activities/TaskEdit.java index b4822bf61..0725857a9 100644 --- a/src/com/timsu/astrid/activities/TaskEdit.java +++ b/src/com/timsu/astrid/activities/TaskEdit.java @@ -286,7 +286,7 @@ public class TaskEdit extends TaskModificationTabbedActivity { try { // write out to database - controller.saveTask(model); + controller.saveTask(model, false); saveTags(); saveAlerts(); Notifications.updateAlarm(this, controller, alertController, model); diff --git a/src/com/timsu/astrid/activities/TaskListAdapter.java b/src/com/timsu/astrid/activities/TaskListAdapter.java index cb34fbd2c..cc4a85b6b 100644 --- a/src/com/timsu/astrid/activities/TaskListAdapter.java +++ b/src/com/timsu/astrid/activities/TaskListAdapter.java @@ -618,7 +618,7 @@ public class TaskListAdapter extends ArrayAdapter { Importance i = Importance.values()[keyCode - KeyEvent.KEYCODE_1]; TaskModelForList task = (TaskModelForList)v.getTag(); task.setImportance(i); - hooks.taskController().saveTask(task); + hooks.taskController().saveTask(task, false); setFieldContentsAndVisibility(v, task); return true; } @@ -683,7 +683,7 @@ public class TaskListAdapter extends ArrayAdapter { private void setTaskProgress(final TaskModelForList task, View view, int progress) { final ImageView timer = ((ImageView)view.findViewById(R.id.imageLeft)); task.setProgressPercentage(progress); - hooks.taskController().saveTask(task); + hooks.taskController().saveTask(task, false); // show this task as completed even if it has repeats if(progress == 100) { @@ -704,7 +704,7 @@ public class TaskListAdapter extends ArrayAdapter { new DialogInterface.OnClickListener() { public void onClick(DialogInterface dialog, int which) { task.stopTimerAndUpdateElapsedTime(); - hooks.taskController().saveTask(task); + hooks.taskController().saveTask(task, false); timer.setVisibility(View.GONE); } }) diff --git a/src/com/timsu/astrid/activities/TaskListSubActivity.java b/src/com/timsu/astrid/activities/TaskListSubActivity.java index 867f1c398..538a30a49 100644 --- a/src/com/timsu/astrid/activities/TaskListSubActivity.java +++ b/src/com/timsu/astrid/activities/TaskListSubActivity.java @@ -823,7 +823,6 @@ public class TaskListSubActivity extends SubActivity { private void reloadList() { if (context.loadingThread != null && context.loadingThread.isAlive()) { context.loadingThread.interrupt(); - context.loadingThread.stop(); } context.loadingThread = new Thread(reLoadRunnable); context.loadingThread.start(); @@ -971,7 +970,7 @@ public class TaskListSubActivity extends SubActivity { FlurryAgent.onEvent("stop-timer"); task.stopTimerAndUpdateElapsedTime(); } - getTaskController().saveTask(task); + getTaskController().saveTask(task, false); context.listAdapter.refreshItem(listView, context.taskArray .indexOf(task)); } @@ -1095,7 +1094,7 @@ public class TaskListSubActivity extends SubActivity { } task.setPostponeCount(postponeCount); - getTaskController().saveTask(task); + getTaskController().saveTask(task, false); getTaskController().updateAlarmForTask( task.getTaskIdentifier()); context.listAdapter.refreshItem(listView, @@ -1120,8 +1119,7 @@ public class TaskListSubActivity extends SubActivity { showTagsView(); return true; case SYNC_ID: - onActivityResult(ACTIVITY_SYNCHRONIZE, - Constants.RESULT_SYNCHRONIZE, null); + synchronize(); return true; case MORE_ID: layout.showContextMenu(); diff --git a/src/com/timsu/astrid/data/task/TaskController.java b/src/com/timsu/astrid/data/task/TaskController.java index 742978e6a..df76d728f 100644 --- a/src/com/timsu/astrid/data/task/TaskController.java +++ b/src/com/timsu/astrid/data/task/TaskController.java @@ -221,8 +221,11 @@ public class TaskController extends AbstractController { return database.delete(TASK_TABLE_NAME, KEY_ROWID + "=" + id, null) > 0; } - /** Saves the given task to the database. Returns true on success. */ - public boolean saveTask(AbstractTaskModel task) { + /** Saves the given task to the database. Returns true on success. + * + * @param duringSync set to true when save is part of a synchronize + */ + public boolean saveTask(AbstractTaskModel task, boolean duringSync) { boolean saveSucessful; if(task.getTaskIdentifier() == null) { @@ -238,7 +241,7 @@ public class TaskController extends AbstractController { if(values.size() == 0) // nothing changed return true; - onTaskSave(task, values); + onTaskSave(task, values, duringSync); saveSucessful = database.update(TASK_TABLE_NAME, values, KEY_ROWID + "=" + id, null) > 0; @@ -247,7 +250,7 @@ public class TaskController extends AbstractController { if(values.containsKey(AbstractTaskModel.PROGRESS_PERCENTAGE) && values.getAsInteger(AbstractTaskModel.PROGRESS_PERCENTAGE) == AbstractTaskModel.COMPLETE_PERCENTAGE) { - onTaskCompleted(task, values); + onTaskCompleted(task, values, duringSync); } if(!(task instanceof TaskModelForSync)) { @@ -267,7 +270,7 @@ public class TaskController extends AbstractController { * @param task * @param values */ - private void onTaskSave(AbstractTaskModel task, ContentValues values) { + private void onTaskSave(AbstractTaskModel task, ContentValues values, boolean duringSync) { // save task completed date if(values.containsKey(AbstractTaskModel.PROGRESS_PERCENTAGE) && values.getAsInteger(AbstractTaskModel.PROGRESS_PERCENTAGE) @@ -330,7 +333,7 @@ public class TaskController extends AbstractController { * @param task task to process * @param values mutable map of values to save */ - private void onTaskCompleted(AbstractTaskModel task, ContentValues values) { + private void onTaskCompleted(AbstractTaskModel task, ContentValues values, boolean duringSync) { Cursor cursor = fetchTaskCursor(task.getTaskIdentifier(), TaskModelForHandlers.FIELD_LIST); TaskModelForHandlers model = new TaskModelForHandlers(cursor, values); @@ -344,7 +347,8 @@ public class TaskController extends AbstractController { } // handle sync-on-complete - if((model.getFlags() & TaskModelForHandlers.FLAG_SYNC_ON_COMPLETE) > 0) { + if((model.getFlags() & TaskModelForHandlers.FLAG_SYNC_ON_COMPLETE) > 0 && + !duringSync) { Synchronizer synchronizer = new Synchronizer(model.getTaskIdentifier()); synchronizer.synchronize(context, new SynchronizerListener() { public void onSynchronizerFinished(int numServicesSynced) { diff --git a/src/com/timsu/astrid/sync/RTMSyncProvider.java b/src/com/timsu/astrid/sync/RTMSyncProvider.java index b4234b1b3..35c262ff6 100644 --- a/src/com/timsu/astrid/sync/RTMSyncProvider.java +++ b/src/com/timsu/astrid/sync/RTMSyncProvider.java @@ -127,46 +127,40 @@ public class RTMSyncProvider extends SynchronizationProvider { } } - // open up a dialog and have the user go to browser if(isBackgroundService()) return; + // open up a dialog and have the user go to browser FlurryAgent.onEvent("rtm-login-dialog"); rtmService = new ServiceImpl(new ApplicationInfo( apiKey, sharedSecret, appName)); final String url = rtmService.beginAuthorization(Perms.delete); - progressDialog.dismiss(); + if(progressDialog != null) + progressDialog.dismiss(); Intent intent = new Intent(context, SyncLoginActivity.class); SyncLoginActivity.setCallback(new SyncLoginCallback() { @Override - public boolean verifyLogin(final Handler syncLoginHandler) { + public String verifyLogin(final Handler syncLoginHandler) { if(rtmService == null) { Log.e("rtmsync", "Error: sync login activity displayed with no service!"); - return true; + return null; } try { String token = rtmService.completeAuthorization(); Log.w("astrid", "got RTM token: " + token); Preferences.setSyncRTMToken(context, token); - return true; + return null; } catch (final Exception e) { // didn't work FlurryAgent.onError("rtm-verify-login", AstridUtilities.throwableToString(e), SynchronizationProvider.class.getSimpleName()); - syncLoginHandler.post(new Runnable() { - @Override - public void run() { - DialogUtilities.okDialog(context, - r.getString(R.string.rtm_login_error) + - " " + e.getMessage(), null); - } - }); - - return false; + rtmService = null; + return r.getString(R.string.rtm_login_error) + + " " + e.getMessage(); } } }); @@ -304,6 +298,13 @@ public class RTMSyncProvider extends SynchronizationProvider { Preferences.setSyncRTMLastSync(context, syncTime); FlurryAgent.onEvent("rtm-sync-finished"); + } catch (IllegalStateException e) { + // occurs when application was closed + + FlurryAgent.onError("rtm-sync-caught", AstridUtilities.throwableToString(e), + SynchronizationProvider.class.getSimpleName()); + + Log.e("rtmsync", "Illegal State during Sync", e); } catch (Exception e) { FlurryAgent.onError("rtm-sync", AstridUtilities.throwableToString(e), diff --git a/src/com/timsu/astrid/sync/SynchronizationProvider.java b/src/com/timsu/astrid/sync/SynchronizationProvider.java index 66a29a2bf..f04c7455e 100644 --- a/src/com/timsu/astrid/sync/SynchronizationProvider.java +++ b/src/com/timsu/astrid/sync/SynchronizationProvider.java @@ -70,9 +70,9 @@ public abstract class SynchronizationProvider { */ void synchronizeService(final Context activity, Synchronizer caller) { this.synchronizer = caller; + this.syncHandler = caller.getHandler(); if(!isBackgroundService()) { - syncHandler = caller.getHandler(); syncHandler.post(new Runnable() { @Override public void run() { @@ -368,7 +368,7 @@ public abstract class SynchronizationProvider { // save the data remoteTask.writeToTaskModel(task); - taskController.saveTask(task); + taskController.saveTask(task, true); // save tags if(remoteTask.tags != null) { diff --git a/src/com/timsu/astrid/sync/Synchronizer.java b/src/com/timsu/astrid/sync/Synchronizer.java index f6056daa9..df14fe5d8 100644 --- a/src/com/timsu/astrid/sync/Synchronizer.java +++ b/src/com/timsu/astrid/sync/Synchronizer.java @@ -28,6 +28,7 @@ import android.os.Handler; import android.os.Looper; import android.util.Log; +import com.flurry.android.FlurryAgent; import com.timsu.astrid.activities.TaskListSubActivity; import com.timsu.astrid.data.AbstractController; import com.timsu.astrid.data.alerts.AlertController; @@ -35,6 +36,7 @@ import com.timsu.astrid.data.sync.SyncDataController; import com.timsu.astrid.data.tag.TagController; import com.timsu.astrid.data.task.TaskController; import com.timsu.astrid.data.task.TaskIdentifier; +import com.timsu.astrid.utilities.AstridUtilities; import com.timsu.astrid.utilities.Preferences; /** @@ -172,24 +174,34 @@ public class Synchronizer { /** Called to do the next step of synchronization. */ void continueSynchronization(Context context) { - ServiceWrapper serviceWrapper = - ServiceWrapper.values()[currentStep]; - currentStep++; - switch(serviceWrapper) { - case _FIRST_SERVICE: - continueSynchronization(context); - break; - case RTM: - if(serviceWrapper.isActivated(context)) { - servicesSynced++; - serviceWrapper.service.synchronizeService(context, this); - } else { - continueSynchronization(context); - } - break; - case _LAST_SERVICE: - finishSynchronization(context); - } + try { + if(currentStep > ServiceWrapper.values().length) + currentStep = ServiceWrapper.values().length - 1; + + ServiceWrapper serviceWrapper = + ServiceWrapper.values()[currentStep]; + currentStep++; + switch(serviceWrapper) { + case _FIRST_SERVICE: + continueSynchronization(context); + break; + case RTM: + if(serviceWrapper.isActivated(context)) { + servicesSynced++; + serviceWrapper.service.synchronizeService(context, this); + } else { + continueSynchronization(context); + } + break; + case _LAST_SERVICE: + finishSynchronization(context); + } + } catch (Exception e) { + Log.e("sync", "Error continuing synchronization", e); + FlurryAgent.onError("sync-continue", AstridUtilities.throwableToString(e), + SynchronizationProvider.class.getSimpleName()); + finishSynchronization(context); + } } /** Called at the end of sync. */