Drive differential revision list with custom fields

Summary:
Build the revision list table out of custom fields instead of hard-coding it, so
installs can add all sorts of zany things to it.

NOTE: You may need to implement sortFieldsForRevisionList() if you have a custom
DifferentialFieldSelector, or some fields might show up out of order.

This implementation will preserve the expected behavior:

  public function sortFieldsForRevisionList(array $fields) {
    $default = new DifferentialDefaultFieldSelector();
    return $default->sortFieldsForRevisionList($fields);
  }

Test Plan:
  - Loaded differential revision list, identical to old list.
  - Profiled page to verify the cost increase isn't significant (it's quite
small).

Reviewers: jungejason, btrahan

Reviewed By: btrahan

CC: aran, btrahan, davidreuss, epriestley

Maniphest Tasks: T773, T729

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

src/__phutil_library_map__.php

@@ -204,6 +204,8 @@ phutil_register_library_map(array(
     'DifferentialCommitsFieldSpecification' => 'applications/differential/field/specification/commits',
     'DifferentialController' => 'applications/differential/controller/base',
     'DifferentialDAO' => 'applications/differential/storage/base',
+    'DifferentialDateCreatedFieldSpecification' => 'applications/differential/field/specification/datecreated',
+    'DifferentialDateModifiedFieldSpecification' => 'applications/differential/field/specification/datemodified',
     'DifferentialDefaultFieldSelector' => 'applications/differential/field/selector/default',
     'DifferentialDependenciesFieldSpecification' => 'applications/differential/field/specification/dependencies',
     'DifferentialDiff' => 'applications/differential/storage/diff',
@@ -1033,6 +1035,8 @@ phutil_register_library_map(array(
     'DifferentialCommitsFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialController' => 'PhabricatorController',
     'DifferentialDAO' => 'PhabricatorLiskDAO',
+    'DifferentialDateCreatedFieldSpecification' => 'DifferentialFieldSpecification',
+    'DifferentialDateModifiedFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialDefaultFieldSelector' => 'DifferentialFieldSelector',
     'DifferentialDependenciesFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialDiff' => 'DifferentialDAO',

src/applications/differential/controller/revisionlist/DifferentialRevisionListController.php

@@ -366,6 +366,10 @@ class DifferentialRevisionListController extends DifferentialController {
   private function buildViews($filter, $user_phid, array $revisions) {
     $user = $this->getRequest()->getUser();
 
+    $template = id(new DifferentialRevisionListView())
+      ->setUser($user)
+      ->setFields(DifferentialRevisionListView::getDefaultFields());
+
     $views = array();
     switch ($filter) {
       case 'active':
@@ -373,18 +377,16 @@ class DifferentialRevisionListController extends DifferentialController {
           $revisions,
           $user_phid);
 
-        $view = id(new DifferentialRevisionListView())
+        $view = id(clone $template)
           ->setRevisions($active)
-          ->setUser($user)
           ->setNoDataString("You have no active revisions requiring action.");
         $views[] = array(
           'title' => 'Action Required',
           'view'  => $view,
         );
 
-        $view = id(new DifferentialRevisionListView())
+        $view = id(clone $template)
           ->setRevisions($waiting)
-          ->setUser($user)
           ->setNoDataString("You have no active revisions waiting on others.");
         $views[] = array(
           'title' => 'Waiting On Others',
@@ -401,9 +403,8 @@ class DifferentialRevisionListController extends DifferentialController {
           'subscribed'  => 'Revisions by Subscriber',
           'all'         => 'Revisions',
         );
-        $view = id(new DifferentialRevisionListView())
+        $view = id(clone $template)
-          ->setRevisions($revisions)
+          ->setRevisions($revisions);
-          ->setUser($user);
         $views[] = array(
           'title' => idx($titles, $filter),
           'view'  => $view,
@@ -412,6 +413,7 @@ class DifferentialRevisionListController extends DifferentialController {
       default:
         throw new Exception("Unknown filter '{$filter}'!");
     }
+
     return $views;
   }
 

src/applications/differential/field/selector/base/DifferentialFieldSelector.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -30,4 +30,8 @@ abstract class DifferentialFieldSelector {
 
   abstract public function getFieldSpecifications();
 
+  public function sortFieldsForRevisionList(array $fields) {
+    return $fields;
+  }
+
 }

src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php

@@ -53,9 +53,33 @@ final class DifferentialDefaultFieldSelector
         new DifferentialApplyPatchFieldSpecification(),
         new DifferentialRevisionIDFieldSpecification(),
         new DifferentialGitSVNIDFieldSpecification(),
+        new DifferentialDateModifiedFieldSpecification(),
+        new DifferentialDateCreatedFieldSpecification(),
       ));
 
     return $fields;
   }
 
+  public function sortFieldsForRevisionList(array $fields) {
+    $map = array();
+    foreach ($fields as $field) {
+      $map[get_class($field)] = $field;
+    }
+
+    $map = array_select_keys(
+      $map,
+      array(
+        'DifferentialRevisionIDFieldSpecification',
+        'DifferentialTitleFieldSpecification',
+        'DifferentialRevisionStatusFieldSpecification',
+        'DifferentialAuthorFieldSpecification',
+        'DifferentialReviewersFieldSpecification',
+        'DifferentialDateModifiedFieldSpecification',
+        'DifferentialDateCreatedFieldSpecification',
+      )) + $map;
+
+    return array_values($map);
+  }
+
 }
+

src/applications/differential/field/selector/default/__init__.php

@@ -13,6 +13,8 @@ phutil_require_module('phabricator', 'applications/differential/field/specificat
 phutil_require_module('phabricator', 'applications/differential/field/specification/branch');
 phutil_require_module('phabricator', 'applications/differential/field/specification/ccs');
 phutil_require_module('phabricator', 'applications/differential/field/specification/commits');
+phutil_require_module('phabricator', 'applications/differential/field/specification/datecreated');
+phutil_require_module('phabricator', 'applications/differential/field/specification/datemodified');
 phutil_require_module('phabricator', 'applications/differential/field/specification/dependencies');
 phutil_require_module('phabricator', 'applications/differential/field/specification/gitsvnid');
 phutil_require_module('phabricator', 'applications/differential/field/specification/host');
@@ -29,5 +31,7 @@ phutil_require_module('phabricator', 'applications/differential/field/specificat
 phutil_require_module('phabricator', 'applications/differential/field/specification/unit');
 phutil_require_module('phabricator', 'infrastructure/env');
 
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('DifferentialDefaultFieldSelector.php');

src/applications/differential/field/specification/author/DifferentialAuthorFieldSpecification.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -43,4 +43,21 @@ final class DifferentialAuthorFieldSpecification
     return $revision->getAuthorPHID();
   }
 
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'Author';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    return $this->getHandle($revision->getAuthorPHID())->renderLink();
+  }
+
+  public function getRequiredHandlePHIDsForRevisionList(
+    DifferentialRevision $revision) {
+    return array($revision->getAuthorPHID());
+  }
+
 }

src/applications/differential/field/specification/base/DifferentialFieldSpecification.php

@@ -27,6 +27,7 @@
  * @task storage Field Storage
  * @task edit Extending the Revision Edit Interface
  * @task view Extending the Revision View Interface
+ * @task list Extending the Revision List Interface
  * @task conduit Extending the Conduit View Interface
  * @task commit Extending Commit Messages
  * @task load Loading Additional Data
@@ -264,6 +265,59 @@ abstract class DifferentialFieldSpecification {
   }
 
 
+/* -(  Extending the Revision List Interface  )------------------------------ */
+
+
+  /**
+   * Determine if this field should appear in the table on the revision list
+   * interface.
+   *
+   * @return bool True if this field should appear in the table.
+   *
+   * @task list
+   */
+  public function shouldAppearOnRevisionList() {
+    return false;
+  }
+
+
+  /**
+   * Return a column header for revision list tables.
+   *
+   * @return string Column header.
+   *
+   * @task list
+   */
+  public function renderHeaderForRevisionList() {
+    throw new DifferentialFieldSpecificationIncompleteException($this);
+  }
+
+
+  /**
+   * Optionally, return a column class for revision list tables.
+   *
+   * @return string CSS class for table cells.
+   *
+   * @task list
+   */
+  public function getColumnClassForRevisionList() {
+    return null;
+  }
+
+
+  /**
+   * Return a table cell value for revision list tables.
+   *
+   * @param DifferentialRevision The revision to render a value for.
+   * @return string Table cell value.
+   *
+   * @task list
+   */
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    throw new DifferentialFieldSpecificationIncompleteException($this);
+  }
+
+
 /* -(  Extending the Conduit Interface  )------------------------------------ */
 
 
@@ -483,6 +537,7 @@ abstract class DifferentialFieldSpecification {
     return array();
   }
 
+
   /**
    * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your
    * field to render correctly on the view interface.
@@ -498,6 +553,25 @@ abstract class DifferentialFieldSpecification {
     return $this->getRequiredHandlePHIDs();
   }
 
+
+  /**
+   * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your
+   * field to render correctly on the list interface.
+   *
+   * This is a more specific version of @{method:getRequiredHandlePHIDs} which
+   * can be overridden to improve field performance by loading only data you
+   * need.
+   *
+   * @param DifferentialRevision The revision to pull PHIDs for.
+   * @return list List of PHIDs to load handles for.
+   * @task load
+   */
+  public function getRequiredHandlePHIDsForRevisionList(
+    DifferentialRevision $revision) {
+    return array();
+  }
+
+
   /**
    * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your
    * field to render correctly on the edit interface.

src/applications/differential/field/specification/datecreated/DifferentialDateCreatedFieldSpecification.php

@@ -0,0 +1,38 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialDateCreatedFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'Created';
+  }
+
+  public function getColumnClassForRevisionList() {
+    return 'right';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    return phabricator_date($revision->getDateCreated(), $this->getUser());
+  }
+
+}

src/applications/differential/field/specification/datecreated/__init__.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+phutil_require_module('phabricator', 'view/utils');
+
+
+phutil_require_source('DifferentialDateCreatedFieldSpecification.php');

src/applications/differential/field/specification/datemodified/DifferentialDateModifiedFieldSpecification.php

@@ -0,0 +1,38 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialDateModifiedFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'Updated';
+  }
+
+  public function getColumnClassForRevisionList() {
+    return 'right';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    return phabricator_datetime($revision->getDateModified(), $this->getUser());
+  }
+
+}

src/applications/differential/field/specification/datemodified/__init__.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+phutil_require_module('phabricator', 'view/utils');
+
+
+phutil_require_source('DifferentialDateModifiedFieldSpecification.php');

src/applications/differential/field/specification/lines/DifferentialLinesFieldSpecification.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -32,4 +32,20 @@ final class DifferentialLinesFieldSpecification
     return phutil_escape_html(number_format($diff->getLineCount()));
   }
 
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'Lines';
+  }
+
+  public function getColumnClassForRevisionList() {
+    return 'n';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    return number_format($revision->getLineCount());
+  }
+
 }

src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -140,4 +140,36 @@ final class DifferentialReviewersFieldSpecification
     return $this->parseCommitMessageUserList($value);
   }
 
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'Reviewers';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    $reviewer_phids = $revision->getReviewers();
+    if ($reviewer_phids) {
+      $first = reset($reviewer_phids);
+      if (count($reviewer_phids) > 1) {
+        $suffix = ' (+'.(count($reviewer_phids) - 1).')';
+      } else {
+        $suffix = null;
+      }
+      return $this->getHandle($first)->renderLink().$suffix;
+    } else {
+      return '<em>None</em>';
+    }
+  }
+
+  public function getRequiredHandlePHIDsForRevisionList(
+    DifferentialRevision $revision) {
+    $reviewer_phids = $revision->getReviewers();
+    if ($reviewer_phids) {
+      return array(reset($reviewer_phids));
+    }
+    return array();
+  }
+
 }

src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php

@@ -92,4 +92,16 @@ final class DifferentialRevisionIDFieldSpecification
     return null;
   }
 
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'ID';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    return 'D'.$revision->getID();
+  }
+
 }

src/applications/differential/field/specification/revisionstatus/DifferentialRevisionStatusFieldSpecification.php

@@ -54,4 +54,17 @@ final class DifferentialRevisionStatusFieldSpecification
     return '<strong>'.$status.'</strong>'.$next_step;
   }
 
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'Status';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    return ArcanistDifferentialRevisionStatus::getNameForRevisionStatus(
+      $revision->getStatus());
+  }
+
 }

src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -86,4 +86,25 @@ final class DifferentialTitleFieldSpecification
     return preg_replace('/\s*\n\s*/', ' ', $value);
   }
 
+  public function shouldAppearOnRevisionList() {
+    return true;
+  }
+
+  public function renderHeaderForRevisionList() {
+    return 'Revision';
+  }
+
+  public function getColumnClassForRevisionList() {
+    return 'wide pri';
+  }
+
+  public function renderValueForRevisionList(DifferentialRevision $revision) {
+    return phutil_render_tag(
+      'a',
+      array(
+        'href' => '/D'.$revision->getID(),
+      ),
+      phutil_escape_html($revision->getTitle()));
+  }
+
 }

