From 54154e4f48d0894e8d6e5e8ac5df7ea10134fb9a Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 May 2011 21:46:29 -0700 Subject: [PATCH] Move "Rendering References" to the DifferentialChangesetParser level Summary: Separates changeset IDs from rendering. Now each changeset has a "rendering reference" which is basically a description of what the ajax endpoint should render. For Differential, it's in the form "id/vs". For Diffusion, "branch/path;commit". I believe this fixes pretty much all of the bugs related to "show more" breaking in various obscure ways, although I never got a great repro for T153. Test Plan: Clicked "show more" in diffusion change and commit views and differential diff, diff-of-diff, standalone-diff, standalone-diff-of-diff views. Verified refs and 'whitespace' were always sent correctly. Made inline comments on diffs and diffs-of-diffs. Used "Reply". Reviewed By: tuomaspelkonen Reviewers: tuomaspelkonen, jungejason, aran CC: aran, tuomaspelkonen, epriestley Differential Revision: 274 --- .../DifferentialChangesetViewController.php | 13 +++++++-- .../DifferentialRevisionViewController.php | 16 ++++++++--- .../changeset/DifferentialChangesetParser.php | 27 ++++++++----------- .../changeset/DifferentialChangeset.php | 13 --------- .../DifferentialChangesetListView.php | 22 +++++---------- .../view/changesetlistview/__init__.php | 1 - .../change/DiffusionChangeController.php | 11 +++++++- .../diffusion/controller/change/__init__.php | 1 + .../commit/DiffusionCommitController.php | 4 ++- .../diff/DiffusionDiffController.php | 5 ++-- .../DiffusionLastModifiedController.php | 2 +- .../query/diff/base/DiffusionDiffQuery.php | 5 ++++ .../query/diff/git/DiffusionGitDiffQuery.php | 4 +-- .../query/diff/svn/DiffusionSvnDiffQuery.php | 3 +-- .../differential/behavior-populate.js | 3 +-- 15 files changed, 67 insertions(+), 63 deletions(-) diff --git a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php index 90f0eee714..647143ebf6 100644 --- a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php @@ -24,9 +24,17 @@ class DifferentialChangesetViewController extends DifferentialController { $author_phid = $request->getUser()->getPHID(); - $id = $request->getStr('id'); - $vs = $request->getInt('vs'); + $rendering_reference = $request->getStr('ref'); + $parts = explode('/', $rendering_reference); + if (count($parts) == 2) { + list($id, $vs) = $parts; + } else { + $id = $parts[0]; + $vs = 0; + } + $id = (int)$id; + $vs = (int)$vs; $changeset = id(new DifferentialChangeset())->load($id); if (!$changeset) { @@ -149,6 +157,7 @@ class DifferentialChangesetViewController extends DifferentialController { $parser = new DifferentialChangesetParser(); $parser->setChangeset($changeset); + $parser->setRenderingReference($rendering_reference); $parser->setRenderCacheKey($render_cache_key); $parser->setRightSideCommentMapping($right_source, $right_new); $parser->setLeftSideCommentMapping($left_source, $left_new); diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 99662a9980..8d4e1d9f5f 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -58,7 +58,7 @@ class DifferentialRevisionViewController extends DifferentialController { $diff_vs = null; } - list($changesets, $vs_map) = + list($changesets, $vs_map, $rendering_references) = $this->loadChangesetsAndVsMap($diffs, $diff_vs, $target); $comments = $revision->loadComments(); @@ -181,7 +181,7 @@ class DifferentialRevisionViewController extends DifferentialController { $changeset_view->setEditable(true); $changeset_view->setStandaloneViews(true); $changeset_view->setRevision($revision); - $changeset_view->setVsMap($vs_map); + $changeset_view->setRenderingReferences($rendering_references); $changeset_view->setWhitespace($whitespace); $diff_history = new DifferentialRevisionUpdateHistoryView(); @@ -604,6 +604,11 @@ class DifferentialRevisionViewController extends DifferentialController { $changesets = idx($changeset_groups, $target->getID(), array()); $changesets = mpull($changesets, null, 'getID'); + $refs = array(); + foreach ($changesets as $changeset) { + $refs[$changeset->getID()] = $changeset->getID(); + } + $vs_map = array(); if ($diff_vs) { $vs_changesets = idx($changeset_groups, $diff_vs, array()); @@ -612,18 +617,23 @@ class DifferentialRevisionViewController extends DifferentialController { $file = $changeset->getFilename(); if (isset($vs_changesets[$file])) { $vs_map[$changeset->getID()] = $vs_changesets[$file]->getID(); + $refs[$changeset->getID()] = + $changeset->getID().'/'.$vs_changesets[$file]->getID(); unset($vs_changesets[$file]); + } else { + $refs[$changeset->getID()] = $changeset->getID(); } } foreach ($vs_changesets as $changeset) { $changesets[$changeset->getID()] = $changeset; $vs_map[$changeset->getID()] = -1; + $refs[$changeset->getID()] = $changeset->getID().'/-1'; } } $changesets = msort($changesets, 'getSortKey'); - return array($changesets, $vs_map); + return array($changesets, $vs_map, $refs); } private function updateViewTime($user_phid, $revision_phid) { diff --git a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php index 1fc45737f6..f3a1c3fc58 100644 --- a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php @@ -28,7 +28,6 @@ class DifferentialChangesetParser { protected $filename = null; protected $filetype = null; - protected $changesetID = null; protected $missingOld = array(); protected $missingNew = array(); @@ -52,6 +51,8 @@ class DifferentialChangesetParser { private $rightSideChangesetID; private $rightSideAttachesToNewFile; + private $renderingReference; + const CACHE_VERSION = 4; const ATTR_GENERATED = 'attr:generated'; @@ -123,9 +124,7 @@ class DifferentialChangesetParser { public function setChangeset($changeset) { $this->changeset = $changeset; - $this->setFilename($changeset->getFilename()); - $this->setChangesetID($changeset->getID()); return $this; } @@ -135,8 +134,8 @@ class DifferentialChangesetParser { return $this; } - public function setChangesetID($changeset_id) { - $this->changesetID = $changeset_id; + public function setRenderingReference($ref) { + $this->renderingReference = $ref; return $this; } @@ -144,10 +143,6 @@ class DifferentialChangesetParser { return $this->changeset; } - public function getChangesetID() { - return $this->changesetID; - } - public function setFilename($filename) { $this->filename = $filename; if (strpos($filename, '.', 1) !== false) { @@ -968,7 +963,7 @@ EOSYNTHETIC; if ($more) { $end = $this->getLength(); - $reference = $this->getChangeset()->getRenderingReference(); + $reference = $this->renderingReference; $more = ' '. javelin_render_tag( @@ -979,8 +974,8 @@ EOSYNTHETIC; 'class' => 'complete', 'href' => '#', 'meta' => array( - 'id' => $reference, - 'range' => "0-{$end}", + 'ref' => $reference, + 'range' => "0-{$end}", ), ), 'Show File Contents'); @@ -1073,7 +1068,7 @@ EOSYNTHETIC; $gaps = array_reverse($gaps); - $reference = $this->getChangeset()->getRenderingReference(); + $reference = $this->renderingReference; $left_id = $this->leftSideChangesetID; $right_id = $this->rightSideChangesetID; @@ -1112,7 +1107,7 @@ EOSYNTHETIC; 'mustcapture' => true, 'sigil' => 'show-more', 'meta' => array( - 'id' => $reference, + 'ref' => $reference, 'range' => "{$top}-{$len}/{$top}-20", ), ), @@ -1126,7 +1121,7 @@ EOSYNTHETIC; 'mustcapture' => true, 'sigil' => 'show-more', 'meta' => array( - 'id' => $reference, + 'ref' => $reference, 'range' => "{$top}-{$len}/{$top}-{$len}", ), ), @@ -1140,7 +1135,7 @@ EOSYNTHETIC; 'mustcapture' => true, 'sigil' => 'show-more', 'meta' => array( - 'id' => $reference, + 'ref' => $reference, 'range' => "{$top}-{$len}/{$end}-20", ), ), diff --git a/src/applications/differential/storage/changeset/DifferentialChangeset.php b/src/applications/differential/storage/changeset/DifferentialChangeset.php index 29f1b402ca..3e87d18ee6 100644 --- a/src/applications/differential/storage/changeset/DifferentialChangeset.php +++ b/src/applications/differential/storage/changeset/DifferentialChangeset.php @@ -32,7 +32,6 @@ class DifferentialChangeset extends DifferentialDAO { private $unsavedHunks = array(); private $hunks; - private $renderingReference; protected function getConfiguration() { return array( @@ -76,18 +75,6 @@ class DifferentialChangeset extends DifferentialDAO { return $name; } - public function setRenderingReference($rendering_reference) { - $this->renderingReference = $rendering_reference; - return $this; - } - - public function getRenderingReference() { - if ($this->renderingReference) { - return $this->renderingReference; - } - return $this->getID(); - } - public function addUnsavedHunk(DifferentialHunk $hunk) { if ($this->hunks === null) { $this->hunks = array(); diff --git a/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php b/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php index a3605a92a6..4acc578a49 100644 --- a/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php +++ b/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php @@ -22,7 +22,6 @@ class DifferentialChangesetListView extends AphrontView { private $editable; private $revision; private $renderURI = '/differential/changeset/'; - private $vsMap = array(); private $whitespace; private $standaloneViews; @@ -46,8 +45,8 @@ class DifferentialChangesetListView extends AphrontView { return $this; } - public function setVsMap(array $vs_map) { - $this->vsMap = $vs_map; + public function setRenderingReferences(array $references) { + $this->references = $references; return $this; } @@ -64,7 +63,6 @@ class DifferentialChangesetListView extends AphrontView { public function render() { require_celerity_resource('differential-changeset-view-css'); - $vs_map = $this->vsMap; $changesets = $this->changesets; $output = array(); @@ -75,22 +73,15 @@ class DifferentialChangesetListView extends AphrontView { if (!$this->editable) { $class .= ' differential-changeset-noneditable'; } - $id = $changeset->getID(); - if ($id) { - $vs_id = idx($vs_map, $id); - } else { - $vs_id = null; - } - $ref = $changeset->getRenderingReference(); + $ref = $this->references[$key]; $detail_button = null; if ($this->standaloneViews) { $detail_uri = new PhutilURI($this->renderURI); $detail_uri->setQueryParams( array( - 'id' => $ref, - 'vs' => $vs_id, + 'ref' => $ref, 'whitespace' => $this->whitespace, )); @@ -118,9 +109,7 @@ class DifferentialChangesetListView extends AphrontView { '
Loading...
')); $output[] = $detail->render(); - $mapping[$uniq_id] = array( - $ref, - $vs_id); + $mapping[$uniq_id] = $ref; } Javelin::initBehavior('differential-populate', array( @@ -131,6 +120,7 @@ class DifferentialChangesetListView extends AphrontView { Javelin::initBehavior('differential-show-more', array( 'uri' => $this->renderURI, + 'whitespace' => $this->whitespace, )); Javelin::initBehavior('differential-comment-jump', array()); diff --git a/src/applications/differential/view/changesetlistview/__init__.php b/src/applications/differential/view/changesetlistview/__init__.php index 09a25e7875..3077c4f454 100644 --- a/src/applications/differential/view/changesetlistview/__init__.php +++ b/src/applications/differential/view/changesetlistview/__init__.php @@ -13,7 +13,6 @@ phutil_require_module('phabricator', 'view/base'); phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'parser/uri'); -phutil_require_module('phutil', 'utils'); phutil_require_source('DifferentialChangesetListView.php'); diff --git a/src/applications/diffusion/controller/change/DiffusionChangeController.php b/src/applications/diffusion/controller/change/DiffusionChangeController.php index 5876d479ed..80f2d754a9 100644 --- a/src/applications/diffusion/controller/change/DiffusionChangeController.php +++ b/src/applications/diffusion/controller/change/DiffusionChangeController.php @@ -32,9 +32,18 @@ class DiffusionChangeController extends DiffusionController { } $changeset_view = new DifferentialChangesetListView(); - $changeset_view->setChangesets(array($changeset)); + $changeset_view->setChangesets( + array( + 0 => $changeset, + )); + $changeset_view->setRenderingReferences( + array( + 0 => $diff_query->getRenderingReference(), + )); $changeset_view->setRenderURI( '/diffusion/'.$drequest->getRepository()->getCallsign().'/diff/'); + $changeset_view->setWhitespace( + DifferentialChangesetParser::WHITESPACE_SHOW_ALL); $content[] = $this->buildCrumbs( array( diff --git a/src/applications/diffusion/controller/change/__init__.php b/src/applications/diffusion/controller/change/__init__.php index 1d9dfc8af6..08d9f0fb30 100644 --- a/src/applications/diffusion/controller/change/__init__.php +++ b/src/applications/diffusion/controller/change/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'aphront/response/404'); +phutil_require_module('phabricator', 'applications/differential/parser/changeset'); phutil_require_module('phabricator', 'applications/differential/view/changesetlistview'); phutil_require_module('phabricator', 'applications/diffusion/controller/base'); phutil_require_module('phabricator', 'applications/diffusion/query/diff/base'); diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index 50e5e5203a..ed34a9bf65 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -146,6 +146,7 @@ class DiffusionCommitController extends DiffusionController { throw new Exception("Unknown VCS."); } + $references = array(); foreach ($changesets as $key => $changeset) { $file_type = $changeset->getFileType(); if ($file_type == DifferentialChangeType::FILE_DIRECTORY) { @@ -160,11 +161,12 @@ class DiffusionCommitController extends DiffusionController { $filename = $changeset->getFilename(); $commit = $drequest->getCommit(); $reference = "{$branch}{$filename};{$commit}"; - $changeset->setRenderingReference($reference); + $references[$key] = $reference; } $change_list = new DifferentialChangesetListView(); $change_list->setChangesets($changesets); + $change_list->setRenderingReferences($references); $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/'); // TODO: This is pretty awkward, unify the CSS between Diffusion and diff --git a/src/applications/diffusion/controller/diff/DiffusionDiffController.php b/src/applications/diffusion/controller/diff/DiffusionDiffController.php index eea585324b..d83bb90202 100644 --- a/src/applications/diffusion/controller/diff/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/diff/DiffusionDiffController.php @@ -20,8 +20,8 @@ class DiffusionDiffController extends DiffusionController { public function willProcessRequest(array $data) { $request = $this->getRequest(); - if ($request->getStr('id')) { - $parts = explode(';', $request->getStr('id')); + if ($request->getStr('ref')) { + $parts = explode(';', $request->getStr('ref')); $data['path'] = idx($parts, 0); $data['commit'] = idx($parts, 1); } @@ -43,6 +43,7 @@ class DiffusionDiffController extends DiffusionController { $parser = new DifferentialChangesetParser(); $parser->setChangeset($changeset); + $parser->setRenderingReference($diff_query->getRenderingReference()); $parser->setWhitespaceMode( DifferentialChangesetParser::WHITESPACE_SHOW_ALL); diff --git a/src/applications/diffusion/controller/lastmodified/DiffusionLastModifiedController.php b/src/applications/diffusion/controller/lastmodified/DiffusionLastModifiedController.php index 04a6381fc6..1e64fd66eb 100644 --- a/src/applications/diffusion/controller/lastmodified/DiffusionLastModifiedController.php +++ b/src/applications/diffusion/controller/lastmodified/DiffusionLastModifiedController.php @@ -27,7 +27,7 @@ class DiffusionLastModifiedController extends DiffusionController { list($commit, $commit_data) = $modified_query->loadLastModification(); $phids = array(); - if ($commit_data->getCommitDetail('authorPHID')) { + if ($commit_data && $commit_data->getCommitDetail('authorPHID')) { $phids = array($commit_data->getCommitDetail('authorPHID')); } diff --git a/src/applications/diffusion/query/diff/base/DiffusionDiffQuery.php b/src/applications/diffusion/query/diff/base/DiffusionDiffQuery.php index 275a08e915..d0eaebf4cc 100644 --- a/src/applications/diffusion/query/diff/base/DiffusionDiffQuery.php +++ b/src/applications/diffusion/query/diff/base/DiffusionDiffQuery.php @@ -19,6 +19,7 @@ abstract class DiffusionDiffQuery { private $request; + protected $renderingReference; final private function __construct() { // @@ -52,6 +53,10 @@ abstract class DiffusionDiffQuery { return $this->request; } + final public function getRenderingReference() { + return $this->renderingReference; + } + final public function loadChangeset() { return $this->executeQuery(); } diff --git a/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php b/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php index 53fbf740b7..2ada296005 100644 --- a/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php +++ b/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php @@ -59,13 +59,11 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery { $changesets = $diff->getChangesets(); $changeset = reset($changesets); - $id = + $this->renderingReference = $drequest->getBranchURIComponent($drequest->getBranch()). $drequest->getPath().';'. $drequest->getCommit(); - $changeset->setID($id); - return $changeset; } diff --git a/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php b/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php index 8dca68469e..afe006935c 100644 --- a/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php +++ b/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php @@ -125,8 +125,7 @@ final class DiffusionSvnDiffQuery extends DiffusionDiffQuery { $changesets = $diff->getChangesets(); $changeset = reset($changesets); - $reference = $drequest->getPath().';'.$drequest->getCommit(); - $changeset->setRenderingReference($reference); + $this->renderingReference = $drequest->getPath().';'.$drequest->getCommit(); return $changeset; } diff --git a/webroot/rsrc/js/application/differential/behavior-populate.js b/webroot/rsrc/js/application/differential/behavior-populate.js index 604f914d91..3ba5a0649c 100644 --- a/webroot/rsrc/js/application/differential/behavior-populate.js +++ b/webroot/rsrc/js/application/differential/behavior-populate.js @@ -15,8 +15,7 @@ JX.behavior('differential-populate', function(config) { for (var k in config.registry) { new JX.Request(config.uri, JX.bind(null, onresponse, k)) .setData({ - id: config.registry[k][0], - vs: config.registry[k][1], + ref : config.registry[k], whitespace: config.whitespace }) .send();