From cebc7f4e8bc55b11715e32cebd2f2b298a01f452 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 19 Mar 2014 19:29:48 -0700
Subject: [PATCH] Show profile pictures in subscribers dialog

Summary: Ref T4400. Also stops rendering "and 1 other" in subscriber lists, since it looks a bit silly in practice (we can just put the other subscriber there instead). Don't do the "and x others" until X is at least 2.

Test Plan: Viewed/clicked subscriber lists and transactions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4400

Differential Revision: https://secure.phabricator.com/D8573
---
 .../view/SubscriptionListDialogBuilder.php    |  8 +++--
 .../view/SubscriptionListStringBuilder.php    | 30 +++++++++++--------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php
index ba3c1ec8f8..b8b7cb5772 100644
--- a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php
+++ b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php
@@ -56,7 +56,7 @@ final class SubscriptionListDialogBuilder {
       ->setClass('subscriber-list-dialog')
       ->setTitle($this->getTitle())
       ->appendChild($this->buildBody($this->getViewer(), $handles))
-      ->addCancelButton($object_handle->getURI(), pht('Dismiss'));
+      ->addCancelButton($object_handle->getURI(), pht('Close'));
   }
 
   private function buildBody(PhabricatorUser $viewer, array $handles) {
@@ -64,11 +64,15 @@ final class SubscriptionListDialogBuilder {
     $list = id(new PHUIObjectItemListView())
       ->setUser($viewer);
     foreach ($handles as $handle) {
-      // TODO - include $handle image - T4400
       $item = id(new PHUIObjectItemView())
         ->setHeader($handle->getFullName())
         ->setHref($handle->getURI())
         ->setDisabled($handle->isDisabled());
+
+      if ($handle->getImageURI()) {
+        $item->setImageURI($handle->getImageURI());
+      }
+
       $list->addItem($item);
     }
 
diff --git a/src/applications/subscriptions/view/SubscriptionListStringBuilder.php b/src/applications/subscriptions/view/SubscriptionListStringBuilder.php
index 71564b994b..a10ffa5d6d 100644
--- a/src/applications/subscriptions/view/SubscriptionListStringBuilder.php
+++ b/src/applications/subscriptions/view/SubscriptionListStringBuilder.php
@@ -48,25 +48,24 @@ final class SubscriptionListStringBuilder {
   private function buildString($list_uri) {
     $handles = $this->getHandles();
 
-    $html = array();
+    // Always show this many subscribers.
     $show_count = 3;
     $subscribers_count = count($handles);
-    if ($subscribers_count <= $show_count) {
+
+    // It looks a bit silly to render "a, b, c, and 1 other", since we could
+    // have just put that other subscriber there in place of the "1 other"
+    // link. Instead, render "a, b, c, d" in this case, and then when we get one
+    // more render "a, b, c, and 2 others".
+    if ($subscribers_count <= ($show_count + 1)) {
       return phutil_implode_html(', ', mpull($handles, 'renderLink'));
     }
 
-    $args = array('%s, %s, %s, and %s');
-    $shown = 0;
-    foreach ($handles as $handle) {
-      $shown++;
-      if ($shown > $show_count) {
-        break;
-      }
-      $args[] = $handle->renderLink();
-    }
+    $show = array_slice($handles, 0, $show_count);
+    $show = array_values($show);
+
     $not_shown_count = $subscribers_count - $show_count;
     $not_shown_txt = pht('%d other(s)', $not_shown_count);
-    $args[] = javelin_tag(
+    $not_shown_link = javelin_tag(
       'a',
       array(
         'href' => $list_uri,
@@ -74,7 +73,12 @@ final class SubscriptionListStringBuilder {
       ),
       $not_shown_txt);
 
-    return call_user_func_array('pht', $args);
+    return pht(
+      '%s, %s, %s and %s',
+      $show[0]->renderLink(),
+      $show[1]->renderLink(),
+      $show[2]->renderLink(),
+      $not_shown_link);
   }
 
 }