src/applications/differential/field/specification/title/__init__.php

@@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'applications/differential/field/exception/
 phutil_require_module('phabricator', 'applications/differential/field/specification/base');
 phutil_require_module('phabricator', 'view/form/control/textarea');
 
+phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/differential/view/revisionlist/DifferentialRevisionListView.php

@@ -25,6 +25,12 @@ final class DifferentialRevisionListView extends AphrontView {
   private $handles;
   private $user;
   private $noDataString;
+  private $fields;
+
+  public function setFields(array $fields) {
+    $this->fields = $fields;
+    return $this;
+  }
 
   public function setRevisions(array $revisions) {
     $this->revisions = $revisions;
@@ -33,14 +39,12 @@ final class DifferentialRevisionListView extends AphrontView {
 
   public function getRequiredHandlePHIDs() {
     $phids = array();
-    foreach ($this->revisions as $revision) {
+    foreach ($this->fields as $field) {
-      $phids[] = $revision->getAuthorPHID();
+      foreach ($this->revisions as $revision) {
-      $reviewers = $revision->getReviewers();
+        $phids[] = $field->getRequiredHandlePHIDsForRevisionList($revision);
-      if ($reviewers) {
-        $phids[] = head($reviewers);
       }
     }
-    return $phids;
+    return array_mergev($phids);
   }
 
   public function setHandles(array $handles) {
@@ -65,67 +69,30 @@ final class DifferentialRevisionListView extends AphrontView {
       throw new Exception("Call setUser() before render()!");
     }
 
+    foreach ($this->fields as $field) {
+      $field->setUser($this->user);
+      $field->setHandles($this->handles);
+    }
+
     $rows = array();
     foreach ($this->revisions as $revision) {
-      $status = $revision->getStatus();
+      $row = array();
-      $status =
+      foreach ($this->fields as $field) {
-        ArcanistDifferentialRevisionStatus::getNameForRevisionStatus($status);
+        $row[] = $field->renderValueForRevisionList($revision);
-
-      $reviewer_phids = $revision->getReviewers();
-      if ($reviewer_phids) {
-        $first = reset($reviewer_phids);
-        if (count($reviewer_phids) > 1) {
-          $suffix = ' (+'.(count($reviewer_phids) - 1).')';
-        } else {
-          $suffix = null;
-        }
-        $reviewers = $this->handles[$first]->renderLink().$suffix;
-      } else {
-        $reviewers = '<em>None</em>';
       }
+      $rows[] = $row;
+    }
 
-      $link = phutil_render_tag(
+    $headers = array();
-        'a',
+    $classes = array();
-        array(
+    foreach ($this->fields as $field) {
-          'href' => '/D'.$revision->getID(),
+      $headers[] = $field->renderHeaderForRevisionList();
-        ),
+      $classes[] = $field->getColumnClassForRevisionList();
-        phutil_escape_html($revision->getTitle()));
-
-      $rows[] = array(
-        'D'.$revision->getID(),
-        $link,
-        phutil_escape_html($status),
-        number_format($revision->getLineCount()),
-        $this->handles[$revision->getAuthorPHID()]->renderLink(),
-        $reviewers,
-        phabricator_datetime($revision->getDateModified(), $user),
-        phabricator_date($revision->getDateCreated(), $user),
-      );
     }
 
     $table = new AphrontTableView($rows);
-    $table->setHeaders(
+    $table->setHeaders($headers);
-      array(
+    $table->setColumnClasses($classes);
-        'ID',
-        'Revision',
-        'Status',
-        'Lines',
-        'Author',
-        'Reviewers',
-        'Modified',
-        'Created',
-      ));
-    $table->setColumnClasses(
-      array(
-        null,
-        'wide pri',
-        null,
-        'n',
-        null,
-        null,
-        'right',
-        'right',
-      ));
 
     if ($this->noDataString) {
       $table->setNoDataString($this->noDataString);
@@ -134,4 +101,22 @@ final class DifferentialRevisionListView extends AphrontView {
     return $table->render();
   }
 
+  public static function getDefaultFields() {
+    $selector = DifferentialFieldSelector::newSelector();
+    $fields = $selector->getFieldSpecifications();
+    foreach ($fields as $key => $field) {
+      if (!$field->shouldAppearOnRevisionList()) {
+        unset($fields[$key]);
+      }
+    }
+
+    if (!$fields) {
+      throw new Exception(
+        "Phabricator configuration has no fields that appear on the list ".
+        "interface!");
+    }
+
+    return $selector->sortFieldsForRevisionList($fields);
+  }
+
 }

src/applications/differential/view/revisionlist/__init__.php

@@ -6,13 +6,10 @@
 
 
 
-phutil_require_module('arcanist', 'differential/constants/revisionstatus');
+phutil_require_module('phabricator', 'applications/differential/field/selector/base');
-
 phutil_require_module('phabricator', 'view/base');
 phutil_require_module('phabricator', 'view/control/table');
-phutil_require_module('phabricator', 'view/utils');
 
-phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/directory/controller/main/PhabricatorDirectoryMainController.php

@@ -292,8 +292,11 @@ class PhabricatorDirectoryMainController
         "View Active Revisions \xC2\xBB"));
 
     if ($active) {
+      $fields =
+
       $revision_view = id(new DifferentialRevisionListView())
         ->setRevisions($active)
+        ->setFields(DifferentialRevisionListView::getDefaultFields())
         ->setUser($user);
       $phids = array_merge(
         array($user_phid),