From cc7ae60aaf1096d9e230e30cd642b0b5c87d58e6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Jul 2016 08:06:51 -0700 Subject: [PATCH] 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 | 6 +- .../DifferentialRevisionViewController.php | 171 +++--------------- .../edge/DifferentialStackGraph.php | 58 ------ .../graph/DifferentialRevisionGraph.php | 77 ++++++++ .../graph/PhabricatorObjectGraph.php | 157 ++++++++++++++++ 5 files changed, 259 insertions(+), 210 deletions(-) delete mode 100644 src/applications/differential/edge/DifferentialStackGraph.php create mode 100644 src/infrastructure/graph/DifferentialRevisionGraph.php create mode 100644 src/infrastructure/graph/PhabricatorObjectGraph.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 47e68b31c2..80f5cc54eb 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 83c6d10288..adf6a42bfd 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/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()) - ->setSeedRevision($revision) + $stack_graph = id(new DifferentialRevisionGraph()) + ->setViewer($viewer) + ->setSeedPHID($revision->getPHID()) ->loadGraph(); if (!$stack_graph->isEmpty()) { - $stack_view = $this->renderStackView($revision, $stack_graph); - list($stack_name, $stack_color, $stack_table) = $stack_view; + $stack_table = $stack_graph->newGraphTable(); + + $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); - } - } diff --git a/src/applications/differential/edge/DifferentialStackGraph.php b/src/applications/differential/edge/DifferentialStackGraph.php deleted file mode 100644 index cc7d735d79..0000000000 --- a/src/applications/differential/edge/DifferentialStackGraph.php +++ /dev/null @@ -1,58 +0,0 @@ -addNodes( - array( - '' => 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; - } - -} diff --git a/src/infrastructure/graph/DifferentialRevisionGraph.php b/src/infrastructure/graph/DifferentialRevisionGraph.php new file mode 100644 index 0000000000..718cd9a824 --- /dev/null +++ b/src/infrastructure/graph/DifferentialRevisionGraph.php @@ -0,0 +1,77 @@ +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', + )); + } + +} diff --git a/src/infrastructure/graph/PhabricatorObjectGraph.php b/src/infrastructure/graph/PhabricatorObjectGraph.php new file mode 100644 index 0000000000..943ddaad29 --- /dev/null +++ b/src/infrastructure/graph/PhabricatorObjectGraph.php @@ -0,0 +1,157 @@ +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( + '' => 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; + } + +}