From eb010b2efc717299d82cf1de52b2bc9720a15fc5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 9 Mar 2013 19:23:50 -0800 Subject: [PATCH] Group inline transactions in Pholio Summary: Fixes T2639 by grouping related transactions at display time, so all the inlines merge into a nice block. (Note that this does not do anything about T2709 yet, so there's still no way to figure out where the inlines actually are.) Test Plan: {F35262} Reviewers: chad Reviewed By: chad CC: aran Maniphest Tasks: T2639 Differential Revision: https://secure.phabricator.com/D5313 --- src/__phutil_library_map__.php | 2 + .../controller/PholioMockViewController.php | 2 +- .../pholio/storage/PholioTransaction.php | 27 +++++- .../pholio/view/PholioTransactionView.php | 83 +++++++++++++++++++ ...bricatorApplicationTransactionResponse.php | 10 ++- .../PhabricatorApplicationTransaction.php | 16 ++++ .../PhabricatorApplicationTransactionView.php | 63 +++++++++++--- .../PhabricatorBaseEnglishTranslation.php | 7 ++ .../layout/PhabricatorPropertyListView.php | 2 +- webroot/rsrc/css/aphront/transaction.css | 11 +++ 10 files changed, 205 insertions(+), 18 deletions(-) create mode 100644 src/applications/pholio/view/PholioTransactionView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index cd779934d0..e9c13fb385 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1484,6 +1484,7 @@ phutil_register_library_map(array( 'PholioTransactionComment' => 'applications/pholio/storage/PholioTransactionComment.php', 'PholioTransactionQuery' => 'applications/pholio/query/PholioTransactionQuery.php', 'PholioTransactionType' => 'applications/pholio/constants/PholioTransactionType.php', + 'PholioTransactionView' => 'applications/pholio/view/PholioTransactionView.php', 'PhortuneMonthYearExpiryControl' => 'applications/phortune/control/PhortuneMonthYearExpiryControl.php', 'PhortuneStripeBaseController' => 'applications/phortune/stripe/controller/PhortuneStripeBaseController.php', 'PhortuneStripePaymentFormView' => 'applications/phortune/stripe/view/PhortuneStripePaymentFormView.php', @@ -2998,6 +2999,7 @@ phutil_register_library_map(array( 'PholioTransactionComment' => 'PhabricatorApplicationTransactionComment', 'PholioTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PholioTransactionType' => 'PholioConstants', + 'PholioTransactionView' => 'PhabricatorApplicationTransactionView', 'PhortuneMonthYearExpiryControl' => 'AphrontFormControl', 'PhortuneStripeBaseController' => 'PhabricatorController', 'PhortuneStripePaymentFormView' => 'AphrontView', diff --git a/src/applications/pholio/controller/PholioMockViewController.php b/src/applications/pholio/controller/PholioMockViewController.php index b9e6c569a1..bb0350f7c4 100644 --- a/src/applications/pholio/controller/PholioMockViewController.php +++ b/src/applications/pholio/controller/PholioMockViewController.php @@ -77,7 +77,7 @@ final class PholioMockViewController extends PholioController { ->setMock($mock) ->setImageID($this->imageID); - $xaction_view = id(new PhabricatorApplicationTransactionView()) + $xaction_view = id(new PholioTransactionView()) ->setUser($this->getRequest()->getUser()) ->setTransactions($xactions) ->setMarkupEngine($engine); diff --git a/src/applications/pholio/storage/PholioTransaction.php b/src/applications/pholio/storage/PholioTransaction.php index ed2a9493e1..ba355c4f63 100644 --- a/src/applications/pholio/storage/PholioTransaction.php +++ b/src/applications/pholio/storage/PholioTransaction.php @@ -17,6 +17,10 @@ final class PholioTransaction extends PhabricatorApplicationTransaction { return new PholioTransactionComment(); } + public function getApplicationTransactionViewObject() { + return new PholioTransactionView(); + } + public function getApplicationObjectTypeName() { return pht('mock'); } @@ -33,13 +37,22 @@ final class PholioTransaction extends PhabricatorApplicationTransaction { return parent::shouldHide(); } + public function getIcon() { + switch ($this->getTransactionType()) { + case PholioTransactionType::TYPE_INLINE: + return 'comment'; + } + return parent::getIcon(); + } + public function getTitle() { $author_phid = $this->getAuthorPHID(); $old = $this->getOldValue(); $new = $this->getNewValue(); - switch ($this->getTransactionType()) { + $type = $this->getTransactionType(); + switch ($type) { case PholioTransactionType::TYPE_NAME: return pht( '%s renamed this mock from "%s" to "%s".', @@ -53,9 +66,17 @@ final class PholioTransaction extends PhabricatorApplicationTransaction { $this->renderHandleLink($author_phid)); break; case PholioTransactionType::TYPE_INLINE: + $count = 1; + foreach ($this->getTransactionGroup() as $xaction) { + if ($xaction->getTransactionType() == $type) { + $count++; + } + } + return pht( - '%s added an inline comment.', - $this->renderHandleLink($author_phid)); + '%s added %d inline comment(s).', + $this->renderHandleLink($author_phid), + $count); } return parent::getTitle(); diff --git a/src/applications/pholio/view/PholioTransactionView.php b/src/applications/pholio/view/PholioTransactionView.php new file mode 100644 index 0000000000..6271d164b3 --- /dev/null +++ b/src/applications/pholio/view/PholioTransactionView.php @@ -0,0 +1,83 @@ +getAuthorPHID() != $v->getAuthorPHID()) { + // Don't group transactions by different authors. + return false; + } + + if (($v->getDateCreated() - $u->getDateCreated()) > 60) { + // Don't group if transactions happend more than 60s apart. + return false; + } + + switch ($u->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: + case PholioTransactionType::TYPE_INLINE: + break; + default: + return false; + } + + switch ($v->getTransactionType()) { + case PholioTransactionType::TYPE_INLINE: + return true; + } + + return parent::shouldGroupTransactions($u, $v); + } + + protected function renderTransactionContent( + PhabricatorApplicationTransaction $xaction) { + + $out = array(); + + $group = $xaction->getTransactionGroup(); + if ($xaction->getTransactionType() == PholioTransactionType::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 PholioTransactionType::TYPE_INLINE: + $inlines[] = $xaction; + break; + default: + throw new Exception("Unknown grouped transaction type!"); + } + } + + if ($inlines) { + $header = phutil_tag( + 'div', + array( + 'class' => 'phabricator-transaction-subheader', + ), + pht('Inline Comments')); + + $out[] = $header; + foreach ($inlines as $inline) { + if (!$inline->getComment()) { + continue; + } + $out[] = parent::renderTransactionContent($inline); + } + } + + return $out; + } + +} diff --git a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php index 1ddb9733eb..0effa8bcd7 100644 --- a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php +++ b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php @@ -47,7 +47,14 @@ final class PhabricatorApplicationTransactionResponse } public function reduceProxyResponse() { - $view = id(new PhabricatorApplicationTransactionView()) + if ($this->getTransactions()) { + $view = head($this->getTransactions()) + ->getApplicationTransactionViewObject(); + } else { + $view = new PhabricatorApplicationTransactionView(); + } + + $view ->setUser($this->getViewer()) ->setTransactions($this->getTransactions()) ->setIsPreview($this->isPreview); @@ -70,4 +77,5 @@ final class PhabricatorApplicationTransactionResponse return $this->getProxy()->setContent($content); } + } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 782e6d8b8d..50251214fc 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -26,11 +26,16 @@ abstract class PhabricatorApplicationTransaction private $handles; private $renderingTarget = self::TARGET_HTML; + private $transactionGroup = array(); abstract public function getApplicationTransactionType(); abstract public function getApplicationTransactionCommentObject(); abstract public function getApplicationObjectTypeName(); + public function getApplicationTransactionViewObject() { + return new PhabricatorApplicationTransactionView(); + } + public function generatePHID() { $type = PhabricatorPHIDConstants::PHID_TYPE_XACT; $subtype = $this->getApplicationTransactionType(); @@ -329,6 +334,17 @@ abstract class PhabricatorApplicationTransaction return null; } + public function attachTransactionGroup(array $group) { + assert_instances_of($group, 'PhabricatorApplicationTransaction'); + $this->transactionGroup = $group; + return $this; + } + + public function getTransactionGroup() { + return $this->transactionGroup; + } + + /* -( PhabricatorPolicyInterface Implementation )-------------------------- */ diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php index 8f9c5f6eb8..84dac0fdd0 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -42,17 +42,33 @@ class PhabricatorApplicationTransactionView extends AphrontView { } public function buildEvents() { - $field = PhabricatorApplicationTransactionComment::MARKUP_FIELD_COMMENT; - $engine = $this->getOrBuildEngine(); - $user = $this->getUser(); $anchor = $this->anchorOffset; $events = array(); - foreach ($this->transactions as $xaction) { + + $xactions = $this->transactions; + foreach ($xactions as $key => $xaction) { if ($xaction->shouldHide()) { - continue; + unset($xactions[$key]); } + } + + $last = null; + $last_key = null; + $groups = array(); + foreach ($xactions as $key => $xaction) { + if ($last && $this->shouldGroupTransactions($last, $xaction)) { + $groups[$last_key][] = $xaction; + unset($xactions[$key]); + } else { + $last = $xaction; + $last_key = $key; + } + } + + foreach ($xactions as $key => $xaction) { + $xaction->attachTransactionGroup(idx($groups, $key, array())); $event = id(new PhabricatorTimelineEventView()) ->setUser($user) @@ -103,12 +119,9 @@ class PhabricatorApplicationTransactionView extends AphrontView { } } - if ($xaction->hasComment()) { - $event->appendChild( - $engine->getOutput($xaction->getComment(), $field)); - } else if ($has_deleted_comment) { - $event->appendChild(phutil_tag('em', array(), pht( - 'This comment has been deleted.'))); + $content = $this->renderTransactionContent($xaction); + if ($content) { + $event->appendChild($content); } $events[] = $event; @@ -141,7 +154,7 @@ class PhabricatorApplicationTransactionView extends AphrontView { } - private function getOrBuildEngine() { + protected function getOrBuildEngine() { if ($this->engine) { return $this->engine; } @@ -215,6 +228,32 @@ class PhabricatorApplicationTransactionView extends AphrontView { ); } + protected function shouldGroupTransactions( + PhabricatorApplicationTransaction $u, + PhabricatorApplicationTransaction $v) { + return false; + } + + protected function renderTransactionContent( + PhabricatorApplicationTransaction $xaction) { + + $field = PhabricatorApplicationTransactionComment::MARKUP_FIELD_COMMENT; + $engine = $this->getOrBuildEngine(); + $comment = $xaction->getComment(); + + if ($comment) { + if ($comment->getIsDeleted()) { + return phutil_tag( + 'em', + array(), + pht('This comment has been deleted.')); + } else { + return $engine->getOutput($comment, $field); + } + } + + return null; + } } diff --git a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php index 71409b8e2c..743be93d34 100644 --- a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php +++ b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php @@ -279,6 +279,13 @@ abstract class PhabricatorBaseEnglishTranslation 'You have %d unresolved setup issues...', ), + '%s added %d inline comment(s).' => array( + array( + '%s added an inline comment.', + '%s added inline comments.', + ), + ), + ); } diff --git a/src/view/layout/PhabricatorPropertyListView.php b/src/view/layout/PhabricatorPropertyListView.php index 4950821403..85805969be 100644 --- a/src/view/layout/PhabricatorPropertyListView.php +++ b/src/view/layout/PhabricatorPropertyListView.php @@ -141,7 +141,7 @@ final class PhabricatorPropertyListView extends AphrontView { array( 'class' => 'phabricator-property-list-properties', ), - $this->renderSingleView($items)); + $items); $shortcuts = null; if ($this->hasKeyboardShortcuts) { diff --git a/webroot/rsrc/css/aphront/transaction.css b/webroot/rsrc/css/aphront/transaction.css index 2c8c3618ab..d16361bc90 100644 --- a/webroot/rsrc/css/aphront/transaction.css +++ b/webroot/rsrc/css/aphront/transaction.css @@ -51,3 +51,14 @@ padding: .3em 1em; overflow: auto; } + +.phabricator-transaction-subheader { + color: #888888; + border-bottom: 1px solid #e0e0e0; + padding-bottom: 4px; + margin-bottom: 4px; +} + +div.phabricator-remarkup + .phabricator-transaction-subheader { + margin-top: 12px; +}