On Workboards, sort groups by "natural order", not subpriority

Summary:
Depends on D20263. Ref T10333. I want to add groups like "Assignee" to workboards. This means you may have several tasks grouped under, say, "Alice".

When you drag the bottom-most task under "Alice" to the top, what does that mean?

Today, the only grouping is "Priority", and it means "change the task's secret/hidden global subpriority". However, this seems to generally be a somewhat-bad answer, and is quite complex. It also doesn't make much sense for an author grouping, since one task can't really be "more assigned" to Alice than another task.

Users likely intend this operation to mean "move it, visually, with no other effects" -- that is, user intent is to shuffle sticky notes around on a board, not edit anything substantive. The meaning is probably something like "this is similar to other nearby tasks" or "maybe this is a good place to start", which we can't really capture with any top-level attribute.

We could extend "subpriority" and give tasks a secret/hidden "sub-assignment strength" and so on, but this seems like a bad road to walk down. We'll also run into trouble later when subproject columns may appear on the board, and a user could want to put a task in different positions on different subprojects, conceivably.

In the "Natural" order view, we already have what is probably a generally better approach for this: a task display order particular to the column, that just remembers where you put the sticky notes.

Move away from "subpriority", and toward a world where we mostly keep sticky notes where you stuck them and move them around only when we have to. With no grouping, we still sort by "natural" order, as before. With priority grouping, we now sort by `<priority, natural>`. When you drag stuff around inside a priority group, we update the natural order.

This means that moving cards around on a "priority" board will also move them around on a "natural" board, at least somewhat. I think this is okay. If it's not intuitive, we could give every ordering its own separate "natural" view, so we remember where you stuck stuff on the "priority" board but that doesn't affect the "Natural" board. But I suspect we won't need to.

Test Plan:
  - Viewed and dragged a natural board.
  - Viewed and dragged a priority board.
  - Dragged within and between groups of 0, 1, and multiple items.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333

Differential Revision: https://secure.phabricator.com/D20265

resources/celerity/map.php

