From e4690a385447a470bc78089daed9da048ef8a824 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Feb 2016 14:43:20 -0800 Subject: [PATCH] Fix an issue where newly created tasks could appear at the bottom of columns Summary: Ref T10349. At HEAD, if you create a task //on a board//, it floats to the top correctly. If you create a task elsewhere and tag it with the board, you were subject to the whims of the layout engine and it would generally end up on the bottom. Instead, make the rules consistent so that "virtual" positions (of tasks which haven't been committed to a particular position yet) still float to the top. Test Plan: - Created tasks from a board. - Created tasks from Maniphest, then looked at them on a board. - Moved tasks around. - In all cases, newly created tasks floated to the top. - Sorted by natural and priority. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10349 Differential Revision: https://secure.phabricator.com/D15276 --- .../editor/ManiphestTransactionEditor.php | 10 +++++-- .../PhabricatorProjectBoardViewController.php | 6 +++- .../engine/PhabricatorBoardLayoutEngine.php | 7 +++-- .../PhabricatorProjectColumnPosition.php | 29 ++++++++++++++----- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 08cfc66632..97c567c30d 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -289,8 +289,14 @@ final class ManiphestTransactionEditor array($select_phids)) ->execute(); - $object_phids = mpull($board_tasks, 'getPHID'); - $object_phids[] = $object_phid; + $board_tasks = mpull($board_tasks, null, 'getPHID'); + $board_tasks[$object_phid] = $object; + + // Make sure tasks are sorted by ID, so we lay out new positions in + // a consistent way. + $board_tasks = msort($board_tasks, 'getID'); + + $object_phids = array_keys($board_tasks); $engine = id(new PhabricatorBoardLayoutEngine()) ->setViewer($omnipotent_viewer) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index e8de61e3cc..963cb2a752 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -124,10 +124,14 @@ final class PhabricatorProjectBoardViewController $board_phid = $project->getPHID(); + // Regardless of display order, pass tasks to the layout engine in ID order + // so layout is consistent. + $board_tasks = msort($tasks, 'getID'); + $layout_engine = id(new PhabricatorBoardLayoutEngine()) ->setViewer($viewer) ->setBoardPHIDs(array($board_phid)) - ->setObjectPHIDs(array_keys($tasks)) + ->setObjectPHIDs(array_keys($board_tasks)) ->setFetchAllBoards(true) ->executeLayout(); diff --git a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php index 3525af5ff0..248677fbec 100644 --- a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php +++ b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php @@ -506,6 +506,7 @@ final class PhabricatorBoardLayoutEngine extends Phobject { } } + $view_sequence = 1; foreach ($object_phids as $object_phid) { $positions = idx($position_groups, $object_phid, array()); @@ -554,7 +555,8 @@ final class PhabricatorBoardLayoutEngine extends Phobject { ->setBoardPHID($board_phid) ->setColumnPHID($proxy_hit) ->setObjectPHID($object_phid) - ->setSequence(0); + ->setSequence(0) + ->setViewSequence($view_sequence++); $this->addQueue[] = $new_position; @@ -578,7 +580,8 @@ final class PhabricatorBoardLayoutEngine extends Phobject { ->setBoardPHID($board_phid) ->setColumnPHID($default_phid) ->setObjectPHID($object_phid) - ->setSequence(0); + ->setSequence(0) + ->setViewSequence($view_sequence++); $this->addQueue[] = $new_position; diff --git a/src/applications/project/storage/PhabricatorProjectColumnPosition.php b/src/applications/project/storage/PhabricatorProjectColumnPosition.php index c59676f8e4..40929606ac 100644 --- a/src/applications/project/storage/PhabricatorProjectColumnPosition.php +++ b/src/applications/project/storage/PhabricatorProjectColumnPosition.php @@ -9,6 +9,7 @@ final class PhabricatorProjectColumnPosition extends PhabricatorProjectDAO protected $sequence; private $column = self::ATTACHABLE; + private $viewSequence = 0; protected function getConfiguration() { return array( @@ -40,18 +41,30 @@ final class PhabricatorProjectColumnPosition extends PhabricatorProjectDAO return $this; } - public function getOrderingKey() { - if (!$this->getID() && !$this->getSequence()) { - return 0; - } + public function setViewSequence($view_sequence) { + $this->viewSequence = $view_sequence; + return $this; + } - // Low sequence numbers go above high sequence numbers. - // High position IDs go above low position IDs. - // Broadly, this makes newly added stuff float to the top. + public function getOrderingKey() { + // We're ordering both real positions and "virtual" positions which we have + // created but not saved yet. + + // Low sequence numbers go above high sequence numbers. Virtual positions + // will have sequence number 0. + + // High virtual sequence numbers go above low virtual sequence numbers. + // The layout engine gets objects in ID order, and this puts them in + // reverse ID order. + + // High IDs go above low IDs. + + // Broadly, this collectively makes newly added stuff float to the top. return sprintf( - '~%012d%012d', + '~%012d%012d%012d', $this->getSequence(), + ((1 << 31) - $this->viewSequence), ((1 << 31) - $this->getID())); }