Separate "Revision" and "Diff" fields in Differential

Summary:
Fixes T5138. Some of the "revision" properties are really "diff" properties, but we only show the properties for the most recent / current diff.

  - Immediately, this makes it hard or impossible to review, e.g., lint/unit results for older diffs.
  - Longer-term, these limits will become more problematic with more data on diffs after Harbormaster.

Instead, separate "revision" from "diff" properties.

(In the long term, it might make sense to show more diffs in this panel -- e.g., tabs for the 8 most recent updates or something -- but I went with the simplest approach for now since I don't have a clean way to deal with 100-update revisions offhand.)

Test Plan: {F500480}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: cburroughs, epriestley

Maniphest Tasks: T5138

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

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php

@@ -34,9 +34,7 @@ final class PhabricatorDifferentialConfigOptions
       new DifferentialReviewedByField(),
       new DifferentialSubscribersField(),
       new DifferentialRepositoryField(),
-      new DifferentialLintField(),
       new DifferentialProjectsField(),
-      new DifferentialUnitField(),
       new DifferentialViewPolicyField(),
       new DifferentialEditPolicyField(),
 
@@ -54,6 +52,8 @@ final class PhabricatorDifferentialConfigOptions
       new DifferentialBlameRevisionField(),
       new DifferentialPathField(),
       new DifferentialHostField(),
+      new DifferentialLintField(),
+      new DifferentialUnitField(),
       new DifferentialRevertPlanField(),
 
       new DifferentialApplyPatchField(),

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -206,27 +206,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $visible_changesets = $changesets;
     }
 
-
-    // TODO: This should be in a DiffQuery or similar.
-    $need_props = array();
-    foreach ($field_list->getFields() as $field) {
-      foreach ($field->getRequiredDiffPropertiesForRevisionView() as $prop) {
-        $need_props[$prop] = $prop;
-      }
-    }
-
-    if ($need_props) {
-      $prop_diff = $revision->getActiveDiff();
-      $load_props = id(new DifferentialDiffProperty())->loadAllWhere(
-        'diffID = %d AND name IN (%Ls)',
-        $prop_diff->getID(),
-        $need_props);
-      $load_props = mpull($load_props, 'getData', 'getName');
-      foreach ($need_props as $need) {
-        $prop_diff->attachProperty($need, idx($load_props, $need));
-      }
-    }
-
     $commit_hashes = mpull($diffs, 'getSourceControlBaseRevision');
     $local_commits = idx($props, 'local:commits', array());
     foreach ($local_commits as $local_commit) {
@@ -286,6 +265,11 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $revision_detail_box->setInfoView($revision_warnings);
     }
 
+    $diff_detail_box = $this->buildDiffDetailView(
+      array_select_keys($diffs, array($diff_vs, $target->getID())),
+      $revision,
+      $field_list);
+
     $comment_view = $this->buildTransactions(
       $revision,
       $diff_vs ? $diffs[$diff_vs] : $target,
@@ -486,6 +470,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
     $content = array(
       $revision_detail_box,
+      $diff_detail_box,
       $page_pane,
     );
 
@@ -974,4 +959,85 @@ final class DifferentialRevisionViewController extends DifferentialController {
     return $warnings;
   }
 
+  private function buildDiffDetailView(
+    array $diffs,
+    DifferentialRevision $revision,
+    PhabricatorCustomFieldList $field_list) {
+    $viewer = $this->getViewer();
+
+    $fields = array();
+    foreach ($field_list->getFields() as $field) {
+      if ($field->shouldAppearInDiffPropertyView()) {
+        $fields[] = $field;
+      }
+    }
+
+    if (!$fields) {
+      return null;
+    }
+
+    // Make sure we're only going to render unique diffs.
+    $diffs = mpull($diffs, null, 'getID');
+    $labels = array(pht('Left'), pht('Right'));
+
+    $property_lists = array();
+    foreach ($diffs as $diff) {
+      if (count($diffs) == 2) {
+        $label = array_shift($labels);
+        $label = pht('%s (Diff %d)', $label, $diff->getID());
+      } else {
+        $label = pht('Diff %d', $diff->getID());
+      }
+
+      $property_lists[] = array(
+        $label,
+        $this->buildDiffPropertyList($diff, $revision, $fields),
+      );
+    }
+
+    $box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Diff Detail'))
+      ->setUser($viewer);
+
+    $last_tab = null;
+    foreach ($property_lists as $key => $property_list) {
+      list($tab_name, $list_view) = $property_list;
+
+      $tab = id(new PHUIListItemView())
+        ->setKey($key)
+        ->setName($tab_name);
+
+      $box->addPropertyList($list_view, $tab);
+      $last_tab = $tab;
+    }
+
+    if ($last_tab) {
+      $last_tab->setSelected(true);
+    }
+
+    return $box;
+  }
+
+  private function buildDiffPropertyList(
+    DifferentialDiff $diff,
+    DifferentialRevision $revision,
+    array $fields) {
+    $viewer = $this->getViewer();
+
+    $view = id(new PHUIPropertyListView())
+      ->setUser($viewer)
+      ->setObject($diff);
+
+    foreach ($fields as $field) {
+      $label = $field->renderDiffPropertyViewLabel($diff);
+      $value = $field->renderDiffPropertyViewValue($diff);
+      if ($value !== null) {
+        $view->addProperty($label, $value);
+      }
+    }
+
+    return $view;
+  }
+
+
 }

