Improve consistency and Harbormaster integration of Diffusion

Summary:
Ref T9123. Two major Harbormaster-related UI changes in Diffusion:

  - Tags table now shows tag build status.
  - Branches table now shows branch build status.

Then some minor consistency / qualtiy of life changes:

  - Picked a nicer looking "history" icon?
  - Branches table now uses the same "history" icon as other tables.
  - Tags table now has a "history" link.
  - Browse table now has a "history" link.
  - Dates now use more consistent formatting.
  - Column order is now more consistent.
  - Use of style is now more consistent.

Test Plan:
{F865056}

{F865057}

{F865058}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9123

Differential Revision: https://secure.phabricator.com/D14242

src/applications/diffusion/controller/DiffusionLastModifiedController.php

@@ -98,12 +98,10 @@ final class DiffusionLastModifiedController extends DiffusionController {
       $modified = DiffusionView::linkCommit(
         $drequest->getRepository(),
         $commit->getCommitIdentifier());
-      $date = phabricator_date($epoch, $viewer);
+      $date = phabricator_datetime($epoch, $viewer);
-      $time = phabricator_time($epoch, $viewer);
     } else {
       $modified = '';
       $date = '';
-      $time = '';
     }
 
     $data = $commit->getCommitData();
@@ -137,7 +135,6 @@ final class DiffusionLastModifiedController extends DiffusionController {
     $return = array(
       'commit'    => $modified,
       'date'      => $date,
-      'time'      => $time,
       'author'    => $author,
       'details'   => $details,
     );

src/applications/diffusion/view/DiffusionBranchTableView.php

@@ -21,6 +21,11 @@ final class DiffusionBranchTableView extends DiffusionView {
     $drequest = $this->getDiffusionRequest();
     $current_branch = $drequest->getBranch();
     $repository = $drequest->getRepository();
+    $commits = $this->commits;
+    $viewer = $this->getUser();
+
+    $buildables = $this->loadBuildables($commits);
+    $have_builds = false;
 
     $can_close_branches = ($repository->isHg());
 
@@ -31,13 +36,21 @@ final class DiffusionBranchTableView extends DiffusionView {
     $rows = array();
     $rowc = array();
     foreach ($this->branches as $branch) {
-      $commit = idx($this->commits, $branch->getCommitIdentifier());
+      $commit = idx($commits, $branch->getCommitIdentifier());
       if ($commit) {
         $details = $commit->getSummary();
-        $datetime = phabricator_datetime($commit->getEpoch(), $this->user);
+        $datetime = phabricator_datetime($commit->getEpoch(), $viewer);
+        $buildable = idx($buildables, $commit->getPHID());
+        if ($buildable) {
+          $build_status = $this->renderBuildable($buildable);
+          $have_builds = true;
+        } else {
+          $build_status = null;
+        }
       } else {
         $datetime = null;
         $details = null;
+        $build_status = null;
       }
 
       switch ($repository->shouldSkipAutocloseBranch($branch->getShortName())) {
@@ -86,16 +99,7 @@ final class DiffusionBranchTableView extends DiffusionView {
       }
 
       $rows[] = array(
-        phutil_tag(
+        $this->linkBranchHistory($branch->getShortName()),
-          'a',
-          array(
-            'href' => $drequest->generateURI(
-              array(
-                'action' => 'history',
-                'branch' => $branch->getShortName(),
-              )),
-          ),
-          pht('History')),
         phutil_tag(
           'a',
           array(
@@ -109,10 +113,11 @@ final class DiffusionBranchTableView extends DiffusionView {
         self::linkCommit(
           $drequest->getRepository(),
           $branch->getCommitIdentifier()),
+        $build_status,
         $status,
+        AphrontTableView::renderSingleDisplayLine($details),
         $status_icon,
         $datetime,
-        AphrontTableView::renderSingleDisplayLine($details),
       );
       if ($branch->getShortName() == $current_branch) {
         $rowc[] = 'highlighted';
@@ -124,33 +129,37 @@ final class DiffusionBranchTableView extends DiffusionView {
     $view = new AphrontTableView($rows);
     $view->setHeaders(
       array(
-        pht('History'),
+        null,
         pht('Branch'),
         pht('Head'),
+        null,
         pht('State'),
-        pht(''),
-        pht('Modified'),
         pht('Details'),
+        null,
+        pht('Committed'),
       ));
     $view->setColumnClasses(
       array(
         '',
         'pri',
         '',
-        '',
+        'icon',
-        '',
         '',
         'wide',
+        '',
+        '',
       ));
     $view->setColumnVisibility(
       array(
         true,
         true,
         true,
+        $have_builds,
         $can_close_branches,
       ));
     $view->setRowClasses($rowc);
     return $view->render();
   }
 
+
 }

src/applications/diffusion/view/DiffusionBrowseTableView.php

@@ -31,6 +31,8 @@ final class DiffusionBrowseTableView extends DiffusionView {
     $show_edit = false;
     foreach ($this->paths as $path) {
 
+      $history_link = $this->linkHistory($path->getPath());
+
       $dir_slash = null;
       $file_type = $path->getFileType();
       if ($file_type == DifferentialChangeType::FILE_DIRECTORY) {
@@ -67,7 +69,6 @@ final class DiffusionBrowseTableView extends DiffusionView {
         'lint'      => celerity_generate_unique_node_id(),
         'commit'    => celerity_generate_unique_node_id(),
         'date'      => celerity_generate_unique_node_id(),
-        'time'      => celerity_generate_unique_node_id(),
         'author'    => celerity_generate_unique_node_id(),
         'details'   => celerity_generate_unique_node_id(),
       );
@@ -78,13 +79,13 @@ final class DiffusionBrowseTableView extends DiffusionView {
       }
 
       $rows[] = array(
+        $history_link,
         $browse_link,
         idx($dict, 'lint'),
         $dict['commit'],
         $dict['author'],
         $dict['details'],
         $dict['date'],
-        $dict['time'],
       );
     }
 
@@ -108,29 +109,29 @@ final class DiffusionBrowseTableView extends DiffusionView {
     $view = new AphrontTableView($rows);
     $view->setHeaders(
       array(
+        null,
         pht('Path'),
         ($lint ? $lint : pht('Lint')),
         pht('Modified'),
         pht('Author/Committer'),
         pht('Details'),
-        pht('Date'),
+        pht('Committed'),
-        pht('Time'),
       ));
     $view->setColumnClasses(
       array(
+        'nudgeright',
+        '',
+        '',
         '',
-        'n',
-        'n',
         '',
         'wide',
         '',
-        'right',
       ));
     $view->setColumnVisibility(
       array(
         true,
-        $show_lint,
         true,
+        $show_lint,
         true,
         true,
         true,
@@ -140,11 +141,11 @@ final class DiffusionBrowseTableView extends DiffusionView {
     $view->setDeviceVisibility(
       array(
         true,
-        false,
         true,
         false,
         true,
         false,
+        true,
         false,
       ));
 

src/applications/diffusion/view/DiffusionHistoryTableView.php

@@ -7,12 +7,10 @@ final class DiffusionHistoryTableView extends DiffusionView {
   private $handles = array();
   private $isHead;
   private $parents;
-  private $buildCache;
 
   public function setHistory(array $history) {
     assert_instances_of($history, 'DiffusionPathChange');
     $this->history = $history;
-    $this->buildCache = null;
     return $this;
   }
 
@@ -62,33 +60,14 @@ final class DiffusionHistoryTableView extends DiffusionView {
     return $this;
   }
 
-  public function loadBuildablesOnDemand() {
-    if ($this->buildCache !== null) {
-      return $this->buildCache;
-    }
-
-    $commits_to_builds = array();
-
-    $commits = mpull($this->history, 'getCommit');
-
-    $commit_phids = mpull($commits, 'getPHID');
-
-    $buildables = id(new HarbormasterBuildableQuery())
-      ->setViewer($this->getUser())
-      ->withBuildablePHIDs($commit_phids)
-      ->withManualBuildables(false)
-      ->execute();
-
-    $this->buildCache = mpull($buildables, null, 'getBuildablePHID');
-
-    return $this->buildCache;
-  }
-
   public function render() {
     $drequest = $this->getDiffusionRequest();
 
     $viewer = $this->getUser();
 
+    $buildables = $this->loadBuildables(mpull($this->history, 'getCommit'));
+    $has_any_build = false;
+
     $show_revisions = PhabricatorApplication::isClassInstalledForViewer(
       'PhabricatorDifferentialApplication',
       $viewer);
@@ -110,11 +89,9 @@ final class DiffusionHistoryTableView extends DiffusionView {
       $epoch = $history->getEpoch();
 
       if ($epoch) {
-        $date = phabricator_date($epoch, $this->user);
+        $committed = phabricator_datetime($epoch, $viewer);
-        $time = phabricator_time($epoch, $this->user);
       } else {
-        $date = null;
+        $committed = null;
-        $time = null;
       }
 
       $data = $history->getCommitData();
@@ -160,36 +137,9 @@ final class DiffusionHistoryTableView extends DiffusionView {
 
       $build = null;
       if ($show_builds) {
-        $buildable_lookup = $this->loadBuildablesOnDemand();
+        $buildable = idx($buildables, $commit->getPHID());
-        $buildable = idx($buildable_lookup, $commit->getPHID());
         if ($buildable !== null) {
-          $icon = HarbormasterBuildable::getBuildableStatusIcon(
+          $build = $this->renderBuildable($buildable);
-            $buildable->getBuildableStatus());
-          $color = HarbormasterBuildable::getBuildableStatusColor(
-            $buildable->getBuildableStatus());
-          $name = HarbormasterBuildable::getBuildableStatusName(
-            $buildable->getBuildableStatus());
-
-          $icon_view = id(new PHUIIconView())
-            ->setIconFont($icon.' '.$color);
-
-          $tooltip_view = javelin_tag(
-            'span',
-            array(
-              'sigil' => 'has-tooltip',
-              'meta' => array('tip' => $name),
-            ),
-            $icon_view);
-
-          Javelin::initBehavior('phabricator-tooltips');
-
-          $href_view = phutil_tag(
-            'a',
-            array('href' => '/'.$buildable->getMonogram()),
-            $tooltip_view);
-
-          $build = $href_view;
-
           $has_any_build = true;
         }
       }
@@ -214,8 +164,7 @@ final class DiffusionHistoryTableView extends DiffusionView {
           null),
         $author,
         $summary,
-        $date,
+        $committed,
-        $time,
       );
     }
 
@@ -226,30 +175,28 @@ final class DiffusionHistoryTableView extends DiffusionView {
         null,
         pht('Commit'),
         null,
-        pht('Revision'),
+        null,
         pht('Author/Committer'),
         pht('Details'),
-        pht('Date'),
+        pht('Committed'),
-        pht('Time'),
       ));
     $view->setColumnClasses(
       array(
         'threads',
         'nudgeright',
-        'n',
+        '',
         'icon',
-        'n',
+        '',
         '',
         'wide',
         '',
-        'right',
       ));
     $view->setColumnVisibility(
       array(
         $graph ? true : false,
         true,
         true,
-        true,
+        $has_any_build,
         $show_revisions,
       ));
     $view->setDeviceVisibility(
@@ -262,7 +209,6 @@ final class DiffusionHistoryTableView extends DiffusionView {
         false,
         true,
         false,
-        false,
       ));
     return $view->render();
   }

src/applications/diffusion/view/DiffusionTagListView.php

@@ -29,6 +29,8 @@ final class DiffusionTagListView extends DiffusionView {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
 
+    $buildables = $this->loadBuildables($this->commits);
+    $has_builds = false;
 
     $rows = array();
     foreach ($this->tags as $tag) {
@@ -80,30 +82,56 @@ final class DiffusionTagListView extends DiffusionView {
         }
       }
 
+      $build = null;
+      if ($commit) {
+        $buildable = idx($buildables, $commit->getPHID());
+        if ($buildable) {
+          $build = $this->renderBuildable($buildable);
+          $has_builds = true;
+        }
+      }
+
+      $history = $this->linkTagHistory($tag->getName());
+
       $rows[] = array(
+        $history,
         $tag_link,
         $commit_link,
-        $description,
+        $build,
         $author,
+        $description,
         phabricator_datetime($tag->getEpoch(), $this->user),
       );
     }
 
-    $table = new AphrontTableView($rows);
+    $table = id(new AphrontTableView($rows))
-    $table->setHeaders(
+      ->setHeaders(
-      array(
+        array(
-        pht('Tag'),
+          null,
-        pht('Commit'),
+          pht('Tag'),
-        pht('Description'),
+          pht('Commit'),
-        pht('Author'),
+          null,
-        pht('Created'),
+          pht('Author'),
-      ));
+          pht('Description'),
-    $table->setColumnClasses(
+          pht('Created'),
-      array(
+        ))
-        'pri',
+      ->setColumnClasses(
-        '',
+        array(
-        'wide',
+          'nudgeright',
-      ));
+          'pri',
+          '',
+          '',
+          '',
+          'wide',
+        ))
+      ->setColumnVisibility(
+        array(
+          true,
+          true,
+          true,
+          $has_builds,
+        ));
+
     return $table->render();
   }
 

src/applications/diffusion/view/DiffusionView.php

@@ -20,6 +20,30 @@ abstract class DiffusionView extends AphrontView {
         'path'   => $path,
       ));
 
+    return $this->renderHistoryLink($href);
+  }
+
+  final public function linkBranchHistory($branch) {
+    $href = $this->getDiffusionRequest()->generateURI(
+      array(
+        'action' => 'history',
+        'branch' => $branch,
+      ));
+
+    return $this->renderHistoryLink($href);
+  }
+
+  final public function linkTagHistory($tag) {
+    $href = $this->getDiffusionRequest()->generateURI(
+      array(
+        'action' => 'history',
+        'commit' => $tag,
+      ));
+
+    return $this->renderHistoryLink($href);
+  }
+
+  private function renderHistoryLink($href) {
     return javelin_tag(
       'a',
       array(
@@ -31,7 +55,7 @@ abstract class DiffusionView extends AphrontView {
           'align' => 'E',
         ),
       ),
-      id(new PHUIIconView())->setIconFont('fa-list-ul blue'));
+      id(new PHUIIconView())->setIconFont('fa-history bluegrey'));
   }
 
   final public function linkBrowse($path, array $details = array()) {
@@ -170,4 +194,58 @@ abstract class DiffusionView extends AphrontView {
     return hsprintf('%s', $name);
   }
 
+  final protected function renderBuildable(HarbormasterBuildable $buildable) {
+    $status = $buildable->getBuildableStatus();
+
+    $icon = HarbormasterBuildable::getBuildableStatusIcon($status);
+    $color = HarbormasterBuildable::getBuildableStatusColor($status);
+    $name = HarbormasterBuildable::getBuildableStatusName($status);
+
+    $icon_view = id(new PHUIIconView())
+      ->setIconFont($icon.' '.$color);
+
+    $tooltip_view = javelin_tag(
+      'span',
+      array(
+        'sigil' => 'has-tooltip',
+        'meta' => array('tip' => $name),
+      ),
+      $icon_view);
+
+    Javelin::initBehavior('phabricator-tooltips');
+
+    return phutil_tag(
+      'a',
+      array('href' => '/'.$buildable->getMonogram()),
+      $tooltip_view);
+  }
+
+  final protected function loadBuildables(array $commits) {
+    assert_instances_of($commits, 'PhabricatorRepositoryCommit');
+
+    if (!$commits) {
+      return array();
+    }
+
+    $viewer = $this->getUser();
+
+    $harbormaster_app = 'PhabricatorHarbormasterApplication';
+    $have_harbormaster = PhabricatorApplication::isClassInstalledForViewer(
+      $harbormaster_app,
+      $viewer);
+
+    if ($have_harbormaster) {
+      $buildables = id(new HarbormasterBuildableQuery())
+        ->setViewer($viewer)
+        ->withBuildablePHIDs(mpull($commits, 'getPHID'))
+        ->withManualBuildables(false)
+        ->execute();
+      $buildables = mpull($buildables, null, 'getBuildablePHID');
+    } else {
+      $buildables = array();
+    }
+
+    return $buildables;
+  }
+
 }