From f311f3f910e31b72345e37bf2e5bea0f278fd111 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 4 May 2015 16:11:20 -0700 Subject: [PATCH] Fix a handle-related fatal in Conpherence I'll file an issue for this, but it locked up my UI since I've got a mention in a recent thread. Auditors: btrahan --- .../PhabricatorConpherenceThreadPHIDType.php | 11 +- .../conpherence/storage/ConpherenceThread.php | 131 ++++++++++-------- .../view/ConpherenceDurableColumnView.php | 4 +- .../view/ConpherenceThreadListView.php | 2 +- 4 files changed, 81 insertions(+), 67 deletions(-) diff --git a/src/applications/conpherence/phid/PhabricatorConpherenceThreadPHIDType.php b/src/applications/conpherence/phid/PhabricatorConpherenceThreadPHIDType.php index 86dbad6307..ccc2756557 100644 --- a/src/applications/conpherence/phid/PhabricatorConpherenceThreadPHIDType.php +++ b/src/applications/conpherence/phid/PhabricatorConpherenceThreadPHIDType.php @@ -32,10 +32,13 @@ final class PhabricatorConpherenceThreadPHIDType extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $thread = $objects[$phid]; - $data = $thread->getDisplayData($query->getViewer()); - $handle->setName($data['title']); - $handle->setFullName($data['title']); - $handle->setURI('/'.$thread->getMonogram()); + + $title = $thread->getDisplayTitle($query->getViewer()); + $monogram = $thread->getMonogram(); + + $handle->setName($title); + $handle->setFullName(pht('%s: %s', $monogram, $title)); + $handle->setURI('/'.$monogram); } } diff --git a/src/applications/conpherence/storage/ConpherenceThread.php b/src/applications/conpherence/storage/ConpherenceThread.php index a705c450bf..03f8f6997e 100644 --- a/src/applications/conpherence/storage/ConpherenceThread.php +++ b/src/applications/conpherence/storage/ConpherenceThread.php @@ -199,19 +199,76 @@ final class ConpherenceThread extends ConpherenceDAO return PhabricatorUser::getDefaultProfileImageURI(); } - public function getDisplayData(PhabricatorUser $user) { + + /** + * Get the thread's display title for a user. + * + * If a thread doesn't have a title set, this will return a string describing + * recent participants. + * + * @param PhabricatorUser Viewer. + * @return string Thread title. + */ + public function getDisplayTitle(PhabricatorUser $viewer) { + $title = $this->getTitle(); + if (strlen($title)) { + return $title; + } + + return $this->getRecentParticipantsString($viewer); + } + + + /** + * Get recent participants (other than the viewer) as a string. + * + * For example, this method might return "alincoln, htaft, gwashington...". + * + * @param PhabricatorUser Viewer. + * @return string Description of other participants. + */ + private function getRecentParticipantsString(PhabricatorUser $viewer) { + $handles = $this->getHandles(); + $phids = $this->getOtherRecentParticipantPHIDs($viewer); + + $limit = 3; + $more = (count($phids) > $limit); + $phids = array_slice($phids, 0, $limit); + + $names = array_select_keys($handles, $phids); + $names = mpull($names, 'getName'); + $names = implode(', ', $names); + + if ($more) { + $names = $names.'...'; + } + + return $names; + } + + + /** + * Get PHIDs for recent participants who are not the viewer. + * + * @param PhabricatorUser Viewer. + * @return list Participants who are not the viewer. + */ + private function getOtherRecentParticipantPHIDs(PhabricatorUser $viewer) { + $phids = $this->getRecentParticipantPHIDs(); + $phids = array_fuse($phids); + unset($phids[$viewer->getPHID()]); + return array_values($phids); + } + + + public function getDisplayData(PhabricatorUser $viewer) { + $handles = $this->getHandles(); + if ($this->hasAttachedTransactions()) { $transactions = $this->getTransactions(); } else { $transactions = array(); } - $set_title = $this->getTitle(); - - if ($set_title) { - $title_mode = 'title'; - } else { - $title_mode = 'recent'; - } if ($transactions) { $subtitle_mode = 'message'; @@ -219,22 +276,7 @@ final class ConpherenceThread extends ConpherenceDAO $subtitle_mode = 'recent'; } - $recent_phids = $this->getRecentParticipantPHIDs(); - $handles = $this->getHandles(); - // Luck has little to do with it really; most recent participant who - // isn't the user.... - $lucky_phid = null; - $lucky_index = null; - $recent_title = null; - foreach ($recent_phids as $index => $phid) { - if ($phid == $user->getPHID()) { - continue; - } - $lucky_phid = $phid; - break; - } - reset($recent_phids); - + $lucky_phid = head($this->getOtherRecentParticipantPHIDs($viewer)); if ($lucky_phid) { $lucky_handle = $handles[$lucky_phid]; } else { @@ -250,38 +292,6 @@ final class ConpherenceThread extends ConpherenceDAO $img_src = $lucky_handle->getImageURI(); } - if ($title_mode == 'recent' || $subtitle_mode == 'recent') { - $count = 0; - $final = false; - foreach ($recent_phids as $phid) { - if ($phid == $user->getPHID()) { - continue; - } - $handle = $handles[$phid]; - if ($recent_title) { - if ($final) { - $recent_title .= '...'; - break; - } else { - $recent_title .= ', '; - } - } - $recent_title .= $handle->getName(); - $count++; - $final = $count == 3; - } - } - - switch ($title_mode) { - case 'recent': - $title = $recent_title; - $js_title = $recent_title; - break; - case 'title': - $title = $js_title = $this->getTitle(); - break; - } - $message_title = null; if ($subtitle_mode == 'message') { $message_transaction = null; @@ -307,18 +317,18 @@ final class ConpherenceThread extends ConpherenceDAO } switch ($subtitle_mode) { case 'recent': - $subtitle = $recent_title; + $subtitle = $this->getRecentParticipantsString($viewer); break; case 'message': if ($message_title) { $subtitle = $message_title; } else { - $subtitle = $recent_title; + $subtitle = $this->getRecentParticipantsString($viewer); } break; } - $user_participation = $this->getParticipantIfExists($user->getPHID()); + $user_participation = $this->getParticipantIfExists($viewer->getPHID()); if ($user_participation) { $user_seen_count = $user_participation->getSeenMessageCount(); } else { @@ -326,9 +336,10 @@ final class ConpherenceThread extends ConpherenceDAO } $unread_count = $this->getMessageCount() - $user_seen_count; + $title = $this->getDisplayTitle($viewer); + return array( 'title' => $title, - 'js_title' => $js_title, 'subtitle' => $subtitle, 'unread_count' => $unread_count, 'epoch' => $this->getDateModified(), diff --git a/src/applications/conpherence/view/ConpherenceDurableColumnView.php b/src/applications/conpherence/view/ConpherenceDurableColumnView.php index d9462b1281..f52f48c8e6 100644 --- a/src/applications/conpherence/view/ConpherenceDurableColumnView.php +++ b/src/applications/conpherence/view/ConpherenceDurableColumnView.php @@ -251,7 +251,7 @@ final class ConpherenceDurableColumnView extends AphrontTagView { array(), array( $icon, - $data['js_title'], + $data['title'], )); $image = $data['image']; Javelin::initBehavior('phabricator-tooltips'); @@ -265,7 +265,7 @@ final class ConpherenceDurableColumnView extends AphrontTagView { 'meta' => array( 'threadID' => $conpherence->getID(), 'threadTitle' => hsprintf('%s', $thread_title), - 'tip' => $data['js_title'], + 'tip' => $data['title'], 'align' => 'S', ), ), diff --git a/src/applications/conpherence/view/ConpherenceThreadListView.php b/src/applications/conpherence/view/ConpherenceThreadListView.php index 532bb799b4..be8c6e4558 100644 --- a/src/applications/conpherence/view/ConpherenceThreadListView.php +++ b/src/applications/conpherence/view/ConpherenceThreadListView.php @@ -111,7 +111,7 @@ final class ConpherenceThreadListView extends AphrontView { ->addSigil('conpherence-menu-click') ->setMetadata( array( - 'title' => $glyph.$data['js_title'], + 'title' => $glyph.$data['title'], 'id' => $dom_id, 'threadID' => $thread->getID(), ));