From 62cb58408346c53d47cdb798b62a48a890bdd484 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 12 Feb 2014 14:05:40 -0800 Subject: [PATCH] Use timeline view in Differential and make inlines somewhat usable again Summary: Ref T2222. This gets rid of Differential's custom view and uses a standard view instead. This also mostly fixes the rendering logic for inlines. This is headed to the `tmp.differential` branch. Test Plan: {F112696} Reviewers: btrahan Reviewed By: btrahan CC: chad, aran Maniphest Tasks: T1790, T2222 Differential Revision: https://secure.phabricator.com/D8215 --- src/__phutil_library_map__.php | 4 +- .../DifferentialRevisionViewController.php | 36 ++- .../storage/DifferentialTransaction.php | 16 ++ .../DifferentialRevisionCommentListView.php | 209 ------------------ .../view/DifferentialTransactionView.php | 127 +++++++++++ .../pholio/storage/PholioTransaction.php | 3 - .../pholio/view/PholioTransactionView.php | 2 +- 7 files changed, 173 insertions(+), 224 deletions(-) delete mode 100644 src/applications/differential/view/DifferentialRevisionCommentListView.php create mode 100644 src/applications/differential/view/DifferentialTransactionView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e5fd4b4590..29eef4a517 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -437,7 +437,6 @@ phutil_register_library_map(array( 'DifferentialReviewersFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewersFieldSpecification.php', 'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php', 'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php', - 'DifferentialRevisionCommentListView' => 'applications/differential/view/DifferentialRevisionCommentListView.php', 'DifferentialRevisionCommentView' => 'applications/differential/view/DifferentialRevisionCommentView.php', 'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php', 'DifferentialRevisionDetailRenderer' => 'applications/differential/controller/DifferentialRevisionDetailRenderer.php', @@ -465,6 +464,7 @@ phutil_register_library_map(array( 'DifferentialTransaction' => 'applications/differential/storage/DifferentialTransaction.php', 'DifferentialTransactionComment' => 'applications/differential/storage/DifferentialTransactionComment.php', 'DifferentialTransactionQuery' => 'applications/differential/query/DifferentialTransactionQuery.php', + 'DifferentialTransactionView' => 'applications/differential/view/DifferentialTransactionView.php', 'DifferentialUnitFieldSpecification' => 'applications/differential/field/specification/DifferentialUnitFieldSpecification.php', 'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php', 'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php', @@ -2951,7 +2951,6 @@ phutil_register_library_map(array( 5 => 'HarbormasterBuildableInterface', 6 => 'PhabricatorSubscribableInterface', ), - 'DifferentialRevisionCommentListView' => 'AphrontView', 'DifferentialRevisionCommentView' => 'AphrontView', 'DifferentialRevisionDetailView' => 'AphrontView', 'DifferentialRevisionEditController' => 'DifferentialController', @@ -2979,6 +2978,7 @@ phutil_register_library_map(array( 'DifferentialTransaction' => 'PhabricatorApplicationTransaction', 'DifferentialTransactionComment' => 'PhabricatorApplicationTransactionComment', 'DifferentialTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'DifferentialTransactionView' => 'PhabricatorApplicationTransactionView', 'DifferentialUnitFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialViewPolicyFieldSpecification' => 'DifferentialFieldSpecification', 'DiffusionBranchTableController' => 'DiffusionController', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index ef913b7863..536e213bde 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -258,15 +258,9 @@ final class DifferentialRevisionViewController extends DifferentialController { $revision_detail->setActions($actions); $revision_detail->setUser($user); - $comment_view = new DifferentialRevisionCommentListView(); - $comment_view->setComments($comments); - $comment_view->setHandles($handles); - $comment_view->setInlineComments($inlines); - $comment_view->setChangesets($all_changesets); - $comment_view->setUser($user); - $comment_view->setTargetDiff($target); - $comment_view->setVersusDiffID($diff_vs); - $comment_view->setRevision($revision); + $comment_view = $this->buildTransactions( + $revision, + $changesets); if ($arc_project) { Javelin::initBehavior( @@ -926,4 +920,28 @@ final class DifferentialRevisionViewController extends DifferentialController { return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); } + + private function buildTransactions( + DifferentialRevision $revision, + array $changesets) { + $viewer = $this->getRequest()->getUser(); + + $xactions = id(new DifferentialTransactionQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($revision->getPHID())) + ->needComments(true) + ->execute(); + + $engine = id(new PhabricatorMarkupEngine()) + ->setViewer($viewer); + + $timeline = id(new DifferentialTransactionView()) + ->setUser($viewer) + ->setObjectPHID($revision->getPHID()) + ->setChangesets($changesets) + ->setTransactions($xactions); + + return $timeline; + } + } diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index 68fb963ee4..f410b868b4 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -18,4 +18,20 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction { return new DifferentialTransactionComment(); } + public function getTitle() { + $author_phid = $this->getAuthorPHID(); + + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + switch ($this->getTransactionType()) { + case self::TYPE_INLINE: + return pht( + '%s added inline comments.', + $this->renderHandleLink($author_phid)); + } + + return parent::getTitle(); + } + } diff --git a/src/applications/differential/view/DifferentialRevisionCommentListView.php b/src/applications/differential/view/DifferentialRevisionCommentListView.php deleted file mode 100644 index 29473bf9e3..0000000000 --- a/src/applications/differential/view/DifferentialRevisionCommentListView.php +++ /dev/null @@ -1,209 +0,0 @@ -revision = $revision; - return $this; - } - - public function getRevision() { - return $this->revision; - } - - public function setComments(array $comments) { - assert_instances_of($comments, 'DifferentialComment'); - $this->comments = $comments; - return $this; - } - - public function setInlineComments(array $inline_comments) { - assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface'); - $this->inlines = $inline_comments; - return $this; - } - - public function setHandles(array $handles) { - assert_instances_of($handles, 'PhabricatorObjectHandle'); - $this->handles = $handles; - return $this; - } - - public function setChangesets(array $changesets) { - assert_instances_of($changesets, 'DifferentialChangeset'); - $this->changesets = $changesets; - return $this; - } - - public function setTargetDiff(DifferentialDiff $target) { - $this->target = $target; - return $this; - } - - public function setVersusDiffID($diff_vs) { - $this->versusDiffID = $diff_vs; - return $this; - } - - public function getID() { - if (!$this->id) { - $this->id = celerity_generate_unique_node_id(); - } - return $this->id; - } - - public function render() { - - $this->requireResource('differential-revision-comment-list-css'); - - $engine = new PhabricatorMarkupEngine(); - $engine->setViewer($this->user); - foreach ($this->comments as $comment) { - $comment->giveFacebookSomeArbitraryDiff($this->target); - - $engine->addObject( - $comment, - DifferentialComment::MARKUP_FIELD_BODY); - } - - foreach ($this->inlines as $inline) { - $engine->addObject( - $inline, - PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); - } - - $engine->process(); - - $inlines = mgroup($this->inlines, 'getCommentID'); - - $num = 1; - $html = array(); - foreach ($this->comments as $comment) { - $view = new DifferentialRevisionCommentView(); - $view->setComment($comment); - $view->setUser($this->user); - $view->setHandles($this->handles); - $view->setMarkupEngine($engine); -// $view->setInlineComments(idx($inlines, $comment->getID(), array())); - $view->setChangesets($this->changesets); - $view->setTargetDiff($this->target); - $view->setRevision($this->getRevision()); - $view->setVersusDiffID($this->versusDiffID); - if ($comment->getAction() == DifferentialAction::ACTION_SUMMARIZE) { - $view->setAnchorName('summary'); - } else if ($comment->getAction() == DifferentialAction::ACTION_TESTPLAN) { - $view->setAnchorName('test-plan'); - } else { - $view->setAnchorName('comment-'.$num); - $num++; - } - - $html[] = $view->render(); - } - - $objs = array_reverse(array_values($this->comments)); - $html = array_reverse(array_values($html)); - $user = $this->user; - - $last_comment = null; - // Find the most recent comment by the viewer. - foreach ($objs as $position => $comment) { - if ($user && ($comment->getAuthorPHID() == $user->getPHID())) { - if ($last_comment === null) { - $last_comment = $position; - } else if ($last_comment == $position - 1) { - // If the viewer made several comments in a row, show them all. This - // is a spaz rule for epriestley. - $last_comment = $position; - } - } - } - - $header = array(); - $hidden = array(); - if ($last_comment !== null) { - foreach ($objs as $position => $comment) { - if (!$comment->getPHID()) { - // These are synthetic comments with summary/test plan information. - $header[] = $html[$position]; - unset($html[$position]); - continue; - } - if ($position <= $last_comment) { - // Always show comments after the viewer's last comment. - continue; - } - if ($position < 3) { - // Always show the 3 most recent comments. - continue; - } - $hidden[] = $position; - } - } - - if (count($hidden) <= 3) { - // Don't hide if there's not much to hide. - $hidden = array(); - } - - $header = array_reverse($header); - - - $hidden = array_select_keys($html, $hidden); - $visible = array_diff_key($html, $hidden); - - $hidden = array_reverse($hidden); - $visible = array_reverse($visible); - - if ($hidden) { - $this->initBehavior( - 'differential-show-all-comments', - array( - 'markup' => implode("\n", $hidden), - )); - $hidden = javelin_tag( - 'div', - array( - 'sigil' => "differential-all-comments-container", - ), - phutil_tag( - 'div', - array( - 'class' => 'differential-older-comments-are-hidden', - ), - array( - pht( - '%s older comments are hidden.', - new PhutilNumber(count($hidden))), - ' ', - javelin_tag( - 'a', - array( - 'href' => '#', - 'mustcapture' => true, - 'sigil' => 'differential-show-all-comments', - ), - pht('Show all comments.')), - ))); - } else { - $hidden = null; - } - - return javelin_tag( - 'div', - array( - 'class' => 'differential-comment-list', - 'id' => $this->getID(), - ), - array_merge($header, array($hidden), $visible)); - } -} diff --git a/src/applications/differential/view/DifferentialTransactionView.php b/src/applications/differential/view/DifferentialTransactionView.php new file mode 100644 index 0000000000..9bec0e5fa5 --- /dev/null +++ b/src/applications/differential/view/DifferentialTransactionView.php @@ -0,0 +1,127 @@ +changesets = $changesets; + return $this; + } + + public function getChangesets() { + return $this->changesets; + } + + // TODO: There's a whole lot of code duplication between this and + // PholioTransactionView to handle inlines. Merge this into the core? Some of + // it can probably be shared, while other parts are trickier. + + protected function shouldGroupTransactions( + PhabricatorApplicationTransaction $u, + PhabricatorApplicationTransaction $v) { + + if ($u->getAuthorPHID() != $v->getAuthorPHID()) { + // Don't group transactions by different authors. + return false; + } + + if (($v->getDateCreated() - $u->getDateCreated()) > 60) { + // Don't group if transactions that happened more than 60s apart. + return false; + } + + switch ($u->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: + case DifferentialTransaction::TYPE_INLINE: + break; + default: + return false; + } + + switch ($v->getTransactionType()) { + case DifferentialTransaction::TYPE_INLINE: + return true; + } + + return parent::shouldGroupTransactions($u, $v); + } + + protected function renderTransactionContent( + PhabricatorApplicationTransaction $xaction) { + + $out = array(); + + $type_inline = DifferentialTransaction::TYPE_INLINE; + + $group = $xaction->getTransactionGroup(); + if ($xaction->getTransactionType() == $type_inline) { + array_unshift($group, $xaction); + } else { + $out[] = parent::renderTransactionContent($xaction); + } + + if (!$group) { + return $out; + } + + $inlines = array(); + foreach ($group as $xaction) { + switch ($xaction->getTransactionType()) { + case DifferentialTransaction::TYPE_INLINE: + $inlines[] = $xaction; + break; + default: + throw new Exception("Unknown grouped transaction type!"); + } + } + + if ($inlines) { + $inline_view = new PhabricatorInlineSummaryView(); + + $changesets = $this->getChangesets(); + $changesets = mpull($changesets, null, 'getID'); + + // Group the changesets by file and reorder them by display order. + $inline_groups = array(); + foreach ($inlines as $inline) { + $inline_groups[$inline->getComment()->getChangesetID()][] = $inline; + } + + $changsets = msort($changesets, 'getFilename'); + $inline_groups = array_select_keys( + $inline_groups, + array_keys($changesets)); + + foreach ($inline_groups as $changeset_id => $group) { + $group = msort($group, 'getLineNumber'); + + $changeset = $changesets[$changeset_id]; + $items = array(); + foreach ($group as $inline) { + $comment = $inline->getComment(); + $item = array( + 'id' => $comment->getID(), + 'line' => $comment->getLineNumber(), + 'length' => $comment->getLineLength(), + 'content' => parent::renderTransactionContent($inline), + ); + + // TODO: Fix the where/href stuff for nonlocal inlines. + + $items[] = $item; + } + $inline_view->addCommentGroup( + $changeset->getFilename(), + $items); + } + + $out[] = $inline_view; + } + + return $out; + } + +} diff --git a/src/applications/pholio/storage/PholioTransaction.php b/src/applications/pholio/storage/PholioTransaction.php index af9c0502a8..c20865b914 100644 --- a/src/applications/pholio/storage/PholioTransaction.php +++ b/src/applications/pholio/storage/PholioTransaction.php @@ -1,8 +1,5 @@ getDateCreated() - $u->getDateCreated()) > 60) { - // Don't group if transactions happend more than 60s apart. + // Don't group if transactions happened more than 60s apart. return false; }