From 8c8e7f07b575e89b2c0907f75f60e3014581bbe9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Apr 2018 16:32:48 -0700 Subject: [PATCH] Add a standalone view for browsing changesets of very large revisions Summary: Ref T13110. Installs have various reasons for sending unreviewable changes (changes where the text of the change will never be reviewed by a human) through Differential anyway. Prepare for accommodating this more gracefully by building a standalone changeset list page which paginates the changesets. Test Plan: Clicked the new "Changeset List" button on a revision, was taken to a separate page. Maniphest Tasks: T13110 Differential Revision: https://secure.phabricator.com/D19295 --- src/__phutil_library_map__.php | 4 + .../PhabricatorDifferentialApplication.php | 8 +- .../DifferentialChangesetListController.php | 51 +++++++ .../DifferentialRevisionViewController.php | 12 +- .../query/DifferentialChangesetQuery.php | 29 ++-- .../DifferentialChangesetSearchEngine.php | 133 ++++++++++++++++++ 6 files changed, 216 insertions(+), 21 deletions(-) create mode 100644 src/applications/differential/controller/DifferentialChangesetListController.php create mode 100644 src/applications/differential/query/DifferentialChangesetSearchEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 80279c40fc..391b8fb1f2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -389,6 +389,7 @@ phutil_register_library_map(array( 'DifferentialChangesetDetailView' => 'applications/differential/view/DifferentialChangesetDetailView.php', 'DifferentialChangesetFileTreeSideNavBuilder' => 'applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php', 'DifferentialChangesetHTMLRenderer' => 'applications/differential/render/DifferentialChangesetHTMLRenderer.php', + 'DifferentialChangesetListController' => 'applications/differential/controller/DifferentialChangesetListController.php', 'DifferentialChangesetListView' => 'applications/differential/view/DifferentialChangesetListView.php', 'DifferentialChangesetOneUpMailRenderer' => 'applications/differential/render/DifferentialChangesetOneUpMailRenderer.php', 'DifferentialChangesetOneUpRenderer' => 'applications/differential/render/DifferentialChangesetOneUpRenderer.php', @@ -397,6 +398,7 @@ phutil_register_library_map(array( 'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php', 'DifferentialChangesetQuery' => 'applications/differential/query/DifferentialChangesetQuery.php', 'DifferentialChangesetRenderer' => 'applications/differential/render/DifferentialChangesetRenderer.php', + 'DifferentialChangesetSearchEngine' => 'applications/differential/query/DifferentialChangesetSearchEngine.php', 'DifferentialChangesetTestRenderer' => 'applications/differential/render/DifferentialChangesetTestRenderer.php', 'DifferentialChangesetTwoUpRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpRenderer.php', 'DifferentialChangesetTwoUpTestRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php', @@ -5605,6 +5607,7 @@ phutil_register_library_map(array( 'DifferentialChangesetDetailView' => 'AphrontView', 'DifferentialChangesetFileTreeSideNavBuilder' => 'Phobject', 'DifferentialChangesetHTMLRenderer' => 'DifferentialChangesetRenderer', + 'DifferentialChangesetListController' => 'DifferentialController', 'DifferentialChangesetListView' => 'AphrontView', 'DifferentialChangesetOneUpMailRenderer' => 'DifferentialChangesetRenderer', 'DifferentialChangesetOneUpRenderer' => 'DifferentialChangesetHTMLRenderer', @@ -5613,6 +5616,7 @@ phutil_register_library_map(array( 'DifferentialChangesetParserTestCase' => 'PhabricatorTestCase', 'DifferentialChangesetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialChangesetRenderer' => 'Phobject', + 'DifferentialChangesetSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DifferentialChangesetTestRenderer' => 'DifferentialChangesetRenderer', 'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetHTMLRenderer', 'DifferentialChangesetTwoUpTestRenderer' => 'DifferentialChangesetTestRenderer', diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index 4141cf8539..da40739579 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -52,7 +52,13 @@ final class PhabricatorDifferentialApplication extends PhabricatorApplication { '(?:query/(?P[^/]+)/)?' => 'DifferentialRevisionListController', 'diff/' => array( - '(?P[1-9]\d*)/' => 'DifferentialDiffViewController', + '(?P[1-9]\d*)/' => array( + '' => 'DifferentialDiffViewController', + 'changesets/' => array( + $this->getQueryRoutePattern() + => 'DifferentialChangesetListController', + ), + ), 'create/' => 'DifferentialDiffCreateController', ), 'changeset/' => 'DifferentialChangesetViewController', diff --git a/src/applications/differential/controller/DifferentialChangesetListController.php b/src/applications/differential/controller/DifferentialChangesetListController.php new file mode 100644 index 0000000000..aa807da9e4 --- /dev/null +++ b/src/applications/differential/controller/DifferentialChangesetListController.php @@ -0,0 +1,51 @@ +getViewer(); + + $diff = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withIDs(array($request->getURIData('id'))) + ->executeOne(); + if (!$diff) { + return new Aphront404Response(); + } + $this->diff = $diff; + + return id(new DifferentialChangesetSearchEngine()) + ->setController($this) + ->setDiff($diff) + ->buildResponse(); + } + + protected function buildApplicationCrumbs() { + $crumbs = parent::buildApplicationCrumbs(); + + $diff = $this->diff; + if ($diff) { + $revision = $diff->getRevision(); + if ($revision) { + $crumbs->addTextCrumb( + $revision->getMonogram(), + $revision->getURI()); + } + + $crumbs->addTextCrumb( + pht('Diff %d', $diff->getID()), + $diff->getURI()); + } + + return $crumbs; + } + + +} diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 4ed773d784..b4f3799f7c 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -399,8 +399,18 @@ final class DifferentialRevisionViewController extends DifferentialController { ->appendChild($other_view)); } + $view_button = id(new PHUIButtonView()) + ->setTag('a') + ->setText(pht('Changeset List')) + ->setHref('/differential/diff/'.$target->getID().'/changesets/') + ->setIcon('fa-align-left'); + + $tab_header = id(new PHUIHeaderView()) + ->setHeader(pht('Revision Contents')) + ->addActionLink($view_button); + $tab_view = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Revision Contents')) + ->setHeader($tab_header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->addTabGroup($tab_group); diff --git a/src/applications/differential/query/DifferentialChangesetQuery.php b/src/applications/differential/query/DifferentialChangesetQuery.php index 7b9f2e6d10..8f69c61a4e 100644 --- a/src/applications/differential/query/DifferentialChangesetQuery.php +++ b/src/applications/differential/query/DifferentialChangesetQuery.php @@ -41,19 +41,12 @@ final class DifferentialChangesetQuery } } + public function newResultObject() { + return new DifferentialChangeset(); + } + protected function loadPage() { - $table = new DifferentialChangeset(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $changesets) { @@ -124,26 +117,24 @@ final class DifferentialChangesetQuery return $changesets; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); if ($this->diffs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'diffID IN (%Ld)', mpull($this->diffs, 'getID')); } if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'id IN (%Ld)', $this->ids); } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { diff --git a/src/applications/differential/query/DifferentialChangesetSearchEngine.php b/src/applications/differential/query/DifferentialChangesetSearchEngine.php new file mode 100644 index 0000000000..0dfec94a53 --- /dev/null +++ b/src/applications/differential/query/DifferentialChangesetSearchEngine.php @@ -0,0 +1,133 @@ +diff = $diff; + return $this; + } + + public function getDiff() { + return $this->diff; + } + + public function getResultTypeDescription() { + return pht('Differential Changesets'); + } + + public function getApplicationClassName() { + return 'PhabricatorDifferentialApplication'; + } + + public function newQuery() { + $query = id(new DifferentialChangesetQuery()); + + if ($this->diff) { + $query->withDiffs(array($this->diff)); + } + + return $query; + } + + protected function buildQueryFromParameters(array $map) { + $query = $this->newQuery(); + return $query; + } + + protected function buildCustomSearchFields() { + return array(); + } + + protected function getURI($path) { + $diff = $this->getDiff(); + if ($diff) { + return '/differential/diff/'.$diff->getID().'/changesets/'.$path; + } + + throw new PhutilMethodNotImplementedException(); + } + + protected function getBuiltinQueryNames() { + $names = array(); + $names['all'] = pht('All Changesets'); + return $names; + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + + $viewer = $this->requireViewer(); + + switch ($query_key) { + case 'all': + return $query->setParameter('order', 'oldest'); + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + + protected function renderResultList( + array $changesets, + PhabricatorSavedQuery $query, + array $handles) { + + assert_instances_of($changesets, 'DifferentialChangeset'); + $viewer = $this->requireViewer(); + + $rows = array(); + foreach ($changesets as $changeset) { + $link = phutil_tag( + 'a', + array( + 'href' => '/differential/changeset/?ref='.$changeset->getID(), + ), + $changeset->getDisplayFilename()); + + $type = $changeset->getChangeType(); + + $title = DifferentialChangeType::getFullNameForChangeType($type); + + $add_lines = $changeset->getAddLines(); + if (!$add_lines) { + $add_lines = null; + } else { + $add_lines = '+'.$add_lines; + } + + $rem_lines = $changeset->getDelLines(); + if (!$rem_lines) { + $rem_lines = null; + } else { + $rem_lines = '-'.$rem_lines; + } + + $rows[] = array( + $changeset->newFileTreeIcon(), + $title, + $link, + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + null, + pht('Change'), + pht('Path'), + )) + ->setColumnClasses( + array( + null, + null, + 'pri wide', + )); + + return id(new PhabricatorApplicationSearchResultView()) + ->setTable($table); + } + +}