From 826e5405b61153180081acc1b79070aeb3d0298b Mon Sep 17 00:00:00 2001 From: vrana Date: Tue, 1 May 2012 12:09:50 -0700 Subject: [PATCH] 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 --- src/__celerity_resource_map__.php | 83 ++++++++++--------- .../DifferentialDiffViewController.php | 1 + .../DifferentialRevisionViewController.php | 5 +- .../DifferentialLintFieldSpecification.php | 3 +- .../DifferentialChangesetListView.php | 30 ++++++- .../DifferentialDiffTableOfContentsView.php | 45 ++++------ .../view/difftableofcontents/__init__.php | 2 +- .../change/DiffusionChangeController.php | 9 +- .../commit/DiffusionCommitController.php | 1 + .../differential/changeset-view.css | 13 +++ .../differential/behavior-populate.js | 20 +++++ 11 files changed, 131 insertions(+), 81 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index a7519fa877..935f52926f 100644 --- 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', diff --git a/src/applications/differential/controller/diffview/DifferentialDiffViewController.php b/src/applications/differential/controller/diffview/DifferentialDiffViewController.php index 4f6326768e..3f9f459798 100644 --- 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()); diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 82926b5bd2..cbdb555aa3 100644 --- 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); diff --git a/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php b/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php index b1154ba128..1d6b3c67df 100644 --- 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( diff --git a/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php b/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php index 73f8b09d9c..b66fdcc96f 100644 --- 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, ), - '
Loading...
')); + '
'.$load.'
')); $output[] = $detail->render(); - - $mapping[$uniq_id] = $ref; } require_celerity_resource('aphront-tooltip-css'); diff --git a/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php b/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php index bd2a996fbf..1f83884de2 100644 --- 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)); } diff --git a/src/applications/differential/view/difftableofcontents/__init__.php b/src/applications/differential/view/difftableofcontents/__init__.php index cc2333699b..7a02fbad3d 100644 --- 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'); diff --git a/src/applications/diffusion/controller/change/DiffusionChangeController.php b/src/applications/diffusion/controller/change/DiffusionChangeController.php index c11c05ea1d..ff30e34deb 100644 --- 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(), diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index ec3afefaae..d5363a4bd9 100644 --- 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); diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 209ca0daf1..1e9261f54d 100644 --- 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; +} diff --git a/webroot/rsrc/js/application/differential/behavior-populate.js b/webroot/rsrc/js/application/differential/behavior-populate.js index 42ab6ac2cd..96f1a40b6f 100644 --- 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('
Loading...
')); + 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'],