From b12ffda59c0daead9047ec0c1c5db3235ecebb69 Mon Sep 17 00:00:00 2001 From: Tim Su Date: Tue, 21 Feb 2012 17:42:09 -0800 Subject: [PATCH] Added a subtask moving test with some extra edge cases to test --- .../astrid/subtasks/OrderedListUpdater.java | 16 +- .../astrid/subtasks/SubtasksListFragment.java | 16 +- .../astrid/subtasks/SubtasksUpdater.java | 17 ++ .../com/todoroo/astrid/dao/MetadataDao.java | 3 +- .../astrid/subtasks/SubtasksMovingTest.java | 155 ++++++++++++++++++ .../todoroo/astrid/test/DatabaseTestCase.java | 6 +- 6 files changed, 194 insertions(+), 19 deletions(-) create mode 100644 tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/OrderedListUpdater.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/OrderedListUpdater.java index 45c2aa51e..2f28e5a51 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/OrderedListUpdater.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/OrderedListUpdater.java @@ -73,6 +73,8 @@ abstract public class OrderedListUpdater { iterateThroughList(filter, list, new OrderedListIterator() { @Override public void processTask(long taskId, Metadata metadata) { + if(!metadata.isSaved()) + metadata = createEmptyMetadata(list, taskId); int indent = metadata.getValue(indentProperty()); if(targetTaskId == taskId) { @@ -163,9 +165,13 @@ abstract public class OrderedListUpdater { target.parent = root; } else { Node sibling = findNode(root, moveBeforeTaskId); - if(sibling != null) { + if(sibling != null && !ancestorOf(target, sibling)) { int index = sibling.parent.children.indexOf(sibling); target.parent.children.remove(target); + + if(target.parent == sibling.parent && + target.parent.children.indexOf(target) < index) + index--; sibling.parent.children.add(index, target); target.parent = sibling.parent; } @@ -175,6 +181,14 @@ abstract public class OrderedListUpdater { traverseTreeAndWriteValues(list, root, new AtomicLong(0), -1); } + private boolean ancestorOf(Node ancestor, Node descendant) { + if(descendant.parent == ancestor) + return true; + if(descendant.parent == null) + return false; + return ancestorOf(ancestor, descendant.parent); + } + protected static class Node { public final long taskId; public Node parent; diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksListFragment.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksListFragment.java index 3b76b74f3..e5c92fa33 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksListFragment.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksListFragment.java @@ -18,7 +18,6 @@ import com.todoroo.andlib.data.Property; import com.todoroo.andlib.data.TodorooCursor; import com.todoroo.astrid.activity.TaskListFragment; import com.todoroo.astrid.adapter.TaskAdapter; -import com.todoroo.astrid.data.Metadata; import com.todoroo.astrid.data.Task; import com.todoroo.astrid.ui.DraggableListView; @@ -54,19 +53,8 @@ public class SubtasksListFragment extends TaskListFragment { @SuppressWarnings("nls") @Override protected void setUpTaskList() { - String query = filter.sqlQuery; - - String subtaskJoin = String.format("LEFT JOIN %s ON (%s = %s AND %s = '%s') ", - Metadata.TABLE, Task.ID, Metadata.TASK, - Metadata.KEY, SubtasksMetadata.METADATA_KEY); - if(!query.contains(subtaskJoin)) { - query = subtaskJoin + query; - query = query.replaceAll("ORDER BY .*", ""); - query = query + String.format(" ORDER BY CAST(%s AS LONG) ASC, %s ASC", - SubtasksMetadata.ORDER, Task.ID); - - filter.sqlQuery = query; - } + + updater.applySubtasksToFilter(filter); super.setUpTaskList(); diff --git a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java index 6d7ebfc5a..6366e565d 100644 --- a/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java +++ b/astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java @@ -77,4 +77,21 @@ public class SubtasksUpdater extends OrderedListUpdater { cursor.close(); } } + + @SuppressWarnings("nls") + public void applySubtasksToFilter(Filter filter) { + String query = filter.sqlQuery; + + String subtaskJoin = String.format("LEFT JOIN %s ON (%s = %s AND %s = '%s') ", + Metadata.TABLE, Task.ID, Metadata.TASK, + Metadata.KEY, SubtasksMetadata.METADATA_KEY); + if(!query.contains(subtaskJoin)) { + query = subtaskJoin + query; + query = query.replaceAll("ORDER BY .*", ""); + query = query + String.format(" ORDER BY CAST(%s AS LONG) ASC, %s ASC", + SubtasksMetadata.ORDER, Task.ID); + + filter.sqlQuery = query; + } + } } diff --git a/astrid/src/com/todoroo/astrid/dao/MetadataDao.java b/astrid/src/com/todoroo/astrid/dao/MetadataDao.java index 778187966..0fac40f1e 100644 --- a/astrid/src/com/todoroo/astrid/dao/MetadataDao.java +++ b/astrid/src/com/todoroo/astrid/dao/MetadataDao.java @@ -74,7 +74,8 @@ public class MetadataDao extends DatabaseDao { boolean state = super.persist(item); if(Preferences.getBoolean(AstridPreferences.P_FIRST_LIST, true)) { - if (state && item.getValue(Metadata.KEY).equals(TagService.KEY)) { + if (state && item.containsNonNullValue(Metadata.KEY) && + item.getValue(Metadata.KEY).equals(TagService.KEY)) { StatisticsService.reportEvent(StatisticsConstants.USER_FIRST_LIST); Preferences.setBoolean(AstridPreferences.P_FIRST_LIST, false); } diff --git a/tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java b/tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java new file mode 100644 index 000000000..33da02bfe --- /dev/null +++ b/tests/src/com/todoroo/astrid/subtasks/SubtasksMovingTest.java @@ -0,0 +1,155 @@ +package com.todoroo.astrid.subtasks; + +import com.todoroo.astrid.api.Filter; +import com.todoroo.astrid.core.CoreFilterExposer; +import com.todoroo.astrid.core.PluginServices; +import com.todoroo.astrid.data.Metadata; +import com.todoroo.astrid.data.Task; +import com.todoroo.astrid.test.DatabaseTestCase; + +@SuppressWarnings("nls") +public class SubtasksMovingTest extends DatabaseTestCase { + + private SubtasksUpdater updater; + + private Filter filter; + private Task A, B, C, D, E, F; + private final long list = SubtasksMetadata.LIST_ACTIVE_TASKS; + + /* Starting State: + * + * A + * B + * C + * D + * E + * F + */ + + public void testMoveBeforeIntoSelf() { + givenTasksABCDEF(); + + whenTriggerMoveBefore(A, B); + + /* + * A + * B + * C + * D + * E + */ + thenExpectMetadataOrderAndIndent(A, 0, 0); + thenExpectMetadataOrderAndIndent(B, 1, 1); + thenExpectMetadataOrderAndIndent(C, 2, 1); + thenExpectMetadataOrderAndIndent(D, 3, 2); + thenExpectMetadataOrderAndIndent(E, 4, 0); + } + + public void testMoveIntoChild() { + givenTasksABCDEF(); + + whenTriggerMoveBefore(A, C); + + /* + * A + * B + * C + * D + * E + */ + thenExpectMetadataOrderAndIndent(A, 0, 0); + thenExpectMetadataOrderAndIndent(B, 1, 1); + thenExpectMetadataOrderAndIndent(C, 2, 1); + thenExpectMetadataOrderAndIndent(D, 3, 2); + thenExpectMetadataOrderAndIndent(E, 4, 0); + } + + public void testMoveEndOfChildren() { + givenTasksABCDEF(); + + whenTriggerMoveBefore(A, E); + + /* + * A + * B + * C + * D + * E + */ + thenExpectMetadataOrderAndIndent(A, 0, 0); + thenExpectMetadataOrderAndIndent(B, 1, 1); + thenExpectMetadataOrderAndIndent(C, 2, 1); + thenExpectMetadataOrderAndIndent(D, 3, 2); + thenExpectMetadataOrderAndIndent(E, 4, 0); + } + + public void testMoveAfterChildren() { + givenTasksABCDEF(); + + whenTriggerMoveBefore(A, F); + + /* + * E + * A + * B + * C + * D + */ + thenExpectMetadataOrderAndIndent(E, 0, 0); + thenExpectMetadataOrderAndIndent(A, 1, 0); + thenExpectMetadataOrderAndIndent(B, 2, 1); + thenExpectMetadataOrderAndIndent(C, 3, 1); + thenExpectMetadataOrderAndIndent(D, 4, 2); + } + + // --- helpers + + /** moveTo = null => move to end */ + private void whenTriggerMoveBefore(Task target, Task moveTo) { + System.err.println("CAN I GET A WITNESS?"); + updater.debugPrint(filter, list); + updater.moveTo(filter, list, target.getId(), moveTo == null ? -1 : moveTo.getId()); + updater.debugPrint(filter, list); + } + + private void thenExpectMetadataOrderAndIndent(Task task, long order, int indent) { + Metadata metadata = updater.getTaskMetadata(list, task.getId()); + assertNotNull("metadata was found", metadata); + assertEquals("order", order, metadata.getValue(SubtasksMetadata.ORDER).longValue()); + assertEquals("indentation", indent, (int)metadata.getValue(SubtasksMetadata.INDENT)); + } + + @Override + protected void setUp() throws Exception { + super.setUp(); + + updater = new SubtasksUpdater(); + filter = CoreFilterExposer.buildInboxFilter(getContext().getResources()); + updater.applySubtasksToFilter(filter); + } + + private Task[] givenTasksABCDEF() { + Task[] tasks = new Task[] { + A = createTask("A", 0, 0), + B = createTask("B", 1, 1), + C = createTask("C", 2, 1), + D = createTask("D", 3, 2), + E = createTask("E", 4, 0), + F = createTask("F", 5, 0), + }; + + return tasks; + } + + private Task createTask(String title, long order, int indent) { + Task task = new Task(); + task.setValue(Task.TITLE, title); + PluginServices.getTaskService().save(task); + Metadata metadata = updater.createEmptyMetadata(list, task.getId()); + metadata.setValue(SubtasksMetadata.ORDER, order); + metadata.setValue(SubtasksMetadata.INDENT, indent); + PluginServices.getMetadataService().save(metadata); + return task; + } + +} diff --git a/tests/src/com/todoroo/astrid/test/DatabaseTestCase.java b/tests/src/com/todoroo/astrid/test/DatabaseTestCase.java index 989ffb67d..0607d315b 100644 --- a/tests/src/com/todoroo/astrid/test/DatabaseTestCase.java +++ b/tests/src/com/todoroo/astrid/test/DatabaseTestCase.java @@ -41,10 +41,10 @@ public class DatabaseTestCase extends TodorooTestCaseWithInjector { /** * Helper to delete a database by name - * @param database + * @param toDelete */ - protected void deleteDatabase(String database) { - File db = getContext().getDatabasePath(database); + protected void deleteDatabase(String toDelete) { + File db = getContext().getDatabasePath(toDelete); if(db.exists()) db.delete(); }