Added a subtask moving test with some extra edge cases to test

astrid/plugin-src/com/todoroo/astrid/subtasks/OrderedListUpdater.java

@@ -73,6 +73,8 @@ abstract public class OrderedListUpdater<LIST> {
         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<LIST> {
                 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<LIST> {
         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;

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;
+
-
+        updater.applySubtasksToFilter(filter);
-        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;
-        }
 
         super.setUpTaskList();
 

astrid/plugin-src/com/todoroo/astrid/subtasks/SubtasksUpdater.java

@@ -77,4 +77,21 @@ public class SubtasksUpdater extends OrderedListUpdater<Long> {
             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;
+        }
+    }
 }

astrid/src/com/todoroo/astrid/dao/MetadataDao.java

@@ -74,7 +74,8 @@ public class MetadataDao extends DatabaseDao<Metadata> {
 
         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);
             }

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;
+    }
+
+}

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) {
+	protected void deleteDatabase(String toDelete) {
-	    File db = getContext().getDatabasePath(database);
+	    File db = getContext().getDatabasePath(toDelete);
 	    if(db.exists())
 	        db.delete();
     }