@@ -408,9 +408,9 @@ return array(
     'rsrc/js/application/phortune/phortune-credit-card-form.js' => 'd12d214f',
     'rsrc/js/application/policy/behavior-policy-control.js' => '0eaa33a9',
     'rsrc/js/application/policy/behavior-policy-rule-editor.js' => '9347f172',
-    'rsrc/js/application/projects/WorkboardBoard.js' => 'a4f1e85d',
+    'rsrc/js/application/projects/WorkboardBoard.js' => '902a1551',
     'rsrc/js/application/projects/WorkboardCard.js' => '887ef74f',
-    'rsrc/js/application/projects/WorkboardColumn.js' => 'ca444dca',
+    'rsrc/js/application/projects/WorkboardColumn.js' => '01ea93b3',
     'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7',
     'rsrc/js/application/projects/WorkboardHeader.js' => '6e75daea',
     'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => '2d641f7d',
@@ -727,9 +727,9 @@ return array(
     'javelin-view-renderer' => '9aae2b66',
     'javelin-view-visitor' => '308f9fe4',
     'javelin-websocket' => 'fdc13e4e',
-    'javelin-workboard-board' => 'a4f1e85d',
+    'javelin-workboard-board' => '902a1551',
     'javelin-workboard-card' => '887ef74f',
-    'javelin-workboard-column' => 'ca444dca',
+    'javelin-workboard-column' => '01ea93b3',
     'javelin-workboard-controller' => '42c7a5a7',
     'javelin-workboard-header' => '6e75daea',
     'javelin-workboard-header-template' => '2d641f7d',
@@ -889,6 +889,11 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
+    '01ea93b3' => array(
+      'javelin-install',
+      'javelin-workboard-card',
+      'javelin-workboard-header',
+    ),
     '022516b4' => array(
       'javelin-install',
       'javelin-util',
@@ -1615,6 +1620,16 @@ return array(
       'javelin-workflow',
       'javelin-stratcom',
     ),
+    '902a1551' => array(
+      'javelin-install',
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-workflow',
+      'phabricator-draggable-list',
+      'javelin-workboard-column',
+      'javelin-workboard-header-template',
+    ),
     91863989 => array(
       'javelin-install',
       'javelin-stratcom',
@@ -1750,16 +1765,6 @@ return array(
       'javelin-request',
       'javelin-util',
     ),
-    'a4f1e85d' => array(
-      'javelin-install',
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-workflow',
-      'phabricator-draggable-list',
-      'javelin-workboard-column',
-      'javelin-workboard-header-template',
-    ),
     'a5257c4e' => array(
       'javelin-install',
       'javelin-dom',
@@ -1957,11 +1962,6 @@ return array(
       'javelin-util',
       'phabricator-keyboard-shortcut-manager',
     ),
-    'ca444dca' => array(
-      'javelin-install',
-      'javelin-workboard-card',
-      'javelin-workboard-header',
-    ),
     'cf32921f' => array(
       'javelin-behavior',
       'javelin-dom',

src/applications/maniphest/storage/ManiphestTask.php

@@ -252,9 +252,6 @@ final class ManiphestTask extends ManiphestDAO
     return array(
       PhabricatorProjectColumn::ORDER_PRIORITY => array(
         (int)-$this->getPriority(),
-        PhabricatorProjectColumn::NODETYPE_CARD,
-        (double)-$this->getSubpriority(),
-        (int)-$this->getID(),
       ),
     );
   }

src/applications/project/controller/PhabricatorProjectBoardViewController.php

@@ -522,13 +522,6 @@ final class PhabricatorProjectBoardViewController
         $column->getPHID());
 
       $column_tasks = array_select_keys($tasks, $task_phids);
-
-      // If we aren't using "natural" order, reorder the column by the original
-      // query order.
-      if ($this->sortKey != PhabricatorProjectColumn::ORDER_NATURAL) {
-        $column_tasks = array_select_keys($column_tasks, array_keys($tasks));
-      }
-
       $column_phid = $column->getPHID();
 
       $visible_columns[$column_phid] = $column;
@@ -684,7 +677,6 @@ final class PhabricatorProjectBoardViewController
     );
     $this->initBehavior('project-boards', $behavior_config);
 
-
     $sort_menu = $this->buildSortMenu(
       $viewer,
       $project,

src/applications/project/controller/PhabricatorProjectMoveController.php

@@ -71,20 +71,14 @@ final class PhabricatorProjectMoveController
       ->setObjectPHIDs(array($object_phid))
       ->executeLayout();
 
-    $columns = $engine->getObjectColumns($board_phid, $object_phid);
-    $old_column_phids = mpull($columns, 'getPHID');
-
-    $xactions = array();
-
     $order_params = array();
-    if ($order == PhabricatorProjectColumn::ORDER_NATURAL) {
+    if ($after_phid) {
-      if ($after_phid) {
+      $order_params['afterPHID'] = $after_phid;
-        $order_params['afterPHID'] = $after_phid;
+    } else if ($before_phid) {
-      } else if ($before_phid) {
+      $order_params['beforePHID'] = $before_phid;
-        $order_params['beforePHID'] = $before_phid;
-      }
     }
 
+    $xactions = array();
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(PhabricatorTransactions::TYPE_COLUMNS)
       ->setNewValue(
@@ -94,18 +88,12 @@ final class PhabricatorProjectMoveController
           ) + $order_params,
         ));
 
-    if ($order == PhabricatorProjectColumn::ORDER_PRIORITY) {
+    $header_xactions = $this->newHeaderTransactions(
-      $header_priority = idx(
+      $object,
-        $edit_header,
+      $order,
-        PhabricatorProjectColumn::ORDER_PRIORITY);
+      $edit_header);
-      $priority_xactions = $this->getPriorityTransactions(
+    foreach ($header_xactions as $header_xaction) {
-        $object,
+      $xactions[] = $header_xaction;
-        $after_phid,
-        $before_phid,
-        $header_priority);
-      foreach ($priority_xactions as $xaction) {
-        $xactions[] = $xaction;
-      }
     }
 
     $editor = id(new ManiphestTransactionEditor())
@@ -119,137 +107,30 @@ final class PhabricatorProjectMoveController
     return $this->newCardResponse($board_phid, $object_phid);
   }
 
-  private function getPriorityTransactions(
+  private function newHeaderTransactions(
     ManiphestTask $task,
-    $after_phid,
+    $order,
-    $before_phid,
+    array $header) {
-    $header_priority) {
 
     $xactions = array();
-    $must_move = false;
 
-    if ($header_priority !== null) {
+    switch ($order) {
-      if ($task->getPriority() !== $header_priority) {
+      case PhabricatorProjectColumn::ORDER_PRIORITY:
-        $task = id(clone $task)
+        $new_priority = idx($header, $order);
-          ->setPriority($header_priority);
 
-        $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+        if ($task->getPriority() !== $new_priority) {
-        $keyword = head(idx($keyword_map, $header_priority));
+          $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+          $keyword = head(idx($keyword_map, $new_priority));
 
-        $xactions[] = id(new ManiphestTransaction())
+          $xactions[] = id(new ManiphestTransaction())
-          ->setTransactionType(
+            ->setTransactionType(
-            ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
+              ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-          ->setNewValue($keyword);
+            ->setNewValue($keyword);
-
-        $must_move = true;
-      }
-    }
-
-    list($after_task, $before_task) = $this->loadPriorityTasks(
-      $after_phid,
-      $before_phid);
-
-    if ($after_task && !$task->isLowerPriorityThan($after_task)) {
-      $must_move = true;
-    }
-
-    if ($before_task && !$task->isHigherPriorityThan($before_task)) {
-      $must_move = true;
-    }
-
-    // The move doesn't require a subpriority change to be valid, so don't
-    // change the subpriority since we are not being forced to.
-    if (!$must_move) {
-      return $xactions;
-    }
-
-    $try = array(
-      array($after_task, true),
-      array($before_task, false),
-    );
-
-    $pri = null;
-    $sub = null;
-    foreach ($try as $spec) {
-      list($nearby_task, $is_after) = $spec;
-
-      if (!$nearby_task) {
-        continue;
-      }
-
-      list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority(
-        $nearby_task,
-        $is_after);
-
-      // If we drag under a "Low" header between a "Normal" task and a "Low"
-      // task, we don't want to accept a subpriority assignment which changes
-      // our priority to "Normal". Only accept a subpriority that keeps us in
-      // the right primary priority.
-      if ($header_priority !== null) {
-        if ($pri !== $header_priority) {
-          continue;
         }
-      }
+        break;
-
-      // If we find a priority on the first try, don't keep going.
-      break;
-    }
-
-    if ($pri !== null) {
-      if ($header_priority === null) {
-        $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
-        $keyword = head(idx($keyword_map, $pri));
-
-        $xactions[] = id(new ManiphestTransaction())
-          ->setTransactionType(
-            ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-          ->setNewValue($keyword);
-      }
-
-      $xactions[] = id(new ManiphestTransaction())
-        ->setTransactionType(
-          ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE)
-        ->setNewValue($sub);
     }
 
     return $xactions;
   }
 
-  private function loadPriorityTasks($after_phid, $before_phid) {
-    $viewer = $this->getViewer();
-
-    $task_phids = array();
-
-    if ($after_phid) {
-      $task_phids[] = $after_phid;
-    }
-    if ($before_phid) {
-      $task_phids[] = $before_phid;
-    }
-
-    if (!$task_phids) {
-      return array(null, null);
-    }
-
-    $tasks = id(new ManiphestTaskQuery())
-      ->setViewer($viewer)
-      ->withPHIDs($task_phids)
-      ->execute();
-    $tasks = mpull($tasks, null, 'getPHID');
-
-    if ($after_phid) {
-      $after_task = idx($tasks, $after_phid);
-    } else {
-      $after_task = null;
-    }
-
-    if ($before_phid) {
-      $before_task = idx($tasks, $before_phid);
-    } else {
-      $before_task = null;
-    }
-
-    return array($after_task, $before_task);
-  }
-
 }

webroot/rsrc/js/application/projects/WorkboardBoard.js

@@ -202,6 +202,14 @@ JX.install('WorkboardBoard', {
         order: this.getOrder()
       };
 
+      // We're going to send an "afterPHID" and a "beforePHID" if the card
+      // was dropped immediately adjacent to another card. If a card was
+      // dropped before or after a header, we don't send a PHID for the card
+      // on the other side of the header.
+
+      // If the view has headers, we always send the header the card was
+      // dropped under.
+
       var after_data;
       var after_card = after_node;
       while (after_card) {
@@ -209,11 +217,16 @@ JX.install('WorkboardBoard', {
         if (after_data.objectPHID) {
           break;
         }
+        if (after_data.headerKey) {
+          break;
+        }
         after_card = after_card.previousSibling;
       }
 
       if (after_data) {
-        data.afterPHID = after_data.objectPHID;
+        if (after_data.objectPHID) {
+          data.afterPHID = after_data.objectPHID;
+        }
       }
 
       var before_data;
@@ -223,18 +236,35 @@ JX.install('WorkboardBoard', {
         if (before_data.objectPHID) {
           break;
         }
+        if (before_data.headerKey) {
+          break;
+        }
         before_card = before_card.nextSibling;
       }
 
       if (before_data) {
-        data.beforePHID = before_data.objectPHID;
+        if (before_data.objectPHID) {
+          data.beforePHID = before_data.objectPHID;
+        }
       }
 
-      var header_key = JX.Stratcom.getData(after_node).headerKey;
+      var header_data;
-      if (header_key) {
+      var header_node = after_node;
-        var properties = this.getHeaderTemplate(header_key)
+      while (header_node) {
-          .getEditProperties();
+        header_data = JX.Stratcom.getData(header_node);
-        data.header = JX.JSON.stringify(properties);
+        if (header_data.headerKey) {
+          break;
+        }
+        header_node = header_node.previousSibling;
+      }
+
+      if (header_data) {
+        var header_key = header_data.headerKey;
+        if (header_key) {
+          var properties = this.getHeaderTemplate(header_key)
+            .getEditProperties();
+          data.header = JX.JSON.stringify(properties);
+        }
       }
 
       var visible_phids = [];

webroot/rsrc/js/application/projects/WorkboardColumn.js

@@ -34,6 +34,7 @@ JX.install('WorkboardColumn', {
     _cards: null,
     _headers: null,
     _naturalOrder: null,
+    _orderVectors: null,
     _panel: null,
     _pointsNode: null,
     _pointsContentNode: null,
@@ -66,6 +67,7 @@ JX.install('WorkboardColumn', {
 
     setNaturalOrder: function(order) {
       this._naturalOrder = order;
+      this._orderVectors = null;
       return this;
     },
 
@@ -86,6 +88,7 @@ JX.install('WorkboardColumn', {
 
       this._cards[phid] = card;
       this._naturalOrder.push(phid);
+      this._orderVectors = null;
 
       return card;
     },
@@ -97,6 +100,7 @@ JX.install('WorkboardColumn', {
       for (var ii = 0; ii < this._naturalOrder.length; ii++) {
         if (this._naturalOrder[ii] == phid) {
           this._naturalOrder.splice(ii, 1);
+          this._orderVectors = null;
           break;
         }
       }
@@ -127,6 +131,8 @@ JX.install('WorkboardColumn', {
         this._naturalOrder.splice(index, 0, phid);
       }
 
+      this._orderVectors = null;
+
       return this;
     },
 
@@ -204,12 +210,7 @@ JX.install('WorkboardColumn', {
       var board = this.getBoard();
       var order = board.getOrder();
 
-      var list;
+      var list = this._getCardsSortedByKey(order);
-      if (order == 'natural') {
-        list = this._getCardsSortedNaturally();
-      } else {
-        list = this._getCardsSortedByKey(order);
-      }
 
       var ii;
       var objects = [];
@@ -285,7 +286,7 @@ JX.install('WorkboardColumn', {
       var data = JX.Stratcom.getData(node);
 
       if (data.objectPHID) {
-        return board.getOrderVector(data.objectPHID, order);
+        return this._getOrderVector(data.objectPHID, order);
       }
 
       return board.getHeaderTemplate(data.headerKey).getVector();
@@ -296,17 +297,6 @@ JX.install('WorkboardColumn', {
       JX.DOM.alterClass(node, 'workboard-column-drop-target', is_target);
     },
 
-    _getCardsSortedNaturally: function() {
-      var list = [];
-
-      for (var ii = 0; ii < this._naturalOrder.length; ii++) {
-        var phid = this._naturalOrder[ii];
-        list.push(this.getCard(phid));
-      }
-
-      return list;
-    },
-
     _getCardsSortedByKey: function(order) {
       var cards = this.getCards();
 
@@ -322,12 +312,62 @@ JX.install('WorkboardColumn', {
 
     _sortCards: function(order, u, v) {
       var board = this.getBoard();
-      var u_vec = board.getOrderVector(u.getPHID(), order);
+      var u_vec = this._getOrderVector(u.getPHID(), order);
-      var v_vec = board.getOrderVector(v.getPHID(), order);
+      var v_vec = this._getOrderVector(v.getPHID(), order);
 
       return board.compareVectors(u_vec, v_vec);
     },
 
+    _getOrderVector: function(phid, order) {
+      if (!this._orderVectors) {
+        this._orderVectors = {};
+      }
+
+      if (!this._orderVectors[order]) {
+        var board = this.getBoard();
+        var cards = this.getCards();
+        var vectors = {};
+
+        for (var k in cards) {
+          var card_phid = cards[k].getPHID();
+          var vector = board.getOrderVector(card_phid, order);
+          vectors[card_phid] = [].concat(vector);
+
+          // Push a "card" type, so cards always sort after headers; headers
+          // have a "0" in this position.
+          vectors[card_phid].push(1);
+        }
+
+        for (var ii = 0; ii < this._naturalOrder.length; ii++) {
+          var natural_phid = this._naturalOrder[ii];
+          if (vectors[natural_phid]) {
+            vectors[natural_phid].push(ii);
+          }
+        }
+
+        this._orderVectors[order] = vectors;
+      }
+
+      if (!this._orderVectors[order][phid]) {
+        // In this case, we're comparing a card being dragged in from another
+        // column to the cards already in this column. We're just going to
+        // build a temporary vector for it.
+        var incoming_vector = this.getBoard().getOrderVector(phid, order);
+        incoming_vector = [].concat(incoming_vector);
+
+        // Add a "card" type to sort this after headers.
+        incoming_vector.push(1);
+
+        // Add a "0" for the natural ordering to put this on top. A new card
+        // has no natural ordering on a column it isn't part of yet.
+        incoming_vector.push(0);
+
+        return incoming_vector;
+      }
+
+      return this._orderVectors[order][phid];
+    },
+
     _redrawFrame: function() {
       var cards = this.getCards();
       var board = this.getBoard();