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
2024-12-30 09:20:58 +01:00 · 2016-12-05 17:40:24 -08:00 · 2016-12-05 17:40:24 -08:00 · fc1adf9875
commit fc1adf9875
parent 43f9927a38
4 changed files with 287 additions and 179 deletions
--- 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' => '',
--- 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();
-
-
-    if (count($refs) == 2) {
-      $content[] = $this->buildCompareContent($drequest);
-    } else {
-      $content[] = $this->buildCompareForm($request, $refs);
+    $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;

-    return $this->newPage()
-      ->setTitle(
+          // 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;
+        }
+      }
+    }
+
+    $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 $form;
+      return id(new AphrontRedirectResponse())->setURI($compare_uri);
    }

-  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('Up To Date'),
        pht(
-            '**Against** is strictly ahead of **Head** '.
-            '- no commits are missing.')));
+          '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,
+    );
  }
 }
--- 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())
--- 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  )---------------------------------------------------- */