diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1053,6 +1053,7 @@ 'ManiphestTaskStatus' => 'applications/maniphest/constants/ManiphestTaskStatus.php', 'ManiphestTaskStatusDatasource' => 'applications/maniphest/typeahead/ManiphestTaskStatusDatasource.php', 'ManiphestTaskStatusTestCase' => 'applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php', + 'ManiphestTaskTestCase' => 'applications/maniphest/__tests__/ManiphestTaskTestCase.php', 'ManiphestTransaction' => 'applications/maniphest/storage/ManiphestTransaction.php', 'ManiphestTransactionComment' => 'applications/maniphest/storage/ManiphestTransactionComment.php', 'ManiphestTransactionEditor' => 'applications/maniphest/editor/ManiphestTransactionEditor.php', @@ -4292,6 +4293,7 @@ 'ManiphestTaskStatus' => 'ManiphestConstants', 'ManiphestTaskStatusDatasource' => 'PhabricatorTypeaheadDatasource', 'ManiphestTaskStatusTestCase' => 'PhabricatorTestCase', + 'ManiphestTaskTestCase' => 'PhabricatorTestCase', 'ManiphestTransaction' => 'PhabricatorApplicationTransaction', 'ManiphestTransactionComment' => 'PhabricatorApplicationTransactionComment', 'ManiphestTransactionEditor' => 'PhabricatorApplicationTransactionEditor', diff --git a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php new file mode 100644 --- /dev/null +++ b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php @@ -0,0 +1,174 @@ + true, + ); + } + + public function testTaskReordering() { + $viewer = $this->generateNewTestUser(); + + $t1 = $this->newTask($viewer, 'Task 1'); + $t2 = $this->newTask($viewer, 'Task 2'); + $t3 = $this->newTask($viewer, 'Task 3'); + + + // Default order should be reverse creation. + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(3, 2, 1), array_keys($tasks)); + + + // Move T3 to the middle. + $this->moveTask($viewer, $t3, $t2, true); + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(2, 3, 1), array_keys($tasks)); + + + // Move T3 to the end. + $this->moveTask($viewer, $t3, $t1, true); + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(2, 1, 3), array_keys($tasks)); + + + // Repeat the move above, there should be no overall change in order. + $this->moveTask($viewer, $t3, $t1, true); + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(2, 1, 3), array_keys($tasks)); + + + // Move T3 to the first slot in the priority. + $this->movePriority($viewer, $t3, $t3->getPriority(), false); + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(3, 2, 1), array_keys($tasks)); + + + // Move T3 to the last slot in the priority. + $this->movePriority($viewer, $t3, $t3->getPriority(), true); + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(2, 1, 3), array_keys($tasks)); + + + // Move T3 before T2. + $this->moveTask($viewer, $t3, $t2, false); + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(3, 2, 1), array_keys($tasks)); + + + // Move T3 before T1. + $this->moveTask($viewer, $t3, $t1, false); + $tasks = $this->loadTasks($viewer); + $t1 = $tasks[1]; + $t2 = $tasks[2]; + $t3 = $tasks[3]; + $this->assertEqual(array(2, 3, 1), array_keys($tasks)); + + } + + private function newTask(PhabricatorUser $viewer, $title) { + $task = ManiphestTask::initializeNewTask($viewer); + + $xactions = array(); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_TITLE) + ->setNewValue($title); + + + $this->applyTaskTransactions($viewer, $task, $xactions); + + return $task; + } + + private function loadTasks(PhabricatorUser $viewer) { + $tasks = id(new ManiphestTaskQuery()) + ->setViewer($viewer) + ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->execute(); + + $tasks = mpull($tasks, null, 'getID'); + + return $tasks; + } + + private function moveTask(PhabricatorUser $viewer, $src, $dst, $is_after) { + list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( + $dst, + $is_after); + + $xactions = array(); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) + ->setNewValue($pri); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) + ->setNewValue($sub); + + return $this->applyTaskTransactions($viewer, $src, $xactions); + } + + private function movePriority( + PhabricatorUser $viewer, + $src, + $target_priority, + $is_end) { + + list($pri, $sub) = ManiphestTransactionEditor::getEdgeSubpriority( + $target_priority, + $is_end); + + $xactions = array(); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) + ->setNewValue($pri); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) + ->setNewValue($sub); + + return $this->applyTaskTransactions($viewer, $src, $xactions); + } + + private function applyTaskTransactions( + PhabricatorUser $viewer, + ManiphestTask $task, + array $xactions) { + + $content_source = PhabricatorContentSource::newConsoleSource(); + + $editor = id(new ManiphestTransactionEditor()) + ->setActor($viewer) + ->setContentSource($content_source) + ->setContinueOnNoEffect(true) + ->applyTransactions($task, $xactions); + + return $task; + } + +} diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php --- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php +++ b/src/applications/maniphest/controller/ManiphestSubpriorityController.php @@ -32,21 +32,25 @@ if (!$after_task) { return new Aphront404Response(); } - $after_pri = $after_task->getPriority(); - $after_sub = $after_task->getSubpriority(); + list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( + $after_task, + $is_after = true); } else { - $after_pri = $request->getInt('priority'); - $after_sub = null; + list($pri, $sub) = ManiphestTransactionEditor::getEdgeSubpriority( + $request->getInt('priority'), + $is_end = false); } - $xactions = array(id(new ManiphestTransaction()) + $xactions = array(); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) + ->setNewValue($pri); + + $xactions[] = id(new ManiphestTransaction()) ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) - ->setNewValue(array( - 'newPriority' => $after_pri, - 'newSubpriorityBase' => $after_sub, - 'direction' => '>', - )), - ); + ->setNewValue($sub); + $editor = id(new ManiphestTransactionEditor()) ->setActor($user) ->setContinueOnMissingFields(true) diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -147,12 +147,7 @@ return $object->setOwnerPHID($phid); case ManiphestTransaction::TYPE_SUBPRIORITY: - $data = $xaction->getNewValue(); - $new_sub = $this->getNextSubpriority( - $data['newPriority'], - $data['newSubpriorityBase'], - $data['direction']); - $object->setSubpriority($new_sub); + $object->setSubpriority($xaction->getNewValue()); return; case ManiphestTransaction::TYPE_PROJECT_COLUMN: // these do external (edge) updates @@ -165,28 +160,6 @@ } } - protected function expandTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - $xactions = parent::expandTransaction($object, $xaction); - switch ($xaction->getTransactionType()) { - case ManiphestTransaction::TYPE_SUBPRIORITY: - $data = $xaction->getNewValue(); - $new_pri = $data['newPriority']; - if ($new_pri != $object->getPriority()) { - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) - ->setNewValue($new_pri); - } - break; - default: - break; - } - - return $xactions; - } - protected function applyCustomExternalTransaction( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { @@ -643,54 +616,99 @@ return $copy; } - private function getNextSubpriority($pri, $sub, $dir = '>') { - switch ($dir) { - case '>': - $order = 'ASC'; - break; - case '<': - $order = 'DESC'; - break; - default: - throw new Exception('$dir must be ">" or "<".'); - break; + /** + * Get priorities for moving a task to a new priority. + */ + public static function getEdgeSubpriority( + $priority, + $is_end) { + + $query = id(new ManiphestTaskQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->withPriorities(array($priority)) + ->setLimit(1); + + if ($is_end) { + $query->setReversePaging(true); } - if ($sub === null) { - $base = 0; + $result = $query->executeOne(); + $step = (double)(2 << 32); + + if ($result) { + $base = $result->getSubpriority(); + if ($is_end) { + $sub = ($base - $step); + } else { + $sub = ($base + $step); + } } else { - $base = $sub; + $sub = 0; } - if ($sub === null) { - $next = id(new ManiphestTask())->loadOneWhere( - 'priority = %d ORDER BY subpriority %Q LIMIT 1', - $pri, - $order); - if ($next) { - if ($dir == '>') { - return $next->getSubpriority() - ((double)(2 << 16)); - } else { - return $next->getSubpriority() + ((double)(2 << 16)); - } - } + return array($priority, $sub); + } + + + /** + * Get priorities for moving a task before or after another task. + */ + public static function getAdjacentSubpriority( + ManiphestTask $dst, + $is_after) { + + $query = id(new ManiphestTaskQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->withPriorities(array($dst->getPriority())) + ->setLimit(1); + + if ($is_after) { + $query->setAfterID($dst->getID()); } else { - $next = id(new ManiphestTask())->loadOneWhere( - 'priority = %d AND subpriority %Q %f ORDER BY subpriority %Q LIMIT 1', - $pri, - $dir, - $sub, - $order); - if ($next) { - return ($sub + $next->getSubpriority()) / 2; - } + $query->setBeforeID($dst->getID()); } - if ($dir == '>') { - return $base + (double)(2 << 32); + $adjacent = $query->executeOne(); + + $base = $dst->getSubpriority(); + $step = (double)(2 << 32); + + // If we find an adjacent task, we average the two subpriorities and + // return the result. + if ($adjacent) { + // If the adjacent task has a subpriority that is identical or very + // close to the task we're looking at, we're going to move it a little + // farther down the subpriority list. + if (abs($adjacent->getSubpriority() - $base) < 0.01) { + list($shift_pri, $shift_sub) = self::getAdjacentSubpriority( + $adjacent, + $is_after); + + queryfx( + $adjacent->establishConnection('r'), + 'UPDATE %T SET subpriority = %f WHERE id = %d', + $adjacent->getTableName(), + $shift_sub, + $adjacent->getID()); + + $adjacent->setSubpriority($shift_sub); + } + + $sub = ($adjacent->getSubpriority() + $base) / 2; } else { - return $base - (double)(2 << 32); + // Otherwise, we take a step away from the target's subpriority and + // use that. + if ($is_after) { + $sub = ($base - $step); + } else { + $sub = ($base + $step); + } } + + return array($dst->getPriority(), $sub); } + } diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -21,6 +21,7 @@ private $dateCreatedBefore; private $dateModifiedAfter; private $dateModifiedBefore; + private $reversePaging; private $fullTextSearch = ''; @@ -765,12 +766,12 @@ foreach ($app_order as $order_by) { $order[] = $order_by; } + } - if ($reverse) { - $order[] = 'task.id ASC'; - } else { - $order[] = 'task.id DESC'; - } + if ($reverse) { + $order[] = 'task.id ASC'; + } else { + $order[] = 'task.id DESC'; } return 'ORDER BY '.implode(', ', $order); @@ -1068,7 +1069,6 @@ 'name' => 'task.subpriority', 'value' => $cursor->getSubpriority(), 'type' => 'float', - 'reverse' => true, ); $columns[] = array( 'name' => 'task.dateModified', @@ -1120,4 +1120,13 @@ return 'PhabricatorManiphestApplication'; } + public function setReversePaging($reverse_paging) { + $this->reversePaging = $reverse_paging; + return $this; + } + + protected function getReversePaging() { + return $this->reversePaging; + } + } diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -112,40 +112,31 @@ } $tasks = mpull($tasks, null, 'getPHID'); - $a_task = idx($tasks, $after_phid); - $b_task = idx($tasks, $before_phid); - - if ($a_task && - (($a_task->getPriority() < $object->getPriority()) || - ($a_task->getPriority() == $object->getPriority() && - $a_task->getSubpriority() >= $object->getSubpriority()))) { + $try = array( + array($after_phid, true), + array($before_phid, false), + ); - $after_pri = $a_task->getPriority(); - $after_sub = $a_task->getSubpriority(); + $pri = null; + $sub = null; + foreach ($try as $spec) { + list($task_phid, $is_after) = $spec; + $task = idx($tasks, $task_phid); + if ($task) { + list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( + $task, + $is_after); + break; + } + } + if ($pri !== null) { $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) - ->setNewValue(array( - 'newPriority' => $after_pri, - 'newSubpriorityBase' => $after_sub, - 'direction' => '>', - )); - - } else if ($b_task && - (($b_task->getPriority() > $object->getPriority()) || - ($b_task->getPriority() == $object->getPriority() && - $b_task->getSubpriority() <= $object->getSubpriority()))) { - - $before_pri = $b_task->getPriority(); - $before_sub = $b_task->getSubpriority(); - + ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) + ->setNewValue($pri); $xactions[] = id(new ManiphestTransaction()) ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) - ->setNewValue(array( - 'newPriority' => $before_pri, - 'newSubpriorityBase' => $before_sub, - 'direction' => '<', - )); + ->setNewValue($sub); } }