From c35cafda2c2f16fe629e07238a9e568ea799cd74 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 8 Mar 2015 04:10:11 -0700 Subject: [PATCH] Use ChangesetListView on Differential standalone view Summary: Fixes T4452. Ref T2009. There's a hierarchy of changeset rendering power: only low-level calls, use of ChangesetDetailView, then use of ChangesetListView (a list of DetailViews). Prior to work here, the various changeset rendering controllers got their hands dirty to varying degrees, with some using only the lowest-level rendering pipeline: - Phriction: no view (lowest level) - Diffusion: DetailView - Differential Changeset: DetailView - Differential Diff: ListView - Differential Revision: ListView I brought Phriction up to use DetailView, but want to bring everything all the way up to use ListView. Each composition layer adds more features to diff browsing. In particular, this change enables "Highlight As", switching 1up vs 2up, adding inlines, etc., on the standalone view. Test Plan: - Viewed a changeset standalone. Could change highlighting, switch 1up vs 2up, add and edit inlines, etc. - Viewed a revision; no behavioral changes. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4452, T2009 Differential Revision: https://secure.phabricator.com/D12012 --- .../DifferentialChangesetViewController.php | 81 +++++++------------ .../parser/DifferentialChangesetParser.php | 19 +++++ .../view/DifferentialChangesetDetailView.php | 11 +++ .../view/DifferentialChangesetListView.php | 64 +++++++++------ .../differential/ChangesetViewManager.js | 1 + 5 files changed, 103 insertions(+), 73 deletions(-) diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 060892436e..aab16791ca 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -147,13 +147,13 @@ final class DifferentialChangesetViewController extends DifferentialController { list($range_s, $range_e, $mask) = DifferentialChangesetParser::parseRangeSpecification($spec); - $parser = new DifferentialChangesetParser(); - $parser->setCoverage($coverage); - $parser->setChangeset($changeset); - $parser->setRenderingReference($rendering_reference); - $parser->setRenderCacheKey($render_cache_key); - $parser->setRightSideCommentMapping($right_source, $right_new); - $parser->setLeftSideCommentMapping($left_source, $left_new); + $parser = id(new DifferentialChangesetParser()) + ->setCoverage($coverage) + ->setChangeset($changeset) + ->setRenderingReference($rendering_reference) + ->setRenderCacheKey($render_cache_key) + ->setRightSideCommentMapping($right_source, $right_new) + ->setLeftSideCommentMapping($left_source, $left_new); $parser->readParametersFromRequest($request); @@ -200,66 +200,51 @@ final class DifferentialChangesetViewController extends DifferentialController { } $engine->process(); - $parser->setMarkupEngine($engine); - $parser->setUser($request->getUser()); + + $parser + ->setUser($request->getUser()) + ->setMarkupEngine($engine) + ->setShowEditAndReplyLinks(true) + ->setRange($range_s, $range_e) + ->setMask($mask); if ($request->isAjax()) { - $parser->setShowEditAndReplyLinks(true); - } else { - $parser->setShowEditAndReplyLinks(false); - } + $mcov = $parser->renderModifiedCoverage(); - $output = $parser->render($range_s, $range_e, $mask); - - $mcov = $parser->renderModifiedCoverage(); - - if ($request->isAjax()) { $coverage = array( 'differential-mcoverage-'.md5($changeset->getFilename()) => $mcov, ); return id(new PhabricatorChangesetResponse()) - ->setRenderedChangeset($output) + ->setRenderedChangeset($parser->renderChangeset()) ->setCoverage($coverage); } - // TODO: [HTML] Clean up DifferentialChangesetParser output, but it's - // undergoing like six kinds of refactoring anyway. - $output = phutil_safe_html($output); + $diff = $changeset->getDiff(); - $detail = id(new DifferentialChangesetDetailView()) + $detail = id(new DifferentialChangesetListView()) ->setUser($this->getViewer()) - ->setChangeset($changeset) - ->setRenderingRef($rendering_reference) + ->setChangesets(array($changeset)) + ->setVisibleChangesets(array($changeset)) + ->setRenderingReferences(array($rendering_reference)) ->setRenderURI('/differential/changeset/') - ->setRenderer($parser->getRenderer()->getRendererKey()) - ->appendChild($output) - ->setVsChangesetID($left_source); + ->setDiff($diff) + ->setTitle(pht('Standalone View')) + ->setParser($parser); - Javelin::initBehavior('differential-populate', array( - 'changesetViewIDs' => array($detail->getID()), - )); - - Javelin::initBehavior('differential-comment-jump', array()); - - $panel = new DifferentialPrimaryPaneView(); - $panel->appendChild( - phutil_tag( - 'div', - array( - 'class' => 'differential-review-stage', - 'id' => 'differential-review-stage', - ), - $detail->render())); + $revision_id = $diff->getRevisionID(); + if ($revision_id) { + $detail->setInlineCommentControllerURI( + '/differential/comment/inline/edit/'.$revision_id.'/'); + } $crumbs = $this->buildApplicationCrumbs(); - $revision_id = $changeset->getDiff()->getRevisionID(); if ($revision_id) { $crumbs->addTextCrumb('D'.$revision_id, '/D'.$revision_id); } - $diff_id = $changeset->getDiff()->getID(); + $diff_id = $diff->getID(); if ($diff_id) { $crumbs->addTextCrumb( pht('Diff %d', $diff_id), @@ -268,14 +253,10 @@ final class DifferentialChangesetViewController extends DifferentialController { $crumbs->addTextCrumb($changeset->getDisplayFilename()); - $box = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Standalone View')) - ->appendChild($panel); - return $this->buildApplicationPage( array( $crumbs, - $box, + $detail, ), array( 'title' => pht('Changeset View'), diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index cd689153f0..39bb8ce80b 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -45,6 +45,25 @@ final class DifferentialChangesetParser { private $highlightAs; private $showEditAndReplyLinks = true; + private $rangeStart; + private $rangeEnd; + private $mask; + + public function setRange($start, $end) { + $this->rangeStart = $start; + $this->rangeEnd = $end; + return $this; + } + + public function setMask(array $mask) { + $this->mask = $mask; + return $this; + } + + public function renderChangeset() { + return $this->render($this->rangeStart, $this->rangeEnd, $this->mask); + } + public function setShowEditAndReplyLinks($bool) { $this->showEditAndReplyLinks = $bool; return $this; diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php index 7723a7be7d..a6dc8a5f3a 100644 --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/DifferentialChangesetDetailView.php @@ -12,6 +12,7 @@ final class DifferentialChangesetDetailView extends AphrontView { private $whitespace; private $renderingRef; private $autoload; + private $loaded; private $renderer; public function setAutoload($autoload) { @@ -23,6 +24,15 @@ final class DifferentialChangesetDetailView extends AphrontView { return $this->autoload; } + public function setLoaded($loaded) { + $this->loaded = $loaded; + return $this; + } + + public function getLoaded() { + return $this->loaded; + } + public function setRenderingRef($rendering_ref) { $this->renderingRef = $rendering_ref; return $this; @@ -213,6 +223,7 @@ final class DifferentialChangesetDetailView extends AphrontView { 'renderer' => $this->getRenderer(), 'ref' => $this->getRenderingRef(), 'autoload' => $this->getAutoload(), + 'loaded' => $this->getLoaded(), ), 'class' => $class, 'id' => $id, diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 077cf688ca..d965156a85 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -20,6 +20,16 @@ final class DifferentialChangesetListView extends AphrontView { private $vsMap = array(); private $title; + private $parser; + + public function setParser(DifferentialChangesetParser $parser) { + $this->parser = $parser; + return $this; + } + + public function getParser() { + return $this->parser; + } public function setTitle($title) { $this->title = $title; @@ -168,38 +178,46 @@ final class DifferentialChangesetListView extends AphrontView { $detail->setVsChangesetID(idx($this->vsMap, $changeset->getID())); $detail->setEditable(true); $detail->setRenderingRef($ref); - $detail->setAutoload(isset($this->visibleChangesets[$key])); $detail->setRenderURI($this->renderURI); $detail->setWhitespace($this->whitespace); $detail->setRenderer($renderer); - if (isset($this->visibleChangesets[$key])) { - $load = 'Loading...'; + if ($this->getParser()) { + $detail->appendChild($this->getParser()->renderChangeset()); + $detail->setLoaded(true); } else { - $load = javelin_tag( - 'a', - array( - 'class' => 'button grey', - 'href' => '#'.$uniq_id, - 'sigil' => 'differential-load', - 'meta' => array( - 'id' => $detail->getID(), - 'kill' => true, + $detail->setAutoload(isset($this->visibleChangesets[$key])); + if (isset($this->visibleChangesets[$key])) { + $load = 'Loading...'; + } else { + $load = javelin_tag( + 'a', + array( + 'class' => 'button grey', + 'href' => '#'.$uniq_id, + 'sigil' => 'differential-load', + 'meta' => array( + 'id' => $detail->getID(), + 'kill' => true, + ), + 'mustcapture' => true, ), - 'mustcapture' => true, - ), - pht('Load File')); + pht('Load File')); + } + $detail->appendChild( + phutil_tag( + 'div', + array( + 'id' => $uniq_id, + ), + phutil_tag( + 'div', + array('class' => 'differential-loading'), + $load))); } - $detail->appendChild( - phutil_tag( - 'div', - array( - 'id' => $uniq_id, - ), - phutil_tag('div', array('class' => 'differential-loading'), $load))); - $output[] = $detail->render(); + $output[] = $detail->render(); $ids[] = $detail->getID(); } diff --git a/webroot/rsrc/js/application/differential/ChangesetViewManager.js b/webroot/rsrc/js/application/differential/ChangesetViewManager.js index 36f097cf49..f80883f910 100644 --- a/webroot/rsrc/js/application/differential/ChangesetViewManager.js +++ b/webroot/rsrc/js/application/differential/ChangesetViewManager.js @@ -23,6 +23,7 @@ JX.install('ChangesetViewManager', { this._renderer = data.renderer; this._highlight = data.highlight; this._encoding = data.encoding; + this._loaded = data.loaded; }, members: {