src/applications/differential/customfield/DifferentialBranchField.php

@@ -19,12 +19,20 @@ final class DifferentialBranchField
     return true;
   }
 
-  public function renderPropertyViewLabel() {
+  public function renderPropertyViewValue(array $handles) {
+    return null;
+  }
+
+  public function shouldAppearInDiffPropertyView() {
+    return true;
+  }
+
+  public function renderDiffPropertyViewLabel(DifferentialDiff $diff) {
     return $this->getFieldName();
   }
 
-  public function renderPropertyViewValue(array $handles) {
-    return $this->getBranchDescription($this->getObject()->getActiveDiff());
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    return $this->getBranchDescription($diff);
   }
 
   private function getBranchDescription(DifferentialDiff $diff) {

src/applications/differential/customfield/DifferentialCustomField.php

@@ -2,6 +2,7 @@
 
 /**
  * @task commitmessage    Integration with Commit Messages
+ * @task diff             Integration with Diff Properties
  */
 abstract class DifferentialCustomField
   extends PhabricatorCustomField {
@@ -31,13 +32,6 @@ abstract class DifferentialCustomField
     return parent::shouldEnableForRole($role);
   }
 
-  public function getRequiredDiffPropertiesForRevisionView() {
-    if ($this->getProxy()) {
-      return $this->getProxy()->getRequiredDiffPropertiesForRevisionView();
-    }
-    return array();
-  }
-
   protected function parseObjectList(
     $value,
     array $types,
@@ -82,6 +76,7 @@ abstract class DifferentialCustomField
     return array();
   }
 
+
 /* -(  Integration with Commit Messages  )----------------------------------- */
 
 
@@ -217,4 +212,40 @@ abstract class DifferentialCustomField
     return;
   }
 
+
+/* -(  Integration with Diff Properties  )----------------------------------- */
+
+
+  /**
+   * @task diff
+   */
+  public function shouldAppearInDiffPropertyView() {
+    if ($this->getProxy()) {
+      return $this->getProxy()->shouldAppearInDiffPropertyView();
+    }
+    return false;
+  }
+
+
+  /**
+   * @task diff
+   */
+  public function renderDiffPropertyViewLabel(DifferentialDiff $diff) {
+    if ($this->proxy) {
+      return $this->proxy->renderDiffPropertyViewLabel($diff);
+    }
+    return $this->getFieldName();
+  }
+
+
+  /**
+   * @task diff
+   */
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    if ($this->proxy) {
+      return $this->proxy->renderDiffPropertyViewValue($diff);
+    }
+    throw new PhabricatorCustomFieldImplementationIncompleteException($this);
+  }
+
 }

src/applications/differential/customfield/DifferentialHostField.php

@@ -23,12 +23,20 @@ final class DifferentialHostField
     return true;
   }
 
