Open files in very large diffs inline

Summary: This allows writing inline comments and reduces different behavior between normal and very large diffs. Test Plan: Verify that normal diff works. Verify that very large diff works. Reviewers: epriestley Reviewed By: epriestley CC: aran, Koolvin Differential Revision: https://secure.phabricator.com/D2361
2024-12-27 07:50:57 +01:00 · 2012-05-01 12:09:50 -07:00 · 2012-05-01 12:09:50 -07:00 · 826e5405b6
commit 826e5405b6
parent ec068ff453
11 changed files with 131 additions and 81 deletions
--- a/src/__celerity_resource_map__.php
+++ b/src/__celerity_resource_map__.php
@ -308,6 +308,13 @@ celerity_register_resource_map(array(
    'disk' => '/rsrc/image/icon/tango/edit.png',
    'type' => 'png',
  ),
+  '/rsrc/image/icon/tango/go-down.png' =>
+  array(
+    'hash' => '96862812cbb0445573c264dc057b8300',
+    'uri' => '/res/96862812/rsrc/image/icon/tango/go-down.png',
+    'disk' => '/rsrc/image/icon/tango/go-down.png',
+    'type' => 'png',
+  ),
  '/rsrc/image/icon/tango/log.png' =>
  array(
    'hash' => 'a6f72499bef279ff6807a7dbc5148f1e',
@ -431,7 +438,7 @@ celerity_register_resource_map(array(
  ),
  'aphront-headsup-action-list-view-css' =>
  array(
-    'uri' => '/res/9c5cd292/rsrc/css/aphront/headsup-action-list-view.css',
+    'uri' => '/res/665cdc49/rsrc/css/aphront/headsup-action-list-view.css',
    'type' => 'css',
    'requires' =>
    array(
@ -531,7 +538,7 @@ celerity_register_resource_map(array(
  ),
  'differential-changeset-view-css' =>
  array(
-    'uri' => '/res/c7edd492/rsrc/css/application/differential/changeset-view.css',
+    'uri' => '/res/a352da17/rsrc/css/application/differential/changeset-view.css',
    'type' => 'css',
    'requires' =>
    array(
@ -962,7 +969,7 @@ celerity_register_resource_map(array(
  ),
  'javelin-behavior-differential-populate' =>
  array(
-    'uri' => '/res/8ea8cb57/rsrc/js/application/differential/behavior-populate.js',
+    'uri' => '/res/c0979571/rsrc/js/application/differential/behavior-populate.js',
    'type' => 'js',
    'requires' =>
    array(
@ -2503,7 +2510,7 @@ celerity_register_resource_map(array(
      'uri' => '/res/pkg/0c96375e/core.pkg.js',
      'type' => 'js',
    ),
-    '27683aba' =>
+    '59d298c3' =>
    array(
      'name' => 'differential.pkg.css',
      'symbols' =>
@ -2522,10 +2529,10 @@ celerity_register_resource_map(array(
        11 => 'differential-local-commits-view-css',
        12 => 'inline-comment-summary-css',
      ),
-      'uri' => '/res/pkg/27683aba/differential.pkg.css',
+      'uri' => '/res/pkg/59d298c3/differential.pkg.css',
      'type' => 'css',
    ),
-    '5e9ae855' =>
+    '5b7b36d7' =>
    array(
      'name' => 'differential.pkg.js',
      'symbols' =>
@ -2549,7 +2556,7 @@ celerity_register_resource_map(array(
        16 => 'javelin-behavior-differential-dropdown-menus',
        17 => 'javelin-behavior-buoyant',
      ),
-      'uri' => '/res/pkg/5e9ae855/differential.pkg.js',
+      'uri' => '/res/pkg/5b7b36d7/differential.pkg.js',
      'type' => 'js',
    ),
    'c8ce2d88' =>
@ -2645,7 +2652,7 @@ celerity_register_resource_map(array(
    'aphront-dialog-view-css' => '9c4e265b',
    'aphront-error-view-css' => '9c4e265b',
    'aphront-form-view-css' => '9c4e265b',
-    'aphront-headsup-action-list-view-css' => '27683aba',
+    'aphront-headsup-action-list-view-css' => '59d298c3',
    'aphront-headsup-view-css' => '9c4e265b',
    'aphront-list-filter-view-css' => '9c4e265b',
    'aphront-pager-view-css' => '9c4e265b',
@ -2655,36 +2662,36 @@ celerity_register_resource_map(array(
    'aphront-tokenizer-control-css' => '9c4e265b',
    'aphront-tooltip-css' => '9c4e265b',
    'aphront-typeahead-control-css' => '9c4e265b',
-    'differential-changeset-view-css' => '27683aba',
-    'differential-core-view-css' => '27683aba',
-    'differential-inline-comment-editor' => '5e9ae855',
-    'differential-local-commits-view-css' => '27683aba',
-    'differential-results-table-css' => '27683aba',
-    'differential-revision-add-comment-css' => '27683aba',
-    'differential-revision-comment-css' => '27683aba',
-    'differential-revision-comment-list-css' => '27683aba',
-    'differential-revision-history-css' => '27683aba',
-    'differential-table-of-contents-css' => '27683aba',
+    'differential-changeset-view-css' => '59d298c3',
+    'differential-core-view-css' => '59d298c3',
+    'differential-inline-comment-editor' => '5b7b36d7',
+    'differential-local-commits-view-css' => '59d298c3',
+    'differential-results-table-css' => '59d298c3',
+    'differential-revision-add-comment-css' => '59d298c3',
+    'differential-revision-comment-css' => '59d298c3',
+    'differential-revision-comment-list-css' => '59d298c3',
+    'differential-revision-history-css' => '59d298c3',
+    'differential-table-of-contents-css' => '59d298c3',
    'diffusion-commit-view-css' => 'c8ce2d88',
    'diffusion-icons-css' => 'c8ce2d88',
-    'inline-comment-summary-css' => '27683aba',
+    'inline-comment-summary-css' => '59d298c3',
    'javelin-behavior' => '8a5de8a3',
    'javelin-behavior-aphront-basic-tokenizer' => '97f65640',
-    'javelin-behavior-aphront-drag-and-drop' => '5e9ae855',
-    'javelin-behavior-aphront-drag-and-drop-textarea' => '5e9ae855',
+    'javelin-behavior-aphront-drag-and-drop' => '5b7b36d7',
+    'javelin-behavior-aphront-drag-and-drop-textarea' => '5b7b36d7',
    'javelin-behavior-aphront-form-disable-on-submit' => '0c96375e',
    'javelin-behavior-audit-preview' => '5e68be89',
-    'javelin-behavior-buoyant' => '5e9ae855',
-    'javelin-behavior-differential-accept-with-errors' => '5e9ae855',
-    'javelin-behavior-differential-add-reviewers-and-ccs' => '5e9ae855',
-    'javelin-behavior-differential-comment-jump' => '5e9ae855',
-    'javelin-behavior-differential-diff-radios' => '5e9ae855',
-    'javelin-behavior-differential-dropdown-menus' => '5e9ae855',
-    'javelin-behavior-differential-edit-inline-comments' => '5e9ae855',
-    'javelin-behavior-differential-feedback-preview' => '5e9ae855',
-    'javelin-behavior-differential-keyboard-navigation' => '5e9ae855',
-    'javelin-behavior-differential-populate' => '5e9ae855',
-    'javelin-behavior-differential-show-more' => '5e9ae855',
+    'javelin-behavior-buoyant' => '5b7b36d7',
+    'javelin-behavior-differential-accept-with-errors' => '5b7b36d7',
+    'javelin-behavior-differential-add-reviewers-and-ccs' => '5b7b36d7',
+    'javelin-behavior-differential-comment-jump' => '5b7b36d7',
+    'javelin-behavior-differential-diff-radios' => '5b7b36d7',
+    'javelin-behavior-differential-dropdown-menus' => '5b7b36d7',
+    'javelin-behavior-differential-edit-inline-comments' => '5b7b36d7',
+    'javelin-behavior-differential-feedback-preview' => '5b7b36d7',
+    'javelin-behavior-differential-keyboard-navigation' => '5b7b36d7',
+    'javelin-behavior-differential-populate' => '5b7b36d7',
+    'javelin-behavior-differential-show-more' => '5b7b36d7',
    'javelin-behavior-diffusion-commit-graph' => '5e68be89',
    'javelin-behavior-diffusion-pull-lastmodified' => '5e68be89',
    'javelin-behavior-maniphest-batch-selector' => '7707de41',
@ -2694,12 +2701,12 @@ celerity_register_resource_map(array(
    'javelin-behavior-maniphest-transaction-preview' => '7707de41',
    'javelin-behavior-phabricator-autofocus' => '0c96375e',
    'javelin-behavior-phabricator-keyboard-shortcuts' => '0c96375e',
-    'javelin-behavior-phabricator-object-selector' => '5e9ae855',
+    'javelin-behavior-phabricator-object-selector' => '5b7b36d7',
    'javelin-behavior-phabricator-oncopy' => '0c96375e',
    'javelin-behavior-phabricator-tooltips' => '0c96375e',
    'javelin-behavior-phabricator-watch-anchor' => '0c96375e',
    'javelin-behavior-refresh-csrf' => '0c96375e',
-    'javelin-behavior-repository-crossreference' => '5e9ae855',
+    'javelin-behavior-repository-crossreference' => '5b7b36d7',
    'javelin-behavior-workflow' => '0c96375e',
    'javelin-dom' => '8a5de8a3',
    'javelin-event' => '8a5de8a3',
@ -2721,23 +2728,23 @@ celerity_register_resource_map(array(
    'maniphest-task-summary-css' => '7839ae2d',
    'maniphest-transaction-detail-css' => '7839ae2d',
    'phabricator-app-buttons-css' => '9c4e265b',
-    'phabricator-content-source-view-css' => '27683aba',
+    'phabricator-content-source-view-css' => '59d298c3',
    'phabricator-core-buttons-css' => '9c4e265b',
    'phabricator-core-css' => '9c4e265b',
    'phabricator-directory-css' => '9c4e265b',
-    'phabricator-drag-and-drop-file-upload' => '5e9ae855',
+    'phabricator-drag-and-drop-file-upload' => '5b7b36d7',
    'phabricator-dropdown-menu' => '0c96375e',
    'phabricator-flag-css' => '9c4e265b',
    'phabricator-jump-nav' => '9c4e265b',
    'phabricator-keyboard-shortcut' => '0c96375e',
    'phabricator-keyboard-shortcut-manager' => '0c96375e',
    'phabricator-menu-item' => '0c96375e',
-    'phabricator-object-selector-css' => '27683aba',
+    'phabricator-object-selector-css' => '59d298c3',
    'phabricator-paste-file-upload' => '0c96375e',
    'phabricator-prefab' => '0c96375e',
    'phabricator-project-tag-css' => '7839ae2d',
    'phabricator-remarkup-css' => '9c4e265b',
-    'phabricator-shaped-request' => '5e9ae855',
+    'phabricator-shaped-request' => '5b7b36d7',
    'phabricator-standard-page-view' => '9c4e265b',
    'phabricator-tooltip' => '0c96375e',
    'phabricator-transaction-view-css' => '9c4e265b',
--- a/src/applications/differential/controller/diffview/DifferentialDiffViewController.php
+++ b/src/applications/differential/controller/diffview/DifferentialDiffViewController.php
@ -115,6 +115,7 @@ final class DifferentialDiffViewController extends DifferentialController {

    $details = id(new DifferentialChangesetListView())
      ->setChangesets($changesets)
+      ->setVisibleChangesets($changesets)
      ->setRenderingReferences($refs)
      ->setStandaloneURI('/differential/changeset/')
      ->setUser($request->getUser());
--- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php
@ -238,7 +238,8 @@ final class DifferentialRevisionViewController extends DifferentialController {
    $comment_view->setVersusDiffID($diff_vs);

    $changeset_view = new DifferentialChangesetListView();
-    $changeset_view->setChangesets($visible_changesets);
+    $changeset_view->setChangesets($changesets);
+    $changeset_view->setVisibleChangesets($visible_changesets);

    if (!$viewer_is_anonymous) {
      $changeset_view->setInlineCommentControllerURI(
@ -288,13 +289,13 @@ final class DifferentialRevisionViewController extends DifferentialController {
    $toc_view = new DifferentialDiffTableOfContentsView();
    $toc_view->setChangesets($changesets);
    $toc_view->setVisibleChangesets($visible_changesets);
+    $toc_view->setRenderingReferences($rendering_references);
    $toc_view->setUnitTestData(idx($props, 'arc:unit', array()));
    if ($repository) {
      $toc_view->setRepository($repository);
    }
    $toc_view->setDiff($target);
    $toc_view->setUser($user);
-    $toc_view->setStandaloneViewLink(empty($visible_changesets));
    $toc_view->setVsMap($vs_map);
    $toc_view->setRevisionID($revision->getID());
    $toc_view->setWhitespace($whitespace);
--- a/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php
+++ b/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php
@ -88,8 +88,7 @@ final class DifferentialLintFieldSpecification

          $line_link = 'line '.phutil_escape_html($line);
          if (isset($path_changesets[$path])) {
-            // TODO: Create standalone links for large diffs. Logic is in
-            // DifferentialDiffTableOfContentsView::renderChangesetLink().
+            // TODO: Load very large diff before linking to line.
            $line_link = phutil_render_tag(
              'a',
              array(
--- a/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php
+++ b/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php
@ -19,6 +19,7 @@
 final class DifferentialChangesetListView extends AphrontView {

  private $changesets = array();
+  private $visibleChangesets = array();
  private $references = array();
  private $inlineURI;
  private $renderURI = '/differential/changeset/';
@ -39,6 +40,11 @@ final class DifferentialChangesetListView extends AphrontView {
    return $this;
  }

+  public function setVisibleChangesets($visible_changesets) {
+    $this->visibleChangesets = $visible_changesets;
+    return $this;
+  }
+
  public function setInlineCommentControllerURI($uri) {
    $this->inlineURI = $uri;
    return $this;
@ -130,17 +136,33 @@ final class DifferentialChangesetListView extends AphrontView {
      $detail->setVsChangesetID(idx($this->vsMap, $changeset->getID()));
      $detail->setEditable(true);

-      $uniq_id = celerity_generate_unique_node_id();
+      $uniq_id = 'diff-'.$changeset->getAnchorName();
+      if (isset($this->visibleChangesets[$key])) {
+        $load = 'Loading...';
+        $mapping[$uniq_id] = $ref;
+      } else {
+        $load = javelin_render_tag(
+          'a',
+          array(
+            'href' => '#'.$uniq_id,
+            'meta' => array(
+              'id' => $uniq_id,
+              'ref' => $ref,
+              'kill' => true,
+            ),
+            'sigil' => 'differential-load',
+            'mustcapture' => true,
+          ),
+          'Load');
+      }
      $detail->appendChild(
        phutil_render_tag(
          'div',
          array(
            'id' => $uniq_id,
          ),
-          '<div class="differential-loading">Loading...</div>'));
+          '<div class="differential-loading">'.$load.'</div>'));
      $output[] = $detail->render();
-
-      $mapping[$uniq_id] = $ref;
    }

    require_celerity_resource('aphront-tooltip-css');
--- a/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php
+++ b/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php
@ -20,10 +20,10 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {

  private $changesets = array();
  private $visibleChangesets = array();
+  private $references = array();
  private $repository;
  private $diff;
  private $user;
-  private $standaloneViewLink = null;
  private $renderURI = '/differential/changeset/';
  private $revisionID;
  private $whitespace;
@ -39,6 +39,11 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
    return $this;
  }

+  public function setRenderingReferences(array $references) {
+    $this->references = $references;
+    return $this;
+  }
+
  public function setRepository(PhabricatorRepository $repository) {
    $this->repository = $repository;
    return $this;
@ -59,11 +64,6 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
    return $this;
  }

-  public function setStandaloneViewLink($standalone_view_link) {
-    $this->standaloneViewLink = $standalone_view_link;
-    return $this;
-  }
-
  public function setVsMap(array $vs_map) {
    $this->vsMap = $vs_map;
    return $this;
@ -108,7 +108,8 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
    foreach ($changesets as $id => $changeset) {
      $type = $changeset->getChangeType();
      $ftype = $changeset->getFileType();
-      $link = $this->renderChangesetLink($changeset);
+      $ref = idx($this->references, $id);
+      $link = $this->renderChangesetLink($changeset, $ref);

      if (DifferentialChangeType::isOldLocationChangeType($type)) {
        $away = $changeset->getAwayPaths();
@ -253,34 +254,18 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
  }


-  private function renderChangesetLink(DifferentialChangeset $changeset) {
+  private function renderChangesetLink(DifferentialChangeset $changeset, $ref) {
    $display_file = $changeset->getDisplayFilename();

-    if ($this->standaloneViewLink) {
-      $id = $changeset->getID();
-      $vs_id = idx($this->vsMap, $id);
-
-      $ref = $vs_id ? $id.'/'.$vs_id : $id;
-      $detail_uri = new PhutilURI($this->renderURI);
-      $detail_uri->setQueryParams(
-        array(
-          'ref'         => $ref,
-          'whitespace'  => $this->whitespace,
-          'revision_id' => $this->revisionID,
-        ));
-
-      return phutil_render_tag(
-        'a',
-        array(
-          'href' => $detail_uri,
-          'target'  => '_blank',
-        ),
-        phutil_escape_html($display_file));
-    }
-    return phutil_render_tag(
+    return javelin_render_tag(
      'a',
      array(
        'href' => '#'.$changeset->getAnchorName(),
+        'meta' => array(
+          'id' => 'diff-'.$changeset->getAnchorName(),
+          'ref' => $ref,
+        ),
+        'sigil' => 'differential-load',
      ),
      phutil_escape_html($display_file));
  }
--- a/src/applications/differential/view/difftableofcontents/init.php
+++ b/src/applications/differential/view/difftableofcontents/init.php
@ -10,10 +10,10 @@ phutil_require_module('arcanist', 'unit/result');

 phutil_require_module('phabricator', 'applications/differential/constants/changetype');
 phutil_require_module('phabricator', 'infrastructure/celerity/api');
+phutil_require_module('phabricator', 'infrastructure/javelin/markup');
 phutil_require_module('phabricator', 'view/base');

 phutil_require_module('phutil', 'markup');
-phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');


--- a/src/applications/diffusion/controller/change/DiffusionChangeController.php
+++ b/src/applications/diffusion/controller/change/DiffusionChangeController.php
@ -32,12 +32,13 @@ final class DiffusionChangeController extends DiffusionController {
    }

    $callsign = $drequest->getRepository()->getCallsign();
+    $changesets = array(
+      0 => $changeset,
+    );

    $changeset_view = new DifferentialChangesetListView();
-    $changeset_view->setChangesets(
-      array(
-        0 => $changeset,
-      ));
+    $changeset_view->setChangesets($changesets);
+    $changeset_view->setVisibleChangesets($changesets);
    $changeset_view->setRenderingReferences(
      array(
        0 => $diff_query->getRenderingReference(),
--- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php
+++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php
@ -232,6 +232,7 @@ final class DiffusionCommitController extends DiffusionController {

      $change_list = new DifferentialChangesetListView();
      $change_list->setChangesets($changesets);
+      $change_list->setVisibleChangesets($changesets);
      $change_list->setRenderingReferences($references);
      $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/');
      $change_list->setRepository($repository);
--- a/webroot/rsrc/css/application/differential/changeset-view.css
+++ b/webroot/rsrc/css/application/differential/changeset-view.css
@ -379,3 +379,16 @@ tr.differential-inline-loading {
  display: block;
  margin-top: -72px;
 }
+
+.differential-loading a {
+  color: #3b5998;
+}
+
+.differential-loading {
+  color: #999966;
+  background: #ffffee;
+  border: 1px solid #ccccaa;
+  padding: 1em;
+  text-align: center;
+  font-size: 11px;
+}
--- a/webroot/rsrc/js/application/differential/behavior-populate.js
+++ b/webroot/rsrc/js/application/differential/behavior-populate.js
@ -37,6 +37,26 @@ JX.behavior('differential-populate', function(config) {
  var highlighted = null;
  var highlight_class = null;

+  JX.Stratcom.listen(
+    'click',
+    'differential-load',
+    function(e) {
+      var meta = e.getNodeData('differential-load');
+      JX.DOM.setContent(
+        JX.$(meta.id),
+        JX.$H('<div class="differential-loading">Loading...</div>'));
+      var data = {
+        ref : meta.ref,
+        whitespace : config.whitespace
+      };
+      new JX.Workflow(config.uri, data)
+        .setHandler(JX.bind(null, onresponse, meta.id))
+        .start();
+      if (meta.kill) {
+        e.kill();
+      }
+    });
+
  JX.Stratcom.listen(
    ['mouseover', 'mouseout'],
    ['differential-changeset', 'tag:td'],