From 7130547bef8719feac159e26359707f070348f27 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 2 Nov 2012 16:55:06 -0700 Subject: [PATCH 01/10] Wrote some helper functions to convert subtask trees between local and remote ids (could be useful for syncing) --- .../subtasks/AstridOrderedListUpdater.java | 35 ++++++--- .../astrid/subtasks/SubtasksHelper.java | 78 ++++++++++++++++++- 2 files changed, 99 insertions(+), 14 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java index d8cf84285..0974ccca1 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java @@ -31,7 +31,7 @@ public abstract class AstridOrderedListUpdater { } public static class Node { - public final long taskId; + public long taskId; public Node parent; public int indent; public final ArrayList children = new ArrayList(); @@ -63,7 +63,12 @@ public abstract class AstridOrderedListUpdater { } public void initializeFromSerializedTree(LIST list, Filter filter, String serializedTree) { - treeRoot = buildTreeModel(serializedTree); + treeRoot = buildTreeModel(serializedTree, new JSONTreeModelBuilder() { + @Override + public void afterAddNode(Node node) { + idToNode.put(node.taskId, node); + } + }); verifyTreeModel(list, filter); } @@ -303,44 +308,54 @@ public abstract class AstridOrderedListUpdater { applyToFilter(filter); } - private Node buildTreeModel(String serializedTree) { + private interface JSONTreeModelBuilder { + void afterAddNode(Node node); + } + + public static Node buildTreeModel(String serializedTree, JSONTreeModelBuilder callback) { Node root = new Node(-1, null, -1); try { JSONArray tree = new JSONArray(serializedTree); - recursivelyBuildChildren(root, tree); + recursivelyBuildChildren(root, tree, callback); } catch (JSONException e) { Log.e("OrderedListUpdater", "Error building tree model", e); //$NON-NLS-1$//$NON-NLS-2$ } return root; } - private void recursivelyBuildChildren(Node node, JSONArray children) throws JSONException { + private static void recursivelyBuildChildren(Node node, JSONArray children, JSONTreeModelBuilder callback) throws JSONException { for (int i = 1; i < children.length(); i++) { JSONArray subarray = children.optJSONArray(i); if (subarray == null) { Long id = children.getLong(i); Node child = new Node(id, node, node.indent + 1); node.children.add(child); - idToNode.put(id, child); + if (callback != null) + callback.afterAddNode(child); } else { Long id = subarray.getLong(0); Node child = new Node(id, node, node.indent + 1); - recursivelyBuildChildren(child, subarray); + recursivelyBuildChildren(child, subarray, callback); node.children.add(child); - idToNode.put(id, child); + if (callback != null) + callback.afterAddNode(child); } } } protected String serializeTree() { + return serializeTree(treeRoot); + } + + public static String serializeTree(Node root) { JSONArray tree = new JSONArray(); - if (treeRoot == null) { + if (root == null) { return tree.toString(); } try { tree.put(-1L); - recursivelySerialize(treeRoot, tree); + recursivelySerialize(root, tree); } catch (JSONException e) { Log.e("OrderedListUpdater", "Error serializing tree model", e); //$NON-NLS-1$//$NON-NLS-2$ } diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java index ba5c063cc..fbff4cc2c 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java @@ -1,13 +1,16 @@ package com.todoroo.astrid.subtasks; import java.util.ArrayList; +import java.util.HashMap; import android.content.SharedPreferences; import android.text.TextUtils; import android.util.Log; +import com.todoroo.andlib.data.TodorooCursor; import com.todoroo.andlib.service.ContextManager; import com.todoroo.andlib.sql.Criterion; +import com.todoroo.andlib.sql.Query; import com.todoroo.andlib.utility.Preferences; import com.todoroo.astrid.actfm.TagViewFragment; import com.todoroo.astrid.api.Filter; @@ -18,6 +21,7 @@ import com.todoroo.astrid.core.SortHelper; import com.todoroo.astrid.dao.TaskDao.TaskCriteria; import com.todoroo.astrid.data.TagData; import com.todoroo.astrid.data.Task; +import com.todoroo.astrid.subtasks.AstridOrderedListUpdater.Node; import com.todoroo.astrid.utility.AstridPreferences; public class SubtasksHelper { @@ -75,12 +79,11 @@ public class SubtasksHelper { else serialized = Preferences.getStringValue(SubtasksUpdater.ACTIVE_TASKS_ORDER); - ArrayList ids = getIdArray(serialized); - return AstridOrderedListUpdater.buildOrderString(ids.toArray(new Long[ids.size()])); + return AstridOrderedListUpdater.buildOrderString(getIdArray(serialized)); } @SuppressWarnings("nls") - private static ArrayList getIdArray(String serializedTree) { + public static Long[] getIdArray(String serializedTree) { ArrayList ids = new ArrayList(); String[] digitsOnly = serializedTree.split("\\D+"); for (String idString : digitsOnly) { @@ -91,7 +94,74 @@ public class SubtasksHelper { Log.e("widget-subtasks", "error parsing id " + idString, e); } } - return ids; + return ids.toArray(new Long[ids.size()]); + } + + public static String convertTreeToRemoteIds(String localTree) { + return convertIdTree(localTree, true); + } + + public static String convertTreeToLocalIds(String remoteTree) { + return convertIdTree(remoteTree, false); + } + + private static String convertIdTree(String treeString, boolean localToRemote) { + Long[] ids = getIdArray(treeString); + HashMap idMap = buildIdMap(ids, localToRemote); + + Node tree = AstridOrderedListUpdater.buildTreeModel(treeString, null); + remapTree(tree, idMap); + return AstridOrderedListUpdater.serializeTree(tree); + } + + private static void remapTree(Node root, HashMap idMap) { + ArrayList children = root.children; + for (int i = 0; i < children.size(); i++) { + Node child = children.get(i); + Long remoteId = idMap.get(child.taskId); + if (remoteId == null || remoteId <= 0) { + children.remove(i); + i--; + } else { + child.taskId = remoteId; + remapTree(child, idMap); + } + } + } + + private static HashMap buildIdMap(Long[] localIds, boolean localToRemote) { // If localToRemote is true, keys are local ids. If false. keys are remtoe ids + HashMap map = new HashMap(); + Criterion criterion; + if (localToRemote) + criterion = Task.ID.in(localIds); + else + criterion = Task.REMOTE_ID.in(localIds); + + TodorooCursor tasks = PluginServices.getTaskService().query(Query.select(Task.ID, Task.REMOTE_ID).where(criterion)); + try { + Task t = new Task(); + for (tasks.moveToFirst(); !tasks.isAfterLast(); tasks.moveToNext()) { + t.clear(); + t.readFromCursor(tasks); + + if (t.containsNonNullValue(Task.REMOTE_ID)) { + Long key; + Long value; + if (localToRemote) { + key = t.getId(); + value = t.getValue(Task.REMOTE_ID); + } else { + key = t.getValue(Task.REMOTE_ID); + value = t.getId(); + } + + map.put(key, value); + } + } + } finally { + tasks.close(); + } + return map; } } From 60ba37662d8e36c0110c12796b0250ff14eade60 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 2 Nov 2012 17:30:18 -0700 Subject: [PATCH 02/10] Don't give up on the entire hierarchy when remapping tree ids--just deindent children if an id isn't found --- .../plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java | 1 + 1 file changed, 1 insertion(+) diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java index fbff4cc2c..e4530878c 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java @@ -121,6 +121,7 @@ public class SubtasksHelper { Long remoteId = idMap.get(child.taskId); if (remoteId == null || remoteId <= 0) { children.remove(i); + children.addAll(i, child.children); i--; } else { child.taskId = remoteId; From 3fb5c150c9f36d534111ebe366067f7c0be80f78 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Mon, 5 Nov 2012 12:42:45 -0800 Subject: [PATCH 03/10] Save ordering obtained from task_list result --- .../astrid/actfm/sync/ActFmSyncService.java | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java index a9eff7681..c8038dd5a 100644 --- a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java +++ b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java @@ -50,6 +50,7 @@ import com.todoroo.andlib.utility.AndroidUtilities; import com.todoroo.andlib.utility.DateUtilities; import com.todoroo.andlib.utility.Preferences; import com.todoroo.astrid.billing.BillingConstants; +import com.todoroo.astrid.core.PluginServices; import com.todoroo.astrid.dao.MetadataDao; import com.todoroo.astrid.dao.TagDataDao; import com.todoroo.astrid.dao.TaskDao; @@ -75,6 +76,7 @@ import com.todoroo.astrid.service.StatisticsService; import com.todoroo.astrid.service.TagDataService; import com.todoroo.astrid.service.TaskService; import com.todoroo.astrid.service.abtesting.ABTestEventReportingService; +import com.todoroo.astrid.subtasks.SubtasksUpdater; import com.todoroo.astrid.sync.SyncV2Provider.SyncExceptionHandler; import com.todoroo.astrid.tags.TagService; import com.todoroo.astrid.tags.reusable.FeaturedListFilterExposer; @@ -937,7 +939,12 @@ public final class ActFmSyncService { * @param done */ public void fetchActiveTasks(final boolean manual, SyncExceptionHandler handler, Runnable done) { - invokeFetchList("task", manual, handler, new TaskListItemProcessor(manual), done, "active_tasks"); + invokeFetchList("task", manual, handler, new TaskListItemProcessor(manual) { + @Override + protected void saveOrdering(JSONArray ordering) { + Preferences.setString(SubtasksUpdater.ACTIVE_TASKS_ORDER, ordering.toString()); + } + }, done, "active_tasks"); } /** @@ -956,6 +963,12 @@ public final class ActFmSyncService { Task.REMOTE_ID.isNotNull(), Criterion.not(Task.ID.in(localIds)))); } + + @Override + protected void saveOrdering(JSONArray ordering) { + tagData.setValue(TagData.TAG_ORDERING, ordering.toString()); + PluginServices.getTagDataService().save(tagData); + } }, done, "tasks:" + tagData.getId(), "tag_id", tagData.getValue(TagData.REMOTE_ID)); } @@ -1195,6 +1208,10 @@ public final class ActFmSyncService { } } + public void processExtras(JSONObject fullResult) { + // Subclasses can override if they want to examine the full JSONObject for other information + } + protected void readRemoteIds(JSONArray list) throws JSONException { remoteIds = new Long[list.length()]; for(int i = 0; i < list.length(); i++) @@ -1229,6 +1246,7 @@ public final class ActFmSyncService { } } + } private class TaskListItemProcessor extends ListItemProcessor { @@ -1241,6 +1259,23 @@ public final class ActFmSyncService { this.modificationDates = new HashMap(); } + protected void saveOrdering(JSONArray ordering) { + // Subclasses should override + } + + @Override + public void processExtras(JSONObject fullResult) { + if (!fullResult.has("ordering")) + return; + + try { + JSONArray ordering = fullResult.getJSONArray("ordering"); + saveOrdering(ordering); + } catch (JSONException e) { + Log.e("sync-ordering", "Error getting ordering from result " + fullResult , e); + } + } + @Override protected void mergeAndSave(JSONArray list, HashMap locals, long serverTime) throws JSONException { Task remote = new Task(); @@ -1421,6 +1456,7 @@ public final class ActFmSyncService { long serverTime = result.optLong("time", 0); JSONArray list = result.getJSONArray("list"); processor.process(list, serverTime); + processor.processExtras(result); Preferences.setLong("actfm_time_" + lastSyncKey, serverTime); Preferences.setLong("actfm_last_" + lastSyncKey, DateUtilities.now()); From cbd2d92c0e8acea59877e51164a93528b475dbd7 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Mon, 5 Nov 2012 12:44:26 -0800 Subject: [PATCH 04/10] Convert to local ordering before saving during sync --- .../com/todoroo/astrid/actfm/sync/ActFmSyncService.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java index c8038dd5a..e9c507f3e 100644 --- a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java +++ b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java @@ -76,6 +76,7 @@ import com.todoroo.astrid.service.StatisticsService; import com.todoroo.astrid.service.TagDataService; import com.todoroo.astrid.service.TaskService; import com.todoroo.astrid.service.abtesting.ABTestEventReportingService; +import com.todoroo.astrid.subtasks.SubtasksHelper; import com.todoroo.astrid.subtasks.SubtasksUpdater; import com.todoroo.astrid.sync.SyncV2Provider.SyncExceptionHandler; import com.todoroo.astrid.tags.TagService; @@ -942,7 +943,8 @@ public final class ActFmSyncService { invokeFetchList("task", manual, handler, new TaskListItemProcessor(manual) { @Override protected void saveOrdering(JSONArray ordering) { - Preferences.setString(SubtasksUpdater.ACTIVE_TASKS_ORDER, ordering.toString()); + String localOrdering = SubtasksHelper.convertTreeToLocalIds(ordering.toString()); + Preferences.setString(SubtasksUpdater.ACTIVE_TASKS_ORDER, localOrdering); } }, done, "active_tasks"); } @@ -966,7 +968,8 @@ public final class ActFmSyncService { @Override protected void saveOrdering(JSONArray ordering) { - tagData.setValue(TagData.TAG_ORDERING, ordering.toString()); + String localOrdering = SubtasksHelper.convertTreeToLocalIds(ordering.toString()); + tagData.setValue(TagData.TAG_ORDERING, localOrdering); PluginServices.getTagDataService().save(tagData); } }, done, "tasks:" + tagData.getId(), "tag_id", tagData.getValue(TagData.REMOTE_ID)); From a0d7ac427cdf814db5edd44832a8293b853feff4 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Tue, 6 Nov 2012 11:01:13 -0800 Subject: [PATCH 05/10] Fix some issues and remove unecessary code --- .../astrid/subtasks/AstridOrderedListUpdater.java | 3 +-- .../astrid/subtasks/SubtasksMetadataMigration.java | 12 +----------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java index 0974ccca1..8312bb68d 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/AstridOrderedListUpdater.java @@ -354,7 +354,6 @@ public abstract class AstridOrderedListUpdater { } try { - tree.put(-1L); recursivelySerialize(root, tree); } catch (JSONException e) { Log.e("OrderedListUpdater", "Error serializing tree model", e); //$NON-NLS-1$//$NON-NLS-2$ @@ -362,7 +361,7 @@ public abstract class AstridOrderedListUpdater { return tree.toString(); } - public static void recursivelySerialize(Node node, JSONArray serializeTo) throws JSONException { + private static void recursivelySerialize(Node node, JSONArray serializeTo) throws JSONException { ArrayList children = node.children; serializeTo.put(node.taskId); for (Node child : children) { diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksMetadataMigration.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksMetadataMigration.java index ca363f8b4..38d750d68 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksMetadataMigration.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksMetadataMigration.java @@ -2,9 +2,6 @@ package com.todoroo.astrid.subtasks; import java.util.ArrayList; -import org.json.JSONArray; -import org.json.JSONException; - import android.util.Log; import com.todoroo.andlib.data.TodorooCursor; @@ -99,14 +96,7 @@ public class SubtasksMetadataMigration { Node newNode = new Node(item.getValue(Metadata.TASK), parent, parent.indent + 1); parent.children.add(newNode); } - - try { - JSONArray array = new JSONArray(); - AstridOrderedListUpdater.recursivelySerialize(root, array); - return array.toString(); - } catch (JSONException e) { - return "[]"; //$NON-NLS-1$ - } + return AstridOrderedListUpdater.serializeTree(root); } private Node findNextParentForIndent(Node root, int indent) { From 575bc4b135c60933c7b3d483078da31323618a14 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Tue, 6 Nov 2012 11:33:45 -0800 Subject: [PATCH 06/10] Added unit tests for subtasks helper functions --- .../astrid/subtasks/SubtasksHelper.java | 3 +- .../astrid/subtasks/SubtasksHelperTest.java | 59 +++++++++++++++++++ .../astrid/subtasks/SubtasksMovingTest.java | 8 +++ .../astrid/subtasks/SubtasksTestCase.java | 1 - 4 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 tests/src/com/todoroo/astrid/subtasks/SubtasksHelperTest.java diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java index e4530878c..99bc1ea49 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksHelper.java @@ -85,7 +85,7 @@ public class SubtasksHelper { @SuppressWarnings("nls") public static Long[] getIdArray(String serializedTree) { ArrayList ids = new ArrayList(); - String[] digitsOnly = serializedTree.split("\\D+"); + String[] digitsOnly = serializedTree.split("[\\[\\],\\s]"); // Split on [ ] , or whitespace chars for (String idString : digitsOnly) { try { if (!TextUtils.isEmpty(idString)) @@ -108,6 +108,7 @@ public class SubtasksHelper { private static String convertIdTree(String treeString, boolean localToRemote) { Long[] ids = getIdArray(treeString); HashMap idMap = buildIdMap(ids, localToRemote); + idMap.put(-1L, -1L); Node tree = AstridOrderedListUpdater.buildTreeModel(treeString, null); remapTree(tree, idMap); diff --git a/tests/src/com/todoroo/astrid/subtasks/SubtasksHelperTest.java b/tests/src/com/todoroo/astrid/subtasks/SubtasksHelperTest.java new file mode 100644 index 000000000..18c7b8aa4 --- /dev/null +++ b/tests/src/com/todoroo/astrid/subtasks/SubtasksHelperTest.java @@ -0,0 +1,59 @@ +package com.todoroo.astrid.subtasks; + +import com.todoroo.astrid.core.PluginServices; +import com.todoroo.astrid.data.Task; + +public class SubtasksHelperTest extends SubtasksTestCase { + + private Task A, B, C, D, E, F; + + @Override + protected void setUp() throws Exception { + super.setUp(); + createTasks(); + updater.initializeFromSerializedTree(null, filter, DEFAULT_SERIALIZED_TREE); + } + + private Task createTask(String title, long remoteId) { + Task t = new Task(); + t.setValue(Task.TITLE, title); + t.setValue(Task.REMOTE_ID, remoteId); + PluginServices.getTaskService().save(t); + return t; + } + + private void createTasks() { + A = createTask("A", 6); // Local id 1 + B = createTask("B", 4); // Local id 2 + C = createTask("C", 3); // Local id 3 + D = createTask("D", 1); // Local id 4 + E = createTask("E", 2); // Local id 5 + F = createTask("F", 5); // Local id 6 + } + + private static final Long[] EXPECTED_ORDER = {-1L, 1L, 2L, 3L, 4L, 5L, 6L }; + + public void testOrderedIdArray() { + Long[] ids = SubtasksHelper.getIdArray(DEFAULT_SERIALIZED_TREE); + assertEquals(EXPECTED_ORDER.length, ids.length); + for (int i = 0; i < EXPECTED_ORDER.length; i++) { + assertEquals(EXPECTED_ORDER[i], ids[i]); + } + } + + // Default order: "[-1, [1, 2, [3, 4]], 5, 6]" + + private static String EXPECTED_REMOTE = "[-1, [6, 4, [3, 1]], 2, 5]".replaceAll("\\s", ""); + public void testLocalToRemoteIdMapping() { + String mapped = SubtasksHelper.convertTreeToRemoteIds(DEFAULT_SERIALIZED_TREE).replaceAll("\\s", ""); + assertEquals(EXPECTED_REMOTE, mapped); + } + + + private static String EXPECTED_LOCAL = "[-1, [4, 5, [3, 2]], 6, 1]".replaceAll("\\s", ""); + public void testRemoteToLocalIdMapping() { + String mapped = SubtasksHelper.convertTreeToLocalIds(DEFAULT_SERIALIZED_TREE).replaceAll("\\s", ""); + assertEquals(EXPECTED_LOCAL, mapped); + } + +} diff --git a/tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java b/tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java index 897b05421..0aeb30978 100644 --- a/tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java +++ b/tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java @@ -12,6 +12,14 @@ public class SubtasksMovingTest extends SubtasksTestCase { super.setUp(); createTasks(); updater.initializeFromSerializedTree(null, filter, DEFAULT_SERIALIZED_TREE); + + // Assert initial state is correct + expectParentAndPosition(A, null, 0); + expectParentAndPosition(B, A, 0); + expectParentAndPosition(C, A, 1); + expectParentAndPosition(D, C, 0); + expectParentAndPosition(E, null, 1); + expectParentAndPosition(F, null, 2); } private void createTasks() { diff --git a/tests/src/com/todoroo/astrid/subtasks/SubtasksTestCase.java b/tests/src/com/todoroo/astrid/subtasks/SubtasksTestCase.java index bd0112b56..90bacfc1e 100644 --- a/tests/src/com/todoroo/astrid/subtasks/SubtasksTestCase.java +++ b/tests/src/com/todoroo/astrid/subtasks/SubtasksTestCase.java @@ -27,7 +27,6 @@ public class SubtasksTestCase extends DatabaseTestCase { * F */ public static final String DEFAULT_SERIALIZED_TREE = "[-1, [1, 2, [3, 4]], 5, 6]".replaceAll("\\s", ""); - //"[{\"1\":[{\"2\":[]}, {\"3\":[{\"4\":[]}]}]}, {\"5\":[]}, {\"6\":[]}]".replaceAll("\\s", ""); @Override protected void setUp() throws Exception { From d17ea10617aa0740652fa5aa2e6c33139f3d8869 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Tue, 6 Nov 2012 15:38:03 -0800 Subject: [PATCH 07/10] Make sure the server provided ordering has the root element before processing --- .../todoroo/astrid/actfm/sync/ActFmSyncService.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java index e9c507f3e..c048a2f0d 100644 --- a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java +++ b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java @@ -1268,11 +1268,18 @@ public final class ActFmSyncService { @Override public void processExtras(JSONObject fullResult) { - if (!fullResult.has("ordering")) + if (!fullResult.has("order")) return; try { - JSONArray ordering = fullResult.getJSONArray("ordering"); + JSONArray ordering = fullResult.getJSONArray("order"); + if (ordering.optLong(0) != -1L) { + JSONArray newOrdering = new JSONArray(); + newOrdering.put(-1L); + for (int i = 0; i < ordering.length(); i++) + newOrdering.put(ordering.get(i)); + ordering = newOrdering; + } saveOrdering(ordering); } catch (JSONException e) { Log.e("sync-ordering", "Error getting ordering from result " + fullResult , e); From 6e1352e3c944e80446225836547c0ce4798a86cf Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Tue, 6 Nov 2012 17:33:31 -0800 Subject: [PATCH 08/10] Naive syncing of list order is working --- .../com/todoroo/astrid/actfm/sync/ActFmSyncService.java | 5 +++++ .../com/todoroo/astrid/subtasks/SubtasksTagListFragment.java | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java index c048a2f0d..85795765e 100644 --- a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java +++ b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java @@ -619,6 +619,11 @@ public final class ActFmSyncService { params.add(silenced ? "1" : "0"); } + if (values.containsKey(TagData.TAG_ORDERING.name)) { + params.add("order"); + params.add(SubtasksHelper.convertTreeToRemoteIds(tagData.getValue(TagData.TAG_ORDERING))); + } + if(params.size() == 0 || !checkForToken()) return; diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksTagListFragment.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksTagListFragment.java index 81d585a09..adfef3ffc 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksTagListFragment.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksTagListFragment.java @@ -79,4 +79,9 @@ public class SubtasksTagListFragment extends TagViewFragment { return helper.createTaskAdapter(cursor, sqlQueryTemplate); } + @Override + protected void refresh() { + setUpTaskList(); + } + } From c6600c536f6089ac157caf5fe93f3b0634b76d4b Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Wed, 7 Nov 2012 11:26:44 -0800 Subject: [PATCH 09/10] Threadsafe version of syncing manual ordering, delay before sync in case many changes are occurring --- .../astrid/actfm/sync/ActFmSyncService.java | 74 +++++++++++++++++-- .../astrid/subtasks/SubtasksUpdater.java | 3 + 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java index 85795765e..e75926cc8 100644 --- a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java +++ b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java @@ -133,11 +133,17 @@ public final class ActFmSyncService { private final List failedPushes = Collections.synchronizedList(new LinkedList()); private Thread pushRetryThread = null; private Runnable pushRetryRunnable; + + private Thread pushTagOrder = null; + private Runnable pushTagOrderRunnable; + private final List tagOrderQueue = Collections.synchronizedList(new LinkedList()); + private final AtomicInteger taskPushThreads = new AtomicInteger(0); private final ConditionVariable waitUntilEmpty = new ConditionVariable(true); public void initialize() { initializeRetryRunnable(); + initializeTagOrderRunnable(); taskDao.addListener(new ModelUpdateListener() { @Override @@ -254,6 +260,35 @@ public final class ActFmSyncService { }; } + private static final long WAIT_BEFORE_PUSH_ORDER = 15 * 1000; + private void initializeTagOrderRunnable() { + pushTagOrderRunnable = new Runnable() { + @Override + public void run() { + while (true) { + if(tagOrderQueue.isEmpty()) { + synchronized(ActFmSyncService.this) { + pushTagOrder = null; + return; + } + } + if (tagOrderQueue.size() > 0) { + try { + AndroidUtilities.sleepDeep(WAIT_BEFORE_PUSH_ORDER); + Long tagDataId = tagOrderQueue.take(); + TagData td = tagDataService.fetchById(tagDataId, TagData.ID, TagData.REMOTE_ID, TagData.TAG_ORDERING); + if (td != null) { + pushTagOrdering(td); + } + } catch (InterruptedException e) { + continue; + } + } + } + } + }; + } + private void addFailedPush(FailedPush fp) { failedPushes.add(fp); synchronized(this) { @@ -552,6 +587,40 @@ public final class ActFmSyncService { pushUpdateOnSave(update, update.getMergedValues(), imageData); } + public void pushTagOrderingOnSave(long tagDataId) { + if (!tagOrderQueue.contains(tagDataId)) { + tagOrderQueue.offer(tagDataId); + synchronized(this) { + if(pushTagOrder == null) { + pushTagOrder = new Thread(pushTagOrderRunnable); + pushTagOrder.start(); + } + } + } + } + + private void pushTagOrdering(TagData tagData) { + if (!checkForToken()) + return; + + Long remoteId = tagData.getValue(TagData.REMOTE_ID); + if (remoteId == null || remoteId <= 0) + return; + + ArrayList params = new ArrayList(); + + params.add("id"); params.add(remoteId); + params.add("order"); + params.add(SubtasksHelper.convertTreeToRemoteIds(tagData.getValue(TagData.TAG_ORDERING))); + params.add("token"); params.add(token); + + try { + actFmInvoker.invoke("tag_save", params.toArray(new Object[params.size()])); + } catch (IOException e) { + handleException("push-tag-order", e); + } + } + /** * Send tagData changes to server * @param setValues @@ -619,11 +688,6 @@ public final class ActFmSyncService { params.add(silenced ? "1" : "0"); } - if (values.containsKey(TagData.TAG_ORDERING.name)) { - params.add("order"); - params.add(SubtasksHelper.convertTreeToRemoteIds(tagData.getValue(TagData.TAG_ORDERING))); - } - if(params.size() == 0 || !checkForToken()) return; diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java index 7cf2c523f..74d07ba63 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java @@ -8,6 +8,7 @@ package com.todoroo.astrid.subtasks; import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.sql.Criterion; import com.todoroo.andlib.utility.Preferences; +import com.todoroo.astrid.actfm.sync.ActFmSyncService; import com.todoroo.astrid.api.Filter; import com.todoroo.astrid.dao.TaskDao.TaskCriteria; import com.todoroo.astrid.data.TagData; @@ -19,6 +20,7 @@ public class SubtasksUpdater extends AstridOrderedListUpdater { @Autowired TagDataService tagDataService; @Autowired TaskService taskService; + @Autowired ActFmSyncService actFmSyncService; public static final String ACTIVE_TASKS_ORDER = "active_tasks_order"; //$NON-NLS-1$ @@ -64,6 +66,7 @@ public class SubtasksUpdater extends AstridOrderedListUpdater { } else { list.setValue(TagData.TAG_ORDERING, serialized); tagDataService.save(list); + actFmSyncService.pushTagOrderingOnSave(list.getId()); } } From 1cdf9ea8b034d0674f86afd9f3bcd7cff2aacbd6 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Wed, 7 Nov 2012 11:38:23 -0800 Subject: [PATCH 10/10] Fixed an error; push order queue doesn't need to be blocking --- .../astrid/actfm/sync/ActFmSyncService.java | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java index e75926cc8..0b9ca527d 100644 --- a/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java +++ b/astrid/plugin-src/com/todoroo/astrid/actfm/sync/ActFmSyncService.java @@ -273,15 +273,11 @@ public final class ActFmSyncService { } } if (tagOrderQueue.size() > 0) { - try { - AndroidUtilities.sleepDeep(WAIT_BEFORE_PUSH_ORDER); - Long tagDataId = tagOrderQueue.take(); - TagData td = tagDataService.fetchById(tagDataId, TagData.ID, TagData.REMOTE_ID, TagData.TAG_ORDERING); - if (td != null) { - pushTagOrdering(td); - } - } catch (InterruptedException e) { - continue; + AndroidUtilities.sleepDeep(WAIT_BEFORE_PUSH_ORDER); + Long tagDataId = tagOrderQueue.remove(0); + TagData td = tagDataService.fetchById(tagDataId, TagData.ID, TagData.REMOTE_ID, TagData.TAG_ORDERING); + if (td != null) { + pushTagOrdering(td); } } } @@ -589,7 +585,7 @@ public final class ActFmSyncService { public void pushTagOrderingOnSave(long tagDataId) { if (!tagOrderQueue.contains(tagDataId)) { - tagOrderQueue.offer(tagDataId); + tagOrderQueue.add(tagDataId); synchronized(this) { if(pushTagOrder == null) { pushTagOrder = new Thread(pushTagOrderRunnable);