-  public function renderPropertyViewLabel() {
+  public function renderPropertyViewValue(array $handles) {
+    return null;
+  }
+
+  public function shouldAppearInDiffPropertyView() {
+    return true;
+  }
+
+  public function renderDiffPropertyViewLabel(DifferentialDiff $diff) {
     return $this->getFieldName();
   }
 
-  public function renderPropertyViewValue(array $handles) {
-    $host = $this->getObject()->getActiveDiff()->getSourceMachine();
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    $host = $diff->getSourceMachine();
     if (!$host) {
       return null;
     }

src/applications/differential/customfield/DifferentialLintField.php

@@ -19,20 +19,37 @@ final class DifferentialLintField
     return true;
   }
 
-  public function renderPropertyViewLabel() {
+  public function renderPropertyViewValue(array $handles) {
+    return null;
+  }
+
+  public function shouldAppearInDiffPropertyView() {
+    return true;
+  }
+
+  public function renderDiffPropertyViewLabel(DifferentialDiff $diff) {
     return $this->getFieldName();
   }
 
-  public function getRequiredDiffPropertiesForRevisionView() {
-    return array(
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    // TODO: This load is slightly inefficient, but most of this is moving
+    // to Harbormaster and this simplifies the transition. Eat 1-2 extra
+    // queries for now.
+    $keys = array(
       'arc:lint',
       'arc:lint-excuse',
       'arc:lint-postponed',
     );
-  }
 
-  public function renderPropertyViewValue(array $handles) {
-    $diff = $this->getObject()->getActiveDiff();
+    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
+      'diffID = %d AND name IN (%Ls)',
+      $diff->getID(),
+      $keys);
+    $properties = mpull($properties, 'getData', 'getName');
+
+    foreach ($keys as $key) {
+      $diff->attachProperty($key, idx($properties, $key));
+    }
 
     $path_changesets = mpull($diff->loadChangesets(), 'getID', 'getFilename');
 

src/applications/differential/customfield/DifferentialPathField.php

@@ -23,12 +23,20 @@ final class DifferentialPathField
     return true;
   }
 
-  public function renderPropertyViewLabel() {
+  public function renderPropertyViewValue(array $handles) {
+    return null;
+  }
+
+  public function shouldAppearInDiffPropertyView() {
+    return true;
+  }
+
+  public function renderDiffPropertyViewLabel(DifferentialDiff $diff) {
     return $this->getFieldName();
   }
 
-  public function renderPropertyViewValue(array $handles) {
-    $path = $this->getObject()->getActiveDiff()->getSourcePath();
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    $path = $diff->getSourcePath();
     if (!$path) {
       return null;
     }

src/applications/differential/customfield/DifferentialRepositoryField.php

@@ -125,20 +125,24 @@ final class DifferentialRepositoryField
     return true;
   }
 
-  public function renderPropertyViewLabel() {
+  public function renderPropertyViewValue(array $handles) {
+    return null;
+  }
+
+  public function shouldAppearInDiffPropertyView() {
+    return true;
+  }
+
+  public function renderDiffPropertyViewLabel(DifferentialDiff $diff) {
     return $this->getFieldName();
   }
 
-  public function getRequiredHandlePHIDsForPropertyView() {
-    $repository_phid = $this->getObject()->getRepositoryPHID();
-    if ($repository_phid) {
-      return array($repository_phid);
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    if (!$diff->getRepositoryPHID()) {
+      return null;
     }
-    return array();
-  }
 
-  public function renderPropertyViewValue(array $handles) {
-    return $this->renderHandleList($handles);
+    return $this->getViewer()->renderHandle($diff->getRepositoryPHID());
   }
 
   public function shouldAppearInTransactionMail() {

src/applications/differential/customfield/DifferentialUnitField.php

@@ -19,19 +19,34 @@ final class DifferentialUnitField
     return true;
   }
 
-  public function renderPropertyViewLabel() {
+  public function renderPropertyViewValue(array $handles) {
+    return null;
+  }
+
+  public function shouldAppearInDiffPropertyView() {
+    return true;
+  }
+
+  public function renderDiffPropertyViewLabel(DifferentialDiff $diff) {
     return $this->getFieldName();
   }
 
-  public function getRequiredDiffPropertiesForRevisionView() {
-    return array(
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    // TODO: See DifferentialLintField.
+    $keys = array(
       'arc:unit',
       'arc:unit-excuse',
     );
-  }
 
-  public function renderPropertyViewValue(array $handles) {
-    $diff = $this->getObject()->getActiveDiff();
+    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
+      'diffID = %d AND name IN (%Ls)',
+      $diff->getID(),
+      $keys);
+    $properties = mpull($properties, 'getData', 'getName');
+
+    foreach ($keys as $key) {
+      $diff->attachProperty($key, idx($properties, $key));
+    }
 
     $ustar = DifferentialRevisionUpdateHistoryView::renderDiffUnitStar($diff);
     $umsg = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage($diff);

src/applications/harbormaster/event/HarbormasterUIEventListener.php

@@ -31,6 +31,13 @@ final class HarbormasterUIEventListener
       return;
     }
 
+    if ($object instanceof DifferentialRevision) {
+      // TODO: This is a bit hacky and we could probably find a cleaner fix
+      // eventually, but we show build status on each diff, immediately below
+      // this property list, so it's redundant to show it on the revision view.
+      return;
+    }
+
     if (!($object instanceof HarbormasterBuildableInterface)) {
       return;
     }