Make the revision graph view more flexible

Summary:
Ref T4788. This separates the revision graph view into a base class with core logic and a revision class with Differential-specific logic, so I can subclass it in Maniphest, etc., and try using it in other applications to show similar graphs.

Not sure if we'll stick with it, but even if we don't this makes the code a bit cleaner and gets custom rendering logic out of the RevisionViewController, which is nice.

Test Plan: Viewed revisions, saw the stack UI completely unchanged.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788

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

src/__phutil_library_map__.php

@@ -521,6 +521,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php',
     'DifferentialRevisionEditController' => 'applications/differential/controller/DifferentialRevisionEditController.php',
     'DifferentialRevisionFulltextEngine' => 'applications/differential/search/DifferentialRevisionFulltextEngine.php',
+    'DifferentialRevisionGraph' => 'infrastructure/graph/DifferentialRevisionGraph.php',
     'DifferentialRevisionHasChildRelationship' => 'applications/differential/relationships/DifferentialRevisionHasChildRelationship.php',
     'DifferentialRevisionHasCommitEdgeType' => 'applications/differential/edge/DifferentialRevisionHasCommitEdgeType.php',
     'DifferentialRevisionHasCommitRelationship' => 'applications/differential/relationships/DifferentialRevisionHasCommitRelationship.php',
@@ -556,7 +557,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php',
     'DifferentialSchemaSpec' => 'applications/differential/storage/DifferentialSchemaSpec.php',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php',
-    'DifferentialStackGraph' => 'applications/differential/edge/DifferentialStackGraph.php',
     'DifferentialStoredCustomField' => 'applications/differential/customfield/DifferentialStoredCustomField.php',
     'DifferentialSubscribersField' => 'applications/differential/customfield/DifferentialSubscribersField.php',
     'DifferentialSummaryField' => 'applications/differential/customfield/DifferentialSummaryField.php',
