Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F13177332
D12166.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
9 KB
Referenced Files
None
Subscribers
None
D12166.diff
View Options
diff --git a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
--- a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
+++ b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
@@ -15,9 +15,11 @@
$t2 = $this->newTask($viewer, 'Task 2');
$t3 = $this->newTask($viewer, 'Task 3');
+ $auto_base = min(mpull(array($t1, $t2, $t3), 'getID'));
+
// Default order should be reverse creation.
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -26,7 +28,7 @@
// Move T3 to the middle.
$this->moveTask($viewer, $t3, $t2, true);
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -35,7 +37,7 @@
// Move T3 to the end.
$this->moveTask($viewer, $t3, $t1, true);
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -44,7 +46,7 @@
// Repeat the move above, there should be no overall change in order.
$this->moveTask($viewer, $t3, $t1, true);
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -53,7 +55,7 @@
// Move T3 to the first slot in the priority.
$this->movePriority($viewer, $t3, $t3->getPriority(), false);
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -62,7 +64,7 @@
// Move T3 to the last slot in the priority.
$this->movePriority($viewer, $t3, $t3->getPriority(), true);
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -71,7 +73,7 @@
// Move T3 before T2.
$this->moveTask($viewer, $t3, $t2, false);
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -80,7 +82,7 @@
// Move T3 before T1.
$this->moveTask($viewer, $t3, $t1, false);
- $tasks = $this->loadTasks($viewer);
+ $tasks = $this->loadTasks($viewer, $auto_base);
$t1 = $tasks[1];
$t2 = $tasks[2];
$t3 = $tasks[3];
@@ -88,6 +90,43 @@
}
+ public function testTaskAdjacentBlocks() {
+ $viewer = $this->generateNewTestUser();
+
+ $t = array();
+ for ($ii = 1; $ii < 10; $ii++) {
+ $t[$ii] = $this->newTask($viewer, "Task Block {$ii}");
+
+ // This makes sure this test remains meaningful if we begin assigning
+ // subpriorities when tasks are created.
+ $t[$ii]->setSubpriority(0)->save();
+ }
+
+ $auto_base = min(mpull($t, 'getID'));
+
+ $tasks = $this->loadTasks($viewer, $auto_base);
+ $this->assertEqual(
+ array(9, 8, 7, 6, 5, 4, 3, 2, 1),
+ array_keys($tasks));
+
+ $this->moveTask($viewer, $t[9], $t[8], true);
+ $tasks = $this->loadTasks($viewer, $auto_base);
+ $this->assertEqual(
+ array(8, 9, 7, 6, 5, 4, 3, 2, 1),
+ array_keys($tasks));
+
+ // When there is a large block of tasks which all have the same
+ // subpriority, they should be assigned distinct subpriorities as a
+ // side effect of having a task moved into the block.
+
+ $subpri = mpull($tasks, 'getSubpriority');
+ $unique_subpri = array_unique($subpri);
+ $this->assertEqual(
+ 9,
+ count($subpri),
+ 'Expected subpriorities to be distributed.');
+ }
+
private function newTask(PhabricatorUser $viewer, $title) {
$task = ManiphestTask::initializeNewTask($viewer);
@@ -103,15 +142,23 @@
return $task;
}
- private function loadTasks(PhabricatorUser $viewer) {
+ private function loadTasks(PhabricatorUser $viewer, $auto_base) {
$tasks = id(new ManiphestTaskQuery())
->setViewer($viewer)
->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
->execute();
- $tasks = mpull($tasks, null, 'getID');
+ // NOTE: AUTO_INCREMENT changes survive ROLLBACK, and we can't throw them
+ // away without committing the current transaction, so we adjust the
+ // apparent task IDs as though the first one had been ID 1. This makes the
+ // tests a little easier to understand.
+
+ $map = array();
+ foreach ($tasks as $task) {
+ $map[($task->getID() - $auto_base) + 1] = $task;
+ }
- return $tasks;
+ return $map;
}
private function moveTask(PhabricatorUser $viewer, $src, $dst, $is_after) {
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
@@ -679,21 +679,57 @@
// 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.
+ // close to the task we're looking at, we're going to move it and all
+ // tasks with the same subpriority a little farther down the subpriority
+ // scale.
if (abs($adjacent->getSubpriority() - $base) < 0.01) {
+ $conn_w = $adjacent->establishConnection('w');
+
+ // Get all of the tasks with the same subpriority as the adjacent
+ // task, including the adjacent task itself.
+ $shift_base = $adjacent->getSubpriority();
+ $shift_all = id(new ManiphestTaskQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
+ ->withPriorities(array($adjacent->getPriority()))
+ ->withSubpriorities(array($shift_base))
+ ->setReversePaging(!$is_after)
+ ->execute();
+ $shift_last = last($shift_all);
+
+ // Find the subpriority before or after the task at the end of the
+ // block.
list($shift_pri, $shift_sub) = self::getAdjacentSubpriority(
- $adjacent,
+ $shift_last,
$is_after);
- queryfx(
- $adjacent->establishConnection('r'),
- 'UPDATE %T SET subpriority = %f WHERE id = %d',
- $adjacent->getTableName(),
- $shift_sub,
- $adjacent->getID());
+ $delta = ($shift_sub - $shift_base);
+ $count = count($shift_all);
+
+ $shift = array();
+ $cursor = 1;
+ foreach ($shift_all as $shift_task) {
+ $shift_target = $shift_base + (($cursor / $count) * $delta);
+ $cursor++;
+
+ queryfx(
+ $conn_w,
+ 'UPDATE %T SET subpriority = %f WHERE id = %d',
+ $adjacent->getTableName(),
+ $shift_target,
+ $shift_task->getID());
+
+ // If we're shifting the adjacent task, update it.
+ if ($shift_task->getID() == $adjacent->getID()) {
+ $adjacent->setSubpriority($shift_target);
+ }
- $adjacent->setSubpriority($shift_sub);
+ // If we're shifting the original target task, update the base
+ // subpriority.
+ if ($shift_task->getID() == $dst->getID()) {
+ $base = $shift_target;
+ }
+ }
}
$sub = ($adjacent->getSubpriority() + $base) / 2;
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
@@ -37,6 +37,7 @@
private $statuses;
private $priorities;
+ private $subpriorities;
private $groupBy = 'group-none';
const GROUP_NONE = 'group-none';
@@ -137,6 +138,11 @@
return $this;
}
+ public function withSubpriorities(array $subpriorities) {
+ $this->subpriorities = $subpriorities;
+ return $this;
+ }
+
public function withSubscribers(array $subscribers) {
$this->subscriberPHIDs = $subscribers;
return $this;
@@ -506,6 +512,13 @@
$this->priorities);
}
+ if ($this->subpriorities) {
+ return qsprintf(
+ $conn,
+ 'task.subpriority IN (%Lf)',
+ $this->subpriorities);
+ }
+
return null;
}
diff --git a/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php b/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php
--- a/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php
+++ b/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php
@@ -11,7 +11,7 @@
$this->name = $name;
execx(
- 'php %s upgrade --force --namespace %s',
+ 'php %s upgrade --force --no-adjust --namespace %s',
$this->getStorageBinPath(),
$this->name);
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Thu, May 9, 7:38 PM (3 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6275747
Default Alt Text
D12166.diff (9 KB)
Attached To
Mode
D12166: Improve task subpriority movement algorithm for homogenous blocks
Attached
Detach File
Event Timeline
Log In to Comment