From fc1adf9875732713139577a8a955b251573ec5c8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 5 Dec 2016 17:40:24 -0800 Subject: [PATCH] Modernize UI for "Compare" in Diffusion Summary: Ref T929. We've made some UI updates since D15330. Test Plan: {F2079125} Reviewers: avivey, chad Reviewed By: chad Maniphest Tasks: T929 Differential Revision: https://secure.phabricator.com/D16990 --- .../controller/DiffusionBrowseController.php | 16 + .../controller/DiffusionCompareController.php | 417 ++++++++++-------- .../diffusion/request/DiffusionRequest.php | 2 +- .../storage/PhabricatorRepository.php | 31 +- 4 files changed, 287 insertions(+), 179 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 153906b25c..d762c60c55 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1651,6 +1651,7 @@ final class DiffusionBrowseController extends DiffusionController { protected function buildCurtain(DiffusionRequest $drequest) { $viewer = $this->getViewer(); + $repository = $drequest->getRepository(); $curtain = $this->newCurtainView($drequest); @@ -1666,6 +1667,21 @@ final class DiffusionBrowseController extends DiffusionController { ->setIcon('fa-list')); $behind_head = $drequest->getSymbolicCommit(); + + if ($repository->supportsBranchComparison()) { + $compare_uri = $drequest->generateURI( + array( + 'action' => 'compare', + )); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Compare Against...')) + ->setIcon('fa-code-fork') + ->setWorkflow(true) + ->setHref($compare_uri)); + } + $head_uri = $drequest->generateURI( array( 'commit' => '', diff --git a/src/applications/diffusion/controller/DiffusionCompareController.php b/src/applications/diffusion/controller/DiffusionCompareController.php index 3ef16fd497..ab47d21910 100644 --- a/src/applications/diffusion/controller/DiffusionCompareController.php +++ b/src/applications/diffusion/controller/DiffusionCompareController.php @@ -16,104 +16,87 @@ final class DiffusionCompareController extends DiffusionController { $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); - $content = array(); - - $crumbs = $this->buildCrumbs(array('view' => 'compare')); - - $empty_title = null; - $error_message = null; - - if ($repository->getVersionControlSystem() != - PhabricatorRepositoryType::REPOSITORY_TYPE_GIT) { - $empty_title = pht('Not supported'); - $error_message = pht( - 'This feature is not yet supported for %s repositories.', - $repository->getVersionControlSystem()); - $content[] = id(new PHUIInfoView()) - ->setTitle($empty_title) - ->setSeverity(PHUIInfoView::SEVERITY_WARNING) - ->setErrors(array($error_message)); + if (!$repository->supportsBranchComparison()) { + return $this->newDialog() + ->setTitle(pht('Not Supported')) + ->appendParagraph( + pht( + 'Branch comparison is not supported for this version control '. + 'system.')) + ->addCancelButton($this->getApplicationURI(), pht('Okay')); } $head_ref = $request->getStr('head'); $against_ref = $request->getStr('against'); - $refs = id(new DiffusionCachedResolveRefsQuery()) - ->setRepository($repository) - ->withRefs(array($head_ref, $against_ref)) - ->execute(); + $must_prompt = false; + if (!$request->isFormPost()) { + if (!strlen($head_ref)) { + $head_ref = $drequest->getSymbolicCommit(); + if (!strlen($head_ref)) { + $head_ref = $drequest->getBranch(); + } + } + if (!strlen($against_ref)) { + $default_branch = $repository->getDefaultBranch(); + if ($default_branch != $head_ref) { + $against_ref = $default_branch; - if (count($refs) == 2) { - $content[] = $this->buildCompareContent($drequest); - } else { - $content[] = $this->buildCompareForm($request, $refs); + // If we filled this in by default, we want to prompt the user to + // confirm that this is really what they want. + $must_prompt = true; + } + } } - - return $this->newPage() - ->setTitle( + $refs = $drequest->resolveRefs( + array_filter( array( - $repository->getName(), - $repository->getDisplayName(), - )) - ->setCrumbs($crumbs) - ->appendChild($content); - } + $head_ref, + $against_ref, + ))); - private function buildCompareForm(AphrontRequest $request, array $resolved) { - $viewer = $this->getViewer(); - - $head_ref = $request->getStr('head'); - $against_ref = $request->getStr('against'); - - if (idx($resolved, $head_ref)) { - $e_head = null; + $identical = false; + if ($head_ref === $against_ref) { + $identical = true; } else { - $e_head = 'Not Found'; + if (count($refs) == 2) { + if ($refs[$head_ref] === $refs[$against_ref]) { + $identical = true; + } + } } - if (idx($resolved, $against_ref)) { - $e_against = null; - } else { - $e_against = 'Not Found'; + if ($must_prompt || count($refs) != 2 || $identical) { + return $this->buildCompareDialog( + $head_ref, + $against_ref, + $refs, + $identical); } + if ($request->isFormPost()) { + // Redirect to a stable URI that can be copy/pasted. + $compare_uri = $drequest->generateURI( + array( + 'action' => 'compare', + 'head' => $head_ref, + 'against' => $against_ref, + )); - $form = id(new AphrontFormView()) - ->setUser($viewer) - ->setMethod('GET') - ->appendControl( - id(new AphrontFormTextControl()) - ->setLabel(pht('Head')) - ->setName('head') - ->setError($e_head) - ->setValue($head_ref)) - ->appendControl( - id(new AphrontFormTextControl()) - ->setLabel(pht('Against')) - ->setName('against') - ->setError($e_against) - ->setValue($against_ref)) - ->appendControl( - id(new AphrontFormSubmitControl()) - ->setValue('Compare')); + return id(new AphrontRedirectResponse())->setURI($compare_uri); + } - return $form; - } - - private function buildCompareContent(DiffusionRequest $drequest) { - $request = $this->getRequest(); - $repository = $drequest->getRepository(); - - $head_ref = $request->getStr('head'); - $against_ref = $request->getStr('against'); - - $content = array(); + $crumbs = $this->buildCrumbs( + array( + 'view' => 'compare', + )); $pager = id(new PHUIPagerView()) ->readFromRequest($request); + $history = null; try { $history_results = $this->callConduitWithDiffusionRequest( 'diffusion.historyquery', @@ -126,111 +109,189 @@ final class DiffusionCompareController extends DiffusionController { )); $history = DiffusionPathChange::newFromConduit( $history_results['pathChanges']); - $history = $pager->sliceResults($history); - $history_exception = null; + $history_view = $this->newHistoryView( + $history_results, + $history, + $pager, + $head_ref, + $against_ref); + } catch (Exception $ex) { - $history_results = null; - $history = null; - $history_exception = $ex; - } - - if ($history_results) { - $content[] = $this->buildCompareProperties($drequest, $history_results); - } - $content[] = $this->buildCompareForm( - $request, - array($head_ref => true, $against_ref => true)); - - $content[] = $this->buildHistoryTable( - $history_results, - $history, - $pager, - $history_exception); - - $content[] = $this->renderTablePagerBox($pager); - - return $content; - } - - private function buildCompareProperties($drequest, array $history_results) { - $viewer = $this->getViewer(); - - $request = $this->getRequest(); - $repository = $drequest->getRepository(); - - $head_ref = $request->getStr('head'); - $against_ref = $request->getStr('against'); - - $reverse_uri = $repository->getPathURI( - "compare/?head=${against_ref}&against=${head_ref}"); - $actions = id(new PhabricatorActionListView()); - $actions->setUser($viewer); - - - $actions->addAction(id(new PhabricatorActionView()) - ->setName(pht('Reverse Comparison')) - ->setHref($reverse_uri) - ->setIcon('fa-list')); - - $view = id(new PHUIPropertyListView()) - ->setUser($viewer) - ->setActionList($actions); - - $readme = - 'These are the commits that are reachable from **Head** commit and not '. - 'from the **Against** commit.'; - $readme = new PHUIRemarkupView($viewer, $readme); - $view->addTextContent($readme); - - $view->addProperty(pht('Head'), $head_ref); - $view->addProperty(pht('Against'), $against_ref); - - $header = id(new PHUIHeaderView()) - ->setUser($viewer) - ->setPolicyObject($drequest->getRepository()); - - $box = id(new PHUIObjectBoxView()) - ->setUser($viewer) - ->setHeader($header) - ->addPropertyList($view); - return $box; - } - - private function buildHistoryTable( - $history_results, - $history, - $pager, - $history_exception) { - - $request = $this->getRequest(); - $viewer = $request->getUser(); - $drequest = $this->getDiffusionRequest(); - $repository = $drequest->getRepository(); - - if ($history_exception) { if ($repository->isImporting()) { - return $this->renderStatusMessage( + $history_view = $this->renderStatusMessage( pht('Still Importing...'), pht( 'This repository is still importing. History is not yet '. 'available.')); } else { - return $this->renderStatusMessage( + $history_view = $this->renderStatusMessage( pht('Unable to Retrieve History'), - $history_exception->getMessage()); + $ex->getMessage()); } } + $header = id(new PHUIHeaderView()) + ->setHeader( + pht( + 'Changes on %s but not %s', + phutil_tag('em', array(), $head_ref), + phutil_tag('em', array(), $against_ref))); + + $curtain = $this->buildCurtain($head_ref, $against_ref); + + $column_view = id(new PHUITwoColumnView()) + ->setHeader($header) + ->setCurtain($curtain) + ->setMainColumn( + array( + $history_view, + )); + + return $this->newPage() + ->setTitle( + array( + $repository->getName(), + $repository->getDisplayName(), + )) + ->setCrumbs($crumbs) + ->appendChild($column_view); + } + + private function buildCompareDialog( + $head_ref, + $against_ref, + array $resolved, + $identical) { + + $viewer = $this->getViewer(); + $request = $this->getRequest(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + $e_head = null; + $e_against = null; + $errors = array(); + if ($request->isFormPost()) { + if (!strlen($head_ref)) { + $e_head = pht('Required'); + $errors[] = pht( + 'You must provide two different commits to compare.'); + } else if (!isset($resolved[$head_ref])) { + $e_head = pht('Not Found'); + $errors[] = pht( + 'Commit "%s" is not a valid commit in this repository.', + $head_ref); + } + + if (!strlen($against_ref)) { + $e_against = pht('Required'); + $errors[] = pht( + 'You must provide two different commits to compare.'); + } else if (!isset($resolved[$against_ref])) { + $e_against = pht('Not Found'); + $errors[] = pht( + 'Commit "%s" is not a valid commit in this repository.', + $against_ref); + } + + if ($identical) { + $e_head = pht('Identical'); + $e_against = pht('Identical'); + $errors[] = pht( + 'Both references identify the same commit. You can not compare a '. + 'commit against itself.'); + } + } + + $form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendControl( + id(new AphrontFormTextControl()) + ->setLabel(pht('Head')) + ->setName('head') + ->setError($e_head) + ->setValue($head_ref)) + ->appendControl( + id(new AphrontFormTextControl()) + ->setLabel(pht('Against')) + ->setName('against') + ->setError($e_against) + ->setValue($against_ref)); + + $cancel_uri = $repository->generateURI( + array( + 'action' => 'browse', + )); + + return $this->newDialog() + ->setTitle(pht('Compare Against')) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->setErrors($errors) + ->appendForm($form) + ->addSubmitButton(pht('Compare')) + ->addCancelButton($cancel_uri, pht('Cancel')); + } + + private function buildCurtain($head_ref, $against_ref) { + $viewer = $this->getViewer(); + $request = $this->getRequest(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + $curtain = $this->newCurtainView(null); + + $reverse_uri = $drequest->generateURI( + array( + 'action' => 'compare', + 'head' => $against_ref, + 'against' => $head_ref, + )); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Reverse Comparison')) + ->setHref($reverse_uri) + ->setIcon('fa-refresh')); + + $compare_uri = $drequest->generateURI( + array( + 'action' => 'compare', + 'head' => $head_ref, + )); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Compare Against...')) + ->setIcon('fa-code-fork') + ->setWorkflow(true) + ->setHref($compare_uri)); + + // TODO: Provide a "Show Diff" action. + + return $curtain; + } + + private function newHistoryView( + array $results, + array $history, + PHUIPagerView $pager, + $head_ref, + $against_ref) { + + $request = $this->getRequest(); + $viewer = $this->getViewer(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + if (!$history) { return $this->renderStatusMessage( - pht('Up to date'), - new PHUIRemarkupView( - $viewer, - pht( - '**Against** is strictly ahead of **Head** '. - '- no commits are missing.'))); + pht('Up To Date'), + pht( + 'There are no commits on %s that are not already on %s.', + phutil_tag('strong', array(), $head_ref), + phutil_tag('strong', array(), $against_ref))); } $history_table = id(new DiffusionHistoryTableView()) @@ -238,23 +299,27 @@ final class DiffusionCompareController extends DiffusionController { ->setDiffusionRequest($drequest) ->setHistory($history); - // TODO: Super sketchy? $history_table->loadRevisions(); - if ($history_results) { - $history_table->setParents($history_results['parents']); + if ($history) { + $history_table->setParents($results['parents']); $history_table->setIsHead(!$pager->getOffset()); $history_table->setIsTail(!$pager->getHasMorePages()); } - // TODO also expose diff. - - $panel = new PHUIObjectBoxView(); $header = id(new PHUIHeaderView()) - ->setHeader(pht('Missing Commits')); - $panel->setHeader($header); - $panel->setTable($history_table); + ->setHeader(pht('Commits')); - return $panel; + $object_box = id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setTable($history_table); + + $pager_box = $this->renderTablePagerBox($pager); + + return array( + $object_box, + $pager_box, + ); } } diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index 417052b594..ab08025349 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -644,7 +644,7 @@ abstract class DiffusionRequest extends Phobject { return $match; } - private function resolveRefs(array $refs, array $types) { + public function resolveRefs(array $refs, array $types = array()) { // First, try to resolve refs from fast cache sources. $cached_query = id(new DiffusionCachedResolveRefsQuery()) ->setRepository($this->getRepository()) diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 17415fb496..cc1942d8d2 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -689,6 +689,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO case 'lint': case 'pathtree': case 'refs': + case 'compare': break; case 'branch': // NOTE: This does not actually require a branch, and won't have one @@ -710,6 +711,9 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $commit = idx($params, 'commit'); $line = idx($params, 'line'); + $head = idx($params, 'head'); + $against = idx($params, 'against'); + if ($req_commit && !strlen($commit)) { throw new Exception( pht( @@ -734,11 +738,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $path = phutil_escape_uri($path); } + $raw_branch = $branch; if (strlen($branch)) { $branch = phutil_escape_uri_path_component($branch); $path = "{$branch}/{$path}"; } + $raw_commit = $commit; if (strlen($commit)) { $commit = str_replace('$', '$$', $commit); $commit = ';'.phutil_escape_uri($commit); @@ -748,6 +754,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $line = '$'.phutil_escape_uri($line); } + $query = array(); switch ($action) { case 'change': case 'history': @@ -760,6 +767,20 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO case 'refs': $uri = $this->getPathURI("/{$action}/{$path}{$commit}{$line}"); break; + case 'compare': + $uri = $this->getPathURI("/{$action}/"); + if (strlen($head)) { + $query['head'] = $head; + } else if (strlen($raw_commit)) { + $query['commit'] = $raw_commit; + } else if (strlen($raw_branch)) { + $query['head'] = $raw_branch; + } + + if (strlen($against)) { + $query['against'] = $against; + } + break; case 'branch': if (strlen($path)) { $uri = $this->getPathURI("/repository/{$path}"); @@ -791,8 +812,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO ); } - if (idx($params, 'params')) { - $uri->setQueryParams($params['params']); + $query = idx($params, 'params', array()) + $query; + + if ($query) { + $uri->setQueryParams($query); } return $uri; @@ -2066,6 +2089,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $result; } + public function supportsBranchComparison() { + return $this->isGit(); + } + /* -( Repository URIs )---------------------------------------------------- */