@@ -2868,6 +2868,7 @@ phutil_register_library_map(array(
     'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php',
     'PhabricatorOAuthServerTransaction' => 'applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php',
     'PhabricatorOAuthServerTransactionQuery' => 'applications/oauthserver/query/PhabricatorOAuthServerTransactionQuery.php',
+    'PhabricatorObjectGraph' => 'infrastructure/graph/PhabricatorObjectGraph.php',
     'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php',
     'PhabricatorObjectHasAsanaSubtaskEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasAsanaSubtaskEdgeType.php',
     'PhabricatorObjectHasAsanaTaskEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasAsanaTaskEdgeType.php',
@@ -4895,6 +4896,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'PhabricatorEdgeType',
     'DifferentialRevisionEditController' => 'DifferentialController',
     'DifferentialRevisionFulltextEngine' => 'PhabricatorFulltextEngine',
+    'DifferentialRevisionGraph' => 'PhabricatorObjectGraph',
     'DifferentialRevisionHasChildRelationship' => 'DifferentialRevisionRelationship',
     'DifferentialRevisionHasCommitEdgeType' => 'PhabricatorEdgeType',
     'DifferentialRevisionHasCommitRelationship' => 'DifferentialRevisionRelationship',
@@ -4930,7 +4932,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionViewController' => 'DifferentialController',
     'DifferentialSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'DifferentialConduitAPIMethod',
-    'DifferentialStackGraph' => 'AbstractDirectedGraph',
     'DifferentialStoredCustomField' => 'DifferentialCustomField',
     'DifferentialSubscribersField' => 'DifferentialCoreCustomField',
     'DifferentialSummaryField' => 'DifferentialCoreCustomField',
@@ -7582,6 +7583,7 @@ phutil_register_library_map(array(
     'PhabricatorOAuthServerTokenController' => 'PhabricatorOAuthServerController',
     'PhabricatorOAuthServerTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorOAuthServerTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'PhabricatorObjectGraph' => 'AbstractDirectedGraph',
     'PhabricatorObjectHandle' => array(
       'Phobject',
       'PhabricatorPolicyInterface',

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -341,12 +341,29 @@ final class DifferentialRevisionViewController extends DifferentialController {
           ->setKey('commits')
           ->appendChild($local_table));
 
-    $stack_graph = id(new DifferentialStackGraph())
+    $stack_graph = id(new DifferentialRevisionGraph())
-      ->setSeedRevision($revision)
+      ->setViewer($viewer)
+      ->setSeedPHID($revision->getPHID())
       ->loadGraph();
     if (!$stack_graph->isEmpty()) {
-      $stack_view = $this->renderStackView($revision, $stack_graph);
+      $stack_table = $stack_graph->newGraphTable();
-      list($stack_name, $stack_color, $stack_table) = $stack_view;
+
+      $parent_type = DifferentialRevisionDependsOnRevisionEdgeType::EDGECONST;
+      $reachable = $stack_graph->getReachableObjects($parent_type);
+
+      foreach ($reachable as $key => $reachable_revision) {
+        if ($reachable_revision->isClosed()) {
+          unset($reachable[$key]);
+        }
+      }
+
+      if ($reachable) {
+        $stack_name = pht('Stack (%s Open)', phutil_count($reachable));
+        $stack_color = PHUIListItemView::STATUS_FAIL;
+      } else {
+        $stack_name = pht('Stack');
+        $stack_color = null;
+      }
 
       $tab_group->addTab(
         id(new PHUITabView())
@@ -1212,150 +1229,4 @@ final class DifferentialRevisionViewController extends DifferentialController {
       ->setShowViewAll(true);
   }
 
-
-  private function renderStackView(
-    DifferentialRevision $current,
-    DifferentialStackGraph $graph) {
-
-    $ancestry = $graph->getParentEdges();
-    $viewer = $this->getViewer();
-
-    $revisions = id(new DifferentialRevisionQuery())
-      ->setViewer($viewer)
-      ->withPHIDs(array_keys($ancestry))
-      ->execute();
-    $revisions = mpull($revisions, null, 'getPHID');
-
-    $order = id(new PhutilDirectedScalarGraph())
-      ->addNodes($ancestry)
-      ->getTopographicallySortedNodes();
-
-    $ancestry = array_select_keys($ancestry, $order);
-
-    $traces = id(new PHUIDiffGraphView())
-      ->renderGraph($ancestry);
-
-    // Load author handles, and also revision handles for any revisions which
-    // we failed to load (they might be policy restricted).
-    $handle_phids = mpull($revisions, 'getAuthorPHID');
-    foreach ($order as $phid) {
-      if (empty($revisions[$phid])) {
-        $handle_phids[] = $phid;
-      }
-    }
-    $handles = $viewer->loadHandles($handle_phids);
-
-    $rows = array();
-    $rowc = array();
-
-    $ii = 0;
-    $seen = false;
-    foreach ($ancestry as $phid => $ignored) {
-      $revision = idx($revisions, $phid);
-      if ($revision) {
-        $status_icon = $revision->getStatusIcon();
-        $status_name = $revision->getStatusDisplayName();
-
-        $status = array(
-          id(new PHUIIconView())->setIcon($status_icon),
-          ' ',
-          $status_name,
-        );
-
-        $author = $viewer->renderHandle($revision->getAuthorPHID());
-        $title = phutil_tag(
-          'a',
-          array(
-            'href' => $revision->getURI(),
-          ),
-          array(
-            $revision->getMonogram(),
-            ' ',
-            $revision->getTitle(),
-          ));
-      } else {
-        $status = null;
-        $author = null;
-        $title = $viewer->renderHandle($phid);
-      }
-
-      $rows[] = array(
-        $traces[$ii++],
-        $status,
-        $author,
-        $title,
-      );
-
-      if ($phid == $current->getPHID()) {
-        $rowc[] = 'highlighted';
-      } else {
-        $rowc[] = null;
-      }
-    }
-
-    $stack_table = id(new AphrontTableView($rows))
-      ->setHeaders(
-        array(
-          null,
-          pht('Status'),
-          pht('Author'),
-          pht('Revision'),
-        ))
-      ->setRowClasses($rowc)
-      ->setColumnClasses(
-        array(
-          'threads',
-          null,
-          null,
-          'wide',
-        ));
-
-    // Count how many revisions this one depends on that are not yet closed.
-    $seen = array();
-    $look = array($current->getPHID());
-    while ($look) {
-      $phid = array_pop($look);
-
-      $parents = idx($ancestry, $phid, array());
-      foreach ($parents as $parent) {
-        if (isset($seen[$parent])) {
-          continue;
-        }
-
-        $seen[$parent] = $parent;
-        $look[] = $parent;
-      }
-    }
-
-    $blocking_count = 0;
-    foreach ($seen as $parent) {
-      if ($parent == $current->getPHID()) {
-        continue;
-      }
-
-      $revision = idx($revisions, $parent);
-      if (!$revision) {
-        continue;
-      }
-
-      if ($revision->isClosed()) {
-        continue;
-      }
-
-      $blocking_count++;
-    }
-
-    if (!$blocking_count) {
-      $stack_name = pht('Stack');
-      $stack_color = null;
-    } else {
-      $stack_name = pht(
-        'Stack (%s Open)',
-        new PhutilNumber($blocking_count));
-      $stack_color = PHUIListItemView::STATUS_FAIL;
-    }
-
-    return array($stack_name, $stack_color, $stack_table);
-  }
-
 }

src/applications/differential/edge/DifferentialStackGraph.php

@@ -1,58 +0,0 @@
-<?php
-
-final class DifferentialStackGraph
-  extends AbstractDirectedGraph {
-
-  private $parentEdges = array();
-  private $childEdges = array();
-
-  public function setSeedRevision(DifferentialRevision $revision) {
-    return $this->addNodes(
-      array(
-        '<seed>' => array($revision->getPHID()),
-      ));
-  }
-
-  public function isEmpty() {
-    return (count($this->getNodes()) <= 2);
-  }
-
-  public function getParentEdges() {
-    return $this->parentEdges;
-  }
-
-  protected function loadEdges(array $nodes) {
-    $query = id(new PhabricatorEdgeQuery())
-      ->withSourcePHIDs($nodes)
-      ->withEdgeTypes(
-        array(
-          DifferentialRevisionDependsOnRevisionEdgeType::EDGECONST,
-          DifferentialRevisionDependedOnByRevisionEdgeType::EDGECONST,
-        ));
-
-    $query->execute();
-
-    $map = array();
-    foreach ($nodes as $node) {
-      $parents = $query->getDestinationPHIDs(
-        array($node),
-        array(
-          DifferentialRevisionDependsOnRevisionEdgeType::EDGECONST,
-        ));
-
-      $children = $query->getDestinationPHIDs(
-        array($node),
-        array(
-          DifferentialRevisionDependedOnByRevisionEdgeType::EDGECONST,
-        ));
-
-      $this->parentEdges[$node] = $parents;
-      $this->childEdges[$node] = $children;
-
-      $map[$node] = array_values(array_fuse($parents) + array_fuse($children));
-    }
-
-    return $map;
-  }
-
-}

src/infrastructure/graph/DifferentialRevisionGraph.php

@@ -0,0 +1,77 @@
+<?php
+
+final class DifferentialRevisionGraph
+  extends PhabricatorObjectGraph {
+
+  protected function getEdgeTypes() {
+    return array(
+      DifferentialRevisionDependsOnRevisionEdgeType::EDGECONST,
+      DifferentialRevisionDependedOnByRevisionEdgeType::EDGECONST,
+    );
+  }
+
+  protected function getParentEdgeType() {
+    return DifferentialRevisionDependsOnRevisionEdgeType::EDGECONST;
+  }
+
+  protected function newQuery() {
+    return new DifferentialRevisionQuery();
+  }
+
+  protected function newTableRow($phid, $object, $trace) {
+    $viewer = $this->getViewer();
+
+    if ($object) {
+      $status_icon = $object->getStatusIcon();
+      $status_name = $object->getStatusDisplayName();
+
+      $status = array(
+        id(new PHUIIconView())->setIcon($status_icon),
+        ' ',
+        $status_name,
+      );
+
+      $author = $viewer->renderHandle($object->getAuthorPHID());
+      $link = phutil_tag(
+        'a',
+        array(
+          'href' => $object->getURI(),
+        ),
+        array(
+          $object->getMonogram(),
+          ' ',
+          $object->getTitle(),
+        ));
+    } else {
+      $status = null;
+      $author = null;
+      $link = $viewer->renderHandle($phid);
+    }
+
+    return array(
+      $trace,
+      $status,
+      $author,
+      $link,
+    );
+  }
+
+  protected function newTable(AphrontTableView $table) {
+    return $table
+      ->setHeaders(
+        array(
+          null,
+          pht('Status'),
+          pht('Author'),
+          pht('Revision'),
+        ))
+      ->setColumnClasses(
+        array(
+          'threads',
+          null,
+          null,
+          'wide',
+        ));
+  }
+
+}

src/infrastructure/graph/PhabricatorObjectGraph.php

@@ -0,0 +1,157 @@
+<?php
+
+abstract class PhabricatorObjectGraph
+  extends AbstractDirectedGraph {
+
+  private $viewer;
+  private $edges = array();
+  private $seedPHID;
+  private $objects;
+
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    if (!$this->viewer) {
+      throw new PhutilInvalidStateException('setViewer');
+    }
+
+    return $this->viewer;
+  }
+
+  abstract protected function getEdgeTypes();
+  abstract protected function getParentEdgeType();
+  abstract protected function newQuery();
+  abstract protected function newTableRow($phid, $object, $trace);
+  abstract protected function newTable(AphrontTableView $table);
+
+  final public function setSeedPHID($phid) {
+    $this->seedPHID = $phid;
+
+    return $this->addNodes(
+      array(
+        '<seed>' => array($phid),
+      ));
+  }
+
+  final public function isEmpty() {
+    return (count($this->getNodes()) <= 2);
+  }
+
+  final public function getEdges($type) {
+    return idx($this->edges, $type, array());
+  }
+
+  final protected function loadEdges(array $nodes) {
+    $edge_types = $this->getEdgeTypes();
+
+    $query = id(new PhabricatorEdgeQuery())
+      ->withSourcePHIDs($nodes)
+      ->withEdgeTypes($edge_types);
+
+    $query->execute();
+
+    $map = array();
+    foreach ($nodes as $node) {
+      foreach ($edge_types as $edge_type) {
+        $dst_phids = $query->getDestinationPHIDs(
+          array($node),
+          array($edge_type));
+
+        $this->edges[$edge_type][$node] = $dst_phids;
+        foreach ($dst_phids as $dst_phid) {
+          $map[$node][] = $dst_phid;
+        }
+      }
+
+      $map[$node] = array_values(array_fuse($map[$node]));
+    }
+
+    return $map;
+  }
+
+  final public function newGraphTable() {
+    $viewer = $this->getViewer();
+
+    $ancestry = $this->getEdges($this->getParentEdgeType());
+
+    $objects = $this->newQuery()
+      ->setViewer($viewer)
+      ->withPHIDs(array_keys($ancestry))
+      ->execute();
+    $objects = mpull($objects, null, 'getPHID');
+
+    $order = id(new PhutilDirectedScalarGraph())
+      ->addNodes($ancestry)
+      ->getTopographicallySortedNodes();
+
+    $ancestry = array_select_keys($ancestry, $order);
+
+    $traces = id(new PHUIDiffGraphView())
+      ->renderGraph($ancestry);
+
+    $ii = 0;
+    $rows = array();
+    $rowc = array();
+    foreach ($ancestry as $phid => $ignored) {
+      $object = idx($objects, $phid);
+      $rows[] = $this->newTableRow($phid, $object, $traces[$ii++]);
+
+      if ($phid == $this->seedPHID) {
+        $rowc[] = 'highlighted';
+      } else {
+        $rowc[] = null;
+      }
+    }
+
+    $table = id(new AphrontTableView($rows))
+      ->setRowClasses($rowc);
+
+    $this->objects = $objects;
+
+    return $this->newTable($table);
+  }
+
+  final public function getReachableObjects($edge_type) {
+    if ($this->objects === null) {
+      throw new PhutilInvalidStateException('newGraphTable');
+    }
+
+    $graph = $this->getEdges($edge_type);
+
+    $seen = array();
+    $look = array($this->seedPHID);
+    while ($look) {
+      $phid = array_pop($look);
+
+      $parents = idx($graph, $phid, array());
+      foreach ($parents as $parent) {
+        if (isset($seen[$parent])) {
+          continue;
+        }
+
+        $seen[$parent] = $parent;
+        $look[] = $parent;
+      }
+    }
+
+    $reachable = array();
+    foreach ($seen as $phid) {
+      if ($phid == $this->seedPHID) {
+        continue;
+      }
+
+      $object = idx($this->objects, $phid);
+      if (!$object) {
+        continue;
+      }
+
+      $reachable[] = $object;
+    }
+
+    return $reachable;
+  }
+
+}