From de2da8355b8f9605c046a36e4e4e306ee5b026b8 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 27 Mar 2014 10:50:54 -0700 Subject: [PATCH] Workboards - make priority changes less aggressive and generally better Summary: Fixes T4641. Test Plan: Dragged a "normal" task between "high" and "low" tasks and it stayed as "normal". Generally seems correct when playing around. Reviewers: epriestley Reviewed By: epriestley Subscribers: mbishopim3, Beltran-rubo, epriestley, Korvin Maniphest Tasks: T4641 Differential Revision: https://secure.phabricator.com/D8622 --- resources/celerity/map.php | 22 +++---- .../ManiphestSubpriorityController.php | 3 +- .../editor/ManiphestTransactionEditor.php | 46 ++++++++++++--- .../PhabricatorProjectMoveController.php | 59 +++++++++++++++---- .../projects/behavior-project-boards.js | 31 +++++++++- 5 files changed, 127 insertions(+), 34 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 85410f9d6e..7b8777e0d6 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -404,7 +404,7 @@ return array( 'rsrc/js/application/policy/behavior-policy-control.js' => 'c01153ea', 'rsrc/js/application/policy/behavior-policy-rule-editor.js' => '263aeb8c', 'rsrc/js/application/ponder/behavior-votebox.js' => '327dbe61', - 'rsrc/js/application/projects/behavior-project-boards.js' => '04c59172', + 'rsrc/js/application/projects/behavior-project-boards.js' => 'd8e135db', 'rsrc/js/application/projects/behavior-project-create.js' => '065227cc', 'rsrc/js/application/releeph/releeph-preview-branch.js' => '9eb2cedb', 'rsrc/js/application/releeph/releeph-request-state-change.js' => 'fe7fc914', @@ -614,7 +614,7 @@ return array( 'javelin-behavior-policy-control' => 'c01153ea', 'javelin-behavior-policy-rule-editor' => '263aeb8c', 'javelin-behavior-ponder-votebox' => '327dbe61', - 'javelin-behavior-project-boards' => '04c59172', + 'javelin-behavior-project-boards' => 'd8e135db', 'javelin-behavior-project-create' => '065227cc', 'javelin-behavior-refresh-csrf' => 'c4b31646', 'javelin-behavior-releeph-preview-branch' => '9eb2cedb', @@ -843,15 +843,6 @@ return array( 3 => 'javelin-vector', 4 => 'javelin-install', ), - '04c59172' => - array( - 0 => 'javelin-behavior', - 1 => 'javelin-dom', - 2 => 'javelin-util', - 3 => 'javelin-stratcom', - 4 => 'javelin-workflow', - 5 => 'phabricator-draggable-list', - ), '065227cc' => array( 0 => 'javelin-behavior', @@ -1752,6 +1743,15 @@ return array( 3 => 'javelin-dom', 4 => 'phabricator-keyboard-shortcut', ), + 'd8e135db' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-dom', + 2 => 'javelin-util', + 3 => 'javelin-stratcom', + 4 => 'javelin-workflow', + 5 => 'phabricator-draggable-list', + ), 'd8ef8659' => array( 0 => 'javelin-behavior', diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php index 8ba98c957b..4290c3bc02 100644 --- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php +++ b/src/applications/maniphest/controller/ManiphestSubpriorityController.php @@ -45,7 +45,8 @@ final class ManiphestSubpriorityController extends ManiphestController { ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) ->setNewValue(array( 'newPriority' => $after_pri, - 'newSubpriorityBase' => $after_sub))); + 'newSubpriorityBase' => $after_sub, + 'direction' => '>'))); $editor = id(new ManiphestTransactionEditor()) ->setActor($user) ->setContinueOnMissingFields(true) diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 419c0db26e..3b3c0e65ff 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -165,7 +165,8 @@ final class ManiphestTransactionEditor $data = $xaction->getNewValue(); $new_sub = $this->getNextSubpriority( $data['newPriority'], - $data['newSubpriorityBase']); + $data['newSubpriorityBase'], + $data['direction']); $object->setSubpriority($new_sub); return; case ManiphestTransaction::TYPE_PROJECT_COLUMN: @@ -441,26 +442,55 @@ final class ManiphestTransactionEditor return $copy; } - private function getNextSubpriority($pri, $sub) { + 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; + } + + if ($sub === null) { + $base = 0; + } else { + $base = $sub; + } if ($sub === null) { $next = id(new ManiphestTask())->loadOneWhere( - 'priority = %d ORDER BY subpriority ASC LIMIT 1', - $pri); + 'priority = %d ORDER BY subpriority %Q LIMIT 1', + $pri, + $order); if ($next) { - return $next->getSubpriority() - ((double)(2 << 16)); + if ($dir == '>') { + return $next->getSubpriority() - ((double)(2 << 16)); + } else { + return $next->getSubpriority() + ((double)(2 << 16)); + } } } else { $next = id(new ManiphestTask())->loadOneWhere( - 'priority = %d AND subpriority > %s ORDER BY subpriority ASC LIMIT 1', + 'priority = %d AND subpriority %Q %f ORDER BY subpriority %Q LIMIT 1', $pri, - $sub); + $dir, + $sub, + $order); if ($next) { return ($sub + $next->getSubpriority()) / 2; } } - return (double)(2 << 32); + if ($dir == '>') { + return $base + (double)(2 << 32); + } else { + return $base - (double)(2 << 32); + } } } diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 906ec3a267..17ad58b30c 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -16,6 +16,7 @@ final class PhabricatorProjectMoveController $column_phid = $request->getStr('columnPHID'); $object_phid = $request->getStr('objectPHID'); $after_phid = $request->getStr('afterPHID'); + $before_phid = $request->getStr('beforePHID'); $project = id(new PhabricatorProjectQuery()) ->setViewer($viewer) @@ -78,24 +79,58 @@ final class PhabricatorProjectMoveController 'columnPHIDs' => $edge_phids, 'projectPHID' => $column->getProjectPHID())); + $task_phids = array(); if ($after_phid) { - $after_task = id(new ManiphestTaskQuery()) + $task_phids[] = $after_phid; + } + if ($before_phid) { + $task_phids[] = $before_phid; + } + if ($task_phids) { + $tasks = id(new ManiphestTaskQuery()) ->setViewer($viewer) - ->withPHIDs(array($after_phid)) + ->withPHIDs($task_phids) ->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT)) - ->executeOne(); - if (!$after_task) { + ->execute(); + if (count($tasks) != count($task_phids)) { return new Aphront404Response(); } - $after_pri = $after_task->getPriority(); - $after_sub = $after_task->getSubpriority(); + $tasks = mpull($tasks, null, 'getPHID'); - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) - ->setNewValue(array( - 'newPriority' => $after_pri, - 'newSubpriorityBase' => $after_sub)); - } + $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()))) { + + $after_pri = $a_task->getPriority(); + $after_sub = $a_task->getSubpriority(); + + $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(); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) + ->setNewValue(array( + 'newPriority' => $before_pri, + 'newSubpriorityBase' => $before_sub, + 'direction' => '<')); + } + } $editor = id(new ManiphestTransactionEditor()) ->setActor($viewer) diff --git a/webroot/rsrc/js/application/projects/behavior-project-boards.js b/webroot/rsrc/js/application/projects/behavior-project-boards.js index 3ad9d0de9d..fd2c6e69e0 100644 --- a/webroot/rsrc/js/application/projects/behavior-project-boards.js +++ b/webroot/rsrc/js/application/projects/behavior-project-boards.js @@ -28,13 +28,40 @@ JX.behavior('project-boards', function(config) { list.lock(); JX.DOM.alterClass(item, 'drag-sending', true); + var item_phid = JX.Stratcom.getData(item).objectPHID; var data = { - objectPHID: JX.Stratcom.getData(item).objectPHID, + objectPHID: item_phid, columnPHID: JX.Stratcom.getData(list.getRootNode()).columnPHID }; + var after_phid = null; + var items = finditems(list.getRootNode()); if (after) { - data.afterPHID = JX.Stratcom.getData(after).objectPHID; + after_phid = JX.Stratcom.getData(after).objectPHID; + data.afterPHID = after_phid; + } + var ii; + var ii_item; + var ii_item_phid; + var ii_prev_item_phid = null; + var before_phid = null; + for (ii = 0; ii < items.length; ii++) { + ii_item = items[ii]; + ii_item_phid = JX.Stratcom.getData(ii_item).objectPHID; + if (ii_item_phid == item_phid) { + // skip the item we just dropped + continue; + } + // note this handles when there is no after phid - we are at the top of + // the list - quite nicely + if (ii_prev_item_phid == after_phid) { + before_phid = ii_item_phid; + break; + } + ii_prev_item_phid = ii_item_phid; + } + if (before_phid) { + data.beforePHID = before_phid; } var workflow = new JX.Workflow(config.moveURI, data)