From e6f9e0bf445fee0f76e71f934641e01aad880a9f Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 15 Jul 2011 18:23:10 -0700 Subject: [PATCH 1/3] Improved synchronization issues with sync on save --- .../astrid/gtasks/GtasksFilterExposer.java | 2 +- .../astrid/gtasks/GtasksListActivity.java | 8 + .../astrid/gtasks/api/GtasksService.java | 44 ++++-- .../gtasks/sync/GtasksSyncOnSaveService.java | 147 ++++++++---------- 4 files changed, 111 insertions(+), 90 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksFilterExposer.java b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksFilterExposer.java index f99311701..c87829fde 100644 --- a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksFilterExposer.java +++ b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksFilterExposer.java @@ -64,7 +64,7 @@ public class GtasksFilterExposer extends BroadcastReceiver { MetadataCriteria.withKey(GtasksMetadata.METADATA_KEY), TaskCriteria.activeAndVisible(), GtasksMetadata.LIST_ID.eq(list.getValue(GtasksList.REMOTE_ID)))).orderBy( - Order.asc(Functions.cast(GtasksMetadata.ORDER, "LONG"))), //$NON-NLS-1$ + Order.asc(Functions.cast(GtasksMetadata.ORDER, "LONG"))).groupBy(Task.ID), //$NON-NLS-1$ values); filter.listingIcon = ((BitmapDrawable)context.getResources().getDrawable(R.drawable.gtasks_icon)).getBitmap(); filter.customTaskList = new ComponentName(ContextManager.getContext(), GtasksListActivity.class); diff --git a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java index faa0abcc4..40060343f 100644 --- a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java +++ b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java @@ -11,11 +11,16 @@ import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.utility.DialogUtilities; import com.todoroo.andlib.utility.Preferences; import com.todoroo.astrid.activity.DraggableTaskListActivity; +import com.todoroo.astrid.gtasks.sync.GtasksSyncOnSaveService; public class GtasksListActivity extends DraggableTaskListActivity { @Autowired private GtasksTaskListUpdater gtasksTaskListUpdater; + @Autowired private GtasksSyncOnSaveService gtasksSyncOnSaveService; + + @Autowired private GtasksMetadataService gtasksMetadataService; + @Override protected IntegerProperty getIndentProperty() { return GtasksMetadata.INDENT; @@ -43,6 +48,7 @@ public class GtasksListActivity extends DraggableTaskListActivity { long targetTaskId = taskAdapter.getItemId(from); long destinationTaskId = taskAdapter.getItemId(to); gtasksTaskListUpdater.moveTo(targetTaskId, destinationTaskId); + gtasksSyncOnSaveService.triggerMoveForMetadata(gtasksMetadataService.getTaskMetadata(targetTaskId)); loadTaskListContent(true); } }; @@ -52,6 +58,7 @@ public class GtasksListActivity extends DraggableTaskListActivity { public void swipeRight(int which) { long targetTaskId = taskAdapter.getItemId(which); gtasksTaskListUpdater.indent(targetTaskId, 1); + gtasksSyncOnSaveService.triggerMoveForMetadata(gtasksMetadataService.getTaskMetadata(targetTaskId)); loadTaskListContent(true); } @@ -59,6 +66,7 @@ public class GtasksListActivity extends DraggableTaskListActivity { public void swipeLeft(int which) { long targetTaskId = taskAdapter.getItemId(which); gtasksTaskListUpdater.indent(targetTaskId, -1); + gtasksSyncOnSaveService.triggerMoveForMetadata(gtasksMetadataService.getTaskMetadata(targetTaskId)); loadTaskListContent(true); } }; diff --git a/astrid/plugin-src/com/todoroo/astrid/gtasks/api/GtasksService.java b/astrid/plugin-src/com/todoroo/astrid/gtasks/api/GtasksService.java index 6890ebe31..d75e364c0 100644 --- a/astrid/plugin-src/com/todoroo/astrid/gtasks/api/GtasksService.java +++ b/astrid/plugin-src/com/todoroo/astrid/gtasks/api/GtasksService.java @@ -4,6 +4,7 @@ import java.io.IOException; import com.google.api.client.extensions.android2.AndroidHttp; import com.google.api.client.googleapis.auth.oauth2.draft10.GoogleAccessProtectedResource; +import com.google.api.client.http.HttpResponseException; import com.google.api.client.json.jackson.JacksonFactory; import com.google.api.services.tasks.v1.Tasks; import com.google.api.services.tasks.v1.Tasks.TasksOperations.Insert; @@ -12,6 +13,7 @@ import com.google.api.services.tasks.v1.Tasks.TasksOperations.Move; import com.google.api.services.tasks.v1.model.Task; import com.google.api.services.tasks.v1.model.TaskList; import com.google.api.services.tasks.v1.model.TaskLists; +import com.todoroo.astrid.gtasks.auth.GtasksTokenValidator; /** * Wrapper around the official Google Tasks API to simplify common operations. In the case @@ -22,28 +24,39 @@ import com.google.api.services.tasks.v1.model.TaskLists; @SuppressWarnings("nls") public class GtasksService { private Tasks service; + private GoogleAccessProtectedResource accessProtectedResource; + private String token; private static final String API_KEY = "AIzaSyCIYZTBo6haRHxmiplZsfYdagFEpaiFnAk"; // non-production API key public static final String AUTH_TOKEN_TYPE = "oauth2:https://www.googleapis.com/auth/tasks"; public GtasksService(String authToken) { - try { - authenticate(authToken); - } catch (Exception e) { - e.printStackTrace(); - } + authenticate(authToken); } - public void authenticate(String authToken) throws IOException { - - GoogleAccessProtectedResource accessProtectedResource = new GoogleAccessProtectedResource(authToken); + public void authenticate(String authToken) { + this.token = authToken; + accessProtectedResource = new GoogleAccessProtectedResource(authToken); service = new Tasks(AndroidHttp.newCompatibleTransport(), accessProtectedResource, new JacksonFactory()); service.accessKey = API_KEY; service.setApplicationName("Astrid"); } + //If we get a 401 or 403, try revalidating the auth token before bailing + private synchronized void handleException(IOException e) { + if (e instanceof HttpResponseException) { + HttpResponseException h = (HttpResponseException)e; + if (h.response.statusCode == 401 || h.response.statusCode == 403) { + token = GtasksTokenValidator.validateAuthToken(token); + if (token != null) { + accessProtectedResource.setAccessToken(token); + } + } + } + } + /** * A simple service query that will throw an exception if anything goes wrong. * Useful for checking if token needs revalidating or if there are network problems-- @@ -59,6 +72,7 @@ public class GtasksService { try { toReturn = service.tasklists.list().execute(); } catch (IOException e) { + handleException(e); toReturn = service.tasklists.list().execute(); } return toReturn; @@ -69,6 +83,7 @@ public class GtasksService { try { toReturn = service.tasklists.get(id).execute(); } catch (IOException e) { + handleException(e); toReturn = service.tasklists.get(id).execute(); } return toReturn; @@ -81,6 +96,7 @@ public class GtasksService { try { toReturn = service.tasklists.insert(newList).execute(); } catch (IOException e) { + handleException(e); toReturn = service.tasklists.insert(newList).execute(); } return toReturn; @@ -91,6 +107,7 @@ public class GtasksService { try { toReturn = service.tasklists.update(list.id, list).execute(); } catch (IOException e) { + handleException(e); toReturn = service.tasklists.update(list.id, list).execute(); } return toReturn; @@ -100,7 +117,8 @@ public class GtasksService { try { service.tasklists.delete(listId).execute(); } catch (IOException e) { - service.tasks.clear(listId).execute(); + handleException(e); + service.tasklists.delete(listId).execute(); } } @@ -109,6 +127,7 @@ public class GtasksService { try { toReturn = getAllGtasksFromListId(list.id, includeDeleted); } catch (IOException e) { + handleException(e); toReturn = getAllGtasksFromListId(list.id, includeDeleted); } return toReturn; @@ -121,6 +140,7 @@ public class GtasksService { try { toReturn = request.execute(); } catch (IOException e) { + handleException(e); toReturn = request.execute(); } return toReturn; @@ -131,6 +151,7 @@ public class GtasksService { try { toReturn = service.tasks.get(listId, taskId).execute(); } catch (IOException e) { + handleException(e); toReturn = service.tasks.get(listId, taskId).execute(); } return toReturn; @@ -158,6 +179,7 @@ public class GtasksService { try { toReturn = insertOp.execute(); } catch (IOException e) { + handleException(e); toReturn = insertOp.execute(); } return toReturn; @@ -168,6 +190,7 @@ public class GtasksService { try { toReturn = service.tasks.update(listId, task.id, task).execute(); } catch (IOException e) { + handleException(e); toReturn = service.tasks.update(listId, task.id, task).execute(); } return toReturn; @@ -182,6 +205,7 @@ public class GtasksService { try { toReturn = move.execute(); } catch (IOException e) { + handleException(e); toReturn = move.execute(); } return toReturn; @@ -191,6 +215,7 @@ public class GtasksService { try { service.tasks.delete(listId, taskId).execute(); } catch (IOException e) { + handleException(e); service.tasks.delete(listId, taskId).execute(); } } @@ -199,6 +224,7 @@ public class GtasksService { try { service.tasks.clear(listId).execute(); } catch (IOException e) { + handleException(e); service.tasks.clear(listId).execute(); } } diff --git a/astrid/plugin-src/com/todoroo/astrid/gtasks/sync/GtasksSyncOnSaveService.java b/astrid/plugin-src/com/todoroo/astrid/gtasks/sync/GtasksSyncOnSaveService.java index 6f4f239b8..056283ccb 100644 --- a/astrid/plugin-src/com/todoroo/astrid/gtasks/sync/GtasksSyncOnSaveService.java +++ b/astrid/plugin-src/com/todoroo/astrid/gtasks/sync/GtasksSyncOnSaveService.java @@ -1,7 +1,7 @@ package com.todoroo.astrid.gtasks.sync; import java.io.IOException; -import java.util.concurrent.Semaphore; +import java.util.concurrent.LinkedBlockingQueue; import android.content.ContentValues; import android.text.TextUtils; @@ -42,93 +42,70 @@ public final class GtasksSyncOnSaveService { DependencyInjectionService.getInstance().inject(this); } - private final Semaphore syncOnSaveSema = new Semaphore(1); + private final LinkedBlockingQueue operationQueue = new LinkedBlockingQueue(); + + private abstract class SyncOnSaveOperation {} + + private class TaskPushOp extends SyncOnSaveOperation { + protected Task model; + + public TaskPushOp(Task model) { + this.model = model; + } + } + + class MoveOp extends SyncOnSaveOperation { + protected Metadata metadata; + + public MoveOp(Metadata metadata) { + this.metadata = metadata; + } + } + public void initialize() { + new Thread(new Runnable() { + public void run() { + while (true) { + SyncOnSaveOperation op; + try { + op = operationQueue.take(); + } catch (InterruptedException e) { + continue; + } + try { + if (syncOnSaveEnabled() && !gtasksPreferenceService.isOngoing()) { + if (op instanceof TaskPushOp) { + TaskPushOp taskPush = (TaskPushOp)op; + pushTaskOnSave(taskPush.model, taskPush.model.getSetValues()); + } else if (op instanceof MoveOp) { + MoveOp move = (MoveOp)op; + pushMetadataOnSave(move.metadata); + } + } + } catch (IOException e){ + System.err.println("Sync on save failed"); //$NON-NLS-1$ + } + } + } + }).start(); + taskDao.addListener(new ModelUpdateListener() { public void onModelUpdated(final Task model) { if (!syncOnSaveEnabled()) return; if (gtasksPreferenceService.isOngoing()) //Don't try and sync changes that occur during a normal sync return; - if(Flags.checkAndClear(Flags.GTASKS_SUPPRESS_SYNC)) - return; final ContentValues setValues = model.getSetValues(); if(setValues == null || !checkForToken()) return; if (!checkValuesForProperties(setValues, TASK_PROPERTIES)) //None of the properties we sync were updated return; - - new Thread(new Runnable() { - @Override - public void run() { - // sleep so metadata associated with task is saved - AndroidUtilities.sleepDeep(1000L); - outer: - try { - try { - syncOnSaveSema.acquire(); - } catch (InterruptedException ingored) { - break outer; - } - pushTaskOnSave(model, setValues); - } catch (IOException e) { - e.printStackTrace(); - System.err.println("Sync on save failed"); //$NON-NLS-1$ - return; - } finally { - syncOnSaveSema.release(); - } - } - }).start(); - - } - });//*/ - - metadataDao.addListener(new ModelUpdateListener() { - public void onModelUpdated(final Metadata model) { - if (!syncOnSaveEnabled()) - return; - if (!model.getValue(Metadata.KEY).equals(GtasksMetadata.METADATA_KEY)) //Don't care about non-gtasks metadata - return; - if (gtasksPreferenceService.isOngoing()) //Don't try and sync changes that occur during a normal sync - return; - final ContentValues setValues = model.getSetValues(); - if (setValues == null || !checkForToken()) - return; - - if (checkValuesForProperties(setValues, METADATA_IGNORE_PROPERTIES)) // don't sync the move cases we don't handle - return; - if (!checkValuesForProperties(setValues, METADATA_PROPERTIES)) - return; - - if (Flags.checkAndClear(Flags.GTASKS_SUPPRESS_SYNC)) + if(Flags.checkAndClear(Flags.GTASKS_SUPPRESS_SYNC)) return; - new Thread(new Runnable() { - @Override - public void run() { - // sleep so metadata associated with task is saved - AndroidUtilities.sleepDeep(1000L); - outer: - try { - try { - syncOnSaveSema.acquire(); - } catch (InterruptedException ignored) { - break outer; - } - pushMetadataOnSave(model); - } catch (IOException e) { - e.printStackTrace(); - System.err.println("Sync on save failed"); //$NON-NLS-1$ - return; - } finally { - syncOnSaveSema.release(); - } - } - }).start(); + operationQueue.offer(new TaskPushOp((Task)model.clone())); } - }); } @@ -139,14 +116,6 @@ public final class GtasksSyncOnSaveService { Task.COMPLETION_DATE, Task.DELETION_DATE }; - private static final Property[] METADATA_PROPERTIES = - { GtasksMetadata.INDENT, - GtasksMetadata.PARENT_TASK }; - - private static final Property[] METADATA_IGNORE_PROPERTIES = - { GtasksMetadata.ORDER, - GtasksMetadata.LIST_ID }; - /** * Checks to see if any of the values changed are among the properties we sync * @param values @@ -162,10 +131,27 @@ public final class GtasksSyncOnSaveService { } + public void triggerMoveForMetadata(final Metadata metadata) { + if (!syncOnSaveEnabled()) + return; + if (!metadata.getValue(Metadata.KEY).equals(GtasksMetadata.METADATA_KEY)) //Don't care about non-gtasks metadata + return; + if (gtasksPreferenceService.isOngoing()) //Don't try and sync changes that occur during a normal sync + return; + if (!checkForToken()) + return; + if (Flags.checkAndClear(Flags.GTASKS_SUPPRESS_SYNC)) + return; + + operationQueue.offer(new MoveOp(metadata)); + } + /** * Synchronize with server when data changes */ private void pushTaskOnSave(Task task, ContentValues values) throws IOException { + AndroidUtilities.sleepDeep(1000L); //Wait for metadata to be saved + Metadata gtasksMetadata = gtasksMetadataService.getTaskMetadata(task.getId()); com.google.api.services.tasks.v1.model.Task remoteModel = null; boolean newlyCreated = false; @@ -276,6 +262,7 @@ public final class GtasksSyncOnSaveService { } private void pushMetadataOnSave(Metadata model) throws IOException { + AndroidUtilities.sleepDeep(1000L); //Initialize the gtasks api service String token = gtasksPreferenceService.getToken(); token = GtasksTokenValidator.validateAuthToken(token); From ae04a726a64f038b56dc8ae7691cc48d5112dae5 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 15 Jul 2011 19:44:43 -0700 Subject: [PATCH 2/3] Fixed the last known ordering bugs --- .../astrid/gtasks/GtasksListActivity.java | 6 +++++- .../astrid/gtasks/GtasksTaskListUpdater.java | 18 ++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java index 40060343f..8d951b973 100644 --- a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java +++ b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksListActivity.java @@ -47,7 +47,11 @@ public class GtasksListActivity extends DraggableTaskListActivity { public void drop(int from, int to) { long targetTaskId = taskAdapter.getItemId(from); long destinationTaskId = taskAdapter.getItemId(to); - gtasksTaskListUpdater.moveTo(targetTaskId, destinationTaskId); + + if(to == getListView().getCount() - 1) + gtasksTaskListUpdater.moveTo(targetTaskId, -1); + else + gtasksTaskListUpdater.moveTo(targetTaskId, destinationTaskId); gtasksSyncOnSaveService.triggerMoveForMetadata(gtasksMetadataService.getTaskMetadata(targetTaskId)); loadTaskListContent(true); } diff --git a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java index ffcc6f814..6dcdcaf69 100644 --- a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java +++ b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java @@ -70,7 +70,6 @@ public class GtasksTaskListUpdater { long newParent = computeNewParent(listRef.get(), taskId, indent + delta - 1); if (newParent == taskId) { - System.err.println("Tried to set parent to self"); metadata.setValue(GtasksMetadata.PARENT_TASK, Task.NO_ID); } else { metadata.setValue(GtasksMetadata.PARENT_TASK, newParent); @@ -131,7 +130,7 @@ public class GtasksTaskListUpdater { private static class Node { public final long taskId; - public final Node parent; + public Node parent; public final ArrayList children = new ArrayList(); public Node(long taskId, Node parent) { @@ -155,22 +154,26 @@ public class GtasksTaskListUpdater { return; Node root = buildTreeModel(list); + debugPrint(root, 0); Node target = findNode(root, targetTaskId); if(target != null && target.parent != null) { if(moveBeforeTaskId == -1) { target.parent.children.remove(target); root.children.add(target); + target.parent = root; } else { Node sibling = findNode(root, moveBeforeTaskId); if(sibling != null) { int index = sibling.parent.children.indexOf(sibling); target.parent.children.remove(target); sibling.parent.children.add(index, target); + target.parent = sibling.parent; } } } + debugPrint(root, 0); traverseTreeAndWriteValues(root, new AtomicLong(0), -1); } @@ -224,6 +227,8 @@ public class GtasksTaskListUpdater { Node node = currentNode.get().parent; for(int i = indent; i < previousIndentValue; i++) node = node.parent; + if(node == null) + node = root; currentNode.set(new Node(taskId, node)); node.children.add(currentNode.get()); } @@ -251,6 +256,15 @@ public class GtasksTaskListUpdater { }); } + public void debugPrint(Node root, int depth) { + for(int i = 0; i < depth; i++) System.err.print(" + "); + System.err.format("%03d", root.taskId); + System.err.print("\n"); + + for(int i = 0; i < root.children.size(); i++) + debugPrint(root.children.get(i), depth + 1); + } + private final Task taskContainer = new Task(); private void saveAndUpdateModifiedDate(Metadata metadata, long taskId) { From bc66634f720456ac92081c57aff54df68620fe10 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 15 Jul 2011 20:09:03 -0700 Subject: [PATCH 3/3] removed spurious prints --- .../com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java index 6dcdcaf69..a379433f9 100644 --- a/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java +++ b/astrid/plugin-src/com/todoroo/astrid/gtasks/GtasksTaskListUpdater.java @@ -154,7 +154,6 @@ public class GtasksTaskListUpdater { return; Node root = buildTreeModel(list); - debugPrint(root, 0); Node target = findNode(root, targetTaskId); if(target != null && target.parent != null) { @@ -173,7 +172,6 @@ public class GtasksTaskListUpdater { } } - debugPrint(root, 0); traverseTreeAndWriteValues(root, new AtomicLong(0), -1); }