From 536c606ddef3ccaffedc233b9110c5f364797ccd Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Dec 2013 05:51:15 -0800 Subject: [PATCH] Implment ApplicationTransaction grouping rules Summary: Ref T4266. This implements rules similar to the old rules. With D7842, maybe this is reasonable? I think it's not like grotesquely bad, at least. Test Plan: See screenshot. Reviewers: chad, wrotte Reviewed By: chad CC: aran Maniphest Tasks: T4266 Differential Revision: https://secure.phabricator.com/D7843 --- .../PhabricatorApplicationTransaction.php | 53 +++- .../PhabricatorApplicationTransactionView.php | 234 ++++++++++++------ .../layout/PhabricatorTimelineEventView.php | 6 +- 3 files changed, 212 insertions(+), 81 deletions(-) diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index e4fb7c52b9..fbb13222c9 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -233,7 +233,7 @@ abstract class PhabricatorApplicationTransaction return 'link'; } - return null; + return 'edit'; } public function getColor() { @@ -537,6 +537,57 @@ abstract class PhabricatorApplicationTransaction return $this->transactionGroup; } + /** + * Should this transaction be visually grouped with an existing transaction + * group? + * + * @param list List of transactions. + * @return bool True to display in a group with the other transactions. + */ + public function shouldDisplayGroupWith(array $group) { + $type_comment = PhabricatorTransactions::TYPE_COMMENT; + + $this_source = null; + if ($this->getContentSource()) { + $this_source = $this->getContentSource()->getSource(); + } + + foreach ($group as $xaction) { + // Don't group transactions by different authors. + if ($xaction->getAuthorPHID() != $this->getAuthorPHID()) { + return false; + } + + // Don't group transactions for different objects. + if ($xaction->getObjectPHID() != $this->getObjectPHID()) { + return false; + } + + // Don't group anything into a group which already has a comment. + if ($xaction->getTransactionType() == $type_comment) { + return false; + } + + // Don't group transactions from different content sources. + $other_source = null; + if ($xaction->getContentSource()) { + $other_source = $xaction->getContentSource()->getSource(); + } + + if ($other_source != $this_source) { + return false; + } + + // Don't group transactions which happened more than 2 minutes apart. + $apart = abs($xaction->getDateCreated() - $this->getDateCreated()); + if ($apart > (60 * 2)) { + return false; + } + } + + return true; + } + /* -( PhabricatorPolicyInterface Implementation )-------------------------- */ diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php index e64b5b86ff..aa1d856c12 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -61,91 +61,26 @@ class PhabricatorApplicationTransactionView extends AphrontView { $user = $this->getUser(); $anchor = $this->anchorOffset; - $events = array(); $xactions = $this->transactions; - foreach ($xactions as $key => $xaction) { - if ($xaction->shouldHide()) { - 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) - ->setTransactionPHID($xaction->getPHID()) - ->setUserHandle($xaction->getHandle($xaction->getAuthorPHID())) - ->setIcon($xaction->getIcon()) - ->setColor($xaction->getColor()); - - $title = $xaction->getTitle(); - if ($xaction->hasChangeDetails()) { - if ($this->isPreview || $this->isDetailView) { - $details = $this->buildChangeDetails($xaction); - } else { - $details = $this->buildChangeDetailsLink($xaction); - } - $title = array( - $title, - ' ', - $details, - ); - } - $event->setTitle($title); - - if ($this->isPreview) { - $event->setIsPreview(true); - } else { - $event - ->setDateCreated($xaction->getDateCreated()) - ->setContentSource($xaction->getContentSource()) - ->setAnchor($anchor); + $xactions = $this->filterHiddenTransactions($xactions); + $xactions = $this->groupRelatedTransactions($xactions); + $groups = $this->groupDisplayTransactions($xactions); + $events = array(); + foreach ($groups as $group) { + $group_event = null; + foreach ($group as $xaction) { + $event = $this->renderEvent($xaction, $group, $anchor); $anchor++; - } - - $has_deleted_comment = $xaction->getComment() && - $xaction->getComment()->getIsDeleted(); - - if ($this->getShowEditActions() && !$this->isPreview) { - if ($xaction->getCommentVersion() > 1) { - $event->setIsEdited(true); - } - - $can_edit = PhabricatorPolicyCapability::CAN_EDIT; - - if ($xaction->hasComment() || $has_deleted_comment) { - $has_edit_capability = PhabricatorPolicyFilter::hasCapability( - $user, - $xaction, - $can_edit); - if ($has_edit_capability) { - $event->setIsEditable(true); - } + if (!$group_event) { + $group_event = $event; + } else { + $group_event->addEventToGroup($event); } } - - $content = $this->renderTransactionContent($xaction); - if ($content) { - $event->appendChild($content); - } - - $events[] = $event; + $events[] = $group_event; } return $events; @@ -295,5 +230,148 @@ class PhabricatorApplicationTransactionView extends AphrontView { return null; } + private function filterHiddenTransactions(array $xactions) { + foreach ($xactions as $key => $xaction) { + if ($xaction->shouldHide()) { + unset($xactions[$key]); + } + } + return $xactions; + } + + private function groupRelatedTransactions(array $xactions) { + $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())); + } + + return $xactions; + } + + private function groupDisplayTransactions(array $xactions) { + $groups = array(); + $group = array(); + foreach ($xactions as $xaction) { + if ($xaction->shouldDisplayGroupWith($group)) { + $group[] = $xaction; + } else { + if ($group) { + $groups[] = $group; + } + $group = array($xaction); + } + } + + if ($group) { + $groups[] = $group; + } + + foreach ($groups as $key => $group) { + $group = msort($group, 'getActionStrength'); + $group = array_reverse($group); + $groups[$key] = $group; + } + + return $groups; + } + + private function renderEvent( + PhabricatorApplicationTransaction $xaction, + array $group, + $anchor) { + $viewer = $this->getUser(); + + $event = id(new PhabricatorTimelineEventView()) + ->setUser($viewer) + ->setTransactionPHID($xaction->getPHID()) + ->setUserHandle($xaction->getHandle($xaction->getAuthorPHID())) + ->setIcon($xaction->getIcon()) + ->setColor($xaction->getColor()); + + if (!$this->shouldSuppressTitle($xaction, $group)) { + $title = $xaction->getTitle(); + if ($xaction->hasChangeDetails()) { + if ($this->isPreview || $this->isDetailView) { + $details = $this->buildChangeDetails($xaction); + } else { + $details = $this->buildChangeDetailsLink($xaction); + } + $title = array( + $title, + ' ', + $details, + ); + } + $event->setTitle($title); + } + + if ($this->isPreview) { + $event->setIsPreview(true); + } else { + $event + ->setDateCreated($xaction->getDateCreated()) + ->setContentSource($xaction->getContentSource()) + ->setAnchor($anchor); + } + + $has_deleted_comment = $xaction->getComment() && + $xaction->getComment()->getIsDeleted(); + + if ($this->getShowEditActions() && !$this->isPreview) { + if ($xaction->getCommentVersion() > 1) { + $event->setIsEdited(true); + } + + $can_edit = PhabricatorPolicyCapability::CAN_EDIT; + + if ($xaction->hasComment() || $has_deleted_comment) { + $has_edit_capability = PhabricatorPolicyFilter::hasCapability( + $viewer, + $xaction, + $can_edit); + if ($has_edit_capability) { + $event->setIsEditable(true); + } + } + } + + $content = $this->renderTransactionContent($xaction); + if ($content) { + $event->appendChild($content); + } + + return $event; + } + + private function shouldSuppressTitle( + PhabricatorApplicationTransaction $xaction, + array $group) { + + // This is a little hard-coded, but we don't have any other reasonable + // cases for now. Suppress "commented on" if there are other actions in + // the display group. + + if (count($group) > 1) { + $type_comment = PhabricatorTransactions::TYPE_COMMENT; + if ($xaction->getTransactionType() == $type_comment) { + return true; + } + } + + return false; + } + } diff --git a/src/view/layout/PhabricatorTimelineEventView.php b/src/view/layout/PhabricatorTimelineEventView.php index f7999c7e91..54f226dbbb 100644 --- a/src/view/layout/PhabricatorTimelineEventView.php +++ b/src/view/layout/PhabricatorTimelineEventView.php @@ -119,15 +119,17 @@ final class PhabricatorTimelineEventView extends AphrontView { $extra = array(); $is_first_extra = true; foreach ($this->getEventGroup() as $event) { - $extra[] = $this->renderExtra($is_first_extra); + $extra[] = $event->renderExtra($is_first_extra); $is_first_extra = false; } + $extra = array_reverse($extra); + $extra = array_mergev($extra); $extra = phutil_tag( 'span', array( 'class' => 'phabricator-timeline-extra', ), - phutil_implode_html(" \xC2\xB7 ", array_mergev($extra))); + phutil_implode_html(" \xC2\xB7 ", $extra)); } else { $extra = null; }