From 00543f0620d1c9ecfbbf89bfef92fa0d1a002d62 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2019 10:55:57 -0800 Subject: [PATCH] Remove the ability to drag tasks up and down on (non-Workboard) priority list views Summary: Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted. I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards. This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards. As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note". Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can. The only real reason to have a global "subpriority" is to support the list-view drag-and-drop. It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein. (This just removes UI, the actual subpriority system is still intact and still used on workboards.) Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13074 Differential Revision: https://secure.phabricator.com/D20263 --- resources/celerity/map.php | 28 +++----- resources/celerity/packages.php | 1 - src/__phutil_library_map__.php | 2 - .../PhabricatorManiphestApplication.php | 1 - .../controller/ManiphestController.php | 1 - .../ManiphestSubpriorityController.php | 70 ------------------ .../ManiphestTaskEditController.php | 1 - .../query/ManiphestTaskSearchEngine.php | 3 - .../maniphest/view/ManiphestTaskListView.php | 14 +--- .../view/ManiphestTaskResultListView.php | 30 -------- src/view/phui/PHUIObjectItemView.php | 20 ++---- .../maniphest/behavior-batch-selector.js | 13 ---- .../maniphest/behavior-subpriorityeditor.js | 72 ------------------- 13 files changed, 16 insertions(+), 240 deletions(-) delete mode 100644 src/applications/maniphest/controller/ManiphestSubpriorityController.php delete mode 100644 webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0e963428bc..58aa364de5 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -16,7 +16,7 @@ return array( 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', 'maniphest.pkg.css' => '35995d6d', - 'maniphest.pkg.js' => '286955ae', + 'maniphest.pkg.js' => 'c9308721', 'rsrc/audio/basic/alert.mp3' => '17889334', 'rsrc/audio/basic/bing.mp3' => 'a817a0c3', 'rsrc/audio/basic/pock.mp3' => '0fa843d0', @@ -395,10 +395,9 @@ return array( 'rsrc/js/application/herald/HeraldRuleEditor.js' => '27daef73', 'rsrc/js/application/herald/PathTypeahead.js' => 'ad486db3', 'rsrc/js/application/herald/herald-rule-editor.js' => '0922e81d', - 'rsrc/js/application/maniphest/behavior-batch-selector.js' => 'cffd39b4', + 'rsrc/js/application/maniphest/behavior-batch-selector.js' => '139ef688', 'rsrc/js/application/maniphest/behavior-line-chart.js' => 'c8147a20', 'rsrc/js/application/maniphest/behavior-list-edit.js' => 'c687e867', - 'rsrc/js/application/maniphest/behavior-subpriorityeditor.js' => '8400307c', 'rsrc/js/application/owners/OwnersPathEditor.js' => '2a8b62d9', 'rsrc/js/application/owners/owners-path-editor.js' => 'ff688a7a', 'rsrc/js/application/passphrase/passphrase-credential-control.js' => '48fe33d0', @@ -619,9 +618,8 @@ return array( 'javelin-behavior-lightbox-attachments' => 'c7e748bf', 'javelin-behavior-line-chart' => 'c8147a20', 'javelin-behavior-linked-container' => '74446546', - 'javelin-behavior-maniphest-batch-selector' => 'cffd39b4', + 'javelin-behavior-maniphest-batch-selector' => '139ef688', 'javelin-behavior-maniphest-list-editor' => 'c687e867', - 'javelin-behavior-maniphest-subpriority-editor' => '8400307c', 'javelin-behavior-owners-path-editor' => 'ff688a7a', 'javelin-behavior-passphrase-credential-control' => '48fe33d0', 'javelin-behavior-phabricator-active-nav' => '7353f43d', @@ -998,6 +996,12 @@ return array( 'javelin-uri', 'phabricator-keyboard-shortcut', ), + '139ef688' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + ), '1c850a26' => array( 'javelin-install', 'javelin-util', @@ -1539,13 +1543,6 @@ return array( 'javelin-dom', 'javelin-vector', ), - '8400307c' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - ), '8437c663' => array( 'javelin-install', 'javelin-dom', @@ -1970,12 +1967,6 @@ return array( 'javelin-dom', 'javelin-stratcom', ), - 'cffd39b4' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - ), 'd0a85a85' => array( 'javelin-dom', 'javelin-util', @@ -2362,7 +2353,6 @@ return array( ), 'maniphest.pkg.js' => array( 'javelin-behavior-maniphest-batch-selector', - 'javelin-behavior-maniphest-subpriority-editor', 'javelin-behavior-maniphest-list-editor', ), ), diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index deef3633a8..6dbb662288 100644 --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -218,7 +218,6 @@ return array( ), 'maniphest.pkg.js' => array( 'javelin-behavior-maniphest-batch-selector', - 'javelin-behavior-maniphest-subpriority-editor', 'javelin-behavior-maniphest-list-editor', ), ); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 651fad9d12..3cab91717a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1709,7 +1709,6 @@ phutil_register_library_map(array( 'ManiphestStatusEmailCommand' => 'applications/maniphest/command/ManiphestStatusEmailCommand.php', 'ManiphestStatusSearchConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestStatusSearchConduitAPIMethod.php', 'ManiphestStatusesConfigType' => 'applications/maniphest/config/ManiphestStatusesConfigType.php', - 'ManiphestSubpriorityController' => 'applications/maniphest/controller/ManiphestSubpriorityController.php', 'ManiphestSubtypesConfigType' => 'applications/maniphest/config/ManiphestSubtypesConfigType.php', 'ManiphestTask' => 'applications/maniphest/storage/ManiphestTask.php', 'ManiphestTaskAssignHeraldAction' => 'applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php', @@ -7406,7 +7405,6 @@ phutil_register_library_map(array( 'ManiphestStatusEmailCommand' => 'ManiphestEmailCommand', 'ManiphestStatusSearchConduitAPIMethod' => 'ManiphestConduitAPIMethod', 'ManiphestStatusesConfigType' => 'PhabricatorJSONConfigType', - 'ManiphestSubpriorityController' => 'ManiphestController', 'ManiphestSubtypesConfigType' => 'PhabricatorJSONConfigType', 'ManiphestTask' => array( 'ManiphestDAO', diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php index ec732791fa..8ed20416bb 100644 --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -54,7 +54,6 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication { => 'ManiphestTaskEditController', 'subtask/(?P[1-9]\d*)/' => 'ManiphestTaskSubtaskController', ), - 'subpriority/' => 'ManiphestSubpriorityController', 'graph/(?P[1-9]\d*)/' => 'ManiphestTaskGraphController', ), ); diff --git a/src/applications/maniphest/controller/ManiphestController.php b/src/applications/maniphest/controller/ManiphestController.php index 872d3f7b38..51a243afac 100644 --- a/src/applications/maniphest/controller/ManiphestController.php +++ b/src/applications/maniphest/controller/ManiphestController.php @@ -53,7 +53,6 @@ abstract class ManiphestController extends PhabricatorController { $view = id(new ManiphestTaskListView()) ->setUser($user) - ->setShowSubpriorityControls(!$request->getStr('ungrippable')) ->setShowBatchControls(true) ->setHandles($handles) ->setTasks(array($task)); diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php deleted file mode 100644 index 8869b6a327..0000000000 --- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php +++ /dev/null @@ -1,70 +0,0 @@ -getViewer(); - - if (!$request->validateCSRF()) { - return new Aphront403Response(); - } - - $task = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->withIDs(array($request->getInt('task'))) - ->needProjectPHIDs(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->executeOne(); - if (!$task) { - return new Aphront404Response(); - } - - if ($request->getInt('after')) { - $after_task = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->withIDs(array($request->getInt('after'))) - ->executeOne(); - if (!$after_task) { - return new Aphront404Response(); - } - list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( - $after_task, - $is_after = true); - } else { - list($pri, $sub) = ManiphestTransactionEditor::getEdgeSubpriority( - $request->getInt('priority'), - $is_end = false); - } - - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head(idx($keyword_map, $pri)); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($sub); - - $editor = id(new ManiphestTransactionEditor()) - ->setActor($viewer) - ->setContinueOnMissingFields(true) - ->setContinueOnNoEffect(true) - ->setContentSourceFromRequest($request); - - $editor->applyTransactions($task, $xactions); - - return id(new AphrontAjaxResponse())->setContent( - array( - 'tasks' => $this->renderSingleTask($task), - )); - } - -} diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 9483529138..341997e325 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -5,7 +5,6 @@ final class ManiphestTaskEditController extends ManiphestController { public function handleRequest(AphrontRequest $request) { return id(new ManiphestEditEngine()) ->setController($this) - ->addContextParameter('ungrippable') ->addContextParameter('responseType') ->addContextParameter('columnPHID') ->addContextParameter('order') diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index f4a1f2b344..4c69c604e4 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -366,10 +366,8 @@ final class ManiphestTaskSearchEngine $viewer = $this->requireViewer(); if ($this->isPanelContext()) { - $can_edit_priority = false; $can_bulk_edit = false; } else { - $can_edit_priority = true; $can_bulk_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $this->getApplication(), @@ -380,7 +378,6 @@ final class ManiphestTaskSearchEngine ->setUser($viewer) ->setTasks($tasks) ->setSavedQuery($saved) - ->setCanEditPriority($can_edit_priority) ->setCanBatchEdit($can_bulk_edit) ->setShowBatchControls($this->showBatchControls); diff --git a/src/applications/maniphest/view/ManiphestTaskListView.php b/src/applications/maniphest/view/ManiphestTaskListView.php index 6bf5daf29b..f9ad9e6046 100644 --- a/src/applications/maniphest/view/ManiphestTaskListView.php +++ b/src/applications/maniphest/view/ManiphestTaskListView.php @@ -5,7 +5,6 @@ final class ManiphestTaskListView extends ManiphestView { private $tasks; private $handles; private $showBatchControls; - private $showSubpriorityControls; private $noDataString; public function setTasks(array $tasks) { @@ -25,11 +24,6 @@ final class ManiphestTaskListView extends ManiphestView { return $this; } - public function setShowSubpriorityControls($show_subpriority_controls) { - $this->showSubpriorityControls = $show_subpriority_controls; - return $this; - } - public function setNoDataString($text) { $this->noDataString = $text; return $this; @@ -102,10 +96,7 @@ final class ManiphestTaskListView extends ManiphestView { phabricator_datetime($task->getDateModified(), $this->getUser())); } - if ($this->showSubpriorityControls) { - $item->setGrippable(true); - } - if ($this->showSubpriorityControls || $this->showBatchControls) { + if ($this->showBatchControls) { $item->addSigil('maniphest-task'); } @@ -134,9 +125,6 @@ final class ManiphestTaskListView extends ManiphestView { if ($this->showBatchControls) { $href = new PhutilURI('/maniphest/task/edit/'.$task->getID().'/'); - if (!$this->showSubpriorityControls) { - $href->replaceQueryParam('ungrippable', 'true'); - } $item->addAction( id(new PHUIListItemView()) ->setIcon('fa-pencil') diff --git a/src/applications/maniphest/view/ManiphestTaskResultListView.php b/src/applications/maniphest/view/ManiphestTaskResultListView.php index 6aafcbdccb..cc2a135292 100644 --- a/src/applications/maniphest/view/ManiphestTaskResultListView.php +++ b/src/applications/maniphest/view/ManiphestTaskResultListView.php @@ -4,7 +4,6 @@ final class ManiphestTaskResultListView extends ManiphestView { private $tasks; private $savedQuery; - private $canEditPriority; private $canBatchEdit; private $showBatchControls; @@ -18,11 +17,6 @@ final class ManiphestTaskResultListView extends ManiphestView { return $this; } - public function setCanEditPriority($can_edit_priority) { - $this->canEditPriority = $can_edit_priority; - return $this; - } - public function setCanBatchEdit($can_batch_edit) { $this->canBatchEdit = $can_batch_edit; return $this; @@ -54,28 +48,12 @@ final class ManiphestTaskResultListView extends ManiphestView { $group_parameter, $handles); - $can_edit_priority = $this->canEditPriority; - - $can_drag = ($order_parameter == 'priority') && - ($can_edit_priority) && - ($group_parameter == 'none' || $group_parameter == 'priority'); - - if (!$viewer->isLoggedIn()) { - // TODO: (T7131) Eventually, we conceivably need to make each task - // draggable individually, since the user may be able to edit some but - // not others. - $can_drag = false; - } - $result = array(); $lists = array(); foreach ($groups as $group => $list) { $task_list = new ManiphestTaskListView(); $task_list->setShowBatchControls($this->showBatchControls); - if ($can_drag) { - $task_list->setShowSubpriorityControls(true); - } $task_list->setUser($viewer); $task_list->setTasks($list); $task_list->setHandles($handles); @@ -91,14 +69,6 @@ final class ManiphestTaskResultListView extends ManiphestView { } - if ($can_drag) { - Javelin::initBehavior( - 'maniphest-subpriority-editor', - array( - 'uri' => '/maniphest/subpriority/', - )); - } - return array( $lists, $this->showBatchControls ? $this->renderBatchEditor($query) : null, diff --git a/src/view/phui/PHUIObjectItemView.php b/src/view/phui/PHUIObjectItemView.php index e1c67c7f32..463f34a2a0 100644 --- a/src/view/phui/PHUIObjectItemView.php +++ b/src/view/phui/PHUIObjectItemView.php @@ -414,25 +414,17 @@ final class PHUIObjectItemView extends AphrontTagView { )); } - // Wrap the header content in a with the "slippery" sigil. This - // prevents us from beginning a drag if you click the text (like "T123"), - // but not if you click the white space after the header. $header = phutil_tag( 'div', array( 'class' => 'phui-oi-name', ), - javelin_tag( - 'span', - array( - 'sigil' => 'slippery', - ), - array( - $this->headIcons, - $header_name, - $header_link, - $description_tag, - ))); + array( + $this->headIcons, + $header_name, + $header_link, + $description_tag, + )); $icons = array(); if ($this->icons) { diff --git a/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js b/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js index b62f40a589..b64abc3503 100644 --- a/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js +++ b/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js @@ -41,19 +41,6 @@ JX.behavior('maniphest-batch-selector', function(config) { update(); }; - var redraw = function (task) { - var selected = is_selected(task); - change(task, selected); - }; - JX.Stratcom.listen( - 'subpriority-changed', - null, - function (e) { - e.kill(); - var data = e.getData(); - redraw(data.task); - }); - // Change all tasks to some state (used by "select all" / "clear selection" // buttons). diff --git a/webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js b/webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js deleted file mode 100644 index 82f16854f8..0000000000 --- a/webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js +++ /dev/null @@ -1,72 +0,0 @@ -/** - * @provides javelin-behavior-maniphest-subpriority-editor - * @requires javelin-behavior - * javelin-dom - * javelin-stratcom - * javelin-workflow - * phabricator-draggable-list - */ - -JX.behavior('maniphest-subpriority-editor', function(config) { - - var draggable = new JX.DraggableList('maniphest-task') - .setFindItemsHandler(function() { - var tasks = JX.DOM.scry(document.body, 'li', 'maniphest-task'); - var heads = JX.DOM.scry(document.body, 'div', 'task-group'); - return tasks.concat(heads); - }) - .setGhostHandler(function(ghost, target) { - if (!target) { - // The user is trying to drag a task above the first group header; - // don't permit that since it doesn't make sense. - return false; - } - - if (target.nextSibling) { - if (JX.DOM.isType(target, 'div')) { - target.nextSibling.insertBefore(ghost, target.nextSibling.firstChild); - } else { - target.parentNode.insertBefore(ghost, target.nextSibling); - } - } else { - target.parentNode.appendChild(ghost); - } - }); - - draggable.listen('shouldBeginDrag', function(e) { - if (e.getNode('slippery') || e.getNode('maniphest-edit-task')) { - JX.Stratcom.context().kill(); - } - }); - - draggable.listen('didDrop', function(node, after) { - var data = { - task: JX.Stratcom.getData(node).taskID - }; - - if (JX.DOM.isType(after, 'div')) { - data.priority = JX.Stratcom.getData(after).priority; - } else { - data.after = JX.Stratcom.getData(after).taskID; - } - - draggable.lock(); - JX.DOM.alterClass(node, 'drag-sending', true); - - var onresponse = function(r) { - var nodes = JX.$H(r.tasks).getFragment().firstChild; - var task = JX.DOM.find(nodes, 'li', 'maniphest-task'); - JX.DOM.replace(node, task); - draggable.unlock(); - JX.Stratcom.invoke( - 'subpriority-changed', - null, - { 'task' : task }); - }; - - new JX.Workflow(config.uri, data) - .setHandler(onresponse) - .start(); - }); - -});